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

Alan Stern
2013-01-17T15:21:53Z

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