Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY
Hi, On Mon, Dec 09, 2013 at 11:26:04AM +0200, Heikki Krogerus wrote: Can you guys explain why is something like this needed? Like with clocks and gpios, the device drivers shouldn't need to care any more if the platform has the phys or not. -ENODEV tells you your platform Shouldn't we report if a particular platform needs a PHY and not able to get it. How will a user know if a particular controller is not working because it's not able to get and initialize the PHYs? Don't you think in such cases it's better to fail (and return from probe) because the controller will not work anyway without the PHY? My point is that you do not need to separately tell this to the driver like you do with the quirks (if you did, then you would need to fix your framework and not hack the drivers). Like I said, ENODEV tells you that there is no phy on this platform for you, allowing you to safely continue. If your phy driver is not loaded, the framework already returns EPROBE_DEFER, right. Any other right. but that doesn't consider broken dt data. With quirks we'll able to tell if a controller in a particular platform has PHY or not without depending on the dt data. Broken dt data? What kind of scenario are you thinking here? Do you mean case where the dt does not describe the phy on a platform that depends on it? Shouldn't that problem be fixed in the dt and not hacked in the drivers? Or are you thinking about something else? Is there a case where something like that is actually happening? I'm guessing I'm not getting an answer to this one. Look, this patch will not work with ACPI enumerated devices. We will have a platform providing a single ACPI id, but there is a whole bunch of boards based on it and we have no way of telling which of them need/have phys to deal with and which ones don't. Thanks, -- heikki -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY
On Wednesday 11 December 2013 02:23 PM, Heikki Krogerus wrote: Hi, On Mon, Dec 09, 2013 at 11:26:04AM +0200, Heikki Krogerus wrote: Can you guys explain why is something like this needed? Like with clocks and gpios, the device drivers shouldn't need to care any more if the platform has the phys or not. -ENODEV tells you your platform Shouldn't we report if a particular platform needs a PHY and not able to get it. How will a user know if a particular controller is not working because it's not able to get and initialize the PHYs? Don't you think in such cases it's better to fail (and return from probe) because the controller will not work anyway without the PHY? My point is that you do not need to separately tell this to the driver like you do with the quirks (if you did, then you would need to fix your framework and not hack the drivers). Like I said, ENODEV tells you that there is no phy on this platform for you, allowing you to safely continue. If your phy driver is not loaded, the framework already returns EPROBE_DEFER, right. Any other right. but that doesn't consider broken dt data. With quirks we'll able to tell if a controller in a particular platform has PHY or not without depending on the dt data. Broken dt data? What kind of scenario are you thinking here? Do you mean case where the dt does not describe the phy on a platform that depends on it? Shouldn't that problem be fixed in the dt and not hacked in the drivers? Or are you thinking about something else? Is there a case where something like that is actually happening? I'm guessing I'm not getting an answer to this one. Look, this patch will not work with ACPI enumerated devices. We will have a platform providing a single ACPI id, but there is a whole bunch of boards based on it and we have no way of telling which of them need/have phys to deal with and which ones don't. Alright.. I'll drop this patch then. Thanks Kishon -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY
Hi, On Mon, Dec 09, 2013 at 12:43:37PM +0530, Kishon Vijay Abraham I wrote: On Thursday 05 December 2013 01:28 PM, Heikki Krogerus wrote: On Thu, Dec 05, 2013 at 12:04:46PM +0530, Kishon Vijay Abraham I wrote: On Wednesday 04 December 2013 08:10 PM, Heikki Krogerus wrote: On Mon, Nov 25, 2013 at 03:31:24PM +0530, Kishon Vijay Abraham I wrote: There can be systems which does not have an external phy, so get phy only if no quirks are added that indicates the PHY is not present. Introduced two quirk flags to indicate the *absence* of usb2 phy and usb3 phy. Also remove checking if return value is -ENXIO since it's now changed to always enable usb_phy layer. Can you guys explain why is something like this needed? Like with clocks and gpios, the device drivers shouldn't need to care any more if the platform has the phys or not. -ENODEV tells you your platform Shouldn't we report if a particular platform needs a PHY and not able to get it. How will a user know if a particular controller is not working because it's not able to get and initialize the PHYs? Don't you think in such cases it's better to fail (and return from probe) because the controller will not work anyway without the PHY? My point is that you do not need to separately tell this to the driver like you do with the quirks (if you did, then you would need to fix your framework and not hack the drivers). Like I said, ENODEV tells you that there is no phy on this platform for you, allowing you to safely continue. If your phy driver is not loaded, the framework already returns EPROBE_DEFER, right. Any other right. but that doesn't consider broken dt data. With quirks we'll able to tell if a controller in a particular platform has PHY or not without depending on the dt data. Broken dt data? What kind of scenario are you thinking here? Do you mean case where the dt does not describe the phy on a platform that depends on it? Shouldn't that problem be fixed in the dt and not hacked in the drivers? Or are you thinking about something else? Is there a case where something like that is actually happening? Br, -- heikki -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY
Hi, On Thursday 05 December 2013 01:28 PM, Heikki Krogerus wrote: Hi, On Thu, Dec 05, 2013 at 12:04:46PM +0530, Kishon Vijay Abraham I wrote: On Wednesday 04 December 2013 08:10 PM, Heikki Krogerus wrote: On Mon, Nov 25, 2013 at 03:31:24PM +0530, Kishon Vijay Abraham I wrote: There can be systems which does not have an external phy, so get phy only if no quirks are added that indicates the PHY is not present. Introduced two quirk flags to indicate the *absence* of usb2 phy and usb3 phy. Also remove checking if return value is -ENXIO since it's now changed to always enable usb_phy layer. Can you guys explain why is something like this needed? Like with clocks and gpios, the device drivers shouldn't need to care any more if the platform has the phys or not. -ENODEV tells you your platform Shouldn't we report if a particular platform needs a PHY and not able to get it. How will a user know if a particular controller is not working because it's not able to get and initialize the PHYs? Don't you think in such cases it's better to fail (and return from probe) because the controller will not work anyway without the PHY? My point is that you do not need to separately tell this to the driver like you do with the quirks (if you did, then you would need to fix your framework and not hack the drivers). Like I said, ENODEV tells you that there is no phy on this platform for you, allowing you to safely continue. If your phy driver is not loaded, the framework already returns EPROBE_DEFER, right. Any other right. but that doesn't consider broken dt data. With quirks we'll able to tell if a controller in a particular platform has PHY or not without depending on the dt data. error when getting the phy you can consider critical. They are the errors telling you that you do need a phy on this platform, but something actually went wrong when getting it. Not on all scenarios though :-s Thanks Kishon -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY
Hi guys, Kishon, sorry I did not see this v3 set. On Mon, Nov 25, 2013 at 03:31:24PM +0530, Kishon Vijay Abraham I wrote: There can be systems which does not have an external phy, so get phy only if no quirks are added that indicates the PHY is not present. Introduced two quirk flags to indicate the *absence* of usb2 phy and usb3 phy. Also remove checking if return value is -ENXIO since it's now changed to always enable usb_phy layer. Can you guys explain why is something like this needed? Like with clocks and gpios, the device drivers shouldn't need to care any more if the platform has the phys or not. -ENODEV tells you your platform does not have them, so go ahead and continue when getting the phy. I just tested removing the phy registering from dwc3-pci.c and all I had to do was to add IS_ERR(phy) checks where the old usb phys were used and everything worked fine? What am I missing? Br, -- heikki -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY
Hi, On Wednesday 04 December 2013 08:10 PM, Heikki Krogerus wrote: Hi guys, Kishon, sorry I did not see this v3 set. On Mon, Nov 25, 2013 at 03:31:24PM +0530, Kishon Vijay Abraham I wrote: There can be systems which does not have an external phy, so get phy only if no quirks are added that indicates the PHY is not present. Introduced two quirk flags to indicate the *absence* of usb2 phy and usb3 phy. Also remove checking if return value is -ENXIO since it's now changed to always enable usb_phy layer. Can you guys explain why is something like this needed? Like with clocks and gpios, the device drivers shouldn't need to care any more if the platform has the phys or not. -ENODEV tells you your platform Shouldn't we report if a particular platform needs a PHY and not able to get it. How will a user know if a particular controller is not working because it's not able to get and initialize the PHYs? Don't you think in such cases it's better to fail (and return from probe) because the controller will not work anyway without the PHY? Thanks Kishon -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY
Hi, On Thu, Dec 05, 2013 at 12:04:46PM +0530, Kishon Vijay Abraham I wrote: On Wednesday 04 December 2013 08:10 PM, Heikki Krogerus wrote: On Mon, Nov 25, 2013 at 03:31:24PM +0530, Kishon Vijay Abraham I wrote: There can be systems which does not have an external phy, so get phy only if no quirks are added that indicates the PHY is not present. Introduced two quirk flags to indicate the *absence* of usb2 phy and usb3 phy. Also remove checking if return value is -ENXIO since it's now changed to always enable usb_phy layer. Can you guys explain why is something like this needed? Like with clocks and gpios, the device drivers shouldn't need to care any more if the platform has the phys or not. -ENODEV tells you your platform Shouldn't we report if a particular platform needs a PHY and not able to get it. How will a user know if a particular controller is not working because it's not able to get and initialize the PHYs? Don't you think in such cases it's better to fail (and return from probe) because the controller will not work anyway without the PHY? My point is that you do not need to separately tell this to the driver like you do with the quirks (if you did, then you would need to fix your framework and not hack the drivers). Like I said, ENODEV tells you that there is no phy on this platform for you, allowing you to safely continue. If your phy driver is not loaded, the framework already returns EPROBE_DEFER, right. Any other error when getting the phy you can consider critical. They are the errors telling you that you do need a phy on this platform, but something actually went wrong when getting it. Those quirks should always be avoided, and I don't see any use for them here. Thanks Kishon Thanks, -- heikki -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY
There can be systems which does not have an external phy, so get
phy only if no quirks are added that indicates the PHY is not present.
Introduced two quirk flags to indicate the *absence* of usb2 phy and
usb3 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 kis...@ti.com
---
drivers/usb/dwc3/Kconfig |1 +
drivers/usb/dwc3/core.c | 93 --
drivers/usb/dwc3/core.h | 15 ++
drivers/usb/dwc3/platform_data.h |5 ++
4 files changed, 69 insertions(+), 45 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 986674f..f20f4a1 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -32,6 +32,7 @@
#include linux/delay.h
#include linux/dma-mapping.h
#include linux/of.h
+#include linux/of_device.h
#include linux/usb/ch9.h
#include linux/usb/gadget.h
@@ -351,13 +352,28 @@ static void dwc3_core_exit(struct dwc3 *dwc)
#define DWC3_ALIGN_MASK(16 - 1)
+#ifdef CONFIG_OF
+static const struct of_device_id of_dwc3_match[] = {
+ {
+ .compatible = snps,dwc3
+ },
+ {
+ .compatible = synopsys,dwc3
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(of, of_dwc3_match);
+#endif
+
static int dwc3_probe(struct platform_device *pdev)
{
struct device *dev = pdev-dev;
struct dwc3_platform_data *pdata = dev_get_platdata(dev);
struct device_node *node = dev-of_node;
+ const struct of_device_id *of_id;
struct resource *res;
struct dwc3 *dwc;
+ struct dwc3_data*dwc_data;
int ret = -ENOMEM;
@@ -391,16 +407,36 @@ 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);
+ of_id = of_match_device(of_match_ptr(of_dwc3_match),
+ pdev-dev);
+ if (!of_id)
+ return -EINVAL;
+
+ dwc_data = (struct dwc3_data *)of_id-data;
+
+ if (dwc_data)
+ dwc-quirks = dwc_data-quirks;
+
+ if (!(dwc-quirks DWC3_QUIRK_NO_USB2_PHY))
+ dwc-usb2_phy = devm_usb_get_phy_by_phandle(dev,
+ usb-phy, 0);
+
+ if (!(dwc-quirks DWC3_QUIRK_NO_USB3_PHY))
+ dwc-usb3_phy = devm_usb_get_phy_by_phandle(dev,
+ usb-phy, 1);
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-quirks DWC3_QUIRK_NO_USB2_PHY))
+ dwc-usb2_phy = devm_usb_get_phy(dev,
+ USB_PHY_TYPE_USB2);
+
+ if (!(pdata-quirks DWC3_QUIRK_NO_USB3_PHY))
+ dwc-usb3_phy = devm_usb_get_phy(dev,
+ USB_PHY_TYPE_USB3);
dwc-needs_fifo_resize = pdata-tx_fifo_resize;
dwc-dr_mode = pdata-dr_mode;
@@ -409,40 +445,20 @@ static int dwc3_probe(struct platform_device *pdev)
dwc-usb3_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB3);
}
- /* default to superspeed if no maximum_speed passed */
- 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;
+ dev_err(dev, usb2 phy not found\n);
+ return
Re: [PATCH v3 04/10] usb: dwc3: use quirks to know if a particualr platform doesn't have PHY
On Mon, Nov 25, 2013 at 03:31:24PM +0530, Kishon Vijay Abraham I wrote:
There can be systems which does not have an external phy, so get
phy only if no quirks are added that indicates the PHY is not present.
Introduced two quirk flags to indicate the *absence* of usb2 phy and
usb3 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 kis...@ti.com
---
drivers/usb/dwc3/Kconfig |1 +
drivers/usb/dwc3/core.c | 93
--
drivers/usb/dwc3/core.h | 15 ++
drivers/usb/dwc3/platform_data.h |5 ++
4 files changed, 69 insertions(+), 45 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
no more selects, we've already had too many problems caused by bogus
usage of select.
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 986674f..f20f4a1 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -32,6 +32,7 @@
#include linux/delay.h
#include linux/dma-mapping.h
#include linux/of.h
+#include linux/of_device.h
#include linux/usb/ch9.h
#include linux/usb/gadget.h
@@ -351,13 +352,28 @@ static void dwc3_core_exit(struct dwc3 *dwc)
#define DWC3_ALIGN_MASK (16 - 1)
+#ifdef CONFIG_OF
+static const struct of_device_id of_dwc3_match[] = {
+ {
+ .compatible = snps,dwc3
+ },
+ {
+ .compatible = synopsys,dwc3
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(of, of_dwc3_match);
+#endif
+
you can split this patch a little bit just to make it easier to review
because there are a few changes which are really obvious:
a) move of_device_id higher in source code so you can use it from probe.
while at that, aso use of_match_device() and return early if it
doesn't match anything.
b) introduce struct dwc3_data.
c) initialize of_id-data field.
d) start using struct dwc3_data
this will make to put all obvious changes (a-c) in separate patches so
review can focus on (d).
--
balbi
signature.asc
Description: Digital signature
