[RESEND][PATCH 0/4] firmware: of: populate /firmware/ node during init

[RESEND][PATCH 0/4] firmware: of: populate /firmware/ node during init

[Sudeep Holla]

Hi Rob, Arnd,

There's a push to place all firmware related device node under
/firmware/ node. However all the associated drivers are dealing with
device creation in their own ways. For example, qcom_scm, optee and
meson-sm drivers deal with the node in their own way. The SCMI drivers
I am about to add also needs to do the same. This small series is to
make it generic so that all the users of /firmware/ node need not
repeat the same.

There was concern that /firmware in DT is fit for /sys/firmware in sysfs.
IMO anything exposed to userspace from such firmware interface must
be in "/sys/firmware", but I don't see any issue with kernel handling
them as platform device/driver internally.

Sudeep Holla (4):
  of: platform: populate /firmware/ node from
    of_platform_default_populate_init()
  firmware: qcom_scm: drop redandant of_platform_populate
  drivers: tee: rework optee_driver_{init,exit} to use platform device
  firmware: meson-sm: rework meson_sm_init to use
    module_platform_driver_probe

 drivers/firmware/meson/meson_sm.c | 21 ++++++-----
 drivers/firmware/qcom_scm.c       | 24 -------------
 drivers/of/platform.c             |  4 +++
 drivers/tee/optee/core.c          | 74 +++++++++++++--------------------------
 4 files changed, 41 insertions(+), 82 deletions(-)

--
2.7.4

[RESEND][PATCH 1/4] of: platform: populate /firmware/ node from of_platform_default_populate_init()

[Sudeep Holla]

Since "/firmware" does not have its own "compatible" property as it's
just collection of nodes representing firmware interface, it's sub-nodes
are not populated during system initialization.

Currently different firmware drivers search the /firmware/ node and
populate the sub-node devices selectively. Instead we can populate
the /firmware/ node during init to avoid more drivers continuing to
populate the devices selectively.

To generalize the solution this patch populates the /firmware/ node
explicitly from of_platform_default_populate_init().

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/of/platform.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index ac15d0e3d27d..3a213a6aee89 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -515,6 +515,10 @@ static int __init of_platform_default_populate_init(void)
 			of_platform_device_create(node, NULL, NULL);
 	}
 
+	node = of_find_node_by_path("/firmware");
+	if (node)
+		of_platform_populate(node, NULL, NULL, NULL);
+
 	/* Populate everything else. */
 	of_platform_default_populate(NULL, NULL, NULL);
 
-- 
2.7.4

[RESEND][PATCH 1/4] of: platform: populate /firmware/ node from of_platform_default_populate_init()

[Frank Rowand]

+ me

On 09/28/17 03:45, Sudeep Holla wrote:
> Since "/firmware" does not have its own "compatible" property as it's
> just collection of nodes representing firmware interface, it's sub-nodes
> are not populated during system initialization.
> 
> Currently different firmware drivers search the /firmware/ node and
> populate the sub-node devices selectively. Instead we can populate
> the /firmware/ node during init to avoid more drivers continuing to
> populate the devices selectively.
> 
> To generalize the solution this patch populates the /firmware/ node
> explicitly from of_platform_default_populate_init().
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/of/platform.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index ac15d0e3d27d..3a213a6aee89 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -515,6 +515,10 @@ static int __init of_platform_default_populate_init(void)
>  			of_platform_device_create(node, NULL, NULL);
>  	}
>  
> +	node = of_find_node_by_path("/firmware");
> +	if (node)
> +		of_platform_populate(node, NULL, NULL, NULL);
> +
>  	/* Populate everything else. */
>  	of_platform_default_populate(NULL, NULL, NULL);
>  
> 

[RESEND][PATCH 1/4] of: platform: populate /firmware/ node from of_platform_default_populate_init()

[Rob Herring]

On Thu, Sep 28, 2017 at 5:45 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> Since "/firmware" does not have its own "compatible" property as it's
> just collection of nodes representing firmware interface, it's sub-nodes
> are not populated during system initialization.
>
> Currently different firmware drivers search the /firmware/ node and
> populate the sub-node devices selectively. Instead we can populate
> the /firmware/ node during init to avoid more drivers continuing to
> populate the devices selectively.
>
> To generalize the solution this patch populates the /firmware/ node
> explicitly from of_platform_default_populate_init().
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/of/platform.c | 4 ++++
>  1 file changed, 4 insertions(+)

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

[RESEND][PATCH 1/4] of: platform: populate /firmware/ node from of_platform_default_populate_init()

[Sudeep Holla]

Hi Arnd,

On 17/10/17 14:02, Rob Herring wrote:
> On Thu, Sep 28, 2017 at 5:45 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> Since "/firmware" does not have its own "compatible" property as it's
>> just collection of nodes representing firmware interface, it's sub-nodes
>> are not populated during system initialization.
>>
>> Currently different firmware drivers search the /firmware/ node and
>> populate the sub-node devices selectively. Instead we can populate
>> the /firmware/ node during init to avoid more drivers continuing to
>> populate the devices selectively.
>>
>> To generalize the solution this patch populates the /firmware/ node
>> explicitly from of_platform_default_populate_init().
>>

You had concernsin the past and I mentioned that firmware interface to
user-space *has* to continue in /sys/firmware, but he platform_device
management is internal to kernel.

Can we proceed with this change at-least for qcom and meson firmware
which are platform device already ? I can drop optee related changes.

-- 
Regards,
Sudeep

[RESEND][PATCH 1/4] of: platform: populate /firmware/ node from of_platform_default_populate_init()

[Bjorn Andersson]

On Thu, Sep 28, 2017 at 3:45 AM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> Since "/firmware" does not have its own "compatible" property as it's
> just collection of nodes representing firmware interface, it's sub-nodes
> are not populated during system initialization.
>
> Currently different firmware drivers search the /firmware/ node and
> populate the sub-node devices selectively. Instead we can populate
> the /firmware/ node during init to avoid more drivers continuing to
> populate the devices selectively.
>
> To generalize the solution this patch populates the /firmware/ node
> explicitly from of_platform_default_populate_init().
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Rob Herring <robh@kernel.org>

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---

Andy, as you picked patch 2/4 into your v4.16 -next branch we no
longer probe the qcom_scm device, causing various breakage. Can you
please pick this patch as well (it has Rob's ack).

Regards,
Bjorn

>  drivers/of/platform.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index ac15d0e3d27d..3a213a6aee89 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -515,6 +515,10 @@ static int __init of_platform_default_populate_init(void)
>                         of_platform_device_create(node, NULL, NULL);
>         }
>
> +       node = of_find_node_by_path("/firmware");
> +       if (node)
> +               of_platform_populate(node, NULL, NULL, NULL);
> +
>         /* Populate everything else. */
>         of_platform_default_populate(NULL, NULL, NULL);
>
> --
> 2.7.4
>

[RESEND][PATCH 2/4] firmware: qcom_scm: drop redandant of_platform_populate

[Sudeep Holla]

Now that of_platform_default_populate_init() takes care of populating
all the devices under the /firmware/ node, this patch removes the
redandant call to of_platform_populate here.

Cc: Andy Gross <andy.gross@linaro.org>
Cc: David Brown <david.brown@linaro.org>
Cc: linux-arm-msm at vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/qcom_scm.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index bb16510d75ba..a11e06e5cdb0 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -440,30 +440,6 @@ static struct platform_driver qcom_scm_driver = {
 
 static int __init qcom_scm_init(void)
 {
-	struct device_node *np, *fw_np;
-	int ret;
-
-	fw_np = of_find_node_by_name(NULL, "firmware");
-
-	if (!fw_np)
-		return -ENODEV;
-
-	np = of_find_matching_node(fw_np, qcom_scm_dt_match);
-
-	if (!np) {
-		of_node_put(fw_np);
-		return -ENODEV;
-	}
-
-	of_node_put(np);
-
-	ret = of_platform_populate(fw_np, qcom_scm_dt_match, NULL, NULL);
-
-	of_node_put(fw_np);
-
-	if (ret)
-		return ret;
-
 	return platform_driver_register(&qcom_scm_driver);
 }
 subsys_initcall(qcom_scm_init);
-- 
2.7.4

[RESEND][PATCH 3/4] drivers: tee: rework optee_driver_{init, exit} to use platform device

[Sudeep Holla]

Now that of_platform_default_populate_init() takes care of populating
all the devices under the /firmware/ node, this patch reworks/removes
custom optee_driver_{init,exit} in favour of module_platform_driver.

Cc: Jens Wiklander <jens.wiklander@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/tee/optee/core.c | 74 ++++++++++++++++--------------------------------
 1 file changed, 25 insertions(+), 49 deletions(-)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index 7952357df9c8..98be0e688949 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -446,8 +446,9 @@ static optee_invoke_fn *get_invoke_func(struct device_node *np)
 	return ERR_PTR(-EINVAL);
 }
 
