| Message ID | 20220911113216.14892-1-pali@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: tegra: Update comment about config space | expand |
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@kernel.org> > --- > 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? 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
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@kernel.org> > > --- > > 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 :-) In any case, wrong documentation (which is the current state) should be fixed (and removal in most case is also proper fix).
On Wed, Oct 05, 2022 at 09:43:36PM +0200, Pali Rohár wrote: [...] > > 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 :-) > > In any case, wrong documentation (which is the current state) should be > fixed (and removal in most case is also proper fix). I agree. I would apply this patch if Thierry is still OK with it. Lorenzo
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@kernel.org> > > > --- > > > 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. > In any case, wrong documentation (which is the current state) should be > fixed (and removal in most case is also proper fix). Again, I don't see that this fixes anything because there was no bug. The documentation change makes the most sense when combined with the change that actually implements this in terms of the new macro. The existing documentation exists to give further background information about the mapping. If we remove the comment out of context we loose that extra information. However, if at the same time we change the code to use another (documented) macro, then we replace the information without loosing anything. Thierry
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@kernel.org> > > > > --- > > > > 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.
diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c index 8e323e93be91..5df90d183526 100644 --- a/drivers/pci/controller/pci-tegra.c +++ b/drivers/pci/controller/pci-tegra.c @@ -395,9 +395,11 @@ static inline u32 pads_readl(struct tegra_pcie *pcie, unsigned long offset) } /* - * The configuration space mapping on Tegra is somewhat similar to the ECAM - * defined by PCIe. However it deviates a bit in how the 4 bits for extended - * register accesses are mapped: + * The configuration space mapping on Tegra is somewhat similar to the Intel + * PCI Configuration Mechanism #1 as defined in PCI Local Bus Specification. + * But it is mapped directly into physical address space as opposite of the + * CF8/CFC indirect access, bit 31 (enable) is unset and reserved bits [27:24] + * are used to access extended PCIe config space registers. * * [27:24] extended register number * [23:16] bus number
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@kernel.org> --- drivers/pci/controller/pci-tegra.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)