[PATCH 0/2] gpio: add support for GPIO controller on Siflower SoCs

[PATCH 0/2] gpio: add support for GPIO controller on Siflower SoCs

[Chuanhong Guo]

This patchset adds support for the GPIO controller found on various
Siflower MIPS and RISC-V SoCs including SF19A2890, SF21A6826 and
SF21H8898.

Chuanhong Guo (1):
  dt-bindings: gpio: add binding doc for siflower,sf19a2890-gpio

Qingfang Deng (1):
  gpio: add support for GPIO controller on Siflower SoCs

 .../gpio/siflower,sf19a2890-gpio.yaml         |  83 +++++
 drivers/gpio/Kconfig                          |   8 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-siflower.c                  | 347 ++++++++++++++++++
 4 files changed, 439 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/siflower,sf19a2890-gpio.yaml
 create mode 100644 drivers/gpio/gpio-siflower.c

-- 
2.47.1

[PATCH 1/2] dt-bindings: gpio: add binding doc for siflower,sf19a2890-gpio

[Chuanhong Guo]

Add dt binding doc for the GPIO controller found on Siflower SF19A2890
and various other Siflower MIPS and RISC-V SoCs.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---
 .../gpio/siflower,sf19a2890-gpio.yaml         | 83 +++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/siflower,sf19a2890-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/siflower,sf19a2890-gpio.yaml b/Documentation/devicetree/bindings/gpio/siflower,sf19a2890-gpio.yaml
new file mode 100644
index 000000000000..7dab1e3f159c
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/siflower,sf19a2890-gpio.yaml
@@ -0,0 +1,83 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/siflower,sf19a2890-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Siflower SF19A2890 GPIO controller
+
+maintainers:
+  - Chuanhong Guo <gch981213@gmail.com>
+
+properties:
+  compatible:
+    const: siflower,sf19a2890-gpio
+
+  gpio-controller: true
+  "#gpio-cells":
+    const: 2
+
+  gpio-ranges: true
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    description:
+      Interrupt mapping, one interrupt per 16 GPIOs.
+    minItems: 1
+    maxItems: 10
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  ngpios:
+    description:
+      The number of GPIOs available on the controller implementation.
+    minimum: 1
+
+required:
+  - compatible
+  - clocks
+  - gpio-controller
+  - gpio-ranges
+  - interrupt-controller
+  - interrupts
+  - ngpios
+  - reg
+  - resets
+  - "#gpio-cells"
+  - "#interrupt-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/mips-gic.h>
+    gpio@19d00000 {
+      compatible = "siflower,sf19a2890-gpio";
+      reg = <0x19d00000 0x100000>;
+      interrupts = <GIC_SHARED 246 IRQ_TYPE_LEVEL_HIGH>,
+        <GIC_SHARED 247 IRQ_TYPE_LEVEL_HIGH>,
+        <GIC_SHARED 248 IRQ_TYPE_LEVEL_HIGH>,
+        <GIC_SHARED 249 IRQ_TYPE_LEVEL_HIGH>;
+      clocks = <&gpioclk 0>;
+      resets = <&gpiorst 0>;
+
+      gpio-controller;
+      #gpio-cells = <2>;
+      ngpios = <49>;
+      gpio-ranges = <&pinctrl 0 0 49>;
+
+      interrupt-controller;
+      #interrupt-cells = <2>;
+    };
-- 
2.47.1

Re: [PATCH 1/2] dt-bindings: gpio: add binding doc for siflower,sf19a2890-gpio

[Linus Walleij]

Hi Chuanhong,

thanks for your patch!

On Wed, Dec 25, 2024 at 4:59 AM Chuanhong Guo <gch981213@gmail.com> wrote:

> Add dt binding doc for the GPIO controller found on Siflower SF19A2890
> and various other Siflower MIPS and RISC-V SoCs.
>
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
(...)
> +  interrupts:
> +    description:
> +      Interrupt mapping, one interrupt per 16 GPIOs.

So from the driver it is very clear that this is lumping together several
GPIO blocks with 16 GPIOs in each into a bigger GPIO controller, despite
the instances are identical. They even each have an individual IRQ.

> +  ngpios:
> +    description:
> +      The number of GPIOs available on the controller implementation.
> +    minimum: 1

I would say minimum: 1 maximum: 16 default: 16.

One instance per block/bank.

> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/mips-gic.h>
> +    gpio@19d00000 {
> +      compatible = "siflower,sf19a2890-gpio";
> +      reg = <0x19d00000 0x100000>;

Just use 4 instances. Since (looking at the driver) it seems there
is an IRQ register that is "off the bulk" I would do something like:

instance 0:

reg = <0x19d00000 0x40>, <0x19d04000 4>;

instance: 1:

reg = <0x19d00040 0x40>, <0x19d04004 4>;

(...etc...)

You can add reg-names if you don't want the implicit order
of registers. (Perhaps the bindings maintainers will push for this
as well.)

> +      interrupts = <GIC_SHARED 246 IRQ_TYPE_LEVEL_HIGH>,
> +        <GIC_SHARED 247 IRQ_TYPE_LEVEL_HIGH>,
> +        <GIC_SHARED 248 IRQ_TYPE_LEVEL_HIGH>,
> +        <GIC_SHARED 249 IRQ_TYPE_LEVEL_HIGH>;

Just one IRQ and handle just one block per instance.

> +      clocks = <&gpioclk 0>;
> +      resets = <&gpiorst 0>;
> +
> +      gpio-controller;
> +      #gpio-cells = <2>;
> +      ngpios = <49>;

Just omit this on instances 0,1,2 and set to 1 on
instance 3.

> +      gpio-ranges = <&pinctrl 0 0 49>;

Augment this accordingly to one instance per bank/range.

Yours,
Linus Walleij

Re: [PATCH 1/2] dt-bindings: gpio: add binding doc for siflower,sf19a2890-gpio

[Chuanhong Guo]

Hello!

On Sat, Dec 28, 2024 at 12:45 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Chuanhong,
>
> thanks for your patch!
>
> On Wed, Dec 25, 2024 at 4:59 AM Chuanhong Guo <gch981213@gmail.com> wrote:
>
> > Add dt binding doc for the GPIO controller found on Siflower SF19A2890
> > and various other Siflower MIPS and RISC-V SoCs.
> >
> > Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> (...)
> > +  interrupts:
> > +    description:
> > +      Interrupt mapping, one interrupt per 16 GPIOs.
>
> So from the driver it is very clear that this is lumping together several
> GPIO blocks with 16 GPIOs in each into a bigger GPIO controller, despite
> the instances are identical. They even each have an individual IRQ.
>
> > +  ngpios:
> > +    description:
> > +      The number of GPIOs available on the controller implementation.
> > +    minimum: 1
>
> I would say minimum: 1 maximum: 16 default: 16.
>
> One instance per block/bank.
>
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/interrupt-controller/mips-gic.h>
> > +    gpio@19d00000 {
> > +      compatible = "siflower,sf19a2890-gpio";
> > +      reg = <0x19d00000 0x100000>;
>
> Just use 4 instances. Since (looking at the driver) it seems there
> is an IRQ register that is "off the bulk" I would do something like:
>
> instance 0:
>
> reg = <0x19d00000 0x40>, <0x19d04000 4>;
>
> instance: 1:
>
> reg = <0x19d00040 0x40>, <0x19d04004 4>;
>
> (...etc...)

Actually, this weird GPIO controller uses one 0x40 block per pin (The
49 pin controller uses 0x40 * 49 or about 3K memory space just for IO
registers!), and they share a single reset signal from the reset
controller.
I don't know how one reset could be shared across multiple platform
devices so I don't think this kind of split is possible.

--
Regards,
Chuanhong Guo

Re: [PATCH 1/2] dt-bindings: gpio: add binding doc for siflower,sf19a2890-gpio

[Linus Walleij]

On Sun, Dec 29, 2024 at 4:06 AM Chuanhong Guo <gch981213@gmail.com> wrote:

> I don't know how one reset could be shared across multiple platform
> devices so I don't think this kind of split is possible.

In general I don't think shared reset lines in silicon is
any problem whatsoever, they are reference counted,
just like clocks or regulators, and there is a flag
RESET_CONTROL_FLAGS_BIT_SHARED
especially for this purpose.

You might have to fix the reset driver, or fix up the way
they are defined in the device tree, I don't know how,
but certainly it is possible. If in doubt consult the reset
controller maintainer. (Philipp.)

Yours,
Linus Walleij

[PATCH 2/2] gpio: add support for GPIO controller on Siflower SoCs

[Chuanhong Guo]

From: Qingfang Deng <qingfang.deng@siflower.com.cn>

Add a driver for the GPIO controller on Siflower SoCs.
This controller is found on all current Siflower MIPS and RISC-V
chips including SF19A2890, SF21A6826 and SF21H8898.

Signed-off-by: Qingfang Deng <qingfang.deng@siflower.com.cn>
Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
---
 drivers/gpio/Kconfig         |   9 +
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-siflower.c | 353 +++++++++++++++++++++++++++++++++++
 3 files changed, 363 insertions(+)
 create mode 100644 drivers/gpio/gpio-siflower.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index add5ad29a673..fdc9a89ffbf3 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -637,6 +637,15 @@ config GPIO_SIFIVE
 	help
 	  Say yes here to support the GPIO device on SiFive SoCs.
 
+config GPIO_SIFLOWER
+	tristate "SiFlower GPIO support"
+	depends on OF_GPIO
+	depends on MIPS || RISCV || COMPILE_TEST
+	select GPIOLIB_IRQCHIP
+	help
+	  GPIO controller driver for SiFlower MIPS and RISC-V SoCs
+	  including SF19A2890, SF21A6826 and SF21H8898.
+
 config GPIO_SIOX
 	tristate "SIOX GPIO support"
 	depends on SIOX
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index af3ba4d81b58..ec400ed6dcd1 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -151,6 +151,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_SIFLOWER)		+= gpio-siflower.o
 obj-$(CONFIG_GPIO_SIM)			+= gpio-sim.o
 obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
 obj-$(CONFIG_GPIO_SL28CPLD)		+= gpio-sl28cpld.o
diff --git a/drivers/gpio/gpio-siflower.c b/drivers/gpio/gpio-siflower.c
new file mode 100644
index 000000000000..bd8002ee127d
--- /dev/null
+++ b/drivers/gpio/gpio-siflower.c
@@ -0,0 +1,353 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * GPIO controller driver for Siflower MIPS and RISC-V SoCs including:
+ *  Siflower SF19A2890
+ *  Siflower SF21A6826
+ *  Siflower SF21H8898
+ *
+ */
+#include <linux/pinctrl/consumer.h>
+#include <linux/clk.h>
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <asm/div64.h>
+
+#define GPIO_IR(n)	(0x40 * (n) + 0x00)
+#define GPIO_OR(n)	(0x40 * (n) + 0x04)
+#define GPIO_OEN(n)	(0x40 * (n) + 0x08)
+#define GPIO_IMR(n)	(0x40 * (n) + 0x0c)
+#define GPIO_GPIMR(n)	(0x40 * (n) + 0x10)
+#define GPIO_PIR(n)	(0x40 * (n) + 0x14)
+#define GPIO_ITR(n)	(0x40 * (n) + 0x18)
+#define GPIO_IFR(n)	(0x40 * (n) + 0x1c)
+#define GPIO_ICR(n)	(0x40 * (n) + 0x20)
+#define GPIO_GPxIR(n)	(0x4 * (n) + 0x4000)
+
+#define GPIOS_PER_GROUP	16
+
+struct sf_gpio_priv {
+	struct gpio_chip gc;
+	void __iomem *base;
+	struct clk *clk;
+	struct reset_control *rstc;
+	unsigned int irq[];
+};
+
+#define to_sf_gpio(x)	container_of(x, struct sf_gpio_priv, gc)
+
+static u32 sf_gpio_rd(struct sf_gpio_priv *priv, unsigned long reg)
+{
+	return readl_relaxed(priv->base + reg);
+}
+
+static void sf_gpio_wr(struct sf_gpio_priv *priv, unsigned long reg,
+		       u32 val)
+{
+	writel_relaxed(val, priv->base + reg);
+}
+
+static int sf_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
+{
+	struct sf_gpio_priv *priv = to_sf_gpio(gc);
+
+	return sf_gpio_rd(priv, GPIO_IR(offset));
+}
+
+static void sf_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
+			      int value)
+{
+	struct sf_gpio_priv *priv = to_sf_gpio(gc);
+
+	sf_gpio_wr(priv, GPIO_OR(offset), value);
+}
+
+static int sf_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct sf_gpio_priv *priv = to_sf_gpio(gc);
+
+	if (sf_gpio_rd(priv, GPIO_OEN(offset)))
+		return GPIO_LINE_DIRECTION_IN;
+	else
+		return GPIO_LINE_DIRECTION_OUT;
+}
+
+static int sf_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+	struct sf_gpio_priv *priv = to_sf_gpio(gc);
+
+	sf_gpio_wr(priv, GPIO_OEN(offset), 1);
+	return 0;
+}
+
+static int sf_gpio_direction_output(struct gpio_chip *gc, unsigned int offset,
+				    int value)
+{
+	struct sf_gpio_priv *priv = to_sf_gpio(gc);
+
+	sf_gpio_wr(priv, GPIO_OR(offset), value);
+	sf_gpio_wr(priv, GPIO_OEN(offset), 0);
+	return 0;
+}
+
+static int sf_gpio_set_debounce(struct gpio_chip *gc, unsigned int offset,
+				u32 debounce)
+{
+	struct sf_gpio_priv *priv = to_sf_gpio(gc);
+	unsigned long freq = clk_get_rate(priv->clk);
+	u64 mul;
+
+	/* (ICR + 1) * IFR = debounce_us * clkfreq_mhz / 4 */
+	mul = (u64)debounce * freq;
+	do_div(mul, 1000000 * 4);
+	if (mul > 0xff00)
+		return -EINVAL;
+
+	sf_gpio_wr(priv, GPIO_ICR(offset), 0xff);
+	sf_gpio_wr(priv, GPIO_IFR(offset), DIV_ROUND_UP(mul, 0x100));
+
+	return 0;
+}
+
+static int sf_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
+			      unsigned long config)
+{
+	switch (pinconf_to_config_param(config)) {
+	case PIN_CONFIG_INPUT_DEBOUNCE:
+		return sf_gpio_set_debounce(gc, offset,
+			pinconf_to_config_argument(config));
+	default:
+		return gpiochip_generic_config(gc, offset, config);
+	}
+}
+
+static void sf_gpio_irq_ack(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct sf_gpio_priv *priv = to_sf_gpio(gc);
+	unsigned long offset = irqd_to_hwirq(data);
+
+	sf_gpio_wr(priv, GPIO_PIR(offset), 0);
+}
+
+static void sf_gpio_irq_mask(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct sf_gpio_priv *priv = to_sf_gpio(gc);
+	unsigned long offset = irqd_to_hwirq(data);
+
+	sf_gpio_wr(priv, GPIO_IMR(offset), 1);
+	sf_gpio_wr(priv, GPIO_GPIMR(offset), 1);
+}
+
+static void sf_gpio_irq_unmask(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct sf_gpio_priv *priv = to_sf_gpio(gc);
+	unsigned long offset = irqd_to_hwirq(data);
+
+	sf_gpio_wr(priv, GPIO_IMR(offset), 0);
+	sf_gpio_wr(priv, GPIO_GPIMR(offset), 0);
+}
+
+/* We are actually setting the parents' affinity. */
+static int sf_gpio_irq_set_affinity(struct irq_data *data,
+				    const struct cpumask *dest, bool force)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	unsigned long offset = irqd_to_hwirq(data);
+	const struct cpumask *pdest;
+	struct irq_desc *pdesc;
+	struct irq_data *pdata;
+	unsigned int group;
+	int ret;
+
+	/* Find the parent IRQ and call its irq_set_affinity */
+	group = offset / GPIOS_PER_GROUP;
+	if (group >= gc->irq.num_parents)
+		return -EINVAL;
+
+	pdesc = irq_to_desc(gc->irq.parents[group]);
+	if (!pdesc)
+		return -EINVAL;
+
+	pdata = irq_desc_get_irq_data(pdesc);
+	if (!pdata->chip->irq_set_affinity)
+		return -EINVAL;
+
+	ret = pdata->chip->irq_set_affinity(pdata, dest, force);
+	if (ret < 0)
+		return ret;
+
+	/* Copy its effective_affinity back */
+	pdest = irq_data_get_effective_affinity_mask(pdata);
+	irq_data_update_effective_affinity(data, pdest);
+	return ret;
+}
+
+static int sf_gpio_irq_set_type(struct irq_data *data, unsigned int flow_type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct sf_gpio_priv *priv = to_sf_gpio(gc);
+	unsigned long offset = irqd_to_hwirq(data);
+	u32 val;
+
+	switch (flow_type) {
+	case IRQ_TYPE_EDGE_RISING:
+		val = 4;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		val = 2;
+		break;
+	case IRQ_TYPE_EDGE_BOTH:
+		val = 6;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		val = 1;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		val = 0;
+		break;
+	default:
+		return -EINVAL;
+	}
+	sf_gpio_wr(priv, GPIO_ITR(offset), val);
+
+	if (flow_type & IRQ_TYPE_LEVEL_MASK)
+		irq_set_handler_locked(data, handle_level_irq);
+	else
+		irq_set_handler_locked(data, handle_edge_irq);
+
+	return 0;
+}
+
+static const struct irq_chip sf_gpio_irqchip = {
+	.name			= KBUILD_MODNAME,
+	.irq_ack		= sf_gpio_irq_ack,
+	.irq_mask		= sf_gpio_irq_mask,
+	.irq_unmask		= sf_gpio_irq_unmask,
+	.irq_set_affinity	= sf_gpio_irq_set_affinity,
+	.irq_set_type		= sf_gpio_irq_set_type,
+	.flags			= IRQCHIP_IMMUTABLE,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static void sf_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+	struct irq_chip *ic = irq_desc_get_chip(desc);
+	struct sf_gpio_priv *priv = to_sf_gpio(gc);
+	unsigned int irq = irq_desc_get_irq(desc);
+	unsigned int group = irq - priv->irq[0];
+	unsigned long pending;
+	unsigned int n;
+
+	chained_irq_enter(ic, desc);
+
+	pending = sf_gpio_rd(priv, GPIO_GPxIR(group));
+	for_each_set_bit(n, &pending, GPIOS_PER_GROUP) {
+		generic_handle_domain_irq(gc->irq.domain,
+					  n + group * GPIOS_PER_GROUP);
+	}
+
+	chained_irq_exit(ic, desc);
+}
+
+static int sf_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sf_gpio_priv *priv;
+	struct gpio_irq_chip *girq;
+	struct gpio_chip *gc;
+	u32 ngpios, ngroups;
+	int ret, i;
+
+	ret = of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios);
+	if (ret)
+		return ret;
+
+	ngroups = DIV_ROUND_UP(ngpios, GPIOS_PER_GROUP);
+	priv = devm_kzalloc(dev, struct_size(priv, irq, ngroups), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(priv->clk))
+		return PTR_ERR(priv->clk);
+
+	priv->rstc = devm_reset_control_get_optional(&pdev->dev, NULL);
+	if (IS_ERR(priv->rstc))
+		return PTR_ERR(priv->rstc);
+
+	ret = reset_control_deassert(priv->rstc);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ngroups; i++) {
+		ret = platform_get_irq(pdev, i);
+		if (ret < 0)
+			return ret;
+
+		priv->irq[i] = ret;
+	}
+
+	gc = &priv->gc;
+	gc->label = KBUILD_MODNAME;
+	gc->parent = dev;
+	gc->owner = THIS_MODULE;
+	gc->request = gpiochip_generic_request;
+	gc->free = gpiochip_generic_free;
+	gc->get_direction = sf_gpio_get_direction;
+	gc->direction_input = sf_gpio_direction_input;
+	gc->direction_output = sf_gpio_direction_output;
+	gc->get = sf_gpio_get_value;
+	gc->set = sf_gpio_set_value;
+	gc->set_config = sf_gpio_set_config;
+	gc->base = -1;
+	gc->ngpio = ngpios;
+
+	girq = &gc->irq;
+	gpio_irq_chip_set_chip(girq, &sf_gpio_irqchip);
+	girq->num_parents = ngroups;
+	girq->parents = priv->irq;
+	girq->parent_handler = sf_gpio_irq_handler;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_bad_irq;
+
+	return devm_gpiochip_add_data(dev, gc, priv);
+}
+
+static void sf_gpio_remove(struct platform_device *pdev)
+{
+	struct sf_gpio_priv *priv = platform_get_drvdata(pdev);
+
+	reset_control_assert(priv->rstc);
+}
+
+static const struct of_device_id sf_gpio_ids[] = {
+	{ .compatible = "siflower,sf19a2890-gpio" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sf_gpio_ids);
+
+static struct platform_driver sf_gpio_driver = {
+	.probe		= sf_gpio_probe,
+	.remove		= sf_gpio_remove,
+	.driver = {
+		.name		= "siflower_gpio",
+		.owner		= THIS_MODULE,
+		.of_match_table	= sf_gpio_ids,
+	},
+};
+module_platform_driver(sf_gpio_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Qingfang Deng <qingfang.deng@siflower.com.cn>");
+MODULE_DESCRIPTION("GPIO driver for SiFlower SoCs");
-- 
2.47.1

Re: [PATCH 2/2] gpio: add support for GPIO controller on Siflower SoCs

[Linus Walleij]

Hi Chuanhong,

thanks for your patch!

I think it is better to split the driver instances into 4, one for each
physical block, as explained in the binding patch.

On Wed, Dec 25, 2024 at 4:59 AM Chuanhong Guo <gch981213@gmail.com> wrote:

> From: Qingfang Deng <qingfang.deng@siflower.com.cn>
>
> Add a driver for the GPIO controller on Siflower SoCs.
> This controller is found on all current Siflower MIPS and RISC-V
> chips including SF19A2890, SF21A6826 and SF21H8898.
>
> Signed-off-by: Qingfang Deng <qingfang.deng@siflower.com.cn>
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>



> +config GPIO_SIFLOWER
> +       tristate "SiFlower GPIO support"
> +       depends on OF_GPIO
> +       depends on MIPS || RISCV || COMPILE_TEST
> +       select GPIOLIB_IRQCHIP

select GPIO_GENERIC

As you have only a set of 32-bit registers to handle for each
instance, then some IRQs on top, you can untilize the MMIO
library.

> +#define GPIO_IR(n)     (0x40 * (n) + 0x00)
> +#define GPIO_OR(n)     (0x40 * (n) + 0x04)
> +#define GPIO_OEN(n)    (0x40 * (n) + 0x08)
> +#define GPIO_IMR(n)    (0x40 * (n) + 0x0c)
> +#define GPIO_GPIMR(n)  (0x40 * (n) + 0x10)
> +#define GPIO_PIR(n)    (0x40 * (n) + 0x14)
> +#define GPIO_ITR(n)    (0x40 * (n) + 0x18)
> +#define GPIO_IFR(n)    (0x40 * (n) + 0x1c)
> +#define GPIO_ICR(n)    (0x40 * (n) + 0x20)
> +#define GPIO_GPxIR(n)  (0x4 * (n) + 0x4000)

Just define GPIO_IR 0, GPIO_OR 4, etc.

Look up the GPIO_GPIR register separately from the
device tree and use it without offset.

Always register 16 GPIO lines unless ngpios is set.

Look at example drivers such as
drivers/gpio/gpio-pl061.c
drivers/gpio/gpio-ftgpio010.c
on how to use the MMIO helper library to implement
a simple driver for one instance reusing the common code.

You will also get implementations for get/set_multiple()
for free with this approach, as the library implements
them!

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: add support for GPIO controller on Siflower SoCs

[Chuanhong Guo]

Hi Linus!

On Sat, Dec 28, 2024 at 12:52 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Chuanhong,
>
> thanks for your patch!
>
> I think it is better to split the driver instances into 4, one for each
> physical block, as explained in the binding patch.
>
> On Wed, Dec 25, 2024 at 4:59 AM Chuanhong Guo <gch981213@gmail.com> wrote:
>
> > From: Qingfang Deng <qingfang.deng@siflower.com.cn>
> >
> > Add a driver for the GPIO controller on Siflower SoCs.
> > This controller is found on all current Siflower MIPS and RISC-V
> > chips including SF19A2890, SF21A6826 and SF21H8898.
> >
> > Signed-off-by: Qingfang Deng <qingfang.deng@siflower.com.cn>
> > Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
>
>
>
> > +config GPIO_SIFLOWER
> > +       tristate "SiFlower GPIO support"
> > +       depends on OF_GPIO
> > +       depends on MIPS || RISCV || COMPILE_TEST
> > +       select GPIOLIB_IRQCHIP
>
> select GPIO_GENERIC
>
> As you have only a set of 32-bit registers to handle for each
> instance, then some IRQs on top, you can untilize the MMIO
> library.
>
> > +#define GPIO_IR(n)     (0x40 * (n) + 0x00)
> > +#define GPIO_OR(n)     (0x40 * (n) + 0x04)
> > +#define GPIO_OEN(n)    (0x40 * (n) + 0x08)
> > +#define GPIO_IMR(n)    (0x40 * (n) + 0x0c)
> > +#define GPIO_GPIMR(n)  (0x40 * (n) + 0x10)
> > +#define GPIO_PIR(n)    (0x40 * (n) + 0x14)
> > +#define GPIO_ITR(n)    (0x40 * (n) + 0x18)
> > +#define GPIO_IFR(n)    (0x40 * (n) + 0x1c)
> > +#define GPIO_ICR(n)    (0x40 * (n) + 0x20)

This set of registers is for a single pin, not a multiple-pin block.

> > +#define GPIO_GPxIR(n)  (0x4 * (n) + 0x4000)
>
> Just define GPIO_IR 0, GPIO_OR 4, etc.
>
> Look up the GPIO_GPIR register separately from the
> device tree and use it without offset.
>
> Always register 16 GPIO lines unless ngpios is set.
>
> Look at example drivers such as
> drivers/gpio/gpio-pl061.c
> drivers/gpio/gpio-ftgpio010.c
> on how to use the MMIO helper library to implement
> a simple driver for one instance reusing the common code.

In my case the MMIO library doesn't seem really helpful since that's
for the more common case where multiple GPIO pins share one set of
registers.

-- 
Regards,
Chuanhong Guo

Re: [PATCH 2/2] gpio: add support for GPIO controller on Siflower SoCs

[Krzysztof Kozlowski]

On Wed, Dec 25, 2024 at 11:58:51AM +0800, Chuanhong Guo wrote:
> From: Qingfang Deng <qingfang.deng@siflower.com.cn>
> 
> Add a driver for the GPIO controller on Siflower SoCs.
> This controller is found on all current Siflower MIPS and RISC-V
> chips including SF19A2890, SF21A6826 and SF21H8898.
> 
> Signed-off-by: Qingfang Deng <qingfang.deng@siflower.com.cn>
> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> ---
>  drivers/gpio/Kconfig         |   9 +
>  drivers/gpio/Makefile        |   1 +
>  drivers/gpio/gpio-siflower.c | 353 +++++++++++++++++++++++++++++++++++
>  3 files changed, 363 insertions(+)
>  create mode 100644 drivers/gpio/gpio-siflower.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index add5ad29a673..fdc9a89ffbf3 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -637,6 +637,15 @@ config GPIO_SIFIVE
>  	help
>  	  Say yes here to support the GPIO device on SiFive SoCs.
>  
> +config GPIO_SIFLOWER
> +	tristate "SiFlower GPIO support"
> +	depends on OF_GPIO
> +	depends on MIPS || RISCV || COMPILE_TEST

This is supposed to be dependency on ARCH, not instruction set. I don't
se anything MIPS or RISCV here.

> +	select GPIOLIB_IRQCHIP
> +	help
> +	  GPIO controller driver for SiFlower MIPS and RISC-V SoCs
> +	  including SF19A2890, SF21A6826 and SF21H8898.

...

> +static void sf_gpio_remove(struct platform_device *pdev)
> +{
> +	struct sf_gpio_priv *priv = platform_get_drvdata(pdev);
> +
> +	reset_control_assert(priv->rstc);
> +}
> +
> +static const struct of_device_id sf_gpio_ids[] = {
> +	{ .compatible = "siflower,sf19a2890-gpio" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sf_gpio_ids);
> +
> +static struct platform_driver sf_gpio_driver = {
> +	.probe		= sf_gpio_probe,
> +	.remove		= sf_gpio_remove,
> +	.driver = {
> +		.name		= "siflower_gpio",
> +		.owner		= THIS_MODULE,

You sent us some old code with old code style, so probably you sent us
donwstream poor driver. Please clean it up before posting.

Please run standard kernel tools for static analysis, like coccinelle,
smatch and sparse, and fix reported warnings. Also please check for
warnings when building with W=1. Most of these commands (checks or W=1
build) can build specific targets, like some directory, to narrow the
scope to only your code. The code here looks like it needs a fix. Feel
free to get in touch if the warning is not clear.

Best regards,
Krzysztof

Re: [PATCH 2/2] gpio: add support for GPIO controller on Siflower SoCs

[Chuanhong Guo]

Hello Krzysztof!

On Tue, Dec 31, 2024 at 4:38 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Wed, Dec 25, 2024 at 11:58:51AM +0800, Chuanhong Guo wrote:
> > From: Qingfang Deng <qingfang.deng@siflower.com.cn>
> >
> > Add a driver for the GPIO controller on Siflower SoCs.
> > This controller is found on all current Siflower MIPS and RISC-V
> > chips including SF19A2890, SF21A6826 and SF21H8898.
> >
> > Signed-off-by: Qingfang Deng <qingfang.deng@siflower.com.cn>
> > Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
> > ---
> >  drivers/gpio/Kconfig         |   9 +
> >  drivers/gpio/Makefile        |   1 +
> >  drivers/gpio/gpio-siflower.c | 353 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 363 insertions(+)
> >  create mode 100644 drivers/gpio/gpio-siflower.c
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index add5ad29a673..fdc9a89ffbf3 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -637,6 +637,15 @@ config GPIO_SIFIVE
> >       help
> >         Say yes here to support the GPIO device on SiFive SoCs.
> >
> > +config GPIO_SIFLOWER
> > +     tristate "SiFlower GPIO support"
> > +     depends on OF_GPIO
> > +     depends on MIPS || RISCV || COMPILE_TEST
>
> This is supposed to be dependency on ARCH, not instruction set. I don't
> se anything MIPS or RISCV here.

I haven't sent any arch patches yet. The SoCs basically work with
MIPS/RISC-V generic kernel so I was planning to deal with it last with
some device trees.
Should I simply drop this dependency line for now, or should I add
ARCH_xxx to arch/{mips,riscv}/Kconfig first?

>
> > +     select GPIOLIB_IRQCHIP
> > +     help
> > +       GPIO controller driver for SiFlower MIPS and RISC-V SoCs
> > +       including SF19A2890, SF21A6826 and SF21H8898.
>
> ...
>
> > +static void sf_gpio_remove(struct platform_device *pdev)
> > +{
> > +     struct sf_gpio_priv *priv = platform_get_drvdata(pdev);
> > +
> > +     reset_control_assert(priv->rstc);
> > +}
> > +
> > +static const struct of_device_id sf_gpio_ids[] = {
> > +     { .compatible = "siflower,sf19a2890-gpio" },
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, sf_gpio_ids);
> > +
> > +static struct platform_driver sf_gpio_driver = {
> > +     .probe          = sf_gpio_probe,
> > +     .remove         = sf_gpio_remove,
> > +     .driver = {
> > +             .name           = "siflower_gpio",
> > +             .owner          = THIS_MODULE,
>
> You sent us some old code with old code style, so probably you sent us
> donwstream poor driver.

I'll drop this owner line.

> Please clean it up before posting.

Do you have other specific review comments? I haven't found other
stuff to clean up currently.

> Please run standard kernel tools for static analysis, like coccinelle,

coccinelle failed on something else:

make ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- C=2
CHECK="scripts/coccicheck" drivers/gpio/gpio-siflower.o

[...]
/usr/bin/spatch -D report --no-show-diff --very-quiet --cocci-file
./scripts/coccinelle/misc/secs_to_jiffies.cocci -I ./arch/mips/include
-I ./arch/mips/include/generated -I ./include -I ./include -I
./arch/mips/include/uapi -I ./arch/mips/include/generated/uapi -I
./include/uapi -I ./include/generated/uapi --include
./include/linux/compiler-version.h --include ./include/linux/kconfig.h
scripts/mod/empty.c
virtual rule report not supported
coccicheck failed

After removing ./scripts/coccinelle/misc/secs_to_jiffies.cocci, I got:

[...]
/usr/bin/spatch -D report --no-show-diff --very-quiet --cocci-file
./scripts/coccinelle/api/stream
_open.cocci -I ./arch/mips/include -I ./arch/mips/include/generated -I
./include -I ./include -I .
/arch/mips/include/uapi -I ./arch/mips/include/generated/uapi -I
./include/uapi -I ./include/gener
ated/uapi --include ./include/linux/compiler-version.h --include
./include/linux/kconfig.h drivers
/gpio/gpio-siflower.c
warning: line 140: should noop_llseek be a metavariable?
warning: line 222: should nonseekable_open be a metavariable?
warning: line 289: should nonseekable_open be a metavariable?
warning: line 337: should nonseekable_open be a metavariable?
[...]

These doesn't seem to be related to gpio-siflower.c. There's no
noop_llseek or nonseekable_open in it.

> smatch

smatch give me nothing:

make ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- C=2 CHECK="smatch
-p=kernel" drivers/gpio/gpio-siflower.o
[...]
CC kernel/bounds.s
CC arch/mips/kernel/asm-offsets.s
CALL scripts/checksyscalls.sh
CHKSHA1 include/linux/atomic/atomic-arch-fallback.h
CHKSHA1 include/linux/atomic/atomic-instrumented.h
CHKSHA1 include/linux/atomic/atomic-long.h
CC drivers/gpio/gpio-siflower.o
CHECK drivers/gpio/gpio-siflower.c

> and sparse

sparse doesn't seem to tell me anything about the driver itself either.

make ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- C=2 CHECK="sparse"
drivers/gpio/gpio-siflower.o
  CHECK   scripts/mod/empty.c
command-line: note: in included file:
builtin:1:9: warning: preprocessor token __ATOMIC_ACQUIRE redefined
builtin:0:0: this was the original definition
builtin:1:9: warning: preprocessor token __ATOMIC_SEQ_CST redefined
builtin:0:0: this was the original definition
builtin:1:9: warning: preprocessor token __ATOMIC_ACQ_REL redefined
builtin:0:0: this was the original definition
builtin:1:9: warning: preprocessor token __ATOMIC_RELEASE redefined
builtin:0:0: this was the original definition
  CALL    scripts/checksyscalls.sh
  CHECK   drivers/gpio/gpio-siflower.c
command-line: note: in included file:
builtin:1:9: warning: preprocessor token __ATOMIC_ACQUIRE redefined
builtin:0:0: this was the original definition
builtin:1:9: warning: preprocessor token __ATOMIC_SEQ_CST redefined
builtin:0:0: this was the original definition
builtin:1:9: warning: preprocessor token __ATOMIC_ACQ_REL redefined
builtin:0:0: this was the original definition
builtin:1:9: warning: preprocessor token __ATOMIC_RELEASE redefined
builtin:0:0: this was the original definition


> , and fix reported warnings. Also please check for
> warnings when building with W=1.

I also got nothing from this one:

rm drivers/gpio/gpio-siflower.o && make ARCH=mips
CROSS_COMPILE=mipsel-linux-gnu- W=1 drivers/gpio/gpio-siflower.o
  CC      scripts/mod/empty.o
  MKELF   scripts/mod/elfconfig.h
  HOSTCC  scripts/mod/modpost.o
  CC      scripts/mod/devicetable-offsets.s
  HOSTCC  scripts/mod/file2alias.o
  HOSTCC  scripts/mod/sumversion.o
  HOSTCC  scripts/mod/symsearch.o
  HOSTLD  scripts/mod/modpost
  CC      kernel/bounds.s
  CC      arch/mips/kernel/asm-offsets.s
  CALL    scripts/checksyscalls.sh
  CC      drivers/gpio/gpio-siflower.o

> Most of these commands (checks or W=1
> build) can build specific targets, like some directory, to narrow the
> scope to only your code. The code here looks like it needs a fix.

Would you mind sharing the warnings you found, and the command to
reproduce those?

> Feel
> free to get in touch if the warning is not clear.
>
> Best regards,
> Krzysztof
>

-- 
Regards,
Chuanhong Guo

Re: [PATCH 2/2] gpio: add support for GPIO controller on Siflower SoCs

[Krzysztof Kozlowski]

On 01/01/2025 10:11, Chuanhong Guo wrote:
> Hello Krzysztof!
> 
> On Tue, Dec 31, 2024 at 4:38 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Wed, Dec 25, 2024 at 11:58:51AM +0800, Chuanhong Guo wrote:
>>> From: Qingfang Deng <qingfang.deng@siflower.com.cn>
>>>
>>> Add a driver for the GPIO controller on Siflower SoCs.
>>> This controller is found on all current Siflower MIPS and RISC-V
>>> chips including SF19A2890, SF21A6826 and SF21H8898.
>>>
>>> Signed-off-by: Qingfang Deng <qingfang.deng@siflower.com.cn>
>>> Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
>>> ---
>>>  drivers/gpio/Kconfig         |   9 +
>>>  drivers/gpio/Makefile        |   1 +
>>>  drivers/gpio/gpio-siflower.c | 353 +++++++++++++++++++++++++++++++++++
>>>  3 files changed, 363 insertions(+)
>>>  create mode 100644 drivers/gpio/gpio-siflower.c
>>>
>>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>>> index add5ad29a673..fdc9a89ffbf3 100644
>>> --- a/drivers/gpio/Kconfig
>>> +++ b/drivers/gpio/Kconfig
>>> @@ -637,6 +637,15 @@ config GPIO_SIFIVE
>>>       help
>>>         Say yes here to support the GPIO device on SiFive SoCs.
>>>
>>> +config GPIO_SIFLOWER
>>> +     tristate "SiFlower GPIO support"
>>> +     depends on OF_GPIO
>>> +     depends on MIPS || RISCV || COMPILE_TEST
>>
>> This is supposed to be dependency on ARCH, not instruction set. I don't
>> se anything MIPS or RISCV here.
> 
> I haven't sent any arch patches yet. The SoCs basically work with
> MIPS/RISC-V generic kernel so I was planning to deal with it last with
> some device trees.
> Should I simply drop this dependency line for now, or should I add

This would work but OTOH it is also easy to forget to re-add it later.

> ARCH_xxx to arch/{mips,riscv}/Kconfig first?

But this is preferred. For me ARCH always comes first (or in parallel),
not the last. Why would we like to accept drivers which are not really
usable without the main parts?

> 
>>
>>> +     select GPIOLIB_IRQCHIP
>>> +     help
>>> +       GPIO controller driver for SiFlower MIPS and RISC-V SoCs
>>> +       including SF19A2890, SF21A6826 and SF21H8898.
>>
>> ...


> rm drivers/gpio/gpio-siflower.o && make ARCH=mips
> CROSS_COMPILE=mipsel-linux-gnu- W=1 drivers/gpio/gpio-siflower.o
>   CC      scripts/mod/empty.o
>   MKELF   scripts/mod/elfconfig.h
>   HOSTCC  scripts/mod/modpost.o
>   CC      scripts/mod/devicetable-offsets.s
>   HOSTCC  scripts/mod/file2alias.o
>   HOSTCC  scripts/mod/sumversion.o
>   HOSTCC  scripts/mod/symsearch.o
>   HOSTLD  scripts/mod/modpost
>   CC      kernel/bounds.s
>   CC      arch/mips/kernel/asm-offsets.s
>   CALL    scripts/checksyscalls.sh
>   CC      drivers/gpio/gpio-siflower.o
> 
>> Most of these commands (checks or W=1
>> build) can build specific targets, like some directory, to narrow the
>> scope to only your code. The code here looks like it needs a fix.
> 
> Would you mind sharing the warnings you found, and the command to
> reproduce those?

Cocci prints warning for the owner and based on owner presence I suspect
there could be more of patterns we fixed over last 10 years.

Best regards,
Krzysztof