[RFC PATCH 00/17] sunxi: rework pinctrl and add T113s support

[Andre Przywara]

On Fri, 16 Jun 2023 19:27:16 +0300
Maxim Kiselev <bigunclemax at gmail.com> wrote:

Hi Maxim,

thanks for the reply! If you have anything that is missing or broken in
the new version of the patchset I put on github, please let me know.

> пт, 16 июн. 2023 г. в 18:59, Andre Przywara <andre.przywara at arm.com>:
> >
> > On Mon, 12 Jun 2023 15:18:17 -0600
> > Sam Edwards <cfsworks at gmail.com> wrote:
> >
> > Hi Sam,
> >
> > something regarding "reset" below ...
> >  
> > > On 6/11/23 18:20, Andre Przywara wrote:  
> > > > Thanks for the update and the list! Can you confirm where you
> > > > still needed code changes compared to say my github branch plus the
> > > > changes we already discussed? Trying some guesses below, please confirm
> > > > or deny:  
> > >
> > > Preeeeetttttyyy much everything I've changed locally has been submitted
> > > to the list or discussed in the relevant patchset. Have you updated your
> > > GitHub branch recently (past couple of weeks)? I haven't been watching
> > > it but I can pull it again and see which of my local changes are still
> > > required.
> > >  
> > > >> I have a build of U-Boot for my target, complete with:
> > > >> - UART3 initialized correctly  
> > > >
> > > > this is problematic because of the other pinmux used on your board,
> > > > which cannot easily be encoded alongside the existing UART3 pinmux?  
> > >
> > > Actually no, my board's UART3 is on PB6/PB7, nice and standard.
> > >  
> > > >> - DRAM coming up correctly
> > > >> - SPL sets configured boot clock correctly  
> > > >
> > > > This should work as per github?  
> > >
> > > Yep, everything was working satisfactorily once I figured out I needed
> > > to set CONFIG_SYS_CLK_FREQ to get acceptable boot speeds.
> > >  
> > > >> - SPI-NAND support (SPL and U-Boot proper)  
> > > >
> > > > This is with Icenow's series? Any D1 specific changes needed there?  
> > >
> > > Yes, with Icenowy's series (322733).
> > >
> > > I learned that the BROM sets the boot medium code to 0x04 when it's an
> > > SPI-*NAND* (while older chips used 0x03 for "it's either SPI-NOR or
> > > SPI-NAND, good luck figuring out which"). Since `env_get_location`
> > > assumes BOOT_DEVICE_SPI is NOR (and my target needs to load env from UBI
> > > iff booting from NAND), I'm hoping I can convince Icenowy to separate
> > > out the SPI-NAND and SPI-NOR load methods entirely (vs. her current
> > > try-NAND-then-NOR approach) with the aid of some disambiguation logic to
> > > probe for an SPI-NAND on the older chips known to report these as 0x03.
> > >
> > > I also needed Maksim's patch series (355747) to support the D1 SPI master.
> > >  
> > > >> - MMC support (SPL and U-Boot proper)
> > > >> - SPL boot from FEL  
> > > >
> > > > again worked already in github?  
> > >
> > > Yes and yes. I was just confirming they're good; no local work needed
> > > from me here.
> > >  
> > > >> - USB gadget support  
> > > >
> > > > So with the fixed SUNXI_SRAMC_BASE you said it worked? What about the
> > > > USB PHY? That needs at least wiring in the compatible string? If you
> > > > have such a patch, can you please rebase it on top of my v2 USB PHY
> > > > series and post that?  
> > >
> > > Yes, with corrected SUNXI_SRAMC_BASE -- though I also needed my patches
> > > to make musb-new/sunxi.c use the UDC gadget model in DM (series 358842),
> > > as I don't think there's another way to init the controller at runtime.
> > >
> > > I can't say whether the endpoint limit is correct or that mUSB *host*
> > > operation works.
> > >
> > > The USB PHY only required that CONFIG_PHY_SUN4I_USB be enabled. The
> > > correct compatible is already wired up. It does look like your PHY
> > > series drops the explicit requirement to set PHY_SUN4I_USB so that's
> > > better than what I was doing (adding a `select` directive under R528).
> > >
> > > I can test your series if you want but I doubt it intersects my work
> > > here in any significant way.
> > >  
> > > >> - Ethernet MAC+PHY support  
> > > >
> > > > Anything surprising here? Is that using an already supported external
> > > > PHY?  
> > >
> > > The only surprise was this was how I noticed that CONFIG_CLK_SUN20I_D1
> > > was not being implicitly enabled. Enabling that was then how I found
> > > that the clock driver wasn't compatible with current upstream (which I
> > > already mentioned).
> > >
> > > The PHY is external and already supported, adding it to my DT required
> > > very little work.
> > >  
> > > >> - I²C support *
> > > >> - GPIO support (LEDs, buttons, misc. board management)  
> > > >
> > > > again should work out of the box, minus your board specific
> > > > configuration?  
> > >
> > > GPIO is completely OotB, yes. I²C is OotB once MVTWSI_CONTROL_CLEAR_IFLG
> > > is set correctly. (The pinctrl requirements for it are a little harder,
> > > more on that below.)
> > >  
> > > >> - `reset` working (requries CONFIG_SYSRESET unset, WDT key)  
> > > >
> > > > Isn't "CONFIG_SYSRESET unset" a hack? I dimly remember we had this for
> > > > some other SoC initially, but later got rid of it?  
> > >
> > > Unsetting it is required for reset_cpu() to be defined. Your patchset
> > > updates that function (albeit without adding the WDT key, so the current
> > > patch is broken) to support NCAT2 already. U-Boot has no driver for
> > > "allwinner,sun20i-d1-wdt-reset", so this is the only way for `reset` to
> > > work.  
> >
> > So I had a look at this, and it's a bit surprising:
> > The watchdog driver already supports "allwinner,sun20i-d1-wdt" for a
> > while. We don't need to list the "-reset" version, because the normal
> > compatible name acts as a fallback string. However the DT node in the base
> > .dtsi sets: status = "reserved"; for this (and the other) watchdog, so
> > U-Boot's DM (correctly!) ignores those devices. Trying to figure out why.
> > Adding:
> > &wdt {
> >         status = "okay";
> > };
> > to sun8i-t113s.dtsi fixes that for me, now the reset command works.  
> 
> I did it the same way for myself. 🙂 But I thought this was the wrong way and
> the watchdog should be managed by a trusted OS  or something like that.
> (which we don't have in the mainline yet)

Well, the name "reserved" suggests so, but I think it's really that those
watchdogs don't seem to affect the RISC-V core, that's why there is a
separate one (riscv_wdt). To avoid those watchdogs being picked up the any
(RISC-V) OS, they are marked as reserved in the *shared* .dtsi.
So I think marking the one as "okay" in the ARM specific .dtsi is the way
to go, but I will wait for someone confirming the reason behind it.

Cheers,
Andre