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

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

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@xxxxxxx>
>>> Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@xxxxxxxxxxx>
>>> Signed-off-by: Chester Lin <clin@xxxxxxxx>
>>> ---
>>>
>>> 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@xxxxxxxxxxx>
>>> +  - Chester Lin <clin@xxxxxxxx>
>>> +
>>> +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@xxxxxxxx>
> 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@xxxxxxx>
> Signed-off-by: Ghennadi Procopciuc <Ghennadi.Procopciuc@xxxxxxxxxxx>
> Signed-off-by: Chester Lin <clin@xxxxxxxx>
> ---
>  .../pinctrl/nxp,s32g2-siul2-pinctrl.yaml      | 136 ++++++++++++++++++
>  include/dt-bindings/pinctrl/s32g2-pinfunc.h   |  17 +++

NAK for bindings.


Best regards,
Krzysztof