Hi Philippe, On Fri, May 12, 2017 at 10:08 AM, Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> On 05/10/2017 09:37 AM, sundeep subbaraya wrote: > >> Hi Phil, >> >> On Wed, May 10, 2017 at 3:11 PM, Philippe Mathieu-Daudé <f4...@amsat.org >> <mailto:f4...@amsat.org>> 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. > > Hmm. OK I will change to mss except for SoC model,file and SOM file. > 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 <sundeep.l...@gmail.com >>>> >>> <mailto:sundeep.l...@gmail.com>> >> >>> --- >>>> 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. Ok I will change it. > > > +} >>>> + >>>> +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). > > Ok will change to 1. > + .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). > > Yeah. Same like hw/microblaze/petalogix_ml605_mmu.c where one design was chosen for FPGA. I have chosen the one which comes preloaded with Emcraft SOM kit. > > Can I attach the datasheet to this thread? > > Isn't this datasheet publicly available? > > We need to download instead of direct google link. I dont know why. Eventually can you upload a binary (like your Linux patches) somewhere? So > it would be easier to test this patchset. > Sure I will put the source code and binaries in github. > > Thank you, >> Sundeep >> > > Good luck! > > Phil. > Thanks for taking your time to review. Sundeep
