From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C3294C433E0 for ; Sat, 30 Jan 2021 09:42:16 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 6DC4F64DE2 for ; Sat, 30 Jan 2021 09:42:16 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6DC4F64DE2 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=2ijiYdiBRoD9JI28bDuvfp06/9xIHRC7sqsd9Am0WdE=; b=gNGDF7oj1nrH5Ssxo97gi4og2 MQ2bc33vRZ68z6xBYBlNHeV+JpJQ1WP/tQPPZ+QSvlo/goypEPC3ZyBFqo5Cv91Zwgcp4Zg97aB5z A4NY9M9k1MxsvsdIAUEOyArEOx3KesXY6h7INqm6Fg/IM7ga+t3xJ604m7Mdn3WVM23d8B/iHrVxD rJoUwmDB/BvuF/F67MDcvQ2dEdhNbF8phKPA4jhNEkiT+LScqwrOfwaTc21IthaBUm0TwOGmSWMUU VYLB+rWUUP6DXWIpErtwD4d5qTvswZ9ViMSfWOoOQ+r9gdJLajUzUvuJc6SMqTGUaPEPkOVgXc9rF h0OB761YQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l5mkG-0001k2-G3; Sat, 30 Jan 2021 09:40:44 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l5mkC-0001jZ-Pv for linux-arm-kernel@lists.infradead.org; Sat, 30 Jan 2021 09:40:43 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 71D6B113E; Sat, 30 Jan 2021 01:40:30 -0800 (PST) Received: from [10.57.44.129] (unknown [10.57.44.129]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EE0423F885; Sat, 30 Jan 2021 01:40:28 -0800 (PST) Subject: Re: [PATCH v6 2/4] scmi-cpufreq: Move CPU initialisation to probe To: Cristian Marussi References: <20210111154524.20196-1-nicola.mazzucato@arm.com> <20210111154524.20196-3-nicola.mazzucato@arm.com> <20210114165427.GC46841@e120937-lin> From: Nicola Mazzucato Message-ID: <5a4c82bb-093c-3953-c762-835e38be5d31@arm.com> Date: Sat, 30 Jan 2021 09:43:08 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: <20210114165427.GC46841@e120937-lin> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210130_044040_963736_DFAA700F X-CRM114-Status: GOOD ( 34.39 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-pm@vger.kernel.org, vireshk@kernel.org, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, sudeep.holla@arm.com, chris.redpath@arm.com, morten.rasmussen@arm.com, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Cristian, sorry for my late reply. Thanks for looking into this. I am preparing a v7 with suggestions proposed by Viresh which, hopefully, should remove some unclear parts and resolve your comments. I had left behind some dealloc, so thanks for spotting! Many thanks, Nicola On 1/14/21 4:54 PM, Cristian Marussi wrote: > Hi Nicola, > > a few remarks down below. > > On Mon, Jan 11, 2021 at 03:45:22PM +0000, Nicola Mazzucato wrote: >> Some of the cpu related initialisation can be done at probe stage. >> This patch moves those initialisations from the ->init callback to the >> probe stage. >> >> This is done in preparation for adding support to retrieve additional >> information from DT (CPUs sharing v/f lines). >> >> Signed-off-by: Nicola Mazzucato >> --- >> drivers/cpufreq/scmi-cpufreq.c | 180 ++++++++++++++++++++++++--------- >> 1 file changed, 135 insertions(+), 45 deletions(-) >> >> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c >> index 15b213ed78fa..4aa97cdc5997 100644 >> --- a/drivers/cpufreq/scmi-cpufreq.c >> +++ b/drivers/cpufreq/scmi-cpufreq.c >> @@ -25,6 +25,14 @@ struct scmi_data { >> struct device *cpu_dev; >> }; >> >> +/* Per-CPU storage for runtime management */ >> +struct scmi_cpudata { >> + cpumask_var_t scmi_shared_cpus; >> + struct cpufreq_frequency_table *freq_table; >> +}; >> + >> +static struct scmi_cpudata *cpudata_table; >> + >> static const struct scmi_handle *handle; >> >> static unsigned int scmi_cpufreq_get_rate(unsigned int cpu) >> @@ -120,13 +128,10 @@ scmi_get_cpu_power(unsigned long *power, unsigned long *KHz, >> >> static int scmi_cpufreq_init(struct cpufreq_policy *policy) >> { >> - int ret, nr_opp; >> + int ret; >> unsigned int latency; >> struct device *cpu_dev; >> struct scmi_data *priv; >> - struct cpufreq_frequency_table *freq_table; >> - struct em_data_callback em_cb = EM_DATA_CB(scmi_get_cpu_power); >> - bool power_scale_mw; >> >> cpu_dev = get_cpu_device(policy->cpu); >> if (!cpu_dev) { >> @@ -134,42 +139,19 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy) >> return -ENODEV; >> } >> >> - ret = handle->perf_ops->device_opps_add(handle, cpu_dev); >> - if (ret) { >> - dev_warn(cpu_dev, "failed to add opps to the device\n"); >> - return ret; >> - } >> - >> - ret = scmi_get_sharing_cpus(cpu_dev, policy->cpus); >> - if (ret) { >> - dev_warn(cpu_dev, "failed to get sharing cpumask\n"); >> - return ret; >> - } >> - >> - ret = dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus); >> - if (ret) { >> - dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", >> - __func__, ret); >> - return ret; >> - } >> - >> priv = kzalloc(sizeof(*priv), GFP_KERNEL); >> if (!priv) { >> ret = -ENOMEM; >> goto out_free_opp; >> } >> >> - ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table); >> - if (ret) { >> - dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret); >> - goto out_free_priv; >> - } >> + cpumask_copy(policy->cpus, cpudata_table[policy->cpu].scmi_shared_cpus); >> >> priv->cpu_dev = cpu_dev; >> priv->domain_id = handle->perf_ops->device_domain_id(cpu_dev); >> >> policy->driver_data = priv; >> - policy->freq_table = freq_table; >> + policy->freq_table = cpudata_table[policy->cpu].freq_table; >> >> /* SCMI allows DVFS request for any domain from any CPU */ >> policy->dvfs_possible_from_any_cpu = true; >> @@ -183,23 +165,8 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy) >> policy->fast_switch_possible = >> handle->perf_ops->fast_switch_possible(handle, cpu_dev); >> >> - nr_opp = dev_pm_opp_get_opp_count(cpu_dev); >> - if (nr_opp <= 0) { >> - dev_err(cpu_dev, "%s: No OPPs for this device: %d\n", >> - __func__, ret); >> - >> - ret = -ENODEV; >> - goto out_free_priv; >> - } >> - >> - power_scale_mw = handle->perf_ops->power_scale_mw_get(handle); >> - em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, policy->cpus, >> - power_scale_mw); >> - >> return 0; >> >> -out_free_priv: >> - kfree(priv); >> out_free_opp: >> dev_pm_opp_remove_all_dynamic(cpu_dev); >> > > My understanding (but I could be wrong given my limited familiarity with > CPUFREQ ... so bear with me) is that dev_pm_opp_remove_all_dynamic() is > meant to clean dynamic OPPs added by dev_pm_opp_add() which in turn in > this driver is folded inside the handle->perf_ops->device_opps_add() call, > so is not that this call should be added also on the error path inside > the new scmi_init_device() ? (this was already faulty this way in the > original code to be honest...if faulty at all :D) > > I added a few such invocations down below as rough untested example of > what I mean. > >> @@ -210,7 +177,6 @@ static int scmi_cpufreq_exit(struct cpufreq_policy *policy) >> { >> struct scmi_data *priv = policy->driver_data; >> >> - dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); >> dev_pm_opp_remove_all_dynamic(priv->cpu_dev); >> kfree(priv); >> >> @@ -231,10 +197,102 @@ static struct cpufreq_driver scmi_cpufreq_driver = { >> .exit = scmi_cpufreq_exit, >> }; >> >> +static int scmi_init_cpudata(void) >> +{ >> + int cpu; >> + unsigned int ncpus = num_possible_cpus(); >> + >> + cpudata_table = kzalloc(sizeof(*cpudata_table) * ncpus, GFP_KERNEL); > Shouldn/t this be a kcalloc() given it's an array allocation, checkpatch > complains too. > >> + if (!cpudata_table) >> + return -ENOMEM; >> + >> + for_each_possible_cpu(cpu) { >> + if (!zalloc_cpumask_var(&cpudata_table[cpu].scmi_shared_cpus, >> + GFP_KERNEL)) >> + goto out; >> + } >> + >> + return 0; >> + >> +out: >> + kfree(cpudata_table); >> + return -ENOMEM; >> +} >> + >> +static int scmi_init_device(const struct scmi_handle *handle, int cpu) >> +{ >> + struct device *cpu_dev; >> + int ret, nr_opp; >> + struct em_data_callback em_cb = EM_DATA_CB(scmi_get_cpu_power); >> + bool power_scale_mw; >> + cpumask_var_t scmi_cpus; >> + >> + if (!zalloc_cpumask_var(&scmi_cpus, GFP_KERNEL)) >> + return -ENOMEM; >> + >> + cpumask_set_cpu(cpu, scmi_cpus); >> + >> + cpu_dev = get_cpu_device(cpu); >> + >> + ret = scmi_get_sharing_cpus(cpu_dev, scmi_cpus); >> + if (ret) { >> + dev_warn(cpu_dev, "failed to get sharing cpumask\n"); >> + goto free_cpumask; >> + } >> + >> + /* >> + * We get here for each CPU. Add OPPs only on those CPUs for which we >> + * haven't already done so, or set their OPPs as shared. >> + */ >> + nr_opp = dev_pm_opp_get_opp_count(cpu_dev); >> + if (nr_opp <= 0) { >> + ret = handle->perf_ops->device_opps_add(handle, cpu_dev); >> + if (ret) { >> + dev_warn(cpu_dev, "failed to add opps to the device\n"); >> + goto free_cpumask; >> + } >> + >> + ret = dev_pm_opp_set_sharing_cpus(cpu_dev, scmi_cpus); >> + if (ret) { >> + dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", >> + __func__, ret); > goto free_dynamic_opps; >> + } >> + >> + nr_opp = dev_pm_opp_get_opp_count(cpu_dev); >> + if (nr_opp <= 0) { >> + dev_err(cpu_dev, "%s: No OPPs for this device: %d\n", >> + __func__, ret); >> + >> + ret = -ENODEV; > goto free_dynamic_opps; >> + } >> + >> + power_scale_mw = handle->perf_ops->power_scale_mw_get(handle); >> + em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb, scmi_cpus, >> + power_scale_mw); >> + } >> + >> + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, >> + &cpudata_table[cpu].freq_table); >> + if (ret) { >> + dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret); > goto free_dynamic_opps; >> + } >> + >> + cpumask_copy(cpudata_table[cpu].scmi_shared_cpus, scmi_cpus); >> + > free_dynamic_opps: > dev_pm_opp_remove_all_dynamic(cpu_dev); >> +free_cpumask: >> + free_cpumask_var(scmi_cpus); >> + return ret; >> +} >> + >> static int scmi_cpufreq_probe(struct scmi_device *sdev) >> { >> int ret; >> struct device *dev = &sdev->dev; >> + int cpu; >> + struct device *cpu_dev; >> >> handle = sdev->handle; >> >> @@ -247,6 +305,24 @@ static int scmi_cpufreq_probe(struct scmi_device *sdev) >> devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, NULL); >> #endif >> >> + ret = scmi_init_cpudata(); >> + if (ret) { >> + pr_err("%s: init cpu data failed\n", __func__); >> + return ret; >> + } >> + >> + for_each_possible_cpu(cpu) { >> + cpu_dev = get_cpu_device(cpu); >> + >> + ret = scmi_init_device(handle, cpu); >> + if (ret) { >> + dev_err(cpu_dev, "%s: init device failed\n", >> + __func__); >> + > goto clean; >> + } >> + } >> + >> ret = cpufreq_register_driver(&scmi_cpufreq_driver); >> if (ret) { >> dev_err(dev, "%s: registering cpufreq failed, err: %d\n", > > /* clean any dynamic OPPs already set */ > clean: > for_each_possible_cpu(cpu) { > cpu_dev = get_cpu_device(cpu); > > dev_pm_opp_remove_all_dynamic(cpu_dev); > } > > return ret; > } > > > Thanks > > Cristian > >> @@ -258,6 +334,20 @@ static int scmi_cpufreq_probe(struct scmi_device *sdev) >> >> static void scmi_cpufreq_remove(struct scmi_device *sdev) >> { >> + int cpu; >> + struct device *cpu_dev; >> + >> + for_each_possible_cpu(cpu) { >> + cpu_dev = get_cpu_device(cpu); >> + >> + dev_pm_opp_free_cpufreq_table(cpu_dev, >> + &cpudata_table[cpu].freq_table); >> + >> + free_cpumask_var(cpudata_table[cpu].scmi_shared_cpus); >> + } >> + >> + kfree(cpudata_table); >> + >> cpufreq_unregister_driver(&scmi_cpufreq_driver); >> } >> >> -- >> 2.27.0 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel