| Message ID | 20240319-topic-sm8x50-upstream-pcie-1-phy-aux-clk-v1-2-926d7a4ccd80@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: qcom-sm8[456]50: properly describe the PCIe Gen4x2 PHY AUX clock | expand |
On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock, > in order to expose it, split the current clock registering in two parts: > - CCF clock registering > - DT clock registering > > Also switch to devm_of_clk_add_hw_provider(). > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 27 +++++++++++---------------- > 1 file changed, 11 insertions(+), 16 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > index 8836bb1ff0cc..079b3e306489 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > @@ -3635,11 +3635,6 @@ static int qmp_pcie_clk_init(struct qmp_pcie *qmp) > return devm_clk_bulk_get_optional(dev, num, qmp->clks); > } > > -static void phy_clk_release_provider(void *res) > -{ > - of_clk_del_provider(res); > -} > - > /* > * Register a fixed rate pipe clock. > * > @@ -3664,7 +3659,7 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np) > struct clk_init_data init = { }; > int ret; > > - ret = of_property_read_string(np, "clock-output-names", &init.name); > + ret = of_property_read_string_index(np, "clock-output-names", 0, &init.name); > if (ret) { > dev_err(qmp->dev, "%pOFn: No clock-output-names\n", np); > return ret; > @@ -3683,19 +3678,19 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np) > > fixed->hw.init = &init; > > - ret = devm_clk_hw_register(qmp->dev, &fixed->hw); > - if (ret) > - return ret; > + return devm_clk_hw_register(qmp->dev, &fixed->hw); > +} > > - ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw); > +static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np) > +{ > + int ret; > + > + ret = phy_pipe_clk_register(qmp, np); > if (ret) > return ret; > > - /* > - * Roll a devm action because the clock provider is the child node, but > - * the child node is not actually a device. > - */ > - return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, np); > + return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get, > + &qmp->pipe_clk_fixed.hw); No. The driver has to register a clock provider at the np rather than at dev->of_node. Otherwise legacy DT will be broken. > } > > static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np) > @@ -3899,7 +3894,7 @@ static int qmp_pcie_probe(struct platform_device *pdev) > if (ret) > goto err_node_put; > > - ret = phy_pipe_clk_register(qmp, np); > + ret = qmp_pcie_register_clocks(qmp, np); > if (ret) > goto err_node_put; > > > -- > 2.34.1 > >
On 19/03/2024 11:50, Dmitry Baryshkov wrote: > On Tue, 19 Mar 2024 at 12:45, Neil Armstrong <neil.armstrong@linaro.org> wrote: >> >> The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock, >> in order to expose it, split the current clock registering in two parts: >> - CCF clock registering >> - DT clock registering >> >> Also switch to devm_of_clk_add_hw_provider(). >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 27 +++++++++++---------------- >> 1 file changed, 11 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >> index 8836bb1ff0cc..079b3e306489 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c >> @@ -3635,11 +3635,6 @@ static int qmp_pcie_clk_init(struct qmp_pcie *qmp) >> return devm_clk_bulk_get_optional(dev, num, qmp->clks); >> } >> >> -static void phy_clk_release_provider(void *res) >> -{ >> - of_clk_del_provider(res); >> -} >> - >> /* >> * Register a fixed rate pipe clock. >> * >> @@ -3664,7 +3659,7 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np) >> struct clk_init_data init = { }; >> int ret; >> >> - ret = of_property_read_string(np, "clock-output-names", &init.name); >> + ret = of_property_read_string_index(np, "clock-output-names", 0, &init.name); >> if (ret) { >> dev_err(qmp->dev, "%pOFn: No clock-output-names\n", np); >> return ret; >> @@ -3683,19 +3678,19 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np) >> >> fixed->hw.init = &init; >> >> - ret = devm_clk_hw_register(qmp->dev, &fixed->hw); >> - if (ret) >> - return ret; >> + return devm_clk_hw_register(qmp->dev, &fixed->hw); >> +} >> >> - ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw); >> +static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np) >> +{ >> + int ret; >> + >> + ret = phy_pipe_clk_register(qmp, np); >> if (ret) >> return ret; >> >> - /* >> - * Roll a devm action because the clock provider is the child node, but >> - * the child node is not actually a device. >> - */ >> - return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, np); >> + return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get, >> + &qmp->pipe_clk_fixed.hw); > > No. The driver has to register a clock provider at the np rather than > at dev->of_node. Otherwise legacy DT will be broken. Indeed Thx Neil > >> } >> >> static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np) >> @@ -3899,7 +3894,7 @@ static int qmp_pcie_probe(struct platform_device *pdev) >> if (ret) >> goto err_node_put; >> >> - ret = phy_pipe_clk_register(qmp, np); >> + ret = qmp_pcie_register_clocks(qmp, np); >> if (ret) >> goto err_node_put; >> >> >> -- >> 2.34.1 >> >> > >
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c index 8836bb1ff0cc..079b3e306489 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c @@ -3635,11 +3635,6 @@ static int qmp_pcie_clk_init(struct qmp_pcie *qmp) return devm_clk_bulk_get_optional(dev, num, qmp->clks); } -static void phy_clk_release_provider(void *res) -{ - of_clk_del_provider(res); -} - /* * Register a fixed rate pipe clock. * @@ -3664,7 +3659,7 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np) struct clk_init_data init = { }; int ret; - ret = of_property_read_string(np, "clock-output-names", &init.name); + ret = of_property_read_string_index(np, "clock-output-names", 0, &init.name); if (ret) { dev_err(qmp->dev, "%pOFn: No clock-output-names\n", np); return ret; @@ -3683,19 +3678,19 @@ static int phy_pipe_clk_register(struct qmp_pcie *qmp, struct device_node *np) fixed->hw.init = &init; - ret = devm_clk_hw_register(qmp->dev, &fixed->hw); - if (ret) - return ret; + return devm_clk_hw_register(qmp->dev, &fixed->hw); +} - ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw); +static int qmp_pcie_register_clocks(struct qmp_pcie *qmp, struct device_node *np) +{ + int ret; + + ret = phy_pipe_clk_register(qmp, np); if (ret) return ret; - /* - * Roll a devm action because the clock provider is the child node, but - * the child node is not actually a device. - */ - return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, np); + return devm_of_clk_add_hw_provider(qmp->dev, of_clk_hw_simple_get, + &qmp->pipe_clk_fixed.hw); } static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np) @@ -3899,7 +3894,7 @@ static int qmp_pcie_probe(struct platform_device *pdev) if (ret) goto err_node_put; - ret = phy_pipe_clk_register(qmp, np); + ret = qmp_pcie_register_clocks(qmp, np); if (ret) goto err_node_put;
The PCIe Gen4x2 PHY found in the SM8[456]50 SoCs have a second clock, in order to expose it, split the current clock registering in two parts: - CCF clock registering - DT clock registering Also switch to devm_of_clk_add_hw_provider(). Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)