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 9F655C433F5 for ; Fri, 22 Apr 2022 10:20:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1446496AbiDVKXt (ORCPT ); Fri, 22 Apr 2022 06:23:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55746 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235156AbiDVKXt (ORCPT ); Fri, 22 Apr 2022 06:23:49 -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 89E4C387BB; Fri, 22 Apr 2022 03:20:56 -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 26ED061E6E; Fri, 22 Apr 2022 10:20:56 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 78215C385A4; Fri, 22 Apr 2022 10:20:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1650622855; bh=l8DMuJMxhFg3oWj3P06LfAZsH8ZofBIGfJ+Zsxq3M3c=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=tXz8dnNMqZa1305TV+OZTAXZYU1mQErY7qQwDOrLEGInkAMbEIzqrG3b+N/MezYx+ nltOcc4WCszV0jlGsgS1jSmOpfo+zKsVzo3QSZyQ+n4wkjVgoZgzp6Ak5D7/TmmJ2X OqlL4P+FUNqoeKRzqD69pPyT0SP9R1wxn9ZML6uohJbjy12uHgUFabDfUL5BAu+U+I 3zZf2YNJh6naLmWslpXXKwp13wozl37QHGEhPsNnvVmppbSUoKH9TTGIzMJMBK5JKR ToquwwGVQQmCB9ufrgASyzefmiULPiVKvutwkGXcCs2etHqxKwQ7ihzemQ8w0C2IDF FgVInoYZe6cag== Received: from johan by xi.lan with local (Exim 4.94.2) (envelope-from ) id 1nhqPA-0002Om-Fm; Fri, 22 Apr 2022 12:20:49 +0200 Date: Fri, 22 Apr 2022 12:20:48 +0200 From: Johan Hovold To: Dmitry Baryshkov Cc: Johan Hovold , Andy Gross , Bjorn Andersson , Lorenzo Pieralisi , Kishon Vijay Abraham I , Vinod Koul , Stephen Boyd , Rob Herring , Krzysztof Kozlowski , Stanimir Varbanov , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Bjorn Helgaas , Prasad Malisetty , linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, linux-phy@lists.infradead.org Subject: Re: [PATCH RFC 1/5] phy: qcom-qmp: add support for pipe clock muxing Message-ID: References: <20220421102041.17345-1-johan+linaro@kernel.org> <20220421102041.17345-2-johan+linaro@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Thu, Apr 21, 2022 at 02:08:27PM +0300, Dmitry Baryshkov wrote: > On 21/04/2022 13:20, Johan Hovold wrote: > > Some QMP PHYs need to remux to their pipe clock input to the pipe clock > > output generated by the PHY before powering on the PHY and restore the > > default source during power down. > > > > Add support for an optional pipe clock mux which will be reparented to > > the generated pipe clock before powering on the PHY and restored to the > > default reference source on power off. > > > > Signed-off-by: Johan Hovold > > --- > > drivers/phy/qualcomm/phy-qcom-qmp.c | 71 ++++++++++++++++++++++++++--- > > 1 file changed, 65 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c > > index 7d2d1ab061f7..bc6db9670291 100644 > > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c > > @@ -3292,6 +3292,8 @@ struct qmp_phy_combo_cfg { > > * @rx2: iomapped memory space for second lane's rx (in dual lane PHYs) > > * @pcs_misc: iomapped memory space for lane's pcs_misc > > * @pipe_clk: pipe clock > > + * @pipemux_clk: pipe clock source mux > > + * @piperef_clk: pipe clock default reference source > > * @index: lane index > > * @qmp: QMP phy to which this lane belongs > > * @lane_rst: lane's reset controller > > @@ -3311,6 +3313,8 @@ struct qmp_phy { > > void __iomem *rx2; > > void __iomem *pcs_misc; > > struct clk *pipe_clk; > > + struct clk *pipemux_clk; > > + struct clk *piperef_clk; > > unsigned int index; > > struct qcom_qmp *qmp; > > struct reset_control *lane_rst; > > @@ -3346,6 +3350,7 @@ struct qcom_qmp { > > void __iomem *dp_com; > > > > struct clk_bulk_data *clks; > > + struct clk *pipe_clksrc; > > Please move this to qmp_phy too. Ok. > > + /* Get optional pipe clock mux and default reference source clock. */ > > + qphy->pipemux_clk = of_clk_get_by_name(np, "mux"); > > + if (IS_ERR(qphy->pipemux_clk)) { > > + ret = PTR_ERR(qphy->pipemux_clk); > > + if (ret == -EPROBE_DEFER) > > + return ret; > > + > > + qphy->pipemux_clk = NULL; > > This makes the driver ignore every possible erorr except -EPROBE_DEFER. > However the driver should behave in quite the oppposite way. Please use > devm_clk_get_optional() instead. It would do that in better way. We'd need to add an optional version of devm_get_clk_from_child() for that due to the questionable "lane" child nodes this driver uses. The above works for an RFC, but testing for -EINVAL and -ENOENT handles a few more theoretical errnos until an optional helper is in place. > Not to mention that this code leaks a refcount on the clock. True, just like the driver has been doing with the pipe clock and lane reset since it was merged. I'll fix that up. > > + } else { > > + qphy->piperef_clk = of_clk_get_by_name(np, "ref"); > > + if (IS_ERR(qphy->piperef_clk)) { > > + ret = PTR_ERR(qphy->piperef_clk); > > + return dev_err_probe(dev, ret, > > + "failed to get lane%d piperef_clk\n", > > + id); > > + } > > + } > > + > > As a second thought. > This needs to be more explicit. If the chipset requires the pipe clock > remuxing, we must fail if the clocks were not provided. So depending on > the qmp instance/property the driver should either use devm_clk_get() > (instead of _optional) or skip this block completely. No, the kernel is not a DT validator (and we have the YAML bindings for that now). > But this will not work with earlier DTS files. So this is not a problem (but if we really wanted to have the driver validate the DT it can be done by updating the compatible strings). Johan