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=-5.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no 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 6E2FDC433DB for ; Thu, 14 Jan 2021 13:36:33 +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 2CBFB239A1 for ; Thu, 14 Jan 2021 13:36:33 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2CBFB239A1 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=6JxAUBvxBZZPA1eNd3t3mRnjSTRJupkwEXBG6/GjW08=; b=dbtk22CGVcEaRcotoAv0mbBml s9Ym9e2JM5tH0oicxf0rDab2oHcgMsG6tKMZXJpGOWjwDOMxxo823DXmrfjQmHe9m0TxFvm8yNn+0 uFx2xEFT2Pl33BF2jC0BATWWsWCufmLiu+2ywr0jK6Rhwb9aRaZkbOOt0eEY5yhLpxvOO4uRVqA2x a76PU0nKHPvhzYbxI8KSWOFRSP/xJHtMgoupw34yVNVNBFbS8k66jzjUsahqXFNw7MRSw+qlDs8pg KPbvYFkVo45a5Kr7xaogNk9B7xGj74/KQqy57teJ0zHnXiCqzgXpYnX5J3fhN5N7MXYYC/PVO2W7R ydBG9cm7g==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l02mQ-0002EU-UD; Thu, 14 Jan 2021 13:35:14 +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 1l02mN-0002DP-Ly for linux-arm-kernel@lists.infradead.org; Thu, 14 Jan 2021 13:35:12 +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 C32031FB; Thu, 14 Jan 2021 05:35:08 -0800 (PST) Received: from [10.57.57.156] (unknown [10.57.57.156]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 068F13F719; Thu, 14 Jan 2021 05:35:06 -0800 (PST) Subject: Re: [PATCH v6 2/4] scmi-cpufreq: Move CPU initialisation to probe To: Viresh Kumar References: <20210111154524.20196-1-nicola.mazzucato@arm.com> <20210111154524.20196-3-nicola.mazzucato@arm.com> <20210112111717.5ds446w2kroxzvhr@vireshk-i7> <20210114050753.u3b5z6kgbup7xbuh@vireshk-i7> From: Nicola Mazzucato Message-ID: <027a2d94-b804-2c99-ad1a-23927d361fe5@arm.com> Date: Thu, 14 Jan 2021 13:37:44 +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: <20210114050753.u3b5z6kgbup7xbuh@vireshk-i7> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210114_083511_847569_7F5F1B13 X-CRM114-Status: GOOD ( 25.74 ) 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, cristian.marussi@arm.com, 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 Viresh, many thanks for your suggestions. I will prepare a new version based on those. Many thanks, Nicola On 1/14/21 5:07 AM, Viresh Kumar wrote: > On 13-01-21, 11:55, Nicola Mazzucato wrote: >> On 1/12/21 11:17 AM, Viresh Kumar wrote: >>> This could have been done with a per-cpu variable instead. >> >> sure, I can do a DEFINE_PER_CPU() for it if it makes it better. > > If we don't go with the linked list approach, then yes. > >>>> + for_each_possible_cpu(cpu) { >>>> + if (!zalloc_cpumask_var(&cpudata_table[cpu].scmi_shared_cpus, >>>> + GFP_KERNEL)) >>>> + goto out; >>>> + } >>> >>> You are making a copy of the struct for each CPU and so for a 16 CPUs >>> sharing their clock lines, you will have 16 copies of the exact same >>> stuff. >>> >>> An optimal approach would be to have a linked-list of this structure >>> and that will only have 1 node per cpufreq policy. >> >> It is allocating space for the cpumask for each of the cpu. No data is copied yet. > > Yes, I was talking about the whole design here. > >> I understand the optimisation, but I don't see a linkage to cpufreq policy to be >> a good idea. This cpudata is for internal storage of scmi and opp-shared info >> and it is not tied to cpufreq policy. > > Well, it is going to be the same information for all CPUs of a policy, isn't it > ? > >> We have moved all the cpu bits to probe >> and at this stage we have no knowledge of cpufreq polices. > > Yes, but you are reading that information from scmi or DT (empty opp tables) and > so you know what the cpumasks are going to be set to. The linked list is the > right solution in my opinion, it is much more optimal. > >>>> +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); >>> >>> Where do you expect the sharing information to come from in this case >>> ? DT ? >> >> Coming from SCMI perf. The source of info has not changed. >> >>> >>>> + 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_cpumask; >>>> + } >>>> + >>>> + nr_opp = dev_pm_opp_get_opp_count(cpu_dev); >>> >>> Shouldn't you do this just after adding the OPPs ? >> >> This was suggested earlier. It was moved closer to em_registration to where the >> nr_opp is used. One way or the other as I don't have a strong preference for its >> place. > > It is better to move it above as this will shorten the error path. > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel