| Message ID | 20210826025144.51992-3-chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [next,v2,1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation" | expand |
Hi Chunfeng, On Thu, Aug 26, 2021 at 10:52 AM Chunfeng Yun <chunfeng.yun@mediatek.com> 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 <chunfeng.yun@mediatek.com> > --- > 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 >
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 < > chunfeng.yun@mediatek.com> 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 <chunfeng.yun@mediatek.com> > > --- > > 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 (云春峰) <Chunfeng.Yun@mediatek.com> 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 < > > chunfeng.yun@mediatek.com> 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 <chunfeng.yun@mediatek.com> > > > --- > > > 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 > > >
On Fri, 2021-08-27 at 17:14 +0800, Ikjoon Jang wrote: > On Fri, Aug 27, 2021 at 2:49 PM Chunfeng Yun (云春峰) > <Chunfeng.Yun@mediatek.com> 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 < > > > chunfeng.yun@mediatek.com> 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 <chunfeng.yun@mediatek.com> > > > > --- > > > > 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 (云春峰) <Chunfeng.Yun@mediatek.com> wrote: > > On Fri, 2021-08-27 at 17:14 +0800, Ikjoon Jang wrote: > > On Fri, Aug 27, 2021 at 2:49 PM Chunfeng Yun (云春峰) > > <Chunfeng.Yun@mediatek.com> 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 < > > > > chunfeng.yun@mediatek.com> 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 <chunfeng.yun@mediatek.com> > > > > > --- > > > > > 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 (云春峰) > <Chunfeng.Yun@mediatek.com> wrote: > > > > On Fri, 2021-08-27 at 17:14 +0800, Ikjoon Jang wrote: > > > On Fri, Aug 27, 2021 at 2:49 PM Chunfeng Yun (云春峰) > > > <Chunfeng.Yun@mediatek.com> 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 < > > > > > chunfeng.yun@mediatek.com> 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 <chunfeng.yun@mediatek.com> > > > > > > --- > > > > > > 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 > > > > > >
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)
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 <chunfeng.yun@mediatek.com> --- v2: new patch, move from another series --- drivers/usb/host/xhci-mtk-sch.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)