Re: [PATCH v10 2/7] phy: qcom: Introduce PCIe UNIPHY 28LP driver

[Vinod Koul <vkoul@kernel.org>]

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 <s>_pipe_clksrc generated by PHY goes to the GCC that gate
> > > + * controls it. The <s>_pipe_clk coming out of the GCC is requested
> > > + * by the PHY driver for its operations.
> > > + * We register the <s>_pipe_clksrc here. The gcc driver takes care
> > > + * of assigning this <s>_pipe_clksrc as parent to <s>_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