Re: [PATCH] pci: dwc: Rename PCIE_PORT_LINK_CONTROL to PORT_LINK_CTRL_OFF

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Sep 04, 2026 at 02:18:23PM +0800, Hans Zhang wrote:
> The register at offset 0x710 is documented in the Synopsys DesignWare
> PCIe Controller Reference Manual as "PORT_LINK_CTRL_OFF" (Port Link
> Control Register). The current macro name PCIE_PORT_LINK_CONTROL does
> not match the documentation, making it difficult to cross-reference
> with the spec when debugging or maintaining the code.

I'm a little skeptical about this change.  I agree it's good to match
the spec, and the leading "PCIE_" seems superfluous.  But I think it's
pointless to add "_OFF" to every register offset.  "PORT_LINK_CTRL"
seems like enough, and it's pretty close to the spec's name.

Adding "_OFF" also invites confusion: does it mean "offset" or "off"
(the opposite of "on")?

It's also nice if names of fields within a register include the
register name, e.g., PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_ASPMC, etc.
For long register names like PCIE_PORT_MULTI_LANE_CTRL, with a
PORT_MLTI_UPCFG_SUPPORT field, you have squint pretty hard to see the
connection between the register and the field, but a name like
"PCIE_PORT_MULTI_LANE_CTRL_UPCFG_SUPPORT" would be just ridiculous.

> Rename the macro to PORT_LINK_CTRL_OFF to align with the Synopsys
> documentation, and update all usage sites accordingly. No functional
> change is intended.
> 
> This improves code maintainability and eases future reference to the
> controller manual.
> 
> Co-developed-by: Feizhou Yu <yufeizhou201@163.com>
> Signed-off-by: Feizhou Yu <yufeizhou201@163.com>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 8 ++++----
>  drivers/pci/controller/dwc/pcie-designware.h | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 593388f29bdd..9a0d8f0be792 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -894,7 +894,7 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
>  		return;
>  
>  	/* Set the number of lanes */
> -	plc = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> +	plc = dw_pcie_readl_dbi(pci, PORT_LINK_CTRL_OFF);
>  	plc &= ~PORT_LINK_FAST_LINK_MODE;
>  	plc &= ~PORT_LINK_MODE_MASK;
>  
> @@ -922,7 +922,7 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes)
>  		dev_err(pci->dev, "num-lanes %u: invalid value\n", num_lanes);
>  		return;
>  	}
> -	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc);
> +	dw_pcie_writel_dbi(pci, PORT_LINK_CTRL_OFF, plc);
>  	dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc);
>  
>  	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> @@ -1295,10 +1295,10 @@ void dw_pcie_setup(struct dw_pcie *pci)
>  		dw_pcie_writel_dbi(pci, PCIE_PL_CHK_REG_CONTROL_STATUS, val);
>  	}
>  
> -	val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> +	val = dw_pcie_readl_dbi(pci, PORT_LINK_CTRL_OFF);
>  	val &= ~PORT_LINK_FAST_LINK_MODE;
>  	val |= PORT_LINK_DLL_LINK_EN;
> -	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
> +	dw_pcie_writel_dbi(pci, PORT_LINK_CTRL_OFF, val);
>  
>  	dw_pcie_link_set_max_link_width(pci, pci->num_lanes);
>  }
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 0735ae940924..77420230433b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -87,7 +87,7 @@
>  #define PORT_AFR_L1_ENTRANCE_LAT_SHIFT	27
>  #define PORT_AFR_L1_ENTRANCE_LAT_MASK	GENMASK(29, 27)
>  
> -#define PCIE_PORT_LINK_CONTROL		0x710
> +#define PORT_LINK_CTRL_OFF		0x710
>  #define PORT_LINK_DLL_LINK_EN		BIT(5)
>  #define PORT_LINK_FAST_LINK_MODE	BIT(7)
>  #define PORT_LINK_MODE_MASK		GENMASK(21, 16)
> -- 
> 2.43.0
>