Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller
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
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
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
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, FS_PAYLOAD_MAX) + 2;
> > +
Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller
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 update_bus_bw(struct mu3h_sch_
Re: [PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller
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
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
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
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
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(&urb->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(&urb->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
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(&urb->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(&urb->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
[PATCH v9 4/5] xhci: mediatek: support MTK xHCI host controller
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
---
drivers/usb/host/Kconfig| 9 +
drivers/usb/host/Makefile | 4 +
drivers/usb/host/xhci-mtk-sch.c | 424 ++
drivers/usb/host/xhci-mtk.c | 776
drivers/usb/host/xhci-mtk.h | 156
drivers/usb/host/xhci-ring.c| 22 +-
drivers/usb/host/xhci.c | 19 +-
drivers/usb/host/xhci.h | 1 +
8 files changed, 1404 insertions(+), 7 deletions(-)
create mode 100644 drivers/usb/host/xhci-mtk-sch.c
create mode 100644 drivers/usb/host/xhci-mtk.c
create mode 100644 drivers/usb/host/xhci-mtk.h
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 079991e..4674118 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -41,6 +41,15 @@ config USB_XHCI_PLATFORM
If unsure, say N.
+config USB_XHCI_MTK
+ tristate "xHCI support for Mediatek MT65xx"
+ select MFD_SYSCON
+ depends on ARCH_MEDIATEK || COMPILE_TEST
+ ---help---
+ Say 'Y' to enable the support for the xHCI host controller
+ found in Mediatek MT65xx SoCs.
+ If unsure, say N.
+
config USB_XHCI_MVEBU
tristate "xHCI support for Marvell Armada 375/38x"
select USB_XHCI_PLATFORM
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 754efaa..00401f9 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -13,6 +13,9 @@ fhci-$(CONFIG_FHCI_DEBUG) += fhci-dbg.o
xhci-hcd-y := xhci.o xhci-mem.o
xhci-hcd-y += xhci-ring.o xhci-hub.o xhci-dbg.o
xhci-hcd-y += xhci-trace.o
+ifneq ($(CONFIG_USB_XHCI_MTK), )
+ xhci-hcd-y += xhci-mtk-sch.o
+endif
xhci-plat-hcd-y := xhci-plat.o
ifneq ($(CONFIG_USB_XHCI_MVEBU), )
@@ -30,6 +33,7 @@ endif
obj-$(CONFIG_USB_XHCI_PCI) += xhci-pci.o
obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
+obj-$(CONFIG_USB_XHCI_MTK) += xhci-mtk.o
obj-$(CONFIG_USB_EHCI_HCD) += ehci-hcd.o
obj-$(CONFIG_USB_EHCI_PCI) += ehci-pci.o
diff --git a/drivers/usb/host/xhci-mtk-sch.c b/drivers/usb/host/xhci-mtk-sch.c
new file mode 100644
index 000..163bc6d
--- /dev/null
+++ b/drivers/usb/host/xhci-mtk-sch.c
@@ -0,0 +1,424 @@
+/*
+ * Copyright (c) 2015 MediaTek Inc.
+ * Author:
+ * Zhigang.Wei
+ * Chunfeng.Yun
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include
+#include
+#include
+
+#include "xhci.h"
+#include "xhci-mtk.h"
+
+#define SS_BW_BOUNDARY 51000
+/* table 5-5. High-speed Isoc Transaction Limits in usb_20 spec */
+#define HS_BW_BOUNDARY 6144
+/* usb2 spec section11.18.1: at most 188 FS bytes per microframe */
+#define FS_PAYLOAD_MAX 188
+
+/* mtk scheduler bitmasks */
+#define EP_BPKTS(p)((p) & 0x3f)
+#define EP_BCSCOUNT(p) (((p) & 0x7) << 8)
+#define EP_BBM(p) ((p) << 11)
+#define EP_BOFFSET(p) ((p) & 0x3fff)
+#define EP_BREPEAT(p) (((p) & 0x7fff) << 16)
+
+static int is_fs_or_ls(enum usb_device_speed speed)
+{
+ return speed == USB_SPEED_FULL || speed == USB_SPEED_LOW;
+}
+
+/*
+* get the index of bandwidth domains array which @ep belongs to.
+*
+* the bandwidth domain array is saved to @sch_array of struct xhci_hcd_mtk,
+* each HS root port is treated as a single bandwidth domain,
+* but each SS root port is treated as two bandwidth domains, one for IN eps,
+* one for OUT eps.
+* @real_port value is defined as follow according to xHCI spec:
+* 1 for SSport0, ..., N+1 for SSportN, N+2 for HSport0, N+3 for HSport1, etc
+* so the bandwidth domain array is organized as follow for simplification:
+* SSport0-OUT, SSport0-IN, ..., SSportX-OUT, SSportX-IN, HSport0, ..., HSportY
+*/
+static int get_bw_index(struct xhci_hcd *xhci, struct usb_device *udev,
+ struct usb_host_endpoint *ep)
+{
+ struct xhci_virt_device *virt_dev;
+ int bw_index;
+
+ virt_dev = xhci->devs[udev->slot_id];
+
+ if (udev->speed == USB_SPEED_SUPER) {
+ if (usb_endpoint_dir_out(&ep->desc))
+ bw_index = (virt_dev->real_port - 1) * 2;
+
