[PATCH v1 4/4] usb: xhci: Add virt_to_phys() to support mapped platforms

[Stefan Roese]

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 at 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 at 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 at denx.de>
>>>> Cc: Bin Meng <bmeng.cn at gmail.com>
>>>> Cc: Marek Vasut <marex at 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