[PATCH 0/3] Add Chameleon v3 devicetree

[PATCH 0/3] Add Chameleon v3 devicetree

[Paweł Anikiel]

The Google Chameleon v3 is a board made for testing both video and audio 
interfaces of external devices. It acts as a base board for the
Mercury+ AA1 module.

socfpga_arria10_mercury_aa1.dtsi and socfpga_arria10_chameleonv3.dts
have also been sent to u-boot:
https://lists.denx.de/pipermail/u-boot/2022-May/485107.html
https://lists.denx.de/pipermail/u-boot/2022-May/485111.html

Paweł Anikiel (3):
  dts: socfpga: Change Mercury+ AA1 devicetree to header
  dts: socfpga: Add Google Chameleon v3 devicetree
  dt-bindings: altera: Update Arria 10 boards

 .../devicetree/bindings/arm/altera.yaml       |  2 +-
 arch/arm/boot/dts/Makefile                    |  2 +-
 .../boot/dts/socfpga_arria10_chameleonv3.dts  | 90 +++++++++++++++++++
 ...1.dts => socfpga_arria10_mercury_aa1.dtsi} | 68 +++-----------
 4 files changed, 106 insertions(+), 56 deletions(-)
 create mode 100644 arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts
 rename arch/arm/boot/dts/{socfpga_arria10_mercury_aa1.dts => socfpga_arria10_mercury_aa1.dtsi} (58%)

-- 
2.36.1.124.g0e6072fb45-goog

[PATCH 1/3] dts: socfpga: Change Mercury+ AA1 devicetree to header

[Paweł Anikiel]

The Mercury+ AA1 is not a standalone board, rather it's a module
with an Arria 10 SoC and some peripherals on it. Remove everything that
is not strictly related to the module.

Signed-off-by: Paweł Anikiel <pan@semihalf.com>
---
 arch/arm/boot/dts/Makefile                    |  1 -
 ...1.dts => socfpga_arria10_mercury_aa1.dtsi} | 68 ++++---------------
 2 files changed, 14 insertions(+), 55 deletions(-)
 rename arch/arm/boot/dts/{socfpga_arria10_mercury_aa1.dts => socfpga_arria10_mercury_aa1.dtsi} (58%)

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index edfbedaa6168..023c8b4ba45c 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -1146,7 +1146,6 @@ dtb-$(CONFIG_ARCH_S5PV210) += \
 	s5pv210-torbreck.dtb
 dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += \
 	socfpga_arria5_socdk.dtb \
-	socfpga_arria10_mercury_aa1.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_mercury_aa1.dts b/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
similarity index 58%
rename from arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dts
rename to arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
index a75c059b6727..fee1fc39bb2b 100644
--- a/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dts
+++ b/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
@@ -1,57 +1,38 @@
 // SPDX-License-Identifier: GPL-2.0
-/dts-v1/;
-
+/*
+ * Copyright 2022 Google LLC
+ */
 #include "socfpga_arria10.dtsi"
 
 / {
-
-	model = "Enclustra Mercury AA1";
-	compatible = "enclustra,mercury-aa1", "altr,socfpga-arria10", "altr,socfpga";
-
 	aliases {
 		ethernet0 = &gmac0;
 		serial1 = &uart1;
-		i2c0 = &i2c0;
-		i2c1 = &i2c1;
-	};
-
-	memory@0 {
-		name = "memory";
-		device_type = "memory";
-		reg = <0x0 0x80000000>; /* 2GB */
 	};
 
 	chosen {
 		stdout-path = "serial1:115200n8";
 	};
-};
 
