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
