On Fri, Jan 2, 2015 at 11:20 AM, Murali Karicheri <m-karicheri2@xxxxxx> wrote:
> Rob,
>
> See my response below. Arnd and Will, please review this as well.
>
> On 12/26/2014 02:33 PM, Rob Herring wrote:
>>
>> On Wed, Dec 24, 2014 at 4:11 PM, Murali Karicheri<m-karicheri2@xxxxxx>
>> wrote:
>>>
>>> Add of_pci_dma_configure() to allow updating the dma configuration
>>> of the pci device using the configuration from DT of the parent of
>>> the root bridge device.
>>>
>>> Signed-off-by: Murali Karicheri<m-karicheri2@xxxxxx>
>>> ---
>>> drivers/of/of_pci.c | 73
>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/of_pci.h | 12 ++++++++
>>> 2 files changed, 85 insertions(+)
>>>
[...]
>>> + coherent = of_dma_is_coherent(parent_np);
>>> + dev_dbg(dev, "device is%sdma coherent\n",
>>> + coherent ? " " : " not ");
>>> +
>>> + arch_setup_dma_ops(dev, dma_addr, size, NULL, coherent);
>>
>>
>> This is the same code as of_dma_configure. The only difference I see
>> is which node ptr is passed to of_dma_get_range. You need to make that
>> a function param of of_dma_configure.
>>
>> of_dma_configure also has iommu handling now. You will probably need
>> something similar for PCI in that you setup an iommu based on the root
>> bus DT properties.
>>
> Initially I had the same idea to re-use the existing function
> of_dma_configure() for this. I wanted to defer this until we have an
> agreement on the changes required for the subject functionality. My quick
> review of the code suggestio this would require additional API changes as
> below. I did a quick test of the changes and it works for Keystone, but need
> to be reviewed by everyone as I touch the IOMMU functionality here and I
> don't have a platform with IOMMU. Need test by someone to make sure I don't
> break anything.
The IOMMU changes look trivial. We may want to address the comment in
of_iommu_configure about parent nodes. We should be sure these changes
work with how we would do searching parent nodes.
> Here are the changes required to implement this suggestion.
>
> 1. Move the of_dma_configure() to drivers/of/device.c (include the API in
> of_device.h) and make it global (using proper EXPORT macro). Otherwise, we
> will have to include of_platform.h in drivers/of/of_pci.c assuming the
> prototype is defined in of_platform.h which doesn't look appropriate to me.
> Would require following additional include files in drivers/of/device.c as
> well.
>
> +#include <linux/of_address.h>
> +#include <linux/of_iommu.h>
> +#include <linux/dma-mapping.h>
Okay.
> 2. drivers/iommu/of_iommu.c. This is needed so that of_iommu_configure() can
> take confuguration from the root bus DT as you have suggested.
>
> -struct iommu_ops *of_iommu_configure(struct device *dev)
> +struct iommu_ops *of_iommu_configure(struct device *dev, struct device_node
> *node)
> {
> struct of_phandle_args iommu_spec;
> struct device_node *np;
> @@ -145,7 +145,7 @@ struct iommu_ops *of_iommu_configure(struct device *dev)
> * See the `Notes:' section of
> * Documentation/devicetree/bindings/iommu/iommu.txt
> */
> - while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> + while (!of_parse_phandle_with_args(node, "iommus",
> "#iommu-cells", idx,
> &iommu_spec)) {
>
Seems safe enough.
> 3. drivers/of/of_pci.c. The existing function (added in this patch) will
> make call to of_dma_configure() as
>
> parent_np = of_get_pci_root_bridge_parent(pci_dev);
> of_dma_configure(dev, parent_np);
Okay.
> 4. drivers/of/platform.c. Add a wrapper function of_platform_dma_configure()
> that calls of_dma_configure() as
> of_dma_configure(dev, dev->of_node). All existing calls converted to this
> wrapper.
There's only a few callers of of_dma_configure, so I don't think this
is necessary. The only thing platform bus specific is who is calling
the function.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
