The driver methods required for hardware LPM have only been added to the
PCI version of the XHCI driver, for no apparent reason. They seem to
work just as well with the platform driver, so let's add them to give
more devices the chance for additional power savings. Tested on the DWC3
xHC of an Exynos5420.
Signed-off-by: Julius Werner <[email protected]>
---
drivers/usb/host/xhci-plat.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 51e22bf..7f46b5d 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -80,6 +80,10 @@ static const struct hc_driver xhci_plat_xhci_driver = {
.hub_status_data = xhci_hub_status_data,
.bus_suspend = xhci_bus_suspend,
.bus_resume = xhci_bus_resume,
+
+ /* LPM support */
+ .update_device = xhci_update_device,
+ .set_usb2_hw_lpm = xhci_set_usb2_hardware_lpm,
};
static int xhci_plat_probe(struct platform_device *pdev)
--
1.7.12.4
Hi Julius,
Thanks for the patch! Did you test with a USB analyzer to see if the
device was actually going into USB 2.0 Link PM? I'd like to confirm we
really aren't breaking anything for DW3 hosts by enabling this.
Sarah Sharp
On Tue, Aug 20, 2013 at 04:21:49PM -0700, Julius Werner wrote:
> The driver methods required for hardware LPM have only been added to the
> PCI version of the XHCI driver, for no apparent reason. They seem to
> work just as well with the platform driver, so let's add them to give
> more devices the chance for additional power savings. Tested on the DWC3
> xHC of an Exynos5420.
>
> Signed-off-by: Julius Werner <[email protected]>
> ---
> drivers/usb/host/xhci-plat.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 51e22bf..7f46b5d 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -80,6 +80,10 @@ static const struct hc_driver xhci_plat_xhci_driver = {
> .hub_status_data = xhci_hub_status_data,
> .bus_suspend = xhci_bus_suspend,
> .bus_resume = xhci_bus_resume,
> +
> + /* LPM support */
> + .update_device = xhci_update_device,
> + .set_usb2_hw_lpm = xhci_set_usb2_hardware_lpm,
> };
>
> static int xhci_plat_probe(struct platform_device *pdev)
> --
> 1.7.12.4
>
> Thanks for the patch! Did you test with a USB analyzer to see if the
> device was actually going into USB 2.0 Link PM? I'd like to confirm we
> really aren't breaking anything for DW3 hosts by enabling this.
Yes, I did. The LPM transfers on the analyzer look good and the device
works as expected.
My platform only runs a 3.8 derivative, though, so I now have also
cherry-picked the BESL patches that went in since then to make sure
they don't break things. I had problems on one device until I found
the XHCI_BLC fix Mathias sent out this morning, so you should pick
that one up first. (Without it LPM doesn't break completely but seems
to assert resume for less time than the HIRD defines, so the device
sometimes gets confused and resets. I can't figure out how BESL is
really supposed to work since the XHCI spec from 8/14/12 where it's
supposedly defined doesn't seem to be publicly available.)
On Wed, Aug 21, 2013 at 02:43:55PM -0700, Julius Werner wrote:
> > Thanks for the patch! Did you test with a USB analyzer to see if the
> > device was actually going into USB 2.0 Link PM? I'd like to confirm we
> > really aren't breaking anything for DW3 hosts by enabling this.
>
> Yes, I did. The LPM transfers on the analyzer look good and the device
> works as expected.
Ok, good to know. Greg, is your USB tree still open for feature
requests? I wouldn't normally send you usb-next requests so late, but
this is pretty simple patch to extend a feature that's already in your
usb-next tree.
> My platform only runs a 3.8 derivative, though, so I now have also
> cherry-picked the BESL patches that went in since then to make sure
> they don't break things. I had problems on one device until I found
> the XHCI_BLC fix Mathias sent out this morning, so you should pick
> that one up first. (Without it LPM doesn't break completely but seems
> to assert resume for less time than the HIRD defines, so the device
> sometimes gets confused and resets. I can't figure out how BESL is
> really supposed to work since the XHCI spec from 8/14/12 where it's
> supposedly defined doesn't seem to be publicly available.)
You need the USB 2.0 spec errata from 2011-11 that describes the changes
made for BESL as well. It's in the USB2-LPM-Errata-final.pdf and
USB2_LinkPowerMangement_ECN[final].pdf files in this zip file:
http://www.usb.org/developers/docs/usb_20_070113.zip
I agree though, it's all a confusing mess for documentation on USB 2.0
Link PM.
Sarah Sharp
On Wed, Aug 21, 2013 at 05:45:14PM -0700, Sarah Sharp wrote:
> On Wed, Aug 21, 2013 at 02:43:55PM -0700, Julius Werner wrote:
> > > Thanks for the patch! Did you test with a USB analyzer to see if the
> > > device was actually going into USB 2.0 Link PM? I'd like to confirm we
> > > really aren't breaking anything for DW3 hosts by enabling this.
> >
> > Yes, I did. The LPM transfers on the analyzer look good and the device
> > works as expected.
>
> Ok, good to know. Greg, is your USB tree still open for feature
> requests? I wouldn't normally send you usb-next requests so late, but
> this is pretty simple patch to extend a feature that's already in your
> usb-next tree.
Depends on how much you trust it and how bad it will break other things :)
> You need the USB 2.0 spec errata from 2011-11 that describes the changes
> made for BESL as well. It's in the USB2-LPM-Errata-final.pdf and
> USB2_LinkPowerMangement_ECN[final].pdf files in this zip file:
>
> http://www.usb.org/developers/docs/usb_20_070113.zip
>
> I agree though, it's all a confusing mess for documentation on USB 2.0
> Link PM.
Thanks, I hadn't found that first one yet. Do you also have a link for
the updated XHCI specification or a separate erratum document
describing the PORTHLPMC register referenced in the BESL patches (they
don't exist on DWC3, but I'm still curious)?
> From: Julius Werner
> Sent: Wednesday, August 21, 2013 9:22 PM
>
> > You need the USB 2.0 spec errata from 2011-11 that describes the changes
> > made for BESL as well. It's in the USB2-LPM-Errata-final.pdf and
> > USB2_LinkPowerMangement_ECN[final].pdf files in this zip file:
> >
> > http://www.usb.org/developers/docs/usb_20_070113.zip
> >
> > I agree though, it's all a confusing mess for documentation on USB 2.0
> > Link PM.
>
> Thanks, I hadn't found that first one yet. Do you also have a link for
> the updated XHCI specification or a separate erratum document
> describing the PORTHLPMC register referenced in the BESL patches (they
> don't exist on DWC3, but I'm still curious)?
Just to set the record straight :)
The PORTHLPMC registers do exist on DWC3, starting with the 1.90a version
of the core or thereabouts. They only supported the HIRD flavor of LPM,
though. Only fairly recently has support for BESL been added, around
version 2.41a or so.
--
Paul
On Thu, Aug 22, 2013 at 05:11:49AM +0000, Paul Zimmerman wrote:
> > From: Julius Werner
> > Sent: Wednesday, August 21, 2013 9:22 PM
> >
> > > You need the USB 2.0 spec errata from 2011-11 that describes the changes
> > > made for BESL as well. It's in the USB2-LPM-Errata-final.pdf and
> > > USB2_LinkPowerMangement_ECN[final].pdf files in this zip file:
> > >
> > > http://www.usb.org/developers/docs/usb_20_070113.zip
> > >
> > > I agree though, it's all a confusing mess for documentation on USB 2.0
> > > Link PM.
> >
> > Thanks, I hadn't found that first one yet. Do you also have a link for
> > the updated XHCI specification or a separate erratum document
> > describing the PORTHLPMC register referenced in the BESL patches (they
> > don't exist on DWC3, but I'm still curious)?
>
> Just to set the record straight :)
>
> The PORTHLPMC registers do exist on DWC3, starting with the 1.90a version
> of the core or thereabouts. They only supported the HIRD flavor of LPM,
> though. Only fairly recently has support for BESL been added, around
> version 2.41a or so.
And the 2.41a version that supports BESL properly sets the BLC flag in
the USB 2.0 Protocol extended capabilities for all the USB 2.0 ports?
Has this functionality been well-tested?
Sarah Sharp
> From: Sarah Sharp [mailto:[email protected]]
> Sent: Monday, August 26, 2013 12:14 PM
>
> On Thu, Aug 22, 2013 at 05:11:49AM +0000, Paul Zimmerman wrote:
> > > From: Julius Werner
> > > Sent: Wednesday, August 21, 2013 9:22 PM
> > >
> > > > You need the USB 2.0 spec errata from 2011-11 that describes the changes
> > > > made for BESL as well. It's in the USB2-LPM-Errata-final.pdf and
> > > > USB2_LinkPowerMangement_ECN[final].pdf files in this zip file:
> > > >
> > > > http://www.usb.org/developers/docs/usb_20_070113.zip
> > > >
> > > > I agree though, it's all a confusing mess for documentation on USB 2.0
> > > > Link PM.
> > >
> > > Thanks, I hadn't found that first one yet. Do you also have a link for
> > > the updated XHCI specification or a separate erratum document
> > > describing the PORTHLPMC register referenced in the BESL patches (they
> > > don't exist on DWC3, but I'm still curious)?
> >
> > Just to set the record straight :)
> >
> > The PORTHLPMC registers do exist on DWC3, starting with the 1.90a version
> > of the core or thereabouts. They only supported the HIRD flavor of LPM,
> > though. Only fairly recently has support for BESL been added, around
> > version 2.41a or so.
>
> And the 2.41a version that supports BESL properly sets the BLC flag in
> the USB 2.0 Protocol extended capabilities for all the USB 2.0 ports?
> Has this functionality been well-tested?
In 2.41a it is described as an "early adopter" feature in the databook,
and no mention is made of the BLC flag. So the answer there is "maybe".
Starting with 2.50a it is a full-fledged feature and the databook does
describe the BLC flag.
So it may be safer to say that the feature is present starting with 2.50a.
In 2.51a it has been well-tested in simulation. In actual hardware, it
has only been briefly tested in an ad-hoc sort of way, since none of the
standard drivers in the market support the feature yet, as far as we know.
Once the support is fully working in the Linux driver we can try testing
it there.
--
Paul
> From: Paul Zimmerman
> Sent: Monday, August 26, 2013 12:38 PM
...
> So it may be safer to say that the feature is present starting with 2.50a.
>
> In 2.51a it has been well-tested in simulation. In actual hardware, it
That should have said:
"In 2.50a it has been well-tested in simulation..."
--
Paul
On Mon, Aug 26, 2013 at 07:37:56PM +0000, Paul Zimmerman wrote:
> > From: Sarah Sharp [mailto:[email protected]]
> > Sent: Monday, August 26, 2013 12:14 PM
> >
> > On Thu, Aug 22, 2013 at 05:11:49AM +0000, Paul Zimmerman wrote:
> > > > From: Julius Werner
> > > > Sent: Wednesday, August 21, 2013 9:22 PM
> > > >
> > > > > You need the USB 2.0 spec errata from 2011-11 that describes the changes
> > > > > made for BESL as well. It's in the USB2-LPM-Errata-final.pdf and
> > > > > USB2_LinkPowerMangement_ECN[final].pdf files in this zip file:
> > > > >
> > > > > http://www.usb.org/developers/docs/usb_20_070113.zip
> > > > >
> > > > > I agree though, it's all a confusing mess for documentation on USB 2.0
> > > > > Link PM.
> > > >
> > > > Thanks, I hadn't found that first one yet. Do you also have a link for
> > > > the updated XHCI specification or a separate erratum document
> > > > describing the PORTHLPMC register referenced in the BESL patches (they
> > > > don't exist on DWC3, but I'm still curious)?
> > >
> > > Just to set the record straight :)
> > >
> > > The PORTHLPMC registers do exist on DWC3, starting with the 1.90a version
> > > of the core or thereabouts. They only supported the HIRD flavor of LPM,
> > > though. Only fairly recently has support for BESL been added, around
> > > version 2.41a or so.
> >
> > And the 2.41a version that supports BESL properly sets the BLC flag in
> > the USB 2.0 Protocol extended capabilities for all the USB 2.0 ports?
> > Has this functionality been well-tested?
>
> In 2.41a it is described as an "early adopter" feature in the databook,
> and no mention is made of the BLC flag. So the answer there is "maybe".
> Starting with 2.50a it is a full-fledged feature and the databook does
> describe the BLC flag.
So the 2.41a has BESL support, but may not set the BLC flag. What
happens if we use the HIRD encoding instead? Will things break? It
seems like we would need to disable USB 2.0 LPM on that host all
together, if it expects BESL encoding, but advertises HIRD encoding.
> So it may be safer to say that the feature is present starting with 2.50a.
Is there a way we can tell the difference between a 2.41a xHCI host and
a 2.50a host? If so, we can add a quirk to disable LPM on the 2.41a
host.
> In 2.51a it has been well-tested in simulation. In actual hardware, it
> has only been briefly tested in an ad-hoc sort of way, since none of the
> standard drivers in the market support the feature yet, as far as we know.
>
> Once the support is fully working in the Linux driver we can try testing
> it there.
Can you please test Julius' patch on the 2.41a, 2.50a, and 2.51a hosts?
Please test against usb-next, since that includes a fix for the BESL
patches.
I'm not going to merge this unless we know it's not going to break those
earlier host controllers. Besides, at this point, it's too late to
include this patch in 3.12. I already got bitten yesterday by trying to
push patches this late, and I'm not doing that again.
Sarah Sharp
> From: Sarah Sharp [mailto:[email protected]]
> Sent: Wednesday, August 28, 2013 2:52 PM
>
> On Mon, Aug 26, 2013 at 07:37:56PM +0000, Paul Zimmerman wrote:
> > > From: Sarah Sharp [mailto:[email protected]]
> > > Sent: Monday, August 26, 2013 12:14 PM
> > >
> > > On Thu, Aug 22, 2013 at 05:11:49AM +0000, Paul Zimmerman wrote:
> > > > > From: Julius Werner
> > > > > Sent: Wednesday, August 21, 2013 9:22 PM
> > > > >
> > > > > > You need the USB 2.0 spec errata from 2011-11 that describes the changes
> > > > > > made for BESL as well. It's in the USB2-LPM-Errata-final.pdf and
> > > > > > USB2_LinkPowerMangement_ECN[final].pdf files in this zip file:
> > > > > >
> > > > > > http://www.usb.org/developers/docs/usb_20_070113.zip
> > > > > >
> > > > > > I agree though, it's all a confusing mess for documentation on USB 2.0
> > > > > > Link PM.
> > > > >
> > > > > Thanks, I hadn't found that first one yet. Do you also have a link for
> > > > > the updated XHCI specification or a separate erratum document
> > > > > describing the PORTHLPMC register referenced in the BESL patches (they
> > > > > don't exist on DWC3, but I'm still curious)?
> > > >
> > > > Just to set the record straight :)
> > > >
> > > > The PORTHLPMC registers do exist on DWC3, starting with the 1.90a version
> > > > of the core or thereabouts. They only supported the HIRD flavor of LPM,
> > > > though. Only fairly recently has support for BESL been added, around
> > > > version 2.41a or so.
> > >
> > > And the 2.41a version that supports BESL properly sets the BLC flag in
> > > the USB 2.0 Protocol extended capabilities for all the USB 2.0 ports?
> > > Has this functionality been well-tested?
> >
> > In 2.41a it is described as an "early adopter" feature in the databook,
> > and no mention is made of the BLC flag. So the answer there is "maybe".
> > Starting with 2.50a it is a full-fledged feature and the databook does
> > describe the BLC flag.
>
> So the 2.41a has BESL support, but may not set the BLC flag. What
> happens if we use the HIRD encoding instead? Will things break? It
> seems like we would need to disable USB 2.0 LPM on that host all
> together, if it expects BESL encoding, but advertises HIRD encoding.
I imagine things would break, but I don't know for sure. I don't have a
2.41a version of the core to test this with.
Maybe the LPM support should be disabled by default, and enabled with a
quirk? That seems safer to me.
> > So it may be safer to say that the feature is present starting with 2.50a.
>
> Is there a way we can tell the difference between a 2.41a xHCI host and
> a 2.50a host? If so, we can add a quirk to disable LPM on the 2.41a
> host.
Once you know it is a Synopsys core, there is a vendor-specific register
that shows the version. But that register is at offset 0xC120, which is
above the normal xHCI register space I believe, so we may not be able to
depend on it being accessible. And you have the problem of how to
determine if it is a Synopsys core to begin with.
So IMHO, having LPM disabled by default, and enabled with a quirk based
on the PCI Vendor/Product ID, or a DT attribute for platform devices,
would be the way to go.
> > In 2.51a it has been well-tested in simulation. In actual hardware, it
> > has only been briefly tested in an ad-hoc sort of way, since none of the
> > standard drivers in the market support the feature yet, as far as we know.
> >
> > Once the support is fully working in the Linux driver we can try testing
> > it there.
>
> Can you please test Julius' patch on the 2.41a, 2.50a, and 2.51a hosts?
> Please test against usb-next, since that includes a fix for the BESL
> patches.
As I said, I don't have the 2.41a version available to test. I do have
2.50a and 2.60a available, so I can try those.
--
Paul
> So the 2.41a has BESL support, but may not set the BLC flag. What
> happens if we use the HIRD encoding instead? Will things break? It
> seems like we would need to disable USB 2.0 LPM on that host all
> together, if it expects BESL encoding, but advertises HIRD encoding.
Wait a second, just for clarity: are you saying that BESL-capable
controllers do not support the old HIRD mechanism and thus just break
on non-BESL aware OSes? I would've assumed that they somehow notice if
software doesn't write to the new register and automatically fall back
to HIRD... it seems like a weird decision to break hardware backwards
compatibility like that (after all, it would mean that Linux 3.10 and
older would also break on LynxPoint systems right now).
On Wed, Aug 28, 2013 at 04:08:12PM -0700, Julius Werner wrote:
> > So the 2.41a has BESL support, but may not set the BLC flag. What
> > happens if we use the HIRD encoding instead? Will things break? It
> > seems like we would need to disable USB 2.0 LPM on that host all
> > together, if it expects BESL encoding, but advertises HIRD encoding.
>
> Wait a second, just for clarity: are you saying that BESL-capable
> controllers do not support the old HIRD mechanism and thus just break
> on non-BESL aware OSes?
Yes.
> I would've assumed that they somehow notice if software doesn't write
> to the new register and automatically fall back to HIRD... it seems
> like a weird decision to break hardware backwards compatibility like
> that (after all, it would mean that Linux 3.10 and older would also
> break on LynxPoint systems right now).
Let me dig this older state out of my brain. ISTR yelling at the xHCI
spec architect for breaking hosts for this very reason (originally the
BLC flag was not in the spec at all)...
If a host supports the HIRD encoding, it sets Hardware LMP Capability
(HLC), bit 19, in the USB 2.0 port protocol register. That bit is set
whether the host supports HIRD or BESL encoding. Bit 20 (BLC) is set if
the host supports BESL.
When the driver goes to write a value in the USB2 Port Hardware LPM
Control Register, if the driver is only aware of the HIRD
specifications, it will use the HIRD encodings in bits 13:10, regardless
of whether the host has BESL support instead of HIRD support. If the
driver has support for BESL and the host has BESL support, it will use
the BESL encodings in those bits instead. If you take a look at Table
13: BESL/HIRD Encoding from the xHCI spec version including errata to
08/14/2012, you'll see the numbers written into bits 13:10 mean
different timeouts, based on whether the host supports HIRD or BESL.
So, basically, if the xHCI driver only supports HIRD and is loaded on a
host that supports BESL, then the driver will write a timeout value in
bits 13:10 that means something different than what the driver thinks it
means. This could lead to issues with USB 2.0 devices that support Link
PM.
Yes, this is broken. Distros that want to run on hosts that have BESL
support need to have the BESL patches from Mathias.
Sarah Sharp
On Wed, Aug 28, 2013 at 10:15:34PM +0000, Paul Zimmerman wrote:
> > From: Sarah Sharp [mailto:[email protected]]
> > Sent: Wednesday, August 28, 2013 2:52 PM
> >
> > On Mon, Aug 26, 2013 at 07:37:56PM +0000, Paul Zimmerman wrote:
> > > > From: Sarah Sharp [mailto:[email protected]]
> > > > Sent: Monday, August 26, 2013 12:14 PM
> > > >
> > > > On Thu, Aug 22, 2013 at 05:11:49AM +0000, Paul Zimmerman wrote:
> > > > > Just to set the record straight :)
> > > > >
> > > > > The PORTHLPMC registers do exist on DWC3, starting with the 1.90a version
> > > > > of the core or thereabouts. They only supported the HIRD flavor of LPM,
> > > > > though. Only fairly recently has support for BESL been added, around
> > > > > version 2.41a or so.
> > > >
> > > > And the 2.41a version that supports BESL properly sets the BLC flag in
> > > > the USB 2.0 Protocol extended capabilities for all the USB 2.0 ports?
> > > > Has this functionality been well-tested?
> > >
> > > In 2.41a it is described as an "early adopter" feature in the databook,
> > > and no mention is made of the BLC flag. So the answer there is "maybe".
> > > Starting with 2.50a it is a full-fledged feature and the databook does
> > > describe the BLC flag.
> >
> > So the 2.41a has BESL support, but may not set the BLC flag. What
> > happens if we use the HIRD encoding instead? Will things break? It
> > seems like we would need to disable USB 2.0 LPM on that host all
> > together, if it expects BESL encoding, but advertises HIRD encoding.
>
> I imagine things would break, but I don't know for sure. I don't have a
> 2.41a version of the core to test this with.
>
> Maybe the LPM support should be disabled by default, and enabled with a
> quirk? That seems safer to me.
I don't think that's a sustainable option.
I expect that the majority of hosts that support USB 2.0 Link PM in the
future will have BESL support. It makes no sense to maintain an
ever-growing list of hosts that support BESL.
We did something similar for the Intel EHCI to xHCI port switchover.
Every time someone added a new skew with a different PCI device ID, we
had to add that to the list of hosts that had the port switchover. The
list grew and grew, and backporting and notifying distros of new list
entries was a pain. It just wasn't sustainable, and we ended up ripping
out the list and dynamically looking for the Intel EHCI companion host
instead.
So, no, I don't want to go there. I would rather have an xHCI quirk
that disables USB 2.0 LPM instead.
> > > So it may be safer to say that the feature is present starting with 2.50a.
> >
> > Is there a way we can tell the difference between a 2.41a xHCI host and
> > a 2.50a host? If so, we can add a quirk to disable LPM on the 2.41a
> > host.
>
> Once you know it is a Synopsys core, there is a vendor-specific register
> that shows the version. But that register is at offset 0xC120, which is
> above the normal xHCI register space I believe, so we may not be able to
> depend on it being accessible. And you have the problem of how to
> determine if it is a Synopsys core to begin with.
>
> So IMHO, having LPM disabled by default, and enabled with a quirk based
> on the PCI Vendor/Product ID, or a DT attribute for platform devices,
> would be the way to go.
I think DT attributes would be the best way to go. I think the patch
should be changed to set those USB 2.0 LPM function pointers for the
platform devices, but add a new xHCI host quirk, XHCI_DISABLE_USB2_LPM.
Make the LPM functions return immediately if that quirk is set. Then
set the quirk based on DT attributes for the Synopsis 2.41a host.
> > > In 2.51a it has been well-tested in simulation. In actual hardware, it
> > > has only been briefly tested in an ad-hoc sort of way, since none of the
> > > standard drivers in the market support the feature yet, as far as we know.
> > >
> > > Once the support is fully working in the Linux driver we can try testing
> > > it there.
> >
> > Can you please test Julius' patch on the 2.41a, 2.50a, and 2.51a hosts?
> > Please test against usb-next, since that includes a fix for the BESL
> > patches.
>
> As I said, I don't have the 2.41a version available to test. I do have
> 2.50a and 2.60a available, so I can try those.
Ok, let me know if those work. In the meantime, can you help Julius
create a patch to add DT attributes to distinguish between the different
versions?
Sarah Sharp
> If you take a look at Table
> 13: BESL/HIRD Encoding from the xHCI spec version including errata to
> 08/14/2012
Could you please provide a link to that errata? I still cannot find
it... but from your explanation, that design decision sounds pretty
horrible. Why didn't they just choose not to set old HLC flag in BESL
controllers? Seems like the only purpose it provides there is to make
old drivers break.
Anyway, looks like we are stuck with it now, and need to deal with
those mislabeled DWC3 versions. I agree with you that we should
blacklist instead of whitelist, but I don't think the device tree is
the best place to put that... we would have to figure out the exact
DWC3 version for every processor/SoC dtsi file to determine if they
are affected, and remember to keep that up to date as we added more.
I would instead propose to check for the revision register directly in
the DWC3 stack. I think I could add a little check to dwc3_host_init()
and hack the quirk bit into the newly created XHCI controller instance
if required. However, I only have an old (unaffected) 1.85 controller
for testing, so I would need Synopsys to provide me with the exact
revision numbers affected (as read from the register) and to test the
change for us.
Also, do we want to make it an XHCI_DISABLE_USB2_LPM or a
XHCI_FORCE_USB2_BESL quirk? Assuming their BESL implementation is
otherwise correct except for omitting that bit, the latter one should
make those controllers work better.
> From: Sarah Sharp [mailto:[email protected]]
> Sent: Thursday, August 29, 2013 10:42 AM
>
> On Wed, Aug 28, 2013 at 10:15:34PM +0000, Paul Zimmerman wrote:
> > > From: Sarah Sharp [mailto:[email protected]]
> > > Sent: Wednesday, August 28, 2013 2:52 PM
> > >
> > > On Mon, Aug 26, 2013 at 07:37:56PM +0000, Paul Zimmerman wrote:
> > > > > From: Sarah Sharp [mailto:[email protected]]
> > > > > Sent: Monday, August 26, 2013 12:14 PM
> > > > >
> > > > > On Thu, Aug 22, 2013 at 05:11:49AM +0000, Paul Zimmerman wrote:
> > > > > > Just to set the record straight :)
> > > > > >
> > > > > > The PORTHLPMC registers do exist on DWC3, starting with the 1.90a version
> > > > > > of the core or thereabouts. They only supported the HIRD flavor of LPM,
> > > > > > though. Only fairly recently has support for BESL been added, around
> > > > > > version 2.41a or so.
> > > > >
> > > > > And the 2.41a version that supports BESL properly sets the BLC flag in
> > > > > the USB 2.0 Protocol extended capabilities for all the USB 2.0 ports?
> > > > > Has this functionality been well-tested?
> > > >
> > > > In 2.41a it is described as an "early adopter" feature in the databook,
> > > > and no mention is made of the BLC flag. So the answer there is "maybe".
> > > > Starting with 2.50a it is a full-fledged feature and the databook does
> > > > describe the BLC flag.
> > >
> > > So the 2.41a has BESL support, but may not set the BLC flag. What
> > > happens if we use the HIRD encoding instead? Will things break? It
> > > seems like we would need to disable USB 2.0 LPM on that host all
> > > together, if it expects BESL encoding, but advertises HIRD encoding.
> >
> > I imagine things would break, but I don't know for sure. I don't have a
> > 2.41a version of the core to test this with.
> >
> > Maybe the LPM support should be disabled by default, and enabled with a
> > quirk? That seems safer to me.
>
> I don't think that's a sustainable option.
>
> I expect that the majority of hosts that support USB 2.0 Link PM in the
> future will have BESL support. It makes no sense to maintain an
> ever-growing list of hosts that support BESL.
>
> We did something similar for the Intel EHCI to xHCI port switchover.
> Every time someone added a new skew with a different PCI device ID, we
> had to add that to the list of hosts that had the port switchover. The
> list grew and grew, and backporting and notifying distros of new list
> entries was a pain. It just wasn't sustainable, and we ended up ripping
> out the list and dynamically looking for the Intel EHCI companion host
> instead.
Yes, but that was a case of things not working at all, correct? The worst
that can happen with LPM disabled is that a new host will consume a little
more power than necessary, until someone gets around to adding a quirk
for it. Whereas if you enable it by default, things could be broken until
the quirk is added.
> So, no, I don't want to go there. I would rather have an xHCI quirk
> that disables USB 2.0 LPM instead.
...
> I think DT attributes would be the best way to go. I think the patch
> should be changed to set those USB 2.0 LPM function pointers for the
> platform devices, but add a new xHCI host quirk, XHCI_DISABLE_USB2_LPM.
> Make the LPM functions return immediately if that quirk is set. Then
> set the quirk based on DT attributes for the Synopsis 2.41a host.
>
> > > > In 2.51a it has been well-tested in simulation. In actual hardware, it
> > > > has only been briefly tested in an ad-hoc sort of way, since none of the
> > > > standard drivers in the market support the feature yet, as far as we know.
> > > >
> > > > Once the support is fully working in the Linux driver we can try testing
> > > > it there.
> > >
> > > Can you please test Julius' patch on the 2.41a, 2.50a, and 2.51a hosts?
> > > Please test against usb-next, since that includes a fix for the BESL
> > > patches.
> >
> > As I said, I don't have the 2.41a version available to test. I do have
> > 2.50a and 2.60a available, so I can try those.
>
> Ok, let me know if those work. In the meantime, can you help Julius
> create a patch to add DT attributes to distinguish between the different
> versions?
I don't think there's a need to distinguish between different versions,
is there? Don't we need just a single DT attribute that means "does not
support BESL"?
--
Paul
> From: [email protected] [mailto:[email protected]] On Behalf Of Julius Werner
> Sent: Thursday, August 29, 2013 11:07 AM
>
> > If you take a look at Table
> > 13: BESL/HIRD Encoding from the xHCI spec version including errata to
> > 08/14/2012
>
> Could you please provide a link to that errata? I still cannot find
> it... but from your explanation, that design decision sounds pretty
> horrible. Why didn't they just choose not to set old HLC flag in BESL
> controllers? Seems like the only purpose it provides there is to make
> old drivers break.
>
> Anyway, looks like we are stuck with it now, and need to deal with
> those mislabeled DWC3 versions. I agree with you that we should
> blacklist instead of whitelist, but I don't think the device tree is
> the best place to put that... we would have to figure out the exact
> DWC3 version for every processor/SoC dtsi file to determine if they
> are affected, and remember to keep that up to date as we added more.
>
> I would instead propose to check for the revision register directly in
> the DWC3 stack. I think I could add a little check to dwc3_host_init()
> and hack the quirk bit into the newly created XHCI controller instance
> if required. However, I only have an old (unaffected) 1.85 controller
> for testing, so I would need Synopsys to provide me with the exact
> revision numbers affected (as read from the register) and to test the
> change for us.
OK, I did a little more digging, and it turns out the 2.41a version
_does_ set bit 20 in the protocol defined field if BESL support is
enabled. It wasn't mentioned in the "registers" section of the databook,
but there is a note to that effect in a different section.
So it looks like we don't need to worry about this for the DWC3
controllers, anyway. Sorry for the noise.
--
Paul