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

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>>  .../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