[U-Boot,U-BOOT] gpio: fu540: add support for DM based gpio driver for FU540 SoC
diff mbox series

Message ID 1566522098-9254-2-git-send-email-sagar.kadam@sifive.com
State New
Delegated to: Andes
Headers show
Series
  • [U-Boot,U-BOOT] gpio: fu540: add support for DM based gpio driver for FU540 SoC
Related show

Commit Message

Sagar Shrikant Kadam Aug. 23, 2019, 1:01 a.m. UTC
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@sifive.com> and/or Atish Patra
<atish.patra@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@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

Comments

Bin Meng Aug. 23, 2019, 3:12 a.m. UTC | #1
On Fri, Aug 23, 2019 at 9:02 AM Sagar Shrikant Kadam
<sagar.kadam@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@sifive.com> and/or Atish Patra
> <atish.patra@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@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

Pleas split this into two patch:

patch [1/2]: add the gpio driver
patch [2/2]: enable the gpio driver in sifive/fu540

Looks DTS change is missing??

>
> 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?

> +};
> +
> +#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

> +       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

> 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.

> + */
> +
> +#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>?

> +
> +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?

> +       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?

> +{
> +       void __iomem *ptr = (void __iomem *)bptr;
> +
> +       u32 bit = BIT(offset), old = readl(ptr);

nits: don't do this in a single line

> +
> +       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

> +       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

> +       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.

Regards,
Bin
Sagar Shrikant Kadam Aug. 27, 2019, 10:32 p.m. UTC | #2
Hi Bin,

On Thu, Aug 22, 2019 at 8:12 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Aug 23, 2019 at 9:02 AM Sagar Shrikant Kadam
> <sagar.kadam@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@sifive.com> and/or Atish Patra
> > <atish.patra@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@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

> >
> > 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
>
> > +       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?

>
> > +       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

Thanks & BR,
Sagar Kadam

> Regards,
> Bin
Bin Meng Aug. 28, 2019, 2:44 a.m. UTC | #3
Hi Sagar,

On Wed, Aug 28, 2019 at 6:32 AM Sagar Kadam <sagar.kadam@sifive.com> wrote:
>
> Hi Bin,
>
> On Thu, Aug 22, 2019 at 8:12 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Fri, Aug 23, 2019 at 9:02 AM Sagar Shrikant Kadam
> > <sagar.kadam@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@sifive.com> and/or Atish Patra
> > > <atish.patra@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@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

Patch
diff mbox series

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 */
+};
+
+#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
+	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
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.
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <asm/arch/gpio.h>
+#include <asm/io.h>
+#include <errno.h>
+#include <asm-generic/gpio.h>
+
+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;
+	uc_priv->gpio_count = NR_GPIOS;
+
+	return 0;
+}
+
+static void fu540_update_gpio_reg(u8 *bptr, u32 offset, bool value)
+{
+	void __iomem *ptr = (void __iomem *)bptr;
+
+	u32 bit = BIT(offset), old = readl(ptr);
+
+	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. */
+	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. */
+	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),
+};