-static struct optee *optee_probe(struct device_node *np)
+static int optee_probe(struct platform_device *pdev)
 {
+	struct device_node *np = pdev->dev.of_node;
 	optee_invoke_fn *invoke_fn;
 	struct tee_shm_pool *pool;
 	struct optee *optee = NULL;
@@ -458,21 +459,21 @@ static struct optee *optee_probe(struct device_node *np)
 
 	invoke_fn = get_invoke_func(np);
 	if (IS_ERR(invoke_fn))
-		return (void *)invoke_fn;
+		return PTR_ERR(invoke_fn);
 
 	if (!optee_msg_api_uid_is_optee_api(invoke_fn)) {
 		pr_warn("api uid mismatch\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
 	if (!optee_msg_api_revision_is_compatible(invoke_fn)) {
 		pr_warn("api revision mismatch\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
 	if (!optee_msg_exchange_capabilities(invoke_fn, &sec_caps)) {
 		pr_warn("capabilities mismatch\n");
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
 	/*
@@ -480,11 +481,11 @@ static struct optee *optee_probe(struct device_node *np)
 	 * doesn't have any reserved memory we can use we can't continue.
 	 */
 	if (!(sec_caps & OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM))
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 
 	pool = optee_config_shm_memremap(invoke_fn, &memremaped_shm);
 	if (IS_ERR(pool))
-		return (void *)pool;
+		return PTR_ERR(pool);
 
 	optee = kzalloc(sizeof(*optee), GFP_KERNEL);
 	if (!optee) {
@@ -524,9 +525,10 @@ static struct optee *optee_probe(struct device_node *np)
 	optee->pool = pool;
 
 	optee_enable_shm_cache(optee);
+	platform_set_drvdata(pdev, optee);
 
 	pr_info("initialized driver\n");
-	return optee;
+	return 0;
 err:
 	if (optee) {
 		/*
@@ -542,11 +544,12 @@ static struct optee *optee_probe(struct device_node *np)
 		tee_shm_pool_free(pool);
 	if (memremaped_shm)
 		memunmap(memremaped_shm);
-	return ERR_PTR(rc);
+	return rc;
 }
 
-static void optee_remove(struct optee *optee)
+static int optee_remove(struct platform_device *pdev)
 {
+	struct optee *optee = platform_get_drvdata(pdev);
 	/*
 	 * Ask OP-TEE to free all cached shared memory objects to decrease
 	 * reference counters and also avoid wild pointers in secure world
@@ -569,52 +572,25 @@ static void optee_remove(struct optee *optee)
 	mutex_destroy(&optee->call_queue.mutex);
 
 	kfree(optee);
+	return 0;
 }
 
 static const struct of_device_id optee_match[] = {
 	{ .compatible = "linaro,optee-tz" },
 	{},
 };
+MODULE_DEVICE_TABLE(of, optee_match);
+
+static struct platform_driver optee_platdrv = {
+	.driver = {
+		.name = "optee",
+		.of_match_table = of_match_ptr(optee_match),
+	},
+	.probe = optee_probe,
+	.remove	= optee_remove,
+};
 
-static struct optee *optee_svc;
-
-static int __init optee_driver_init(void)
-{
-	struct device_node *fw_np;
-	struct device_node *np;
-	struct optee *optee;
-
-	/* Node is supposed to be below /firmware */
-	fw_np = of_find_node_by_name(NULL, "firmware");
-	if (!fw_np)
-		return -ENODEV;
-
-	np = of_find_matching_node(fw_np, optee_match);
-	of_node_put(fw_np);
-	if (!np)
-		return -ENODEV;
-
-	optee = optee_probe(np);
-	of_node_put(np);
-
-	if (IS_ERR(optee))
-		return PTR_ERR(optee);
-
-	optee_svc = optee;
-
-	return 0;
-}
-module_init(optee_driver_init);
-
-static void __exit optee_driver_exit(void)
-{
-	struct optee *optee = optee_svc;
-
-	optee_svc = NULL;
-	if (optee)
-		optee_remove(optee);
-}
-module_exit(optee_driver_exit);
+module_platform_driver(optee_platdrv);
 
 MODULE_AUTHOR("Linaro");
 MODULE_DESCRIPTION("OP-TEE driver");
-- 
2.7.4

[RESEND][PATCH 4/4] firmware: meson-sm: rework meson_sm_init to use module_platform_driver_probe

[Sudeep Holla]

Now that of_platform_default_populate_init() takes care of populating
all the devices under the /firmware/ node, this patch reworks
meson_sm_init in favour of module_platform_driver_probe.

Cc: Carlo Caione <carlo@caione.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: linux-amlogic at lists.infradead.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/meson/meson_sm.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
index ff204421117b..ccd61be0dd49 100644
--- a/drivers/firmware/meson/meson_sm.c
+++ b/drivers/firmware/meson/meson_sm.c
@@ -17,8 +17,10 @@
 #include <linux/arm-smccc.h>
 #include <linux/bug.h>
 #include <linux/io.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/platform_device.h>
 #include <linux/printk.h>
 #include <linux/types.h>
 #include <linux/sizes.h>
@@ -217,17 +219,11 @@ static const struct of_device_id meson_sm_ids[] = {
 	{ /* sentinel */ },
 };
 
-int __init meson_sm_init(void)
+static int __init meson_sm_probe(struct platform_device *pdev)
 {
 	const struct meson_sm_chip *chip;
-	const struct of_device_id *matched_np;
-	struct device_node *np;
 
-	np = of_find_matching_node_and_match(NULL, meson_sm_ids, &matched_np);
-	if (!np)
-		return -ENODEV;
-
-	chip = matched_np->data;
+	chip = of_match_device(meson_sm_ids, &pdev->dev)->data;
 	if (!chip) {
 		pr_err("unable to setup secure-monitor data\n");
 		goto out;
@@ -257,4 +253,11 @@ int __init meson_sm_init(void)
 out:
 	return -EINVAL;
 }
-device_initcall(meson_sm_init);
+
+static struct platform_driver meson_sm_driver = {
+	.driver = {
+		.name = "meson-sm",
+		.of_match_table = of_match_ptr(meson_sm_ids),
+	},
+};
+module_platform_driver_probe(meson_sm_driver, meson_sm_probe);
-- 
2.7.4

[RESEND][PATCH 0/4] firmware: of: populate /firmware/ node during init

[Frank Rowand]

+ me

On 09/28/17 03:45, Sudeep Holla wrote:
> Hi Rob, Arnd,
> 
> There's a push to place all firmware related device node under
> /firmware/ node. However all the associated drivers are dealing with
> device creation in their own ways. For example, qcom_scm, optee and
> meson-sm drivers deal with the node in their own way. The SCMI drivers
> I am about to add also needs to do the same. This small series is to
> make it generic so that all the users of /firmware/ node need not
> repeat the same.
> 
> There was concern that /firmware in DT is fit for /sys/firmware in sysfs.
> IMO anything exposed to userspace from such firmware interface must
> be in "/sys/firmware", but I don't see any issue with kernel handling
> them as platform device/driver internally.
> 
> Sudeep Holla (4):
>   of: platform: populate /firmware/ node from
>     of_platform_default_populate_init()
>   firmware: qcom_scm: drop redandant of_platform_populate
>   drivers: tee: rework optee_driver_{init,exit} to use platform device
>   firmware: meson-sm: rework meson_sm_init to use
>     module_platform_driver_probe
> 
>  drivers/firmware/meson/meson_sm.c | 21 ++++++-----
>  drivers/firmware/qcom_scm.c       | 24 -------------
>  drivers/of/platform.c             |  4 +++
>  drivers/tee/optee/core.c          | 74 +++++++++++++--------------------------
>  4 files changed, 41 insertions(+), 82 deletions(-)
> 
> --
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[RESEND][PATCH 0/4] firmware: of: populate /firmware/ node during init

[Sudeep Holla]

Hi Rob, Arnd,

There's a push to place all firmware related device node under
/firmware/ node. However all the associated drivers are dealing with
device creation in their own ways. For example, qcom_scm, optee and
meson-sm drivers deal with the node in their own way. The SCMI drivers
I am about to add also needs to do the same. This small series is to
make it generic so that all the users of /firmware/ node need not
repeat the same.

There was concern that /firmware in DT is fit for /sys/firmware in sysfs.
IMO anything exposed to userspace from such firmware interface must
be in "/sys/firmware", but I don't see any issue with kernel handling
them as platform device/driver internally.

Sudeep Holla (4):
  of: platform: populate /firmware/ node from
    of_platform_default_populate_init()
  firmware: qcom_scm: drop redandant of_platform_populate
  drivers: tee: rework optee_driver_{init,exit} to use platform device
  firmware: meson-sm: rework meson_sm_init to use
    module_platform_driver_probe

 drivers/firmware/meson/meson_sm.c | 21 ++++++-----
 drivers/firmware/qcom_scm.c       | 24 -------------
 drivers/of/platform.c             |  4 +++
 drivers/tee/optee/core.c          | 74 +++++++++++++--------------------------
 4 files changed, 41 insertions(+), 82 deletions(-)

--
2.7.4

[RESEND][PATCH 0/4] firmware: of: populate /firmware/ node during init

[Sudeep Holla]

On 28/09/17 11:44, Sudeep Holla wrote:
> Hi Rob, Arnd,
> 
> There's a push to place all firmware related device node under
> /firmware/ node. However all the associated drivers are dealing with
> device creation in their own ways. For example, qcom_scm, optee and
> meson-sm drivers deal with the node in their own way. The SCMI drivers
> I am about to add also needs to do the same. This small series is to
> make it generic so that all the users of /firmware/ node need not
> repeat the same.
> 
> There was concern that /firmware in DT is fit for /sys/firmware in sysfs.
> IMO anything exposed to userspace from such firmware interface must
> be in "/sys/firmware", but I don't see any issue with kernel handling
> them as platform device/driver internally.
> 

Please ignore this, something went wrong and patches were not delivered,
have reposted it separately.

-- 
Regards,
Sudeep