On 06/12/2012 12:21 AM, Thierry Reding wrote:

* Stephen Warren wrote:

quoted

On 06/11/2012 09:05 AM, Thierry Reding wrote:

quoted

This commit adds support for instantiating the Tegra PCIe
controller from a device tree.

quoted

+++ b/Documentation/devicetree/bindings/pci/tegra-pcie.txt

Can we please name this nvidia,tegra20-pcie.txt to match the
naming of all the other Tegra bindings.

Yes, will do.

quoted

quoted

+Required properties: +- compatible: "nvidia,tegra20-pcie" +-
reg: physical base address and length of the controller's
registers

Since there's more than one range now, that should specify how
many entries are required and what they represent.

Okay.

quoted

quoted

+Optional properties: +- pex-clk-supply: supply voltage for
internal reference clock +- vdd-supply: power supply for
controller (1.05V)

Those shouldn't be optional. If the board has no regulator, the
board's .dts should provide a fixed always-on regulator that
those properties can refer to, so that the driver can always
get() those regulators.

That'll add more dummy regulators and I don't think sprinkling them
across the DTS is going to work very well. Maybe collecting them
under a top-level "regulators" node is a good option. If you have a
better alternative, I'm all open for it.

quoted

quoted

diff --git a/arch/arm/boot/dts/tegra20.dtsi

b/arch/arm/boot/dts/tegra20.dtsi

quoted

+	pci {

...

quoted

+		status = "disable";

That should be "disabled"; sorry for providing a bad example.

Yes.

quoted

quoted

diff --git a/arch/arm/mach-tegra/pcie.c

b/arch/arm/mach-tegra/pcie.c

quoted

+static struct tegra_pcie_pdata *tegra_pcie_parse_dt(struct
platform_device *pdev)

quoted

+	if (of_find_property(node, "vdd-supply", NULL)) {

As mentioned above, that if statement should be removed, since
the regulators shouldn't be optional.

Okay.

quoted

quoted

+		pcie->vdd_supply = regulator_get(&pdev->dev, "vdd");

Those could be devm_regulator_get(). Then tegra_pcie_remove()
wouldn't have to put() them.

Okay.

quoted

quoted

+	for (i = 0; i < TEGRA_PCIE_MAX_PORTS; i++) +
pdata->enable_ports[i] = true;

Shouldn't the DT indicate which ports are used? I assume there's
some reason that the existing driver allows that to be
configured, rather than always enabling all ports. At least,
enumeration time wasted on non-existent ports springs to mind,
and perhaps attempting to enable port 1 when port 0 is x4 and
using all the lanes would cause errors in port 0?

Yes, that's been on my mind as well. I'm not sure about the best
binding for this. Perhaps something like:

pci { enable-ports = <0 1 2>; };

Would do?

That seems reasonable, although since the property is presumably
something specific to the Tegra PCIe binding, not generic, I think it
should be nvidia,enable-ports.