This series adds:
- the onboard_usb_hub_driver
- glue in the xhci-plat driver to create and destroy the
onboard_usb_hub platform devices 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 v16:
- added patch 'ARM: configs: Explicitly enable USB_XHCI_PLATFORM
where needed' to keep arm32 defconfigs effectively unchanged
Changes in v15:
- adjusted dependencies of USB_DWC3_CORE to make sure it can only
be enabled when at least one of USB_DWC3_HOST, USB_DWC3_GADGET
or USB_DWC3_DUAL_ROLE is selectable
Changes in v14:
- rebased on top of v5.14-rc1
- dropped DT binding patch which landed in v5.13
Changes in v13:
- added patch "usb: Specify dependency on USB_XHCI_PLATFORM with
'depends on'" to the series to avoid Kconfig conflicts
- added patch "arm64: defconfig: Explicitly enable USB_XHCI_PLATFORM"
to the series to keep effective defconfig unchanged
Changes in v12:
- onboard_hub driver: use IS_ENABLED(CONFIG_USB_ONBOARD_HUB_MODULE)
in onboard_hub.h to also check for the driver built as module
- onboard_hub_driver: include onboard_hub.h again to make sure there
are prototype declarations for the public functions
Changes in v11:
- support multiple onboard hubs connected to the same parent
- don't include ‘onboard_hub.h’ from the onboard hub driver
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 (7):
usb: misc: Add onboard_usb_hub driver
of/platform: Add stubs for of_platform_device_create/destroy()
ARM: configs: Explicitly enable USB_XHCI_PLATFORM where needed
arm64: defconfig: Explicitly enable USB_XHCI_PLATFORM
usb: Specify dependencies on USB_XHCI_PLATFORM with 'depends on'
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 +
MAINTAINERS | 7 +
arch/arm/configs/exynos_defconfig | 1 +
arch/arm/configs/keystone_defconfig | 1 +
arch/arm/configs/multi_v7_defconfig | 1 +
arch/arm/configs/mvebu_v7_defconfig | 1 +
arch/arm/configs/omap2plus_defconfig | 1 +
arch/arm/configs/pxa_defconfig | 1 +
.../boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 19 +-
.../boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 12 +-
.../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts | 19 +-
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 19 +-
arch/arm64/configs/defconfig | 1 +
drivers/usb/cdns3/Kconfig | 2 +-
drivers/usb/dwc3/Kconfig | 5 +-
drivers/usb/host/Kconfig | 5 +-
drivers/usb/host/xhci-plat.c | 6 +
drivers/usb/host/xhci.h | 2 +
drivers/usb/misc/Kconfig | 17 +
drivers/usb/misc/Makefile | 1 +
drivers/usb/misc/onboard_usb_hub.c | 497 ++++++++++++++++++
include/linux/of_platform.h | 22 +-
include/linux/usb/onboard_hub.h | 18 +
23 files changed, 627 insertions(+), 39 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub
create mode 100644 drivers/usb/misc/onboard_usb_hub.c
create mode 100644 include/linux/usb/onboard_hub.h
--
2.33.0.rc1.237.g0d66db33f3-goog
The main issue this 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.
Technically the driver consists of two drivers, the platform driver
described above and a very thin USB driver that subclasses the
generic driver. The purpose of this driver is to provide the platform
driver with the USB devices corresponding to the hub(s) (a hub
controller may provide multiple 'logical' hubs, e.g. one to support
USB 2.0 and another for USB 3.x).
Note: the current series only supports hubs connected directly to
a root hub (through xhci-plat), support for other configurations
could be added if needed.
Co-developed-by: Ravi Chandra Sadineni <redacted>
Signed-off-by: Ravi Chandra Sadineni <redacted>
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
---
Changes in v16:
- none
Changes in v15:
- none
Changes in v14:
- none
Changes in v13:
- none
Changes in v12:
- use IS_ENABLED(CONFIG_USB_ONBOARD_HUB_MODULE) in onboard_hub.h to
also check for the driver built as module
- include onboard_hub.h again from the driver to make sure there are
prototype declarations for the public functions
- remove indentation from label in onboard_hub_create_pdevs()
Changes in v11:
- added onboard_hub_create/destroy_pdevs() helpers, to support multiple onboard
hubs connected to the same parent hub
- don't include ‘onboard_hub.h’ from the onboard hub driver
- updated commit message
- added ‘Acked-by' from Alan
Changes in v10:
- always use of_is_onboard_usb_hub() stub unless ONBOARD_USB_HUB=y/m
Changes in v9:
- none
Changes in v8:
- none
Changes in v7:
- don't declare stub for of_is_onboard_usb_hub() when
CONFIG_COMPILE_TEST is defined
Changes in v6:
- use 'companion-hub' to locate the platform device, instead of
scanning through the nodes of the parent
- added ABI documentation for 'always_powered_in_suspend'
- sysfs_emit() instead of sprintf() in always_powered_in_suspend_show()
- register sysfs attribute through driver.dev_groups
- evaluate return value of driver_attach() in _probe()
- use dev_warn() instead of WARN_ON() in _probe()
- include 'onboard_hub.h'
Changes in v5:
- the platform device is now instantiated from the same DT node
as the 'primary' USB hub device
- use the USB compatible strings for the platform device
- refactored _find_onboard_hub() to search the parents child
nodes for a platform device with a matching compatible string
- added exported function of_is_onboard_usb_hub() to allow other
drivers (like xhci_plat) to check if one of their child DT nodes
is a supported hub
- use late suspend to make sure info about wakeup enabled descendants
is updated
- call driver_attach() for the USB driver in onboard_hub_probe() to
make sure the driver is re-attached after the device_release_driver()
calls in onboard_hub_remove()
- renamed sysfs attribute 'power_off_in_suspend' to
'always_powered_in_suspend'
- added sysfs symlinks between platform device and USB devices
- marked 'onboard_hub_pm_ops' as __maybe_unused
- removed 'realtek' compatible string which is not needed at this
point
- fix log for regulator_disable() failure
Changes in v4:
- updated Kconfig documentation
- changed the loop in onboard_hub_remove() to release the hub lock
before unbinding the USB device and make self deadlock prevention
less clunky
- fixed return value in onboard_hub_usbdev_probe()
- added entry to MAINTAINERS file
Changes in v3:
- updated the commit message
- updated description in Kconfig
- remove include of 'core/usb.h'
- use 'is_powered_on' flag instead of 'has_wakeup_capable_descendants'
- added 'going_away' flag to struct onboard_hub
- don't allow adding new USB devices when the platform device is going away
- don't bother with deleting the list item in onboard_hub_remove_usbdev()
when the platform device is going away
- don't assume in onboard_hub_suspend() that all USB hub devices are
connected to the same controller
- removed unnecessary devm_kfree() from onboard_hub_remove_usbdev()
- fixed error handling in onboard_hub_remove_usbdev()
- use kstrtobool() instead of strtobool() in power_off_in_suspend_store()
- unbind USB devices in onboard_hub_remove() to avoid dangling references
to the platform device
- moved put_device() for platform device to _find_onboard_hub()
- changed return value of onboard_hub_remove_usbdev() to void
- evaluate return value of onboard_hub_add_usbdev()
- register 'power_off_in_suspend' as managed device attribute
- use USB_DEVICE macro instead manual initialization
- add unwinding to onboard_hub_init()
- updated MODULE_DESCRIPTION
- use module_init() instead of device_initcall()
Changes in v2:
- check wakeup enabled state of the USB controller instead of
using 'wakeup-source' property
- use sysfs attribute instead of DT property to determine if
the hub should be powered off at all during system suspend
- added missing brace in onboard_hub_suspend()
- updated commit message
- use pm_ptr for pm_ops as suggested by Alan
Changes in v1:
- renamed the driver to 'onboard_usb_hub'
- single file for platform and USB driver
- USB hub devices register with the platform device
- the DT includes a phandle of the platform device
- the platform device now controls when power is turned off
- the USB driver became a very thin subclass of the generic USB
driver
- enabled autosuspend support
.../sysfs-bus-platform-onboard-usb-hub | 8 +
MAINTAINERS | 7 +
drivers/usb/misc/Kconfig | 17 +
drivers/usb/misc/Makefile | 1 +
drivers/usb/misc/onboard_usb_hub.c | 497 ++++++++++++++++++
include/linux/usb/onboard_hub.h | 18 +
6 files changed, 548 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub
create mode 100644 drivers/usb/misc/onboard_usb_hub.c
create mode 100644 include/linux/usb/onboard_hub.h
@@ -0,0 +1,8 @@+What: /sys/bus/platform/devices/<dev>/always_powered_in_suspend+Date: March 2021+KernelVersion: 5.13+Contact: Matthias Kaehlcke <matthias@kaehlcke.net>+ linux-usb@vger.kernel.org+Description:+ (RW) Controls whether the USB hub remains always powered+ during system suspend or not.
@@ -0,0 +1,497 @@+// SPDX-License-Identifier: GPL-2.0-only+/*+*DriverforonboardUSBhubs+*+*Copyright(c)2020,GoogleLLC+*/++#include<linux/device.h>+#include<linux/init.h>+#include<linux/kernel.h>+#include<linux/list.h>+#include<linux/module.h>+#include<linux/mutex.h>+#include<linux/of.h>+#include<linux/of_platform.h>+#include<linux/platform_device.h>+#include<linux/regulator/consumer.h>+#include<linux/suspend.h>+#include<linux/sysfs.h>+#include<linux/usb.h>+#include<linux/usb/hcd.h>+#include<linux/usb/of.h>+#include<linux/usb/onboard_hub.h>++staticstructusb_device_driveronboard_hub_usbdev_driver;++/************************** Platform driver **************************/++structudev_node{+structusb_device*udev;+structlist_headlist;+};++structonboard_hub{+structregulator*vdd;+structdevice*dev;+boolalways_powered_in_suspend;+boolis_powered_on;+boolgoing_away;+structlist_headudev_list;+structmutexlock;+};++staticintonboard_hub_power_on(structonboard_hub*hub)+{+interr;++err=regulator_enable(hub->vdd);+if(err){+dev_err(hub->dev,"failed to enable regulator: %d\n",err);+returnerr;+}++hub->is_powered_on=true;++return0;+}++staticintonboard_hub_power_off(structonboard_hub*hub)+{+interr;++err=regulator_disable(hub->vdd);+if(err){+dev_err(hub->dev,"failed to disable regulator: %d\n",err);+returnerr;+}++hub->is_powered_on=false;++return0;+}++staticint__maybe_unusedonboard_hub_suspend(structdevice*dev)+{+structonboard_hub*hub=dev_get_drvdata(dev);+structudev_node*node;+boolpower_off;+intrc=0;++if(hub->always_powered_in_suspend)+return0;++power_off=true;++mutex_lock(&hub->lock);++list_for_each_entry(node,&hub->udev_list,list){+if(!device_may_wakeup(node->udev->bus->controller))+continue;++if(usb_wakeup_enabled_descendants(node->udev)){+power_off=false;+break;+}+}++mutex_unlock(&hub->lock);++if(power_off)+rc=onboard_hub_power_off(hub);++returnrc;+}++staticint__maybe_unusedonboard_hub_resume(structdevice*dev)+{+structonboard_hub*hub=dev_get_drvdata(dev);+intrc=0;++if(!hub->is_powered_on)+rc=onboard_hub_power_on(hub);++returnrc;+}++staticintonboard_hub_add_usbdev(structonboard_hub*hub,structusb_device*udev)+{+structudev_node*node;+charlink_name[64];+intret=0;++mutex_lock(&hub->lock);++if(hub->going_away){+ret=-EINVAL;+gotounlock;+}++node=devm_kzalloc(hub->dev,sizeof(*node),GFP_KERNEL);+if(!node){+ret=-ENOMEM;+gotounlock;+}++node->udev=udev;++list_add(&node->list,&hub->udev_list);++snprintf(link_name,sizeof(link_name),"usb_dev.%s",dev_name(&udev->dev));+WARN_ON(sysfs_create_link(&hub->dev->kobj,&udev->dev.kobj,link_name));++unlock:+mutex_unlock(&hub->lock);++returnret;+}++staticvoidonboard_hub_remove_usbdev(structonboard_hub*hub,structusb_device*udev)+{+structudev_node*node;+charlink_name[64];++snprintf(link_name,sizeof(link_name),"usb_dev.%s",dev_name(&udev->dev));+sysfs_remove_link(&hub->dev->kobj,link_name);++mutex_lock(&hub->lock);++list_for_each_entry(node,&hub->udev_list,list){+if(node->udev==udev){+list_del(&node->list);+break;+}+}++mutex_unlock(&hub->lock);+}++staticssize_talways_powered_in_suspend_show(structdevice*dev,structdevice_attribute*attr,+char*buf)+{+structonboard_hub*hub=dev_get_drvdata(dev);++returnsysfs_emit(buf,"%d\n",hub->always_powered_in_suspend);+}++staticssize_talways_powered_in_suspend_store(structdevice*dev,structdevice_attribute*attr,+constchar*buf,size_tcount)+{+structonboard_hub*hub=dev_get_drvdata(dev);+boolval;+intret;++ret=kstrtobool(buf,&val);+if(ret<0)+returnret;++hub->always_powered_in_suspend=val;++returncount;+}+staticDEVICE_ATTR_RW(always_powered_in_suspend);++staticstructattribute*onboard_hub_attrs[]={+&dev_attr_always_powered_in_suspend.attr,+NULL,+};+ATTRIBUTE_GROUPS(onboard_hub);++staticintonboard_hub_probe(structplatform_device*pdev)+{+structdevice*dev=&pdev->dev;+structonboard_hub*hub;+interr;++hub=devm_kzalloc(dev,sizeof(*hub),GFP_KERNEL);+if(!hub)+return-ENOMEM;++hub->vdd=devm_regulator_get(dev,"vdd");+if(IS_ERR(hub->vdd))+returnPTR_ERR(hub->vdd);++hub->dev=dev;+mutex_init(&hub->lock);+INIT_LIST_HEAD(&hub->udev_list);++dev_set_drvdata(dev,hub);++err=onboard_hub_power_on(hub);+if(err)+returnerr;++/*+*TheUSBdrivermighthavebeendetachedfromtheUSBdevicesby+*onboard_hub_remove(),makesuretore-attachitifneeded.+*/+err=driver_attach(&onboard_hub_usbdev_driver.drvwrap.driver);+if(err){+onboard_hub_power_off(hub);+returnerr;+}++return0;+}++staticintonboard_hub_remove(structplatform_device*pdev)+{+structonboard_hub*hub=dev_get_drvdata(&pdev->dev);+structudev_node*node;+structusb_device*udev;++hub->going_away=true;++mutex_lock(&hub->lock);++/* unbind the USB devices to avoid dangling references to this device */+while(!list_empty(&hub->udev_list)){+node=list_first_entry(&hub->udev_list,structudev_node,list);+udev=node->udev;++/*+*Unbindingthedriverwillcallonboard_hub_remove_usbdev(),+*whichacquireshub->lock.Wemustreleasethelockfirst.+*/+get_device(&udev->dev);+mutex_unlock(&hub->lock);+device_release_driver(&udev->dev);+put_device(&udev->dev);+mutex_lock(&hub->lock);+}++mutex_unlock(&hub->lock);++returnonboard_hub_power_off(hub);+}++staticconststructof_device_idonboard_hub_match[]={+{.compatible="usbbda,411"},+{.compatible="usbbda,5411"},+{}+};+MODULE_DEVICE_TABLE(of,onboard_hub_match);++staticboolof_is_onboard_usb_hub(conststructdevice_node*np)+{+return!!of_match_node(onboard_hub_match,np);+}++staticconststructdev_pm_ops__maybe_unusedonboard_hub_pm_ops={+SET_LATE_SYSTEM_SLEEP_PM_OPS(onboard_hub_suspend,onboard_hub_resume)+};++staticstructplatform_driveronboard_hub_driver={+.probe=onboard_hub_probe,+.remove=onboard_hub_remove,++.driver={+.name="onboard-usb-hub",+.of_match_table=onboard_hub_match,+.pm=pm_ptr(&onboard_hub_pm_ops),+.dev_groups=onboard_hub_groups,+},+};++/************************** USB driver **************************/++#define VENDOR_ID_REALTEK 0x0bda++/*+*Returnstheonboard_hubplatformdevicethatisassociatedwiththeUSB+*devicepassedasparameter.+*/+staticstructonboard_hub*_find_onboard_hub(structdevice*dev)+{+structplatform_device*pdev;+structdevice_node*np;+phandleph;++pdev=of_find_device_by_node(dev->of_node);+if(!pdev){+if(of_property_read_u32(dev->of_node,"companion-hub",&ph)){+dev_err(dev,"failed to read 'companion-hub' property\n");+returnERR_PTR(-EINVAL);+}++np=of_find_node_by_phandle(ph);+if(!np){+dev_err(dev,"failed to find device node for companion hub\n");+returnERR_PTR(-EINVAL);+}++pdev=of_find_device_by_node(np);+of_node_put(np);++if(!pdev)+returnERR_PTR(-EPROBE_DEFER);+}++put_device(&pdev->dev);++returndev_get_drvdata(&pdev->dev);+}++staticintonboard_hub_usbdev_probe(structusb_device*udev)+{+structdevice*dev=&udev->dev;+structonboard_hub*hub;+interr;++/* ignore supported hubs without device tree node */+if(!dev->of_node)+return-ENODEV;++hub=_find_onboard_hub(dev);+if(IS_ERR(hub))+returnPTR_ERR(hub);++dev_set_drvdata(dev,hub);++err=onboard_hub_add_usbdev(hub,udev);+if(err)+returnerr;++err=sysfs_create_link(&udev->dev.kobj,&hub->dev->kobj,"onboard_hub_dev");+if(err)+dev_warn(&udev->dev,"failed to create symlink to platform device: %d\n",err);++return0;+}++staticvoidonboard_hub_usbdev_disconnect(structusb_device*udev)+{+structonboard_hub*hub=dev_get_drvdata(&udev->dev);++sysfs_remove_link(&udev->dev.kobj,"onboard_hub_dev");++onboard_hub_remove_usbdev(hub,udev);+}++staticconststructusb_device_idonboard_hub_id_table[]={+{USB_DEVICE(VENDOR_ID_REALTEK,0x0411)},/* RTS5411 USB 3.0 */+{USB_DEVICE(VENDOR_ID_REALTEK,0x5411)},/* RTS5411 USB 2.0 */+{},+};++MODULE_DEVICE_TABLE(usb,onboard_hub_id_table);++staticstructusb_device_driveronboard_hub_usbdev_driver={++.name="onboard-usb-hub",+.probe=onboard_hub_usbdev_probe,+.disconnect=onboard_hub_usbdev_disconnect,+.generic_subclass=1,+.supports_autosuspend=1,+.id_table=onboard_hub_id_table,+};++/*** Helpers for creating/destroying platform devices for onboard hubs ***/++structpdev_list_entry{+structplatform_device*pdev;+structlist_headnode;+};++/*+*Createsaplatformdeviceforeachsupportedonboardhubthatisconnectedto+*thegivenparenthub.Tokeeptrackoftheplatformdevicestheyareaddedto+*alistthatisownedbytheparenthub.+*/+voidonboard_hub_create_pdevs(structusb_device*parent_hub,structlist_head*pdev_list)+{+inti;+phandleph;+structdevice_node*np,*npc;+structplatform_device*pdev;+structpdev_list_entry*pdle;++for(i=1;i<=parent_hub->maxchild;i++){+np=usb_of_get_device_node(parent_hub,i);+if(!np)+continue;++if(!of_is_onboard_usb_hub(np))+gotonode_put;++if(of_property_read_u32(np,"companion-hub",&ph))+gotonode_put;++npc=of_find_node_by_phandle(ph);+if(!npc)+gotonode_put;++pdev=of_find_device_by_node(npc);+of_node_put(npc);++if(pdev){+/* the companion hub already has a platform device, nothing to do here */+put_device(&pdev->dev);+gotonode_put;+}++pdev=of_platform_device_create(np,NULL,&parent_hub->dev);+if(pdev){+pdle=kzalloc(sizeof(*pdle),GFP_KERNEL);+if(!pdle)+gotonode_put;++INIT_LIST_HEAD(&pdle->node);++pdle->pdev=pdev;+list_add(&pdle->node,pdev_list);+}else{+dev_err(&parent_hub->dev,+"failed to create platform device for onboard hub '%s'\n",+of_node_full_name(np));+}++node_put:+of_node_put(np);+}+}+EXPORT_SYMBOL_GPL(onboard_hub_create_pdevs);++/*+*Destroystheplatformdevicesinthegivenlistandfreesthememoryassociated+*withthelistentry.+*/+voidonboard_hub_destroy_pdevs(structlist_head*pdev_list)+{+structpdev_list_entry*pdle,*tmp;++list_for_each_entry_safe(pdle,tmp,pdev_list,node){+of_platform_device_destroy(&pdle->pdev->dev,NULL);+kfree(pdle);+}+}+EXPORT_SYMBOL_GPL(onboard_hub_destroy_pdevs);++/************************** Driver (de)registration **************************/++staticint__initonboard_hub_init(void)+{+intret;++ret=platform_driver_register(&onboard_hub_driver);+if(ret)+returnret;++ret=usb_register_device_driver(&onboard_hub_usbdev_driver,THIS_MODULE);+if(ret)+platform_driver_unregister(&onboard_hub_driver);++returnret;+}+module_init(onboard_hub_init);++staticvoid__exitonboard_hub_exit(void)+{+usb_deregister_device_driver(&onboard_hub_usbdev_driver);+platform_driver_unregister(&onboard_hub_driver);+}+module_exit(onboard_hub_exit);++MODULE_AUTHOR("Matthias Kaehlcke <mka@chromium.org>");+MODULE_DESCRIPTION("Driver for discrete onboard USB hubs");+MODULE_LICENSE("GPL v2");
Code for platform_device_create() and of_platform_device_destroy() is
only generated if CONFIG_OF_ADDRESS=y. Add stubs to avoid unresolved
symbols when CONFIG_OF_ADDRESS is not set.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes in v16:
- none
Changes in v15:
- none
Changes in v14:
- none
Changes in v13:
- none
Changes in v12:
- none
Changes in v11:
- none
Changes in v10:
- none
Changes in v9:
- added Rob's 'Acked-by' tag
Changes in v8:
- fixed C&P error in commit message
Changes in v7:
- none
Changes in v6:
- patch added to the series
include/linux/of_platform.h | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
The dependency of USB_DWC3 and USB_XHCI_MVEBU on USB_XHCI_PLATFORM
is being changed from 'select' to 'depends on' by another patch.
With that patch the defconfigs that enable one of these host
controllers also need to select USB_XHCI_PLATFORM explicitly
to keep the resulting config unchanged.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v16:
- patch added to the series
arch/arm/configs/exynos_defconfig | 1 +
arch/arm/configs/keystone_defconfig | 1 +
arch/arm/configs/multi_v7_defconfig | 1 +
arch/arm/configs/mvebu_v7_defconfig | 1 +
arch/arm/configs/omap2plus_defconfig | 1 +
arch/arm/configs/pxa_defconfig | 1 +
6 files changed, 6 insertions(+)
The dependency of USB_DWC3 on USB_XHCI_PLATFORM is being changed
from 'select' to 'depends on' by another patch. The defconfig selects
USB_DWC3 and implicitly USB_DWC3_DUAL_ROLE, to keep this unchanged
USB_XHCI_PLATFORM now needs to be selected explicitly.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v16:
- none
Changes in v15:
- none
Changes in v14:
- rebased on v5.14-rc1 (with the rest of the series)
- added 'Reviewed-by' tag from Doug
Changes in v13:
- patch added to the series
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
Some USB controller drivers that depend on the xhci-plat driver
specify this dependency using 'select' in Kconfig. This is not
recommended for symbols that have other dependencies as it may
lead to invalid configurations. Use 'depends on' to specify the
dependency instead of 'select'.
For dwc3 specify the dependency on USB_XHCI_PLATFORM in
USB_DWC3_HOST and USB_DWC3_DUAL_ROLE. Also adjust the
dependencies of USB_DWC3_CORE to make sure that at least one
of USB_DWC3_HOST, USB_DWC3_GADGET or USB_DWC3_DUAL_ROLE can be
selected.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v16:
- none
Changes in v15:
- adjusted dependencies of USB_DWC3_CORE to make sure it can only
be enabled when at least one of USB_DWC3_HOST, USB_DWC3_GADGET
or USB_DWC3_DUAL_ROLE is selectable
- updated commit message
Changes in v14:
- none
Changes in v13:
- patch added to the series
drivers/usb/cdns3/Kconfig | 2 +-
drivers/usb/dwc3/Kconfig | 5 +++--
drivers/usb/host/Kconfig | 4 ++--
3 files changed, 6 insertions(+), 5 deletions(-)
@@ -80,7 +80,7 @@ config USB_XHCI_MTKconfigUSB_XHCI_MVEBUtristate"xHCI support for Marvell Armada 375/38x/37xx"-selectUSB_XHCI_PLATFORM+depends onUSB_XHCI_PLATFORMdepends onHAS_IOMEMdepends onARCH_MVEBU||COMPILE_TESThelp
Call onboard_hub_create/destroy_pdevs() from _probe()/_remove()
to create/destroy platform devices for onboard USB hubs that may
be connected to the root hub of the controller. These functions
are a NOP unless CONFIG_USB_ONBOARD_HUB=y/m.
Also add a field to struct xhci_hcd to keep track of the onboard hub
platform devices that are owned by the xHCI.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v16:
- none
Changes in v15:
- none
Changes in v14:
- none
Changes in v13:
- added comment for 'depends on USB_ONBOARD_HUB || !USB_ONBOARD_HUB'
construct
Changes in v12:
- none
Changes in v11:
- use onboard_hub_create/destroy_pdevs() to support multiple onboard
hubs that are connected to the same root hub
- moved field/list to keep track of platform devices from struct
usb_hcd to struct xhci_hcd
- updated commit message
Changes in v10:
- none
Changes in v9:
- added dependency on USB_ONBOARD_HUB (or !!USB_ONBOARD_HUB) to
USB_XHCI_PLATFORM
Changes in v8:
- none
Changes in v7:
- none
Changes in v6:
- none
Changes in v5:
- patch added to the series
drivers/usb/host/Kconfig | 1 +
drivers/usb/host/xhci-plat.c | 6 ++++++
drivers/usb/host/xhci.h | 2 ++
3 files changed, 9 insertions(+)
@@ -54,6 +54,7 @@ config USB_XHCI_PCI_RENESASconfigUSB_XHCI_PLATFORMtristate"Generic xHCI driver for a platform device"selectUSB_XHCI_RCARifARCH_RENESAS+depends onUSB_ONBOARD_HUB||!USB_ONBOARD_HUB# if USB_ONBOARD_HUB=m, this can't be 'y'helpAddsanxHCIhostdriverforagenericplatformdevice,whichprovidesamemoryspaceandanirq.
@@ -1923,6 +1923,8 @@ struct xhci_hcd {structdentry*debugfs_slots;structlist_headregset_list;+structlist_headonboard_hub_devs;+void*dbc;/* platform-specific data -- must come last */unsignedlongpriv[]__aligned(sizeof(s64));
Add nodes for the onboard USB hub on trogdor devices. Remove the
'always-on' property from the hub regulator, since the regulator
is now managed by the onboard_usb_hub driver.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v16:
- none
Changes in v15:
- none
Changes in v14:
- none
Changes in v13:
- none
Changes in v12:
- none
Changes in v11:
- rebased on qcom/arm64-for-5.14 (with the rest of the series)
Changes in v10:
- keep 'regulator-boot-on' property
- updated commit message
Changes in v9:
- none
Changes in v8:
- none
Changes in v7:
- rebased on qcom/arm64-for-5.13 (with the rest of the series)
Changes in v6:
- added 'companion-hub' entry to both USB devices
- added 'vdd-supply' also to hub@2
Changes in v5:
- patch added to the series
.../boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 19 ++++++++-----------
.../boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 12 +++++-------
.../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts | 19 ++++++++-----------
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 19 ++++++++++++++++++-
4 files changed, 39 insertions(+), 30 deletions(-)
@@ -14,17 +14,6 @@ / {compatible="google,lazor-rev0","qcom,sc7180";};-&pp3300_hub{-/* pp3300_l7c is used to power the USB hub */-/delete-property/regulator-always-on;-/delete-property/regulator-boot-on;-};--&pp3300_l7c{-regulator-always-on;-regulator-boot-on;-};-&sn65dsi86_out{/**Lane0wasincorrectlymappedonthecable,butwe'venowdecided
@@ -14,13 +14,11 @@ / {compatible="google,lazor-rev1","google,lazor-rev2","qcom,sc7180";};-&pp3300_hub{-/* pp3300_l7c is used to power the USB hub */-/delete-property/regulator-always-on;-/delete-property/regulator-boot-on;++&usb_hub_2_0{+vdd-supply=<&pp3300_l7c>;};-&pp3300_l7c{-regulator-always-on;-regulator-boot-on;+&usb_hub_3_0{+vdd-supply=<&pp3300_l7c>;};
@@ -42,17 +42,6 @@ &panel {compatible="auo,b116xa01";};-&pp3300_hub{-/* pp3300_l7c is used to power the USB hub */-/delete-property/regulator-always-on;-/delete-property/regulator-boot-on;-};--&pp3300_l7c{-regulator-always-on;-regulator-boot-on;-};-&sdhc_2{status="okay";};
@@ -898,6 +897,24 @@ &usb_1 {&usb_1_dwc3{dr_mode="host";+#address-cells=<1>;+#size-cells=<0>;++/* 2.0 hub on port 1 */+usb_hub_2_0:hub@1{+compatible="usbbda,5411";+reg=<1>;+vdd-supply=<&pp3300_hub>;+companion-hub=<&usb_hub_3_0>;+};++/* 3.0 hub on port 2 */+usb_hub_3_0:hub@2{+compatible="usbbda,411";+reg=<2>;+vdd-supply=<&pp3300_hub>;+companion-hub=<&usb_hub_2_0>;+};};&usb_1_hsphy{
From: Roger Quadros <rogerq@kernel.org> Date: 2021-08-26 06:46:16
On 13/08/2021 22:52, Matthias Kaehlcke wrote:
The dependency of USB_DWC3 and USB_XHCI_MVEBU on USB_XHCI_PLATFORM
is being changed from 'select' to 'depends on' by another patch.
With that patch the defconfigs that enable one of these host
controllers also need to select USB_XHCI_PLATFORM explicitly
to keep the resulting config unchanged.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Roger Quadros <rogerq@kernel.org>
cheers,
-roger
quoted hunk
---
Changes in v16:
- patch added to the series
arch/arm/configs/exynos_defconfig | 1 +
arch/arm/configs/keystone_defconfig | 1 +
arch/arm/configs/multi_v7_defconfig | 1 +
arch/arm/configs/mvebu_v7_defconfig | 1 +
arch/arm/configs/omap2plus_defconfig | 1 +
arch/arm/configs/pxa_defconfig | 1 +
6 files changed, 6 insertions(+)
From: Roger Quadros <rogerq@kernel.org> Date: 2021-08-26 06:46:24
On 13/08/2021 22:52, Matthias Kaehlcke wrote:
The dependency of USB_DWC3 on USB_XHCI_PLATFORM is being changed
from 'select' to 'depends on' by another patch. The defconfig selects
USB_DWC3 and implicitly USB_DWC3_DUAL_ROLE, to keep this unchanged
USB_XHCI_PLATFORM now needs to be selected explicitly.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Roger Quadros <rogerq@kernel.org>
cheers,
-roger
quoted hunk
---
Changes in v16:
- none
Changes in v15:
- none
Changes in v14:
- rebased on v5.14-rc1 (with the rest of the series)
- added 'Reviewed-by' tag from Doug
Changes in v13:
- patch added to the series
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
From: Roger Quadros <rogerq@kernel.org> Date: 2021-08-26 06:46:39
On 13/08/2021 22:52, Matthias Kaehlcke wrote:
Some USB controller drivers that depend on the xhci-plat driver
specify this dependency using 'select' in Kconfig. This is not
recommended for symbols that have other dependencies as it may
lead to invalid configurations. Use 'depends on' to specify the
dependency instead of 'select'.
For dwc3 specify the dependency on USB_XHCI_PLATFORM in
USB_DWC3_HOST and USB_DWC3_DUAL_ROLE. Also adjust the
dependencies of USB_DWC3_CORE to make sure that at least one
of USB_DWC3_HOST, USB_DWC3_GADGET or USB_DWC3_DUAL_ROLE can be
selected.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Roger Quadros <rogerq@kernel.org>
cheers,
-roger
quoted hunk
---
Changes in v16:
- none
Changes in v15:
- adjusted dependencies of USB_DWC3_CORE to make sure it can only
be enabled when at least one of USB_DWC3_HOST, USB_DWC3_GADGET
or USB_DWC3_DUAL_ROLE is selectable
- updated commit message
Changes in v14:
- none
Changes in v13:
- patch added to the series
drivers/usb/cdns3/Kconfig | 2 +-
drivers/usb/dwc3/Kconfig | 5 +++--
drivers/usb/host/Kconfig | 4 ++--
3 files changed, 6 insertions(+), 5 deletions(-)
@@ -80,7 +80,7 @@ config USB_XHCI_MTKconfigUSB_XHCI_MVEBUtristate"xHCI support for Marvell Armada 375/38x/37xx"-selectUSB_XHCI_PLATFORM+depends onUSB_XHCI_PLATFORMdepends onHAS_IOMEMdepends onARCH_MVEBU||COMPILE_TESThelp
From: Krzysztof Kozlowski <hidden> Date: 2021-08-26 07:56:54
On 13/08/2021 21:52, Matthias Kaehlcke wrote:
The dependency of USB_DWC3 and USB_XHCI_MVEBU on USB_XHCI_PLATFORM
is being changed from 'select' to 'depends on' by another patch.
With that patch the defconfigs that enable one of these host
controllers also need to select USB_XHCI_PLATFORM explicitly
to keep the resulting config unchanged.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v16:
- patch added to the series
arch/arm/configs/exynos_defconfig | 1 +
Acked-by: Krzysztof Kozlowski <redacted>
Best regards,
Krzysztof
Hi Greg,
are there any actions pending or can this land in usb-testing?
I confirmed that this series can be rebased on top of v5.15-rc2
without conflicts.
Thanks
m.
On Fri, Aug 13, 2021 at 12:52:21PM -0700, Matthias Kaehlcke wrote:
This series adds:
- the onboard_usb_hub_driver
- glue in the xhci-plat driver to create and destroy the
onboard_usb_hub platform devices 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 v16:
- added patch 'ARM: configs: Explicitly enable USB_XHCI_PLATFORM
where needed' to keep arm32 defconfigs effectively unchanged
Changes in v15:
- adjusted dependencies of USB_DWC3_CORE to make sure it can only
be enabled when at least one of USB_DWC3_HOST, USB_DWC3_GADGET
or USB_DWC3_DUAL_ROLE is selectable
Changes in v14:
- rebased on top of v5.14-rc1
- dropped DT binding patch which landed in v5.13
Changes in v13:
- added patch "usb: Specify dependency on USB_XHCI_PLATFORM with
'depends on'" to the series to avoid Kconfig conflicts
- added patch "arm64: defconfig: Explicitly enable USB_XHCI_PLATFORM"
to the series to keep effective defconfig unchanged
Changes in v12:
- onboard_hub driver: use IS_ENABLED(CONFIG_USB_ONBOARD_HUB_MODULE)
in onboard_hub.h to also check for the driver built as module
- onboard_hub_driver: include onboard_hub.h again to make sure there
are prototype declarations for the public functions
Changes in v11:
- support multiple onboard hubs connected to the same parent
- don't include ‘onboard_hub.h’ from the onboard hub driver
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 (7):
usb: misc: Add onboard_usb_hub driver
of/platform: Add stubs for of_platform_device_create/destroy()
ARM: configs: Explicitly enable USB_XHCI_PLATFORM where needed
arm64: defconfig: Explicitly enable USB_XHCI_PLATFORM
usb: Specify dependencies on USB_XHCI_PLATFORM with 'depends on'
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 +
MAINTAINERS | 7 +
arch/arm/configs/exynos_defconfig | 1 +
arch/arm/configs/keystone_defconfig | 1 +
arch/arm/configs/multi_v7_defconfig | 1 +
arch/arm/configs/mvebu_v7_defconfig | 1 +
arch/arm/configs/omap2plus_defconfig | 1 +
arch/arm/configs/pxa_defconfig | 1 +
.../boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 19 +-
.../boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 12 +-
.../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts | 19 +-
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 19 +-
arch/arm64/configs/defconfig | 1 +
drivers/usb/cdns3/Kconfig | 2 +-
drivers/usb/dwc3/Kconfig | 5 +-
drivers/usb/host/Kconfig | 5 +-
drivers/usb/host/xhci-plat.c | 6 +
drivers/usb/host/xhci.h | 2 +
drivers/usb/misc/Kconfig | 17 +
drivers/usb/misc/Makefile | 1 +
drivers/usb/misc/onboard_usb_hub.c | 497 ++++++++++++++++++
include/linux/of_platform.h | 22 +-
include/linux/usb/onboard_hub.h | 18 +
23 files changed, 627 insertions(+), 39 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub
create mode 100644 drivers/usb/misc/onboard_usb_hub.c
create mode 100644 include/linux/usb/onboard_hub.h
--
2.33.0.rc1.237.g0d66db33f3-goog
From: Doug Anderson <dianders@chromium.org> Date: 2021-10-14 21:46:24
Hi,
On Tue, Sep 21, 2021 at 10:09 AM Matthias Kaehlcke [off-list ref] wrote:
Hi Greg,
are there any actions pending or can this land in usb-testing?
I confirmed that this series can be rebased on top of v5.15-rc2
without conflicts.
I'm quite interested to know what the next action items are, too. This
is one of the very few patches we have for trogdor (excluding MIPI
camera, which is a long story) that we're carrying downstream, so I'm
keenly interested in making sure it's unblocked (if, indeed, it's
blocked on anything).
If folks feel that this needs more review eyes before landing again
then I'll try to find some time in the next week or two. If it's just
waiting for the merge window to open/close so it can have maximal bake
time, that's cool too. Please yell if there's something that I can do
to help, though! :-)
On Fri, Aug 13, 2021 at 12:52:21PM -0700, Matthias Kaehlcke wrote:
quoted
This series adds:
- the onboard_usb_hub_driver
- glue in the xhci-plat driver to create and destroy the
onboard_usb_hub platform devices 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 v16:
- added patch 'ARM: configs: Explicitly enable USB_XHCI_PLATFORM
where needed' to keep arm32 defconfigs effectively unchanged
Changes in v15:
- adjusted dependencies of USB_DWC3_CORE to make sure it can only
be enabled when at least one of USB_DWC3_HOST, USB_DWC3_GADGET
or USB_DWC3_DUAL_ROLE is selectable
Changes in v14:
- rebased on top of v5.14-rc1
- dropped DT binding patch which landed in v5.13
Changes in v13:
- added patch "usb: Specify dependency on USB_XHCI_PLATFORM with
'depends on'" to the series to avoid Kconfig conflicts
- added patch "arm64: defconfig: Explicitly enable USB_XHCI_PLATFORM"
to the series to keep effective defconfig unchanged
Changes in v12:
- onboard_hub driver: use IS_ENABLED(CONFIG_USB_ONBOARD_HUB_MODULE)
in onboard_hub.h to also check for the driver built as module
- onboard_hub_driver: include onboard_hub.h again to make sure there
are prototype declarations for the public functions
Changes in v11:
- support multiple onboard hubs connected to the same parent
- don't include ‘onboard_hub.h’ from the onboard hub driver
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 (7):
usb: misc: Add onboard_usb_hub driver
of/platform: Add stubs for of_platform_device_create/destroy()
ARM: configs: Explicitly enable USB_XHCI_PLATFORM where needed
arm64: defconfig: Explicitly enable USB_XHCI_PLATFORM
usb: Specify dependencies on USB_XHCI_PLATFORM with 'depends on'
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 +
MAINTAINERS | 7 +
arch/arm/configs/exynos_defconfig | 1 +
arch/arm/configs/keystone_defconfig | 1 +
arch/arm/configs/multi_v7_defconfig | 1 +
arch/arm/configs/mvebu_v7_defconfig | 1 +
arch/arm/configs/omap2plus_defconfig | 1 +
arch/arm/configs/pxa_defconfig | 1 +
.../boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 19 +-
.../boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 12 +-
.../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts | 19 +-
arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 19 +-
arch/arm64/configs/defconfig | 1 +
drivers/usb/cdns3/Kconfig | 2 +-
drivers/usb/dwc3/Kconfig | 5 +-
drivers/usb/host/Kconfig | 5 +-
drivers/usb/host/xhci-plat.c | 6 +
drivers/usb/host/xhci.h | 2 +
drivers/usb/misc/Kconfig | 17 +
drivers/usb/misc/Makefile | 1 +
drivers/usb/misc/onboard_usb_hub.c | 497 ++++++++++++++++++
include/linux/of_platform.h | 22 +-
include/linux/usb/onboard_hub.h | 18 +
23 files changed, 627 insertions(+), 39 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub
create mode 100644 drivers/usb/misc/onboard_usb_hub.c
create mode 100644 include/linux/usb/onboard_hub.h
--
2.33.0.rc1.237.g0d66db33f3-goog
On Thu, Oct 14, 2021 at 02:38:55PM -0700, Doug Anderson wrote:
Hi,
On Tue, Sep 21, 2021 at 10:09 AM Matthias Kaehlcke [off-list ref] wrote:
quoted
Hi Greg,
are there any actions pending or can this land in usb-testing?
I confirmed that this series can be rebased on top of v5.15-rc2
without conflicts.
I'm quite interested to know what the next action items are, too. This
is one of the very few patches we have for trogdor (excluding MIPI
camera, which is a long story) that we're carrying downstream, so I'm
keenly interested in making sure it's unblocked (if, indeed, it's
blocked on anything).
If folks feel that this needs more review eyes before landing again
then I'll try to find some time in the next week or two. If it's just
waiting for the merge window to open/close so it can have maximal bake
time, that's cool too. Please yell if there's something that I can do
to help, though! :-)
I would love more review-eyes on this please.
It's in my queue to review, I just need to spend the time on it, sorry
for the delay.
greg k-h
On Thu, Oct 14, 2021 at 02:38:55PM -0700, Doug Anderson wrote:
quoted
Hi,
On Tue, Sep 21, 2021 at 10:09 AM Matthias Kaehlcke [off-list ref] wrote:
quoted
Hi Greg,
are there any actions pending or can this land in usb-testing?
I confirmed that this series can be rebased on top of v5.15-rc2
without conflicts.
I'm quite interested to know what the next action items are, too. This
is one of the very few patches we have for trogdor (excluding MIPI
camera, which is a long story) that we're carrying downstream, so I'm
keenly interested in making sure it's unblocked (if, indeed, it's
blocked on anything).
If folks feel that this needs more review eyes before landing again
then I'll try to find some time in the next week or two. If it's just
waiting for the merge window to open/close so it can have maximal bake
time, that's cool too. Please yell if there's something that I can do
to help, though! :-)
I would love more review-eyes on this please.
Hi,
I noticed this series some time ago, and wanted to take a closer look.
The same issue this series address is seen on stm32 board for instance.
(arch/arm/boot/dts/stm32mp15xx-dkx.dtsi). On board HUB (not described in
the DT) is supplied by an always-on regulator.
So it could could be interesting/useful to address the same case ,
on stm32 boards, where USB2 (ehci-platform driver) is used currently.
I noticed a few things, especially on the dt-bindings. I've some
questions here.
In this series, RTS5411 is used. The dt-bindings documents it as a child
node of the USB controller. E.g.
&usb {
usb_hub_2_0: hub@1 {
...
};
usb_hub_3_0: hub@2 {
};
}
I had a quick look at RTS5411 datasheet. It looks like there's an i2c
interface too.
- I guess the I2C interface isn't used in your case ?
(I haven't checked what it could be used for...)
In the stm32 boards (stm32mp15xx-dkx), there's an usb2514b chip
- that also could be wired on I2C interface (0R mount option)
- unused on stm32 boards by default
usb2514b chip already has a dt-bindings (with compatible), and a driver:
- drivers/usb/misc/usb251xb.c
- Documentation/devicetree/bindings/usb/usb251xb.txt
It is defined more as an i2c chip, so I'd expect it as an i2c child,
e.g. like:
&i2c {
usb2514b@2c {
compatible = "microchip,usb2514b";
...
};
};
This way, I don't see how it could be used together with onboard_usb_hub
driver ? (But I may have missed it)
Is it possible to use a phandle, instead of a child node ?
However, in the stm32mp15xx-dkx case, i2c interface isn't wired/used by
default. So obviously the i2c driver isn't used. In this case, could the
"microchip,usb2514b" be listed in onboard_usb_hub driver ?
(wouldn't it be redundant ?)
In this case it would be a child node of the usb DT node... Maybe that's
more a question for Rob: would it be "legal" regarding existing
dt-bindings ?
Thanks in advance
Best Regards,
Fabrice
It's in my queue to review, I just need to spend the time on it, sorry
for the delay.
greg k-h
On Tue, Oct 19, 2021 at 06:04:06PM +0200, Fabrice Gasnier wrote:
On 10/15/21 8:39 AM, Greg Kroah-Hartman wrote:
quoted
On Thu, Oct 14, 2021 at 02:38:55PM -0700, Doug Anderson wrote:
quoted
Hi,
On Tue, Sep 21, 2021 at 10:09 AM Matthias Kaehlcke [off-list ref] wrote:
quoted
Hi Greg,
are there any actions pending or can this land in usb-testing?
I confirmed that this series can be rebased on top of v5.15-rc2
without conflicts.
I'm quite interested to know what the next action items are, too. This
is one of the very few patches we have for trogdor (excluding MIPI
camera, which is a long story) that we're carrying downstream, so I'm
keenly interested in making sure it's unblocked (if, indeed, it's
blocked on anything).
If folks feel that this needs more review eyes before landing again
then I'll try to find some time in the next week or two. If it's just
waiting for the merge window to open/close so it can have maximal bake
time, that's cool too. Please yell if there's something that I can do
to help, though! :-)
I would love more review-eyes on this please.
Hi,
I noticed this series some time ago, and wanted to take a closer look.
The same issue this series address is seen on stm32 board for instance.
(arch/arm/boot/dts/stm32mp15xx-dkx.dtsi). On board HUB (not described in
the DT) is supplied by an always-on regulator.
So it could could be interesting/useful to address the same case ,
on stm32 boards, where USB2 (ehci-platform driver) is used currently.
I noticed a few things, especially on the dt-bindings. I've some
questions here.
In this series, RTS5411 is used. The dt-bindings documents it as a child
node of the USB controller. E.g.
&usb {
usb_hub_2_0: hub@1 {
...
};
usb_hub_3_0: hub@2 {
};
}
I had a quick look at RTS5411 datasheet. It looks like there's an i2c
interface too.
- I guess the I2C interface isn't used in your case ?
(I haven't checked what it could be used for...)
In the stm32 boards (stm32mp15xx-dkx), there's an usb2514b chip
- that also could be wired on I2C interface (0R mount option)
- unused on stm32 boards by default
usb2514b chip already has a dt-bindings (with compatible), and a driver:
- drivers/usb/misc/usb251xb.c
- Documentation/devicetree/bindings/usb/usb251xb.txt
It is defined more as an i2c chip, so I'd expect it as an i2c child,
e.g. like:
&i2c {
usb2514b@2c {
compatible = "microchip,usb2514b";
...
};
};
This way, I don't see how it could be used together with onboard_usb_hub
driver ? (But I may have missed it)
Indeed, you can either use the i2c driver for the hub or the onboard_usb_hub
driver, but not both at the same time. The i2c driver requires the hub to be
powered before communicating with it over i2c, hence the power sequence
should not be delegated to the onboard_usb_hub driver.
Is it possible to use a phandle, instead of a child node ?
The child node is part of the generic USB binding. The onboard_usb_hub
driver needs it to find the USB device(s) that correspond to the hub,
to optionally power the hub off during system suspend when no wakeup
capable devices are connected.
However, in the stm32mp15xx-dkx case, i2c interface isn't wired/used by
default. So obviously the i2c driver isn't used. In this case, could the
"microchip,usb2514b" be listed in onboard_usb_hub driver ?
(wouldn't it be redundant ?)
You would use the compatible string of the generic USB binding, i.e.
"usbVID,PID", which would have to be added to the onboard_usb_hub driver.
In this case it would be a child node of the usb DT node... Maybe that's
more a question for Rob: would it be "legal" regarding existing
dt-bindings ?
From: Michal Simek <hidden> Date: 2021-10-20 06:21:44
On 10/19/21 18:04, Fabrice Gasnier wrote:
On 10/15/21 8:39 AM, Greg Kroah-Hartman wrote:
quoted
On Thu, Oct 14, 2021 at 02:38:55PM -0700, Doug Anderson wrote:
quoted
Hi,
On Tue, Sep 21, 2021 at 10:09 AM Matthias Kaehlcke [off-list ref] wrote:
quoted
Hi Greg,
are there any actions pending or can this land in usb-testing?
I confirmed that this series can be rebased on top of v5.15-rc2
without conflicts.
I'm quite interested to know what the next action items are, too. This
is one of the very few patches we have for trogdor (excluding MIPI
camera, which is a long story) that we're carrying downstream, so I'm
keenly interested in making sure it's unblocked (if, indeed, it's
blocked on anything).
If folks feel that this needs more review eyes before landing again
then I'll try to find some time in the next week or two. If it's just
waiting for the merge window to open/close so it can have maximal bake
time, that's cool too. Please yell if there's something that I can do
to help, though! :-)
I would love more review-eyes on this please.
Hi,
I noticed this series some time ago, and wanted to take a closer look.
The same issue this series address is seen on stm32 board for instance.
(arch/arm/boot/dts/stm32mp15xx-dkx.dtsi). On board HUB (not described in
the DT) is supplied by an always-on regulator.
So it could could be interesting/useful to address the same case ,
on stm32 boards, where USB2 (ehci-platform driver) is used currently.
I noticed a few things, especially on the dt-bindings. I've some
questions here.
In this series, RTS5411 is used. The dt-bindings documents it as a child
node of the USB controller. E.g.
&usb {
usb_hub_2_0: hub@1 {
...
};
usb_hub_3_0: hub@2 {
};
}
I had a quick look at RTS5411 datasheet. It looks like there's an i2c
interface too.
- I guess the I2C interface isn't used in your case ?
(I haven't checked what it could be used for...)
In the stm32 boards (stm32mp15xx-dkx), there's an usb2514b chip
- that also could be wired on I2C interface (0R mount option)
- unused on stm32 boards by default
usb2514b chip already has a dt-bindings (with compatible), and a driver:
- drivers/usb/misc/usb251xb.c
- Documentation/devicetree/bindings/usb/usb251xb.txt
It is defined more as an i2c chip, so I'd expect it as an i2c child,
e.g. like:
&i2c {
usb2514b@2c {
compatible = "microchip,usb2514b";
...
};
};
This way, I don't see how it could be used together with onboard_usb_hub
driver ? (But I may have missed it)
Is it possible to use a phandle, instead of a child node ?
However, in the stm32mp15xx-dkx case, i2c interface isn't wired/used by
default. So obviously the i2c driver isn't used. In this case, could the
"microchip,usb2514b" be listed in onboard_usb_hub driver ?
(wouldn't it be redundant ?)
In this case it would be a child node of the usb DT node... Maybe that's
more a question for Rob: would it be "legal" regarding existing
dt-bindings ?
We wanted to upstream driver for microchip usb5744 and based on this
thread with Rob
https://lore.kernel.org/all/CAL_JsqJZBbu+UXqUNdZwg-uv0PAsNg55026PTwhKr5wQtxCjVQ@mail.gmail.com/
the recommendation was to use i2c-bus link. And in our usb5744 case
where usb hub has only one i2c address we just hardcoded it in the
driver. I should be pushing this driver to xilinx soc tree soon if you
want to take a look.
Thanks,
Michal
Call onboard_hub_create/destroy_pdevs() from _probe()/_remove()
to create/destroy platform devices for onboard USB hubs that may
be connected to the root hub of the controller. These functions
are a NOP unless CONFIG_USB_ONBOARD_HUB=y/m.
Also add a field to struct xhci_hcd to keep track of the onboard hub
platform devices that are owned by the xHCI.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Haven't really looked at this series until now.
Is there any reason why the xhci platform driver was selected as
the best place to create/remove these onboard hub devices?
This ties the onboard hubs to xhci, and won't work in case we have onboard
hubs connected to a ehci controllers.
If separate devices for controlling onboard hub power is the right solution then
how about creating the onboard hub device in usb_add_hcd() (hcd.c), and
store it in struct usb_hcd.
A bit like how the roothub device is created, or PHYs are tuned.
Thanks
Mathias
On Wed, Oct 20, 2021 at 08:21:21AM +0200, Michal Simek wrote:
On 10/19/21 18:04, Fabrice Gasnier wrote:
quoted
On 10/15/21 8:39 AM, Greg Kroah-Hartman wrote:
quoted
On Thu, Oct 14, 2021 at 02:38:55PM -0700, Doug Anderson wrote:
quoted
Hi,
On Tue, Sep 21, 2021 at 10:09 AM Matthias Kaehlcke [off-list ref] wrote:
quoted
Hi Greg,
are there any actions pending or can this land in usb-testing?
I confirmed that this series can be rebased on top of v5.15-rc2
without conflicts.
I'm quite interested to know what the next action items are, too. This
is one of the very few patches we have for trogdor (excluding MIPI
camera, which is a long story) that we're carrying downstream, so I'm
keenly interested in making sure it's unblocked (if, indeed, it's
blocked on anything).
If folks feel that this needs more review eyes before landing again
then I'll try to find some time in the next week or two. If it's just
waiting for the merge window to open/close so it can have maximal bake
time, that's cool too. Please yell if there's something that I can do
to help, though! :-)
I would love more review-eyes on this please.
Hi,
I noticed this series some time ago, and wanted to take a closer look.
The same issue this series address is seen on stm32 board for instance.
(arch/arm/boot/dts/stm32mp15xx-dkx.dtsi). On board HUB (not described in
the DT) is supplied by an always-on regulator.
So it could could be interesting/useful to address the same case ,
on stm32 boards, where USB2 (ehci-platform driver) is used currently.
I noticed a few things, especially on the dt-bindings. I've some
questions here.
In this series, RTS5411 is used. The dt-bindings documents it as a child
node of the USB controller. E.g.
&usb {
usb_hub_2_0: hub@1 {
...
};
usb_hub_3_0: hub@2 {
};
}
I had a quick look at RTS5411 datasheet. It looks like there's an i2c
interface too.
- I guess the I2C interface isn't used in your case ?
(I haven't checked what it could be used for...)
In the stm32 boards (stm32mp15xx-dkx), there's an usb2514b chip
- that also could be wired on I2C interface (0R mount option)
- unused on stm32 boards by default
usb2514b chip already has a dt-bindings (with compatible), and a driver:
- drivers/usb/misc/usb251xb.c
- Documentation/devicetree/bindings/usb/usb251xb.txt
It is defined more as an i2c chip, so I'd expect it as an i2c child,
e.g. like:
&i2c {
usb2514b@2c {
compatible = "microchip,usb2514b";
...
};
};
This way, I don't see how it could be used together with onboard_usb_hub
driver ? (But I may have missed it)
Is it possible to use a phandle, instead of a child node ?
However, in the stm32mp15xx-dkx case, i2c interface isn't wired/used by
default. So obviously the i2c driver isn't used. In this case, could the
"microchip,usb2514b" be listed in onboard_usb_hub driver ?
(wouldn't it be redundant ?)
In this case it would be a child node of the usb DT node... Maybe that's
more a question for Rob: would it be "legal" regarding existing
dt-bindings ?
We wanted to upstream driver for microchip usb5744 and based on this thread
with Rob
https://lore.kernel.org/all/CAL_JsqJZBbu+UXqUNdZwg-uv0PAsNg55026PTwhKr5wQtxCjVQ@mail.gmail.com/
the recommendation was to use i2c-bus link. And in our usb5744 case where
usb hub has only one i2c address we just hardcoded it in the driver. I
should be pushing this driver to xilinx soc tree soon if you want to take a
look.
Interesting, with the 'i2c-bus' link it might be feasible to support the
i2c functionality in the onboard_usb_hub driver if desired. Not sure how
things would look when the hub can have different i2c addresses. Also in
case of powering down the hub during system suspend any configuration
through i2c would have to be done again on resume.
Hi Mathias,
On Wed, Oct 20, 2021 at 04:05:37PM +0300, Mathias Nyman wrote:
Hi
On 13.8.2021 22.52, Matthias Kaehlcke wrote:
quoted
Call onboard_hub_create/destroy_pdevs() from _probe()/_remove()
to create/destroy platform devices for onboard USB hubs that may
be connected to the root hub of the controller. These functions
are a NOP unless CONFIG_USB_ONBOARD_HUB=y/m.
Also add a field to struct xhci_hcd to keep track of the onboard hub
platform devices that are owned by the xHCI.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Haven't really looked at this series until now.
Is there any reason why the xhci platform driver was selected as
the best place to create/remove these onboard hub devices?
IIRC Alan suggested to use the xhci platform driver for creating/removing
the onboard hub devices when we were trying to get rid of a separate DT
node on the 'platform bus', which was suitable the board for my use case.
This ties the onboard hubs to xhci, and won't work in case we have onboard
hubs connected to a ehci controllers.
Right, the driver itself isn't limited to xhci. The initial idea was that
support for other types of USB controllers could be added as needed (I only
have a config with xhci for testing).
If separate devices for controlling onboard hub power is the right solution then
how about creating the onboard hub device in usb_add_hcd() (hcd.c), and
store it in struct usb_hcd.
A bit like how the roothub device is created, or PHYs are tuned.
Sure, that sounds feasible, even better if it's handled in a single place
and different types of controllers don't have to add support separately.
From: Alan Stern <stern@rowland.harvard.edu> Date: 2021-10-20 20:37:24
On Wed, Oct 20, 2021 at 01:27:40PM -0700, Matthias Kaehlcke wrote:
Hi Mathias,
On Wed, Oct 20, 2021 at 04:05:37PM +0300, Mathias Nyman wrote:
quoted
If separate devices for controlling onboard hub power is the right solution then
how about creating the onboard hub device in usb_add_hcd() (hcd.c), and
store it in struct usb_hcd.
A bit like how the roothub device is created, or PHYs are tuned.
Sure, that sounds feasible, even better if it's handled in a single place
and different types of controllers don't have to add support separately.
Bear in mind that this would prevent you from working with onboard
non-root hubs.
Alan Stern
On Wed, Oct 20, 2021 at 04:37:20PM -0400, Alan Stern wrote:
On Wed, Oct 20, 2021 at 01:27:40PM -0700, Matthias Kaehlcke wrote:
quoted
Hi Mathias,
On Wed, Oct 20, 2021 at 04:05:37PM +0300, Mathias Nyman wrote:
quoted
If separate devices for controlling onboard hub power is the right solution then
how about creating the onboard hub device in usb_add_hcd() (hcd.c), and
store it in struct usb_hcd.
A bit like how the roothub device is created, or PHYs are tuned.
Sure, that sounds feasible, even better if it's handled in a single place
and different types of controllers don't have to add support separately.
Bear in mind that this would prevent you from working with onboard
non-root hubs.
My goal is to (architecturally) support nested hubs, but TBH I haven't
looked much into such a configuration since I don't have hardware for
testing. My assumption was that support for onboard hubs connected to
non-root hubs whould have to be added to the generic hub driver.
Could you elaborate in how far you think it would be different for
xhci_plat vs generic hcd?
From: Alan Stern <stern@rowland.harvard.edu> Date: 2021-10-20 21:57:41
On Wed, Oct 20, 2021 at 02:01:21PM -0700, Matthias Kaehlcke wrote:
On Wed, Oct 20, 2021 at 04:37:20PM -0400, Alan Stern wrote:
quoted
On Wed, Oct 20, 2021 at 01:27:40PM -0700, Matthias Kaehlcke wrote:
quoted
Hi Mathias,
On Wed, Oct 20, 2021 at 04:05:37PM +0300, Mathias Nyman wrote:
quoted
If separate devices for controlling onboard hub power is the right solution then
how about creating the onboard hub device in usb_add_hcd() (hcd.c), and
store it in struct usb_hcd.
A bit like how the roothub device is created, or PHYs are tuned.
Sure, that sounds feasible, even better if it's handled in a single place
and different types of controllers don't have to add support separately.
Bear in mind that this would prevent you from working with onboard
non-root hubs.
My goal is to (architecturally) support nested hubs, but TBH I haven't
looked much into such a configuration since I don't have hardware for
testing. My assumption was that support for onboard hubs connected to
non-root hubs whould have to be added to the generic hub driver.
Could you elaborate in how far you think it would be different for
xhci_plat vs generic hcd?
A lot of this material has slipped from my mind. However, I don't see
much difference between those two approaches.
Alan Stern
I'm confused. Where is this .yaml file? It doesn't look landed and it
doesn't look to be in your series. I guess this should be updated to:
F: Documentation/devicetree/bindings/usb/realtek,rts5411.yaml
Also: should this have:
F: Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub
It feels like it would be safer to call dev_get_drvdata() before
putting the device? ...and actually, are you sure you should even be
putting the device? Maybe we should wait to put it until
onboard_hub_usbdev_disconnect()
Should the `INIT_LIST_HEAD(pdev_list);` go here? Is there any reason
why we need to push this into the caller?
+ for (i = 1; i <= parent_hub->maxchild; i++) {
+ np = usb_of_get_device_node(parent_hub, i);
+ if (!np)
+ continue;
+
+ if (!of_is_onboard_usb_hub(np))
+ goto node_put;
+
+ if (of_property_read_u32(np, "companion-hub", &ph))
+ goto node_put;
+
+ npc = of_find_node_by_phandle(ph);
+ if (!npc)
+ goto node_put;
Aren't the above two calls equivalent to this?
npc = of_parse_phandle(np, "companion-hub", 0)
I'm also curious why a companion-hub is a _required_ property.
Couldn't you support USB 2.0 hubs better by just allowing
companion-hub to be optional? I guess that could be a future
improvement, but it also seems trivial to support from the start.
+ pdev = of_find_device_by_node(npc);
+ of_node_put(npc);
+
+ if (pdev) {
+ /* the companion hub already has a platform device, nothing to do here */
+ put_device(&pdev->dev);
+ goto node_put;
+ }
+
+ pdev = of_platform_device_create(np, NULL, &parent_hub->dev);
+ if (pdev) {
+ pdle = kzalloc(sizeof(*pdle), GFP_KERNEL);
Maybe devm_kzalloc(&pdev->dev, GFP_KERNEL) ? Then you can get rid of
the free in the destroy function?
+ if (!pdle)
+ goto node_put;
If your memory allocation fails here, don't you need to
of_platform_device_destroy() ?
+ INIT_LIST_HEAD(&pdle->node);
I don't believe that the INIT_LIST_HEAD() does anything useful here.
&pdle->node is not a list head--it's a list element. Adding it to the
end of the existing list will fully initialize its ->next and ->prev
pointers but won't look at what they were.
It feels like you should be removing the node from the list too,
right? Otherwise if you unbind / bind the USB driver you'll still have
garbage in your list the 2nd time?
Technically you don't need the "USB &&", right? Since
USB_XHCI_PLATFORM is defined in 'usb/host/Kconfig' and that's only
even included if USB is defined. So it can be just:
depends on (USB_XHCI_PLATFORM || USB_GADGET) && HAS_DMA
That's not terribly important, though, so:
Reviewed-by: Douglas Anderson <dianders@chromium.org>
nit: 'udev' has a whole different connotation to me. Maybe just go
with `usbdev_node` ?
Will change to 'usbdev_dev' node as suggested, I think it's ok to keep
'udev' for the pointer to the USB device itself, since that abbreviation
is used commonly in USB kernel land.
Shouldn't you also defer if the dev_get_drvdata() returns NULL? What
if you're racing the probe of the platform device?
Yeah, it seems that race could happen. IIUC we could use device_is_bound()
to check if probing completed, really_probe() calls driver_bound() only
after successfully probing the device.
It feels like it would be safer to call dev_get_drvdata() before
putting the device? ...and actually, are you sure you should even be
putting the device? Maybe we should wait to put it until
onboard_hub_usbdev_disconnect()
It shouldn't be necessary, when the platform device is destroyed it
unbinds the associated USB devices (see onboard_hub_remove()), hence
they don't keep using the drvdata. There was a related discussion in
the early days of this series: https://lkml.org/lkml/2020/9/21/2153
Should the `INIT_LIST_HEAD(pdev_list);` go here? Is there any reason
why we need to push this into the caller?
That would limit pdev_list to a single entry, which is not what we want. A
parent hub might have multiple compatible onboard hubs connected to it.
quoted
+ for (i = 1; i <= parent_hub->maxchild; i++) {
+ np = usb_of_get_device_node(parent_hub, i);
+ if (!np)
+ continue;
+
+ if (!of_is_onboard_usb_hub(np))
+ goto node_put;
+
+ if (of_property_read_u32(np, "companion-hub", &ph))
+ goto node_put;
+
+ npc = of_find_node_by_phandle(ph);
+ if (!npc)
+ goto node_put;
Aren't the above two calls equivalent to this?
npc = of_parse_phandle(np, "companion-hub", 0)
yes, will change to of_parse_phandle()
I'm also curious why a companion-hub is a _required_ property.
Couldn't you support USB 2.0 hubs better by just allowing
companion-hub to be optional? I guess that could be a future
improvement, but it also seems trivial to support from the start.
The evolution of this driver somewhat tied it to xHCI, however that
isn't strictly necessary. In a sense it is nice when 'companion-hub'
is mandatory, since things can get messy if it is forgotten when it
should be there.
The property should be mandatory in the bindings of the USB >= 3.0
hubs that are supported by this driver, but it could be optional
for USB 2.0 hubs. Instead of doing the enforcement in the driver
it could be limited to checking a DT against the bindings in .yaml.
It's also an option to make it mandatory in the driver through a
list of compatible strings / VIDs/PIDs.
quoted
+ pdev = of_find_device_by_node(npc);
+ of_node_put(npc);
+
+ if (pdev) {
+ /* the companion hub already has a platform device, nothing to do here */
+ put_device(&pdev->dev);
+ goto node_put;
+ }
+
+ pdev = of_platform_device_create(np, NULL, &parent_hub->dev);
+ if (pdev) {
+ pdle = kzalloc(sizeof(*pdle), GFP_KERNEL);
Maybe devm_kzalloc(&pdev->dev, GFP_KERNEL) ? Then you can get rid of
the free in the destroy function?
it feels a bit sneaky to do it after creation instead of probe(), but I guess
it's fine.
quoted
+ if (!pdle)
+ goto node_put;
If your memory allocation fails here, don't you need to
of_platform_device_destroy() ?
right, will call of_platform_device_destroy() in case of failure
quoted
+ INIT_LIST_HEAD(&pdle->node);
I don't believe that the INIT_LIST_HEAD() does anything useful here.
&pdle->node is not a list head--it's a list element. Adding it to the
end of the existing list will fully initialize its ->next and ->prev
pointers but won't look at what they were.
It feels like you should be removing the node from the list too,
right? Otherwise if you unbind / bind the USB driver you'll still have
garbage in your list the 2nd time?
Could catch, it seems I limited testing to a single removal ...
Technically you don't need the "USB &&", right? Since
USB_XHCI_PLATFORM is defined in 'usb/host/Kconfig' and that's only
even included if USB is defined. So it can be just:
depends on (USB_XHCI_PLATFORM || USB_GADGET) && HAS_DMA
True, the dependency on USB isn't strictly needed.
That's not terribly important, though, so:
Reviewed-by: Douglas Anderson <dianders@chromium.org>