[PATCH v4 0/4] gpio: siul2-s32g2: add initial GPIO driver

[PATCH v4 0/4] gpio: siul2-s32g2: add initial GPIO driver

[Andrei Stefanescu]

This patch series adds support for basic GPIO
operations(set, get, direction_output/input, set_config).

There are two SIUL2 hardware modules: SIUL2_0 and SIUL2_1.
However, this driver exports both as a single GPIO driver.
This is because the interrupt registers are located only
in SIUL2_1, even for GPIOs that are part of SIUL2_0.

There are two gaps in the GPIO ranges:
- 102-111(inclusive) are invalid
- 123-143(inclusive) are invalid

These will be excluded via the `gpio-reserved-ranges`
property.

Writing and reading GPIO values is done via the PGPDO/PGPDI
registers(Parallel GPIO Pad Data Output/Input) which are
16 bit registers, each bit corresponding to a GPIO.

Note that the PGPDO order is similar to a big-endian grouping
of two registers:
PGPDO1, PGPDO0, PGPDO3, PGPDO2, PGPDO5, PGPDO4, gap, PGPDO6.

I have other patches for this driver:
- interrupt support
- power management callbacks

which I plan to upstream after this series gets merged
in order to simplify the review process.

v4 -> v3
- removed useless parentheses
- added S32G3 fallback compatible
- fixed comment alignment
- fixed dt-bindings license
- fixed modpost: "__udivdi3"
- moved MAINTAINERS entry to have the new GPIO driver
  together with other files related to S32G

v3 -> v2
- fix dt-bindings schema id
- add maxItems to gpio-ranges
- removed gpio label from dt-bindings example
- added changelog for the MAINTAINERS commit and
  added separate entry for the SIUL2 GPIO driver
- added guard(raw_spinlock_irqsave) in
  'siul2_gpio_set_direction'
- updated the description for
  'devm_platform_get_and_ioremap_resource_byname'

v2 -> v1
dt-bindings:
- changed filename to match compatible
- fixed commit messages
- removed dt-bindings unnecessary properties descriptions
- added minItems for the interrupts property
driver:
- added depends on ARCH_S32 || COMPILE_TEST to Kconfig
- added select REGMAP_MMIO to Kconfig
- remove unnecessary include
- add of_node_put after `siul2_get_gpio_pinspec`
- removed inline from function definitions
- removed match data and moved the previous platdata
  definition to the top of the file to be visible
- replace bitmap_set/clear with __clear_bit/set_bit
  and devm_bitmap_zalloc with devm_kzalloc
- switched to gpiochip_generic_request/free/config
- fixed dev_err format for size_t reported by
  kernel test robot
- add platform_get_and_ioremap_resource_byname wrapper

Andrei Stefanescu (4):
  drivers: provide devm_platform_get_and_ioremap_resource_byname()
  dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs
  gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support
  MAINTAINERS: add MAINTAINER for S32G2 SIUL2 GPIO driver

 .../bindings/gpio/nxp,s32g2-siul2-gpio.yaml   | 110 ++++
 MAINTAINERS                                   |   2 +
 drivers/base/platform.c                       |  27 +
 drivers/gpio/Kconfig                          |  10 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-siul2-s32g2.c               | 576 ++++++++++++++++++
 include/linux/platform_device.h               |  13 +
 7 files changed, 739 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml
 create mode 100644 drivers/gpio/gpio-siul2-s32g2.c

-- 
2.45.2

[PATCH v4 1/4] drivers: provide devm_platform_get_and_ioremap_resource_byname()

[Andrei Stefanescu]

Similar to commit 890cc39a879906b63912482dfc41944579df2dc6
("drivers: provide devm_platform_get_and_ioremap_resource()")
add a wrapper for "platform_get_resource_byname" and
"devm_ioremap_resource". This new wrapper also returns the resource, if
any, via a pointer.

Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Matthias Brugger <mbrugger@suse.com>
Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
 drivers/base/platform.c         | 27 +++++++++++++++++++++++++++
 include/linux/platform_device.h | 13 +++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4c3ee6521ba5..da6827f9462a 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -108,6 +108,33 @@ devm_platform_get_and_ioremap_resource(struct platform_device *pdev,
 }
 EXPORT_SYMBOL_GPL(devm_platform_get_and_ioremap_resource);
 
+/**
+ * devm_platform_get_and_ioremap_resource_byname - call devm_ioremap_resource()
+ *					    for a platform device and get
+ *					    a resource by its name
+ *
+ * @pdev: platform device to use both for memory resource lookup as well as
+ *        resource management
+ * @name: resource name
+ * @res: optional output parameter to store a pointer to the obtained resource.
+ *
+ * Return: a pointer to the remapped memory or an ERR_PTR() encoded error code
+ * on failure.
+ */
+void __iomem *
+devm_platform_get_and_ioremap_resource_byname(struct platform_device *pdev,
+					      const char *name,
+					      struct resource **res)
+{
+	struct resource *r;
+
+	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
+	if (res)
+		*res = r;
+	return devm_ioremap_resource(&pdev->dev, r);
+}
+EXPORT_SYMBOL_GPL(devm_platform_get_and_ioremap_resource_byname);
+
 /**
  * devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform
  *				    device
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index d422db6eec63..ab7f33f3c426 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -68,6 +68,12 @@ platform_find_device_by_driver(struct device *start,
 extern void __iomem *
 devm_platform_get_and_ioremap_resource(struct platform_device *pdev,
 				unsigned int index, struct resource **res);
+
+extern void __iomem *
+devm_platform_get_and_ioremap_resource_byname(struct platform_device *pdev,
+					      const char *name,
+					      struct resource **res);
+
 extern void __iomem *
 devm_platform_ioremap_resource(struct platform_device *pdev,
 			       unsigned int index);
@@ -83,6 +89,13 @@ devm_platform_get_and_ioremap_resource(struct platform_device *pdev,
 	return ERR_PTR(-EINVAL);
 }
 
+static inline void __iomem *
+devm_platform_get_and_ioremap_resource_byname(struct platform_device *pdev,
+					      const char *name,
+					      struct resource **res)
+{
+	return ERR_PTR(-EINVAL);
+}
 
 static inline void __iomem *
 devm_platform_ioremap_resource(struct platform_device *pdev,
-- 
2.45.2

Re: [PATCH v4 1/4] drivers: provide devm_platform_get_and_ioremap_resource_byname()

[Greg Kroah-Hartman]

On Thu, Sep 26, 2024 at 05:31:18PM +0300, Andrei Stefanescu wrote:
> Similar to commit 890cc39a879906b63912482dfc41944579df2dc6
> ("drivers: provide devm_platform_get_and_ioremap_resource()")
> add a wrapper for "platform_get_resource_byname" and
> "devm_ioremap_resource". This new wrapper also returns the resource, if
> any, via a pointer.
> 
> Suggested-by: Krzysztof Kozlowski <krzk@kernel.org>
> Reviewed-by: Matthias Brugger <mbrugger@suse.com>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> ---
>  drivers/base/platform.c         | 27 +++++++++++++++++++++++++++
>  include/linux/platform_device.h | 13 +++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 4c3ee6521ba5..da6827f9462a 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -108,6 +108,33 @@ devm_platform_get_and_ioremap_resource(struct platform_device *pdev,
>  }
>  EXPORT_SYMBOL_GPL(devm_platform_get_and_ioremap_resource);
>  
> +/**
> + * devm_platform_get_and_ioremap_resource_byname - call devm_ioremap_resource()
> + *					    for a platform device and get
> + *					    a resource by its name
> + *
> + * @pdev: platform device to use both for memory resource lookup as well as
> + *        resource management
> + * @name: resource name
> + * @res: optional output parameter to store a pointer to the obtained resource.
> + *
> + * Return: a pointer to the remapped memory or an ERR_PTR() encoded error code
> + * on failure.
> + */
> +void __iomem *
> +devm_platform_get_and_ioremap_resource_byname(struct platform_device *pdev,
> +					      const char *name,
> +					      struct resource **res)
> +{
> +	struct resource *r;
> +
> +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> +	if (res)
> +		*res = r;
> +	return devm_ioremap_resource(&pdev->dev, r);

Does this really help out much?  Where will the end be if we keep
stacking these up like this, the function names are getting huge...

I'm not going to object, but I'm also not going to ack it :)

thanks,

greg k-h

[PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Andrei Stefanescu]

Add support for the GPIO driver of the NXP S32G2/S32G3 SoCs.

Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
 .../bindings/gpio/nxp,s32g2-siul2-gpio.yaml   | 110 ++++++++++++++++++
 1 file changed, 110 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml b/Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml
new file mode 100644
index 000000000000..4556505ee9c9
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml
@@ -0,0 +1,110 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+# Copyright 2024 NXP
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/nxp,s32g2-siul2-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP S32G2 SIUL2 GPIO controller
+
+maintainers:
+  - Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
+  - Larisa Grigore <larisa.grigore@nxp.com>
+  - Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
+
+description:
+  Support for the SIUL2 GPIOs found on the S32G2 and S32G3
+  chips. It includes an IRQ controller for all pins which have
+  an EIRQ associated.
+
+properties:
+  compatible:
+    oneOf:
+      - const: nxp,s32g2-siul2-gpio
+      - items:
+        - const: nxp,s32g3-siul2-gpio
+        - const: nxp,s32g2-siul2-gpio
+
+  reg:
+    items:
+      - description: PGPDO (output value) registers for SIUL2_0
+      - description: PGPDO (output value) registers for SIUL2_1
+      - description: PGPDI (input value) registers for SIUL2_0
+      - description: PGPDI (input value) registers for SIUL2_1
+      - description: EIRQ (interrupt) configuration registers from SIUL2_1
+      - description: EIRQ IMCR registers for interrupt muxing between pads
+
+  reg-names:
+    items:
+      - const: opads0
+      - const: opads1
+      - const: ipads0
+      - const: ipads1
+      - const: eirqs
+      - const: eirq-imcrs
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  gpio-ranges:
+    minItems: 2
+    maxItems: 2
+
+  gpio-reserved-ranges:
+    minItems: 2
+
+patternProperties:
+  "-hog(-[0-9]+)?$":
+    required:
+      - gpio-hog
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - gpio-controller
+  - "#gpio-cells"
+  - gpio-ranges
+  - gpio-reserved-ranges
+  - interrupts
+  - interrupt-controller
+  - "#interrupt-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    gpio@4009d700 {
+        compatible = "nxp,s32g2-siul2-gpio";
+        reg = <0x4009d700 0x10>,
+              <0x44011700 0x18>,
+              <0x4009d740 0x10>,
+              <0x44011740 0x18>,
+              <0x44010010 0xb4>,
+              <0x44011078 0x80>;
+        reg-names = "opads0", "opads1", "ipads0",
+                    "ipads1", "eirqs", "eirq-imcrs";
+        gpio-controller;
+        #gpio-cells = <2>;
+                      /* GPIO 0-101 */
+        gpio-ranges = <&pinctrl 0 0 102>,
+                      /* GPIO 112-190 */
+                      <&pinctrl 112 112 79>;
+        gpio-reserved-ranges = <102 10>, <123 21>;
+        interrupts = <GIC_SPI 210 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+    };
-- 
2.45.2

Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4736 bytes --]

On Thu, Sep 26, 2024 at 05:31:19PM +0300, Andrei Stefanescu wrote:
> Add support for the GPIO driver of the NXP S32G2/S32G3 SoCs.
> 
> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>

What's up with this SoB chain? You're the author what did
the other 3 people do? Are they missing co-developed-by tags?

> ---
>  .../bindings/gpio/nxp,s32g2-siul2-gpio.yaml   | 110 ++++++++++++++++++
>  1 file changed, 110 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml b/Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml
> new file mode 100644
> index 000000000000..4556505ee9c9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +# Copyright 2024 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/nxp,s32g2-siul2-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP S32G2 SIUL2 GPIO controller
> +
> +maintainers:
> +  - Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
> +  - Larisa Grigore <larisa.grigore@nxp.com>
> +  - Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> +
> +description:
> +  Support for the SIUL2 GPIOs found on the S32G2 and S32G3
> +  chips. It includes an IRQ controller for all pins which have
> +  an EIRQ associated.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - const: nxp,s32g2-siul2-gpio
> +      - items:
> +        - const: nxp,s32g3-siul2-gpio
> +        - const: nxp,s32g2-siul2-gpio
> +
> +  reg:
> +    items:
> +      - description: PGPDO (output value) registers for SIUL2_0
> +      - description: PGPDO (output value) registers for SIUL2_1
> +      - description: PGPDI (input value) registers for SIUL2_0
> +      - description: PGPDI (input value) registers for SIUL2_1
> +      - description: EIRQ (interrupt) configuration registers from SIUL2_1
> +      - description: EIRQ IMCR registers for interrupt muxing between pads
> +
> +  reg-names:
> +    items:
> +      - const: opads0
> +      - const: opads1
> +      - const: ipads0
> +      - const: ipads1
> +      - const: eirqs
> +      - const: eirq-imcrs
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  gpio-ranges:
> +    minItems: 2
> +    maxItems: 2
> +
> +  gpio-reserved-ranges:
> +    minItems: 2
> +
> +patternProperties:
> +  "-hog(-[0-9]+)?$":
> +    required:
> +      - gpio-hog
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - gpio-ranges
> +  - gpio-reserved-ranges
> +  - interrupts
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    gpio@4009d700 {
> +        compatible = "nxp,s32g2-siul2-gpio";
> +        reg = <0x4009d700 0x10>,
> +              <0x44011700 0x18>,
> +              <0x4009d740 0x10>,
> +              <0x44011740 0x18>,
> +              <0x44010010 0xb4>,
> +              <0x44011078 0x80>;

Huh, I only noticed this now. Are you sure that this is a correct
representation of this device, and it is not really part of some syscon?
The "random" nature of the addresses  and the tiny sizes of the
reservations make it seem that way. What other devices are in these
regions?

Additionally, it looks like "opads0" and "ipads0" are in a different
region to their "1" equivalents. Should this really be represented as
two disctint GPIO controllers?


Cheers,
Conor.

> +        reg-names = "opads0", "opads1", "ipads0",
> +                    "ipads1", "eirqs", "eirq-imcrs";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +                      /* GPIO 0-101 */
> +        gpio-ranges = <&pinctrl 0 0 102>,
> +                      /* GPIO 112-190 */
> +                      <&pinctrl 112 112 79>;
> +        gpio-reserved-ranges = <102 10>, <123 21>;
> +        interrupts = <GIC_SPI 210 IRQ_TYPE_LEVEL_HIGH>;
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +    };
> -- 
> 2.45.2
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Andrei Stefanescu]

