| Message ID | 20230115114146.12628-4-quic_kriskura@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
On Sun, Jan 15, 2023, Krishna Kurapati wrote: > Multiport controllers being host-only capable do not have GEVNTADDR > HI/LO, SIZE, COUNT reigsters present. Accsesing them to setup event I think you should reword "present" to something else. They're still present, but those registers are to be set while operating in device mode. The rest looks fine. Thanks, Thinh > buffers during core_init can cause an SMMU Fault. Avoid event buffers > setup if the GHWPARAMS0 tells that the controller is host-only. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 7e0a9a598dfd..f61ebddaecc0 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -871,9 +871,12 @@ static void dwc3_clk_disable(struct dwc3 *dwc) > > static void dwc3_core_exit(struct dwc3 *dwc) > { > - int i; > + int i; > + unsigned int hw_mode; > > - dwc3_event_buffers_cleanup(dwc); > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) > + dwc3_event_buffers_cleanup(dwc); > > usb_phy_set_suspend(dwc->usb2_phy, 1); > usb_phy_set_suspend(dwc->usb3_phy, 1); > @@ -1246,10 +1249,12 @@ static int dwc3_core_init(struct dwc3 *dwc) > } > } > > - ret = dwc3_event_buffers_setup(dwc); > - if (ret) { > - dev_err(dwc->dev, "failed to setup event buffers\n"); > - goto err4; > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) { > + ret = dwc3_event_buffers_setup(dwc); > + if (ret) { > + dev_err(dwc->dev, "failed to setup event buffers\n"); > + goto err4; > + } > } > > /* > @@ -1886,7 +1891,7 @@ static int dwc3_probe(struct platform_device *pdev) > struct resource *res, dwc_res; > struct dwc3 *dwc; > int ret, i; > - > + unsigned int hw_mode; > void __iomem *regs; > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > @@ -2063,7 +2068,9 @@ static int dwc3_probe(struct platform_device *pdev) > err5: > dwc3_debugfs_exit(dwc); > > - dwc3_event_buffers_cleanup(dwc); > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) > + dwc3_event_buffers_cleanup(dwc); > > usb_phy_set_suspend(dwc->usb2_phy, 1); > usb_phy_set_suspend(dwc->usb3_phy, 1); > -- > 2.39.0 >
Hi Thinh, On Thu, Jan 19, 2023 at 12:38:51AM +0000, Thinh Nguyen wrote: > On Sun, Jan 15, 2023, Krishna Kurapati wrote: > > Multiport controllers being host-only capable do not have GEVNTADDR Multiport may not be relevant here. Host-only is though. > > HI/LO, SIZE, COUNT reigsters present. Accsesing them to setup event > > I think you should reword "present" to something else. They're still > present In our case we have an instance where the IP is statically configured via coreConsultant with DWC_USB31_MODE==1 (host only) and we did observe that none of the registers pertaining to device mode (including GEVNT* and of course all the D* ones) are even *present* in the register map. If we try to access them we encounter some kind of access error or stall (or translation fault as described). So the approach here is to first verify by checking the HWPARAMS0 register if the HW is even capable of device mode in the first place. > but those registers are to be set while operating in device > mode. The rest looks fine. Are you suggesting only touching the GEVNT* registers when *operating* in device mode, even in the case of a dual-role capable controller? In that case would it make more sense to additionally move the calls to dwc3_event_buffers_{setup,cleanup} out of core.c and into dwc3_gadget_{init,exit} perhaps? That way we avoid them completely unless and until we switch into peripheral mode (assuming controller supports that, which we should already have checks for). Moreover, if the devicetree dr_mode property is set to host-only we'd also avoid calling these. > > buffers during core_init can cause an SMMU Fault. Avoid event buffers > > setup if the GHWPARAMS0 tells that the controller is host-only. > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > --- > > drivers/usb/dwc3/core.c | 23 +++++++++++++++-------- > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 7e0a9a598dfd..f61ebddaecc0 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -871,9 +871,12 @@ static void dwc3_clk_disable(struct dwc3 *dwc) > > > > static void dwc3_core_exit(struct dwc3 *dwc) > > { > > - int i; > > + int i; > > + unsigned int hw_mode; > > > > - dwc3_event_buffers_cleanup(dwc); > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) If we stick with this approach, we probably could just check dwc->dr_mode instead as probe should have already set that to be an intersection between the values given in devicetree "dr_mode" and the HWPARAMS0 capability. Thanks, Jack > > + dwc3_event_buffers_cleanup(dwc); > > > > usb_phy_set_suspend(dwc->usb2_phy, 1); > > usb_phy_set_suspend(dwc->usb3_phy, 1); > > @@ -1246,10 +1249,12 @@ static int dwc3_core_init(struct dwc3 *dwc) > > } > > } > > > > - ret = dwc3_event_buffers_setup(dwc); > > - if (ret) { > > - dev_err(dwc->dev, "failed to setup event buffers\n"); > > - goto err4; > > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) { > > + ret = dwc3_event_buffers_setup(dwc); > > + if (ret) { > > + dev_err(dwc->dev, "failed to setup event buffers\n"); > > + goto err4; > > + } > > } > > > > /* > > @@ -1886,7 +1891,7 @@ static int dwc3_probe(struct platform_device *pdev) > > struct resource *res, dwc_res; > > struct dwc3 *dwc; > > int ret, i; > > - > > + unsigned int hw_mode; > > void __iomem *regs; > > > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > > @@ -2063,7 +2068,9 @@ static int dwc3_probe(struct platform_device *pdev) > > err5: > > dwc3_debugfs_exit(dwc); > > > > - dwc3_event_buffers_cleanup(dwc); > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) > > + dwc3_event_buffers_cleanup(dwc); > > > > usb_phy_set_suspend(dwc->usb2_phy, 1); > > usb_phy_set_suspend(dwc->usb3_phy, 1); > > -- > > 2.39.0 > >
On Wed, Jan 18, 2023, Jack Pham wrote: > Hi Thinh, > > On Thu, Jan 19, 2023 at 12:38:51AM +0000, Thinh Nguyen wrote: > > On Sun, Jan 15, 2023, Krishna Kurapati wrote: > > > Multiport controllers being host-only capable do not have GEVNTADDR > > Multiport may not be relevant here. Host-only is though. > > > > HI/LO, SIZE, COUNT reigsters present. Accsesing them to setup event > > > > I think you should reword "present" to something else. They're still > > present > > In our case we have an instance where the IP is statically configured > via coreConsultant with DWC_USB31_MODE==1 (host only) and we did observe > that none of the registers pertaining to device mode (including GEVNT* > and of course all the D* ones) are even *present* in the register map. > If we try to access them we encounter some kind of access error or stall > (or translation fault as described). So the approach here is to first > verify by checking the HWPARAMS0 register if the HW is even capable of > device mode in the first place. I see. > > > but those registers are to be set while operating in device > > mode. The rest looks fine. > > Are you suggesting only touching the GEVNT* registers when *operating* > in device mode, even in the case of a dual-role capable controller? In > that case would it make more sense to additionally move the calls to > dwc3_event_buffers_{setup,cleanup} out of core.c and into > dwc3_gadget_{init,exit} perhaps? That way we avoid them completely While it shouldn't be a problem for DRD, it may be cleaner to do that. > unless and until we switch into peripheral mode (assuming controller > supports that, which we should already have checks for). Moreover, if > the devicetree dr_mode property is set to host-only we'd also avoid > calling these. > > > > buffers during core_init can cause an SMMU Fault. Avoid event buffers > > > setup if the GHWPARAMS0 tells that the controller is host-only. > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > --- > > > drivers/usb/dwc3/core.c | 23 +++++++++++++++-------- > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > index 7e0a9a598dfd..f61ebddaecc0 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > @@ -871,9 +871,12 @@ static void dwc3_clk_disable(struct dwc3 *dwc) > > > > > > static void dwc3_core_exit(struct dwc3 *dwc) > > > { > > > - int i; > > > + int i; > > > + unsigned int hw_mode; > > > > > > - dwc3_event_buffers_cleanup(dwc); > > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) > > If we stick with this approach, we probably could just check > dwc->dr_mode instead as probe should have already set that to be an > intersection between the values given in devicetree "dr_mode" and the > HWPARAMS0 capability. > What we have here should not break DRD, so it's fine either way. Thanks, Thinh
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 7e0a9a598dfd..f61ebddaecc0 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -871,9 +871,12 @@ static void dwc3_clk_disable(struct dwc3 *dwc) static void dwc3_core_exit(struct dwc3 *dwc) { - int i; + int i; + unsigned int hw_mode; - dwc3_event_buffers_cleanup(dwc); + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) + dwc3_event_buffers_cleanup(dwc); usb_phy_set_suspend(dwc->usb2_phy, 1); usb_phy_set_suspend(dwc->usb3_phy, 1); @@ -1246,10 +1249,12 @@ static int dwc3_core_init(struct dwc3 *dwc) } } - ret = dwc3_event_buffers_setup(dwc); - if (ret) { - dev_err(dwc->dev, "failed to setup event buffers\n"); - goto err4; + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) { + ret = dwc3_event_buffers_setup(dwc); + if (ret) { + dev_err(dwc->dev, "failed to setup event buffers\n"); + goto err4; + } } /* @@ -1886,7 +1891,7 @@ static int dwc3_probe(struct platform_device *pdev) struct resource *res, dwc_res; struct dwc3 *dwc; int ret, i; - + unsigned int hw_mode; void __iomem *regs; dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); @@ -2063,7 +2068,9 @@ static int dwc3_probe(struct platform_device *pdev) err5: dwc3_debugfs_exit(dwc); - dwc3_event_buffers_cleanup(dwc); + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) + dwc3_event_buffers_cleanup(dwc); usb_phy_set_suspend(dwc->usb2_phy, 1); usb_phy_set_suspend(dwc->usb3_phy, 1);
Multiport controllers being host-only capable do not have GEVNTADDR HI/LO, SIZE, COUNT reigsters present. Accsesing them to setup event buffers during core_init can cause an SMMU Fault. Avoid event buffers setup if the GHWPARAMS0 tells that the controller is host-only. Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> --- drivers/usb/dwc3/core.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)