Re: [PATCH 8/8] phy: add driver for TI TUSB1210 ULPI PHY

[David Cohen <david.a.cohen@linux.intel.com>]

On Tue, Feb 03, 2015 at 01:37:39PM +0200, Heikki Krogerus wrote:
> Hi David, Felipe,

Hi Heikki,

> 
> > > > > why would you have dwc3 mess around with the PHY's gpios ? Doesn't look
> > > > > very good.
> > > > 
> > > > ..but unfortunately we can't use the bus without it :(. We depend on
> > > > being able to read the vendor and product id's in the bus driver.
> > > 
> > > Doesn't the ugly platform device case look less ugly than this?
> > 
> > The platform device would in any case need to be created only for this
> > case. We can't any more just create the phy device unconditionally for
> > every PCI platform like it was created before, as it's clear now the
> > PHY device can be be created from other sources. It was a risk always.
> > 
> > But the big problem is that since the PHY on your board is ULPI PHY,
> > it will make it really difficult to add the ULPI bus support. And like
> > I said in my previous mail, we would really need it for the boards
> > that expect the PHY driver to provide functions like charger
> > detection. We don't need it only for BYT based boards.
> > 
> > So on top of the above, since the GPIO resources are given to dwc3, I
> > actually think that my hack is better then the platform device.
> 
> So what do you guys think we should do? I can't figure out how would
> we make the platform device work with ulpi bus. If we think about
> handling this in ulpi bus driver by setting setting the gpios before
> attempting to access the phy, which I'm not completely against, we
> have couple of problems.

It's still not enough :/
In order to make the phy functional (in BYT case) you need to bring DWC3
to reset state, then toggle these gpios to reset and power on phy, then
remove DWC3 from reset state so both can sync clocks.
You can still reset and power on phy before DWC3 goes to and from reset,
but then there's a chance phy may become unstable. It's then expected to
fail long stability tests.

> 
> Firstly, the gpio resources are given to the dwc3 in this case,
> while normally they will be given to separate device object for the
> phy.

Yup. You're correct. We can influence new products to not repeat this
issue, but we still need to support legacy ones.

> 
> Secondly, these gpios were not labeled in DSDT, but apparently
> requesting the gpios with index will be deprecated and not acceptable
> any more. With the remaining platforms that have not labeled the gpios
> we have to bind the gpios to labels separately in the drivers. With
> ACPI platforms the labels are introduced in struct acpi_gpio_mapping
> which needs to be registered with acpi_dev_add_driver_gpios(). Check
> net/rfkill/rfkill-gpio.c as an example how to use it.

For new products, certanly.

> 
> I think those points would make this too platform specific case to be
> handled in the ulpi bus driver.
> 
> Suggestions? I still think the correct thing to do is to handle this
> in the quirk in dwc3-pci.c.

IMO it would make things uglier than it was before.

BR, David

> 
> 
> Cheers,
> 
> -- 
> heikki