Re: [RFC PATCH v2 1/3] usb: support Samsung Exynos xHCI Controller

[Krzysztof Kozlowski]

On 02/01/2023 07:24, Jung Daehwan wrote:
> On Thu, Dec 29, 2022 at 11:25:58AM +0100, Krzysztof Kozlowski wrote:
>> On 29/12/2022 10:57, Daehwan Jung wrote:
>>> Currently, dwc3 invokes just xhci platform driver without any data.
>>> We add xhci node as child of dwc3 node in order to get data from
>>> device tree. It populates "xhci" child by name during initialization
>>> of host. This patch only effects if dwc3 node has a child named "xhci"
>>> not to disturb original path.
>>>
>>> We add "samsung,exynos-xhci" compatible in xhci platform driver
>>
>> Where? It is not documented.
> 
> I submitted the patch of dt bindings on same patchset.
> Is there any missing documentation?

This is your first patch in the series and in this patch there is no
such bindings. Re-order the patches to have proper order.

> 
>>
>>> to support Exynos SOCs. 
>>
>> That's so not true. You do nothing to support Exynos SoC here. Please
>> stop pasting incorrect and misleading commit msgs.
> 
> I agree misleading commit msgs. I will modify it.
> 
>>
>>> We introduce roothub wakeup, which uses roothub
>>> as system wakeup source. It needs xhci platform driver to override
>>> roothub ops.
>>
>> You did not explain why you introduced wakelocks...
>>
> 
> I'm sorry I didn't write description enough.
> I add it below.
> 
>>
>> (...)
>>
>>>  	if (shared_hcd) {
>>>  		usb_remove_hcd(shared_hcd);
>>>  		xhci->shared_hcd = NULL;
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>> index 79d7931c048a..693495054001 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>>> @@ -5502,6 +5502,10 @@ void xhci_init_driver(struct hc_driver *drv,
>>>  			drv->check_bandwidth = over->check_bandwidth;
>>>  		if (over->reset_bandwidth)
>>>  			drv->reset_bandwidth = over->reset_bandwidth;
>>> +		if (over->bus_suspend)
>>> +			drv->bus_suspend = over->bus_suspend;
>>> +		if (over->bus_resume)
>>> +			drv->bus_resume = over->bus_resume;
>>>  	}
>>>  }
>>>  EXPORT_SYMBOL_GPL(xhci_init_driver);
>>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>>> index c9f06c5e4e9d..cb9c54a6a22c 100644
>>> --- a/drivers/usb/host/xhci.h
>>> +++ b/drivers/usb/host/xhci.h
>>> @@ -1752,6 +1752,8 @@ struct xhci_hub {
>>>  struct xhci_hcd {
>>>  	struct usb_hcd *main_hcd;
>>>  	struct usb_hcd *shared_hcd;
>>> +	struct wakeup_source *main_wakelock;
>>> +	struct wakeup_source *shared_wakelock;
>>
>> Drop wakelocks. This is not related to USB and not needed here. Do you
>> see anywhere else in core kernel code usage of the wakelocks?
>>
>> You got this comment already, didn't you? So why you do not address it?
>>
> 
> I want to add a new feature in xhci platform driver. I want to make it
> possible to enter system sleep while usb host connected like USB Mouse.
> It gets system enter sleep only if there's no usb transaction at all.
> Deciding if there's tranaction or not is in root hub because it's parent
> of all child usb devices.


I have USB mouse connected to my system and the system enters suspend,
thus I don't think this patch solves this particular issue.

Best regards,
Krzysztof