Hi Alexander,

On Mon 18 May 26, 22:05, Alexander Sverdlin wrote:
> Hi Paul,
> 
> thanks for the review!
> 
> On Mon, 2026-05-18 at 13:52 +0200, Paul Kocialkowski wrote:
> > 
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a133-baije-core.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a133-baije-core.dtsi
> > > new file mode 100644
> > > index 000000000000..65b094f30bf5
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a133-baije-core.dtsi
> 
> []
> 
> > You should add:
> > 
> > 	chosen {
> > 		stdout-path = "serial0:115200n8";
> > 	};
> 
> I actually have it in .dts, but it's theoretically possible to deploy
> the core board in a way that serial0 is *not* a console, so the above
> probably will not be valid in all cases in .dtsi.

Yes I figured out later that it was in the board dts file and I initially
assumed it was missing entirely.

In practice both options are fine, although the reference software does
hardcode UART0 as debug serial.

> > > +&reg_dcdc2 {
> > > +	regulator-always-on;
> > > +	regulator-min-microvolt = <500000>;
> > > +	regulator-max-microvolt = <1300000>;
> > 
> > Should be:
> > 	regulator-min-microvolt = <900000>;
> > 	regulator-max-microvolt = <1300000>;
> 
> 0.81..1.2v according to A133 Datasheet Revision 1.1 Jul.14, 2020?

I guess the initial values are taken from the allwinner-perf1 board dts.

The 900 mV-1.3 V range matches the CPU OPPs (although it really only goes up
to 1.13 V). Maybe down to 810 mV does work, but we don't have an OPP for it.
I think I took these values from the reference BSP for the board.

Also it would be good to add:

	regulator-name = "vdd-cpux";

> > 
> > > +&reg_dcdc4 {
> > > +	regulator-always-on;
> > > +	regulator-min-microvolt = <500000>;
> > > +	regulator-max-microvolt = <1300000>;
> > > +	regulator-name = "vdd-sys";
> > 
> > Should be:
> > 	regulator-min-microvolt = <810000>;
> > 	regulator-max-microvolt = <990000>;
> > 	regulator-name = "vcc-usb-sys";
> 
> I'm a bit puzzled here: datasheet says 0.9..1.0v
> and it has no "Typ" value, similar to VDD_CPU, but
> VDD_SYS is not part of OPP tables, so who is going
> to adjust this? Or shall it be just
> 
> regulator-min-microvolt = <950000>;
> regulator-max-microvolt = <950000>;
> 
> ?

Yes the reference BSP runs it at 950 mV, LGTM.

> 
> > 
> > > +};
> > > +
> > > +&reg_dcdc5 {
> > > +	regulator-always-on;
> > > +	regulator-min-microvolt = <800000>;
> > > +	regulator-max-microvolt = <1840000>;
> > > +	regulator-name = "vcc-dram";
> > 
> > Should be:
> > 	regulator-min-microvolt = <1100000>;
> > 	regulator-max-microvolt = <1100000>;
> > 	regulator-name = "vcc-dram-2";
> > 
> > ALDO2 is the main DRAM supply, this is the second one.
> 
> Core schematics mentions 1.1V/1.2/1.35/1.5 on this rail...
> Currently U-Boot has CONFIG_AXP_DCDC5_VOLT=1100, but potentially
> this is adjustable, right? At some point LPDDR4 chips they
> are soldering today will be unavailable. And in the current
> market it will happen rather sooner than later...

It is part of the LPDDR4 spec that the main voltage should be 1.8 V and
the second and I/O buffer ones should be 1.1 V. See JESD209-4D Table 180 —
Recommended DC Operating Conditions.

Maybe they jsut copied this comment from a reference design that allows for
other types of DRAM too. In any case their BSP hardcodes 1.1 V anyway.

> > 
> > > +};
> > > +
> > > +/* DCDC6 unused */
> > > +
> > > +&reg_dldo1 {
> > > +	regulator-min-microvolt = <700000>;
> > > +	regulator-max-microvolt = <3300000>;
> > > +	regulator-enable-ramp-delay = <1000>;
> > 
> > Should be:
> > 	regulator-min-microvolt = <1800000>;
> > 	regulator-max-microvolt = <1800000>;
> > 	regulator-name = "vcc-pg";
> 
> Do suggest to drop vendor's
> 
> regulator-enable-ramp-delay = <1000>;
> 
> in all cases?

Well we generally don't have the delays in the axp regulator definitions and
it works well without them, but I guess they don't hurt either.

In practice many drivers will have a delay after a regulator power on anyway
because we generally expect that hardware needs some time to power up,
in addition to the regulator. So all in all it's rarely critical.

> 
> > > 
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a133-baijie-helper.dts b/arch/arm64/boot/dts/allwinner/sun50i-a133-baijie-helper.dts
> > > new file mode 100644
> > > index 000000000000..ccbca5d0a40c
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a133-baijie-helper.dts
> 
> []
> 
> > > +	aliases {
> > > +		serial0 = &uart0;
> > 
> > The is best added to the core dtsi.
> > 
> > > +	};
> > > +
> > > +	chosen {
> > > +		stdout-path = "serial0:115200n8";
> > 
> > Ditto.
> 
> But it only physically materializes in Helperboard, the carrier.
> Potentially this one can be left floating or used for something else.

Yes fair enough, I'm happy with having it on the helperboard dts file.

All the best,

Paul

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.