On Thursday 06 October 2022 14:50:25 Thierry Reding wrote: > On Wed, Oct 05, 2022 at 09:43:36PM +0200, Pali Rohár wrote: > > On Wednesday 28 September 2022 16:38:27 Thierry Reding wrote: > > > On Sun, Sep 11, 2022 at 01:32:16PM +0200, Pali Rohár wrote: > > > > Like many other ARM PCIe controllers, it uses old PCI Configuration > > > > Mechanism #1 from PCI Local Bus for accessing PCI config space. > > > > It is not PCIe ECAM in any case. > > > > > > > > Signed-off-by: Pali Rohár <pali@xxxxxxxxxx> > > > > --- > > > > drivers/pci/controller/pci-tegra.c | 8 +++++--- > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > Perhaps this should be rolled into the PCI_CONF1_EXT_ADDRESS patch? > > > > Well, I split documentation change and PCI_CONF1_EXT_ADDRESS usage into > > two patches as those are two different / separate things. Documentation > > change is a fix (because documentation is wrong) and PCI_CONF1_EXT_ADDRESS > > is an improvement - code cleanup. And in case if there is a issue with > > "cleanup" patch it can be reverted without need to revert also "fix" > > part. This is just information how I looked at these changes and why I > > decided to split them. > > > > > On > > > the other hand there's really no use in keeping this comment around > > > after that other patch because the documentation for the new macro lays > > > out the details already. > > > > > > Thierry > > > > Ok, whether documentation is needed or not - it is your maintainer > > decision. Maybe really obvious things do not have to be documented. > > Also another look at this problem can be that if somebody wrote wrong > > documentation for it, maybe it is not too obvious? I do not have opinion > > on this, so choose what is better :-) > > I wrote that documentation back at the time and I fail to see what > exactly is wrong about it. Granted, it doesn't mention the Intel PCI > Configuration mechanism #1 from the PCI Local Bus Specification, but > that's just because I didn't know about it. Back when I wrote this I > was looking at the PCIe specifications (because, well, this supports > PCIe) and I noticed that it was similar to ECAM. And that's exactly > what the comment says and it points out what the differences are. > > So just because the mapping is closer to PCI_CONF1_EXT_ADDRESS than > ECAM, it doesn't automatically make the comment wrong. The mapping also > isn't exactly PCI_CONF1_EXT_ADDRESS, so the new comment can be > considered equally wrong. The mapping is neither ECAM nor PCI_CONF1, so > describing it one way or the other doesn't make a difference. PCI_CONF1_EXT_ADDRESS express indirect register access. If you look at the address space of Intel PCI Configuration Mechanism #1 then it is really what this ARM PCIe controller implements (plus uses additional bits for larger 4kB space). This is really common what lot of non-ECAM ARM SoC implements. It is really bad to mix this mapping with ECAM.
