[PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Li Jun]

New schema of usb controller DT-node should be named with prefix
"^usb(@.*)?", dt changed the node name, but missed counter part
change in driver, fix it by switching to use compatible string as
the dwc3 core compatible string keeps "snps,dwc3" in all dt.

Fixes: d1689cd3c0f4 ("arm64: dts: imx8mp: Use the correct name for child node "snps, dwc3"")
Signed-off-by: Li Jun <jun.li@nxp.com>
---
 drivers/usb/dwc3/dwc3-imx8mp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-imx8mp.c b/drivers/usb/dwc3/dwc3-imx8mp.c
index b13cfab89d53..e9fced6f7a7c 100644
--- a/drivers/usb/dwc3/dwc3-imx8mp.c
+++ b/drivers/usb/dwc3/dwc3-imx8mp.c
@@ -165,7 +165,7 @@ static int dwc3_imx8mp_probe(struct platform_device *pdev)
 	if (err < 0)
 		goto disable_rpm;
 
-	dwc3_np = of_get_child_by_name(node, "dwc3");
+	dwc3_np = of_get_compatible_child(node, "snps,dwc3");
 	if (!dwc3_np) {
 		dev_err(dev, "failed to find dwc3 core child\n");
 		goto disable_rpm;
-- 
2.25.1

Re: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

Li Jun <jun.li@nxp.com> writes:

> New schema of usb controller DT-node should be named with prefix
> "^usb(@.*)?", dt changed the node name, but missed counter part
> change in driver, fix it by switching to use compatible string as
> the dwc3 core compatible string keeps "snps,dwc3" in all dt.
>
> Fixes: d1689cd3c0f4 ("arm64: dts: imx8mp: Use the correct name for child node "snps, dwc3"")
> Signed-off-by: Li Jun <jun.li@nxp.com>


Nice fix :-) It may break down if we have two dwc3 nodes as child of a
single parent, but I guess that's very unlikely anyway.

Acked-by: Felipe Balbi <balbi@kernel.org>

That being said, why do need to keep a pointer to the child? I had a
quick look at the driver and it doesn't seem like the pointer is
necessary at all.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Jun Li]

Hi,

> -----Original Message-----
> From: Felipe Balbi <balbi@kernel.org>
> Sent: Friday, April 30, 2021 4:24 PM
> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org
> Cc: shawnguo@kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> thunder.leizhen@huawei.com; linux-usb@vger.kernel.org
> Subject: Re: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible
> string
> 
> Li Jun <jun.li@nxp.com> writes:
> 
> > New schema of usb controller DT-node should be named with prefix
> > "^usb(@.*)?", dt changed the node name, but missed counter part change
> > in driver, fix it by switching to use compatible string as the dwc3
> > core compatible string keeps "snps,dwc3" in all dt.
> >
> > Fixes: d1689cd3c0f4 ("arm64: dts: imx8mp: Use the correct name for
> > child node "snps, dwc3"")
> > Signed-off-by: Li Jun <jun.li@nxp.com>
> 
> 
> Nice fix :-) It may break down if we have two dwc3 nodes as child of a single
> parent, but I guess that's very unlikely anyway.
> 
> Acked-by: Felipe Balbi <balbi@kernel.org>
> 
> That being said, why do need to keep a pointer to the child? I had a quick
> look at the driver and it doesn't seem like the pointer is necessary at all.

I need keep the child pointer(dwc3 core platform device) to find the dwc3 core
instance struct(struct dwc3), the wakeup setting need check the dwc3 core's
current_dr_role and do runtime resume based on the child's dev. 

Thanks
Li Jun 
> 
> --
> balbi

RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3114 bytes --]


Hi,

Jun Li <jun.li@nxp.com> writes:
>> -----Original Message-----
>> From: Felipe Balbi <balbi@kernel.org>
>> Sent: Friday, April 30, 2021 4:24 PM
>> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org
>> Cc: shawnguo@kernel.org; dl-linux-imx <linux-imx@nxp.com>;
>> thunder.leizhen@huawei.com; linux-usb@vger.kernel.org
>> Subject: Re: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible
>> string
>> 
>> Li Jun <jun.li@nxp.com> writes:
>> 
>> > New schema of usb controller DT-node should be named with prefix
>> > "^usb(@.*)?", dt changed the node name, but missed counter part change
>> > in driver, fix it by switching to use compatible string as the dwc3
>> > core compatible string keeps "snps,dwc3" in all dt.
>> >
>> > Fixes: d1689cd3c0f4 ("arm64: dts: imx8mp: Use the correct name for
>> > child node "snps, dwc3"")
>> > Signed-off-by: Li Jun <jun.li@nxp.com>
>> 
>> 
>> Nice fix :-) It may break down if we have two dwc3 nodes as child of a single
>> parent, but I guess that's very unlikely anyway.
>> 
>> Acked-by: Felipe Balbi <balbi@kernel.org>
>> 
>> That being said, why do need to keep a pointer to the child? I had a quick
>> look at the driver and it doesn't seem like the pointer is necessary at all.
>
> I need keep the child pointer(dwc3 core platform device) to find the dwc3 core
> instance struct(struct dwc3), the wakeup setting need check the dwc3 core's
> current_dr_role and do runtime resume based on the child's dev. 

Right, you're talking about the code below. Some suggestions inline:

> static void dwc3_imx8mp_wakeup_enable(struct dwc3_imx8mp *dwc3_imx)
> {
> 	struct dwc3	*dwc3 = platform_get_drvdata(dwc3_imx->dwc3);
> 	u32		val;
>
> 	if (!dwc3)
> 		return;

I don't think you need to care if dwc3 has probed or not here.

> 	val = readl(dwc3_imx->glue_base + USB_WAKEUP_CTRL);
>
> 	if ((dwc3->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc3->xhci)
> 		val |= USB_WAKEUP_EN | USB_WAKEUP_SS_CONN |
> 		       USB_WAKEUP_U3_EN | USB_WAKEUP_DPDM_EN;
> 	else if (dwc3->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
> 		val |= USB_WAKEUP_EN | USB_WAKEUP_VBUS_EN |
> 		       USB_WAKEUP_VBUS_SRC_SESS_VAL;

for this, you could register a listener to the extcon notifier and
update these bits accordingly. With that, you would already *know* that
dwc3 is probed.

> static irqreturn_t dwc3_imx8mp_interrupt(int irq, void *_dwc3_imx)
> {
> 	struct dwc3_imx8mp	*dwc3_imx = _dwc3_imx;
> 	struct dwc3		*dwc = platform_get_drvdata(dwc3_imx->dwc3);
>
> 	if (!dwc3_imx->pm_suspended)
> 		return IRQ_HANDLED;
>
> 	disable_irq_nosync(dwc3_imx->irq);
> 	dwc3_imx->wakeup_pending = true;
>
> 	if ((dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc->xhci)
> 		pm_runtime_resume(&dwc->xhci->dev);
> 	else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
> 		pm_runtime_get(dwc->dev);
>
> 	return IRQ_HANDLED;
> }

for this, maybe you need to teach dwc3 core about wakeup irqs
instead. Have a look dev_pm_set_dedicated_wake_irq().

That would clean things up, I think ;)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Jun Li]

Hi,
> -----Original Message-----
> From: Felipe Balbi <balbi@kernel.org>
> Sent: Friday, April 30, 2021 6:00 PM
> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org
> Cc: shawnguo@kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> thunder.leizhen@huawei.com; linux-usb@vger.kernel.org
> Subject: RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible
> string
> 
> 
> Hi,
> 
> Jun Li <jun.li@nxp.com> writes:
> >> -----Original Message-----
> >> From: Felipe Balbi <balbi@kernel.org>
> >> Sent: Friday, April 30, 2021 4:24 PM
> >> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org
> >> Cc: shawnguo@kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> >> thunder.leizhen@huawei.com; linux-usb@vger.kernel.org
> >> Subject: Re: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via
> >> compatible string
> >>
> >> Li Jun <jun.li@nxp.com> writes:
> >>
> >> > New schema of usb controller DT-node should be named with prefix
> >> > "^usb(@.*)?", dt changed the node name, but missed counter part
> >> > change in driver, fix it by switching to use compatible string as
> >> > the dwc3 core compatible string keeps "snps,dwc3" in all dt.
> >> >
> >> > Fixes: d1689cd3c0f4 ("arm64: dts: imx8mp: Use the correct name for
> >> > child node "snps, dwc3"")
> >> > Signed-off-by: Li Jun <jun.li@nxp.com>
> >>
> >>
> >> Nice fix :-) It may break down if we have two dwc3 nodes as child of
> >> a single parent, but I guess that's very unlikely anyway.
> >>
> >> Acked-by: Felipe Balbi <balbi@kernel.org>
> >>
> >> That being said, why do need to keep a pointer to the child? I had a
> >> quick look at the driver and it doesn't seem like the pointer is necessary
> at all.
> >
> > I need keep the child pointer(dwc3 core platform device) to find the
> > dwc3 core instance struct(struct dwc3), the wakeup setting need check
> > the dwc3 core's current_dr_role and do runtime resume based on the child's
> dev.
> 
> Right, you're talking about the code below. Some suggestions inline:
> 
> > static void dwc3_imx8mp_wakeup_enable(struct dwc3_imx8mp *dwc3_imx) {
> > 	struct dwc3	*dwc3 = platform_get_drvdata(dwc3_imx->dwc3);
> > 	u32		val;
> >
> > 	if (!dwc3)
> > 		return;
> 
> I don't think you need to care if dwc3 has probed or not here.

Yes if there is other way to know current role.
 
> 
> > 	val = readl(dwc3_imx->glue_base + USB_WAKEUP_CTRL);
> >
> > 	if ((dwc3->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc3->xhci)
> > 		val |= USB_WAKEUP_EN | USB_WAKEUP_SS_CONN |
> > 		       USB_WAKEUP_U3_EN | USB_WAKEUP_DPDM_EN;
> > 	else if (dwc3->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
> > 		val |= USB_WAKEUP_EN | USB_WAKEUP_VBUS_EN |
> > 		       USB_WAKEUP_VBUS_SRC_SESS_VAL;
> 
> for this, you could register a listener to the extcon notifier and update
> these bits accordingly. With that, you would already *know* that
> dwc3 is probed.

With usb role switch class, there no extcon provider, so I think this way
can't work for me.

> 
> > static irqreturn_t dwc3_imx8mp_interrupt(int irq, void *_dwc3_imx) {
> > 	struct dwc3_imx8mp	*dwc3_imx = _dwc3_imx;
> > 	struct dwc3		*dwc = platform_get_drvdata(dwc3_imx->dwc3);
> >
> > 	if (!dwc3_imx->pm_suspended)
> > 		return IRQ_HANDLED;
> >
> > 	disable_irq_nosync(dwc3_imx->irq);
> > 	dwc3_imx->wakeup_pending = true;
> >
> > 	if ((dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc->xhci)
> > 		pm_runtime_resume(&dwc->xhci->dev);
> > 	else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
> > 		pm_runtime_get(dwc->dev);
> >
> > 	return IRQ_HANDLED;
> > }
> 
> for this, maybe you need to teach dwc3 core about wakeup irqs instead. Have
> a look dev_pm_set_dedicated_wake_irq().

Good suggestion, but if extcon notifier listener can't work for me, 
my understanding is this *teach* in glue layer driver still need
access dwc3 core instance struct, right?

> 
> That would clean things up, I think ;)

Yes, I may use dev_pm_set_dedicated_wake_irq() to attach the wakeup irq
to dwc->dev and &dwc->xhci->dev for device and host mode, which will
clean things up.

thanks
Li Jun
> 
> --
> balbi

RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]


Hi,

Jun Li <jun.li@nxp.com> writes:
>> > 	val = readl(dwc3_imx->glue_base + USB_WAKEUP_CTRL);
>> >
>> > 	if ((dwc3->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc3->xhci)
>> > 		val |= USB_WAKEUP_EN | USB_WAKEUP_SS_CONN |
>> > 		       USB_WAKEUP_U3_EN | USB_WAKEUP_DPDM_EN;
>> > 	else if (dwc3->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
>> > 		val |= USB_WAKEUP_EN | USB_WAKEUP_VBUS_EN |
>> > 		       USB_WAKEUP_VBUS_SRC_SESS_VAL;
>> 
>> for this, you could register a listener to the extcon notifier and update
>> these bits accordingly. With that, you would already *know* that
>> dwc3 is probed.
>
> With usb role switch class, there no extcon provider, so I think this way
> can't work for me.

perhaps role switch class could learn about notifiers ;-)

>> > static irqreturn_t dwc3_imx8mp_interrupt(int irq, void *_dwc3_imx) {
>> > 	struct dwc3_imx8mp	*dwc3_imx = _dwc3_imx;
>> > 	struct dwc3		*dwc = platform_get_drvdata(dwc3_imx->dwc3);
>> >
>> > 	if (!dwc3_imx->pm_suspended)
>> > 		return IRQ_HANDLED;
>> >
>> > 	disable_irq_nosync(dwc3_imx->irq);
>> > 	dwc3_imx->wakeup_pending = true;
>> >
>> > 	if ((dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc->xhci)
>> > 		pm_runtime_resume(&dwc->xhci->dev);
>> > 	else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
>> > 		pm_runtime_get(dwc->dev);
>> >
>> > 	return IRQ_HANDLED;
>> > }
>> 
>> for this, maybe you need to teach dwc3 core about wakeup irqs instead. Have
>> a look dev_pm_set_dedicated_wake_irq().
>
> Good suggestion, but if extcon notifier listener can't work for me, 
> my understanding is this *teach* in glue layer driver still need
> access dwc3 core instance struct, right?

for now, maybe. But it may be better to implement a notifier method in
role switch class.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Jun Li]

Hi

> -----Original Message-----
> From: Felipe Balbi <balbi@kernel.org>
> Sent: Thursday, May 6, 2021 10:32 PM
> To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org
> Cc: shawnguo@kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> thunder.leizhen@huawei.com; linux-usb@vger.kernel.org
> Subject: RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible
> string
> 
> 
> Hi,
> 
> Jun Li <jun.li@nxp.com> writes:
> >> > 	val = readl(dwc3_imx->glue_base + USB_WAKEUP_CTRL);
> >> >
> >> > 	if ((dwc3->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc3->xhci)
> >> > 		val |= USB_WAKEUP_EN | USB_WAKEUP_SS_CONN |
> >> > 		       USB_WAKEUP_U3_EN | USB_WAKEUP_DPDM_EN;
> >> > 	else if (dwc3->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
> >> > 		val |= USB_WAKEUP_EN | USB_WAKEUP_VBUS_EN |
> >> > 		       USB_WAKEUP_VBUS_SRC_SESS_VAL;
> >>
> >> for this, you could register a listener to the extcon notifier and
> >> update these bits accordingly. With that, you would already *know*
> >> that
> >> dwc3 is probed.
> >
> > With usb role switch class, there no extcon provider, so I think this
> > way can't work for me.
> 
> perhaps role switch class could learn about notifiers ;-)
> 
> >> > static irqreturn_t dwc3_imx8mp_interrupt(int irq, void *_dwc3_imx) {
> >> > 	struct dwc3_imx8mp	*dwc3_imx = _dwc3_imx;
> >> > 	struct dwc3		*dwc = platform_get_drvdata(dwc3_imx->dwc3);
> >> >
> >> > 	if (!dwc3_imx->pm_suspended)
> >> > 		return IRQ_HANDLED;
> >> >
> >> > 	disable_irq_nosync(dwc3_imx->irq);
> >> > 	dwc3_imx->wakeup_pending = true;
> >> >
> >> > 	if ((dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc->xhci)
> >> > 		pm_runtime_resume(&dwc->xhci->dev);
> >> > 	else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
> >> > 		pm_runtime_get(dwc->dev);
> >> >
> >> > 	return IRQ_HANDLED;
> >> > }
> >>
> >> for this, maybe you need to teach dwc3 core about wakeup irqs
> >> instead. Have a look dev_pm_set_dedicated_wake_irq().
> >
> > Good suggestion, but if extcon notifier listener can't work for me, my
> > understanding is this *teach* in glue layer driver still need access
> > dwc3 core instance struct, right?
> 
> for now, maybe. But it may be better to implement a notifier method in role
> switch class.

I am not sure if introduce notifier in role switch class is a good idea,
I had the impression extcon is not encouraged to use if possible.

+ Heikki

Thanks
Li Jun

> 
> --
> balbi

Re: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Heikki Krogerus]

Hi guys,

On Fri, May 07, 2021 at 07:31:43AM +0000, Jun Li wrote:
> Hi
> 
> > -----Original Message-----
> > From: Felipe Balbi <balbi@kernel.org>
> > Sent: Thursday, May 6, 2021 10:32 PM
> > To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org
> > Cc: shawnguo@kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > thunder.leizhen@huawei.com; linux-usb@vger.kernel.org
> > Subject: RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible
> > string
> > 
> > 
> > Hi,
> > 
> > Jun Li <jun.li@nxp.com> writes:
> > >> > 	val = readl(dwc3_imx->glue_base + USB_WAKEUP_CTRL);
> > >> >
> > >> > 	if ((dwc3->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc3->xhci)
> > >> > 		val |= USB_WAKEUP_EN | USB_WAKEUP_SS_CONN |
> > >> > 		       USB_WAKEUP_U3_EN | USB_WAKEUP_DPDM_EN;
> > >> > 	else if (dwc3->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
> > >> > 		val |= USB_WAKEUP_EN | USB_WAKEUP_VBUS_EN |
> > >> > 		       USB_WAKEUP_VBUS_SRC_SESS_VAL;
> > >>
> > >> for this, you could register a listener to the extcon notifier and
> > >> update these bits accordingly. With that, you would already *know*
> > >> that
> > >> dwc3 is probed.
> > >
> > > With usb role switch class, there no extcon provider, so I think this
> > > way can't work for me.
> > 
> > perhaps role switch class could learn about notifiers ;-)
> > 
> > >> > static irqreturn_t dwc3_imx8mp_interrupt(int irq, void *_dwc3_imx) {
> > >> > 	struct dwc3_imx8mp	*dwc3_imx = _dwc3_imx;
> > >> > 	struct dwc3		*dwc = platform_get_drvdata(dwc3_imx->dwc3);
> > >> >
> > >> > 	if (!dwc3_imx->pm_suspended)
> > >> > 		return IRQ_HANDLED;
> > >> >
> > >> > 	disable_irq_nosync(dwc3_imx->irq);
> > >> > 	dwc3_imx->wakeup_pending = true;
> > >> >
> > >> > 	if ((dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc->xhci)
> > >> > 		pm_runtime_resume(&dwc->xhci->dev);
> > >> > 	else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
> > >> > 		pm_runtime_get(dwc->dev);
> > >> >
> > >> > 	return IRQ_HANDLED;
> > >> > }
> > >>
> > >> for this, maybe you need to teach dwc3 core about wakeup irqs
> > >> instead. Have a look dev_pm_set_dedicated_wake_irq().
> > >
> > > Good suggestion, but if extcon notifier listener can't work for me, my
> > > understanding is this *teach* in glue layer driver still need access
> > > dwc3 core instance struct, right?
> > 
> > for now, maybe. But it may be better to implement a notifier method in role
> > switch class.
> 
> I am not sure if introduce notifier in role switch class is a good idea,
> I had the impression extcon is not encouraged to use if possible.

I'm not against role switch notifiers. They were proposed before
already couple of years ago, but at that time there just were no users
them notifier, so the patch was just dropped from the series [1].
But I don't think anybody was against the idea. Please feel free to
add them to the class if you have use for them.

Or, are we talking about some other notifier here?

[1] https://lore.kernel.org/linux-usb/20191002231617.3670-3-john.stultz@linaro.org/


thanks,

-- 
heikki

RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Jun Li]

Hi,
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Tuesday, May 11, 2021 3:59 PM
> To: Jun Li <jun.li@nxp.com>
> Cc: Felipe Balbi <balbi@kernel.org>; gregkh@linuxfoundation.org;
> shawnguo@kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> thunder.leizhen@huawei.com; linux-usb@vger.kernel.org
> Subject: Re: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible
> string
> 
> Hi guys,
> 
> On Fri, May 07, 2021 at 07:31:43AM +0000, Jun Li wrote:
> > Hi
> >
> > > -----Original Message-----
> > > From: Felipe Balbi <balbi@kernel.org>
> > > Sent: Thursday, May 6, 2021 10:32 PM
> > > To: Jun Li <jun.li@nxp.com>; gregkh@linuxfoundation.org
> > > Cc: shawnguo@kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > > thunder.leizhen@huawei.com; linux-usb@vger.kernel.org
> > > Subject: RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via
> > > compatible string
> > >
> > >
> > > Hi,
> > >
> > > Jun Li <jun.li@nxp.com> writes:
> > > >> > 	val = readl(dwc3_imx->glue_base + USB_WAKEUP_CTRL);
> > > >> >
> > > >> > 	if ((dwc3->current_dr_role == DWC3_GCTL_PRTCAP_HOST) &&
> dwc3->xhci)
> > > >> > 		val |= USB_WAKEUP_EN | USB_WAKEUP_SS_CONN |
> > > >> > 		       USB_WAKEUP_U3_EN | USB_WAKEUP_DPDM_EN;
> > > >> > 	else if (dwc3->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
> > > >> > 		val |= USB_WAKEUP_EN | USB_WAKEUP_VBUS_EN |
> > > >> > 		       USB_WAKEUP_VBUS_SRC_SESS_VAL;
> > > >>
> > > >> for this, you could register a listener to the extcon notifier
> > > >> and update these bits accordingly. With that, you would already
> > > >> *know* that
> > > >> dwc3 is probed.
> > > >
> > > > With usb role switch class, there no extcon provider, so I think
> > > > this way can't work for me.
> > >
> > > perhaps role switch class could learn about notifiers ;-)
> > >
> > > >> > static irqreturn_t dwc3_imx8mp_interrupt(int irq, void *_dwc3_imx)
> {
> > > >> > 	struct dwc3_imx8mp	*dwc3_imx = _dwc3_imx;
> > > >> > 	struct dwc3		*dwc = platform_get_drvdata(dwc3_imx->dwc3);
> > > >> >
> > > >> > 	if (!dwc3_imx->pm_suspended)
> > > >> > 		return IRQ_HANDLED;
> > > >> >
> > > >> > 	disable_irq_nosync(dwc3_imx->irq);
> > > >> > 	dwc3_imx->wakeup_pending = true;
> > > >> >
> > > >> > 	if ((dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) &&
> dwc->xhci)
> > > >> > 		pm_runtime_resume(&dwc->xhci->dev);
> > > >> > 	else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
> > > >> > 		pm_runtime_get(dwc->dev);
> > > >> >
> > > >> > 	return IRQ_HANDLED;
> > > >> > }
> > > >>
> > > >> for this, maybe you need to teach dwc3 core about wakeup irqs
> > > >> instead. Have a look dev_pm_set_dedicated_wake_irq().
> > > >
> > > > Good suggestion, but if extcon notifier listener can't work for
> > > > me, my understanding is this *teach* in glue layer driver still
> > > > need access
> > > > dwc3 core instance struct, right?
> > >
> > > for now, maybe. But it may be better to implement a notifier method
> > > in role switch class.
> >
> > I am not sure if introduce notifier in role switch class is a good
> > idea, I had the impression extcon is not encouraged to use if possible.
> 
> I'm not against role switch notifiers. They were proposed before already
> couple of years ago, but at that time there just were no users them notifier,
> so the patch was just dropped from the series [1].
> But I don't think anybody was against the idea. Please feel free to add them
> to the class if you have use for them.

So I had the incorrect impression.

Yes, that's the Felipe was referring to, I think.

I will pick up [1] and improve my driver as Felipe suggested.

Thanks
Li Jun

> 
> Or, are we talking about some other notifier here?
> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.
> kernel.org%2Flinux-usb%2F20191002231617.3670-3-john.stultz%40linaro.org
> %2F&amp;data=04%7C01%7Cjun.li%40nxp.com%7C9479f8f61e1a47245c2e08d91452a
> dd6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637563167667138204%7CU
> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
> wiLCJXVCI6Mn0%3D%7C1000&amp;sdata=EhSPBC1sGeH65EPuFV22pEcVS6nDeRNPdN9Z1
> 3lPPYg%3D&amp;reserved=0
> 
> 
> thanks,
> 
> --
> heikki

RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]


Hi,

Jun Li <jun.li@nxp.com> writes:
>> > > > Good suggestion, but if extcon notifier listener can't work for
>> > > > me, my understanding is this *teach* in glue layer driver still
>> > > > need access
>> > > > dwc3 core instance struct, right?
>> > >
>> > > for now, maybe. But it may be better to implement a notifier method
>> > > in role switch class.
>> >
>> > I am not sure if introduce notifier in role switch class is a good
>> > idea, I had the impression extcon is not encouraged to use if possible.
>> 
>> I'm not against role switch notifiers. They were proposed before already
>> couple of years ago, but at that time there just were no users them notifier,
>> so the patch was just dropped from the series [1].
>> But I don't think anybody was against the idea. Please feel free to add them
>> to the class if you have use for them.
>
> So I had the incorrect impression.
>
> Yes, that's the Felipe was referring to, I think.
>
> I will pick up [1] and improve my driver as Felipe suggested.

sounds great, thahnks Li Jun

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]

RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Jun Li]

Hi,
> -----Original Message-----
> From: Felipe Balbi <balbi@kernel.org>
> Sent: Tuesday, May 11, 2021 5:27 PM
> To: Jun Li <jun.li@nxp.com>; Heikki Krogerus
> <heikki.krogerus@linux.intel.com>
> Cc: gregkh@linuxfoundation.org; shawnguo@kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; thunder.leizhen@huawei.com;
> linux-usb@vger.kernel.org
> Subject: RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible
> string
> 
> 
> Hi,
> 
> Jun Li <jun.li@nxp.com> writes:
> >> > > > Good suggestion, but if extcon notifier listener can't work for
> >> > > > me, my understanding is this *teach* in glue layer driver still
> >> > > > need access
> >> > > > dwc3 core instance struct, right?
> >> > >
> >> > > for now, maybe. But it may be better to implement a notifier
> >> > > method in role switch class.
> >> >
> >> > I am not sure if introduce notifier in role switch class is a good
> >> > idea, I had the impression extcon is not encouraged to use if possible.
> >>
> >> I'm not against role switch notifiers. They were proposed before
> >> already couple of years ago, but at that time there just were no
> >> users them notifier, so the patch was just dropped from the series [1].
> >> But I don't think anybody was against the idea. Please feel free to
> >> add them to the class if you have use for them.
> >
> > So I had the incorrect impression.
> >
> > Yes, that's the Felipe was referring to, I think.
> >
> > I will pick up [1] and improve my driver as Felipe suggested.
> 
> sounds great, thahnks Li Jun

Got chance to check this, but it turns out this glue driver
still has to be care of the dwc3 core and its probe completion,
because the role switch class is created by dwc3 core in its
probe.

dev_pm_set_dedicated_wake_irq() is a good solution for case
a sperate/glue driver is not required, in my imx8mp case, I
need a glue driver anyway, so this seems can't make my driver
much simpler. Rough change please see below:  

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 9f7c7f587d38..bcb63bcf27be 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -874,7 +874,6 @@ usb3_0: usb@32f10100 {
 			clocks = <&clk IMX8MP_CLK_HSIO_ROOT>,
 				 <&clk IMX8MP_CLK_USB_ROOT>;
 			clock-names = "hsio", "suspend";
-			interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			dma-ranges = <0x40000000 0x40000000 0xc0000000>;
@@ -891,7 +890,9 @@ usb_dwc3_0: usb@38100000 {
 				assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>;
 				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>;
 				assigned-clock-rates = <500000000>;
-				interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
+							<GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-names = "irq", "wakeup";
 				phys = <&usb3_phy0>, <&usb3_phy0>;
 				phy-names = "usb2-phy", "usb3-phy";
 				snps,dis-u2-freeclk-exists-quirk;
@@ -915,7 +916,6 @@ usb3_1: usb@32f10108 {
 			clocks = <&clk IMX8MP_CLK_HSIO_ROOT>,
 				 <&clk IMX8MP_CLK_USB_ROOT>;
 			clock-names = "hsio", "suspend";
-			interrupts = <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			dma-ranges = <0x40000000 0x40000000 0xc0000000>;
@@ -932,7 +932,9 @@ usb_dwc3_1: usb@38200000 {
 				assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>;
 				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>;
 				assigned-clock-rates = <500000000>;
-				interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
+							<GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-names = "irq", "wakeup";
 				phys = <&usb3_phy1>, <&usb3_phy1>;
 				phy-names = "usb2-phy", "usb3-phy";
 				snps,dis-u2-freeclk-exists-quirk;
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index b6871ee5e8ca..4dac7cd98a31 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1615,6 +1615,12 @@ static int dwc3_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	dwc->wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
+	if (dwc->wakeup_irq == -EPROBE_DEFER)
+		goto assert_reset;
+	else
+		dev_err(dwc->dev, "the wakeup irq No. is %d\n", dwc->wakeup_irq);
+
 	ret = clk_bulk_prepare_enable(dwc->num_clks, dwc->clks);
 	if (ret)
 		goto assert_reset;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 1c85f40a8dd8..264ae7def903 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -32,6 +32,7 @@
 #include <linux/phy/phy.h>
 
 #include <linux/power_supply.h>
+#include <linux/pm_wakeirq.h>
 
 #define DWC3_MSG_MAX	500
 
@@ -1295,6 +1296,7 @@ struct dwc3 {
 	int			max_cfg_eps;
 	int			last_fifo_depth;
 	int			num_ep_resized;
+	int			wakeup_irq;
 };
 
 #define INCRX_BURST_MODE 0
diff --git a/drivers/usb/dwc3/dwc3-imx8mp.c b/drivers/usb/dwc3/dwc3-imx8mp.c
index 756faa46d33a..76f83fe64e4b 100644
--- a/drivers/usb/dwc3/dwc3-imx8mp.c
+++ b/drivers/usb/dwc3/dwc3-imx8mp.c
@@ -38,29 +38,24 @@
 
 struct dwc3_imx8mp {
 	struct device			*dev;
-	struct platform_device		*dwc3;
 	void __iomem			*glue_base;
 	struct clk			*hsio_clk;
 	struct clk			*suspend_clk;
-	int				irq;
+	enum usb_role			role;
+	struct notifier_block		notifier;
 	bool				pm_suspended;
-	bool				wakeup_pending;
 };
 
 static void dwc3_imx8mp_wakeup_enable(struct dwc3_imx8mp *dwc3_imx)
 {
-	struct dwc3	*dwc3 = platform_get_drvdata(dwc3_imx->dwc3);
-	u32		val;
-
-	if (!dwc3)
-		return;
+	u32 val;
 
 	val = readl(dwc3_imx->glue_base + USB_WAKEUP_CTRL);
 
-	if ((dwc3->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc3->xhci)
+	if (dwc3_imx->role == USB_ROLE_HOST)
 		val |= USB_WAKEUP_EN | USB_WAKEUP_SS_CONN |
 		       USB_WAKEUP_U3_EN | USB_WAKEUP_DPDM_EN;
-	else if (dwc3->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
+	else if (dwc3_imx->role == USB_ROLE_DEVICE)
 		val |= USB_WAKEUP_EN | USB_WAKEUP_VBUS_EN |
 		       USB_WAKEUP_VBUS_SRC_SESS_VAL;
 
@@ -76,23 +71,16 @@ static void dwc3_imx8mp_wakeup_disable(struct dwc3_imx8mp *dwc3_imx)
 	writel(val, dwc3_imx->glue_base + USB_WAKEUP_CTRL);
 }
 
-static irqreturn_t dwc3_imx8mp_interrupt(int irq, void *_dwc3_imx)
+static int dwc3_imx8mp_role_event(struct notifier_block *nb,
+				  unsigned long event, void *ptr)
 {
-	struct dwc3_imx8mp	*dwc3_imx = _dwc3_imx;
-	struct dwc3		*dwc = platform_get_drvdata(dwc3_imx->dwc3);
+	struct dwc3_imx8mp *dwc3_imx;
 
-	if (!dwc3_imx->pm_suspended)
-		return IRQ_HANDLED;
-
-	disable_irq_nosync(dwc3_imx->irq);
-	dwc3_imx->wakeup_pending = true;
+	dwc3_imx = container_of(nb, struct dwc3_imx8mp, notifier);
 
-	if ((dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc->xhci)
-		pm_runtime_resume(&dwc->xhci->dev);
-	else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
-		pm_runtime_get(dwc->dev);
+	dwc3_imx->role = event;
 
-	return IRQ_HANDLED;
+	return NOTIFY_DONE;
 }
 
 static int dwc3_imx8mp_probe(struct platform_device *pdev)
@@ -100,7 +88,7 @@ static int dwc3_imx8mp_probe(struct platform_device *pdev)
 	struct device		*dev = &pdev->dev;
 	struct device_node	*dwc3_np, *node = dev->of_node;
 	struct dwc3_imx8mp	*dwc3_imx;
-	int			err, irq;
+	int			err;
 
 	if (!node) {
 		dev_err(dev, "device node not found\n");
@@ -145,20 +133,6 @@ static int dwc3_imx8mp_probe(struct platform_device *pdev)
 		goto disable_hsio_clk;
 	}
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0) {
-		err = irq;
-		goto disable_clks;
-	}
-	dwc3_imx->irq = irq;
-
-	err = devm_request_threaded_irq(dev, irq, NULL, dwc3_imx8mp_interrupt,
-					IRQF_ONESHOT, dev_name(dev), dwc3_imx);
-	if (err) {
-		dev_err(dev, "failed to request IRQ #%d --> %d\n", irq, err);
-		goto disable_clks;
-	}
-
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
 	err = pm_runtime_get_sync(dev);
@@ -177,13 +151,6 @@ static int dwc3_imx8mp_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "failed to create dwc3 core\n");
 		goto err_node_put;
 	}
-
-	dwc3_imx->dwc3 = of_find_device_by_node(dwc3_np);
-	if (!dwc3_imx->dwc3) {
-		dev_err(dev, "failed to get dwc3 platform device\n");
-		err = -ENODEV;
-		goto depopulate;
-	}
 	of_node_put(dwc3_np);
 
 	device_set_wakeup_capable(dev, true);
@@ -191,14 +158,11 @@ static int dwc3_imx8mp_probe(struct platform_device *pdev)
 
 	return 0;
 
-depopulate:
-	of_platform_depopulate(dev);
 err_node_put:
 	of_node_put(dwc3_np);
 disable_rpm:
 	pm_runtime_disable(dev);
 	pm_runtime_put_noidle(dev);
-disable_clks:
 	clk_disable_unprepare(dwc3_imx->suspend_clk);
 disable_hsio_clk:
 	clk_disable_unprepare(dwc3_imx->hsio_clk);
@@ -224,12 +188,54 @@ static int dwc3_imx8mp_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int dwc3_imx8mp_register_notifier(struct dwc3_imx8mp *dwc3_imx)
+{
+	struct device_node	*dwc3_np, *node = dwc3_imx->dev->of_node;
+	struct platform_device	*dwc3_pdev;
+	struct dwc3		*dwc3;
+	int			err;
+
+	dwc3_np = of_get_compatible_child(node, "snps,dwc3");
+	if (!dwc3_np) {
+		err = -ENODEV;
+		dev_err(dwc3_imx->dev, "failed to find dwc3 core child\n");
+		return err;
+	}
+
+	dwc3_pdev = of_find_device_by_node(dwc3_np);
+	if (!dwc3_pdev) {
+		dev_err(dwc3_imx->dev, "failed to get dwc3 platform device\n");
+		err = -ENODEV;
+		return err;
+	}
+	of_node_put(dwc3_np);
+
+	dwc3 = platform_get_drvdata(dwc3_pdev);
+	if (!dwc3)
+		return 0;
+
+	if (!IS_ERR_OR_NULL(dwc3->role_sw)) {
+		dwc3_imx->notifier.notifier_call = dwc3_imx8mp_role_event;
+		err = usb_role_switch_register_notifier(dwc3->role_sw,
+							&dwc3_imx->notifier);
+		if (err < 0) {
+			dev_err(dwc3_imx->dev, "failed to register notifier\n");
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 static int __maybe_unused dwc3_imx8mp_suspend(struct dwc3_imx8mp *dwc3_imx,
 					      pm_message_t msg)
 {
 	if (dwc3_imx->pm_suspended)
 		return 0;
 
+	if (!dwc3_imx->notifier.notifier_call)
+		dwc3_imx8mp_register_notifier(dwc3_imx);
+
 	/* Wakeup enable */
 	if (PMSG_IS_AUTO(msg) || device_may_wakeup(dwc3_imx->dev))
 		dwc3_imx8mp_wakeup_enable(dwc3_imx);
@@ -242,31 +248,18 @@ static int __maybe_unused dwc3_imx8mp_suspend(struct dwc3_imx8mp *dwc3_imx,
 static int __maybe_unused dwc3_imx8mp_resume(struct dwc3_imx8mp *dwc3_imx,
 					     pm_message_t msg)
 {
-	struct dwc3	*dwc = platform_get_drvdata(dwc3_imx->dwc3);
 	int ret = 0;
 
 	if (!dwc3_imx->pm_suspended)
 		return 0;
 
+	if (!dwc3_imx->notifier.notifier_call)
+		dwc3_imx8mp_register_notifier(dwc3_imx);
+
 	/* Wakeup disable */
 	dwc3_imx8mp_wakeup_disable(dwc3_imx);
 	dwc3_imx->pm_suspended = false;
 
-	if (dwc3_imx->wakeup_pending) {
-		dwc3_imx->wakeup_pending = false;
-		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE) {
-			pm_runtime_mark_last_busy(dwc->dev);
-			pm_runtime_put_autosuspend(dwc->dev);
-		} else {
-			/*
-			 * Add wait for xhci switch from suspend
-			 * clock to normal clock to detect connection.
-			 */
-			usleep_range(9000, 10000);
-		}
-		enable_irq(dwc3_imx->irq);
-	}
-
 	return ret;
 }
 
@@ -277,9 +270,7 @@ static int __maybe_unused dwc3_imx8mp_pm_suspend(struct device *dev)
 
 	ret = dwc3_imx8mp_suspend(dwc3_imx, PMSG_SUSPEND);
 
-	if (device_may_wakeup(dwc3_imx->dev))
-		enable_irq_wake(dwc3_imx->irq);
-	else
+	if (!device_may_wakeup(dwc3_imx->dev))
 		clk_disable_unprepare(dwc3_imx->suspend_clk);
 
 	clk_disable_unprepare(dwc3_imx->hsio_clk);
@@ -293,9 +284,7 @@ static int __maybe_unused dwc3_imx8mp_pm_resume(struct device *dev)
 	struct dwc3_imx8mp *dwc3_imx = dev_get_drvdata(dev);
 	int ret;
 
-	if (device_may_wakeup(dwc3_imx->dev)) {
-		disable_irq_wake(dwc3_imx->irq);
-	} else {
+	if (!device_may_wakeup(dwc3_imx->dev)) {
 		ret = clk_prepare_enable(dwc3_imx->suspend_clk);
 		if (ret)
 			return ret;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e56f1a6db2de..4ac9d262e608 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2666,6 +2666,16 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 	dwc->gadget_driver	= driver;
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	if (dwc->wakeup_irq > 0) {
+		ret = dev_pm_set_dedicated_wake_irq(dwc->dev,
+						    dwc->wakeup_irq);
+		if (ret) {
+			dev_err(dwc->dev, "set wakeup irq %d failed\n",
+				dwc->wakeup_irq);
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -2681,6 +2691,7 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
 	struct dwc3		*dwc = gadget_to_dwc(g);
 	unsigned long		flags;
 
+	dev_pm_clear_wake_irq(dwc->dev);
 	spin_lock_irqsave(&dwc->lock, flags);
 	dwc->gadget_driver	= NULL;
 	dwc->max_cfg_eps = 0;
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index f29a264635aa..3df0523906e5 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -121,6 +121,17 @@ int dwc3_host_init(struct dwc3 *dwc)
 		goto err;
 	}
 
+	if (dwc->wakeup_irq > 0) {
+		ret = dev_pm_set_dedicated_wake_irq(&xhci->dev,
+						    dwc->wakeup_irq);
+		if (ret) {
+			dev_err(dwc->dev, "set wakeup irq %d failed\n",
+				dwc->wakeup_irq);
+			goto err;
+		}
+		dev_err(&xhci->dev, "wakeup irq of xhci set OK.\n");
+	}
+
 	return 0;
 err:
 	platform_device_put(xhci);
@@ -129,5 +140,6 @@ int dwc3_host_init(struct dwc3 *dwc)
 
 void dwc3_host_exit(struct dwc3 *dwc)
 {
+	dev_pm_clear_wake_irq(&dwc->xhci->dev);
 	platform_device_unregister(dwc->xhci);
 }
-- 
2.25.1


Thanks
Li Jun
> 
> --
> balbi

Re: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Felipe Balbi]

Hi,

Jun Li <jun.li@nxp.com> writes:
>> Jun Li <jun.li@nxp.com> writes:
>> >> > > > Good suggestion, but if extcon notifier listener can't work for
>> >> > > > me, my understanding is this *teach* in glue layer driver still
>> >> > > > need access
>> >> > > > dwc3 core instance struct, right?
>> >> > >
>> >> > > for now, maybe. But it may be better to implement a notifier
>> >> > > method in role switch class.
>> >> >
>> >> > I am not sure if introduce notifier in role switch class is a good
>> >> > idea, I had the impression extcon is not encouraged to use if possible.
>> >>
>> >> I'm not against role switch notifiers. They were proposed before
>> >> already couple of years ago, but at that time there just were no
>> >> users them notifier, so the patch was just dropped from the series [1].
>> >> But I don't think anybody was against the idea. Please feel free to
>> >> add them to the class if you have use for them.
>> >
>> > So I had the incorrect impression.
>> >
>> > Yes, that's the Felipe was referring to, I think.
>> >
>> > I will pick up [1] and improve my driver as Felipe suggested.
>> 
>> sounds great, thahnks Li Jun
>
> Got chance to check this, but it turns out this glue driver
> still has to be care of the dwc3 core and its probe completion,
> because the role switch class is created by dwc3 core in its
> probe.
>
> dev_pm_set_dedicated_wake_irq() is a good solution for case
> a sperate/glue driver is not required, in my imx8mp case, I
> need a glue driver anyway, so this seems can't make my driver
> much simpler. Rough change please see below:  
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index 9f7c7f587d38..bcb63bcf27be 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -874,7 +874,6 @@ usb3_0: usb@32f10100 {
>  			clocks = <&clk IMX8MP_CLK_HSIO_ROOT>,
>  				 <&clk IMX8MP_CLK_USB_ROOT>;
>  			clock-names = "hsio", "suspend";
> -			interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
>  			#address-cells = <1>;
>  			#size-cells = <1>;
>  			dma-ranges = <0x40000000 0x40000000 0xc0000000>;
> @@ -891,7 +890,9 @@ usb_dwc3_0: usb@38100000 {
>  				assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>;
>  				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>;
>  				assigned-clock-rates = <500000000>;
> -				interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> +				interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
> +							<GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
> +				interrupt-names = "irq", "wakeup";
>  				phys = <&usb3_phy0>, <&usb3_phy0>;
>  				phy-names = "usb2-phy", "usb3-phy";
>  				snps,dis-u2-freeclk-exists-quirk;
> @@ -915,7 +916,6 @@ usb3_1: usb@32f10108 {
>  			clocks = <&clk IMX8MP_CLK_HSIO_ROOT>,
>  				 <&clk IMX8MP_CLK_USB_ROOT>;
>  			clock-names = "hsio", "suspend";
> -			interrupts = <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>;
>  			#address-cells = <1>;
>  			#size-cells = <1>;
>  			dma-ranges = <0x40000000 0x40000000 0xc0000000>;
> @@ -932,7 +932,9 @@ usb_dwc3_1: usb@38200000 {
>  				assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>;
>  				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>;
>  				assigned-clock-rates = <500000000>;
> -				interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
> +				interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
> +							<GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>;
> +				interrupt-names = "irq", "wakeup";
>  				phys = <&usb3_phy1>, <&usb3_phy1>;
>  				phy-names = "usb2-phy", "usb3-phy";
>  				snps,dis-u2-freeclk-exists-quirk;
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index b6871ee5e8ca..4dac7cd98a31 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1615,6 +1615,12 @@ static int dwc3_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	dwc->wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
> +	if (dwc->wakeup_irq == -EPROBE_DEFER)
> +		goto assert_reset;
> +	else
> +		dev_err(dwc->dev, "the wakeup irq No. is %d\n", dwc->wakeup_irq);

why is this an error?

-- 
balbi

RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Jun Li]

> -----Original Message-----
> From: Felipe Balbi <balbi@kernel.org>
> Sent: Monday, August 2, 2021 3:49 PM
> To: Jun Li <jun.li@nxp.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>;
> gregkh@linuxfoundation.org; shawnguo@kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; thunder.leizhen@huawei.com;
> linux-usb@vger.kernel.org
> Subject: Re: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible
> string
> 
> 
> Hi,
> 
> Jun Li <jun.li@nxp.com> writes:
> >> Jun Li <jun.li@nxp.com> writes:
> >> >> > > > Good suggestion, but if extcon notifier listener can't work
> >> >> > > > for me, my understanding is this *teach* in glue layer
> >> >> > > > driver still need access
> >> >> > > > dwc3 core instance struct, right?
> >> >> > >
> >> >> > > for now, maybe. But it may be better to implement a notifier
> >> >> > > method in role switch class.
> >> >> >
> >> >> > I am not sure if introduce notifier in role switch class is a
> >> >> > good idea, I had the impression extcon is not encouraged to use if
> possible.
> >> >>
> >> >> I'm not against role switch notifiers. They were proposed before
> >> >> already couple of years ago, but at that time there just were no
> >> >> users them notifier, so the patch was just dropped from the series
> [1].
> >> >> But I don't think anybody was against the idea. Please feel free
> >> >> to add them to the class if you have use for them.
> >> >
> >> > So I had the incorrect impression.
> >> >
> >> > Yes, that's the Felipe was referring to, I think.
> >> >
> >> > I will pick up [1] and improve my driver as Felipe suggested.
> >>
> >> sounds great, thahnks Li Jun
> >
> > Got chance to check this, but it turns out this glue driver still has
> > to be care of the dwc3 core and its probe completion, because the role
> > switch class is created by dwc3 core in its probe.
> >
> > dev_pm_set_dedicated_wake_irq() is a good solution for case a
> > sperate/glue driver is not required, in my imx8mp case, I need a glue
> > driver anyway, so this seems can't make my driver much simpler. Rough
> > change please see below:
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > index 9f7c7f587d38..bcb63bcf27be 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > @@ -874,7 +874,6 @@ usb3_0: usb@32f10100 {
> >  			clocks = <&clk IMX8MP_CLK_HSIO_ROOT>,
> >  				 <&clk IMX8MP_CLK_USB_ROOT>;
> >  			clock-names = "hsio", "suspend";
> > -			interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
> >  			#address-cells = <1>;
> >  			#size-cells = <1>;
> >  			dma-ranges = <0x40000000 0x40000000 0xc0000000>; @@ -891,7 +890,9
> > @@ usb_dwc3_0: usb@38100000 {
> >  				assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>;
> >  				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>;
> >  				assigned-clock-rates = <500000000>;
> > -				interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
> > +				interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
> > +							<GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
> > +				interrupt-names = "irq", "wakeup";
> >  				phys = <&usb3_phy0>, <&usb3_phy0>;
> >  				phy-names = "usb2-phy", "usb3-phy";
> >  				snps,dis-u2-freeclk-exists-quirk; @@ -915,7 +916,6 @@
> usb3_1:
> > usb@32f10108 {
> >  			clocks = <&clk IMX8MP_CLK_HSIO_ROOT>,
> >  				 <&clk IMX8MP_CLK_USB_ROOT>;
> >  			clock-names = "hsio", "suspend";
> > -			interrupts = <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>;
> >  			#address-cells = <1>;
> >  			#size-cells = <1>;
> >  			dma-ranges = <0x40000000 0x40000000 0xc0000000>; @@ -932,7 +932,9
> > @@ usb_dwc3_1: usb@38200000 {
> >  				assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>;
> >  				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>;
> >  				assigned-clock-rates = <500000000>;
> > -				interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
> > +				interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
> > +							<GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>;
> > +				interrupt-names = "irq", "wakeup";
> >  				phys = <&usb3_phy1>, <&usb3_phy1>;
> >  				phy-names = "usb2-phy", "usb3-phy";
> >  				snps,dis-u2-freeclk-exists-quirk; diff --git
> > a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> > b6871ee5e8ca..4dac7cd98a31 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1615,6 +1615,12 @@ static int dwc3_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >
> > +	dwc->wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
> > +	if (dwc->wakeup_irq == -EPROBE_DEFER)
> > +		goto assert_reset;
> > +	else
> > +		dev_err(dwc->dev, "the wakeup irq No. is %d\n", dwc->wakeup_irq);
> 
> why is this an error?

Forget to remove this debug purpose log, sorry.

Li Jun 

> 
> --
> balbi