qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Qemu devel v8 PATCH 2/5] msf2: Microsemi Smartfusion2


From: sundeep subbaraya
Subject: Re: [Qemu-devel] [Qemu devel v8 PATCH 2/5] msf2: Microsemi Smartfusion2 System Register block
Date: Thu, 14 Sep 2017 21:04:33 +0530

Hi Philippe,

On Thu, Sep 14, 2017 at 10:06 AM, Philippe Mathieu-Daudé <address@hidden>
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 <address@hidden>
>> Reviewed-by: Alistair Francis <address@hidden>
>> ---
>>   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 <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"
>> +
>> +#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.
>


reply via email to

[Prev in Thread] Current Thread [Next in Thread]