Re: [U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller

Hi Mario,

On 26 April 2016 at 08:08, Mario Six mario.six@gdsys.cc wrote:

Signed-off-by: Mario Six mario.six@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@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