Re: [PATCH v2 2/2] phy: spacemit: Add USB3/PCIe comb PHY driver for Spacemit K3

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware register clobbering due to missing read-modify-write operations in PHY initialization.
--

commit c71cb5c4ebfcb4fc5aff5c18a7769949e15dae55
Author: Inochi Amaoto <inochiama@gmail.com>

phy: spacemit: Add USB3/PCIe comb PHY driver for Spacemit K3

Adds a USB3/PCIe comb PHY driver for Spacemit K3. The comb PHY requires
configuring a syscon device for the correct mux configuration and needs
calibration before usage.

> diff --git a/drivers/phy/spacemit/phy-k3-common.c b/drivers/phy/spacemit/phy-k3-common.c
> new file mode 100644
> index 0000000000000..0b289e63db999
> --- /dev/null
> +++ b/drivers/phy/spacemit/phy-k3-common.c

[ ... ]

> +	/* Power down 100MHz refclk buffer */
> +	val = readl(base + PHY_PU_CK_REG);
> +	val = u32_replace_bits(val, 0, PHY_PU_REFCLK_100);
> +	writel(val, base + PHY_PU_CK_REG);
> +
> +	/* Program PLL REG1 configure the SSC */
> +	val = FIELD_PREP(PHY_PLL_SSC_MODE, PHY_PLL_SSC_MODE_DOWN_SPREAD1) |
> +	      FIELD_PREP(PHY_PLL_SSC_DEP_SEL, PHY_PLL_SSC_5000PPM) |
> +	      FIELD_PREP(PHY_PLL_FREF_SEL, PHY_PLL_FREF_24M);
> +	writel(val, base + PHY_PLL_REG1);

[Severity: High]
Does this completely overwrite PHY_PLL_REG1 and zero out unspecified bits?

Unlike the PCIe initialization path which uses a read-modify-write for this
register, this direct writel() in k3_usb3phy_init_single() clobbers the
entire register. Could this cause important default settings or calibration
values to be lost?

[ ... ]

> +	for (i = 0; i < lg->data->lanes; i++) {
> +		void __iomem *lane_base = lg->base + lg->data->offsets[i];
> +
> +		val = readl(lane_base + PCIE_RX_REG1);
> +		val = u32_replace_bits(val, 0, 0x3);
> +		writel(val, lane_base + PCIE_RX_REG1);
> +	}
> +
> +	val = readl(phy_base + PHY_PLL_REG2);
> +	val |= BIT(20);
> +	writel(val, phy_base + PHY_PLL_REG2);
> +
> +	/* The write is needed as clock requires renegotiation */
> +	val = FIELD_PREP(PCIE_RX_REFCLK_MODE, PCIE_RX_REFCLK_MODE_DRIVER) |
> +	      PCIE_RX_SEL_TRI_CODE |
> +	      FIELD_PREP(PCIE_RX_LEGACY, PCIE_RX_LEGACY_DEFAULT);
> +	writel(val, phy_base + PCIE_RX_REG1);

[Severity: High]
Does this clobber the entire PCIE_RX_REG1 register for lane 0?

The preceding loop performs a read-modify-write on PCIE_RX_REG1 for all lanes,
but this direct writel() on phy_base (which points to lane 0) invalidates
that previous work and zeroes out any bits not explicitly set here.

Should this also use a read-modify-write operation to preserve the register
state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703021024.495433-1-inochiama@gmail.com?part=2