[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Ajay Bhargav]

This patch adds generic GPIO driver framework support for Armada100 series.

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

NOTE: This driver assumes proper configuration of MFP register for the GPIO
in use.

These patches depends on patch series that adds support for Marvell
Guruplug-Display [1,2].

Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
---
 arch/arm/include/asm/arch-armada100/gpio.h |  130 +++++++++++++++++++
 drivers/gpio/Makefile                      |    1 +
 drivers/gpio/armada100_gpio.c              |  192 ++++++++++++++++++++++++++++
 3 files changed, 323 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-armada100/gpio.h
 create mode 100644 drivers/gpio/armada100_gpio.c

diff --git a/arch/arm/include/asm/arch-armada100/gpio.h b/arch/arm/include/asm/arch-armada100/gpio.h
new file mode 100644
index 0000000..9024a2e
--- /dev/null
+++ b/arch/arm/include/asm/arch-armada100/gpio.h
@@ -0,0 +1,130 @@
+/*
+ * (C) Copyright 2010
+ * 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
+ */
+
+#ifndef _ASM_ARCH_GPIO_H
+#define _ASM_ARCH_GPIO_H
+
+#ifndef __ASSEMBLY__
+#include <asm/types.h>
+#endif
+
+#if defined(CONFIG_ARMADA100_GPIO)
+#include <asm/arch/armada100.h>
+
+#define GPIO_LABEL_MAX		20
+#define ARMD_MAX_GPIO		128
+
+#define GPIO_TO_REG(gp)		(gp >> 5)
+#define GPIO_TO_BIT(gp)		(1 << (gp & 0x1F))
+#define GPIO_VAL(gp, val)	((val >> (gp & 0x1F)) & 0x01)
+
+#define GPIO_SET		1
+#define GPIO_CLR		0
+
+/*
+ * GPIO register map
+ * Refer Datasheet Appendix A.36
+ */
+struct armdgpio_registers {
+	u32 gplr0;	/* Pin Level Register - 0x0000 */
+	u32 gplr1;	/* 0x0004 */
+	u32 gplr2;	/* 0x0008 */
+	u32 gpdr0;	/* Pin Direction Register - 0x000C */
+	u32 gpdr1;	/* 0x0010 */
+	u32 gpdr2;	/* 0x0014 */
+	u32 gpsr0;	/* Pin Output Set Register - 0x0018 */
+	u32 gpsr1;	/* 0x001C */
+	u32 gpsr2;	/* 0x0020 */
+	u32 gpcr0;	/* Pin Output Clear Register - 0x0024 */
+	u32 gpcr1;	/* 0x0028 */
+	u32 gpcr2;	/* 0x002C */
+	u32 grer0;	/* Rising-Edge Detect Enable Register - 0x0030 */
+	u32 grer1;	/* 0x0034 */
+	u32 grer2;	/* 0x0038 */
+	u32 gfer0;	/* Falling-Edge Detect Enable Register - 0x003C */
+	u32 gfer1;	/* 0x0040 */
+	u32 gfer2;	/* 0x0044 */
+	u32 gedr0;	/* Edge Detect Status Register - 0x0048 */
+	u32 gedr1;	/* 0x004C */
+	u32 gedr2;	/* 0x0050 */
+	u32 gsdr0;	/* Bitwise Set of GPIO Direction Register - 0x0054 */
+	u32 gsdr1;	/* 0x0058 */
+	u32 gsdr2;	/* 0x005C */
+	u32 gcdr0;	/* Bitwise Clear of GPIO Direction Register - 0x0060 */
+	u32 gcdr1;	/* 0x0064 */
+	u32 gcdr2;	/* 0x0068 */
+	u32 gsrer0;	/* Bitwise Set of Rising-Edge Detect Enable
+			   Register - 0x006C */
+	u32 gsrer1;	/* 0x0070 */
+	u32 gsrer2;	/* 0x0074 */
+	u32 gcrer0;	/* Bitwise Clear of Rising-Edge Detect Enable
+			   Register - 0x0078 */
+	u32 gcrer1;	/* 0x007C */
+	u32 gcrer2;	/* 0x0080 */
+	u32 gsfer0;	/* Bitwise Set of Falling-Edge Detect Enable
+			   Register - 0x0084 */
+	u32 gsfer1;	/* 0x0088 */
+	u32 gsfer2;	/* 0x008C */
+	u32 gcfer0;	/* Bitwise Clear of Falling-Edge Detect Enable
+			   Register - 0x0090 */
+	u32 gcfer1;	/* 0x0094 */
+	u32 gcfer2;	/* 0x0098 */
+	u32 apmask0;	/* Bitwise Mask of Edge Detect Register - 0x009C */
+	u32 apmask1;	/* 0x00A0 */
+	u32 apmask2;	/* 0x00A4 */
+	u8 pad[0x0100 - 0x00A4 - 4];
+	u32 gplr3;	/* 0x0100 */
+	u8 pad1[0x010C - 0x0100 - 4];
+	u32 gpdr3;	/* 0x010C */
+	u8 pad2[0x0118 - 0x010C - 4];
+	u32 gpsr3;	/* 0x0118 */
+	u8 pad3[0x0124 - 0x0118 - 4];
+	u32 gpcr3;	/* 0x0124 */
+	u8 pad4[0x0130 - 0x0124 - 4];
+	u32 grer3;	/* 0x0130 */
+	u8 pad5[0x013C - 0x0130 - 4];
+	u32 gfer3;	/* 0x013C */
+	u8 pad6[0x0148 - 0x013C - 4];
+	u32 gedr3;	/* 0x0148 */
+	u8 pad7[0x0154 - 0x0148 - 4];
+	u32 gsdr3;	/* 0x0154 */
+	u8 pad8[0x0160 - 0x0154 - 4];
+	u32 gcdr3;	/* 0x0160 */
+	u8 pad9[0x016C - 0x0160 - 4];
+	u32 gsrer3;	/* 0x016C */
+	u8 pad10[0x0178 - 0x016C - 4];
+	u32 gcrer3;	/* 0x0178 */
+	u8 pad11[0x0184 - 0x0178 - 4];
+	u32 gsfer3;	/* 0x0184 */
+	u8 pad12[0x0190 - 0x0184 - 4];
+	u32 gcfer3;	/* 0x0190 */
+	u8 pad13[0x019C - 0x0190 - 4];
+	u32 apmask3;	/* 0x019C */
+};
+
+#endif /* CONFIG_ARMADA100_GPIO */
+#endif /* _ASM_ARCH_GPIO_H */
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 1e3ae11..31b83cd 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk
 LIB 	:= $(obj)libgpio.o
 
 COBJS-$(CONFIG_AT91_GPIO)	+= at91_gpio.o
+COBJS-$(CONFIG_ARMADA100_GPIO)	+= armada100_gpio.o
 COBJS-$(CONFIG_KIRKWOOD_GPIO)	+= kw_gpio.o
 COBJS-$(CONFIG_MARVELL_MFP)	+= mvmfp.o
 COBJS-$(CONFIG_MXC_GPIO)	+= mxc_gpio.o
diff --git a/drivers/gpio/armada100_gpio.c b/drivers/gpio/armada100_gpio.c
new file mode 100644
index 0000000..578fdac
--- /dev/null
+++ b/drivers/gpio/armada100_gpio.c
@@ -0,0 +1,192 @@
+/*
+ * (C) Copyright 2010
+ * 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>
+
+int gpio_request(int gp, const char *label)
+{
+	/*
+	 * Assumes corresponding MFP is configured peoperly
+	 * for use as GPIO
+	 */
+	return 0;
+}
+
+void gpio_free(int gp)
+{
+	/* default direction for GPIO is input */
+	gpio_direction_input(gp);
+}
+
+void gpio_toggle_value(int gp)
+{
+	gpio_set_value(gp, !gpio_get_value(gp));
+}
+
+int gpio_direction_input(int gp)
+{
+	struct armdgpio_registers *gpio_regs =
+		(struct armdgpio_registers *)ARMD1_GPIO_BASE;
+
+	if (gp < ARMD_MAX_GPIO) {
+		switch (GPIO_TO_REG(gp)) {
+		case 0:
+			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr0);
+			break;
+		case 1:
+			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr1);
+			break;
+		case 2:
+			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr2);
+			break;
+		case 3:
+			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr3);
+			break;
+		default:
+			return -EINVAL;
+		}
+	} else {
+		return -EINVAL;
+	}
+	return 0;
+}
+
+int gpio_direction_output(int gp, int value)
+{
+	struct armdgpio_registers *gpio_regs =
+		(struct armdgpio_registers *)ARMD1_GPIO_BASE;
+
+	if (gp < ARMD_MAX_GPIO) {
+		switch (GPIO_TO_REG(gp)) {
+		case 0:
+			writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr0);
+			if (value)
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr0);
+			else
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr0);
+			break;
+		case 1:
+			writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr1);
+			if (value)
+				writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr1);
+			else
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr1);
+			break;
+		case 2:
+			writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr2);
+			if (value)
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr2);
+			else
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr2);
+			break;
+		case 3:
+			writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr3);
+			if (value)
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr3);
+			else
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr3);
+			break;
+		default:
+			return -EINVAL;
+		}
+	} else {
+		return -EINVAL;
+	}
+	return 0;
+}
+
+int gpio_get_value(int gp)
+{
+	struct armdgpio_registers *gpio_regs =
+		(struct armdgpio_registers *)ARMD1_GPIO_BASE;
+	u32 gp_val;
+
+	if (gp < ARMD_MAX_GPIO) {
+		switch (GPIO_TO_REG(gp)) {
+		case 0:
+			gp_val = readl(&gpio_regs->gplr0);
+			break;
+		case 1:
+			gp_val = readl(&gpio_regs->gplr1);
+			break;
+		case 2:
+			gp_val = readl(&gpio_regs->gplr2);
+			break;
+		case 3:
+			gp_val = readl(&gpio_regs->gplr3);
+			break;
+		default:
+			return -EINVAL;
+		}
+	} else {
+		return -EINVAL;
+	}
+
+	return GPIO_VAL(gp, gp_val);
+}
+
+void gpio_set_value(int gp, int value)
+{
+	struct armdgpio_registers *gpio_regs =
+		(struct armdgpio_registers *)ARMD1_GPIO_BASE;
+
+	if (gp < ARMD_MAX_GPIO) {
+		switch (GPIO_TO_REG(gp)) {
+		case 0:
+			if (value)
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr0);
+			else
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr0);
+			break;
+		case 1:
+			if (value)
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr1);
+			else
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr1);
+			break;
+		case 2:
+			if (value)
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr2);
+			else
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr2);
+			break;
+		case 3:
+			if (value)
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr3);
+			else
+				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr3);
+			break;
+		default:
+			panic("Invalid GPIO pin %u\n", gp);
+		}
+	} else {
+		panic("Invalid GPIO pin %u\n", gp);
+	}
+}
-- 
1.7.0.4

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Mike Frysinger]

On Mon, Jul 18, 2011 at 05:41, Ajay Bhargav wrote:
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-armada100/gpio.h
>
> +#ifndef __ASSEMBLY__
> +#include <asm/types.h>
> +#endif
> ...
> +struct armdgpio_registers {
> +       u32 gplr0;      /* Pin Level Register - 0x0000 */
> ...

considering you dont protect actual C code in this file with
__ASSEMBLY__, it doesnt make much sense to protect the include

> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
>
> ?COBJS-$(CONFIG_AT91_GPIO) ? ? ?+= at91_gpio.o
> +COBJS-$(CONFIG_ARMADA100_GPIO) += armada100_gpio.o

"R" comes before "T", so your new entry should be above the at91 one

> --- /dev/null
> +++ b/drivers/gpio/armada100_gpio.c
>
> +int gpio_request(int gp, const char *label)
> +{
> + ? ? ? /*
> + ? ? ? ?* Assumes corresponding MFP is configured peoperly
> + ? ? ? ?* for use as GPIO
> + ? ? ? ?*/
> + ? ? ? return 0;
> +}

i think you mean "properly" in the comment, but this assumption is
bogus.  error checking for valid pins has to be in the gpio_request()
so that all the other funcs can assume the pin is valid.  not the
other way around.

> +void gpio_free(int gp)
> +{
> + ? ? ? /* default direction for GPIO is input */
> + ? ? ? gpio_direction_input(gp);
> +}

usually people dont do this.  usually the free step leaves the pin
alone.  i know in the linux kernel, they assume (and thus require)
that behavior.
-mike

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Prafulla Wadaskar]

> -----Original Message-----
> From: Ajay Bhargav [mailto:ajay.bhargav at einfochips.com]
> Sent: Monday, July 18, 2011 3:12 PM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de; Ajay Bhargav
> Subject: [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

Correct patch name as
gpio: Add GPIO driver for Marvell SoC Armada100

> 
> This patch adds generic GPIO driver framework support for Armada100
> series.
> 
> To enable GPIO driver define CONFIG_ARMADA100_GPIO and for GPIO commands
> define CONFIG_CMD_GPIO in your board configuration file.
> 
> NOTE: This driver assumes proper configuration of MFP register for the
> GPIO
> in use.
> 
> These patches depends on patch series that adds support for Marvell
> Guruplug-Display [1,2].
> 
> Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
> ---
>  arch/arm/include/asm/arch-armada100/gpio.h |  130 +++++++++++++++++++
>  drivers/gpio/Makefile                      |    1 +
>  drivers/gpio/armada100_gpio.c              |  192
> ++++++++++++++++++++++++++++
>  3 files changed, 323 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/include/asm/arch-armada100/gpio.h
>  create mode 100644 drivers/gpio/armada100_gpio.c
> 
> diff --git a/arch/arm/include/asm/arch-armada100/gpio.h
> b/arch/arm/include/asm/arch-armada100/gpio.h
> new file mode 100644
> index 0000000..9024a2e
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-armada100/gpio.h
> @@ -0,0 +1,130 @@
> +/*
> + * (C) Copyright 2010

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
> + */
> +
> +#ifndef _ASM_ARCH_GPIO_H
> +#define _ASM_ARCH_GPIO_H
> +
> +#ifndef __ASSEMBLY__
> +#include <asm/types.h>
> +#endif
> +
> +#if defined(CONFIG_ARMADA100_GPIO)
> +#include <asm/arch/armada100.h>
> +
> +#define GPIO_LABEL_MAX		20
> +#define ARMD_MAX_GPIO		128

Pls derive it from MAX_MFP

> +
> +#define GPIO_TO_REG(gp)		(gp >> 5)
> +#define GPIO_TO_BIT(gp)		(1 << (gp & 0x1F))
> +#define GPIO_VAL(gp, val)	((val >> (gp & 0x1F)) & 0x01)
> +
> +#define GPIO_SET		1
> +#define GPIO_CLR		0
> +
> +/*
> + * GPIO register map
> + * Refer Datasheet Appendix A.36
> + */
> +struct armdgpio_registers {
> +	u32 gplr0;	/* Pin Level Register - 0x0000 */
> +	u32 gplr1;	/* 0x0004 */
> +	u32 gplr2;	/* 0x0008 */
> +	u32 gpdr0;	/* Pin Direction Register - 0x000C */

NAK

You should define all GPIO register in a single struct
And point them with base address offsets


> +	u32 gpdr1;	/* 0x0010 */
> +	u32 gpdr2;	/* 0x0014 */
> +	u32 gpsr0;	/* Pin Output Set Register - 0x0018 */
> +	u32 gpsr1;	/* 0x001C */
> +	u32 gpsr2;	/* 0x0020 */
> +	u32 gpcr0;	/* Pin Output Clear Register - 0x0024 */
> +	u32 gpcr1;	/* 0x0028 */
> +	u32 gpcr2;	/* 0x002C */
> +	u32 grer0;	/* Rising-Edge Detect Enable Register - 0x0030 */
> +	u32 grer1;	/* 0x0034 */
> +	u32 grer2;	/* 0x0038 */
> +	u32 gfer0;	/* Falling-Edge Detect Enable Register - 0x003C */
> +	u32 gfer1;	/* 0x0040 */
> +	u32 gfer2;	/* 0x0044 */
> +	u32 gedr0;	/* Edge Detect Status Register - 0x0048 */
> +	u32 gedr1;	/* 0x004C */
> +	u32 gedr2;	/* 0x0050 */
> +	u32 gsdr0;	/* Bitwise Set of GPIO Direction Register - 0x0054 */
> +	u32 gsdr1;	/* 0x0058 */
> +	u32 gsdr2;	/* 0x005C */
> +	u32 gcdr0;	/* Bitwise Clear of GPIO Direction Register - 0x0060 */
> +	u32 gcdr1;	/* 0x0064 */
> +	u32 gcdr2;	/* 0x0068 */
> +	u32 gsrer0;	/* Bitwise Set of Rising-Edge Detect Enable
> +			   Register - 0x006C */
> +	u32 gsrer1;	/* 0x0070 */
> +	u32 gsrer2;	/* 0x0074 */
> +	u32 gcrer0;	/* Bitwise Clear of Rising-Edge Detect Enable
> +			   Register - 0x0078 */
> +	u32 gcrer1;	/* 0x007C */
> +	u32 gcrer2;	/* 0x0080 */
> +	u32 gsfer0;	/* Bitwise Set of Falling-Edge Detect Enable
> +			   Register - 0x0084 */
> +	u32 gsfer1;	/* 0x0088 */
> +	u32 gsfer2;	/* 0x008C */
> +	u32 gcfer0;	/* Bitwise Clear of Falling-Edge Detect Enable
> +			   Register - 0x0090 */
> +	u32 gcfer1;	/* 0x0094 */
> +	u32 gcfer2;	/* 0x0098 */
> +	u32 apmask0;	/* Bitwise Mask of Edge Detect Register - 0x009C */
> +	u32 apmask1;	/* 0x00A0 */
> +	u32 apmask2;	/* 0x00A4 */
> +	u8 pad[0x0100 - 0x00A4 - 4];
> +	u32 gplr3;	/* 0x0100 */
> +	u8 pad1[0x010C - 0x0100 - 4];
> +	u32 gpdr3;	/* 0x010C */
> +	u8 pad2[0x0118 - 0x010C - 4];
> +	u32 gpsr3;	/* 0x0118 */
> +	u8 pad3[0x0124 - 0x0118 - 4];
> +	u32 gpcr3;	/* 0x0124 */
> +	u8 pad4[0x0130 - 0x0124 - 4];
> +	u32 grer3;	/* 0x0130 */
> +	u8 pad5[0x013C - 0x0130 - 4];
> +	u32 gfer3;	/* 0x013C */
> +	u8 pad6[0x0148 - 0x013C - 4];
> +	u32 gedr3;	/* 0x0148 */
> +	u8 pad7[0x0154 - 0x0148 - 4];
> +	u32 gsdr3;	/* 0x0154 */
> +	u8 pad8[0x0160 - 0x0154 - 4];
> +	u32 gcdr3;	/* 0x0160 */
> +	u8 pad9[0x016C - 0x0160 - 4];
> +	u32 gsrer3;	/* 0x016C */
> +	u8 pad10[0x0178 - 0x016C - 4];
> +	u32 gcrer3;	/* 0x0178 */
> +	u8 pad11[0x0184 - 0x0178 - 4];
> +	u32 gsfer3;	/* 0x0184 */
> +	u8 pad12[0x0190 - 0x0184 - 4];
> +	u32 gcfer3;	/* 0x0190 */
> +	u8 pad13[0x019C - 0x0190 - 4];
> +	u32 apmask3;	/* 0x019C */
> +};
> +
> +#endif /* CONFIG_ARMADA100_GPIO */
> +#endif /* _ASM_ARCH_GPIO_H */
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 1e3ae11..31b83cd 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk
>  LIB 	:= $(obj)libgpio.o
> 
>  COBJS-$(CONFIG_AT91_GPIO)	+= at91_gpio.o
> +COBJS-$(CONFIG_ARMADA100_GPIO)	+= armada100_gpio.o

I suggest you to add mvgpio.c instead of armada100_gpio.c
This will be used in future by other Marvell SoCs.

>  COBJS-$(CONFIG_KIRKWOOD_GPIO)	+= kw_gpio.o
>  COBJS-$(CONFIG_MARVELL_MFP)	+= mvmfp.o
>  COBJS-$(CONFIG_MXC_GPIO)	+= mxc_gpio.o
> diff --git a/drivers/gpio/armada100_gpio.c
> b/drivers/gpio/armada100_gpio.c
> new file mode 100644
> index 0000000..578fdac
> --- /dev/null
> +++ b/drivers/gpio/armada100_gpio.c
> @@ -0,0 +1,192 @@
> +/*
> + * (C) Copyright 2010

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>
> +
> +int gpio_request(int gp, const char *label)
> +{
> +	/*
> +	 * Assumes corresponding MFP is configured peoperly
> +	 * for use as GPIO
> +	 */

NAK, you should check here, respective MFP is being configured as GPIO, if not you should return error

> +	return 0;
> +}
> +
> +void gpio_free(int gp)
> +{
> +	/* default direction for GPIO is input */
> +	gpio_direction_input(gp);
> +}
> +
> +void gpio_toggle_value(int gp)
> +{
> +	gpio_set_value(gp, !gpio_get_value(gp));
> +}
> +
> +int gpio_direction_input(int gp)
> +{
> +	struct armdgpio_registers *gpio_regs =
> +		(struct armdgpio_registers *)ARMD1_GPIO_BASE;

Consider now this is generic GPIO driver for marvell SOCs, define this macro in gpio.h

> +
> +	if (gp < ARMD_MAX_GPIO) {
> +		switch (GPIO_TO_REG(gp)) {

Some code comments are welcomed here.

> +		case 0:
> +			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr0);
> +			break;
> +		case 1:
> +			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr1);
> +			break;
> +		case 2:
> +			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr2);
> +			break;
> +		case 3:
> +			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr3);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +int gpio_direction_output(int gp, int value)
> +{
> +	struct armdgpio_registers *gpio_regs =
> +		(struct armdgpio_registers *)ARMD1_GPIO_BASE;
> +
> +	if (gp < ARMD_MAX_GPIO) {
> +		switch (GPIO_TO_REG(gp)) {
> +		case 0:
> +			writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr0);

Call gpio_set_value(gp, value) here which is defined below doing the same


> +			if (value)
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr0);
> +			else
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr0);
> +			break;
> +		case 1:
> +			writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr1);
> +			if (value)
> +				writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr1);
> +			else
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr1);
> +			break;
> +		case 2:
> +			writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr2);
> +			if (value)
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr2);
> +			else
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr2);
> +			break;
> +		case 3:
> +			writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr3);
> +			if (value)
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr3);
> +			else
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr3);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +int gpio_get_value(int gp)
> +{
> +	struct armdgpio_registers *gpio_regs =
> +		(struct armdgpio_registers *)ARMD1_GPIO_BASE;
> +	u32 gp_val;
> +
> +	if (gp < ARMD_MAX_GPIO) {
> +		switch (GPIO_TO_REG(gp)) {
> +		case 0:
> +			gp_val = readl(&gpio_regs->gplr0);
> +			break;
> +		case 1:
> +			gp_val = readl(&gpio_regs->gplr1);
> +			break;
> +		case 2:
> +			gp_val = readl(&gpio_regs->gplr2);
> +			break;
> +		case 3:
> +			gp_val = readl(&gpio_regs->gplr3);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return GPIO_VAL(gp, gp_val);
> +}
> +
> +void gpio_set_value(int gp, int value)
> +{
> +	struct armdgpio_registers *gpio_regs =
> +		(struct armdgpio_registers *)ARMD1_GPIO_BASE;
> +
> +	if (gp < ARMD_MAX_GPIO) {
> +		switch (GPIO_TO_REG(gp)) {
> +		case 0:
> +			if (value)
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr0);
> +			else
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr0);
> +			break;
> +		case 1:
> +			if (value)
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr1);
> +			else
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr1);
> +			break;
> +		case 2:
> +			if (value)
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr2);
> +			else
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr2);
> +			break;
> +		case 3:
> +			if (value)
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpsr3);
> +			else
> +				writel(GPIO_TO_BIT(gp),	&gpio_regs->gpcr3);
> +			break;
> +		default:
> +			panic("Invalid GPIO pin %u\n", gp);
> +		}
> +	} else {
> +		panic("Invalid GPIO pin %u\n", gp);

Can you eliminate one panic line here?

Regards..
Prafulla . .

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Ajay Bhargav]

> How about merge this into current mvmfp.c? Just add some function
> into
> it, then no need another c file.
> 
> Best regards,
> Lei
> 

Hi Lei,

According to current ongoing development there is generic GPIO framework being introduced. Its good if we keep gpio separate though they are connected to MFP too, It makes more sense if they are kept in different file. lets see what Prafulla has to say about this.

Regards,
Ajay Bhargav

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Lei Wen]

How about merge this into current mvmfp.c? Just add some function into
it, then no need another c file.

Best regards,
Lei

On Tue, Jul 19, 2011 at 3:01 AM, Prafulla Wadaskar <prafulla@marvell.com> wrote:
>
>
>> -----Original Message-----
>> From: Ajay Bhargav [mailto:ajay.bhargav at einfochips.com]
>> Sent: Monday, July 18, 2011 3:12 PM
>> To: Prafulla Wadaskar
>> Cc: u-boot at lists.denx.de; Ajay Bhargav
>> Subject: [PATCH 1/4] gpio: Adds GPIO driver support for Armada100
>
> Correct patch name as
> gpio: Add GPIO driver for Marvell SoC Armada100
>
>>
>> This patch adds generic GPIO driver framework support for Armada100
>> series.
>>
>> To enable GPIO driver define CONFIG_ARMADA100_GPIO and for GPIO commands
>> define CONFIG_CMD_GPIO in your board configuration file.
>>
>> NOTE: This driver assumes proper configuration of MFP register for the
>> GPIO
>> in use.
>>
>> These patches depends on patch series that adds support for Marvell
>> Guruplug-Display [1,2].
>>
>> Signed-off-by: Ajay Bhargav <ajay.bhargav@einfochips.com>
>> ---
>> ?arch/arm/include/asm/arch-armada100/gpio.h | ?130 +++++++++++++++++++
>> ?drivers/gpio/Makefile ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
>> ?drivers/gpio/armada100_gpio.c ? ? ? ? ? ? ?| ?192
>> ++++++++++++++++++++++++++++
>> ?3 files changed, 323 insertions(+), 0 deletions(-)
>> ?create mode 100644 arch/arm/include/asm/arch-armada100/gpio.h
>> ?create mode 100644 drivers/gpio/armada100_gpio.c
>>
>> diff --git a/arch/arm/include/asm/arch-armada100/gpio.h
>> b/arch/arm/include/asm/arch-armada100/gpio.h
>> new file mode 100644
>> index 0000000..9024a2e
>> --- /dev/null
>> +++ b/arch/arm/include/asm/arch-armada100/gpio.h
>> @@ -0,0 +1,130 @@
>> +/*
>> + * (C) Copyright 2010
>
> 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
>> + */
>> +
>> +#ifndef _ASM_ARCH_GPIO_H
>> +#define _ASM_ARCH_GPIO_H
>> +
>> +#ifndef __ASSEMBLY__
>> +#include <asm/types.h>
>> +#endif
>> +
>> +#if defined(CONFIG_ARMADA100_GPIO)
>> +#include <asm/arch/armada100.h>
>> +
>> +#define GPIO_LABEL_MAX ? ? ? ? ? ? ? 20
>> +#define ARMD_MAX_GPIO ? ? ? ? ? ? ? ?128
>
> Pls derive it from MAX_MFP
>
>> +
>> +#define GPIO_TO_REG(gp) ? ? ? ? ? ? ?(gp >> 5)
>> +#define GPIO_TO_BIT(gp) ? ? ? ? ? ? ?(1 << (gp & 0x1F))
>> +#define GPIO_VAL(gp, val) ? ?((val >> (gp & 0x1F)) & 0x01)
>> +
>> +#define GPIO_SET ? ? ? ? ? ? 1
>> +#define GPIO_CLR ? ? ? ? ? ? 0
>> +
>> +/*
>> + * GPIO register map
>> + * Refer Datasheet Appendix A.36
>> + */
>> +struct armdgpio_registers {
>> + ? ? u32 gplr0; ? ? ?/* Pin Level Register - 0x0000 */
>> + ? ? u32 gplr1; ? ? ?/* 0x0004 */
>> + ? ? u32 gplr2; ? ? ?/* 0x0008 */
>> + ? ? u32 gpdr0; ? ? ?/* Pin Direction Register - 0x000C */
>
> NAK
>
> You should define all GPIO register in a single struct
> And point them with base address offsets
>
>
>> + ? ? u32 gpdr1; ? ? ?/* 0x0010 */
>> + ? ? u32 gpdr2; ? ? ?/* 0x0014 */
>> + ? ? u32 gpsr0; ? ? ?/* Pin Output Set Register - 0x0018 */
>> + ? ? u32 gpsr1; ? ? ?/* 0x001C */
>> + ? ? u32 gpsr2; ? ? ?/* 0x0020 */
>> + ? ? u32 gpcr0; ? ? ?/* Pin Output Clear Register - 0x0024 */
>> + ? ? u32 gpcr1; ? ? ?/* 0x0028 */
>> + ? ? u32 gpcr2; ? ? ?/* 0x002C */
>> + ? ? u32 grer0; ? ? ?/* Rising-Edge Detect Enable Register - 0x0030 */
>> + ? ? u32 grer1; ? ? ?/* 0x0034 */
>> + ? ? u32 grer2; ? ? ?/* 0x0038 */
>> + ? ? u32 gfer0; ? ? ?/* Falling-Edge Detect Enable Register - 0x003C */
>> + ? ? u32 gfer1; ? ? ?/* 0x0040 */
>> + ? ? u32 gfer2; ? ? ?/* 0x0044 */
>> + ? ? u32 gedr0; ? ? ?/* Edge Detect Status Register - 0x0048 */
>> + ? ? u32 gedr1; ? ? ?/* 0x004C */
>> + ? ? u32 gedr2; ? ? ?/* 0x0050 */
>> + ? ? u32 gsdr0; ? ? ?/* Bitwise Set of GPIO Direction Register - 0x0054 */
>> + ? ? u32 gsdr1; ? ? ?/* 0x0058 */
>> + ? ? u32 gsdr2; ? ? ?/* 0x005C */
>> + ? ? u32 gcdr0; ? ? ?/* Bitwise Clear of GPIO Direction Register - 0x0060 */
>> + ? ? u32 gcdr1; ? ? ?/* 0x0064 */
>> + ? ? u32 gcdr2; ? ? ?/* 0x0068 */
>> + ? ? u32 gsrer0; ? ? /* Bitwise Set of Rising-Edge Detect Enable
>> + ? ? ? ? ? ? ? ? ? ? ? ?Register - 0x006C */
>> + ? ? u32 gsrer1; ? ? /* 0x0070 */
>> + ? ? u32 gsrer2; ? ? /* 0x0074 */
>> + ? ? u32 gcrer0; ? ? /* Bitwise Clear of Rising-Edge Detect Enable
>> + ? ? ? ? ? ? ? ? ? ? ? ?Register - 0x0078 */
>> + ? ? u32 gcrer1; ? ? /* 0x007C */
>> + ? ? u32 gcrer2; ? ? /* 0x0080 */
>> + ? ? u32 gsfer0; ? ? /* Bitwise Set of Falling-Edge Detect Enable
>> + ? ? ? ? ? ? ? ? ? ? ? ?Register - 0x0084 */
>> + ? ? u32 gsfer1; ? ? /* 0x0088 */
>> + ? ? u32 gsfer2; ? ? /* 0x008C */
>> + ? ? u32 gcfer0; ? ? /* Bitwise Clear of Falling-Edge Detect Enable
>> + ? ? ? ? ? ? ? ? ? ? ? ?Register - 0x0090 */
>> + ? ? u32 gcfer1; ? ? /* 0x0094 */
>> + ? ? u32 gcfer2; ? ? /* 0x0098 */
>> + ? ? u32 apmask0; ? ?/* Bitwise Mask of Edge Detect Register - 0x009C */
>> + ? ? u32 apmask1; ? ?/* 0x00A0 */
>> + ? ? u32 apmask2; ? ?/* 0x00A4 */
>> + ? ? u8 pad[0x0100 - 0x00A4 - 4];
>> + ? ? u32 gplr3; ? ? ?/* 0x0100 */
>> + ? ? u8 pad1[0x010C - 0x0100 - 4];
>> + ? ? u32 gpdr3; ? ? ?/* 0x010C */
>> + ? ? u8 pad2[0x0118 - 0x010C - 4];
>> + ? ? u32 gpsr3; ? ? ?/* 0x0118 */
>> + ? ? u8 pad3[0x0124 - 0x0118 - 4];
>> + ? ? u32 gpcr3; ? ? ?/* 0x0124 */
>> + ? ? u8 pad4[0x0130 - 0x0124 - 4];
>> + ? ? u32 grer3; ? ? ?/* 0x0130 */
>> + ? ? u8 pad5[0x013C - 0x0130 - 4];
>> + ? ? u32 gfer3; ? ? ?/* 0x013C */
>> + ? ? u8 pad6[0x0148 - 0x013C - 4];
>> + ? ? u32 gedr3; ? ? ?/* 0x0148 */
>> + ? ? u8 pad7[0x0154 - 0x0148 - 4];
>> + ? ? u32 gsdr3; ? ? ?/* 0x0154 */
>> + ? ? u8 pad8[0x0160 - 0x0154 - 4];
>> + ? ? u32 gcdr3; ? ? ?/* 0x0160 */
>> + ? ? u8 pad9[0x016C - 0x0160 - 4];
>> + ? ? u32 gsrer3; ? ? /* 0x016C */
>> + ? ? u8 pad10[0x0178 - 0x016C - 4];
>> + ? ? u32 gcrer3; ? ? /* 0x0178 */
>> + ? ? u8 pad11[0x0184 - 0x0178 - 4];
>> + ? ? u32 gsfer3; ? ? /* 0x0184 */
>> + ? ? u8 pad12[0x0190 - 0x0184 - 4];
>> + ? ? u32 gcfer3; ? ? /* 0x0190 */
>> + ? ? u8 pad13[0x019C - 0x0190 - 4];
>> + ? ? u32 apmask3; ? ?/* 0x019C */
>> +};
>> +
>> +#endif /* CONFIG_ARMADA100_GPIO */
>> +#endif /* _ASM_ARCH_GPIO_H */
>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>> index 1e3ae11..31b83cd 100644
>> --- a/drivers/gpio/Makefile
>> +++ b/drivers/gpio/Makefile
>> @@ -26,6 +26,7 @@ include $(TOPDIR)/config.mk
>> ?LIB ?:= $(obj)libgpio.o
>>
>> ?COBJS-$(CONFIG_AT91_GPIO) ? ?+= at91_gpio.o
>> +COBJS-$(CONFIG_ARMADA100_GPIO) ? ? ? += armada100_gpio.o
>
> I suggest you to add mvgpio.c instead of armada100_gpio.c
> This will be used in future by other Marvell SoCs.
>
>> ?COBJS-$(CONFIG_KIRKWOOD_GPIO) ? ? ? ?+= kw_gpio.o
>> ?COBJS-$(CONFIG_MARVELL_MFP) ?+= mvmfp.o
>> ?COBJS-$(CONFIG_MXC_GPIO) ? ? += mxc_gpio.o
>> diff --git a/drivers/gpio/armada100_gpio.c
>> b/drivers/gpio/armada100_gpio.c
>> new file mode 100644
>> index 0000000..578fdac
>> --- /dev/null
>> +++ b/drivers/gpio/armada100_gpio.c
>> @@ -0,0 +1,192 @@
>> +/*
>> + * (C) Copyright 2010
>
> 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>
>> +
>> +int gpio_request(int gp, const char *label)
>> +{
>> + ? ? /*
>> + ? ? ?* Assumes corresponding MFP is configured peoperly
>> + ? ? ?* for use as GPIO
>> + ? ? ?*/
>
> NAK, you should check here, respective MFP is being configured as GPIO, if not you should return error
>
>> + ? ? return 0;
>> +}
>> +
>> +void gpio_free(int gp)
>> +{
>> + ? ? /* default direction for GPIO is input */
>> + ? ? gpio_direction_input(gp);
>> +}
>> +
>> +void gpio_toggle_value(int gp)
>> +{
>> + ? ? gpio_set_value(gp, !gpio_get_value(gp));
>> +}
>> +
>> +int gpio_direction_input(int gp)
>> +{
>> + ? ? struct armdgpio_registers *gpio_regs =
>> + ? ? ? ? ? ? (struct armdgpio_registers *)ARMD1_GPIO_BASE;
>
> Consider now this is generic GPIO driver for marvell SOCs, define this macro in gpio.h
>
>> +
>> + ? ? if (gp < ARMD_MAX_GPIO) {
>> + ? ? ? ? ? ? switch (GPIO_TO_REG(gp)) {
>
> Some code comments are welcomed here.
>
>> + ? ? ? ? ? ? case 0:
>> + ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr0);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? case 1:
>> + ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr1);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? case 2:
>> + ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr2);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? case 3:
>> + ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr3);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? default:
>> + ? ? ? ? ? ? ? ? ? ? return -EINVAL;
>> + ? ? ? ? ? ? }
>> + ? ? } else {
>> + ? ? ? ? ? ? return -EINVAL;
>> + ? ? }
>> + ? ? return 0;
>> +}
>> +
>> +int gpio_direction_output(int gp, int value)
>> +{
>> + ? ? struct armdgpio_registers *gpio_regs =
>> + ? ? ? ? ? ? (struct armdgpio_registers *)ARMD1_GPIO_BASE;
>> +
>> + ? ? if (gp < ARMD_MAX_GPIO) {
>> + ? ? ? ? ? ? switch (GPIO_TO_REG(gp)) {
>> + ? ? ? ? ? ? case 0:
>> + ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr0);
>
> Call gpio_set_value(gp, value) here which is defined below doing the same
>
>
>> + ? ? ? ? ? ? ? ? ? ? if (value)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr0);
>> + ? ? ? ? ? ? ? ? ? ? else
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr0);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? case 1:
>> + ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr1);
>> + ? ? ? ? ? ? ? ? ? ? if (value)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr1);
>> + ? ? ? ? ? ? ? ? ? ? else
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr1);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? case 2:
>> + ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr2);
>> + ? ? ? ? ? ? ? ? ? ? if (value)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr2);
>> + ? ? ? ? ? ? ? ? ? ? else
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr2);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? case 3:
>> + ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr3);
>> + ? ? ? ? ? ? ? ? ? ? if (value)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr3);
>> + ? ? ? ? ? ? ? ? ? ? else
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr3);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? default:
>> + ? ? ? ? ? ? ? ? ? ? return -EINVAL;
>> + ? ? ? ? ? ? }
>> + ? ? } else {
>> + ? ? ? ? ? ? return -EINVAL;
>> + ? ? }
>> + ? ? return 0;
>> +}
>> +
>> +int gpio_get_value(int gp)
>> +{
>> + ? ? struct armdgpio_registers *gpio_regs =
>> + ? ? ? ? ? ? (struct armdgpio_registers *)ARMD1_GPIO_BASE;
>> + ? ? u32 gp_val;
>> +
>> + ? ? if (gp < ARMD_MAX_GPIO) {
>> + ? ? ? ? ? ? switch (GPIO_TO_REG(gp)) {
>> + ? ? ? ? ? ? case 0:
>> + ? ? ? ? ? ? ? ? ? ? gp_val = readl(&gpio_regs->gplr0);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? case 1:
>> + ? ? ? ? ? ? ? ? ? ? gp_val = readl(&gpio_regs->gplr1);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? case 2:
>> + ? ? ? ? ? ? ? ? ? ? gp_val = readl(&gpio_regs->gplr2);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? case 3:
>> + ? ? ? ? ? ? ? ? ? ? gp_val = readl(&gpio_regs->gplr3);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? default:
>> + ? ? ? ? ? ? ? ? ? ? return -EINVAL;
>> + ? ? ? ? ? ? }
>> + ? ? } else {
>> + ? ? ? ? ? ? return -EINVAL;
>> + ? ? }
>> +
>> + ? ? return GPIO_VAL(gp, gp_val);
>> +}
>> +
>> +void gpio_set_value(int gp, int value)
>> +{
>> + ? ? struct armdgpio_registers *gpio_regs =
>> + ? ? ? ? ? ? (struct armdgpio_registers *)ARMD1_GPIO_BASE;
>> +
>> + ? ? if (gp < ARMD_MAX_GPIO) {
>> + ? ? ? ? ? ? switch (GPIO_TO_REG(gp)) {
>> + ? ? ? ? ? ? case 0:
>> + ? ? ? ? ? ? ? ? ? ? if (value)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr0);
>> + ? ? ? ? ? ? ? ? ? ? else
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr0);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? case 1:
>> + ? ? ? ? ? ? ? ? ? ? if (value)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr1);
>> + ? ? ? ? ? ? ? ? ? ? else
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr1);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? case 2:
>> + ? ? ? ? ? ? ? ? ? ? if (value)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr2);
>> + ? ? ? ? ? ? ? ? ? ? else
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr2);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? case 3:
>> + ? ? ? ? ? ? ? ? ? ? if (value)
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpsr3);
>> + ? ? ? ? ? ? ? ? ? ? else
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? writel(GPIO_TO_BIT(gp), &gpio_regs->gpcr3);
>> + ? ? ? ? ? ? ? ? ? ? break;
>> + ? ? ? ? ? ? default:
>> + ? ? ? ? ? ? ? ? ? ? panic("Invalid GPIO pin %u\n", gp);
>> + ? ? ? ? ? ? }
>> + ? ? } else {
>> + ? ? ? ? ? ? panic("Invalid GPIO pin %u\n", gp);
>
> Can you eliminate one panic line here?
>
> Regards..
> Prafulla . .
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Ajay Bhargav]

----- "Lei Wen" <adrian.wenl@gmail.com> wrote:

> Hi Ajay,
> 
> On Tue, Jul 19, 2011 at 12:01 PM, Ajay Bhargav
> <ajay.bhargav@einfochips.com> wrote:
> >
> >> How about merge this into current mvmfp.c? Just add some function
> >> into
> >> it, then no need another c file.
> >>
> >> Best regards,
> >> Lei
> >>
> >
> > Hi Lei,
> >
> > According to current ongoing development there is generic GPIO
> framework being introduced. Its good if we keep gpio separate though
> they are connected to MFP too, It makes more sense if they are kept in
> different file. lets see what Prafulla has to say about this.
> >
> Ok.
> BTW, I also have some comments towards your patch.
> You define a huge structure in
> arch/arm/include/asm/arch-armada100/gpio.h, which don't looks so good
> to me.
> 

Comments are welcome. I know its huge.. I just followed what is suggested by Wolfgang.
He told not to use BASE+OFFSET thing. I am bit confused here whom to follow :)

> 
> Macro here save a lot space, and keep the code clean.
> 
> Best regards,
> Lei
> 

Prafulla is suggesting something.. Let me ask him how he want this.

Regards,
Ajay Bhargav

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Lei Wen]

Hi Ajay,

On Tue, Jul 19, 2011 at 12:01 PM, Ajay Bhargav
<ajay.bhargav@einfochips.com> wrote:
>
>> How about merge this into current mvmfp.c? Just add some function
>> into
>> it, then no need another c file.
>>
>> Best regards,
>> Lei
>>
>
> Hi Lei,
>
> According to current ongoing development there is generic GPIO framework being introduced. Its good if we keep gpio separate though they are connected to MFP too, It makes more sense if they are kept in different file. lets see what Prafulla has to say about this.
>
Ok.
BTW, I also have some comments towards your patch.
You define a huge structure in
arch/arm/include/asm/arch-armada100/gpio.h, which don't looks so good
to me.

In our code base, that file only keeps as short as:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
#define BANK_OFF(n)     (((n) < 3) ? (n) << 2 : 0x100 + (((n) - 3) << 2))
#define GPIO_REG(x)     (*((volatile u32 *)(CONFIG_GPIO_REGBASE + (x))))

#define NR_BUILTIN_GPIO (128)

#define GPIO_bit(gpio)  (1 << ((gpio) & 0x1f))

#define GPLR(x)         GPIO_REG(BANK_OFF((x) >> 5) + 0x00)
#define GPDR(x)         GPIO_REG(BANK_OFF((x) >> 5) + 0x0c)
#define GPSR(x)         GPIO_REG(BANK_OFF((x) >> 5) + 0x18)
#define GPCR(x)         GPIO_REG(BANK_OFF((x) >> 5) + 0x24)

int gpio_get_value(unsigned gpio);
void gpio_set_value(unsigned gpio, int value);
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

Macro here save a lot space, and keep the code clean.

Best regards,
Lei

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Ajay Bhargav]

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

> You should define all GPIO register in a single struct
> And point them with base address offsets
> 

> 
> I suggest you to add mvgpio.c instead of armada100_gpio.c
> This will be used in future by other Marvell SoCs.
> 
GPIO registers might vary for other Marvell SoCs? How to deal with
that?

> > +int gpio_request(int gp, const char *label)
> > +{
> > +	/*
> > +	 * Assumes corresponding MFP is configured peoperly
> > +	 * for use as GPIO
> > +	 */
> 
> NAK, you should check here, respective MFP is being configured as
> GPIO, if not you should return error
> 
I checked datasheet and for most GPIOs, AF1 is given as GPIO but for few
its not, so adding a glue logic to check for specific GPIOs wont be a good idea.
Thats the reason i thought its good to keep MFP out of this. Please give suggestions.

> 
> Consider now this is generic GPIO driver for marvell SOCs, define this
> macro in gpio.h
> 

ok

> > +
> > +	if (gp < ARMD_MAX_GPIO) {
> > +		switch (GPIO_TO_REG(gp)) {
> 
> Some code comments are welcomed here.
> 
> > +		case 0:
> > +			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr0);
> > +			break;
> > +		case 1:
> > +			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr1);
> > +			break;
> > +		case 2:
> > +			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr2);
> > +			break;
> > +		case 3:
> > +			writel(GPIO_TO_BIT(gp), &gpio_regs->gcdr3);
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +	return 0;
> > +}
> > +
> > +int gpio_direction_output(int gp, int value)
> > +{
> > +	struct armdgpio_registers *gpio_regs =
> > +		(struct armdgpio_registers *)ARMD1_GPIO_BASE;
> > +
> > +	if (gp < ARMD_MAX_GPIO) {
> > +		switch (GPIO_TO_REG(gp)) {
> > +		case 0:
> > +			writel(GPIO_TO_BIT(gp), &gpio_regs->gsdr0);
> 
> Call gpio_set_value(gp, value) here which is defined below doing the
> same
> 
thanks for suggestion.

> > +			panic("Invalid GPIO pin %u\n", gp);
> > +		}
> > +	} else {
> > +		panic("Invalid GPIO pin %u\n", gp);
> 
> Can you eliminate one panic line here?
> 
> Regards..
> Prafulla . .
> 
will do that... there is no return value from this function so i thought
it wont be good to continue from this point incase designed direction is not set.

Regards,
Ajay Bhargav

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Wolfgang Denk]

Dear Lei Wen,

A: Full quoting.

Q: What is the second most annoying thing in e-mail?

A: Because it messes up the order in which people normally read text.

Q: Why is top-posting such a bad thing?

A: Top-posting.

Q: What is the most annoying thing in e-mail?


In message <CALZhoSTVOiLQ2Br9A1bDEbE=9XjSHUs=402cFfqP_qQ2S0bZDw@mail.gmail.com> you wrote:
> How about merge this into current mvmfp.c? Just add some function into
> it, then no need another c file.
...
> >> -----Original Message-----

500 lines full quote deleted.


It makes ZERO sense to quote 500 lines of text that you are not really
referring to.

Please STOP top posting / full quoting.  If you need help, please
read http://www.netmeister.org/news/learn2quote.html

Thanks.


Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"One planet is all you get."

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Lei Wen]

Hi Wolfgang

On Tue, Jul 19, 2011 at 1:27 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> A: Full quoting.
>
> Q: What is the second most annoying thing in e-mail?
>
> A: Because it messes up the order in which people normally read text.
>
> Q: Why is top-posting such a bad thing?
>
> A: Top-posting.
>
> Q: What is the most annoying thing in e-mail?
>
Understand. Sorry for this...

Best regards,
Lei

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Prafulla Wadaskar]

> -----Original Message-----
> From: Ajay Bhargav [mailto:ajay.bhargav at einfochips.com]
> Sent: Tuesday, July 19, 2011 9:53 AM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de; Ashish Karkare; Prabhanjan Sarnaik
> Subject: Re: [PATCH 1/4] gpio: Adds GPIO driver support for Armada100
> 
> 
> ----- "Prafulla Wadaskar" <prafulla@marvell.com> wrote:
> 
> > You should define all GPIO register in a single struct
> > And point them with base address offsets
> >
> 
> >
> > I suggest you to add mvgpio.c instead of armada100_gpio.c
> > This will be used in future by other Marvell SoCs.
> >
> GPIO registers might vary for other Marvell SoCs? How to deal with
> that?

That can be thought of while adding support for othe SoCs.
Preferably define register struct in asm/arch/gpio.h

Regards..
Prafulla . .

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Prafulla Wadaskar]

> -----Original Message-----
> From: Ajay Bhargav [mailto:ajay.bhargav at einfochips.com]
> Sent: Tuesday, July 19, 2011 9:44 AM
> To: Lei Wen
> Cc: u-boot at lists.denx.de; Ashish Karkare; Prabhanjan Sarnaik; Prafulla
> Wadaskar
> Subject: Re: [U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for
> Armada100
> 
> 
> ----- "Lei Wen" <adrian.wenl@gmail.com> wrote:
> 
> > Hi Ajay,
> >
> > On Tue, Jul 19, 2011 at 12:01 PM, Ajay Bhargav
> > <ajay.bhargav@einfochips.com> wrote:
> > >
> > >> How about merge this into current mvmfp.c? Just add some function
> > >> into
> > >> it, then no need another c file.
> > > According to current ongoing development there is generic GPIO
> > framework being introduced. Its good if we keep gpio separate though
> > they are connected to MFP too, It makes more sense if they are kept in
> > different file. lets see what Prafulla has to say about this.

I think mvgpio.c will address gpio driver, it can be used by Kirkwood, Orion arch too where as MFP is totally different on this core than armada.

To me it makes more sense to call mvgpio.c as gpio driver and mvmfp.c as Multi Function Pin driver.

> > >
> > Ok.
> > BTW, I also have some comments towards your patch.
> > You define a huge structure in
> > arch/arm/include/asm/arch-armada100/gpio.h, which don't looks so good
> > to me.
> >
> 
> Comments are welcome. I know its huge.. I just followed what is
> suggested by Wolfgang.
> He told not to use BASE+OFFSET thing. I am bit confused here whom to
> follow :)

You have to follow all :-), more reviewers more better code output.
BASE+OFFSET strictly not recommended.

> 
> >
> > Macro here save a lot space, and keep the code clean.
> >
> > Best regards,
> > Lei
> >
> 
> Prafulla is suggesting something.. Let me ask him how he want this.

I think lei and me are suggesting similar things, macros should be used precisely, the code should be small and smarter.

Regards..
Prafulla . .

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Lei Wen]

Hi Ajay,

On Tue, Jul 19, 2011 at 12:23 PM, Ajay Bhargav
<ajay.bhargav@einfochips.com> wrote:
>
> ----- "Prafulla Wadaskar" <prafulla@marvell.com> wrote:
>
>> You should define all GPIO register in a single struct
>> And point them with base address offsets
>>
>
>>
>> I suggest you to add mvgpio.c instead of armada100_gpio.c
>> This will be used in future by other Marvell SoCs.
>>
> GPIO registers might vary for other Marvell SoCs? How to deal with
> that?

I could tell you that for the pxa series branch, the gpio register arrangement
not change for years, and still keep it valid for the next generation product.

So mvgpio.c could be used by pxa168(aspen), pxa910(td), pxa610(mmp2),
pxa620(mmp3).

Best regards,
Lei

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Ajay Bhargav]

----- "Lei Wen" <adrian.wenl@gmail.com> wrote:

> Hi Ajay,
> 
> On Wed, Jul 20, 2011 at 6:43 PM, Ajay Bhargav
> <ajay.bhargav@einfochips.com> wrote:
> > Hi Lei,
> >
> >> I think we make thing complicated here. For GPIO driver, the only
> >> structure we need to
> >> define is the GPIO register itself, like GPIO_PLR, GPIO_PDR,
> etc...
> >>
> >
> > I got your point, but lemme show you where the problem is..
> > GPIO_PLR0 ? ?0x0000
> > GPIO_PLR1 ? ?0x0004
> > GPIO_PLR2 ? ?0x0008
> > GPIO_PLR3 ? ?0x0100
> >
> > till 3 registers its fine... fourth register is way out of league.
> >
> 
> I know this is a bit tricky, but couldn't be included in a logic
> hidden by MACRO or funcion?
> 
> Best regards,
> Lei
> 

Hi Lei,

I am rewriting the whole logic. Will submit the patch soon :)
I will use function to get base address.

Regards,
Ajay Bhargav

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Wolfgang Denk]

Dear Ajay Bhargav,

In message <1499501074.35394.1311158638174.JavaMail.root@ahm.einfochips.com> you wrote:
> Hi Lei,
> 
> > I think we make thing complicated here. For GPIO driver, the only
> > structure we need to
> > define is the GPIO register itself, like GPIO_PLR, GPIO_PDR, etc...
> > 
> 
> I got your point, but lemme show you where the problem is..
> GPIO_PLR0    0x0000
> GPIO_PLR1    0x0004
> GPIO_PLR2    0x0008
> GPIO_PLR3    0x0100
> 
> till 3 registers its fine... fourth register is way out of league.

In which way?  Just because there is a gap between?  No problem.
Insert a filler element to your struct.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
How many seconds are there in a year? If I tell you there are 3.155 x
10^7, you won't even try to remember it. On the other hand, who could
forget that, to within half a percent, pi seconds is  a  nanocentury.
                                               -- Tom Duff, Bell Labs

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Lei Wen]

Hi Ajay,

On Wed, Jul 20, 2011 at 6:43 PM, Ajay Bhargav
<ajay.bhargav@einfochips.com> wrote:
> Hi Lei,
>
>> I think we make thing complicated here. For GPIO driver, the only
>> structure we need to
>> define is the GPIO register itself, like GPIO_PLR, GPIO_PDR, etc...
>>
>
> I got your point, but lemme show you where the problem is..
> GPIO_PLR0 ? ?0x0000
> GPIO_PLR1 ? ?0x0004
> GPIO_PLR2 ? ?0x0008
> GPIO_PLR3 ? ?0x0100
>
> till 3 registers its fine... fourth register is way out of league.
>

I know this is a bit tricky, but couldn't be included in a logic
hidden by MACRO or funcion?

Best regards,
Lei

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Wolfgang Denk]

Dear Ajay Bhargav,

In message <794341286.33968.1311143785027.JavaMail.root@ahm.einfochips.com> you wrote:
> 
> > Is there any specific reason for not using u32 for the padding as
> > well?
> > 
> nothing specific. It makes easy to find number of bytes than words.

If there is no specific reason (which I could not think of either),
then please use u32 consistently.

If you think in number of bytes, feel free to write

	u32	reserved[128/sizeof(u32)];

> > Why would you need this BASE + OFFSET notation when using a C struct
> > for the registers? Thi smakes little sense to me.
> 
> Well I did use the C struct method, if you see my patches submitted earlier
> but according to Prafulla and Lie structure size is too big. so they want me

Who cares about the size of the struct?  We never alocate any memory
for such a structure - it is just an overlay over the existing
register space, so nobody cares if this is 16 kB of 16 MB.

> to use a mix of C struct and BASE + OFFSET notation. so I thought to break the big
> C struct into smaller grouped structures pointing each group with GPIO base + group
> offset. I would be glad if you can suggest me something better or smarter.

This may make sense of you have separate, logically independent IP
blocks.  But the reason for such a desicion is if the data belong
together in any way or not.

The "size" of such a structure is completely irrelevant.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Pray to God, but keep rowing to shore. - Russian Proverb

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Ajay Bhargav]

Hi Lei,

> I think we make thing complicated here. For GPIO driver, the only
> structure we need to
> define is the GPIO register itself, like GPIO_PLR, GPIO_PDR, etc...
> 

I got your point, but lemme show you where the problem is..
GPIO_PLR0    0x0000
GPIO_PLR1    0x0004
GPIO_PLR2    0x0008
GPIO_PLR3    0x0100

till 3 registers its fine... fourth register is way out of league.

Regards,
Ajay Bhargav

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Lei Wen]

Hi Ajay,

On Wed, Jul 20, 2011 at 2:36 PM, Ajay Bhargav
<ajay.bhargav@einfochips.com> wrote:
> Dear Wolfgang,
>
>>
>> Is there any specific reason for not using u32 for the padding as
>> well?
>>
> nothing specific. It makes easy to find number of bytes than words.
>
>>
>> Why would you need this BASE + OFFSET notation when using a C struct
>> for the registers? Thi smakes little sense to me.
>>
>
> Well I did use the C struct method, if you see my patches submitted earlier
> but according to Prafulla and Lie structure size is too big. so they want me
> to use a mix of C struct and BASE + OFFSET notation. so I thought to break the big
> C struct into smaller grouped structures pointing each group with GPIO base + group
> offset. I would be glad if you can suggest me something better or smarter.
>

I think we make thing complicated here. For GPIO driver, the only
structure we need to
define is the GPIO register itself, like GPIO_PLR, GPIO_PDR, etc...

For the previous long huge structure and include your recent short
version is still not good
enough. What we want to get is the base address, and based on the
register structure
to do explicit work. So either get the base address based on MACRO, or
get from a function
I think both should be ok for me. But please not continue work on how
to define the base
address into a structure.

Best regards,
Lei

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Ajay Bhargav]

----- "Lei Wen" <adrian.wenl@gmail.com> wrote:

> I mean why there has to be one request() function call is need?
> Is this mandatory for the gpio general framwork as you said?
> 
> Best regards,
> Lei
> 

If you're enabling GPIO command support (CONFIG_CMD_GPIO), Its necessary. and yes its a
part of framework.

Regards,
Ajay Bhargav

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Lei Wen]

On Wed, Jul 20, 2011 at 3:29 PM, Ajay Bhargav
<ajay.bhargav@einfochips.com> wrote:
> Hi Lei,
>
>> I am not mixing those two concept together, and in our pratice, we
>> also do as you said,
>> use mfp to set that pin to GPIO state, and use gpio function to
>> manupulate the gpio.
>> So there is no need checking MFP setting for gpio requreset. Directly
>> set would be ok.
>
> exactly... so here is code snip from my patch and Prafulla's reply.
> ---
>> > +int gpio_request(int gp, const char *label)
>> > +{
>> > + ? ? ? ?/*
>> > + ? ? ? ? * Assumes corresponding MFP is configured peoperly
>> > + ? ? ? ? * for use as GPIO
>> > + ? ? ? ? */
>>
>> NAK, you should check here, respective MFP is being configured as GPIO, if not you should return error
>>
>> > + ? ? ? ?return 0;
>> > +}
>> > +
> ---
>
>> BTW, why there is need the gpio_request function?
>>
> You can request a pin as GPIO for using within your code. In Linux Kernel
> source this function checks for valid number and if requested pin is in use
> or not. To check this they have used a very simple logic.
> ..pseudo code..
> if(pin_label == NULL)
> ? ?pin is free
> else
> ? ?pin in use
>
> I think I should do the same thing in my request function rather than going for
> complicated stuff, what you say?
>

I mean why there has to be one request() function call is need?
Is this mandatory for the gpio general framwork as you said?

Best regards,
Lei

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Ajay Bhargav]

Hi Lei,

> I am not mixing those two concept together, and in our pratice, we
> also do as you said,
> use mfp to set that pin to GPIO state, and use gpio function to
> manupulate the gpio.
> So there is no need checking MFP setting for gpio requreset. Directly
> set would be ok.

exactly... so here is code snip from my patch and Prafulla's reply.
---
> > +int gpio_request(int gp, const char *label)
> > +{
> > +        /*
> > +         * Assumes corresponding MFP is configured peoperly
> > +         * for use as GPIO
> > +         */
> 
> NAK, you should check here, respective MFP is being configured as GPIO, if not you should return error
> 
> > +        return 0;
> > +}
> > +
---

> BTW, why there is need the gpio_request function?
> 
You can request a pin as GPIO for using within your code. In Linux Kernel
source this function checks for valid number and if requested pin is in use
or not. To check this they have used a very simple logic.
..pseudo code..
if(pin_label == NULL)
    pin is free
else
    pin in use

I think I should do the same thing in my request function rather than going for
complicated stuff, what you say?

> 
> Actually, we use the same gpio driver for several series till now.
> And configure pin as GPIO(AF0 or whatever) is the business of MFP.
> 
right.. 

Regards,
Ajay Bhargav

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Lei Wen]

On Wed, Jul 20, 2011 at 3:14 PM, Ajay Bhargav
<ajay.bhargav@einfochips.com> wrote:
> Hi Lei,
>
>> Actually, as uboot target at small size, I tend to don't add too much
>> logic to it. So there is no need to
>> check the "MAX", if one need to set the gpio, he should notice by
>> himself, the gpio number he specified
>> is a valid gpio address in the system.
>
> Yeah so i made this assumption already that gpio entered has to be valid.
> But do you think checking MFP setup for each gpio requested is good? this way
> we are mixing MFP with GPIO again. If a person want to use a GPIO he should
> know that MFP has to be configured before using as GPIO. That's the reason we
> have a mfp driver.
>
I am not mixing those two concept together, and in our pratice, we
also do as you said,
use mfp to set that pin to GPIO state, and use gpio function to
manupulate the gpio.
So there is no need checking MFP setting for gpio requreset. Directly
set would be ok.
BTW, why there is need the gpio_request function?

> and I don't think MFP logic written once for one series can be applied to all series
> to find out if Selected function is GPIO or not. In Armada100 series not all MFPs have
> AF0 as GPIO. That's the reason I left gpio_request function returning only 0.

Actually, we use the same gpio driver for several series till now.
And configure pin as GPIO(AF0 or whatever) is the business of MFP.

Best regards,
Lei

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Ajay Bhargav]

Hi Lei,

> Actually, as uboot target at small size, I tend to don't add too much
> logic to it. So there is no need to
> check the "MAX", if one need to set the gpio, he should notice by
> himself, the gpio number he specified
> is a valid gpio address in the system.

Yeah so i made this assumption already that gpio entered has to be valid.
But do you think checking MFP setup for each gpio requested is good? this way
we are mixing MFP with GPIO again. If a person want to use a GPIO he should
know that MFP has to be configured before using as GPIO. That's the reason we
have a mfp driver.

and I don't think MFP logic written once for one series can be applied to all series
to find out if Selected function is GPIO or not. In Armada100 series not all MFPs have
AF0 as GPIO. That's the reason I left gpio_request function returning only 0.

Even in Linux kernel source if you see, gpio_request just checks for valid pin gpio number
nothing more than that. I seriously need some input to move ahead.

Regards,
Ajay Bhargav

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Lei Wen]

Hi Ajay,

On Tue, Jul 19, 2011 at 6:29 PM, Ajay Bhargav
<ajay.bhargav@einfochips.com> wrote:
> Hi Prafulla,
>
>> I checked datasheet and for most GPIOs, AF1 is given as GPIO but for few
>> its not, so adding a glue logic to check for specific GPIOs wont be a good idea.
>> That's the reason i thought its good to keep MFP out of this. Please give suggestions.
>
> correcting my previous message, I meant AF0.
> adding to this.. I checked Linux kernel source and they are simply validating if gpio number
> is within max limits so request returns 0.
>
> And you told me to use MAX_MFP instead of using ARMD_MAX_GPIO, but MAX_MFP is defined lesser
> than the actual number of GPIO e.g. in case of armada100 MAX_MFP is defined 117 whereas max
> GPIOs are around 123.
>
> Please give me some suggestion.

Actually, as uboot target at small size, I tend to don't add too much
logic to it. So there is no need to
check the "MAX", if one need to set the gpio, he should notice by
himself, the gpio number he specified
is a valid gpio address in the system.

Best regards,
Lei

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Ajay Bhargav]

Dear Wolfgang,

> 
> Is there any specific reason for not using u32 for the padding as
> well?
> 
nothing specific. It makes easy to find number of bytes than words.

> 
> Why would you need this BASE + OFFSET notation when using a C struct
> for the registers? Thi smakes little sense to me.
> 

Well I did use the C struct method, if you see my patches submitted earlier
but according to Prafulla and Lie structure size is too big. so they want me
to use a mix of C struct and BASE + OFFSET notation. so I thought to break the big
C struct into smaller grouped structures pointing each group with GPIO base + group
offset. I would be glad if you can suggest me something better or smarter.

Regards,
Ajay Bhargav

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Wolfgang Denk]

Dear Ajay Bhargav,

In message <1207509190.33599.1311140198703.JavaMail.root@ahm.einfochips.com> you wrote:
> 
> e.g. 
> struct armdgpio_gplr_register {
>     u32 gplr0;
>     u32 gplr1;
>     u32 gplr2;
>     u8 pad[some_value]; //this padding is going to be big
>     u32 gplr3;
> }

Is there any specific reason for not using u32 for the padding as
well?

> then while using this particular set i can just use ARMD1_GPLR_BASE
> ( ARMD1_GPIO_BASE + GPLR_OFFSET). moreover i am not using all the
> registers so i define only those register sets which are in use. what
> you say about this?

Why would you need this BASE + OFFSET notation when using a C struct
for the registers? Thi smakes little sense to me.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The thing is, as you progress in the Craft,  you'll  learn  there  is
another rule... When you break rules, break 'em good and hard.
                                    - Terry Pratchett, _Wyrd Sisters_

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Ajay Bhargav]

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

> 
> That can be thought of while adding support for othe SoCs.
> Preferably define register struct in asm/arch/gpio.h
> 
> Regards..
> Prafulla . .
> 

..snip.. (quoting from another reply..)
> You have to follow all :-), more reviewers more better code output.
> BASE+OFFSET strictly not recommended.

> I think lei and me are suggesting similar things, macros should be used precisely, the code should be small and smarter.

Hi Prafulla,

I agree that macros make code look smaller and smarter. Now if you see the registers of GPIO they are not in order, I mean i cannot group together particular gpio set. can i do it this way, 

e.g. 
struct armdgpio_gplr_register {
    u32 gplr0;
    u32 gplr1;
    u32 gplr2;
    u8 pad[some_value]; //this padding is going to be big
    u32 gplr3;
}

then while using this particular set i can just use ARMD1_GPLR_BASE ( = ARMD1_GPIO_BASE + GPLR_OFFSET). moreover i am not using all the registers so i define only those register sets which are in use. what you say about this?

Regards,
Ajay Bhargav

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Prafulla Wadaskar]

> -----Original Message-----
> From: Ajay Bhargav [mailto:ajay.bhargav at einfochips.com]
> Sent: Tuesday, July 19, 2011 3:59 PM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de; Ashish Karkare; Prabhanjan Sarnaik
> Subject: Re: [PATCH 1/4] gpio: Adds GPIO driver support for Armada100
> 
> Hi Prafulla,
> 
> > I checked datasheet and for most GPIOs, AF1 is given as GPIO but for
> few
> > its not, so adding a glue logic to check for specific GPIOs wont be a
> good idea.
> > That's the reason i thought its good to keep MFP out of this. Please
> give suggestions.
> 
> correcting my previous message, I meant AF0.
> adding to this.. I checked Linux kernel source and they are simply
> validating if gpio number
> is within max limits so request returns 0.
> 
> And you told me to use MAX_MFP instead of using ARMD_MAX_GPIO, but
> MAX_MFP is defined lesser
> than the actual number of GPIO e.g. in case of armada100 MAX_MFP is
> defined 117 whereas max
> GPIOs are around 123.

I will check and let you know.

Regards..
Prafulla . .

[U-Boot] [PATCH 1/4] gpio: Adds GPIO driver support for Armada100

[Ajay Bhargav]

Hi Prafulla,

> I checked datasheet and for most GPIOs, AF1 is given as GPIO but for few
> its not, so adding a glue logic to check for specific GPIOs wont be a good idea.
> That's the reason i thought its good to keep MFP out of this. Please give suggestions.

correcting my previous message, I meant AF0.
adding to this.. I checked Linux kernel source and they are simply validating if gpio number
is within max limits so request returns 0.

And you told me to use MAX_MFP instead of using ARMD_MAX_GPIO, but MAX_MFP is defined lesser
than the actual number of GPIO e.g. in case of armada100 MAX_MFP is defined 117 whereas max
GPIOs are around 123.

Please give me some suggestion.

Regards,
Ajay Bhargav