Re: [PATCH v3 5/5] phy: qcom: qmp-pcie: Add Nord Gen5x16 PCIe multi-PHY support

From: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
To: Shawn Guo <shengchao.guo@oss.qualcomm.com>
Cc: "Vinod Koul" <vkoul@kernel.org>,
	"Neil Armstrong" <neil.armstrong@linaro.org>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kwilczynski@kernel.org>,
	"Manivannan Sadhasivam" <mani@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Bjorn Andersson" <andersson@kernel.org>,
	"Maxime Chevallier" <maxime.chevallier@bootlin.com>,
	"Philipp Zabel" <p.zabel@pengutronix.de>,
	linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-pci@vger.kernel.org
Subject: Re: [PATCH v3 5/5] phy: qcom: qmp-pcie: Add Nord Gen5x16 PCIe multi-PHY support
Date: Wed, 16 Sep 2026 18:56:50 +0530	[thread overview]
Message-ID: <5f862178-ba18-4b27-b5db-1732d57d7b47@oss.qualcomm.com> (raw)
In-Reply-To: <aqJFo3beKde404qd@QCOM-aGQu4IUr3Y>

On 9/10/2026 11:22 AM, Shawn Guo wrote:
>> const struct qmp_phy_cfg *cfg = qmp->cfg;
>>  
>> -	reset_control_bulk_assert(qmp->cfg->num_nocsr_resets, qmp->nocsr_resets);
>> +	if (qmp->nocsr_resets)
>> +		reset_control_bulk_assert(qmp->cfg->num_nocsr_resets, qmp->nocsr_resets);
>> +	if (qmp->resets)
>> +		reset_control_bulk_assert(cfg->num_resets, qmp->resets);
>>  
>>  	clk_bulk_disable_unprepare(qmp->cfg->num_clks, qmp->clks);
>>  	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>> @@ -335,6 +1067,20 @@ static int qmp_pcie_power_on(struct phy *phy)
>>  		goto err_disable_pipe_clk;
>>  	}
>>  
>> +	if (!qmp->skip_init) {
>> +		qmp_pcie_init_port_registers(qmp);
> Have we tested the patch on Glymur? As Glymur's cfgs have either .pll_tbls
> nor .tbls, so skip_init is unconditionally false for Glymur. Is it
> intentional to run qmp_pcie_init_port_registers() on Glymur?
Thanks for the catch, I will test for glymur before posting next series.
>
>> +
>> +		for (i = 0; i < cfg->num_regs; i++) {
>> +			qphy_clrbits(qmp->base[i] + offs->pcs,
>> +				     cfg->regs[QPHY_PCS_SW_RESET], SW_RESET);
>> +			qphy_setbits(qmp->base[i] + offs->pcs,
>> +				     cfg->regs[QPHY_PCS_START_CONTROL],
>> +				     SERDES_START | PCS_START);
>> +		}
>> +
>> +		usleep_range(2000, 2500);
>> +	}
>> +
>>  	for (i = 0; i < cfg->num_regs; i++) {
>>  		status = qmp->base[i] + offs->pcs + cfg->regs[QPHY_PCS_STATUS];
>>  		ret = readl_poll_timeout(status, val, !(val & cfg->phy_status), 200,
>> @@ -357,9 +1103,36 @@ static int qmp_pcie_power_on(struct phy *phy)
>>  static int qmp_pcie_power_off(struct phy *phy)
>>  {
>>  	struct qmp_pcie *qmp = phy_get_drvdata(phy);
>> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
>> +	const struct qmp_pcie_offsets *offs = cfg->offsets;
>> +	int i;
>>  
>>  	clk_bulk_disable_unprepare(qmp->cfg->num_pipe_clks, qmp->pipe_clks);
>>  
>> +	/*
>> +	 * While powering off the PHY, only nocsr_resets needs to be checked.
>> +	 * This way, no matter whether the PHY settings were initially
>> +	 * programmed by the bootloader or the PHY driver itself, we can
>> +	 * reuse them the next time the PHY is powered on.
>> +	 */
>> +	if (cfg->pll_tbls && !qmp->nocsr_resets) {
> Every cfg in the file (Glymur and all seven Nord ones) declares
> nocsr_reset_list, and qmp_pcie_reset_init() only leaves qmp->nocsr_resets
> NULL when num_nocsr_resets == 0. So the condition is always false, no?
Thats true for current targets, but on future targets it might not be true.
I will remove the check here for now, in future if we have any target with
nocsr then
we can add it back.

Ack for rest of the comments.

- Krishna Chaitanya.