Patchwork [2/3] usb: ehci: fsl: Update register accessing for arm/arm64 platforms

login
register
mail settings
Submitter Ran Wang
Date Jan. 8, 2019, 6:04 a.m.
Message ID <20190108060426.11581-2-ran.wang_1@nxp.com>
Download mbox | patch
Permalink /patch/694425/
State New
Headers show

Comments

Ran Wang - Jan. 8, 2019, 6:04 a.m.
arm/arm64's io.h doesn't define clrbits32() and clrsetbits_be32(), which
causing compile failure on some Layerscape Platforms (such as LS1021A and
LS2012A which also integrates FSL EHCI controller). So use
ioread32be()/iowrite32be() instead to make it workable on both
powerpc and arm.

Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
---
 drivers/usb/host/ehci-fsl.c |   64 ++++++++++++++++++++++++++++---------------
 1 files changed, 42 insertions(+), 22 deletions(-)
Greg Kroah-Hartman - Jan. 8, 2019, 3:45 p.m.
On Tue, Jan 08, 2019 at 06:04:26AM +0000, Ran Wang wrote:
> arm/arm64's io.h doesn't define clrbits32() and clrsetbits_be32(), which
> causing compile failure on some Layerscape Platforms (such as LS1021A and
> LS2012A which also integrates FSL EHCI controller). So use
> ioread32be()/iowrite32be() instead to make it workable on both
> powerpc and arm.

Shouldn't you have this patch before the Kconfig change, so that you do
not break the build on ARM systems on that one and then fix it up on
this one?

thanks,

greg k-h
Alan Stern - Jan. 8, 2019, 4:20 p.m.
On Tue, 8 Jan 2019, Ran Wang wrote:

> arm/arm64's io.h doesn't define clrbits32() and clrsetbits_be32(), which
> causing compile failure on some Layerscape Platforms (such as LS1021A and
> LS2012A which also integrates FSL EHCI controller). So use
> ioread32be()/iowrite32be() instead to make it workable on both
> powerpc and arm.
> 
> Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> ---
>  drivers/usb/host/ehci-fsl.c |   64 ++++++++++++++++++++++++++++---------------
>  1 files changed, 42 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> index 0a9fd20..59ebe1b 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -23,6 +23,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/fsl_devices.h>
>  #include <linux/of_platform.h>
> +#include <linux/io.h>
>  
>  #include "ehci.h"
>  #include "ehci-fsl.h"
> @@ -50,6 +51,7 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	int irq;
>  	int retval;
> +	u32 tmp;
>  
>  	pr_debug("initializing FSL-SOC USB Controller\n");
>  
> @@ -114,18 +116,23 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Enable USB controller, 83xx or 8536 */
> -	if (pdata->have_sysif_regs && pdata->controller_ver < FSL_USB_VER_1_6)
> -		clrsetbits_be32(hcd->regs + FSL_SOC_USB_CTRL,
> -				CONTROL_REGISTER_W1C_MASK, 0x4);
> -
> +	if (pdata->have_sysif_regs && pdata->controller_ver < FSL_USB_VER_1_6) {
> +		tmp = ioread32be(hcd->regs + FSL_SOC_USB_CTRL);
> +		tmp &= ~CONTROL_REGISTER_W1C_MASK;
> +		tmp |= 0x4;
> +		iowrite32be(tmp, hcd->regs + FSL_SOC_USB_CTRL);
> +	}
>  	/*
>  	 * Enable UTMI phy and program PTS field in UTMI mode before asserting
>  	 * controller reset for USB Controller version 2.5
>  	 */
>  	if (pdata->has_fsl_erratum_a007792) {
> -		clrsetbits_be32(hcd->regs + FSL_SOC_USB_CTRL,
> -				CONTROL_REGISTER_W1C_MASK, CTRL_UTMI_PHY_EN);
> -		writel(PORT_PTS_UTMI, hcd->regs + FSL_SOC_USB_PORTSC1);
> +		tmp = ioread32be(hcd->regs + FSL_SOC_USB_CTRL);
> +		tmp &= ~CONTROL_REGISTER_W1C_MASK;
> +		tmp |= CTRL_UTMI_PHY_EN;
> +		iowrite32be(tmp, hcd->regs + FSL_SOC_USB_CTRL);
> +
> +		iowrite32be(PORT_PTS_UTMI, hcd->regs + FSL_SOC_USB_PORTSC1);

Why do you change this writel() into iowrite32be() but leave other 
instances of writel() unchanged?  Was this a mistake?

>  	}
>  
>  	/* Don't need to set host mode here. It will be done by tdi_reset() */
> @@ -174,7 +181,7 @@ static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
>  			       enum fsl_usb2_phy_modes phy_mode,
>  			       unsigned int port_offset)
>  {
> -	u32 portsc;
> +	u32 portsc, tmp;
>  	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
>  	void __iomem *non_ehci = hcd->regs;
>  	struct device *dev = hcd->self.controller;
> @@ -192,11 +199,16 @@ static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
>  	case FSL_USB2_PHY_ULPI:
>  		if (pdata->have_sysif_regs && pdata->controller_ver) {
>  			/* controller version 1.6 or above */
> -			clrbits32(non_ehci + FSL_SOC_USB_CTRL,
> -				  CONTROL_REGISTER_W1C_MASK | UTMI_PHY_EN);
> -			clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
> -					CONTROL_REGISTER_W1C_MASK,
> -					ULPI_PHY_CLK_SEL | USB_CTRL_USB_EN);
> +			/* turn off UTMI PHY first */
> +			tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> +			tmp &= ~(CONTROL_REGISTER_W1C_MASK | UTMI_PHY_EN);
> +			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
> +
> +			/* then turn on ULPI and enable USB controller */
> +			tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> +			tmp &= ~(CONTROL_REGISTER_W1C_MASK);

Unnecessary parens.

> +			tmp |= ULPI_PHY_CLK_SEL | USB_CTRL_USB_EN;
> +			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
>  		}
>  		portsc |= PORT_PTS_ULPI;
>  		break;
> @@ -210,16 +222,21 @@ static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
>  	case FSL_USB2_PHY_UTMI_DUAL:
>  		if (pdata->have_sysif_regs && pdata->controller_ver) {
>  			/* controller version 1.6 or above */
> -			clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
> -					CONTROL_REGISTER_W1C_MASK, UTMI_PHY_EN);
> +			tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> +			tmp &= ~(CONTROL_REGISTER_W1C_MASK);

Unnecessary parens.

> +			tmp |= UTMI_PHY_EN;
> +			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
> +
>  			mdelay(FSL_UTMI_PHY_DLY);  /* Delay for UTMI PHY CLK to
>  						become stable - 10ms*/
>  		}
>  		/* enable UTMI PHY */
> -		if (pdata->have_sysif_regs)
> -			clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
> -					CONTROL_REGISTER_W1C_MASK,
> -					CTRL_UTMI_PHY_EN);
> +		if (pdata->have_sysif_regs) {
> +			tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> +			tmp &= ~CONTROL_REGISTER_W1C_MASK;
> +			tmp |= CTRL_UTMI_PHY_EN;
> +			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
> +		}
>  		portsc |= PORT_PTS_UTMI;
>  		break;
>  	case FSL_USB2_PHY_NONE:
> @@ -241,9 +258,12 @@ static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
>  
>  	ehci_writel(ehci, portsc, &ehci->regs->port_status[port_offset]);
>  
> -	if (phy_mode != FSL_USB2_PHY_ULPI && pdata->have_sysif_regs)
> -		clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
> -				CONTROL_REGISTER_W1C_MASK, USB_CTRL_USB_EN);
> +	if (phy_mode != FSL_USB2_PHY_ULPI && pdata->have_sysif_regs) {
> +		tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> +		tmp &= ~CONTROL_REGISTER_W1C_MASK;
> +		tmp |= USB_CTRL_USB_EN;
> +		iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
> +	}
>  
>  	return 0;
>  }

Alan Stern
Ran Wang - Jan. 9, 2019, 3:37 a.m.
Hi Alan,

