Current code uses custom-defined register offsets
and bitfields for standard PCIe registers. Change
to using standard PCIe defines.
Suggested-By: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
---
drivers/pci/controller/pcie-rockchip-host.c | 44 ++++++++++-----------
1 file changed, 22 insertions(+), 22 deletions(-)
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:
--
2.49.0On 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
On Fri, Jun 13, 2025 at 03:14:09PM -0500, Bjorn Helgaas wrote: > On Fri, Jun 13, 2025 at 12:05:31PM -0300, Geraldo Nascimento wrote: > > - 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. Hi Bjorn, unfortunately Rockchip PCIe IP does not support 16-bit accesses, I tried and it only rendered the kernel unbootable, which made people in my house angry since the RK3399 box is my Internet Gateway! :-) For thit particular case, it is OK since LABS and LBMS are precisely the only RW1C bits in LNKSTA as far as I know. But see below. > > > > - 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. > This is a bit more concerning then above. I'm out of ideas regarding this particular issue you raised. Geraldo Nascimento > Bjorn
On Fri, Jun 13, 2025 at 03:14:09PM -0500, Bjorn Helgaas wrote: > On Fri, Jun 13, 2025 at 12:05:31PM -0300, Geraldo Nascimento wrote: > > - 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. Hi Bjorn and thank you for the review, while your rationale is correct per PCIe spec, per RK3399 TRM those registers are indeed 32 bits in the Rockchip-IP PCIe, so I'm forced to work with that, but without fear that other registers get messed-up. (See for example Section 17.6.6.1.30 of RK3399 TRM, Part 2) Regards, Geraldo Nascimento
On Fri, Jun 13, 2025 at 05:26:46PM -0300, Geraldo Nascimento wrote: > On Fri, Jun 13, 2025 at 03:14:09PM -0500, Bjorn Helgaas wrote: > > On Fri, Jun 13, 2025 at 12:05:31PM -0300, Geraldo Nascimento wrote: > > > - 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. > > Hi Bjorn and thank you for the review, > > while your rationale is correct per PCIe spec, per RK3399 TRM > those registers are indeed 32 bits in the Rockchip-IP PCIe, so > I'm forced to work with that, but without fear that other > registers get messed-up. (See for example Section 17.6.6.1.30 > of RK3399 TRM, Part 2) I don't have access to any of these TRMs, so I only know what's in the driver. When you say "without fear", are you saying there's a way to do that 32-bit write such that the LNKSTA bits are discarded by the hardware? Or just that the hardware forces us to accept this potential status register corruption? Is this something that could be written using the config access path? I guess probably not, based on this: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-rockchip-host.c?id=v6.15#n141 Bjorn
On Fri, Jun 13, 2025 at 03:50:23PM -0500, Bjorn Helgaas wrote: > I don't have access to any of these TRMs, so I only know what's in the > driver. > They are not under NDA and can be obtained though Rockchip's official site: https://rockchip.fr/Rockchip%20RK3399%20TRM%20V1.3%20Part2.pdf > When you say "without fear", are you saying there's a way to do that > 32-bit write such that the LNKSTA bits are discarded by the hardware? > Or just that the hardware forces us to accept this potential status > register corruption? I meant to say those registers themselves are defined in TRM as 32 bits. > > Is this something that could be written using the config access path? > I guess probably not, based on this: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-rockchip-host.c?id=v6.15#n141 > That certainly looks frightening. > Bjorn
On Fri, Jun 13, 2025 at 06:01:40PM -0300, Geraldo Nascimento wrote: > They are not under NDA and can be obtained though Rockchip's > official site: > https://rockchip.fr/Rockchip%20RK3399%20TRM%20V1.3%20Part2.pdf Hm, sorry about the confusion Bjorn, that is not an official website at all it seems, so I assume these are leaked? If so, it's a real pity, there's not particularly confidential about these docs, and they are essential for working with Rockchip chips. Sorry, Geraldo Nascimento
© 2016 - 2025 Red Hat, Inc.