Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1709633 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] firmware: of: populate /firmware/ node during init Sudeep Holla <sudeep.holla@arm.com> - 2017-08-11 15:40 +0200
    [PATCH 1/3] firmware: of: populate /firmware/ node during init Sudeep Holla <sudeep.holla@arm.com> - 2017-08-11 15:40 +0200
      Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init Rob Herring <robh@kernel.org> - 2017-08-11 16:20 +0200
        Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init Sudeep Holla <sudeep.holla@arm.com> - 2017-08-11 17:20 +0200
          Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init Sudeep Holla <sudeep.holla@arm.com> - 2017-08-11 17:30 +0200
      Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init Arnd Bergmann <arnd@arndb.de> - 2017-08-11 16:40 +0200
        Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init Rob Herring <robh@kernel.org> - 2017-08-11 17:10 +0200
          Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init Arnd Bergmann <arnd@arndb.de> - 2017-08-11 18:00 +0200
            Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init Sudeep Holla <sudeep.holla@arm.com> - 2017-08-14 15:30 +0200
        Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init Sudeep Holla <sudeep.holla@arm.com> - 2017-08-11 17:40 +0200

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

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

[toc] | [next] | [standalone]

#1709634 — [PATCH 1/3] firmware: of: populate /firmware/ node during init

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

[toc] | [prev] | [next] | [standalone]

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

+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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web