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 A0598C433FE for ; Mon, 16 May 2022 06:10:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240213AbiEPGKh (ORCPT ); Mon, 16 May 2022 02:10:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57700 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240217AbiEPGKf (ORCPT ); Mon, 16 May 2022 02:10:35 -0400 Received: from mail-lj1-x235.google.com (mail-lj1-x235.google.com [IPv6:2a00:1450:4864:20::235]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 04B8620F7B for ; Sun, 15 May 2022 23:10:31 -0700 (PDT) Received: by mail-lj1-x235.google.com with SMTP id 4so16822098ljw.11 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=LoaiUnDQ9hnghZfPdo1jPOq/O+iUP9ewPSJ4S3IAgb/ZY67ANiw3EdPDVxyx9nrT1s 7AxcXE9vqbM6dDzVrTkIkAweveX6a6EyrKGSkQqYSo5kqRA/zsZbnuGjIds2I8RKfNRu mSyLP76N2Qz+tqdlhKw6xws8zDrqNI0Ao9CzO9G5fNJo7S+GCYzD1R3Ym+N4vVZ0Uq6v Q9SiZzdnrB6HUFHGcS/B689GiLgFrkVjiUOUe6WyMXtvY6gw+kyekRTLRe0h2AHM5+52 5O2wKLn/kvLZ4w/Udser6xeWpPGDtuFDw67XqA+1I2QT7SiLOMBubZnru+ML67VIy8Oc CwbQ== X-Gm-Message-State: AOAM5334Vwd/Dq2iry2nzdslKzJvWSDvwfoYyck+C2hNzISJz1K65SDK aEKvCbPbR1FVChYhwNsY2fNcAA== 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: devicetree@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