1. Patchew
  2. linux
  3. [RFC v1 0/5] Fix some register offset as per RK3399 TRM part 2
  4. View patch

[RFC v1 2/5] PCI: rockchip: Fix Device Control register offset for Max payload size

Anand Moon posted 5 patches 2 months, 3 weeks ago
[RFC v1 2/5] PCI: rockchip: Fix Device Control register offset for Max payload size
Posted by Anand Moon 2 months, 3 weeks ago 
As per 17.6.6.1.29 PCI Express Device Capabilities Register
(PCIE_RC_CONFIG_DC) reside at offset 0xc8 within the Root Complex (RC)
configuration space, not at the offset of the PCI Express Capability
List (0xc0). Following changes corrects the register offset to use
PCIE_RC_CONFIG_DC (0xc8) to configure Max Payload Size.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 4 ++--
 drivers/pci/controller/pcie-rockchip.h      | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index f0de5b2590c4..d51780f4a254 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -382,10 +382,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP);
 	}
 
-	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCTL);
+	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DC + 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);
+	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DC + 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 5d8a3ae38599..c0ec6c32ea16 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -157,6 +157,7 @@
 #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_CR		(PCIE_RC_CONFIG_BASE + 0xc0)
+#define PCIE_RC_CONFIG_DC		(PCIE_RC_CONFIG_BASE + 0xc8)
 #define PCIE_RC_CONFIG_LC		(PCIE_RC_CONFIG_BASE + 0xd0)
 #define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)
 #define PCIE_RC_CONFIG_THP_CAP		(PCIE_RC_CONFIG_BASE + 0x274)
-- 
2.50.1
Re: [RFC v1 2/5] PCI: rockchip: Fix Device Control register offset for Max payload size
Posted by Bjorn Helgaas 2 months, 3 weeks ago 
On Mon, Nov 17, 2025 at 11:40:10PM +0530, Anand Moon wrote:
> As per 17.6.6.1.29 PCI Express Device Capabilities Register
> (PCIE_RC_CONFIG_DC) reside at offset 0xc8 within the Root Complex (RC)
> configuration space, not at the offset of the PCI Express Capability
> List (0xc0). Following changes corrects the register offset to use
> PCIE_RC_CONFIG_DC (0xc8) to configure Max Payload Size.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 4 ++--
>  drivers/pci/controller/pcie-rockchip.h      | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index f0de5b2590c4..d51780f4a254 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -382,10 +382,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
>  		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP);
>  	}
>  
> -	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCTL);
> +	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DC + 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);
> +	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DC + 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 5d8a3ae38599..c0ec6c32ea16 100644
> --- a/drivers/pci/controller/pcie-rockchip.h
> +++ b/drivers/pci/controller/pcie-rockchip.h
> @@ -157,6 +157,7 @@
>  #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_CR		(PCIE_RC_CONFIG_BASE + 0xc0)
> +#define PCIE_RC_CONFIG_DC		(PCIE_RC_CONFIG_BASE + 0xc8)

Per the RK3399 TRM:

  DEVCAP              0xc4
  DEVCTL and DEVSTA   0xc8
  LNKCAP              0xcc
  LNKCTL and LNKSTA   0xd0
  SLTCAP              0xd4
  SLTCTL and SLTSTA   0xd8

That all makes good sense and matches the relative offsets in the PCIe
Capability.

I have no idea how we got from that to the mind-bendingly obtuse
#defines here:

  PCIE_RC_CONFIG_CR == PCIE_RC_CONFIG_BASE + 0xc0
                    == 0xa00000 + 0xc0
                    == 0xa000c0

  PCIE_RC_CONFIG_DC == PCIE_RC_CONFIG_BASE + 0xc8
                    == 0xa00000 + 0xc8
                    == 0xa000c8

  PCIE_RC_CONFIG_LC == PCIE_RC_CONFIG_BASE + 0xd0
                    == 0xa00000 + 0xd0
                    == 0xa000d0

  PCIE_RC_CONFIG_SR == PCIE_RC_CONFIG_BASE + 0xd4
                    == 0xa00000 + 0xd4
                    == 0xa000d4

