Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Ran Wang <ran.wang_1@nxp.com>
To:     Alan Stern <stern@rowland.harvard.edu>
CC:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 2/3] usb: ehci: fsl: Update register accessing for
 arm/arm64 platforms
Thread-Topic: [PATCH 2/3] usb: ehci: fsl: Update register accessing for
 arm/arm64 platforms
Thread-Index: AQHUpxgC6yBGLPIm9UWYEo0ogu0Vc6WljdKAgAC5jzA=
Date:   Wed, 9 Jan 2019 03:37:01 +0000
Message-ID: <AM5PR0402MB28654E2A6EB5914CB3C35174F18B0@AM5PR0402MB2865.eurprd04.prod.outlook.com>
References: <20190108060426.11581-2-ran.wang_1@nxp.com>
 <Pine.LNX.4.44L0.1901081118121.1355-100000@iolanthe.rowland.org>
In-Reply-To: <Pine.LNX.4.44L0.1901081118121.1355-100000@iolanthe.rowland.org>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: b711b1fe-2c4d-4007-2b38-08d675e3b79f
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Jan 2019 03:37:01.7722
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0402MB2900
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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
>=20
> On Tue, 8 Jan 2019, Ran Wang wrote:
>=20
> > 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 =3D ioread32be(hcd->regs + FSL_SOC_USB_CTRL);
> > +		tmp &=3D ~CONTROL_REGISTER_W1C_MASK;
> > +		tmp |=3D 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 =3D ioread32be(hcd->regs + FSL_SOC_USB_CTRL);
> > +		tmp &=3D ~CONTROL_REGISTER_W1C_MASK;
> > +		tmp |=3D CTRL_UTMI_PHY_EN;
> > +		iowrite32be(tmp, hcd->regs + FSL_SOC_USB_CTRL);
> > +
> > +		iowrite32be(PORT_PTS_UTMI, hcd->regs +
> FSL_SOC_USB_PORTSC1);
>=20
> Why do you change this writel() into iowrite32be() but leave other instan=
ces
> 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 mappe=
d
register block is always Big-endian, so I'd like to replace all writel() wi=
th iowrite32be()
in this file. Is it necessary?=20

Or I just replace them with ehci_writel() and select CONFIG_USB_EHCI_BIG_EN=
DIAN_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 =3D hcd_to_ehci(hcd);
> >  	void __iomem *non_ehci =3D hcd->regs;
> >  	struct device *dev =3D 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 =3D ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> > +			tmp &=3D ~(CONTROL_REGISTER_W1C_MASK |
> UTMI_PHY_EN);
> > +			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
> > +
> > +			/* then turn on ULPI and enable USB controller */
> > +			tmp =3D ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> > +			tmp &=3D ~(CONTROL_REGISTER_W1C_MASK);
>=20
> Unnecessary parens.

Got it, fix it in next version.

> > +			tmp |=3D ULPI_PHY_CLK_SEL | USB_CTRL_USB_EN;
> > +			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
> >  		}
> >  		portsc |=3D 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 =3D ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> > +			tmp &=3D ~(CONTROL_REGISTER_W1C_MASK);
>=20
> Unnecessary parens.

Got it.

Regards,
Ran
> > +			tmp |=3D 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 =3D ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> > +			tmp &=3D ~CONTROL_REGISTER_W1C_MASK;
> > +			tmp |=3D CTRL_UTMI_PHY_EN;
> > +			iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
> > +		}
> >  		portsc |=3D 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 !=3D 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 !=3D FSL_USB2_PHY_ULPI && pdata->have_sysif_regs) {
> > +		tmp =3D ioread32be(non_ehci + FSL_SOC_USB_CTRL);
> > +		tmp &=3D ~CONTROL_REGISTER_W1C_MASK;
> > +		tmp |=3D USB_CTRL_USB_EN;
> > +		iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
> > +	}
> >
> >  	return 0;
> >  }
>=20
> Alan Stern