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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20251120034437.GA2625966@bhelgaas>
Date: Wed, 19 Nov 2025 21:44:37 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Anand Moon <linux.amoon@...il.com>
Cc: Shawn Lin <shawn.lin@...k-chips.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kwilczynski@...nel.org>,
	Manivannan Sadhasivam <mani@...nel.org>,
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
	Heiko Stuebner <heiko@...ech.de>,
	"open list:PCIE DRIVER FOR ROCKCHIP" <linux-pci@...r.kernel.org>,
	"open list:PCIE DRIVER FOR ROCKCHIP" <linux-rockchip@...ts.infradead.org>,
	"moderated list:ARM/Rockchip SoC support" <linux-arm-kernel@...ts.infradead.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC v1 1/5] PCI: rockchip: Fix Link Control register offset and
 enable ASPM/CLKREQ

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