> -----Original Message-----
> From: Alan Stern <stern@rowland.harvard.edu>
> Sent: Wednesday, January 09, 2019 00:20
> To: Ran Wang <ran.wang_1@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> usb@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/3] usb: ehci: fsl: Update register accessing for
> arm/arm64 platforms
> 
> On Tue, 8 Jan 2019, Ran Wang wrote:
> 
> > arm/arm64's io.h doesn't define clrbits32() and clrsetbits_be32(),
> > which causing compile failure on some Layerscape Platforms (such as
> > LS1021A and LS2012A which also integrates FSL EHCI controller). So use
> > ioread32be()/iowrite32be() instead to make it workable on both powerpc
> > and arm.
> >
> > Signed-off-by: Ran Wang <ran.wang_1@nxp.com>
> > ---
> >  drivers/usb/host/ehci-fsl.c |   64 ++++++++++++++++++++++++++++------------
> ---
> >  1 files changed, 42 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> > index 0a9fd20..59ebe1b 100644
> > --- a/drivers/usb/host/ehci-fsl.c
> > +++ b/drivers/usb/host/ehci-fsl.c
> > @@ -23,6 +23,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/fsl_devices.h>
> >  #include <linux/of_platform.h>
> > +#include <linux/io.h>
> >
> >  #include "ehci.h"
> >  #include "ehci-fsl.h"
> > @@ -50,6 +51,7 @@ static int fsl_ehci_drv_probe(struct platform_device
> *pdev)
> >  	struct resource *res;
> >  	int irq;
> >  	int retval;
> > +	u32 tmp;
> >
> >  	pr_debug("initializing FSL-SOC USB Controller\n");
> >
> > @@ -114,18 +116,23 @@ static int fsl_ehci_drv_probe(struct
> platform_device *pdev)
> >  	}
> >
> >  	/* Enable USB controller, 83xx or 8536 */
> > -	if (pdata->have_sysif_regs && pdata->controller_ver <
> FSL_USB_VER_1_6)
> > -		clrsetbits_be32(hcd->regs + FSL_SOC_USB_CTRL,
> > -				CONTROL_REGISTER_W1C_MASK, 0x4);
> > -
> > +	if (pdata->have_sysif_regs && pdata->controller_ver <
> FSL_USB_VER_1_6) {
> > +		tmp = ioread32be(hcd->regs + FSL_SOC_USB_CTRL);
> > +		tmp &= ~CONTROL_REGISTER_W1C_MASK;
> > +		tmp |= 0x4;
> > +		iowrite32be(tmp, hcd->regs + FSL_SOC_USB_CTRL);
> > +	}
> >  	/*
> >  	 * Enable UTMI phy and program PTS field in UTMI mode before
> asserting
> >  	 * controller reset for USB Controller version 2.5
> >  	 */
> >  	if (pdata->has_fsl_erratum_a007792) {
> > -		clrsetbits_be32(hcd->regs + FSL_SOC_USB_CTRL,
> > -				CONTROL_REGISTER_W1C_MASK,
> CTRL_UTMI_PHY_EN);
> > -		writel(PORT_PTS_UTMI, hcd->regs + FSL_SOC_USB_PORTSC1);
> > +		tmp = ioread32be(hcd->regs + FSL_SOC_USB_CTRL);
> > +		tmp &= ~CONTROL_REGISTER_W1C_MASK;
> > +		tmp |= CTRL_UTMI_PHY_EN;
> > +		iowrite32be(tmp, hcd->regs + FSL_SOC_USB_CTRL);
> > +
> > +		iowrite32be(PORT_PTS_UTMI, hcd->regs +
> FSL_SOC_USB_PORTSC1);
> 
> Why do you change this writel() into iowrite32be() but leave other instances
> of writel() unchanged?  Was this a mistake?

Yes, I didn't notice there are other writel() used in this file.
However, as I know, on both powerpc and arm SoC, EHCI FSL IP's memory mapped
register block is always Big-endian, so I'd like to replace all writel() with iowrite32be()
in this file. Is it necessary? 

Or I just replace them with ehci_writel() and select CONFIG_USB_EHCI_BIG_ENDIAN_MMIO?

> >  	}
> >
> >  	/* Don't need to set host mode here. It will be done by tdi_reset()
> > */ @@ -174,7 +181,7 @@ static int ehci_fsl_setup_phy(struct usb_hcd
> *hcd,
> >  			       enum fsl_usb2_phy_modes phy_mode,
> >  			       unsigned int port_offset)
> >  {
> > -	u32 portsc;
> > +	u32 portsc, tmp;
> >  	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> >  	void __iomem *non_ehci = hcd->regs;
> >  	struct device *dev = hcd->self.controller; @@ -192,11 +199,16 @@
> > static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
> >  	case FSL_USB2_PHY_ULPI:
> >  		if (pdata->have_sysif_regs && pdata->controller_ver) {
> >  			/* controller version 1.6 or above */
> > -			clrbits32(non_ehci + FSL_SOC_USB_CTRL,
> > -				  CONTROL_REGISTER_W1C_MASK |
> UTMI_PHY_EN);
> > -			clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
> > -					CONTROL_REGISTER_W1C_MASK,
> > -					ULPI_PHY_CLK_SEL |
> USB_CTRL_USB_EN);
> > +			/* turn off UTMI PHY first */
> > +			tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> > +			tmp &= ~(CONTROL_REGISTER_W1C_MASK |
> UTMI_PHY_EN);
> > +			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
> > +
> > +			/* then turn on ULPI and enable USB controller */
> > +			tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> > +			tmp &= ~(CONTROL_REGISTER_W1C_MASK);
> 
> Unnecessary parens.

Got it, fix it in next version.

> > +			tmp |= ULPI_PHY_CLK_SEL | USB_CTRL_USB_EN;
> > +			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
> >  		}
> >  		portsc |= PORT_PTS_ULPI;
> >  		break;
> > @@ -210,16 +222,21 @@ static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
> >  	case FSL_USB2_PHY_UTMI_DUAL:
> >  		if (pdata->have_sysif_regs && pdata->controller_ver) {
> >  			/* controller version 1.6 or above */
> > -			clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
> > -					CONTROL_REGISTER_W1C_MASK,
> UTMI_PHY_EN);
> > +			tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> > +			tmp &= ~(CONTROL_REGISTER_W1C_MASK);
> 
> Unnecessary parens.

