[RFC PATCH 0/3] PCI: Introduce new PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() macros

[RFC PATCH 0/3] PCI: Introduce new PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() macros

[Pali Rohár]

PCI controllers and lot of non-ECAM compliant PCIe controllers still use
Intel PCI Configuration Mechanism #1 for accessing PCI config space.

Native PCIe controller drivers invents its own macros which implements
config space address calculation and in lof of cases it is just
duplication of the same code.

PCIe ECAM address macro PCIE_ECAM_OFFSET() is already in include header
file linux/pci-ecam.h and ECAM compliant drivers were already converted
to use it.

Do similar thing also for Intel PCI Configuration Mechanism #1.
Introduce a new file linux/pci-conf1.h with PCI_CONF1_ADDRESS() and
PCI_CONF1_EXT_ADDRESS() macros and convert two drivers pci-ftpci100.c
and pcie-mt7621.c to use it.

There are many more drivers which could be converted to this common
macros. This is just RFC patch series and if you like it, I can look at
conversion of other drivers.

What do you think?

Note that similar cleanup was applied for U-Boot PCI controller drivers:
https://lore.kernel.org/u-boot/20211126104252.5443-1-pali@kernel.org/

Pali Rohár (3):
  PCI: Add standard PCI Config Address macros
  PCI: ftpci100: Use PCI_CONF1_ADDRESS() macro
  PCI: mt7621: Use PCI_CONF1_EXT_ADDRESS() macro

 drivers/pci/controller/pci-ftpci100.c | 22 +++---------
 drivers/pci/controller/pcie-mt7621.c  |  4 +--
 include/linux/pci-conf1.h             | 51 +++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 19 deletions(-)
 create mode 100644 include/linux/pci-conf1.h

-- 
2.20.1

[RFC PATCH 1/3] PCI: Add standard PCI Config Address macros

[Pali Rohár]

Lot of PCI and PCIe controllers are using standard Config Address for PCI
Configuration Mechanism #1 (as defined inPCI Local Bus Specification) or
its extended version.

So introduce new macros PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() in
new include file linux/pci-conf1.h which can be suitable for PCI and PCIe
controllers which uses this type of access to PCI config space.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 include/linux/pci-conf1.h | 51 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 include/linux/pci-conf1.h

diff --git a/include/linux/pci-conf1.h b/include/linux/pci-conf1.h
new file mode 100644
index 000000000000..12d2c581a67f
--- /dev/null
+++ b/include/linux/pci-conf1.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright 2022 Pali Rohár <pali@kernel.org> */
+#ifndef PCI_CONF1_H
+#define PCI_CONF1_H
+
+/*
+ * Config Address for PCI Configuration Mechanism #1
+ *
+ * See PCI Local Bus Specification, Revision 3.0,
+ * Section 3.2.2.3.2, Figure 3-2, p. 50.
+ */
+
+#define PCI_CONF1_BUS_SHIFT	16 /* Bus number */
+#define PCI_CONF1_DEV_SHIFT	11 /* Device number */
+#define PCI_CONF1_FUNC_SHIFT	8  /* Function number */
+
+#define PCI_CONF1_BUS_MASK	0xff
+#define PCI_CONF1_DEV_MASK	0x1f
+#define PCI_CONF1_FUNC_MASK	0x7
+#define PCI_CONF1_REG_MASK	0xfc /* Limit aligned offset to a maximum of 256B */
+
+#define PCI_CONF1_ENABLE	BIT(31)
+#define PCI_CONF1_BUS(x)	(((x) & PCI_CONF1_BUS_MASK) << PCI_CONF1_BUS_SHIFT)
+#define PCI_CONF1_DEV(x)	(((x) & PCI_CONF1_DEV_MASK) << PCI_CONF1_DEV_SHIFT)
+#define PCI_CONF1_FUNC(x)	(((x) & PCI_CONF1_FUNC_MASK) << PCI_CONF1_FUNC_SHIFT)
+#define PCI_CONF1_REG(x)	((x) & PCI_CONF1_REG_MASK)
+
+#define PCI_CONF1_ADDRESS(bus, dev, func, reg) \
+	(PCI_CONF1_ENABLE | \
+	 PCI_CONF1_BUS(bus) | \
+	 PCI_CONF1_DEV(dev) | \
+	 PCI_CONF1_FUNC(func) | \
+	 PCI_CONF1_REG(reg))
+
+/*
+ * Extension of PCI Config Address for accessing extended PCIe registers
+ *
+ * No standardized specification, but used on lot of non-ECAM-compliant ARM SoCs
+ * or on AMD Barcelona and new CPUs. Reserved bits [27:24] of PCI Config Address
+ * are used for specifying additional 4 high bits of PCI Express register.
+ */
+
+#define PCI_CONF1_EXT_REG_SHIFT	16
+#define PCI_CONF1_EXT_REG_MASK	0xf00
+#define PCI_CONF1_EXT_REG(x)	(((x) & PCI_CONF1_EXT_REG_MASK) << PCI_CONF1_EXT_REG_SHIFT)
+
+#define PCI_CONF1_EXT_ADDRESS(bus, dev, func, reg) \
+	(PCI_CONF1_ADDRESS(bus, dev, func, reg) | \
+	 PCI_CONF1_EXT_REG(reg))
+
+#endif
-- 
2.20.1

Re: [RFC PATCH 1/3] PCI: Add standard PCI Config Address macros

[Bjorn Helgaas]

On Sun, Sep 11, 2022 at 01:20:22PM +0200, Pali Rohár wrote:
> Lot of PCI and PCIe controllers are using standard Config Address for PCI
> Configuration Mechanism #1 (as defined inPCI Local Bus Specification) or
> its extended version.
> 
> So introduce new macros PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() in
> new include file linux/pci-conf1.h which can be suitable for PCI and PCIe
> controllers which uses this type of access to PCI config space.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  include/linux/pci-conf1.h | 51 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 include/linux/pci-conf1.h

This seems like a nice addition, but it would be nice if we could
encapsulate it in drivers/pci.

I know it's parallel to the existing include/linux/pci-ecam.h.  I wish
we could encapsulate *that* in drivers/pci, too.  For pci-ecam.h, I
think the only things that prevent that are drivers/acpi/pci_mcfg.c,
loongarch, and a few arm64 things.

pci_mcfg.c arguably would make more sense in drivers/pci; it uses
acpi_table_parse(), but no other ACPI services.

The arm64 code that uses pci-ecam.h is really generic code that would
not be in arch/arm64 except for the fact that x86 has really ugly
legacy x86-specific mmconfig code.

I guess that's a long-winded way of saying that I think maybe we could
put this in drivers/pci/pci.h even though the parallel ECAM stuff is
in include/linux/pci-ecam.h.

Bjorn

Re: [RFC PATCH 1/3] PCI: Add standard PCI Config Address macros

[Pali Rohár]

On Tuesday 13 September 2022 16:11:43 Bjorn Helgaas wrote:
> On Sun, Sep 11, 2022 at 01:20:22PM +0200, Pali Rohár wrote:
> > Lot of PCI and PCIe controllers are using standard Config Address for PCI
> > Configuration Mechanism #1 (as defined inPCI Local Bus Specification) or
> > its extended version.
> > 
> > So introduce new macros PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() in
> > new include file linux/pci-conf1.h which can be suitable for PCI and PCIe
> > controllers which uses this type of access to PCI config space.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  include/linux/pci-conf1.h | 51 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >  create mode 100644 include/linux/pci-conf1.h
> 
> This seems like a nice addition, but it would be nice if we could
> encapsulate it in drivers/pci.
> 
> I know it's parallel to the existing include/linux/pci-ecam.h.  I wish
> we could encapsulate *that* in drivers/pci, too.  For pci-ecam.h, I
> think the only things that prevent that are drivers/acpi/pci_mcfg.c,
> loongarch, and a few arm64 things.

As these macros describe original Intel x86 API, it can be used also in
arch/x86 PCI code.

> pci_mcfg.c arguably would make more sense in drivers/pci; it uses
> acpi_table_parse(), but no other ACPI services.
> 
> The arm64 code that uses pci-ecam.h is really generic code that would
> not be in arch/arm64 except for the fact that x86 has really ugly
> legacy x86-specific mmconfig code.

IIRC that legacy x86-specific code is used also on modern AMD processors
which have broken ECAM. AMD supports that extended version of CF8/CFC
with access to PCIe extended config space registers.

> I guess that's a long-winded way of saying that I think maybe we could
> put this in drivers/pci/pci.h even though the parallel ECAM stuff is
> in include/linux/pci-ecam.h.
> 
> Bjorn

Well, if you like this change, let me know where to put those new
macros, into which file and in which subdirectory, and I can prepare a
new patch version.

But doing all those arm64, x86, ACPI cleanup is a huge cross-tree work
which I'm really not going to do...

Re: [RFC PATCH 1/3] PCI: Add standard PCI Config Address macros

[Bjorn Helgaas]

On Tue, Sep 13, 2022 at 11:24:21PM +0200, Pali Rohár wrote:
> On Tuesday 13 September 2022 16:11:43 Bjorn Helgaas wrote:
> > On Sun, Sep 11, 2022 at 01:20:22PM +0200, Pali Rohár wrote:
> > > Lot of PCI and PCIe controllers are using standard Config Address for PCI
> > > Configuration Mechanism #1 (as defined inPCI Local Bus Specification) or
> > > its extended version.
> > > 
> > > So introduce new macros PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() in
> > > new include file linux/pci-conf1.h which can be suitable for PCI and PCIe
> > > controllers which uses this type of access to PCI config space.
> > > 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > ---
> > >  include/linux/pci-conf1.h | 51 +++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 51 insertions(+)
> > >  create mode 100644 include/linux/pci-conf1.h
> > 
> > This seems like a nice addition, but it would be nice if we could
> > encapsulate it in drivers/pci.
> > 
> > I know it's parallel to the existing include/linux/pci-ecam.h.  I wish
> > we could encapsulate *that* in drivers/pci, too.  For pci-ecam.h, I
> > think the only things that prevent that are drivers/acpi/pci_mcfg.c,
> > loongarch, and a few arm64 things.
> 
> As these macros describe original Intel x86 API, it can be used also in
> arch/x86 PCI code.

I would love to see that happen, too, and that could be a reason to
put pci-conf.h in include/linux.  But this series doesn't include
that.

> > I guess that's a long-winded way of saying that I think maybe we could
> > put this in drivers/pci/pci.h even though the parallel ECAM stuff is
> > in include/linux/pci-ecam.h.
> 
> Well, if you like this change, let me know where to put those new
> macros, into which file and in which subdirectory, and I can prepare a
> new patch version.

drivers/pci/pci.h

> But doing all those arm64, x86, ACPI cleanup is a huge cross-tree work
> which I'm really not going to do...

Of course not, I didn't suggest or expect that.  What I'm trying to
point out is that I don't think we have very good reasons for
pci-ecam.h to be public.  And therefore, I don't think we need
pci-conf1.h to be next to it.

Unless you want to convert the arch/x86 code to use them as well.  I'm
not asking you to do that either, just that if you *did* do that, it
would be an argument for keeping the macros where you put them.

Bjorn

[RFC PATCH 2/3] PCI: ftpci100: Use PCI_CONF1_ADDRESS() macro

[Pali Rohár]

Simplify pci-ftpci100.c driver code and use new PCI_CONF1_ADDRESS() macro
for accessing PCI config space.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/controller/pci-ftpci100.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/pci-ftpci100.c b/drivers/pci/controller/pci-ftpci100.c
index 88980a44461d..86f6ab165850 100644
--- a/drivers/pci/controller/pci-ftpci100.c
+++ b/drivers/pci/controller/pci-ftpci100.c
@@ -27,6 +27,7 @@
 #include <linux/bitops.h>
 #include <linux/irq.h>
 #include <linux/clk.h>
+#include <linux/pci-conf1.h>
 
 #include "../pci.h"
 
@@ -103,13 +104,6 @@
 #define FARADAY_PCI_DMA_MEM2_BASE	0x00000000
 #define FARADAY_PCI_DMA_MEM3_BASE	0x00000000
 
-/* Defines for PCI configuration command register */
-#define PCI_CONF_ENABLE		BIT(31)
-#define PCI_CONF_WHERE(r)	((r) & 0xFC)
-#define PCI_CONF_BUS(b)		(((b) & 0xFF) << 16)
-#define PCI_CONF_DEVICE(d)	(((d) & 0x1F) << 11)
-#define PCI_CONF_FUNCTION(f)	(((f) & 0x07) << 8)
-
 /**
  * struct faraday_pci_variant - encodes IP block differences
  * @cascaded_irq: this host has cascaded IRQs from an interrupt controller
@@ -190,11 +184,8 @@ static int faraday_raw_pci_read_config(struct faraday_pci *p, int bus_number,
 				       unsigned int fn, int config, int size,
 				       u32 *value)
 {
-	writel(PCI_CONF_BUS(bus_number) |
-			PCI_CONF_DEVICE(PCI_SLOT(fn)) |
-			PCI_CONF_FUNCTION(PCI_FUNC(fn)) |
-			PCI_CONF_WHERE(config) |
-			PCI_CONF_ENABLE,
+	writel(PCI_CONF1_ADDRESS(bus_number, PCI_SLOT(fn),
+				 PCI_FUNC(fn), config),
 			p->base + FTPCI_CONFIG);
 
 	*value = readl(p->base + FTPCI_DATA);
@@ -225,11 +216,8 @@ static int faraday_raw_pci_write_config(struct faraday_pci *p, int bus_number,
 {
 	int ret = PCIBIOS_SUCCESSFUL;
 
-	writel(PCI_CONF_BUS(bus_number) |
-			PCI_CONF_DEVICE(PCI_SLOT(fn)) |
-			PCI_CONF_FUNCTION(PCI_FUNC(fn)) |
-			PCI_CONF_WHERE(config) |
-			PCI_CONF_ENABLE,
+	writel(PCI_CONF1_ADDRESS(bus_number, PCI_SLOT(fn),
+				 PCI_FUNC(fn), config),
 			p->base + FTPCI_CONFIG);
 
 	switch (size) {
-- 
2.20.1

[RFC PATCH 3/3] PCI: mt7621: Use PCI_CONF1_EXT_ADDRESS() macro

[Pali Rohár]

Simplify pcie-mt7621.c driver code and use new PCI_CONF1_EXT_ADDRESS()
macro for accessing PCIe config space.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/pci/controller/pcie-mt7621.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
index 33eb37a2225c..28cde116cd27 100644
--- a/drivers/pci/controller/pcie-mt7621.c
+++ b/drivers/pci/controller/pcie-mt7621.c
@@ -25,6 +25,7 @@
 #include <linux/of_pci.h>
 #include <linux/of_platform.h>
 #include <linux/pci.h>
+#include <linux/pci-conf1.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/reset.h>
@@ -123,8 +124,7 @@ static inline void pcie_port_write(struct mt7621_pcie_port *port,
 static inline u32 mt7621_pcie_get_cfgaddr(unsigned int bus, unsigned int slot,
 					 unsigned int func, unsigned int where)
 {
-	return (((where & 0xf00) >> 8) << 24) | (bus << 16) | (slot << 11) |
-		(func << 8) | (where & 0xfc) | 0x80000000;
+	return PCI_CONF1_EXT_ADDRESS(bus, slot, func, where);
 }
 
 static void __iomem *mt7621_pcie_map_bus(struct pci_bus *bus,
-- 
2.20.1

Re: [RFC PATCH 3/3] PCI: mt7621: Use PCI_CONF1_EXT_ADDRESS() macro

[Sergio Paracuellos]

Hi Pali,

On Sun, Sep 11, 2022 at 1:21 PM Pali Rohár <pali@kernel.org> wrote:
>
> Simplify pcie-mt7621.c driver code and use new PCI_CONF1_EXT_ADDRESS()
> macro for accessing PCIe config space.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  drivers/pci/controller/pcie-mt7621.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> index 33eb37a2225c..28cde116cd27 100644
> --- a/drivers/pci/controller/pcie-mt7621.c
> +++ b/drivers/pci/controller/pcie-mt7621.c
> @@ -25,6 +25,7 @@
>  #include <linux/of_pci.h>
>  #include <linux/of_platform.h>
>  #include <linux/pci.h>
> +#include <linux/pci-conf1.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
>  #include <linux/reset.h>
> @@ -123,8 +124,7 @@ static inline void pcie_port_write(struct mt7621_pcie_port *port,
>  static inline u32 mt7621_pcie_get_cfgaddr(unsigned int bus, unsigned int slot,
>                                          unsigned int func, unsigned int where)
>  {
> -       return (((where & 0xf00) >> 8) << 24) | (bus << 16) | (slot << 11) |
> -               (func << 8) | (where & 0xfc) | 0x80000000;
> +       return PCI_CONF1_EXT_ADDRESS(bus, slot, func, where);
>  }
>
>  static void __iomem *mt7621_pcie_map_bus(struct pci_bus *bus,
> --
> 2.20.1
>

I have just added you patches in the top of my 5.19 build:

commit 70cb6afe0e2ff1b7854d840978b1849bffb3ed21 (tag: v5.19.8,
stable/linux-5.19.y)

After building them and boot with your changes no regressions seem to
appear, so if this series are finally added to the tree, feel free to
add my:

Acked-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Best regards,
    Sergio Paracuellos