Re: [PATCH] usb: host: xhci-plat: Make enum xhci_plat_type start at a non zero value

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6040 bytes --]


Hi,

Gregory CLEMENT <gregory.clement@free-electrons.com> writes:
>> Peter Griffin <peter.griffin@linaro.org> writes:
>>> Otherwise generic-xhci and xhci-platform which have no data get wrongly
>>> detected as XHCI_PLAT_TYPE_MARVELL_ARMADA by xhci_plat_type_is().
>>>
>>> This fixes a regression in v4.5 for STiH407 family SoC's which use the
>>> synopsis dwc3 IP, whereby the disable_clk error path gets taken due to
>>> wrongly being detected as XHCI_PLAT_TYPE_MARVELL_ARMADA and the hcd never
>>> gets added.
>>>
>>> I suspect this will also fix other dwc3 DT platforms such as Exynos,
>>> although I've only tested on STih410 SoC.
>>>
>>> Fixes: 4efb2f694114 ("usb: host: xhci-plat: add struct xhci_plat_priv")
>>> Cc: stable@vger.kernel.org
>>> Cc: gregory.clement@free-electrons.com
>>> Cc: yoshihiro.shimoda.uh@renesas.com
>>> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
>>> ---
>>>  drivers/usb/host/xhci-plat.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
>>> index 5a2e2e3..529c3c4 100644
>>> --- a/drivers/usb/host/xhci-plat.h
>>> +++ b/drivers/usb/host/xhci-plat.h
>>> @@ -14,7 +14,7 @@
>>>  #include "xhci.h"	/* for hcd_to_xhci() */
>>>  
>>>  enum xhci_plat_type {
>>> -	XHCI_PLAT_TYPE_MARVELL_ARMADA,
>>> +	XHCI_PLAT_TYPE_MARVELL_ARMADA = 1,
>>>  	XHCI_PLAT_TYPE_RENESAS_RCAR_GEN2,
>>>  	XHCI_PLAT_TYPE_RENESAS_RCAR_GEN3,
>>
>> aren't these platforms using device tree ? Why aren't these just
>> different compatible strings ?
>
> According to 4efb2f69411456d35051e9047c15157c9a5ba217 "usb: host:
> xhci-plat: add struct xhci_plat_priv" :
>
> This patch adds struct xhci_plat_priv to simplify the code to match
> platform specific variables. For now, this patch adds a member "type" in
> the structure

that's fine but the answer doesn't exactly match my question ;-)

My point is that this enum shouldn't be necessary at all. We have
compatible flags to make these checks instead. How about below ?
(untested, uncompiled, yada yada yada). Note that we DON'T need this
xhci_plat_type trickery, just need to be a little bit smarter about how
we use driver_data:

diff --git a/drivers/usb/host/xhci-mvebu.c b/drivers/usb/host/xhci-mvebu.c
index 1eefc988192d..1ea6c18b74f3 100644
--- a/drivers/usb/host/xhci-mvebu.c
+++ b/drivers/usb/host/xhci-mvebu.c
@@ -41,8 +41,9 @@ static void xhci_mvebu_mbus_config(void __iomem *base,
 	}
 }
 
-int xhci_mvebu_mbus_init_quirk(struct platform_device *pdev)
+int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd)
 {
+	struct platform_device *pdev = to_platform_device(hcd->self.controller);
 	struct resource	*res;
 	void __iomem *base;
 	const struct mbus_dram_target_info *dram;
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 5c15e9bc5f7a..adb77c60a9ae 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -47,43 +47,56 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 	xhci->quirks |= XHCI_PLAT;
 }
 
+static void xhci_priv_plat_start(struct usb_hcd *hcd)
+{
+	struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
+
+	if (priv->plat_start)
+		priv->plat_start(hcd);
+}
+
+static int xhci_priv_init_quirk(struct usb_hcd *hcd)
+{
+	struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
+
+	if (!priv->init_quirk)
+		return 0;
+
+	return priv->init_quirk(hcd);
+}
+
 /* called during probe() after chip reset completes */
 static int xhci_plat_setup(struct usb_hcd *hcd)
 {
 	int ret;
 
-	if (xhci_plat_type_is(hcd, XHCI_PLAT_TYPE_RENESAS_RCAR_GEN2) ||
-	    xhci_plat_type_is(hcd, XHCI_PLAT_TYPE_RENESAS_RCAR_GEN3)) {
-		ret = xhci_rcar_init_quirk(hcd);
-		if (ret)
-			return ret;
-	}
+	ret = xhci_priv_init_quirk(hcd);
+	if (ret)
+		return ret;
 
 	return xhci_gen_setup(hcd, xhci_plat_quirks);
 }
 
 static int xhci_plat_start(struct usb_hcd *hcd)
 {
-	if (xhci_plat_type_is(hcd, XHCI_PLAT_TYPE_RENESAS_RCAR_GEN2) ||
-	    xhci_plat_type_is(hcd, XHCI_PLAT_TYPE_RENESAS_RCAR_GEN3))
-		xhci_rcar_start(hcd);
-
+	xhci_priv_plat_start(hcd);
 	return xhci_run(hcd);
 }
 
 #ifdef CONFIG_OF
 static const struct xhci_plat_priv xhci_plat_marvell_armada = {
-	.type = XHCI_PLAT_TYPE_MARVELL_ARMADA,
+	.init_quirk = xhci_mvebu_mbus_init_quirk,
 };
 
 static const struct xhci_plat_priv xhci_plat_renesas_rcar_gen2 = {
-	.type = XHCI_PLAT_TYPE_RENESAS_RCAR_GEN2,
 	.firmware_name = XHCI_RCAR_FIRMWARE_NAME_V1,
+	.init_quirk = xhci_rcar_init_quirk,
 };
 
 static const struct xhci_plat_priv xhci_plat_renesas_rcar_gen3 = {
-	.type = XHCI_PLAT_TYPE_RENESAS_RCAR_GEN3,
 	.firmware_name = XHCI_RCAR_FIRMWARE_NAME_V2,
+	.init_quirk = xhci_rcar_init_quirk,
+	.plat_start = xhci_rcar_start,
 };
 
 static const struct of_device_id usb_xhci_of_match[] = {
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
index 5a2e2e3936c4..c4d565980832 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/drivers/usb/host/xhci-plat.h
@@ -13,27 +13,13 @@
 
 #include "xhci.h"	/* for hcd_to_xhci() */
 
-enum xhci_plat_type {
-	XHCI_PLAT_TYPE_MARVELL_ARMADA,
-	XHCI_PLAT_TYPE_RENESAS_RCAR_GEN2,
-	XHCI_PLAT_TYPE_RENESAS_RCAR_GEN3,
-};
-
 struct xhci_plat_priv {
 	enum xhci_plat_type type;
 	const char *firmware_name;
+	void (*plat_start)(struct usb_hcd *);
+	int (*init_quirk)(struct usb_hcd *);
 };
 
 #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv)
 
-static inline bool xhci_plat_type_is(struct usb_hcd *hcd,
-				     enum xhci_plat_type type)
-{
-	struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
-
-	if (priv && priv->type == type)
-		return true;
-	else
-		return false;
-}
 #endif	/* _XHCI_PLAT_H */

ps: there might be bugs there, but it's a holiday and I really shouldn't
be spending time on this right now ;-)

Anyway, have fun testing. Let me know if it doesn't work.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]