[U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller
Simon Glass
sjg at chromium.org
Sun May 1 19:46:01 CEST 2016
Hi Mario,
On 26 April 2016 at 08:08, Mario Six <mario.six at gdsys.cc> wrote:
>
> Signed-off-by: Mario Six <mario.six at gdsys.cc>
> ---
> arch/powerpc/include/asm/arch-mpc85xx/gpio.h | 2 +
> arch/powerpc/include/asm/immap_85xx.h | 2 +
> drivers/gpio/Kconfig | 6 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/mpc85xx_gpio.c | 182 +++++++++++++++++++++++++++
> 5 files changed, 193 insertions(+)
> create mode 100644 drivers/gpio/mpc85xx_gpio.c
>
Seems OK but I have some comments below.
> diff --git a/arch/powerpc/include/asm/arch-mpc85xx/gpio.h b/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
> index da7352a..41b6677 100644
> --- a/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
> +++ b/arch/powerpc/include/asm/arch-mpc85xx/gpio.h
> @@ -14,6 +14,8 @@
> #ifndef __ASM_ARCH_MX85XX_GPIO_H
> #define __ASM_ARCH_MX85XX_GPIO_H
>
> +#ifndef CONFIG_MPC85XX_GPIO
> #include <asm/mpc85xx_gpio.h>
> +#endif
>
> #endif
> diff --git a/arch/powerpc/include/asm/immap_85xx.h b/arch/powerpc/include/asm/immap_85xx.h
> index 53ca6d9..dcc50b2 100644
> --- a/arch/powerpc/include/asm/immap_85xx.h
> +++ b/arch/powerpc/include/asm/immap_85xx.h
> @@ -265,6 +265,7 @@ typedef struct ccsr_pcix {
> #define PIWAR_WRITE_SNOOP 0x00005000
> #define PIWAR_MEM_2G 0x0000001e
>
> +#ifndef CONFIG_MPC85XX_GPIO
> typedef struct ccsr_gpio {
> u32 gpdir;
> u32 gpodr;
> @@ -273,6 +274,7 @@ typedef struct ccsr_gpio {
> u32 gpimr;
> u32 gpicr;
> } ccsr_gpio_t;
> +#endif
>
> /* L2 Cache Registers */
> typedef struct ccsr_l2cache {
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 2b4624d..72a5bdc 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -143,4 +143,10 @@ config ZYNQ_GPIO
> help
> Supports GPIO access on Zynq SoC.
>
> +config MPC85XX_GPIO
> + bool "MPC85xx GPIO driver"
> + depends on DM_GPIO && MPC85xx
> + help
> + This driver supports the built-in GPIO controller of MPC85XX CPUs.
Can you please add a bit more info - how many GPIOs and banks, what
features are supported (input/output/etc.)
> +
> endmenu
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 4f071c4..1e4f16b 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_DA8XX_GPIO) += da8xx_gpio.o
> obj-$(CONFIG_DM644X_GPIO) += da8xx_gpio.o
> obj-$(CONFIG_ALTERA_PIO) += altera_pio.o
> obj-$(CONFIG_MPC83XX_GPIO) += mpc83xx_gpio.o
> +obj-$(CONFIG_MPC85XX_GPIO) += mpc85xx_gpio.o
> obj-$(CONFIG_SH_GPIO_PFC) += sh_pfc.o
> obj-$(CONFIG_OMAP_GPIO) += omap_gpio.o
> obj-$(CONFIG_DB8500_GPIO) += db8500_gpio.o
> diff --git a/drivers/gpio/mpc85xx_gpio.c b/drivers/gpio/mpc85xx_gpio.c
> new file mode 100644
> index 0000000..2289eb7
> --- /dev/null
> +++ b/drivers/gpio/mpc85xx_gpio.c
> @@ -0,0 +1,182 @@
> +/*
> + * (C) Copyright 2016
> + * Mario Six, Guntermann & Drunck GmbH, six at gdsys.de
> + *
> + * based on arch/powerpc/include/asm/mpc85xx_gpio.h, which is
> + *
> + * Copyright 2010 eXMeritus, A Boeing Company
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <asm/gpio.h>
> +#include <mapmem.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct ccsr_gpio {
> + u32 gpdir;
> + u32 gpodr;
> + u32 gpdat;
> + u32 gpier;
> + u32 gpimr;
> + u32 gpicr;
> +};
> +
> +struct mpc85xx_gpio_data {
> + struct ccsr_gpio __iomem *base;
> + u32 addr;
ulong?
> + u8 gpio_count;
uint is better
Also please add comments on these 3 members.
> +};
> +
> +static inline unsigned int
please put on same line
> +mpc85xx_gpio_get_val(struct ccsr_gpio *base, unsigned int mask)
> +{
> + /* Read the requested values */
> + return in_be32(&base->gpdat) & mask;
> +}
> +
> +static inline unsigned int
> +mpc85xx_gpio_get_dir(struct ccsr_gpio *base, unsigned int mask)
> +{
> + /* Read the requested values */
> + return in_be32(&base->gpdir) & mask;
> +}
> +
> +static inline void
> +mpc85xx_gpio_set_in(struct ccsr_gpio *base, unsigned int gpios)
> +{
> + clrbits_be32(&base->gpdat, gpios);
> + clrbits_be32(&base->gpdir, gpios);
> +}
> +
> +static inline void
> +mpc85xx_gpio_set_low(struct ccsr_gpio *base, unsigned int gpios)
> +{
> + clrbits_be32(&base->gpdat, gpios);
> + setbits_be32(&base->gpdir, gpios);
> +}
> +
> +static inline void
> +mpc85xx_gpio_set_high(struct ccsr_gpio *base, unsigned int gpios)
> +{
> + setbits_be32(&base->gpdat, gpios);
> + setbits_be32(&base->gpdir, gpios);
> +}
> +
> +static int mpc85xx_gpio_direction_input(struct udevice *dev, unsigned int gpio)
> +{
> + struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> +
> + mpc85xx_gpio_set_in(data->base, 1U << (31 - gpio));
How about defining something at the top and using that throughout:
#define GPIO_MASK(gpio) (1U << (31 - (gpio)))
> + return 0;
> +}
> +
> +static int
> +mpc85xx_gpio_set_value(struct udevice *dev, unsigned gpio, int value)
> +{
> + struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> +
> + if (value)
> + mpc85xx_gpio_set_high(data->base, 1U << (31 - gpio));
> + else
> + mpc85xx_gpio_set_low(data->base, 1U << (31 - gpio));
> + return 0;
> +}
> +
> +static int
> +mpc85xx_gpio_direction_output(struct udevice *dev, unsigned gpio, int value)
> +{
> + struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> +
> + if (value)
> + mpc85xx_gpio_set_high(data->base, 1U << (31 - gpio));
> + else
> + mpc85xx_gpio_set_low(data->base, 1U << (31 - gpio));
> + return 0;
> +}
> +
> +static int
> +mpc85xx_gpio_get_value(struct udevice *dev, unsigned gpio)
> +{
> + struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> +
> + return !!mpc85xx_gpio_get_val(data->base, 1U << (31 - gpio));
> +}
> +
> +static int
> +mpc85xx_gpio_get_function(struct udevice *dev, unsigned gpio)
> +{
> + struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> + int dir;
> +
> + dir = !!mpc85xx_gpio_get_dir(data->base, 1U << (31 - gpio));
> + return dir ? GPIOF_OUTPUT : GPIOF_INPUT;
> +}
> +
> +static int
> +mpc85xx_gpio_ofdata_to_platdata(struct udevice *dev) {
> + struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> + u64 reg;
> + u32 addr, size;
> +
> + reg = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
This seem really odd. It seems like it returns a 64-bit value but you
turn it into two 32-bit values. My suggestions:
- create dev_map_sysmem() ius
- figure out why apparently phys_addr_t is 64 bits but your device
tree has 32-bit addresses
> + addr = reg >> 32;
> + size = reg & 0xFFFFFFFF;
> +
> + data->addr = addr;
> + data->base = map_sysmem(CONFIG_SYS_IMMR + addr, size);
> +
> + if (!data->base)
> + return -ENOMEM;
> +
> + data->gpio_count = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
> + "ngpios", 32);
> +
> + return 0;
> +}
> +
> +static int
> +mpc85xx_gpio_probe(struct udevice *dev)
> +{
> + struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> + struct mpc85xx_gpio_data *data = dev_get_priv(dev);
> + char name[32], *str;
> +
> + snprintf(name, sizeof(name), "MPC@%x_", data->addr);
> + str = strdup(name);
> +
> + if (!str)
> + return -ENOMEM;
> +
> + uc_priv->bank_name = str;
> +
Drop blank line.
> + uc_priv->gpio_count = data->gpio_count;
> +
> + return 0;
> +}
> +
> +static const struct dm_gpio_ops gpio_mpc85xx_ops = {
> + .direction_input = mpc85xx_gpio_direction_input,
> + .direction_output = mpc85xx_gpio_direction_output,
> + .get_value = mpc85xx_gpio_get_value,
> + .set_value = mpc85xx_gpio_set_value,
> + .get_function = mpc85xx_gpio_get_function,
> +};
> +
> +static const struct udevice_id mpc85xx_gpio_ids[] = {
> + { .compatible = "fsl,pq3-gpio" },
> + { /* sentinel */ }
> +};
> +
> +U_BOOT_DRIVER(gpio_mpc85xx) = {
> + .name = "gpio_mpc85xx",
> + .id = UCLASS_GPIO,
> + .ops = &gpio_mpc85xx_ops,
> + .ofdata_to_platdata = mpc85xx_gpio_ofdata_to_platdata,
> + .of_match = mpc85xx_gpio_ids,
> + .probe = mpc85xx_gpio_probe,
> + .priv_auto_alloc_size = sizeof(struct mpc85xx_gpio_data),
> +};
> --
> 2.7.0.GIT
>
Regards,
Simon
More information about the U-Boot
mailing list