[PATCH v1 0/3] of_devlink: Minor fixes and new properties

[PATCH v1 0/3] of_devlink: Minor fixes and new properties

[Saravana Kannan]

Sending again since the cover letter missed everyone/mailing lists.

First two patches are just code clean up and logging with no functional
difference. The 3rd patch adds support for the following DT properties:
iommus, mboxes and io-channels.

These patches are on top of driver-core-next since that's where the rest
of the of_devlink patches are.

Greg and Rob,
On a side note, I was wondering if I should rename the of_devlink kernel
command line to fw_devlink and move it out of drivers/of/property.c and
into drivers/base/core.c. This feature isn't really limited to
devicetree, so I don't see a need to have devicetree specific kernel
command line option.  Please let me know if that sounds okay to you.

Thanks,
Saravana

Saravana Kannan (3):
  of: property: Minor style clean up of of_link_to_phandle()
  of: property: Make it easy to add device links from DT properties
  of: property: Add device link support for iommus, mboxes and
    io-channels

 drivers/of/property.c | 78 +++++++++++++++++++++++++++++++++----------
 1 file changed, 61 insertions(+), 17 deletions(-)

-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH v1 1/3] of: property: Minor style clean up of of_link_to_phandle()

[Saravana Kannan]

Adding a debug log instead of silently ignoring a phandle for an early
device. Also, return the right error code instead of 0 even though the
actual execution flow won't change.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/property.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index e225ab17f598..fbc201330ba0 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1051,8 +1051,14 @@ static int of_link_to_phandle(struct device *dev, struct device_node *sup_np,
 	sup_dev = get_dev_from_fwnode(&sup_np->fwnode);
 	is_populated = of_node_check_flag(sup_np, OF_POPULATED);
 	of_node_put(sup_np);
-	if (!sup_dev)
-		return is_populated ? 0 : -EAGAIN;
+	if (!sup_dev && is_populated) {
+		/* Early device without struct device. */
+		dev_dbg(dev, "Not linking to %pOFP - No struct device\n",
+			sup_np);
+		return -ENODEV;
+	} else if (!sup_dev) {
+		return -EAGAIN;
+	}
 	if (!device_link_add(dev, sup_dev, dl_flags))
 		ret = -EAGAIN;
 	put_device(sup_dev);
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

Re: [PATCH v1 1/3] of: property: Minor style clean up of of_link_to_phandle()

[Rob Herring]

On Mon,  4 Nov 2019 22:49:58 -0800, Saravana Kannan wrote:
> Adding a debug log instead of silently ignoring a phandle for an early
> device. Also, return the right error code instead of 0 even though the
> actual execution flow won't change.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/property.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v1 2/3] of: property: Make it easy to add device links from DT properties

[Saravana Kannan]

Add a DEFINE_SIMPLE_PROP macro to make it easy to add support for simple
properties with fixed names that just list phandles and phandle args.

Add a DEFINE_SUFFIX_PROP macro to make it easy to add support for
properties with fixes suffix that just list phandles and phandle args.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/property.c | 62 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 47 insertions(+), 15 deletions(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index fbc201330ba0..812b69a029d1 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1101,17 +1101,11 @@ static struct device_node *parse_prop_cells(struct device_node *np,
 	return sup_args.np;
 }
 
-static struct device_node *parse_clocks(struct device_node *np,
-					const char *prop_name, int index)
-{
-	return parse_prop_cells(np, prop_name, index, "clocks", "#clock-cells");
-}
-
-static struct device_node *parse_interconnects(struct device_node *np,
-					       const char *prop_name, int index)
-{
-	return parse_prop_cells(np, prop_name, index, "interconnects",
-				"#interconnect-cells");
+#define DEFINE_SIMPLE_PROP(fname, name, cells)				  \
+static struct device_node *parse_##fname(struct device_node *np,	  \
+					const char *prop_name, int index) \
+{									  \
+	return parse_prop_cells(np, prop_name, index, name, cells);	  \
 }
 
 static int strcmp_suffix(const char *str, const char *suffix)
@@ -1125,13 +1119,47 @@ static int strcmp_suffix(const char *str, const char *suffix)
 	return strcmp(str + len - suffix_len, suffix);
 }
 
