From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753373AbdHKPP4 (ORCPT ); Fri, 11 Aug 2017 11:15:56 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:45738 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753342AbdHKPPx (ORCPT ); Fri, 11 Aug 2017 11:15:53 -0400 Cc: Sudeep Holla , Linux Kernel Mailing List , "linux-arm-msm@vger.kernel.org" , "devicetree@vger.kernel.org" , Arnd Bergmann , Andy Gross , Jens Wiklander , Bjorn Andersson Subject: Re: [PATCH 1/3] firmware: of: populate /firmware/ node during init To: Rob Herring References: <1502458237-1683-1-git-send-email-sudeep.holla@arm.com> <1502458237-1683-2-git-send-email-sudeep.holla@arm.com> From: Sudeep Holla Organization: ARM Message-ID: <7a3b73d0-988c-1d2a-43cc-4d30b4eac0f1@arm.com> Date: Fri, 11 Aug 2017 16:16:14 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/08/17 15:15, Rob Herring wrote: > +Bjorn > > On Fri, Aug 11, 2017 at 8:30 AM, 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. >> >> This patch adds initcall to achieve the same. >> >> Cc: Arnd Bergmann >> Cc: Rob Herring >> Signed-off-by: Sudeep Holla >> --- >> 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 >> +#include >> +#include >> + >> +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