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

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

[Ajay Bhargav]

This patch adds generic GPIO driver framework support for Marvell SoCs.

To enable GPIO driver define CONFIG_MARVELL_GPIO and for GPIO commands
define CONFIG_CMD_GPIO in your board configuration file.

Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
---
 drivers/gpio/Makefile |    1 +
 drivers/gpio/mvgpio.c |  114 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/mvgpio.c

diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 62ec97d..beca1da 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -27,6 +27,7 @@ LIB 	:= $(obj)libgpio.o
 
 COBJS-$(CONFIG_AT91_GPIO)	+= at91_gpio.o
 COBJS-$(CONFIG_KIRKWOOD_GPIO)	+= kw_gpio.o
+COBJS-$(CONFIG_MARVELL_GPIO)	+= mvgpio.o
 COBJS-$(CONFIG_MARVELL_MFP)	+= mvmfp.o
 COBJS-$(CONFIG_MXC_GPIO)	+= mxc_gpio.o
 COBJS-$(CONFIG_PCA953X)		+= pca953x.o
diff --git a/drivers/gpio/mvgpio.c b/drivers/gpio/mvgpio.c
new file mode 100644
index 0000000..0cc8ed7
--- /dev/null
+++ b/drivers/gpio/mvgpio.c
@@ -0,0 +1,114 @@
+/*
+ * (C) Copyright 2011
+ * eInfochips Ltd. <www.einfochips.com>
+ * Written-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
+ *
+ * (C) Copyright 2010
+ * Marvell Semiconductor <www.marvell.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301 USA
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <asm/errno.h>
+#include <asm/gpio.h>
+
+#ifndef MV_MAX_GPIO
+#define MV_MAX_GPIO	128
+#endif
+
+int gpio_request(int gp, const char *label)
+{
+	if (gp >= MV_MAX_GPIO) {
+		printf("%s: Invalid GPIO requested %d\n", __func__, gp);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+void gpio_free(int gp)
+{
+}
+
+void gpio_toggle_value(int gp)
+{
+	gpio_set_value(gp, !gpio_get_value(gp));
+}
+
+int gpio_direction_input(int gp)
+{
+	struct gpio_reg *gpio_reg_bank;
+
+	if (gp >= MV_MAX_GPIO) {
+		printf("%s: Invalid GPIO %d\n", __func__, gp);
+		return -EINVAL;
+	}
+
+	gpio_reg_bank = get_gpio_base(GPIO_TO_REG(gp));
+	writel(GPIO_TO_BIT(gp), &gpio_reg_bank->gcdr);
+	return 0;
+}
+
+int gpio_direction_output(int gp, int value)
+{
+	struct gpio_reg *gpio_reg_bank;
+
+	if (gp >= MV_MAX_GPIO) {
+		printf("%s: Invalid GPIO %d\n", __func__, gp);
+		return -EINVAL;
+	}
+
+	gpio_reg_bank = get_gpio_base(GPIO_TO_REG(gp));
+	writel(GPIO_TO_BIT(gp), &gpio_reg_bank->gsdr);
+	gpio_set_value(gp, value);
+	return 0;
+}
+
+int gpio_get_value(int gp)
+{
+	struct gpio_reg *gpio_reg_bank;
+	u32 gp_val;
+
+	if (gp >= MV_MAX_GPIO) {
+		printf("%s: Invalid GPIO %d\n", __func__, gp);
+		return -EINVAL;
+	}
+
+	gpio_reg_bank = get_gpio_base(GPIO_TO_REG(gp));
+	gp_val = readl(&gpio_reg_bank->gplr);
+
+	return GPIO_VAL(gp, gp_val);
+}
+
+void gpio_set_value(int gp, int value)
+{
+	struct gpio_reg *gpio_reg_bank;
+
+	if (gp >= MV_MAX_GPIO) {
+		printf("%s: Invalid GPIO %d\n", __func__, gp);
+		return;
+	}
+
+	gpio_reg_bank = get_gpio_base(GPIO_TO_REG(gp));
+	if (value)
+		writel(GPIO_TO_BIT(gp),	&gpio_reg_bank->gpsr);
+	else
+		writel(GPIO_TO_BIT(gp),	&gpio_reg_bank->gpcr);
+}
-- 
1.7.0.4

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

[Simon Guinot]

Hi Ajay,

On Fri, Jul 22, 2011 at 12:46:33PM +0530, Ajay Bhargav wrote:
> This patch adds generic GPIO driver framework support for Marvell SoCs.
> 
> To enable GPIO driver define CONFIG_MARVELL_GPIO and for GPIO commands
> define CONFIG_CMD_GPIO in your board configuration file.
> 
> Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> ---
>  drivers/gpio/Makefile |    1 +
>  drivers/gpio/mvgpio.c |  114 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 115 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/gpio/mvgpio.c

... snip ...

> +int gpio_direction_input(int gp)
> +{
> +	struct gpio_reg *gpio_reg_bank;
> +
> +	if (gp >= MV_MAX_GPIO) {
> +		printf("%s: Invalid GPIO %d\n", __func__, gp);
> +		return -EINVAL;
> +	}
> +
> +	gpio_reg_bank = get_gpio_base(GPIO_TO_REG(gp));
> +	writel(GPIO_TO_BIT(gp), &gpio_reg_bank->gcdr);

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110802/3932c5e3/attachment.pgp 

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

[Ajay Bhargav]

----- "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.

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

so anyone can have their own version of these registers in gpio.h of their
arch. The only thing which can complicate this is banking of registers, no.
of banks etc.

Please provide comments on this, so we can have a better code.

Thanks & Regards,
Ajay Bhargav

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

[Prafulla Wadaskar]

> -----Original Message-----
> From: Simon Guinot [mailto:simon at sequanux.org]
> Sent: Tuesday, August 02, 2011 7:41 PM
> To: Ajay Bhargav
> Cc: Prafulla Wadaskar; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH v2 1/2] gpio: Add GPIO driver framework for
> Marvell SoCs
> 
> Hi Ajay,
> 
> On Fri, Jul 22, 2011 at 12:46:33PM +0530, Ajay Bhargav wrote:
> > This patch adds generic GPIO driver framework support for Marvell
> SoCs.
> >
> > To enable GPIO driver define CONFIG_MARVELL_GPIO and for GPIO commands
> > define CONFIG_CMD_GPIO in your board configuration file.
> >
> > Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> > ---
> >  drivers/gpio/Makefile |    1 +
> >  drivers/gpio/mvgpio.c |  114
> +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 115 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/gpio/mvgpio.c
> 
> ... snip ...
> 
> > +int gpio_direction_input(int gp)
> > +{
> > +	struct gpio_reg *gpio_reg_bank;
> > +
> > +	if (gp >= MV_MAX_GPIO) {
> > +		printf("%s: Invalid GPIO %d\n", __func__, gp);
> > +		return -EINVAL;
> > +	}
> > +
> > +	gpio_reg_bank = get_gpio_base(GPIO_TO_REG(gp));
> > +	writel(GPIO_TO_BIT(gp), &gpio_reg_bank->gcdr);
> 
> 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...

Hi Simon, you are right.

I think lets keep it mvgpio.c at the moment because most of other Marvell SoC uses this method. Any way Orion and Kirkwood are mainlined, if needed to be supported for this driver we can think of adding some #ifdef or macros to mvgpio.c latter.

Regards..
Prafulla . .

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

[Simon Guinot]

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) ?

Regards,

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110804/4672a522/attachment.pgp 

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

[Albert ARIBAUD]

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?

Or a single driver with a single API but implemented differently 
according to the presence / absence of a variant flag, e.g. 
CONFIG_MVGIPO_HAS_SET_AND_CLR_REGS?

> Regards,
>
> Simon

Amicalement,
-- 
Albert.

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

[Lei Wen]

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

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

[Ajay Bhargav]

----- "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

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

[Prafulla Wadaskar]

> -----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@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

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 . .

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

[Lei Wen]

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
> 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
>
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

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

[Ajay Bhargav]

----- "Prafulla Wadaskar" <prafulla@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@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
> 
> 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

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

[Prafulla Wadaskar]

> -----Original Message-----
> From: Ajay Bhargav [mailto:ajay.bhargav at einfochips.com]
> Sent: Saturday, August 06, 2011 10:40 AM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de; Lei Wen; Simon Guinot
> Subject: Re: [U-Boot] [PATCH v2 1/2] gpio: Add GPIO driver framework for
> Marvell SoCs
> 
...snip...

> > >
> > > @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?

Hi Ajay
This is straight forward solution, but the idea here to share the code, better design.
To simplify further, you can go ahead with below arch for this design.

1. Generic driver skeleton in mvgpio.c
2. Generic driver header mvgpio.h (optional you can avoid this)
3. Arch specific header asm/arch/gpio.h, to hold MAX GPIOs, register definition and gpio set/clr,config inlined functions or macros only.
4. Base address in asm/arch/<soc_name>.h and this file to be included in asm/arch/gpio.h

In future if we need to add support for other SoC we will add gpio.h.

Regards..
Prafulla . .