Re: [PATCH v2 2/5] usb: dwc2: gadget: fix TX FIFO size and address initialization

[Vahram Aharonyan]

On 2/3/2016 3:38 PM, Robert Baldyga wrote:
> According to DWC2 documentation, DPTxFSize field of DPTXFSIZn register
> is read only, which means that software cannot change FIFO size.
> 
> Register description says:
> "The value of this register is the Largest Device Mode Periodic Tx Data
> FIFO Depth (parameter OTG_TX_DPERIO_DFIFO_DEPTH_n), as specified during
> coreConsultant configuration."
> 
> That means, that we have to setup only FIFO start addresses (DPTxFStAddr),
> taking into account reset values of DPTxFSize.
> 
> Initialize FIFO start addresses properly and remove unneeded core related
> to incorrect FIFO size initialization.
> 
> Signed-off-by: Robert Baldyga <r.baldyga@xxxxxxxxxxx>
> ---
>  drivers/usb/dwc2/core.h   |  7 -------
>  drivers/usb/dwc2/gadget.c | 47 ++++++++---------------------------------------
>  2 files changed, 8 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 7fb6434..441da5c 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -208,13 +208,6 @@ enum dwc2_lx_state {
>  	DWC2_L3,	/* Off state */
>  };
>  
> -/*
> - * Gadget periodic tx fifo sizes as used by legacy driver
> - * EP0 is not included
> - */
> -#define DWC2_G_P_LEGACY_TX_FIFO_SIZE {256, 256, 256, 256, 768, 768, 768, \
> -					   768, 0, 0, 0, 0, 0, 0, 0}
> -
>  /* Gadget ep0 states */
>  enum dwc2_ep0_state {
>  	DWC2_EP0_SETUP,
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 0d0f6fe..5de9236 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -170,6 +170,7 @@ static void dwc2_hsotg_init_fifo(struct dwc2_hsotg *hsotg)
>  	unsigned int ep;
>  	unsigned int addr;
>  	int timeout;
> +	u32 dptxfsizn;
>  	u32 val;
>  
>  	/* Reset fifo map if not correctly cleared during previous session */
> @@ -198,13 +199,13 @@ static void dwc2_hsotg_init_fifo(struct dwc2_hsotg *hsotg)
>  	 * given endpoint.
>  	 */
>  	for (ep = 1; ep < MAX_EPS_CHANNELS; ep++) {
> -		if (!hsotg->g_tx_fifo_sz[ep])
> -			continue;
> -		val = addr;
> -		val |= hsotg->g_tx_fifo_sz[ep] << FIFOSIZE_DEPTH_SHIFT;
> -		WARN_ONCE(addr + hsotg->g_tx_fifo_sz[ep] > hsotg->fifo_mem,
> -			  "insufficient fifo memory");
> -		addr += hsotg->g_tx_fifo_sz[ep];
> +		dptxfsizn = dwc2_readl(hsotg->regs + DPTXFSIZN(ep));
> +
> +		val = (dptxfsizn & FIFOSIZE_DEPTH_MASK) | addr;
> +		addr += dptxfsizn >> FIFOSIZE_DEPTH_SHIFT;
> +
> +		if (addr > hsotg->fifo_mem)
> +			break;
>  
>  		dwc2_writel(val, hsotg->regs + DPTXFSIZN(ep));
>  	}
> @@ -3453,36 +3454,10 @@ static void dwc2_hsotg_dump(struct dwc2_hsotg *hsotg)
>  static void dwc2_hsotg_of_probe(struct dwc2_hsotg *hsotg)
>  {
>  	struct device_node *np = hsotg->dev->of_node;
> -	u32 len = 0;
> -	u32 i = 0;
>  
>  	/* Enable dma if requested in device tree */
>  	hsotg->g_using_dma = of_property_read_bool(np, "g-use-dma");
>  
> -	/*
> -	* Register TX periodic fifo size per endpoint.
> -	* EP0 is excluded since it has no fifo configuration.
> -	*/
> -	if (!of_find_property(np, "g-tx-fifo-size", &len))
> -		goto rx_fifo;
> -
> -	len /= sizeof(u32);
> -
> -	/* Read tx fifo sizes other than ep0 */
> -	if (of_property_read_u32_array(np, "g-tx-fifo-size",
> -						&hsotg->g_tx_fifo_sz[1], len))
> -		goto rx_fifo;
> -
> -	/* Add ep0 */
> -	len++;
> -
> -	/* Make remaining TX fifos unavailable */
> -	if (len < MAX_EPS_CHANNELS) {
> -		for (i = len; i < MAX_EPS_CHANNELS; i++)
> -			hsotg->g_tx_fifo_sz[i] = 0;
> -	}
> -
> -rx_fifo:
>  	/* Register RX fifo size */
>  	of_property_read_u32(np, "g-rx-fifo-size", &hsotg->g_rx_fifo_sz);
>  
> @@ -3504,13 +3479,10 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
>  	struct device *dev = hsotg->dev;
>  	int epnum;
>  	int ret;
> -	int i;
> -	u32 p_tx_fifo[] = DWC2_G_P_LEGACY_TX_FIFO_SIZE;
>  
>  	/* Initialize to legacy fifo configuration values */
>  	hsotg->g_rx_fifo_sz = 2048;
>  	hsotg->g_np_g_tx_fifo_sz = 1024;
> -	memcpy(&hsotg->g_tx_fifo_sz[1], p_tx_fifo, sizeof(p_tx_fifo));
>  	/* Device tree specific probe */
>  	dwc2_hsotg_of_probe(hsotg);
>  
> @@ -3528,9 +3500,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
>  	dev_dbg(dev, "NonPeriodic TXFIFO size: %d\n",
>  						hsotg->g_np_g_tx_fifo_sz);
>  	dev_dbg(dev, "RXFIFO size: %d\n", hsotg->g_rx_fifo_sz);
> -	for (i = 0; i < MAX_EPS_CHANNELS; i++)
> -		dev_dbg(dev, "Periodic TXFIFO%2d size: %d\n", i,
> -						hsotg->g_tx_fifo_sz[i]);
>  
>  	hsotg->gadget.max_speed = USB_SPEED_HIGH;
>  	hsotg->gadget.ops = &dwc2_hsotg_gadget_ops;
> 

Hi Robert,

Memory access type of this HW register's field depends on the core configuration. It's read-only in case of shared FIFO mode of the controller. In dedicated FIFO mode with dynamic FIFO sizing enabled this bit-field is writable and SW is allowed to set user-defined values in the HW during core initialization.

In Shared FIFO operation HW register with offset 104h + (FIFO_num -1) * 04h is named DPTXFSIZn with read-only upper bits. For dedicated FIFO mode SW should base on DIEPTXFn register description. 

Thanks,
Vahram.