Re: [PATCH v4 2/5] usb: phy: Workaround for USB erratum-A005728
From: Greg Kroah-Hartman
Date: Wed May 08 2019 - 02:42:40 EST
On Wed, May 08, 2019 at 03:26:09AM +0000, Yinbo Zhu wrote:
>
>
> > -----Original Message-----
> > From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx]
> > Sent: 2019å1æ28æ 23:37
> > To: Yinbo Zhu <yinbo.zhu@xxxxxxx>
> > Cc: Xiaobo Xie <xiaobo.xie@xxxxxxx>; Jerry Huang <jerry.huang@xxxxxxx>;
> > Ran Wang <ran.wang_1@xxxxxxx>; Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx>; Ramneek Mehresh
> > <ramneek.mehresh@xxxxxxxxxxxxx>; Nikhil Badola
> > <nikhil.badola@xxxxxxxxxxxxx>; Suresh Gupta <suresh.gupta@xxxxxxxxxxxxx>;
> > linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Suresh Gupta
> > <B42813@xxxxxxxxxxxxx>
> > Subject: Re: [PATCH v4 2/5] usb: phy: Workaround for USB erratum-A005728
> >
> > On Fri, 25 Jan 2019, Yinbo Zhu wrote:
> >
> > > From: Suresh Gupta <B42813@xxxxxxxxxxxxx>
> > >
> > > PHY_CLK_VALID bit for UTMI PHY in USBDR does not set even if PHY is
> > > providing valid clock. Workaround for this involves resetting of PHY
> > > and check PHY_CLK_VALID bit multiple times. If PHY_CLK_VALID bit is
> > > still not set even after 5 retries, it would be safe to deaclare that
> > > PHY clock is not available.
> > > This erratum is applicable for USBDR less then ver 2.4.
> > >
> > > Signed-off-by: Suresh Gupta <B42813@xxxxxxxxxxxxx>
> > > Signed-off-by: Yinbo Zhu <yinbo.zhu@xxxxxxx>
> > > ---
> > > Change in v4:
> > > Incorrect indentation of the continuation line.
> > > replace pr_err with dev_err.
> > >
> > > drivers/usb/host/ehci-fsl.c | 38
> > +++++++++++++++++++++++++++-----------
> > > drivers/usb/host/ehci-fsl.h | 3 +++
> > > 2 files changed, 30 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> > > index 38674b7..373a816 100644
> > > --- a/drivers/usb/host/ehci-fsl.c
> > > +++ b/drivers/usb/host/ehci-fsl.c
> > > @@ -183,6 +183,17 @@ static int fsl_ehci_drv_probe(struct platform_device
> > *pdev)
> > > return retval;
> > > }
> > >
> > > +static bool usb_phy_clk_valid(struct usb_hcd *hcd) {
> > > + void __iomem *non_ehci = hcd->regs;
> > > + bool ret = true;
> > > +
> > > + if (!(ioread32be(non_ehci + FSL_SOC_USB_CTRL) & PHY_CLK_VALID))
> > > + ret = false;
> > > +
> > > + return ret;
> > > +}
> > > +
> > > static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
> > > enum fsl_usb2_phy_modes phy_mode,
> > > unsigned int port_offset)
> > > @@ -226,6 +237,17 @@ static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
> > > /* fall through */
> > > case FSL_USB2_PHY_UTMI:
> > > case FSL_USB2_PHY_UTMI_DUAL:
> > > + /* PHY_CLK_VALID bit is de-featured from all controller
> > > + * versions below 2.4 and is to be checked only for
> > > + * internal UTMI phy
> > > + */
> > > + if (pdata->controller_ver > FSL_USB_VER_2_4 &&
> > > + pdata->have_sysif_regs && !usb_phy_clk_valid(hcd)) {
> > > + dev_err(dev,
> > > + "%s: USB PHY clock invalid\n", dev_name(dev));
> >
> > This looks silly; it prints the device name twice (once because that's what
> > dev_err() does, and then again because you explicitly told it to print the device
> > name).
> >
> > Look at how dev_err() is used in other parts of the driver and do the same thing.
> >
> > > + return -EINVAL;
> > > + }
> > > +
> > > if (pdata->have_sysif_regs && pdata->controller_ver) {
> > > /* controller version 1.6 or above */
> > > tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL); @@ -249,17
> > +271,11
> > > @@ static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
> > > break;
> > > }
> > >
> > > - /*
> > > - * check PHY_CLK_VALID to determine phy clock presence before writing
> > > - * to portsc
> > > - */
> > > - if (pdata->check_phy_clk_valid) {
> > > - if (!(ioread32be(non_ehci + FSL_SOC_USB_CTRL) &
> > > - PHY_CLK_VALID)) {
> > > - dev_warn(hcd->self.controller,
> > > - "USB PHY clock invalid\n");
> > > - return -EINVAL;
> > > - }
> > > + if (pdata->have_sysif_regs &&
> > > + pdata->controller_ver > FSL_USB_VER_1_6 &&
> > > + !usb_phy_clk_valid(hcd)) {
> > > + dev_warn(hcd->self.controller, "USB PHY clock invalid\n");
> >
> > Once again, you have a continuation line that is indented by the same amount as
> > the code in the inner block. Please fix this properly.
> Hi Alan stern,
>
> Above code indented is in oder to ensure that every line less than 80 characters,
> Otherwise, check-patch tool to check patch that will has error.
No, that's is not what Alan is asking you to do here. Please fix this
up to match other multi-line if statements.
greg k-h