| Message ID | 1378136591-7463-2-git-send-email-kishon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Kishon, On 09/02/2013 06:43 PM, Kishon Vijay Abraham I wrote: > There can be systems which does not have a external usb_phy, so get > usb_phy only if usb-phy property is added in the case of dt boot or if > platform_data indicates the presence of PHY. Also remove checking if > return value is -ENXIO since it's now changed to always enable usb_phy layer. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/usb/dwc3/Kconfig | 1 + > drivers/usb/dwc3/core.c | 60 +++++++++++++++++--------------------- > drivers/usb/dwc3/platform_data.h | 1 + > 3 files changed, 28 insertions(+), 34 deletions(-) > > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig > index f969ea2..cfc16dd 100644 > --- a/drivers/usb/dwc3/Kconfig > +++ b/drivers/usb/dwc3/Kconfig > @@ -2,6 +2,7 @@ config USB_DWC3 > tristate "DesignWare USB3 DRD Core Support" > depends on (USB || USB_GADGET) && GENERIC_HARDIRQS && HAS_DMA > depends on EXTCON > + select USB_PHY > select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD > help > Say Y or M here if your system has a Dual Role SuperSpeed > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 474162e..428c29e 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -387,16 +387,38 @@ static int dwc3_probe(struct platform_device *pdev) > if (node) { > dwc->maximum_speed = of_usb_get_maximum_speed(node); > > - dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); > - dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1); > + if (of_property_read_bool(node, "usb-phy")) { > + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, > + "usb-phy", 0); > + if (IS_ERR(dwc->usb2_phy)) > + return PTR_ERR(dwc->usb2_phy); > + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, > + "usb-phy", 1); > + if (IS_ERR(dwc->usb3_phy)) > + return PTR_ERR(dwc->usb3_phy); Some DWC3 instances use only usb2_phy. e.g. on DRA7 the 2nd dwc3 instance doesn't use usb3_phy. This needs to be a valid case and driver shouldn't error out. > + } else { > + dwc->usb2_phy = NULL; > + dwc->usb3_phy = NULL; > + } > > dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize"); > dwc->dr_mode = of_usb_get_dr_mode(node); > } else if (pdata) { > dwc->maximum_speed = pdata->maximum_speed; > > - dwc->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2); > - dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3); > + if (pdata->has_phy) { > + dwc->usb2_phy = devm_usb_get_phy(dev, > + USB_PHY_TYPE_USB2); > + if (IS_ERR(dwc->usb2_phy)) > + return PTR_ERR(dwc->usb2_phy); > + dwc->usb3_phy = devm_usb_get_phy(dev, > + USB_PHY_TYPE_USB3); > + if (IS_ERR(dwc->usb3_phy)) > + return PTR_ERR(dwc->usb3_phy); same here? > + } else { > + dwc->usb2_phy = NULL; > + dwc->usb3_phy = NULL; > + } > > dwc->needs_fifo_resize = pdata->tx_fifo_resize; > dwc->dr_mode = pdata->dr_mode; > @@ -409,36 +431,6 @@ static int dwc3_probe(struct platform_device *pdev) > if (dwc->maximum_speed == USB_SPEED_UNKNOWN) > dwc->maximum_speed = USB_SPEED_SUPER; > > - if (IS_ERR(dwc->usb2_phy)) { > - ret = PTR_ERR(dwc->usb2_phy); > - > - /* > - * if -ENXIO is returned, it means PHY layer wasn't > - * enabled, so it makes no sense to return -EPROBE_DEFER > - * in that case, since no PHY driver will ever probe. > - */ > - if (ret == -ENXIO) > - return ret; > - > - dev_err(dev, "no usb2 phy configured\n"); > - return -EPROBE_DEFER; > - } > - > - if (IS_ERR(dwc->usb3_phy)) { > - ret = PTR_ERR(dwc->usb3_phy); > - > - /* > - * if -ENXIO is returned, it means PHY layer wasn't > - * enabled, so it makes no sense to return -EPROBE_DEFER > - * in that case, since no PHY driver will ever probe. > - */ > - if (ret == -ENXIO) > - return ret; > - > - dev_err(dev, "no usb3 phy configured\n"); > - return -EPROBE_DEFER; > - } > - > dwc->xhci_resources[0].start = res->start; > dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + > DWC3_XHCI_REGS_END; > diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h > index 7db34f0..5a5e068 100644 > --- a/drivers/usb/dwc3/platform_data.h > +++ b/drivers/usb/dwc3/platform_data.h > @@ -24,4 +24,5 @@ struct dwc3_platform_data { > enum usb_device_speed maximum_speed; > enum usb_dr_mode dr_mode; > bool tx_fifo_resize; > + bool has_phy; > }; > cheers, -roger
On Thu, Sep 12, 2013 at 4:06 PM, Roger Quadros <rogerq@ti.com> wrote: > Hi Kishon, > > On 09/02/2013 06:43 PM, Kishon Vijay Abraham I wrote: >> There can be systems which does not have a external usb_phy, so get >> usb_phy only if usb-phy property is added in the case of dt boot or if >> platform_data indicates the presence of PHY. Also remove checking if >> return value is -ENXIO since it's now changed to always enable usb_phy layer. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> drivers/usb/dwc3/Kconfig | 1 + >> drivers/usb/dwc3/core.c | 60 +++++++++++++++++--------------------- >> drivers/usb/dwc3/platform_data.h | 1 + >> 3 files changed, 28 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >> index f969ea2..cfc16dd 100644 >> --- a/drivers/usb/dwc3/Kconfig >> +++ b/drivers/usb/dwc3/Kconfig >> @@ -2,6 +2,7 @@ config USB_DWC3 >> tristate "DesignWare USB3 DRD Core Support" >> depends on (USB || USB_GADGET) && GENERIC_HARDIRQS && HAS_DMA >> depends on EXTCON >> + select USB_PHY >> select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD >> help >> Say Y or M here if your system has a Dual Role SuperSpeed >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 474162e..428c29e 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -387,16 +387,38 @@ static int dwc3_probe(struct platform_device *pdev) >> if (node) { >> dwc->maximum_speed = of_usb_get_maximum_speed(node); >> >> - dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); >> - dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1); >> + if (of_property_read_bool(node, "usb-phy")) { >> + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, >> + "usb-phy", 0); >> + if (IS_ERR(dwc->usb2_phy)) >> + return PTR_ERR(dwc->usb2_phy); >> + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, >> + "usb-phy", 1); >> + if (IS_ERR(dwc->usb3_phy)) >> + return PTR_ERR(dwc->usb3_phy); > > Some DWC3 instances use only usb2_phy. e.g. on DRA7 the 2nd dwc3 instance doesn't use usb3_phy. > This needs to be a valid case and driver shouldn't error out. So, i think adding flexibility to DWC3 to have either usb2-phy/usb3-phy or both of them seems to be valid point. Any suggestions ? > >> + } else { >> + dwc->usb2_phy = NULL; >> + dwc->usb3_phy = NULL; >> + } >> >> dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize"); >> dwc->dr_mode = of_usb_get_dr_mode(node); >> } else if (pdata) { >> dwc->maximum_speed = pdata->maximum_speed; >> >> - dwc->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2); >> - dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3); >> + if (pdata->has_phy) { >> + dwc->usb2_phy = devm_usb_get_phy(dev, >> + USB_PHY_TYPE_USB2); >> + if (IS_ERR(dwc->usb2_phy)) >> + return PTR_ERR(dwc->usb2_phy); >> + dwc->usb3_phy = devm_usb_get_phy(dev, >> + USB_PHY_TYPE_USB3); >> + if (IS_ERR(dwc->usb3_phy)) >> + return PTR_ERR(dwc->usb3_phy); > > same here? > >> + } else { >> + dwc->usb2_phy = NULL; >> + dwc->usb3_phy = NULL; >> + } >> >> dwc->needs_fifo_resize = pdata->tx_fifo_resize; >> dwc->dr_mode = pdata->dr_mode; >> @@ -409,36 +431,6 @@ static int dwc3_probe(struct platform_device *pdev) >> if (dwc->maximum_speed == USB_SPEED_UNKNOWN) >> dwc->maximum_speed = USB_SPEED_SUPER; >> >> - if (IS_ERR(dwc->usb2_phy)) { >> - ret = PTR_ERR(dwc->usb2_phy); >> - >> - /* >> - * if -ENXIO is returned, it means PHY layer wasn't >> - * enabled, so it makes no sense to return -EPROBE_DEFER >> - * in that case, since no PHY driver will ever probe. >> - */ >> - if (ret == -ENXIO) >> - return ret; >> - >> - dev_err(dev, "no usb2 phy configured\n"); >> - return -EPROBE_DEFER; >> - } >> - >> - if (IS_ERR(dwc->usb3_phy)) { >> - ret = PTR_ERR(dwc->usb3_phy); >> - >> - /* >> - * if -ENXIO is returned, it means PHY layer wasn't >> - * enabled, so it makes no sense to return -EPROBE_DEFER >> - * in that case, since no PHY driver will ever probe. >> - */ >> - if (ret == -ENXIO) >> - return ret; >> - >> - dev_err(dev, "no usb3 phy configured\n"); >> - return -EPROBE_DEFER; >> - } >> - >> dwc->xhci_resources[0].start = res->start; >> dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + >> DWC3_XHCI_REGS_END; >> diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h >> index 7db34f0..5a5e068 100644 >> --- a/drivers/usb/dwc3/platform_data.h >> +++ b/drivers/usb/dwc3/platform_data.h >> @@ -24,4 +24,5 @@ struct dwc3_platform_data { >> enum usb_device_speed maximum_speed; >> enum usb_dr_mode dr_mode; >> bool tx_fifo_resize; >> + bool has_phy; >> }; >> > > cheers, > -roger > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
Hi, On 09/12/2013 01:47 PM, Vivek Gautam wrote: > On Thu, Sep 12, 2013 at 4:06 PM, Roger Quadros <rogerq@ti.com> wrote: >> Hi Kishon, >> >> On 09/02/2013 06:43 PM, Kishon Vijay Abraham I wrote: >>> There can be systems which does not have a external usb_phy, so get >>> usb_phy only if usb-phy property is added in the case of dt boot or if >>> platform_data indicates the presence of PHY. Also remove checking if >>> return value is -ENXIO since it's now changed to always enable usb_phy layer. >>> >>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>> --- >>> drivers/usb/dwc3/Kconfig | 1 + >>> drivers/usb/dwc3/core.c | 60 +++++++++++++++++--------------------- >>> drivers/usb/dwc3/platform_data.h | 1 + >>> 3 files changed, 28 insertions(+), 34 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >>> index f969ea2..cfc16dd 100644 >>> --- a/drivers/usb/dwc3/Kconfig >>> +++ b/drivers/usb/dwc3/Kconfig >>> @@ -2,6 +2,7 @@ config USB_DWC3 >>> tristate "DesignWare USB3 DRD Core Support" >>> depends on (USB || USB_GADGET) && GENERIC_HARDIRQS && HAS_DMA >>> depends on EXTCON >>> + select USB_PHY >>> select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD >>> help >>> Say Y or M here if your system has a Dual Role SuperSpeed >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>> index 474162e..428c29e 100644 >>> --- a/drivers/usb/dwc3/core.c >>> +++ b/drivers/usb/dwc3/core.c >>> @@ -387,16 +387,38 @@ static int dwc3_probe(struct platform_device *pdev) >>> if (node) { >>> dwc->maximum_speed = of_usb_get_maximum_speed(node); >>> >>> - dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); >>> - dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1); >>> + if (of_property_read_bool(node, "usb-phy")) { >>> + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, >>> + "usb-phy", 0); >>> + if (IS_ERR(dwc->usb2_phy)) >>> + return PTR_ERR(dwc->usb2_phy); >>> + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, >>> + "usb-phy", 1); >>> + if (IS_ERR(dwc->usb3_phy)) >>> + return PTR_ERR(dwc->usb3_phy); >> >> Some DWC3 instances use only usb2_phy. e.g. on DRA7 the 2nd dwc3 instance doesn't use usb3_phy. >> This needs to be a valid case and driver shouldn't error out. > > So, i think adding flexibility to DWC3 to have either > usb2-phy/usb3-phy or both of them seems to be valid point. > Any suggestions ? > For high speed operation we need only usb2_phy but for super speed we need both usb2_phy and usb3_phy. Why would a dwc3 controller use only usb3_phy? A USB3.0 interface has to be compatible with USB2.0 as well, no? cheers, -roger
Hi, On Thu, Sep 12, 2013 at 4:34 PM, Roger Quadros <rogerq@ti.com> wrote: > Hi, > > On 09/12/2013 01:47 PM, Vivek Gautam wrote: >> On Thu, Sep 12, 2013 at 4:06 PM, Roger Quadros <rogerq@ti.com> wrote: >>> Hi Kishon, >>> >>> On 09/02/2013 06:43 PM, Kishon Vijay Abraham I wrote: >>>> There can be systems which does not have a external usb_phy, so get >>>> usb_phy only if usb-phy property is added in the case of dt boot or if >>>> platform_data indicates the presence of PHY. Also remove checking if >>>> return value is -ENXIO since it's now changed to always enable usb_phy layer. >>>> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>> --- >>>> drivers/usb/dwc3/Kconfig | 1 + >>>> drivers/usb/dwc3/core.c | 60 +++++++++++++++++--------------------- >>>> drivers/usb/dwc3/platform_data.h | 1 + >>>> 3 files changed, 28 insertions(+), 34 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >>>> index f969ea2..cfc16dd 100644 >>>> --- a/drivers/usb/dwc3/Kconfig >>>> +++ b/drivers/usb/dwc3/Kconfig >>>> @@ -2,6 +2,7 @@ config USB_DWC3 >>>> tristate "DesignWare USB3 DRD Core Support" >>>> depends on (USB || USB_GADGET) && GENERIC_HARDIRQS && HAS_DMA >>>> depends on EXTCON >>>> + select USB_PHY >>>> select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD >>>> help >>>> Say Y or M here if your system has a Dual Role SuperSpeed >>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>> index 474162e..428c29e 100644 >>>> --- a/drivers/usb/dwc3/core.c >>>> +++ b/drivers/usb/dwc3/core.c >>>> @@ -387,16 +387,38 @@ static int dwc3_probe(struct platform_device *pdev) >>>> if (node) { >>>> dwc->maximum_speed = of_usb_get_maximum_speed(node); >>>> >>>> - dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); >>>> - dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1); >>>> + if (of_property_read_bool(node, "usb-phy")) { >>>> + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, >>>> + "usb-phy", 0); >>>> + if (IS_ERR(dwc->usb2_phy)) >>>> + return PTR_ERR(dwc->usb2_phy); >>>> + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, >>>> + "usb-phy", 1); >>>> + if (IS_ERR(dwc->usb3_phy)) >>>> + return PTR_ERR(dwc->usb3_phy); >>> >>> Some DWC3 instances use only usb2_phy. e.g. on DRA7 the 2nd dwc3 instance doesn't use usb3_phy. >>> This needs to be a valid case and driver shouldn't error out. >> >> So, i think adding flexibility to DWC3 to have either >> usb2-phy/usb3-phy or both of them seems to be valid point. >> Any suggestions ? >> > > For high speed operation we need only usb2_phy but for super speed we need both usb2_phy > and usb3_phy. > > Why would a dwc3 controller use only usb3_phy? A USB3.0 interface has to be compatible with > USB2.0 as well, no? True and for that reason we need both UTMI+ interface as well as PIPE3 interface, right ? But as also mentioned in the thread: https://lkml.org/lkml/2013/9/12/181, on Samsung exynos5 architecturally same block is managing UTMI+ and PIPE3 interfaces, which is handled by phy-samsung-usb3 driver and denoted by "usb3_phy: usbphy@12100000" node in "arch/arm/boot/dts/exynos5250.dtsi". So on exynos5250, DWC3 really needs just usb3-phy, which is compatible for USB 2.0 as well. > > cheers, > -roger >
On 09/12/2013 02:26 PM, Vivek Gautam wrote: > Hi, > > > On Thu, Sep 12, 2013 at 4:34 PM, Roger Quadros <rogerq@ti.com> wrote: >> Hi, >> >> On 09/12/2013 01:47 PM, Vivek Gautam wrote: >>> On Thu, Sep 12, 2013 at 4:06 PM, Roger Quadros <rogerq@ti.com> wrote: >>>> Hi Kishon, >>>> >>>> On 09/02/2013 06:43 PM, Kishon Vijay Abraham I wrote: >>>>> There can be systems which does not have a external usb_phy, so get >>>>> usb_phy only if usb-phy property is added in the case of dt boot or if >>>>> platform_data indicates the presence of PHY. Also remove checking if >>>>> return value is -ENXIO since it's now changed to always enable usb_phy layer. >>>>> >>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>>> --- >>>>> drivers/usb/dwc3/Kconfig | 1 + >>>>> drivers/usb/dwc3/core.c | 60 +++++++++++++++++--------------------- >>>>> drivers/usb/dwc3/platform_data.h | 1 + >>>>> 3 files changed, 28 insertions(+), 34 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >>>>> index f969ea2..cfc16dd 100644 >>>>> --- a/drivers/usb/dwc3/Kconfig >>>>> +++ b/drivers/usb/dwc3/Kconfig >>>>> @@ -2,6 +2,7 @@ config USB_DWC3 >>>>> tristate "DesignWare USB3 DRD Core Support" >>>>> depends on (USB || USB_GADGET) && GENERIC_HARDIRQS && HAS_DMA >>>>> depends on EXTCON >>>>> + select USB_PHY >>>>> select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD >>>>> help >>>>> Say Y or M here if your system has a Dual Role SuperSpeed >>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>> index 474162e..428c29e 100644 >>>>> --- a/drivers/usb/dwc3/core.c >>>>> +++ b/drivers/usb/dwc3/core.c >>>>> @@ -387,16 +387,38 @@ static int dwc3_probe(struct platform_device *pdev) >>>>> if (node) { >>>>> dwc->maximum_speed = of_usb_get_maximum_speed(node); >>>>> >>>>> - dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); >>>>> - dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1); >>>>> + if (of_property_read_bool(node, "usb-phy")) { >>>>> + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, >>>>> + "usb-phy", 0); >>>>> + if (IS_ERR(dwc->usb2_phy)) >>>>> + return PTR_ERR(dwc->usb2_phy); >>>>> + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, >>>>> + "usb-phy", 1); >>>>> + if (IS_ERR(dwc->usb3_phy)) >>>>> + return PTR_ERR(dwc->usb3_phy); >>>> >>>> Some DWC3 instances use only usb2_phy. e.g. on DRA7 the 2nd dwc3 instance doesn't use usb3_phy. >>>> This needs to be a valid case and driver shouldn't error out. >>> >>> So, i think adding flexibility to DWC3 to have either >>> usb2-phy/usb3-phy or both of them seems to be valid point. >>> Any suggestions ? >>> >> >> For high speed operation we need only usb2_phy but for super speed we need both usb2_phy >> and usb3_phy. >> >> Why would a dwc3 controller use only usb3_phy? A USB3.0 interface has to be compatible with >> USB2.0 as well, no? > > True and for that reason we need both UTMI+ interface as well as PIPE3 > interface, right ? > But as also mentioned in the thread: > https://lkml.org/lkml/2013/9/12/181, on Samsung exynos5 > architecturally same block is managing UTMI+ and PIPE3 interfaces, > which is handled by phy-samsung-usb3 driver and denoted by "usb3_phy: > usbphy@12100000" node in "arch/arm/boot/dts/exynos5250.dtsi". > > So on exynos5250, DWC3 really needs just usb3-phy, which is compatible > for USB 2.0 as well. I'm not familiar with exynos5250 dwc3, but looking at the dts file I can see both usb3_phy and usb2_phy nodes and both are referenced in the dwc3 node. The ehci and ohci controllers don't reference any PHY. cheers, -roger
Hi, On Thu, Sep 12, 2013 at 6:41 PM, Roger Quadros <rogerq@ti.com> wrote: > On 09/12/2013 02:26 PM, Vivek Gautam wrote: >> Hi, >> >> >> On Thu, Sep 12, 2013 at 4:34 PM, Roger Quadros <rogerq@ti.com> wrote: >>> Hi, >>> >>> On 09/12/2013 01:47 PM, Vivek Gautam wrote: >>>> On Thu, Sep 12, 2013 at 4:06 PM, Roger Quadros <rogerq@ti.com> wrote: >>>>> Hi Kishon, >>>>> >>>>> On 09/02/2013 06:43 PM, Kishon Vijay Abraham I wrote: >>>>>> There can be systems which does not have a external usb_phy, so get >>>>>> usb_phy only if usb-phy property is added in the case of dt boot or if >>>>>> platform_data indicates the presence of PHY. Also remove checking if >>>>>> return value is -ENXIO since it's now changed to always enable usb_phy layer. >>>>>> >>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>>>> --- >>>>>> drivers/usb/dwc3/Kconfig | 1 + >>>>>> drivers/usb/dwc3/core.c | 60 +++++++++++++++++--------------------- >>>>>> drivers/usb/dwc3/platform_data.h | 1 + >>>>>> 3 files changed, 28 insertions(+), 34 deletions(-) >>>>>> >>>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >>>>>> index f969ea2..cfc16dd 100644 >>>>>> --- a/drivers/usb/dwc3/Kconfig >>>>>> +++ b/drivers/usb/dwc3/Kconfig >>>>>> @@ -2,6 +2,7 @@ config USB_DWC3 >>>>>> tristate "DesignWare USB3 DRD Core Support" >>>>>> depends on (USB || USB_GADGET) && GENERIC_HARDIRQS && HAS_DMA >>>>>> depends on EXTCON >>>>>> + select USB_PHY >>>>>> select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD >>>>>> help >>>>>> Say Y or M here if your system has a Dual Role SuperSpeed >>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>>> index 474162e..428c29e 100644 >>>>>> --- a/drivers/usb/dwc3/core.c >>>>>> +++ b/drivers/usb/dwc3/core.c >>>>>> @@ -387,16 +387,38 @@ static int dwc3_probe(struct platform_device *pdev) >>>>>> if (node) { >>>>>> dwc->maximum_speed = of_usb_get_maximum_speed(node); >>>>>> >>>>>> - dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); >>>>>> - dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1); >>>>>> + if (of_property_read_bool(node, "usb-phy")) { >>>>>> + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, >>>>>> + "usb-phy", 0); >>>>>> + if (IS_ERR(dwc->usb2_phy)) >>>>>> + return PTR_ERR(dwc->usb2_phy); >>>>>> + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, >>>>>> + "usb-phy", 1); >>>>>> + if (IS_ERR(dwc->usb3_phy)) >>>>>> + return PTR_ERR(dwc->usb3_phy); >>>>> >>>>> Some DWC3 instances use only usb2_phy. e.g. on DRA7 the 2nd dwc3 instance doesn't use usb3_phy. >>>>> This needs to be a valid case and driver shouldn't error out. >>>> >>>> So, i think adding flexibility to DWC3 to have either >>>> usb2-phy/usb3-phy or both of them seems to be valid point. >>>> Any suggestions ? >>>> >>> >>> For high speed operation we need only usb2_phy but for super speed we need both usb2_phy >>> and usb3_phy. >>> >>> Why would a dwc3 controller use only usb3_phy? A USB3.0 interface has to be compatible with >>> USB2.0 as well, no? >> >> True and for that reason we need both UTMI+ interface as well as PIPE3 >> interface, right ? >> But as also mentioned in the thread: >> https://lkml.org/lkml/2013/9/12/181, on Samsung exynos5 >> architecturally same block is managing UTMI+ and PIPE3 interfaces, >> which is handled by phy-samsung-usb3 driver and denoted by "usb3_phy: >> usbphy@12100000" node in "arch/arm/boot/dts/exynos5250.dtsi". >> >> So on exynos5250, DWC3 really needs just usb3-phy, which is compatible >> for USB 2.0 as well. > > I'm not familiar with exynos5250 dwc3, but looking at the dts file I can see both usb3_phy > and usb2_phy nodes and both are referenced in the dwc3 node. Right, and that's what we intend to fix. Exynos5250 dwc3 is not in the need of both usb2_phy and usb3_phy. In fact usb3_phy handle (which is served by phy-samsung-usb3 driver) sets up the UTMI+ as well as PIPE3 phy on DWC3. I know this looks different from the usual OMAP style of phy for dwc3. Hi Sylwester, any suggestions here ? As contained in the exynos5250 user manual too, there's only one block of PHY handling both UTMI+ and PIPE3 interfaces, and the set of registers (same base address) also indicate one block of PHY. So we have the usb3_phy (phy-samsung-usb3 driver) which setup the entire PHY require for DWC3 operation, and thereby we __shouldn't__ need usb2_phy (phy-samsung-usb2 driver which actually setup the PHY for a separate USB 2.0 controller on exynos5250) for DWC3. > > The ehci and ohci controllers don't reference any PHY. ehci-s5p and ohci-exynos are not using devm_usb_get_phy_by_phandle() api until now, so they don't need to have the PHY handlers in their node. > > cheers, > -roger
Hi, On Thu, Sep 12, 2013 at 04:17:08PM +0530, Vivek Gautam wrote: > On Thu, Sep 12, 2013 at 4:06 PM, Roger Quadros <rogerq@ti.com> wrote: > > Hi Kishon, > > > > On 09/02/2013 06:43 PM, Kishon Vijay Abraham I wrote: > >> There can be systems which does not have a external usb_phy, so get > >> usb_phy only if usb-phy property is added in the case of dt boot or if > >> platform_data indicates the presence of PHY. Also remove checking if > >> return value is -ENXIO since it's now changed to always enable usb_phy layer. > >> > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > >> --- > >> drivers/usb/dwc3/Kconfig | 1 + > >> drivers/usb/dwc3/core.c | 60 +++++++++++++++++--------------------- > >> drivers/usb/dwc3/platform_data.h | 1 + > >> 3 files changed, 28 insertions(+), 34 deletions(-) > >> > >> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig > >> index f969ea2..cfc16dd 100644 > >> --- a/drivers/usb/dwc3/Kconfig > >> +++ b/drivers/usb/dwc3/Kconfig > >> @@ -2,6 +2,7 @@ config USB_DWC3 > >> tristate "DesignWare USB3 DRD Core Support" > >> depends on (USB || USB_GADGET) && GENERIC_HARDIRQS && HAS_DMA > >> depends on EXTCON > >> + select USB_PHY > >> select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD > >> help > >> Say Y or M here if your system has a Dual Role SuperSpeed > >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >> index 474162e..428c29e 100644 > >> --- a/drivers/usb/dwc3/core.c > >> +++ b/drivers/usb/dwc3/core.c > >> @@ -387,16 +387,38 @@ static int dwc3_probe(struct platform_device *pdev) > >> if (node) { > >> dwc->maximum_speed = of_usb_get_maximum_speed(node); > >> > >> - dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); > >> - dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1); > >> + if (of_property_read_bool(node, "usb-phy")) { > >> + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, > >> + "usb-phy", 0); > >> + if (IS_ERR(dwc->usb2_phy)) > >> + return PTR_ERR(dwc->usb2_phy); > >> + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, > >> + "usb-phy", 1); > >> + if (IS_ERR(dwc->usb3_phy)) > >> + return PTR_ERR(dwc->usb3_phy); > > > > Some DWC3 instances use only usb2_phy. e.g. on DRA7 the 2nd dwc3 instance doesn't use usb3_phy. > > This needs to be a valid case and driver shouldn't error out. > > So, i think adding flexibility to DWC3 to have either > usb2-phy/usb3-phy or both of them seems to be valid point. > Any suggestions ? I've gone through that. There's no easy way to make USB3 PHY optional. For platforms which actually have the USB3 side of things, not initializing the PHY kills dwc3. I've been trying to understand what it is that the USB3 PHY generates and gets backfed into dwc3, but so far no luck. I even have a patch for that, which I have dropped when I found out about this problem.
diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig index f969ea2..cfc16dd 100644 --- a/drivers/usb/dwc3/Kconfig +++ b/drivers/usb/dwc3/Kconfig @@ -2,6 +2,7 @@ config USB_DWC3 tristate "DesignWare USB3 DRD Core Support" depends on (USB || USB_GADGET) && GENERIC_HARDIRQS && HAS_DMA depends on EXTCON + select USB_PHY select USB_XHCI_PLATFORM if USB_SUPPORT && USB_XHCI_HCD help Say Y or M here if your system has a Dual Role SuperSpeed diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 474162e..428c29e 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -387,16 +387,38 @@ static int dwc3_probe(struct platform_device *pdev) if (node) { dwc->maximum_speed = of_usb_get_maximum_speed(node); - dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); - dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 1); + if (of_property_read_bool(node, "usb-phy")) { + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, + "usb-phy", 0); + if (IS_ERR(dwc->usb2_phy)) + return PTR_ERR(dwc->usb2_phy); + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, + "usb-phy", 1); + if (IS_ERR(dwc->usb3_phy)) + return PTR_ERR(dwc->usb3_phy); + } else { + dwc->usb2_phy = NULL; + dwc->usb3_phy = NULL; + } dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize"); dwc->dr_mode = of_usb_get_dr_mode(node); } else if (pdata) { dwc->maximum_speed = pdata->maximum_speed; - dwc->usb2_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2); - dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3); + if (pdata->has_phy) { + dwc->usb2_phy = devm_usb_get_phy(dev, + USB_PHY_TYPE_USB2); + if (IS_ERR(dwc->usb2_phy)) + return PTR_ERR(dwc->usb2_phy); + dwc->usb3_phy = devm_usb_get_phy(dev, + USB_PHY_TYPE_USB3); + if (IS_ERR(dwc->usb3_phy)) + return PTR_ERR(dwc->usb3_phy); + } else { + dwc->usb2_phy = NULL; + dwc->usb3_phy = NULL; + } dwc->needs_fifo_resize = pdata->tx_fifo_resize; dwc->dr_mode = pdata->dr_mode; @@ -409,36 +431,6 @@ static int dwc3_probe(struct platform_device *pdev) if (dwc->maximum_speed == USB_SPEED_UNKNOWN) dwc->maximum_speed = USB_SPEED_SUPER; - if (IS_ERR(dwc->usb2_phy)) { - ret = PTR_ERR(dwc->usb2_phy); - - /* - * if -ENXIO is returned, it means PHY layer wasn't - * enabled, so it makes no sense to return -EPROBE_DEFER - * in that case, since no PHY driver will ever probe. - */ - if (ret == -ENXIO) - return ret; - - dev_err(dev, "no usb2 phy configured\n"); - return -EPROBE_DEFER; - } - - if (IS_ERR(dwc->usb3_phy)) { - ret = PTR_ERR(dwc->usb3_phy); - - /* - * if -ENXIO is returned, it means PHY layer wasn't - * enabled, so it makes no sense to return -EPROBE_DEFER - * in that case, since no PHY driver will ever probe. - */ - if (ret == -ENXIO) - return ret; - - dev_err(dev, "no usb3 phy configured\n"); - return -EPROBE_DEFER; - } - dwc->xhci_resources[0].start = res->start; dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + DWC3_XHCI_REGS_END; diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h index 7db34f0..5a5e068 100644 --- a/drivers/usb/dwc3/platform_data.h +++ b/drivers/usb/dwc3/platform_data.h @@ -24,4 +24,5 @@ struct dwc3_platform_data { enum usb_device_speed maximum_speed; enum usb_dr_mode dr_mode; bool tx_fifo_resize; + bool has_phy; };
There can be systems which does not have a external usb_phy, so get usb_phy only if usb-phy property is added in the case of dt boot or if platform_data indicates the presence of PHY. Also remove checking if return value is -ENXIO since it's now changed to always enable usb_phy layer. Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> --- drivers/usb/dwc3/Kconfig | 1 + drivers/usb/dwc3/core.c | 60 +++++++++++++++++--------------------- drivers/usb/dwc3/platform_data.h | 1 + 3 files changed, 28 insertions(+), 34 deletions(-)