Re: [PATCH v4 3/4] arm64: dts: allwinner: h616: Add BigTreeTech CB1 SoM & boards support

[Martin Botka <martin.botka@xxxxxxxxxxxxxx>]

On Sun, Aug 13 2023 at 05:52:10 PM +02:00:00, Jernej Škrabec 
<jernej.skrabec@xxxxxxxxx> wrote:
> Hi Martin,
>
> since this will be obviously delayed to 6.7 cycle due to mfd patch 
> not being
> merged in time, I have few nits below.
>
> Dne ponedeljek, 07. avgust 2023 ob 16:53:23 CEST je Martin Botka 
> napisal(a):
> >  From: Martin Botka <martin.botka@xxxxxxxxxxxxxx>
> >
> >  CB1 is Compute Module style board that plugs into Rpi board style 
> > adapter or
> >  Manta 3D printer boards (M4P/M8P).
> >
> >  The SoM features:
> >    - H616 SoC
> >    - 1GiB of RAM
> >    - AXP313A PMIC
> >    - RTL8189FTV WiFi
> >
> >  Boards feature:
> >    - 4x USB via USB2 hub (usb1 on SoM).
> >    - SDcard slot for loading images.
> >    - Ethernet port wired to the internal PHY. (100M)
> >    - 2x HDMI 2.0. (Only 1 usable on CB1)
> >    - Power and Status LEDs. (Only Status LED usable on CB1)
> >    - 40 pin GPIO header
> >
> >  Currently working:
> >    - Booting
> >    - USB
> >    - UART
> >    - MMC
> >    - Status LED
> >    - WiFi (RTL8189FS via out of tree driver)
> >
> >  I didnt want to duplicate things so the manta DTS can also be used 
> > on BTT
> >  pi4b adapter. CB1 SoM has its own DTSI file in case other boards 
> > shows up
> >  that accept this SoM.
> >
> >  Signed-off-by: Martin Botka <martin.botka@xxxxxxxxxxxxxx>
> >  Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
> >  ---
> >  Changes in V2:
> >      - Fixed whitespace errors
> >      - Move UART into carrier boards and BTT Pi
> >      - Remove usb1-vbus regulator
> >      - Fix ranges and naming of AXP313A rails
> >      - Add comment specifying why broken-cd in mmc0 is needed
> >      - Rename sdio_wifi to wifi
> >      - Specify in commit description that USB-OTG doesnt work
> >  Changes in V3:
> >      - Add missed semicolons
> >      - Move model string from dtsi to board dts
> >      - Add cb1 compatible
> >      - Remove extra empty line
> >  Changed in V4:
> >      - Extend the range of vcc-dram to 1.5V (1.35V max caused issues 
> > with
> >  booting up
> >
> >   arch/arm64/boot/dts/allwinner/Makefile        |   1 +
> >   .../sun50i-h616-bigtreetech-cb1-manta.dts     |  35 +++++
> >   .../sun50i-h616-bigtreetech-cb1.dtsi          | 140 
> > ++++++++++++++++++
> >   3 files changed, 176 insertions(+)
> >   create mode 100644
> >  arch/arm64/boot/dts/allwinner/sun50i-h616-bigtreetech-cb1-manta.dts 
> > create
> >  mode 100644 
> > arch/arm64/boot/dts/allwinner/sun50i-h616-bigtreetech-cb1.dtsi
> >
> >  diff --git a/arch/arm64/boot/dts/allwinner/Makefile
> >  b/arch/arm64/boot/dts/allwinner/Makefile index 
> > 6a96494a2e0a..7b386428510b
> >  100644
> >  --- a/arch/arm64/boot/dts/allwinner/Makefile
> >  +++ b/arch/arm64/boot/dts/allwinner/Makefile
> >  @@ -38,5 +38,6 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-pine-h64.dtb
> >   dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-pine-h64-model-b.dtb
> >   dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-tanix-tx6.dtb
> >   dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-tanix-tx6-mini.dtb
> >  +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-bigtreetech-cb1-manta.dtb
> >   dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-orangepi-zero2.dtb
> >   dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-x96-mate.dtb
> >  diff --git
> >  
> > a/arch/arm64/boot/dts/allwinner/sun50i-h616-bigtreetech-cb1-manta.dts
> >  
> > b/arch/arm64/boot/dts/allwinner/sun50i-h616-bigtreetech-cb1-manta.dts 
> > new
> >  file mode 100644
> >  index 000000000000..dbce61b355d6
> >  --- /dev/null
> >  +++ 
> > b/arch/arm64/boot/dts/allwinner/sun50i-h616-bigtreetech-cb1-manta.dts
> >  @@ -0,0 +1,35 @@
> >  +// SPDX-License-Identifier: (GPL-2.0+ or MIT)
> >  +/*
> >  + * Copyright (C) 2023 Martin Botka <martin.botka@xxxxxxxxxxxxxx>.
> >  + */
> >  +
> >  +/dts-v1/;
> >  +
> >  +#include "sun50i-h616-bigtreetech-cb1.dtsi"
> >  +
> >  +/ {
> >  +	model = "BigTreeTech CB1";
> >  +	compatible = "bigtreetech,cb1-manta", "bigtreetech,cb1",
> >  "allwinner,sun50i-h616"; +
> >  +	aliases {
> >  +		serial0 = &uart0;
> >  +	};
> >  +
> >  +	chosen {
> >  +		stdout-path = "serial0:115200n8";
> >  +	};
> >  +};
> >  +
> >  +&ehci1 {
> >  +	status = "okay";
> >  +};
> >  +
> >  +&ohci1 {
> >  +	status = "okay";
> >  +};
> >  +
> >  +&uart0 {
> >  +	pinctrl-names = "default";
> >  +	pinctrl-0 = <&uart0_ph_pins>;
> >  +	status = "okay";
> >  +};
> >  diff --git 
> > a/arch/arm64/boot/dts/allwinner/sun50i-h616-bigtreetech-cb1.dtsi
> >  b/arch/arm64/boot/dts/allwinner/sun50i-h616-bigtreetech-cb1.dtsi 
> > new file
> >  mode 100644
> >  index 000000000000..5e756f217813
> >  --- /dev/null
> >  +++ b/arch/arm64/boot/dts/allwinner/sun50i-h616-bigtreetech-cb1.dtsi
> >  @@ -0,0 +1,140 @@
> >  +// SPDX-License-Identifier: (GPL-2.0+ or MIT)
> >  +/*
> >  + * Copyright (C) 2023 Martin Botka <martin.botka@xxxxxxxxxxxxxx>.
> >  + */
> >  +
> >  +/dts-v1/;
> >  +
> >  +#include "sun50i-h616.dtsi"
> >  +
> >  +#include <dt-bindings/gpio/gpio.h>
> >  +#include <dt-bindings/interrupt-controller/arm-gic.h>
> >  +#include <dt-bindings/leds/common.h>
> >  +
> >  +/ {
> >  +	aliases {
> >  +		ethernet0 = &rtl8189ftv;
> >  +	};
> >  +
> >  +	leds {
> >  +		compatible = "gpio-leds";
> >  +
> >  +		led-0 {
> >  +			function = LED_FUNCTION_STATUS;
> >  +			color = <LED_COLOR_ID_GREEN>;
> >  +			gpios = <&pio 7 5 GPIO_ACTIVE_HIGH>; /* PH5
> */
> >  +		};
> >  +	};
> >  +
> >  +	reg_vcc5v: regulator-vcc5v {
> >  +		/* board wide 5V supply from carrier boards */
> >  +		compatible = "regulator-fixed";
> >  +		regulator-name = "vcc-5v";
> >  +		regulator-min-microvolt = <5000000>;
> >  +		regulator-max-microvolt = <5000000>;
> >  +		regulator-always-on;
> >  +	};
> >  +
> >  +	reg_vcc33_wifi: vcc33-wifi {
> >  +		/* Always on 3.3V regulator for WiFi */
>
> Please drop the comment. It's pretty obvious from properties.
>
> >  +		compatible = "regulator-fixed";
> >  +		regulator-name = "vcc33-wifi";
> >  +		regulator-min-microvolt = <3300000>;
> >  +		regulator-max-microvolt = <3300000>;
> >  +		regulator-always-on;
> >  +		vin-supply = <&reg_vcc5v>;
> >  +	};
> >  +
> >  +	reg_vcc_wifi_io: vcc-wifi-io {
> >  +		/* Always on 1.8V/300mA regulator for WiFi */
>
> Ditto.
>
> Once fixed, you can add:
> Reviewed-by: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
>
> Best regards,
> Jernej
Got it for both comments. Will send V5 tomorrow.

Cheers,
Martin
> >  +		compatible = "regulator-fixed";
> >  +		regulator-name = "vcc-wifi-io";
> >  +		regulator-min-microvolt = <1800000>;
> >  +		regulator-max-microvolt = <1800000>;
> >  +		regulator-always-on;
> >  +		vin-supply = <&reg_vcc33_wifi>;
> >  +	};
> >  +
> >  +	wifi_pwrseq: wifi-pwrseq {
> >  +		compatible = "mmc-pwrseq-simple";
> >  +		clocks = <&rtc 1>;
> >  +		clock-names = "ext_clock";
> >  +		reset-gpios = <&pio 6 18 GPIO_ACTIVE_LOW>; /* PG18 */
> >  +		post-power-on-delay-ms = <200>;
> >  +	};
> >  +};
> >  +
> >  +&mmc0 {
> >  +	vmmc-supply = <&reg_dldo1>;
> >  +	/* Card detection pin is not connected */
> >  +	broken-cd;
> >  +	bus-width = <4>;
> >  +	status = "okay";
> >  +};
> >  +
> >  +&mmc1 {
> >  +	vmmc-supply = <&reg_vcc33_wifi>;
> >  +	vqmmc-supply = <&reg_vcc_wifi_io>;
> >  +	mmc-pwrseq = <&wifi_pwrseq>;
> >  +	bus-width = <4>;
> >  +	non-removable;
> >  +	mmc-ddr-1_8v;
> >  +	status = "okay";
> >  +
> >  +	rtl8189ftv: wifi@1 {
> >  +		reg = <1>;
> >  +	};
> >  +};
> >  +
> >  +&r_i2c {
> >  +	status = "okay";
> >  +
> >  +	axp313a: pmic@36 {
> >  +		compatible = "x-powers,axp313a";
> >  +		reg = <0x36>;
> >  +		interrupt-controller;
> >  +		#interrupt-cells = <1>;
> >  +
> >  +		regulators{
> >  +			reg_dcdc1: dcdc1 {
> >  +				regulator-name = "vdd-gpu-sys";
> >  +				regulator-min-microvolt =
> <810000>;
> >  +				regulator-max-microvolt =
> <990000>;
> >  +				regulator-always-on;
> >  +			};
> >  +
> >  +			reg_dcdc2: dcdc2 {
> >  +				regulator-name = "vdd-cpu";
> >  +				regulator-min-microvolt =
> <810000>;
> >  +				regulator-max-microvolt =
> <1100000>;
> >  +				regulator-ramp-delay = <200>;
> >  +				regulator-always-on;
> >  +			};
> >  +
> >  +			reg_dcdc3: dcdc3 {
> >  +				regulator-name = "vcc-dram";
> >  +				regulator-min-microvolt =
> <1350000>;
> >  +				regulator-max-microvolt =
> <1500000>;
> >  +				regulator-always-on;
> >  +			};
> >  +
> >  +			reg_aldo1: aldo1 {
> >  +				regulator-name = "vcc-1v8-pll";
> >  +				regulator-min-microvolt =
> <1800000>;
> >  +				regulator-max-microvolt =
> <1800000>;
> >  +				regulator-always-on;
> >  +			};
> >  +
> >  +			reg_dldo1: dldo1 {
> >  +				regulator-name = "vcc-3v3-io";
> >  +				regulator-min-microvolt =
> <3300000>;
> >  +				regulator-max-microvolt =
> <3300000>;
> >  +				regulator-always-on;
> >  +			};
> >  +		};
> >  +	};
> >  +};
> >  +
> >  +&usbphy {
> >  +	status = "okay";
> >  +};