Hi,
> -----Original Message-----
> From: Felipe Balbi <balbi@xxxxxxxxxx>
> Sent: Friday, April 30, 2021 6:00 PM
> To: Jun Li <jun.li@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx
> Cc: shawnguo@xxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> thunder.leizhen@xxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible
> string
>
>
> Hi,
>
> Jun Li <jun.li@xxxxxxx> writes:
> >> -----Original Message-----
> >> From: Felipe Balbi <balbi@xxxxxxxxxx>
> >> Sent: Friday, April 30, 2021 4:24 PM
> >> To: Jun Li <jun.li@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx
> >> Cc: shawnguo@xxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> >> thunder.leizhen@xxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx
> >> Subject: Re: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via
> >> compatible string
> >>
> >> Li Jun <jun.li@xxxxxxx> 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@xxxxxxx>
> >>
> >>
> >> 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@xxxxxxxxxx>
> >>
> >> 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
