| Message ID | 1419459099-6667-2-git-send-email-m-karicheri2@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 24, 2014 at 4:11 PM, Murali Karicheri <m-karicheri2@ti.com> 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@ti.com> > --- > drivers/of/of_pci.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/of_pci.h | 12 ++++++++ > 2 files changed, 85 insertions(+) > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > index 88471d3..6d43f59 100644 > --- a/drivers/of/of_pci.c > +++ b/drivers/of/of_pci.c > @@ -229,6 +229,79 @@ parse_failed: > return err; > } > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > + > +/** > + * of_get_pci_root_bridge_parent - Helper function to get the OF node of > + * the root bridge's parent I believe this has to be a one line description for DocBook. > + * @dev: ptr to pci_dev struct of the pci device > + * > + * This function will traverse the bus up to the root bus starting with > + * the child and return the of node ptr to bridge device's parent device. > + */ > +struct device_node *of_get_pci_root_bridge_parent(struct pci_dev *dev) > +{ > + struct pci_bus *bus = dev->bus; > + struct device *bridge; > + > + while (!pci_is_root_bus(bus)) > + bus = bus->parent; > + bridge = bus->bridge; > + > + return bridge->parent->of_node; > +} > +EXPORT_SYMBOL_GPL(of_get_pci_root_bridge_parent); > + > +/** > + * of_pci_dma_configure - Setup DMA configuration > + * @dev: ptr to pci_dev struct of the pci device > + * > + * Try to get PCI devices's DMA configuration from DT and update it > + * accordingly. This is similar to of_dma_configure() in of/platform.c > + */ > +void of_pci_dma_configure(struct pci_dev *pci_dev) > +{ > + struct device *dev = &pci_dev->dev; > + u64 dma_addr, paddr, size; > + struct device_node *parent_np; > + unsigned long offset; > + bool coherent; > + int ret; > + > + parent_np = of_get_pci_root_bridge_parent(pci_dev); > + > + if (parent_np) { Save a level of indentation and do: if (!parent_np) return; > + /* > + * Set default dma-mask to 32 bit. Drivers are expected to setup > + * the correct supported dma_mask. > + */ > + dev->coherent_dma_mask = DMA_BIT_MASK(32); > + > + /* > + * Set it to coherent_dma_mask by default if the architecture > + * code has not set it. > + */ > + if (!dev->dma_mask) > + dev->dma_mask = &dev->coherent_dma_mask; > + > + ret = of_dma_get_range(parent_np, &dma_addr, &paddr, &size); > + if (ret < 0) { > + dma_addr = offset = 0; > + size = dev->coherent_dma_mask + 1; > + } else { > + offset = PFN_DOWN(paddr - dma_addr); > + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); > + } > + dev->dma_pfn_offset = offset; > + > + 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. Rob
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@ti.com> 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@ti.com> >> --- >> drivers/of/of_pci.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/of_pci.h | 12 ++++++++ >> 2 files changed, 85 insertions(+) >> ----------------------CUT-------------------------------------------- >> + unsigned long offset; >> + bool coherent; >> + int ret; >> + >> + parent_np = of_get_pci_root_bridge_parent(pci_dev); >> + >> + if (parent_np) { > > Save a level of indentation and do: > > if (!parent_np) > return; Agree. This will not be needed if I go with changes proposed by your next comment below. > >> + /* -------CUT------------------------------------------------------ >> + >> + 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. 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> 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)) { 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); 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. If the above looks good, I can post v3 of the patch with these changes. > Rob
On Fri, Jan 2, 2015 at 11:20 AM, Murali Karicheri <m-karicheri2@ti.com> 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@ti.com> >> 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@ti.com> >>> --- >>> 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
On Friday 02 January 2015 12:20:53 Murali Karicheri wrote: > 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. > > 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> Yes, sounds good. > 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)) { Right. > 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); With dev = &pci_dev->dev, I assume? > 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 are only two callers, I don't think we need a wrapper for it, just change the calling conventions along with step 2. > If the above looks good, I can post v3 of the patch with these changes. With that one minor change, sounds perfect to me. The same can also be used by other bus types if we ever need it. Arnd
On 01/02/2015 03:57 PM, Arnd Bergmann wrote: > On Friday 02 January 2015 12:20:53 Murali Karicheri wrote: >> 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. >> >> 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> > > Yes, sounds good. > >> 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)) { > > Right. > >> 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); > > With dev =&pci_dev->dev, I assume? Yes > >> 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 are only two callers, I don't think we need a wrapper for it, > just change the calling conventions along with step 2. Ok. That is what Rob's comment as well. > >> If the above looks good, I can post v3 of the patch with these changes. > > With that one minor change, sounds perfect to me. The same can also > be used by other bus types if we ever need it. > > Arnd > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/02/2015 03:45 PM, Rob Herring wrote: > On Fri, Jan 2, 2015 at 11:20 AM, Murali Karicheri<m-karicheri2@ti.com> 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@ti.com> >>> 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@ti.com> >>>> --- >>>> 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. I have no experience with IOMMU and may not offer much help here as I originally wrote above. Will Deacon has added this API and probably able to offer some help in this discussion. Will Deacon, Any comment? Looking at the iommu documentation and of_iommu.c, I get a feeling that this API is not really used at present as there are no callers of of_iommu_set_ops() and I assume this is a WIP. I believe the way it is expected to work is to have the iommu driver of the master IOMMU devices call of_iommu_set_ops(). The device node of this master IOMMU device is specified as a phandle in the OF node of the device (various bus devices such as platform, PCI etc). This allow to retrieve the iommu ops though the of_iommu_configure() API and use it in arch_setup_dma_ops(). So my gut feeling is that for PCI devices, as there are no DT node, the root bus node may specify iommus phandle to IOMMU master OF nodes. W.r.t your comment "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", I believe, the parent node search itself should work the same way in the case of PCI as with platform bus case. PCI's case, we are providing the OF node of the root bus host bridge. Why should this be any different in terms of search? I see a potential issue with dma-ranges as described in the notes below. As noted below the usage of dma-range for iommu is to be determined. For keystone, the of_iommu_configure() always return false as we don't use the iommu. But don't know if this has any consequences for other platforms. Or I got your questions wrong. Any help here from others on the list? ======================================================================== One possible extension to the above is to use an "iommus" property along with a "dma-ranges" property in a bus device node (such as PCI host bridges). This can be useful to describe how children on the bus relate to the IOMMU if they are not explicitly listed in the device tree (e.g. PCI devices). However, the requirements of that use-case haven't been fully determined yet. Implementing this is therefore not recommended without further discussion and extension of this binding. ========================================================================= The code is struct iommu_ops *of_iommu_configure(struct device *dev) { struct of_phandle_args iommu_spec; struct device_node *np; struct iommu_ops *ops = NULL; int idx = 0; /* * 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", "#iommu-cells", idx, &iommu_spec)) { np = iommu_spec.np; ops = of_iommu_get_ops(np); if (!ops || !ops->of_xlate || ops->of_xlate(dev, &iommu_spec)) goto err_put_node; of_node_put(np); idx++; } return ops; err_put_node: of_node_put(np); return NULL; } >> 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. Ok. > > Rob
On Friday 02 January 2015 17:33:53 Murali Karicheri wrote: > > I have no experience with IOMMU and may not offer much help here as I > originally wrote above. Will Deacon has added this API and probably able > to offer some help in this discussion. > > Will Deacon, > > Any comment? It's complicated :( > Looking at the iommu documentation and of_iommu.c, I get a feeling that > this API is not really used at present as there are no callers of > of_iommu_set_ops() and I assume this is a WIP. Right, but we have patches for some iommu drivers based on this API, and we should migrate all of them eventually. > I believe the way it is > expected to work is to have the iommu driver of the master IOMMU devices > call of_iommu_set_ops(). The device node of this master IOMMU device is > specified as a phandle in the OF node of the device (various bus devices > such as platform, PCI etc). This allow to retrieve the iommu ops though > the of_iommu_configure() API and use it in arch_setup_dma_ops(). So my > gut feeling is that for PCI devices, as there are no DT node, the root > bus node may specify iommus phandle to IOMMU master OF nodes. Yes, but we also need to pass a PCI device specific identifier along with the root bus node, because some iommu drivers take the PCI bus/device/function number into account for creating per-function i/o page tables. > W.r.t your comment "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", > > I believe, the parent node search itself should work the same way in the > case of PCI as with platform bus case. PCI's case, we are providing the > OF node of the root bus host bridge. Why should this be any different in > terms of search? > > I see a potential issue with dma-ranges as described in the notes below. > As noted below the usage of dma-range for iommu is to be determined. For > keystone, the of_iommu_configure() always return false as we don't use > the iommu. But don't know if this has any consequences for other > platforms. Or I got your questions wrong. Any help here from others on > the list? > > ======================================================================== > One possible extension to the above is to use an "iommus" property along > with a "dma-ranges" property in a bus device node (such as PCI host > bridges). This can be useful to describe how children on the bus relate > to the IOMMU if they are not explicitly listed in the device tree (e.g. > PCI devices). However, the requirements of that use-case haven't been > fully determined yet. Implementing this is therefore not recommended > without further discussion and extension of this binding. > ========================================================================= This probably won't ever apply to PCI devices, so let's ignore it for now. For the moment (and for PCI), we should assume that we either configure an iommu directly or we use dma-ranges if no iommu is in use. Arnd
On 01/03/2015 04:37 PM, Arnd Bergmann wrote: > On Friday 02 January 2015 17:33:53 Murali Karicheri wrote: >> >> I have no experience with IOMMU and may not offer much help here as I >> originally wrote above. Will Deacon has added this API and probably able >> to offer some help in this discussion. >> >> Will Deacon, >> >> Any comment? > > It's complicated :( > >> Looking at the iommu documentation and of_iommu.c, I get a feeling that >> this API is not really used at present as there are no callers of >> of_iommu_set_ops() and I assume this is a WIP. > > Right, but we have patches for some iommu drivers based on this API, > and we should migrate all of them eventually. > >> I believe the way it is >> expected to work is to have the iommu driver of the master IOMMU devices >> call of_iommu_set_ops(). The device node of this master IOMMU device is >> specified as a phandle in the OF node of the device (various bus devices >> such as platform, PCI etc). This allow to retrieve the iommu ops though >> the of_iommu_configure() API and use it in arch_setup_dma_ops(). So my >> gut feeling is that for PCI devices, as there are no DT node, the root >> bus node may specify iommus phandle to IOMMU master OF nodes. > > Yes, but we also need to pass a PCI device specific identifier along > with the root bus node, because some iommu drivers take the PCI > bus/device/function number into account for creating per-function > i/o page tables. > >> W.r.t your comment "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", >> >> I believe, the parent node search itself should work the same way in the >> case of PCI as with platform bus case. PCI's case, we are providing the >> OF node of the root bus host bridge. Why should this be any different in >> terms of search? >> >> I see a potential issue with dma-ranges as described in the notes below. >> As noted below the usage of dma-range for iommu is to be determined. For >> keystone, the of_iommu_configure() always return false as we don't use >> the iommu. But don't know if this has any consequences for other >> platforms. Or I got your questions wrong. Any help here from others on >> the list? >> >> ======================================================================== >> One possible extension to the above is to use an "iommus" property along >> with a "dma-ranges" property in a bus device node (such as PCI host >> bridges). This can be useful to describe how children on the bus relate >> to the IOMMU if they are not explicitly listed in the device tree (e.g. >> PCI devices). However, the requirements of that use-case haven't been >> fully determined yet. Implementing this is therefore not recommended >> without further discussion and extension of this binding. >> ========================================================================= > > This probably won't ever apply to PCI devices, so let's ignore it for now. > For the moment (and for PCI), we should assume that we either configure > an iommu directly or we use dma-ranges if no iommu is in use. > Thanks Arnd, I will post v3 of the patch with what is agreed before in my response and I understand there is no additional change required based on this particular discussion about iommu. Right? Murali > Arnd
On Monday 05 January 2015 15:06:57 Murali Karicheri wrote: > On 01/03/2015 04:37 PM, Arnd Bergmann wrote: > > Yes, but we also need to pass a PCI device specific identifier along > > with the root bus node, because some iommu drivers take the PCI > > bus/device/function number into account for creating per-function > > i/o page tables. > ... > I will post v3 of the patch with what is agreed before in my response > and I understand there is no additional change required based on this > particular discussion about iommu. Right? Actually regarding the bit I wrote above, it might be helpful to pass the PCI_DEVID() into both of_iommu_configure and of_dma_configure. While this may or may not be sufficient, I think there is no question about it being needed for the ARM SMMU with PCI, so we may as well add it at the point when you touch the same lines already. In the platform bus case, just pass zero here. Arnd
On 01/05/2015 05:26 PM, Arnd Bergmann wrote: > On Monday 05 January 2015 15:06:57 Murali Karicheri wrote: >> On 01/03/2015 04:37 PM, Arnd Bergmann wrote: >>> Yes, but we also need to pass a PCI device specific identifier along >>> with the root bus node, because some iommu drivers take the PCI >>> bus/device/function number into account for creating per-function >>> i/o page tables. > >> ... > >> I will post v3 of the patch with what is agreed before in my response >> and I understand there is no additional change required based on this >> particular discussion about iommu. Right? > > Actually regarding the bit I wrote above, it might be helpful to pass > the PCI_DEVID() into both of_iommu_configure and of_dma_configure. > > While this may or may not be sufficient, I think there is no question > about it being needed for the ARM SMMU with PCI, so we may as well add > it at the point when you touch the same lines already. In the platform > bus case, just pass zero here. Arnd, PCI_DEVID() is defined as #define PCI_DEVID(bus, devfn) ((((u16)(bus)) << 8) | (devfn)) So PCI_DEVID of 0 is a valid PCI DEVID. How about checking if the device is PCI in of_iommu_configure() using dev_is_pci(dev) macro and return immediately for PCI? Need to include pci.h in this file though. Some of the iommu drivers already include this. a0868495@ares-ubuntu:~/projects/linux-keystone$ grep -r pci.h drivers/iommu/ drivers/iommu/amd_iommu_v2.c:#include <linux/pci.h> drivers/iommu/dmar.c:#include <linux/pci.h> drivers/iommu/amd_iommu_types.h:#include <linux/pci.h> drivers/iommu/amd_iommu.c:#include <linux/pci.h> drivers/iommu/fsl_pamu_domain.c:#include <sysdev/fsl_pci.h> drivers/iommu/iommu.c:#include <linux/pci.h> drivers/iommu/intel-iommu.c:#include <linux/pci.h> drivers/iommu/intel_irq_remapping.c:#include <linux/pci.h> drivers/iommu/arm-smmu.c:#include <linux/pci.h> drivers/iommu/amd_iommu_init.c:#include <linux/pci.h> drivers/iommu/irq_remapping.c:#include <linux/pci.h> This will allow us to re-visit this later for IOMMU support for PCI without polluting the API. Murali > > Arnd
On Fri, Jan 02, 2015 at 10:33:53PM +0000, Murali Karicheri wrote: > On 01/02/2015 03:45 PM, Rob Herring wrote: > > On Fri, Jan 2, 2015 at 11:20 AM, Murali Karicheri<m-karicheri2@ti.com> wrote: > >> On 12/26/2014 02:33 PM, Rob Herring wrote: > >>> On Wed, Dec 24, 2014 at 4:11 PM, Murali Karicheri<m-karicheri2@ti.com> > >>>> + 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. > > I have no experience with IOMMU and may not offer much help here as I > originally wrote above. Will Deacon has added this API and probably able > to offer some help in this discussion. > > Will Deacon, > > Any comment? Sorry for the delay; I'm still catching up on email after the Christmas break, but I *will* get around to looking at this. If you have a new version to post based on the comments from Rob and Arnd, feel free to send that and I'll look at that instead. Will
On 01/06/2015 02:50 PM, Will Deacon wrote: > On Fri, Jan 02, 2015 at 10:33:53PM +0000, Murali Karicheri wrote: >> On 01/02/2015 03:45 PM, Rob Herring wrote: >>> On Fri, Jan 2, 2015 at 11:20 AM, Murali Karicheri<m-karicheri2@ti.com> wrote: >>>> On 12/26/2014 02:33 PM, Rob Herring wrote: >>>>> On Wed, Dec 24, 2014 at 4:11 PM, Murali Karicheri<m-karicheri2@ti.com> >>>>>> + 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. >> >> I have no experience with IOMMU and may not offer much help here as I >> originally wrote above. Will Deacon has added this API and probably able >> to offer some help in this discussion. >> >> Will Deacon, >> >> Any comment? > > Sorry for the delay; I'm still catching up on email after the Christmas > break, but I *will* get around to looking at this. If you have a new version > to post based on the comments from Rob and Arnd, feel free to send that and > I'll look at that instead. > > Will Will, I will be posting v3 of the patch tomorrow and you can review that. Regards, Murali
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c index 88471d3..6d43f59 100644 --- a/drivers/of/of_pci.c +++ b/drivers/of/of_pci.c @@ -229,6 +229,79 @@ parse_failed: return err; } EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); + +/** + * of_get_pci_root_bridge_parent - Helper function to get the OF node of + * the root bridge's parent + * @dev: ptr to pci_dev struct of the pci device + * + * This function will traverse the bus up to the root bus starting with + * the child and return the of node ptr to bridge device's parent device. + */ +struct device_node *of_get_pci_root_bridge_parent(struct pci_dev *dev) +{ + struct pci_bus *bus = dev->bus; + struct device *bridge; + + while (!pci_is_root_bus(bus)) + bus = bus->parent; + bridge = bus->bridge; + + return bridge->parent->of_node; +} +EXPORT_SYMBOL_GPL(of_get_pci_root_bridge_parent); + +/** + * of_pci_dma_configure - Setup DMA configuration + * @dev: ptr to pci_dev struct of the pci device + * + * Try to get PCI devices's DMA configuration from DT and update it + * accordingly. This is similar to of_dma_configure() in of/platform.c + */ +void of_pci_dma_configure(struct pci_dev *pci_dev) +{ + struct device *dev = &pci_dev->dev; + u64 dma_addr, paddr, size; + struct device_node *parent_np; + unsigned long offset; + bool coherent; + int ret; + + parent_np = of_get_pci_root_bridge_parent(pci_dev); + + if (parent_np) { + /* + * Set default dma-mask to 32 bit. Drivers are expected to setup + * the correct supported dma_mask. + */ + dev->coherent_dma_mask = DMA_BIT_MASK(32); + + /* + * Set it to coherent_dma_mask by default if the architecture + * code has not set it. + */ + if (!dev->dma_mask) + dev->dma_mask = &dev->coherent_dma_mask; + + ret = of_dma_get_range(parent_np, &dma_addr, &paddr, &size); + if (ret < 0) { + dma_addr = offset = 0; + size = dev->coherent_dma_mask + 1; + } else { + offset = PFN_DOWN(paddr - dma_addr); + dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); + } + dev->dma_pfn_offset = offset; + + 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); + } +} +EXPORT_SYMBOL_GPL(of_pci_dma_configure); + #endif /* CONFIG_OF_ADDRESS */ #ifdef CONFIG_PCI_MSI diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index ce0e5ab..0465a2a 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -16,6 +16,8 @@ int of_pci_get_devfn(struct device_node *np); int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin); int of_pci_parse_bus_range(struct device_node *node, struct resource *res); int of_get_pci_domain_nr(struct device_node *node); +struct device_node *of_get_pci_root_bridge_parent(struct pci_dev *dev); +void of_pci_dma_configure(struct pci_dev *pci_dev); #else static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq) { @@ -50,6 +52,16 @@ of_get_pci_domain_nr(struct device_node *node) { return -1; } + +static inline struct device_node +*of_get_pci_root_bridge_parent(struct pci_dev *dev) +{ + return NULL; +} + +static inline void of_pci_dma_configure(struct pci_dev *pci_dev) +{ +} #endif #if defined(CONFIG_OF_ADDRESS)
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@ti.com> --- drivers/of/of_pci.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/of_pci.h | 12 ++++++++ 2 files changed, 85 insertions(+)