R-Car Gen2 PCIe host driver

drivers/pci/host/pcie-rcar.c new file +19 -0

+static int rcar_pcie_setup_window(int win, struct resource *res,
+					struct rcar_pcie *pcie)
+{
+	/* Setup PCIe address space mappings for each resource */
+	resource_size_t size;
+	u32 mask;
+
+	pci_write_reg(pcie, 0x00000000, PCIEPTCTLR(win));
+
+	/*
+	 * The PAMR mask is calculated in units of 128Bytes, which
+	 * keeps things pretty simple.
+	 */
+	size = resource_size(res);
+	mask = (roundup_pow_of_two(size) / SZ_128) - 1;
+	pci_write_reg(pcie, mask << 7, PCIEPAMR(win));
+
+	pci_write_reg(pcie, upper_32_bits(res->start), PCIEPARH(win));
+	pci_write_reg(pcie, lower_32_bits(res->start), PCIEPARL(win));

drivers/pci/host/pcie-rcar.c new file +20 -0

+
+static int rcar_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+	struct rcar_pcie *pcie = sys_to_pcie(sys);
+	struct resource *res;
+	int i, ret;
+
+	pcie->root_bus_nr = sys->busnr;
+
+	/* Setup PCI resources */
+	for (i = 0; i < PCI_MAX_RESOURCES; i++) {
+
+		res = &pcie->res[i];
+		if (!res->flags)
+			continue;
+
+		if (res->flags & IORESOURCE_IO) {
+			/* Setup IO mapped memory accesses */
+			ret = pci_ioremap_io(0, res->start);
+			if (ret)

From: Arnd Bergmann <[email protected]>
To: linux-arm-kernel at lists.infradead.org,
Cc: Phil Edworthy <[email protected]>,

linux-sh at vger.kernel.org, Magnus Damm <[email protected]>, Valentine

Barshak <[email protected]>, Simon Horman <[email protected]>, Bjorn Helgaas <[email protected]>, Ben Dooks <[email protected]>
Date: 21/03/2014 11:04
Subject: Re: [PATCH v4 1/9] PCI: host: rcar: Add Renesas R-Car PCIe

drivers/pci/host/pcie-rcar.c

Quoted 3 lines

drivers/pci/host/pcie-rcar.c new file +3 -0

+static const struct of_device_id rcar_pcie_of_match[] = {
+   { .compatible = "renesas,r8a7779-pcie", .data = (void *)RCAR_H1 },
+   { .compatible = "renesas,r8a7790-pcie", .data = (void 

Quoted 1 line

+ { .compatible = "renesas,r8a7791-pcie", .data = (void

drivers/pci/host/pcie-rcar.c line 53

Quoted 3 lines

drivers/pci/host/pcie-rcar.c new file +3 -0

+   {},
+};
+MODULE_DEVICE_TABLE(of, rcar_pcie_of_match);

The only difference between RCAR_H1 and RCAR_GENERIC seems to be the use of the rcar_pcie_phy_init_rcar_h1 vs rcar_pcie_hw_init functions. I think this would be expressed in a nicer way doing

static const struct of_device_id rcar_pcie_of_match[] = {
   { .compatible = "renesas,r8a7779-pcie", .data = 

   { .compatible = "renesas,r8a7790-pcie", .data =  rcar_pcie_hw_init},
   { .compatible = "renesas,r8a7791-pcie", .data =  rcar_pcie_hw_init},
   {},
};

If you need other differences in the future, you can extend this to use a pointer to a struct containing the init function pointer.

Quoted 24 lines

drivers/pci/host/pcie-rcar.c new file +24 -0

+static int rcar_pcie_setup_window(int win, struct resource *res,
+               struct rcar_pcie *pcie)
+{
+   /* Setup PCIe address space mappings for each resource */
+   resource_size_t size;
+   u32 mask;
+
+   pci_write_reg(pcie, 0x00000000, PCIEPTCTLR(win));
+
+   /*
+    * The PAMR mask is calculated in units of 128Bytes, which
+    * keeps things pretty simple.
+    */
+   size = resource_size(res);
+   mask = (roundup_pow_of_two(size) / SZ_128) - 1;
+   pci_write_reg(pcie, mask << 7, PCIEPAMR(win));
+
+   pci_write_reg(pcie, upper_32_bits(res->start), PCIEPARH(win));
+   pci_write_reg(pcie, lower_32_bits(res->start), PCIEPARL(win));
+
+   /* First resource is for IO */
+   mask = PAR_ENABLE;
+   if (res->flags & IORESOURCE_IO)
+      mask |= IO_SPACE;

drivers/pci/host/pcie-rcar.c line 280

Quoted 5 lines

drivers/pci/host/pcie-rcar.c new file +5 -0

+
+   pci_write_reg(pcie, mask, PCIEPTCTLR(win));
+
+   return 0;
+}

Would it be possible to have this done in the boot loader so the kernel doesn't need to be bothered with it?

Quoted 26 lines

drivers/pci/host/pcie-rcar.c new file +26 -0

+
+static int rcar_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+   struct rcar_pcie *pcie = sys_to_pcie(sys);
+   struct resource *res;
+   int i, ret;
+
+   pcie->root_bus_nr = sys->busnr;
+
+   /* Setup PCI resources */
+   for (i = 0; i < PCI_MAX_RESOURCES; i++) {
+
+      res = &pcie->res[i];
+      if (!res->flags)
+         continue;
+
+      if (res->flags & IORESOURCE_IO) {
+         /* Setup IO mapped memory accesses */
+         ret = pci_ioremap_io(0, res->start);
+         if (ret)
+            return 1;
+
+         /* Correct addresses for remapped IO */
+         res->end = res->end - res->start;
+         res->start = 0;
+      }

drivers/pci/host/pcie-rcar.c line 310

Quoted 4 lines

drivers/pci/host/pcie-rcar.c new file +4 -0

+
+      rcar_pcie_setup_window(i, res, pcie);
+      pci_add_resource(&sys->resources, res);
+   }

This assumes that the start of the I/O window is always at bus address 0, and that you have only one PCI host in the system. Are both guaranteed through the hardware design?

drivers/pci/host/pcie-rcar.c line 326

Quoted 12 lines

drivers/pci/host/pcie-rcar.c new file +12 -0

+static void __init rcar_pcie_enable(struct rcar_pcie *pcie)
+{
+   struct platform_device *pdev = to_platform_device(pcie->dev);
+   struct hw_pci hw;
+
+   memset(&hw, 0, sizeof(hw));
+
+   hw.nr_controllers = 1;
+   hw.private_data   = (void **)&pcie;
+   hw.setup          = rcar_pcie_setup,
+   hw.map_irq        = of_irq_parse_and_map_pci,
+   hw.ops        = &rcar_pcie_ops,

You can write this slightly nicer doing

   struct hw_pci hw = {
      .nr_controllers = 1,
      .private_data = (void **)&pcie,
      ...
   };

drivers/pci/host/pcie-rcar.c line 384

Quoted 12 lines

drivers/pci/host/pcie-rcar.c new file +12 -0

+static int __init rcar_pcie_phy_init_rcar_h1(struct rcar_pcie *pcie)
+{
+   unsigned int timeout = 10;
+
+   /* Initialize the phy */
+   phy_write_reg(pcie, 0, 0x42, 0x1, 0x0EC34191);
+   phy_write_reg(pcie, 1, 0x42, 0x1, 0x0EC34180);
+   phy_write_reg(pcie, 0, 0x43, 0x1, 0x00210188);
+   phy_write_reg(pcie, 1, 0x43, 0x1, 0x00210188);
+   phy_write_reg(pcie, 0, 0x44, 0x1, 0x015C0014);
+   phy_write_reg(pcie, 1, 0x44, 0x1, 0x015C0014);
+   phy_write_reg(pcie, 1, 0x4C, 0x1, 0x786174A0);

Have you considered moving this into a separate PHY driver? There are probably good reasons either way, so I'm not asking you to do it, just to think about what would be best for this driver.

It seems that you already have two different implementations that only vary in how they access the PHY, so moving that into a separate driver would keep the knowledge out of the host driver. OTOH, the PHY registers look like they are part of the pci host itself.

Review

Quoted 2 lines

(quoted diff) +2 -0

+   /*
+    * For target transfers, setup a single 64-bit 4GB mapping at 

Quoted 1 line

+ * 0. The hardware can only map memory that starts on a power of

drivers/pci/host/pcie-rcar.c line 443

Quoted 9 lines

drivers/pci/host/pcie-rcar.c new file +9 -0

+    * boundary, and size is power of 2, so best to ignore it.
+    */
+   pci_write_reg(pcie, 0, PCIEPRAR(0));
+   pci_write_reg(pcie, 0, PCIELAR(0));
+   pci_write_reg(pcie, 0xfffffff0UL | LAM_PREFETCH |
+      LAM_64BIT | LAR_ENABLE, PCIELAMR(0));
+   pci_write_reg(pcie, 0, PCIELAR(1));
+   pci_write_reg(pcie, 0, PCIEPRAR(1));
+   pci_write_reg(pcie, 0, PCIELAMR(1));

Is this the only reasonable configuration? If not, you might want to do the same thing as the x-gene PCI driver that is not yet merged, and parse the 'dma-ranges' property to determine what the mapping on a particular machine should be.

Most importantly, why don't you use a mapping that includes RAM beyond the first 4GB?

drivers/pci/host/pcie-rcar.c line 597

Quoted 5 lines

drivers/pci/host/pcie-rcar.c new file +5 -0

+static int __init pcie_init(void)
+{
+   return platform_driver_probe(&rcar_pcie_driver, rcar_pcie_probe);
+}
+subsys_initcall(pcie_init);

Why so early?

Overall, this driver looks pretty good.

Arnd

Hi Arnd,

Thanks for the review.

Quoted 7 lines

drivers/pci/host/pcie-rcar.c line 597

Quoted 5 lines

drivers/pci/host/pcie-rcar.c new file +5 -0

+static int __init pcie_init(void)
+{
+   return platform_driver_probe(&rcar_pcie_driver, rcar_pcie_probe);
+}
+subsys_initcall(pcie_init);

Why so early?

Good catch, copied from another PCIe driver!

Quoted 3 lines

From: Arnd Bergmann <[email protected]>

static const struct of_device_id rcar_pcie_of_match[] = {
   { .compatible = "renesas,r8a7779-pcie", .data = 

rcar_pcie_phy_init_rcar_h1},

Quoted 5 lines

   { .compatible = "renesas,r8a7790-pcie", .data =  rcar_pcie_hw_init},
   { .compatible = "renesas,r8a7791-pcie", .data =  rcar_pcie_hw_init},
   {},
};

Quoted 2 lines

If you need other differences in the future, you can extend this to use a pointer to a struct containing the init function pointer.

Ok, I understand what you mean, though the rcar_pcie_phy_init_rcar_h1 function is an extra call for r8a7779 devices to setup the PHY, not an alternative to rcar_pcie_hw_init.

Quoted 24 lines

Quoted 24 lines

drivers/pci/host/pcie-rcar.c new file +24 -0

+static int rcar_pcie_setup_window(int win, struct resource *res,
+               struct rcar_pcie *pcie)
+{
+   /* Setup PCIe address space mappings for each resource */
+   resource_size_t size;
+   u32 mask;
+
+   pci_write_reg(pcie, 0x00000000, PCIEPTCTLR(win));
+
+   /*
+    * The PAMR mask is calculated in units of 128Bytes, which
+    * keeps things pretty simple.
+    */
+   size = resource_size(res);
+   mask = (roundup_pow_of_two(size) / SZ_128) - 1;
+   pci_write_reg(pcie, mask << 7, PCIEPAMR(win));
+
+   pci_write_reg(pcie, upper_32_bits(res->start), PCIEPARH(win));
+   pci_write_reg(pcie, lower_32_bits(res->start), PCIEPARL(win));
+
+   /* First resource is for IO */
+   mask = PAR_ENABLE;
+   if (res->flags & IORESOURCE_IO)
+      mask |= IO_SPACE;

Quoted 8 lines

drivers/pci/host/pcie-rcar.c line 280

Quoted 5 lines

drivers/pci/host/pcie-rcar.c new file +5 -0

+
+   pci_write_reg(pcie, mask, PCIEPTCTLR(win));
+
+   return 0;
+}

Would it be possible to have this done in the boot loader so the kernel doesn't need to be bothered with it?

Hmm, I don't like the idea of depending on settings made by a bootloader, when it may be that a different bootloader could be used, or no bootloader at all.

Quoted 25 lines

Quoted 25 lines

drivers/pci/host/pcie-rcar.c new file +25 -0

+static int rcar_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+   struct rcar_pcie *pcie = sys_to_pcie(sys);
+   struct resource *res;
+   int i, ret;
+
+   pcie->root_bus_nr = sys->busnr;
+
+   /* Setup PCI resources */
+   for (i = 0; i < PCI_MAX_RESOURCES; i++) {
+
+      res = &pcie->res[i];
+      if (!res->flags)
+         continue;
+
+      if (res->flags & IORESOURCE_IO) {
+         /* Setup IO mapped memory accesses */
+         ret = pci_ioremap_io(0, res->start);
+         if (ret)
+            return 1;
+
+         /* Correct addresses for remapped IO */
+         res->end = res->end - res->start;
+         res->start = 0;
+      }

Quoted 8 lines

drivers/pci/host/pcie-rcar.c line 310

Quoted 4 lines

drivers/pci/host/pcie-rcar.c new file +4 -0

+
+      rcar_pcie_setup_window(i, res, pcie);
+      pci_add_resource(&sys->resources, res);
+   }

This assumes that the start of the I/O window is always at bus address 0, and that you have only one PCI host in the system. Are both guaranteed through the hardware design?

The I/O window can be anywhere I believe, so I guess I should pull the actual bus address from the DT.

All existing devices I have seen, have a single PCIe host.

Quoted 17 lines

drivers/pci/host/pcie-rcar.c line 384

Quoted 12 lines

drivers/pci/host/pcie-rcar.c new file +12 -0

+static int __init rcar_pcie_phy_init_rcar_h1(struct rcar_pcie *pcie)
+{
+   unsigned int timeout = 10;
+
+   /* Initialize the phy */
+   phy_write_reg(pcie, 0, 0x42, 0x1, 0x0EC34191);
+   phy_write_reg(pcie, 1, 0x42, 0x1, 0x0EC34180);
+   phy_write_reg(pcie, 0, 0x43, 0x1, 0x00210188);
+   phy_write_reg(pcie, 1, 0x43, 0x1, 0x00210188);
+   phy_write_reg(pcie, 0, 0x44, 0x1, 0x015C0014);
+   phy_write_reg(pcie, 1, 0x44, 0x1, 0x015C0014);
+   phy_write_reg(pcie, 1, 0x4C, 0x1, 0x786174A0);

Have you considered moving this into a separate PHY driver? There are probably good reasons either way, so I'm not asking you to do it, just to think about what would be best for this driver.

Quoted 5 lines

It seems that you already have two different implementations that only vary in how they access the PHY, so moving that into a separate driver would keep the knowledge out of the host driver. OTOH, the PHY registers look like they are part of the pci host itself.

The PHYs are separate entities, but access to them is via the PCIe controller's registers. In the general case, there shouldn't be any PHY setup to do, it's just the R-Car H1 devices that needs a little tweaking.

Quoted 2 lines

Most importantly, why don't you use a mapping that includes RAM beyond the first 4GB?

I had intended to send a patch later on to open this up to the first 12GB. Limitations of this block of hardware mean that we can do at most three 4GB regions. The first 12GB would be ok for LPAE on existing devices as all DDR is mapped within that range, but obviously not ideal.

Quoted 12 lines · On Friday 21 March 2014 13:59:35 Phil.Edworthy at renesas.com wrote:

Quoted 3 lines

From: Arnd Bergmann <[email protected]>

static const struct of_device_id rcar_pcie_of_match[] = {
   { .compatible = "renesas,r8a7779-pcie", .data =

rcar_pcie_phy_init_rcar_h1},

Quoted 7 lines

   { .compatible = "renesas,r8a7790-pcie", .data =  rcar_pcie_hw_init},
   { .compatible = "renesas,r8a7791-pcie", .data =  rcar_pcie_hw_init},
   {},
};

If you need other differences in the future, you can extend this to use a pointer to a struct containing the init function pointer.

Quoted 3 lines

Ok, I understand what you mean, though the rcar_pcie_phy_init_rcar_h1 function is an extra call for r8a7779 devices to setup the PHY, not an alternative to rcar_pcie_hw_init.

I assumed that would be trivial to change, by calling rcar_pcie_hw_init at the end of rcar_pcie_phy_init_rcar_h1 rather than the other way round.

Signed-off-by Phil Edworthy <[email protected]>

---
 drivers/pci/host/pcie-rcar.c | 232 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/host/pcie-rcar.h |   5 +
 2 files changed, 236 insertions(+), 1 deletion(-)

drivers/pci/host/pcie-rcar.c +0 -0

index 16670e5..cbbcd77 100644

drivers/pci/host/pcie-rcar.c +37 -0

+
+static irqreturn_t rcar_pcie_msi_irq(int irq, void *data)
+{
+	struct rcar_pcie *pcie = data;
+	struct rcar_msi *msi = &pcie->msi;
+	unsigned long reg;
+
+	reg = pci_read_reg(pcie, PCIEMSIFR);
+
+	while (reg) {
+		unsigned int index = find_first_bit(&reg, 32);
+		unsigned int irq;
+
+		/* clear the interrupt */
+		pci_write_reg(pcie, 1 << index, PCIEMSIFR);
+
+		irq = irq_find_mapping(msi->domain, index);
+		if (irq) {
+			if (test_bit(index, msi->used))
+				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");
+		}
+
+		/* see if there's any more pending in this vector */
+		reg = pci_read_reg(pcie, PCIEMSIFR);
+	}
+
+	return IRQ_HANDLED;
+}
+

From your DT binding it seems you have only one interrupt from the PCIe

Subject: Re: [PATCH v4 2/9] PCI: host: rcar: Add MSI support

Am Freitag, den 21.03.2014, 10:32 +0000 schrieb Phil Edworthy:

Quoted 3 lines

Signed-off-by Phil Edworthy <[email protected]>

---
 drivers/pci/host/pcie-rcar.c | 232 

Review

Quoted 4 lines

 drivers/pci/host/pcie-rcar.h |   5 +
 2 files changed, 236 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

Quoted 3 lines

index 16670e5..cbbcd77 100644

drivers/pci/host/pcie-rcar.c +0 -0

[...]

Quoted 33 lines

drivers/pci/host/pcie-rcar.c +33 -0

+
+static irqreturn_t rcar_pcie_msi_irq(int irq, void *data)
+{
+   struct rcar_pcie *pcie = data;
+   struct rcar_msi *msi = &pcie->msi;
+   unsigned long reg;
+
+   reg = pci_read_reg(pcie, PCIEMSIFR);
+
+   while (reg) {
+      unsigned int index = find_first_bit(&reg, 32);
+      unsigned int irq;
+
+      /* clear the interrupt */
+      pci_write_reg(pcie, 1 << index, PCIEMSIFR);
+
+      irq = irq_find_mapping(msi->domain, index);
+      if (irq) {
+         if (test_bit(index, msi->used))
+            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");
+      }
+
+      /* see if there's any more pending in this vector */
+      reg = pci_read_reg(pcie, PCIEMSIFR);
+   }

drivers/pci/host/pcie-rcar.c static void __init rcar_pcie_hw_init(struct rcar_pcie *pcie) · line 598

Quoted 4 lines

drivers/pci/host/pcie-rcar.c +4 -0

+
+   return IRQ_HANDLED;
+}
+

From your DT binding it seems you have only one interrupt from the PCIe core, shared between the MSI irqs and the PCI legacy interrupts. This means this handler may get called without an MSI irq pending, so this function really should have a path where it's returning IRQ_NONE.

Regards,
Lucas

Signature · 2 lines

Pengutronix e.K.                           | Lucas Stach |
Industrial Linux Solutions                 | http://www.pengutronix.de/ 

Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076

Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555

Hi Lucas,

Thanks for the review.

Quoted 6 lines · Lucas

Subject: Re: [PATCH v4 2/9] PCI: host: rcar: Add MSI support

Am Freitag, den 21.03.2014, 10:32 +0000 schrieb Phil Edworthy:

Quoted 3 lines

Signed-off-by Phil Edworthy <[email protected]>

---
 drivers/pci/host/pcie-rcar.c | 232 

++++++++++++++++++++++++++++++++++++++++++-

Quoted 4 lines

Review

Quoted 4 lines

 drivers/pci/host/pcie-rcar.h |   5 +
 2 files changed, 236 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/pci/host/pcie-rcar.c

Quoted 37 lines

Quoted 3 lines

index 16670e5..cbbcd77 100644

drivers/pci/host/pcie-rcar.c +0 -0

[...]

Quoted 33 lines

drivers/pci/host/pcie-rcar.c +33 -0

+
+static irqreturn_t rcar_pcie_msi_irq(int irq, void *data)
+{
+   struct rcar_pcie *pcie = data;
+   struct rcar_msi *msi = &pcie->msi;
+   unsigned long reg;
+
+   reg = pci_read_reg(pcie, PCIEMSIFR);
+
+   while (reg) {
+      unsigned int index = find_first_bit(&reg, 32);
+      unsigned int irq;
+
+      /* clear the interrupt */
+      pci_write_reg(pcie, 1 << index, PCIEMSIFR);
+
+      irq = irq_find_mapping(msi->domain, index);
+      if (irq) {
+         if (test_bit(index, msi->used))
+            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");
+      }
+
+      /* see if there's any more pending in this vector */
+      reg = pci_read_reg(pcie, PCIEMSIFR);
+   }

Quoted 8 lines

drivers/pci/host/pcie-rcar.c static void __init rcar_pcie_hw_init(struct rcar_pcie *pcie) · line 598

Quoted 4 lines

drivers/pci/host/pcie-rcar.c +4 -0

+
+   return IRQ_HANDLED;
+}
+

From your DT binding it seems you have only one interrupt from the PCIe core, shared between the MSI irqs and the PCI legacy interrupts. This means this handler may get called without an MSI irq pending, so this function really should have a path where it's returning IRQ_NONE.

Ah, yes you are right... though actually there are two interrupts, one has some MSI and INTx, the other has the rest of the MSIs.

Subject: Re: [PATCH v4 2/9] PCI: host: rcar: Add MSI support

Am Freitag, den 21.03.2014, 14:15 +0000 schrieb
Phil.Edworthy at renesas.com:

Quoted 59 lines

Hi Lucas,

Thanks for the review.

Quoted 6 lines · Lucas

Subject: Re: [PATCH v4 2/9] PCI: host: rcar: Add MSI support

Am Freitag, den 21.03.2014, 10:32 +0000 schrieb Phil Edworthy:

Quoted 3 lines

Signed-off-by Phil Edworthy <[email protected]>

---
 drivers/pci/host/pcie-rcar.c | 232 

++++++++++++++++++++++++++++++++++++++++++-

Quoted 4 lines

Review

Quoted 4 lines

 drivers/pci/host/pcie-rcar.h |   5 +
 2 files changed, 236 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/pci/host/pcie-rcar.c

Quoted 42 lines

Quoted 3 lines

index 16670e5..cbbcd77 100644

drivers/pci/host/pcie-rcar.c +0 -0

[...]

Quoted 27 lines

drivers/pci/host/pcie-rcar.c +27 -0

+
+static irqreturn_t rcar_pcie_msi_irq(int irq, void *data)
+{
+   struct rcar_pcie *pcie = data;
+   struct rcar_msi *msi = &pcie->msi;
+   unsigned long reg;
+
+   reg = pci_read_reg(pcie, PCIEMSIFR);
+
+   while (reg) {
+      unsigned int index = find_first_bit(&reg, 32);
+      unsigned int irq;
+
+      /* clear the interrupt */
+      pci_write_reg(pcie, 1 << index, PCIEMSIFR);
+
+      irq = irq_find_mapping(msi->domain, index);
+      if (irq) {
+         if (test_bit(index, msi->used))
+            generic_handle_irq(irq);
+         else
+            dev_info(pcie->dev, "unhandled MSI\n");
+      } else {
+         /*
+          * that's weird who triggered this?
+          * just clear it
+          */

drivers/pci/host/pcie-rcar.c static void __init rcar_pcie_hw_init(struct rcar_pcie *pcie) · line 598

Quoted 10 lines

drivers/pci/host/pcie-rcar.c +10 -0

+         dev_info(pcie->dev, "unexpected MSI\n");
+      }
+
+      /* see if there's any more pending in this vector */
+      reg = pci_read_reg(pcie, PCIEMSIFR);
+   }
+
+   return IRQ_HANDLED;
+}
+

From your DT binding it seems you have only one interrupt from the

Quoted 2 lines

Quoted 2 lines

core, shared between the MSI irqs and the PCI legacy interrupts. This means this handler may get called without an MSI irq pending,

Quoted 1 line

Quoted 1 line

this function really should have a path where it's returning

Quoted 1 line

Ah, yes you are right... though actually there are two interrupts, one

Quoted 2 lines

some MSI and INTx, the other has the rest of the MSIs.

This isn't reflected in the DT binding. If you already know that there may be more than a single interrupt for MSI, please push this into the binding by using named interrupts. Even if your driver doesn't support it yet, additional functionality can always be added later, but the DT should be a stable ABI describing your hardware.

So if you already know your hardware has more than one interrupt, please put it in the binding, to avoid churn later on.

Regards,
Lucas

Signature · 2 lines

Pengutronix e.K.                           | Lucas Stach |
Industrial Linux Solutions                 | http://www.pengutronix.de/ 

Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076

Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555

This patch adds the bindings for the rcar PCIE driver. The driver resides under drivers/pci/host/pcie-rcar.c

Signed-off-by Phil Edworthy <[email protected]>

---
 Documentation/devicetree/bindings/pci/rcar-pci.txt | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci.txt

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +7 -0

new file mode 100644
index 0000000..0e219b0
@@ -0,0 +1,40 @@
+* Renesas RCar PCIe interface
+
+Required properties:
+- compatible: should contain one of the following
+	"renesas,r8a7779-pcie", "renesas,r8a7790-pcie", "renesas,r8a7791-pcie"
+- reg: base addresses and lengths of the pcie controller.
+- #address-cells: set to <3>

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +23 -0

+
+Example:
+
+SoC specific DT Entry:
+
+	pcie: pcie at 0x01000000 {
+		compatible = "renesas,r8a7790-pcie";
+		reg = <0 0xfe000000 0 0x80000>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x01000000 0 0x00000000 0 0xfe100000 0 0x00100000
+			  0x02000000 0 0xfe200000 0 0xfe200000 0 0x00200000
+			  0x02000000 0 0x30000000 0 0x30000000 0 0x08000000
+			  0x42000000 0 0x38000000 0 0x38000000 0 0x08000000>;
+		interrupts = <0 116 4>;
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 0>;
+		interrupt-map = <0 0 0 0 &gic 0 116 4>;
+		clocks = <&mstp3_clks R8A7790_CLK_PCIE>;
+		clock-names = "pcie";
+		status = "disabled";
+	};

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

Am Freitag, den 21.03.2014, 10:32 +0000 schrieb Phil Edworthy:

Quoted 3 lines

This patch adds the bindings for the rcar PCIE driver. The driver resides under drivers/pci/host/pcie-rcar.c

Quoted 1 line

Signed-off-by Phil Edworthy <[email protected]>

I have one question below. If you can answer this with yes after thinking again about it, this is:

Reviewed-by Lucas Stach <[email protected]>

Quoted 2 lines

---
 Documentation/devicetree/bindings/pci/rcar-pci.txt | 40 

Review

Quoted 4 lines

 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci.txt

(quoted diff) renamed from Documentation/devicetree/bindings/pci/rcar-pci.txt +0 -0

Documentation/devicetree/bindings/pci/rcar-pci.txt

Documentation/devicetree/bindings/pci/rcar-pci.txt line 5

Quoted 10 lines

new file mode 100644
index 0000000..0e219b0

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +5 -0

@@ -0,0 +1,40 @@
+* Renesas RCar PCIe interface
+
+Required properties:
+- compatible: should contain one of the following
+   "renesas,r8a7779-pcie", "renesas,r8a7790-pcie", 

Documentation/devicetree/bindings/pci/rcar-pci.txt line 17

Quoted 12 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +12 -0

+- reg: base addresses and lengths of the pcie controller.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions
+- interrupts: interrupt values for MSI interrupt
+- #interrupt-cells: set to <1>
+- interrupt-map-mask and interrupt-map: standard PCI properties
+   to define the mapping of the PCIe interface to interrupt
+   numbers.
+- clocks: from common clock binding: handle to pci clock.
+- clock-names: from common clock binding: should be "pcie"

You really only have one PCIe clock? So the controller is generating the PCIe bus clock from it's register clock? No need to enable any other clock for PCIe to work?

Quoted 15 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +15 -0

+
+Example:
+
+SoC specific DT Entry:
+
+   pcie: pcie at 0x01000000 {
+      compatible = "renesas,r8a7790-pcie";
+      reg = <0 0xfe000000 0 0x80000>;
+      #address-cells = <3>;
+      #size-cells = <2>;
+      device_type = "pci";
+      ranges = <0x01000000 0 0x00000000 0 0xfe100000 0 0x00100000
+           0x02000000 0 0xfe200000 0 0xfe200000 0 0x00200000
+           0x02000000 0 0x30000000 0 0x30000000 0 0x08000000
+           0x42000000 0 0x38000000 0 0x38000000 0 0x08000000>;

Quoted 8 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +8 -0

+      interrupts = <0 116 4>;
+      #interrupt-cells = <1>;
+      interrupt-map-mask = <0 0 0 0>;
+      interrupt-map = <0 0 0 0 &gic 0 116 4>;
+      clocks = <&mstp3_clks R8A7790_CLK_PCIE>;
+      clock-names = "pcie";
+      status = "disabled";
+   };

Signature · 2 lines

Pengutronix e.K.                           | Lucas Stach |
Industrial Linux Solutions                 | http://www.pengutronix.de/ 

Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076

Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555

Hi Lucas,

Quoted 1 line · Lucas

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

bindings

Quoted 5 lines

Am Freitag, den 21.03.2014, 10:32 +0000 schrieb Phil Edworthy:

Quoted 3 lines

This patch adds the bindings for the rcar PCIE driver. The driver resides under drivers/pci/host/pcie-rcar.c

Quoted 8 lines

Quoted 1 line

Signed-off-by Phil Edworthy <[email protected]>

I have one question below. If you can answer this with yes after thinking again about it, this is:

Reviewed-by Lucas Stach <[email protected]>

Quoted 2 lines

---
 Documentation/devicetree/bindings/pci/rcar-pci.txt | 40 

++++++++++++++++++++++

Quoted 5 lines

Review

Quoted 4 lines

 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci.txt

(quoted diff) renamed from Documentation/devicetree/bindings/pci/rcar-pci.txt +0 -0

Documentation/devicetree/bindings/pci/rcar-pci.txt

Quoted 10 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt line 5

Quoted 10 lines

new file mode 100644
index 0000000..0e219b0

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +5 -0

@@ -0,0 +1,40 @@
+* Renesas RCar PCIe interface
+
+Required properties:
+- compatible: should contain one of the following
+   "renesas,r8a7779-pcie", "renesas,r8a7790-pcie", 

"renesas,r8a7791-pcie"

Quoted 13 lines

Quoted 12 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +12 -0

+- reg: base addresses and lengths of the pcie controller.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions
+- interrupts: interrupt values for MSI interrupt
+- #interrupt-cells: set to <1>
+- interrupt-map-mask and interrupt-map: standard PCI properties
+   to define the mapping of the PCIe interface to interrupt
+   numbers.
+- clocks: from common clock binding: handle to pci clock.
+- clock-names: from common clock binding: should be "pcie"

Quoted 3 lines

You really only have one PCIe clock? So the controller is generating the PCIe bus clock from it's register clock? No need to enable any other clock for PCIe to work?

Yes, just the one clock. The PCIe bus clock is an external clock dedicated to PCIe and SATA, and we have no control over it.

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

Am Freitag, den 21.03.2014, 14:18 +0000 schrieb
Phil.Edworthy at renesas.com:

Quoted 21 lines

Hi Lucas,

Quoted 1 line · Lucas

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

bindings

Quoted 5 lines

Am Freitag, den 21.03.2014, 10:32 +0000 schrieb Phil Edworthy:

Quoted 3 lines

This patch adds the bindings for the rcar PCIE driver. The driver resides under drivers/pci/host/pcie-rcar.c

Quoted 8 lines

Quoted 1 line

Signed-off-by Phil Edworthy <[email protected]>

I have one question below. If you can answer this with yes after thinking again about it, this is:

Reviewed-by Lucas Stach <[email protected]>

Quoted 2 lines

---
 Documentation/devicetree/bindings/pci/rcar-pci.txt | 40 

++++++++++++++++++++++

Quoted 2 lines

Quoted 2 lines

 1 file changed, 40 insertions(+)
 create mode 100644 

Quoted 27 lines

Quoted 13 lines

Review

Quoted 2 lines

(quoted diff) renamed from Documentation/devicetree/bindings/pci/rcar-pci.txt +0 -0

Documentation/devicetree/bindings/pci/rcar-pci.txt

Documentation/devicetree/bindings/pci/rcar-pci.txt line 5

Quoted 10 lines

new file mode 100644
index 0000000..0e219b0

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +5 -0

@@ -0,0 +1,40 @@
+* Renesas RCar PCIe interface
+
+Required properties:
+- compatible: should contain one of the following
+   "renesas,r8a7779-pcie", "renesas,r8a7790-pcie", 

"renesas,r8a7791-pcie"

Quoted 13 lines

Quoted 12 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +12 -0

+- reg: base addresses and lengths of the pcie controller.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions
+- interrupts: interrupt values for MSI interrupt
+- #interrupt-cells: set to <1>
+- interrupt-map-mask and interrupt-map: standard PCI properties
+   to define the mapping of the PCIe interface to interrupt
+   numbers.
+- clocks: from common clock binding: handle to pci clock.
+- clock-names: from common clock binding: should be "pcie"

Quoted 1 line

Quoted 1 line

You really only have one PCIe clock? So the controller is generating

Quoted 3 lines

Quoted 2 lines

PCIe bus clock from it's register clock? No need to enable any other clock for PCIe to work?

Yes, just the one clock. The PCIe bus clock is an external clock

Quoted 2 lines

to PCIe and SATA, and we have no control over it.

Is this some kind of always-on clock? If not you probably want a reference to it from the PCIe driver even if it's shared between SATA and PCIe. After all you don't want to end up unconditionally enabling this clock from the board or SoC level just to have PCIe working, right?

Regards,
Lucas

Signature · 2 lines

Pengutronix e.K.                           | Lucas Stach |
Industrial Linux Solutions                 | http://www.pengutronix.de/ 

Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076

Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

Hi Lucas,

Quoted 1 line · Lucas

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

Quoted 7 lines

Am Freitag, den 21.03.2014, 14:18 +0000 schrieb
Phil.Edworthy at renesas.com:

Quoted 4 lines

Hi Lucas,

Quoted 1 line · Lucas

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device

Quoted 4 lines

Quoted 4 lines

bindings

Quoted 3 lines

Am Freitag, den 21.03.2014, 10:32 +0000 schrieb Phil Edworthy:

Quoted 1 line

This patch adds the bindings for the rcar PCIE driver. The

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

resides under drivers/pci/host/pcie-rcar.c

Signed-off-by Phil Edworthy <[email protected]>

I have one question below. If you can answer this with yes after thinking again about it, this is:

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

Reviewed-by Lucas Stach <[email protected]>

Quoted 2 lines

---
 Documentation/devicetree/bindings/pci/rcar-pci.txt | 40 

++++++++++++++++++++++

Quoted 2 lines

Quoted 2 lines

 1 file changed, 40 insertions(+)
 create mode 100644 

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +0 -0

Quoted 25 lines

Quoted 25 lines

Quoted 11 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt

Documentation/devicetree/bindings/pci/rcar-pci.txt line 5

Quoted 10 lines

new file mode 100644
index 0000000..0e219b0

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +5 -0

@@ -0,0 +1,40 @@
+* Renesas RCar PCIe interface
+
+Required properties:
+- compatible: should contain one of the following
+   "renesas,r8a7779-pcie", "renesas,r8a7790-pcie", 

"renesas,r8a7791-pcie"

Quoted 13 lines

Quoted 12 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +12 -0

+- reg: base addresses and lengths of the pcie controller.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions
+- interrupts: interrupt values for MSI interrupt
+- #interrupt-cells: set to <1>
+- interrupt-map-mask and interrupt-map: standard PCI properties
+   to define the mapping of the PCIe interface to interrupt
+   numbers.
+- clocks: from common clock binding: handle to pci clock.
+- clock-names: from common clock binding: should be "pcie"

Quoted 1 line

Quoted 1 line

Quoted 1 line

You really only have one PCIe clock? So the controller is

Quoted 1 line

Quoted 1 line

Quoted 1 line

PCIe bus clock from it's register clock? No need to enable any

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

clock for PCIe to work?

Yes, just the one clock. The PCIe bus clock is an external clock

Quoted 6 lines

Quoted 2 lines

to PCIe and SATA, and we have no control over it.

Is this some kind of always-on clock? If not you probably want a reference to it from the PCIe driver even if it's shared between SATA and PCIe. After all you don't want to end up unconditionally enabling this clock from the board or SoC level just to have PCIe working,

Ok, I see, I'll need a reference to a board clock, even if it's always

these boards.

Hi Lucas,

Quoted 1 line · Phil

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

bindings

Quoted 5 lines

Hi Lucas,

Quoted 1 line · Lucas

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

bindings

Quoted 7 lines

Quoted 7 lines

Am Freitag, den 21.03.2014, 14:18 +0000 schrieb
Phil.Edworthy at renesas.com:

Quoted 4 lines

Hi Lucas,

Quoted 1 line · Lucas

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device

tree

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

bindings

Quoted 3 lines

Am Freitag, den 21.03.2014, 10:32 +0000 schrieb Phil Edworthy:

Quoted 1 line

This patch adds the bindings for the rcar PCIE driver. The

driver

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

resides under drivers/pci/host/pcie-rcar.c

Signed-off-by Phil Edworthy <[email protected]>

I have one question below. If you can answer this with yes after thinking again about it, this is:

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

Reviewed-by Lucas Stach <[email protected]>

Quoted 2 lines

---
  Documentation/devicetree/bindings/pci/rcar-pci.txt | 40

++++++++++++++++++++++

Quoted 2 lines

Quoted 2 lines

  1 file changed, 40 insertions(+)

create mode 100644

Documentation/devicetree/bindings/pci/rcar-pci.txt

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +0 -0

b/

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

Quoted 11 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt

Documentation/devicetree/bindings/pci/rcar-pci.txt line 5

Quoted 10 lines

new file mode 100644
index 0000000..0e219b0

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +5 -0

@@ -0,0 +1,40 @@
+* Renesas RCar PCIe interface
+
+Required properties:
+- compatible: should contain one of the following
+   "renesas,r8a7779-pcie", "renesas,r8a7790-pcie",

"renesas,r8a7791-pcie"

Quoted 13 lines

Quoted 12 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +12 -0

+- reg: base addresses and lengths of the pcie controller.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions
+- interrupts: interrupt values for MSI interrupt
+- #interrupt-cells: set to <1>
+- interrupt-map-mask and interrupt-map: standard PCI properties
+   to define the mapping of the PCIe interface to interrupt
+   numbers.
+- clocks: from common clock binding: handle to pci clock.
+- clock-names: from common clock binding: should be "pcie"

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

You really only have one PCIe clock? So the controller is

generating the

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

PCIe bus clock from it's register clock? No need to enable any

other

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

clock for PCIe to work?

Yes, just the one clock. The PCIe bus clock is an external clock

dedicated

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

to PCIe and SATA, and we have no control over it.

Is this some kind of always-on clock? If not you probably want a reference to it from the PCIe driver even if it's shared between SATA and PCIe. After all you don't want to end up unconditionally enabling this clock from the board or SoC level just to have PCIe working,

right?

Quoted 1 line

Ok, I see, I'll need a reference to a board clock, even if it's always

on for

Quoted 1 line

these boards.

One small question...

The patches added the PCIe host controller to two devices, r8a7790 and r8a7791. The r8a7790-based Lager board doesn't have the necessary hardware to use PCIe, so should I add a dummy pcie_bus clock to the Lager board dts, even though PCIe is not used?

I'm just wondering what the dts file looks like for a board that doesn't support many interfaces. Does it become littered with dummy clocks and regulators?

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

Quoted 69 lines

Hi Lucas,

Quoted 1 line · Phil

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

bindings

Quoted 5 lines

Hi Lucas,

Quoted 1 line · Lucas

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

bindings

Quoted 7 lines

Quoted 7 lines

Am Freitag, den 21.03.2014, 14:18 +0000 schrieb
Phil.Edworthy at renesas.com:

Quoted 4 lines

Hi Lucas,

Quoted 1 line · Lucas

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device

tree

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

bindings

Quoted 3 lines

Am Freitag, den 21.03.2014, 10:32 +0000 schrieb Phil Edworthy:

Quoted 1 line

This patch adds the bindings for the rcar PCIE driver. The

driver

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

resides under drivers/pci/host/pcie-rcar.c

Signed-off-by Phil Edworthy <[email protected]>

I have one question below. If you can answer this with yes after thinking again about it, this is:

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

Reviewed-by Lucas Stach <[email protected]>

Quoted 2 lines

---
  Documentation/devicetree/bindings/pci/rcar-pci.txt | 40

++++++++++++++++++++++

Quoted 2 lines

Quoted 2 lines

  1 file changed, 40 insertions(+)

create mode 100644

Documentation/devicetree/bindings/pci/rcar-pci.txt

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +0 -0

b/

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

Quoted 11 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt

Documentation/devicetree/bindings/pci/rcar-pci.txt line 5

Quoted 10 lines

new file mode 100644
index 0000000..0e219b0

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +5 -0

@@ -0,0 +1,40 @@
+* Renesas RCar PCIe interface
+
+Required properties:
+- compatible: should contain one of the following
+   "renesas,r8a7779-pcie", "renesas,r8a7790-pcie",

"renesas,r8a7791-pcie"

Quoted 13 lines

Quoted 12 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +12 -0

+- reg: base addresses and lengths of the pcie controller.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions
+- interrupts: interrupt values for MSI interrupt
+- #interrupt-cells: set to <1>
+- interrupt-map-mask and interrupt-map: standard PCI properties
+   to define the mapping of the PCIe interface to interrupt
+   numbers.
+- clocks: from common clock binding: handle to pci clock.
+- clock-names: from common clock binding: should be "pcie"

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

You really only have one PCIe clock? So the controller is

generating the

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

PCIe bus clock from it's register clock? No need to enable any

Quoted 8 lines

other

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

clock for PCIe to work?

Yes, just the one clock. The PCIe bus clock is an external clock

dedicated

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

to PCIe and SATA, and we have no control over it.

Is this some kind of always-on clock? If not you probably want a reference to it from the PCIe driver even if it's shared between

Quoted 1 line

Quoted 1 line

Quoted 1 line

and PCIe. After all you don't want to end up unconditionally

Quoted 3 lines

Quoted 1 line

Quoted 1 line

this clock from the board or SoC level just to have PCIe working,

right?

Quoted 1 line

Ok, I see, I'll need a reference to a board clock, even if it's

Quoted 6 lines

on for

Quoted 1 line

these boards.

One small question...

The patches added the PCIe host controller to two devices, r8a7790 and r8a7791. The r8a7790-based Lager board doesn't have the necessary

Quoted 4 lines

to use PCIe, so should I add a dummy pcie_bus clock to the Lager board dts, even though PCIe is not used?

I'm just wondering what the dts file looks like for a board that

Quoted 2 lines

support many interfaces. Does it become littered with dummy clocks and regulators?

The bridge node should be set to disabled by default so it never gets probed. Any boards that want to use it can over-ride the status to be enabled.

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

Hi Ben,

Quoted 1 line · Ben

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

Quoted 4 lines · Phil.Edworthy at renesas.com

Quoted 2 lines

Hi Lucas,

Quoted 8 lines

Quoted 8 lines

Quoted 1 line · Phil

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

bindings

Quoted 5 lines

Hi Lucas,

Quoted 1 line · Lucas

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device

Quoted 43 lines

Quoted 43 lines

bindings

Quoted 7 lines

Quoted 7 lines

Am Freitag, den 21.03.2014, 14:18 +0000 schrieb
Phil.Edworthy at renesas.com:

Quoted 4 lines

Hi Lucas,

Quoted 1 line · Lucas

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device

tree

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

bindings

Quoted 3 lines

Am Freitag, den 21.03.2014, 10:32 +0000 schrieb Phil Edworthy:

Quoted 1 line

This patch adds the bindings for the rcar PCIE driver. The

driver

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

resides under drivers/pci/host/pcie-rcar.c

Signed-off-by Phil Edworthy <[email protected]>

I have one question below. If you can answer this with yes after thinking again about it, this is:

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

Reviewed-by Lucas Stach <[email protected]>

Quoted 2 lines

---
  Documentation/devicetree/bindings/pci/rcar-pci.txt | 40

++++++++++++++++++++++

Quoted 2 lines

Quoted 2 lines

  1 file changed, 40 insertions(+)

create mode 100644

Documentation/devicetree/bindings/pci/rcar-pci.txt

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +0 -0

b/

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt

Documentation/devicetree/bindings/pci/rcar-pci.txt line 5

Quoted 10 lines

new file mode 100644
index 0000000..0e219b0

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +5 -0

@@ -0,0 +1,40 @@
+* Renesas RCar PCIe interface
+
+Required properties:
+- compatible: should contain one of the following
+   "renesas,r8a7779-pcie", "renesas,r8a7790-pcie",

"renesas,r8a7791-pcie"

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt line 13

Quoted 8 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +8 -0

+- reg: base addresses and lengths of the pcie controller.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions
+- interrupts: interrupt values for MSI interrupt
+- #interrupt-cells: set to <1>
+- interrupt-map-mask and interrupt-map: standard PCI 

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt line 17

Quoted 4 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +4 -0

+   to define the mapping of the PCIe interface to interrupt
+   numbers.
+- clocks: from common clock binding: handle to pci clock.
+- clock-names: from common clock binding: should be "pcie"

You really only have one PCIe clock? So the controller is

generating the

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

PCIe bus clock from it's register clock? No need to enable any

Quoted 8 lines

Quoted 8 lines

other

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

clock for PCIe to work?

Yes, just the one clock. The PCIe bus clock is an external clock

dedicated

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

to PCIe and SATA, and we have no control over it.

Is this some kind of always-on clock? If not you probably want a reference to it from the PCIe driver even if it's shared between

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

and PCIe. After all you don't want to end up unconditionally

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

this clock from the board or SoC level just to have PCIe working,

right?

Quoted 1 line

Ok, I see, I'll need a reference to a board clock, even if it's

Quoted 5 lines

Quoted 5 lines

on for

Quoted 1 line

these boards.

One small question...

The patches added the PCIe host controller to two devices, r8a7790

Quoted 1 line

Quoted 1 line

r8a7791. The r8a7790-based Lager board doesn't have the necessary

Quoted 1 line

Quoted 1 line

to use PCIe, so should I add a dummy pcie_bus clock to the Lager

Quoted 3 lines

Quoted 3 lines

dts, even though PCIe is not used?

I'm just wondering what the dts file looks like for a board that

Quoted 1 line

Quoted 1 line

support many interfaces. Does it become littered with dummy clocks

Quoted 5 lines

Quoted 1 line

regulators?

The bridge node should be set to disabled by default so it never gets probed. Any boards that want to use it can over-ride the status to be enabled.

Ok, thanks.

Hi Ben,

Quoted 1 line · Phil

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

bindings

Quoted 5 lines

Hi Ben,

Quoted 1 line · Ben

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

bindings

Quoted 4 lines

Quoted 4 lines · Phil.Edworthy at renesas.com

Quoted 2 lines

Hi Lucas,

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 1 line · Phil

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device tree

bindings

Quoted 5 lines

Hi Lucas,

Quoted 1 line · Lucas

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device

tree

Quoted 43 lines

Quoted 43 lines

Quoted 43 lines

bindings

Quoted 7 lines

Quoted 7 lines

Am Freitag, den 21.03.2014, 14:18 +0000 schrieb
Phil.Edworthy at renesas.com:

Quoted 4 lines

Hi Lucas,

Quoted 1 line · Lucas

Subject: Re: [PATCH v4 5/9] dt-bindings: pci: rcar pcie device

tree

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

bindings

Quoted 3 lines

Am Freitag, den 21.03.2014, 10:32 +0000 schrieb Phil Edworthy:

Quoted 1 line

This patch adds the bindings for the rcar PCIE driver. The

driver

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

resides under drivers/pci/host/pcie-rcar.c

Signed-off-by Phil Edworthy <[email protected]>

I have one question below. If you can answer this with yes after thinking again about it, this is:

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

Reviewed-by Lucas Stach <[email protected]>

Quoted 2 lines

---
  Documentation/devicetree/bindings/pci/rcar-pci.txt | 40

++++++++++++++++++++++

Quoted 2 lines

Quoted 2 lines

  1 file changed, 40 insertions(+)

create mode 100644

Documentation/devicetree/bindings/pci/rcar-pci.txt

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +0 -0

b/

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt

Documentation/devicetree/bindings/pci/rcar-pci.txt line 5

Quoted 10 lines

new file mode 100644
index 0000000..0e219b0

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +5 -0

@@ -0,0 +1,40 @@
+* Renesas RCar PCIe interface
+
+Required properties:
+- compatible: should contain one of the following
+   "renesas,r8a7779-pcie", "renesas,r8a7790-pcie",

"renesas,r8a7791-pcie"

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt line 13

Quoted 8 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +8 -0

+- reg: base addresses and lengths of the pcie controller.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions
+- interrupts: interrupt values for MSI interrupt
+- #interrupt-cells: set to <1>
+- interrupt-map-mask and interrupt-map: standard PCI 

properties

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt line 17

Quoted 4 lines

Documentation/devicetree/bindings/pci/rcar-pci.txt new file +4 -0

+   to define the mapping of the PCIe interface to interrupt
+   numbers.
+- clocks: from common clock binding: handle to pci clock.
+- clock-names: from common clock binding: should be "pcie"

You really only have one PCIe clock? So the controller is

generating the

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

PCIe bus clock from it's register clock? No need to enable any

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

other

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

clock for PCIe to work?

Yes, just the one clock. The PCIe bus clock is an external clock

dedicated

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

to PCIe and SATA, and we have no control over it.

Is this some kind of always-on clock? If not you probably want a reference to it from the PCIe driver even if it's shared between

SATA

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

and PCIe. After all you don't want to end up unconditionally

enabling

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

Quoted 1 line

this clock from the board or SoC level just to have PCIe working,

right?

Quoted 1 line

Ok, I see, I'll need a reference to a board clock, even if it's

always

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

on for

Quoted 1 line

these boards.

One small question...

The patches added the PCIe host controller to two devices, r8a7790

and

Quoted 1 line

Quoted 1 line

Quoted 1 line

r8a7791. The r8a7790-based Lager board doesn't have the necessary

hardware

Quoted 1 line

Quoted 1 line

Quoted 1 line

to use PCIe, so should I add a dummy pcie_bus clock to the Lager

board

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

dts, even though PCIe is not used?

I'm just wondering what the dts file looks like for a board that

doesn't

Quoted 1 line

Quoted 1 line

Quoted 1 line

support many interfaces. Does it become littered with dummy clocks

and

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

regulators?

Quoted 4 lines

Quoted 3 lines

The bridge node should be set to disabled by default so it never gets probed. Any boards that want to use it can over-ride the status to be enabled.

Ok, thanks.

Hmm, yes it doesn't get probed, but you still get a build problem as the DT entry references an clock that doesn't exist. Maybe I'm missing something...

This patch adds the device tree nodes for R8A7790

Signed-off-by Phil Edworthy <[email protected]>

---
  arch/arm/boot/dts/r8a7790.dtsi | 19 +++++++++++++++++++
  1 file changed, 19 insertions(+)

arch/arm/boot/dts/r8a7790.dtsi +0 -0

index df9ec61..e7d498a 100644

Subject: Re: [PATCH v4 6/9] ARM: shmobile: Add PCIe device tree nodes

Hello.

On 03/21/2014 01:32 PM, Phil Edworthy wrote:

Review

Quoted 8 lines

This patch adds the device tree nodes for R8A7790

Signed-off-by Phil Edworthy <[email protected]>

---
  arch/arm/boot/dts/r8a7790.dtsi | 19 +++++++++++++++++++
  1 file changed, 19 insertions(+)

(quoted diff) +0 -0

Quoted 4 lines

index df9ec61..e7d498a 100644

arch/arm/boot/dts/r8a7790.dtsi +0 -0

@@ -821,4 +821,23 @@

arch/arm/boot/dts/r8a7790.dtsi line 826

Quoted 6 lines

arch/arm/boot/dts/r8a7790.dtsi +3 -0

        #size-cells = <0>;
        status = "disabled";
     };
+
+   pcie: pcie at fe000000 {
+      compatible = "renesas,r8a7790-pcie";

The convention adopted on this list is to put the "pcie" first, and

name the last.

arch/arm/boot/dts/r8a7790.dtsi line 830

Quoted 4 lines

arch/arm/boot/dts/r8a7790.dtsi +4 -0

+      reg = <0 0xfe000000 0 0x80000>;
+      #address-cells = <3>;
+      #size-cells = <2>;
+      device_type = "pci";

This legacy property is only meaningful for the true OF firmware and

have been phased out (it's only actively used for the memory nodes).

WBR, Sergei

Quoted 7 lines

arch/arm/boot/dts/r8a7790.dtsi line 830

Quoted 5 lines

arch/arm/boot/dts/r8a7790.dtsi +4 -0

+      reg = <0 0xfe000000 0 0x80000>;
+      #address-cells = >;
+      #size-cells = <2>;
+      device_type = "pci";

This legacy property is only meaningful for the true OF firmware and

thus

Quoted 1 line

have been phased out (it's only actively used for the memory nodes).

Ok, thanks for pointing this out.

Quoted 9 lines

Quoted 7 lines

arch/arm/boot/dts/r8a7790.dtsi line 830

Quoted 4 lines

arch/arm/boot/dts/r8a7790.dtsi +4 -0

+      reg = <0 0xfe000000 0 0x80000>;
+      #address-cells = >;
+      #size-cells = <2>;
+      device_type = "pci";

This legacy property is only meaningful for the true OF firmware and thus
have been phased out (it's only actively used for the memory nodes).

Ok, thanks for pointing this out.

I think the code still relies on it for PCI though.

Arnd

Subject: Re: [PATCH v4 6/9] ARM: shmobile: Add PCIe device tree nodes

Hello.

Quoted 6 lines · Arnd Bergmann

Quoted 6 lines

Quoted 6 lines

arch/arm/boot/dts/r8a7790.dtsi line 830

Quoted 4 lines

arch/arm/boot/dts/r8a7790.dtsi +4 -0

+      reg = <0 0xfe000000 0 0x80000>;
+      #address-cells = >;
+      #size-cells = <2>;
+      device_type = "pci";

This legacy property is only meaningful for the true OF

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

have been phased out (it's only actively used for the memory nodes).

Ok, thanks for pointing this out.

I think the code still relies on it for PCI though.

OK, I have found where it does that but the recommended value seems

"pciex" for PCI-Express case.

Quoted 2 lines

OK, I have found where it does that but the recommended value
seems to be "pciex" for PCI-Express case.

Ok, but why then do all the other PCIe hosts use "pci"?

Quoted 6 lines · On Friday 21 March 2014 14:53:48 Phil.Edworthy at renesas.com wrote:

Quoted 6 lines

Quoted 5 lines

arch/arm/boot/dts/r8a7790.dtsi +4 -0

+      reg = <0 0xfe000000 0 0x80000>;
+      #address-cells = >;
+      #size-cells = <2>;
+      device_type = "pci";

Quoted 5 lines

Quoted 1 line

This legacy property is only meaningful for the true OF firmware and

thus

Quoted 1 line

have been phased out (it's only actively used for the memory nodes).

Ok, thanks for pointing this out.

I think the code still relies on it for PCI though.

arch/arm/boot/dts/r8a7790.dtsi line 826

Quoted 3 lines

arch/arm/boot/dts/r8a7790.dtsi +3 -0

+
+       pcie: pcie at fe000000 {
+               compatible = "renesas,r8a7790-pcie";

The convention adopted on this list is to put the "pcie" first, and SoC
name the last.

This patch adds the device tree nodes for R8A7790

Signed-off-by Phil Edworthy <[email protected]>

---
  arch/arm/boot/dts/r8a7790.dtsi | 19 +++++++++++++++++++
  1 file changed, 19 insertions(+)

Subject: Re: [PATCH v4 6/9] ARM: shmobile: Add PCIe device tree nodes

Hello.

On 03/21/2014 01:32 PM, Phil Edworthy wrote:

Quoted 6 lines

This patch adds the device tree nodes for R8A7790

Signed-off-by Phil Edworthy <[email protected]>

---
  arch/arm/boot/dts/r8a7790.dtsi | 19 +++++++++++++++++++
  1 file changed, 19 insertions(+)

Why are you not enabling PCIe for Lager, only on Koelsch?

WBR, Sergei