[U-Boot] [PATCH] MX: Set a common gpio.h for all i.MX

["Benoît Thébaudeau" <benoit.thebaudeau@advansee.com>]

Hi Stefano,

> Each i.MX has its own gpio.h, defining the same structure.
> The internal GPIO controller has the same layout
> (at least for the register used by u-boot) and can be shared.

Good!

> 
> Signed-off-by: Stefano Babic <sbabic@denx.de>
> CC: Matt Sealey <matt@genesi-usa.com>
> CC: Marek Vasut <marex@denx.de>
> CC: Benoit Thebaudeau <benoit.thebaudeau@advansee.com>
> CC: Jason Liu <jason.hui@linaro.org>
> ---
>  arch/arm/include/asm/arch-mx25/gpio.h    |   12 +--------
>  arch/arm/include/asm/arch-mx31/gpio.h    |    7 +-----
>  arch/arm/include/asm/arch-mx35/gpio.h    |   12 +--------
>  arch/arm/include/asm/arch-mx5/gpio.h     |    7 +-----
>  arch/arm/include/asm/arch-mx6/gpio.h     |    7 +-----
>  arch/arm/include/asm/arch-mx6/imx-regs.h |    2 --
>  arch/arm/include/asm/imx-common/gpio.h   |   39
>  ++++++++++++++++++++++++++++++
>  include/configs/mx6qsabrelite.h          |    1 +
>  8 files changed, 45 insertions(+), 42 deletions(-)
>  create mode 100644 arch/arm/include/asm/imx-common/gpio.h
> 
> diff --git a/arch/arm/include/asm/arch-mx25/gpio.h
> b/arch/arm/include/asm/arch-mx25/gpio.h
> index dc6edc7..5ab1f6d 100644
> --- a/arch/arm/include/asm/arch-mx25/gpio.h
> +++ b/arch/arm/include/asm/arch-mx25/gpio.h
> @@ -30,16 +30,6 @@
>   */
>  #define MXC_GPIO_PORT_TO_NUM(port, bit) (((port - 1) << 5) + (bit &
>  0x1f))

Keeping this is also useless. GPIO_NUMBER() from the new <asm/imx-common/gpio.h>
can be used instead everywhere needed.

>  
> -/* GPIO registers */
> -struct gpio_regs {
> -	u32 gpio_dr;	/* data */
> -	u32 gpio_dir;	/* direction */
> -	u32 psr;	/* pad satus */
> -	u32 icr1;	/* interrupt config 1 */
> -	u32 icr2;	/* interrupt config 2 */
> -	u32 imr;	/* interrupt mask */
> -	u32 isr;	/* interrupt status */
> -	u32 edge_sel;	/* edge select */
> -};
> +#include <asm/imx-common/gpio.h>
>  
>  #endif
> diff --git a/arch/arm/include/asm/arch-mx31/gpio.h
> b/arch/arm/include/asm/arch-mx31/gpio.h
> index 95b73bf..55c0afa 100644
> --- a/arch/arm/include/asm/arch-mx31/gpio.h
> +++ b/arch/arm/include/asm/arch-mx31/gpio.h
> @@ -25,11 +25,6 @@
>  #ifndef __ASM_ARCH_MX31_GPIO_H
>  #define __ASM_ARCH_MX31_GPIO_H
>  
> -/* GPIO Registers */
> -struct gpio_regs {
> -	u32	gpio_dr;
> -	u32	gpio_dir;
> -	u32	gpio_psr;
> -};
> +#include <asm/imx-common/gpio.h>
>  
>  #endif
> diff --git a/arch/arm/include/asm/arch-mx35/gpio.h
> b/arch/arm/include/asm/arch-mx35/gpio.h
> index 7bcc3e8..1deb292 100644
> --- a/arch/arm/include/asm/arch-mx35/gpio.h
> +++ b/arch/arm/include/asm/arch-mx35/gpio.h
> @@ -25,16 +25,6 @@
>  #ifndef __ASM_ARCH_MX35_GPIO_H
>  #define __ASM_ARCH_MX35_GPIO_H
>  
> -/* GPIO registers */
> -struct gpio_regs {
> -	u32 gpio_dr;	/* data */
> -	u32 gpio_dir;	/* direction */
> -	u32 psr;	/* pad satus */
> -	u32 icr1;	/* interrupt config 1 */
> -	u32 icr2;	/* interrupt config 2 */
> -	u32 imr;	/* interrupt mask */
> -	u32 isr;	/* interrupt status */
> -	u32 edge_sel;	/* edge select */
> -};
> +#include <asm/imx-common/gpio.h>
>  
>  #endif
> diff --git a/arch/arm/include/asm/arch-mx5/gpio.h
> b/arch/arm/include/asm/arch-mx5/gpio.h
> index 1dc34e9..b1b1218 100644
> --- a/arch/arm/include/asm/arch-mx5/gpio.h
> +++ b/arch/arm/include/asm/arch-mx5/gpio.h
> @@ -25,11 +25,6 @@
>  #ifndef __ASM_ARCH_MX5_GPIO_H
>  #define __ASM_ARCH_MX5_GPIO_H
>  
> -/* GPIO registers */
> -struct gpio_regs {
> -	u32	gpio_dr;
> -	u32	gpio_dir;
> -	u32	gpio_psr;
> -};
> +#include <asm/imx-common/gpio.h>
>  
>  #endif
> diff --git a/arch/arm/include/asm/arch-mx6/gpio.h
> b/arch/arm/include/asm/arch-mx6/gpio.h
> index 20c4e57..24c10f8 100644
> --- a/arch/arm/include/asm/arch-mx6/gpio.h
> +++ b/arch/arm/include/asm/arch-mx6/gpio.h
> @@ -25,11 +25,6 @@
>  #ifndef __ASM_ARCH_MX6_GPIO_H
>  #define __ASM_ARCH_MX6_GPIO_H
>  
> -/* GPIO registers */
> -struct gpio_regs {
> -	u32	gpio_dr;
> -	u32	gpio_dir;
> -	u32	gpio_psr;
> -};
> +#include <asm/imx-common/gpio.h>
>  
>  #endif	/* __ASM_ARCH_MX6_GPIO_H */

