2021-08-05 07:23:43

by Chester Lin

[permalink] [raw]
Subject: [PATCH 3/8] dt-bindings: serial: fsl-linflexuart: Add compatible for S32G2

Add a compatible string for the uart binding of NXP S32G2 platforms. Here
we use "s32v234-linflexuart" as fallback since the current linflexuart
driver still works on S32G2.

Signed-off-by: Chester Lin <[email protected]>
---
.../bindings/serial/fsl,s32-linflexuart.yaml | 26 ++++++++++++++++---
1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
index acfe34706ccb..e731f3f6cea4 100644
--- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
+++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
@@ -21,10 +21,20 @@ allOf:

properties:
compatible:
- description: The LINFlexD controller on S32V234 SoC, which can be
- configured in UART mode.
- items:
- - const: fsl,s32v234-linflexuart
+ minItems: 1
+ maxItems: 2
+ oneOf:
+ - description: The LINFlexD controller on S32G2 SoC, which can be
+ configured in UART mode.
+ items:
+ - enum:
+ - fsl,s32g2-linflexuart
+ - const: fsl,s32v234-linflexuart
+
+ - description: The LINFlexD controller on S32V234 SoC, which can be
+ configured in UART mode.
+ items:
+ - const: fsl,s32v234-linflexuart

reg:
maxItems: 1
@@ -41,8 +51,16 @@ unevaluatedProperties: false

examples:
- |
+ /* S32V234 */
serial@40053000 {
compatible = "fsl,s32v234-linflexuart";
reg = <0x40053000 0x1000>;
interrupts = <0 59 4>;
};
+
+ /* S32G2 */
+ serial@401c8000 {
+ compatible = "fsl,s32g2-linflexuart", "fsl,s32v234-linflexuart";
+ reg = <0x401c8000 0x3000>;
+ interrupts = <0 82 1>;
+ };
--
2.30.0


2021-08-12 16:30:14

by Andreas Färber

[permalink] [raw]
Subject: Re: [PATCH 3/8] dt-bindings: serial: fsl-linflexuart: Add compatible for S32G2

On 05.08.21 08:54, Chester Lin wrote:
> Add a compatible string for the uart binding of NXP S32G2 platforms. Here
> we use "s32v234-linflexuart" as fallback since the current linflexuart
> driver still works on S32G2.
>
> Signed-off-by: Chester Lin <[email protected]>
> ---
> .../bindings/serial/fsl,s32-linflexuart.yaml | 26 ++++++++++++++++---
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
> index acfe34706ccb..e731f3f6cea4 100644
> --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
> +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
> @@ -21,10 +21,20 @@ allOf:
>
> properties:
> compatible:
> - description: The LINFlexD controller on S32V234 SoC, which can be
> - configured in UART mode.
> - items:
> - - const: fsl,s32v234-linflexuart
> + minItems: 1
> + maxItems: 2

Are these necessary for oneOf?

> + oneOf:
> + - description: The LINFlexD controller on S32G2 SoC, which can be
> + configured in UART mode.
> + items:
> + - enum:
> + - fsl,s32g2-linflexuart
> + - const: fsl,s32v234-linflexuart

This reads inconsistent to me: Either this oneOf is for S32G2 only, then
please turn the enum into a const. Or change the description to "on SoCs
compatible with S32V234" if we expect the enum list to grow.

I believe the idea here was to avoid unnecessary driver compatible and
earlycon compatible additions, while preparing for eventual quirks
specific to S32G2.

@NXP: Should this be s32g2- like above or s32g274a- specifically? Do you
agree this is a useful thing to prepare here, as opposed to using only
s32v234- in the s32g2* DT?

I assume the ordering is done alphabetically as S32G < S32V;
alternatively we might order S32V234 first and then the compatible ones.

> +
> + - description: The LINFlexD controller on S32V234 SoC, which can be
> + configured in UART mode.
> + items:
> + - const: fsl,s32v234-linflexuart

To minimize this S32G2 patch, would it be valid to do oneOf for the
single S32V in the preceding patch already? Then we would avoid the text
movement and re-indentation above and more easily see the lines newly
getting added for S32G2.

>
> reg:
> maxItems: 1
> @@ -41,8 +51,16 @@ unevaluatedProperties: false
>
> examples:
> - |
> + /* S32V234 */

Could this be:
- description: S32V234
|
?

> serial@40053000 {
> compatible = "fsl,s32v234-linflexuart";
> reg = <0x40053000 0x1000>;
> interrupts = <0 59 4>;
> };
> +
> + /* S32G2 */

This should not be part of the S32V example, but a new one:

- |

(or with description, as discussed above)

> + serial@401c8000 {
> + compatible = "fsl,s32g2-linflexuart", "fsl,s32v234-linflexuart";

Potentially affected by naming discussions above.

> + reg = <0x401c8000 0x3000>;
> + interrupts = <0 82 1>;
> + };

Regards,
Andreas

--
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer
HRB 36809 (AG Nürnberg)

2021-08-13 14:40:39

by Radu Pirea (NXP OSS)

[permalink] [raw]
Subject: Re: [PATCH 3/8] dt-bindings: serial: fsl-linflexuart: Add compatible for S32G2

On Thu, 2021-08-12 at 18:27 +0200, Andreas Färber wrote:
> On 05.08.21 08:54, Chester Lin wrote:
> > Add a compatible string for the uart binding of NXP S32G2
> > platforms. Here
> > we use "s32v234-linflexuart" as fallback since the current
> > linflexuart
> > driver still works on S32G2.
> >
> > Signed-off-by: Chester Lin <[email protected]>
> > ---
> >  .../bindings/serial/fsl,s32-linflexuart.yaml  | 26
> > ++++++++++++++++---
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-
> > linflexuart.yaml
> > b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
> > index acfe34706ccb..e731f3f6cea4 100644
> > --- a/Documentation/devicetree/bindings/serial/fsl,s32-
> > linflexuart.yaml
> > +++ b/Documentation/devicetree/bindings/serial/fsl,s32-
> > linflexuart.yaml
> > @@ -21,10 +21,20 @@ allOf:
> >  
> >  properties:
> >    compatible:
> > -    description: The LINFlexD controller on S32V234 SoC, which can
> > be
> > -      configured in UART mode.
> > -    items:
> > -      - const: fsl,s32v234-linflexuart
> > +    minItems: 1
> > +    maxItems: 2
>
> Are these necessary for oneOf?
>
> > +    oneOf:
> > +      - description: The LINFlexD controller on S32G2 SoC, which
> > can be
> > +          configured in UART mode.
> > +        items:
> > +          - enum:
> > +              - fsl,s32g2-linflexuart
> > +          - const: fsl,s32v234-linflexuart
>
> This reads inconsistent to me: Either this oneOf is for S32G2 only,
> then
> please turn the enum into a const. Or change the description to "on
> SoCs
> compatible with S32V234" if we expect the enum list to grow.
>
> I believe the idea here was to avoid unnecessary driver compatible
> and
> earlycon compatible additions, while preparing for eventual quirks
> specific to S32G2.
>
> @NXP: Should this be s32g2- like above or s32g274a- specifically? Do
> you
> agree this is a useful thing to prepare here, as opposed to using
> only
> s32v234- in the s32g2* DT?

s32g2- is fine, but the vendor should be nxp, not fsl.
nxp,s32g2-linflexuart

>
> I assume the ordering is done alphabetically as S32G < S32V;
> alternatively we might order S32V234 first and then the compatible
> ones.
>
> > +
> > +      - description: The LINFlexD controller on S32V234 SoC, which
> > can be
> > +          configured in UART mode.
> > +        items:
> > +          - const: fsl,s32v234-linflexuart
>
> To minimize this S32G2 patch, would it be valid to do oneOf for the
> single S32V in the preceding patch already? Then we would avoid the
> text
> movement and re-indentation above and more easily see the lines newly
> getting added for S32G2.
>
> >  
> >    reg:
> >      maxItems: 1
> > @@ -41,8 +51,16 @@ unevaluatedProperties: false
> >  
> >  examples:
> >    - |
> > +    /* S32V234 */
>
> Could this be:
>   - description: S32V234
>     |
> ?
>
> >      serial@40053000 {
> >          compatible = "fsl,s32v234-linflexuart";
> >          reg = <0x40053000 0x1000>;
> >          interrupts = <0 59 4>;
> >      };
> > +
> > +    /* S32G2 */
>
> This should not be part of the S32V example, but a new one:
>
>   - |
>
> (or with description, as discussed above)
>
> > +    serial@401c8000 {
> > +        compatible = "fsl,s32g2-linflexuart", "fsl,s32v234-
> > linflexuart";
>
> Potentially affected by naming discussions above.
>
> > +        reg = <0x401c8000 0x3000>;
> > +        interrupts = <0 82 1>;
> > +    };
>
> Regards,
> Andreas
>


2021-08-13 18:11:17

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 3/8] dt-bindings: serial: fsl-linflexuart: Add compatible for S32G2

On Thu, Aug 05, 2021 at 02:54:24PM +0800, Chester Lin wrote:
> Add a compatible string for the uart binding of NXP S32G2 platforms. Here
> we use "s32v234-linflexuart" as fallback since the current linflexuart
> driver still works on S32G2.
>
> Signed-off-by: Chester Lin <[email protected]>
> ---
> .../bindings/serial/fsl,s32-linflexuart.yaml | 26 ++++++++++++++++---
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
> index acfe34706ccb..e731f3f6cea4 100644
> --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
> +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
> @@ -21,10 +21,20 @@ allOf:
>
> properties:
> compatible:
> - description: The LINFlexD controller on S32V234 SoC, which can be
> - configured in UART mode.
> - items:
> - - const: fsl,s32v234-linflexuart
> + minItems: 1
> + maxItems: 2

minItems/maxItems not needed here.

> + oneOf:
> + - description: The LINFlexD controller on S32G2 SoC, which can be
> + configured in UART mode.
> + items:
> + - enum:
> + - fsl,s32g2-linflexuart
> + - const: fsl,s32v234-linflexuart
> +
> + - description: The LINFlexD controller on S32V234 SoC, which can be
> + configured in UART mode.
> + items:
> + - const: fsl,s32v234-linflexuart
>
> reg:
> maxItems: 1
> @@ -41,8 +51,16 @@ unevaluatedProperties: false
>
> examples:
> - |
> + /* S32V234 */
> serial@40053000 {
> compatible = "fsl,s32v234-linflexuart";
> reg = <0x40053000 0x1000>;
> interrupts = <0 59 4>;
> };
> +
> + /* S32G2 */
> + serial@401c8000 {
> + compatible = "fsl,s32g2-linflexuart", "fsl,s32v234-linflexuart";
> + reg = <0x401c8000 0x3000>;
> + interrupts = <0 82 1>;
> + };

This doesn't really warrant another example just for a new compatible
string.

2021-08-13 18:15:19

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH 3/8] dt-bindings: serial: fsl-linflexuart: Add compatible for S32G2

On Thu, Aug 12, 2021 at 06:27:57PM +0200, Andreas F?rber wrote:
> On 05.08.21 08:54, Chester Lin wrote:
> > Add a compatible string for the uart binding of NXP S32G2 platforms. Here
> > we use "s32v234-linflexuart" as fallback since the current linflexuart
> > driver still works on S32G2.
> >
> > Signed-off-by: Chester Lin <[email protected]>
> > ---
> > .../bindings/serial/fsl,s32-linflexuart.yaml | 26 ++++++++++++++++---
> > 1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
> > index acfe34706ccb..e731f3f6cea4 100644
> > --- a/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
> > +++ b/Documentation/devicetree/bindings/serial/fsl,s32-linflexuart.yaml
> > @@ -21,10 +21,20 @@ allOf:
> >
> > properties:
> > compatible:
> > - description: The LINFlexD controller on S32V234 SoC, which can be
> > - configured in UART mode.
> > - items:
> > - - const: fsl,s32v234-linflexuart
> > + minItems: 1
> > + maxItems: 2
>
> Are these necessary for oneOf?
>
> > + oneOf:
> > + - description: The LINFlexD controller on S32G2 SoC, which can be
> > + configured in UART mode.
> > + items:
> > + - enum:
> > + - fsl,s32g2-linflexuart
> > + - const: fsl,s32v234-linflexuart
>
> This reads inconsistent to me: Either this oneOf is for S32G2 only, then
> please turn the enum into a const. Or change the description to "on SoCs
> compatible with S32V234" if we expect the enum list to grow.
>
> I believe the idea here was to avoid unnecessary driver compatible and
> earlycon compatible additions, while preparing for eventual quirks
> specific to S32G2.
>
> @NXP: Should this be s32g2- like above or s32g274a- specifically? Do you
> agree this is a useful thing to prepare here, as opposed to using only
> s32v234- in the s32g2* DT?
>
> I assume the ordering is done alphabetically as S32G < S32V;
> alternatively we might order S32V234 first and then the compatible ones.
>
> > +
> > + - description: The LINFlexD controller on S32V234 SoC, which can be
> > + configured in UART mode.
> > + items:
> > + - const: fsl,s32v234-linflexuart
>
> To minimize this S32G2 patch, would it be valid to do oneOf for the
> single S32V in the preceding patch already? Then we would avoid the text
> movement and re-indentation above and more easily see the lines newly
> getting added for S32G2.
>
> >
> > reg:
> > maxItems: 1
> > @@ -41,8 +51,16 @@ unevaluatedProperties: false
> >
> > examples:
> > - |
> > + /* S32V234 */
>
> Could this be:
> - description: S32V234
> |
> ?

No, that would not be valid json-schema.

Rob