| Message ID | 1422052359-12384-2-git-send-email-m-karicheri2@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Murali, Thank you for the patch. On Friday 23 January 2015 17:32:34 Murali Karicheri wrote: > Function of_iommu_configure() is called from of_dma_configure() to > setup iommu ops using DT property. This API is currently used for > platform devices for which DMA configuration (including iommu ops) > may come from device's parent. To extend this functionality for PCI > devices, this API need to take a parent node ptr as an argument > instead of assuming device's parent. This is needed since for PCI, the > dma configuration may be defined in the DT node of the root bus bridge's > parent device. Currently only dma-range is used for PCI and iommu is not > supported. So return error if the device is PCI. > > Cc: Joerg Roedel <joro@8bytes.org> > Cc: Grant Likely <grant.likely@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > > Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > --- > drivers/iommu/of_iommu.c | 10 ++++++++-- > drivers/of/platform.c | 2 +- > include/linux/of_iommu.h | 6 ++++-- > 3 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index af1dc6a..439235b 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct device_node > *np) return ops; > } > > -struct iommu_ops *of_iommu_configure(struct device *dev) > +struct iommu_ops *of_iommu_configure(struct device *dev, > + struct device_node *iommu_np) > { > struct of_phandle_args iommu_spec; > struct device_node *np; > struct iommu_ops *ops = NULL; > int idx = 0; > > + if (dev_is_pci(dev)) { > + dev_err(dev, "iommu is currently not supported for PCI\n"); > + return NULL; > + } > + > /* > * We don't currently walk up the tree looking for a parent IOMMU. > * See the `Notes:' section of > * Documentation/devicetree/bindings/iommu/iommu.txt > */ Wouldn't it be better to fix this rather than passing the device node pointer to the function ? The solution would be more generic. > - while (!of_parse_phandle_with_args(dev->of_node, "iommus", > + while (!of_parse_phandle_with_args(iommu_np, "iommus", > "#iommu-cells", idx, > &iommu_spec)) { > np = iommu_spec.np; > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index a54ec10..7675b79 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -196,7 +196,7 @@ static void of_dma_configure(struct device *dev) > dev_dbg(dev, "device is%sdma coherent\n", > coherent ? " " : " not "); > > - iommu = of_iommu_configure(dev); > + iommu = of_iommu_configure(dev, dev->of_node); > dev_dbg(dev, "device is%sbehind an iommu\n", > iommu ? " " : " not "); > > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h > index 16c7554..a97e5bd 100644 > --- a/include/linux/of_iommu.h > +++ b/include/linux/of_iommu.h > @@ -12,7 +12,8 @@ extern int of_get_dma_window(struct device_node *dn, const > char *prefix, size_t *size); > > extern void of_iommu_init(void); > -extern struct iommu_ops *of_iommu_configure(struct device *dev); > +extern struct iommu_ops *of_iommu_configure(struct device *dev, > + struct device_node *iommu_np); > > #else > > @@ -24,7 +25,8 @@ static inline int of_get_dma_window(struct device_node > *dn, const char *prefix, } > > static inline void of_iommu_init(void) { } > -static inline struct iommu_ops *of_iommu_configure(struct device *dev) > +static inline struct iommu_ops *of_iommu_configure(struct device *dev, > + struct device_node *iommu_np) > { > return NULL; > }
On 01/25/2015 08:32 AM, Laurent Pinchart wrote: > Hi Murali, > > Thank you for the patch. > > On Friday 23 January 2015 17:32:34 Murali Karicheri wrote: >> Function of_iommu_configure() is called from of_dma_configure() to >> setup iommu ops using DT property. This API is currently used for >> platform devices for which DMA configuration (including iommu ops) >> may come from device's parent. To extend this functionality for PCI >> devices, this API need to take a parent node ptr as an argument >> instead of assuming device's parent. This is needed since for PCI, the >> dma configuration may be defined in the DT node of the root bus bridge's >> parent device. Currently only dma-range is used for PCI and iommu is not >> supported. So return error if the device is PCI. >> >> Cc: Joerg Roedel<joro@8bytes.org> >> Cc: Grant Likely<grant.likely@linaro.org> >> Cc: Rob Herring<robh+dt@kernel.org> >> Cc: Bjorn Helgaas<bhelgaas@google.com> >> Cc: Will Deacon<will.deacon@arm.com> >> Cc: Russell King<linux@arm.linux.org.uk> >> Cc: Arnd Bergmann<arnd@arndb.de> >> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com> >> >> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> >> --- >> drivers/iommu/of_iommu.c | 10 ++++++++-- >> drivers/of/platform.c | 2 +- >> include/linux/of_iommu.h | 6 ++++-- >> 3 files changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c >> index af1dc6a..439235b 100644 >> --- a/drivers/iommu/of_iommu.c >> +++ b/drivers/iommu/of_iommu.c >> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct device_node >> *np) return ops; >> } >> >> -struct iommu_ops *of_iommu_configure(struct device *dev) >> +struct iommu_ops *of_iommu_configure(struct device *dev, >> + struct device_node *iommu_np) >> { >> struct of_phandle_args iommu_spec; >> struct device_node *np; >> struct iommu_ops *ops = NULL; >> int idx = 0; >> >> + if (dev_is_pci(dev)) { >> + dev_err(dev, "iommu is currently not supported for PCI\n"); >> + return NULL; >> + } >> + >> /* >> * We don't currently walk up the tree looking for a parent IOMMU. >> * See the `Notes:' section of >> * Documentation/devicetree/bindings/iommu/iommu.txt >> */ > Wouldn't it be better to fix this rather than passing the device node pointer > to the function ? The solution would be more generic. Laurent, Will Deacon (Copied here) is working on this as we alteady discussed in this thread. So it will be addressed by him in another series. Murali > >> - while (!of_parse_phandle_with_args(dev->of_node, "iommus", >> + while (!of_parse_phandle_with_args(iommu_np, "iommus", >> "#iommu-cells", idx, >> &iommu_spec)) { >> np = iommu_spec.np; >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c >> index a54ec10..7675b79 100644 >> --- a/drivers/of/platform.c >> +++ b/drivers/of/platform.c >> @@ -196,7 +196,7 @@ static void of_dma_configure(struct device *dev) >> dev_dbg(dev, "device is%sdma coherent\n", >> coherent ? " " : " not "); >> >> - iommu = of_iommu_configure(dev); >> + iommu = of_iommu_configure(dev, dev->of_node); >> dev_dbg(dev, "device is%sbehind an iommu\n", >> iommu ? " " : " not "); >> >> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h >> index 16c7554..a97e5bd 100644 >> --- a/include/linux/of_iommu.h >> +++ b/include/linux/of_iommu.h >> @@ -12,7 +12,8 @@ extern int of_get_dma_window(struct device_node *dn, const >> char *prefix, size_t *size); >> >> extern void of_iommu_init(void); >> -extern struct iommu_ops *of_iommu_configure(struct device *dev); >> +extern struct iommu_ops *of_iommu_configure(struct device *dev, >> + struct device_node *iommu_np); >> >> #else >> >> @@ -24,7 +25,8 @@ static inline int of_get_dma_window(struct device_node >> *dn, const char *prefix, } >> >> static inline void of_iommu_init(void) { } >> -static inline struct iommu_ops *of_iommu_configure(struct device *dev) >> +static inline struct iommu_ops *of_iommu_configure(struct device *dev, >> + struct device_node *iommu_np) >> { >> return NULL; >> }
On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote: > On 01/25/2015 08:32 AM, Laurent Pinchart wrote: > > Hi Murali, > > > > Thank you for the patch. > > > > On Friday 23 January 2015 17:32:34 Murali Karicheri wrote: > >> Function of_iommu_configure() is called from of_dma_configure() to > >> setup iommu ops using DT property. This API is currently used for > >> platform devices for which DMA configuration (including iommu ops) > >> may come from device's parent. To extend this functionality for PCI > >> devices, this API need to take a parent node ptr as an argument > >> instead of assuming device's parent. This is needed since for PCI, the > >> dma configuration may be defined in the DT node of the root bus bridge's > >> parent device. Currently only dma-range is used for PCI and iommu is not > >> supported. So return error if the device is PCI. > >> > >> Cc: Joerg Roedel<joro@8bytes.org> > >> Cc: Grant Likely<grant.likely@linaro.org> > >> Cc: Rob Herring<robh+dt@kernel.org> > >> Cc: Bjorn Helgaas<bhelgaas@google.com> > >> Cc: Will Deacon<will.deacon@arm.com> > >> Cc: Russell King<linux@arm.linux.org.uk> > >> Cc: Arnd Bergmann<arnd@arndb.de> > >> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com> > >> > >> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> > >> --- > >> drivers/iommu/of_iommu.c | 10 ++++++++-- > >> drivers/of/platform.c | 2 +- > >> include/linux/of_iommu.h | 6 ++++-- > >> 3 files changed, 13 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > >> index af1dc6a..439235b 100644 > >> --- a/drivers/iommu/of_iommu.c > >> +++ b/drivers/iommu/of_iommu.c > >> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct device_node > >> *np) return ops; > >> } > >> > >> -struct iommu_ops *of_iommu_configure(struct device *dev) > >> +struct iommu_ops *of_iommu_configure(struct device *dev, > >> + struct device_node *iommu_np) > >> { > >> struct of_phandle_args iommu_spec; > >> struct device_node *np; > >> struct iommu_ops *ops = NULL; > >> int idx = 0; > >> > >> + if (dev_is_pci(dev)) { > >> + dev_err(dev, "iommu is currently not supported for PCI\n"); > >> + return NULL; > >> + } > >> + > >> /* > >> * We don't currently walk up the tree looking for a parent IOMMU. > >> * See the `Notes:' section of > >> * Documentation/devicetree/bindings/iommu/iommu.txt > >> */ > > Wouldn't it be better to fix this rather than passing the device node pointer > > to the function ? The solution would be more generic. > Laurent, > > Will Deacon (Copied here) is working on this as we alteady discussed in > this thread. So it will be > addressed by him in another series. Well, "working on this" equates to "has it somewhere near the bottom of a very long list" :) Will
Hi Will, On Wednesday 28 January 2015 11:33:00 Will Deacon wrote: > On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote: > > On 01/25/2015 08:32 AM, Laurent Pinchart wrote: > >> On Friday 23 January 2015 17:32:34 Murali Karicheri wrote: > >>> Function of_iommu_configure() is called from of_dma_configure() to > >>> setup iommu ops using DT property. This API is currently used for > >>> platform devices for which DMA configuration (including iommu ops) > >>> may come from device's parent. To extend this functionality for PCI > >>> devices, this API need to take a parent node ptr as an argument > >>> instead of assuming device's parent. This is needed since for PCI, the > >>> dma configuration may be defined in the DT node of the root bus > >>> bridge's parent device. Currently only dma-range is used for PCI and > >>> iommu is not supported. So return error if the device is PCI. > >>> > >>> Cc: Joerg Roedel<joro@8bytes.org> > >>> Cc: Grant Likely<grant.likely@linaro.org> > >>> Cc: Rob Herring<robh+dt@kernel.org> > >>> Cc: Bjorn Helgaas<bhelgaas@google.com> > >>> Cc: Will Deacon<will.deacon@arm.com> > >>> Cc: Russell King<linux@arm.linux.org.uk> > >>> Cc: Arnd Bergmann<arnd@arndb.de> > >>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com> > >>> > >>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> > >>> --- > >>> > >>> drivers/iommu/of_iommu.c | 10 ++++++++-- > >>> drivers/of/platform.c | 2 +- > >>> include/linux/of_iommu.h | 6 ++++-- > >>> 3 files changed, 13 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > >>> index af1dc6a..439235b 100644 > >>> --- a/drivers/iommu/of_iommu.c > >>> +++ b/drivers/iommu/of_iommu.c > >>> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct > >>> device_node *np) > >>> return ops; > >>> } > >>> > >>> -struct iommu_ops *of_iommu_configure(struct device *dev) > >>> +struct iommu_ops *of_iommu_configure(struct device *dev, > >>> + struct device_node *iommu_np) > >>> { > >>> struct of_phandle_args iommu_spec; > >>> struct device_node *np; > >>> struct iommu_ops *ops = NULL; > >>> int idx = 0; > >>> > >>> + if (dev_is_pci(dev)) { > >>> + dev_err(dev, "iommu is currently not supported for PCI\n"); > >>> + return NULL; > >>> + } > >>> + > >>> /* > >>> * We don't currently walk up the tree looking for a parent IOMMU. > >>> * See the `Notes:' section of > >>> * Documentation/devicetree/bindings/iommu/iommu.txt > >>> */ > >> > >> Wouldn't it be better to fix this rather than passing the device node > >> pointer to the function ? The solution would be more generic. > > > > Laurent, > > > > Will Deacon (Copied here) is working on this as we alteady discussed in > > this thread. So it will be addressed by him in another series. > > Well, "working on this" equates to "has it somewhere near the bottom of > a very long list" :) What's your opinion on this patch then, do you think adding the iommu_np argument makes sense as a temporary hack, or should we instead walk up the tree looking for an iommus attribute in parent nodes ? I don't expect that to be insanely difficult to code.
On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote: > On Wednesday 28 January 2015 11:33:00 Will Deacon wrote: > > On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote: > > > On 01/25/2015 08:32 AM, Laurent Pinchart wrote: > > >> On Friday 23 January 2015 17:32:34 Murali Karicheri wrote: > > >>> Function of_iommu_configure() is called from of_dma_configure() to > > >>> setup iommu ops using DT property. This API is currently used for > > >>> platform devices for which DMA configuration (including iommu ops) > > >>> may come from device's parent. To extend this functionality for PCI > > >>> devices, this API need to take a parent node ptr as an argument > > >>> instead of assuming device's parent. This is needed since for PCI, the > > >>> dma configuration may be defined in the DT node of the root bus > > >>> bridge's parent device. Currently only dma-range is used for PCI and > > >>> iommu is not supported. So return error if the device is PCI. > > >>> > > >>> Cc: Joerg Roedel<joro@8bytes.org> > > >>> Cc: Grant Likely<grant.likely@linaro.org> > > >>> Cc: Rob Herring<robh+dt@kernel.org> > > >>> Cc: Bjorn Helgaas<bhelgaas@google.com> > > >>> Cc: Will Deacon<will.deacon@arm.com> > > >>> Cc: Russell King<linux@arm.linux.org.uk> > > >>> Cc: Arnd Bergmann<arnd@arndb.de> > > >>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com> > > >>> > > >>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> > > >>> --- > > >>> > > >>> drivers/iommu/of_iommu.c | 10 ++++++++-- > > >>> drivers/of/platform.c | 2 +- > > >>> include/linux/of_iommu.h | 6 ++++-- > > >>> 3 files changed, 13 insertions(+), 5 deletions(-) > > >>> > > >>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > > >>> index af1dc6a..439235b 100644 > > >>> --- a/drivers/iommu/of_iommu.c > > >>> +++ b/drivers/iommu/of_iommu.c > > >>> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct > > >>> device_node *np) > > >>> return ops; > > >>> } > > >>> > > >>> -struct iommu_ops *of_iommu_configure(struct device *dev) > > >>> +struct iommu_ops *of_iommu_configure(struct device *dev, > > >>> + struct device_node *iommu_np) > > >>> { > > >>> struct of_phandle_args iommu_spec; > > >>> struct device_node *np; > > >>> struct iommu_ops *ops = NULL; > > >>> int idx = 0; > > >>> > > >>> + if (dev_is_pci(dev)) { > > >>> + dev_err(dev, "iommu is currently not supported for PCI\n"); > > >>> + return NULL; > > >>> + } > > >>> + > > >>> /* > > >>> * We don't currently walk up the tree looking for a parent IOMMU. > > >>> * See the `Notes:' section of > > >>> * Documentation/devicetree/bindings/iommu/iommu.txt > > >>> */ > > >> > > >> Wouldn't it be better to fix this rather than passing the device node > > >> pointer to the function ? The solution would be more generic. > > > > > > Will Deacon (Copied here) is working on this as we alteady discussed in > > > this thread. So it will be addressed by him in another series. > > > > Well, "working on this" equates to "has it somewhere near the bottom of > > a very long list" :) > > What's your opinion on this patch then, do you think adding the iommu_np > argument makes sense as a temporary hack, or should we instead walk up the > tree looking for an iommus attribute in parent nodes ? I don't expect that to > be insanely difficult to code. If walking up the tree is useful, then I think we should do that and update the Documentation to reflect the new behaviour. The only reason that I didn't code that in of_iommu_configure originally is because I didn't want to go against the binding spec for the initial version, especially as we didn't have a reason to look at parent nodes. The only thing to double-check is that we don't break any existing users of the binding with this change, but I don't think that we do. Will
On Wednesday 28 January 2015 12:29:42 Will Deacon wrote: > On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote: > > On Wednesday 28 January 2015 11:33:00 Will Deacon wrote: > >> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote: > >>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote: > >>>> On Friday 23 January 2015 17:32:34 Murali Karicheri wrote: > >>>>> Function of_iommu_configure() is called from of_dma_configure() to > >>>>> setup iommu ops using DT property. This API is currently used for > >>>>> platform devices for which DMA configuration (including iommu ops) > >>>>> may come from device's parent. To extend this functionality for PCI > >>>>> devices, this API need to take a parent node ptr as an argument > >>>>> instead of assuming device's parent. This is needed since for PCI, > >>>>> the dma configuration may be defined in the DT node of the root bus > >>>>> bridge's parent device. Currently only dma-range is used for PCI and > >>>>> iommu is not supported. So return error if the device is PCI. > >>>>> > >>>>> Cc: Joerg Roedel<joro@8bytes.org> > >>>>> Cc: Grant Likely<grant.likely@linaro.org> > >>>>> Cc: Rob Herring<robh+dt@kernel.org> > >>>>> Cc: Bjorn Helgaas<bhelgaas@google.com> > >>>>> Cc: Will Deacon<will.deacon@arm.com> > >>>>> Cc: Russell King<linux@arm.linux.org.uk> > >>>>> Cc: Arnd Bergmann<arnd@arndb.de> > >>>>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com> > >>>>> > >>>>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> > >>>>> --- > >>>>> > >>>>> drivers/iommu/of_iommu.c | 10 ++++++++-- > >>>>> drivers/of/platform.c | 2 +- > >>>>> include/linux/of_iommu.h | 6 ++++-- > >>>>> 3 files changed, 13 insertions(+), 5 deletions(-) > >>>>> > >>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > >>>>> index af1dc6a..439235b 100644 > >>>>> --- a/drivers/iommu/of_iommu.c > >>>>> +++ b/drivers/iommu/of_iommu.c > >>>>> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct > >>>>> device_node *np) > >>>>> return ops; > >>>>> } > >>>>> > >>>>> -struct iommu_ops *of_iommu_configure(struct device *dev) > >>>>> +struct iommu_ops *of_iommu_configure(struct device *dev, > >>>>> + struct device_node *iommu_np) > >>>>> { > >>>>> struct of_phandle_args iommu_spec; > >>>>> struct device_node *np; > >>>>> struct iommu_ops *ops = NULL; > >>>>> int idx = 0; > >>>>> > >>>>> + if (dev_is_pci(dev)) { > >>>>> + dev_err(dev, "iommu is currently not supported for PCI\n"); > >>>>> + return NULL; > >>>>> + } > >>>>> + > >>>>> /* > >>>>> * We don't currently walk up the tree looking for a parent > >>>>> IOMMU. > >>>>> * See the `Notes:' section of > >>>>> * Documentation/devicetree/bindings/iommu/iommu.txt > >>>>> */ > >>>> > >>>> Wouldn't it be better to fix this rather than passing the device node > >>>> pointer to the function ? The solution would be more generic. > >>> > >>> Will Deacon (Copied here) is working on this as we alteady discussed > >>> in this thread. So it will be addressed by him in another series. > >> > >> Well, "working on this" equates to "has it somewhere near the bottom of > >> a very long list" :) > > > > What's your opinion on this patch then, do you think adding the iommu_np > > argument makes sense as a temporary hack, or should we instead walk up the > > tree looking for an iommus attribute in parent nodes ? I don't expect that > > to be insanely difficult to code. > > If walking up the tree is useful, then I think we should do that and update > the Documentation to reflect the new behaviour. If I understand Murali's patch set right (please correct me if that's not the case) the PCI code walks up the DT nodes hierarchy to the parent node that contains the iommus attribute and passes a pointer to that node to this function. It's thus a PCI-specific solution. As a temporary hack that's OK I suppose, but if implementing it right straight away isn't difficult that would be better. > The only reason that I didn't code that in of_iommu_configure originally is > because I didn't want to go against the binding spec for the initial > version, especially as we didn't have a reason to look at parent nodes. > > The only thing to double-check is that we don't break any existing users > of the binding with this change, but I don't think that we do.
On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote: > On Wednesday 28 January 2015 12:29:42 Will Deacon wrote: > > On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote: > > > On Wednesday 28 January 2015 11:33:00 Will Deacon wrote: > > >> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote: > > >>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote: > > >>>> On Friday 23 January 2015 17:32:34 Murali Karicheri wrote: > > >>>>> Function of_iommu_configure() is called from of_dma_configure() to > > >>>>> setup iommu ops using DT property. This API is currently used for > > >>>>> platform devices for which DMA configuration (including iommu ops) > > >>>>> may come from device's parent. To extend this functionality for PCI > > >>>>> devices, this API need to take a parent node ptr as an argument > > >>>>> instead of assuming device's parent. This is needed since for PCI, > > >>>>> the dma configuration may be defined in the DT node of the root bus > > >>>>> bridge's parent device. Currently only dma-range is used for PCI and > > >>>>> iommu is not supported. So return error if the device is PCI. > > >>>>> > > >>>>> Cc: Joerg Roedel<joro@8bytes.org> > > >>>>> Cc: Grant Likely<grant.likely@linaro.org> > > >>>>> Cc: Rob Herring<robh+dt@kernel.org> > > >>>>> Cc: Bjorn Helgaas<bhelgaas@google.com> > > >>>>> Cc: Will Deacon<will.deacon@arm.com> > > >>>>> Cc: Russell King<linux@arm.linux.org.uk> > > >>>>> Cc: Arnd Bergmann<arnd@arndb.de> > > >>>>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com> > > >>>>> > > >>>>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> > > >>>>> --- > > >>>>> > > >>>>> drivers/iommu/of_iommu.c | 10 ++++++++-- > > >>>>> drivers/of/platform.c | 2 +- > > >>>>> include/linux/of_iommu.h | 6 ++++-- > > >>>>> 3 files changed, 13 insertions(+), 5 deletions(-) > > >>>>> > > >>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > > >>>>> index af1dc6a..439235b 100644 > > >>>>> --- a/drivers/iommu/of_iommu.c > > >>>>> +++ b/drivers/iommu/of_iommu.c > > >>>>> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct > > >>>>> device_node *np) > > >>>>> return ops; > > >>>>> } > > >>>>> > > >>>>> -struct iommu_ops *of_iommu_configure(struct device *dev) > > >>>>> +struct iommu_ops *of_iommu_configure(struct device *dev, > > >>>>> + struct device_node *iommu_np) > > >>>>> { > > >>>>> struct of_phandle_args iommu_spec; > > >>>>> struct device_node *np; > > >>>>> struct iommu_ops *ops = NULL; > > >>>>> int idx = 0; > > >>>>> > > >>>>> + if (dev_is_pci(dev)) { > > >>>>> + dev_err(dev, "iommu is currently not supported for PCI\n"); > > >>>>> + return NULL; > > >>>>> + } > > >>>>> + > > >>>>> /* > > >>>>> * We don't currently walk up the tree looking for a parent > > >>>>> IOMMU. > > >>>>> * See the `Notes:' section of > > >>>>> * Documentation/devicetree/bindings/iommu/iommu.txt > > >>>>> */ > > >>>> > > >>>> Wouldn't it be better to fix this rather than passing the device node > > >>>> pointer to the function ? The solution would be more generic. > > >>> > > >>> Will Deacon (Copied here) is working on this as we alteady discussed > > >>> in this thread. So it will be addressed by him in another series. > > >> > > >> Well, "working on this" equates to "has it somewhere near the bottom of > > >> a very long list" :) > > > > > > What's your opinion on this patch then, do you think adding the iommu_np > > > argument makes sense as a temporary hack, or should we instead walk up the > > > tree looking for an iommus attribute in parent nodes ? I don't expect that > > > to be insanely difficult to code. > > > > If walking up the tree is useful, then I think we should do that and update > > the Documentation to reflect the new behaviour. > > If I understand Murali's patch set right (please correct me if that's not the > case) the PCI code walks up the DT nodes hierarchy to the parent node that > contains the iommus attribute and passes a pointer to that node to this > function. It's thus a PCI-specific solution. As a temporary hack that's OK I > suppose, but if implementing it right straight away isn't difficult that would > be better. It looks to me like the code walks the PCI topology to get the DT node for the host controller, and passes *that* to of_dma_configure. That sounds like the right thing to do to me, especially since the PCI topology is likely not encoded in the device-tree. So actually, it is passing the first parent node afaict. The part I'm more interested in is the mapping of RequesterID (PCI dma alias) to IOMMU ID when we transition from the PCI topology to the DT topology. Currently, it seems to be 1:1 on the platforms I have, but I doubt this will always be the case. Will
On 01/28/2015 08:32 AM, Will Deacon wrote: > On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote: >> On Wednesday 28 January 2015 12:29:42 Will Deacon wrote: >>> On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote: >>>> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote: >>>>> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote: >>>>>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote: >>>>>>> On Friday 23 January 2015 17:32:34 Murali Karicheri wrote: >>>>>>>> Function of_iommu_configure() is called from of_dma_configure() to >>>>>>>> setup iommu ops using DT property. This API is currently used for >>>>>>>> platform devices for which DMA configuration (including iommu ops) >>>>>>>> may come from device's parent. To extend this functionality for PCI >>>>>>>> devices, this API need to take a parent node ptr as an argument >>>>>>>> instead of assuming device's parent. This is needed since for PCI, >>>>>>>> the dma configuration may be defined in the DT node of the root bus >>>>>>>> bridge's parent device. Currently only dma-range is used for PCI and >>>>>>>> iommu is not supported. So return error if the device is PCI. >>>>>>>> >>>>>>>> Cc: Joerg Roedel<joro@8bytes.org> >>>>>>>> Cc: Grant Likely<grant.likely@linaro.org> >>>>>>>> Cc: Rob Herring<robh+dt@kernel.org> >>>>>>>> Cc: Bjorn Helgaas<bhelgaas@google.com> >>>>>>>> Cc: Will Deacon<will.deacon@arm.com> >>>>>>>> Cc: Russell King<linux@arm.linux.org.uk> >>>>>>>> Cc: Arnd Bergmann<arnd@arndb.de> >>>>>>>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com> >>>>>>>> >>>>>>>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> >>>>>>>> --- >>>>>>>> >>>>>>>> drivers/iommu/of_iommu.c | 10 ++++++++-- >>>>>>>> drivers/of/platform.c | 2 +- >>>>>>>> include/linux/of_iommu.h | 6 ++++-- >>>>>>>> 3 files changed, 13 insertions(+), 5 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c >>>>>>>> index af1dc6a..439235b 100644 >>>>>>>> --- a/drivers/iommu/of_iommu.c >>>>>>>> +++ b/drivers/iommu/of_iommu.c >>>>>>>> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct >>>>>>>> device_node *np) >>>>>>>> return ops; >>>>>>>> } >>>>>>>> >>>>>>>> -struct iommu_ops *of_iommu_configure(struct device *dev) >>>>>>>> +struct iommu_ops *of_iommu_configure(struct device *dev, >>>>>>>> + struct device_node *iommu_np) >>>>>>>> { >>>>>>>> struct of_phandle_args iommu_spec; >>>>>>>> struct device_node *np; >>>>>>>> struct iommu_ops *ops = NULL; >>>>>>>> int idx = 0; >>>>>>>> >>>>>>>> + if (dev_is_pci(dev)) { >>>>>>>> + dev_err(dev, "iommu is currently not supported for PCI\n"); >>>>>>>> + return NULL; >>>>>>>> + } >>>>>>>> + >>>>>>>> /* >>>>>>>> * We don't currently walk up the tree looking for a parent >>>>>>>> IOMMU. >>>>>>>> * See the `Notes:' section of >>>>>>>> * Documentation/devicetree/bindings/iommu/iommu.txt >>>>>>>> */ >>>>>>> >>>>>>> Wouldn't it be better to fix this rather than passing the device node >>>>>>> pointer to the function ? The solution would be more generic. >>>>>> >>>>>> Will Deacon (Copied here) is working on this as we alteady discussed >>>>>> in this thread. So it will be addressed by him in another series. >>>>> >>>>> Well, "working on this" equates to "has it somewhere near the bottom of >>>>> a very long list" :) >>>> >>>> What's your opinion on this patch then, do you think adding the iommu_np >>>> argument makes sense as a temporary hack, or should we instead walk up the >>>> tree looking for an iommus attribute in parent nodes ? I don't expect that >>>> to be insanely difficult to code. >>> >>> If walking up the tree is useful, then I think we should do that and update >>> the Documentation to reflect the new behaviour. >> >> If I understand Murali's patch set right (please correct me if that's not the >> case) the PCI code walks up the DT nodes hierarchy to the parent node that >> contains the iommus attribute and passes a pointer to that node to this >> function. It's thus a PCI-specific solution. As a temporary hack that's OK I >> suppose, but if implementing it right straight away isn't difficult that would >> be better. > > It looks to me like the code walks the PCI topology to get the DT node for > the host controller, and passes *that* to of_dma_configure. That sounds > like the right thing to do to me, especially since the PCI topology is > likely not encoded in the device-tree. So actually, it is passing the > first parent node afaict. > Laurent, Will, I don't have an IOMMU hardware to do more work on this. My patch series has been on this list for long and it is also increasing in scope :( I would suggest a follow up patch to this from someone (probably Will) and my patch goes as is with out further update. Hope this is reasonable. Murali > The part I'm more interested in is the mapping of RequesterID (PCI dma > alias) to IOMMU ID when we transition from the PCI topology to the DT > topology. Currently, it seems to be 1:1 on the platforms I have, but I > doubt this will always be the case. > > Will
Hi Will, On Wednesday 28 January 2015 13:32:19 Will Deacon wrote: > On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote: > > On Wednesday 28 January 2015 12:29:42 Will Deacon wrote: > >> On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote: > >>> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote: > >>>> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote: > >>>>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote: > >>>>>> On Friday 23 January 2015 17:32:34 Murali Karicheri wrote: > >>>>>>> Function of_iommu_configure() is called from of_dma_configure() to > >>>>>>> setup iommu ops using DT property. This API is currently used for > >>>>>>> platform devices for which DMA configuration (including iommu ops) > >>>>>>> may come from device's parent. To extend this functionality for > >>>>>>> PCI devices, this API need to take a parent node ptr as an argument > >>>>>>> instead of assuming device's parent. This is needed since for PCI, > >>>>>>> the dma configuration may be defined in the DT node of the root > >>>>>>> bus bridge's parent device. Currently only dma-range is used for PCI > >>>>>>> and iommu is not supported. So return error if the device is PCI. > >>>>>>> > >>>>>>> Cc: Joerg Roedel<joro@8bytes.org> > >>>>>>> Cc: Grant Likely<grant.likely@linaro.org> > >>>>>>> Cc: Rob Herring<robh+dt@kernel.org> > >>>>>>> Cc: Bjorn Helgaas<bhelgaas@google.com> > >>>>>>> Cc: Will Deacon<will.deacon@arm.com> > >>>>>>> Cc: Russell King<linux@arm.linux.org.uk> > >>>>>>> Cc: Arnd Bergmann<arnd@arndb.de> > >>>>>>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com> > >>>>>>> > >>>>>>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> > >>>>>>> --- > >>>>>>> > >>>>>>> drivers/iommu/of_iommu.c | 10 ++++++++-- > >>>>>>> drivers/of/platform.c | 2 +- > >>>>>>> include/linux/of_iommu.h | 6 ++++-- > >>>>>>> 3 files changed, 13 insertions(+), 5 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > >>>>>>> index af1dc6a..439235b 100644 > >>>>>>> --- a/drivers/iommu/of_iommu.c > >>>>>>> +++ b/drivers/iommu/of_iommu.c > >>>>>>> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct > >>>>>>> device_node *np) > >>>>>>> return ops; > >>>>>>> } > >>>>>>> > >>>>>>> -struct iommu_ops *of_iommu_configure(struct device *dev) > >>>>>>> +struct iommu_ops *of_iommu_configure(struct device *dev, > >>>>>>> + struct device_node *iommu_np) > >>>>>>> { > >>>>>>> struct of_phandle_args iommu_spec; > >>>>>>> struct device_node *np; > >>>>>>> struct iommu_ops *ops = NULL; > >>>>>>> int idx = 0; > >>>>>>> > >>>>>>> + if (dev_is_pci(dev)) { > >>>>>>> + dev_err(dev, "iommu is currently not supported for PCI\n"); > >>>>>>> + return NULL; > >>>>>>> + } > >>>>>>> + > >>>>>>> /* > >>>>>>> * We don't currently walk up the tree looking for a parent > >>>>>>> IOMMU. > >>>>>>> * See the `Notes:' section of > >>>>>>> * Documentation/devicetree/bindings/iommu/iommu.txt > >>>>>>> */ > >>>>>> > >>>>>> Wouldn't it be better to fix this rather than passing the device > >>>>>> node pointer to the function ? The solution would be more generic. > >>>>> > >>>>> Will Deacon (Copied here) is working on this as we alteady discussed > >>>>> in this thread. So it will be addressed by him in another series. > >>>> > >>>> Well, "working on this" equates to "has it somewhere near the bottom > >>>> of a very long list" :) > >>> > >>> What's your opinion on this patch then, do you think adding the > >>> iommu_np argument makes sense as a temporary hack, or should we instead > >>> walk up the tree looking for an iommus attribute in parent nodes ? I > >>> don't expect that to be insanely difficult to code. > >> > >> If walking up the tree is useful, then I think we should do that and > >> update the Documentation to reflect the new behaviour. > > > > If I understand Murali's patch set right (please correct me if that's not > > the case) the PCI code walks up the DT nodes hierarchy to the parent node > > that contains the iommus attribute and passes a pointer to that node to > > this function. It's thus a PCI-specific solution. As a temporary hack > > that's OK I suppose, but if implementing it right straight away isn't > > difficult that would be better. > > It looks to me like the code walks the PCI topology to get the DT node for > the host controller, and passes *that* to of_dma_configure. That sounds > like the right thing to do to me, especially since the PCI topology is > likely not encoded in the device-tree. So actually, it is passing the > first parent node afaict. Indeed, that's right. I forgot for a moment that we have non-DT devices ;-) Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Murali, nitpicking a bit, shouldn't the iommu_np parameter be renamed ? It points to the node containing the iommus parameter, not to the iommu node, so the current name is slightly confusing. A brief kerneldoc above the function would also help. This can be the subject of a separate patch. > The part I'm more interested in is the mapping of RequesterID (PCI dma > alias) to IOMMU ID when we transition from the PCI topology to the DT > topology. Currently, it seems to be 1:1 on the platforms I have, but I > doubt this will always be the case.
On 01/28/2015 06:32 PM, Laurent Pinchart wrote: > Hi Will, > > On Wednesday 28 January 2015 13:32:19 Will Deacon wrote: >> On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote: >>> On Wednesday 28 January 2015 12:29:42 Will Deacon wrote: >>>> On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote: >>>>> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote: >>>>>> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote: >>>>>>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote: >>>>>>>> On Friday 23 January 2015 17:32:34 Murali Karicheri wrote: >>>>>>>>> Function of_iommu_configure() is called from of_dma_configure() to >>>>>>>>> setup iommu ops using DT property. This API is currently used for >>>>>>>>> platform devices for which DMA configuration (including iommu ops) >>>>>>>>> may come from device's parent. To extend this functionality for >>>>>>>>> PCI devices, this API need to take a parent node ptr as an argument >>>>>>>>> instead of assuming device's parent. This is needed since for PCI, >>>>>>>>> the dma configuration may be defined in the DT node of the root >>>>>>>>> bus bridge's parent device. Currently only dma-range is used for PCI >>>>>>>>> and iommu is not supported. So return error if the device is PCI. >>>>>>>>> >>>>>>>>> Cc: Joerg Roedel<joro@8bytes.org> >>>>>>>>> Cc: Grant Likely<grant.likely@linaro.org> >>>>>>>>> Cc: Rob Herring<robh+dt@kernel.org> >>>>>>>>> Cc: Bjorn Helgaas<bhelgaas@google.com> >>>>>>>>> Cc: Will Deacon<will.deacon@arm.com> >>>>>>>>> Cc: Russell King<linux@arm.linux.org.uk> >>>>>>>>> Cc: Arnd Bergmann<arnd@arndb.de> >>>>>>>>> Cc: Suravee Suthikulpanit<Suravee.Suthikulpanit@amd.com> >>>>>>>>> >>>>>>>>> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> >>>>>>>>> --- >>>>>>>>> >>>>>>>>> drivers/iommu/of_iommu.c | 10 ++++++++-- >>>>>>>>> drivers/of/platform.c | 2 +- >>>>>>>>> include/linux/of_iommu.h | 6 ++++-- >>>>>>>>> 3 files changed, 13 insertions(+), 5 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c >>>>>>>>> index af1dc6a..439235b 100644 >>>>>>>>> --- a/drivers/iommu/of_iommu.c >>>>>>>>> +++ b/drivers/iommu/of_iommu.c >>>>>>>>> @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct >>>>>>>>> device_node *np) >>>>>>>>> return ops; >>>>>>>>> } >>>>>>>>> >>>>>>>>> -struct iommu_ops *of_iommu_configure(struct device *dev) >>>>>>>>> +struct iommu_ops *of_iommu_configure(struct device *dev, >>>>>>>>> + struct device_node *iommu_np) >>>>>>>>> { >>>>>>>>> struct of_phandle_args iommu_spec; >>>>>>>>> struct device_node *np; >>>>>>>>> struct iommu_ops *ops = NULL; >>>>>>>>> int idx = 0; >>>>>>>>> >>>>>>>>> + if (dev_is_pci(dev)) { >>>>>>>>> + dev_err(dev, "iommu is currently not supported for PCI\n"); >>>>>>>>> + return NULL; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> /* >>>>>>>>> * We don't currently walk up the tree looking for a parent >>>>>>>>> IOMMU. >>>>>>>>> * See the `Notes:' section of >>>>>>>>> * Documentation/devicetree/bindings/iommu/iommu.txt >>>>>>>>> */ >>>>>>>> >>>>>>>> Wouldn't it be better to fix this rather than passing the device >>>>>>>> node pointer to the function ? The solution would be more generic. >>>>>>> >>>>>>> Will Deacon (Copied here) is working on this as we alteady discussed >>>>>>> in this thread. So it will be addressed by him in another series. >>>>>> >>>>>> Well, "working on this" equates to "has it somewhere near the bottom >>>>>> of a very long list" :) >>>>> >>>>> What's your opinion on this patch then, do you think adding the >>>>> iommu_np argument makes sense as a temporary hack, or should we instead >>>>> walk up the tree looking for an iommus attribute in parent nodes ? I >>>>> don't expect that to be insanely difficult to code. >>>> >>>> If walking up the tree is useful, then I think we should do that and >>>> update the Documentation to reflect the new behaviour. >>> >>> If I understand Murali's patch set right (please correct me if that's not >>> the case) the PCI code walks up the DT nodes hierarchy to the parent node >>> that contains the iommus attribute and passes a pointer to that node to >>> this function. It's thus a PCI-specific solution. As a temporary hack >>> that's OK I suppose, but if implementing it right straight away isn't >>> difficult that would be better. >> >> It looks to me like the code walks the PCI topology to get the DT node for >> the host controller, and passes *that* to of_dma_configure. That sounds >> like the right thing to do to me, especially since the PCI topology is >> likely not encoded in the device-tree. So actually, it is passing the >> first parent node afaict. > > Indeed, that's right. I forgot for a moment that we have non-DT devices ;-) > > Acked-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com> > > Murali, nitpicking a bit, shouldn't the iommu_np parameter be renamed ? It > points to the node containing the iommus parameter, not to the iommu node, so > the current name is slightly confusing. A brief kerneldoc above the function > would also help. This can be the subject of a separate patch. Probably the doc part can be added by Will. The iommu_np was a suggestion from Rob, if I recollect. Isn't the iommu_np points to the iommu device, and if so, iommu_np make sense. Murali > >> The part I'm more interested in is the mapping of RequesterID (PCI dma >> alias) to IOMMU ID when we transition from the PCI topology to the DT >> topology. Currently, it seems to be 1:1 on the platforms I have, but I >> doubt this will always be the case. >
On Wed, Jan 28, 2015 at 5:32 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Will, > > On Wednesday 28 January 2015 13:32:19 Will Deacon wrote: >> On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote: >> > On Wednesday 28 January 2015 12:29:42 Will Deacon wrote: >> >> On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote: >> >>> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote: >> >>>> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote: >> >>>>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote: >> >>>>>> On Friday 23 January 2015 17:32:34 Murali Karicheri wrote: >> >>>>>>> Function of_iommu_configure() is called from of_dma_configure() to >> >>>>>>> setup iommu ops using DT property. This API is currently used for >> >>>>>>> platform devices for which DMA configuration (including iommu ops) >> >>>>>>> may come from device's parent. To extend this functionality for >> >>>>>>> PCI devices, this API need to take a parent node ptr as an argument >> >>>>>>> instead of assuming device's parent. This is needed since for PCI, >> >>>>>>> the dma configuration may be defined in the DT node of the root >> >>>>>>> bus bridge's parent device. Currently only dma-range is used for PCI >> >>>>>>> and iommu is not supported. So return error if the device is PCI. [...] >> > If I understand Murali's patch set right (please correct me if that's not >> > the case) the PCI code walks up the DT nodes hierarchy to the parent node >> > that contains the iommus attribute and passes a pointer to that node to >> > this function. It's thus a PCI-specific solution. As a temporary hack >> > that's OK I suppose, but if implementing it right straight away isn't >> > difficult that would be better. >> >> It looks to me like the code walks the PCI topology to get the DT node for >> the host controller, and passes *that* to of_dma_configure. That sounds >> like the right thing to do to me, especially since the PCI topology is >> likely not encoded in the device-tree. So actually, it is passing the >> first parent node afaict. > > Indeed, that's right. I forgot for a moment that we have non-DT devices ;-) > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Murali, nitpicking a bit, shouldn't the iommu_np parameter be renamed ? It > points to the node containing the iommus parameter, not to the iommu node, so > the current name is slightly confusing. A brief kerneldoc above the function > would also help. This can be the subject of a separate patch. It was more confusing having np and node within the function. Rob
Hi Rob, On Thursday 29 January 2015 10:49:38 Rob Herring wrote: > On Wed, Jan 28, 2015 at 5:32 PM, Laurent Pinchart wrote: > > On Wednesday 28 January 2015 13:32:19 Will Deacon wrote: > >> On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote: > >>> On Wednesday 28 January 2015 12:29:42 Will Deacon wrote: > >>>> On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote: > >>>>> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote: > >>>>>> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote: > >>>>>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote: > >>>>>>>> On Friday 23 January 2015 17:32:34 Murali Karicheri wrote: > >>>>>>>>> Function of_iommu_configure() is called from of_dma_configure() > >>>>>>>>> to setup iommu ops using DT property. This API is currently used > >>>>>>>>> for platform devices for which DMA configuration (including iommu > >>>>>>>>> ops) may come from device's parent. To extend this functionality > >>>>>>>>> for PCI devices, this API need to take a parent node ptr as an > >>>>>>>>> argument instead of assuming device's parent. This is needed since > >>>>>>>>> for PCI, the dma configuration may be defined in the DT node of > >>>>>>>>> the root bus bridge's parent device. Currently only dma-range is > >>>>>>>>> used for PCI and iommu is not supported. So return error if the > >>>>>>>>> device is PCI. > > [...] > > >>> If I understand Murali's patch set right (please correct me if that's > >>> not the case) the PCI code walks up the DT nodes hierarchy to the > >>> parent node that contains the iommus attribute and passes a pointer to > >>> that node to this function. It's thus a PCI-specific solution. As a > >>> temporary hack that's OK I suppose, but if implementing it right > >>> straight away isn't difficult that would be better. > >> > >> It looks to me like the code walks the PCI topology to get the DT node > >> for the host controller, and passes *that* to of_dma_configure. That > >> sounds like the right thing to do to me, especially since the PCI > >> topology is likely not encoded in the device-tree. So actually, it is > >> passing the first parent node afaict. > > > > Indeed, that's right. I forgot for a moment that we have non-DT devices > > ;-) > > > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Murali, nitpicking a bit, shouldn't the iommu_np parameter be renamed ? It > > points to the node containing the iommus parameter, not to the iommu node, > > so the current name is slightly confusing. A brief kerneldoc above the > > function would also help. This can be the subject of a separate patch. > > It was more confusing having np and node within the function. Agreed. How about master_np or iommu_master_np ? The latter might be a bit long.
On 01/29/2015 07:24 PM, Laurent Pinchart wrote: > Hi Rob, > > On Thursday 29 January 2015 10:49:38 Rob Herring wrote: >> On Wed, Jan 28, 2015 at 5:32 PM, Laurent Pinchart wrote: >>> On Wednesday 28 January 2015 13:32:19 Will Deacon wrote: >>>> On Wed, Jan 28, 2015 at 01:15:10PM +0000, Laurent Pinchart wrote: >>>>> On Wednesday 28 January 2015 12:29:42 Will Deacon wrote: >>>>>> On Wed, Jan 28, 2015 at 12:23:03PM +0000, Laurent Pinchart wrote: >>>>>>> On Wednesday 28 January 2015 11:33:00 Will Deacon wrote: >>>>>>>> On Mon, Jan 26, 2015 at 06:49:01PM +0000, Murali Karicheri wrote: >>>>>>>> On 01/25/2015 08:32 AM, Laurent Pinchart wrote: >>>>>>>>>> On Friday 23 January 2015 17:32:34 Murali Karicheri wrote: >>>>>>>>>>> Function of_iommu_configure() is called from of_dma_configure() >>>>>>>>>>> to setup iommu ops using DT property. This API is currently used >>>>>>>>>>> for platform devices for which DMA configuration (including iommu >>>>>>>>>>> ops) may come from device's parent. To extend this functionality >>>>>>>>>>> for PCI devices, this API need to take a parent node ptr as an >>>>>>>>>>> argument instead of assuming device's parent. This is needed since >>>>>>>>>>> for PCI, the dma configuration may be defined in the DT node of >>>>>>>>>>> the root bus bridge's parent device. Currently only dma-range is >>>>>>>>>>> used for PCI and iommu is not supported. So return error if the >>>>>>>>>>> device is PCI. >> >> [...] >> >>>>> If I understand Murali's patch set right (please correct me if that's >>>>> not the case) the PCI code walks up the DT nodes hierarchy to the >>>>> parent node that contains the iommus attribute and passes a pointer to >>>>> that node to this function. It's thus a PCI-specific solution. As a >>>>> temporary hack that's OK I suppose, but if implementing it right >>>>> straight away isn't difficult that would be better. >>>> >>>> It looks to me like the code walks the PCI topology to get the DT node >>>> for the host controller, and passes *that* to of_dma_configure. That >>>> sounds like the right thing to do to me, especially since the PCI >>>> topology is likely not encoded in the device-tree. So actually, it is >>>> passing the first parent node afaict. >>> >>> Indeed, that's right. I forgot for a moment that we have non-DT devices >>> ;-) >>> >>> Acked-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com> >>> >>> Murali, nitpicking a bit, shouldn't the iommu_np parameter be renamed ? It >>> points to the node containing the iommus parameter, not to the iommu node, >>> so the current name is slightly confusing. A brief kerneldoc above the >>> function would also help. This can be the subject of a separate patch. >> >> It was more confusing having np and node within the function. > > Agreed. > > How about master_np or iommu_master_np ? The latter might be a bit long. > Probabaly master_np suffice as this function is named of_iommu_configure(). I will update it.
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index af1dc6a..439235b 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -133,19 +133,25 @@ struct iommu_ops *of_iommu_get_ops(struct device_node *np) return ops; } -struct iommu_ops *of_iommu_configure(struct device *dev) +struct iommu_ops *of_iommu_configure(struct device *dev, + struct device_node *iommu_np) { struct of_phandle_args iommu_spec; struct device_node *np; struct iommu_ops *ops = NULL; int idx = 0; + if (dev_is_pci(dev)) { + dev_err(dev, "iommu is currently not supported for PCI\n"); + return NULL; + } + /* * We don't currently walk up the tree looking for a parent IOMMU. * 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(iommu_np, "iommus", "#iommu-cells", idx, &iommu_spec)) { np = iommu_spec.np; diff --git a/drivers/of/platform.c b/drivers/of/platform.c index a54ec10..7675b79 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -196,7 +196,7 @@ static void of_dma_configure(struct device *dev) dev_dbg(dev, "device is%sdma coherent\n", coherent ? " " : " not "); - iommu = of_iommu_configure(dev); + iommu = of_iommu_configure(dev, dev->of_node); dev_dbg(dev, "device is%sbehind an iommu\n", iommu ? " " : " not "); diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h index 16c7554..a97e5bd 100644 --- a/include/linux/of_iommu.h +++ b/include/linux/of_iommu.h @@ -12,7 +12,8 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix, size_t *size); extern void of_iommu_init(void); -extern struct iommu_ops *of_iommu_configure(struct device *dev); +extern struct iommu_ops *of_iommu_configure(struct device *dev, + struct device_node *iommu_np); #else @@ -24,7 +25,8 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix, } static inline void of_iommu_init(void) { } -static inline struct iommu_ops *of_iommu_configure(struct device *dev) +static inline struct iommu_ops *of_iommu_configure(struct device *dev, + struct device_node *iommu_np) { return NULL; }
Function of_iommu_configure() is called from of_dma_configure() to setup iommu ops using DT property. This API is currently used for platform devices for which DMA configuration (including iommu ops) may come from device's parent. To extend this functionality for PCI devices, this API need to take a parent node ptr as an argument instead of assuming device's parent. This is needed since for PCI, the dma configuration may be defined in the DT node of the root bus bridge's parent device. Currently only dma-range is used for PCI and iommu is not supported. So return error if the device is PCI. Cc: Joerg Roedel <joro@8bytes.org> Cc: Grant Likely <grant.likely@linaro.org> Cc: Rob Herring <robh+dt@kernel.org> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Russell King <linux@arm.linux.org.uk> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> --- drivers/iommu/of_iommu.c | 10 ++++++++-- drivers/of/platform.c | 2 +- include/linux/of_iommu.h | 6 ++++-- 3 files changed, 13 insertions(+), 5 deletions(-)