| Message ID | 1345303610-3964-1-git-send-email-sbabic@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
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
On Sat, Aug 18, 2012 at 2:25 PM, Benoît Thébaudeau <benoit.thebaudeau@advansee.com> wrote: > Hi Stefano, > > 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. True, I'd advocate that.
Dear Matt Sealey, > On Sat, Aug 18, 2012 at 2:25 PM, Benoît Thébaudeau > > <benoit.thebaudeau@advansee.com> wrote: > > Hi Stefano, > > > > 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. > > True, I'd advocate that. Ceretainly, but it can be done in a subsequent patch Best regards, Marek Vasut
Am 18/08/2012 21:25, schrieb Benoît Thébaudeau: >> #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. That is right - I drop it. >> >> -/* 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. No. The GPIO is common for all SOCs in u-boot, not only i.MX. The common interface requires that a asm/gpio.h exists. See common/cmd_gpio.c. >> +#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? I can do it, or drop it: psr is not used by the driver. > > This made me wonder how mxc_gpio.c could build successfully with this naming. psr is not used > It's because gpio_get_value() uses DR instead of PSR. Exactly > 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. Right, this is a patch for the gpio driver itself. > > The rest sounds good. > > This is a bit off topic, Never mind. > 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. Right. I miss in u-boot a plat-mxc for common code. > 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)? Up now it was not yet done, because it is an exception in u-boot. But surely, a common place for code across cpu boundaries helps for i.MX. Best regards, Stefano
Hi Stefano,
On Sat, Aug 18, 2012 at 12:26 PM, Stefano Babic <sbabic@denx.de> wrote:
> +#define GPIO_NUMBER(port, index) ((((port)-1)*32)+((index)&31))
What about calling this macro IMX_GPIO_NR instead?
This way we can have the same macro name in U-boot and in the kernel.
Thanks,
Fabio Estevam
Dear Fabio Estevam, > Hi Stefano, > > On Sat, Aug 18, 2012 at 12:26 PM, Stefano Babic <sbabic@denx.de> wrote: > > +#define GPIO_NUMBER(port, index) > > ((((port)-1)*32)+((index)&31)) > > What about calling this macro IMX_GPIO_NR instead? > > This way we can have the same macro name in U-boot and in the kernel. Agreed > Thanks, > > Fabio Estevam Best regards, Marek Vasut
Hi Stefano, > >> #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. > > That is right - I drop it. I don't know if you are aware of it, but just to let you know, I've seen the following patch that will interfere: http://patchwork.ozlabs.org/patch/165311/ http://git.denx.de/?p=u-boot/u-boot-staging.git;a=commitdiff;h=72739219a12bf02820d29a89cb2b7fdc4d0e840f You may want to merge it to your imx tree and rebase after it for your patch. > >> > >> -/* 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. > > No. The GPIO is common for all SOCs in u-boot, not only i.MX. The > common > interface requires that a asm/gpio.h exists. See common/cmd_gpio.c. Right. Best regards, Benoît
Am 19/08/2012 00:59, schrieb Fabio Estevam: > Hi Stefano, > > On Sat, Aug 18, 2012 at 12:26 PM, Stefano Babic <sbabic@denx.de> wrote: > >> +#define GPIO_NUMBER(port, index) ((((port)-1)*32)+((index)&31)) > > What about calling this macro IMX_GPIO_NR instead? > > This way we can have the same macro name in U-boot and in the kernel. Right, agree, fix in V2. Regards, Stefano
On 19/08/2012 02:25, Benoît Thébaudeau wrote: > Hi Stefano, > >>>> #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. >> >> That is right - I drop it. > > I don't know if you are aware of it, but just to let you know, I've seen the > following patch that will interfere: > http://patchwork.ozlabs.org/patch/165311/ > http://git.denx.de/?p=u-boot/u-boot-staging.git;a=commitdiff;h=72739219a12bf02820d29a89cb2b7fdc4d0e840f Thanks, I cherry-pick it and I rebase on it. Best regards, Stefano
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)) -/* 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 */ 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 */ +}; +#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
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. 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