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
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 <[email protected]>
Signed-off-by: Huan Feng <[email protected]>
---
.../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 <[email protected]>
+ - Drew Fustini <[email protected]>
+
+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
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 <[email protected]>
Signed-off-by: Huan Feng <[email protected]>
Signed-off-by: Drew Fustini <[email protected]>
---
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 <[email protected]>
+M: Huan Feng <[email protected]>
+L: [email protected]
+L: [email protected]
+F: Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml
+F: drivers/gpio/gpio-starfive-jh7100.c
+
STARFIRE/DURALAN NETWORK DRIVER
M: Ion Badulescu <[email protected]>
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 <[email protected]>");
+MODULE_DESCRIPTION("StarFive JH7100 GPIO driver");
--
2.27.0
On Thu, Jul 1, 2021 at 8:23 AM Drew Fustini <[email protected]> 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 <[email protected]>
> Signed-off-by: Huan Feng <[email protected]>
> Signed-off-by: Drew Fustini <[email protected]>
> ---
> 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 <[email protected]>
> +M: Huan Feng <[email protected]>
> +L: [email protected]
> +L: [email protected]
> +F: Documentation/devicetree/bindings/gpio/starfive,jh7100-gpio.yaml
> +F: drivers/gpio/gpio-starfive-jh7100.c
> +
[snip]
Regards,
Bin
Hi Drew,
Am 2021-07-01 02:20, schrieb Drew Fustini:
> Add GPIO driver for the StarFive JH7100 SoC [1] used on the
> BeagleV Starlight JH7100 board [2].
>
> [1] https://github.com/starfive-tech/beaglev_doc/
> [2] https://github.com/beagleboard/beaglev-starlight
>
> Signed-off-by: Emil Renner Berthing <[email protected]>
> Signed-off-by: Huan Feng <[email protected]>
> Signed-off-by: Drew Fustini <[email protected]>
Could this driver use GPIO_REGMAP and REGMAP_IRQ? See
drivers/gpio/gpio-sl28cpld.c for an example.
-michael
Hi Drew,
On Thu, Jul 1, 2021 at 2:22 AM Drew Fustini <[email protected]> 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 <[email protected]>
> Signed-off-by: Huan Feng <[email protected]>
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 <[email protected]>
> + - Drew Fustini <[email protected]>
> +
> +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 -- [email protected]
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
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 <[email protected]>
> > Signed-off-by: Huan Feng <[email protected]>
> > Signed-off-by: Drew Fustini <[email protected]>
>
> Could this driver use GPIO_REGMAP and REGMAP_IRQ? See
> drivers/gpio/gpio-sl28cpld.c for an example.
>
> -michael
Thank you for the suggestion. I am not familiar with GPIO_REGMAP and
REGMAP_IRQ so I will read about it. Is the advantage is that is helps
to reduce code duplication by using an abstraction?
I did notice that the gpio-sifive.c driver used regmap_update_bits() and
regmap_write().
I suppose that is better than writel_relaxed() and iowrite32() which
this RFC driver does?
thanks,
drew
On Thu, Jul 01, 2021 at 10:25:12AM +0800, Bin Meng wrote:
> On Thu, Jul 1, 2021 at 8:23 AM Drew Fustini <[email protected]> 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 <[email protected]>
> > Signed-off-by: Huan Feng <[email protected]>
> > Signed-off-by: Drew Fustini <[email protected]>
> > ---
> > MAINTAINERS | 8 +
> > drivers/gpio/Kconfig | 8 +
> > drivers/gpio/Makefile | 1 +
> > drivers/gpio/gpio-starfive-jh7100.c | 425 ++++++++++++++++++++++++++++
> > 4 files changed, 442 insertions(+)
> > create mode 100644 drivers/gpio/gpio-starfive-jh7100.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index bc0ceef87b73..04fccc2ceffa 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -17423,6 +17423,14 @@ S: Supported
> > T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> > F: drivers/staging/
> >
> > +SIFVE JH7100 SOC GPIO DRIVER
>
> typo of SIFIVE, but it should be STARFIVE
Thank you! My eyes should have caught that.
-Drew
Hi Drew,
Am 2021-07-01 22:33, schrieb Drew Fustini:
> On Thu, Jul 01, 2021 at 08:39:40AM +0200, Michael Walle wrote:
>> Hi Drew,
>>
>> Am 2021-07-01 02:20, schrieb Drew Fustini:
>> > Add GPIO driver for the StarFive JH7100 SoC [1] used on the
>> > BeagleV Starlight JH7100 board [2].
>> >
>> > [1] https://github.com/starfive-tech/beaglev_doc/
>> > [2] https://github.com/beagleboard/beaglev-starlight
>> >
>> > Signed-off-by: Emil Renner Berthing <[email protected]>
>> > Signed-off-by: Huan Feng <[email protected]>
>> > Signed-off-by: Drew Fustini <[email protected]>
>>
>> Could this driver use GPIO_REGMAP and REGMAP_IRQ? See
>> drivers/gpio/gpio-sl28cpld.c for an example.
>>
>> -michael
>
> Thank you for the suggestion. I am not familiar with GPIO_REGMAP and
> REGMAP_IRQ so I will read about it. Is the advantage is that is helps
> to reduce code duplication by using an abstraction?
Yes, I've looked briefly at your patch and it seemed that GPIO_REGMAP
might fit here which will reduce code.
> I did notice that the gpio-sifive.c driver used regmap_update_bits()
> and
> regmap_write().
>
> I suppose that is better than writel_relaxed() and iowrite32() which
> this RFC driver does?
Its just another abstraction layer in between. For MMIO it will also
end up using some variant of the above (see regmap-mmio.c). But if you
use regmap, you can also use REGMAP_IRQ which might also be a fit
for your GPIO controller and thus don't have to implement your own
versions for the irq_chip ops.
-michael
On Thu, Jul 1, 2021 at 3:23 AM Drew Fustini <[email protected]> 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 <[email protected]>
> Signed-off-by: Huan Feng <[email protected]>
> Signed-off-by: Drew Fustini <[email protected]>
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
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 <[email protected]> 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 <[email protected]>
> > Signed-off-by: Huan Feng <[email protected]>
>
> 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 <[email protected]>
> > + - Drew Fustini <[email protected]>
> > +
> > +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
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 <[email protected]>
> > Signed-off-by: Huan Feng <[email protected]>
> > Signed-off-by: Drew Fustini <[email protected]>
>
> Could this driver use GPIO_REGMAP and REGMAP_IRQ? See
> drivers/gpio/gpio-sl28cpld.c for an example.
>
> -michael
I looked more at the example. Do you have a suggestion of how to handle
different types of interrupts?
This gpio controller can handle level triggered and edge triggered.
Edge triggered can be positve, negative or both. Level trigger can be
high or low.
Thanks,
Drew
Hi Drew,
On Fri, Jul 2, 2021 at 10:56 PM Drew Fustini <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Huan Feng <[email protected]>
> >
> > 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 <[email protected]>
> > > + - Drew Fustini <[email protected]>
> > > +
> > > +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 -- [email protected]
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
On Fri, Jul 02, 2021 at 07:03:19PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 1, 2021 at 3:23 AM Drew Fustini <[email protected]> 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 <[email protected]>
> > Signed-off-by: Huan Feng <[email protected]>
> > Signed-off-by: Drew Fustini <[email protected]>
>
> 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
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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > Signed-off-by: Huan Feng <[email protected]>
> > >
> > > 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 <[email protected]>
> > > > + - Drew Fustini <[email protected]>
> > > > +
> > > > +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
Hi Drew,
On Sat, Jul 3, 2021 at 8:46 AM Drew Fustini <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > Signed-off-by: Huan Feng <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > > + - Drew Fustini <[email protected]>
> > > > > +
> > > > > +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 -- [email protected]
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
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 <[email protected]>
>> 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 <[email protected]>
>> > Signed-off-by: Huan Feng <[email protected]>
>> > Signed-off-by: Drew Fustini <[email protected]>
>>
>> Seems some Co-developed-by are missing.
>
> Thank you for suggesting this. Huan Feng originally wrote the driver.
> Emil and I have made some changes to reorganize and clean it up for
> submission.
>
> Do you think all three of us should list Co-developed-by: for our names
> in addition to the SOB?
>
>> Brief look into the code brings the Q. Can't you utilize gpio-regmap
>> here? Why not?
>
> Michael Walle asked about this yesterday and it was my first time
> looking at regmap and gpio-regmap. I've been reading the code and it
> does look like I should try convert this driver over to using
> gpio-regmap.
>
> The open question in my mind is how to handle the interrupt type (edge
> trigged on positive or negative, level triggered on high or low).
> Hopefully I can find some other examples that can help me think about
> how to do that correctly.
Have a look at include/linux/regmap.h, there is "struct
regmap_irq_type".
If you're lucky, you can just supply the corresponding values that fits
your hardware. If it doesn't match your hardware at all, then you can
keep your own functions, or if its slightly different, then maybe you
can add support for your quirk in regmap-irq. You don't necessarily have
to use regmap-irq together with gpio-regmap. You can also just use
regmap-irq or gpio-regmap independently.
A quick grep for "type_rising_" lists drivers/mfd/max77650.c and
drivers/mfd/rohm-bd70528.c for example.
-michael
Hi deee Ho Drew, Michael, All
On Mon, 2021-07-05 at 15:29 +0200, Michael Walle wrote:
> Hi Drew,
>
> Am 2021-07-02 23:06, schrieb Drew Fustini:
> > On Fri, Jul 02, 2021 at 07:03:19PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jul 1, 2021 at 3:23 AM Drew Fustini <[email protected]
> > > >
> > > 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 <[email protected]>
> > > > Signed-off-by: Huan Feng <[email protected]>
> > > > Signed-off-by: Drew Fustini <[email protected]>
> > >
> > > Seems some Co-developed-by are missing.
> >
> > Thank you for suggesting this. Huan Feng originally wrote the
> > driver.
> > Emil and I have made some changes to reorganize and clean it up for
> > submission.
> >
> > Do you think all three of us should list Co-developed-by: for our
> > names
> > in addition to the SOB?
> >
> > > Brief look into the code brings the Q. Can't you utilize gpio-
> > > regmap
> > > here? Why not?
> >
> > Michael Walle asked about this yesterday and it was my first time
> > looking at regmap and gpio-regmap. I've been reading the code and
> > it
> > does look like I should try convert this driver over to using
> > gpio-regmap.
> >
> > The open question in my mind is how to handle the interrupt type
> > (edge
> > trigged on positive or negative, level triggered on high or low).
> > Hopefully I can find some other examples that can help me think
> > about
> > how to do that correctly.
> regmap_irq_type".
> If you're lucky, you can just supply the corresponding values that
> fits
> your hardware.
I added some level IRQ type-configuration support to regmap_irq back
when I wrote the BD70528 support. You should be able to just fill the
bit-mask indicating IRQ types supported by your GPIO controller
hardware, and then the corresponding type register values. As far as I
remember the supported types and values are given "per IRQ". If my
memory serves me right there was a limitation that the regmap-IRQ does
not distinguish setup where GPIO controller supports rising and falling
edges - but not both. That would have required adding another type
flag.
> If it doesn't match your hardware at all, then you can
> keep your own functions, or if its slightly different, then maybe you
> can add support for your quirk in regmap-irq. You don't necessarily
> have
> to use regmap-irq together with gpio-regmap. You can also just use
> regmap-irq or gpio-regmap independently.
>
> A quick grep for "type_rising_" lists drivers/mfd/max77650.c and
> drivers/mfd/rohm-bd70528.c for example.
The BD70528 has not been used too much and is scheduled for removal. It
may have received only limited testing but it *should* be functional
though.
Best Regards
Matti Vaittinen
Hi Drew
On Thu, Jul 1, 2021 at 8:25 AM Drew Fustini <[email protected]> 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 <[email protected]>
> Signed-off-by: Huan Feng <[email protected]>
> Signed-off-by: Drew Fustini <[email protected]>
> ---
> 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 <[email protected]>");
> +MODULE_DESCRIPTION("StarFive JH7100 GPIO driver");
> --
Regards
Ley Foon
On Thu, Jul 1, 2021 at 8:39 AM Michael Walle <[email protected]> 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 <[email protected]>
> > Signed-off-by: Huan Feng <[email protected]>
> > Signed-off-by: Drew Fustini <[email protected]>
>
> 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
On Fri, Jul 23, 2021 at 11:04:41PM +0200, Linus Walleij wrote:
> On Thu, Jul 1, 2021 at 8:39 AM Michael Walle <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Huan Feng <[email protected]>
> > > Signed-off-by: Drew Fustini <[email protected]>
> >
> > Could this driver use GPIO_REGMAP and REGMAP_IRQ? See
> > drivers/gpio/gpio-sl28cpld.c for an example.
>
> To me it looks just memory-mapped?
>
> Good old gpio-mmio.c (select GPIO_GENERIC) should
> suffice I think.
>
> Drew please look at drivers/gpio/gpio-ftgpio010.c for an example
> of GPIO_GENERIC calling bgpio_init() in probe().
Thank you for the suggestion. However, I am not sure that will work for
this SoC.
The GPIO registers are described in section 12 of JH7100 datasheet [1]
and I don't think they fit the expectation of gpio-mmio.c because there
is a seperate register for each GPIO line for output data value and
output enable.
There are 64 output data config registers which are 4 bytes wide. There
are 64 output enable config registers which are 4 bytes wide too. Output
data and output enable registers for a given GPIO pad are contiguous.
GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while GPIO1_DOUT_CFG
is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO pads is
effectively 8, which yields the formula: GPIOn_DOUT_CFG is 0x50+8n.
Similarly, GPIO0_DOEN_CFG is 0x54 and thus GPIOn_DOEN_CFG is 0x54+8n.
However, GPIO input data does use just one bit for each line. GPIODIN_0
at 0x48 covers GPIO[31:0] and GPIODIN_1 at 0x4c covers GPIO[63:32].
Thus the input could work with gpio-mmio but I am not sure how to
reconcile the register-per-gpio for the output value and output enable.
Is there way a way to adapt gpio-mmio for this situation?
Thanks,
Drew
[1] https://github.com/starfive-tech/beaglev_doc
Hi Drew, Hi Linus,
Am 2021-07-26 09:11, schrieb Drew Fustini:
> On Fri, Jul 23, 2021 at 11:04:41PM +0200, Linus Walleij wrote:
>> On Thu, Jul 1, 2021 at 8:39 AM Michael Walle <[email protected]> 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 <[email protected]>
>> > > Signed-off-by: Huan Feng <[email protected]>
>> > > Signed-off-by: Drew Fustini <[email protected]>
>> >
>> > Could this driver use GPIO_REGMAP and REGMAP_IRQ? See
>> > drivers/gpio/gpio-sl28cpld.c for an example.
>>
>> To me it looks just memory-mapped?
>>
>> Good old gpio-mmio.c (select GPIO_GENERIC) should
>> suffice I think.
But that doesn't mean gpio-regmap can't be used, no? Or what are
the advantages of gpio-mmio?
>> Drew please look at drivers/gpio/gpio-ftgpio010.c for an example
>> of GPIO_GENERIC calling bgpio_init() in probe().
>
> Thank you for the suggestion. However, I am not sure that will work for
> this SoC.
>
> The GPIO registers are described in section 12 of JH7100 datasheet [1]
> and I don't think they fit the expectation of gpio-mmio.c because there
> is a seperate register for each GPIO line for output data value and
> output enable.
>
> There are 64 output data config registers which are 4 bytes wide. There
> are 64 output enable config registers which are 4 bytes wide too.
> Output
> data and output enable registers for a given GPIO pad are contiguous.
> GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while GPIO1_DOUT_CFG
> is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO pads is
> effectively 8, which yields the formula: GPIOn_DOUT_CFG is 0x50+8n.
> Similarly, GPIO0_DOEN_CFG is 0x54 and thus GPIOn_DOEN_CFG is 0x54+8n.
>
> However, GPIO input data does use just one bit for each line. GPIODIN_0
> at 0x48 covers GPIO[31:0] and GPIODIN_1 at 0x4c covers GPIO[63:32].
I'd say, that should work with the .reg_mask_xlate of the gpio-regmap.
-michael
> Thus the input could work with gpio-mmio but I am not sure how to
> reconcile the register-per-gpio for the output value and output enable.
>
> Is there way a way to adapt gpio-mmio for this situation?
>
> Thanks,
> Drew
>
> [1] https://github.com/starfive-tech/beaglev_doc
On Mon, Jul 26, 2021 at 09:21:31AM +0200, Michael Walle wrote:
> Hi Drew, Hi Linus,
>
> Am 2021-07-26 09:11, schrieb Drew Fustini:
> > On Fri, Jul 23, 2021 at 11:04:41PM +0200, Linus Walleij wrote:
> > > On Thu, Jul 1, 2021 at 8:39 AM Michael Walle <[email protected]> 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 <[email protected]>
> > > > > Signed-off-by: Huan Feng <[email protected]>
> > > > > Signed-off-by: Drew Fustini <[email protected]>
> > > >
> > > > Could this driver use GPIO_REGMAP and REGMAP_IRQ? See
> > > > drivers/gpio/gpio-sl28cpld.c for an example.
> > >
> > > To me it looks just memory-mapped?
> > >
> > > Good old gpio-mmio.c (select GPIO_GENERIC) should
> > > suffice I think.
>
> But that doesn't mean gpio-regmap can't be used, no? Or what are
> the advantages of gpio-mmio?
>
> > > Drew please look at drivers/gpio/gpio-ftgpio010.c for an example
> > > of GPIO_GENERIC calling bgpio_init() in probe().
> >
> > Thank you for the suggestion. However, I am not sure that will work for
> > this SoC.
> >
> > The GPIO registers are described in section 12 of JH7100 datasheet [1]
> > and I don't think they fit the expectation of gpio-mmio.c because there
> > is a seperate register for each GPIO line for output data value and
> > output enable.
> >
> > There are 64 output data config registers which are 4 bytes wide. There
> > are 64 output enable config registers which are 4 bytes wide too. Output
> > data and output enable registers for a given GPIO pad are contiguous.
> > GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while GPIO1_DOUT_CFG
> > is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO pads is
> > effectively 8, which yields the formula: GPIOn_DOUT_CFG is 0x50+8n.
> > Similarly, GPIO0_DOEN_CFG is 0x54 and thus GPIOn_DOEN_CFG is 0x54+8n.
> >
> > However, GPIO input data does use just one bit for each line. GPIODIN_0
> > at 0x48 covers GPIO[31:0] and GPIODIN_1 at 0x4c covers GPIO[63:32].
>
> I'd say, that should work with the .reg_mask_xlate of the gpio-regmap.
>
> -michael
Thanks, yes, I think trying to figure out how .reg_mask_xlate would need
to work this SoC. I believe these are the only two implementations.
From drivers/gpio/gpio-regmap.c:
static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
unsigned int base, unsigned int offset,
unsigned int *reg, unsigned int *mask)
{
unsigned int line = offset % gpio->ngpio_per_reg;
unsigned int stride = offset / gpio->ngpio_per_reg;
*reg = base + stride * gpio->reg_stride;
*mask = BIT(line);
return 0;
}
From drivers/pinctrl/bcm/pinctrl-bcm63xx.c:
static int bcm63xx_reg_mask_xlate(struct gpio_regmap *gpio,
unsigned int base, unsigned int offset,
unsigned int *reg, unsigned int *mask)
{
unsigned int line = offset % BCM63XX_BANK_GPIOS;
unsigned int stride = offset / BCM63XX_BANK_GPIOS;
*reg = base - stride * BCM63XX_BANK_SIZE;
*mask = BIT(line);
return 0;
}
Let's say a driver calls gpio_regmap_set(chip, 0, 5) to set line 5 to
value 1.
I believe this would result in call to:
gpio->reg_mask_xlate(gpio, gpio->reg_set_base, 5, ®, &mask)
Then this would be called to set the register:
regmap_update_bits(gpio->regmap, reg, mask, mask);
From datasheet section 12 [1], there are 64 output data registers which
are 4 bytes wide. There are 64 output enable registers which are also 4
bytes wide too. Output data and output enable registers for a GPIO line
are contiguous. Thus GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54.
The forumla is GPIOn_DOUT_CFG is 0x50+8n and GPIOn_DOEN_CFG is 0x54+8n.
Thus for GPIO line 5:
GPIO5_DOUT_CFG is 0x50 + 0x28 = 0x78
GPIO5_DOEN_CFG is 0x54 + 0x28 = 0x7C
Enable GPIO line 5 as output by writing 0x1 to 0x7C and set output value
to 1 by writing 1 to 0x7C.
Using gpio_regmap_simple_xlate() as a template, I am thinking through
xlate for this gpio controller:
static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio,
unsigned int base, unsigned int offset,
unsigned int *reg, unsigned int *mask)
{
// reg_set_base is passed as base
// let reg_set_base = 0x50 (GPIO0_DOUT_CFG)
// let gpio->reg_stride = 8
// let offest = 5 (for gpio line 5)
*reg = base + offset * gpio->reg_stride;
// *reg = base:0x50 + offset:0x5 * reg_stride:0x8
// *reg = 0x50 + 0x28
// *reg= 0x78
// Each gpio line has a full register, not just a bit. To output
// a digital 1, then GPIO5_DOUT_CFG would be 0x1. To output
// digital 0, GPIO5_DOUT_CFG would be 0x0. Thus I think the mask
// should be the least significant bit.
*mask = BIT(1);
return 0;
}
Let's walk through what would happen if gpio_regmap_set() was the
caller:
static void gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
int val)
{
// for gpio line, offset = 5
// if want to set line 5 high, then val = 1
struct gpio_regmap *gpio = gpiochip_get_data(chip);
// reg_set_base would be set to 0x50 (GPIO0_DOUT_CFG)
unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
unsigned int reg, mask;
gpio->reg_mask_xlate(gpio, base /* 0x50 */, offset /* 5 */, ®, &mask);
if (val) /* if val is 1 */
regmap_update_bits(gpio->regmap, reg, mask, mask);
// if mask returned was 0x1, then this would set the
// bit 0 in GPIO5_DOUT_CFG
else /* if val is 0 */
regmap_update_bits(gpio->regmap, reg, mask, 0);
// if mask returned was 0x1, then this would clear
// bit 0 in GPIO5_DOUT_CFG
}
Now for the output enable register GPIO5_DOEN_CFG, the output driver is
active low so 0x0 is actually enables output where as 0x1 disables
output. Thus maybe I need to add logic like:
static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio,
unsigned int base, unsigned int offset,
unsigned int *reg, unsigned int *mask)
{
<snip>
if (base == GPIO0_DOUT_CFG)
*mask = 0x1U;
else if (base == GPIO0_DOEN_CFG)
*bit = ~(0x1U);
return 0;
}
What do you think of that approach?
Are there any other examples of regmap xlate that I missed?
Thanks,
Drew
[1] https://github.com/starfive-tech/beaglev_doc/blob/main/JH7100%20Data%20Sheet%20V01.01.04-EN%20(4-21-2021).pdf
Hi Drew,
Am 2021-07-27 07:28, schrieb Drew Fustini:
[..]
>> > > Drew please look at drivers/gpio/gpio-ftgpio010.c for an example
>> > > of GPIO_GENERIC calling bgpio_init() in probe().
>> >
>> > Thank you for the suggestion. However, I am not sure that will work for
>> > this SoC.
>> >
>> > The GPIO registers are described in section 12 of JH7100 datasheet [1]
>> > and I don't think they fit the expectation of gpio-mmio.c because there
>> > is a seperate register for each GPIO line for output data value and
>> > output enable.
>> >
>> > There are 64 output data config registers which are 4 bytes wide. There
>> > are 64 output enable config registers which are 4 bytes wide too. Output
>> > data and output enable registers for a given GPIO pad are contiguous.
>> > GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while GPIO1_DOUT_CFG
>> > is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO pads is
>> > effectively 8, which yields the formula: GPIOn_DOUT_CFG is 0x50+8n.
>> > Similarly, GPIO0_DOEN_CFG is 0x54 and thus GPIOn_DOEN_CFG is 0x54+8n.
>> >
>> > However, GPIO input data does use just one bit for each line. GPIODIN_0
>> > at 0x48 covers GPIO[31:0] and GPIODIN_1 at 0x4c covers GPIO[63:32].
Mh, I'm not sure I'm understanding the datasheet/registers. _DOUT_CFG
and _DOEN_CFG seem to specify the pad where this GPIO is mapped to.
Shouldn't this be some kind of pinctrl then? Apparently you can map
any GPIO number to any output pad, no? Or at least to all pads
which are described in Table 11-2. What happens if two different GPIOs
are mapped to the same pad? Bit 31 in these _CFG seems to be an invert
bit, but what does it invert?
Similar, the input GPIOs are connected to an output pad by all the
GPI_*_CFG registers.
To me it seems, that there two multiplexers for each GPIO, where
you can connect any GPIOn to any input pad and output pad. Sound
like a huge overkill. I must be missing something here.
But what puzzles me the most, where do I set the actual GPIO output
value?
>> I'd say, that should work with the .reg_mask_xlate of the gpio-regmap.
>>
>> -michael
>
> Thanks, yes, I think trying to figure out how .reg_mask_xlate would
> need
> to work this SoC. I believe these are the only two implementations.
>
> From drivers/gpio/gpio-regmap.c:
>
> static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
> unsigned int base, unsigned int offset,
> unsigned int *reg, unsigned int *mask)
> {
> unsigned int line = offset % gpio->ngpio_per_reg;
> unsigned int stride = offset / gpio->ngpio_per_reg;
>
> *reg = base + stride * gpio->reg_stride;
> *mask = BIT(line);
>
> return 0;
> }
>
> From drivers/pinctrl/bcm/pinctrl-bcm63xx.c:
>
> static int bcm63xx_reg_mask_xlate(struct gpio_regmap *gpio,
> unsigned int base, unsigned int offset,
> unsigned int *reg, unsigned int *mask)
> {
> unsigned int line = offset % BCM63XX_BANK_GPIOS;
> unsigned int stride = offset / BCM63XX_BANK_GPIOS;
>
> *reg = base - stride * BCM63XX_BANK_SIZE;
> *mask = BIT(line);
>
> return 0;
> }
>
> Let's say a driver calls gpio_regmap_set(chip, 0, 5) to set line 5 to
> value 1.
>
> I believe this would result in call to:
>
> gpio->reg_mask_xlate(gpio, gpio->reg_set_base, 5, ®, &mask)
>
> Then this would be called to set the register:
>
> regmap_update_bits(gpio->regmap, reg, mask, mask);
>
> From datasheet section 12 [1], there are 64 output data registers which
> are 4 bytes wide. There are 64 output enable registers which are also 4
> bytes wide too. Output data and output enable registers for a GPIO line
> are contiguous. Thus GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54.
> The forumla is GPIOn_DOUT_CFG is 0x50+8n and GPIOn_DOEN_CFG is 0x54+8n.
> Thus for GPIO line 5:
>
> GPIO5_DOUT_CFG is 0x50 + 0x28 = 0x78
> GPIO5_DOEN_CFG is 0x54 + 0x28 = 0x7C
>
> Enable GPIO line 5 as output by writing 0x1 to 0x7C and set output
> value
> to 1 by writing 1 to 0x7C.
>
> Using gpio_regmap_simple_xlate() as a template, I am thinking through
> xlate for this gpio controller:
>
>
> static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio,
> unsigned int base, unsigned int offset,
> unsigned int *reg, unsigned int *mask)
> {
> // reg_set_base is passed as base
> // let reg_set_base = 0x50 (GPIO0_DOUT_CFG)
> // let gpio->reg_stride = 8
> // let offest = 5 (for gpio line 5)
>
> *reg = base + offset * gpio->reg_stride;
> // *reg = base:0x50 + offset:0x5 * reg_stride:0x8
> // *reg = 0x50 + 0x28
> // *reg= 0x78
>
> // Each gpio line has a full register, not just a bit. To output
> // a digital 1, then GPIO5_DOUT_CFG would be 0x1. To output
> // digital 0, GPIO5_DOUT_CFG would be 0x0. Thus I think the mask
> // should be the least significant bit.
> *mask = BIT(1);
>
> return 0;
> }
>
> Let's walk through what would happen if gpio_regmap_set() was the
> caller:
>
> static void gpio_regmap_set(struct gpio_chip *chip, unsigned int
> offset,
> int val)
> {
> // for gpio line, offset = 5
> // if want to set line 5 high, then val = 1
> struct gpio_regmap *gpio = gpiochip_get_data(chip);
>
> // reg_set_base would be set to 0x50 (GPIO0_DOUT_CFG)
> unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
> unsigned int reg, mask;
>
> gpio->reg_mask_xlate(gpio, base /* 0x50 */, offset /* 5 */, ®,
> &mask);
> if (val) /* if val is 1 */
> regmap_update_bits(gpio->regmap, reg, mask, mask);
> // if mask returned was 0x1, then this would set the
> // bit 0 in GPIO5_DOUT_CFG
> else /* if val is 0 */
> regmap_update_bits(gpio->regmap, reg, mask, 0);
> // if mask returned was 0x1, then this would clear
> // bit 0 in GPIO5_DOUT_CFG
> }
>
> Now for the output enable register GPIO5_DOEN_CFG, the output driver is
> active low so 0x0 is actually enables output where as 0x1 disables
> output. Thus maybe I need to add logic like:
>
>
> static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio,
> unsigned int base, unsigned int offset,
> unsigned int *reg, unsigned int *mask)
> {
> <snip>
> if (base == GPIO0_DOUT_CFG)
> *mask = 0x1U;
> else if (base == GPIO0_DOEN_CFG)
> *bit = ~(0x1U);
>
> return 0;
> }
>
> What do you think of that approach?
I'm also not opposed to add a new flag to gpio-regmap which
invert the value itself.
But the idea was that you can differentiate in _xlate() by the
base register offset, like you already did:
static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio,
unsigned int base, unsigned int offset,
unsigned int *reg, unsigned int *mask)
{
switch (base) {
case GPIO0_DOUT_CFG:
/* do some custom mapping just for DOUT_CFG */
case GPIO0_DOEN_CFG:
/* do some custom mapping just for DOEN_CFG */
default:
/* do normal mapping */
}
> Are there any other examples of regmap xlate that I missed?
No there aren't much yet. Usually the simple one is enough.
-michael
> [1]
> https://github.com/starfive-tech/beaglev_doc/blob/main/JH7100%20Data%20Sheet%20V01.01.04-EN%20(4-21-2021).pdf
On Wed, 28 Jul 2021 at 11:49, Michael Walle <[email protected]> wrote:
> Hi Drew,
> Am 2021-07-27 07:28, schrieb Drew Fustini:
> [..]
> >> > > Drew please look at drivers/gpio/gpio-ftgpio010.c for an example
> >> > > of GPIO_GENERIC calling bgpio_init() in probe().
> >> >
> >> > Thank you for the suggestion. However, I am not sure that will work for
> >> > this SoC.
> >> >
> >> > The GPIO registers are described in section 12 of JH7100 datasheet [1]
> >> > and I don't think they fit the expectation of gpio-mmio.c because there
> >> > is a seperate register for each GPIO line for output data value and
> >> > output enable.
> >> >
> >> > There are 64 output data config registers which are 4 bytes wide. There
> >> > are 64 output enable config registers which are 4 bytes wide too. Output
> >> > data and output enable registers for a given GPIO pad are contiguous.
> >> > GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while GPIO1_DOUT_CFG
> >> > is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO pads is
> >> > effectively 8, which yields the formula: GPIOn_DOUT_CFG is 0x50+8n.
> >> > Similarly, GPIO0_DOEN_CFG is 0x54 and thus GPIOn_DOEN_CFG is 0x54+8n.
> >> >
> >> > However, GPIO input data does use just one bit for each line. GPIODIN_0
> >> > at 0x48 covers GPIO[31:0] and GPIODIN_1 at 0x4c covers GPIO[63:32].
>
> Mh, I'm not sure I'm understanding the datasheet/registers. _DOUT_CFG
> and _DOEN_CFG seem to specify the pad where this GPIO is mapped to.
> Shouldn't this be some kind of pinctrl then? Apparently you can map
> any GPIO number to any output pad, no? Or at least to all pads
> which are described in Table 11-2. What happens if two different GPIOs
> are mapped to the same pad? Bit 31 in these _CFG seems to be an invert
> bit, but what does it invert?
>
> Similar, the input GPIOs are connected to an output pad by all the
> GPI_*_CFG registers.
>
> To me it seems, that there two multiplexers for each GPIO, where
> you can connect any GPIOn to any input pad and output pad. Sound
> like a huge overkill. I must be missing something here.
>
> But what puzzles me the most, where do I set the actual GPIO output
> value?
Yeah, it's a little confusing. The DOUT registers choose between a number of
signals from various peripherals to control the output value of the
pin. Similarly
the DOEN registers chose between a number of signals to control the output
enable of the pin. However, two of those signals are special in that they are
constant 0 or constant 1. This is how you control the output value and output
enable from software like a regular GPIO.
You're completely right though. This ought to be managed by a proper pinctrl
driver, and I'm working on one here:
https://github.com/esmil/linux/commits/beaglev-pinctrl
> >> I'd say, that should work with the .reg_mask_xlate of the gpio-regmap.
> >>
> >> -michael
> >
> > Thanks, yes, I think trying to figure out how .reg_mask_xlate would
> > need
> > to work this SoC. I believe these are the only two implementations.
> >
> > From drivers/gpio/gpio-regmap.c:
> >
> > static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
> > unsigned int base, unsigned int offset,
> > unsigned int *reg, unsigned int *mask)
> > {
> > unsigned int line = offset % gpio->ngpio_per_reg;
> > unsigned int stride = offset / gpio->ngpio_per_reg;
> >
> > *reg = base + stride * gpio->reg_stride;
> > *mask = BIT(line);
> >
> > return 0;
> > }
> >
> > From drivers/pinctrl/bcm/pinctrl-bcm63xx.c:
> >
> > static int bcm63xx_reg_mask_xlate(struct gpio_regmap *gpio,
> > unsigned int base, unsigned int offset,
> > unsigned int *reg, unsigned int *mask)
> > {
> > unsigned int line = offset % BCM63XX_BANK_GPIOS;
> > unsigned int stride = offset / BCM63XX_BANK_GPIOS;
> >
> > *reg = base - stride * BCM63XX_BANK_SIZE;
> > *mask = BIT(line);
> >
> > return 0;
> > }
> >
> > Let's say a driver calls gpio_regmap_set(chip, 0, 5) to set line 5 to
> > value 1.
> >
> > I believe this would result in call to:
> >
> > gpio->reg_mask_xlate(gpio, gpio->reg_set_base, 5, ®, &mask)
> >
> > Then this would be called to set the register:
> >
> > regmap_update_bits(gpio->regmap, reg, mask, mask);
> >
> > From datasheet section 12 [1], there are 64 output data registers which
> > are 4 bytes wide. There are 64 output enable registers which are also 4
> > bytes wide too. Output data and output enable registers for a GPIO line
> > are contiguous. Thus GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54.
> > The forumla is GPIOn_DOUT_CFG is 0x50+8n and GPIOn_DOEN_CFG is 0x54+8n.
> > Thus for GPIO line 5:
> >
> > GPIO5_DOUT_CFG is 0x50 + 0x28 = 0x78
> > GPIO5_DOEN_CFG is 0x54 + 0x28 = 0x7C
> >
> > Enable GPIO line 5 as output by writing 0x1 to 0x7C and set output
> > value
> > to 1 by writing 1 to 0x7C.
> >
> > Using gpio_regmap_simple_xlate() as a template, I am thinking through
> > xlate for this gpio controller:
> >
> >
> > static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio,
> > unsigned int base, unsigned int offset,
> > unsigned int *reg, unsigned int *mask)
> > {
> > // reg_set_base is passed as base
> > // let reg_set_base = 0x50 (GPIO0_DOUT_CFG)
> > // let gpio->reg_stride = 8
> > // let offest = 5 (for gpio line 5)
> >
> > *reg = base + offset * gpio->reg_stride;
> > // *reg = base:0x50 + offset:0x5 * reg_stride:0x8
> > // *reg = 0x50 + 0x28
> > // *reg= 0x78
> >
> > // Each gpio line has a full register, not just a bit. To output
> > // a digital 1, then GPIO5_DOUT_CFG would be 0x1. To output
> > // digital 0, GPIO5_DOUT_CFG would be 0x0. Thus I think the mask
> > // should be the least significant bit.
> > *mask = BIT(1);
> >
> > return 0;
> > }
> >
> > Let's walk through what would happen if gpio_regmap_set() was the
> > caller:
> >
> > static void gpio_regmap_set(struct gpio_chip *chip, unsigned int
> > offset,
> > int val)
> > {
> > // for gpio line, offset = 5
> > // if want to set line 5 high, then val = 1
> > struct gpio_regmap *gpio = gpiochip_get_data(chip);
> >
> > // reg_set_base would be set to 0x50 (GPIO0_DOUT_CFG)
> > unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
> > unsigned int reg, mask;
> >
> > gpio->reg_mask_xlate(gpio, base /* 0x50 */, offset /* 5 */, ®,
> > &mask);
> > if (val) /* if val is 1 */
> > regmap_update_bits(gpio->regmap, reg, mask, mask);
> > // if mask returned was 0x1, then this would set the
> > // bit 0 in GPIO5_DOUT_CFG
> > else /* if val is 0 */
> > regmap_update_bits(gpio->regmap, reg, mask, 0);
> > // if mask returned was 0x1, then this would clear
> > // bit 0 in GPIO5_DOUT_CFG
> > }
> >
> > Now for the output enable register GPIO5_DOEN_CFG, the output driver is
> > active low so 0x0 is actually enables output where as 0x1 disables
> > output. Thus maybe I need to add logic like:
> >
> >
> > static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio,
> > unsigned int base, unsigned int offset,
> > unsigned int *reg, unsigned int *mask)
> > {
> > <snip>
> > if (base == GPIO0_DOUT_CFG)
> > *mask = 0x1U;
> > else if (base == GPIO0_DOEN_CFG)
> > *bit = ~(0x1U);
> >
> > return 0;
> > }
> >
> > What do you think of that approach?
>
> I'm also not opposed to add a new flag to gpio-regmap which
> invert the value itself.
>
> But the idea was that you can differentiate in _xlate() by the
> base register offset, like you already did:
>
> static int gpio_regmap_starfive_xlate(struct gpio_regmap *gpio,
> unsigned int base, unsigned int offset,
> unsigned int *reg, unsigned int *mask)
> {
> switch (base) {
> case GPIO0_DOUT_CFG:
> /* do some custom mapping just for DOUT_CFG */
> case GPIO0_DOEN_CFG:
> /* do some custom mapping just for DOEN_CFG */
> default:
> /* do normal mapping */
> }
>
> > Are there any other examples of regmap xlate that I missed?
>
> No there aren't much yet. Usually the simple one is enough.
>
> -michael
>
> > [1]
> > https://github.com/starfive-tech/beaglev_doc/blob/main/JH7100%20Data%20Sheet%20V01.01.04-EN%20(4-21-2021).pdf
>
Am 2021-07-28 12:59, schrieb Emil Renner Berthing:
> On Wed, 28 Jul 2021 at 11:49, Michael Walle <[email protected]> wrote:
>> Hi Drew,
>> Am 2021-07-27 07:28, schrieb Drew Fustini:
>> [..]
>> >> > > Drew please look at drivers/gpio/gpio-ftgpio010.c for an example
>> >> > > of GPIO_GENERIC calling bgpio_init() in probe().
>> >> >
>> >> > Thank you for the suggestion. However, I am not sure that will work for
>> >> > this SoC.
>> >> >
>> >> > The GPIO registers are described in section 12 of JH7100 datasheet [1]
>> >> > and I don't think they fit the expectation of gpio-mmio.c because there
>> >> > is a seperate register for each GPIO line for output data value and
>> >> > output enable.
>> >> >
>> >> > There are 64 output data config registers which are 4 bytes wide. There
>> >> > are 64 output enable config registers which are 4 bytes wide too. Output
>> >> > data and output enable registers for a given GPIO pad are contiguous.
>> >> > GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while GPIO1_DOUT_CFG
>> >> > is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO pads is
>> >> > effectively 8, which yields the formula: GPIOn_DOUT_CFG is 0x50+8n.
>> >> > Similarly, GPIO0_DOEN_CFG is 0x54 and thus GPIOn_DOEN_CFG is 0x54+8n.
>> >> >
>> >> > However, GPIO input data does use just one bit for each line. GPIODIN_0
>> >> > at 0x48 covers GPIO[31:0] and GPIODIN_1 at 0x4c covers GPIO[63:32].
>>
>> Mh, I'm not sure I'm understanding the datasheet/registers. _DOUT_CFG
>> and _DOEN_CFG seem to specify the pad where this GPIO is mapped to.
>> Shouldn't this be some kind of pinctrl then? Apparently you can map
>> any GPIO number to any output pad, no? Or at least to all pads
>> which are described in Table 11-2. What happens if two different GPIOs
>> are mapped to the same pad? Bit 31 in these _CFG seems to be an invert
>> bit, but what does it invert?
>>
>> Similar, the input GPIOs are connected to an output pad by all the
>> GPI_*_CFG registers.
>>
>> To me it seems, that there two multiplexers for each GPIO, where
>> you can connect any GPIOn to any input pad and output pad. Sound
>> like a huge overkill. I must be missing something here.
>>
>> But what puzzles me the most, where do I set the actual GPIO output
>> value?
>
> Yeah, it's a little confusing. The DOUT registers choose between a
> number of
> signals from various peripherals to control the output value of the
> pin. Similarly
> the DOEN registers chose between a number of signals to control the
> output
> enable of the pin. However, two of those signals are special in that
> they are
> constant 0 or constant 1. This is how you control the output value and
> output
> enable from software like a regular GPIO.
>
> You're completely right though. This ought to be managed by a proper
> pinctrl
> driver, and I'm working on one here:
> https://github.com/esmil/linux/commits/beaglev-pinctrl
Ahh, I see. So for the non-gpio function you have to set a value other
than 0 or 1, correct?
And as an implementation detail you have to set the corresponding OE
pin if the non-gpio function will need a tristate pin (or whatever).
So, the _DOUT_CFG will actually be shared between the pinctrl and the
gpio driver, right? (I haven't done anything with pinctrl, so this might
be a stupid question).
-michael
On Wed, 28 Jul 2021 at 13:19, Michael Walle <[email protected]> wrote:
> Am 2021-07-28 12:59, schrieb Emil Renner Berthing:
> > On Wed, 28 Jul 2021 at 11:49, Michael Walle <[email protected]> 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