Re: [Qemu-devel] [PATCH 1/6] [RFC] Emulation of GRLIB GPTimer as defined in GRLIB IP Core User's Manual.

[Fabien Chouteau]

On 12/08/2010 09:30 AM, Edgar E. Iglesias wrote:
> On Tue, Dec 07, 2010 at 10:55:33AM +0100, Fabien Chouteau wrote:
> > On 12/06/2010 06:12 PM, Blue Swirl wrote:
> > > On Mon, Dec 6, 2010 at 9:26 AM, Fabien Chouteau<address@hidden>   wrote:
> > > > Signed-off-by: Fabien Chouteau<address@hidden>
> > > > ---
> > > >    hw/grlib_gptimer.c |  448 
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >    1 files changed, 448 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/hw/grlib_gptimer.c b/hw/grlib_gptimer.c
> > > > new file mode 100644
> > > > index 0000000..41edbe4
> > > > --- /dev/null
> > > > +++ b/hw/grlib_gptimer.c
> > > > @@ -0,0 +1,448 @@
> > > > +/*
> > > > + * QEMU GRLIB GPTimer Emulator
> > > > + *
> > > > + * Copyright (c) 2010 AdaCore
> > > > + *
> > > > + * 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.
> > > > + */
> > > > +
> > > > +#include "sysbus.h"
> > > > +#include "qemu-timer.h"
> > > > +
> > > > +#include "grlib.h"
> > > > +
> > > > +/* #define DEBUG_TIMER */
> > > The usual convention is
> > > //#define DEBUG_TIMER
> > > for easy editing.
> > Actually, it's easier for me with the /* */, but OK.
> >
> > > However, very often the much more powerful tracepoints can replace
> > > debug statements.
> > >
> > > > +
> > > > +#ifdef DEBUG_TIMER
> > > > +#define DPRINTF(fmt, ...)                                       \
> > > > +    do { printf("GPTIMER: " fmt , ## __VA_ARGS__); } while (0)
> > > > +#else
> > > > +#define DPRINTF(fmt, ...)
> > > > +#endif
> > > > +
> > > > +#define UNIT_REG_SIZE    16     /* Size of memory mapped regs for the unit */
> > > > +#define GPTIMER_REG_SIZE 16     /* Size of memory mapped regs for a GPTimer */
> > > > +
> > > > +#define GPTIMER_MAX_TIMERS 8
> > > > +
> > > > +/* GPTimer Config register fields */
> > > > +#define GPTIMER_ENABLE      (1<<   0)
> > > > +#define GPTIMER_RESTART     (1<<   1)
> > > > +#define GPTIMER_LOAD        (1<<   2)
> > > > +#define GPTIMER_INT_ENABLE  (1<<   3)
> > > > +#define GPTIMER_INT_PENDING (1<<   4)
> > > > +#define GPTIMER_CHAIN       (1<<   5) /* Not supported */
> > > > +#define GPTIMER_DEBUG_HALT  (1<<   6) /* Not supported */
> > > > +
> > > > +/* Memory mapped register offsets */
> > > > +#define SCALER_OFFSET         0x00
> > > > +#define SCALER_RELOAD_OFFSET  0x04
> > > > +#define CONFIG_OFFSET         0x08
> > > > +#define COUNTER_OFFSET        0x00
> > > > +#define COUNTER_RELOAD_OFFSET 0x04
> > > > +#define TIMER_BASE            0x10
> > > > +
> > > > +typedef struct GPTimer     GPTimer;
> > > > +typedef struct GPTimerUnit GPTimerUnit;
> > > > +
> > > > +struct GPTimer
> > > > +{
> > > > +    QEMUBH *bh;
> > > > +    struct ptimer_state *ptimer;
> > > > +
> > > > +    qemu_irq     irq;
> > > > +    int          id;
> > > > +    GPTimerUnit *unit;
> > > > +
> > > > +    /* registers */
> > > > +    uint32_t counter;
> > > > +    uint32_t reload;
> > > > +    uint32_t config;
> > > > +};
> > > > +
> > > > +struct GPTimerUnit
> > > > +{
> > > > +    SysBusDevice  busdev;
> > > > +
> > > > +    uint32_t nr_timers;         /* Number of timers available */
> > > > +    uint32_t freq_hz;           /* System frequency */
> > > > +    uint32_t irq_line;          /* Base irq line */
> > > > +
> > > > +    GPTimer *timers;
> > > > +
> > > > +    /* registers */
> > > > +    uint32_t scaler;
> > > > +    uint32_t reload;
> > > > +    uint32_t config;
> > > > +};
> > > > +
> > > > +DeviceState *grlib_gptimer_create(target_phys_addr_t  base,
> > > > +                                  uint32_t            nr_timers,
> > > > +                                  uint32_t            freq,
> > > > +                                  qemu_irq           *cpu_irqs,
> > > > +                                  int                 base_irq)
> > > This function belongs to leon3.c.
> > I don't see why. GPTimer is a peripheral and you may want to use it in
> > an other system.
> This might depend a bit on taste, but I agree with Blue that we shouldn't
> clutter the device models with this kind of instantiator helper calls.
> IMO it's better to put them higher up (e.g board code or similar).

Do you mean like Xilinx devices where the instantiators are in-lined 
functions in hw/xilinx.h?

--
Fabien Chouteau