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

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

[Sudeep Holla]

Hi Rob, Arnd,

Recently Rob pointed to place SCMI device node under /firmware/ node
and when I did that, I observed I had to populate the node first to
get scmi node populated. Some research showed qcom_scm and optee drivers
handle it in their own ways. This small series is to make it generic
so that all the users of /firmware/ node need not repeat the same.

Sudeep Holla (3):
  firmware: of: populate /firmware/ node during init
  firmware: qcom_scm: drop redandant of_platform_populate
  drivers: tee: rework optee_driver_{init,exit} to use platform device

 drivers/firmware/Makefile   |  1 +
 drivers/firmware/of.c       | 34 +++++++++++++++++++++
 drivers/firmware/qcom_scm.c | 24 ---------------
 drivers/tee/optee/core.c    | 74 +++++++++++++++------------------------------
 4 files changed, 60 insertions(+), 73 deletions(-)
 create mode 100644 drivers/firmware/of.c

-- 
2.7.4

[PATCH 1/3] firmware: of: populate /firmware/ node during 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.

This patch adds initcall to achieve the same.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Rob Herring <robh@kernel.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/Makefile |  1 +
 drivers/firmware/of.c     | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 drivers/firmware/of.c

diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 91d3ff62c653..d9a6fce43613 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_ISCSI_IBFT)	+= iscsi_ibft.o
 obj-$(CONFIG_FIRMWARE_MEMMAP)	+= memmap.o
 obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
 obj-$(CONFIG_FW_CFG_SYSFS)	+= qemu_fw_cfg.o
+obj-$(CONFIG_OF)		+= of.o
 obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o
 obj-$(CONFIG_QCOM_SCM_64)	+= qcom_scm-64.o
 obj-$(CONFIG_QCOM_SCM_32)	+= qcom_scm-32.o
