Message-ID: <20241211130853.25b82d7b@donnerap.manchester.arm.com>
Date: Wed, 11 Dec 2024 13:08:53 +0000
From: Andre Przywara <andre.przywara@....com>
To: Icenowy Zheng <uwu@...nowy.me>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>, Jernej
 Skrabec <jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>,
 Maxime Ripard <mripard@...nel.org>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] ARM: dts: sunxi: add support for RerVision
 A33-Vstar board

On Tue, 17 Sep 2024 17:54:38 +0800
Icenowy Zheng <uwu@...nowy.me> wrote:

Hi,

I do understand it's too late now, but just saw some oddities about the
regulator voltages when looking at the U-Boot version of this patch:

> 在 2024-09-13星期五的 18:48 +0800，Icenowy Zheng写道：
> > RerVision A33-Vstar board is a board based on their A33-Core1 SoM
> > (A33
> > SoC + 512MiB DRAM + 4GiB eMMC + AXP223 PMIC), with multiple
> > peripherals:
> > 
> > - MicroSD card slot
> > - 4.0mm/1.7mm DC jack connected to ACIN of AXP223 (and a XH2.54 2-pin
> >   connector for alternative 5V DC IN)
> > - OTG-capable microUSB port
> > - Reserved pads for soldering Li-ion battery and/or 3V RTC battery
> > - 3 LRADC-attached keys and 2 fixed function power/reset keys
> > - AP6212 Wi-Fi/BT combo module
> > - On-board GL850G hub attached to the USB host port of A33, and a
> >   RTL8152 USB Ethernet chip at the downstream of the hub
> > - Onboard microphone (not supported yet) and headphone jack
> > - 3 UART ports as PH2.0 3-pin connectors (UART2 one is currently used
> > as
> >   debug output and others are ignored yet)
> > 
> > Signed-off-by: Icenowy Zheng <uwu@...nowy.me>
> > ---  
> 
> Add a dependency to this patch at [1], which allows GL850G to have
> downstream device nodes in DT.
> 
> [1]
> https://lore.kernel.org/lkml/20240917094008.283529-1-uwu@icenowy.me/
> 
> >  arch/arm/boot/dts/allwinner/Makefile          |   1 +
> >  .../dts/allwinner/sun8i-a33-vstar-core1.dtsi  |  96 ++++++++
> >  .../boot/dts/allwinner/sun8i-a33-vstar.dts    | 205
> > ++++++++++++++++++
> >  3 files changed, 302 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/allwinner/sun8i-a33-vstar-
> > core1.dtsi
> >  create mode 100644 arch/arm/boot/dts/allwinner/sun8i-a33-vstar.dts
> > 
> > diff --git a/arch/arm/boot/dts/allwinner/Makefile
> > b/arch/arm/boot/dts/allwinner/Makefile
> > index cd0d044882cf8..d548f4a2621a1 100644
> > --- a/arch/arm/boot/dts/allwinner/Makefile
> > +++ b/arch/arm/boot/dts/allwinner/Makefile
> > @@ -215,6 +215,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
> >         sun8i-a33-olinuxino.dtb \
> >         sun8i-a33-q8-tablet.dtb \
> >         sun8i-a33-sinlinx-sina33.dtb \
> > +       sun8i-a33-vstar.dtb \
> >         sun8i-a83t-allwinner-h8homlet-v2.dtb \
> >         sun8i-a83t-bananapi-m3.dtb \
> >         sun8i-a83t-cubietruck-plus.dtb \
> > diff --git a/arch/arm/boot/dts/allwinner/sun8i-a33-vstar-core1.dtsi
> > b/arch/arm/boot/dts/allwinner/sun8i-a33-vstar-core1.dtsi
> > new file mode 100644
> > index 0000000000000..ba794b842ec4e
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/allwinner/sun8i-a33-vstar-core1.dtsi
> > @@ -0,0 +1,96 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright (C) 2024 Icenowy Zheng <uwu@...nowy.me>
> > + */
> > +
> > +#include "sun8i-a33.dtsi"
> > +
> > +&mmc2 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&mmc2_8bit_pins>;
> > +       vmmc-supply = <&reg_dcdc1>;
> > +       bus-width = <8>;
> > +       non-removable;
> > +       cap-mmc-hw-reset;
> > +       status = "okay";
> > +};
> > +
> > +&mmc2_8bit_pins {
> > +       /* Increase drive strength for DDR modes */
> > +       drive-strength = <40>;
> > +};
> > +
> > +&r_rsb {
> > +       status = "okay";
> > +
> > +       axp22x: pmic@3a3 {
> > +               compatible = "x-powers,axp223";
> > +               reg = <0x3a3>;
> > +               interrupt-parent = <&r_intc>;
> > +               interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_LOW>;
> > +               eldoin-supply = <&reg_dcdc1>;
> > +               x-powers,drive-vbus-en;
> > +       };
> > +};
> > +
> > +#include "axp223.dtsi"
> > +
> > +&reg_aldo1 {
> > +       regulator-always-on;
> > +       regulator-min-microvolt = <3300000>;
> > +       regulator-max-microvolt = <3300000>;
> > +       regulator-name = "vcc-io";
> > +};
> > +
> > +&reg_aldo2 {
> > +       regulator-always-on;
> > +       regulator-min-microvolt = <2350000>;
> > +       regulator-max-microvolt = <2650000>;

So what is the value? I guess no one is adjusting this, ever? So it stays
at the 2.5V that U-Boot programs? I think we should fix this to this
voltage, then. What's the value that the BSP uses, is that different?

> > +       regulator-name = "vdd-dll";
> > +};
> > +
> > +&reg_aldo3 {
> > +       regulator-always-on;
> > +       regulator-min-microvolt = <3300000>;
> > +       regulator-max-microvolt = <3300000>;
> > +       regulator-name = "vcc-avcc";
> > +};
> > +
> > +&reg_dc5ldo {
> > +       regulator-always-on;
> > +       regulator-min-microvolt = <900000>;
> > +       regulator-max-microvolt = <1400000>;

Same here, why a range? I don't see U-Boot setting this, so does it stay
at some reset value, or does the kernel adjust it when the AXP driver
comes up? If any case, we should have exactly one voltage in here.

> > +       regulator-name = "vdd-cpus";
> > +};
> > +
> > +&reg_dcdc1 {
> > +       regulator-always-on;
> > +       regulator-min-microvolt = <3300000>;
> > +       regulator-max-microvolt = <3300000>;
> > +       regulator-name = "vcc-3v3";
> > +};
> > +
> > +&reg_dcdc2 {
> > +       regulator-always-on;
> > +       regulator-min-microvolt = <900000>;
> > +       regulator-max-microvolt = <1400000>;

Sames as above, looks like to be 1.1V here?

There reason I am pointing this out is this relies on U-Boot setting some
hardcoded values, which I want to get rid off, as U-Boot should read the
voltages from the DT as well.

Cheers,
Andre

> > +       regulator-name = "vdd-sys";
> > +};
> > +
> > +&reg_dcdc3 {
> > +       regulator-always-on;
> > +       regulator-min-microvolt = <900000>;
> > +       regulator-max-microvolt = <1400000>;
> > +       regulator-name = "vdd-cpu";
> > +};
> > +
> > +&reg_dcdc5 {
> > +       regulator-always-on;
> > +       regulator-min-microvolt = <1500000>;
> > +       regulator-max-microvolt = <1500000>;
> > +       regulator-name = "vcc-dram";
> > +};
> > +
> > +&reg_rtc_ldo {
> > +       regulator-name = "vcc-rtc";
> > +};
> > diff --git a/arch/arm/boot/dts/allwinner/sun8i-a33-vstar.dts
> > b/arch/arm/boot/dts/allwinner/sun8i-a33-vstar.dts
> > new file mode 100644
> > index 0000000000000..9f5c29b3df46d
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/allwinner/sun8i-a33-vstar.dts
> > @@ -0,0 +1,205 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > +/*
> > + * Copyright (C) 2024 Icenowy Zheng <uwu@...nowy.me>
> > + */
> > +
> > +/dts-v1/;
> > +#include "sun8i-a33-vstar-core1.dtsi"
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/input/input.h>
> > +
> > +/ {
> > +       model = "Rervision A33-Vstar";
> > +       compatible = "rervision,a33-vstar",
> > +                    "rervision,a33-core1",
> > +                    "allwinner,sun8i-a33";
> > +
> > +       aliases {
> > +               serial0 = &uart0;
> > +               ethernet0 = &r8152;
> > +       };
> > +
> > +       chosen {
> > +               stdout-path = "serial0:115200n8";
> > +       };
> > +
> > +       reg_usb1_vbus: regulator-usb1-vbus {
> > +               compatible = "regulator-fixed";
> > +               regulator-name = "usb1-vbus";
> > +               regulator-min-microvolt = <5000000>;
> > +               regulator-max-microvolt = <5000000>;
> > +               regulator-boot-on;
> > +               enable-active-high;
> > +               gpio = <&pio 1 2 GPIO_ACTIVE_HIGH>; /* PB2 */
> > +       };
> > +
> > +       wifi_pwrseq: pwrseq {
> > +               compatible = "mmc-pwrseq-simple";
> > +               reset-gpios = <&r_pio 0 6 GPIO_ACTIVE_LOW>; /* PL6 */
> > +               clocks = <&rtc CLK_OSC32K_FANOUT>;
> > +               clock-names = "ext_clock";
> > +       };
> > +};
> > +
> > +&ac_power_supply {
> > +       status = "okay";
> > +};
> > +
> > +&codec {
> > +       status = "okay";
> > +};
> > +
> > +&dai {
> > +       status = "okay";
> > +};
> > +
> > +&ehci0 {
> > +       #address-cells = <1>;
> > +       #size-cells = <0>;
> > +       status = "okay";
> > +
> > +       hub@1 {
> > +               /* Onboard GL850G hub which needs no extra power
> > sequence */
> > +               compatible = "usb5e3,608";
> > +               reg = <1>;
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +
> > +               r8152: ethernet@4 {
> > +                       /*
> > +                        * Onboard Realtek RTL8152 USB Ethernet,
> > +                        * with no MAC address programmed
> > +                        */
> > +                       compatible = "usbbda,8152";
> > +                       reg = <4>;
> > +               };
> > +       };
> > +};
> > +
> > +&lradc {
> > +       vref-supply = <&reg_aldo3>;
> > +       status = "okay";
> > +
> > +       button-191 {
> > +               label = "V+";
> > +               linux,code = <KEY_VOLUMEUP>;
> > +               channel = <0>;
> > +               voltage = <191011>;
> > +       };
> > +
> > +       button-391 {
> > +               label = "V-";
> > +               linux,code = <KEY_VOLUMEDOWN>;
> > +               channel = <0>;
> > +               voltage = <391304>;
> > +       };
> > +
> > +       button-600 {
> > +               label = "BACK";
> > +               linux,code = <KEY_BACK>;
> > +               channel = <0>;
> > +               voltage = <600000>;
> > +       };
> > +};
> > +
> > +&mmc0 {
> > +       vmmc-supply = <&reg_dcdc1>;
> > +       bus-width = <4>;
> > +       cd-gpios = <&pio 1 4 GPIO_ACTIVE_LOW>; /* PB4 */
> > +       status = "okay";
> > +};
> > +
> > +&mmc1 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&mmc1_pg_pins>;
> > +       vmmc-supply = <&reg_dldo1>;
> > +       mmc-pwrseq = <&wifi_pwrseq>;
> > +       bus-width = <4>;
> > +       non-removable;
> > +       status = "okay";
> > +
> > +       brcmf: wifi@1 {
> > +               reg = <1>;
> > +               compatible = "brcm,bcm4329-fmac";
> > +               interrupt-parent = <&r_pio>;
> > +               interrupts = <0 7 IRQ_TYPE_LEVEL_LOW>; /* PL7 */
> > +               interrupt-names = "host-wake";
> > +       };
> > +};
> > +
> > +/*
> > + * Our WiFi chip needs both DLDO1 and DLDO2 to be powered at the
> > same
> > + * time, with the two being in sync. Since this is not really
> > + * supported right now, just use the two as always on, and we will
> > fix
> > + * it later.
> > + */
> > +&reg_dldo1 {
> > +       regulator-always-on;
> > +       regulator-min-microvolt = <3300000>;
> > +       regulator-max-microvolt = <3300000>;
> > +       regulator-name = "vcc-wifi0";
> > +};
> > +
> > +&reg_dldo2 {
> > +       regulator-always-on;
> > +       regulator-min-microvolt = <3300000>;
> > +       regulator-max-microvolt = <3300000>;
> > +       regulator-name = "vcc-wifi1";
> > +};
> > +
> > +&reg_drivevbus {
> > +       regulator-name = "usb0-vbus";
> > +       status = "okay";
> > +};
> > +
> > +&sound {
> > +       /* TODO: on-board microphone */
> > +
> > +       simple-audio-card,widgets = "Headphone", "Headphone Jack";
> > +       simple-audio-card,routing =
> > +               "Left DAC", "DACL",
> > +               "Right DAC", "DACR",
> > +               "Headphone Jack", "HP";
> > +       status = "okay";
> > +};
> > +
> > +&uart0 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&uart0_pb_pins>;
> > +       status = "okay";
> > +};
> > +
> > +&uart1 {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&uart1_pg_pins>, <&uart1_cts_rts_pg_pins>;
> > +       uart-has-rtscts;
> > +       status = "okay";
> > +
> > +       bluetooth {
> > +               compatible = "brcm,bcm43438-bt";
> > +               clocks = <&rtc CLK_OSC32K_FANOUT>;
> > +               clock-names = "lpo";
> > +               vbat-supply = <&reg_dldo1>;
> > +               device-wakeup-gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>;
> > /* PL10 */
> > +               host-wakeup-gpios = <&r_pio 0 9 GPIO_ACTIVE_HIGH>; /*
> > PL9 */
> > +               shutdown-gpios = <&r_pio 0 8 GPIO_ACTIVE_HIGH>; /*
> > PL8 */
> > +       };
> > +};
> > +
> > +&usb_otg {
> > +       dr_mode = "otg";
> > +       status = "okay";
> > +};
> > +
> > +&usb_power_supply {
> > +       status = "okay";
> > +};
> > +
> > +&usbphy {
> > +       usb0_id_det-gpios = <&pio 7 8 GPIO_ACTIVE_HIGH>; /* PH8 */
> > +       usb0_vbus_power-supply = <&usb_power_supply>;
> > +       usb0_vbus-supply = <&reg_drivevbus>;
> > +       usb1_vbus-supply = <&reg_usb1_vbus>;
> > +       status = "okay";
> > +};  
> 

Powered by blists - more mailing lists