diff mbox series

[v2,08/20] dt-bindings: serial: samsung: Add google-gs101-uart compatible

Message ID 20231010224928.2296997-9-peter.griffin@linaro.org (mailing list archive)
State Not Applicable, archived
Headers show
Series Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board | expand

Commit Message

Peter Griffin Oct. 10, 2023, 10:49 p.m. UTC
Add dedicated google-gs101-uart compatible to the dt-schema for
representing uart of the Google Tensor gs101 SoC.

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

Comments

Tudor Ambarus Oct. 11, 2023, 8:49 a.m. UTC | #1
Hi, Greg,

On 10/11/23 08:48, Greg KH wrote:
> On Tue, Oct 10, 2023 at 11:49:16PM +0100, Peter Griffin wrote:
>> Add dedicated google-gs101-uart compatible to the dt-schema for
>> representing uart of the Google Tensor gs101 SoC.
>>
>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
>> index 8bd88d5cbb11..72471ebe5734 100644
>> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
>> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
>> @@ -19,11 +19,13 @@ properties:
>>    compatible:
>>      oneOf:
>>        - items:
>> +          - const: google,gs101-uart
>>            - const: samsung,exynosautov9-uart
>>            - const: samsung,exynos850-uart
>>        - enum:
>>            - apple,s5l-uart
>>            - axis,artpec8-uart
>> +          - google,gs101-uart
> 
> These shouldn't be needed, just declare the device as the same as what

We should have SoC specific compatibles so that any further quirks or
incompatibilities can be easily addressed. It's not only the IP itself
that can differ, it's also the integration of the IP into the final
product that could have an influence on the behavior.

Cheers,
ta

> the chip really is (i.e. a samsung uart), that way no .yaml or kernel
> driver changes are needed at all.
>
Peter Griffin Oct. 11, 2023, 9:22 a.m. UTC | #2
Hi Greg,

Thanks for your review feedback!

On Wed, 11 Oct 2023 at 08:48, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Oct 10, 2023 at 11:49:16PM +0100, Peter Griffin wrote:
> > Add dedicated google-gs101-uart compatible to the dt-schema for
> > representing uart of the Google Tensor gs101 SoC.
> >
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> > index 8bd88d5cbb11..72471ebe5734 100644
> > --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> > +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> > @@ -19,11 +19,13 @@ properties:
> >    compatible:
> >      oneOf:
> >        - items:
> > +          - const: google,gs101-uart
> >            - const: samsung,exynosautov9-uart
> >            - const: samsung,exynos850-uart
> >        - enum:
> >            - apple,s5l-uart
> >            - axis,artpec8-uart
> > +          - google,gs101-uart
>
> These shouldn't be needed, just declare the device as the same as what
> the chip really is (i.e. a samsung uart), that way no .yaml or kernel
> driver changes are needed at all.

What you describe is actually how I had it in the v1 submission, which is also
similar to what exynosautov9.dtsi is doing by re-using the
"samsung,exynos850-uart" compatible, and associated data in the driver.

However the review feedback in v1 from Krzysztof and Tudor was to add a
dedicated compatible for it. I guess I could have re-used the existing
EXYNOS850_SERIAL_DRV_DATA structure though rather than duplicating
that as well.

I'll let Krzysztof comment on why a dedicated compatible is required.

regards,

Peter
Arnd Bergmann Oct. 11, 2023, 9:30 a.m. UTC | #3
On Wed, Oct 11, 2023, at 10:57, Greg KH wrote:
> On Wed, Oct 11, 2023 at 09:49:07AM +0100, Tudor Ambarus wrote:
>> On 10/11/23 08:48, Greg KH wrote:
>> > On Tue, Oct 10, 2023 at 11:49:16PM +0100, Peter Griffin wrote:
>> >> Add dedicated google-gs101-uart compatible to the dt-schema for
>> >> representing uart of the Google Tensor gs101 SoC.
>> >>
>> >> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
>> >> ---
>> >>  Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >>
>> >>      oneOf:
>> >>        - items:
>> >> +          - const: google,gs101-uart
>> >>            - const: samsung,exynosautov9-uart
>> >>            - const: samsung,exynos850-uart
>> >>        - enum:
>> >>            - apple,s5l-uart
>> >>            - axis,artpec8-uart
>> >> +          - google,gs101-uart
>> > 
>> > These shouldn't be needed, just declare the device as the same as what
>> 
>> We should have SoC specific compatibles so that any further quirks or
>> incompatibilities can be easily addressed.
>
> "further" work on quirks or incompatibilities can be added when they are
> found and needed.  We don't add stuff for no good reason to the kernel.
>
>> It's not only the IP itself
>> that can differ, it's also the integration of the IP into the final
>> product that could have an influence on the behavior.
>
> This is for the Pixel 6, a device that is no longer even shipping.  The
> "final product" is long stable, so this should not be an issue.

