Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753352AbdFMNLx (ORCPT ); Tue, 13 Jun 2017 09:11:53 -0400 Received: from mga05.intel.com ([192.55.52.43]:16670 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752369AbdFMNLv (ORCPT ); Tue, 13 Jun 2017 09:11:51 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,338,1493708400"; d="scan'208";a="980237410" Subject: Re: [v2 1/1] usb:host:xhci support option to disable xHCI 1.0 USB2 HW LPM To: "Thang Q. Nguyen" , Mathias Nyman References: <1495265096-14851-1-git-send-email-tqnguyen@apm.com> <59353D0B.1070005@linux.intel.com> <59356BCA.6@intel.com> Cc: Greg Kroah-Hartman , Felipe Balbi , Rob Herring , Mark Rutland , linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Phong Vo , Loc Ho , Duc Tran , Quang Han , Tung Nguyen , patches From: Mathias Nyman Message-ID: <593FE4BC.4020709@linux.intel.com> Date: Tue, 13 Jun 2017 16:12:28 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3893 Lines: 84 On 06.06.2017 09:33, Thang Q. Nguyen wrote: > On Mon, Jun 5, 2017 at 9:33 PM, Mathias Nyman wrote: >> On 05.06.2017 15:57, Thang Q. Nguyen wrote: >>> >>> On Mon, Jun 5, 2017 at 6:14 PM, Mathias Nyman >>> 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