Got it.

Regards,
Ran
> > +			tmp |= UTMI_PHY_EN;
> > +			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
> > +
> >  			mdelay(FSL_UTMI_PHY_DLY);  /* Delay for UTMI PHY
> CLK to
> >  						become stable - 10ms*/
> >  		}
> >  		/* enable UTMI PHY */
> > -		if (pdata->have_sysif_regs)
> > -			clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
> > -					CONTROL_REGISTER_W1C_MASK,
> > -					CTRL_UTMI_PHY_EN);
> > +		if (pdata->have_sysif_regs) {
> > +			tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> > +			tmp &= ~CONTROL_REGISTER_W1C_MASK;
> > +			tmp |= CTRL_UTMI_PHY_EN;
> > +			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
> > +		}
> >  		portsc |= PORT_PTS_UTMI;
> >  		break;
> >  	case FSL_USB2_PHY_NONE:
> > @@ -241,9 +258,12 @@ static int ehci_fsl_setup_phy(struct usb_hcd
> > *hcd,
> >
> >  	ehci_writel(ehci, portsc, &ehci->regs->port_status[port_offset]);
> >
> > -	if (phy_mode != FSL_USB2_PHY_ULPI && pdata->have_sysif_regs)
> > -		clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
> > -				CONTROL_REGISTER_W1C_MASK,
> USB_CTRL_USB_EN);
> > +	if (phy_mode != FSL_USB2_PHY_ULPI && pdata->have_sysif_regs) {
> > +		tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> > +		tmp &= ~CONTROL_REGISTER_W1C_MASK;
> > +		tmp |= USB_CTRL_USB_EN;
> > +		iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
> > +	}
> >
> >  	return 0;
> >  }
> 
> Alan Stern
Alan Stern - Jan. 9, 2019, 3:14 p.m.
On Wed, 9 Jan 2019, Ran Wang wrote:

