[1/2] dt-bindings: pinctrl: add schema for NXP S32 SoCs

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@nxp.com>
Signed-off-by: Chester Lin <clin@suse.com>
---
 .../pinctrl/nxp,s32cc-siul2-pinctrl.yaml      | 91 +++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml

On Mon, Oct 31, 2022 at 06:08:42PM +0800, 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@nxp.com>
> Signed-off-by: Chester Lin <clin@suse.com>
> ---
>  .../pinctrl/nxp,s32cc-siul2-pinctrl.yaml      | 91 +++++++++++++++++++
>  1 file changed, 91 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> new file mode 100644
> index 000000000000..eafb9091cbf7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> @@ -0,0 +1,91 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2022 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/nxp,s32cc-siul2-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP S32 Common Chassis SIUL2 iomux controller
> +
> +maintainers:
> +  - Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
> +  - Chester Lin <clin@suse.com>
> +
> +description: |
> +  Core driver for the pin controller found on S32 Common Chassis SoC.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,s32g-siul2-pinctrl
> +
> +  reg:
> +    minItems: 5
> +    maxItems: 6
> +    description: A list of register regions to be reserved.

Need to be explicit about what each entry is.

> +
> +  nxp,pins:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +      A list of [start, end] pin ID boundaries that correspond to each of
> +      the register regions reserved.

Looks like a matrix rather than an array.

> +
> +required:
> +  - compatible
> +  - reg
> +  - nxp,pins
> +
> +patternProperties:
> +  '_pins$':

s/_/-/

> +    type: object

       additionalProperties: false

(and a blank line after)

> +    patternProperties:
> +      '_grp[0-9]$':

s/_/-/

> +        type: object
> +        allOf:
> +          - $ref: pinmux-node.yaml#
> +          - $ref: pincfg-node.yaml#

           unevaluatedProperties: false

> +        description:
> +          Pinctrl node's client devices specify pin muxes using subnodes,
> +          which in turn use the standard properties below.
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    pinctrl: siul2-pinctrl@4009c240 {

pinctrl@...

> +        compatible = "nxp,s32g-siul2-pinctrl";
> +
> +              /* MSCR range */
> +        reg = <0x4009c240 0x198>,
> +              <0x44010400 0x2c>,
> +              <0x44010480 0xbc>,
> +              /* MSCR range */
> +              <0x4009ca40 0x150>,
> +              <0x44010c1c 0x45c>,
> +              <0x440110f8 0x108>;

What is in these holes in the memory map? Is this part of some larger 
block? If so, that block needs to be described.

> +
> +                   /* MSCR range */
> +        nxp,pins = <0   101>,
> +                   <112 122>,
> +                   <144 190>,
> +                   /* IMCR range */
> +                   <512 595>,
> +                   <631 909>,
> +                   <942 1007>;
> +
> +        llce_can0_pins {
> +            llce_can0_grp0 {
> +                pinmux = <0x2b0>;
> +                input-enable;
> +                slew-rate = <0x00>;
> +            };
> +
> +            llce_can0_grp1 {
> +                pinmux = <0x2c2>;
> +                output-enable;
> +                slew-rate = <0x00>;
> +            };
> +        };
> +    };
> +...
> -- 
> 2.37.3
> 
>

Hi Chester,

thanks for your patch!

On Mon, Oct 31, 2022 at 11:09 AM Chester Lin <clin@suse.com> 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@nxp.com>
> Signed-off-by: Chester Lin <clin@suse.com>
(...)
> +  nxp,pins:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description:
> +      A list of [start, end] pin ID boundaries that correspond to each of
> +      the register regions reserved.

That's an interesting approach.

But what about just hardcoding this into the driver instead?

If you have the compatible, surely you know these indexes from
that compatible string?

Yours,
Linus Walleij

Hi Rob,

Thank you for reviewing this patch.

On Wed, Nov 02, 2022 at 10:49:03AM -0500, Rob Herring wrote:
> On Mon, Oct 31, 2022 at 06:08:42PM +0800, 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@nxp.com>
> > Signed-off-by: Chester Lin <clin@suse.com>
> > ---
> >  .../pinctrl/nxp,s32cc-siul2-pinctrl.yaml      | 91 +++++++++++++++++++
> >  1 file changed, 91 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> > new file mode 100644
> > index 000000000000..eafb9091cbf7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> > @@ -0,0 +1,91 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (c) 2022 NXP
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pinctrl/nxp,s32cc-siul2-pinctrl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP S32 Common Chassis SIUL2 iomux controller
> > +
> > +maintainers:
> > +  - Ghennadi Procopciuc <Ghennadi.Procopciuc@nxp.com>
> > +  - Chester Lin <clin@suse.com>
> > +
> > +description: |
> > +  Core driver for the pin controller found on S32 Common Chassis SoC.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - nxp,s32g-siul2-pinctrl
> > +
> > +  reg:
> > +    minItems: 5
> > +    maxItems: 6
> > +    description: A list of register regions to be reserved.
> 
> Need to be explicit about what each entry is.
> 

They are
  - MSCR registers group 0 managed by the SIUL2 controller #0
  - MSCR registers group 1 managed by the SIUL2 controller #1
  - MSCR registers group 2 managed by the SIUL2 controller #1
  - IMCR registers group 0 managed by the SIUL2 controller #0
  - IMCR registers group 1 managed by the SIUL2 controller #1
  - IMCR registers group 2 managed by the SIUL2 controller #1

  - 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.


I will add descriptions for each entry in v2, thanks.

> > +
> > +  nxp,pins:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    description:
> > +      A list of [start, end] pin ID boundaries that correspond to each of
> > +      the register regions reserved.
> 
> Looks like a matrix rather than an array.
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - nxp,pins
> > +
> > +patternProperties:
> > +  '_pins$':
> 
> s/_/-/
> 
> > +    type: object
> 
>        additionalProperties: false
> 
> (and a blank line after)
> 
> > +    patternProperties:
> > +      '_grp[0-9]$':
> 
> s/_/-/
> 
> > +        type: object
> > +        allOf:
> > +          - $ref: pinmux-node.yaml#
> > +          - $ref: pincfg-node.yaml#
> 
>            unevaluatedProperties: false
> 
> > +        description:
> > +          Pinctrl node's client devices specify pin muxes using subnodes,
> > +          which in turn use the standard properties below.
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> > +    pinctrl: siul2-pinctrl@4009c240 {
> 
> pinctrl@...
> 
> > +        compatible = "nxp,s32g-siul2-pinctrl";
> > +
> > +              /* MSCR range */
> > +        reg = <0x4009c240 0x198>,
> > +              <0x44010400 0x2c>,
> > +              <0x44010480 0xbc>,
> > +              /* MSCR range */

It's IMCR range but not the 2nd MSCR range, will fix it in v2.

> > +              <0x4009ca40 0x150>,
> > +              <0x44010c1c 0x45c>,
> > +              <0x440110f8 0x108>;
> 
> What is in these holes in the memory map? Is this part of some larger 
> block? If so, that block needs to be described.
> 

In this case S32G contains two SIUL2 controllers, which are located in two
different memory regions, siul2_0@4009C000 and siul2_1@44010000. The siul2_0
contains the MSCR0-MSCR101 [starts at 0x4009c240] and IMCR0-IMCR83 [starts
at 0x4009ca40] registers, and there are some other reserved regions and
registers which are not related to pin muxing in siul2 memory ranges. Not sure
what the original reason it could be, but the expanded MSCR112-MSCR122, MSCR144
-MSCR190 and IMCR119-IMCR397, IMCR430-IMCR495 registers are present in
siul2_1's memory space. Besides, in the S32G pin map, some indexes are hidden
since they're not used as pinouts, such as MSCR indexes 102-111, 123-143, and
IMCR indexes 84-118, 398-429.

Anyway, that's why they look so fragmented. I will add more descriptions for
each entry in v2.

Thanks,
Chester

> > +
> > +                   /* MSCR range */
> > +        nxp,pins = <0   101>,
> > +                   <112 122>,
> > +                   <144 190>,
> > +                   /* IMCR range */
> > +                   <512 595>,
> > +                   <631 909>,
> > +                   <942 1007>;
> > +
> > +        llce_can0_pins {
> > +            llce_can0_grp0 {
> > +                pinmux = <0x2b0>;
> > +                input-enable;
> > +                slew-rate = <0x00>;
> > +            };
> > +
> > +            llce_can0_grp1 {
> > +                pinmux = <0x2c2>;
> > +                output-enable;
> > +                slew-rate = <0x00>;
> > +            };
> > +        };
> > +    };
> > +...
> > -- 
> > 2.37.3
> > 
> >

Hi Linus,

On Tue, Nov 08, 2022 at 01:31:16PM +0100, Linus Walleij wrote:
> Hi Chester,
> 
> thanks for your patch!
> 

Thanks for reviewing this patch series as well!

> On Mon, Oct 31, 2022 at 11:09 AM Chester Lin <clin@suse.com> 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@nxp.com>
> > Signed-off-by: Chester Lin <clin@suse.com>
> (...)
> > +  nxp,pins:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    description:
> > +      A list of [start, end] pin ID boundaries that correspond to each of
> > +      the register regions reserved.
> 
> That's an interesting approach.
> 
> But what about just hardcoding this into the driver instead?
> 
> If you have the compatible, surely you know these indexes from
> that compatible string?
> 

The nxp,pins property is more like a common language that both kernel and
u-boot can share with. Of course hardcoding indexes can be achieved as well
but that also means the index matrices in both kernel driver and u-boot driver
must be revised and synchronized if new boards may have different reg and
nxp,pins. IMHO, having these ID ranges in DT could be more intuitive, developers
can easily compare every pair with the corresponding reg entry in the same dts
file since reg maps are fragmented as I mentioned in Rob's code review[1].
Based on this approach, both kernel driver and u-boot driver can reuse the
same parser to acquire pin ID ranges from different boards, which can also
simplify codes and have less patches in the future.

Regards,
Chester

[1] https://lkml.org/lkml/2022/11/9/722

> Yours,
> Linus Walleij

Hi Chester and Linus,

> > > If you have the compatible, surely you know these indexes from that 
> > > compatible string?
> >
> > The nxp,pins property is more like a common language that both kernel 
> > and u-boot can share with. Of course hardcoding indexes can be 
> > achieved as well but that also means the index matrices in both kernel 
> > driver and u-boot driver must be revised and synchronized if new 
> > boards may have different reg and nxp,pins.
> 
> Is it new *BOARDS* that need a new table really or new *SOC:s*?
> 
> I was under the impression that this is per-soc and then it can be derived from the compatible string and should be in the driver.

Yes, the "nxp,pins" property only depends on the SoC. It will not change from board to board(boards using the same SoC). It can be derived from the compatible string.

Best regards,
Andrei