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

[Andre Przywara]

On Mon, 12 Jun 2023 15:18:17 -0600
Sam Edwards <cfsworks at gmail.com> wrote:

Hi Sam,

> 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.

So I finally found some time to address some issues in the series,
especially in the first patches (pinctrl rework and preparation).
I pushed a branch to https://github.com/apritzel/u-boot/commits/r528-rc
I need to do more testing, most importantly regression testing on other
SoCs, and will only be able to post something next week, I guess.

If you could briefly list the things that are still missing, I could
try to pick some low hanging fruits.

> >> 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.

Interesting, indeed this is left at 0, which I think will result in 288
MHz. What is that frequency in your case? Do you know what the BSP
programs? Traditionally we used something conservative that works
without cooling and with the default voltage, but I don't know that
value for the T113s.

> >> - 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).

Ah, right, we already merged the "allwinner,sun20i-d1-usb-phy" patch.

> 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).

I think CLK_SUN20I_D1 should be set by default now, so can you check
that this is fixed?

> 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.

OK, thanks, need to dig into this again.

> > For the WDT key: it seems like Linux got a nice patch to integrate this
> > neatly into the driver without quirking this too much, do you have
> > something ready for U-Boot as well? Would love to see it on the list
> > then ;-)  
> 
> I just hacked the correct value into the function; nothing really 
> suitable for the list, sorry.
> 
> >> - PSCI, nonsec  
> > 
> > ah yeah, owe you some reviews on this one ...  
> 
> It occurs to me that my work *might* support H6 as well (they both use 
> CPUX blocks, right?) so perhaps it would be better if I de-RFC'd my 
> series and instead worked to upstream it chasing H6, for you to come 
> along later and tack on NCAT2 support with your R528 patchset?

Why would we need H6 PSCI support? On the ARMv8 parts we use Trusted
Firmware-A (TF-A) to provide PSCI services, which has a much more mature
implementation.

> >> - Able to boot Linux ;)
> >>
> >> * Requires nonzero `MVTWSI_CONTROL_CLEAR_IFLG` for NCAT2, and a patch to
> >> the pinctrl driver to configure the proper mux function for my necessary
> >> pins.  
> > 
> > Are those pinmuxes straight forward to add to the pinctrl driver? Or
> > are there conflicts similar to UART3?  
> 
> The conflict is that I'm on i2c2 + muxval 2. I suspect this one's going 
> to be a downstream patch to add the necessary line:
> { "i2c2",	2 },	/* PE12-PE13 */

How would this conflict, exactly? I don't see any other I2C2
definition? And what do you need I2C2 for, exactly?

> ...and since no other assignment for i2c2 uses muxval 2, the only hope 
> for this to be supported upstream would be for the pinctrl driver to 
> include the full pin->muxval LUT.

Well, there are shortcuts. I sketched some simpler idea in the comment
at the top of pinctrl-sunxi.c.

> >> I figured I'd share this list as a sort of checklist for your own work,
> >> too. The remainder of my efforts now will probably be focused on
> >> mainlining this stuff (let me know how else I can be of help), and then
> >> I'm afraid I'll have to disappear back downstream to the Turing Pi 2
> >> development effort, but maybe our paths will cross again in the kernel
> >> lists. :)  
> > 
> > Yeah, as you may know, the DT has to go through the kernel list. DT
> > patches can be tedious to upstream, there is now much attention to
> > every detail. Running checkpatch and dtbs_check should reveal most
> > issues beforehand, though.  
> 
> At this time I have no interest in upstreaming the DT.

Why not?

> That might change 
> in the future, but for now it's very much meant to be out-of-tree.

Why is this? This only increases your update burden, and we might break
something and not realise that, if your DT is not in the tree.
The question to ask should be rather: why *not* to upstream the DT?
Please keep in mind that this would block U-Boot support, since we need
the DT approved in the kernel before we could merge it into U-Boot.

Cheers,
Andre