Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 4 Jun 2021 16:36:11 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Felipe Balbi <balbi@kernel.org>
Cc:     Wesley Cheng <wcheng@codeaurora.org>, agross@kernel.org,
        bjorn.andersson@linaro.org, robh+dt@kernel.org,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        jackp@codeaurora.org, Thinh.Nguyen@synopsys.com
Subject: Re: [PATCH v9 0/5] Re-introduce TX FIFO resize for larger EP bursting
Message-ID: <YLo6W5sKaXvy51eW@kroah.com>
References: <1621410561-32762-1-git-send-email-wcheng@codeaurora.org>
 <YLoUiO8tpRpmvcyU@kroah.com>
 <87k0n9btnb.fsf@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <87k0n9btnb.fsf@kernel.org>
Precedence: bulk

On Fri, Jun 04, 2021 at 05:18:16PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Greg KH <gregkh@linuxfoundation.org> writes:
> > On Wed, May 19, 2021 at 12:49:16AM -0700, Wesley Cheng wrote:
> >> Changes in V9:
> >>  - Fixed incorrect patch in series.  Removed changes in DTSI, as dwc3-qcom will
> >>    add the property by default from the kernel.
> >
> > This patch series has one build failure and one warning added:
> >
> > drivers/usb/dwc3/gadget.c: In function ‘dwc3_gadget_calc_tx_fifo_size’:
> > drivers/usb/dwc3/gadget.c:653:45: warning: passing argument 1 of ‘dwc3_mdwidth’ makes pointer from integer without a cast [-Wint-conversion]
> >   653 |         mdwidth = dwc3_mdwidth(dwc->hwparams.hwparams0);
> >       |                                ~~~~~~~~~~~~~^~~~~~~~~~
> >       |                                             |
> >       |                                             u32 {aka unsigned int}
> > In file included from drivers/usb/dwc3/debug.h:14,
> >                  from drivers/usb/dwc3/gadget.c:25:
> > drivers/usb/dwc3/core.h:1493:45: note: expected ‘struct dwc3 *’ but argument is of type ‘u32’ {aka ‘unsigned int’}
> >  1493 | static inline u32 dwc3_mdwidth(struct dwc3 *dwc)
> >       |                                ~~~~~~~~~~~~~^~~
> >
> >
> > drivers/usb/dwc3/dwc3-qcom.c: In function ‘dwc3_qcom_of_register_core’:
> > drivers/usb/dwc3/dwc3-qcom.c:660:23: error: implicit declaration of function ‘of_add_property’; did you mean ‘of_get_property’? [-Werror=implicit-function-declaration]
> >   660 |                 ret = of_add_property(dwc3_np, prop);
> >       |                       ^~~~~~~~~~~~~~~
> >       |                       of_get_property
> >
> >
> > How did you test these?
> 
> to be honest, I don't think these should go in (apart from the build
> failure) because it's likely to break instantiations of the core with
> differing FIFO sizes. Some instantiations even have some endpoints with
> dedicated functionality that requires the default FIFO size configured
> during coreConsultant instantiation. I know of at OMAP5 and some Intel
> implementations which have dedicated endpoints for processor tracing.
> 
> With OMAP5, these endpoints are configured at the top of the available
> endpoints, which means that if a gadget driver gets loaded and takes
> over most of the FIFO space because of this resizing, processor tracing
> will have a hard time running. That being said, processor tracing isn't
> supported in upstream at this moment.
> 
> I still think this may cause other places to break down. The promise the
> databook makes is that increasing the FIFO size over 2x wMaxPacketSize
> should bring little to no benefit, if we're not maintaining that, I
> wonder if the problem is with some of the BUSCFG registers instead,
> where we configure interconnect bursting and the like.

Good points.

Wesley, what kind of testing have you done on this on different devices?

thanks,

greg k-h