| Message ID | 1381866857-3861-3-git-send-email-kishon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote: > Adapted dwc3 core to use the Generic PHY Framework. So for init, exit, > power_on and power_off the following APIs are used phy_init(), phy_exit(), > phy_power_on() and phy_power_off(). > > However using the old USB phy library wont be removed till the PHYs of all > other SoC's using dwc3 core is adapted to the Generic PHY Framework. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > Documentation/devicetree/bindings/usb/dwc3.txt | 6 ++- > drivers/usb/dwc3/Kconfig | 1 + > drivers/usb/dwc3/core.c | 52 ++++++++++++++++++++++++ > drivers/usb/dwc3/core.h | 7 ++++ > drivers/usb/dwc3/platform_data.h | 2 + > 5 files changed, 66 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt > index e807635..471366d 100644 > --- a/Documentation/devicetree/bindings/usb/dwc3.txt > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt > @@ -6,11 +6,13 @@ Required properties: > - compatible: must be "snps,dwc3" > - reg : Address and length of the register set for the device > - interrupts: Interrupts used by the dwc3 controller. > + > +Optional properties: > - usb-phy : array of phandle for the PHY device. The first element > in the array is expected to be a handle to the USB2/HS PHY and > the second element is expected to be a handle to the USB3/SS PHY > - > -Optional properties: > + - phys: from the *Generic PHY* bindings > + - phy-names: from the *Generic PHY* bindings > - tx-fifo-resize: determines if the FIFO *has* to be reallocated. > > This is usually a subnode to DWC3 glue to which it is connected. > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig > index 8e385b4..64eed6f 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) && HAS_DMA > select USB_PHY > + select GENERIC_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 cb91d70..28bfa5b 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -82,6 +82,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc) > > usb_phy_init(dwc->usb2_phy); > usb_phy_init(dwc->usb3_phy); > + > + if (dwc->usb2_generic_phy) > + phy_init(dwc->usb2_generic_phy); > + if (dwc->usb3_generic_phy) > + phy_init(dwc->usb3_generic_phy); > + dwc3_core_soft_reset() is called from dwc3_core_init() which is called from dwc3_probe() but before the PHYs are initialized. This will cause phy power_on() to be called before phy_init() which is wrong. > mdelay(100); > > /* Clear USB3 PHY reset */ > @@ -343,6 +349,11 @@ static void dwc3_core_exit(struct dwc3 *dwc) > { > usb_phy_shutdown(dwc->usb2_phy); > usb_phy_shutdown(dwc->usb3_phy); > + > + if (dwc->usb2_generic_phy) > + phy_power_off(dwc->usb2_generic_phy); > + if (dwc->usb3_generic_phy) > + phy_power_off(dwc->usb3_generic_phy); > } > > #define DWC3_ALIGN_MASK (16 - 1) > @@ -439,6 +450,26 @@ static int dwc3_probe(struct platform_device *pdev) > dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3); > } > > + count = of_property_match_string(node, "phy-names", "usb2-phy"); > + if (count >= 0 || (pdata && pdata->usb2_generic_phy)) { > + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy"); > + if (IS_ERR(dwc->usb2_generic_phy)) { > + dev_err(dev, "no usb2 phy configured yet"); > + return PTR_ERR(dwc->usb2_generic_phy); > + } > + dwc->usb2_phy = NULL; > + } > + > + count = of_property_match_string(node, "phy-names", "usb3-phy"); > + if (count >= 0 || (pdata && pdata->usb3_generic_phy)) { > + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); > + if (IS_ERR(dwc->usb3_generic_phy)) { > + dev_err(dev, "no usb3 phy configured yet"); > + return PTR_ERR(dwc->usb3_generic_phy); > + } > + dwc->usb3_phy = NULL; > + } > + There are a couple of issues here. - The above code must be executed only if it node is valid. - We must either get both PHYs via old method or via new method and not support mix matching them. e.g. it is possible with this code that usb2_phy is set and usb3_generic_phy is set. If you give the new method preference then you don't even need to attempt a devm_usb_get_phy() if the generic phy is present in the node. > /* default to superspeed if no maximum_speed passed */ > if (dwc->maximum_speed == USB_SPEED_UNKNOWN) > dwc->maximum_speed = USB_SPEED_SUPER; > @@ -462,6 +493,11 @@ static int dwc3_probe(struct platform_device *pdev) > usb_phy_set_suspend(dwc->usb2_phy, 0); > usb_phy_set_suspend(dwc->usb3_phy, 0); > > + if (dwc->usb2_generic_phy) > + phy_power_on(dwc->usb2_generic_phy); > + if (dwc->usb3_generic_phy) > + phy_power_on(dwc->usb3_generic_phy); > + > spin_lock_init(&dwc->lock); > platform_set_drvdata(pdev, dwc); > > @@ -588,6 +624,11 @@ static int dwc3_remove(struct platform_device *pdev) > usb_phy_set_suspend(dwc->usb2_phy, 1); > usb_phy_set_suspend(dwc->usb3_phy, 1); > > + if (dwc->usb2_generic_phy) > + phy_power_off(dwc->usb2_generic_phy); > + if (dwc->usb3_generic_phy) > + phy_power_off(dwc->usb3_generic_phy); > + > pm_runtime_put(&pdev->dev); > pm_runtime_disable(&pdev->dev); > > @@ -685,6 +726,11 @@ static int dwc3_suspend(struct device *dev) > usb_phy_shutdown(dwc->usb3_phy); > usb_phy_shutdown(dwc->usb2_phy); > > + if (dwc->usb2_generic_phy) > + phy_exit(dwc->usb2_generic_phy); > + if (dwc->usb3_generic_phy) > + phy_exit(dwc->usb3_generic_phy); > + > return 0; > } > > @@ -695,6 +741,12 @@ static int dwc3_resume(struct device *dev) > > usb_phy_init(dwc->usb3_phy); > usb_phy_init(dwc->usb2_phy); > + > + if (dwc->usb2_generic_phy) > + phy_init(dwc->usb2_generic_phy); > + if (dwc->usb3_generic_phy) > + phy_init(dwc->usb3_generic_phy); > + > msleep(100); > > spin_lock_irqsave(&dwc->lock, flags); > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index f8af8d4..01ec7d7 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -31,6 +31,8 @@ > #include <linux/usb/gadget.h> > #include <linux/usb/otg.h> > > +#include <linux/phy/phy.h> > + > /* Global constants */ > #define DWC3_EP0_BOUNCE_SIZE 512 > #define DWC3_ENDPOINTS_NUM 32 > @@ -613,6 +615,8 @@ struct dwc3_scratchpad_array { > * @dr_mode: requested mode of operation > * @usb2_phy: pointer to USB2 PHY > * @usb3_phy: pointer to USB3 PHY > + * @usb2_generic_phy: pointer to USB2 PHY > + * @usb3_generic_phy: pointer to USB3 PHY > * @dcfg: saved contents of DCFG register > * @gctl: saved contents of GCTL register > * @is_selfpowered: true when we are selfpowered > @@ -665,6 +669,9 @@ struct dwc3 { > struct usb_phy *usb2_phy; > struct usb_phy *usb3_phy; > > + struct phy *usb2_generic_phy; > + struct phy *usb3_generic_phy; > + > void __iomem *regs; > size_t regs_size; > > diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h > index 49ffa6d..fc62a0f 100644 > --- a/drivers/usb/dwc3/platform_data.h > +++ b/drivers/usb/dwc3/platform_data.h > @@ -26,4 +26,6 @@ struct dwc3_platform_data { > bool tx_fifo_resize; > bool usb2_phy; > bool usb3_phy; > + bool usb2_generic_phy; > + bool usb3_generic_phy; > }; > cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wednesday 16 October 2013 06:48 PM, Roger Quadros wrote: > Hi, > > On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote: >> Adapted dwc3 core to use the Generic PHY Framework. So for init, exit, >> power_on and power_off the following APIs are used phy_init(), phy_exit(), >> phy_power_on() and phy_power_off(). >> >> However using the old USB phy library wont be removed till the PHYs of all >> other SoC's using dwc3 core is adapted to the Generic PHY Framework. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> Documentation/devicetree/bindings/usb/dwc3.txt | 6 ++- >> drivers/usb/dwc3/Kconfig | 1 + >> drivers/usb/dwc3/core.c | 52 ++++++++++++++++++++++++ >> drivers/usb/dwc3/core.h | 7 ++++ >> drivers/usb/dwc3/platform_data.h | 2 + >> 5 files changed, 66 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt >> index e807635..471366d 100644 >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >> @@ -6,11 +6,13 @@ Required properties: >> - compatible: must be "snps,dwc3" >> - reg : Address and length of the register set for the device >> - interrupts: Interrupts used by the dwc3 controller. >> + >> +Optional properties: >> - usb-phy : array of phandle for the PHY device. The first element >> in the array is expected to be a handle to the USB2/HS PHY and >> the second element is expected to be a handle to the USB3/SS PHY >> - >> -Optional properties: >> + - phys: from the *Generic PHY* bindings >> + - phy-names: from the *Generic PHY* bindings >> - tx-fifo-resize: determines if the FIFO *has* to be reallocated. >> >> This is usually a subnode to DWC3 glue to which it is connected. >> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >> index 8e385b4..64eed6f 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) && HAS_DMA >> select USB_PHY >> + select GENERIC_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 cb91d70..28bfa5b 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -82,6 +82,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc) >> >> usb_phy_init(dwc->usb2_phy); >> usb_phy_init(dwc->usb3_phy); >> + >> + if (dwc->usb2_generic_phy) >> + phy_init(dwc->usb2_generic_phy); >> + if (dwc->usb3_generic_phy) >> + phy_init(dwc->usb3_generic_phy); >> + > > dwc3_core_soft_reset() is called from dwc3_core_init() which is called from > dwc3_probe() but before the PHYs are initialized. > > This will cause phy power_on() to be called before phy_init() which is wrong. > >> mdelay(100); >> >> /* Clear USB3 PHY reset */ >> @@ -343,6 +349,11 @@ static void dwc3_core_exit(struct dwc3 *dwc) >> { >> usb_phy_shutdown(dwc->usb2_phy); >> usb_phy_shutdown(dwc->usb3_phy); >> + >> + if (dwc->usb2_generic_phy) >> + phy_power_off(dwc->usb2_generic_phy); >> + if (dwc->usb3_generic_phy) >> + phy_power_off(dwc->usb3_generic_phy); >> } >> >> #define DWC3_ALIGN_MASK (16 - 1) >> @@ -439,6 +450,26 @@ static int dwc3_probe(struct platform_device *pdev) >> dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3); >> } >> >> + count = of_property_match_string(node, "phy-names", "usb2-phy"); >> + if (count >= 0 || (pdata && pdata->usb2_generic_phy)) { >> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy"); >> + if (IS_ERR(dwc->usb2_generic_phy)) { >> + dev_err(dev, "no usb2 phy configured yet"); >> + return PTR_ERR(dwc->usb2_generic_phy); >> + } >> + dwc->usb2_phy = NULL; >> + } >> + >> + count = of_property_match_string(node, "phy-names", "usb3-phy"); >> + if (count >= 0 || (pdata && pdata->usb3_generic_phy)) { >> + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); >> + if (IS_ERR(dwc->usb3_generic_phy)) { >> + dev_err(dev, "no usb3 phy configured yet"); >> + return PTR_ERR(dwc->usb3_generic_phy); >> + } >> + dwc->usb3_phy = NULL; >> + } >> + > > There are a couple of issues here. > - The above code must be executed only if it node is valid. of_property_match_string will give a error value if the node is not present. *count >= 0* can take care of this. > - We must either get both PHYs via old method or via new method and not support mix matching > them. e.g. it is possible with this code that usb2_phy is set and usb3_generic_phy is set. Why? As long as both the frameworks co-exist (and we support both in dwc3), I don't see any reason why we shouldn't allow it. We can avoid adding a few more checks by leaving it the way it is currently. Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/16/2013 04:52 PM, Kishon Vijay Abraham I wrote: > Hi, > > On Wednesday 16 October 2013 06:48 PM, Roger Quadros wrote: >> Hi, >> >> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote: >>> Adapted dwc3 core to use the Generic PHY Framework. So for init, exit, >>> power_on and power_off the following APIs are used phy_init(), phy_exit(), >>> phy_power_on() and phy_power_off(). >>> >>> However using the old USB phy library wont be removed till the PHYs of all >>> other SoC's using dwc3 core is adapted to the Generic PHY Framework. >>> >>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>> --- >>> Documentation/devicetree/bindings/usb/dwc3.txt | 6 ++- >>> drivers/usb/dwc3/Kconfig | 1 + >>> drivers/usb/dwc3/core.c | 52 ++++++++++++++++++++++++ >>> drivers/usb/dwc3/core.h | 7 ++++ >>> drivers/usb/dwc3/platform_data.h | 2 + >>> 5 files changed, 66 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt >>> index e807635..471366d 100644 >>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >>> @@ -6,11 +6,13 @@ Required properties: >>> - compatible: must be "snps,dwc3" >>> - reg : Address and length of the register set for the device >>> - interrupts: Interrupts used by the dwc3 controller. >>> + >>> +Optional properties: >>> - usb-phy : array of phandle for the PHY device. The first element >>> in the array is expected to be a handle to the USB2/HS PHY and >>> the second element is expected to be a handle to the USB3/SS PHY >>> - >>> -Optional properties: >>> + - phys: from the *Generic PHY* bindings >>> + - phy-names: from the *Generic PHY* bindings >>> - tx-fifo-resize: determines if the FIFO *has* to be reallocated. >>> >>> This is usually a subnode to DWC3 glue to which it is connected. >>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >>> index 8e385b4..64eed6f 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) && HAS_DMA >>> select USB_PHY >>> + select GENERIC_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 cb91d70..28bfa5b 100644 >>> --- a/drivers/usb/dwc3/core.c >>> +++ b/drivers/usb/dwc3/core.c >>> @@ -82,6 +82,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc) >>> >>> usb_phy_init(dwc->usb2_phy); >>> usb_phy_init(dwc->usb3_phy); >>> + >>> + if (dwc->usb2_generic_phy) >>> + phy_init(dwc->usb2_generic_phy); >>> + if (dwc->usb3_generic_phy) >>> + phy_init(dwc->usb3_generic_phy); >>> + >> >> dwc3_core_soft_reset() is called from dwc3_core_init() which is called from >> dwc3_probe() but before the PHYs are initialized. >> >> This will cause phy power_on() to be called before phy_init() which is wrong. >> You overlooked the above? >>> mdelay(100); >>> >>> /* Clear USB3 PHY reset */ >>> @@ -343,6 +349,11 @@ static void dwc3_core_exit(struct dwc3 *dwc) >>> { >>> usb_phy_shutdown(dwc->usb2_phy); >>> usb_phy_shutdown(dwc->usb3_phy); >>> + >>> + if (dwc->usb2_generic_phy) >>> + phy_power_off(dwc->usb2_generic_phy); >>> + if (dwc->usb3_generic_phy) >>> + phy_power_off(dwc->usb3_generic_phy); >>> } >>> >>> #define DWC3_ALIGN_MASK (16 - 1) >>> @@ -439,6 +450,26 @@ static int dwc3_probe(struct platform_device *pdev) >>> dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3); >>> } >>> >>> + count = of_property_match_string(node, "phy-names", "usb2-phy"); >>> + if (count >= 0 || (pdata && pdata->usb2_generic_phy)) { >>> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy"); >>> + if (IS_ERR(dwc->usb2_generic_phy)) { >>> + dev_err(dev, "no usb2 phy configured yet"); >>> + return PTR_ERR(dwc->usb2_generic_phy); >>> + } >>> + dwc->usb2_phy = NULL; >>> + } >>> + >>> + count = of_property_match_string(node, "phy-names", "usb3-phy"); >>> + if (count >= 0 || (pdata && pdata->usb3_generic_phy)) { >>> + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); >>> + if (IS_ERR(dwc->usb3_generic_phy)) { >>> + dev_err(dev, "no usb3 phy configured yet"); >>> + return PTR_ERR(dwc->usb3_generic_phy); >>> + } >>> + dwc->usb3_phy = NULL; >>> + } >>> + >> >> There are a couple of issues here. >> - The above code must be executed only if it node is valid. > > of_property_match_string will give a error value if the node is not present. > *count >= 0* can take care of this. OK. >> - We must either get both PHYs via old method or via new method and not support mix matching >> them. e.g. it is possible with this code that usb2_phy is set and usb3_generic_phy is set. > > Why? As long as both the frameworks co-exist (and we support both in dwc3), I > don't see any reason why we shouldn't allow it. We can avoid adding a few more > checks by leaving it the way it is currently. Because it just doesn't seem elegant. Why would you want to even allow both types of PHY to be used simultaneously? cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wednesday 16 October 2013 07:53 PM, Roger Quadros wrote: > On 10/16/2013 04:52 PM, Kishon Vijay Abraham I wrote: >> Hi, >> >> On Wednesday 16 October 2013 06:48 PM, Roger Quadros wrote: >>> Hi, >>> >>> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote: >>>> Adapted dwc3 core to use the Generic PHY Framework. So for init, exit, >>>> power_on and power_off the following APIs are used phy_init(), phy_exit(), >>>> phy_power_on() and phy_power_off(). >>>> >>>> However using the old USB phy library wont be removed till the PHYs of all >>>> other SoC's using dwc3 core is adapted to the Generic PHY Framework. >>>> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>> --- >>>> Documentation/devicetree/bindings/usb/dwc3.txt | 6 ++- >>>> drivers/usb/dwc3/Kconfig | 1 + >>>> drivers/usb/dwc3/core.c | 52 ++++++++++++++++++++++++ >>>> drivers/usb/dwc3/core.h | 7 ++++ >>>> drivers/usb/dwc3/platform_data.h | 2 + >>>> 5 files changed, 66 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt >>>> index e807635..471366d 100644 >>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >>>> @@ -6,11 +6,13 @@ Required properties: >>>> - compatible: must be "snps,dwc3" >>>> - reg : Address and length of the register set for the device >>>> - interrupts: Interrupts used by the dwc3 controller. >>>> + >>>> +Optional properties: >>>> - usb-phy : array of phandle for the PHY device. The first element >>>> in the array is expected to be a handle to the USB2/HS PHY and >>>> the second element is expected to be a handle to the USB3/SS PHY >>>> - >>>> -Optional properties: >>>> + - phys: from the *Generic PHY* bindings >>>> + - phy-names: from the *Generic PHY* bindings >>>> - tx-fifo-resize: determines if the FIFO *has* to be reallocated. >>>> >>>> This is usually a subnode to DWC3 glue to which it is connected. >>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >>>> index 8e385b4..64eed6f 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) && HAS_DMA >>>> select USB_PHY >>>> + select GENERIC_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 cb91d70..28bfa5b 100644 >>>> --- a/drivers/usb/dwc3/core.c >>>> +++ b/drivers/usb/dwc3/core.c >>>> @@ -82,6 +82,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc) >>>> >>>> usb_phy_init(dwc->usb2_phy); >>>> usb_phy_init(dwc->usb3_phy); >>>> + >>>> + if (dwc->usb2_generic_phy) >>>> + phy_init(dwc->usb2_generic_phy); >>>> + if (dwc->usb3_generic_phy) >>>> + phy_init(dwc->usb3_generic_phy); >>>> + >>> >>> dwc3_core_soft_reset() is called from dwc3_core_init() which is called from >>> dwc3_probe() but before the PHYs are initialized. >>> >>> This will cause phy power_on() to be called before phy_init() which is wrong. >>> > > You overlooked the above? No. Forgot to comment back. Yeah, you are right.. that should be fixed. > >>>> mdelay(100); >>>> >>>> /* Clear USB3 PHY reset */ >>>> @@ -343,6 +349,11 @@ static void dwc3_core_exit(struct dwc3 *dwc) >>>> { >>>> usb_phy_shutdown(dwc->usb2_phy); >>>> usb_phy_shutdown(dwc->usb3_phy); >>>> + >>>> + if (dwc->usb2_generic_phy) >>>> + phy_power_off(dwc->usb2_generic_phy); >>>> + if (dwc->usb3_generic_phy) >>>> + phy_power_off(dwc->usb3_generic_phy); >>>> } >>>> >>>> #define DWC3_ALIGN_MASK (16 - 1) >>>> @@ -439,6 +450,26 @@ static int dwc3_probe(struct platform_device *pdev) >>>> dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3); >>>> } >>>> >>>> + count = of_property_match_string(node, "phy-names", "usb2-phy"); >>>> + if (count >= 0 || (pdata && pdata->usb2_generic_phy)) { >>>> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy"); >>>> + if (IS_ERR(dwc->usb2_generic_phy)) { >>>> + dev_err(dev, "no usb2 phy configured yet"); >>>> + return PTR_ERR(dwc->usb2_generic_phy); >>>> + } >>>> + dwc->usb2_phy = NULL; >>>> + } >>>> + >>>> + count = of_property_match_string(node, "phy-names", "usb3-phy"); >>>> + if (count >= 0 || (pdata && pdata->usb3_generic_phy)) { >>>> + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); >>>> + if (IS_ERR(dwc->usb3_generic_phy)) { >>>> + dev_err(dev, "no usb3 phy configured yet"); >>>> + return PTR_ERR(dwc->usb3_generic_phy); >>>> + } >>>> + dwc->usb3_phy = NULL; >>>> + } >>>> + >>> >>> There are a couple of issues here. >>> - The above code must be executed only if it node is valid. >> >> of_property_match_string will give a error value if the node is not present. >> *count >= 0* can take care of this. > > OK. > >>> - We must either get both PHYs via old method or via new method and not support mix matching >>> them. e.g. it is possible with this code that usb2_phy is set and usb3_generic_phy is set. >> >> Why? As long as both the frameworks co-exist (and we support both in dwc3), I >> don't see any reason why we shouldn't allow it. We can avoid adding a few more >> checks by leaving it the way it is currently. > > Because it just doesn't seem elegant. Why would you want to even allow both types of PHY > to be used simultaneously? We actually don't allow both types of PHYs to be used simultaneously. We set usb2_phy/usb3_phy to NULL, if we had got the the generic PHY. If you just doesn't even want to get PHY by the second method if we have got PHY by the first method, then we might need to add more *if* checks which IMO don't make it any elegant either. Having a clean dt data wont have both types of PHYs anyway. Thanks Kishon > > cheers, > -roger > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/21/2013 02:55 PM, Kishon Vijay Abraham I wrote: > Hi, > > On Wednesday 16 October 2013 07:53 PM, Roger Quadros wrote: >> On 10/16/2013 04:52 PM, Kishon Vijay Abraham I wrote: >>> Hi, >>> >>> On Wednesday 16 October 2013 06:48 PM, Roger Quadros wrote: >>>> Hi, >>>> >>>> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote: >>>>> Adapted dwc3 core to use the Generic PHY Framework. So for init, exit, >>>>> power_on and power_off the following APIs are used phy_init(), phy_exit(), >>>>> phy_power_on() and phy_power_off(). >>>>> >>>>> However using the old USB phy library wont be removed till the PHYs of all >>>>> other SoC's using dwc3 core is adapted to the Generic PHY Framework. >>>>> >>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>>> --- >>>>> Documentation/devicetree/bindings/usb/dwc3.txt | 6 ++- >>>>> drivers/usb/dwc3/Kconfig | 1 + >>>>> drivers/usb/dwc3/core.c | 52 ++++++++++++++++++++++++ >>>>> drivers/usb/dwc3/core.h | 7 ++++ >>>>> drivers/usb/dwc3/platform_data.h | 2 + >>>>> 5 files changed, 66 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt >>>>> index e807635..471366d 100644 >>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >>>>> @@ -6,11 +6,13 @@ Required properties: >>>>> - compatible: must be "snps,dwc3" >>>>> - reg : Address and length of the register set for the device >>>>> - interrupts: Interrupts used by the dwc3 controller. >>>>> + >>>>> +Optional properties: >>>>> - usb-phy : array of phandle for the PHY device. The first element >>>>> in the array is expected to be a handle to the USB2/HS PHY and >>>>> the second element is expected to be a handle to the USB3/SS PHY >>>>> - >>>>> -Optional properties: >>>>> + - phys: from the *Generic PHY* bindings >>>>> + - phy-names: from the *Generic PHY* bindings >>>>> - tx-fifo-resize: determines if the FIFO *has* to be reallocated. >>>>> >>>>> This is usually a subnode to DWC3 glue to which it is connected. >>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >>>>> index 8e385b4..64eed6f 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) && HAS_DMA >>>>> select USB_PHY >>>>> + select GENERIC_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 cb91d70..28bfa5b 100644 >>>>> --- a/drivers/usb/dwc3/core.c >>>>> +++ b/drivers/usb/dwc3/core.c >>>>> @@ -82,6 +82,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc) >>>>> >>>>> usb_phy_init(dwc->usb2_phy); >>>>> usb_phy_init(dwc->usb3_phy); >>>>> + >>>>> + if (dwc->usb2_generic_phy) >>>>> + phy_init(dwc->usb2_generic_phy); >>>>> + if (dwc->usb3_generic_phy) >>>>> + phy_init(dwc->usb3_generic_phy); >>>>> + >>>> >>>> dwc3_core_soft_reset() is called from dwc3_core_init() which is called from >>>> dwc3_probe() but before the PHYs are initialized. >>>> >>>> This will cause phy power_on() to be called before phy_init() which is wrong. >>>> >> >> You overlooked the above? > > No. Forgot to comment back. > Yeah, you are right.. that should be fixed. >> >>>>> mdelay(100); >>>>> >>>>> /* Clear USB3 PHY reset */ >>>>> @@ -343,6 +349,11 @@ static void dwc3_core_exit(struct dwc3 *dwc) >>>>> { >>>>> usb_phy_shutdown(dwc->usb2_phy); >>>>> usb_phy_shutdown(dwc->usb3_phy); >>>>> + >>>>> + if (dwc->usb2_generic_phy) >>>>> + phy_power_off(dwc->usb2_generic_phy); >>>>> + if (dwc->usb3_generic_phy) >>>>> + phy_power_off(dwc->usb3_generic_phy); >>>>> } >>>>> >>>>> #define DWC3_ALIGN_MASK (16 - 1) >>>>> @@ -439,6 +450,26 @@ static int dwc3_probe(struct platform_device *pdev) >>>>> dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3); >>>>> } >>>>> >>>>> + count = of_property_match_string(node, "phy-names", "usb2-phy"); >>>>> + if (count >= 0 || (pdata && pdata->usb2_generic_phy)) { >>>>> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy"); >>>>> + if (IS_ERR(dwc->usb2_generic_phy)) { >>>>> + dev_err(dev, "no usb2 phy configured yet"); >>>>> + return PTR_ERR(dwc->usb2_generic_phy); >>>>> + } >>>>> + dwc->usb2_phy = NULL; >>>>> + } >>>>> + >>>>> + count = of_property_match_string(node, "phy-names", "usb3-phy"); >>>>> + if (count >= 0 || (pdata && pdata->usb3_generic_phy)) { >>>>> + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); >>>>> + if (IS_ERR(dwc->usb3_generic_phy)) { >>>>> + dev_err(dev, "no usb3 phy configured yet"); >>>>> + return PTR_ERR(dwc->usb3_generic_phy); >>>>> + } >>>>> + dwc->usb3_phy = NULL; >>>>> + } >>>>> + >>>> >>>> There are a couple of issues here. >>>> - The above code must be executed only if it node is valid. >>> >>> of_property_match_string will give a error value if the node is not present. >>> *count >= 0* can take care of this. >> >> OK. >> >>>> - We must either get both PHYs via old method or via new method and not support mix matching >>>> them. e.g. it is possible with this code that usb2_phy is set and usb3_generic_phy is set. >>> >>> Why? As long as both the frameworks co-exist (and we support both in dwc3), I >>> don't see any reason why we shouldn't allow it. We can avoid adding a few more >>> checks by leaving it the way it is currently. >> >> Because it just doesn't seem elegant. Why would you want to even allow both types of PHY >> to be used simultaneously? > > We actually don't allow both types of PHYs to be used simultaneously. We set > usb2_phy/usb3_phy to NULL, if we had got the the generic PHY. > If you just doesn't even want to get PHY by the second method if we have got > PHY by the first method, then we might need to add more *if* checks which IMO > don't make it any elegant either. > Having a clean dt data wont have both types of PHYs anyway. OK. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Kishon, On Wed, Oct 16, 2013 at 01:24:12AM +0530, Kishon Vijay Abraham I wrote: > + count = of_property_match_string(node, "phy-names", "usb2-phy"); > + if (count >= 0 || (pdata && pdata->usb2_generic_phy)) { > + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy"); > + if (IS_ERR(dwc->usb2_generic_phy)) { > + dev_err(dev, "no usb2 phy configured yet"); > + return PTR_ERR(dwc->usb2_generic_phy); > + } > + dwc->usb2_phy = NULL; > + } > + > + count = of_property_match_string(node, "phy-names", "usb3-phy"); > + if (count >= 0 || (pdata && pdata->usb3_generic_phy)) { > + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); > + if (IS_ERR(dwc->usb3_generic_phy)) { > + dev_err(dev, "no usb3 phy configured yet"); > + return PTR_ERR(dwc->usb3_generic_phy); > + } > + dwc->usb3_phy = NULL; > + } Is there some specific reason for these checks? The driver should not need to care about the platform (DT, ACPI, platform based). Just get the phys and check the return value. In case ERR_PTR(-ENODEV) leave the phy as NULL and let the driver continue normally. With other errors you make the dwc3 probe fail. Thanks,
Hi, On Tuesday 03 December 2013 05:29 PM, Heikki Krogerus wrote: > Hi Kishon, > > On Wed, Oct 16, 2013 at 01:24:12AM +0530, Kishon Vijay Abraham I wrote: >> + count = of_property_match_string(node, "phy-names", "usb2-phy"); >> + if (count >= 0 || (pdata && pdata->usb2_generic_phy)) { >> + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy"); >> + if (IS_ERR(dwc->usb2_generic_phy)) { >> + dev_err(dev, "no usb2 phy configured yet"); >> + return PTR_ERR(dwc->usb2_generic_phy); >> + } >> + dwc->usb2_phy = NULL; >> + } >> + >> + count = of_property_match_string(node, "phy-names", "usb3-phy"); >> + if (count >= 0 || (pdata && pdata->usb3_generic_phy)) { >> + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); >> + if (IS_ERR(dwc->usb3_generic_phy)) { >> + dev_err(dev, "no usb3 phy configured yet"); >> + return PTR_ERR(dwc->usb3_generic_phy); >> + } >> + dwc->usb3_phy = NULL; >> + } > > Is there some specific reason for these checks? The driver should not > need to care about the platform (DT, ACPI, platform based). yeah just wanted to throw an error if a platform needs PHY but wasn't able to get it. Btw this has changed after my v3 of this patch series which I sent sometime back [1] where we use quirks to know if a PHY is needed for that platform or not. http://www.spinics.net/lists/linux-usb/msg98077.html Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt index e807635..471366d 100644 --- a/Documentation/devicetree/bindings/usb/dwc3.txt +++ b/Documentation/devicetree/bindings/usb/dwc3.txt @@ -6,11 +6,13 @@ Required properties: - compatible: must be "snps,dwc3" - reg : Address and length of the register set for the device - interrupts: Interrupts used by the dwc3 controller. + +Optional properties: - usb-phy : array of phandle for the PHY device. The first element in the array is expected to be a handle to the USB2/HS PHY and the second element is expected to be a handle to the USB3/SS PHY - -Optional properties: + - phys: from the *Generic PHY* bindings + - phy-names: from the *Generic PHY* bindings - tx-fifo-resize: determines if the FIFO *has* to be reallocated. This is usually a subnode to DWC3 glue to which it is connected. diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig index 8e385b4..64eed6f 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) && HAS_DMA select USB_PHY + select GENERIC_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 cb91d70..28bfa5b 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -82,6 +82,12 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc) usb_phy_init(dwc->usb2_phy); usb_phy_init(dwc->usb3_phy); + + if (dwc->usb2_generic_phy) + phy_init(dwc->usb2_generic_phy); + if (dwc->usb3_generic_phy) + phy_init(dwc->usb3_generic_phy); + mdelay(100); /* Clear USB3 PHY reset */ @@ -343,6 +349,11 @@ static void dwc3_core_exit(struct dwc3 *dwc) { usb_phy_shutdown(dwc->usb2_phy); usb_phy_shutdown(dwc->usb3_phy); + + if (dwc->usb2_generic_phy) + phy_power_off(dwc->usb2_generic_phy); + if (dwc->usb3_generic_phy) + phy_power_off(dwc->usb3_generic_phy); } #define DWC3_ALIGN_MASK (16 - 1) @@ -439,6 +450,26 @@ static int dwc3_probe(struct platform_device *pdev) dwc->usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3); } + count = of_property_match_string(node, "phy-names", "usb2-phy"); + if (count >= 0 || (pdata && pdata->usb2_generic_phy)) { + dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy"); + if (IS_ERR(dwc->usb2_generic_phy)) { + dev_err(dev, "no usb2 phy configured yet"); + return PTR_ERR(dwc->usb2_generic_phy); + } + dwc->usb2_phy = NULL; + } + + count = of_property_match_string(node, "phy-names", "usb3-phy"); + if (count >= 0 || (pdata && pdata->usb3_generic_phy)) { + dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); + if (IS_ERR(dwc->usb3_generic_phy)) { + dev_err(dev, "no usb3 phy configured yet"); + return PTR_ERR(dwc->usb3_generic_phy); + } + dwc->usb3_phy = NULL; + } + /* default to superspeed if no maximum_speed passed */ if (dwc->maximum_speed == USB_SPEED_UNKNOWN) dwc->maximum_speed = USB_SPEED_SUPER; @@ -462,6 +493,11 @@ static int dwc3_probe(struct platform_device *pdev) usb_phy_set_suspend(dwc->usb2_phy, 0); usb_phy_set_suspend(dwc->usb3_phy, 0); + if (dwc->usb2_generic_phy) + phy_power_on(dwc->usb2_generic_phy); + if (dwc->usb3_generic_phy) + phy_power_on(dwc->usb3_generic_phy); + spin_lock_init(&dwc->lock); platform_set_drvdata(pdev, dwc); @@ -588,6 +624,11 @@ static int dwc3_remove(struct platform_device *pdev) usb_phy_set_suspend(dwc->usb2_phy, 1); usb_phy_set_suspend(dwc->usb3_phy, 1); + if (dwc->usb2_generic_phy) + phy_power_off(dwc->usb2_generic_phy); + if (dwc->usb3_generic_phy) + phy_power_off(dwc->usb3_generic_phy); + pm_runtime_put(&pdev->dev); pm_runtime_disable(&pdev->dev); @@ -685,6 +726,11 @@ static int dwc3_suspend(struct device *dev) usb_phy_shutdown(dwc->usb3_phy); usb_phy_shutdown(dwc->usb2_phy); + if (dwc->usb2_generic_phy) + phy_exit(dwc->usb2_generic_phy); + if (dwc->usb3_generic_phy) + phy_exit(dwc->usb3_generic_phy); + return 0; } @@ -695,6 +741,12 @@ static int dwc3_resume(struct device *dev) usb_phy_init(dwc->usb3_phy); usb_phy_init(dwc->usb2_phy); + + if (dwc->usb2_generic_phy) + phy_init(dwc->usb2_generic_phy); + if (dwc->usb3_generic_phy) + phy_init(dwc->usb3_generic_phy); + msleep(100); spin_lock_irqsave(&dwc->lock, flags); diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index f8af8d4..01ec7d7 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -31,6 +31,8 @@ #include <linux/usb/gadget.h> #include <linux/usb/otg.h> +#include <linux/phy/phy.h> + /* Global constants */ #define DWC3_EP0_BOUNCE_SIZE 512 #define DWC3_ENDPOINTS_NUM 32 @@ -613,6 +615,8 @@ struct dwc3_scratchpad_array { * @dr_mode: requested mode of operation * @usb2_phy: pointer to USB2 PHY * @usb3_phy: pointer to USB3 PHY + * @usb2_generic_phy: pointer to USB2 PHY + * @usb3_generic_phy: pointer to USB3 PHY * @dcfg: saved contents of DCFG register * @gctl: saved contents of GCTL register * @is_selfpowered: true when we are selfpowered @@ -665,6 +669,9 @@ struct dwc3 { struct usb_phy *usb2_phy; struct usb_phy *usb3_phy; + struct phy *usb2_generic_phy; + struct phy *usb3_generic_phy; + void __iomem *regs; size_t regs_size; diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h index 49ffa6d..fc62a0f 100644 --- a/drivers/usb/dwc3/platform_data.h +++ b/drivers/usb/dwc3/platform_data.h @@ -26,4 +26,6 @@ struct dwc3_platform_data { bool tx_fifo_resize; bool usb2_phy; bool usb3_phy; + bool usb2_generic_phy; + bool usb3_generic_phy; };
Adapted dwc3 core to use the Generic PHY Framework. So for init, exit, power_on and power_off the following APIs are used phy_init(), phy_exit(), phy_power_on() and phy_power_off(). However using the old USB phy library wont be removed till the PHYs of all other SoC's using dwc3 core is adapted to the Generic PHY Framework. Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> --- Documentation/devicetree/bindings/usb/dwc3.txt | 6 ++- drivers/usb/dwc3/Kconfig | 1 + drivers/usb/dwc3/core.c | 52 ++++++++++++++++++++++++ drivers/usb/dwc3/core.h | 7 ++++ drivers/usb/dwc3/platform_data.h | 2 + 5 files changed, 66 insertions(+), 2 deletions(-)