[RFC PATH 0/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO bindings and driver

[RFC PATH 0/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO bindings and driver

[Drew Fustini]

Add device tree bindings and driver for the GPIO controller in the
StarFive JH7100 SoC [1] used on the BeagleV Starlight JH7100 board [2].

The dts using "starfive,jh7100-gpio" is in StarFive's linux repo [3] and
is being cleaned up in preperation for submission.

[1] https://github.com/starfive-tech/beaglev_doc
[2] https://github.com/beagleboard/beaglev-starlight
[3] https://github.com/starfive-tech/linux/blob/beaglev/arch/riscv/boot/dts/starfive/jh7100.dtsi#L262

Drew Fustini (2):
  dt-bindings: gpio: add starfive,jh7100-gpio bindings
  gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

 .../bindings/gpio/starfive,jh7100-gpio.yaml   |  60 +++
 MAINTAINERS                                   |   8 +
 drivers/gpio/Kconfig                          |   8 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-starfive-jh7100.c           | 425 ++++++++++++++++++
 5 files changed, 502 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml
 create mode 100644 drivers/gpio/gpio-starfive-jh7100.c

-- 
2.27.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[RFC PATH 1/2] dt-bindings: gpio: add starfive,jh7100-gpio bindings

[Drew Fustini]

Add bindings for the GPIO controller in the StarFive JH7100 SoC [1].

[1] https://github.com/starfive-tech/beaglev_doc

Signed-off-by: Drew Fustini <drew@beagleboard.org>
Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
---
 .../bindings/gpio/starfive,jh7100-gpio.yaml   | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml b/Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml
new file mode 100644
index 000000000000..8c9d14d9ac3b
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/starfive,jh7100-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH7100 GPIO controller
+
+maintainers:
+  - Huan Feng <huan.feng@starfivetech.com>
+  - Drew Fustini <drew@beagleboard.org>
+
+properties:
+  compatible:
+    items:
+      - const: starfive,jh7100-gpio
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    description:
+      Interrupt mapping, one per GPIO. Maximum 32 GPIOs.
+    minItems: 1
+    maxItems: 32
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-controller
+  - "#interrupt-cells"
+  - "#gpio-cells"
+  - gpio-controller
+
+additionalProperties: false
+
+examples:
+  - |
+      gpio@11910000 {
+        compatible = "starfive,jh7100-gpio";
+        reg = <0x11910000 0x10000>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupts = <32>;
+      };
+
+...
-- 
2.27.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[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>
---
 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
+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");
-- 
2.27.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Bin Meng]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Michael Walle]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 1/2] dt-bindings: gpio: add starfive, jh7100-gpio bindings

[Geert Uytterhoeven]

Hi Drew,

On Thu, Jul 1, 2021 at 2:22 AM Drew Fustini <drew@beagleboard.org> wrote:
> Add bindings for the GPIO controller in the StarFive JH7100 SoC [1].
>
> [1] https://github.com/starfive-tech/beaglev_doc
>
> Signed-off-by: Drew Fustini <drew@beagleboard.org>
> Signed-off-by: Huan Feng <huan.feng@starfivetech.com>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/starfive,jh7100-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive JH7100 GPIO controller
> +
> +maintainers:
> +  - Huan Feng <huan.feng@starfivetech.com>
> +  - Drew Fustini <drew@beagleboard.org>
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: starfive,jh7100-gpio
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    description:
> +      Interrupt mapping, one per GPIO. Maximum 32 GPIOs.
> +    minItems: 1
> +    maxItems: 32

What about clocks and resets?

> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +  - "#gpio-cells"
> +  - gpio-controller
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +      gpio@11910000 {
> +        compatible = "starfive,jh7100-gpio";
> +        reg = <0x11910000 0x10000>;
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +        interrupts = <32>;
> +      };
> +
> +...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

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

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Drew Fustini]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Michael Walle]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Andy Shevchenko]

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.

Brief look into the code brings the Q. Can't you utilize gpio-regmap
here? Why not?

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 1/2] dt-bindings: gpio: add starfive,jh7100-gpio bindings

