On Mon, Dec 05, 2022 at 10:02:14AM +0100, Krzysztof Kozlowski wrote:
> On 05/12/2022 07:16, Chester Lin wrote:
> > Hi Krzysztof,
> >
> > On Wed, Nov 30, 2022 at 03:58:52PM +0100, Krzysztof Kozlowski wrote:
> >> On 28/11/2022 06:48, 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@xxxxxxx>
> >>> Signed-off-by: Andrei Stefanescu <andrei.stefanescu@xxxxxxx>
> >>> Signed-off-by: Chester Lin <clin@xxxxxxxx>
> >>> ---
> >>>
> >>> 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,s32cc-siul2-pinctrl.yaml | 125 ++++++++++++++++++
> >>> 1 file changed, 125 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..2fc25a9362af
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/pinctrl/nxp,s32cc-siul2-pinctrl.yaml
> >>
> >> Usually filename matches the compatible (or family name), so any reason
> >> why compatible is "nxp,s32g2" but filename is "nxp,s32cc"?
> >>
> >
> > According to NXP, the S32CC is a microarch which is adapted by different S32 SoCs,
> > such as S32G2/G3 and S32R45. Some common IPs are implemented in S32CC, such as
> > serial, pinctrl, mmc, gmac and some other peripheral interfaces. S32R45 has
> > different pinouts compared to S32G2, which means that there would not be just
> > "s32g2-siul2-pinctrl" but also "s32r45-siul2-pinctrl" in the compatible enum if
> > S32R45 has to be upstreamed in the future. For this case, it seems to be
> > inappropriate that adding a compatible name without any "s32g" keyword in the
> > filename "nxp,s32g2-.." unless creating a new yaml for each platform, such as
> > nxp,s32r45-siul2-pinctl.yaml.
>
> First, you can always rename a file if such need arises. Maybe new SoCs
> will come, maybe not.
>
> Second, when you actually upstream new SoC it might anyway require new
> bindings file, because pinctrls are quite specific and it is usually
> difficult to support multiple devices in a nice, readable way in one
> file. Therefore anyway another file is quite likely.
>
Thanks for your guidance. Will fix it.
> (...)
>
> >>> +
> >>> +patternProperties:
> >>> + '-pins$':
> >>> + type: object
> >>> + additionalProperties: false
> >>> +
> >>> + patternProperties:
> >>> + '-grp[0-9]$':
> >>> + 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.
> >>
> >> All properties are accepted? What about values, e.g. for drive strength?
> >
> > For those unsupported properties such as drive-strength, the s32g2 pinctrl driver
> > returns -EOPNOTSUPP.
>
> I don't care what the driver is doing, we do not discuss the driver. You
> need to describe properly the hardware and I doubt that hardware accepts
> all drive-strengths, all forms of pull resistors (so any Ohm value).
>
> Add constrains.
>
Thanks for the suggestion. IIUC, I should specifically described the supported
pinmux and pincfg properties in this schema and then add an "additionalProperties: false"
in the end in order to constrain unsupported properties listed in the pattern
pin groups.
> >>
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> + - |
> >>> +
> >>> + /* Pins functions (SSS field) */
> >>> + #define FUNC0 0
> >>> + #define FUNC1 1
> >>> + #define FUNC2 2
> >>> + #define FUNC3 3
> >>> + #define FUNC4 4
> >>> + #define FUNC5 5
> >>> + #define FUNC6 6
> >>> + #define FUNC7 7
>
> This is another surprise - functions are texts, not numbers.
>
Maybe the FUNC[0|9] are not accurate to describe Source Signal Select [SSS].
I will drop these definitions from the example and try elaborating 'pinmux'
in its property description.
> >>> +
> >>> + #define S32CC_PINMUX(PIN, FUNC) (((PIN) << 4) | (FUNC))
> >>> +
> >>> + #define S32CC_SLEW_208MHZ 0
> >>> + #define S32CC_SLEW_166MHZ 4
> >>> + #define S32CC_SLEW_150MHZ 5
> >>> + #define S32CC_SLEW_133MHZ 6
> >>> + #define S32CC_SLEW_83MHZ 7
>
> Don't store register values in the bindings examples. Instead you need
> to be explain the slew-rate property.
>
Will do.
> >>> +
> >>> + pinctrl@4009c240 {
> >>> + compatible = "nxp,s32g2-siul2-pinctrl";
> >>> +
> >>> + /*
> >>> + * There are two SIUL2 controllers in S32G2:
> >>> + *
> >>> + * siul2_0 @ 0x4009c000
> >>> + * siul2_1 @ 0x44010000
> >>> + *
> >>> + * Every SIUL2 controller has multiple register types, and here
> >>> + * only MSCR and IMCR registers need to be revealed for kernel
> >>> + * to configure pinmux. Please note that some indexes are reserved,
> >>> + * such as MSCR102-MSCR111 in the following reg property.
> >>> + */
> >>> +
> >>
> >> Either this should be part of description or should be dropped. It blows
> >> example and probably duplicates DTS.
> >>
> >>
> >> Best regards,
> >> Krzysztof
> >>
>
> Best regards,
> Krzysztof
>
