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

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