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