Why do you keep all these old <asm/gpio.h>? The new <asm/imx-common/gpio.h> can
be included instead everywhere needed.

> diff --git a/arch/arm/include/asm/arch-mx6/imx-regs.h
> b/arch/arm/include/asm/arch-mx6/imx-regs.h
> index 5d77603..f3e58b5 100644
> --- a/arch/arm/include/asm/arch-mx6/imx-regs.h
> +++ b/arch/arm/include/asm/arch-mx6/imx-regs.h
> @@ -172,8 +172,6 @@
>  #define IMX_IIM_BASE                 OCOTP_BASE_ADDR
>  #define FEC_QUIRK_ENET_MAC
>  
> -#define GPIO_NUMBER(port, index)		((((port)-1)*32)+((index)&31))
> -
>  #if !(defined(__KERNEL_STRICT_NAMES) || defined(__ASSEMBLY__))
>  #include <asm/types.h>
>  
> diff --git a/arch/arm/include/asm/imx-common/gpio.h
> b/arch/arm/include/asm/imx-common/gpio.h
> new file mode 100644
> index 0000000..fcc25e8
> --- /dev/null
> +++ b/arch/arm/include/asm/imx-common/gpio.h
> @@ -0,0 +1,39 @@
> +/*
> + * Copyright (C) 2011
> + * Stefano Babic, DENX Software Engineering, <sbabic@denx.de>
> + *
> + * 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., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +
> +#ifndef __ASM_ARCH_IMX_GPIO_H
> +#define __ASM_ARCH_IMX_GPIO_H
> +
> +#if !(defined(__KERNEL_STRICT_NAMES) || defined(__ASSEMBLY__))
> +/* GPIO registers */
> +struct gpio_regs {
> +	u32 gpio_dr;	/* data */
> +	u32 gpio_dir;	/* direction */
> +	u32 psr;	/* pad satus */

Why don't you rename this to gpio_psr to be more consistent?

This made me wonder how mxc_gpio.c could build successfully with this naming.
It's because gpio_get_value() uses DR instead of PSR. This is an issue. Linux
uses PSR, and U-Boot should do so since DR does not always reflect the pin
state, while PSR does. This makes a big difference if you want to detect a short
circuit on a GPIO pin configured as output, or if you want to read the level of
a pin driven by a non-GPIO function. This is another patch to make.

> +};
> +#endif
> +
> +#define GPIO_NUMBER(port, index)		((((port)-1)*32)+((index)&31))
> +
> +#endif
> diff --git a/include/configs/mx6qsabrelite.h
> b/include/configs/mx6qsabrelite.h
> index 0d376ba..700268e 100644
> --- a/include/configs/mx6qsabrelite.h
> +++ b/include/configs/mx6qsabrelite.h
> @@ -31,6 +31,7 @@
>  #define CONFIG_MACH_TYPE	3769
>  
>  #include <asm/arch/imx-regs.h>
> +#include <asm/imx-common/gpio.h>
>  
>  #define CONFIG_CMDLINE_TAG
>  #define CONFIG_SETUP_MEMORY_TAGS
> --
> 1.7.9.5
> 
> 

The rest sounds good.

This is a bit off topic, but shouldn't the iomux-v3 stuff be moved to a common
location for all these i.MXs too? As to the header file, it's already done, but
the C file is under arch/arm/cpu/armv7/imx-common/ while it is absolutely not
armv7-specific. Unlike Linux's, U-Boot's SoC files are organized per CPU instead
of per platform, which makes the organization of such platform common code a bit
complicated. This also encourages the duplication of platform code. Perhaps it
could be moved to a new arch/arm/cpu/imx-common/ folder (this would require an
addition to the main Makefile)?

Best regards,
Beno?t