2022-05-30 14:13:53

by Paweł Anikiel

[permalink] [raw]
Subject: [PATCH 0/3] Add Chameleon v3 devicetree

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



2022-05-30 18:29:20

by Paweł Anikiel

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

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 <[email protected]>
---
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


2022-06-01 15:57:38

by Paweł Anikiel

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

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 <[email protected]>
---
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


2022-06-01 19:20:28

by Krzysztof Kozlowski

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

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
> <[email protected]> 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 <[email protected]>
>>
>> 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

2022-06-01 19:35:07

by Krzysztof Kozlowski

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

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 <[email protected]>

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

2022-06-01 20:16:03

by Paweł Anikiel

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

Add devicetree for the Google Chameleon v3 board.

Signed-off-by: Paweł Anikiel <[email protected]>
Signed-off-by: Alexandru M Stan <[email protected]>
---
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


2022-06-01 21:12:30

by Paweł Anikiel

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

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
<[email protected]> 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 <[email protected]>
>
> 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ł

2022-06-01 21:36:46

by Krzysztof Kozlowski

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

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 <[email protected]>
> ---
> 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