-&eccmgr {
-	sdmmca-ecc@ff8c2c00 {
-		compatible = "altr,socfpga-sdmmc-ecc";
-		reg = <0xff8c2c00 0x400>;
-		altr,ecc-parent = <&mmc>;
-		interrupts = <15 IRQ_TYPE_LEVEL_HIGH>,
-			     <47 IRQ_TYPE_LEVEL_HIGH>,
-			     <16 IRQ_TYPE_LEVEL_HIGH>,
-			     <48 IRQ_TYPE_LEVEL_HIGH>;
+	memory@0 {
+		name = "memory";
+		device_type = "memory";
+		reg = <0x0 0x80000000>; /* 2GB */
 	};
 };
 
 &gmac0 {
 	phy-mode = "rgmii";
-	phy-addr = <0xffffffff>; /* probe for phy addr */
+	phy-handle = <&phy3>;
 
 	max-frame-size = <3800>;
-	status = "okay";
-
-	phy-handle = <&phy3>;
 
 	mdio {
 		#address-cells = <1>;
 		#size-cells = <0>;
 		compatible = "snps,dwmac-mdio";
 		phy3: ethernet-phy@3 {
+			reg = <3>;
 			txd0-skew-ps = <0>; /* -420ps */
 			txd1-skew-ps = <0>; /* -420ps */
 			txd2-skew-ps = <0>; /* -420ps */
@@ -64,35 +45,23 @@ phy3: ethernet-phy@3 {
 			txc-skew-ps = <1860>; /* 960ps */
 			rxdv-skew-ps = <420>; /* 0ps */
 			rxc-skew-ps = <1680>; /* 780ps */
-			reg = <3>;
 		};
 	};
 };
 
-&gpio0 {
-	status = "okay";
-};
-
-&gpio1 {
-	status = "okay";
-};
-
-&gpio2 {
-	status = "okay";
-};
-
 &i2c1 {
-	status = "okay";
+	atsha204a: atsha204a@64 {
+		compatible = "atmel,atsha204a";
+		reg = <0x64>;
+	};
+
 	isl12022: isl12022@6f {
-		status = "okay";
 		compatible = "isil,isl12022";
 		reg = <0x6f>;
 	};
 };
 
-/* Following mappings are taken from arria10 socdk dts */
 &mmc {
-	status = "okay";
 	cap-sd-highspeed;
 	broken-cd;
 	bus-width = <4>;
@@ -101,12 +70,3 @@ &mmc {
 &osc1 {
 	clock-frequency = <33330000>;
 };
-
-&uart1 {
-	status = "okay";
-};
-
-&usb0 {
-	status = "okay";
-	dr_mode = "host";
-};
-- 
2.36.1.124.g0e6072fb45-goog

Re: [PATCH 1/3] dts: socfpga: Change Mercury+ AA1 devicetree to header

[Krzysztof Kozlowski]

On 30/05/2022 15:08, Paweł Anikiel wrote:
> The Mercury+ AA1 is not a standalone board, rather it's a module
> with an Arria 10 SoC and some peripherals on it. Remove everything that
> is not strictly related to the module.

Subject has several issues:
1. Use prefix matching subsystem (git log --oneline)
2. You are not changing here anything to header. Header files have
different format and end with .h. This is a DTSI file.

> 
> Signed-off-by: Paweł Anikiel <pan@semihalf.com>

Thank you for your patch. There is something to discuss/improve.

> ---
>  arch/arm/boot/dts/Makefile                    |  1 -
>  ...1.dts => socfpga_arria10_mercury_aa1.dtsi} | 68 ++++---------------
>  2 files changed, 14 insertions(+), 55 deletions(-)
>  rename arch/arm/boot/dts/{socfpga_arria10_mercury_aa1.dts => socfpga_arria10_mercury_aa1.dtsi} (58%)
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index edfbedaa6168..023c8b4ba45c 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1146,7 +1146,6 @@ dtb-$(CONFIG_ARCH_S5PV210) += \
>  	s5pv210-torbreck.dtb
>  dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += \
>  	socfpga_arria5_socdk.dtb \
> -	socfpga_arria10_mercury_aa1.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_mercury_aa1.dts b/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
> similarity index 58%
> rename from arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dts
> rename to arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
> index a75c059b6727..fee1fc39bb2b 100644
> --- a/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dts
> +++ b/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
> @@ -1,57 +1,38 @@
>  // SPDX-License-Identifier: GPL-2.0
> -/dts-v1/;
> -
> +/*
> + * Copyright 2022 Google LLC
> + */

How is this related to the patch?

>  #include "socfpga_arria10.dtsi"
>  
>  / {
> -
> -	model = "Enclustra Mercury AA1";
> -	compatible = "enclustra,mercury-aa1", "altr,socfpga-arria10", "altr,socfpga";
> -
>  	aliases {
>  		ethernet0 = &gmac0;
>  		serial1 = &uart1;
> -		i2c0 = &i2c0;
> -		i2c1 = &i2c1;
> -	};
> -
> -	memory@0 {
> -		name = "memory";
> -		device_type = "memory";
> -		reg = <0x0 0x80000000>; /* 2GB */
>  	};
>  
>  	chosen {
>  		stdout-path = "serial1:115200n8";
>  	};
> -};
>  
> -&eccmgr {
> -	sdmmca-ecc@ff8c2c00 {
> -		compatible = "altr,socfpga-sdmmc-ecc";
> -		reg = <0xff8c2c00 0x400>;
> -		altr,ecc-parent = <&mmc>;
> -		interrupts = <15 IRQ_TYPE_LEVEL_HIGH>,
> -			     <47 IRQ_TYPE_LEVEL_HIGH>,
> -			     <16 IRQ_TYPE_LEVEL_HIGH>,
> -			     <48 IRQ_TYPE_LEVEL_HIGH>;
> +	memory@0 {
> +		name = "memory";
> +		device_type = "memory";
> +		reg = <0x0 0x80000000>; /* 2GB */
>  	};
>  };
>  
>  &gmac0 {
>  	phy-mode = "rgmii";
> -	phy-addr = <0xffffffff>; /* probe for phy addr */
> +	phy-handle = <&phy3>;
>  
>  	max-frame-size = <3800>;
> -	status = "okay";
> -
> -	phy-handle = <&phy3>;
>  
>  	mdio {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  		compatible = "snps,dwmac-mdio";
>  		phy3: ethernet-phy@3 {
> +			reg = <3>;
>  			txd0-skew-ps = <0>; /* -420ps */
>  			txd1-skew-ps = <0>; /* -420ps */
>  			txd2-skew-ps = <0>; /* -420ps */
> @@ -64,35 +45,23 @@ phy3: ethernet-phy@3 {
>  			txc-skew-ps = <1860>; /* 960ps */
>  			rxdv-skew-ps = <420>; /* 0ps */
>  			rxc-skew-ps = <1680>; /* 780ps */
> -			reg = <3>;

This and few other changes (like memory) are not related to the commit.
Do not mix different cleanups together.

>  		};
>  	};
>  };
>  
> -&gpio0 {
> -	status = "okay";
> -};
> -
> -&gpio1 {
> -	status = "okay";
> -};
> -
> -&gpio2 {
> -	status = "okay";
> -};

Why removing all these? Aren't they available on the SoM? The same
question applies to several other pieces, for example UART and USB -
aren't these part of SoM?

> -
>  &i2c1 {
> -	status = "okay";
> +	atsha204a: atsha204a@64 {

How is this change related at all to what you described in commit? There
was no atsha204a before.

> +		compatible = "atmel,atsha204a";
> +		reg = <0x64>;
> +	};
> +
>  	isl12022: isl12022@6f {
> -		status = "okay";
>  		compatible = "isil,isl12022";
>  		reg = <0x6f>;
>  	};
>  };
>  

Best regards,
Krzysztof

Re: [PATCH 1/3] dts: socfpga: Change Mercury+ AA1 devicetree to header

[Paweł Anikiel]

Thank you for the review, I'm thinking of splitting this commit into
several smaller ones:
- remove all status = "okay" things and i2c aliases
- remove sdmmc-ecc node (it's a part of the Arria 10 SoC, not the mercury)
- add atsha204a node
- add copyright header
- style fixes (phy reg, memory)
What do you think?

Please see my other comments below.

On Mon, May 30, 2022 at 8:55 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 30/05/2022 15:08, Paweł Anikiel wrote:
> > The Mercury+ AA1 is not a standalone board, rather it's a module
> > with an Arria 10 SoC and some peripherals on it. Remove everything that
> > is not strictly related to the module.
>
> Subject has several issues:
> 1. Use prefix matching subsystem (git log --oneline)

Just to make sure, are you referring to the ARM: prefix?

> 2. You are not changing here anything to header. Header files have
> different format and end with .h. This is a DTSI file.

Yes, I meant dtsi.

>
> >
> > Signed-off-by: Paweł Anikiel <pan@semihalf.com>
>
> Thank you for your patch. There is something to discuss/improve.
>
> > ---
> >  arch/arm/boot/dts/Makefile                    |  1 -
> >  ...1.dts => socfpga_arria10_mercury_aa1.dtsi} | 68 ++++---------------
> >  2 files changed, 14 insertions(+), 55 deletions(-)
> >  rename arch/arm/boot/dts/{socfpga_arria10_mercury_aa1.dts => socfpga_arria10_mercury_aa1.dtsi} (58%)
> >
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index edfbedaa6168..023c8b4ba45c 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -1146,7 +1146,6 @@ dtb-$(CONFIG_ARCH_S5PV210) += \
> >       s5pv210-torbreck.dtb
> >  dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += \
> >       socfpga_arria5_socdk.dtb \
> > -     socfpga_arria10_mercury_aa1.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_mercury_aa1.dts b/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
> > similarity index 58%
> > rename from arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dts
> > rename to arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
> > index a75c059b6727..fee1fc39bb2b 100644
> > --- a/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dts
> > +++ b/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
> > @@ -1,57 +1,38 @@
> >  // SPDX-License-Identifier: GPL-2.0
> > -/dts-v1/;
> > -
> > +/*
> > + * Copyright 2022 Google LLC
> > + */
>
> How is this related to the patch?

I'll move this change to a seperate commit.

>
> >  #include "socfpga_arria10.dtsi"
> >
> >  / {
> > -
> > -     model = "Enclustra Mercury AA1";
> > -     compatible = "enclustra,mercury-aa1", "altr,socfpga-arria10", "altr,socfpga";
> > -
> >       aliases {
> >               ethernet0 = &gmac0;
> >               serial1 = &uart1;
> > -             i2c0 = &i2c0;
> > -             i2c1 = &i2c1;
> > -     };
> > -
> > -     memory@0 {
> > -             name = "memory";
> > -             device_type = "memory";
> > -             reg = <0x0 0x80000000>; /* 2GB */
> >       };
> >
> >       chosen {
> >               stdout-path = "serial1:115200n8";
> >       };
> > -};
> >
> > -&eccmgr {
> > -     sdmmca-ecc@ff8c2c00 {
> > -             compatible = "altr,socfpga-sdmmc-ecc";
> > -             reg = <0xff8c2c00 0x400>;
> > -             altr,ecc-parent = <&mmc>;
> > -             interrupts = <15 IRQ_TYPE_LEVEL_HIGH>,
> > -                          <47 IRQ_TYPE_LEVEL_HIGH>,
> > -                          <16 IRQ_TYPE_LEVEL_HIGH>,
> > -                          <48 IRQ_TYPE_LEVEL_HIGH>;
> > +     memory@0 {
> > +             name = "memory";
> > +             device_type = "memory";
> > +             reg = <0x0 0x80000000>; /* 2GB */
> >       };
> >  };
> >
> >  &gmac0 {
> >       phy-mode = "rgmii";
> > -     phy-addr = <0xffffffff>; /* probe for phy addr */
> > +     phy-handle = <&phy3>;
> >
> >       max-frame-size = <3800>;
> > -     status = "okay";
> > -
> > -     phy-handle = <&phy3>;
> >
> >       mdio {
> >               #address-cells = <1>;
> >               #size-cells = <0>;
> >               compatible = "snps,dwmac-mdio";
> >               phy3: ethernet-phy@3 {
> > +                     reg = <3>;
> >                       txd0-skew-ps = <0>; /* -420ps */
> >                       txd1-skew-ps = <0>; /* -420ps */
> >                       txd2-skew-ps = <0>; /* -420ps */
> > @@ -64,35 +45,23 @@ phy3: ethernet-phy@3 {
> >                       txc-skew-ps = <1860>; /* 960ps */
> >                       rxdv-skew-ps = <420>; /* 0ps */
> >                       rxc-skew-ps = <1680>; /* 780ps */
> > -                     reg = <3>;
>
> This and few other changes (like memory) are not related to the commit.
> Do not mix different cleanups together.

l'll put the cleanup changes into a seperate commit.

>
> >               };
> >       };
> >  };
> >
> > -&gpio0 {
> > -     status = "okay";
> > -};
> > -
> > -&gpio1 {
> > -     status = "okay";
> > -};
> > -
> > -&gpio2 {
> > -     status = "okay";
> > -};
>
> Why removing all these? Aren't they available on the SoM? The same
> question applies to several other pieces, for example UART and USB -
> aren't these part of SoM?

I'm removing them from here, but adding them to socfpga_arria10_chameleonv3.dts.
I think that enabling them should be done in the base board's dts, as
the connections
go to the base board. The Chameleon v3 has a USB port, but a different
base board might
not have one.

>
> > -
> >  &i2c1 {
> > -     status = "okay";
> > +     atsha204a: atsha204a@64 {
>
> How is this change related at all to what you described in commit? There
> was no atsha204a before.

As previously mentioned, I'll move this change to a seperate commit.

>
> > +             compatible = "atmel,atsha204a";
> > +             reg = <0x64>;
> > +     };
> > +
> >       isl12022: isl12022@6f {
> > -             status = "okay";
> >               compatible = "isil,isl12022";
> >               reg = <0x6f>;
> >       };
> >  };
> >
>
> Best regards,
> Krzysztof


Regards,
Paweł

Re: [PATCH 1/3] dts: socfpga: Change Mercury+ AA1 devicetree to header

[Krzysztof Kozlowski]

On 31/05/2022 14:43, Paweł Anikiel wrote:
> Thank you for the review, I'm thinking of splitting this commit into
> several smaller ones:
> - remove all status = "okay" things and i2c aliases

This might have sense, might not. Depends whether the interface is
actively used in the SoM or not. If the latter - the interface is only
exposed to the carrier board - then this seems reasonable choice.

> - remove sdmmc-ecc node (it's a part of the Arria 10 SoC, not the mercury)

Sounds good, but maybe not remove but move?

> - add atsha204a node
> - add copyright header

These can come together. Copyright by itself is not a meaningful change,
but usually addon to actual copyrighted work.

> - style fixes (phy reg, memory)

Sure.

> What do you think?
> 
> Please see my other comments below.
> 
> On Mon, May 30, 2022 at 8:55 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 30/05/2022 15:08, Paweł Anikiel wrote:
>>> The Mercury+ AA1 is not a standalone board, rather it's a module
>>> with an Arria 10 SoC and some peripherals on it. Remove everything that
>>> is not strictly related to the module.
>>
>> Subject has several issues:
>> 1. Use prefix matching subsystem (git log --oneline)
> 
> Just to make sure, are you referring to the ARM: prefix?

Yes, ARM: dts: socfpga:

>> 2. You are not changing here anything to header. Header files have
>> different format and end with .h. This is a DTSI file.
> 
> Yes, I meant dtsi.
> 
>>
>>>
>>> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> ---
>>>  arch/arm/boot/dts/Makefile                    |  1 -
>>>  ...1.dts => socfpga_arria10_mercury_aa1.dtsi} | 68 ++++---------------
>>>  2 files changed, 14 insertions(+), 55 deletions(-)
>>>  rename arch/arm/boot/dts/{socfpga_arria10_mercury_aa1.dts => socfpga_arria10_mercury_aa1.dtsi} (58%)
>>>
>>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>>> index edfbedaa6168..023c8b4ba45c 100644
>>> --- a/arch/arm/boot/dts/Makefile
>>> +++ b/arch/arm/boot/dts/Makefile
>>> @@ -1146,7 +1146,6 @@ dtb-$(CONFIG_ARCH_S5PV210) += \
>>>       s5pv210-torbreck.dtb
>>>  dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += \
>>>       socfpga_arria5_socdk.dtb \
>>> -     socfpga_arria10_mercury_aa1.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_mercury_aa1.dts b/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
>>> similarity index 58%
>>> rename from arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dts
>>> rename to arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
>>> index a75c059b6727..fee1fc39bb2b 100644
>>> --- a/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dts
>>> +++ b/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
>>> @@ -1,57 +1,38 @@
>>>  // SPDX-License-Identifier: GPL-2.0
>>> -/dts-v1/;
>>> -
>>> +/*
>>> + * Copyright 2022 Google LLC
>>> + */
>>
>> How is this related to the patch?
> 
> I'll move this change to a seperate commit.
> 
>>
>>>  #include "socfpga_arria10.dtsi"
>>>
>>>  / {
>>> -
>>> -     model = "Enclustra Mercury AA1";
>>> -     compatible = "enclustra,mercury-aa1", "altr,socfpga-arria10", "altr,socfpga";
>>> -
>>>       aliases {
>>>               ethernet0 = &gmac0;
>>>               serial1 = &uart1;
>>> -             i2c0 = &i2c0;
>>> -             i2c1 = &i2c1;
>>> -     };
>>> -
>>> -     memory@0 {
>>> -             name = "memory";
>>> -             device_type = "memory";
>>> -             reg = <0x0 0x80000000>; /* 2GB */
>>>       };
>>>
>>>       chosen {
>>>               stdout-path = "serial1:115200n8";
>>>       };
>>> -};
>>>
>>> -&eccmgr {
>>> -     sdmmca-ecc@ff8c2c00 {
>>> -             compatible = "altr,socfpga-sdmmc-ecc";
>>> -             reg = <0xff8c2c00 0x400>;
>>> -             altr,ecc-parent = <&mmc>;
>>> -             interrupts = <15 IRQ_TYPE_LEVEL_HIGH>,
>>> -                          <47 IRQ_TYPE_LEVEL_HIGH>,
>>> -                          <16 IRQ_TYPE_LEVEL_HIGH>,
>>> -                          <48 IRQ_TYPE_LEVEL_HIGH>;
>>> +     memory@0 {
>>> +             name = "memory";
>>> +             device_type = "memory";
>>> +             reg = <0x0 0x80000000>; /* 2GB */
>>>       };
>>>  };
>>>
>>>  &gmac0 {
>>>       phy-mode = "rgmii";
>>> -     phy-addr = <0xffffffff>; /* probe for phy addr */
>>> +     phy-handle = <&phy3>;
>>>
>>>       max-frame-size = <3800>;
>>> -     status = "okay";
>>> -
>>> -     phy-handle = <&phy3>;
>>>
>>>       mdio {
>>>               #address-cells = <1>;
>>>               #size-cells = <0>;
>>>               compatible = "snps,dwmac-mdio";
>>>               phy3: ethernet-phy@3 {
>>> +                     reg = <3>;
>>>                       txd0-skew-ps = <0>; /* -420ps */
>>>                       txd1-skew-ps = <0>; /* -420ps */
>>>                       txd2-skew-ps = <0>; /* -420ps */
>>> @@ -64,35 +45,23 @@ phy3: ethernet-phy@3 {
>>>                       txc-skew-ps = <1860>; /* 960ps */
>>>                       rxdv-skew-ps = <420>; /* 0ps */
>>>                       rxc-skew-ps = <1680>; /* 780ps */
>>> -                     reg = <3>;
>>
>> This and few other changes (like memory) are not related to the commit.
>> Do not mix different cleanups together.
> 
> l'll put the cleanup changes into a seperate commit.
> 
>>
>>>               };
>>>       };
>>>  };
>>>
>>> -&gpio0 {
>>> -     status = "okay";
>>> -};
>>> -
>>> -&gpio1 {
>>> -     status = "okay";
>>> -};
>>> -
>>> -&gpio2 {
>>> -     status = "okay";
>>> -};
>>
>> Why removing all these? Aren't they available on the SoM? The same
>> question applies to several other pieces, for example UART and USB -
>> aren't these part of SoM?
> 
> I'm removing them from here, but adding them to socfpga_arria10_chameleonv3.dts.
> I think that enabling them should be done in the base board's dts, as
> the connections
> go to the base board. The Chameleon v3 has a USB port, but a different
> base board might
> not have one.

This sounds reasonable (unless such bus is still used internally in the
SoM).

Best regards,
Krzysztof

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

[Paweł Anikiel]

Add devicetree for the Google Chameleon v3 board.

Signed-off-by: Paweł Anikiel <pan@semihalf.com>
Signed-off-by: Alexandru M Stan <amstan@chromium.org>
---
 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",
+		     "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 {
+		compatible = "adi,ssm2603";
+		reg = <0x1a>;
+	};
+};
+
+&i2c1 {
+	status = "okay";
+
+	u80: u80@21 {
+		compatible = "nxp,pca9535";
+		reg = <0x21>;
+		gpio-controller;
+		#gpio-cells = <2>;
+
+		gpio-line-names =
+			"SOM_AUD_MUTE",
+			"DP1_OUT_CEC_EN",
+			"DP2_OUT_CEC_EN",
+			"DP1_SOM_PS8469_CAD",
+			"DPD_SOM_PS8469_CAD",
+			"DP_OUT_PWR_EN",
+			"STM32_RST_L",
+			"STM32_BOOT0",
+
+			"FPGA_PROT",
+			"STM32_FPGA_COMM0",
+			"TP119",
+			"TP120",
+			"TP121",
+			"TP122",
+			"TP123",
+			"TP124";
+	};
+};
+
+&mmc {
+	status = "okay";
+};
+
+&uart0 {
+	status = "okay";
+};
+
+&uart1 {
+	status = "okay";
+};
+
+&usb0 {
+	status = "okay";
+	dr_mode = "host";
+};
-- 
2.36.1.124.g0e6072fb45-goog

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

[Krzysztof Kozlowski]

On 30/05/2022 15:08, Paweł Anikiel wrote:
> Add devicetree for the Google Chameleon v3 board.
> 
> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
> Signed-off-by: Alexandru M Stan <amstan@chromium.org>

Your SoB chain looks odd. Who did what here?

> ---
>  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.

> +		     "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.

> +		compatible = "adi,ssm2603";
> +		reg = <0x1a>;
> +	};
> +};
> +
> +&i2c1 {
> +	status = "okay";
> +
> +	u80: u80@21 {
> +		compatible = "nxp,pca9535";

Generic node names.



Best regards,
Krzysztof

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

[Alexandru M Stan]

Hello Krzysztof

On Mon, May 30, 2022 at 11:56 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 30/05/2022 15:08, Paweł Anikiel wrote:
> > Add devicetree for the Google Chameleon v3 board.
> >
> > Signed-off-by: Paweł Anikiel <pan@semihalf.com>
> > Signed-off-by: Alexandru M Stan <amstan@chromium.org>
>
> 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).

>
> > ---
> >  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".

>
> > +                  "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.

>
> > +             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

Thanks,
Alexandru Stan (amstan)

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

[Krzysztof Kozlowski]

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@linaro.org> wrote:
>>
>> On 30/05/2022 15:08, Paweł Anikiel wrote:
>>> Add devicetree for the Google Chameleon v3 board.
>>>
>>> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
>>> Signed-off-by: Alexandru M Stan <amstan@chromium.org>
>>
>> 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

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@linaro.org> 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@linaro.org> wrote:
> >>
> >> On 30/05/2022 15:08, Paweł Anikiel wrote:
> >>> Add devicetree for the Google Chameleon v3 board.
> >>>
> >>> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
> >>> Signed-off-by: Alexandru M Stan <amstan@chromium.org>
> >>
> >> 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ł

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

[Krzysztof Kozlowski]

On 31/05/2022 16:47, Paweł Anikiel wrote:
> On Tue, May 31, 2022 at 11:11 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> 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@linaro.org> wrote:
>>>>
>>>> On 30/05/2022 15:08, Paweł Anikiel wrote:
>>>>> Add devicetree for the Google Chameleon v3 board.
>>>>>
>>>>> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
>>>>> Signed-off-by: Alexandru M Stan <amstan@chromium.org>
>>>>
>>>> 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.

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

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@linaro.org> wrote:
>
> On 31/05/2022 16:47, Paweł Anikiel wrote:
> > On Tue, May 31, 2022 at 11:11 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> 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@linaro.org> wrote:
> >>>>
> >>>> On 30/05/2022 15:08, Paweł Anikiel wrote:
> >>>>> Add devicetree for the Google Chameleon v3 board.
> >>>>>
> >>>>> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
> >>>>> Signed-off-by: Alexandru M Stan <amstan@chromium.org>
> >>>>
> >>>> 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

[PATCH 3/3] dt-bindings: altera: Update Arria 10 boards

[Paweł Anikiel]

The Mercury+ AA1 is not a standalone board, rather it's a module with
an Arria 10 SoC and some peripherals on it.

The Google Chameleon v3 is a base board for the Mercury+ AA1.

Signed-off-by: Paweł Anikiel <pan@semihalf.com>
---
 Documentation/devicetree/bindings/arm/altera.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/altera.yaml b/Documentation/devicetree/bindings/arm/altera.yaml
index 5e2017c0a051..c6e198bb5b29 100644
--- a/Documentation/devicetree/bindings/arm/altera.yaml
+++ b/Documentation/devicetree/bindings/arm/altera.yaml
@@ -25,7 +25,7 @@ properties:
         items:
           - enum:
               - altr,socfpga-arria10-socdk
-              - enclustra,mercury-aa1
+              - google,chameleon-v3
           - const: altr,socfpga-arria10
           - const: altr,socfpga
 
-- 
2.36.1.124.g0e6072fb45-goog

Re: [PATCH 3/3] dt-bindings: altera: Update Arria 10 boards

[Krzysztof Kozlowski]

Subject is too generic, please describe what are you doing. Anything
could be an "update".


On 30/05/2022 15:08, Paweł Anikiel wrote:
> The Mercury+ AA1 is not a standalone board, rather it's a module with
> an Arria 10 SoC and some peripherals on it.
> 
> The Google Chameleon v3 is a base board for the Mercury+ AA1.
> 
> Signed-off-by: Paweł Anikiel <pan@semihalf.com>
> ---
>  Documentation/devicetree/bindings/arm/altera.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/altera.yaml b/Documentation/devicetree/bindings/arm/altera.yaml
> index 5e2017c0a051..c6e198bb5b29 100644
> --- a/Documentation/devicetree/bindings/arm/altera.yaml
> +++ b/Documentation/devicetree/bindings/arm/altera.yaml
> @@ -25,7 +25,7 @@ properties:
>          items:
>            - enum:
>                - altr,socfpga-arria10-socdk
> -              - enclustra,mercury-aa1
> +              - google,chameleon-v3

As mentioned in patch 2, I would expect to have still enclustra
compatible. It's a SoM, so as usual it goes with its own binding, even
if not used standalone.

Best regards,
Krzysztof