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

[Inochi Amaoto <inochiama@gmail.com>]

On Fri, Jul 03, 2026 at 02:19:28AM +0000, sashiko-bot@kernel.org wrote:
> 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?
> 

No, All the field are filled, so no data is lost.

> 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?
> 

This also serves as the whole clock. If the phy has two lanes, they share some
data and the data is store at 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?
> 

No, it is not needed, as all the field are filled or it should be zero.

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