| Message ID | 20241016111904.11375-1-quic_akakum@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v6] usb: dwc3: gadget: Refine the logic for resizing Tx FIFOs | expand |
On Wed, Oct 16, 2024, Akash Kumar wrote: > The current logic is rigid, setting num_fifos to fixed values. > 3 for any maxburst greater than 1. > tx_fifo_resize_max_num for maxburst greater than 6. > Additionally, it did not differentiate much between bulk and > isochronous transfers, applying similar logic to both. > > The updated logic is more flexible and specifically designed to meet > the unique requirements of both bulk and isochronous transfers. We > have made every effort to satisfy all needs and requirements, verified > on our specific platform and application. > > Bulk Transfers: Ensures that num_fifos is optimized by considering both > the maxburst and DT property "tx-fifo-max-num" for super speed and > above. For high-speed and below bulk endpoints, a 2K TxFIFO allocation > is used to meet efficient data transfer needs, considering > FIFO-constrained platforms. > > Isochronous Transfers: Ensures that num_fifos is sufficient by > considering the maximum packet multiplier for HS and below and maxburst > for Super-speed and above eps, along with a constraint with the DT > property "tx-fifo-max-num". > > This change aims to optimize the allocation of Tx FIFOs for both bulk > and isochronous endpoints, potentially improving data transfer efficiency > and overall performance. It also enhances support for all use cases, > which can be tweaked with DT parameters and the endpoint’s maxburst and > maxpacket. This structured approach ensures that the appropriate number > of FIFOs is allocated based on the endpoint type and USB speed. > > Signed-off-by: Akash Kumar <quic_akakum@quicinc.com> > --- > Changes for v6: > The code has been refactored to replace multiple if checks with a > switch-case structure based on the USB speed. This change improves > readability and maintainability by clearly defining behavior for > different USB speeds. This structured approach ensures that the > appropriate number of FIFOs is allocated based on the endpoint type > and USB speed. > > Changes for v5: > Update Calculation for HS and below bulk and isoc eps based on > suggestion and fixed at 2k for bulk eps considering fifo constrained > platforms. > > Changes for v4: > Updated commit message as per review comments to clarify that it has > been tested on specific platforms only and tried best to match all > expectations. > > Changes for v3: > Redefine logic for resizing tx fifos,added check based on operating > speed and used maxp for HS and maxburst for SS and defined max > allocation based on dt property. > > Changes for v2: > Redefine logic for resizing tx fifos, handled fifo based on minimum > of maxp and maxburts. > > Changes for v1: > Added additional condition to allocate tx fifo for hs isoc eps, > keeping the other resize logic same > --- > drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 10178e5eda5a..dc62d0626e53 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -771,15 +771,30 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) > > ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); > > - if ((dep->endpoint.maxburst > 1 && > - usb_endpoint_xfer_bulk(dep->endpoint.desc)) || > + switch (dwc->gadget->speed) { > + case USB_SPEED_SUPER_PLUS: > + case USB_SPEED_SUPER: Can you fix the indentations? Should be something like this: switch (speed) { case SSP: case SS: if (...) xxx; break; case HS: if (...) ...; default: break; } Thanks, Thinh > + if (usb_endpoint_xfer_bulk(dep->endpoint.desc) || > usb_endpoint_xfer_isoc(dep->endpoint.desc)) > - num_fifos = 3; > - > - if (dep->endpoint.maxburst > 6 && > - (usb_endpoint_xfer_bulk(dep->endpoint.desc) || > - usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) > - num_fifos = dwc->tx_fifo_resize_max_num; > + num_fifos = min_t(unsigned int, > + dep->endpoint.maxburst, > + dwc->tx_fifo_resize_max_num); > + break; > + case USB_SPEED_HIGH: > + if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) { > + num_fifos = min_t(unsigned int, > + usb_endpoint_maxp_mult(dep->endpoint.desc) + 1, > + dwc->tx_fifo_resize_max_num); > + break; > + } > + fallthrough; > + case USB_SPEED_FULL: > + if (usb_endpoint_xfer_bulk(dep->endpoint.desc)) > + num_fifos = 2; > + break; > + default: > + break; > + } > > /* FIFO size for a single buffer */ > fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); > -- > 2.17.1 >
Hi Akash, kernel test robot noticed the following build warnings: [auto build test WARNING on usb/usb-testing] [also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.12-rc3 next-20241016] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Akash-Kumar/usb-dwc3-gadget-Refine-the-logic-for-resizing-Tx-FIFOs/20241016-192104 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing patch link: https://lore.kernel.org/r/20241016111904.11375-1-quic_akakum%40quicinc.com patch subject: [PATCH v6] usb: dwc3: gadget: Refine the logic for resizing Tx FIFOs config: i386-buildonly-randconfig-001-20241017 (https://download.01.org/0day-ci/archive/20241017/202410171133.eGVTBDtP-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241017/202410171133.eGVTBDtP-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202410171133.eGVTBDtP-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/usb/dwc3/gadget.c: In function 'dwc3_gadget_resize_tx_fifos': >> drivers/usb/dwc3/gadget.c:777:9: warning: this 'if' clause does not guard... [-Wmisleading-indentation] 777 | if (usb_endpoint_xfer_bulk(dep->endpoint.desc) || | ^~ drivers/usb/dwc3/gadget.c:782:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if' 782 | break; | ^~~~~ drivers/usb/dwc3/gadget.c:792:9: warning: this 'if' clause does not guard... [-Wmisleading-indentation] 792 | if (usb_endpoint_xfer_bulk(dep->endpoint.desc)) | ^~ drivers/usb/dwc3/gadget.c:794:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if' 794 | break; | ^~~~~ vim +/if +777 drivers/usb/dwc3/gadget.c 726 727 /* 728 * dwc3_gadget_resize_tx_fifos - reallocate fifo spaces for current use-case 729 * @dwc: pointer to our context structure 730 * 731 * This function will a best effort FIFO allocation in order 732 * to improve FIFO usage and throughput, while still allowing 733 * us to enable as many endpoints as possible. 734 * 735 * Keep in mind that this operation will be highly dependent 736 * on the configured size for RAM1 - which contains TxFifo -, 737 * the amount of endpoints enabled on coreConsultant tool, and 738 * the width of the Master Bus. 739 * 740 * In general, FIFO depths are represented with the following equation: 741 * 742 * fifo_size = mult * ((max_packet + mdwidth)/mdwidth + 1) + 1 743 * 744 * In conjunction with dwc3_gadget_check_config(), this resizing logic will 745 * ensure that all endpoints will have enough internal memory for one max 746 * packet per endpoint. 747 */ 748 static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) 749 { 750 struct dwc3 *dwc = dep->dwc; 751 int fifo_0_start; 752 int ram1_depth; 753 int fifo_size; 754 int min_depth; 755 int num_in_ep; 756 int remaining; 757 int num_fifos = 1; 758 int fifo; 759 int tmp; 760 761 if (!dwc->do_fifo_resize) 762 return 0; 763 764 /* resize IN endpoints except ep0 */ 765 if (!usb_endpoint_dir_in(dep->endpoint.desc) || dep->number <= 1) 766 return 0; 767 768 /* bail if already resized */ 769 if (dep->flags & DWC3_EP_TXFIFO_RESIZED) 770 return 0; 771 772 ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); 773 774 switch (dwc->gadget->speed) { 775 case USB_SPEED_SUPER_PLUS: 776 case USB_SPEED_SUPER: > 777 if (usb_endpoint_xfer_bulk(dep->endpoint.desc) || 778 usb_endpoint_xfer_isoc(dep->endpoint.desc)) 779 num_fifos = min_t(unsigned int, 780 dep->endpoint.maxburst, 781 dwc->tx_fifo_resize_max_num); 782 break; 783 case USB_SPEED_HIGH: 784 if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) { 785 num_fifos = min_t(unsigned int, 786 usb_endpoint_maxp_mult(dep->endpoint.desc) + 1, 787 dwc->tx_fifo_resize_max_num); 788 break; 789 } 790 fallthrough; 791 case USB_SPEED_FULL: 792 if (usb_endpoint_xfer_bulk(dep->endpoint.desc)) 793 num_fifos = 2; 794 break; 795 default: 796 break; 797 } 798 799 /* FIFO size for a single buffer */ 800 fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); 801 802 /* Calculate the number of remaining EPs w/o any FIFO */ 803 num_in_ep = dwc->max_cfg_eps; 804 num_in_ep -= dwc->num_ep_resized; 805 806 /* Reserve at least one FIFO for the number of IN EPs */ 807 min_depth = num_in_ep * (fifo + 1); 808 remaining = ram1_depth - min_depth - dwc->last_fifo_depth; 809 remaining = max_t(int, 0, remaining); 810 /* 811 * We've already reserved 1 FIFO per EP, so check what we can fit in 812 * addition to it. If there is not enough remaining space, allocate 813 * all the remaining space to the EP. 814 */ 815 fifo_size = (num_fifos - 1) * fifo; 816 if (remaining < fifo_size) 817 fifo_size = remaining; 818 819 fifo_size += fifo; 820 /* Last increment according to the TX FIFO size equation */ 821 fifo_size++; 822 823 /* Check if TXFIFOs start at non-zero addr */ 824 tmp = dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(0)); 825 fifo_0_start = DWC3_GTXFIFOSIZ_TXFSTADDR(tmp); 826 827 fifo_size |= (fifo_0_start + (dwc->last_fifo_depth << 16)); 828 if (DWC3_IP_IS(DWC3)) 829 dwc->last_fifo_depth += DWC3_GTXFIFOSIZ_TXFDEP(fifo_size); 830 else 831 dwc->last_fifo_depth += DWC31_GTXFIFOSIZ_TXFDEP(fifo_size); 832 833 /* Check fifo size allocation doesn't exceed available RAM size. */ 834 if (dwc->last_fifo_depth >= ram1_depth) { 835 dev_err(dwc->dev, "Fifosize(%d) > RAM size(%d) %s depth:%d\n", 836 dwc->last_fifo_depth, ram1_depth, 837 dep->endpoint.name, fifo_size); 838 if (DWC3_IP_IS(DWC3)) 839 fifo_size = DWC3_GTXFIFOSIZ_TXFDEP(fifo_size); 840 else 841 fifo_size = DWC31_GTXFIFOSIZ_TXFDEP(fifo_size); 842 843 dwc->last_fifo_depth -= fifo_size; 844 return -ENOMEM; 845 } 846 847 dwc3_writel(dwc->regs, DWC3_GTXFIFOSIZ(dep->number >> 1), fifo_size); 848 dep->flags |= DWC3_EP_TXFIFO_RESIZED; 849 dwc->num_ep_resized++; 850 851 return 0; 852 } 853
Hi Akash, kernel test robot noticed the following build warnings: [auto build test WARNING on usb/usb-testing] [also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.12-rc3 next-20241016] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Akash-Kumar/usb-dwc3-gadget-Refine-the-logic-for-resizing-Tx-FIFOs/20241016-192104 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing patch link: https://lore.kernel.org/r/20241016111904.11375-1-quic_akakum%40quicinc.com patch subject: [PATCH v6] usb: dwc3: gadget: Refine the logic for resizing Tx FIFOs config: i386-buildonly-randconfig-003-20241017 (https://download.01.org/0day-ci/archive/20241017/202410171121.i9xLy4CF-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241017/202410171121.i9xLy4CF-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202410171121.i9xLy4CF-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/usb/dwc3/gadget.c:782:3: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation] 782 | break; | ^ drivers/usb/dwc3/gadget.c:777:2: note: previous statement is here 777 | if (usb_endpoint_xfer_bulk(dep->endpoint.desc) || | ^ drivers/usb/dwc3/gadget.c:794:3: warning: misleading indentation; statement is not part of the previous 'if' [-Wmisleading-indentation] 794 | break; | ^ drivers/usb/dwc3/gadget.c:792:2: note: previous statement is here 792 | if (usb_endpoint_xfer_bulk(dep->endpoint.desc)) | ^ 2 warnings generated. vim +/if +782 drivers/usb/dwc3/gadget.c 726 727 /* 728 * dwc3_gadget_resize_tx_fifos - reallocate fifo spaces for current use-case 729 * @dwc: pointer to our context structure 730 * 731 * This function will a best effort FIFO allocation in order 732 * to improve FIFO usage and throughput, while still allowing 733 * us to enable as many endpoints as possible. 734 * 735 * Keep in mind that this operation will be highly dependent 736 * on the configured size for RAM1 - which contains TxFifo -, 737 * the amount of endpoints enabled on coreConsultant tool, and 738 * the width of the Master Bus. 739 * 740 * In general, FIFO depths are represented with the following equation: 741 * 742 * fifo_size = mult * ((max_packet + mdwidth)/mdwidth + 1) + 1 743 * 744 * In conjunction with dwc3_gadget_check_config(), this resizing logic will 745 * ensure that all endpoints will have enough internal memory for one max 746 * packet per endpoint. 747 */ 748 static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) 749 { 750 struct dwc3 *dwc = dep->dwc; 751 int fifo_0_start; 752 int ram1_depth; 753 int fifo_size; 754 int min_depth; 755 int num_in_ep; 756 int remaining; 757 int num_fifos = 1; 758 int fifo; 759 int tmp; 760 761 if (!dwc->do_fifo_resize) 762 return 0; 763 764 /* resize IN endpoints except ep0 */ 765 if (!usb_endpoint_dir_in(dep->endpoint.desc) || dep->number <= 1) 766 return 0; 767 768 /* bail if already resized */ 769 if (dep->flags & DWC3_EP_TXFIFO_RESIZED) 770 return 0; 771 772 ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); 773 774 switch (dwc->gadget->speed) { 775 case USB_SPEED_SUPER_PLUS: 776 case USB_SPEED_SUPER: 777 if (usb_endpoint_xfer_bulk(dep->endpoint.desc) || 778 usb_endpoint_xfer_isoc(dep->endpoint.desc)) 779 num_fifos = min_t(unsigned int, 780 dep->endpoint.maxburst, 781 dwc->tx_fifo_resize_max_num); > 782 break; 783 case USB_SPEED_HIGH: 784 if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) { 785 num_fifos = min_t(unsigned int, 786 usb_endpoint_maxp_mult(dep->endpoint.desc) + 1, 787 dwc->tx_fifo_resize_max_num); 788 break; 789 } 790 fallthrough; 791 case USB_SPEED_FULL: 792 if (usb_endpoint_xfer_bulk(dep->endpoint.desc)) 793 num_fifos = 2; 794 break; 795 default: 796 break; 797 } 798 799 /* FIFO size for a single buffer */ 800 fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); 801 802 /* Calculate the number of remaining EPs w/o any FIFO */ 803 num_in_ep = dwc->max_cfg_eps; 804 num_in_ep -= dwc->num_ep_resized; 805 806 /* Reserve at least one FIFO for the number of IN EPs */ 807 min_depth = num_in_ep * (fifo + 1); 808 remaining = ram1_depth - min_depth - dwc->last_fifo_depth; 809 remaining = max_t(int, 0, remaining); 810 /* 811 * We've already reserved 1 FIFO per EP, so check what we can fit in 812 * addition to it. If there is not enough remaining space, allocate 813 * all the remaining space to the EP. 814 */ 815 fifo_size = (num_fifos - 1) * fifo; 816 if (remaining < fifo_size) 817 fifo_size = remaining; 818 819 fifo_size += fifo; 820 /* Last increment according to the TX FIFO size equation */ 821 fifo_size++; 822 823 /* Check if TXFIFOs start at non-zero addr */ 824 tmp = dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(0)); 825 fifo_0_start = DWC3_GTXFIFOSIZ_TXFSTADDR(tmp); 826 827 fifo_size |= (fifo_0_start + (dwc->last_fifo_depth << 16)); 828 if (DWC3_IP_IS(DWC3)) 829 dwc->last_fifo_depth += DWC3_GTXFIFOSIZ_TXFDEP(fifo_size); 830 else 831 dwc->last_fifo_depth += DWC31_GTXFIFOSIZ_TXFDEP(fifo_size); 832 833 /* Check fifo size allocation doesn't exceed available RAM size. */ 834 if (dwc->last_fifo_depth >= ram1_depth) { 835 dev_err(dwc->dev, "Fifosize(%d) > RAM size(%d) %s depth:%d\n", 836 dwc->last_fifo_depth, ram1_depth, 837 dep->endpoint.name, fifo_size); 838 if (DWC3_IP_IS(DWC3)) 839 fifo_size = DWC3_GTXFIFOSIZ_TXFDEP(fifo_size); 840 else 841 fifo_size = DWC31_GTXFIFOSIZ_TXFDEP(fifo_size); 842 843 dwc->last_fifo_depth -= fifo_size; 844 return -ENOMEM; 845 } 846 847 dwc3_writel(dwc->regs, DWC3_GTXFIFOSIZ(dep->number >> 1), fifo_size); 848 dep->flags |= DWC3_EP_TXFIFO_RESIZED; 849 dwc->num_ep_resized++; 850 851 return 0; 852 } 853
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 10178e5eda5a..dc62d0626e53 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -771,15 +771,30 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); - if ((dep->endpoint.maxburst > 1 && - usb_endpoint_xfer_bulk(dep->endpoint.desc)) || + switch (dwc->gadget->speed) { + case USB_SPEED_SUPER_PLUS: + case USB_SPEED_SUPER: + if (usb_endpoint_xfer_bulk(dep->endpoint.desc) || usb_endpoint_xfer_isoc(dep->endpoint.desc)) - num_fifos = 3; - - if (dep->endpoint.maxburst > 6 && - (usb_endpoint_xfer_bulk(dep->endpoint.desc) || - usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) - num_fifos = dwc->tx_fifo_resize_max_num; + num_fifos = min_t(unsigned int, + dep->endpoint.maxburst, + dwc->tx_fifo_resize_max_num); + break; + case USB_SPEED_HIGH: + if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) { + num_fifos = min_t(unsigned int, + usb_endpoint_maxp_mult(dep->endpoint.desc) + 1, + dwc->tx_fifo_resize_max_num); + break; + } + fallthrough; + case USB_SPEED_FULL: + if (usb_endpoint_xfer_bulk(dep->endpoint.desc)) + num_fifos = 2; + break; + default: + break; + } /* FIFO size for a single buffer */ fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1);
The current logic is rigid, setting num_fifos to fixed values. 3 for any maxburst greater than 1. tx_fifo_resize_max_num for maxburst greater than 6. Additionally, it did not differentiate much between bulk and isochronous transfers, applying similar logic to both. The updated logic is more flexible and specifically designed to meet the unique requirements of both bulk and isochronous transfers. We have made every effort to satisfy all needs and requirements, verified on our specific platform and application. Bulk Transfers: Ensures that num_fifos is optimized by considering both the maxburst and DT property "tx-fifo-max-num" for super speed and above. For high-speed and below bulk endpoints, a 2K TxFIFO allocation is used to meet efficient data transfer needs, considering FIFO-constrained platforms. Isochronous Transfers: Ensures that num_fifos is sufficient by considering the maximum packet multiplier for HS and below and maxburst for Super-speed and above eps, along with a constraint with the DT property "tx-fifo-max-num". This change aims to optimize the allocation of Tx FIFOs for both bulk and isochronous endpoints, potentially improving data transfer efficiency and overall performance. It also enhances support for all use cases, which can be tweaked with DT parameters and the endpoint’s maxburst and maxpacket. This structured approach ensures that the appropriate number of FIFOs is allocated based on the endpoint type and USB speed. Signed-off-by: Akash Kumar <quic_akakum@quicinc.com> --- Changes for v6: The code has been refactored to replace multiple if checks with a switch-case structure based on the USB speed. This change improves readability and maintainability by clearly defining behavior for different USB speeds. This structured approach ensures that the appropriate number of FIFOs is allocated based on the endpoint type and USB speed. Changes for v5: Update Calculation for HS and below bulk and isoc eps based on suggestion and fixed at 2k for bulk eps considering fifo constrained platforms. Changes for v4: Updated commit message as per review comments to clarify that it has been tested on specific platforms only and tried best to match all expectations. Changes for v3: Redefine logic for resizing tx fifos,added check based on operating speed and used maxp for HS and maxburst for SS and defined max allocation based on dt property. Changes for v2: Redefine logic for resizing tx fifos, handled fifo based on minimum of maxp and maxburts. Changes for v1: Added additional condition to allocate tx fifo for hs isoc eps, keeping the other resize logic same --- drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)