The driver does have soc specific settings for each compatible
string, in this case it looks like it overrides the FIFO size
based on driver specific data and the order in which the
ports are probed [1]. I don't understand why the driver does
this, but my impression is that if we wanted to change it to no
longer rely on that data, we'd also need a new compatible
string.

Ideally, the actual compatible list in the DTB lists both the
specific implementation (google,gs101-uart) in order to allow
such hacks if needed, and a more generic string (e.g. 
"samsung,exynos850-uart" for an older device that is entirely
compatible) in order to not actually need driver changes.

      Arnd

[1] https://lore.kernel.org/linux-arm-kernel/20231010224928.2296997-17-peter.griffin@linaro.org/
Arnd Bergmann Oct. 11, 2023, 10:19 a.m. UTC | #4
On Wed, Oct 11, 2023, at 11:42, Greg Kroah-Hartman wrote:
> On Wed, Oct 11, 2023 at 11:30:25AM +0200, Arnd Bergmann wrote:
>> On Wed, Oct 11, 2023, at 10:57, Greg KH wrote:
>> >
>> >> It's not only the IP itself
>> >> that can differ, it's also the integration of the IP into the final
>> >> product that could have an influence on the behavior.
>> >
>> > This is for the Pixel 6, a device that is no longer even shipping.  The
>> > "final product" is long stable, so this should not be an issue.
>> 
>> The driver does have soc specific settings for each compatible
>> string, in this case it looks like it overrides the FIFO size
>> based on driver specific data and the order in which the
>> ports are probed [1]. I don't understand why the driver does
>> this, but my impression is that if we wanted to change it to no
>> longer rely on that data, we'd also need a new compatible
>> string.
>
> As I reviewed that patch already, it is just duplicating an existing
> quirk/device that the driver already supports, so there is no need for
> any "new device type" to be added to that driver, just use the existing
> hardware description in the dt and all should be fine.

The thing is, I suspect that the FIFO size override is actually
wrong for the exynos850 as well, and is almost certainly wrong
for both exynosautov9 and google-gs101:

- The driver overrides an exynos850 compatible uart to use a
  256 byte FIFO on whichever port is probed first, 64 byte
  on the next three ports, and the setting from DT on any
  later ones, falling back to 16 bytes if the DT does not set
  anything.

- exynos850 only actually has three of these ports, not
  four. It does not lists  FIFO size in the dts at all.

- exynosautov9 has a total of 11 ports, each of these
  compatible with both "samsung,exynosautov9-uart" as
  the specific value and "samsung,exynos850-uart" as
  the generic fallback. The DT lists a FIFO size of 256
  bytes for ports 0, 1, and 6, but lists FIFO size 64
  for each of the other ones.

- google-gs101 only lists a single uart in the dts,
  and sets it to a 256 byte FIFO.

- testla-fsd claims to be compatible with exynos4210,
  which also overrides the first two ports in probe
  order to 256 and 64 bytes respectively (like exynos850),
  but it only has two ports.

- artpec8 has a separate compatible string so it overrides
  all ports to 64 bytes.

I don't know why probe order would have anything to do
with this, so most likely these are all the same thing
and should just put a fixed FIFO size into the DT for
each port instance.

      Arnd
Peter Griffin Oct. 11, 2023, 11:55 a.m. UTC | #5
Hi Arnd,

Thanks for your feedback.

On Wed, 11 Oct 2023 at 11:19, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Oct 11, 2023, at 11:42, Greg Kroah-Hartman wrote:
> > On Wed, Oct 11, 2023 at 11:30:25AM +0200, Arnd Bergmann wrote:
> >> On Wed, Oct 11, 2023, at 10:57, Greg KH wrote:
> >> >
> >> >> It's not only the IP itself
> >> >> that can differ, it's also the integration of the IP into the final
> >> >> product that could have an influence on the behavior.
> >> >
> >> > This is for the Pixel 6, a device that is no longer even shipping.  The
> >> > "final product" is long stable, so this should not be an issue.
> >>
> >> The driver does have soc specific settings for each compatible
> >> string, in this case it looks like it overrides the FIFO size
> >> based on driver specific data and the order in which the
> >> ports are probed [1]. I don't understand why the driver does
> >> this, but my impression is that if we wanted to change it to no
> >> longer rely on that data, we'd also need a new compatible
> >> string.
> >
> > As I reviewed that patch already, it is just duplicating an existing
> > quirk/device that the driver already supports, so there is no need for
> > any "new device type" to be added to that driver, just use the existing
> > hardware description in the dt and all should be fine.
>
> The thing is, I suspect that the FIFO size override is actually
> wrong for the exynos850 as well, and is almost certainly wrong
> for both exynosautov9 and google-gs101:
>
> - The driver overrides an exynos850 compatible uart to use a
>   256 byte FIFO on whichever port is probed first, 64 byte
>   on the next three ports, and the setting from DT on any
>   later ones, falling back to 16 bytes if the DT does not set
>   anything.
>
> - exynos850 only actually has three of these ports, not
>   four. It does not lists  FIFO size in the dts at all.
>
> - exynosautov9 has a total of 11 ports, each of these
>   compatible with both "samsung,exynosautov9-uart" as
>   the specific value and "samsung,exynos850-uart" as
>   the generic fallback. The DT lists a FIFO size of 256
>   bytes for ports 0, 1, and 6, but lists FIFO size 64
>   for each of the other ones.
>
> - google-gs101 only lists a single uart in the dts,
>   and sets it to a 256 byte FIFO.
>
> - testla-fsd claims to be compatible with exynos4210,
>   which also overrides the first two ports in probe
>   order to 256 and 64 bytes respectively (like exynos850),
>   but it only has two ports.
>
> - artpec8 has a separate compatible string so it overrides
>   all ports to 64 bytes.
>
> I don't know why probe order would have anything to do
> with this, so most likely these are all the same thing
> and should just put a fixed FIFO size into the DT for
> each port instance.

I agree, I just looked again at gs101 and in total we can have
19 uarts on this SoC. 3 of them are 256byte rx/tx fifo and the
other 16 have 64byte tx/rx fifo size, but this is a SoC design
choice and has nothing to do with probe order.

I will update in v3 to get fifo size from DT.

regards,

Peter.
Linus Walleij Oct. 11, 2023, 11:58 a.m. UTC | #6
On Wed, Oct 11, 2023 at 9:48 AM Greg KH <gregkh@linuxfoundation.org> wrote:

> >        - enum:
> >            - apple,s5l-uart
> >            - axis,artpec8-uart
> > +          - google,gs101-uart
>
> These shouldn't be needed, just declare the device as the same as what
> the chip really is (i.e. a samsung uart), that way no .yaml or kernel
> driver changes are needed at all.

We strive to have these as unique as possible, as it is a hardware
description. It is fine to write drivers in Linux or any other OS just
being aware of a "courser" idea of what UART this is, in this case
would have looked something like this:

    compatible = "google,gs101-uart", "samsung-uart";

And the driver would be able to match to just the latter string
(these are listed in "particularity order").

BUT! The binding authors chose not to go that path, instead they
have one unique compatible string per hardware/integration version,
essentially per-SoC. So in this case it is just:

    compatible = "google,gs101-uart";

It is kind of impossible to fix now as well, because these bindings
are already deployed. So they are like a BIOS: written in stone.

It is possible to add dual compatibles for this *and following*
variants, but I don't know how Krzysztof feels about that, and as
others point out, probably knowledge of the exact SoC is
necessary.

Yours,
Linus Walleij
Krzysztof Kozlowski Oct. 11, 2023, 12:07 p.m. UTC | #7
On 11/10/2023 10:57, Greg KH wrote:
> On Wed, Oct 11, 2023 at 09:49:07AM +0100, Tudor Ambarus wrote:
>> Hi, Greg,
>>
>> On 10/11/23 08:48, Greg KH wrote:
>>> On Tue, Oct 10, 2023 at 11:49:16PM +0100, Peter Griffin wrote:
>>>> Add dedicated google-gs101-uart compatible to the dt-schema for
>>>> representing uart of the Google Tensor gs101 SoC.
>>>>
>>>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
>>>> ---
>>>>  Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
>>>> index 8bd88d5cbb11..72471ebe5734 100644
>>>> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
>>>> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
>>>> @@ -19,11 +19,13 @@ properties:
>>>>    compatible:
>>>>      oneOf:
>>>>        - items:
>>>> +          - const: google,gs101-uart
>>>>            - const: samsung,exynosautov9-uart
>>>>            - const: samsung,exynos850-uart
>>>>        - enum:
>>>>            - apple,s5l-uart
>>>>            - axis,artpec8-uart
>>>> +          - google,gs101-uart
>>>
>>> These shouldn't be needed, just declare the device as the same as what
>>
>> We should have SoC specific compatibles so that any further quirks or
>> incompatibilities can be easily addressed.
> 
> "further" work on quirks or incompatibilities can be added when they are
> found and needed.  We don't add stuff for no good reason to the kernel.

With a Devicetree bindings maintainer hat:
We expect the device-specific compatible in all bindings, followed by
fallback. The fallback is used by the driver, the device-specific for
any future needs.

This is the practice we follow everywhere and recommend everywhere since
some time. It is also documented here:

