Hi Felipe,
On Fri, 25 Mar 2016, Felipe Balbi wrote:
>
> Hi,
>
> Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx> writes:
> >> Peter Griffin <peter.griffin@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> >>> Cc: gregory.clement@xxxxxxxxxxxxxxxxxx
> >>> Cc: yoshihiro.shimoda.uh@xxxxxxxxxxx
> >>> Signed-off-by: Peter Griffin <peter.griffin@xxxxxxxxxx>
> >>> ---
> >>> 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:
Your solution certainly looks more elegant.
>
> 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 ;-)
I'm also off on holiday now until Sunday 10th April... yay :-)
>
> Anyway, have fun testing. Let me know if it doesn't work.
I only have access to STi platforms which were broken by this change.
Not any of the platforms which rely on the functionality which
was introduced (although I can't see any reason why your patch wouldn't work).
Maybe Yoshihiro (on CC) could test this on the Renesas platforms and confirm?
regards,
Peter.
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html