[U-Boot] [U-BOOT PATCH] gpio: fu540: add support for DM based gpio driver for FU540 SoC
Bin Meng
bmeng.cn at gmail.com
Wed Aug 28 02:44:38 UTC 2019
Hi Sagar,
On Wed, Aug 28, 2019 at 6:32 AM Sagar Kadam <sagar.kadam at sifive.com> wrote:
>
> Hi Bin,
>
> On Thu, Aug 22, 2019 at 8:12 PM Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > On Fri, Aug 23, 2019 at 9:02 AM Sagar Shrikant Kadam
> > <sagar.kadam at sifive.com> wrote:
> > >
> > > This patch adds a DM based driver model for gpio controller present in
> > > FU540-C000 SoC on HiFive Unleashed A00 board. This SoC has one GPIO
> > > bank and 16 GPIO lines in total, out of which GPIO0 to GPIO9 and
> > > GPIO15 are routed to the J1 header on the board.
> > >
> > > This implementation is ported from linux based gpio driver submitted
> > > for review by Wesley W. Terpstra <wesley at sifive.com> and/or Atish Patra
> > > <atish.patra at wdc.com>. The linux driver can be referred
> > > here [1]
> > >
> > > [1]: https://lkml.org/lkml/2018/10/9/1103
> > >
> > > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam at sifive.com>
> > > ---
> > > arch/riscv/include/asm/arch-generic/gpio.h | 35 +++++++
> > > arch/riscv/include/asm/gpio.h | 6 ++
> > > board/sifive/fu540/Kconfig | 3 +
> > > drivers/gpio/Kconfig | 8 ++
> > > drivers/gpio/Makefile | 1 +
> > > drivers/gpio/fu540-gpio.c | 145 +++++++++++++++++++++++++++++
> > > 6 files changed, 198 insertions(+)
> > > create mode 100644 arch/riscv/include/asm/arch-generic/gpio.h
> > > create mode 100644 arch/riscv/include/asm/gpio.h
> > > create mode 100644 drivers/gpio/fu540-gpio.c
> >
>
> Thanks for the review.
>
> > Pleas split this into two patch:
> >
> > patch [1/2]: add the gpio driver
> > patch [2/2]: enable the gpio driver in sifive/fu540
> >
> Yes, I will split in the next version.
>
> > Looks DTS change is missing??
> >
> The mainline linux bound device tree for hifive-unleashed can be passed to
> OpenSBI using FW_PAYLOAD_FDT_PATH. The necessary device
> tree changes to enable gpio can be found in dev/sagark/mlv5.3-rc5 branch of
> https://github.com/sagsifive/riscv-linux-hifive
Oops, I forgot that!
>
> > >
> > > diff --git a/arch/riscv/include/asm/arch-generic/gpio.h b/arch/riscv/include/asm/arch-generic/gpio.h
> > > new file mode 100644
> > > index 0000000..bedb8d8
> > > --- /dev/null
> > > +++ b/arch/riscv/include/asm/arch-generic/gpio.h
> > > @@ -0,0 +1,35 @@
> > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > +/*
> > > + * Copyright (C) 2019 SiFive, Inc.
> > > + */
> > > +
> > > +#ifndef _GPIO_FU540_H
> > > +#define _GPIO_FU540_H
> > > +
> > > +#define GPIO_INPUT_VAL 0x00
> > > +#define GPIO_INPUT_EN 0x04
> > > +#define GPIO_OUTPUT_EN 0x08
> > > +#define GPIO_OUTPUT_VAL 0x0C
> > > +#define GPIO_RISE_IE 0x18
> > > +#define GPIO_RISE_IP 0x1C
> > > +#define GPIO_FALL_IE 0x20
> > > +#define GPIO_FALL_IP 0x24
> > > +#define GPIO_HIGH_IE 0x28
> > > +#define GPIO_HIGH_IP 0x2C
> > > +#define GPIO_LOW_IE 0x30
> > > +#define GPIO_LOW_IP 0x34
> > > +#define GPIO_OUTPUT_XOR 0x40
> > > +
> > > +#define NR_GPIOS 16
> > > +
> > > +enum gpio_state {
> > > + LOW,
> > > + HIGH
> > > +};
> > > +
> > > +/* Details about a GPIO bank */
> > > +struct fu540_gpio_platdata {
> > > + u8 *base; /* address of registers in physical memory */
> >
> > Shouldn't it be u32?
> Yes, will update in next version of the patch
> >
> > > +};
> > > +
> > > +#endif /* _GPIO_FU540_H */
> > > diff --git a/arch/riscv/include/asm/gpio.h b/arch/riscv/include/asm/gpio.h
> > > new file mode 100644
> > > index 0000000..008d756
> > > --- /dev/null
> > > +++ b/arch/riscv/include/asm/gpio.h
> > > @@ -0,0 +1,6 @@
> > > +/* SPDX-License-Identifier: GPL-2.0+ */
> > > +/*
> > > + * Copyright 2018 SiFive, Inc.
> > > + */
> > > +
> > > +#include <asm-generic/gpio.h>
> > > diff --git a/board/sifive/fu540/Kconfig b/board/sifive/fu540/Kconfig
> > > index 5d65080..f939ed2 100644
> > > --- a/board/sifive/fu540/Kconfig
> > > +++ b/board/sifive/fu540/Kconfig
> > > @@ -44,6 +44,9 @@ config BOARD_SPECIFIC_OPTIONS # dummy
> > > imply MMC_SPI
> > > imply MMC_BROKEN_CD
> > > imply CMD_MMC
> > > + imply DM_GPIO
> > > + imply FU540_GPIO
> > > + imply CMD_GPIO
> > > imply SMP
> > >
> > > endif
> > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > > index 7d9c97f..b93092a 100644
> > > --- a/drivers/gpio/Kconfig
> > > +++ b/drivers/gpio/Kconfig
> > > @@ -280,6 +280,14 @@ config STM32_GPIO
> > > usable on many stm32 families like stm32f4/f7/h7 and stm32mp1.
> > > Tested on STM32F7.
> > >
> > > +config FU540_GPIO
> >
> > rename this to SIFIVE_GPIO
>
> Few drivers followed names like <VENDOR>_GPIO and few are like
> <SOC>_GPIO.
> I will change it to SIFIVE_GPIO in new version of patch
No, it depends. If this GPIO IP is solely used by all SoCs from a
vendor we should name it as <vendor>_gpio. Otherwise we can name it
<soc>_gpio indicating that is an SoC-specific GPIO driver. This is the
common naming practice in both U-Boot and Linux kernel.
> >
> > > + bool "FU540 GPIO Driver"
> > > + depends on DM_GPIO
> > > + help
> > > + Device model driver for GPIO controller present in FU540 SoC. This
> > > + driver enables GPIO interface on HiFive Unleashed A00 board a board
> > > + from SiFive Inc. having FU540-C000 SoC.
> > > +
> > > config MVEBU_GPIO
> > > bool "Marvell MVEBU GPIO driver"
> > > depends on DM_GPIO && ARCH_MVEBU
> > > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > > index 4a8aa0f..238ad17 100644
> > > --- a/drivers/gpio/Makefile
> > > +++ b/drivers/gpio/Makefile
> > > @@ -61,3 +61,4 @@ obj-$(CONFIG_$(SPL_)PCF8575_GPIO) += pcf8575_gpio.o
> > > obj-$(CONFIG_PM8916_GPIO) += pm8916_gpio.o
> > > obj-$(CONFIG_MT7621_GPIO) += mt7621_gpio.o
> > > obj-$(CONFIG_MSCC_SGPIO) += mscc_sgpio.o
> > > +obj-$(CONFIG_FU540_GPIO) += fu540-gpio.o
> > > diff --git a/drivers/gpio/fu540-gpio.c b/drivers/gpio/fu540-gpio.c
> >
> > I think the name should be: sifive_gpio.c
> >
> Yes, this will also change.
>
> > > new file mode 100644
> > > index 0000000..7761689
> > > --- /dev/null
> > > +++ b/drivers/gpio/fu540-gpio.c
> > > @@ -0,0 +1,145 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * SiFive GPIO driver
> > > + *
> > > + * Copyright (C) 2019 SiFive, Inc.
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> >
> > No need this because there is already SPDX in the first line.
> >
> Agree. Will remove the statement "This program ..... Foundation"
>
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <dm.h>
> > > +#include <asm/arch/gpio.h>
> > > +#include <asm/io.h>
> > > +#include <errno.h>
> > > +#include <asm-generic/gpio.h>
> >
> > I think you need include <asm/gpio.h>?
> >
> Yeah, Good catch !!
> I will change it.
> > > +
> > > +static int fu540_gpio_probe(struct udevice *dev)
> > > +{
> > > + struct fu540_gpio_platdata *plat = dev_get_platdata(dev);
> > > + struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> > > + char name[18], *str;
> > > +
> > > + sprintf(name, "gpio@%4p", plat->base);
> > > + str = strdup(name);
> > > + if (!str)
> > > + return -ENOMEM;
> > > + uc_priv->bank_name = str;
> >
> > Shouldn't we directly use dev->name as the bank_name?
> Yes, dev->name holds the device node name, and so
> can be used to update the bank_name within gpio_dev_priv,
> so that it will be available for uclass if required.
> I will skip creating the duplicate string and using it to generate the
> bank name for uc_priv, in next version as :
> uc_priv->bank_name = dev->name
>
> Does it seem ok?
Yes.
>
> >
> > > + uc_priv->gpio_count = NR_GPIOS;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void fu540_update_gpio_reg(u8 *bptr, u32 offset, bool value)
> >
> > Why bool value?
> >
> > Linux driver uses int value, and why not you just reuse the same Linux
> > function name (sifive_assign_bit) and parameters?
> >
> The argument value here is just to signify if the offset bit should be masked
> or not, it doesn't play any role in the value been written to the register.
> IMHO, bool should be sufficient here.
>
> > > +{
> > > + void __iomem *ptr = (void __iomem *)bptr;
> > > +
> > > + u32 bit = BIT(offset), old = readl(ptr);
> >
> > nits: don't do this in a single line
> >
> Ok. will update
> > > +
> > > + if (value)
> > > + writel(old | bit, ptr);
> > > + else
> > > + writel(old & ~bit, ptr);
> > > +}
> > > +
> > > +static int fu540_gpio_direction_input(struct udevice *dev, u32 offset)
> > > +{
> > > + struct fu540_gpio_platdata *plat = dev_get_platdata(dev);
> > > +
> > > + if (offset > NR_GPIOS)
> > > + return -EINVAL;
> > > +
> > > + /* Configure GPIO direction as input. */
> >
> > nits: remove the ending period
> Ok. will update
> >
> > > + fu540_update_gpio_reg(plat->base + GPIO_INPUT_EN, offset, true);
> > > + fu540_update_gpio_reg(plat->base + GPIO_OUTPUT_EN, offset, false);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int fu540_gpio_direction_output(struct udevice *dev, u32 offset,
> > > + int value)
> > > +{
> > > + struct fu540_gpio_platdata *plat = dev_get_platdata(dev);
> > > +
> > > + if (offset > NR_GPIOS)
> > > + return -EINVAL;
> > > +
> > > + /* Configure GPIO direction as output. */
> >
> > nits: remove the ending period
> Ok, will do.
> >
> > > + fu540_update_gpio_reg(plat->base + GPIO_OUTPUT_EN, offset, true);
> > > + fu540_update_gpio_reg(plat->base + GPIO_INPUT_EN, offset, false);
> > > +
> > > + /* Set the Output state of the PIN */
> > > + fu540_update_gpio_reg(plat->base + GPIO_OUTPUT_VAL, offset, value);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int fu540_gpio_get_value(struct udevice *dev, u32 offset)
> > > +{
> > > + struct fu540_gpio_platdata *plat = dev_get_platdata(dev);
> > > + int val;
> > > + int dir;
> > > +
> > > + if (offset > NR_GPIOS)
> > > + return -EINVAL;
> > > +
> > > + /* Get direction of the pin OUTPUT=0 INPUT=1 */
> > > + dir = !(readl(plat->base + GPIO_OUTPUT_EN) & BIT(offset));
> > > +
> > > + if (dir)
> > > + val = readl(plat->base + GPIO_INPUT_VAL) & BIT(offset);
> > > + else
> > > + val = readl(plat->base + GPIO_OUTPUT_VAL) & BIT(offset);
> > > +
> > > + return val ? HIGH : LOW;
> > > +}
> > > +
> > > +static int fu540_gpio_set_value(struct udevice *dev, u32 offset, int value)
> > > +{
> > > + struct fu540_gpio_platdata *plat = dev_get_platdata(dev);
> > > +
> > > + if (offset > NR_GPIOS)
> > > + return -EINVAL;
> > > +
> > > + fu540_update_gpio_reg(plat->base + GPIO_OUTPUT_VAL, offset, value);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct udevice_id fu540_gpio_match[] = {
> > > + { .compatible = "sifive,gpio0" },
> > > + { }
> > > +};
> > > +
> > > +static const struct dm_gpio_ops gpio_sifive_ops = {
> > > + .direction_input = fu540_gpio_direction_input,
> > > + .direction_output = fu540_gpio_direction_output,
> > > + .get_value = fu540_gpio_get_value,
> > > + .set_value = fu540_gpio_set_value,
> > > +};
> > > +
> > > +static int fu540_gpio_ofdata_to_platdata(struct udevice *dev)
> > > +{
> > > + struct fu540_gpio_platdata *plat = dev_get_platdata(dev);
> > > + fdt_addr_t addr;
> > > +
> > > + addr = devfdt_get_addr(dev);
> > > + if (addr == FDT_ADDR_T_NONE)
> > > + return -EINVAL;
> > > +
> > > + plat->base = (u8 *)addr;
> > > + return 0;
> > > +}
> > > +
> > > +U_BOOT_DRIVER(gpio_sifive) = {
> > > + .name = "gpio_sifive",
> > > + .id = UCLASS_GPIO,
> > > + .of_match = fu540_gpio_match,
> > > + .ofdata_to_platdata = of_match_ptr(fu540_gpio_ofdata_to_platdata),
> > > + .platdata_auto_alloc_size = sizeof(struct fu540_gpio_platdata),
> > > + .ops = &gpio_sifive_ops,
> > > + .probe = fu540_gpio_probe,
> > > + .priv_auto_alloc_size = sizeof(struct fu540_gpio_platdata),
> >
> > This priv_auto_alloc_size is not used anywhere in this driver.
> >
> Yes, the current implementation used the platform data for accessing
> the gpio registers. I will update the driver to use the devices private space
> allocated by priv_auto_alloc_size
>
Regards,
Bin
More information about the U-Boot
mailing list