| Message ID | 1381866857-3861-2-git-send-email-kishon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Kishon, On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote: > There can be systems which does not have a external usb_phy, so get > usb_phy only if dt data indicates the presence of PHY 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> > --- > In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always > refers to usb3 phy. Since we've lived so long with this, this patch will make > an assumption that if only one entry is populated in *usb-phy* property, it will > be usb2 phy and the next entry will be usb3 phy. > > drivers/usb/dwc3/Kconfig | 1 + > drivers/usb/dwc3/core.c | 72 ++++++++++++++++++++------------------ > drivers/usb/dwc3/platform_data.h | 2 ++ > 3 files changed, 41 insertions(+), 34 deletions(-) > > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig > index 70fc430..8e385b4 100644 > --- a/drivers/usb/dwc3/Kconfig > +++ b/drivers/usb/dwc3/Kconfig > @@ -1,6 +1,7 @@ > config USB_DWC3 > tristate "DesignWare USB3 DRD Core Support" > depends on (USB || USB_GADGET) && HAS_DMA > + 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..cb91d70 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev) > struct device_node *node = dev->of_node; > struct resource *res; > struct dwc3 *dwc; > + int count; > > int ret = -ENOMEM; > > @@ -387,16 +388,49 @@ 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); > + count = of_count_phandle_with_args(node, "usb-phy", NULL); > + switch (count) { > + case 2: > + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, > + "usb-phy", 1); > + if (IS_ERR(dwc->usb3_phy)) { > + dev_err(dev, "usb3 phy not found\n"); > + return PTR_ERR(dwc->usb3_phy); > + } > + case 1: > + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, > + "usb-phy", 0); > + if (IS_ERR(dwc->usb2_phy)) { > + dev_err(dev, "usb2 phy not found\n"); > + return PTR_ERR(dwc->usb2_phy); > + } > + break; In the Exynos case, there is only 1 phy and it is the USB3 phy. This code will wrongly treat it as usb2_phy. > + default: > + dev_err(dev, "usb phy nodes not configured\n"); > + } > > 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->usb2_phy) { > + dwc->usb2_phy = devm_usb_get_phy(dev, > + USB_PHY_TYPE_USB2); > + if (IS_ERR(dwc->usb2_phy)) { > + dev_err(dev, "usb2 phy not found\n"); > + return PTR_ERR(dwc->usb2_phy); > + } > + } > + > + if (pdata->usb3_phy) { > + dwc->usb3_phy = devm_usb_get_phy(dev, > + USB_PHY_TYPE_USB3); > + if (IS_ERR(dwc->usb3_phy)) { > + dev_err(dev, "usb3 phy not found\n"); > + return PTR_ERR(dwc->usb3_phy); > + } > + } This part looks fine to me. > > dwc->needs_fifo_resize = pdata->tx_fifo_resize; > dwc->dr_mode = pdata->dr_mode; > @@ -409,36 +443,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..49ffa6d 100644 > --- a/drivers/usb/dwc3/platform_data.h > +++ b/drivers/usb/dwc3/platform_data.h > @@ -24,4 +24,6 @@ struct dwc3_platform_data { > enum usb_device_speed maximum_speed; > enum usb_dr_mode dr_mode; > bool tx_fifo_resize; > + bool usb2_phy; > + bool usb3_phy; > }; > cheers, -roger
Hi roger, On Wednesday 16 October 2013 06:33 PM, Roger Quadros wrote: > Hi Kishon, > > On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote: >> There can be systems which does not have a external usb_phy, so get >> usb_phy only if dt data indicates the presence of PHY 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> >> --- >> In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always >> refers to usb3 phy. Since we've lived so long with this, this patch will make >> an assumption that if only one entry is populated in *usb-phy* property, it will >> be usb2 phy and the next entry will be usb3 phy. >> >> drivers/usb/dwc3/Kconfig | 1 + >> drivers/usb/dwc3/core.c | 72 ++++++++++++++++++++------------------ >> drivers/usb/dwc3/platform_data.h | 2 ++ >> 3 files changed, 41 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >> index 70fc430..8e385b4 100644 >> --- a/drivers/usb/dwc3/Kconfig >> +++ b/drivers/usb/dwc3/Kconfig >> @@ -1,6 +1,7 @@ >> config USB_DWC3 >> tristate "DesignWare USB3 DRD Core Support" >> depends on (USB || USB_GADGET) && HAS_DMA >> + 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..cb91d70 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev) >> struct device_node *node = dev->of_node; >> struct resource *res; >> struct dwc3 *dwc; >> + int count; >> >> int ret = -ENOMEM; >> >> @@ -387,16 +388,49 @@ 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); >> + count = of_count_phandle_with_args(node, "usb-phy", NULL); >> + switch (count) { >> + case 2: >> + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, >> + "usb-phy", 1); >> + if (IS_ERR(dwc->usb3_phy)) { >> + dev_err(dev, "usb3 phy not found\n"); >> + return PTR_ERR(dwc->usb3_phy); >> + } >> + case 1: >> + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, >> + "usb-phy", 0); >> + if (IS_ERR(dwc->usb2_phy)) { >> + dev_err(dev, "usb2 phy not found\n"); >> + return PTR_ERR(dwc->usb2_phy); >> + } >> + break; > > In the Exynos case, there is only 1 phy and it is the USB3 phy. This code > will wrongly treat it as usb2_phy. That was the case even before this patch no? Unfortunately the old USB PHY library doesn't have APIs to get PHYs in a better way. If we try modifying the USB PHY library, it'll be kind of duplicating what is already there in the Generic PHY library. I'd rather prefer Exynos guys to use the new framework. Thanks Kishon
On 10/16/2013 04:10 PM, Kishon Vijay Abraham I wrote: > Hi roger, > > On Wednesday 16 October 2013 06:33 PM, Roger Quadros wrote: >> Hi Kishon, >> >> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote: >>> There can be systems which does not have a external usb_phy, so get >>> usb_phy only if dt data indicates the presence of PHY 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> >>> --- >>> In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always >>> refers to usb3 phy. Since we've lived so long with this, this patch will make >>> an assumption that if only one entry is populated in *usb-phy* property, it will >>> be usb2 phy and the next entry will be usb3 phy. >>> >>> drivers/usb/dwc3/Kconfig | 1 + >>> drivers/usb/dwc3/core.c | 72 ++++++++++++++++++++------------------ >>> drivers/usb/dwc3/platform_data.h | 2 ++ >>> 3 files changed, 41 insertions(+), 34 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >>> index 70fc430..8e385b4 100644 >>> --- a/drivers/usb/dwc3/Kconfig >>> +++ b/drivers/usb/dwc3/Kconfig >>> @@ -1,6 +1,7 @@ >>> config USB_DWC3 >>> tristate "DesignWare USB3 DRD Core Support" >>> depends on (USB || USB_GADGET) && HAS_DMA >>> + 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..cb91d70 100644 >>> --- a/drivers/usb/dwc3/core.c >>> +++ b/drivers/usb/dwc3/core.c >>> @@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev) >>> struct device_node *node = dev->of_node; >>> struct resource *res; >>> struct dwc3 *dwc; >>> + int count; >>> >>> int ret = -ENOMEM; >>> >>> @@ -387,16 +388,49 @@ 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); >>> + count = of_count_phandle_with_args(node, "usb-phy", NULL); >>> + switch (count) { >>> + case 2: >>> + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, >>> + "usb-phy", 1); >>> + if (IS_ERR(dwc->usb3_phy)) { >>> + dev_err(dev, "usb3 phy not found\n"); >>> + return PTR_ERR(dwc->usb3_phy); >>> + } >>> + case 1: >>> + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, >>> + "usb-phy", 0); >>> + if (IS_ERR(dwc->usb2_phy)) { >>> + dev_err(dev, "usb2 phy not found\n"); >>> + return PTR_ERR(dwc->usb2_phy); >>> + } >>> + break; >> >> In the Exynos case, there is only 1 phy and it is the USB3 phy. This code >> will wrongly treat it as usb2_phy. > > That was the case even before this patch no? Unfortunately the old USB PHY > library doesn't have APIs to get PHYs in a better way. If we try modifying the > USB PHY library, it'll be kind of duplicating what is already there in the > Generic PHY library. I'd rather prefer Exynos guys to use the new framework. OK. I agree with you. Do you know if there are users of dwc3 other than exynos5250 and omap5? If not, we should get rid of the old USB PHY altogether. cheers, -roger
On Wed, Oct 16, 2013 at 04:27:26PM +0300, Roger Quadros wrote: > On 10/16/2013 04:10 PM, Kishon Vijay Abraham I wrote: > > Hi roger, > > > > On Wednesday 16 October 2013 06:33 PM, Roger Quadros wrote: > >> Hi Kishon, > >> > >> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote: > >>> There can be systems which does not have a external usb_phy, so get > >>> usb_phy only if dt data indicates the presence of PHY 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> > >>> --- > >>> In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always > >>> refers to usb3 phy. Since we've lived so long with this, this patch will make > >>> an assumption that if only one entry is populated in *usb-phy* property, it will > >>> be usb2 phy and the next entry will be usb3 phy. > >>> > >>> drivers/usb/dwc3/Kconfig | 1 + > >>> drivers/usb/dwc3/core.c | 72 ++++++++++++++++++++------------------ > >>> drivers/usb/dwc3/platform_data.h | 2 ++ > >>> 3 files changed, 41 insertions(+), 34 deletions(-) > >>> > >>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig > >>> index 70fc430..8e385b4 100644 > >>> --- a/drivers/usb/dwc3/Kconfig > >>> +++ b/drivers/usb/dwc3/Kconfig > >>> @@ -1,6 +1,7 @@ > >>> config USB_DWC3 > >>> tristate "DesignWare USB3 DRD Core Support" > >>> depends on (USB || USB_GADGET) && HAS_DMA > >>> + 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..cb91d70 100644 > >>> --- a/drivers/usb/dwc3/core.c > >>> +++ b/drivers/usb/dwc3/core.c > >>> @@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev) > >>> struct device_node *node = dev->of_node; > >>> struct resource *res; > >>> struct dwc3 *dwc; > >>> + int count; > >>> > >>> int ret = -ENOMEM; > >>> > >>> @@ -387,16 +388,49 @@ 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); > >>> + count = of_count_phandle_with_args(node, "usb-phy", NULL); > >>> + switch (count) { > >>> + case 2: > >>> + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, > >>> + "usb-phy", 1); > >>> + if (IS_ERR(dwc->usb3_phy)) { > >>> + dev_err(dev, "usb3 phy not found\n"); > >>> + return PTR_ERR(dwc->usb3_phy); > >>> + } > >>> + case 1: > >>> + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, > >>> + "usb-phy", 0); > >>> + if (IS_ERR(dwc->usb2_phy)) { > >>> + dev_err(dev, "usb2 phy not found\n"); > >>> + return PTR_ERR(dwc->usb2_phy); > >>> + } > >>> + break; > >> > >> In the Exynos case, there is only 1 phy and it is the USB3 phy. This code > >> will wrongly treat it as usb2_phy. > > > > That was the case even before this patch no? Unfortunately the old USB PHY > > library doesn't have APIs to get PHYs in a better way. If we try modifying the > > USB PHY library, it'll be kind of duplicating what is already there in the > > Generic PHY library. I'd rather prefer Exynos guys to use the new framework. > > OK. I agree with you. > Do you know if there are users of dwc3 other than exynos5250 and omap5? > If not, we should get rid of the old USB PHY altogether. Intel's Baytrail, at least. But they don't use DT.
Hi, On Wed, Oct 16, 2013 at 01:24:11AM +0530, Kishon Vijay Abraham I wrote: > There can be systems which does not have a external usb_phy, so get > usb_phy only if dt data indicates the presence of PHY 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> I'm fine with this patch, but one comment below: > @@ -387,16 +388,49 @@ 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); > + count = of_count_phandle_with_args(node, "usb-phy", NULL); > + switch (count) { > + case 2: > + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, > + "usb-phy", 1); > + if (IS_ERR(dwc->usb3_phy)) { > + dev_err(dev, "usb3 phy not found\n"); > + return PTR_ERR(dwc->usb3_phy); > + } > + case 1: > + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, > + "usb-phy", 0); > + if (IS_ERR(dwc->usb2_phy)) { > + dev_err(dev, "usb2 phy not found\n"); > + return PTR_ERR(dwc->usb2_phy); > + } > + break; > + default: > + dev_err(dev, "usb phy nodes not configured\n"); > + } > > 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->usb2_phy) { > + dwc->usb2_phy = devm_usb_get_phy(dev, > + USB_PHY_TYPE_USB2); > + if (IS_ERR(dwc->usb2_phy)) { > + dev_err(dev, "usb2 phy not found\n"); > + return PTR_ERR(dwc->usb2_phy); > + } > + } > + > + if (pdata->usb3_phy) { > + dwc->usb3_phy = devm_usb_get_phy(dev, > + USB_PHY_TYPE_USB3); > + if (IS_ERR(dwc->usb3_phy)) { > + dev_err(dev, "usb3 phy not found\n"); > + return PTR_ERR(dwc->usb3_phy); > + } > + } > > dwc->needs_fifo_resize = pdata->tx_fifo_resize; > dwc->dr_mode = pdata->dr_mode; > @@ -409,36 +443,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; > - } the idea for doing the error checking here was actually to avoid duplicating it in all previous cases, as you did. Please keep the error checking here and you're good to go. > - > 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..49ffa6d 100644 > --- a/drivers/usb/dwc3/platform_data.h > +++ b/drivers/usb/dwc3/platform_data.h > @@ -24,4 +24,6 @@ struct dwc3_platform_data { > enum usb_device_speed maximum_speed; > enum usb_dr_mode dr_mode; > bool tx_fifo_resize; > + bool usb2_phy; > + bool usb3_phy; This would look better as a quirks flag, then we could: unsigned long quirks; #define DWC3_QUIRK_NO_USB3_PHY BIT(0) #define DWC3_QUIRK_NO_USB2_PHY BIT(1) ... if (!test_bit(DWC3_QUIRK_NO_USB3_PHY), &dwc->quirks && dwc->has_usb3_phy) { grab_usb3_phy(); } cheers
Hi, On Thursday 17 October 2013 10:08 PM, Felipe Balbi wrote: > Hi, > > On Wed, Oct 16, 2013 at 01:24:11AM +0530, Kishon Vijay Abraham I wrote: >> There can be systems which does not have a external usb_phy, so get >> usb_phy only if dt data indicates the presence of PHY 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> > > I'm fine with this patch, but one comment below: > >> @@ -387,16 +388,49 @@ 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); >> + count = of_count_phandle_with_args(node, "usb-phy", NULL); >> + switch (count) { >> + case 2: >> + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, >> + "usb-phy", 1); >> + if (IS_ERR(dwc->usb3_phy)) { >> + dev_err(dev, "usb3 phy not found\n"); >> + return PTR_ERR(dwc->usb3_phy); >> + } >> + case 1: >> + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, >> + "usb-phy", 0); >> + if (IS_ERR(dwc->usb2_phy)) { >> + dev_err(dev, "usb2 phy not found\n"); >> + return PTR_ERR(dwc->usb2_phy); >> + } >> + break; >> + default: >> + dev_err(dev, "usb phy nodes not configured\n"); >> + } >> >> 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->usb2_phy) { >> + dwc->usb2_phy = devm_usb_get_phy(dev, >> + USB_PHY_TYPE_USB2); >> + if (IS_ERR(dwc->usb2_phy)) { >> + dev_err(dev, "usb2 phy not found\n"); >> + return PTR_ERR(dwc->usb2_phy); >> + } >> + } >> + >> + if (pdata->usb3_phy) { >> + dwc->usb3_phy = devm_usb_get_phy(dev, >> + USB_PHY_TYPE_USB3); >> + if (IS_ERR(dwc->usb3_phy)) { >> + dev_err(dev, "usb3 phy not found\n"); >> + return PTR_ERR(dwc->usb3_phy); >> + } >> + } >> >> dwc->needs_fifo_resize = pdata->tx_fifo_resize; >> dwc->dr_mode = pdata->dr_mode; >> @@ -409,36 +443,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; >> - } > > the idea for doing the error checking here was actually to avoid > duplicating it in all previous cases, as you did. Please keep the error > checking here and you're good to go. Ok. > >> - >> 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..49ffa6d 100644 >> --- a/drivers/usb/dwc3/platform_data.h >> +++ b/drivers/usb/dwc3/platform_data.h >> @@ -24,4 +24,6 @@ struct dwc3_platform_data { >> enum usb_device_speed maximum_speed; >> enum usb_dr_mode dr_mode; >> bool tx_fifo_resize; >> + bool usb2_phy; >> + bool usb3_phy; > > This would look better as a quirks flag, then we could: > > unsigned long quirks; > #define DWC3_QUIRK_NO_USB3_PHY BIT(0) > #define DWC3_QUIRK_NO_USB2_PHY BIT(1) > > ... > > if (!test_bit(DWC3_QUIRK_NO_USB3_PHY), &dwc->quirks && > dwc->has_usb3_phy) { > grab_usb3_phy(); > } Ok. Thanks Kishon
Dear Kishon, Roger On Wed, Oct 16, 2013 at 6:40 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote: > Hi roger, > > On Wednesday 16 October 2013 06:33 PM, Roger Quadros wrote: >> Hi Kishon, Apologies for missing this thread for so long. >> >> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote: >>> There can be systems which does not have a external usb_phy, so get >>> usb_phy only if dt data indicates the presence of PHY 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> >>> --- >>> In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always >>> refers to usb3 phy. Since we've lived so long with this, this patch will make >>> an assumption that if only one entry is populated in *usb-phy* property, it will >>> be usb2 phy and the next entry will be usb3 phy. >>> >>> drivers/usb/dwc3/Kconfig | 1 + >>> drivers/usb/dwc3/core.c | 72 ++++++++++++++++++++------------------ >>> drivers/usb/dwc3/platform_data.h | 2 ++ >>> 3 files changed, 41 insertions(+), 34 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >>> index 70fc430..8e385b4 100644 >>> --- a/drivers/usb/dwc3/Kconfig >>> +++ b/drivers/usb/dwc3/Kconfig >>> @@ -1,6 +1,7 @@ >>> config USB_DWC3 >>> tristate "DesignWare USB3 DRD Core Support" >>> depends on (USB || USB_GADGET) && HAS_DMA >>> + 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..cb91d70 100644 >>> --- a/drivers/usb/dwc3/core.c >>> +++ b/drivers/usb/dwc3/core.c >>> @@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev) >>> struct device_node *node = dev->of_node; >>> struct resource *res; >>> struct dwc3 *dwc; >>> + int count; >>> >>> int ret = -ENOMEM; >>> >>> @@ -387,16 +388,49 @@ 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); >>> + count = of_count_phandle_with_args(node, "usb-phy", NULL); >>> + switch (count) { >>> + case 2: >>> + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, >>> + "usb-phy", 1); >>> + if (IS_ERR(dwc->usb3_phy)) { >>> + dev_err(dev, "usb3 phy not found\n"); >>> + return PTR_ERR(dwc->usb3_phy); >>> + } >>> + case 1: >>> + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, >>> + "usb-phy", 0); >>> + if (IS_ERR(dwc->usb2_phy)) { >>> + dev_err(dev, "usb2 phy not found\n"); >>> + return PTR_ERR(dwc->usb2_phy); >>> + } >>> + break; >> >> In the Exynos case, there is only 1 phy and it is the USB3 phy. This code >> will wrongly treat it as usb2_phy. Thank you Roger for your concern regarding Exynos case. It's true that, Exynos5 series of SoCs have got only one IP block for DWC3'c PHY. This block is actually a combo of USB2 phy (UTMI+) as well as USB3 phy (PIPE3). And the same is served by a single USB phy driver. This is also clarified in the thread : https://lkml.org/lkml/2013/11/5/160 > > That was the case even before this patch no? Unfortunately the old USB PHY > library doesn't have APIs to get PHYs in a better way. If we try modifying the > USB PHY library, it'll be kind of duplicating what is already there in the > Generic PHY library. I'd rather prefer Exynos guys to use the new framework. > > Thanks > Kishon > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 5, 2013 at 11:41 PM, Vivek Gautam <gautamvivek1987@gmail.com> wrote: > Dear Kishon, Roger > > > On Wed, Oct 16, 2013 at 6:40 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote: >> Hi roger, >> >> On Wednesday 16 October 2013 06:33 PM, Roger Quadros wrote: >>> Hi Kishon, > > Apologies for missing this thread for so long. > >>> >>> On 10/15/2013 10:54 PM, Kishon Vijay Abraham I wrote: >>>> There can be systems which does not have a external usb_phy, so get >>>> usb_phy only if dt data indicates the presence of PHY 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> >>>> --- >>>> In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always >>>> refers to usb3 phy. Since we've lived so long with this, this patch will make >>>> an assumption that if only one entry is populated in *usb-phy* property, it will >>>> be usb2 phy and the next entry will be usb3 phy. >>>> >>>> drivers/usb/dwc3/Kconfig | 1 + >>>> drivers/usb/dwc3/core.c | 72 ++++++++++++++++++++------------------ >>>> drivers/usb/dwc3/platform_data.h | 2 ++ >>>> 3 files changed, 41 insertions(+), 34 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig >>>> index 70fc430..8e385b4 100644 >>>> --- a/drivers/usb/dwc3/Kconfig >>>> +++ b/drivers/usb/dwc3/Kconfig >>>> @@ -1,6 +1,7 @@ >>>> config USB_DWC3 >>>> tristate "DesignWare USB3 DRD Core Support" >>>> depends on (USB || USB_GADGET) && HAS_DMA >>>> + 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..cb91d70 100644 >>>> --- a/drivers/usb/dwc3/core.c >>>> +++ b/drivers/usb/dwc3/core.c >>>> @@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev) >>>> struct device_node *node = dev->of_node; >>>> struct resource *res; >>>> struct dwc3 *dwc; >>>> + int count; >>>> >>>> int ret = -ENOMEM; >>>> >>>> @@ -387,16 +388,49 @@ 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); >>>> + count = of_count_phandle_with_args(node, "usb-phy", NULL); >>>> + switch (count) { >>>> + case 2: >>>> + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, >>>> + "usb-phy", 1); >>>> + if (IS_ERR(dwc->usb3_phy)) { >>>> + dev_err(dev, "usb3 phy not found\n"); >>>> + return PTR_ERR(dwc->usb3_phy); >>>> + } >>>> + case 1: >>>> + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, >>>> + "usb-phy", 0); >>>> + if (IS_ERR(dwc->usb2_phy)) { >>>> + dev_err(dev, "usb2 phy not found\n"); >>>> + return PTR_ERR(dwc->usb2_phy); >>>> + } >>>> + break; >>> >>> In the Exynos case, there is only 1 phy and it is the USB3 phy. This code >>> will wrongly treat it as usb2_phy. > > Thank you Roger for your concern regarding Exynos case. > It's true that, Exynos5 series of SoCs have got only one IP block for > DWC3'c PHY. > This block is actually a combo of USB2 phy (UTMI+) as well as USB3 phy (PIPE3). > And the same is served by a single USB phy driver. This is also > clarified in the thread : https://lkml.org/lkml/2013/11/5/160 > >> >> That was the case even before this patch no? Unfortunately the old USB PHY >> library doesn't have APIs to get PHYs in a better way. If we try modifying the >> USB PHY library, it'll be kind of duplicating what is already there in the >> Generic PHY library. I'd rather prefer Exynos guys to use the new framework. we have tried moving Samsung's USB phy controller driver for DWC3, to generic phy framework so that things look clearer on Samsung's side too. The necessary patches are availble at: https://lkml.org/lkml/2013/10/31/79 >> >> Thanks >> Kishon >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > Best Regards > Vivek Gautam > Samsung R&D Institute, Bangalore > India
Hi Felipe, On Thursday 17 October 2013 10:08 PM, Felipe Balbi wrote: > Hi, > > On Wed, Oct 16, 2013 at 01:24:11AM +0530, Kishon Vijay Abraham I wrote: >> There can be systems which does not have a external usb_phy, so get >> usb_phy only if dt data indicates the presence of PHY 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> > > I'm fine with this patch, but one comment below: > >> @@ -387,16 +388,49 @@ 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); >> + count = of_count_phandle_with_args(node, "usb-phy", NULL); >> + switch (count) { >> + case 2: >> + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, >> + "usb-phy", 1); >> + if (IS_ERR(dwc->usb3_phy)) { >> + dev_err(dev, "usb3 phy not found\n"); >> + return PTR_ERR(dwc->usb3_phy); >> + } >> + case 1: >> + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, >> + "usb-phy", 0); >> + if (IS_ERR(dwc->usb2_phy)) { >> + dev_err(dev, "usb2 phy not found\n"); >> + return PTR_ERR(dwc->usb2_phy); >> + } >> + break; >> + default: >> + dev_err(dev, "usb phy nodes not configured\n"); >> + } >> >> 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->usb2_phy) { >> + dwc->usb2_phy = devm_usb_get_phy(dev, >> + USB_PHY_TYPE_USB2); >> + if (IS_ERR(dwc->usb2_phy)) { >> + dev_err(dev, "usb2 phy not found\n"); >> + return PTR_ERR(dwc->usb2_phy); >> + } >> + } >> + >> + if (pdata->usb3_phy) { >> + dwc->usb3_phy = devm_usb_get_phy(dev, >> + USB_PHY_TYPE_USB3); >> + if (IS_ERR(dwc->usb3_phy)) { >> + dev_err(dev, "usb3 phy not found\n"); >> + return PTR_ERR(dwc->usb3_phy); >> + } >> + } >> >> dwc->needs_fifo_resize = pdata->tx_fifo_resize; >> dwc->dr_mode = pdata->dr_mode; >> @@ -409,36 +443,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; >> - } > > the idea for doing the error checking here was actually to avoid > duplicating it in all previous cases, as you did. Please keep the error > checking here and you're good to go. > >> - >> 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..49ffa6d 100644 >> --- a/drivers/usb/dwc3/platform_data.h >> +++ b/drivers/usb/dwc3/platform_data.h >> @@ -24,4 +24,6 @@ struct dwc3_platform_data { >> enum usb_device_speed maximum_speed; >> enum usb_dr_mode dr_mode; >> bool tx_fifo_resize; >> + bool usb2_phy; >> + bool usb3_phy; > > This would look better as a quirks flag, then we could: > > unsigned long quirks; > #define DWC3_QUIRK_NO_USB3_PHY BIT(0) > #define DWC3_QUIRK_NO_USB2_PHY BIT(1) Should this quirk be used for dt also? Currently we find if it has usb3 phy or usb2 phy from the dt data only. But if we add a quirk, we'll have to add a property to populate the quirk no? Thanks Kishon > > ... > > if (!test_bit(DWC3_QUIRK_NO_USB3_PHY), &dwc->quirks && > dwc->has_usb3_phy) { > grab_usb3_phy(); > } > > cheers >
Hi, On Mon, Nov 11, 2013 at 08:06:12PM +0530, Kishon Vijay Abraham I wrote: > >> diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h > >> index 7db34f0..49ffa6d 100644 > >> --- a/drivers/usb/dwc3/platform_data.h > >> +++ b/drivers/usb/dwc3/platform_data.h > >> @@ -24,4 +24,6 @@ struct dwc3_platform_data { > >> enum usb_device_speed maximum_speed; > >> enum usb_dr_mode dr_mode; > >> bool tx_fifo_resize; > >> + bool usb2_phy; > >> + bool usb3_phy; > > > > This would look better as a quirks flag, then we could: > > > > unsigned long quirks; > > #define DWC3_QUIRK_NO_USB3_PHY BIT(0) > > #define DWC3_QUIRK_NO_USB2_PHY BIT(1) > > Should this quirk be used for dt also? Currently we find if it has usb3 phy or > usb2 phy from the dt data only. But if we add a quirk, we'll have to add a > property to populate the quirk no? either we use the quirk, or use the fact that no <&usb2_phy> phandle is defined, would work both ways, no ?
Hi, On Tuesday 26 November 2013 03:02 AM, Felipe Balbi wrote: > Hi, > > On Mon, Nov 11, 2013 at 08:06:12PM +0530, Kishon Vijay Abraham I wrote: >>>> diff --git a/drivers/usb/dwc3/platform_data.h b/drivers/usb/dwc3/platform_data.h >>>> index 7db34f0..49ffa6d 100644 >>>> --- a/drivers/usb/dwc3/platform_data.h >>>> +++ b/drivers/usb/dwc3/platform_data.h >>>> @@ -24,4 +24,6 @@ struct dwc3_platform_data { >>>> enum usb_device_speed maximum_speed; >>>> enum usb_dr_mode dr_mode; >>>> bool tx_fifo_resize; >>>> + bool usb2_phy; >>>> + bool usb3_phy; >>> >>> This would look better as a quirks flag, then we could: >>> >>> unsigned long quirks; >>> #define DWC3_QUIRK_NO_USB3_PHY BIT(0) >>> #define DWC3_QUIRK_NO_USB2_PHY BIT(1) >> >> Should this quirk be used for dt also? Currently we find if it has usb3 phy or >> usb2 phy from the dt data only. But if we add a quirk, we'll have to add a >> property to populate the quirk no? > > either we use the quirk, or use the fact that no <&usb2_phy> phandle is > defined, would work both ways, no ? In my v3, I've made both to use quirks since we don't want to have separate mechanism for dt and non-dt stuff to know the presence of a particular PHY. Thanks Kishon
Hi, On Thu, Oct 17, 2013 at 09:54:26AM -0500, Felipe Balbi wrote: > On Wed, Oct 16, 2013 at 04:27:26PM +0300, Roger Quadros wrote: > > On 10/16/2013 04:10 PM, Kishon Vijay Abraham I wrote: > > Do you know if there are users of dwc3 other than exynos5250 and omap5? > > If not, we should get rid of the old USB PHY altogether. > > Intel's Baytrail, at least. But they don't use DT. I don't see any use for the generic-phy when the device is enumerated from PCI. If dwc3 can live without phys, there should not be any problem dropping the old USB PHY support. Br,
On Tue, Dec 03, 2013 at 12:39:50PM +0200, Heikki Krogerus wrote: > Hi, > > On Thu, Oct 17, 2013 at 09:54:26AM -0500, Felipe Balbi wrote: > > On Wed, Oct 16, 2013 at 04:27:26PM +0300, Roger Quadros wrote: > > > On 10/16/2013 04:10 PM, Kishon Vijay Abraham I wrote: > > > Do you know if there are users of dwc3 other than exynos5250 and omap5? > > > If not, we should get rid of the old USB PHY altogether. > > > > Intel's Baytrail, at least. But they don't use DT. > > I don't see any use for the generic-phy when the device is enumerated > from PCI. If dwc3 can live without phys, there should not be any > problem dropping the old USB PHY support. yeah, I don't want to drop PHY support, I want to require everybody to provide a PHY, otherwise we have too many options to handle and that's never clean.
Hi, On Fri, Dec 06, 2013 at 02:15:30PM -0600, Felipe Balbi wrote: > On Tue, Dec 03, 2013 at 12:39:50PM +0200, Heikki Krogerus wrote: > > On Thu, Oct 17, 2013 at 09:54:26AM -0500, Felipe Balbi wrote: > > > On Wed, Oct 16, 2013 at 04:27:26PM +0300, Roger Quadros wrote: > > > > On 10/16/2013 04:10 PM, Kishon Vijay Abraham I wrote: > > > > Do you know if there are users of dwc3 other than exynos5250 and omap5? > > > > If not, we should get rid of the old USB PHY altogether. > > > > > > Intel's Baytrail, at least. But they don't use DT. > > > > I don't see any use for the generic-phy when the device is enumerated > > from PCI. If dwc3 can live without phys, there should not be any > > problem dropping the old USB PHY support. > > yeah, I don't want to drop PHY support, I want to require everybody to > provide a PHY, otherwise we have too many options to handle and that's > never clean. I have to clarify in case there was a misunderstanding. When I said generic-phy I meant drivers/usb/phy/phy-generic.c and I was not talking about Kishon's new generic PHY framework. So I was only saying that if the dwc3-pci.c is the only thing that needs the old usb phy support, then there should not be any problem dropping the support for it. Thanks,
On Mon, Dec 09, 2013 at 10:52:45AM +0200, Heikki Krogerus wrote: > Hi, > > On Fri, Dec 06, 2013 at 02:15:30PM -0600, Felipe Balbi wrote: > > On Tue, Dec 03, 2013 at 12:39:50PM +0200, Heikki Krogerus wrote: > > > On Thu, Oct 17, 2013 at 09:54:26AM -0500, Felipe Balbi wrote: > > > > On Wed, Oct 16, 2013 at 04:27:26PM +0300, Roger Quadros wrote: > > > > > On 10/16/2013 04:10 PM, Kishon Vijay Abraham I wrote: > > > > > Do you know if there are users of dwc3 other than exynos5250 and omap5? > > > > > If not, we should get rid of the old USB PHY altogether. > > > > > > > > Intel's Baytrail, at least. But they don't use DT. > > > > > > I don't see any use for the generic-phy when the device is enumerated > > > from PCI. If dwc3 can live without phys, there should not be any > > > problem dropping the old USB PHY support. > > > > yeah, I don't want to drop PHY support, I want to require everybody to > > provide a PHY, otherwise we have too many options to handle and that's > > never clean. > > I have to clarify in case there was a misunderstanding. When I said > generic-phy I meant drivers/usb/phy/phy-generic.c and I was not > talking about Kishon's new generic PHY framework. > > So I was only saying that if the dwc3-pci.c is the only thing that > needs the old usb phy support, then there should not be any problem > dropping the support for it. oh, ok. Got it now, thanks.
diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig index 70fc430..8e385b4 100644 --- a/drivers/usb/dwc3/Kconfig +++ b/drivers/usb/dwc3/Kconfig @@ -1,6 +1,7 @@ config USB_DWC3 tristate "DesignWare USB3 DRD Core Support" depends on (USB || USB_GADGET) && HAS_DMA + 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..cb91d70 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -354,6 +354,7 @@ static int dwc3_probe(struct platform_device *pdev) struct device_node *node = dev->of_node; struct resource *res; struct dwc3 *dwc; + int count; int ret = -ENOMEM; @@ -387,16 +388,49 @@ 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); + count = of_count_phandle_with_args(node, "usb-phy", NULL); + switch (count) { + case 2: + dwc->usb3_phy = devm_usb_get_phy_by_phandle(dev, + "usb-phy", 1); + if (IS_ERR(dwc->usb3_phy)) { + dev_err(dev, "usb3 phy not found\n"); + return PTR_ERR(dwc->usb3_phy); + } + case 1: + dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, + "usb-phy", 0); + if (IS_ERR(dwc->usb2_phy)) { + dev_err(dev, "usb2 phy not found\n"); + return PTR_ERR(dwc->usb2_phy); + } + break; + default: + dev_err(dev, "usb phy nodes not configured\n"); + } 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->usb2_phy) { + dwc->usb2_phy = devm_usb_get_phy(dev, + USB_PHY_TYPE_USB2); + if (IS_ERR(dwc->usb2_phy)) { + dev_err(dev, "usb2 phy not found\n"); + return PTR_ERR(dwc->usb2_phy); + } + } + + if (pdata->usb3_phy) { + dwc->usb3_phy = devm_usb_get_phy(dev, + USB_PHY_TYPE_USB3); + if (IS_ERR(dwc->usb3_phy)) { + dev_err(dev, "usb3 phy not found\n"); + return PTR_ERR(dwc->usb3_phy); + } + } dwc->needs_fifo_resize = pdata->tx_fifo_resize; dwc->dr_mode = pdata->dr_mode; @@ -409,36 +443,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..49ffa6d 100644 --- a/drivers/usb/dwc3/platform_data.h +++ b/drivers/usb/dwc3/platform_data.h @@ -24,4 +24,6 @@ struct dwc3_platform_data { enum usb_device_speed maximum_speed; enum usb_dr_mode dr_mode; bool tx_fifo_resize; + bool usb2_phy; + bool usb3_phy; };
There can be systems which does not have a external usb_phy, so get usb_phy only if dt data indicates the presence of PHY 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> --- In usb_get_phy_by_phandle, index 0 always refers to usb2 phy and index 1 always refers to usb3 phy. Since we've lived so long with this, this patch will make an assumption that if only one entry is populated in *usb-phy* property, it will be usb2 phy and the next entry will be usb3 phy. drivers/usb/dwc3/Kconfig | 1 + drivers/usb/dwc3/core.c | 72 ++++++++++++++++++++------------------ drivers/usb/dwc3/platform_data.h | 2 ++ 3 files changed, 41 insertions(+), 34 deletions(-)