On 8/30/2023 11:29 PM, Krzysztof Kozlowski wrote:
> On 31/08/2023 05:01, Thinh Nguyen wrote:
>> On Wed, Aug 30, 2023, Krzysztof Kozlowski wrote:
>>> On 30/08/2023 06:31, Elson Serrao wrote:
>>>>
>>>>
>>>> On 8/29/2023 6:37 PM, Thinh Nguyen wrote:
>>>>> Just want to clarify, there are dwc3 properties and there are dt binding
>>>>> properties. Often the case that dt binding matches 1-to-1 with dwc3
>>>>> driver property. Now, we need to enhance the checkers so that the dwc3
>>>>> driver property to match cases where it is platform specific and through
>>>>> compatible string.
>>>>>
>>>>
>>>> Thank you for the clarification Thinh.
>>>> To confirm, we would need to modify the driver to parse a new compatible
>>>> string (say "snps,dwc3-ext-wakeup") and add .data field so that the
>>>> driver is aware that this particular platform supports external wakeup
>>>> detection.Right ?
>>>
>>> No, it's not then platform specific. You said it depends on each
>>> platform. Platform is Qualcomm SM8450 for example.
>>>
>>
>> Hi Elson,
>>
>> Use the compatible string of your platform.
>>
>> e.g.
>> if (dev->of_node) {
>> struct device_node *parent = of_get_parent(dev->of_node);
>>
>> dwc->no_disconnect_on_usb_suspend =
>> of_device_is_compatible(parent, "qcom,your-compatible-string") ||
>> of_device_is_compatible(parent, "some-other-platform");
>> }
>>
>> You need to enhance dwc3_get_properties(). This may get big as dwc3 adds
>> more properties. Perhaps you can help come up with ideas to keep this
>> clean. Perhaps we can separate this out of dwc3 core.c?
HI Thinh
Apologies for the delayed response.
Series
https://patchwork.kernel.org/project/linux-usb/cover/[email protected]/
from Krishna K, introduced a dt property 'wakeup-source' which indicates
a platforms capability to handle wakeup interrupts. Based on this
property, glue drivers can inform dwc3 core that the device is wakeup
capable through device_init_wakeup(). For example dwc3-qcom driver
informs it like below as per the implementation done in the above series
wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
device_init_wakeup(&pdev->dev, wakeup_source);
device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
The dwc3 core now can access this info through
device_may_wakeup(dwc->dev) while checking for bus suspend scenario to
know whether the platform is capable of detecting wakeup.
Please let me know your thoughts on this approach.
Thanks
Elson
On Thu, Sep 21, 2023, Elson Serrao wrote:
>
>
> On 8/30/2023 11:29 PM, Krzysztof Kozlowski wrote:
> > On 31/08/2023 05:01, Thinh Nguyen wrote:
> > > On Wed, Aug 30, 2023, Krzysztof Kozlowski wrote:
> > > > On 30/08/2023 06:31, Elson Serrao wrote:
> > > > >
> > > > >
> > > > > On 8/29/2023 6:37 PM, Thinh Nguyen wrote:
> > > > > > Just want to clarify, there are dwc3 properties and there are dt binding
> > > > > > properties. Often the case that dt binding matches 1-to-1 with dwc3
> > > > > > driver property. Now, we need to enhance the checkers so that the dwc3
> > > > > > driver property to match cases where it is platform specific and through
> > > > > > compatible string.
> > > > > >
> > > > >
> > > > > Thank you for the clarification Thinh.
> > > > > To confirm, we would need to modify the driver to parse a new compatible
> > > > > string (say "snps,dwc3-ext-wakeup") and add .data field so that the
> > > > > driver is aware that this particular platform supports external wakeup
> > > > > detection.Right ?
> > > >
> > > > No, it's not then platform specific. You said it depends on each
> > > > platform. Platform is Qualcomm SM8450 for example.
> > > >
> > >
> > > Hi Elson,
> > >
> > > Use the compatible string of your platform.
> > >
> > > e.g.
> > > if (dev->of_node) {
> > > struct device_node *parent = of_get_parent(dev->of_node);
> > >
> > > dwc->no_disconnect_on_usb_suspend =
> > > of_device_is_compatible(parent, "qcom,your-compatible-string") ||
> > > of_device_is_compatible(parent, "some-other-platform");
> > > }
> > >
> > > You need to enhance dwc3_get_properties(). This may get big as dwc3 adds
> > > more properties. Perhaps you can help come up with ideas to keep this
> > > clean. Perhaps we can separate this out of dwc3 core.c?
>
> HI Thinh
>
> Apologies for the delayed response.
> Series https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/cover/[email protected]/__;!!A4F2R9G_pg!YGlVy7No98zfEM-X5iWRhIUJ-gJEJn_gbTR4k12avzENV1TXf7cwJLZUezYzAU-rnHIbbqA1UWM0IE0R-t5SMMTJLwLZ$
> from Krishna K, introduced a dt property 'wakeup-source' which indicates a
> platforms capability to handle wakeup interrupts. Based on this property,
> glue drivers can inform dwc3 core that the device is wakeup capable through
> device_init_wakeup(). For example dwc3-qcom driver informs it like below as
> per the implementation done in the above series
>
> wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
> device_init_wakeup(&pdev->dev, wakeup_source);
> device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
>
> The dwc3 core now can access this info through device_may_wakeup(dwc->dev)
> while checking for bus suspend scenario to know whether the platform is
> capable of detecting wakeup.
>
> Please let me know your thoughts on this approach.
>
Hi Elson,
I think that it may not work for everyone. Some platforms may indicate
wakeup-source but should only be applicable in selected scenarios.
(e.g. Roger's platform was only intended to keep connect on suspend)
Also, how will you disable it for certain platforms? Probably will need
to use compatible string then too.
Thanks,
Thinh
>> HI Thinh
>>
>> Apologies for the delayed response.
>> Series https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/cover/[email protected]/__;!!A4F2R9G_pg!YGlVy7No98zfEM-X5iWRhIUJ-gJEJn_gbTR4k12avzENV1TXf7cwJLZUezYzAU-rnHIbbqA1UWM0IE0R-t5SMMTJLwLZ$
>> from Krishna K, introduced a dt property 'wakeup-source' which indicates a
>> platforms capability to handle wakeup interrupts. Based on this property,
>> glue drivers can inform dwc3 core that the device is wakeup capable through
>> device_init_wakeup(). For example dwc3-qcom driver informs it like below as
>> per the implementation done in the above series
>>
>> wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
>> device_init_wakeup(&pdev->dev, wakeup_source);
>> device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
>>
>> The dwc3 core now can access this info through device_may_wakeup(dwc->dev)
>> while checking for bus suspend scenario to know whether the platform is
>> capable of detecting wakeup.
>>
>> Please let me know your thoughts on this approach.
>>
>
> Hi Elson,
>
> I think that it may not work for everyone. Some platforms may indicate
> wakeup-source but should only be applicable in selected scenarios.
> (e.g. Roger's platform was only intended to keep connect on suspend)
>
> Also, how will you disable it for certain platforms? Probably will need
> to use compatible string then too.
>
Hi Thinh
Thank you for your feedback. As an alternative approach, how about
exposing an API from dwc3 core that glue drivers can call to enable
runtime suspend during bus suspend feature ( i.e this API sets
dwc->runtime_suspend_on_usb_suspend field).
Only the platforms that need this feature to be enabled, can call this
API after the child (dwc3 core) probe.
Thanks
Elson
On Tue, Oct 17, 2023, Elson Serrao wrote:
>
>
> > > HI Thinh
> > >
> > > Apologies for the delayed response.
> > > Series https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-usb/cover/[email protected]/__;!!A4F2R9G_pg!YGlVy7No98zfEM-X5iWRhIUJ-gJEJn_gbTR4k12avzENV1TXf7cwJLZUezYzAU-rnHIbbqA1UWM0IE0R-t5SMMTJLwLZ$
> > > from Krishna K, introduced a dt property 'wakeup-source' which indicates a
> > > platforms capability to handle wakeup interrupts. Based on this property,
> > > glue drivers can inform dwc3 core that the device is wakeup capable through
> > > device_init_wakeup(). For example dwc3-qcom driver informs it like below as
> > > per the implementation done in the above series
> > >
> > > wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
> > > device_init_wakeup(&pdev->dev, wakeup_source);
> > > device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
> > >
> > > The dwc3 core now can access this info through device_may_wakeup(dwc->dev)
> > > while checking for bus suspend scenario to know whether the platform is
> > > capable of detecting wakeup.
> > >
> > > Please let me know your thoughts on this approach.
> > >
> >
> > Hi Elson,
> >
> > I think that it may not work for everyone. Some platforms may indicate
> > wakeup-source but should only be applicable in selected scenarios.
> > (e.g. Roger's platform was only intended to keep connect on suspend)
> >
> > Also, how will you disable it for certain platforms? Probably will need
> > to use compatible string then too.
> >
>
> Hi Thinh
>
> Thank you for your feedback. As an alternative approach, how about exposing
> an API from dwc3 core that glue drivers can call to enable runtime suspend
> during bus suspend feature ( i.e this API sets
> dwc->runtime_suspend_on_usb_suspend field).
>
> Only the platforms that need this feature to be enabled, can call this API
> after the child (dwc3 core) probe.
>
You mean after Bjorn's updates? That sounds good to me. Please make it
generic so that we can set other driver properties too.
Thanks,
Thinh