Re: [PATCH 2/2] arm64: dts: allwinner: h700: Add Anbernic RG35XX-SP

[Andre Przywara <andre.przywara@arm.com>]

On Thu, 6 Jun 2024 12:06:54 -0500
Chris Morgan <macromorgan@hotmail.com> wrote:

Hi,

> On Thu, Jun 06, 2024 at 01:35:15PM +0100, Andre Przywara wrote:
> > On Thu, 6 Jun 2024 19:58:07 +0800
> > Chen-Yu Tsai <wens@csie.org> wrote:
> > 
> > Hi Chen-Yu,
> >   
> > > On Thu, Jun 6, 2024 at 5:52 PM Andre Przywara <andre.przywara@arm.com> wrote:  
> > > >
> > > > On Wed,  5 Jun 2024 13:53:39 -0500
> > > > Chris Morgan <macroalpha82@gmail.com> wrote:
> > > >    
> > > > > From: Chris Morgan <macromorgan@hotmail.com>
> > > > >
> > > > > The Anbernic RG35XXSP is almost identical to the RG35XX-Plus, but in a
> > > > > clamshell form-factor. The key differences between the SP and the Plus
> > > > > is a lid switch and an RTC on the same i2c bus as the PMIC.
> > > > >
> > > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > > > > ---
> > > > >  arch/arm64/boot/dts/allwinner/Makefile        |   3 +-
> > > > >  .../sun50i-h700-anbernic-rg35xx-sp.dts        | 145 ++++++++++++++++++
> > > > >  2 files changed, 147 insertions(+), 1 deletion(-)
> > > > >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > > > > index 0db7b60b49a1..00bed412ee31 100644
> > > > > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > > > > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > > > > @@ -49,5 +49,6 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero2w.dtb
> > > > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-orangepi-zero3.dtb
> > > > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h618-transpeed-8k618-t.dtb
> > > > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-2024.dtb
> > > > > -dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
> > > > >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-h.dtb
> > > > > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-plus.dtb
> > > > > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h700-anbernic-rg35xx-sp.dtb
> > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > > > > new file mode 100644
> > > > > index 000000000000..a1d16b65ef5d
> > > > > --- /dev/null
> > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-sp.dts
> > > > > @@ -0,0 +1,145 @@
> > > > > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +/*
> > > > > + * Copyright (C) 2024 Ryan Walklin <ryan@testtoast.com>.
> > > > > + * Copyright (C) 2024 Chris Morgan <macroalpha82@gmail.com>.
> > > > > + */
> > > > > +
> > > > > +#include <dt-bindings/input/gpio-keys.h>
> > > > > +#include "sun50i-h700-anbernic-rg35xx-plus.dts"
> > > > > +
> > > > > +/ {
> > > > > +     model = "Anbernic RG35XX SP";
> > > > > +     compatible = "anbernic,rg35xx-sp", "allwinner,sun50i-h700";
> > > > > +
> > > > > +     gpio-keys-lid {
> > > > > +             compatible = "gpio-keys";
> > > > > +
> > > > > +             lid-switch {
> > > > > +                     label = "Lid Switch";
> > > > > +                     gpios = <&pio 4 7 GPIO_ACTIVE_LOW>; /* PE7 */
> > > > > +                     linux,can-disable;
> > > > > +                     linux,code = <SW_LID>;
> > > > > +                     linux,input-type = <EV_SW>;
> > > > > +                     wakeup-event-action = <EV_ACT_DEASSERTED>;
> > > > > +                     wakeup-source;
> > > > > +             };
> > > > > +     };
> > > > > +};
> > > > > +
> > > > > +/delete-node/ &r_rsb;    
> > > >
> > > > I don't think deleting the *RSB* node is right here, if at all, I'd expect
> > > > status="disabled", and then maybe deleting the axp717 node within?
> > > > Or maybe factor the AXP node out into a separate file, and include it from
> > > > the -2024.dts and from this one?    
> > > 
> > > Doesn't quite work because the unit address is different.  
> > 
> > Ah, right, good point. It's a bit annoying because the node name itself is
> > mostly irrelevant, but I see that this wouldn't pass validation.
> >   
> > > > Or move every board to I2C?    
> > > 
> > > Doesn't this depend on the bootloader also running in RSB mode? My memory
> > > is a bit foggy on this, but IIRC we did a conversion on some other boards
> > > before?  
> > 
> > In the SPL we use I2C only, mostly because we have an SPL capable I2C
> > driver already.
> > In U-Boot proper we use whatever the DT says, that should work like in the
> > kernel.
> > In TF-A we used to have one hardcoded transport per SoC, and that happens
> > to be RSB everywhere at the moment, but I have a patch series [1] to
> > determine this dynamically, via the DT. As it stands, that chooses the
> > transport by looking at the PMIC (I2C for the AXP313, RSB for the AXP717
> > or AXP305), but I think it's fairly easy to check for the status property
> > of both RSB and I2C, or look at the parent node of the AXP node to find
> > the transport protocol to use.
> > I will sketch something up.
> > 
> > Chris has plans to auto-detect the exact Anbernic model in U-Boot, and
> > switch to the right DT then automatically. IIUC I2C devices play a role in
> > this, so switching to I2C for all Anbernic models would make this more
> > viable.
> > It just leaves a bit of a bitter taste that we now model the DT after this
> > particular requirement, and not after what the hardware looks like.  
> 
> I honestly would rather simply have the Linux tree use i2c for all
> devices, to be honest. For at least the Anbernic RG35XXSP device
> as well as a device I haven't started yet (the RG28XX) there is an
> external rtc on the same i2c bus as the PMIC at 0x51. This device
> does not appear to be present on the other devices.
> 
> I'm thinking that with U-Boot I can use a single bootloader and load
> one of five device trees based on what happens when I check for this
> RTC at 0x51, when I attempt to communicate with mmc1 (wifi), and when
> I check GPIO PE11. I don't need to actually use the wifi in U-Boot,
> just find out if it's there.
> 
> This means basically:
> 
> GPIO PE11 - RG35XX-H
> RTC present, wifi present - RG35XX-SP
> RTC present, wifi missing - RG28XX
> RTC missing, wifi missing - RG35XX-2024
> RTC missing, wifi present - RG35XX-Plus
> 
> Regardless of this though, I will find a way to manage no matter what
> we decide here. But for the RG35XX-SP and RG28XX (not yet submitted) I
> want to run the PMIC off i2c so I can use the external RTC too.

Yes, if you have I2C devices connected to those pins, then we *need* to
use I2C for the PMIC. We have the same situation for the Pine H64 board,
check sun50i-h6-pine-h64.dts.
That's why the idea to switch the others, too.

Cheers,
Andre

> > 
> > [1] https://review.trustedfirmware.org/q/topic:%22h616_pmics%22
> >   
> > > > What do people think about this?    
> > > 
> > > "disabled" in RSB node and deleting the axp717 node is probably the right
> > > thing to do.
> > > 
> > >   
> > > > Cheers,
> > > > Andre
> > > >    
> > > > > +
> > > > > +&r_i2c {
> > > > > +     pinctrl-0 = <&r_i2c_pins>;
> > > > > +     pinctrl-names = "default";
> > > > > +     status = "okay";
> > > > > +
> > > > > +     axp717: pmic@34 {
> > > > > +             compatible = "x-powers,axp717";
> > > > > +             reg = <0x34>;
> > > > > +             interrupt-controller;
> > > > > +             #interrupt-cells = <1>;
> > > > > +             interrupt-parent = <&nmi_intc>;
> > > > > +             interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > > > +
> > > > > +             vin1-supply = <&reg_vcc5v>;
> > > > > +             vin2-supply = <&reg_vcc5v>;
> > > > > +             vin3-supply = <&reg_vcc5v>;
> > > > > +             vin4-supply = <&reg_vcc5v>;
> > > > > +
> > > > > +             regulators {
> > > > > +                     reg_dcdc1: dcdc1 {
> > > > > +                             regulator-always-on;
> > > > > +                             regulator-min-microvolt = <900000>;
> > > > > +                             regulator-max-microvolt = <1100000>;
> > > > > +                             regulator-name = "vdd-cpu";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_dcdc2: dcdc2 {
> > > > > +                             regulator-always-on;
> > > > > +                             regulator-min-microvolt = <940000>;
> > > > > +                             regulator-max-microvolt = <940000>;
> > > > > +                             regulator-name = "vdd-gpu-sys";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_dcdc3: dcdc3 {
> > > > > +                             regulator-always-on;
> > > > > +                             regulator-min-microvolt = <1100000>;
> > > > > +                             regulator-max-microvolt = <1100000>;
> > > > > +                             regulator-name = "vdd-dram";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_aldo1: aldo1 {
> > > > > +                             /* 1.8v - unused */
> > > > > +                     };    
> > > 
> > > You can drop all the unused ones, unless you plan to include *this*
> > > file from another one and use those nodes/labels.
> > > 
> > > ChenYu
> > >   
> > > > > +
> > > > > +                     reg_aldo2: aldo2 {
> > > > > +                             /* 1.8v - unused */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_aldo3: aldo3 {
> > > > > +                             /* 1.8v - unused */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_aldo4: aldo4 {
> > > > > +                             regulator-min-microvolt = <1800000>;
> > > > > +                             regulator-max-microvolt = <1800000>;
> > > > > +                             regulator-name = "vcc-pg";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_bldo1: bldo1 {
> > > > > +                             /* 1.8v - unused */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_bldo2: bldo2 {
> > > > > +                             regulator-always-on;
> > > > > +                             regulator-min-microvolt = <1800000>;
> > > > > +                             regulator-max-microvolt = <1800000>;
> > > > > +                             regulator-name = "vcc-pll";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_bldo3: bldo3 {
> > > > > +                             /* 2.8v - unused */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_bldo4: bldo4 {
> > > > > +                             /* 1.2v - unused */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_cldo1: cldo1 {
> > > > > +                             /* 3.3v - audio codec - not yet implemented */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_cldo2: cldo2 {
> > > > > +                             /* 3.3v - unused */
> > > > > +                     };
> > > > > +
> > > > > +                     reg_cldo3: cldo3 {
> > > > > +                             regulator-always-on;
> > > > > +                             regulator-min-microvolt = <3300000>;
> > > > > +                             regulator-max-microvolt = <3300000>;
> > > > > +                             regulator-name = "vcc-io";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_cldo4: cldo4 {
> > > > > +                             regulator-min-microvolt = <3300000>;
> > > > > +                             regulator-max-microvolt = <3300000>;
> > > > > +                             regulator-name = "vcc-wifi";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_boost: boost {
> > > > > +                             regulator-min-microvolt = <5000000>;
> > > > > +                             regulator-max-microvolt = <5200000>;
> > > > > +                             regulator-name = "boost";
> > > > > +                     };
> > > > > +
> > > > > +                     reg_cpusldo: cpusldo {
> > > > > +                             /* unused */
> > > > > +                     };
> > > > > +             };
> > > > > +     };
> > > > > +
> > > > > +     rtc_ext: rtc@51 {
> > > > > +             compatible = "nxp,pcf8563";
> > > > > +             reg = <0x51>;
> > > > > +     };
> > > > > +};    
> > > >
> > > >    
> >