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

[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 into file drivers/pci/pci.h new 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 can be done later.

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

Changes since RFC:
* Move macros to file drivers/pci/pci.h

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 | 21 +++----------
 drivers/pci/controller/pcie-mt7621.c  | 17 ++++------
 drivers/pci/pci.h                     | 45 +++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 28 deletions(-)

-- 
2.20.1

[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 in PCI Local Bus Specification) or
its extended version.

So introduce new macros PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() in
include file drivers/pci/pci.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>
---
 drivers/pci/pci.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 785f31086313..88bd77107103 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -774,4 +774,49 @@ static inline pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
 }
 #endif
 
+/*
+ * 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 /* DRIVERS_PCI_H */
-- 
2.20.1

[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 | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/pci-ftpci100.c b/drivers/pci/controller/pci-ftpci100.c
index 88980a44461d..0cfd9d5a497c 100644
--- a/drivers/pci/controller/pci-ftpci100.c
+++ b/drivers/pci/controller/pci-ftpci100.c
@@ -103,13 +103,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 +183,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 +215,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

[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 | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
index 33eb37a2225c..4bd1abf26008 100644
--- a/drivers/pci/controller/pcie-mt7621.c
+++ b/drivers/pci/controller/pcie-mt7621.c
@@ -30,6 +30,8 @@
 #include <linux/reset.h>
 #include <linux/sys_soc.h>
 
+#include "../pci.h"
+
 /* MediaTek-specific configuration registers */
 #define PCIE_FTS_NUM			0x70c
 #define PCIE_FTS_NUM_MASK		GENMASK(15, 8)
@@ -120,19 +122,12 @@ static inline void pcie_port_write(struct mt7621_pcie_port *port,
 	writel_relaxed(val, port->base + reg);
 }
 
-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;
-}
-
 static void __iomem *mt7621_pcie_map_bus(struct pci_bus *bus,
 					 unsigned int devfn, int where)
 {
 	struct mt7621_pcie *pcie = bus->sysdata;
-	u32 address = mt7621_pcie_get_cfgaddr(bus->number, PCI_SLOT(devfn),
-					     PCI_FUNC(devfn), where);
+	u32 address = PCI_CONF1_EXT_ADDRESS(bus->number, PCI_SLOT(devfn),
+					    PCI_FUNC(devfn), where);
 
 	writel_relaxed(address, pcie->base + RALINK_PCI_CONFIG_ADDR);
 
@@ -147,7 +142,7 @@ static struct pci_ops mt7621_pcie_ops = {
 
 static u32 read_config(struct mt7621_pcie *pcie, unsigned int dev, u32 reg)
 {
-	u32 address = mt7621_pcie_get_cfgaddr(0, dev, 0, reg);
+	u32 address = PCI_CONF1_EXT_ADDRESS(0, dev, 0, reg);
 
 	pcie_write(pcie, address, RALINK_PCI_CONFIG_ADDR);
 	return pcie_read(pcie, RALINK_PCI_CONFIG_DATA);
@@ -156,7 +151,7 @@ static u32 read_config(struct mt7621_pcie *pcie, unsigned int dev, u32 reg)
 static void write_config(struct mt7621_pcie *pcie, unsigned int dev,
 			 u32 reg, u32 val)
 {
-	u32 address = mt7621_pcie_get_cfgaddr(0, dev, 0, reg);
+	u32 address = PCI_CONF1_EXT_ADDRESS(0, dev, 0, reg);
 
 	pcie_write(pcie, address, RALINK_PCI_CONFIG_ADDR);
 	pcie_write(pcie, val, RALINK_PCI_CONFIG_DATA);
-- 
2.20.1

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

[Bjorn Helgaas]

On Sat, Sep 24, 2022 at 11:24:02AM +0200, Pali Rohár wrote:
> Lot of PCI and PCIe controllers are using standard Config Address for PCI
> Configuration Mechanism #1 (as defined in PCI Local Bus Specification) or
> its extended version.
> 
> So introduce new macros PCI_CONF1_ADDRESS() and PCI_CONF1_EXT_ADDRESS() in
> include file drivers/pci/pci.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>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> +#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 */

Since all the above are used only in the macros below, I personally
don't think they're really necessary and I would find it easier to
read if they were just open-coded, e.g.,

  +#define PCI_CONF1_BUS(x)     (((x) & 0xff) << 16)
  +#define PCI_CONF1_DEV(x)	(((x) & 0x1f) << 11)
  +#define PCI_CONF1_FUNC(x)	(((x) & 0x07) <<  8)
  +#define PCI_CONF1_REG(x)     ( (x) & 0xfc)

> +#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)

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

[Sergio Paracuellos]

Hi Pali,

On Sat, Sep 24, 2022 at 11:24 AM 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 | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)

I think I have already given my Acked-by and Tested-by for this series
in its RFC version [0]. I don't know if because that was a RFC my tags
are not added to this patch. Anyway:

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

Thanks,
    Sergio Paracuellos

[0]: https://lore.kernel.org/all/CAMhs-H9VHekbXg0avHpYP4=2mHoepnkH8rrshU9ZVnbAB=3h-A@mail.gmail.com/

>
> diff --git a/drivers/pci/controller/pcie-mt7621.c b/drivers/pci/controller/pcie-mt7621.c
> index 33eb37a2225c..4bd1abf26008 100644
> --- a/drivers/pci/controller/pcie-mt7621.c
> +++ b/drivers/pci/controller/pcie-mt7621.c
> @@ -30,6 +30,8 @@
>  #include <linux/reset.h>
>  #include <linux/sys_soc.h>
>
> +#include "../pci.h"
> +
>  /* MediaTek-specific configuration registers */
>  #define PCIE_FTS_NUM                   0x70c
>  #define PCIE_FTS_NUM_MASK              GENMASK(15, 8)
> @@ -120,19 +122,12 @@ static inline void pcie_port_write(struct mt7621_pcie_port *port,
>         writel_relaxed(val, port->base + reg);
>  }
>
> -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;
> -}
> -
>  static void __iomem *mt7621_pcie_map_bus(struct pci_bus *bus,
>                                          unsigned int devfn, int where)
>  {
>         struct mt7621_pcie *pcie = bus->sysdata;
> -       u32 address = mt7621_pcie_get_cfgaddr(bus->number, PCI_SLOT(devfn),
> -                                            PCI_FUNC(devfn), where);
> +       u32 address = PCI_CONF1_EXT_ADDRESS(bus->number, PCI_SLOT(devfn),
> +                                           PCI_FUNC(devfn), where);
>
>         writel_relaxed(address, pcie->base + RALINK_PCI_CONFIG_ADDR);
>
> @@ -147,7 +142,7 @@ static struct pci_ops mt7621_pcie_ops = {
>
>  static u32 read_config(struct mt7621_pcie *pcie, unsigned int dev, u32 reg)
>  {
> -       u32 address = mt7621_pcie_get_cfgaddr(0, dev, 0, reg);
> +       u32 address = PCI_CONF1_EXT_ADDRESS(0, dev, 0, reg);
>
>         pcie_write(pcie, address, RALINK_PCI_CONFIG_ADDR);
>         return pcie_read(pcie, RALINK_PCI_CONFIG_DATA);
> @@ -156,7 +151,7 @@ static u32 read_config(struct mt7621_pcie *pcie, unsigned int dev, u32 reg)
>  static void write_config(struct mt7621_pcie *pcie, unsigned int dev,
>                          u32 reg, u32 val)
>  {
> -       u32 address = mt7621_pcie_get_cfgaddr(0, dev, 0, reg);
> +       u32 address = PCI_CONF1_EXT_ADDRESS(0, dev, 0, reg);
>
>         pcie_write(pcie, address, RALINK_PCI_CONFIG_ADDR);
>         pcie_write(pcie, val, RALINK_PCI_CONFIG_DATA);
> --
> 2.20.1
>

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

[Lorenzo Pieralisi]

On Sat, 24 Sep 2022 11:24:01 +0200, Pali Rohár wrote:
> 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.
> 
> [...]

Applied to pci/misc, thanks!

[1/3] PCI: Add standard PCI Config Address macros
      https://git.kernel.org/lpieralisi/pci/c/8a9b7ef74369
[2/3] PCI: ftpci100: Use PCI_CONF1_ADDRESS() macro
      https://git.kernel.org/lpieralisi/pci/c/f75a27dc6c07
[3/3] PCI: mt7621: Use PCI_CONF1_EXT_ADDRESS() macro
      https://git.kernel.org/lpieralisi/pci/c/2301a3e1a566

Thanks,
Lorenzo