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@xxxxxxxxxxx> wrote:
>
> On Tue, 2 Apr 2019 at 15:34, Robin Murphy <robin.murphy@xxxxxxx> wrote:
> >
> > On 02/04/2019 10:40, Pankaj Dubey wrote:
> > > From: Sriram Dash <sriram.dash@xxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxx>
> > > Signed-off-by: Sriram Dash <sriram.dash@xxxxxxxxxxx>
> > > ---
> > >   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