From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932200AbdIFNlq (ORCPT ); Wed, 6 Sep 2017 09:41:46 -0400 Received: from foss.arm.com ([217.140.101.70]:51128 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754638AbdIFNlm (ORCPT ); Wed, 6 Sep 2017 09:41:42 -0400 Subject: Re: [PATCH v2 09/18] firmware: arm_scmi: probe and initialise all the supported protocols To: Sudeep Holla , ALKML , LKML , DTML Cc: Roy Franz , Harb Abdulhamid , Nishanth Menon , Arnd Bergmann , Loc Ho , Alexey Klimov , Ryan Harkin , Jassi Brar References: <1501857104-11279-1-git-send-email-sudeep.holla@arm.com> <1501857104-11279-10-git-send-email-sudeep.holla@arm.com> <7c82503d-229f-08e7-6605-18d778b6f525@arm.com> From: Julien Thierry Message-ID: <934ce254-15af-56a7-5d93-ade0aa1bf4fa@arm.com> Date: Wed, 6 Sep 2017 14:41:38 +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: <7c82503d-229f-08e7-6605-18d778b6f525@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed 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 06/09/17 14:31, Sudeep Holla wrote: > > > On 06/09/17 10:41, Julien Thierry wrote: >> >> >> On 04/08/17 15:31, Sudeep Holla wrote: >>> Now that we have basic support for all the protocols in the >>> specification, let's probe them individually and initialise them. >>> >>> Cc: Arnd Bergmann >>> Signed-off-by: Sudeep Holla >>> --- >>> drivers/firmware/arm_scmi/common.h | 5 +++ >>> drivers/firmware/arm_scmi/driver.c | 80 >>> +++++++++++++++++++++++++++++++++++++- >>> 2 files changed, 84 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/firmware/arm_scmi/common.h >>> b/drivers/firmware/arm_scmi/common.h >>> index 7473dfcad4ee..d7c73a8d260b 100644 >>> --- a/drivers/firmware/arm_scmi/common.h >>> +++ b/drivers/firmware/arm_scmi/common.h >>> @@ -118,4 +118,9 @@ int scmi_version_get(const struct scmi_handle *h, >>> u8 protocol, u32 *version); >>> void scmi_setup_protocol_implemented(const struct scmi_handle *handle, >>> u8 *prot_imp); >>> +typedef int (*scmi_init_fn_t)(struct scmi_handle *); >>> int scmi_base_protocol_init(struct scmi_handle *h); >>> +int scmi_perf_protocol_init(struct scmi_handle *h); >>> +int scmi_sensors_protocol_init(struct scmi_handle *h); >>> +int scmi_power_protocol_init(struct scmi_handle *h); >>> +int scmi_clock_protocol_init(struct scmi_handle *h); >>> diff --git a/drivers/firmware/arm_scmi/driver.c >>> b/drivers/firmware/arm_scmi/driver.c >>> index 601d0d7210d9..6f31761043e2 100644 >>> --- a/drivers/firmware/arm_scmi/driver.c >>> +++ b/drivers/firmware/arm_scmi/driver.c >>> @@ -157,6 +157,12 @@ struct scmi_shared_mem { >>> u8 msg_payload[0]; >>> }; >>> +struct scmi_protocol_match { >>> + u8 protocol_id; >>> + scmi_init_fn_t fn; >> >> Could we call this "init" or "prot_init"? >> > > Done > >>> + char name[32]; >>> +}; >>> + >>> static int scmi_linux_errmap[] = { >>> /* better than switch case as long as return value is continuous */ >>> 0, /* SCMI_SUCCESS */ >>> @@ -687,6 +693,41 @@ static int scmi_xfer_info_init(struct scmi_info >>> *sinfo) >>> return 0; >>> } >>> +static const struct scmi_protocol_match scmi_protocols[] = { >>> + { >>> + .protocol_id = SCMI_PROTOCOL_PERF, >>> + .fn = scmi_perf_protocol_init, >>> + .name = "scmi-cpufreq", >>> + }, { >>> + .protocol_id = SCMI_PROTOCOL_CLOCK, >>> + .fn = scmi_clock_protocol_init, >>> + .name = "scmi-clocks", >>> + }, { >>> + .protocol_id = SCMI_PROTOCOL_POWER, >>> + .fn = scmi_power_protocol_init, >>> + .name = "scmi-power-domain", >>> + }, { >>> + .protocol_id = SCMI_PROTOCOL_SENSOR, >>> + .fn = scmi_sensors_protocol_init, >>> + .name = "scmi-hwmon", >>> + }, >>> + {} >>> +}; >>> + >>> +static const struct scmi_protocol_match *scmi_protocol_match_get(u8 >>> protocol_id) >>> +{ >>> + int i; >>> + const struct scmi_protocol_match *match = NULL, *loop = >>> scmi_protocols; >>> + >>> + for (i = 0; i < ARRAY_SIZE(scmi_protocols); i++, loop++) >>> + if (loop->protocol_id == protocol_id) { >>> + match = loop; >>> + break; >>> + } >>> + >>> + return match; >>> +} >>> + >> >> The "match" variable is not needed. We can just return "loop" in the if >> branch and return NULL at the end of the function. Unless we are >> following some coding standard that advises against that? >> > > Agreed and fixed locally. > >>> static int scmi_mailbox_check(struct device_node *np) >>> { >>> struct of_phandle_args arg; >>> @@ -778,7 +819,7 @@ static int scmi_probe(struct platform_device *pdev) >>> const struct scmi_desc *desc; >>> struct scmi_info *info; >>> struct device *dev = &pdev->dev; >>> - struct device_node *np = dev->of_node; >>> + struct device_node *child, *np = dev->of_node; >>> /* Only mailbox method supported, check for the presence of >>> one */ >>> if (scmi_mailbox_check(np)) { >>> @@ -817,6 +858,43 @@ static int scmi_probe(struct platform_device *pdev) >>> return ret; >>> } >>> + for_each_available_child_of_node(np, child) { >>> + int init_ret; >>> + u32 prot_id; >>> + const struct scmi_protocol_match *match; >>> + >>> + if (of_property_read_u32(child, "reg", &prot_id)) >>> + continue; >>> + >>> + prot_id &= MSG_PROTOCOL_ID_MASK; >>> + >>> + if (!scmi_is_protocol_implemented(handle, prot_id)) { >>> + dev_err(dev, "SCMI protocol %d not implemented\n", >>> + prot_id); >>> + continue; >>> + } >>> + >>> + match = scmi_protocol_match_get(prot_id); >>> + if (match) { >>> + struct platform_device *cdev; >>> + >>> + cdev = of_platform_device_create(child, match->name, >>> + dev); >>> + if (!cdev) { >>> + dev_err(dev, "failed to create %s device\n", >>> + match->name); >>> + continue; >>> + } >>> + >>> + init_ret = match->fn(handle); >>> + if (init_ret) { >>> + dev_err(dev, "SCMI protocol %d init error %d\n", >>> + prot_id, init_ret); >>> + of_platform_device_destroy(&cdev->dev, NULL); >>> + } >>> + } >>> + } >>> + >> >> For the code in the "if (match)" branch, would it be better to have an >> inline function "scmi_create_protocol_device" or something with a name >> that fits? >> > > Done > >> Also I was wondering, what is the difference between the protocol being >> implemented and finding the matching structure? Feels like there is a >> bit of redundancy. If the protocol is implemented but doesn't have a >> match structure does it just mean it doesn't need any specific >> initialization? >> > > Yes, that's the idea. One reason to have both check is backward > compatibility. Suppose new protocols are implemented in the f/w but DT > doesn't have them or know how to use it or there are no users of that > feature, then skip initializing it. Similarly if DT is updated but not > the firmware. > Oh I see, makes sense. Thanks for explaining. Cheers, -- Julien Thierry