As discussed in following patch:
https://patchwork.kernel.org/patch/12420339
No need calculate number of uframes again, but should use value
form check_sch_tt(), if we plan to remove extra CS, also can do
it in check_sch_tt(). So revert this patch, and prepare to send
new patch for it.
This reverts commit 548011957d1d72e0b662300c8b32b81d593b796e.
Cc: Ikjoon Jang <[email protected]>
Signed-off-by: Chunfeng Yun <[email protected]>
---
v2: no changes
---
drivers/usb/host/xhci-mtk-sch.c | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
index 46cbf5d54f4f..f9b4d27ce449 100644
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -459,17 +459,16 @@ static int check_fs_bus_bw(struct mu3h_sch_ep_info *sch_ep, int offset)
u32 num_esit, tmp;
int base;
int i, j;
- u8 uframes = DIV_ROUND_UP(sch_ep->maxpkt, FS_PAYLOAD_MAX);
num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
-
- if (sch_ep->ep_type == INT_IN_EP || sch_ep->ep_type == ISOC_IN_EP)
- offset++;
-
for (i = 0; i < num_esit; i++) {
base = offset + i * sch_ep->esit;
- for (j = 0; j < uframes; j++) {
+ /*
+ * Compared with hs bus, no matter what ep type,
+ * the hub will always delay one uframe to send data
+ */
+ for (j = 0; j < sch_ep->cs_count; j++) {
tmp = tt->fs_bus_bw[base + j] + sch_ep->bw_cost_per_microframe;
if (tmp > FS_PAYLOAD_MAX)
return -ESCH_BW_OVERFLOW;
@@ -547,8 +546,6 @@ static void update_sch_tt(struct mu3h_sch_ep_info *sch_ep, bool used)
u32 base, num_esit;
int bw_updated;
int i, j;
- int offset = sch_ep->offset;
- u8 uframes = DIV_ROUND_UP(sch_ep->maxpkt, FS_PAYLOAD_MAX);
num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
@@ -557,13 +554,10 @@ static void update_sch_tt(struct mu3h_sch_ep_info *sch_ep, bool used)
else
bw_updated = -sch_ep->bw_cost_per_microframe;
- if (sch_ep->ep_type == INT_IN_EP || sch_ep->ep_type == ISOC_IN_EP)
- offset++;
-
for (i = 0; i < num_esit; i++) {
- base = offset + i * sch_ep->esit;
+ base = sch_ep->offset + i * sch_ep->esit;
- for (j = 0; j < uframes; j++)
+ for (j = 0; j < sch_ep->cs_count; j++)
tt->fs_bus_bw[base + j] += bw_updated;
}
--
2.18.0
xhci-mtk has 64 slots for periodic bandwidth calculations and each
slot represents byte budgets on a microframe. When an endpoint's
allocation sits on the boundary of the table, byte budgets' slot
can be rolled over but the current implementation doesn't.
This patch allows the microframe index rollover and prevent
out-of-bounds array access.
Signed-off-by: Ikjoon Jang <[email protected]>
Signed-off-by: Chunfeng Yun <[email protected]>
---
v2: new patch
---
drivers/usb/host/xhci-mtk-sch.c | 51 +++++++++++----------------------
drivers/usb/host/xhci-mtk.h | 3 +-
2 files changed, 18 insertions(+), 36 deletions(-)
diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
index f3c6f078f82d..134f4789bd89 100644
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -416,15 +416,14 @@ static u32 get_max_bw(struct mu3h_sch_bw_info *sch_bw,
{
u32 max_bw = 0;
u32 bw;
- int i;
- int j;
+ int i, j, k;
for (i = 0; i < sch_ep->num_esit; i++) {
u32 base = offset + i * sch_ep->esit;
for (j = 0; j < sch_ep->num_budget_microframes; j++) {
- bw = sch_bw->bus_bw[base + j] +
- sch_ep->bw_budget_table[j];
+ k = XHCI_MTK_BW_INDEX(base + j);
+ bw = sch_bw->bus_bw[k] + sch_ep->bw_budget_table[j];
if (bw > max_bw)
max_bw = bw;
}
@@ -436,18 +435,16 @@ static void update_bus_bw(struct mu3h_sch_bw_info *sch_bw,
struct mu3h_sch_ep_info *sch_ep, bool used)
{
u32 base;
- int i;
- int j;
+ int i, j, k;
for (i = 0; i < sch_ep->num_esit; i++) {
base = sch_ep->offset + i * sch_ep->esit;
for (j = 0; j < sch_ep->num_budget_microframes; j++) {
+ k = XHCI_MTK_BW_INDEX(base + j);
if (used)
- sch_bw->bus_bw[base + j] +=
- sch_ep->bw_budget_table[j];
+ sch_bw->bus_bw[k] += sch_ep->bw_budget_table[j];
else
- sch_bw->bus_bw[base + j] -=
- sch_ep->bw_budget_table[j];
+ sch_bw->bus_bw[k] -= sch_ep->bw_budget_table[j];
}
}
}
@@ -457,7 +454,7 @@ static int check_fs_bus_bw(struct mu3h_sch_ep_info *sch_ep, int offset)
struct mu3h_sch_tt *tt = sch_ep->sch_tt;
u32 tmp;
int base;
- int i, j;
+ int i, j, k;
for (i = 0; i < sch_ep->num_esit; i++) {
base = offset + i * sch_ep->esit;
@@ -467,7 +464,8 @@ static int check_fs_bus_bw(struct mu3h_sch_ep_info *sch_ep, int offset)
* the hub will always delay one uframe to send data
*/
for (j = 0; j < sch_ep->num_budget_microframes; j++) {
- tmp = tt->fs_bus_bw[base + j] + sch_ep->bw_budget_table[j];
+ k = XHCI_MTK_BW_INDEX(base + j);
+ tmp = tt->fs_bus_bw[k] + sch_ep->bw_budget_table[j];
if (tmp > FS_PAYLOAD_MAX)
return -ESCH_BW_OVERFLOW;
}
@@ -542,16 +540,18 @@ static void update_sch_tt(struct mu3h_sch_ep_info *sch_ep, bool used)
{
struct mu3h_sch_tt *tt = sch_ep->sch_tt;
u32 base;
- int i, j;
+ int i, j, k;
for (i = 0; i < sch_ep->num_esit; i++) {
base = sch_ep->offset + i * sch_ep->esit;
- for (j = 0; j < sch_ep->num_budget_microframes; j++)
+ for (j = 0; j < sch_ep->num_budget_microframes; j++) {
+ k = XHCI_MTK_BW_INDEX(base + j);
if (used)
- tt->fs_bus_bw[base + j] += sch_ep->bw_budget_table[j];
+ tt->fs_bus_bw[k] += sch_ep->bw_budget_table[j];
else
- tt->fs_bus_bw[base + j] -= sch_ep->bw_budget_table[j];
+ tt->fs_bus_bw[k] -= sch_ep->bw_budget_table[j];
+ }
}
if (used)
@@ -573,25 +573,9 @@ static int load_ep_bw(struct mu3h_sch_bw_info *sch_bw,
return 0;
}
-static u32 get_esit_boundary(struct mu3h_sch_ep_info *sch_ep)
-{
- u32 boundary = sch_ep->esit;
-
- if (sch_ep->sch_tt) { /* LS/FS with TT */
- /* tune for CS */
- if (sch_ep->ep_type != ISOC_OUT_EP)
- boundary++;
- else if (boundary > 1) /* normally esit >= 8 for FS/LS */
- boundary--;
- }
-
- return boundary;
-}
-
static int check_sch_bw(struct mu3h_sch_ep_info *sch_ep)
{
struct mu3h_sch_bw_info *sch_bw = sch_ep->bw_info;
- const u32 esit_boundary = get_esit_boundary(sch_ep);
const u32 bw_boundary = get_bw_boundary(sch_ep->speed);
u32 offset;
u32 worst_bw;
@@ -608,9 +592,6 @@ static int check_sch_bw(struct mu3h_sch_ep_info *sch_ep)
if (ret)
continue;
- if ((offset + sch_ep->num_budget_microframes) > esit_boundary)
- break;
-
worst_bw = get_max_bw(sch_bw, sch_ep, offset);
if (worst_bw > bw_boundary)
continue;
diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index dc5e83f5c5cc..7cc0123eada0 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -25,7 +25,8 @@
* round down to the limit value, that means allocating more
* bandwidth to it.
*/
-#define XHCI_MTK_MAX_ESIT 64
+#define XHCI_MTK_MAX_ESIT (1 << 6)
+#define XHCI_MTK_BW_INDEX(x) ((x) & (XHCI_MTK_MAX_ESIT - 1))
/**
* @fs_bus_bw: array to keep track of bandwidth already used for FS
--
2.18.0
Add a member num_esit to save the number of esit, then no need
caculate it in some functions.
Signed-off-by: Chunfeng Yun <[email protected]>
---
v2: new patch, move from another series
---
drivers/usb/host/xhci-mtk-sch.c | 20 +++++++-------------
drivers/usb/host/xhci-mtk.h | 2 ++
2 files changed, 9 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
index 83abd28269ca..54c521524bfc 100644
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -297,6 +297,7 @@ static void setup_sch_info(struct xhci_ep_ctx *ep_ctx,
CTX_TO_MAX_ESIT_PAYLOAD(le32_to_cpu(ep_ctx->tx_info));
sch_ep->esit = get_esit(ep_ctx);
+ sch_ep->num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
sch_ep->ep_type = ep_type;
sch_ep->maxpkt = maxpkt;
sch_ep->offset = 0;
@@ -401,14 +402,12 @@ static void setup_sch_info(struct xhci_ep_ctx *ep_ctx,
static u32 get_max_bw(struct mu3h_sch_bw_info *sch_bw,
struct mu3h_sch_ep_info *sch_ep, u32 offset)
{
- u32 num_esit;
u32 max_bw = 0;
u32 bw;
int i;
int j;
- num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
- for (i = 0; i < num_esit; i++) {
+ for (i = 0; i < sch_ep->num_esit; i++) {
u32 base = offset + i * sch_ep->esit;
for (j = 0; j < sch_ep->num_budget_microframes; j++) {
@@ -424,13 +423,11 @@ static u32 get_max_bw(struct mu3h_sch_bw_info *sch_bw,
static void update_bus_bw(struct mu3h_sch_bw_info *sch_bw,
struct mu3h_sch_ep_info *sch_ep, bool used)
{
- u32 num_esit;
u32 base;
int i;
int j;
- num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
- for (i = 0; i < num_esit; i++) {
+ for (i = 0; i < sch_ep->num_esit; i++) {
base = sch_ep->offset + i * sch_ep->esit;
for (j = 0; j < sch_ep->num_budget_microframes; j++) {
if (used)
@@ -446,12 +443,11 @@ static void update_bus_bw(struct mu3h_sch_bw_info *sch_bw,
static int check_fs_bus_bw(struct mu3h_sch_ep_info *sch_ep, int offset)
{
struct mu3h_sch_tt *tt = sch_ep->sch_tt;
- u32 num_esit, tmp;
+ u32 tmp;
int base;
int i, j;
- num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
- for (i = 0; i < num_esit; i++) {
+ for (i = 0; i < sch_ep->num_esit; i++) {
base = offset + i * sch_ep->esit;
/*
@@ -533,12 +529,10 @@ static int check_sch_tt(struct mu3h_sch_ep_info *sch_ep, u32 offset)
static void update_sch_tt(struct mu3h_sch_ep_info *sch_ep, bool used)
{
struct mu3h_sch_tt *tt = sch_ep->sch_tt;
- u32 base, num_esit;
+ u32 base;
int i, j;
- num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
-
- for (i = 0; i < num_esit; i++) {
+ for (i = 0; i < sch_ep->num_esit; i++) {
base = sch_ep->offset + i * sch_ep->esit;
for (j = 0; j < sch_ep->num_budget_microframes; j++)
diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index ace432356c41..eb05aaf6edbe 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -50,6 +50,7 @@ struct mu3h_sch_bw_info {
* struct mu3h_sch_ep_info: schedule information for endpoint
*
* @esit: unit is 125us, equal to 2 << Interval field in ep-context
+ * @num_esit: number of @esit in a period
* @num_budget_microframes: number of continuous uframes
* (@repeat==1) scheduled within the interval
* @bw_cost_per_microframe: bandwidth cost per microframe
@@ -79,6 +80,7 @@ struct mu3h_sch_bw_info {
*/
struct mu3h_sch_ep_info {
u32 esit;
+ u32 num_esit;
u32 num_budget_microframes;
u32 bw_cost_per_microframe;
struct list_head endpoint;
--
2.18.0
Use @bw_budget_table[] to update fs bus bandwidth due to
not all microframes consume @bw_cost_per_microframe, see
setup_sch_info().
Signed-off-by: Chunfeng Yun <[email protected]>
---
v2: new patch, move from another series
---
drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
index cffcaf4dfa9f..83abd28269ca 100644
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct mu3h_sch_ep_info *sch_ep, int offset)
* Compared with hs bus, no matter what ep type,
* the hub will always delay one uframe to send data
*/
- for (j = 0; j < sch_ep->cs_count; j++) {
- tmp = tt->fs_bus_bw[base + j] + sch_ep->bw_cost_per_microframe;
+ for (j = 0; j < sch_ep->num_budget_microframes; j++) {
+ tmp = tt->fs_bus_bw[base + j] + sch_ep->bw_budget_table[j];
if (tmp > FS_PAYLOAD_MAX)
return -ESCH_BW_OVERFLOW;
}
@@ -534,21 +534,18 @@ static void update_sch_tt(struct mu3h_sch_ep_info *sch_ep, bool used)
{
struct mu3h_sch_tt *tt = sch_ep->sch_tt;
u32 base, num_esit;
- int bw_updated;
int i, j;
num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
- if (used)
- bw_updated = sch_ep->bw_cost_per_microframe;
- else
- bw_updated = -sch_ep->bw_cost_per_microframe;
-
for (i = 0; i < num_esit; i++) {
base = sch_ep->offset + i * sch_ep->esit;
- for (j = 0; j < sch_ep->cs_count; j++)
- tt->fs_bus_bw[base + j] += bw_updated;
+ for (j = 0; j < sch_ep->num_budget_microframes; j++)
+ if (used)
+ tt->fs_bus_bw[base + j] += sch_ep->bw_budget_table[j];
+ else
+ tt->fs_bus_bw[base + j] -= sch_ep->bw_budget_table[j];
}
if (used)
--
2.18.0
xhci-mtk depends on xhci's internal virt_dev when it retrieves its
internal data from usb_host_endpoint both in add_endpoint and
drop_endpoint callbacks. But when setup packet was retired by
transaction errors in xhci_setup_device() path, a virt_dev for the slot
is newly created with real_port 0. This leads to xhci-mtks's NULL pointer
dereference from drop_endpoint callback as xhci-mtk assumes that virt_dev's
real_port is always started from one. The similar problems were addressed
by [1] but that can't cover the failure cases from setup_device.
This patch drops the usages of xhci's virt_dev in xhci-mtk's drop_endpoint
callback by adopting hashtable for searching mtk's schedule entity
from a given usb_host_endpoint pointer instead of searching a linked list.
So mtk's drop_endpoint callback doesn't have to rely on virt_dev at all.
[1] f351f4b63dac ("usb: xhci-mtk: fix oops when unbind driver")
Signed-off-by: Ikjoon Jang <[email protected]>
Signed-off-by: Chunfeng Yun <[email protected]>
---
v2: use fixed size hash table instead of rhashtable;
fix some issues as described in this series [2/6]
---
drivers/usb/host/xhci-mtk-sch.c | 100 ++++++++++++++++----------------
drivers/usb/host/xhci-mtk.h | 11 +++-
2 files changed, 60 insertions(+), 51 deletions(-)
diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
index 54c521524bfc..f3c6f078f82d 100644
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -80,7 +80,7 @@ decode_ep(struct usb_host_endpoint *ep, enum usb_device_speed speed)
interval /= 1000;
}
- snprintf(buf, DBG_BUF_EN, "%s ep%d%s %s, mpkt:%d, interval:%d/%d%s\n",
+ snprintf(buf, DBG_BUF_EN, "%s ep%d%s %s, mpkt:%d, interval:%d/%d%s",
usb_speed_string(speed), usb_endpoint_num(epd),
usb_endpoint_dir_in(epd) ? "in" : "out",
usb_ep_type_string(usb_endpoint_type(epd)),
@@ -129,6 +129,10 @@ get_bw_info(struct xhci_hcd_mtk *mtk, struct usb_device *udev,
int bw_index;
virt_dev = xhci->devs[udev->slot_id];
+ if (!virt_dev->real_port) {
+ WARN_ONCE(1, "%s invalid real_port\n", dev_name(&udev->dev));
+ return NULL;
+ }
if (udev->speed >= USB_SPEED_SUPER) {
if (usb_endpoint_dir_out(&ep->desc))
@@ -236,14 +240,20 @@ static void drop_tt(struct usb_device *udev)
}
}
-static struct mu3h_sch_ep_info *create_sch_ep(struct usb_device *udev,
- struct usb_host_endpoint *ep, struct xhci_ep_ctx *ep_ctx)
+static struct mu3h_sch_ep_info *
+create_sch_ep(struct xhci_hcd_mtk *mtk, struct usb_device *udev,
+ struct usb_host_endpoint *ep, struct xhci_ep_ctx *ep_ctx)
{
struct mu3h_sch_ep_info *sch_ep;
+ struct mu3h_sch_bw_info *bw_info;
struct mu3h_sch_tt *tt = NULL;
u32 len_bw_budget_table;
size_t mem_size;
+ bw_info = get_bw_info(mtk, udev, ep);
+ if (!bw_info)
+ return ERR_PTR(-ENODEV);
+
if (is_fs_or_ls(udev->speed))
len_bw_budget_table = TT_MICROFRAMES_MAX;
else if ((udev->speed >= USB_SPEED_SUPER)
@@ -266,11 +276,13 @@ static struct mu3h_sch_ep_info *create_sch_ep(struct usb_device *udev,
}
}
+ sch_ep->bw_info = bw_info;
sch_ep->sch_tt = tt;
sch_ep->ep = ep;
sch_ep->speed = udev->speed;
INIT_LIST_HEAD(&sch_ep->endpoint);
INIT_LIST_HEAD(&sch_ep->tt_endpoint);
+ INIT_HLIST_NODE(&sch_ep->hentry);
return sch_ep;
}
@@ -576,9 +588,9 @@ static u32 get_esit_boundary(struct mu3h_sch_ep_info *sch_ep)
return boundary;
}
-static int check_sch_bw(struct mu3h_sch_bw_info *sch_bw,
- struct mu3h_sch_ep_info *sch_ep)
+static int check_sch_bw(struct mu3h_sch_ep_info *sch_ep)
{
+ struct mu3h_sch_bw_info *sch_bw = sch_ep->bw_info;
const u32 esit_boundary = get_esit_boundary(sch_ep);
const u32 bw_boundary = get_bw_boundary(sch_ep->speed);
u32 offset;
@@ -624,23 +636,26 @@ static int check_sch_bw(struct mu3h_sch_bw_info *sch_bw,
return load_ep_bw(sch_bw, sch_ep, true);
}
-static void destroy_sch_ep(struct usb_device *udev,
- struct mu3h_sch_bw_info *sch_bw, struct mu3h_sch_ep_info *sch_ep)
+static void destroy_sch_ep(struct xhci_hcd_mtk *mtk, struct usb_device *udev,
+ struct mu3h_sch_ep_info *sch_ep)
{
/* only release ep bw check passed by check_sch_bw() */
if (sch_ep->allocated)
- load_ep_bw(sch_bw, sch_ep, false);
+ load_ep_bw(sch_ep->bw_info, sch_ep, false);
if (sch_ep->sch_tt)
drop_tt(udev);
list_del(&sch_ep->endpoint);
+ hlist_del(&sch_ep->hentry);
kfree(sch_ep);
}
-static bool need_bw_sch(struct usb_host_endpoint *ep,
- enum usb_device_speed speed, int has_tt)
+static bool need_bw_sch(struct usb_device *udev,
+ struct usb_host_endpoint *ep)
{
+ bool has_tt = udev->tt && udev->tt->hub->parent;
+
/* only for periodic endpoints */
if (usb_endpoint_xfer_control(&ep->desc)
|| usb_endpoint_xfer_bulk(&ep->desc))
@@ -651,7 +666,7 @@ static bool need_bw_sch(struct usb_host_endpoint *ep,
* a TT are also ignored, root-hub will schedule them directly,
* but need set @bpkts field of endpoint context to 1.
*/
- if (is_fs_or_ls(speed) && !has_tt)
+ if (is_fs_or_ls(udev->speed) && !has_tt)
return false;
/* skip endpoint with zero maxpkt */
@@ -666,7 +681,6 @@ int xhci_mtk_sch_init(struct xhci_hcd_mtk *mtk)
struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
struct mu3h_sch_bw_info *sch_array;
int num_usb_bus;
- int i;
/* ss IN and OUT are separated */
num_usb_bus = xhci->usb3_rhub.num_ports * 2 + xhci->usb2_rhub.num_ports;
@@ -675,12 +689,10 @@ int xhci_mtk_sch_init(struct xhci_hcd_mtk *mtk)
if (sch_array == NULL)
return -ENOMEM;
- for (i = 0; i < num_usb_bus; i++)
- INIT_LIST_HEAD(&sch_array[i].bw_ep_list);
-
mtk->sch_array = sch_array;
INIT_LIST_HEAD(&mtk->bw_ep_chk_list);
+ hash_init(mtk->sch_ep_hash);
return 0;
}
@@ -704,9 +716,7 @@ static int add_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
ep_index = xhci_get_endpoint_index(&ep->desc);
ep_ctx = xhci_get_ep_ctx(xhci, virt_dev->in_ctx, ep_index);
- xhci_dbg(xhci, "%s %s\n", __func__, decode_ep(ep, udev->speed));
-
- if (!need_bw_sch(ep, udev->speed, !!virt_dev->tt_info)) {
+ if (!need_bw_sch(udev, ep)) {
/*
* set @bpkts to 1 if it is LS or FS periodic endpoint, and its
* device does not connected through an external HS hub
@@ -718,13 +728,16 @@ static int add_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
return 0;
}
- sch_ep = create_sch_ep(udev, ep, ep_ctx);
+ xhci_dbg(xhci, "%s %s\n", __func__, decode_ep(ep, udev->speed));
+
+ sch_ep = create_sch_ep(mtk, udev, ep, ep_ctx);
if (IS_ERR_OR_NULL(sch_ep))
return -ENOMEM;
setup_sch_info(ep_ctx, sch_ep);
list_add_tail(&sch_ep->endpoint, &mtk->bw_ep_chk_list);
+ hash_add(mtk->sch_ep_hash, &sch_ep->hentry, (unsigned long)ep);
return 0;
}
@@ -734,22 +747,18 @@ static void drop_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
{
struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
- struct xhci_virt_device *virt_dev;
- struct mu3h_sch_bw_info *sch_bw;
- struct mu3h_sch_ep_info *sch_ep, *tmp;
-
- virt_dev = xhci->devs[udev->slot_id];
-
- xhci_dbg(xhci, "%s %s\n", __func__, decode_ep(ep, udev->speed));
+ struct mu3h_sch_ep_info *sch_ep;
+ struct hlist_node *hn;
- if (!need_bw_sch(ep, udev->speed, !!virt_dev->tt_info))
+ if (!need_bw_sch(udev, ep))
return;
- sch_bw = get_bw_info(mtk, udev, ep);
+ xhci_err(xhci, "%s %s\n", __func__, decode_ep(ep, udev->speed));
- list_for_each_entry_safe(sch_ep, tmp, &sch_bw->bw_ep_list, endpoint) {
+ hash_for_each_possible_safe(mtk->sch_ep_hash, sch_ep,
+ hn, hentry, (unsigned long)ep) {
if (sch_ep->ep == ep) {
- destroy_sch_ep(udev, sch_bw, sch_ep);
+ destroy_sch_ep(mtk, udev, sch_ep);
break;
}
}
@@ -760,30 +769,22 @@ int xhci_mtk_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
struct xhci_virt_device *virt_dev = xhci->devs[udev->slot_id];
- struct mu3h_sch_bw_info *sch_bw;
- struct mu3h_sch_ep_info *sch_ep, *tmp;
+ struct mu3h_sch_ep_info *sch_ep;
int ret;
xhci_dbg(xhci, "%s() udev %s\n", __func__, dev_name(&udev->dev));
list_for_each_entry(sch_ep, &mtk->bw_ep_chk_list, endpoint) {
- sch_bw = get_bw_info(mtk, udev, sch_ep->ep);
+ struct xhci_ep_ctx *ep_ctx;
+ struct usb_host_endpoint *ep = sch_ep->ep;
+ unsigned int ep_index = xhci_get_endpoint_index(&ep->desc);
- ret = check_sch_bw(sch_bw, sch_ep);
+ ret = check_sch_bw(sch_ep);
if (ret) {
xhci_err(xhci, "Not enough bandwidth! (%s)\n",
sch_error_string(-ret));
return -ENOSPC;
}
- }
-
- list_for_each_entry_safe(sch_ep, tmp, &mtk->bw_ep_chk_list, endpoint) {
- struct xhci_ep_ctx *ep_ctx;
- struct usb_host_endpoint *ep = sch_ep->ep;
- unsigned int ep_index = xhci_get_endpoint_index(&ep->desc);
-
- sch_bw = get_bw_info(mtk, udev, ep);
- list_move_tail(&sch_ep->endpoint, &sch_bw->bw_ep_list);
ep_ctx = xhci_get_ep_ctx(xhci, virt_dev->in_ctx, ep_index);
ep_ctx->reserved[0] = cpu_to_le32(EP_BPKTS(sch_ep->pkts)
@@ -797,22 +798,23 @@ int xhci_mtk_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
sch_ep->offset, sch_ep->repeat);
}
- return xhci_check_bandwidth(hcd, udev);
+ ret = xhci_check_bandwidth(hcd, udev);
+ if (!ret)
+ INIT_LIST_HEAD(&mtk->bw_ep_chk_list);
+
+ return ret;
}
void xhci_mtk_reset_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
{
struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
- struct mu3h_sch_bw_info *sch_bw;
struct mu3h_sch_ep_info *sch_ep, *tmp;
xhci_dbg(xhci, "%s() udev %s\n", __func__, dev_name(&udev->dev));
- list_for_each_entry_safe(sch_ep, tmp, &mtk->bw_ep_chk_list, endpoint) {
- sch_bw = get_bw_info(mtk, udev, sch_ep->ep);
- destroy_sch_ep(udev, sch_bw, sch_ep);
- }
+ list_for_each_entry_safe(sch_ep, tmp, &mtk->bw_ep_chk_list, endpoint)
+ destroy_sch_ep(mtk, udev, sch_ep);
xhci_reset_bandwidth(hcd, udev);
}
diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index eb05aaf6edbe..dc5e83f5c5cc 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -10,11 +10,15 @@
#define _XHCI_MTK_H_
#include <linux/clk.h>
+#include <linux/hashtable.h>
#include "xhci.h"
#define BULK_CLKS_NUM 5
+/* support at most 64 ep, use 32 size hash table */
+#define SCH_EP_HASH_BITS 5
+
/**
* To simplify scheduler algorithm, set a upper limit for ESIT,
* if a synchromous ep's ESIT is larger than @XHCI_MTK_MAX_ESIT,
@@ -36,14 +40,12 @@ struct mu3h_sch_tt {
* struct mu3h_sch_bw_info: schedule information for bandwidth domain
*
* @bus_bw: array to keep track of bandwidth already used at each uframes
- * @bw_ep_list: eps in the bandwidth domain
*
* treat a HS root port as a bandwidth domain, but treat a SS root port as
* two bandwidth domains, one for IN eps and another for OUT eps.
*/
struct mu3h_sch_bw_info {
u32 bus_bw[XHCI_MTK_MAX_ESIT];
- struct list_head bw_ep_list;
};
/**
@@ -54,8 +56,10 @@ struct mu3h_sch_bw_info {
* @num_budget_microframes: number of continuous uframes
* (@repeat==1) scheduled within the interval
* @bw_cost_per_microframe: bandwidth cost per microframe
+ * @hentry: hash table entry
* @endpoint: linked into bandwidth domain which it belongs to
* @tt_endpoint: linked into mu3h_sch_tt's list which it belongs to
+ * @bw_info: bandwidth domain which this endpoint belongs
* @sch_tt: mu3h_sch_tt linked into
* @ep_type: endpoint type
* @maxpkt: max packet size of endpoint
@@ -84,7 +88,9 @@ struct mu3h_sch_ep_info {
u32 num_budget_microframes;
u32 bw_cost_per_microframe;
struct list_head endpoint;
+ struct hlist_node hentry;
struct list_head tt_endpoint;
+ struct mu3h_sch_bw_info *bw_info;
struct mu3h_sch_tt *sch_tt;
u32 ep_type;
u32 maxpkt;
@@ -137,6 +143,7 @@ struct xhci_hcd_mtk {
struct usb_hcd *hcd;
struct mu3h_sch_bw_info *sch_array;
struct list_head bw_ep_chk_list;
+ DECLARE_HASHTABLE(sch_ep_hash, SCH_EP_HASH_BITS);
struct mu3c_ippc_regs __iomem *ippc_regs;
int num_u2_ports;
int num_u3_ports;
--
2.18.0
On Thu, Aug 26, 2021 at 10:51:39AM +0800, Chunfeng Yun wrote:
> As discussed in following patch:
> https://patchwork.kernel.org/patch/12420339
>
> No need calculate number of uframes again, but should use value
> form check_sch_tt(), if we plan to remove extra CS, also can do
> it in check_sch_tt(). So revert this patch, and prepare to send
> new patch for it.
>
> This reverts commit 548011957d1d72e0b662300c8b32b81d593b796e.
>
> Cc: Ikjoon Jang <[email protected]>
> Signed-off-by: Chunfeng Yun <[email protected]>
> ---
> v2: no changes
This series does not apply to my tree at all now, can you please rebase
and resend?
thanks,
greg k-h
Hi Chunfeng,
On Thu, Aug 26, 2021 at 10:52 AM Chunfeng Yun <[email protected]> wrote:
>
> Use @bw_budget_table[] to update fs bus bandwidth due to
> not all microframes consume @bw_cost_per_microframe, see
> setup_sch_info().
>
> Signed-off-by: Chunfeng Yun <[email protected]>
> ---
> v2: new patch, move from another series
> ---
> drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
> index cffcaf4dfa9f..83abd28269ca 100644
> --- a/drivers/usb/host/xhci-mtk-sch.c
> +++ b/drivers/usb/host/xhci-mtk-sch.c
> @@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct mu3h_sch_ep_info *sch_ep, int offset)
> * Compared with hs bus, no matter what ep type,
> * the hub will always delay one uframe to send data
> */
> - for (j = 0; j < sch_ep->cs_count; j++) {
> - tmp = tt->fs_bus_bw[base + j] + sch_ep->bw_cost_per_microframe;
> + for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> + tmp = tt->fs_bus_bw[base + j] + sch_ep->bw_budget_table[j];
I'm worrying about this case with two endpoints,
* EP1OUT: isochronous, maxpacket=192: bw_budget_table[] = { 188, 188, 0, ... }
* EP2IN: interrupt, maxpacket=64: bw_budget_table[] = { 0, 0, 64, 64, ... }
(Is this correct bw_budget_table contents for those eps?)
I'm not sure if it's okay for those two endpoints to be allocated
on the same u-frame slot.
Can you please check if this is okay for xhci-mtk?
(I feel like I already asked the same questions many times.)
> if (tmp > FS_PAYLOAD_MAX)
> return -ESCH_BW_OVERFLOW;
> }
> @@ -534,21 +534,18 @@ static void update_sch_tt(struct mu3h_sch_ep_info *sch_ep, bool used)
> {
> struct mu3h_sch_tt *tt = sch_ep->sch_tt;
> u32 base, num_esit;
> - int bw_updated;
> int i, j;
>
> num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
>
> - if (used)
> - bw_updated = sch_ep->bw_cost_per_microframe;
> - else
> - bw_updated = -sch_ep->bw_cost_per_microframe;
> -
> for (i = 0; i < num_esit; i++) {
> base = sch_ep->offset + i * sch_ep->esit;
>
> - for (j = 0; j < sch_ep->cs_count; j++)
> - tt->fs_bus_bw[base + j] += bw_updated;
> + for (j = 0; j < sch_ep->num_budget_microframes; j++)
> + if (used)
> + tt->fs_bus_bw[base + j] += sch_ep->bw_budget_table[j];
> + else
> + tt->fs_bus_bw[base + j] -= sch_ep->bw_budget_table[j];
> }
>
> if (used)
> --
> 2.18.0
>
Hi Greg,
On Thu, 2021-08-26 at 13:41 +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 26, 2021 at 10:51:39AM +0800, Chunfeng Yun wrote:
> > As discussed in following patch:
> > https://patchwork.kernel.org/patch/12420339
> >
> > No need calculate number of uframes again, but should use value
> > form check_sch_tt(), if we plan to remove extra CS, also can do
> > it in check_sch_tt(). So revert this patch, and prepare to send
> > new patch for it.
> >
> > This reverts commit 548011957d1d72e0b662300c8b32b81d593b796e.
> >
> > Cc: Ikjoon Jang <[email protected]>
> > Signed-off-by: Chunfeng Yun <[email protected]>
> > ---
> > v2: no changes
>
> This series does not apply to my tree at all now, can you please
> rebase
> and resend?
Very sorry, I send out two series [1][2] for xhci-mtk, but don't take
care of conflicts, suppose that series [1] will be applied firstly, due
to one binding patch [3] of series [2] is not acked/reviewed by Rob (I
think only need modify some misleading commit message).
anyway, I find that only one patch [4] of series [1] is not applied, so
I'll fix conficts and resend it based on usb-testing branch.
Sorry again.
[1]: Series = [next,v2,1/6] Revert "usb: xhci-mtk: relax TT periodic
bandwidth allocation"
https://patchwork.kernel.org/project/linux-mediatek/list/?series=537471
[2]: Series = [RESEND,1/9] dt-bindings: usb: mtk-xhci: add optional
property to disable usb2 ports
https://patchwork.kernel.org/project/linux-mediatek/list/?series=532595
[3]:
https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/
[RESEND,2/9] dt-bindings: usb: mtk-xhci: add compatible for mt8195
[4]:
https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/
[next,v2,6/6] usb: xhci-mtk: allow bandwidth table rollover
>
> thanks,
>
> greg k-h
On Thu, 2021-08-26 at 19:54 +0800, Ikjoon Jang wrote:
> Hi Chunfeng,
>
> On Thu, Aug 26, 2021 at 10:52 AM Chunfeng Yun <
> [email protected]> wrote:
> >
> > Use @bw_budget_table[] to update fs bus bandwidth due to
> > not all microframes consume @bw_cost_per_microframe, see
> > setup_sch_info().
> >
> > Signed-off-by: Chunfeng Yun <[email protected]>
> > ---
> > v2: new patch, move from another series
> > ---
> > drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
> > 1 file changed, 7 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-mtk-sch.c
> > b/drivers/usb/host/xhci-mtk-sch.c
> > index cffcaf4dfa9f..83abd28269ca 100644
> > --- a/drivers/usb/host/xhci-mtk-sch.c
> > +++ b/drivers/usb/host/xhci-mtk-sch.c
> > @@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct
> > mu3h_sch_ep_info *sch_ep, int offset)
> > * Compared with hs bus, no matter what ep type,
> > * the hub will always delay one uframe to send
> > data
> > */
> > - for (j = 0; j < sch_ep->cs_count; j++) {
> > - tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > >bw_cost_per_microframe;
> > + for (j = 0; j < sch_ep->num_budget_microframes;
> > j++) {
> > + tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > >bw_budget_table[j];
>
> I'm worrying about this case with two endpoints,
> * EP1OUT: isochronous, maxpacket=192: bw_budget_table[] = { 188, 188,
> 0, ... }
> * EP2IN: interrupt, maxpacket=64: bw_budget_table[] = { 0, 0, 64, 64,
> ... }
> (Is this correct bw_budget_table contents for those eps?)
Yes, ep1out isoc use two uframe, ep2in intr use a extra cs;
>
> I'm not sure if it's okay for those two endpoints to be allocated
> on the same u-frame slot.
> Can you please check if this is okay for xhci-mtk?
Already test it this afternoon, can transfer data rightly on our dvt
env.
> (I feel like I already asked the same questions many times.)
Yes, as said before, prefer to use bw_budget_table[], if there is
issue, we can fix it by building this table.
Thanks
>
>
> > if (tmp > FS_PAYLOAD_MAX)
> > return -ESCH_BW_OVERFLOW;
> > }
> > @@ -534,21 +534,18 @@ static void update_sch_tt(struct
> > mu3h_sch_ep_info *sch_ep, bool used)
> > {
> > struct mu3h_sch_tt *tt = sch_ep->sch_tt;
> > u32 base, num_esit;
> > - int bw_updated;
> > int i, j;
> >
> > num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> >
> > - if (used)
> > - bw_updated = sch_ep->bw_cost_per_microframe;
> > - else
> > - bw_updated = -sch_ep->bw_cost_per_microframe;
> > -
> > for (i = 0; i < num_esit; i++) {
> > base = sch_ep->offset + i * sch_ep->esit;
> >
> > - for (j = 0; j < sch_ep->cs_count; j++)
> > - tt->fs_bus_bw[base + j] += bw_updated;
> > + for (j = 0; j < sch_ep->num_budget_microframes;
> > j++)
> > + if (used)
> > + tt->fs_bus_bw[base + j] += sch_ep-
> > >bw_budget_table[j];
> > + else
> > + tt->fs_bus_bw[base + j] -= sch_ep-
> > >bw_budget_table[j];
> > }
> >
> > if (used)
> > --
> > 2.18.0
> >
On Fri, Aug 27, 2021 at 2:49 PM Chunfeng Yun (云春峰)
<[email protected]> wrote:
>
> On Thu, 2021-08-26 at 19:54 +0800, Ikjoon Jang wrote:
> > Hi Chunfeng,
> >
> > On Thu, Aug 26, 2021 at 10:52 AM Chunfeng Yun <
> > [email protected]> wrote:
> > >
> > > Use @bw_budget_table[] to update fs bus bandwidth due to
> > > not all microframes consume @bw_cost_per_microframe, see
> > > setup_sch_info().
> > >
> > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > ---
> > > v2: new patch, move from another series
> > > ---
> > > drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
> > > 1 file changed, 7 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/xhci-mtk-sch.c
> > > b/drivers/usb/host/xhci-mtk-sch.c
> > > index cffcaf4dfa9f..83abd28269ca 100644
> > > --- a/drivers/usb/host/xhci-mtk-sch.c
> > > +++ b/drivers/usb/host/xhci-mtk-sch.c
> > > @@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct
> > > mu3h_sch_ep_info *sch_ep, int offset)
> > > * Compared with hs bus, no matter what ep type,
> > > * the hub will always delay one uframe to send
> > > data
> > > */
> > > - for (j = 0; j < sch_ep->cs_count; j++) {
> > > - tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > > >bw_cost_per_microframe;
> > > + for (j = 0; j < sch_ep->num_budget_microframes;
> > > j++) {
> > > + tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > > >bw_budget_table[j];
> >
> > I'm worrying about this case with two endpoints,
> > * EP1OUT: isochronous, maxpacket=192: bw_budget_table[] = { 188, 188,
> > 0, ... }
> > * EP2IN: interrupt, maxpacket=64: bw_budget_table[] = { 0, 0, 64, 64,
> > ... }
> > (Is this correct bw_budget_table contents for those eps?)
> Yes, ep1out isoc use two uframe, ep2in intr use a extra cs;
> >
> > I'm not sure if it's okay for those two endpoints to be allocated
> > on the same u-frame slot.
> > Can you please check if this is okay for xhci-mtk?
> Already test it this afternoon, can transfer data rightly on our dvt
> env.
>
> > (I feel like I already asked the same questions many times.)
> Yes, as said before, prefer to use bw_budget_table[], if there is
> issue, we can fix it by building this table.
So do you mean such an allocation shouldn't be a problem by IP design?
This patch starts to allow such an allocation (again).
But i remember my earlier tests showed that when those two eps
in the above example are allocated on the same u-frame slot,
xhci-mtk puts "SSPLIT for EP2" between
"SSPLIT-start and SSPLIT-end for EP1OUT transaction",
which is a spec violation. Hub will generate bit stuffing errors on the
full-speed bus.
>
> Thanks
> >
> >
> > > if (tmp > FS_PAYLOAD_MAX)
> > > return -ESCH_BW_OVERFLOW;
> > > }
> > > @@ -534,21 +534,18 @@ static void update_sch_tt(struct
> > > mu3h_sch_ep_info *sch_ep, bool used)
> > > {
> > > struct mu3h_sch_tt *tt = sch_ep->sch_tt;
> > > u32 base, num_esit;
> > > - int bw_updated;
> > > int i, j;
> > >
> > > num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> > >
> > > - if (used)
> > > - bw_updated = sch_ep->bw_cost_per_microframe;
> > > - else
> > > - bw_updated = -sch_ep->bw_cost_per_microframe;
> > > -
> > > for (i = 0; i < num_esit; i++) {
> > > base = sch_ep->offset + i * sch_ep->esit;
> > >
> > > - for (j = 0; j < sch_ep->cs_count; j++)
> > > - tt->fs_bus_bw[base + j] += bw_updated;
> > > + for (j = 0; j < sch_ep->num_budget_microframes;
> > > j++)
> > > + if (used)
> > > + tt->fs_bus_bw[base + j] += sch_ep-
> > > >bw_budget_table[j];
> > > + else
> > > + tt->fs_bus_bw[base + j] -= sch_ep-
> > > >bw_budget_table[j];
> > > }
> > >
> > > if (used)
> > > --
> > > 2.18.0
> > >
Hi Greg,
On Thu, 2021-08-26 at 13:41 +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 26, 2021 at 10:51:39AM +0800, Chunfeng Yun wrote:
> > As discussed in following patch:
> > https://patchwork.kernel.org/patch/12420339
> >
> > No need calculate number of uframes again, but should use value
> > form check_sch_tt(), if we plan to remove extra CS, also can do
> > it in check_sch_tt(). So revert this patch, and prepare to send
> > new patch for it.
> >
> > This reverts commit 548011957d1d72e0b662300c8b32b81d593b796e.
> >
> > Cc: Ikjoon Jang <[email protected]>
> > Signed-off-by: Chunfeng Yun <[email protected]>
> > ---
> > v2: no changes
>
> This series does not apply to my tree at all now, can you please
> rebase
> and resend?
Very sorry, I send two series [1][2] for xhci-mtk, but don't take care
of conficts:
I suppose you will apply [1] firstly, due to [3] is not acked or
reviewed by Rob (I think, only need modify commit message).
Anyway, only one patch [4] is not in usb-testing branch due to
conflicts, I'll fix it and send out, sorry again.
[1]. Series = [next,v2,1/6] Revert "usb: xhci-mtk: relax TT periodic
bandwidth allocation"
https://patchwork.kernel.org/project/linux-mediatek/list/?series=537471
[2]. Series = [RESEND,1/9] dt-bindings: usb: mtk-xhci: add optional
property to disable usb2 ports
https://patchwork.kernel.org/project/linux-mediatek/list/?series=532595
[3]:
https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/
[RESEND,2/9] dt-bindings: usb: mtk-xhci: add compatible for mt8195
[4]:
https://patchwork.kernel.org/project/linux-mediatek/patch/[email protected]/
[next,v2,6/6] usb: xhci-mtk: allow bandwidth table rollover
>
> thanks,
>
> greg k-h
On Fri, 2021-08-27 at 17:14 +0800, Ikjoon Jang wrote:
> On Fri, Aug 27, 2021 at 2:49 PM Chunfeng Yun (云春峰)
> <[email protected]> wrote:
> >
> > On Thu, 2021-08-26 at 19:54 +0800, Ikjoon Jang wrote:
> > > Hi Chunfeng,
> > >
> > > On Thu, Aug 26, 2021 at 10:52 AM Chunfeng Yun <
> > > [email protected]> wrote:
> > > >
> > > > Use @bw_budget_table[] to update fs bus bandwidth due to
> > > > not all microframes consume @bw_cost_per_microframe, see
> > > > setup_sch_info().
> > > >
> > > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > > ---
> > > > v2: new patch, move from another series
> > > > ---
> > > > drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
> > > > 1 file changed, 7 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/host/xhci-mtk-sch.c
> > > > b/drivers/usb/host/xhci-mtk-sch.c
> > > > index cffcaf4dfa9f..83abd28269ca 100644
> > > > --- a/drivers/usb/host/xhci-mtk-sch.c
> > > > +++ b/drivers/usb/host/xhci-mtk-sch.c
> > > > @@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct
> > > > mu3h_sch_ep_info *sch_ep, int offset)
> > > > * Compared with hs bus, no matter what ep
> > > > type,
> > > > * the hub will always delay one uframe to send
> > > > data
> > > > */
> > > > - for (j = 0; j < sch_ep->cs_count; j++) {
> > > > - tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > > > > bw_cost_per_microframe;
> > > >
> > > > + for (j = 0; j < sch_ep->num_budget_microframes;
> > > > j++) {
> > > > + tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > > > > bw_budget_table[j];
> > >
> > > I'm worrying about this case with two endpoints,
> > > * EP1OUT: isochronous, maxpacket=192: bw_budget_table[] = { 188,
> > > 188,
> > > 0, ... }
> > > * EP2IN: interrupt, maxpacket=64: bw_budget_table[] = { 0, 0, 64,
> > > 64,
> > > ... }
> > > (Is this correct bw_budget_table contents for those eps?)
> >
> > Yes, ep1out isoc use two uframe, ep2in intr use a extra cs;
> > >
> > > I'm not sure if it's okay for those two endpoints to be allocated
> > > on the same u-frame slot.
> > > Can you please check if this is okay for xhci-mtk?
> >
> > Already test it this afternoon, can transfer data rightly on our
> > dvt
> > env.
> >
> > > (I feel like I already asked the same questions many times.)
> >
> > Yes, as said before, prefer to use bw_budget_table[], if there is
> > issue, we can fix it by building this table.
>
> So do you mean such an allocation shouldn't be a problem by IP
> design?
Yes, at least on our dvt platform
>
> This patch starts to allow such an allocation (again).
> But i remember my earlier tests showed that when those two eps
> in the above example are allocated on the same u-frame slot,
> xhci-mtk puts "SSPLIT for EP2" between
> "SSPLIT-start and SSPLIT-end for EP1OUT transaction",
> which is a spec violation.
Which section in usb2.0 spec?
> Hub will generate bit stuffing errors on the
> full-speed bus.
which platform?
>
>
> >
> > Thanks
> > >
> > >
> > > > if (tmp > FS_PAYLOAD_MAX)
> > > > return -ESCH_BW_OVERFLOW;
> > > > }
> > > > @@ -534,21 +534,18 @@ static void update_sch_tt(struct
> > > > mu3h_sch_ep_info *sch_ep, bool used)
> > > > {
> > > > struct mu3h_sch_tt *tt = sch_ep->sch_tt;
> > > > u32 base, num_esit;
> > > > - int bw_updated;
> > > > int i, j;
> > > >
> > > > num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> > > >
> > > > - if (used)
> > > > - bw_updated = sch_ep->bw_cost_per_microframe;
> > > > - else
> > > > - bw_updated = -sch_ep->bw_cost_per_microframe;
> > > > -
> > > > for (i = 0; i < num_esit; i++) {
> > > > base = sch_ep->offset + i * sch_ep->esit;
> > > >
> > > > - for (j = 0; j < sch_ep->cs_count; j++)
> > > > - tt->fs_bus_bw[base + j] += bw_updated;
> > > > + for (j = 0; j < sch_ep->num_budget_microframes;
> > > > j++)
> > > > + if (used)
> > > > + tt->fs_bus_bw[base + j] +=
> > > > sch_ep-
> > > > > bw_budget_table[j];
> > > >
> > > > + else
> > > > + tt->fs_bus_bw[base + j] -=
> > > > sch_ep-
> > > > > bw_budget_table[j];
> > > >
> > > > }
> > > >
> > > > if (used)
> > > > --
> > > > 2.18.0
> > > >
On Fri, Aug 27, 2021 at 5:49 PM Chunfeng Yun (云春峰)
<[email protected]> wrote:
>
> On Fri, 2021-08-27 at 17:14 +0800, Ikjoon Jang wrote:
> > On Fri, Aug 27, 2021 at 2:49 PM Chunfeng Yun (云春峰)
> > <[email protected]> wrote:
> > >
> > > On Thu, 2021-08-26 at 19:54 +0800, Ikjoon Jang wrote:
> > > > Hi Chunfeng,
> > > >
> > > > On Thu, Aug 26, 2021 at 10:52 AM Chunfeng Yun <
> > > > [email protected]> wrote:
> > > > >
> > > > > Use @bw_budget_table[] to update fs bus bandwidth due to
> > > > > not all microframes consume @bw_cost_per_microframe, see
> > > > > setup_sch_info().
> > > > >
> > > > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > > > ---
> > > > > v2: new patch, move from another series
> > > > > ---
> > > > > drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
> > > > > 1 file changed, 7 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/drivers/usb/host/xhci-mtk-sch.c
> > > > > b/drivers/usb/host/xhci-mtk-sch.c
> > > > > index cffcaf4dfa9f..83abd28269ca 100644
> > > > > --- a/drivers/usb/host/xhci-mtk-sch.c
> > > > > +++ b/drivers/usb/host/xhci-mtk-sch.c
> > > > > @@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct
> > > > > mu3h_sch_ep_info *sch_ep, int offset)
> > > > > * Compared with hs bus, no matter what ep
> > > > > type,
> > > > > * the hub will always delay one uframe to send
> > > > > data
> > > > > */
> > > > > - for (j = 0; j < sch_ep->cs_count; j++) {
> > > > > - tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > > > > > bw_cost_per_microframe;
> > > > >
> > > > > + for (j = 0; j < sch_ep->num_budget_microframes;
> > > > > j++) {
> > > > > + tmp = tt->fs_bus_bw[base + j] + sch_ep-
> > > > > > bw_budget_table[j];
> > > >
> > > > I'm worrying about this case with two endpoints,
> > > > * EP1OUT: isochronous, maxpacket=192: bw_budget_table[] = { 188,
> > > > 188,
> > > > 0, ... }
> > > > * EP2IN: interrupt, maxpacket=64: bw_budget_table[] = { 0, 0, 64,
> > > > 64,
> > > > ... }
> > > > (Is this correct bw_budget_table contents for those eps?)
> > >
> > > Yes, ep1out isoc use two uframe, ep2in intr use a extra cs;
> > > >
> > > > I'm not sure if it's okay for those two endpoints to be allocated
> > > > on the same u-frame slot.
> > > > Can you please check if this is okay for xhci-mtk?
> > >
> > > Already test it this afternoon, can transfer data rightly on our
> > > dvt
> > > env.
> > >
> > > > (I feel like I already asked the same questions many times.)
> > >
> > > Yes, as said before, prefer to use bw_budget_table[], if there is
> > > issue, we can fix it by building this table.
> >
> > So do you mean such an allocation shouldn't be a problem by IP
> > design?
> Yes, at least on our dvt platform
Did you check that your side also has a similar allocation
(SSPLIT-all sits between SSPLIT-start ~ -end for another ep)?
My audio headset doesn't work properly with this scheme.
>
> >
> > This patch starts to allow such an allocation (again).
> > But i remember my earlier tests showed that when those two eps
> > in the above example are allocated on the same u-frame slot,
> > xhci-mtk puts "SSPLIT for EP2" between
> > "SSPLIT-start and SSPLIT-end for EP1OUT transaction",
> > which is a spec violation.
>
> Which section in usb2.0 spec?
I think that's just a basic rule - if software wants to send 192 bytes
through a full-speed bus, HC should send OUT/DATA 192 bytes
continuously without inserting any other packets during that 192 bytes.
and usb2 11.14.2 mentions that TT has separated
Start-Split and Complete-Split buffers
but not tracked each transaction per endpoint basis.
>
> > Hub will generate bit stuffing errors on the
> > full-speed bus.
> which platform?
I remember it was mt8173.
And for bit stuffing errors I mentioned in the earlier mail.
when I read the spec again, 11.21 mentions that bit stuffing error
is generated when _a microframe_ should be passed without
corresponding SSPLIT-mid/end. So this is not the case and also
I'm not sure what will happen on the full-speed bus, sorry.
In my case what I can be sure of is that the audio output was
broken with those allotments.
What is the xhci-mtk's policy when there are more than two EPs
marked as the same u-frame offset like in the above example?
>
> >
> >
> > >
> > > Thanks
> > > >
> > > >
> > > > > if (tmp > FS_PAYLOAD_MAX)
> > > > > return -ESCH_BW_OVERFLOW;
> > > > > }
> > > > > @@ -534,21 +534,18 @@ static void update_sch_tt(struct
> > > > > mu3h_sch_ep_info *sch_ep, bool used)
> > > > > {
> > > > > struct mu3h_sch_tt *tt = sch_ep->sch_tt;
> > > > > u32 base, num_esit;
> > > > > - int bw_updated;
> > > > > int i, j;
> > > > >
> > > > > num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> > > > >
> > > > > - if (used)
> > > > > - bw_updated = sch_ep->bw_cost_per_microframe;
> > > > > - else
> > > > > - bw_updated = -sch_ep->bw_cost_per_microframe;
> > > > > -
> > > > > for (i = 0; i < num_esit; i++) {
> > > > > base = sch_ep->offset + i * sch_ep->esit;
> > > > >
> > > > > - for (j = 0; j < sch_ep->cs_count; j++)
> > > > > - tt->fs_bus_bw[base + j] += bw_updated;
> > > > > + for (j = 0; j < sch_ep->num_budget_microframes;
> > > > > j++)
> > > > > + if (used)
> > > > > + tt->fs_bus_bw[base + j] +=
> > > > > sch_ep-
> > > > > > bw_budget_table[j];
> > > > >
> > > > > + else
> > > > > + tt->fs_bus_bw[base + j] -=
> > > > > sch_ep-
> > > > > > bw_budget_table[j];
> > > > >
> > > > > }
> > > > >
> > > > > if (used)
> > > > > --
> > > > > 2.18.0
> > > > >
On Mon, 2021-08-30 at 11:49 +0800, Ikjoon Jang wrote:
> On Fri, Aug 27, 2021 at 5:49 PM Chunfeng Yun (云春峰)
> <[email protected]> wrote:
> >
> > On Fri, 2021-08-27 at 17:14 +0800, Ikjoon Jang wrote:
> > > On Fri, Aug 27, 2021 at 2:49 PM Chunfeng Yun (云春峰)
> > > <[email protected]> wrote:
> > > >
> > > > On Thu, 2021-08-26 at 19:54 +0800, Ikjoon Jang wrote:
> > > > > Hi Chunfeng,
> > > > >
> > > > > On Thu, Aug 26, 2021 at 10:52 AM Chunfeng Yun <
> > > > > [email protected]> wrote:
> > > > > >
> > > > > > Use @bw_budget_table[] to update fs bus bandwidth due to
> > > > > > not all microframes consume @bw_cost_per_microframe, see
> > > > > > setup_sch_info().
> > > > > >
> > > > > > Signed-off-by: Chunfeng Yun <[email protected]>
> > > > > > ---
> > > > > > v2: new patch, move from another series
> > > > > > ---
> > > > > > drivers/usb/host/xhci-mtk-sch.c | 17 +++++++----------
> > > > > > 1 file changed, 7 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/usb/host/xhci-mtk-sch.c
> > > > > > b/drivers/usb/host/xhci-mtk-sch.c
> > > > > > index cffcaf4dfa9f..83abd28269ca 100644
> > > > > > --- a/drivers/usb/host/xhci-mtk-sch.c
> > > > > > +++ b/drivers/usb/host/xhci-mtk-sch.c
> > > > > > @@ -458,8 +458,8 @@ static int check_fs_bus_bw(struct
> > > > > > mu3h_sch_ep_info *sch_ep, int offset)
> > > > > > * Compared with hs bus, no matter what ep
> > > > > > type,
> > > > > > * the hub will always delay one uframe to
> > > > > > send
> > > > > > data
> > > > > > */
> > > > > > - for (j = 0; j < sch_ep->cs_count; j++) {
> > > > > > - tmp = tt->fs_bus_bw[base + j] +
> > > > > > sch_ep-
> > > > > > > bw_cost_per_microframe;
> > > > > >
> > > > > > + for (j = 0; j < sch_ep-
> > > > > > >num_budget_microframes;
> > > > > > j++) {
> > > > > > + tmp = tt->fs_bus_bw[base + j] +
> > > > > > sch_ep-
> > > > > > > bw_budget_table[j];
> > > > >
> > > > > I'm worrying about this case with two endpoints,
> > > > > * EP1OUT: isochronous, maxpacket=192: bw_budget_table[] = {
> > > > > 188,
> > > > > 188,
> > > > > 0, ... }
> > > > > * EP2IN: interrupt, maxpacket=64: bw_budget_table[] = { 0, 0,
> > > > > 64,
> > > > > 64,
> > > > > ... }
> > > > > (Is this correct bw_budget_table contents for those eps?)
> > > >
> > > > Yes, ep1out isoc use two uframe, ep2in intr use a extra cs;
> > > > >
> > > > > I'm not sure if it's okay for those two endpoints to be
> > > > > allocated
> > > > > on the same u-frame slot.
> > > > > Can you please check if this is okay for xhci-mtk?
> > > >
> > > > Already test it this afternoon, can transfer data rightly on
> > > > our
> > > > dvt
> > > > env.
> > > >
> > > > > (I feel like I already asked the same questions many times.)
> > > >
> > > > Yes, as said before, prefer to use bw_budget_table[], if there
> > > > is
> > > > issue, we can fix it by building this table.
> > >
> > > So do you mean such an allocation shouldn't be a problem by IP
> > > design?
> >
> > Yes, at least on our dvt platform
>
> Did you check that your side also has a similar allocation
> (SSPLIT-all sits between SSPLIT-start ~ -end for another ep)?
> My audio headset doesn't work properly with this scheme.
>
> >
> > >
> > > This patch starts to allow such an allocation (again).
> > > But i remember my earlier tests showed that when those two eps
> > > in the above example are allocated on the same u-frame slot,
> > > xhci-mtk puts "SSPLIT for EP2" between
> > > "SSPLIT-start and SSPLIT-end for EP1OUT transaction",
> > > which is a spec violation.
> >
> > Which section in usb2.0 spec?
>
> I think that's just a basic rule - if software wants to send 192
> bytes
> through a full-speed bus, HC should send OUT/DATA 192 bytes
> continuously without inserting any other packets during that 192
> bytes.
> and usb2 11.14.2 mentions that TT has separated
> Start-Split and Complete-Split buffers
> but not tracked each transaction per endpoint basis.
>
> >
> > > Hub will generate bit stuffing errors on the
> > > full-speed bus.
> >
> > which platform?
>
> I remember it was mt8173.
Does it happen on mt8192?
>
> And for bit stuffing errors I mentioned in the earlier mail.
> when I read the spec again, 11.21 mentions that bit stuffing error
> is generated when _a microframe_ should be passed without
> corresponding SSPLIT-mid/end. So this is not the case and also
> I'm not sure what will happen on the full-speed bus, sorry.
> In my case what I can be sure of is that the audio output was
> broken with those allotments.
>
> What is the xhci-mtk's policy when there are more than two EPs
> marked as the same u-frame offset like in the above example?
Seems no this limitation, an EP doesn't monopolize an u-frame
>
> >
> > >
> > >
> > > >
> > > > Thanks
> > > > >
> > > > >
> > > > > > if (tmp > FS_PAYLOAD_MAX)
> > > > > > return -ESCH_BW_OVERFLOW;
> > > > > > }
> > > > > > @@ -534,21 +534,18 @@ static void update_sch_tt(struct
> > > > > > mu3h_sch_ep_info *sch_ep, bool used)
> > > > > > {
> > > > > > struct mu3h_sch_tt *tt = sch_ep->sch_tt;
> > > > > > u32 base, num_esit;
> > > > > > - int bw_updated;
> > > > > > int i, j;
> > > > > >
> > > > > > num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> > > > > >
> > > > > > - if (used)
> > > > > > - bw_updated = sch_ep-
> > > > > > >bw_cost_per_microframe;
> > > > > > - else
> > > > > > - bw_updated = -sch_ep-
> > > > > > >bw_cost_per_microframe;
> > > > > > -
> > > > > > for (i = 0; i < num_esit; i++) {
> > > > > > base = sch_ep->offset + i * sch_ep->esit;
> > > > > >
> > > > > > - for (j = 0; j < sch_ep->cs_count; j++)
> > > > > > - tt->fs_bus_bw[base + j] +=
> > > > > > bw_updated;
> > > > > > + for (j = 0; j < sch_ep-
> > > > > > >num_budget_microframes;
> > > > > > j++)
> > > > > > + if (used)
> > > > > > + tt->fs_bus_bw[base + j] +=
> > > > > > sch_ep-
> > > > > > > bw_budget_table[j];
> > > > > >
> > > > > > + else
> > > > > > + tt->fs_bus_bw[base + j] -=
> > > > > > sch_ep-
> > > > > > > bw_budget_table[j];
> > > > > >
> > > > > > }
> > > > > >
> > > > > > if (used)
> > > > > > --
> > > > > > 2.18.0
> > > > > >