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@xxxxxxxxxxxxxxx/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@xxxxxxxxxxxxxxx> > 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@xxxxxxxxxxxxxxx> > --- > 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
