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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

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

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?

> 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 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?

> 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.


Have nice weekend guys,

-- 
heikki

[-- Attachment #2: 0001-usb-dwc3-pci-remove-use-of-platform_data.patch --]
[-- Type: text/plain, Size: 2858 bytes --]

>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