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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 26955C433F5 for ; Mon, 16 May 2022 06:10:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240207AbiEPGKg (ORCPT ); Mon, 16 May 2022 02:10:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57686 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240213AbiEPGKe (ORCPT ); Mon, 16 May 2022 02:10:34 -0400 Received: from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com [IPv6:2a00:1450:4864:20::22d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E87D320F78 for ; Sun, 15 May 2022 23:10:30 -0700 (PDT) Received: by mail-lj1-x22d.google.com with SMTP id v4so16823949ljd.10 for ; Sun, 15 May 2022 23:10:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=zib3uaRuFHQp5m+3/8oVVWs2fT/RhjrDBvhpgKAcf3A=; b=T8a68Wcn4gY9NFXddamoH+rRrUy8g81C6ugx+hQrWs90thVEoFpfoW5rfePda8J8Ti KhYLE5GeSNkseGhg6lt7Uw3CFsrabahfUTN6w49xWFaBgVGI0x9XP77w7igMPyaJ68Cq qg8vUoA0kBuM1wr5DCpHhE3FpkgNX+KKJZK6qCoalPD/rQiLoRs9/bYs/l0ErwlkoH0P 1viy/5jm8JiHRHEICvrFZ3mP4KXxz85YQeOIAL0QqJ1J/jLc0LdzwVX5OH8SXaqpQEH3 cNP78pu268Ud97vfkkEiS4RW+MJ/RzJICDEG0kc8MdHqBzJU5DPGyHb+S8K2sDRzFhMV K1KA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=zib3uaRuFHQp5m+3/8oVVWs2fT/RhjrDBvhpgKAcf3A=; b=kr1qKNUD/XXCysu/Sl7Crbz52RbOYEc3iMCL9SXJNu0HhAwaWKtlgb0z7J+zax0i5b Qe6Dk+YbQfPmFT61JEa+r4PPRwduVNxS4myxu+GSF8D2D5o+Ix/chpCUzd+MK8GRMGZp 4LEKQwc7Dj0Tl89wBde/wU+YRPkIhN9fMEbmSYoRcU+0hic4Dz72mDmfvXnr95pKm5IA tIv6Anjl0lQma2ONr5qRPTMICdxEOnJ58+4rHtmoA553ui1FAFTW1UPK9yR+PHNkL2Wi 6x51cso6Rw3doxYneMs66/TEeGwnEdMdfno5zXgZDDmlQkkhxKA0CqMyM0wwpG4mqZgt rH7A== X-Gm-Message-State: AOAM533ZXvLxEAJwqhrzxQmLZc+NM3GDm4Ac6vIb9H6mgibFnBd4hinW LXezNBxtsXTLogrrRLO3F0EFD7BmAvj938GM X-Google-Smtp-Source: ABdhPJyrZUQf7WD5zZ0nqfhRfXzM6EGNE+a7PhnPAP8HR3cuJ7y91FoKEpx/vEUszqhSySkNWYqKKw== X-Received: by 2002:a05:651c:399:b0:24f:18d:5bbd with SMTP id e25-20020a05651c039900b0024f018d5bbdmr10166934ljp.481.1652681429224; Sun, 15 May 2022 23:10:29 -0700 (PDT) Received: from [192.168.0.17] (78-11-189-27.static.ip.netia.com.pl. [78.11.189.27]) by smtp.gmail.com with ESMTPSA id j9-20020a19f509000000b0047255d210f2sm1208147lfb.33.2022.05.15.23.10.27 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 15 May 2022 23:10:28 -0700 (PDT) Message-ID: Date: Mon, 16 May 2022 08:10:27 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1 Subject: Re: [PATCH v3 7/7] ufs: use PM OPP when scaling gears Content-Language: en-US To: Manivannan Sadhasivam Cc: Andy Gross , Bjorn Andersson , Michael Turquette , Stephen Boyd , Rob Herring , Krzysztof Kozlowski , Viresh Kumar , Nishanth Menon , Alim Akhtar , Avri Altman , "James E.J. Bottomley" , "Martin K. Petersen" , "Rafael J. Wysocki" , Taniya Das , linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, linux-scsi@vger.kernel.org References: <20220513061347.46480-1-krzysztof.kozlowski@linaro.org> <20220513061347.46480-8-krzysztof.kozlowski@linaro.org> <20220513182546.GD1922@thinkpad> From: Krzysztof Kozlowski In-Reply-To: <20220513182546.GD1922@thinkpad> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On 13/05/2022 20:25, Manivannan Sadhasivam wrote: > On Fri, May 13, 2022 at 08:13:47AM +0200, Krzysztof Kozlowski wrote: >> Scaling gears requires not only scaling clocks, but also voltage levels, >> e.g. via performance states. >> >> Use the provided OPP table, to set proper OPP frequency which through >> required-opps will trigger performance state change. This deprecates >> the old freq-table-hz Devicetree property and old clock scaling method >> in favor of PM core code. >> > > To be clear, you are not changing the voltages (UFS supplies) through OPP. But > rather handle only clks and leave the power domain handling to parent OPP > device. Correct, the patchset itself does not introduce itself regulator control. For Qualcomm (and maybe others) these will be scaled via OPP performance states. > >> Signed-off-by: Krzysztof Kozlowski >> Reviewed-by: Bjorn Andersson >> >> --- >> >> Cc: Manivannan Sadhasivam >> --- >> drivers/scsi/ufs/ufshcd-pltfrm.c | 73 +++++++++++++++ >> drivers/scsi/ufs/ufshcd.c | 150 ++++++++++++++++++++++++------- >> drivers/scsi/ufs/ufshcd.h | 6 ++ >> 3 files changed, 195 insertions(+), 34 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c >> index 3ab555f6e66e..a603ca8e383b 100644 >> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c >> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c >> @@ -10,6 +10,7 @@ >> >> #include >> #include >> +#include >> #include >> #include >> >> @@ -108,6 +109,72 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba) >> return ret; >> } >> >> +static int ufshcd_parse_operating_points(struct ufs_hba *hba) >> +{ >> + struct device *dev = hba->dev; >> + struct device_node *np = dev->of_node; >> + struct ufs_clk_info *clki; >> + const char *names[16]; >> + int cnt, i, ret; >> + >> + if (!of_find_property(dev->of_node, "operating-points-v2", NULL)) >> + return 0; >> + >> + cnt = of_property_count_strings(np, "clock-names"); >> + if (cnt <= 0) { >> + dev_warn(dev, "%s: Missing clock-names\n", >> + __func__); > > This is a hard error, right? So why not dev_err()? Good point, but actually this (and following cases) should be return 0, because clocks/freq-table/opp-points are not required properties. The original code (parsing it for freq-table-hz) also does not treat it as error. > >> + return -EINVAL; >> + } >> + >> + if (cnt > ARRAY_SIZE(names)) { >> + dev_info(dev, "%s: Too many clock-names\n", __func__); > > dev_err()? > >> + return -EINVAL; >> + } >> + >> + if (of_find_property(np, "freq-table-hz", NULL)) { >> + dev_info(dev, "%s: operating-points and freq-table-hz are incompatible\n", >> + __func__); > > dev_err()? > >> + return -EINVAL; >> + } >> + >> + for (i = 0; i < cnt; i++) { >> + ret = of_property_read_string_index(np, "clock-names", i, >> + &names[i]); >> + if (ret) >> + return ret; >> + >> + clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL); >> + if (!clki) >> + return -ENOMEM; >> + >> + clki->name = devm_kstrdup(dev, names[i], GFP_KERNEL); >> + if (!clki->name) >> + return -ENOMEM; >> + >> + if (!strcmp(names[i], "ref_clk")) >> + clki->keep_link_active = true; >> + >> + list_add_tail(&clki->list, &hba->clk_list_head); >> + } >> + >> + ret = devm_pm_opp_set_clknames(dev, names, i); >> + if (ret) >> + return ret; >> + >> + ret = devm_pm_opp_register_set_opp_helper(dev, ufshcd_set_opp); >> + if (ret) >> + return ret; >> + >> + ret = devm_pm_opp_of_add_table(dev); >> + if (ret) >> + return ret; >> + >> + hba->use_pm_opp = true; >> + > > Since you are only handling the clks in UFS driver's OPP implementation, it > warrants atleast a comment. Otherwise, someone will add voltage to the OPP > table and complain that it is not getting changed. Eventhough the UFS driver > won't allow doing it, it is safer to mention it explicitly. Sure. > > Also I'm worried about the implementation specific to Qcom platforms. Like we > rely on RPMHPD to handle the power domains, but that may not be true for other > platforms. I know that we cannot support all possible implementations but > atleast we should document this limitation. > > Rest looks fine to me. I'll take one more look after testing this series on > SM8450. Using OPPs is quite generic, so other platform could implement also regulator scaling. The changes are indeed targetting Qcom platforms, but they are not restricting any other usage. Best regards, Krzysztof