[PATCH v3 07/12] PCI: tegra: Move PCIe driver to drivers/pci/host

[Stephen Warren]

On 04/03/2013 08:45 AM, Thierry Reding wrote:
> Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host
> directory. The motivation is to collect various host controller drivers
> in the same location in order to facilitate refactoring.
> 
> The Tegra PCIe driver has been largely rewritten, both in order to turn
> it into a proper platform driver and to add MSI (based on code by
> Krishna Kishore <kthota at nvidia.com>) as well as device tree support.

>  arch/arm/boot/dts/tegra20.dtsi                     |   53 +

I guess this has to touch both the driver and the dtsi file so that
bisectabilty isn't broken? I guess that's OK.

> diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt b/Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt

> +Required properties:

> +- interrupts: the interrupt outputs of the controller
> +- interrupt-names: list of names to identify interrupts

The specification part of this file should define which interrupt
outputs must be included in this list, and the order they must appear in
the list. I believe that the entries in the interrupts property must
have a defined order, so I'm not sure whether interrupt-names is useful
here?

> +- ranges: describes the translation of addresses for root ports

Shouldn't there be some discussion re: how the range are expected to be
set up so that everything works? We shouldn't expect people to just
blindly copy the example without any way of understanding it.

> +- clocks: the clock inputs of the controller
> +- clock-names: list of names to identify clocks

The specification part of this file should define which clocks are
required, and not rely on the example below to do this. I would suggest
re-writing this as:

- clocks : Must contain an entry for each entry in clock-names.
- clock-names : Must include the following entries:
  "pex" (The Tegra clock of that name)
  "afi" (The Tegra clock of that name)
  "pcie_xclk" (The Tegra clock of that name)
  "pll_e" (The Tegra clock of that name)

> +Root ports are defined as subnodes of the PCIe controller node.
> +
> +Required properties:
...
> +- ranges: sub-ranges distributed from the PCIe controller node

Here, I think it's worth mentioning that an empty ranges is all that's
required.

> +Board DTS:
> +
> +	pcie-controller {
> +		status = "okay";
> +
> +		vdd-supply = <&pci_vdd_reg>;
> +		pex-clk-supply = <&pci_clk_reg>;
> +
> +		/* root port 00:01.0 */
> +		pci at 1,0 {
> +			status = "okay";

Is it worth putting a comment here stating that the explicit devices
included below in this example are entirely optional?

> diff --git a/arch/arm/mach-tegra/board-harmony-pcie.c b/arch/arm/mach-tegra/board-harmony-pcie.c
> deleted file mode 100644

Hmmm. Is this patch meant to include a change to tegra20-harmony.dtsi to
hook up all the regulators through device tree? Same for TrimSlice?

> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile

I think the creation of those files should be a separate patch, and
hopefully get into 3.10 to remove any dependencies. Otherwise, 3 or 4
different patches are all going to try and do the same thing. Didn't the
Marvell series split out the creation of drivers/pci/host/ into a
separate patch that's suitable for this, and could go into 3.10?

> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c

I'll review that file separately later.