Re: [Qemu-arm] [Qemu devel v9 PATCH 2/5] msf2: Microsemi Smartfusion2 System Register block

[Peter Maydell]

On 18 September 2017 at 11:17, sundeep subbaraya <address@hidden> wrote:
> Hi Philippe,
>
> On Mon, Sep 18, 2017 at 6:31 AM, Philippe Mathieu-Daudé <address@hidden>
> wrote:
>>
>> Hi Sundeep, Peter,
>>
>>
>> On 09/15/2017 01:59 PM, Subbaraya Sundeep wrote:
>>>
>>> Added Sytem register block of Smartfusion2.
>>> This block has PLL registers which are accessed by guest.
>>>
>>> Signed-off-by: Subbaraya Sundeep <address@hidden>
>>> Reviewed-by: Alistair Francis <address@hidden>
>>> ---
>>>   hw/misc/Makefile.objs         |   1 +
>>>   hw/misc/msf2-sysreg.c         | 168
>>> ++++++++++++++++++++++++++++++++++++++++++
>>>   hw/misc/trace-events          |   5 ++
>>>   include/hw/misc/msf2-sysreg.h |  77 +++++++++++++++++++
>>>   4 files changed, 251 insertions(+)
>>>   create mode 100644 hw/misc/msf2-sysreg.c
>>>   create mode 100644 include/hw/misc/msf2-sysreg.h
>>>
>>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>>> index 29fb922..e8f0a02 100644
>>> --- a/hw/misc/Makefile.objs
>>> +++ b/hw/misc/Makefile.objs
>>> @@ -59,3 +59,4 @@ obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
>>>   obj-$(CONFIG_AUX) += auxbus.o
>>>   obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
>>>   obj-y += mmio_interface.o
>>> +obj-$(CONFIG_MSF2) += msf2-sysreg.o
>>> diff --git a/hw/misc/msf2-sysreg.c b/hw/misc/msf2-sysreg.c
>>> new file mode 100644
>>> index 0000000..dc3597b
>>> --- /dev/null
>>> +++ b/hw/misc/msf2-sysreg.c
>>> @@ -0,0 +1,168 @@
>>> +/*
>>> + * System Register block model of Microsemi SmartFusion2.
>>> + *
>>> + * Copyright (c) 2017 Subbaraya Sundeep <address@hidden>
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * as published by the Free Software Foundation; either version
>>> + * 2 of the License, or (at your option) any later version.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> along
>>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qemu/log.h"
>>> +#include "hw/misc/msf2-sysreg.h"
>>> +#include "trace.h"
>>> +
>>> +static inline int msf2_divbits(uint32_t div)
>>> +{
>>> +    int ret = 0;
>>> +
>>> +    switch (div) {
>>> +    case 1:
>>> +        ret = 0;
>>> +        break;
>>> +    case 2:
>>> +        ret = 1;
>>> +        break;
>>> +    case 4:
>>> +        ret = 2;
>>> +        break;
>>> +    case 8:
>>> +        ret = 4;
>>> +        break;
>>> +    case 16:
>>> +        ret = 5;
>>> +        break;
>>> +    case 32:
>>> +        ret = 6;
>>> +        break;
>>> +    default:
>>> +        break;
>>
>>
>> eh?
>>
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static void msf2_sysreg_reset(DeviceState *d)
>>> +{
>>> +    MSF2SysregState *s = MSF2_SYSREG(d);
>>> +
>>> +    s->regs[MSSDDR_PLL_STATUS_LOW_CR] = 0x021A2358;
>>> +    s->regs[MSSDDR_PLL_STATUS] = 0x3;
>>> +    s->regs[MSSDDR_FACC1_CR] = msf2_divbits(s->apb0div) << 5 |
>>
>>
>> ctz32(s->apb0div) << 5 ...
>
>
> I will modify like below:
> div0 = s->apb0div < 8 ? ctz32(s->apb0div) : ctz32(s->apb0div) + 1;
> div1 = s->apb1div < 8 ? ctz32(s->apb1div) : ctz32(s->apb1div) + 1;

I think you should keep an msf2_divbits() function for that,
even if it's only

static inline int msf2_divbits(uint32_t div)
{
    int r = ctz32(div);

    return (div < 8) ? r : r + 1;
}

thanks
-- PMM