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

[Paweł Anikiel]

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
arch/arm/boot/dts/socfpga_cyclone5_mcvevk.dts
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).

>
> It's the same for every other SoM. Neither Google nor Enclustra are
> special...
>
> >
> >>
> >>> +                  "altr,socfpga-arria10", "altr,socfpga";
> >>> +
> >>> +     aliases {
> >>> +             serial0 = &uart0;
> >>> +             i2c0 = &i2c0;
> >>> +             i2c1 = &i2c1;
> >>> +     };
> >>> +};
> >>> +
> >>> +&gmac0 {
> >>> +     status = "okay";
> >>> +};
> >>> +
> >>> +&gpio0 {
> >>> +     status = "okay";
> >>> +};
> >>> +
> >>> +&gpio1 {
> >>> +     status = "okay";
> >>> +};
> >>> +
> >>> +&gpio2 {
> >>> +     status = "okay";
> >>> +};
> >>> +
> >>> +&i2c0 {
> >>> +     status = "okay";
> >>> +
> >>> +     ssm2603: ssm2603@1a {
> >>
> >> Generic node names.
> >
> > Dumb question: what does this mean?
> >
> > Are you saying the name is too generic? As someone reading the
> > schematics this would be immediately clear what chip it's talking
> > about.
>
> 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 {

Regards,
Paweł