diff mbox series

[next,v2,3/6] usb: xhci-mtk: update fs bus bandwidth by bw_budget_table

Message ID 20210826025144.51992-3-chunfeng.yun@mediatek.com
State New
Headers show
Series [next,v2,1/6] Revert "usb: xhci-mtk: relax TT periodic bandwidth allocation" | expand

Commit Message

Chunfeng Yun (云春峰) Aug. 26, 2021, 2:51 a.m. UTC
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(-)

Comments

Chunfeng Yun (云春峰) Aug. 27, 2021, 6:49 a.m. UTC | #1
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

> >
Ikjoon Jang Aug. 27, 2021, 9:14 a.m. UTC | #2
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

> > >
Chunfeng Yun (云春峰) Aug. 27, 2021, 9:48 a.m. UTC | #3
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

> > > >
Ikjoon Jang Aug. 30, 2021, 3:49 a.m. UTC | #4
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

> > > > >
Chunfeng Yun (云春峰) Aug. 31, 2021, 6:50 a.m. UTC | #5
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 mbox series

Patch

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)