On Tue, Aug 22, 2023 at 12:17:39AM +0100, Andre Przywara wrote:
> On Mon, 21 Aug 2023 12:43:13 -0500
> Chris Morgan <macromorgan@xxxxxxxxxxx> wrote:
>
> Hi,
>
> > On Mon, Aug 21, 2023 at 06:06:07PM +0100, Andre Przywara wrote:
> > > On Mon, 21 Aug 2023 10:38:38 -0500
> > > Chris Morgan <macromorgan@xxxxxxxxxxx> wrote:
> > >
> > > Hi Chris,
> > >
> > > > On Sat, Aug 19, 2023 at 11:03:52PM +0100, Andre Przywara wrote:
> > > > > On Fri, 18 Aug 2023 22:21:05 -0500
> > > > > Chris Morgan <macroalpha82@xxxxxxxxx> wrote:
> > > > >
> > > > > Hi Chris,
> > > > >
> > > > > thanks for the update!
> > > > >
> > > > > > From: Chris Morgan <macromorgan@xxxxxxxxxxx>
> > > > > >
> > > > > > The Anbernic RG-Nano is a small portable game device based on the
> > > > > > Allwinner V3s SoC. It has GPIO buttons on the face and side for
> > > > > > input, a single mono speaker, a 240x240 SPI controlled display, a USB-C
> > > > > > OTG port, an SD card slot for booting, and 64MB of RAM included in the
> > > > > > SoC.
> > > > > >
> > > > > > The SPI display is currently unsupported, as it will either require
> > > > > > a new tinydrm driver or changes to the staging fbtft driver to support.
> > > > > > I plan on working on a tinydrm driver to properly support it. The USB-C
> > > > > > port currently only works as a peripheral port, however in the BSP
> > > > > > kernel it also works in host mode allowing included USB-C headphones
> > > > > > with a built-in DAC to work.
> > > > > >
> > > > > > Working:
> > > > > > - SDMMC
> > > > > > - UART (for debugging)
> > > > > > - Buttons
> > > > > > - Charging/battery/PMIC
> > > > > > - Speaker
> > > > > > - USB Gadget
> > > > > >
> > > > > > Not working:
> > > > > > - Display
> > > > > > - USB Host
> > > > > >
> > > > > > Signed-off-by: Chris Morgan <macromorgan@xxxxxxxxxxx>
> > > > > > ---
> > > > > > arch/arm/boot/dts/allwinner/Makefile | 1 +
> > > > > > .../allwinner/sun8i-v3s-anbernic-rg-nano.dts | 219 ++++++++++++++++++
> > > > > > 2 files changed, 220 insertions(+)
> > > > > > create mode 100644 arch/arm/boot/dts/allwinner/sun8i-v3s-anbernic-rg-nano.dts
> > > > > >
> > > > > > diff --git a/arch/arm/boot/dts/allwinner/Makefile b/arch/arm/boot/dts/allwinner/Makefile
> > > > > > index 589a1ce1120a..2be83a1edcbb 100644
> > > > > > --- a/arch/arm/boot/dts/allwinner/Makefile
> > > > > > +++ b/arch/arm/boot/dts/allwinner/Makefile
> > > > > > @@ -237,6 +237,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
> > > > > > sun8i-t113s-mangopi-mq-r-t113.dtb \
> > > > > > sun8i-t3-cqa3t-bv3.dtb \
> > > > > > sun8i-v3-sl631-imx179.dtb \
> > > > > > + sun8i-v3s-anbernic-rg-nano.dtb \
> > > > > > sun8i-v3s-licheepi-zero.dtb \
> > > > > > sun8i-v3s-licheepi-zero-dock.dtb \
> > > > > > sun8i-v40-bananapi-m2-berry.dtb
> > > > > > diff --git a/arch/arm/boot/dts/allwinner/sun8i-v3s-anbernic-rg-nano.dts b/arch/arm/boot/dts/allwinner/sun8i-v3s-anbernic-rg-nano.dts
> > > > > > new file mode 100644
> > > > > > index 000000000000..c49b5431d04e
> > > > > > --- /dev/null
> > > > > > +++ b/arch/arm/boot/dts/allwinner/sun8i-v3s-anbernic-rg-nano.dts
> > > > > > @@ -0,0 +1,219 @@
> > > > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > > > +
> > > > > > +/dts-v1/;
> > > > > > +#include <dt-bindings/input/linux-event-codes.h>
> > > > > > +#include "sun8i-v3s.dtsi"
> > > > > > +#include "sunxi-common-regulators.dtsi"
> > > > > > +
> > > > > > +/ {
> > > > > > + model = "Anbernic RG Nano";
> > > > > > + compatible = "anbernic,rg-nano", "allwinner,sun8i-v3s";
> > > > > > +
> > > > > > + aliases {
> > > > > > + serial0 = &uart0;
> > > > > > + };
> > > > > > +
> > > > > > + backlight: backlight {
> > > > > > + compatible = "pwm-backlight";
> > > > > > + pwms = <&pwm 0 40000 1>;
> > > > > > + brightness-levels = <0 1 2 3 8 14 21 32 46 60 80 100>;
> > > > > > + default-brightness-level = <11>;
> > > > > > + power-supply = <®_vcc5v0>;
> > > > > > + };
> > > > > > +
> > > > > > + chosen {
> > > > > > + stdout-path = "serial0:115200n8";
> > > > > > + };
> > > > > > +
> > > > > > + gpio_keys: gpio-keys {
> > > > > > + compatible = "gpio-keys";
> > > > > > +
> > > > > > + button-a {
> > > > > > + gpios = <&gpio_expander 12 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> > > > > > + label = "BTN-A";
> > > > > > + linux,code = <BTN_EAST>;
> > > > > > + };
> > > > > > +
> > > > > > + button-b {
> > > > > > + gpios = <&gpio_expander 14 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> > > > > > + label = "BTN-B";
> > > > > > + linux,code = <BTN_SOUTH>;
> > > > > > + };
> > > > > > +
> > > > > > + button-down {
> > > > > > + gpios = <&gpio_expander 1 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> > > > > > + label = "DPAD-DOWN";
> > > > > > + linux,code = <BTN_DPAD_DOWN>;
> > > > > > + };
> > > > > > +
> > > > > > + button-left {
> > > > > > + gpios = <&gpio_expander 4 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> > > > > > + label = "DPAD-LEFT";
> > > > > > + linux,code = <BTN_DPAD_LEFT>;
> > > > > > + };
> > > > > > +
> > > > > > + button-right {
> > > > > > + gpios = <&gpio_expander 0 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> > > > > > + label = "DPAD-RIGHT";
> > > > > > + linux,code = <BTN_DPAD_RIGHT>;
> > > > > > + };
> > > > > > +
> > > > > > + button-se {
> > > > > > + gpios = <&gpio_expander 7 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> > > > > > + label = "BTN-SELECT";
> > > > > > + linux,code = <BTN_SELECT>;
> > > > > > + };
> > > > > > +
> > > > > > + button-st {
> > > > > > + gpios = <&gpio_expander 6 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> > > > > > + label = "BTN-START";
> > > > > > + linux,code = <BTN_START>;
> > > > > > + };
> > > > > > +
> > > > > > + button-tl {
> > > > > > + gpios = <&gpio_expander 2 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> > > > > > + label = "BTN-L";
> > > > > > + linux,code = <BTN_TL>;
> > > > > > + };
> > > > > > +
> > > > > > + button-tr {
> > > > > > + gpios = <&gpio_expander 15 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> > > > > > + label = "BTN-R";
> > > > > > + linux,code = <BTN_TR>;
> > > > > > + };
> > > > > > +
> > > > > > + button-up {
> > > > > > + gpios = <&gpio_expander 3 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> > > > > > + label = "DPAD-UP";
> > > > > > + linux,code = <BTN_DPAD_UP>;
> > > > > > + };
> > > > > > +
> > > > > > + button-x {
> > > > > > + gpios = <&gpio_expander 11 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> > > > > > + label = "BTN-X";
> > > > > > + linux,code = <BTN_NORTH>;
> > > > > > + };
> > > > > > +
> > > > > > + button-y {
> > > > > > + gpios = <&gpio_expander 13 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
> > > > > > + label = "BTN-Y";
> > > > > > + linux,code = <BTN_WEST>;
> > > > > > + };
> > > > > > + };
> > > > > > +};
> > > > > > +
> > > > > > +&codec {
> > > > > > + allwinner,audio-routing = "Speaker", "HP",
> > > > > > + "MIC1", "Mic",
> > > > > > + "Mic", "HBIAS";
> > > > > > + allwinner,pa-gpios = <&pio 5 6 (GPIO_ACTIVE_HIGH | GPIO_PULL_UP)>;
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&cpu0 {
> > > > > > + clock-frequency = <1296000>;
> > > > >
> > > > > I understand that the kernel complains when this is missing, but I
> > > > > think this property is some ancient legacy, as there is no such thing
> > > > > as a fixed CPU frequency anymore. That becomes evident with the OPPs
> > > > > below. So please remove it.
> > > >
> > > > Okay, I'll remove it. I always try to make sure dmesg log is as error
> > > > free as possible, and yes, that's the only reason I put this here.
> > >
> > > Yeah, I hear you. I actually pushed
> > > https://github.com/devicetree-org/devicetree-specification/commit/c2cdd4a0db1d79f
> > > into the DT spec, so we have some leg to stand on when trying to remove
> > > the kernel message. Just need to find the time to make this patch ...
> >
> > Good to know. I'll drop this then. Thank you.
> >
> > >
> > > > >
> > > > > > + clock-latency = <244144>;
> > > > > > + operating-points = <
> > > > > > + /* kHz uV */
> > > > > > + 1296000 1200000
> > > > > > + 1008000 1200000
> > > > > > + 864000 1200000
> > > > > > + 720000 1100000
> > > > > > + 480000 1000000>;
> > > > >
> > > > > Don't you need a cpu-supply property to point to the regulator in
> > > > > charge of providing the core voltage? Otherwise I don't see how the
> > > > > kernel would be able to adjust the voltage, to program the OPPs.
> > > > >
> > > >
> > > > I'll add cpu-supply. Based on the schematic it looks like the
> > > > cpu_supply is the DCDC2. DCDC2 is also hooked up to some other inactive
> > > > pins according to the schematic but I can't see those pins on the
> > > > SoC datasheet (so maybe it's just a schematic quirk?).
> > >
> > > Mmmh, what are the names of those pins?
> > >
> > > In general I wonder if you should enable DVFS if the vendor firmware
> > > doesn't do that. Especially for a retro gaming device, I wonder if some
> > > code relies on the CPU running at the same frequency all the time, so that
> > > timing loops stay stable, and other real-time properties are still met.
> > >
> > > On the other hand, without DVFS the chip will run at at constant 1.08GHz
> > > (as set up by mainline U-Boot), which might leave some performance on the
> > > table. And it might affect the runtime, since this is battery powered. What
> > > frequency does the BSP firmware run with?
> >
> > According to the schematic the names are vdd-sys[n] where n is between
> > 0 and 5, as well as ephy-vdd. I see in the datasheet there are 6
> > different vdd-sys pins, so I assume this is those.
> >
> > BSP U-Boot looks to be basically mainline forked from 2020.10, and
> > doesn't set any values for the CPU clock (suggesting whatever default
> > U-Boot uses today for the v3s is what it's running at).
> >
> > I'll just drop all this for today and we can iterate on it later if it
> > is needed.
>
> Fair enough! Out of interest, what is the voltage for DCDC2, when you
> boot into Linux? U-Boot's default for the AXP209 DCDC2 is 1.4V, which
> is too high. The Pinecube sets it to 1.25V, which sounds more
> reasonable.
> So given your OPPs mentioned above, can you make the range 1.0-1.2V in
> the DT, and use CONFIG_AXP_DCDC2_VOLT=1200 in U-Boot's defconfig?
> You could still put the cpu-supply property in the DT, but comment it
> out, with a comment about this being untested and potentially
> problematic due to the shared supply.
> I think this should be the best between describing the hardware and
> sticking to the (better tested) BSP behaviour.
>
> Alternatively you could add the OPPs above, but use 1.2V everywhere,
> this would give you the opportunity to adjust the frequency, without
> disturbing VDD_SYS. And add the real voltages in a comment.
I actually see that the value isn't set in U-Boot at all, which is
interesting. I guess it will be what we make it, and in this case I'd
like to follow the schematic I guess at 1.25v. I'll just set it at that
and make sure when I do the U-Boot porting I set it correctly there
too.
>
> > >
> > > > > > +};
> > > > > > +
> > > > > > +&i2c0 {
> > > > > > + status = "okay";
> > > > > > +
> > > > > > + gpio_expander: gpio@20 {
> > > > > > + compatible = "nxp,pcal6416";
> > > > > > + reg = <0x20>;
> > > > > > + gpio-controller;
> > > > > > + #gpio-cells = <2>;
> > > > > > + #interrupt-cells = <2>;
> > > > > > + interrupt-controller;
> > > > > > + interrupt-parent = <&pio>;
> > > > > > + interrupts = <1 3 IRQ_TYPE_EDGE_BOTH>;
> > > > > > + vcc-supply = <®_vcc3v3>;
> > > > > > + };
> > > > > > +
> > > > > > + axp209: pmic@34 {
> > > > > > + reg = <0x34>;
> > > > > > + interrupt-parent = <&pio>;
> > > > > > + interrupts = <1 5 IRQ_TYPE_EDGE_FALLING>;
> > > > > > + };
> > > > > > +
> > > > > > + pcf8563: rtc@51 {
> > > > > > + compatible = "nxp,pcf8563";
> > > > > > + reg = <0x51>;
> > > > > > + };
> > > > > > +};
> > > > > > +
> > > > > > +#include "axp209.dtsi"
> > > > > > +
> > > > > > +&battery_power_supply {
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&mmc0 {
> > > > > > + broken-cd;
> > > > > > + bus-width = <4>;
> > > > > > + disable-wp;
> > > > > > + vmmc-supply = <®_vcc3v3>;
> > > > > > + vqmmc-supply = <®_vcc3v3>;
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&pio {
> > > > > > + vcc-pb-supply = <®_vcc3v3>;
> > > > > > + vcc-pc-supply = <®_vcc3v3>;
> > > > > > + vcc-pf-supply = <®_vcc3v3>;
> > > > > > + vcc-pg-supply = <®_vcc3v3>;
> > > > > > +};
> > > > > > +
> > > > > > +&pwm {
> > > > > > + pinctrl-0 = <&pwm0_pins>;
> > > > > > + pinctrl-names = "default";
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +®_dcdc2 {
> > > > >
> > > > > Any reason your dropped the regulator-name property? This would help to
> > > > > identify what this regulator is used for and would explain why it needs
> > > > > to be always on.
> > > > > In v1 this was "vdd-cpu-sys-ephy", are you sure about this name? I
> > > > > guess it supplies the CPU, but I wonder if there are other users of
> > > > > this rail, which would possibly throw a spanner into DVFS. So what's the
> > > > > "ephy" doing here? And can you adjust the voltage, if this also driving
> > > > > VDD-SYS? What does the BSP kernel do?
> > > > >
> > > >
> > > > This rail appears to supply the CPU (vdd-cpu) SYS (vdd-sys) and
> > > > vdd-ephy. The ratings for each of these according to the datasheet are
> > > > between -0.3v and 1.3v (CPU/SYS) or 1.4v (for the EPHY). The BSP kernel
> > > > has this as variable between 1v and 1.25v, but it doesn't appear to
> > > > have DVFS enabled. I can change the range back to 1/1.25v since it
> > > > supplies the CPU so we can enable DVFS; the datasheet isn't clear
> > > > what vdd-sys or vdd-ephy do, but in the case of EPHY we aren't using
> > > > that feature at least (and these voltages are within range).
> > >
> > > Right, if the board doesn't use the EPHY, then the voltage changing
> > > shouldn't matter, as long as it's within range.
> > > As for VDD-SYS: I am not so sure about this, I only seem to see GPU
> > > and SYS coupled together (in the mainline DTs), but not CPU and SYS.
> > > And VDD_SYS having the same range is one thing, but its voltage changing
> > > at runtime is another topic: I don't really know if the chips can cope
> > > with that, or if the system become unstable.
> > >
> > > > Do you think I should keep the cpu-supply and frequency stuff in place,
> > > > or should I drop it all instead?
> > >
> > > If the BSP doesn't use that, I'd rather drop the DVFS nodes, or maybe
> > > deactivate them, so users can bring them back at their own discretion?
> > >
> >
> > I'll just drop it for the moment and keep it simple.
> >
> > > > > > + regulator-always-on;
> > > > > > + regulator-max-microvolt = <1250000>;
> > > > > > + regulator-min-microvolt = <1250000>;
> > > > > > +};
> > > > > > +
> > > > > > +®_dcdc3 {
> > > > >
> > > > > Same here, please provide a speaking name, or a comment explaining
> > > > > why this must be always on. There is no need to enumerate every user,
> > > > > just "vdd-3v3" seems to be a common name for that regulator.
> > > > >
> > > >
> > > > I'll go with vcc-io like the BSP, unless you feel strongly otherwise.
> > > > The regulator appears to power all the different IO pins on the SoC
> > > > and basically everything else that uses 3.3v that isn't the RTC.
> > > >
> > > > > > + regulator-always-on;
> > > > > > + regulator-max-microvolt = <3300000>;
> > > > > > + regulator-min-microvolt = <3300000>;
> > > > > > +};
> > > > > > +
> > > > > > +®_ldo2 {
> > > > >
> > > > > Same here, name or comment, please.
> > > > >
> > > >
> > > > You got it.
> > > >
> > > > > > + regulator-always-on;
> > > > > > + regulator-max-microvolt = <3000000>;
> > > > > > + regulator-min-microvolt = <3000000>;
> > > > > > +};
> > > > > > +
> > > > > > +&spi0 {
> > > > >
> > > > > Can you add a comment here mentioning the not-yet-supported display?
> > > > > And you should specify the pins used here.
> > > > >
> > > >
> > > > The SPI pins are defined in the main v3s file.
> > >
> > > Ah, right, sorry, I missed that.
> > >
> > > > Additionally I have a
> > > > TE pin (GPIO PB1) and a RESET pin for the panel (GPIO PB2). The RESET
> > > > pin looks like it has an external pull-up to 3.3v. The RS pin on the
> > > > panel is wired to MISO, otherwise CS on the panel goes to CS on the
> > > > SoC, CLK to CLK, and MOSI to MOSI. Everything but the RESET panel
> > > > is pulled to ground (I think, still not perfect at schematics).
> > >
> > > Those other pins would be described in the (upcoming) panel DT node. The
> > > SPI pins should indeed be already activated, courtesy of pinctrl-0 in the
> > > .dtsi.
> >
> > >
> > > > > Cheers,
> > > > > Andre
> > > > >
> > > > >
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&uart0 {
> > > > > > + pinctrl-0 = <&uart0_pb_pins>;
> > > > > > + pinctrl-names = "default";
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&usb_otg {
> > > > > > + dr_mode = "otg";
> > >
> > > Just seeing this: if you have "otg" here, wouldn't you need to specify the
> > > ID pin (usb0_id_det-gpios) in the usbphy DT node? Because otherwise the
> > > kernel won't know when to switch between host and peripheral mode.
> > >
> >
> > I've added it for v3 and gotten OTG working fully now.
> >
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&usb_power_supply {
> > > > > > + status = "okay";
> > > > > > +};
> > > > > > +
> > > > > > +&usbphy {
> > > > > > + status = "okay";
> > > > > > +};
> > > > >
> > > >
> > > > Also, I did another comparison with the BSP kernel and realized I was
> > > > missing the OHCI/EHCI and now OTG is working (at least the extcon
> > > > shows host when it has a device attached and not host when it's hooked
> > > > to my computer).
> > >
> > > Wait, that's odd. What OHCI/EHCI? DT nodes? I don't find any in the V3s
> > > .dtsi, and normally you don't need them for OTG anyway.
> > > There is some oddity with the PHY switching code, but I think what
> > > should work is the OTG operation, with an ID pin.
> >
> > Check lines 254 and 263 of the decompiled BSP device tree. Basically if
> > I add an ohci and ehci node host mode works, but without it then it
> > doesn't.
>
> Ah, right, I mixed something up for a moment, looking again this should
> be the story:
> The V3s USB PHY is a dual-route PHY (as set in the Linux driver for
> that compatible string), so the driver will try to switch to the HCI
> controller pair for host mode, and will not try the MUSB's host mode.
> Curiously we don't have the OHCI/EHCI nodes in our sun8i-v3s.dtsi, I
> wonder why? Was that just not needed so far, because the USB port was
> always peripheral only?
>
> Icenowy, can you shed some light on this?
> It looks like the V3s is like the first USB port of the H3, so we
> should have both an HCI pair, plus the OTG node?
>
> Cheers,
> Andre
>
> >
> > https://gist.github.com/macromorgan/e1f9e8e2275ae7e53c45fa864148ffed#file-sun8i-v3s-anbernic-dts-L254
>
>
> >
> > >
> > > Cheers,
> > > Andre
> >
> > Thank you,
> > Chris
> >
>
