| 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
| ||
|
Message-ID: <CANAwSgRHuwoQjr95sXp-X97=L-X3vqUPxjR5=2jNtFZA+4gnwQ@mail.gmail.com>
Date: Wed, 19 Nov 2025 19:49:06 +0530
From: Anand Moon <linux.amoon@...il.com>
To: Bjorn Helgaas <helgaas@...nel.org>
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
Hi Bjorn / Shwan
Thanks for your review comments.
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.
> >
> > Additionally, this commit explicitly enables ASPM (Active State Power
> > Management) control and the CLKREQ# (Clock Request) mechanism as part of
> > the Link Control register programming when enabling bandwidth
> > notifications.
>
> 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.
>
Ok, got that.
> What I'd really like to see is at least two patches here: one that
> clearly makes no functional change -- don't try to fix anything, just
> make it 100% obvious that all the offsets stay the same. Then make a
> separate patch that *only* changes any of the offsets that are wrong.
>
Ok, understood.
> I don't think there should even be an ASPM change. The PCI core
> should be enabling L0s and L1 itself for DT systems like this. And
> ASPM needs to be enabled only when both ends of the link support it,
> and only in a specific order. The PCI core pays attention to that,
> but this patch does not.
>
Ok, I get it.
> Bjorn
>
> [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.
Thanks
-Anand
Here is the example. Is this the correct approach?
---
drivers/pci/controller/pcie-rockchip-host.c | 12 ++++++------
drivers/pci/controller/pcie-rockchip.h | 2 ++
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/controller/pcie-rockchip-host.c
b/drivers/pci/controller/pcie-rockchip-host.c
index ee1822ca01db..af438d59e788 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -32,18 +32,18 @@ static void rockchip_pcie_enable_bw_int(struct
rockchip_pcie *rockchip)
{
u32 status;
- 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);
}
static void rockchip_pcie_clr_bw_int(struct rockchip_pcie *rockchip)
{
u32 status;
- status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR +
PCI_EXP_LNKCTL);
+ status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LC);
status |= (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_LABS) << 16;
- rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR +
PCI_EXP_LNKCTL);
+ rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LC);
}
static void rockchip_pcie_update_txcredit_mui(struct rockchip_pcie *rockchip)
@@ -306,9 +306,9 @@ static int rockchip_pcie_host_init_port(struct
rockchip_pcie *rockchip)
rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR +
PCI_EXP_LNKCTL);
/* Set RC's RCB to 128 */
- 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_RCB;
- rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR +
PCI_EXP_LNKCTL);
+ rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LC);
/* Enable Gen1 training */
rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
diff --git a/drivers/pci/controller/pcie-rockchip.h
b/drivers/pci/controller/pcie-rockchip.h
index 3e82a69b9c00..3313cd8c585f 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -157,6 +157,8 @@
#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)
+/* Link Control and Status Register */
+#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)
#define PCIE_RC_CONFIG_THP_CAP_NEXT_MASK GENMASK(31, 20)
Powered by blists - more mailing lists