Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 18 Jan 2023 17:57:19 -0800
From:   Jack Pham <quic_jackp@quicinc.com>
To:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>
CC:     Krishna Kurapati <quic_kriskura@quicinc.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        "Andy Gross" <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        "Konrad Dybcio" <konrad.dybcio@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Felipe Balbi <balbi@kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "quic_pkondeti@quicinc.com" <quic_pkondeti@quicinc.com>,
        "quic_ppratap@quicinc.com" <quic_ppratap@quicinc.com>,
        "quic_wcheng@quicinc.com" <quic_wcheng@quicinc.com>,
        "quic_harshq@quicinc.com" <quic_harshq@quicinc.com>
Subject: Re: [RFC v4 3/5] usb: dwc3: core: Do not setup event buffers for
 host only controllers
Message-ID: <20230119015535.GF28337@jackp-linux.qualcomm.com>
References: <20230115114146.12628-1-quic_kriskura@quicinc.com>
 <20230115114146.12628-4-quic_kriskura@quicinc.com>
 <20230119003850.id3gtcokdim5pvf7@synopsys.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230119003850.id3gtcokdim5pvf7@synopsys.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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