[RFC v1 1/5] PCI: rockchip: Fix Link Control register offset and enable ASPM/CLKREQ

[Anand Moon]

Hi Bjorn,

Thanks for your input.

On Thu, 20 Nov 2025 at 09:14, Bjorn Helgaas <helgaas at kernel.org> wrote:
>
> On Wed, Nov 19, 2025 at 07:49:06PM +0530, Anand Moon wrote:
> > On Tue, 18 Nov 2025 at 23:20, Bjorn Helgaas <helgaas at kernel.org> wrote:
> > > On Mon, Nov 17, 2025 at 11:40:09PM +0530, Anand Moon wrote:
> > > > As per the RK3399 TRM (Part 2, 17.6.6.1.31), the Link Control register
> > > > (RC_CONFIG_LC) resides at an offset of 0xd0 within the Root Complex (RC)
> > > > configuration space, not at the offset of the PCI Express Capability List
> > > > (0xc0). Following changes correct the register offset to use
> > > > PCIE_RC_CONFIG_LC (0xd0) to configure link control.
> > ...
>
> > > Don't do two things at once in the same patch.  Fix the register
> > > offset in one patch.  Actually, as I mentioned at [1], there's a lot
> > > of fixing to do there, and I'm not even going to consider other
> > > changes until the #define mess is cleaned up.
>
> > > [1] https://lore.kernel.org/r/20251118005056.GA2541796@bhelgaas
> >
> > According to the RK3399 Technical Reference Manual (TRM), and pci_regs.h
> > already includes the correct, pre-defined offsets for all PCI Express
> > device types
> > and their capabilities registers. To avoid overlapping register mappings,
> > we should explicitly remove the addition of manual offsets within the code.
>
> > Here is the example. Is this the correct approach?
>
> > -       status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR +
> > PCI_EXP_LNKCTL);
> > +       status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LC);
> >         status |= (PCI_EXP_LNKCTL_LBMIE | PCI_EXP_LNKCTL_LABIE);
> > -       rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR +
> > PCI_EXP_LNKCTL);
> > +       rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LC);
>
> No.  The call should include PCI_EXP_LNKCTL because that's what we
> grep for when we want to see where Link Control is updated.
>
> See my example from [1] above:
>
>   rockchip_pcie_read(rockchip, ROCKCHIP_RP_PCIE_CAP + PCI_EXP_DEVCAP)
>   rockchip_pcie_read(rockchip, ROCKCHIP_RP_PCIE_CAP + PCI_EXP_LNKCTL)
>
> You should have a single #define for the offset of the PCIe
> Capability, e.g., ROCKCHIP_RP_PCIE_CAP.  Every access to registers in
> that capability would use ROCKCHIP_RP_PCIE_CAP and the relevant
> PCI_EXP_* offset, e.g., PCI_EXP_DEVCAP, PCI_EXP_DEVCTL,
> PCI_EXP_DEVSTA, PCI_EXP_LNKCAP, PCI_EXP_LNKCTL, PCI_EXP_LNKSTA, etc.
>
I apologize for not fully understanding the issue earlier. I did not
carefully review your email,
 which caused me to overlook the required changes.

So, as per the TRM

17.6.4.5.1 PCI Express Capability List Register
Propname：PCI Express Capability List Register
Address：@0xc0

#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_CR               (PCIE_RC_CONFIG_BASE + 0xc0)
#define ROCKCHIP_RP_PCIE_CAP            (PCIE_RC_CONFIG_BASE + 0xc0)
/* Link Control and Status Register */
#define PCIE_RC_CONFIG_LC               (ROCKCHIP_RP_PCIE_CAP + 0xd0)
/* Device Control */
#define PCIE_RC_CONFIG_DC               (ROCKCHIP_RP_PCIE_CAP + 0xc8)
/* Slot Capability Register */
#define PCIE_RC_CONFIG_SC               (ROCKCHIP_RP_PCIE_CAP + 0xd4)
/* Link Control 2 Register */
#define PCIE_RC_CONFIG_LC2              (ROCKCHIP_RP_PCIE_CAP + 0xf0)
/* Linkwidth Control Register */
#define PCIE_RC_CONFIG_LWC              (ROCKCHIP_RP_PCIE_CAP + 0x50)

And then use these like this.

status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LC + PCI_EXP_LNKCTL);
status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DC + PCI_EXP_DEVCTL);
status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_SC + PCI_EXP_DEVCAP);
status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LC2 + PCI_EXP_LNKCTL2);
status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LWC + PCI_EXP_LNKCTL);

If you agree that this approach correctly resolves the issue,
I would prefer to confirm now to prevent further iterative changes
that might confuse.

> Bjorn

Thanks
-Anand