Re: [PATCH] usb: xhci: inherit dma_mask from bus if set correctly

[Pankaj Dubey <pankaj.dubey@samsung.com>]

On 4/10/19 4:32 AM, Robin Murphy wrote:
> On 2019-04-09 3:56 am, Sriram Dash wrote:
>> On Tue, Apr 2, 2019 at 9:53 PM Pankaj Dubey 
>> <pankaj.dubey@samsung.com> wrote:
>>>
>>> On Tue, 2 Apr 2019 at 15:34, Robin Murphy <robin.murphy@arm.com> wrote:
>>>>
>>>> On 02/04/2019 10:40, Pankaj Dubey wrote:
>>>>> From: Sriram Dash <sriram.dash@samsung.com>
>>>>>
>>>>> The xhci forcefully converts the dma_mask to either 64 or 32 and the
>>>>> dma-mask set by the bus is somewhat ignored. If the platform  sets 
>>>>> the
>>>>> correct dma_mask, then respect that.
>>>>
>>>> It's expected for dma_mask to be larger than bus_dma_mask if the 
>>>> latter
>>>> is set - conceptually, the device mask represents what the device is
>>>> inherently capable of, while the bus mask represents external
>>>> interconnect restrictions which individual drivers should not have to
>>>> care about. The DMA API backend should take care of combining the 
>>>> two to
>>>> find the intersection.
>>>
>>> Thanks for the review.
>>>
>>> We are dealing here with the xhci platform which inherits the dma mask
>>> of the parent, which is from a controller device.
>>>
>>> When the controller dma mask is set by the platform in DT, what we
>>> observe is, its not getting inherited properly and the xhci bus is
>>> forcing the dma address to be either 32 bit or 64 bit.
>>>
>>> In "drivers/usb/host/xhci-plat.c" we have dma_mask setting as below:
>>>
>>>   /* Try to set 64-bit DMA first */
>>> if (WARN_ON(!sysdev->dma_mask))
>>>       /* Platform did not initialize dma_mask */
>>>       ret = dma_coerce_mask_and_coherent(sysdev, DMA_BIT_MASK(64));
>>> else
>>>       ret = dma_set_mask_and_coherent(sysdev, DMA_BIT_MASK(64));
>>>
>>> So even if the controller device has set the dma_mask as per it's
>>> configuration in DT, xhci-plat.c will override it here in else part.
>>>
>>> Next it goes to "drivers/usb/host/xhci.c" file, here we have code as:
>>>
>>> /* Set dma_mask and coherent_dma_mask to 64-bits,
>>>   * if xHC supports 64-bit addressing */
>>> if (HCC_64BIT_ADDR(xhci->hcc_params) &&
>>>                  !dma_set_mask(dev, DMA_BIT_MASK(64))) {
>>>          xhci_dbg(xhci, "Enabling 64-bit DMA addresses.\n");
>>>          dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
>>> } else {
>>>          /*
>>>           * This is to avoid error in cases where a 32-bit USB
>>>           * controller is used on a 64-bit capable system.
>>>           */
>>>          retval = dma_set_mask(dev, DMA_BIT_MASK(32));
>>>          if (retval)
>>>                  return retval;
>>>          xhci_dbg(xhci, "Enabling 32-bit DMA addresses.\n");
>>>          dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>>> }
>>>
>>> So xhci will force the dma_mask to either DMA_BIT_MASK(32) or
>>> DMA_BIT_MASK(64), what if my device needs other than 32 bit or 64 bit
>>> dma_mask.
>>>
>>> The bus_dma_mask was introduced for a case when the bus from a
>>> device's dma interface may carry fewer address bits. But apparently,
>>> it is the only mask which retains the original dma addressing from the
>>> parent. So basically what we observe is currently there is no way we
>>> can pass dma_mask greater than 32-bit, from DT via dev->dma_mask or
>>> dev->coherent_dma_mask due to below logic in
>>>
>>> from "drivers/of/platform.c" we have
>>> static struct platform_device *of_platform_device_create_pdata(
>>>                                          struct device_node *np,
>>>                                          const char *bus_id,
>>>                                          void *platform_data,
>>>                                          struct device *parent)
>>> {
>>>        struct platform_device *dev;
>>>        ...
>>>        dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>>>        if (!dev->dev.dma_mask)
>>>               dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
>>>    ...
>>> }
>>>
>>> and then in of_dma_configure function in "drivers/of/device.c" we 
>>> have..
>>>
>>> mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1); //This mask
>>> computation is going fine and gets mask greater than 32-bit if defined
>>> in DT
>>> dev->coherent_dma_mask &= mask;  // Here the higher bit [63:32] will
>>> get truncated as coherent_dma_mask is initialized to DMA_BIT_MASK(32)
>>> in platform.c
>>>
>>> *dev->dma_mask &= mask; //Same here higher bits will get truncated
>>> /* ...but only set bus mask if we found valid dma-ranges earlier */
>>> if (!ret)
>>> dev->bus_dma_mask = mask; //Only bus_dma_mask can carry the original
>>> mask as specified in platform DT.
>>>
>>> To minimise the impact on existing code, we reused the bus_dma_mask
>>> for finding the dma addressing bits.
>>>
>>> Or other way we may need to initialise dma_mask/coherent_dma_mask as
>>> DMA_BIT_MASK(64) in "drivers/of/platform.c" and let all devices set
>>> dma_mask via DT using "dma-ranges" property or respective platform
>>> driver.
>>>
>>>> Are you seeing an actual problem here, and if so
>>>> on which platform? (If the bus mask is set at all then it wouldn't 
>>>> seem
>>>> to be the DT PCI issue that I'm still trying to fix).
>>>>
>>>
>>>
>>> We are facing this issue in one of the Samsung's upcoming SoC where we
>>> need dma_mask greater than 32-bit.
>>>
>>> Thanks,
>>> Pankaj
>>>> Robin.
>>>>
>>>>> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
>>>>> Signed-off-by: Sriram Dash <sriram.dash@samsung.com>
>>>>> ---
>>>>>    drivers/usb/host/xhci.c | 10 ++++++++++
>>>>>    1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>>>> index 005e659..55cf89e 100644
>>>>> --- a/drivers/usb/host/xhci.c
>>>>> +++ b/drivers/usb/host/xhci.c
>>>>> @@ -5119,6 +5119,16 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
>>>>> xhci_get_quirks_t get_quirks)
>>>>>                dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>>>>>        }
>>>>>
>>>>> +     /*
>>>>> +      * A platform may require coherent masks other than 64/32 
>>>>> bit, and we
>>>>> +      * should respect that. If the firmware has already 
>>>>> requested for a
>>>>> +      * dma-range, we inherit the dma_mask presuming the platform 
>>>>> knows
>>>>> +      * what it is doing.
>>>>> +      */
>>>>> +
>>>>> +     if (dev->bus_dma_mask)
>>>>> +             dma_set_mask_and_coherent(dev, dev->bus_dma_mask);
>>>>> +
>>>>>        xhci_dbg(xhci, "Calling HCD init\n");
>>>>>        /* Initialize HCD and host controller data structures. */
>>>>>        retval = xhci_init(hcd);
>>>>>
>>
>> Hello Robin,
>>
>> Hope you found the crux of the matter. Any comments on the same?
>
> Sorry, I never received either of these replies - I've just happened 
> to notice this thread again by pure chance while looking at the 
> linux-usb patchwork for something else entirely, and managed to dredge 
> an mbox off lore.kernel.org to reply to. Mail is not my area of 
> expertise, but looking at the headers of the initial patch in my inbox 
> it seems that outlook.com is doing SPF negotiation with samsung.com, 
> so sending via gmail (as those replies appear to be) may be failing 
> that and getting silently discarded (they're not even in my spam 
> quarantine).
>
> From the snippets of code quoted above I don't see anything obviously 
> wrong, but I'll take a closer look tomorrow. AFAICS though, if 
> dev->bus_dma_mask is set then dev is probably the appropriate device 
> for DMA, so I wouldn't expect a problem - XHCI is inherently a 64-bit 
> device, so its driver *should* be setting a 64-bit mask in this case. 
> To reiterate, what's the nature of the DMA issue? Do the mapping 
> operations fail, or do you actually see transfers going wrong due to 
> address truncation? Also, which arch is involved here - is it arm64 
> (as I seem to have assumed), or something else?
>
> Robin.
>

