Re: [PATCH 1/3] of/pci/dma: fix DMA configuration for PCI masters

[Oza Oza]

On Thu, May 4, 2017 at 11:32 PM, Robin Murphy <robin.murphy@xxxxxxx> wrote:
> [apologies for the silence - I've been on holiday]
>
> On 03/05/17 05:46, Oza Pawandeep wrote:
>> current device framework and of framework integration assumes
>> dma-ranges in a way where memory-mapped devices define their
>> dma-ranges. (child-bus-address, parent-bus-address, length).
>
> Well, yes, that is simply the definition of dma-ranges, and remains true
> regardless of the particular format of either bus address.
>
>> of_dma_configure is specifically written to take care of memory
>> mapped devices. but no implementation exists for pci to take
>> care of pcie based memory ranges.
>
> That still doesn't make sense. To repeat myself again, PCI devices *ARE*
> memory-mapped devices. Yes, there do exist some platforms where I/O
> space is not treated as MMIO, but config space and memory space are very
> much memory-mapped however you look at them, and in the context of DMA,
> only memory space is relevant anyway.
>
> What *is* true about the current code is that of_dma_get_range() expects
> to be passed an OF node representing the device itself, and doesn't work
> properly when passed the node of the device's parent bus directly, which
> happens to be what pci_dma_configure() currently does. That's the only
> reason why it doesn't work for (single-entry) host controller dma-ranges
> today. This does not mean it's a PCI problem, it is simply the case that
> pci_dma_configure() is the only caller currently hitting it. Other
> discoverable, DMA-capable buses like fsl-mc are still going to face the
> exact same problem with or without this patch.
>

the new v2 is hooking callbacks for defualt and pci bus.
so now the implementation will not really look cluttered.
and for fsl-mc buses, we could choose to implement it in default bus callbacks.
will post the patch-set soon.

also with these patch-set we really do not need to prepare emulated child node.


>> for e.g. iproc based SOCs and other SOCs(suc as rcar) have PCI
>> world dma-ranges.
>> dma-ranges = <0x43000000 0x00 0x00 0x00 0x00 0x80 0x00>;
>>
>> this patch serves following:
>>
>> 1) exposes interface to the pci host driver for their
>> inbound memory ranges
>>
>> 2) provide an interface to callers such as of_dma_get_ranges.
>> so then the returned size get best possible (largest) dma_mask.
>> because PCI RC drivers do not call APIs such as
>> dma_set_coherent_mask() and hence rather it shows its addressing
>> capabilities based on dma-ranges.
>> for e.g.
>> dma-ranges = <0x43000000 0x00 0x00 0x00 0x00 0x80 0x00>;
>> we should get dev->coherent_dma_mask=0x7fffffffff.
>>
>> 3) this patch handles multiple inbound windows and dma-ranges.
>> it is left to the caller, how it wants to use them.
>> the new function returns the resources in a standard and unform way
>>
>> 4) this way the callers of for e.g. of_dma_get_ranges
>> does not need to change.
>>
>> 5) leaves scope of adding PCI flag handling for inbound memory
>> by the new function.
>
> Which flags would ever actually matter? DMA windows aren't going to be
> to config or I/O space, so the memory type can be assumed, and the
> 32/64-bit distinction is irrelevant as it's not a relocatable BAR;
> DMA-able system memory isn't going to be read-sensitive, so the
> prefetchable flag shouldn't matter; and not being a BAR none of the
> others would be relevant either.
>
>>
>> Bug: SOC-5216
>> Change-Id: Ie045386df91e1e0587846bb147ae40d96f6d7d2e
>> Signed-off-by: Oza Pawandeep <oza.oza@xxxxxxxxxxxx>
>> Reviewed-on: http://gerrit-ccxsw.broadcom.net/40428
>> Reviewed-by: vpx_checkpatch status <vpx_checkpatch@xxxxxxxxxxxx>
>> Reviewed-by: CCXSW <ccxswbuild@xxxxxxxxxxxx>
>> Reviewed-by: Ray Jui <ray.jui@xxxxxxxxxxxx>
>> Tested-by: vpx_autobuild status <vpx_autobuild@xxxxxxxxxxxx>
>> Tested-by: vpx_smoketest status <vpx_smoketest@xxxxxxxxxxxx>
>> Tested-by: CCXSW <ccxswbuild@xxxxxxxxxxxx>
>> Reviewed-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
>>
>> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
>> index 0ee42c3..ed6e69a 100644
>> --- a/drivers/of/of_pci.c
>> +++ b/drivers/of/of_pci.c
>> @@ -283,6 +283,83 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
>>       return err;
>>  }
>>  EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
>> +
>> +/**
>> + * of_pci_get_dma_ranges - Parse PCI host bridge inbound resources from DT
>> + * @np: device node of the host bridge having the dma-ranges property
>> + * @resources: list where the range of resources will be added after DT parsing
>> + *
>> + * It is the caller's job to free the @resources list.
>> + *
>> + * This function will parse the "dma-ranges" property of a
>> + * PCI host bridge device node and setup the resource mapping based
>> + * on its content.
>> + *
>> + * It returns zero if the range parsing has been successful or a standard error
>> + * value if it failed.
>> + */
>> +
>> +int of_pci_get_dma_ranges(struct device_node *np, struct list_head *resources)
>> +{
>> +     struct device_node *node = of_node_get(np);
>> +     int rlen;
>> +     int ret = 0;
>> +     const int na = 3, ns = 2;
>> +     struct resource *res;
>> +     struct of_pci_range_parser parser;
>> +     struct of_pci_range range;
>> +
>> +     if (!node)
>> +             return -EINVAL;
>> +
>> +     parser.node = node;
>> +     parser.pna = of_n_addr_cells(node);
>> +     parser.np = parser.pna + na + ns;
>> +
>> +     parser.range = of_get_property(node, "dma-ranges", &rlen);
>> +
>> +     if (!parser.range) {
>> +             pr_debug("pcie device has no dma-ranges defined for node(%s)\n",
>> +                       np->full_name);
>> +             ret = -EINVAL;
>> +             goto out;
>> +     }
>> +
>> +     parser.end = parser.range + rlen / sizeof(__be32);
>> +
>> +     for_each_of_pci_range(&parser, &range) {
>
> This is plain wrong - of_pci_range_parser_one() will translate upwards
> through parent "ranges" properties, which is completely backwards for
> DMA addresses.
>
> Robin.
>
>> +             /*
>> +              * If we failed translation or got a zero-sized region
>> +              * then skip this range
>> +              */
>> +             if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
>> +                     continue;
>> +
>> +             res = kzalloc(sizeof(struct resource), GFP_KERNEL);
>> +             if (!res) {
>> +                     ret = -ENOMEM;
>> +                     goto parse_failed;
>> +             }
>> +
>> +             ret = of_pci_range_to_resource(&range, np, res);
>> +             if (ret) {
>> +                     kfree(res);
>> +                     continue;
>> +             }
>> +
>> +             pci_add_resource_offset(resources, res,
>> +                                     res->start - range.pci_addr);
>> +     }
>> +
>> +     return ret;
>> +
>> +parse_failed:
>> +     pci_free_resource_list(resources);
>> +out:
>> +     of_node_put(node);
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(of_pci_get_dma_ranges);
>>  #endif /* CONFIG_OF_ADDRESS */
>>
>>  #ifdef CONFIG_PCI_MSI
>> diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
>> index 0e0974e..617b90d 100644
>> --- a/include/linux/of_pci.h
>> +++ b/include/linux/of_pci.h
>> @@ -76,6 +76,7 @@ static inline void of_pci_check_probe_only(void) { }
>>  int of_pci_get_host_bridge_resources(struct device_node *dev,
>>                       unsigned char busno, unsigned char bus_max,
>>                       struct list_head *resources, resource_size_t *io_base);
>> +int of_pci_get_dma_ranges(struct device_node *np, struct list_head *resources);
>>  #else
>>  static inline int of_pci_get_host_bridge_resources(struct device_node *dev,
>>                       unsigned char busno, unsigned char bus_max,
>> @@ -83,6 +84,12 @@ static inline int of_pci_get_host_bridge_resources(struct device_node *dev,
>>  {
>>       return -EINVAL;
>>  }
>> +
>> +static inline int of_pci_get_dma_ranges(struct device_node *np,
>> +                                     struct list_head *resources)
>> +{
>> +     return -EINVAL;
>> +}
>>  #endif
>>
>>  #if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI)
>>
>