[PATCH 00/11] ARM: tegra: Add PCIe device tree support

[PATCH 00/11] ARM: tegra: Add PCIe device tree support

[Thierry Reding]

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

-- 
1.7.9.3

[PATCH 01/11] drivercore: Add driver probe deferral mechanism

[Thierry Reding]

From: Grant Likely <grant.likely@secretlab.ca>

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 <grant.likely@secretlab.ca>
[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(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index b858dfd..2c13dea 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -105,6 +105,7 @@ extern void bus_remove_driver(struct device_driver *drv);
 
 extern void driver_detach(struct device_driver *drv);
 extern int driver_probe_device(struct device_driver *drv, struct device *dev);
+extern void driver_deferred_probe_del(struct device *dev);
 static inline int driver_match_device(struct device_driver *drv,
 				      struct device *dev)
 {
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 0841a36..946094f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -649,6 +649,7 @@ void device_initialize(struct device *dev)
 {
 	dev->kobj.kset = devices_kset;
 	kobject_init(&dev->kobj, &device_ktype);
+	INIT_LIST_HEAD(&dev->deferred_probe);
 	INIT_LIST_HEAD(&dev->dma_pools);
 	mutex_init(&dev->mutex);
 	lockdep_set_novalidate_class(&dev->mutex);
@@ -1193,6 +1194,7 @@ void device_del(struct device *dev)
 	device_remove_file(dev, &uevent_attr);
 	device_remove_attrs(dev);
 	bus_remove_device(dev);
+	driver_deferred_probe_del(dev);
 
 	/*
 	 * Some platform devices are driven without driver attached
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 142e3d600..442b764 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -28,6 +28,133 @@
 #include "base.h"
 #include "power/power.h"
 
+/*
+ * Deferred Probe infrastructure.
+ *
+ * Sometimes driver probe order matters, but the kernel doesn't always have
+ * dependency information which means some drivers will get probed before a
+ * resource it depends on is available.  For example, an SDHCI driver may
+ * first need a GPIO line from an i2c GPIO controller before it can be
+ * initialized.  If a required resource is not available yet, a driver can
+ * request probing to be deferred by returning -EPROBE_DEFER from its probe hook
+ *
+ * Deferred probe maintains two lists of devices, a pending list and an active
+ * list.  A driver returning -EPROBE_DEFER causes the device to be added to the
+ * pending list.  A successful driver probe will trigger moving all devices
+ * from the pending to the active list so that the workqueue will eventually
+ * retry them.
+ *
+ * The deferred_probe_mutex must be held any time the deferred_probe_*_list
+ * of the (struct device*)->deferred_probe pointers are manipulated
+ */
+static DEFINE_MUTEX(deferred_probe_mutex);
+static LIST_HEAD(deferred_probe_pending_list);
+static LIST_HEAD(deferred_probe_active_list);
+static struct workqueue_struct *deferred_wq;
+
+/**
+ * deferred_probe_work_func() - Retry probing devices in the active list.
+ */
+static void deferred_probe_work_func(struct work_struct *work)
+{
+	struct device *dev;
+	/*
+	 * This block processes every device in the deferred 'active' list.
+	 * Each device is removed from the active list and passed to
+	 * bus_probe_device() to re-attempt the probe.  The loop continues
+	 * until every device in the active list is removed and retried.
+	 *
+	 * Note: Once the device is removed from the list and the mutex is
+	 * released, it is possible for the device get freed by another thread
+	 * and cause a illegal pointer dereference.  This code uses
+	 * get/put_device() to ensure the device structure cannot disappear
+	 * from under our feet.
+	 */
+	mutex_lock(&deferred_probe_mutex);
+	while (!list_empty(&deferred_probe_active_list)) {
+		dev = list_first_entry(&deferred_probe_active_list,
+					typeof(*dev), deferred_probe);
+		list_del_init(&dev->deferred_probe);
+
+		get_device(dev);
+
+		/* Drop the mutex while probing each device; the probe path
+		 * may manipulate the deferred list */
+		mutex_unlock(&deferred_probe_mutex);
+		dev_dbg(dev, "Retrying from deferred list\n");
+		bus_probe_device(dev);
+		mutex_lock(&deferred_probe_mutex);
+
+		put_device(dev);
+	}
+	mutex_unlock(&deferred_probe_mutex);
+}
+static DECLARE_WORK(deferred_probe_work, deferred_probe_work_func);
+
+static void driver_deferred_probe_add(struct device *dev)
+{
+	mutex_lock(&deferred_probe_mutex);
+	if (list_empty(&dev->deferred_probe)) {
+		dev_dbg(dev, "Added to deferred list\n");
+		list_add(&dev->deferred_probe, &deferred_probe_pending_list);
+	}
+	mutex_unlock(&deferred_probe_mutex);
+}
+
+void driver_deferred_probe_del(struct device *dev)
+{
+	mutex_lock(&deferred_probe_mutex);
+	if (!list_empty(&dev->deferred_probe)) {
+		dev_dbg(dev, "Removed from deferred list\n");
+		list_del_init(&dev->deferred_probe);
+	}
+	mutex_unlock(&deferred_probe_mutex);
+}
+
+static bool driver_deferred_probe_enable = false;
+/**
+ * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
+ *
+ * This functions moves all devices from the pending list to the active
+ * list and schedules the deferred probe workqueue to process them.  It
+ * should be called anytime a driver is successfully bound to a device.
+ */
+static void driver_deferred_probe_trigger(void)
+{
+	if (!driver_deferred_probe_enable)
+		return;
+
+	/* A successful probe means that all the devices in the pending list
+	 * should be triggered to be reprobed.  Move all the deferred devices
+	 * into the active list so they can be retried by the workqueue */
+	mutex_lock(&deferred_probe_mutex);
+	list_splice_tail_init(&deferred_probe_pending_list,
+			      &deferred_probe_active_list);
+	mutex_unlock(&deferred_probe_mutex);
+
+	/* Kick the re-probe thread.  It may already be scheduled, but
+	 * it is safe to kick it again. */
+	queue_work(deferred_wq, &deferred_probe_work);
+}
+
+/**
+ * deferred_probe_initcall() - Enable probing of deferred devices
+ *
+ * We don't want to get in the way when the bulk of drivers are getting probed.
+ * Instead, this initcall makes sure that deferred probing is delayed until
+ * late_initcall time.
+ */
+static int deferred_probe_initcall(void)
+{
+	deferred_wq = create_singlethread_workqueue("deferwq");
+	if (WARN_ON(!deferred_wq))
+		return -ENOMEM;
+
+	driver_deferred_probe_enable = true;
+	driver_deferred_probe_trigger();
+	return 0;
+}
+late_initcall(deferred_probe_initcall);
 
 static void driver_bound(struct device *dev)
 {
@@ -42,6 +169,11 @@ static void driver_bound(struct device *dev)
 
 	klist_add_tail(&dev->p->knode_driver, &dev->driver->p->klist_devices);
 
+	/* Make sure the device is no longer in one of the deferred lists
+	 * and kick off retrying all pending devices */
+	driver_deferred_probe_del(dev);
+	driver_deferred_probe_trigger();
+
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_BOUND_DRIVER, dev);
@@ -142,7 +274,11 @@ probe_failed:
 	driver_sysfs_remove(dev);
 	dev->driver = NULL;
 
-	if (ret != -ENODEV && ret != -ENXIO) {
+	if (ret == -EPROBE_DEFER) {
+		/* Driver requested deferred probing */
+		dev_info(dev, "Driver %s requests probe deferral\n", drv->name);
+		driver_deferred_probe_add(dev);
+	} else if (ret != -ENODEV && ret != -ENXIO) {
 		/* driver matched but the probe failed */
 		printk(KERN_WARNING
 		       "%s: probe of %s failed with error %d\n",
diff --git a/include/linux/device.h b/include/linux/device.h
index f62e216..22d6938 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -585,6 +585,10 @@ struct device_dma_parameters {
  * @mutex:	Mutex to synchronize calls to its driver.
  * @bus:	Type of bus device is on.
  * @driver:	Which driver has allocated this
+ * @deferred_probe: entry in deferred_probe_list which is used to retry the
+ * 		binding of drivers which were unable to get all the resources
+ * 		needed by the device; typically because it depends on another
+ * 		driver getting probed first.
  * @platform_data: Platform data specific to the device.
  * 		Example: For devices on custom boards, as typical of embedded
  * 		and SOC based hardware, Linux often uses platform_data to point
@@ -644,6 +648,7 @@ struct device {
 	struct bus_type	*bus;		/* type of bus device is on */
 	struct device_driver *driver;	/* which driver has allocated this
 					   device */
+	struct list_head	deferred_probe;
 	void		*platform_data;	/* Platform specific data, device
 					   core doesn't touch it */
 	struct dev_pm_info	power;
diff --git a/include/linux/errno.h b/include/linux/errno.h
index 4668583..2d09bfa 100644
--- a/include/linux/errno.h
+++ b/include/linux/errno.h
@@ -16,6 +16,7 @@
 #define ERESTARTNOHAND	514	/* restart if no handler.. */
 #define ENOIOCTLCMD	515	/* No ioctl command */
 #define ERESTART_RESTARTBLOCK 516 /* restart by calling sys_restart_syscall */
+#define EPROBE_DEFER	517	/* Driver requests probe retry */
 
 /* Defined for the NFSv3 protocol */
 #define EBADHANDLE	521	/* Illegal NFS file handle */
-- 
1.7.9.3

Re: [PATCH 01/11] drivercore: Add driver probe deferral mechanism

[Sylwester Nawrocki]

On 03/08/2012 03:51 PM, Thierry Reding wrote:
> From: Grant Likely<grant.likely@secretlab.ca>
>
> 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<grant.likely@secretlab.ca>
> [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(-)

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.

--

Thanks,
Sylwester

Re: [PATCH 01/11] drivercore: Add driver probe deferral mechanism

[Grant Likely]

On Tue, 20 Mar 2012 00:12:56 +0100, Sylwester Nawrocki <sylvester.nawrocki@gmail.com> wrote:
> On 03/08/2012 03:51 PM, Thierry Reding wrote:
> > From: Grant Likely<grant.likely@secretlab.ca>
> >
> > 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<grant.likely@secretlab.ca>
> > [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(-)
> 
> 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.

Greg has sent it on to Linus for merging, so yes.

g.

[PATCH 02/11] regulator: Support driver probe deferral

[Thierry Reding]

From: Mark Brown <broonie@opensource.wolfsonmicro.com>

If we fail to locate a requested regulator return -EPROBE_DEFER. If drivers
pass this error code through to their caller (which they really should)
then this will ensure that the probe is retried later when further devices
become available.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
[G,Manjunath Kondaiah removed in order not to bother him]
Acked-by: Grant Likely <grant.likely@secretlab.ca>
Acked-by: Liam Girdwood <lrg@ti.com>
---
 drivers/regulator/core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 5a6b8ba..8a4b0a8 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1198,7 +1198,7 @@ static struct regulator *_regulator_get(struct device *dev, const char *id,
 {
 	struct regulator_dev *rdev;
 	struct regulator_map *map;
-	struct regulator *regulator = ERR_PTR(-ENODEV);
+	struct regulator *regulator = ERR_PTR(-EPROBE_DEFER);
 	const char *devname = NULL;
 	int ret;
 
@@ -2926,7 +2926,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
 
 		if (!r) {
 			dev_err(dev, "Failed to find supply %s\n", supply);
-			ret = -ENODEV;
+			ret = -EPROBE_DEFER;
 			goto scrub;
 		}
 
-- 
1.7.9.3

[PATCH 03/11] regulator: fixed: Support driver probe deferral

[Thierry Reding]

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 <thierry.reding@avionic-design.de>
---
 drivers/regulator/fixed.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 40f3803..35cec44 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -58,12 +58,19 @@ of_get_fixed_voltage_config(struct device *dev)
 	struct device_node *np = dev->of_node;
 	const __be32 *delay;
 	struct regulator_init_data *init_data;
+	int gpio;
+
+	gpio = of_get_named_gpio(np, "gpio", 0);
+	if (gpio < 0)
+		return ERR_PTR(-EPROBE_DEFER);
 
 	config = devm_kzalloc(dev, sizeof(struct fixed_voltage_config),
 								 GFP_KERNEL);
 	if (!config)
 		return NULL;
 
+	config->gpio = gpio;
+
 	config->init_data = of_get_regulator_init_data(dev, dev->of_node);
 	if (!config->init_data)
 		return NULL;
@@ -83,7 +90,6 @@ of_get_fixed_voltage_config(struct device *dev)
 	if (init_data->constraints.boot_on)
 		config->enabled_at_boot = true;
 
-	config->gpio = of_get_named_gpio(np, "gpio", 0);
 	delay = of_get_property(np, "startup-delay-us", NULL);
 	if (delay)
 		config->startup_delay = be32_to_cpu(*delay);
@@ -168,9 +174,11 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
 	struct fixed_voltage_data *drvdata;
 	int ret;
 
-	if (pdev->dev.of_node)
+	if (pdev->dev.of_node) {
 		config = of_get_fixed_voltage_config(&pdev->dev);
-	else
+		if (IS_ERR(config))
+			return PTR_ERR(config);
+	} else
 		config = pdev->dev.platform_data;
 
 	if (!config)
-- 
1.7.9.3

Re: [PATCH 03/11] regulator: fixed: Support driver probe deferral

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

On Thu, Mar 08, 2012 at 03:51:23PM +0100, Thierry Reding wrote:
> 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.

Might be worth pushing this into gpiolib?  It seems like wanting to
defer if a GPIO is not found will be the overwhelmingly common case
given that the GPIO numbers need to be explicitly specified.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 04/11] regulator: tps6586x: fix typo in debug message

[Thierry Reding]

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/regulator/tps6586x-regulator.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c
index c75fb20..29b615c 100644
--- a/drivers/regulator/tps6586x-regulator.c
+++ b/drivers/regulator/tps6586x-regulator.c
@@ -383,7 +383,7 @@ static int __devinit tps6586x_regulator_probe(struct platform_device *pdev)
 	int id = pdev->id;
 	int err;
 
-	dev_dbg(&pdev->dev, "Probing reulator %d\n", id);
+	dev_dbg(&pdev->dev, "Probing regulator %d\n", id);
 
 	ri = find_regulator_info(id);
 	if (ri == NULL) {
-- 
1.7.9.3

Re: [PATCH 04/11] regulator: tps6586x: fix typo in debug message

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 149 bytes --]

On Thu, Mar 08, 2012 at 03:51:24PM +0100, Thierry Reding wrote:
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 05/11] tps6586x: Add device-tree support

[Thierry Reding]

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 .../devicetree/bindings/regulator/tps6586x.txt     |   98 ++++++++++++++++++++
 drivers/mfd/tps6586x.c                             |   66 +++++++++++++
 drivers/regulator/tps6586x-regulator.c             |    2 +-
 include/linux/mfd/tps6586x.h                       |    1 +
 4 files changed, 166 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/tps6586x.txt

diff --git a/Documentation/devicetree/bindings/regulator/tps6586x.txt b/Documentation/devicetree/bindings/regulator/tps6586x.txt
new file mode 100644
index 0000000..e8d2888
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/tps6586x.txt
@@ -0,0 +1,98 @@
+TPS6586x family of regulators
+
+Required properties:
+- compatible: "ti,tps6586x"
+- reg: I2C slave address
+- interrupts: the interrupt outputs of the controller
+- #gpio-cells: number of cells to describe a GPIO
+- gpio-controller: mark the device as a GPIO controller
+- regulators: list of regulators provided by this controller, must be in the
+  following order:
+    SM0, SM1, SM2, LDO0, LDO1, LDO2, LDO3, LDO4, LDO5, LDO6, LDO7, LDO8, LDO9
+
+Each regulator is defined using the standard binding for regulators.
+
+Example:
+
+	pmu: tps6586x@34 {
+		compatible = "ti,tps6586x";
+		reg = <0x34>;
+		interrupts = <0 88 0x4>;
+
+		#gpio-cells = <2>;
+		gpio-controller;
+
+		regulators {
+			sm0_reg: sm0 {
+				regulator-min-microvolt = < 725000>;
+				regulator-max-microvolt = <1500000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			sm1_reg: sm1 {
+				regulator-min-microvolt = < 725000>;
+				regulator-max-microvolt = <1500000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			sm2_reg: sm2 {
+				regulator-min-microvolt = <3000000>;
+				regulator-max-microvolt = <4550000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			ldo0_reg: ldo0 {
+				regulator-name = "PCIE CLK";
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo1_reg: ldo1 {
+				regulator-min-microvolt = < 725000>;
+				regulator-max-microvolt = <1500000>;
+			};
+
+			ldo2_reg: ldo2 {
+				regulator-min-microvolt = < 725000>;
+				regulator-max-microvolt = <1500000>;
+			};
+
+			ldo3_reg: ldo3 {
+				regulator-min-microvolt = <1250000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo4_reg: ldo4 {
+				regulator-min-microvolt = <1700000>;
+				regulator-max-microvolt = <2475000>;
+			};
+
+			ldo5_reg: ldo5 {
+				regulator-min-microvolt = <1250000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo6_reg: ldo6 {
+				regulator-min-microvolt = <1250000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo7_reg: ldo7 {
+				regulator-min-microvolt = <1250000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo8_reg: ldo8 {
+				regulator-min-microvolt = <1250000>;
+				regulator-max-microvolt = <3300000>;
+			};
+
+			ldo9_reg: ldo9 {
+				regulator-min-microvolt = <1250000>;
+				regulator-max-microvolt = <3300000>;
+			};
+		};
+	};
diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index a5ddf31..71997d2 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/gpio.h>
 #include <linux/i2c.h>
+#include <linux/regulator/of_regulator.h>
 
 #include <linux/mfd/core.h>
 #include <linux/mfd/tps6586x.h>
@@ -460,6 +461,7 @@ static int __devinit tps6586x_add_subdevs(struct tps6586x *tps6586x,
 
 		pdev->dev.parent = tps6586x->dev;
 		pdev->dev.platform_data = subdev->platform_data;
+		pdev->dev.of_node = subdev->of_node;
 
 		ret = platform_device_add(pdev);
 		if (ret) {
@@ -474,6 +476,66 @@ failed:
 	return ret;
 }
 
+#ifdef CONFIG_OF
+static struct tps6586x_platform_data *tps6586x_parse_dt(struct i2c_client *client)
+{
+	struct device_node *np = client->dev.of_node;
+	struct tps6586x_platform_data *pdata;
+	struct tps6586x_subdev_info *devs;
+	struct device_node *regulators;
+	struct device_node *child;
+	unsigned int num = 0;
+
+	regulators = of_find_node_by_name(np, "regulators");
+
+	for_each_child_of_node(regulators, child)
+		num++;
+
+	devs = devm_kzalloc(&client->dev, num * sizeof(*devs), GFP_KERNEL);
+	if (!devs)
+		return NULL;
+
+	num = 0;
+
+	for_each_child_of_node(regulators, child) {
+		struct regulator_init_data *init_data;
+
+		init_data = of_get_regulator_init_data(&client->dev, child);
+		if (!init_data)
+			dev_err(&client->dev,
+				"failed to parse DT for regulator %s\n",
+				child->name);
+
+		devs[num].name = "tps6586x-regulator";
+		devs[num].platform_data = init_data;
+		devs[num].of_node = child;
+		devs[num].id = num;
+		num++;
+	}
+
+	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	pdata->num_subdevs = num;
+	pdata->subdevs = devs;
+	pdata->gpio_base = -1;
+	pdata->irq_base = -1;
+
+	return pdata;
+}
+
+static struct of_device_id tps6586x_of_match[] = {
+	{ .compatible = "ti,tps6586x", },
+	{ },
+};
+#else
+static struct tps6586x_platform_data *tps6586x_parse_dt(struct i2c_client *client)
+{
+	return NULL;
+}
+#endif
+
 static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
 {
@@ -481,6 +543,9 @@ static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 	struct tps6586x *tps6586x;
 	int ret;
 
+	if (!pdata && client->dev.of_node)
+		pdata = tps6586x_parse_dt(client);
+
 	if (!pdata) {
 		dev_err(&client->dev, "tps6586x requires platform data\n");
 		return -ENOTSUPP;
@@ -573,6 +638,7 @@ static struct i2c_driver tps6586x_driver = {
 	.driver	= {
 		.name	= "tps6586x",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(tps6586x_of_match),
 	},
 	.probe		= tps6586x_i2c_probe,
 	.remove		= __devexit_p(tps6586x_i2c_remove),
diff --git a/drivers/regulator/tps6586x-regulator.c b/drivers/regulator/tps6586x-regulator.c
index 29b615c..f10a735 100644
--- a/drivers/regulator/tps6586x-regulator.c
+++ b/drivers/regulator/tps6586x-regulator.c
@@ -396,7 +396,7 @@ static int __devinit tps6586x_regulator_probe(struct platform_device *pdev)
 		return err;
 
 	rdev = regulator_register(&ri->desc, &pdev->dev,
-				  pdev->dev.platform_data, ri, NULL);
+				  pdev->dev.platform_data, ri, pdev->dev.of_node);
 	if (IS_ERR(rdev)) {
 		dev_err(&pdev->dev, "failed to register regulator %s\n",
 				ri->desc.name);
diff --git a/include/linux/mfd/tps6586x.h b/include/linux/mfd/tps6586x.h
index b19176e..f350fd0 100644
--- a/include/linux/mfd/tps6586x.h
+++ b/include/linux/mfd/tps6586x.h
@@ -68,6 +68,7 @@ struct tps6586x_subdev_info {
 	int		id;
 	const char	*name;
 	void		*platform_data;
+	struct device_node *of_node;
 };
 
 struct tps6586x_platform_data {
-- 
1.7.9.3

Re: [PATCH 05/11] tps6586x: Add device-tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

On Thu, Mar 08, 2012 at 03:51:25PM +0100, Thierry Reding wrote:

> +- gpio-controller: mark the device as a GPIO controller
> +- regulators: list of regulators provided by this controller, must be in the
> +  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...

> +			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.

Otherwise this looks good.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 05/11] tps6586x: Add device-tree support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]

* Mark Brown wrote:
> On Thu, Mar 08, 2012 at 03:51:25PM +0100, Thierry Reding wrote:
> 
> > +- gpio-controller: mark the device as a GPIO controller
> > +- regulators: list of regulators provided by this controller, must be in the
> > +  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...
> 
> > +			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.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 05/11] tps6586x: Add device-tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 718 bytes --]

On Thu, Mar 08, 2012 at 04:15:46PM +0100, Thierry Reding wrote:
> * Mark Brown wrote:

> > ...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.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 05/11] tps6586x: Add device-tree support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 838 bytes --]

* Mark Brown wrote:
> On Thu, Mar 08, 2012 at 04:15:46PM +0100, Thierry Reding wrote:
> > * Mark Brown wrote:
> 
> > > ...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.
> 
> Yes, that's going to be more code but much nicer for users.

Okay, I'll add that for the next round.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 05/11] tps6586x: Add device-tree support

[Grant Likely]

On Thu, 8 Mar 2012 16:15:46 +0100, Thierry Reding <thierry.reding@avionic-design.de> wrote:
> * Mark Brown wrote:
> > On Thu, Mar 08, 2012 at 03:51:25PM +0100, Thierry Reding wrote:
> > 
> > > +- gpio-controller: mark the device as a GPIO controller
> > > +- regulators: list of regulators provided by this controller, must be in the
> > > +  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...

It's worse than that; the DT provides absolutely no guarantees about the
ordering of either child nodes or properties.

> > 
> > > +			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.

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).

g.

Re: [PATCH 05/11] tps6586x: Add device-tree support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]

* Grant Likely wrote:
> On Thu, 8 Mar 2012 16:15:46 +0100, Thierry Reding <thierry.reding@avionic-design.de> wrote:
> > * Mark Brown wrote:
> > > ...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.
> 
> 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).

I'll go with the lookup via name then because as far as I can tell the
datasheet doesn't list any specific ordering of the regulators.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 06/11] PCI: Keep pci_fixup_irqs() around after init

[Thierry Reding]

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/pci/setup-irq.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
index eb219a1..270ae7b 100644
--- a/drivers/pci/setup-irq.c
+++ b/drivers/pci/setup-irq.c
@@ -18,7 +18,7 @@
 #include <linux/cache.h>
 
 
-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))
@@ -54,7 +54,7 @@ pdev_fixup_irq(struct pci_dev *dev,
 	pcibios_update_irq(dev, irq);
 }
 
-void __init
+void
 pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
 	       int (*map_irq)(const struct pci_dev *, u8, u8))
 {
-- 
1.7.9.3

Re: [PATCH 06/11] PCI: Keep pci_fixup_irqs() around after init

[Bjorn Helgaas]

On Thu, Mar 8, 2012 at 7:51 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> ---
>  drivers/pci/setup-irq.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> index eb219a1..270ae7b 100644
> --- a/drivers/pci/setup-irq.c
> +++ b/drivers/pci/setup-irq.c
> @@ -18,7 +18,7 @@
>  #include <linux/cache.h>
>
>
> -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))
> @@ -54,7 +54,7 @@ pdev_fixup_irq(struct pci_dev *dev,
>        pcibios_update_irq(dev, irq);
>  }
>
> -void __init
> +void
>  pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
>               int (*map_irq)(const struct pci_dev *, u8, u8))
>  {


        struct pci_dev *dev = NULL;
        for_each_pci_dev(dev)
                pdev_fixup_irq(dev, swizzle, map_irq);
}

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?

Bjorn

Re: [PATCH 06/11] PCI: Keep pci_fixup_irqs() around after init

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

* Bjorn Helgaas wrote:
> 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.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 06/11] PCI: Keep pci_fixup_irqs() around after init

[Bjorn Helgaas]

On Thu, Mar 8, 2012 at 11:37 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> * Bjorn Helgaas wrote:
>> 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.

Oh, I know *you're* not using the hotplug scenario.  I'm just thinking
that the PCi core looks a little broken in that scenario.

I think your patch is fine; it's just that I think the PCI core needs
a little more attention in this area.

Bjorn

[PATCH 07/11] ARM: pci: Keep pci_common_init() around after init

[Thierry Reding]

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 arch/arm/kernel/bios32.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index be5d3b2..c88dcf5 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -411,7 +411,7 @@ static int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 	return irq;
 }
 
-static void __init pcibios_init_hw(struct hw_pci *hw)
+static void pcibios_init_hw(struct hw_pci *hw)
 {
 	struct pci_sys_data *sys = NULL;
 	int ret;
@@ -457,7 +457,7 @@ static void __init pcibios_init_hw(struct hw_pci *hw)
 	}
 }
 
-void __init pci_common_init(struct hw_pci *hw)
+void pci_common_init(struct hw_pci *hw)
 {
 	struct pci_sys_data *sys;
 
-- 
1.7.9.3

[PATCH 08/11] ARM: tegra: Move tegra_pcie_xclk_clamp() to PMC

[Thierry Reding]

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 <thierry.reding@avionic-design.de>
---
 arch/arm/mach-tegra/pcie.c |   30 ++++--------------------------
 arch/arm/mach-tegra/pmc.c  |   16 ++++++++++++++++
 arch/arm/mach-tegra/pmc.h  |    1 +
 3 files changed, 21 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-tegra/pcie.c b/arch/arm/mach-tegra/pcie.c
index 7313059..7704af7a 100644
--- a/arch/arm/mach-tegra/pcie.c
+++ b/arch/arm/mach-tegra/pcie.c
@@ -42,6 +42,7 @@
 #include <mach/powergate.h>
 
 #include "board.h"
+#include "pmc.h"
 
 /* register definitions */
 #define AFI_OFFSET	0x3800
@@ -145,17 +146,6 @@
 #define  PADS_PLL_CTL_TXCLKREF_DIV10		(0 << 20)
 #define  PADS_PLL_CTL_TXCLKREF_DIV5		(1 << 20)
 
-/* PMC access is required for PCIE xclk (un)clamping */
-#define PMC_SCRATCH42		0x144
-#define PMC_SCRATCH42_PCX_CLAMP	(1 << 0)
-
-static void __iomem *reg_pmc_base = IO_ADDRESS(TEGRA_PMC_BASE);
-
-#define pmc_writel(value, reg) \
-	__raw_writel(value, reg_pmc_base + (reg))
-#define pmc_readl(reg) \
-	__raw_readl(reg_pmc_base + (reg))
-
 /*
  * Tegra2 defines 1GB in the AXI address map for PCIe.
  *
@@ -680,18 +670,6 @@ static int tegra_pcie_enable_controller(void)
 	return;
 }
 
-static void tegra_pcie_xclk_clamp(bool clamp)
-{
-	u32 reg;
-
-	reg = pmc_readl(PMC_SCRATCH42) & ~PMC_SCRATCH42_PCX_CLAMP;
-
-	if (clamp)
-		reg |= PMC_SCRATCH42_PCX_CLAMP;
-
-	pmc_writel(reg, PMC_SCRATCH42);
-}
-
 static void tegra_pcie_power_off(void)
 {
 	tegra_periph_reset_assert(tegra_pcie.pcie_xclk);
@@ -699,7 +677,7 @@ static void tegra_pcie_power_off(void)
 	tegra_periph_reset_assert(tegra_pcie.pex_clk);
 
 	tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
-	tegra_pcie_xclk_clamp(true);
+	tegra_pmc_pcie_xclk_clamp(true);
 }
 
 static int tegra_pcie_power_regate(void)
@@ -708,7 +686,7 @@ static int tegra_pcie_power_regate(void)
 
 	tegra_pcie_power_off();
 
-	tegra_pcie_xclk_clamp(true);
+	tegra_pmc_pcie_xclk_clamp(true);
 
 	tegra_periph_reset_assert(tegra_pcie.pcie_xclk);
 	tegra_periph_reset_assert(tegra_pcie.afi_clk);
@@ -722,7 +700,7 @@ static int tegra_pcie_power_regate(void)
 
 	tegra_periph_reset_deassert(tegra_pcie.afi_clk);
 
-	tegra_pcie_xclk_clamp(false);
+	tegra_pmc_pcie_xclk_clamp(false);
 
 	clk_enable(tegra_pcie.afi_clk);
 	clk_enable(tegra_pcie.pex_clk);
diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
index 7af6a54..399dc3a 100644
--- a/arch/arm/mach-tegra/pmc.c
+++ b/arch/arm/mach-tegra/pmc.c
@@ -24,6 +24,10 @@
 #define PMC_CTRL		0x0
 #define PMC_CTRL_INTR_LOW	(1 << 17)
 
+/* PMC access is required for PCIE xclk (un)clamping */
+#define PMC_SCRATCH42		0x144
+#define PMC_SCRATCH42_PCX_CLAMP	(1 << 0)
+
 static inline u32 tegra_pmc_readl(u32 reg)
 {
 	return readl(IO_ADDRESS(TEGRA_PMC_BASE + reg));
@@ -74,3 +78,15 @@ void __init tegra_pmc_init(void)
 		val &= ~PMC_CTRL_INTR_LOW;
 	tegra_pmc_writel(val, PMC_CTRL);
 }
+
+void tegra_pmc_pcie_xclk_clamp(bool clamp)
+{
+	u32 reg;
+
+	reg = tegra_pmc_readl(PMC_SCRATCH42) & ~PMC_SCRATCH42_PCX_CLAMP;
+
+	if (clamp)
+		reg |= PMC_SCRATCH42_PCX_CLAMP;
+
+	tegra_pmc_writel(reg, PMC_SCRATCH42);
+}
diff --git a/arch/arm/mach-tegra/pmc.h b/arch/arm/mach-tegra/pmc.h
index 8995ee4..2631c9a 100644
--- a/arch/arm/mach-tegra/pmc.h
+++ b/arch/arm/mach-tegra/pmc.h
@@ -19,5 +19,6 @@
 #define __MACH_TEGRA_PMC_H
 
 void tegra_pmc_init(void);
+void tegra_pmc_pcie_xclk_clamp(bool clamp);
 
 #endif
-- 
1.7.9.3

Re: [PATCH 08/11] ARM: tegra: Move tegra_pcie_xclk_clamp() to PMC

[Stephen Warren]

On 03/08/2012 07:51 AM, Thierry Reding wrote:
> 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 <thierry.reding@avionic-design.de>

Acked-by: Stephen Warren <swarren@wwwdotorg.org>

[PATCH 09/11] ARM: tegra: Rewrite PCIe support as a driver

[Thierry Reding]

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 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            |   25 ++
 arch/arm/mach-tegra/devices.h            |    1 +
 arch/arm/mach-tegra/include/mach/iomap.h |    6 +
 arch/arm/mach-tegra/pcie.c               |  509 ++++++++++++++++++------------
 9 files changed, 386 insertions(+), 222 deletions(-)

diff --git a/arch/arm/mach-tegra/board-harmony-pcie.c b/arch/arm/mach-tegra/board-harmony-pcie.c
index 33c4fed..e624a9c 100644
--- a/arch/arm/mach-tegra/board-harmony-pcie.c
+++ b/arch/arm/mach-tegra/board-harmony-pcie.c
@@ -23,18 +23,15 @@
 #include <asm/mach-types.h>
 
 #include "board.h"
+#include "devices.h"
 #include "board-harmony.h"
 
 #ifdef CONFIG_TEGRA_PCI
-
-static int __init harmony_pcie_init(void)
+static int tegra_pcie_init(struct platform_device *pdev)
 {
 	struct regulator *regulator = NULL;
 	int err;
 
-	if (!machine_is_harmony())
-		return 0;
-
 	err = gpio_request(TEGRA_GPIO_EN_VDD_1V05_GPIO, "EN_VDD_1V05");
 	if (err)
 		return err;
@@ -47,22 +44,35 @@ static int __init harmony_pcie_init(void)
 
 	regulator_enable(regulator);
 
-	err = tegra_pcie_init(true, true);
-	if (err)
-		goto err_pcie;
-
 	return 0;
 
-err_pcie:
-	regulator_disable(regulator);
-	regulator_put(regulator);
 err_reg:
 	gpio_free(TEGRA_GPIO_EN_VDD_1V05_GPIO);
 
 	return err;
 }
 
-/* PCI should be initialized after I2C, mfd and regulators */
-subsys_initcall_sync(harmony_pcie_init);
+static struct tegra_pcie_pdata tegra_pcie_pdata = {
+	.init = tegra_pcie_init,
+	.enable_ports = {
+		[0] = true,
+		[1] = true,
+	},
+};
+
+int __init harmony_pcie_init(void)
+{
+	if (!machine_is_harmony())
+		return 0;
+
+	tegra_pcie_device.dev.platform_data = &tegra_pcie_pdata;
+	platform_device_register(&tegra_pcie_device);
 
+	return 0;
+}
+#else
+int __init harmony_pcie_init(void)
+{
+	return 0;
+}
 #endif
diff --git a/arch/arm/mach-tegra/board-harmony.c b/arch/arm/mach-tegra/board-harmony.c
index c00aadb..ca20680 100644
--- a/arch/arm/mach-tegra/board-harmony.c
+++ b/arch/arm/mach-tegra/board-harmony.c
@@ -180,6 +180,7 @@ static void __init tegra_harmony_init(void)
 	platform_add_devices(harmony_devices, ARRAY_SIZE(harmony_devices));
 	harmony_i2c_init();
 	harmony_regulator_init();
+	harmony_pcie_init();
 }
 
 MACHINE_START(HARMONY, "harmony")
diff --git a/arch/arm/mach-tegra/board-harmony.h b/arch/arm/mach-tegra/board-harmony.h
index 139d96c..afa68e2 100644
--- a/arch/arm/mach-tegra/board-harmony.h
+++ b/arch/arm/mach-tegra/board-harmony.h
@@ -37,5 +37,6 @@
 
 void harmony_pinmux_init(void);
 int harmony_regulator_init(void);
+int harmony_pcie_init(void);
 
 #endif
diff --git a/arch/arm/mach-tegra/board-trimslice.c b/arch/arm/mach-tegra/board-trimslice.c
index cd52820..9bc414a 100644
--- a/arch/arm/mach-tegra/board-trimslice.c
+++ b/arch/arm/mach-tegra/board-trimslice.c
@@ -147,14 +147,23 @@ static __initdata struct tegra_clk_init_table trimslice_clk_init_table[] = {
 	{ NULL,		NULL,		0,		0},
 };
 
-static int __init tegra_trimslice_pci_init(void)
+static struct tegra_pcie_pdata tegra_pcie_pdata = {
+	.enable_ports = {
+		[0] = true,
+		[1] = true,
+	},
+};
+
+static int __init trimslice_pci_init(void)
 {
 	if (!machine_is_trimslice())
 		return 0;
 
-	return tegra_pcie_init(true, true);
+	tegra_pcie_device.dev.platform_data = &tegra_pcie_pdata;
+	platform_device_register(&tegra_pcie_device);
+
+	return 0;
 }
-subsys_initcall(tegra_trimslice_pci_init);
 
 static void __init tegra_trimslice_init(void)
 {
@@ -169,6 +178,7 @@ static void __init tegra_trimslice_init(void)
 
 	trimslice_i2c_init();
 	trimslice_usb_init();
+	trimslice_pci_init();
 }
 
 MACHINE_START(TRIMSLICE, "trimslice")
diff --git a/arch/arm/mach-tegra/board.h b/arch/arm/mach-tegra/board.h
index 75d1543..40f7e6b 100644
--- a/arch/arm/mach-tegra/board.h
+++ b/arch/arm/mach-tegra/board.h
@@ -21,6 +21,7 @@
 #ifndef __MACH_TEGRA_BOARD_H
 #define __MACH_TEGRA_BOARD_H
 
+#include <linux/platform_device.h>
 #include <linux/types.h>
 
 void tegra_assert_system_reset(char mode, const char *cmd);
@@ -30,7 +31,15 @@ void __init tegra30_init_early(void);
 void __init tegra_map_common_io(void);
 void __init tegra_init_irq(void);
 void __init tegra_dt_init_irq(void);
-int __init tegra_pcie_init(bool init_port0, bool init_port1);
 
 extern struct sys_timer tegra_timer;
+
+#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];
+};
+
 #endif
diff --git a/arch/arm/mach-tegra/devices.c b/arch/arm/mach-tegra/devices.c
index 7a2a02d..09e24e1 100644
--- a/arch/arm/mach-tegra/devices.c
+++ b/arch/arm/mach-tegra/devices.c
@@ -704,3 +704,28 @@ struct platform_device tegra_pcm_device = {
 	.name = "tegra-pcm-audio",
 	.id = -1,
 };
+
+static struct resource tegra_pcie_resources[] = {
+	[0] = {
+		.start = TEGRA_PCIE_BASE,
+		.end = TEGRA_PCIE_BASE + TEGRA_PCIE_SIZE - 1,
+		.flags = IORESOURCE_MEM,
+	},
+	[1] = {
+		.start = TEGRA_PCIE_MMIO_BASE,
+		.end = TEGRA_PCIE_MMIO_BASE + TEGRA_PCIE_MMIO_SIZE - 1,
+		.flags = IORESOURCE_MEM,
+	},
+	[2] = {
+		.start = INT_PCIE_INTR,
+		.end = INT_PCIE_INTR,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
+struct platform_device tegra_pcie_device = {
+	.name = "tegra-pcie",
+	.id = -1,
+	.resource = tegra_pcie_resources,
+	.num_resources = ARRAY_SIZE(tegra_pcie_resources),
+};
diff --git a/arch/arm/mach-tegra/devices.h b/arch/arm/mach-tegra/devices.h
index 873ecb2..95f0a97 100644
--- a/arch/arm/mach-tegra/devices.h
+++ b/arch/arm/mach-tegra/devices.h
@@ -48,5 +48,6 @@ extern struct platform_device tegra_i2s_device1;
 extern struct platform_device tegra_i2s_device2;
 extern struct platform_device tegra_das_device;
 extern struct platform_device tegra_pcm_device;
+extern struct platform_device tegra_pcie_device;
 
 #endif
diff --git a/arch/arm/mach-tegra/include/mach/iomap.h b/arch/arm/mach-tegra/include/mach/iomap.h
index cff672a..53b0dbe 100644
--- a/arch/arm/mach-tegra/include/mach/iomap.h
+++ b/arch/arm/mach-tegra/include/mach/iomap.h
@@ -242,6 +242,12 @@
 #define TEGRA_CSITE_BASE		0x70040000
 #define TEGRA_CSITE_SIZE		SZ_256K
 
+#define TEGRA_PCIE_BASE			0x80000000
+#define TEGRA_PCIE_SIZE			SZ_4M
+
+#define TEGRA_PCIE_MMIO_BASE		0x80400000
+#define TEGRA_PCIE_MMIO_SIZE		SZ_64K
+
 #define TEGRA_USB_BASE			0xC5000000
 #define TEGRA_USB_SIZE			SZ_16K
 
diff --git a/arch/arm/mach-tegra/pcie.c b/arch/arm/mach-tegra/pcie.c
index 7704af7a..8b20bc5 100644
--- a/arch/arm/mach-tegra/pcie.c
+++ b/arch/arm/mach-tegra/pcie.c
@@ -28,6 +28,7 @@
 
 #include <linux/kernel.h>
 #include <linux/pci.h>
+#include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/clk.h>
@@ -161,17 +162,12 @@
  * 0x90000000 - 0x9fffffff - non-prefetchable memory
  * 0xa0000000 - 0xbfffffff - prefetchable memory
  */
-#define TEGRA_PCIE_BASE		0x80000000
-
 #define PCIE_REGS_SZ		SZ_16K
 #define PCIE_CFG_OFF		PCIE_REGS_SZ
 #define PCIE_CFG_SZ		SZ_1M
 #define PCIE_EXT_CFG_OFF	(PCIE_CFG_SZ + PCIE_CFG_OFF)
 #define PCIE_EXT_CFG_SZ		SZ_1M
-#define PCIE_IOMAP_SZ		(PCIE_REGS_SZ + PCIE_CFG_SZ + PCIE_EXT_CFG_SZ)
 
-#define MMIO_BASE		(TEGRA_PCIE_BASE + SZ_4M)
-#define MMIO_SIZE		SZ_64K
 #define MEM_BASE_0		(TEGRA_PCIE_BASE + SZ_256M)
 #define MEM_SIZE_0		SZ_128M
 #define MEM_BASE_1		(MEM_BASE_0 + MEM_SIZE_0)
@@ -201,11 +197,15 @@ struct tegra_pcie_port {
 };
 
 struct tegra_pcie_info {
-	struct tegra_pcie_port	port[2];
+	struct device		*dev;
+	struct hw_pci		hw;
+
+	struct tegra_pcie_port	port[TEGRA_PCIE_MAX_PORTS];
 	int			num_ports;
 
 	void __iomem		*regs;
-	struct resource		res_mmio;
+	void __iomem		*mmio;
+	int			irq;
 
 	struct clk		*pex_clk;
 	struct clk		*afi_clk;
@@ -213,55 +213,53 @@ struct tegra_pcie_info {
 	struct clk		*pll_e;
 };
 
-static struct tegra_pcie_info tegra_pcie = {
-	.res_mmio = {
-		.name = "PCI IO",
-		.start = MMIO_BASE,
-		.end = MMIO_BASE + MMIO_SIZE - 1,
-		.flags = IORESOURCE_MEM,
-	},
-};
+static inline struct tegra_pcie_info *sys_to_pcie(struct pci_sys_data *sys)
+{
+	return container_of(sys->hw, struct tegra_pcie_info, hw);
+}
 
 void __iomem *tegra_pcie_io_base;
 EXPORT_SYMBOL(tegra_pcie_io_base);
 
-static inline void afi_writel(u32 value, unsigned long offset)
+static inline void afi_writel(struct tegra_pcie_info *pcie, u32 value, unsigned long offset)
 {
-	writel(value, offset + AFI_OFFSET + tegra_pcie.regs);
+	writel(value, offset + AFI_OFFSET + pcie->regs);
 }
 
-static inline u32 afi_readl(unsigned long offset)
+static inline u32 afi_readl(struct tegra_pcie_info *pcie, unsigned long offset)
 {
-	return readl(offset + AFI_OFFSET + tegra_pcie.regs);
+	return readl(offset + AFI_OFFSET + pcie->regs);
 }
 
-static inline void pads_writel(u32 value, unsigned long offset)
+static inline void pads_writel(struct tegra_pcie_info *pcie, u32 value, unsigned long offset)
 {
-	writel(value, offset + PADS_OFFSET + tegra_pcie.regs);
+	writel(value, offset + PADS_OFFSET + pcie->regs);
 }
 
-static inline u32 pads_readl(unsigned long offset)
+static inline u32 pads_readl(struct tegra_pcie_info *pcie, unsigned long offset)
 {
-	return readl(offset + PADS_OFFSET + tegra_pcie.regs);
+	return readl(offset + PADS_OFFSET + pcie->regs);
 }
 
-static struct tegra_pcie_port *bus_to_port(int bus)
+static struct tegra_pcie_port *bus_to_port(struct pci_bus *bus)
 {
+	struct tegra_pcie_info *pcie = sys_to_pcie(bus->sysdata);
 	int i;
 
-	for (i = tegra_pcie.num_ports - 1; i >= 0; i--) {
-		int rbus = tegra_pcie.port[i].root_bus_nr;
-		if (rbus != -1 && rbus == bus)
+	for (i = pcie->num_ports - 1; i >= 0; i--) {
+		int rbus = pcie->port[i].root_bus_nr;
+		if (rbus != -1 && rbus == bus->number)
 			break;
 	}
 
-	return i >= 0 ? tegra_pcie.port + i : NULL;
+	return i >= 0 ? pcie->port + i : NULL;
 }
 
 static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
 				int where, int size, u32 *val)
 {
-	struct tegra_pcie_port *pp = bus_to_port(bus->number);
+	struct tegra_pcie_info *pcie = sys_to_pcie(bus->sysdata);
+	struct tegra_pcie_port *pp = bus_to_port(bus);
 	void __iomem *addr;
 
 	if (pp) {
@@ -272,10 +270,10 @@ static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
 
 		addr = pp->base + (where & ~0x3);
 	} else {
-		addr = tegra_pcie.regs + (PCIE_CONF_BUS(bus->number) +
-					  PCIE_CONF_DEV(PCI_SLOT(devfn)) +
-					  PCIE_CONF_FUNC(PCI_FUNC(devfn)) +
-					  PCIE_CONF_REG(where));
+		addr = pcie->regs + (PCIE_CONF_BUS(bus->number) +
+				     PCIE_CONF_DEV(PCI_SLOT(devfn)) +
+				     PCIE_CONF_FUNC(PCI_FUNC(devfn)) +
+				     PCIE_CONF_REG(where));
 	}
 
 	*val = readl(addr);
@@ -291,7 +289,8 @@ static int tegra_pcie_read_conf(struct pci_bus *bus, unsigned int devfn,
 static int tegra_pcie_write_conf(struct pci_bus *bus, unsigned int devfn,
 				 int where, int size, u32 val)
 {
-	struct tegra_pcie_port *pp = bus_to_port(bus->number);
+	struct tegra_pcie_info *pcie = sys_to_pcie(bus->sysdata);
+	struct tegra_pcie_port *pp = bus_to_port(bus);
 	void __iomem *addr;
 
 	u32 mask;
@@ -303,10 +302,10 @@ static int tegra_pcie_write_conf(struct pci_bus *bus, unsigned int devfn,
 
 		addr = pp->base + (where & ~0x3);
 	} else {
-		addr = tegra_pcie.regs + (PCIE_CONF_BUS(bus->number) +
-					  PCIE_CONF_DEV(PCI_SLOT(devfn)) +
-					  PCIE_CONF_FUNC(PCI_FUNC(devfn)) +
-					  PCIE_CONF_REG(where));
+		addr = pcie->regs + (PCIE_CONF_BUS(bus->number) +
+				     PCIE_CONF_DEV(PCI_SLOT(devfn)) +
+				     PCIE_CONF_FUNC(PCI_FUNC(devfn)) +
+				     PCIE_CONF_REG(where));
 	}
 
 	if (size == 4) {
@@ -373,12 +372,13 @@ DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_relax_enable);
 
 static int tegra_pcie_setup(int nr, struct pci_sys_data *sys)
 {
+	struct tegra_pcie_info *pcie = sys_to_pcie(sys);
 	struct tegra_pcie_port *pp;
 
-	if (nr >= tegra_pcie.num_ports)
+	if (nr >= pcie->num_ports)
 		return 0;
 
-	pp = tegra_pcie.port + nr;
+	pp = pcie->port + nr;
 	pp->root_bus_nr = sys->busnr;
 
 	/*
@@ -441,35 +441,29 @@ static int tegra_pcie_setup(int nr, struct pci_sys_data *sys)
 	return 1;
 }
 
-static int tegra_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
+static int tegra_pcie_map_irq(const struct pci_dev *pdev, u8 slot, u8 pin)
 {
-	return INT_PCIE_INTR;
+	struct tegra_pcie_info *pcie = sys_to_pcie(pdev->bus->sysdata);
+
+	return pcie->irq;
 }
 
-static struct pci_bus __init *tegra_pcie_scan_bus(int nr,
-						  struct pci_sys_data *sys)
+static struct pci_bus __devinit *tegra_pcie_scan_bus(int nr,
+						     struct pci_sys_data *sys)
 {
+	struct tegra_pcie_info *pcie = sys_to_pcie(sys);
 	struct tegra_pcie_port *pp;
 
-	if (nr >= tegra_pcie.num_ports)
+	if (nr >= pcie->num_ports)
 		return NULL;
 
-	pp = tegra_pcie.port + nr;
+	pp = pcie->port + nr;
 	pp->root_bus_nr = sys->busnr;
 
 	return pci_scan_root_bus(NULL, sys->busnr, &tegra_pcie_ops, sys,
 				 &sys->resources);
 }
 
-static struct hw_pci tegra_pcie_hw __initdata = {
-	.nr_controllers	= 2,
-	.setup		= tegra_pcie_setup,
-	.scan		= tegra_pcie_scan_bus,
-	.swizzle	= pci_std_swizzle,
-	.map_irq	= tegra_pcie_map_irq,
-};
-
-
 static irqreturn_t tegra_pcie_isr(int irq, void *arg)
 {
 	const char *err_msg[] = {
@@ -483,12 +477,12 @@ static irqreturn_t tegra_pcie_isr(int irq, void *arg)
 		"AXI response decoding error",
 		"Transcation timeout",
 	};
-
+	struct tegra_pcie_info *pcie = arg;
 	u32 code, signature;
 
-	code = afi_readl(AFI_INTR_CODE) & AFI_INTR_CODE_MASK;
-	signature = afi_readl(AFI_INTR_SIGNATURE);
-	afi_writel(0, AFI_INTR_CODE);
+	code = afi_readl(pcie, AFI_INTR_CODE) & AFI_INTR_CODE_MASK;
+	signature = afi_readl(pcie, AFI_INTR_SIGNATURE);
+	afi_writel(pcie, 0, AFI_INTR_CODE);
 
 	if (code == AFI_INTR_LEGACY)
 		return IRQ_NONE;
@@ -501,14 +495,16 @@ static irqreturn_t tegra_pcie_isr(int irq, void *arg)
 	 * happen a lot during enumeration
 	 */
 	if (code == AFI_INTR_MASTER_ABORT)
-		pr_debug("PCIE: %s, signature: %08x\n", err_msg[code], signature);
+		dev_dbg(pcie->dev, "%s, signature: %08x\n", err_msg[code],
+			signature);
 	else
-		pr_err("PCIE: %s, signature: %08x\n", err_msg[code], signature);
+		dev_err(pcie->dev, "%s, signature: %08x\n", err_msg[code],
+			signature);
 
 	return IRQ_HANDLED;
 }
 
-static void tegra_pcie_setup_translations(void)
+static void tegra_pcie_setup_translations(struct tegra_pcie_info *pcie)
 {
 	u32 fpci_bar;
 	u32 size;
@@ -518,120 +514,120 @@ static void tegra_pcie_setup_translations(void)
 	fpci_bar = ((u32)0xfdff << 16);
 	size = PCIE_CFG_SZ;
 	axi_address = TEGRA_PCIE_BASE + PCIE_CFG_OFF;
-	afi_writel(axi_address, AFI_AXI_BAR0_START);
-	afi_writel(size >> 12, AFI_AXI_BAR0_SZ);
-	afi_writel(fpci_bar, AFI_FPCI_BAR0);
+	afi_writel(pcie, axi_address, AFI_AXI_BAR0_START);
+	afi_writel(pcie, size >> 12, AFI_AXI_BAR0_SZ);
+	afi_writel(pcie, fpci_bar, AFI_FPCI_BAR0);
 
 	/* Bar 1: extended config Bar */
 	fpci_bar = ((u32)0xfe1 << 20);
 	size = PCIE_EXT_CFG_SZ;
 	axi_address = TEGRA_PCIE_BASE + PCIE_EXT_CFG_OFF;
-	afi_writel(axi_address, AFI_AXI_BAR1_START);
-	afi_writel(size >> 12, AFI_AXI_BAR1_SZ);
-	afi_writel(fpci_bar, AFI_FPCI_BAR1);
+	afi_writel(pcie, axi_address, AFI_AXI_BAR1_START);
+	afi_writel(pcie, size >> 12, AFI_AXI_BAR1_SZ);
+	afi_writel(pcie, fpci_bar, AFI_FPCI_BAR1);
 
 	/* Bar 2: downstream IO bar */
 	fpci_bar = ((__u32)0xfdfc << 16);
-	size = MMIO_SIZE;
-	axi_address = MMIO_BASE;
-	afi_writel(axi_address, AFI_AXI_BAR2_START);
-	afi_writel(size >> 12, AFI_AXI_BAR2_SZ);
-	afi_writel(fpci_bar, AFI_FPCI_BAR2);
+	size = TEGRA_PCIE_MMIO_SIZE;
+	axi_address = TEGRA_PCIE_MMIO_BASE;
+	afi_writel(pcie, axi_address, AFI_AXI_BAR2_START);
+	afi_writel(pcie, size >> 12, AFI_AXI_BAR2_SZ);
+	afi_writel(pcie, fpci_bar, AFI_FPCI_BAR2);
 
 	/* Bar 3: prefetchable memory BAR */
 	fpci_bar = (((PREFETCH_MEM_BASE_0 >> 12) & 0x0fffffff) << 4) | 0x1;
 	size =  PREFETCH_MEM_SIZE_0 +  PREFETCH_MEM_SIZE_1;
 	axi_address = PREFETCH_MEM_BASE_0;
-	afi_writel(axi_address, AFI_AXI_BAR3_START);
-	afi_writel(size >> 12, AFI_AXI_BAR3_SZ);
-	afi_writel(fpci_bar, AFI_FPCI_BAR3);
+	afi_writel(pcie, axi_address, AFI_AXI_BAR3_START);
+	afi_writel(pcie, size >> 12, AFI_AXI_BAR3_SZ);
+	afi_writel(pcie, fpci_bar, AFI_FPCI_BAR3);
 
 	/* Bar 4: non prefetchable memory BAR */
 	fpci_bar = (((MEM_BASE_0 >> 12)	& 0x0FFFFFFF) << 4) | 0x1;
 	size = MEM_SIZE_0 + MEM_SIZE_1;
 	axi_address = MEM_BASE_0;
-	afi_writel(axi_address, AFI_AXI_BAR4_START);
-	afi_writel(size >> 12, AFI_AXI_BAR4_SZ);
-	afi_writel(fpci_bar, AFI_FPCI_BAR4);
+	afi_writel(pcie, axi_address, AFI_AXI_BAR4_START);
+	afi_writel(pcie, size >> 12, AFI_AXI_BAR4_SZ);
+	afi_writel(pcie, fpci_bar, AFI_FPCI_BAR4);
 
 	/* Bar 5: NULL out the remaining BAR as it is not used */
 	fpci_bar = 0;
 	size = 0;
 	axi_address = 0;
-	afi_writel(axi_address, AFI_AXI_BAR5_START);
-	afi_writel(size >> 12, AFI_AXI_BAR5_SZ);
-	afi_writel(fpci_bar, AFI_FPCI_BAR5);
+	afi_writel(pcie, axi_address, AFI_AXI_BAR5_START);
+	afi_writel(pcie, size >> 12, AFI_AXI_BAR5_SZ);
+	afi_writel(pcie, fpci_bar, AFI_FPCI_BAR5);
 
 	/* map all upstream transactions as uncached */
-	afi_writel(PHYS_OFFSET, AFI_CACHE_BAR0_ST);
-	afi_writel(0, AFI_CACHE_BAR0_SZ);
-	afi_writel(0, AFI_CACHE_BAR1_ST);
-	afi_writel(0, AFI_CACHE_BAR1_SZ);
+	afi_writel(pcie, PHYS_OFFSET, AFI_CACHE_BAR0_ST);
+	afi_writel(pcie, 0, AFI_CACHE_BAR0_SZ);
+	afi_writel(pcie, 0, AFI_CACHE_BAR1_ST);
+	afi_writel(pcie, 0, AFI_CACHE_BAR1_SZ);
 
 	/* No MSI */
-	afi_writel(0, AFI_MSI_FPCI_BAR_ST);
-	afi_writel(0, AFI_MSI_BAR_SZ);
-	afi_writel(0, AFI_MSI_AXI_BAR_ST);
-	afi_writel(0, AFI_MSI_BAR_SZ);
+	afi_writel(pcie, 0, AFI_MSI_FPCI_BAR_ST);
+	afi_writel(pcie, 0, AFI_MSI_BAR_SZ);
+	afi_writel(pcie, 0, AFI_MSI_AXI_BAR_ST);
+	afi_writel(pcie, 0, AFI_MSI_BAR_SZ);
 }
 
-static int tegra_pcie_enable_controller(void)
+static int tegra_pcie_enable_controller(struct tegra_pcie_info *pcie)
 {
 	u32 val, reg;
 	int i, timeout;
 
 	/* Enable slot clock and pulse the reset signals */
 	for (i = 0, reg = AFI_PEX0_CTRL; i < 2; i++, reg += 0x8) {
-		val = afi_readl(reg) |  AFI_PEX_CTRL_REFCLK_EN;
-		afi_writel(val, reg);
+		val = afi_readl(pcie, reg) |  AFI_PEX_CTRL_REFCLK_EN;
+		afi_writel(pcie, val, reg);
 		val &= ~AFI_PEX_CTRL_RST;
-		afi_writel(val, reg);
+		afi_writel(pcie, val, reg);
 
-		val = afi_readl(reg) | AFI_PEX_CTRL_RST;
-		afi_writel(val, reg);
+		val = afi_readl(pcie, reg) | AFI_PEX_CTRL_RST;
+		afi_writel(pcie, val, reg);
 	}
 
 	/* Enable dual controller and both ports */
-	val = afi_readl(AFI_PCIE_CONFIG);
+	val = afi_readl(pcie, AFI_PCIE_CONFIG);
 	val &= ~(AFI_PCIE_CONFIG_PCIEC0_DISABLE_DEVICE |
 		 AFI_PCIE_CONFIG_PCIEC1_DISABLE_DEVICE |
 		 AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_MASK);
 	val |= AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_DUAL;
-	afi_writel(val, AFI_PCIE_CONFIG);
+	afi_writel(pcie, val, AFI_PCIE_CONFIG);
 
-	val = afi_readl(AFI_FUSE) & ~AFI_FUSE_PCIE_T0_GEN2_DIS;
-	afi_writel(val, AFI_FUSE);
+	val = afi_readl(pcie, AFI_FUSE) & ~AFI_FUSE_PCIE_T0_GEN2_DIS;
+	afi_writel(pcie, val, AFI_FUSE);
 
 	/* Initialze internal PHY, enable up to 16 PCIE lanes */
-	pads_writel(0x0, PADS_CTL_SEL);
+	pads_writel(pcie, 0x0, PADS_CTL_SEL);
 
 	/* override IDDQ to 1 on all 4 lanes */
-	val = pads_readl(PADS_CTL) | PADS_CTL_IDDQ_1L;
-	pads_writel(val, PADS_CTL);
+	val = pads_readl(pcie, PADS_CTL) | PADS_CTL_IDDQ_1L;
+	pads_writel(pcie, val, PADS_CTL);
 
 	/*
 	 * set up PHY PLL inputs select PLLE output as refclock,
 	 * set TX ref sel to div10 (not div5)
 	 */
-	val = pads_readl(PADS_PLL_CTL);
+	val = pads_readl(pcie, PADS_PLL_CTL);
 	val &= ~(PADS_PLL_CTL_REFCLK_MASK | PADS_PLL_CTL_TXCLKREF_MASK);
 	val |= (PADS_PLL_CTL_REFCLK_INTERNAL_CML | PADS_PLL_CTL_TXCLKREF_DIV10);
-	pads_writel(val, PADS_PLL_CTL);
+	pads_writel(pcie, val, PADS_PLL_CTL);
 
 	/* take PLL out of reset  */
-	val = pads_readl(PADS_PLL_CTL) | PADS_PLL_CTL_RST_B4SM;
-	pads_writel(val, PADS_PLL_CTL);
+	val = pads_readl(pcie, PADS_PLL_CTL) | PADS_PLL_CTL_RST_B4SM;
+	pads_writel(pcie, val, PADS_PLL_CTL);
 
 	/*
 	 * Hack, set the clock voltage to the DEFAULT provided by hw folks.
 	 * This doesn't exist in the documentation
 	 */
-	pads_writel(0xfa5cfa5c, 0xc8);
+	pads_writel(pcie, 0xfa5cfa5c, 0xc8);
 
 	/* Wait for the PLL to lock */
 	timeout = 2000;
 	do {
-		val = pads_readl(PADS_PLL_CTL);
+		val = pads_readl(pcie, PADS_PLL_CTL);
 		mdelay(1);
 		if (--timeout == 0) {
 			pr_err("Tegra PCIe error: timeout waiting for PLL\n");
@@ -640,188 +636,237 @@ static int tegra_pcie_enable_controller(void)
 	} while (!(val & PADS_PLL_CTL_LOCKDET));
 
 	/* turn off IDDQ override */
-	val = pads_readl(PADS_CTL) & ~PADS_CTL_IDDQ_1L;
-	pads_writel(val, PADS_CTL);
+	val = pads_readl(pcie, PADS_CTL) & ~PADS_CTL_IDDQ_1L;
+	pads_writel(pcie, val, PADS_CTL);
 
 	/* enable TX/RX data */
-	val = pads_readl(PADS_CTL);
+	val = pads_readl(pcie, PADS_CTL);
 	val |= (PADS_CTL_TX_DATA_EN_1L | PADS_CTL_RX_DATA_EN_1L);
-	pads_writel(val, PADS_CTL);
+	pads_writel(pcie, val, PADS_CTL);
 
 	/* Take the PCIe interface module out of reset */
-	tegra_periph_reset_deassert(tegra_pcie.pcie_xclk);
+	tegra_periph_reset_deassert(pcie->pcie_xclk);
 
 	/* Finally enable PCIe */
-	val = afi_readl(AFI_CONFIGURATION) | AFI_CONFIGURATION_EN_FPCI;
-	afi_writel(val, AFI_CONFIGURATION);
+	val = afi_readl(pcie, AFI_CONFIGURATION) | AFI_CONFIGURATION_EN_FPCI;
+	afi_writel(pcie, val, AFI_CONFIGURATION);
 
 	val = (AFI_INTR_EN_INI_SLVERR | AFI_INTR_EN_INI_DECERR |
 	       AFI_INTR_EN_TGT_SLVERR | AFI_INTR_EN_TGT_DECERR |
 	       AFI_INTR_EN_TGT_WRERR | AFI_INTR_EN_DFPCI_DECERR);
-	afi_writel(val, AFI_AFI_INTR_ENABLE);
-	afi_writel(0xffffffff, AFI_SM_INTR_ENABLE);
+	afi_writel(pcie, val, AFI_AFI_INTR_ENABLE);
+	afi_writel(pcie, 0xffffffff, AFI_SM_INTR_ENABLE);
 
 	/* FIXME: No MSI for now, only INT */
-	afi_writel(AFI_INTR_MASK_INT_MASK, AFI_INTR_MASK);
+	afi_writel(pcie, AFI_INTR_MASK_INT_MASK, AFI_INTR_MASK);
 
 	/* Disable all execptions */
-	afi_writel(0, AFI_FPCI_ERROR_MASKS);
+	afi_writel(pcie, 0, AFI_FPCI_ERROR_MASKS);
 
-	return;
+	return 0;
 }
 
-static void tegra_pcie_power_off(void)
+static void tegra_pcie_power_off(struct tegra_pcie_info *pcie)
 {
-	tegra_periph_reset_assert(tegra_pcie.pcie_xclk);
-	tegra_periph_reset_assert(tegra_pcie.afi_clk);
-	tegra_periph_reset_assert(tegra_pcie.pex_clk);
+	tegra_periph_reset_assert(pcie->pcie_xclk);
+	tegra_periph_reset_assert(pcie->afi_clk);
+	tegra_periph_reset_assert(pcie->pex_clk);
 
 	tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
 	tegra_pmc_pcie_xclk_clamp(true);
 }
 
-static int tegra_pcie_power_regate(void)
+static int tegra_pcie_power_regate(struct tegra_pcie_info *pcie)
 {
 	int err;
 
-	tegra_pcie_power_off();
+	tegra_pcie_power_off(pcie);
 
 	tegra_pmc_pcie_xclk_clamp(true);
 
-	tegra_periph_reset_assert(tegra_pcie.pcie_xclk);
-	tegra_periph_reset_assert(tegra_pcie.afi_clk);
+	tegra_periph_reset_assert(pcie->pcie_xclk);
+	tegra_periph_reset_assert(pcie->afi_clk);
 
 	err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_PCIE,
-						tegra_pcie.pex_clk);
+						pcie->pex_clk);
 	if (err) {
-		pr_err("PCIE: powerup sequence failed: %d\n", err);
+		dev_err(pcie->dev, "powerup sequence failed: %d\n", err);
 		return err;
 	}
 
-	tegra_periph_reset_deassert(tegra_pcie.afi_clk);
+	tegra_periph_reset_deassert(pcie->afi_clk);
 
 	tegra_pmc_pcie_xclk_clamp(false);
 
-	clk_enable(tegra_pcie.afi_clk);
-	clk_enable(tegra_pcie.pex_clk);
-	return clk_enable(tegra_pcie.pll_e);
+	clk_enable(pcie->afi_clk);
+	clk_enable(pcie->pex_clk);
+	return clk_enable(pcie->pll_e);
 }
 
-static int tegra_pcie_clocks_get(void)
+static int tegra_pcie_clocks_get(struct tegra_pcie_info *pcie)
 {
 	int err;
 
-	tegra_pcie.pex_clk = clk_get(NULL, "pex");
-	if (IS_ERR(tegra_pcie.pex_clk))
-		return PTR_ERR(tegra_pcie.pex_clk);
+	pcie->pex_clk = clk_get(NULL, "pex");
+	if (IS_ERR(pcie->pex_clk))
+		return PTR_ERR(pcie->pex_clk);
 
-	tegra_pcie.afi_clk = clk_get(NULL, "afi");
-	if (IS_ERR(tegra_pcie.afi_clk)) {
-		err = PTR_ERR(tegra_pcie.afi_clk);
+	pcie->afi_clk = clk_get(NULL, "afi");
+	if (IS_ERR(pcie->afi_clk)) {
+		err = PTR_ERR(pcie->afi_clk);
 		goto err_afi_clk;
 	}
 
-	tegra_pcie.pcie_xclk = clk_get(NULL, "pcie_xclk");
-	if (IS_ERR(tegra_pcie.pcie_xclk)) {
-		err =  PTR_ERR(tegra_pcie.pcie_xclk);
+	pcie->pcie_xclk = clk_get(NULL, "pcie_xclk");
+	if (IS_ERR(pcie->pcie_xclk)) {
+		err = PTR_ERR(pcie->pcie_xclk);
 		goto err_pcie_xclk;
 	}
 
-	tegra_pcie.pll_e = clk_get_sys(NULL, "pll_e");
-	if (IS_ERR(tegra_pcie.pll_e)) {
-		err = PTR_ERR(tegra_pcie.pll_e);
+	pcie->pll_e = clk_get_sys(NULL, "pll_e");
+	if (IS_ERR(pcie->pll_e)) {
+		err = PTR_ERR(pcie->pll_e);
 		goto err_pll_e;
 	}
 
 	return 0;
 
 err_pll_e:
-	clk_put(tegra_pcie.pcie_xclk);
+	clk_put(pcie->pcie_xclk);
 err_pcie_xclk:
-	clk_put(tegra_pcie.afi_clk);
+	clk_put(pcie->afi_clk);
 err_afi_clk:
-	clk_put(tegra_pcie.pex_clk);
+	clk_put(pcie->pex_clk);
 
 	return err;
 }
 
-static void tegra_pcie_clocks_put(void)
+static void tegra_pcie_clocks_put(struct tegra_pcie_info *pcie)
 {
-	clk_put(tegra_pcie.pll_e);
-	clk_put(tegra_pcie.pcie_xclk);
-	clk_put(tegra_pcie.afi_clk);
-	clk_put(tegra_pcie.pex_clk);
+	clk_put(pcie->pll_e);
+	clk_put(pcie->pcie_xclk);
+	clk_put(pcie->afi_clk);
+	clk_put(pcie->pex_clk);
 }
 
-static int __init tegra_pcie_get_resources(void)
+static int __devinit tegra_pcie_get_resources(struct platform_device *pdev)
 {
-	struct resource *res_mmio = &tegra_pcie.res_mmio;
+	struct tegra_pcie_info *pcie = platform_get_drvdata(pdev);
+	struct resource *regs;
+	struct resource *mmio;
 	int err;
 
-	err = tegra_pcie_clocks_get();
+	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mmio = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+
+	if (!regs || !mmio) {
+		dev_err(&pdev->dev, "failed to get I/O resources\n");
+		return -ENXIO;
+	}
+
+	err = tegra_pcie_clocks_get(pcie);
 	if (err) {
-		pr_err("PCIE: failed to get clocks: %d\n", err);
+		dev_err(&pdev->dev, "failed to get clocks: %d\n", err);
 		return err;
 	}
 
-	err = tegra_pcie_power_regate();
+	err = tegra_pcie_power_regate(pcie);
 	if (err) {
-		pr_err("PCIE: failed to power up: %d\n", err);
+		dev_err(&pdev->dev, "failed to power up: %d\n", err);
 		goto err_pwr_on;
 	}
 
-	tegra_pcie.regs = ioremap_nocache(TEGRA_PCIE_BASE, PCIE_IOMAP_SZ);
-	if (tegra_pcie.regs == NULL) {
-		pr_err("PCIE: Failed to map PCI/AFI registers\n");
+	regs = request_mem_region(regs->start, resource_size(regs), "PCI/AFI");
+	if (regs == NULL) {
+		dev_err(&pdev->dev, "failed to request PCI/AFI region: %d\n", err);
+		goto err_req_reg;
+	}
+
+	pcie->regs = ioremap_nocache(regs->start, resource_size(regs));
+	if (pcie->regs == NULL) {
+		dev_err(&pdev->dev, "failed to map PCI/AFI registers\n");
 		err = -ENOMEM;
 		goto err_map_reg;
 	}
 
-	err = request_resource(&iomem_resource, res_mmio);
-	if (err) {
-		pr_err("PCIE: Failed to request resources: %d\n", err);
+	mmio = request_mem_region(mmio->start, resource_size(mmio), "PCI I/O");
+	if (mmio == NULL) {
+		dev_err(&pdev->dev, "failed to request PCI I/O region: %d\n", err);
 		goto err_req_io;
 	}
 
-	tegra_pcie_io_base = ioremap_nocache(res_mmio->start,
-					     resource_size(res_mmio));
-	if (tegra_pcie_io_base == NULL) {
-		pr_err("PCIE: Failed to map IO\n");
+	pcie->mmio = ioremap_nocache(mmio->start, resource_size(mmio));
+	if (pcie->mmio == NULL) {
+		dev_err(&pdev->dev, "failed to map PCI I/O region\n");
 		err = -ENOMEM;
 		goto err_map_io;
 	}
 
-	err = request_irq(INT_PCIE_INTR, tegra_pcie_isr,
-			  IRQF_SHARED, "PCIE", &tegra_pcie);
+	tegra_pcie_io_base = pcie->mmio;
+
+	err = platform_get_irq(pdev, 0);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to get IRQ: %d\n", err);
+		goto err_irq;
+	}
+
+	pcie->irq = err;
+
+	err = request_irq(pcie->irq, tegra_pcie_isr, IRQF_SHARED, "PCIE",
+			  pcie);
 	if (err) {
-		pr_err("PCIE: Failed to register IRQ: %d\n", err);
+		dev_err(&pdev->dev, "failed to register IRQ: %d\n", err);
 		goto err_irq;
 	}
-	set_irq_flags(INT_PCIE_INTR, IRQF_VALID);
 
 	return 0;
 
 err_irq:
-	iounmap(tegra_pcie_io_base);
+	iounmap(pcie->mmio);
 err_map_io:
-	release_resource(&tegra_pcie.res_mmio);
+	release_resource(mmio);
 err_req_io:
-	iounmap(tegra_pcie.regs);
+	iounmap(pcie->regs);
 err_map_reg:
-	tegra_pcie_power_off();
+	release_resource(regs);
+err_req_reg:
+	tegra_pcie_power_off(pcie);
 err_pwr_on:
-	tegra_pcie_clocks_put();
+	tegra_pcie_clocks_put(pcie);
 
 	return err;
 }
 
+static int tegra_pcie_put_resources(struct platform_device *pdev)
+{
+	struct tegra_pcie_info *pcie = platform_get_drvdata(pdev);
+	struct resource *regs;
+	struct resource *mmio;
+
+	free_irq(pcie->irq, pcie);
+
+	iounmap(pcie->mmio);
+	mmio = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	release_resource(mmio);
+
+	iounmap(pcie->regs);
+	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	release_resource(regs);
+
+	tegra_pcie_power_off(pcie);
+	tegra_pcie_clocks_put(pcie);
+
+	return 0;
+}
+
 /*
  * FIXME: If there are no PCIe cards attached, then calling this function
  * can result in the increase of the bootup time as there are big timeout
  * loops.
  */
 #define TEGRA_PCIE_LINKUP_TIMEOUT	200	/* up to 1.2 seconds */
-static bool tegra_pcie_check_link(struct tegra_pcie_port *pp, int idx,
+static bool tegra_pcie_check_link(struct tegra_pcie_info *pcie,
+				  struct tegra_pcie_port *pp, int idx,
 				  u32 reset_reg)
 {
 	u32 reg;
@@ -841,7 +886,7 @@ static bool tegra_pcie_check_link(struct tegra_pcie_port *pp, int idx,
 		}
 
 		if (!timeout)  {
-			pr_err("PCIE: port %d: link down, retrying\n", idx);
+			dev_err(pcie->dev, "port %d: link down, retrying\n", idx);
 			goto retry;
 		}
 
@@ -858,11 +903,11 @@ static bool tegra_pcie_check_link(struct tegra_pcie_port *pp, int idx,
 
 retry:
 		/* Pulse the PEX reset */
-		reg = afi_readl(reset_reg) | AFI_PEX_CTRL_RST;
-		afi_writel(reg, reset_reg);
+		reg = afi_readl(pcie, reset_reg) | AFI_PEX_CTRL_RST;
+		afi_writel(pcie, reg, reset_reg);
 		mdelay(1);
-		reg = afi_readl(reset_reg) & ~AFI_PEX_CTRL_RST;
-		afi_writel(reg, reset_reg);
+		reg = afi_readl(pcie, reset_reg) & ~AFI_PEX_CTRL_RST;
+		afi_writel(pcie, reg, reset_reg);
 
 		retries--;
 	} while (retries);
@@ -870,55 +915,111 @@ retry:
 	return false;
 }
 
-static void __init tegra_pcie_add_port(int index, u32 offset, u32 reset_reg)
+static void __devinit tegra_pcie_add_port(struct tegra_pcie_info *pcie,
+					  int index, u32 offset,
+					  u32 reset_reg)
 {
 	struct tegra_pcie_port *pp;
 
-	pp = tegra_pcie.port + tegra_pcie.num_ports;
+	pp = pcie->port + pcie->num_ports;
 
 	pp->index = -1;
-	pp->base = tegra_pcie.regs + offset;
-	pp->link_up = tegra_pcie_check_link(pp, index, reset_reg);
+	pp->base = pcie->regs + offset;
+	pp->link_up = tegra_pcie_check_link(pcie, pp, index, reset_reg);
 
 	if (!pp->link_up) {
 		pp->base = NULL;
-		printk(KERN_INFO "PCIE: port %d: link down, ignoring\n", index);
+		dev_info(pcie->dev, "port %d: link down, ignoring\n", index);
 		return;
 	}
 
-	tegra_pcie.num_ports++;
+	pcie->num_ports++;
 	pp->index = index;
 	pp->root_bus_nr = -1;
 	memset(pp->res, 0, sizeof(pp->res));
 }
 
-int __init tegra_pcie_init(bool init_port0, bool init_port1)
+static int __devinit tegra_pcie_probe(struct platform_device *pdev)
 {
+	struct tegra_pcie_pdata *pdata = pdev->dev.platform_data;
+	struct tegra_pcie_info *pcie;
 	int err;
 
-	if (!(init_port0 || init_port1))
+	if (!pdata->enable_ports[0] && !pdata->enable_ports[1])
 		return -ENODEV;
 
+	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pcie);
+	pcie->dev = &pdev->dev;
+
 	pcibios_min_mem = 0;
 
-	err = tegra_pcie_get_resources();
-	if (err)
+	err = tegra_pcie_get_resources(pdev);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to request resources: %d\n", err);
 		return err;
+	}
 
-	err = tegra_pcie_enable_controller();
+	if (pdata->init) {
+		err = pdata->init(pdev);
+		if (err < 0) {
+			tegra_pcie_put_resources(pdev);
+			return err;
+		}
+	}
+
+	err = tegra_pcie_enable_controller(pcie);
 	if (err)
 		return err;
 
 	/* setup the AFI address translations */
-	tegra_pcie_setup_translations();
+	tegra_pcie_setup_translations(pcie);
+
+	if (pdata->enable_ports[0])
+		tegra_pcie_add_port(pcie, 0, RP0_OFFSET, AFI_PEX0_CTRL);
 
-	if (init_port0)
-		tegra_pcie_add_port(0, RP0_OFFSET, AFI_PEX0_CTRL);
+	if (pdata->enable_ports[1])
+		tegra_pcie_add_port(pcie, 1, RP1_OFFSET, AFI_PEX1_CTRL);
 
-	if (init_port1)
-		tegra_pcie_add_port(1, RP1_OFFSET, AFI_PEX1_CTRL);
+	pcie->hw.nr_controllers = 2;
+	pcie->hw.setup = tegra_pcie_setup;
+	pcie->hw.scan = tegra_pcie_scan_bus;
+	pcie->hw.swizzle = pci_std_swizzle;
+	pcie->hw.map_irq = tegra_pcie_map_irq;
 
-	pci_common_init(&tegra_pcie_hw);
+	pci_common_init(&pcie->hw);
 
 	return 0;
 }
+
+static int __devexit tegra_pcie_remove(struct platform_device *pdev)
+{
+	struct tegra_pcie_pdata *pdata = pdev->dev.platform_data;
+	int err;
+
+	err = tegra_pcie_put_resources(pdev);
+	if (err < 0)
+		return err;
+
+	if (pdata->exit) {
+		err = pdata->exit(pdev);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+
+static struct platform_driver tegra_pcie_driver = {
+	.driver = {
+		.name = "tegra-pcie",
+		.owner = THIS_MODULE,
+	},
+	.probe = tegra_pcie_probe,
+	.remove = __devexit_p(tegra_pcie_remove),
+};
+
+module_platform_driver(tegra_pcie_driver);
-- 
1.7.9.3

Re: [PATCH 09/11] ARM: tegra: Rewrite PCIe support as a driver

[Stephen Warren]

On 03/08/2012 07:51 AM, Thierry Reding wrote:
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>

Patch description?

> diff --git a/arch/arm/mach-tegra/board-harmony-pcie.c b/arch/arm/mach-tegra/board-harmony-pcie.c
...
> -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:-)

> diff --git a/arch/arm/mach-tegra/board-trimslice.c b/arch/arm/mach-tegra/board-trimslice.c

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

You can remove that test now.

> diff --git a/arch/arm/mach-tegra/board.h b/arch/arm/mach-tegra/board.h

> +#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.

Aside from those pretty minor issues, this all looks great. Thanks for
fixing our driver!

Re: [PATCH 09/11] ARM: tegra: Rewrite PCIe support as a driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1660 bytes --]

* Stephen Warren wrote:
> On 03/08/2012 07:51 AM, Thierry Reding wrote:
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> 
> Patch description?

I actually sat in front of the editor for a couple of minutes and I just
couldn't think of anything. I'll try harder for the next version.

> > diff --git a/arch/arm/mach-tegra/board-harmony-pcie.c b/arch/arm/mach-tegra/board-harmony-pcie.c
> ...
> > -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.

Okay, I'll rename it to harmony_pcie_init().

> I wonder how having a per-board callback will integrate with device
> tree. I guess I'll find out when I review patch 11:-)
> 
> > diff --git a/arch/arm/mach-tegra/board-trimslice.c b/arch/arm/mach-tegra/board-trimslice.c
> 
> > +static int __init trimslice_pci_init(void)
> >  {
> >  	if (!machine_is_trimslice())
> >  		return 0;
> 
> You can remove that test now.

Okay.

> > diff --git a/arch/arm/mach-tegra/board.h b/arch/arm/mach-tegra/board.h
> 
> > +#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.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 09/11] ARM: tegra: Rewrite PCIe support as a driver

[Stephen Warren]

On 03/08/2012 11:37 PM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 03/08/2012 07:51 AM, Thierry Reding wrote:
>>> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
...
>>> diff --git a/arch/arm/mach-tegra/board.h b/arch/arm/mach-tegra/board.h
>>
>>> +#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.

New files in that directory should have tegra in the name somewhere, so
that when multi-SoC kernels are built, and multiple mach-*/include/mach
directories are in the include path (or files get moved from those paths
into a single common place), you know which include file you're getting.

[PATCH 10/11] ARM: tegra: pcie: Add MSI support

[Thierry Reding]

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

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
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/Kconfig             |    1 +
 arch/arm/mach-tegra/devices.c           |    7 +
 arch/arm/mach-tegra/include/mach/irqs.h |    5 +-
 arch/arm/mach-tegra/pcie.c              |  239 +++++++++++++++++++++++++++++++
 4 files changed, 251 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
index 1651119..7c596e6 100644
--- a/arch/arm/mach-tegra/Kconfig
+++ b/arch/arm/mach-tegra/Kconfig
@@ -48,6 +48,7 @@ config ARCH_TEGRA_3x_SOC
 config TEGRA_PCI
 	bool "PCI Express support"
 	depends on ARCH_TEGRA_2x_SOC
+	select ARCH_SUPPORTS_MSI
 	select PCI
 
 comment "Tegra board type"
diff --git a/arch/arm/mach-tegra/devices.c b/arch/arm/mach-tegra/devices.c
index 09e24e1..195f165 100644
--- a/arch/arm/mach-tegra/devices.c
+++ b/arch/arm/mach-tegra/devices.c
@@ -721,6 +721,13 @@ static struct resource tegra_pcie_resources[] = {
 		.end = INT_PCIE_INTR,
 		.flags = IORESOURCE_IRQ,
 	},
+#ifdef CONFIG_PCI_MSI
+	[3] = {
+		.start = INT_PCIE_MSI,
+		.end = INT_PCIE_MSI,
+		.flags = IORESOURCE_IRQ,
+	},
+#endif
 };
 
 struct platform_device tegra_pcie_device = {
diff --git a/arch/arm/mach-tegra/include/mach/irqs.h b/arch/arm/mach-tegra/include/mach/irqs.h
index aad1a2c..02e84bc 100644
--- a/arch/arm/mach-tegra/include/mach/irqs.h
+++ b/arch/arm/mach-tegra/include/mach/irqs.h
@@ -172,7 +172,10 @@
 /* Tegra30 has 8 banks of 32 GPIOs */
 #define INT_GPIO_NR			(32 * 8)
 
-#define TEGRA_NR_IRQS			(INT_GPIO_BASE + INT_GPIO_NR)
+#define INT_PCI_MSI_BASE		(INT_GPIO_BASE + INT_GPIO_NR)
+#define INT_PCI_MSI_NR			(32 * 8)
+
+#define TEGRA_NR_IRQS			(INT_PCI_MSI_BASE + INT_PCI_MSI_NR)
 
 #define INT_BOARD_BASE			TEGRA_NR_IRQS
 #define NR_BOARD_IRQS			32
diff --git a/arch/arm/mach-tegra/pcie.c b/arch/arm/mach-tegra/pcie.c
index 8b20bc5..85db9fb 100644
--- a/arch/arm/mach-tegra/pcie.c
+++ b/arch/arm/mach-tegra/pcie.c
@@ -31,11 +31,14 @@
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/export.h>
+#include <linux/msi.h>
 
 #include <asm/sizes.h>
+#include <asm/mach/irq.h>
 #include <asm/mach/pci.h>
 
 #include <mach/iomap.h>
@@ -81,6 +84,24 @@
 #define AFI_MSI_FPCI_BAR_ST	0x64
 #define AFI_MSI_AXI_BAR_ST	0x68
 
+#define AFI_MSI_VEC0		0x6c
+#define AFI_MSI_VEC1		0x70
+#define AFI_MSI_VEC2		0x74
+#define AFI_MSI_VEC3		0x78
+#define AFI_MSI_VEC4		0x7c
+#define AFI_MSI_VEC5		0x80
+#define AFI_MSI_VEC6		0x84
+#define AFI_MSI_VEC7		0x88
+
+#define AFI_MSI_EN_VEC0		0x8c
+#define AFI_MSI_EN_VEC1		0x90
+#define AFI_MSI_EN_VEC2		0x94
+#define AFI_MSI_EN_VEC3		0x98
+#define AFI_MSI_EN_VEC4		0x9c
+#define AFI_MSI_EN_VEC5		0xa0
+#define AFI_MSI_EN_VEC6		0xa4
+#define AFI_MSI_EN_VEC7		0xa8
+
 #define AFI_CONFIGURATION		0xac
 #define  AFI_CONFIGURATION_EN_FPCI	(1 << 0)
 
@@ -211,6 +232,14 @@ struct tegra_pcie_info {
 	struct clk		*afi_clk;
 	struct clk		*pcie_xclk;
 	struct clk		*pll_e;
+
+#ifdef CONFIG_PCI_MSI
+	int msi_irq;
+	struct irq_chip msi_chip;
+	DECLARE_BITMAP(msi_in_use, INT_PCI_MSI_NR);
+	struct irq_domain *msi_domain;
+	struct mutex msi_lock;
+#endif
 };
 
 static inline struct tegra_pcie_info *sys_to_pcie(struct pci_sys_data *sys)
@@ -939,6 +968,208 @@ static void __devinit tegra_pcie_add_port(struct tegra_pcie_info *pcie,
 	memset(pp->res, 0, sizeof(pp->res));
 }
 
+#ifdef CONFIG_PCI_MSI
+static int tegra_pcie_msi_alloc(struct tegra_pcie_info *pcie)
+{
+	int msi;
+
+	mutex_lock(&pcie->msi_lock);
+
+	msi = find_first_zero_bit(pcie->msi_in_use, INT_PCI_MSI_NR);
+	if (msi < INT_PCI_MSI_NR)
+		set_bit(msi, pcie->msi_in_use);
+	else
+		msi = -ENOSPC;
+
+	mutex_unlock(&pcie->msi_lock);
+
+	return msi;
+}
+
+static void tegra_pcie_msi_free(struct tegra_pcie_info *pcie, unsigned long irq)
+{
+	mutex_lock(&pcie->msi_lock);
+
+	if (!test_bit(irq, pcie->msi_in_use))
+		dev_err(pcie->dev, "trying to free unused MSI#%lu\n", irq);
+	else
+		clear_bit(irq, pcie->msi_in_use);
+
+	mutex_unlock(&pcie->msi_lock);
+}
+
+static void tegra_pcie_msi_isr(unsigned int irq, struct irq_desc *desc)
+{
+	struct tegra_pcie_info *pcie = irq_get_handler_data(irq);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned int i;
+
+	chained_irq_enter(chip, desc);
+
+	for (i = 0; i < 8; i++) {
+		unsigned long reg = afi_readl(pcie, AFI_MSI_VEC0 + i * 4);
+
+		while (reg) {
+			unsigned int offset = find_first_bit(&reg, 32);
+			unsigned int index = i * 32 + offset;
+			unsigned int irq;
+
+			irq = irq_find_mapping(pcie->msi_domain, index);
+			if (irq) {
+				if (test_bit(index, pcie->msi_in_use))
+					generic_handle_irq(irq);
+				else
+					dev_info(pcie->dev, "unhandled MSI\n");
+			} else {
+				/*
+				 * that's weird who triggered this?
+				 * just clear it
+				 */
+				dev_info(pcie->dev, "unexpected MSI\n");
+			}
+
+			/* clear the interrupt */
+			afi_writel(pcie, 1 << offset, AFI_MSI_VEC0 + i * 4);
+			/* see if there's any more pending in this vector */
+			reg = afi_readl(pcie, AFI_MSI_VEC0 + i * 4);
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+/* called by arch_setup_msi_irqs in drivers/pci/msi.c */
+int arch_setup_msi_irq(struct pci_dev *pdev, struct msi_desc *desc)
+{
+	struct tegra_pcie_info *pcie = sys_to_pcie(pdev->bus->sysdata);
+	struct msi_msg msg;
+	unsigned int irq;
+	int hwirq;
+
+	hwirq = tegra_pcie_msi_alloc(pcie);
+	if (hwirq < 0)
+		return hwirq;
+
+	irq = irq_find_mapping(pcie->msi_domain, hwirq);
+	if (!irq)
+		return -EINVAL;
+
+	irq_set_msi_desc(irq, desc);
+
+	msg.address_lo = afi_readl(pcie, AFI_MSI_AXI_BAR_ST);
+	/* 32 bit address only */
+	msg.address_hi = 0;
+	msg.data = hwirq;
+
+	write_msi_msg(irq, &msg);
+
+	return 0;
+}
+
+void arch_teardown_msi_irq(unsigned int irq)
+{
+	struct tegra_pcie_info *pcie = irq_get_chip_data(irq);
+	struct irq_data *d = irq_get_irq_data(irq);
+
+	tegra_pcie_msi_free(pcie, d->hwirq);
+}
+
+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;
+	}
+
+	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");
+		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);
+		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);
+		return err;
+	}
+
+	pcie->msi_irq = err;
+
+	irq_set_chained_handler(pcie->msi_irq, tegra_pcie_msi_isr);
+	irq_set_handler_data(pcie->msi_irq, pcie);
+
+	/* setup AFI/FPCI range */
+	pages = (volatile void *)__get_free_pages(GFP_KERNEL, 3);
+	base = virt_to_phys(pages);
+
+	afi_writel(pcie, base, AFI_MSI_FPCI_BAR_ST);
+	afi_writel(pcie, base, AFI_MSI_AXI_BAR_ST);
+	/* this register is in 4K increments */
+	afi_writel(pcie, 1, AFI_MSI_BAR_SZ);
+
+	/* enable all MSI vectors */
+	afi_writel(pcie, 0xffffffff, AFI_MSI_EN_VEC0);
+	afi_writel(pcie, 0xffffffff, AFI_MSI_EN_VEC1);
+	afi_writel(pcie, 0xffffffff, AFI_MSI_EN_VEC2);
+	afi_writel(pcie, 0xffffffff, AFI_MSI_EN_VEC3);
+	afi_writel(pcie, 0xffffffff, AFI_MSI_EN_VEC4);
+	afi_writel(pcie, 0xffffffff, AFI_MSI_EN_VEC5);
+	afi_writel(pcie, 0xffffffff, AFI_MSI_EN_VEC6);
+	afi_writel(pcie, 0xffffffff, AFI_MSI_EN_VEC7);
+
+	/* and unmask the MSI interrupt */
+	reg = afi_readl(pcie, AFI_INTR_MASK);
+	reg |= AFI_INTR_MASK_MSI_MASK;
+	afi_writel(pcie, reg, AFI_INTR_MASK);
+
+	return 0;
+}
+
+static int tegra_pcie_disable_msi(struct platform_device *pdev)
+{
+	return 0;
+}
+#else
+static int tegra_pcie_enable_msi(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static int tegra_pcie_disable_msi(struct platform_device *pdev)
+{
+	return 0;
+}
+#endif
+
 static int __devinit tegra_pcie_probe(struct platform_device *pdev)
 {
 	struct tegra_pcie_pdata *pdata = pdev->dev.platform_data;
@@ -978,6 +1209,10 @@ static int __devinit tegra_pcie_probe(struct platform_device *pdev)
 	/* setup the AFI address translations */
 	tegra_pcie_setup_translations(pcie);
 
+	err = tegra_pcie_enable_msi(pdev);
+	if (err < 0)
+		dev_err(&pdev->dev, "failed to enable MSI support: %d\n", err);
+
 	if (pdata->enable_ports[0])
 		tegra_pcie_add_port(pcie, 0, RP0_OFFSET, AFI_PEX0_CTRL);
 
@@ -1000,6 +1235,10 @@ static int __devexit tegra_pcie_remove(struct platform_device *pdev)
 	struct tegra_pcie_pdata *pdata = pdev->dev.platform_data;
 	int err;
 
+	err = tegra_pcie_disable_msi(pdev);
+	if (err < 0)
+		return err;
+
 	err = tegra_pcie_put_resources(pdev);
 	if (err < 0)
 		return err;
-- 
1.7.9.3

Re: [PATCH 10/11] ARM: tegra: pcie: Add MSI support

[Stephen Warren]

On 03/08/2012 07:51 AM, Thierry Reding wrote:
> This commit adds support for message signaled interrupts to the Tegra
> PCIe controller.
> 
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> ---
> 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 <kthota@nvidia.com>" is enough...

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

> +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;
> +	}
> +
> +	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?

> +		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);
> +		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?

> +		return err;
> +	}
...

> +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?

Re: [PATCH 10/11] ARM: tegra: pcie: Add MSI support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4017 bytes --]

* Stephen Warren wrote:
> On 03/08/2012 07:51 AM, Thierry Reding wrote:
> > This commit adds support for message signaled interrupts to the Tegra
> > PCIe controller.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > ---
> > 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 <kthota@nvidia.com>" 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?

Thierry

> 
> > diff --git a/arch/arm/mach-tegra/pcie.c b/arch/arm/mach-tegra/pcie.c
> 
> > +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;
> > +	}
> > +
> > +	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?

Yes, that would make sense.

> > +		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);
> > +		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?

Right. I assume it would be best to also free the struct irq_domain and set
the chip data and handler back to NULL? AFAICT there is no canonical way to
teardown an irq_domain.

> > +		return err;
> > +	}
> ...
> 
> > +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?

Yes, initially this contained a call to free_irq(), which I removed when I
switched to using a chained handler. I can probably put all of the cleanup
code from your comments above in here and perhaps even call that in the error
paths of tegra_pcie_enable_msi().

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 10/11] ARM: tegra: pcie: Add MSI support

[Stephen Warren]

On 03/08/2012 11:50 PM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 03/08/2012 07:51 AM, Thierry Reding wrote:
>>> This commit adds support for message signaled interrupts to the Tegra
>>> PCIe controller.
>>>
>>> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
>>> ---
>>> 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 <kthota@nvidia.com>" 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?

Yes, I think just mentioning the original code author in free-form text,
and including only your S-o-b sounds good to me.

Re: [PATCH 10/11] ARM: tegra: pcie: Add MSI support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2264 bytes --]

* Stephen Warren wrote:
> On 03/08/2012 07:51 AM, Thierry Reding wrote:
> > diff --git a/arch/arm/mach-tegra/pcie.c b/arch/arm/mach-tegra/pcie.c
> 
> > +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;
> > +	}
> > +
> > +	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?
> 
> > +		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);
> > +		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 also think it would be necessary
to free the struct irq_domain, but I wasn't able to find any function that
does this other than just calling kfree(), which obviously wouldn't be
enough. Maybe Grant can shed some light onto this. 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.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 10/11] ARM: tegra: pcie: Add MSI support

[Stephen Warren]

On 03/12/2012 02:00 AM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 03/08/2012 07:51 AM, Thierry Reding wrote:
>>> diff --git a/arch/arm/mach-tegra/pcie.c b/arch/arm/mach-tegra/pcie.c
...
>> Free the IRQ descriptors in the error paths?
...
>>> +	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);
>>> +		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?

Good point. Probably not per my gut instinct.

...
> 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.

But yes, best to check with someone more familiar with interrupts.

[PATCH 11/11] ARM: tegra: pcie: Add device tree support

[Thierry Reding]

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

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 .../devicetree/bindings/pci/tegra-pcie.txt         |   23 ++++
 arch/arm/boot/dts/tegra20.dtsi                     |    8 ++
 arch/arm/mach-tegra/pcie.c                         |  123 +++++++++++++++++++-
 3 files changed, 151 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pci/tegra-pcie.txt

diff --git a/Documentation/devicetree/bindings/pci/tegra-pcie.txt b/Documentation/devicetree/bindings/pci/tegra-pcie.txt
new file mode 100644
index 0000000..10bbc40
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/tegra-pcie.txt
@@ -0,0 +1,23 @@
+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)
+
+Example:
+
+	pci@80000000 {
+		compatible = "nvidia,tegra20-pcie",
+		reg = <0x80000000 0x400000   /* PCIe AFI registers */
+		       0x80400000 0x010000>; /* PCI I/O space */
+		interrupts = < 0 98 0x04    /* controller interrupt */
+		               0 99 0x04 >; /* MSI interrupt */
+
+		pex-clk-supply = <&ldo0_reg>;
+		vdd-supply = <&pcie_reg>;
+	};
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 108e894..9b4692e 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -161,6 +161,14 @@
 		reg = <0x7000f400 0x200>;
 	};
 
+	pci@80000000 {
+		compatible = "nvidia,tegra20-pcie";
+		reg = <0x80000000 0x400000   /* PCIe AFI registers */
+		       0x80400000 0x010000>; /* PCI I/O space */
+		interrupts = < 0 98 0x04    /* controller interrupt */
+		               0 99 0x04 >; /* MSI interrupt */
+	};
+
 	sdhci@c8000000 {
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000000 0x200>;
diff --git a/arch/arm/mach-tegra/pcie.c b/arch/arm/mach-tegra/pcie.c
index 85db9fb..829af6e 100644
--- a/arch/arm/mach-tegra/pcie.c
+++ b/arch/arm/mach-tegra/pcie.c
@@ -36,6 +36,8 @@
 #include <linux/delay.h>
 #include <linux/export.h>
 #include <linux/msi.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
 
 #include <asm/sizes.h>
 #include <asm/mach/irq.h>
@@ -240,6 +242,9 @@ struct tegra_pcie_info {
 	struct irq_domain *msi_domain;
 	struct mutex msi_lock;
 #endif
+
+	struct regulator *pex_clk_supply;
+	struct regulator *vdd_supply;
 };
 
 static inline struct tegra_pcie_info *sys_to_pcie(struct pci_sys_data *sys)
@@ -1170,15 +1175,110 @@ static int tegra_pcie_disable_msi(struct platform_device *pdev)
 }
 #endif
 
+#ifdef CONFIG_OF
+static int tegra_pcie_dt_init(struct platform_device *pdev)
+{
+	struct tegra_pcie_info *pcie = platform_get_drvdata(pdev);
+	int err;
+
+	if (!IS_ERR_OR_NULL(pcie->vdd_supply)) {
+		err = regulator_enable(pcie->vdd_supply);
+		if (err < 0) {
+			dev_err(&pdev->dev,
+				"failed to enable VDD regulator: %d\n", err);
+			return err;
+		}
+	}
+
+	if (!IS_ERR_OR_NULL(pcie->pex_clk_supply)) {
+		err = regulator_enable(pcie->pex_clk_supply);
+		if (err < 0) {
+			dev_err(&pdev->dev,
+				"failed to enable pex-clk regulator: %d\n",
+				err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static int tegra_pcie_dt_exit(struct platform_device *pdev)
+{
+	struct tegra_pcie_info *pcie = platform_get_drvdata(pdev);
+	int err;
+
+	if (!IS_ERR_OR_NULL(pcie->pex_clk_supply)) {
+		err = regulator_disable(pcie->pex_clk_supply);
+		if (err < 0) {
+			dev_err(&pdev->dev,
+				"failed to disable pex-clk regulator: %d\n",
+				err);
+			return err;
+		}
+	}
+
+	if (!IS_ERR_OR_NULL(pcie->vdd_supply)) {
+		err = regulator_disable(pcie->vdd_supply);
+		if (err < 0) {
+			dev_err(&pdev->dev,
+				"failed to disable VDD regulator: %d\n", err);
+			return err;
+		}
+	}
+
+	return 0;
+}
+
+static struct tegra_pcie_pdata *tegra_pcie_parse_dt(struct platform_device *pdev)
+{
+	struct tegra_pcie_info *pcie = platform_get_drvdata(pdev);
+	struct device_node *node = pdev->dev.of_node;
+	struct tegra_pcie_pdata *pdata;
+	unsigned int i;
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	pdata->init = tegra_pcie_dt_init;
+	pdata->exit = tegra_pcie_dt_exit;
+
+	if (of_find_property(node, "vdd-supply", NULL)) {
+		pcie->vdd_supply = regulator_get(&pdev->dev, "vdd");
+		if (IS_ERR_OR_NULL(pcie->vdd_supply))
+			return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	if (of_find_property(node, "pex-clk-supply", NULL)) {
+		pcie->pex_clk_supply = regulator_get(&pdev->dev, "pex-clk");
+		if (IS_ERR_OR_NULL(pcie->pex_clk_supply))
+			return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	for (i = 0; i < TEGRA_PCIE_MAX_PORTS; i++)
+		pdata->enable_ports[i] = true;
+
+	return pdata;
+}
+
+static struct of_device_id tegra_pcie_of_match[] = {
+	{ .compatible = "nvidia,tegra20-pcie", },
+	{ },
+};
+#else
+static struct tegra_pcie_pdata *tegra_pcie_parse_dt(struct platform_device *pdev)
+{
+	return NULL;
+}
+#endif
+
 static int __devinit tegra_pcie_probe(struct platform_device *pdev)
 {
 	struct tegra_pcie_pdata *pdata = pdev->dev.platform_data;
 	struct tegra_pcie_info *pcie;
 	int err;
 
-	if (!pdata->enable_ports[0] && !pdata->enable_ports[1])
-		return -ENODEV;
-
 	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
 	if (!pcie)
 		return -ENOMEM;
@@ -1186,6 +1286,18 @@ static int __devinit tegra_pcie_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, pcie);
 	pcie->dev = &pdev->dev;
 
+	if (!pdata && pdev->dev.of_node) {
+		pdata = tegra_pcie_parse_dt(pdev);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+	}
+
+	if (!pdata)
+		return -ENODEV;
+
+	if (!pdata->enable_ports[0] && !pdata->enable_ports[1])
+		return -ENODEV;
+
 	pcibios_min_mem = 0;
 
 	err = tegra_pcie_get_resources(pdev);
@@ -1232,6 +1344,7 @@ static int __devinit tegra_pcie_probe(struct platform_device *pdev)
 
 static int __devexit tegra_pcie_remove(struct platform_device *pdev)
 {
+	struct tegra_pcie_info *pcie = platform_get_drvdata(pdev);
 	struct tegra_pcie_pdata *pdata = pdev->dev.platform_data;
 	int err;
 
@@ -1249,6 +1362,9 @@ static int __devexit tegra_pcie_remove(struct platform_device *pdev)
 			return err;
 	}
 
+	regulator_put(pcie->pex_clk_supply);
+	regulator_put(pcie->vdd_supply);
+
 	return 0;
 }
 
@@ -1256,6 +1372,7 @@ static struct platform_driver tegra_pcie_driver = {
 	.driver = {
 		.name = "tegra-pcie",
 		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(tegra_pcie_of_match),
 	},
 	.probe = tegra_pcie_probe,
 	.remove = __devexit_p(tegra_pcie_remove),
-- 
1.7.9.3

Re: [PATCH 11/11] ARM: tegra: pcie: Add device tree support

[Stephen Warren]

On 03/08/2012 07:51 AM, Thierry Reding wrote:
> This commit adds support for instantiating the Tegra PCIe controller
> from a device tree.
> 
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>

> diff --git a/Documentation/devicetree/bindings/pci/tegra-pcie.txt b/Documentation/devicetree/bindings/pci/tegra-pcie.txt

> +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.

Re: [PATCH 11/11] ARM: tegra: pcie: Add device tree support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

* Stephen Warren wrote:
> On 03/08/2012 07:51 AM, Thierry Reding wrote:
> > This commit adds support for instantiating the Tegra PCIe controller
> > from a device tree.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> 
> > diff --git a/Documentation/devicetree/bindings/pci/tegra-pcie.txt b/Documentation/devicetree/bindings/pci/tegra-pcie.txt
> 
> > +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.

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.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 11/11] ARM: tegra: pcie: Add device tree support

[Stephen Warren]

On 03/08/2012 11:31 PM, Thierry Reding wrote:
> * Stephen Warren wrote:
>> On 03/08/2012 07:51 AM, Thierry Reding wrote:
>>> This commit adds support for instantiating the Tegra PCIe controller
>>> from a device tree.
>>>
>>> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
>>
>>> diff --git a/Documentation/devicetree/bindings/pci/tegra-pcie.txt b/Documentation/devicetree/bindings/pci/tegra-pcie.txt
>>
>>> +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.
> 
> 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.

Yes. I believe that's exactly what the fixed regulator is for.

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

Mark indicated dummy regulators weren't the preferred mechanism:

http://lkml.org/lkml/2012/2/25/30

Re: [PATCH 11/11] ARM: tegra: pcie: Add device tree support

[Mark Brown]

On Thu, Mar 08, 2012 at 02:31:41PM -0700, Stephen Warren wrote:
> On 03/08/2012 07:51 AM, Thierry Reding wrote:

> > +- 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.

Re: [PATCH 11/11] ARM: tegra: pcie: Add device tree support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2045 bytes --]

* Mark Brown wrote:
> On Thu, Mar 08, 2012 at 02:31:41PM -0700, Stephen Warren wrote:
> > On 03/08/2012 07:51 AM, Thierry Reding wrote:
> 
> > > +- 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.

The PCIe controller actually needs those supplies and won't work without
them. However on TrimSlice both of these supplies seem to be hardwired and
always on. At least that's what I deduce from the code because there is no
code that enables either the 1.05V or the pex-clk supplies.

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.

Then again, the fixed regulator binding says that the gpio property is
optional, so perhaps doing something like the following will work:

	pex_clk_reg: fixedregulator@0 {
		compatible = "regulator-fixed";
		regulator-name = "PCIE CLK";
		regulator-min-microvolt = <3300000>;
		regulator-max-microvolt = <3300000>;
		regulator-always-on;
	};

	pcie_reg: fixedregulator@1 {
		compatible = "regulator-fixed";
		regulator-name = "PCIE VDD";
		regulator-min-microvolt = <1050000>;
		regulator-max-microvolt = <1050000>;
		regulator-always-on;
	};

Is that how it is supposed to work?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 11/11] ARM: tegra: pcie: Add device tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 665 bytes --]

On Mon, Mar 12, 2012 at 03:17:05PM +0100, Thierry Reding wrote:

> 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.

> Is that how it is supposed to work?

Yes.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 11/11] ARM: tegra: pcie: Add device tree support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 966 bytes --]

* Mark Brown wrote:
> On Mon, Mar 12, 2012 at 03:17:05PM +0100, Thierry Reding wrote:
> 
> > 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.
> 
> > 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?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 11/11] ARM: tegra: pcie: Add device tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 580 bytes --]

On Mon, Mar 12, 2012 at 03:28:58PM +0100, Thierry Reding wrote:

> 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?

Probably the best thing here is to just reference the regulator binding
documentation which mentions -supply properties (or should do) and then
clarify that if need be.  No sense in putting things like this into the
binding document for a specific driver, there's nothing driver specific.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 00/11] ARM: tegra: Add PCIe device tree support

[Rob Herring]

On 03/08/2012 08:51 AM, Thierry Reding wrote:
> 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.

Rob

Re: [PATCH 00/11] ARM: tegra: Add PCIe device tree support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

* Rob Herring wrote:
> 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.

My particular setup may be much simpler than the average PowerPC machine. The
patchset is only what I need to get PCIe running on the platform, which has
an FPGA connected to the Tegra via PCIe. For instance there is no legacy
interrupt support (only MSI), so interrupt remapping is not required. I also
know that OF nodes are not properly hooked up with the PCI devices yet, which
I will need, among other things, to provide platform data for an SPI flash
connected to an SPI master implemented within the FPGA. I also haven't looked
at specifying the memory ranges via the device tree.

So quite a lot may still be missing, but does that prevent any of this from
being added to mainline? Things can be added on an as-needed basis, can't
they?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 00/11] ARM: tegra: Add PCIe device tree support

[Grant Likely]

On Thu,  8 Mar 2012 15:51:20 +0100, Thierry Reding <thierry.reding@avionic-design.de> wrote:
> 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

Greg has picked up the drivercore patch, so the regulator one can be queued
up too now.

g.

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies,Ltd.

Re: [PATCH 00/11] ARM: tegra: Add PCIe device tree support

[Olof Johansson]

On Thu, Mar 08, 2012 at 03:51:20PM +0100, Thierry Reding wrote:
> 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?

Hi,

This patch series looks good to me, but I'm out of time to stage it for
3.4. Stephen is taking over lead maintainership for 3.5 and forward,
so I'll let him pick it up for that release.


-Olof