[PATCH 1/1] arm: dts: icnova-a20-adb4006: Add board support
Ludwig Kormann
ludwig.kormann at in-circuit.de
Mon Apr 17 14:54:10 CEST 2023
Hi Andre,
sorry for the mess - I've reformatted my last mail properly to be
readable below.
kind regards
Ludwig
---
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> 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?
> > 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>
> > > ---
> > > 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>
> > > + *
> > > + * 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 = <®_ahci_5v>;
> > > + status = "okay";
> > > +};
> > > +
> > > +&codec {
> > > + status = "okay";
> > > +};
> > > +
> > > +&cpu0 {
> > > + cpu-supply = <®_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 = <®_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.
> >
> > > +
> > > +®_ahci_5v {
> > > + status = "okay";
> > > +};
> > > +
> > > +#include "axp209.dtsi"
> > > +
> > > +&ac_power_supply {
> > > + status = "okay";
> > > +};
> > > +
> > > +®_dcdc2 {
> > > + regulator-always-on;
> > > + regulator-min-microvolt = <1000000>;
> > > + regulator-max-microvolt = <1400000>;
> > > + regulator-name = "vdd-cpu";
> > > +};
> > > +
> > > +®_dcdc3 {
> > > + regulator-always-on;
> > > + regulator-min-microvolt = <1000000>;
> > > + regulator-max-microvolt = <1400000>;
> > > + regulator-name = "vdd-int-dll";
> > > +};
> > > +
> > > +®_ldo1 {
> > > + regulator-name = "vdd-rtc";
> > > +};
> > > +
> > > +®_ldo2 {
> > > + regulator-always-on;
> > > + regulator-min-microvolt = <3000000>;
> > > + regulator-max-microvolt = <3000000>;
> > > + regulator-name = "avcc";
> > > +};
> > > +
> > > +®_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?
> >
> > > +};
> > > +
> > > +®_usb1_vbus {
> > > + status = "okay";
> > > +};
> > > +
> > > +®_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 = <®_usb1_vbus>;
> > > + usb2_vbus-supply = <®_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>
> > > 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?
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