| Message ID | 20200702084733.2032531-3-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [v1,1/4] usb: xhci: Add missing endian conversions (cpu_to_leXX / leXX_to_cpu) | expand |
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>
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;
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(-)