[PATCH 1/2] DM_USB: allow building without OF_CONTROL
Tom Rini
trini at konsulko.com
Wed Jun 30 17:48:57 CEST 2021
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.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210630/865c8892/attachment.sig>
More information about the U-Boot
mailing list