Hi Conor,

Thank you very much for the prompt review!

On 26/09/2024 18:38, Conor Dooley wrote:
> On Thu, Sep 26, 2024 at 05:31:19PM +0300, Andrei Stefanescu wrote:
>> Add support for the GPIO driver of the NXP S32G2/S32G3 SoCs.
>>
>> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> 
> What's up with this SoB chain? You're the author what did
> the other 3 people do? Are they missing co-developed-by tags?

Yes, thank you for suggesting it! I will also add Co-developed-by tags
for them. In the end it should look like this:

Co-developed-by: Phu Luu An <phu.luuan@nxp.com>
Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
Co-developed-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>

>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +    gpio@4009d700 {
>> +        compatible = "nxp,s32g2-siul2-gpio";
>> +        reg = <0x4009d700 0x10>,
>> +              <0x44011700 0x18>,
>> +              <0x4009d740 0x10>,
>> +              <0x44011740 0x18>,
>> +              <0x44010010 0xb4>,
>> +              <0x44011078 0x80>;
> 
> Huh, I only noticed this now. Are you sure that this is a correct
> representation of this device, and it is not really part of some syscon?
> The "random" nature of the addresses  and the tiny sizes of the
> reservations make it seem that way. What other devices are in these
> regions?
> 
> Additionally, it looks like "opads0" and "ipads0" are in a different
> region to their "1" equivalents. Should this really be represented as
> two disctint GPIO controllers?

I will add a bit more context regarding the hardware.

The hardware module which implements pinctrl & GPIO is called SIUL2.
For both S32G2 and S32G3 we have the same version of the module and 
it is integrated in the same way. Each SoC has two SIUL2 instances which
mostly have the same register types and only differ in the number
of pads associated to them:

- SIUL2_0 mapped at address 0x4009c000, handling pins 0 - 101
- SIUL2_1 mapped at address 0x44010000, handling pins 112 - 190

There are multiple registers for the SIUL2 modules which are important
for pinctrl & GPIO:

- MSCR (Multiplexed Signal Configuration Register)
  It configures the function of a pin and some
  pinconf properties:
    - input buffer
    - output buffer
    - open-drain
    - pull-up/pull-down
    - slew rate
  Function 0 means the pin is to be used as a GPIO.

- IMCR (Input Multiplexed Signal and Configuration Register)
  If the signal on this pad is to be read by another hardware
  module, this register is similar to a multiplexer, its value
  configures which pad the hardware will link to the module.
  As an example let's consider the I2C0 SDA line. It has one
  IMCR associated to it. Below are its possible pins and
  corresponding IMCR values:
    pin 16 <- 2
    pin 24 <- 7
    pin 31 <- 3
    pin 122 <- 4 
      (Note that MSCR122 is part of SIUL2_1 but the IMCR for
       I2C0_SDA is part of SIUL2_0)
    pin 153 <- 5
    pin 161 <- 6
  The IMCR values should be aligned with the function bits in the
  MSCR bits. If we want to use pin 122 for I2C0_SDA we will configure
  the function bits in MSCR122 and write the value 4 to the I2C0_SDA
  IMCR. 

- PGPDO/PGPDI Parallel GPIO Pad Data Out/In
  16 bit registers where each bit(besides some gaps) represents
  a GPIO's output/input value

- DISR0, DIRER0, IREER0, IFEER0
  These registers are used for: status, enable, rising/falling edge
  configuration for interrupts. We have 32 interrupts called EIRQ and
  each interrupt has one or more pads associated with it (controlled
  by an IMCR register per EIRQ).

  However, one important thing to note is that even though there are
  EIRQs for SIUL2_0 pads, all the interrupt registers mentioned above
  are only present in SIUL2_1.

Because of mixed pins (I2C0_SDA in the example above with the MSCR
in SIUL2_1 for pad 122 and the IMCR in SIUL2_0) and the interrupt
configuration registers in SIUL2_1 we decided to have a single
driver instance.

> 
> 
> Cheers,
> Conor.
> 

Best regards,
Andrei

Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5085 bytes --]

On Fri, Sep 27, 2024 at 10:13:54AM +0300, Andrei Stefanescu wrote:
> Hi Conor,
> 
> Thank you very much for the prompt review!
> 
> On 26/09/2024 18:38, Conor Dooley wrote:
> > On Thu, Sep 26, 2024 at 05:31:19PM +0300, Andrei Stefanescu wrote:
> >> Add support for the GPIO driver of the NXP S32G2/S32G3 SoCs.
> >>
> >> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
> >> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> >> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> >> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> > 
> > What's up with this SoB chain? You're the author what did
> > the other 3 people do? Are they missing co-developed-by tags?
> 
> Yes, thank you for suggesting it! I will also add Co-developed-by tags
> for them. In the end it should look like this:
> 
> Co-developed-by: Phu Luu An <phu.luuan@nxp.com>
> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
> Co-developed-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> 
> >> +
> >> +examples:
> >> +  - |
> >> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> >> +    #include <dt-bindings/interrupt-controller/irq.h>
> >> +
> >> +    gpio@4009d700 {
> >> +        compatible = "nxp,s32g2-siul2-gpio";
> >> +        reg = <0x4009d700 0x10>,
> >> +              <0x44011700 0x18>,
> >> +              <0x4009d740 0x10>,
> >> +              <0x44011740 0x18>,
> >> +              <0x44010010 0xb4>,
> >> +              <0x44011078 0x80>;
> > 
> > Huh, I only noticed this now. Are you sure that this is a correct
> > representation of this device, and it is not really part of some syscon?
> > The "random" nature of the addresses  and the tiny sizes of the
> > reservations make it seem that way. What other devices are in these
> > regions?

Thanks for your answer to my second question, but I think you missed this
part here ^^^

> > 
> > Additionally, it looks like "opads0" and "ipads0" are in a different
> > region to their "1" equivalents. Should this really be represented as
> > two disctint GPIO controllers?
> 
> I will add a bit more context regarding the hardware.
> 
> The hardware module which implements pinctrl & GPIO is called SIUL2.
> For both S32G2 and S32G3 we have the same version of the module and 
> it is integrated in the same way. Each SoC has two SIUL2 instances which
> mostly have the same register types and only differ in the number
> of pads associated to them:
> 
> - SIUL2_0 mapped at address 0x4009c000, handling pins 0 - 101
> - SIUL2_1 mapped at address 0x44010000, handling pins 112 - 190
> 
> There are multiple registers for the SIUL2 modules which are important
> for pinctrl & GPIO:
> 
> - MSCR (Multiplexed Signal Configuration Register)
>   It configures the function of a pin and some
>   pinconf properties:
>     - input buffer
>     - output buffer
>     - open-drain
>     - pull-up/pull-down
>     - slew rate
>   Function 0 means the pin is to be used as a GPIO.
> 
> - IMCR (Input Multiplexed Signal and Configuration Register)
>   If the signal on this pad is to be read by another hardware
>   module, this register is similar to a multiplexer, its value
>   configures which pad the hardware will link to the module.
>   As an example let's consider the I2C0 SDA line. It has one
>   IMCR associated to it. Below are its possible pins and
>   corresponding IMCR values:
>     pin 16 <- 2
>     pin 24 <- 7
>     pin 31 <- 3
>     pin 122 <- 4 
>       (Note that MSCR122 is part of SIUL2_1 but the IMCR for
>        I2C0_SDA is part of SIUL2_0)
>     pin 153 <- 5
>     pin 161 <- 6
>   The IMCR values should be aligned with the function bits in the
>   MSCR bits. If we want to use pin 122 for I2C0_SDA we will configure
>   the function bits in MSCR122 and write the value 4 to the I2C0_SDA
>   IMCR. 
> 
> - PGPDO/PGPDI Parallel GPIO Pad Data Out/In
>   16 bit registers where each bit(besides some gaps) represents
>   a GPIO's output/input value
> 
> - DISR0, DIRER0, IREER0, IFEER0
>   These registers are used for: status, enable, rising/falling edge
>   configuration for interrupts. We have 32 interrupts called EIRQ and
>   each interrupt has one or more pads associated with it (controlled
>   by an IMCR register per EIRQ).
> 
>   However, one important thing to note is that even though there are
>   EIRQs for SIUL2_0 pads, all the interrupt registers mentioned above
>   are only present in SIUL2_1.
> 
> Because of mixed pins (I2C0_SDA in the example above with the MSCR
> in SIUL2_1 for pad 122 and the IMCR in SIUL2_0) and the interrupt
> configuration registers in SIUL2_1 we decided to have a single
> driver instance.
> 
> > 
> > 
> > Cheers,
> > Conor.
> > 
> 
> Best regards,
> Andrei
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 5746 bytes --]

On Mon, Sep 30, 2024 at 04:00:57PM +0100, Conor Dooley wrote:
> On Fri, Sep 27, 2024 at 10:13:54AM +0300, Andrei Stefanescu wrote:
> > Hi Conor,
> > 
> > Thank you very much for the prompt review!
> > 
> > On 26/09/2024 18:38, Conor Dooley wrote:
> > > On Thu, Sep 26, 2024 at 05:31:19PM +0300, Andrei Stefanescu wrote:
> > >> Add support for the GPIO driver of the NXP S32G2/S32G3 SoCs.
> > >>
> > >> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
> > >> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> > >> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > >> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> > > 
> > > What's up with this SoB chain? You're the author what did
> > > the other 3 people do? Are they missing co-developed-by tags?
> > 
> > Yes, thank you for suggesting it! I will also add Co-developed-by tags
> > for them. In the end it should look like this:
> > 
> > Co-developed-by: Phu Luu An <phu.luuan@nxp.com>
> > Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
> > Co-developed-by: Larisa Grigore <larisa.grigore@nxp.com>
> > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> > Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> > Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> > 
> > >> +
> > >> +examples:
> > >> +  - |
> > >> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > >> +    #include <dt-bindings/interrupt-controller/irq.h>
> > >> +
> > >> +    gpio@4009d700 {
> > >> +        compatible = "nxp,s32g2-siul2-gpio";
> > >> +        reg = <0x4009d700 0x10>,
> > >> +              <0x44011700 0x18>,
> > >> +              <0x4009d740 0x10>,
> > >> +              <0x44011740 0x18>,
> > >> +              <0x44010010 0xb4>,
> > >> +              <0x44011078 0x80>;
> > > 
> > > Huh, I only noticed this now. Are you sure that this is a correct
> > > representation of this device, and it is not really part of some syscon?
> > > The "random" nature of the addresses  and the tiny sizes of the
> > > reservations make it seem that way. What other devices are in these
> > > regions?
> 
> Thanks for your answer to my second question, but I think you missed this
> part here ^^^

Reading it again, I think you might have answered my first question,
though not explicitly. The regions in question do both pinctrl and gpio,
but you have chosen to represent it has lots of mini register regions,
rather than as a simple-mfd type device - which I think would be the
correct representation. .

Cheers,
Conor.

> 
> > > 
> > > Additionally, it looks like "opads0" and "ipads0" are in a different
> > > region to their "1" equivalents. Should this really be represented as
> > > two disctint GPIO controllers?
> > 
> > I will add a bit more context regarding the hardware.
> > 
> > The hardware module which implements pinctrl & GPIO is called SIUL2.
> > For both S32G2 and S32G3 we have the same version of the module and 
> > it is integrated in the same way. Each SoC has two SIUL2 instances which
> > mostly have the same register types and only differ in the number
> > of pads associated to them:
> > 
> > - SIUL2_0 mapped at address 0x4009c000, handling pins 0 - 101
> > - SIUL2_1 mapped at address 0x44010000, handling pins 112 - 190
> > 
> > There are multiple registers for the SIUL2 modules which are important
> > for pinctrl & GPIO:
> > 
> > - MSCR (Multiplexed Signal Configuration Register)
> >   It configures the function of a pin and some
> >   pinconf properties:
> >     - input buffer
> >     - output buffer
> >     - open-drain
> >     - pull-up/pull-down
> >     - slew rate
> >   Function 0 means the pin is to be used as a GPIO.
> > 
> > - IMCR (Input Multiplexed Signal and Configuration Register)
> >   If the signal on this pad is to be read by another hardware
> >   module, this register is similar to a multiplexer, its value
> >   configures which pad the hardware will link to the module.
> >   As an example let's consider the I2C0 SDA line. It has one
> >   IMCR associated to it. Below are its possible pins and
> >   corresponding IMCR values:
> >     pin 16 <- 2
> >     pin 24 <- 7
> >     pin 31 <- 3
> >     pin 122 <- 4 
> >       (Note that MSCR122 is part of SIUL2_1 but the IMCR for
> >        I2C0_SDA is part of SIUL2_0)
> >     pin 153 <- 5
> >     pin 161 <- 6
> >   The IMCR values should be aligned with the function bits in the
> >   MSCR bits. If we want to use pin 122 for I2C0_SDA we will configure
> >   the function bits in MSCR122 and write the value 4 to the I2C0_SDA
> >   IMCR. 
> > 
> > - PGPDO/PGPDI Parallel GPIO Pad Data Out/In
> >   16 bit registers where each bit(besides some gaps) represents
> >   a GPIO's output/input value
> > 
> > - DISR0, DIRER0, IREER0, IFEER0
> >   These registers are used for: status, enable, rising/falling edge
> >   configuration for interrupts. We have 32 interrupts called EIRQ and
> >   each interrupt has one or more pads associated with it (controlled
> >   by an IMCR register per EIRQ).
> > 
> >   However, one important thing to note is that even though there are
> >   EIRQs for SIUL2_0 pads, all the interrupt registers mentioned above
> >   are only present in SIUL2_1.
> > 
> > Because of mixed pins (I2C0_SDA in the example above with the MSCR
> > in SIUL2_1 for pad 122 and the IMCR in SIUL2_0) and the interrupt
> > configuration registers in SIUL2_1 we decided to have a single
> > driver instance.
> > 
> > > 
> > > 
> > > Cheers,
> > > Conor.
> > > 
> > 
> > Best regards,
> > Andrei
> > 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Andrei Stefanescu]

Hi Conor,

On 30/09/2024 18:07, Conor Dooley wrote:
> On Mon, Sep 30, 2024 at 04:00:57PM +0100, Conor Dooley wrote:
>> On Fri, Sep 27, 2024 at 10:13:54AM +0300, Andrei Stefanescu wrote:
>>> Hi Conor,
>>>
>>> Thank you very much for the prompt review!
>>>
>>> On 26/09/2024 18:38, Conor Dooley wrote:
>>>> On Thu, Sep 26, 2024 at 05:31:19PM +0300, Andrei Stefanescu wrote:
>>>>> Add support for the GPIO driver of the NXP S32G2/S32G3 SoCs.
>>>>>
>>>>> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
>>>>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>>>>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>>>>> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
>>>>
>>>> What's up with this SoB chain? You're the author what did
>>>> the other 3 people do? Are they missing co-developed-by tags?
>>>
>>> Yes, thank you for suggesting it! I will also add Co-developed-by tags
>>> for them. In the end it should look like this:
>>>
>>> Co-developed-by: Phu Luu An <phu.luuan@nxp.com>
>>> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
>>> Co-developed-by: Larisa Grigore <larisa.grigore@nxp.com>
>>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>>> Co-developed-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>>> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
>>>
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>>> +    #include <dt-bindings/interrupt-controller/irq.h>
>>>>> +
>>>>> +    gpio@4009d700 {
>>>>> +        compatible = "nxp,s32g2-siul2-gpio";
>>>>> +        reg = <0x4009d700 0x10>,
>>>>> +              <0x44011700 0x18>,
>>>>> +              <0x4009d740 0x10>,
>>>>> +              <0x44011740 0x18>,
>>>>> +              <0x44010010 0xb4>,
>>>>> +              <0x44011078 0x80>;
>>>>
>>>> Huh, I only noticed this now. Are you sure that this is a correct
>>>> representation of this device, and it is not really part of some syscon?
>>>> The "random" nature of the addresses  and the tiny sizes of the
>>>> reservations make it seem that way. What other devices are in these
>>>> regions?
>>
>> Thanks for your answer to my second question, but I think you missed this
>> part here ^^^
> 
> Reading it again, I think you might have answered my first question,
> though not explicitly. The regions in question do both pinctrl and gpio,
> but you have chosen to represent it has lots of mini register regions,
> rather than as a simple-mfd type device - which I think would be the
> correct representation. .

Yes, SIUL2 is mostly used for pinctrl and GPIO. The only other uses case is
to register a nvmem device for the first two registers in the SIUL2 MIDR1/MIDR2
(MCU ID Register) which tell us information about the SoC (revision,
SRAM size and so on).

I will convert the SIUL2 node into a simple-mfd device and switch the
GPIO and pinctrl drivers to use the syscon regmap in v5.

Best regards,
Andrei

 
> Cheers,
> Conor.
> 
>>
>>>>
>>>> Additionally, it looks like "opads0" and "ipads0" are in a different
>>>> region to their "1" equivalents. Should this really be represented as
>>>> two disctint GPIO controllers?
>>>
>>> I will add a bit more context regarding the hardware.
>>>
>>> The hardware module which implements pinctrl & GPIO is called SIUL2.
>>> For both S32G2 and S32G3 we have the same version of the module and 
>>> it is integrated in the same way. Each SoC has two SIUL2 instances which
>>> mostly have the same register types and only differ in the number
>>> of pads associated to them:
>>>
>>> - SIUL2_0 mapped at address 0x4009c000, handling pins 0 - 101
>>> - SIUL2_1 mapped at address 0x44010000, handling pins 112 - 190
>>>
>>> There are multiple registers for the SIUL2 modules which are important
>>> for pinctrl & GPIO:
>>>
>>> - MSCR (Multiplexed Signal Configuration Register)
>>>   It configures the function of a pin and some
>>>   pinconf properties:
>>>     - input buffer
>>>     - output buffer
>>>     - open-drain
>>>     - pull-up/pull-down
>>>     - slew rate
>>>   Function 0 means the pin is to be used as a GPIO.
>>>
>>> - IMCR (Input Multiplexed Signal and Configuration Register)
>>>   If the signal on this pad is to be read by another hardware
>>>   module, this register is similar to a multiplexer, its value
>>>   configures which pad the hardware will link to the module.
>>>   As an example let's consider the I2C0 SDA line. It has one
>>>   IMCR associated to it. Below are its possible pins and
>>>   corresponding IMCR values:
>>>     pin 16 <- 2
>>>     pin 24 <- 7
>>>     pin 31 <- 3
>>>     pin 122 <- 4 
>>>       (Note that MSCR122 is part of SIUL2_1 but the IMCR for
>>>        I2C0_SDA is part of SIUL2_0)
>>>     pin 153 <- 5
>>>     pin 161 <- 6
>>>   The IMCR values should be aligned with the function bits in the
>>>   MSCR bits. If we want to use pin 122 for I2C0_SDA we will configure
>>>   the function bits in MSCR122 and write the value 4 to the I2C0_SDA
>>>   IMCR. 
>>>
>>> - PGPDO/PGPDI Parallel GPIO Pad Data Out/In
>>>   16 bit registers where each bit(besides some gaps) represents
>>>   a GPIO's output/input value
>>>
>>> - DISR0, DIRER0, IREER0, IFEER0
>>>   These registers are used for: status, enable, rising/falling edge
>>>   configuration for interrupts. We have 32 interrupts called EIRQ and
>>>   each interrupt has one or more pads associated with it (controlled
>>>   by an IMCR register per EIRQ).
>>>
>>>   However, one important thing to note is that even though there are
>>>   EIRQs for SIUL2_0 pads, all the interrupt registers mentioned above
>>>   are only present in SIUL2_1.
>>>
>>> Because of mixed pins (I2C0_SDA in the example above with the MSCR
>>> in SIUL2_1 for pad 122 and the IMCR in SIUL2_0) and the interrupt
>>> configuration registers in SIUL2_1 we decided to have a single
>>> driver instance.
>>>
>>>>
>>>>
>>>> Cheers,
>>>> Conor.
>>>>
>>>
>>> Best regards,
>>> Andrei
>>>
> 
>
 

Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Andrei Stefanescu]

Hi Conor,

>>>>>
>>>>> Huh, I only noticed this now. Are you sure that this is a correct
>>>>> representation of this device, and it is not really part of some syscon?
>>>>> The "random" nature of the addresses  and the tiny sizes of the
>>>>> reservations make it seem that way. What other devices are in these
>>>>> regions?
>>>
>>> Thanks for your answer to my second question, but I think you missed this
>>> part here ^^^
>>
>> Reading it again, I think you might have answered my first question,
>> though not explicitly. The regions in question do both pinctrl and gpio,
>> but you have chosen to represent it has lots of mini register regions,
>> rather than as a simple-mfd type device - which I think would be the
>> correct representation. .
> 
> Yes, SIUL2 is mostly used for pinctrl and GPIO. The only other uses case is
> to register a nvmem device for the first two registers in the SIUL2 MIDR1/MIDR2
> (MCU ID Register) which tell us information about the SoC (revision,
> SRAM size and so on).
> 
> I will convert the SIUL2 node into a simple-mfd device and switch the
> GPIO and pinctrl drivers to use the syscon regmap in v5.

I replied in the other patch series
https://lore.kernel.org/all/a924bbb6-96ec-40be-9d82-a76b2ab73afd@oss.nxp.com/
that I actually decided to unify the pinctrl&GPIO drivers instead of making
them mfd_cells.

I have a question regarding the NVMEM driver that I mentioned earlier. I haven't
yet created a patch series to upstream it but I wanted to discuss about it
here since it relates to SIUL2 and, in the future, we would like to upstream it
as well.

We register a NVMEM driver for the first two registers of SIUL2 which can
then be read by other drivers to get information about the SoC. I think
there are two options for integrating it:

- Separate it from the pinctrl&GPIO driver as if it were part of a different
IP. This would look something like this in the device tree

/* SIUL2_0 base address is 0x4009c000 */
/* SIUL2_1 base address is 0x44010000 */

nvmem1@4009c000 {
	/* The registers are 32bit wide but start at offset 0x4 */
	reg = <0x4009c000 0xc>;
	[..]
};

pinctrl-gpio@4009c010 {
	reg = <0x4009c010 0xb84>,  /* SIUL2_0 32bit registers */
	      <0x4009d700 0x50>,   /* SIUL2_0 16bit registers */
	      <0x44010010 0x11f0>, /* SIUL2_1 32bit registers */
	      <0x4401170c 0x4c>,   /* SIUL2_1 16bit registers */  
	[..]
};

nvmem2@0x44010000 {
	reg = <0x44010000 0xc>;
	[..]
}

- have the nvmem as an mfd cell and the pinctrl&GPIO as another mfd cell

The first option keeps the nvmem completely separated from pinctrl&GPIO
but it makes the pinctrl&GPIO node start at an "odd" address. The second one
more accurately represents the hardware (since the functionality is part of
the same hardware block) but I am not sure if adding the mfd layer would add
any benefit since the two functionalities don't have any shared resources in
common.

What do you think?

Best regards,
Andrei


> 
> Best regards,
> Andrei
> 
>  
>> Cheers,
>> Conor.
>>
>>>
>>>>>
>>>>> Additionally, it looks like "opads0" and "ipads0" are in a different
>>>>> region to their "1" equivalents. Should this really be represented as
>>>>> two disctint GPIO controllers?
>>>>
>>>> I will add a bit more context regarding the hardware.
>>>>
>>>> The hardware module which implements pinctrl & GPIO is called SIUL2.
>>>> For both S32G2 and S32G3 we have the same version of the module and 
>>>> it is integrated in the same way. Each SoC has two SIUL2 instances which
>>>> mostly have the same register types and only differ in the number
>>>> of pads associated to them:
>>>>
>>>> - SIUL2_0 mapped at address 0x4009c000, handling pins 0 - 101
>>>> - SIUL2_1 mapped at address 0x44010000, handling pins 112 - 190
>>>>
>>>> There are multiple registers for the SIUL2 modules which are important
>>>> for pinctrl & GPIO:
>>>>
>>>> - MSCR (Multiplexed Signal Configuration Register)
>>>>   It configures the function of a pin and some
>>>>   pinconf properties:
>>>>     - input buffer
>>>>     - output buffer
>>>>     - open-drain
>>>>     - pull-up/pull-down
>>>>     - slew rate
>>>>   Function 0 means the pin is to be used as a GPIO.
>>>>
>>>> - IMCR (Input Multiplexed Signal and Configuration Register)
>>>>   If the signal on this pad is to be read by another hardware
>>>>   module, this register is similar to a multiplexer, its value
>>>>   configures which pad the hardware will link to the module.
>>>>   As an example let's consider the I2C0 SDA line. It has one
>>>>   IMCR associated to it. Below are its possible pins and
>>>>   corresponding IMCR values:
>>>>     pin 16 <- 2
>>>>     pin 24 <- 7
>>>>     pin 31 <- 3
>>>>     pin 122 <- 4 
>>>>       (Note that MSCR122 is part of SIUL2_1 but the IMCR for
>>>>        I2C0_SDA is part of SIUL2_0)
>>>>     pin 153 <- 5
>>>>     pin 161 <- 6
>>>>   The IMCR values should be aligned with the function bits in the
>>>>   MSCR bits. If we want to use pin 122 for I2C0_SDA we will configure
>>>>   the function bits in MSCR122 and write the value 4 to the I2C0_SDA
>>>>   IMCR. 
>>>>
>>>> - PGPDO/PGPDI Parallel GPIO Pad Data Out/In
>>>>   16 bit registers where each bit(besides some gaps) represents
>>>>   a GPIO's output/input value
>>>>
>>>> - DISR0, DIRER0, IREER0, IFEER0
>>>>   These registers are used for: status, enable, rising/falling edge
>>>>   configuration for interrupts. We have 32 interrupts called EIRQ and
>>>>   each interrupt has one or more pads associated with it (controlled
>>>>   by an IMCR register per EIRQ).
>>>>
>>>>   However, one important thing to note is that even though there are
>>>>   EIRQs for SIUL2_0 pads, all the interrupt registers mentioned above
>>>>   are only present in SIUL2_1.
>>>>
>>>> Because of mixed pins (I2C0_SDA in the example above with the MSCR
>>>> in SIUL2_1 for pad 122 and the IMCR in SIUL2_0) and the interrupt
>>>> configuration registers in SIUL2_1 we decided to have a single
>>>> driver instance.
>>>>
>>>>>
>>>>>
>>>>> Cheers,
>>>>> Conor.
>>>>>
>>>>
>>>> Best regards,
>>>> Andrei
>>>>
>>
>>
>  
> 

Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4348 bytes --]

On Thu, Oct 03, 2024 at 01:22:35PM +0300, Andrei Stefanescu wrote:
> Hi Conor,
> 
> >>>>>
> >>>>> Huh, I only noticed this now. Are you sure that this is a correct
> >>>>> representation of this device, and it is not really part of some syscon?
> >>>>> The "random" nature of the addresses  and the tiny sizes of the
> >>>>> reservations make it seem that way. What other devices are in these
> >>>>> regions?
> >>>
> >>> Thanks for your answer to my second question, but I think you missed this
> >>> part here ^^^
> >>
> >> Reading it again, I think you might have answered my first question,
> >> though not explicitly. The regions in question do both pinctrl and gpio,
> >> but you have chosen to represent it has lots of mini register regions,
> >> rather than as a simple-mfd type device - which I think would be the
> >> correct representation. .
> > 
> > Yes, SIUL2 is mostly used for pinctrl and GPIO. The only other uses case is
> > to register a nvmem device for the first two registers in the SIUL2 MIDR1/MIDR2
> > (MCU ID Register) which tell us information about the SoC (revision,
> > SRAM size and so on).
> > 
> > I will convert the SIUL2 node into a simple-mfd device and switch the
> > GPIO and pinctrl drivers to use the syscon regmap in v5.
> 
> I replied in the other patch series
> https://lore.kernel.org/all/a924bbb6-96ec-40be-9d82-a76b2ab73afd@oss.nxp.com/
> that I actually decided to unify the pinctrl&GPIO drivers instead of making
> them mfd_cells.

Yeah, I'm sorry I didn't reply to that sooner. I was being a lazy shit,
and read a book instead of replying yesterday. Almost did it again today
too...

To answer the question there, about simple-mfd/syscon not being quite
right:
I guess you aren't a simple-mfd, but this region does seem to be an mfd
to me, given it has 3 features. I wouldn't object to this being a single
node/device with two reg regions, given you're saying that the SIUL2_0
and SIUL2_1 registers both are required for the SIUL2 device to work but
are in different regions of the memory map.

> I have a question regarding the NVMEM driver that I mentioned earlier. I haven't
> yet created a patch series to upstream it but I wanted to discuss about it
> here since it relates to SIUL2 and, in the future, we would like to upstream it
> as well.
> 
> We register a NVMEM driver for the first two registers of SIUL2 which can
> then be read by other drivers to get information about the SoC. I think
> there are two options for integrating it:
> 
> - Separate it from the pinctrl&GPIO driver as if it were part of a different
> IP. This would look something like this in the device tree
> 
> /* SIUL2_0 base address is 0x4009c000 */
> /* SIUL2_1 base address is 0x44010000 */
> 
> nvmem1@4009c000 {
> 	/* The registers are 32bit wide but start at offset 0x4 */
> 	reg = <0x4009c000 0xc>;
> 	[..]
> };
> 
> pinctrl-gpio@4009c010 {
> 	reg = <0x4009c010 0xb84>,  /* SIUL2_0 32bit registers */
> 	      <0x4009d700 0x50>,   /* SIUL2_0 16bit registers */
> 	      <0x44010010 0x11f0>, /* SIUL2_1 32bit registers */
> 	      <0x4401170c 0x4c>,   /* SIUL2_1 16bit registers */  
> 	[..]
> };
> 
> nvmem2@0x44010000 {
> 	reg = <0x44010000 0xc>;
> 	[..]
> }
> 
> - have the nvmem as an mfd cell and the pinctrl&GPIO as another mfd cell
> 
> The first option keeps the nvmem completely separated from pinctrl&GPIO
> but it makes the pinctrl&GPIO node start at an "odd" address. The second one
> more accurately represents the hardware (since the functionality is part of
> the same hardware block) but I am not sure if adding the mfd layer would add
> any benefit since the two functionalities don't have any shared resources in
> common.

That's kinda what mfd is for innit, multiple (disparate) functions. I'm
not sure that you need an nvmem child node though, you may be able to
"just" ref nvmem.yaml, but I am not 100% sure how that interacts with
the pinctrl child node you will probably want to house pinctrl
properties in. The mfd driver would be capable of registering drivers
for each of the functions, you don't need to have a child node and a
compatible to register them. Cos of that, you shouldn't really require
a child node for GPIO, the gpio controller properties could go in the
mfd node itself.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Andrei Stefanescu]

Hi Conor,

On 04/10/2024 00:01, Conor Dooley wrote:
> On Thu, Oct 03, 2024 at 01:22:35PM +0300, Andrei Stefanescu wrote:
>> Hi Conor,
>>
>>>>>>>
>>>>>>> Huh, I only noticed this now. Are you sure that this is a correct
>>>>>>> representation of this device, and it is not really part of some syscon?
>>>>>>> The "random" nature of the addresses  and the tiny sizes of the
>>>>>>> reservations make it seem that way. What other devices are in these
>>>>>>> regions?
>>>>>
>>>>> Thanks for your answer to my second question, but I think you missed this
>>>>> part here ^^^
>>>>
>>>> Reading it again, I think you might have answered my first question,
>>>> though not explicitly. The regions in question do both pinctrl and gpio,
>>>> but you have chosen to represent it has lots of mini register regions,
>>>> rather than as a simple-mfd type device - which I think would be the
>>>> correct representation. .
>>>
>>> Yes, SIUL2 is mostly used for pinctrl and GPIO. The only other uses case is
>>> to register a nvmem device for the first two registers in the SIUL2 MIDR1/MIDR2
>>> (MCU ID Register) which tell us information about the SoC (revision,
>>> SRAM size and so on).
>>>
>>> I will convert the SIUL2 node into a simple-mfd device and switch the
>>> GPIO and pinctrl drivers to use the syscon regmap in v5.
>>
>> I replied in the other patch series
>> https://lore.kernel.org/all/a924bbb6-96ec-40be-9d82-a76b2ab73afd@oss.nxp.com/
>> that I actually decided to unify the pinctrl&GPIO drivers instead of making
>> them mfd_cells.
> 
> Yeah, I'm sorry I didn't reply to that sooner. I was being a lazy shit,
> and read a book instead of replying yesterday. Almost did it again today
> too...
> 
> To answer the question there, about simple-mfd/syscon not being quite
> right:
> I guess you aren't a simple-mfd, but this region does seem to be an mfd
> to me, given it has 3 features. I wouldn't object to this being a single
> node/device with two reg regions, given you're saying that the SIUL2_0
> and SIUL2_1 registers both are required for the SIUL2 device to work but
> are in different regions of the memory map.
> 
>> I have a question regarding the NVMEM driver that I mentioned earlier. I haven't
>> yet created a patch series to upstream it but I wanted to discuss about it
>> here since it relates to SIUL2 and, in the future, we would like to upstream it
>> as well.
>>
>> We register a NVMEM driver for the first two registers of SIUL2 which can
>> then be read by other drivers to get information about the SoC. I think
>> there are two options for integrating it:
>>
>> - Separate it from the pinctrl&GPIO driver as if it were part of a different
>> IP. This would look something like this in the device tree
>>
>> /* SIUL2_0 base address is 0x4009c000 */
>> /* SIUL2_1 base address is 0x44010000 */
>>
>> nvmem1@4009c000 {
>> 	/* The registers are 32bit wide but start at offset 0x4 */
>> 	reg = <0x4009c000 0xc>;
>> 	[..]
>> };
>>
>> pinctrl-gpio@4009c010 {
>> 	reg = <0x4009c010 0xb84>,  /* SIUL2_0 32bit registers */
>> 	      <0x4009d700 0x50>,   /* SIUL2_0 16bit registers */
>> 	      <0x44010010 0x11f0>, /* SIUL2_1 32bit registers */
>> 	      <0x4401170c 0x4c>,   /* SIUL2_1 16bit registers */  
>> 	[..]
>> };
>>
>> nvmem2@0x44010000 {
>> 	reg = <0x44010000 0xc>;
>> 	[..]
>> }
>>
>> - have the nvmem as an mfd cell and the pinctrl&GPIO as another mfd cell
>>
>> The first option keeps the nvmem completely separated from pinctrl&GPIO
>> but it makes the pinctrl&GPIO node start at an "odd" address. The second one
>> more accurately represents the hardware (since the functionality is part of
>> the same hardware block) but I am not sure if adding the mfd layer would add
>> any benefit since the two functionalities don't have any shared resources in
>> common.
> 
> That's kinda what mfd is for innit, multiple (disparate) functions. I'm
> not sure that you need an nvmem child node though, you may be able to
> "just" ref nvmem.yaml, but I am not 100% sure how that interacts with
> the pinctrl child node you will probably want to house pinctrl
> properties in. The mfd driver would be capable of registering drivers
> for each of the functions, you don't need to have a child node and a
> compatible to register them. Cos of that, you shouldn't really require
> a child node for GPIO, the gpio controller properties could go in the
> mfd node itself.

Just to confirm that I got it right, SIUL2 would end up being a single node,
looking something like:


siul2: siul2@4009c000 {
	compatible = "nxp,s32g2-siul2";
	reg = <0x4009C000 SIUL2_0_SIZE>,
	      <0x44010000 SIUL2_1_SIZE>;
	gpio-controller;
	#gpio-cells = <2>;
	gpio-ranges = <&siul2 0 0 102>, <&siul2 112 112 79>;
	gpio-reserved-ranges = <102 10>, <123 21>;
	interrupt-controller;
	#interrupt-cells = <2>;
	interrupts = <GIC_SPI ..>;

	/* for nvmem */
	#address-cells = <1>;
	#size-cells = <1>;

	*-nvmem-*@index {
		reg = <index 0x4>;
		[..]	
	};

	*-pins-* {
		pinmux = <...>
		[..]
	};
};

The siul2 node is for a mfd driver which will have two mfd_cells:
- one for the combined pinctrl&GPIO
- one for the nvmem driver

I think I can distinguish between nvmem cells and pinctrl functions
based on the presence of the pinmux property. Otherwise, I could
create two subnodes in the siul2 node for them:

siul2 {
	[..]
	nvmem-cells {
		*-nvmem-*@index {
			reg = <index 0x4>;
			[..]	
		};
	};

	pinctrl-functions {
		*-pins-* {
			pinmux = <...>
			[..]
		};
	};
	[..]
};


The siul2 driver will pass to the mfd_cells:
- access to the multiple regmaps(2 * SIUL2 each with 32bit and 16bit registers)
- the interrupt resource
- nvmem cells

This also implies that I should add a new separate binding for the siul2 node and
deprecate the existing pinctrl one(nxp,s32g2-siul2-pinctrl.yaml).

Would that be right?


Best regards,
Andrei

Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Krzysztof Kozlowski]

On 04/10/2024 13:10, Andrei Stefanescu wrote:
> 
> Just to confirm that I got it right, SIUL2 would end up being a single node,
> looking something like:
> 
> 
> siul2: siul2@4009c000 {
> 	compatible = "nxp,s32g2-siul2";
> 	reg = <0x4009C000 SIUL2_0_SIZE>,
> 	      <0x44010000 SIUL2_1_SIZE>;
> 	gpio-controller;
> 	#gpio-cells = <2>;
> 	gpio-ranges = <&siul2 0 0 102>, <&siul2 112 112 79>;
> 	gpio-reserved-ranges = <102 10>, <123 21>;
> 	interrupt-controller;
> 	#interrupt-cells = <2>;
> 	interrupts = <GIC_SPI ..>;
> 
> 	/* for nvmem */
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 
> 	*-nvmem-*@index {
> 		reg = <index 0x4>;
> 		[..]	

This looks like using deprecated binding. Switch to the non-deprecated
cells.


Best regards,
Krzysztof

Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Rob Herring (Arm)]

On Thu, 26 Sep 2024 17:31:19 +0300, Andrei Stefanescu wrote:
> Add support for the GPIO driver of the NXP S32G2/S32G3 SoCs.
> 
> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> ---
>  .../bindings/gpio/nxp,s32g2-siul2-gpio.yaml   | 110 ++++++++++++++++++
>  1 file changed, 110 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml:25:9: [warning] wrong indentation: expected 10 but found 8 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240926143122.1385658-3-andrei.stefanescu@oss.nxp.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Andrei Stefanescu]

Hi,

On 26/09/2024 20:43, Rob Herring (Arm) wrote:
> 
> On Thu, 26 Sep 2024 17:31:19 +0300, Andrei Stefanescu wrote:
>> Add support for the GPIO driver of the NXP S32G2/S32G3 SoCs.
>>
>> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
>> ---
>>  .../bindings/gpio/nxp,s32g2-siul2-gpio.yaml   | 110 ++++++++++++++++++
>>  1 file changed, 110 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml
>>
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml:25:9: [warning] wrong indentation: expected 10 but found 8 (indentation)

I don't get this error locally:

$ make DT_SCHEMA_FILES=nxp,s32g2-siul2-gpio.yaml dt_binding_check
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
  CHKDT   Documentation/devicetree/bindings
  LINT    Documentation/devicetree/bindings
  DTEX    Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.example.dts
  DTC [C] Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.example.dtb

$ pip3 show dtschema
Name: dtschema
Version: 2024.9

$ yamllint --version
yamllint 1.35.1


Lines around 25:

 20 properties:
 21   compatible:
 22     oneOf:
 23       - description: for S32G2 SoCs
 24         items:
 25           - const: nxp,s32g2-siul2-gpio
 26       - description: for S32G3 SoCs
 27         items:
 28           - const: nxp,s32g3-siul2-gpio
 29           - const: nxp,s32g2-siul2-gpio

I initially had the reported error but I fixed it locally by adding the following:

- description: [..]
  items:

Any ideas?

Best regards,
Andrei

> 
> dtschema/dtc warnings/errors:
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240926143122.1385658-3-andrei.stefanescu@oss.nxp.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 

Re: [PATCH v4 2/4] dt-bindings: gpio: add support for NXP S32G2/S32G3 SoCs

[Krzysztof Kozlowski]

On 27/09/2024 09:19, Andrei Stefanescu wrote:
> Hi,
> 
> On 26/09/2024 20:43, Rob Herring (Arm) wrote:
>>
>> On Thu, 26 Sep 2024 17:31:19 +0300, Andrei Stefanescu wrote:
>>> Add support for the GPIO driver of the NXP S32G2/S32G3 SoCs.
>>>
>>> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
>>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>>> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
>>> ---
>>>  .../bindings/gpio/nxp,s32g2-siul2-gpio.yaml   | 110 ++++++++++++++++++
>>>  1 file changed, 110 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml
>>>
>>
>> My bot found errors running 'make dt_binding_check' on your patch:
>>
>> yamllint warnings/errors:
>> ./Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml:25:9: [warning] wrong indentation: expected 10 but found 8 (indentation)
> 
> I don't get this error locally:

Really?

The code is clearly not correct, wrong indentation, so if you do not see
the error, then something in your setup needs to be fixed.

> 
> $ make DT_SCHEMA_FILES=nxp,s32g2-siul2-gpio.yaml dt_binding_check
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>   CHKDT   Documentation/devicetree/bindings
>   LINT    Documentation/devicetree/bindings
>   DTEX    Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.example.dts
>   DTC [C] Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.example.dtb
> 
> $ pip3 show dtschema
> Name: dtschema
> Version: 2024.9
> 
> $ yamllint --version
> yamllint 1.35.1
> 
> 
> Lines around 25:
> 
>  20 properties:
>  21   compatible:
>  22     oneOf:
>  23       - description: for S32G2 SoCs
>  24         items:
>  25           - const: nxp,s32g2-siul2-gpio
>  26       - description: for S32G3 SoCs
>  27         items:
>  28           - const: nxp,s32g3-siul2-gpio
>  29           - const: nxp,s32g2-siul2-gpio
> 
> I initially had the reported error but I fixed it locally by adding the following:

I don't understand. So you had the error, but you fixed it, but you sent
wrong patch with the error?

This is just confusing. Re-apply this patch (b4 shazam) and test it
again. Please keep testing till you see the error, so you know your
environment works properly.

Best regards,
Krzysztof

[PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Andrei Stefanescu]

Add the GPIO driver for S32G2/S32G3 SoCs. This driver uses the SIUL2
(System Integration Unit Lite2) hardware module. There are two
SIUL2 hardware modules present, SIUL2_0(controlling GPIOs 0-101) and
SIUL2_1 for the rest.

The GPIOs are not fully contiguous, there are some gaps:
- GPIO102 up to GPIO111(inclusive) are invalid
- GPIO123 up to GPIO143(inclusive) are invalid

Some GPIOs are input only(i.e. GPI182) though this restriction
is not yet enforced in code.

This patch adds basic GPIO functionality(no interrupts, no
suspend/resume functions).

Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
 drivers/gpio/Kconfig            |  10 +
 drivers/gpio/Makefile           |   1 +
 drivers/gpio/gpio-siul2-s32g2.c | 576 ++++++++++++++++++++++++++++++++
 3 files changed, 587 insertions(+)
 create mode 100644 drivers/gpio/gpio-siul2-s32g2.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d93cd4f722b4..ae6aa6f64db3 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -643,6 +643,16 @@ config GPIO_SIOX
 	  Say yes here to support SIOX I/O devices. These are units connected
 	  via a SIOX bus and have a number of fixed-direction I/O lines.
 
+config GPIO_SIUL2_S32G2
+        tristate "GPIO driver for S32G2/S32G3"
+        depends on ARCH_S32 || COMPILE_TEST
+        depends on OF_GPIO
+        select REGMAP_MMIO
+        help
+          This enables support for the SIUL2 GPIOs found on the S32G2/S32G3
+          chips. Say yes here to enable the SIUL2 to be used as an GPIO
+          controller for S32G2/S32G3 platforms.
+
 config GPIO_SNPS_CREG
 	bool "Synopsys GPIO via CREG (Control REGisters) driver"
 	depends on ARC || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 1429e8c0229b..8d5f35689fed 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -150,6 +150,7 @@ obj-$(CONFIG_GPIO_SCH)			+= gpio-sch.o
 obj-$(CONFIG_GPIO_SIFIVE)		+= gpio-sifive.o
 obj-$(CONFIG_GPIO_SIM)			+= gpio-sim.o
 obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
+obj-$(CONFIG_GPIO_SIUL2_S32G2)		+= gpio-siul2-s32g2.o
 obj-$(CONFIG_GPIO_SL28CPLD)		+= gpio-sl28cpld.o
 obj-$(CONFIG_GPIO_SLOPPY_LOGIC_ANALYZER) += gpio-sloppy-logic-analyzer.o
 obj-$(CONFIG_GPIO_SODAVILLE)		+= gpio-sodaville.o
