Message ID | 20240408140254.1645317-1-festevam@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ARM: multi_v7_defconfig: Select CONFIG_USB_ONBOARD_DEV as built-in | expand |
Hi Arnd, On Mon, Apr 8, 2024 at 11:03 AM Fabio Estevam <festevam@gmail.com> wrote: > > From: Fabio Estevam <festevam@denx.de> > > Selecting CONFIG_USB_ONBOARD_DEV as a module may cause the following > run-time error when probing a USB2514 hub on a imx6q-udoo board: > > usb 1-1: device not accepting address 5, error -71 > usb usb1-port1: unable to enumerate USB device > > Fix this issue by selecting CONFIG_USB_ONBOARD_DEV as built-in so > that the USB hub reset line and clock can be activated earlier. > > Signed-off-by: Fabio Estevam <festevam@denx.de> > --- > Here is the imx6q-udoo boot log from Mark's farm that shows the problem: > > https://storage.kernelci.org/next/master/next-20240408/arm/multi_v7_defconfig/gcc-10/lab-broonie/baseline-imx6dl-udoo.html A gentle ping.
Hi, Am Dienstag, 30. April 2024, 00:09:52 CEST schrieb Fabio Estevam: > Hi Arnd, > > On Mon, Apr 8, 2024 at 11:03 AM Fabio Estevam <festevam@gmail.com> wrote: > > > > From: Fabio Estevam <festevam@denx.de> > > > > Selecting CONFIG_USB_ONBOARD_DEV as a module may cause the following > > run-time error when probing a USB2514 hub on a imx6q-udoo board: > > > > usb 1-1: device not accepting address 5, error -71 > > usb usb1-port1: unable to enumerate USB device > > > > Fix this issue by selecting CONFIG_USB_ONBOARD_DEV as built-in so > > that the USB hub reset line and clock can be activated earlier. > > > > Signed-off-by: Fabio Estevam <festevam@denx.de> > > --- > > Here is the imx6q-udoo boot log from Mark's farm that shows the problem: > > > > https://storage.kernelci.org/next/master/next-20240408/arm/multi_v7_defconfig/gcc-10/lab-broonie/baseline-imx6dl-udoo.html > > A gentle ping. Despite the change to the defconfig. If this driver causes problems when being built as module, shouldn't it be bool instead of tristate then? Best regards, Alexander
On Tue, Apr 30, 2024, at 08:28, Alexander Stein wrote: > Am Dienstag, 30. April 2024, 00:09:52 CEST schrieb Fabio Estevam: >> Hi Arnd, >> >> On Mon, Apr 8, 2024 at 11:03 AM Fabio Estevam <festevam@gmail.com> wrote: >> > >> > From: Fabio Estevam <festevam@denx.de> >> > >> > Selecting CONFIG_USB_ONBOARD_DEV as a module may cause the following >> > run-time error when probing a USB2514 hub on a imx6q-udoo board: >> > >> > usb 1-1: device not accepting address 5, error -71 >> > usb usb1-port1: unable to enumerate USB device >> > >> > Fix this issue by selecting CONFIG_USB_ONBOARD_DEV as built-in so >> > that the USB hub reset line and clock can be activated earlier. >> > >> > Signed-off-by: Fabio Estevam <festevam@denx.de> >> > --- >> > Here is the imx6q-udoo boot log from Mark's farm that shows the problem: >> > >> > https://storage.kernelci.org/next/master/next-20240408/arm/multi_v7_defconfig/gcc-10/lab-broonie/baseline-imx6dl-udoo.html > > Despite the change to the defconfig. If this driver causes problems when > being built as module, shouldn't it be bool instead of tristate then? It does sound like this is something the kernel should be able to get to work properly in some form, but I don't think making it a 'bool' symbol is the correct answer here: if CONFIG_USB is set to =m, it would be impossible to include USB_ONBOARD_DEV in this case. Fabio, can you explain how making it built-in addresses the problem here? I assume this is related to probe order, so I wonder if it's just a matter of making the usb hub driver properly handle -EPROBE_DEFER until the onboard dev has been initialized. Arnd
On Tue, Apr 30, 2024 at 4:53 AM Arnd Bergmann <arnd@arndb.de> wrote: > It does sound like this is something the kernel should be able to > get to work properly in some form, but I don't think making it > a 'bool' symbol is the correct answer here: if CONFIG_USB is > set to =m, it would be impossible to include USB_ONBOARD_DEV > in this case. > > Fabio, can you explain how making it built-in addresses the > problem here? I assume this is related to probe order, so I > wonder if it's just a matter of making the usb hub driver > properly handle -EPROBE_DEFER until the onboard dev has been > initialized. From drivers/usb/misc/Kconfig: "config USB_ONBOARD_DEV tristate "Onboard USB device support" ..... This driver can be used as a module but its state (module vs builtin) must match the state of the USB subsystem. Enabling this config will enable the driver and it will automatically match the state of the USB subsystem. If this driver is a module it will be called onboard_usb_dev." In multi_v7_defconfig: CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m, so there is a mismatch. My patch enforces CONFIG_USB_ONBOARD_DEV=y to guarantee the matching. Is there any other way to solve this?
On Tue, Apr 30, 2024, at 21:53, Fabio Estevam wrote: > On Tue, Apr 30, 2024 at 4:53 AM Arnd Bergmann <arnd@arndb.de> wrote: > >> It does sound like this is something the kernel should be able to >> get to work properly in some form, but I don't think making it >> a 'bool' symbol is the correct answer here: if CONFIG_USB is >> set to =m, it would be impossible to include USB_ONBOARD_DEV >> in this case. >> >> Fabio, can you explain how making it built-in addresses the >> problem here? I assume this is related to probe order, so I >> wonder if it's just a matter of making the usb hub driver >> properly handle -EPROBE_DEFER until the onboard dev has been >> initialized. > > From drivers/usb/misc/Kconfig: > > "config USB_ONBOARD_DEV > tristate "Onboard USB device support" > ..... > This driver can be used as a module but its state (module vs > builtin) must match the state of the USB subsystem. Enabling > this config will enable the driver and it will automatically > match the state of the USB subsystem. If this driver is a > module it will be called onboard_usb_dev." Ok, so there is some kind of design mistake here that this is trying to paper over. Kbuild should always be powerful enough to enforce these things without having a person read a comment though. > In multi_v7_defconfig: > CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m, so there is a mismatch. > > My patch enforces CONFIG_USB_ONBOARD_DEV=y to guarantee the matching. > > Is there any other way to solve this? I can think of multiple ways to enforce this in Kbuild: a) make CONFIG_USB_ONBOARD_DEV a 'bool' symbol and then add a hidden symbol that duplicates the state of CONFIG_USB when USB_ONBOARD_DEV is enabled: config CONFIG_USB_ONBOARD_DEV_MODULE def_tristate USB depends on CONFIG_USB_ONBOARD_DEV b) Do the same thing but use Makefile syntax instead of Kconfig syntax: usb-onboard-dev-$(CONFIG_USB_ONBOARD_DEV) += onboard_usb_dev.o obj-$(CONFIG_USB) += usb-onboard-dev.o (or something close to this, need to try). However, I still feel there should be a better way to solve the problem at the code level rather than the Kbuild level. I don't know exactly how a "usb_device_driver" (as opposed to a "usb_driver") works here, but from what I can tell, the idea here is that the USB bus probe finds a device, matches the driver and then continues probing it. If the onboard-dev driver gets loaded after the initial bus probe, that driver won't exist yet and it then runs into some error that prevents it from being retried when after the onboard-dev driver is there. If you can pinpoint exactly what error it runs into, try to make it retry the same thing after -EPROBE_DEFER. Arnd
Adding Matthias, the onboard usb hub driver maintainer. On Tue, Apr 30, 2024 at 5:25 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Tue, Apr 30, 2024, at 21:53, Fabio Estevam wrote: > > On Tue, Apr 30, 2024 at 4:53 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > >> It does sound like this is something the kernel should be able to > >> get to work properly in some form, but I don't think making it > >> a 'bool' symbol is the correct answer here: if CONFIG_USB is > >> set to =m, it would be impossible to include USB_ONBOARD_DEV > >> in this case. > >> > >> Fabio, can you explain how making it built-in addresses the > >> problem here? I assume this is related to probe order, so I > >> wonder if it's just a matter of making the usb hub driver > >> properly handle -EPROBE_DEFER until the onboard dev has been > >> initialized. > > > > From drivers/usb/misc/Kconfig: > > > > "config USB_ONBOARD_DEV > > tristate "Onboard USB device support" > > ..... > > This driver can be used as a module but its state (module vs > > builtin) must match the state of the USB subsystem. Enabling > > this config will enable the driver and it will automatically > > match the state of the USB subsystem. If this driver is a > > module it will be called onboard_usb_dev." > > Ok, so there is some kind of design mistake here that this > is trying to paper over. Kbuild should always be powerful > enough to enforce these things without having a person read > a comment though. > > > In multi_v7_defconfig: > > CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m, so there is a mismatch. > > > > My patch enforces CONFIG_USB_ONBOARD_DEV=y to guarantee the matching. > > > > Is there any other way to solve this? > > I can think of multiple ways to enforce this in Kbuild: > > a) make CONFIG_USB_ONBOARD_DEV a 'bool' symbol and then add > a hidden symbol that duplicates the state of CONFIG_USB when > USB_ONBOARD_DEV is enabled: > > config CONFIG_USB_ONBOARD_DEV_MODULE > def_tristate USB > depends on CONFIG_USB_ONBOARD_DEV > > b) Do the same thing but use Makefile syntax instead of Kconfig > syntax: > > usb-onboard-dev-$(CONFIG_USB_ONBOARD_DEV) += onboard_usb_dev.o > obj-$(CONFIG_USB) += usb-onboard-dev.o > > (or something close to this, need to try). > > > However, I still feel there should be a better way to solve > the problem at the code level rather than the Kbuild level. > > I don't know exactly how a "usb_device_driver" (as opposed > to a "usb_driver") works here, but from what I can tell, > the idea here is that the USB bus probe finds a device, matches > the driver and then continues probing it. If the onboard-dev > driver gets loaded after the initial bus probe, that driver > won't exist yet and it then runs into some error that > prevents it from being retried when after the onboard-dev > driver is there. If you can pinpoint exactly what error it > runs into, try to make it retry the same thing after > -EPROBE_DEFER. > > Arnd
On Wed, May 1, 2024 at 4:04 PM Fabio Estevam <festevam@gmail.com> wrote: > > Adding Matthias, the onboard usb hub driver maintainer. > > On Tue, Apr 30, 2024 at 5:25 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Tue, Apr 30, 2024, at 21:53, Fabio Estevam wrote: > > > On Tue, Apr 30, 2024 at 4:53 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > >> It does sound like this is something the kernel should be able to > > >> get to work properly in some form, but I don't think making it > > >> a 'bool' symbol is the correct answer here: if CONFIG_USB is > > >> set to =m, it would be impossible to include USB_ONBOARD_DEV > > >> in this case. > > >> > > >> Fabio, can you explain how making it built-in addresses the > > >> problem here? I assume this is related to probe order, so I > > >> wonder if it's just a matter of making the usb hub driver > > >> properly handle -EPROBE_DEFER until the onboard dev has been > > >> initialized. > > > > > > From drivers/usb/misc/Kconfig: > > > > > > "config USB_ONBOARD_DEV > > > tristate "Onboard USB device support" > > > ..... > > > This driver can be used as a module but its state (module vs > > > builtin) must match the state of the USB subsystem. Enabling > > > this config will enable the driver and it will automatically > > > match the state of the USB subsystem. If this driver is a > > > module it will be called onboard_usb_dev." > > > > Ok, so there is some kind of design mistake here that this > > is trying to paper over. Kbuild should always be powerful > > enough to enforce these things without having a person read > > a comment though. Agreed that it would be preferable to enforce this at Kbuild level (or address it otherwise without having a person to read the comment). Kconfig *build* dependencies (mutual dependencies between the USB core and the onboard_hub/dev driver) were a major headache that stalled landing the driver for a long time, with at least one revert after landing. The change log of the final version that landed reflects some of that ordeal: https://lore.kernel.org/linux-usb/20220630123445.v24.3.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/ Eventually the build dependencies were fixed, but apparently there is still a runtime issue :( > > > In multi_v7_defconfig: > > > CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m, so there is a mismatch. > > > > > > My patch enforces CONFIG_USB_ONBOARD_DEV=y to guarantee the matching. > > > > > > Is there any other way to solve this? > > > > I can think of multiple ways to enforce this in Kbuild: > > > > a) make CONFIG_USB_ONBOARD_DEV a 'bool' symbol and then add > > a hidden symbol that duplicates the state of CONFIG_USB when > > USB_ONBOARD_DEV is enabled: > > > > config CONFIG_USB_ONBOARD_DEV_MODULE > > def_tristate USB > > depends on CONFIG_USB_ONBOARD_DEV > > An intermediate version of the driver had something similar: config USB_ONBOARD_HUB bool "Onboard USB hub support" if USB_ONBOARD_HUB config USB_ONBOARD_HUB_ACTUAL tristate default m if USB=m default y if USB=y endif https://lore.kernel.org/linux-usb/20220609121838.v22.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/ But it was removed later since the *build* dependency issues were resolved otherwise. We could add a construct like that again if we can't come up with a better solution. > > b) Do the same thing but use Makefile syntax instead of Kconfig > > syntax: > > > > usb-onboard-dev-$(CONFIG_USB_ONBOARD_DEV) += onboard_usb_dev.o > > obj-$(CONFIG_USB) += usb-onboard-dev.o > > > > (or something close to this, need to try). > > > > > > However, I still feel there should be a better way to solve > > the problem at the code level rather than the Kbuild level. > > > > I don't know exactly how a "usb_device_driver" (as opposed > > to a "usb_driver") works here, but from what I can tell, > > the idea here is that the USB bus probe finds a device, matches > > the driver and then continues probing it. If the onboard-dev > > driver gets loaded after the initial bus probe, that driver > > won't exist yet and it then runs into some error that > > prevents it from being retried when after the onboard-dev > > driver is there. If you can pinpoint exactly what error it > > runs into, try to make it retry the same thing after > > -EPROBE_DEFER. > > > > Arnd
On Thu, May 2, 2024, at 16:49, Matthias Kaehlcke wrote: > On Wed, May 1, 2024 at 4:04 PM Fabio Estevam <festevam@gmail.com> wrote: >> On Tue, Apr 30, 2024 at 5:25 PM Arnd Bergmann <arnd@arndb.de> wrote: >> > >> > On Tue, Apr 30, 2024, at 21:53, Fabio Estevam wrote: >> > > On Tue, Apr 30, 2024 at 4:53 AM Arnd Bergmann <arnd@arndb.de> wrote: >> > > >> > >> It does sound like this is something the kernel should be able to >> > >> get to work properly in some form, but I don't think making it >> > >> a 'bool' symbol is the correct answer here: if CONFIG_USB is >> > >> set to =m, it would be impossible to include USB_ONBOARD_DEV >> > >> in this case. >> > >> >> > >> Fabio, can you explain how making it built-in addresses the >> > >> problem here? I assume this is related to probe order, so I >> > >> wonder if it's just a matter of making the usb hub driver >> > >> properly handle -EPROBE_DEFER until the onboard dev has been >> > >> initialized. >> > > >> > > From drivers/usb/misc/Kconfig: >> > > >> > > "config USB_ONBOARD_DEV >> > > tristate "Onboard USB device support" >> > > ..... >> > > This driver can be used as a module but its state (module vs >> > > builtin) must match the state of the USB subsystem. Enabling >> > > this config will enable the driver and it will automatically >> > > match the state of the USB subsystem. If this driver is a >> > > module it will be called onboard_usb_dev." >> > >> > Ok, so there is some kind of design mistake here that this >> > is trying to paper over. Kbuild should always be powerful >> > enough to enforce these things without having a person read >> > a comment though. > > Agreed that it would be preferable to enforce this at Kbuild level (or > address it otherwise without having a person to read the comment). > > Kconfig *build* dependencies (mutual dependencies between the USB core > and the onboard_hub/dev driver) were a major headache that stalled > landing the driver for a long time, with at least one revert after > landing. > > The change log of the final version that landed reflects some of that ordeal: > > https://lore.kernel.org/linux-usb/20220630123445.v24.3.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/ > > Eventually the build dependencies were fixed, but apparently there is > still a runtime issue :( I can see what the link time problem was and how it's currently being worked around. It would be nice to have something better than the ifdef CONFIG_USB_ONBOARD_DEV usbcore-y += ../misc/onboard_usb_dev_pdevs.o endif bit, but this bit is obviously not the problem here. >> > > In multi_v7_defconfig: >> > > CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m, so there is a mismatch. >> > > >> > > My patch enforces CONFIG_USB_ONBOARD_DEV=y to guarantee the matching. >> > > >> > > Is there any other way to solve this? >> > >> > I can think of multiple ways to enforce this in Kbuild: >> > >> > a) make CONFIG_USB_ONBOARD_DEV a 'bool' symbol and then add >> > a hidden symbol that duplicates the state of CONFIG_USB when >> > USB_ONBOARD_DEV is enabled: >> > >> > config CONFIG_USB_ONBOARD_DEV_MODULE >> > def_tristate USB >> > depends on CONFIG_USB_ONBOARD_DEV >> > > > An intermediate version of the driver had something similar: > > config USB_ONBOARD_HUB > bool "Onboard USB hub support" > > if USB_ONBOARD_HUB > config USB_ONBOARD_HUB_ACTUAL > tristate > default m if USB=m > default y if USB=y > endif I think this is exactly the same think I wrote above, just written in a more verbose way. > https://lore.kernel.org/linux-usb/20220609121838.v22.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/ > > But it was removed later since the *build* dependency issues were > resolved otherwise. > > We could add a construct like that again if we can't come up with a > better solution. The bit I still don't understand is why the current code is broken with CONFIG_USB=y CONFIG_USB_ONBOARD_DEV=m but not with CONFIG_USB=m. Most likely there is a runtime dependency that requires the usb_onboard_dev module to be initialized before probing any USB host controller that is a parent of an onboard hub. This can work if you load the modules in the right order, but I don't see this being actually enforced, so why doesn't CONFIG_USB=m CONFIG_USB_ONBOARD_DEV=m fail if you load xhci first. If the probe deferral for the hub works correctly, why doesn't it work with USB=y? Arnd
On Thu, May 2, 2024 at 11:37 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, May 2, 2024, at 16:49, Matthias Kaehlcke wrote: > > On Wed, May 1, 2024 at 4:04 PM Fabio Estevam <festevam@gmail.com> wrote: > >> On Tue, Apr 30, 2024 at 5:25 PM Arnd Bergmann <arnd@arndb.de> wrote: > >> > > >> > On Tue, Apr 30, 2024, at 21:53, Fabio Estevam wrote: > >> > > On Tue, Apr 30, 2024 at 4:53 AM Arnd Bergmann <arnd@arndb.de> wrote: > >> > > > >> > >> It does sound like this is something the kernel should be able to > >> > >> get to work properly in some form, but I don't think making it > >> > >> a 'bool' symbol is the correct answer here: if CONFIG_USB is > >> > >> set to =m, it would be impossible to include USB_ONBOARD_DEV > >> > >> in this case. > >> > >> > >> > >> Fabio, can you explain how making it built-in addresses the > >> > >> problem here? I assume this is related to probe order, so I > >> > >> wonder if it's just a matter of making the usb hub driver > >> > >> properly handle -EPROBE_DEFER until the onboard dev has been > >> > >> initialized. > >> > > > >> > > From drivers/usb/misc/Kconfig: > >> > > > >> > > "config USB_ONBOARD_DEV > >> > > tristate "Onboard USB device support" > >> > > ..... > >> > > This driver can be used as a module but its state (module vs > >> > > builtin) must match the state of the USB subsystem. Enabling > >> > > this config will enable the driver and it will automatically > >> > > match the state of the USB subsystem. If this driver is a > >> > > module it will be called onboard_usb_dev." > >> > > >> > Ok, so there is some kind of design mistake here that this > >> > is trying to paper over. Kbuild should always be powerful > >> > enough to enforce these things without having a person read > >> > a comment though. > > > > Agreed that it would be preferable to enforce this at Kbuild level (or > > address it otherwise without having a person to read the comment). > > > > Kconfig *build* dependencies (mutual dependencies between the USB core > > and the onboard_hub/dev driver) were a major headache that stalled > > landing the driver for a long time, with at least one revert after > > landing. > > > > The change log of the final version that landed reflects some of that ordeal: > > > > https://lore.kernel.org/linux-usb/20220630123445.v24.3.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/ > > > > Eventually the build dependencies were fixed, but apparently there is > > still a runtime issue :( > > I can see what the link time problem was and how it's currently > being worked around. It would be nice to have something better > than the > > ifdef CONFIG_USB_ONBOARD_DEV > usbcore-y += ../misc/onboard_usb_dev_pdevs.o > endif > > bit, but this bit is obviously not the problem here. > > >> > > In multi_v7_defconfig: > >> > > CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m, so there is a mismatch. > >> > > > >> > > My patch enforces CONFIG_USB_ONBOARD_DEV=y to guarantee the matching. > >> > > > >> > > Is there any other way to solve this? > >> > > >> > I can think of multiple ways to enforce this in Kbuild: > >> > > >> > a) make CONFIG_USB_ONBOARD_DEV a 'bool' symbol and then add > >> > a hidden symbol that duplicates the state of CONFIG_USB when > >> > USB_ONBOARD_DEV is enabled: > >> > > >> > config CONFIG_USB_ONBOARD_DEV_MODULE > >> > def_tristate USB > >> > depends on CONFIG_USB_ONBOARD_DEV > >> > > > > > An intermediate version of the driver had something similar: > > > > config USB_ONBOARD_HUB > > bool "Onboard USB hub support" > > > > if USB_ONBOARD_HUB > > config USB_ONBOARD_HUB_ACTUAL > > tristate > > default m if USB=m > > default y if USB=y > > endif > > I think this is exactly the same think I wrote above, just written > in a more verbose way. Could be, I tried a few variants and recall one less verbose one didn't work as intended, but it could have been different from your suggestion. > > https://lore.kernel.org/linux-usb/20220609121838.v22.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid/ > > > > But it was removed later since the *build* dependency issues were > > resolved otherwise. > > > > We could add a construct like that again if we can't come up with a > > better solution. > > The bit I still don't understand is why the current code is > broken with CONFIG_USB=y CONFIG_USB_ONBOARD_DEV=m but not > with CONFIG_USB=m. > > Most likely there is a runtime dependency that requires the > usb_onboard_dev module to be initialized before probing > any USB host controller that is a parent of an onboard hub. Indeed, there is a dependency: Even though the onboard_usb_dev driver lives under drivers/usb its core component is not a USB driver but a platform driver. The platform driver is in charge of powering the onboard USB device on, so that the parent hub can probe it. hub_probe() calls onboard_dev_create_pdevs(), which creates platform devices for eligible USB devices with a device tree node. onboard_dev_create_pdevs() is linked into the USB core to avoid the aforementioned build dependency issues. IIUC the creation of the platform device(s) should trigger the onboard_usb_dev driver being loaded (if it wasn't already), which in turn would result in an invocation of onboard_dev_probe(), which would power the onboard USB device on. It is also not clear to me why things would be broken with CONFIG_USB=y CONFIG_USB_ONBOARD_DEV=m, but not with CONFIG_USB=m. It doesn't seem to be an universal issue, I can't repro it locally. > This can work if you load the modules in the right order, but > I don't see this being actually enforced, so why doesn't > CONFIG_USB=m CONFIG_USB_ONBOARD_DEV=m fail if you load > xhci first. If the probe deferral for the hub works correctly, > why doesn't it work with USB=y? > > Arnd
Hi Matthias, On Thu, May 2, 2024 at 6:45 PM Matthias Kaehlcke <mka@chromium.org> wrote: > It is also not clear to me why things would be broken with > CONFIG_USB=y CONFIG_USB_ONBOARD_DEV=m, but not with CONFIG_USB=m. It > doesn't seem to be an universal issue, I can't repro it locally. This issue happens on an imx6q-udoo board. multi_v7_defconfig has CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m: https://storage.kernelci.org/next/master/next-20240502/arm/multi_v7_defconfig+CONFIG_THUMB2_KERNEL=y/gcc-10/lab-broonie/baseline-imx6q-udoo.html usb 1-1: device descriptor read/64, error -71 usb 1-1: device descriptor read/64, error -71 usb 1-1: new full-speed USB device number 3 using ci_hdrc usb 1-1: device descriptor read/64, error -71 usb 1-1: device descriptor read/64, error -71 usb usb1-port1: attempt power cycle usb 1-1: new full-speed USB device number 4 using ci_hdrc usb 1-1: device not accepting address 4, error -71 usb 1-1: new full-speed USB device number 5 using ci_hdrc usb 1-1: device not accepting address 5, error -71 usb usb1-port1: unable to enumerate USB device imx_v6_v7_defconfig has CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=y: https://storage.kernelci.org/next/master/next-20240502/arm/imx_v6_v7_defconfig/gcc-10/lab-broonie/baseline-imx6q-udoo.html In this case, the USB Wifi chip connected to the USB2514 hub is correctly detected: usb 1-1.3: new high-speed USB device number 3 using ci_hdrc usb 1-1.3: New USB device found, idVendor=148f, idProduct=5370, bcdDevice= 1.01 usb 1-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3 usb 1-1.3: Product: 802.11 n WLAN usb 1-1.3: Manufacturer: Ralink usb 1-1.3: SerialNumber: 1.0
On Thu, May 2, 2024 at 3:59 PM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Matthias, > > On Thu, May 2, 2024 at 6:45 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > It is also not clear to me why things would be broken with > > CONFIG_USB=y CONFIG_USB_ONBOARD_DEV=m, but not with CONFIG_USB=m. It > > doesn't seem to be an universal issue, I can't repro it locally. > > This issue happens on an imx6q-udoo board. > > multi_v7_defconfig has CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m: > https://storage.kernelci.org/next/master/next-20240502/arm/multi_v7_defconfig+CONFIG_THUMB2_KERNEL=y/gcc-10/lab-broonie/baseline-imx6q-udoo.html > > usb 1-1: device descriptor read/64, error -71 > usb 1-1: device descriptor read/64, error -71 > usb 1-1: new full-speed USB device number 3 using ci_hdrc > usb 1-1: device descriptor read/64, error -71 > usb 1-1: device descriptor read/64, error -71 > usb usb1-port1: attempt power cycle > usb 1-1: new full-speed USB device number 4 using ci_hdrc > usb 1-1: device not accepting address 4, error -71 > usb 1-1: new full-speed USB device number 5 using ci_hdrc > usb 1-1: device not accepting address 5, error -71 > usb usb1-port1: unable to enumerate USB device > > imx_v6_v7_defconfig has CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=y: > https://storage.kernelci.org/next/master/next-20240502/arm/imx_v6_v7_defconfig/gcc-10/lab-broonie/baseline-imx6q-udoo.html > > In this case, the USB Wifi chip connected to the USB2514 hub is > correctly detected: > > usb 1-1.3: new high-speed USB device number 3 using ci_hdrc > usb 1-1.3: New USB device found, idVendor=148f, idProduct=5370, bcdDevice= 1.01 > usb 1-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3 > usb 1-1.3: Product: 802.11 n WLAN > usb 1-1.3: Manufacturer: Ralink > usb 1-1.3: SerialNumber: 1.0 Ah, from the discussion my impression was that CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m works, but not CONFIG_USB=m and CONFIG_USB_ONBOARD_DEV=m, good we clarified that. Is the rootfs by chance on a USB device and that prevents the onboard_usb_dev module from being loaded? You could add debug logs to hub_probe(), onboard_dev_init() and onboard_dev_probe() to get more information.
On Fri, May 3, 2024 at 2:16 PM Matthias Kaehlcke <mka@chromium.org> wrote: > Ah, from the discussion my impression was that CONFIG_USB=y and > CONFIG_USB_ONBOARD_DEV=m works, but not CONFIG_USB=m and > CONFIG_USB_ONBOARD_DEV=m, good we clarified that. > > Is the rootfs by chance on a USB device and that prevents the > onboard_usb_dev module from being loaded? No, the logs show the rootfs come from a ramdisk: root=/dev/ram0.
On Thu, May 02, 2024 at 07:58:57PM -0300, Fabio Estevam wrote: > Hi Matthias, > > On Thu, May 2, 2024 at 6:45 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > It is also not clear to me why things would be broken with > > CONFIG_USB=y CONFIG_USB_ONBOARD_DEV=m, but not with CONFIG_USB=m. It > > doesn't seem to be an universal issue, I can't repro it locally. > > This issue happens on an imx6q-udoo board. > > multi_v7_defconfig has CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=m: > https://storage.kernelci.org/next/master/next-20240502/arm/multi_v7_defconfig+CONFIG_THUMB2_KERNEL=y/gcc-10/lab-broonie/baseline-imx6q-udoo.html > > usb 1-1: device descriptor read/64, error -71 > usb 1-1: device descriptor read/64, error -71 > usb 1-1: new full-speed USB device number 3 using ci_hdrc > usb 1-1: device descriptor read/64, error -71 > usb 1-1: device descriptor read/64, error -71 > usb usb1-port1: attempt power cycle > usb 1-1: new full-speed USB device number 4 using ci_hdrc > usb 1-1: device not accepting address 4, error -71 > usb 1-1: new full-speed USB device number 5 using ci_hdrc > usb 1-1: device not accepting address 5, error -71 > usb usb1-port1: unable to enumerate USB device > > imx_v6_v7_defconfig has CONFIG_USB=y and CONFIG_USB_ONBOARD_DEV=y: > https://storage.kernelci.org/next/master/next-20240502/arm/imx_v6_v7_defconfig/gcc-10/lab-broonie/baseline-imx6q-udoo.html > > In this case, the USB Wifi chip connected to the USB2514 hub is > correctly detected: > > usb 1-1.3: new high-speed USB device number 3 using ci_hdrc > usb 1-1.3: New USB device found, idVendor=148f, idProduct=5370, bcdDevice= 1.01 > usb 1-1.3: New USB device strings: Mfr=1, Product=2, SerialNumber=3 > usb 1-1.3: Product: 802.11 n WLAN > usb 1-1.3: Manufacturer: Ralink > usb 1-1.3: SerialNumber: 1.0 Here are some debug logs from my side with CONFIG_USB_ONBOARD_DEV=m: [ 0.755965] DBG: hub_probe: adding onboard pdevs [ 0.756204] DBG: hub_probe: done [ 0.756618] DBG: hub_probe: adding onboard pdevs [ 0.756621] DBG: hub_probe: done [ 8.094539] DBG: onboard_dev_init [ 9.141729] DBG: onboard_dev_probe [ 9.142237] DBG: onboard_dev_probe (done) [ 9.142428] DBG: onboard_dev_init (done) The root hub adds the onboard pdev at 0.75..., but the onboard_dev module is only loaded more than 7s later (and probed even later). In the meantime there are no errors of failed enumerations as seen on the imx6q-udoo. I wonder if the problem could be that the sense resistors of the hub on the imx6q-udoo are always active and not only when the hub is powered, indicating the controller the presence of a device, which results in an attempt to enumerate it.
On Fri, May 3, 2024 at 5:11 PM Matthias Kaehlcke <mka@chromium.org> wrote: > Here are some debug logs from my side with CONFIG_USB_ONBOARD_DEV=m: > > [ 0.755965] DBG: hub_probe: adding onboard pdevs > [ 0.756204] DBG: hub_probe: done > [ 0.756618] DBG: hub_probe: adding onboard pdevs > [ 0.756621] DBG: hub_probe: done > [ 8.094539] DBG: onboard_dev_init > [ 9.141729] DBG: onboard_dev_probe > [ 9.142237] DBG: onboard_dev_probe (done) > [ 9.142428] DBG: onboard_dev_init (done) > > The root hub adds the onboard pdev at 0.75..., but the onboard_dev > module is only loaded more than 7s later (and probed even later). In > the meantime there are no errors of failed enumerations as seen on > the imx6q-udoo. Thanks for investigating. I haven't had a chance to extract these logs on the imx6q-udoo board yet. > I wonder if the problem could be that the sense resistors of the hub > on the imx6q-udoo are always active and not only when the hub is > powered, indicating the controller the presence of a device, which > results in an attempt to enumerate it. The imx6q-udoo schematics are here: https://udoo.org/download/files/UDOO_QUAD-DUAL/schematics/UDOO_QUAD-DUAL_REV_D_schematics.pdf
On Fri, May 03, 2024 at 06:13:32PM -0300, Fabio Estevam wrote: > On Fri, May 3, 2024 at 5:11 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > Here are some debug logs from my side with CONFIG_USB_ONBOARD_DEV=m: > > > > [ 0.755965] DBG: hub_probe: adding onboard pdevs > > [ 0.756204] DBG: hub_probe: done > > [ 0.756618] DBG: hub_probe: adding onboard pdevs > > [ 0.756621] DBG: hub_probe: done > > [ 8.094539] DBG: onboard_dev_init > > [ 9.141729] DBG: onboard_dev_probe > > [ 9.142237] DBG: onboard_dev_probe (done) > > [ 9.142428] DBG: onboard_dev_init (done) > > > > The root hub adds the onboard pdev at 0.75..., but the onboard_dev > > module is only loaded more than 7s later (and probed even later). In > > the meantime there are no errors of failed enumerations as seen on > > the imx6q-udoo. > > Thanks for investigating. > > I haven't had a chance to extract these logs on the imx6q-udoo board yet. > > > I wonder if the problem could be that the sense resistors of the hub > > on the imx6q-udoo are always active and not only when the hub is > > powered, indicating the controller the presence of a device, which > > results in an attempt to enumerate it. > > The imx6q-udoo schematics are here: > https://udoo.org/download/files/UDOO_QUAD-DUAL/schematics/UDOO_QUAD-DUAL_REV_D_schematics.pdf There are no external pull-ups on USB_H1_DP/USB_H1_DN, maybe internal pulls are enabled when the hub isn't powered?
On Mon, May 06, 2024 at 02:54:12PM +0000, Matthias Kaehlcke wrote: > On Fri, May 03, 2024 at 06:13:32PM -0300, Fabio Estevam wrote: > > On Fri, May 3, 2024 at 5:11 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > I wonder if the problem could be that the sense resistors of the hub > > > on the imx6q-udoo are always active and not only when the hub is > > > powered, indicating the controller the presence of a device, which > > > results in an attempt to enumerate it. > > The imx6q-udoo schematics are here: > > https://udoo.org/download/files/UDOO_QUAD-DUAL/schematics/UDOO_QUAD-DUAL_REV_D_schematics.pdf > There are no external pull-ups on USB_H1_DP/USB_H1_DN, maybe internal > pulls are enabled when the hub isn't powered? I do note that the vendor BSP worked (and I think also some older mainline versions too?).
On Tue, May 07, 2024 at 12:04:27AM +0900, Mark Brown wrote: > On Mon, May 06, 2024 at 02:54:12PM +0000, Matthias Kaehlcke wrote: > > On Fri, May 03, 2024 at 06:13:32PM -0300, Fabio Estevam wrote: > > > On Fri, May 3, 2024 at 5:11 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > I wonder if the problem could be that the sense resistors of the hub > > > > on the imx6q-udoo are always active and not only when the hub is > > > > powered, indicating the controller the presence of a device, which > > > > results in an attempt to enumerate it. > > > > The imx6q-udoo schematics are here: > > > https://udoo.org/download/files/UDOO_QUAD-DUAL/schematics/UDOO_QUAD-DUAL_REV_D_schematics.pdf > > > There are no external pull-ups on USB_H1_DP/USB_H1_DN, maybe internal > > pulls are enabled when the hub isn't powered? > > I do note that the vendor BSP worked (and I think also some older > mainline versions too?). The official BSP releases pre-date the onboard_usb_dev/hub driver: https://www.udoo.org/resources-quad-dual/. I guess they use a fixed regulator hack to power the hub on at boot. Yup, upstream also still uses a regulator hack: reg_usb_h1_vbus: regulator-usb-h1-vbus { compatible = "regulator-fixed"; regulator-name = "usb_h1_vbus"; regulator-min-microvolt = <5000000>; regulator-max-microvolt = <5000000>; enable-active-high; startup-delay-us = <2>; /* USB2415 requires a POR of 1 us minimum */ gpio = <&gpio7 12 0>; }; arch/arm/boot/dts/nxp/imx/imx6qdl-udoo.dtsi
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig index 86bf057ac366..1cc5f2c1a0c6 100644 --- a/arch/arm/configs/multi_v7_defconfig +++ b/arch/arm/configs/multi_v7_defconfig @@ -885,7 +885,7 @@ CONFIG_USB_CHIPIDEA_UDC=y CONFIG_USB_CHIPIDEA_HOST=y CONFIG_USB_ISP1760=y CONFIG_USB_HSIC_USB3503=y -CONFIG_USB_ONBOARD_DEV=m +CONFIG_USB_ONBOARD_DEV=y CONFIG_AB8500_USB=y CONFIG_KEYSTONE_USB_PHY=m CONFIG_NOP_USB_XCEIV=y