And they're used like this:

  PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP == 0xa000c0 + 0x0c
                                     == 0xa000cc

  PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL == 0xa000c0 + 0x10
                                     == 0xa000d0

  PCIE_RC_CONFIG_DC + PCI_EXP_DEVCTL == 0xa000c8 + 0x08
                                     == 0xa000d0     <-- same as LNKCTL? 

  PCIE_RC_CONFIG_SR + PCI_EXP_DEVCAP == 0xa000d4 + 0x04
                                     == 0xa000d8     <--

  PCIE_RC_CONFIG_LC + PCI_EXP_LNKCTL == 0xa000d0 + 0x10
                                     == 0xa000e0     <-- but LNKCTL was at 0xa000d0 above?

And the mappings don't make any sense to me:

  CR -> LNKCAP & LNKCTL
  DC -> DEVCTL (ok, this one makes a *little* sense)
  SR -> DEVCAP
  LC -> LNKCTL (would make some sense except that we have CR above)

This is all just really hard to read.  It looks like if we defined a
single base address for the PCIe Capability, we shouldn't need all the
_CR, _DC, _LC, _SR, etc #defines.  E.g., we could have

  #define ROCKCHIP_RP_PCIE_CAP ...

  rockchip_pcie_read(rockchip, ROCKCHIP_RP_PCIE_CAP + PCI_EXP_DEVCAP)
  rockchip_pcie_read(rockchip, ROCKCHIP_RP_PCIE_CAP + PCI_EXP_LNKCTL)
  ...

with maybe some minor adjustment for 16-bit registers since Rockchip
only seems to support 32-bit accesses (?)

None of these registers are *Root Complex* registers; they're all part
of a PCIe Capability, which applies to a Root *Port*.

>  #define PCIE_RC_CONFIG_LC		(PCIE_RC_CONFIG_BASE + 0xd0)
>  #define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)
>  #define PCIE_RC_CONFIG_THP_CAP		(PCIE_RC_CONFIG_BASE + 0x274)
> -- 
> 2.50.1
>
Re: [RFC v1 2/5] PCI: rockchip: Fix Device Control register offset for Max payload size
Posted by Anand Moon 2 months, 3 weeks ago 
Hi Bjorn

Thanks for your review comments.

On Tue, 18 Nov 2025 at 06:21, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Mon, Nov 17, 2025 at 11:40:10PM +0530, Anand Moon wrote:
> > As per 17.6.6.1.29 PCI Express Device Capabilities Register
> > (PCIE_RC_CONFIG_DC) reside at offset 0xc8 within the Root Complex (RC)
> > configuration space, not at the offset of the PCI Express Capability
> > List (0xc0). Following changes corrects the register offset to use
> > PCIE_RC_CONFIG_DC (0xc8) to configure Max Payload Size.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >  drivers/pci/controller/pcie-rockchip-host.c | 4 ++--
> >  drivers/pci/controller/pcie-rockchip.h      | 1 +
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> > index f0de5b2590c4..d51780f4a254 100644
> > --- a/drivers/pci/controller/pcie-rockchip-host.c
> > +++ b/drivers/pci/controller/pcie-rockchip-host.c
> > @@ -382,10 +382,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
> >               rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP);
> >       }
> >
> > -     status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCTL);
> > +     status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DC + 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);
> > +     rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DC + 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 5d8a3ae38599..c0ec6c32ea16 100644
> > --- a/drivers/pci/controller/pcie-rockchip.h
> > +++ b/drivers/pci/controller/pcie-rockchip.h
> > @@ -157,6 +157,7 @@
> >  #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_CR            (PCIE_RC_CONFIG_BASE + 0xc0)
> > +#define PCIE_RC_CONFIG_DC            (PCIE_RC_CONFIG_BASE + 0xc8)
>
> Per the RK3399 TRM:
>
>   DEVCAP              0xc4
>   DEVCTL and DEVSTA   0xc8
>   LNKCAP              0xcc
>   LNKCTL and LNKSTA   0xd0
>   SLTCAP              0xd4
>   SLTCTL and SLTSTA   0xd8
>
> That all makes good sense and matches the relative offsets in the PCIe
> Capability.
>
> I have no idea how we got from that to the mind-bendingly obtuse
> #defines here:
>
>   PCIE_RC_CONFIG_CR == PCIE_RC_CONFIG_BASE + 0xc0
>                     == 0xa00000 + 0xc0
>                     == 0xa000c0
>
>   PCIE_RC_CONFIG_DC == PCIE_RC_CONFIG_BASE + 0xc8
>                     == 0xa00000 + 0xc8
>                     == 0xa000c8
>
>   PCIE_RC_CONFIG_LC == PCIE_RC_CONFIG_BASE + 0xd0
>                     == 0xa00000 + 0xd0
>                     == 0xa000d0
>
>   PCIE_RC_CONFIG_SR == PCIE_RC_CONFIG_BASE + 0xd4
>                     == 0xa00000 + 0xd4
>                     == 0xa000d4
>
> And they're used like this:
>
>   PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP == 0xa000c0 + 0x0c
>                                      == 0xa000cc
>
>   PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL == 0xa000c0 + 0x10
>                                      == 0xa000d0
>
>   PCIE_RC_CONFIG_DC + PCI_EXP_DEVCTL == 0xa000c8 + 0x08
>                                      == 0xa000d0     <-- same as LNKCTL?
>
>   PCIE_RC_CONFIG_SR + PCI_EXP_DEVCAP == 0xa000d4 + 0x04
>                                      == 0xa000d8     <--
>
>   PCIE_RC_CONFIG_LC + PCI_EXP_LNKCTL == 0xa000d0 + 0x10
>                                      == 0xa000e0     <-- but LNKCTL was at 0xa000d0 above?
>
> And the mappings don't make any sense to me:
You are correct, PCIE_RC_CONFIG_DC + PCI_EXP_DEVCTL  results in an
incorrect offset.

As per my understanding
#define PCI_EXP_DEVCTL 0x08 /* Device Control */
#define  PCI_EXP_DEVCTL_PAYLOAD 0x00e0 /* Max_Payload_Size */
#define  PCI_EXP_DEVCTL_PAYLOAD_128B 0x0000 /* 128 Bytes */
#define  PCI_EXP_DEVCTL_PAYLOAD_256B 0x0020 /* 256 Bytes */
#define  PCI_EXP_DEVCTL_PAYLOAD_512B 0x0040 /* 512 Bytes */
#define  PCI_EXP_DEVCTL_PAYLOAD_1024B 0x0060 /* 1024 Bytes */

What I was thinking about was mapping it to a register to offset the
Max_Payload_Size (7:5) at PCIE_RC_CONFIG_DC (0xa000c8).

Sorry for the noise.
>
>   CR -> LNKCAP & LNKCTL
>   DC -> DEVCTL (ok, this one makes a *little* sense)
>   SR -> DEVCAP
>   LC -> LNKCTL (would make some sense except that we have CR above)
>
> This is all just really hard to read.  It looks like if we defined a
> single base address for the PCIe Capability, we shouldn't need all the
> _CR, _DC, _LC, _SR, etc #defines.  E.g., we could have
>
>   #define ROCKCHIP_RP_PCIE_CAP ...
#define ROCKCHIP_RP_PCIE_CAP (PCIE_RC_CONFIG_BASE + 0xc0)
Is this ok? But the current code uses the same.
>
>   rockchip_pcie_read(rockchip, ROCKCHIP_RP_PCIE_CAP + PCI_EXP_DEVCAP)
>   rockchip_pcie_read(rockchip, ROCKCHIP_RP_PCIE_CAP + PCI_EXP_LNKCTL)
>   ...
>
> with maybe some minor adjustment for 16-bit registers since Rockchip
> only seems to support 32-bit accesses (?)
>
> None of these registers are *Root Complex* registers; they're all part
> of a PCIe Capability, which applies to a Root *Port*.
>
Ok, I understood
> >  #define PCIE_RC_CONFIG_LC            (PCIE_RC_CONFIG_BASE + 0xd0)
> >  #define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)
> >  #define PCIE_RC_CONFIG_THP_CAP               (PCIE_RC_CONFIG_BASE + 0x274)
> > --
> > 2.50.1
> >
Thanks
-Anand

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.