On Wed, Nov 1, 2017 at 4:17 PM, Thang Q. Nguyen <tqnguyen@xxxxxxx> wrote:
> Hi,
>
> On Mon, Oct 23, 2017 at 5:27 PM, Mathias Nyman
> <mathias.nyman@xxxxxxxxxxxxxxx> wrote:
>> On 23.10.2017 12:37, Thang Q. Nguyen wrote:
>>>
>>> Hi,
>>> In our latest ARM64-based CPU, we use the DesignWare USB which is
>>> xHCI-compatible. For some reasons, we disable USB3.0 support so remove the
>>> unnecessary USB3.0 capability structure information as specified in the
>>> xHCI specification 1.1, section 7.2:
>>> "At least one of these capability structures is required for all xHCI
>>> implementations. More than one may be defined for implementations that
>>> support more that one bus protocol."
>>>
>>
>> So there is no supported protocol capability with revision major == 3, in
>> your case.
>>
>>
>>> When booting kernel, linux kernel displays an error message to claim about
>>> no USB port on the USB3.0 roothub:
>>> [ 3.497767] hub 2-0:1.0: config failed, hub doesn't have any ports!
>>> (err -19)
>>> Although the error message does not affect USB functionality, error
>>> message will make users confused.
>>>
>>> Looking into the XHCI driver, there are always two USB hub ports
>>> implemented. This implements the xHCI specification as in 4.19.7:
>>> "In a USB3 hub, two independently addressable hub ports exist for each
>>> physical down stream connector; a USB2 compatible port accessed through
>>> the USB2 connection and a USB3 compatible port accessed through the
>>> SuperSpeed connection. The Root Hub of the xHCI emulates this operation by
>>> defining a Root Hub PORTSC register for each connection type; USB2
>>> (Low-/Full-/High-Speed) or USB3 (SuperSpeed)."
>>>
>>> But the problem is that, xhci-hcd always pass both USB hub ports to
>>> usb-core driver. In case any of USB2 or USB3 compatible port is missing,
>>> there is no port on the corresponding hub and kernel will displays the
>>> error message.
>>>
>>> Below are some approaches we suggest to overcome the issue:
>>> Approach 1: Allow roothub has no port. The change will look like:
>>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>>> index b5c7336..995d62d 100644
>>> --- a/drivers/usb/core/hub.c
>>> +++ b/drivers/usb/core/hub.c
>>> @@ -1347,6 +1347,9 @@ static int hub_configure(struct usb_hub
>>> *hub,
>>> ret = -ENODEV;
>>> goto fail;
>>> } else if (hub->descriptor->bNbrPorts == 0) {
>>> + if (hdev->parent)
>>> + return -ENODEV;
>>> +
>>> message = "hub doesn't have any
>>> ports!";
>>> ret = -ENODEV;
>>> goto fail;
>>> Approach 2: do not pass a USB hub port to usb-core if it has no port.
>>> Don't know if this approach is feasible or not.
>>> Approach 3: implement maximum-speed attribute support and this will be set
>>> to high-speed in case of missing USB3 support. But this works only for the
>>> case of USB3.0 disabled.
>>>
>>> Can you help review and suggest what approach should be used to fix this
>>> issue?
>>
>>
>> Current xhci driver design assumes there are two hcds available.
>> Several functions assume a shared hcd exists.
>>
>> Creating a hcd with a roothub with zero ports is probably a faster way to
>> get
>> things running, but I think we should redesign parts of the xhci driver to
>> work with only one hcd.
>>
>> So best would be to never add the secodary hcd if there are no USB 3 ports,
>> meaning no:
>> xhci->shared_hcd = usb_create_shared_hcd(..);
>> usb_add_hcd(xhci->shared_hcd, dev->irq,
> Below code is to not add shared_hcd if no USB 3 port:
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 1cb6eae..0881fb5 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -285,12 +285,14 @@ static int xhci_plat_probe(struct platform_device *pdev)
> if (ret)
> goto disable_usb_phy;
>
> - if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
> - xhci->shared_hcd->can_do_streams = 1;
> + if (xhci->num_usb3_ports > 0) {
> + if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
> + xhci->shared_hcd->can_do_streams = 1;
>
> - ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
> - if (ret)
> - goto dealloc_usb2_hcd;
> + ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
> + if (ret)
> + goto dealloc_usb2_hcd;
> + }
>
> device_enable_async_suspend(&pdev->dev);
> pm_runtime_put_noidle(&pdev->dev);
>
> Then, the xhci_run should execute xhci_start() in case no USB 3 port
> (xhci->shared_hcd->run() will execute xhci_run_finished() which run
> xhci_start().
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 51535ba..3ad41aa 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -630,6 +630,16 @@ int xhci_run(struct usb_hcd *hcd)
> if (ret)
> xhci_free_command(xhci, command);
> }
> +
> + /* Execute xhci_start() in case xhci->shared_hcd is not registered */
> + if (xhci->num_usb3_ports > 0) {
> + if (xhci_start(xhci)) {
> + xhci_halt(xhci);
> + return -ENODEV;
> + }
> + xhci->cmd_ring_state = CMD_RING_STATE_RUNNING;
> + }
> +
> xhci_dbg_trace(xhci, trace_xhci_dbg_init,
> "Finished xhci_run for USB2 roothub");
> return 0;
>
>>
>> Instead we need to make sure xhci->shared_hcd is set to NULL,
>> and modify at least the following functions to work with only one xhci hcd:
> Do we really need to set xhci->shared_hcd to NULL? when
> xhci->shared_hcd is not registered to usb_core, no operation work on
> it.
>>
>> xhci_run()
>> xhci_stop()
>> xhci_suspend()
>> xhci_resume()
>> xhci_gen_setup()
>>
>> More changes are very likely needed, but this is a place to start
>>
>> -Mathias
>>
>>
> - ThangQ.
Hi Mathias,
Do you have any comment on my approach?
Regards,
ThangQ.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