-static struct device_node *parse_regulators(struct device_node *np,
-					    const char *prop_name, int index)
+/**
+ * parse_suffix_prop_cells - Suffix property parsing function for suppliers
+ *
+ * @np:		Pointer to device tree node containing a list
+ * @prop_name:	Name of property to be parsed. Expected to hold phandle values
+ * @index:	For properties holding a list of phandles, this is the index
+ *		into the list.
+ * @suffix:	Property suffix that is known to contain list of phandle(s) to
+ *		supplier(s)
+ * @cells_name:	property name that specifies phandles' arguments count
+ *
+ * This is a helper function to parse properties that have a known fixed suffix
+ * and are a list of phandles and phandle arguments.
+ *
+ * Returns:
+ * - phandle node pointer with refcount incremented. Caller must of_node_put()
+ *   on it when done.
+ * - NULL if no phandle found at index
+ */
+static struct device_node *parse_suffix_prop_cells(struct device_node *np,
+					    const char *prop_name, int index,
+					    const char *suffix,
+					    const char *cells_name)
 {
-	if (index || strcmp_suffix(prop_name, "-supply"))
+	struct of_phandle_args sup_args;
+
+	if (strcmp_suffix(prop_name, suffix))
 		return NULL;
 
-	return of_parse_phandle(np, prop_name, 0);
+	if (of_parse_phandle_with_args(np, prop_name, cells_name, index,
+				       &sup_args))
+		return NULL;
+
+	return sup_args.np;
+}
+
+#define DEFINE_SUFFIX_PROP(fname, suffix, cells)			     \
+static struct device_node *parse_##fname(struct device_node *np,	     \
+					const char *prop_name, int index)    \
+{									     \
+	return parse_suffix_prop_cells(np, prop_name, index, suffix, cells); \
 }
 
 /**
@@ -1155,6 +1183,10 @@ struct supplier_bindings {
 					  const char *prop_name, int index);
 };
 
+DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
+DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
+DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
+
 static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_clocks, },
 	{ .parse_prop = parse_interconnects, },
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

Re: [PATCH v1 2/3] of: property: Make it easy to add device links from DT properties

[Rob Herring]

On Mon,  4 Nov 2019 22:49:59 -0800, Saravana Kannan wrote:
> Add a DEFINE_SIMPLE_PROP macro to make it easy to add support for simple
> properties with fixed names that just list phandles and phandle args.
> 
> Add a DEFINE_SUFFIX_PROP macro to make it easy to add support for
> properties with fixes suffix that just list phandles and phandle args.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/property.c | 62 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 47 insertions(+), 15 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v1 3/3] of: property: Add device link support for iommus, mboxes and io-channels

[Saravana Kannan]

Add support for creating device links out of more DT properties.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/property.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 812b69a029d1..0fa04692e3cc 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1185,11 +1185,17 @@ struct supplier_bindings {
 
 DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
 DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
+DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
+DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
+DEFINE_SIMPLE_PROP(io_channels, "io-channel", "#io-channel-cells")
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
 
 static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_clocks, },
 	{ .parse_prop = parse_interconnects, },
+	{ .parse_prop = parse_iommus, },
+	{ .parse_prop = parse_mboxes, },
+	{ .parse_prop = parse_io_channels, },
 	{ .parse_prop = parse_regulators, },
 	{}
 };
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

Re: [PATCH v1 3/3] of: property: Add device link support for iommus, mboxes and io-channels

[Rob Herring]

On Mon,  4 Nov 2019 22:50:00 -0800, Saravana Kannan wrote:
> Add support for creating device links out of more DT properties.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/property.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v1 0/3] of_devlink: Minor fixes and new properties

[Saravana Kannan]

On Mon, Nov 4, 2019 at 10:50 PM Saravana Kannan <saravanak@google.com> wrote:
>
> Sending again since the cover letter missed everyone/mailing lists.
>
> First two patches are just code clean up and logging with no functional
> difference. The 3rd patch adds support for the following DT properties:
> iommus, mboxes and io-channels.
>
> These patches are on top of driver-core-next since that's where the rest
> of the of_devlink patches are.
>
> Greg and Rob,
> On a side note, I was wondering if I should rename the of_devlink kernel
> command line to fw_devlink and move it out of drivers/of/property.c and
> into drivers/base/core.c. This feature isn't really limited to
> devicetree, so I don't see a need to have devicetree specific kernel
> command line option.  Please let me know if that sounds okay to you.

Hi Rob,

Thanks for the reviews. Can you let me know what you think of this too?

Rob/Greg,

If I rename of_devlink to fw_devlink, I might also make it a setting
like fw_devlink=none/permissive/enforce
- none would be same as disabled completely.
- permissive would use SYNC_STATE_ONLY for all device links created by
firmware. So it won't block any probes even with cycles but
sync_state() will still work correctly.
- enforce would be the current "of_devlinkg=1" behavior where direct
dependencies would block probing and the child dependencies would use
SYNC_STATE_ONLY.

-Saravana

Re: [PATCH v1 0/3] of_devlink: Minor fixes and new properties

[Greg Kroah-Hartman]

On Wed, Nov 06, 2019 at 09:53:40PM -0800, Saravana Kannan wrote:
> On Mon, Nov 4, 2019 at 10:50 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > Sending again since the cover letter missed everyone/mailing lists.
> >
> > First two patches are just code clean up and logging with no functional
> > difference. The 3rd patch adds support for the following DT properties:
> > iommus, mboxes and io-channels.
> >
> > These patches are on top of driver-core-next since that's where the rest
> > of the of_devlink patches are.
> >
> > Greg and Rob,
> > On a side note, I was wondering if I should rename the of_devlink kernel
> > command line to fw_devlink and move it out of drivers/of/property.c and
> > into drivers/base/core.c. This feature isn't really limited to
> > devicetree, so I don't see a need to have devicetree specific kernel
> > command line option.  Please let me know if that sounds okay to you.
> 
> Hi Rob,
> 
> Thanks for the reviews. Can you let me know what you think of this too?
> 
> Rob/Greg,
> 
> If I rename of_devlink to fw_devlink, I might also make it a setting
> like fw_devlink=none/permissive/enforce
> - none would be same as disabled completely.
> - permissive would use SYNC_STATE_ONLY for all device links created by
> firmware. So it won't block any probes even with cycles but
> sync_state() will still work correctly.
> - enforce would be the current "of_devlinkg=1" behavior where direct
> dependencies would block probing and the child dependencies would use
> SYNC_STATE_ONLY.

Renaming makes sense to me, and all of the above is fine as well.

thanks,

greg k-h

[PATCH v1 2/3] of: property: Make it easy to add device links from DT properties

[Saravana Kannan]

Add a DEFINE_SIMPLE_PROP macro to make it easy to add support for simple
properties with fixed names that just list phandles and phandle args.

Add a DEFINE_SUFFIX_PROP macro to make it easy to add support for
properties with fixes suffix that just list phandles and phandle args.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/property.c | 62 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 47 insertions(+), 15 deletions(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index fbc201330ba0..812b69a029d1 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1101,17 +1101,11 @@ static struct device_node *parse_prop_cells(struct device_node *np,
 	return sup_args.np;
 }
 
-static struct device_node *parse_clocks(struct device_node *np,
-					const char *prop_name, int index)
-{
-	return parse_prop_cells(np, prop_name, index, "clocks", "#clock-cells");
-}
-
-static struct device_node *parse_interconnects(struct device_node *np,
-					       const char *prop_name, int index)
-{
-	return parse_prop_cells(np, prop_name, index, "interconnects",
-				"#interconnect-cells");
+#define DEFINE_SIMPLE_PROP(fname, name, cells)				  \
+static struct device_node *parse_##fname(struct device_node *np,	  \
+					const char *prop_name, int index) \
+{									  \
+	return parse_prop_cells(np, prop_name, index, name, cells);	  \
 }
 
 static int strcmp_suffix(const char *str, const char *suffix)
@@ -1125,13 +1119,47 @@ static int strcmp_suffix(const char *str, const char *suffix)
 	return strcmp(str + len - suffix_len, suffix);
 }
 
-static struct device_node *parse_regulators(struct device_node *np,
-					    const char *prop_name, int index)
+/**
+ * parse_suffix_prop_cells - Suffix property parsing function for suppliers
+ *
+ * @np:		Pointer to device tree node containing a list
+ * @prop_name:	Name of property to be parsed. Expected to hold phandle values
+ * @index:	For properties holding a list of phandles, this is the index
+ *		into the list.
+ * @suffix:	Property suffix that is known to contain list of phandle(s) to
+ *		supplier(s)
+ * @cells_name:	property name that specifies phandles' arguments count
+ *
+ * This is a helper function to parse properties that have a known fixed suffix
+ * and are a list of phandles and phandle arguments.
+ *
+ * Returns:
+ * - phandle node pointer with refcount incremented. Caller must of_node_put()
+ *   on it when done.
+ * - NULL if no phandle found at index
+ */
+static struct device_node *parse_suffix_prop_cells(struct device_node *np,
+					    const char *prop_name, int index,
+					    const char *suffix,
+					    const char *cells_name)
 {
-	if (index || strcmp_suffix(prop_name, "-supply"))
+	struct of_phandle_args sup_args;
+
+	if (strcmp_suffix(prop_name, suffix))
 		return NULL;
 
-	return of_parse_phandle(np, prop_name, 0);
+	if (of_parse_phandle_with_args(np, prop_name, cells_name, index,
+				       &sup_args))
+		return NULL;
+
+	return sup_args.np;
+}
+
+#define DEFINE_SUFFIX_PROP(fname, suffix, cells)			     \
+static struct device_node *parse_##fname(struct device_node *np,	     \
+					const char *prop_name, int index)    \
+{									     \
+	return parse_suffix_prop_cells(np, prop_name, index, suffix, cells); \
 }
 
 /**
@@ -1155,6 +1183,10 @@ struct supplier_bindings {
 					  const char *prop_name, int index);
 };
 
+DEFINE_SIMPLE_PROP(clocks, "clocks", "#clock-cells")
+DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
+DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
+
 static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_clocks, },
 	{ .parse_prop = parse_interconnects, },
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog