[PATCH 1/2] usb: dwc3: exynos: force PHY control from XHCI HCD

[PATCH 1/2] usb: dwc3: exynos: force PHY control from XHCI HCD

[Marek Szyprowski]

Force controlling of the generic USB PHYs also from XHCI HCD to let XHCI
driver call calibrate() method during the HCD reset. DWC3 also controls
USB PHYs, but it is not able to call the calibrate() method at the right
moment.

Fixes: 6000b8d900cd ("usb: dwc3: disable USB core PHY management")
Reported-by: Stefan Agner <stefan@agner.ch>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
This fixes the issue reported here:
https://lore.kernel.org/all/808bdba846bb60456adf10a3016911ee@agner.ch/

Alternative for this change is a complete revert of the 6000b8d900cd
("usb: dwc3: disable USB core PHY management") commit.

Best regards
Marek Szyprowski, PhD
Samsung R&D Institute Poland 
---
 drivers/usb/dwc3/core.c        |  2 ++
 drivers/usb/dwc3/core.h        |  1 +
 drivers/usb/dwc3/dwc3-exynos.c | 65 +++++++++++++++++++++++-----------
 drivers/usb/dwc3/host.c        |  3 +-
 4 files changed, 49 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index c0e7c76dc5c8..87d4472fc031 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1548,6 +1548,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 				"snps,parkmode-disable-ss-quirk");
 	dwc->gfladj_refclk_lpm_sel = device_property_read_bool(dev,
 				"snps,gfladj-refclk-lpm-sel-quirk");
+	dwc->xhci_force_phy_control_quirk = device_property_read_bool(dev,
+				"snps,xhci_force_phy_control");
 
 	dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
 				"snps,tx_de_emphasis_quirk");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 8f9959ba9fd4..4d53edaf5999 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1318,6 +1318,7 @@ struct dwc3 {
 	unsigned		resume_hs_terminations:1;
 	unsigned		parkmode_disable_ss_quirk:1;
 	unsigned		gfladj_refclk_lpm_sel:1;
+	unsigned		xhci_force_phy_control_quirk:1;
 
 	unsigned		tx_de_emphasis_quirk:1;
 	unsigned		tx_de_emphasis:2;
diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index 0ecf20eeceee..fbeefc9eb9c5 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -13,6 +13,7 @@
 #include <linux/slab.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
+#include <linux/dma-mapping.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/regulator/consumer.h>
@@ -35,22 +36,20 @@ struct dwc3_exynos {
 
 	struct regulator	*vdd33;
 	struct regulator	*vdd10;
+	struct platform_device	*dwc3;
 };
 
-static int dwc3_exynos_remove_child(struct device *dev, void *unused)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-
-	platform_device_unregister(pdev);
-
-	return 0;
-}
+static const struct property_entry dwc3_exynos_properties[] = {
+	PROPERTY_ENTRY_BOOL("snps,xhci_force_phy_control"),
+	{}
+};
 
 static int dwc3_exynos_probe(struct platform_device *pdev)
 {
 	struct dwc3_exynos	*exynos;
 	struct device		*dev = &pdev->dev;
-	struct device_node	*node = dev->of_node;
+	struct device_node	*node = dev->of_node, *child;
+	struct platform_device	*dwc3;
 	const struct dwc3_exynos_driverdata *driver_data;
 	int			i, ret;
 
@@ -109,21 +108,44 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 		goto vdd10_err;
 	}
 
-	if (node) {
-		ret = of_platform_populate(node, NULL, NULL, dev);
-		if (ret) {
-			dev_err(dev, "failed to add dwc3 core\n");
-			goto populate_err;
-		}
-	} else {
-		dev_err(dev, "no device node, failed to add dwc3 core\n");
-		ret = -ENODEV;
-		goto populate_err;
+	child = of_get_next_child(node, NULL);
+	if (!child) {
+		dev_err(dev, "Failed to find DWC3 core device node\n");
+		goto dwc3_child_err;
+	}
+
+	dwc3 = of_device_alloc(child, NULL, dev);
+	if (!dwc3) {
+		dev_err(dev, "Failed to allocate DWC3 core device\n");
+		goto dwc3_alloc_err;
+	}
+
+	dwc3->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	dwc3->dev.dma_mask = &dwc3->dev.coherent_dma_mask;
+	dwc3->dev.bus = &platform_bus_type;
+
+	ret = device_create_managed_software_node(&dwc3->dev,
+						  dwc3_exynos_properties, NULL);
+	if (ret < 0) {
+		dev_err(dev, "Failed to add properties to DWC3 device\n");
+		goto dwc3_props_err;
+	}
+
+	ret = of_device_add(dwc3);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register DWC3 core device\n");
+		goto dwc3_props_err;
 	}
+	exynos->dwc3 = dwc3;
+	of_node_put(child);
 
 	return 0;
 
-populate_err:
+dwc3_props_err:
+	platform_device_put(dwc3);
+dwc3_alloc_err:
+	of_node_put(child);
+dwc3_child_err:
 	regulator_disable(exynos->vdd10);
 vdd10_err:
 	regulator_disable(exynos->vdd33);
@@ -142,7 +164,8 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
 	struct dwc3_exynos	*exynos = platform_get_drvdata(pdev);
 	int i;
 
-	device_for_each_child(&pdev->dev, NULL, dwc3_exynos_remove_child);
+	device_remove_software_node(&exynos->dwc3->dev);
+	of_device_unregister(exynos->dwc3);
 
 	for (i = exynos->num_clks - 1; i >= 0; i--)
 		clk_disable_unprepare(exynos->clks[i]);
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index a7154fe8206d..84f25f392dbd 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -97,7 +97,8 @@ int dwc3_host_init(struct dwc3 *dwc)
 		goto err;
 	}
 
-	ret = platform_device_add_data(xhci, &dwc3_xhci_plat_priv,
+	if (!dwc->xhci_force_phy_control_quirk)
+		ret = platform_device_add_data(xhci, &dwc3_xhci_plat_priv,
 					sizeof(dwc3_xhci_plat_priv));
 	if (ret)
 		goto err;
-- 
2.17.1

[PATCH 2/2] of: export of_device_add()

[Marek Szyprowski]

Export of_device_add() function to let DWC3 driver (Exynos variant) to
instantiate DWC3 core device from the respective child OF-node with a
custom, addtional properties added.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/of/device.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 8cefe5a7d04e..bc60c9b6863c 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -52,6 +52,7 @@ int of_device_add(struct platform_device *ofdev)
 
 	return device_add(&ofdev->dev);
 }
+EXPORT_SYMBOL(of_device_add);
 
 static void
 of_dma_set_restricted_buffer(struct device *dev, struct device_node *np)
-- 
2.17.1

Re: [PATCH 2/2] of: export of_device_add()

[Greg Kroah-Hartman]

On Thu, Nov 03, 2022 at 03:12:33PM +0100, Marek Szyprowski wrote:
> Export of_device_add() function to let DWC3 driver (Exynos variant) to
> instantiate DWC3 core device from the respective child OF-node with a
> custom, addtional properties added.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/of/device.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 8cefe5a7d04e..bc60c9b6863c 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -52,6 +52,7 @@ int of_device_add(struct platform_device *ofdev)
>  
>  	return device_add(&ofdev->dev);
>  }
> +EXPORT_SYMBOL(of_device_add);

I do not see any user of this symbol after this patch (no 3/2?)

So why is it needed?  No driver should ever be calling this function
directly.

thanks,

greg k-h

Re: [PATCH 2/2] of: export of_device_add()

[Johan Hovold]

On Thu, Nov 03, 2022 at 11:40:12PM +0900, Greg Kroah-Hartman wrote:
> On Thu, Nov 03, 2022 at 03:12:33PM +0100, Marek Szyprowski wrote:
> > Export of_device_add() function to let DWC3 driver (Exynos variant) to
> > instantiate DWC3 core device from the respective child OF-node with a
> > custom, addtional properties added.
> > 
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> >  drivers/of/device.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > index 8cefe5a7d04e..bc60c9b6863c 100644
> > --- a/drivers/of/device.c
> > +++ b/drivers/of/device.c
> > @@ -52,6 +52,7 @@ int of_device_add(struct platform_device *ofdev)
> >  
> >  	return device_add(&ofdev->dev);
> >  }
> > +EXPORT_SYMBOL(of_device_add);
> 
> I do not see any user of this symbol after this patch (no 3/2?)
> 
> So why is it needed?  No driver should ever be calling this function
> directly.

It was used by 1/2 so this would have broken bisectability.

Johan

Re: [PATCH 1/2] usb: dwc3: exynos: force PHY control from XHCI HCD

[Greg Kroah-Hartman]

On Thu, Nov 03, 2022 at 03:12:32PM +0100, Marek Szyprowski wrote:
> Force controlling of the generic USB PHYs also from XHCI HCD to let XHCI
> driver call calibrate() method during the HCD reset. DWC3 also controls
> USB PHYs, but it is not able to call the calibrate() method at the right
> moment.
> 
> Fixes: 6000b8d900cd ("usb: dwc3: disable USB core PHY management")
> Reported-by: Stefan Agner <stefan@agner.ch>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> This fixes the issue reported here:
> https://lore.kernel.org/all/808bdba846bb60456adf10a3016911ee@agner.ch/
> 
> Alternative for this change is a complete revert of the 6000b8d900cd
> ("usb: dwc3: disable USB core PHY management") commit.

We can revert that commit if something is broken so it can be fixed
properly instead of this.

thanks,

greg k-h

Re: [PATCH 1/2] usb: dwc3: exynos: force PHY control from XHCI HCD

[Johan Hovold]

On Thu, Nov 03, 2022 at 11:41:06PM +0900, Greg Kroah-Hartman wrote:
> On Thu, Nov 03, 2022 at 03:12:32PM +0100, Marek Szyprowski wrote:
> > Force controlling of the generic USB PHYs also from XHCI HCD to let XHCI
> > driver call calibrate() method during the HCD reset. DWC3 also controls
> > USB PHYs, but it is not able to call the calibrate() method at the right
> > moment.
> > 
> > Fixes: 6000b8d900cd ("usb: dwc3: disable USB core PHY management")
> > Reported-by: Stefan Agner <stefan@agner.ch>
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> > This fixes the issue reported here:
> > https://lore.kernel.org/all/808bdba846bb60456adf10a3016911ee@agner.ch/
> > 
> > Alternative for this change is a complete revert of the 6000b8d900cd
> > ("usb: dwc3: disable USB core PHY management") commit.
> 
> We can revert that commit if something is broken so it can be fixed
> properly instead of this.

Yeah, I'll send the revert I've prepared.

Johan