Re: [PATCH V4 1/2] PCI: hisi: Add ECAM support for devices that are not RC

[Bjorn Helgaas]

[+cc Jingoo]

On Wed, Nov 09, 2016 at 05:14:56PM +0800, Dongdong Liu wrote:
> This patch modifies the current Hip05/Hip06 PCIe host
> controller driver to add support for 'almost ECAM'
> compliant platforms. Some controllers are ECAM compliant
> for all the devices of the hierarchy except the root
> complex; this patch adds support for such controllers.
> 
> This is needed in preparation for the ACPI based driver
> to allow both DT and ACPI drivers to use the same BIOS
> (that configure the Designware iATUs).
> This commit doesn't break backward compatibility with
> previous non-ECAM platforms.
> 
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx>
> Signed-off-by: Dongdong Liu <liudongdong3@xxxxxxxxxx>
> ---
>  .../devicetree/bindings/pci/hisilicon-pcie.txt     | 15 +++++---
>  drivers/pci/host/Kconfig                           |  4 +-
>  drivers/pci/host/pcie-designware.c                 |  4 +-
>  drivers/pci/host/pcie-designware.h                 |  2 +
>  drivers/pci/host/pcie-hisi.c                       | 45 ++++++++++++++++++++++
>  5 files changed, 60 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt b/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
> index 59c2f47..87a597a 100644
> --- a/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
> @@ -9,10 +9,13 @@ Additional properties are described here:
>  
>  Required properties
>  - compatible: Should contain "hisilicon,hip05-pcie" or "hisilicon,hip06-pcie".
> -- reg: Should contain rc_dbi, config registers location and length.
> -- reg-names: Must include the following entries:
> +- reg: Should contain rc_dbi and  either config or ecam-cfg registers
> +       location and length (it depends on the platform BIOS).
> +- reg-names: Must include
>    "rc_dbi": controller configuration registers;
> -  "config": PCIe configuration space registers.
> +  and one of the following entries:
> +    "config": PCIe configuration space registers for non-ECAM platforms.
> +    "ecam-cfg": PCIe configuration space registers for ECAM platforms
>  - msi-parent: Should be its_pcie which is an ITS receiving MSI interrupts.
>  - port-id: Should be 0, 1, 2 or 3.
>  
> @@ -23,8 +26,10 @@ Optional properties:
>  Hip05 Example (note that Hip06 is the same except compatible):
>  	pcie@0xb0080000 {
>  		compatible = "hisilicon,hip05-pcie", "snps,dw-pcie";
> -		reg = <0 0xb0080000 0 0x10000>, <0x220 0x00000000 0 0x2000>;
> -		reg-names = "rc_dbi", "config";
> +		reg = <0 0xb0080000 0 0x10000>,
> +		      <0x220 0x00000000 0 0x2000>
> +		/* or <0x220 0x00100000 0 0x0f00000> for ecam-cfg*/;

Add space before closing "*/".

> +		reg-names = "rc_dbi", "config" /* or "ecam-cfg" */;
>  		bus-range = <0  15>;
>  		msi-parent = <&its_pcie>;
>  		#address-cells = <3>;
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index d7e7c0a..ae98644 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -219,13 +219,13 @@ config PCIE_ALTERA_MSI
>  
>  config PCI_HISI
>  	depends on OF && ARM64
> -	bool "HiSilicon Hip05 and Hip06 SoCs PCIe controllers"
> +	bool "HiSilicon Hip05 and Hip06 and Hip07 SoCs PCIe controllers"
>  	depends on PCI_MSI_IRQ_DOMAIN
>  	select PCIEPORTBUS
>  	select PCIE_DW
>  	help
>  	  Say Y here if you want PCIe controller support on HiSilicon
> -	  Hip05 and Hip06 SoCs
> +	  Hip05 and Hip06 and Hip07 SoCs
>  
>  config PCIE_QCOM
>  	bool "Qualcomm PCIe controller"
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 035f50c..da11644 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -101,8 +101,6 @@
>  #define PCIE_PHY_DEBUG_R1_LINK_UP	(0x1 << 4)
>  #define PCIE_PHY_DEBUG_R1_LINK_IN_TRAINING	(0x1 << 29)
>  
> -static struct pci_ops dw_pcie_ops;
> -
>  int dw_pcie_cfg_read(void __iomem *addr, int size, u32 *val)
>  {
>  	if ((uintptr_t)addr & (size - 1)) {
> @@ -800,7 +798,7 @@ static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>  	return dw_pcie_wr_other_conf(pp, bus, devfn, where, size, val);
>  }
>  
> -static struct pci_ops dw_pcie_ops = {
> +struct pci_ops dw_pcie_ops = {
>  	.read = dw_pcie_rd_conf,
>  	.write = dw_pcie_wr_conf,
>  };
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index a567ea2..30d228a 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -83,4 +83,6 @@ struct pcie_host_ops {
>  void dw_pcie_setup_rc(struct pcie_port *pp);
>  int dw_pcie_host_init(struct pcie_port *pp);
>  
> +extern struct pci_ops dw_pcie_ops;
> +
>  #endif /* _PCIE_DESIGNWARE_H */
> diff --git a/drivers/pci/host/pcie-hisi.c b/drivers/pci/host/pcie-hisi.c
> index 56154c2..555c964 100644
> --- a/drivers/pci/host/pcie-hisi.c
> +++ b/drivers/pci/host/pcie-hisi.c
> @@ -43,6 +43,34 @@ struct hisi_pcie {
>  	struct pcie_soc_ops *soc_ops;
>  };
>  
> +static inline int hisi_rd_ecam_conf(struct pcie_port *pp, struct pci_bus *bus,
> +				    unsigned int devfn, int where, int size,
> +				    u32 *value)
> +{
> +	return pci_generic_config_read(bus, devfn, where, size, value);
> +}
> +
> +static inline int hisi_wr_ecam_conf(struct pcie_port *pp, struct pci_bus *bus,
> +				    unsigned int devfn, int where, int size,
> +				    u32 value)
> +{
> +	return pci_generic_config_write(bus, devfn, where, size, value);
> +}
> +
> +static void __iomem *hisi_pci_map_cfg_bus_cam(struct pci_bus *bus,
> +					      unsigned int devfn,
> +					      int where)
> +{
> +	void __iomem *addr;
> +	struct pcie_port *pp = bus->sysdata;
> +
> +	addr = pp->va_cfg1_base - (pp->busn->start << 20) +
> +		((bus->number << 20) | (devfn << 12)) +
> +		where;
> +
> +	return addr;
> +}
> +
>  /* HipXX PCIe host only supports 32-bit config access */
>  static int hisi_pcie_cfg_read(struct pcie_port *pp, int where, int size,
>  			      u32 *val)
> @@ -190,6 +218,23 @@ static int hisi_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(pp->dbi_base);
>  	}
>  
> +	reg = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ecam-cfg");
> +	if (reg) {
> +		/* ECAM driver version */
> +		hisi_pcie->pp.va_cfg0_base =
> +		devm_ioremap_resource(&pdev->dev, reg);
> +		if (IS_ERR(hisi_pcie->pp.va_cfg0_base)) {
> +			dev_err(pp->dev, "cannot get ecam-cfg\n");
> +			return PTR_ERR(hisi_pcie->pp.va_cfg0_base);
> +		}
> +		hisi_pcie->pp.va_cfg1_base = hisi_pcie->pp.va_cfg0_base;
> +
> +		dw_pcie_ops.map_bus = hisi_pci_map_cfg_bus_cam;

I don't really like this part.  We're scribbling on dw_pcie_ops, which this
driver does not own.  That breaks the encapsulation and means that using
two different DW-based drivers in the same system (admittedly unlikely)
would fail mysteriously.

I wonder if this could be solved by allowing the DW-based drivers to supply
their own pci_ops.  If we did that, it might let us get rid of the
rd_own_conf, wr_own_conf, rd_other_conf, and wr_other_conf function
pointers.  I haven't worked through it, but it's possible the result would
be easier to read.

It's also possible it would be a lot of code churn for no real gain, and
breaking the encapsulation makes the most sense.  What do you think?

> +
> +		hisi_pcie_host_ops.rd_other_conf = hisi_rd_ecam_conf;
> +		hisi_pcie_host_ops.wr_other_conf = hisi_wr_ecam_conf;
> +	}
> +
>  	ret = hisi_add_pcie_port(hisi_pcie, pdev);
>  	if (ret)
>  		return ret;
> -- 
> 1.9.1
>