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.2On 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.
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. >
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
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
>
>
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
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
>
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
> >
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
>>>
>
>
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
>>>>
>>
>>
>
>
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.
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
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
© 2016 - 2024 Red Hat, Inc.