Add USB/DP switchable PHY clock registration and DT parsing for DP offsets.
Extend qmp_usbc_register_clocks and clock provider logic to support both
USB and DP instances.
Signed-off-by: Xiangxu Yin <xiangxu.yin@oss.qualcomm.com>
---
drivers/phy/qualcomm/phy-qcom-qmp-usbc.c | 208 +++++++++++++++++++++++++++++--
1 file changed, 195 insertions(+), 13 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usbc.c b/drivers/phy/qualcomm/phy-qcom-qmp-usbc.c
index 7b2b47320cbb2d16e4f316b0f52fdc1bd09fe656..95a099de908e7f3478eb1e18326b21d4014d8da6 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-usbc.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-usbc.c
@@ -22,6 +22,7 @@
#include <linux/slab.h>
#include <linux/usb/typec.h>
#include <linux/usb/typec_mux.h>
+#include <dt-bindings/phy/phy-qcom-qmp.h>
#include "phy-qcom-qmp-common.h"
@@ -1245,9 +1246,23 @@ static int qmp_usbc_clk_init(struct qmp_usbc *qmp)
return devm_clk_bulk_get_optional(dev, num, qmp->clks);
}
-static void phy_clk_release_provider(void *res)
+static struct clk_hw *qmp_usbc_clks_hw_get(struct of_phandle_args *clkspec, void *data)
{
- of_clk_del_provider(res);
+ struct qmp_usbc *qmp = data;
+
+ if (clkspec->args_count == 0)
+ return &qmp->pipe_clk_fixed.hw;
+
+ switch (clkspec->args[0]) {
+ case QMP_USB43DP_USB3_PIPE_CLK:
+ return &qmp->pipe_clk_fixed.hw;
+ case QMP_USB43DP_DP_LINK_CLK:
+ return &qmp->dp_link_hw;
+ case QMP_USB43DP_DP_VCO_DIV_CLK:
+ return &qmp->dp_pixel_hw;
+ }
+
+ return ERR_PTR(-EINVAL);
}
/*
@@ -1276,8 +1291,11 @@ static int phy_pipe_clk_register(struct qmp_usbc *qmp, struct device_node *np)
ret = of_property_read_string(np, "clock-output-names", &init.name);
if (ret) {
- dev_err(qmp->dev, "%pOFn: No clock-output-names\n", np);
- return ret;
+ char name[64];
+
+ /* Clock name is not mandatory. */
+ snprintf(name, sizeof(name), "%s::pipe_clk", dev_name(qmp->dev));
+ init.name = name;
}
init.ops = &clk_fixed_rate_ops;
@@ -1286,19 +1304,176 @@ static int phy_pipe_clk_register(struct qmp_usbc *qmp, struct device_node *np)
fixed->fixed_rate = 125000000;
fixed->hw.init = &init;
- ret = devm_clk_hw_register(qmp->dev, &fixed->hw);
- if (ret)
+ return devm_clk_hw_register(qmp->dev, &fixed->hw);
+}
+
+
+/*
+ * Display Port PLL driver block diagram for branch clocks
+ *
+ * +------------------------------+
+ * | DP_VCO_CLK |
+ * | |
+ * | +-------------------+ |
+ * | | (DP PLL/VCO) | |
+ * | +---------+---------+ |
+ * | v |
+ * | +----------+-----------+ |
+ * | | hsclk_divsel_clk_src | |
+ * | +----------+-----------+ |
+ * +------------------------------+
+ * |
+ * +---------<---------v------------>----------+
+ * | |
+ * +--------v----------------+ |
+ * | dp_phy_pll_link_clk | |
+ * | link_clk | |
+ * +--------+----------------+ |
+ * | |
+ * | |
+ * v v
+ * Input to DISPCC block |
+ * for link clk, crypto clk |
+ * and interface clock |
+ * |
+ * |
+ * +--------<------------+-----------------+---<---+
+ * | | |
+ * +----v---------+ +--------v-----+ +--------v------+
+ * | vco_divided | | vco_divided | | vco_divided |
+ * | _clk_src | | _clk_src | | _clk_src |
+ * | | | | | |
+ * |divsel_six | | divsel_two | | divsel_four |
+ * +-------+------+ +-----+--------+ +--------+------+
+ * | | |
+ * v---->----------v-------------<------v
+ * |
+ * +----------+-----------------+
+ * | dp_phy_pll_vco_div_clk |
+ * +---------+------------------+
+ * |
+ * v
+ * Input to DISPCC block
+ * for DP pixel clock
+ *
+ */
+static int qmp_dp_pixel_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
+{
+ switch (req->rate) {
+ case 1620000000UL / 2:
+ case 2700000000UL / 2:
+ /* 5.4 and 8.1 GHz are same link rate as 2.7GHz, i.e. div 4 and div 6 */
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+static unsigned long qmp_dp_pixel_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+ const struct qmp_usbc *qmp;
+ const struct phy_configure_opts_dp *dp_opts;
+
+ qmp = container_of(hw, struct qmp_usbc, dp_pixel_hw);
+
+ dp_opts = &qmp->dp_opts;
+
+ switch (dp_opts->link_rate) {
+ case 1620:
+ return 1620000000UL / 2;
+ case 2700:
+ return 2700000000UL / 2;
+ case 5400:
+ return 5400000000UL / 4;
+ default:
+ return 0;
+ }
+}
+
+static const struct clk_ops qmp_dp_pixel_clk_ops = {
+ .determine_rate = qmp_dp_pixel_clk_determine_rate,
+ .recalc_rate = qmp_dp_pixel_clk_recalc_rate,
+};
+
+static int qmp_dp_link_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
+{
+ switch (req->rate) {
+ case 162000000:
+ case 270000000:
+ case 540000000:
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+static unsigned long qmp_dp_link_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
+{
+ const struct qmp_usbc *qmp;
+ const struct phy_configure_opts_dp *dp_opts;
+
+ qmp = container_of(hw, struct qmp_usbc, dp_link_hw);
+ dp_opts = &qmp->dp_opts;
+
+ switch (dp_opts->link_rate) {
+ case 1620:
+ case 2700:
+ case 5400:
+ return dp_opts->link_rate * 100000;
+ default:
+ return 0;
+ }
+}
+
+static const struct clk_ops qmp_dp_link_clk_ops = {
+ .determine_rate = qmp_dp_link_clk_determine_rate,
+ .recalc_rate = qmp_dp_link_clk_recalc_rate,
+};
+
+static int phy_dp_clks_register(struct qmp_usbc *qmp, struct device_node *np)
+{
+ struct clk_init_data init = { };
+ char name[64];
+ int ret;
+
+ snprintf(name, sizeof(name), "%s::link_clk", dev_name(qmp->dev));
+ init.ops = &qmp_dp_link_clk_ops;
+ init.name = name;
+ qmp->dp_link_hw.init = &init;
+ ret = devm_clk_hw_register(qmp->dev, &qmp->dp_link_hw);
+ if (ret < 0) {
+ dev_err(qmp->dev, "link clk reg fail ret=%d\n", ret);
+ return ret;
+ }
+
+ snprintf(name, sizeof(name), "%s::vco_div_clk", dev_name(qmp->dev));
+ init.ops = &qmp_dp_pixel_clk_ops;
+ init.name = name;
+ qmp->dp_pixel_hw.init = &init;
+ ret = devm_clk_hw_register(qmp->dev, &qmp->dp_pixel_hw);
+ if (ret) {
+ dev_err(qmp->dev, "pxl clk reg fail ret=%d\n", ret);
return ret;
+ }
+
+ return 0;
+}
+
+static int qmp_usbc_register_clocks(struct qmp_usbc *qmp, struct device_node *np)
+{
+ int ret;
- ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
+ 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);
+ if (qmp->dp_serdes != 0) {
+ ret = phy_dp_clks_register(qmp, np);
+ if (ret)
+ return ret;
+ }
+
+ return devm_of_clk_add_hw_provider(qmp->dev, qmp_usbc_clks_hw_get, qmp);
}
#if IS_ENABLED(CONFIG_TYPEC)
@@ -1429,6 +1604,13 @@ static int qmp_usbc_parse_dt(struct qmp_usbc *qmp)
if (IS_ERR(base))
return PTR_ERR(base);
+ if (offs->dp_serdes != 0) {
+ qmp->dp_serdes = base + offs->dp_serdes;
+ qmp->dp_tx = base + offs->dp_txa;
+ qmp->dp_tx2 = base + offs->dp_txb;
+ qmp->dp_dp_phy = base + offs->dp_dp_phy;
+ }
+
qmp->serdes = base + offs->serdes;
qmp->pcs = base + offs->pcs;
if (offs->pcs_misc)
@@ -1537,7 +1719,7 @@ static int qmp_usbc_probe(struct platform_device *pdev)
*/
pm_runtime_forbid(dev);
- ret = phy_pipe_clk_register(qmp, np);
+ ret = qmp_usbc_register_clocks(qmp, np);
if (ret)
goto err_node_put;
--
2.34.1On Thu, Sep 11, 2025 at 10:55:05PM +0800, Xiangxu Yin wrote:
> Add USB/DP switchable PHY clock registration and DT parsing for DP offsets.
> Extend qmp_usbc_register_clocks and clock provider logic to support both
> USB and DP instances.
>
> Signed-off-by: Xiangxu Yin <xiangxu.yin@oss.qualcomm.com>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-usbc.c | 208 +++++++++++++++++++++++++++++--
> 1 file changed, 195 insertions(+), 13 deletions(-)
>
> @@ -1276,8 +1291,11 @@ static int phy_pipe_clk_register(struct qmp_usbc *qmp, struct device_node *np)
>
> ret = of_property_read_string(np, "clock-output-names", &init.name);
> if (ret) {
> - dev_err(qmp->dev, "%pOFn: No clock-output-names\n", np);
> - return ret;
> + char name[64];
> +
> + /* Clock name is not mandatory. */
> + snprintf(name, sizeof(name), "%s::pipe_clk", dev_name(qmp->dev));
> + init.name = name;
> }
Do we have any guarantees that memory for 'name' exists beyond this point?
>
> init.ops = &clk_fixed_rate_ops;
> @@ -1286,19 +1304,176 @@ static int phy_pipe_clk_register(struct qmp_usbc *qmp, struct device_node *np)
> fixed->fixed_rate = 125000000;
> fixed->hw.init = &init;
>
> - ret = devm_clk_hw_register(qmp->dev, &fixed->hw);
> - if (ret)
> + return devm_clk_hw_register(qmp->dev, &fixed->hw);
> +}
> +
> +
> +/*
> + * Display Port PLL driver block diagram for branch clocks
> + *
> + * +------------------------------+
> + * | DP_VCO_CLK |
> + * | |
> + * | +-------------------+ |
> + * | | (DP PLL/VCO) | |
> + * | +---------+---------+ |
> + * | v |
> + * | +----------+-----------+ |
> + * | | hsclk_divsel_clk_src | |
> + * | +----------+-----------+ |
> + * +------------------------------+
> + * |
> + * +---------<---------v------------>----------+
> + * | |
> + * +--------v----------------+ |
> + * | dp_phy_pll_link_clk | |
> + * | link_clk | |
> + * +--------+----------------+ |
> + * | |
> + * | |
> + * v v
> + * Input to DISPCC block |
> + * for link clk, crypto clk |
> + * and interface clock |
> + * |
> + * |
> + * +--------<------------+-----------------+---<---+
> + * | | |
> + * +----v---------+ +--------v-----+ +--------v------+
> + * | vco_divided | | vco_divided | | vco_divided |
> + * | _clk_src | | _clk_src | | _clk_src |
> + * | | | | | |
> + * |divsel_six | | divsel_two | | divsel_four |
> + * +-------+------+ +-----+--------+ +--------+------+
> + * | | |
> + * v---->----------v-------------<------v
> + * |
> + * +----------+-----------------+
> + * | dp_phy_pll_vco_div_clk |
> + * +---------+------------------+
> + * |
> + * v
> + * Input to DISPCC block
> + * for DP pixel clock
> + *
> + */
> +static int qmp_dp_pixel_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> +{
> + switch (req->rate) {
> + case 1620000000UL / 2:
> + case 2700000000UL / 2:
> + /* 5.4 and 8.1 GHz are same link rate as 2.7GHz, i.e. div 4 and div 6 */
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static unsigned long qmp_dp_pixel_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> +{
> + const struct qmp_usbc *qmp;
> + const struct phy_configure_opts_dp *dp_opts;
> +
> + qmp = container_of(hw, struct qmp_usbc, dp_pixel_hw);
> +
> + dp_opts = &qmp->dp_opts;
> +
> + switch (dp_opts->link_rate) {
> + case 1620:
> + return 1620000000UL / 2;
> + case 2700:
> + return 2700000000UL / 2;
> + case 5400:
> + return 5400000000UL / 4;
No HBR3 support? Then why was it mentioned few lines above?
> + default:
> + return 0;
> + }
> +}
> +
> +static int qmp_usbc_register_clocks(struct qmp_usbc *qmp, struct device_node *np)
> +{
> + int ret;
>
> - ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
> + 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);
> + if (qmp->dp_serdes != 0) {
> + ret = phy_dp_clks_register(qmp, np);
> + if (ret)
> + return ret;
> + }
> +
> + return devm_of_clk_add_hw_provider(qmp->dev, qmp_usbc_clks_hw_get, qmp);
Do you understand what did the comment (that you've removed) say? And
why?
> }
>
> #if IS_ENABLED(CONFIG_TYPEC)
> @@ -1429,6 +1604,13 @@ static int qmp_usbc_parse_dt(struct qmp_usbc *qmp)
> if (IS_ERR(base))
> return PTR_ERR(base);
>
> + if (offs->dp_serdes != 0) {
> + qmp->dp_serdes = base + offs->dp_serdes;
> + qmp->dp_tx = base + offs->dp_txa;
> + qmp->dp_tx2 = base + offs->dp_txb;
> + qmp->dp_dp_phy = base + offs->dp_dp_phy;
> + }
> +
> qmp->serdes = base + offs->serdes;
> qmp->pcs = base + offs->pcs;
> if (offs->pcs_misc)
> @@ -1537,7 +1719,7 @@ static int qmp_usbc_probe(struct platform_device *pdev)
> */
> pm_runtime_forbid(dev);
>
> - ret = phy_pipe_clk_register(qmp, np);
> + ret = qmp_usbc_register_clocks(qmp, np);
> if (ret)
> goto err_node_put;
>
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
On 9/12/2025 6:19 PM, Dmitry Baryshkov wrote:
> On Thu, Sep 11, 2025 at 10:55:05PM +0800, Xiangxu Yin wrote:
>> Add USB/DP switchable PHY clock registration and DT parsing for DP offsets.
>> Extend qmp_usbc_register_clocks and clock provider logic to support both
>> USB and DP instances.
>>
>> Signed-off-by: Xiangxu Yin <xiangxu.yin@oss.qualcomm.com>
>> ---
>> drivers/phy/qualcomm/phy-qcom-qmp-usbc.c | 208 +++++++++++++++++++++++++++++--
>> 1 file changed, 195 insertions(+), 13 deletions(-)
>>
>> @@ -1276,8 +1291,11 @@ static int phy_pipe_clk_register(struct qmp_usbc *qmp, struct device_node *np)
>>
>> ret = of_property_read_string(np, "clock-output-names", &init.name);
>> if (ret) {
>> - dev_err(qmp->dev, "%pOFn: No clock-output-names\n", np);
>> - return ret;
>> + char name[64];
>> +
>> + /* Clock name is not mandatory. */
>> + snprintf(name, sizeof(name), "%s::pipe_clk", dev_name(qmp->dev));
>> + init.name = name;
>> }
> Do we have any guarantees that memory for 'name' exists beyond this point?
If the previous of_property_read_string() call succeeded, could 'name'
still be empty? or you means 'char name[64]' will be release after '}'?
From local verification, I can see 88e8000.phy::pipe_clk node from clk_summary.
>>
>> init.ops = &clk_fixed_rate_ops;
>> @@ -1286,19 +1304,176 @@ static int phy_pipe_clk_register(struct qmp_usbc *qmp, struct device_node *np)
>> fixed->fixed_rate = 125000000;
>> fixed->hw.init = &init;
>>
>> - ret = devm_clk_hw_register(qmp->dev, &fixed->hw);
>> - if (ret)
>> + return devm_clk_hw_register(qmp->dev, &fixed->hw);
>> +}
>> +
>> +
>> +/*
>> + * Display Port PLL driver block diagram for branch clocks
>> + *
>> + * +------------------------------+
>> + * | DP_VCO_CLK |
>> + * | |
>> + * | +-------------------+ |
>> + * | | (DP PLL/VCO) | |
>> + * | +---------+---------+ |
>> + * | v |
>> + * | +----------+-----------+ |
>> + * | | hsclk_divsel_clk_src | |
>> + * | +----------+-----------+ |
>> + * +------------------------------+
>> + * |
>> + * +---------<---------v------------>----------+
>> + * | |
>> + * +--------v----------------+ |
>> + * | dp_phy_pll_link_clk | |
>> + * | link_clk | |
>> + * +--------+----------------+ |
>> + * | |
>> + * | |
>> + * v v
>> + * Input to DISPCC block |
>> + * for link clk, crypto clk |
>> + * and interface clock |
>> + * |
>> + * |
>> + * +--------<------------+-----------------+---<---+
>> + * | | |
>> + * +----v---------+ +--------v-----+ +--------v------+
>> + * | vco_divided | | vco_divided | | vco_divided |
>> + * | _clk_src | | _clk_src | | _clk_src |
>> + * | | | | | |
>> + * |divsel_six | | divsel_two | | divsel_four |
>> + * +-------+------+ +-----+--------+ +--------+------+
>> + * | | |
>> + * v---->----------v-------------<------v
>> + * |
>> + * +----------+-----------------+
>> + * | dp_phy_pll_vco_div_clk |
>> + * +---------+------------------+
>> + * |
>> + * v
>> + * Input to DISPCC block
>> + * for DP pixel clock
>> + *
>> + */
>> +static int qmp_dp_pixel_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
>> +{
>> + switch (req->rate) {
>> + case 1620000000UL / 2:
>> + case 2700000000UL / 2:
>> + /* 5.4 and 8.1 GHz are same link rate as 2.7GHz, i.e. div 4 and div 6 */
>> + return 0;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static unsigned long qmp_dp_pixel_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
>> +{
>> + const struct qmp_usbc *qmp;
>> + const struct phy_configure_opts_dp *dp_opts;
>> +
>> + qmp = container_of(hw, struct qmp_usbc, dp_pixel_hw);
>> +
>> + dp_opts = &qmp->dp_opts;
>> +
>> + switch (dp_opts->link_rate) {
>> + case 1620:
>> + return 1620000000UL / 2;
>> + case 2700:
>> + return 2700000000UL / 2;
>> + case 5400:
>> + return 5400000000UL / 4;
> No HBR3 support? Then why was it mentioned few lines above?
>
>> + default:
>> + return 0;
>> + }
>> +}
>> +
>
>> +static int qmp_usbc_register_clocks(struct qmp_usbc *qmp, struct device_node *np)
>> +{
>> + int ret;
>>
>> - ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
>> + 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);
>> + if (qmp->dp_serdes != 0) {
>> + ret = phy_dp_clks_register(qmp, np);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + return devm_of_clk_add_hw_provider(qmp->dev, qmp_usbc_clks_hw_get, qmp);
> Do you understand what did the comment (that you've removed) say? And
> why?
>
>> }
>>
>> #if IS_ENABLED(CONFIG_TYPEC)
>> @@ -1429,6 +1604,13 @@ static int qmp_usbc_parse_dt(struct qmp_usbc *qmp)
>> if (IS_ERR(base))
>> return PTR_ERR(base);
>>
>> + if (offs->dp_serdes != 0) {
>> + qmp->dp_serdes = base + offs->dp_serdes;
>> + qmp->dp_tx = base + offs->dp_txa;
>> + qmp->dp_tx2 = base + offs->dp_txb;
>> + qmp->dp_dp_phy = base + offs->dp_dp_phy;
>> + }
>> +
>> qmp->serdes = base + offs->serdes;
>> qmp->pcs = base + offs->pcs;
>> if (offs->pcs_misc)
>> @@ -1537,7 +1719,7 @@ static int qmp_usbc_probe(struct platform_device *pdev)
>> */
>> pm_runtime_forbid(dev);
>>
>> - ret = phy_pipe_clk_register(qmp, np);
>> + ret = qmp_usbc_register_clocks(qmp, np);
>> if (ret)
>> goto err_node_put;
>>
>>
>> --
>> 2.34.1
>>
On Fri, Sep 12, 2025 at 08:00:14PM +0800, Xiangxu Yin wrote:
>
> On 9/12/2025 6:19 PM, Dmitry Baryshkov wrote:
> > On Thu, Sep 11, 2025 at 10:55:05PM +0800, Xiangxu Yin wrote:
> >> Add USB/DP switchable PHY clock registration and DT parsing for DP offsets.
> >> Extend qmp_usbc_register_clocks and clock provider logic to support both
> >> USB and DP instances.
> >>
> >> Signed-off-by: Xiangxu Yin <xiangxu.yin@oss.qualcomm.com>
> >> ---
> >> drivers/phy/qualcomm/phy-qcom-qmp-usbc.c | 208 +++++++++++++++++++++++++++++--
> >> 1 file changed, 195 insertions(+), 13 deletions(-)
> >>
> >> @@ -1276,8 +1291,11 @@ static int phy_pipe_clk_register(struct qmp_usbc *qmp, struct device_node *np)
> >>
> >> ret = of_property_read_string(np, "clock-output-names", &init.name);
> >> if (ret) {
> >> - dev_err(qmp->dev, "%pOFn: No clock-output-names\n", np);
> >> - return ret;
> >> + char name[64];
> >> +
> >> + /* Clock name is not mandatory. */
> >> + snprintf(name, sizeof(name), "%s::pipe_clk", dev_name(qmp->dev));
> >> + init.name = name;
> >> }
> > Do we have any guarantees that memory for 'name' exists beyond this point?
>
>
> If the previous of_property_read_string() call succeeded, could 'name'
> still be empty? or you means 'char name[64]' will be release after '}'?
>
> From local verification, I can see 88e8000.phy::pipe_clk node from clk_summary.
char name[64] belong to a stack frame that is not guaranteed to exist
after you've close this bracked. So it can be easily overwritten with
other values.
>
>
> >>
> >> init.ops = &clk_fixed_rate_ops;
> >> @@ -1286,19 +1304,176 @@ static int phy_pipe_clk_register(struct qmp_usbc *qmp, struct device_node *np)
> >> fixed->fixed_rate = 125000000;
> >> fixed->hw.init = &init;
> >>
> >> - ret = devm_clk_hw_register(qmp->dev, &fixed->hw);
> >> - if (ret)
> >> + return devm_clk_hw_register(qmp->dev, &fixed->hw);
> >> +}
> >> +
> >> +
> >> +/*
> >> + * Display Port PLL driver block diagram for branch clocks
> >> + *
> >> + * +------------------------------+
> >> + * | DP_VCO_CLK |
> >> + * | |
> >> + * | +-------------------+ |
> >> + * | | (DP PLL/VCO) | |
> >> + * | +---------+---------+ |
> >> + * | v |
> >> + * | +----------+-----------+ |
> >> + * | | hsclk_divsel_clk_src | |
> >> + * | +----------+-----------+ |
> >> + * +------------------------------+
> >> + * |
> >> + * +---------<---------v------------>----------+
> >> + * | |
> >> + * +--------v----------------+ |
> >> + * | dp_phy_pll_link_clk | |
> >> + * | link_clk | |
> >> + * +--------+----------------+ |
> >> + * | |
> >> + * | |
> >> + * v v
> >> + * Input to DISPCC block |
> >> + * for link clk, crypto clk |
> >> + * and interface clock |
> >> + * |
> >> + * |
> >> + * +--------<------------+-----------------+---<---+
> >> + * | | |
> >> + * +----v---------+ +--------v-----+ +--------v------+
> >> + * | vco_divided | | vco_divided | | vco_divided |
> >> + * | _clk_src | | _clk_src | | _clk_src |
> >> + * | | | | | |
> >> + * |divsel_six | | divsel_two | | divsel_four |
> >> + * +-------+------+ +-----+--------+ +--------+------+
> >> + * | | |
> >> + * v---->----------v-------------<------v
> >> + * |
> >> + * +----------+-----------------+
> >> + * | dp_phy_pll_vco_div_clk |
> >> + * +---------+------------------+
> >> + * |
> >> + * v
> >> + * Input to DISPCC block
> >> + * for DP pixel clock
> >> + *
> >> + */
> >> +static int qmp_dp_pixel_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
> >> +{
> >> + switch (req->rate) {
> >> + case 1620000000UL / 2:
> >> + case 2700000000UL / 2:
> >> + /* 5.4 and 8.1 GHz are same link rate as 2.7GHz, i.e. div 4 and div 6 */
> >> + return 0;
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> +}
> >> +
> >> +static unsigned long qmp_dp_pixel_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> >> +{
> >> + const struct qmp_usbc *qmp;
> >> + const struct phy_configure_opts_dp *dp_opts;
> >> +
> >> + qmp = container_of(hw, struct qmp_usbc, dp_pixel_hw);
> >> +
> >> + dp_opts = &qmp->dp_opts;
> >> +
> >> + switch (dp_opts->link_rate) {
> >> + case 1620:
> >> + return 1620000000UL / 2;
> >> + case 2700:
> >> + return 2700000000UL / 2;
> >> + case 5400:
> >> + return 5400000000UL / 4;
> > No HBR3 support? Then why was it mentioned few lines above?
> >
> >> + default:
> >> + return 0;
> >> + }
> >> +}
> >> +
> >
> >> +static int qmp_usbc_register_clocks(struct qmp_usbc *qmp, struct device_node *np)
> >> +{
> >> + int ret;
> >>
> >> - ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
> >> + 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);
> >> + if (qmp->dp_serdes != 0) {
> >> + ret = phy_dp_clks_register(qmp, np);
> >> + if (ret)
> >> + return ret;
> >> + }
> >> +
> >> + return devm_of_clk_add_hw_provider(qmp->dev, qmp_usbc_clks_hw_get, qmp);
> > Do you understand what did the comment (that you've removed) say? And
> > why?
And this was ignored :-(
> >
> >> }
> >>
> >> #if IS_ENABLED(CONFIG_TYPEC)
> >> @@ -1429,6 +1604,13 @@ static int qmp_usbc_parse_dt(struct qmp_usbc *qmp)
> >> if (IS_ERR(base))
> >> return PTR_ERR(base);
> >>
> >> + if (offs->dp_serdes != 0) {
> >> + qmp->dp_serdes = base + offs->dp_serdes;
> >> + qmp->dp_tx = base + offs->dp_txa;
> >> + qmp->dp_tx2 = base + offs->dp_txb;
> >> + qmp->dp_dp_phy = base + offs->dp_dp_phy;
> >> + }
> >> +
> >> qmp->serdes = base + offs->serdes;
> >> qmp->pcs = base + offs->pcs;
> >> if (offs->pcs_misc)
> >> @@ -1537,7 +1719,7 @@ static int qmp_usbc_probe(struct platform_device *pdev)
> >> */
> >> pm_runtime_forbid(dev);
> >>
> >> - ret = phy_pipe_clk_register(qmp, np);
> >> + ret = qmp_usbc_register_clocks(qmp, np);
> >> if (ret)
> >> goto err_node_put;
> >>
> >>
> >> --
> >> 2.34.1
> >>
--
With best wishes
Dmitry
On 9/12/2025 8:08 PM, Dmitry Baryshkov wrote:
> On Fri, Sep 12, 2025 at 08:00:14PM +0800, Xiangxu Yin wrote:
>> On 9/12/2025 6:19 PM, Dmitry Baryshkov wrote:
>>> On Thu, Sep 11, 2025 at 10:55:05PM +0800, Xiangxu Yin wrote:
>>>> Add USB/DP switchable PHY clock registration and DT parsing for DP offsets.
>>>> Extend qmp_usbc_register_clocks and clock provider logic to support both
>>>> USB and DP instances.
>>>>
>>>> Signed-off-by: Xiangxu Yin <xiangxu.yin@oss.qualcomm.com>
>>>> ---
>>>> drivers/phy/qualcomm/phy-qcom-qmp-usbc.c | 208 +++++++++++++++++++++++++++++--
>>>> 1 file changed, 195 insertions(+), 13 deletions(-)
>>>>
>>>> @@ -1276,8 +1291,11 @@ static int phy_pipe_clk_register(struct qmp_usbc *qmp, struct device_node *np)
>>>>
>>>> ret = of_property_read_string(np, "clock-output-names", &init.name);
>>>> if (ret) {
>>>> - dev_err(qmp->dev, "%pOFn: No clock-output-names\n", np);
>>>> - return ret;
>>>> + char name[64];
>>>> +
>>>> + /* Clock name is not mandatory. */
>>>> + snprintf(name, sizeof(name), "%s::pipe_clk", dev_name(qmp->dev));
>>>> + init.name = name;
>>>> }
>>> Do we have any guarantees that memory for 'name' exists beyond this point?
>>
>> If the previous of_property_read_string() call succeeded, could 'name'
>> still be empty? or you means 'char name[64]' will be release after '}'?
>>
>> From local verification, I can see 88e8000.phy::pipe_clk node from clk_summary.
> char name[64] belong to a stack frame that is not guaranteed to exist
> after you've close this bracked. So it can be easily overwritten with
> other values.
You are right, will move char name[64] declaration to beginning of function.
>>
>>>>
>>>> init.ops = &clk_fixed_rate_ops;
>>>> @@ -1286,19 +1304,176 @@ static int phy_pipe_clk_register(struct qmp_usbc *qmp, struct device_node *np)
>>>> fixed->fixed_rate = 125000000;
>>>> fixed->hw.init = &init;
>>>>
>>>> - ret = devm_clk_hw_register(qmp->dev, &fixed->hw);
>>>> - if (ret)
>>>> + return devm_clk_hw_register(qmp->dev, &fixed->hw);
>>>> +}
>>>> +
>>>> +
>>>> +/*
>>>> + * Display Port PLL driver block diagram for branch clocks
>>>> + *
>>>> + * +------------------------------+
>>>> + * | DP_VCO_CLK |
>>>> + * | |
>>>> + * | +-------------------+ |
>>>> + * | | (DP PLL/VCO) | |
>>>> + * | +---------+---------+ |
>>>> + * | v |
>>>> + * | +----------+-----------+ |
>>>> + * | | hsclk_divsel_clk_src | |
>>>> + * | +----------+-----------+ |
>>>> + * +------------------------------+
>>>> + * |
>>>> + * +---------<---------v------------>----------+
>>>> + * | |
>>>> + * +--------v----------------+ |
>>>> + * | dp_phy_pll_link_clk | |
>>>> + * | link_clk | |
>>>> + * +--------+----------------+ |
>>>> + * | |
>>>> + * | |
>>>> + * v v
>>>> + * Input to DISPCC block |
>>>> + * for link clk, crypto clk |
>>>> + * and interface clock |
>>>> + * |
>>>> + * |
>>>> + * +--------<------------+-----------------+---<---+
>>>> + * | | |
>>>> + * +----v---------+ +--------v-----+ +--------v------+
>>>> + * | vco_divided | | vco_divided | | vco_divided |
>>>> + * | _clk_src | | _clk_src | | _clk_src |
>>>> + * | | | | | |
>>>> + * |divsel_six | | divsel_two | | divsel_four |
>>>> + * +-------+------+ +-----+--------+ +--------+------+
>>>> + * | | |
>>>> + * v---->----------v-------------<------v
>>>> + * |
>>>> + * +----------+-----------------+
>>>> + * | dp_phy_pll_vco_div_clk |
>>>> + * +---------+------------------+
>>>> + * |
>>>> + * v
>>>> + * Input to DISPCC block
>>>> + * for DP pixel clock
>>>> + *
>>>> + */
>>>> +static int qmp_dp_pixel_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
>>>> +{
>>>> + switch (req->rate) {
>>>> + case 1620000000UL / 2:
>>>> + case 2700000000UL / 2:
>>>> + /* 5.4 and 8.1 GHz are same link rate as 2.7GHz, i.e. div 4 and div 6 */
>>>> + return 0;
>>>> + default:
>>>> + return -EINVAL;
>>>> + }
>>>> +}
>>>> +
>>>> +static unsigned long qmp_dp_pixel_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
>>>> +{
>>>> + const struct qmp_usbc *qmp;
>>>> + const struct phy_configure_opts_dp *dp_opts;
>>>> +
>>>> + qmp = container_of(hw, struct qmp_usbc, dp_pixel_hw);
>>>> +
>>>> + dp_opts = &qmp->dp_opts;
>>>> +
>>>> + switch (dp_opts->link_rate) {
>>>> + case 1620:
>>>> + return 1620000000UL / 2;
>>>> + case 2700:
>>>> + return 2700000000UL / 2;
>>>> + case 5400:
>>>> + return 5400000000UL / 4;
>>> No HBR3 support? Then why was it mentioned few lines above?
Yes, no HBR3 support, will update annotation in qmp_dp_pixel_clk_determine_rate
>>>> + default:
>>>> + return 0;
>>>> + }
>>>> +}
>>>> +
>>>> +static int qmp_usbc_register_clocks(struct qmp_usbc *qmp, struct device_node *np)
>>>> +{
>>>> + int ret;
>>>>
>>>> - ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
>>>> + 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);
>>>> + if (qmp->dp_serdes != 0) {
>>>> + ret = phy_dp_clks_register(qmp, np);
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>> +
>>>> + return devm_of_clk_add_hw_provider(qmp->dev, qmp_usbc_clks_hw_get, qmp);
>>> Do you understand what did the comment (that you've removed) say? And
>>> why?
>
> And this was ignored :-(
Sorry for missing this part.
For USB-C PHY, the legacy implementation only supports USB with a single
device node. The new driver for USB and DP also uses a single device node.
The function devm_of_clk_add_hw_provider internally handles both
of_clk_add_hw_provider and devres_add, and supports automatic resource
release.
So I think using devm_of_clk_add_hw_provider allows us to remove
of_clk_add_hw_provider and devm_add_action_or_reset.
For combo PHY, the legacy implementation uses two device nodes: dp_np and
usb_np. To maintain forward compatibility, we need to keep support for
both nodes and retain the related logic.
>>>> }
>>>>
>>>> #if IS_ENABLED(CONFIG_TYPEC)
>>>> @@ -1429,6 +1604,13 @@ static int qmp_usbc_parse_dt(struct qmp_usbc *qmp)
>>>> if (IS_ERR(base))
>>>> return PTR_ERR(base);
>>>>
>>>> + if (offs->dp_serdes != 0) {
>>>> + qmp->dp_serdes = base + offs->dp_serdes;
>>>> + qmp->dp_tx = base + offs->dp_txa;
>>>> + qmp->dp_tx2 = base + offs->dp_txb;
>>>> + qmp->dp_dp_phy = base + offs->dp_dp_phy;
>>>> + }
>>>> +
>>>> qmp->serdes = base + offs->serdes;
>>>> qmp->pcs = base + offs->pcs;
>>>> if (offs->pcs_misc)
>>>> @@ -1537,7 +1719,7 @@ static int qmp_usbc_probe(struct platform_device *pdev)
>>>> */
>>>> pm_runtime_forbid(dev);
>>>>
>>>> - ret = phy_pipe_clk_register(qmp, np);
>>>> + ret = qmp_usbc_register_clocks(qmp, np);
>>>> if (ret)
>>>> goto err_node_put;
>>>>
>>>>
>>>> --
>>>> 2.34.1
>>>>
On Mon, Sep 15, 2025 at 06:02:19PM +0800, Xiangxu Yin wrote:
>
> On 9/12/2025 8:08 PM, Dmitry Baryshkov wrote:
> > On Fri, Sep 12, 2025 at 08:00:14PM +0800, Xiangxu Yin wrote:
> >> On 9/12/2025 6:19 PM, Dmitry Baryshkov wrote:
> >>> On Thu, Sep 11, 2025 at 10:55:05PM +0800, Xiangxu Yin wrote:
> >>>> Add USB/DP switchable PHY clock registration and DT parsing for DP offsets.
> >>>> Extend qmp_usbc_register_clocks and clock provider logic to support both
> >>>> USB and DP instances.
> >>>>
> >>>> Signed-off-by: Xiangxu Yin <xiangxu.yin@oss.qualcomm.com>
> >>>> ---
> >>>> drivers/phy/qualcomm/phy-qcom-qmp-usbc.c | 208 +++++++++++++++++++++++++++++--
> >>>> 1 file changed, 195 insertions(+), 13 deletions(-)
> >>>> + default:
> >>>> + return 0;
> >>>> + }
> >>>> +}
> >>>> +
> >>>> +static int qmp_usbc_register_clocks(struct qmp_usbc *qmp, struct device_node *np)
> >>>> +{
> >>>> + int ret;
> >>>>
> >>>> - ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
> >>>> + 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);
> >>>> + if (qmp->dp_serdes != 0) {
> >>>> + ret = phy_dp_clks_register(qmp, np);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> + }
> >>>> +
> >>>> + return devm_of_clk_add_hw_provider(qmp->dev, qmp_usbc_clks_hw_get, qmp);
> >>> Do you understand what did the comment (that you've removed) say? And
> >>> why?
> >
> > And this was ignored :-(
>
> Sorry for missing this part.
>
> For USB-C PHY, the legacy implementation only supports USB with a single
> device node. The new driver for USB and DP also uses a single device node.
There is no 'new driver'. It's about DT.
> The function devm_of_clk_add_hw_provider internally handles both
> of_clk_add_hw_provider and devres_add, and supports automatic resource
> release.
>
> So I think using devm_of_clk_add_hw_provider allows us to remove
> of_clk_add_hw_provider and devm_add_action_or_reset.
Which node is passed to of_clk_add_hw_provider() in the legacy DT case?
Which node is passed to of_clk_add_hw_provider() by
devm_of_clk_add_hw_provider()?
> For combo PHY, the legacy implementation uses two device nodes: dp_np and
> usb_np. To maintain forward compatibility, we need to keep support for
> both nodes and retain the related logic.
--
With best wishes
Dmitry
On 9/15/2025 6:14 PM, Dmitry Baryshkov wrote:
> On Mon, Sep 15, 2025 at 06:02:19PM +0800, Xiangxu Yin wrote:
>> On 9/12/2025 8:08 PM, Dmitry Baryshkov wrote:
>>> On Fri, Sep 12, 2025 at 08:00:14PM +0800, Xiangxu Yin wrote:
>>>> On 9/12/2025 6:19 PM, Dmitry Baryshkov wrote:
>>>>> On Thu, Sep 11, 2025 at 10:55:05PM +0800, Xiangxu Yin wrote:
>>>>>> Add USB/DP switchable PHY clock registration and DT parsing for DP offsets.
>>>>>> Extend qmp_usbc_register_clocks and clock provider logic to support both
>>>>>> USB and DP instances.
>>>>>>
>>>>>> Signed-off-by: Xiangxu Yin <xiangxu.yin@oss.qualcomm.com>
>>>>>> ---
>>>>>> drivers/phy/qualcomm/phy-qcom-qmp-usbc.c | 208 +++++++++++++++++++++++++++++--
>>>>>> 1 file changed, 195 insertions(+), 13 deletions(-)
>>>>>> + default:
>>>>>> + return 0;
>>>>>> + }
>>>>>> +}
>>>>>> +
>>>>>> +static int qmp_usbc_register_clocks(struct qmp_usbc *qmp, struct device_node *np)
>>>>>> +{
>>>>>> + int ret;
>>>>>>
>>>>>> - ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, &fixed->hw);
>>>>>> + 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);
>>>>>> + if (qmp->dp_serdes != 0) {
>>>>>> + ret = phy_dp_clks_register(qmp, np);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + return devm_of_clk_add_hw_provider(qmp->dev, qmp_usbc_clks_hw_get, qmp);
>>>>> Do you understand what did the comment (that you've removed) say? And
>>>>> why?
>>> And this was ignored :-(
>> Sorry for missing this part.
>>
>> For USB-C PHY, the legacy implementation only supports USB with a single
>> device node. The new driver for USB and DP also uses a single device node.
> There is no 'new driver'. It's about DT.
>
>> The function devm_of_clk_add_hw_provider internally handles both
>> of_clk_add_hw_provider and devres_add, and supports automatic resource
>> release.
>>
>> So I think using devm_of_clk_add_hw_provider allows us to remove
>> of_clk_add_hw_provider and devm_add_action_or_reset.
> Which node is passed to of_clk_add_hw_provider() in the legacy DT case?
> Which node is passed to of_clk_add_hw_provider() by
> devm_of_clk_add_hw_provider()?
Ohh, legacy is child node and devm is dev->of_node.
Will add that back for compatibility.
>> For combo PHY, the legacy implementation uses two device nodes: dp_np and
>> usb_np. To maintain forward compatibility, we need to keep support for
>> both nodes and retain the related logic.
© 2016 - 2026 Red Hat, Inc.