| Message ID | 20221206004549.29015-1-andre.przywara@arm.com |
|---|---|
| Headers | show |
| Series | sunxi: rework pinctrl and add T113s support | expand |
Hi Andre,
On 12/5/22 17:45, Andre Przywara wrote:
> Please let me know if you have any opinions!
I believe I promised you last month I'd let you know once I had a build
I'm happy with, and I'm pleased to say that I think I've reached that
point. I'm running quite rapidly out of sharp edges to sand down, too.
I have a build of U-Boot for my target, complete with:
- UART3 initialized correctly
- DRAM coming up correctly
- SPL sets configured boot clock correctly
- SPI-NAND support (SPL and U-Boot proper)
- MMC support (SPL and U-Boot proper)
- SPL boot from FEL
- USB gadget support
- Ethernet MAC+PHY support
- I²C support *
- GPIO support (LEDs, buttons, misc. board management)
- `reset` working (requries CONFIG_SYSRESET unset, WDT key)
- PSCI, nonsec
- 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.
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. :)
Thank you greatly,
Sam
P.S. I figure the reason there aren't I²C function defs in the d1
pinctrl table already is because Allwinner tends to kick around the I²C
mux values a lot and we would need a per-pin lookup table that would eat
up too much valuable image space?
In an entirely JUST FOR FUN exercise to give myself a break from staring
at datasheets/patches and do a "pure CS" coding challenge for a change,
I came up with a terse encoding scheme for this table. Here is the size
(in bits) for a selection of D1's functions (pin assignments harvested
from Linux):
'emac': 50,
'i2c0': 101,
'i2c1': 64,
'i2c2': 109,
'i2c3': 91,
'mmc0': 23,
'mmc1': 23,
'mmc2': 20,
'spi0': 41,
'spi1': 48,
'uart0': 78,
'uart1': 87,
'uart2': 88,
'uart3': 102,
'uart4': 68,
'uart5': 66,
...and yes, it also identifies invalid pin assignments! I'd be willing
to contribute something like this if there's big interest, but I figure
needing to compress this at build-time might be a bit too complicated
for the U-Boot project's liking.
On Fri, 9 Jun 2023 16:16:43 -0600 Sam Edwards <cfsworks@gmail.com> wrote: Hi Sam, > On 12/5/22 17:45, Andre Przywara wrote: > > Please let me know if you have any opinions! > > I believe I promised you last month I'd let you know once I had a build > I'm happy with, and I'm pleased to say that I think I've reached that > point. I'm running quite rapidly out of sharp edges to sand down, too. 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: > 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? > - DRAM coming up correctly > - SPL sets configured boot clock correctly This should work as per github? > - SPI-NAND support (SPL and U-Boot proper) This is with Icenow's series? Any D1 specific changes needed there? > - MMC support (SPL and U-Boot proper) > - SPL boot from FEL again worked already in github? > - 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? > - Ethernet MAC+PHY support Anything surprising here? Is that using an already supported external PHY? > - I²C support * > - GPIO support (LEDs, buttons, misc. board management) again should work out of the box, minus your board specific configuration? > - `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? 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 ;-) > - PSCI, nonsec ah yeah, owe you some reviews on this one ... > - 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? > 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. Cheers, Andre > > Thank you greatly, > Sam > > P.S. I figure the reason there aren't I²C function defs in the d1 > pinctrl table already is because Allwinner tends to kick around the I²C > mux values a lot and we would need a per-pin lookup table that would eat > up too much valuable image space? > > In an entirely JUST FOR FUN exercise to give myself a break from staring > at datasheets/patches and do a "pure CS" coding challenge for a change, > I came up with a terse encoding scheme for this table. Here is the size > (in bits) for a selection of D1's functions (pin assignments harvested > from Linux): > > 'emac': 50, > 'i2c0': 101, > 'i2c1': 64, > 'i2c2': 109, > 'i2c3': 91, > 'mmc0': 23, > 'mmc1': 23, > 'mmc2': 20, > 'spi0': 41, > 'spi1': 48, > 'uart0': 78, > 'uart1': 87, > 'uart2': 88, > 'uart3': 102, > 'uart4': 68, > 'uart5': 66, > > ...and yes, it also identifies invalid pin assignments! I'd be willing > to contribute something like this if there's big interest, but I figure > needing to compress this at build-time might be a bit too complicated > for the U-Boot project's liking.
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
On Mon, 12 Jun 2023 15:18:17 -0600 Sam Edwards <cfsworks@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
On Mon, 12 Jun 2023 15:18:17 -0600 Sam Edwards <cfsworks@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. > > 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. CONFIG_SYSRESET is only applicable for U-Boot proper, so we need reset_cpu for the SPL still. That is not as critical, since the SPL resets the board only in case of an error, but should be fixed nevertheless. I will have a stab at this. Cheers, Andre > >> - 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
Hi Andre, Sam, пт, 16 июн. 2023 г. в 18:59, Andre Przywara <andre.przywara@arm.com>: > > On Mon, 12 Jun 2023 15:18:17 -0600 > Sam Edwards <cfsworks@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)
On Fri, 16 Jun 2023 19:27:16 +0300 Maxim Kiselev <bigunclemax@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@arm.com>: > > > > On Mon, 12 Jun 2023 15:18:17 -0600 > > Sam Edwards <cfsworks@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
Hi Andre, пт, 16 июн. 2023 г. в 19:36, Andre Przywara <andre.przywara@arm.com>: [..] > > 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. I tried the new version and everything looks pretty good for me. Great job! Just one note. Could you please add SUNXI_R_CPUCFG_BASE 0x07000400 to cpu_sunxi_ncat2.h file. This register is required for Sam's PCSI patches. Otherwise it leads to undeclared error: In file included from ./arch/arm/include/asm/armv7.h:60, from arch/arm/cpu/armv7/sunxi/psci.c:16: arch/arm/cpu/armv7/sunxi/psci.c: In function ‘sunxi_cpu_set_entry’: arch/arm/cpu/armv7/sunxi/psci.c:138:24: error: ‘SUNXI_R_CPUCFG_BASE’ undeclared (first use in this function); did you mean ‘SUNXI_CPUCFG_BASE’? 138 | SUNXI_R_CPUCFG_BASE + SUN8I_R528_SOFT_ENTRY); | ^~~~~~~~~~~~~~~~~~~
Hi Andre, On 6/14/23 18:07, Andre Przywara wrote: > 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. Rebasing onto this branch ended up eliminating a good chunk of my local hack commits. I've verified that everything is still working (but have not yet retested NAND SPL). The remaining local changes I have are two additions to cpu_sunxi_ncat2.h: +#define SUNXI_R_CPUCFG_BASE 0x07000400 /* for PSCI */ +#define SUNXI_RTC_BASE 0x07090000 /* for FEL */ The former can probably be brought into my PSCI series somehow (unless we expect more chips with CPUX blocks which might move those soft entry registers around, then it should be defined in cpu_sunxi_*.h). The latter is to support a reimplementation of Allwinner's `efex` command that I'm using for development (it pokes the magic number 0x5AA5A55A into RTC's GP_DATA_REG[2] and then resets; SPL clears that magic number and then does an early branch to BROM+0x0020 -- exactly what Allwinner's fork does). I've also noticed exactly(!) one formatting difference in our clk_d1.c: - .num_gates = ARRAY_SIZE(d1_gates), + .num_gates = ARRAY_SIZE(d1_gates), Up to you if you prefer to align the = or not, but it does look inconsistent when .gates and .resets are aligned and .num_* aren't - might be a nitpick that comes up in patch review. > Interesting, indeed this is left at 0, which I think will result in 288 > MHz. Correct, at least that's what I was seeing. > What is that frequency in your case? Do you know what the BSP > programs? 1008 MHz, both. > Traditionally we used something conservative that works > without cooling and with the default voltage, but I don't know that > value for the T113s. For what it's worth, this board has a bare T113-s3 and the current OS does not reclock from 1008 MHz at all, and I don't know of any users of the board having stability issues. In my own case, it idles at that clock at around ~35°C. > I think CLK_SUN20I_D1 should be set by default now, so can you check > that this is fixed? It is now gone from my defconfig and still working, so indeed this is fixed. > 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. It's not about the H6 and more about me being unsure whether R528/T113 is the first ARMv7-based SoC to use the new CPU management registers. If it's not, and there's another such chip supported in U-Boot that just lacks PSCI, it would make more sense for me to land my PSCI series independently of our work here, and then you can add the R528 case later. It sounds like R528/T113 may be the first such chip needing this new code, though, so this may have to wait until the R528 series lands. > How would this conflict, exactly? I don't see any other I2C2 > definition? Well, no, the other definitions haven't landed in U-Boot yet. But they do exist in the kernel, datasheets, and physical chips themselves: PB0/PB1/PB8/PB9/PE4/PE5: i2c2 defined as muxval 4 PC0/PC1/PD20/PD21/PG6/PG7/PG14/PG15: i2c2 defined as muxval 3 PE12/PE13: i2c2 defined as muxval 2 Defining i2c2=2 universally would mean that the pins for i2c2 cannot be changed, since it would conflict with every other definition. > And what do you need I2C2 for, exactly? Pins PE12/PE13 host an I²C bus with the board ID EEPROM and an Ethernet switch that should be reset (and have a few registers set to configure proper port isolation) very shortly after power-on. > Well, there are shortcuts. I sketched some simpler idea in the comment > at the top of pinctrl-sunxi.c. My shortcut for the time being will probably be, "downstream patch." >> 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. Currently, downstream is still fairly dependent on the Tina Linux kernel, not mainline. This is a situation I'd like to change, but it's a push for another day -- my focus right now is only on improving the bootloader situation. This means that there are actually two DTs: one for the kernel, using the Tina Linux binding values, and one for U-Boot's control FDT, which can only support U-Boot right now (and cannot yet be tested on a real kernel). So neither DT is acceptable upstream: the former uses incompatible values/includes, and the latter isn't meant for Linux. Even after(/if) this situation is resolved, the unified DT will probably remain in a state of flux for a while, until some drivers can be updated upstream (there's a slight mess with the I²C driver that needs to be cleaned up and we have to use GPIO-bitbanged I²C until then, for example) so it'll take more work before we have a "final" DT. At *that* time, upstreaming would be a good idea... ...but for now it's very much meant to be out-of-tree. :) (I also do not work for the company that produced this board -- I'm just a contributor to the firmware project. Whether the project would even use the mainline version of its DT in the first place is, though likely, ultimately not my call.) > Cheers, > Andre Likewise, Sam
On Sun, 18 Jun 2023 13:01:33 -0600 Sam Edwards <cfsworks@gmail.com> wrote: Hi Sam, thanks for the reply, that's very helpful. > On 6/14/23 18:07, Andre Przywara wrote: > > 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. > > Rebasing onto this branch ended up eliminating a good chunk of my local > hack commits. I've verified that everything is still working (but have > not yet retested NAND SPL). Great, thanks! > The remaining local changes I have are two additions to cpu_sunxi_ncat2.h: > +#define SUNXI_R_CPUCFG_BASE 0x07000400 /* for PSCI */ > +#define SUNXI_RTC_BASE 0x07090000 /* for FEL */ Right, I will definitely take the PSCI bit, but not so sure about FEL yet. > The former can probably be brought into my PSCI series somehow (unless > we expect more chips with CPUX blocks which might move those soft entry > registers around, then it should be defined in cpu_sunxi_*.h). The > latter is to support a reimplementation of Allwinner's `efex` command > that I'm using for development (it pokes the magic number 0x5AA5A55A > into RTC's GP_DATA_REG[2] and then resets; SPL clears that magic number > and then does an early branch to BROM+0x0020 -- exactly what Allwinner's > fork does). So yeah, the request of a "Enter FEL" command came up multiple times, but so far no one could be bothered to implement this properly. The idea would be to have a generic command (more like "fel-reset" than efex), and allow each SoC (family) to implement this differently, as every SoC requires something a bit different here (32-bit vs. 64-bit, having an RTC vs not, etc). If you could post your solution somewhere, we could start this effort. There was some patch for the H3 already, and it's relatively straight-forward on the newer SoCs (H616, IIRC), so if at least two popular, but different SoCs would be supported, we could make sure to have the right abstractions in place. > I've also noticed exactly(!) one formatting difference in our clk_d1.c: > - .num_gates = ARRAY_SIZE(d1_gates), > + .num_gates = ARRAY_SIZE(d1_gates), > > Up to you if you prefer to align the = or not, but it does look > inconsistent when .gates and .resets are aligned and .num_* aren't - > might be a nitpick that comes up in patch review. Well, this is how it is in all the other clock drivers, so I chose to stay consistent with them ;-) > > Interesting, indeed this is left at 0, which I think will result in 288 > > MHz. > > Correct, at least that's what I was seeing. > > > What is that frequency in your case? Do you know what the BSP > > programs? > > 1008 MHz, both. > > > Traditionally we used something conservative that works > > without cooling and with the default voltage, but I don't know that > > value for the T113s. > > For what it's worth, this board has a bare T113-s3 and the current OS > does not reclock from 1008 MHz at all, and I don't know of any users of > the board having stability issues. > > In my own case, it idles at that clock at around ~35°C. OK, many thanks, it looks like 1008 MHz it is, then. > > I think CLK_SUN20I_D1 should be set by default now, so can you check > > that this is fixed? > > It is now gone from my defconfig and still working, so indeed this is fixed. > > > 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. > > It's not about the H6 and more about me being unsure whether R528/T113 > is the first ARMv7-based SoC to use the new CPU management registers. If > it's not, and there's another such chip supported in U-Boot that just > lacks PSCI, it would make more sense for me to land my PSCI series > independently of our work here, and then you can add the R528 case > later. It sounds like R528/T113 may be the first such chip needing this > new code, though, so this may have to wait until the R528 series lands. > > > How would this conflict, exactly? I don't see any other I2C2 > > definition? > > Well, no, the other definitions haven't landed in U-Boot yet. But they > do exist in the kernel, datasheets, and physical chips themselves: > > PB0/PB1/PB8/PB9/PE4/PE5: i2c2 defined as muxval 4 > PC0/PC1/PD20/PD21/PG6/PG7/PG14/PG15: i2c2 defined as muxval 3 > PE12/PE13: i2c2 defined as muxval 2 > > Defining i2c2=2 universally would mean that the pins for i2c2 cannot be > changed, since it would conflict with every other definition. Well, we are well aware that the current pinmux code is limited, but we figured it does work for all practical purposes, and U-Boot has a far tighter scope than Linux, so we don't need to support every theoretically possible use case. So if the TuringPi 2 board would be upstream, and you need I2C2, you would just set the precedence for muxval 2. It's then the next board's problem to live with that. Either by adding the port number (PB/PC/PD/PG) to the definitions, or by reverting to a DT solution: https://lore.kernel.org/linux-sunxi/20221110014255.20711-1-andre.przywara@arm.com/ > > And what do you need I2C2 for, exactly? > > Pins PE12/PE13 host an I²C bus with the board ID EEPROM and an Ethernet > switch that should be reset (and have a few registers set to configure > proper port isolation) very shortly after power-on. > > > Well, there are shortcuts. I sketched some simpler idea in the comment > > at the top of pinctrl-sunxi.c. > > My shortcut for the time being will probably be, "downstream patch." see below ;-) > >> 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. > > Currently, downstream is still fairly dependent on the Tina Linux > kernel, not mainline. This is a situation I'd like to change, but it's a > push for another day -- my focus right now is only on improving the > bootloader situation. Ah, depending on the BSP kernel is indeed quite bad. I wonder what features of the kernel you rely on that upstream does not have? Or is it more about the BMC userland parts that are married to the Allwinner kernel and its own interfaces? > This means that there are actually two DTs: one for the kernel, using > the Tina Linux binding values, and one for U-Boot's control FDT, which > can only support U-Boot right now (and cannot yet be tested on a real > kernel). So neither DT is acceptable upstream: the former uses > incompatible values/includes, and the latter isn't meant for Linux. So as you probably know, conceptually there is only one DT per machine, as the DT describes the hardware. That's why try hard to align U-Boot and Linux at least, and IIUC the BSDs copied the version in the Linux kernel tree as well. Allwinner seems to have little clue or interest here, and we gave up on their (quite misguided) interpretation of the DT a long time ago. > Even after(/if) this situation is resolved, the unified DT will probably > remain in a state of flux for a while, until some drivers can be updated > upstream (there's a slight mess with the I²C driver that needs to be > cleaned up and we have to use GPIO-bitbanged I²C until then, for > example) so it'll take more work before we have a "final" DT. At *that* > time, upstreaming would be a good idea... Final DT is a noble goal, but in reality there will always be room for improvement and additions. So what we typically do is to start with a simple .dts for the kernel tree, describing the basic peripherals, and everything that already works and is not subject to debate. If in doubt, include a node, and we will comment. Could you prepare such a patch? This should not contradict any DT nodes that U-Boot uses, so it's not a double effort. This would mean we have a *second* board DT for the T113s SoC in the kernel, which always helps to improve quality and prevents hacks that just work on the MangoPi. Besides, the TuringPi board is an actually useful application of the SoC, deployed and available, in contrast to just some development board from Chinese websites. And once this is merged, we could just copy this over to U-Boot and add the defconfig and any other support patches there. > ...but for now it's very much meant to be out-of-tree. :) > > (I also do not work for the company that produced this board -- I'm just Ah, that would have been a first anyway ;-) > a contributor to the firmware project. Whether the project would even > use the mainline version of its DT in the first place is, though likely, > ultimately not my call.) Yeah, I understand it's not the most grateful job to chase up on doing things properly and stay on with the upstreaming process. Ultimately it's the right thing to do, though, and will save you hassle over time. Plus we (the community) will help you with that, and you'd get a second commit in the kernel ;-) Cheers, Andre
Hi Andre, On 6/20/23 06:42, Andre Przywara wrote: > So yeah, the request of a "Enter FEL" command came up multiple times, but > so far no one could be bothered to implement this properly. The idea would > be to have a generic command (more like "fel-reset" than efex), and > allow each SoC (family) to implement this differently, as every > SoC requires something a bit different here (32-bit vs. 64-bit, having an > RTC vs not, etc). > > If you could post your solution somewhere, we could start this effort. > There was some patch for the H3 already, and it's relatively > straight-forward on the newer SoCs (H616, IIRC), so if at least two > popular, but different SoCs would be supported, we could make sure to have > the right abstractions in place. I already have a "go_to_fel()" that does the right thing to enter FEL from the SPL; I would pretty much just need to introduce the following per-SoC(-family) functions: - bool sunxi_fel_flag_test(void) - void sunxi_fel_flag_clear(void) - void sunxi_fel_flag_set(void) The "fel-reset" command (which is easier to type than what I have, "run fel_do_enter") would then call sunxi_fel_flag_set() and initiate a reset, and the SPL's early init just has to do sunxi_fel_flag_test() -> sunxi_fel_flag_clear() -> go_to_fel(). Seems easy enough. Could you recommend to me a sufficiently different chip to test my abstractions against? Something ARMv8 and *without* RTC? I can then send in a series adding FEL support for that. (Also, did that H3 patch actually land? I didn't see anything but want to know if I should be refactoring my approach to extend what that H3 patch does or not.) > Ah, depending on the BSP kernel is indeed quite bad. I wonder what > features of the kernel you rely on that upstream does not have? Or is it > more about the BMC userland parts that are married to the Allwinner kernel > and its own interfaces? I don't fully know; getting the kernel back on mainline is, as I said, a push for another day. I'm very much making a point of not looking into it before the bootloader can be upgraded to something that isn't a crashy, hard-to-update, failure-prone mess. (I'm working in "biggest fire, first out" order.) That said, the first such dependent feature that leaps to mind is the AWNAND driver's CONFIG_AW_SPINAND_SIMULATE_MULTIPLANE, which logically interleaves pages of the NAND in a different ordering vs. what the physical NAND (and mainline's spi-nand driver) does. Alas this is a feature we're dependent on not because it provides benefits to our users (it does not, and in downstream discussions I've been soapboxing about how it's likely wearing down people's NANDs) but because the boards are flashed at the factory with this flag enabled so we need it set for the NAND to be accessible. We've experimented with reflashing the board with that flag disabled, but that has so far only resulted in corrupted flash. Hope is not lost, though, for I have a half-written tool which shows some promise in being able to "unscramble the egg" and migrate existing NANDs over to the correct layout. That should be sufficient to get mainline U-Boot (and Tina Linux with the flag disabled) working, but I have no idea about mainline Linux still: this would only peel back one layer of the onion, and I don't know whether the next obstacle will be easier, harder, or about the same difficulty. But it does mean that, for now, we're stuck with Tina Linux. > Final DT is a noble goal, but in reality there will always be room for > improvement and additions. So what we typically do is to start with a > simple .dts for the kernel tree, describing the basic peripherals, and > everything that already works and is not subject to debate. If in doubt, > include a node, and we will comment. Could you prepare such a patch? The peripheral-describing .dts that I have is for Tina Linux, and uses incompatible compatibles (ha), includes, dt-bindings, temporary hacks while better driver support can be developed, and would otherwise not fly upstream. I can send it in *anyway* if for some reason you think that's a good idea, but I really don't see that as being anything other than a waste of time. As well, I can't write a fresh .dts for mainline (one more likely to be accepted on the list). A mainline kernel has never been booted on this board, so I would do no better at this than a kernel contributor selected at random. The best I can do now is write something that *looks* like the correct .dts. As I keep saying, that may change in the future. But the answer today is still "no, I cannot." > This > should not contradict any DT nodes that U-Boot uses, so it's not a double > effort. True, in theory it *shouldn't* but in practice, I've found it does. One way I've been bitten is that the sunxi SPI driver in U-Boot doesn't support Quad-SPI, so if the DT says the SPI-NAND is connected with a bus width of 4, the SPI-NAND driver requests Quad-SPI transfers, but the SPI master driver has no idea that it needs to handle the transfer any differently, and we're left with corrupt NAND reads/writes. Without Quad-SPI support in U-Boot's master driver (and/or, better yet, a U-Boot equivalent to Linux commit 83596fbeb5) -- also a push for another day -- I have no choice but to give U-Boot a specially edited version of the DT that omits this property. > This would mean we have a *second* board DT for the T113s SoC in the > kernel, which always helps to improve quality and prevents hacks that just > work on the MangoPi. Besides, the TuringPi board is an actually useful > application of the SoC, deployed and available, in contrast to just some > development board from Chinese websites. > And once this is merged, we could just copy this over to U-Boot and add > the defconfig and any other support patches there. See below. >> ...but for now it's very much meant to be out-of-tree. :) >> >> (I also do not work for the company that produced this board -- I'm just > > Ah, that would have been a first anyway ;-) Oh? What would have been a first? I could pass it along to my contact at this company and encourage him to get involved in some way. I'm sure they'd appreciate the opportunity for the good press associated with being the first at something in the F/OSS world, and it might help to get them in the habit of cooperating closely with upstream (to make it less likely that they just fork things the moment upstream doesn't solve some problem they're having). > Yeah, I understand it's not the most grateful job to chase up on doing > things properly and stay on with the upstreaming process. Ultimately it's > the right thing to do, though, and will save you hassle over time. Plus we > (the community) will help you with that, and you'd get a second commit in > the kernel ;-) Ideologically-speaking, this is music to my ears, and I think we would even be having this same discussion were our roles reversed: we do both agree fully on the (mutual) benefits of upstream contribution. But even more ultimately: the available time on any given day is limited, and I have to choose my battles. There are often things that either require less effort, save an even greater hassle over time, or provide more urgently-needed benefits, which (pragmatically speaking) ought to take priority. That doesn't mean the other lower-priority items have no benefit, it just means they should not be done *now.* > Cheers, > Andre Likewise, Sam
On Tue, 20 Jun 2023 16:11:48 -0600 Sam Edwards <cfsworks@gmail.com> wrote: Hi Sam, pleasure to write with you ;-) > On 6/20/23 06:42, Andre Przywara wrote: > > So yeah, the request of a "Enter FEL" command came up multiple times, but > > so far no one could be bothered to implement this properly. The idea would > > be to have a generic command (more like "fel-reset" than efex), and > > allow each SoC (family) to implement this differently, as every > > SoC requires something a bit different here (32-bit vs. 64-bit, having an > > RTC vs not, etc). > > > > If you could post your solution somewhere, we could start this effort. > > There was some patch for the H3 already, and it's relatively > > straight-forward on the newer SoCs (H616, IIRC), so if at least two > > popular, but different SoCs would be supported, we could make sure to have > > the right abstractions in place. > > I already have a "go_to_fel()" that does the right thing to enter FEL > from the SPL; I would pretty much just need to introduce the following > per-SoC(-family) functions: > - bool sunxi_fel_flag_test(void) > - void sunxi_fel_flag_clear(void) > - void sunxi_fel_flag_set(void) Well, so this is actually the fallback implementation which should somewhat work on most SoCs: set a flag, reset, and catch the flag in the SPL. For modern SoCs with CPU hotplug support (the H616 is one one of those, and it looks like the T113s is as well), there is actually a more direct route: We put some magic and the FEL entry address into some special memory locations, then just reset. Now the *BootROM* will do the check already, and branch to the provided entry point, which would be the FEL routine. This doesn't rely on a prepared SPL to be loaded, so works without a boot device with mainline U-Boot around. Refer to section 3.4.2.3 and 3.4.2.4 of the T113-S3 user manual (v1.1). According to this, the magic would be 0xfa50392f, the magic's address is 0x070005C0, and CPU0's entry point address would be in 0x070005C4. I had a proof of concept implementation for the H616 using this method. The only problem left would be that someone needs to clean the magic afterwards, otherwise any follow-up reset would trigger FEL mode again. > The "fel-reset" command (which is easier to type than what I have, "run > fel_do_enter") would then call sunxi_fel_flag_set() and initiate a > reset, and the SPL's early init just has to do sunxi_fel_flag_test() -> > sunxi_fel_flag_clear() -> go_to_fel(). Seems easy enough. > > Could you recommend to me a sufficiently different chip to test my > abstractions against? Something ARMv8 and *without* RTC? I think all ARMv8 parts have an RTC, so your generic approach might work there as well. The complication is that the SPL switches to AArch64 very early, in hand-stitched AArch32 assembly, check out arch/arm/include/asm/arch-sunxi/boot0.h. The check would need to be coded like this, then. > I can then send > in a series adding FEL support for that. (Also, did that H3 patch > actually land? I didn't see anything but want to know if I should be > refactoring my approach to extend what that H3 patch does or not.) https://patchwork.ozlabs.org/project/uboot/patch/c211aa414c59e7275fef82bf6f0035276d6e29f3.1656875482.git.msuchanek@suse.de/ Another generic approach for ARMv7 parts would be to reset the peripherals as much as possible, then configure the core in a BROM compatible way (MMU off, etc) and just branch to the BROM FEL entry address. This idea is already somewhat used in our return-to-FEL code in the SPL, although we don't change too much of the core setup in the SPL. > > Ah, depending on the BSP kernel is indeed quite bad. I wonder what > > features of the kernel you rely on that upstream does not have? Or is it > > more about the BMC userland parts that are married to the Allwinner kernel > > and its own interfaces? > > I don't fully know; getting the kernel back on mainline is, as I said, a > push for another day. I'm very much making a point of not looking into > it before the bootloader can be upgraded to something that isn't a > crashy, hard-to-update, failure-prone mess. (I'm working in "biggest > fire, first out" order.) Fair enough, from a mainlining point of view you need to funnel the board .dts through the Linux tree first, though. Also, with the right DT, a mainline kernel would run on the board already, maybe just not with the full functionality you'd expect from it. What I mean to say: you can surely continue using Tina Linux for the BMC functionality on the board right now, but still upstream the board .dts. As mentioned, the DT just describes the hardware, so it doesn't dictate what to do with it. One might abuse the board as a T113s dev board, maybe ;-) Does it work without any of the modules populated? > That said, the first such dependent feature that leaps to mind is the > AWNAND driver's CONFIG_AW_SPINAND_SIMULATE_MULTIPLANE, which logically > interleaves pages of the NAND in a different ordering vs. what the > physical NAND (and mainline's spi-nand driver) does. Alas this is a > feature we're dependent on not because it provides benefits to our users > (it does not, and in downstream discussions I've been soapboxing about > how it's likely wearing down people's NANDs) but because the boards are > flashed at the factory with this flag enabled so we need it set for the > NAND to be accessible. We've experimented with reflashing the board with > that flag disabled, but that has so far only resulted in corrupted flash. > > Hope is not lost, though, for I have a half-written tool which shows > some promise in being able to "unscramble the egg" and migrate existing > NANDs over to the correct layout. That should be sufficient to get > mainline U-Boot (and Tina Linux with the flag disabled) working, but I > have no idea about mainline Linux still: this would only peel back one > layer of the onion, and I don't know whether the next obstacle will be > easier, harder, or about the same difficulty. > > But it does mean that, for now, we're stuck with Tina Linux. > > > Final DT is a noble goal, but in reality there will always be room for > > improvement and additions. So what we typically do is to start with a > > simple .dts for the kernel tree, describing the basic peripherals, and > > everything that already works and is not subject to debate. If in doubt, > > include a node, and we will comment. Could you prepare such a patch? > > The peripheral-describing .dts that I have is for Tina Linux, and uses > incompatible compatibles (ha), includes, dt-bindings, temporary hacks > while better driver support can be developed, and would otherwise not > fly upstream. Sure, you keep that nasty piece downstream, and load it in U-Boot into $fdt_addr_r, then take it from there. That doesn't affect the mainline Linux and U-Boot DT, though, which could be upstreamed independently. Then you can use the mainline DT (as used by U-Boot), using $fdtcontroladdr, or any loaded DT, via $fdt_addr_r. > I can send it in *anyway* if for some reason you think > that's a good idea, but I really don't see that as being anything other > than a waste of time. There is indeed no point in sending a DT which only works with the Allwinner BSP kernel. > As well, I can't write a fresh .dts for mainline (one more likely to be > accepted on the list). Yes, please! > A mainline kernel has never been booted on this > board, so I would do no better at this than a kernel contributor > selected at random. ... having a board. So far you are the one contributor with access to the hardware, so: thanks for volunteering! ;-) > The best I can do now is write something that *looks* like the correct .dts. Yes, and that would be the right .dts, if it passes the kernel review. > As I keep saying, that may change in the future. But the answer today is > still "no, I cannot." So to summarise what I am trying to say: You create a simple .dts file, basically #include "sun8i-t113s.dtsi" and declaring the UART. Then add the devices that already work (Ethernet?, USB-OTG) and see how far you get. You could just load a kernel and initrd via FEL, and use the mainline kernel just via serial like this. Granted, this is not really useful in the BMC context, but would be a start. This is actually similar to what Chris just did [1] for the RG Nano: this initial DT for this mini handheld gaming console has no display support, so is pretty useless in its original context, but starting mainline support is the important thing here. Plus: this exposes the problems you face (PHY config?) to a wider range of people, who can help with the solution. [1] https://lore.kernel.org/linux-sunxi/20230620200022.295674-1-macroalpha82@gmail.com/T > > > This > > should not contradict any DT nodes that U-Boot uses, so it's not a double > > effort. > > True, in theory it *shouldn't* but in practice, I've found it does. > > One way I've been bitten is that the sunxi SPI driver in U-Boot doesn't > support Quad-SPI, so if the DT says the SPI-NAND is connected with a bus > width of 4, the SPI-NAND driver requests Quad-SPI transfers, but the SPI > master driver has no idea that it needs to handle the transfer any > differently, and we're left with corrupt NAND reads/writes. Without > Quad-SPI support in U-Boot's master driver (and/or, better yet, a U-Boot > equivalent to Linux commit 83596fbeb5) -- also a push for another day -- > I have no choice but to give U-Boot a specially edited version of the DT > that omits this property. The U-Boot build system support some kind of build time DT "overlay" feature: You put a file with the same name, but ending in "-u-boot.dtsi" in the arch/arm/dts directory, and it will be included into the DT which gets embedded into the U-Boot image. See arch/arm/dts/sun50i-a64-sopine-baseboard-u-boot.dts for an example, and doc/develop/devicetree/control.rst for the proper documentation. So we upstream a minimal, non-controversial and non-contradicting base .dts into the kernel tree, and can fix things up for the time being using this method. This hack can then go away if either the mainline kernel DT gets fixed and/or U-Boot learns the quad-SPI trick. > > This would mean we have a *second* board DT for the T113s SoC in the > > kernel, which always helps to improve quality and prevents hacks that just > > work on the MangoPi. Besides, the TuringPi board is an actually useful > > application of the SoC, deployed and available, in contrast to just some > > development board from Chinese websites. > > And once this is merged, we could just copy this over to U-Boot and add > > the defconfig and any other support patches there. > > See below. > > >> ...but for now it's very much meant to be out-of-tree. :) > >> > >> (I also do not work for the company that produced this board -- I'm just > > > > Ah, that would have been a first anyway ;-) > > Oh? What would have been a first? I could pass it along to my contact at Someone from the board vendor company actually actively adding upstream support for their device early. There were some examples in the past where employees participated in upstreaming, but I cannot remember seeing this too often when it comes to the initial DT support. > this company and encourage him to get involved in some way. I'm sure > they'd appreciate the opportunity for the good press associated with > being the first at something in the F/OSS world, and it might help to > get them in the habit of cooperating closely with upstream (to make it > less likely that they just fork things the moment upstream doesn't solve > some problem they're having). Yes, I understand the pressure in a product-centric world with release dates and timelines, but the advantage of vendor-backed contributions are early access to hardware and documentation, plus access to the hardware engineers. > > Yeah, I understand it's not the most grateful job to chase up on doing > > things properly and stay on with the upstreaming process. Ultimately it's > > the right thing to do, though, and will save you hassle over time. Plus we > > (the community) will help you with that, and you'd get a second commit in > > the kernel ;-) > > Ideologically-speaking, this is music to my ears, and I think we would > even be having this same discussion were our roles reversed: we do both > agree fully on the (mutual) benefits of upstream contribution. > > But even more ultimately: the available time on any given day is > limited, and I have to choose my battles. There are often things that Fair enough, and there is no real pressure in getting the mainline DT fully functional and merged today. But we should start the process *now*, as this helps to detect problems early, and allows other people to jump on this and continue the work or help out. Cheers, Andre > either require less effort, save an even greater hassle over time, or > provide more urgently-needed benefits, which (pragmatically speaking) > ought to take priority. That doesn't mean the other lower-priority items > have no benefit, it just means they should not be done *now.* > > > Cheers, > > Andre > > Likewise, > Sam
On 6/21/23 04:55, Andre Przywara wrote: > On Tue, 20 Jun 2023 16:11:48 -0600 > Sam Edwards <cfsworks@gmail.com> wrote: > > Hi Sam, > > pleasure to write with you ;-) Hi Andre, Likewise! > Well, so this is actually the fallback implementation which should > somewhat work on most SoCs: set a flag, reset, and catch the flag in > the SPL. For modern SoCs with CPU hotplug support (the H616 is one one > of those, and it looks like the T113s is as well), there is actually a more > direct route: Oh man, I would definitely prefer a direct route that doesn't require the SPL coming up a second time, but... > We put some magic and the FEL entry address into some special memory > locations, then just reset. Now the *BootROM* will do the check already, > and branch to the provided entry point, which would be the FEL routine. > This doesn't rely on a prepared SPL to be loaded, so works without a > boot device with mainline U-Boot around. > Refer to section 3.4.2.3 and 3.4.2.4 of the T113-S3 user manual (v1.1). > According to this, the magic would be 0xfa50392f, the magic's address is > 0x070005C0, and CPU0's entry point address would be in 0x070005C4. I had a > proof of concept implementation for the H616 using this method. ...I tried this and it seems that the 070005C* block hardware-resets to zero before BROM runs. Is there a softer reset method you had in mind that would avoid this? > The only > problem left would be that someone needs to clean the magic afterwards, > otherwise any follow-up reset would trigger FEL mode again. That's at least pretty fixable though: instead of setting the entry address to the FEL entry point, set it to a thunk placed in SRAM that clears the flag before continuing onward to FEL. >> The "fel-reset" command (which is easier to type than what I have, "run >> fel_do_enter") would then call sunxi_fel_flag_set() and initiate a >> reset, and the SPL's early init just has to do sunxi_fel_flag_test() -> >> sunxi_fel_flag_clear() -> go_to_fel(). Seems easy enough. >> >> Could you recommend to me a sufficiently different chip to test my >> abstractions against? Something ARMv8 and *without* RTC? > > I think all ARMv8 parts have an RTC, so your generic approach might work > there as well. The complication is that the SPL switches to AArch64 very > early, in hand-stitched AArch32 assembly, check out > arch/arm/include/asm/arch-sunxi/boot0.h. > The check would need to be coded like this, then. > >> I can then send >> in a series adding FEL support for that. (Also, did that H3 patch >> actually land? I didn't see anything but want to know if I should be >> refactoring my approach to extend what that H3 patch does or not.) > > https://patchwork.ozlabs.org/project/uboot/patch/c211aa414c59e7275fef82bf6f0035276d6e29f3.1656875482.git.msuchanek@suse.de/ This approach seems close to mine, only my go_to_fel() enters by way of return_to_fel() after first modifying fel_stash.lr, since the return_to_fel() mechanism already takes care of restoring the core to a BROM-friendly state. > One might abuse the board as a T113s dev > board, maybe ;-) Does it work without any of the modules populated? Sure, if you're thinking about getting one. You just need an ATX-pinout PSU to power the BMC (it runs off of the 5V standby rail). > ... having a board. So far you are the one contributor with access to > the hardware, so: thanks for volunteering! ;-) Andre, please, I know you're being tongue-in-cheek here, but I said "no." We should have reached the agree-to-disagree point 2 emails ago: you've made your (very compelling) case for why downstream would benefit from the early expertise of the upstream DT reviewers, and how upstream would benefit from having the DT for a second "real" T113-using board, but at some point you need to trust that I understand that and that I must therefore have very good reasons not to be distracting myself with trying to (dual-)boot a mainline kernel yet. One thing at a time, y'know? :) > The U-Boot build system support some kind of build time DT "overlay" > feature: You put a file with the same name, but ending in > "-u-boot.dtsi" in the arch/arm/dts directory, and it will be included > into the DT which gets embedded into the U-Boot image. See > arch/arm/dts/sun50i-a64-sopine-baseboard-u-boot.dts for an > example, and doc/develop/devicetree/control.rst for the proper > documentation. > So we upstream a minimal, non-controversial and non-contradicting base > .dts into the kernel tree, and can fix things up for the time being > using this method. This hack can then go away if either the mainline > kernel DT gets fixed and/or U-Boot learns the quad-SPI trick. Oh, good to know! I'll try to remember that this option exists when the time comes to use it. > Someone from the board vendor company actually actively adding upstream > support for their device early. There were some examples in the past > where employees participated in upstreaming, but I cannot remember > seeing this too often when it comes to the initial DT support. I brought this email thread to the attention of the firmware development team at this company, then. No promises (they seem to have their hands sufficiently full with userspace work) but FWIW my opinion of them is that they do have a community-centric and F/OSS-oriented mindset, so with a bit of luck they may make themselves known on the upstream mailing lists at some point. Thank you for your ongoing efforts, Sam