Regarding issue in above code snippet, current code sets 
"dev->dev.coherent_dma_mask" as DMA_BIT_MASK(32) in platform.c 
(irrespective of architecture) and when we parse "dma-ranges" property 
and try to set coherent_dma_mask or dma_mask in of_dma_configure 
function in "drivers/of/device.c", even if "dma-ranges" specified in DT 
is more than 32-bit, 32-63 bits will be cleared to zero due to masking 
done in platform.c.

So effectively we are not able to set dma_mask or coherent_dma_mask 
larger than 32-bit mask.

For the SoC concerned here is based on arm64, the USB IP (64 bit 
capable) is connected to a 36-bit Data bus and a 32-bit Control bus. The 
36-bit Data bus is connected to an IOMMU which translates the address 
before they are passed on to other blocks. Here we have IOMMU is capable 
of 40-bit addressing. But as data bus is only capable of 36-bit, we need 
to ensure that IOMMU translates to address which does not exceed 36-bit.

Without this fix we are observing context fault from IOMMU.

Now, to get a workaround to this problem, we are inheriting the 
bus_dma_mask which is apparently the only one which contains the 36-bit 
bus mask.

Or as alternate solution we need to change coherent_dma_mask default 
mask as DMA_BIT_MASK(64) rather DMA_BIT_MASK(32) so that in 
of_dma_configure, the dma_mask/coherent_dma_mask get populated from 
"dma_ranges" DT property during device registration.