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.
Thanks
Varada
> > +
> > + return 0;
> > +}
[ . . . ]