> > Why do you change this writel() into iowrite32be() but leave other instances
> > of writel() unchanged?  Was this a mistake?
> 
> Yes, I didn't notice there are other writel() used in this file.
> However, as I know, on both powerpc and arm SoC, EHCI FSL IP's memory mapped
> register block is always Big-endian, so I'd like to replace all writel() with iowrite32be()
> in this file. Is it necessary? 
> 
> Or I just replace them with ehci_writel() and select CONFIG_USB_EHCI_BIG_ENDIAN_MMIO?

That should work okay.  ehci_fsl_setup() sets ehci->big_endian_desc and
ehci->big_endian_mmio according to the platform data, so you just have
to make sure the platform data is initialized correctly.

Alan Stern
Ran Wang - Jan. 10, 2019, 3:41 a.m.
Hi Alan,

> -----Original Message-----
> From: Alan Stern <stern@rowland.harvard.edu>
> Sent: Wednesday, January 09, 2019 23:14
> To: Ran Wang <ran.wang_1@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> usb@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH 2/3] usb: ehci: fsl: Update register accessing for
> arm/arm64 platforms
> 
> On Wed, 9 Jan 2019, Ran Wang wrote:
> 
> > > Why do you change this writel() into iowrite32be() but leave other
> > > instances of writel() unchanged?  Was this a mistake?
> >
> > Yes, I didn't notice there are other writel() used in this file.
> > However, as I know, on both powerpc and arm SoC, EHCI FSL IP's memory
> > mapped register block is always Big-endian, so I'd like to replace all
> > writel() with iowrite32be() in this file. Is it necessary?
> >
> > Or I just replace them with ehci_writel() and select
> CONFIG_USB_EHCI_BIG_ENDIAN_MMIO?
> 
> That should work okay.  ehci_fsl_setup() sets ehci->big_endian_desc and
> ehci->big_endian_mmio according to the platform data, so you just have
> to make sure the platform data is initialized correctly.
>
OK, so I should not change writel() into iowrite32be() at that
place you mentioned, and still using iowrite32be()/ioread32be() to replace
clrsetbits_be32(), am I right?

Regards,
Ran

> Alan Stern
Alan Stern - Jan. 10, 2019, 3:29 p.m.
On Thu, 10 Jan 2019, Ran Wang wrote:

> Hi Alan,
> 
> > -----Original Message-----
> > From: Alan Stern <stern@rowland.harvard.edu>
> > Sent: Wednesday, January 09, 2019 23:14
> > To: Ran Wang <ran.wang_1@nxp.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> > usb@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH 2/3] usb: ehci: fsl: Update register accessing for
> > arm/arm64 platforms
> > 
> > On Wed, 9 Jan 2019, Ran Wang wrote:
> > 
> > > > Why do you change this writel() into iowrite32be() but leave other
> > > > instances of writel() unchanged?  Was this a mistake?
> > >
> > > Yes, I didn't notice there are other writel() used in this file.
> > > However, as I know, on both powerpc and arm SoC, EHCI FSL IP's memory
> > > mapped register block is always Big-endian, so I'd like to replace all
> > > writel() with iowrite32be() in this file. Is it necessary?
> > >
> > > Or I just replace them with ehci_writel() and select
> > CONFIG_USB_EHCI_BIG_ENDIAN_MMIO?
> > 
> > That should work okay.  ehci_fsl_setup() sets ehci->big_endian_desc and
> > ehci->big_endian_mmio according to the platform data, so you just have
> > to make sure the platform data is initialized correctly.
> >
> OK, so I should not change writel() into iowrite32be() at that
> place you mentioned, and still using iowrite32be()/ioread32be() to replace
> clrsetbits_be32(), am I right?

Actually, I think it would be good to use ehci_writel() and
ehci_readl() everywhere except in fsl_ehci_drv_probe().

But if you prefer to use iowrite32be() and ioread32be() instead, that's
probably okay.  (However, it won't work if anyone ever produces a
little-endian version of the IP.)

Alan Stern
Ran Wang - Jan. 11, 2019, 9:10 a.m.
Hi Alan,

On Thursday, January 10, 2019, Alan wrote:
> 
> On Thu, 10 Jan 2019, Ran Wang wrote:
> 
> > Hi Alan,
> >
> > > -----Original Message-----
> > > From: Alan Stern <stern@rowland.harvard.edu>
> > > Sent: Wednesday, January 09, 2019 23:14
> > > To: Ran Wang <ran.wang_1@nxp.com>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> > > usb@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: RE: [PATCH 2/3] usb: ehci: fsl: Update register accessing
> > > for
> > > arm/arm64 platforms
> > >
> > > On Wed, 9 Jan 2019, Ran Wang wrote:
> > >
> > > > > Why do you change this writel() into iowrite32be() but leave
> > > > > other instances of writel() unchanged?  Was this a mistake?
> > > >
> > > > Yes, I didn't notice there are other writel() used in this file.
> > > > However, as I know, on both powerpc and arm SoC, EHCI FSL IP's
> > > > memory mapped register block is always Big-endian, so I'd like to
> > > > replace all
> > > > writel() with iowrite32be() in this file. Is it necessary?
> > > >
> > > > Or I just replace them with ehci_writel() and select
> > > CONFIG_USB_EHCI_BIG_ENDIAN_MMIO?
> > >
> > > That should work okay.  ehci_fsl_setup() sets ehci->big_endian_desc
> > > and
> > > ehci->big_endian_mmio according to the platform data, so you just
> > > ehci->have
> > > to make sure the platform data is initialized correctly.
> > >
> > OK, so I should not change writel() into iowrite32be() at that place
> > you mentioned, and still using iowrite32be()/ioread32be() to replace
> > clrsetbits_be32(), am I right?
> 
> Actually, I think it would be good to use ehci_writel() and
> ehci_readl() everywhere except in fsl_ehci_drv_probe().
> 
> But if you prefer to use iowrite32be() and ioread32be() instead, that's
> probably okay.  (However, it won't work if anyone ever produces a little-
> endian version of the IP.)

Got it, I'll verify with ehci_readl() then work out the v2 patch if pass.
Thanks for advice.

Regards,
Ran
Ran Wang - Jan. 17, 2019, 8:06 a.m.
Hi Greg,

On 08, 2019 23:45, Greg Kroah-Hartman wrote:
> 
> On Tue, Jan 08, 2019 at 06:04:26AM +0000, Ran Wang wrote:
> > arm/arm64's io.h doesn't define clrbits32() and clrsetbits_be32(),
> > which causing compile failure on some Layerscape Platforms (such as
> > LS1021A and LS2012A which also integrates FSL EHCI controller). So use
> > ioread32be()/iowrite32be() instead to make it workable on both powerpc
> > and arm.
> 
> Shouldn't you have this patch before the Kconfig change, so that you do not
> break the build on ARM systems on that one and then fix it up on this one?

Yes, you are right, will adjust it in next version, thanks for pointing out.

Regards,
Ran

Patch

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 0a9fd20..59ebe1b 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -23,6 +23,7 @@ 
 #include <linux/platform_device.h>
 #include <linux/fsl_devices.h>
 #include <linux/of_platform.h>
+#include <linux/io.h>
 
 #include "ehci.h"
 #include "ehci-fsl.h"
@@ -50,6 +51,7 @@  static int fsl_ehci_drv_probe(struct platform_device *pdev)
 	struct resource *res;
 	int irq;
 	int retval;
+	u32 tmp;
 
 	pr_debug("initializing FSL-SOC USB Controller\n");
 
@@ -114,18 +116,23 @@  static int fsl_ehci_drv_probe(struct platform_device *pdev)
 	}
 
 	/* Enable USB controller, 83xx or 8536 */
-	if (pdata->have_sysif_regs && pdata->controller_ver < FSL_USB_VER_1_6)
-		clrsetbits_be32(hcd->regs + FSL_SOC_USB_CTRL,
-				CONTROL_REGISTER_W1C_MASK, 0x4);
-
+	if (pdata->have_sysif_regs && pdata->controller_ver < FSL_USB_VER_1_6) {
+		tmp = ioread32be(hcd->regs + FSL_SOC_USB_CTRL);
+		tmp &= ~CONTROL_REGISTER_W1C_MASK;
+		tmp |= 0x4;
+		iowrite32be(tmp, hcd->regs + FSL_SOC_USB_CTRL);
+	}
 	/*
 	 * Enable UTMI phy and program PTS field in UTMI mode before asserting
 	 * controller reset for USB Controller version 2.5
 	 */
 	if (pdata->has_fsl_erratum_a007792) {
-		clrsetbits_be32(hcd->regs + FSL_SOC_USB_CTRL,
-				CONTROL_REGISTER_W1C_MASK, CTRL_UTMI_PHY_EN);
-		writel(PORT_PTS_UTMI, hcd->regs + FSL_SOC_USB_PORTSC1);
+		tmp = ioread32be(hcd->regs + FSL_SOC_USB_CTRL);
+		tmp &= ~CONTROL_REGISTER_W1C_MASK;
+		tmp |= CTRL_UTMI_PHY_EN;
+		iowrite32be(tmp, hcd->regs + FSL_SOC_USB_CTRL);
+
+		iowrite32be(PORT_PTS_UTMI, hcd->regs + FSL_SOC_USB_PORTSC1);
 	}
 
 	/* Don't need to set host mode here. It will be done by tdi_reset() */
@@ -174,7 +181,7 @@  static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
 			       enum fsl_usb2_phy_modes phy_mode,
 			       unsigned int port_offset)
 {
-	u32 portsc;
+	u32 portsc, tmp;
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
 	void __iomem *non_ehci = hcd->regs;
 	struct device *dev = hcd->self.controller;
@@ -192,11 +199,16 @@  static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
 	case FSL_USB2_PHY_ULPI:
 		if (pdata->have_sysif_regs && pdata->controller_ver) {
 			/* controller version 1.6 or above */
-			clrbits32(non_ehci + FSL_SOC_USB_CTRL,
-				  CONTROL_REGISTER_W1C_MASK | UTMI_PHY_EN);
-			clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
-					CONTROL_REGISTER_W1C_MASK,
-					ULPI_PHY_CLK_SEL | USB_CTRL_USB_EN);
+			/* turn off UTMI PHY first */
+			tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
+			tmp &= ~(CONTROL_REGISTER_W1C_MASK | UTMI_PHY_EN);
+			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
+
+			/* then turn on ULPI and enable USB controller */
+			tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
+			tmp &= ~(CONTROL_REGISTER_W1C_MASK);
+			tmp |= ULPI_PHY_CLK_SEL | USB_CTRL_USB_EN;
+			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
 		}
 		portsc |= PORT_PTS_ULPI;
 		break;
@@ -210,16 +222,21 @@  static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
 	case FSL_USB2_PHY_UTMI_DUAL:
 		if (pdata->have_sysif_regs && pdata->controller_ver) {
 			/* controller version 1.6 or above */
-			clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
-					CONTROL_REGISTER_W1C_MASK, UTMI_PHY_EN);
+			tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
+			tmp &= ~(CONTROL_REGISTER_W1C_MASK);
+			tmp |= UTMI_PHY_EN;
+			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
+
 			mdelay(FSL_UTMI_PHY_DLY);  /* Delay for UTMI PHY CLK to
 						become stable - 10ms*/
 		}
 		/* enable UTMI PHY */
-		if (pdata->have_sysif_regs)
-			clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
-					CONTROL_REGISTER_W1C_MASK,
-					CTRL_UTMI_PHY_EN);
+		if (pdata->have_sysif_regs) {
+			tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
+			tmp &= ~CONTROL_REGISTER_W1C_MASK;
+			tmp |= CTRL_UTMI_PHY_EN;
+			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
+		}
 		portsc |= PORT_PTS_UTMI;
 		break;
 	case FSL_USB2_PHY_NONE:
@@ -241,9 +258,12 @@  static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
 
 	ehci_writel(ehci, portsc, &ehci->regs->port_status[port_offset]);
 
-	if (phy_mode != FSL_USB2_PHY_ULPI && pdata->have_sysif_regs)
-		clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
-				CONTROL_REGISTER_W1C_MASK, USB_CTRL_USB_EN);
+	if (phy_mode != FSL_USB2_PHY_ULPI && pdata->have_sysif_regs) {
+		tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
+		tmp &= ~CONTROL_REGISTER_W1C_MASK;
+		tmp |= USB_CTRL_USB_EN;
+		iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
+	}
 
 	return 0;
 }