[PATCH v3 0/2] gpio: ixp4xx: Handle external clock output

[PATCH v3 0/2] gpio: ixp4xx: Handle external clock output

[Linus Walleij]

The GPIO block on the very legacy IXP4xx GPIO can provide
a generated clock output on GPIO 14 and GPIO 15. This
provides a straight-forward solution with a flag for each
clock output.

More complicated solutions are thinkable, but I deemed them
overdesigned for this legacy SoC.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes in v3:
- Make sure to only manipulate the clock bits if one of the clock
  DT properties is set. Devices we can't test may rely on HW defaults being
  preserved in the clock bits.
- Link to v2: https://lore.kernel.org/r/20230922-ixp4xx-gpio-clocks-v2-0-0215ee10976d@linaro.org

Changes in v2:
- Fixed formatting pipe | in bindings
- Fixed som blank lines in bindings
- When we will just blank out the clock register settings,
  don't spend time reading the initial value.
- Link to v1: https://lore.kernel.org/r/20230921-ixp4xx-gpio-clocks-v1-0-574942bf944a@linaro.org

---
Linus Walleij (2):
      gpio: Rewrite IXP4xx GPIO bindings in schema
      gpio: ixp4xx: Handle clock output on pin 14 and 15

 .../devicetree/bindings/gpio/intel,ixp4xx-gpio.txt | 38 -----------
 .../bindings/gpio/intel,ixp4xx-gpio.yaml           | 73 ++++++++++++++++++++++
 MAINTAINERS                                        |  2 +-
 drivers/gpio/gpio-ixp4xx.c                         | 49 ++++++++++++++-
 4 files changed, 122 insertions(+), 40 deletions(-)
---
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
change-id: 20230921-ixp4xx-gpio-clocks-7e82289f4bb3

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

[PATCH v3 1/2] gpio: Rewrite IXP4xx GPIO bindings in schema

[Linus Walleij]

This rewrites the IXP4xx GPIO bindings to use YAML schema,
and adds two new properties to enable fixed clock output on
pins 14 and 15.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../devicetree/bindings/gpio/intel,ixp4xx-gpio.txt | 38 -----------
 .../bindings/gpio/intel,ixp4xx-gpio.yaml           | 73 ++++++++++++++++++++++
 MAINTAINERS                                        |  2 +-
 3 files changed, 74 insertions(+), 39 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/intel,ixp4xx-gpio.txt b/Documentation/devicetree/bindings/gpio/intel,ixp4xx-gpio.txt
deleted file mode 100644
index 8dc41ed99685..000000000000
--- a/Documentation/devicetree/bindings/gpio/intel,ixp4xx-gpio.txt
+++ /dev/null
@@ -1,38 +0,0 @@
-Intel IXP4xx XScale Networking Processors GPIO
-
-This GPIO controller is found in the Intel IXP4xx processors.
-It supports 16 GPIO lines.
-
-The interrupt portions of the GPIO controller is hierarchical:
-the synchronous edge detector is part of the GPIO block, but the
-actual enabling/disabling of the interrupt line is done in the
-main IXP4xx interrupt controller which has a 1:1 mapping for
-the first 12 GPIO lines to 12 system interrupts.
-
-The remaining 4 GPIO lines can not be used for receiving
-interrupts.
-
-The interrupt parent of this GPIO controller must be the
-IXP4xx interrupt controller.
-
-Required properties:
-
-- compatible : Should be
-  "intel,ixp4xx-gpio"
-- reg : Should contain registers location and length
-- gpio-controller : marks this as a GPIO controller
-- #gpio-cells : Should be 2, see gpio/gpio.txt
-- interrupt-controller : marks this as an interrupt controller
-- #interrupt-cells : a standard two-cell interrupt, see
-  interrupt-controller/interrupts.txt
-
-Example:
-
-gpio0: gpio@c8004000 {
-	compatible = "intel,ixp4xx-gpio";
-	reg = <0xc8004000 0x1000>;
-	gpio-controller;
-	#gpio-cells = <2>;
-	interrupt-controller;
-	#interrupt-cells = <2>;
-};
diff --git a/Documentation/devicetree/bindings/gpio/intel,ixp4xx-gpio.yaml b/Documentation/devicetree/bindings/gpio/intel,ixp4xx-gpio.yaml
new file mode 100644
index 000000000000..bfcb1f364c3a
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/intel,ixp4xx-gpio.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/intel,ixp4xx-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel IXP4xx XScale Networking Processors GPIO Controller
+
+description: |
+  This GPIO controller is found in the Intel IXP4xx
+  processors. It supports 16 GPIO lines.
+  The interrupt portions of the GPIO controller is hierarchical.
+  The synchronous edge detector is part of the GPIO block, but the
+  actual enabling/disabling of the interrupt line is done in the
+  main IXP4xx interrupt controller which has a 1-to-1 mapping for
+  the first 12 GPIO lines to 12 system interrupts.
+  The remaining 4 GPIO lines can not be used for receiving
+  interrupts.
+  The interrupt parent of this GPIO controller must be the
+  IXP4xx interrupt controller.
+  GPIO 14 and 15 can be used as clock outputs rather than GPIO,
+  and this can be enabled by a special flag.
+
+maintainers:
+  - Linus Walleij <linus.walleij@linaro.org>
+
+properties:
+  compatible:
+    const: intel,ixp4xx-gpio
+
+  reg:
+    maxItems: 1
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  intel,ixp4xx-gpio14-clkout:
+    description: If defined, enables clock output on GPIO 14
+      instead of GPIO.
+    type: boolean
+
+  intel,ixp4xx-gpio15-clkout:
+    description: If defined, enables clock output on GPIO 15
+      instead of GPIO.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+  - "#gpio-cells"
+  - interrupt-controller
+  - "#interrupt-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    gpio@c8004000 {
+        compatible = "intel,ixp4xx-gpio";
+        reg = <0xc8004000 0x1000>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 90f13281d297..4e216887eb76 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2215,7 +2215,7 @@ M:	Krzysztof Halasa <khalasa@piap.pl>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
 F:	Documentation/devicetree/bindings/arm/intel-ixp4xx.yaml
-F:	Documentation/devicetree/bindings/gpio/intel,ixp4xx-gpio.txt
+F:	Documentation/devicetree/bindings/gpio/intel,ixp4xx-gpio.yaml
 F:	Documentation/devicetree/bindings/interrupt-controller/intel,ixp4xx-interrupt.yaml
 F:	Documentation/devicetree/bindings/memory-controllers/intel,ixp4xx-expansion*
 F:	Documentation/devicetree/bindings/timer/intel,ixp4xx-timer.yaml

-- 
2.41.0

Re: [PATCH v3 1/2] gpio: Rewrite IXP4xx GPIO bindings in schema

[Bartosz Golaszewski]

On Sat, Sep 23, 2023 at 6:02 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> This rewrites the IXP4xx GPIO bindings to use YAML schema,
> and adds two new properties to enable fixed clock output on
> pins 14 and 15.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

I queued this, no need to resend it with the next respin of patch 2/2.

Bart

[PATCH v3 2/2] gpio: ixp4xx: Handle clock output on pin 14 and 15

[Linus Walleij]

This makes it possible to provide basic clock output on pins
14 and 15. The clocks are typically used by random electronics,
not modeled in the device tree, so they just need to be provided
on request.

In order to not disturb old systems that require that the
hardware defaults are kept in the clock setting bits, we only
manipulate these if either device tree property is present.
Once we know a device needs one of the clocks we can set it
in the device tree.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-ixp4xx.c | 49 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-ixp4xx.c b/drivers/gpio/gpio-ixp4xx.c
index dde6cf3a5779..1ca3217d0aab 100644
--- a/drivers/gpio/gpio-ixp4xx.c
+++ b/drivers/gpio/gpio-ixp4xx.c
@@ -38,6 +38,18 @@
 #define IXP4XX_GPIO_STYLE_MASK		GENMASK(2, 0)
 #define IXP4XX_GPIO_STYLE_SIZE		3
 
+/*
+ * Clock output control register defines.
+ */
+#define IXP4XX_GPCLK_CLK0DC_SHIFT	0
+#define IXP4XX_GPCLK_CLK0TC_SHIFT	4
+#define IXP4XX_GPCLK_CLK0_MASK		GENMASK(7, 0)
+#define IXP4XX_GPCLK_MUX14		BIT(8)
+#define IXP4XX_GPCLK_CLK1DC_SHIFT	16
+#define IXP4XX_GPCLK_CLK1TC_SHIFT	20
+#define IXP4XX_GPCLK_CLK1_MASK		GENMASK(23, 16)
+#define IXP4XX_GPCLK_MUX15		BIT(24)
+
 /**
  * struct ixp4xx_gpio - IXP4 GPIO state container
  * @dev: containing device for this instance
@@ -202,6 +214,8 @@ static int ixp4xx_gpio_probe(struct platform_device *pdev)
 	struct ixp4xx_gpio *g;
 	struct gpio_irq_chip *girq;
 	struct device_node *irq_parent;
+	bool clk_14, clk_15;
+	u32 val;
 	int ret;
 
 	g = devm_kzalloc(dev, sizeof(*g), GFP_KERNEL);
@@ -231,7 +245,40 @@ static int ixp4xx_gpio_probe(struct platform_device *pdev)
 	 */
 	if (of_machine_is_compatible("dlink,dsm-g600-a") ||
 	    of_machine_is_compatible("iom,nas-100d"))
