| Message ID | 1456999276-6315-3-git-send-email-peter.chen@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 03, 2016 at 06:01:15PM +0800, Peter Chen wrote: > From: Peter Chen <peter.chen@freescale.com> > > Since the hcd (chipidea core device) has no device node, so > if we want to describe the child node under the hcd, we had > to put it under its parent's node (glue layer device), and > in the code, we need to let the hcd knows glue layer's code, > then the USB core can handle this node. > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > --- > drivers/usb/chipidea/host.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c > index 053bac9..55120ef 100644 > --- a/drivers/usb/chipidea/host.c > +++ b/drivers/usb/chipidea/host.c > @@ -109,15 +109,25 @@ static int host_start(struct ci_hdrc *ci) > struct ehci_hcd *ehci; > struct ehci_ci_priv *priv; > int ret; > + struct device *dev = ci->dev; > > - if (usb_disabled()) > + if (usb_disabled() || !dev) > return -ENODEV; > > - hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev)); > + /* > + * USB Core will try to get child node under roothub, > + * but chipidea core has no of_node, and the child node > + * for controller is located at glue layer's node which > + * is chipidea core's parent. > + */ > + if (dev->parent && dev->parent->of_node) > + dev->of_node = dev->parent->of_node; Is this a good idea? Two devices with the same of_node? I know the networking code assumes of_node values are unique, and uses it to find a device. Are you 100% sure the USB code does not make this assumption. Andrew
On Thu, Mar 03, 2016 at 03:42:47PM +0100, Andrew Lunn wrote: > On Thu, Mar 03, 2016 at 06:01:15PM +0800, Peter Chen wrote: > > From: Peter Chen <peter.chen@freescale.com> > > > > Since the hcd (chipidea core device) has no device node, so > > if we want to describe the child node under the hcd, we had > > to put it under its parent's node (glue layer device), and > > in the code, we need to let the hcd knows glue layer's code, > > then the USB core can handle this node. > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > --- > > drivers/usb/chipidea/host.c | 18 ++++++++++++++---- > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c > > index 053bac9..55120ef 100644 > > --- a/drivers/usb/chipidea/host.c > > +++ b/drivers/usb/chipidea/host.c > > @@ -109,15 +109,25 @@ static int host_start(struct ci_hdrc *ci) > > struct ehci_hcd *ehci; > > struct ehci_ci_priv *priv; > > int ret; > > + struct device *dev = ci->dev; > > > > - if (usb_disabled()) > > + if (usb_disabled() || !dev) > > return -ENODEV; > > > > - hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev)); > > + /* > > + * USB Core will try to get child node under roothub, > > + * but chipidea core has no of_node, and the child node > > + * for controller is located at glue layer's node which > > + * is chipidea core's parent. > > + */ > > + if (dev->parent && dev->parent->of_node) > > + dev->of_node = dev->parent->of_node; > > Is this a good idea? Two devices with the same of_node? > This is only for chipidea driver whose host controller device doesn't have entry at dts, but other host controller driver which supports device tree should have its entry at dts. > I know the networking code assumes of_node values are unique, and uses > it to find a device. Are you 100% sure the USB code does not make this > assumption. > The controller device is the root for USB device, the common USB code will not touch its glue layer device (controller's parent).
> > > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c > > > index 053bac9..55120ef 100644 > > > --- a/drivers/usb/chipidea/host.c > > > +++ b/drivers/usb/chipidea/host.c > > > @@ -109,15 +109,25 @@ static int host_start(struct ci_hdrc *ci) > > > struct ehci_hcd *ehci; > > > struct ehci_ci_priv *priv; > > > int ret; > > > + struct device *dev = ci->dev; > > > > > > - if (usb_disabled()) > > > + if (usb_disabled() || !dev) > > > return -ENODEV; > > > > > > - hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev)); > > > + /* > > > + * USB Core will try to get child node under roothub, > > > + * but chipidea core has no of_node, and the child node > > > + * for controller is located at glue layer's node which > > > + * is chipidea core's parent. > > > + */ > > > + if (dev->parent && dev->parent->of_node) > > > + dev->of_node = dev->parent->of_node; > > > > Is this a good idea? Two devices with the same of_node? > > > > This is only for chipidea driver whose host controller device > doesn't have entry at dts, but other host controller driver which > supports device tree should have its entry at dts. > > > I know the networking code assumes of_node values are unique, and uses > > it to find a device. Are you 100% sure the USB code does not make this > > assumption. > > > > The controller device is the root for USB device, the common > USB code will not touch its glue layer device (controller's parent). I'm just thinking about code like: of_find_spi_master_by_node(), of_find_net_device_by_node(), of_find_backlight_by_node(), etc. If somebody was to implement an of_find_usb_host_by_node() are you 100% sure the right node will be found? This seems like a bug waiting to happen. Andrew
On Fri, Mar 04, 2016 at 03:17:30AM +0100, Andrew Lunn wrote: > > > > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c > > > > index 053bac9..55120ef 100644 > > > > --- a/drivers/usb/chipidea/host.c > > > > +++ b/drivers/usb/chipidea/host.c > > > > @@ -109,15 +109,25 @@ static int host_start(struct ci_hdrc *ci) > > > > struct ehci_hcd *ehci; > > > > struct ehci_ci_priv *priv; > > > > int ret; > > > > + struct device *dev = ci->dev; > > > > > > > > - if (usb_disabled()) > > > > + if (usb_disabled() || !dev) > > > > return -ENODEV; > > > > > > > > - hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev)); > > > > + /* > > > > + * USB Core will try to get child node under roothub, > > > > + * but chipidea core has no of_node, and the child node > > > > + * for controller is located at glue layer's node which > > > > + * is chipidea core's parent. > > > > + */ > > > > + if (dev->parent && dev->parent->of_node) > > > > + dev->of_node = dev->parent->of_node; > > > > > > Is this a good idea? Two devices with the same of_node? > > > > > > > This is only for chipidea driver whose host controller device > > doesn't have entry at dts, but other host controller driver which > > supports device tree should have its entry at dts. > > > > > I know the networking code assumes of_node values are unique, and uses > > > it to find a device. Are you 100% sure the USB code does not make this > > > assumption. > > > > > > > The controller device is the root for USB device, the common > > USB code will not touch its glue layer device (controller's parent). > > I'm just thinking about code like: > > of_find_spi_master_by_node(), of_find_net_device_by_node(), > of_find_backlight_by_node(), etc. > > If somebody was to implement an of_find_usb_host_by_node() are you > 100% sure the right node will be found? This seems like a bug waiting > to happen. Yes, almost 100% sure it is ok since the common USB code will never use its root's parent node to match or get something.
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index 053bac9..55120ef 100644 --- a/drivers/usb/chipidea/host.c +++ b/drivers/usb/chipidea/host.c @@ -109,15 +109,25 @@ static int host_start(struct ci_hdrc *ci) struct ehci_hcd *ehci; struct ehci_ci_priv *priv; int ret; + struct device *dev = ci->dev; - if (usb_disabled()) + if (usb_disabled() || !dev) return -ENODEV; - hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev)); + /* + * USB Core will try to get child node under roothub, + * but chipidea core has no of_node, and the child node + * for controller is located at glue layer's node which + * is chipidea core's parent. + */ + if (dev->parent && dev->parent->of_node) + dev->of_node = dev->parent->of_node; + + hcd = usb_create_hcd(&ci_ehci_hc_driver, dev, dev_name(dev)); if (!hcd) return -ENOMEM; - dev_set_drvdata(ci->dev, ci); + dev_set_drvdata(dev, ci); hcd->rsrc_start = ci->hw_bank.phys; hcd->rsrc_len = ci->hw_bank.size; hcd->regs = ci->hw_bank.abs; @@ -143,7 +153,7 @@ static int host_start(struct ci_hdrc *ci) if (ci->platdata->flags & CI_HDRC_TURN_VBUS_EARLY_ON) { ret = regulator_enable(ci->platdata->reg_vbus); if (ret) { - dev_err(ci->dev, + dev_err(dev, "Failed to enable vbus regulator, ret=%d\n", ret); goto put_hcd;