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

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

[Pankaj Dubey]

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.

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);
-- 
2.7.4

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

[Robin Murphy]

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. 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).

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);
> 

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

[Pankaj Dubey]

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);
> >

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

[Sriram Dash]

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?

-- 
Regards,
Sriram

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

[Robin Murphy]

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.

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

[Pankaj Dubey]

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.

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

[Robin Murphy]

On 24/04/2019 10:05, Pankaj Dubey wrote:
> 
> 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 better or worse, that is the expected and intended behaviour for the 
default device masks. Drivers these days are expected to explicitly set 
their supported mask to replace the default, but there are still some 
remaining legacy assumptions that the default masks are 32-bit, so 
making them bigger risks subtle breakage, and that's why 
of_dma_configure() does the weird things it does.

> 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.

Thanks for the clarification.

> 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.

If the bus mask is correctly set to 36 bits, but the DMA API 
implementation is failing to take that into account and giving you 
40-bit DMA addresses, that is a bug in the DMA API implementation, and 
it needs to be fixed in that DMA API code, not worked around in 
individual drivers.

Is this a 32-bit Arm system, by any chance?

Robin.

> 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.
> 
> 

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

[Pankaj Dubey]

On 4/24/19 4:28 PM, Robin Murphy wrote:
> On 24/04/2019 10:05, Pankaj Dubey wrote:
>>
>> 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 better or worse, that is the expected and intended behaviour for 
> the default device masks. Drivers these days are expected to 
> explicitly set their supported mask to replace the default, but there 
> are still some remaining legacy assumptions that the default masks are 
> 32-bit, so making them bigger risks subtle breakage, and that's why 
> of_dma_configure() does the weird things it does.
>
In that case, for systems supporting masks greater than 32-bit, IMO, 
they should be able to handle the mask properly via DT. Without 
disturbing legacy code, this is one solution that can be considered. 
Requesting you to give your opinion on this, if it is acceptable we will 
submit formal patch for this.

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 3717f2a..9cc7a28 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -151,10 +151,19 @@ int of_dma_configure(struct device *dev, struct 
device_node *np, bool force_dma)
         mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1);
         dev->coherent_dma_mask &= mask;
         *dev->dma_mask &= mask;
-       /* ...but only set bus mask if we found valid dma-ranges earlier */
-       if (!ret)
-               dev->bus_dma_mask = mask;

+       /*
+        * ...but only set bus mask if we found valid dma-ranges earlier
+        * and also, set the coherent_dma_mask and dma_mask properly
+        * for busses with size more than 32-bit
+        */
+       if (!ret) {
+               dev->bus_dma_mask = mask;
+               if (mask  >= (1ULL << 32)) {
+                       dev->coherent_dma_mask = mask;
+                       *dev->dma_mask = mask;
+               }
+       }
         coherent = of_dma_is_coherent(np);
         dev_dbg(dev, "device is%sdma coherent\n",
                 coherent ? " " : " not ");


>> 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.
>
> Thanks for the clarification.
>
>> 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.
>
> If the bus mask is correctly set to 36 bits, but the DMA API 
> implementation is failing to take that into account and giving you 
> 40-bit DMA addresses, that is a bug in the DMA API implementation, and 
> it needs to be fixed in that DMA API code, not worked around in 
> individual drivers.

There are 2 issues here. First being, the 32-bit limitation for the 
device dma_mask during device registration. You have already suggested 
one approach for this to set from driver itself. IMO, this would require 
another DT node property addition for any individual IP. As same driver 
is being used in another SoC where, it may not need more than 
32-bit/64-bit dma_mask.

For the SoC concerned here, there are multiple IPs which are sharing the 
36-bit DATA bus. If of_dma_configure" takes care of assigning the 
dma_mask properly from "dma-ranges" DT property, it would solve this 
issue and driver's do not need to set this explicitly either via 
hard-coding or through another DT property. For this we suggest code 
snippet as given above.

The second problem is the XHCI overrides dma_mask to 32-bit or 64-bit. 
During the device registration, the DWC3 device get the default 32-bit 
dma_mask. This DWC3 serves as the parent device for XHCI device, which 
also gets 32-bit dma_mask from inception. There are 2 places for the 
xhci-device, where the dma_mask of the xhci-device is explicitly 
modified to either 32-bit or 64-bit.

1) In "drivers/usb/host/xhci-plat.c" we have dma_mask setting as below:

    ret = dma_coerce_mask_and_coherent(sysdev, DMA_BIT_MASK(64));

2) In "drivers/usb/host/xhci.c" file, here we have code as:

    dma_set_coherent_mask(dev, DMA_BIT_MASK(64));

So, even if the platform sets the dma_mask properly to 36-bit, the xhci 
driver overrides it to 32-bit or 64-bit. This can be fixed in the xhci 
driver by checking if the dma_mask is already getting set in the parent 
driver. For this we have not yet submitted the change, as in this case 
of_dma_configure needs to be fixed first.

However, the proposed solution (current patch) is leveraging 
the dma_mask from bus_dma_mask, which is set from the dma-ranges 
properly, and this case we don't need any changes in of_dma_configure.

> Is this a 32-bit Arm system, by any chance?
>
For the SoC concerned here, it is a 64-bit ARM system, which has many 
IPs connected via the 36-bit DATA bus.

> Robin.
>
>> 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.
>>
>>
>
>

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

[Robin Murphy]

On 26/04/2019 05:46, Pankaj Dubey wrote:
> 
> On 4/24/19 4:28 PM, Robin Murphy wrote:
>> On 24/04/2019 10:05, Pankaj Dubey wrote:
>>> 
>>> 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 better or worse, that is the expected and intended behaviour
>> for the default device masks. Drivers these days are expected to 
>> explicitly set their supported mask to replace the default, but
>> there are still some remaining legacy assumptions that the default
>> masks are 32-bit, so making them bigger risks subtle breakage, and
>> that's why of_dma_configure() does the weird things it does.
>> 
> In that case, for systems supporting masks greater than 32-bit, IMO, 
> they should be able to handle the mask properly via DT. Without 
> disturbing legacy code, this is one solution that can be considered. 
> Requesting you to give your opinion on this, if it is acceptable we
> will submit formal patch for this.
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c index
> 3717f2a..9cc7a28 100644 --- a/drivers/of/device.c +++
> b/drivers/of/device.c @@ -151,10 +151,19 @@ int
> of_dma_configure(struct device *dev, struct device_node *np, bool
> force_dma) mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1); 
> dev->coherent_dma_mask &= mask; *dev->dma_mask &= mask; -       /*
> ...but only set bus mask if we found valid dma-ranges earlier */ -
> if (!ret) -               dev->bus_dma_mask = mask;
> 
> +       /* +        * ...but only set bus mask if we found valid
> dma-ranges earlier +        * and also, set the coherent_dma_mask and
> dma_mask properly +        * for busses with size more than 32-bit +
> */ +       if (!ret) { +               dev->bus_dma_mask = mask; +
> if (mask  >= (1ULL << 32)) { +
> dev->coherent_dma_mask = mask; +                       *dev->dma_mask
> = mask; +               } +       } coherent =
> of_dma_is_coherent(np); dev_dbg(dev, "device is%sdma coherent\n", 
> coherent ? " " : " not ");
> 
> 
>>> 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.
>> 
>> Thanks for the clarification.
>> 
>>> 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.
>> 
>> If the bus mask is correctly set to 36 bits, but the DMA API 
>> implementation is failing to take that into account and giving you 
>> 40-bit DMA addresses, that is a bug in the DMA API implementation,
>> and it needs to be fixed in that DMA API code, not worked around
>> in individual drivers.
> 
> There are 2 issues here. First being, the 32-bit limitation for the 
> device dma_mask during device registration. You have already
> suggested one approach for this to set from driver itself. IMO, this
> would require another DT node property addition for any individual
> IP. As same driver is being used in another SoC where, it may not
> need more than 32-bit/64-bit dma_mask.
> 
> For the SoC concerned here, there are multiple IPs which are sharing
> the 36-bit DATA bus. If of_dma_configure" takes care of assigning
> the dma_mask properly from "dma-ranges" DT property, it would solve
> this issue and driver's do not need to set this explicitly either
> via hard-coding or through another DT property. For this we suggest
> code snippet as given above.
> 
> The second problem is the XHCI overrides dma_mask to 32-bit or
> 64-bit. During the device registration, the DWC3 device get the
> default 32-bit dma_mask. This DWC3 serves as the parent device for
> XHCI device, which also gets 32-bit dma_mask from inception. There
> are 2 places for the xhci-device, where the dma_mask of the
> xhci-device is explicitly modified to either 32-bit or 64-bit.
> 
> 1) In "drivers/usb/host/xhci-plat.c" we have dma_mask setting as
> below:
> 
> ret = dma_coerce_mask_and_coherent(sysdev, DMA_BIT_MASK(64));
> 
> 2) In "drivers/usb/host/xhci.c" file, here we have code as:
> 
> dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
> 
> So, even if the platform sets the dma_mask properly to 36-bit, the
> xhci driver overrides it to 32-bit or 64-bit. This can be fixed in
> the xhci driver by checking if the dma_mask is already getting set in
> the parent driver. For this we have not yet submitted the change, as
> in this case of_dma_configure needs to be fixed first.

None of that is a real problem, and none of it needs to be fixed. As I 
tried to explain before, the bus mask and device masks are *expected* to 
be different sizes, because they represent different things, and are 
owned by different levels of the driver model abstraction.

> However, the proposed solution (current patch) is leveraging the
> dma_mask from bus_dma_mask, which is set from the dma-ranges 
> properly, and this case we don't need any changes in
> of_dma_configure.
> 
>> Is this a 32-bit Arm system, by any chance?
>> 
> For the SoC concerned here, it is a 64-bit ARM system, which has
> many IPs connected via the 36-bit DATA bus.

OK, if it's an arm64 system with an IOMMU, then all your DMA addresses 
come from here:


static dma_addr_t iommu_dma_alloc_iova(struct iommu_domain *domain,
		size_t size, dma_addr_t dma_limit, struct device *dev)
{
	...
	if (dev->bus_dma_mask)
		dma_limit &= dev->bus_dma_mask;
	...
}


Most likely something somewhere is passing the wrong device into DMA API 
calls - *that* is what needs to be fixed.

Of course I can't 100% rule out the possibility that something else is 
going screwy as well or instead, but either way, start debugging from 
iommu_dma_alloc_iova() and work backwards until you can see why 
dma_limit is not getting clamped to the appropriate bus mask.

Robin.