| Message ID | 20210511225223.550762-1-mka@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | USB: misc: Add onboard_usb_hub driver | expand |
Hello Matthias, just a curious informal question, see below. Am Tue, May 11, 2021 at 03:52:18PM -0700 schrieb Matthias Kaehlcke: > This series adds: > - the onboard_usb_hub_driver > - glue in the xhci-plat driver to create the onboard_usb_hub > platform device if needed > - a device tree binding for the Realtek RTS5411 USB hub controller > - device tree changes that add RTS5411 entries for the QCA SC7180 > based boards trogdor and lazor > - a couple of stubs for platform device functions to avoid > unresolved symbols with certain kernel configs > > The main issue the driver addresses is that a USB hub needs to be > powered before it can be discovered. For discrete onboard hubs (an > example for such a hub is the Realtek RTS5411) this is often solved > by supplying the hub with an 'always-on' regulator, which is kind > of a hack. Some onboard hubs may require further initialization > steps, like changing the state of a GPIO or enabling a clock, which > requires even more hacks. This driver creates a platform device > representing the hub which performs the necessary initialization. > Currently it only supports switching on a single regulator, support > for multiple regulators or other actions can be added as needed. > Different initialization sequences can be supported based on the > compatible string. This sounds like it would be useful for other hub controllers as well? For example, would the Microchip USB3503 (former SMSC, drivers/usb/misc/usb3503.c, [1]) fall into this category? That chip is used on the "Cubietech Cubietruck Plus" for example. > Besides performing the initialization the driver can be configured > to power the hub off during system suspend. This can help to extend > battery life on battery powered devices which have no requirements > to keep the hub powered during suspend. The driver can also be > configured to leave the hub powered when a wakeup capable USB device > is connected when suspending, and power it off otherwise. Sounds interesting. Greets Alex [1] https://www.microchip.com/wwwproducts/en/USB3503 > Changes in v10: > - always use of_is_onboard_usb_hub() stub unless ONBOARD_USB_HUB=y/m > - keep 'regulator-boot-on' property for pp3300_hub > > Changes in v9: > - added dependency on ONBOARD_USB_HUB (or !!ONBOARD_USB_HUB) to > USB_PLATFORM_XHCI > > Changes in v7: > - updated DT binding > - series rebased on qcom/arm64-for-5.13 > > Changes in v6: > - updated summary > > Changes in v5: > - cover letter added > > Matthias Kaehlcke (5): > dt-bindings: usb: Add binding for Realtek RTS5411 hub controller > USB: misc: Add onboard_usb_hub driver > of/platform: Add stubs for of_platform_device_create/destroy() > usb: host: xhci-plat: Create platform device for onboard hubs in > probe() > arm64: dts: qcom: sc7180-trogdor: Add nodes for onboard USB hub > > .../sysfs-bus-platform-onboard-usb-hub | 8 + > .../bindings/usb/realtek,rts5411.yaml | 62 +++ > MAINTAINERS | 7 + > .../boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 19 +- > .../boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 11 +- > .../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts | 19 +- > arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 19 +- > drivers/usb/host/Kconfig | 1 + > drivers/usb/host/xhci-plat.c | 16 + > drivers/usb/misc/Kconfig | 17 + > drivers/usb/misc/Makefile | 1 + > drivers/usb/misc/onboard_usb_hub.c | 415 ++++++++++++++++++ > include/linux/of_platform.h | 22 +- > include/linux/usb/hcd.h | 2 + > include/linux/usb/onboard_hub.h | 15 + > 15 files changed, 600 insertions(+), 34 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub > create mode 100644 Documentation/devicetree/bindings/usb/realtek,rts5411.yaml > create mode 100644 drivers/usb/misc/onboard_usb_hub.c > create mode 100644 include/linux/usb/onboard_hub.h > > -- > 2.31.1.607.g51e8a6a459-goog >
Hi Alexander, On Wed, May 12, 2021 at 09:19:54AM +0200, Alexander Dahl wrote: > Hello Matthias, > > just a curious informal question, see below. > > Am Tue, May 11, 2021 at 03:52:18PM -0700 schrieb Matthias Kaehlcke: > > This series adds: > > - the onboard_usb_hub_driver > > - glue in the xhci-plat driver to create the onboard_usb_hub > > platform device if needed > > - a device tree binding for the Realtek RTS5411 USB hub controller > > - device tree changes that add RTS5411 entries for the QCA SC7180 > > based boards trogdor and lazor > > - a couple of stubs for platform device functions to avoid > > unresolved symbols with certain kernel configs > > > > The main issue the driver addresses is that a USB hub needs to be > > powered before it can be discovered. For discrete onboard hubs (an > > example for such a hub is the Realtek RTS5411) this is often solved > > by supplying the hub with an 'always-on' regulator, which is kind > > of a hack. Some onboard hubs may require further initialization > > steps, like changing the state of a GPIO or enabling a clock, which > > requires even more hacks. This driver creates a platform device > > representing the hub which performs the necessary initialization. > > Currently it only supports switching on a single regulator, support > > for multiple regulators or other actions can be added as needed. > > Different initialization sequences can be supported based on the > > compatible string. > > This sounds like it would be useful for other hub controllers as well? > For example, would the Microchip USB3503 (former SMSC, > drivers/usb/misc/usb3503.c, [1]) fall into this category? That chip is > used on the "Cubietech Cubietruck Plus" for example. usb3503.c provides two 'separate' USB3503 drivers (which share some code), a i2c client driver and a platform driver. IIUC on a system with an USB3503 only one of these drivers is used. Theoretically it should be feasible to extend the onboard_usb_hub driver to cover the functionality of the platform driver in usb3503.c (essentially to control GPIOs and clocks at initialization time and suspend/resume). Another question is whether that would be desirable, since the i2c and the platform driver share code, which then would be duplicated in the i2c and onboard_usb_hub driver, unless a way is found to keep sharing that code. The i2c driver can't be completely replaced by the onboard_usb_hub driver, due to the i2c communications. It might be possible to have the i2c driver and the onboard_usb_hub collaborate, however I expect it would take a certain effort to design and implement a solid solution. Thanks Matthias
On Tue, May 11, 2021 at 03:52:18PM -0700, Matthias Kaehlcke wrote: > This series adds: > - the onboard_usb_hub_driver > - glue in the xhci-plat driver to create the onboard_usb_hub > platform device if needed > - a device tree binding for the Realtek RTS5411 USB hub controller > - device tree changes that add RTS5411 entries for the QCA SC7180 > based boards trogdor and lazor > - a couple of stubs for platform device functions to avoid > unresolved symbols with certain kernel configs > > The main issue the driver addresses is that a USB hub needs to be > powered before it can be discovered. For discrete onboard hubs (an > example for such a hub is the Realtek RTS5411) this is often solved > by supplying the hub with an 'always-on' regulator, which is kind > of a hack. Some onboard hubs may require further initialization > steps, like changing the state of a GPIO or enabling a clock, which > requires even more hacks. This driver creates a platform device > representing the hub which performs the necessary initialization. > Currently it only supports switching on a single regulator, support > for multiple regulators or other actions can be added as needed. > Different initialization sequences can be supported based on the > compatible string. > > Besides performing the initialization the driver can be configured > to power the hub off during system suspend. This can help to extend > battery life on battery powered devices which have no requirements > to keep the hub powered during suspend. The driver can also be > configured to leave the hub powered when a wakeup capable USB device > is connected when suspending, and power it off otherwise. I get a build error when I apply this series to my tree: drivers/usb/misc/onboard_usb_hub.c:273:6: error: redefinition of ‘of_is_onboard_usb_hub’ 273 | bool of_is_onboard_usb_hub(const struct device_node *np) | ^~~~~~~~~~~~~~~~~~~~~ In file included from drivers/usb/misc/onboard_usb_hub.c:21: ./include/linux/usb/onboard_hub.h:9:20: note: previous definition of ‘of_is_onboard_usb_hub’ with type ‘bool(const struct device_node *)’ {aka ‘_Bool(const struct device_node *)’} 9 | static inline bool of_is_onboard_usb_hub(const struct device_node *np) | ^~~~~~~~~~~~~~~~~~~~~ Any thoughts? greg k-h
On Fri, May 21, 2021 at 02:30:39PM +0200, Greg Kroah-Hartman wrote: > On Tue, May 11, 2021 at 03:52:18PM -0700, Matthias Kaehlcke wrote: > > This series adds: > > - the onboard_usb_hub_driver > > - glue in the xhci-plat driver to create the onboard_usb_hub > > platform device if needed > > - a device tree binding for the Realtek RTS5411 USB hub controller > > - device tree changes that add RTS5411 entries for the QCA SC7180 > > based boards trogdor and lazor > > - a couple of stubs for platform device functions to avoid > > unresolved symbols with certain kernel configs > > > > The main issue the driver addresses is that a USB hub needs to be > > powered before it can be discovered. For discrete onboard hubs (an > > example for such a hub is the Realtek RTS5411) this is often solved > > by supplying the hub with an 'always-on' regulator, which is kind > > of a hack. Some onboard hubs may require further initialization > > steps, like changing the state of a GPIO or enabling a clock, which > > requires even more hacks. This driver creates a platform device > > representing the hub which performs the necessary initialization. > > Currently it only supports switching on a single regulator, support > > for multiple regulators or other actions can be added as needed. > > Different initialization sequences can be supported based on the > > compatible string. > > > > Besides performing the initialization the driver can be configured > > to power the hub off during system suspend. This can help to extend > > battery life on battery powered devices which have no requirements > > to keep the hub powered during suspend. The driver can also be > > configured to leave the hub powered when a wakeup capable USB device > > is connected when suspending, and power it off otherwise. > > I get a build error when I apply this series to my tree: > > drivers/usb/misc/onboard_usb_hub.c:273:6: error: redefinition of ‘of_is_onboard_usb_hub’ > 273 | bool of_is_onboard_usb_hub(const struct device_node *np) > | ^~~~~~~~~~~~~~~~~~~~~ > In file included from drivers/usb/misc/onboard_usb_hub.c:21: > ./include/linux/usb/onboard_hub.h:9:20: note: previous definition of ‘of_is_onboard_usb_hub’ with type ‘bool(const struct device_node *)’ {aka ‘_Bool(const struct device_node *)’} > 9 | static inline bool of_is_onboard_usb_hub(const struct device_node *np) > | ^~~~~~~~~~~~~~~~~~~~~ > > Any thoughts? That function keeps haunting me in different ways ... I suspect this was a build with CONFIG_COMPILE_TEST=y. The driver is compiled for such a config and has the actual implementation, however the header defines the static inline unless CONFIG_USB_ONBOARD_HUB=y/m. I realized earlier that the driver doesn't really need to include the header, and planned to remove it in the next version, which might be the most practical solution.
This series adds: - the onboard_usb_hub_driver - glue in the xhci-plat driver to create the onboard_usb_hub platform device if needed - a device tree binding for the Realtek RTS5411 USB hub controller - device tree changes that add RTS5411 entries for the QCA SC7180 based boards trogdor and lazor - a couple of stubs for platform device functions to avoid unresolved symbols with certain kernel configs The main issue the driver addresses is that a USB hub needs to be powered before it can be discovered. For discrete onboard hubs (an example for such a hub is the Realtek RTS5411) this is often solved by supplying the hub with an 'always-on' regulator, which is kind of a hack. Some onboard hubs may require further initialization steps, like changing the state of a GPIO or enabling a clock, which requires even more hacks. This driver creates a platform device representing the hub which performs the necessary initialization. Currently it only supports switching on a single regulator, support for multiple regulators or other actions can be added as needed. Different initialization sequences can be supported based on the compatible string. Besides performing the initialization the driver can be configured to power the hub off during system suspend. This can help to extend battery life on battery powered devices which have no requirements to keep the hub powered during suspend. The driver can also be configured to leave the hub powered when a wakeup capable USB device is connected when suspending, and power it off otherwise. Changes in v10: - always use of_is_onboard_usb_hub() stub unless ONBOARD_USB_HUB=y/m - keep 'regulator-boot-on' property for pp3300_hub Changes in v9: - added dependency on ONBOARD_USB_HUB (or !!ONBOARD_USB_HUB) to USB_PLATFORM_XHCI Changes in v7: - updated DT binding - series rebased on qcom/arm64-for-5.13 Changes in v6: - updated summary Changes in v5: - cover letter added Matthias Kaehlcke (5): dt-bindings: usb: Add binding for Realtek RTS5411 hub controller USB: misc: Add onboard_usb_hub driver of/platform: Add stubs for of_platform_device_create/destroy() usb: host: xhci-plat: Create platform device for onboard hubs in probe() arm64: dts: qcom: sc7180-trogdor: Add nodes for onboard USB hub .../sysfs-bus-platform-onboard-usb-hub | 8 + .../bindings/usb/realtek,rts5411.yaml | 62 +++ MAINTAINERS | 7 + .../boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 19 +- .../boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 11 +- .../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts | 19 +- arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 19 +- drivers/usb/host/Kconfig | 1 + drivers/usb/host/xhci-plat.c | 16 + drivers/usb/misc/Kconfig | 17 + drivers/usb/misc/Makefile | 1 + drivers/usb/misc/onboard_usb_hub.c | 415 ++++++++++++++++++ include/linux/of_platform.h | 22 +- include/linux/usb/hcd.h | 2 + include/linux/usb/onboard_hub.h | 15 + 15 files changed, 600 insertions(+), 34 deletions(-) create mode 100644 Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub create mode 100644 Documentation/devicetree/bindings/usb/realtek,rts5411.yaml create mode 100644 drivers/usb/misc/onboard_usb_hub.c create mode 100644 include/linux/usb/onboard_hub.h