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

Message ID 1502458237-1683-2-git-send-email-sudeep.holla@arm.com
State New
Headers show
Series
  • firmware: of: populate /firmware/ node during init
Related show

Commit Message

Sudeep Holla Aug. 11, 2017, 1:30 p.m.
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

-- 
2.7.4

Comments

Rob Herring Aug. 11, 2017, 2:15 p.m. | #1
+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
Sudeep Holla Aug. 11, 2017, 3:16 p.m. | #2
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

Patch hide | download patch | download mbox

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);