Re: spec violation of xHCI?
On 12/12/13 08:15, Hans Petter Selasky wrote: On 12/12/13 01:59, Kohji Okuno wrote: Hi Kohji, Did you check using a USB analyzer what the difference is when setting the CHAIN bit and not setting the chain bit? I would guess that if you set the CHAIN-bit in this case, no ZLP will be sent, because the TRB is associated with the previous one. What endpoint type is this? BULK/CONTROL/INTR/ISOC What direction is this? IN or OUT? --HPS Hi Kohji, If there is no CHAIN bit in the IN-direction, my TD chain will receive multiple short packets. This is of course not correct. This only happens if you have very large buffers above 64KByte that don't fit in a single TD. For OUT direction: There are no short packets received, though errors can happen and those will stop the endpoint. Are we sure that by setting the CHAIN bit, that the HC's will transfer ZLP's if the length of the second last "TD" is a multiple of wMaxPacketSize and the last one has a length of zero bytes? I will do some testing to verify the corner cases. Thank you! --HPS ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: spec violation of xHCI?
From: Hans Petter Selasky Date: Thu, 12 Dec 2013 08:15:02 +0100 > On 12/12/13 01:59, Kohji Okuno wrote: >> From: Hans Petter Selasky >> Date: Wed, 11 Dec 2013 15:04:42 +0100 >>> On 12/11/13 14:06, Kohji Okuno wrote: Hi HPS, All link trbs which are not the end need CHAIN bit, I think. But, this is errata in xHCI ver 0.95. So, linux has quirk for chain bit. Could you check linux codes? Regards, Kohji Okuno >>> >>> Hi Kohji, >>> >>> I went through the Linux codes a bit, and I see they have some quirks for >>> the >>> chaining bit. Unfortunately Linux does the queuing quite differently than >>> in >>> FreeBSD and Shara Sharp which is the author of that code, stated recently a >>> need for rewrite of the TRB/TD stuff in Linux, so I'm not sure if that >>> means >>> there are more bugs in there or not. Let me explain a bit how things work >>> in >>> FreeBSD and why I did not put the chaining bit in line 1895 which you >>> suggest. >>> >>> In my design the chaining bit should not be set at line 1895, because if >>> you >>> receive a short packet and nframes > 1, the XHCI should not go to the end >>> of >>> the frame list, but rather the next frame, nframes + 1. >>> >>> If the single short OK flag is set on a BULK transfer, yes, it would be >>> correct to set the chaining bit here, but it is not required, because we >>> are >>> already are handling activation of the next frame in the function >>> "xhci_activate_transfer()" and "xhci_skip_transfer()". Transfer here means >>> zero or more TRBs. We use the cycle bit on the TRB to single step the >>> frames >>> in software, although you are right that we might optimise this by setting >>> the >>> chaining bit instead for the BULK case so that we don't need software >>> intervention to handle the job. >>> >>> If the multi short OK flag is set, multiple short terminated frames can be >>> received and then the chaining bit should not be set. >>> >>> Are you seeing a real problem because of the chain bit not being set, or is >>> this more the result of code review? >>> >>> Thank you for the interest in my XHCI driver. >>> >>> --HPS >> >> Hi HPS, >> >> Unfortunately, I can not explain in detail. But, I encountered a real >> problem for ZLP. And, when I added CHAIN bit, this problem was >> resolved. >> >> When a device driver needs force_short(ZLP), your device driver >> creates TRB in the followings. >> >> NORMAL_TRB- LINK_TRB - NORMAL_TRB - LINK_TRB => Kick DOORBELL >> (with payload) (#1) (ZLP)(#2) >> >> In this case, I think LINK_TRB #1 needs chain bit. > > Hi Kohji, > > Did you check using a USB analyzer what the difference is when setting the > CHAIN bit and not setting the chain bit? > > I would guess that if you set the CHAIN-bit in this case, no ZLP will be sent, > because the TRB is associated with the previous one. > > What endpoint type is this? BULK/CONTROL/INTR/ISOC > > What direction is this? IN or OUT? > > --HPS Hi HPS, The endpoint type is BULK, and the direction is OUT. I checked by using a USB analyzer. When I did not set CHAIN bit in LINK TRB, my host controller sent illegal packets sometimes. But, ZLPs were sent. Regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: spec violation of xHCI?
On 12/12/13 01:59, Kohji Okuno wrote: From: Hans Petter Selasky Date: Wed, 11 Dec 2013 15:04:42 +0100 On 12/11/13 14:06, Kohji Okuno wrote: Hi HPS, All link trbs which are not the end need CHAIN bit, I think. But, this is errata in xHCI ver 0.95. So, linux has quirk for chain bit. Could you check linux codes? Regards, Kohji Okuno Hi Kohji, I went through the Linux codes a bit, and I see they have some quirks for the chaining bit. Unfortunately Linux does the queuing quite differently than in FreeBSD and Shara Sharp which is the author of that code, stated recently a need for rewrite of the TRB/TD stuff in Linux, so I'm not sure if that means there are more bugs in there or not. Let me explain a bit how things work in FreeBSD and why I did not put the chaining bit in line 1895 which you suggest. In my design the chaining bit should not be set at line 1895, because if you receive a short packet and nframes > 1, the XHCI should not go to the end of the frame list, but rather the next frame, nframes + 1. If the single short OK flag is set on a BULK transfer, yes, it would be correct to set the chaining bit here, but it is not required, because we are already are handling activation of the next frame in the function "xhci_activate_transfer()" and "xhci_skip_transfer()". Transfer here means zero or more TRBs. We use the cycle bit on the TRB to single step the frames in software, although you are right that we might optimise this by setting the chaining bit instead for the BULK case so that we don't need software intervention to handle the job. If the multi short OK flag is set, multiple short terminated frames can be received and then the chaining bit should not be set. Are you seeing a real problem because of the chain bit not being set, or is this more the result of code review? Thank you for the interest in my XHCI driver. --HPS Hi HPS, Unfortunately, I can not explain in detail. But, I encountered a real problem for ZLP. And, when I added CHAIN bit, this problem was resolved. When a device driver needs force_short(ZLP), your device driver creates TRB in the followings. NORMAL_TRB- LINK_TRB - NORMAL_TRB - LINK_TRB => Kick DOORBELL (with payload) (#1) (ZLP)(#2) In this case, I think LINK_TRB #1 needs chain bit. Hi Kohji, Did you check using a USB analyzer what the difference is when setting the CHAIN bit and not setting the chain bit? I would guess that if you set the CHAIN-bit in this case, no ZLP will be sent, because the TRB is associated with the previous one. What endpoint type is this? BULK/CONTROL/INTR/ISOC What direction is this? IN or OUT? --HPS ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Request for testing an alternate branch
On Dec 11, 2013, at 1:26 PM, John Baldwin wrote: > Also, I'm still not a fan of the EAGAIN approach. I'd rather have a method > in bus_if.m to suspend or resume a single device and to track that a device > is suspended or resumed via a device_t flag or some such. (I think I had > suggested this previously as it would also allow us to have a tool to > suspend/resume individual drivers at runtime apart from a full suspend/resume > request). Anything that made it easier to test suspend/resume would be a huge bonus. Tim ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: spec violation of xHCI?
From: Hans Petter Selasky Date: Wed, 11 Dec 2013 15:04:42 +0100 > On 12/11/13 14:06, Kohji Okuno wrote: >> >> Hi HPS, >> >> All link trbs which are not the end need CHAIN bit, I think. >> But, this is errata in xHCI ver 0.95. So, linux has quirk for chain >> bit. Could you check linux codes? >> >> Regards, >> Kohji Okuno > > Hi Kohji, > > I went through the Linux codes a bit, and I see they have some quirks for the > chaining bit. Unfortunately Linux does the queuing quite differently than in > FreeBSD and Shara Sharp which is the author of that code, stated recently a > need for rewrite of the TRB/TD stuff in Linux, so I'm not sure if that means > there are more bugs in there or not. Let me explain a bit how things work in > FreeBSD and why I did not put the chaining bit in line 1895 which you suggest. > > In my design the chaining bit should not be set at line 1895, because if you > receive a short packet and nframes > 1, the XHCI should not go to the end of > the frame list, but rather the next frame, nframes + 1. > > If the single short OK flag is set on a BULK transfer, yes, it would be > correct to set the chaining bit here, but it is not required, because we are > already are handling activation of the next frame in the function > "xhci_activate_transfer()" and "xhci_skip_transfer()". Transfer here means > zero or more TRBs. We use the cycle bit on the TRB to single step the frames > in software, although you are right that we might optimise this by setting the > chaining bit instead for the BULK case so that we don't need software > intervention to handle the job. > > If the multi short OK flag is set, multiple short terminated frames can be > received and then the chaining bit should not be set. > > Are you seeing a real problem because of the chain bit not being set, or is > this more the result of code review? > > Thank you for the interest in my XHCI driver. > > --HPS Hi HPS, Unfortunately, I can not explain in detail. But, I encountered a real problem for ZLP. And, when I added CHAIN bit, this problem was resolved. When a device driver needs force_short(ZLP), your device driver creates TRB in the followings. NORMAL_TRB- LINK_TRB - NORMAL_TRB - LINK_TRB => Kick DOORBELL (with payload) (#1) (ZLP)(#2) In this case, I think LINK_TRB #1 needs chain bit. Regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Request for testing an alternate branch
On Wed, Dec 11, 2013 at 1:26 PM, John Baldwin wrote: > On Thursday, December 05, 2013 1:21:13 am Justin Hibbits wrote: >> I've been working on the projects/pmac_pmu branch for some time now to >> add suspend/resume as well as CPU speed change for certain PowerPC >> machines, about a year since I created the branch, and now it's stable >> enough that I want to merge it into HEAD, hence this request. However, >> it does touch several drivers, turning them into "early drivers", such >> that they can be initialized, and suspended and resumed at a different >> time. Saying that, I do need testing from other architectures, to make >> sure I haven't broken anything. >> >> The technical details: >> >> To get proper ordering, I've extended the bus_generic_suspend() and >> bus_generic_resume() to do multiple passes. Devices which cannot be >> enabled or disabled at the current pass level would return an EAGAIN. >> This could possibly cause problems, since it's an addition to an >> existing API rather than a new API to run along side it, so it needs a >> great deal of testing. It works fine on PowerPC, but I don't have any >> i386/amd64 or sparc64 hardware to test it on, so would like others who >> do to test it. I don't think that it would impact x86 at all (testing >> is obviously required), because the nexus is not an EARLY_DRIVER_MODULE, >> so all devices would be handled at the same pass. But, I do know the >> sparc64 has an EARLY_DRIVER_MODULE() nexus, so that will likely be >> impacted. > > I have patches to change many x86 drivers to use EARLY_DRIVER_MODULE() > FWIW. > > Also, I'm still not a fan of the EAGAIN approach. I'd rather have a method > in bus_if.m to suspend or resume a single device and to track that a device > is suspended or resumed via a device_t flag or some such. (I think I had > suggested this previously as it would also allow us to have a tool to > suspend/resume individual drivers at runtime apart from a full suspend/resume > request). > > -- > John Baldwin Understood. You had mentioned something along those lines before. Is it safe/sane to partially merge a branch into HEAD? If so, I can merge only the changes necessary for PMU cpufreq, and work on the suspend/resume separately. I put them together because most of the low level code involved is the same between them. I do like your idea of a device_t flag, but I'm not sure what the best way to go with that would be. I do already use a device_t flag to determine if the device is suspended, but only bus_generic_* access that in this patch. Would a better way be: * root_suspend instead of suspending its children, instead traverses and suspends each descendent in reverse order. * While doing this, insert each device upon successful suspend into a list. * For root_resume(), traverse the list back, and suspend each device in the reverse order. With this, add a new method, called device_suspend_child() to suspend a specific child if it hasn't already been suspended. * This could require modifying the PCI driver to move the device child suspend/resume into those functions, instead of doing that logic in the device_suspend/device_resume itself. I guess, in short, I'm asking: Is it fine if I merge only the code necessary for this cpufreq? That would require making other changes to this before merging in, to isolate that code, but it's very doable. - Justin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Request for testing an alternate branch
On Thursday, December 05, 2013 1:21:13 am Justin Hibbits wrote: > I've been working on the projects/pmac_pmu branch for some time now to > add suspend/resume as well as CPU speed change for certain PowerPC > machines, about a year since I created the branch, and now it's stable > enough that I want to merge it into HEAD, hence this request. However, > it does touch several drivers, turning them into "early drivers", such > that they can be initialized, and suspended and resumed at a different > time. Saying that, I do need testing from other architectures, to make > sure I haven't broken anything. > > The technical details: > > To get proper ordering, I've extended the bus_generic_suspend() and > bus_generic_resume() to do multiple passes. Devices which cannot be > enabled or disabled at the current pass level would return an EAGAIN. > This could possibly cause problems, since it's an addition to an > existing API rather than a new API to run along side it, so it needs a > great deal of testing. It works fine on PowerPC, but I don't have any > i386/amd64 or sparc64 hardware to test it on, so would like others who > do to test it. I don't think that it would impact x86 at all (testing > is obviously required), because the nexus is not an EARLY_DRIVER_MODULE, > so all devices would be handled at the same pass. But, I do know the > sparc64 has an EARLY_DRIVER_MODULE() nexus, so that will likely be > impacted. I have patches to change many x86 drivers to use EARLY_DRIVER_MODULE() FWIW. Also, I'm still not a fan of the EAGAIN approach. I'd rather have a method in bus_if.m to suspend or resume a single device and to track that a device is suspended or resumed via a device_t flag or some such. (I think I had suggested this previously as it would also allow us to have a tool to suspend/resume individual drivers at runtime apart from a full suspend/resume request). -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: svn commit: r259221 - head/sys/dev/vt
This looks like it fixes my -CURRENT crash..
Thanks GNN/JHB
On Wed, Dec 11, 2013 at 05:18:10PM +, George V. Neville-Neil wrote:
> Author: gnn
> Date: Wed Dec 11 17:18:10 2013
> New Revision: 259221
> URL: http://svnweb.freebsd.org/changeset/base/259221
>
> Log:
> Fix a panic when booting with kernels that have FREEBBSD_COMPAT
> 4, 5, 6 or 43 by only thunking the data parameter for old ioctls
> compatability ioctls instead of doing it for all of them.
>
> Submitted by: jhb@
>
> Modified:
> head/sys/dev/vt/vt_core.c
>
> Modified: head/sys/dev/vt/vt_core.c
> ==
> --- head/sys/dev/vt/vt_core.c Wed Dec 11 15:32:28 2013(r259220)
> +++ head/sys/dev/vt/vt_core.c Wed Dec 11 17:18:10 2013(r259221)
> @@ -1321,9 +1321,12 @@ vtterm_ioctl(struct terminal *tm, u_long
> case _IO('c', 110):
> cmd = CONS_SETKBD;
> break;
> + default:
> + goto skip_thunk;
> }
> ival = IOCPARM_IVAL(data);
> data = (caddr_t)&ival;
> +skip_thunk:
> #endif
>
> switch (cmd) {
> ___
> svn-src-...@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/svn-src-all
> To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
--
Larry Rosenman http://www.lerctr.org/~ler
Phone: +1 214-642-9640 E-Mail: l...@lerctr.org
US Mail: 108 Turvey Cove, Hutto, TX 78634-5688
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: spec violation of xHCI?
On 12/11/13 14:06, Kohji Okuno wrote: Hi HPS, All link trbs which are not the end need CHAIN bit, I think. But, this is errata in xHCI ver 0.95. So, linux has quirk for chain bit. Could you check linux codes? Regards, Kohji Okuno Hi Kohji, I went through the Linux codes a bit, and I see they have some quirks for the chaining bit. Unfortunately Linux does the queuing quite differently than in FreeBSD and Shara Sharp which is the author of that code, stated recently a need for rewrite of the TRB/TD stuff in Linux, so I'm not sure if that means there are more bugs in there or not. Let me explain a bit how things work in FreeBSD and why I did not put the chaining bit in line 1895 which you suggest. In my design the chaining bit should not be set at line 1895, because if you receive a short packet and nframes > 1, the XHCI should not go to the end of the frame list, but rather the next frame, nframes + 1. If the single short OK flag is set on a BULK transfer, yes, it would be correct to set the chaining bit here, but it is not required, because we are already are handling activation of the next frame in the function "xhci_activate_transfer()" and "xhci_skip_transfer()". Transfer here means zero or more TRBs. We use the cycle bit on the TRB to single step the frames in software, although you are right that we might optimise this by setting the chaining bit instead for the BULK case so that we don't need software intervention to handle the job. If the multi short OK flag is set, multiple short terminated frames can be received and then the chaining bit should not be set. Are you seeing a real problem because of the chain bit not being set, or is this more the result of code review? Thank you for the interest in my XHCI driver. --HPS ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: svn merge to stable/10 has lotsa mergeinfo
On Wed, Dec 11, 2013 at 8:53 AM, Julian Elischer wrote: > On 12/11/13, 8:24 AM, Eitan Adler wrote: >> >> On Tue, Dec 10, 2013 at 7:04 PM, Rick Macklem >> wrote: >>> >>> Hi, >>> >>> I just tried to MFC into stable/10 and it worked, but with >>> a lot of mergeinfo. I know diddly about svn, so is this ok? >> >> Starting with stable/10 and later you must merge into the *root*, not into >> sys/. >> >> P.S., with svn, it can be very helpful to provide the exact commands you >> used. >> >> >> > so how about you tell people what you call root? base? releng? 8? sys? the > place you did the checkout to? Details here: http://www.freebsd.org/doc/en/articles/committers-guide/article.html#merge If this not clear please let me know. -- Eitan Adler ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: svn merge to stable/10 has lotsa mergeinfo
On 12/11/13, 8:24 AM, Eitan Adler wrote: On Tue, Dec 10, 2013 at 7:04 PM, Rick Macklem wrote: Hi, I just tried to MFC into stable/10 and it worked, but with a lot of mergeinfo. I know diddly about svn, so is this ok? Starting with stable/10 and later you must merge into the *root*, not into sys/. P.S., with svn, it can be very helpful to provide the exact commands you used. so how about you tell people what you call root? base? releng? 8? sys? the place you did the checkout to? Experts always assume that everyone has the decoder rings. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: Request for testing an alternate branch
On Sun, Dec 08, 2013 at 03:48:54PM -0800, Justin Hibbits wrote: > On Sun, 8 Dec 2013 14:38:53 +0100 > Marius Strobl wrote: > > > On Wed, Dec 04, 2013 at 10:21:13PM -0800, Justin Hibbits wrote: > > > I've been working on the projects/pmac_pmu branch for some time now > > > to add suspend/resume as well as CPU speed change for certain > > > PowerPC machines, about a year since I created the branch, and now > > > it's stable enough that I want to merge it into HEAD, hence this > > > request. However, it does touch several drivers, turning them into > > > "early drivers", such that they can be initialized, and suspended > > > and resumed at a different time. Saying that, I do need testing > > > from other architectures, to make sure I haven't broken anything. > > > > > > The technical details: > > > > > > To get proper ordering, I've extended the bus_generic_suspend() and > > > bus_generic_resume() to do multiple passes. Devices which cannot be > > > enabled or disabled at the current pass level would return an > > > EAGAIN. This could possibly cause problems, since it's an addition > > > to an existing API rather than a new API to run along side it, so > > > it needs a great deal of testing. It works fine on PowerPC, but I > > > don't have any i386/amd64 or sparc64 hardware to test it on, so > > > would like others who do to test it. I don't think that it would > > > impact x86 at all (testing is obviously required), because the > > > nexus is not an EARLY_DRIVER_MODULE, so all devices would be > > > handled at the same pass. But, I do know the sparc64 has an > > > EARLY_DRIVER_MODULE() nexus, so that will likely be impacted. > > > > > > Also, any comments are of course welcome. Technical concerns are > > > obviously welcome, and I will try to address everything. > > > > Do you have a patch against head? > > > > Marius > > > > Here you go. > Thanks; on a sparc64 machine where the EARLY_DRIVER_MODULE nexus actually matters, your patch doesn't seem to have an ill effect. Marius ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: spec violation of xHCI?
From: Hans Petter Selasky Date: Wed, 11 Dec 2013 13:50:50 +0100 > On 12/11/13 13:44, Hans Petter Selasky wrote: >> On 12/11/13 12:12, Kohji Okuno wrote: On 12/11/13 11:12, Kohji Okuno wrote: > Hi, > > I think the xHCI host controller driver has a spec violation. > > Could you refer to > ``Table 126: Offset 0Ch – Link TRB Field Definitions'' > in xHCI_Specification_for_USB.pdf(Revision 1.0)? > > The following is an excerpt about the CHAIN BIT. > > Chain bit (CH). Set to ‘1’ by software to associate this TRB with > the next TRB on the Ring. A Transfer Descriptor (TD) is defined as > one or more TRBs. The Chain bit is used to identify the TRBs that > comprise a TD. Refer to section 4.11.7 for more information on Link > TRB placement within a TD. On a Command Ring this bit is ignored by > the xHC. > > > I think that we should add XHCI_TRB_3_CHAIN_BIT to line 1895. > How do you think? > Hi Kohji, The double word written at line 1895 does not set the "chain bit" because this is the end of a transfer descriptor, TD. I'm unsure how hardware interprets this bit, if setting the bit on the previous TRB makes the next one connect to the previous one, or the other way around. If setting this bit makes the TRB connect to the previous one, you are correct. Else the current code is correct. >>> >>> Hi, HPS, >>> >>> Thank you for your comment. >>> >>> I think that this (line 1895) is not the end of a transfer descriptor. >>> When the device driver needs a Zero Length Packet, this is not the >>> end. And, If xfer has nframes, this is not the end, too. >>> >>> Regards, >>> Kohji Okuno >>> >> >> Hi Kohji, >> >> Yes, you are right that if nframes is greater than one, and/or if a ZLP >> needs to be sent this is not the end of the USB transfers. Are we sure >> that if the XHCI_TRB_3_CHAIN_BIT is added at line 1895, that we will >> receive a completion TRB-event for each of the nframes, or will the >> chain bit result in loss of TRB completion events? >> >> Does setting this bit have any impact on performance? >> >> Thank you! >> >> --HPS > > Some more thoughts: > > The code in question handle all four USB transfer types. Are you saying that > the CHAIN bit should be set for isochronous transfers too, so all the packets > send in all the intervals are chained together? Or is this only for BULK > traffic you want to add the CHAIN bit? > > Thank you! > > --HPS Hi HPS, All link trbs which are not the end need CHAIN bit, I think. But, this is errata in xHCI ver 0.95. So, linux has quirk for chain bit. Could you check linux codes? Regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: spec violation of xHCI?
From: Hans Petter Selasky Date: Wed, 11 Dec 2013 13:44:37 +0100 > On 12/11/13 12:12, Kohji Okuno wrote: >>> On 12/11/13 11:12, Kohji Okuno wrote: Hi, I think the xHCI host controller driver has a spec violation. Could you refer to ``Table 126: Offset 0Ch – Link TRB Field Definitions'' in xHCI_Specification_for_USB.pdf(Revision 1.0)? The following is an excerpt about the CHAIN BIT. Chain bit (CH). Set to ‘1’ by software to associate this TRB with the next TRB on the Ring. A Transfer Descriptor (TD) is defined as one or more TRBs. The Chain bit is used to identify the TRBs that comprise a TD. Refer to section 4.11.7 for more information on Link TRB placement within a TD. On a Command Ring this bit is ignored by the xHC. I think that we should add XHCI_TRB_3_CHAIN_BIT to line 1895. How do you think? >>> >>> Hi Kohji, >>> >>> The double word written at line 1895 does not set the "chain bit" because >>> this >>> is the end of a transfer descriptor, TD. I'm unsure how hardware interprets >>> this bit, if setting the bit on the previous TRB makes the next one connect >>> to >>> the previous one, or the other way around. If setting this bit makes the >>> TRB >>> connect to the previous one, you are correct. Else the current code is >>> correct. >> >> Hi, HPS, >> >> Thank you for your comment. >> >> I think that this (line 1895) is not the end of a transfer descriptor. >> When the device driver needs a Zero Length Packet, this is not the >> end. And, If xfer has nframes, this is not the end, too. >> >> Regards, >> Kohji Okuno >> > > Hi Kohji, > > Yes, you are right that if nframes is greater than one, and/or if a ZLP needs > to be sent this is not the end of the USB transfers. Are we sure that if the > XHCI_TRB_3_CHAIN_BIT is added at line 1895, that we will receive a completion > TRB-event for each of the nframes, or will the chain bit result in loss of TRB > completion events? > > Does setting this bit have any impact on performance? > > Thank you! > > --HPS Hi HPS, I don't know about any impact on performance. But, in linux, link trb has CHAIN BIT if necessary, I think. Regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: spec violation of xHCI?
On 12/11/13 13:44, Hans Petter Selasky wrote: On 12/11/13 12:12, Kohji Okuno wrote: On 12/11/13 11:12, Kohji Okuno wrote: Hi, I think the xHCI host controller driver has a spec violation. Could you refer to ``Table 126: Offset 0Ch – Link TRB Field Definitions'' in xHCI_Specification_for_USB.pdf(Revision 1.0)? The following is an excerpt about the CHAIN BIT. Chain bit (CH). Set to ‘1’ by software to associate this TRB with the next TRB on the Ring. A Transfer Descriptor (TD) is defined as one or more TRBs. The Chain bit is used to identify the TRBs that comprise a TD. Refer to section 4.11.7 for more information on Link TRB placement within a TD. On a Command Ring this bit is ignored by the xHC. I think that we should add XHCI_TRB_3_CHAIN_BIT to line 1895. How do you think? Hi Kohji, The double word written at line 1895 does not set the "chain bit" because this is the end of a transfer descriptor, TD. I'm unsure how hardware interprets this bit, if setting the bit on the previous TRB makes the next one connect to the previous one, or the other way around. If setting this bit makes the TRB connect to the previous one, you are correct. Else the current code is correct. Hi, HPS, Thank you for your comment. I think that this (line 1895) is not the end of a transfer descriptor. When the device driver needs a Zero Length Packet, this is not the end. And, If xfer has nframes, this is not the end, too. Regards, Kohji Okuno Hi Kohji, Yes, you are right that if nframes is greater than one, and/or if a ZLP needs to be sent this is not the end of the USB transfers. Are we sure that if the XHCI_TRB_3_CHAIN_BIT is added at line 1895, that we will receive a completion TRB-event for each of the nframes, or will the chain bit result in loss of TRB completion events? Does setting this bit have any impact on performance? Thank you! --HPS Some more thoughts: The code in question handle all four USB transfer types. Are you saying that the CHAIN bit should be set for isochronous transfers too, so all the packets send in all the intervals are chained together? Or is this only for BULK traffic you want to add the CHAIN bit? Thank you! --HPS ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: spec violation of xHCI?
On 12/11/13 12:12, Kohji Okuno wrote: On 12/11/13 11:12, Kohji Okuno wrote: Hi, I think the xHCI host controller driver has a spec violation. Could you refer to ``Table 126: Offset 0Ch – Link TRB Field Definitions'' in xHCI_Specification_for_USB.pdf(Revision 1.0)? The following is an excerpt about the CHAIN BIT. Chain bit (CH). Set to ‘1’ by software to associate this TRB with the next TRB on the Ring. A Transfer Descriptor (TD) is defined as one or more TRBs. The Chain bit is used to identify the TRBs that comprise a TD. Refer to section 4.11.7 for more information on Link TRB placement within a TD. On a Command Ring this bit is ignored by the xHC. I think that we should add XHCI_TRB_3_CHAIN_BIT to line 1895. How do you think? Hi Kohji, The double word written at line 1895 does not set the "chain bit" because this is the end of a transfer descriptor, TD. I'm unsure how hardware interprets this bit, if setting the bit on the previous TRB makes the next one connect to the previous one, or the other way around. If setting this bit makes the TRB connect to the previous one, you are correct. Else the current code is correct. Hi, HPS, Thank you for your comment. I think that this (line 1895) is not the end of a transfer descriptor. When the device driver needs a Zero Length Packet, this is not the end. And, If xfer has nframes, this is not the end, too. Regards, Kohji Okuno Hi Kohji, Yes, you are right that if nframes is greater than one, and/or if a ZLP needs to be sent this is not the end of the USB transfers. Are we sure that if the XHCI_TRB_3_CHAIN_BIT is added at line 1895, that we will receive a completion TRB-event for each of the nframes, or will the chain bit result in loss of TRB completion events? Does setting this bit have any impact on performance? Thank you! --HPS ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: spec violation of xHCI?
> On 12/11/13 11:12, Kohji Okuno wrote:
>> Hi,
>>
>> I think the xHCI host controller driver has a spec violation.
>>
>> Could you refer to
>> ``Table 126: Offset 0Ch – Link TRB Field Definitions''
>> in xHCI_Specification_for_USB.pdf(Revision 1.0)?
>>
>> The following is an excerpt about the CHAIN BIT.
>>
>>Chain bit (CH). Set to ‘1’ by software to associate this TRB with
>>the next TRB on the Ring. A Transfer Descriptor (TD) is defined as
>>one or more TRBs. The Chain bit is used to identify the TRBs that
>>comprise a TD. Refer to section 4.11.7 for more information on Link
>>TRB placement within a TD. On a Command Ring this bit is ignored by
>>the xHC.
>>
>>
>> I think that we should add XHCI_TRB_3_CHAIN_BIT to line 1895.
>> How do you think?
>>
>
> Hi Kohji,
>
> The double word written at line 1895 does not set the "chain bit" because this
> is the end of a transfer descriptor, TD. I'm unsure how hardware interprets
> this bit, if setting the bit on the previous TRB makes the next one connect to
> the previous one, or the other way around. If setting this bit makes the TRB
> connect to the previous one, you are correct. Else the current code is
> correct.
Hi, HPS,
Thank you for your comment.
I think that this (line 1895) is not the end of a transfer descriptor.
When the device driver needs a Zero Length Packet, this is not the
end. And, If xfer has nframes, this is not the end, too.
Regards,
Kohji Okuno
>>
>> src/sys/dev/usb/controller/xhci.c:
>> 1879 /* fill out link TRB */
>> 1880
>> 1881 if (td_next != NULL) {
>> 1882 /* link the current TD with the next one */
>> 1883 td->td_trb[x].qwTrb0 = htole64((uint64_t)td_next->td_self);
>> 1884 DPRINTF("LINK=0x%08llx\n", (long long)td_next->td_self);
>> 1885 } else {
>> 1886 /* this field will get updated later */
>> 1887 DPRINTF("NOLINK\n");
>> 1888 }
>> 1889
>> 1890 dword = XHCI_TRB_2_IRQ_SET(0);
>> 1891
>> 1892 td->td_trb[x].dwTrb2 = htole32(dword);
>> 1893
>> 1894 dword = XHCI_TRB_3_TYPE_SET(XHCI_TRB_TYPE_LINK) |
>> 1895 XHCI_TRB_3_CYCLE_BIT | XHCI_TRB_3_IOC_BIT;
>> 1896
>> 1897 td->td_trb[x].dwTrb3 = htole32(dword);
>> 1898
>> 1899 td->alt_next = td_alt_next;
>>
>> --
>> Best regards,
>> Kohji Okuno
>>
>> ___
>> freebsd-...@freebsd.org mailing list
>> http://lists.freebsd.org/mailman/listinfo/freebsd-usb
>> To unsubscribe, send any mail to "freebsd-usb-unsubscr...@freebsd.org"
>>
>
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: spec violation of xHCI?
On 12/11/13 11:12, Kohji Okuno wrote:
Hi,
I think the xHCI host controller driver has a spec violation.
Could you refer to
``Table 126: Offset 0Ch – Link TRB Field Definitions''
in xHCI_Specification_for_USB.pdf(Revision 1.0)?
The following is an excerpt about the CHAIN BIT.
Chain bit (CH). Set to ‘1’ by software to associate this TRB with
the next TRB on the Ring. A Transfer Descriptor (TD) is defined as
one or more TRBs. The Chain bit is used to identify the TRBs that
comprise a TD. Refer to section 4.11.7 for more information on Link
TRB placement within a TD. On a Command Ring this bit is ignored by
the xHC.
I think that we should add XHCI_TRB_3_CHAIN_BIT to line 1895.
How do you think?
Hi Kohji,
The double word written at line 1895 does not set the "chain bit"
because this is the end of a transfer descriptor, TD. I'm unsure how
hardware interprets this bit, if setting the bit on the previous TRB
makes the next one connect to the previous one, or the other way around.
If setting this bit makes the TRB connect to the previous one, you are
correct. Else the current code is correct.
Thank you!
--HPS
src/sys/dev/usb/controller/xhci.c:
1879/* fill out link TRB */
1880
1881if (td_next != NULL) {
1882/* link the current TD with the next one */
1883td->td_trb[x].qwTrb0 =
htole64((uint64_t)td_next->td_self);
1884DPRINTF("LINK=0x%08llx\n", (long
long)td_next->td_self);
1885} else {
1886/* this field will get updated later */
1887DPRINTF("NOLINK\n");
1888}
1889
1890dword = XHCI_TRB_2_IRQ_SET(0);
1891
1892td->td_trb[x].dwTrb2 = htole32(dword);
1893
1894dword = XHCI_TRB_3_TYPE_SET(XHCI_TRB_TYPE_LINK) |
1895XHCI_TRB_3_CYCLE_BIT | XHCI_TRB_3_IOC_BIT;
1896
1897td->td_trb[x].dwTrb3 = htole32(dword);
1898
1899td->alt_next = td_alt_next;
--
Best regards,
Kohji Okuno
___
freebsd-...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to "freebsd-usb-unsubscr...@freebsd.org"
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
spec violation of xHCI?
Hi,
I think the xHCI host controller driver has a spec violation.
Could you refer to
``Table 126: Offset 0Ch – Link TRB Field Definitions''
in xHCI_Specification_for_USB.pdf(Revision 1.0)?
The following is an excerpt about the CHAIN BIT.
Chain bit (CH). Set to ‘1’ by software to associate this TRB with
the next TRB on the Ring. A Transfer Descriptor (TD) is defined as
one or more TRBs. The Chain bit is used to identify the TRBs that
comprise a TD. Refer to section 4.11.7 for more information on Link
TRB placement within a TD. On a Command Ring this bit is ignored by
the xHC.
I think that we should add XHCI_TRB_3_CHAIN_BIT to line 1895.
How do you think?
src/sys/dev/usb/controller/xhci.c:
1879/* fill out link TRB */
1880
1881if (td_next != NULL) {
1882/* link the current TD with the next one */
1883td->td_trb[x].qwTrb0 =
htole64((uint64_t)td_next->td_self);
1884DPRINTF("LINK=0x%08llx\n", (long
long)td_next->td_self);
1885} else {
1886/* this field will get updated later */
1887DPRINTF("NOLINK\n");
1888}
1889
1890dword = XHCI_TRB_2_IRQ_SET(0);
1891
1892td->td_trb[x].dwTrb2 = htole32(dword);
1893
1894dword = XHCI_TRB_3_TYPE_SET(XHCI_TRB_TYPE_LINK) |
1895XHCI_TRB_3_CYCLE_BIT | XHCI_TRB_3_IOC_BIT;
1896
1897td->td_trb[x].dwTrb3 = htole32(dword);
1898
1899td->alt_next = td_alt_next;
--
Best regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
