diff mbox series

[v5,3/4] usb: host: xhci-plat: Create platform device for onboard hubs in probe()

Message ID 20210210091015.v5.3.I7a3a7d9d2126c34079b1cab87aa0b2ec3030f9b7@changeid (mailing list archive)
State Superseded
Headers show
Series USB: misc: Add onboard_usb_hub driver | expand

Commit Message

Matthias Kaehlcke Feb. 10, 2021, 5:10 p.m. UTC
Check during probe() if a hub supported by the onboard_usb_hub
driver is connected to the controller. If such a hub is found
create the corresponding platform device. This requires the
device tree to have a node for the hub with its vendor and
product id (which is not common for USB devices). Further the
platform device is only created when CONFIG_USB_ONBOARD_HUB=y/m.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---

Changes in v5:
- patch added to the series

 drivers/usb/host/xhci-plat.c | 16 ++++++++++++++++
 include/linux/usb/hcd.h      |  2 ++
 2 files changed, 18 insertions(+)

Comments

Krzysztof Kozlowski Feb. 10, 2021, 9:06 p.m. UTC | #1
On Wed, Feb 10, 2021 at 09:10:38AM -0800, Matthias Kaehlcke wrote:
> Check during probe() if a hub supported by the onboard_usb_hub
> driver is connected to the controller. If such a hub is found
> create the corresponding platform device. This requires the
> device tree to have a node for the hub with its vendor and
> product id (which is not common for USB devices). Further the
> platform device is only created when CONFIG_USB_ONBOARD_HUB=y/m.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> 
> Changes in v5:
> - patch added to the series
> 
>  drivers/usb/host/xhci-plat.c | 16 ++++++++++++++++
>  include/linux/usb/hcd.h      |  2 ++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 4d34f6005381..e785fa109eea 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>
> @@ -184,6 +185,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	int			ret;
>  	int			irq;
>  	struct xhci_plat_priv	*priv = NULL;
> +	struct device_node	*np;
>  
>  
>  	if (usb_disabled())
> @@ -356,6 +358,17 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	 */
>  	pm_runtime_forbid(&pdev->dev);
>  
> +	np = usb_of_get_device_node(hcd->self.root_hub, hcd->self.busnum);
> +	if (np && of_is_onboard_usb_hub(np)) {

This looks hackish... what if later we have something else than hub?
Another if()?

What if hub could be connected to something else than XHCI controller?

Best regards,
Krzysztof
Matthias Kaehlcke Feb. 10, 2021, 10:20 p.m. UTC | #2
Hi Krzysztof,

On Wed, Feb 10, 2021 at 10:06:45PM +0100, Krzysztof Kozlowski wrote:
> On Wed, Feb 10, 2021 at 09:10:38AM -0800, Matthias Kaehlcke wrote:
> > Check during probe() if a hub supported by the onboard_usb_hub
> > driver is connected to the controller. If such a hub is found
> > create the corresponding platform device. This requires the
> > device tree to have a node for the hub with its vendor and
> > product id (which is not common for USB devices). Further the
> > platform device is only created when CONFIG_USB_ONBOARD_HUB=y/m.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > 
> > Changes in v5:
> > - patch added to the series
> > 
> >  drivers/usb/host/xhci-plat.c | 16 ++++++++++++++++
> >  include/linux/usb/hcd.h      |  2 ++
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 4d34f6005381..e785fa109eea 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>
> > @@ -184,6 +185,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	int			ret;
> >  	int			irq;
> >  	struct xhci_plat_priv	*priv = NULL;
> > +	struct device_node	*np;
> >  
> >  
> >  	if (usb_disabled())
> > @@ -356,6 +358,17 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	 */
> >  	pm_runtime_forbid(&pdev->dev);
> >  
> > +	np = usb_of_get_device_node(hcd->self.root_hub, hcd->self.busnum);
> > +	if (np && of_is_onboard_usb_hub(np)) {
> 
> This looks hackish... what if later we have something else than hub?
> Another if()?
> 
> What if hub could be connected to something else than XHCI controller?

In earlier versions this was standalone driver, which was more flexible and
didn't require cooperation from the XHCI driver:

https://lore.kernel.org/patchwork/patch/1313001/

Rob Herring raised objections about the DT bindings, since the USB hub would be
represented twice in the DT, once in the USB hierachry (with an explicit node or
implicitly) plus a node for the platform device for the new driver:

https://lore.kernel.org/patchwork/patch/1305395/
https://lore.kernel.org/patchwork/patch/1313000/

Alan Stern suggested to create the platform device in the XHCI platform driver:

https://lore.kernel.org/patchwork/patch/1313000/#1510227

I wasn't super happy about involving xhci-plat, but at least the code is minimal
and all the device specific stuff is handled by the onboard_usb_hub driver.

If you have better suggestions that might satisfy all parties please let us
know :)

Matthias
Greg KH Feb. 11, 2021, 6:58 a.m. UTC | #3
On Wed, Feb 10, 2021 at 09:10:38AM -0800, Matthias Kaehlcke wrote:
> Check during probe() if a hub supported by the onboard_usb_hub
> driver is connected to the controller. If such a hub is found
> create the corresponding platform device. This requires the
> device tree to have a node for the hub with its vendor and
> product id (which is not common for USB devices). Further the
> platform device is only created when CONFIG_USB_ONBOARD_HUB=y/m.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> 
> Changes in v5:
> - patch added to the series
> 
>  drivers/usb/host/xhci-plat.c | 16 ++++++++++++++++
>  include/linux/usb/hcd.h      |  2 ++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 4d34f6005381..e785fa109eea 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>
> @@ -184,6 +185,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	int			ret;
>  	int			irq;
>  	struct xhci_plat_priv	*priv = NULL;
> +	struct device_node	*np;
>  
>  
>  	if (usb_disabled())
> @@ -356,6 +358,17 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	 */
>  	pm_runtime_forbid(&pdev->dev);
>  
> +	np = usb_of_get_device_node(hcd->self.root_hub, hcd->self.busnum);
> +	if (np && of_is_onboard_usb_hub(np)) {
> +		struct platform_device *pdev;
> +
> +		pdev = of_platform_device_create(np, NULL, NULL);

A platform device is a child of another platform device?  Ok, but
really, why?  What uses this device?

thanks,

greg k-h
Matthias Kaehlcke Feb. 11, 2021, 6:45 p.m. UTC | #4
Hi Greg,

On Thu, Feb 11, 2021 at 07:58:23AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Feb 10, 2021 at 09:10:38AM -0800, Matthias Kaehlcke wrote:
> > Check during probe() if a hub supported by the onboard_usb_hub
> > driver is connected to the controller. If such a hub is found
> > create the corresponding platform device. This requires the
> > device tree to have a node for the hub with its vendor and
> > product id (which is not common for USB devices). Further the
> > platform device is only created when CONFIG_USB_ONBOARD_HUB=y/m.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > 
> > Changes in v5:
> > - patch added to the series
> > 
> >  drivers/usb/host/xhci-plat.c | 16 ++++++++++++++++
> >  include/linux/usb/hcd.h      |  2 ++
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 4d34f6005381..e785fa109eea 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>
> > @@ -184,6 +185,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	int			ret;
> >  	int			irq;
> >  	struct xhci_plat_priv	*priv = NULL;
> > +	struct device_node	*np;
> >  
> >  
> >  	if (usb_disabled())
> > @@ -356,6 +358,17 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	 */
> >  	pm_runtime_forbid(&pdev->dev);
> >  
> > +	np = usb_of_get_device_node(hcd->self.root_hub, hcd->self.busnum);
> > +	if (np && of_is_onboard_usb_hub(np)) {
> > +		struct platform_device *pdev;
> > +
> > +		pdev = of_platform_device_create(np, NULL, NULL);
> 
> A platform device is a child of another platform device?  Ok, but
> really, why?  What uses this device?

In earlier versions there was a standalone platform device:
https://lore.kernel.org/patchwork/patch/1313001/

However this was rejected by Rob, since the DT would require a node for the
platform device and (implicit or explicit) nodes for the USB devices,
representing the same physical device:

https://lore.kernel.org/patchwork/patch/1305395/
https://lore.kernel.org/patchwork/patch/1313000/

Both Doug Anderson and myself argued that it seems legitimate to distinguish
between the devices connected to the USB bus, and the chip which might have
GPIOs, regulators, clocks, ... but apparently our arguments were not
convincing enough.
Matthias Kaehlcke Feb. 11, 2021, 7:01 p.m. UTC | #5
On Thu, Feb 11, 2021 at 10:45:48AM -0800, Matthias Kaehlcke wrote:
> Hi Greg,
> 
> On Thu, Feb 11, 2021 at 07:58:23AM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Feb 10, 2021 at 09:10:38AM -0800, Matthias Kaehlcke wrote:
> > > Check during probe() if a hub supported by the onboard_usb_hub
> > > driver is connected to the controller. If such a hub is found
> > > create the corresponding platform device. This requires the
> > > device tree to have a node for the hub with its vendor and
> > > product id (which is not common for USB devices). Further the
> > > platform device is only created when CONFIG_USB_ONBOARD_HUB=y/m.
> > > 
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > > 
> > > Changes in v5:
> > > - patch added to the series
> > > 
> > >  drivers/usb/host/xhci-plat.c | 16 ++++++++++++++++
> > >  include/linux/usb/hcd.h      |  2 ++
> > >  2 files changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > > index 4d34f6005381..e785fa109eea 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>
> > > @@ -184,6 +185,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > >  	int			ret;
> > >  	int			irq;
> > >  	struct xhci_plat_priv	*priv = NULL;
> > > +	struct device_node	*np;
> > >  
> > >  
> > >  	if (usb_disabled())
> > > @@ -356,6 +358,17 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > >  	 */
> > >  	pm_runtime_forbid(&pdev->dev);
> > >  
> > > +	np = usb_of_get_device_node(hcd->self.root_hub, hcd->self.busnum);
> > > +	if (np && of_is_onboard_usb_hub(np)) {
> > > +		struct platform_device *pdev;
> > > +
> > > +		pdev = of_platform_device_create(np, NULL, NULL);
> > 
> > A platform device is a child of another platform device?  Ok, but
> > really, why?  What uses this device?
> 
> In earlier versions there was a standalone platform device:
> https://lore.kernel.org/patchwork/patch/1313001/
> 
> However this was rejected by Rob, since the DT would require a node for the
> platform device and (implicit or explicit) nodes for the USB devices,
> representing the same physical device:
> 
> https://lore.kernel.org/patchwork/patch/1305395/
> https://lore.kernel.org/patchwork/patch/1313000/
> 
> Both Doug Anderson and myself argued that it seems legitimate to distinguish
> between the devices connected to the USB bus, and the chip which might have
> GPIOs, regulators, clocks, ... but apparently our arguments were not
> convincing enough.

To let the xhci-plat driver create the platform device was suggested by Alan:

https://lore.kernel.org/patchwork/patch/1313000/#1510227

Personally I would favor a standalone platform device, since it provides more
flexiblity (also works for hubs connected to a non-root hub) and doesn't require
cooperation from other driver, however I doubt I could convince Rob of the
corresponding DT bindings.
Stephen Boyd Feb. 11, 2021, 7:14 p.m. UTC | #6
Quoting Matthias Kaehlcke (2021-02-10 14:20:18)
> 
> On Wed, Feb 10, 2021 at 10:06:45PM +0100, Krzysztof Kozlowski wrote:
> > 
> > This looks hackish... what if later we have something else than hub?
> > Another if()?
> > 
> > What if hub could be connected to something else than XHCI controller?
> 
> In earlier versions this was standalone driver, which was more flexible and
> didn't require cooperation from the XHCI driver:
> 
> https://lore.kernel.org/patchwork/patch/1313001/
> 
> Rob Herring raised objections about the DT bindings, since the USB hub would be
> represented twice in the DT, once in the USB hierachry (with an explicit node or
> implicitly) plus a node for the platform device for the new driver:
> 
> https://lore.kernel.org/patchwork/patch/1305395/
> https://lore.kernel.org/patchwork/patch/1313000/
> 
> Alan Stern suggested to create the platform device in the XHCI platform driver:
> 
> https://lore.kernel.org/patchwork/patch/1313000/#1510227
> 
> I wasn't super happy about involving xhci-plat, but at least the code is minimal
> and all the device specific stuff is handled by the onboard_usb_hub driver.
> 
> If you have better suggestions that might satisfy all parties please let us
> know :)
> 

Is it possible to use the graph binding to connect the USB controller on
the SoC to the port on the hub? Then the hub would be a standalone node
at the root of DT connected to the USB controller (or phy) and xhci code
could probe the firmware to see if there's a graph connection downstream
that is a powered hub like this. I didn't see this idea mentioned in the
previous discussions, but maybe I missed it.
Matthias Kaehlcke Feb. 11, 2021, 8:36 p.m. UTC | #7
Hi Stephen,

On Thu, Feb 11, 2021 at 11:14:39AM -0800, Stephen Boyd wrote:
> Quoting Matthias Kaehlcke (2021-02-10 14:20:18)
> > 
> > On Wed, Feb 10, 2021 at 10:06:45PM +0100, Krzysztof Kozlowski wrote:
> > > 
> > > This looks hackish... what if later we have something else than hub?
> > > Another if()?
> > > 
> > > What if hub could be connected to something else than XHCI controller?
> > 
> > In earlier versions this was standalone driver, which was more flexible and
> > didn't require cooperation from the XHCI driver:
> > 
> > https://lore.kernel.org/patchwork/patch/1313001/
> > 
> > Rob Herring raised objections about the DT bindings, since the USB hub would be
> > represented twice in the DT, once in the USB hierachry (with an explicit node or
> > implicitly) plus a node for the platform device for the new driver:
> > 
> > https://lore.kernel.org/patchwork/patch/1305395/
> > https://lore.kernel.org/patchwork/patch/1313000/
> > 
> > Alan Stern suggested to create the platform device in the XHCI platform driver:
> > 
> > https://lore.kernel.org/patchwork/patch/1313000/#1510227
> > 
> > I wasn't super happy about involving xhci-plat, but at least the code is minimal
> > and all the device specific stuff is handled by the onboard_usb_hub driver.
> > 
> > If you have better suggestions that might satisfy all parties please let us
> > know :)
> > 
> 
> Is it possible to use the graph binding to connect the USB controller on
> the SoC to the port on the hub? Then the hub would be a standalone node
> at the root of DT connected to the USB controller (or phy) and xhci code
> could probe the firmware to see if there's a graph connection downstream
> that is a powered hub like this. I didn't see this idea mentioned in the
> previous discussions, but maybe I missed it.

Thanks for bringing this up. I'm not sure I completely understand your
suggestion, but in general it seems a direction that could be worth
exploring.

I think something like the following should work even without requiring
cooperation from the XHCI code:

onboard-usb-hub {
    compatible = “realtek,rts5411”, “onboard_usb_hub”;
    #address-cells = <1>;
    #size-cells = <0>;

    vdd-supply = <&pp3300_hub>;

    port@0 {
        reg = <0>;

        rts5411_3_0: endpoint {
            // should not be needed
            remote-endpoint = <&usb_1_dwc3_port1>;
        };
    };

    port@1 {
        reg = <1>;

        rts5411_2_0: endpoint {
            // should not be needed
            remote-endpoint = <&usb_1_dwc3_port2>;
        };
    };
};

&usb_1_dwc3 {
    dr_mode = "host";
    #address-cells = <1>;
    #size-cells = <0>;

    port@1 {
        reg = <1>;

        usb_1_dwc3_port1: endpoint {
            remote-endpoint = <&rts5411_3_0>;
        };
    };

    port@2 {
        reg = <2>;

        usb_1_dwc3_port2: endpoint {
            remote-endpoint = <&rts5411_2_0>;
        };
    };
};

That looks like an actual description of the hardware, without multiple DT
nodes for the hub.

The USB part of the onboard_hub driver could determine the platform device
from the remote endpoint and register the USB device with it.
Stephen Boyd Feb. 11, 2021, 8:45 p.m. UTC | #8
Quoting Matthias Kaehlcke (2021-02-11 12:36:35)
> Hi Stephen,
> 
> On Thu, Feb 11, 2021 at 11:14:39AM -0800, Stephen Boyd wrote:
> > 
> > Is it possible to use the graph binding to connect the USB controller on
> > the SoC to the port on the hub? Then the hub would be a standalone node
> > at the root of DT connected to the USB controller (or phy) and xhci code
> > could probe the firmware to see if there's a graph connection downstream
> > that is a powered hub like this. I didn't see this idea mentioned in the
> > previous discussions, but maybe I missed it.
> 
> Thanks for bringing this up. I'm not sure I completely understand your
> suggestion, but in general it seems a direction that could be worth
> exploring.

Cool.

> 
> I think something like the following should work even without requiring
> cooperation from the XHCI code:
> 
> onboard-usb-hub {
>     compatible = “realtek,rts5411”, “onboard_usb_hub”;
>     #address-cells = <1>;
>     #size-cells = <0>;
> 
>     vdd-supply = <&pp3300_hub>;
> 
>     port@0 {
>         reg = <0>;
> 
>         rts5411_3_0: endpoint {
>             // should not be needed
>             remote-endpoint = <&usb_1_dwc3_port1>;
>         };
>     };
> 
>     port@1 {
>         reg = <1>;
> 
>         rts5411_2_0: endpoint {
>             // should not be needed
>             remote-endpoint = <&usb_1_dwc3_port2>;
>         };
>     };
> };
> 
> &usb_1_dwc3 {
>     dr_mode = "host";
>     #address-cells = <1>;
>     #size-cells = <0>;
> 
>     port@1 {
>         reg = <1>;
> 
>         usb_1_dwc3_port1: endpoint {
>             remote-endpoint = <&rts5411_3_0>;
>         };
>     };
> 
>     port@2 {
>         reg = <2>;
> 
>         usb_1_dwc3_port2: endpoint {
>             remote-endpoint = <&rts5411_2_0>;
>         };
>     };
> };
> 
> That looks like an actual description of the hardware, without multiple DT
> nodes for the hub.
> 
> The USB part of the onboard_hub driver could determine the platform device
> from the remote endpoint and register the USB device with it.

Yeah you got it, this is what I'm suggesting. I'd like to go even
further and also describe the ports on "the other side" or "downstream
facing side" of the USB hub that go to things like type-c ports or
type-a ports. This would allow us to describe the USB topology and how
it is physically connected to ports on the device that humans see. But
that's mostly a job for the type-c subsystem, so it's a parallel
discussion that's only related because of the graph binding.
Rob Herring Feb. 11, 2021, 10:40 p.m. UTC | #9
On Thu, Feb 11, 2021 at 1:14 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Matthias Kaehlcke (2021-02-10 14:20:18)
> >
> > On Wed, Feb 10, 2021 at 10:06:45PM +0100, Krzysztof Kozlowski wrote:
> > >
> > > This looks hackish... what if later we have something else than hub?
> > > Another if()?
> > >
> > > What if hub could be connected to something else than XHCI controller?
> >
> > In earlier versions this was standalone driver, which was more flexible and
> > didn't require cooperation from the XHCI driver:
> >
> > https://lore.kernel.org/patchwork/patch/1313001/
> >
> > Rob Herring raised objections about the DT bindings, since the USB hub would be
> > represented twice in the DT, once in the USB hierachry (with an explicit node or
> > implicitly) plus a node for the platform device for the new driver:
> >
> > https://lore.kernel.org/patchwork/patch/1305395/
> > https://lore.kernel.org/patchwork/patch/1313000/
> >
> > Alan Stern suggested to create the platform device in the XHCI platform driver:
> >
> > https://lore.kernel.org/patchwork/patch/1313000/#1510227
> >
> > I wasn't super happy about involving xhci-plat, but at least the code is minimal
> > and all the device specific stuff is handled by the onboard_usb_hub driver.
> >
> > If you have better suggestions that might satisfy all parties please let us
> > know :)
> >
>
> Is it possible to use the graph binding to connect the USB controller on
> the SoC to the port on the hub? Then the hub would be a standalone node
> at the root of DT connected to the USB controller (or phy) and xhci code
> could probe the firmware to see if there's a graph connection downstream
> that is a powered hub like this. I didn't see this idea mentioned in the
> previous discussions, but maybe I missed it.

An orphan at the root node is exactly what I don't want to see here.
I'm happy with the binding now from a skim of it. The kernel support
does seem kind of hacky though.

Rob
Rob Herring Feb. 11, 2021, 10:46 p.m. UTC | #10
On Thu, Feb 11, 2021 at 2:45 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Matthias Kaehlcke (2021-02-11 12:36:35)
> > Hi Stephen,
> >
> > On Thu, Feb 11, 2021 at 11:14:39AM -0800, Stephen Boyd wrote:
> > >
> > > Is it possible to use the graph binding to connect the USB controller on
> > > the SoC to the port on the hub? Then the hub would be a standalone node
> > > at the root of DT connected to the USB controller (or phy) and xhci code
> > > could probe the firmware to see if there's a graph connection downstream
> > > that is a powered hub like this. I didn't see this idea mentioned in the
> > > previous discussions, but maybe I missed it.
> >
> > Thanks for bringing this up. I'm not sure I completely understand your
> > suggestion, but in general it seems a direction that could be worth
> > exploring.
>
> Cool.
>
> >
> > I think something like the following should work even without requiring
> > cooperation from the XHCI code:
> >
> > onboard-usb-hub {
> >     compatible = “realtek,rts5411”, “onboard_usb_hub”;
> >     #address-cells = <1>;
> >     #size-cells = <0>;
> >
> >     vdd-supply = <&pp3300_hub>;
> >
> >     port@0 {
> >         reg = <0>;
> >
> >         rts5411_3_0: endpoint {
> >             // should not be needed
> >             remote-endpoint = <&usb_1_dwc3_port1>;
> >         };
> >     };
> >
> >     port@1 {
> >         reg = <1>;
> >
> >         rts5411_2_0: endpoint {
> >             // should not be needed
> >             remote-endpoint = <&usb_1_dwc3_port2>;
> >         };
> >     };
> > };
> >
> > &usb_1_dwc3 {
> >     dr_mode = "host";
> >     #address-cells = <1>;
> >     #size-cells = <0>;
> >
> >     port@1 {
> >         reg = <1>;
> >
> >         usb_1_dwc3_port1: endpoint {
> >             remote-endpoint = <&rts5411_3_0>;
> >         };
> >     };
> >
> >     port@2 {
> >         reg = <2>;
> >
> >         usb_1_dwc3_port2: endpoint {
> >             remote-endpoint = <&rts5411_2_0>;
> >         };
> >     };
> > };
> >
> > That looks like an actual description of the hardware, without multiple DT
> > nodes for the hub.
> >
> > The USB part of the onboard_hub driver could determine the platform device
> > from the remote endpoint and register the USB device with it.
>
> Yeah you got it, this is what I'm suggesting. I'd like to go even
> further and also describe the ports on "the other side" or "downstream
> facing side" of the USB hub that go to things like type-c ports or
> type-a ports. This would allow us to describe the USB topology and how
> it is physically connected to ports on the device that humans see. But
> that's mostly a job for the type-c subsystem, so it's a parallel
> discussion that's only related because of the graph binding.

I'd like to see a complete case that includes connectors. I really
need someone to worry about USB bindings with a view on the big
picture. The piecemeal additions don't result in the best design.

Rob
diff mbox series

Patch

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 4d34f6005381..e785fa109eea 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>
@@ -184,6 +185,7 @@  static int xhci_plat_probe(struct platform_device *pdev)
 	int			ret;
 	int			irq;
 	struct xhci_plat_priv	*priv = NULL;
+	struct device_node	*np;
 
 
 	if (usb_disabled())
@@ -356,6 +358,17 @@  static int xhci_plat_probe(struct platform_device *pdev)
 	 */
 	pm_runtime_forbid(&pdev->dev);
 
+	np = usb_of_get_device_node(hcd->self.root_hub, hcd->self.busnum);
+	if (np && of_is_onboard_usb_hub(np)) {
+		struct platform_device *pdev;
+
+		pdev = of_platform_device_create(np, NULL, NULL);
+		if (pdev)
+			hcd->onboard_hub_dev = &pdev->dev;
+		else
+			xhci_warn(xhci, "failed to create onboard hub platform device\n");
+	}
+
 	return 0;
 
 
@@ -402,6 +415,9 @@  static int xhci_plat_remove(struct platform_device *dev)
 	usb_remove_hcd(hcd);
 	usb_put_hcd(shared_hcd);
 
+	if (hcd->onboard_hub_dev)
+		of_platform_device_destroy(hcd->onboard_hub_dev, NULL);
+
 	clk_disable_unprepare(clk);
 	clk_disable_unprepare(reg_clk);
 	usb_put_hcd(hcd);
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 96281cd50ff6..d02a508e19b0 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -225,6 +225,8 @@  struct usb_hcd {
 	 * (ohci 32, uhci 1024, ehci 256/512/1024).
 	 */
 
+	struct device *onboard_hub_dev;
+
 	/* The HC driver's private data is stored at the end of
 	 * this structure.
 	 */