Re: [PATCH v4 08/13] phy: qcom: qmp-usbc: Add USB/DP switchable PHY clk register

[Xiangxu Yin <xiangxu.yin@oss.qualcomm.com>]

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
>>>>