| Message ID | 20210813125146.v16.6.I7a3a7d9d2126c34079b1cab87aa0b2ec3030f9b7@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: misc: Add onboard_usb_hub driver | expand |
Hi On 13.8.2021 22.52, Matthias Kaehlcke wrote: > 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
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: > > 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.
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: > > 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: > > Hi Mathias, > > > > On Wed, Oct 20, 2021 at 04:05:37PM +0300, Mathias Nyman wrote: > > > 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?
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: > > 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: > > > > 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
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index 518c2312ef0c..099e9615919c 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -54,6 +54,7 @@ config USB_XHCI_PCI_RENESAS config USB_XHCI_PLATFORM tristate "Generic xHCI driver for a platform device" select USB_XHCI_RCAR if ARCH_RENESAS + depends on USB_ONBOARD_HUB || !USB_ONBOARD_HUB # if USB_ONBOARD_HUB=m, this can't be 'y' help Adds an xHCI host driver for a generic platform device, which provides a memory space and an irq. diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index c1edcc9b13ce..ee98a3671619 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -15,6 +15,7 @@ #include <linux/of.h> #include <linux/of_device.h> #include <linux/platform_device.h> +#include <linux/usb/onboard_hub.h> #include <linux/usb/phy.h> #include <linux/slab.h> #include <linux/acpi.h> @@ -374,6 +375,9 @@ static int xhci_plat_probe(struct platform_device *pdev) */ pm_runtime_forbid(&pdev->dev); + INIT_LIST_HEAD(&xhci->onboard_hub_devs); + onboard_hub_create_pdevs(hcd->self.root_hub, &xhci->onboard_hub_devs); + return 0; @@ -420,6 +424,8 @@ static int xhci_plat_remove(struct platform_device *dev) usb_remove_hcd(hcd); usb_put_hcd(shared_hcd); + onboard_hub_destroy_pdevs(&xhci->onboard_hub_devs); + clk_disable_unprepare(clk); clk_disable_unprepare(reg_clk); usb_put_hcd(hcd); diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 3c7d281672ae..5ba01d5ccab8 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1923,6 +1923,8 @@ struct xhci_hcd { struct dentry *debugfs_slots; struct list_head regset_list; + struct list_head onboard_hub_devs; + void *dbc; /* platform-specific data -- must come last */ unsigned long priv[] __aligned(sizeof(s64));
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(+)