| Message ID | 20221221073232.21888-1-clin@suse.com |
|---|---|
| Headers | show |
| Series | Add pinctrl support for S32 SoC family | expand |
On 21/12/2022 13:28, Andrei Stefanescu wrote: > Hi Chester, > >> +patternProperties: >> + '-pins$': > > Sorry, I missed this in the previous versions. Could you change it to '_pins' (underscore)? In our .dts files we use underscore in the names for pinctrl configuration nodes e.g. i2c4_pins, usbotg_pins. You cannot have underscores as node names, so what do you mean here? You need to fix your DTS not introduce bad practices to mainline kernel. Best regards, Krzysztof
Hi Krzysztof and Andrei, Thanks for reviewing this patch! On Wed, Dec 21, 2022 at 01:30:12PM +0100, Krzysztof Kozlowski wrote: > On 21/12/2022 13:28, Andrei Stefanescu wrote: > > Hi Chester, > > > >> +patternProperties: > >> + '-pins$': > > > > Sorry, I missed this in the previous versions. Could you change it to '_pins' (underscore)? In our .dts files we use underscore in the names for pinctrl configuration nodes e.g. i2c4_pins, usbotg_pins. > > You cannot have underscores as node names, so what do you mean here? You Krzysztof is right, and Rob also reminded me in his comments in v1: https://lore.kernel.org/linux-arm-kernel/20221102154903.GA3726664-robh@kernel.org/ Regards, Chester > need to fix your DTS not introduce bad practices to mainline kernel. > > Best regards, > Krzysztof >
On 21/12/2022 08:32, Chester Lin wrote: > Add DT schema for the pinctrl driver of NXP S32 SoC family. > > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com> > Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@oss.nxp.com> > Signed-off-by: Chester Lin <clin@suse.com> > --- > > Changes in v3: > - Remove the minItems from reg because there's no optional item for s32g2. > - List supported properties of pinmux-node and pincfg-node and add more > descriptions. > - Adjust the location of "required:". > - Fix descriptions and wordings. > - Rename the yaml file to nxp,s32g2-siul2-pinctrl.yaml. > > Changes in v2: > - Remove the "nxp,pins" property since it has been moved into the driver. > - Add descriptions for reg entries. > - Refine the compatible name from "nxp,s32g-..." to "nxp,s32g2-...". > - Fix schema issues and revise the example. > - Fix the copyright format suggested by NXP. > > .../pinctrl/nxp,s32g2-siul2-pinctrl.yaml | 129 ++++++++++++++++++ > 1 file changed, 129 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml > new file mode 100644 > index 000000000000..1554ce14214a > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml > @@ -0,0 +1,129 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright 2022 NXP > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/nxp,s32g2-siul2-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: NXP S32G2 pin controller > + > +maintainers: > + - Ghennadi Procopciuc <Ghennadi.Procopciuc@oss.nxp.com> > + - Chester Lin <clin@suse.com> > + > +description: | > + S32G2 pinmux is implemented in SIUL2 (System Integration Unit Lite2), > + whose memory map is split into two regions: > + SIUL2_0 @ 0x4009c000 > + SIUL2_1 @ 0x44010000 > + > + Every SIUL2 region has multiple register types, and here only MSCR and > + IMCR registers need to be revealed for kernel to configure pinmux. > + > + Please note that some register indexes are reserved in S32G2, such as > + MSCR102-MSCR111, MSCR123-MSCR143, IMCR84-IMCR118 and IMCR398-IMCR429. > + > +properties: > + compatible: > + enum: > + - nxp,s32g2-siul2-pinctrl > + > + reg: > + description: | > + A list of MSCR/IMCR register regions to be reserved. > + - MSCR (Multiplexed Signal Configuration Register) > + An MSCR register can configure the associated pin as either a GPIO pin > + or a function output pin depends on the selected signal source. > + - IMCR (Input Multiplexed Signal Configuration Register) > + An IMCR register can configure the associated pin as function input > + pin depends on the selected signal source. > + items: > + - description: MSCR registers group 0 in SIUL2_0 > + - description: MSCR registers group 1 in SIUL2_1 > + - description: MSCR registers group 2 in SIUL2_1 > + - description: IMCR registers group 0 in SIUL2_0 > + - description: IMCR registers group 1 in SIUL2_1 > + - description: IMCR registers group 2 in SIUL2_1 > + > +patternProperties: > + '-pins$': > + type: object > + additionalProperties: false > + > + patternProperties: > + '-grp[0-9]$': > + type: object > + allOf: > + - $ref: pinmux-node.yaml# > + - $ref: pincfg-node.yaml# > + description: | > + Pinctrl node's client devices specify pin muxes using subnodes, > + which in turn use the standard properties below. > + > + properties: > + bias-disable: true > + bias-high-impedance: true > + bias-pull-up: true > + bias-pull-down: true > + drive-open-drain: true > + input-enable: true > + output-enable: true > + > + pinmux: > + description: | > + An integer array for representing pinmux configurations of > + a device. Each integer consists of a PIN_ID and a 4-bit > + selected signal source(SSS) as IOMUX setting, which is > + calculated as: pinmux = (PIN_ID << 4 | SSS) > + > + slew-rate: > + description: | > + 0: 208MHz > + 1-3: Reserved > + 4: 166MHz > + 5: 150MHz > + 6: 133MHz > + 7: 83MHz > + enum: [0, 4, 5, 6, 7] You have known values, then use them. This is much more readable in DTS. Best regards, Krzysztof
Hi Krzysztof, On Thu, Dec 22, 2022 at 12:28:31PM +0100, Krzysztof Kozlowski wrote: > On 21/12/2022 08:32, Chester Lin wrote: > > Add DT schema for the pinctrl driver of NXP S32 SoC family. > > > > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com> > > Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@oss.nxp.com> > > Signed-off-by: Chester Lin <clin@suse.com> > > --- > > > > Changes in v3: > > - Remove the minItems from reg because there's no optional item for s32g2. > > - List supported properties of pinmux-node and pincfg-node and add more > > descriptions. > > - Adjust the location of "required:". > > - Fix descriptions and wordings. > > - Rename the yaml file to nxp,s32g2-siul2-pinctrl.yaml. > > > > Changes in v2: > > - Remove the "nxp,pins" property since it has been moved into the driver. > > - Add descriptions for reg entries. > > - Refine the compatible name from "nxp,s32g-..." to "nxp,s32g2-...". > > - Fix schema issues and revise the example. > > - Fix the copyright format suggested by NXP. > > > > .../pinctrl/nxp,s32g2-siul2-pinctrl.yaml | 129 ++++++++++++++++++ > > 1 file changed, 129 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml > > new file mode 100644 > > index 000000000000..1554ce14214a > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml > > @@ -0,0 +1,129 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +# Copyright 2022 NXP > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pinctrl/nxp,s32g2-siul2-pinctrl.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: NXP S32G2 pin controller > > + > > +maintainers: > > + - Ghennadi Procopciuc <Ghennadi.Procopciuc@oss.nxp.com> > > + - Chester Lin <clin@suse.com> > > + > > +description: | > > + S32G2 pinmux is implemented in SIUL2 (System Integration Unit Lite2), > > + whose memory map is split into two regions: > > + SIUL2_0 @ 0x4009c000 > > + SIUL2_1 @ 0x44010000 > > + > > + Every SIUL2 region has multiple register types, and here only MSCR and > > + IMCR registers need to be revealed for kernel to configure pinmux. > > + > > + Please note that some register indexes are reserved in S32G2, such as > > + MSCR102-MSCR111, MSCR123-MSCR143, IMCR84-IMCR118 and IMCR398-IMCR429. > > + > > +properties: > > + compatible: > > + enum: > > + - nxp,s32g2-siul2-pinctrl > > + > > + reg: > > + description: | > > + A list of MSCR/IMCR register regions to be reserved. > > + - MSCR (Multiplexed Signal Configuration Register) > > + An MSCR register can configure the associated pin as either a GPIO pin > > + or a function output pin depends on the selected signal source. > > + - IMCR (Input Multiplexed Signal Configuration Register) > > + An IMCR register can configure the associated pin as function input > > + pin depends on the selected signal source. > > + items: > > + - description: MSCR registers group 0 in SIUL2_0 > > + - description: MSCR registers group 1 in SIUL2_1 > > + - description: MSCR registers group 2 in SIUL2_1 > > + - description: IMCR registers group 0 in SIUL2_0 > > + - description: IMCR registers group 1 in SIUL2_1 > > + - description: IMCR registers group 2 in SIUL2_1 > > + > > +patternProperties: > > + '-pins$': > > + type: object > > + additionalProperties: false > > + > > + patternProperties: > > + '-grp[0-9]$': > > + type: object > > + allOf: > > + - $ref: pinmux-node.yaml# > > + - $ref: pincfg-node.yaml# > > + description: | > > + Pinctrl node's client devices specify pin muxes using subnodes, > > + which in turn use the standard properties below. > > + > > + properties: > > + bias-disable: true > > + bias-high-impedance: true > > + bias-pull-up: true > > + bias-pull-down: true > > + drive-open-drain: true > > + input-enable: true > > + output-enable: true > > + > > + pinmux: > > + description: | > > + An integer array for representing pinmux configurations of > > + a device. Each integer consists of a PIN_ID and a 4-bit > > + selected signal source(SSS) as IOMUX setting, which is > > + calculated as: pinmux = (PIN_ID << 4 | SSS) > > + > > + slew-rate: > > + description: | > > + 0: 208MHz > > + 1-3: Reserved > > + 4: 166MHz > > + 5: 150MHz > > + 6: 133MHz > > + 7: 83MHz > > + enum: [0, 4, 5, 6, 7] > > You have known values, then use them. This is much more readable in DTS. The main reason of mapping with register values [0-7] is to simplify the driver implementation while handling register r/w. To improve readability as you suggested, I am thinking of having a DT header "s32g2-pinfunc.h" with a few binding macros/helper as below, the only difference compared to v3 is using S32G2_PINMUX and S32G2_SLEW_XXXMHZ macros rather than pure integers to represent pinmux and slew-rate property values. Regards, Chester
On 09/01/2023 08:04, Chester Lin wrote: > Hi Krzysztof, > > On Thu, Dec 22, 2022 at 12:28:31PM +0100, Krzysztof Kozlowski wrote: >> On 21/12/2022 08:32, Chester Lin wrote: >>> Add DT schema for the pinctrl driver of NXP S32 SoC family. >>> >>> Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com> >>> Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@oss.nxp.com> >>> Signed-off-by: Chester Lin <clin@suse.com> >>> --- >>> >>> Changes in v3: >>> - Remove the minItems from reg because there's no optional item for s32g2. >>> - List supported properties of pinmux-node and pincfg-node and add more >>> descriptions. >>> - Adjust the location of "required:". >>> - Fix descriptions and wordings. >>> - Rename the yaml file to nxp,s32g2-siul2-pinctrl.yaml. >>> >>> Changes in v2: >>> - Remove the "nxp,pins" property since it has been moved into the driver. >>> - Add descriptions for reg entries. >>> - Refine the compatible name from "nxp,s32g-..." to "nxp,s32g2-...". >>> - Fix schema issues and revise the example. >>> - Fix the copyright format suggested by NXP. >>> >>> .../pinctrl/nxp,s32g2-siul2-pinctrl.yaml | 129 ++++++++++++++++++ >>> 1 file changed, 129 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml >>> new file mode 100644 >>> index 000000000000..1554ce14214a >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml >>> @@ -0,0 +1,129 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +# Copyright 2022 NXP >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/pinctrl/nxp,s32g2-siul2-pinctrl.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: NXP S32G2 pin controller >>> + >>> +maintainers: >>> + - Ghennadi Procopciuc <Ghennadi.Procopciuc@oss.nxp.com> >>> + - Chester Lin <clin@suse.com> >>> + >>> +description: | >>> + S32G2 pinmux is implemented in SIUL2 (System Integration Unit Lite2), >>> + whose memory map is split into two regions: >>> + SIUL2_0 @ 0x4009c000 >>> + SIUL2_1 @ 0x44010000 >>> + >>> + Every SIUL2 region has multiple register types, and here only MSCR and >>> + IMCR registers need to be revealed for kernel to configure pinmux. >>> + >>> + Please note that some register indexes are reserved in S32G2, such as >>> + MSCR102-MSCR111, MSCR123-MSCR143, IMCR84-IMCR118 and IMCR398-IMCR429. >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - nxp,s32g2-siul2-pinctrl >>> + >>> + reg: >>> + description: | >>> + A list of MSCR/IMCR register regions to be reserved. >>> + - MSCR (Multiplexed Signal Configuration Register) >>> + An MSCR register can configure the associated pin as either a GPIO pin >>> + or a function output pin depends on the selected signal source. >>> + - IMCR (Input Multiplexed Signal Configuration Register) >>> + An IMCR register can configure the associated pin as function input >>> + pin depends on the selected signal source. >>> + items: >>> + - description: MSCR registers group 0 in SIUL2_0 >>> + - description: MSCR registers group 1 in SIUL2_1 >>> + - description: MSCR registers group 2 in SIUL2_1 >>> + - description: IMCR registers group 0 in SIUL2_0 >>> + - description: IMCR registers group 1 in SIUL2_1 >>> + - description: IMCR registers group 2 in SIUL2_1 >>> + >>> +patternProperties: >>> + '-pins$': >>> + type: object >>> + additionalProperties: false >>> + >>> + patternProperties: >>> + '-grp[0-9]$': >>> + type: object >>> + allOf: >>> + - $ref: pinmux-node.yaml# >>> + - $ref: pincfg-node.yaml# >>> + description: | >>> + Pinctrl node's client devices specify pin muxes using subnodes, >>> + which in turn use the standard properties below. >>> + >>> + properties: >>> + bias-disable: true >>> + bias-high-impedance: true >>> + bias-pull-up: true >>> + bias-pull-down: true >>> + drive-open-drain: true >>> + input-enable: true >>> + output-enable: true >>> + >>> + pinmux: >>> + description: | >>> + An integer array for representing pinmux configurations of >>> + a device. Each integer consists of a PIN_ID and a 4-bit >>> + selected signal source(SSS) as IOMUX setting, which is >>> + calculated as: pinmux = (PIN_ID << 4 | SSS) >>> + >>> + slew-rate: >>> + description: | >>> + 0: 208MHz >>> + 1-3: Reserved >>> + 4: 166MHz >>> + 5: 150MHz >>> + 6: 133MHz >>> + 7: 83MHz >>> + enum: [0, 4, 5, 6, 7] >> >> You have known values, then use them. This is much more readable in DTS. > > The main reason of mapping with register values [0-7] is to simplify the > driver implementation while handling register r/w. Define bindings for the DTS, not for the drivers. > To improve readability > as you suggested, I am thinking of having a DT header "s32g2-pinfunc.h" with > a few binding macros/helper as below, the only difference compared to v3 is > using S32G2_PINMUX and S32G2_SLEW_XXXMHZ macros rather than pure integers > to represent pinmux and slew-rate property values. Binding headers is not a place for register values. By definition - these are bindings, not hardware description. Hardware description is DTS. Feel free to store them in DTS headers, but anyway this does not solve the issue here. The issue is: you store register values in DTS, which is limited, not extendable description. Each of your devices would need entirely different binding for this because register values can change between every SoC version. Several other pinctrl bindings use similar approach, but they have not got a clear mapping to values (e.g. they have fast and slow). For the case with real values, use the same solution as drive-strength - real values. > > Regards, > Chester > > > From 3a29d905ae104e694230ffc02dc9f9de4191c5d1 Mon Sep 17 00:00:00 2001 > From: Chester Lin <clin@suse.com> > Date: Fri, 28 Oct 2022 16:44:29 +0800 > Subject: [PATCH] dt-bindings: pinctrl: add support for NXP S32 SoCs > > Add DT schema and hedaer file for the pinctrl driver of NXP S32 SoC family. > > Signed-off-by: Larisa Grigore <larisa.grigore@nxp.com> > Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@oss.nxp.com> > Signed-off-by: Chester Lin <clin@suse.com> > --- > .../pinctrl/nxp,s32g2-siul2-pinctrl.yaml | 136 ++++++++++++++++++ > include/dt-bindings/pinctrl/s32g2-pinfunc.h | 17 +++ NAK for bindings. Best regards, Krzysztof
Hello, Here I want to introduce a new patch series, which aims to support IOMUX functions provided by SIUL2 [System Integration Unit Lite2] on S32 SoCs, such as S32G2. This series is originally from NXP's implementation on CodeAurora[1] and it will be required by upstream kernel for supporting a variety of devices on S32 SoCs which need to config PINMUXs, such as PHYs and MAC controllers. Thanks, Chester Changes in v3: - dt-bindings: - Remove the minItems from reg because there's no optional item for s32g2. - List supported properties of pinmux-node and pincfg-node and add more descriptions. - Adjust the location of "required:". - Fix descriptions and wordings. - Rename the yaml file to nxp,s32g2-siul2-pinctrl.yaml. - Rename pinctrl-s32g.c to pinctrl-s32g2.c - Adjust Kconfig options [menu-invisible] and names [S32G -> S32G2]. - Add .suppress_bind_attrs - Drop the .remove callback and replace the module_platform_driver() call with builtin_platform_driver() Changes in v2: - Move the "nxp,pins" ID range information from DT to the driver. - dt-bindings: - Fix schema issues. - Add descriptions for reg entries. - Revise the example. - Refine the compatible name from "nxp,s32g-..." to "nxp,s32g2-...". - Fix the copyright format suggested by NXP. [1] https://source.codeaurora.org/external/autobsps32/linux/tree/drivers/pinctrl/freescale?h=bsp34.0-5.10.120-rt Chester Lin (2): dt-bindings: pinctrl: add schema for NXP S32 SoCs pinctrl: add NXP S32 SoC family support .../pinctrl/nxp,s32g2-siul2-pinctrl.yaml | 129 +++ drivers/pinctrl/freescale/Kconfig | 14 + drivers/pinctrl/freescale/Makefile | 2 + drivers/pinctrl/freescale/pinctrl-s32.h | 76 ++ drivers/pinctrl/freescale/pinctrl-s32cc.c | 983 ++++++++++++++++++ drivers/pinctrl/freescale/pinctrl-s32g2.c | 773 ++++++++++++++ 6 files changed, 1977 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32g2-siul2-pinctrl.yaml create mode 100644 drivers/pinctrl/freescale/pinctrl-s32.h create mode 100644 drivers/pinctrl/freescale/pinctrl-s32cc.c create mode 100644 drivers/pinctrl/freescale/pinctrl-s32g2.c