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

[Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3910 bytes --]

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 ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]