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@xxxxxxxx> wrote:
>
> Hi Chen-Yu,
>
> > On Thu, Jun 6, 2024 at 5:52 PM Andre Przywara <andre.przywara@xxxxxxx> wrote:
> > >
> > > On Wed, 5 Jun 2024 13:53:39 -0500
> > > Chris Morgan <macroalpha82@xxxxxxxxx> wrote:
> > >
> > > > From: Chris Morgan <macromorgan@xxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxx>.
> > > > + * Copyright (C) 2024 Chris Morgan <macroalpha82@xxxxxxxxx>.
> > > > + */
> > > > +
> > > > +#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.
Thank you,
Chris.
>
> 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 = <®_vcc5v>;
> > > > + vin2-supply = <®_vcc5v>;
> > > > + vin3-supply = <®_vcc5v>;
> > > > + vin4-supply = <®_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>;
> > > > + };
> > > > +};
> > >
> > >
>
