Re: [U-Boot] [PATCH v2 1/2] gpio: Add GPIO driver framework for Marvell SoCs
----- "Lei Wen" adrian.wenl@gmail.com wrote:
On Thu, Aug 4, 2011 at 4:51 PM, Albert ARIBAUD albert.u.boot@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@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
Regards, Ajay Bhargav
-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Ajay Bhargav Sent: Thursday, August 04, 2011 4:21 PM To: Lei Wen Cc: u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH v2 1/2] gpio: Add GPIO driver framework for Marvell SoCs
----- "Lei Wen" adrian.wenl@gmail.com wrote:
On Thu, Aug 4, 2011 at 4:51 PM, Albert ARIBAUD albert.u.boot@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@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
- Direction (read/write)
- Pin level set (write only)
- 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 Ajay,
On Thu, Aug 4, 2011 at 6:51 PM, Ajay Bhargav ajay.bhargav@einfochips.com wrote:
----- "Lei Wen" adrian.wenl@gmail.com wrote:
On Thu, Aug 4, 2011 at 4:51 PM, Albert ARIBAUD albert.u.boot@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@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
- Direction (read/write)
- Pin level set (write only)
- 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
I agree with Prafulla, if this not unalignment could be fixed in the .h file which is located in the arch directory, or ifdef there?, then I think I would totally fine with that. Or maybe seperate with two c file is a better solution.
Best regards, Lei
participants (3)
-
Ajay Bhargav -
Lei Wen -
Prafulla Wadaskar