Re: [Qemu-arm] [Qemu-devel PATCH 1/5] msf2: Add Smartfusion2 System timer

[Philippe Mathieu-Daudé]

On 05/10/2017 09:37 AM, sundeep subbaraya wrote:
> Hi Phil,
>
> On Wed, May 10, 2017 at 3:11 PM, Philippe Mathieu-Daudé <address@hidden
> <mailto:address@hidden>> wrote:
> > Hi Subbaraya, nice work!
> >
> > The timer you are modeling is the mss_timer, which is in particular
> used in
> > the smartfusion2, I'd rather name it mss_timer.c so it can be reused by
> > other SoC models.
> Ok I will change all other file names also to mss. Do I need to change
> type names
> also to mss?

As you wish :) Actel/Microsemi keep changing how they name it, MSS, 
M2S... What I mean is this timer is valid for a Actel SmartFusion and 
for the MicroSemi SmartFusion2, naming it "msf2-timer" seems to restrict 
it to the SF2 only.

> > I added few comments.
> >
> >
> > On 05/09/2017 01:44 PM, Subbaraya Sundeep wrote:
> > > Modelled System Timer in Microsemi's Smartfusion2 Soc.
> > > Timer has two 32bit down counters and two interrupts.
> > >
> > > Signed-off-by: Subbaraya Sundeep <address@hidden
> <mailto:address@hidden>>
> > > ---
> > >  hw/timer/Makefile.objs        |   1 +
> > >  hw/timer/msf2-timer.c         | 252
> > > ++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/timer/msf2-timer.h |  85 ++++++++++++++
> > >  3 files changed, 338 insertions(+)
> > >  create mode 100644 hw/timer/msf2-timer.c
> > >  create mode 100644 include/hw/timer/msf2-timer.h
[...]
> > > +        if (addr < ARRAY_SIZE(st->regs)) {
> > > +            st->regs[addr] = value;
> > > +        } else {
> > > +            qemu_log_mask(LOG_GUEST_ERROR,
> > > +                         "%s: Bad offset 0x%" HWADDR_PRIx "\n",
> __func__,
> > > +                         addr * 4);
> > > +        }
> > > +        break;
> > > +    }
> > > +    timer_update_irq(st);
> >
> >
> > Here if addr >= (NUM_TIMERS * R_TIM1_MAX) you still update Timer1 IRQ,
> while
> > this is unharmful right now this is likely to be break later.
> As long as Interrupt status register and Interrupt enable register are not
> modified calling timer_update_irq will not harm. Am I missing something
> here?

Indeed, this is unharmful. It just surprised me when I follow the 
control flow.

> > > +}
> > > +
> > > +static const MemoryRegionOps timer_ops = {
> > > +    .read = timer_read,
> > > +    .write = timer_write,
> > > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > > +    .valid = {
> > > +        .min_access_size = 4,
> >
> >
> > I believe min_access_size = 1 is valid for any APB device.
> Ok. I followed Xilinx soft IP models while writing this. I am really not
> sure it is mandatory to put access_size. Can i remove it?

checking the datasheet "UG0331: SmartFusion2 Microcontroller Subsystem":

'''
CMSIS Data types:

The [Cortex-M3] processor:
* supports the following data types:
- 32-bit words
- 16-bit halfwords
- 8-bit bytes.
* manages all data memory accesses as little-endian or big-endian. 
Instruction memory and Private Peripheral Bus (PPB) accesses are always 
performed as little-endian. The Cortex-M3 processor configured for 
SmartFusion2 SoC FPGA MSS uses only little-endian.
'''

So Yes, ".min_access_size = 1" is correct for this Cortex-M3.

If you remove it memory_region_access_valid() will do:

    access_size_min = mr->ops->valid.min_access_size;
    if (!mr->ops->valid.min_access_size) {
        access_size_min = 1;
    }

So that is the same, personally I prefer it to be explicit (not removed).

> > > +        .max_access_size = 4
> > > +    }
> > > +};
> > > +
> > > +static void timer_hit(void *opaque)
> > > +{
> > > +    struct Msf2Timer *st = opaque;
> > > +
> > > +    st->regs[R_TIM_RIS] |= TIMER_RIS_ACK;
> > > +
> > > +    if (!(st->regs[R_TIM_CTRL] & TIMER_CTRL_ONESHOT)) {
> > > +        timer_update(st);
> > > +    }
> > > +    timer_update_irq(st);
> > > +}
[...]
> > > +/*
> > > + * There are two 32-bit down counting timers.
> > > + * Timers 1 and 2 can be concatenated into a single 64-bit Timer
> > > + * that operates either in Periodic mode or in One-shot mode.
> > > + * Writing 1 to the TIM64_MODE register bit 0 sets the Timers in 64-bit
> > > mode.
> > > + * In 64-bit mode, writing to the 32-bit registers has no effect.
> > > + * Similarly, in 32-bit mode, writing to the 64-bit mode registers
> > > + * has no effect. Only two 32-bit timers are supported currently.
> > > + */
> > > +#define NUM_TIMERS        2
> > > +
> > > +#define MSF2_TIMER_FREQ   (83 * 1000000)
> >
> >
> > I can not find this value, can you point me to the datasheet? It seems SoC
> > specific to me.
> It is configured in Microsemi Libero. The SOM kit from Emcraft comes
> with this default setting.
> I guess this property should be set and passed from board file and not
> from SoC.
> Am I correct?

It seems an option configurable in Libero before synthesizing, so that 
would be SoM/bitfile specific?

What I mean here is I don't think this is a fixed value for a mss_timer 
and I'd rather have it configurable (but ok to default 83MHz in your SoM).

> Can I attach the datasheet to this thread?

Isn't this datasheet publicly available?

Eventually can you upload a binary (like your Linux patches) somewhere? 
So it would be easier to test this patchset.

> Thank you,
> Sundeep

Good luck!

Phil.