Re: [Qemu-devel] [PATCH v3 1/3] hw/gpio: Add basic Aspeed GPIO model for AST2400 and AST2500

[Rashmica Gupta <rashmica.g@gmail.com>]

On Tue, 2019-08-06 at 14:57 +0100, Peter Maydell wrote:
> On Tue, 30 Jul 2019 at 06:45, Rashmica Gupta <rashmica.g@gmail.com>
> wrote:
> > GPIO pins are arranged in groups of 8 pins labeled
> > A,B,..,Y,Z,AA,AB,AC.
> > (Note that the ast2400 controller only goes up to group AB).
> > A set has four groups (except set AC which only has one) and is
> > referred to by the groups it is composed of (eg
> > ABCD,EFGH,...,YZAAAB).
> > Each set is accessed and controlled by a bank of 14 registers.
> > 
> > These registers operate on a per pin level where each bit in the
> > register
> > corresponds to a pin, except for the command source registers. The
> > command
> > source registers operate on a per group level where bits 24, 16, 8
> > and 0
> > correspond to each group in the set.
> > 
> >  eg. registers for set ABCD:
> >  |D7...D0|C7...C0|B7...B0|A7...A0| <- GPIOs
> >  |31...24|23...16|15....8|7.....0| <- bit position
> > 
> > Note that there are a couple of groups that only have 4 pins.
> > 
> > There are two ways that this model deviates from the behaviour of
> > the
> > actual controller:
> > (1) The only control source driving the GPIO pins in the model is
> > the ARM
> > model (as there currently aren't models for the LPC or
> > Coprocessor).
> > 
> > (2) None of the registers in the model are reset tolerant (needs
> > integration with the watchdog).
> > 
> > +typedef struct AspeedGPIOReg {
> > +    uint16_t set_idx;
> > +    uint32_t (*read)(GPIOSets *regs);
> > +    void (*write)(AspeedGPIOState *s, GPIOSets *regs,
> > +                const GPIOSetProperties *props, uint32_t val);
> > + } AspeedGPIOReg;
> 
> Please don't create new abstractions for implementing
> registers that are only used in one device model. We
> have a couple of basic approaches that we use already:

That's fair enough :)

> 
>  * just open coded switch statements in the read and write
>    functions for the device's MMIO regions

Thoughts on the switch statement approach in v4? I think it's
much nicer than two huge switch statements. 

>  * if you'd rather have a data structure defining each
>    register with hook functions where they need to do
>    particular behaviour on reads and writes, have a look
>    at the APIs in include/hw/register.h. Currently these are
>    used just by some of the Xilinx devices, but if you
>    want an API shaped like that you can use it.
> 

I had a play with the existing register API. I couldn't quite
get it to work. When writing to some registers in a set we
need to look at other registers in that set, and so we need
some kind of structure tells us how to find those registers.


> thanks
> -- PMM