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