-		__raw_writel(0x0, g->base + IXP4XX_REG_GPCLK);
+		val = 0;
+	else
+		val = __raw_readl(g->base + IXP4XX_REG_GPCLK);
+
+	/*
+	 * If either clock output is enabled explicitly in the device tree
+	 * we take full control of the clock by masking off all bits for
+	 * the clock control and selectively enabling them. Otherwise
+	 * we leave the hardware default settings.
+	 *
+	 * Enable clock outputs with default timings of requested clock.
+	 * If you need control over TC and DC, add these to the device
+	 * tree bindings and use them here.
+	 */
+	clk_14 = of_property_read_bool(np, "intel,ixp4xx-gpio14-clkout");
+	clk_15 = of_property_read_bool(np, "intel,ixp4xx-gpio15-clkout");
+	if (clk_14 || clk_15) {
+		val &= ~(IXP4XX_GPCLK_MUX14 | IXP4XX_GPCLK_MUX15);
+		val &= ~IXP4XX_GPCLK_CLK0_MASK;
+		val &= ~IXP4XX_GPCLK_CLK1_MASK;
+		if (clk_14) {
+			val |= (0 << IXP4XX_GPCLK_CLK0DC_SHIFT);
+			val |= (1 << IXP4XX_GPCLK_CLK0TC_SHIFT);
+			val |= IXP4XX_GPCLK_MUX14;
+		}
+
+		if (clk_15) {
+			val |= (0 << IXP4XX_GPCLK_CLK1DC_SHIFT);
+			val |= (1 << IXP4XX_GPCLK_CLK1TC_SHIFT);
+			val |= IXP4XX_GPCLK_MUX15;
+		}
+	}
+
+	__raw_writel(val, g->base + IXP4XX_REG_GPCLK);
 
 	/*
 	 * This is a very special big-endian ARM issue: when the IXP4xx is

-- 
2.41.0

Re: [PATCH v3 2/2] gpio: ixp4xx: Handle clock output on pin 14 and 15

[Andy Shevchenko]

On Sat, Sep 23, 2023 at 06:02:29PM +0200, Linus Walleij wrote:
> This makes it possible to provide basic clock output on pins
> 14 and 15. The clocks are typically used by random electronics,
> not modeled in the device tree, so they just need to be provided
> on request.
> 
> In order to not disturb old systems that require that the
> hardware defaults are kept in the clock setting bits, we only
> manipulate these if either device tree property is present.
> Once we know a device needs one of the clocks we can set it
> in the device tree.

Given that cover letter implicitly explains why not PPS,
Reviewed-by: Andy Shevchenko <andy@kernel.org>

Also see below.

...

>  	if (of_machine_is_compatible("dlink,dsm-g600-a") ||
>  	    of_machine_is_compatible("iom,nas-100d"))
> -		__raw_writel(0x0, g->base + IXP4XX_REG_GPCLK);
> +		val = 0;
> +	else
> +		val = __raw_readl(g->base + IXP4XX_REG_GPCLK);
> +
> +	/*
> +	 * If either clock output is enabled explicitly in the device tree
> +	 * we take full control of the clock by masking off all bits for
> +	 * the clock control and selectively enabling them. Otherwise
> +	 * we leave the hardware default settings.
> +	 *
> +	 * Enable clock outputs with default timings of requested clock.
> +	 * If you need control over TC and DC, add these to the device
> +	 * tree bindings and use them here.
> +	 */
> +	clk_14 = of_property_read_bool(np, "intel,ixp4xx-gpio14-clkout");
> +	clk_15 = of_property_read_bool(np, "intel,ixp4xx-gpio15-clkout");
> +	if (clk_14 || clk_15) {
> +		val &= ~(IXP4XX_GPCLK_MUX14 | IXP4XX_GPCLK_MUX15);
> +		val &= ~IXP4XX_GPCLK_CLK0_MASK;
> +		val &= ~IXP4XX_GPCLK_CLK1_MASK;
> +		if (clk_14) {
> +			val |= (0 << IXP4XX_GPCLK_CLK0DC_SHIFT);
> +			val |= (1 << IXP4XX_GPCLK_CLK0TC_SHIFT);
> +			val |= IXP4XX_GPCLK_MUX14;
> +		}
> +
> +		if (clk_15) {
> +			val |= (0 << IXP4XX_GPCLK_CLK1DC_SHIFT);
> +			val |= (1 << IXP4XX_GPCLK_CLK1TC_SHIFT);
> +			val |= IXP4XX_GPCLK_MUX15;
> +		}
> +	}
> +
> +	__raw_writel(val, g->base + IXP4XX_REG_GPCLK);

Can be optimized this way (not insisting, though):

	/*
	 * If either clock output is enabled explicitly in the device tree
	 * we take full control of the clock by masking off all bits for
	 * the clock control and selectively enabling them. Otherwise
	 * we leave the hardware default settings.
	 *
	 * Enable clock outputs with default timings of requested clock.
	 * If you need control over TC and DC, add these to the device
	 * tree bindings and use them here.
	 */
	clk_14 = of_property_read_bool(np, "intel,ixp4xx-gpio14-clkout");
	clk_15 = of_property_read_bool(np, "intel,ixp4xx-gpio15-clkout");

	if (of_machine_is_compatible("dlink,dsm-g600-a") ||
	    of_machine_is_compatible("iom,nas-100d")) {
		val = 0;
	} else {
		val = __raw_readl(g->base + IXP4XX_REG_GPCLK);
		if (clk_14 || clk_15) {

I'm wondering if it's fine to have them both to be cleared if not defined?
I.o.w. does it meant that appearance of one of the properties (to be set)
implies the other (to be not set)?

			val &= ~(IXP4XX_GPCLK_MUX14 | IXP4XX_GPCLK_MUX15);
			val &= ~IXP4XX_GPCLK_CLK0_MASK;
			val &= ~IXP4XX_GPCLK_CLK1_MASK;
		}
	}

	if (clk_14) {

		val |= (0 << IXP4XX_GPCLK_CLK0DC_SHIFT);

Wondering why you simply can't replace this...

		val |= (1 << IXP4XX_GPCLK_CLK0TC_SHIFT);
		val |= IXP4XX_GPCLK_MUX14;
	}

	if (clk_15) {
		val |= (0 << IXP4XX_GPCLK_CLK1DC_SHIFT);

...and this by a comment?

		val |= (1 << IXP4XX_GPCLK_CLK1TC_SHIFT);
		val |= IXP4XX_GPCLK_MUX15;
	}

	__raw_writel(val, g->base + IXP4XX_REG_GPCLK);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] gpio: ixp4xx: Handle clock output on pin 14 and 15

[Linus Walleij]

Oops found some unaddressed feedback and outliers since
september.

Time to catch up.

On Mon, Sep 25, 2023 at 9:19 AM Andy Shevchenko <andy@kernel.org> wrote:

> Given that cover letter implicitly explains why not PPS,
> Reviewed-by: Andy Shevchenko <andy@kernel.org>

Thanks!

> Can be optimized this way (not insisting, though):

It's nice, I changed it.

> I'm wondering if it's fine to have them both to be cleared if not defined?
> I.o.w. does it meant that appearance of one of the properties (to be set)
> implies the other (to be not set)?

I'm just cautious, there may be some components that require this
clocking that we don't know about, and that may be a reason why
they are enabled by e.g. RedBoot (the bootloader) at power on
and never touched.

The entire support is reverse-engineered so... we don't know exactly
what is going on. Would be easier if we has source for everything :/

>                         val &= ~(IXP4XX_GPCLK_MUX14 | IXP4XX_GPCLK_MUX15);
>                         val &= ~IXP4XX_GPCLK_CLK0_MASK;
>                         val &= ~IXP4XX_GPCLK_CLK1_MASK;
>                 }
>         }
>
>         if (clk_14) {
>
>                 val |= (0 << IXP4XX_GPCLK_CLK0DC_SHIFT);
>
> Wondering why you simply can't replace this...
>
>                 val |= (1 << IXP4XX_GPCLK_CLK0TC_SHIFT);
>                 val |= IXP4XX_GPCLK_MUX14;
>         }
>
>         if (clk_15) {
>                 val |= (0 << IXP4XX_GPCLK_CLK1DC_SHIFT);
>
> ...and this by a comment?

Yup, fixed it.

Yours,
Linus Walleij