Re: [RESEND RFC PATCH v4 1/5] PCI: rockchip: Use standard PCIe defines

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Jun 13, 2025 at 12:05:31PM -0300, Geraldo Nascimento wrote:
> Current code uses custom-defined register offsets
> and bitfields for standard PCIe registers. Change
> to using standard PCIe defines.

Wrap to fill 75 columns so there's space for "git log" to add
indentation.

> @@ -40,18 +40,18 @@ static void rockchip_pcie_enable_bw_int(struct rockchip_pcie *rockchip)
>  {
>  	u32 status;
>  
> -	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> +	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
>  	status |= (PCI_EXP_LNKCTL_LBMIE | PCI_EXP_LNKCTL_LABIE);
> -	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
> +	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
>  }
>  
>  static void rockchip_pcie_clr_bw_int(struct rockchip_pcie *rockchip)
>  {
>  	u32 status;
>  
> -	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> +	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
>  	status |= (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_LABS) << 16;

It looks funny to write PCI_EXP_LNKCTL with bits from PCI_EXP_LNKSTA.
I guess this is because rockchip_pcie_write() does 32-bit writes, but
PCI_EXP_LNKCTL and PCI_EXP_LNKSTA are adjacent 16-bit registers.

If the hardware supports it, adding rockchip_pcie_readw() and
rockchip_pcie_writew() for 16-bit accesses would make this read
better.

Hopefully the hardware *does* support this (it's required per spec at
least for config accesses, which would be a different path in the
hardware).  Doing the 32-bit write of PCI_EXP_LNKCTL above is
problematic because writes PCI_EXP_LNKSTA as well, and PCI_EXP_LNKSTA
includes some RW1C bits that may be unintentionally cleared.

> -	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
> +	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
>  }
>  
>  static void rockchip_pcie_update_txcredit_mui(struct rockchip_pcie *rockchip)
> @@ -269,7 +269,7 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
>  	scale = 3; /* 0.001x */
>  	curr = curr / 1000; /* convert to mA */
>  	power = (curr * 3300) / 1000; /* milliwatt */
> -	while (power > PCIE_RC_CONFIG_DCR_CSPL_LIMIT) {
> +	while (power > FIELD_MAX(PCI_EXP_DEVCAP_PWR_VAL)) {
>  		if (!scale) {
>  			dev_warn(rockchip->dev, "invalid power supply\n");
>  			return;
> @@ -278,10 +278,10 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
>  		power = power / 10;
>  	}
>  
> -	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DCR);
> -	status |= (power << PCIE_RC_CONFIG_DCR_CSPL_SHIFT) |
> -		  (scale << PCIE_RC_CONFIG_DCR_CPLS_SHIFT);
> -	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCR);
> +	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCAP);
> +	status |= FIELD_PREP(PCI_EXP_DEVCAP_PWR_VAL, power);
> +	status |= FIELD_PREP(PCI_EXP_DEVCAP_PWR_SCL, scale);

This assumes the value you read from PCI_EXP_DEVCAP had zeroes in
these bits.  It might, but it would look safer to do:

  status &= ~(PCI_EXP_DEVCAP_PWR_VAL | PCI_EXP_DEVCAP_PWR_SCL);
  status |= FIELD_PREP(PCI_EXP_DEVCAP_PWR_VAL, power);
  status |= FIELD_PREP(PCI_EXP_DEVCAP_PWR_SCL, scale);

> +	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCAP);
>  }

>  /**
> @@ -309,14 +309,14 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
>  	rockchip_pcie_set_power_limit(rockchip);
>  
>  	/* Set RC's clock architecture as common clock */
> -	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> +	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
>  	status |= PCI_EXP_LNKSTA_SLC << 16;
> -	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
> +	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
>  
>  	/* Set RC's RCB to 128 */
> -	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> +	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
>  	status |= PCI_EXP_LNKCTL_RCB;
> -	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
> +	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
>  
>  	/* Enable Gen1 training */
>  	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
> @@ -341,9 +341,9 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
>  		 * Enable retrain for gen2. This should be configured only after
>  		 * gen1 finished.
>  		 */
> -		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
> +		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
>  		status |= PCI_EXP_LNKCTL_RL;
> -		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
> +		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
>  
>  		err = readl_poll_timeout(rockchip->apb_base + PCIE_CORE_CTRL,
>  					 status, PCIE_LINK_IS_GEN2(status), 20,
> @@ -380,15 +380,15 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
>  
>  	/* Clear L0s from RC's link cap */
>  	if (of_property_read_bool(dev->of_node, "aspm-no-l0s")) {
> -		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LINK_CAP);
> -		status &= ~PCIE_RC_CONFIG_LINK_CAP_L0S;
> -		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LINK_CAP);
> +		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP);
> +		status &= ~PCI_EXP_LNKCAP_ASPM_L0S;
> +		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP);
>  	}
>  
> -	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DCSR);
> -	status &= ~PCIE_RC_CONFIG_DCSR_MPS_MASK;
> -	status |= PCIE_RC_CONFIG_DCSR_MPS_256;
> -	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCSR);
> +	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCTL);
> +	status &= ~PCI_EXP_DEVCTL_PAYLOAD;
> +	status |= PCI_EXP_DEVCTL_PAYLOAD_256B;
> +	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCTL);

Similar problem here; PCI_EXP_DEVCTL is only 16 bits, and writing the
adjacent PCI_EXP_DEVSTA may clear RW1C bits you didn't want to clear.

Bjorn