* [PATCH v8 1/1] usb: cdns3: allocate TX FIFO size according to composite EP number
@ 2022-04-27 16:35 Frank Li
2022-04-27 16:57 ` Greg KH
2022-05-05 2:21 ` Peter Chen
0 siblings, 2 replies; 7+ messages in thread
From: Frank Li @ 2022-04-27 16:35 UTC (permalink / raw)
To: peter.chen, pawell, rogerq, a-govindraju, linux-usb, jun.li, lznuaa
Cc: gregkh, linux-kernel
Some devices have USB compositions which may require multiple endpoints.
To get better performance, need bigger CDNS3_EP_BUF_SIZE.
But bigger CDNS3_EP_BUF_SIZE may exceed total hardware FIFO size when
multiple endpoints.
By introducing the check_config() callback, calculate CDNS3_EP_BUF_SIZE.
Move CDNS3_EP_BUF_SIZE into cnds3_device: ep_buf_size
Combine CDNS3_EP_ISO_SS_BURST and CDNS3_EP_ISO_HS_MULT into
ecnds3_device:ep_iso_burst
Using a simple algorithm to calculate ep_buf_size.
ep_buf_size = ep_iso_burst = (onchip_buffers - 2k) / (number of IN EP +
1).
Test at 8qxp:
Gadget ep_buf_size
RNDIS: 5
RNDIS+ACM: 3
Mass Storage + NCM + ACM 2
Previous CDNS3_EP_BUF_SIZE is 4, RNDIS + ACM will be failure because
exceed FIFO memory.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v1 to v2:
Add safe check for mult, buffering and maxburst
drivers/usb/cdns3/cdns3-gadget.c | 50 +++++++++++++++++++++++++++++---
drivers/usb/cdns3/cdns3-gadget.h | 9 ++++--
2 files changed, 52 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
index 1f3b4a1422126..e25e7a2b55862 100644
--- a/drivers/usb/cdns3/cdns3-gadget.c
+++ b/drivers/usb/cdns3/cdns3-gadget.c
@@ -2050,7 +2050,7 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
u8 mult = 0;
int ret;
- buffering = CDNS3_EP_BUF_SIZE - 1;
+ buffering = priv_dev->ep_buf_size - 1;
cdns3_configure_dmult(priv_dev, priv_ep);
@@ -2069,7 +2069,7 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
break;
default:
ep_cfg = EP_CFG_EPTYPE(USB_ENDPOINT_XFER_ISOC);
- mult = CDNS3_EP_ISO_HS_MULT - 1;
+ mult = priv_dev->ep_iso_burst - 1;
buffering = mult + 1;
}
@@ -2085,14 +2085,14 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
mult = 0;
max_packet_size = 1024;
if (priv_ep->type == USB_ENDPOINT_XFER_ISOC) {
- maxburst = CDNS3_EP_ISO_SS_BURST - 1;
+ maxburst = priv_dev->ep_iso_burst - 1;
buffering = (mult + 1) *
(maxburst + 1);
if (priv_ep->interval > 1)
buffering++;
} else {
- maxburst = CDNS3_EP_BUF_SIZE - 1;
+ maxburst = priv_dev->ep_buf_size - 1;
}
break;
default:
@@ -2136,6 +2136,10 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
ep_cfg |= EP_CFG_STREAM_EN | EP_CFG_TDL_CHK | EP_CFG_SID_CHK;
}
+ mult = min_t(u8, mult, EP_CFG_MULT_MAX);
+ buffering = min_t(u8, buffering, EP_CFG_BUFFERING_MAX);
+ maxburst = min_t(u8, maxburst, EP_CFG_MAXBURST_MAX);
+
ep_cfg |= EP_CFG_MAXPKTSIZE(max_packet_size) |
EP_CFG_MULT(mult) |
EP_CFG_BUFFERING(buffering) |
@@ -2970,6 +2974,43 @@ static int cdns3_gadget_udc_stop(struct usb_gadget *gadget)
return 0;
}
+/**
+ * cdns3_gadget_check_config - ensure cdns3 can support the USB configuration
+ * @gadget: pointer to the USB gadget
+ *
+ * Used to record the maximum number of endpoints being used in a USB composite
+ * device. (across all configurations) This is to be used in the calculation
+ * of the TXFIFO sizes when resizing internal memory for individual endpoints.
+ * It will help ensured that the resizing logic reserves enough space for at
+ * least one max packet.
+ */
+static int cdns3_gadget_check_config(struct usb_gadget *gadget)
+{
+ struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget);
+ struct usb_ep *ep;
+ int n_in = 0;
+ int total;
+
+ list_for_each_entry(ep, &gadget->ep_list, ep_list) {
+ if (ep->claimed && (ep->address & USB_DIR_IN))
+ n_in++;
+ }
+
+ priv_dev->ep_buf_size = 1;
+ priv_dev->ep_iso_burst = 1;
+
+ /* 2KB are reserved for EP0, 1KB for out*/
+ total = 2 + n_in + 1;
+
+ if (total > priv_dev->onchip_buffers)
+ return -ENOMEM;
+
+ priv_dev->ep_buf_size = priv_dev->ep_iso_burst =
+ (priv_dev->onchip_buffers - 2) / (n_in + 1);
+
+ return 0;
+}
+
static const struct usb_gadget_ops cdns3_gadget_ops = {
.get_frame = cdns3_gadget_get_frame,
.wakeup = cdns3_gadget_wakeup,
@@ -2978,6 +3019,7 @@ static const struct usb_gadget_ops cdns3_gadget_ops = {
.udc_start = cdns3_gadget_udc_start,
.udc_stop = cdns3_gadget_udc_stop,
.match_ep = cdns3_gadget_match_ep,
+ .check_config = cdns3_gadget_check_config,
};
static void cdns3_free_all_eps(struct cdns3_device *priv_dev)
diff --git a/drivers/usb/cdns3/cdns3-gadget.h b/drivers/usb/cdns3/cdns3-gadget.h
index c5660f2c4293f..fbe4a8e3aa897 100644
--- a/drivers/usb/cdns3/cdns3-gadget.h
+++ b/drivers/usb/cdns3/cdns3-gadget.h
@@ -562,15 +562,18 @@ struct cdns3_usb_regs {
/* Max burst size (used only in SS mode). */
#define EP_CFG_MAXBURST_MASK GENMASK(11, 8)
#define EP_CFG_MAXBURST(p) (((p) << 8) & EP_CFG_MAXBURST_MASK)
+#define EP_CFG_MAXBURST_MAX 15
/* ISO max burst. */
#define EP_CFG_MULT_MASK GENMASK(15, 14)
#define EP_CFG_MULT(p) (((p) << 14) & EP_CFG_MULT_MASK)
+#define EP_CFG_MULT_MAX 2
/* ISO max burst. */
#define EP_CFG_MAXPKTSIZE_MASK GENMASK(26, 16)
#define EP_CFG_MAXPKTSIZE(p) (((p) << 16) & EP_CFG_MAXPKTSIZE_MASK)
/* Max number of buffered packets. */
#define EP_CFG_BUFFERING_MASK GENMASK(31, 27)
#define EP_CFG_BUFFERING(p) (((p) << 27) & EP_CFG_BUFFERING_MASK)
+#define EP_CFG_BUFFERING_MAX 15
/* EP_CMD - bitmasks */
/* Endpoint reset. */
@@ -1094,9 +1097,6 @@ struct cdns3_trb {
#define CDNS3_ENDPOINTS_MAX_COUNT 32
#define CDNS3_EP_ZLP_BUF_SIZE 1024
-#define CDNS3_EP_BUF_SIZE 4 /* KB */
-#define CDNS3_EP_ISO_HS_MULT 3
-#define CDNS3_EP_ISO_SS_BURST 3
#define CDNS3_MAX_NUM_DESCMISS_BUF 32
#define CDNS3_DESCMIS_BUF_SIZE 2048 /* Bytes */
#define CDNS3_WA2_NUM_BUFFERS 128
@@ -1333,6 +1333,9 @@ struct cdns3_device {
/*in KB */
u16 onchip_buffers;
u16 onchip_used_size;
+
+ u16 ep_buf_size;
+ u16 ep_iso_burst;
};
void cdns3_set_register_bit(void __iomem *ptr, u32 mask);
--
2.35.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v8 1/1] usb: cdns3: allocate TX FIFO size according to composite EP number
2022-04-27 16:35 [PATCH v8 1/1] usb: cdns3: allocate TX FIFO size according to composite EP number Frank Li
@ 2022-04-27 16:57 ` Greg KH
2022-04-27 17:03 ` Zhi Li
2022-05-05 2:21 ` Peter Chen
1 sibling, 1 reply; 7+ messages in thread
From: Greg KH @ 2022-04-27 16:57 UTC (permalink / raw)
To: Frank Li
Cc: peter.chen, pawell, rogerq, a-govindraju, linux-usb, jun.li,
lznuaa, linux-kernel
On Wed, Apr 27, 2022 at 11:35:25AM -0500, Frank Li wrote:
> Some devices have USB compositions which may require multiple endpoints.
> To get better performance, need bigger CDNS3_EP_BUF_SIZE.
>
> But bigger CDNS3_EP_BUF_SIZE may exceed total hardware FIFO size when
> multiple endpoints.
>
> By introducing the check_config() callback, calculate CDNS3_EP_BUF_SIZE.
>
> Move CDNS3_EP_BUF_SIZE into cnds3_device: ep_buf_size
> Combine CDNS3_EP_ISO_SS_BURST and CDNS3_EP_ISO_HS_MULT into
> ecnds3_device:ep_iso_burst
>
> Using a simple algorithm to calculate ep_buf_size.
> ep_buf_size = ep_iso_burst = (onchip_buffers - 2k) / (number of IN EP +
> 1).
>
> Test at 8qxp:
>
> Gadget ep_buf_size
>
> RNDIS: 5
> RNDIS+ACM: 3
> Mass Storage + NCM + ACM 2
>
> Previous CDNS3_EP_BUF_SIZE is 4, RNDIS + ACM will be failure because
> exceed FIFO memory.
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v1 to v2:
> Add safe check for mult, buffering and maxburst
that's nice, but this is v8, not v2?
confused,
greg k-h
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v8 1/1] usb: cdns3: allocate TX FIFO size according to composite EP number
2022-04-27 16:57 ` Greg KH
@ 2022-04-27 17:03 ` Zhi Li
2022-05-05 19:31 ` Greg KH
0 siblings, 1 reply; 7+ messages in thread
From: Zhi Li @ 2022-04-27 17:03 UTC (permalink / raw)
To: Greg KH
Cc: Frank Li, Peter Chen, pawell, rogerq, a-govindraju, linux-usb,
Jun Li, kernel list
On Wed, Apr 27, 2022 at 11:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Apr 27, 2022 at 11:35:25AM -0500, Frank Li wrote:
> > Some devices have USB compositions which may require multiple endpoints.
> > To get better performance, need bigger CDNS3_EP_BUF_SIZE.
> >
> > But bigger CDNS3_EP_BUF_SIZE may exceed total hardware FIFO size when
> > multiple endpoints.
> >
> > By introducing the check_config() callback, calculate CDNS3_EP_BUF_SIZE.
> >
> > Move CDNS3_EP_BUF_SIZE into cnds3_device: ep_buf_size
> > Combine CDNS3_EP_ISO_SS_BURST and CDNS3_EP_ISO_HS_MULT into
> > ecnds3_device:ep_iso_burst
> >
> > Using a simple algorithm to calculate ep_buf_size.
> > ep_buf_size = ep_iso_burst = (onchip_buffers - 2k) / (number of IN EP +
> > 1).
> >
> > Test at 8qxp:
> >
> > Gadget ep_buf_size
> >
> > RNDIS: 5
> > RNDIS+ACM: 3
> > Mass Storage + NCM + ACM 2
> >
> > Previous CDNS3_EP_BUF_SIZE is 4, RNDIS + ACM will be failure because
> > exceed FIFO memory.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > Change from v1 to v2:
> > Add safe check for mult, buffering and maxburst
>
> that's nice, but this is v8, not v2?
Sorry, It should be v2. I made a mistake with the title.
>
> confused,
>
> greg k-h
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v8 1/1] usb: cdns3: allocate TX FIFO size according to composite EP number
2022-04-27 16:35 [PATCH v8 1/1] usb: cdns3: allocate TX FIFO size according to composite EP number Frank Li
2022-04-27 16:57 ` Greg KH
@ 2022-05-05 2:21 ` Peter Chen
2022-05-05 15:24 ` Zhi Li
1 sibling, 1 reply; 7+ messages in thread
From: Peter Chen @ 2022-05-05 2:21 UTC (permalink / raw)
To: Frank Li
Cc: pawell, rogerq, a-govindraju, linux-usb, jun.li, lznuaa, gregkh,
linux-kernel
On 22-04-27 11:35:25, Frank Li wrote:
> Some devices have USB compositions which may require multiple endpoints.
> To get better performance, need bigger CDNS3_EP_BUF_SIZE.
>
> But bigger CDNS3_EP_BUF_SIZE may exceed total hardware FIFO size when
> multiple endpoints.
>
> By introducing the check_config() callback, calculate CDNS3_EP_BUF_SIZE.
>
> Move CDNS3_EP_BUF_SIZE into cnds3_device: ep_buf_size
> Combine CDNS3_EP_ISO_SS_BURST and CDNS3_EP_ISO_HS_MULT into
> ecnds3_device:ep_iso_burst
typo, and would you please explain usage for ep_iso_burst in your patch?
>
> Using a simple algorithm to calculate ep_buf_size.
> ep_buf_size = ep_iso_burst = (onchip_buffers - 2k) / (number of IN EP +
> 1).
>
> Test at 8qxp:
>
> Gadget ep_buf_size
>
> RNDIS: 5
> RNDIS+ACM: 3
> Mass Storage + NCM + ACM 2
>
> Previous CDNS3_EP_BUF_SIZE is 4, RNDIS + ACM will be failure because
> exceed FIFO memory.
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v1 to v2:
> Add safe check for mult, buffering and maxburst
>
> drivers/usb/cdns3/cdns3-gadget.c | 50 +++++++++++++++++++++++++++++---
> drivers/usb/cdns3/cdns3-gadget.h | 9 ++++--
> 2 files changed, 52 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
> index 1f3b4a1422126..e25e7a2b55862 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.c
> +++ b/drivers/usb/cdns3/cdns3-gadget.c
> @@ -2050,7 +2050,7 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
> u8 mult = 0;
> int ret;
>
> - buffering = CDNS3_EP_BUF_SIZE - 1;
> + buffering = priv_dev->ep_buf_size - 1;
>
> cdns3_configure_dmult(priv_dev, priv_ep);
>
> @@ -2069,7 +2069,7 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
> break;
> default:
> ep_cfg = EP_CFG_EPTYPE(USB_ENDPOINT_XFER_ISOC);
> - mult = CDNS3_EP_ISO_HS_MULT - 1;
> + mult = priv_dev->ep_iso_burst - 1;
> buffering = mult + 1;
> }
>
> @@ -2085,14 +2085,14 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
> mult = 0;
> max_packet_size = 1024;
> if (priv_ep->type == USB_ENDPOINT_XFER_ISOC) {
> - maxburst = CDNS3_EP_ISO_SS_BURST - 1;
> + maxburst = priv_dev->ep_iso_burst - 1;
> buffering = (mult + 1) *
> (maxburst + 1);
>
> if (priv_ep->interval > 1)
> buffering++;
> } else {
> - maxburst = CDNS3_EP_BUF_SIZE - 1;
> + maxburst = priv_dev->ep_buf_size - 1;
> }
> break;
> default:
> @@ -2136,6 +2136,10 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
> ep_cfg |= EP_CFG_STREAM_EN | EP_CFG_TDL_CHK | EP_CFG_SID_CHK;
> }
>
> + mult = min_t(u8, mult, EP_CFG_MULT_MAX);
> + buffering = min_t(u8, buffering, EP_CFG_BUFFERING_MAX);
> + maxburst = min_t(u8, maxburst, EP_CFG_MAXBURST_MAX);
> +
> ep_cfg |= EP_CFG_MAXPKTSIZE(max_packet_size) |
> EP_CFG_MULT(mult) |
> EP_CFG_BUFFERING(buffering) |
> @@ -2970,6 +2974,43 @@ static int cdns3_gadget_udc_stop(struct usb_gadget *gadget)
> return 0;
> }
>
> +/**
> + * cdns3_gadget_check_config - ensure cdns3 can support the USB configuration
> + * @gadget: pointer to the USB gadget
> + *
> + * Used to record the maximum number of endpoints being used in a USB composite
> + * device. (across all configurations) This is to be used in the calculation
> + * of the TXFIFO sizes when resizing internal memory for individual endpoints.
> + * It will help ensured that the resizing logic reserves enough space for at
> + * least one max packet.
> + */
> +static int cdns3_gadget_check_config(struct usb_gadget *gadget)
> +{
> + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget);
> + struct usb_ep *ep;
> + int n_in = 0;
> + int total;
> +
> + list_for_each_entry(ep, &gadget->ep_list, ep_list) {
> + if (ep->claimed && (ep->address & USB_DIR_IN))
> + n_in++;
> + }
> +
> + priv_dev->ep_buf_size = 1;
> + priv_dev->ep_iso_burst = 1;
You will re-calculate above variables below, why need to initialize them?
Peter
> +
> + /* 2KB are reserved for EP0, 1KB for out*/
> + total = 2 + n_in + 1;
> +
> + if (total > priv_dev->onchip_buffers)
> + return -ENOMEM;
> +
> + priv_dev->ep_buf_size = priv_dev->ep_iso_burst =
> + (priv_dev->onchip_buffers - 2) / (n_in + 1);
> +
> + return 0;
> +}
> +
> static const struct usb_gadget_ops cdns3_gadget_ops = {
> .get_frame = cdns3_gadget_get_frame,
> .wakeup = cdns3_gadget_wakeup,
> @@ -2978,6 +3019,7 @@ static const struct usb_gadget_ops cdns3_gadget_ops = {
> .udc_start = cdns3_gadget_udc_start,
> .udc_stop = cdns3_gadget_udc_stop,
> .match_ep = cdns3_gadget_match_ep,
> + .check_config = cdns3_gadget_check_config,
> };
>
> static void cdns3_free_all_eps(struct cdns3_device *priv_dev)
> diff --git a/drivers/usb/cdns3/cdns3-gadget.h b/drivers/usb/cdns3/cdns3-gadget.h
> index c5660f2c4293f..fbe4a8e3aa897 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.h
> +++ b/drivers/usb/cdns3/cdns3-gadget.h
> @@ -562,15 +562,18 @@ struct cdns3_usb_regs {
> /* Max burst size (used only in SS mode). */
> #define EP_CFG_MAXBURST_MASK GENMASK(11, 8)
> #define EP_CFG_MAXBURST(p) (((p) << 8) & EP_CFG_MAXBURST_MASK)
> +#define EP_CFG_MAXBURST_MAX 15
> /* ISO max burst. */
> #define EP_CFG_MULT_MASK GENMASK(15, 14)
> #define EP_CFG_MULT(p) (((p) << 14) & EP_CFG_MULT_MASK)
> +#define EP_CFG_MULT_MAX 2
> /* ISO max burst. */
> #define EP_CFG_MAXPKTSIZE_MASK GENMASK(26, 16)
> #define EP_CFG_MAXPKTSIZE(p) (((p) << 16) & EP_CFG_MAXPKTSIZE_MASK)
> /* Max number of buffered packets. */
> #define EP_CFG_BUFFERING_MASK GENMASK(31, 27)
> #define EP_CFG_BUFFERING(p) (((p) << 27) & EP_CFG_BUFFERING_MASK)
> +#define EP_CFG_BUFFERING_MAX 15
>
> /* EP_CMD - bitmasks */
> /* Endpoint reset. */
> @@ -1094,9 +1097,6 @@ struct cdns3_trb {
> #define CDNS3_ENDPOINTS_MAX_COUNT 32
> #define CDNS3_EP_ZLP_BUF_SIZE 1024
>
> -#define CDNS3_EP_BUF_SIZE 4 /* KB */
> -#define CDNS3_EP_ISO_HS_MULT 3
> -#define CDNS3_EP_ISO_SS_BURST 3
> #define CDNS3_MAX_NUM_DESCMISS_BUF 32
> #define CDNS3_DESCMIS_BUF_SIZE 2048 /* Bytes */
> #define CDNS3_WA2_NUM_BUFFERS 128
> @@ -1333,6 +1333,9 @@ struct cdns3_device {
> /*in KB */
> u16 onchip_buffers;
> u16 onchip_used_size;
> +
> + u16 ep_buf_size;
> + u16 ep_iso_burst;
> };
>
> void cdns3_set_register_bit(void __iomem *ptr, u32 mask);
> --
> 2.35.1
>
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v8 1/1] usb: cdns3: allocate TX FIFO size according to composite EP number
2022-05-05 2:21 ` Peter Chen
@ 2022-05-05 15:24 ` Zhi Li
2022-05-09 3:16 ` Peter Chen
0 siblings, 1 reply; 7+ messages in thread
From: Zhi Li @ 2022-05-05 15:24 UTC (permalink / raw)
To: Peter Chen
Cc: Frank Li, pawell, rogerq, a-govindraju, linux-usb, Jun Li,
Greg KH, kernel list
On Wed, May 4, 2022 at 9:21 PM Peter Chen <peter.chen@kernel.org> wrote:
>
> On 22-04-27 11:35:25, Frank Li wrote:
> > Some devices have USB compositions which may require multiple endpoints.
> > To get better performance, need bigger CDNS3_EP_BUF_SIZE.
> >
> > But bigger CDNS3_EP_BUF_SIZE may exceed total hardware FIFO size when
> > multiple endpoints.
> >
> > By introducing the check_config() callback, calculate CDNS3_EP_BUF_SIZE.
> >
> > Move CDNS3_EP_BUF_SIZE into cnds3_device: ep_buf_size
> > Combine CDNS3_EP_ISO_SS_BURST and CDNS3_EP_ISO_HS_MULT into
> > ecnds3_device:ep_iso_burst
>
> typo, and would you please explain usage for ep_iso_burst in your patch?
ep_iso_burst just directly replaced old CDNS3_EP_ISO_SS_BURST.
This patch uses a simple algorithm to make ep_buf_size equal to ep_iso_burst.
using ep_iso_burst reduces code change and keeps old code logic.
Keep capability to finial tune ep_iso_burst for ep_buf_size in future
for difference
composite devices combination user case.
>
> >
> > Using a simple algorithm to calculate ep_buf_size.
> > ep_buf_size = ep_iso_burst = (onchip_buffers - 2k) / (number of IN EP +
> > 1).
> >
> > Test at 8qxp:
> >
> > Gadget ep_buf_size
> >
> > RNDIS: 5
> > RNDIS+ACM: 3
> > Mass Storage + NCM + ACM 2
> >
> > Previous CDNS3_EP_BUF_SIZE is 4, RNDIS + ACM will be failure because
> > exceed FIFO memory.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > Change from v1 to v2:
> > Add safe check for mult, buffering and maxburst
> >
> > drivers/usb/cdns3/cdns3-gadget.c | 50 +++++++++++++++++++++++++++++---
> > drivers/usb/cdns3/cdns3-gadget.h | 9 ++++--
> > 2 files changed, 52 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
> > index 1f3b4a1422126..e25e7a2b55862 100644
> > --- a/drivers/usb/cdns3/cdns3-gadget.c
> > +++ b/drivers/usb/cdns3/cdns3-gadget.c
> > @@ -2050,7 +2050,7 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
> > u8 mult = 0;
> > int ret;
> >
> > - buffering = CDNS3_EP_BUF_SIZE - 1;
> > + buffering = priv_dev->ep_buf_size - 1;
> >
> > cdns3_configure_dmult(priv_dev, priv_ep);
> >
> > @@ -2069,7 +2069,7 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
> > break;
> > default:
> > ep_cfg = EP_CFG_EPTYPE(USB_ENDPOINT_XFER_ISOC);
> > - mult = CDNS3_EP_ISO_HS_MULT - 1;
> > + mult = priv_dev->ep_iso_burst - 1;
> > buffering = mult + 1;
> > }
> >
> > @@ -2085,14 +2085,14 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
> > mult = 0;
> > max_packet_size = 1024;
> > if (priv_ep->type == USB_ENDPOINT_XFER_ISOC) {
> > - maxburst = CDNS3_EP_ISO_SS_BURST - 1;
> > + maxburst = priv_dev->ep_iso_burst - 1;
> > buffering = (mult + 1) *
> > (maxburst + 1);
> >
> > if (priv_ep->interval > 1)
> > buffering++;
> > } else {
> > - maxburst = CDNS3_EP_BUF_SIZE - 1;
> > + maxburst = priv_dev->ep_buf_size - 1;
> > }
> > break;
> > default:
> > @@ -2136,6 +2136,10 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
> > ep_cfg |= EP_CFG_STREAM_EN | EP_CFG_TDL_CHK | EP_CFG_SID_CHK;
> > }
> >
> > + mult = min_t(u8, mult, EP_CFG_MULT_MAX);
> > + buffering = min_t(u8, buffering, EP_CFG_BUFFERING_MAX);
> > + maxburst = min_t(u8, maxburst, EP_CFG_MAXBURST_MAX);
> > +
> > ep_cfg |= EP_CFG_MAXPKTSIZE(max_packet_size) |
> > EP_CFG_MULT(mult) |
> > EP_CFG_BUFFERING(buffering) |
> > @@ -2970,6 +2974,43 @@ static int cdns3_gadget_udc_stop(struct usb_gadget *gadget)
> > return 0;
> > }
> >
> > +/**
> > + * cdns3_gadget_check_config - ensure cdns3 can support the USB configuration
> > + * @gadget: pointer to the USB gadget
> > + *
> > + * Used to record the maximum number of endpoints being used in a USB composite
> > + * device. (across all configurations) This is to be used in the calculation
> > + * of the TXFIFO sizes when resizing internal memory for individual endpoints.
> > + * It will help ensured that the resizing logic reserves enough space for at
> > + * least one max packet.
> > + */
> > +static int cdns3_gadget_check_config(struct usb_gadget *gadget)
> > +{
> > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget);
> > + struct usb_ep *ep;
> > + int n_in = 0;
> > + int total;
> > +
> > + list_for_each_entry(ep, &gadget->ep_list, ep_list) {
> > + if (ep->claimed && (ep->address & USB_DIR_IN))
> > + n_in++;
> > + }
> > +
> > + priv_dev->ep_buf_size = 1;
> > + priv_dev->ep_iso_burst = 1;
>
> You will re-calculate above variables below, why need to initialize them?
I worry about it is 0 or other random data at below error return branch.
Anyway, it should be okay to remove it.
>
> Peter
> > +
> > + /* 2KB are reserved for EP0, 1KB for out*/
> > + total = 2 + n_in + 1;
> > +
> > + if (total > priv_dev->onchip_buffers)
> > + return -ENOMEM;
> > +
> > + priv_dev->ep_buf_size = priv_dev->ep_iso_burst =
> > + (priv_dev->onchip_buffers - 2) / (n_in + 1);
> > +
> > + return 0;
> > +}
> > +
> > static const struct usb_gadget_ops cdns3_gadget_ops = {
> > .get_frame = cdns3_gadget_get_frame,
> > .wakeup = cdns3_gadget_wakeup,
> > @@ -2978,6 +3019,7 @@ static const struct usb_gadget_ops cdns3_gadget_ops = {
> > .udc_start = cdns3_gadget_udc_start,
> > .udc_stop = cdns3_gadget_udc_stop,
> > .match_ep = cdns3_gadget_match_ep,
> > + .check_config = cdns3_gadget_check_config,
> > };
> >
> > static void cdns3_free_all_eps(struct cdns3_device *priv_dev)
> > diff --git a/drivers/usb/cdns3/cdns3-gadget.h b/drivers/usb/cdns3/cdns3-gadget.h
> > index c5660f2c4293f..fbe4a8e3aa897 100644
> > --- a/drivers/usb/cdns3/cdns3-gadget.h
> > +++ b/drivers/usb/cdns3/cdns3-gadget.h
> > @@ -562,15 +562,18 @@ struct cdns3_usb_regs {
> > /* Max burst size (used only in SS mode). */
> > #define EP_CFG_MAXBURST_MASK GENMASK(11, 8)
> > #define EP_CFG_MAXBURST(p) (((p) << 8) & EP_CFG_MAXBURST_MASK)
> > +#define EP_CFG_MAXBURST_MAX 15
> > /* ISO max burst. */
> > #define EP_CFG_MULT_MASK GENMASK(15, 14)
> > #define EP_CFG_MULT(p) (((p) << 14) & EP_CFG_MULT_MASK)
> > +#define EP_CFG_MULT_MAX 2
> > /* ISO max burst. */
> > #define EP_CFG_MAXPKTSIZE_MASK GENMASK(26, 16)
> > #define EP_CFG_MAXPKTSIZE(p) (((p) << 16) & EP_CFG_MAXPKTSIZE_MASK)
> > /* Max number of buffered packets. */
> > #define EP_CFG_BUFFERING_MASK GENMASK(31, 27)
> > #define EP_CFG_BUFFERING(p) (((p) << 27) & EP_CFG_BUFFERING_MASK)
> > +#define EP_CFG_BUFFERING_MAX 15
> >
> > /* EP_CMD - bitmasks */
> > /* Endpoint reset. */
> > @@ -1094,9 +1097,6 @@ struct cdns3_trb {
> > #define CDNS3_ENDPOINTS_MAX_COUNT 32
> > #define CDNS3_EP_ZLP_BUF_SIZE 1024
> >
> > -#define CDNS3_EP_BUF_SIZE 4 /* KB */
> > -#define CDNS3_EP_ISO_HS_MULT 3
> > -#define CDNS3_EP_ISO_SS_BURST 3
> > #define CDNS3_MAX_NUM_DESCMISS_BUF 32
> > #define CDNS3_DESCMIS_BUF_SIZE 2048 /* Bytes */
> > #define CDNS3_WA2_NUM_BUFFERS 128
> > @@ -1333,6 +1333,9 @@ struct cdns3_device {
> > /*in KB */
> > u16 onchip_buffers;
> > u16 onchip_used_size;
> > +
> > + u16 ep_buf_size;
> > + u16 ep_iso_burst;
> > };
> >
> > void cdns3_set_register_bit(void __iomem *ptr, u32 mask);
> > --
> > 2.35.1
> >
>
> --
>
> Thanks,
> Peter Chen
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v8 1/1] usb: cdns3: allocate TX FIFO size according to composite EP number
2022-04-27 17:03 ` Zhi Li
@ 2022-05-05 19:31 ` Greg KH
0 siblings, 0 replies; 7+ messages in thread
From: Greg KH @ 2022-05-05 19:31 UTC (permalink / raw)
To: Zhi Li
Cc: Frank Li, Peter Chen, pawell, rogerq, a-govindraju, linux-usb,
Jun Li, kernel list
On Wed, Apr 27, 2022 at 12:03:22PM -0500, Zhi Li wrote:
> On Wed, Apr 27, 2022 at 11:57 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Apr 27, 2022 at 11:35:25AM -0500, Frank Li wrote:
> > > Some devices have USB compositions which may require multiple endpoints.
> > > To get better performance, need bigger CDNS3_EP_BUF_SIZE.
> > >
> > > But bigger CDNS3_EP_BUF_SIZE may exceed total hardware FIFO size when
> > > multiple endpoints.
> > >
> > > By introducing the check_config() callback, calculate CDNS3_EP_BUF_SIZE.
> > >
> > > Move CDNS3_EP_BUF_SIZE into cnds3_device: ep_buf_size
> > > Combine CDNS3_EP_ISO_SS_BURST and CDNS3_EP_ISO_HS_MULT into
> > > ecnds3_device:ep_iso_burst
> > >
> > > Using a simple algorithm to calculate ep_buf_size.
> > > ep_buf_size = ep_iso_burst = (onchip_buffers - 2k) / (number of IN EP +
> > > 1).
> > >
> > > Test at 8qxp:
> > >
> > > Gadget ep_buf_size
> > >
> > > RNDIS: 5
> > > RNDIS+ACM: 3
> > > Mass Storage + NCM + ACM 2
> > >
> > > Previous CDNS3_EP_BUF_SIZE is 4, RNDIS + ACM will be failure because
> > > exceed FIFO memory.
> > >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > > Change from v1 to v2:
> > > Add safe check for mult, buffering and maxburst
> >
> > that's nice, but this is v8, not v2?
>
> Sorry, It should be v2. I made a mistake with the title.
Ok, the next version needs to be 9 to keep everything from getting
really confused.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v8 1/1] usb: cdns3: allocate TX FIFO size according to composite EP number
2022-05-05 15:24 ` Zhi Li
@ 2022-05-09 3:16 ` Peter Chen
0 siblings, 0 replies; 7+ messages in thread
From: Peter Chen @ 2022-05-09 3:16 UTC (permalink / raw)
To: Zhi Li
Cc: Frank Li, pawell, rogerq, a-govindraju, linux-usb, Jun Li,
Greg KH, kernel list
On 22-05-05 10:24:12, Zhi Li wrote:
> On Wed, May 4, 2022 at 9:21 PM Peter Chen <peter.chen@kernel.org> wrote:
> >
> > On 22-04-27 11:35:25, Frank Li wrote:
> > > Some devices have USB compositions which may require multiple endpoints.
> > > To get better performance, need bigger CDNS3_EP_BUF_SIZE.
> > >
> > > But bigger CDNS3_EP_BUF_SIZE may exceed total hardware FIFO size when
> > > multiple endpoints.
> > >
> > > By introducing the check_config() callback, calculate CDNS3_EP_BUF_SIZE.
> > >
> > > Move CDNS3_EP_BUF_SIZE into cnds3_device: ep_buf_size
> > > Combine CDNS3_EP_ISO_SS_BURST and CDNS3_EP_ISO_HS_MULT into
> > > ecnds3_device:ep_iso_burst
> >
> > typo, and would you please explain usage for ep_iso_burst in your patch?
>
> ep_iso_burst just directly replaced old CDNS3_EP_ISO_SS_BURST.
>
> This patch uses a simple algorithm to make ep_buf_size equal to ep_iso_burst.
>
> using ep_iso_burst reduces code change and keeps old code logic.
> Keep capability to finial tune ep_iso_burst for ep_buf_size in future
> for difference
> composite devices combination user case.
>
> >
> > >
> > > Using a simple algorithm to calculate ep_buf_size.
> > > ep_buf_size = ep_iso_burst = (onchip_buffers - 2k) / (number of IN EP +
> > > 1).
> > >
> > > Test at 8qxp:
> > >
> > > Gadget ep_buf_size
> > >
> > > RNDIS: 5
> > > RNDIS+ACM: 3
> > > Mass Storage + NCM + ACM 2
> > >
> > > Previous CDNS3_EP_BUF_SIZE is 4, RNDIS + ACM will be failure because
> > > exceed FIFO memory.
> > >
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > > Change from v1 to v2:
> > > Add safe check for mult, buffering and maxburst
> > >
> > > drivers/usb/cdns3/cdns3-gadget.c | 50 +++++++++++++++++++++++++++++---
> > > drivers/usb/cdns3/cdns3-gadget.h | 9 ++++--
> > > 2 files changed, 52 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
> > > index 1f3b4a1422126..e25e7a2b55862 100644
> > > --- a/drivers/usb/cdns3/cdns3-gadget.c
> > > +++ b/drivers/usb/cdns3/cdns3-gadget.c
> > > @@ -2050,7 +2050,7 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
> > > u8 mult = 0;
> > > int ret;
> > >
> > > - buffering = CDNS3_EP_BUF_SIZE - 1;
> > > + buffering = priv_dev->ep_buf_size - 1;
> > >
> > > cdns3_configure_dmult(priv_dev, priv_ep);
> > >
> > > @@ -2069,7 +2069,7 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
> > > break;
> > > default:
> > > ep_cfg = EP_CFG_EPTYPE(USB_ENDPOINT_XFER_ISOC);
> > > - mult = CDNS3_EP_ISO_HS_MULT - 1;
> > > + mult = priv_dev->ep_iso_burst - 1;
> > > buffering = mult + 1;
> > > }
> > >
> > > @@ -2085,14 +2085,14 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
> > > mult = 0;
> > > max_packet_size = 1024;
> > > if (priv_ep->type == USB_ENDPOINT_XFER_ISOC) {
> > > - maxburst = CDNS3_EP_ISO_SS_BURST - 1;
> > > + maxburst = priv_dev->ep_iso_burst - 1;
> > > buffering = (mult + 1) *
> > > (maxburst + 1);
> > >
> > > if (priv_ep->interval > 1)
> > > buffering++;
> > > } else {
> > > - maxburst = CDNS3_EP_BUF_SIZE - 1;
> > > + maxburst = priv_dev->ep_buf_size - 1;
> > > }
> > > break;
> > > default:
> > > @@ -2136,6 +2136,10 @@ int cdns3_ep_config(struct cdns3_endpoint *priv_ep, bool enable)
> > > ep_cfg |= EP_CFG_STREAM_EN | EP_CFG_TDL_CHK | EP_CFG_SID_CHK;
> > > }
> > >
> > > + mult = min_t(u8, mult, EP_CFG_MULT_MAX);
> > > + buffering = min_t(u8, buffering, EP_CFG_BUFFERING_MAX);
> > > + maxburst = min_t(u8, maxburst, EP_CFG_MAXBURST_MAX);
These compare need to move above to keep variable "buffering" calculation
correctly.
> > > +
> > > ep_cfg |= EP_CFG_MAXPKTSIZE(max_packet_size) |
> > > EP_CFG_MULT(mult) |
> > > EP_CFG_BUFFERING(buffering) |
> > > @@ -2970,6 +2974,43 @@ static int cdns3_gadget_udc_stop(struct usb_gadget *gadget)
> > > return 0;
> > > }
> > >
> > > +/**
> > > + * cdns3_gadget_check_config - ensure cdns3 can support the USB configuration
> > > + * @gadget: pointer to the USB gadget
> > > + *
> > > + * Used to record the maximum number of endpoints being used in a USB composite
> > > + * device. (across all configurations) This is to be used in the calculation
> > > + * of the TXFIFO sizes when resizing internal memory for individual endpoints.
> > > + * It will help ensured that the resizing logic reserves enough space for at
> > > + * least one max packet.
> > > + */
> > > +static int cdns3_gadget_check_config(struct usb_gadget *gadget)
> > > +{
> > > + struct cdns3_device *priv_dev = gadget_to_cdns3_device(gadget);
> > > + struct usb_ep *ep;
> > > + int n_in = 0;
> > > + int total;
> > > +
> > > + list_for_each_entry(ep, &gadget->ep_list, ep_list) {
> > > + if (ep->claimed && (ep->address & USB_DIR_IN))
> > > + n_in++;
> > > + }
> > > +
> > > + priv_dev->ep_buf_size = 1;
> > > + priv_dev->ep_iso_burst = 1;
> >
> > You will re-calculate above variables below, why need to initialize them?
>
> I worry about it is 0 or other random data at below error return branch.
> Anyway, it should be okay to remove it.
If it is failed, the .bind will fail, so it is okay to remove these.
Other parts are okay for me.
Peter
>
> >
> > Peter
> > > +
> > > + /* 2KB are reserved for EP0, 1KB for out*/
> > > + total = 2 + n_in + 1;
> > > +
> > > + if (total > priv_dev->onchip_buffers)
> > > + return -ENOMEM;
> > > +
> > > + priv_dev->ep_buf_size = priv_dev->ep_iso_burst =
> > > + (priv_dev->onchip_buffers - 2) / (n_in + 1);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static const struct usb_gadget_ops cdns3_gadget_ops = {
> > > .get_frame = cdns3_gadget_get_frame,
> > > .wakeup = cdns3_gadget_wakeup,
> > > @@ -2978,6 +3019,7 @@ static const struct usb_gadget_ops cdns3_gadget_ops = {
> > > .udc_start = cdns3_gadget_udc_start,
> > > .udc_stop = cdns3_gadget_udc_stop,
> > > .match_ep = cdns3_gadget_match_ep,
> > > + .check_config = cdns3_gadget_check_config,
> > > };
> > >
> > > static void cdns3_free_all_eps(struct cdns3_device *priv_dev)
> > > diff --git a/drivers/usb/cdns3/cdns3-gadget.h b/drivers/usb/cdns3/cdns3-gadget.h
> > > index c5660f2c4293f..fbe4a8e3aa897 100644
> > > --- a/drivers/usb/cdns3/cdns3-gadget.h
> > > +++ b/drivers/usb/cdns3/cdns3-gadget.h
> > > @@ -562,15 +562,18 @@ struct cdns3_usb_regs {
> > > /* Max burst size (used only in SS mode). */
> > > #define EP_CFG_MAXBURST_MASK GENMASK(11, 8)
> > > #define EP_CFG_MAXBURST(p) (((p) << 8) & EP_CFG_MAXBURST_MASK)
> > > +#define EP_CFG_MAXBURST_MAX 15
> > > /* ISO max burst. */
> > > #define EP_CFG_MULT_MASK GENMASK(15, 14)
> > > #define EP_CFG_MULT(p) (((p) << 14) & EP_CFG_MULT_MASK)
> > > +#define EP_CFG_MULT_MAX 2
> > > /* ISO max burst. */
> > > #define EP_CFG_MAXPKTSIZE_MASK GENMASK(26, 16)
> > > #define EP_CFG_MAXPKTSIZE(p) (((p) << 16) & EP_CFG_MAXPKTSIZE_MASK)
> > > /* Max number of buffered packets. */
> > > #define EP_CFG_BUFFERING_MASK GENMASK(31, 27)
> > > #define EP_CFG_BUFFERING(p) (((p) << 27) & EP_CFG_BUFFERING_MASK)
> > > +#define EP_CFG_BUFFERING_MAX 15
> > >
> > > /* EP_CMD - bitmasks */
> > > /* Endpoint reset. */
> > > @@ -1094,9 +1097,6 @@ struct cdns3_trb {
> > > #define CDNS3_ENDPOINTS_MAX_COUNT 32
> > > #define CDNS3_EP_ZLP_BUF_SIZE 1024
> > >
> > > -#define CDNS3_EP_BUF_SIZE 4 /* KB */
> > > -#define CDNS3_EP_ISO_HS_MULT 3
> > > -#define CDNS3_EP_ISO_SS_BURST 3
> > > #define CDNS3_MAX_NUM_DESCMISS_BUF 32
> > > #define CDNS3_DESCMIS_BUF_SIZE 2048 /* Bytes */
> > > #define CDNS3_WA2_NUM_BUFFERS 128
> > > @@ -1333,6 +1333,9 @@ struct cdns3_device {
> > > /*in KB */
> > > u16 onchip_buffers;
> > > u16 onchip_used_size;
> > > +
> > > + u16 ep_buf_size;
> > > + u16 ep_iso_burst;
> > > };
> > >
> > > void cdns3_set_register_bit(void __iomem *ptr, u32 mask);
> > > --
> > > 2.35.1
> > >
> >
> > --
> >
> > Thanks,
> > Peter Chen
> >
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-05-09 3:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-27 16:35 [PATCH v8 1/1] usb: cdns3: allocate TX FIFO size according to composite EP number Frank Li
2022-04-27 16:57 ` Greg KH
2022-04-27 17:03 ` Zhi Li
2022-05-05 19:31 ` Greg KH
2022-05-05 2:21 ` Peter Chen
2022-05-05 15:24 ` Zhi Li
2022-05-09 3:16 ` Peter Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).