Re: [PATCH 2/3] dts: socfpga: Add Google Chameleon v3 devicetree

[Paweł Anikiel]

On Tue, May 31, 2022 at 5:36 PM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 31/05/2022 16:47, Paweł Anikiel wrote:
> > On Tue, May 31, 2022 at 11:11 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> >>
> >> On 31/05/2022 03:20, Alexandru M Stan wrote:
> >>> Hello Krzysztof
> >>>
> >>> On Mon, May 30, 2022 at 11:56 AM Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> >>>>
> >>>> On 30/05/2022 15:08, Paweł Anikiel wrote:
> >>>>> Add devicetree for the Google Chameleon v3 board.
> >>>>>
> >>>>> Signed-off-by: Paweł Anikiel <pan@xxxxxxxxxxxx>
> >>>>> Signed-off-by: Alexandru M Stan <amstan@xxxxxxxxxxxx>
> >>>>
> >>>> Your SoB chain looks odd. Who did what here?
> >>>
> >>> Sorry about this.
> >>>
> >>> It was mainly Pawel but I did some small changes at some point before
> >>> it landed in our tree (particularly the GPIOs).
> >>
> >> Then usually Paweł should be the owner of the patch, not you.
> >> Alternatively it could be also co-developed.
> >>
> >>>
> >>>>
> >>>>> ---
> >>>>>  arch/arm/boot/dts/Makefile                    |  1 +
> >>>>>  .../boot/dts/socfpga_arria10_chameleonv3.dts  | 90 +++++++++++++++++++
> >>>>>  2 files changed, 91 insertions(+)
> >>>>>  create mode 100644 arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts
> >>>>>
> >>>>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> >>>>> index 023c8b4ba45c..9417106d3289 100644
> >>>>> --- a/arch/arm/boot/dts/Makefile
> >>>>> +++ b/arch/arm/boot/dts/Makefile
> >>>>> @@ -1146,6 +1146,7 @@ dtb-$(CONFIG_ARCH_S5PV210) += \
> >>>>>       s5pv210-torbreck.dtb
> >>>>>  dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += \
> >>>>>       socfpga_arria5_socdk.dtb \
> >>>>> +     socfpga_arria10_chameleonv3.dtb \
> >>>>>       socfpga_arria10_socdk_nand.dtb \
> >>>>>       socfpga_arria10_socdk_qspi.dtb \
> >>>>>       socfpga_arria10_socdk_sdmmc.dtb \
> >>>>> diff --git a/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts b/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts
> >>>>> new file mode 100644
> >>>>> index 000000000000..988cc445438e
> >>>>> --- /dev/null
> >>>>> +++ b/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts
> >>>>> @@ -0,0 +1,90 @@
> >>>>> +// SPDX-License-Identifier: GPL-2.0
> >>>>> +/*
> >>>>> + * Copyright 2022 Google LLC
> >>>>> + */
> >>>>> +/dts-v1/;
> >>>>> +#include "socfpga_arria10_mercury_aa1.dtsi"
> >>>>> +
> >>>>> +/ {
> >>>>> +     model = "Google Chameleon V3";
> >>>>> +     compatible = "google,chameleon-v3",
> >>>>
> >>>> You miss here enclustra compatible.
> >>>
> >>> Does this make sense? I don't expect this device tree to boot/work on
> >>> an enclustra motherboard. It's only really compatible with a
> >>> "chameleon-v3".
> >>
> >> You also do not expect it to boot on altr,socfpga, do you?
> >>
> >> If I understood correctly, this board has physically Mercury AA1 SoM, so
> >> that compatible should be there.
> >
> > Yes, you understood correctly.
> > I looked at a similar device - the Cyclone V MCV (SoM) and the MCVEVK
> > (base board):
> > arch/arm/boot/dts/socfpga_cyclone5_mcv.dtsi
>
> This one is clearly incorrect, so using it as example is wrong.
>
> > arch/arm/boot/dts/socfpga_cyclone5_mcvevk.dts
>
> Since it is based on wrong MCV, then no wonder it's the same.
>
> > And there is no denx,mcv compatible anywhere, only denx,mcvevk.
> > Also, devicetree bindings documentation lists denx,mcvevk under
> > "Cyclone 5 boards", and, unless you consider the MCV to be a board,
> > there isn't a good place to put denx,mcv (same story with mercury+
> > aa1/chameleon).
>
> socfpga are not the best example... upstreaming looks incomplete or even
> incorrect, like this case of Enclustra SOM. Much better examples are
> FSL-based SoMs. Although some of them are also not in the best shape.
>
> Still someone might prefer to skip SoM compatible arguing that it cannot
> be a separate final product. Sure, but also SoC cannot be a separate
> product. Having SoM compatible allows to match against it and find
> common hardware parts.

Ok, I understand. Thanks for the explanation, I will add the SoM compatible.

>
> In any case you want to remove here parts of bindings (so affect ABI),
> to which I do not agree.
>
> >> Let me clarify - please use generic node names, as asked by Devicetree
> >> specification (2.2.1. Node Name Requirements). There is also list of
> >> some examples in the spec, but you can use some other generic node name.
> >>
> >> Several bindings also require it.
> >
> > Do you mean something like this?
> > ssm2603: audio-codec@1a {
> > u80: gpio@21 {
>
> Yes.
>
>
> Best regards,
> Krzysztof