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 46512C4332F for ; Fri, 29 Apr 2022 06:53:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1349898AbiD2G4p (ORCPT ); Fri, 29 Apr 2022 02:56:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53080 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1348954AbiD2G4p (ORCPT ); Fri, 29 Apr 2022 02:56:45 -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 C323756768; Thu, 28 Apr 2022 23:53: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 ams.source.kernel.org (Postfix) with ESMTPS id 0E9D3B832F4; Fri, 29 Apr 2022 06:53:26 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B806DC385AC; Fri, 29 Apr 2022 06:53:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1651215204; bh=jFzLIgFDBXvEXK9brj3GiRx598j1W1BP4ZGK9938VXo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=TYg+QiGKPQu2FAiyx7bvU2SOo7ce9+OXiJ0n1cAuExR2I38Zq8OapYQCAgiO2T8bV lTY+gdKLQKEKrpiuBBs8AgJje9vxzh9f2nQd9Zyk3aeWv9u6CnW4fjlhykrxXOYPfR pMuewp9WzGC0sO5SHR6BGUlzm/0KxwkerfQwrzBJq17sHM1ukKURBvoO3Hsc2bECHs lRf+J4LuM2YTV0bTPnyI/zMZQO+DudRZTBsAe2x8DAefgFrqKYC+QAXpCLnOsriNZu nxPKOtA+EKE9ILxmzg8Kc5bDPZ9VYIJKt78U5qxiaRTSa9QUZOvH9lyhOU2l65ZBI3 fQHdYTnQyNfNg== Received: from johan by xi.lan with local (Exim 4.94.2) (envelope-from ) id 1nkKVI-0001ir-A6; Fri, 29 Apr 2022 08:53:25 +0200 Date: Fri, 29 Apr 2022 08:53:24 +0200 From: Johan Hovold To: Bjorn Andersson Cc: Johan Hovold , Andy Gross , Lorenzo Pieralisi , Kishon Vijay Abraham I , Vinod Koul , Stephen Boyd , Rob Herring , Krzysztof Kozlowski , Stanimir Varbanov , Krzysztof Wilczy??ski , Bjorn Helgaas , Dmitry Baryshkov , 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-arm-msm@vger.kernel.org On Thu, Apr 28, 2022 at 06:11:44AM -0700, Bjorn Andersson wrote: > On Thu 21 Apr 03:20 PDT 2022, 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. > > > > After considering this for a while, I have two objections to doing this > explicitly: > > 1) The QMP block is fed &gcc_pcie_N_pipe_clk (and on sc8280xp) > gcc_pcie_N_pipediv2_clk. But neither of these clocks are the mux, so > what this patch (and the existing muxing logic in the controller) does > is to poke into gcc "internals". I agree that this is perhaps the strongest argument against describing the mux in DT (as is also currently done for sc7280). > 2) The actual reason for the mux dance is that toggling the associated > GDSC without a valid parent of this clock would cause the clock to lock > up and GDSC transition to time out. This property is shared with a wide > range of other clocks (so far we have 84 users of clk_rcg2_shared_ops on > sc8280xp). Right, but the situation with rcg2 is a little different. From what I gather the problem there is that some downstream clock could have been enabled by some other part of the system behind the kernel's back and then things go wrong when the kernel configures the clock. Here it is the kernel that controls the source of the pipe clock mux (the PHY PLL) and knows when the source is valid (and the PHY is both provider and consumer of the pipe clock). For rcg2, there's no choice but to work around the hardware in the clock driver, while for QMP the PHY power sequences could be made explicit in the driver: clk_prepare_enable(pipe); clk_set_parent(pipe_src, pll); start_pll(); stop_pll(); clk_set_parent(pipe_src, xo); clk_disable_unprepare(pipe); Note that with the above sequences it would be possible to drop the pipe clock BRANCH_HALT_SKIP flag, which is only there in the clock driver because of the how the PHY works (i.e. that the pipe clock must be ungated before the PLL is started). (This wouldn't be possible with a pipe-mux implementation in the clock driver since the parent mux would be enabled before the child pipe clock.) But sure, the requirement to restore XO to prevent a later GDSC hang has little to do with the PHY. > It would be nice if clk_summary would represent the real state of these > clocks, but unfortunately I don't think the state matches reality with > this approach either. > > E.g. we prepare/enable the pipe clock before setting > QPHY_POWER_DOWN_CONTROL, during this time there's shouldn't be any pipe > clock coming out of the PHY... Right, there's a small window there where the source is still off (due to hardware requirements), but at least the topology is always reported correctly, which is really useful when dealing with boot handover. I have an sc8280xp here where the boot firmware leaves the pipe clock muxed in despite the GDSC being disabled. Fortunately, it doesn't seem to trigger the lockup when toggling the GDSC as it does on an sa8540p. My concerns are otherwise mostly related to the implementation of the safe-mux (e.g. ad-hoc, missing locking) and can probably be addressed. As an example, with the current implementation it is not possible to use an assigned clock parent in DT to make sure that the XO source is selected before toggling the GDSC. The muxing doesn't happen until the pipe clock is enabled, which is much too late in case the boot firmware left the pipe clock muxed in, and when enabling the pipe clock you really want the PHY PLL as source and not the cached XO. Can you still drop the current safe-mux patches from your tree or would they need to be fixed up incrementally? I think you merged v2, but there's already a v3 out (addressing the hardcoded mux configuration values). Johan 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 13FD9C433EF for ; Fri, 29 Apr 2022 06:53:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=HDgDu5tP5rxIMnQFGtlKcm3cKuNA2EJimkGsrvOM75Q=; b=33vxfT58DiyK2U E5aAiprJS2TOkQOJi8KIlAspW2S5NtdO907oX4Q7C9+KFEDDfkoTLTNTGtaC88TJWKZH7cyvcakwP jHb6ErSAaeEexUR3YzHbxrivyMclodOOZSKVEPRLyRo3waOy8PPVGm0UWIBy2tIV1m6GdAEfc6inE KOi7VlMDwkgm/k33yxFtd6GIl3wTlZhBsPIFFOStxiyCnT75YW9I1ocjIv4qP5vW3dK1Wn1V3Q27y K0dMHTbTjhrHshU55GxP77UT2S0EDauqsqCf5P4Ut+ShuTmrZtcaMJSpjkNequgx0yK6K8uZbAnYj WeVy/Cdk2LB9EQ0yAOPw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nkKVR-009rTs-FU; Fri, 29 Apr 2022 06:53:33 +0000 Received: from ams.source.kernel.org ([145.40.68.75]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nkKVO-009rSJ-Ap for linux-phy@lists.infradead.org; Fri, 29 Apr 2022 06:53:32 +0000 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 F1891B832F1; Fri, 29 Apr 2022 06:53:25 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B806DC385AC; Fri, 29 Apr 2022 06:53:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1651215204; bh=jFzLIgFDBXvEXK9brj3GiRx598j1W1BP4ZGK9938VXo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=TYg+QiGKPQu2FAiyx7bvU2SOo7ce9+OXiJ0n1cAuExR2I38Zq8OapYQCAgiO2T8bV lTY+gdKLQKEKrpiuBBs8AgJje9vxzh9f2nQd9Zyk3aeWv9u6CnW4fjlhykrxXOYPfR pMuewp9WzGC0sO5SHR6BGUlzm/0KxwkerfQwrzBJq17sHM1ukKURBvoO3Hsc2bECHs lRf+J4LuM2YTV0bTPnyI/zMZQO+DudRZTBsAe2x8DAefgFrqKYC+QAXpCLnOsriNZu nxPKOtA+EKE9ILxmzg8Kc5bDPZ9VYIJKt78U5qxiaRTSa9QUZOvH9lyhOU2l65ZBI3 fQHdYTnQyNfNg== Received: from johan by xi.lan with local (Exim 4.94.2) (envelope-from ) id 1nkKVI-0001ir-A6; Fri, 29 Apr 2022 08:53:25 +0200 Date: Fri, 29 Apr 2022 08:53:24 +0200 From: Johan Hovold To: Bjorn Andersson Cc: Johan Hovold , Andy Gross , Lorenzo Pieralisi , Kishon Vijay Abraham I , Vinod Koul , Stephen Boyd , Rob Herring , Krzysztof Kozlowski , Stanimir Varbanov , Krzysztof Wilczy??ski , Bjorn Helgaas , Dmitry Baryshkov , 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-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220428_235330_698619_95B81BEE X-CRM114-Status: GOOD ( 35.29 ) X-BeenThere: linux-phy@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Linux Phy Mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-phy" Errors-To: linux-phy-bounces+linux-phy=archiver.kernel.org@lists.infradead.org On Thu, Apr 28, 2022 at 06:11:44AM -0700, Bjorn Andersson wrote: > On Thu 21 Apr 03:20 PDT 2022, 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. > > > > After considering this for a while, I have two objections to doing this > explicitly: > > 1) The QMP block is fed &gcc_pcie_N_pipe_clk (and on sc8280xp) > gcc_pcie_N_pipediv2_clk. But neither of these clocks are the mux, so > what this patch (and the existing muxing logic in the controller) does > is to poke into gcc "internals". I agree that this is perhaps the strongest argument against describing the mux in DT (as is also currently done for sc7280). > 2) The actual reason for the mux dance is that toggling the associated > GDSC without a valid parent of this clock would cause the clock to lock > up and GDSC transition to time out. This property is shared with a wide > range of other clocks (so far we have 84 users of clk_rcg2_shared_ops on > sc8280xp). Right, but the situation with rcg2 is a little different. From what I gather the problem there is that some downstream clock could have been enabled by some other part of the system behind the kernel's back and then things go wrong when the kernel configures the clock. Here it is the kernel that controls the source of the pipe clock mux (the PHY PLL) and knows when the source is valid (and the PHY is both provider and consumer of the pipe clock). For rcg2, there's no choice but to work around the hardware in the clock driver, while for QMP the PHY power sequences could be made explicit in the driver: clk_prepare_enable(pipe); clk_set_parent(pipe_src, pll); start_pll(); stop_pll(); clk_set_parent(pipe_src, xo); clk_disable_unprepare(pipe); Note that with the above sequences it would be possible to drop the pipe clock BRANCH_HALT_SKIP flag, which is only there in the clock driver because of the how the PHY works (i.e. that the pipe clock must be ungated before the PLL is started). (This wouldn't be possible with a pipe-mux implementation in the clock driver since the parent mux would be enabled before the child pipe clock.) But sure, the requirement to restore XO to prevent a later GDSC hang has little to do with the PHY. > It would be nice if clk_summary would represent the real state of these > clocks, but unfortunately I don't think the state matches reality with > this approach either. > > E.g. we prepare/enable the pipe clock before setting > QPHY_POWER_DOWN_CONTROL, during this time there's shouldn't be any pipe > clock coming out of the PHY... Right, there's a small window there where the source is still off (due to hardware requirements), but at least the topology is always reported correctly, which is really useful when dealing with boot handover. I have an sc8280xp here where the boot firmware leaves the pipe clock muxed in despite the GDSC being disabled. Fortunately, it doesn't seem to trigger the lockup when toggling the GDSC as it does on an sa8540p. My concerns are otherwise mostly related to the implementation of the safe-mux (e.g. ad-hoc, missing locking) and can probably be addressed. As an example, with the current implementation it is not possible to use an assigned clock parent in DT to make sure that the XO source is selected before toggling the GDSC. The muxing doesn't happen until the pipe clock is enabled, which is much too late in case the boot firmware left the pipe clock muxed in, and when enabling the pipe clock you really want the PHY PLL as source and not the cached XO. Can you still drop the current safe-mux patches from your tree or would they need to be fixed up incrementally? I think you merged v2, but there's already a v3 out (addressing the hardcoded mux configuration values). Johan -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy