RE: [PATCH 2/6 v2] iommu: of: make of_pci_map_rid() available for other devices too
> -Original Message- > From: Robin Murphy [mailto:robin.mur...@arm.com] > Sent: Tuesday, April 17, 2018 10:23 PM > To: Nipun Gupta ; robh...@kernel.org; > frowand.l...@gmail.com > Cc: will.dea...@arm.com; mark.rutl...@arm.com; catalin.mari...@arm.com; > h...@lst.de; gre...@linuxfoundation.org; j...@8bytes.org; > m.szyprow...@samsung.com; shawn...@kernel.org; bhelg...@google.com; > io...@lists.linux-foundation.org; linux-ker...@vger.kernel.org; > devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linuxppc- > d...@lists.ozlabs.org; linux-...@vger.kernel.org; Bharat Bhushan > ; stuyo...@gmail.com; Laurentiu Tudor > ; Leo Li > Subject: Re: [PATCH 2/6 v2] iommu: of: make of_pci_map_rid() available for > other devices too > > On 17/04/18 11:21, Nipun Gupta wrote: > > iommu-map property is also used by devices with fsl-mc. This patch > > moves the of_pci_map_rid to generic location, so that it can be used > > by other busses too. Nipun, please clarify that only function name is changed and rest of body is same. > > > > Signed-off-by: Nipun Gupta > > --- > > drivers/iommu/of_iommu.c | 106 > > +-- > > Doesn't this break "msi-parent" parsing for !CONFIG_OF_IOMMU? Yes, this will be a problem with MSI > I guess you > don't want fsl-mc to have to depend on PCI, but this looks like a step in the > wrong direction. > > I'm not entirely sure where of_map_rid() fits best, but from a quick look > around > the least-worst option might be drivers/of/of_address.c, unless Rob and Frank > have a better idea of where generic DT-based ID translation routines could > live? drivers/of/address.c may be proper place to move until someone have better idea. Thanks -Bharat > > > drivers/of/irq.c | 6 +-- > > drivers/pci/of.c | 101 > > > > include/linux/of_iommu.h | 11 + > > include/linux/of_pci.h | 10 - > > 5 files changed, 117 insertions(+), 117 deletions(-) > > > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index > > 5c36a8b..4e7712f 100644 > > --- a/drivers/iommu/of_iommu.c > > +++ b/drivers/iommu/of_iommu.c > > @@ -138,6 +138,106 @@ static int of_iommu_xlate(struct device *dev, > > return ops->of_xlate(dev, iommu_spec); > > } > > > > +/** > > + * of_map_rid - Translate a requester ID through a downstream mapping. > > + * @np: root complex device node. > > + * @rid: device requester ID to map. > > + * @map_name: property name of the map to use. > > + * @map_mask_name: optional property name of the mask to use. > > + * @target: optional pointer to a target device node. > > + * @id_out: optional pointer to receive the translated ID. > > + * > > + * Given a device requester ID, look up the appropriate > > +implementation-defined > > + * platform ID and/or the target device which receives transactions > > +on that > > + * ID, as per the "iommu-map" and "msi-map" bindings. Either of > > +@target or > > + * @id_out may be NULL if only the other is required. If @target > > +points to > > + * a non-NULL device node pointer, only entries targeting that node > > +will be > > + * matched; if it points to a NULL value, it will receive the device > > +node of > > + * the first matching target phandle, with a reference held. > > + * > > + * Return: 0 on success or a standard error code on failure. > > + */ > > +int of_map_rid(struct device_node *np, u32 rid, > > + const char *map_name, const char *map_mask_name, > > + struct device_node **target, u32 *id_out) { > > + u32 map_mask, masked_rid; > > + int map_len; > > + const __be32 *map = NULL; > > + > > + if (!np || !map_name || (!target && !id_out)) > > + return -EINVAL; > > + > > + map = of_get_property(np, map_name, &map_len); > > + if (!map) { > > + if (target) > > + return -ENODEV; > > + /* Otherwise, no map implies no translation */ > > + *id_out = rid; > > + return 0; > > + } > > + > > + if (!map_len || map_len % (4 * sizeof(*map))) { > > + pr_err("%pOF: Error: Bad %s length: %d\n", np, > > + map_name, map_len); > > + return -EINVAL; > > + } > > + > > + /* The default is to select all bits. */ > > + map_mask = 0x; >
RE: [PATCH 2/6 v2] iommu: of: make of_pci_map_rid() available for other devices too
> -Original Message- > From: Robin Murphy [mailto:robin.mur...@arm.com] > Sent: Tuesday, April 17, 2018 10:23 PM > To: Nipun Gupta ; robh...@kernel.org; > frowand.l...@gmail.com > Cc: will.dea...@arm.com; mark.rutl...@arm.com; catalin.mari...@arm.com; > h...@lst.de; gre...@linuxfoundation.org; j...@8bytes.org; > m.szyprow...@samsung.com; shawn...@kernel.org; bhelg...@google.com; > io...@lists.linux-foundation.org; linux-ker...@vger.kernel.org; > devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; linuxppc- > d...@lists.ozlabs.org; linux-...@vger.kernel.org; Bharat Bhushan > ; stuyo...@gmail.com; Laurentiu Tudor > ; Leo Li > Subject: Re: [PATCH 2/6 v2] iommu: of: make of_pci_map_rid() available for > other devices too > > On 17/04/18 11:21, Nipun Gupta wrote: > > iommu-map property is also used by devices with fsl-mc. This patch > > moves the of_pci_map_rid to generic location, so that it can be used > > by other busses too. > > > > Signed-off-by: Nipun Gupta > > --- > > drivers/iommu/of_iommu.c | 106 > > +-- > > Doesn't this break "msi-parent" parsing for !CONFIG_OF_IOMMU? I guess you > don't want fsl-mc to have to depend on PCI, but this looks like a step in the > wrong direction. Thanks for pointing out. Agree, this will break "msi-parent" parsing for !CONFIG_OF_IOMMU case. > > I'm not entirely sure where of_map_rid() fits best, but from a quick look > around > the least-worst option might be drivers/of/of_address.c, unless Rob and Frank > have a better idea of where generic DT-based ID translation routines could > live? > > > drivers/of/irq.c | 6 +-- > > drivers/pci/of.c | 101 > > > > include/linux/of_iommu.h | 11 + > > include/linux/of_pci.h | 10 - > > 5 files changed, 117 insertions(+), 117 deletions(-) > > [...] > > struct of_pci_iommu_alias_info { > > struct device *dev; > > struct device_node *np; > > @@ -149,9 +249,9 @@ static int of_pci_iommu_init(struct pci_dev *pdev, u16 > alias, void *data) > > struct of_phandle_args iommu_spec = { .args_count = 1 }; > > int err; > > > > - err = of_pci_map_rid(info->np, alias, "iommu-map", > > -"iommu-map-mask", &iommu_spec.np, > > -iommu_spec.args); > > + err = of_map_rid(info->np, alias, "iommu-map", > > +"iommu-map-mask", &iommu_spec.np, > > +iommu_spec.args); > > Super-nit: Apparently I missed rewrapping this to 2 lines in d87beb749281, > but if > it's being touched again, that would be nice ;) Sure.. I'll take care of this in the next version :) Regards, Nipun
Re: [PATCH 2/6 v2] iommu: of: make of_pci_map_rid() available for other devices too
On 17/04/18 11:21, Nipun Gupta wrote: iommu-map property is also used by devices with fsl-mc. This patch moves the of_pci_map_rid to generic location, so that it can be used by other busses too. Signed-off-by: Nipun Gupta --- drivers/iommu/of_iommu.c | 106 +-- Doesn't this break "msi-parent" parsing for !CONFIG_OF_IOMMU? I guess you don't want fsl-mc to have to depend on PCI, but this looks like a step in the wrong direction. I'm not entirely sure where of_map_rid() fits best, but from a quick look around the least-worst option might be drivers/of/of_address.c, unless Rob and Frank have a better idea of where generic DT-based ID translation routines could live? drivers/of/irq.c | 6 +-- drivers/pci/of.c | 101 include/linux/of_iommu.h | 11 + include/linux/of_pci.h | 10 - 5 files changed, 117 insertions(+), 117 deletions(-) diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index 5c36a8b..4e7712f 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -138,6 +138,106 @@ static int of_iommu_xlate(struct device *dev, return ops->of_xlate(dev, iommu_spec); } +/** + * of_map_rid - Translate a requester ID through a downstream mapping. + * @np: root complex device node. + * @rid: device requester ID to map. + * @map_name: property name of the map to use. + * @map_mask_name: optional property name of the mask to use. + * @target: optional pointer to a target device node. + * @id_out: optional pointer to receive the translated ID. + * + * Given a device requester ID, look up the appropriate implementation-defined + * platform ID and/or the target device which receives transactions on that + * ID, as per the "iommu-map" and "msi-map" bindings. Either of @target or + * @id_out may be NULL if only the other is required. If @target points to + * a non-NULL device node pointer, only entries targeting that node will be + * matched; if it points to a NULL value, it will receive the device node of + * the first matching target phandle, with a reference held. + * + * Return: 0 on success or a standard error code on failure. + */ +int of_map_rid(struct device_node *np, u32 rid, + const char *map_name, const char *map_mask_name, + struct device_node **target, u32 *id_out) +{ + u32 map_mask, masked_rid; + int map_len; + const __be32 *map = NULL; + + if (!np || !map_name || (!target && !id_out)) + return -EINVAL; + + map = of_get_property(np, map_name, &map_len); + if (!map) { + if (target) + return -ENODEV; + /* Otherwise, no map implies no translation */ + *id_out = rid; + return 0; + } + + if (!map_len || map_len % (4 * sizeof(*map))) { + pr_err("%pOF: Error: Bad %s length: %d\n", np, + map_name, map_len); + return -EINVAL; + } + + /* The default is to select all bits. */ + map_mask = 0x; + + /* +* Can be overridden by "{iommu,msi}-map-mask" property. +*/ + if (map_mask_name) + of_property_read_u32(np, map_mask_name, &map_mask); + + masked_rid = map_mask & rid; + for ( ; map_len > 0; map_len -= 4 * sizeof(*map), map += 4) { + struct device_node *phandle_node; + u32 rid_base = be32_to_cpup(map + 0); + u32 phandle = be32_to_cpup(map + 1); + u32 out_base = be32_to_cpup(map + 2); + u32 rid_len = be32_to_cpup(map + 3); + + if (rid_base & ~map_mask) { + pr_err("%pOF: Invalid %s translation - %s-mask (0x%x) ignores rid-base (0x%x)\n", + np, map_name, map_name, + map_mask, rid_base); + return -EFAULT; + } + + if (masked_rid < rid_base || masked_rid >= rid_base + rid_len) + continue; + + phandle_node = of_find_node_by_phandle(phandle); + if (!phandle_node) + return -ENODEV; + + if (target) { + if (*target) + of_node_put(phandle_node); + else + *target = phandle_node; + + if (*target != phandle_node) + continue; + } + + if (id_out) + *id_out = masked_rid - rid_base + out_base; + + pr_debug("%pOF: %s, using mask %08x, rid-base: %08x, out-base: %08x, length: %08x, rid: %08x -> %08x\n", + np, map_name, map_mask, rid_base, out_base, + rid_len, rid, masked_rid - rid_base + out_base); +