[PATCH 1/1] arm: dts: icnova-a20-adb4006: Add board support

[Ludwig Kormann]

Hi Andre,

thanks for your immediate feedback!

Somehow I didn't receive your response via the mailing list, I just 
grabbed it from the mailing list archive [1].
Maybe because my post was still beeing moderated while you responded.

[1] https://lists.denx.de/pipermail/u-boot/2023-April/514575.html

I've got a few questions regarding your comments below.

> On Thu, 6 Apr 2023 14:32:18 +0100
> Andre Przywara <andre.przywara at arm.com  <https://lists.denx.de/listinfo/u-boot>> wrote:
>
> Hi,
>
> briefly forgot about sunxi-common-regulators.dtsi, so:
>
> >/On Thu, 6 Apr 2023 14:35:13 +0200 />/Ludwig Kormann <ludwig.kormann at in-circuit.de 
> <https://lists.denx.de/listinfo/u-boot>> wrote: />//>/Hi Ludwig, />//>/> Add board support for ICnova A20 SomPi compute module on />/> ICnova ADB4006 development board. />//>/thanks for sending this! />/For new boards we need to get the DT reviewed on the Linux list 
> first, so />/please send the DT there (To: Samuel, Jernej, Chen-Yu, Rob, Krzysztof 
> , Cc: />/linux-sunxi, linux-arm-kernel). This is more a process thing, since 
> the DT />/review knowledge and also compliance testing is more prominent on the />/Linux side. />//>/Once it has been approved and queued, we pick it up from there. />//
Alright, I'll create a patch v2 and post it to the linux list. I'll also 
put a link to this conversation as reference for the patch v1?

> //>/I will give you some feedback on the DT anyway: />//>/> Specification: />/> SoM />/> - Processor: Allwinner A20 Cortex-A7 Dual Core at 1GHz />/> - 512MB DDR3 RAM />/> - Fast Ethernet (Phy: Realtek RTL8201CP) />//>/So if you have a SoM/host board setup, we typically describe both />/components in separate files: a SoM .dtsi, and a devboard .dts. />/The devboard includes the SoM then. See the SoPine [1] for an example. />//>/[1] arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts />//
/Thanks - I'll check the SoPine and create the appropriate .dtsi and .dts.

/
> //>/I also see that the SoM has NAND flash, does that work for you with />/mainline U-Boot and/or Linux? Is it SLC, by any chance?/
/It's MLC NAND, but the NAND option for the module is not relevant 
anymore and will be removed due to problems with wear leveling and 
component availability in the past.
I didn't test if it would work with mainline drivers.

/
> //>//>/> ADB4006 />/> - I2C />/> - 2x USB 2.0 />/> - 1x Fast Ethernet port />/> - 1x SATA />/> - 2x buttons />//>/Are those buttons "power" and "FEL"? If not (connected to GPIOs), you />/should describe them in the DT, see "gpio-keys" in />/sun50i-h5-orangepi-pc2.dts for an example. /
The buttons are "pwron" and "boot" (not connected to GPIOs).

> >//>/> - 2x LEDS />/> - serial console />/> - HDMI />/> - µSD-Card slot />/> - Audio Line-In / Line-Out />/> - GPIO pinheaders />/> />/> https://wiki.in-circuit.de/index.php5?title=ICnova_ADB4006 />/> https://wiki.in-circuit.de/index.php5?title=ICnova_A20_SODIMM />/> Signed-off-by: Ludwig Kormann <ludwig.kormann at in-circuit.de 
> <https://lists.denx.de/listinfo/u-boot>> />/> --- />/> arch/arm/dts/Makefile | 1 + />/> arch/arm/dts/sun7i-a20-icnova-a20-adb4006.dts | 248 ++++++++++++++++++ />/> board/sunxi/MAINTAINERS | 5 + />/> configs/icnova-a20-adb4006_defconfig | 25 ++ />/> 4 files changed, 279 insertions(+) />/> create mode 100644 arch/arm/dts/sun7i-a20-icnova-a20-adb4006.dts />/> create mode 100644 configs/icnova-a20-adb4006_defconfig />/> />/> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile />/> index 0a9b1f7749..47dcaff780 100644 />/> --- a/arch/arm/dts/Makefile />/> +++ b/arch/arm/dts/Makefile />/> @@ -623,6 +623,7 @@ dtb-$(CONFIG_MACH_SUN7I) += \ />/> sun7i-a20-haoyu-marsboard.dtb \ />/> sun7i-a20-hummingbird.dtb \ />/> sun7i-a20-i12-tvbox.dtb \ />/> + sun7i-a20-icnova-a20-adb4006.dtb \ />/> sun7i-a20-icnova-swac.dtb \ />//>/Out of interest, how does this SoM/board relate to this "swac" thing 
> here? />/Is that an older, integrated board?/
It's a board of one of our customers that also features the ICnova A20 SoM.
Therefore the "sun7i-a20-icnova-swac.dts" could later also be updated 
for a splitted *.dtsi / *.dts configuration.

In my understanding this would be a seperate patch (series)? Or would it 
be good practice to include a patch that updates this board to use the 
SoM *.dtsi?

> >//>/> sun7i-a20-itead-ibox.dtb \ />/> sun7i-a20-lamobo-r1.dtb \ />/> diff --git a/arch/arm/dts/sun7i-a20-icnova-a20-adb4006.dts 
> b/arch/arm/dts/sun7i-a20-icnova-a20-adb4006.dts />/> new file mode 100644 />/> index 0000000000..e43df838ec />/> --- /dev/null />/> +++ b/arch/arm/dts/sun7i-a20-icnova-a20-adb4006.dts />/> @@ -0,0 +1,248 @@ />/> +/* />/> + * Copyright 2023 Ludwig Kormann <ludwig.kormann at in-circuit.de 
> <https://lists.denx.de/listinfo/u-boot>> />/> + * />/> + * This file is dual-licensed: you can use it either under the terms />//>/Please use the shorter SPDX header for licensing: />/// SPDX-License-Identifier: (GPL-2.0 OR MIT)/
/Okay./
> //>//>/> + * of the GPL or the X11 license, at your option. Note that this dual />/> + * licensing only applies to this file, and not this project as a />/> + * whole. />/> + * />/> + * a) This file is free software; you can redistribute it and/or />/> + * modify it under the terms of the GNU General Public License as />/> + * published by the Free Software Foundation; either version 2 of the />/> + * License, or (at your option) any later version. />/> + * />/> + * This file is distributed in the hope that it will be useful, />/> + * but WITHOUT ANY WARRANTY; without even the implied warranty of />/> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the />/> + * GNU General Public License for more details. />/> + * />/> + * Or, alternatively, />/> + * />/> + * b) Permission is hereby granted, free of charge, to any person />/> + * obtaining a copy of this software and associated documentation />/> + * files (the "Software"), to deal in the Software without />/> + * restriction, including without limitation the rights to use, />/> + * copy, modify, merge, publish, distribute, sublicense, and/or />/> + * sell copies of the Software, and to permit persons to whom the />/> + * Software is furnished to do so, subject to the following />/> + * conditions: />/> + * />/> + * The above copyright notice and this permission notice shall be />/> + * included in all copies or substantial portions of the Software. />/> + * />/> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, />/> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES />/> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND />/> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT />/> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, />/> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING />/> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR />/> + * OTHER DEALINGS IN THE SOFTWARE. />/> + */ />/> + />/> +/dts-v1/; />/> +#include "sun7i-a20.dtsi" />/> +#include "sunxi-common-regulators.dtsi" />/> + />/> +#include <dt-bindings/gpio/gpio.h> />/> +#include <dt-bindings/interrupt-controller/irq.h> />/> + />/> +/ { />/> + model = "In-Circuit ICnova A20 ADB4006"; />/> + compatible = "in-circuit,icnova-a20-adb4006", 
> "incircuit,icnova-a20", />/> + "allwinner,sun7i-a20"; />/> + />/> + aliases { />/> + serial0 = &uart0; />/> + }; />/> + />/> + chosen { />/> + stdout-path = "serial0:115200n8"; />/> + }; />/> + />/> + hdmi-connector { />/> + compatible = "hdmi-connector"; />/> + type = "a"; />/> + />/> + port { />/> + hdmi_con_in: endpoint { />/> + remote-endpoint = <&hdmi_out_con>; />/> + }; />/> + }; />/> + }; />/> + />/> + leds { />/> + compatible = "gpio-leds"; />/> + pinctrl-names = "default"; />/> + pinctrl-0 = <&led_pins_icnova_a20_adb4006>; />//>/Those two properties (and the pinctrl child node it refers to) are not />/needed, the "gpios" property takes care of that./
/Thanks for the hints, I was actually unsure if the pinctrl* entries are 
still relevant or not./
> //>//>/> + />/> + led-0 { />/> + label = "icnova_a20_adb4006:yellow:usr"; />//>/The "label" property is deprecated, please use "function" and "color" />/instead. Compare to sun50i-h616-orangepi-zero2.dts. />//>/> + gpios = <&pio 7 21 GPIO_ACTIVE_HIGH>; />//>/Please add the pin name ("PH21") as a comment, behind the semicolon./
Okay.
> //>//>/> + }; />/> + />/> + led-1 { />/> + label = "icnova_a20_adb4006:red:usr"; />/> + gpios = <&pio 7 20 GPIO_ACTIVE_HIGH>; />/> + linux,default-trigger = "heartbeat"; />/> + }; />/> + }; />/> +}; />/> + />/> +&ahci { />/> + target-supply = <&reg_ahci_5v>; />/> + status = "okay"; />/> +}; />/> + />/> +&codec { />/> + status = "okay"; />/> +}; />/> + />/> +&cpu0 { />/> + cpu-supply = <&reg_dcdc2>; />/> +}; />/> + />/> +&de { />/> + status = "okay"; />/> +}; />/> + />/> +&ehci0 { />/> + status = "okay"; />/> +}; />/> + />/> +&ehci1 { />/> + status = "okay"; />/> +}; />/> + />/> +&gmac { />/> + pinctrl-names = "default"; />/> + pinctrl-0 = <&gmac_mii_pins>; />/> + phy-handle = <&phy1>; />/> + phy-mode = "mii"; />/> + status = "okay"; />/> +}; />/> + />/> +&hdmi { />/> + status = "okay"; />/> +}; />/> + />/> +&hdmi_out { />/> + hdmi_out_con: endpoint { />/> + remote-endpoint = <&hdmi_con_in>; />/> + }; />/> +}; />/> + />/> +&i2c0 { />/> + status = "okay"; />/> + />/> + axp209: pmic at 34 <https://lists.denx.de/listinfo/u-boot> { />/> + reg = <0x34>; />/> + interrupt-parent = <&nmi_intc>; />/> + interrupts = <0 IRQ_TYPE_LEVEL_LOW>; />/> + }; />/> +}; />/> + />/> +&i2c1 { />/> + status = "okay"; />/> +}; />/> + />/> +&gmac_mdio { />/> + phy1: ethernet-phy at 1 <https://lists.denx.de/listinfo/u-boot> { />/> + reg = <1>; />/> + }; />/> +}; />/> + />/> +&mmc0 { />/> + vmmc-supply = <&reg_vcc3v3>; />/> + bus-width = <4>; />/> + cd-gpios = <&pio 7 1 GPIO_ACTIVE_LOW>; /* PH1 */ />/> + status = "okay"; />/> +}; />/> + />/> +&ohci0 { />/> + status = "okay"; />/> +}; />/> + />/> +&ohci1 { />/> + status = "okay"; />/> +}; />/> + />/> +&otg_sram { />/> + status = "okay"; />/> +}; />/> + />/> +&pio { />/> + led_pins_icnova_a20_adb4006: led-pins { />/> + pins = "PH20", "PH21"; />/> + function = "gpio_out"; />/> + drive-strength = <20>; />/> + }; />/> +}; />//>/As mentioned above, this whole pio node is not needed./
/Okay./
> //>//>/> + />/> +&reg_ahci_5v { />/> + status = "okay"; />/> +}; />/> + />/> +#include "axp209.dtsi" />/> + />/> +&ac_power_supply { />/> + status = "okay"; />/> +}; />/> + />/> +&reg_dcdc2 { />/> + regulator-always-on; />/> + regulator-min-microvolt = <1000000>; />/> + regulator-max-microvolt = <1400000>; />/> + regulator-name = "vdd-cpu"; />/> +}; />/> + />/> +&reg_dcdc3 { />/> + regulator-always-on; />/> + regulator-min-microvolt = <1000000>; />/> + regulator-max-microvolt = <1400000>; />/> + regulator-name = "vdd-int-dll"; />/> +}; />/> + />/> +&reg_ldo1 { />/> + regulator-name = "vdd-rtc"; />/> +}; />/> + />/> +&reg_ldo2 { />/> + regulator-always-on; />/> + regulator-min-microvolt = <3000000>; />/> + regulator-max-microvolt = <3000000>; />/> + regulator-name = "avcc"; />/> +}; />/> + />/> +&reg_ldo4 { />/> + regulator-always-on; />/> + regulator-min-microvolt = <2800000>; />/> + regulator-max-microvolt = <2800000>; />/> + regulator-name = "csi-io"; />//>/Why is this always on? Is there a camera connected that's not 
> described in />/the DT?/
/There's a CSI connector on the ADB4006 carrier board so I assumed I 
should also enable its supply.
But without adding a camera and describing it in the dts there's not 
much use to the connector - so it's better to disable the CSI power?

Also in general - should the DTS always be as "minimal" as possible?
The ADB4006 carrier board e.g. also has (unpopulated) pinheaders for all 
of its 8 serial interfaces - so should they be described in the dts?

/
> //>//>/> +}; />/> + />/> +&reg_usb1_vbus { />/> + status = "okay"; />/> +}; />/> + />/> +&reg_usb2_vbus { />/> + status = "okay"; />/> +}; />/> + />/> +&uart0 { />/> + pinctrl-names = "default"; />/> + pinctrl-0 = <&uart0_pb_pins>; />/> + status = "okay"; />/> +}; />/> + />/> +&usb_otg { />/> + dr_mode = "otg"; />/> + status = "okay"; />/> +}; />/> + />/> +&usbphy { />/> + usb0_id_det-gpios = <&pio 7 4 GPIO_ACTIVE_HIGH | GPIO_PULL_UP>; 
> /* PH4 */ />/> + usb1_vbus-supply = <&reg_usb1_vbus>; />/> + usb2_vbus-supply = <&reg_usb2_vbus>; />//>/So looking at the defconfig below, PH3, PH5 and PH6 also seem to play a />/role for USB. Please describe them in the DT here. We are in the process />/of removing the U-Boot Kconfig hacks for those GPIOs, in favour of using />/DT. Not sure if VBUS is really provided by the AXP then, are there 
> separate />/GPIO controlled regulators, by any chance? /
> Scratch the part about the regulators, that's what is already in
> sunxi-common-regulators.dtsi.
> You should add a usb0_vbus_det-gpios property for PH5, though.

This was also a part that confused me as I couldn't tell if it's better 
to declare these pins in the config or dts.
So in general the dts option should always be the preferred one relating 
hardware description / declaration?

> >//>/> + status = "okay"; />/> +}; />/> diff --git a/board/sunxi/MAINTAINERS b/board/sunxi/MAINTAINERS />/> index 80e3f4be4b..2bbf0188d7 100644 />/> --- a/board/sunxi/MAINTAINERS />/> +++ b/board/sunxi/MAINTAINERS />/> @@ -231,6 +231,11 @@ M: Stefan Roese <sr at denx.de 
> <https://lists.denx.de/listinfo/u-boot>> />/> S: Maintained />/> F: configs/icnova-a20-swac_defconfig />/> />/> +ICnova-A20-ADB4006 BOARD />/> +M: Ludwig Kormann <ludwig.kormann at in-circuit.de 
> <https://lists.denx.de/listinfo/u-boot>> />/> +S: Maintained />/> +F: configs/icnova-a20-adb4006_defconfig />/> + />/> ITEAD IBOX BOARD />/> M: Marcus Cooper <codekipper at gmail.com 
> <https://lists.denx.de/listinfo/u-boot>> />/> S: Maintained />/> diff --git a/configs/icnova-a20-adb4006_defconfig 
> b/configs/icnova-a20-adb4006_defconfig />/> new file mode 100644 />/> index 0000000000..c327410ca7 />/> --- /dev/null />/> +++ b/configs/icnova-a20-adb4006_defconfig />/> @@ -0,0 +1,25 @@ />/> +CONFIG_ARM=y />/> +CONFIG_ARCH_SUNXI=y />/> +CONFIG_DEFAULT_DEVICE_TREE="sun7i-a20-icnova-a20-adb4006" />/> +CONFIG_SPL=y />/> +CONFIG_MACH_SUN7I=y />/> +CONFIG_DRAM_CLK=384 />/> +CONFIG_USB0_VBUS_DET="PH5" />/> +CONFIG_USB0_ID_DET="PH4" />/> +CONFIG_USB1_VBUS_PIN="PH6" />/> +CONFIG_USB2_VBUS_PIN="PH3" />//>/These four pins should all have a DT equivalent, then they could go away />/(eventually). />//>/> +CONFIG_SATAPWR="PB8" />//>/SATAPWR is going to go away. Is that regulator controlling power for a />/SATA *disk*? Via this white 2pin header?/
/Yes, it's used to directly power a SATA disk./
> //>/There is no really perfect DT replacement for this, but you could copy />/this solution here: />/https://lore.kernel.org/linux-arm-kernel/20230120012616.30960-1-andre.przywara@arm.com/ 
> /
> Also scratch that, PB8 is mentioned via target-supply already, so that
> should be fine.
I'll remove all pin configurations from the config and update the dts 
accordingly.

After updating the files, can I post the patch for another review as a 
response to this thread or should I directly provide a patch v2 to the 
linux kernel list for their review?

Thanks again for your help!

kind regards,
Ludwig

> Sorry for the confusion about those!
>
> Cheers,
> Andre
>
> >//>/Cheers (und Grüße an die Elbe!), />/Andre />//>/> +CONFIG_AHCI=y />/> +# CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set />/> +CONFIG_SPL_I2C=y />/> +CONFIG_SCSI_AHCI=y />/> +CONFIG_SYS_I2C_MVTWSI=y />/> +CONFIG_SYS_I2C_SLAVE=0x7f />/> +CONFIG_SYS_I2C_SPEED=400000 />/> +CONFIG_ETH_DESIGNWARE=y />/> +CONFIG_MII=y />/> +CONFIG_SUN7I_GMAC=y />/> +CONFIG_AXP_ALDO4_VOLT=2800 />/> +CONFIG_SCSI=y />/> +CONFIG_USB_EHCI_HCD=y />/> +CONFIG_USB_OHCI_HCD=y />//