Patchwork [v2,4/4] usb: Add APIs to access host registers from Tegra PHY

login
register
mail settings
Submitter Venu Byravarasu
Date Jan. 17, 2013, 8:28 a.m.
Message ID <1358411292-12288-1-git-send-email-vbyravarasu@nvidia.com>
Download mbox | patch
Permalink /patch/213171/
State Superseded, archived
Headers show

Comments

Venu Byravarasu - Jan. 17, 2013, 8:28 a.m.
As Tegra PHY driver need to access one of the Host registers,
added few APIs to ehci tegra driver.

Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com>
---
delta from v1:
Taken care of RWC bits, while accessing PORTSC register.

 drivers/usb/host/ehci-tegra.c     |   70 ++++++++++++++++++++++++++++++++++++-
 drivers/usb/phy/tegra_usb_phy.c   |   51 +++++++--------------------
 include/linux/usb/tegra_usb_phy.h |    6 +++
 3 files changed, 88 insertions(+), 39 deletions(-)
Felipe Balbi - Jan. 17, 2013, 9:02 a.m.
Hi,

On Thu, Jan 17, 2013 at 01:58:12PM +0530, Venu Byravarasu wrote:
> As Tegra PHY driver need to access one of the Host registers,
> added few APIs to ehci tegra driver.
> 
> Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com>

Stephen is this another of those patches you're gonna take care of
yourself ?

> ---
> delta from v1:
> Taken care of RWC bits, while accessing PORTSC register.
> 
>  drivers/usb/host/ehci-tegra.c     |   70 ++++++++++++++++++++++++++++++++++++-
>  drivers/usb/phy/tegra_usb_phy.c   |   51 +++++++--------------------
>  include/linux/usb/tegra_usb_phy.h |    6 +++
>  3 files changed, 88 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index 55a9cde..6bbf66a 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -2,7 +2,7 @@
>   * EHCI-compliant USB host controller driver for NVIDIA Tegra SoCs
>   *
>   * Copyright (C) 2010 Google, Inc.
> - * Copyright (C) 2009 NVIDIA Corporation
> + * Copyright (C) 2009 - 2013 NVIDIA Corporation
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms of the GNU General Public License as published by the
> @@ -33,6 +33,16 @@
>  #define TEGRA_USB2_BASE			0xC5004000
>  #define TEGRA_USB3_BASE			0xC5008000
>  
> +/* PORTSC registers */
> +#define USB_PORTSC1			0x184
> +#define USB_PORTSC1_PTS(x)	(((x) & 0x3) << 30)
> +#define USB_PORTSC1_PHCD	(1 << 23)
> +#define USB_PORTSC1_WKOC	(1 << 22)
> +#define USB_PORTSC1_WKDS	(1 << 21)
> +#define USB_PORTSC1_WKCN	(1 << 20)
> +#define USB_PORTSC1_PEC		(1 << 3)
> +#define USB_PORTSC1_CSC		(1 << 1)

please prepend this with TEGRA_ just as other defines.

> +
>  #define TEGRA_USB_DMA_ALIGN 32
>  
>  struct tegra_ehci_hcd {
> @@ -605,6 +615,53 @@ static const struct dev_pm_ops tegra_ehci_pm_ops = {
>  
>  #endif
>  
> +/* Bits of PORTSC1, which will get cleared by writing 1 into them */
> +#define TEGRA_PORTSC_RWC_BITS (USB_PORTSC1_CSC | USB_PORTSC1_PEC)
> +
> +void tegra_ehci_set_wakeon_events(struct usb_phy *x, bool enable)
> +{
> +	unsigned long val;
> +	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
> +	void __iomem *base = hcd->regs;
> +	u32 wake = USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN;
> +
> +	val = readl(base + USB_PORTSC1) & ~TEGRA_PORTSC_RWC_BITS;
> +	if (enable)
> +		val |= wake;
> +	else
> +		val &= ~wake;
> +	writel(val, base + USB_PORTSC1);
> +}
> +EXPORT_SYMBOL_GPL(tegra_ehci_set_wakeon_events);
> +
> +void tegra_ehci_set_pts(struct usb_phy *x, u8 pts_val)
> +{
> +	unsigned long val;
> +	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
> +	void __iomem *base = hcd->regs;
> +
> +	val = readl(base + USB_PORTSC1) & ~TEGRA_PORTSC_RWC_BITS;
> +	val &= ~USB_PORTSC1_PTS(3);
> +	val |= USB_PORTSC1_PTS(pts_val & 3);
> +	writel(val, base + USB_PORTSC1);
> +}
> +EXPORT_SYMBOL_GPL(tegra_ehci_set_pts);
> +
> +void tegra_ehci_set_phcd(struct usb_phy *x, bool enable)
> +{
> +	unsigned long val;
> +	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
> +	void __iomem *base = hcd->regs;
> +
> +	val = readl(base + USB_PORTSC1) & ~TEGRA_PORTSC_RWC_BITS;
> +	if (enable)
> +		val |= USB_PORTSC1_PHCD;
> +	else
> +		val &= ~USB_PORTSC1_PHCD;
> +	writel(val, base + USB_PORTSC1);
> +}
> +EXPORT_SYMBOL_GPL(tegra_ehci_set_phcd);

NAK to these three functions, you need to use whatever the PHY API
provides you, if it misses something, let's see how we can add those as
generic calls.

In fact, I wonder why do you want PHY driver to access Host address
space. Why don't you let your host driver handle the above ?
Venu Byravarasu - Jan. 17, 2013, 12:37 p.m.
> -----Original Message-----
> From: Felipe Balbi [mailto:balbi@ti.com]
> Sent: Thursday, January 17, 2013 2:33 PM
> To: Venu Byravarasu
> Cc: gregkh@linuxfoundation.org; stern@rowland.harvard.edu;
> balbi@ti.com; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org;
> swarren@wwwdotorg.org; linux-tegra@vger.kernel.org
> Subject: Re: [PATCH v2 4/4] usb: Add APIs to access host registers from Tegra
> PHY
> 
> * PGP Signed by an unknown key
> 
> Hi,
> 
> On Thu, Jan 17, 2013 at 01:58:12PM +0530, Venu Byravarasu wrote:
> > As Tegra PHY driver need to access one of the Host registers,
> > added few APIs to ehci tegra driver.
> >
> > Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com>
> 
> Stephen is this another of those patches you're gonna take care of
> yourself ?
> 
> > ---
> > delta from v1:
> > Taken care of RWC bits, while accessing PORTSC register.
> >
> >  drivers/usb/host/ehci-tegra.c     |   70
> ++++++++++++++++++++++++++++++++++++-
> >  drivers/usb/phy/tegra_usb_phy.c   |   51 +++++++--------------------
> >  include/linux/usb/tegra_usb_phy.h |    6 +++
> >  3 files changed, 88 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> > index 55a9cde..6bbf66a 100644
> > --- a/drivers/usb/host/ehci-tegra.c
> > +++ b/drivers/usb/host/ehci-tegra.c
> > @@ -2,7 +2,7 @@
> >   * EHCI-compliant USB host controller driver for NVIDIA Tegra SoCs
> >   *
> >   * Copyright (C) 2010 Google, Inc.
> > - * Copyright (C) 2009 NVIDIA Corporation
> > + * Copyright (C) 2009 - 2013 NVIDIA Corporation
> >   *
> >   * This program is free software; you can redistribute it and/or modify it
> >   * under the terms of the GNU General Public License as published by the
> > @@ -33,6 +33,16 @@
> >  #define TEGRA_USB2_BASE			0xC5004000
> >  #define TEGRA_USB3_BASE			0xC5008000
> >
> > +/* PORTSC registers */
> > +#define USB_PORTSC1			0x184
> > +#define USB_PORTSC1_PTS(x)	(((x) & 0x3) << 30)
> > +#define USB_PORTSC1_PHCD	(1 << 23)
> > +#define USB_PORTSC1_WKOC	(1 << 22)
> > +#define USB_PORTSC1_WKDS	(1 << 21)
> > +#define USB_PORTSC1_WKCN	(1 << 20)
> > +#define USB_PORTSC1_PEC		(1 << 3)
> > +#define USB_PORTSC1_CSC		(1 << 1)
> 
> please prepend this with TEGRA_ just as other defines.

Sure, will add it. 

> 
> > +
> >  #define TEGRA_USB_DMA_ALIGN 32
> >
> >  struct tegra_ehci_hcd {
> > @@ -605,6 +615,53 @@ static const struct dev_pm_ops
> tegra_ehci_pm_ops = {
> >
> >  #endif
> >
> > +/* Bits of PORTSC1, which will get cleared by writing 1 into them */
> > +#define TEGRA_PORTSC_RWC_BITS (USB_PORTSC1_CSC |
> USB_PORTSC1_PEC)
> > +
> > +void tegra_ehci_set_wakeon_events(struct usb_phy *x, bool enable)
> > +{
> > +	unsigned long val;
> > +	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
> > +	void __iomem *base = hcd->regs;
> > +	u32 wake = USB_PORTSC1_WKOC | USB_PORTSC1_WKDS |
> USB_PORTSC1_WKCN;
> > +
> > +	val = readl(base + USB_PORTSC1) & ~TEGRA_PORTSC_RWC_BITS;
> > +	if (enable)
> > +		val |= wake;
> > +	else
> > +		val &= ~wake;
> > +	writel(val, base + USB_PORTSC1);
> > +}
> > +EXPORT_SYMBOL_GPL(tegra_ehci_set_wakeon_events);
> > +
> > +void tegra_ehci_set_pts(struct usb_phy *x, u8 pts_val)
> > +{
> > +	unsigned long val;
> > +	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
> > +	void __iomem *base = hcd->regs;
> > +
> > +	val = readl(base + USB_PORTSC1) & ~TEGRA_PORTSC_RWC_BITS;
> > +	val &= ~USB_PORTSC1_PTS(3);
> > +	val |= USB_PORTSC1_PTS(pts_val & 3);
> > +	writel(val, base + USB_PORTSC1);
> > +}
> > +EXPORT_SYMBOL_GPL(tegra_ehci_set_pts);
> > +
> > +void tegra_ehci_set_phcd(struct usb_phy *x, bool enable)
> > +{
> > +	unsigned long val;
> > +	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
> > +	void __iomem *base = hcd->regs;
> > +
> > +	val = readl(base + USB_PORTSC1) & ~TEGRA_PORTSC_RWC_BITS;
> > +	if (enable)
> > +		val |= USB_PORTSC1_PHCD;
> > +	else
> > +		val &= ~USB_PORTSC1_PHCD;
> > +	writel(val, base + USB_PORTSC1);
> > +}
> > +EXPORT_SYMBOL_GPL(tegra_ehci_set_phcd);
> 
> NAK to these three functions, you need to use whatever the PHY API
> provides you, if it misses something, let's see how we can add those as
> generic calls.
> 
> In fact, I wonder why do you want PHY driver to access Host address
> space. Why don't you let your host driver handle the above ?

Tegra20 SOC contains 3 instances of USB controllers.
Some of these controllers can be configured to use different varieties
of PHYs e.g. instance 3 can be configured to use either UTMI or ICUSB PHYs.
Bits 31 & 30 from PORTSC register were allocated by our SOC designers
to inform the host controller about the PHY type to be used. 

As type of PHY is a property related to PHY DT nodes, PHY driver will get this info.
(Will remove phy_type from controller DT node soon, after all patches get merged.)
However as PORTSC register is in controller domain, added tegra_ehci_set_pts() API
 to serve the purpose.

As per Tegra USB PHY design, need to wait for PHY clock to stabilize once we
set/clear PHCD bit. Hence this is being accessed from PHY driver. To serve this
purpose added tegra_ehci_set_phcd().

On further analysis, seems tegra_ehci_set_wakeon_events() can be removed.

If you are okay with above explanation, I can send updated patch for review.

> 
> --
> balbi
> 
> * Unknown Key
> * 0x35CAA444
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi - Jan. 17, 2013, 1:50 p.m.
Hi,

On Thu, Jan 17, 2013 at 06:07:56PM +0530, Venu Byravarasu wrote:
> > > @@ -605,6 +615,53 @@ static const struct dev_pm_ops
> > tegra_ehci_pm_ops = {
> > >
> > >  #endif
> > >
> > > +/* Bits of PORTSC1, which will get cleared by writing 1 into them */
> > > +#define TEGRA_PORTSC_RWC_BITS (USB_PORTSC1_CSC |
> > USB_PORTSC1_PEC)
> > > +
> > > +void tegra_ehci_set_wakeon_events(struct usb_phy *x, bool enable)
> > > +{
> > > +	unsigned long val;
> > > +	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
> > > +	void __iomem *base = hcd->regs;
> > > +	u32 wake = USB_PORTSC1_WKOC | USB_PORTSC1_WKDS |
> > USB_PORTSC1_WKCN;
> > > +
> > > +	val = readl(base + USB_PORTSC1) & ~TEGRA_PORTSC_RWC_BITS;
> > > +	if (enable)
> > > +		val |= wake;
> > > +	else
> > > +		val &= ~wake;
> > > +	writel(val, base + USB_PORTSC1);
> > > +}
> > > +EXPORT_SYMBOL_GPL(tegra_ehci_set_wakeon_events);
> > > +
> > > +void tegra_ehci_set_pts(struct usb_phy *x, u8 pts_val)
> > > +{
> > > +	unsigned long val;
> > > +	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
> > > +	void __iomem *base = hcd->regs;
> > > +
> > > +	val = readl(base + USB_PORTSC1) & ~TEGRA_PORTSC_RWC_BITS;
> > > +	val &= ~USB_PORTSC1_PTS(3);
> > > +	val |= USB_PORTSC1_PTS(pts_val & 3);
> > > +	writel(val, base + USB_PORTSC1);
> > > +}
> > > +EXPORT_SYMBOL_GPL(tegra_ehci_set_pts);
> > > +
> > > +void tegra_ehci_set_phcd(struct usb_phy *x, bool enable)
> > > +{
> > > +	unsigned long val;
> > > +	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
> > > +	void __iomem *base = hcd->regs;
> > > +
> > > +	val = readl(base + USB_PORTSC1) & ~TEGRA_PORTSC_RWC_BITS;
> > > +	if (enable)
> > > +		val |= USB_PORTSC1_PHCD;
> > > +	else
> > > +		val &= ~USB_PORTSC1_PHCD;
> > > +	writel(val, base + USB_PORTSC1);
> > > +}
> > > +EXPORT_SYMBOL_GPL(tegra_ehci_set_phcd);
> > 
> > NAK to these three functions, you need to use whatever the PHY API
> > provides you, if it misses something, let's see how we can add those as
> > generic calls.
> > 
> > In fact, I wonder why do you want PHY driver to access Host address
> > space. Why don't you let your host driver handle the above ?
> 
> Tegra20 SOC contains 3 instances of USB controllers.

fair enough.

> Some of these controllers can be configured to use different varieties
> of PHYs e.g. instance 3 can be configured to use either UTMI or ICUSB
> PHYs.

just like OMAP...

> Bits 31 & 30 from PORTSC register were allocated by our SOC designers
> to inform the host controller about the PHY type to be used. 

Wow, that's something you should never do. PORTSC register belongs to
the EHCI controller and those bits are reserved for future use and they
*MUST* return zero. I wouldn't be surprised if current EHCI driver
assumes those bits will be zero and/or makes sure they're set to zero
when writing to PORTSC register.

What if after configuring those two bits, ehci-hcd.ko clears them by
accident ? Your platform won't work.

> As type of PHY is a property related to PHY DT nodes, PHY driver will get this info.
> (Will remove phy_type from controller DT node soon, after all patches get merged.)
> However as PORTSC register is in controller domain, added tegra_ehci_set_pts() API
>  to serve the purpose.
> 
> As per Tegra USB PHY design, need to wait for PHY clock to stabilize once we
> set/clear PHCD bit. Hence this is being accessed from PHY driver. To serve this
> purpose added tegra_ehci_set_phcd().
> 
> On further analysis, seems tegra_ehci_set_wakeon_events() can be
> removed.
> 
> If you are okay with above explanation, I can send updated patch for
> review.

not sure I want to sign-off such a patch. I understand it's how your HW
was done, but this shouldn't have been done, really.

Alan, what do you think ? Are you ok with PHY driver overwritting PORTSC
register ?
Alan Stern - Jan. 17, 2013, 3:15 p.m.
On Thu, 17 Jan 2013, Venu Byravarasu wrote:

> As Tegra PHY driver need to access one of the Host registers,
> added few APIs to ehci tegra driver.
> 
> Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com>
> ---
> delta from v1:
> Taken care of RWC bits, while accessing PORTSC register.
> 
>  drivers/usb/host/ehci-tegra.c     |   70 ++++++++++++++++++++++++++++++++++++-
>  drivers/usb/phy/tegra_usb_phy.c   |   51 +++++++--------------------
>  include/linux/usb/tegra_usb_phy.h |    6 +++
>  3 files changed, 88 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index 55a9cde..6bbf66a 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -2,7 +2,7 @@
>   * EHCI-compliant USB host controller driver for NVIDIA Tegra SoCs
>   *
>   * Copyright (C) 2010 Google, Inc.
> - * Copyright (C) 2009 NVIDIA Corporation
> + * Copyright (C) 2009 - 2013 NVIDIA Corporation
>   *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms of the GNU General Public License as published by the
> @@ -33,6 +33,16 @@
>  #define TEGRA_USB2_BASE			0xC5004000
>  #define TEGRA_USB3_BASE			0xC5008000
>  
> +/* PORTSC registers */
> +#define USB_PORTSC1			0x184
> +#define USB_PORTSC1_PTS(x)	(((x) & 0x3) << 30)
> +#define USB_PORTSC1_PHCD	(1 << 23)
> +#define USB_PORTSC1_WKOC	(1 << 22)
> +#define USB_PORTSC1_WKDS	(1 << 21)
> +#define USB_PORTSC1_WKCN	(1 << 20)
> +#define USB_PORTSC1_PEC		(1 << 3)
> +#define USB_PORTSC1_CSC		(1 << 1)

Why redefine these values when they are already defined in 
include/linux/usb/ehci_defs.h?

> @@ -605,6 +615,53 @@ static const struct dev_pm_ops tegra_ehci_pm_ops = {
>  
>  #endif
>  
> +/* Bits of PORTSC1, which will get cleared by writing 1 into them */
> +#define TEGRA_PORTSC_RWC_BITS (USB_PORTSC1_CSC | USB_PORTSC1_PEC)

Likewise for this.  Not to mention that you forgot to include the 
overcurrent-change bit.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern - Jan. 17, 2013, 3:21 p.m.
On Thu, 17 Jan 2013, Felipe Balbi wrote:

> > Bits 31 & 30 from PORTSC register were allocated by our SOC designers
> > to inform the host controller about the PHY type to be used. 
> 
> Wow, that's something you should never do. PORTSC register belongs to
> the EHCI controller and those bits are reserved for future use and they
> *MUST* return zero. I wouldn't be surprised if current EHCI driver
> assumes those bits will be zero and/or makes sure they're set to zero
> when writing to PORTSC register.

In fact, those bits _have_ been assigned an official purpose in the 
EHCI-1.1 addendum.  Presumably the Tegra hardware only supports 
EHCI-1.0.

> What if after configuring those two bits, ehci-hcd.ko clears them by
> accident ? Your platform won't work.

That won't happen; ehci-hcd is careful to preserve the values of PORTSC
register bits it doesn't use.  But this still isn't a good idea.

> > As type of PHY is a property related to PHY DT nodes, PHY driver will get this info.
> > (Will remove phy_type from controller DT node soon, after all patches get merged.)
> > However as PORTSC register is in controller domain, added tegra_ehci_set_pts() API
> >  to serve the purpose.
> > 
> > As per Tegra USB PHY design, need to wait for PHY clock to stabilize once we
> > set/clear PHCD bit. Hence this is being accessed from PHY driver. To serve this
> > purpose added tegra_ehci_set_phcd().
> > 
> > On further analysis, seems tegra_ehci_set_wakeon_events() can be
> > removed.
> > 
> > If you are okay with above explanation, I can send updated patch for
> > review.
> 
> not sure I want to sign-off such a patch. I understand it's how your HW
> was done, but this shouldn't have been done, really.
> 
> Alan, what do you think ? Are you ok with PHY driver overwritting PORTSC
> register ?

It's okay provided it is done correctly -- i.e., in a way that won't 
interfere with the normal EHCI operation.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi - Jan. 17, 2013, 3:32 p.m.
On Thu, Jan 17, 2013 at 10:21:53AM -0500, Alan Stern wrote:
> On Thu, 17 Jan 2013, Felipe Balbi wrote:
> 
> > > Bits 31 & 30 from PORTSC register were allocated by our SOC designers
> > > to inform the host controller about the PHY type to be used. 
> > 
> > Wow, that's something you should never do. PORTSC register belongs to
> > the EHCI controller and those bits are reserved for future use and they
> > *MUST* return zero. I wouldn't be surprised if current EHCI driver
> > assumes those bits will be zero and/or makes sure they're set to zero
> > when writing to PORTSC register.
> 
> In fact, those bits _have_ been assigned an official purpose in the 
> EHCI-1.1 addendum.  Presumably the Tegra hardware only supports 
> EHCI-1.0.

I see, they're used for device addresses now.

How can we make sure on Tegra systems we won't use those top two bits ?
Alan Stern - Jan. 17, 2013, 4:03 p.m.
On Thu, 17 Jan 2013, Felipe Balbi wrote:

> On Thu, Jan 17, 2013 at 10:21:53AM -0500, Alan Stern wrote:
> > On Thu, 17 Jan 2013, Felipe Balbi wrote:
> > 
> > > > Bits 31 & 30 from PORTSC register were allocated by our SOC designers
> > > > to inform the host controller about the PHY type to be used. 
> > > 
> > > Wow, that's something you should never do. PORTSC register belongs to
> > > the EHCI controller and those bits are reserved for future use and they
> > > *MUST* return zero. I wouldn't be surprised if current EHCI driver
> > > assumes those bits will be zero and/or makes sure they're set to zero
> > > when writing to PORTSC register.
> > 
> > In fact, those bits _have_ been assigned an official purpose in the 
> > EHCI-1.1 addendum.  Presumably the Tegra hardware only supports 
> > EHCI-1.0.
> 
> I see, they're used for device addresses now.
> 
> How can we make sure on Tegra systems we won't use those top two bits ?

ehci-hcd doesn't use those device address bits at all.  They are meant
for the LPM (Link Power Management) extension, which ehci-hcd doesn't
support.  Even if support gets added in the future, we'll know that the
LPM capability isn't present unless bit 17 in the HCCPARAMS register is
set.

On the other hand, what I wrote earlier about not overwriting those 
bits wasn't quite correct.  ehci-hcd _does_ overwrite them with 0's 
during shutdown or removal.  I don't know if this will matter for the 
Tegra platform.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren - Jan. 17, 2013, 10:18 p.m.
On 01/17/2013 02:02 AM, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Jan 17, 2013 at 01:58:12PM +0530, Venu Byravarasu wrote:
>> As Tegra PHY driver need to access one of the Host registers, 
>> added few APIs to ehci tegra driver.
>> 
>> Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com>
> 
> Stephen is this another of those patches you're gonna take care of 
> yourself ?

Yes; in kernel 3.9, I believe I need to take any significant Tegra
EHCI or PHY patches through the Tegra tree due to the need to change
the Tegra device trees in lock-step with some of the code changes, and
other Tegra-related changes also changing the USB-related nodes in
device tree e.g. to add clocks properties.

Hopefully we get enough USB patches into 3.9 that the dependencies are
then removed, and we can revert back to the usual merging path in 3.10...
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Venu Byravarasu - Jan. 18, 2013, 4:56 a.m.
> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Thursday, January 17, 2013 8:45 PM
> To: Venu Byravarasu
> Cc: gregkh@linuxfoundation.org; balbi@ti.com; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org; swarren@wwwdotorg.org; linux-
> tegra@vger.kernel.org
> Subject: Re: [PATCH v2 4/4] usb: Add APIs to access host registers from Tegra
> PHY
> 
> On Thu, 17 Jan 2013, Venu Byravarasu wrote:
> 
> > As Tegra PHY driver need to access one of the Host registers,
> > added few APIs to ehci tegra driver.
> >
> > Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com>
> > ---
> > delta from v1:
> > Taken care of RWC bits, while accessing PORTSC register.
> >
> >  drivers/usb/host/ehci-tegra.c     |   70
> ++++++++++++++++++++++++++++++++++++-
> >  drivers/usb/phy/tegra_usb_phy.c   |   51 +++++++--------------------
> >  include/linux/usb/tegra_usb_phy.h |    6 +++
> >  3 files changed, 88 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> > index 55a9cde..6bbf66a 100644
> > --- a/drivers/usb/host/ehci-tegra.c
> > +++ b/drivers/usb/host/ehci-tegra.c
> > @@ -2,7 +2,7 @@
> >   * EHCI-compliant USB host controller driver for NVIDIA Tegra SoCs
> >   *
> >   * Copyright (C) 2010 Google, Inc.
> > - * Copyright (C) 2009 NVIDIA Corporation
> > + * Copyright (C) 2009 - 2013 NVIDIA Corporation
> >   *
> >   * This program is free software; you can redistribute it and/or modify it
> >   * under the terms of the GNU General Public License as published by the
> > @@ -33,6 +33,16 @@
> >  #define TEGRA_USB2_BASE			0xC5004000
> >  #define TEGRA_USB3_BASE			0xC5008000
> >
> > +/* PORTSC registers */
> > +#define USB_PORTSC1			0x184
> > +#define USB_PORTSC1_PTS(x)	(((x) & 0x3) << 30)
> > +#define USB_PORTSC1_PHCD	(1 << 23)
> > +#define USB_PORTSC1_WKOC	(1 << 22)
> > +#define USB_PORTSC1_WKDS	(1 << 21)
> > +#define USB_PORTSC1_WKCN	(1 << 20)
> > +#define USB_PORTSC1_PEC		(1 << 3)
> > +#define USB_PORTSC1_CSC		(1 << 1)
> 
> Why redefine these values when they are already defined in
> include/linux/usb/ehci_defs.h?

Agreed. Will remove all above defines except PHCD & PTS, as these two
fields are added by tegra SOC to PORTSC.

> 
> > @@ -605,6 +615,53 @@ static const struct dev_pm_ops
> tegra_ehci_pm_ops = {
> >
> >  #endif
> >
> > +/* Bits of PORTSC1, which will get cleared by writing 1 into them */
> > +#define TEGRA_PORTSC_RWC_BITS (USB_PORTSC1_CSC |
> USB_PORTSC1_PEC)
> 
> Likewise for this.  Not to mention that you forgot to include the
> overcurrent-change bit.

As OCC bit is "not supported" by Tegra20 SOC, did not include it.
Should I still add this, to be compliant with EHCI spec?
Based on your comments, I can update the patch and send for review.

> 
> Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Venu Byravarasu - Jan. 18, 2013, 5:39 a.m.
> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu]
> Sent: Thursday, January 17, 2013 9:34 PM
> To: Felipe Balbi
> Cc: Venu Byravarasu; gregkh@linuxfoundation.org; linux-
> usb@vger.kernel.org; linux-kernel@vger.kernel.org;
> swarren@wwwdotorg.org; linux-tegra@vger.kernel.org
> Subject: Re: [PATCH v2 4/4] usb: Add APIs to access host registers from Tegra
> PHY
> 
> On Thu, 17 Jan 2013, Felipe Balbi wrote:
> 
> > On Thu, Jan 17, 2013 at 10:21:53AM -0500, Alan Stern wrote:
> > > On Thu, 17 Jan 2013, Felipe Balbi wrote:
> > >
> > > > > Bits 31 & 30 from PORTSC register were allocated by our SOC
> designers
> > > > > to inform the host controller about the PHY type to be used.
> > > >
> > > > Wow, that's something you should never do. PORTSC register belongs
> to
> > > > the EHCI controller and those bits are reserved for future use and they
> > > > *MUST* return zero. I wouldn't be surprised if current EHCI driver
> > > > assumes those bits will be zero and/or makes sure they're set to zero
> > > > when writing to PORTSC register.
> > >
> > > In fact, those bits _have_ been assigned an official purpose in the
> > > EHCI-1.1 addendum.  Presumably the Tegra hardware only supports
> > > EHCI-1.0.
> >
> > I see, they're used for device addresses now.
> >
> > How can we make sure on Tegra systems we won't use those top two bits ?
> 
> ehci-hcd doesn't use those device address bits at all.  They are meant
> for the LPM (Link Power Management) extension, which ehci-hcd doesn't
> support.  Even if support gets added in the future, we'll know that the
> LPM capability isn't present unless bit 17 in the HCCPARAMS register is
> set.
> 
> On the other hand, what I wrote earlier about not overwriting those
> bits wasn't quite correct.  ehci-hcd _does_ overwrite them with 0's
> during shutdown or removal.  I don't know if this will matter for the
> Tegra platform.

This should not cause any issue to Tegra platform, as these bits are set
during PHY power on.

> 
> Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Venu Byravarasu - Jan. 18, 2013, 5:42 a.m.
> -----Original Message-----
> From: Venu Byravarasu
> Sent: Friday, January 18, 2013 10:26 AM
> To: 'Alan Stern'
> Cc: gregkh@linuxfoundation.org; balbi@ti.com; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org; swarren@wwwdotorg.org; linux-
> tegra@vger.kernel.org
> Subject: RE: [PATCH v2 4/4] usb: Add APIs to access host registers from Tegra
> PHY
> 
> > -----Original Message-----
> > From: Alan Stern [mailto:stern@rowland.harvard.edu]
> > Sent: Thursday, January 17, 2013 8:45 PM
> > To: Venu Byravarasu
> > Cc: gregkh@linuxfoundation.org; balbi@ti.com; linux-usb@vger.kernel.org;
> > linux-kernel@vger.kernel.org; swarren@wwwdotorg.org; linux-
> > tegra@vger.kernel.org
> > Subject: Re: [PATCH v2 4/4] usb: Add APIs to access host registers from
> Tegra
> > PHY
> >
> > On Thu, 17 Jan 2013, Venu Byravarasu wrote:
> >
> > > As Tegra PHY driver need to access one of the Host registers,
> > > added few APIs to ehci tegra driver.
> > >
> > > Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com>
> > > ---
> > > delta from v1:
> > > Taken care of RWC bits, while accessing PORTSC register.
> > >
> > >  drivers/usb/host/ehci-tegra.c     |   70
> > ++++++++++++++++++++++++++++++++++++-
> > >  drivers/usb/phy/tegra_usb_phy.c   |   51 +++++++--------------------
> > >  include/linux/usb/tegra_usb_phy.h |    6 +++
> > >  3 files changed, 88 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> > > index 55a9cde..6bbf66a 100644
> > > --- a/drivers/usb/host/ehci-tegra.c
> > > +++ b/drivers/usb/host/ehci-tegra.c
> > > @@ -2,7 +2,7 @@
> > >   * EHCI-compliant USB host controller driver for NVIDIA Tegra SoCs
> > >   *
> > >   * Copyright (C) 2010 Google, Inc.
> > > - * Copyright (C) 2009 NVIDIA Corporation
> > > + * Copyright (C) 2009 - 2013 NVIDIA Corporation
> > >   *
> > >   * This program is free software; you can redistribute it and/or modify it
> > >   * under the terms of the GNU General Public License as published by the
> > > @@ -33,6 +33,16 @@
> > >  #define TEGRA_USB2_BASE			0xC5004000
> > >  #define TEGRA_USB3_BASE			0xC5008000
> > >
> > > +/* PORTSC registers */
> > > +#define USB_PORTSC1			0x184
> > > +#define USB_PORTSC1_PTS(x)	(((x) & 0x3) << 30)
> > > +#define USB_PORTSC1_PHCD	(1 << 23)
> > > +#define USB_PORTSC1_WKOC	(1 << 22)
> > > +#define USB_PORTSC1_WKDS	(1 << 21)
> > > +#define USB_PORTSC1_WKCN	(1 << 20)
> > > +#define USB_PORTSC1_PEC		(1 << 3)
> > > +#define USB_PORTSC1_CSC		(1 << 1)
> >
> > Why redefine these values when they are already defined in
> > include/linux/usb/ehci_defs.h?
> 
> Agreed. Will remove all above defines except PHCD & PTS, as these two
> fields are added by tegra SOC to PORTSC.
> 
> >
> > > @@ -605,6 +615,53 @@ static const struct dev_pm_ops
> > tegra_ehci_pm_ops = {
> > >
> > >  #endif
> > >
> > > +/* Bits of PORTSC1, which will get cleared by writing 1 into them */
> > > +#define TEGRA_PORTSC_RWC_BITS (USB_PORTSC1_CSC |
> > USB_PORTSC1_PEC)
> >
> > Likewise for this.  Not to mention that you forgot to include the
> > overcurrent-change bit.
> 
> As OCC bit is "not supported" by Tegra20 SOC, did not include it.
> Should I still add this, to be compliant with EHCI spec?
> Based on your comments, I can update the patch and send for review.

Any ways, as we're just adding OCC for proper RWC treatment, I don't
see any harm in adding it to the list.
Hence will update the patch with two changes mentioned by Alan. 

> 
> >
> > Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 55a9cde..6bbf66a 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -2,7 +2,7 @@ 
  * EHCI-compliant USB host controller driver for NVIDIA Tegra SoCs
  *
  * Copyright (C) 2010 Google, Inc.
- * Copyright (C) 2009 NVIDIA Corporation
+ * Copyright (C) 2009 - 2013 NVIDIA Corporation
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License as published by the
@@ -33,6 +33,16 @@ 
 #define TEGRA_USB2_BASE			0xC5004000
 #define TEGRA_USB3_BASE			0xC5008000
 
+/* PORTSC registers */
+#define USB_PORTSC1			0x184
+#define USB_PORTSC1_PTS(x)	(((x) & 0x3) << 30)
+#define USB_PORTSC1_PHCD	(1 << 23)
+#define USB_PORTSC1_WKOC	(1 << 22)
+#define USB_PORTSC1_WKDS	(1 << 21)
+#define USB_PORTSC1_WKCN	(1 << 20)
+#define USB_PORTSC1_PEC		(1 << 3)
+#define USB_PORTSC1_CSC		(1 << 1)
+
 #define TEGRA_USB_DMA_ALIGN 32
 
 struct tegra_ehci_hcd {
@@ -605,6 +615,53 @@  static const struct dev_pm_ops tegra_ehci_pm_ops = {
 
 #endif
 
+/* Bits of PORTSC1, which will get cleared by writing 1 into them */
+#define TEGRA_PORTSC_RWC_BITS (USB_PORTSC1_CSC | USB_PORTSC1_PEC)
+
+void tegra_ehci_set_wakeon_events(struct usb_phy *x, bool enable)
+{
+	unsigned long val;
+	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
+	void __iomem *base = hcd->regs;
+	u32 wake = USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN;
+
+	val = readl(base + USB_PORTSC1) & ~TEGRA_PORTSC_RWC_BITS;
+	if (enable)
+		val |= wake;
+	else
+		val &= ~wake;
+	writel(val, base + USB_PORTSC1);
+}
+EXPORT_SYMBOL_GPL(tegra_ehci_set_wakeon_events);
+
+void tegra_ehci_set_pts(struct usb_phy *x, u8 pts_val)
+{
+	unsigned long val;
+	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
+	void __iomem *base = hcd->regs;
+
+	val = readl(base + USB_PORTSC1) & ~TEGRA_PORTSC_RWC_BITS;
+	val &= ~USB_PORTSC1_PTS(3);
+	val |= USB_PORTSC1_PTS(pts_val & 3);
+	writel(val, base + USB_PORTSC1);
+}
+EXPORT_SYMBOL_GPL(tegra_ehci_set_pts);
+
+void tegra_ehci_set_phcd(struct usb_phy *x, bool enable)
+{
+	unsigned long val;
+	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
+	void __iomem *base = hcd->regs;
+
+	val = readl(base + USB_PORTSC1) & ~TEGRA_PORTSC_RWC_BITS;
+	if (enable)
+		val |= USB_PORTSC1_PHCD;
+	else
+		val &= ~USB_PORTSC1_PHCD;
+	writel(val, base + USB_PORTSC1);
+}
+EXPORT_SYMBOL_GPL(tegra_ehci_set_phcd);
+
 static u64 tegra_ehci_dma_mask = DMA_BIT_MASK(32);
 
 static int tegra_ehci_probe(struct platform_device *pdev)
@@ -616,6 +673,7 @@  static int tegra_ehci_probe(struct platform_device *pdev)
 	int err = 0;
 	int irq;
 	int instance = pdev->id;
+	struct usb_phy *u_phy;
 
 	pdata = pdev->dev.platform_data;
 	if (!pdata) {
@@ -718,6 +776,16 @@  static int tegra_ehci_probe(struct platform_device *pdev)
 
 	usb_phy_init(&tegra->phy->u_phy);
 
+	hcd->phy = u_phy = &tegra->phy->u_phy;
+	u_phy->otg = devm_kzalloc(&pdev->dev, sizeof(struct usb_otg),
+			     GFP_KERNEL);
+	if (!u_phy->otg) {
+		dev_err(&pdev->dev, "Failed to alloc memory for otg\n");
+		err = -ENOMEM;
+		goto fail_io;
+	}
+	u_phy->otg->host = hcd_to_bus(hcd);
+
 	err = usb_phy_set_suspend(&tegra->phy->u_phy, 0);
 	if (err) {
 		dev_err(&pdev->dev, "Failed to power on the phy\n");
diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
index ce1ff2a..f3b73b3 100644
--- a/drivers/usb/phy/tegra_usb_phy.c
+++ b/drivers/usb/phy/tegra_usb_phy.c
@@ -36,19 +36,6 @@ 
 
 #define ULPI_VIEWPORT		0x170
 
-#define USB_PORTSC1		0x184
-#define   USB_PORTSC1_PTS(x)	(((x) & 0x3) << 30)
-#define   USB_PORTSC1_PSPD(x)	(((x) & 0x3) << 26)
-#define   USB_PORTSC1_PHCD	(1 << 23)
-#define   USB_PORTSC1_WKOC	(1 << 22)
-#define   USB_PORTSC1_WKDS	(1 << 21)
-#define   USB_PORTSC1_WKCN	(1 << 20)
-#define   USB_PORTSC1_PTC(x)	(((x) & 0xf) << 16)
-#define   USB_PORTSC1_PP	(1 << 12)
-#define   USB_PORTSC1_SUSP	(1 << 7)
-#define   USB_PORTSC1_PE	(1 << 2)
-#define   USB_PORTSC1_CCS	(1 << 0)
-
 #define USB_SUSP_CTRL		0x400
 #define   USB_WAKE_ON_CNNT_EN_DEV	(1 << 3)
 #define   USB_WAKE_ON_DISCON_EN_DEV	(1 << 4)
@@ -311,11 +298,8 @@  static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
 		val = readl(base + USB_SUSP_CTRL);
 		val &= ~USB_SUSP_SET;
 		writel(val, base + USB_SUSP_CTRL);
-	} else {
-		val = readl(base + USB_PORTSC1);
-		val |= USB_PORTSC1_PHCD;
-		writel(val, base + USB_PORTSC1);
-	}
+	} else
+		tegra_ehci_set_phcd(&phy->u_phy, true);
 
 	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID, 0) < 0)
 		pr_err("%s: timeout waiting for phy to stabilize\n", __func__);
@@ -336,11 +320,8 @@  static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
 		val = readl(base + USB_SUSP_CTRL);
 		val &= ~USB_SUSP_CLR;
 		writel(val, base + USB_SUSP_CTRL);
-	} else {
-		val = readl(base + USB_PORTSC1);
-		val &= ~USB_PORTSC1_PHCD;
-		writel(val, base + USB_PORTSC1);
-	}
+	} else
+		tegra_ehci_set_phcd(&phy->u_phy, false);
 
 	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID,
 						     USB_PHY_CLK_VALID))
@@ -462,11 +443,8 @@  static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 
 	utmi_phy_clk_enable(phy);
 
-	if (!phy->is_legacy_phy) {
-		val = readl(base + USB_PORTSC1);
-		val &= ~USB_PORTSC1_PTS(~0);
-		writel(val, base + USB_PORTSC1);
-	}
+	if (!phy->is_legacy_phy)
+		tegra_ehci_set_pts(&phy->u_phy, 0);
 
 	return 0;
 }
@@ -611,10 +589,7 @@  static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
 		return ret;
 	}
 
-	val = readl(base + USB_PORTSC1);
-	val |= USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN;
-	writel(val, base + USB_PORTSC1);
-
+	tegra_ehci_set_wakeon_events(&phy->u_phy, true);
 	val = readl(base + USB_SUSP_CTRL);
 	val |= USB_SUSP_CLR;
 	writel(val, base + USB_SUSP_CTRL);
@@ -629,17 +604,12 @@  static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
 
 static int ulpi_phy_power_off(struct tegra_usb_phy *phy)
 {
-	unsigned long val;
-	void __iomem *base = phy->regs;
 	struct tegra_ulpi_config *config = phy->config;
 
 	/* Clear WKCN/WKDS/WKOC wake-on events that can cause the USB
 	 * Controller to immediately bring the ULPI PHY out of low power
 	 */
-	val = readl(base + USB_PORTSC1);
-	val &= ~(USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN);
-	writel(val, base + USB_PORTSC1);
-
+	tegra_ehci_set_wakeon_events(&phy->u_phy, false);
 	clk_disable(phy->clk);
 	return gpio_direction_output(config->reset_gpio, 0);
 }
@@ -742,6 +712,11 @@  struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
 	phy->dev = dev;
 	phy->is_legacy_phy =
 		of_property_read_bool(np, "nvidia,has-legacy-mode");
+	err = of_property_match_string(np, "phy_type", "ulpi");
+	if (err < 0)
+		phy->is_ulpi_phy = false;
+	else
+		phy->is_ulpi_phy = true;
 
 	if (!phy->config) {
 		if (phy->is_ulpi_phy) {
diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
index a6a89d4..d5f695b 100644
--- a/include/linux/usb/tegra_usb_phy.h
+++ b/include/linux/usb/tegra_usb_phy.h
@@ -75,4 +75,10 @@  void tegra_ehci_phy_restore_start(struct tegra_usb_phy *phy,
 
 void tegra_ehci_phy_restore_end(struct tegra_usb_phy *phy);
 
+void tegra_ehci_set_wakeon_events(struct usb_phy *x, bool enable);
+
+void tegra_ehci_set_pts(struct usb_phy *x, u8 pts_val);
+
+void tegra_ehci_set_phcd(struct usb_phy *x, bool enable);
+
 #endif /* __TEGRA_USB_PHY_H */