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

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

On Fri, Jan 30, 2015 at 02:18:42PM +0200, Heikki Krogerus wrote:
> Hi,
> 
> > > > What exactly are we breaking here? The USB on BYT-CR does not work yet
> > > > with the mainline kernel, or does it? To enable it, I already
> > > > suggested the BYT quirk (attached again).
> > 
> > It used to work with mainline with minor restrictions. It stopped
> > working (and I failed to catch it during patch review) when NOP phy
> > enumeration was removed from dwc3-pci.c (but my understanding is that
> > Felipe is expecting to add it back as default phy in case no phy is
> > found by dwc3).
> > 
> > BYT-CR worked well except for operations that needed to access phy's
> > registers via ULPI bus (e.g. eye optimization). But to power on i.e.
> > TUSB1210 all you need it to toggle GPIOs, which is done by generic phy.
> > The need for ULPI bus was to complement this missing features, but
> > instead we're breaking BYT-CR :/
> 
> So what you are saying that if I get one of those devices you
> mentioned and try vanilla kernel on it, the USB will work without any
> modifications to the kernel?

You're misunderstanding BYT-CR SoC and external board components. The
only reason that prevents most of BYT-CR boards' USB device work
out-of-the-box is a switch device muxing D+/D- between host and device
controllers (they depend on a single GPIO level to be toggled to get USB
device working). I started discussion on how to upstream this case, but
this is board related, not BYT-CR related. Some boards have also an i2c
switch which requires extra i2c driver to get USB device working. But it
doesn't mean the phy/otg controllers aren't fully functional with dwc3 +
generic phy drivers.

FWIW if you test a board without such switch (e.g. a reference BYT-T
board called FFRD8 - BYT-CR is a derivation of BYT-T) it will work
out-of-the-box.

> 
> > Let's propose this non-BYT-CR scenario:
> > You have dwc3/phy powered on by BIOS and dwc3 + phy as modules. You
> > probe the modules for the first time and it works because phy was
> > accessible. Then we remove the modules and load it again. By removing
> > the modules phy is not functional anymore (remove operation will put
> > them in reset state). Then you load the modules again. It certainly will
> > fail to access ULPI bus during phy's probe this time. BYT-CR is just an
> > example where BIOS lets phy in reset during probe, but you can get this
> > same behavior on other platforms too.
> 
> You have a point here. I'm curious now what you reply to my question
> about the possibility to modify DSDT in my previous mail. Because the
> properties would be a way out of the PHY powering issue.

You missed my question. Have you tried to remove and reload dwc3 and phy
modules with your test case?

> 
> You know, even if we can't make changes to the DSDT (like I suspect)
> we can still use the properties once this is accepted:
> http://www.spinics.net/lists/linux-acpi/msg55337.html
> 
> So we can pass the "ulpi,vendor" and "ulpi,product" also with these
> generic properties. I'm attaching a patch where I'm converting the
> platform data AMD uses to them as an example how they could be used.
> 
> > > > BYT-CR's USB is not supported in mainline yet unless I'm completely
> > > > mistaken, but we have the plan for it. Instead of trying to take any
> > > > shortcuts, let's follow that plan.
> > 
> > It is supported, as I stated above. I don't know where you got the idea
> > it wasn't. I made BYT-T (and BYT-CR) + Merrifield USB device layer work
> > with mainline since 3.14. But this patch made the regression to stop
> > working:
> > https://www.mail-archive.com/linux-usb@vger.kernel.org/msg54400.html
> 
> How can it work with mainline if there was nothing toggling the gpios
> yet?

Again, that's board related, not BYT-CR. See my reply above and redo
your tests using BYT-T reference board.

> 
> > I'd prefer to go back to platform device case (which is less ugly) for
> > products on market (i.e. legacy and unwanted way for future platforms).
> 
> We really can't go back to what we had. Please keep in mind that it
> tied us to the USB PHY framework, possibly forever, and we shouldn't
> have to depend on two different PHY frameworks. If we have to register
> the PHY device in dwc3-pci.c then you should create new nop phy for
> the generic phy framework and use that instead. But before you do
> that, we better be damn sure there is no way to make ulpi bus work,
> and we are not there yet.

You have a point. But the transition should happen without creating
regressions. You cannot remove previous design while we don't have the
next one merged and functional.

> 
> 
> Have nice weekend guys,
> 

You too :)

Br, David

> -- 
> heikki

> From 0afd47616c03d268d1c0e2ad5f4108f7f1a2401d Mon Sep 17 00:00:00 2001
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Date: Mon, 12 Jan 2015 16:15:10 +0200
> Subject: [PATCH] usb: dwc3: pci: remove use of platform_data
> 
> This replaces the platform_data used on AMD Nolan SoC with
> generic device property.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/dwc3/dwc3-pci.c | 53 ++++++++++++++++++++++++---------------------
>  1 file changed, 28 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> index 8d95056..0f20139 100644
> --- a/drivers/usb/dwc3/dwc3-pci.c
> +++ b/drivers/usb/dwc3/dwc3-pci.c
> @@ -22,8 +22,6 @@
>  #include <linux/pci.h>
>  #include <linux/platform_device.h>
>  
> -#include "platform_data.h"
> -
>  /* FIXME define these in <linux/pci_ids.h> */
>  #define PCI_VENDOR_ID_SYNOPSYS		0x16c3
>  #define PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3	0xabcd
> @@ -37,34 +35,39 @@ static int dwc3_pci_quirks(struct pci_dev *pdev)
>  {
>  	if (pdev->vendor == PCI_VENDOR_ID_AMD &&
>  	    pdev->device == PCI_DEVICE_ID_AMD_NL_USB) {
> -		struct dwc3_platform_data pdata;
> -
> -		memset(&pdata, 0, sizeof(pdata));
> -
> -		pdata.has_lpm_erratum = true;
> -		pdata.lpm_nyet_threshold = 0xf;
> -
> -		pdata.u2exit_lfps_quirk = true;
> -		pdata.u2ss_inp3_quirk = true;
> -		pdata.req_p1p2p3_quirk = true;
> -		pdata.del_p1p2p3_quirk = true;
> -		pdata.del_phy_power_chg_quirk = true;
> -		pdata.lfps_filter_quirk = true;
> -		pdata.rx_detect_poll_quirk = true;
> -
> -		pdata.tx_de_emphasis_quirk = true;
> -		pdata.tx_de_emphasis = 1;
> -
> +		struct platform_device *dwc3 = pci_get_drvdata(pdev);
> +		struct dev_gen_prop *prop;
> +
> +		prop = devm_kcalloc(&pdev->dev, 14, sizeof(*prop), GFP_KERNEL);
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		prop[1].num = devm_kzalloc(&pdev->dev, sizeof(u8 *),
> +					   GFP_KERNEL);
> +		if (!prop[1].num)
> +			return -ENOMEM;
> +
> +		prop[0].name = "snps,has-lpm-erratum";
> +		prop[1].name = "snps,lpm-nyet-threshold";
> +		prop[1].nval = 1;
> +		prop[1].num[0] = 0xf;
> +		prop[2].name = "snps,u2exit_lfps_quirk";
> +		prop[3].name = "snps,u2ss_inp3_quirk";
> +		prop[4].name = "snps,req_p1p2p3_quirk";
> +		prop[5].name = "snps,del_p1p2p3_quirk";
> +		prop[6].name = "snps,del_phy_power_chg_quirk";
> +		prop[7].name = "snps,lfps_filter_quirk";
> +		prop[8].name = "snps,rx_detect_poll_quirk";
> +		prop[9].name = "snps,tx_de_emphasis_quirk";
>  		/*
>  		 * FIXME these quirks should be removed when AMD NL
>  		 * taps out
>  		 */
> -		pdata.disable_scramble_quirk = true;
> -		pdata.dis_u3_susphy_quirk = true;
> -		pdata.dis_u2_susphy_quirk = true;
> +		prop[10].name = "snps,disable_scramble_quirk";
> +		prop[11].name = "snps,dis_u3_susphy_quirk";
> +		prop[12].name = "snps,dis_u2_susphy_quirk";
>  
> -		return platform_device_add_data(pci_get_drvdata(pdev), &pdata,
> -						sizeof(pdata));
> +		dwc3->dev.gen_prop = prop;
>  	}
>  
>  	return 0;
> -- 
> 2.1.4
>