Re: [PATCH] usb: dwc2: host: fix isoc urb actual length
Hi Minas, 在 2017年11月06日 17:28, Minas Harutyunyan 写道: Hi, On 11/6/2017 12:46 PM, William Wu wrote: The actual_length in dwc2_hcd_urb structure is used to indicate the total data length transferred so far, but in dwc2_update_isoc_urb_state(), it just updates the actual_length of isoc frame, and don't update the urb actual_length at the same time, this will cause device drivers working error which depend on the urb actual_length. we can easily find this issue if use an USB camera， the userspace use libusb to get USB data from kernel via devio driver.In usb devio driver, processcompl() function will process urb complete and copy data to userspace depending on urb actual_length. Let's update the urb actual_length if the isoc frame is valid. Signed-off-by: William Wu --- drivers/usb/dwc2/hcd_intr.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c index 28a8210..01b1e13 100644 --- a/drivers/usb/dwc2/hcd_intr.c +++ b/drivers/usb/dwc2/hcd_intr.c @@ -580,6 +580,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state( frame_desc->status = 0; frame_desc->actual_length = dwc2_get_actual_xfer_length(hsotg, chan, chnum, qtd, halt_status, NULL); + urb->actual_length += frame_desc->actual_length; break; case DWC2_HC_XFER_FRAME_OVERRUN: urb->error_count++; @@ -599,6 +600,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state( frame_desc->status = -EPROTO; frame_desc->actual_length = dwc2_get_actual_xfer_length(hsotg, chan, chnum, qtd, halt_status, NULL); + urb->actual_length += frame_desc->actual_length; /* Skip whole frame */ if (chan->qh->do_split && According urb description in usb.h urb->actual_length used for non-iso transfers: "@actual_length: This is read in non-iso completion functions, and ... * ISO transfer status is reported in the status and actual_length fields * of the iso_frame_desc array, " Yes, urb->actual_length is used for non-iso transfers. And for ISO transfer, the most usb class drivers can only use iso frame status and actual_length to handle usb data, like: kernel v4.4 drivers/media/usb/uvc/uvc_video.c But the usb devio driver really need the urb actual_length in the processcompl() if handle ISO transfer, just as I mentioned in the commit message. And I also found the same issue on raspberrypi board: https://github.com/raspberrypi/linux/issues/903 So do you think we need to fix the usb devio driver, rather than fix dwc2? I think maybe we can use urb actual length for ISO transfer, it seems that don't cause any side-effect. Thanks, Minas -- 吴良峰 William.Wu 福建省福州市铜盘路软件大道89号软件园A区21号楼 No.21 Building, A District, No.89,software Boulevard Fuzhou,Fujian, PRC 手机: 13685012275 座机: 0591-83991906-8520 邮件:w...@rock-chips.com
Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
Dear Doug,
在 2018年05月02日 12:33, Doug Anderson 写道:
Hi,
On Tue, May 1, 2018 at 8:04 PM, William Wu wrote:
The commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in
a more supported way") rips out a lot of code to simply the
allocation of aligned DMA. However, it also introduces a new
issue when use isoc split in transfer.
In my test case, I connect the dwc2 controller with an usb hs
Hub (GL852G-12), and plug an usb fs audio device (Plantronics
headset) into the downstream port of Hub. Then use the usb mic
to record, we can find noise when playback.
It's because that the usb Hub uses an MDATA for the first
transaction and a DATA0 for the second transaction for the isoc
split in transaction. An typical isoc split in transaction sequence
like this:
- SSPLIT IN transaction
- CSPLIT IN transaction
- MDATA packet
- CSPLIT IN transaction
- DATA0 packet
The DMA address of MDATA (urb->dma) is always DWORD-aligned, but
the DMA address of DATA0 (urb->dma + qtd->isoc_split_offset) may
not be DWORD-aligned, it depends on the qtd->isoc_split_offset (the
length of MDATA). In my test case, the length of MDATA is usually
unaligned, this casue DATA0 packet transmission error.
This patch base on the old way of aligned DMA allocation in the
dwc2 driver to get aligned DMA for isoc split in.
Signed-off-by: William Wu
---
Changes in v2:
- None
drivers/usb/dwc2/hcd.c | 63 +---
drivers/usb/dwc2/hcd.h | 10 +++
drivers/usb/dwc2/hcd_intr.c | 8 ++
drivers/usb/dwc2/hcd_queue.c | 8 +-
4 files changed, 85 insertions(+), 4 deletions(-)
A word of warning that I'm pretty rusty on dwc2 and even when I was
making lots of patches I still considered myself a bit clueless.
...so if something seems wrong, please call me on it...
Most of your suggestions are great, thanks very much！
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 190f959..8c2b35f 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1562,11 +1562,20 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg
*hsotg,
}
if (hsotg->params.host_dma) {
- dwc2_writel((u32)chan->xfer_dma,
- hsotg->regs + HCDMA(chan->hc_num));
+ dma_addr_t dma_addr;
+
+ if (chan->align_buf) {
+ if (dbg_hc(chan))
+ dev_vdbg(hsotg->dev, "align_buf\n");
+ dma_addr = chan->align_buf;
+ } else {
+ dma_addr = chan->xfer_dma;
+ }
+ dwc2_writel((u32)dma_addr, hsotg->regs + HCDMA(chan->hc_num));
+
if (dbg_hc(chan))
dev_vdbg(hsotg->dev, "Wrote %08lx to HCDMA(%d)\n",
-(unsigned long)chan->xfer_dma, chan->hc_num);
+(unsigned long)dma_addr, chan->hc_num);
}
/* Start the split */
@@ -2620,6 +2629,33 @@ static void dwc2_hc_init_xfer(struct dwc2_hsotg *hsotg,
}
}
+static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
+ struct dwc2_qh *qh,
+ struct dwc2_host_chan *chan)
+{
+ if (!qh->dw_align_buf) {
+ qh->dw_align_buf = kmalloc(chan->max_packet,
So you're potentially doing a bounce buffer atop a bounce buffer now,
right? That seems pretty non-optimal. You're also back to doing a
kmalloc at interrupt time which I found was pretty bad for
performance.
Yes, I just allocate an additional bounce buffer here. I haven't thought
consummately
about this patch, it's really not a good way to use a kmalloc at
interrut time.
Is there really no way you can do your memory allocation in
dwc2_alloc_dma_aligned_buffer() instead of here? For input packets,
if you could just allocate an extra 3 bytes in the original bounce
buffer you could just re-use the original bounce buffer together with
a memmove? AKA:
transfersize = 13 + 64;
buf = alloc(16 + 64);
// Do the first transfer, no problems.
dma_into(buf, 13);
// 2nd transfer isn't aligned, so align.
// we allocated a little extra to account for this
dma_into(buf + 16, 64);
// move back where it belongs.
memmove(buf + 13, buf + 16, 64);
To make the above work you'd need to still allocate a bounce buffer
even if the original "urb->transfer_buffer" was already aligned.
Ideally you'd be able to know when dwc2_alloc_dma_aligned_buffer()
that this is one of the special cases where you need a slightly
oversized bounce buffer.
It's a good way to allocate an extra 3 bytes in the original bounce
buffer for this unaligned
issue， it's similar to the tailroom of sk_buff. However, just as you
said, we'd better find
the special cases where we need an oversized bounce buffer, otherwise，we
need to allocate
a bounce buffer for all of urbs.
It's hard for me to know the special cases in
Re: [PATCH v2 2/2] usb: dwc2: fix isoc split in transfer with no data
Dear Doug,
在 2018年05月02日 13:02, Doug Anderson 写道:
Hi,
On Tue, May 1, 2018 at 8:04 PM, William Wu wrote:
If isoc split in transfer with no data (the length of DATA0
packet is zero), we can't simply return immediately. Because
the DATA0 can be the first transaction or the second transaction
for the isoc split in transaction. If the DATA0 packet with no
data is in the first transaction, we can return immediately.
But if the DATA0 packet with no data is in the second transaction
of isoc split in transaction sequence, we need to increase the
qtd->isoc_frame_index and giveback urb to device driver if needed,
otherwise, the MDATA packet will be lost.
A typical test case is that connect the dwc2 controller with an
usb hs Hub (GL852G-12), and plug an usb fs audio device (Plantronics
headset) into the downstream port of Hub. Then use the usb mic
to record, we can find noise when playback.
In the case, the isoc split in transaction sequence like this:
- SSPLIT IN transaction
- CSPLIT IN transaction
- MDATA packet (176 bytes)
- CSPLIT IN transaction
- DATA0 packet (0 byte)
This patch use both the length of DATA0 and qtd->isoc_split_offset
to check if the DATA0 is in the second transaction.
Signed-off-by: William Wu
---
Changes in v2:
- Modify the commit message
drivers/usb/dwc2/hcd_intr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 5e2378f..479f628 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -930,7 +930,7 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg
*hsotg,
frame_desc = &qtd->urb->iso_descs[qtd->isoc_frame_index];
len = dwc2_get_actual_xfer_length(hsotg, chan, chnum, qtd,
DWC2_HC_XFER_COMPLETE, NULL);
- if (!len) {
+ if (!len && !qtd->isoc_split_offset) {
qtd->complete_split = 0;
qtd->isoc_split_offset = 0;
return 0;
I don't think my USB-fu is strong enough to do a real review of this
patch, but one small nitpick is that you can remove
"qtd->isoc_split_offset = 0" in the if test now. AKA:
if (!len && !qtd->isoc_split_offset) {
qtd->complete_split = 0;
return 0;
}
Yes, good idea! I will fix it in next patch. Thank you!
...since you only enter the "if" test now when isoc_split_offset is
already 0... Hopefully John Youn will have some time to comment on
this patch with more real USB knowledge...
-Doug
Best regards,
wulf
Re: [PATCH 2/2] usb: dwc2: fix isoc split in transfer with no data
Dear Sergei, 在 2018年04月24日 16:27, Sergei Shtylyov 写道: Hello! On 4/24/2018 5:43 AM, William Wu wrote: If isoc split in transfer with no data (the length of DATA0 packet is 0), we can't simply return immediately. Because the DATA0 can be the first transaction or the second transaction for the isoc split in transaction. If the DATA0 packet with on data ^^ no? Thank you for correcting me. I will fix it in next patch version. is in the first transaction, we can return immediately. But if the the DATA0 packet with on data is in the second transaction One "the" too many. And that "on data" again... :-) Ah, sorry to make such a simple mistake. I will fix these in next patch version. of isoc split in transaction sequence, we need to increase the qtd->isoc_frame_index and giveback urb to device driver if needed, otherwise, the MDATA packet will be lost. A typical test case is that connect the dwc2 controller with an usb hs Hub (GL852G-12), and plug an usb fs audio device (Plantronics headset) into the downstream port of Hub. Then use the usb mic to record, we can find noise when playback. In the case, the isoc split in transaction sequence like this: - SSPLIT IN transaction - CSPLIT IN transaction - MDATA packet (176 bytes) - CSPLIT IN transaction - DATA0 packet (0 byte) This patch use both the length of DATA0 and qtd->isoc_split_offset to check if the DATA0 is in the second transaction. Signed-off-by: William Wu [...] MBR, Sergei Best regards, wulf
Re: [PATCH v3 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
Dear Doug,
在 2018年05月08日 23:29, Doug Anderson 写道:
Hi,
On Tue, May 8, 2018 at 12:43 AM, wlf wrote:
Dear Doug,
在 2018年05月08日 13:11, Doug Anderson 写道:
Hi,
On Mon, May 7, 2018 at 8:07 PM, William Wu
wrote:
+static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
+ struct dwc2_qh *qh,
+ struct dwc2_host_chan *chan)
+{
+ if (!hsotg->unaligned_cache)
+ return -ENOMEM;
+
+ if (!qh->dw_align_buf) {
+ qh->dw_align_buf =
kmem_cache_alloc(hsotg->unaligned_cache,
+ GFP_ATOMIC |
GFP_DMA);
+ if (!qh->dw_align_buf)
+ return -ENOMEM;
+
+ qh->dw_align_buf_size = min_t(u32, chan->max_packet,
+
DWC2_KMEM_UNALIGNED_BUF_SIZE);
Rather than using min_t, wouldn't it be better to return -ENOMEM if
"max_packet" > DWC2_KMEM_UNALIGNED_BUF_SIZE? As it is, you might
allocate less space than you need, right? That seems like it would be
bad (even though this is probably impossible).
Yes, good idea! So is it good to fix it like this?
if (!qh->dw_align_buf || chan->max_packet >
DWC2_KMEM_UNALIGNED_BUF_SIZE)
return -ENOMEM;
qh->dw_align_buf_size = chan->max_packet;
Won't that orphan memory in the case that the allocation is OK but the
size is wrong?
I would have put it all the way at the top:
if (!hsotg->unaligned_cache ||
chan->max_packet > DWC2_KMEM_UNALIGNED_BUF_SIZE)
return -ENOMEM;
Ah, yes, you're right! Thank you for correcting me.
It also feels like you should get rid of "qh->dw_align_buf_size". The
buffer is always DWC2_KMEM_UNALIGNED_BUF_SIZE.
...if you need to keep track of how many bytes you mapped with
dma_map_single() and somehow can't get back to "chan", rename this to
qh->dw_align_buf_mapped_bytes or something. I haven't followed
through all the code, but I _think_ you'd want to make sure to set
qh->dw_align_buf_mapped_bytes every time through
dwc2_alloc_split_dma_aligned_buf(), even if dw_align_buf was already
allocated. Specifically, my worry is in the case where you enter
dwc2_alloc_split_dma_aligned_buf() and qh->dw_align_buf is non-NULL.
Could "max_packet" be different in this case compared to what it was
when dw_align_buf was first allocated?
Sorry to make you feel confused. It's really not suitable to use
"qh->dw_align_buf_size" for the dma mapped size. And I think the
"max_packet" should be always be the same. However, just in case, maybe
I can get rid of "qh->dw_align_buf_size" and just use
DWC2_KMEM_UNALIGNED_BUF_SIZE to do dma_map_single().
@@ -2797,6 +2837,32 @@ static int dwc2_assign_and_init_hc(struct
dwc2_hsotg *hsotg, struct dwc2_qh *qh)
/* Set the transfer attributes */
dwc2_hc_init_xfer(hsotg, chan, qtd);
+ /* For non-dword aligned buffers */
+ if (hsotg->params.host_dma && qh->do_split &&
+ chan->ep_is_in && (chan->xfer_dma & 0x3)) {
+ dev_vdbg(hsotg->dev, "Non-aligned buffer\n");
+ if (dwc2_alloc_split_dma_aligned_buf(hsotg, qh, chan)) {
+ dev_err(hsotg->dev,
+ "Failed to allocate memory to handle
non-aligned buffer\n");
+ /* Add channel back to free list */
+ chan->align_buf = 0;
+ chan->multi_count = 0;
+ list_add_tail(&chan->hc_list_entry,
+ &hsotg->free_hc_list);
+ qtd->in_process = 0;
+ qh->channel = NULL;
+ return -ENOMEM;
+ }
+ } else {
+ /*
+* We assume that DMA is always aligned in non-split
+* case or split out case. Warn if not.
+*/
+ WARN_ON_ONCE(hsotg->params.host_dma &&
+(chan->xfer_dma & 0x3));
+ chan->align_buf = 0;
+ }
+
if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
chan->ep_type == USB_ENDPOINT_XFER_ISOC)
/*
@@ -5241,6 +5307,17 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
hsotg->params.dma_desc_enable = false;
hsotg->params.dma_desc_fs_enable = false;
}
+ } else if (hsotg->params.host_dma) {
Are you sure this is "else if"? Can't you have descriptor DMA enabled
in the controller and still need to do a normal DMA transfer if you
plug in a hub? Seems like this should be just "if".
Sorry, I don't understand
Re: [PATCH v3 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
Dear Doug,
在 2018年05月11日 04:59, Doug Anderson 写道:
Hi,
On Wed, May 9, 2018 at 1:55 AM, wlf wrote:
+ } else if (hsotg->params.host_dma) {
Are you sure this is "else if"? Can't you have descriptor DMA enabled
in the controller and still need to do a normal DMA transfer if you
plug in a hub? Seems like this should be just "if".
Sorry, I don't understand the case "have descriptor DMA enabled in the
controller and still need to do a normal DMA transfer". But maybe it
still
has another problem if just use "if" here, because it will create kmem
caches for Slave mode which actually doesn't need aligned DMA buf.
So right now your code looks like:
if (hsotg->params.dma_desc_enable ||
hsotg->params.dma_desc_fs_enable) {
...
...
} else if (hsotg->params.host_dma) {
...
}
I've never played much with "descriptor DMA" on dwc2 because every
time I enabled it (last I tried was years ago) a whole bunch of
peripherals stopped working and I didn't dig (I just left it off).
Thus, I'm no expert on descriptor DMA. ...that being said, my
understanding is that if you enable descriptor DMA in the controller
then it will enable a smarter DMA mode for some of the transfers.
IIRC Descriptor DMA mode specifically can't handle splits. Is my
memory correct there? Presumably that means that when you enable
descriptor DMA in the controller the driver will automatically fall
back to normal Address DMA mode if things get too complicated. When
it falls back to Address DMA mode, presumably dwc2_hcd_init() won't
get called again. That means that any data structures that are needed
for Address DMA need to be allocated in dwc2_hcd_init() even if
descriptor DMA is enabled because we might need to fall back to
Address DMA.
Thus, I'm suggesting changing the code to:
if (hsotg->params.dma_desc_enable ||
hsotg->params.dma_desc_fs_enable) {
...
...
}
if (hsotg->params.host_dma) {
...
}
...as I said, I'm no expert and I could just be confused. If
something I say seems wrong please correct me.
Ah, I got it. Thanks for your detailed explanation. Although I don't know if
it's possible that dwc2 driver automatically fall back to normal Address DMA
mode when desc DMA work abnormally with split, I agree with your suggestion.
I'll fix it in V4 patch.
Hmm, I guess you're right. I did a quick search and I can't find any
evidence of a fallback like this. Maybe I dreamed that. I found some
old comment in the commit history that said:
I think you're right. I find that it's possible to fall back to Address
DMA mode if desc DMA initialization failure. The error case is：in
dwc2_hcd_init(), if it fails to create dwc2 generic desc cache or dwc2
hs isoc desc cache, then it will disable desc DMA and fall back to
Address DMA mode.
/*
* Disable descriptor dma mode by default as the HW can support
* it, but does not support it for SPLIT transactions.
* Disable it for FS devices as well.
*/
...and it looks there's currently nobody using descriptor DMA anyway
(assuming I read the code correctly). It does seem like people were
ever going to turn it on then it would have to be dynamic as I was
dreaming it was...
Yes, the dwc2 desc DMA mode can't support split transaction. So few
people use desc DMA mode for OTG Host mode. BTW, I find that the latest
code enable desc DMA mode by default for the OTG peripheral mode if the
dwc2 hardware support desc DMA.
Re: [PATCH v4 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
Dear Doug,
在 2018年05月11日 05:01, Doug Anderson 写道:
Hi,
On Wed, May 9, 2018 at 3:11 AM, William Wu wrote:
The commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in
a more supported way") rips out a lot of code to simply the
allocation of aligned DMA. However, it also introduces a new
issue when use isoc split in transfer.
In my test case, I connect the dwc2 controller with an usb hs
Hub (GL852G-12), and plug an usb fs audio device (Plantronics
headset) into the downstream port of Hub. Then use the usb mic
to record, we can find noise when playback.
It's because that the usb Hub uses an MDATA for the first
transaction and a DATA0 for the second transaction for the isoc
split in transaction. An typical isoc split in transaction sequence
like this:
- SSPLIT IN transaction
- CSPLIT IN transaction
- MDATA packet
- CSPLIT IN transaction
- DATA0 packet
The DMA address of MDATA (urb->dma) is always DWORD-aligned, but
the DMA address of DATA0 (urb->dma + qtd->isoc_split_offset) may
not be DWORD-aligned, it depends on the qtd->isoc_split_offset (the
length of MDATA). In my test case, the length of MDATA is usually
unaligned, this cause DATA0 packet transmission error.
This patch use kmem_cache to allocate aligned DMA buf for isoc
split in transaction. Note that according to usb 2.0 spec, the
maximum data payload size is 1023 bytes for each fs isoc ep,
and the maximum allowable interrupt data payload size is 64 bytes
or less for fs interrupt ep. So we set the size of object to be
1024 bytes in the kmem cache.
Signed-off-by: William Wu
---
Changes in v4:
- get rid of "qh->dw_align_buf_size"
- allocate unaligned_cache for Address DMA mode and Desc DMA mode
- freeing order opposite of allocation
You only did half of this. You changed the order under "error4" but
forgot to make dwc2_hcd_remove() match.
Ah, sorry, I'm careless.
- do dma_map_single() with the size of DWC2_KMEM_UNALIGNED_BUF_SIZE
Changes in v3:
- Modify the commit message
- Use Kmem_cache to allocate aligned DMA buf
- Fix coding style
Changes in v2:
- None
drivers/usb/dwc2/core.h | 3 ++
drivers/usb/dwc2/hcd.c | 87 ++--
drivers/usb/dwc2/hcd.h | 8
drivers/usb/dwc2/hcd_intr.c | 8
drivers/usb/dwc2/hcd_queue.c | 3 ++
5 files changed, 105 insertions(+), 4 deletions(-)
My only remaining comment is a really nitty detail. Unless someone
else feels strongly about it, I'm not sure it's worth spinning the
patch just for that nit unless someone else has review comments.
I believe I've looked at this enough to say:
Reviewed-by: Douglas Anderson
Thank you very much for your review. I will submit V5 patches to fix the
order of memory free in dwc2_hcd_remove()， and also add Review-by.
Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
Dear Doug,
在 2018年05月07日 15:19, Allen Hsu (許嘉銘) 写道:
Add more:
Best regards,
BU4 EE
Allen Hsu
QCI 886-3-327-2345 ext 15410
-Original Message-
From: Doug Anderson [mailto:diand...@google.com]
Sent: Friday, May 4, 2018 11:58 PM
To: wlf
Cc: William Wu ; hmi...@synopsys.com; felipe.ba...@linux.intel.com; Greg Kroah-Hartman ;
Sergei Shtylyov ; Heiko Stübner ; LKML ;
linux-...@vger.kernel.org; open list:ARM/Rockchip SoC... ; Frank Wang ; 黄涛
; daniel.meng ; John Youn ; 王征增 ;
z...@rock-chips.com; Allen Hsu (許嘉銘) ; Stan Tsui
Subject: Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split
in
Hi,
On Wed, May 2, 2018 at 10:14 AM, wlf wrote:
It's a good way to allocate an extra 3 bytes in the original bounce
buffer for this unaligned
issue， it's similar to the tailroom of sk_buff. However, just as you
said, we'd better find the special cases where we need an oversized
bounce buffer, otherwise，we
need to allocate
a bounce buffer for all of urbs.
It's hard for me to know the special cases in the
dwc2_alloc_dma_aligned_buffer(), because it's called from
usb_submit_urb() in the device class driver, and I hardly know the
split state in this process, much less if the split transaction need
aligned buffer. Do you have any idea?
I suppose that we can't find the special cases where we need an
oversized bounce buffer in the dwc2_alloc_dma_aligned_buffer(), and if
we still want to re-use the original bounce buffer with extra 3 bytes,
then we need to allocate a bounce buffer for all of urbs, and do
unnecessary data copy for these urbs whose transfer_buffer were
already aligned. This may reduce the transmission rate of USB.
Can we just pre-allocate an additional aligned buffer (the size is 200
bytes) for split transaction
in dwc2_map_urb_for_dma for all of urbs. And if we find the split
transaction is unaligned，
we can easily use the pre-allocated aligned buffer.
OK, so thinking about this more...
Previously things got really slow at interrupt time because we were trying to
allocate as much as 64K at interrupt time. That wasn't so great. In your
case, you're only allocating 200 bytes. As I understand things, allocating 200
bytes at interrupt time is probably not a huge deal.
...so I guess it come down to a tradeoff here: is it worth eating 200 bytes for
each URB to save an 200 byte allocation at interrupt time in this one rare case.
I'd certainly welcome anyone's opinion here, but I'm going to go with saying
it's fine to allocate the 200 bytes at interrupt time (like your patch does).
...but, I _think_ you want to use
kmem_cache_create() to create a cache and then kmem_cache_zalloc().
Since all allocations are the same size and you want this to be fast, I think
using kmem_cache is good.
Yes, it seems good to use kmem_cache. I'm trying to test a new patch
with kmem_cache, and I will upstream v3 patch as soon as possible.
+ /* For non-dword aligned buffers */
+ if (hsotg->params.host_dma > 0 && qh->do_split &&
+ chan->ep_is_in && (chan->xfer_dma & 0x3)) {
So what happens if were unaligned (AKA (chan->xfer_dma & 0x3)) but
we're not doing split or it's not an IN EP? Do we just fail then?
I guess the rest of this patch only handles the "in" case and maybe
you expect that the problems will only come about for do_split, but
it still might be wise to at least print a warning in the other cases?
>From reading dwc2_hc_init_xfer() it seems like you could run into
this
same problem in the "out" case?
Actually, I only find non-dword aligned issue in the case of split in
transaction.
And I think that if we're not doing split or it's an OUT EP, we can
always get aligned buffer in the current code. For non-split case, the
dwc2_alloc_dma_aligned_buffer() is enough. And for split out case, if
the transaction is subdivided into multiple start-splits, each with a
data payload of 188 bytes or less, so the DMA address is always
aligned.
Can you at least print an error message if you end up with non-aligned DMA in
one of the other cases?
That's an excellent suggestion. I will add warning message if end up
with unexpected non-aligned DMA.
DMA_FROM_DEVICE);
+ memcpy(qtd->urb->buf + frame_desc->offset +
+ qtd->isoc_split_offset,
+ chan->qh->dw_align_buf,
len);
Assuming I'm understanding this patch correctly, I think it would be
better to write:
memcpy(qtd->xfer_dma, chan->qh->dw_align_buf, len);
Sorry, there's no "xfer_buf" in qtd, do you means the
"chan->xfer_dma"? If it's, I think we can't do memcpy from a transfer
buffer to a DMA address. Maybe chan->xfer_buf is more suitable, but it
seems that the dwc2 driver doesn't update the chan->xfer_buf for isoc
transfer with dma enabl
Re: [PATCH v3 2/2] usb: dwc2: fix isoc split in transfer with no data
Dear Doug,
在 2018年05月08日 13:13, Doug Anderson 写道:
Hi,
On Mon, May 7, 2018 at 8:07 PM, William Wu wrote:
If isoc split in transfer with no data (the length of DATA0
packet is zero), we can't simply return immediately. Because
the DATA0 can be the first transaction or the second transaction
for the isoc split in transaction. If the DATA0 packet with no
data is in the first transaction, we can return immediately.
But if the DATA0 packet with no data is in the second transaction
of isoc split in transaction sequence, we need to increase the
qtd->isoc_frame_index and giveback urb to device driver if needed,
otherwise, the MDATA packet will be lost.
A typical test case is that connect the dwc2 controller with an
usb hs Hub (GL852G-12), and plug an usb fs audio device (Plantronics
headset) into the downstream port of Hub. Then use the usb mic
to record, we can find noise when playback.
In the case, the isoc split in transaction sequence like this:
- SSPLIT IN transaction
- CSPLIT IN transaction
- MDATA packet (176 bytes)
- CSPLIT IN transaction
- DATA0 packet (0 byte)
This patch use both the length of DATA0 and qtd->isoc_split_offset
to check if the DATA0 is in the second transaction.
Signed-off-by: William Wu
---
Changes in v3:
- Remove "qtd->isoc_split_offset = 0" in the if test
Changes in v2:
- Modify the commit message
drivers/usb/dwc2/hcd_intr.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index ba6fd852..3003594 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -930,9 +930,8 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg
*hsotg,
frame_desc = &qtd->urb->iso_descs[qtd->isoc_frame_index];
len = dwc2_get_actual_xfer_length(hsotg, chan, chnum, qtd,
DWC2_HC_XFER_COMPLETE, NULL);
- if (!len) {
+ if (!len && !qtd->isoc_split_offset) {
qtd->complete_split = 0;
- qtd->isoc_split_offset = 0;
return 0;
}
This looks fine to me now, but as per my comments on the previous
version I don't think I've dug through this problem enough to add my
Reviewed-by tag. I'll assume that John or someone with more knowledge
of the USB protocol than I have will Review / Ack.
Thanks very much for your review. Let's wait for other experts' suggestion.
-Doug
Re: [PATCH v3 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
Dear Doug,
在 2018年05月08日 13:11, Doug Anderson 写道:
Hi,
On Mon, May 7, 2018 at 8:07 PM, William Wu wrote:
+static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
+ struct dwc2_qh *qh,
+ struct dwc2_host_chan *chan)
+{
+ if (!hsotg->unaligned_cache)
+ return -ENOMEM;
+
+ if (!qh->dw_align_buf) {
+ qh->dw_align_buf = kmem_cache_alloc(hsotg->unaligned_cache,
+ GFP_ATOMIC | GFP_DMA);
+ if (!qh->dw_align_buf)
+ return -ENOMEM;
+
+ qh->dw_align_buf_size = min_t(u32, chan->max_packet,
+ DWC2_KMEM_UNALIGNED_BUF_SIZE);
Rather than using min_t, wouldn't it be better to return -ENOMEM if
"max_packet" > DWC2_KMEM_UNALIGNED_BUF_SIZE? As it is, you might
allocate less space than you need, right? That seems like it would be
bad (even though this is probably impossible).
Yes, good idea! So is it good to fix it like this?
if (!qh->dw_align_buf || chan->max_packet >
DWC2_KMEM_UNALIGNED_BUF_SIZE)
return -ENOMEM;
qh->dw_align_buf_size = chan->max_packet;
@@ -2797,6 +2837,32 @@ static int dwc2_assign_and_init_hc(struct dwc2_hsotg
*hsotg, struct dwc2_qh *qh)
/* Set the transfer attributes */
dwc2_hc_init_xfer(hsotg, chan, qtd);
+ /* For non-dword aligned buffers */
+ if (hsotg->params.host_dma && qh->do_split &&
+ chan->ep_is_in && (chan->xfer_dma & 0x3)) {
+ dev_vdbg(hsotg->dev, "Non-aligned buffer\n");
+ if (dwc2_alloc_split_dma_aligned_buf(hsotg, qh, chan)) {
+ dev_err(hsotg->dev,
+ "Failed to allocate memory to handle non-aligned
buffer\n");
+ /* Add channel back to free list */
+ chan->align_buf = 0;
+ chan->multi_count = 0;
+ list_add_tail(&chan->hc_list_entry,
+ &hsotg->free_hc_list);
+ qtd->in_process = 0;
+ qh->channel = NULL;
+ return -ENOMEM;
+ }
+ } else {
+ /*
+* We assume that DMA is always aligned in non-split
+* case or split out case. Warn if not.
+*/
+ WARN_ON_ONCE(hsotg->params.host_dma &&
+(chan->xfer_dma & 0x3));
+ chan->align_buf = 0;
+ }
+
if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
chan->ep_type == USB_ENDPOINT_XFER_ISOC)
/*
@@ -5241,6 +5307,17 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
hsotg->params.dma_desc_enable = false;
hsotg->params.dma_desc_fs_enable = false;
}
+ } else if (hsotg->params.host_dma) {
Are you sure this is "else if"? Can't you have descriptor DMA enabled
in the controller and still need to do a normal DMA transfer if you
plug in a hub? Seems like this should be just "if".
Sorry, I don't understand the case "have descriptor DMA enabled in the
controller and still need to do a normal DMA transfer". But maybe it
still has another problem if just use "if" here, because it will create
kmem caches for Slave mode which actually doesn't need aligned DMA buf.
+ /*
+* Create kmem caches to handle non-aligned buffer
+* in Buffer DMA mode.
+*/
+ hsotg->unaligned_cache = kmem_cache_create("dwc2-unaligned-dma",
+ DWC2_KMEM_UNALIGNED_BUF_SIZE, 4,
Worth using "DWC2_USB_DMA_ALIGN" rather than 4?
+ SLAB_CACHE_DMA, NULL);
+ if (!hsotg->unaligned_cache)
+ dev_err(hsotg->dev,
+ "unable to create dwc2 unaligned cache\n");
}
hsotg->otg_port = 1;
@@ -5279,6 +5356,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
error4:
kmem_cache_destroy(hsotg->desc_gen_cache);
kmem_cache_destroy(hsotg->desc_hsisoc_cache);
+ kmem_cache_destroy(hsotg->unaligned_cache);
nitty nit: freeing order should be opposite of allocation, so the new
line should be above the other two.
Ah, I got it. But note that it's impossible to allocate the
"unaligned_cache" and "desc *cache" at the same time. Should we still
fix the free order? If yes, maybe the correct free order is:
kmem_cache_destroy(hsotg->unaligned_cache);
kmem_cache_destroy(hsotg->desc_hsisoc_cache);
kmem_cache_destroy(hsotg->desc_gen_cache);
Right?
And should we also need to fix the same free order in the "dwc2_hcd_remove"?
Best regards,
wulf
Re: [PATCH] usb: hcd: initialize hcd->flags to 0 when rm hcd
Hi Roger， 在 2017年01月12日 23:38, Roger Quadros 写道: On 12/01/17 17:33, Alan Stern wrote: On Thu, 12 Jan 2017, Roger Quadros wrote: William, On 12/01/17 14:03, William Wu wrote: From: William wu On some platforms(e.g. rk3399 board), we can call hcd_add/remove consecutively without calling usb_put_hcd/usb_create_hcd in between, so hcd->flags can be stale. If the HC dies due to whatever reason then without this patch we get the below error on next hcd_add. [173.296154] xhci-hcd xhci-hcd.2.auto: HC died; cleaning up [173.296209] xhci-hcd xhci-hcd.2.auto: xHCI Host Controller [173.296762] xhci-hcd xhci-hcd.2.auto: new USB bus registered, assigned bus number 6 [173.296931] usb usb6: We don't know the algorithms for LPM for this host, disabling LPM. [173.297179] usb usb6: New USB device found, idVendor=1d6b, idProduct=0003 [173.297203] usb usb6: New USB device strings: Mfr=3, Product=2, SerialNumber=1 [173.297222] usb usb6: Product: xHCI Host Controller [173.297240] usb usb6: Manufacturer: Linux 4.4.21 xhci-hcd [173.297257] usb usb6: SerialNumber: xhci-hcd.2.auto [173.298680] hub 6-0:1.0: USB hub found [173.298749] hub 6-0:1.0: 1 port detected [173.299382] rockchip-dwc3 usb@fe80: USB HOST connected [173.395418] hub 5-0:1.0: activate --> -19 [173.603447] irq 228: nobody cared (try booting with the "irqpoll" option) [173.603493] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.4.21 #9 [173.603513] Hardware name: Google Kevin (DT) [173.603531] Call trace: [173.603568] [] dump_backtrace+0x0/0x160 [173.603596] [] show_stack+0x20/0x28 [173.603623] [] dump_stack+0x90/0xb0 [173.603650] [] __report_bad_irq+0x48/0xe8 [173.603674] [] note_interrupt+0x1e8/0x28c [173.603698] [] handle_irq_event_percpu+0x1d4/0x25c [173.603722] [] handle_irq_event+0x4c/0x7c [173.603748] [] handle_fasteoi_irq+0xb4/0x124 [173.603777] [] generic_handle_irq+0x30/0x44 [173.603804] [] __handle_domain_irq+0x90/0xbc [173.603827] [] gic_handle_irq+0xcc/0x188 ... [173.604500] [] el1_irq+0x80/0xf8 [173.604530] [] cpu_startup_entry+0x38/0x3cc [173.604558] [] rest_init+0x8c/0x94 [173.604585] [] start_kernel+0x3d0/0x3fc [173.604607] [<00b16000>] 0xb16000 [173.604622] handlers: [173.604648] [] usb_hcd_irq [173.604673] Disabling IRQ #228 Signed-off-by: William wu Signed-off-by: William wu --- drivers/usb/core/hcd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 479e223..612fab6 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -3017,6 +3017,7 @@ void usb_remove_hcd(struct usb_hcd *hcd) } usb_put_invalidate_rhdev(hcd); + hcd->flags = 0; } EXPORT_SYMBOL_GPL(usb_remove_hcd); I suggest to initialize hcd->flags to 0 in usb_add_hcd() instead. cheers, -roger Roger, didn't you originally propose this very same patch in http://marc.info/?l=linux-usb&m=146556415503583&w=2 (and of course, the earlier versions before v10)? What happened to it? Alan, You are right. That patch got lost in the ML. Looks like I didn't push it hard enough and then forgot about it. Sorry. William, You don't need to do any changes. My earlier version was clearing the flag in usb_add_hcd() and I guess Alan suggested to move it to usb_remove_hcd(). So your patch is good. You can add my. Acked-by: Roger Quadros cheers, -roger Thanks very much for your suggestion，I will add acked-by immediately.
Re: [PATCH] usb: host: xhci: plat: check hcc_params after add hcd
Hi Roger,
在 2017年01月13日 19:02, Roger Quadros 写道:
Hi,
On 13/01/17 05:18, William Wu wrote:
From: William wu
The commit 4ac53087d6d4 ("usb: xhci: plat: Create both
HCDs before adding them") move add hcd to the end of
probe, this cause hcc_params uninitiated, because xHCI
driver sets hcc_params in xhci_gen_setup() called from
usb_add_hcd().
This patch checks the Maximum Primary Stream Array Size
in the hcc_params register after add hcd.
Signed-off-by: William wu
---
drivers/usb/host/xhci-plat.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ddfab30..52ce697 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -232,9 +232,6 @@ static int xhci_plat_probe(struct platform_device *pdev)
if (device_property_read_bool(&pdev->dev, "usb3-lpm-capable"))
xhci->quirks |= XHCI_LPM_SUPPORT;
- if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
- xhci->shared_hcd->can_do_streams = 1;
-
hcd->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
if (IS_ERR(hcd->usb_phy)) {
ret = PTR_ERR(hcd->usb_phy);
@@ -255,6 +252,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
if (ret)
goto dealloc_usb2_hcd;
+ if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
+ xhci->shared_hcd->can_do_streams = 1;
+
xhci->hcc_params is initialized after the first usb_add_hcd().
Should this bit come before the usb_add_hcd(xhci->shared_hcd,..)?
Yes, good idea！I will move it behind the first usb_add_hcd().
You will also need to copy to v4.2+ . Thanks.
OK, thank you for reminding me!
return 0;
cheers,
-roger
Re: linux-next: build warning after merge of the phy-next tree
Hi Stephen，
在 2016年11月16日 11:00, Stephen Rothwell 写道:
Hi Kishon,
After merging the phy-next tree, today's linux-next build (x86_64
allmodconfig) produced this warning:
drivers/phy/phy-rockchip-inno-usb2.c: In function 'rockchip_chg_detect_work':
drivers/phy/phy-rockchip-inno-usb2.c:717:7: warning: 'tmout' may be used
uninitialized in this function [-Wmaybe-uninitialized]
if (tmout) {
^
Introduced by commit
0c42fe48fd23 ("phy: rockchip-inno-usb2: support otg-port for rk3399")
The variable "tmout" is initialized in the prior code "case
USB_CHG_STATE_WAIT_FOR_DCD",
and this case will be executed before "if (tmout)", so I think it's a
mis-warning for some
special compiler.
Could you provide me some suggestion about this warning? Should I upload
a new patch to
fix this compiler mis-warning or just ignore it ？ :-)
Best regards，
wulf
Re: [PATCH 1/2] phy: rockchip-inno-usb2: fix uninitialized tmout variable
Hi Arnd,
在 2016年11月16日 22:22, Arnd Bergmann 写道:
The newly added OTG support has an obvious uninitialized variable
access that gcc warns about:
drivers/phy/phy-rockchip-inno-usb2.c: In function 'rockchip_chg_detect_work':
drivers/phy/phy-rockchip-inno-usb2.c:717:7: error: 'tmout' may be used
uninitialized in this function [-Werror=maybe-uninitialized]
This replaces the use of the uninitialized variable with what
the value was in the previous USB_CHG_STATE_WAIT_FOR_DCD
state.
Fixes: 0c42fe48fd23 ("phy: rockchip-inno-usb2: support otg-port for rk3399")
Signed-off-by: Arnd Bergmann
---
drivers/phy/phy-rockchip-inno-usb2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/phy/phy-rockchip-inno-usb2.c
b/drivers/phy/phy-rockchip-inno-usb2.c
index eb89de59b68f..2f99ec95079c 100644
--- a/drivers/phy/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/phy-rockchip-inno-usb2.c
@@ -714,7 +714,7 @@ static void rockchip_chg_detect_work(struct work_struct
*work)
delay = CHG_SECONDARY_DET_TIME;
rphy->chg_state = USB_CHG_STATE_PRIMARY_DONE;
} else {
- if (tmout) {
+ if (rphy->dcd_retries == CHG_DCD_MAX_RETRIES) {
/* floating charger found */
rphy->chg_type = POWER_SUPPLY_TYPE_USB_DCP;
rphy->chg_state = USB_CHG_STATE_DETECTED;
Thanks very much for your help.
Reviewed-by: William Wu
Best Regards,
wulf
Re: [PATCH v2 1/2] phy: rockchip-inno-usb2: correct clk_ops callback
Hi Doug, 在 2016年11月15日 02:15, Doug Anderson 写道: William On Sun, Nov 13, 2016 at 11:01 PM, William Wu wrote: Since we needs to delay ~1ms to wait for 480MHz output clock of USB2 PHY to become stable after turn on it, the delay time is pretty long for something that's supposed to be "atomic" like a clk_enable(). Consider that clk_enable() will disable interrupt and that a 1ms interrupt latency is not sensible. The 480MHz output clock should be handled in prepare callbacks which support gate a clk if the operation may sleep. Signed-off-by: William Wu --- drivers/phy/phy-rockchip-inno-usb2.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) Reviewed-by: Douglas Anderson Thanks! I'll add Reviewed-by.
Re: [PATCH v3 2/2] phy: rockchip-inno-usb2: correct 480MHz output clock stable time
Hi Doug,
在 2016年11月15日 02:17, Doug Anderson 写道:
William,
On Mon, Nov 14, 2016 at 1:27 AM, William Wu wrote:
We found that the system crashed due to 480MHz output clock of
USB2 PHY was unstable after clock had been enabled by gpu module.
Theoretically, 1 millisecond is a critical value for 480MHz
output clock stable time, so we try to change the delay time
to 1.2 millisecond to avoid this issue.
And the commit ed907fb1d7c3 ("phy: rockchip-inno-usb2: correct
clk_ops callback") used prepare callbacks instead of enable
callbacks to support gate a clk if the operation may sleep. So
we can switch from delay to sleep functions.
Signed-off-by: William Wu
---
Changes in v3:
- fix kbuild test error: too few arguments to function 'usleep_range'
Changes in v2:
- use usleep_range() function instead of mdelay()
drivers/phy/phy-rockchip-inno-usb2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/phy/phy-rockchip-inno-usb2.c
b/drivers/phy/phy-rockchip-inno-usb2.c
index 365e077..0e52b25 100644
--- a/drivers/phy/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/phy-rockchip-inno-usb2.c
@@ -166,7 +166,7 @@ static int rockchip_usb2phy_clk480m_prepare(struct clk_hw
*hw)
return ret;
/* waitting for the clk become stable */
- mdelay(1);
+ usleep_range(1200, 1300);
Sight nit that you could also fix the spelling from "waitting" to "waiting".
...but that's pre-existing, so:
Reviewed-by: Douglas Anderson
Thanks! I'll add Reviewed-by and fix the spelling issue.
Re: [PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock stable time
Hi Doug, 在 2016年11月10日 04:54, Doug Anderson 写道: Hi, On Mon, Nov 7, 2016 at 5:00 AM, William Wu wrote: We found that the system crashed due to 480MHz output clock of USB2 PHY was unstable after clock had been enabled by gpu module. Theoretically, 1 millisecond is a critical value for 480MHz output clock stable time, so we try to change the delay time to 1.2 millisecond to avoid this issue. Signed-off-by: William Wu --- drivers/phy/phy-rockchip-inno-usb2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/phy/phy-rockchip-inno-usb2.c b/drivers/phy/phy-rockchip-inno-usb2.c index ecfd7d1..8f2d2b6 100644 --- a/drivers/phy/phy-rockchip-inno-usb2.c +++ b/drivers/phy/phy-rockchip-inno-usb2.c @@ -267,7 +267,7 @@ static int rockchip_usb2phy_clk480m_enable(struct clk_hw *hw) return ret; /* waitting for the clk become stable */ - mdelay(1); + udelay(1200); Several people who have seen this patch have expressed concern that a 1.2 ms delay is pretty long for something that's supposed to be "atomic" like a clk_enable(). Consider that someone might call clk_enable() while interrupts are disabled and that a 1.2 ms interrupt latency is not so great. It seems like this clock should be moved to be enabled in "prepare" and the "enable" should be a no-op. This is a functionality change, but I don't think there are any real users for this clock at the moment so it should be fine. (of course, the 1 ms latency that existed before this patch was still pretty bad, but ...) Thanks a lot for your suggestion. I agree with you. clk_enable() will call spin_lock_irqsave() to disable interrupt, and we add more than 1ms in clk_enable may cause big latency. And according to clk_prepare() description: In a simple case, clk_prepare can be used instead of clk_enable to ungate a clk if the operation may sleep. One example is a clk which is accessed over I2c. So maybe we can remove the clock to clk_prepare. Hi Heiko, Frank, What do you think of it? Best regards, wulf -Doug
Re: [PATCH] phy: rockchip-inno-usb2: correct 480MHz output clock stable time
Hi Heiko， 在 2016年11月10日 17:21, Heiko Stübner 写道: Am Donnerstag, 10. November 2016, 10:54:49 schrieb wlf: Hi Doug, 在 2016年11月10日 04:54, Doug Anderson 写道: Hi, On Mon, Nov 7, 2016 at 5:00 AM, William Wu wrote: We found that the system crashed due to 480MHz output clock of USB2 PHY was unstable after clock had been enabled by gpu module. Theoretically, 1 millisecond is a critical value for 480MHz output clock stable time, so we try to change the delay time to 1.2 millisecond to avoid this issue. Signed-off-by: William Wu --- drivers/phy/phy-rockchip-inno-usb2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/phy/phy-rockchip-inno-usb2.c b/drivers/phy/phy-rockchip-inno-usb2.c index ecfd7d1..8f2d2b6 100644 --- a/drivers/phy/phy-rockchip-inno-usb2.c +++ b/drivers/phy/phy-rockchip-inno-usb2.c @@ -267,7 +267,7 @@ static int rockchip_usb2phy_clk480m_enable(struct clk_hw *hw)>> return ret; /* waitting for the clk become stable */ - mdelay(1); + udelay(1200); Several people who have seen this patch have expressed concern that a 1.2 ms delay is pretty long for something that's supposed to be "atomic" like a clk_enable(). Consider that someone might call clk_enable() while interrupts are disabled and that a 1.2 ms interrupt latency is not so great. It seems like this clock should be moved to be enabled in "prepare" and the "enable" should be a no-op. This is a functionality change, but I don't think there are any real users for this clock at the moment so it should be fine. (of course, the 1 ms latency that existed before this patch was still pretty bad, but ...) Thanks a lot for your suggestion. I agree with you. clk_enable() will call spin_lock_irqsave() to disable interrupt, and we add more than 1ms in clk_enable may cause big latency. And according to clk_prepare() description: In a simple case, clk_prepare can be used instead of clk_enable to ungate a clk if the operation may sleep. One example is a clk which is accessed over I2c. So maybe we can remove the clock to clk_prepare. Hi Heiko, Frank, What do you think of it? moving to clk_prepare sounds sensible. That way you can switch from delay to sleep functions as well. Thanks very much. I will try to update a new patch. Best regards, Wulf Heiko
Re: [RESEND PATCH v2] arm64: dts: rockchip: add u2phy clock for ehci and ohci of rk3399
Dear Doug & Xing Zheng, 在 2016年12月22日 08:47, Doug Anderson 写道: Hi, On Wed, Dec 21, 2016 at 2:41 AM, Xing Zheng wrote: From: William wu We found that the suspend process was blocked when it run into ehci/ohci module due to clk-480m of usb2-phy was disabled. The root cause is that usb2-phy suspended earlier than ehci/ohci (usb2-phy will be auto suspended if no devices plug-in). and the clk-480m provided by it was disabled if no module used. However, I suggest that change the "clk-480m" to "utmi clock" here. some suspend process related ehci/ohci are base on this clock, so we should refer it into ehci/ohci driver to prevent this case. The u2phy clock flow like this: === u2phy |||-> UTMI_CLK -> | EHCI | OSC_24M ---|---> PHY_PLL|| |^___||-> 480M_CLK ---|G|---> | USBPHY_480M_SRC| > USBPHY_480M for SoC | | GRF === Signed-off-by: William wu Signed-off-by: Xing Zheng --- Changes in v2: - update the commit message - remove patches whic add and export the USBPHYx_480M_SRC clock IDs arch/arm64/boot/dts/rockchip/rk3399.dtsi | 28 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index b65c193..2ad9255 100644 --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi @@ -315,8 +315,10 @@ compatible = "generic-ehci"; reg = <0x0 0xfe38 0x0 0x2>; interrupts = ; - clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>; - clock-names = "hclk_host0", "hclk_host0_arb"; + clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>, +<&u2phy0>; + clock-names = "usbhost", "arbiter", + "utmi"; phys = <&u2phy0_host>; phy-names = "usb"; status = "disabled"; @@ -326,8 +328,12 @@ compatible = "generic-ohci"; reg = <0x0 0xfe3a 0x0 0x2>; interrupts = ; - clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>; - clock-names = "hclk_host0", "hclk_host0_arb"; + clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>, +<&u2phy0>; + clock-names = "usbhost", "arbiter", + "utmi"; + phys = <&u2phy0_host>; + phy-names = "usb"; status = "disabled"; }; @@ -335,8 +341,10 @@ compatible = "generic-ehci"; reg = <0x0 0xfe3c 0x0 0x2>; interrupts = ; - clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>; - clock-names = "hclk_host1", "hclk_host1_arb"; + clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>, +<&u2phy1>; + clock-names = "usbhost", "arbiter", + "utmi"; phys = <&u2phy1_host>; phy-names = "usb"; status = "disabled"; @@ -346,8 +354,12 @@ compatible = "generic-ohci"; reg = <0x0 0xfe3e 0x0 0x2>; interrupts = ; - clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>; - clock-names = "hclk_host1", "hclk_host1_arb"; + clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>, +<&u2phy1>; + clock-names = "usbhost", "arbiter", + "utmi"; + phys = <&u2phy1_host>; + phy-names = "usb"; This all looks better to me. From a device tree point of view it makes lots of sense to expose this PHY clock to the controller. Thus: Reviewed-by: Douglas Anderson I can't say that I understand all the interactions between the PHY code and the USB driver, but presumably others have reviewed that more? Offline Heiko pointed me at rockchip_usb2phy_otg_sm_work() which apparently calls rockchip_usb2phy_power_off() and rockchip_usb2phy_power_on() directly sometimes behind the back of the PHY framework. Very strange. Yes, the rockchip_usb2phy_otg_sm_work() in the USB2 PHY driver and EHCI/OHCI platform driver via the PHY framework can both do phy power on/off. We do that because we want to reduce USB2 PHY consumption in system runtime and deep sleep. For EHCI/OHCI platform drivers, they call phy_power_on() in probe to power on USB2 PHY, and don't power off USB2 PHY in system runtime, even if there's no USB2 device connected. They just call phy_power_off() in PM suspend, and call phy_power_on() in PM resume to power on USB2 PHY again. AKA the EHCI/OHCI platform driver can only support USB power management in deep s
Re: [PATCH v2 1/2] phy: rockchip-inno-usb2: support otg-port for rk3399
Hi Kishon,
在 2016年11月04日 01:17, Kishon Vijay Abraham I 写道:
On Thursday 03 November 2016 07:36 AM, William Wu wrote:
The rk3399 SoC USB2 PHY is comprised of one Host port and
one OTG port. And OTG port is for USB2.0 part of USB3.0 OTG
controller, as a part to construct a fully feature Type-C
subsystem.
With this patch, we can support OTG port with the following
functions:
- Support BC1.2 charger detect, and use extcon notifier to
send USB charger types to power driver.
- Support PHY suspend for power management.
- Support OTG Host only mode.
Also, correct 480MHz output clock stable time. We found that
the system crashed due to 480MHz output clock of USB2 PHY was
unstable after clock had been enabled by gpu module.
Can you split the clock fix into a separate patch?
OK, I will separate the clock patch in next version.
Best regards,
wulf
Thanks
Kishon
Theoretically, 1 millisecond is a critical value for 480 output
clock stable time, so we try changing the delay time to 1.2
millisecond to avoid this issue.
Signed-off-by: William Wu
---
Changes in v2:
- remove wakelock
drivers/phy/phy-rockchip-inno-usb2.c | 593 +--
1 file changed, 562 insertions(+), 31 deletions(-)
diff --git a/drivers/phy/phy-rockchip-inno-usb2.c
b/drivers/phy/phy-rockchip-inno-usb2.c
index ac20310..8f2d2b6 100644
--- a/drivers/phy/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/phy-rockchip-inno-usb2.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -30,11 +31,15 @@
#include
#include
#include
+#include
#include
#include
+#include
+#include
#define BIT_WRITEABLE_SHIFT 16
-#define SCHEDULE_DELAY (60 * HZ)
+#define SCHEDULE_DELAY (60 * HZ)
+#define OTG_SCHEDULE_DELAY (2 * HZ)
enum rockchip_usb2phy_port_id {
USB2PHY_PORT_OTG,
@@ -49,6 +54,37 @@ enum rockchip_usb2phy_host_state {
PHY_STATE_FS_LS_ONLINE = 4,
};
+/**
+ * Different states involved in USB charger detection.
+ * USB_CHG_STATE_UNDEFINED USB charger is not connected or detection
+ * process is not yet started.
+ * USB_CHG_STATE_WAIT_FOR_DCD Waiting for Data pins contact.
+ * USB_CHG_STATE_DCD_DONE Data pin contact is detected.
+ * USB_CHG_STATE_PRIMARY_DONE Primary detection is completed (Detects
+ * between SDP and DCP/CDP).
+ * USB_CHG_STATE_SECONDARY_DONESecondary detection is completed
(Detects
+ * between DCP and CDP).
+ * USB_CHG_STATE_DETECTED USB charger type is determined.
+ */
+enum usb_chg_state {
+ USB_CHG_STATE_UNDEFINED = 0,
+ USB_CHG_STATE_WAIT_FOR_DCD,
+ USB_CHG_STATE_DCD_DONE,
+ USB_CHG_STATE_PRIMARY_DONE,
+ USB_CHG_STATE_SECONDARY_DONE,
+ USB_CHG_STATE_DETECTED,
+};
+
+static const unsigned int rockchip_usb2phy_extcon_cable[] = {
+ EXTCON_USB,
+ EXTCON_USB_HOST,
+ EXTCON_CHG_USB_SDP,
+ EXTCON_CHG_USB_CDP,
+ EXTCON_CHG_USB_DCP,
+ EXTCON_CHG_USB_SLOW,
+ EXTCON_NONE,
+};
+
struct usb2phy_reg {
unsigned intoffset;
unsigned intbitend;
@@ -58,19 +94,55 @@ struct usb2phy_reg {
};
/**
+ * struct rockchip_chg_det_reg: usb charger detect registers
+ * @cp_det: charging port detected successfully.
+ * @dcp_det: dedicated charging port detected successfully.
+ * @dp_det: assert data pin connect successfully.
+ * @idm_sink_en: open dm sink curren.
+ * @idp_sink_en: open dp sink current.
+ * @idp_src_en: open dm source current.
+ * @rdm_pdwn_en: open dm pull down resistor.
+ * @vdm_src_en: open dm voltage source.
+ * @vdp_src_en: open dp voltage source.
+ * @opmode: utmi operational mode.
+ */
+struct rockchip_chg_det_reg {
+ struct usb2phy_reg cp_det;
+ struct usb2phy_reg dcp_det;
+ struct usb2phy_reg dp_det;
+ struct usb2phy_reg idm_sink_en;
+ struct usb2phy_reg idp_sink_en;
+ struct usb2phy_reg idp_src_en;
+ struct usb2phy_reg rdm_pdwn_en;
+ struct usb2phy_reg vdm_src_en;
+ struct usb2phy_reg vdp_src_en;
+ struct usb2phy_reg opmode;
+};
+
+/**
* struct rockchip_usb2phy_port_cfg: usb-phy port configuration.
* @phy_sus: phy suspend register.
+ * @bvalid_det_en: vbus valid rise detection enable register.
+ * @bvalid_det_st: vbus valid rise detection status register.
+ * @bvalid_det_clr: vbus valid rise detection clear register.
* @ls_det_en: linestate detection enable register.
* @ls_det_st: linestate detection state register.
* @ls_det_clr: linestate detection clear register.
+ * @utmi_avalid: utmi vbus avalid status register.
+ * @utmi_bvalid: utmi vbus bvalid status register.
* @utmi_ls: utmi linestate state register.
* @utmi_hstdet: utmi host disconnect register.
*/
struct rockchip_usb2phy_port_cfg {
struct usb2phy_reg phy_sus;
+ str
Re: [PATCH v2] usb: dwc3: add disable u2mac linestate check quirk
Dear Guenter,
在 2017年04月18日 21:18, Guenter Roeck 写道:
On Mon, Apr 17, 2017 at 10:17 PM, William Wu wrote:
This patch adds a quirk to disable USB 2.0 MAC linestate check
during HS transmit. Refer the dwc3 databook, we can use it for
some special platforms if the linestate not reflect the expected
line state(J) during transmission.
When use this quirk, the controller implements a fixed 40-bit
TxEndDelay after the packet is given on UTMI and ignores the
linestate during the transmit of a token (during token-to-token
and token-to-data IPGAP).
On some rockchip platforms (e.g. rk3399), it requires to disable
the u2mac linestate check to decrease the SSPLIT token to SETUP
token inter-packet delay from 566ns to 466ns, and fix the issue
that FS/LS devices not recognized if inserted through USB 3.0 HUB.
Signed-off-by: William Wu
---
Changes in v2:
- fix coding style
Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
drivers/usb/dwc3/core.c| 14 ++
drivers/usb/dwc3/core.h| 4
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
b/Documentation/devicetree/bindings/usb/dwc3.txt
index f658f39..6a89f0c 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -45,6 +45,8 @@ Optional properties:
a free-running PHY clock.
- snps,dis-del-phy-power-chg-quirk: when set core will change PHY power
from P0 to P1/P2/P3 without delay.
+ - snps,tx-ipgap-linecheck-dis-quirk: when set, disable u2mac linestate check
+ during HS transmit.
All other disable-something quirks are named
"snps,dis-something-quirk". Maybe use the same naming convention ?
Yes, good idea！ I will fix it with "snps,dis-tx-ipgap-linecheck-quirk"
in next patch verison.
Thanks:-)
- snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
utmi_l1_suspend_n, false when asserts utmi_sleep_n
- snps,hird-threshold: HIRD threshold
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 455d89a..03429c5 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -796,15 +796,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
dwc3_writel(dwc->regs, DWC3_GUCTL2, reg);
}
+ reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+
My understanding is that the register was only introduced with dwc3
revision 2.50a. Is it ok to read and write it unconditionally ?
Yes, refer to dwc3 databook, the DWC3_GUCTL1 was introduced since 2.50a.
Maybe it's better
to read and write it only when we know our controller version.
Is it good to fix it like the following patch?
But this patch has a problem that we need to read and write the register
twice if our controller verison > = 2.90a, and need this quirk.
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -806,6 +806,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
}
+ if (dwc->dis_tx_ipgap_linecheck_quirk) {
+ reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+ reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
+ dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
+ }
+
Hi John & Felipe,
Could you provide me some suggestion？
Thank you！
/*
* Enable hardware control of sending remote wakeup in HS when
* the device is in the L1 state.
*/
- if (dwc->revision >= DWC3_REVISION_290A) {
- reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+ if (dwc->revision >= DWC3_REVISION_290A)
reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
- dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
- }
+
+ if (dwc->tx_ipgap_linecheck_dis_quirk)
+ reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
+
+ dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
return 0;
@@ -1023,6 +1027,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
"snps,dis-u2-freeclk-exists-quirk");
dwc->dis_del_phy_power_chg_quirk = device_property_read_bool(dev,
"snps,dis-del-phy-power-chg-quirk");
+ dwc->tx_ipgap_linecheck_dis_quirk = device_property_read_bool(dev,
+ "snps,tx-ipgap-linecheck-dis-quirk");
dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
"snps,tx_de_emphasis_quirk");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 981c77f..3c2537b 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -204,6 +204,7 @@
#define DWC3_GCTL_DSBLCLKGTNG BIT(0)
/* Global User Control 1 Register */
+#define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS BIT(28)
#define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW BIT(24)
/* Global USB2 P
Re: [PATCH v2] usb: dwc3: add disable u2mac linestate check quirk
Dear Guenter，
在 2017年04月19日 13:15, Guenter Roeck 写道:
On Tue, Apr 18, 2017 at 8:59 PM, wlf wrote:
Dear Guenter,
在 2017年04月18日 21:18, Guenter Roeck 写道:
On Mon, Apr 17, 2017 at 10:17 PM, William Wu
wrote:
This patch adds a quirk to disable USB 2.0 MAC linestate check
during HS transmit. Refer the dwc3 databook, we can use it for
some special platforms if the linestate not reflect the expected
line state(J) during transmission.
When use this quirk, the controller implements a fixed 40-bit
TxEndDelay after the packet is given on UTMI and ignores the
linestate during the transmit of a token (during token-to-token
and token-to-data IPGAP).
On some rockchip platforms (e.g. rk3399), it requires to disable
the u2mac linestate check to decrease the SSPLIT token to SETUP
token inter-packet delay from 566ns to 466ns, and fix the issue
that FS/LS devices not recognized if inserted through USB 3.0 HUB.
Signed-off-by: William Wu
---
Changes in v2:
- fix coding style
Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
drivers/usb/dwc3/core.c| 14 ++
drivers/usb/dwc3/core.h| 4
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
b/Documentation/devicetree/bindings/usb/dwc3.txt
index f658f39..6a89f0c 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -45,6 +45,8 @@ Optional properties:
a free-running PHY clock.
- snps,dis-del-phy-power-chg-quirk: when set core will change PHY
power
from P0 to P1/P2/P3 without delay.
+ - snps,tx-ipgap-linecheck-dis-quirk: when set, disable u2mac linestate
check
+ during HS transmit.
All other disable-something quirks are named
"snps,dis-something-quirk". Maybe use the same naming convention ?
Yes, good idea！ I will fix it with "snps,dis-tx-ipgap-linecheck-quirk" in
next patch verison.
Thanks:-)
- snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
utmi_l1_suspend_n, false when asserts
utmi_sleep_n
- snps,hird-threshold: HIRD threshold
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 455d89a..03429c5 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -796,15 +796,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
dwc3_writel(dwc->regs, DWC3_GUCTL2, reg);
}
+ reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+
My understanding is that the register was only introduced with dwc3
revision 2.50a. Is it ok to read and write it unconditionally ?
Yes, refer to dwc3 databook, the DWC3_GUCTL1 was introduced since 2.50a.
Maybe it's better
to read and write it only when we know our controller version.
Is it good to fix it like the following patch?
But this patch has a problem that we need to read and write the register
twice if our controller verison > = 2.90a, and need this quirk.
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -806,6 +806,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
}
+ if (dwc->dis_tx_ipgap_linecheck_quirk) {
+ reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+ reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
+ dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
+ }
+
How about this ?
if (dwc->revision >= DWC3_REVISION_250A) {
reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
if (dwc->revision >= DWC3_REVISION_290A)
reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
if (dwc->dis_tx_ipgap_linecheck_quirk)
reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
}
Thanks,
Guenter
Thanks， looks good to me. I will fix it in patch v2.
Hi John & Felipe,
Could you provide me some suggestion？
Thank you！
/*
* Enable hardware control of sending remote wakeup in HS when
* the device is in the L1 state.
*/
- if (dwc->revision >= DWC3_REVISION_290A) {
- reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+ if (dwc->revision >= DWC3_REVISION_290A)
reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
- dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
- }
+
+ if (dwc->tx_ipgap_linecheck_dis_quirk)
+ reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
+
+ dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
return 0;
@@ -1023,6 +1027,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
"snps,dis-u2-freeclk-exists-quirk");
dwc->dis_del_phy_power_chg_quirk =
device_property_read_bool(dev,
&q
Re: [PATCH 1/3] dt-bindings: phy: phy-rockchip-typec: add usb3 otg reset
Dear Heiko, 在 2018年02月11日 03:55, Heiko Stuebner 写道: Hi Wulf, Am Montag, 22. Januar 2018, 12:33:05 CET schrieb wlf: 在 2018年01月20日 05:49, Rob Herring 写道: On Thu, Jan 18, 2018 at 09:47:50AM -0800, Brian Norris wrote: On Thu, Jan 18, 2018 at 06:20:09PM +0100, Enric Balletbo Serra wrote: As Brian said commit 06c47e6286d5 'usb: dwc3: of-simple: Add support to get resets for the device' introduced the support to get the resets from dwc3-of-simple and the queued commit 'b7e63d95c14d arm64: dts: rockchip: add reset property for dwc3 controllers on rk3399' started using it. Without the latest I get errors like this doing bind/unbind tests. dwc3: probe of fe90.dwc3 failed with error -110 I just tested these series on top of mainline, I reverted my patch because otherwise two drivers are requesting the same reset and fails, and I did some of the bind/unbind test. They just worked fine, and seems that this is right way, so this makes me think some questions. Actually, this was intended to coexist with DWC3 optionally controlling the same reset. It was written before the reset framework was rewritten to have shared and exclusive resets. Should this be rewritten to use shared resets? We'd have to modify both dwc3 core and the PHY driver. Seems like abuse of DT to me. If you need to control the controller's reset from the phy driver, then get the reset out of the controller node. The phy node should describe the connections to the phy. I try to get the reset out of the controller, but I don't find a good way to get the reset ofthe controller associated with the given phy device node. Is there an API like theof_usb_get_dr_mode_by_phy() to get 'dr_mode' of the controller? I guess the easiest way would be taking the of_usb_get_dr_mode_by_phy() function move the part above the "finish" label into a separate function like of_usb_get_node_by_phy (or possibly move that to an even more general place as it is not usb-related at all) and use that function in of_usb_get_dr_mode_by_phy() and also use it to get the reset you want via something like: node = of_usb_get_node_by_phy(...); rst = of_reset_control_get(node, ...); Thanks for your good suggestion. I think it's a good way to get the reset of the controller by phy. And we're trying to find another method to fix the RK3399 tcphy power on fail issue.If we get another proper method, we may not need these phy patch series. Did you find any different solution for that yet? Heiko Yes, finally, we have found another way to fix the RK3399 tcphy usb power on fail issue. And Enric Balletbo i Serra has submitted new patch series, refer to the following patches: https://patchwork.kernel.org/patch/10207261/ https://patchwork.kernel.org/patch/10207267/ https://patchwork.kernel.org/patch/10207263/ So my patches can be abandoned.
Re: [PATCH] usb: gadget: f_fs: get the correct address of comp_desc
Hi Jack,
在 2018年02月06日 02:17, Jack Pham 写道:
Hi William,
On Mon, Feb 05, 2018 at 07:33:38PM +0800, William Wu wrote:
Refer to the USB 3.0 spec '9.6.7 SuperSpeed Endpoint Companion',
the companion descriptor follows the standard endpoint descriptor.
This descriptor is only defined for SuperSpeed endpoints. The
f_fs driver gets the address of the companion descriptor via
'ds + USB_DT_ENDPOINT_SIZE', and actually, the ds variable is
a pointer to the struct usb_endpoint_descriptor, so the offset
of the companion descriptor which we get is USB_DT_ENDPOINT_SIZE *
sizeof(struct usb_endpoint_descriptor), the wrong offset is 63
bytes. This cause out-of-bound with the following error log if
CONFIG_KASAN and CONFIG_SLUB_DEBUG is enabled on Rockchip RK3399
Evaluation Board.
android_work: sent uevent USB_STATE=CONNECTED
configfs-gadget gadget: super-speed config #1: b
==
BUG: KASAN: slab-out-of-bounds in ffs_func_set_alt+0x230/0x398
Read of size 1 at addr ffc0ce2d0b10 by task irq/224-dwc3/364
Memory state around the buggy address:
ffc0ce2d0a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffc0ce2d0a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffc0ce2d0b00: 00 04 fc fc fc fc fc fc fc fc fc fc fc fc fc fc
^
ffc0ce2d0b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffc0ce2d0c00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==
Disabling lock debugging due to kernel taint
android_work: sent uevent USB_STATE=CONFIGURED
This patch adds struct usb_endpoint_descriptor * -> u8 * type conversion
for ds variable, then we can get the correct address of comp_desc
with offset USB_DT_ENDPOINT_SIZE bytes.
Signed-off-by: William Wu
---
drivers/usb/gadget/function/f_fs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 6756472..f13ead0 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1882,8 +1882,8 @@ static int ffs_func_eps_enable(struct ffs_function *func)
ep->ep->desc = ds;
if (needs_comp_desc) {
- comp_desc = (struct usb_ss_ep_comp_descriptor *)(ds +
- USB_DT_ENDPOINT_SIZE);
+ comp_desc = (struct usb_ss_ep_comp_descriptor *)
+((u8 *)ds + USB_DT_ENDPOINT_SIZE);
ep->ep->maxburst = comp_desc->bMaxBurst + 1;
ep->ep->comp_desc = comp_desc;
}
Please see my alternative fix for this. I proposed changing this
function to use config_ep_by_speed() instead.
https://www.spinics.net/lists/linux-usb/msg165149.html
Thanks for your great job!
Your patch seems good, I will test your patch on my RK3399-EVB board.
William
Jack
Re: [PATCH v3 1/3] dt-bindings: usb: add DT binding for RK3328 dwc3 controller
Dear Balbi & Gregkh, 在 2017年08月21日 21:36, William Wu 写道: Adds the device tree bindings description for RK3328 and compatible USB DWC3 controller. Signed-off-by: William Wu --- Changes in v3: - Add this for separate usb dt-bindings patch. Changes in v2: - None Documentation/devicetree/bindings/usb/rockchip,dwc3.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt index 0536a93..d6b2e47 100644 --- a/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt +++ b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt @@ -1,7 +1,9 @@ Rockchip SuperSpeed DWC3 USB SoC controller Required properties: -- compatible: should contain "rockchip,rk3399-dwc3" for rk3399 SoC +- compatible: should be one of the following: + - "rockchip,rk3399-dwc3": for rk3399 SoC + - "rockchip,rk3328-dwc3", "rockchip,rk3399-dwc3": for rk3328 SoC - clocks: A list of phandle + clock-specifier pairs for the clocks listed in clock-names - clock-names:Should contain the following: Could you help to reviewed this patch? According to Heiko's suggestion, this patch need to be submitted to usb tree or at least get an Ack from you. Thank you! -- 吴良峰 William.Wu 福建省福州市铜盘路软件大道89号软件园A区21号楼 No.21 Building, A District, No.89,software Boulevard Fuzhou,Fujian, PRC 手机: 13685012275 座机: 0591-83991906-8520 邮件:w...@rock-chips.com
Re: [PATCH v2 1/2] arm64: dts: rockchip: add usb3 controller node for RK3328 SoCs
Dear Heiko, 在 2017年08月18日 17:24, Heiko Stuebner 写道: Hi William, Am Donnerstag, 17. August 2017, 15:54:49 CEST schrieb William Wu: RK3328 has one USB 3.0 OTG controller which uses DWC_USB3 core's general architecture. It can act as static xHCI host controller, static device controller, USB 3.0/2.0 OTG basing on ID of USB3.0 PHY. Signed-off-by: William Wu --- Changes in v2: - Modify the dwc3 quirk "snps,tx-ipgap-linecheck-dis-quirk" to "snps,dis-tx-ipgap-linecheck-quirk" .../devicetree/bindings/usb/rockchip,dwc3.txt | 4 +++- arch/arm64/boot/dts/rockchip/rk3328.dtsi | 27 ++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt index 0536a93..d6b2e47 100644 --- a/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt +++ b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt @@ -1,7 +1,9 @@ Rockchip SuperSpeed DWC3 USB SoC controller Required properties: -- compatible: should contain "rockchip,rk3399-dwc3" for rk3399 SoC +- compatible: should be one of the following: + - "rockchip,rk3399-dwc3": for rk3399 SoC + - "rockchip,rk3328-dwc3", "rockchip,rk3399-dwc3": for rk3328 SoC - clocks: A list of phandle + clock-specifier pairs for the clocks listed in clock-names - clock-names:Should contain the following: This probably shouldn't be part of the patch adding the dts node, but instead should be a separate patch and should either go through some usb tree or at least get an Ack from usb maintainers (Felipe Balbi and/or Greg Kroah Hartman), so you should definitly include them into your recipient list. Thanks for your suggestion. I will submit two patches separately, and add usb maintainers in the recipient list. Heiko -- 吴良峰 William.Wu 福建省福州市铜盘路软件大道89号软件园A区21号楼 No.21 Building, A District, No.89,software Boulevard Fuzhou,Fujian, PRC 手机: 13685012275 座机: 0591-83991906-8520 邮件:w...@rock-chips.com
Re: [PATCH 1/2] ARM: dts: rockchip: add usb nodes on rk322x
Dear Heiko,
在 2017年06月02日 04:22, Heiko Stuebner 写道:
Hi William,
Am Mittwoch, 31. Mai 2017, 09:21:23 CEST schrieb William Wu:
This patch adds usb otg/host controllers and phys nodes on rk322x.
Signed-off-by: William Wu
---
arch/arm/boot/dts/rk322x.dtsi | 138 +-
1 file changed, 137 insertions(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/rk322x.dtsi b/arch/arm/boot/dts/rk322x.dtsi
index df57413..d4bfd3c 100644
--- a/arch/arm/boot/dts/rk322x.dtsi
+++ b/arch/arm/boot/dts/rk322x.dtsi
@@ -210,8 +210,61 @@
};
grf: syscon@1100 {
- compatible = "syscon";
+ compatible = "syscon", "simple-mfd";
reg = <0x1100 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ u2phy0: usb2-phy@760 {
+ compatible = "rockchip,rk322x-usb2phy";
as commented on the driver-side some moments ago, compatibles should not
contain wildcards, so please make this
"rockchip,rk3228-usb2phy"
instead.
Same below and also for the dwc2 node.
Thanks, I'll fix it immediately.
Thanks
Heiko
+ reg = <0x0760 0x0c>;
+ clocks = <&cru SCLK_OTGPHY0>;
+ clock-names = "phyclk";
+ #clock-cells = <0>;
+ clock-output-names = "usb480m_phy0";
+ status = "disabled";
+
+ u2phy0_otg: otg-port {
+ #phy-cells = <0>;
+ interrupts = ,
+,
+;
+ interrupt-names = "otg-bvalid", "otg-id",
+ "linestate";
+ status = "disabled";
+ };
+
+ u2phy0_host: host-port {
+ #phy-cells = <0>;
+ interrupts = ;
+ interrupt-names = "linestate";
+ status = "disabled";
+ };
+ };
+
+ u2phy1: usb2-phy@800 {
+ compatible = "rockchip,rk322x-usb2phy";
rockchip,rk3228-usb2phy
OK, I'll fix it immediately.
+ reg = <0x0800 0x0c>;
+ clocks = <&cru SCLK_OTGPHY1>;
+ clock-names = "phyclk";
+ #clock-cells = <0>;
+ clock-output-names = "usb480m_phy1";
+ status = "disabled";
+
+ u2phy1_otg: otg-port {
+ #phy-cells = <0>;
+ interrupts = ;
+ interrupt-names = "linestate";
+ status = "disabled";
+ };
+
+ u2phy1_host: host-port {
+ #phy-cells = <0>;
+ interrupts = ;
+ interrupt-names = "linestate";
+ status = "disabled";
+ };
+ };
};
uart0: serial@1101 {
@@ -467,6 +520,89 @@
status = "disabled";
};
+ usb_otg: usb@3004 {
+ compatible = "rockchip,rk322x-usb", "rockchip,rk3066-usb",
rockchip,rk3228-usb
OK, I'll fix it immediately.:-)
+"snps,dwc2";
+ reg = <0x3004 0x4>;
+ interrupts = ;
+ clocks = <&cru HCLK_OTG>;
+ clock-names = "otg";
+ dr_mode = "otg";
+ g-np-tx-fifo-size = <16>;
+ g-rx-fifo-size = <280>;
+ g-tx-fifo-size = <256 128 128 64 32 16>;
+ g-use-dma;
+ phys = <&u2phy0_otg>;
+ phy-names = "usb2-phy";
+ status = "disabled";
+ };
+
+ usb_host0_ehci: usb@3008 {
+ compatible = "generic-ehci";
+ reg = <0x3008 0x2>;
+ interrupts = ;
+ clocks = <&cru HCLK_HOST0>, <&u2phy0>;
+ clock-names = "usbhost", "utmi";
+ phys = <&u2phy0_host>;
+ phy-names = "usb";
+ status = "disabled";
+ };
+
+ usb_host0_ohci: usb@300a {
+ compatible = "generic-ohci";
+ reg = <0x300a 0x2>;
+ interrupts = ;
+ clocks = <&cru HCLK_HOST0>, <&u2phy0>;
+ clock-names = "usbhost", "utmi";
+ phys = <&u2phy0_host>;
+ phy-names = "usb";
+ status = "disabled";
+ };
+
+ usb_host1_ehci: usb@300c {
+ compatible = "generic-ehci";
+ reg = <0x300c 0x2>;
+
Re: [PATCH] usb: dwc2: host: fix isoc urb actual length
Hi Alan,
在 2017年11月07日 03:17, Alan Stern 写道:
On Mon, 6 Nov 2017, wlf wrote:
Hi Minas,
在 2017年11月06日 17:28, Minas Harutyunyan 写道:
Hi,
On 11/6/2017 12:46 PM, William Wu wrote:
The actual_length in dwc2_hcd_urb structure is used
to indicate the total data length transferred so far,
but in dwc2_update_isoc_urb_state(), it just updates
the actual_length of isoc frame, and don't update the
urb actual_length at the same time, this will cause
device drivers working error which depend on the urb
actual_length.
we can easily find this issue if use an USB camera，
the userspace use libusb to get USB data from kernel
via devio driver.In usb devio driver, processcompl()
function will process urb complete and copy data to
userspace depending on urb actual_length.
Let's update the urb actual_length if the isoc frame
is valid.
Signed-off-by: William Wu
---
drivers/usb/dwc2/hcd_intr.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 28a8210..01b1e13 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -580,6 +580,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state(
frame_desc->status = 0;
frame_desc->actual_length = dwc2_get_actual_xfer_length(hsotg,
chan, chnum, qtd, halt_status, NULL);
+ urb->actual_length += frame_desc->actual_length;
break;
case DWC2_HC_XFER_FRAME_OVERRUN:
urb->error_count++;
@@ -599,6 +600,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state(
frame_desc->status = -EPROTO;
frame_desc->actual_length = dwc2_get_actual_xfer_length(hsotg,
chan, chnum, qtd, halt_status, NULL);
+ urb->actual_length += frame_desc->actual_length;
/* Skip whole frame */
if (chan->qh->do_split &&
According urb description in usb.h urb->actual_length used for non-iso
transfers:
"@actual_length: This is read in non-iso completion functions, and ...
* ISO transfer status is reported in the status and actual_length fields
* of the iso_frame_desc array, "
Yes, urb->actual_length is used for non-iso transfers. And for ISO
transfer, the most
usb class drivers can only use iso frame status and actual_length to
handle usb data,
like: kernel v4.4 drivers/media/usb/uvc/uvc_video.c
But the usb devio driver really need the urb actual_length in the
processcompl() if
handle ISO transfer, just as I mentioned in the commit message.
And I also found the same issue on raspberrypi board:
https://github.com/raspberrypi/linux/issues/903
So do you think we need to fix the usb devio driver, rather than fix dwc2?
I think maybe we can use urb actual length for ISO transfer, it seems that
don't cause any side-effect.
That sounds like a good idea. Minas, does the following patch fix your
problem?
In theory we could do this calculation for every isochronous URB, not
just those coming from usbfs. But I don't think there's any point,
since the USB class drivers don't use it.
Alan Stern
Index: usb-4.x/drivers/usb/core/devio.c
===
--- usb-4.x.orig/drivers/usb/core/devio.c
+++ usb-4.x/drivers/usb/core/devio.c
@@ -1828,6 +1828,18 @@ static int proc_unlinkurb(struct usb_dev
return 0;
}
+static void compute_isochronous_actual_length(struct urb *urb)
+{
+ unsigned i;
+
+ if (urb->number_of_packets > 0) {
+ urb->actual_length = 0;
+ for (i = 0; i < urb->number_of_packets; i++)
+ urb->actual_length +=
+ urb->iso_frame_desc[i].actual_length;
+ }
+}
+
static int processcompl(struct async *as, void __user * __user *arg)
{
struct urb *urb = as->urb;
@@ -1835,6 +1847,8 @@ static int processcompl(struct async *as
void __user *addr = as->userurb;
unsigned int i;
+ compute_isochronous_actual_length(urb);
+
if (as->userbuffer && urb->actual_length) {
if (copy_urb_data_to_user(as->userbuffer, urb))
goto err_out;
@@ -2003,6 +2017,8 @@ static int processcompl_compat(struct as
void __user *addr = as->userurb;
unsigned int i;
+ compute_isochronous_actual_length(urb);
+
if (as->userbuffer && urb->actual_length) {
if (copy_urb_data_to_user(as->userbuffer, urb))
return -EFAULT;
Yeah, it's a good idea to calculate the urb actual length in the devio
driver.
Your patch seems good, and I think we can do a small optimization base
your patch,
like the following patch:
diff --git a/drivers/usb/core/dev
Re: [PATCH] usb: dwc2: host: fix isoc urb actual length
Hi Alan,
在 2017年11月07日 23:18, Alan Stern 写道:
On Tue, 7 Nov 2017, wlf wrote:
That sounds like a good idea. Minas, does the following patch fix your
problem?
In theory we could do this calculation for every isochronous URB, not
just those coming from usbfs. But I don't think there's any point,
since the USB class drivers don't use it.
Alan Stern
Index: usb-4.x/drivers/usb/core/devio.c
===
--- usb-4.x.orig/drivers/usb/core/devio.c
+++ usb-4.x/drivers/usb/core/devio.c
@@ -1828,6 +1828,18 @@ static int proc_unlinkurb(struct usb_dev
return 0;
}
+static void compute_isochronous_actual_length(struct urb *urb)
+{
+ unsigned i;
+
+ if (urb->number_of_packets > 0) {
+ urb->actual_length = 0;
+ for (i = 0; i < urb->number_of_packets; i++)
+ urb->actual_length +=
+ urb->iso_frame_desc[i].actual_length;
+ }
+}
+
static int processcompl(struct async *as, void __user * __user *arg)
{
struct urb *urb = as->urb;
@@ -1835,6 +1847,8 @@ static int processcompl(struct async *as
void __user *addr = as->userurb;
unsigned int i;
+ compute_isochronous_actual_length(urb);
+
if (as->userbuffer && urb->actual_length) {
if (copy_urb_data_to_user(as->userbuffer, urb))
goto err_out;
@@ -2003,6 +2017,8 @@ static int processcompl_compat(struct as
void __user *addr = as->userurb;
unsigned int i;
+ compute_isochronous_actual_length(urb);
+
if (as->userbuffer && urb->actual_length) {
if (copy_urb_data_to_user(as->userbuffer, urb))
return -EFAULT;
Yeah, it's a good idea to calculate the urb actual length in the devio
driver.
Your patch seems good, and I think we can do a small optimization base
your patch,
like the following patch:
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index bd94192..a2e7b97 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1664,6 +1664,9 @@ static int processcompl(struct async *as, void
__user * __user *arg)
void __user *addr = as->userurb;
unsigned int i;
+ if (usb_endpoint_xfer_isoc(&urb->ep->desc))
+ compute_isochronous_actual_length(urb);
+
if (as->userbuffer && urb->actual_length) {
if (copy_urb_data_to_user(as->userbuffer, urb))
goto err_out;
@@ -1833,6 +1836,9 @@ static int processcompl_compat(struct async *as,
void __user * __user *arg)
void __user *addr = as->userurb;
unsigned int i;
+ if (usb_endpoint_xfer_isoc(&urb->ep->desc))
+ compute_isochronous_actual_length(urb);
+
Well, this depends on whether you want to optimize for space or for
speed. I've been going for space. And since usbfs is inherently
rather slow, I don't think this will make any significant speed
difference. So I don't think adding the extra tests is worthwhile.
(Besides, if you really wanted to do it this way, you should have moved
the test for "urb->number_of_packets > 0" into the callers instead of
adding an additional test of the endpoint type.)
Yes, agree with you.
However, nobody has answered my original question: Does the patch
actually fix the problem?
I test your patch on Rockchip RK3188 platform, use UsbWebCamera APP by
Serenegiant
(libusb + devio) with usb camera, it work well.
Alan Stern
--
吴良峰 William.Wu
福建省福州市铜盘路软件大道89号软件园A区21号楼
No.21 Building, A District, No.89,software Boulevard Fuzhou,Fujian, PRC
手机: 13685012275
座机: 0591-83991906-8520
邮件:w...@rock-chips.com
