From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6A3241E835A; Thu, 27 Feb 2025 06:00:30 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740636030; cv=none; b=QmNYoBHENIW2L2J0uRHVShbMT9xBHhp4xZYXwQk421ReWZymeYTQCHuZsL+xr93Udy4BDG7vGskZSqlBHvLpkzPsmsav59WJe0J6RBdyYLruh4iCqHHhfQOv67DBzCL3lsnFYGVxItf9xXzvSEK7uF+aKTKWqtdsQ0WPgvBH75M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740636030; c=relaxed/simple; bh=tRUaBjfQeJhK+q/8PKXwiUJCfypbozRNjwBfu7BchpY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=jkFUQUkbg16wM/Ob//Xc6FNqaNkc7sW1ZUlrciICNxAotV8/gE4ZbKcX5XQ+cm76BEd7Kbgha1j3Zh+rqMGqkNA6I8Q7Uvnbzv7BKmjORET56dRu/kW4r38tUw6OoP0rdna2fJHx/WfpNoCVdyqQs1rPCwI1o1THSwxI5pcIAZo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=WMJrpTBB; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="WMJrpTBB" Received: by smtp.kernel.org (Postfix) with ESMTPSA id D976AC4CEDD; Thu, 27 Feb 2025 06:00:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1740636029; bh=tRUaBjfQeJhK+q/8PKXwiUJCfypbozRNjwBfu7BchpY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=WMJrpTBBQURZ3HaqKnlBxryXqMVZR/xjlhDc1PBJR9hEKzncjvOQA7FY9HHaQzP/d phWfOj+N+1sK9JpXRQIkslQuZNXBzN0LSMORIWpyPRdynEIv+aUM7jtFFm4m76+l1x +UAe0EtqgNsBm30m9XzXP1SsfG+71/dcvKSgqsrQdPoXMGMOtVGpjZCx85hl58mJtH vAW2MbRkUvwzW9O1qM0vt12s2vPBVkJnEHpa3OKFjI2BCv+SFzpXppoVUn3vjZ81+3 rxjMS9svehUMC6bJlgJiFU4dwuFr7wgObypr86lbU78CUl5f+clfGUr8LVC2r8fd9u +Ge8iPjFZAZSg== Date: Thu, 27 Feb 2025 11:30:24 +0530 From: Vinod Koul To: Varadarajan Narayanan Cc: lpieralisi@kernel.org, kw@linux.com, manivannan.sadhasivam@linaro.org, robh@kernel.org, bhelgaas@google.com, krzk+dt@kernel.org, conor+dt@kernel.org, kishon@kernel.org, andersson@kernel.org, konradybcio@kernel.org, p.zabel@pengutronix.de, quic_nsekar@quicinc.com, dmitry.baryshkov@linaro.org, linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-phy@lists.infradead.org Subject: Re: [PATCH v10 2/7] phy: qcom: Introduce PCIe UNIPHY 28LP driver Message-ID: References: <20250206121803.1128216-1-quic_varada@quicinc.com> <20250206121803.1128216-3-quic_varada@quicinc.com> Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On 17-02-25, 15:03, Varadarajan Narayanan wrote: > On Thu, Feb 13, 2025 at 11:22:01PM +0530, Vinod Koul wrote: > [ . . .] > > > > +static const struct qcom_uniphy_pcie_data ipq5332_data = { > > > + .lane_offset = 0x800, > > > + .phy_type = PHY_TYPE_PCIE_GEN3, > > > + .init_seq = ipq5332_regs, > > > + .init_seq_num = ARRAY_SIZE(ipq5332_regs), > > > + .pipe_clk_rate = 250000000, > > > > can be written as 250 * MEGA > > Ok. > > [ . . .] > > > > +/* > > > + * Register a fixed rate pipe clock. > > > + * > > > + * The _pipe_clksrc generated by PHY goes to the GCC that gate > > > + * controls it. The _pipe_clk coming out of the GCC is requested > > > + * by the PHY driver for its operations. > > > + * We register the _pipe_clksrc here. The gcc driver takes care > > > + * of assigning this _pipe_clksrc as parent to _pipe_clk. > > > + * Below picture shows this relationship. > > > + * > > > + * +---------------+ > > > + * | PHY block |<<---------------------------------------+ > > > + * | | | > > > + * | +-------+ | +-----+ | > > > + * I/P---^-->| PLL |---^--->pipe_clksrc--->| GCC |--->pipe_clk---+ > > > + * clk | +-------+ | +-----+ > > > + * +---------------+ > > > + */ > > > +static inline int phy_pipe_clk_register(struct qcom_uniphy_pcie *phy, int id) > > > +{ > > > + const struct qcom_uniphy_pcie_data *data = phy->data; > > > + struct clk_hw *hw; > > > + char name[64]; > > > + > > > + snprintf(name, sizeof(name), "phy%d_pipe_clk_src", id); > > > + hw = devm_clk_hw_register_fixed_rate(phy->dev, name, NULL, 0, > > > + data->pipe_clk_rate); > > > + if (IS_ERR(hw)) > > > + return dev_err_probe(phy->dev, PTR_ERR(hw), > > > + "Unable to register %s\n", name); > > > + > > > + return devm_of_clk_add_hw_provider(phy->dev, of_clk_hw_simple_get, hw); > > > +} > > > + > > > +static const struct of_device_id qcom_uniphy_pcie_id_table[] = { > > > + { > > > + .compatible = "qcom,ipq5332-uniphy-pcie-phy", > > > + .data = &ipq5332_data, > > > + }, { > > > + /* Sentinel */ > > > + }, > > > +}; > > > +MODULE_DEVICE_TABLE(of, qcom_uniphy_pcie_id_table); > > > + > > > +static const struct phy_ops pcie_ops = { > > > + .power_on = qcom_uniphy_pcie_power_on, > > > + .power_off = qcom_uniphy_pcie_power_off, > > > + .owner = THIS_MODULE, > > > +}; > > > + > > > +static int qcom_uniphy_pcie_probe(struct platform_device *pdev) > > > +{ > > > + struct phy_provider *phy_provider; > > > + struct device *dev = &pdev->dev; > > > + struct qcom_uniphy_pcie *phy; > > > + struct phy *generic_phy; > > > + int ret; > > > + > > > + phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL); > > > + if (!phy) > > > + return -ENOMEM; > > > + > > > + platform_set_drvdata(pdev, phy); > > > + phy->dev = &pdev->dev; > > > + > > > + phy->data = of_device_get_match_data(dev); > > > + if (!phy->data) > > > + return -EINVAL; > > > + > > > + ret = of_property_read_u32(dev_of_node(dev), "num-lanes", &phy->lanes); > > > + if (ret) > > > + return dev_err_probe(dev, ret, "Couldn't read num-lanes\n"); > > > + > > > + ret = qcom_uniphy_pcie_get_resources(pdev, phy); > > > + if (ret < 0) > > > + return dev_err_probe(&pdev->dev, ret, > > > + "failed to get resources: %d\n", ret); > > > + > > > + generic_phy = devm_phy_create(phy->dev, NULL, &pcie_ops); > > > + if (IS_ERR(generic_phy)) > > > + return PTR_ERR(generic_phy); > > > + > > > + phy_set_drvdata(generic_phy, phy); > > > + > > > + ret = phy_pipe_clk_register(phy, generic_phy->id); > > > + if (ret) > > > + dev_err(&pdev->dev, "failed to register phy pipe clk\n"); > > > + > > > + phy_provider = devm_of_phy_provider_register(phy->dev, > > > + of_phy_simple_xlate); > > > + if (IS_ERR(phy_provider)) > > > + return PTR_ERR(phy_provider); > > > > should we not unroll the pipe clk registration here? > > Since it is a 'devm_' clk_hw_register_fixed_rate, wouldn't the devm > framework do the unregister? > > $ git diff > diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c > index 6b4f76b9c4da..3fd1a12cc163 100644 > --- a/drivers/clk/clk-fixed-rate.c > +++ b/drivers/clk/clk-fixed-rate.c > @@ -58,6 +58,7 @@ static void > devm_clk_hw_register_fixed_rate_release(struct device *dev, void *re > * the hw, resulting in double free. Just unregister the hw and > * let > * devres code kfree() it. > */ > + printk("--> %s: %s\n", __func__, __clk_get_name(fix->hw.clk)); > clk_hw_unregister(&fix->hw); > } > > diff --git a/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c > b/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c > index 311f98181177..9a8d8d9a7c2b 100644 > --- a/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c > +++ b/drivers/phy/qualcomm/phy-qcom-uniphy-pcie-28lp.c > @@ -267,6 +268,7 @@ static int qcom_uniphy_pcie_probe(struct > platform_device *pdev) > > phy_provider = devm_of_phy_provider_register(phy->dev, > of_phy_simple_xlate); > + phy_provider = ERR_PTR(-EINVAL); > if (IS_ERR(phy_provider)) > return PTR_ERR(phy_provider); > > I forced an error here and saw that devm_clk_hw_register_fixed_rate_release > is getting called, which in turn calls clk_hw_unregister. Is that sufficient? > Or am i missing something. I missed that internally this is devm_, this is fine -- ~Vinod