Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1664837 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [v2 1/1] usb:host:xhci support option to disable xHCI 1.0 USB2 HW  LPM Mathias Nyman <mathias.nyman@linux.intel.com> - 2017-06-13 15:20 +0200
    Re: [v2 1/1] usb:host:xhci support option to disable xHCI 1.0 USB2 HW LPM "Thang Q. Nguyen" <tqnguyen@apm.com> - 2017-06-14 15:30 +0200

#1664837 — 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 <mathias.nyman@intel.com> wrote:
>> On 05.06.2017 15:57, Thang Q. Nguyen wrote:
>>>
>>> On Mon, Jun 5, 2017 at 6:14 PM, Mathias Nyman
>>> <mathias.nyman@linux.intel.com> 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.

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

[toc] | [next] | [standalone]

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

On Tue, Jun 13, 2017 at 8:12 PM, Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
> On 06.06.2017 09:33, Thang Q. Nguyen wrote:
>>
>> On Mon, Jun 5, 2017 at 9:33 PM, Mathias Nyman <mathias.nyman@intel.com>
>> wrote:
>>>
>>> On 05.06.2017 15:57, Thang Q. Nguyen wrote:
>>>>
>>>>
>>>> On Mon, Jun 5, 2017 at 6:14 PM, Mathias Nyman
>>>> <mathias.nyman@linux.intel.com> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web