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 9F4EBC433EF for ; Fri, 22 Apr 2022 10:41:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1446783AbiDVKog (ORCPT ); Fri, 22 Apr 2022 06:44:36 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42608 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1446674AbiDVKod (ORCPT ); Fri, 22 Apr 2022 06:44:33 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9A1A0F4F; Fri, 22 Apr 2022 03:41:33 -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 ams.source.kernel.org (Postfix) with ESMTPS id C11A8B82C14; Fri, 22 Apr 2022 10:41:31 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 535B0C385A0; Fri, 22 Apr 2022 10:41:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1650624090; bh=XgeHDscq4XiMKn3nikqVv26WbVAc1uoCYL9IXMXzt9U=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=U7rO0t94WUjIwU5upicuh6PLMNXEsvt+bhoOcdH63t/Tzz6noC7mBAFjt9F4SVCPC zhPYMjy8XQTXW3nUsCL5rB3dO0Fg6kXiv8NHCiVx0ypj1bfN+V8tQ2rtU1f/zIIjfD vMmhwAZFoLdjZ4Y/NzHy7a56lvCVySO8qfe1tEYlwUdHBK84jbKWYuBlFhyv0S/+Mw 3nchJRG4qQh3OUWbPpyMB/EQf2PDXJqXgnz2QALSevvdK3MAySn+EDH665betUemdD 8xQGZ2ykaOUOY3DPT/OQt2LmksyDnzOHePeNILOiIA4N/OEK+3UTj2DkmazxBDSmx4 qCfJo4EmMsRBw== Received: from johan by xi.lan with local (Exim 4.94.2) (envelope-from ) id 1nhqj6-0002YD-2f; Fri, 22 Apr 2022 12:41:24 +0200 Date: Fri, 22 Apr 2022 12:41:24 +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: devicetree@vger.kernel.org On Thu, Apr 21, 2022 at 02:36:05PM +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 > > --- > > +static int qcom_qmp_phy_pipe_clk_enable(struct qmp_phy *qphy) > > +{ > > + struct qcom_qmp *qmp = qphy->qmp; > > + int ret; > > + > > + ret = clk_set_parent(qphy->pipemux_clk, qmp->pipe_clksrc); > > + if (ret) > > + dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret); > > + > > + > > + ret = clk_prepare_enable(qphy->pipe_clk); > > + if (ret) { > > + dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret); > > + goto err_restore_parent; > > + } > > So, what you do here is you manually set the parent of > GCC_PCIE_1_PIPE_CLK_SRC to PHY pipe clock right before enabling > GCC_PCIE_1_PIPE_CLK and set it back to XO after disabling > GCC_PCIE_1_PIPE_CLK. > > My proposal is doing exactly the same, but doing that automatically > through the clock infrastructure. After removing pipe_clock handling > from pcie driver itself, we can be sure that nobody is playing dirty > tricks around the pipe_clock. Yes, the end result is similar, but I believe handling it explicitly in the driver is preferred for a number of reasons that I've already mentioned. Not least because the mux needs to be updated when the PHY is powered on, not when the GCC pipe clock is ungated. In practise, powering on the PHY and ungating the clock happen to coincide in time because only the PHY driver will use the GCC pipe clock, but conceptually they are unrelated (and as the GDSC hang shows, something in the system appears to be ungating the clock while the PHY is powered off). The QMP PHY driver implementation is much more straight forward and easier to reason about than having the mux implementation spread out over multiple clock drivers where it's not clear at all what is really going on or why (and even debugfs will give you a false view of the clock tree state). > > + > > + return 0; > > + > > +err_restore_parent: > > + clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk); > > + > > + return ret; > > +} > > + > > +static void qcom_qmp_phy_pipe_clk_disable(struct qmp_phy *qphy) > > +{ > > + struct qcom_qmp *qmp = qphy->qmp; > > + int ret; > > + > > + clk_disable_unprepare(qphy->pipe_clk); > > + > > + ret = clk_set_parent(qphy->pipemux_clk, qphy->piperef_clk); > > + if (ret) > > + dev_err(qmp->dev, "failed to reparent pipe clock: %d\n", ret); > > +} > > + Johan