Re: [PATCH 2/3] phy: aspeed: Add ASPEED PCIe PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] phy: aspeed: Destructive overwrite of PEHR_GLOBAL and PEHR_MISC_5C registers
- [Low] build: missing <linux/bitfield.h> and <linux/io.h> causes COMPILE_TEST failures
--

commit d6c687af08f52ce8c5106883a0ada373528fe1b8
Author: Jacky Chou <jacky_chou@aspeedtech.com>

phy: aspeed: Add ASPEED PCIe PHY driver

This commit introduces the Aspeed PCIe PHY controller driver for
AST2600/2700 platforms. It adds Kconfig and Makefile changes alongside
the main driver code implementing PHY operations.

> diff --git a/drivers/phy/aspeed/phy-aspeed-pcie.c b/drivers/phy/aspeed/phy-aspeed-pcie.c
> new file mode 100644
> index 0000000000000..eb300887b6441
> --- /dev/null
> +++ b/drivers/phy/aspeed/phy-aspeed-pcie.c
> @@ -0,0 +1,237 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2025 Aspeed Technology Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/phy/pcie.h>
> +#include <linux/phy/phy.h>

[Severity: Low]
Can this cause build failures when COMPILE_TEST is enabled on architectures
that do not implicitly include <linux/bitfield.h> and <linux/io.h>?

The FIELD_PREP, FIELD_MODIFY, readl, and writel macros and functions are
used throughout this file, but their headers are not explicitly included.

[ ... ]
> +static int ast2600_phy_set_mode(struct phy *phy, enum phy_mode mode,
> +				int submode)
> +{
> +	struct aspeed_pcie_phy *pcie_phy = phy_get_drvdata(phy);
> +
> +	switch (submode) {
> +	case PHY_MODE_PCIE_RC:
> +		writel(AST2600_PORT_TYPE(0x3), pcie_phy->reg + PEHR_GLOBAL);

[Severity: Medium]
Could this write unintentionally clear other configuration bits in PEHR_GLOBAL?
It overwrites the entire register instead of performing a read-modify-write
operation.

> +		break;
> +	default:
> +		dev_err(&phy->dev, "Unsupported submode %d\n", submode);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[ ... ]
> +static int ast2700_phy_set_mode(struct phy *phy, enum phy_mode mode,
> +				int submode)
> +{
> +	struct aspeed_pcie_phy *pcie_phy = phy_get_drvdata(phy);
> +	u32 cfg_val;
> +
> +	switch (submode) {
> +	case PHY_MODE_PCIE_RC:
> +		writel(0, pcie_phy->reg + PEHR_MISC_278);
> +		writel(SEL_INTERNAL_RESET, pcie_phy->reg + PEHR_MISC_280);
> +		writel(POSTED_DATA_CREDITS(0xc0) | POSTED_HEADER_CREDITS(0xa),
> +		       pcie_phy->reg + PEHR_MISC_70);
> +		writel(COMPLETION_DATA_CREDITS(0x30) | COMPLETION_HEADER_CREDITS(0x8),
> +		       pcie_phy->reg + PEHR_MISC_78);
> +		writel(LOCAL_SCALE_SUP, pcie_phy->reg + PEHR_MISC_58);
> +		writel(CONFIG_RC_DEVICE, pcie_phy->reg + PEHR_MISC_5C);

[Severity: Medium]
Does writing CONFIG_RC_DEVICE directly to PEHR_MISC_5C risk zeroing out other
bits in this register?

A read-modify-write pattern might be needed here, similar to how PEHR_MISC_60
is handled just below this.

> +		cfg_val = readl(pcie_phy->reg + PEHR_MISC_60);
> +		FIELD_MODIFY(AST2700_PORT_TYPE_MASK, &cfg_val,
> +			     PORT_TYPE_ROOT);
> +		writel(cfg_val, pcie_phy->reg + PEHR_MISC_60);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-aspeed_pcie_phy-v1-0-96764937aef0@aspeedtech.com?part=2