| Message ID | 1358411292-12288-1-git-send-email-vbyravarasu@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 ?
> -----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
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 ?
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
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
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 ?
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
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
> -----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
> -----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
> -----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
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 */
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(-)