linux-kernel - Re: [PATCH v3 4/4] phy: qcom: edp: Add Glymur platform support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <DCQAGDC63M8X.3DVH6I9FA0IZD@linaro.org>
Date: Thu, 11 Sep 2025 22:28:24 +0100
From: "Alexey Klimov" <alexey.klimov@...aro.org>
To: "Abel Vesa" <abel.vesa@...aro.org>, "Vinod Koul" <vkoul@...nel.org>,
 "Kishon Vijay Abraham I" <kishon@...nel.org>, "Rob Herring"
 <robh@...nel.org>, "Krzysztof Kozlowski" <krzk+dt@...nel.org>, "Conor
 Dooley" <conor+dt@...nel.org>, "Bjorn Andersson" <andersson@...nel.org>,
 "Dmitry Baryshkov" <lumag@...nel.org>, "Konrad Dybcio"
 <konradybcio@...nel.org>, "Neil Armstrong" <neil.armstrong@...aro.org>
Cc: "Johan Hovold" <johan@...nel.org>, <linux-arm-msm@...r.kernel.org>,
 <linux-phy@...ts.infradead.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 4/4] phy: qcom: edp: Add Glymur platform support

On Thu Sep 11, 2025 at 3:45 PM BST, Abel Vesa wrote:
> The Qualcomm Glymur platform has the new v8 version
> of the eDP/DP PHY. So rework the driver to support this
> new version and add the platform specific configuration data.

It is a bit confusing. Subject suggests that it is an addition
of a new platform but patch itself and description looks more like a
rework rather than new platform addition.

The ->aux_cfg_size() rework here reminds me
913463587d52 phy: qcom: edp: Introduce aux_cfg array for version specific aux settings

Ideally this should be split into rework and adding support for a
new platform. Or please update the commit desc and subject to explain
why this is the way.

> Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-edp.c | 240 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 234 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c
> index 7b642742412e63149442e4befeb095307ec38173..b670cda0fa066d3ff45c66b73cc67e165e55b79a 100644
> --- a/drivers/phy/qualcomm/phy-qcom-edp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-edp.c

[..]

>  static int qcom_edp_phy_init(struct phy *phy)
>  {
>  	struct qcom_edp *edp = phy_get_drvdata(phy);
> @@ -224,7 +241,11 @@ static int qcom_edp_phy_init(struct phy *phy)
>  	if (ret)
>  		goto out_disable_supplies;
>  
> -	memcpy(aux_cfg, edp->cfg->aux_cfg, sizeof(aux_cfg));
> +	memcpy(aux_cfg, edp->cfg->aux_cfg, edp->cfg->aux_cfg_size);

So, if I understand this correctly, when or if init sequence will
span beyond DP_PHY_AUX_CFG9 and DP_AUX_CFG_SIZE won't be updated,
then we might end up doing something fishy here?

Maybe add an if-check or even
BUILD_BUG_ON(edp->cfg->aux_cfg_size > sizeof(aux_cfg))
or something like this? Or kmalloc aux_cfg eventually at least,
however it seems to overcomplicate things.

[..]

> +static int qcom_edp_com_configure_ssc_v8(const struct qcom_edp *edp)
> +{
> +	const struct phy_configure_opts_dp *dp_opts = &edp->dp_opts;
> +	u32 step1;
> +	u32 step2;
> +
> +	switch (dp_opts->link_rate) {
> +	case 1620:
> +	case 2700:
> +	case 8100:
> +		step1 = 0x5b;
> +		step2 = 0x02;
> +		break;
> +
> +	case 5400:
> +		step1 = 0x5b;
> +		step2 = 0x02;
> +		break;
> +
> +	default:
> +		/* Other link rates aren't supported */
> +		return -EINVAL;
> +	}
> +
> +	writel(0x01, edp->pll + DP_QSERDES_V8_COM_SSC_EN_CENTER);
> +	writel(0x00, edp->pll + DP_QSERDES_V8_COM_SSC_ADJ_PER1);
> +	writel(0x6b, edp->pll + DP_QSERDES_V8_COM_SSC_PER1);
> +	writel(0x02, edp->pll + DP_QSERDES_V8_COM_SSC_PER2);
> +	writel(step1, edp->pll + DP_QSERDES_V8_COM_SSC_STEP_SIZE1_MODE0);
> +	writel(step2, edp->pll + DP_QSERDES_V8_COM_SSC_STEP_SIZE2_MODE0);
> +
> +	return 0;
> +}
> +
> +static int qcom_edp_com_configure_pll_v8(const struct qcom_edp *edp)
> +{
> +	const struct phy_configure_opts_dp *dp_opts = &edp->dp_opts;
> +	u32 div_frac_start2_mode0;
> +	u32 div_frac_start3_mode0;
> +	u32 dec_start_mode0;
> +	u32 lock_cmp1_mode0;
> +	u32 lock_cmp2_mode0;
> +	u32 code1_mode0;
> +	u32 code2_mode0;
> +	u32 hsclk_sel;
> +
> +	switch (dp_opts->link_rate) {
> +	case 1620:
> +		hsclk_sel = 0x5;
> +		dec_start_mode0 = 0x34;
> +		div_frac_start2_mode0 = 0xc0;
> +		div_frac_start3_mode0 = 0x0b;
> +		lock_cmp1_mode0 = 0x37;
> +		lock_cmp2_mode0 = 0x04;
> +		code1_mode0 = 0x71;
> +		code2_mode0 = 0x0c;
> +		break;
> +
> +	case 2700:
> +		hsclk_sel = 0x3;
> +		dec_start_mode0 = 0x34;
> +		div_frac_start2_mode0 = 0xc0;
> +		div_frac_start3_mode0 = 0x0b;
> +		lock_cmp1_mode0 = 0x07;
> +		lock_cmp2_mode0 = 0x07;
> +		code1_mode0 = 0x71;
> +		code2_mode0 = 0x0c;
> +		break;
> +
> +	case 5400:
> +		hsclk_sel = 0x2;
> +		dec_start_mode0 = 0x4f;
> +		div_frac_start2_mode0 = 0xa0;
> +		div_frac_start3_mode0 = 0x01;
> +		lock_cmp1_mode0 = 0x18;
> +		lock_cmp2_mode0 = 0x15;
> +		code1_mode0 = 0x14;
> +		code2_mode0 = 0x25;
> +		break;
> +
> +	case 8100:
> +		hsclk_sel = 0x2;
> +		dec_start_mode0 = 0x4f;
> +		div_frac_start2_mode0 = 0xa0;
> +		div_frac_start3_mode0 = 0x01;
> +		lock_cmp1_mode0 = 0x18;
> +		lock_cmp2_mode0 = 0x15;
> +		code1_mode0 = 0x14;
> +		code2_mode0 = 0x25;
> +		break;

These sections for 5400 and 8100 rates seem to be the same. Is it correct?
If yes, then maybe join them together and drop duplicating lines?

There is probably similar thingy in qcom_edp_com_configure_ssc_v8() above.

Best regards,
Alexey