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

[Sam Edwards <cfsworks@gmail.com>]

Hey Andre,

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.

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

>> - 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 */

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

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

> Cheers,
> Andre

Likewise,
Sam