On 31/05/2022 03:20, Alexandru M Stan wrote:
> Hello Krzysztof
>
> On Mon, May 30, 2022 at 11:56 AM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 30/05/2022 15:08, Paweł Anikiel wrote:
>>> Add devicetree for the Google Chameleon v3 board.
>>>
>>> Signed-off-by: Paweł Anikiel <[email protected]>
>>> Signed-off-by: Alexandru M Stan <[email protected]>
>>
>> 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.
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.
>
>>
>>> + compatible = "adi,ssm2603";
>>> + reg = <0x1a>;
>>> + };
>>> +};
>>> +
>>> +&i2c1 {
>>> + status = "okay";
>>> +
>>> + u80: u80@21 {
>>> + compatible = "nxp,pca9535";
>>
>> Generic node names.
>
> FWIW: Schematic is full of these pca9535 io expanders, only one (U80)
> is visible to linux on an I2C bus.
Best regards,
Krzysztof
On Tue, May 31, 2022 at 11:11 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 31/05/2022 03:20, Alexandru M Stan wrote:
> > Hello Krzysztof
> >
> > On Mon, May 30, 2022 at 11:56 AM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 30/05/2022 15:08, Paweł Anikiel wrote:
> >>> Add devicetree for the Google Chameleon v3 board.
> >>>
> >>> Signed-off-by: Paweł Anikiel <[email protected]>
> >>> Signed-off-by: Alexandru M Stan <[email protected]>
> >>
> >> 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ł