https://elixir.bootlin.com/linux/v6.6-rc5/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

Best regards,
Krzysztof
Krzysztof Kozlowski Oct. 11, 2023, 12:09 p.m. UTC | #8
On 11/10/2023 00:49, Peter Griffin wrote:
> Add dedicated google-gs101-uart compatible to the dt-schema for
> representing uart of the Google Tensor gs101 SoC.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> index 8bd88d5cbb11..72471ebe5734 100644
> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> @@ -19,11 +19,13 @@ properties:
>    compatible:
>      oneOf:
>        - items:
> +          - const: google,gs101-uart

You just broke existing users.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).



Best regards,
Krzysztof
Peter Griffin Oct. 11, 2023, 1:27 p.m. UTC | #9
Hi Krzysztof,

Thanks for your review.

On Wed, 11 Oct 2023 at 13:09, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/10/2023 00:49, Peter Griffin wrote:
> > Add dedicated google-gs101-uart compatible to the dt-schema for
> > representing uart of the Google Tensor gs101 SoC.
> >
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> > index 8bd88d5cbb11..72471ebe5734 100644
> > --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> > +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
> > @@ -19,11 +19,13 @@ properties:
> >    compatible:
> >      oneOf:
> >        - items:
> > +          - const: google,gs101-uart
>
> You just broke existing users.
>
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
>

Will fix in v3

fyi I've been running with

make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-
dt_binding_check DT_SCHEMA_FILES=google
make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-
dt_binding_check DT_SCHEMA_FILES=samsung
make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-  CHECK_DTBS=y
W=1 google/gs101-oriole.dtb

But clearly that wasn't enough to catch this.  `make dtbs_check W=1`
takes a long time
and gives so much output. I suppose adding a few of the other exynos
based boards should
still be fairly quick and hopefully catch things like this. For example adding

make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y
W=1 exynos/exynos850-e850-96.dtb
make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y
W=1 exynos/exynos5433-tm2.dtb
make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y
W=1 exynos/exynosautov9-sadk.dtb

regards,

Peter.
Krzysztof Kozlowski Oct. 11, 2023, 1:32 p.m. UTC | #10
On 11/10/2023 15:27, Peter Griffin wrote:
> Hi Krzysztof,
> 
> Thanks for your review.
> 
> On Wed, 11 Oct 2023 at 13:09, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 11/10/2023 00:49, Peter Griffin wrote:
>>> Add dedicated google-gs101-uart compatible to the dt-schema for
>>> representing uart of the Google Tensor gs101 SoC.
>>>
>>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
>>> ---
>>>  Documentation/devicetree/bindings/serial/samsung_uart.yaml | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
>>> index 8bd88d5cbb11..72471ebe5734 100644
>>> --- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
>>> +++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
>>> @@ -19,11 +19,13 @@ properties:
>>>    compatible:
>>>      oneOf:
>>>        - items:
>>> +          - const: google,gs101-uart
>>
>> You just broke existing users.
>>
>> It does not look like you tested the DTS against bindings. Please run
>> `make dtbs_check W=1` (see
>> Documentation/devicetree/bindings/writing-schema.rst or
>> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
>> for instructions).
>>
> 
> Will fix in v3
> 
> fyi I've been running with
> 
> make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-
> dt_binding_check DT_SCHEMA_FILES=google
> make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-
> dt_binding_check DT_SCHEMA_FILES=samsung
> make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu-  CHECK_DTBS=y
> W=1 google/gs101-oriole.dtb
> 
> But clearly that wasn't enough to catch this.  

None of the commands above test existing DTS...

>`make dtbs_check W=1`
> takes a long time
> and gives so much output. I suppose adding a few of the other exynos
> based boards should
> still be fairly quick and hopefully catch things like this. For example adding
> 
> make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y
> W=1 exynos/exynos850-e850-96.dtb
> make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y
> W=1 exynos/exynos5433-tm2.dtb
> make -j$js ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CHECK_DTBS=y
> W=1 exynos/exynosautov9-sadk.dtb
> 
> regards,
> 
> Peter.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/serial/samsung_uart.yaml b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
index 8bd88d5cbb11..72471ebe5734 100644
--- a/Documentation/devicetree/bindings/serial/samsung_uart.yaml
+++ b/Documentation/devicetree/bindings/serial/samsung_uart.yaml
@@ -19,11 +19,13 @@  properties:
   compatible:
     oneOf:
       - items:
+          - const: google,gs101-uart
           - const: samsung,exynosautov9-uart
           - const: samsung,exynos850-uart
       - enum:
           - apple,s5l-uart
           - axis,artpec8-uart
+          - google,gs101-uart
           - samsung,s3c2410-uart
           - samsung,s3c2412-uart
           - samsung,s3c2440-uart