Message-ID: <aPAXRiGA8aTZCNTm@aurel32.net>
Date: Wed, 15 Oct 2025 23:51:02 +0200
From: Aurelien Jarno <aurelien@...el32.net>
To: Alex Elder <elder@...cstar.com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
	bhelgaas@...gle.com, lpieralisi@...nel.org, kwilczynski@...nel.org,
	mani@...nel.org, vkoul@...nel.org, kishon@...nel.org,
	dlan@...too.org, guodong@...cstar.com, pjw@...nel.org,
	palmer@...belt.com, aou@...s.berkeley.edu, alex@...ti.fr,
	p.zabel@...gutronix.de, christian.bruel@...s.st.com,
	shradha.t@...sung.com, krishna.chundru@....qualcomm.com,
	qiang.yu@....qualcomm.com, namcao@...utronix.de,
	thippeswamy.havalige@....com, inochiama@...il.com,
	devicetree@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-phy@...ts.infradead.org, spacemit@...ts.linux.dev,
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Junzhong Pan <panjunzhong@...ux.spacemit.com>
Subject: Re: [PATCH v2 4/7] phy: spacemit: introduce PCIe/combo PHY

Hi,

On 2025-10-13 10:35, Alex Elder wrote:
> Introduce a driver that supports three PHYs found on the SpacemiT
> K1 SoC.  The first PHY is a combo PHY that can be configured for
> use for either USB 3 or PCIe.  The other two PHYs support PCIe
> only.
> 
> All three PHYs must be programmed with an 8 bit receiver termination
> value, which must be determined dynamically.  Only the combo PHY is
> able to determine this value.  The combo PHY performs a special
> calibration step at probe time to discover this, and that value is
> used to program each PHY that operates in PCIe mode.  The combo
> PHY must therefore be probed before either of the PCIe-only PHYs
> will be used.
> 
> Each PHY has an internal PLL driven from an external oscillator.
> This PLL started when the PHY is first initialized, and stays
> on thereafter.
> 
> During normal operation, the USB or PCIe driver using the PHY must
> ensure (other) clocks and resets are set up properly.
> 
> However PCIe mode clocks are enabled and resets are de-asserted
> temporarily by this driver to perform the calibration step on the
> combo PHY.
> 
> Tested-by: Junzhong Pan <panjunzhong@...ux.spacemit.com>
> Signed-off-by: Alex Elder <elder@...cstar.com>

Thanks for this new version. I have tried it on top of v6.18-rc1 + 
spacemit DTS commits from next on a BPI-F3, and it fails calibrating the 
PHY with:

[    2.748405] spacemit-k1-pcie-phy c0b10000.phy: error -ENOENT: calibration failed
[    2.755300] spacemit-k1-pcie-phy c0b10000.phy: error -ENOENT: error probing combo phy
[    2.763088] spacemit-k1-pcie-phy c0b10000.phy: probe with driver spacemit-k1-pcie-phy failed with error -2
[   14.309031] platform c0d10000.phy: deferred probe pending: (reason unknown)
[   14.313426] platform c0c10000.phy: deferred probe pending: (reason unknown)
[   14.320347] platform ca400000.pcie: deferred probe pending: platform: supplier c0c10000.phy not ready
[   14.329542] platform ca800000.pcie: deferred probe pending: platform: supplier c0d10000.phy not ready

Note that version 1 was working fine on the same board.

[ snip ]

> diff --git a/drivers/phy/phy-spacemit-k1-pcie.c b/drivers/phy/phy-spacemit-k1-pcie.c
> new file mode 100644
> index 0000000000000..81bc05823d080
> --- /dev/null
> +++ b/drivers/phy/phy-spacemit-k1-pcie.c

[ snip ]

> +static int k1_pcie_combo_phy_calibrate(struct k1_pcie_phy *k1_phy)
> +{
> +	struct reset_control_bulk_data resets[] = {
> +		{ .id = "dbi", },
> +		{ .id = "mstr", },
> +		{ .id = "slv", },
> +	};
> +	struct clk_bulk_data clocks[] = {
> +		{ .id = "dbi", },
> +		{ .id = "mstr", },
> +		{ .id = "slv", },
> +	};
> +	struct device *dev = k1_phy->dev;
> +	struct reset_control *phy_reset;
> +	int ret = 0;
> +	int val;
> +
> +	/* Nothing to do if we already set the receiver termination value */
> +	if (k1_phy_rterm_valid())
> +		return 0;
> +
> +	/* De-assert the PHY (global) reset and leave it that way for USB */
> +	phy_reset = devm_reset_control_get_exclusive_deasserted(dev, "phy");
> +	if (IS_ERR(phy_reset))
> +		return PTR_ERR(phy_reset);
> +
> +	/*
> +	 * We also guarantee the APP_HOLD_PHY_RESET bit is clear.  We can
> +	 * leave this bit clear even if an error happens below.
> +	 */
> +	regmap_assign_bits(k1_phy->pmu, PCIE_CLK_RES_CTRL,
> +			   PCIE_APP_HOLD_PHY_RST, false);
> +
> +	/* If the calibration already completed (e.g. by U-Boot), we're done */
> +	val = readl(k1_phy->regs + PCIE_RCAL_RESULT);
> +	if (val & R_TUNE_DONE)
> +		goto out_tune_done;
> +
> +	/* Put the PHY into PCIe mode */
> +	k1_combo_phy_sel(k1_phy, false);
> +
> +	/* Get and enable the PCIe app clocks */
> +	ret = clk_bulk_get(dev, ARRAY_SIZE(clocks), clocks);
> +	if (ret <= 0) {
> +		if (!ret)
> +			ret = -ENOENT;
> +		goto out_tune_done;
> +	}

This part doesn't look correct. The documentation says this function 
"returns 0 if all clocks specified in clk_bulk_data table are obtained
successfully, or valid IS_ERR() condition containing errno."

To me, it seems the code should only be:

	ret = clk_bulk_get(dev, ARRAY_SIZE(clocks), clocks);
	if (ret)
		goto out_tune_done;

[snip]

> +out_put_clocks:
> +	clk_bulk_put_all(ARRAY_SIZE(clocks), clocks);

When fixing the above bug, this then crashes with:

[    2.776109] Unable to handle kernel paging request at virtual address ffffffc41a0110c8
[    2.783958] Current kworker/u36:0 pgtable: 4K pagesize, 39-bit VAs, pgdp=0x00000000022a7000
[    2.792302] [ffffffc41a0110c8] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
[    2.800980] Oops [#1]
[    2.803217] Modules linked in:
[    2.806261] CPU: 3 UID: 0 PID: 58 Comm: kworker/u36:0 Not tainted 6.18.0-rc1+ #4 PREEMPTLAZY 
[    2.814763] Hardware name: Banana Pi BPI-F3 (DT)
[    2.819366] Workqueue: events_unbound deferred_probe_work_func
[    2.825180] epc : virt_to_folio+0x5e/0xb8
[    2.829172]  ra : kfree+0x3a/0x528
[    2.832558] epc : ffffffff8034e12e ra : ffffffff8035557a sp : ffffffc600243980
[    2.839762]  gp : ffffffff82074258 tp : ffffffd700994d80 t0 : ffffffff80021540
[    2.846967]  t1 : 0000000000000018 t2 : 2d74696d65636170 s0 : ffffffc600243990
[    2.854172]  s1 : ffffffc600243ab8 a0 : 03ffffc41a0110c0 a1 : ffffffff82123bd0
[    2.861377]  a2 : 7c137c69131cec36 a3 : ffffffff816606d8 a4 : 0000000000000000
[    2.868583]  a5 : ffffffc500000000 a6 : 0000000000000004 a7 : 0000000000000004
[    2.875787]  s2 : ffffffd700b98410 s3 : ffffffc600243ab8 s4 : 0000000000000000
[    2.882991]  s5 : ffffffff80828f1c s6 : 0000000000008437 s7 : ffffffd700b98410
[    2.890197]  s8 : ffffffd700b98410 s9 : ffffffd700900240 s10: ffffffff81fc4100
[    2.897401]  s11: ffffffd700987400 t3 : 0000000000000004 t4 : 0000000000000001
[    2.904607]  t5 : 000000000000001f t6 : 0000000000000003
[    2.909902] status: 0000000200000120 badaddr: ffffffc41a0110c8 cause: 000000000000000d
[    2.917802] [<ffffffff8034e12e>] virt_to_folio+0x5e/0xb8
[    2.923097] [<ffffffff8035557a>] kfree+0x3a/0x528
[    2.927784] [<ffffffff80828f1c>] clk_bulk_put_all+0x64/0x78
[    2.933340] [<ffffffff807249d6>] k1_pcie_phy_probe+0x4ee/0x618
[    2.939155] [<ffffffff808e35e6>] platform_probe+0x56/0x98
[    2.944538] [<ffffffff808e0328>] really_probe+0xa0/0x348
[    2.949832] [<ffffffff808e064c>] __driver_probe_device+0x7c/0x140
[    2.955909] [<ffffffff808e07f8>] driver_probe_device+0x38/0xd0
[    2.961724] [<ffffffff808e0912>] __device_attach_driver+0x82/0xf0
[    2.967801] [<ffffffff808dde6a>] bus_for_each_drv+0x72/0xd0
[    2.973356] [<ffffffff808e0cac>] __device_attach+0x94/0x198
[    2.978912] [<ffffffff808e0fca>] device_initial_probe+0x1a/0x30
[    2.984815] [<ffffffff808defee>] bus_probe_device+0x96/0xa0
[    2.990370] [<ffffffff808dff0e>] deferred_probe_work_func+0xa6/0x110
[    2.996707] [<ffffffff8005cb66>] process_one_work+0x15e/0x340
[    3.002436] [<ffffffff8005d58c>] worker_thread+0x22c/0x348
[    3.007905] [<ffffffff80066b7c>] kthread+0x10c/0x208
[    3.012853] [<ffffffff80014de0>] ret_from_fork_kernel+0x18/0x1c0
[    3.018843] [<ffffffff80c917d6>] ret_from_fork_kernel_asm+0x16/0x18
[    3.025098] Code: 7a98 8d19 2717 0131 3703 5fa7 8131 8d19 051a 953e (651c) f713 
[    3.032497] ---[ end trace 0000000000000000 ]---

It seems that we want clk_bulk_put() and not clk_bulk_put_all(). The 
latter free the clocks, while they have been allocated on the stack.

Regards,
Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@...el32.net                     http://aurel32.net

Powered by blists - more mailing lists