Re: [PATCH V5 7/8] pcie: SPEAr13xx: Add designware pcie support

[Arnd Bergmann <arnd@arndb.de>]

On Monday 10 February 2014, Mohit Kumar wrote:
> diff --git a/Documentation/devicetree/bindings/pci/spear13xx-pcie.txt b/Documentation/devicetree/bindings/pci/spear13xx-pcie.txt
> new file mode 100644
> index 0000000..dc8ae44
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/spear13xx-pcie.txt
> @@ -0,0 +1,7 @@
> +Required properties:
> +- compatible : should be "st,spear1340-pcie", "snps,dw-pcie".
> +- pcie_is_gen1: pass <1> if forced gen1 initialization is needed to work with
> +  some buggy cards else pass <0>.

Better make this an optional property that is only present if gen1 is forced,
and then use of_property_read_bool to check it.

Also, use the common naming convention: "-" instead of "_", and you may want to
prefix it with 'st,'.

> +- phys		    : phandle to phy node associated with pcie controller
> +- phy-names	    : must be "pcie-phy"
> +- All other definitions as per generic PCI bindings

Please send the binding to the devicetree mailing list, ideally as a separate patch.

> diff --git a/arch/arm/boot/dts/spear1310.dtsi b/arch/arm/boot/dts/spear1310.dtsi
> index 64e7dd5..8d7aefe 100644
> --- a/arch/arm/boot/dts/spear1310.dtsi
> +++ b/arch/arm/boot/dts/spear1310.dtsi

Best split out the DT changes as well, since we may want to apply them through
the arm-soc tree.

> @@ -83,6 +83,63 @@
>  			status = "disabled";
>  		};
>  
> +		pcie0: pcie@b1000000 {
> +			compatible = "st,spear1340-pcie", "snps,dw-pcie";
> +			reg = <0xb1000000 0x4000>;
> +			interrupts = <0 68 0x4>;
> +			interrupt-map-mask = <0 0 0 0>;
> +			interrupt-map = <0x0 0 &gic 68>;
> +			pcie_is_gen1 = <0>;
> +			num-lanes = <1>;
> +			phys = <&miphy0 1>;
> +			phy-names = "pcie-phy";
> +			#address-cells = <3>;
> +			#size-cells = <2>;
> +			device_type = "pci";
> +			ranges = <0x00000800 0 0x80000000 0x80000000 0 0x00020000   /* configuration space */
> +				0x81000000 0 0	 0x80020000 0 0x00010000   /* downstream I/O */
> +				0x82000000 0 0x80030000 0xc0030000 0 0x0ffd0000>; /* non-prefetchable memory */
> +			status = "disabled";
> +		};

But keep the changes for the phy together with the pci changes.

> @@ -338,6 +338,7 @@
>  				reg = <0xe07008c4 0x4>;
>  				thermal_flags = <0x7000>;
>  			};
> +
>  		};
>  	};
>  };

I assume this change was not intentional.

> diff --git a/arch/arm/configs/spear13xx_defconfig b/arch/arm/configs/spear13xx_defconfig
> index 1e0bb6f..96ede50 100644
> --- a/arch/arm/configs/spear13xx_defconfig
> +++ b/arch/arm/configs/spear13xx_defconfig
> @@ -11,6 +11,8 @@ CONFIG_ARCH_SPEAR13XX=y
>  CONFIG_MACH_SPEAR1310=y
>  CONFIG_MACH_SPEAR1340=y
>  # CONFIG_SWP_EMULATE is not set
> +CONFIG_PCI_MSI=y
> +CONFIG_PCIE_SPEAR13XX=y
>  CONFIG_SMP=y
>  # CONFIG_SMP_ON_UP is not set
>  # CONFIG_ARM_CPU_TOPOLOGY is not set

This change can also get folded into patch 3. 

> diff --git a/arch/arm/mach-spear/Kconfig b/arch/arm/mach-spear/Kconfig
> index 7e7f1b0..701b6c3 100644
> --- a/arch/arm/mach-spear/Kconfig
> +++ b/arch/arm/mach-spear/Kconfig
> @@ -28,6 +28,7 @@ config ARCH_SPEAR13XX
>  	select USE_OF
>  	select MFD_SYSCON
>  	select PHY_ST_MIPHY40LP
> +	select PCI
>  	help
>  	  Supports for ARM's SPEAR13XX family
>  

This doesn't seem right: You are making it impossible to turn off PCI
here. Better use MIGHT_HAVE_PCI instead to make CONFIG_PCI a user-selectable
option.

> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 47d46c6..df52fad 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -33,4 +33,9 @@ config PCI_RCAR_GEN2
>  	  There are 3 internal PCI controllers available with a single
>  	  built-in EHCI/OHCI host controller present on each one.
>  
> +config PCIE_SPEAR13XX
> +	bool "STMicroelectronics SPEAr PCIe controller"
> +	depends on ARCH_SPEAR13XX
> +	select PCIEPORTBUS
> +	select PCIE_DW
>  endmenu

This can probably be "tristate" instead of "bool. You need to add
a help text.

> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 13fb333..42a491d 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
>  obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
>  obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o
>  obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
> +obj-$(CONFIG_PCIE_SPEAR13XX) += pcie-spear13xx.o

I'd use the "pci" naming instead of "pcie" here for consistency.

> +
> +/* SPEAr13xx PCIe driver does not allow module unload */
> +
> +static int __init pcie_init(void)
> +{
> +
> +	return platform_driver_probe(&spear13xx_pcie_driver,
> +				spear13xx_pcie_probe);
> +}

This should be platform_driver_register() so you can support
deferred probing, e.g. if the phy driver gets loaded after the
PCI driver.

	Arnd