| Message ID | 20200421165059.19394-2-s.nawrocki@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB host support for Raspberry Pi 4 board | expand |
On Wed, Apr 22, 2020 at 12:51 AM Sylwester Nawrocki <s.nawrocki at samsung.com> wrote: > > In current code there is no cache flush after initializing the scratchpad > buffer array with the scratchpad buffer pointers. This leads to a failure > of the "slot enable" command on the rpi4 board (Broadcom STB PCIe > controller + VL805 USB hub) - the very first TRB transfer on the command > ring fails and there is a timeout while waiting for the command completion > event. After adding the missing cache flush everything seems to be working > as expected. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com> > --- > drivers/usb/host/xhci-mem.c | 3 +++ > 1 file changed, 3 insertions(+) > Good catch! Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
Hi Sylwester, Marek On Tue, 2020-04-21 at 18:50 +0200, Sylwester Nawrocki wrote: > In current code there is no cache flush after initializing the scratchpad > buffer array with the scratchpad buffer pointers. This leads to a failure > of the "slot enable" command on the rpi4 board (Broadcom STB PCIe > controller + VL805 USB hub) - the very first TRB transfer on the command > ring fails and there is a timeout while waiting for the command completion > event. After adding the missing cache flush everything seems to be working > as expected. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com> > Reviewed-by: Bin Meng <bmeng.cn at gmail.com> > --- I've been trying to get this working on my own and got stuck with this specific issue. I'm glad you found a solution, it was driving me crazy. Out of curiosity how did you found the solution? > drivers/usb/host/xhci-mem.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > index 93450ee..729bdc3 100644 > --- a/drivers/usb/host/xhci-mem.c > +++ b/drivers/usb/host/xhci-mem.c > @@ -393,6 +393,9 @@ static int xhci_scratchpad_alloc(struct xhci_ctrl *ctrl) > scratchpad->sp_array[i] = cpu_to_le64(ptr); > } > > + xhci_flush_cache((uintptr_t)scratchpad->sp_array, > + sizeof(u64) * num_sp); > + Marek, souldn't running 'dcache off; icache off' be equivalent to this (which didn't do the trick for me)? or am I missing somthing? Regards, Nicolas -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: This is a digitally signed message part URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200422/62601f47/attachment.sig>
Hi Nicolas, On Wed, Apr 22, 2020 at 4:53 PM Nicolas Saenz Julienne <nsaenzjulienne at suse.de> wrote: > > Hi Sylwester, Marek > > On Tue, 2020-04-21 at 18:50 +0200, Sylwester Nawrocki wrote: > > In current code there is no cache flush after initializing the scratchpad > > buffer array with the scratchpad buffer pointers. This leads to a failure > > of the "slot enable" command on the rpi4 board (Broadcom STB PCIe > > controller + VL805 USB hub) - the very first TRB transfer on the command > > ring fails and there is a timeout while waiting for the command completion > > event. After adding the missing cache flush everything seems to be working > > as expected. > > > > Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com> > > Reviewed-by: Bin Meng <bmeng.cn at gmail.com> > > --- > > I've been trying to get this working on my own and got stuck with this specific > issue. I'm glad you found a solution, it was driving me crazy. > > Out of curiosity how did you found the solution? > > > drivers/usb/host/xhci-mem.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > > index 93450ee..729bdc3 100644 > > --- a/drivers/usb/host/xhci-mem.c > > +++ b/drivers/usb/host/xhci-mem.c > > @@ -393,6 +393,9 @@ static int xhci_scratchpad_alloc(struct xhci_ctrl *ctrl) > > scratchpad->sp_array[i] = cpu_to_le64(ptr); > > } > > > > + xhci_flush_cache((uintptr_t)scratchpad->sp_array, > > + sizeof(u64) * num_sp); > > + > > Marek, souldn't running 'dcache off; icache off' be equivalent to this (which > didn't do the trick for me)? or am I missing somthing? I guess something is wrong with RPi4's "dcache off" command .. Regards, Bin
On Wed, 2020-04-22 at 17:21 +0800, Bin Meng wrote: > Hi Nicolas, > > On Wed, Apr 22, 2020 at 4:53 PM Nicolas Saenz Julienne > <nsaenzjulienne at suse.de> wrote: > > Hi Sylwester, Marek > > > > On Tue, 2020-04-21 at 18:50 +0200, Sylwester Nawrocki wrote: > > > In current code there is no cache flush after initializing the scratchpad > > > buffer array with the scratchpad buffer pointers. This leads to a failure > > > of the "slot enable" command on the rpi4 board (Broadcom STB PCIe > > > controller + VL805 USB hub) - the very first TRB transfer on the command > > > ring fails and there is a timeout while waiting for the command completion > > > event. After adding the missing cache flush everything seems to be working > > > as expected. > > > > > > Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com> > > > Reviewed-by: Bin Meng <bmeng.cn at gmail.com> > > > --- > > > > I've been trying to get this working on my own and got stuck with this > > specific > > issue. I'm glad you found a solution, it was driving me crazy. > > > > Out of curiosity how did you found the solution? > > > > > drivers/usb/host/xhci-mem.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > > > index 93450ee..729bdc3 100644 > > > --- a/drivers/usb/host/xhci-mem.c > > > +++ b/drivers/usb/host/xhci-mem.c > > > @@ -393,6 +393,9 @@ static int xhci_scratchpad_alloc(struct xhci_ctrl > > > *ctrl) > > > scratchpad->sp_array[i] = cpu_to_le64(ptr); > > > } > > > > > > + xhci_flush_cache((uintptr_t)scratchpad->sp_array, > > > + sizeof(u64) * num_sp); > > > + > > > > Marek, souldn't running 'dcache off; icache off' be equivalent to this > > (which > > didn't do the trick for me)? or am I missing somthing? > > I guess something is wrong with RPi4's "dcache off" command .. You can't trust anything these times :) I'll look into it. Regards, Nicolas -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: This is a digitally signed message part URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200422/35821d5b/attachment.sig>
On Wed, 2020-04-22 at 11:26 +0200, Nicolas Saenz Julienne wrote: > On Wed, 2020-04-22 at 17:21 +0800, Bin Meng wrote: > > Hi Nicolas, > > > > On Wed, Apr 22, 2020 at 4:53 PM Nicolas Saenz Julienne > > <nsaenzjulienne at suse.de> wrote: > > > Hi Sylwester, Marek > > > > > > On Tue, 2020-04-21 at 18:50 +0200, Sylwester Nawrocki wrote: > > > > In current code there is no cache flush after initializing the > > > > scratchpad > > > > buffer array with the scratchpad buffer pointers. This leads to a > > > > failure > > > > of the "slot enable" command on the rpi4 board (Broadcom STB PCIe > > > > controller + VL805 USB hub) - the very first TRB transfer on the command > > > > ring fails and there is a timeout while waiting for the command > > > > completion > > > > event. After adding the missing cache flush everything seems to be > > > > working > > > > as expected. > > > > > > > > Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com> > > > > Reviewed-by: Bin Meng <bmeng.cn at gmail.com> > > > > --- > > > > > > I've been trying to get this working on my own and got stuck with this > > > specific > > > issue. I'm glad you found a solution, it was driving me crazy. > > > > > > Out of curiosity how did you found the solution? > > > > > > > drivers/usb/host/xhci-mem.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > > > > index 93450ee..729bdc3 100644 > > > > --- a/drivers/usb/host/xhci-mem.c > > > > +++ b/drivers/usb/host/xhci-mem.c > > > > @@ -393,6 +393,9 @@ static int xhci_scratchpad_alloc(struct xhci_ctrl > > > > *ctrl) > > > > scratchpad->sp_array[i] = cpu_to_le64(ptr); > > > > } > > > > > > > > + xhci_flush_cache((uintptr_t)scratchpad->sp_array, > > > > + sizeof(u64) * num_sp); > > > > + > > > > > > Marek, souldn't running 'dcache off; icache off' be equivalent to this > > > (which > > > didn't do the trick for me)? or am I missing somthing? > > > > I guess something is wrong with RPi4's "dcache off" command .. > > You can't trust anything these times :) > > I'll look into it. Well it's not only that disabling the caches was needed, but also avoiding 64bit accesses, since I was missed that one, I didn't see the change in behavior. In other words, dcache/icache commands are fine. Sorry for the noise. Regards, Nicolas -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: This is a digitally signed message part URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200422/efa8d4cb/attachment.sig>
Hi Nicolas, (fixed Simon's email address, apologies for mistyping it, I will make sure it's correct in next iteration) On 22.04.2020 10:53, Nicolas Saenz Julienne wrote: > I've been trying to get this working on my own and got stuck with this specific > issue. I'm glad you found a solution, it was driving me crazy. > > Out of curiosity how did you found the solution? It took me many days of debugging...given my nearly non existent previous experience in u-boot development.In short, it started with a suggestion to map all memory for CPU as uncached. As in such a case booting was failing I checked where the xhci shared buffer allocation fall and created only a small uncached window to cover those allocations. This was first thing that started working, after fixing the 64-bit pointers setup in XHCI registers. Then I discovered "dcache" command and that was also helpful. It was sufficient to run "dcache off; usb start; dcache on". Then USB worked even after "usb reset" IIRC. But that was with my old development branch based on v2019.10-rc4 tag. Marek tried the same with newer tree and dcache_disable() was not helping, but dcache_flush_all() was. By moving dcache_disable(), dcache_enable() around I found out that it was sufficient to disable dcache before xhci_start() call and to enable it right afterwards. Then I just "bisected" the uncached memory region which narrowed it roughly to the scratchpad buffer allocations. By inspecting the code carefully again it turned there is one more cache flush call needed. >> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c >> index 93450ee..729bdc3 100644 >> --- a/drivers/usb/host/xhci-mem.c >> +++ b/drivers/usb/host/xhci-mem.c >> @@ -393,6 +393,9 @@ static int xhci_scratchpad_alloc(struct xhci_ctrl *ctrl) >> scratchpad->sp_array[i] = cpu_to_le64(ptr); >> } >> >> + xhci_flush_cache((uintptr_t)scratchpad->sp_array, >> + sizeof(u64) * num_sp); >> + > > Marek, souldn't running 'dcache off; icache off' be equivalent to this (which > didn't do the trick for me)? or am I missing somthing? Regards,
On Wed, 2020-04-22 at 14:01 +0200, Sylwester Nawrocki wrote: > Hi Nicolas, > > (fixed Simon's email address, apologies for mistyping it, I will make sure > it's correct in next iteration) > > On 22.04.2020 10:53, Nicolas Saenz Julienne wrote: > > I've been trying to get this working on my own and got stuck with this > > specific > > issue. I'm glad you found a solution, it was driving me crazy. > > > > Out of curiosity how did you found the solution? > > It took me many days of debugging...given my nearly non existent previous > experience in u-boot development.In short, it started with a suggestion to map > all memory for CPU as uncached. > As in such a case booting was failing I checked where the xhci shared buffer > allocation fall and created only a small uncached window to cover those > allocations. This was first thing that started working, after fixing the > 64-bit pointers setup in XHCI registers. > Then I discovered "dcache" command and that was also helpful. It was > sufficient > to run "dcache off; usb start; dcache on". Then USB worked even after "usb > reset" > IIRC. But that was with my old development branch based on v2019.10-rc4 tag. > Marek tried the same with newer tree and dcache_disable() was not helping, > but dcache_flush_all() was. > > By moving dcache_disable(), dcache_enable() around I found out that it was > sufficient to disable dcache before xhci_start() call and to enable it right > afterwards. > > Then I just "bisected" the uncached memory region which narrowed it roughly > to the scratchpad buffer allocations. By inspecting the code carefully again > it turned there is one more cache flush call needed. Thanks for the in-depth explanation, it's very much appreciated! Regards, Nicolas -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: This is a digitally signed message part URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200422/74d3a662/attachment.sig>
On Wed, 22 Apr 2020 at 06:33, Nicolas Saenz Julienne <nsaenzjulienne at suse.de> wrote: > > On Wed, 2020-04-22 at 14:01 +0200, Sylwester Nawrocki wrote: > > Hi Nicolas, > > > > (fixed Simon's email address, apologies for mistyping it, I will make sure > > it's correct in next iteration) > > > > On 22.04.2020 10:53, Nicolas Saenz Julienne wrote: > > > I've been trying to get this working on my own and got stuck with this > > > specific > > > issue. I'm glad you found a solution, it was driving me crazy. > > > > > > Out of curiosity how did you found the solution? > > > > It took me many days of debugging...given my nearly non existent previous > > experience in u-boot development.In short, it started with a suggestion to map > > all memory for CPU as uncached. > > As in such a case booting was failing I checked where the xhci shared buffer > > allocation fall and created only a small uncached window to cover those > > allocations. This was first thing that started working, after fixing the > > 64-bit pointers setup in XHCI registers. > > Then I discovered "dcache" command and that was also helpful. It was > > sufficient > > to run "dcache off; usb start; dcache on". Then USB worked even after "usb > > reset" > > IIRC. But that was with my old development branch based on v2019.10-rc4 tag. > > Marek tried the same with newer tree and dcache_disable() was not helping, > > but dcache_flush_all() was. > > > > By moving dcache_disable(), dcache_enable() around I found out that it was > > sufficient to disable dcache before xhci_start() call and to enable it right > > afterwards. > > > > Then I just "bisected" the uncached memory region which narrowed it roughly > > to the scratchpad buffer allocations. By inspecting the code carefully again > > it turned there is one more cache flush call needed. > > Thanks for the in-depth explanation, it's very much appreciated! Yes indeed, thank you! - Simon
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 93450ee..729bdc3 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -393,6 +393,9 @@ static int xhci_scratchpad_alloc(struct xhci_ctrl *ctrl) scratchpad->sp_array[i] = cpu_to_le64(ptr); } + xhci_flush_cache((uintptr_t)scratchpad->sp_array, + sizeof(u64) * num_sp); + return 0; fail_sp3:
In current code there is no cache flush after initializing the scratchpad buffer array with the scratchpad buffer pointers. This leads to a failure of the "slot enable" command on the rpi4 board (Broadcom STB PCIe controller + VL805 USB hub) - the very first TRB transfer on the command ring fails and there is a timeout while waiting for the command completion event. After adding the missing cache flush everything seems to be working as expected. Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com> --- drivers/usb/host/xhci-mem.c | 3 +++ 1 file changed, 3 insertions(+)