ARM: tegra: Add PCIe device tree support

From: Grant Likely<[email protected]>

Allow drivers to report at probe time that they cannot get all the resources required by the device, and should be retried at a later time.

This should completely solve the problem of getting devices initialized in the right order. Right now this is mostly handled by mucking about with initcall ordering which is a complete hack, and doesn't even remotely handle the case where device drivers are in modules. This approach completely sidesteps the issues by allowing driver registration to occur in any order, and any driver can request to be retried after a few more other drivers get probed.

v4: - Integrate Manjunath's addition of a separate workqueue
- Change -EAGAIN to -EPROBE_DEFER for drivers to trigger deferral
- Update comment blocks to reflect how the code really works
v3: - Hold off workqueue scheduling until late_initcall so that the bulk
of driver probes are complete before we start retrying deferred devices.
- Tested with simple use cases. Still needs more testing though. Using it to get rid of the gpio early_initcall madness, or to replace the ASoC internal probe deferral code would be ideal.
v2: - added locking so it should no longer be utterly broken in that regard
- remove device from deferred list at device_del time.
- Still completely untested with any real use case, but has been boot tested.

Signed-off-by Grant Likely<[email protected]>

[Cc list stripped in order not to get on people's nerves]

---
  drivers/base/base.h    |    1 +
  drivers/base/core.c    |    2 +
  drivers/base/dd.c      |  138 +++++++++++++++++++++++++++++++++++++++++++++++-
  include/linux/device.h |    5 ++
  include/linux/errno.h  |    1 +
  5 files changed, 146 insertions(+), 1 deletion(-)

Quoted 28 lines · Thierry Reding

From: Grant Likely<[email protected]>

Allow drivers to report at probe time that they cannot get all the resources required by the device, and should be retried at a later time.

This should completely solve the problem of getting devices initialized in the right order. Right now this is mostly handled by mucking about with initcall ordering which is a complete hack, and doesn't even remotely handle the case where device drivers are in modules. This approach completely sidesteps the issues by allowing driver registration to occur in any order, and any driver can request to be retried after a few more other drivers get probed.

v4: - Integrate Manjunath's addition of a separate workqueue
- Change -EAGAIN to -EPROBE_DEFER for drivers to trigger deferral
- Update comment blocks to reflect how the code really works
v3: - Hold off workqueue scheduling until late_initcall so that the bulk
of driver probes are complete before we start retrying deferred devices.
- Tested with simple use cases. Still needs more testing though. Using it to get rid of the gpio early_initcall madness, or to replace the ASoC internal probe deferral code would be ideal.
v2: - added locking so it should no longer be utterly broken in that regard
- remove device from deferred list at device_del time.
- Still completely untested with any real use case, but has been boot tested.

Signed-off-by Grant Likely<[email protected]>

[Cc list stripped in order not to get on people's nerves]

Quoted 7 lines

---
  drivers/base/base.h    |    1 +
  drivers/base/core.c    |    2 +
  drivers/base/dd.c      |  138 +++++++++++++++++++++++++++++++++++++++++++++++-
  include/linux/device.h |    5 ++
  include/linux/errno.h  |    1 +
  5 files changed, 146 insertions(+), 1 deletion(-)

Is this patch going to be included in v3.4 ? I can see it's in -next, but not sure where I could check if its really queued for v3.4. It would be nice to have it in v3.4, I've got already one more client of this deferred probe infrastructure.

If the specified GPIO is not found, return -EPROBE_DEFER. This will cause the driver to be probed again later when the required GPIO may have become available.

Signed-off-by Thierry Reding <[email protected]>

Documentation/devicetree/bindings/regulator/tps6586x.txt line 11

Quoted 4 lines · Thierry Reding

+- gpio-controller: mark the device as a GPIO controller
+- regulators: list of regulators provided by this controller, must be in the

Documentation/devicetree/bindings/regulator/tps6586x.txt new file +2 -0

+  following order:
+    SM0, SM1, SM2, LDO0, LDO1, LDO2, LDO3, LDO4, LDO5, LDO6, LDO7, LDO8, LDO9

This ordering requirement is fairly sad, if there are unused regulators they still need to be listed even though...

Documentation/devicetree/bindings/regulator/tps6586x.txt line 33

Quoted 8 lines

Documentation/devicetree/bindings/regulator/tps6586x.txt new file +8 -0

+			sm0_reg: sm0 {
+				regulator-min-microvolt = < 725000>;
+				regulator-max-microvolt = <1500000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			sm1_reg: sm1 {

...they all seem to be explicitly named in the device tree so presumably there's enough information in there for the driver to pick any set of regulators in any order. This would be much nicer to use.

Quoted 3 lines · Mark Brown

...they all seem to be explicitly named in the device tree so presumably there's enough information in there for the driver to pick any set of regulators in any order. This would be much nicer to use.

I don't like it much either. The only reason that requirement exists is because it makes the assignment of the regulator ID (as defined in the include/linux/mfd/tps6586x.h header) very trivial. Would it be better to look up the ID based on the node name (sm0 --> TPS6586X_ID_SM_0, ...)?

Then the only requirement would be that the names match.

Quoted 5 lines · Thierry Reding

Quoted 3 lines · Mark Brown

...they all seem to be explicitly named in the device tree so presumably there's enough information in there for the driver to pick any set of regulators in any order. This would be much nicer to use.

Quoted 6 lines

I don't like it much either. The only reason that requirement exists is because it makes the assignment of the regulator ID (as defined in the include/linux/mfd/tps6586x.h header) very trivial. Would it be better to look up the ID based on the node name (sm0 --> TPS6586X_ID_SM_0, ...)?

Then the only requirement would be that the names match.

Yes, that's going to be more code but much nicer for users.

Quoted 9 lines · Mark Brown

Documentation/devicetree/bindings/regulator/tps6586x.txt line 11

Quoted 4 lines · Thierry Reding

+- gpio-controller: mark the device as a GPIO controller
+- regulators: list of regulators provided by this controller, must be in the

Documentation/devicetree/bindings/regulator/tps6586x.txt new file +2 -0

+  following order:
+    SM0, SM1, SM2, LDO0, LDO1, LDO2, LDO3, LDO4, LDO5, LDO6, LDO7, LDO8, LDO9

This ordering requirement is fairly sad, if there are unused regulators they still need to be listed even though...

Quoted 13 lines

Documentation/devicetree/bindings/regulator/tps6586x.txt line 33

Quoted 9 lines

Documentation/devicetree/bindings/regulator/tps6586x.txt new file +8 -0

+			sm0_reg: sm0 {
+				regulator-min-microvolt = < 725000>;
+				regulator-max-microvolt = <1500000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			sm1_reg: sm1 {

...they all seem to be explicitly named in the device tree so presumably there's enough information in there for the driver to pick any set of regulators in any order. This would be much nicer to use.

I don't like it much either. The only reason that requirement exists is because it makes the assignment of the regulator ID (as defined in the include/linux/mfd/tps6586x.h header) very trivial. Would it be better to look up the ID based on the node name (sm0 --> TPS6586X_ID_SM_0, ...)?

Then the only requirement would be that the names match.

Quoted 10 lines · Thierry Reding

Quoted 3 lines · Mark Brown

...they all seem to be explicitly named in the device tree so presumably there's enough information in there for the driver to pick any set of regulators in any order. This would be much nicer to use.

I don't like it much either. The only reason that requirement exists is because it makes the assignment of the regulator ID (as defined in the include/linux/mfd/tps6586x.h header) very trivial. Would it be better to look up the ID based on the node name (sm0 --> TPS6586X_ID_SM_0, ...)?

Quoted 1 line

Then the only requirement would be that the names match.

Yes, please look up id via name. Alternately you can give each child node a 'reg' property and put #address-cells = <1>; #size-cells = <0>; in the parent (assuming the regulator number is a documented attribute of the hardware and not just a convenient linux construct).

Signed-off-by Thierry Reding <[email protected]>

---
?drivers/pci/setup-irq.c | ? ?4 ++--
?1 file changed, 2 insertions(+), 2 deletions(-)

drivers/pci/setup-irq.c +0 -0

index eb219a1..270ae7b 100644
@@ -18,7 +18,7 @@

?#include <linux/cache.h>

drivers/pci/setup-irq.c +1 -1

-static void __init
+static void

?pdev_fixup_irq(struct pci_dev *dev,
? ? ? ? ? ? ? u8 (*swizzle)(struct pci_dev *, u8 *), ? ? ? ? ? ? ? int (*map_irq)(const struct pci_dev *, u8, u8))

Hmm, your patch looks OK as far as it goes, but the implementation of pci_fixup_irqs() as a call-once-at-boot-time thing with a loop through all the PCI devices we've found so far looks completely broken with regard to hotplug.

Who does the swizzle for devices hot-added after boot?

Quoted 6 lines · Bjorn Helgaas

Hmm, your patch looks OK as far as it goes, but the implementation of pci_fixup_irqs() as a call-once-at-boot-time thing with a loop through all the PCI devices we've found so far looks completely broken with regard to hotplug.

Who does the swizzle for devices hot-added after boot?

Perhaps I should have mentioned this in the commit message, but the patch isn't actually used in a hotplug scenario. In fact, the code is still run once at boot time, only very late. Grant's deferred probing allows the PCIe controller driver to postpone initialization until all the devices that it depends on (regulators) have been probed. This has the side-effect that the probing actually takes place *after* code and data annotated with __init have already been freed.

The PMC code already accesses the PMC registers so it makes sense to move this function there as well. While at it, rename the function to tegra_pmc_pcie_xclk_clamp() for consistency.

Signed-off-by Thierry Reding <[email protected]>

Signed-off-by Thierry Reding <[email protected]>

arch/arm/mach-tegra/board-harmony-pcie.c +0 -0

Quoted 1 line · Thierry Reding

Signed-off-by Thierry Reding <[email protected]>

Patch description?

Quoted 1 line

arch/arm/mach-tegra/board-harmony-pcie.c +0 -0

...

arch/arm/mach-tegra/board-harmony-pcie.c line 30

Quoted 2 lines

arch/arm/mach-tegra/board-harmony-pcie.c +1 -1

-static int __init harmony_pcie_init(void)
+static int tegra_pcie_init(struct platform_device *pdev)

That name should probably still be called harmony_something() since it's still board-specific.

I wonder how having a per-board callback will integrate with device tree. I guess I'll find out when I review patch 11:-)

arch/arm/mach-tegra/board-trimslice.c static __initdata struct tegra_clk_init_table trimslice_clk_init_table[] = { · line 160

Quoted 6 lines

arch/arm/mach-tegra/board-trimslice.c +1 -0

 
+static int __init trimslice_pci_init(void)
 {
 	if (!machine_is_trimslice())
 		return 0;

You can remove that test now.

arch/arm/mach-tegra/board.h void __init tegra30_init_early(void); · line 43

Quoted 9 lines

arch/arm/mach-tegra/board.h +7 -0

 
+#define TEGRA_PCIE_MAX_PORTS 2
+
+struct tegra_pcie_pdata {
+	int (*init)(struct platform_device *pdev);
+	int (*exit)(struct platform_device *pdev);
+	bool enable_ports[TEGRA_PCIE_MAX_PORTS];
+};

That's a somewhat odd place to put the header; a dedicated mach-tegra/include/mach/tegra_pcie_pdata.h or include/linux/platform_data/tegra_pcie.h might make more sense.

Quoted 2 lines · Stephen Warren

Quoted 1 line · Thierry Reding

Signed-off-by Thierry Reding <[email protected]>

Quoted 13 lines

arch/arm/mach-tegra/board.h void __init tegra30_init_early(void); · line 43

Quoted 9 lines

arch/arm/mach-tegra/board.h +7 -0

 
+#define TEGRA_PCIE_MAX_PORTS 2
+
+struct tegra_pcie_pdata {
+	int (*init)(struct platform_device *pdev);
+	int (*exit)(struct platform_device *pdev);
+	bool enable_ports[TEGRA_PCIE_MAX_PORTS];
+};

That's a somewhat odd place to put the header; a dedicated mach-tegra/include/mach/tegra_pcie_pdata.h or include/linux/platform_data/tegra_pcie.h might make more sense.

mach-tegra/include/mach/pcie.h perhaps? That would be less redundant.

This commit adds support for message signaled interrupts to the Tegra PCIe controller.

Signed-off-by Thierry Reding <[email protected]>

---
This code is taken from the NVIDIA Vibrante kernel and therefore has no appropriate Signed-off-by from the original author. Maybe someone at NVIDIA can find out who wrote this code and maybe provide a proper Signed-off-by that I can add?

arch/arm/mach-tegra/pcie.c +14 -0

 
+static int tegra_pcie_enable_msi(struct platform_device *pdev)
+{
+	struct tegra_pcie_info *pcie = platform_get_drvdata(pdev);
+	volatile void *pages;
+	unsigned long base;
+	unsigned int msi;
+	int msi_base;
+	int err;
+	u32 reg;
+
+	mutex_init(&pcie->msi_lock);
+
+	msi_base = irq_alloc_descs(-1, 0, INT_PCI_MSI_NR, 0);
+	if (msi_base < 0) {

arch/arm/mach-tegra/pcie.c +11 -0

+		return -ENOMEM;
+	}
+
+	pcie->msi_chip.name = "PCIe-MSI";
+	pcie->msi_chip.irq_enable = unmask_msi_irq;
+	pcie->msi_chip.irq_disable = mask_msi_irq;
+	pcie->msi_chip.irq_mask = mask_msi_irq;
+	pcie->msi_chip.irq_unmask = unmask_msi_irq;
+
+	for (msi = 0; msi < INT_PCI_MSI_NR; msi++) {
+		unsigned int irq = irq_find_mapping(pcie->msi_domain, msi);

arch/arm/mach-tegra/pcie.c +2 -0

+		return err;
+	}

Quoted 9 lines · Thierry Reding

This commit adds support for message signaled interrupts to the Tegra PCIe controller.

Signed-off-by Thierry Reding <[email protected]>

---
This code is taken from the NVIDIA Vibrante kernel and therefore has no appropriate Signed-off-by from the original author. Maybe someone at NVIDIA can find out who wrote this code and maybe provide a proper Signed-off-by that I can add?

I think if you look in:
git://nv-tegra.nvidia.com/linux-2.6.git android-tegra-2.6.36

the following commits are what you're after:

de7fd8768b32da66eaf4eaf58473c65f7a76808d
arm: tegra: pcie: enabling MSI support for pcie

ac1f8310811c64a084511d2afc27f66334b31a81
ARM: tegra: pcie: fix return value from MSI irq routine

Although the patch below only partially resembles those patches, I guess because you've rewritten the code a lot to conform to the current kernel APIs, clean stuff up, etc. Perhaps just saying "based on code by Krishna Kishore <[email protected]>" is enough...

Quoted 21 lines

arch/arm/mach-tegra/pcie.c +18 -0

 
+static int tegra_pcie_enable_msi(struct platform_device *pdev)
+{
+	struct tegra_pcie_info *pcie = platform_get_drvdata(pdev);
+	volatile void *pages;
+	unsigned long base;
+	unsigned int msi;
+	int msi_base;
+	int err;
+	u32 reg;
+
+	mutex_init(&pcie->msi_lock);
+
+	msi_base = irq_alloc_descs(-1, 0, INT_PCI_MSI_NR, 0);
+	if (msi_base < 0) {
+		dev_err(&pdev->dev, "failed to allocate IRQs\n");
+		return msi_base;
+	}
+

arch/arm/mach-tegra/pcie.c static void __devinit tegra_pcie_add_port(struct tegra_pcie_info *pcie, · line 1100

Quoted 6 lines

arch/arm/mach-tegra/pcie.c +6 -0

+	pcie->msi_domain = irq_domain_add_legacy(pcie->dev->of_node,
+						 INT_PCI_MSI_NR, msi_base,
+						 0, &irq_domain_simple_ops,
+						 NULL);
+	if (!pcie->msi_domain) {
+		dev_err(&pdev->dev, "failed to create IRQ domain\n");

Free the IRQ descriptors in the error paths?

Quoted 15 lines

arch/arm/mach-tegra/pcie.c +15 -0

+		return -ENOMEM;
+	}
+
+	pcie->msi_chip.name = "PCIe-MSI";
+	pcie->msi_chip.irq_enable = unmask_msi_irq;
+	pcie->msi_chip.irq_disable = mask_msi_irq;
+	pcie->msi_chip.irq_mask = mask_msi_irq;
+	pcie->msi_chip.irq_unmask = unmask_msi_irq;
+
+	for (msi = 0; msi < INT_PCI_MSI_NR; msi++) {
+		unsigned int irq = irq_find_mapping(pcie->msi_domain, msi);
+
+		irq_set_chip_data(irq, pcie);
+		irq_set_chip_and_handler(irq, &pcie->msi_chip,
+					 handle_simple_irq);

arch/arm/mach-tegra/pcie.c static void __devinit tegra_pcie_add_port(struct tegra_pcie_info *pcie, · line 1121

Quoted 6 lines

arch/arm/mach-tegra/pcie.c +6 -0

+		set_irq_flags(irq, IRQF_VALID);
+	}
+
+	err = platform_get_irq(pdev, 1);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to get IRQ: %d\n", err);

Same here, and undo setting IRQF_VALID?

Quoted 2 lines

arch/arm/mach-tegra/pcie.c +2 -0

+		return err;
+	}

...

arch/arm/mach-tegra/pcie.c static void __devinit tegra_pcie_add_port(struct tegra_pcie_info *pcie, · line 1160

Quoted 4 lines

arch/arm/mach-tegra/pcie.c +4 -0

+static int tegra_pcie_disable_msi(struct platform_device *pdev)
+{
+	return 0;
+}

This is empty in both the ifdef(CONFIG_PCI_MSI) case and otherwise. It should probably clean everything up here right?

Quoted 19 lines · Stephen Warren

Quoted 9 lines · Thierry Reding

This commit adds support for message signaled interrupts to the Tegra PCIe controller.

Signed-off-by Thierry Reding <[email protected]>

---
This code is taken from the NVIDIA Vibrante kernel and therefore has no appropriate Signed-off-by from the original author. Maybe someone at NVIDIA can find out who wrote this code and maybe provide a proper Signed-off-by that I can add?

I think if you look in:
git://nv-tegra.nvidia.com/linux-2.6.git android-tegra-2.6.36

the following commits are what you're after:

de7fd8768b32da66eaf4eaf58473c65f7a76808d
arm: tegra: pcie: enabling MSI support for pcie

Quoted 7 lines

ac1f8310811c64a084511d2afc27f66334b31a81
ARM: tegra: pcie: fix return value from MSI irq routine

Although the patch below only partially resembles those patches, I guess because you've rewritten the code a lot to conform to the current kernel APIs, clean stuff up, etc. Perhaps just saying "based on code by Krishna Kishore <[email protected]>" is enough...

Yes, it is indeed a major rewrite because the original code had some peculiarities and FIXME that I thought wouldn't make it through the review anyway so I fixed them up.

I'll add some comment about the original authorship. There is no official Signed-off-by in the original commit. Do I still need one or is it enough to mention the original authors in the commit message and add keep my own Signed-off-by?

Quoted 16 lines · Thierry Reding

arch/arm/mach-tegra/pcie.c +14 -0

 
+static int tegra_pcie_enable_msi(struct platform_device *pdev)
+{
+	struct tegra_pcie_info *pcie = platform_get_drvdata(pdev);
+	volatile void *pages;
+	unsigned long base;
+	unsigned int msi;
+	int msi_base;
+	int err;
+	u32 reg;
+
+	mutex_init(&pcie->msi_lock);
+
+	msi_base = irq_alloc_descs(-1, 0, INT_PCI_MSI_NR, 0);
+	if (msi_base < 0) {

arch/arm/mach-tegra/pcie.c static void __devinit tegra_pcie_add_port(struct tegra_pcie_info *pcie, · line 1100

Quoted 10 lines

arch/arm/mach-tegra/pcie.c +10 -0

+		dev_err(&pdev->dev, "failed to allocate IRQs\n");
+		return msi_base;
+	}
+
+	pcie->msi_domain = irq_domain_add_legacy(pcie->dev->of_node,
+						 INT_PCI_MSI_NR, msi_base,
+						 0, &irq_domain_simple_ops,
+						 NULL);
+	if (!pcie->msi_domain) {
+		dev_err(&pdev->dev, "failed to create IRQ domain\n");

Free the IRQ descriptors in the error paths?

Quoted 15 lines

arch/arm/mach-tegra/pcie.c +15 -0

+		return -ENOMEM;
+	}
+
+	pcie->msi_chip.name = "PCIe-MSI";
+	pcie->msi_chip.irq_enable = unmask_msi_irq;
+	pcie->msi_chip.irq_disable = mask_msi_irq;
+	pcie->msi_chip.irq_mask = mask_msi_irq;
+	pcie->msi_chip.irq_unmask = unmask_msi_irq;
+
+	for (msi = 0; msi < INT_PCI_MSI_NR; msi++) {
+		unsigned int irq = irq_find_mapping(pcie->msi_domain, msi);
+
+		irq_set_chip_data(irq, pcie);
+		irq_set_chip_and_handler(irq, &pcie->msi_chip,
+					 handle_simple_irq);

arch/arm/mach-tegra/pcie.c static void __devinit tegra_pcie_add_port(struct tegra_pcie_info *pcie, · line 1121

Quoted 6 lines

arch/arm/mach-tegra/pcie.c +6 -0

+		set_irq_flags(irq, IRQF_VALID);
+	}
+
+	err = platform_get_irq(pdev, 1);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to get IRQ: %d\n", err);

Same here, and undo setting IRQF_VALID?

Quoted 2 lines · Stephen Warren

Quoted 1 line · Thierry Reding

arch/arm/mach-tegra/pcie.c +0 -0

Quoted 1 line

Free the IRQ descriptors in the error paths?

Quoted 6 lines

Quoted 6 lines

arch/arm/mach-tegra/pcie.c +6 -0

+	for (msi = 0; msi < INT_PCI_MSI_NR; msi++) {
+		unsigned int irq = irq_find_mapping(pcie->msi_domain, msi);
+
+		irq_set_chip_data(irq, pcie);
+		irq_set_chip_and_handler(irq, &pcie->msi_chip,
+					 handle_simple_irq);

Quoted 8 lines

arch/arm/mach-tegra/pcie.c static void __devinit tegra_pcie_add_port(struct tegra_pcie_info *pcie, · line 1121

Quoted 6 lines

arch/arm/mach-tegra/pcie.c +6 -0

+		set_irq_flags(irq, IRQF_VALID);
+	}
+
+	err = platform_get_irq(pdev, 1);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to get IRQ: %d\n", err);

Same here, and undo setting IRQF_VALID?

Does it make sense to explicitly unset the IRQF_VALID flag when the IRQ descriptors are free'd afterwards anyway?

I'm also Cc'ing Thomas
Gleixner as maintainer of the IRQ subsystem, he probably knows best how dynamically allocated interrupts should be cleaned up.

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

Signed-off-by Thierry Reding <[email protected]>

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

Quoted 7 lines · Thierry Reding

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

Signed-off-by Thierry Reding <[email protected]>

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

Documentation/devicetree/bindings/pci/tegra-pcie.txt line 10

Quoted 10 lines

Documentation/devicetree/bindings/pci/tegra-pcie.txt +10 -0

+NVIDIA Tegra PCIe controller
+
+Required properties:
+- compatible: "nvidia,tegra20-pcie"
+- reg: physical base address and length of the controller's registers
+- interrupts: the interrupt outputs of the controller
+
+Optional properties:
+- pex-clk-supply: supply voltage for internal reference clock
+- vdd-supply: power supply for controller (1.05V)

Mark, Liam,

Should those *-supply properties really be optional? I got the impression talking to Mark in a different thread that all regulators should exist, although their implementation may be via a fixed regulator, with no GPIO, as determined by the board design.

Quoted 21 lines · Stephen Warren

Quoted 7 lines · Thierry Reding

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

Signed-off-by Thierry Reding <[email protected]>

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

Documentation/devicetree/bindings/pci/tegra-pcie.txt line 10

Quoted 10 lines

Documentation/devicetree/bindings/pci/tegra-pcie.txt +10 -0

+NVIDIA Tegra PCIe controller
+
+Required properties:
+- compatible: "nvidia,tegra20-pcie"
+- reg: physical base address and length of the controller's registers
+- interrupts: the interrupt outputs of the controller
+
+Optional properties:
+- pex-clk-supply: supply voltage for internal reference clock
+- vdd-supply: power supply for controller (1.05V)

Mark, Liam,

Quoted 4 lines

Should those *-supply properties really be optional? I got the impression talking to Mark in a different thread that all regulators should exist, although their implementation may be via a fixed regulator, with no GPIO, as determined by the board design.

If I read the TrimSlice code correctly, there is no regulator setup required to make PCIe work there presumably because they hardwire the proper voltages.

On the other hand, maybe a better solution in this case would be to use the dummy regulator in those cases.

Quoted 1 line · Thierry Reding

+- vdd-supply: power supply for controller (1.05V)

Should those *-supply properties really be optional? I got the impression talking to Mark in a different thread that all regulators should exist, although their implementation may be via a fixed regulator, with no GPIO, as determined by the board design.

Quoted 8 lines · Stephen Warren

Quoted 1 line · Thierry Reding

+- vdd-supply: power supply for controller (1.05V)

Should those *-supply properties really be optional? I got the impression talking to Mark in a different thread that all regulators should exist, although their implementation may be via a fixed regulator, with no GPIO, as determined by the board design.

Depends on the electronics, if the device can function without a supply then it is OK for it to be optional. This is moderately unusual but not unknown. It's obviously also OK to omit the supply if the system doesn't use software regulators at all, though a kernel with regulator support enabled would need to use dummy regulators to stub things out.

My understanding of the fixed regulator was that it was meant to be used for fixed voltage supplies that can still be enabled or disabled (for example as supplied by a GPIO) but not regulators that are fix in the sense that they cannot be enabled or disabled at all but are always on.

Is that how it is supposed to work?

Quoted 4 lines · Thierry Reding

My understanding of the fixed regulator was that it was meant to be used for fixed voltage supplies that can still be enabled or disabled (for example as supplied by a GPIO) but not regulators that are fix in the sense that they cannot be enabled or disabled at all but are always on.

No, not at all. It's for any random supply that doesn't have much meaningful control - as you'll see from the code the GPIO is totally optional if you look at the git history you'll see that the GPIO was added some time after the driver was originally merged.

Quoted 1 line

Is that how it is supposed to work?

Yes.

In that case I'll mark the *-supply properties required. Would it be a good idea to mention this (or even give an example with fixed regulators) in the binding documentation or can I assume this to be common knowledge?

This patch series adds support for device tree based probing of the PCIe controller found on Tegra SoCs.

Patches 1 and 2 have already been posted by Grant and Mark respectively but I've included them here for reference. Both patches are dependencies for the subsequent patches.

Patches 3, 4 and 5 add support for obtaining the regulators required to power the PCIe controller from the device tree. Patches 6 and 7 are required to make some common PCI functions available after the init phase because the deferred probing will postpone the probing of the PCIe controller driver until that point.

Patches 8 and 9 make the PCIe controller code available as a driver and add the corresponding platform devices to the boards that enable PCIe (Harmony and TrimSlice). Patch 10 adds MSI support and patch 11 finally adds a device tree binding with documentation.

The code in patch 10 (MSI support) is based on the Vibrante kernel and I wasn't able to get a proper Signed-off-by or even copyright information. Perhaps somebody at NVIDIA can find out?

Thierry

Grant Likely (1):
drivercore: Add driver probe deferral mechanism

Mark Brown (1):
regulator: Support driver probe deferral

Thierry Reding (9):
regulator: fixed: Support driver probe deferral
regulator: tps6586x: fix typo in debug message
tps6586x: Add device-tree support
PCI: Keep pci_fixup_irqs() around after init
ARM: pci: Keep pci_common_init() around after init
ARM: tegra: Move tegra_pcie_xclk_clamp() to PMC
ARM: tegra: Rewrite PCIe support as a driver
ARM: tegra: pcie: Add MSI support
ARM: tegra: pcie: Add device tree support

 .../devicetree/bindings/pci/tegra-pcie.txt         |   23 +
 .../devicetree/bindings/regulator/tps6586x.txt     |   98 +++
 arch/arm/boot/dts/tegra20.dtsi                     |    8 +
 arch/arm/kernel/bios32.c                           |    4 +-
 arch/arm/mach-tegra/Kconfig                        |    1 +
 arch/arm/mach-tegra/board-harmony-pcie.c           |   38 +-
 arch/arm/mach-tegra/board-harmony.c                |    1 +
 arch/arm/mach-tegra/board-harmony.h                |    1 +
 arch/arm/mach-tegra/board-trimslice.c              |   16 +-
 arch/arm/mach-tegra/board.h                        |   11 +-
 arch/arm/mach-tegra/devices.c                      |   32 +
 arch/arm/mach-tegra/devices.h                      |    1 +
 arch/arm/mach-tegra/include/mach/iomap.h           |    6 +
 arch/arm/mach-tegra/include/mach/irqs.h            |    5 +-
 arch/arm/mach-tegra/pcie.c                         |  895 +++++++++++++++-----
 arch/arm/mach-tegra/pmc.c                          |   16 +
 arch/arm/mach-tegra/pmc.h                          |    1 +
 drivers/base/base.h                                |    1 +
 drivers/base/core.c                                |    2 +
 drivers/base/dd.c                                  |  138 ++-
 drivers/mfd/tps6586x.c                             |   66 ++
 drivers/pci/setup-irq.c                            |    4 +-
 drivers/regulator/core.c                           |    4 +-

 drivers/regulator/fixed.c                          |   14 +-
 drivers/regulator/tps6586x-regulator.c             |    4 +-
 include/linux/device.h                             |    5 +
 include/linux/errno.h                              |    1 +
 include/linux/mfd/tps6586x.h                       |    1 +
 28 files changed, 1136 insertions(+), 261 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/tegra-pcie.txt
 create mode 100644 Documentation/devicetree/bindings/regulator/tps6586x.txt

There is a whole bunch of PCI DT code sitting in the powerpc tree that probably needs to be moved to common location and used here.

This looks way too simple to be what's needed if I compare to PCI bindings in powerpc dts files.

This patch series adds support for device tree based probing of the PCIe controller found on Tegra SoCs.

Patches 1 and 2 have already been posted by Grant and Mark respectively but I've included them here for reference. Both patches are dependencies for the subsequent patches.

Patches 3, 4 and 5 add support for obtaining the regulators required to power the PCIe controller from the device tree. Patches 6 and 7 are required to make some common PCI functions available after the init phase because the deferred probing will postpone the probing of the PCIe controller driver until that point.

Patches 8 and 9 make the PCIe controller code available as a driver and add the corresponding platform devices to the boards that enable PCIe (Harmony and TrimSlice). Patch 10 adds MSI support and patch 11 finally adds a device tree binding with documentation.

The code in patch 10 (MSI support) is based on the Vibrante kernel and I wasn't able to get a proper Signed-off-by or even copyright information. Perhaps somebody at NVIDIA can find out?

Thierry

Grant Likely (1):
drivercore: Add driver probe deferral mechanism

Mark Brown (1):
regulator: Support driver probe deferral

This patch series adds support for device tree based probing of the PCIe controller found on Tegra SoCs.

Patches 1 and 2 have already been posted by Grant and Mark respectively but I've included them here for reference. Both patches are dependencies for the subsequent patches.

Patches 3, 4 and 5 add support for obtaining the regulators required to power the PCIe controller from the device tree. Patches 6 and 7 are required to make some common PCI functions available after the init phase because the deferred probing will postpone the probing of the PCIe controller driver until that point.

Patches 8 and 9 make the PCIe controller code available as a driver and add the corresponding platform devices to the boards that enable PCIe (Harmony and TrimSlice). Patch 10 adds MSI support and patch 11 finally adds a device tree binding with documentation.

The code in patch 10 (MSI support) is based on the Vibrante kernel and I wasn't able to get a proper Signed-off-by or even copyright information. Perhaps somebody at NVIDIA can find out?