[U-Boot] [PATCH 1/3] dm: gpio: Add driver for MPC85XX GPIO controller
Mario Six
mario.six at gdsys.cc
Mon May 2 13:43:27 CEST 2016
Hi Simon,
On Sun, May 1, 2016 at 7:46 PM, Simon Glass <sjg at chromium.org> wrote:
> 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
>
Regarding the address width discrepancy: The system I'm working on is a
P1022 Qoriq, which has 36 bit width, which implies that phys_addr_t needs
to be 64 bit. But the everything else (including the GPIO controller) uses
32 bits, thus the device tree addresses are 32 bit wide. I'm not quite sure
how to handle this difference; DM support for this platform is brand-new,
so there are no drivers to look to for guidance.
With "dev_map_sysmem" you mean a function that takes the value of the "reg"
property and returns the mapped memory region, I presume?
Everything else will be addressed in v2.
Thanks for reviewing!
Best regards,
Mario
More information about the U-Boot
mailing list