Message-ID: <llrt3xnd5gagovnmyzqebp2da5v67bkxjntfcgc5r5auamspyj@7v5taph3i3c4>
Date: Fri, 22 Aug 2025 13:08:37 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Xiangxu Yin <xiangxu.yin@....qualcomm.com>
Cc: Rob Clark <robin.clark@....qualcomm.com>,
        Dmitry Baryshkov <lumag@...nel.org>,
        Abhinav Kumar <abhinav.kumar@...ux.dev>,
        Jessica Zhang <jessica.zhang@....qualcomm.com>,
        Sean Paul <sean@...rly.run>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Kuogee Hsieh <quic_khsieh@...cinc.com>, Vinod Koul <vkoul@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>, linux-arm-msm@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-phy@...ts.infradead.org, fange.zhang@....qualcomm.com,
        yongxing.mou@....qualcomm.com, tingwei.zhang@....qualcomm.com,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>
Subject: Re: [PATCH v3 07/14] phy: qcom: qmp-usbc: Move reset and regulator
 config into PHY cfg

On Fri, Aug 22, 2025 at 04:29:28PM +0800, Xiangxu Yin wrote:
> 
> On 8/20/2025 7:30 PM, Dmitry Baryshkov wrote:
> > On Wed, Aug 20, 2025 at 05:34:49PM +0800, Xiangxu Yin wrote:
> >> Refactor reset and regulator configuration to be managed via qmp_phy_cfg
> >> instead of hardcoded lists. This enables per-PHY customization and
> >> simplifies initialization logic for USB-only and USB/DP switchable PHYs.
> > Please split into two patches in order to simplify reviewing.
> 
> 
> Ok, will split reset and regulator part.
> 
> 
> >> Signed-off-by: Xiangxu Yin <xiangxu.yin@....qualcomm.com>
> >> ---
> >>  drivers/phy/qualcomm/phy-qcom-qmp-usbc.c | 108 +++++++++++++++----------------
> >>  1 file changed, 53 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usbc.c b/drivers/phy/qualcomm/phy-qcom-qmp-usbc.c
> >> index 61128d606238321d1b573655b3b987226aa2d594..4e797b7e65da0e3a827efa9a179f1c150c1b8b00 100644
> >> --- a/drivers/phy/qualcomm/phy-qcom-qmp-usbc.c
> >> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-usbc.c
> >> @@ -421,8 +421,9 @@ struct qmp_phy_cfg {
> >>  	int (*configure_dp_phy)(struct qmp_usbc *qmp);
> >>  	int (*calibrate_dp_phy)(struct qmp_usbc *qmp);
> >>  
> >> -	/* regulators to be requested */
> >> -	const char * const *vreg_list;
> >> +	const char * const *reset_list;
> >> +	int num_resets;
> >> +	const struct regulator_bulk_data *vreg_list;
> >>  	int num_vregs;
> >>  
> >>  	/* array of registers with different offsets */
> >> @@ -453,7 +454,6 @@ struct qmp_usbc {
> >>  	struct clk_hw dp_pixel_hw;
> >>  	struct clk_bulk_data *clks;
> >>  	int num_clks;
> >> -	int num_resets;
> >>  	struct reset_control_bulk_data *resets;
> >>  	struct regulator_bulk_data *vregs;
> >>  
> >> @@ -514,9 +514,18 @@ static const char * const usb3phy_reset_l[] = {
> >>  	"phy_phy", "phy",
> >>  };
> >>  
> >> -/* list of regulators */
> >> -static const char * const qmp_phy_vreg_l[] = {
> >> -	"vdda-phy", "vdda-pll",
> >> +static const char * const usb3dpphy_reset_l[] = {
> >> +	"phy_phy", "dp_phy",
> >> +};
> >> +
> >> +static const struct regulator_bulk_data qmp_phy_usb_vreg_l[] = {
> >> +	{ .supply = "vdda-phy" },
> >> +	{ .supply = "vdda-pll" },
> > Please fill in the values for all platforms. It well might be that they
> > all share the same current requirements.
> 
> 
> I checked previous DP projects and found all platforms configured vdda-phy with 21.8mA and vdda-pll with 36mA.

No. On SDM660 and MSM8998 DP defines 73.4 mA for 0.9V supply and
12.560 mA for 1.8 V supply.

> 
> However, I didn’t find USB load configs in downstream and from SoC related power grids:

Please check the actual HW documentation for those platforms.

> QCS615
> L12A: VDDA_USB0_SS_1P8/VDDA_USB1_SS_1P8 Ipk:20ma
> L5A: VDDA_USB0_SS_0P9/VDDA_USB1_SS_0P9 Ipk:50mA
> 
> sm6150
> L11A: VDDA_USB0_SS_1P8/VDDA_USB1_SS_1P8 Ipk:20ma
> L4A: VDDA_USB0_SS_0P9/VDDA_USB1_SS_0P9 Ipk:50mA
> 
> SM6115
> L12A: VDDA_USB_SS_DP_1P8 Ipk:13.3mA
> L4A: VDDA_USB_SS_DP_CORE Ipk:66.1mA
> 
> QCM2290
> L13A: VDDA_USB_SS_DP_1P8 Ipk:13.3mA
> L12A: VDDA_USB_SS_DP_CORE Ipk:66.1mA
> 
> sdm660
> LDO10A: VDDA_USB_SS_1P8 Ipk:14mA
> LDO1B: VDDA_USB_SS_CORE Ipk:68.6mA
> 
> msm8998
> L2A: VDDA_USB_SS_1P2 Ipk:14.2mA
> L1A: VDDA_USB_SS_CORE Ipk:68.6mA
> 
> It seems the USB power requirements vary across platforms, and the
> 21800 µA load for vdda-phy exceeds the Ipk range in most cases.

Ipk being ?

> I also tested removing the load settings for USB+DP PHY, and DP still works fine.

It mostly works either because we don't allow mode switching on older
platforms (yet) or because somebody else has already voted and that vote
keeps the required mode.

As you've started looking on specifying proper current load, please
finish the work.

> So, can we keep the regulator config as original qmp_phy_vreg_l?
> static const char * const qmp_phy_vreg_l[] = { "vdda-phy", "vdda-pll"} 
> 
> 
> >> +};
> >> +
> >> +static const struct regulator_bulk_data qmp_phy_usbdp_vreg_l[] = {
> >> +	{ .supply = "vdda-phy", .init_load_uA = 21800 },
> >> +	{ .supply = "vdda-phy", .init_load_uA = 36000 },
> > Typo
> 
> 
> Sorry for Typo, will fix in next patch.
> 
> 
> >>  };
> >>  
> >>  static const struct qmp_usbc_offsets qmp_usbc_offsets_v3_qcm2290 = {
> >> @@ -569,8 +578,10 @@ static const struct qmp_phy_cfg msm8998_usb3phy_cfg = {
> >>  	.rx_tbl_num             = ARRAY_SIZE(msm8998_usb3_rx_tbl),
> >>  	.pcs_tbl                = msm8998_usb3_pcs_tbl,
> >>  	.pcs_tbl_num            = ARRAY_SIZE(msm8998_usb3_pcs_tbl),
> >> -	.vreg_list              = qmp_phy_vreg_l,
> >> -	.num_vregs              = ARRAY_SIZE(qmp_phy_vreg_l),
> >> +	.reset_list		= usb3phy_reset_l,
> >> +	.num_resets		= ARRAY_SIZE(usb3phy_reset_l),
> >> +	.vreg_list              = qmp_phy_usb_vreg_l,
> >> +	.num_vregs              = ARRAY_SIZE(qmp_phy_usb_vreg_l),
> >>  	.regs                   = qmp_v3_usb3phy_regs_layout,
> >>  };
> >>  
> >> @@ -586,8 +597,10 @@ static const struct qmp_phy_cfg qcm2290_usb3phy_cfg = {
> >>  	.rx_tbl_num		= ARRAY_SIZE(qcm2290_usb3_rx_tbl),
> >>  	.pcs_tbl		= qcm2290_usb3_pcs_tbl,
> >>  	.pcs_tbl_num		= ARRAY_SIZE(qcm2290_usb3_pcs_tbl),
> >> -	.vreg_list		= qmp_phy_vreg_l,
> >> -	.num_vregs		= ARRAY_SIZE(qmp_phy_vreg_l),
> >> +	.reset_list		= usb3phy_reset_l,
> >> +	.num_resets		= ARRAY_SIZE(usb3phy_reset_l),
> >> +	.vreg_list		= qmp_phy_usb_vreg_l,
> >> +	.num_vregs		= ARRAY_SIZE(qmp_phy_usb_vreg_l),
> >>  	.regs			= qmp_v3_usb3phy_regs_layout_qcm2290,
> >>  };
> >>  
> >> @@ -603,8 +616,10 @@ static const struct qmp_phy_cfg sdm660_usb3phy_cfg = {
> >>  	.rx_tbl_num		= ARRAY_SIZE(sdm660_usb3_rx_tbl),
> >>  	.pcs_tbl		= qcm2290_usb3_pcs_tbl,
> >>  	.pcs_tbl_num		= ARRAY_SIZE(qcm2290_usb3_pcs_tbl),
> >> -	.vreg_list		= qmp_phy_vreg_l,
> >> -	.num_vregs		= ARRAY_SIZE(qmp_phy_vreg_l),
> >> +	.reset_list		= usb3phy_reset_l,
> >> +	.num_resets		= ARRAY_SIZE(usb3phy_reset_l),
> >> +	.vreg_list		= qmp_phy_usb_vreg_l,
> >> +	.num_vregs		= ARRAY_SIZE(qmp_phy_usb_vreg_l),
> >>  	.regs			= qmp_v3_usb3phy_regs_layout_qcm2290,
> >>  };
> >>  
> >> @@ -637,6 +652,11 @@ static const struct qmp_phy_cfg qcs615_usb3dp_phy_cfg = {
> >>  
> >>  	.swing_tbl		= &qmp_dp_voltage_swing_hbr2_rbr,
> >>  	.pre_emphasis_tbl	= &qmp_dp_pre_emphasis_hbr2_rbr,
> >> +
> >> +	.reset_list		= usb3dpphy_reset_l,
> >> +	.num_resets		= ARRAY_SIZE(usb3dpphy_reset_l),
> >> +	.vreg_list		= qmp_phy_usbdp_vreg_l,
> >> +	.num_vregs		= ARRAY_SIZE(qmp_phy_usbdp_vreg_l),
> >>  };
> >>  
> >>  static int qmp_usbc_com_init(struct phy *phy)
> >> @@ -653,13 +673,13 @@ static int qmp_usbc_com_init(struct phy *phy)
> >>  		return ret;
> >>  	}
> >>  
> >> -	ret = reset_control_bulk_assert(qmp->num_resets, qmp->resets);
> >> +	ret = reset_control_bulk_assert(cfg->num_resets, qmp->resets);
> >>  	if (ret) {
> >>  		dev_err(qmp->dev, "reset assert failed\n");
> >>  		goto err_disable_regulators;
> >>  	}
> >>  
> >> -	ret = reset_control_bulk_deassert(qmp->num_resets, qmp->resets);
> >> +	ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets);
> >>  	if (ret) {
> >>  		dev_err(qmp->dev, "reset deassert failed\n");
> >>  		goto err_disable_regulators;
> >> @@ -682,7 +702,7 @@ static int qmp_usbc_com_init(struct phy *phy)
> >>  	return 0;
> >>  
> >>  err_assert_reset:
> >> -	reset_control_bulk_assert(qmp->num_resets, qmp->resets);
> >> +	reset_control_bulk_assert(cfg->num_resets, qmp->resets);
> >>  err_disable_regulators:
> >>  	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
> >>  
> >> @@ -694,7 +714,7 @@ static int qmp_usbc_com_exit(struct phy *phy)
> >>  	struct qmp_usbc *qmp = phy_get_drvdata(phy);
> >>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
> >>  
> >> -	reset_control_bulk_assert(qmp->num_resets, qmp->resets);
> >> +	reset_control_bulk_assert(cfg->num_resets, qmp->resets);
> >>  
> >>  	clk_bulk_disable_unprepare(qmp->num_clks, qmp->clks);
> >>  
> >> @@ -921,42 +941,22 @@ static const struct dev_pm_ops qmp_usbc_pm_ops = {
> >>  			   qmp_usbc_runtime_resume, NULL)
> >>  };
> >>  
> >> -static int qmp_usbc_vreg_init(struct qmp_usbc *qmp)
> >> +static int qmp_usbc_reset_init(struct qmp_usbc *qmp)
> >>  {
> >>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
> >> -	struct device *dev = qmp->dev;
> >> -	int num = cfg->num_vregs;
> >> -	int i;
> >> -
> >> -	qmp->vregs = devm_kcalloc(dev, num, sizeof(*qmp->vregs), GFP_KERNEL);
> >> -	if (!qmp->vregs)
> >> -		return -ENOMEM;
> >> -
> >> -	for (i = 0; i < num; i++)
> >> -		qmp->vregs[i].supply = cfg->vreg_list[i];
> >> -
> >> -	return devm_regulator_bulk_get(dev, num, qmp->vregs);
> >> -}
> >> -
> >> -static int qmp_usbc_reset_init(struct qmp_usbc *qmp,
> >> -			      const char *const *reset_list,
> >> -			      int num_resets)
> >> -{
> >>  	struct device *dev = qmp->dev;
> >>  	int i;
> >>  	int ret;
> >>  
> >> -	qmp->resets = devm_kcalloc(dev, num_resets,
> >> +	qmp->resets = devm_kcalloc(dev, cfg->num_resets,
> >>  				   sizeof(*qmp->resets), GFP_KERNEL);
> >>  	if (!qmp->resets)
> >>  		return -ENOMEM;
> >>  
> >> -	for (i = 0; i < num_resets; i++)
> >> -		qmp->resets[i].id = reset_list[i];
> >> +	for (i = 0; i < cfg->num_resets; i++)
> >> +		qmp->resets[i].id = cfg->reset_list[i];
> >>  
> >> -	qmp->num_resets = num_resets;
> >> -
> >> -	ret = devm_reset_control_bulk_get_exclusive(dev, num_resets, qmp->resets);
> >> +	ret = devm_reset_control_bulk_get_exclusive(dev, cfg->num_resets, qmp->resets);
> >>  	if (ret)
> >>  		return dev_err_probe(dev, ret, "failed to get resets\n");
> >>  
> >> @@ -1146,11 +1146,6 @@ static int qmp_usbc_parse_usb_dt_legacy(struct qmp_usbc *qmp, struct device_node
> >>  
> >>  	qmp->num_clks = ret;
> >>  
> >> -	ret = qmp_usbc_reset_init(qmp, usb3phy_legacy_reset_l,
> >> -				 ARRAY_SIZE(usb3phy_legacy_reset_l));
> >> -	if (ret)
> >> -		return ret;
> >> -
> >>  	return 0;
> >>  }
> >>  
> >> @@ -1187,14 +1182,9 @@ static int qmp_usbc_parse_usb_dt(struct qmp_usbc *qmp)
> >>  	qmp->pipe_clk = devm_clk_get(dev, "pipe");
> >>  	if (IS_ERR(qmp->pipe_clk)) {
> >>  		return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk),
> >> -				     "failed to get pipe clock\n");
> >> +					"failed to get pipe clock\n");
> > unrelated
> 
> 
> Ack.
> 
> 
> >>  	}
> >>  
> >> -	ret = qmp_usbc_reset_init(qmp, usb3phy_reset_l,
> >> -				 ARRAY_SIZE(usb3phy_reset_l));
> >> -	if (ret)
> >> -		return ret;
> >> -
> >>  	return 0;
> >>  }
> >>  
> >> @@ -1228,6 +1218,7 @@ static int qmp_usbc_probe(struct platform_device *pdev)
> >>  	struct phy_provider *phy_provider;
> >>  	struct device_node *np;
> >>  	struct qmp_usbc *qmp;
> >> +	const struct qmp_phy_cfg *cfg;
> >>  	int ret;
> >>  
> >>  	qmp = devm_kzalloc(dev, sizeof(*qmp), GFP_KERNEL);
> >> @@ -1239,13 +1230,20 @@ static int qmp_usbc_probe(struct platform_device *pdev)
> >>  
> >>  	qmp->orientation = TYPEC_ORIENTATION_NORMAL;
> >>  
> >> -	qmp->cfg = of_device_get_match_data(dev);
> >> -	if (!qmp->cfg)
> >> +	cfg = of_device_get_match_data(dev);
> >> +	if (!cfg)
> >>  		return -EINVAL;
> >>  
> >> +	qmp->cfg = cfg;
> > Why? This doesn't seem related at all.
> 
> 
> I added the |cfg| variable to simplify access to |num_vregs| and |vreg_list| in the following lines, 
> 
> avoiding repeated |qmp->cfg->...| usage. 
> 
> If this is considered unrelated, I’ll remove it in the next version.

This all gets clogged in a mixture of regulator and reset changes.
Hopefully it will be more obvious with the patches being split.

> 
> 
> >
> >> +
> >>  	mutex_init(&qmp->phy_mutex);
> >>  
> >> -	ret = qmp_usbc_vreg_init(qmp);
> >> +	ret = qmp_usbc_reset_init(qmp);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = devm_regulator_bulk_get_const(qmp->dev, cfg->num_vregs,
> >> +					 cfg->vreg_list, &qmp->vregs);
> >>  	if (ret)
> >>  		return ret;
> >>  
> >>
> >> -- 
> >> 2.34.1
> >>

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists