Re: [PATCH v4 2/5] usb: phy: Workaround for USB erratum-A005728

[Greg Kroah-Hartman]

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