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