Message ID | 20221019113552.22353-14-johan+linaro@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp300014wrs; Wed, 19 Oct 2022 05:33:26 -0700 (PDT) X-Google-Smtp-Source: AMsMyM5Wtwj27chrDNlDHaVYSKhfvIduWCgwHN2D4eIoqEzTQszov+poQTwrXuL3sguzY+VGeM1C X-Received: by 2002:a17:907:168c:b0:78d:8b6c:a209 with SMTP id hc12-20020a170907168c00b0078d8b6ca209mr6926879ejc.185.1666182806178; Wed, 19 Oct 2022 05:33:26 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666182806; cv=none; d=google.com; s=arc-20160816; b=VS5ClOoZkZKtNk7swTMyEXP4S4ynMROypGb93aneYvRJRp0LwNphK0/s18XEnIcp1A SD0D2dJDC66eHVg3Sii/Cu2NNjLqsGMXe4IFmmsbCD1kvisq9bhZFsTr/9GXDg6i5/jS WZSL0W+CH8oHoZK/kr9EKeYiEYyn/5wmxUlggIau0oFyyhSty0N2JO1a82BmBjXsKPtX VY2EGqI4DNcuLc5JfwTv+UWtJ2fPRnievNxej38DJNPwwDgTLZXxCjNPFrS0vyQdVrw3 Msz+nv23D/o3ngjvu7YRgeKjB7iqLjspePFZsvag3hW66fG9Q16UNCFW+GD/Ita8FoLL eJdw== 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=bO4gJlple3/SG2VRxOUg3KQUnlmA2PCazSPc/ud6IC0=; b=nD4GSrTH+3yIL0CWvJO2CKvF26ho8xYEFIm5X/7Dj/Lil89fG9Zyj8PfQ+qgnczKC9 v/yAHa22Ox3+vDpKsz2aXxFBPY/4SrTUgOpNBpb1+A8aV2ORYmilD1lYK9woVDFWKzti T3Z2vbwSYDt+bnY4XKDYy73H1HrccJ1KbWWCk7FwRArWPq1IczOsDkS8HqNbLm0nhtC6 8+K/S4MvGjXZ3KVbVadfbFa3qUjIP2x5R5t+1SfCpiK9MUsvw9w7KGDvecAmZQOWKOG4 A1vpu2h2Xir3oqOrBQg45yx3uGmD85oYa12jDIEunq3kqTPzcKspzTYJp+d4ab7YtwIh r1ww== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=e4lz3FXV; 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=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id sh40-20020a1709076ea800b0078d1e610b3esi14024727ejc.26.2022.10.19.05.33.00; Wed, 19 Oct 2022 05:33:26 -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=@kernel.org header.s=k20201202 header.b=e4lz3FXV; 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=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232971AbiJSMU2 (ORCPT <rfc822;samuel.l.nystrom@gmail.com> + 99 others); Wed, 19 Oct 2022 08:20:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38034 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233215AbiJSMTK (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 19 Oct 2022 08:19:10 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B5F1A1D672; Wed, 19 Oct 2022 04:54:27 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 8C72760E9B; Wed, 19 Oct 2022 11:36:20 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B16C0C4FF11; Wed, 19 Oct 2022 11:36:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1666179379; bh=5U2mLRcMewZ/OXbcaeXyQCALagcvHGf09jg9Pu1Jzck=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=e4lz3FXVHtouVjpeHJ/MP5Ed7MbLAkAa3aBzd9bHam99R89pARdz5WQhbm9ca8VM+ L+hDbbrbh1Tbqs/5QrVZ2YMT/pu4Iuj3/KQwE2r0NgnigEoVYvwBuUAjSTmSs4GeIV A3MdMpHIA1jGL2OyDfjGyGK/9fm8vxA+N2aRkjCigjCQokIhCF/pfjpFOvGHy7umcz FnCh8o0ky/xApZzu+KCZggWIxGmnIiaOHHM6ywyzLKmHJlbPVsloqkjo8Ws890Y267 wvkuHxlML8F7IVTvI9eRG1OU3RQL4kR+SvnQ1Pgmxc3W4CCX3CiwJHNSknu+3abbMl srCUtCOd/YwkQ== Received: from johan by xi.lan with local (Exim 4.94.2) (envelope-from <johan+linaro@kernel.org>) id 1ol7Mm-0005po-CU; Wed, 19 Oct 2022 13:36:08 +0200 From: Johan Hovold <johan+linaro@kernel.org> To: Vinod Koul <vkoul@kernel.org> Cc: Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@somainline.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Johan Hovold <johan+linaro@kernel.org> Subject: [PATCH v2 13/15] phy: qcom-qmp-pcie: add support for pipediv2 clock Date: Wed, 19 Oct 2022 13:35:50 +0200 Message-Id: <20221019113552.22353-14-johan+linaro@kernel.org> X-Mailer: git-send-email 2.37.3 In-Reply-To: <20221019113552.22353-1-johan+linaro@kernel.org> References: <20221019113552.22353-1-johan+linaro@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS 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: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747119302043094452?= X-GMAIL-MSGID: =?utf-8?q?1747119302043094452?= |
Series |
phy: qcom-qmp-pcie: add support for sc8280xp
|
|
Commit Message
Johan Hovold
Oct. 19, 2022, 11:35 a.m. UTC
Some QMP PHYs have a second fixed-divider pipe clock that needs to be
enabled along with the pipe clock.
Add support for an optional "pipediv2" clock.
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 42 ++++++++++++++++++++----
1 file changed, 36 insertions(+), 6 deletions(-)
Comments
On 19/10/2022 14:35, Johan Hovold wrote: > Some QMP PHYs have a second fixed-divider pipe clock that needs to be > enabled along with the pipe clock. > > Add support for an optional "pipediv2" clock. > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 42 ++++++++++++++++++++---- > 1 file changed, 36 insertions(+), 6 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > index 9c8e009033f1..c1d74c06fad1 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > @@ -1379,7 +1379,9 @@ struct qmp_pcie { > void __iomem *rx2; > > struct clk *pipe_clk; > + struct clk *pipediv2_clk; > struct clk_bulk_data *clks; > + > struct reset_control_bulk_data *resets; > struct regulator_bulk_data *vregs; > > @@ -1902,6 +1904,36 @@ static int qmp_pcie_exit(struct phy *phy) > return 0; > } > > +static int pipe_clk_enable(struct qmp_pcie *qmp) > +{ > + int ret; > + > + ret = clk_prepare_enable(qmp->pipe_clk); > + if (ret) { > + dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret); > + return ret; > + } > + > + ret = clk_prepare_enable(qmp->pipediv2_clk); > + if (ret) { > + dev_err(qmp->dev, "failed to enable pipediv2 clock: %d\n", ret); > + goto err_disable_pipe_clk; > + } > + > + return 0; > + > +err_disable_pipe_clk: > + clk_disable_unprepare(qmp->pipe_clk); > + > + return ret; > +} > + > +static void pipe_clk_disable(struct qmp_pcie *qmp) > +{ > + clk_disable_unprepare(qmp->pipediv2_clk); > + clk_disable_unprepare(qmp->pipe_clk); > +} > + > static int qmp_pcie_power_on(struct phy *phy) > { > struct qmp_pcie *qmp = phy_get_drvdata(phy); > @@ -1923,11 +1955,9 @@ static int qmp_pcie_power_on(struct phy *phy) > qmp_pcie_init_registers(qmp, &cfg->tables); > qmp_pcie_init_registers(qmp, mode_tables); > > - ret = clk_prepare_enable(qmp->pipe_clk); > - if (ret) { > - dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret); > + ret = pipe_clk_enable(qmp); > + if (ret) > return ret; > - } > > /* Pull PHY out of reset state */ > qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); > @@ -1950,7 +1980,7 @@ static int qmp_pcie_power_on(struct phy *phy) > return 0; > > err_disable_pipe_clk: > - clk_disable_unprepare(qmp->pipe_clk); > + pipe_clk_disable(qmp); > > return ret; > } > @@ -1960,7 +1990,7 @@ static int qmp_pcie_power_off(struct phy *phy) > struct qmp_pcie *qmp = phy_get_drvdata(phy); > const struct qmp_phy_cfg *cfg = qmp->cfg; > > - clk_disable_unprepare(qmp->pipe_clk); > + pipe_clk_disable(qmp); > > /* PHY reset */ > qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); I still think that the attached patch is somewhat simpler. Diffstat supports that idea: $ diffstat /tmp/pipe.diff phy-qcom-qmp-pcie.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) Yes, I'm speaking this after having cleaned up several open-coded versions of clk_bulk_foo from the drm/msm code. It typically starts with the 'just another clock' story, and then suddenly they are all over the code.
On Thu, Oct 20, 2022 at 11:31:35AM +0300, Dmitry Baryshkov wrote: > On 19/10/2022 14:35, Johan Hovold wrote: > > Some QMP PHYs have a second fixed-divider pipe clock that needs to be > > enabled along with the pipe clock. > > > > Add support for an optional "pipediv2" clock. > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > > --- > > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 42 ++++++++++++++++++++---- > > 1 file changed, 36 insertions(+), 6 deletions(-) > I still think that the attached patch is somewhat simpler. Diffstat > supports that idea: > > $ diffstat /tmp/pipe.diff > phy-qcom-qmp-pcie.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) It's not just about LoC. > Yes, I'm speaking this after having cleaned up several open-coded > versions of clk_bulk_foo from the drm/msm code. It typically starts with > the 'just another clock' story, and then suddenly they are all over the > code. But you don't start using the bulk API when you have one clock. Two, maybe. Three, sure. It's a decision that needs to be done on a case-by case basis, and pipe clocks for the QMP block is different from general interface clocks (which are more likely to be extended). (And of course the clocks would need to be independent in the first place.) Here's your example diff inline: diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c index 9c8e009033f1..a148b143dd90 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c @@ -1378,8 +1378,10 @@ struct qmp_pcie { void __iomem *tx2; void __iomem *rx2; - struct clk *pipe_clk; + struct clk_bulk_data *pipe_clks; + int num_pipe_clks; struct clk_bulk_data *clks; + struct reset_control_bulk_data *resets; struct regulator_bulk_data *vregs; @@ -1923,11 +1925,9 @@ static int qmp_pcie_power_on(struct phy *phy) qmp_pcie_init_registers(qmp, &cfg->tables); qmp_pcie_init_registers(qmp, mode_tables); - ret = clk_prepare_enable(qmp->pipe_clk); - if (ret) { - dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret); + ret = clk_bulk_prepare_enable(qmp->num_pipe_clks, qmp->pipe_clks); + if (ret) return ret; - } /* Pull PHY out of reset state */ qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); @@ -1950,7 +1950,7 @@ static int qmp_pcie_power_on(struct phy *phy) return 0; err_disable_pipe_clk: - clk_disable_unprepare(qmp->pipe_clk); + clk_bulk_disable_unprepare(qmp->num_pipe_clks, qmp->pipe_clks); return ret; } @@ -1960,7 +1960,7 @@ static int qmp_pcie_power_off(struct phy *phy) struct qmp_pcie *qmp = phy_get_drvdata(phy); const struct qmp_phy_cfg *cfg = qmp->cfg; - clk_disable_unprepare(qmp->pipe_clk); + clk_bulk_disable_unprepare(qmp->num_pipe_clks, qmp->pipe_clks); /* PHY reset */ qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); The above is pretty much identical, expect for using the bulk API instead of the very straight-forward pipe-clock helpers. @@ -2154,6 +2154,7 @@ static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np struct platform_device *pdev = to_platform_device(qmp->dev); const struct qmp_phy_cfg *cfg = qmp->cfg; struct device *dev = qmp->dev; + struct clk *clk; qmp->serdes = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(qmp->serdes)) @@ -2206,12 +2207,17 @@ static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np } } - qmp->pipe_clk = devm_get_clk_from_child(dev, np, NULL); - if (IS_ERR(qmp->pipe_clk)) { - return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk), + clk = devm_get_clk_from_child(dev, np, NULL); + if (IS_ERR(clk)) { + return dev_err_probe(dev, PTR_ERR(clk), "failed to get pipe clock\n"); } + qmp->num_pipe_clks = 1; + qmp->pipe_clks = devm_kcalloc(dev, qmp->num_pipe_clks, + sizeof(*qmp->pipe_clks), GFP_KERNEL); + qmp->pipe_clks[0].clk = clk; So here you're poking at bulk API internals and forgot to set the id string, which the implementation uses. For reasons like this, and the fact that will likely never have a third pipe clock, I'm reluctant to using the bulk API. + return 0; } Johan
On 20/10/2022 12:05, Johan Hovold wrote: > On Thu, Oct 20, 2022 at 11:31:35AM +0300, Dmitry Baryshkov wrote: >> On 19/10/2022 14:35, Johan Hovold wrote: >>> Some QMP PHYs have a second fixed-divider pipe clock that needs to be >>> enabled along with the pipe clock. >>> >>> Add support for an optional "pipediv2" clock. >>> >>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> >>> --- >>> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 42 ++++++++++++++++++++---- >>> 1 file changed, 36 insertions(+), 6 deletions(-) > >> I still think that the attached patch is somewhat simpler. Diffstat >> supports that idea: >> >> $ diffstat /tmp/pipe.diff >> phy-qcom-qmp-pcie.c | 26 ++++++++++++++++---------- >> 1 file changed, 16 insertions(+), 10 deletions(-) > > It's not just about LoC. > >> Yes, I'm speaking this after having cleaned up several open-coded >> versions of clk_bulk_foo from the drm/msm code. It typically starts with >> the 'just another clock' story, and then suddenly they are all over the >> code. > > But you don't start using the bulk API when you have one clock. Two, > maybe. Three, sure. It's a decision that needs to be done on a case-by > case basis, and pipe clocks for the QMP block is different from general > interface clocks (which are more likely to be extended). (And of course > the clocks would need to be independent in the first place.) > > Here's your example diff inline: > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > index 9c8e009033f1..a148b143dd90 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > @@ -1378,8 +1378,10 @@ struct qmp_pcie { > void __iomem *tx2; > void __iomem *rx2; > > - struct clk *pipe_clk; > + struct clk_bulk_data *pipe_clks; > + int num_pipe_clks; > struct clk_bulk_data *clks; > + > struct reset_control_bulk_data *resets; > struct regulator_bulk_data *vregs; > > @@ -1923,11 +1925,9 @@ static int qmp_pcie_power_on(struct phy *phy) > qmp_pcie_init_registers(qmp, &cfg->tables); > qmp_pcie_init_registers(qmp, mode_tables); > > - ret = clk_prepare_enable(qmp->pipe_clk); > - if (ret) { > - dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret); > + ret = clk_bulk_prepare_enable(qmp->num_pipe_clks, qmp->pipe_clks); > + if (ret) > return ret; > - } > > /* Pull PHY out of reset state */ > qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); > @@ -1950,7 +1950,7 @@ static int qmp_pcie_power_on(struct phy *phy) > return 0; > > err_disable_pipe_clk: > - clk_disable_unprepare(qmp->pipe_clk); > + clk_bulk_disable_unprepare(qmp->num_pipe_clks, qmp->pipe_clks); > > return ret; > } > @@ -1960,7 +1960,7 @@ static int qmp_pcie_power_off(struct phy *phy) > struct qmp_pcie *qmp = phy_get_drvdata(phy); > const struct qmp_phy_cfg *cfg = qmp->cfg; > > - clk_disable_unprepare(qmp->pipe_clk); > + clk_bulk_disable_unprepare(qmp->num_pipe_clks, qmp->pipe_clks); > > /* PHY reset */ > qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); > > The above is pretty much identical, expect for using the bulk API > instead of the very straight-forward pipe-clock helpers. > > @@ -2154,6 +2154,7 @@ static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np > struct platform_device *pdev = to_platform_device(qmp->dev); > const struct qmp_phy_cfg *cfg = qmp->cfg; > struct device *dev = qmp->dev; > + struct clk *clk; > > qmp->serdes = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(qmp->serdes)) > @@ -2206,12 +2207,17 @@ static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np > } > } > > - qmp->pipe_clk = devm_get_clk_from_child(dev, np, NULL); > - if (IS_ERR(qmp->pipe_clk)) { > - return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk), > + clk = devm_get_clk_from_child(dev, np, NULL); > + if (IS_ERR(clk)) { > + return dev_err_probe(dev, PTR_ERR(clk), > "failed to get pipe clock\n"); > } > > + qmp->num_pipe_clks = 1; > + qmp->pipe_clks = devm_kcalloc(dev, qmp->num_pipe_clks, > + sizeof(*qmp->pipe_clks), GFP_KERNEL); > + qmp->pipe_clks[0].clk = clk; > > So here you're poking at bulk API internals and forgot to set the id > string, which the implementation uses. I didn't forget, I just skipped setting it. Hmm. I thought that it is used only for clk_bulk_get. But after checking, it seems it's also used for error messages. Mea culpa. But it's not that I was poking into the internals. These items are in the public header. > > For reasons like this, and the fact that will likely never have a third > pipe clock, I'm reluctant to using the bulk API. Let's resort to the maintainer opinion then. > > + > return 0; > } > > Johan
On Thu, Oct 20, 2022 at 12:28:14PM +0300, Dmitry Baryshkov wrote: > On 20/10/2022 12:05, Johan Hovold wrote: > > Here's your example diff inline: > > @@ -2206,12 +2207,17 @@ static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np > > } > > } > > > > - qmp->pipe_clk = devm_get_clk_from_child(dev, np, NULL); > > - if (IS_ERR(qmp->pipe_clk)) { > > - return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk), > > + clk = devm_get_clk_from_child(dev, np, NULL); > > + if (IS_ERR(clk)) { > > + return dev_err_probe(dev, PTR_ERR(clk), > > "failed to get pipe clock\n"); > > } > > > > + qmp->num_pipe_clks = 1; > > + qmp->pipe_clks = devm_kcalloc(dev, qmp->num_pipe_clks, > > + sizeof(*qmp->pipe_clks), GFP_KERNEL); > > + qmp->pipe_clks[0].clk = clk; > > > > So here you're poking at bulk API internals and forgot to set the id > > string, which the implementation uses. > > I didn't forget, I just skipped setting it. Hmm. I thought that it is > used only for clk_bulk_get. But after checking, it seems it's also used > for error messages. Mea culpa. > > But it's not that I was poking into the internals. These items are in > the public header. My point is that you're not using the bulk API as it was intended (e.g. with clk_bulk_get()) and you risk running into issues like the above. And looking up the actual clock name for this is overkill, even in the case were it is provided, so we'd need to set it unconditionally to "pipe" (which is fine). > > For reasons like this, and the fact that will likely never have a third > > pipe clock, I'm reluctant to using the bulk API. > > Let's resort to the maintainer opinion then. I'll take another look at it too. Johan
On 20/10/2022 13:49, Johan Hovold wrote: > On Thu, Oct 20, 2022 at 12:28:14PM +0300, Dmitry Baryshkov wrote: >> On 20/10/2022 12:05, Johan Hovold wrote: > >>> Here's your example diff inline: > >>> @@ -2206,12 +2207,17 @@ static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np >>> } >>> } >>> >>> - qmp->pipe_clk = devm_get_clk_from_child(dev, np, NULL); >>> - if (IS_ERR(qmp->pipe_clk)) { >>> - return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk), >>> + clk = devm_get_clk_from_child(dev, np, NULL); >>> + if (IS_ERR(clk)) { >>> + return dev_err_probe(dev, PTR_ERR(clk), >>> "failed to get pipe clock\n"); >>> } >>> >>> + qmp->num_pipe_clks = 1; >>> + qmp->pipe_clks = devm_kcalloc(dev, qmp->num_pipe_clks, >>> + sizeof(*qmp->pipe_clks), GFP_KERNEL); >>> + qmp->pipe_clks[0].clk = clk; >>> >>> So here you're poking at bulk API internals and forgot to set the id >>> string, which the implementation uses. >> >> I didn't forget, I just skipped setting it. Hmm. I thought that it is >> used only for clk_bulk_get. But after checking, it seems it's also used >> for error messages. Mea culpa. >> >> But it's not that I was poking into the internals. These items are in >> the public header. > > My point is that you're not using the bulk API as it was intended (e.g. > with clk_bulk_get()) and you risk running into issues like the above. > > And looking up the actual clock name for this is overkill, even in the > case were it is provided, so we'd need to set it unconditionally to > "pipe" (which is fine). > >>> For reasons like this, and the fact that will likely never have a third >>> pipe clock, I'm reluctant to using the bulk API. >> >> Let's resort to the maintainer opinion then. > > I'll take another look at it too. Thanks!
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c index 9c8e009033f1..c1d74c06fad1 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c @@ -1379,7 +1379,9 @@ struct qmp_pcie { void __iomem *rx2; struct clk *pipe_clk; + struct clk *pipediv2_clk; struct clk_bulk_data *clks; + struct reset_control_bulk_data *resets; struct regulator_bulk_data *vregs; @@ -1902,6 +1904,36 @@ static int qmp_pcie_exit(struct phy *phy) return 0; } +static int pipe_clk_enable(struct qmp_pcie *qmp) +{ + int ret; + + ret = clk_prepare_enable(qmp->pipe_clk); + if (ret) { + dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret); + return ret; + } + + ret = clk_prepare_enable(qmp->pipediv2_clk); + if (ret) { + dev_err(qmp->dev, "failed to enable pipediv2 clock: %d\n", ret); + goto err_disable_pipe_clk; + } + + return 0; + +err_disable_pipe_clk: + clk_disable_unprepare(qmp->pipe_clk); + + return ret; +} + +static void pipe_clk_disable(struct qmp_pcie *qmp) +{ + clk_disable_unprepare(qmp->pipediv2_clk); + clk_disable_unprepare(qmp->pipe_clk); +} + static int qmp_pcie_power_on(struct phy *phy) { struct qmp_pcie *qmp = phy_get_drvdata(phy); @@ -1923,11 +1955,9 @@ static int qmp_pcie_power_on(struct phy *phy) qmp_pcie_init_registers(qmp, &cfg->tables); qmp_pcie_init_registers(qmp, mode_tables); - ret = clk_prepare_enable(qmp->pipe_clk); - if (ret) { - dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret); + ret = pipe_clk_enable(qmp); + if (ret) return ret; - } /* Pull PHY out of reset state */ qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET); @@ -1950,7 +1980,7 @@ static int qmp_pcie_power_on(struct phy *phy) return 0; err_disable_pipe_clk: - clk_disable_unprepare(qmp->pipe_clk); + pipe_clk_disable(qmp); return ret; } @@ -1960,7 +1990,7 @@ static int qmp_pcie_power_off(struct phy *phy) struct qmp_pcie *qmp = phy_get_drvdata(phy); const struct qmp_phy_cfg *cfg = qmp->cfg; - clk_disable_unprepare(qmp->pipe_clk); + pipe_clk_disable(qmp); /* PHY reset */ qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);