Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6451aafa-7021-0769-d408-9dba40d58668@linaro.org>
Date:   Mon, 2 Jan 2023 09:30:40 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [RFC PATCH v2 1/3] usb: support Samsung Exynos xHCI Controller
Content-Language: en-US
To:     Jung Daehwan <dh10.jung@samsung.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Mathias Nyman <mathias.nyman@intel.com>,
        Felipe Balbi <balbi@kernel.org>,
        "open list:USB SUBSYSTEM" <linux-usb@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>, sc.suh@samsung.com,
        taehyun.cho@samsung.com, jh0801.jung@samsung.com,
        eomji.oh@samsung.com
References: <1672307866-25839-1-git-send-email-dh10.jung@samsung.com>
 <CGME20221229100413epcas2p34c702faf8c96d207cf1659b1173f8858@epcas2p3.samsung.com>
 <1672307866-25839-2-git-send-email-dh10.jung@samsung.com>
 <bc531774-ed70-39b7-3534-5683dff20591@linaro.org>
 <20230102062448.GC74470@ubuntu>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230102062448.GC74470@ubuntu>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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