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

[sundeep subbaraya]

H Peter,

On Mon, Sep 18, 2017 at 4:05 PM, Peter Maydell <address@hidden>
wrote:

> 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;
> }
>
> Ok will change like this.

Thanks,
Sundeep


> thanks
> -- PMM
>