| Message ID | 1502458237-1683-2-git-send-email-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
+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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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"
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
(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.
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);
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