On Fri, Jun 13, 2025 at 03:14:09PM -0500, Bjorn Helgaas wrote: > On Fri, Jun 13, 2025 at 12:05:31PM -0300, Geraldo Nascimento wrote: > > - status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS); > > + status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL); > > status |= (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_LABS) << 16; > > It looks funny to write PCI_EXP_LNKCTL with bits from PCI_EXP_LNKSTA. > I guess this is because rockchip_pcie_write() does 32-bit writes, but > PCI_EXP_LNKCTL and PCI_EXP_LNKSTA are adjacent 16-bit registers. > > If the hardware supports it, adding rockchip_pcie_readw() and > rockchip_pcie_writew() for 16-bit accesses would make this read > better. > > Hopefully the hardware *does* support this (it's required per spec at > least for config accesses, which would be a different path in the > hardware). Doing the 32-bit write of PCI_EXP_LNKCTL above is > problematic because writes PCI_EXP_LNKSTA as well, and PCI_EXP_LNKSTA > includes some RW1C bits that may be unintentionally cleared. Hi Bjorn, unfortunately Rockchip PCIe IP does not support 16-bit accesses, I tried and it only rendered the kernel unbootable, which made people in my house angry since the RK3399 box is my Internet Gateway! :-) For thit particular case, it is OK since LABS and LBMS are precisely the only RW1C bits in LNKSTA as far as I know. But see below. > > > > - status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DCSR); > > - status &= ~PCIE_RC_CONFIG_DCSR_MPS_MASK; > > - status |= PCIE_RC_CONFIG_DCSR_MPS_256; > > - rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCSR); > > + status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCTL); > > + status &= ~PCI_EXP_DEVCTL_PAYLOAD; > > + status |= PCI_EXP_DEVCTL_PAYLOAD_256B; > > + rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCTL); > > Similar problem here; PCI_EXP_DEVCTL is only 16 bits, and writing the > adjacent PCI_EXP_DEVSTA may clear RW1C bits you didn't want to clear. > This is a bit more concerning then above. I'm out of ideas regarding this particular issue you raised. Geraldo Nascimento > Bjorn
