Re: [RESEND RFC PATCH v4 1/5] PCI: rockchip: Use standard PCIe defines

From: Geraldo Nascimento <geraldogabriel@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-rockchip@lists.infradead.org,
	"Shawn Lin" <shawn.lin@rock-chips.com>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Manivannan Sadhasivam" <mani@kernel.org>,
	"Rob Herring" <robh@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Heiko Stuebner" <heiko@sntech.de>,
	"Vinod Koul" <vkoul@kernel.org>,
	"Kishon Vijay Abraham I" <kishon@kernel.org>,
	linux-phy@lists.infradead.org, linux-pci@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [RESEND RFC PATCH v4 1/5] PCI: rockchip: Use standard PCIe defines
Date: Fri, 13 Jun 2025 22:38:18 -0300	[thread overview]
Message-ID: <aEzSilET0wAQ7ozA@geday> (raw)
In-Reply-To: <20250613201409.GA973486@bhelgaas>

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