2018-04-30 06:29:38

by Nipun Gupta

[permalink] [raw]

2018-04-30 20:57:20

by Bjorn Helgaas

[permalink] [raw]

Subject: Re: [PATCH v4 2/6] iommu: of: make of_pci_map_rid() available for other devices too

On Mon, Apr 30, 2018 at 11:57:17AM +0530, 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.
>
> 'of_pci_map_rid' is renamed here to 'of_map_rid' and there is no
> functional change done in the API.
>
> Signed-off-by: Nipun Gupta <[email protected]>

Acked-by: Bjorn Helgaas <[email protected]>

> ---
> drivers/iommu/of_iommu.c | 5 +--
> drivers/of/base.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++
> drivers/of/irq.c | 5 +--
> drivers/pci/of.c | 101 ----------------------------------------------
> include/linux/of.h | 11 +++++
> include/linux/of_pci.h | 10 -----
> 6 files changed, 117 insertions(+), 117 deletions(-)
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 5c36a8b..811e160 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -149,9 +149,8 @@ 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);
> if (err)
> return err == -ENODEV ? NO_IOMMU : err;
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 848f549..c7aac81 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1995,3 +1995,105 @@ int of_find_last_cache_level(unsigned int cpu)
>
> return cache_level;
> }
> +
> +/**
> + * 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 = 0xffffffff;
> +
> + /*
> + * Can be overridden by "{iommu,msi}-map-mask" property.
> + * If of_property_read_u32() fails, the default is used.
> + */
> + 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);
> + return 0;
> + }
> +
> + pr_err("%pOF: Invalid %s translation - no match for rid 0x%x on %pOF\n",
> + np, map_name, rid, target && *target ? *target : NULL);
> + return -EFAULT;
> +}
> +EXPORT_SYMBOL_GPL(of_map_rid);
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 02ad93a..e1f6f39 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -22,7 +22,6 @@
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_irq.h>
> -#include <linux/of_pci.h>
> #include <linux/string.h>
> #include <linux/slab.h>
>
> @@ -588,8 +587,8 @@ static u32 __of_msi_map_rid(struct device *dev, struct device_node **np,
> * "msi-map" property.
> */
> for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent)
> - if (!of_pci_map_rid(parent_dev->of_node, rid_in, "msi-map",
> - "msi-map-mask", np, &rid_out))
> + if (!of_map_rid(parent_dev->of_node, rid_in, "msi-map",
> + "msi-map-mask", np, &rid_out))
> break;
> return rid_out;
> }
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index a28355c..d2cebbe 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -362,107 +362,6 @@ int of_pci_get_host_bridge_resources(struct device_node *dev,
> EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
> #endif /* CONFIG_OF_ADDRESS */
>
> -/**
> - * of_pci_map_rid - Translate a requester ID through a downstream mapping.
> - * @np: root complex device node.
> - * @rid: PCI 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 PCI 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_pci_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 = 0xffffffff;
> -
> - /*
> - * Can be overridden by "{iommu,msi}-map-mask" property.
> - * If of_property_read_u32() fails, the default is used.
> - */
> - 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);
> - return 0;
> - }
> -
> - pr_err("%pOF: Invalid %s translation - no match for rid 0x%x on %pOF\n",
> - np, map_name, rid, target && *target ? *target : NULL);
> - return -EFAULT;
> -}
> -
> #if IS_ENABLED(CONFIG_OF_IRQ)
> /**
> * of_irq_parse_pci - Resolve the interrupt for a PCI device
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 4d25e4f..f4251c3 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -545,6 +545,10 @@ const __be32 *of_prop_next_u32(struct property *prop, const __be32 *cur,
>
> extern int of_cpu_node_to_id(struct device_node *np);
>
> +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);
> +
> #else /* CONFIG_OF */
>
> static inline void of_core_init(void)
> @@ -931,6 +935,13 @@ static inline int of_cpu_node_to_id(struct device_node *np)
> return -ENODEV;
> }
>
> +static inline 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)
> +{
> + return -EINVAL;
> +}
> +
> #define of_match_ptr(_ptr) NULL
> #define of_match_node(_matches, _node) NULL
> #endif /* CONFIG_OF */
> diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
> index 091033a..a23b44a 100644
> --- a/include/linux/of_pci.h
> +++ b/include/linux/of_pci.h
> @@ -17,9 +17,6 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
> int of_get_pci_domain_nr(struct device_node *node);
> int of_pci_get_max_link_speed(struct device_node *node);
> void of_pci_check_probe_only(void);
> -int of_pci_map_rid(struct device_node *np, u32 rid,
> - const char *map_name, const char *map_mask_name,
> - struct device_node **target, u32 *id_out);
> #else
> static inline struct device_node *of_pci_find_child_device(struct device_node *parent,
> unsigned int devfn)
> @@ -44,13 +41,6 @@ static inline int of_pci_get_devfn(struct device_node *np)
> return -1;
> }
>
> -static inline int of_pci_map_rid(struct device_node *np, u32 rid,
> - const char *map_name, const char *map_mask_name,
> - struct device_node **target, u32 *id_out)
> -{
> - return -EINVAL;
> -}
> -
> static inline int
> of_pci_get_max_link_speed(struct device_node *node)
> {
> --
> 1.9.1
>

2018-05-01 05:44:52

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v4 5/6] bus: fsl-mc: supoprt dma configure for devices on fsl-mc bus

