Message ID | 20230712103213.101770-14-manivannan.sadhasivam@linaro.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:a6b2:0:b0:3e4:2afc:c1 with SMTP id c18csp1057871vqm; Wed, 12 Jul 2023 03:49:53 -0700 (PDT) X-Google-Smtp-Source: APBJJlFSyVTiHQwIe1eKd0nGcYoFxJVkEXvHXxcUfkoARCVm8Cjx9v0O8d9Y19IXrUMU0I9YKlZ+ X-Received: by 2002:a17:906:74da:b0:988:6491:98e3 with SMTP id z26-20020a17090674da00b00988649198e3mr19233805ejl.68.1689158993250; Wed, 12 Jul 2023 03:49:53 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1689158993; cv=none; d=google.com; s=arc-20160816; b=auPqbqlI8BUHvY1qm07G1acyirfmFHhHsvp7Uaq3MQH+/pMoV8O7NH3dI0Zwa9CrZ8 LXQOFeGK4ARBuj+ULRlQYMFTT+jh1qe4Iaa5VgW739xhTTLAqMM/udArH21vzJmMzQSI LA5mOUAtN5FLOx8tRkRQQyJge/QMvqfhDP8PFwSECpBXLQ93pxFPmPQJ4q6lAsza7w6r +EF+X4VTZ3MbxTbs0q/L6VLJz02g7BIszmORf//E17rcQu1aJPVL3NAfdOuxzw8+rw9E ZGEzZya5HtKp+una26V9IlhbrXOpUVZ8t57q+XcfxwIYee1HYF9L5oF7ADbP3T7TfInC 7ApQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=+PtBZ5U/uPeERQpI5byb7Cw2i1LitARzlq3AxNOTl7Y=; fh=cS2dop9BQ3I5XB0BoTkY5FviL9WhE7fTaUcXZactGFA=; b=L039zPJmis9sxDb2tpyUwtgVFBcXakx0j8ObQO41WA7K3cCvuZNr1OJTo0cg6+1zo7 S+KAHncXZiGmiozbhVn+Gx6odt5bzUY05rRYVVFk94QSlc6qYzHy3SToEJpBke/ArTLE w7oY5igz8AMxTw6XFtpAqEoQDBPfdrsyaPxpKwRM7cZXzj/lZFoITItpWzlIx0XihQQ/ 64HiuJX9d2b1w06ZV7AsQZP4b0oaWRSLP4Dmm9dGP6VML9R3k6IH23Xi2KdBf086eoZc 3jEyyKpe4WwaqJ1boFVygtPKlV2tEqI8Oelr/RFgiSQ2dgj5+JYZKdm0DQ5Se3gXx+Jk ueaA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=r7WllUK3; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id a10-20020a170906468a00b0098642e99c22si4039498ejr.604.2023.07.12.03.49.28; Wed, 12 Jul 2023 03:49:53 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=r7WllUK3; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232081AbjGLKgE (ORCPT <rfc822;gnulinuxfreebsd@gmail.com> + 99 others); Wed, 12 Jul 2023 06:36:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50160 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232221AbjGLKft (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 12 Jul 2023 06:35:49 -0400 Received: from mail-pf1-x42e.google.com (mail-pf1-x42e.google.com [IPv6:2607:f8b0:4864:20::42e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7222A2699 for <linux-kernel@vger.kernel.org>; Wed, 12 Jul 2023 03:35:23 -0700 (PDT) Received: by mail-pf1-x42e.google.com with SMTP id d2e1a72fcca58-666e97fcc60so4665678b3a.3 for <linux-kernel@vger.kernel.org>; Wed, 12 Jul 2023 03:35:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1689158118; x=1691750118; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=+PtBZ5U/uPeERQpI5byb7Cw2i1LitARzlq3AxNOTl7Y=; b=r7WllUK3aizqmTErli8XKT+tdveXXrxsPymKU/mra2cIbrgPXN8MIassa7PByvkKgc 1mFMXC3ZcEhTC0ZirVNdbEVlT3xSfUoubHkW+qTOzTRBhMGaUSQhdM43T2vy9ez6EqpX nhTU6JokNBUpDZC+CyOxHPQ3EWimNHvFA9c8TQN9whsBOocgcCY2qDrKvLJ0nfarNhfB 4t3HWE5B/OHkjlm+sPOtzF2u5nQv0Gb0ikuvxxuRXVCJNw3AiNFSqjIsUv6DmVrh2R5T 3QiVve5zBwPGlY4s/esJ2qiY1P28ebmv6nMVvWqBUj226X/0UIuuIqMLZKdPP/GRAhWs uGtQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689158118; x=1691750118; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=+PtBZ5U/uPeERQpI5byb7Cw2i1LitARzlq3AxNOTl7Y=; b=DwD3+1khofvE6uM2OzGpqD2shExbfXxG0y797I1QxcM7jldZCsRAlql5Pj3Rzpt+rW fYUKyHfbglFfHV9hHEcjppt6k8wNW5QGBcgSTv7hgEJHksHYTvvAJ0qBiFAKVKfcHmjx NjGdzTwt+4EH68EGJuv75aw0ZS45o+OkRV8wTUclsL/YBEy9wJwb/zcwowMlj9BNVrtC BW72E2n7bFaj6AXhQWxy+CDIeJWjt8umg6Yo1qosHcpFIWTb267taD+KD75Wc4t55L3c GeVjZtWK+U4Od5SzR1z2Jbv1l1GU+mbkJNJWomT08MP0Yl+yagS8d6SvSfIHiHopfZ0t ZvGQ== X-Gm-Message-State: ABy/qLZNQgyRrHWhCzTlDY3VQFMqGrx55o5bEzPEetpYJ6ujwuyhR00u fOc7eeHqgMOuzhyiePfCNU9D X-Received: by 2002:a05:6a20:12d4:b0:12d:e596:df21 with SMTP id v20-20020a056a2012d400b0012de596df21mr17179274pzg.7.1689158118058; Wed, 12 Jul 2023 03:35:18 -0700 (PDT) Received: from localhost.localdomain ([117.207.27.131]) by smtp.gmail.com with ESMTPSA id k15-20020aa790cf000000b00666b3706be6sm3247860pfk.107.2023.07.12.03.35.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 12 Jul 2023 03:35:17 -0700 (PDT) From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> To: vireshk@kernel.org, nm@ti.com, sboyd@kernel.org, myungjoo.ham@samsung.com, kyungmin.park@samsung.com, cw00.choi@samsung.com, andersson@kernel.org, konrad.dybcio@linaro.org, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, quic_asutoshd@quicinc.com, quic_cang@quicinc.com, quic_nitirawa@quicinc.com, quic_narepall@quicinc.com, quic_bhaskarv@quicinc.com, quic_richardp@quicinc.com, quic_nguyenb@quicinc.com, quic_ziqichen@quicinc.com, bmasney@redhat.com, krzysztof.kozlowski@linaro.org, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Subject: [PATCH 11/14] scsi: ufs: host: Add support for parsing OPP Date: Wed, 12 Jul 2023 16:02:08 +0530 Message-Id: <20230712103213.101770-14-manivannan.sadhasivam@linaro.org> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20230712103213.101770-1-manivannan.sadhasivam@linaro.org> References: <20230712103213.101770-1-manivannan.sadhasivam@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1771211580507929675 X-GMAIL-MSGID: 1771211580507929675 |
Series |
UFS: Add OPP and interconnect support
|
|
Commit Message
Manivannan Sadhasivam
July 12, 2023, 10:32 a.m. UTC
OPP framework can be used to scale the clocks along with other entities such as regulators, performance state etc... So let's add support for parsing OPP from devicetree. OPP support in devicetree is added through the "operating-points-v2" property which accepts the OPP table defining clock frequency, regulator voltage, power domain performance state etc... Since the UFS controller requires multiple clocks to be controlled for proper working, devm_pm_opp_set_config() has been used which supports scaling multiple clocks through custom ufshcd_opp_config_clks() callback. It should be noted that the OPP support is not compatible with the old "freq-table-hz" property. So only one can be used at a time even though the UFS core supports both. Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/ufs/host/ufshcd-pltfrm.c | 116 +++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+)
Comments
On 12/07/2023 13:32, Manivannan Sadhasivam wrote: > OPP framework can be used to scale the clocks along with other entities > such as regulators, performance state etc... So let's add support for > parsing OPP from devicetree. OPP support in devicetree is added through > the "operating-points-v2" property which accepts the OPP table defining > clock frequency, regulator voltage, power domain performance state etc... > > Since the UFS controller requires multiple clocks to be controlled for > proper working, devm_pm_opp_set_config() has been used which supports > scaling multiple clocks through custom ufshcd_opp_config_clks() callback. > > It should be noted that the OPP support is not compatible with the old > "freq-table-hz" property. So only one can be used at a time even though > the UFS core supports both. > > Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/ufs/host/ufshcd-pltfrm.c | 116 +++++++++++++++++++++++++++++++ > 1 file changed, 116 insertions(+) > > diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c > index 0b7430033047..068c22378c88 100644 > --- a/drivers/ufs/host/ufshcd-pltfrm.c > +++ b/drivers/ufs/host/ufshcd-pltfrm.c > @@ -8,8 +8,10 @@ > * Vinayak Holikatti <h.vinayak@samsung.com> > */ > > +#include <linux/clk.h> > #include <linux/module.h> > #include <linux/platform_device.h> > +#include <linux/pm_opp.h> > #include <linux/pm_runtime.h> > #include <linux/of.h> > > @@ -17,6 +19,8 @@ > #include "ufshcd-pltfrm.h" > #include <ufs/unipro.h> > > +#include <trace/events/ufs.h> > + > #define UFSHCD_DEFAULT_LANES_PER_DIRECTION 2 > > static int ufshcd_parse_clock_info(struct ufs_hba *hba) > @@ -205,6 +209,112 @@ static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba) > } > } > > +static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table, > + struct dev_pm_opp *opp, void *data, > + bool scaling_down) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + struct list_head *head = &hba->clk_list_head; > + struct ufs_clk_info *clki; > + unsigned long freq; > + u8 idx = 0; > + int ret; > + > + list_for_each_entry(clki, head, list) { > + if (!IS_ERR_OR_NULL(clki->clk)) { > + freq = dev_pm_opp_get_freq_indexed(opp, idx++); > + > + /* Do not set rate for clocks having frequency as 0 */ > + if (!freq) > + continue; Can we omit these clocks from the opp table? I don't think they serve any purpose. Maybe it would even make sense to move this function to drivers/opp then, as it will be generic enough. > + > + ret = clk_set_rate(clki->clk, freq); > + if (ret) { > + dev_err(dev, "%s: %s clk set rate(%ldHz) failed, %d\n", > + __func__, clki->name, freq, ret); > + return ret; > + } > + > + trace_ufshcd_clk_scaling(dev_name(dev), > + (scaling_down ? "scaled down" : "scaled up"), > + clki->name, hba->clk_scaling.target_freq, freq); > + } > + } > + > + return 0; > +} > + > +static int ufshcd_parse_operating_points(struct ufs_hba *hba) > +{ > + struct device *dev = hba->dev; > + struct device_node *np = dev->of_node; > + struct dev_pm_opp_config config = {}; > + struct ufs_clk_info *clki; > + const char **clk_names; > + int cnt, i, ret; > + > + if (!of_find_property(np, "operating-points-v2", NULL)) > + return 0; > + > + if (of_find_property(np, "freq-table-hz", NULL)) { > + dev_err(dev, "%s: operating-points and freq-table-hz are incompatible\n", > + __func__); > + return -EINVAL; > + } > + > + cnt = of_property_count_strings(np, "clock-names"); > + if (cnt <= 0) { > + dev_err(dev, "%s: Missing clock-names\n", __func__); > + return -ENODEV; > + } > + > + /* OPP expects clk_names to be NULL terminated */ > + clk_names = devm_kcalloc(dev, cnt + 1, sizeof(*clk_names), GFP_KERNEL); > + if (!clk_names) > + return -ENOMEM; > + > + /* > + * We still need to get reference to all clocks as the UFS core uses > + * them separately. > + */ > + for (i = 0; i < cnt; i++) { > + ret = of_property_read_string_index(np, "clock-names", i, > + &clk_names[i]); > + if (ret) > + return ret; > + > + clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL); > + if (!clki) > + return -ENOMEM; > + > + clki->name = devm_kstrdup(dev, clk_names[i], GFP_KERNEL); > + if (!clki->name) > + return -ENOMEM; > + > + if (!strcmp(clk_names[i], "ref_clk")) > + clki->keep_link_active = true; > + > + list_add_tail(&clki->list, &hba->clk_list_head); > + } > + > + config.clk_names = clk_names, > + config.config_clks = ufshcd_opp_config_clks; > + > + ret = devm_pm_opp_set_config(dev, &config); > + if (ret) > + return ret; > + > + ret = devm_pm_opp_of_add_table(dev); > + if (ret) { > + dev_err(dev, "Failed to add OPP table: %d\n", ret); > + return ret; > + } > + > + hba->use_pm_opp = true; > + > + return 0; > +} > + > /** > * ufshcd_get_pwr_dev_param - get finally agreed attributes for > * power mode change > @@ -371,6 +481,12 @@ int ufshcd_pltfrm_init(struct platform_device *pdev, > > ufshcd_init_lanes_per_dir(hba); > > + err = ufshcd_parse_operating_points(hba); > + if (err) { > + dev_err(dev, "%s: OPP parse failed %d\n", __func__, err); > + goto dealloc_host; > + } > + > err = ufshcd_init(hba, mmio_base, irq); > if (err) { > dev_err(dev, "Initialization failed\n");
On Wed, Jul 12, 2023 at 04:15:12PM +0300, Dmitry Baryshkov wrote: > On 12/07/2023 13:32, Manivannan Sadhasivam wrote: > > OPP framework can be used to scale the clocks along with other entities > > such as regulators, performance state etc... So let's add support for > > parsing OPP from devicetree. OPP support in devicetree is added through > > the "operating-points-v2" property which accepts the OPP table defining > > clock frequency, regulator voltage, power domain performance state etc... > > > > Since the UFS controller requires multiple clocks to be controlled for > > proper working, devm_pm_opp_set_config() has been used which supports > > scaling multiple clocks through custom ufshcd_opp_config_clks() callback. > > > > It should be noted that the OPP support is not compatible with the old > > "freq-table-hz" property. So only one can be used at a time even though > > the UFS core supports both. > > > > Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/ufs/host/ufshcd-pltfrm.c | 116 +++++++++++++++++++++++++++++++ > > 1 file changed, 116 insertions(+) > > > > diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c > > index 0b7430033047..068c22378c88 100644 > > --- a/drivers/ufs/host/ufshcd-pltfrm.c > > +++ b/drivers/ufs/host/ufshcd-pltfrm.c > > @@ -8,8 +8,10 @@ > > * Vinayak Holikatti <h.vinayak@samsung.com> > > */ > > +#include <linux/clk.h> > > #include <linux/module.h> > > #include <linux/platform_device.h> > > +#include <linux/pm_opp.h> > > #include <linux/pm_runtime.h> > > #include <linux/of.h> > > @@ -17,6 +19,8 @@ > > #include "ufshcd-pltfrm.h" > > #include <ufs/unipro.h> > > +#include <trace/events/ufs.h> > > + > > #define UFSHCD_DEFAULT_LANES_PER_DIRECTION 2 > > static int ufshcd_parse_clock_info(struct ufs_hba *hba) > > @@ -205,6 +209,112 @@ static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba) > > } > > } > > +static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table, > > + struct dev_pm_opp *opp, void *data, > > + bool scaling_down) > > +{ > > + struct ufs_hba *hba = dev_get_drvdata(dev); > > + struct list_head *head = &hba->clk_list_head; > > + struct ufs_clk_info *clki; > > + unsigned long freq; > > + u8 idx = 0; > > + int ret; > > + > > + list_for_each_entry(clki, head, list) { > > + if (!IS_ERR_OR_NULL(clki->clk)) { > > + freq = dev_pm_opp_get_freq_indexed(opp, idx++); > > + > > + /* Do not set rate for clocks having frequency as 0 */ > > + if (!freq) > > + continue; > > Can we omit these clocks from the opp table? I don't think they serve any > purpose. > No, we cannot. OPP requires the clocks and opp-hz to be of same length. And we cannot omit those clocks as well since linux needs to gate control them. > Maybe it would even make sense to move this function to drivers/opp then, as > it will be generic enough. > There is already a generic function available in OPP core. But we cannot use it as we need to skip setting 0 freq and that's not applicable in OPP core as discussed with Viresh offline. - Mani > > + > > + ret = clk_set_rate(clki->clk, freq); > > + if (ret) { > > + dev_err(dev, "%s: %s clk set rate(%ldHz) failed, %d\n", > > + __func__, clki->name, freq, ret); > > + return ret; > > + } > > + > > + trace_ufshcd_clk_scaling(dev_name(dev), > > + (scaling_down ? "scaled down" : "scaled up"), > > + clki->name, hba->clk_scaling.target_freq, freq); > > + } > > + } > > + > > + return 0; > > +} > + > > +static int ufshcd_parse_operating_points(struct ufs_hba *hba) > > +{ > > + struct device *dev = hba->dev; > > + struct device_node *np = dev->of_node; > > + struct dev_pm_opp_config config = {}; > > + struct ufs_clk_info *clki; > > + const char **clk_names; > > + int cnt, i, ret; > > + > > + if (!of_find_property(np, "operating-points-v2", NULL)) > > + return 0; > > + > > + if (of_find_property(np, "freq-table-hz", NULL)) { > > + dev_err(dev, "%s: operating-points and freq-table-hz are incompatible\n", > > + __func__); > > + return -EINVAL; > > + } > > + > > + cnt = of_property_count_strings(np, "clock-names"); > > + if (cnt <= 0) { > > + dev_err(dev, "%s: Missing clock-names\n", __func__); > > + return -ENODEV; > > + } > > + > > + /* OPP expects clk_names to be NULL terminated */ > > + clk_names = devm_kcalloc(dev, cnt + 1, sizeof(*clk_names), GFP_KERNEL); > > + if (!clk_names) > > + return -ENOMEM; > > + > > + /* > > + * We still need to get reference to all clocks as the UFS core uses > > + * them separately. > > + */ > > + for (i = 0; i < cnt; i++) { > > + ret = of_property_read_string_index(np, "clock-names", i, > > + &clk_names[i]); > > + if (ret) > > + return ret; > > + > > + clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL); > > + if (!clki) > > + return -ENOMEM; > > + > > + clki->name = devm_kstrdup(dev, clk_names[i], GFP_KERNEL); > > + if (!clki->name) > > + return -ENOMEM; > > + > > + if (!strcmp(clk_names[i], "ref_clk")) > > + clki->keep_link_active = true; > > + > > + list_add_tail(&clki->list, &hba->clk_list_head); > > + } > > + > > + config.clk_names = clk_names, > > + config.config_clks = ufshcd_opp_config_clks; > > + > > + ret = devm_pm_opp_set_config(dev, &config); > > + if (ret) > > + return ret; > > + > > + ret = devm_pm_opp_of_add_table(dev); > > + if (ret) { > > + dev_err(dev, "Failed to add OPP table: %d\n", ret); > > + return ret; > > + } > > + > > + hba->use_pm_opp = true; > > + > > + return 0; > > +} > > + > > /** > > * ufshcd_get_pwr_dev_param - get finally agreed attributes for > > * power mode change > > @@ -371,6 +481,12 @@ int ufshcd_pltfrm_init(struct platform_device *pdev, > > ufshcd_init_lanes_per_dir(hba); > > + err = ufshcd_parse_operating_points(hba); > > + if (err) { > > + dev_err(dev, "%s: OPP parse failed %d\n", __func__, err); > > + goto dealloc_host; > > + } > > + > > err = ufshcd_init(hba, mmio_base, irq); > > if (err) { > > dev_err(dev, "Initialization failed\n"); > > -- > With best wishes > Dmitry >
On Wed, 12 Jul 2023 at 19:34, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > On Wed, Jul 12, 2023 at 04:15:12PM +0300, Dmitry Baryshkov wrote: > > On 12/07/2023 13:32, Manivannan Sadhasivam wrote: > > > OPP framework can be used to scale the clocks along with other entities > > > such as regulators, performance state etc... So let's add support for > > > parsing OPP from devicetree. OPP support in devicetree is added through > > > the "operating-points-v2" property which accepts the OPP table defining > > > clock frequency, regulator voltage, power domain performance state etc... > > > > > > Since the UFS controller requires multiple clocks to be controlled for > > > proper working, devm_pm_opp_set_config() has been used which supports > > > scaling multiple clocks through custom ufshcd_opp_config_clks() callback. > > > > > > It should be noted that the OPP support is not compatible with the old > > > "freq-table-hz" property. So only one can be used at a time even though > > > the UFS core supports both. > > > > > > Co-developed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > > drivers/ufs/host/ufshcd-pltfrm.c | 116 +++++++++++++++++++++++++++++++ > > > 1 file changed, 116 insertions(+) > > > > > > diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c > > > index 0b7430033047..068c22378c88 100644 > > > --- a/drivers/ufs/host/ufshcd-pltfrm.c > > > +++ b/drivers/ufs/host/ufshcd-pltfrm.c > > > @@ -8,8 +8,10 @@ > > > * Vinayak Holikatti <h.vinayak@samsung.com> > > > */ > > > +#include <linux/clk.h> > > > #include <linux/module.h> > > > #include <linux/platform_device.h> > > > +#include <linux/pm_opp.h> > > > #include <linux/pm_runtime.h> > > > #include <linux/of.h> > > > @@ -17,6 +19,8 @@ > > > #include "ufshcd-pltfrm.h" > > > #include <ufs/unipro.h> > > > +#include <trace/events/ufs.h> > > > + > > > #define UFSHCD_DEFAULT_LANES_PER_DIRECTION 2 > > > static int ufshcd_parse_clock_info(struct ufs_hba *hba) > > > @@ -205,6 +209,112 @@ static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba) > > > } > > > } > > > +static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table, > > > + struct dev_pm_opp *opp, void *data, > > > + bool scaling_down) > > > +{ > > > + struct ufs_hba *hba = dev_get_drvdata(dev); > > > + struct list_head *head = &hba->clk_list_head; > > > + struct ufs_clk_info *clki; > > > + unsigned long freq; > > > + u8 idx = 0; > > > + int ret; > > > + > > > + list_for_each_entry(clki, head, list) { > > > + if (!IS_ERR_OR_NULL(clki->clk)) { > > > + freq = dev_pm_opp_get_freq_indexed(opp, idx++); > > > + > > > + /* Do not set rate for clocks having frequency as 0 */ > > > + if (!freq) > > > + continue; > > > > Can we omit these clocks from the opp table? I don't think they serve any > > purpose. > > > > No, we cannot. OPP requires the clocks and opp-hz to be of same length. And we > cannot omit those clocks as well since linux needs to gate control them. Hmm, I thought we push the list of "interesting" clocks through config->clock_names. > > > Maybe it would even make sense to move this function to drivers/opp then, as > > it will be generic enough. > > > > There is already a generic function available in OPP core. But we cannot use it > as we need to skip setting 0 freq and that's not applicable in OPP core as > discussed with Viresh offline. Ack. > > - Mani > > > > + > > > + ret = clk_set_rate(clki->clk, freq); > > > + if (ret) { > > > + dev_err(dev, "%s: %s clk set rate(%ldHz) failed, %d\n", > > > + __func__, clki->name, freq, ret); > > > + return ret; > > > + } > > > + > > > + trace_ufshcd_clk_scaling(dev_name(dev), > > > + (scaling_down ? "scaled down" : "scaled up"), > > > + clki->name, hba->clk_scaling.target_freq, freq); > > > + } > > > + } > > > + > > > + return 0; > > > +} > + > > > +static int ufshcd_parse_operating_points(struct ufs_hba *hba) > > > +{ > > > + struct device *dev = hba->dev; > > > + struct device_node *np = dev->of_node; > > > + struct dev_pm_opp_config config = {}; > > > + struct ufs_clk_info *clki; > > > + const char **clk_names; > > > + int cnt, i, ret; > > > + > > > + if (!of_find_property(np, "operating-points-v2", NULL)) > > > + return 0; > > > + > > > + if (of_find_property(np, "freq-table-hz", NULL)) { > > > + dev_err(dev, "%s: operating-points and freq-table-hz are incompatible\n", > > > + __func__); > > > + return -EINVAL; > > > + } > > > + > > > + cnt = of_property_count_strings(np, "clock-names"); > > > + if (cnt <= 0) { > > > + dev_err(dev, "%s: Missing clock-names\n", __func__); > > > + return -ENODEV; > > > + } > > > + > > > + /* OPP expects clk_names to be NULL terminated */ > > > + clk_names = devm_kcalloc(dev, cnt + 1, sizeof(*clk_names), GFP_KERNEL); > > > + if (!clk_names) > > > + return -ENOMEM; > > > + > > > + /* > > > + * We still need to get reference to all clocks as the UFS core uses > > > + * them separately. > > > + */ > > > + for (i = 0; i < cnt; i++) { > > > + ret = of_property_read_string_index(np, "clock-names", i, > > > + &clk_names[i]); > > > + if (ret) > > > + return ret; > > > + > > > + clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL); > > > + if (!clki) > > > + return -ENOMEM; > > > + > > > + clki->name = devm_kstrdup(dev, clk_names[i], GFP_KERNEL); > > > + if (!clki->name) > > > + return -ENOMEM; > > > + > > > + if (!strcmp(clk_names[i], "ref_clk")) > > > + clki->keep_link_active = true; > > > + > > > + list_add_tail(&clki->list, &hba->clk_list_head); > > > + } > > > + > > > + config.clk_names = clk_names, > > > + config.config_clks = ufshcd_opp_config_clks; > > > + > > > + ret = devm_pm_opp_set_config(dev, &config); > > > + if (ret) > > > + return ret; > > > + > > > + ret = devm_pm_opp_of_add_table(dev); > > > + if (ret) { > > > + dev_err(dev, "Failed to add OPP table: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + hba->use_pm_opp = true; > > > + > > > + return 0; > > > +} > > > + > > > /** > > > * ufshcd_get_pwr_dev_param - get finally agreed attributes for > > > * power mode change > > > @@ -371,6 +481,12 @@ int ufshcd_pltfrm_init(struct platform_device *pdev, > > > ufshcd_init_lanes_per_dir(hba); > > > + err = ufshcd_parse_operating_points(hba); > > > + if (err) { > > > + dev_err(dev, "%s: OPP parse failed %d\n", __func__, err); > > > + goto dealloc_host; > > > + } > > > + > > > err = ufshcd_init(hba, mmio_base, irq); > > > if (err) { > > > dev_err(dev, "Initialization failed\n"); > > > > -- > > With best wishes > > Dmitry > > > > -- > மணிவண்ணன் சதாசிவம்
On 12-07-23, 19:48, Dmitry Baryshkov wrote: > On Wed, 12 Jul 2023 at 19:34, Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> wrote: > > On Wed, Jul 12, 2023 at 04:15:12PM +0300, Dmitry Baryshkov wrote: > > > On 12/07/2023 13:32, Manivannan Sadhasivam wrote: > > > > +static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table, > > > > + struct dev_pm_opp *opp, void *data, > > > > + bool scaling_down) > > > > +{ > > > > + struct ufs_hba *hba = dev_get_drvdata(dev); > > > > + struct list_head *head = &hba->clk_list_head; > > > > + struct ufs_clk_info *clki; > > > > + unsigned long freq; > > > > + u8 idx = 0; > > > > + int ret; > > > > + > > > > + list_for_each_entry(clki, head, list) { > > > > + if (!IS_ERR_OR_NULL(clki->clk)) { > > > > + freq = dev_pm_opp_get_freq_indexed(opp, idx++); > > > > + > > > > + /* Do not set rate for clocks having frequency as 0 */ > > > > + if (!freq) > > > > + continue; > > > > > > Can we omit these clocks from the opp table? I don't think they serve any > > > purpose. > > > > > > > No, we cannot. OPP requires the clocks and opp-hz to be of same length. I am okay with having a patch for the OPP core to modify this behavior, as I told privately earlier. > > And we > > cannot omit those clocks as well since linux needs to gate control them. > > Hmm, I thought we push the list of "interesting" clocks through > config->clock_names. Yes, another way to solve this would be keep the interesting clocks in the beginning in "clock-names" field and let the platform pass only those to the OPP core. > > > > > Maybe it would even make sense to move this function to drivers/opp then, as > > > it will be generic enough. > > > > > > > There is already a generic function available in OPP core. But we cannot use it > > as we need to skip setting 0 freq and that's not applicable in OPP core as > > discussed with Viresh offline. > > Ack. I am okay with either of the solutions, it is for you guys to decide what works better for your platform.
On Thu, Jul 13, 2023 at 09:39:18AM +0530, Viresh Kumar wrote: > On 12-07-23, 19:48, Dmitry Baryshkov wrote: > > On Wed, 12 Jul 2023 at 19:34, Manivannan Sadhasivam > > <manivannan.sadhasivam@linaro.org> wrote: > > > On Wed, Jul 12, 2023 at 04:15:12PM +0300, Dmitry Baryshkov wrote: > > > > On 12/07/2023 13:32, Manivannan Sadhasivam wrote: > > > > > > +static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table, > > > > > + struct dev_pm_opp *opp, void *data, > > > > > + bool scaling_down) > > > > > +{ > > > > > + struct ufs_hba *hba = dev_get_drvdata(dev); > > > > > + struct list_head *head = &hba->clk_list_head; > > > > > + struct ufs_clk_info *clki; > > > > > + unsigned long freq; > > > > > + u8 idx = 0; > > > > > + int ret; > > > > > + > > > > > + list_for_each_entry(clki, head, list) { > > > > > + if (!IS_ERR_OR_NULL(clki->clk)) { > > > > > + freq = dev_pm_opp_get_freq_indexed(opp, idx++); > > > > > + > > > > > + /* Do not set rate for clocks having frequency as 0 */ > > > > > + if (!freq) > > > > > + continue; > > > > > > > > Can we omit these clocks from the opp table? I don't think they serve any > > > > purpose. > > > > > > > > > > No, we cannot. OPP requires the clocks and opp-hz to be of same length. > > I am okay with having a patch for the OPP core to modify this > behavior, as I told privately earlier. > > > > And we > > > cannot omit those clocks as well since linux needs to gate control them. > > > > Hmm, I thought we push the list of "interesting" clocks through > > config->clock_names. > > Yes, another way to solve this would be keep the interesting clocks in > the beginning in "clock-names" field and let the platform pass only > those to the OPP core. > > > > > > > > Maybe it would even make sense to move this function to drivers/opp then, as > > > > it will be generic enough. > > > > > > > > > > There is already a generic function available in OPP core. But we cannot use it > > > as we need to skip setting 0 freq and that's not applicable in OPP core as > > > discussed with Viresh offline. > > > > Ack. > > I am okay with either of the solutions, it is for you guys to decide > what works better for your platform. > We can settle with this custom callback for now. If there are drivers in the future trying to do the same (skipping 0 freq) then we can generalize. - Mani > -- > viresh
On 13-07-23, 10:35, Manivannan Sadhasivam wrote: > We can settle with this custom callback for now. If there are drivers in the > future trying to do the same (skipping 0 freq) then we can generalize. Just for completeness, there isn't much to generalize here apart from changing the DT order of clocks. Isn't it ? The change require for the OPP core makes sense, I will probably just push it anyway.
On Thu, Jul 13, 2023 at 10:42:35AM +0530, Viresh Kumar wrote: > On 13-07-23, 10:35, Manivannan Sadhasivam wrote: > > We can settle with this custom callback for now. If there are drivers in the > > future trying to do the same (skipping 0 freq) then we can generalize. > > Just for completeness, there isn't much to generalize here apart from > changing the DT order of clocks. Isn't it ? > Even with changing the order, driver has to know the "interesting" clocks beforehand. But that varies between platforms (this is a generic driver for ufshc platforms). And I do not know if clocks have any dependency between them, atleast not in Qcom platforms. But not sure about others. - Mani > The change require for the OPP core makes sense, I will probably just > push it anyway. > > -- > viresh
Okay, sorry about missing one point first. I thought we are adding the clk config callback (which neglects 0 frequencies) to a Qcom only driver and so was okay-ish with that. But now that I realize that this is a generic driver instead (my mistake here), I wonder if it is the right thing to do anymore. On 13-07-23, 10:58, Manivannan Sadhasivam wrote: > On Thu, Jul 13, 2023 at 10:42:35AM +0530, Viresh Kumar wrote: > > On 13-07-23, 10:35, Manivannan Sadhasivam wrote: > > > We can settle with this custom callback for now. If there are drivers in the > > > future trying to do the same (skipping 0 freq) then we can generalize. > > > > Just for completeness, there isn't much to generalize here apart from > > changing the DT order of clocks. Isn't it ? > > > > Even with changing the order, driver has to know the "interesting" clocks > beforehand. But that varies between platforms (this is a generic driver for > ufshc platforms). > > And I do not know if clocks have any dependency between them, atleast not in > Qcom platforms. But not sure about others. Maybe this requires some sort of callback, per-platform, which gets you these details or the struct dev_pm_opp_config itself (so platforms can choose the callback too, in case order is important). > > The change require for the OPP core makes sense, I will probably just > > push it anyway. I tried to look at this code and I think it is doing the right thing currently, i.e. it matches clk-count with the number of frequencies in opp-hz, which should turn out to be the same in your case. So nothing to change here I guess.
On Thu, Jul 13, 2023 at 11:13:02AM +0530, Viresh Kumar wrote: > Okay, sorry about missing one point first. I thought we are adding the > clk config callback (which neglects 0 frequencies) to a Qcom only > driver and so was okay-ish with that. But now that I realize that this > is a generic driver instead (my mistake here), I wonder if it is the > right thing to do anymore. > That's the pre-opp behavior as well. Reason is, most of the platforms have only gate clocks supplied to the ufs controller and cannot change the frequency. Only Qcom requires changing the frequency of _some_ clocks, so that's why we have to use this hack of skipping 0 freq clocks. > On 13-07-23, 10:58, Manivannan Sadhasivam wrote: > > On Thu, Jul 13, 2023 at 10:42:35AM +0530, Viresh Kumar wrote: > > > On 13-07-23, 10:35, Manivannan Sadhasivam wrote: > > > > We can settle with this custom callback for now. If there are drivers in the > > > > future trying to do the same (skipping 0 freq) then we can generalize. > > > > > > Just for completeness, there isn't much to generalize here apart from > > > changing the DT order of clocks. Isn't it ? > > > > > > > Even with changing the order, driver has to know the "interesting" clocks > > beforehand. But that varies between platforms (this is a generic driver for > > ufshc platforms). > > > > And I do not know if clocks have any dependency between them, atleast not in > > Qcom platforms. But not sure about others. > > Maybe this requires some sort of callback, per-platform, which gets > you these details or the struct dev_pm_opp_config itself (so platforms > can choose the callback too, in case order is important). > Yeah but that seems overkill since the current config_clks helper satisfies the requirement. - Mani > > > The change require for the OPP core makes sense, I will probably just > > > push it anyway. > > I tried to look at this code and I think it is doing the right thing > currently, i.e. it matches clk-count with the number of frequencies in > opp-hz, which should turn out to be the same in your case. So nothing > to change here I guess. > > -- > viresh
diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c index 0b7430033047..068c22378c88 100644 --- a/drivers/ufs/host/ufshcd-pltfrm.c +++ b/drivers/ufs/host/ufshcd-pltfrm.c @@ -8,8 +8,10 @@ * Vinayak Holikatti <h.vinayak@samsung.com> */ +#include <linux/clk.h> #include <linux/module.h> #include <linux/platform_device.h> +#include <linux/pm_opp.h> #include <linux/pm_runtime.h> #include <linux/of.h> @@ -17,6 +19,8 @@ #include "ufshcd-pltfrm.h" #include <ufs/unipro.h> +#include <trace/events/ufs.h> + #define UFSHCD_DEFAULT_LANES_PER_DIRECTION 2 static int ufshcd_parse_clock_info(struct ufs_hba *hba) @@ -205,6 +209,112 @@ static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba) } } +static int ufshcd_opp_config_clks(struct device *dev, struct opp_table *opp_table, + struct dev_pm_opp *opp, void *data, + bool scaling_down) +{ + struct ufs_hba *hba = dev_get_drvdata(dev); + struct list_head *head = &hba->clk_list_head; + struct ufs_clk_info *clki; + unsigned long freq; + u8 idx = 0; + int ret; + + list_for_each_entry(clki, head, list) { + if (!IS_ERR_OR_NULL(clki->clk)) { + freq = dev_pm_opp_get_freq_indexed(opp, idx++); + + /* Do not set rate for clocks having frequency as 0 */ + if (!freq) + continue; + + ret = clk_set_rate(clki->clk, freq); + if (ret) { + dev_err(dev, "%s: %s clk set rate(%ldHz) failed, %d\n", + __func__, clki->name, freq, ret); + return ret; + } + + trace_ufshcd_clk_scaling(dev_name(dev), + (scaling_down ? "scaled down" : "scaled up"), + clki->name, hba->clk_scaling.target_freq, freq); + } + } + + return 0; +} + +static int ufshcd_parse_operating_points(struct ufs_hba *hba) +{ + struct device *dev = hba->dev; + struct device_node *np = dev->of_node; + struct dev_pm_opp_config config = {}; + struct ufs_clk_info *clki; + const char **clk_names; + int cnt, i, ret; + + if (!of_find_property(np, "operating-points-v2", NULL)) + return 0; + + if (of_find_property(np, "freq-table-hz", NULL)) { + dev_err(dev, "%s: operating-points and freq-table-hz are incompatible\n", + __func__); + return -EINVAL; + } + + cnt = of_property_count_strings(np, "clock-names"); + if (cnt <= 0) { + dev_err(dev, "%s: Missing clock-names\n", __func__); + return -ENODEV; + } + + /* OPP expects clk_names to be NULL terminated */ + clk_names = devm_kcalloc(dev, cnt + 1, sizeof(*clk_names), GFP_KERNEL); + if (!clk_names) + return -ENOMEM; + + /* + * We still need to get reference to all clocks as the UFS core uses + * them separately. + */ + for (i = 0; i < cnt; i++) { + ret = of_property_read_string_index(np, "clock-names", i, + &clk_names[i]); + if (ret) + return ret; + + clki = devm_kzalloc(dev, sizeof(*clki), GFP_KERNEL); + if (!clki) + return -ENOMEM; + + clki->name = devm_kstrdup(dev, clk_names[i], GFP_KERNEL); + if (!clki->name) + return -ENOMEM; + + if (!strcmp(clk_names[i], "ref_clk")) + clki->keep_link_active = true; + + list_add_tail(&clki->list, &hba->clk_list_head); + } + + config.clk_names = clk_names, + config.config_clks = ufshcd_opp_config_clks; + + ret = devm_pm_opp_set_config(dev, &config); + if (ret) + return ret; + + ret = devm_pm_opp_of_add_table(dev); + if (ret) { + dev_err(dev, "Failed to add OPP table: %d\n", ret); + return ret; + } + + hba->use_pm_opp = true; + + return 0; +} + /** * ufshcd_get_pwr_dev_param - get finally agreed attributes for * power mode change @@ -371,6 +481,12 @@ int ufshcd_pltfrm_init(struct platform_device *pdev, ufshcd_init_lanes_per_dir(hba); + err = ufshcd_parse_operating_points(hba); + if (err) { + dev_err(dev, "%s: OPP parse failed %d\n", __func__, err); + goto dealloc_host; + } + err = ufshcd_init(hba, mmio_base, irq); if (err) { dev_err(dev, "Initialization failed\n");