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@xxxxxxxxxx>
s/By/by
P.
> Signed-off-by: Geraldo Nascimento <geraldogabriel@xxxxxxxxx>
> ---
> 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)
