Re: [PATCH] usb: dwc2: Fix TxFIFOn sizes and total TxFIFO size issues

[John Youn]

On 11/30/2017 12:16 AM, Minas Harutyunyan wrote:
> In host mode reading from DPTXSIZn returning invalid value in
> dwc2_check_param_tx_fifo_sizes function.
>
> In total TxFIFO size calculations unnecessarily reducing by ep_info.
> hw->total_fifo_size can be fully allocated for FIFO's.
>
> Added num_dev_in_eps member in dwc2_hw_params structure to save number
> of IN EPs.
>
> Added g_tx_fifo_size array in dwc2_hw_params structure to store power
> on reset values of DPTXSIZn registers in forced device mode.
>
> Updated dwc2_hsotg_tx_fifo_count() function to get TxFIFO count from
> num_dev_in_eps.
>
> Updated dwc2_get_dev_hwparams() function to store DPTXFSIZn in
> g_tx_fifo_size array.
>
> dwc2_get_host/dev_hwparams() functions call moved after num_dev_in_eps
> set from hwcfg4.
>
> Modified dwc2_check_param_tx_fifo_sizes() function to check TxFIFOn
> sizes based on g_tx_fifo_size array.
>
> Removed ep_info subtraction during calculation of tx_addr_max in
> dwc2_hsotg_tx_fifo_total_depth() function. Also removed
> dwc2_hsotg_ep_info_size() function as no more need.
>
> Signed-off-by: Gevorg Sahakyan <sahakyan@xxxxxxxxxxxx>
> Signed-off-by: Minas Harutyunyan <hminas@xxxxxxxxxxxx>
> ---
>  drivers/usb/dwc2/core.h   |  4 ++++
>  drivers/usb/dwc2/gadget.c | 42 ++----------------------------------------
>  drivers/usb/dwc2/params.c | 29 +++++++++++++++++++----------
>  3 files changed, 25 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 8367d4f985c1..686e9b5527dd 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -532,6 +532,7 @@ struct dwc2_core_params {
>   *                       2 - Internal DMA
>   * @power_optimized     Are power optimizations enabled?
>   * @num_dev_ep          Number of device endpoints available
> + * @num_dev_in_eps      Number of device IN endpoints available
>   * @num_dev_perio_in_ep Number of device periodic IN endpoints
>   *                      available
>   * @dev_token_q_depth   Device Mode IN Token Sequence Learning Queue
> @@ -560,6 +561,7 @@ struct dwc2_core_params {
>   *                       2 - 8 or 16 bits
>   * @snpsid:             Value from SNPSID register
>   * @dev_ep_dirs:        Direction of device endpoints (GHWCFG1)
> + * @g_tx_fifo_size[]	Power-on values of TxFIFO sizes
>   */
>  struct dwc2_hw_params {
>  	unsigned op_mode:3;
> @@ -581,12 +583,14 @@ struct dwc2_hw_params {
>  	unsigned fs_phy_type:2;
>  	unsigned i2c_enable:1;
>  	unsigned num_dev_ep:4;
> +	unsigned num_dev_in_eps : 4;
>  	unsigned num_dev_perio_in_ep:4;
>  	unsigned total_fifo_size:16;
>  	unsigned power_optimized:1;
>  	unsigned utmi_phy_data_width:2;
>  	u32 snpsid;
>  	u32 dev_ep_dirs;
> +	u32 g_tx_fifo_size[MAX_EPS_CHANNELS];
>  };
>
>  /* Size of control and EP0 buffers */
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 0d8e09ccb59c..55950baae437 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -198,55 +198,18 @@ int dwc2_hsotg_tx_fifo_count(struct dwc2_hsotg *hsotg)
>  {
>  	if (hsotg->hw_params.en_multiple_tx_fifo)
>  		/* In dedicated FIFO mode we need count of IN EPs */
> -		return (dwc2_readl(hsotg->regs + GHWCFG4)  &
> -			GHWCFG4_NUM_IN_EPS_MASK) >> GHWCFG4_NUM_IN_EPS_SHIFT;
> +		return hsotg->hw_params.num_dev_in_eps;
>  	else
>  		/* In shared FIFO mode we need count of Periodic IN EPs */
>  		return hsotg->hw_params.num_dev_perio_in_ep;
>  }
>
> -/**
> - * dwc2_hsotg_ep_info_size - return Endpoint Info Control block size in DWORDs
> - */
> -static int dwc2_hsotg_ep_info_size(struct dwc2_hsotg *hsotg)
> -{
> -	int val = 0;
> -	int i;
> -	u32 ep_dirs;
> -
> -	/*
> -	 * Don't need additional space for ep info control registers in
> -	 * slave mode.
> -	 */
> -	if (!using_dma(hsotg)) {
> -		dev_dbg(hsotg->dev, "Buffer DMA ep info size 0\n");
> -		return 0;
> -	}
> -
> -	/*
> -	 * Buffer DMA mode - 1 location per endpoit
> -	 * Descriptor DMA mode - 4 locations per endpoint
> -	 */
> -	ep_dirs = hsotg->hw_params.dev_ep_dirs;
> -
> -	for (i = 0; i <= hsotg->hw_params.num_dev_ep; i++) {
> -		val += ep_dirs & 3 ? 1 : 2;
> -		ep_dirs >>= 2;
> -	}
> -
> -	if (using_desc_dma(hsotg))
> -		val = val * 4;
> -
> -	return val;
> -}
> -
>  /**
>   * dwc2_hsotg_tx_fifo_total_depth - return total FIFO depth available for
>   * device mode TX FIFOs
>   */
>  int dwc2_hsotg_tx_fifo_total_depth(struct dwc2_hsotg *hsotg)
>  {
> -	int ep_info_size;
>  	int addr;
>  	int tx_addr_max;
>  	u32 np_tx_fifo_size;
> @@ -255,8 +218,7 @@ int dwc2_hsotg_tx_fifo_total_depth(struct dwc2_hsotg *hsotg)
>  				hsotg->params.g_np_tx_fifo_size);
>
>  	/* Get Endpoint Info Control block size in DWORDs. */
> -	ep_info_size = dwc2_hsotg_ep_info_size(hsotg);
> -	tx_addr_max = hsotg->hw_params.total_fifo_size - ep_info_size;
> +	tx_addr_max = hsotg->hw_params.total_fifo_size;
>
>  	addr = hsotg->params.g_rx_fifo_size + np_tx_fifo_size;
>  	if (tx_addr_max <= addr)
> diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
> index a3ffe97170ff..ad2ebff74924 100644
> --- a/drivers/usb/dwc2/params.c
> +++ b/drivers/usb/dwc2/params.c
> @@ -469,8 +469,7 @@ static void dwc2_check_param_tx_fifo_sizes(struct dwc2_hsotg *hsotg)
>  	}
>
>  	for (fifo = 1; fifo <= fifo_count; fifo++) {
> -		dptxfszn = (dwc2_readl(hsotg->regs + DPTXFSIZN(fifo)) &
> -			FIFOSIZE_DEPTH_MASK) >> FIFOSIZE_DEPTH_SHIFT;
> +		dptxfszn = hsotg->hw_params.g_tx_fifo_size[fifo];
>
>  		if (hsotg->params.g_tx_fifo_size[fifo] < min ||
>  		    hsotg->params.g_tx_fifo_size[fifo] >  dptxfszn) {
> @@ -594,6 +593,7 @@ static void dwc2_get_dev_hwparams(struct dwc2_hsotg *hsotg)
>  	struct dwc2_hw_params *hw = &hsotg->hw_params;
>  	bool forced;
>  	u32 gnptxfsiz;
> +	int fifo, fifo_count;
>
>  	if (hsotg->dr_mode == USB_DR_MODE_HOST)
>  		return;
> @@ -602,6 +602,14 @@ static void dwc2_get_dev_hwparams(struct dwc2_hsotg *hsotg)
>
>  	gnptxfsiz = dwc2_readl(hsotg->regs + GNPTXFSIZ);
>
> +	fifo_count = dwc2_hsotg_tx_fifo_count(hsotg);
> +
> +	for (fifo = 1; fifo <= fifo_count; fifo++) {
> +		hw->g_tx_fifo_size[fifo] =
> +			(dwc2_readl(hsotg->regs + DPTXFSIZN(fifo)) &
> +			 FIFOSIZE_DEPTH_MASK) >> FIFOSIZE_DEPTH_SHIFT;
> +	}
> +
>  	if (forced)
>  		dwc2_clear_force_mode(hsotg);
>
> @@ -646,14 +654,6 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
>  	hwcfg4 = dwc2_readl(hsotg->regs + GHWCFG4);
>  	grxfsiz = dwc2_readl(hsotg->regs + GRXFSIZ);
>
> -	/*
> -	 * Host specific hardware parameters. Reading these parameters
> -	 * requires the controller to be in host mode. The mode will
> -	 * be forced, if necessary, to read these values.
> -	 */
> -	dwc2_get_host_hwparams(hsotg);
> -	dwc2_get_dev_hwparams(hsotg);
> -
>  	/* hwcfg1 */
>  	hw->dev_ep_dirs = hwcfg1;
>
> @@ -696,6 +696,8 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
>  	hw->en_multiple_tx_fifo = !!(hwcfg4 & GHWCFG4_DED_FIFO_EN);
>  	hw->num_dev_perio_in_ep = (hwcfg4 & GHWCFG4_NUM_DEV_PERIO_IN_EP_MASK) >>
>  				  GHWCFG4_NUM_DEV_PERIO_IN_EP_SHIFT;
> +	hw->num_dev_in_eps = (hwcfg4 & GHWCFG4_NUM_IN_EPS_MASK) >>
> +			     GHWCFG4_NUM_IN_EPS_SHIFT;
>  	hw->dma_desc_enable = !!(hwcfg4 & GHWCFG4_DESC_DMA);
>  	hw->power_optimized = !!(hwcfg4 & GHWCFG4_POWER_OPTIMIZ);
>  	hw->utmi_phy_data_width = (hwcfg4 & GHWCFG4_UTMI_PHY_DATA_WIDTH_MASK) >>
> @@ -704,6 +706,13 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
>  	/* fifo sizes */
>  	hw->rx_fifo_size = (grxfsiz & GRXFSIZ_DEPTH_MASK) >>
>  				GRXFSIZ_DEPTH_SHIFT;
> +	/*
> +	 * Host specific hardware parameters. Reading these parameters
> +	 * requires the controller to be in host mode. The mode will
> +	 * be forced, if necessary, to read these values.
> +	 */
> +	dwc2_get_host_hwparams(hsotg);
> +	dwc2_get_dev_hwparams(hsotg);
>
>  	return 0;
>  }
>


Acked-by: John Youn <johnyoun@xxxxxxxxxxxx>


John