Re: [PATCH 2/2] of: configure DMA for host1x devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 25, 2017 at 7:26 AM, Robin Murphy <robin.murphy@xxxxxxx> wrote:
> On 25/09/17 08:44, Thierry Reding wrote:
>> On Sun, Sep 24, 2017 at 12:04:54PM +0300, Mikko Perttunen wrote:
>>> Devices in the host1x bus rely on the old behavior of of_dma_configure
>>> to set up DMA ops. Add a check for them into of_dma_configure.
>>>
>>> We must do the check using a string comparison instead of using
>>> pointers since the host1x bus can be compiled into a module.
>>>
>>> Fixes: 723288836628 ("of: restrict DMA configuration")
>>> Signed-off-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
>>> ---
>>>  drivers/of/device.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>> index 64b710265d39..12368418cd33 100644
>>> --- a/drivers/of/device.c
>>> +++ b/drivers/of/device.c
>>> @@ -104,6 +104,9 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>>>              if (!dev_is_pci(dev) &&
>>>  #ifdef CONFIG_ARM_AMBA
>>>                  dev->bus != &amba_bustype &&
>>> +#endif
>>> +#if IS_ENABLED(CONFIG_TEGRA_HOST1X)
>>> +                !(dev->bus && !strcmp("host1x", dev->bus->name)) &&
>>>  #endif
>>>                  dev->bus != &platform_bus_type)
>>>                      return ret == -ENODEV ? 0 : ret;
>>
>> Maybe a slightly better solution would be to add a dev_is_host1x()
>> function that returns dev->bus == &host1x_bus_type if TEGRA_HOST1X is
>> enabled and returns false otherwise. That way we can have a "proper"
>> check for the bus type and avoid the #if in this file.
>>
>> It's slightly more complicated because of the dependencies between the
>> DT and Tegra DRM trees, but I'm sure we can get Rob to either give his
>> Acked-by on the drivers/of/device.c change or carry the Tegra DRM patch
>> in the DT tree.
>
> Alternatively (and modulo Greg's opinion, obviously) it would arguably be
> neatest to handle this at a slightly lower level as below - I only really
> considered this idea after writing the existing fix, but now that I have
> actually written it up I do rather like it.
>
> Robin.
>
> ----->8-----
> From: Robin Murphy <robin.murphy@xxxxxxx>
> Date: Mon, 25 Sep 2017 12:45:58 +0100
> Subject: [PATCH] drivers: Flag buses which demand DMA configuration
>
> We do not want the common dma_configure() pathway to apply
> indiscriminately to all devices, since there are plenty of buses which
> do not have DMA capability, and if their child devices were used for
> DMA API calls it would only be indicative of a driver bug. However,
> there are a number of buses for which DMA is implicitly expected even
> when not described by firmware, so we currently whitelist those to
> automatically opt in to dma_configure(), assuming a 1:1 mapping between
> the DMA address space and the physical address space.
>
> Commit 723288836628 ("of: restrict DMA configuration") introduced a
> short-term fix by comparing explicit bus types, but this approach is far
> from pretty, doesn't scale well, and fails to cope at all with bus
> drivers which may be built as modules. Let's refine things by making
> that opt-in a property of the bus type, which neatly addresses those
> problems and lets the decision of whether firmware description of DMA
> capability should be optional or mandatory stay internal to the bus
> drivers themselves.
>
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>

Acked-by: Rob Herring <robh@xxxxxxxxxx>

Are you going to send out as a proper patch? We've now got another
case with pci_epf_bus_type...

Rob

> ---
>  drivers/amba/bus.c       | 1 +
>  drivers/base/platform.c  | 1 +
>  drivers/gpu/host1x/bus.c | 1 +
>  drivers/of/device.c      | 8 +-------
>  drivers/pci/pci-driver.c | 1 +
>  include/linux/device.h   | 4 ++++
>  6 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index e0f74ddc22b7..594c228d2f02 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -195,6 +195,7 @@ struct bus_type amba_bustype = {
>         .match          = amba_match,
>         .uevent         = amba_uevent,
>         .pm             = &amba_pm,
> +       .force_dma      = true,
>  };
>
>  static int __init amba_init(void)
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index d1bd99271066..9c95c8db00f8 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1142,6 +1142,7 @@ struct bus_type platform_bus_type = {
>         .match          = platform_match,
>         .uevent         = platform_uevent,
>         .pm             = &platform_dev_pm_ops,
> +       .force_dma      = true,
>  };
>  EXPORT_SYMBOL_GPL(platform_bus_type);
>
> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> index f9cde03030fd..ed03b3243195 100644
> --- a/drivers/gpu/host1x/bus.c
> +++ b/drivers/gpu/host1x/bus.c
> @@ -320,6 +320,7 @@ struct bus_type host1x_bus_type = {
>         .name = "host1x",
>         .match = host1x_device_match,
>         .pm = &host1x_device_pm_ops,
> +       .force_dma = true,
>  };
>
>  static void __host1x_device_del(struct host1x_device *device)
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 64b710265d39..25bddf9c9fe1 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -9,9 +9,7 @@
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/slab.h>
> -#include <linux/pci.h>
>  #include <linux/platform_device.h>
> -#include <linux/amba/bus.h>
>
>  #include <asm/errno.h>
>  #include "of_private.h"
> @@ -101,11 +99,7 @@ int of_dma_configure(struct device *dev, struct device_node *np)
>                  * DMA configuration regardless of whether "dma-ranges" is
>                  * correctly specified or not.
>                  */
> -               if (!dev_is_pci(dev) &&
> -#ifdef CONFIG_ARM_AMBA
> -                   dev->bus != &amba_bustype &&
> -#endif
> -                   dev->bus != &platform_bus_type)
> +               if (!dev->bus->force_dma)
>                         return ret == -ENODEV ? 0 : ret;
>
>                 dma_addr = offset = 0;
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 11bd267fc137..38bdb97b6dc6 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1466,6 +1466,7 @@ struct bus_type pci_bus_type = {
>         .drv_groups     = pci_drv_groups,
>         .pm             = PCI_PM_OPS_PTR,
>         .num_vf         = pci_bus_num_vf,
> +       .force_dma      = true,
>  };
>  EXPORT_SYMBOL(pci_bus_type);
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index c6f27207dbe8..3a1efa56e7c4 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -97,6 +97,8 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
>   * @p:         The private data of the driver core, only the driver core can
>   *             touch this.
>   * @lock_key:  Lock class key for use by the lock validator
> + * @force_dma: Assume devices on this bus should be set up by dma_configure()
> + *             even if DMA capability is not explicitly described by firmware.
>   *
>   * A bus is a channel between the processor and one or more devices. For the
>   * purposes of the device model, all devices are connected via a bus, even if
> @@ -135,6 +137,8 @@ struct bus_type {
>
>         struct subsys_private *p;
>         struct lock_class_key lock_key;
> +
> +       bool force_dma;
>  };
>
>  extern int __must_check bus_register(struct bus_type *bus);
> --
> 2.13.4.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [ARM Kernel]     [Linux ARM]     [Linux ARM MSM]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux