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

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Nov 19, 2025 at 07:49:06PM +0530, Anand Moon wrote:
> On Tue, 18 Nov 2025 at 23:20, Bjorn Helgaas <helgaas@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.

Bjorn