diff mbox

[Qemu,devel,v8,2/5] msf2: Microsemi Smartfusion2 System Register block

Message ID 1504812251-23438-3-git-send-email-sundeep.lkml@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

sundeep subbaraya Sept. 7, 2017, 7:24 p.m. UTC
Added Sytem register block of Smartfusion2.
This block has PLL registers which are accessed by guest.

Signed-off-by: Subbaraya Sundeep <sundeep.lkml@gmail.com>
Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
---
 hw/misc/Makefile.objs         |   1 +
 hw/misc/msf2-sysreg.c         | 195 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/misc/msf2-sysreg.h |  78 +++++++++++++++++
 3 files changed, 274 insertions(+)
 create mode 100644 hw/misc/msf2-sysreg.c
 create mode 100644 include/hw/misc/msf2-sysreg.h

Comments

Philippe Mathieu-Daudé Sept. 14, 2017, 4:36 a.m. UTC | #1
Hi Sundeep,

On 09/07/2017 04:24 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 <sundeep.lkml@gmail.com>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>   hw/misc/Makefile.objs         |   1 +
>   hw/misc/msf2-sysreg.c         | 195 ++++++++++++++++++++++++++++++++++++++++++
>   include/hw/misc/msf2-sysreg.h |  78 +++++++++++++++++
>   3 files changed, 274 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..40d603d
> --- /dev/null
> +++ b/hw/misc/msf2-sysreg.c
> @@ -0,0 +1,195 @@
> +/*
> + * System Register block model of Microsemi SmartFusion2.
> + *
> + * Copyright (c) 2017 Subbaraya Sundeep <sundeep.lkml@gmail.com>
> + *
> + * 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"
> +
> +#ifndef MSF2_SYSREG_ERR_DEBUG
> +#define MSF2_SYSREG_ERR_DEBUG  0
> +#endif
> +
> +#define DB_PRINT_L(lvl, fmt, args...) do { \
> +    if (MSF2_SYSREG_ERR_DEBUG >= lvl) { \
> +        qemu_log("%s: " fmt "\n", __func__, ## args); \
> +    } \
> +} while (0);
> +
> +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
> +
> +static inline int msf2_divbits(uint32_t div)

Please directly use ctz32() instead of msf2_divbits()

> +{
> +    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;
> +    }
> +
> +    return ret;
> +}
> +
> +static void msf2_sysreg_reset(DeviceState *d)
> +{
> +    MSF2SysregState *s = MSF2_SYSREG(d);
> +
> +    DB_PRINT("RESET");
> +
> +    s->regs[MSSDDR_PLL_STATUS_LOW_CR] = 0x021A2358;
> +    s->regs[MSSDDR_PLL_STATUS] = 0x3;
> +    s->regs[MSSDDR_FACC1_CR] = msf2_divbits(s->apb0div) << 5 |
> +                               msf2_divbits(s->apb1div) << 2;
> +}
> +
> +static uint64_t msf2_sysreg_read(void *opaque, hwaddr offset,
> +    unsigned size)
> +{
> +    MSF2SysregState *s = opaque;
> +    uint32_t ret = 0;
> +
> +    offset >>= 2;
> +    if (offset < ARRAY_SIZE(s->regs)) {

This comment is controversial, I'll let Peter nod.

The SYSREG behaves differently regarding which bus access it (CPU, AHB).
You are implementing CPU access to the SYSREG, the registers have 
different permissions when accessed by the CPU. (see the SmartFusion2 
User Guide: Table 21-1 "Register Types" and Table 21-2 "Register Map").

I'd think of this stub:

switch(reg) {
case register_supported1:
case register_supported2:
case register_supported3:
            ret = s->regs[offset];
      	   trace_msf2_sysreg_read(offset, ret);
            break;
case RO-U:
            qemu_log_mask(LOG_GUEST_ERROR, "Illegal AHB access 0x%08"...
            break;
case W1P:
            qemu_log_mask(LOG_GUEST_ERROR, "Illegal read access ...
            break;
case RW:
case RW-P:
case RO:
case RO-P:
default:
            ret = s->regs[offset];
            qemu_log_mask(LOG_UNIMP, "...
            break;
}

Anyway keep this comment for further improvements, it's probably not 
useful for your use case.

> +        ret = s->regs[offset];
> +        DB_PRINT("addr: 0x%08" HWADDR_PRIx " data: 0x%08" PRIx32,
> +                    offset << 2, ret);

Can you replace DB_PRINT by traces? such:

	   trace_msf2_sysreg_read(...);

> +    } else {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                    "%s: Bad offset 0x%08" HWADDR_PRIx "\n", __func__,
> +                    offset << 2);
> +    }
> +
> +    return ret;
> +}
> +
> +static void msf2_sysreg_write(void *opaque, hwaddr offset,
> +                          uint64_t val, unsigned size)
> +{
> +    MSF2SysregState *s = opaque;
> +    uint32_t newval = val;
> +
> +    DB_PRINT("addr: 0x%08" HWADDR_PRIx " data: 0x%08" PRIx64,
> +            offset, val);
> +
> +    offset >>= 2;
> +
> +    switch (offset) {
> +    case MSSDDR_PLL_STATUS:
	   trace_msf2_sysreg_write_pll_status();

> +        break;
> +
> +    case ESRAM_CR:
> +        if (newval != s->regs[ESRAM_CR]) {
> +            qemu_log_mask(LOG_GUEST_ERROR,

Since this isn't a guest error, use LOG_UNIMP instead.

> +                       TYPE_MSF2_SYSREG": eSRAM remapping not supported\n");
> +        }
> +        break;
> +
> +    case DDR_CR:
> +        if (newval != s->regs[DDR_CR]) {
> +            qemu_log_mask(LOG_GUEST_ERROR,

LOG_UNIMP

> +                       TYPE_MSF2_SYSREG": DDR remapping not supported\n");
> +        }
> +        break;
> +
> +    case ENVM_REMAP_BASE_CR:
> +        if (newval != s->regs[ENVM_REMAP_BASE_CR]) {
> +            qemu_log_mask(LOG_GUEST_ERROR,

LOG_UNIMP

> +                       TYPE_MSF2_SYSREG": eNVM remapping not supported\n");
> +        }
> +        break;
> +

Or shorter:

     case ESRAM_CR:
     case DDR_CR:
     case ENVM_REMAP_BASE_CR:
          oldval = s->regs[offset];
          if (oldval ^ newval) {
              qemu_log_mask(LOG_UNIMP,
                         TYPE_MSF2_SYSREG": remapping not supported\n");
          }
          break;


> +    default:
> +        if (offset < ARRAY_SIZE(s->regs)) {

	   trace_msf2_sysreg_write(offset, val, s->regs[offset]);

> +            s->regs[offset] = newval;
> +        } else {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                        "%s: Bad offset 0x%08" HWADDR_PRIx "\n", __func__,
> +                        offset << 2);
> +        }
> +        break;
> +    }
> +}

Good work, almost there, next respin should be ready for Peter to take 
in arm-next :)

Regards,

Phil.
Peter Maydell Sept. 14, 2017, 1:13 p.m. UTC | #2
On 14 September 2017 at 05:36, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 09/07/2017 04:24 PM, Subbaraya Sundeep wrote:
>> +static uint64_t msf2_sysreg_read(void *opaque, hwaddr offset,
>> +    unsigned size)
>> +{
>> +    MSF2SysregState *s = opaque;
>> +    uint32_t ret = 0;
>> +
>> +    offset >>= 2;
>> +    if (offset < ARRAY_SIZE(s->regs)) {
>
>
> This comment is controversial, I'll let Peter nod.
>
> The SYSREG behaves differently regarding which bus access it (CPU, AHB).
> You are implementing CPU access to the SYSREG, the registers have different
> permissions when accessed by the CPU. (see the SmartFusion2 User Guide:
> Table 21-1 "Register Types" and Table 21-2 "Register Map").
>
> I'd think of this stub:
>
> switch(reg) {
> case register_supported1:
> case register_supported2:
> case register_supported3:
>            ret = s->regs[offset];
>            trace_msf2_sysreg_read(offset, ret);
>            break;
> case RO-U:
>            qemu_log_mask(LOG_GUEST_ERROR, "Illegal AHB access 0x%08"...
>            break;
> case W1P:
>            qemu_log_mask(LOG_GUEST_ERROR, "Illegal read access ...
>            break;
> case RW:
> case RW-P:
> case RO:
> case RO-P:
> default:
>            ret = s->regs[offset];
>            qemu_log_mask(LOG_UNIMP, "...
>            break;
> }

This doesn't look entirely right, since this is the read interface.
Shouldn't we be allowing pretty much all of these register types
except maybe W1P to do a read?

On the other hand, in the write function we should probably not allow
writes to registers documented as read only.

thanks
-- PMM
sundeep subbaraya Sept. 14, 2017, 3:30 p.m. UTC | #3
Hi Philippe,

On Thu, Sep 14, 2017 at 6:43 PM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 14 September 2017 at 05:36, Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
> > On 09/07/2017 04:24 PM, Subbaraya Sundeep wrote:
> >> +static uint64_t msf2_sysreg_read(void *opaque, hwaddr offset,
> >> +    unsigned size)
> >> +{
> >> +    MSF2SysregState *s = opaque;
> >> +    uint32_t ret = 0;
> >> +
> >> +    offset >>= 2;
> >> +    if (offset < ARRAY_SIZE(s->regs)) {
> >
> >
> > This comment is controversial, I'll let Peter nod.
> >
> > The SYSREG behaves differently regarding which bus access it (CPU, AHB).
> > You are implementing CPU access to the SYSREG, the registers have
> different
> > permissions when accessed by the CPU. (see the SmartFusion2 User Guide:
> > Table 21-1 "Register Types" and Table 21-2 "Register Map").
>

CPU is also one of the bus masters in AHB matrix (Fig 6.1 in page 248).

>
> > I'd think of this stub:
> >
> > switch(reg) {
> > case register_supported1:
> > case register_supported2:
> > case register_supported3:
> >            ret = s->regs[offset];
> >            trace_msf2_sysreg_read(offset, ret);
> >            break;
> > case RO-U:
> >            qemu_log_mask(LOG_GUEST_ERROR, "Illegal AHB access 0x%08"...
> >            break;
> > case W1P:
> >            qemu_log_mask(LOG_GUEST_ERROR, "Illegal read access ...
> >            break;
> > case RW:
> > case RW-P:
> > case RO:
> > case RO-P:
> > default:
> >            ret = s->regs[offset];
> >            qemu_log_mask(LOG_UNIMP, "...
> >            break;
> > }
>

This sounds good to me and will fix later by rearranging registers in enum
sorted
based on type (RO, RW, etc.,) and use those ranges in switch case.

>
> This doesn't look entirely right, since this is the read interface.
> Shouldn't we be allowing pretty much all of these register types
> except maybe W1P to do a read?
>

Peter, some of the registers are not allowed to access by CPU and are only
set during
device programming. For those registers Philippe is suggesting to log
"Illegal AHB access"
when guest is trying to read/write.

Thanks,
Sundeep


>
> On the other hand, in the write function we should probably not allow
> writes to registers documented as read only.
>
> thanks
> -- PMM
>
sundeep subbaraya Sept. 14, 2017, 3:34 p.m. UTC | #4
Hi Philippe,

On Thu, Sep 14, 2017 at 10:06 AM, Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Hi Sundeep,
>
>
> On 09/07/2017 04:24 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 <sundeep.lkml@gmail.com>
>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>   hw/misc/Makefile.objs         |   1 +
>>   hw/misc/msf2-sysreg.c         | 195 ++++++++++++++++++++++++++++++
>> ++++++++++++
>>   include/hw/misc/msf2-sysreg.h |  78 +++++++++++++++++
>>   3 files changed, 274 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..40d603d
>> --- /dev/null
>> +++ b/hw/misc/msf2-sysreg.c
>> @@ -0,0 +1,195 @@
>> +/*
>> + * System Register block model of Microsemi SmartFusion2.
>> + *
>> + * Copyright (c) 2017 Subbaraya Sundeep <sundeep.lkml@gmail.com>
>> + *
>> + * 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"
>> +
>> +#ifndef MSF2_SYSREG_ERR_DEBUG
>> +#define MSF2_SYSREG_ERR_DEBUG  0
>> +#endif
>> +
>> +#define DB_PRINT_L(lvl, fmt, args...) do { \
>> +    if (MSF2_SYSREG_ERR_DEBUG >= lvl) { \
>> +        qemu_log("%s: " fmt "\n", __func__, ## args); \
>> +    } \
>> +} while (0);
>> +
>> +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
>> +
>> +static inline int msf2_divbits(uint32_t div)
>>
>
> Please directly use ctz32() instead of msf2_divbits()
>
> ctz32() will not work for input 8,16,32 so need to use msf2_divbits().

>
> +{
>> +    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;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void msf2_sysreg_reset(DeviceState *d)
>> +{
>> +    MSF2SysregState *s = MSF2_SYSREG(d);
>> +
>> +    DB_PRINT("RESET");
>> +
>> +    s->regs[MSSDDR_PLL_STATUS_LOW_CR] = 0x021A2358;
>> +    s->regs[MSSDDR_PLL_STATUS] = 0x3;
>> +    s->regs[MSSDDR_FACC1_CR] = msf2_divbits(s->apb0div) << 5 |
>> +                               msf2_divbits(s->apb1div) << 2;
>> +}
>> +
>> +static uint64_t msf2_sysreg_read(void *opaque, hwaddr offset,
>> +    unsigned size)
>> +{
>> +    MSF2SysregState *s = opaque;
>> +    uint32_t ret = 0;
>> +
>> +    offset >>= 2;
>> +    if (offset < ARRAY_SIZE(s->regs)) {
>>
>
> This comment is controversial, I'll let Peter nod.
>
> The SYSREG behaves differently regarding which bus access it (CPU, AHB).
> You are implementing CPU access to the SYSREG, the registers have
> different permissions when accessed by the CPU. (see the SmartFusion2 User
> Guide: Table 21-1 "Register Types" and Table 21-2 "Register Map").
>
> I'd think of this stub:
>
> switch(reg) {
> case register_supported1:
> case register_supported2:
> case register_supported3:
>            ret = s->regs[offset];
>            trace_msf2_sysreg_read(offset, ret);
>            break;
> case RO-U:
>            qemu_log_mask(LOG_GUEST_ERROR, "Illegal AHB access 0x%08"...
>            break;
> case W1P:
>            qemu_log_mask(LOG_GUEST_ERROR, "Illegal read access ...
>            break;
> case RW:
> case RW-P:
> case RO:
> case RO-P:
> default:
>            ret = s->regs[offset];
>            qemu_log_mask(LOG_UNIMP, "...
>            break;
> }
>
> Anyway keep this comment for further improvements, it's probably not
> useful for your use case.


Sure will remember this comment and fix it later.

>
>
> +        ret = s->regs[offset];
>> +        DB_PRINT("addr: 0x%08" HWADDR_PRIx " data: 0x%08" PRIx32,
>> +                    offset << 2, ret);
>>
>
> Can you replace DB_PRINT by traces? such:
>
>            trace_msf2_sysreg_read(...);


Ok

>
>
> +    } else {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                    "%s: Bad offset 0x%08" HWADDR_PRIx "\n", __func__,
>> +                    offset << 2);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void msf2_sysreg_write(void *opaque, hwaddr offset,
>> +                          uint64_t val, unsigned size)
>> +{
>> +    MSF2SysregState *s = opaque;
>> +    uint32_t newval = val;
>> +
>> +    DB_PRINT("addr: 0x%08" HWADDR_PRIx " data: 0x%08" PRIx64,
>> +            offset, val);
>> +
>> +    offset >>= 2;
>> +
>> +    switch (offset) {
>> +    case MSSDDR_PLL_STATUS:
>>
>            trace_msf2_sysreg_write_pll_status();
>
> +        break;
>> +
>> +    case ESRAM_CR:
>> +        if (newval != s->regs[ESRAM_CR]) {
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>>
>
> Since this isn't a guest error, use LOG_UNIMP instead.
>
> Ok

> +                       TYPE_MSF2_SYSREG": eSRAM remapping not
>> supported\n");
>> +        }
>> +        break;
>> +
>> +    case DDR_CR:
>> +        if (newval != s->regs[DDR_CR]) {
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>>
>
> LOG_UNIMP
>
> Ok

> +                       TYPE_MSF2_SYSREG": DDR remapping not supported\n");
>> +        }
>> +        break;
>> +
>> +    case ENVM_REMAP_BASE_CR:
>> +        if (newval != s->regs[ENVM_REMAP_BASE_CR]) {
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>>
>
> LOG_UNIMP
>
> Ok

> +                       TYPE_MSF2_SYSREG": eNVM remapping not
>> supported\n");
>> +        }
>> +        break;
>> +
>>
>
> Or shorter:
>
>     case ESRAM_CR:
>     case DDR_CR:
>     case ENVM_REMAP_BASE_CR:
>          oldval = s->regs[offset];
>          if (oldval ^ newval) {
>              qemu_log_mask(LOG_UNIMP,
>                         TYPE_MSF2_SYSREG": remapping not supported\n");
>          }
>          break;
>
> Ok

>
> +    default:
>> +        if (offset < ARRAY_SIZE(s->regs)) {
>>
>
>            trace_msf2_sysreg_write(offset, val, s->regs[offset]);
>
> +            s->regs[offset] = newval;
>> +        } else {
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                        "%s: Bad offset 0x%08" HWADDR_PRIx "\n",
>> __func__,
>> +                        offset << 2);
>> +        }
>> +        break;
>> +    }
>> +}
>>
>
> Good work, almost there, next respin should be ready for Peter to take in
> arm-next :)
>
> Thank you,
Sundeep


> Regards,
>
> Phil.
>
Philippe Mathieu-Daudé Sept. 14, 2017, 6:05 p.m. UTC | #5
>>>> +static uint64_t msf2_sysreg_read(void *opaque, hwaddr offset,
>>>> +    unsigned size)
>>>> +{
>>>> +    MSF2SysregState *s = opaque;
>>>> +    uint32_t ret = 0;
>>>> +
>>>> +    offset >>= 2;
>>>> +    if (offset < ARRAY_SIZE(s->regs)) {
>>>
>>>
>>> This comment is controversial, I'll let Peter nod.
>>>
>>> The SYSREG behaves differently regarding which bus access it (CPU, AHB).
>>> You are implementing CPU access to the SYSREG, the registers have
>> different
>>> permissions when accessed by the CPU. (see the SmartFusion2 User Guide:
>>> Table 21-1 "Register Types" and Table 21-2 "Register Map").
>>
> 
> CPU is also one of the bus masters in AHB matrix (Fig 6.1 in page 248).

I was worried about Fabric access but Peter remembered me QEMU doesn't 
model it ;)

>>> I'd think of this stub:
>>>
>>> switch(reg) {
>>> case register_supported1:
>>> case register_supported2:
>>> case register_supported3:
>>>             ret = s->regs[offset];
>>>             trace_msf2_sysreg_read(offset, ret);
>>>             break;
>>> case RO-U:
>>>             qemu_log_mask(LOG_GUEST_ERROR, "Illegal AHB access 0x%08"...
>>>             break;
>>> case W1P:
>>>             qemu_log_mask(LOG_GUEST_ERROR, "Illegal read access ...
>>>             break;
>>> case RW:
>>> case RW-P:
>>> case RO:
>>> case RO-P:
>>> default:
>>>             ret = s->regs[offset];
>>>             qemu_log_mask(LOG_UNIMP, "...
>>>             break;
>>> }
>>
> 
> This sounds good to me and will fix later by rearranging registers in enum
> sorted
> based on type (RO, RW, etc.,) and use those ranges in switch case.

The Register API (hw/register.h) is helpful to check such properties but 
might turn your code harder to read.

>>
>> This doesn't look entirely right, since this is the read interface.
>> Shouldn't we be allowing pretty much all of these register types
>> except maybe W1P to do a read?
>>
> 
> Peter, some of the registers are not allowed to access by CPU and are only
> set during
> device programming. For those registers Philippe is suggesting to log
> "Illegal AHB access"
> when guest is trying to read/write.

I was worried about accessing those registers when the flash 
WriteProtect bit is set, however the eNVM flash library is not Open 
Source, it is unlikely a guest access those registers, and if it is the 
library then MicroSemi already tried it on real hardware.
There is probably no need to worry about "Illegal AHB access" :)

>> On the other hand, in the write function we should probably not allow
>> writes to registers documented as read only.

Surely.

trace_msf2_sysreg_write(offset, s->regs[offset], ret);

switch(reg) {
case register_supported1:
case register_supported2:
case register_supported3:
            s->regs[offset] = ret;
            break;
case RO:
case RO-P:
case RO-U:
            qemu_log_mask(LOG_GUEST_ERROR, "Illegal write access on RO...
            break;
default:
            qemu_log_mask(LOG_UNIMP, "...
            break;
}
Philippe Mathieu-Daudé Sept. 17, 2017, 11:57 p.m. UTC | #6
Hi Sundeep,

On 09/14/2017 01:36 AM, Philippe Mathieu-Daudé wrote:
> On 09/07/2017 04:24 PM, Subbaraya Sundeep wrote:
[...]
>> +static inline int msf2_divbits(uint32_t div)
> 
> Please directly use ctz32() instead of msf2_divbits()

It seems you missed this review comment in your v9.

> 
>> +{
>> +    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;
>> +    }
>> +
>> +    return ret;
>> +}
sundeep subbaraya Sept. 18, 2017, 6:29 a.m. UTC | #7
Hi Philippe,

On Mon, Sep 18, 2017 at 5:27 AM, Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Hi Sundeep,
>
> On 09/14/2017 01:36 AM, Philippe Mathieu-Daudé wrote:
>
>> On 09/07/2017 04:24 PM, Subbaraya Sundeep wrote:
>>
> [...]
>
>> +static inline int msf2_divbits(uint32_t div)
>>>
>>
>> Please directly use ctz32() instead of msf2_divbits()
>>
>
> It seems you missed this review comment in your v9.


ctz32(1) = 0
ctz32(2) = 1
ctz32(4) = 2
ctz32(8) = 3
ctz32(16) = 4
ctz32(32) = 5

but for inputs 8,16,32 output should be 4,5,6 so didn't use ctz32().
I replied to this comment in the same mail chain earlier. Please check.

Thanks,
Sundeep

>
>
>
>> +{
>>> +    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;
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>>
>>
diff mbox

Patch

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..40d603d
--- /dev/null
+++ b/hw/misc/msf2-sysreg.c
@@ -0,0 +1,195 @@ 
+/*
+ * System Register block model of Microsemi SmartFusion2.
+ *
+ * Copyright (c) 2017 Subbaraya Sundeep <sundeep.lkml@gmail.com>
+ *
+ * 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"
+
+#ifndef MSF2_SYSREG_ERR_DEBUG
+#define MSF2_SYSREG_ERR_DEBUG  0
+#endif
+
+#define DB_PRINT_L(lvl, fmt, args...) do { \
+    if (MSF2_SYSREG_ERR_DEBUG >= lvl) { \
+        qemu_log("%s: " fmt "\n", __func__, ## args); \
+    } \
+} while (0);
+
+#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
+
+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;
+    }
+
+    return ret;
+}
+
+static void msf2_sysreg_reset(DeviceState *d)
+{
+    MSF2SysregState *s = MSF2_SYSREG(d);
+
+    DB_PRINT("RESET");
+
+    s->regs[MSSDDR_PLL_STATUS_LOW_CR] = 0x021A2358;
+    s->regs[MSSDDR_PLL_STATUS] = 0x3;
+    s->regs[MSSDDR_FACC1_CR] = msf2_divbits(s->apb0div) << 5 |
+                               msf2_divbits(s->apb1div) << 2;
+}
+
+static uint64_t msf2_sysreg_read(void *opaque, hwaddr offset,
+    unsigned size)
+{
+    MSF2SysregState *s = opaque;
+    uint32_t ret = 0;
+
+    offset >>= 2;
+    if (offset < ARRAY_SIZE(s->regs)) {
+        ret = s->regs[offset];
+        DB_PRINT("addr: 0x%08" HWADDR_PRIx " data: 0x%08" PRIx32,
+                    offset << 2, ret);
+    } else {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                    "%s: Bad offset 0x%08" HWADDR_PRIx "\n", __func__,
+                    offset << 2);
+    }
+
+    return ret;
+}
+
+static void msf2_sysreg_write(void *opaque, hwaddr offset,
+                          uint64_t val, unsigned size)
+{
+    MSF2SysregState *s = opaque;
+    uint32_t newval = val;
+
+    DB_PRINT("addr: 0x%08" HWADDR_PRIx " data: 0x%08" PRIx64,
+            offset, val);
+
+    offset >>= 2;
+
+    switch (offset) {
+    case MSSDDR_PLL_STATUS:
+        break;
+
+    case ESRAM_CR:
+        if (newval != s->regs[ESRAM_CR]) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                       TYPE_MSF2_SYSREG": eSRAM remapping not supported\n");
+        }
+        break;
+
+    case DDR_CR:
+        if (newval != s->regs[DDR_CR]) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                       TYPE_MSF2_SYSREG": DDR remapping not supported\n");
+        }
+        break;
+
+    case ENVM_REMAP_BASE_CR:
+        if (newval != s->regs[ENVM_REMAP_BASE_CR]) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                       TYPE_MSF2_SYSREG": eNVM remapping not supported\n");
+        }
+        break;
+
+    default:
+        if (offset < ARRAY_SIZE(s->regs)) {
+            s->regs[offset] = newval;
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                        "%s: Bad offset 0x%08" HWADDR_PRIx "\n", __func__,
+                        offset << 2);
+        }
+        break;
+    }
+}
+
+static const MemoryRegionOps sysreg_ops = {
+    .read = msf2_sysreg_read,
+    .write = msf2_sysreg_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void msf2_sysreg_init(Object *obj)
+{
+    MSF2SysregState *s = MSF2_SYSREG(obj);
+
+    memory_region_init_io(&s->iomem, obj, &sysreg_ops, s, TYPE_MSF2_SYSREG,
+                          MSF2_SYSREG_MMIO_SIZE);
+    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
+}
+
+static const VMStateDescription vmstate_msf2_sysreg = {
+    .name = TYPE_MSF2_SYSREG,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, MSF2SysregState, MSF2_SYSREG_MMIO_SIZE / 4),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property msf2_sysreg_properties[] = {
+    /* default divisors in Libero GUI */
+    DEFINE_PROP_UINT32("apb0divisor", MSF2SysregState, apb0div, 2),
+    DEFINE_PROP_UINT32("apb1divisor", MSF2SysregState, apb1div, 2),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void msf2_sysreg_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->vmsd = &vmstate_msf2_sysreg;
+    dc->reset = msf2_sysreg_reset;
+    dc->props = msf2_sysreg_properties;
+}
+
+static const TypeInfo msf2_sysreg_info = {
+    .name  = TYPE_MSF2_SYSREG,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .class_init = msf2_sysreg_class_init,
+    .instance_size  = sizeof(MSF2SysregState),
+    .instance_init = msf2_sysreg_init,
+};
+
+static void msf2_sysreg_register_types(void)
+{
+    type_register_static(&msf2_sysreg_info);
+}
+
+type_init(msf2_sysreg_register_types)
diff --git a/include/hw/misc/msf2-sysreg.h b/include/hw/misc/msf2-sysreg.h
new file mode 100644
index 0000000..f39cc41
--- /dev/null
+++ b/include/hw/misc/msf2-sysreg.h
@@ -0,0 +1,78 @@ 
+/*
+ * Microsemi SmartFusion2 SYSREG
+ *
+ * Copyright (c) 2017 Subbaraya Sundeep <sundeep.lkml@gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef HW_MSF2_SYSREG_H
+#define HW_MSF2_SYSREG_H
+
+#include "hw/sysbus.h"
+
+enum {
+    ESRAM_CR        = 0x00 / 4,
+    ESRAM_MAX_LAT,
+    DDR_CR,
+    ENVM_CR,
+    ENVM_REMAP_BASE_CR,
+    ENVM_REMAP_FAB_CR,
+    CC_CR,
+    CC_REGION_CR,
+    CC_LOCK_BASE_ADDR_CR,
+    CC_FLUSH_INDX_CR,
+    DDRB_BUF_TIMER_CR,
+    DDRB_NB_ADDR_CR,
+    DDRB_NB_SIZE_CR,
+    DDRB_CR,
+
+    SOFT_RESET_CR  = 0x48 / 4,
+    M3_CR,
+
+    GPIO_SYSRESET_SEL_CR = 0x58 / 4,
+
+    MDDR_CR = 0x60 / 4,
+
+    MSSDDR_PLL_STATUS_LOW_CR = 0x90 / 4,
+    MSSDDR_PLL_STATUS_HIGH_CR,
+    MSSDDR_FACC1_CR,
+    MSSDDR_FACC2_CR,
+
+    MSSDDR_PLL_STATUS = 0x150 / 4,
+
+};
+
+#define MSF2_SYSREG_MMIO_SIZE     0x300
+
+#define TYPE_MSF2_SYSREG          "msf2-sysreg"
+#define MSF2_SYSREG(obj)  OBJECT_CHECK(MSF2SysregState, (obj), TYPE_MSF2_SYSREG)
+
+typedef struct MSF2SysregState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+
+    uint32_t apb0div;
+    uint32_t apb1div;
+
+    uint32_t regs[MSF2_SYSREG_MMIO_SIZE / 4];
+} MSF2SysregState;
+
+#endif /* HW_MSF2_SYSREG_H */