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

[Thang Q. Nguyen]

On Tue, Jun 13, 2017 at 8:12 PM, Mathias Nyman
<mathias.nyman@xxxxxxxxxxxxxxx> wrote:
> On 06.06.2017 09:33, Thang Q. Nguyen wrote:
>>
>> On Mon, Jun 5, 2017 at 9:33 PM, Mathias Nyman <mathias.nyman@xxxxxxxxx>
>> wrote:
>>>
>>> On 05.06.2017 15:57, Thang Q. Nguyen wrote:
>>>>
>>>>
>>>> On Mon, Jun 5, 2017 at 6:14 PM, Mathias Nyman
>>>> <mathias.nyman@xxxxxxxxxxxxxxx> wrote:
>>>>>
>>>>>
>>>>> On 20.05.2017 10:24, Thang Q. Nguyen wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> 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.
>>>
>>>> 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.
So, I will continue to use usb2-lpm-disable to let kernel know that we
want to disable USB2 HW LPM.
>
>>>
>>>
>>> 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.
How's about adding a new attribute such as sw_lpm_disable to the
xhci_hcd struct to indicate that we will disable USB2 HW LPM by
software?
>
> -Mathias