Re: [PATCH v3 1/2] usb: dwc3: core: configure TX/RX threshold for DWC3_IP

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

On Tue, Sep 12, 2023, Wesley Cheng wrote:
> Hi,
> 
> On 9/11/2023 9:19 PM, Stanley Chang wrote:
> > In Synopsys's dwc3 data book:
> > To avoid underrun and overrun during the burst, in a high-latency bus
> > system (like USB), threshold and burst size control is provided through
> > GTXTHRCFG and GRXTHRCFG registers.
> > 
> > In Realtek DHC SoC, DWC3 USB 3.0 uses AHB system bus. When dwc3 is
> > connected with USB 2.5G Ethernet, there will be overrun problem.
> > Therefore, setting TX/RX thresholds can avoid this issue.
> > 
> > Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> > ---
> > v2 to v3 change:
> >      Move the threshold setting to new function dwc3_config_threshold.
> >      Add the threshold setting for usb31 and usb32
> > v1 to v2 change:
> >      Add the properties for TX/RX threshold setting
> > ---
> >   drivers/usb/dwc3/core.c | 160 +++++++++++++++++++++++++++++++---------
> >   drivers/usb/dwc3/core.h |  13 ++++
> >   2 files changed, 137 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 9c6bf054f15d..44ee8526dc28 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1057,6 +1057,111 @@ static void dwc3_set_power_down_clk_scale(struct dwc3 *dwc)
> >   	}
> >   }
> > +static void dwc3_config_threshold(struct dwc3 *dwc)
> > +{
> > +	u32 reg;
> > +	u8 rx_thr_num;
> > +	u8 rx_maxburst;
> > +	u8 tx_thr_num;
> > +	u8 tx_maxburst;
> > +
> > +	/*
> > +	 * Must config both number of packets and max burst settings to enable
> > +	 * RX and/or TX threshold.
> > +	 */
> > +	if (!DWC3_IP_IS(DWC3) && dwc->dr_mode == USB_DR_MODE_HOST) {
> > +		rx_thr_num = dwc->rx_thr_num_pkt_prd;
> > +		rx_maxburst = dwc->rx_max_burst_prd;
> > +		tx_thr_num = dwc->tx_thr_num_pkt_prd;
> > +		tx_maxburst = dwc->tx_max_burst_prd;
> > +
> > +		if (rx_thr_num && rx_maxburst) {
> > +			reg = dwc3_readl(dwc->regs, DWC3_GRXTHRCFG);
> > +			reg |= DWC31_RXTHRNUMPKTSEL_PRD;
> > +
> > +			reg &= ~DWC31_RXTHRNUMPKT_PRD(~0);
> > +			reg |= DWC31_RXTHRNUMPKT_PRD(rx_thr_num);
> > +
> > +			reg &= ~DWC31_MAXRXBURSTSIZE_PRD(~0);
> > +			reg |= DWC31_MAXRXBURSTSIZE_PRD(rx_maxburst);
> > +
> > +			dwc3_writel(dwc->regs, DWC3_GRXTHRCFG, reg);
> > +		}
> > +
> > +		if (tx_thr_num && tx_maxburst) {
> > +			reg = dwc3_readl(dwc->regs, DWC3_GTXTHRCFG);
> > +			reg |= DWC31_TXTHRNUMPKTSEL_PRD;
> > +
> > +			reg &= ~DWC31_TXTHRNUMPKT_PRD(~0);
> > +			reg |= DWC31_TXTHRNUMPKT_PRD(tx_thr_num);
> > +
> > +			reg &= ~DWC31_MAXTXBURSTSIZE_PRD(~0);
> > +			reg |= DWC31_MAXTXBURSTSIZE_PRD(tx_maxburst);
> > +
> > +			dwc3_writel(dwc->regs, DWC3_GTXTHRCFG, reg);
> > +		}
> > +	}
> > +
> > +	rx_thr_num = dwc->rx_thr_num_pkt;
> > +	rx_maxburst = dwc->rx_max_burst;
> > +	tx_thr_num = dwc->tx_thr_num_pkt;
> > +	tx_maxburst = dwc->tx_max_burst;
> > +
> > +	if (DWC3_IP_IS(DWC3)) {
> > +		if (rx_thr_num && rx_maxburst) {
> > +			reg = dwc3_readl(dwc->regs, DWC3_GRXTHRCFG);
> > +			reg |= DWC3_GRXTHRCFG_PKTCNTSEL;
> > +
> > +			reg &= ~DWC3_GRXTHRCFG_RXPKTCNT(~0);
> > +			reg |= DWC3_GRXTHRCFG_RXPKTCNT(rx_thr_num);
> > +
> > +			reg &= ~DWC3_GRXTHRCFG_MAXRXBURSTSIZE(~0);
> > +			reg |= DWC3_GRXTHRCFG_MAXRXBURSTSIZE(rx_maxburst);
> > +
> > +			dwc3_writel(dwc->regs, DWC3_GRXTHRCFG, reg);
> > +		}
> > +
> > +		if (tx_thr_num && tx_maxburst) {
> > +			reg = dwc3_readl(dwc->regs, DWC3_GTXTHRCFG);
> > +			reg |= DWC3_GTXTHRCFG_PKTCNTSEL;
> > +
> > +			reg &= ~DWC3_GTXTHRCFG_TXPKTCNT(~0);
> > +			reg |= DWC3_GTXTHRCFG_TXPKTCNT(tx_thr_num);
> > +
> > +			reg &= ~DWC3_GTXTHRCFG_MAXTXBURSTSIZE(~0);
> > +			reg |= DWC3_GTXTHRCFG_MAXTXBURSTSIZE(tx_maxburst);
> > +
> > +			dwc3_writel(dwc->regs, DWC3_GTXTHRCFG, reg);
> > +		}
> > +	} else {
> > +		if (rx_thr_num && rx_maxburst) {
> > +			reg = dwc3_readl(dwc->regs, DWC3_GRXTHRCFG);
> > +			reg |= DWC31_GRXTHRCFG_PKTCNTSEL;
> > +
> > +			reg &= ~DWC31_GRXTHRCFG_RXPKTCNT(~0);
> > +			reg |= DWC31_GRXTHRCFG_RXPKTCNT(rx_thr_num);
> > +
> > +			reg &= ~DWC31_GRXTHRCFG_MAXRXBURSTSIZE(~0);
> > +			reg |= DWC31_GRXTHRCFG_MAXRXBURSTSIZE(rx_maxburst);
> > +
> > +			dwc3_writel(dwc->regs, DWC3_GRXTHRCFG, reg);
> > +		}
> > +
> > +		if (tx_thr_num && tx_maxburst) {
> > +			reg = dwc3_readl(dwc->regs, DWC3_GTXTHRCFG);
> > +			reg |= DWC31_GTXTHRCFG_PKTCNTSEL;
> > +
> > +			reg &= ~DWC31_GTXTHRCFG_TXPKTCNT(~0);
> > +			reg |= DWC31_GTXTHRCFG_TXPKTCNT(tx_thr_num);
> > +
> > +			reg &= ~DWC31_GTXTHRCFG_MAXTXBURSTSIZE(~0);
> > +			reg |= DWC31_GTXTHRCFG_MAXTXBURSTSIZE(tx_maxburst);
> > +
> > +			dwc3_writel(dwc->regs, DWC3_GTXTHRCFG, reg);
> > +		}
> > +	}
> > +}
> > +
> >   /**
> >    * dwc3_core_init - Low-level initialization of DWC3 Core
> >    * @dwc: Pointer to our controller context structure
> > @@ -1209,42 +1314,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
> >   		dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
> >   	}
> > -	/*
> > -	 * Must config both number of packets and max burst settings to enable
> > -	 * RX and/or TX threshold.
> > -	 */
> > -	if (!DWC3_IP_IS(DWC3) && dwc->dr_mode == USB_DR_MODE_HOST) {
> > -		u8 rx_thr_num = dwc->rx_thr_num_pkt_prd;
> > -		u8 rx_maxburst = dwc->rx_max_burst_prd;
> > -		u8 tx_thr_num = dwc->tx_thr_num_pkt_prd;
> > -		u8 tx_maxburst = dwc->tx_max_burst_prd;
> > -
> > -		if (rx_thr_num && rx_maxburst) {
> > -			reg = dwc3_readl(dwc->regs, DWC3_GRXTHRCFG);
> > -			reg |= DWC31_RXTHRNUMPKTSEL_PRD;
> > -
> > -			reg &= ~DWC31_RXTHRNUMPKT_PRD(~0);
> > -			reg |= DWC31_RXTHRNUMPKT_PRD(rx_thr_num);
> > -
> > -			reg &= ~DWC31_MAXRXBURSTSIZE_PRD(~0);
> > -			reg |= DWC31_MAXRXBURSTSIZE_PRD(rx_maxburst);
> > -
> > -			dwc3_writel(dwc->regs, DWC3_GRXTHRCFG, reg);
> > -		}
> > -
> > -		if (tx_thr_num && tx_maxburst) {
> > -			reg = dwc3_readl(dwc->regs, DWC3_GTXTHRCFG);
> > -			reg |= DWC31_TXTHRNUMPKTSEL_PRD;
> > -
> > -			reg &= ~DWC31_TXTHRNUMPKT_PRD(~0);
> > -			reg |= DWC31_TXTHRNUMPKT_PRD(tx_thr_num);
> > -
> > -			reg &= ~DWC31_MAXTXBURSTSIZE_PRD(~0);
> > -			reg |= DWC31_MAXTXBURSTSIZE_PRD(tx_maxburst);
> > -
> > -			dwc3_writel(dwc->regs, DWC3_GTXTHRCFG, reg);
> > -		}
> > -	}
> > +	dwc3_config_threshold(dwc);
> >   	return 0;
> > @@ -1380,6 +1450,10 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> >   	u8			lpm_nyet_threshold;
> >   	u8			tx_de_emphasis;
> >   	u8			hird_threshold;
> > +	u8			rx_thr_num_pkt = 0;
> > +	u8			rx_max_burst = 0;
> > +	u8			tx_thr_num_pkt = 0;
> > +	u8			tx_max_burst = 0;
> >   	u8			rx_thr_num_pkt_prd = 0;
> >   	u8			rx_max_burst_prd = 0;
> >   	u8			tx_thr_num_pkt_prd = 0;
> > @@ -1442,6 +1516,14 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> >   				"snps,usb2-lpm-disable");
> >   	dwc->usb2_gadget_lpm_disable = device_property_read_bool(dev,
> >   				"snps,usb2-gadget-lpm-disable");
> > +	device_property_read_u8(dev, "snps,rx-thr-num-pkt",
> > +				&rx_thr_num_pkt);
> > +	device_property_read_u8(dev, "snps,rx-max-burst",
> > +				&rx_max_burst);
> > +	device_property_read_u8(dev, "snps,tx-thr-num-pkt",
> > +				&tx_thr_num_pkt);
> > +	device_property_read_u8(dev, "snps,tx-max-burst",
> > +				&tx_max_burst);
> 
> Aren't there value limitations for some of these (if not all) settings?  For
> example, in the DWC3 programming guide, the max burst fields say (for
> DWC31):
> 	"Note: This field can only be set to 2, 4, 8 or 16."
> 
> And for the packet threshold count:
> 	"Valid values are from 1 to 16."
> 

Stanley documented this in the dt binding. I think what he had written
there is fine.

Thanks,
Thinh