Received: by 2002:ac0:a5b6:0:0:0:0:0 with SMTP id m51-v6csp4310342imm; Wed, 30 May 2018 03:16:54 -0700 (PDT) X-Google-Smtp-Source: ADUXVKI7azpP9hXsXxtpsJL7QOVKeSIKVinGxVsrb2+48hAzM+0jHUP7Amzx80Nn2WUURmpYay08 X-Received: by 2002:a17:902:6546:: with SMTP id d6-v6mr1692610pln.196.1527675413993; Wed, 30 May 2018 03:16:53 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1527675413; cv=none; d=google.com; s=arc-20160816; b=lSRITsTCLQ928NxzkFo1O1kIztRf4tQf9cQT0xnRDmbVWJ4FG77QNli1g8uewoV4cP ndYHhNPuxlwVd2OHQan0mNNbCvp/F5kQXJc9cQQgQ/a+Hi3PVzbObEex/Sa5JCaZYhO5 RcyPeIn1ST83knVRXVLAq3A9L51v4LMIv1V26cr66kmW4q8KOfNZjT6UUL/3PsaU/L2f 50/puF3AO997WTqwG19QgvsuAA050yJVYzuRHtRZq57o6ZIORAJ+VHadbpFQiwCnCx+q j+5br2icMt967hKrtAcRVIZGQuwAQqY5Wa6Et7ki0ryW8ZsWZP9q2Dg6uMrETXorCRMI Y/+w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:dmarc-filter :dkim-signature:dkim-signature:arc-authentication-results; bh=xKqOimC2378qovdVKwrFF7kwhXVi+0c0d0uqJ+Thp5o=; b=Hn8igYe8BkH3Uht7+iSL5Iqszs1X41sbkO9wZFmtFRaT3ykB5i6dzFhqfnbxdtNIte 2R3FhyEzHsPCCaETQM/i+Yxb6WjvtyWSWfqh3p3E6maxwcVkUXY/F99QJ54sLxKRR9mF zkP0RquZ3U3jmlDLZOP+NcDFurM+Pr5P0Q6YOuPGryWXhyWWMbMancXAmsjJeVsqndlG 1qdjKaWt3VWbk8ETpspXUzpcPDKOYz5X2+a6ENx1tu9zAo65Tux2dTCWu7/N5m6fPST4 Fx5eplpKJGxIGZtD8vdBWDr6tjj/O3OFxJN2Zgp7McU6VZpS/kYvZx50QKXbZIHj+wWK vmPw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=kwt1n8g+; dkim=pass header.i=@codeaurora.org header.s=default header.b=PBXT5279; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id t27-v6si543228pgo.566.2018.05.30.03.16.39; Wed, 30 May 2018 03:16:53 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=kwt1n8g+; dkim=pass header.i=@codeaurora.org header.s=default header.b=PBXT5279; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751461AbeE3KPD (ORCPT + 99 others); Wed, 30 May 2018 06:15:03 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:55170 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750710AbeE3KO7 (ORCPT ); Wed, 30 May 2018 06:14:59 -0400 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id 073776060A; Wed, 30 May 2018 10:14:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1527675299; bh=nHOoxn61Z7UpR0P2Yysk2oq2Gj0PWPGO5XpG+3xKoU8=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=kwt1n8g+6R2H6jlZbqDspoEUnPBfMQ5SDE4kW52Ou7H84PvSApZzvrYK7v2gbkuoj DD+oxtSAaPSDalRrp22ff6wWkj4V22/E0FLwHniiQ/3xTYfE7mZcmSozkVPH7PFxcr wjTyQvhDdjJC/J5UXcBcVbUDA9eFtER9jf/6/OK0= X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on pdx-caf-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.8 required=2.0 tests=ALL_TRUSTED,BAYES_00, DKIM_SIGNED,T_DKIM_INVALID autolearn=no autolearn_force=no version=3.4.0 Received: from [10.79.40.88] (blr-bdr-fw-01_globalnat_allzones-outside.qualcomm.com [103.229.18.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: rnayak@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 5E28A60261; Wed, 30 May 2018 10:14:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1527675297; bh=nHOoxn61Z7UpR0P2Yysk2oq2Gj0PWPGO5XpG+3xKoU8=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=PBXT5279lSG6dSyPbnY3tzPZGtMPkBYLPSPH41krPZIH/iSgZVA2ympMNWi0fWhy6 NV41Y9GeB4J0LTwjMySDpbTrdZMqOPEuSQ2B8kxxJbwZz2G3RK/7KbWkeYOzVMgvku 2r2YbRqw5G+qXxBLEhDcQdqmkHKFjCyT+qGDCSbg= DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 5E28A60261 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=rnayak@codeaurora.org Subject: Re: [PATCH v2 1/6] soc: qcom: rpmpd: Add a powerdomain driver to model corners To: Ulf Hansson Cc: Viresh Kumar , Stephen Boyd , Andy Gross , devicetree@vger.kernel.org, linux-arm-msm , Linux Kernel Mailing List , collinsd@codeaurora.org References: <20180525100121.28214-1-rnayak@codeaurora.org> <20180525100121.28214-2-rnayak@codeaurora.org> From: Rajendra Nayak Message-ID: <0e07d577-9728-e97a-2da0-dd7dd324f058@codeaurora.org> Date: Wed, 30 May 2018 15:44:53 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 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 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/30/2018 02:47 PM, Ulf Hansson wrote: > On 25 May 2018 at 12:01, Rajendra Nayak wrote: >> The powerdomains for corners just pass the performance state set by the >> consumers to the RPM (Remote Power manager) which then takes care >> of setting the appropriate voltage on the corresponding rails to >> meet the performance needs. >> >> We add all powerdomain data needed on msm8996 here. This driver can easily >> be extended by adding data for other qualcomm SoCs as well. >> >> Signed-off-by: Rajendra Nayak >> Signed-off-by: Viresh Kumar >> --- >> .../devicetree/bindings/power/qcom,rpmpd.txt | 55 ++++ > > Please split DT doc changes into separate patches, to simplify review. yes, i will split it when I resend the series. > >> drivers/soc/qcom/Kconfig | 9 + >> drivers/soc/qcom/Makefile | 1 + >> drivers/soc/qcom/rpmpd.c | 299 ++++++++++++++++++ >> 4 files changed, 364 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmpd.txt >> create mode 100644 drivers/soc/qcom/rpmpd.c >> > > [...] > >> +++ b/drivers/soc/qcom/rpmpd.c >> @@ -0,0 +1,299 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> + >> +#define domain_to_rpmpd(domain) container_of(domain, struct rpmpd, pd) >> + >> +/* Resource types */ >> +#define RPMPD_SMPA 0x61706d73 >> +#define RPMPD_LDOA 0x616f646c >> + >> +/* Operation Keys */ >> +#define KEY_CORNER 0x6e726f63 /* corn */ >> +#define KEY_ENABLE 0x6e657773 /* swen */ >> +#define KEY_FLOOR_CORNER 0x636676 /* vfc */ >> + >> +#define DEFINE_RPMPD_CORN_SMPA(_platform, _name, _active, r_id) \ >> + static struct rpmpd _platform##_##_active; \ >> + static struct rpmpd _platform##_##_name = { \ >> + .pd = { .name = #_name, }, \ >> + .peer = &_platform##_##_active, \ >> + .res_type = RPMPD_SMPA, \ >> + .res_id = r_id, \ >> + .key = KEY_CORNER, \ >> + }; \ >> + static struct rpmpd _platform##_##_active = { \ >> + .pd = { .name = #_active, }, \ >> + .peer = &_platform##_##_name, \ >> + .active_only = true, \ >> + .res_type = RPMPD_SMPA, \ >> + .res_id = r_id, \ >> + .key = KEY_CORNER, \ >> + } >> + >> +#define DEFINE_RPMPD_CORN_LDOA(_platform, _name, r_id) \ >> + static struct rpmpd _platform##_##_name = { \ >> + .pd = { .name = #_name, }, \ >> + .res_type = RPMPD_LDOA, \ >> + .res_id = r_id, \ >> + .key = KEY_CORNER, \ >> + } >> + >> +#define DEFINE_RPMPD_VFC(_platform, _name, r_id, r_type) \ >> + static struct rpmpd _platform##_##_name = { \ >> + .pd = { .name = #_name, }, \ >> + .res_type = r_type, \ >> + .res_id = r_id, \ >> + .key = KEY_FLOOR_CORNER, \ >> + } >> + >> +#define DEFINE_RPMPD_VFC_SMPA(_platform, _name, r_id) \ >> + DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_SMPA) >> + >> +#define DEFINE_RPMPD_VFC_LDOA(_platform, _name, r_id) \ >> + DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_LDOA) >> + >> +struct rpmpd_req { >> + __le32 key; >> + __le32 nbytes; >> + __le32 value; >> +}; >> + >> +struct rpmpd { >> + struct generic_pm_domain pd; >> + struct rpmpd *peer; >> + const bool active_only; >> + unsigned long corner; >> + bool enabled; >> + const char *res_name; >> + const int res_type; >> + const int res_id; >> + struct qcom_smd_rpm *rpm; >> + __le32 key; >> +}; >> + >> +struct rpmpd_desc { >> + struct rpmpd **rpmpds; >> + size_t num_pds; >> +}; >> + >> +static DEFINE_MUTEX(rpmpd_lock); >> + >> +/* msm8996 RPM powerdomains */ >> +DEFINE_RPMPD_CORN_SMPA(msm8996, vddcx, vddcx_ao, 1); >> +DEFINE_RPMPD_CORN_SMPA(msm8996, vddmx, vddmx_ao, 2); >> +DEFINE_RPMPD_CORN_LDOA(msm8996, vddsscx, 26); >> + >> +DEFINE_RPMPD_VFC_SMPA(msm8996, vddcx_vfc, 1); >> +DEFINE_RPMPD_VFC_LDOA(msm8996, vddsscx_vfc, 26); >> + >> +static struct rpmpd *msm8996_rpmpds[] = { >> + [0] = &msm8996_vddcx, >> + [1] = &msm8996_vddcx_ao, >> + [2] = &msm8996_vddcx_vfc, >> + [3] = &msm8996_vddmx, >> + [4] = &msm8996_vddmx_ao, >> + [5] = &msm8996_vddsscx, >> + [6] = &msm8996_vddsscx_vfc, >> +}; > > It's not my call, but honestly the above all macros makes the code > less readable. This is all static data per SoC. The macros will keep the new additions needed for every new SoC to a minimal. Currently this supports only msm8996. > > Anyway, I think you should convert to allocate these structs > dynamically from the heap (kzalloc/kcalloc), instead of statically as > above. > >> + >> +static const struct rpmpd_desc msm8996_desc = { >> + .rpmpds = msm8996_rpmpds, >> + .num_pds = ARRAY_SIZE(msm8996_rpmpds), >> +}; >> + >> +static const struct of_device_id rpmpd_match_table[] = { >> + { .compatible = "qcom,msm8996-rpmpd", .data = &msm8996_desc }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, rpmpd_match_table); > > [...] > >> +static int rpmpd_aggregate_corner(struct rpmpd *pd) >> +{ > > Isn't the aggregation of the performance states in genpd sufficient > for your case? > > I guess this is SoC specific and needed anyways, but then could you > perhaps add a few comments about what goes on here? Yes, this is SoC specific aggregation for active and sleep votes. i will add comments to clarify this is different from the aggregation done at the framework level. > >> + int ret; >> + struct rpmpd *peer = pd->peer; >> + unsigned long active_corner, sleep_corner; >> + unsigned long this_corner = 0, this_sleep_corner = 0; >> + unsigned long peer_corner = 0, peer_sleep_corner = 0; >> + >> + to_active_sleep(pd, pd->corner, &this_corner, &this_sleep_corner); >> + >> + if (peer && peer->enabled) >> + to_active_sleep(peer, peer->corner, &peer_corner, >> + &peer_sleep_corner); >> + >> + active_corner = max(this_corner, peer_corner); >> + >> + ret = rpmpd_send_corner(pd, QCOM_RPM_ACTIVE_STATE, active_corner); >> + if (ret) >> + return ret; >> + >> + sleep_corner = max(this_sleep_corner, peer_sleep_corner); >> + >> + return rpmpd_send_corner(pd, QCOM_RPM_SLEEP_STATE, sleep_corner); >> +} > > [...] > >> +static int rpmpd_probe(struct platform_device *pdev) >> +{ >> + int i; >> + size_t num; >> + struct genpd_onecell_data *data; >> + struct qcom_smd_rpm *rpm; >> + struct rpmpd **rpmpds; >> + const struct rpmpd_desc *desc; >> + >> + rpm = dev_get_drvdata(pdev->dev.parent); >> + if (!rpm) { >> + dev_err(&pdev->dev, "Unable to retrieve handle to RPM\n"); >> + return -ENODEV; >> + } >> + >> + desc = of_device_get_match_data(&pdev->dev); >> + if (!desc) >> + return -EINVAL; >> + >> + rpmpds = desc->rpmpds; >> + num = desc->num_pds; >> + >> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; >> + >> + data->domains = devm_kcalloc(&pdev->dev, num, sizeof(*data->domains), >> + GFP_KERNEL); >> + data->num_domains = num; >> + >> + for (i = 0; i < num; i++) { >> + if (!rpmpds[i]) >> + continue; >> + >> + rpmpds[i]->rpm = rpm; >> + rpmpds[i]->pd.power_off = rpmpd_power_off; >> + rpmpds[i]->pd.power_on = rpmpd_power_on; >> + pm_genpd_init(&rpmpds[i]->pd, NULL, true); > > Question: Is there no hierarchical topology of the PM domains. No > genpd subdomains? The hierarchy if any is all handled by the remote core (RPM in this case). For Linux its just a flat view. > >> + >> + data->domains[i] = &rpmpds[i]->pd; >> + } >> + >> + return of_genpd_add_provider_onecell(pdev->dev.of_node, data); >> +} >> + >> +static int rpmpd_remove(struct platform_device *pdev) >> +{ >> + of_genpd_del_provider(pdev->dev.of_node); >> + return 0; >> +} >> + >> +static struct platform_driver rpmpd_driver = { >> + .driver = { >> + .name = "qcom-rpmpd", >> + .of_match_table = rpmpd_match_table, >> + }, >> + .probe = rpmpd_probe, >> + .remove = rpmpd_remove, >> +}; >> + >> +static int __init rpmpd_init(void) >> +{ >> + return platform_driver_register(&rpmpd_driver); >> +} >> +core_initcall(rpmpd_init); >> + >> +static void __exit rpmpd_exit(void) >> +{ >> + platform_driver_unregister(&rpmpd_driver); >> +} >> +module_exit(rpmpd_exit); >> + >> +MODULE_DESCRIPTION("Qualcomm RPM Power Domain Driver"); >> +MODULE_LICENSE("GPL v2"); >> +MODULE_ALIAS("platform:qcom-rpmpd"); >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >> of Code Aurora Forum, hosted by The Linux Foundation >> > > Besides the minor things above, this looks good to me. thanks, Rajendra -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation