Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller

[chunfeng yun]

hi,
On Mon, 2015-10-19 at 14:25 +0300, Mathias Nyman wrote:
> >>
> >> So basically we are trying to use as many microframes as possible with as 
> >> few packets
> >> per microframe as possible.
> >>
> >> Did I understand this correctly?
> > Yes, you are right.
> >
> >> How will devices react if they expect to get 16 packets every 16th 
> >> microframe,
> >> but they get one packet every microframe instead?
> > I think that the synchronous endpoint must specify its period by
> > bInterval, but can't specify how data should be transfered during the
> > period by the host, and it just only receives data passively. So the
> > device can receive data correctly in the case(bInterval is 5).
> >
> > quote from usb3_r1.0 section4.4.8 Isochronous Transfers:
> > "The host can request data from the device or send data to the device at
> > any time during the service interval for a particular endpoint on that
> > device"
> >
> 
> As I understand the 4.4.8 section it just means the device can't assume a 
> fixed
> time interval between transfers, meaning that the host can use the last 
> microframe
> in one esit and the first microframe in the next esit, but still only use 1 
> microframe
> per esit.
> 
> Section 8.12.6.1 describes how a 11 packet isoc transfer is allowed to be 
> split
> to 1 burst of 11 packets, 2 burst (8 + 3),  3 burst (4+4+3) 6 bursts 
> (2+2+2+2+2+1) or
> 11 bursts of 1. These are however all within the same microframe. Splitting 
> the
> transfer into several microframes in a esit kind of makes the whole interval 
> concept pointless.
> 
It doesn't say that the packets should be transfered within the same
microframe (bus interval), as I understand it means service interval;

The direct prove resides in figure 8-56/8-57.

Term: 
1. BI, bus interval, a 125 us period that establishes the internal
boundary of service interval, aka uframe;
2. SSI, Support Smart Isochronous;
3. DBI, Data in this Bus Interval is done;
4. NBI, Numbers of Bus Interval;

As the figure shows, the service interval = 8 BI, that host distribute 2
packets @1st uframe, keep U1/U2 state for the next 3uframe, then
transmit 4 packets @4th uframe, and the remaining 3 packet in the last
frame.

Please notice that this just is an example illustrated by spec, but we
can derive the conclusion that the distribution of packet in a service
interval is completely decided by host, and can split isoc transfers
across multiple uframes.

PS: as you can see, MTK implementation of schedule algorithms is an
implementation of Smart Isochronous of which the smart side resides in
software.


> -Mathias
> 
> 
> 
>   
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller

[Mathias Nyman]

On 20.10.2015 09:29, chunfeng yun wrote:

hi,
On Mon, 2015-10-19 at 14:25 +0300, Mathias Nyman wrote:


So basically we are trying to use as many microframes as possible with as few 
packets
per microframe as possible.

Did I understand this correctly?

Yes, you are right.


How will devices react if they expect to get 16 packets every 16th microframe,
but they get one packet every microframe instead?

I think that the synchronous endpoint must specify its period by
bInterval, but can't specify how data should be transfered during the
period by the host, and it just only receives data passively. So the
device can receive data correctly in the case(bInterval is 5).

quote from usb3_r1.0 section4.4.8 Isochronous Transfers:
"The host can request data from the device or send data to the device at
any time during the service interval for a particular endpoint on that
device"



As I understand the 4.4.8 section it just means the device can't assume a fixed
time interval between transfers, meaning that the host can use the last 
microframe
in one esit and the first microframe in the next esit, but still only use 1 
microframe
per esit.

Section 8.12.6.1 describes how a 11 packet isoc transfer is allowed to be split
to 1 burst of 11 packets, 2 burst (8 + 3),  3 burst (4+4+3) 6 bursts 
(2+2+2+2+2+1) or
11 bursts of 1. These are however all within the same microframe. Splitting the
transfer into several microframes in a esit kind of makes the whole interval 
concept pointless.


It doesn't say that the packets should be transfered within the same
microframe (bus interval), as I understand it means service interval;

The direct prove resides in figure 8-56/8-57.

Term:
1. BI, bus interval, a 125 us period that establishes the internal
boundary of service interval, aka uframe;
2. SSI, Support Smart Isochronous;
3. DBI, Data in this Bus Interval is done;
4. NBI, Numbers of Bus Interval;

As the figure shows, the service interval = 8 BI, that host distribute 2
packets @1st uframe, keep U1/U2 state for the next 3uframe, then
transmit 4 packets @4th uframe, and the remaining 3 packet in the last
frame.

Please notice that this just is an example illustrated by spec, but we
can derive the conclusion that the distribution of packet in a service
interval is completely decided by host, and can split isoc transfers
across multiple uframes.


So it seems. You're right



PS: as you can see, MTK implementation of schedule algorithms is an
implementation of Smart Isochronous of which the smart side resides in
software.


Thanks for the clarification, I now understand how the implementation works

-Mathias  


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller

[Mathias Nyman]

So basically we are trying to use as many microframes as possible with as few 
packets
per microframe as possible.

Did I understand this correctly?

Yes, you are right.


How will devices react if they expect to get 16 packets every 16th microframe,
but they get one packet every microframe instead?

I think that the synchronous endpoint must specify its period by
bInterval, but can't specify how data should be transfered during the
period by the host, and it just only receives data passively. So the
device can receive data correctly in the case(bInterval is 5).

quote from usb3_r1.0 section4.4.8 Isochronous Transfers:
"The host can request data from the device or send data to the device at
any time during the service interval for a particular endpoint on that
device"



As I understand the 4.4.8 section it just means the device can't assume a fixed
time interval between transfers, meaning that the host can use the last 
microframe
in one esit and the first microframe in the next esit, but still only use 1 
microframe
per esit.

Section 8.12.6.1 describes how a 11 packet isoc transfer is allowed to be split
to 1 burst of 11 packets, 2 burst (8 + 3),  3 burst (4+4+3) 6 bursts 
(2+2+2+2+2+1) or
11 bursts of 1. These are however all within the same microframe. Splitting the
transfer into several microframes in a esit kind of makes the whole interval 
concept pointless.

-Mathias



 


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller

[chunfeng yun]

Hi,
On Thu, 2015-10-15 at 17:46 +0300, Mathias Nyman wrote:
> On 29.09.2015 06:01, Chunfeng Yun wrote:
> > There some vendor quirks for MTK xhci host controller:
> > 1. It defines some extra SW scheduling parameters for HW
> >to minimize the scheduling effort for synchronous and
> >interrupt endpoints. The parameters are put into reseved
> >DWs of slot context and endpoint context.
> > 2. Its IMODI unit for Interrupter Moderation register is
> >8 times as much as that defined in xHCI spec.
> > 3. Its TDS in  Normal TRB defines a number of packets that
> >remains to be transferred for a TD after processing all
> >Max packets in all previous TRBs.
> >
> > Signed-off-by: Chunfeng Yun 
> 
> Looks good in my opinion, There's one part about how we split the ISOC 
> transafers
> across multiple microframes that I don't fully understand:
> 
> > +   } else if (udev->speed == USB_SPEED_SUPER) {
> > +   /* usb3_r1 spec section4.4.7 & 4.4.8 */
> > +   sch_ep->cs_count = 0;
> > +   esit_pkts = (mult + 1) * (max_burst + 1);
> 
> So the device would like the transfers to be esit_pkts packets every esit 
> microframe apart.
> 
> > +   if (ep_type == INT_IN_EP || ep_type == INT_OUT_EP) {
> > +   sch_ep->pkts = esit_pkts;
> > +   sch_ep->num_budget_microframes = 1;
> > +   sch_ep->repeat = 0;
> > +   }
> > +
> > +   if (ep_type == ISOC_IN_EP || ep_type == ISOC_OUT_EP) {
> > +   if (esit_pkts <= sch_ep->esit)
> > +   sch_ep->pkts = 1;
> > +   else
> > +   sch_ep->pkts = roundup_pow_of_two(esit_pkts)
> > +   / sch_ep->esit;
> 
> sch_ep->pkts now contain a rounded up average value of how many packets per 
> microframe
> would be transferred in case we'd transfer packets every microframe instead 
> of every esit frame apart.
> 
> > +
> > +   sch_ep->num_budget_microframes =
> > +   DIV_ROUND_UP(esit_pkts, sch_ep->pkts);
> 
> sch_ep->num_budget_microframes now contains the number of microframes during 
> an esit needed
> to transer all data if each microframe contains the average amount of data.
> 
> So basically we are trying to use as many microframes as possible with as few 
> packets
> per microframe as possible.
> 
> Did I understand this correctly?
Yes, you are right.

> How will devices react if they expect to get 16 packets every 16th microframe,
> but they get one packet every microframe instead?
I think that the synchronous endpoint must specify its period by
bInterval, but can't specify how data should be transfered during the
period by the host, and it just only receives data passively. So the
device can receive data correctly in the case(bInterval is 5).

quote from usb3_r1.0 section4.4.8 Isochronous Transfers:
"The host can request data from the device or send data to the device at
any time during the service interval for a particular endpoint on that
device"

> 
> Or is this just theoretical bw calculations for the host and it will in the 
> end do
> whatever it wants with the information.
The host will schedule synchronous endpoints in a specific
micro-frame/frame according to this parameters provided by software.

Thanks a lot.
> 
> Otherwise, by making the changes Daniel Thompson suggested I think the xhci 
> parts looks ready.
> 
> -Mathias
> 
>   
> 
>
>   
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> > +   } else if (is_fs_or_ls(udev->speed)) {
> > +
> > +   /*
> > +* usb_20 spec section11.18.4
> > +* assume worst cases
> > +*/
> > +   sch_ep->repeat = 0;
> > +   sch_ep->pkts = 1; /* at most one packet for each microframe */
> > +   if (ep_type == INT_IN_EP || ep_type == INT_OUT_EP) {
> > +   sch_ep->cs_count = 3; /* at most need 3 CS*/
> > +   /* one for SS and one for budgeted transaction */
> > +   sch_ep->num_budget_microframes = sch_ep->cs_count + 2;
> > +   sch_ep->bw_cost_per_microframe = max_packet_size;
> > +   }
> > +   if (ep_type == ISOC_OUT_EP) {
> > +
> > +   /*
> > +* the best case FS budget assumes that 188 FS bytes
> > +* occur in each microframe
> > +*/
> > +   sch_ep->num_budget_microframes = DIV_ROUND_UP(
> > +   max_packet_size, FS_PAYLOAD_MAX);
> > +   sch_ep->bw_cost_per_microframe = FS_PAYLOAD_MAX;
> > +   sch_ep->cs_count = sch_ep->num_budget_microframes;
> > +   }
> > +   if (ep_type == ISOC_IN_EP) {
> > +   /* at most need additional two CS. */
> > +   sch_ep->cs_count = DIV_ROUND_UP(
> > +   max_packet_size, 

Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller

[Mathias Nyman]

On 29.09.2015 06:01, Chunfeng Yun wrote:

There some vendor quirks for MTK xhci host controller:
1. It defines some extra SW scheduling parameters for HW
   to minimize the scheduling effort for synchronous and
   interrupt endpoints. The parameters are put into reseved
   DWs of slot context and endpoint context.
2. Its IMODI unit for Interrupter Moderation register is
   8 times as much as that defined in xHCI spec.
3. Its TDS in  Normal TRB defines a number of packets that
   remains to be transferred for a TD after processing all
   Max packets in all previous TRBs.

Signed-off-by: Chunfeng Yun 


Looks good in my opinion, There's one part about how we split the ISOC 
transafers
across multiple microframes that I don't fully understand:


+   } else if (udev->speed == USB_SPEED_SUPER) {
+   /* usb3_r1 spec section4.4.7 & 4.4.8 */
+   sch_ep->cs_count = 0;
+   esit_pkts = (mult + 1) * (max_burst + 1);


So the device would like the transfers to be esit_pkts packets every esit 
microframe apart.


+   if (ep_type == INT_IN_EP || ep_type == INT_OUT_EP) {
+   sch_ep->pkts = esit_pkts;
+   sch_ep->num_budget_microframes = 1;
+   sch_ep->repeat = 0;
+   }
+
+   if (ep_type == ISOC_IN_EP || ep_type == ISOC_OUT_EP) {
+   if (esit_pkts <= sch_ep->esit)
+   sch_ep->pkts = 1;
+   else
+   sch_ep->pkts = roundup_pow_of_two(esit_pkts)
+   / sch_ep->esit;


sch_ep->pkts now contain a rounded up average value of how many packets per 
microframe
would be transferred in case we'd transfer packets every microframe instead of 
every esit frame apart.


+
+   sch_ep->num_budget_microframes =
+   DIV_ROUND_UP(esit_pkts, sch_ep->pkts);


sch_ep->num_budget_microframes now contains the number of microframes during an 
esit needed
to transer all data if each microframe contains the average amount of data.

So basically we are trying to use as many microframes as possible with as few 
packets
per microframe as possible.

Did I understand this correctly?
How will devices react if they expect to get 16 packets every 16th microframe,
but they get one packet every microframe instead?

Or is this just theoretical bw calculations for the host and it will in the end 
do
whatever it wants with the information.

Otherwise, by making the changes Daniel Thompson suggested I think the xhci 
parts looks ready.

-Mathias

 

  
 



















+   } else if (is_fs_or_ls(udev->speed)) {
+
+   /*
+* usb_20 spec section11.18.4
+* assume worst cases
+*/
+   sch_ep->repeat = 0;
+   sch_ep->pkts = 1; /* at most one packet for each microframe */
+   if (ep_type == INT_IN_EP || ep_type == INT_OUT_EP) {
+   sch_ep->cs_count = 3; /* at most need 3 CS*/
+   /* one for SS and one for budgeted transaction */
+   sch_ep->num_budget_microframes = sch_ep->cs_count + 2;
+   sch_ep->bw_cost_per_microframe = max_packet_size;
+   }
+   if (ep_type == ISOC_OUT_EP) {
+
+   /*
+* the best case FS budget assumes that 188 FS bytes
+* occur in each microframe
+*/
+   sch_ep->num_budget_microframes = DIV_ROUND_UP(
+   max_packet_size, FS_PAYLOAD_MAX);
+   sch_ep->bw_cost_per_microframe = FS_PAYLOAD_MAX;
+   sch_ep->cs_count = sch_ep->num_budget_microframes;
+   }
+   if (ep_type == ISOC_IN_EP) {
+   /* at most need additional two CS. */
+   sch_ep->cs_count = DIV_ROUND_UP(
+   max_packet_size, FS_PAYLOAD_MAX) + 2;
+   sch_ep->num_budget_microframes = sch_ep->cs_count + 2;
+   sch_ep->bw_cost_per_microframe = FS_PAYLOAD_MAX;
+   }
+   }
+}
+
+/* Get maximum bandwidth when we schedule at offset slot. */
+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;
+   int i;
+   int j;
+
+   num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
+   for (i = 0; i < num_esit; i++) {
+   u32 base = offset + i * sch_ep->esit;
+
+   for (j = 0; j < sch_ep->num_budget_microframes; j++) {
+   if (sch_bw->bus_bw[base + j] > max_bw)
+   max_bw = sch_bw->bus_bw[base + j];
+   }
+   }
+   return max_bw;
+}
+
+static void 

Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller

[Daniel Thompson]

On 08/10/15 13:05, chunfeng yun wrote:

Hi,
On Thu, 2015-10-01 at 12:44 +0100, Daniel Thompson wrote:

On 29/09/15 04:01, Chunfeng Yun wrote:

There some vendor quirks for MTK xhci host controller:
1. It defines some extra SW scheduling parameters for HW
to minimize the scheduling effort for synchronous and
interrupt endpoints. The parameters are put into reseved
DWs of slot context and endpoint context.
2. Its IMODI unit for Interrupter Moderation register is
8 times as much as that defined in xHCI spec.
3. Its TDS in  Normal TRB defines a number of packets that
remains to be transferred for a TD after processing all
Max packets in all previous TRBs.

Signed-off-by: Chunfeng Yun 


I've done some basic soak tests, both with a directly attached USB3 HDD
and, given the extra code to manage isochronous xfer, also with a hub,
disc and two audio interfaces.

Tested-by: Daniel Thompson 



diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 57f40a1..243f696 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -68,6 +68,7 @@
   #include 
   #include "xhci.h"
   #include "xhci-trace.h"
+#include "xhci-mtk.h"

   /*
* Returns zero if the TRB isn't in this segment, otherwise it returns the 
DMA
@@ -3044,18 +3045,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
  struct urb *urb, unsigned int num_trbs_left)
   {
u32 maxp, total_packet_count;
+   u32 skip_current_trb = 0;


A bit of a nitpick but why do we need skip_current_trb? Testing
(xhci->quirks & XHCI_MTK_HOST) twice would make what the code does, and
why, more obvious.


I am not sure which way it is better, so add variable of
skip_current_trb to reduce test times.


I can't imagine either approach impacts performance. However introducing 
skip_current_trb makes the flow of the branches harder to follow.


The first branch was a version check and can remain so; we only need to 
do something special with the quirks because the MTK controller is 0.97 
but with a few bits added:


/* MTK xHCI is mostly 0.97 but contains some features from 1.0 */
if (xhci->hci_version < 0x100 && !(xhci->quirks & XHCI_MTK_HOST))
return ...

Setting trb_buff_len to zero is accommodating a quirk and this is 
clearer when we have:


/* for MTK xHCI, TD size doesn't include this TRB */
if (xhci->quirks & XHCI_MTK_HOST)
trb_buff_len = 0;



Anyhow with that looked at, and the caveat that I'm not much of USB
expert, you're welcome to add my Reviewed-by: to v10.



Thanks a lot


No worries.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller

[chunfeng yun]

Hi,
On Thu, 2015-10-01 at 12:44 +0100, Daniel Thompson wrote:
> On 29/09/15 04:01, Chunfeng Yun wrote:
> > There some vendor quirks for MTK xhci host controller:
> > 1. It defines some extra SW scheduling parameters for HW
> >to minimize the scheduling effort for synchronous and
> >interrupt endpoints. The parameters are put into reseved
> >DWs of slot context and endpoint context.
> > 2. Its IMODI unit for Interrupter Moderation register is
> >8 times as much as that defined in xHCI spec.
> > 3. Its TDS in  Normal TRB defines a number of packets that
> >remains to be transferred for a TD after processing all
> >Max packets in all previous TRBs.
> >
> > Signed-off-by: Chunfeng Yun 
> 
> I've done some basic soak tests, both with a directly attached USB3 HDD 
> and, given the extra code to manage isochronous xfer, also with a hub, 
> disc and two audio interfaces.
> 
> Tested-by: Daniel Thompson 
> 
> 
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 57f40a1..243f696 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -68,6 +68,7 @@
> >   #include 
> >   #include "xhci.h"
> >   #include "xhci-trace.h"
> > +#include "xhci-mtk.h"
> >
> >   /*
> >* Returns zero if the TRB isn't in this segment, otherwise it returns 
> > the DMA
> > @@ -3044,18 +3045,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, 
> > int transferred,
> >   struct urb *urb, unsigned int num_trbs_left)
> >   {
> > u32 maxp, total_packet_count;
> > +   u32 skip_current_trb = 0;
> 
> A bit of a nitpick but why do we need skip_current_trb? Testing 
> (xhci->quirks & XHCI_MTK_HOST) twice would make what the code does, and 
> why, more obvious.
> 
I am not sure which way it is better, so add variable of
skip_current_trb to reduce test times.

> Anyhow with that looked at, and the caveat that I'm not much of USB 
> expert, you're welcome to add my Reviewed-by: to v10.
> 

Thanks a lot
> >
> > -   if (xhci->hci_version < 0x100)
> > -   return ((td_total_len - transferred) >> 10);
> > -
> > -   maxp = GET_MAX_PACKET(usb_endpoint_maxp(>ep->desc));
> > -   total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
> > +   if (xhci->hci_version < 0x100) {
> > +   /* for MTK xHCI, TD size doesn't include this TRB */
> > +   if (xhci->quirks & XHCI_MTK_HOST)
> > +   skip_current_trb = 1;
> > +   else
> > +   return ((td_total_len - transferred) >> 10);
> > +   }
> >
> > /* One TRB with a zero-length data packet. */
> > if (num_trbs_left == 0 || (transferred == 0 && trb_buff_len == 0) ||
> > trb_buff_len == td_total_len)
> > return 0;
> >
> > +   if (skip_current_trb)
> > +   trb_buff_len = 0;
> > +
> > +   maxp = GET_MAX_PACKET(usb_endpoint_maxp(>ep->desc));
> > +   total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
> > +
> > /* Queueing functions don't count the current TRB into transferred */
> > return (total_packet_count - ((transferred + trb_buff_len) / maxp));
> >   }
> 
> 
> Daniel.
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller

[Mathias Nyman]

On 08.10.2015 15:28, Daniel Thompson wrote:

On 08/10/15 13:05, chunfeng yun wrote:

Hi,
On Thu, 2015-10-01 at 12:44 +0100, Daniel Thompson wrote:

On 29/09/15 04:01, Chunfeng Yun wrote:

There some vendor quirks for MTK xhci host controller:
1. It defines some extra SW scheduling parameters for HW
to minimize the scheduling effort for synchronous and
interrupt endpoints. The parameters are put into reseved
DWs of slot context and endpoint context.
2. Its IMODI unit for Interrupter Moderation register is
8 times as much as that defined in xHCI spec.
3. Its TDS in  Normal TRB defines a number of packets that
remains to be transferred for a TD after processing all
Max packets in all previous TRBs.

Signed-off-by: Chunfeng Yun 


I've done some basic soak tests, both with a directly attached USB3 HDD
and, given the extra code to manage isochronous xfer, also with a hub,
disc and two audio interfaces.

Tested-by: Daniel Thompson 



diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 57f40a1..243f696 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -68,6 +68,7 @@
   #include 
   #include "xhci.h"
   #include "xhci-trace.h"
+#include "xhci-mtk.h"

   /*
* Returns zero if the TRB isn't in this segment, otherwise it returns the 
DMA
@@ -3044,18 +3045,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
 struct urb *urb, unsigned int num_trbs_left)
   {
   u32 maxp, total_packet_count;
+u32 skip_current_trb = 0;


A bit of a nitpick but why do we need skip_current_trb? Testing
(xhci->quirks & XHCI_MTK_HOST) twice would make what the code does, and
why, more obvious.


I am not sure which way it is better, so add variable of
skip_current_trb to reduce test times.


I can't imagine either approach impacts performance. However introducing 
skip_current_trb makes the flow of the branches harder to follow.

The first branch was a version check and can remain so; we only need to do 
something special with the quirks because the MTK controller is 0.97 but with a 
few bits added:

 /* MTK xHCI is mostly 0.97 but contains some features from 1.0 */
 if (xhci->hci_version < 0x100 && !(xhci->quirks & XHCI_MTK_HOST))
 return ...

Setting trb_buff_len to zero is accommodating a quirk and this is clearer when 
we have:

 /* for MTK xHCI, TD size doesn't include this TRB */
 if (xhci->quirks & XHCI_MTK_HOST)
 trb_buff_len = 0;


Both will work, but I'd prefer this solution as well.

-Mathias

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller

[chunfeng yun]

On Thu, 2015-10-08 at 13:28 +0100, Daniel Thompson wrote:
> On 08/10/15 13:05, chunfeng yun wrote:
> > Hi,
> > On Thu, 2015-10-01 at 12:44 +0100, Daniel Thompson wrote:
> >> On 29/09/15 04:01, Chunfeng Yun wrote:
> >>> There some vendor quirks for MTK xhci host controller:
> >>> 1. It defines some extra SW scheduling parameters for HW
> >>> to minimize the scheduling effort for synchronous and
> >>> interrupt endpoints. The parameters are put into reseved
> >>> DWs of slot context and endpoint context.
> >>> 2. Its IMODI unit for Interrupter Moderation register is
> >>> 8 times as much as that defined in xHCI spec.
> >>> 3. Its TDS in  Normal TRB defines a number of packets that
> >>> remains to be transferred for a TD after processing all
> >>> Max packets in all previous TRBs.
> >>>
> >>> Signed-off-by: Chunfeng Yun 
> >>
> >> I've done some basic soak tests, both with a directly attached USB3 HDD
> >> and, given the extra code to manage isochronous xfer, also with a hub,
> >> disc and two audio interfaces.
> >>
> >> Tested-by: Daniel Thompson 
> >>
> >>
> >>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> >>> index 57f40a1..243f696 100644
> >>> --- a/drivers/usb/host/xhci-ring.c
> >>> +++ b/drivers/usb/host/xhci-ring.c
> >>> @@ -68,6 +68,7 @@
> >>>#include 
> >>>#include "xhci.h"
> >>>#include "xhci-trace.h"
> >>> +#include "xhci-mtk.h"
> >>>
> >>>/*
> >>> * Returns zero if the TRB isn't in this segment, otherwise it returns 
> >>> the DMA
> >>> @@ -3044,18 +3045,27 @@ static u32 xhci_td_remainder(struct xhci_hcd 
> >>> *xhci, int transferred,
> >>> struct urb *urb, unsigned int 
> >>> num_trbs_left)
> >>>{
> >>>   u32 maxp, total_packet_count;
> >>> + u32 skip_current_trb = 0;
> >>
> >> A bit of a nitpick but why do we need skip_current_trb? Testing
> >> (xhci->quirks & XHCI_MTK_HOST) twice would make what the code does, and
> >> why, more obvious.
> >>
> > I am not sure which way it is better, so add variable of
> > skip_current_trb to reduce test times.
> 
> I can't imagine either approach impacts performance. However introducing 
> skip_current_trb makes the flow of the branches harder to follow.
> 
> The first branch was a version check and can remain so; we only need to 
> do something special with the quirks because the MTK controller is 0.97 
> but with a few bits added:
> 
>  /* MTK xHCI is mostly 0.97 but contains some features from 1.0 */
>  if (xhci->hci_version < 0x100 && !(xhci->quirks & XHCI_MTK_HOST))
>   return ...
> 
> Setting trb_buff_len to zero is accommodating a quirk and this is 
> clearer when we have:
> 
>  /* for MTK xHCI, TD size doesn't include this TRB */
>  if (xhci->quirks & XHCI_MTK_HOST)
>   trb_buff_len = 0;
> 
I will revise the code according to your suggestion, thanks

> 
> >> Anyhow with that looked at, and the caveat that I'm not much of USB
> >> expert, you're welcome to add my Reviewed-by: to v10.
> >>
> >
> > Thanks a lot
> 
> No worries.
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller

[chunfeng yun]

Hi,
On Thu, 2015-10-08 at 16:05 +0300, Mathias Nyman wrote:
> On 08.10.2015 15:28, Daniel Thompson wrote:
> > On 08/10/15 13:05, chunfeng yun wrote:
> >> Hi,
> >> On Thu, 2015-10-01 at 12:44 +0100, Daniel Thompson wrote:
> >>> On 29/09/15 04:01, Chunfeng Yun wrote:
>  There some vendor quirks for MTK xhci host controller:
>  1. It defines some extra SW scheduling parameters for HW
>  to minimize the scheduling effort for synchronous and
>  interrupt endpoints. The parameters are put into reseved
>  DWs of slot context and endpoint context.
>  2. Its IMODI unit for Interrupter Moderation register is
>  8 times as much as that defined in xHCI spec.
>  3. Its TDS in  Normal TRB defines a number of packets that
>  remains to be transferred for a TD after processing all
>  Max packets in all previous TRBs.
> 
>  Signed-off-by: Chunfeng Yun 
> >>>
> >>> I've done some basic soak tests, both with a directly attached USB3 HDD
> >>> and, given the extra code to manage isochronous xfer, also with a hub,
> >>> disc and two audio interfaces.
> >>>
> >>> Tested-by: Daniel Thompson 
> >>>
> >>>
>  diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>  index 57f40a1..243f696 100644
>  --- a/drivers/usb/host/xhci-ring.c
>  +++ b/drivers/usb/host/xhci-ring.c
>  @@ -68,6 +68,7 @@
> #include 
> #include "xhci.h"
> #include "xhci-trace.h"
>  +#include "xhci-mtk.h"
> 
> /*
>  * Returns zero if the TRB isn't in this segment, otherwise it 
>  returns the DMA
>  @@ -3044,18 +3045,27 @@ static u32 xhci_td_remainder(struct xhci_hcd 
>  *xhci, int transferred,
>   struct urb *urb, unsigned int num_trbs_left)
> {
> u32 maxp, total_packet_count;
>  +u32 skip_current_trb = 0;
> >>>
> >>> A bit of a nitpick but why do we need skip_current_trb? Testing
> >>> (xhci->quirks & XHCI_MTK_HOST) twice would make what the code does, and
> >>> why, more obvious.
> >>>
> >> I am not sure which way it is better, so add variable of
> >> skip_current_trb to reduce test times.
> >
> > I can't imagine either approach impacts performance. However introducing 
> > skip_current_trb makes the flow of the branches harder to follow.
> >
> > The first branch was a version check and can remain so; we only need to do 
> > something special with the quirks because the MTK controller is 0.97 but 
> > with a few bits added:
> >
> >  /* MTK xHCI is mostly 0.97 but contains some features from 1.0 */
> >  if (xhci->hci_version < 0x100 && !(xhci->quirks & XHCI_MTK_HOST))
> >  return ...
> >
> > Setting trb_buff_len to zero is accommodating a quirk and this is clearer 
> > when we have:
> >
> >  /* for MTK xHCI, TD size doesn't include this TRB */
> >  if (xhci->quirks & XHCI_MTK_HOST)
> >  trb_buff_len = 0;
> 
> Both will work, but I'd prefer this solution as well.
> 
Thanks for your suggestion as well

> -Mathias
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller

[Daniel Thompson]

On 29/09/15 04:01, Chunfeng Yun wrote:

There some vendor quirks for MTK xhci host controller:
1. It defines some extra SW scheduling parameters for HW
   to minimize the scheduling effort for synchronous and
   interrupt endpoints. The parameters are put into reseved
   DWs of slot context and endpoint context.
2. Its IMODI unit for Interrupter Moderation register is
   8 times as much as that defined in xHCI spec.
3. Its TDS in  Normal TRB defines a number of packets that
   remains to be transferred for a TD after processing all
   Max packets in all previous TRBs.

Signed-off-by: Chunfeng Yun 


I've done some basic soak tests, both with a directly attached USB3 HDD 
and, given the extra code to manage isochronous xfer, also with a hub, 
disc and two audio interfaces.


Tested-by: Daniel Thompson 



diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 57f40a1..243f696 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -68,6 +68,7 @@
  #include 
  #include "xhci.h"
  #include "xhci-trace.h"
+#include "xhci-mtk.h"

  /*
   * Returns zero if the TRB isn't in this segment, otherwise it returns the DMA
@@ -3044,18 +3045,27 @@ static u32 xhci_td_remainder(struct xhci_hcd *xhci, int 
transferred,
  struct urb *urb, unsigned int num_trbs_left)
  {
u32 maxp, total_packet_count;
+   u32 skip_current_trb = 0;


A bit of a nitpick but why do we need skip_current_trb? Testing 
(xhci->quirks & XHCI_MTK_HOST) twice would make what the code does, and 
why, more obvious.


Anyhow with that looked at, and the caveat that I'm not much of USB 
expert, you're welcome to add my Reviewed-by: to v10.




-   if (xhci->hci_version < 0x100)
-   return ((td_total_len - transferred) >> 10);
-
-   maxp = GET_MAX_PACKET(usb_endpoint_maxp(>ep->desc));
-   total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
+   if (xhci->hci_version < 0x100) {
+   /* for MTK xHCI, TD size doesn't include this TRB */
+   if (xhci->quirks & XHCI_MTK_HOST)
+   skip_current_trb = 1;
+   else
+   return ((td_total_len - transferred) >> 10);
+   }

/* One TRB with a zero-length data packet. */
if (num_trbs_left == 0 || (transferred == 0 && trb_buff_len == 0) ||
trb_buff_len == td_total_len)
return 0;

+   if (skip_current_trb)
+   trb_buff_len = 0;
+
+   maxp = GET_MAX_PACKET(usb_endpoint_maxp(>ep->desc));
+   total_packet_count = DIV_ROUND_UP(td_total_len, maxp);
+
/* Queueing functions don't count the current TRB into transferred */
return (total_packet_count - ((transferred + trb_buff_len) / maxp));
  }



Daniel.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html