Hi Tom, On Wed, 30 Jun 2021 at 09:49, Tom Rini <tr...@konsulko.com> wrote: > > On Wed, Jun 30, 2021 at 05:31:44PM +0300, Ivaylo Dimitrov wrote: > > Hi, > > > > On 30.06.21 г. 16:33 ч., Tom Rini wrote: > > > On Wed, Jun 30, 2021 at 10:30:20AM +0300, Ivaylo Dimitrov wrote: > > > > Hi, > > > > > > > > On 26.06.21 г. 17:58 ч., Tom Rini wrote: > > > > > On Sat, Jun 26, 2021 at 12:59:21PM +0200, Merlijn Wajer wrote: > > > > > > Hi Tom, Marek, > > > > > > > > > > > > On 25/06/2021 23:59, Tom Rini wrote: > > > > > > > On Fri, Jun 25, 2021 at 11:51:51PM +0200, Pali Rohár wrote: > > > > > > > > On Friday 25 June 2021 17:37:44 Tom Rini wrote: > > > > > > > > > One thing I want to say here as I think it maybe wasn't clear > > > > > > > > > in Marek's > > > > > > > > > suggestion. Why not have X-Loader boot SPL which loads > > > > > > > > > U-Boot from extN > > > > > > > > > on the eMMC? > > > > > > > > > > > > > > > > Hello Tom! I have already answered this in my previous email. > > > > > > > > > > > > > > I just re-read things and I don't see it. But perhaps I'm not > > > > > > > being > > > > > > > clear enough. Why can't you just have NOLO start SPL, not > > > > > > > re-initialize > > > > > > > things (which is a really common case now thanks to aarch64) and > > > > > > > just > > > > > > > use that to load full U-Boot from a not size restricted place? > > > > > > > > > > > > > > > > > > > I think there are a few problems. > > > > > > > > > > > > 1. One is a practical one, from Pali's email: > > > > > > > > > > > > > There is no easy access to eMMC until you start full U-Boot. So > > > > > > > even if all these problems are solved then "bootstrapping" or > > > > > > > flashing U-Boot into such location is not possible, plus there is > > > > > > > no recovery. Plus this loose existing and working operating > > > > > > > system, which is no-go. So this way is basically undebugable and > > > > > > > therefore perfectly hard to develop. > > > > > > > > > > > > Not being able to access the eMMC to write u-boot until u-boot is > > > > > > started does sound like it would make things a bit more tricky. > > > > > > > > > > I don't understand this. Either way it's drivers/mmc/omap_hsmmc.c. > > > > > > > > > > > 2. According to Pali, adding SPL support would not be a trivial > > > > > > task, > > > > > > and might end up with a lot more "#ifdef"s in SPL than the one in > > > > > > Ivaylo's patch. As I understand it, this hypothetical SPL would > > > > > > mostly > > > > > > not do any hardware initialisation on the N900, so that might be > > > > > > where > > > > > > instead hacks would need to be added to SPL. > > > > > > > > > > > > Pali also wrote: > > > > > > > > > > > > > U-Boot for N900 does not use SPL. There is no SPL code > > > > > > > implemented. > > > > > > > Nobody ever tried to implement it and neither tested. As you have > > > > > > > correctly pointed instead of SPL is used vendor X-Loader binary, > > > > > > > which > > > > > > > is signed by RSA key. > > > > > > And when I asked him today: > > > > > > > > > > > > > 12:11 < Pali> in past (10 years ago?) I was investigating the way > > > > > > > how we can boot u-boot and the only reasable way was the current > > > > > > > one, directly load main u-boot by x-loader/nolo > > > > > > > 12:12 < Pali> I have already spend some time with spl on n900 and > > > > > > > at that time I have rejected this idea, because it did not work > > > > > > > that > > > > > > > 12:13 < Pali> spl is also doing hw initialization which cannot be > > > > > > > called on n900 as this code basically crash / freeze n900 > > > > > > So perhaps it would make sense to get more clarity on that, since > > > > > > that > > > > > > seems to be where the argument gets stuck. > > > > > > > > > > This also doesn't make sense to me. CONFIG_SKIP_LOWLEVEL_INIT should > > > > > let you skip whatever initialization doesn't need to be done. If > > > > > there's some init that needs to be skipped but isn't, that's a bug. > > > > > > > > > > > Also, I'd like to point what Ivaylo wrote earlier: > > > > > > > > > > > > > > This sounds like a workaround though. Can you instead do the > > > > > > > > full conversion of the board? I am sure the board removal patch > > > > > > > > can be postponed if there is plan to convert it. > > > > > > > > > > > > > > Hard to say if migration to device-tree is even possible on N900 > > > > > > > ATM, enabling OF_CONTROL increases the size of the produced > > > > > > > binary with some 100k (.dtb not included), making the size of the > > > > > > > binary way above our budget of ~256k. Sure, board config does not > > > > > > > enable -mthumb, but omap3 in rx-51 suffers from ARM errata 430973 > > > > > > > and noone can guarantee we're not going to see SIGILL faults if > > > > > > > we enable it. Which it seems we are forced to do even with DM_USB > > > > > > > migration only. > > > > > > > > > > > > > > Re workaround - I took examples of #ifdef's from the current > > > > > > > u-boot code (mmc, i2c, etc.) so workaround or not, it is no > > > > > > > different to what the other drivers are doing. > > > > > > > > > > > > If the other drivers have the same logic, it seems a bit weird to me > > > > > > that the change made in Ivaylo's patch would be rejected because of > > > > > > a > > > > > > preference to port the board to DT. Unless maybe this was the first > > > > > > driver to be migrated to support only DT and the patch is in effect > > > > > > a > > > > > > reversal of some prior work? > > > > > > > > > > Yes, part of the problem here is that DM_USB is the first case that > > > > > N900 > > > > > has hit as part of DM conversion that written with using OF_CONTROL in > > > > > mind, only. And he's not interested in taking workarounds to provide > > > > > the required information via another mechanism (while i2c for example, > > > > > is). > > > > > > > > > > But another one of the problems here is that N900 doesn't need USB > > > > > host, > > > > > only USB gadget, and you were disabling some of the host stuff that's > > > > > being built but not used. A change to not include unused host mode > > > > > code > > > > > entirely would be another path, and you can address moving to > > > > > DM_USB_GADGET (which doesn't have a deadline yet, but should be > > > > > done...) > > > > > and see if you have to re-visit the OF_CONTROL problem or not. > > > > > > > > Removing usb-uclass.c and usb_hub.c from the build allows me to enable > > > > DM_USB without OF_CONTROL. The resulting binary is 247312 bytes without > > > > -mthumb and runs just fine on a real HW. So I guess this is what we were > > > > aiming for. Please advice how to proceed - shall I introduce > > > > CONFIG_USB_HOST > > > > Kconfig option that is enabled by default and ifdef those files based > > > > on it? > > > > > > Dropping OF_CONTROL is not a solution. You're just delaying the problem > > > until you go to enable DM_USB_GADGET and then once again hitthe case of > > > needing OF_CONTROL. > > > > Let worry about it when we have to. If we delay it for long enough, the > > problem will resolve by itself, as there will be no more HW to support :). > > So, we have another issue here, IIUC. > > I know you're joking here, but I don't think that helps. Looking over > all of the N900 related USB code has left me a bit depressed. It's not > a trivial DM_USB_GADGET conversion because the platform hasn't been > converted to our nearly 10 year old "new" USB_GADGET framework and > almost 9 year old "new" MUSB driver port. And in the end, it's both > doable (there's other omap3 platforms, with and without DM_USB_GADGET > and in turn OF_CONTROL) but a challenge as usbtty is the main (only?) > gadget N900 cares about, and that's not converted to anything modern > either. > > > > I think part of the problem here is that while no, OF_CONTROL is not a > > > hard requirement, it means that you instead need to implement an > > > alternative to provide the same information, in a way that keeps you > > > within the size constraints. As you've seen with USB, no one has yet > > > come up with the case of both "we need USB in a very size constrained > > > area" and "we cannot use OF_CONTROL". > > > > It is not about OF_CONTROL (only), it is about including code which is never > > used (hostmode stuff). Even if we do DT migration and whatnot, the code in > > usb-uclass.c and usb_hub.c will still be not used on N900, so I don't really > > understand why you insist on having it included on a gadget-only device. Or, > > you say that if we move to DM_USB_GADGET, hostmode code is not going to be > > included? Please elaborate. > > The good thing is this has shown that indeed, we build some host stuff > when not needed, and should stop doing that. The bad thing is it's > shown piles of other technical debt. > > To put this another way, I can silence the warning with: > diff --git a/Makefile b/Makefile > index 027e31e09e4c..96d3eaa18add 100644 > --- a/Makefile > +++ b/Makefile > @@ -824,7 +824,7 @@ libs-y += drivers/usb/eth/ > libs-$(CONFIG_USB_DEVICE) += drivers/usb/gadget/ > libs-$(CONFIG_USB_GADGET) += drivers/usb/gadget/ > libs-$(CONFIG_USB_GADGET) += drivers/usb/gadget/udc/ > -libs-y += drivers/usb/host/ > +libs-$(CONFIG_USB_HOST) += drivers/usb/host/ > libs-y += drivers/usb/mtu3/ > libs-y += drivers/usb/musb/ > libs-y += drivers/usb/musb-new/ > @@ -1115,7 +1115,7 @@ ifneq ($(CONFIG_DM),y) > @echo >&2 "====================================================" > endif > $(call deprecated,CONFIG_DM_USB CONFIG_OF_CONTROL CONFIG_BLK,\ > - USB,v2019.07,$(CONFIG_USB)) > + USB_HOST,v2019.07,$(CONFIG_USB_HOST)) > $(call deprecated,CONFIG_DM_PCI,PCI,v2019.07,$(CONFIG_PCI)) > $(call deprecated,CONFIG_DM_VIDEO,video,v2019.07,\ > $(CONFIG_LCD)$(CONFIG_VIDEO)) > diff --git a/cmd/Kconfig b/cmd/Kconfig > index a9fb4eead29b..a13a68f8f333 100644 > --- a/cmd/Kconfig > +++ b/cmd/Kconfig > @@ -1322,7 +1322,7 @@ config CMD_UNIVERSE > > config CMD_USB > bool "usb" > - depends on USB > + depends on USB_HOST > select HAVE_BLOCK_DEVICE > help > USB support. > > and this is mostly right, but there's a few host drivers that don't > depends on USB_HOST like they should. But it still leaves nokia_rx51 > with piles of technical debt. There's just no screaming warnings. > > What's the best way to get attention drawn to all of the bits of > technical debt that are left, and not just on this platform? I don't > know. I was hoping the build time warnings we have would have helped, > but as shown by this case here, they have not. And honestly, this isn't > the first platform to near the deadline come up and ask for some sort of > reprieve. > > > In case of N900, no DT information is needed to be provided for USB gadget > > mode to work and hostmode code (that seems to require DT and stuff) is > > simply not needed. > > Because of using very old code, yes. > > > Also, could you comment on SPL and DM_USB, please see my other email. > > I did in this email. No one requires SPL_DM_USB && !SPL_OF_CONTROL > today, so no one has made it work. > > > > So can we please confirm at run time on the real hardware that there are > > > thumb problems, and some clever solution will need to be created here > > > (which will probably be introducing CONFIG_USB_HOST and migrating > > > CONFIG_USB_DEVICE to Kconfig and then someone from the N900 community > > > implementing DM_USB_GADGET without OF_CONTROL), or that no, thumb is OK > > > at runtime after all and the Kconfig cleanup isn't as urgent but still > > > would be good. > > > > And how exactly we are supposed to confirm if thumb2 is ok or not? Do you > > have a good test-case? My gut feeling tells me thumb2 is fine, but I can > > neither prove nor disprove it. NOLO itself uses thumb2 code, so I guess we > > can assume it to be safe to use it for u-boot. > > Does it boot to Linux? That's really all we check for with > test/nokia_rx51_test.sh but I wish it would get hooked up to also run > our pytest framework. That framework can also be used on real hardware, > and running those tests would be real nice to see too. But that's all > an aside, at this point. > > > But still, the code size is not the main issue to me, it is that you seem to > > implicitly mandate something that is still not mandatory to my knowledge - > > migration to DT. Could you please at least define what is the deadline for > > us to finish with it? > > Well, DT migration isn't required if you update a given subsystem to be > able to be given the required platform information via another mechanism > such as how you're using U_BOOT_DRVINFOS today with i2c. But the > flipside is that subsystems do not have to support a non-DT way, and USB > does not currently have a non-DT way. Using U_BOOT_DRVINFOS is > discouraged when OF_CONTROL is possible instead. > > And I want to say finally, I've redrafted this email a few times now, > and I want to apologize now if this comes out cranky or mean, it's not > my intention.
Somewhat more broadly... IMO there is a lot of energy invested in discussions about migration. I feel that energy would be better invested in moving things forward. In other words, why so much discussion about whether to migrate X to the latest thing? Being a custodian of a board does have some responsibilities. Perhaps we need to have a dashboard for each maintainer, showing the things that need to be addressed? Something seems wrong with the current process, where every migration generates so much push-back and discussion. Regards, Simon