diff --git a/drivers/firmware/of.c b/drivers/firmware/of.c
new file mode 100644
index 000000000000..149b9660fb44
--- /dev/null
+++ b/drivers/firmware/of.c
@@ -0,0 +1,34 @@
+/*
+ * Populates the nodes under /firmware/ device tree node
+ *
+ * Copyright (C) 2017 ARM Ltd.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Released under the GPLv2 only.
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include <linux/init.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+
+static int __init firmware_of_init(void)
+{
+	struct device_node *fw_np;
+	int ret;
+
+	fw_np = of_find_node_by_name(NULL, "firmware");
+
+	if (!fw_np)
+		return 0;
+
+	ret = of_platform_populate(fw_np, NULL, NULL, NULL);
+
+	of_node_put(fw_np);
+
+	return ret;
+}
+arch_initcall_sync(firmware_of_init);
-- 
2.7.4

[PATCH 2/3] firmware: qcom_scm: drop redandant of_platform_populate

[Sudeep Holla]

Now that firmware_of_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>
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

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

[Sudeep Holla]

Now that firmware_of_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 58169e519422..6c368508e835 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -445,8 +445,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;
@@ -457,21 +458,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;
 	}
 
 	/*
@@ -479,11 +480,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) {
@@ -523,9 +524,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) {
 		/*
@@ -541,11 +543,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
@@ -568,52 +571,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

Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init

[Rob Herring]

+Bjorn

On Fri, Aug 11, 2017 at 8:30 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.
>
> This patch adds initcall to achieve the same.
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/Makefile |  1 +
>  drivers/firmware/of.c     | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
>  create mode 100644 drivers/firmware/of.c
>
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 91d3ff62c653..d9a6fce43613 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_ISCSI_IBFT)      += iscsi_ibft.o
>  obj-$(CONFIG_FIRMWARE_MEMMAP)  += memmap.o
>  obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
>  obj-$(CONFIG_FW_CFG_SYSFS)     += qemu_fw_cfg.o
> +obj-$(CONFIG_OF)               += of.o
>  obj-$(CONFIG_QCOM_SCM)         += qcom_scm.o
>  obj-$(CONFIG_QCOM_SCM_64)      += qcom_scm-64.o
>  obj-$(CONFIG_QCOM_SCM_32)      += qcom_scm-32.o
> diff --git a/drivers/firmware/of.c b/drivers/firmware/of.c
> new file mode 100644
> index 000000000000..149b9660fb44
> --- /dev/null
> +++ b/drivers/firmware/of.c
> @@ -0,0 +1,34 @@
> +/*
> + * Populates the nodes under /firmware/ device tree node
> + *
> + * Copyright (C) 2017 ARM Ltd.
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *
> + * Released under the GPLv2 only.

Why do you have both the above 2 paragraphs and an SPDX tag? I'd drop
the above (unless your lawyers told you otherwise).

> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#include <linux/init.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +
> +static int __init firmware_of_init(void)

I'd prefer this is added to of_platform_default_populate_init() and
handled like /reserved-memory.

But be aware that Bjorn is reworking this function[1].

> +{
> +       struct device_node *fw_np;
> +       int ret;
> +
> +       fw_np = of_find_node_by_name(NULL, "firmware");

This matches any node named firmware. I see a few instances like RPi
that are not /firmware (though RPi we can move probably).
of_find_node_by_path would be safer.

> +
> +       if (!fw_np)
> +               return 0;
> +
> +       ret = of_platform_populate(fw_np, NULL, NULL, NULL);
> +
> +       of_node_put(fw_np);
> +
> +       return ret;
> +}
> +arch_initcall_sync(firmware_of_init);

You perhaps missed a few instances. "amlogic,meson-gxbb-sm" looks like
it could be converted to a driver. "tlm,trusted-foundations" appears
to be needed early for bringing up cores, so it probably can't change.
They don't have to be converted as part of this series though.

Rob

[1] https://lkml.org/lkml/2017/8/2/981

Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init

[Arnd Bergmann]

On Fri, Aug 11, 2017 at 3:30 PM, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> 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.
>
> This patch adds initcall to achieve the same.

Hmm, I'm a bit skeptical whether representing anything under /firmware
as a platform device is a good idea. Having a more structured way to
probe those seems like a good idea, but maybe a different subsystem
would be more appropriate.

I do realize that a 'platform_device' has become a rather generic abstraction
for almost anything, but at some point we might want to draw the line
of what is a platform_device.

       Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init

[Arnd Bergmann]

On Fri, Aug 11, 2017 at 3:30 PM, 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.
>
> This patch adds initcall to achieve the same.

Hmm, I'm a bit skeptical whether representing anything under /firmware
as a platform device is a good idea. Having a more structured way to
probe those seems like a good idea, but maybe a different subsystem
would be more appropriate.

I do realize that a 'platform_device' has become a rather generic abstraction
for almost anything, but at some point we might want to draw the line
of what is a platform_device.

       Arnd

Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init

[Rob Herring]

On Fri, Aug 11, 2017 at 9:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Aug 11, 2017 at 3:30 PM, 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.
>>
>> This patch adds initcall to achieve the same.
>
> Hmm, I'm a bit skeptical whether representing anything under /firmware
> as a platform device is a good idea. Having a more structured way to
> probe those seems like a good idea, but maybe a different subsystem
> would be more appropriate.
>
> I do realize that a 'platform_device' has become a rather generic abstraction
> for almost anything, but at some point we might want to draw the line
> of what is a platform_device.

I guess the question how are they different? Most of what's under
drivers/firmware/ are platform drivers. I think they are mostly either
smc calls or mailbox interfaces. Would there be any advantage to
creating an smc bus or mailbox bus?

It's easier to convert from a platform driver to some new bus_type
than convert from a non-driver if we decide to do that later.

The other approach would be to do a whitelist of compatibles. That's
what's being done for /reserved-memory (currently there's one
(ramoops) and a 2nd is being added).

Rob

Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init

[Sudeep Holla]

On 11/08/17 15:15, Rob Herring wrote:
> +Bjorn
> 
> On Fri, Aug 11, 2017 at 8:30 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.
>>
>> This patch adds initcall to achieve the same.
>>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Rob Herring <robh@kernel.org>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  drivers/firmware/Makefile |  1 +
>>  drivers/firmware/of.c     | 34 ++++++++++++++++++++++++++++++++++
>>  2 files changed, 35 insertions(+)
>>  create mode 100644 drivers/firmware/of.c
>>
>> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
>> index 91d3ff62c653..d9a6fce43613 100644
>> --- a/drivers/firmware/Makefile
>> +++ b/drivers/firmware/Makefile
>> @@ -17,6 +17,7 @@ obj-$(CONFIG_ISCSI_IBFT)      += iscsi_ibft.o
>>  obj-$(CONFIG_FIRMWARE_MEMMAP)  += memmap.o
>>  obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
>>  obj-$(CONFIG_FW_CFG_SYSFS)     += qemu_fw_cfg.o
>> +obj-$(CONFIG_OF)               += of.o
>>  obj-$(CONFIG_QCOM_SCM)         += qcom_scm.o
>>  obj-$(CONFIG_QCOM_SCM_64)      += qcom_scm-64.o
>>  obj-$(CONFIG_QCOM_SCM_32)      += qcom_scm-32.o
>> diff --git a/drivers/firmware/of.c b/drivers/firmware/of.c
>> new file mode 100644
>> index 000000000000..149b9660fb44
>> --- /dev/null
>> +++ b/drivers/firmware/of.c
>> @@ -0,0 +1,34 @@
>> +/*
>> + * Populates the nodes under /firmware/ device tree node
>> + *
>> + * Copyright (C) 2017 ARM Ltd.
>> + *
>> + * This file is subject to the terms and conditions of the GNU General Public
>> + * License.  See the file "COPYING" in the main directory of this archive
>> + * for more details.
>> + *
>> + * Released under the GPLv2 only.
> 
> Why do you have both the above 2 paragraphs and an SPDX tag? I'd drop
> the above (unless your lawyers told you otherwise).
> 

I thought so initially but then just copied from existing file
(drivers/base/arch_topology.c in this case)

>> + * SPDX-License-Identifier: GPL-2.0
>> + */
>> +
>> +#include <linux/init.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +
>> +static int __init firmware_of_init(void)
> 
> I'd prefer this is added to of_platform_default_populate_init() and
> handled like /reserved-memory.
> 

Yes that was my other option, wanted to check in the cover letter but
forgot.

> But be aware that Bjorn is reworking this function[1].
> 

OK, thanks for the pointers.

>> +{
>> +       struct device_node *fw_np;
>> +       int ret;
>> +
>> +       fw_np = of_find_node_by_name(NULL, "firmware");
> 
> This matches any node named firmware. I see a few instances like RPi
> that are not /firmware (though RPi we can move probably).
> of_find_node_by_path would be safer.
> 

OK

>> +
>> +       if (!fw_np)
>> +               return 0;
>> +
>> +       ret = of_platform_populate(fw_np, NULL, NULL, NULL);
>> +
>> +       of_node_put(fw_np);
>> +
>> +       return ret;
>> +}
>> +arch_initcall_sync(firmware_of_init);
> 
> You perhaps missed a few instances. "amlogic,meson-gxbb-sm" looks like
> it could be converted to a driver.

Indeed, was looking for the term "firmware" only :(, will add.

 "tlm,trusted-foundations" appears
> to be needed early for bringing up cores, so it probably can't change.

That was my intially understand too when I looked at the way probe was
handled, but then I see it's module_init, so I went ahead with this change.

> They don't have to be converted as part of this series though.
> 

OK

-- 
Regards,
Sudeep

Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init

[Sudeep Holla]

On 11/08/17 16:16, Sudeep Holla wrote:
> 
> 
> On 11/08/17 15:15, Rob Herring wrote:

[...]

>  "tlm,trusted-foundations" appears
>> to be needed early for bringing up cores, so it probably can't change.
> 
> That was my intially understand too when I looked at the way probe was
> handled, but then I see it's module_init, so I went ahead with this change.
> 

Scratch this, I was referring "linaro,optee-tz" and not
"tlm,trusted-foundations"

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init

[Sudeep Holla]

On 11/08/17 16:16, Sudeep Holla wrote:
> 
> 
> On 11/08/17 15:15, Rob Herring wrote:

[...]

>  "tlm,trusted-foundations" appears
>> to be needed early for bringing up cores, so it probably can't change.
> 
> That was my intially understand too when I looked at the way probe was
> handled, but then I see it's module_init, so I went ahead with this change.
> 

Scratch this, I was referring "linaro,optee-tz" and not
"tlm,trusted-foundations"

-- 
Regards,
Sudeep

Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init

[Sudeep Holla]

On 11/08/17 15:37, Arnd Bergmann wrote:
> On Fri, Aug 11, 2017 at 3:30 PM, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> 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.
>>
>> This patch adds initcall to achieve the same.
> 
> Hmm, I'm a bit skeptical whether representing anything under /firmware
> as a platform device is a good idea. Having a more structured way to
> probe those seems like a good idea, but maybe a different subsystem
> would be more appropriate.
> 

Just a vague thought: if we go to an extent of creating a bus to deal
with these, won't we need to be more formal and create compatible for that ?

If we do that, then how do we support existing device trees ? Again we
are back to the same point but I do agree with your views.

> I do realize that a 'platform_device' has become a rather generic abstraction
> for almost anything, but at some point we might want to draw the line
> of what is a platform_device.
> 

As Rob pointed out it's already being handled as platform_devices in
many cases and my aim was just to reduce the duplication.

-- 
Regards,
Sudeep
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init

[Sudeep Holla]

On 11/08/17 15:37, Arnd Bergmann wrote:
> On Fri, Aug 11, 2017 at 3:30 PM, 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.
>>
>> This patch adds initcall to achieve the same.
> 
> Hmm, I'm a bit skeptical whether representing anything under /firmware
> as a platform device is a good idea. Having a more structured way to
> probe those seems like a good idea, but maybe a different subsystem
> would be more appropriate.
> 

Just a vague thought: if we go to an extent of creating a bus to deal
with these, won't we need to be more formal and create compatible for that ?

If we do that, then how do we support existing device trees ? Again we
are back to the same point but I do agree with your views.

> I do realize that a 'platform_device' has become a rather generic abstraction
> for almost anything, but at some point we might want to draw the line
> of what is a platform_device.
> 

As Rob pointed out it's already being handled as platform_devices in
many cases and my aim was just to reduce the duplication.

-- 
Regards,
Sudeep

Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init

[Arnd Bergmann]

On Fri, Aug 11, 2017 at 5:05 PM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Aug 11, 2017 at 9:37 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> On Fri, Aug 11, 2017 at 3:30 PM, Sudeep Holla <sudeep.holla-5wv7dgnIgG8@public.gmane.org> 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.
>>>
>>> This patch adds initcall to achieve the same.
>>
>> Hmm, I'm a bit skeptical whether representing anything under /firmware
>> as a platform device is a good idea. Having a more structured way to
>> probe those seems like a good idea, but maybe a different subsystem
>> would be more appropriate.
>>
>> I do realize that a 'platform_device' has become a rather generic abstraction
>> for almost anything, but at some point we might want to draw the line
>> of what is a platform_device.
>
> I guess the question how are they different? Most of what's under
> drivers/firmware/ are platform drivers. I think they are mostly either
> smc calls or mailbox interfaces. Would there be any advantage to
> creating an smc bus or mailbox bus?

I guess one difference I see is between things that are purely software
based (smc, efi runtime, rtas,  ...) and those that talk to some
hardware other than the CPU running some firmware.

The first category seems like a good fit for /firmware in DT and
for /sys/firmware in sysfs, while the second category would be
represented elsewhere in both DT and sysfs.

drivers/base/firmware.c currently is extremely rudimentary but this
is where /sys/firmware objects hook into. How about extending
this with a firmware_device that gets populated from /firmware
in DT? Not using platform_device obviously means we lose
all of the automatic probing of reg/interrupts/... resources, but
then again that is sort of the idea of firmware-only nodes.

      Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init

[Arnd Bergmann]

On Fri, Aug 11, 2017 at 5:05 PM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Aug 11, 2017 at 9:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Aug 11, 2017 at 3:30 PM, 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.
>>>
>>> This patch adds initcall to achieve the same.
>>
>> Hmm, I'm a bit skeptical whether representing anything under /firmware
>> as a platform device is a good idea. Having a more structured way to
>> probe those seems like a good idea, but maybe a different subsystem
>> would be more appropriate.
>>
>> I do realize that a 'platform_device' has become a rather generic abstraction
>> for almost anything, but at some point we might want to draw the line
>> of what is a platform_device.
>
> I guess the question how are they different? Most of what's under
> drivers/firmware/ are platform drivers. I think they are mostly either
> smc calls or mailbox interfaces. Would there be any advantage to
> creating an smc bus or mailbox bus?

I guess one difference I see is between things that are purely software
based (smc, efi runtime, rtas,  ...) and those that talk to some
hardware other than the CPU running some firmware.

The first category seems like a good fit for /firmware in DT and
for /sys/firmware in sysfs, while the second category would be
represented elsewhere in both DT and sysfs.

drivers/base/firmware.c currently is extremely rudimentary but this
is where /sys/firmware objects hook into. How about extending
this with a firmware_device that gets populated from /firmware
in DT? Not using platform_device obviously means we lose
all of the automatic probing of reg/interrupts/... resources, but
then again that is sort of the idea of firmware-only nodes.

      Arnd

Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init

[Sudeep Holla]

On 11/08/17 16:54, Arnd Bergmann wrote:
> On Fri, Aug 11, 2017 at 5:05 PM, Rob Herring <robh@kernel.org> wrote:
>> On Fri, Aug 11, 2017 at 9:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Fri, Aug 11, 2017 at 3:30 PM, 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.
>>>>
>>>> This patch adds initcall to achieve the same.
>>>
>>> Hmm, I'm a bit skeptical whether representing anything under /firmware
>>> as a platform device is a good idea. Having a more structured way to
>>> probe those seems like a good idea, but maybe a different subsystem
>>> would be more appropriate.
>>>
>>> I do realize that a 'platform_device' has become a rather generic abstraction
>>> for almost anything, but at some point we might want to draw the line
>>> of what is a platform_device.
>>
>> I guess the question how are they different? Most of what's under
>> drivers/firmware/ are platform drivers. I think they are mostly either
>> smc calls or mailbox interfaces. Would there be any advantage to
>> creating an smc bus or mailbox bus?
> 
> I guess one difference I see is between things that are purely software
> based (smc, efi runtime, rtas,  ...) and those that talk to some
> hardware other than the CPU running some firmware.
> 
> The first category seems like a good fit for /firmware in DT and
> for /sys/firmware in sysfs, while the second category would be
> represented elsewhere in both DT and sysfs.
> 
> drivers/base/firmware.c currently is extremely rudimentary but this
> is where /sys/firmware objects hook into. How about extending
> this with a firmware_device that gets populated from /firmware
> in DT? Not using platform_device obviously means we lose
> all of the automatic probing of reg/interrupts/... resources

While I agree conceptually, but with things like SCMI which provides
clock, power domain, dvfs and other providers which are generally
fit into the driver model and have other devices that are consumers
linked to it.

Also I noticed quite a few ACPI based static tables which are purely
firmware interface driven as platform device. As Rob pointed out we
already have quite a few DT based drivers too. I know all these points
doesn't prove anything against valid points you have made, but I was
just referring the reality out there.

One thing I observed the devices are getting "firmware:"(e.g.:
firmware:scmi) in the beginning of their name which is better than
explicitly adding in the driver.

-- 
Regards,
Sudeep

Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init

[Sudeep Holla]

(sorry for replying on old thread)

On 11/08/17 16:54, Arnd Bergmann wrote:
> On Fri, Aug 11, 2017 at 5:05 PM, Rob Herring <robh@kernel.org> wrote:
>> On Fri, Aug 11, 2017 at 9:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Fri, Aug 11, 2017 at 3:30 PM, 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.
>>>>
>>>> This patch adds initcall to achieve the same.
>>>
>>> Hmm, I'm a bit skeptical whether representing anything under /firmware
>>> as a platform device is a good idea. Having a more structured way to
>>> probe those seems like a good idea, but maybe a different subsystem
>>> would be more appropriate.
>>>
>>> I do realize that a 'platform_device' has become a rather generic abstraction
>>> for almost anything, but at some point we might want to draw the line
>>> of what is a platform_device.
>>
>> I guess the question how are they different? Most of what's under
>> drivers/firmware/ are platform drivers. I think they are mostly either
>> smc calls or mailbox interfaces. Would there be any advantage to
>> creating an smc bus or mailbox bus?
> 
> I guess one difference I see is between things that are purely software
> based (smc, efi runtime, rtas,  ...) and those that talk to some
> hardware other than the CPU running some firmware.
> 
> The first category seems like a good fit for /firmware in DT and
> for /sys/firmware in sysfs, while the second category would be
> represented elsewhere in both DT and sysfs.
> 

After some thoughts and looking around I see examples of lots of drivers
using platform device for firmware interface including rtc-efi.

So IIUC, anything exposed to userspace about sch firmware interface must
be in "/sys/firmware", but I don't see any issue with kernel handling
them as platform device/driver internally.

-- 
Regards,
Sudeep