| Message ID | 20241107104040.502-1-selvarasu.g@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: gadget: Add TxFIFO resizing supports for single port RAM | expand |
On 11/8/2024 5:04 AM, Thinh Nguyen wrote: > On Thu, Nov 07, 2024, Selvarasu Ganesan wrote: >> This commit adds support for resizing the TxFIFO in USB2.0-only mode >> where using single port RAM, and limit the use of extra FIFOs for bulk > This should be split into 2 changes: 1 for adding support for > single-port RAM, and the other for budgeting the bulk fifo setting. > > The first change is not a fix, and the latter may be a fix (may need > more feedback from others). Hi Thinh, Thanks for reviewing. Sure i will do split into 2 changes. >> transfers in non-SS mode. It prevents the issue of limited RAM size >> usage. >> >> Fixes: fad16c823e66 ("usb: dwc3: gadget: Refine the logic for resizing Tx FIFOs") >> Cc: stable@vger.kernel.org # 6.12.x: fad16c82: usb: dwc3: gadget: Refine the logic for resizing Tx FIFOs >> Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> >> --- >> drivers/usb/dwc3/core.h | 4 +++ >> drivers/usb/dwc3/gadget.c | 56 ++++++++++++++++++++++++++++++--------- >> 2 files changed, 48 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index eaa55c0cf62f..8306b39e5c64 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -915,6 +915,7 @@ struct dwc3_hwparams { >> #define DWC3_MODE(n) ((n) & 0x7) >> >> /* HWPARAMS1 */ >> +#define DWC3_SPRAM_TYPE(n) (((n) >> 23) & 1) >> #define DWC3_NUM_INT(n) (((n) & (0x3f << 15)) >> 15) >> >> /* HWPARAMS3 */ >> @@ -925,6 +926,9 @@ struct dwc3_hwparams { >> #define DWC3_NUM_IN_EPS(p) (((p)->hwparams3 & \ >> (DWC3_NUM_IN_EPS_MASK)) >> 18) >> >> +/* HWPARAMS6 */ >> +#define DWC3_RAM0_DEPTH(n) (((n) & (0xffff0000)) >> 16) >> + >> /* HWPARAMS7 */ >> #define DWC3_RAM1_DEPTH(n) ((n) & 0xffff) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 2fed2aa01407..d3e25f7d7cd0 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -687,6 +687,42 @@ static int dwc3_gadget_calc_tx_fifo_size(struct dwc3 *dwc, int mult) >> return fifo_size; >> } >> >> +/** >> + * dwc3_gadget_calc_ram_depth - calculates the ram depth for txfifo >> + * @dwc: pointer to the DWC3 context >> + */ >> +static int dwc3_gadget_calc_ram_depth(struct dwc3 *dwc) >> +{ >> + int ram_depth; >> + int fifo_0_start; >> + bool spram_type; >> + int tmp; >> + >> + /* Check supporting RAM type by HW */ >> + spram_type = DWC3_SPRAM_TYPE(dwc->hwparams.hwparams1); >> + >> + /* If a single port RAM is utilized, then allocate TxFIFOs from >> + * RAM0. otherwise, allocate them from RAM1. >> + */ > Please use this comment block style > /* > * xxxx > * xxxx > */ Sure, will update it in next version. >> + ram_depth = spram_type ? DWC3_RAM0_DEPTH(dwc->hwparams.hwparams6) : >> + DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); > Don't use spram_type as a boolean. Perhaps define a macro for type value > 1 and 0 (for single vs 2-port) Are you expecting something like below? #define DWC3_SINGLE_PORT_RAM 1 #define DWC3_TW0_PORT_RAM 0 // ... int ram_depth; int fifo_0_start; int spram_type; int tmp; /* * Check supporting RAM type by HW. If a single port RAM * is utilized, then allocate TxFIFOs from RAM0. otherwise, * allocate them from RAM1. */ spram_type = DWC3_SPRAM_TYPE(dwc->hwparams.hwparams1); /* * In a single port RAM configuration, the available RAM is shared * between the RX and TX FIFOs. This means that the txfifo can begin * at a non-zero address. */ if (spram_type == DWC3_SINGLE_PORT_RAM) { ram_depth = DWC3_RAM0_DEPTH(dwc->hwparams.hwparams6); /* Check if TXFIFOs start at non-zero addr */ tmp = dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(0)); fifo_0_start = DWC3_GTXFIFOSIZ_TXFSTADDR(tmp); ram_depth -= (fifo_0_start >> 16); } else ram_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); return ram_depth; >> + >> + >> + /* In a single port RAM configuration, the available RAM is shared >> + * between the RX and TX FIFOs. This means that the txfifo can begin >> + * at a non-zero address. >> + */ >> + if (spram_type) { > if (spram_type == DWC3_SPRAM_TYPE_SINGLE) { > ... > } > >> + /* Check if TXFIFOs start at non-zero addr */ >> + tmp = dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(0)); >> + fifo_0_start = DWC3_GTXFIFOSIZ_TXFSTADDR(tmp); >> + >> + ram_depth -= (fifo_0_start >> 16); >> + } >> + >> + return ram_depth; >> +} >> + >> /** >> * dwc3_gadget_clear_tx_fifos - Clears txfifo allocation >> * @dwc: pointer to the DWC3 context >> @@ -753,7 +789,7 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) >> { >> struct dwc3 *dwc = dep->dwc; >> int fifo_0_start; >> - int ram1_depth; >> + int ram_depth; >> int fifo_size; >> int min_depth; >> int num_in_ep; >> @@ -773,7 +809,7 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) >> if (dep->flags & DWC3_EP_TXFIFO_RESIZED) >> return 0; >> >> - ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); >> + ram_depth = dwc3_gadget_calc_ram_depth(dwc); >> >> switch (dwc->gadget->speed) { >> case USB_SPEED_SUPER_PLUS: >> @@ -792,10 +828,6 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) >> break; >> } >> fallthrough; >> - case USB_SPEED_FULL: >> - if (usb_endpoint_xfer_bulk(dep->endpoint.desc)) >> - num_fifos = 2; >> - break; > Please take out the fallthrough above if you remove this condition. will update it in next version. > >> default: >> break; >> } >> @@ -809,7 +841,7 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) >> >> /* Reserve at least one FIFO for the number of IN EPs */ >> min_depth = num_in_ep * (fifo + 1); >> - remaining = ram1_depth - min_depth - dwc->last_fifo_depth; >> + remaining = ram_depth - min_depth - dwc->last_fifo_depth; >> remaining = max_t(int, 0, remaining); >> /* >> * We've already reserved 1 FIFO per EP, so check what we can fit in >> @@ -835,9 +867,9 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) >> dwc->last_fifo_depth += DWC31_GTXFIFOSIZ_TXFDEP(fifo_size); >> >> /* Check fifo size allocation doesn't exceed available RAM size. */ >> - if (dwc->last_fifo_depth >= ram1_depth) { >> + if (dwc->last_fifo_depth >= ram_depth) { >> dev_err(dwc->dev, "Fifosize(%d) > RAM size(%d) %s depth:%d\n", >> - dwc->last_fifo_depth, ram1_depth, >> + dwc->last_fifo_depth, ram_depth, >> dep->endpoint.name, fifo_size); >> if (DWC3_IP_IS(DWC3)) >> fifo_size = DWC3_GTXFIFOSIZ_TXFDEP(fifo_size); >> @@ -3090,7 +3122,7 @@ static int dwc3_gadget_check_config(struct usb_gadget *g) >> struct dwc3 *dwc = gadget_to_dwc(g); >> struct usb_ep *ep; >> int fifo_size = 0; >> - int ram1_depth; >> + int ram_depth; >> int ep_num = 0; >> >> if (!dwc->do_fifo_resize) >> @@ -3113,8 +3145,8 @@ static int dwc3_gadget_check_config(struct usb_gadget *g) >> fifo_size += dwc->max_cfg_eps; >> >> /* Check if we can fit a single fifo per endpoint */ >> - ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); >> - if (fifo_size > ram1_depth) >> + ram_depth = dwc3_gadget_calc_ram_depth(dwc); >> + if (fifo_size > ram_depth) >> return -ENOMEM; >> >> return 0; >> -- >> 2.17.1 >> > We may need to think a little more on how to budgeting the resource > properly to accomodate for different requirements. If there's no single > formula to satisfy for all platform, perhaps we may need to introduce > parameters that users can set base on the needs of their application. Agree. Need to introduce some parameters to control the required fifos by user that based their usecase. Here's a rephrased version of your proposal: To address the issue of determining the required number of FIFOs for different types of transfers, we propose introducing dynamic FIFO calculation for all type of EP transfers based on the maximum packet size, and remove hard code value for required fifos in driver, Additionally, we suggest introducing DT properties(tx-fifo-max-num-iso, tx-fifo-max-bulk and tx-fifo-max-intr) for all types of transfers (except control EP) to allow users to control the required FIFOs instead of relying solely on the tx-fifo-max-num. This approach will provide more flexibility and customization options for users based on their specific use cases. Please let me know if you have any comments on the above approach. Thanks, Selva > > I'd like to Ack on the new change that checks single-port RAM. For the > budgeting of fifo, let's keep the discussion going a little more. > > Thanks, > Thinh
++ Alan Stern On Fri, Nov 08, 2024, Selvarasu Ganesan wrote: > >> + ram_depth = spram_type ? DWC3_RAM0_DEPTH(dwc->hwparams.hwparams6) : > >> + DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); > > Don't use spram_type as a boolean. Perhaps define a macro for type value > > 1 and 0 (for single vs 2-port) > Are you expecting something like below? > > #define DWC3_SINGLE_PORT_RAM 1 > #define DWC3_TW0_PORT_RAM 0 Yes. I think it's more readable if we name the variable to "ram_type" and use the macros above as I suggested. If you still plan to use it as boolean, please rename the variable spram_type to is_single_port_ram (no one knows what "spram_type" mean without the programming guide or some documention). > < snip > > >> > > We may need to think a little more on how to budgeting the resource > > properly to accomodate for different requirements. If there's no single > > formula to satisfy for all platform, perhaps we may need to introduce > > parameters that users can set base on the needs of their application. > Agree. Need to introduce some parameters to control the required fifos > by user that based their usecase. > Here's a rephrased version of your proposal: > > To address the issue of determining the required number of FIFOs for > different types of transfers, we propose introducing dynamic FIFO > calculation for all type of EP transfers based on the maximum packet > size, and remove hard code value for required fifos in driver, The current fifo calculation already takes on the max packet size into account. For SuperSpeed and above, we can guess how much fifo is needed base on the maxburst and mult settings. However, for bulk endpoint in highspeed, it needs a bit more checking. > Additionally, we suggest introducing DT properties(tx-fifo-max-num-iso, > tx-fifo-max-bulk and tx-fifo-max-intr) for all types of transfers This constraint should be decided from the function driver. We should try to keep this more generic since your gadget may be used as mass storage device instead of UVC where bulk performance is needed more. > (except control EP) to allow users to control the required FIFOs instead > of relying solely on the tx-fifo-max-num. This approach will provide > more flexibility and customization options for users based on their > specific use cases. > > Please let me know if you have any comments on the above approach. > How about this: Implement gadget->ops->match_ep() for dwc3 and update the note in usb_ep_autoconfig() API. If the function driver looks for an endpoint by passing in the descriptor with wMaxPacketSize set to 0, mark the endpoint to used for performance. This is closely related to the usb_ep_autoconfig() behavior where it returns the endpoint's maxpacket_limit if wMaxPacketSize is not provided. We just need to expand this behavior to look for performance endpoint. If the function driver provides the wMaxPacketSize during usb_ep_autoconfig(), then use the minimum required fifo. What do you think? Will this work for you? BR, Thinh
On 11/9/2024 6:35 AM, Thinh Nguyen wrote: > ++ Alan Stern > > On Fri, Nov 08, 2024, Selvarasu Ganesan wrote: > >>>> + ram_depth = spram_type ? DWC3_RAM0_DEPTH(dwc->hwparams.hwparams6) : >>>> + DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); >>> Don't use spram_type as a boolean. Perhaps define a macro for type value >>> 1 and 0 (for single vs 2-port) >> Are you expecting something like below? >> >> #define DWC3_SINGLE_PORT_RAM 1 >> #define DWC3_TW0_PORT_RAM 0 > Yes. I think it's more readable if we name the variable to "ram_type" > and use the macros above as I suggested. > > If you still plan to use it as boolean, please rename the variable > spram_type to is_single_port_ram (no one knows what "spram_type" mean > without the programming guide or some documention). We are fine to use variable name as a is_single_port_ram with boolean. Please find the below updated new patch for your review. https://lore.kernel.org/linux-usb/20241111142049.604-1-selvarasu.g@samsung.com/. > < snip > > >>> We may need to think a little more on how to budgeting the resource >>> properly to accomodate for different requirements. If there's no single >>> formula to satisfy for all platform, perhaps we may need to introduce >>> parameters that users can set base on the needs of their application. >> Agree. Need to introduce some parameters to control the required fifos >> by user that based their usecase. >> Here's a rephrased version of your proposal: >> >> To address the issue of determining the required number of FIFOs for >> different types of transfers, we propose introducing dynamic FIFO >> calculation for all type of EP transfers based on the maximum packet >> size, and remove hard code value for required fifos in driver, > The current fifo calculation already takes on the max packet size into > account. > > For SuperSpeed and above, we can guess how much fifo is needed base on > the maxburst and mult settings. However, for bulk endpoint in highspeed, > it needs a bit more checking. Agree. > >> Additionally, we suggest introducing DT properties(tx-fifo-max-num-iso, >> tx-fifo-max-bulk and tx-fifo-max-intr) for all types of transfers > This constraint should be decided from the function driver. We should > try to keep this more generic since your gadget may be used as mass > storage device instead of UVC where bulk performance is needed more. Agree. > >> (except control EP) to allow users to control the required FIFOs instead >> of relying solely on the tx-fifo-max-num. This approach will provide >> more flexibility and customization options for users based on their >> specific use cases. >> >> Please let me know if you have any comments on the above approach. >> > How about this: Implement gadget->ops->match_ep() for dwc3 and update > the note in usb_ep_autoconfig() API. > > If the function driver looks for an endpoint by passing in the > descriptor with wMaxPacketSize set to 0, mark the endpoint to used for > performance. This is closely related to the usb_ep_autoconfig() behavior > where it returns the endpoint's maxpacket_limit if wMaxPacketSize is not > provided. We just need to expand this behavior to look for performance > endpoint. > > If the function driver provides the wMaxPacketSize during > usb_ep_autoconfig(), then use the minimum required fifo. > > What do you think? Will this work for you? Hi Thinh, Thank you for the suggestions. This method makes more sense to us, and we are fine proceeding with it. As we previously discussed, we plan to create a separate patch for allocating resources for bulk EP. However, it may take some time on our end as we need to perform additional testing with all possible scenarios. In the meantime, please review and help to merge the patch for the single port RAM FIFO resizing logic. Thanks, Selva > > BR, > Thinh
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index eaa55c0cf62f..8306b39e5c64 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -915,6 +915,7 @@ struct dwc3_hwparams { #define DWC3_MODE(n) ((n) & 0x7) /* HWPARAMS1 */ +#define DWC3_SPRAM_TYPE(n) (((n) >> 23) & 1) #define DWC3_NUM_INT(n) (((n) & (0x3f << 15)) >> 15) /* HWPARAMS3 */ @@ -925,6 +926,9 @@ struct dwc3_hwparams { #define DWC3_NUM_IN_EPS(p) (((p)->hwparams3 & \ (DWC3_NUM_IN_EPS_MASK)) >> 18) +/* HWPARAMS6 */ +#define DWC3_RAM0_DEPTH(n) (((n) & (0xffff0000)) >> 16) + /* HWPARAMS7 */ #define DWC3_RAM1_DEPTH(n) ((n) & 0xffff) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 2fed2aa01407..d3e25f7d7cd0 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -687,6 +687,42 @@ static int dwc3_gadget_calc_tx_fifo_size(struct dwc3 *dwc, int mult) return fifo_size; } +/** + * dwc3_gadget_calc_ram_depth - calculates the ram depth for txfifo + * @dwc: pointer to the DWC3 context + */ +static int dwc3_gadget_calc_ram_depth(struct dwc3 *dwc) +{ + int ram_depth; + int fifo_0_start; + bool spram_type; + int tmp; + + /* Check supporting RAM type by HW */ + spram_type = DWC3_SPRAM_TYPE(dwc->hwparams.hwparams1); + + /* If a single port RAM is utilized, then allocate TxFIFOs from + * RAM0. otherwise, allocate them from RAM1. + */ + ram_depth = spram_type ? DWC3_RAM0_DEPTH(dwc->hwparams.hwparams6) : + DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); + + + /* In a single port RAM configuration, the available RAM is shared + * between the RX and TX FIFOs. This means that the txfifo can begin + * at a non-zero address. + */ + if (spram_type) { + /* Check if TXFIFOs start at non-zero addr */ + tmp = dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(0)); + fifo_0_start = DWC3_GTXFIFOSIZ_TXFSTADDR(tmp); + + ram_depth -= (fifo_0_start >> 16); + } + + return ram_depth; +} + /** * dwc3_gadget_clear_tx_fifos - Clears txfifo allocation * @dwc: pointer to the DWC3 context @@ -753,7 +789,7 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) { struct dwc3 *dwc = dep->dwc; int fifo_0_start; - int ram1_depth; + int ram_depth; int fifo_size; int min_depth; int num_in_ep; @@ -773,7 +809,7 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) if (dep->flags & DWC3_EP_TXFIFO_RESIZED) return 0; - ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); + ram_depth = dwc3_gadget_calc_ram_depth(dwc); switch (dwc->gadget->speed) { case USB_SPEED_SUPER_PLUS: @@ -792,10 +828,6 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) break; } fallthrough; - case USB_SPEED_FULL: - if (usb_endpoint_xfer_bulk(dep->endpoint.desc)) - num_fifos = 2; - break; default: break; } @@ -809,7 +841,7 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) /* Reserve at least one FIFO for the number of IN EPs */ min_depth = num_in_ep * (fifo + 1); - remaining = ram1_depth - min_depth - dwc->last_fifo_depth; + remaining = ram_depth - min_depth - dwc->last_fifo_depth; remaining = max_t(int, 0, remaining); /* * We've already reserved 1 FIFO per EP, so check what we can fit in @@ -835,9 +867,9 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) dwc->last_fifo_depth += DWC31_GTXFIFOSIZ_TXFDEP(fifo_size); /* Check fifo size allocation doesn't exceed available RAM size. */ - if (dwc->last_fifo_depth >= ram1_depth) { + if (dwc->last_fifo_depth >= ram_depth) { dev_err(dwc->dev, "Fifosize(%d) > RAM size(%d) %s depth:%d\n", - dwc->last_fifo_depth, ram1_depth, + dwc->last_fifo_depth, ram_depth, dep->endpoint.name, fifo_size); if (DWC3_IP_IS(DWC3)) fifo_size = DWC3_GTXFIFOSIZ_TXFDEP(fifo_size); @@ -3090,7 +3122,7 @@ static int dwc3_gadget_check_config(struct usb_gadget *g) struct dwc3 *dwc = gadget_to_dwc(g); struct usb_ep *ep; int fifo_size = 0; - int ram1_depth; + int ram_depth; int ep_num = 0; if (!dwc->do_fifo_resize) @@ -3113,8 +3145,8 @@ static int dwc3_gadget_check_config(struct usb_gadget *g) fifo_size += dwc->max_cfg_eps; /* Check if we can fit a single fifo per endpoint */ - ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); - if (fifo_size > ram1_depth) + ram_depth = dwc3_gadget_calc_ram_depth(dwc); + if (fifo_size > ram_depth) return -ENOMEM; return 0;
This commit adds support for resizing the TxFIFO in USB2.0-only mode where using single port RAM, and limit the use of extra FIFOs for bulk transfers in non-SS mode. It prevents the issue of limited RAM size usage. Fixes: fad16c823e66 ("usb: dwc3: gadget: Refine the logic for resizing Tx FIFOs") Cc: stable@vger.kernel.org # 6.12.x: fad16c82: usb: dwc3: gadget: Refine the logic for resizing Tx FIFOs Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com> --- drivers/usb/dwc3/core.h | 4 +++ drivers/usb/dwc3/gadget.c | 56 ++++++++++++++++++++++++++++++--------- 2 files changed, 48 insertions(+), 12 deletions(-)