linux-kernel - Re: [PATCH 2/2] drivers: xhci: Add quirk to reset xHCI port PHY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d176e6bf-10b8-374c-1739-47bc98d12f09@linux.intel.com>
Date:   Fri, 8 Feb 2019 14:35:02 +0200
From:   Mathias Nyman <mathias.nyman@...ux.intel.com>
To:     Srinath Mannam <srinath.mannam@...adcom.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>, linux-usb@...r.kernel.org,
        devicetree@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] drivers: xhci: Add quirk to reset xHCI port PHY

On 07.02.2019 17:17, Srinath Mannam wrote:
> Hi Mathias,
> 
> Thanks for review, please see my comments below inline.
> 
> On Thu, Feb 7, 2019 at 8:32 PM Mathias Nyman
> <mathias.nyman@...ux.intel.com> wrote:
>>
>> On 05.02.2019 08:18, Srinath Mannam wrote:
>>> Add a quirk to reset xHCI port PHY on port disconnect event.
>>> Stingray USB HS PHY has an issue, that USB High Speed device detected
>>> at Full Speed after the same port has connected to Full speed device.
>>> This problem can be resolved with that port PHY reset on disconnect.
>>>
>>> Signed-off-by: Srinath Mannam <srinath.mannam@...adcom.com>
>>> Reviewed-by: Ray Jui <ray.jui@...adcom.com>
>>> ---
>>>    drivers/usb/core/hcd.c       |  6 ++++++
>>>    drivers/usb/core/phy.c       | 21 +++++++++++++++++++++
>>>    drivers/usb/core/phy.h       |  1 +
>>>    drivers/usb/host/xhci-plat.c |  3 +++
>>>    drivers/usb/host/xhci-ring.c |  9 ++++++---
>>>    drivers/usb/host/xhci.h      |  1 +
>>>    include/linux/usb/hcd.h      |  1 +
>>>    7 files changed, 39 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>>> index 015b126..e2b87a6 100644
>>> --- a/drivers/usb/core/hcd.c
>>> +++ b/drivers/usb/core/hcd.c
>>> @@ -2663,6 +2663,12 @@ int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1)
>>>        return hcd->driver->find_raw_port_number(hcd, port1);
>>>    }
>>>
>>> +int usb_hcd_phy_port_reset(struct usb_hcd *hcd, int port)
>>> +{
>>> +     return usb_phy_roothub_port_reset(hcd->phy_roothub, port);
>>> +}
>>> +EXPORT_SYMBOL_GPL(usb_hcd_phy_port_reset);
>>> +
>>>    static int usb_hcd_request_irqs(struct usb_hcd *hcd,
>>>                unsigned int irqnum, unsigned long irqflags)
>>>    {
>>> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
>>> index 38b2c77..c64767d 100644
>>> --- a/drivers/usb/core/phy.c
>>> +++ b/drivers/usb/core/phy.c
>>> @@ -162,6 +162,27 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
>>>    }
>>>    EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
>>>
>>> +int usb_phy_roothub_port_reset(struct usb_phy_roothub *phy_roothub, int port)
>>> +{
>>> +     struct usb_phy_roothub *roothub_entry;
>>> +     struct list_head *head;
>>> +     int i = 0;
>>> +
>>> +     if (!phy_roothub)
>>> +             return -EINVAL;
>>> +
>>> +     head = &phy_roothub->list;
>>> +
>>> +     list_for_each_entry(roothub_entry, head, list) {
>>> +             if (i == port)
>>> +                     return phy_reset(roothub_entry->phy);
>>> +             i++;
>>> +     }
>>
>> I'm not that familiar with SoC's that have several PHYs per controller,
>> but this looks odd.
>>
>> For the above code to work wouldn't it require that each port has their own PHY,
>> and the PHYs are added to the list of usb_phy_roothub is in the same order as usb ports?
>>
>> Or is there something I don't understand here?
>>
> In our SOC (Stingray), xHCI controller has three ports and each port
> connected to separate PHY.
> Stingray xHCI controller supports both SS and HS ports and connected
> separate PHYs.
> We passed PHY phandlers in xHCI DT node in the order of port numbers.
> as shown below xHCI DT node.
> So that all PHYs added to usb_phy_roothub are in order of port numbers.
>   xhci1: usb@...00 {
>                          compatible = "generic-xhci";
>                          reg = <0x00011000 0x1000>;
>                          interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH>;
>                          phys = <&usb1_phy1>, <&usbphy2>, <&usb1_phy0>;
>                          phy-names = "phy0", "phy1", "phy2";
>                          dma-coherent;
>                          status = "disabled";
> };
> But we have issue with HS PHYs, so that those PHYs are required to reset.

This is very specific to your SOC.
A quick grep shows most xhci controllers have one or two PHYs in total.

The suggested usb_phy_roothub_port_reset() above will only work for your SOC.
I think it would be better to have a stingray specific quirk/workaround where you
can find the right PHY to reset based on somthing like port number and PHY name.

For a generic solution we would need a better way to map usb ports to the PHY it uses.

-Mathias