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

From: Bjorn Helgaas <helgaas@kernel.org>
To: Anand Moon <linux.amoon@gmail.com>
Cc: "Shawn Lin" <shawn.lin@rock-chips.com>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kwilczynski@kernel.org>,
	"Manivannan Sadhasivam" <mani@kernel.org>,
	"Rob Herring" <robh@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Heiko Stuebner" <heiko@sntech.de>,
	"open list:PCIE DRIVER FOR ROCKCHIP" <linux-pci@vger.kernel.org>,
	"open list:PCIE DRIVER FOR ROCKCHIP"
	<linux-rockchip@lists.infradead.org>,
	"moderated list:ARM/Rockchip SoC support"
	<linux-arm-kernel@lists.infradead.org>,
	"open list" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC v1 1/5] PCI: rockchip: Fix Link Control register offset and enable ASPM/CLKREQ
Date: Wed, 19 Nov 2025 21:44:37 -0600	[thread overview]
Message-ID: <20251120034437.GA2625966@bhelgaas> (raw)
In-Reply-To: <CANAwSgRHuwoQjr95sXp-X97=L-X3vqUPxjR5=2jNtFZA+4gnwQ@mail.gmail.com>

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