Re: [v2 1/1] usb:host:xhci support option to disable xHCI 1.0 USB2 HW LPM

On 06.06.2017 09:33, Thang Q. Nguyen wrote:

On Mon, Jun 5, 2017 at 9:33 PM, Mathias Nyman [off-list ref] wrote:

quoted

On 05.06.2017 15:57, Thang Q. Nguyen wrote:

quoted

On Mon, Jun 5, 2017 at 6:14 PM, Mathias Nyman
[off-list ref] wrote:

quoted

On 20.05.2017 10:24, Thang Q. Nguyen wrote:

quoted

XHCI specification 1.1 does not require xHCI 1.0 compliant controllers
to always enable hardware USB2 LPM.
However, the current xHCI driver always enable it by setting HLE=1 when
seeing HLC=1. This makes certain xHCI controllers that have broken USB2
HW LPM fail to work as there is no way to disable this feature.
This patch adds support to control disabling USB2 Hardware LPM via
DT/ACPI attribute.

   Wouldn't it be better to just keep  xhci->hw_lpm_support = 0 if the
host
doesn't support Hardware LPM Capability, (HLC)?

This should prevent all those extra steps getting here just to do
nothing.

No, HLC = 0 means the host doesn't support Hardware LPM.
The problem here is the host support Hardware LPM but there is a bug
in host controller that make the LPM fail to work.

So the host support hw LPM, and has its HLC capability bit set,
but in the end it just doesn't work at all, and should be prevented.

quoted

When debugging the host controller, we detect there are some holes in
the current usb specifications which can can result in inter-operating
problems between USB Host Controller and USB PHY. To be more specific,
the specs have not clarified the resume recovery timing after the port
has just waken up from L1. This can lead to different interpretations
of the specs by Host Controller and PHY. What happened in our case is
that a Host controller cannot work with a PHY right after resuming
from L1 because these two Vendors have different views of the specs
regarding LPM timing after L1. These views are contradictory and
cannot work together.

If Host Controller and PHY are from the same vendor, they might have
some "internal handshake mechanisms" to avoid these holes of the USB
specs. However, these mechanisms are not standardized in USB specs;
and not all vendors follow these mechanisms. In fact, we have observed
this kind of "internal handshake mechanism" in HOST Controller and PHY
from SYNOPSYS DWC. So, we can say that if users use Host Controller
and PHY from different Vendors, the inteopering problems after waking
up from L1 are more likely to occur.

Can you explain the reason why you prefer preventing hw lpm inside the
xhci_set_usb2_hardware_lpm() function instead of preventing hw lpm usage
all together for this platform -i.e. by not setting xhci->hw_lpm_support

The reason I don't change in the xhci_add_in_port() function inside
xhci-mem.c is because of the description for xhci->hw_lpm_support in
the drivers/usb/host/xhci.h header file: support xHCI 1.0 spec USB2
hardware LPM. Per my understanding, this attribute is used to indicate
if the host supports HW LPM and this can be checked via HLC.
My intension is to support an option for user to disable the HW LPM
because of some reasons (in my case because HW LPM is broken).

I think we should keep supporting new options separate from workarounds
for broken HW.

quoted

Again, something like:
if (temp & XHCI_HLC && !(xhci->quirks & XHCI_HW_LPM_BROKEN))
         xhci->hw_lpm_support = 1;

This should work too. But the DT/ACPI attribute should change to
"usb2-lpm-broken".

This would be more clear for future developers and prevent them from
enabling hw lpm for this host to gain some powersaving.

A new feature allowing optional host hw lpm disabling can then be written separately,
preferable without using the quirk bits.

-Mathias