usb: xhci: Prepare xHCI driver for MIPS Octeon big-endian support
These patches fix a few issues, found while porting the xHCI to the MIPS Octeon platforms. The basic issues here are:
- Endianess issues: missing cpu_to_leXX() & leXX_to_cpu() conversions - Use physical (DMA) address for the xHCI DMA controller
These patches are the groundwork for the upcoming xHCI Octeon support that will follow soon.
Thanks, Stefan
Stefan Roese (4): usb: xhci: Add missing endian conversions (cpu_to_leXX / leXX_to_cpu) usb: xhci: xhci_mem_init: Use cpu_to_le64() and not xhci_writeq() usb: usb-uclass.c: Drop le16_to_cpu() as values are already swapped usb: xhci: Add virt_to_phys() to support mapped platforms
drivers/usb/host/usb-uclass.c | 8 ++++---- drivers/usb/host/xhci-mem.c | 30 +++++++++++++++--------------- drivers/usb/host/xhci-ring.c | 8 ++++---- drivers/usb/host/xhci.c | 3 +-- 4 files changed, 24 insertions(+), 25 deletions(-)
While trying to use the U-Boot xHCI driver on the MIPS Octeon platform, which is big endian, I noticed that the driver is missing a few endian conversion calls. This patch adds these missing endian conversion calls.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de ---
drivers/usb/host/xhci-mem.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 2d968aafb0..bd959b4093 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -110,7 +110,7 @@ static void xhci_scratchpad_free(struct xhci_ctrl *ctrl)
ctrl->dcbaa->dev_context_ptrs[0] = 0;
- free((void *)(uintptr_t)ctrl->scratchpad->sp_array[0]); + free((void *)le64_to_cpu(ctrl->scratchpad->sp_array[0])); free(ctrl->scratchpad->sp_array); free(ctrl->scratchpad); ctrl->scratchpad = NULL; @@ -225,7 +225,8 @@ static void xhci_link_segments(struct xhci_segment *prev, prev->next = next; if (link_trbs) { val_64 = (uintptr_t)next->trbs; - prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = val_64; + prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = + cpu_to_le64(val_64);
/* * Set the last TRB in the segment to @@ -486,7 +487,7 @@ int xhci_alloc_virt_device(struct xhci_ctrl *ctrl, unsigned int slot_id) byte_64 = (uintptr_t)(virt_dev->out_ctx->bytes);
/* Point to output device context in dcbaa. */ - ctrl->dcbaa->dev_context_ptrs[slot_id] = byte_64; + ctrl->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(byte_64);
xhci_flush_cache((uintptr_t)&ctrl->dcbaa->dev_context_ptrs[slot_id], sizeof(__le64)); @@ -768,7 +769,7 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
debug("route string %x\n", route); #endif - slot_ctx->dev_info |= route; + slot_ctx->dev_info |= cpu_to_le32(route);
switch (speed) { case USB_SPEED_SUPER:
Hi Bin,
On 02.07.20 10:47, Stefan Roese wrote:
While trying to use the U-Boot xHCI driver on the MIPS Octeon platform, which is big endian, I noticed that the driver is missing a few endian conversion calls. This patch adds these missing endian conversion calls.
Did you find the time to review this patchset with endianess corrections and support for physical vs virtual addresses? Its in preparation for the Octeon (MIPS big endian) xHCI support.
I would like to have this common xhci parts integrated in mainline early in this release (if there are no issues of course), so that these changes can get a broad testing on multiple platforms.
Thanks, Stefan
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 2d968aafb0..bd959b4093 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -110,7 +110,7 @@ static void xhci_scratchpad_free(struct xhci_ctrl *ctrl)
ctrl->dcbaa->dev_context_ptrs[0] = 0;
- free((void *)(uintptr_t)ctrl->scratchpad->sp_array[0]);
- free((void *)le64_to_cpu(ctrl->scratchpad->sp_array[0])); free(ctrl->scratchpad->sp_array); free(ctrl->scratchpad); ctrl->scratchpad = NULL;
@@ -225,7 +225,8 @@ static void xhci_link_segments(struct xhci_segment *prev, prev->next = next; if (link_trbs) { val_64 = (uintptr_t)next->trbs;
prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = val_64;
prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr =cpu_to_le64(val_64);/*
- Set the last TRB in the segment to
@@ -486,7 +487,7 @@ int xhci_alloc_virt_device(struct xhci_ctrl *ctrl, unsigned int slot_id) byte_64 = (uintptr_t)(virt_dev->out_ctx->bytes);
/* Point to output device context in dcbaa. */
- ctrl->dcbaa->dev_context_ptrs[slot_id] = byte_64;
ctrl->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(byte_64);
xhci_flush_cache((uintptr_t)&ctrl->dcbaa->dev_context_ptrs[slot_id], sizeof(__le64));
@@ -768,7 +769,7 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
debug("route string %x\n", route); #endif
- slot_ctx->dev_info |= route;
slot_ctx->dev_info |= cpu_to_le32(route);
switch (speed) { case USB_SPEED_SUPER:
Viele Grüße, Stefan
Hi Stefan,
On Thu, Jul 16, 2020 at 5:25 PM Stefan Roese sr@denx.de wrote:
Hi Bin,
On 02.07.20 10:47, Stefan Roese wrote:
While trying to use the U-Boot xHCI driver on the MIPS Octeon platform, which is big endian, I noticed that the driver is missing a few endian conversion calls. This patch adds these missing endian conversion calls.
Did you find the time to review this patchset with endianess corrections and support for physical vs virtual addresses? Its in preparation for the Octeon (MIPS big endian) xHCI support.
Sorry I missed this series. I will take a look.
I would like to have this common xhci parts integrated in mainline early in this release (if there are no issues of course), so that these changes can get a broad testing on multiple platforms.
Regards, Bin
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
While trying to use the U-Boot xHCI driver on the MIPS Octeon platform, which is big endian, I noticed that the driver is missing a few endian conversion calls. This patch adds these missing endian conversion calls.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Good catch! It's hard to detect these problems if we only validate xHCI on ARM/x86 which are little-endian. Apparently there is no xHCI on PPC, so MIPS becomes the first big endian platform using xHCI.
Reviewed-by: Bin Meng bmeng.cn@gmail.com
On 17.07.20 07:15, Bin Meng wrote:
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
While trying to use the U-Boot xHCI driver on the MIPS Octeon platform, which is big endian, I noticed that the driver is missing a few endian conversion calls. This patch adds these missing endian conversion calls.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Good catch! It's hard to detect these problems if we only validate xHCI on ARM/x86 which are little-endian. Apparently there is no xHCI on PPC, so MIPS becomes the first big endian platform using xHCI.
Yes, I was also astonished that the xHCI driver has only been used by little-endian platforms so far. I did expect that at least some PPC platforms supported this code. Apparently this is not the case.
Reviewed-by: Bin Meng bmeng.cn@gmail.com
Thanks, Stefan
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
While trying to use the U-Boot xHCI driver on the MIPS Octeon platform, which is big endian, I noticed that the driver is missing a few endian conversion calls. This patch adds these missing endian conversion calls.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 2d968aafb0..bd959b4093 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -110,7 +110,7 @@ static void xhci_scratchpad_free(struct xhci_ctrl *ctrl)
ctrl->dcbaa->dev_context_ptrs[0] = 0;
free((void *)(uintptr_t)ctrl->scratchpad->sp_array[0]);
free((void *)le64_to_cpu(ctrl->scratchpad->sp_array[0]));
There is a build warning for this:
drivers/usb/host/xhci-mem.c: In function 'xhci_scratchpad_free': drivers/usb/host/xhci-mem.c:113:7: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] free((void *)le64_to_cpu(ctrl->scratchpad->sp_array[0])); ^
free(ctrl->scratchpad->sp_array); free(ctrl->scratchpad); ctrl->scratchpad = NULL;@@ -225,7 +225,8 @@ static void xhci_link_segments(struct xhci_segment *prev, prev->next = next; if (link_trbs) { val_64 = (uintptr_t)next->trbs;
prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = val_64;
prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr =cpu_to_le64(val_64); /* * Set the last TRB in the segment to@@ -486,7 +487,7 @@ int xhci_alloc_virt_device(struct xhci_ctrl *ctrl, unsigned int slot_id) byte_64 = (uintptr_t)(virt_dev->out_ctx->bytes);
/* Point to output device context in dcbaa. */
ctrl->dcbaa->dev_context_ptrs[slot_id] = byte_64;
ctrl->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(byte_64); xhci_flush_cache((uintptr_t)&ctrl->dcbaa->dev_context_ptrs[slot_id], sizeof(__le64));@@ -768,7 +769,7 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
debug("route string %x\n", route);#endif
slot_ctx->dev_info |= route;
slot_ctx->dev_info |= cpu_to_le32(route); switch (speed) { case USB_SPEED_SUPER:--
Test results on Minnowmax which has one USB 2.0 port and one 3.0 port:
USB 2.0 flash drive inserted to USB 2.0 port: recognized, read/write OK USB 2.0 flash drive inserted to USB 3.0 port: recognized, read/write OK
USB 3.0 flash drive inserted to USB 2.0 port: recognized, read/write OK USB 3.0 flash drive inserted to USB 3.0 port: recognized, read/write OK
USB 2.0 flash drive connected to a USB 3.0 HUB inserted to USB 2.0 port: recognized, read/write OK USB 2.0 flash drive connected to a USB 3.0 HUB inserted to USB 3.0 port: recognized, read/write OK
USB 3.0 flash drive connected to a USB 3.0 HUB inserted to USB 2.0 port: recognized, read/write OK USB 3.0 flash drive connected to a USB 3.0 HUB inserted to USB 3.0 port: recognized, read/write OK
Tested-by: Bin Meng bmeng.cn@gmail.com
Regards, Bin
Hi Bin,
On 17.07.20 13:18, Bin Meng wrote:
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
While trying to use the U-Boot xHCI driver on the MIPS Octeon platform, which is big endian, I noticed that the driver is missing a few endian conversion calls. This patch adds these missing endian conversion calls.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 2d968aafb0..bd959b4093 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -110,7 +110,7 @@ static void xhci_scratchpad_free(struct xhci_ctrl *ctrl)
ctrl->dcbaa->dev_context_ptrs[0] = 0;
free((void *)(uintptr_t)ctrl->scratchpad->sp_array[0]);
free((void *)le64_to_cpu(ctrl->scratchpad->sp_array[0]));There is a build warning for this:
drivers/usb/host/xhci-mem.c: In function 'xhci_scratchpad_free': drivers/usb/host/xhci-mem.c:113:7: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] free((void *)le64_to_cpu(ctrl->scratchpad->sp_array[0])); ^
So we need the (uintptr_t) here as well?
free((void *)(uintptr_t)le64_to_cpu(ctrl->scratchpad->sp_array[0]));
Should I send v2 for this patch?
free(ctrl->scratchpad->sp_array); free(ctrl->scratchpad); ctrl->scratchpad = NULL;@@ -225,7 +225,8 @@ static void xhci_link_segments(struct xhci_segment *prev, prev->next = next; if (link_trbs) { val_64 = (uintptr_t)next->trbs;
prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = val_64;
prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr =cpu_to_le64(val_64); /* * Set the last TRB in the segment to@@ -486,7 +487,7 @@ int xhci_alloc_virt_device(struct xhci_ctrl *ctrl, unsigned int slot_id) byte_64 = (uintptr_t)(virt_dev->out_ctx->bytes);
/* Point to output device context in dcbaa. */
ctrl->dcbaa->dev_context_ptrs[slot_id] = byte_64;
ctrl->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(byte_64); xhci_flush_cache((uintptr_t)&ctrl->dcbaa->dev_context_ptrs[slot_id], sizeof(__le64));@@ -768,7 +769,7 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
debug("route string %x\n", route);#endif
slot_ctx->dev_info |= route;
slot_ctx->dev_info |= cpu_to_le32(route); switch (speed) { case USB_SPEED_SUPER:--
Test results on Minnowmax which has one USB 2.0 port and one 3.0 port:
USB 2.0 flash drive inserted to USB 2.0 port: recognized, read/write OK USB 2.0 flash drive inserted to USB 3.0 port: recognized, read/write OK
USB 3.0 flash drive inserted to USB 2.0 port: recognized, read/write OK USB 3.0 flash drive inserted to USB 3.0 port: recognized, read/write OK
USB 2.0 flash drive connected to a USB 3.0 HUB inserted to USB 2.0 port: recognized, read/write OK USB 2.0 flash drive connected to a USB 3.0 HUB inserted to USB 3.0 port: recognized, read/write OK
USB 3.0 flash drive connected to a USB 3.0 HUB inserted to USB 2.0 port: recognized, read/write OK USB 3.0 flash drive connected to a USB 3.0 HUB inserted to USB 3.0 port: recognized, read/write OK
Tested-by: Bin Meng bmeng.cn@gmail.com
Very good. :)
Thanks, Stefan
Hi Stefan,
On Fri, Jul 17, 2020 at 7:34 PM Stefan Roese sr@denx.de wrote:
Hi Bin,
On 17.07.20 13:18, Bin Meng wrote:
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
While trying to use the U-Boot xHCI driver on the MIPS Octeon platform, which is big endian, I noticed that the driver is missing a few endian conversion calls. This patch adds these missing endian conversion calls.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 2d968aafb0..bd959b4093 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -110,7 +110,7 @@ static void xhci_scratchpad_free(struct xhci_ctrl *ctrl)
ctrl->dcbaa->dev_context_ptrs[0] = 0;
free((void *)(uintptr_t)ctrl->scratchpad->sp_array[0]);
free((void *)le64_to_cpu(ctrl->scratchpad->sp_array[0]));There is a build warning for this:
drivers/usb/host/xhci-mem.c: In function 'xhci_scratchpad_free': drivers/usb/host/xhci-mem.c:113:7: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast] free((void *)le64_to_cpu(ctrl->scratchpad->sp_array[0])); ^
So we need the (uintptr_t) here as well?
free((void *)(uintptr_t)le64_to_cpu(ctrl->scratchpad->sp_array[0]));
I think so.
Should I send v2 for this patch?
Yes, please.
free(ctrl->scratchpad->sp_array); free(ctrl->scratchpad); ctrl->scratchpad = NULL;@@ -225,7 +225,8 @@ static void xhci_link_segments(struct xhci_segment *prev, prev->next = next; if (link_trbs) { val_64 = (uintptr_t)next->trbs;
prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = val_64;
prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr =cpu_to_le64(val_64); /* * Set the last TRB in the segment to@@ -486,7 +487,7 @@ int xhci_alloc_virt_device(struct xhci_ctrl *ctrl, unsigned int slot_id) byte_64 = (uintptr_t)(virt_dev->out_ctx->bytes);
/* Point to output device context in dcbaa. */
ctrl->dcbaa->dev_context_ptrs[slot_id] = byte_64;
ctrl->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(byte_64); xhci_flush_cache((uintptr_t)&ctrl->dcbaa->dev_context_ptrs[slot_id], sizeof(__le64));@@ -768,7 +769,7 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
debug("route string %x\n", route);#endif
slot_ctx->dev_info |= route;
slot_ctx->dev_info |= cpu_to_le32(route); switch (speed) { case USB_SPEED_SUPER:--
Test results on Minnowmax which has one USB 2.0 port and one 3.0 port:
USB 2.0 flash drive inserted to USB 2.0 port: recognized, read/write OK USB 2.0 flash drive inserted to USB 3.0 port: recognized, read/write OK
USB 3.0 flash drive inserted to USB 2.0 port: recognized, read/write OK USB 3.0 flash drive inserted to USB 3.0 port: recognized, read/write OK
USB 2.0 flash drive connected to a USB 3.0 HUB inserted to USB 2.0 port: recognized, read/write OK USB 2.0 flash drive connected to a USB 3.0 HUB inserted to USB 3.0 port: recognized, read/write OK
USB 3.0 flash drive connected to a USB 3.0 HUB inserted to USB 2.0 port: recognized, read/write OK USB 3.0 flash drive connected to a USB 3.0 HUB inserted to USB 3.0 port: recognized, read/write OK
Tested-by: Bin Meng bmeng.cn@gmail.com
Very good. :)
Regards, Bin
While trying to use the U-Boot xHCI driver on the MIPS Octeon platform, which is big endian, I noticed that the driver is missing a few endian conversion calls. This patch adds these missing endian conversion calls.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Bin Meng bmeng.cn@gmail.com Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de --- v2: - Add missing (uintptr_t) cast to remove compile time warning
drivers/usb/host/xhci-mem.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 2d968aafb0..7d55944765 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -110,7 +110,7 @@ static void xhci_scratchpad_free(struct xhci_ctrl *ctrl)
ctrl->dcbaa->dev_context_ptrs[0] = 0;
- free((void *)(uintptr_t)ctrl->scratchpad->sp_array[0]); + free((void *)(uintptr_t)le64_to_cpu(ctrl->scratchpad->sp_array[0])); free(ctrl->scratchpad->sp_array); free(ctrl->scratchpad); ctrl->scratchpad = NULL; @@ -225,7 +225,8 @@ static void xhci_link_segments(struct xhci_segment *prev, prev->next = next; if (link_trbs) { val_64 = (uintptr_t)next->trbs; - prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = val_64; + prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = + cpu_to_le64(val_64);
/* * Set the last TRB in the segment to @@ -486,7 +487,7 @@ int xhci_alloc_virt_device(struct xhci_ctrl *ctrl, unsigned int slot_id) byte_64 = (uintptr_t)(virt_dev->out_ctx->bytes);
/* Point to output device context in dcbaa. */ - ctrl->dcbaa->dev_context_ptrs[slot_id] = byte_64; + ctrl->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(byte_64);
xhci_flush_cache((uintptr_t)&ctrl->dcbaa->dev_context_ptrs[slot_id], sizeof(__le64)); @@ -768,7 +769,7 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl,
debug("route string %x\n", route); #endif - slot_ctx->dev_info |= route; + slot_ctx->dev_info |= cpu_to_le32(route);
switch (speed) { case USB_SPEED_SUPER:
Hi Stefan,
On Fri, Jul 17, 2020 at 10:04 PM Stefan Roese sr@denx.de wrote:
While trying to use the U-Boot xHCI driver on the MIPS Octeon platform, which is big endian, I noticed that the driver is missing a few endian conversion calls. This patch adds these missing endian conversion calls.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Bin Meng bmeng.cn@gmail.com Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
v2:
- Add missing (uintptr_t) cast to remove compile time warning
drivers/usb/host/xhci-mem.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Tested-by: Bin Meng bmeng.cn@gmail.com
Only this patch is sent as v2?
Regards, Bin
Hi Bin,
On 17.07.20 16:14, Bin Meng wrote:
Hi Stefan,
On Fri, Jul 17, 2020 at 10:04 PM Stefan Roese sr@denx.de wrote:
While trying to use the U-Boot xHCI driver on the MIPS Octeon platform, which is big endian, I noticed that the driver is missing a few endian conversion calls. This patch adds these missing endian conversion calls.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Bin Meng bmeng.cn@gmail.com Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
v2:
Add missing (uintptr_t) cast to remove compile time warning
drivers/usb/host/xhci-mem.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Tested-by: Bin Meng bmeng.cn@gmail.com
Only this patch is sent as v2?
Yes. I didn't want to pollute the list with unneeded patches. If you prefer a complete patchset in the new version, then I can of course do as as well.
Thanks, Stefan
Hi Stefan,
On Sat, Jul 18, 2020 at 2:22 AM Stefan Roese sr@denx.de wrote:
Hi Bin,
On 17.07.20 16:14, Bin Meng wrote:
Hi Stefan,
On Fri, Jul 17, 2020 at 10:04 PM Stefan Roese sr@denx.de wrote:
While trying to use the U-Boot xHCI driver on the MIPS Octeon platform, which is big endian, I noticed that the driver is missing a few endian conversion calls. This patch adds these missing endian conversion calls.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Bin Meng bmeng.cn@gmail.com Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
v2:
Add missing (uintptr_t) cast to remove compile time warning
drivers/usb/host/xhci-mem.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Tested-by: Bin Meng bmeng.cn@gmail.com
Only this patch is sent as v2?
Yes. I didn't want to pollute the list with unneeded patches. If you prefer a complete patchset in the new version, then I can of course do as as well.
IMHO it would be good for maintainers to apply this whole series from patchwork in a batch if we send the complete series.
Regards, Bin
Hi Bin,
On 20.07.20 03:48, Bin Meng wrote:
Hi Stefan,
On Sat, Jul 18, 2020 at 2:22 AM Stefan Roese sr@denx.de wrote:
Hi Bin,
On 17.07.20 16:14, Bin Meng wrote:
Hi Stefan,
On Fri, Jul 17, 2020 at 10:04 PM Stefan Roese sr@denx.de wrote:
While trying to use the U-Boot xHCI driver on the MIPS Octeon platform, which is big endian, I noticed that the driver is missing a few endian conversion calls. This patch adds these missing endian conversion calls.
Signed-off-by: Stefan Roese sr@denx.de Reviewed-by: Bin Meng bmeng.cn@gmail.com Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
v2:
Add missing (uintptr_t) cast to remove compile time warning
drivers/usb/host/xhci-mem.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
Tested-by: Bin Meng bmeng.cn@gmail.com
Only this patch is sent as v2?
Yes. I didn't want to pollute the list with unneeded patches. If you prefer a complete patchset in the new version, then I can of course do as as well.
IMHO it would be good for maintainers to apply this whole series from patchwork in a batch if we send the complete series.
No problem. v2 series will be out shortly.
Thanks, Stefan
xhci_writeq() makes the CPU->LE swapping only when addressing registers in the xHCI controller address range and not in the local memory (RAM). We need to use cpu_to_le64() here to ensure that the conversion is done correctly.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de ---
drivers/usb/host/xhci-mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index bd959b4093..3b805ecb9e 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -562,7 +562,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, trb_64 = 0; trb_64 = (uintptr_t)seg->trbs; struct xhci_erst_entry *entry = &ctrl->erst.entries[val]; - xhci_writeq(&entry->seg_addr, trb_64); + entry->seg_addr = cpu_to_le64(trb_64); entry->seg_size = cpu_to_le32(TRBS_PER_SEGMENT); entry->rsvd = 0; seg = seg->next;
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
xhci_writeq() makes the CPU->LE swapping only when addressing registers in the xHCI controller address range and not in the local memory (RAM).
Is the above behavior exposed by the MIPS platform's writel()?
We need to use cpu_to_le64() here to ensure that the conversion is done correctly.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index bd959b4093..3b805ecb9e 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -562,7 +562,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, trb_64 = 0; trb_64 = (uintptr_t)seg->trbs; struct xhci_erst_entry *entry = &ctrl->erst.entries[val];
xhci_writeq(&entry->seg_addr, trb_64);
entry->seg_addr = cpu_to_le64(trb_64); entry->seg_size = cpu_to_le32(TRBS_PER_SEGMENT); entry->rsvd = 0; seg = seg->next;
Reviewed-by: Bin Meng bmeng.cn@gmail.com
Regards, Bin
On 17.07.20 07:24, Bin Meng wrote:
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
xhci_writeq() makes the CPU->LE swapping only when addressing registers in the xHCI controller address range and not in the local memory (RAM).
Is the above behavior exposed by the MIPS platform's writel()?
Not sure what you mean with this. Without this patch, xhci_writeq() will not swap on Octeon MIPS, as the destination address is located in local memory (DDR). Using the xhci_read/write accessor functions should be restricted to accessing the controller registers.
BTW: The Octeon MIPS writel will swap to little-endian, when the location is in the xHCI controller address space (and PCI etc). This support for selective swapping is not pushed into mainline yet. I will send it in some follow up patches.
Thanks, Stefan
We need to use cpu_to_le64() here to ensure that the conversion is done correctly.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index bd959b4093..3b805ecb9e 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -562,7 +562,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, trb_64 = 0; trb_64 = (uintptr_t)seg->trbs; struct xhci_erst_entry *entry = &ctrl->erst.entries[val];
xhci_writeq(&entry->seg_addr, trb_64);
entry->seg_addr = cpu_to_le64(trb_64); entry->seg_size = cpu_to_le32(TRBS_PER_SEGMENT); entry->rsvd = 0; seg = seg->next;Reviewed-by: Bin Meng bmeng.cn@gmail.com
Regards, Bin
Thanks, Stefan
Hi Stefan,
On Fri, Jul 17, 2020 at 6:04 PM Stefan Roese sr@denx.de wrote:
On 17.07.20 07:24, Bin Meng wrote:
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
xhci_writeq() makes the CPU->LE swapping only when addressing registers in the xHCI controller address range and not in the local memory (RAM).
Is the above behavior exposed by the MIPS platform's writel()?
Not sure what you mean with this. Without this patch, xhci_writeq() will not swap on Octeon MIPS, as the destination address is located in local memory (DDR).
I wonder why xhci_writeq() does not swap? Is this due to the writel() implementation on Octeon MIPS?
Using the xhci_read/write accessor functions should be restricted to accessing the controller registers.
Yes, this is the supposed usage that xhci_read/write should be called to operate on xHCI registers. However my question was why xhci_read/write does not swap even it is called on memory space, hence the writel() question.
BTW: The Octeon MIPS writel will swap to little-endian, when the location is in the xHCI controller address space (and PCI etc). This support for selective swapping is not pushed into mainline yet. I will send it in some follow up patches.
Regards, Bin
Hi Bin,
On 17.07.20 12:09, Bin Meng wrote:
Hi Stefan,
On Fri, Jul 17, 2020 at 6:04 PM Stefan Roese sr@denx.de wrote:
On 17.07.20 07:24, Bin Meng wrote:
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
xhci_writeq() makes the CPU->LE swapping only when addressing registers in the xHCI controller address range and not in the local memory (RAM).
Is the above behavior exposed by the MIPS platform's writel()?
Not sure what you mean with this. Without this patch, xhci_writeq() will not swap on Octeon MIPS, as the destination address is located in local memory (DDR).
I wonder why xhci_writeq() does not swap? Is this due to the writel() implementation on Octeon MIPS?
Ah, okay. Please see my comment below for this. Here again:
BTW: The Octeon MIPS writel will swap to little-endian, when the location is in the xHCI controller address space (and PCI etc). This support for selective swapping is not pushed into mainline yet. I will send it in some follow up patches.
So to answer your question: writel will not swap when addressing local memory.
Using the xhci_read/write accessor functions should be restricted to accessing the controller registers.
Yes, this is the supposed usage that xhci_read/write should be called to operate on xHCI registers. However my question was why xhci_read/write does not swap even it is called on memory space, hence the writel() question.
BTW: The Octeon MIPS writel will swap to little-endian, when the location is in the xHCI controller address space (and PCI etc). This support for selective swapping is not pushed into mainline yet. I will send it in some follow up patches.
Regards, Bin
Thanks, Stefan
Hi Stefan,
On Fri, Jul 17, 2020 at 6:11 PM Stefan Roese sr@denx.de wrote:
Hi Bin,
On 17.07.20 12:09, Bin Meng wrote:
Hi Stefan,
On Fri, Jul 17, 2020 at 6:04 PM Stefan Roese sr@denx.de wrote:
On 17.07.20 07:24, Bin Meng wrote:
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
xhci_writeq() makes the CPU->LE swapping only when addressing registers in the xHCI controller address range and not in the local memory (RAM).
Is the above behavior exposed by the MIPS platform's writel()?
Not sure what you mean with this. Without this patch, xhci_writeq() will not swap on Octeon MIPS, as the destination address is located in local memory (DDR).
I wonder why xhci_writeq() does not swap? Is this due to the writel() implementation on Octeon MIPS?
Ah, okay. Please see my comment below for this. Here again:
BTW: The Octeon MIPS writel will swap to little-endian, when the location is in the xHCI controller address space (and PCI etc). This support for selective swapping is not pushed into mainline yet. I will send it in some follow up patches.
So to answer your question: writel will not swap when addressing local memory.
Thanks. That explains.
Using the xhci_read/write accessor functions should be restricted to accessing the controller registers.
Yes, this is the supposed usage that xhci_read/write should be called to operate on xHCI registers. However my question was why xhci_read/write does not swap even it is called on memory space, hence the writel() question.
Regards, Bin
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
xhci_writeq() makes the CPU->LE swapping only when addressing registers in the xHCI controller address range and not in the local memory (RAM). We need to use cpu_to_le64() here to ensure that the conversion is done correctly.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Tested-by: Bin Meng bmeng.cn@gmail.com
These values are already swapped to CPU endianess, so swapping them again is a bug. Let's remove the swap here instead.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de ---
drivers/usb/host/usb-uclass.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index cb79dfbbd5..aa08d4ffc6 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -416,21 +416,21 @@ static int usb_match_device(const struct usb_device_descriptor *desc, const struct usb_device_id *id) { if ((id->match_flags & USB_DEVICE_ID_MATCH_VENDOR) && - id->idVendor != le16_to_cpu(desc->idVendor)) + id->idVendor != desc->idVendor) return 0;
if ((id->match_flags & USB_DEVICE_ID_MATCH_PRODUCT) && - id->idProduct != le16_to_cpu(desc->idProduct)) + id->idProduct != desc->idProduct) return 0;
/* No need to test id->bcdDevice_lo != 0, since 0 is never greater than any unsigned number. */ if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_LO) && - (id->bcdDevice_lo > le16_to_cpu(desc->bcdDevice))) + (id->bcdDevice_lo > desc->bcdDevice)) return 0;
if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_HI) && - (id->bcdDevice_hi < le16_to_cpu(desc->bcdDevice))) + (id->bcdDevice_hi < desc->bcdDevice)) return 0;
if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_CLASS) &&
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
These values are already swapped to CPU endianess, so swapping them
Can you please add more details as to when these values are swapped? I assume this is inside usb_select_config() which is called before this function is called?
But I wonder how this code ever worked on ARM/x86?
again is a bug. Let's remove the swap here instead.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/usb-uclass.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index cb79dfbbd5..aa08d4ffc6 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -416,21 +416,21 @@ static int usb_match_device(const struct usb_device_descriptor *desc, const struct usb_device_id *id) { if ((id->match_flags & USB_DEVICE_ID_MATCH_VENDOR) &&
id->idVendor != le16_to_cpu(desc->idVendor))
id->idVendor != desc->idVendor) return 0; if ((id->match_flags & USB_DEVICE_ID_MATCH_PRODUCT) &&
id->idProduct != le16_to_cpu(desc->idProduct))
id->idProduct != desc->idProduct) return 0; /* No need to test id->bcdDevice_lo != 0, since 0 is never greater than any unsigned number. */ if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_LO) &&
(id->bcdDevice_lo > le16_to_cpu(desc->bcdDevice)))
(id->bcdDevice_lo > desc->bcdDevice)) return 0; if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_HI) &&
(id->bcdDevice_hi < le16_to_cpu(desc->bcdDevice)))
(id->bcdDevice_hi < desc->bcdDevice)) return 0; if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_CLASS) &&
Reviewed-by: Bin Meng bmeng.cn@gmail.com
Regards, Bin
On 17.07.20 07:33, Bin Meng wrote:
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
These values are already swapped to CPU endianess, so swapping them
Can you please add more details as to when these values are swapped? I assume this is inside usb_select_config() which is called before this function is called?
Yes. Its swapped exactly here:
int usb_select_config(struct usb_device *dev) { unsigned char *tmpbuf = NULL; int err;
err = get_descriptor_len(dev, USB_DT_DEVICE_SIZE, USB_DT_DEVICE_SIZE); if (err) return err;
/* correct le values */ le16_to_cpus(&dev->descriptor.bcdUSB); le16_to_cpus(&dev->descriptor.idVendor); le16_to_cpus(&dev->descriptor.idProduct); le16_to_cpus(&dev->descriptor.bcdDevice);
But I wonder how this code ever worked on ARM/x86?
On ARM/x86, the little-endian swapping is a no-op. So it doesn't matter if you swap once or twice or... ;)
again is a bug. Let's remove the swap here instead.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/usb-uclass.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index cb79dfbbd5..aa08d4ffc6 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -416,21 +416,21 @@ static int usb_match_device(const struct usb_device_descriptor *desc, const struct usb_device_id *id) { if ((id->match_flags & USB_DEVICE_ID_MATCH_VENDOR) &&
id->idVendor != le16_to_cpu(desc->idVendor))
id->idVendor != desc->idVendor) return 0; if ((id->match_flags & USB_DEVICE_ID_MATCH_PRODUCT) &&
id->idProduct != le16_to_cpu(desc->idProduct))
id->idProduct != desc->idProduct) return 0; /* No need to test id->bcdDevice_lo != 0, since 0 is never greater than any unsigned number. */ if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_LO) &&
(id->bcdDevice_lo > le16_to_cpu(desc->bcdDevice)))
(id->bcdDevice_lo > desc->bcdDevice)) return 0; if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_HI) &&
(id->bcdDevice_hi < le16_to_cpu(desc->bcdDevice)))
(id->bcdDevice_hi < desc->bcdDevice)) return 0; if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_CLASS) &&Reviewed-by: Bin Meng bmeng.cn@gmail.com
Regards, Bin
Thanks, Stefan
Hi Stefan,
On Fri, Jul 17, 2020 at 6:08 PM Stefan Roese sr@denx.de wrote:
On 17.07.20 07:33, Bin Meng wrote:
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
These values are already swapped to CPU endianess, so swapping them
Can you please add more details as to when these values are swapped? I assume this is inside usb_select_config() which is called before this function is called?
Yes. Its swapped exactly here:
int usb_select_config(struct usb_device *dev) { unsigned char *tmpbuf = NULL; int err;
err = get_descriptor_len(dev, USB_DT_DEVICE_SIZE, USB_DT_DEVICE_SIZE); if (err) return err; /* correct le values */ le16_to_cpus(&dev->descriptor.bcdUSB); le16_to_cpus(&dev->descriptor.idVendor); le16_to_cpus(&dev->descriptor.idProduct); le16_to_cpus(&dev->descriptor.bcdDevice);But I wonder how this code ever worked on ARM/x86?
On ARM/x86, the little-endian swapping is a no-op. So it doesn't matter if you swap once or twice or... ;)
Ah, yes! I misread this as an unconditional swap ...
again is a bug. Let's remove the swap here instead.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/usb-uclass.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Regards, Bin
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
These values are already swapped to CPU endianess, so swapping them again is a bug. Let's remove the swap here instead.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/usb-uclass.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Tested-by: Bin Meng bmeng.cn@gmail.com
Some platforms, like MIPS Octeon, use mapped addresses (virtual address != physical address). On these platforms we need to make sure, that the local virtual addresses are converted to physical (DMA) addresses for the xHCI controller. This patch adds the missing virt_to_phys() calls, so that the correct addresses are used.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
---
drivers/usb/host/xhci-mem.c | 19 +++++++++---------- drivers/usb/host/xhci-ring.c | 8 ++++---- drivers/usb/host/xhci.c | 3 +-- 3 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 3b805ecb9e..874caf4761 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -224,7 +224,7 @@ static void xhci_link_segments(struct xhci_segment *prev, return; prev->next = next; if (link_trbs) { - val_64 = (uintptr_t)next->trbs; + val_64 = virt_to_phys(next->trbs); prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = cpu_to_le64(val_64);
@@ -484,7 +484,7 @@ int xhci_alloc_virt_device(struct xhci_ctrl *ctrl, unsigned int slot_id) /* Allocate endpoint 0 ring */ virt_dev->eps[0].ring = xhci_ring_alloc(1, true);
- byte_64 = (uintptr_t)(virt_dev->out_ctx->bytes); + byte_64 = virt_to_phys(virt_dev->out_ctx->bytes);
/* Point to output device context in dcbaa. */ ctrl->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(byte_64); @@ -509,7 +509,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, uint64_t val_64; uint64_t trb_64; uint32_t val; - unsigned long deq; + uint64_t deq; int i; struct xhci_segment *seg;
@@ -521,7 +521,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, return -ENOMEM; }
- val_64 = (uintptr_t)ctrl->dcbaa; + val_64 = virt_to_phys(ctrl->dcbaa); /* Set the pointer in DCBAA register */ xhci_writeq(&hcor->or_dcbaap, val_64);
@@ -529,7 +529,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, ctrl->cmd_ring = xhci_ring_alloc(1, true);
/* Set the address in the Command Ring Control register */ - trb_64 = (uintptr_t)ctrl->cmd_ring->first_seg->trbs; + trb_64 = virt_to_phys(ctrl->cmd_ring->first_seg->trbs); val_64 = xhci_readq(&hcor->or_crcr); val_64 = (val_64 & (u64) CMD_RING_RSVD_BITS) | (trb_64 & (u64) ~CMD_RING_RSVD_BITS) | @@ -559,8 +559,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, for (val = 0, seg = ctrl->event_ring->first_seg; val < ERST_NUM_SEGS; val++) { - trb_64 = 0; - trb_64 = (uintptr_t)seg->trbs; + trb_64 = virt_to_phys(seg->trbs); struct xhci_erst_entry *entry = &ctrl->erst.entries[val]; entry->seg_addr = cpu_to_le64(trb_64); entry->seg_size = cpu_to_le32(TRBS_PER_SEGMENT); @@ -570,7 +569,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, xhci_flush_cache((uintptr_t)ctrl->erst.entries, ERST_NUM_SEGS * sizeof(struct xhci_erst_entry));
- deq = (unsigned long)ctrl->event_ring->dequeue; + deq = virt_to_phys(ctrl->event_ring->dequeue);
/* Update HC event ring dequeue pointer */ xhci_writeq(&ctrl->ir_set->erst_dequeue, @@ -585,7 +584,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, /* this is the event ring segment table pointer */ val_64 = xhci_readq(&ctrl->ir_set->erst_base); val_64 &= ERST_PTR_MASK; - val_64 |= ((uintptr_t)(ctrl->erst.entries) & ~ERST_PTR_MASK); + val_64 |= virt_to_phys(ctrl->erst.entries) & ~ERST_PTR_MASK;
xhci_writeq(&ctrl->ir_set->erst_base, val_64);
@@ -853,7 +852,7 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl, cpu_to_le32(((0 & MAX_BURST_MASK) << MAX_BURST_SHIFT) | ((3 & ERROR_COUNT_MASK) << ERROR_COUNT_SHIFT));
- trb_64 = (uintptr_t)virt_dev->eps[0].ring->first_seg->trbs; + trb_64 = virt_to_phys(virt_dev->eps[0].ring->first_seg->trbs); ep0_ctx->deq = cpu_to_le64(trb_64 | virt_dev->eps[0].ring->cycle_state);
/* diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 86aeaab412..092ed6eaf1 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -275,7 +275,7 @@ void xhci_queue_command(struct xhci_ctrl *ctrl, u8 *ptr, u32 slot_id, u32 ep_index, trb_type cmd) { u32 fields[4]; - u64 val_64 = (uintptr_t)ptr; + u64 val_64 = virt_to_phys(ptr);
BUG_ON(prepare_ring(ctrl, ctrl->cmd_ring, EP_STATE_RUNNING));
@@ -399,7 +399,7 @@ void xhci_acknowledge_event(struct xhci_ctrl *ctrl)
/* Inform the hardware */ xhci_writeq(&ctrl->ir_set->erst_dequeue, - (uintptr_t)ctrl->event_ring->dequeue | ERST_EHB); + virt_to_phys(ctrl->event_ring->dequeue) | ERST_EHB); }
/** @@ -577,7 +577,7 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, u64 addr; int ret; u32 trb_fields[4]; - u64 val_64 = (uintptr_t)buffer; + u64 val_64 = virt_to_phys(buffer);
debug("dev=%p, pipe=%lx, buffer=%p, length=%d\n", udev, pipe, buffer, length); @@ -876,7 +876,7 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe, if (length > 0) { if (req->requesttype & USB_DIR_IN) field |= TRB_DIR_IN; - buf_64 = (uintptr_t)buffer; + buf_64 = virt_to_phys(buffer);
trb_fields[0] = lower_32_bits(buf_64); trb_fields[1] = upper_32_bits(buf_64); diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index ebd2954571..f89e274b0d 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -600,8 +600,7 @@ static int xhci_set_configuration(struct usb_device *udev) cpu_to_le32(MAX_BURST(max_burst) | ERROR_COUNT(err_count));
- trb_64 = (uintptr_t) - virt_dev->eps[ep_index].ring->enqueue; + trb_64 = virt_to_phys(virt_dev->eps[ep_index].ring->enqueue); ep_ctx[ep_index]->deq = cpu_to_le64(trb_64 | virt_dev->eps[ep_index].ring->cycle_state);
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
Some platforms, like MIPS Octeon, use mapped addresses (virtual address != physical address). On these platforms we need to make sure, that the local virtual addresses are converted to physical (DMA) addresses for the xHCI controller. This patch adds the missing virt_to_phys() calls, so that the correct addresses are used.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 19 +++++++++---------- drivers/usb/host/xhci-ring.c | 8 ++++---- drivers/usb/host/xhci.c | 3 +-- 3 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 3b805ecb9e..874caf4761 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -224,7 +224,7 @@ static void xhci_link_segments(struct xhci_segment *prev, return; prev->next = next; if (link_trbs) {
val_64 = (uintptr_t)next->trbs;
val_64 = virt_to_phys(next->trbs); prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = cpu_to_le64(val_64);@@ -484,7 +484,7 @@ int xhci_alloc_virt_device(struct xhci_ctrl *ctrl, unsigned int slot_id) /* Allocate endpoint 0 ring */ virt_dev->eps[0].ring = xhci_ring_alloc(1, true);
byte_64 = (uintptr_t)(virt_dev->out_ctx->bytes);
byte_64 = virt_to_phys(virt_dev->out_ctx->bytes); /* Point to output device context in dcbaa. */ ctrl->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(byte_64);@@ -509,7 +509,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, uint64_t val_64; uint64_t trb_64; uint32_t val;
unsigned long deq;
uint64_t deq;
This change seems unnecessary?
int i; struct xhci_segment *seg;@@ -521,7 +521,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, return -ENOMEM; }
val_64 = (uintptr_t)ctrl->dcbaa;
val_64 = virt_to_phys(ctrl->dcbaa); /* Set the pointer in DCBAA register */ xhci_writeq(&hcor->or_dcbaap, val_64);@@ -529,7 +529,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, ctrl->cmd_ring = xhci_ring_alloc(1, true);
/* Set the address in the Command Ring Control register */
trb_64 = (uintptr_t)ctrl->cmd_ring->first_seg->trbs;
trb_64 = virt_to_phys(ctrl->cmd_ring->first_seg->trbs); val_64 = xhci_readq(&hcor->or_crcr); val_64 = (val_64 & (u64) CMD_RING_RSVD_BITS) | (trb_64 & (u64) ~CMD_RING_RSVD_BITS) |@@ -559,8 +559,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, for (val = 0, seg = ctrl->event_ring->first_seg; val < ERST_NUM_SEGS; val++) {
trb_64 = 0;trb_64 = (uintptr_t)seg->trbs;
trb_64 = virt_to_phys(seg->trbs); struct xhci_erst_entry *entry = &ctrl->erst.entries[val]; entry->seg_addr = cpu_to_le64(trb_64); entry->seg_size = cpu_to_le32(TRBS_PER_SEGMENT);@@ -570,7 +569,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, xhci_flush_cache((uintptr_t)ctrl->erst.entries, ERST_NUM_SEGS * sizeof(struct xhci_erst_entry));
deq = (unsigned long)ctrl->event_ring->dequeue;
deq = virt_to_phys(ctrl->event_ring->dequeue); /* Update HC event ring dequeue pointer */ xhci_writeq(&ctrl->ir_set->erst_dequeue,@@ -585,7 +584,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, /* this is the event ring segment table pointer */ val_64 = xhci_readq(&ctrl->ir_set->erst_base); val_64 &= ERST_PTR_MASK;
val_64 |= ((uintptr_t)(ctrl->erst.entries) & ~ERST_PTR_MASK);
val_64 |= virt_to_phys(ctrl->erst.entries) & ~ERST_PTR_MASK; xhci_writeq(&ctrl->ir_set->erst_base, val_64);@@ -853,7 +852,7 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl, cpu_to_le32(((0 & MAX_BURST_MASK) << MAX_BURST_SHIFT) | ((3 & ERROR_COUNT_MASK) << ERROR_COUNT_SHIFT));
trb_64 = (uintptr_t)virt_dev->eps[0].ring->first_seg->trbs;
trb_64 = virt_to_phys(virt_dev->eps[0].ring->first_seg->trbs); ep0_ctx->deq = cpu_to_le64(trb_64 | virt_dev->eps[0].ring->cycle_state); /*diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 86aeaab412..092ed6eaf1 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -275,7 +275,7 @@ void xhci_queue_command(struct xhci_ctrl *ctrl, u8 *ptr, u32 slot_id, u32 ep_index, trb_type cmd) { u32 fields[4];
u64 val_64 = (uintptr_t)ptr;
u64 val_64 = virt_to_phys(ptr); BUG_ON(prepare_ring(ctrl, ctrl->cmd_ring, EP_STATE_RUNNING));@@ -399,7 +399,7 @@ void xhci_acknowledge_event(struct xhci_ctrl *ctrl)
/* Inform the hardware */ xhci_writeq(&ctrl->ir_set->erst_dequeue,
(uintptr_t)ctrl->event_ring->dequeue | ERST_EHB);
virt_to_phys(ctrl->event_ring->dequeue) | ERST_EHB);}
/** @@ -577,7 +577,7 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, u64 addr; int ret; u32 trb_fields[4];
u64 val_64 = (uintptr_t)buffer;
u64 val_64 = virt_to_phys(buffer); debug("dev=%p, pipe=%lx, buffer=%p, length=%d\n", udev, pipe, buffer, length);@@ -876,7 +876,7 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe, if (length > 0) { if (req->requesttype & USB_DIR_IN) field |= TRB_DIR_IN;
buf_64 = (uintptr_t)buffer;
buf_64 = virt_to_phys(buffer); trb_fields[0] = lower_32_bits(buf_64); trb_fields[1] = upper_32_bits(buf_64);diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index ebd2954571..f89e274b0d 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -600,8 +600,7 @@ static int xhci_set_configuration(struct usb_device *udev) cpu_to_le32(MAX_BURST(max_burst) | ERROR_COUNT(err_count));
trb_64 = (uintptr_t)virt_dev->eps[ep_index].ring->enqueue;
trb_64 = virt_to_phys(virt_dev->eps[ep_index].ring->enqueue); ep_ctx[ep_index]->deq = cpu_to_le64(trb_64 | virt_dev->eps[ep_index].ring->cycle_state);--
Other than above, Reviewed-by: Bin Meng bmeng.cn@gmail.com
Regards, Bin
Hi Bin,
On 17.07.20 07:57, Bin Meng wrote:
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
Some platforms, like MIPS Octeon, use mapped addresses (virtual address != physical address). On these platforms we need to make sure, that the local virtual addresses are converted to physical (DMA) addresses for the xHCI controller. This patch adds the missing virt_to_phys() calls, so that the correct addresses are used.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 19 +++++++++---------- drivers/usb/host/xhci-ring.c | 8 ++++---- drivers/usb/host/xhci.c | 3 +-- 3 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 3b805ecb9e..874caf4761 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -224,7 +224,7 @@ static void xhci_link_segments(struct xhci_segment *prev, return; prev->next = next; if (link_trbs) {
val_64 = (uintptr_t)next->trbs;
val_64 = virt_to_phys(next->trbs); prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = cpu_to_le64(val_64);@@ -484,7 +484,7 @@ int xhci_alloc_virt_device(struct xhci_ctrl *ctrl, unsigned int slot_id) /* Allocate endpoint 0 ring */ virt_dev->eps[0].ring = xhci_ring_alloc(1, true);
byte_64 = (uintptr_t)(virt_dev->out_ctx->bytes);
byte_64 = virt_to_phys(virt_dev->out_ctx->bytes); /* Point to output device context in dcbaa. */ ctrl->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(byte_64);@@ -509,7 +509,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, uint64_t val_64; uint64_t trb_64; uint32_t val;
unsigned long deq;
uint64_t deq;This change seems unnecessary?
In most (all other?) places, the variable containing these 64 bit addresses has the type uint64_t in this driver. So I wanted to make the code more consistant here. Or do you see a problem with this change?
int i; struct xhci_segment *seg;@@ -521,7 +521,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, return -ENOMEM; }
val_64 = (uintptr_t)ctrl->dcbaa;
val_64 = virt_to_phys(ctrl->dcbaa); /* Set the pointer in DCBAA register */ xhci_writeq(&hcor->or_dcbaap, val_64);@@ -529,7 +529,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, ctrl->cmd_ring = xhci_ring_alloc(1, true);
/* Set the address in the Command Ring Control register */
trb_64 = (uintptr_t)ctrl->cmd_ring->first_seg->trbs;
trb_64 = virt_to_phys(ctrl->cmd_ring->first_seg->trbs); val_64 = xhci_readq(&hcor->or_crcr); val_64 = (val_64 & (u64) CMD_RING_RSVD_BITS) | (trb_64 & (u64) ~CMD_RING_RSVD_BITS) |@@ -559,8 +559,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, for (val = 0, seg = ctrl->event_ring->first_seg; val < ERST_NUM_SEGS; val++) {
trb_64 = 0;trb_64 = (uintptr_t)seg->trbs;
trb_64 = virt_to_phys(seg->trbs); struct xhci_erst_entry *entry = &ctrl->erst.entries[val]; entry->seg_addr = cpu_to_le64(trb_64); entry->seg_size = cpu_to_le32(TRBS_PER_SEGMENT);@@ -570,7 +569,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, xhci_flush_cache((uintptr_t)ctrl->erst.entries, ERST_NUM_SEGS * sizeof(struct xhci_erst_entry));
deq = (unsigned long)ctrl->event_ring->dequeue;
deq = virt_to_phys(ctrl->event_ring->dequeue); /* Update HC event ring dequeue pointer */ xhci_writeq(&ctrl->ir_set->erst_dequeue,@@ -585,7 +584,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, /* this is the event ring segment table pointer */ val_64 = xhci_readq(&ctrl->ir_set->erst_base); val_64 &= ERST_PTR_MASK;
val_64 |= ((uintptr_t)(ctrl->erst.entries) & ~ERST_PTR_MASK);
val_64 |= virt_to_phys(ctrl->erst.entries) & ~ERST_PTR_MASK; xhci_writeq(&ctrl->ir_set->erst_base, val_64);@@ -853,7 +852,7 @@ void xhci_setup_addressable_virt_dev(struct xhci_ctrl *ctrl, cpu_to_le32(((0 & MAX_BURST_MASK) << MAX_BURST_SHIFT) | ((3 & ERROR_COUNT_MASK) << ERROR_COUNT_SHIFT));
trb_64 = (uintptr_t)virt_dev->eps[0].ring->first_seg->trbs;
trb_64 = virt_to_phys(virt_dev->eps[0].ring->first_seg->trbs); ep0_ctx->deq = cpu_to_le64(trb_64 | virt_dev->eps[0].ring->cycle_state); /*diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 86aeaab412..092ed6eaf1 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -275,7 +275,7 @@ void xhci_queue_command(struct xhci_ctrl *ctrl, u8 *ptr, u32 slot_id, u32 ep_index, trb_type cmd) { u32 fields[4];
u64 val_64 = (uintptr_t)ptr;
u64 val_64 = virt_to_phys(ptr); BUG_ON(prepare_ring(ctrl, ctrl->cmd_ring, EP_STATE_RUNNING));@@ -399,7 +399,7 @@ void xhci_acknowledge_event(struct xhci_ctrl *ctrl)
/* Inform the hardware */ xhci_writeq(&ctrl->ir_set->erst_dequeue,
(uintptr_t)ctrl->event_ring->dequeue | ERST_EHB);
virt_to_phys(ctrl->event_ring->dequeue) | ERST_EHB);}
/**
@@ -577,7 +577,7 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, u64 addr; int ret; u32 trb_fields[4];
u64 val_64 = (uintptr_t)buffer;
u64 val_64 = virt_to_phys(buffer); debug("dev=%p, pipe=%lx, buffer=%p, length=%d\n", udev, pipe, buffer, length);@@ -876,7 +876,7 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe, if (length > 0) { if (req->requesttype & USB_DIR_IN) field |= TRB_DIR_IN;
buf_64 = (uintptr_t)buffer;
buf_64 = virt_to_phys(buffer); trb_fields[0] = lower_32_bits(buf_64); trb_fields[1] = upper_32_bits(buf_64);diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index ebd2954571..f89e274b0d 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -600,8 +600,7 @@ static int xhci_set_configuration(struct usb_device *udev) cpu_to_le32(MAX_BURST(max_burst) | ERROR_COUNT(err_count));
trb_64 = (uintptr_t)virt_dev->eps[ep_index].ring->enqueue;
trb_64 = virt_to_phys(virt_dev->eps[ep_index].ring->enqueue); ep_ctx[ep_index]->deq = cpu_to_le64(trb_64 | virt_dev->eps[ep_index].ring->cycle_state);--
Other than above, Reviewed-by: Bin Meng bmeng.cn@gmail.com
Regards, Bin
Thanks, Stefan
Hi Stefan,
On Fri, Jul 17, 2020 at 6:17 PM Stefan Roese sr@denx.de wrote:
Hi Bin,
On 17.07.20 07:57, Bin Meng wrote:
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
Some platforms, like MIPS Octeon, use mapped addresses (virtual address != physical address). On these platforms we need to make sure, that the local virtual addresses are converted to physical (DMA) addresses for the xHCI controller. This patch adds the missing virt_to_phys() calls, so that the correct addresses are used.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 19 +++++++++---------- drivers/usb/host/xhci-ring.c | 8 ++++---- drivers/usb/host/xhci.c | 3 +-- 3 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 3b805ecb9e..874caf4761 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -224,7 +224,7 @@ static void xhci_link_segments(struct xhci_segment *prev, return; prev->next = next; if (link_trbs) {
val_64 = (uintptr_t)next->trbs;
val_64 = virt_to_phys(next->trbs); prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = cpu_to_le64(val_64);@@ -484,7 +484,7 @@ int xhci_alloc_virt_device(struct xhci_ctrl *ctrl, unsigned int slot_id) /* Allocate endpoint 0 ring */ virt_dev->eps[0].ring = xhci_ring_alloc(1, true);
byte_64 = (uintptr_t)(virt_dev->out_ctx->bytes);
byte_64 = virt_to_phys(virt_dev->out_ctx->bytes); /* Point to output device context in dcbaa. */ ctrl->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(byte_64);@@ -509,7 +509,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, uint64_t val_64; uint64_t trb_64; uint32_t val;
unsigned long deq;
uint64_t deq;This change seems unnecessary?
In most (all other?) places, the variable containing these 64 bit addresses has the type uint64_t in this driver. So I wanted to make the code more consistant here. Or do you see a problem with this change?
No, just wondering if you see something is wrong with (unsigned long) on a 32-bit platform.
Regards, Bin
Hi Bin,
On 17.07.20 12:23, Bin Meng wrote:
Hi Stefan,
On Fri, Jul 17, 2020 at 6:17 PM Stefan Roese sr@denx.de wrote:
Hi Bin,
On 17.07.20 07:57, Bin Meng wrote:
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
Some platforms, like MIPS Octeon, use mapped addresses (virtual address != physical address). On these platforms we need to make sure, that the local virtual addresses are converted to physical (DMA) addresses for the xHCI controller. This patch adds the missing virt_to_phys() calls, so that the correct addresses are used.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 19 +++++++++---------- drivers/usb/host/xhci-ring.c | 8 ++++---- drivers/usb/host/xhci.c | 3 +-- 3 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 3b805ecb9e..874caf4761 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -224,7 +224,7 @@ static void xhci_link_segments(struct xhci_segment *prev, return; prev->next = next; if (link_trbs) {
val_64 = (uintptr_t)next->trbs;
val_64 = virt_to_phys(next->trbs); prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = cpu_to_le64(val_64);@@ -484,7 +484,7 @@ int xhci_alloc_virt_device(struct xhci_ctrl *ctrl, unsigned int slot_id) /* Allocate endpoint 0 ring */ virt_dev->eps[0].ring = xhci_ring_alloc(1, true);
byte_64 = (uintptr_t)(virt_dev->out_ctx->bytes);
byte_64 = virt_to_phys(virt_dev->out_ctx->bytes); /* Point to output device context in dcbaa. */ ctrl->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(byte_64);@@ -509,7 +509,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, uint64_t val_64; uint64_t trb_64; uint32_t val;
unsigned long deq;
uint64_t deq;This change seems unnecessary?
In most (all other?) places, the variable containing these 64 bit addresses has the type uint64_t in this driver. So I wanted to make the code more consistant here. Or do you see a problem with this change?
No, just wondering if you see something is wrong with (unsigned long) on a 32-bit platform.
I see. Thanks for checking.
Do you have a (little-endian) platform that you could use to test this patchset on? I currently do not have such a board on my desk and would very much like to see it tested on little-endian systems before being applied.
Thanks, Stefan
Hi Stefan,
On Fri, Jul 17, 2020 at 6:28 PM Stefan Roese sr@denx.de wrote:
Hi Bin,
On 17.07.20 12:23, Bin Meng wrote:
Hi Stefan,
On Fri, Jul 17, 2020 at 6:17 PM Stefan Roese sr@denx.de wrote:
Hi Bin,
On 17.07.20 07:57, Bin Meng wrote:
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
Some platforms, like MIPS Octeon, use mapped addresses (virtual address != physical address). On these platforms we need to make sure, that the local virtual addresses are converted to physical (DMA) addresses for the xHCI controller. This patch adds the missing virt_to_phys() calls, so that the correct addresses are used.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 19 +++++++++---------- drivers/usb/host/xhci-ring.c | 8 ++++---- drivers/usb/host/xhci.c | 3 +-- 3 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 3b805ecb9e..874caf4761 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -224,7 +224,7 @@ static void xhci_link_segments(struct xhci_segment *prev, return; prev->next = next; if (link_trbs) {
val_64 = (uintptr_t)next->trbs;
val_64 = virt_to_phys(next->trbs); prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = cpu_to_le64(val_64);@@ -484,7 +484,7 @@ int xhci_alloc_virt_device(struct xhci_ctrl *ctrl, unsigned int slot_id) /* Allocate endpoint 0 ring */ virt_dev->eps[0].ring = xhci_ring_alloc(1, true);
byte_64 = (uintptr_t)(virt_dev->out_ctx->bytes);
byte_64 = virt_to_phys(virt_dev->out_ctx->bytes); /* Point to output device context in dcbaa. */ ctrl->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(byte_64);@@ -509,7 +509,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, uint64_t val_64; uint64_t trb_64; uint32_t val;
unsigned long deq;
uint64_t deq;This change seems unnecessary?
In most (all other?) places, the variable containing these 64 bit addresses has the type uint64_t in this driver. So I wanted to make the code more consistant here. Or do you see a problem with this change?
No, just wondering if you see something is wrong with (unsigned long) on a 32-bit platform.
I see. Thanks for checking.
Do you have a (little-endian) platform that you could use to test this patchset on? I currently do not have such a board on my desk and would very much like to see it tested on little-endian systems before being applied.
Yes, I can test this series on x86 Minnowmax. Will send results then.
Regards, Bin
Hi Bin,
On 17.07.20 12:29, Bin Meng wrote:
Hi Stefan,
On Fri, Jul 17, 2020 at 6:28 PM Stefan Roese sr@denx.de wrote:
Hi Bin,
On 17.07.20 12:23, Bin Meng wrote:
Hi Stefan,
On Fri, Jul 17, 2020 at 6:17 PM Stefan Roese sr@denx.de wrote:
Hi Bin,
On 17.07.20 07:57, Bin Meng wrote:
Hi Stefan,
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
Some platforms, like MIPS Octeon, use mapped addresses (virtual address != physical address). On these platforms we need to make sure, that the local virtual addresses are converted to physical (DMA) addresses for the xHCI controller. This patch adds the missing virt_to_phys() calls, so that the correct addresses are used.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 19 +++++++++---------- drivers/usb/host/xhci-ring.c | 8 ++++---- drivers/usb/host/xhci.c | 3 +-- 3 files changed, 14 insertions(+), 16 deletions(-)diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 3b805ecb9e..874caf4761 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -224,7 +224,7 @@ static void xhci_link_segments(struct xhci_segment *prev, return; prev->next = next; if (link_trbs) {
val_64 = (uintptr_t)next->trbs;
val_64 = virt_to_phys(next->trbs); prev->trbs[TRBS_PER_SEGMENT-1].link.segment_ptr = cpu_to_le64(val_64);@@ -484,7 +484,7 @@ int xhci_alloc_virt_device(struct xhci_ctrl *ctrl, unsigned int slot_id) /* Allocate endpoint 0 ring */ virt_dev->eps[0].ring = xhci_ring_alloc(1, true);
byte_64 = (uintptr_t)(virt_dev->out_ctx->bytes);
byte_64 = virt_to_phys(virt_dev->out_ctx->bytes); /* Point to output device context in dcbaa. */ ctrl->dcbaa->dev_context_ptrs[slot_id] = cpu_to_le64(byte_64);@@ -509,7 +509,7 @@ int xhci_mem_init(struct xhci_ctrl *ctrl, struct xhci_hccr *hccr, uint64_t val_64; uint64_t trb_64; uint32_t val;
unsigned long deq;
uint64_t deq;This change seems unnecessary?
In most (all other?) places, the variable containing these 64 bit addresses has the type uint64_t in this driver. So I wanted to make the code more consistant here. Or do you see a problem with this change?
No, just wondering if you see something is wrong with (unsigned long) on a 32-bit platform.
I see. Thanks for checking.
Do you have a (little-endian) platform that you could use to test this patchset on? I currently do not have such a board on my desk and would very much like to see it tested on little-endian systems before being applied.
Yes, I can test this series on x86 Minnowmax. Will send results then.
Thanks a lot.
Thanks, Stefan
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese sr@denx.de wrote:
Some platforms, like MIPS Octeon, use mapped addresses (virtual address != physical address). On these platforms we need to make sure, that the local virtual addresses are converted to physical (DMA) addresses for the xHCI controller. This patch adds the missing virt_to_phys() calls, so that the correct addresses are used.
Signed-off-by: Stefan Roese sr@denx.de Cc: Bin Meng bmeng.cn@gmail.com Cc: Marek Vasut marex@denx.de
drivers/usb/host/xhci-mem.c | 19 +++++++++---------- drivers/usb/host/xhci-ring.c | 8 ++++---- drivers/usb/host/xhci.c | 3 +-- 3 files changed, 14 insertions(+), 16 deletions(-)
Tested-by: Bin Meng bmeng.cn@gmail.com
participants (2)
-
Bin Meng -
Stefan Roese