Re: [PATCH v2] PCI: Unify ECAM constants in native PCI Express drivers

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Sep 25, 2020 at 09:15:13PM +0000, Krzysztof Wilczyński wrote:
> Unify ECAM-related constants into a single set of standard constants
> defining memory address shift values for the byte-level address that can
> be used when accessing the PCI Express Configuration Space, and then
> move native PCI Express controller drivers to use newly introduced
> definitions retiring any driver-specific ones.
> 
> The ECAM ("Enhanced Configuration Access Mechanism") is defined by the
> PCI Express specification (see PCI Base Specification, Revision 5.0,
> Version 1.0, Section 7.2.2, p. 676), thus most hardware should implement
> it the same way.  Most of the native PCI Express controller drivers
> define their ECAM-related constants, many of these could be shared, or
> use open-coded values when setting the .bus_shift field of the struct
> pci_ecam_ops.
> 
> All of the newly added constants should remove ambiguity and reduce the
> number of open-coded values, and also correlate more strongly with the
> descriptions in the aforementioned specification (see Table 7-1
> "Enhanced Configuration Address Mapping", p. 677).
> 
> There is no change to functionality.

I like this a lot.  A few minor comments below.

> --- a/drivers/pci/controller/dwc/pcie-al.c
> +++ b/drivers/pci/controller/dwc/pcie-al.c
> @@ -76,7 +76,7 @@ static int al_pcie_init(struct pci_config_window *cfg)
>  }
>  
>  const struct pci_ecam_ops al_pcie_ops = {
> -	.bus_shift    = 20,
> +	.bus_shift    = PCIE_ECAM_BUS_SHIFT,
>  	.init         =  al_pcie_init,
>  	.pci_ops      = {
>  		.map_bus    = al_pcie_map_bus,
> @@ -138,8 +138,6 @@ struct al_pcie {
>  	struct al_pcie_target_bus_cfg target_bus_cfg;
>  };
>  
> -#define PCIE_ECAM_DEVFN(x)		(((x) & 0xff) << 12)
> -
>  #define to_al_pcie(x)		dev_get_drvdata((x)->dev)
>  
>  static inline u32 al_pcie_controller_readl(struct al_pcie *pcie, u32 offset)
> @@ -228,7 +226,7 @@ static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
>  	void __iomem *pci_base_addr;
>  
>  	pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
> -					 (busnr_ecam << 20) +
> +					 PCIE_ECAM_BUS(busnr_ecam) +
>  					 PCIE_ECAM_DEVFN(devfn));

Apparently this driver lets you limit the number of buses, since it
does:

  unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;

I'm not entirely sure I believe that, but OK.  It's a shame because
it would be really nice to be able to use PCIE_ECAM_OFFSET() here
(with a little rework of the al_pcie_conf_addr_map() interface).

> --- a/drivers/pci/controller/pci-thunder-pem.c
> +++ b/drivers/pci/controller/pci-thunder-pem.c
> @@ -19,6 +19,9 @@
>  #define PEM_CFG_WR 0x28
>  #define PEM_CFG_RD 0x30
>  
> +/* Enhanced Configuration Access Mechanism (ECAM) */

Maybe just add a hint here that this is *non-standard* ECAM.

> +#define THUNDER_PCIE_ECAM_BUS_SHIFT	24

> --- a/drivers/pci/controller/pci-xgene.c
> +++ b/drivers/pci/controller/pci-xgene.c
> @@ -60,6 +60,9 @@
>  #define XGENE_PCIE_IP_VER_1		1
>  #define XGENE_PCIE_IP_VER_2		2
>  
> +/* Enhanced Configuration Access Mechanism (ECAM) */

Ditto.

> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -162,8 +162,7 @@ static int rockchip_pcie_rd_other_conf(struct rockchip_pcie *rockchip,
>  {
>  	u32 busdev;
>  
> -	busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn),
> -				PCI_FUNC(devfn), where);
> +	busdev = PCIE_ECAM_ADDR(bus, devfn, where);

You made the minimal change, which is good, but I don't think I could
resist doing this, which would make this code read a lot better:

  u32 ecam_addr;

  ecam_addr = rockchip->reg_base + PCIE_ECAM_OFFSET(bus, devfn, where);
  ...
  *val = readl(ecam_addr);

> --- a/drivers/pci/controller/pcie-xilinx-nwl.c
> +++ b/drivers/pci/controller/pcie-xilinx-nwl.c
> @@ -18,6 +18,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/of_irq.h>
>  #include <linux/pci.h>
> +#include <linux/pci-ecam.h>
>  #include <linux/platform_device.h>
>  #include <linux/irqchip/chained_irq.h>
>  
> @@ -124,8 +125,6 @@
>  #define E_ECAM_CR_ENABLE		BIT(0)
>  #define E_ECAM_SIZE_LOC			GENMASK(20, 16)
>  #define E_ECAM_SIZE_SHIFT		16
> -#define ECAM_BUS_LOC_SHIFT		20
> -#define ECAM_DEV_LOC_SHIFT		12
>  #define NWL_ECAM_VALUE_DEFAULT		12
>  
>  #define CFG_DMA_REG_BAR			GENMASK(2, 0)
> @@ -245,10 +244,9 @@ static void __iomem *nwl_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>  	if (!nwl_pcie_valid_device(bus, devfn))
>  		return NULL;
>  
> -	relbus = (bus->number << ECAM_BUS_LOC_SHIFT) |
> -			(devfn << ECAM_DEV_LOC_SHIFT);
> +	relbus = PCIE_ECAM_ADDR(bus, devfn, where);
>  
> -	return pcie->ecam_base + relbus + where;
> +	return pcie->ecam_base + relbus;

"relbus" seems pointless:

  return return pcie->ecam_base + PCIE_ECAM_OFFSET(bus, devfn, where);

> --- a/drivers/pci/controller/pcie-xilinx.c
> +++ b/drivers/pci/controller/pcie-xilinx.c
> @@ -21,6 +21,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/of_irq.h>
>  #include <linux/pci.h>
> +#include <linux/pci-ecam.h>
>  #include <linux/platform_device.h>
>  
>  #include "../pci.h"
> @@ -86,10 +87,6 @@
>  /* Phy Status/Control Register definitions */
>  #define XILINX_PCIE_REG_PSCR_LNKUP	BIT(11)
>  
> -/* ECAM definitions */
> -#define ECAM_BUS_NUM_SHIFT		20
> -#define ECAM_DEV_NUM_SHIFT		12
> -
>  /* Number of MSI IRQs */
>  #define XILINX_NUM_MSI_IRQS		128
>  
> @@ -188,10 +185,9 @@ static void __iomem *xilinx_pcie_map_bus(struct pci_bus *bus,
>  	if (!xilinx_pcie_valid_device(bus, devfn))
>  		return NULL;
>  
> -	relbus = (bus->number << ECAM_BUS_NUM_SHIFT) |
> -		 (devfn << ECAM_DEV_NUM_SHIFT);
> +	relbus = PCIE_ECAM_ADDR(bus, devfn, where);
>  
> -	return port->reg_base + relbus + where;
> +	return port->reg_base + relbus;

Same here.

> --- a/include/linux/pci-ecam.h
> +++ b/include/linux/pci-ecam.h
> @@ -9,6 +9,28 @@
>  #include <linux/kernel.h>
>  #include <linux/platform_device.h>
>  
> +/*
> + * Memory address shift values for the byte-level address that
> + * can be used when accessing the PCI Express Configuration Space.
> + */
> +
> +/* Configuration Access Mechanism (CAM) */
> +#define PCIE_CAM_BUS_SHIFT	16 /* Bus Number */

Is this a generic spec thing like ECAM is?  If it is, I'll cite the
spec here.

> +/* Enhanced Configuration Access Mechanism (ECAM) */
> +#define PCIE_ECAM_BUS_SHIFT	20 /* Bus Number */
> +#define PCIE_ECAM_DEV_SHIFT	15 /* Device Number */
> +#define PCIE_ECAM_FUN_SHIFT	12 /* Function Number */
> +
> +#define PCIE_ECAM_BUS(x)	(((x) & 0xff) << PCIE_ECAM_BUS_SHIFT)
> +#define PCIE_ECAM_DEVFN(x)	(((x) & 0xff) << PCIE_ECAM_FUN_SHIFT)
> +#define PCIE_ECAM_REG(x)	((x) & 0xfff)
> +
> +#define PCIE_ECAM_ADDR(bus, devfn, where) \
> +    (PCIE_ECAM_BUS(bus->number) | \
> +     PCIE_ECAM_DEVFN(devfn) | \
> +     PCIE_ECAM_REG(where))

Can you name this PCIE_ECAM_OFFSET() instead of PCIE_ECAM_ADDR()?
The result isn't an address we can use as-is; we have to add it to a
base address, and it doesn't really make sense to add an address to an
address.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel