diff mbox series

[RFC,v4,3/5] usb: dwc3: core: Do not setup event buffers for host only controllers

Message ID 20230115114146.12628-4-quic_kriskura@quicinc.com
State New
Headers show
Series Add multiport support for DWC3 controllers | expand

Commit Message

Krishna Kurapati Jan. 15, 2023, 11:41 a.m. UTC
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(-)

Comments

Jack Pham Jan. 19, 2023, 1:57 a.m. UTC | #1
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
> >
Thinh Nguyen Jan. 19, 2023, 2:32 a.m. UTC | #2
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 mbox series

Patch

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);