[U-Boot] [PATCH v2 1/2] gpio: Add GPIO driver framework for Marvell SoCs

[Ajay Bhargav]

----- "Prafulla Wadaskar" <prafulla at marvell.com> wrote:

> > -----Original Message-----
> > From: u-boot-bounces at lists.denx.de
> [mailto:u-boot-bounces at lists.denx.de]
> > On Behalf Of Ajay Bhargav
> > Sent: Thursday, August 04, 2011 4:21 PM
> > To: Lei Wen
> > Cc: u-boot at lists.denx.de
> > Subject: Re: [U-Boot] [PATCH v2 1/2] gpio: Add GPIO driver framework
> for
> > Marvell SoCs
> > 
> > ----- "Lei Wen" <adrian.wenl at gmail.com> wrote:
> > 
> > > On Thu, Aug 4, 2011 at 4:51 PM, Albert ARIBAUD
> > > <albert.u.boot at aribaud.net> wrote:
> > > > Hi Simon,
> > > >
> > > > On 04/08/2011 02:04, Simon Guinot wrote:
> > > >> Hi Ajay,
> > > >>
> > > >> On Wed, Aug 03, 2011 at 10:10:00AM +0530, Ajay Bhargav wrote:
> > > >>> ----- "Simon Guinot"<simon at sequanux.org>  wrote:
> > > >>>
> > > >>>> AFAIK, Orion and Kirkwood SoCs don't provide bitwise
> set/clear
> > > for
> > > >>>> GPIO output/direction registers. Instead, a register must be
> > > read
> > > >>>> first to leave other bits unchanged (see __set_direction in
> > > >>>> kw_gpio.c).
> > > >>>>
> > > >>>> Is it possible to handle Armada SoCs GPIOs in a same way ?
> maybe
> > > >>>> using
> > > >>>> the pin registers (gpxx in the Armada struct gpio_reg array)
> ?
> > > >>>>
> > > >>>> If not, this code is not Marvell generic but rather specific
> for
> > > >>>> Armada
> > > >>>> SoCs and then maybe armada_gpio is a better name...
> > > >>>>
> > > >>>> Regards,
> > > >>>>
> > > >>>> Simon
> > > >>>
> > > >>> Hi Simon,
> > > >>>
> > > >>> Yes its possible to implement code that way, Armada SoC does
> have
> > > GPIO
> > > >>> registers for set/clear. what about register naming?? I think
> they
> > > are
> > > >>> different for Kirkwood and Orion.
> > > >>
> > > >> I think that the register names could be OK. But here is a
> most
> > > >> important problem: On Orion/Kirkwood SoCs, a single GPIO
> output
> > > register
> > > >> is available (no set/clear variants as for Armada). I missed
> that
> > > point
> > > >> at my first look. It is quite problematic because only two
> > > registers are
> > > >> shared between the different Marvell SoCs: level and direction.
> In
> > > fact,
> > > >> this registers are probably relevant on every machines
> providing
> > > GPIOs...
> > > >>
> > > >> Maybe that having two common registers is not enough to add
> > > >> Orion/Kirkwood support to the mvgpio driver ?
> > > >  >
> > > >>> One more thing which can be done to make this code generic is
> to
> > > have
> > > >>> some macros which can be defined by individual arch for
> specific
> > > registers
> > > >>> which are going to be in use e.g.
> > > >>>
> > > >>> #define GPIO_PIN_LEVEL_REG
> > > >>> #define GPIO_DIR_REG
> > > >>> #define GPIO_PIN_SET_REG
> > > >>> #define GPIO_PIN_CLR_REG
> > > >>
> > > >> Yes, but how to handle both a single GPI0 output register and
> some
> > > GPIO
> > > >> {set,clear} output registers (in a nice way) ?
> > > >
> > > > Two distinct gipo drivers for the two marvell variants?
> > >
> > > If let I choose, I'd prefer two, since the register set is
> different.
> > >
> > > Best regards,
> > > Lei
> > >
> > 
> > Hi Simon,
> > 
> > For Armada minimum 3 registers are required and available for
> armada
> > 1. Direction (read/write)
> > 2. Pin level set (write only)
> > 3. Pin level clear (write only)
> > 
> > @lei
> > How bout if we check for architecture and use specific code or
> defines?
> > i.e.
> > #ifdef CONFIG_KIRKWOOD
> > //KW code
> > #elif CONFIG_ARMADA100
> > //Armada code
> > #else
> > //orion or other?
> > #endif
> 
> Let's avoid this, because there will be several SoC architectures that
> uses similar GPIO register definitions, like kirkwood/orion have
> similar definition and armada/mmp/pantheon/etc.. have different one.
> 
> So we will end up having several #ifdefs. Ideally #ifdefs are
> discouraged for better coding practices.
> 
> Instead,
> I would suggest to use macros for this code segments or alternatively
> inlined functions and those should be defined in mvgpio.h, #ifdefed
> with  CPU core subversion (i.e. CONFIG_FEROCEION,
> CONFIG_SHEEVA_88SV331xV5)
> 
> Regards..
> Prafulla . .

Hi Prafulla,

I think it will be better to keep two driver files. Let this patch be for
Armada/mmp/pantheon and other compatible SoCs. Should the common GPIO
struct for armada/mmp etc.. be moved out of GPIO.h to mvgpio.h?

Regards,
Ajay Bhargav