Re: [RESEND PATCH v9 1/4] PCI: rockchip: Use standard PCIe defines

Philipp Stanner
2025-07-01T08:00:08Z

On Mon, 2025-06-30 at 19:24 -0300, Geraldo Nascimento wrote:
> Current code uses custom-defined register offsets and bitfields for
> standard PCIe registers. Change to using standard PCIe defines. Since
> we are now using standard PCIe defines, drop unused custom-defined
> ones,
> which are now referenced from offset at added Capabilities Register.

This could be phrased a bit more cleanly. At least I don't get exactly
what "from offset" means. You mean you replace the unused custom ones?
But if they're unused, why are they even being replaced?

>
> Suggested-By: Bjorn Helgaas <bhelgaas@google.com>

s/By/by

P.

> Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
> ---
>  drivers/pci/controller/pcie-rockchip-ep.c   |  4 +-
>  drivers/pci/controller/pcie-rockchip-host.c | 44 ++++++++++---------
> --
>  drivers/pci/controller/pcie-rockchip.h      | 12 +-----
>  3 files changed, 25 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-rockchip-ep.c
> b/drivers/pci/controller/pcie-rockchip-ep.c
> index 55416b8311dd..300cd85fa035 100644
> --- a/drivers/pci/controller/pcie-rockchip-ep.c
> +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> @@ -518,9 +518,9 @@ static void rockchip_pcie_ep_retrain_link(struct
> rockchip_pcie *rockchip)
>  {
>   u32 status;
>
> - status = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_LCS);
> + status = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE +
> PCI_EXP_LNKCTL);
>   status |= PCI_EXP_LNKCTL_RL;
> - rockchip_pcie_write(rockchip, status, PCIE_EP_CONFIG_LCS);
> + rockchip_pcie_write(rockchip, status, PCIE_EP_CONFIG_BASE +
> PCI_EXP_LNKCTL);
>  }
>
>  static bool rockchip_pcie_ep_link_up(struct rockchip_pcie *rockchip)
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c
> b/drivers/pci/controller/pcie-rockchip-host.c
> index b9e7a8710cf0..65653218b9ab 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -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;
> - 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);
> + 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);
>
>   return 0;
>  err_power_off_phy:
> diff --git a/drivers/pci/controller/pcie-rockchip.h
> b/drivers/pci/controller/pcie-rockchip.h
> index 5864a20323f2..f5cbf3c9d2d9 100644
> --- a/drivers/pci/controller/pcie-rockchip.h
> +++ b/drivers/pci/controller/pcie-rockchip.h
> @@ -155,17 +155,7 @@
>  #define PCIE_EP_CONFIG_DID_VID (PCIE_EP_CONFIG_BASE + 0x00)
>  #define PCIE_EP_CONFIG_LCS (PCIE_EP_CONFIG_BASE + 0xd0)
>  #define PCIE_RC_CONFIG_RID_CCR (PCIE_RC_CONFIG_BASE + 0x08)
> -#define PCIE_RC_CONFIG_DCR (PCIE_RC_CONFIG_BASE + 0xc4)
> -#define   PCIE_RC_CONFIG_DCR_CSPL_SHIFT 18
> -#define   PCIE_RC_CONFIG_DCR_CSPL_LIMIT 0xff
> -#define   PCIE_RC_CONFIG_DCR_CPLS_SHIFT 26
> -#define PCIE_RC_CONFIG_DCSR (PCIE_RC_CONFIG_BASE + 0xc8)
> -#define   PCIE_RC_CONFIG_DCSR_MPS_MASK GENMASK(7, 5)
> -#define   PCIE_RC_CONFIG_DCSR_MPS_256 (0x1 << 5)
> -#define PCIE_RC_CONFIG_LINK_CAP (PCIE_RC_CONFIG_BASE
> + 0xcc)
> -#define   PCIE_RC_CONFIG_LINK_CAP_L0S BIT(10)
> -#define PCIE_RC_CONFIG_LCS (PCIE_RC_CONFIG_BASE + 0xd0)
> -#define PCIE_EP_CONFIG_LCS (PCIE_EP_CONFIG_BASE + 0xd0)
> +#define PCIE_RC_CONFIG_CR (PCIE_RC_CONFIG_BASE + 0xc0)
>  #define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE +
> 0x90c)
>  #define PCIE_RC_CONFIG_THP_CAP (PCIE_RC_CONFIG_BASE +
> 0x274)
>  #define   PCIE_RC_CONFIG_THP_CAP_NEXT_MASK GENMASK(31, 20)