Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753056AbcCZKKz (ORCPT ); Sat, 26 Mar 2016 06:10:55 -0400 Received: from mail-lf0-f66.google.com ([209.85.215.66]:33643 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751712AbcCZKKr (ORCPT ); Sat, 26 Mar 2016 06:10:47 -0400 From: Felipe Balbi To: Peter Griffin Cc: Gregory CLEMENT , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, mathias.nyman@intel.com, gregkh@linuxfoundation.org, lee.jones@linaro.org, linux-usb@vger.kernel.org, maxime.coquelin@st.com, patrice.chotard@st.com, stable@vger.kernel.org, yoshihiro.shimoda.uh@renesas.com, felipe.balbi@linux.intel.com Subject: Re: [PATCH] usb: host: xhci-plat: Make enum xhci_plat_type start at a non zero value In-Reply-To: <20160326091050.GA7793@griffinp-ThinkPad-X1-Carbon-2nd> References: <1458917188-28452-1-git-send-email-peter.griffin@linaro.org> <87mvpm8pfl.fsf@ti.com> <87mvpmlc5e.fsf@free-electrons.com> <87io0a8nbd.fsf@ti.com> <20160326091050.GA7793@griffinp-ThinkPad-X1-Carbon-2nd> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/25.0.91.3 (x86_64-pc-linux-gnu) Date: Sat, 26 Mar 2016 12:10:39 +0200 Message-ID: <87fuvd8s80.fsf@ti.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8309 Lines: 252 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Peter Griffin writes: > Hi Felipe, > > On Fri, 25 Mar 2016, Felipe Balbi wrote: > >>=20 >> Hi, >>=20 >> Gregory CLEMENT writes: >> >> Peter Griffin writes: >> >>> Otherwise generic-xhci and xhci-platform which have no data get wron= gly >> >>> 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 t= he >> >>> 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_pri= v") >> >>> Cc: stable@vger.kernel.org >> >>> Cc: gregory.clement@free-electrons.com >> >>> Cc: yoshihiro.shimoda.uh@renesas.com >> >>> Signed-off-by: Peter Griffin >> >>> --- >> >>> 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-pl= at.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() */ >> >>>=20=20 >> >>> enum xhci_plat_type { >> >>> - XHCI_PLAT_TYPE_MARVELL_ARMADA, >> >>> + XHCI_PLAT_TYPE_MARVELL_ARMADA =3D 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 >>=20 >> that's fine but the answer doesn't exactly match my question ;-) >>=20 >> 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. cool thanks. Now that I think about this more carefully, we might wanna take $subject anyway for the -rc and get my version applied for v4.7 merge window. What do you think ? >> 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, >> } >> } >>=20=20 >> -int xhci_mvebu_mbus_init_quirk(struct platform_device *pdev) >> +int xhci_mvebu_mbus_init_quirk(struct usb_hcd *hcd) >> { >> + struct platform_device *pdev =3D to_platform_device(hcd->self.controll= er); >> 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, str= uct xhci_hcd *xhci) >> xhci->quirks |=3D XHCI_PLAT; >> } >>=20=20 >> +static void xhci_priv_plat_start(struct usb_hcd *hcd) >> +{ >> + struct xhci_plat_priv *priv =3D 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 =3D 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; >>=20=20 >> - if (xhci_plat_type_is(hcd, XHCI_PLAT_TYPE_RENESAS_RCAR_GEN2) || >> - xhci_plat_type_is(hcd, XHCI_PLAT_TYPE_RENESAS_RCAR_GEN3)) { >> - ret =3D xhci_rcar_init_quirk(hcd); >> - if (ret) >> - return ret; >> - } >> + ret =3D xhci_priv_init_quirk(hcd); >> + if (ret) >> + return ret; >>=20=20 >> return xhci_gen_setup(hcd, xhci_plat_quirks); >> } >>=20=20 >> 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); >> } >>=20=20 >> #ifdef CONFIG_OF >> static const struct xhci_plat_priv xhci_plat_marvell_armada =3D { >> - .type =3D XHCI_PLAT_TYPE_MARVELL_ARMADA, >> + .init_quirk =3D xhci_mvebu_mbus_init_quirk, >> }; >>=20=20 >> static const struct xhci_plat_priv xhci_plat_renesas_rcar_gen2 =3D { >> - .type =3D XHCI_PLAT_TYPE_RENESAS_RCAR_GEN2, >> .firmware_name =3D XHCI_RCAR_FIRMWARE_NAME_V1, >> + .init_quirk =3D xhci_rcar_init_quirk, >> }; >>=20=20 >> static const struct xhci_plat_priv xhci_plat_renesas_rcar_gen3 =3D { >> - .type =3D XHCI_PLAT_TYPE_RENESAS_RCAR_GEN3, >> .firmware_name =3D XHCI_RCAR_FIRMWARE_NAME_V2, >> + .init_quirk =3D xhci_rcar_init_quirk, >> + .plat_start =3D xhci_rcar_start, >> }; >>=20=20 >> static const struct of_device_id usb_xhci_of_match[] =3D { >> 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 @@ >>=20=20 >> #include "xhci.h" /* for hcd_to_xhci() */ >>=20=20 >> -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 *); >> }; >>=20=20 >> #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->p= riv) >>=20=20 >> -static inline bool xhci_plat_type_is(struct usb_hcd *hcd, >> - enum xhci_plat_type type) >> -{ >> - struct xhci_plat_priv *priv =3D hcd_to_xhci_priv(hcd); >> - >> - if (priv && priv->type =3D=3D type) >> - return true; >> - else >> - return false; >> -} >> #endif /* _XHCI_PLAT_H */ >>=20 >> 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 :-) heh, cool :-) >> 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 w= ork). > > Maybe Yoshihiro (on CC) could test this on the Renesas platforms and > confirm? sure, that would be great; then we avoid further regressions ;-) =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJW9mAfAAoJEIaOsuA1yqREnlIP/RYZPMzOtbtWzk8xdNOvMcsc sS3NOnJX46gbyeyYtUAgrFvJCmDH4oaWzRpgDK91pXO6vEBnYqaSCZqHVBDiwXMU QwySHZaCdXzUI/HFbjnHOass44OC8CQXSCRv0PIcWx6HATli4Cp4eoB4cYyoIfn2 0Y9ES1wJqVT5sILwzI176XXEJ1mMOTiUhUY3Z1N3vWif7Eh7aL5Go9V1rI42XFfB 7eCMymv15comWoaKt0d19yrBgkDcyWcFSMhMVmABVOs+E00vb6b3h9Qa25gImkQ9 tutLD7CqnQuigFug9cWA3ut24ylMWJfHTTlWPidQjdX6OlnNFU8pQ4ipH2sDjz/v EvTADKDfvBvWnEGfk0naD5R+rW5XQM8ME6/OiNCYcYexgb3D1QvUoWSnHB3pcw8B x8IopZ1pGRGWPsOvnjT8CXu7OOvdugntx0cnXOTr5TO3nq8yLQvelqC6zayir31M IJlr/kxJmK8iwDuUx3G4xcXM056Bu6O+hbKp+51FApOAhL/h5OLzV0EMlKZZtBTf wwi6uKx/QVnp79p2PqGAeckqnu7TXmwoVr6QYVHPwS9DMx0AbhpyBmYI9QfFq38X 3S1LxBa0SbsqNHF2J+PuH5GriilE+qkfGiq4nXaJRxp2VvwKpShxGBKKyxBlhHhl U5XaUcWbQA08A8PPio0o =MnOm -----END PGP SIGNATURE----- --=-=-=--