Re: [PATCH v3] usb: dwc3: enable CCI support for AMD-xilinx DWC3 controller

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Mon, Jun 10, 2024, Pandey, Radhey Shyam wrote:
> > -----Original Message-----
> > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> > Sent: Saturday, June 8, 2024 5:39 AM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>;
> > gregkh@linuxfoundation.org; Simek, Michal <michal.simek@amd.com>;
> > robh+dt@kernel.org; krzysztof.kozlowski@linaro.org; linux-
> > usb@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>
> > Subject: Re: [PATCH v3] usb: dwc3: enable CCI support for AMD-xilinx DWC3
> > controller
> > 
> > Hi,
> > 
> > On Thu, Jun 06, 2024, Radhey Shyam Pandey wrote:
> > > The GSBUSCFG0 register bits [31:16] are used to configure the cache type
> > > settings of the descriptor and data write/read transfers (Cacheable,
> > > Bufferable/Posted). When CCI is enabled in the design, DWC3 core
> > GSBUSCFG0
> > > cache bits must be updated to support CCI enabled transfers in USB.
> > >
> > > To program GSBUSCFG0 cache bits create a software node property
> > > in AMD-xilinx dwc3 glue driver and pass it to dwc3 core. The core
> > > then reads this property value and configures it in dwc3_core_init()
> > > sequence.
> > >
> > > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> > > ---
> > > Changes for v3:
> > > In v2 review as suggested by Thinh Nguyen, switch to swnode
> > implementation
> > > for passing GSBUSCFG0 cache bits from AMD-xilinx dwc3 glue driver to
> > > core driver.
> > >
> > > Changes for v2:
> > > Make GSBUSCFG0 configuration specific to AMD-xilinx platform.
> > > Taken reference from existing commit ec5eb43813a4 ("usb: dwc3: core:
> > > add support for realtek SoCs custom's global register start address")
> > >
> > > v1 link:
> > >
> > https://urldefense.com/v3/__https://lore.kernel.org/all/20231013053448.11
> > 056-1-piyush.mehta@amd.com__;!!A4F2R9G_pg!cOoWxmacxPeYVCxDfg3-
> > xlQLhKm8MIEgwWx45cLQjgwRWA4e4QyY_kGVVHo2m_dcRbpBQEFpB9JsYP6
> > nzasK2AIAsyefjQ$
> > > ---
> > >  drivers/usb/dwc3/core.c        | 24 ++++++++++++++++++++++++
> > >  drivers/usb/dwc3/core.h        |  8 ++++++++
> > >  drivers/usb/dwc3/dwc3-xilinx.c | 18 +++++++++++++++++-
> > >  3 files changed, 49 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 7ee61a89520b..159d21b25629 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -23,6 +23,7 @@
> > >  #include <linux/delay.h>
> > >  #include <linux/dma-mapping.h>
> > >  #include <linux/of.h>
> > > +#include <linux/of_address.h>
> > >  #include <linux/of_graph.h>
> > >  #include <linux/acpi.h>
> > >  #include <linux/pinctrl/consumer.h>
> > > @@ -599,6 +600,19 @@ static void dwc3_cache_hwparams(struct dwc3
> > *dwc)
> > >  		parms->hwparams9 = dwc3_readl(dwc->regs,
> > DWC3_GHWPARAMS9);
> > >  }
> > >
> > > +static void dwc3_config_soc_bus(struct dwc3 *dwc)
> > > +{
> > > +	if (of_dma_is_coherent(dwc->dev->of_node)) {
> > 
> > This can be applicable outside of of_node, do we need this if case?
> 
> This of_dma_is_coherent detect presence of dma-coherent property 
> in dwc3 node and then if propery is present we program 
> GSBUSCFG0_REQINFO.
> 
> Alternatively glue driver can also read/detect DWC3 node dma-coherent 
> property and then override  snps,acache-data-rd-wr-info value only 
> if dma-coherent property is present. 

The user/designer of the platform should decide whether this property is
applicable to the platform. In this case, this logic should be applied
from your glue driver. If the user sets this property, apply the value
to the GSBUSCFG0 regardless of dma-coherent.

> 
> Or do you mean we should support presence of dma-coherent
> property in either parent node ("xlnx,zynqmp-dwc3") or child
> node(snps,dwc3)?

See above.

> 
> > 
> > > +		u32 reg;
> > > +
> > > +		reg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0);
> > > +		reg &=
> > ~DWC3_GSBUSCFG0_DAT_DES_RD_WR_REQINFO_MASK;
> > > +		reg |= (dwc->acache_data_rd_wr_info <<
> > 
> > What if the user doesn't specify this property? We should not
> > automatically write 0 by default.
> 
> USB3 3.30a core configured for 2.0 I see reset value for 
> GSBUSCFG0 [31:16] bits are 0.  Will cross-check it for other
> versions.

Other platform may have a default setting from coreConsultant that's not
0. Let's not write 0 by default.

> 
> > 
> > > +
> > 	DWC3_GSBUSCFG0_DAT_DES_RD_WR_REQINFO_SHIFT);
> > > +		dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, reg);
> > > +	}
> > > +}
> > > +
> > >  static int dwc3_core_ulpi_init(struct dwc3 *dwc)
> > >  {
> > >  	int intf;
> > > @@ -1320,6 +1334,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
> > >
> > >  	dwc3_set_incr_burst_type(dwc);
> > >
> > > +	dwc3_config_soc_bus(dwc);
> > > +
> > >  	ret = dwc3_phy_power_on(dwc);
> > >  	if (ret)
> > >  		goto err_exit_phy;
> > > @@ -1574,6 +1590,7 @@ static void dwc3_get_properties(struct dwc3
> > *dwc)
> > >  	u8			tx_max_burst_prd = 0;
> > >  	u8			tx_fifo_resize_max_num;
> > >  	const char		*usb_psy_name;
> > > +	struct device		*tmpdev;
> > >  	int			ret;
> > >
> > >  	/* default to highest possible threshold */
> > > @@ -1716,6 +1733,13 @@ static void dwc3_get_properties(struct dwc3
> > *dwc)
> > >  	dwc->dis_split_quirk = device_property_read_bool(dev,
> > >  				"snps,dis-split-quirk");
> > >
> > > +	/* Iterate over all parent nodes for finding swnode properties */
> > > +	for (tmpdev = dwc->dev; tmpdev; tmpdev = tmpdev->parent) {
> > > +		device_property_read_u16(tmpdev,
> > > +					 "snps,acache-data-rd-wr-info",
> > > +					  &dwc->acache_data_rd_wr_info);
> > > +	}
> > > +
> > 
> > Please split this to a separate function and name it as
> > dwc3_get_software_properties(). For now, just the new property you
> > create is fine. We can introduce new patches to move all the software
> > defined properties (ie. non ABI/DS) there such as sysdev_is_parent.
> 
> Sure , will split it to separate patch.
> > 
> > >  	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
> > >  	dwc->tx_de_emphasis = tx_de_emphasis;
> > >
> > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > > index 3781c736c1a1..57b3cb739353 100644
> > > --- a/drivers/usb/dwc3/core.h
> > > +++ b/drivers/usb/dwc3/core.h
> > > @@ -194,6 +194,10 @@
> > >  #define DWC3_GSBUSCFG0_INCRBRSTENA	(1 << 0) /* undefined length
> > enable */
> > >  #define DWC3_GSBUSCFG0_INCRBRST_MASK	0xff
> > >
> > > +/* Global SoC Bus Configuration Register: AHB-prot/AXI-cache/OCP-
> > ReqInfo */
> > > +#define DWC3_GSBUSCFG0_DAT_DES_RD_WR_REQINFO_MASK
> > 	GENMASK(31, 16)
> > 
> > Can we rename this to DWC3_GSBUSCFG0_REQINFO_MASK
> 
> Yes, will do it in next version.
> 
> > 
> > > +#define DWC3_GSBUSCFG0_DAT_DES_RD_WR_REQINFO_SHIFT	16
> > 
> > We don't need a shift value. Either define DWC3_GSBUSCFG0_REQINFO(n) or
> > use FIELD_PREP and the mask.
> 
> Yes, will fix it in next version.
> > 
> > > +
> > >  /* Global Debug LSP MUX Select */
> > >  #define DWC3_GDBGLSPMUX_ENDBC		BIT(15)	/* Host only
> > */
> > >  #define DWC3_GDBGLSPMUX_HOSTSELECT(n)	((n) & 0x3fff)
> > > @@ -1153,6 +1157,9 @@ struct dwc3_scratchpad_array {
> > >   * @num_ep_resized: carries the current number endpoints which have
> > had its tx
> > >   *		    fifo resized.
> > >   * @debug_root: root debugfs directory for this device to put its files in.
> > > + * @acache_data_rd_wr_info: store GSBUSCFG0.DATRDREQINFO,
> > DESRDREQINFO,
> > > + *                          DATWRREQINFO, and DESWRREQINFO value passed from
> > > + *                          glue driver.
> > >   */
> > >  struct dwc3 {
> > >  	struct work_struct	drd_work;
> > > @@ -1380,6 +1387,7 @@ struct dwc3 {
> > >  	int			last_fifo_depth;
> > >  	int			num_ep_resized;
> > >  	struct dentry		*debug_root;
> > > +	u16			acache_data_rd_wr_info;
> > 
> > If we do need to keep this field around, please define the default
> > for unspecified value. Also rename this to gsbuscfg0_reqinfo.
> 
> Sure. will define default (likely 0 ) and rename it to gsbuscfg0_reqinfo 
> in next version.

The default should should not be a valid value that can be set. Perhaps
use type u32 and define 0xffffffff as unspecified?

Thanks,
Thinh