Hi Nipun,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.17-rc3 next-20180430]
[cannot apply to iommu/next glikely/devicetree/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Nipun-Gupta/Support-for-fsl-mc-bus-and-its-devices-in-SMMU/20180501-125745
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64

All errors (new ones prefixed by >>):

drivers/bus/fsl-mc/fsl-mc-bus.c: In function 'fsl_mc_dma_configure':
>> drivers/bus/fsl-mc/fsl-mc-bus.c:137:9: error: too many arguments to function 'of_dma_configure'
return of_dma_configure(dev, dma_dev->of_node, 0);
^~~~~~~~~~~~~~~~
In file included from drivers/bus/fsl-mc/fsl-mc-bus.c:13:0:
include/linux/of_device.h:58:5: note: declared here
int of_dma_configure(struct device *dev, struct device_node *np);
^~~~~~~~~~~~~~~~
drivers/bus/fsl-mc/fsl-mc-bus.c: At top level:
>> drivers/bus/fsl-mc/fsl-mc-bus.c:161:3: error: 'struct bus_type' has no member named 'dma_configure'
.dma_configure = fsl_mc_dma_configure,
^~~~~~~~~~~~~

vim +/of_dma_configure +137 drivers/bus/fsl-mc/fsl-mc-bus.c

129
130 static int fsl_mc_dma_configure(struct device *dev)
131 {
132 struct device *dma_dev = dev;
133
134 while (dev_is_fsl_mc(dma_dev))
135 dma_dev = dma_dev->parent;
136
> 137 return of_dma_configure(dev, dma_dev->of_node, 0);
138 }
139
140 static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
141 char *buf)
142 {
143 struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
144
145 return sprintf(buf, "fsl-mc:v%08Xd%s\n", mc_dev->obj_desc.vendor,
146 mc_dev->obj_desc.type);
147 }
148 static DEVICE_ATTR_RO(modalias);
149
150 static struct attribute *fsl_mc_dev_attrs[] = {
151 &dev_attr_modalias.attr,
152 NULL,
153 };
154
155 ATTRIBUTE_GROUPS(fsl_mc_dev);
156
157 struct bus_type fsl_mc_bus_type = {
158 .name = "fsl-mc",
159 .match = fsl_mc_bus_match,
160 .uevent = fsl_mc_bus_uevent,
> 161 .dma_configure = fsl_mc_dma_configure,
162 .dev_groups = fsl_mc_dev_groups,
163 };
164 EXPORT_SYMBOL_GPL(fsl_mc_bus_type);
165

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (2.67 kB)
.config.gz (61.62 kB)
Download all attachments

2018-05-01 12:45:26

by Rob Herring (Arm)

[permalink] [raw]

> -----Original Message-----
> From: Laurentiu Tudor
> Sent: Monday, May 14, 2018 7:10 PM
> To: Nipun Gupta <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> foundation.org; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; Bharat Bhushan <[email protected]>;
> [email protected]; Leo Li <[email protected]>
> Subject: Re: [PATCH 5/6 v3] bus: fsl-mc: supoprt dma configure for devices
> on fsl-mc bus
>
> Hi Nipun,
>
> On 04/27/2018 01:27 PM, Nipun Gupta wrote:
> > Signed-off-by: Nipun Gupta <[email protected]>
> > ---
> > drivers/bus/fsl-mc/fsl-mc-bus.c | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-
> bus.c
> > index 5d8266c..624828b 100644
> > --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> > +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> > @@ -127,6 +127,16 @@ static int fsl_mc_bus_uevent(struct device *dev,
> struct kobj_uevent_env *env)
> > return 0;
> > }
> >
> > +static int fsl_mc_dma_configure(struct device *dev)
> > +{
> > + struct device *dma_dev = dev;
> > +
> > + while (dev_is_fsl_mc(dma_dev))
> > + dma_dev = dma_dev->parent;
> > +
> > + return of_dma_configure(dev, dma_dev->of_node, 0);
> > +}
> > +
> > static ssize_t modalias_show(struct device *dev, struct device_attribute
> *attr,
> > char *buf)
> > {
> > @@ -148,6 +158,7 @@ struct bus_type fsl_mc_bus_type = {
> > .name = "fsl-mc",
> > .match = fsl_mc_bus_match,
> > .uevent = fsl_mc_bus_uevent,
> > + .dma_configure = fsl_mc_dma_configure,
> > .dev_groups = fsl_mc_dev_groups,
> > };
> > EXPORT_SYMBOL_GPL(fsl_mc_bus_type);
> > @@ -616,6 +627,7 @@ int fsl_mc_device_add(struct fsl_mc_obj_desc
> *obj_desc,
> > mc_dev->icid = parent_mc_dev->icid;
> > mc_dev->dma_mask = FSL_MC_DEFAULT_DMA_MASK;
> > mc_dev->dev.dma_mask = &mc_dev->dma_mask;
> > + mc_dev->dev.coherent_dma_mask = mc_dev->dma_mask;
>
> This change seems a bit unrelated to the patch subject. I wonder if it
> makes sense to split it it in a distinct patch.

Okay. I will spin a v5 after splitting this patch and adding changelog (Greg's comment), fixing typo (Bjorn's comment).

Regards,
Nipun

>
> ---
> Best Regards, Laurentiu