diff --git a/drivers/gpio/gpio-siul2-s32g2.c b/drivers/gpio/gpio-siul2-s32g2.c
new file mode 100644
index 000000000000..d9c04aacb3cc
--- /dev/null
+++ b/drivers/gpio/gpio-siul2-s32g2.c
@@ -0,0 +1,576 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * SIUL2 GPIO support.
+ *
+ * Copyright (c) 2016 Freescale Semiconductor, Inc.
+ * Copyright 2018-2024 NXP
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/gpio/driver.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+/* PGPDOs are 16bit registers that come in big endian
+ * order if they are grouped in pairs of two.
+ *
+ * For example, the order is PGPDO1, PGPDO0, PGPDO3, PGPDO2...
+ */
+#define SIUL2_PGPDO(N)		(((N) ^ 1) * 2)
+#define S32G2_SIUL2_NUM		2
+#define S32G2_PADS_DTS_TAG_LEN	7
+
+#define SIUL2_GPIO_16_PAD_SIZE	16
+
+/**
+ * struct siul2_device_data  - platform data attached to the compatible.
+ * @pad_access: access table for I/O pads, consists of S32G2_SIUL2_NUM tables.
+ * @reset_cnt: reset the pin name counter to zero when switching to SIUL2_1.
+ */
+struct siul2_device_data {
+	const struct regmap_access_table **pad_access;
+	const bool reset_cnt;
+};
+
+/**
+ * struct siul2_desc - describes a SIUL2 hw module.
+ * @pad_access: array of valid I/O pads.
+ * @opadmap: the regmap of the Parallel GPIO Pad Data Out Register.
+ * @ipadmap: the regmap of the Parallel GPIO Pad Data In Register.
+ * @gpio_base: the first GPIO pin.
+ * @gpio_num: the number of GPIO pins.
+ */
+struct siul2_desc {
+	const struct regmap_access_table *pad_access;
+	struct regmap *opadmap;
+	struct regmap *ipadmap;
+	u32 gpio_base;
+	u32 gpio_num;
+};
+
+/**
+ * struct siul2_gpio_dev - describes a group of GPIO pins.
+ * @platdata: the platform data.
+ * @siul2: SIUL2_0 and SIUL2_1 modules information.
+ * @pin_dir_bitmap: the bitmap with pin directions.
+ * @gc: the GPIO chip.
+ * @lock: mutual access to bitmaps.
+ */
+struct siul2_gpio_dev {
+	const struct siul2_device_data *platdata;
+	struct siul2_desc siul2[S32G2_SIUL2_NUM];
+	unsigned long *pin_dir_bitmap;
+	struct gpio_chip gc;
+	raw_spinlock_t lock;
+};
+
+static const struct regmap_range s32g2_siul20_pad_yes_ranges[] = {
+	regmap_reg_range(SIUL2_PGPDO(0), SIUL2_PGPDO(0)),
+	regmap_reg_range(SIUL2_PGPDO(1), SIUL2_PGPDO(1)),
+	regmap_reg_range(SIUL2_PGPDO(2), SIUL2_PGPDO(2)),
+	regmap_reg_range(SIUL2_PGPDO(3), SIUL2_PGPDO(3)),
+	regmap_reg_range(SIUL2_PGPDO(4), SIUL2_PGPDO(4)),
+	regmap_reg_range(SIUL2_PGPDO(5), SIUL2_PGPDO(5)),
+	regmap_reg_range(SIUL2_PGPDO(6), SIUL2_PGPDO(6)),
+};
+
+static const struct regmap_access_table s32g2_siul20_pad_access_table = {
+	.yes_ranges	= s32g2_siul20_pad_yes_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(s32g2_siul20_pad_yes_ranges),
+};
+
+static const struct regmap_range s32g2_siul21_pad_yes_ranges[] = {
+	regmap_reg_range(SIUL2_PGPDO(7), SIUL2_PGPDO(7)),
+	regmap_reg_range(SIUL2_PGPDO(9), SIUL2_PGPDO(9)),
+	regmap_reg_range(SIUL2_PGPDO(10), SIUL2_PGPDO(10)),
+	regmap_reg_range(SIUL2_PGPDO(11), SIUL2_PGPDO(11)),
+};
+
+static const struct regmap_access_table s32g2_siul21_pad_access_table = {
+	.yes_ranges	= s32g2_siul21_pad_yes_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(s32g2_siul21_pad_yes_ranges),
+};
+
+static const struct regmap_access_table *s32g2_pad_access_table[] = {
+	&s32g2_siul20_pad_access_table,
+	&s32g2_siul21_pad_access_table
+};
+
+static_assert(ARRAY_SIZE(s32g2_pad_access_table) == S32G2_SIUL2_NUM);
+
+static const struct siul2_device_data s32g2_device_data = {
+	.pad_access	= s32g2_pad_access_table,
+	.reset_cnt	= true,
+};
+
+static int siul2_get_gpio_pinspec(struct platform_device *pdev,
+				  struct of_phandle_args *pinspec,
+				  unsigned int range_index)
+{
+	struct device_node *np = pdev->dev.of_node;
+
+	return of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
+						range_index, pinspec);
+}
+
+static struct regmap *siul2_offset_to_regmap(struct siul2_gpio_dev *dev,
+					     unsigned int offset,
+					     bool input)
+{
+	struct siul2_desc *siul2;
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(dev->siul2); i++) {
+		siul2 = &dev->siul2[i];
+		if (offset >= siul2->gpio_base &&
+		    offset - siul2->gpio_base < siul2->gpio_num)
+			return input ? siul2->ipadmap : siul2->opadmap;
+	}
+
+	return NULL;
+}
+
+static void siul2_gpio_set_direction(struct siul2_gpio_dev *dev,
+				     unsigned int gpio, int dir)
+{
+	guard(raw_spinlock_irqsave)(&dev->lock);
+
+	if (dir == GPIO_LINE_DIRECTION_IN)
+		__clear_bit(gpio, dev->pin_dir_bitmap);
+	else
+		__set_bit(gpio, dev->pin_dir_bitmap);
+}
+
+static int siul2_get_direction(struct siul2_gpio_dev *dev,
+			       unsigned int gpio)
+{
+	return test_bit(gpio, dev->pin_dir_bitmap) ? GPIO_LINE_DIRECTION_OUT :
+						     GPIO_LINE_DIRECTION_IN;
+}
+
+static struct siul2_gpio_dev *to_siul2_gpio_dev(struct gpio_chip *chip)
+{
+	return container_of(chip, struct siul2_gpio_dev, gc);
+}
+
+static int siul2_gpio_dir_in(struct gpio_chip *chip, unsigned int gpio)
+{
+	struct siul2_gpio_dev *gpio_dev;
+	int ret = 0;
+
+	ret = pinctrl_gpio_direction_input(chip, gpio);
+	if (ret)
+		return ret;
+
+	gpio_dev = to_siul2_gpio_dev(chip);
+	siul2_gpio_set_direction(gpio_dev, gpio, GPIO_LINE_DIRECTION_IN);
+
+	return 0;
+}
+
+static int siul2_gpio_get_dir(struct gpio_chip *chip, unsigned int gpio)
+{
+	return siul2_get_direction(to_siul2_gpio_dev(chip), gpio);
+}
+
+static unsigned int siul2_pin2pad(unsigned int pin)
+{
+	return pin / SIUL2_GPIO_16_PAD_SIZE;
+}
+
+static u16 siul2_pin2mask(unsigned int pin)
+{
+	/**
+	 * From Reference manual :
+	 * PGPDOx[PPDOy] = GPDO(x × 16) + (15 - y)[PDO_(x × 16) + (15 - y)]
+	 */
+	return BIT(SIUL2_GPIO_16_PAD_SIZE - 1 - pin % SIUL2_GPIO_16_PAD_SIZE);
+}
+
+static void siul2_gpio_set_val(struct gpio_chip *chip, unsigned int offset,
+			       int value)
+{
+	struct siul2_gpio_dev *gpio_dev = to_siul2_gpio_dev(chip);
+	unsigned int pad, reg_offset;
+	struct regmap *regmap;
+	u16 mask;
+
+	mask = siul2_pin2mask(offset);
+	pad = siul2_pin2pad(offset);
+
+	reg_offset = SIUL2_PGPDO(pad);
+	regmap = siul2_offset_to_regmap(gpio_dev, offset, false);
+	if (!regmap)
+		return;
+
+	value = value ? mask : 0;
+
+	regmap_update_bits(regmap, reg_offset, mask, value);
+}
+
+static int siul2_gpio_dir_out(struct gpio_chip *chip, unsigned int gpio,
+			      int val)
+{
+	struct siul2_gpio_dev *gpio_dev;
+	int ret = 0;
+
+	gpio_dev = to_siul2_gpio_dev(chip);
+	siul2_gpio_set_val(chip, gpio, val);
+
+	ret = pinctrl_gpio_direction_output(chip, gpio);
+	if (ret)
+		return ret;
+
+	siul2_gpio_set_direction(gpio_dev, gpio, GPIO_LINE_DIRECTION_OUT);
+
+	return 0;
+}
+
+static void siul2_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			   int value)
+{
+	struct siul2_gpio_dev *gpio_dev = to_siul2_gpio_dev(chip);
+
+	if (!gpio_dev)
+		return;
+
+	if (siul2_get_direction(gpio_dev, offset) == GPIO_LINE_DIRECTION_IN)
+		return;
+
+	siul2_gpio_set_val(chip, offset, value);
+}
+
+static int siul2_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct siul2_gpio_dev *gpio_dev = to_siul2_gpio_dev(chip);
+	unsigned int mask, pad, reg_offset, data = 0;
+	struct regmap *regmap;
+
+	mask = siul2_pin2mask(offset);
+	pad = siul2_pin2pad(offset);
+
+	reg_offset = SIUL2_PGPDO(pad);
+	regmap = siul2_offset_to_regmap(gpio_dev, offset, true);
+	if (!regmap)
+		return -EINVAL;
+
+	regmap_read(regmap, reg_offset, &data);
+
+	return !!(data & mask);
+}
+
+static const struct regmap_config siul2_regmap_conf = {
+	.val_bits = 32,
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.cache_type = REGCACHE_FLAT,
+};
+
+static struct regmap *common_regmap_init(struct platform_device *pdev,
+					 struct regmap_config *conf,
+					 const char *name)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	resource_size_t size;
+	void __iomem *base;
+
+	base = devm_platform_get_and_ioremap_resource_byname(pdev, name, &res);
+	if (IS_ERR(base)) {
+		dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
+		return ERR_PTR(-EINVAL);
+	}
+
+	size = resource_size(res);
+	conf->val_bits = conf->reg_stride * 8;
+	conf->max_register = size - conf->reg_stride;
+	conf->name = name;
+	conf->use_raw_spinlock = true;
+
+	if (conf->cache_type != REGCACHE_NONE)
+		conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
+
+	return devm_regmap_init_mmio(dev, base, conf);
+}
+
+static bool not_writable(__always_unused struct device *dev,
+			 __always_unused unsigned int reg)
+{
+	return false;
+}
+
+static struct regmap *init_padregmap(struct platform_device *pdev,
+				     struct siul2_gpio_dev *gpio_dev,
+				     int selector, bool input)
+{
+	const struct siul2_device_data *platdata = gpio_dev->platdata;
+	struct regmap_config regmap_conf = siul2_regmap_conf;
+	char dts_tag[S32G2_PADS_DTS_TAG_LEN];
+	int err;
+
+	regmap_conf.reg_stride = 2;
+
+	if (selector != 0 && selector != 1)
+		return ERR_PTR(-EINVAL);
+
+	regmap_conf.rd_table = platdata->pad_access[selector];
+
+	err = snprintf(dts_tag, ARRAY_SIZE(dts_tag),  "%cpads%d",
+		       input ? 'i' : 'o', selector);
+	if (err < 0)
+		return ERR_PTR(-EINVAL);
+
+	if (input) {
+		regmap_conf.writeable_reg = not_writable;
+		regmap_conf.cache_type = REGCACHE_NONE;
+	} else {
+		regmap_conf.wr_table = platdata->pad_access[selector];
+	}
+
+	return common_regmap_init(pdev, &regmap_conf, dts_tag);
+}
+
+static int siul2_gpio_pads_init(struct platform_device *pdev,
+				struct siul2_gpio_dev *gpio_dev)
+{
+	struct device *dev = &pdev->dev;
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(gpio_dev->siul2); i++) {
+		gpio_dev->siul2[i].opadmap = init_padregmap(pdev, gpio_dev, i,
+							    false);
+		if (IS_ERR(gpio_dev->siul2[i].opadmap)) {
+			dev_err(dev,
+				"Failed to initialize opad2%zu regmap config\n",
+				i);
+			return PTR_ERR(gpio_dev->siul2[i].opadmap);
+		}
+
+		gpio_dev->siul2[i].ipadmap = init_padregmap(pdev, gpio_dev, i,
+							    true);
+		if (IS_ERR(gpio_dev->siul2[i].ipadmap)) {
+			dev_err(dev,
+				"Failed to initialize ipad2%zu regmap config\n",
+				i);
+			return PTR_ERR(gpio_dev->siul2[i].ipadmap);
+		}
+	}
+
+	return 0;
+}
+
+static int siul2_gen_names(struct device *dev, unsigned int cnt, char **names,
+			   char *ch_index, unsigned int *num_index)
+{
+	unsigned int i;
+
+	for (i = 0; i < cnt; i++) {
+		if (i != 0 && !(*num_index % 16))
+			(*ch_index)++;
+
+		names[i] = devm_kasprintf(dev, GFP_KERNEL, "P%c_%02d",
+					  *ch_index, 0xFU & (*num_index)++);
+		if (!names[i])
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int siul2_gpio_remove_reserved_names(struct device *dev,
+					    struct siul2_gpio_dev *gpio_dev,
+					    char **names)
+{
+	struct device_node *np = dev->of_node;
+	int num_ranges, i, j, ret;
+	u32 base_gpio, num_gpio;
+
+	/* Parse the gpio-reserved-ranges to know which GPIOs to exclude. */
+
+	num_ranges = of_property_count_u32_elems(dev->of_node,
+						 "gpio-reserved-ranges");
+
+	/* The "gpio-reserved-ranges" is optional. */
+	if (num_ranges < 0)
+		return 0;
+	num_ranges /= 2;
+
+	for (i = 0; i < num_ranges; i++) {
+		ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
+						 i * 2, &base_gpio);
+		if (ret) {
+			dev_err(dev, "Could not parse the start GPIO: %d\n",
+				ret);
+			return ret;
+		}
+
+		ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
+						 i * 2 + 1, &num_gpio);
+		if (ret) {
+			dev_err(dev, "Could not parse num. GPIOs: %d\n", ret);
+			return ret;
+		}
+
+		if (base_gpio + num_gpio > gpio_dev->gc.ngpio) {
+			dev_err(dev, "Reserved GPIOs outside of GPIO range\n");
+			return -EINVAL;
+		}
+
+		/* Remove names set for reserved GPIOs. */
+		for (j = base_gpio; j < base_gpio + num_gpio; j++) {
+			devm_kfree(dev, names[j]);
+			names[j] = NULL;
+		}
+	}
+
+	return 0;
+}
+
+static int siul2_gpio_populate_names(struct device *dev,
+				     struct siul2_gpio_dev *gpio_dev)
+{
+	unsigned int num_index = 0;
+	char ch_index = 'A';
+	char **names;
+	int i, ret;
+
+	names = devm_kcalloc(dev, gpio_dev->gc.ngpio, sizeof(*names),
+			     GFP_KERNEL);
+	if (!names)
+		return -ENOMEM;
+
+	for (i = 0; i < S32G2_SIUL2_NUM; i++) {
+		ret = siul2_gen_names(dev, gpio_dev->siul2[i].gpio_num,
+				      names + gpio_dev->siul2[i].gpio_base,
+				      &ch_index, &num_index);
+		if (ret) {
+			dev_err(dev, "Could not set names for SIUL2_%d GPIOs\n",
+				i);
+			return ret;
+		}
+
+		if (gpio_dev->platdata->reset_cnt)
+			num_index = 0;
+
+		ch_index++;
+	}
+
+	ret = siul2_gpio_remove_reserved_names(dev, gpio_dev, names);
+	if (ret)
+		return ret;
+
+	gpio_dev->gc.names = (const char *const *)names;
+
+	return 0;
+}
+
+static int siul2_gpio_probe(struct platform_device *pdev)
+{
+	struct siul2_gpio_dev *gpio_dev;
+	struct device *dev = &pdev->dev;
+	struct of_phandle_args pinspec;
+	size_t i, bitmap_size;
+	struct gpio_chip *gc;
+	int ret = 0;
+
+	gpio_dev = devm_kzalloc(dev, sizeof(*gpio_dev), GFP_KERNEL);
+	if (!gpio_dev)
+		return -ENOMEM;
+
+	gpio_dev->platdata = &s32g2_device_data;
+
+	for (i = 0; i < S32G2_SIUL2_NUM; i++)
+		gpio_dev->siul2[i].pad_access =
+			gpio_dev->platdata->pad_access[i];
+
+	ret = siul2_gpio_pads_init(pdev, gpio_dev);
+	if (ret)
+		return ret;
+
+	gc = &gpio_dev->gc;
+
+	platform_set_drvdata(pdev, gpio_dev);
+
+	raw_spin_lock_init(&gpio_dev->lock);
+
+	for (i = 0; i < ARRAY_SIZE(gpio_dev->siul2); i++) {
+		ret = siul2_get_gpio_pinspec(pdev, &pinspec, i);
+		if (ret) {
+			dev_err(dev,
+				"unable to get pinspec %zu from device tree\n",
+				i);
+			return -EINVAL;
+		}
+
+		of_node_put(pinspec.np);
+
+		if (pinspec.args_count != 3) {
+			dev_err(dev, "Invalid pinspec count: %d\n",
+				pinspec.args_count);
+			return -EINVAL;
+		}
+
+		gpio_dev->siul2[i].gpio_base = pinspec.args[1];
+		gpio_dev->siul2[i].gpio_num = pinspec.args[2];
+	}
+
+	gc->base = -1;
+
+	/* In some cases, there is a gap between the SIUL GPIOs. */
+	gc->ngpio = gpio_dev->siul2[S32G2_SIUL2_NUM - 1].gpio_base +
+		    gpio_dev->siul2[S32G2_SIUL2_NUM - 1].gpio_num;
+
+	ret = siul2_gpio_populate_names(&pdev->dev, gpio_dev);
+	if (ret)
+		return ret;
+
+	bitmap_size = BITS_TO_LONGS(gc->ngpio) *
+		      sizeof(*gpio_dev->pin_dir_bitmap);
+	gpio_dev->pin_dir_bitmap = devm_kzalloc(dev, bitmap_size, GFP_KERNEL);
+	if (!gpio_dev->pin_dir_bitmap)
+		return -ENOMEM;
+
+	gc->parent = dev;
+	gc->label = dev_name(dev);
+
+	gc->set = siul2_gpio_set;
+	gc->get = siul2_gpio_get;
+	gc->set_config = gpiochip_generic_config;
+	gc->request = gpiochip_generic_request;
+	gc->free = gpiochip_generic_free;
+	gc->direction_output = siul2_gpio_dir_out;
+	gc->direction_input = siul2_gpio_dir_in;
+	gc->get_direction = siul2_gpio_get_dir;
+	gc->owner = THIS_MODULE;
+
+	ret = devm_gpiochip_add_data(dev, gc, gpio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "unable to add gpiochip\n");
+
+	return 0;
+}
+
+static const struct of_device_id siul2_gpio_dt_ids[] = {
+	{ .compatible = "nxp,s32g2-siul2-gpio" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, siul2_gpio_dt_ids);
+
+static struct platform_driver siul2_gpio_driver = {
+	.driver			= {
+		.name		= "s32g2-siul2-gpio",
+		.of_match_table = siul2_gpio_dt_ids,
+	},
+	.probe			= siul2_gpio_probe,
+};
+
+module_platform_driver(siul2_gpio_driver);
+
+MODULE_AUTHOR("NXP");
+MODULE_DESCRIPTION("NXP SIUL2 GPIO");
+MODULE_LICENSE("GPL");
-- 
2.45.2

Re: [PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[kernel test robot]

Hi Andrei,

kernel test robot noticed the following build errors:

[auto build test ERROR on brgl/gpio/for-next]
[also build test ERROR on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus robh/for-next linus/master v6.11 next-20240926]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrei-Stefanescu/drivers-provide-devm_platform_get_and_ioremap_resource_byname/20240926-223448
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20240926143122.1385658-4-andrei.stefanescu%40oss.nxp.com
patch subject: [PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support
config: openrisc-allyesconfig (https://download.01.org/0day-ci/archive/20240927/202409271406.SOjGw98h-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240927/202409271406.SOjGw98h-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409271406.SOjGw98h-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from ./arch/openrisc/include/generated/asm/div64.h:1,
                    from include/linux/math.h:6,
                    from include/linux/kernel.h:27,
                    from include/linux/cpumask.h:11,
                    from include/linux/smp.h:13,
                    from include/linux/lockdep.h:14,
                    from include/linux/spinlock.h:63,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:7,
                    from include/linux/mm.h:7,
                    from arch/openrisc/include/asm/pgalloc.h:20,
                    from arch/openrisc/include/asm/io.h:18,
                    from include/linux/io.h:14,
                    from drivers/gpio/gpio-siul2-s32g2.c:11:
   drivers/gpio/gpio-siul2-s32g2.c: In function 'common_regmap_init':
   include/asm-generic/div64.h:222:35: warning: comparison of distinct pointer types lacks a cast [-Wcompare-distinct-pointer-types]
     222 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
         |                                   ^~
   drivers/gpio/gpio-siul2-s32g2.c:296:46: note: in expansion of macro 'do_div'
     296 |                 conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
         |                                              ^~~~~~
   In file included from include/linux/err.h:5,
                    from drivers/gpio/gpio-siul2-s32g2.c:9:
   include/asm-generic/div64.h:234:32: warning: right shift count >= width of type [-Wshift-count-overflow]
     234 |         } else if (likely(((n) >> 32) == 0)) {          \
         |                                ^~
   include/linux/compiler.h:76:45: note: in definition of macro 'likely'
      76 | # define likely(x)      __builtin_expect(!!(x), 1)
         |                                             ^
   drivers/gpio/gpio-siul2-s32g2.c:296:46: note: in expansion of macro 'do_div'
     296 |                 conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
         |                                              ^~~~~~
>> include/asm-generic/div64.h:238:36: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Wincompatible-pointer-types]
     238 |                 __rem = __div64_32(&(n), __base);       \
         |                                    ^~~~
         |                                    |
         |                                    resource_size_t * {aka unsigned int *}
   drivers/gpio/gpio-siul2-s32g2.c:296:46: note: in expansion of macro 'do_div'
     296 |                 conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
         |                                              ^~~~~~
   include/asm-generic/div64.h:213:38: note: expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is of type 'resource_size_t *' {aka 'unsigned int *'}
     213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
         |                            ~~~~~~~~~~^~~~~~~~


vim +/__div64_32 +238 include/asm-generic/div64.h

^1da177e4c3f41 Linus Torvalds     2005-04-16  215  
^1da177e4c3f41 Linus Torvalds     2005-04-16  216  /* The unnecessary pointer compare is there
^1da177e4c3f41 Linus Torvalds     2005-04-16  217   * to check for type safety (n must be 64bit)
^1da177e4c3f41 Linus Torvalds     2005-04-16  218   */
^1da177e4c3f41 Linus Torvalds     2005-04-16  219  # define do_div(n,base) ({				\
^1da177e4c3f41 Linus Torvalds     2005-04-16  220  	uint32_t __base = (base);			\
^1da177e4c3f41 Linus Torvalds     2005-04-16  221  	uint32_t __rem;					\
^1da177e4c3f41 Linus Torvalds     2005-04-16  222  	(void)(((typeof((n)) *)0) == ((uint64_t *)0));	\
911918aa7ef6f8 Nicolas Pitre      2015-11-02  223  	if (__builtin_constant_p(__base) &&		\
911918aa7ef6f8 Nicolas Pitre      2015-11-02  224  	    is_power_of_2(__base)) {			\
911918aa7ef6f8 Nicolas Pitre      2015-11-02  225  		__rem = (n) & (__base - 1);		\
911918aa7ef6f8 Nicolas Pitre      2015-11-02  226  		(n) >>= ilog2(__base);			\
c747ce4706190e Geert Uytterhoeven 2021-08-11  227  	} else if (__builtin_constant_p(__base) &&	\
461a5e51060c93 Nicolas Pitre      2015-10-30  228  		   __base != 0) {			\
461a5e51060c93 Nicolas Pitre      2015-10-30  229  		uint32_t __res_lo, __n_lo = (n);	\
461a5e51060c93 Nicolas Pitre      2015-10-30  230  		(n) = __div64_const32(n, __base);	\
461a5e51060c93 Nicolas Pitre      2015-10-30  231  		/* the remainder can be computed with 32-bit regs */ \
461a5e51060c93 Nicolas Pitre      2015-10-30  232  		__res_lo = (n);				\
461a5e51060c93 Nicolas Pitre      2015-10-30  233  		__rem = __n_lo - __res_lo * __base;	\
911918aa7ef6f8 Nicolas Pitre      2015-11-02  234  	} else if (likely(((n) >> 32) == 0)) {		\
^1da177e4c3f41 Linus Torvalds     2005-04-16  235  		__rem = (uint32_t)(n) % __base;		\
^1da177e4c3f41 Linus Torvalds     2005-04-16  236  		(n) = (uint32_t)(n) / __base;		\
c747ce4706190e Geert Uytterhoeven 2021-08-11  237  	} else {					\
^1da177e4c3f41 Linus Torvalds     2005-04-16 @238  		__rem = __div64_32(&(n), __base);	\
c747ce4706190e Geert Uytterhoeven 2021-08-11  239  	}						\
^1da177e4c3f41 Linus Torvalds     2005-04-16  240  	__rem;						\
^1da177e4c3f41 Linus Torvalds     2005-04-16  241   })
^1da177e4c3f41 Linus Torvalds     2005-04-16  242  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[kernel test robot]

Hi Andrei,

kernel test robot noticed the following build warnings:

[auto build test WARNING on brgl/gpio/for-next]
[also build test WARNING on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus robh/for-next linus/master v6.11 next-20240927]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrei-Stefanescu/drivers-provide-devm_platform_get_and_ioremap_resource_byname/20240926-223448
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20240926143122.1385658-4-andrei.stefanescu%40oss.nxp.com
patch subject: [PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support
config: mips-randconfig-r123-20240928 (https://download.01.org/0day-ci/archive/20240928/202409280936.0GoVcXMc-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 14.1.0
reproduce: (https://download.01.org/0day-ci/archive/20240928/202409280936.0GoVcXMc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409280936.0GoVcXMc-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/gpio/gpio-siul2-s32g2.c:296:46: sparse: sparse: incompatible types in comparison expression (different type sizes):
   drivers/gpio/gpio-siul2-s32g2.c:296:46: sparse:    unsigned int *
   drivers/gpio/gpio-siul2-s32g2.c:296:46: sparse:    unsigned long long [usertype] *
   drivers/gpio/gpio-siul2-s32g2.c:143:9: sparse: sparse: context imbalance in 'siul2_gpio_set_direction' - wrong count at exit
   drivers/gpio/gpio-siul2-s32g2.c:296:46: sparse: sparse: shift too big (32) for type unsigned int

vim +296 drivers/gpio/gpio-siul2-s32g2.c

   273	
   274	static struct regmap *common_regmap_init(struct platform_device *pdev,
   275						 struct regmap_config *conf,
   276						 const char *name)
   277	{
   278		struct device *dev = &pdev->dev;
   279		struct resource *res;
   280		resource_size_t size;
   281		void __iomem *base;
   282	
   283		base = devm_platform_get_and_ioremap_resource_byname(pdev, name, &res);
   284		if (IS_ERR(base)) {
   285			dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
   286			return ERR_PTR(-EINVAL);
   287		}
   288	
   289		size = resource_size(res);
   290		conf->val_bits = conf->reg_stride * 8;
   291		conf->max_register = size - conf->reg_stride;
   292		conf->name = name;
   293		conf->use_raw_spinlock = true;
   294	
   295		if (conf->cache_type != REGCACHE_NONE)
 > 296			conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
   297	
   298		return devm_regmap_init_mmio(dev, base, conf);
   299	}
   300	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[kernel test robot]

Hi Andrei,

kernel test robot noticed the following build errors:

[auto build test ERROR on brgl/gpio/for-next]
[also build test ERROR on driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus robh/for-next linus/master v6.11 next-20240927]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andrei-Stefanescu/drivers-provide-devm_platform_get_and_ioremap_resource_byname/20240926-223448
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20240926143122.1385658-4-andrei.stefanescu%40oss.nxp.com
patch subject: [PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240928/202409281117.IOrJBXgL-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 7773243d9916f98ba0ffce0c3a960e4aa9f03e81)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240928/202409281117.IOrJBXgL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409281117.IOrJBXgL-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from drivers/gpio/gpio-siul2-s32g2.c:11:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/gpio/gpio-siul2-s32g2.c:11:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/gpio/gpio-siul2-s32g2.c:11:
   In file included from include/linux/io.h:14:
   In file included from arch/hexagon/include/asm/io.h:328:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
>> drivers/gpio/gpio-siul2-s32g2.c:296:32: warning: comparison of distinct pointer types ('typeof ((size)) *' (aka 'unsigned int *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
     296 |                 conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
         |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
     222 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
         |                ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~
>> drivers/gpio/gpio-siul2-s32g2.c:296:32: error: incompatible pointer types passing 'resource_size_t *' (aka 'unsigned int *') to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
     296 |                 conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
         |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
     238 |                 __rem = __div64_32(&(n), __base);       \
         |                                    ^~~~
   include/asm-generic/div64.h:213:38: note: passing argument to parameter 'dividend' here
     213 | extern uint32_t __div64_32(uint64_t *dividend, uint32_t divisor);
         |                                      ^
>> drivers/gpio/gpio-siul2-s32g2.c:296:32: warning: shift count >= width of type [-Wshift-count-overflow]
     296 |                 conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
         |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
     234 |         } else if (likely(((n) >> 32) == 0)) {          \
         |                                ^  ~~
   include/linux/compiler.h:76:40: note: expanded from macro 'likely'
      76 | # define likely(x)      __builtin_expect(!!(x), 1)
         |                                             ^
   8 warnings and 1 error generated.


vim +296 drivers/gpio/gpio-siul2-s32g2.c

   273	
   274	static struct regmap *common_regmap_init(struct platform_device *pdev,
   275						 struct regmap_config *conf,
   276						 const char *name)
   277	{
   278		struct device *dev = &pdev->dev;
   279		struct resource *res;
   280		resource_size_t size;
   281		void __iomem *base;
   282	
   283		base = devm_platform_get_and_ioremap_resource_byname(pdev, name, &res);
   284		if (IS_ERR(base)) {
   285			dev_err(&pdev->dev, "Failed to get MEM resource: %s\n", name);
   286			return ERR_PTR(-EINVAL);
   287		}
   288	
   289		size = resource_size(res);
   290		conf->val_bits = conf->reg_stride * 8;
   291		conf->max_register = size - conf->reg_stride;
   292		conf->name = name;
   293		conf->use_raw_spinlock = true;
   294	
   295		if (conf->cache_type != REGCACHE_NONE)
 > 296			conf->num_reg_defaults_raw = do_div(size, conf->reg_stride);
   297	
   298		return devm_regmap_init_mmio(dev, base, conf);
   299	}
   300	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Linus Walleij]

Hi Andrei,

thanks for your patch!

Sorry for being so late in giving any deeper review of it.

On Thu, Sep 26, 2024 at 4:32 PM Andrei Stefanescu
<andrei.stefanescu@oss.nxp.com> wrote:

> Add the GPIO driver for S32G2/S32G3 SoCs. This driver uses the SIUL2
> (System Integration Unit Lite2) hardware module. There are two
> SIUL2 hardware modules present, SIUL2_0(controlling GPIOs 0-101) and
> SIUL2_1 for the rest.
>
> The GPIOs are not fully contiguous, there are some gaps:
> - GPIO102 up to GPIO111(inclusive) are invalid
> - GPIO123 up to GPIO143(inclusive) are invalid
>
> Some GPIOs are input only(i.e. GPI182) though this restriction
> is not yet enforced in code.
>
> This patch adds basic GPIO functionality(no interrupts, no
> suspend/resume functions).
>
> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>

(...)

> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/pinctrl/consumer.h>

Hm, are you sure you don't want one of those combined
GPIO+pinctrl drivers? Look in drivers/pinctrl for examples such
as drivers/pinctrl/pinctrl-stmfx.c

> +/* PGPDOs are 16bit registers that come in big endian
> + * order if they are grouped in pairs of two.
> + *
> + * For example, the order is PGPDO1, PGPDO0, PGPDO3, PGPDO2...
> + */
> +#define SIUL2_PGPDO(N)         (((N) ^ 1) * 2)
> +#define S32G2_SIUL2_NUM                2
> +#define S32G2_PADS_DTS_TAG_LEN 7
> +
> +#define SIUL2_GPIO_16_PAD_SIZE 16

You seem to be manipulating "pads" which is pin control territory.
That should be done in a pin control driver.

> +/**
> + * struct siul2_device_data  - platform data attached to the compatible.
> + * @pad_access: access table for I/O pads, consists of S32G2_SIUL2_NUM tables.
> + * @reset_cnt: reset the pin name counter to zero when switching to SIUL2_1.
> + */
> +struct siul2_device_data {
> +       const struct regmap_access_table **pad_access;
> +       const bool reset_cnt;

I don't understand the reset_cnt variable at all. Explain why it exists in the
kerneldoc please.

> +/**
> + * struct siul2_desc - describes a SIUL2 hw module.
> + * @pad_access: array of valid I/O pads.

Now that is really pin control isn't it.

> + * @opadmap: the regmap of the Parallel GPIO Pad Data Out Register.
> + * @ipadmap: the regmap of the Parallel GPIO Pad Data In Register.
> + * @gpio_base: the first GPIO pin.
> + * @gpio_num: the number of GPIO pins.
> + */
> +struct siul2_desc {
> +       const struct regmap_access_table *pad_access;
> +       struct regmap *opadmap;
> +       struct regmap *ipadmap;
> +       u32 gpio_base;
> +       u32 gpio_num;
> +};
(...)

> +static int siul2_get_gpio_pinspec(struct platform_device *pdev,
> +                                 struct of_phandle_args *pinspec,
> +                                 unsigned int range_index)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +
> +       return of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
> +                                               range_index, pinspec);
> +}

I do not see why a driver would parse gpio-ranges since the gpiolib
core should normally deal with the translation between GPIO lines
and pins.

This looks hacky and probably an indication that you want to use
a combined pinctrl+gpio driver so the different parts of the driver
can access the same structures easily without hacks.

> +static void siul2_gpio_set_direction(struct siul2_gpio_dev *dev,
> +                                    unsigned int gpio, int dir)
> +{
> +       guard(raw_spinlock_irqsave)(&dev->lock);
> +
> +       if (dir == GPIO_LINE_DIRECTION_IN)
> +               __clear_bit(gpio, dev->pin_dir_bitmap);
> +       else
> +               __set_bit(gpio, dev->pin_dir_bitmap);
> +}

This looks like a job for gpio regmap?

select GPIO_REGMAP,
look in other driver for examples of how to use it.

> +static int siul2_get_direction(struct siul2_gpio_dev *dev,
> +                              unsigned int gpio)
> +{
> +       return test_bit(gpio, dev->pin_dir_bitmap) ? GPIO_LINE_DIRECTION_OUT :
> +                                                    GPIO_LINE_DIRECTION_IN;
> +}

Also looks like a reimplementation of GPIO_REGMAP.

> +static int siul2_gpio_dir_out(struct gpio_chip *chip, unsigned int gpio,
> +                             int val)
> +{
> +       struct siul2_gpio_dev *gpio_dev;
> +       int ret = 0;
> +
> +       gpio_dev = to_siul2_gpio_dev(chip);
> +       siul2_gpio_set_val(chip, gpio, val);
> +
> +       ret = pinctrl_gpio_direction_output(chip, gpio);
(...)

This is nice, pin control is properly used as the back-end.

> +static int siul2_gpio_remove_reserved_names(struct device *dev,
> +                                           struct siul2_gpio_dev *gpio_dev,
> +                                           char **names)
> +{
> +       struct device_node *np = dev->of_node;
> +       int num_ranges, i, j, ret;
> +       u32 base_gpio, num_gpio;
> +
> +       /* Parse the gpio-reserved-ranges to know which GPIOs to exclude. */
> +
> +       num_ranges = of_property_count_u32_elems(dev->of_node,
> +                                                "gpio-reserved-ranges");
> +
> +       /* The "gpio-reserved-ranges" is optional. */
> +       if (num_ranges < 0)
> +               return 0;
> +       num_ranges /= 2;
> +
> +       for (i = 0; i < num_ranges; i++) {
> +               ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
> +                                                i * 2, &base_gpio);
> +               if (ret) {
> +                       dev_err(dev, "Could not parse the start GPIO: %d\n",
> +                               ret);
> +                       return ret;
> +               }
> +
> +               ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
> +                                                i * 2 + 1, &num_gpio);
> +               if (ret) {
> +                       dev_err(dev, "Could not parse num. GPIOs: %d\n", ret);
> +                       return ret;
> +               }
> +
> +               if (base_gpio + num_gpio > gpio_dev->gc.ngpio) {
> +                       dev_err(dev, "Reserved GPIOs outside of GPIO range\n");
> +                       return -EINVAL;
> +               }
> +
> +               /* Remove names set for reserved GPIOs. */
> +               for (j = base_gpio; j < base_gpio + num_gpio; j++) {
> +                       devm_kfree(dev, names[j]);
> +                       names[j] = NULL;
> +               }
> +       }
> +
> +       return 0;
> +}

I don't get this. gpio-reserved-ranges is something that is parsed and
handled by gpiolib. Read the code in gpiolib.c and you'll probably
figure out how to let gpiolib take care of this for you.

> +static int siul2_gpio_populate_names(struct device *dev,
> +                                    struct siul2_gpio_dev *gpio_dev)
> +{
> +       unsigned int num_index = 0;
> +       char ch_index = 'A';
> +       char **names;
> +       int i, ret;
> +
> +       names = devm_kcalloc(dev, gpio_dev->gc.ngpio, sizeof(*names),
> +                            GFP_KERNEL);
> +       if (!names)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < S32G2_SIUL2_NUM; i++) {
> +               ret = siul2_gen_names(dev, gpio_dev->siul2[i].gpio_num,
> +                                     names + gpio_dev->siul2[i].gpio_base,
> +                                     &ch_index, &num_index);
> +               if (ret) {
> +                       dev_err(dev, "Could not set names for SIUL2_%d GPIOs\n",
> +                               i);
> +                       return ret;
> +               }
> +
> +               if (gpio_dev->platdata->reset_cnt)
> +                       num_index = 0;
> +
> +               ch_index++;
> +       }
> +
> +       ret = siul2_gpio_remove_reserved_names(dev, gpio_dev, names);
> +       if (ret)
> +               return ret;
> +
> +       gpio_dev->gc.names = (const char *const *)names;
> +
> +       return 0;
> +}

Interesting!

I'm not against, on the contrary this looks really helpful to users.

> +       gc = &gpio_dev->gc;

No poking around in the gpiolib internals please.

Look at what other drivers do and do like they do.

On top of these comments:
everywhere you do [raw_spin_]locks: try to use guards or
scoped guards instead. git grep guard drivers/gpio for
many examples.

Yours,
Linus Walleij

Re: [PATCH v4 3/4] gpio: siul2-s32g2: add NXP S32G2/S32G3 SoCs support

[Andrei Stefanescu]

Hi Linus,

Thank you for the review!

On 01/10/2024 16:15, Linus Walleij wrote:
> Hi Andrei,
> 
> thanks for your patch!
> 
> Sorry for being so late in giving any deeper review of it.
> 
> On Thu, Sep 26, 2024 at 4:32 PM Andrei Stefanescu
> <andrei.stefanescu@oss.nxp.com> wrote:
> 
>> Add the GPIO driver for S32G2/S32G3 SoCs. This driver uses the SIUL2
>> (System Integration Unit Lite2) hardware module. There are two
>> SIUL2 hardware modules present, SIUL2_0(controlling GPIOs 0-101) and
>> SIUL2_1 for the rest.
>>
>> The GPIOs are not fully contiguous, there are some gaps:
>> - GPIO102 up to GPIO111(inclusive) are invalid
>> - GPIO123 up to GPIO143(inclusive) are invalid
>>
>> Some GPIOs are input only(i.e. GPI182) though this restriction
>> is not yet enforced in code.
>>
>> This patch adds basic GPIO functionality(no interrupts, no
>> suspend/resume functions).
>>
>> Signed-off-by: Ghennadi Procopciuc <ghennadi.procopciuc@nxp.com>
>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com>
>> Signed-off-by: Phu Luu An <phu.luuan@nxp.com>
>> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
> 
> (...)

I will add the Co-developed-by tags in v5.

> 
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/module.h>
>> +#include <linux/gpio/driver.h>
>> +#include <linux/pinctrl/consumer.h>
> 
> Hm, are you sure you don't want one of those combined
> GPIO+pinctrl drivers? Look in drivers/pinctrl for examples such
> as drivers/pinctrl/pinctrl-stmfx.c

Thank you for the suggestion! I will merge the two drivers in v5.
> 
>> +/* PGPDOs are 16bit registers that come in big endian
>> + * order if they are grouped in pairs of two.
>> + *
>> + * For example, the order is PGPDO1, PGPDO0, PGPDO3, PGPDO2...
>> + */
>> +#define SIUL2_PGPDO(N)         (((N) ^ 1) * 2)
>> +#define S32G2_SIUL2_NUM                2
>> +#define S32G2_PADS_DTS_TAG_LEN 7
>> +
>> +#define SIUL2_GPIO_16_PAD_SIZE 16
> 
> You seem to be manipulating "pads" which is pin control territory.
> That should be done in a pin control driver.
> 

This is more of a case of bad naming, the registers used for
setting/reading a GPIO's value are called Parallel GPIO Pad Data
Output/Input (PGPDO/PGPDI) and they are 16bit wide. I will try to
find a better name for this.

>> +/**
>> + * struct siul2_device_data  - platform data attached to the compatible.
>> + * @pad_access: access table for I/O pads, consists of S32G2_SIUL2_NUM tables.
>> + * @reset_cnt: reset the pin name counter to zero when switching to SIUL2_1.
>> + */
>> +struct siul2_device_data {
>> +       const struct regmap_access_table **pad_access;
>> +       const bool reset_cnt;
> 
> I don't understand the reset_cnt variable at all. Explain why it exists in the
> kerneldoc please.

It is used while generating the GPIO line names. The naming scheme is to
have: P + letter + number(0 to 15). The "reset_cnt" is used to tell if
the number should change to 0 when naming the SIUL21 GPIOs. For example,
for S32G2, the SIUL20 module has 102 pins named PA00, PA01, ..., PA15,
PB0, .., PG05. SIUL21 starts at 112 and reset_cnt is true so the first
pin will be PH00.

We have another platform which uses this driver and doesn't have the 102-111 gap
and the first SIUL21 pin is named PH06.

I will remove this. I figured out now that I can see if the first pin is
divisible by 16 and reset the counter in that case.

> 
>> +/**
>> + * struct siul2_desc - describes a SIUL2 hw module.
>> + * @pad_access: array of valid I/O pads.
> 
> Now that is really pin control isn't it.

This is again referring to the registers for setting the
the output value/reading the input value of a GPIO. I will
change the name in v5.

> 
>> + * @opadmap: the regmap of the Parallel GPIO Pad Data Out Register.
>> + * @ipadmap: the regmap of the Parallel GPIO Pad Data In Register.
>> + * @gpio_base: the first GPIO pin.
>> + * @gpio_num: the number of GPIO pins.
>> + */
>> +struct siul2_desc {
>> +       const struct regmap_access_table *pad_access;
>> +       struct regmap *opadmap;
>> +       struct regmap *ipadmap;
>> +       u32 gpio_base;
>> +       u32 gpio_num;
>> +};
> (...)
> 
>> +static int siul2_get_gpio_pinspec(struct platform_device *pdev,
>> +                                 struct of_phandle_args *pinspec,
>> +                                 unsigned int range_index)
>> +{
>> +       struct device_node *np = pdev->dev.of_node;
>> +
>> +       return of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3,
>> +                                               range_index, pinspec);
>> +}
> 
> I do not see why a driver would parse gpio-ranges since the gpiolib
> core should normally deal with the translation between GPIO lines
> and pins.
> 
> This looks hacky and probably an indication that you want to use
> a combined pinctrl+gpio driver so the different parts of the driver
> can access the same structures easily without hacks.

We parse the gpio-ranges property because this driver handles
two GPIO hardware IPs in a single driver instance. This is because
both SIUL2 modules have pins with interrupt capability but the
registers to configure interrupts are part of SIUL21.
We use the gpio_base and gpio_num to generate the line names
and to select the correct regmap for PGPDOs/PGPDIs
(Parallel GPIO Pad Data Output/Input).

> 
>> +static void siul2_gpio_set_direction(struct siul2_gpio_dev *dev,
>> +                                    unsigned int gpio, int dir)
>> +{
>> +       guard(raw_spinlock_irqsave)(&dev->lock);
>> +
>> +       if (dir == GPIO_LINE_DIRECTION_IN)
>> +               __clear_bit(gpio, dev->pin_dir_bitmap);
>> +       else
>> +               __set_bit(gpio, dev->pin_dir_bitmap);
>> +}
> 
> This looks like a job for gpio regmap?
> 
> select GPIO_REGMAP,
> look in other driver for examples of how to use it.

We use the bitmap just to store the current direction of a pad.
I don't think we can use the gpio-regmap driver because we
rely on the pinctrl driver to configure the pin(pinmux&pinconf).

> 
>> +static int siul2_get_direction(struct siul2_gpio_dev *dev,
>> +                              unsigned int gpio)
>> +{
>> +       return test_bit(gpio, dev->pin_dir_bitmap) ? GPIO_LINE_DIRECTION_OUT :
>> +                                                    GPIO_LINE_DIRECTION_IN;
>> +}
> 
> Also looks like a reimplementation of GPIO_REGMAP.

The answer above applies here as well.

> 
>> +static int siul2_gpio_dir_out(struct gpio_chip *chip, unsigned int gpio,
>> +                             int val)
>> +{
>> +       struct siul2_gpio_dev *gpio_dev;
>> +       int ret = 0;
>> +
>> +       gpio_dev = to_siul2_gpio_dev(chip);
>> +       siul2_gpio_set_val(chip, gpio, val);
>> +
>> +       ret = pinctrl_gpio_direction_output(chip, gpio);
> (...)
> 
> This is nice, pin control is properly used as the back-end.
> 
>> +static int siul2_gpio_remove_reserved_names(struct device *dev,
>> +                                           struct siul2_gpio_dev *gpio_dev,
>> +                                           char **names)
>> +{
>> +       struct device_node *np = dev->of_node;
>> +       int num_ranges, i, j, ret;
>> +       u32 base_gpio, num_gpio;
>> +
>> +       /* Parse the gpio-reserved-ranges to know which GPIOs to exclude. */
>> +
>> +       num_ranges = of_property_count_u32_elems(dev->of_node,
>> +                                                "gpio-reserved-ranges");
>> +
>> +       /* The "gpio-reserved-ranges" is optional. */
>> +       if (num_ranges < 0)
>> +               return 0;
>> +       num_ranges /= 2;
>> +
>> +       for (i = 0; i < num_ranges; i++) {
>> +               ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
>> +                                                i * 2, &base_gpio);
>> +               if (ret) {
>> +                       dev_err(dev, "Could not parse the start GPIO: %d\n",
>> +                               ret);
>> +                       return ret;
>> +               }
>> +
>> +               ret = of_property_read_u32_index(np, "gpio-reserved-ranges",
>> +                                                i * 2 + 1, &num_gpio);
>> +               if (ret) {
>> +                       dev_err(dev, "Could not parse num. GPIOs: %d\n", ret);
>> +                       return ret;
>> +               }
>> +
>> +               if (base_gpio + num_gpio > gpio_dev->gc.ngpio) {
>> +                       dev_err(dev, "Reserved GPIOs outside of GPIO range\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               /* Remove names set for reserved GPIOs. */
>> +               for (j = base_gpio; j < base_gpio + num_gpio; j++) {
>> +                       devm_kfree(dev, names[j]);
>> +                       names[j] = NULL;
>> +               }
>> +       }
>> +
>> +       return 0;
>> +}
> 
> I don't get this. gpio-reserved-ranges is something that is parsed and
> handled by gpiolib. Read the code in gpiolib.c and you'll probably
> figure out how to let gpiolib take care of this for you.

We use this just to remove line names for reserved GPIOs(in our case
not present, we have some gaps).

> 
>> +static int siul2_gpio_populate_names(struct device *dev,
>> +                                    struct siul2_gpio_dev *gpio_dev)
>> +{
>> +       unsigned int num_index = 0;
>> +       char ch_index = 'A';
>> +       char **names;
>> +       int i, ret;
>> +
>> +       names = devm_kcalloc(dev, gpio_dev->gc.ngpio, sizeof(*names),
>> +                            GFP_KERNEL);
>> +       if (!names)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < S32G2_SIUL2_NUM; i++) {
>> +               ret = siul2_gen_names(dev, gpio_dev->siul2[i].gpio_num,
>> +                                     names + gpio_dev->siul2[i].gpio_base,
>> +                                     &ch_index, &num_index);
>> +               if (ret) {
>> +                       dev_err(dev, "Could not set names for SIUL2_%d GPIOs\n",
>> +                               i);
>> +                       return ret;
>> +               }
>> +
>> +               if (gpio_dev->platdata->reset_cnt)
>> +                       num_index = 0;
>> +
>> +               ch_index++;
>> +       }
>> +
>> +       ret = siul2_gpio_remove_reserved_names(dev, gpio_dev, names);
>> +       if (ret)
>> +               return ret;
>> +
>> +       gpio_dev->gc.names = (const char *const *)names;
>> +
>> +       return 0;
>> +}
> 
> Interesting!
> 
> I'm not against, on the contrary this looks really helpful to users.

I can try to integrate this into gpiolib after I merge this driver.

> 
>> +       gc = &gpio_dev->gc;
> 
> No poking around in the gpiolib internals please.
> 
> Look at what other drivers do and do like they do.

I will fix in v5.

> 
> On top of these comments:
> everywhere you do [raw_spin_]locks: try to use guards or
> scoped guards instead. git grep guard drivers/gpio for
> many examples.

I will fix in v5.

> 
> Yours,
> Linus Walleij

Best regards,
Andrei

[PATCH v4 4/4] MAINTAINERS: add MAINTAINER for S32G2 SIUL2 GPIO driver

[Andrei Stefanescu]

Add myself as a maintainer for the S32G2 SIUL2 GPIO driver and
the NXP S32 mailing list for reviews.

Signed-off-by: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7bfef98226d9..1201f284b0b5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2763,7 +2763,9 @@ R:	Ghennadi Procopciuc <ghennadi.procopciuc@oss.nxp.com>
 L:	NXP S32 Linux Team <s32@nxp.com>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
+F:	Documentation/devicetree/bindings/gpio/nxp,s32g2-siul2-gpio.yaml
 F:	arch/arm64/boot/dts/freescale/s32g*.dts*
+F:	drivers/gpio/gpio-siul2-s32g2.c
 F:	drivers/pinctrl/nxp/
 
 ARM/Orion SoC/Technologic Systems TS-78xx platform support
-- 
2.45.2