| Message ID | 20210701002037.912625-3-drew@beagleboard.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: starfive-jh7100: Add StarFive JH7100 GPIO bindings and driver | expand |
On Thu, Jul 1, 2021 at 8:23 AM Drew Fustini <drew@beagleboard.org> wrote: > > Add GPIO driver for the StarFive JH7100 SoC [1] used on the > BeagleV Starlight JH7100 board [2]. > > [1] https://github.com/starfive-tech/beaglev_doc/ > [2] https://github.com/beagleboard/beaglev-starlight > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > Signed-off-by: Huan Feng <huan.feng@starfivetech.com> > Signed-off-by: Drew Fustini <drew@beagleboard.org> > --- > MAINTAINERS | 8 + > drivers/gpio/Kconfig | 8 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-starfive-jh7100.c | 425 ++++++++++++++++++++++++++++ > 4 files changed, 442 insertions(+) > create mode 100644 drivers/gpio/gpio-starfive-jh7100.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index bc0ceef87b73..04fccc2ceffa 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -17423,6 +17423,14 @@ S: Supported > T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git > F: drivers/staging/ > > +SIFVE JH7100 SOC GPIO DRIVER typo of SIFIVE, but it should be STARFIVE > +M: Drew Fustini <drew@beagleboard.org> > +M: Huan Feng <huan.feng@starfivetech.com> > +L: linux-riscv@lists.infradead.org > +L: linux-gpio@vger.kernel.org > +F: Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml > +F: drivers/gpio/gpio-starfive-jh7100.c > + [snip] Regards, Bin
Hi Drew, Am 2021-07-01 02:20, schrieb Drew Fustini: > Add GPIO driver for the StarFive JH7100 SoC [1] used on the > BeagleV Starlight JH7100 board [2]. > > [1] https://github.com/starfive-tech/beaglev_doc/ > [2] https://github.com/beagleboard/beaglev-starlight > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > Signed-off-by: Huan Feng <huan.feng@starfivetech.com> > Signed-off-by: Drew Fustini <drew@beagleboard.org> Could this driver use GPIO_REGMAP and REGMAP_IRQ? See drivers/gpio/gpio-sl28cpld.c for an example. -michael
On Thu, Jul 01, 2021 at 08:39:40AM +0200, Michael Walle wrote: > Hi Drew, > > Am 2021-07-01 02:20, schrieb Drew Fustini: > > Add GPIO driver for the StarFive JH7100 SoC [1] used on the > > BeagleV Starlight JH7100 board [2]. > > > > [1] https://github.com/starfive-tech/beaglev_doc/ > > [2] https://github.com/beagleboard/beaglev-starlight > > > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > > Signed-off-by: Huan Feng <huan.feng@starfivetech.com> > > Signed-off-by: Drew Fustini <drew@beagleboard.org> > > Could this driver use GPIO_REGMAP and REGMAP_IRQ? See > drivers/gpio/gpio-sl28cpld.c for an example. > > -michael Thank you for the suggestion. I am not familiar with GPIO_REGMAP and REGMAP_IRQ so I will read about it. Is the advantage is that is helps to reduce code duplication by using an abstraction? I did notice that the gpio-sifive.c driver used regmap_update_bits() and regmap_write(). I suppose that is better than writel_relaxed() and iowrite32() which this RFC driver does? thanks, drew
On Thu, Jul 01, 2021 at 10:25:12AM +0800, Bin Meng wrote: > On Thu, Jul 1, 2021 at 8:23 AM Drew Fustini <drew@beagleboard.org> wrote: > > > > Add GPIO driver for the StarFive JH7100 SoC [1] used on the > > BeagleV Starlight JH7100 board [2]. > > > > [1] https://github.com/starfive-tech/beaglev_doc/ > > [2] https://github.com/beagleboard/beaglev-starlight > > > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > > Signed-off-by: Huan Feng <huan.feng@starfivetech.com> > > Signed-off-by: Drew Fustini <drew@beagleboard.org> > > --- > > MAINTAINERS | 8 + > > drivers/gpio/Kconfig | 8 + > > drivers/gpio/Makefile | 1 + > > drivers/gpio/gpio-starfive-jh7100.c | 425 ++++++++++++++++++++++++++++ > > 4 files changed, 442 insertions(+) > > create mode 100644 drivers/gpio/gpio-starfive-jh7100.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index bc0ceef87b73..04fccc2ceffa 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -17423,6 +17423,14 @@ S: Supported > > T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git > > F: drivers/staging/ > > > > +SIFVE JH7100 SOC GPIO DRIVER > > typo of SIFIVE, but it should be STARFIVE Thank you! My eyes should have caught that. -Drew
Hi Drew, Am 2021-07-01 22:33, schrieb Drew Fustini: > On Thu, Jul 01, 2021 at 08:39:40AM +0200, Michael Walle wrote: >> Hi Drew, >> >> Am 2021-07-01 02:20, schrieb Drew Fustini: >> > Add GPIO driver for the StarFive JH7100 SoC [1] used on the >> > BeagleV Starlight JH7100 board [2]. >> > >> > [1] https://github.com/starfive-tech/beaglev_doc/ >> > [2] https://github.com/beagleboard/beaglev-starlight >> > >> > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> >> > Signed-off-by: Huan Feng <huan.feng@starfivetech.com> >> > Signed-off-by: Drew Fustini <drew@beagleboard.org> >> >> Could this driver use GPIO_REGMAP and REGMAP_IRQ? See >> drivers/gpio/gpio-sl28cpld.c for an example. >> >> -michael > > Thank you for the suggestion. I am not familiar with GPIO_REGMAP and > REGMAP_IRQ so I will read about it. Is the advantage is that is helps > to reduce code duplication by using an abstraction? Yes, I've looked briefly at your patch and it seemed that GPIO_REGMAP might fit here which will reduce code. > I did notice that the gpio-sifive.c driver used regmap_update_bits() > and > regmap_write(). > > I suppose that is better than writel_relaxed() and iowrite32() which > this RFC driver does? Its just another abstraction layer in between. For MMIO it will also end up using some variant of the above (see regmap-mmio.c). But if you use regmap, you can also use REGMAP_IRQ which might also be a fit for your GPIO controller and thus don't have to implement your own versions for the irq_chip ops. -michael
On Thu, Jul 01, 2021 at 08:39:40AM +0200, Michael Walle wrote: > Hi Drew, > > Am 2021-07-01 02:20, schrieb Drew Fustini: > > Add GPIO driver for the StarFive JH7100 SoC [1] used on the > > BeagleV Starlight JH7100 board [2]. > > > > [1] https://github.com/starfive-tech/beaglev_doc/ > > [2] https://github.com/beagleboard/beaglev-starlight > > > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > > Signed-off-by: Huan Feng <huan.feng@starfivetech.com> > > Signed-off-by: Drew Fustini <drew@beagleboard.org> > > Could this driver use GPIO_REGMAP and REGMAP_IRQ? See > drivers/gpio/gpio-sl28cpld.c for an example. > > -michael I looked more at the example. Do you have a suggestion of how to handle different types of interrupts? This gpio controller can handle level triggered and edge triggered. Edge triggered can be positve, negative or both. Level trigger can be high or low. Thanks, Drew
Hi Drew, Am 2021-07-02 23:06, schrieb Drew Fustini: > On Fri, Jul 02, 2021 at 07:03:19PM +0300, Andy Shevchenko wrote: >> On Thu, Jul 1, 2021 at 3:23 AM Drew Fustini <drew@beagleboard.org> >> wrote: >> > >> > Add GPIO driver for the StarFive JH7100 SoC [1] used on the >> > BeagleV Starlight JH7100 board [2]. >> > >> > [1] https://github.com/starfive-tech/beaglev_doc/ >> > [2] https://github.com/beagleboard/beaglev-starlight >> >> > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> >> > Signed-off-by: Huan Feng <huan.feng@starfivetech.com> >> > Signed-off-by: Drew Fustini <drew@beagleboard.org> >> >> Seems some Co-developed-by are missing. > > Thank you for suggesting this. Huan Feng originally wrote the driver. > Emil and I have made some changes to reorganize and clean it up for > submission. > > Do you think all three of us should list Co-developed-by: for our names > in addition to the SOB? > >> Brief look into the code brings the Q. Can't you utilize gpio-regmap >> here? Why not? > > Michael Walle asked about this yesterday and it was my first time > looking at regmap and gpio-regmap. I've been reading the code and it > does look like I should try convert this driver over to using > gpio-regmap. > > The open question in my mind is how to handle the interrupt type (edge > trigged on positive or negative, level triggered on high or low). > Hopefully I can find some other examples that can help me think about > how to do that correctly. Have a look at include/linux/regmap.h, there is "struct regmap_irq_type". If you're lucky, you can just supply the corresponding values that fits your hardware. If it doesn't match your hardware at all, then you can keep your own functions, or if its slightly different, then maybe you can add support for your quirk in regmap-irq. You don't necessarily have to use regmap-irq together with gpio-regmap. You can also just use regmap-irq or gpio-regmap independently. A quick grep for "type_rising_" lists drivers/mfd/max77650.c and drivers/mfd/rohm-bd70528.c for example. -michael
Hi deee Ho Drew, Michael, All On Mon, 2021-07-05 at 15:29 +0200, Michael Walle wrote: > Hi Drew, > > Am 2021-07-02 23:06, schrieb Drew Fustini: > > On Fri, Jul 02, 2021 at 07:03:19PM +0300, Andy Shevchenko wrote: > > > On Thu, Jul 1, 2021 at 3:23 AM Drew Fustini <drew@beagleboard.org > > > > > > > wrote: > > > > Add GPIO driver for the StarFive JH7100 SoC [1] used on the > > > > BeagleV Starlight JH7100 board [2]. > > > > > > > > [1] https://github.com/starfive-tech/beaglev_doc/ > > > > [2] https://github.com/beagleboard/beaglev-starlight > > > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > > > > Signed-off-by: Huan Feng <huan.feng@starfivetech.com> > > > > Signed-off-by: Drew Fustini <drew@beagleboard.org> > > > > > > Seems some Co-developed-by are missing. > > > > Thank you for suggesting this. Huan Feng originally wrote the > > driver. > > Emil and I have made some changes to reorganize and clean it up for > > submission. > > > > Do you think all three of us should list Co-developed-by: for our > > names > > in addition to the SOB? > > > > > Brief look into the code brings the Q. Can't you utilize gpio- > > > regmap > > > here? Why not? > > > > Michael Walle asked about this yesterday and it was my first time > > looking at regmap and gpio-regmap. I've been reading the code and > > it > > does look like I should try convert this driver over to using > > gpio-regmap. > > > > The open question in my mind is how to handle the interrupt type > > (edge > > trigged on positive or negative, level triggered on high or low). > > Hopefully I can find some other examples that can help me think > > about > > how to do that correctly. > regmap_irq_type". > If you're lucky, you can just supply the corresponding values that > fits > your hardware. I added some level IRQ type-configuration support to regmap_irq back when I wrote the BD70528 support. You should be able to just fill the bit-mask indicating IRQ types supported by your GPIO controller hardware, and then the corresponding type register values. As far as I remember the supported types and values are given "per IRQ". If my memory serves me right there was a limitation that the regmap-IRQ does not distinguish setup where GPIO controller supports rising and falling edges - but not both. That would have required adding another type flag. > If it doesn't match your hardware at all, then you can > keep your own functions, or if its slightly different, then maybe you > can add support for your quirk in regmap-irq. You don't necessarily > have > to use regmap-irq together with gpio-regmap. You can also just use > regmap-irq or gpio-regmap independently. > > A quick grep for "type_rising_" lists drivers/mfd/max77650.c and > drivers/mfd/rohm-bd70528.c for example. The BD70528 has not been used too much and is scheduled for removal. It may have received only limited testing but it *should* be functional though. Best Regards Matti Vaittinen
Hi Drew On Thu, Jul 1, 2021 at 8:25 AM Drew Fustini <drew@beagleboard.org> wrote: > > Add GPIO driver for the StarFive JH7100 SoC [1] used on the > BeagleV Starlight JH7100 board [2]. > > [1] https://github.com/starfive-tech/beaglev_doc/ > [2] https://github.com/beagleboard/beaglev-starlight > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > Signed-off-by: Huan Feng <huan.feng@starfivetech.com> > Signed-off-by: Drew Fustini <drew@beagleboard.org> > --- > MAINTAINERS | 8 + > drivers/gpio/Kconfig | 8 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-starfive-jh7100.c | 425 ++++++++++++++++++++++++++++ > 4 files changed, 442 insertions(+) > create mode 100644 drivers/gpio/gpio-starfive-jh7100.c [...] > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > index d7c81e1611a4..939922eaf5f3 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -132,6 +132,7 @@ obj-$(CONFIG_GPIO_SAMA5D2_PIOBU) += gpio-sama5d2-piobu.o > obj-$(CONFIG_GPIO_SCH311X) += gpio-sch311x.o > obj-$(CONFIG_GPIO_SCH) += gpio-sch.o > obj-$(CONFIG_GPIO_SIFIVE) += gpio-sifive.o > +obj-$(CONFIG_GPIO_STARFIVE_JH7100) += gpio-starfive-jh7100.o Sort in alphabetical order. > obj-$(CONFIG_GPIO_SIOX) += gpio-siox.o > obj-$(CONFIG_GPIO_SL28CPLD) += gpio-sl28cpld.o > obj-$(CONFIG_GPIO_SODAVILLE) += gpio-sodaville.o > diff --git a/drivers/gpio/gpio-starfive-jh7100.c b/drivers/gpio/gpio-starfive-jh7100.c > new file mode 100644 > index 000000000000..b94ebfe9eaf7 > --- /dev/null > +++ b/drivers/gpio/gpio-starfive-jh7100.c > @@ -0,0 +1,425 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * GPIO driver for StarFive JH7100 SoC > + * > + * Copyright (C) 2020 Shanghai StarFive Technology Co., Ltd. > + */ > + > +#include <linux/module.h> > +#include <linux/gpio/driver.h> > +#include <linux/platform_device.h> > +#include <linux/spinlock.h> Include files sort in alphabetical orde too. > + > +/* > + * refer to Section 12. GPIO Registers in JH7100 datasheet: > + * https://github.com/starfive-tech/beaglev_doc > + */ > + > +/* global enable */ > +#define GPIO_EN 0x0 > + > +/* interrupt type */ > +#define GPIO_IS_LOW 0x10 > +#define GPIO_IS_HIGH 0x14 > + > +/* edge trigger interrupt type */ > +#define GPIO_IBE_LOW 0x18 > +#define GPIO_IBE_HIGH 0x1c > + > +/* edge trigger interrupt polarity */ > +#define GPIO_IEV_LOW 0x20 > +#define GPIO_IEV_HIGH 0x24 > + > +/* interrupt max */ > +#define GPIO_IE_LOW 0x28 > +#define GPIO_IE_HIGH 0x2c > + > +/* clear edge-triggered interrupt */ > +#define GPIO_IC_LOW 0x30 > +#define GPIO_IC_HIGH 0x34 > + > +/* edge-triggered interrupt status (read-only) */ > +#define GPIO_RIS_LOW 0x38 > +#define GPIO_RIS_HIGH 0x3c > + > +/* interrupt status after masking (read-only) */ > +#define GPIO_MIS_LOW 0x40 > +#define GPIO_MIS_HIGH 0x44 > + > +/* data value of gpio */ > +#define GPIO_DIN_LOW 0x48 > +#define GPIO_DIN_HIGH 0x4c > + > +/* GPIO0_DOUT_CFG is 0x50, GPIOn_DOUT_CFG is 0x50+(n*8) */ > +#define GPIO_DOUT_X_REG 0x50 > + > +/* GPIO0_DOEN_CFG is 0x54, GPIOn_DOEN_CFG is 0x54+(n*8) */ > +#define GPIO_DOEN_X_REG 0x54 > + > +#define MAX_GPIO 64 > + > +struct starfive_gpio { > + raw_spinlock_t lock; > + void __iomem *base; > + struct gpio_chip gc; > + unsigned long enabled; > + unsigned int trigger[MAX_GPIO]; > + unsigned int irq_parent[MAX_GPIO]; > +}; > + > +static int starfive_direction_input(struct gpio_chip *gc, unsigned int offset) > +{ > + struct starfive_gpio *chip = gpiochip_get_data(gc); > + unsigned long flags; > + > + if (offset >= gc->ngpio) > + return -EINVAL; > + > + raw_spin_lock_irqsave(&chip->lock, flags); > + writel_relaxed(0x1, chip->base + GPIO_DOEN_X_REG + offset * 8); > + raw_spin_unlock_irqrestore(&chip->lock, flags); > + > + return 0; > +} > + > +static int starfive_direction_output(struct gpio_chip *gc, unsigned int offset, int value) > +{ > + struct starfive_gpio *chip = gpiochip_get_data(gc); > + unsigned long flags; > + > + if (offset >= gc->ngpio) > + return -EINVAL; > + > + raw_spin_lock_irqsave(&chip->lock, flags); > + writel_relaxed(0x0, chip->base + GPIO_DOEN_X_REG + offset * 8); > + writel_relaxed(value, chip->base + GPIO_DOUT_X_REG + offset * 8); > + raw_spin_unlock_irqrestore(&chip->lock, flags); > + > + return 0; > +} > + > +static int starfive_get_direction(struct gpio_chip *gc, unsigned int offset) > +{ > + struct starfive_gpio *chip = gpiochip_get_data(gc); > + > + if (offset >= gc->ngpio) > + return -EINVAL; > + > + return readl_relaxed(chip->base + GPIO_DOEN_X_REG + offset * 8) & 0x1; > +} > + > +static int starfive_get_value(struct gpio_chip *gc, unsigned int offset) > +{ > + struct starfive_gpio *chip = gpiochip_get_data(gc); > + int value; > + > + if (offset >= gc->ngpio) > + return -EINVAL; > + > + if (offset < 32) { > + value = readl_relaxed(chip->base + GPIO_DIN_LOW); > + value = (value >> offset) & 0x1; > + } else { > + value = readl_relaxed(chip->base + GPIO_DIN_HIGH); > + value = (value >> (offset - 32)) & 0x1; > + } > + > + return value; > +} > + > +static void starfive_set_value(struct gpio_chip *gc, unsigned int offset, int value) > +{ > + struct starfive_gpio *chip = gpiochip_get_data(gc); > + unsigned long flags; > + > + if (offset >= gc->ngpio) > + return; > + > + raw_spin_lock_irqsave(&chip->lock, flags); > + writel_relaxed(value, chip->base + GPIO_DOUT_X_REG + offset * 8); > + raw_spin_unlock_irqrestore(&chip->lock, flags); > +} > + > +static void starfive_set_ie(struct starfive_gpio *chip, int offset) > +{ > + unsigned long flags; > + int old_value, new_value; > + int reg_offset, index; > + > + if (offset < 32) { > + reg_offset = 0; > + index = offset; > + } else { > + reg_offset = 4; > + index = offset - 32; > + } Quite a number of places do this checking/calculation, can move this to a helper function. > + raw_spin_lock_irqsave(&chip->lock, flags); > + old_value = readl_relaxed(chip->base + GPIO_IE_LOW + reg_offset); > + new_value = old_value | (1 << index); > + writel_relaxed(new_value, chip->base + GPIO_IE_LOW + reg_offset); > + raw_spin_unlock_irqrestore(&chip->lock, flags); > +} > + > +static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct starfive_gpio *chip = gpiochip_get_data(gc); > + int offset = irqd_to_hwirq(d); > + unsigned int reg_is, reg_ibe, reg_iev; > + int reg_offset, index; > + > + if (offset < 0 || offset >= gc->ngpio) > + return -EINVAL; > + > + if (offset < 32) { > + reg_offset = 0; > + index = offset; > + } else { > + reg_offset = 4; > + index = offset - 32; > + } > + > + reg_is = readl_relaxed(chip->base + GPIO_IS_LOW + reg_offset); > + reg_ibe = readl_relaxed(chip->base + GPIO_IBE_LOW + reg_offset); > + reg_iev = readl_relaxed(chip->base + GPIO_IEV_LOW + reg_offset); > + > + switch (trigger) { > + case IRQ_TYPE_LEVEL_HIGH: > + reg_is &= ~(1 << index); > + reg_ibe &= ~(1 << index); > + reg_iev |= (1 << index); > + break; > + case IRQ_TYPE_LEVEL_LOW: > + reg_is &= ~(1 << index); > + reg_ibe &= ~(1 << index); > + reg_iev &= (1 << index); > + break; > + case IRQ_TYPE_EDGE_BOTH: > + reg_is |= ~(1 << index); > + reg_ibe |= ~(1 << index); > + // no need to set edge type when both Use /**/ comment style. > + break; > + case IRQ_TYPE_EDGE_RISING: > + reg_is |= ~(1 << index); > + reg_ibe &= ~(1 << index); > + reg_iev |= (1 << index); > + break; > + case IRQ_TYPE_EDGE_FALLING: > + reg_is |= ~(1 << index); > + reg_ibe &= ~(1 << index); > + reg_iev &= (1 << index); > + break; > + } > + > + writel_relaxed(reg_is, chip->base + GPIO_IS_LOW + reg_offset); > + writel_relaxed(reg_ibe, chip->base + GPIO_IBE_LOW + reg_offset); > + writel_relaxed(reg_iev, chip->base + GPIO_IEV_LOW + reg_offset); > + chip->trigger[offset] = trigger; > + starfive_set_ie(chip, offset); > + return 0; > +} > + > +/* chained_irq_{enter,exit} already mask the parent */ > +static void starfive_irq_mask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct starfive_gpio *chip = gpiochip_get_data(gc); > + unsigned int value; > + int offset = irqd_to_hwirq(d); > + int reg_offset, index; > + > + if (offset < 0 || offset >= gc->ngpio) > + return; > + > + if (offset < 32) { > + reg_offset = 0; > + index = offset; > + } else { > + reg_offset = 4; > + index = offset - 32; > + } > + > + value = readl_relaxed(chip->base + GPIO_IE_LOW + reg_offset); > + value &= ~(0x1 << index); > + writel_relaxed(value, chip->base + GPIO_IE_LOW + reg_offset); > +} > + > +static void starfive_irq_unmask(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct starfive_gpio *chip = gpiochip_get_data(gc); > + unsigned int value; > + int offset = irqd_to_hwirq(d); > + int reg_offset, index; > + > + if (offset < 0 || offset >= gc->ngpio) > + return; > + > + if (offset < 32) { > + reg_offset = 0; > + index = offset; > + } else { > + reg_offset = 4; > + index = offset - 32; > + } > + > + value = readl_relaxed(chip->base + GPIO_IE_LOW + reg_offset); > + value |= (0x1 << index); > + writel_relaxed(value, chip->base + GPIO_IE_LOW + reg_offset); > +} > + > +static void starfive_irq_enable(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct starfive_gpio *chip = gpiochip_get_data(gc); > + int offset = irqd_to_hwirq(d); > + > + starfive_irq_unmask(d); > + assign_bit(offset, &chip->enabled, 1); > +} > + > +static void starfive_irq_disable(struct irq_data *d) > +{ > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > + struct starfive_gpio *chip = gpiochip_get_data(gc); > + int offset = irqd_to_hwirq(d) % MAX_GPIO; // must not fail > + > + assign_bit(offset, &chip->enabled, 0); > + starfive_set_ie(chip, offset); > +} > + > +static struct irq_chip starfive_irqchip = { > + .name = "starfive-jh7100-gpio", > + .irq_set_type = starfive_irq_set_type, > + .irq_mask = starfive_irq_mask, > + .irq_unmask = starfive_irq_unmask, > + .irq_enable = starfive_irq_enable, > + .irq_disable = starfive_irq_disable, > +}; > + > +static irqreturn_t starfive_irq_handler(int irq, void *gc) > +{ > + int offset; > + int reg_offset, index; > + unsigned int value; > + unsigned long flags; > + struct starfive_gpio *chip = gc; > + > + for (offset = 0; offset < MAX_GPIO; offset++) { > + if (offset < 32) { > + reg_offset = 0; > + index = offset; > + } else { > + reg_offset = 4; > + index = offset - 32; > + } > + > + raw_spin_lock_irqsave(&chip->lock, flags); > + value = readl_relaxed(chip->base + GPIO_MIS_LOW + reg_offset); > + if (value & BIT(index)) > + writel_relaxed(BIT(index), chip->base + GPIO_IC_LOW + > + reg_offset); > + raw_spin_unlock_irqrestore(&chip->lock, flags); > + } > + > + return IRQ_HANDLED; > +} > + > +static int starfive_gpio_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct starfive_gpio *chip; > + struct gpio_irq_chip *girq; > + struct resource *res; > + int irq, ret, ngpio; > + > + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + chip->base = devm_ioremap_resource(dev, res); Can use device managed function devm_ devm_platform_ioremap_resource(), then combile these 2 functions into 1. > + if (IS_ERR(chip->base)) { > + dev_err(dev, "failed to allocate device memory\n"); Perhaps change "allocate" to get or ioremap. > + return PTR_ERR(chip->base); > + } > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(dev, "Cannot get IRQ resource\n"); > + return irq; > + } > + > + raw_spin_lock_init(&chip->lock); > + chip->gc.direction_input = starfive_direction_input; > + chip->gc.direction_output = starfive_direction_output; > + chip->gc.get_direction = starfive_get_direction; > + chip->gc.get = starfive_get_value; > + chip->gc.set = starfive_set_value; > + chip->gc.base = 0; > + chip->gc.ngpio = MAX_GPIO; > + chip->gc.label = dev_name(dev); > + chip->gc.parent = dev; > + chip->gc.owner = THIS_MODULE; > + > + girq = &chip->gc.irq; > + girq->chip = &starfive_irqchip; > + girq->parent_handler = NULL; > + girq->num_parents = 0; > + girq->parents = NULL; > + girq->default_type = IRQ_TYPE_NONE; > + girq->handler = handle_simple_irq; > + > + ret = gpiochip_add_data(&chip->gc, chip); Use devm_version, devm_gpiochip_add_data(). > + if (ret) { > + dev_err(dev, "gpiochip_add_data ret=%d!\n", ret); > + return ret; > + } > + > + /* Disable all GPIO interrupts before enabling parent interrupts */ Clear any pending interrupts as well when initialization. > + iowrite32(0, chip->base + GPIO_IE_HIGH); > + iowrite32(0, chip->base + GPIO_IE_LOW); > + chip->enabled = 0; > + > + ret = devm_request_irq(dev, irq, starfive_irq_handler, IRQF_SHARED, > + dev_name(dev), chip); > + if (ret) { > + dev_err(dev, "IRQ handler registering failed (%d)\n", ret); > + return ret; > + } > + > + writel_relaxed(1, chip->base + GPIO_EN); > + > + dev_info(dev, "StarFive GPIO chip registered %d GPIOs\n", ngpio); > + > + return 0; > +} > + > +static const struct of_device_id starfive_gpio_match[] = { > + { .compatible = "starfive,jh7100-gpio", }, > + { }, > +}; > + > +static struct platform_driver starfive_gpio_driver = { > + .probe = starfive_gpio_probe, > + .driver = { > + .name = "gpio_starfive_jh7100", > + .of_match_table = of_match_ptr(starfive_gpio_match), > + }, > +}; > + > +static int __init starfive_gpio_init(void) > +{ > + return platform_driver_register(&starfive_gpio_driver); > +} > +subsys_initcall(starfive_gpio_init); > + > +static void __exit starfive_gpio_exit(void) > +{ > + platform_driver_unregister(&starfive_gpio_driver); Do you expect GPIO driver can be removed? The driver needs proper removal, provides .remove callback. Example, call gpiochip_remove() , disable interrupt when removing. > +} > +module_exit(starfive_gpio_exit); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Huan Feng <huan.feng@starfivetech.com>"); > +MODULE_DESCRIPTION("StarFive JH7100 GPIO driver"); > -- Regards Ley Foon
On Fri, Jul 23, 2021 at 11:04:41PM +0200, Linus Walleij wrote: > On Thu, Jul 1, 2021 at 8:39 AM Michael Walle <michael@walle.cc> wrote: > > Am 2021-07-01 02:20, schrieb Drew Fustini: > > > Add GPIO driver for the StarFive JH7100 SoC [1] used on the > > > BeagleV Starlight JH7100 board [2]. > > > > > > [1] https://github.com/starfive-tech/beaglev_doc/ > > > [2] https://github.com/beagleboard/beaglev-starlight > > > > > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > > > Signed-off-by: Huan Feng <huan.feng@starfivetech.com> > > > Signed-off-by: Drew Fustini <drew@beagleboard.org> > > > > Could this driver use GPIO_REGMAP and REGMAP_IRQ? See > > drivers/gpio/gpio-sl28cpld.c for an example. > > To me it looks just memory-mapped? > > Good old gpio-mmio.c (select GPIO_GENERIC) should > suffice I think. > > Drew please look at drivers/gpio/gpio-ftgpio010.c for an example > of GPIO_GENERIC calling bgpio_init() in probe(). Thank you for the suggestion. However, I am not sure that will work for this SoC. The GPIO registers are described in section 12 of JH7100 datasheet [1] and I don't think they fit the expectation of gpio-mmio.c because there is a seperate register for each GPIO line for output data value and output enable. There are 64 output data config registers which are 4 bytes wide. There are 64 output enable config registers which are 4 bytes wide too. Output data and output enable registers for a given GPIO pad are contiguous. GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while GPIO1_DOUT_CFG is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO pads is effectively 8, which yields the formula: GPIOn_DOUT_CFG is 0x50+8n. Similarly, GPIO0_DOEN_CFG is 0x54 and thus GPIOn_DOEN_CFG is 0x54+8n. However, GPIO input data does use just one bit for each line. GPIODIN_0 at 0x48 covers GPIO[31:0] and GPIODIN_1 at 0x4c covers GPIO[63:32]. Thus the input could work with gpio-mmio but I am not sure how to reconcile the register-per-gpio for the output value and output enable. Is there way a way to adapt gpio-mmio for this situation? Thanks, Drew [1] https://github.com/starfive-tech/beaglev_doc
Hi Drew, Hi Linus, Am 2021-07-26 09:11, schrieb Drew Fustini: > On Fri, Jul 23, 2021 at 11:04:41PM +0200, Linus Walleij wrote: >> On Thu, Jul 1, 2021 at 8:39 AM Michael Walle <michael@walle.cc> wrote: >> > Am 2021-07-01 02:20, schrieb Drew Fustini: >> > > Add GPIO driver for the StarFive JH7100 SoC [1] used on the >> > > BeagleV Starlight JH7100 board [2]. >> > > >> > > [1] https://github.com/starfive-tech/beaglev_doc/ >> > > [2] https://github.com/beagleboard/beaglev-starlight >> > > >> > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> >> > > Signed-off-by: Huan Feng <huan.feng@starfivetech.com> >> > > Signed-off-by: Drew Fustini <drew@beagleboard.org> >> > >> > Could this driver use GPIO_REGMAP and REGMAP_IRQ? See >> > drivers/gpio/gpio-sl28cpld.c for an example. >> >> To me it looks just memory-mapped? >> >> Good old gpio-mmio.c (select GPIO_GENERIC) should >> suffice I think. But that doesn't mean gpio-regmap can't be used, no? Or what are the advantages of gpio-mmio? >> Drew please look at drivers/gpio/gpio-ftgpio010.c for an example >> of GPIO_GENERIC calling bgpio_init() in probe(). > > Thank you for the suggestion. However, I am not sure that will work for > this SoC. > > The GPIO registers are described in section 12 of JH7100 datasheet [1] > and I don't think they fit the expectation of gpio-mmio.c because there > is a seperate register for each GPIO line for output data value and > output enable. > > There are 64 output data config registers which are 4 bytes wide. There > are 64 output enable config registers which are 4 bytes wide too. > Output > data and output enable registers for a given GPIO pad are contiguous. > GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while GPIO1_DOUT_CFG > is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO pads is > effectively 8, which yields the formula: GPIOn_DOUT_CFG is 0x50+8n. > Similarly, GPIO0_DOEN_CFG is 0x54 and thus GPIOn_DOEN_CFG is 0x54+8n. > > However, GPIO input data does use just one bit for each line. GPIODIN_0 > at 0x48 covers GPIO[31:0] and GPIODIN_1 at 0x4c covers GPIO[63:32]. I'd say, that should work with the .reg_mask_xlate of the gpio-regmap. -michael > Thus the input could work with gpio-mmio but I am not sure how to > reconcile the register-per-gpio for the output value and output enable. > > Is there way a way to adapt gpio-mmio for this situation? > > Thanks, > Drew > > [1] https://github.com/starfive-tech/beaglev_doc
On Mon, Jul 26, 2021 at 09:21:31AM +0200, Michael Walle wrote: > Hi Drew, Hi Linus, > > Am 2021-07-26 09:11, schrieb Drew Fustini: > > On Fri, Jul 23, 2021 at 11:04:41PM +0200, Linus Walleij wrote: > > > On Thu, Jul 1, 2021 at 8:39 AM Michael Walle <michael@walle.cc> wrote: > > > > Am 2021-07-01 02:20, schrieb Drew Fustini: > > > > > Add GPIO driver for the StarFive JH7100 SoC [1] used on the > > > > > BeagleV Starlight JH7100 board [2]. > > > > > > > > > > [1] https://github.com/starfive-tech/beaglev_doc/ > > > > > [2] https://github.com/beagleboard/beaglev-starlight > > > > > > > > > > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > > > > > Signed-off-by: Huan Feng <huan.feng@starfivetech.com> > > > > > Signed-off-by: Drew Fustini <drew@beagleboard.org> > > > > > > > > Could this driver use GPIO_REGMAP and REGMAP_IRQ? See > > > > drivers/gpio/gpio-sl28cpld.c for an example. > > > > > > To me it looks just memory-mapped? > > > > > > Good old gpio-mmio.c (select GPIO_GENERIC) should > > > suffice I think. > > But that doesn't mean gpio-regmap can't be used, no? Or what are > the advantages of gpio-mmio? > > > > Drew please look at drivers/gpio/gpio-ftgpio010.c for an example > > > of GPIO_GENERIC calling bgpio_init() in probe(). > > > > Thank you for the suggestion. However, I am not sure that will work for > > this SoC. > > > > The GPIO registers are described in section 12 of JH7100 datasheet [1] > > and I don't think they fit the expectation of gpio-mmio.c because there > > is a seperate register for each GPIO line for output data value and > > output enable. > > > > There are 64 output data config registers which are 4 bytes wide. There > > are 64 output enable config registers which are 4 bytes wide too. Output > > data and output enable registers for a given GPIO pad are contiguous. > > GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while GPIO1_DOUT_CFG > > is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO pads is > > effectively 8, which yields the formula: GPIOn_DOUT_CFG is 0x50+8n. > > Similarly, GPIO0_DOEN_CFG is 0x54 and thus GPIOn_DOEN_CFG is 0x54+8n. > > > > However, GPIO input data does use just one bit for each line. GPIODIN_0 > > at 0x48 covers GPIO[31:0] and GPIODIN_1 at 0x4c covers GPIO[63:32]. > > I'd say, that should work with the .reg_mask_xlate of the gpio-regmap. > > -michael Thanks, yes, I think trying to figure out how .reg_mask_xlate would need to work this SoC. I believe these are the only two implementations. From drivers/gpio/gpio-regmap.c: static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int base, unsigned int offset, unsigned int *reg, unsigned int *mask) { unsigned int line = offset % gpio->ngpio_per_reg; unsigned int stride = offset / gpio->ngpio_per_reg; *reg = base + stride * gpio->reg_stride; *mask = BIT(line); return 0; } From drivers/pinctrl/bcm/pinctrl-bcm63xx.c: static int bcm63xx_reg_mask_xlate(struct gpio_regmap *gpio, unsigned int base, unsigned int offset, unsigned int *reg, unsigned int *mask) { unsigned int line = offset % BCM63XX_BANK_GPIOS; unsigned int stride = offset / BCM63XX_BANK_GPIOS; *reg = base - stride * BCM63XX_BANK_SIZE; *mask = BIT(line); return 0; } Let's say a driver calls gpio_regmap_set(chip, 0, 5) to set line 5 to value 1. I believe this would result in call to: gpio->reg_mask_xlate(gpio, gpio->reg_set_base, 5, ®, &mask) Then this would be called to set the register: regmap_update_bits(gpio->regmap, reg, mask, mask); From datasheet section 12 [1], there are 64 output data registers which are 4 bytes wide. There are 64 output enable registers which are also 4 bytes wide too. Output data and output enable registers for a GPIO line are contiguous. Thus GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54. The forumla is GPIOn_DOUT_CFG is 0x50+8n and GPIOn_DOEN_CFG is 0x54+8n. Thus for GPIO line 5: GPIO5_DOUT_CFG is 0x50 + 0x28 = 0x78 GPIO5_DOEN_CFG is 0x54 + 0x28 = 0x7C Enable GPIO line 5 as output by writing 0x1 to 0x7C and set output value to 1 by writing 1 to 0x7C. Using gpio_regmap_simple_xlate() as a template, I am thinking through xlate for this gpio controller: static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio, unsigned int base, unsigned int offset, unsigned int *reg, unsigned int *mask) { // reg_set_base is passed as base // let reg_set_base = 0x50 (GPIO0_DOUT_CFG) // let gpio->reg_stride = 8 // let offest = 5 (for gpio line 5) *reg = base + offset * gpio->reg_stride; // *reg = base:0x50 + offset:0x5 * reg_stride:0x8 // *reg = 0x50 + 0x28 // *reg= 0x78 // Each gpio line has a full register, not just a bit. To output // a digital 1, then GPIO5_DOUT_CFG would be 0x1. To output // digital 0, GPIO5_DOUT_CFG would be 0x0. Thus I think the mask // should be the least significant bit. *mask = BIT(1); return 0; } Let's walk through what would happen if gpio_regmap_set() was the caller: static void gpio_regmap_set(struct gpio_chip *chip, unsigned int offset, int val) { // for gpio line, offset = 5 // if want to set line 5 high, then val = 1 struct gpio_regmap *gpio = gpiochip_get_data(chip); // reg_set_base would be set to 0x50 (GPIO0_DOUT_CFG) unsigned int base = gpio_regmap_addr(gpio->reg_set_base); unsigned int reg, mask; gpio->reg_mask_xlate(gpio, base /* 0x50 */, offset /* 5 */, ®, &mask); if (val) /* if val is 1 */ regmap_update_bits(gpio->regmap, reg, mask, mask); // if mask returned was 0x1, then this would set the // bit 0 in GPIO5_DOUT_CFG else /* if val is 0 */ regmap_update_bits(gpio->regmap, reg, mask, 0); // if mask returned was 0x1, then this would clear // bit 0 in GPIO5_DOUT_CFG } Now for the output enable register GPIO5_DOEN_CFG, the output driver is active low so 0x0 is actually enables output where as 0x1 disables output. Thus maybe I need to add logic like: static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio, unsigned int base, unsigned int offset, unsigned int *reg, unsigned int *mask) { <snip> if (base == GPIO0_DOUT_CFG) *mask = 0x1U; else if (base == GPIO0_DOEN_CFG) *bit = ~(0x1U); return 0; } What do you think of that approach? Are there any other examples of regmap xlate that I missed? Thanks, Drew [1] https://github.com/starfive-tech/beaglev_doc/blob/main/JH7100%20Data%20Sheet%20V01.01.04-EN%20(4-21-2021).pdf
Hi Drew, Am 2021-07-27 07:28, schrieb Drew Fustini: [..] >> > > Drew please look at drivers/gpio/gpio-ftgpio010.c for an example >> > > of GPIO_GENERIC calling bgpio_init() in probe(). >> > >> > Thank you for the suggestion. However, I am not sure that will work for >> > this SoC. >> > >> > The GPIO registers are described in section 12 of JH7100 datasheet [1] >> > and I don't think they fit the expectation of gpio-mmio.c because there >> > is a seperate register for each GPIO line for output data value and >> > output enable. >> > >> > There are 64 output data config registers which are 4 bytes wide. There >> > are 64 output enable config registers which are 4 bytes wide too. Output >> > data and output enable registers for a given GPIO pad are contiguous. >> > GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while GPIO1_DOUT_CFG >> > is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO pads is >> > effectively 8, which yields the formula: GPIOn_DOUT_CFG is 0x50+8n. >> > Similarly, GPIO0_DOEN_CFG is 0x54 and thus GPIOn_DOEN_CFG is 0x54+8n. >> > >> > However, GPIO input data does use just one bit for each line. GPIODIN_0 >> > at 0x48 covers GPIO[31:0] and GPIODIN_1 at 0x4c covers GPIO[63:32]. Mh, I'm not sure I'm understanding the datasheet/registers. _DOUT_CFG and _DOEN_CFG seem to specify the pad where this GPIO is mapped to. Shouldn't this be some kind of pinctrl then? Apparently you can map any GPIO number to any output pad, no? Or at least to all pads which are described in Table 11-2. What happens if two different GPIOs are mapped to the same pad? Bit 31 in these _CFG seems to be an invert bit, but what does it invert? Similar, the input GPIOs are connected to an output pad by all the GPI_*_CFG registers. To me it seems, that there two multiplexers for each GPIO, where you can connect any GPIOn to any input pad and output pad. Sound like a huge overkill. I must be missing something here. But what puzzles me the most, where do I set the actual GPIO output value? >> I'd say, that should work with the .reg_mask_xlate of the gpio-regmap. >> >> -michael > > Thanks, yes, I think trying to figure out how .reg_mask_xlate would > need > to work this SoC. I believe these are the only two implementations. > > From drivers/gpio/gpio-regmap.c: > > static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, > unsigned int base, unsigned int offset, > unsigned int *reg, unsigned int *mask) > { > unsigned int line = offset % gpio->ngpio_per_reg; > unsigned int stride = offset / gpio->ngpio_per_reg; > > *reg = base + stride * gpio->reg_stride; > *mask = BIT(line); > > return 0; > } > > From drivers/pinctrl/bcm/pinctrl-bcm63xx.c: > > static int bcm63xx_reg_mask_xlate(struct gpio_regmap *gpio, > unsigned int base, unsigned int offset, > unsigned int *reg, unsigned int *mask) > { > unsigned int line = offset % BCM63XX_BANK_GPIOS; > unsigned int stride = offset / BCM63XX_BANK_GPIOS; > > *reg = base - stride * BCM63XX_BANK_SIZE; > *mask = BIT(line); > > return 0; > } > > Let's say a driver calls gpio_regmap_set(chip, 0, 5) to set line 5 to > value 1. > > I believe this would result in call to: > > gpio->reg_mask_xlate(gpio, gpio->reg_set_base, 5, ®, &mask) > > Then this would be called to set the register: > > regmap_update_bits(gpio->regmap, reg, mask, mask); > > From datasheet section 12 [1], there are 64 output data registers which > are 4 bytes wide. There are 64 output enable registers which are also 4 > bytes wide too. Output data and output enable registers for a GPIO line > are contiguous. Thus GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54. > The forumla is GPIOn_DOUT_CFG is 0x50+8n and GPIOn_DOEN_CFG is 0x54+8n. > Thus for GPIO line 5: > > GPIO5_DOUT_CFG is 0x50 + 0x28 = 0x78 > GPIO5_DOEN_CFG is 0x54 + 0x28 = 0x7C > > Enable GPIO line 5 as output by writing 0x1 to 0x7C and set output > value > to 1 by writing 1 to 0x7C. > > Using gpio_regmap_simple_xlate() as a template, I am thinking through > xlate for this gpio controller: > > > static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio, > unsigned int base, unsigned int offset, > unsigned int *reg, unsigned int *mask) > { > // reg_set_base is passed as base > // let reg_set_base = 0x50 (GPIO0_DOUT_CFG) > // let gpio->reg_stride = 8 > // let offest = 5 (for gpio line 5) > > *reg = base + offset * gpio->reg_stride; > // *reg = base:0x50 + offset:0x5 * reg_stride:0x8 > // *reg = 0x50 + 0x28 > // *reg= 0x78 > > // Each gpio line has a full register, not just a bit. To output > // a digital 1, then GPIO5_DOUT_CFG would be 0x1. To output > // digital 0, GPIO5_DOUT_CFG would be 0x0. Thus I think the mask > // should be the least significant bit. > *mask = BIT(1); > > return 0; > } > > Let's walk through what would happen if gpio_regmap_set() was the > caller: > > static void gpio_regmap_set(struct gpio_chip *chip, unsigned int > offset, > int val) > { > // for gpio line, offset = 5 > // if want to set line 5 high, then val = 1 > struct gpio_regmap *gpio = gpiochip_get_data(chip); > > // reg_set_base would be set to 0x50 (GPIO0_DOUT_CFG) > unsigned int base = gpio_regmap_addr(gpio->reg_set_base); > unsigned int reg, mask; > > gpio->reg_mask_xlate(gpio, base /* 0x50 */, offset /* 5 */, ®, > &mask); > if (val) /* if val is 1 */ > regmap_update_bits(gpio->regmap, reg, mask, mask); > // if mask returned was 0x1, then this would set the > // bit 0 in GPIO5_DOUT_CFG > else /* if val is 0 */ > regmap_update_bits(gpio->regmap, reg, mask, 0); > // if mask returned was 0x1, then this would clear > // bit 0 in GPIO5_DOUT_CFG > } > > Now for the output enable register GPIO5_DOEN_CFG, the output driver is > active low so 0x0 is actually enables output where as 0x1 disables > output. Thus maybe I need to add logic like: > > > static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio, > unsigned int base, unsigned int offset, > unsigned int *reg, unsigned int *mask) > { > <snip> > if (base == GPIO0_DOUT_CFG) > *mask = 0x1U; > else if (base == GPIO0_DOEN_CFG) > *bit = ~(0x1U); > > return 0; > } > > What do you think of that approach? I'm also not opposed to add a new flag to gpio-regmap which invert the value itself. But the idea was that you can differentiate in _xlate() by the base register offset, like you already did: static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio, unsigned int base, unsigned int offset, unsigned int *reg, unsigned int *mask) { switch (base) { case GPIO0_DOUT_CFG: /* do some custom mapping just for DOUT_CFG */ case GPIO0_DOEN_CFG: /* do some custom mapping just for DOEN_CFG */ default: /* do normal mapping */ } > Are there any other examples of regmap xlate that I missed? No there aren't much yet. Usually the simple one is enough. -michael > [1] > https://github.com/starfive-tech/beaglev_doc/blob/main/JH7100%20Data%20Sheet%20V01.01.04-EN%20(4-21-2021).pdf
On Wed, 28 Jul 2021 at 11:49, Michael Walle <michael@walle.cc> wrote: > Hi Drew, > Am 2021-07-27 07:28, schrieb Drew Fustini: > [..] > >> > > Drew please look at drivers/gpio/gpio-ftgpio010.c for an example > >> > > of GPIO_GENERIC calling bgpio_init() in probe(). > >> > > >> > Thank you for the suggestion. However, I am not sure that will work for > >> > this SoC. > >> > > >> > The GPIO registers are described in section 12 of JH7100 datasheet [1] > >> > and I don't think they fit the expectation of gpio-mmio.c because there > >> > is a seperate register for each GPIO line for output data value and > >> > output enable. > >> > > >> > There are 64 output data config registers which are 4 bytes wide. There > >> > are 64 output enable config registers which are 4 bytes wide too. Output > >> > data and output enable registers for a given GPIO pad are contiguous. > >> > GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while GPIO1_DOUT_CFG > >> > is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO pads is > >> > effectively 8, which yields the formula: GPIOn_DOUT_CFG is 0x50+8n. > >> > Similarly, GPIO0_DOEN_CFG is 0x54 and thus GPIOn_DOEN_CFG is 0x54+8n. > >> > > >> > However, GPIO input data does use just one bit for each line. GPIODIN_0 > >> > at 0x48 covers GPIO[31:0] and GPIODIN_1 at 0x4c covers GPIO[63:32]. > > Mh, I'm not sure I'm understanding the datasheet/registers. _DOUT_CFG > and _DOEN_CFG seem to specify the pad where this GPIO is mapped to. > Shouldn't this be some kind of pinctrl then? Apparently you can map > any GPIO number to any output pad, no? Or at least to all pads > which are described in Table 11-2. What happens if two different GPIOs > are mapped to the same pad? Bit 31 in these _CFG seems to be an invert > bit, but what does it invert? > > Similar, the input GPIOs are connected to an output pad by all the > GPI_*_CFG registers. > > To me it seems, that there two multiplexers for each GPIO, where > you can connect any GPIOn to any input pad and output pad. Sound > like a huge overkill. I must be missing something here. > > But what puzzles me the most, where do I set the actual GPIO output > value? Yeah, it's a little confusing. The DOUT registers choose between a number of signals from various peripherals to control the output value of the pin. Similarly the DOEN registers chose between a number of signals to control the output enable of the pin. However, two of those signals are special in that they are constant 0 or constant 1. This is how you control the output value and output enable from software like a regular GPIO. You're completely right though. This ought to be managed by a proper pinctrl driver, and I'm working on one here: https://github.com/esmil/linux/commits/beaglev-pinctrl > >> I'd say, that should work with the .reg_mask_xlate of the gpio-regmap. > >> > >> -michael > > > > Thanks, yes, I think trying to figure out how .reg_mask_xlate would > > need > > to work this SoC. I believe these are the only two implementations. > > > > From drivers/gpio/gpio-regmap.c: > > > > static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, > > unsigned int base, unsigned int offset, > > unsigned int *reg, unsigned int *mask) > > { > > unsigned int line = offset % gpio->ngpio_per_reg; > > unsigned int stride = offset / gpio->ngpio_per_reg; > > > > *reg = base + stride * gpio->reg_stride; > > *mask = BIT(line); > > > > return 0; > > } > > > > From drivers/pinctrl/bcm/pinctrl-bcm63xx.c: > > > > static int bcm63xx_reg_mask_xlate(struct gpio_regmap *gpio, > > unsigned int base, unsigned int offset, > > unsigned int *reg, unsigned int *mask) > > { > > unsigned int line = offset % BCM63XX_BANK_GPIOS; > > unsigned int stride = offset / BCM63XX_BANK_GPIOS; > > > > *reg = base - stride * BCM63XX_BANK_SIZE; > > *mask = BIT(line); > > > > return 0; > > } > > > > Let's say a driver calls gpio_regmap_set(chip, 0, 5) to set line 5 to > > value 1. > > > > I believe this would result in call to: > > > > gpio->reg_mask_xlate(gpio, gpio->reg_set_base, 5, ®, &mask) > > > > Then this would be called to set the register: > > > > regmap_update_bits(gpio->regmap, reg, mask, mask); > > > > From datasheet section 12 [1], there are 64 output data registers which > > are 4 bytes wide. There are 64 output enable registers which are also 4 > > bytes wide too. Output data and output enable registers for a GPIO line > > are contiguous. Thus GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54. > > The forumla is GPIOn_DOUT_CFG is 0x50+8n and GPIOn_DOEN_CFG is 0x54+8n. > > Thus for GPIO line 5: > > > > GPIO5_DOUT_CFG is 0x50 + 0x28 = 0x78 > > GPIO5_DOEN_CFG is 0x54 + 0x28 = 0x7C > > > > Enable GPIO line 5 as output by writing 0x1 to 0x7C and set output > > value > > to 1 by writing 1 to 0x7C. > > > > Using gpio_regmap_simple_xlate() as a template, I am thinking through > > xlate for this gpio controller: > > > > > > static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio, > > unsigned int base, unsigned int offset, > > unsigned int *reg, unsigned int *mask) > > { > > // reg_set_base is passed as base > > // let reg_set_base = 0x50 (GPIO0_DOUT_CFG) > > // let gpio->reg_stride = 8 > > // let offest = 5 (for gpio line 5) > > > > *reg = base + offset * gpio->reg_stride; > > // *reg = base:0x50 + offset:0x5 * reg_stride:0x8 > > // *reg = 0x50 + 0x28 > > // *reg= 0x78 > > > > // Each gpio line has a full register, not just a bit. To output > > // a digital 1, then GPIO5_DOUT_CFG would be 0x1. To output > > // digital 0, GPIO5_DOUT_CFG would be 0x0. Thus I think the mask > > // should be the least significant bit. > > *mask = BIT(1); > > > > return 0; > > } > > > > Let's walk through what would happen if gpio_regmap_set() was the > > caller: > > > > static void gpio_regmap_set(struct gpio_chip *chip, unsigned int > > offset, > > int val) > > { > > // for gpio line, offset = 5 > > // if want to set line 5 high, then val = 1 > > struct gpio_regmap *gpio = gpiochip_get_data(chip); > > > > // reg_set_base would be set to 0x50 (GPIO0_DOUT_CFG) > > unsigned int base = gpio_regmap_addr(gpio->reg_set_base); > > unsigned int reg, mask; > > > > gpio->reg_mask_xlate(gpio, base /* 0x50 */, offset /* 5 */, ®, > > &mask); > > if (val) /* if val is 1 */ > > regmap_update_bits(gpio->regmap, reg, mask, mask); > > // if mask returned was 0x1, then this would set the > > // bit 0 in GPIO5_DOUT_CFG > > else /* if val is 0 */ > > regmap_update_bits(gpio->regmap, reg, mask, 0); > > // if mask returned was 0x1, then this would clear > > // bit 0 in GPIO5_DOUT_CFG > > } > > > > Now for the output enable register GPIO5_DOEN_CFG, the output driver is > > active low so 0x0 is actually enables output where as 0x1 disables > > output. Thus maybe I need to add logic like: > > > > > > static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio, > > unsigned int base, unsigned int offset, > > unsigned int *reg, unsigned int *mask) > > { > > <snip> > > if (base == GPIO0_DOUT_CFG) > > *mask = 0x1U; > > else if (base == GPIO0_DOEN_CFG) > > *bit = ~(0x1U); > > > > return 0; > > } > > > > What do you think of that approach? > > I'm also not opposed to add a new flag to gpio-regmap which > invert the value itself. > > But the idea was that you can differentiate in _xlate() by the > base register offset, like you already did: > > static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio, > unsigned int base, unsigned int offset, > unsigned int *reg, unsigned int *mask) > { > switch (base) { > case GPIO0_DOUT_CFG: > /* do some custom mapping just for DOUT_CFG */ > case GPIO0_DOEN_CFG: > /* do some custom mapping just for DOEN_CFG */ > default: > /* do normal mapping */ > } > > > Are there any other examples of regmap xlate that I missed? > > No there aren't much yet. Usually the simple one is enough. > > -michael > > > [1] > > https://github.com/starfive-tech/beaglev_doc/blob/main/JH7100%20Data%20Sheet%20V01.01.04-EN%20(4-21-2021).pdf >
Am 2021-07-28 12:59, schrieb Emil Renner Berthing: > On Wed, 28 Jul 2021 at 11:49, Michael Walle <michael@walle.cc> wrote: >> Hi Drew, >> Am 2021-07-27 07:28, schrieb Drew Fustini: >> [..] >> >> > > Drew please look at drivers/gpio/gpio-ftgpio010.c for an example >> >> > > of GPIO_GENERIC calling bgpio_init() in probe(). >> >> > >> >> > Thank you for the suggestion. However, I am not sure that will work for >> >> > this SoC. >> >> > >> >> > The GPIO registers are described in section 12 of JH7100 datasheet [1] >> >> > and I don't think they fit the expectation of gpio-mmio.c because there >> >> > is a seperate register for each GPIO line for output data value and >> >> > output enable. >> >> > >> >> > There are 64 output data config registers which are 4 bytes wide. There >> >> > are 64 output enable config registers which are 4 bytes wide too. Output >> >> > data and output enable registers for a given GPIO pad are contiguous. >> >> > GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while GPIO1_DOUT_CFG >> >> > is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO pads is >> >> > effectively 8, which yields the formula: GPIOn_DOUT_CFG is 0x50+8n. >> >> > Similarly, GPIO0_DOEN_CFG is 0x54 and thus GPIOn_DOEN_CFG is 0x54+8n. >> >> > >> >> > However, GPIO input data does use just one bit for each line. GPIODIN_0 >> >> > at 0x48 covers GPIO[31:0] and GPIODIN_1 at 0x4c covers GPIO[63:32]. >> >> Mh, I'm not sure I'm understanding the datasheet/registers. _DOUT_CFG >> and _DOEN_CFG seem to specify the pad where this GPIO is mapped to. >> Shouldn't this be some kind of pinctrl then? Apparently you can map >> any GPIO number to any output pad, no? Or at least to all pads >> which are described in Table 11-2. What happens if two different GPIOs >> are mapped to the same pad? Bit 31 in these _CFG seems to be an invert >> bit, but what does it invert? >> >> Similar, the input GPIOs are connected to an output pad by all the >> GPI_*_CFG registers. >> >> To me it seems, that there two multiplexers for each GPIO, where >> you can connect any GPIOn to any input pad and output pad. Sound >> like a huge overkill. I must be missing something here. >> >> But what puzzles me the most, where do I set the actual GPIO output >> value? > > Yeah, it's a little confusing. The DOUT registers choose between a > number of > signals from various peripherals to control the output value of the > pin. Similarly > the DOEN registers chose between a number of signals to control the > output > enable of the pin. However, two of those signals are special in that > they are > constant 0 or constant 1. This is how you control the output value and > output > enable from software like a regular GPIO. > > You're completely right though. This ought to be managed by a proper > pinctrl > driver, and I'm working on one here: > https://github.com/esmil/linux/commits/beaglev-pinctrl Ahh, I see. So for the non-gpio function you have to set a value other than 0 or 1, correct? And as an implementation detail you have to set the corresponding OE pin if the non-gpio function will need a tristate pin (or whatever). So, the _DOUT_CFG will actually be shared between the pinctrl and the gpio driver, right? (I haven't done anything with pinctrl, so this might be a stupid question). -michael
On Wed, 28 Jul 2021 at 13:19, Michael Walle <michael@walle.cc> wrote: > Am 2021-07-28 12:59, schrieb Emil Renner Berthing: > > On Wed, 28 Jul 2021 at 11:49, Michael Walle <michael@walle.cc> wrote: > >> Hi Drew, > >> Am 2021-07-27 07:28, schrieb Drew Fustini: > >> [..] > >> >> > > Drew please look at drivers/gpio/gpio-ftgpio010.c for an example > >> >> > > of GPIO_GENERIC calling bgpio_init() in probe(). > >> >> > > >> >> > Thank you for the suggestion. However, I am not sure that will work for > >> >> > this SoC. > >> >> > > >> >> > The GPIO registers are described in section 12 of JH7100 datasheet [1] > >> >> > and I don't think they fit the expectation of gpio-mmio.c because there > >> >> > is a seperate register for each GPIO line for output data value and > >> >> > output enable. > >> >> > > >> >> > There are 64 output data config registers which are 4 bytes wide. There > >> >> > are 64 output enable config registers which are 4 bytes wide too. Output > >> >> > data and output enable registers for a given GPIO pad are contiguous. > >> >> > GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while GPIO1_DOUT_CFG > >> >> > is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO pads is > >> >> > effectively 8, which yields the formula: GPIOn_DOUT_CFG is 0x50+8n. > >> >> > Similarly, GPIO0_DOEN_CFG is 0x54 and thus GPIOn_DOEN_CFG is 0x54+8n. > >> >> > > >> >> > However, GPIO input data does use just one bit for each line. GPIODIN_0 > >> >> > at 0x48 covers GPIO[31:0] and GPIODIN_1 at 0x4c covers GPIO[63:32]. > >> > >> Mh, I'm not sure I'm understanding the datasheet/registers. _DOUT_CFG > >> and _DOEN_CFG seem to specify the pad where this GPIO is mapped to. > >> Shouldn't this be some kind of pinctrl then? Apparently you can map > >> any GPIO number to any output pad, no? Or at least to all pads > >> which are described in Table 11-2. What happens if two different GPIOs > >> are mapped to the same pad? Bit 31 in these _CFG seems to be an invert > >> bit, but what does it invert? > >> > >> Similar, the input GPIOs are connected to an output pad by all the > >> GPI_*_CFG registers. > >> > >> To me it seems, that there two multiplexers for each GPIO, where > >> you can connect any GPIOn to any input pad and output pad. Sound > >> like a huge overkill. I must be missing something here. > >> > >> But what puzzles me the most, where do I set the actual GPIO output > >> value? > > > > Yeah, it's a little confusing. The DOUT registers choose between a > > number of > > signals from various peripherals to control the output value of the > > pin. Similarly > > the DOEN registers chose between a number of signals to control the > > output > > enable of the pin. However, two of those signals are special in that > > they are > > constant 0 or constant 1. This is how you control the output value and > > output > > enable from software like a regular GPIO. > > > > You're completely right though. This ought to be managed by a proper > > pinctrl > > driver, and I'm working on one here: > > https://github.com/esmil/linux/commits/beaglev-pinctrl > > Ahh, I see. So for the non-gpio function you have to set a value other > than 0 or 1, correct? > > And as an implementation detail you have to set the corresponding OE > pin if the non-gpio function will need a tristate pin (or whatever). > > So, the _DOUT_CFG will actually be shared between the pinctrl and the > gpio driver, right? (I haven't done anything with pinctrl, so this might > be a stupid question). No, not a stupid question. You've got that exactly right. /Emil
diff --git a/MAINTAINERS b/MAINTAINERS index bc0ceef87b73..04fccc2ceffa 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17423,6 +17423,14 @@ S: Supported T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git F: drivers/staging/ +SIFVE JH7100 SOC GPIO DRIVER +M: Drew Fustini <drew@beagleboard.org> +M: Huan Feng <huan.feng@starfivetech.com> +L: linux-riscv@lists.infradead.org +L: linux-gpio@vger.kernel.org +F: Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml +F: drivers/gpio/gpio-starfive-jh7100.c + STARFIRE/DURALAN NETWORK DRIVER M: Ion Badulescu <ionut@badula.org> S: Odd Fixes diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 1dd0ec6727fd..26630e4852c0 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -542,6 +542,14 @@ config GPIO_SIFIVE help Say yes here to support the GPIO device on SiFive SoCs. +config GPIO_STARFIVE_JH7100 + bool "StarFive JH7100 GPIO support" + depends on OF_GPIO + select GPIOLIB_IRQCHIP + default y if SOC_STARFIVE_VIC7100 + help + Say yes here to support the GPIO device on StarFive JH7100 SoC. + config GPIO_SIOX tristate "SIOX GPIO support" depends on SIOX diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index d7c81e1611a4..939922eaf5f3 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -132,6 +132,7 @@ obj-$(CONFIG_GPIO_SAMA5D2_PIOBU) += gpio-sama5d2-piobu.o obj-$(CONFIG_GPIO_SCH311X) += gpio-sch311x.o obj-$(CONFIG_GPIO_SCH) += gpio-sch.o obj-$(CONFIG_GPIO_SIFIVE) += gpio-sifive.o +obj-$(CONFIG_GPIO_STARFIVE_JH7100) += gpio-starfive-jh7100.o obj-$(CONFIG_GPIO_SIOX) += gpio-siox.o obj-$(CONFIG_GPIO_SL28CPLD) += gpio-sl28cpld.o obj-$(CONFIG_GPIO_SODAVILLE) += gpio-sodaville.o diff --git a/drivers/gpio/gpio-starfive-jh7100.c b/drivers/gpio/gpio-starfive-jh7100.c new file mode 100644 index 000000000000..b94ebfe9eaf7 --- /dev/null +++ b/drivers/gpio/gpio-starfive-jh7100.c @@ -0,0 +1,425 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * GPIO driver for StarFive JH7100 SoC + * + * Copyright (C) 2020 Shanghai StarFive Technology Co., Ltd. + */ + +#include <linux/module.h> +#include <linux/gpio/driver.h> +#include <linux/platform_device.h> +#include <linux/spinlock.h> + +/* + * refer to Section 12. GPIO Registers in JH7100 datasheet: + * https://github.com/starfive-tech/beaglev_doc + */ + +/* global enable */ +#define GPIO_EN 0x0 + +/* interrupt type */ +#define GPIO_IS_LOW 0x10 +#define GPIO_IS_HIGH 0x14 + +/* edge trigger interrupt type */ +#define GPIO_IBE_LOW 0x18 +#define GPIO_IBE_HIGH 0x1c + +/* edge trigger interrupt polarity */ +#define GPIO_IEV_LOW 0x20 +#define GPIO_IEV_HIGH 0x24 + +/* interrupt max */ +#define GPIO_IE_LOW 0x28 +#define GPIO_IE_HIGH 0x2c + +/* clear edge-triggered interrupt */ +#define GPIO_IC_LOW 0x30 +#define GPIO_IC_HIGH 0x34 + +/* edge-triggered interrupt status (read-only) */ +#define GPIO_RIS_LOW 0x38 +#define GPIO_RIS_HIGH 0x3c + +/* interrupt status after masking (read-only) */ +#define GPIO_MIS_LOW 0x40 +#define GPIO_MIS_HIGH 0x44 + +/* data value of gpio */ +#define GPIO_DIN_LOW 0x48 +#define GPIO_DIN_HIGH 0x4c + +/* GPIO0_DOUT_CFG is 0x50, GPIOn_DOUT_CFG is 0x50+(n*8) */ +#define GPIO_DOUT_X_REG 0x50 + +/* GPIO0_DOEN_CFG is 0x54, GPIOn_DOEN_CFG is 0x54+(n*8) */ +#define GPIO_DOEN_X_REG 0x54 + +#define MAX_GPIO 64 + +struct starfive_gpio { + raw_spinlock_t lock; + void __iomem *base; + struct gpio_chip gc; + unsigned long enabled; + unsigned int trigger[MAX_GPIO]; + unsigned int irq_parent[MAX_GPIO]; +}; + +static int starfive_direction_input(struct gpio_chip *gc, unsigned int offset) +{ + struct starfive_gpio *chip = gpiochip_get_data(gc); + unsigned long flags; + + if (offset >= gc->ngpio) + return -EINVAL; + + raw_spin_lock_irqsave(&chip->lock, flags); + writel_relaxed(0x1, chip->base + GPIO_DOEN_X_REG + offset * 8); + raw_spin_unlock_irqrestore(&chip->lock, flags); + + return 0; +} + +static int starfive_direction_output(struct gpio_chip *gc, unsigned int offset, int value) +{ + struct starfive_gpio *chip = gpiochip_get_data(gc); + unsigned long flags; + + if (offset >= gc->ngpio) + return -EINVAL; + + raw_spin_lock_irqsave(&chip->lock, flags); + writel_relaxed(0x0, chip->base + GPIO_DOEN_X_REG + offset * 8); + writel_relaxed(value, chip->base + GPIO_DOUT_X_REG + offset * 8); + raw_spin_unlock_irqrestore(&chip->lock, flags); + + return 0; +} + +static int starfive_get_direction(struct gpio_chip *gc, unsigned int offset) +{ + struct starfive_gpio *chip = gpiochip_get_data(gc); + + if (offset >= gc->ngpio) + return -EINVAL; + + return readl_relaxed(chip->base + GPIO_DOEN_X_REG + offset * 8) & 0x1; +} + +static int starfive_get_value(struct gpio_chip *gc, unsigned int offset) +{ + struct starfive_gpio *chip = gpiochip_get_data(gc); + int value; + + if (offset >= gc->ngpio) + return -EINVAL; + + if (offset < 32) { + value = readl_relaxed(chip->base + GPIO_DIN_LOW); + value = (value >> offset) & 0x1; + } else { + value = readl_relaxed(chip->base + GPIO_DIN_HIGH); + value = (value >> (offset - 32)) & 0x1; + } + + return value; +} + +static void starfive_set_value(struct gpio_chip *gc, unsigned int offset, int value) +{ + struct starfive_gpio *chip = gpiochip_get_data(gc); + unsigned long flags; + + if (offset >= gc->ngpio) + return; + + raw_spin_lock_irqsave(&chip->lock, flags); + writel_relaxed(value, chip->base + GPIO_DOUT_X_REG + offset * 8); + raw_spin_unlock_irqrestore(&chip->lock, flags); +} + +static void starfive_set_ie(struct starfive_gpio *chip, int offset) +{ + unsigned long flags; + int old_value, new_value; + int reg_offset, index; + + if (offset < 32) { + reg_offset = 0; + index = offset; + } else { + reg_offset = 4; + index = offset - 32; + } + raw_spin_lock_irqsave(&chip->lock, flags); + old_value = readl_relaxed(chip->base + GPIO_IE_LOW + reg_offset); + new_value = old_value | (1 << index); + writel_relaxed(new_value, chip->base + GPIO_IE_LOW + reg_offset); + raw_spin_unlock_irqrestore(&chip->lock, flags); +} + +static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct starfive_gpio *chip = gpiochip_get_data(gc); + int offset = irqd_to_hwirq(d); + unsigned int reg_is, reg_ibe, reg_iev; + int reg_offset, index; + + if (offset < 0 || offset >= gc->ngpio) + return -EINVAL; + + if (offset < 32) { + reg_offset = 0; + index = offset; + } else { + reg_offset = 4; + index = offset - 32; + } + + reg_is = readl_relaxed(chip->base + GPIO_IS_LOW + reg_offset); + reg_ibe = readl_relaxed(chip->base + GPIO_IBE_LOW + reg_offset); + reg_iev = readl_relaxed(chip->base + GPIO_IEV_LOW + reg_offset); + + switch (trigger) { + case IRQ_TYPE_LEVEL_HIGH: + reg_is &= ~(1 << index); + reg_ibe &= ~(1 << index); + reg_iev |= (1 << index); + break; + case IRQ_TYPE_LEVEL_LOW: + reg_is &= ~(1 << index); + reg_ibe &= ~(1 << index); + reg_iev &= (1 << index); + break; + case IRQ_TYPE_EDGE_BOTH: + reg_is |= ~(1 << index); + reg_ibe |= ~(1 << index); + // no need to set edge type when both + break; + case IRQ_TYPE_EDGE_RISING: + reg_is |= ~(1 << index); + reg_ibe &= ~(1 << index); + reg_iev |= (1 << index); + break; + case IRQ_TYPE_EDGE_FALLING: + reg_is |= ~(1 << index); + reg_ibe &= ~(1 << index); + reg_iev &= (1 << index); + break; + } + + writel_relaxed(reg_is, chip->base + GPIO_IS_LOW + reg_offset); + writel_relaxed(reg_ibe, chip->base + GPIO_IBE_LOW + reg_offset); + writel_relaxed(reg_iev, chip->base + GPIO_IEV_LOW + reg_offset); + chip->trigger[offset] = trigger; + starfive_set_ie(chip, offset); + return 0; +} + +/* chained_irq_{enter,exit} already mask the parent */ +static void starfive_irq_mask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct starfive_gpio *chip = gpiochip_get_data(gc); + unsigned int value; + int offset = irqd_to_hwirq(d); + int reg_offset, index; + + if (offset < 0 || offset >= gc->ngpio) + return; + + if (offset < 32) { + reg_offset = 0; + index = offset; + } else { + reg_offset = 4; + index = offset - 32; + } + + value = readl_relaxed(chip->base + GPIO_IE_LOW + reg_offset); + value &= ~(0x1 << index); + writel_relaxed(value, chip->base + GPIO_IE_LOW + reg_offset); +} + +static void starfive_irq_unmask(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct starfive_gpio *chip = gpiochip_get_data(gc); + unsigned int value; + int offset = irqd_to_hwirq(d); + int reg_offset, index; + + if (offset < 0 || offset >= gc->ngpio) + return; + + if (offset < 32) { + reg_offset = 0; + index = offset; + } else { + reg_offset = 4; + index = offset - 32; + } + + value = readl_relaxed(chip->base + GPIO_IE_LOW + reg_offset); + value |= (0x1 << index); + writel_relaxed(value, chip->base + GPIO_IE_LOW + reg_offset); +} + +static void starfive_irq_enable(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct starfive_gpio *chip = gpiochip_get_data(gc); + int offset = irqd_to_hwirq(d); + + starfive_irq_unmask(d); + assign_bit(offset, &chip->enabled, 1); +} + +static void starfive_irq_disable(struct irq_data *d) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct starfive_gpio *chip = gpiochip_get_data(gc); + int offset = irqd_to_hwirq(d) % MAX_GPIO; // must not fail + + assign_bit(offset, &chip->enabled, 0); + starfive_set_ie(chip, offset); +} + +static struct irq_chip starfive_irqchip = { + .name = "starfive-jh7100-gpio", + .irq_set_type = starfive_irq_set_type, + .irq_mask = starfive_irq_mask, + .irq_unmask = starfive_irq_unmask, + .irq_enable = starfive_irq_enable, + .irq_disable = starfive_irq_disable, +}; + +static irqreturn_t starfive_irq_handler(int irq, void *gc) +{ + int offset; + int reg_offset, index; + unsigned int value; + unsigned long flags; + struct starfive_gpio *chip = gc; + + for (offset = 0; offset < MAX_GPIO; offset++) { + if (offset < 32) { + reg_offset = 0; + index = offset; + } else { + reg_offset = 4; + index = offset - 32; + } + + raw_spin_lock_irqsave(&chip->lock, flags); + value = readl_relaxed(chip->base + GPIO_MIS_LOW + reg_offset); + if (value & BIT(index)) + writel_relaxed(BIT(index), chip->base + GPIO_IC_LOW + + reg_offset); + raw_spin_unlock_irqrestore(&chip->lock, flags); + } + + return IRQ_HANDLED; +} + +static int starfive_gpio_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct starfive_gpio *chip; + struct gpio_irq_chip *girq; + struct resource *res; + int irq, ret, ngpio; + + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); + if (!chip) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + chip->base = devm_ioremap_resource(dev, res); + if (IS_ERR(chip->base)) { + dev_err(dev, "failed to allocate device memory\n"); + return PTR_ERR(chip->base); + } + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_err(dev, "Cannot get IRQ resource\n"); + return irq; + } + + raw_spin_lock_init(&chip->lock); + chip->gc.direction_input = starfive_direction_input; + chip->gc.direction_output = starfive_direction_output; + chip->gc.get_direction = starfive_get_direction; + chip->gc.get = starfive_get_value; + chip->gc.set = starfive_set_value; + chip->gc.base = 0; + chip->gc.ngpio = MAX_GPIO; + chip->gc.label = dev_name(dev); + chip->gc.parent = dev; + chip->gc.owner = THIS_MODULE; + + girq = &chip->gc.irq; + girq->chip = &starfive_irqchip; + girq->parent_handler = NULL; + girq->num_parents = 0; + girq->parents = NULL; + girq->default_type = IRQ_TYPE_NONE; + girq->handler = handle_simple_irq; + + ret = gpiochip_add_data(&chip->gc, chip); + if (ret) { + dev_err(dev, "gpiochip_add_data ret=%d!\n", ret); + return ret; + } + + /* Disable all GPIO interrupts before enabling parent interrupts */ + iowrite32(0, chip->base + GPIO_IE_HIGH); + iowrite32(0, chip->base + GPIO_IE_LOW); + chip->enabled = 0; + + ret = devm_request_irq(dev, irq, starfive_irq_handler, IRQF_SHARED, + dev_name(dev), chip); + if (ret) { + dev_err(dev, "IRQ handler registering failed (%d)\n", ret); + return ret; + } + + writel_relaxed(1, chip->base + GPIO_EN); + + dev_info(dev, "StarFive GPIO chip registered %d GPIOs\n", ngpio); + + return 0; +} + +static const struct of_device_id starfive_gpio_match[] = { + { .compatible = "starfive,jh7100-gpio", }, + { }, +}; + +static struct platform_driver starfive_gpio_driver = { + .probe = starfive_gpio_probe, + .driver = { + .name = "gpio_starfive_jh7100", + .of_match_table = of_match_ptr(starfive_gpio_match), + }, +}; + +static int __init starfive_gpio_init(void) +{ + return platform_driver_register(&starfive_gpio_driver); +} +subsys_initcall(starfive_gpio_init); + +static void __exit starfive_gpio_exit(void) +{ + platform_driver_unregister(&starfive_gpio_driver); +} +module_exit(starfive_gpio_exit); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Huan Feng <huan.feng@starfivetech.com>"); +MODULE_DESCRIPTION("StarFive JH7100 GPIO driver");