From: Zhanyong Wang <[email protected]>
After inserted the usb type-c 3.5mm dongle with headset, dmesg showed:
usb 1-1.1: new full-speed USB device number 5 using xhci-mtk
usb 1-1.1: New USB device found, idVendor=05ac, idProduct=110a, bcdDevice=26.11
usb 1-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
usb 1-1.1: Product: USB-C to 3.5mm Headphone Jack Adapter
usb 1-1.1: Manufacturer: Apple, Inc.
usb 1-1.1: SerialNumber: DWH915501TFJKLTAM
xhci-mtk 11200000.xhci: Not enough bandwidth!
usb 1-1.1: can't set config #2, error -28
improve low-speed/full-speed INT/ISOC bandwidth scheduling with USB
muli-TT.
Signed-off-by: Yaqii Wu <[email protected]>
Signed-off-by: Chunfeng Yun <[email protected]>
---
v2~v3: no changes
---
drivers/usb/host/xhci-mtk-sch.c | 91 ++++++++++++++++++++++++++++-----
drivers/usb/host/xhci-mtk.h | 8 ++-
2 files changed, 84 insertions(+), 15 deletions(-)
mode change 100644 => 100755 drivers/usb/host/xhci-mtk-sch.c
diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
old mode 100644
new mode 100755
index 45c54d56ecbd..94292b9bbc63
--- a/drivers/usb/host/xhci-mtk-sch.c
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -383,7 +383,9 @@ static int check_sch_tt(struct usb_device *udev,
u32 fs_budget_start;
u32 start_ss, last_ss;
u32 start_cs, last_cs;
- int i;
+ u32 num_esit, base;
+ int i, j;
+ u32 tmp;
start_ss = offset % 8;
fs_budget_start = (start_ss + 1) % 8;
@@ -398,10 +400,13 @@ static int check_sch_tt(struct usb_device *udev,
if (!(start_ss == 7 || last_ss < 6))
return -ERANGE;
- for (i = 0; i < sch_ep->cs_count; i++)
- if (test_bit(offset + i, tt->split_bit_map))
+ for (i = 0; i < sch_ep->cs_count; i++) {
+ if (test_bit(offset + i, tt->ss_bit_map))
return -ERANGE;
+ if (test_bit(offset + i, tt->cs_bit_map))
+ return -ERANGE;
+ }
} else {
u32 cs_count = DIV_ROUND_UP(sch_ep->maxpkt, FS_PAYLOAD_MAX);
@@ -428,8 +433,10 @@ static int check_sch_tt(struct usb_device *udev,
if (cs_count > 7)
cs_count = 7; /* HW limit */
- for (i = 0; i < cs_count + 2; i++) {
- if (test_bit(offset + i, tt->split_bit_map))
+ if (test_bit(offset, tt->ss_bit_map))
+ return -ERANGE;
+ for (i = 0; i < cs_count; i++) {
+ if (test_bit(offset + 2 + i, tt->cs_bit_map))
return -ERANGE;
}
@@ -445,11 +452,22 @@ static int check_sch_tt(struct usb_device *udev,
sch_ep->num_budget_microframes = sch_ep->esit;
}
+ num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
+ for (i = 0; i < num_esit; i++) {
+ base = sch_ep->offset + i * sch_ep->esit;
+ for (j = 0; j < sch_ep->num_budget_microframes; j++) {
+ tmp = tt->fs_bus_bw[base + j]
+ + sch_ep->bw_cost_per_microframe;
+ if (tmp > FS_PAYLOAD_MAX)
+ return -ERANGE;
+ }
+ }
+
return 0;
}
static void update_sch_tt(struct usb_device *udev,
- struct mu3h_sch_ep_info *sch_ep)
+ struct mu3h_sch_ep_info *sch_ep, bool used)
{
struct mu3h_sch_tt *tt = sch_ep->sch_tt;
u32 base, num_esit;
@@ -458,11 +476,52 @@ static void update_sch_tt(struct usb_device *udev,
num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
for (i = 0; i < num_esit; i++) {
base = sch_ep->offset + i * sch_ep->esit;
- for (j = 0; j < sch_ep->num_budget_microframes; j++)
- set_bit(base + j, tt->split_bit_map);
+ for (j = 0; j < sch_ep->num_budget_microframes; j++) {
+ if (used)
+ set_bit(base + j, tt->split_bit_map);
+ else
+ clear_bit(base + j, tt->split_bit_map);
+ }
+
+ if (sch_ep->ep_type == ISOC_OUT_EP) {
+ for (j = 0; j < sch_ep->num_budget_microframes; j++) {
+ if (used) {
+ set_bit(base + j, tt->ss_bit_map);
+ set_bit(base + j, tt->cs_bit_map);
+ tt->fs_bus_bw[base + j] +=
+ sch_ep->bw_cost_per_microframe;
+ } else {
+ clear_bit(base + j, tt->ss_bit_map);
+ clear_bit(base + j, tt->cs_bit_map);
+ tt->fs_bus_bw[base + j] -=
+ sch_ep->bw_cost_per_microframe;
+ }
+ }
+ } else {
+ if (used)
+ set_bit(base, tt->ss_bit_map);
+ else
+ clear_bit(base, tt->ss_bit_map);
+
+ for (j = 0; j < sch_ep->cs_count; j++) {
+ if (used) {
+ set_bit(base + 2 + j, tt->cs_bit_map);
+
+ tt->fs_bus_bw[base + 2 + j] +=
+ sch_ep->bw_cost_per_microframe;
+ } else {
+ clear_bit(base + 2 + j, tt->cs_bit_map);
+ tt->fs_bus_bw[base + 2 + j] -=
+ sch_ep->bw_cost_per_microframe;
+ }
+ }
+ }
}
- list_add_tail(&sch_ep->tt_endpoint, &tt->ep_list);
+ if (used)
+ list_add_tail(&sch_ep->tt_endpoint, &tt->ep_list);
+ else
+ list_del(&sch_ep->tt_endpoint);
}
static int check_sch_bw(struct usb_device *udev,
@@ -470,6 +529,7 @@ static int check_sch_bw(struct usb_device *udev,
{
u32 offset;
u32 esit;
+ u32 boundary;
u32 min_bw;
u32 min_index;
u32 worst_bw;
@@ -487,10 +547,13 @@ static int check_sch_bw(struct usb_device *udev,
*/
min_bw = ~0;
min_index = 0;
+ boundary = esit;
min_cs_count = sch_ep->cs_count;
min_num_budget = sch_ep->num_budget_microframes;
for (offset = 0; offset < esit; offset++) {
if (is_fs_or_ls(udev->speed)) {
+ if (sch_ep->ep_type != ISOC_OUT_EP)
+ boundary = esit + 1;
ret = check_sch_tt(udev, sch_ep, offset);
if (ret)
continue;
@@ -498,7 +561,7 @@ static int check_sch_bw(struct usb_device *udev,
tt_offset_ok = true;
}
- if ((offset + sch_ep->num_budget_microframes) > sch_ep->esit)
+ if ((offset + sch_ep->num_budget_microframes) > boundary)
break;
worst_bw = get_max_bw(sch_bw, sch_ep, offset);
@@ -532,7 +595,7 @@ static int check_sch_bw(struct usb_device *udev,
if (!tt_offset_ok)
return -ERANGE;
- update_sch_tt(udev, sch_ep);
+ update_sch_tt(udev, sch_ep, 1);
}
/* update bus bandwidth info */
@@ -696,12 +759,12 @@ void xhci_mtk_drop_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
list_for_each_entry(sch_ep, &sch_bw->bw_ep_list, endpoint) {
if (sch_ep->ep == ep) {
- update_bus_bw(sch_bw, sch_ep, 0);
- list_del(&sch_ep->endpoint);
if (is_fs_or_ls(udev->speed)) {
- list_del(&sch_ep->tt_endpoint);
+ update_sch_tt(udev, sch_ep, 0);
drop_tt(udev);
}
+ update_bus_bw(sch_bw, sch_ep, 0);
+ list_del(&sch_ep->endpoint);
kfree(sch_ep);
break;
}
diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index a93cfe817904..323b281933b9 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -21,12 +21,18 @@
/**
* @split_bit_map: used to avoid split microframes overlay
+ * @ss_bit_map: used to avoid start split microframes overlay
+ * @cs_bit_map: used to avoid complete split microframes overlay
+ * @fs_bus_bw: array to keep track of bandwidth already used at full speed
* @ep_list: Endpoints using this TT
* @usb_tt: usb TT related
* @tt_port: TT port number
*/
struct mu3h_sch_tt {
DECLARE_BITMAP(split_bit_map, XHCI_MTK_MAX_ESIT);
+ DECLARE_BITMAP(ss_bit_map, XHCI_MTK_MAX_ESIT);
+ DECLARE_BITMAP(cs_bit_map, XHCI_MTK_MAX_ESIT + 1);
+ u32 fs_bus_bw[XHCI_MTK_MAX_ESIT];
struct list_head ep_list;
struct usb_tt *usb_tt;
int tt_port;
@@ -42,7 +48,7 @@ struct mu3h_sch_tt {
* two bandwidth domains, one for IN eps and another for OUT eps.
*/
struct mu3h_sch_bw_info {
- u32 bus_bw[XHCI_MTK_MAX_ESIT];
+ u32 bus_bw[XHCI_MTK_MAX_ESIT + 1];
struct list_head bw_ep_list;
};
--
2.18.0
On Tue, Dec 22, 2020 at 5:35 PM Chunfeng Yun <[email protected]> wrote:
>
> From: Zhanyong Wang <[email protected]>
>
> After inserted the usb type-c 3.5mm dongle with headset, dmesg showed:
> usb 1-1.1: new full-speed USB device number 5 using xhci-mtk
> usb 1-1.1: New USB device found, idVendor=05ac, idProduct=110a, bcdDevice=26.11
> usb 1-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> usb 1-1.1: Product: USB-C to 3.5mm Headphone Jack Adapter
> usb 1-1.1: Manufacturer: Apple, Inc.
> usb 1-1.1: SerialNumber: DWH915501TFJKLTAM
> xhci-mtk 11200000.xhci: Not enough bandwidth!
> usb 1-1.1: can't set config #2, error -28
>
> improve low-speed/full-speed INT/ISOC bandwidth scheduling with USB
> muli-TT.
>
> Signed-off-by: Yaqii Wu <[email protected]>
> Signed-off-by: Chunfeng Yun <[email protected]>
> ---
> v2~v3: no changes
> ---
> drivers/usb/host/xhci-mtk-sch.c | 91 ++++++++++++++++++++++++++++-----
> drivers/usb/host/xhci-mtk.h | 8 ++-
> 2 files changed, 84 insertions(+), 15 deletions(-)
> mode change 100644 => 100755 drivers/usb/host/xhci-mtk-sch.c
>
> diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
> old mode 100644
> new mode 100755
> index 45c54d56ecbd..94292b9bbc63
> --- a/drivers/usb/host/xhci-mtk-sch.c
> +++ b/drivers/usb/host/xhci-mtk-sch.c
> @@ -383,7 +383,9 @@ static int check_sch_tt(struct usb_device *udev,
> u32 fs_budget_start;
> u32 start_ss, last_ss;
> u32 start_cs, last_cs;
> - int i;
> + u32 num_esit, base;
> + int i, j;
> + u32 tmp;
>
> start_ss = offset % 8;
> fs_budget_start = (start_ss + 1) % 8;
> @@ -398,10 +400,13 @@ static int check_sch_tt(struct usb_device *udev,
> if (!(start_ss == 7 || last_ss < 6))
> return -ERANGE;
>
> - for (i = 0; i < sch_ep->cs_count; i++)
> - if (test_bit(offset + i, tt->split_bit_map))
> + for (i = 0; i < sch_ep->cs_count; i++) {
> + if (test_bit(offset + i, tt->ss_bit_map))
> return -ERANGE;
>
> + if (test_bit(offset + i, tt->cs_bit_map))
> + return -ERANGE;
> + }
Are there any reasons for doing this?
Why can only one split packet be scheduled in a u-frame for isochronous out?
This looks like overkill.
> } else {
> u32 cs_count = DIV_ROUND_UP(sch_ep->maxpkt, FS_PAYLOAD_MAX);
>
> @@ -428,8 +433,10 @@ static int check_sch_tt(struct usb_device *udev,
> if (cs_count > 7)
> cs_count = 7; /* HW limit */
>
> - for (i = 0; i < cs_count + 2; i++) {
> - if (test_bit(offset + i, tt->split_bit_map))
> + if (test_bit(offset, tt->ss_bit_map))
> + return -ERANGE;
> + for (i = 0; i < cs_count; i++) {
> + if (test_bit(offset + 2 + i, tt->cs_bit_map))
> return -ERANGE;
> }
>
same here. It would be much better to understand
if you can provide some counterexamples of schedule
that can happen when this bitmap checking logic is omitted.
> @@ -445,11 +452,22 @@ static int check_sch_tt(struct usb_device *udev,
> sch_ep->num_budget_microframes = sch_ep->esit;
> }
>
> + num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> + for (i = 0; i < num_esit; i++) {
> + base = sch_ep->offset + i * sch_ep->esit;
> + for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> + tmp = tt->fs_bus_bw[base + j]
> + + sch_ep->bw_cost_per_microframe;
> + if (tmp > FS_PAYLOAD_MAX)
> + return -ERANGE;
> + }
> + }
I guess this is enough to check the bandwidth limit of the
lower speed bus without a bitmap.
> +
> return 0;
> }
>
> static void update_sch_tt(struct usb_device *udev,
> - struct mu3h_sch_ep_info *sch_ep)
> + struct mu3h_sch_ep_info *sch_ep, bool used)
> {
> struct mu3h_sch_tt *tt = sch_ep->sch_tt;
> u32 base, num_esit;
> @@ -458,11 +476,52 @@ static void update_sch_tt(struct usb_device *udev,
> num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> for (i = 0; i < num_esit; i++) {
> base = sch_ep->offset + i * sch_ep->esit;
> - for (j = 0; j < sch_ep->num_budget_microframes; j++)
> - set_bit(base + j, tt->split_bit_map);
> + for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> + if (used)
> + set_bit(base + j, tt->split_bit_map);
> + else
> + clear_bit(base + j, tt->split_bit_map);
> + }
> +
> + if (sch_ep->ep_type == ISOC_OUT_EP) {
> + for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> + if (used) {
> + set_bit(base + j, tt->ss_bit_map);
> + set_bit(base + j, tt->cs_bit_map);
> + tt->fs_bus_bw[base + j] +=
> + sch_ep->bw_cost_per_microframe;
> + } else {
> + clear_bit(base + j, tt->ss_bit_map);
> + clear_bit(base + j, tt->cs_bit_map);
> + tt->fs_bus_bw[base + j] -=
> + sch_ep->bw_cost_per_microframe;
> + }
> + }
> + } else {
> + if (used)
> + set_bit(base, tt->ss_bit_map);
> + else
> + clear_bit(base, tt->ss_bit_map);
> +
> + for (j = 0; j < sch_ep->cs_count; j++) {
> + if (used) {
> + set_bit(base + 2 + j, tt->cs_bit_map);
> +
> + tt->fs_bus_bw[base + 2 + j] +=
> + sch_ep->bw_cost_per_microframe;
> + } else {
> + clear_bit(base + 2 + j, tt->cs_bit_map);
> + tt->fs_bus_bw[base + 2 + j] -=
> + sch_ep->bw_cost_per_microframe;
> + }
> + }
> + }
> }
>
> - list_add_tail(&sch_ep->tt_endpoint, &tt->ep_list);
> + if (used)
> + list_add_tail(&sch_ep->tt_endpoint, &tt->ep_list);
> + else
> + list_del(&sch_ep->tt_endpoint);
> }
>
> static int check_sch_bw(struct usb_device *udev,
> @@ -470,6 +529,7 @@ static int check_sch_bw(struct usb_device *udev,
> {
> u32 offset;
> u32 esit;
> + u32 boundary;
> u32 min_bw;
> u32 min_index;
> u32 worst_bw;
> @@ -487,10 +547,13 @@ static int check_sch_bw(struct usb_device *udev,
> */
> min_bw = ~0;
> min_index = 0;
> + boundary = esit;
> min_cs_count = sch_ep->cs_count;
> min_num_budget = sch_ep->num_budget_microframes;
> for (offset = 0; offset < esit; offset++) {
> if (is_fs_or_ls(udev->speed)) {
> + if (sch_ep->ep_type != ISOC_OUT_EP)
> + boundary = esit + 1;
> ret = check_sch_tt(udev, sch_ep, offset);
> if (ret)
> continue;
> @@ -498,7 +561,7 @@ static int check_sch_bw(struct usb_device *udev,
> tt_offset_ok = true;
> }
>
> - if ((offset + sch_ep->num_budget_microframes) > sch_ep->esit)
> + if ((offset + sch_ep->num_budget_microframes) > boundary)
> break;
>
> worst_bw = get_max_bw(sch_bw, sch_ep, offset);
> @@ -532,7 +595,7 @@ static int check_sch_bw(struct usb_device *udev,
> if (!tt_offset_ok)
> return -ERANGE;
>
> - update_sch_tt(udev, sch_ep);
> + update_sch_tt(udev, sch_ep, 1);
> }
>
> /* update bus bandwidth info */
> @@ -696,12 +759,12 @@ void xhci_mtk_drop_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
>
> list_for_each_entry(sch_ep, &sch_bw->bw_ep_list, endpoint) {
> if (sch_ep->ep == ep) {
> - update_bus_bw(sch_bw, sch_ep, 0);
> - list_del(&sch_ep->endpoint);
> if (is_fs_or_ls(udev->speed)) {
> - list_del(&sch_ep->tt_endpoint);
> + update_sch_tt(udev, sch_ep, 0);
> drop_tt(udev);
> }
> + update_bus_bw(sch_bw, sch_ep, 0);
> + list_del(&sch_ep->endpoint);
> kfree(sch_ep);
> break;
> }
> diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
> index a93cfe817904..323b281933b9 100644
> --- a/drivers/usb/host/xhci-mtk.h
> +++ b/drivers/usb/host/xhci-mtk.h
> @@ -21,12 +21,18 @@
>
> /**
> * @split_bit_map: used to avoid split microframes overlay
> + * @ss_bit_map: used to avoid start split microframes overlay
> + * @cs_bit_map: used to avoid complete split microframes overlay
> + * @fs_bus_bw: array to keep track of bandwidth already used at full speed
> * @ep_list: Endpoints using this TT
> * @usb_tt: usb TT related
> * @tt_port: TT port number
> */
> struct mu3h_sch_tt {
> DECLARE_BITMAP(split_bit_map, XHCI_MTK_MAX_ESIT);
> + DECLARE_BITMAP(ss_bit_map, XHCI_MTK_MAX_ESIT);
> + DECLARE_BITMAP(cs_bit_map, XHCI_MTK_MAX_ESIT + 1);
> + u32 fs_bus_bw[XHCI_MTK_MAX_ESIT];
> struct list_head ep_list;
> struct usb_tt *usb_tt;
> int tt_port;
> @@ -42,7 +48,7 @@ struct mu3h_sch_tt {
> * two bandwidth domains, one for IN eps and another for OUT eps.
> */
> struct mu3h_sch_bw_info {
> - u32 bus_bw[XHCI_MTK_MAX_ESIT];
> + u32 bus_bw[XHCI_MTK_MAX_ESIT + 1];
> struct list_head bw_ep_list;
> };
>
> --
> 2.18.0
> _______________________________________________
> Linux-mediatek mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
HI Chunfeng
请回复谷歌以下三个问题：
Queston: Are there any reasons for doing this? can only one split packet be scheduled in a u-frame for isochronous out? This looks like overkill.
Answer: We will calculate the number of uframes based on the isoc out ep maxp. Except for the last uframe, all other consumed bandwidth is 188. But to be safe, we also count the bandwidth consumed by the last SS as 188.
Queston: same here. It would be much better to understand if you can provide some counterexamples of schedule that can happen when this bitmap checking logic is omitted.
Answer: Maybe you are right, it will indeed double check. I will delete the bandwidth check, and then perform a regression test.
Queston: I guess this is enough to check the bandwidth limit of the lower speed bus without a bitmap.
Answer: I explained to you last time that ss cannot be overlapped. This is a hub compatibility issue. Therefore we need bitmap to prevent ss overlap.
BR
yaqii
-----邮件原件-----
发件人: Ikjoon Jang [mailto:[email protected]]
发送时间: 2021年1月13日 17:44
收件人: Chunfeng Yun (云春峰) <[email protected]>
抄送: Yaqii Wu (武亚奇) <[email protected]>; Zhanyong Wang (王战勇) <[email protected]>; Zhanyong Wang <[email protected]>; [email protected]; open list <[email protected]>; Tianping Fang (方天平) <[email protected]>; moderated list:ARM/Mediatek SoC support <[email protected]>
主题: Re: [RFC PATCH v3 1/5] usb: xhci-mtk: improve bandwidth scheduling with multi-TT
On Tue, Dec 22, 2020 at 5:35 PM Chunfeng Yun <[email protected]> wrote:
>
> From: Zhanyong Wang <[email protected]>
>
> After inserted the usb type-c 3.5mm dongle with headset, dmesg showed:
> usb 1-1.1: new full-speed USB device number 5 using xhci-mtk usb
> 1-1.1: New USB device found, idVendor=05ac, idProduct=110a,
> bcdDevice=26.11 usb 1-1.1: New USB device strings: Mfr=1, Product=2,
> SerialNumber=3 usb 1-1.1: Product: USB-C to 3.5mm Headphone Jack
> Adapter usb 1-1.1: Manufacturer: Apple, Inc.
> usb 1-1.1: SerialNumber: DWH915501TFJKLTAM xhci-mtk 11200000.xhci: Not
> enough bandwidth!
> usb 1-1.1: can't set config #2, error -28
>
> improve low-speed/full-speed INT/ISOC bandwidth scheduling with USB
> muli-TT.
>
> Signed-off-by: Yaqii Wu <[email protected]>
> Signed-off-by: Chunfeng Yun <[email protected]>
> ---
> v2~v3: no changes
> ---
> drivers/usb/host/xhci-mtk-sch.c | 91 ++++++++++++++++++++++++++++-----
> drivers/usb/host/xhci-mtk.h | 8 ++-
> 2 files changed, 84 insertions(+), 15 deletions(-) mode change
> 100644 => 100755 drivers/usb/host/xhci-mtk-sch.c
>
> diff --git a/drivers/usb/host/xhci-mtk-sch.c
> b/drivers/usb/host/xhci-mtk-sch.c old mode 100644 new mode 100755
> index 45c54d56ecbd..94292b9bbc63
> --- a/drivers/usb/host/xhci-mtk-sch.c
> +++ b/drivers/usb/host/xhci-mtk-sch.c
> @@ -383,7 +383,9 @@ static int check_sch_tt(struct usb_device *udev,
> u32 fs_budget_start;
> u32 start_ss, last_ss;
> u32 start_cs, last_cs;
> - int i;
> + u32 num_esit, base;
> + int i, j;
> + u32 tmp;
>
> start_ss = offset % 8;
> fs_budget_start = (start_ss + 1) % 8; @@ -398,10 +400,13 @@
> static int check_sch_tt(struct usb_device *udev,
> if (!(start_ss == 7 || last_ss < 6))
> return -ERANGE;
>
> - for (i = 0; i < sch_ep->cs_count; i++)
> - if (test_bit(offset + i, tt->split_bit_map))
> + for (i = 0; i < sch_ep->cs_count; i++) {
> + if (test_bit(offset + i, tt->ss_bit_map))
> return -ERANGE;
>
> + if (test_bit(offset + i, tt->cs_bit_map))
> + return -ERANGE;
> + }
Are there any reasons for doing this?
Why can only one split packet be scheduled in a u-frame for isochronous out?
This looks like overkill.
> } else {
> u32 cs_count = DIV_ROUND_UP(sch_ep->maxpkt,
> FS_PAYLOAD_MAX);
>
> @@ -428,8 +433,10 @@ static int check_sch_tt(struct usb_device *udev,
> if (cs_count > 7)
> cs_count = 7; /* HW limit */
>
> - for (i = 0; i < cs_count + 2; i++) {
> - if (test_bit(offset + i, tt->split_bit_map))
> + if (test_bit(offset, tt->ss_bit_map))
> + return -ERANGE;
> + for (i = 0; i < cs_count; i++) {
> + if (test_bit(offset + 2 + i, tt->cs_bit_map))
> return -ERANGE;
> }
>
same here. It would be much better to understand if you can provide some counterexamples of schedule that can happen when this bitmap checking logic is omitted.
> @@ -445,11 +452,22 @@ static int check_sch_tt(struct usb_device *udev,
> sch_ep->num_budget_microframes = sch_ep->esit;
> }
>
> + num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> + for (i = 0; i < num_esit; i++) {
> + base = sch_ep->offset + i * sch_ep->esit;
> + for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> + tmp = tt->fs_bus_bw[base + j]
> + + sch_ep->bw_cost_per_microframe;
> + if (tmp > FS_PAYLOAD_MAX)
> + return -ERANGE;
> + }
> + }
I guess this is enough to check the bandwidth limit of the lower speed bus without a bitmap.
> +
> return 0;
> }
>
> static void update_sch_tt(struct usb_device *udev,
> - struct mu3h_sch_ep_info *sch_ep)
> + struct mu3h_sch_ep_info *sch_ep, bool used)
> {
> struct mu3h_sch_tt *tt = sch_ep->sch_tt;
> u32 base, num_esit;
> @@ -458,11 +476,52 @@ static void update_sch_tt(struct usb_device *udev,
> num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> for (i = 0; i < num_esit; i++) {
> base = sch_ep->offset + i * sch_ep->esit;
> - for (j = 0; j < sch_ep->num_budget_microframes; j++)
> - set_bit(base + j, tt->split_bit_map);
> + for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> + if (used)
> + set_bit(base + j, tt->split_bit_map);
> + else
> + clear_bit(base + j, tt->split_bit_map);
> + }
> +
> + if (sch_ep->ep_type == ISOC_OUT_EP) {
> + for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> + if (used) {
> + set_bit(base + j, tt->ss_bit_map);
> + set_bit(base + j, tt->cs_bit_map);
> + tt->fs_bus_bw[base + j] +=
> + sch_ep->bw_cost_per_microframe;
> + } else {
> + clear_bit(base + j, tt->ss_bit_map);
> + clear_bit(base + j, tt->cs_bit_map);
> + tt->fs_bus_bw[base + j] -=
> + sch_ep->bw_cost_per_microframe;
> + }
> + }
> + } else {
> + if (used)
> + set_bit(base, tt->ss_bit_map);
> + else
> + clear_bit(base, tt->ss_bit_map);
> +
> + for (j = 0; j < sch_ep->cs_count; j++) {
> + if (used) {
> + set_bit(base + 2 + j,
> + tt->cs_bit_map);
> +
> + tt->fs_bus_bw[base + 2 + j] +=
> + sch_ep->bw_cost_per_microframe;
> + } else {
> + clear_bit(base + 2 + j, tt->cs_bit_map);
> + tt->fs_bus_bw[base + 2 + j] -=
> + sch_ep->bw_cost_per_microframe;
> + }
> + }
> + }
> }
>
> - list_add_tail(&sch_ep->tt_endpoint, &tt->ep_list);
> + if (used)
> + list_add_tail(&sch_ep->tt_endpoint, &tt->ep_list);
> + else
> + list_del(&sch_ep->tt_endpoint);
> }
>
> static int check_sch_bw(struct usb_device *udev, @@ -470,6 +529,7 @@
> static int check_sch_bw(struct usb_device *udev, {
> u32 offset;
> u32 esit;
> + u32 boundary;
> u32 min_bw;
> u32 min_index;
> u32 worst_bw;
> @@ -487,10 +547,13 @@ static int check_sch_bw(struct usb_device *udev,
> */
> min_bw = ~0;
> min_index = 0;
> + boundary = esit;
> min_cs_count = sch_ep->cs_count;
> min_num_budget = sch_ep->num_budget_microframes;
> for (offset = 0; offset < esit; offset++) {
> if (is_fs_or_ls(udev->speed)) {
> + if (sch_ep->ep_type != ISOC_OUT_EP)
> + boundary = esit + 1;
> ret = check_sch_tt(udev, sch_ep, offset);
> if (ret)
> continue; @@ -498,7 +561,7 @@ static
> int check_sch_bw(struct usb_device *udev,
> tt_offset_ok = true;
> }
>
> - if ((offset + sch_ep->num_budget_microframes) > sch_ep->esit)
> + if ((offset + sch_ep->num_budget_microframes) >
> + boundary)
> break;
>
> worst_bw = get_max_bw(sch_bw, sch_ep, offset); @@
> -532,7 +595,7 @@ static int check_sch_bw(struct usb_device *udev,
> if (!tt_offset_ok)
> return -ERANGE;
>
> - update_sch_tt(udev, sch_ep);
> + update_sch_tt(udev, sch_ep, 1);
> }
>
> /* update bus bandwidth info */ @@ -696,12 +759,12 @@ void
> xhci_mtk_drop_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
>
> list_for_each_entry(sch_ep, &sch_bw->bw_ep_list, endpoint) {
> if (sch_ep->ep == ep) {
> - update_bus_bw(sch_bw, sch_ep, 0);
> - list_del(&sch_ep->endpoint);
> if (is_fs_or_ls(udev->speed)) {
> - list_del(&sch_ep->tt_endpoint);
> + update_sch_tt(udev, sch_ep, 0);
> drop_tt(udev);
> }
> + update_bus_bw(sch_bw, sch_ep, 0);
> + list_del(&sch_ep->endpoint);
> kfree(sch_ep);
> break;
> }
> diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
> index a93cfe817904..323b281933b9 100644
> --- a/drivers/usb/host/xhci-mtk.h
> +++ b/drivers/usb/host/xhci-mtk.h
> @@ -21,12 +21,18 @@
>
> /**
> * @split_bit_map: used to avoid split microframes overlay
> + * @ss_bit_map: used to avoid start split microframes overlay
> + * @cs_bit_map: used to avoid complete split microframes overlay
> + * @fs_bus_bw: array to keep track of bandwidth already used at full
> + speed
> * @ep_list: Endpoints using this TT
> * @usb_tt: usb TT related
> * @tt_port: TT port number
> */
> struct mu3h_sch_tt {
> DECLARE_BITMAP(split_bit_map, XHCI_MTK_MAX_ESIT);
> + DECLARE_BITMAP(ss_bit_map, XHCI_MTK_MAX_ESIT);
> + DECLARE_BITMAP(cs_bit_map, XHCI_MTK_MAX_ESIT + 1);
> + u32 fs_bus_bw[XHCI_MTK_MAX_ESIT];
> struct list_head ep_list;
> struct usb_tt *usb_tt;
> int tt_port;
> @@ -42,7 +48,7 @@ struct mu3h_sch_tt {
> * two bandwidth domains, one for IN eps and another for OUT eps.
> */
> struct mu3h_sch_bw_info {
> - u32 bus_bw[XHCI_MTK_MAX_ESIT];
> + u32 bus_bw[XHCI_MTK_MAX_ESIT + 1];
> struct list_head bw_ep_list;
> };
>
> --
> 2.18.0
> _______________________________________________
> Linux-mediatek mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
On Wed, 2021-01-13 at 17:43 +0800, Ikjoon Jang wrote:
> On Tue, Dec 22, 2020 at 5:35 PM Chunfeng Yun <[email protected]> wrote:
> >
> > From: Zhanyong Wang <[email protected]>
> >
> > After inserted the usb type-c 3.5mm dongle with headset, dmesg showed:
> > usb 1-1.1: new full-speed USB device number 5 using xhci-mtk
> > usb 1-1.1: New USB device found, idVendor=05ac, idProduct=110a, bcdDevice=26.11
> > usb 1-1.1: New USB device strings: Mfr=1, Product=2, SerialNumber=3
> > usb 1-1.1: Product: USB-C to 3.5mm Headphone Jack Adapter
> > usb 1-1.1: Manufacturer: Apple, Inc.
> > usb 1-1.1: SerialNumber: DWH915501TFJKLTAM
> > xhci-mtk 11200000.xhci: Not enough bandwidth!
> > usb 1-1.1: can't set config #2, error -28
> >
> > improve low-speed/full-speed INT/ISOC bandwidth scheduling with USB
> > muli-TT.
> >
> > Signed-off-by: Yaqii Wu <[email protected]>
> > Signed-off-by: Chunfeng Yun <[email protected]>
> > ---
> > v2~v3: no changes
> > ---
> > drivers/usb/host/xhci-mtk-sch.c | 91 ++++++++++++++++++++++++++++-----
> > drivers/usb/host/xhci-mtk.h | 8 ++-
> > 2 files changed, 84 insertions(+), 15 deletions(-)
> > mode change 100644 => 100755 drivers/usb/host/xhci-mtk-sch.c
> >
> > diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
> > old mode 100644
> > new mode 100755
> > index 45c54d56ecbd..94292b9bbc63
> > --- a/drivers/usb/host/xhci-mtk-sch.c
> > +++ b/drivers/usb/host/xhci-mtk-sch.c
> > @@ -383,7 +383,9 @@ static int check_sch_tt(struct usb_device *udev,
> > u32 fs_budget_start;
> > u32 start_ss, last_ss;
> > u32 start_cs, last_cs;
> > - int i;
> > + u32 num_esit, base;
> > + int i, j;
> > + u32 tmp;
> >
> > start_ss = offset % 8;
> > fs_budget_start = (start_ss + 1) % 8;
> > @@ -398,10 +400,13 @@ static int check_sch_tt(struct usb_device *udev,
> > if (!(start_ss == 7 || last_ss < 6))
> > return -ERANGE;
> >
> > - for (i = 0; i < sch_ep->cs_count; i++)
> > - if (test_bit(offset + i, tt->split_bit_map))
> > + for (i = 0; i < sch_ep->cs_count; i++) {
> > + if (test_bit(offset + i, tt->ss_bit_map))
> > return -ERANGE;
> >
> > + if (test_bit(offset + i, tt->cs_bit_map))
> > + return -ERANGE;
> > + }
>
> Are there any reasons for doing this?
> Why can only one split packet be scheduled in a u-frame for isochronous out?
> This looks like overkill.
[From DR]: We will calculate the number of uframes based on the isoc out
ep maxp. Except for the last uframe, all other consumed bandwidth is
188. But to be safe, we also count the bandwidth consumed by the last SS
as 188.
>
> > } else {
> > u32 cs_count = DIV_ROUND_UP(sch_ep->maxpkt, FS_PAYLOAD_MAX);
> >
> > @@ -428,8 +433,10 @@ static int check_sch_tt(struct usb_device *udev,
> > if (cs_count > 7)
> > cs_count = 7; /* HW limit */
> >
> > - for (i = 0; i < cs_count + 2; i++) {
> > - if (test_bit(offset + i, tt->split_bit_map))
> > + if (test_bit(offset, tt->ss_bit_map))
> > + return -ERANGE;
> > + for (i = 0; i < cs_count; i++) {
> > + if (test_bit(offset + 2 + i, tt->cs_bit_map))
> > return -ERANGE;
> > }
> >
>
> same here. It would be much better to understand
> if you can provide some counterexamples of schedule
> that can happen when this bitmap checking logic is omitted.
[From DR] Maybe you are right, it will indeed double check. I will
delete the bandwidth check, and then perform a regression test.
>
> > @@ -445,11 +452,22 @@ static int check_sch_tt(struct usb_device *udev,
> > sch_ep->num_budget_microframes = sch_ep->esit;
> > }
> >
> > + num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> > + for (i = 0; i < num_esit; i++) {
> > + base = sch_ep->offset + i * sch_ep->esit;
> > + for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> > + tmp = tt->fs_bus_bw[base + j]
> > + + sch_ep->bw_cost_per_microframe;
> > + if (tmp > FS_PAYLOAD_MAX)
> > + return -ERANGE;
> > + }
> > + }
>
> I guess this is enough to check the bandwidth limit of the
> lower speed bus without a bitmap.
[From DR]As explained in issue tracker that ss cannot be overlapped.
This is a hub compatibility issue. Therefore we need bitmap to prevent
ss overlap.
Thanks
>
> > +
> > return 0;
> > }
> >
> > static void update_sch_tt(struct usb_device *udev,
> > - struct mu3h_sch_ep_info *sch_ep)
> > + struct mu3h_sch_ep_info *sch_ep, bool used)
> > {
> > struct mu3h_sch_tt *tt = sch_ep->sch_tt;
> > u32 base, num_esit;
> > @@ -458,11 +476,52 @@ static void update_sch_tt(struct usb_device *udev,
> > num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> > for (i = 0; i < num_esit; i++) {
> > base = sch_ep->offset + i * sch_ep->esit;
> > - for (j = 0; j < sch_ep->num_budget_microframes; j++)
> > - set_bit(base + j, tt->split_bit_map);
> > + for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> > + if (used)
> > + set_bit(base + j, tt->split_bit_map);
> > + else
> > + clear_bit(base + j, tt->split_bit_map);
> > + }
> > +
> > + if (sch_ep->ep_type == ISOC_OUT_EP) {
> > + for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> > + if (used) {
> > + set_bit(base + j, tt->ss_bit_map);
> > + set_bit(base + j, tt->cs_bit_map);
> > + tt->fs_bus_bw[base + j] +=
> > + sch_ep->bw_cost_per_microframe;
> > + } else {
> > + clear_bit(base + j, tt->ss_bit_map);
> > + clear_bit(base + j, tt->cs_bit_map);
> > + tt->fs_bus_bw[base + j] -=
> > + sch_ep->bw_cost_per_microframe;
> > + }
> > + }
> > + } else {
> > + if (used)
> > + set_bit(base, tt->ss_bit_map);
> > + else
> > + clear_bit(base, tt->ss_bit_map);
> > +
> > + for (j = 0; j < sch_ep->cs_count; j++) {
> > + if (used) {
> > + set_bit(base + 2 + j, tt->cs_bit_map);
> > +
> > + tt->fs_bus_bw[base + 2 + j] +=
> > + sch_ep->bw_cost_per_microframe;
> > + } else {
> > + clear_bit(base + 2 + j, tt->cs_bit_map);
> > + tt->fs_bus_bw[base + 2 + j] -=
> > + sch_ep->bw_cost_per_microframe;
> > + }
> > + }
> > + }
> > }
> >
> > - list_add_tail(&sch_ep->tt_endpoint, &tt->ep_list);
> > + if (used)
> > + list_add_tail(&sch_ep->tt_endpoint, &tt->ep_list);
> > + else
> > + list_del(&sch_ep->tt_endpoint);
> > }
> >
> > static int check_sch_bw(struct usb_device *udev,
> > @@ -470,6 +529,7 @@ static int check_sch_bw(struct usb_device *udev,
> > {
> > u32 offset;
> > u32 esit;
> > + u32 boundary;
> > u32 min_bw;
> > u32 min_index;
> > u32 worst_bw;
> > @@ -487,10 +547,13 @@ static int check_sch_bw(struct usb_device *udev,
> > */
> > min_bw = ~0;
> > min_index = 0;
> > + boundary = esit;
> > min_cs_count = sch_ep->cs_count;
> > min_num_budget = sch_ep->num_budget_microframes;
> > for (offset = 0; offset < esit; offset++) {
> > if (is_fs_or_ls(udev->speed)) {
> > + if (sch_ep->ep_type != ISOC_OUT_EP)
> > + boundary = esit + 1;
> > ret = check_sch_tt(udev, sch_ep, offset);
> > if (ret)
> > continue;
> > @@ -498,7 +561,7 @@ static int check_sch_bw(struct usb_device *udev,
> > tt_offset_ok = true;
> > }
> >
> > - if ((offset + sch_ep->num_budget_microframes) > sch_ep->esit)
> > + if ((offset + sch_ep->num_budget_microframes) > boundary)
> > break;
> >
> > worst_bw = get_max_bw(sch_bw, sch_ep, offset);
> > @@ -532,7 +595,7 @@ static int check_sch_bw(struct usb_device *udev,
> > if (!tt_offset_ok)
> > return -ERANGE;
> >
> > - update_sch_tt(udev, sch_ep);
> > + update_sch_tt(udev, sch_ep, 1);
> > }
> >
> > /* update bus bandwidth info */
> > @@ -696,12 +759,12 @@ void xhci_mtk_drop_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
> >
> > list_for_each_entry(sch_ep, &sch_bw->bw_ep_list, endpoint) {
> > if (sch_ep->ep == ep) {
> > - update_bus_bw(sch_bw, sch_ep, 0);
> > - list_del(&sch_ep->endpoint);
> > if (is_fs_or_ls(udev->speed)) {
> > - list_del(&sch_ep->tt_endpoint);
> > + update_sch_tt(udev, sch_ep, 0);
> > drop_tt(udev);
> > }
> > + update_bus_bw(sch_bw, sch_ep, 0);
> > + list_del(&sch_ep->endpoint);
> > kfree(sch_ep);
> > break;
> > }
> > diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
> > index a93cfe817904..323b281933b9 100644
> > --- a/drivers/usb/host/xhci-mtk.h
> > +++ b/drivers/usb/host/xhci-mtk.h
> > @@ -21,12 +21,18 @@
> >
> > /**
> > * @split_bit_map: used to avoid split microframes overlay
> > + * @ss_bit_map: used to avoid start split microframes overlay
> > + * @cs_bit_map: used to avoid complete split microframes overlay
> > + * @fs_bus_bw: array to keep track of bandwidth already used at full speed
> > * @ep_list: Endpoints using this TT
> > * @usb_tt: usb TT related
> > * @tt_port: TT port number
> > */
> > struct mu3h_sch_tt {
> > DECLARE_BITMAP(split_bit_map, XHCI_MTK_MAX_ESIT);
> > + DECLARE_BITMAP(ss_bit_map, XHCI_MTK_MAX_ESIT);
> > + DECLARE_BITMAP(cs_bit_map, XHCI_MTK_MAX_ESIT + 1);
> > + u32 fs_bus_bw[XHCI_MTK_MAX_ESIT];
> > struct list_head ep_list;
> > struct usb_tt *usb_tt;
> > int tt_port;
> > @@ -42,7 +48,7 @@ struct mu3h_sch_tt {
> > * two bandwidth domains, one for IN eps and another for OUT eps.
> > */
> > struct mu3h_sch_bw_info {
> > - u32 bus_bw[XHCI_MTK_MAX_ESIT];
> > + u32 bus_bw[XHCI_MTK_MAX_ESIT + 1];
> > struct list_head bw_ep_list;
> > };
> >
> > --
> > 2.18.0
> > _______________________________________________
> > Linux-mediatek mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek