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

[Peter Griffin <peter.griffin@linaro.org>]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel