| Message ID | 1461099580-3866-16-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Rejected |
| Delegated to: | Tom Warren |
| Headers | show |
Hi Stephen, On 19 April 2016 at 14:58, Stephen Warren <swarren@wwwdotorg.org> wrote: > From: Stephen Warren <swarren@nvidia.com> > > Tegra's gpio.h contains a mix of private definitions for use inside the > GPIO driver and custom machine-specific APIs. Move the private definitions > out of the global include directory since nothing should need to access > them. Move the public definitions to the machine-specific include > directory <mach/>. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > arch/arm/include/asm/arch-tegra114/gpio.h | 20 ------ > arch/arm/include/asm/arch-tegra124/gpio.h | 44 ------------- > arch/arm/include/asm/arch-tegra20/gpio.h | 36 ---------- > arch/arm/include/asm/arch-tegra210/gpio.h | 44 ------------- > arch/arm/include/asm/arch-tegra30/gpio.h | 43 ------------ > arch/arm/include/asm/gpio.h | 2 +- > .../include/mach/tegra_gpio.h} | 13 +--- > board/nvidia/e2220-1170/e2220-1170.c | 3 +- > board/nvidia/jetson-tk1/jetson-tk1.c | 3 +- > board/nvidia/nyan-big/nyan-big.c | 1 + > board/nvidia/p2371-0000/p2371-0000.c | 3 +- > board/nvidia/p2371-2180/p2371-2180.c | 3 +- > board/nvidia/p2571/p2571.c | 2 +- > board/nvidia/venice2/venice2.c | 3 +- > drivers/gpio/tegra_gpio.c | 2 + > drivers/gpio/tegra_gpio_priv.h | 76 ++++++++++++++++++++++ > drivers/i2c/tegra_i2c.c | 1 - > 17 files changed, 94 insertions(+), 205 deletions(-) > delete mode 100644 arch/arm/include/asm/arch-tegra114/gpio.h > delete mode 100644 arch/arm/include/asm/arch-tegra124/gpio.h > delete mode 100644 arch/arm/include/asm/arch-tegra20/gpio.h > delete mode 100644 arch/arm/include/asm/arch-tegra210/gpio.h > delete mode 100644 arch/arm/include/asm/arch-tegra30/gpio.h > rename arch/arm/{include/asm/arch-tegra/gpio.h => mach-tegra/include/mach/tegra_gpio.h} (67%) > create mode 100644 drivers/gpio/tegra_gpio_priv.h > > diff --git a/arch/arm/include/asm/arch-tegra114/gpio.h b/arch/arm/include/asm/arch-tegra114/gpio.h > deleted file mode 100644 > index d6eaa1bd40e8..000000000000 > --- a/arch/arm/include/asm/arch-tegra114/gpio.h > +++ /dev/null > @@ -1,20 +0,0 @@ > -/* > - * Copyright (c) 2010-2013, NVIDIA CORPORATION. All rights reserved. > - * > - * SPDX-License-Identifier: GPL-2.0 > - */ > - > -#ifndef _TEGRA114_GPIO_H_ > -#define _TEGRA114_GPIO_H_ > - > -/* > - * The Tegra114 GPIO controller has 246 GPIOS in 8 banks of 4 ports, > - * each with 8 GPIOs. > - */ > -#define TEGRA_GPIO_PORTS 4 /* number of ports per bank */ > -#define TEGRA_GPIO_BANKS 8 /* number of banks */ > - > -#include <asm/arch-tegra/gpio.h> > -#include <asm/arch-tegra30/gpio.h> > - > -#endif /* _TEGRA114_GPIO_H_ */ > diff --git a/arch/arm/include/asm/arch-tegra124/gpio.h b/arch/arm/include/asm/arch-tegra124/gpio.h > deleted file mode 100644 > index 8fddb63f44c2..000000000000 > --- a/arch/arm/include/asm/arch-tegra124/gpio.h > +++ /dev/null > @@ -1,44 +0,0 @@ > -/* > - * (C) Copyright 2013-2016 > - * NVIDIA Corporation <www.nvidia.com> > - * > - * SPDX-License-Identifier: GPL-2.0+ > - */ > - > -#ifndef _TEGRA124_GPIO_H_ > -#define _TEGRA124_GPIO_H_ > - > -/* > - * The Tegra124 GPIO controller has 256 GPIOS in 8 banks of 4 ports, > - * each with 8 GPIOs. > - */ > -#define TEGRA_GPIO_PORTS 4 /* number of ports per bank */ > -#define TEGRA_GPIO_BANKS 8 /* number of banks */ > - > -#include <asm/arch-tegra/gpio.h> > - > -/* GPIO Controller registers for a single bank */ > -struct gpio_ctlr_bank { > - uint gpio_config[TEGRA_GPIO_PORTS]; > - uint gpio_dir_out[TEGRA_GPIO_PORTS]; > - uint gpio_out[TEGRA_GPIO_PORTS]; > - uint gpio_in[TEGRA_GPIO_PORTS]; > - uint gpio_int_status[TEGRA_GPIO_PORTS]; > - uint gpio_int_enable[TEGRA_GPIO_PORTS]; > - uint gpio_int_level[TEGRA_GPIO_PORTS]; > - uint gpio_int_clear[TEGRA_GPIO_PORTS]; > - uint gpio_masked_config[TEGRA_GPIO_PORTS]; > - uint gpio_masked_dir_out[TEGRA_GPIO_PORTS]; > - uint gpio_masked_out[TEGRA_GPIO_PORTS]; > - uint gpio_masked_in[TEGRA_GPIO_PORTS]; > - uint gpio_masked_int_status[TEGRA_GPIO_PORTS]; > - uint gpio_masked_int_enable[TEGRA_GPIO_PORTS]; > - uint gpio_masked_int_level[TEGRA_GPIO_PORTS]; > - uint gpio_masked_int_clear[TEGRA_GPIO_PORTS]; > -}; > - > -struct gpio_ctlr { > - struct gpio_ctlr_bank gpio_bank[TEGRA_GPIO_BANKS]; > -}; > - > -#endif /* _TEGRA124_GPIO_H_ */ > diff --git a/arch/arm/include/asm/arch-tegra20/gpio.h b/arch/arm/include/asm/arch-tegra20/gpio.h > deleted file mode 100644 > index 46dcc28f727c..000000000000 > --- a/arch/arm/include/asm/arch-tegra20/gpio.h > +++ /dev/null > @@ -1,36 +0,0 @@ > -/* > - * Copyright (c) 2011, Google Inc. All rights reserved. > - * Portions Copyright 2011-2016 NVIDIA Corporation > - * > - * SPDX-License-Identifier: GPL-2.0+ > - */ > - > -#ifndef _TEGRA20_GPIO_H_ > -#define _TEGRA20_GPIO_H_ > - > -/* > - * The Tegra 2x GPIO controller has 224 GPIOs arranged in 7 banks of 4 ports, > - * each with 8 GPIOs. > - */ > -#define TEGRA_GPIO_PORTS 4 /* number of ports per bank */ > -#define TEGRA_GPIO_BANKS 7 /* number of banks */ > - > -#include <asm/arch-tegra/gpio.h> > - > -/* GPIO Controller registers for a single bank */ > -struct gpio_ctlr_bank { > - uint gpio_config[TEGRA_GPIO_PORTS]; > - uint gpio_dir_out[TEGRA_GPIO_PORTS]; > - uint gpio_out[TEGRA_GPIO_PORTS]; > - uint gpio_in[TEGRA_GPIO_PORTS]; > - uint gpio_int_status[TEGRA_GPIO_PORTS]; > - uint gpio_int_enable[TEGRA_GPIO_PORTS]; > - uint gpio_int_level[TEGRA_GPIO_PORTS]; > - uint gpio_int_clear[TEGRA_GPIO_PORTS]; > -}; > - > -struct gpio_ctlr { > - struct gpio_ctlr_bank gpio_bank[TEGRA_GPIO_BANKS]; > -}; > - > -#endif /* TEGRA20_GPIO_H_ */ > diff --git a/arch/arm/include/asm/arch-tegra210/gpio.h b/arch/arm/include/asm/arch-tegra210/gpio.h > deleted file mode 100644 > index f2279d0f3e92..000000000000 > --- a/arch/arm/include/asm/arch-tegra210/gpio.h > +++ /dev/null > @@ -1,44 +0,0 @@ > -/* > - * (C) Copyright 2013-2016 > - * NVIDIA Corporation <www.nvidia.com> > - * > - * SPDX-License-Identifier: GPL-2.0+ > - */ > - > -#ifndef _TEGRA210_GPIO_H_ > -#define _TEGRA210_GPIO_H_ > - > -/* > - * The Tegra210 GPIO controller has 256 GPIOS in 8 banks of 4 ports, > - * each with 8 GPIOs. > - */ > -#define TEGRA_GPIO_PORTS 4 /* number of ports per bank */ > -#define TEGRA_GPIO_BANKS 8 /* number of banks */ > - > -#include <asm/arch-tegra/gpio.h> > - > -/* GPIO Controller registers for a single bank */ > -struct gpio_ctlr_bank { > - uint gpio_config[TEGRA_GPIO_PORTS]; > - uint gpio_dir_out[TEGRA_GPIO_PORTS]; > - uint gpio_out[TEGRA_GPIO_PORTS]; > - uint gpio_in[TEGRA_GPIO_PORTS]; > - uint gpio_int_status[TEGRA_GPIO_PORTS]; > - uint gpio_int_enable[TEGRA_GPIO_PORTS]; > - uint gpio_int_level[TEGRA_GPIO_PORTS]; > - uint gpio_int_clear[TEGRA_GPIO_PORTS]; > - uint gpio_masked_config[TEGRA_GPIO_PORTS]; > - uint gpio_masked_dir_out[TEGRA_GPIO_PORTS]; > - uint gpio_masked_out[TEGRA_GPIO_PORTS]; > - uint gpio_masked_in[TEGRA_GPIO_PORTS]; > - uint gpio_masked_int_status[TEGRA_GPIO_PORTS]; > - uint gpio_masked_int_enable[TEGRA_GPIO_PORTS]; > - uint gpio_masked_int_level[TEGRA_GPIO_PORTS]; > - uint gpio_masked_int_clear[TEGRA_GPIO_PORTS]; > -}; > - > -struct gpio_ctlr { > - struct gpio_ctlr_bank gpio_bank[TEGRA_GPIO_BANKS]; > -}; > - > -#endif /* _TEGRA210_GPIO_H_ */ > diff --git a/arch/arm/include/asm/arch-tegra30/gpio.h b/arch/arm/include/asm/arch-tegra30/gpio.h > deleted file mode 100644 > index 288451df2ff6..000000000000 > --- a/arch/arm/include/asm/arch-tegra30/gpio.h > +++ /dev/null > @@ -1,43 +0,0 @@ > -/* > - * Copyright (c) 2010-2016, NVIDIA CORPORATION. All rights reserved. > - * > - * SPDX-License-Identifier: GPL-2.0 > - */ > - > -#ifndef _TEGRA30_GPIO_H_ > -#define _TEGRA30_GPIO_H_ > - > -/* > - * The Tegra 3x GPIO controller has 246 GPIOS in 8 banks of 4 ports, > - * each with 8 GPIOs. > - */ > -#define TEGRA_GPIO_PORTS 4 /* number of ports per bank */ > -#define TEGRA_GPIO_BANKS 8 /* number of banks */ > - > -#include <asm/arch-tegra/gpio.h> > - > -/* GPIO Controller registers for a single bank */ > -struct gpio_ctlr_bank { > - uint gpio_config[TEGRA_GPIO_PORTS]; > - uint gpio_dir_out[TEGRA_GPIO_PORTS]; > - uint gpio_out[TEGRA_GPIO_PORTS]; > - uint gpio_in[TEGRA_GPIO_PORTS]; > - uint gpio_int_status[TEGRA_GPIO_PORTS]; > - uint gpio_int_enable[TEGRA_GPIO_PORTS]; > - uint gpio_int_level[TEGRA_GPIO_PORTS]; > - uint gpio_int_clear[TEGRA_GPIO_PORTS]; > - uint gpio_masked_config[TEGRA_GPIO_PORTS]; > - uint gpio_masked_dir_out[TEGRA_GPIO_PORTS]; > - uint gpio_masked_out[TEGRA_GPIO_PORTS]; > - uint gpio_masked_in[TEGRA_GPIO_PORTS]; > - uint gpio_masked_int_status[TEGRA_GPIO_PORTS]; > - uint gpio_masked_int_enable[TEGRA_GPIO_PORTS]; > - uint gpio_masked_int_level[TEGRA_GPIO_PORTS]; > - uint gpio_masked_int_clear[TEGRA_GPIO_PORTS]; > -}; > - > -struct gpio_ctlr { > - struct gpio_ctlr_bank gpio_bank[TEGRA_GPIO_BANKS]; > -}; > - > -#endif /* _TEGRA30_GPIO_H_ */ > diff --git a/arch/arm/include/asm/gpio.h b/arch/arm/include/asm/gpio.h > index fe4419cae4cf..751a5ade17c7 100644 > --- a/arch/arm/include/asm/gpio.h > +++ b/arch/arm/include/asm/gpio.h > @@ -1,4 +1,4 @@ > -#ifndef CONFIG_ARCH_UNIPHIER > +#if !defined(CONFIG_ARCH_UNIPHIER) && !defined(CONFIG_TEGRA) > #include <asm/arch/gpio.h> > #endif > #include <asm-generic/gpio.h> > diff --git a/arch/arm/include/asm/arch-tegra/gpio.h b/arch/arm/mach-tegra/include/mach/tegra_gpio.h > similarity index 67% > rename from arch/arm/include/asm/arch-tegra/gpio.h > rename to arch/arm/mach-tegra/include/mach/tegra_gpio.h > index 07921f34b9d7..09b813bc79ac 100644 > --- a/arch/arm/include/asm/arch-tegra/gpio.h > +++ b/arch/arm/mach-tegra/include/mach/tegra_gpio.h > @@ -5,15 +5,8 @@ > * SPDX-License-Identifier: GPL-2.0+ > */ > > -#ifndef _TEGRA_GPIO_H_ > -#define _TEGRA_GPIO_H_ > - > -#define TEGRA_GPIOS_PER_PORT 8 > - > -#define GPIO_BANK(x) ((x) >> 5) > -#define GPIO_PORT(x) (((x) >> 3) & 0x3) > -#define GPIO_FULLPORT(x) ((x) >> 3) > -#define GPIO_BIT(x) ((x) & 0x7) > +#ifndef _MACH_TEGRA_GPIO_H > +#define _MACH_TEGRA_GPIO_H > > enum tegra_gpio_init { > TEGRA_GPIO_INIT_IN, > @@ -34,4 +27,4 @@ struct tegra_gpio_config { > */ > void gpio_config_table(const struct tegra_gpio_config *config, int len); > > -#endif /* TEGRA_GPIO_H_ */ > +#endif > diff --git a/board/nvidia/e2220-1170/e2220-1170.c b/board/nvidia/e2220-1170/e2220-1170.c > index db6fa74ae1fe..c2b5e5e09d21 100644 > --- a/board/nvidia/e2220-1170/e2220-1170.c > +++ b/board/nvidia/e2220-1170/e2220-1170.c > @@ -8,8 +8,9 @@ > #include <common.h> > #include <i2c.h> > #include <dt-bindings/gpio/tegra-gpio.h> > -#include <asm/arch/gpio.h> > +#include <asm/gpio.h> > #include <asm/arch/pinmux.h> > +#include <mach/tegra_gpio.h> > #include "../p2571/max77620_init.h" > #include "pinmux-config-e2220-1170.h" > > diff --git a/board/nvidia/jetson-tk1/jetson-tk1.c b/board/nvidia/jetson-tk1/jetson-tk1.c > index 4b7058e3bc89..422a18a4e530 100644 > --- a/board/nvidia/jetson-tk1/jetson-tk1.c > +++ b/board/nvidia/jetson-tk1/jetson-tk1.c > @@ -8,8 +8,9 @@ > #include <common.h> > #include <dt-bindings/gpio/tegra-gpio.h> > #include <power/as3722.h> > -#include <asm/arch/gpio.h> > +#include <asm/gpio.h> > #include <asm/arch/pinmux.h> > +#include <mach/tegra_gpio.h> > #include "pinmux-config-jetson-tk1.h" > > DECLARE_GLOBAL_DATA_PTR; > diff --git a/board/nvidia/nyan-big/nyan-big.c b/board/nvidia/nyan-big/nyan-big.c > index 56e15bda93ec..64d98f59a4c7 100644 > --- a/board/nvidia/nyan-big/nyan-big.c > +++ b/board/nvidia/nyan-big/nyan-big.c > @@ -17,6 +17,7 @@ > #include <asm/arch/mc.h> > #include <asm/arch-tegra/clk_rst.h> > #include <asm/arch-tegra/pmc.h> > +#include <mach/tegra_gpio.h> > #include "pinmux-config-nyan-big.h" > > /* > diff --git a/board/nvidia/p2371-0000/p2371-0000.c b/board/nvidia/p2371-0000/p2371-0000.c > index 0fec3d497d7d..a23dfc8afaef 100644 > --- a/board/nvidia/p2371-0000/p2371-0000.c > +++ b/board/nvidia/p2371-0000/p2371-0000.c > @@ -8,8 +8,9 @@ > #include <common.h> > #include <i2c.h> > #include <dt-bindings/gpio/tegra-gpio.h> > -#include <asm/arch/gpio.h> > +#include <asm/gpio.h> > #include <asm/arch/pinmux.h> > +#include <mach/tegra_gpio.h> > #include "../p2571/max77620_init.h" > #include "pinmux-config-p2371-0000.h" > > diff --git a/board/nvidia/p2371-2180/p2371-2180.c b/board/nvidia/p2371-2180/p2371-2180.c > index bc3389976942..3989ab8bde32 100644 > --- a/board/nvidia/p2371-2180/p2371-2180.c > +++ b/board/nvidia/p2371-2180/p2371-2180.c > @@ -8,8 +8,9 @@ > #include <common.h> > #include <i2c.h> > #include <dt-bindings/gpio/tegra-gpio.h> > -#include <asm/arch/gpio.h> > +#include <asm/gpio.h> > #include <asm/arch/pinmux.h> > +#include <mach/tegra_gpio.h> > #include "../p2571/max77620_init.h" > #include "pinmux-config-p2371-2180.h" > > diff --git a/board/nvidia/p2571/p2571.c b/board/nvidia/p2571/p2571.c > index 794830bac3b3..7faec788f3cf 100644 > --- a/board/nvidia/p2571/p2571.c > +++ b/board/nvidia/p2571/p2571.c > @@ -9,8 +9,8 @@ > #include <i2c.h> > #include <dt-bindings/gpio/tegra-gpio.h> > #include <asm/gpio.h> > -#include <asm/arch/gpio.h> > #include <asm/arch/pinmux.h> > +#include <mach/tegra_gpio.h> > #include "max77620_init.h" > #include "pinmux-config-p2571.h" > > diff --git a/board/nvidia/venice2/venice2.c b/board/nvidia/venice2/venice2.c > index 4373de6a26bd..5791e0d73522 100644 > --- a/board/nvidia/venice2/venice2.c > +++ b/board/nvidia/venice2/venice2.c > @@ -7,8 +7,9 @@ > > #include <common.h> > #include <dt-bindings/gpio/tegra-gpio.h> > -#include <asm/arch/gpio.h> > +#include <asm/gpio.h> > #include <asm/arch/pinmux.h> > +#include <mach/tegra_gpio.h> > #include "pinmux-config-venice2.h" > > /* > diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c > index 9abab13daaa9..a3b6878e47bd 100644 > --- a/drivers/gpio/tegra_gpio.c > +++ b/drivers/gpio/tegra_gpio.c > @@ -22,6 +22,8 @@ > #include <asm/gpio.h> > #include <dm/device-internal.h> > #include <dt-bindings/gpio/gpio.h> > +#include <mach/tegra_gpio.h> > +#include "tegra_gpio_priv.h" > > DECLARE_GLOBAL_DATA_PTR; > > diff --git a/drivers/gpio/tegra_gpio_priv.h b/drivers/gpio/tegra_gpio_priv.h > new file mode 100644 > index 000000000000..edf5540da72f > --- /dev/null > +++ b/drivers/gpio/tegra_gpio_priv.h > @@ -0,0 +1,76 @@ > +/* > + * Copyright (c) 2011, Google Inc. All rights reserved. > + * Copyright 2011-2016 NVIDIA Corporation > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#ifndef _TEGRA_GPIO_PRIV_H > +#define _TEGRA_GPIO_PRIV_H > + > +/* > + * GPIOs are arranged into ports, each of which controls 8 GPIOs. > + * > + * Ports are aggregated into banks (U-Boot terminology used below) or > + * "controllers" (Tegra TRM terminology) each containing 4 ports. > + * > + * Banks/controllers are aggregated into the overall GPIO controller hardware > + * module. Tegra20 implements only 7 banks. However, the register location of > + * the "masked" registers is designed to leave space for 8 banks. Tegra30 and > + * onwards implement 8 banks. In many cases, the last bank contains ports > + * and/or GPIOs that are not implemented, or at least not connected to any > + * pad/pin. > + */ > +#define TEGRA_GPIO_BANKS 8 /* number of banks */ > +#define TEGRA_GPIO_PORTS 4 /* number of ports per bank */ > +#define TEGRA_GPIOS_PER_PORT 8 > + > +#define GPIO_BANK(x) ((x) >> 5) > +#define GPIO_PORT(x) (((x) >> 3) & 0x3) > +#define GPIO_FULLPORT(x) ((x) >> 3) > +#define GPIO_BIT(x) ((x) & 0x7) > + > +/* > + * GPIO registers are split into two chunks; low and high. > + * On Tegra20, all low chunks appear first, then all high chunks. > + * In later SoCs, the low and high chunks are interleaved together. > + */ > +#define GPIO_CTLR_BANK_HIGH_REGS \ > + uint gpio_masked_config[TEGRA_GPIO_PORTS]; \ > + uint gpio_masked_dir_out[TEGRA_GPIO_PORTS]; \ > + uint gpio_masked_out[TEGRA_GPIO_PORTS]; \ > + uint reserved0[TEGRA_GPIO_PORTS]; \ > + uint gpio_masked_int_status[TEGRA_GPIO_PORTS]; \ > + uint gpio_masked_int_enable[TEGRA_GPIO_PORTS]; \ > + uint gpio_masked_int_level[TEGRA_GPIO_PORTS]; \ > + uint reserved1[TEGRA_GPIO_PORTS]; > + > +/* GPIO Controller registers for a single bank */ > +struct gpio_ctlr_bank { > + uint gpio_config[TEGRA_GPIO_PORTS]; > + uint gpio_dir_out[TEGRA_GPIO_PORTS]; > + uint gpio_out[TEGRA_GPIO_PORTS]; > + uint gpio_in[TEGRA_GPIO_PORTS]; > + uint gpio_int_status[TEGRA_GPIO_PORTS]; > + uint gpio_int_enable[TEGRA_GPIO_PORTS]; > + uint gpio_int_level[TEGRA_GPIO_PORTS]; > + uint gpio_int_clear[TEGRA_GPIO_PORTS]; > +#ifndef CONFIG_TEGRA20 > + GPIO_CTLR_BANK_HIGH_REGS > +#endif > +}; > + > +#ifdef CONFIG_TEGRA20 > +struct gpio_ctlr_bank_high { > + GPIO_CTLR_BANK_HIGH_REGS This seems a bit ugly. Perhaps you could havestruct gpio_ctlr_high_regs and include that here? It adds a level of indirection but that doesn't seem very important. > +}; > +#endif > + > +struct gpio_ctlr { > + struct gpio_ctlr_bank gpio_bank[TEGRA_GPIO_BANKS]; > +#ifdef CONFIG_TEGRA20 > + struct gpio_ctlr_bank_high gpio_bank_high[TEGRA_GPIO_BANKS]; > +#endif > +}; > + > +#endif > diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c > index 2a8ab2d5e0b9..f1b8877be228 100644 > --- a/drivers/i2c/tegra_i2c.c > +++ b/drivers/i2c/tegra_i2c.c > @@ -14,7 +14,6 @@ > #include <asm/io.h> > #include <asm/arch/clock.h> > #include <asm/arch/funcmux.h> > -#include <asm/arch/gpio.h> > #include <asm/arch/pinmux.h> > #include <asm/arch-tegra/clk_rst.h> > #include <mach/tegra_i2c.h> > -- > 2.8.1 > Regards, Simon
On 04/20/2016 01:26 PM, Simon Glass wrote: > Hi Stephen, > > On 19 April 2016 at 14:58, Stephen Warren <swarren@wwwdotorg.org> wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> Tegra's gpio.h contains a mix of private definitions for use inside the >> GPIO driver and custom machine-specific APIs. Move the private definitions >> out of the global include directory since nothing should need to access >> them. Move the public definitions to the machine-specific include >> directory <mach/>. >> diff --git a/drivers/gpio/tegra_gpio_priv.h b/drivers/gpio/tegra_gpio_priv.h >> +/* >> + * GPIO registers are split into two chunks; low and high. >> + * On Tegra20, all low chunks appear first, then all high chunks. >> + * In later SoCs, the low and high chunks are interleaved together. >> + */ >> +#define GPIO_CTLR_BANK_HIGH_REGS \ >> + uint gpio_masked_config[TEGRA_GPIO_PORTS]; \ >> + uint gpio_masked_dir_out[TEGRA_GPIO_PORTS]; \ >> + uint gpio_masked_out[TEGRA_GPIO_PORTS]; \ >> + uint reserved0[TEGRA_GPIO_PORTS]; \ >> + uint gpio_masked_int_status[TEGRA_GPIO_PORTS]; \ >> + uint gpio_masked_int_enable[TEGRA_GPIO_PORTS]; \ >> + uint gpio_masked_int_level[TEGRA_GPIO_PORTS]; \ >> + uint reserved1[TEGRA_GPIO_PORTS]; >> + >> +/* GPIO Controller registers for a single bank */ >> +struct gpio_ctlr_bank { >> + uint gpio_config[TEGRA_GPIO_PORTS]; >> + uint gpio_dir_out[TEGRA_GPIO_PORTS]; >> + uint gpio_out[TEGRA_GPIO_PORTS]; >> + uint gpio_in[TEGRA_GPIO_PORTS]; >> + uint gpio_int_status[TEGRA_GPIO_PORTS]; >> + uint gpio_int_enable[TEGRA_GPIO_PORTS]; >> + uint gpio_int_level[TEGRA_GPIO_PORTS]; >> + uint gpio_int_clear[TEGRA_GPIO_PORTS]; >> +#ifndef CONFIG_TEGRA20 >> + GPIO_CTLR_BANK_HIGH_REGS >> +#endif >> +}; >> + >> +#ifdef CONFIG_TEGRA20 >> +struct gpio_ctlr_bank_high { >> + GPIO_CTLR_BANK_HIGH_REGS > > This seems a bit ugly. Perhaps you could havestruct > gpio_ctlr_high_regs and include that here? It adds a level of > indirection but that doesn't seem very important. In newer Tegras, there's no differentiation between the two register sets that were "low" and "high" in Tegra20. I'd rather not saddle the non-Tegra20 struct layouts with some odd naming/nesting just because the Tegra20 layout was odd. I don't see any problem with using a #define for this; it doesn't seem to make the code complex. I wonder if we should just convert away from structs for registers entirely. Everything in the HW docs is just numbers, matching those to the structs is always painful, and if we used #defines instead of structs, representing this HW difference would end up being much cleaner and avoid using a macro to "cut/paste" a register list 2 times; see the way the Linux kernel driver handles this.
Hi Stephen, On 20 April 2016 at 16:01, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/20/2016 01:26 PM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 19 April 2016 at 14:58, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> From: Stephen Warren <swarren@nvidia.com> >>> >>> Tegra's gpio.h contains a mix of private definitions for use inside the >>> GPIO driver and custom machine-specific APIs. Move the private >>> definitions >>> out of the global include directory since nothing should need to access >>> them. Move the public definitions to the machine-specific include >>> directory <mach/>. > > >>> diff --git a/drivers/gpio/tegra_gpio_priv.h >>> b/drivers/gpio/tegra_gpio_priv.h > > >>> +/* >>> + * GPIO registers are split into two chunks; low and high. >>> + * On Tegra20, all low chunks appear first, then all high chunks. >>> + * In later SoCs, the low and high chunks are interleaved together. >>> + */ >>> +#define GPIO_CTLR_BANK_HIGH_REGS \ >>> + uint gpio_masked_config[TEGRA_GPIO_PORTS]; \ >>> + uint gpio_masked_dir_out[TEGRA_GPIO_PORTS]; \ >>> + uint gpio_masked_out[TEGRA_GPIO_PORTS]; \ >>> + uint reserved0[TEGRA_GPIO_PORTS]; \ >>> + uint gpio_masked_int_status[TEGRA_GPIO_PORTS]; \ >>> + uint gpio_masked_int_enable[TEGRA_GPIO_PORTS]; \ >>> + uint gpio_masked_int_level[TEGRA_GPIO_PORTS]; \ >>> + uint reserved1[TEGRA_GPIO_PORTS]; >>> + >>> +/* GPIO Controller registers for a single bank */ >>> +struct gpio_ctlr_bank { >>> + uint gpio_config[TEGRA_GPIO_PORTS]; >>> + uint gpio_dir_out[TEGRA_GPIO_PORTS]; >>> + uint gpio_out[TEGRA_GPIO_PORTS]; >>> + uint gpio_in[TEGRA_GPIO_PORTS]; >>> + uint gpio_int_status[TEGRA_GPIO_PORTS]; >>> + uint gpio_int_enable[TEGRA_GPIO_PORTS]; >>> + uint gpio_int_level[TEGRA_GPIO_PORTS]; >>> + uint gpio_int_clear[TEGRA_GPIO_PORTS]; >>> +#ifndef CONFIG_TEGRA20 >>> + GPIO_CTLR_BANK_HIGH_REGS >>> +#endif >>> +}; >>> + >>> +#ifdef CONFIG_TEGRA20 >>> +struct gpio_ctlr_bank_high { >>> + GPIO_CTLR_BANK_HIGH_REGS >> >> >> This seems a bit ugly. Perhaps you could havestruct >> gpio_ctlr_high_regs and include that here? It adds a level of >> indirection but that doesn't seem very important. > > > In newer Tegras, there's no differentiation between the two register sets > that were "low" and "high" in Tegra20. I'd rather not saddle the non-Tegra20 > struct layouts with some odd naming/nesting just because the Tegra20 layout > was odd. I don't see any problem with using a #define for this; it doesn't > seem to make the code complex. OK, well then how about just duplicating the two structs, and dropping the #define? #ifdfef CONFIG_TEGRA20 struct gpio_ctlr_bank { }; #else struct gpio_ctlr_bank { }; #endif > I wonder if we should just convert away from structs for registers entirely. > Everything in the HW docs is just numbers, matching those to the structs is > always painful, and if we used #defines instead of structs, representing > this HW difference would end up being much cleaner and avoid using a macro > to "cut/paste" a register list 2 times; see the way the Linux kernel driver > handles this. Structs are definitely easier to read and particularly in this case where each struct element is an array. Why are you worried about code duplication in a header file? Regards, Simon
On 04/21/2016 08:11 AM, Simon Glass wrote: > Hi Stephen, > > On 20 April 2016 at 16:01, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 04/20/2016 01:26 PM, Simon Glass wrote: >>> >>> Hi Stephen, >>> >>> On 19 April 2016 at 14:58, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> >>>> From: Stephen Warren <swarren@nvidia.com> >>>> >>>> Tegra's gpio.h contains a mix of private definitions for use inside the >>>> GPIO driver and custom machine-specific APIs. Move the private >>>> definitions >>>> out of the global include directory since nothing should need to access >>>> them. Move the public definitions to the machine-specific include >>>> directory <mach/>. >> >> >>>> diff --git a/drivers/gpio/tegra_gpio_priv.h >>>> b/drivers/gpio/tegra_gpio_priv.h >> >> >>>> +/* >>>> + * GPIO registers are split into two chunks; low and high. >>>> + * On Tegra20, all low chunks appear first, then all high chunks. >>>> + * In later SoCs, the low and high chunks are interleaved together. >>>> + */ >>>> +#define GPIO_CTLR_BANK_HIGH_REGS \ >>>> + uint gpio_masked_config[TEGRA_GPIO_PORTS]; \ >>>> + uint gpio_masked_dir_out[TEGRA_GPIO_PORTS]; \ >>>> + uint gpio_masked_out[TEGRA_GPIO_PORTS]; \ >>>> + uint reserved0[TEGRA_GPIO_PORTS]; \ >>>> + uint gpio_masked_int_status[TEGRA_GPIO_PORTS]; \ >>>> + uint gpio_masked_int_enable[TEGRA_GPIO_PORTS]; \ >>>> + uint gpio_masked_int_level[TEGRA_GPIO_PORTS]; \ >>>> + uint reserved1[TEGRA_GPIO_PORTS]; >>>> + >>>> +/* GPIO Controller registers for a single bank */ >>>> +struct gpio_ctlr_bank { >>>> + uint gpio_config[TEGRA_GPIO_PORTS]; >>>> + uint gpio_dir_out[TEGRA_GPIO_PORTS]; >>>> + uint gpio_out[TEGRA_GPIO_PORTS]; >>>> + uint gpio_in[TEGRA_GPIO_PORTS]; >>>> + uint gpio_int_status[TEGRA_GPIO_PORTS]; >>>> + uint gpio_int_enable[TEGRA_GPIO_PORTS]; >>>> + uint gpio_int_level[TEGRA_GPIO_PORTS]; >>>> + uint gpio_int_clear[TEGRA_GPIO_PORTS]; >>>> +#ifndef CONFIG_TEGRA20 >>>> + GPIO_CTLR_BANK_HIGH_REGS >>>> +#endif >>>> +}; >>>> + >>>> +#ifdef CONFIG_TEGRA20 >>>> +struct gpio_ctlr_bank_high { >>>> + GPIO_CTLR_BANK_HIGH_REGS >>> >>> >>> This seems a bit ugly. Perhaps you could havestruct >>> gpio_ctlr_high_regs and include that here? It adds a level of >>> indirection but that doesn't seem very important. >> >> >> In newer Tegras, there's no differentiation between the two register sets >> that were "low" and "high" in Tegra20. I'd rather not saddle the non-Tegra20 >> struct layouts with some odd naming/nesting just because the Tegra20 layout >> was odd. I don't see any problem with using a #define for this; it doesn't >> seem to make the code complex. > > OK, well then how about just duplicating the two structs, and dropping > the #define? > > #ifdfef CONFIG_TEGRA20 > struct gpio_ctlr_bank { > > }; > #else > struct gpio_ctlr_bank { > }; > #endif Given that the driver doesn't use any registers in the high bank, and indeed can't; the driver's reliance on structs rather than register defines means that the high bank registers would have to be accessed differently between Tegra20 and other SoCs, I propose simply not representing those registers. Instead, how about: struct gpio_ctlr_bank { uint gpio_config[TEGRA_GPIO_PORTS]; uint gpio_dir_out[TEGRA_GPIO_PORTS]; uint gpio_out[TEGRA_GPIO_PORTS]; uint gpio_in[TEGRA_GPIO_PORTS]; uint gpio_int_status[TEGRA_GPIO_PORTS]; uint gpio_int_enable[TEGRA_GPIO_PORTS]; uint gpio_int_level[TEGRA_GPIO_PORTS]; uint gpio_int_clear[TEGRA_GPIO_PORTS]; #ifndef CONFIG_TEGRA20 uint unused[TEGRA_GPIO_PORTS * 8]; #endif }; struct gpio_ctlr { struct gpio_ctlr_bank gpio_bank[TEGRA_GPIO_BANKS]; }; That removes all the duplication, without any macros etc. Does that look reasonable? If I fixup the patch like that, I'll add a brief comment describing what the unused registers are, similar to the one in patch v1. >> I wonder if we should just convert away from structs for registers entirely. >> Everything in the HW docs is just numbers, matching those to the structs is >> always painful, and if we used #defines instead of structs, representing >> this HW difference would end up being much cleaner and avoid using a macro >> to "cut/paste" a register list 2 times; see the way the Linux kernel driver >> handles this. > > Structs are definitely easier to read and particularly in this case > where each struct element is an array. I'm not really sure there's much objective difference between the readability of the two. Arrays can easily be abstracted via a macro or inline function so that the call site looks essentially identical; () vs []. > Why are you worried about code duplication in a header file? I'm not sure why I would special case my concerns and ignore duplication in certain locations yet still care about duplication in general or elsewhere?
Hi Stephen, On 21 April 2016 at 10:40, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/21/2016 08:11 AM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 20 April 2016 at 16:01, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> On 04/20/2016 01:26 PM, Simon Glass wrote: >>>> >>>> >>>> Hi Stephen, >>>> >>>> On 19 April 2016 at 14:58, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>> >>>>> >>>>> From: Stephen Warren <swarren@nvidia.com> >>>>> >>>>> Tegra's gpio.h contains a mix of private definitions for use inside the >>>>> GPIO driver and custom machine-specific APIs. Move the private >>>>> definitions >>>>> out of the global include directory since nothing should need to access >>>>> them. Move the public definitions to the machine-specific include >>>>> directory <mach/>. >>> >>> >>> >>>>> diff --git a/drivers/gpio/tegra_gpio_priv.h >>>>> b/drivers/gpio/tegra_gpio_priv.h >>> >>> >>> >>>>> +/* >>>>> + * GPIO registers are split into two chunks; low and high. >>>>> + * On Tegra20, all low chunks appear first, then all high chunks. >>>>> + * In later SoCs, the low and high chunks are interleaved together. >>>>> + */ >>>>> +#define GPIO_CTLR_BANK_HIGH_REGS \ >>>>> + uint gpio_masked_config[TEGRA_GPIO_PORTS]; \ >>>>> + uint gpio_masked_dir_out[TEGRA_GPIO_PORTS]; \ >>>>> + uint gpio_masked_out[TEGRA_GPIO_PORTS]; \ >>>>> + uint reserved0[TEGRA_GPIO_PORTS]; \ >>>>> + uint gpio_masked_int_status[TEGRA_GPIO_PORTS]; \ >>>>> + uint gpio_masked_int_enable[TEGRA_GPIO_PORTS]; \ >>>>> + uint gpio_masked_int_level[TEGRA_GPIO_PORTS]; \ >>>>> + uint reserved1[TEGRA_GPIO_PORTS]; >>>>> + >>>>> +/* GPIO Controller registers for a single bank */ >>>>> +struct gpio_ctlr_bank { >>>>> + uint gpio_config[TEGRA_GPIO_PORTS]; >>>>> + uint gpio_dir_out[TEGRA_GPIO_PORTS]; >>>>> + uint gpio_out[TEGRA_GPIO_PORTS]; >>>>> + uint gpio_in[TEGRA_GPIO_PORTS]; >>>>> + uint gpio_int_status[TEGRA_GPIO_PORTS]; >>>>> + uint gpio_int_enable[TEGRA_GPIO_PORTS]; >>>>> + uint gpio_int_level[TEGRA_GPIO_PORTS]; >>>>> + uint gpio_int_clear[TEGRA_GPIO_PORTS]; >>>>> +#ifndef CONFIG_TEGRA20 >>>>> + GPIO_CTLR_BANK_HIGH_REGS >>>>> +#endif >>>>> +}; >>>>> + >>>>> +#ifdef CONFIG_TEGRA20 >>>>> +struct gpio_ctlr_bank_high { >>>>> + GPIO_CTLR_BANK_HIGH_REGS >>>> >>>> >>>> >>>> This seems a bit ugly. Perhaps you could havestruct >>>> gpio_ctlr_high_regs and include that here? It adds a level of >>>> indirection but that doesn't seem very important. >>> >>> >>> >>> In newer Tegras, there's no differentiation between the two register sets >>> that were "low" and "high" in Tegra20. I'd rather not saddle the >>> non-Tegra20 >>> struct layouts with some odd naming/nesting just because the Tegra20 >>> layout >>> was odd. I don't see any problem with using a #define for this; it >>> doesn't >>> seem to make the code complex. >> >> >> OK, well then how about just duplicating the two structs, and dropping >> the #define? >> >> #ifdfef CONFIG_TEGRA20 >> struct gpio_ctlr_bank { >> >> }; >> #else >> struct gpio_ctlr_bank { >> }; >> #endif > > > Given that the driver doesn't use any registers in the high bank, and indeed > can't; the driver's reliance on structs rather than register defines means > that the high bank registers would have to be accessed differently between > Tegra20 and other SoCs, I propose simply not representing those registers. > Instead, how about: > > struct gpio_ctlr_bank { > uint gpio_config[TEGRA_GPIO_PORTS]; > uint gpio_dir_out[TEGRA_GPIO_PORTS]; > uint gpio_out[TEGRA_GPIO_PORTS]; > uint gpio_in[TEGRA_GPIO_PORTS]; > uint gpio_int_status[TEGRA_GPIO_PORTS]; > uint gpio_int_enable[TEGRA_GPIO_PORTS]; > uint gpio_int_level[TEGRA_GPIO_PORTS]; > uint gpio_int_clear[TEGRA_GPIO_PORTS]; > #ifndef CONFIG_TEGRA20 > uint unused[TEGRA_GPIO_PORTS * 8]; > #endif > }; > > struct gpio_ctlr { > struct gpio_ctlr_bank gpio_bank[TEGRA_GPIO_BANKS]; > }; > > That removes all the duplication, without any macros etc. > > Does that look reasonable? If I fixup the patch like that, I'll add a brief > comment describing what the unused registers are, similar to the one in > patch v1. SGTM > >>> I wonder if we should just convert away from structs for registers >>> entirely. >>> Everything in the HW docs is just numbers, matching those to the structs >>> is >>> always painful, and if we used #defines instead of structs, representing >>> this HW difference would end up being much cleaner and avoid using a >>> macro >>> to "cut/paste" a register list 2 times; see the way the Linux kernel >>> driver >>> handles this. >> >> >> Structs are definitely easier to read and particularly in this case >> where each struct element is an array. > > > I'm not really sure there's much objective difference between the > readability of the two. Arrays can easily be abstracted via a macro or > inline function so that the call site looks essentially identical; () vs []. IMO the code is much harder to follow when you need to look up macros, etc. C already supports arrays :-) > >> Why are you worried about code duplication in a header file? > > > I'm not sure why I would special case my concerns and ignore duplication in > certain locations yet still care about duplication in general or elsewhere? We commonly put ugliness in header files. So long as the resulting syntax (in C files) is pretty obvious and non-surprising, this makes sense. Most of the time these header files are ignored by humans when reading the code since it is pretty obvious from the C code what is going on. Examples include static inline to drop functions, hardware register definitions, bitfield definitions, #ifdef setup (see image.h), etc. Perhaps by that argument your original #define scheme is fine. I don't like things that make it hard to grep the code for stuff, but this is minor. So I'm going to withdraw my objection (sorry). Reviewed-by: Simon Glass <sjg@chromium.org> Regards, Simon
diff --git a/arch/arm/include/asm/arch-tegra114/gpio.h b/arch/arm/include/asm/arch-tegra114/gpio.h deleted file mode 100644 index d6eaa1bd40e8..000000000000 --- a/arch/arm/include/asm/arch-tegra114/gpio.h +++ /dev/null @@ -1,20 +0,0 @@ -/* - * Copyright (c) 2010-2013, NVIDIA CORPORATION. All rights reserved. - * - * SPDX-License-Identifier: GPL-2.0 - */ - -#ifndef _TEGRA114_GPIO_H_ -#define _TEGRA114_GPIO_H_ - -/* - * The Tegra114 GPIO controller has 246 GPIOS in 8 banks of 4 ports, - * each with 8 GPIOs. - */ -#define TEGRA_GPIO_PORTS 4 /* number of ports per bank */ -#define TEGRA_GPIO_BANKS 8 /* number of banks */ - -#include <asm/arch-tegra/gpio.h> -#include <asm/arch-tegra30/gpio.h> - -#endif /* _TEGRA114_GPIO_H_ */ diff --git a/arch/arm/include/asm/arch-tegra124/gpio.h b/arch/arm/include/asm/arch-tegra124/gpio.h deleted file mode 100644 index 8fddb63f44c2..000000000000 --- a/arch/arm/include/asm/arch-tegra124/gpio.h +++ /dev/null @@ -1,44 +0,0 @@ -/* - * (C) Copyright 2013-2016 - * NVIDIA Corporation <www.nvidia.com> - * - * SPDX-License-Identifier: GPL-2.0+ - */ - -#ifndef _TEGRA124_GPIO_H_ -#define _TEGRA124_GPIO_H_ - -/* - * The Tegra124 GPIO controller has 256 GPIOS in 8 banks of 4 ports, - * each with 8 GPIOs. - */ -#define TEGRA_GPIO_PORTS 4 /* number of ports per bank */ -#define TEGRA_GPIO_BANKS 8 /* number of banks */ - -#include <asm/arch-tegra/gpio.h> - -/* GPIO Controller registers for a single bank */ -struct gpio_ctlr_bank { - uint gpio_config[TEGRA_GPIO_PORTS]; - uint gpio_dir_out[TEGRA_GPIO_PORTS]; - uint gpio_out[TEGRA_GPIO_PORTS]; - uint gpio_in[TEGRA_GPIO_PORTS]; - uint gpio_int_status[TEGRA_GPIO_PORTS]; - uint gpio_int_enable[TEGRA_GPIO_PORTS]; - uint gpio_int_level[TEGRA_GPIO_PORTS]; - uint gpio_int_clear[TEGRA_GPIO_PORTS]; - uint gpio_masked_config[TEGRA_GPIO_PORTS]; - uint gpio_masked_dir_out[TEGRA_GPIO_PORTS]; - uint gpio_masked_out[TEGRA_GPIO_PORTS]; - uint gpio_masked_in[TEGRA_GPIO_PORTS]; - uint gpio_masked_int_status[TEGRA_GPIO_PORTS]; - uint gpio_masked_int_enable[TEGRA_GPIO_PORTS]; - uint gpio_masked_int_level[TEGRA_GPIO_PORTS]; - uint gpio_masked_int_clear[TEGRA_GPIO_PORTS]; -}; - -struct gpio_ctlr { - struct gpio_ctlr_bank gpio_bank[TEGRA_GPIO_BANKS]; -}; - -#endif /* _TEGRA124_GPIO_H_ */ diff --git a/arch/arm/include/asm/arch-tegra20/gpio.h b/arch/arm/include/asm/arch-tegra20/gpio.h deleted file mode 100644 index 46dcc28f727c..000000000000 --- a/arch/arm/include/asm/arch-tegra20/gpio.h +++ /dev/null @@ -1,36 +0,0 @@ -/* - * Copyright (c) 2011, Google Inc. All rights reserved. - * Portions Copyright 2011-2016 NVIDIA Corporation - * - * SPDX-License-Identifier: GPL-2.0+ - */ - -#ifndef _TEGRA20_GPIO_H_ -#define _TEGRA20_GPIO_H_ - -/* - * The Tegra 2x GPIO controller has 224 GPIOs arranged in 7 banks of 4 ports, - * each with 8 GPIOs. - */ -#define TEGRA_GPIO_PORTS 4 /* number of ports per bank */ -#define TEGRA_GPIO_BANKS 7 /* number of banks */ - -#include <asm/arch-tegra/gpio.h> - -/* GPIO Controller registers for a single bank */ -struct gpio_ctlr_bank { - uint gpio_config[TEGRA_GPIO_PORTS]; - uint gpio_dir_out[TEGRA_GPIO_PORTS]; - uint gpio_out[TEGRA_GPIO_PORTS]; - uint gpio_in[TEGRA_GPIO_PORTS]; - uint gpio_int_status[TEGRA_GPIO_PORTS]; - uint gpio_int_enable[TEGRA_GPIO_PORTS]; - uint gpio_int_level[TEGRA_GPIO_PORTS]; - uint gpio_int_clear[TEGRA_GPIO_PORTS]; -}; - -struct gpio_ctlr { - struct gpio_ctlr_bank gpio_bank[TEGRA_GPIO_BANKS]; -}; - -#endif /* TEGRA20_GPIO_H_ */ diff --git a/arch/arm/include/asm/arch-tegra210/gpio.h b/arch/arm/include/asm/arch-tegra210/gpio.h deleted file mode 100644 index f2279d0f3e92..000000000000 --- a/arch/arm/include/asm/arch-tegra210/gpio.h +++ /dev/null @@ -1,44 +0,0 @@ -/* - * (C) Copyright 2013-2016 - * NVIDIA Corporation <www.nvidia.com> - * - * SPDX-License-Identifier: GPL-2.0+ - */ - -#ifndef _TEGRA210_GPIO_H_ -#define _TEGRA210_GPIO_H_ - -/* - * The Tegra210 GPIO controller has 256 GPIOS in 8 banks of 4 ports, - * each with 8 GPIOs. - */ -#define TEGRA_GPIO_PORTS 4 /* number of ports per bank */ -#define TEGRA_GPIO_BANKS 8 /* number of banks */ - -#include <asm/arch-tegra/gpio.h> - -/* GPIO Controller registers for a single bank */ -struct gpio_ctlr_bank { - uint gpio_config[TEGRA_GPIO_PORTS]; - uint gpio_dir_out[TEGRA_GPIO_PORTS]; - uint gpio_out[TEGRA_GPIO_PORTS]; - uint gpio_in[TEGRA_GPIO_PORTS]; - uint gpio_int_status[TEGRA_GPIO_PORTS]; - uint gpio_int_enable[TEGRA_GPIO_PORTS]; - uint gpio_int_level[TEGRA_GPIO_PORTS]; - uint gpio_int_clear[TEGRA_GPIO_PORTS]; - uint gpio_masked_config[TEGRA_GPIO_PORTS]; - uint gpio_masked_dir_out[TEGRA_GPIO_PORTS]; - uint gpio_masked_out[TEGRA_GPIO_PORTS]; - uint gpio_masked_in[TEGRA_GPIO_PORTS]; - uint gpio_masked_int_status[TEGRA_GPIO_PORTS]; - uint gpio_masked_int_enable[TEGRA_GPIO_PORTS]; - uint gpio_masked_int_level[TEGRA_GPIO_PORTS]; - uint gpio_masked_int_clear[TEGRA_GPIO_PORTS]; -}; - -struct gpio_ctlr { - struct gpio_ctlr_bank gpio_bank[TEGRA_GPIO_BANKS]; -}; - -#endif /* _TEGRA210_GPIO_H_ */ diff --git a/arch/arm/include/asm/arch-tegra30/gpio.h b/arch/arm/include/asm/arch-tegra30/gpio.h deleted file mode 100644 index 288451df2ff6..000000000000 --- a/arch/arm/include/asm/arch-tegra30/gpio.h +++ /dev/null @@ -1,43 +0,0 @@ -/* - * Copyright (c) 2010-2016, NVIDIA CORPORATION. All rights reserved. - * - * SPDX-License-Identifier: GPL-2.0 - */ - -#ifndef _TEGRA30_GPIO_H_ -#define _TEGRA30_GPIO_H_ - -/* - * The Tegra 3x GPIO controller has 246 GPIOS in 8 banks of 4 ports, - * each with 8 GPIOs. - */ -#define TEGRA_GPIO_PORTS 4 /* number of ports per bank */ -#define TEGRA_GPIO_BANKS 8 /* number of banks */ - -#include <asm/arch-tegra/gpio.h> - -/* GPIO Controller registers for a single bank */ -struct gpio_ctlr_bank { - uint gpio_config[TEGRA_GPIO_PORTS]; - uint gpio_dir_out[TEGRA_GPIO_PORTS]; - uint gpio_out[TEGRA_GPIO_PORTS]; - uint gpio_in[TEGRA_GPIO_PORTS]; - uint gpio_int_status[TEGRA_GPIO_PORTS]; - uint gpio_int_enable[TEGRA_GPIO_PORTS]; - uint gpio_int_level[TEGRA_GPIO_PORTS]; - uint gpio_int_clear[TEGRA_GPIO_PORTS]; - uint gpio_masked_config[TEGRA_GPIO_PORTS]; - uint gpio_masked_dir_out[TEGRA_GPIO_PORTS]; - uint gpio_masked_out[TEGRA_GPIO_PORTS]; - uint gpio_masked_in[TEGRA_GPIO_PORTS]; - uint gpio_masked_int_status[TEGRA_GPIO_PORTS]; - uint gpio_masked_int_enable[TEGRA_GPIO_PORTS]; - uint gpio_masked_int_level[TEGRA_GPIO_PORTS]; - uint gpio_masked_int_clear[TEGRA_GPIO_PORTS]; -}; - -struct gpio_ctlr { - struct gpio_ctlr_bank gpio_bank[TEGRA_GPIO_BANKS]; -}; - -#endif /* _TEGRA30_GPIO_H_ */ diff --git a/arch/arm/include/asm/gpio.h b/arch/arm/include/asm/gpio.h index fe4419cae4cf..751a5ade17c7 100644 --- a/arch/arm/include/asm/gpio.h +++ b/arch/arm/include/asm/gpio.h @@ -1,4 +1,4 @@ -#ifndef CONFIG_ARCH_UNIPHIER +#if !defined(CONFIG_ARCH_UNIPHIER) && !defined(CONFIG_TEGRA) #include <asm/arch/gpio.h> #endif #include <asm-generic/gpio.h> diff --git a/arch/arm/include/asm/arch-tegra/gpio.h b/arch/arm/mach-tegra/include/mach/tegra_gpio.h similarity index 67% rename from arch/arm/include/asm/arch-tegra/gpio.h rename to arch/arm/mach-tegra/include/mach/tegra_gpio.h index 07921f34b9d7..09b813bc79ac 100644 --- a/arch/arm/include/asm/arch-tegra/gpio.h +++ b/arch/arm/mach-tegra/include/mach/tegra_gpio.h @@ -5,15 +5,8 @@ * SPDX-License-Identifier: GPL-2.0+ */ -#ifndef _TEGRA_GPIO_H_ -#define _TEGRA_GPIO_H_ - -#define TEGRA_GPIOS_PER_PORT 8 - -#define GPIO_BANK(x) ((x) >> 5) -#define GPIO_PORT(x) (((x) >> 3) & 0x3) -#define GPIO_FULLPORT(x) ((x) >> 3) -#define GPIO_BIT(x) ((x) & 0x7) +#ifndef _MACH_TEGRA_GPIO_H +#define _MACH_TEGRA_GPIO_H enum tegra_gpio_init { TEGRA_GPIO_INIT_IN, @@ -34,4 +27,4 @@ struct tegra_gpio_config { */ void gpio_config_table(const struct tegra_gpio_config *config, int len); -#endif /* TEGRA_GPIO_H_ */ +#endif diff --git a/board/nvidia/e2220-1170/e2220-1170.c b/board/nvidia/e2220-1170/e2220-1170.c index db6fa74ae1fe..c2b5e5e09d21 100644 --- a/board/nvidia/e2220-1170/e2220-1170.c +++ b/board/nvidia/e2220-1170/e2220-1170.c @@ -8,8 +8,9 @@ #include <common.h> #include <i2c.h> #include <dt-bindings/gpio/tegra-gpio.h> -#include <asm/arch/gpio.h> +#include <asm/gpio.h> #include <asm/arch/pinmux.h> +#include <mach/tegra_gpio.h> #include "../p2571/max77620_init.h" #include "pinmux-config-e2220-1170.h" diff --git a/board/nvidia/jetson-tk1/jetson-tk1.c b/board/nvidia/jetson-tk1/jetson-tk1.c index 4b7058e3bc89..422a18a4e530 100644 --- a/board/nvidia/jetson-tk1/jetson-tk1.c +++ b/board/nvidia/jetson-tk1/jetson-tk1.c @@ -8,8 +8,9 @@ #include <common.h> #include <dt-bindings/gpio/tegra-gpio.h> #include <power/as3722.h> -#include <asm/arch/gpio.h> +#include <asm/gpio.h> #include <asm/arch/pinmux.h> +#include <mach/tegra_gpio.h> #include "pinmux-config-jetson-tk1.h" DECLARE_GLOBAL_DATA_PTR; diff --git a/board/nvidia/nyan-big/nyan-big.c b/board/nvidia/nyan-big/nyan-big.c index 56e15bda93ec..64d98f59a4c7 100644 --- a/board/nvidia/nyan-big/nyan-big.c +++ b/board/nvidia/nyan-big/nyan-big.c @@ -17,6 +17,7 @@ #include <asm/arch/mc.h> #include <asm/arch-tegra/clk_rst.h> #include <asm/arch-tegra/pmc.h> +#include <mach/tegra_gpio.h> #include "pinmux-config-nyan-big.h" /* diff --git a/board/nvidia/p2371-0000/p2371-0000.c b/board/nvidia/p2371-0000/p2371-0000.c index 0fec3d497d7d..a23dfc8afaef 100644 --- a/board/nvidia/p2371-0000/p2371-0000.c +++ b/board/nvidia/p2371-0000/p2371-0000.c @@ -8,8 +8,9 @@ #include <common.h> #include <i2c.h> #include <dt-bindings/gpio/tegra-gpio.h> -#include <asm/arch/gpio.h> +#include <asm/gpio.h> #include <asm/arch/pinmux.h> +#include <mach/tegra_gpio.h> #include "../p2571/max77620_init.h" #include "pinmux-config-p2371-0000.h" diff --git a/board/nvidia/p2371-2180/p2371-2180.c b/board/nvidia/p2371-2180/p2371-2180.c index bc3389976942..3989ab8bde32 100644 --- a/board/nvidia/p2371-2180/p2371-2180.c +++ b/board/nvidia/p2371-2180/p2371-2180.c @@ -8,8 +8,9 @@ #include <common.h> #include <i2c.h> #include <dt-bindings/gpio/tegra-gpio.h> -#include <asm/arch/gpio.h> +#include <asm/gpio.h> #include <asm/arch/pinmux.h> +#include <mach/tegra_gpio.h> #include "../p2571/max77620_init.h" #include "pinmux-config-p2371-2180.h" diff --git a/board/nvidia/p2571/p2571.c b/board/nvidia/p2571/p2571.c index 794830bac3b3..7faec788f3cf 100644 --- a/board/nvidia/p2571/p2571.c +++ b/board/nvidia/p2571/p2571.c @@ -9,8 +9,8 @@ #include <i2c.h> #include <dt-bindings/gpio/tegra-gpio.h> #include <asm/gpio.h> -#include <asm/arch/gpio.h> #include <asm/arch/pinmux.h> +#include <mach/tegra_gpio.h> #include "max77620_init.h" #include "pinmux-config-p2571.h" diff --git a/board/nvidia/venice2/venice2.c b/board/nvidia/venice2/venice2.c index 4373de6a26bd..5791e0d73522 100644 --- a/board/nvidia/venice2/venice2.c +++ b/board/nvidia/venice2/venice2.c @@ -7,8 +7,9 @@ #include <common.h> #include <dt-bindings/gpio/tegra-gpio.h> -#include <asm/arch/gpio.h> +#include <asm/gpio.h> #include <asm/arch/pinmux.h> +#include <mach/tegra_gpio.h> #include "pinmux-config-venice2.h" /* diff --git a/drivers/gpio/tegra_gpio.c b/drivers/gpio/tegra_gpio.c index 9abab13daaa9..a3b6878e47bd 100644 --- a/drivers/gpio/tegra_gpio.c +++ b/drivers/gpio/tegra_gpio.c @@ -22,6 +22,8 @@ #include <asm/gpio.h> #include <dm/device-internal.h> #include <dt-bindings/gpio/gpio.h> +#include <mach/tegra_gpio.h> +#include "tegra_gpio_priv.h" DECLARE_GLOBAL_DATA_PTR; diff --git a/drivers/gpio/tegra_gpio_priv.h b/drivers/gpio/tegra_gpio_priv.h new file mode 100644 index 000000000000..edf5540da72f --- /dev/null +++ b/drivers/gpio/tegra_gpio_priv.h @@ -0,0 +1,76 @@ +/* + * Copyright (c) 2011, Google Inc. All rights reserved. + * Copyright 2011-2016 NVIDIA Corporation + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef _TEGRA_GPIO_PRIV_H +#define _TEGRA_GPIO_PRIV_H + +/* + * GPIOs are arranged into ports, each of which controls 8 GPIOs. + * + * Ports are aggregated into banks (U-Boot terminology used below) or + * "controllers" (Tegra TRM terminology) each containing 4 ports. + * + * Banks/controllers are aggregated into the overall GPIO controller hardware + * module. Tegra20 implements only 7 banks. However, the register location of + * the "masked" registers is designed to leave space for 8 banks. Tegra30 and + * onwards implement 8 banks. In many cases, the last bank contains ports + * and/or GPIOs that are not implemented, or at least not connected to any + * pad/pin. + */ +#define TEGRA_GPIO_BANKS 8 /* number of banks */ +#define TEGRA_GPIO_PORTS 4 /* number of ports per bank */ +#define TEGRA_GPIOS_PER_PORT 8 + +#define GPIO_BANK(x) ((x) >> 5) +#define GPIO_PORT(x) (((x) >> 3) & 0x3) +#define GPIO_FULLPORT(x) ((x) >> 3) +#define GPIO_BIT(x) ((x) & 0x7) + +/* + * GPIO registers are split into two chunks; low and high. + * On Tegra20, all low chunks appear first, then all high chunks. + * In later SoCs, the low and high chunks are interleaved together. + */ +#define GPIO_CTLR_BANK_HIGH_REGS \ + uint gpio_masked_config[TEGRA_GPIO_PORTS]; \ + uint gpio_masked_dir_out[TEGRA_GPIO_PORTS]; \ + uint gpio_masked_out[TEGRA_GPIO_PORTS]; \ + uint reserved0[TEGRA_GPIO_PORTS]; \ + uint gpio_masked_int_status[TEGRA_GPIO_PORTS]; \ + uint gpio_masked_int_enable[TEGRA_GPIO_PORTS]; \ + uint gpio_masked_int_level[TEGRA_GPIO_PORTS]; \ + uint reserved1[TEGRA_GPIO_PORTS]; + +/* GPIO Controller registers for a single bank */ +struct gpio_ctlr_bank { + uint gpio_config[TEGRA_GPIO_PORTS]; + uint gpio_dir_out[TEGRA_GPIO_PORTS]; + uint gpio_out[TEGRA_GPIO_PORTS]; + uint gpio_in[TEGRA_GPIO_PORTS]; + uint gpio_int_status[TEGRA_GPIO_PORTS]; + uint gpio_int_enable[TEGRA_GPIO_PORTS]; + uint gpio_int_level[TEGRA_GPIO_PORTS]; + uint gpio_int_clear[TEGRA_GPIO_PORTS]; +#ifndef CONFIG_TEGRA20 + GPIO_CTLR_BANK_HIGH_REGS +#endif +}; + +#ifdef CONFIG_TEGRA20 +struct gpio_ctlr_bank_high { + GPIO_CTLR_BANK_HIGH_REGS +}; +#endif + +struct gpio_ctlr { + struct gpio_ctlr_bank gpio_bank[TEGRA_GPIO_BANKS]; +#ifdef CONFIG_TEGRA20 + struct gpio_ctlr_bank_high gpio_bank_high[TEGRA_GPIO_BANKS]; +#endif +}; + +#endif diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c index 2a8ab2d5e0b9..f1b8877be228 100644 --- a/drivers/i2c/tegra_i2c.c +++ b/drivers/i2c/tegra_i2c.c @@ -14,7 +14,6 @@ #include <asm/io.h> #include <asm/arch/clock.h> #include <asm/arch/funcmux.h> -#include <asm/arch/gpio.h> #include <asm/arch/pinmux.h> #include <asm/arch-tegra/clk_rst.h> #include <mach/tegra_i2c.h>