diff mbox series

[v4,09/19] dt-bindings: serial: samsung: Make samsung,uart-fifosize required property

Message ID 20231120212037.911774-10-peter.griffin@linaro.org
State New
Headers show
Series Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board | expand

Commit Message

Peter Griffin Nov. 20, 2023, 9:20 p.m. UTC
Specifying samsung,uart-fifosize in both DT and driver static data is error
prone and relies on driver probe order and dt aliases to be correct.

Additionally on many Exynos platforms these are (USI) universal serial
interfaces which can be uart, spi or i2c, so it can change per board.

For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a
required property. For these platforms fifosize now *only* comes from DT.

It is hoped other Exynos platforms will also switch over time.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 .../bindings/serial/samsung_uart.yaml           | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Rob Herring (Arm) Nov. 20, 2023, 11:15 p.m. UTC | #1
On Mon, 20 Nov 2023 21:20:27 +0000, Peter Griffin wrote:
> Specifying samsung,uart-fifosize in both DT and driver static data is error
> prone and relies on driver probe order and dt aliases to be correct.
> 
> Additionally on many Exynos platforms these are (USI) universal serial
> interfaces which can be uart, spi or i2c, so it can change per board.
> 
> For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a
> required property. For these platforms fifosize now *only* comes from DT.
> 
> It is hoped other Exynos platforms will also switch over time.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  .../bindings/serial/samsung_uart.yaml           | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/serial/samsung_uart.yaml:141:8: [warning] wrong indentation: expected 8 but found 7 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231120212037.911774-10-peter.griffin@linaro.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
kernel test robot Nov. 21, 2023, 6:23 a.m. UTC | #2
Hi Peter,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pinctrl-samsung/for-next]
[also build test WARNING on next-20231120]
[cannot apply to krzk/for-next robh/for-next linus/master v6.7-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Peter-Griffin/dt-bindings-soc-samsung-exynos-pmu-Add-gs101-compatible/20231121-052449
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pinctrl/samsung.git for-next
patch link:    https://lore.kernel.org/r/20231120212037.911774-10-peter.griffin%40linaro.org
patch subject: [PATCH v4 09/19] dt-bindings: serial: samsung: Make samsung,uart-fifosize required property
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231121/202311211435.CJVOACBE-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311211435.CJVOACBE-lkp@intel.com/

dtcheck warnings: (new ones prefixed by >>)
>> Documentation/devicetree/bindings/serial/samsung_uart.yaml:141:8: [warning] wrong indentation: expected 8 but found 7 (indentation)

vim +141 Documentation/devicetree/bindings/serial/samsung_uart.yaml

     8	
     9	maintainers:
    10	  - Krzysztof Kozlowski <krzk@kernel.org>
    11	  - Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    12	
    13	description: |+
    14	  Each Samsung UART should have an alias correctly numbered in the "aliases"
    15	  node, according to serialN format, where N is the port number (non-negative
    16	  decimal integer) as specified by User's Manual of respective SoC.
    17	
    18	properties:
    19	  compatible:
    20	    oneOf:
    21	      - items:
    22	          - const: samsung,exynosautov9-uart
    23	          - const: samsung,exynos850-uart
    24	      - enum:
    25	          - apple,s5l-uart
    26	          - axis,artpec8-uart
    27	          - google,gs101-uart
    28	          - samsung,s3c6400-uart
    29	          - samsung,s5pv210-uart
    30	          - samsung,exynos4210-uart
    31	          - samsung,exynos5433-uart
    32	          - samsung,exynos850-uart
    33	
    34	  reg:
    35	    maxItems: 1
    36	
    37	  reg-io-width:
    38	    description: |
    39	      The size (in bytes) of the IO accesses that should be performed
    40	      on the device.
    41	    enum: [ 1, 4 ]
    42	
    43	  clocks:
    44	    minItems: 2
    45	    maxItems: 5
    46	
    47	  clock-names:
    48	    description: N = 0 is allowed for SoCs without internal baud clock mux.
    49	    minItems: 2
    50	    items:
    51	      - const: uart
    52	      - pattern: '^clk_uart_baud[0-3]$'
    53	      - pattern: '^clk_uart_baud[0-3]$'
    54	      - pattern: '^clk_uart_baud[0-3]$'
    55	      - pattern: '^clk_uart_baud[0-3]$'
    56	
    57	  dmas:
    58	    items:
    59	      - description: DMA controller phandle and request line for RX
    60	      - description: DMA controller phandle and request line for TX
    61	
    62	  dma-names:
    63	    items:
    64	      - const: rx
    65	      - const: tx
    66	
    67	  interrupts:
    68	    description: RX interrupt and optionally TX interrupt.
    69	    minItems: 1
    70	    maxItems: 2
    71	
    72	  power-domains:
    73	    maxItems: 1
    74	
    75	  samsung,uart-fifosize:
    76	    description: The fifo size supported by the UART channel.
    77	    $ref: /schemas/types.yaml#/definitions/uint32
    78	    enum: [16, 64, 256]
    79	
    80	required:
    81	  - compatible
    82	  - clocks
    83	  - clock-names
    84	  - interrupts
    85	  - reg
    86	
    87	allOf:
    88	  - $ref: serial.yaml#
    89	
    90	  - if:
    91	      properties:
    92	        compatible:
    93	          contains:
    94	            enum:
    95	              - samsung,s5pv210-uart
    96	    then:
    97	      properties:
    98	        clocks:
    99	          minItems: 2
   100	          maxItems: 3
   101	        clock-names:
   102	          minItems: 2
   103	          items:
   104	            - const: uart
   105	            - pattern: '^clk_uart_baud[0-1]$'
   106	            - pattern: '^clk_uart_baud[0-1]$'
   107	
   108	  - if:
   109	      properties:
   110	        compatible:
   111	          contains:
   112	            enum:
   113	              - apple,s5l-uart
   114	              - axis,artpec8-uart
   115	              - samsung,exynos4210-uart
   116	              - samsung,exynos5433-uart
   117	    then:
   118	      properties:
   119	        clocks:
   120	          maxItems: 2
   121	        clock-names:
   122	          items:
   123	            - const: uart
   124	            - const: clk_uart_baud0
   125	
   126	  - if:
   127	      properties:
   128	        compatible:
   129	          contains:
   130	            enum:
   131	              - google,gs101-uart
   132	              - samsung,exynosautov9-uart
   133	    then:
   134	      properties:
   135	        samsung,uart-fifosize:
   136	          description: The fifo size supported by the UART channel.
   137	          $ref: /schemas/types.yaml#/definitions/uint32
   138	          enum: [16, 64, 256]
   139	
   140	      required:
 > 141	       - samsung,uart-fifosize
   142
Rob Herring (Arm) Nov. 21, 2023, 3:16 p.m. UTC | #3
On Mon, Nov 20, 2023 at 09:20:27PM +0000, Peter Griffin wrote:
> Specifying samsung,uart-fifosize in both DT and driver static data is error
> prone and relies on driver probe order and dt aliases to be correct.
> 
> Additionally on many Exynos platforms these are (USI) universal serial
> interfaces which can be uart, spi or i2c, so it can change per board.
> 
> For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a
> required property. For these platforms fifosize now *only* comes from DT.
> 
> It is hoped other Exynos platforms will also switch over time.

Then allow the property on them.

> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  .../bindings/serial/samsung_uart.yaml           | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> index ccc3626779d9..22a1edadc4fe 100644
> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> @@ -133,6 +133,23 @@ allOf:
>              - const: uart
>              - const: clk_uart_baud0
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - google,gs101-uart
> +              - samsung,exynosautov9-uart
> +    then:
> +      properties:
> +        samsung,uart-fifosize:
> +          description: The fifo size supported by the UART channel.
> +          $ref: /schemas/types.yaml#/definitions/uint32
> +          enum: [16, 64, 256]

We already have 'fifo-size' in several drivers. Use that. Please move 
its type/description definitions to serial.yaml and make drivers just do 
'fifo-size: true' if they use it.

> +
> +      required:
> +       - samsung,uart-fifosize

A new required property is an ABI break. Please explain why that is okay 
in the commit message.

Rob
Peter Griffin Nov. 21, 2023, 5:15 p.m. UTC | #4
Hi Rob,

Thanks for your review.

On Tue, 21 Nov 2023 at 15:16, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Nov 20, 2023 at 09:20:27PM +0000, Peter Griffin wrote:
> > Specifying samsung,uart-fifosize in both DT and driver static data is error
> > prone and relies on driver probe order and dt aliases to be correct.
> >
> > Additionally on many Exynos platforms these are (USI) universal serial
> > interfaces which can be uart, spi or i2c, so it can change per board.
> >
> > For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a
> > required property. For these platforms fifosize now *only* comes from DT.
> >
> > It is hoped other Exynos platforms will also switch over time.
>
> Then allow the property on them.

Not sure I fully understand your comment. Can you elaborate? Do you
mean leave the 'samsung,uart-fifosize' as an optional property like it
is currently even for the platforms that now require it to be present
to function correctly?

I deliberately restricted the yaml change to only require this
property for the SoCs that already set the 'samsung,uart-fifosize'  dt
property. As setting the property and having the driver use what is
specified in DT also requires a corresponding driver update (otherwise
fifosize gets overwritten by the driver static data, and then becomes
dependent on probe order, dt aliases etc). The rationale was drivers
'opt in' and add themselves to the compatibles in this patch as they
migrate away from obtaining fifo size from driver static data to
obtaining it from DT.

>
> >
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> >  .../bindings/serial/samsung_uart.yaml           | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> > index ccc3626779d9..22a1edadc4fe 100644
> > --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> > +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> > @@ -133,6 +133,23 @@ allOf:
> >              - const: uart
> >              - const: clk_uart_baud0
> >
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - google,gs101-uart
> > +              - samsung,exynosautov9-uart
> > +    then:
> > +      properties:
> > +        samsung,uart-fifosize:
> > +          description: The fifo size supported by the UART channel.
> > +          $ref: /schemas/types.yaml#/definitions/uint32
> > +          enum: [16, 64, 256]
>
> We already have 'fifo-size' in several drivers. Use that. Please move
> its type/description definitions to serial.yaml and make drivers just do
> 'fifo-size: true' if they use it.

What do you suggest we do for the samsung,uart-fifosize property that
is being used upstream?

>
> > +
> > +      required:
> > +       - samsung,uart-fifosize
>
> A new required property is an ABI break. Please explain why that is okay
> in the commit message.
>

I can update the commit message to make clear there is an ABI break.
As mentioned above the platforms where this is now required are either
already setting the property or are new in this series. Is that
sufficient justification?

regards,

Peter
Krzysztof Kozlowski Nov. 22, 2023, 7:49 a.m. UTC | #5
On 21/11/2023 18:15, Peter Griffin wrote:
> Hi Rob,
> 
> Thanks for your review.
> 
> On Tue, 21 Nov 2023 at 15:16, Rob Herring <robh@kernel.org> wrote:
>>
>> On Mon, Nov 20, 2023 at 09:20:27PM +0000, Peter Griffin wrote:
>>> Specifying samsung,uart-fifosize in both DT and driver static data is error
>>> prone and relies on driver probe order and dt aliases to be correct.
>>>
>>> Additionally on many Exynos platforms these are (USI) universal serial
>>> interfaces which can be uart, spi or i2c, so it can change per board.
>>>
>>> For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a
>>> required property. For these platforms fifosize now *only* comes from DT.
>>>
>>> It is hoped other Exynos platforms will also switch over time.
>>
>> Then allow the property on them.
> 
> Not sure I fully understand your comment. Can you elaborate? Do you
> mean leave the 'samsung,uart-fifosize' as an optional property like it
> is currently even for the platforms that now require it to be present
> to function correctly?
> 
> I deliberately restricted the yaml change to only require this
> property for the SoCs that already set the 'samsung,uart-fifosize'  dt
> property. As setting the property and having the driver use what is
> specified in DT also requires a corresponding driver update (otherwise
> fifosize gets overwritten by the driver static data, and then becomes
> dependent on probe order, dt aliases etc). The rationale was drivers
> 'opt in' and add themselves to the compatibles in this patch as they
> migrate away from obtaining fifo size from driver static data to
> obtaining it from DT.

Your code diff looks like you are adding the property only to these models.

> 
>>
>>>
>>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
>>> ---
>>>  .../bindings/serial/samsung_uart.yaml           | 17 +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
>>> index ccc3626779d9..22a1edadc4fe 100644
>>> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
>>> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
>>> @@ -133,6 +133,23 @@ allOf:
>>>              - const: uart
>>>              - const: clk_uart_baud0
>>>
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - google,gs101-uart
>>> +              - samsung,exynosautov9-uart
>>> +    then:
>>> +      properties:
>>> +        samsung,uart-fifosize:
>>> +          description: The fifo size supported by the UART channel.
>>> +          $ref: /schemas/types.yaml#/definitions/uint32
>>> +          enum: [16, 64, 256]
>>
>> We already have 'fifo-size' in several drivers. Use that. Please move
>> its type/description definitions to serial.yaml and make drivers just do
>> 'fifo-size: true' if they use it.
> 
> What do you suggest we do for the samsung,uart-fifosize property that
> is being used upstream?

Nothing, your diff is just wrong. Or at least nothing needed. Just drop
all this properties: here and only make it required for Google GS101.


> 
>>
>>> +
>>> +      required:
>>> +       - samsung,uart-fifosize
>>
>> A new required property is an ABI break. Please explain why that is okay
>> in the commit message.
>>
> 
> I can update the commit message to make clear there is an ABI break.
> As mentioned above the platforms where this is now required are either
> already setting the property or are new in this series. Is that
> sufficient justification?
Yes, but only first case. You need to order your patches correctly -
first is ABI break expecting ExynopsAutov9 to provide FIFO size in DTS
with its explanation. Second commit is adding GS101 where there is no
ABI break.

Best regards,
Krzysztof
Peter Griffin Nov. 22, 2023, 8:42 a.m. UTC | #6
Hi Krzysztof,

On Wed, 22 Nov 2023 at 07:49, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 21/11/2023 18:15, Peter Griffin wrote:
> > Hi Rob,
> >
> > Thanks for your review.
> >
> > On Tue, 21 Nov 2023 at 15:16, Rob Herring <robh@kernel.org> wrote:
> >>
> >> On Mon, Nov 20, 2023 at 09:20:27PM +0000, Peter Griffin wrote:
> >>> Specifying samsung,uart-fifosize in both DT and driver static data is error
> >>> prone and relies on driver probe order and dt aliases to be correct.
> >>>
> >>> Additionally on many Exynos platforms these are (USI) universal serial
> >>> interfaces which can be uart, spi or i2c, so it can change per board.
> >>>
> >>> For google,gs101-uart and exynosautov9-uart make samsung,uart-fifosize a
> >>> required property. For these platforms fifosize now *only* comes from DT.
> >>>
> >>> It is hoped other Exynos platforms will also switch over time.
> >>
> >> Then allow the property on them.
> >
> > Not sure I fully understand your comment. Can you elaborate? Do you
> > mean leave the 'samsung,uart-fifosize' as an optional property like it
> > is currently even for the platforms that now require it to be present
> > to function correctly?
> >
> > I deliberately restricted the yaml change to only require this
> > property for the SoCs that already set the 'samsung,uart-fifosize'  dt
> > property. As setting the property and having the driver use what is
> > specified in DT also requires a corresponding driver update (otherwise
> > fifosize gets overwritten by the driver static data, and then becomes
> > dependent on probe order, dt aliases etc). The rationale was drivers
> > 'opt in' and add themselves to the compatibles in this patch as they
> > migrate away from obtaining fifo size from driver static data to
> > obtaining it from DT.
>
> Your code diff looks like you are adding the property only to these models.

OK, I intended to only make it a required DT property for these
models. Presumably there is a better way to achieve that.

>
> >
> >>
> >>>
> >>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> >>> ---
> >>>  .../bindings/serial/samsung_uart.yaml           | 17 +++++++++++++++++
> >>>  1 file changed, 17 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> >>> index ccc3626779d9..22a1edadc4fe 100644
> >>> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> >>> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> >>> @@ -133,6 +133,23 @@ allOf:
> >>>              - const: uart
> >>>              - const: clk_uart_baud0
> >>>
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            enum:
> >>> +              - google,gs101-uart
> >>> +              - samsung,exynosautov9-uart
> >>> +    then:
> >>> +      properties:
> >>> +        samsung,uart-fifosize:
> >>> +          description: The fifo size supported by the UART channel.
> >>> +          $ref: /schemas/types.yaml#/definitions/uint32
> >>> +          enum: [16, 64, 256]
> >>
> >> We already have 'fifo-size' in several drivers. Use that. Please move
> >> its type/description definitions to serial.yaml and make drivers just do
> >> 'fifo-size: true' if they use it.
> >
> > What do you suggest we do for the samsung,uart-fifosize property that
> > is being used upstream?
>
> Nothing, your diff is just wrong. Or at least nothing needed. Just drop
> all this properties: here and only make it required for Google GS101.

OK, so your happy with the approach just not the implementation/patch
and you don't want it updated to use fifo-size instead of
samsung,uart-fifosize

>
>
> >
> >>
> >>> +
> >>> +      required:
> >>> +       - samsung,uart-fifosize
> >>
> >> A new required property is an ABI break. Please explain why that is okay
> >> in the commit message.
> >>
> >
> > I can update the commit message to make clear there is an ABI break.
> > As mentioned above the platforms where this is now required are either
> > already setting the property or are new in this series. Is that
> > sufficient justification?
> Yes, but only first case. You need to order your patches correctly -
> first is ABI break expecting ExynopsAutov9 to provide FIFO size in DTS
> with its explanation. Second commit is adding GS101 where there is no
> ABI break.

OK, I'll drop the ExynopsAutov9 part then. I don't want to complicate
this series by introducing an ABI breakage on some other unrelated
Exynos platform.

Peter
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
index ccc3626779d9..22a1edadc4fe 100644
--- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
+++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
@@ -133,6 +133,23 @@  allOf:
             - const: uart
             - const: clk_uart_baud0
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - google,gs101-uart
+              - samsung,exynosautov9-uart
+    then:
+      properties:
+        samsung,uart-fifosize:
+          description: The fifo size supported by the UART channel.
+          $ref: /schemas/types.yaml#/definitions/uint32
+          enum: [16, 64, 256]
+
+      required:
+       - samsung,uart-fifosize
+
 unevaluatedProperties: false
 
 examples: