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

Andre Przywara andre.przywara at arm.com
Thu Apr 20 10:51:23 CEST 2023 

On Mon, 17 Apr 2023 14:54:10 +0200
Ludwig Kormann <ludwig.kormann at in-circuit.de> wrote:

Hi Ludwig,

> thanks for your immediate feedback!

sorry for the delay, but I see you made all the right decisions in the
Linux post ;-)

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

Yes, as you did ;-)

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

Yeah, raw NAND is only cheap, and nothing else, so if you can avoid it,
all the better. There is some level of support for SLC, but since this is
not really cheap, it contradict its only purpose ;-).

> I didn't test if it would work with mainline drivers.

Don't bother, just use eMMC and move on ;-)

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

Right, in this case they indeed don't appear in the DT.

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

Yes, but this is a separate patch. Let's first get the SoM.dtsi correct,
then you can refactor the SWAC .dts.

> 
> > >  
> > > >      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>
> > > > + *
> > > > + * 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 {
> > > > +        reg = <0x34>;
> > > > +        interrupt-parent = <&nmi_intc>;
> > > > +        interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > > +    };
> > > > +};
> > > > +
> > > > +&i2c1 {
> > > > +    status = "okay";
> > > > +};
> > > > +
> > > > +&gmac_mdio {
> > > > +    phy1: ethernet-phy at 1 {
> > > > +        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?

In general we don't enable peripherals that have their pins only on
headers. People might use those pins as GPIOs, so it would be up to an
instance specific DT overlay to activate the pinmuxes for specific
peripherals, as needed.

> > > > +};
> > > > +
> > > > +&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?

Yes, originally we didn't use the DT much in U-Boot, but this is changing.
We are about to remove all special GPIO symbols from the configs, and
rely on what's in the DT already. We only need some GPIOs for SPL code (no
DT at this point), but this should be rare. See:
https://source.denx.de/u-boot/u-boot/-/commit/f0500692972a

> > > > +    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>
> > > >  S:    Maintained
> > > >  F:    configs/icnova-a20-swac_defconfig
> > > >
> > > > +ICnova-A20-ADB4006 BOARD
> > > > +M:    Ludwig Kormann <ludwig.kormann at in-circuit.de>
> > > > +S:    Maintained
> > > > +F:    configs/icnova-a20-adb4006_defconfig
> > > > +
> > > >  ITEAD IBOX BOARD
> > > >  M:    Marcus Cooper <codekipper at gmail.com>
> > > >  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?

Yeah, going directly to the Linux list was the right choice.

Thanks,
Andre

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

More information about the U-Boot mailing list