[Drew Fustini]

On Thu, Jul 01, 2021 at 10:34:56AM +0200, Geert Uytterhoeven wrote:
> Hi Drew,
> 
> On Thu, Jul 1, 2021 at 2:22 AM Drew Fustini <drew@beagleboard.org> wrote:
> > Add bindings for the GPIO controller in the StarFive JH7100 SoC [1].
> >
> > [1] https://github.com/starfive-tech/beaglev_doc
> >
> > Signed-off-by: Drew Fustini <drew@beagleboard.org>
> > Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml
> > @@ -0,0 +1,60 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/starfive,jh7100-gpio.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: StarFive JH7100 GPIO controller
> > +
> > +maintainers:
> > +  - Huan Feng <huan.feng@starfivetech.com>
> > +  - Drew Fustini <drew@beagleboard.org>
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: starfive,jh7100-gpio
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    description:
> > +      Interrupt mapping, one per GPIO. Maximum 32 GPIOs.
> > +    minItems: 1
> > +    maxItems: 32
> 
> What about clocks and resets?

Thank you for your feedback, Geert.

GPIO controller uses clk_apb1_bus under dom0_sys.  I believe the device
tree node would use something like this:

  clocks = <&clkgen JH7100_CLK_APB1>;

I see the sifive-gpio.yaml has:

  clocks:
    maxItems: 1

Would that be the correct way to do it for the starfive gpio yaml?


The reset for GPIO controller is presetn under dom_sys.  Do you think
know you know an example that has reset in the YAML?  Is there some code
that would actually make use of that information?

> 
> > +
> > +  gpio-controller: true
> > +
> > +  "#gpio-cells":
> > +    const: 2
> > +
> > +  interrupt-controller: true
> > +
> > +  "#interrupt-cells":
> > +    const: 2
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - interrupt-controller
> > +  - "#interrupt-cells"
> > +  - "#gpio-cells"
> > +  - gpio-controller

Do you think I should add 'clocks' to 'required:'?

> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +      gpio@11910000 {
> > +        compatible = "starfive,jh7100-gpio";
> > +        reg = <0x11910000 0x10000>;
> > +        gpio-controller;
> > +        #gpio-cells = <2>;
> > +        interrupt-controller;
> > +        #interrupt-cells = <2>;
> > +        interrupts = <32>;

I would add:

  clocks = <&clkgen JH7100_CLK_APB1>;

But I am not sure how reset would work?


Thank you,
Drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[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

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 1/2] dt-bindings: gpio: add starfive, jh7100-gpio bindings

[Geert Uytterhoeven]

Hi Drew,

On Fri, Jul 2, 2021 at 10:56 PM Drew Fustini <drew@beagleboard.org> wrote:
> On Thu, Jul 01, 2021 at 10:34:56AM +0200, Geert Uytterhoeven wrote:
> > On Thu, Jul 1, 2021 at 2:22 AM Drew Fustini <drew@beagleboard.org> wrote:
> > > Add bindings for the GPIO controller in the StarFive JH7100 SoC [1].
> > >
> > > [1] https://github.com/starfive-tech/beaglev_doc
> > >
> > > Signed-off-by: Drew Fustini <drew@beagleboard.org>
> > > Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> >
> > Thanks for your patch!
> >
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml
> > > @@ -0,0 +1,60 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/gpio/starfive,jh7100-gpio.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: StarFive JH7100 GPIO controller
> > > +
> > > +maintainers:
> > > +  - Huan Feng <huan.feng@starfivetech.com>
> > > +  - Drew Fustini <drew@beagleboard.org>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - const: starfive,jh7100-gpio
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    description:
> > > +      Interrupt mapping, one per GPIO. Maximum 32 GPIOs.
> > > +    minItems: 1
> > > +    maxItems: 32
> >
> > What about clocks and resets?
>
> Thank you for your feedback, Geert.
>
> GPIO controller uses clk_apb1_bus under dom0_sys.  I believe the device
> tree node would use something like this:
>
>   clocks = <&clkgen JH7100_CLK_APB1>;
>
> I see the sifive-gpio.yaml has:
>
>   clocks:
>     maxItems: 1
>
> Would that be the correct way to do it for the starfive gpio yaml?

Yep.

> The reset for GPIO controller is presetn under dom_sys.  Do you think
> know you know an example that has reset in the YAML?  Is there some code
> that would actually make use of that information?
>
> >
> > > +
> > > +  gpio-controller: true
> > > +
> > > +  "#gpio-cells":
> > > +    const: 2
> > > +
> > > +  interrupt-controller: true
> > > +
> > > +  "#interrupt-cells":
> > > +    const: 2
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +  - interrupt-controller
> > > +  - "#interrupt-cells"
> > > +  - "#gpio-cells"
> > > +  - gpio-controller
>
> Do you think I should add 'clocks' to 'required:'?

I'm still having issues with i2c if the GPIO block lists a clock, due to
fw_devlink dependencies.

> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +      gpio@11910000 {
> > > +        compatible = "starfive,jh7100-gpio";
> > > +        reg = <0x11910000 0x10000>;
> > > +        gpio-controller;
> > > +        #gpio-cells = <2>;
> > > +        interrupt-controller;
> > > +        #interrupt-cells = <2>;
> > > +        interrupts = <32>;
>
> I would add:
>
>   clocks = <&clkgen JH7100_CLK_APB1>;
>
> But I am not sure how reset would work?

That should become "resets = <&rstgen JH7100_RSTN_GPIO_APB>",
but we don't have the reset controller in Linux yet (we do in barebox).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

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

Thanks,
Drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 1/2] dt-bindings: gpio: add starfive,jh7100-gpio bindings

[Drew Fustini]

On Fri, Jul 02, 2021 at 11:03:56PM +0200, Geert Uytterhoeven wrote:
> Hi Drew,
> 
> On Fri, Jul 2, 2021 at 10:56 PM Drew Fustini <drew@beagleboard.org> wrote:
> > On Thu, Jul 01, 2021 at 10:34:56AM +0200, Geert Uytterhoeven wrote:
> > > On Thu, Jul 1, 2021 at 2:22 AM Drew Fustini <drew@beagleboard.org> wrote:
> > > > Add bindings for the GPIO controller in the StarFive JH7100 SoC [1].
> > > >
> > > > [1] https://github.com/starfive-tech/beaglev_doc
> > > >
> > > > Signed-off-by: Drew Fustini <drew@beagleboard.org>
> > > > Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> > >
> > > Thanks for your patch!
> > >
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml
> > > > @@ -0,0 +1,60 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/gpio/starfive,jh7100-gpio.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: StarFive JH7100 GPIO controller
> > > > +
> > > > +maintainers:
> > > > +  - Huan Feng <huan.feng@starfivetech.com>
> > > > +  - Drew Fustini <drew@beagleboard.org>
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - const: starfive,jh7100-gpio
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    description:
> > > > +      Interrupt mapping, one per GPIO. Maximum 32 GPIOs.
> > > > +    minItems: 1
> > > > +    maxItems: 32
> > >
> > > What about clocks and resets?
> >
> > Thank you for your feedback, Geert.
> >
> > GPIO controller uses clk_apb1_bus under dom0_sys.  I believe the device
> > tree node would use something like this:
> >
> >   clocks = <&clkgen JH7100_CLK_APB1>;
> >
> > I see the sifive-gpio.yaml has:
> >
> >   clocks:
> >     maxItems: 1
> >
> > Would that be the correct way to do it for the starfive gpio yaml?
> 
> Yep.
> 
> > The reset for GPIO controller is presetn under dom_sys.  Do you think
> > know you know an example that has reset in the YAML?  Is there some code
> > that would actually make use of that information?
> >
> > >
> > > > +
> > > > +  gpio-controller: true
> > > > +
> > > > +  "#gpio-cells":
> > > > +    const: 2
> > > > +
> > > > +  interrupt-controller: true
> > > > +
> > > > +  "#interrupt-cells":
> > > > +    const: 2
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - interrupts
> > > > +  - interrupt-controller
> > > > +  - "#interrupt-cells"
> > > > +  - "#gpio-cells"
> > > > +  - gpio-controller
> >
> > Do you think I should add 'clocks' to 'required:'?
> 
> I'm still having issues with i2c if the GPIO block lists a clock, due to
> fw_devlink dependencies.
> 
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +      gpio@11910000 {
> > > > +        compatible = "starfive,jh7100-gpio";
> > > > +        reg = <0x11910000 0x10000>;
> > > > +        gpio-controller;
> > > > +        #gpio-cells = <2>;
> > > > +        interrupt-controller;
> > > > +        #interrupt-cells = <2>;
> > > > +        interrupts = <32>;
> >
> > I would add:
> >
> >   clocks = <&clkgen JH7100_CLK_APB1>;
> >
> > But I am not sure how reset would work?
> 
> That should become "resets = <&rstgen JH7100_RSTN_GPIO_APB>",
> but we don't have the reset controller in Linux yet (we do in barebox).

Do you think I should add reset item like this?

  resets:
    maxItems: 1

I suppose this is supposed to describe the hardware and it shouldn't
matter whether or not Linux uses the property, right?

Thank you,
Drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 1/2] dt-bindings: gpio: add starfive, jh7100-gpio bindings

[Geert Uytterhoeven]

Hi Drew,

On Sat, Jul 3, 2021 at 8:46 AM Drew Fustini <drew@beagleboard.org> wrote:
> On Fri, Jul 02, 2021 at 11:03:56PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Jul 2, 2021 at 10:56 PM Drew Fustini <drew@beagleboard.org> wrote:
> > > On Thu, Jul 01, 2021 at 10:34:56AM +0200, Geert Uytterhoeven wrote:
> > > > On Thu, Jul 1, 2021 at 2:22 AM Drew Fustini <drew@beagleboard.org> wrote:
> > > > > Add bindings for the GPIO controller in the StarFive JH7100 SoC [1].
> > > > >
> > > > > [1] https://github.com/starfive-tech/beaglev_doc
> > > > >
> > > > > Signed-off-by: Drew Fustini <drew@beagleboard.org>
> > > > > Signed-off-by: Huan Feng <huan.feng@starfivetech.com>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml
> > > > > @@ -0,0 +1,60 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/gpio/starfive,jh7100-gpio.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: StarFive JH7100 GPIO controller
> > > > > +
> > > > > +maintainers:
> > > > > +  - Huan Feng <huan.feng@starfivetech.com>
> > > > > +  - Drew Fustini <drew@beagleboard.org>
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    items:
> > > > > +      - const: starfive,jh7100-gpio
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  interrupts:
> > > > > +    description:
> > > > > +      Interrupt mapping, one per GPIO. Maximum 32 GPIOs.
> > > > > +    minItems: 1
> > > > > +    maxItems: 32
> > > >
> > > > What about clocks and resets?

> > > But I am not sure how reset would work?
> >
> > That should become "resets = <&rstgen JH7100_RSTN_GPIO_APB>",
> > but we don't have the reset controller in Linux yet (we do in barebox).
>
> Do you think I should add reset item like this?
>
>   resets:
>     maxItems: 1
>
> I suppose this is supposed to describe the hardware and it shouldn't
> matter whether or not Linux uses the property, right?

Exactly.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Michael Walle]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Matti Vaittinen]

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Ley Foon Tan]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Linus Walleij]

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

Yours,
Linus Walleij

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Michael Walle]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Drew Fustini]

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, &reg, &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 */, &reg, &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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Michael Walle]

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, &reg, &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 */, &reg, 
> &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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[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

> >> 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, &reg, &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 */, &reg,
> > &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
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Michael Walle]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [RFC PATH 2/2] gpio: starfive-jh7100: Add StarFive JH7100 GPIO driver

[Emil Renner Berthing]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv