Skip to contentSkip to conversation

R-Car Gen2 PCIe host driver

PATCH New Phil Edworthy 5 participants 15 messages May 12, 2014 – Jun 27, 2014

Subsystems (6)
lore
Share & tools
lore
#0 lore raw

v8 cover R-Car Gen2 PCIe host driver

This is version 8 of a PCIe Host driver for the R-Car Gen2 devices, i.e. R-Car H2 (r8a7790) and R-Car M2 (r8a7791).

v8:
- Platform changes removed from this patch set
- Moved header file contents into c file
- Formatting cleaned up
- Remove bus/dev/func range checks for config access
- Add comment about config access serialization
- Made rcar_pcie_setup_window() return void as no errors possible
- Remove unused register definitions
- Removed __init markers to fix section mismatches
- Add explicit bus number range
- Get the root bus nr from config writes instead of sys->busnr
- Use PCI domains
- Removed unused variable in rcar_msi_free()
- Split interrupt bindings into separate cells

v7:
- Change binding description of clocks to 'clock specifiers'

v6:
- Correct DT bindings description for reg and clocks
- Split device and board DT changes
- Add shmobile to subject for shmobile DT patches
- Don't check MSI irq number is valid, as upper level checks this
- Change "Unexpected MSI" msg to debug level
- Reword "Unexpected MSI" comment so that it's one line
- Remove patch that adds HAVE_ARM_ARCH_TIMER to koelsch defconfig as not needed

v5:
- Use module_platform_driver instead of subsys_initcall
- Use the of_device_id data field for HW init function
- Init hw_pci struct in declaration
- Renesas SoC compatible string has peripheral before device name
- Add PCIe bus clock reference
- Use dma-ranges property to specify inbound memory regions
- Support multiple IO windows and correct resources
- Return IRQ_NONE from MSI isr when there is no pending MSI
- Add additional interrupt bindings

v4:
- Use runtime PM properly

Phil Edworthy (3):
PCI: host: rcar: Add Renesas R-Car PCIe driver
PCI: host: rcar: Add MSI support
dt-bindings: pci: rcar pcie device tree bindings

 Documentation/devicetree/bindings/pci/rcar-pci.txt |   47 +
 drivers/pci/host/Kconfig                           |    6 +
 drivers/pci/host/Makefile                          |    1 +
 drivers/pci/host/pcie-rcar.c                       | 1008 ++++++++++++++++++++
 4 files changed, 1062 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci.txt
 create mode 100644 drivers/pci/host/pcie-rcar.c
Signature · 1 line
1.9.1
#1 lore raw

PATCH v8 1/3 PCI: host: rcar: Add Renesas R-Car PCIe driver

This PCIe Host driver currently does not support MSI, so cards fall back to INTx interrupts.

v8:
- Moved header file contents into c file
- Formatting cleaned up
- Remove bus/dev/func range checks for config access
- Add comment about config access serialization
- Made rcar_pcie_setup_window() return void as no errors possible
- Remove unused register definitions
- Removed __init markers to fix section mismatches
- Add explicit bus number range
- Get the root bus nr from config writes instead of sys->busnr
- Use PCI domains

v7, v6:
- No changes

v5:
- Use module_platform_driver instead of subsys_initcall
- Use the of_device_id data field for HW init function
- Init hw_pci struct in declaration
- Renesas SoC compatible string has peripheral before device name
- Add PCIe bus clock reference
- Use dma-ranges property to specify inbound memory regions
- Support multiple IO windows and correct resources

v4:
- Use runtime PM properly

v3:
- Add DT support
- Use 'of_irq_parse_and_map_pci' for '.map_irq'
- Use pm ops to enable clocks
- Fix checkpatch errors
- Use subsys_initcall to overcome issues with port bus driver
- Adjust Kconfig to match other R-Car drivers

v2:
- Use msleep instead of udelay when waiting for the link
- Use pm_runtime
- Removed unused definition
- Also replaced call to devm_request_and_ioremap with devm_ioremap_resource and fixed a bug with this when reporting errors.

---
 drivers/pci/host/Kconfig     |   6 +
 drivers/pci/host/Makefile    |   1 +
 drivers/pci/host/pcie-rcar.c | 768 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 775 insertions(+)
 create mode 100644 drivers/pci/host/pcie-rcar.c
drivers/pci/host/Kconfig +6 -0
index a6f67ec..24d290d 100644
@@ -33,4 +33,10 @@ config PCI_RCAR_GEN2
 	  There are 3 internal PCI controllers available with a single
 	  built-in EHCI/OHCI host controller present on each one.
 
+config PCI_RCAR_GEN2_PCIE
+	bool "Renesas R-Car PCIe controller"
+	depends on ARCH_SHMOBILE || (ARM && COMPILE_TEST)
+	help
+	  Say Y here if you want PCIe controller support on R-Car Gen2 SoCs.
+
 endmenu
drivers/pci/host/Makefile +1 -0
index 13fb333..19946f9 100644
@@ -4,3 +4,4 @@ obj-$(CONFIG_PCI_IMX6) += pci-imx6.o
 obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
 obj-$(CONFIG_PCI_TEGRA) += pci-tegra.o
 obj-$(CONFIG_PCI_RCAR_GEN2) += pci-rcar-gen2.o
+obj-$(CONFIG_PCI_RCAR_GEN2_PCIE) += pcie-rcar.o
drivers/pci/host/pcie-rcar.c new file +768 -0

768 lines not on this page · raw message

Signature · 1 line
1.9.1
#2 lore raw

PATCH v8 1/3 PCI: host: rcar: Add Renesas R-Car PCIe driver

Hello.

On 05/12/2014 02:57 PM, Phil Edworthy wrote:

I'm investigating an imprecise external abort occurring once userland is
started when I have NetMos PCIe serial card inserted and the '8250_pci' driver enabled and I have found some issues in this driver, while at it...

Quoted 4 lines

This PCIe Host driver currently does not support MSI, so cards fall back to INTx interrupts.

[...]

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +0 -0
new file mode 100644
index 0000000..3c524b9
@@ -0,0 +1,768 @@

[...]

Quoted 1 line

+#define PCI_MAX_RESOURCES 4

As a side note, this risks collision with <linux/pci*.h>...

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

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+static void pci_write_reg(struct rcar_pcie *pcie, unsigned long val,
+			  unsigned long reg)
+{
+	writel(val, pcie->base + reg);
+}
+
+static unsigned long pci_read_reg(struct rcar_pcie *pcie, unsigned long reg)
+{
+	return readl(pcie->base + reg);
+}

As a side note, these functions are hardly needed, and risk collision too...

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

Quoted 4 lines
drivers/pci/host/pcie-rcar.c new file +4 -0
+
+enum {
+	PCI_ACCESS_READ,
+	PCI_ACCESS_WRITE,

These risk collision too...

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

Quoted 2 lines
drivers/pci/host/pcie-rcar.c new file +2 -0
+static void rcar_pcie_setup_window(int win, struct resource *res,
+				   struct rcar_pcie *pcie)

As a side note, 'res' parameter is hardly needed here, as the function
always gets
called with the resources contained within 'struct rcar_pcie'...

Quoted 7 lines
drivers/pci/host/pcie-rcar.c new file +7 -0
+{
+	/* Setup PCIe address space mappings for each resource */
+	resource_size_t size;
+	u32 mask;
+
+	pci_write_reg(pcie, 0x00000000, PCIEPTCTLR(win));
+

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

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+	/*
+	 * 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));

My investigation showed and printk() here confirmed that instead of a PCI
bus address here we have CPU address written to these registers:

rcar_pcie_setup_window: window 0, resource [io 0xfe100000-0xfe1fffff]
rcar_pcie_setup_window: window 1, resource [mem 0xfe200000-0xfe3fffff]
rcar_pcie_setup_window: window 2, resource [mem 0x30000000-0x37ffffff]
rcar_pcie_setup_window: window 3, resource [mem 0x38000000-0x3fffffff pref] rcar-pcie fe000000.pcie: PCI host bridge to bus 0000:00

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

Quoted 5 lines
drivers/pci/host/pcie-rcar.c new file +5 -0
+
+	/* First resource is for IO */
+	mask = PAR_ENABLE;
+	if (res->flags & IORESOURCE_IO)
+		mask |= IO_SPACE;

For the memory space this works OK as you're identity-mapping the memory
ranges in your device trees. However, for the I/O space this means that it won't work as the BARs in the PCIe devices get programmed with the PCI bus addresses but the PCIe window translation register is programmed with a CPU address which don't at all match (given your device trees) and hence one can't access the card's I/O mapped registers at all...

Quoted 13 lines
drivers/pci/host/pcie-rcar.c new file +13 -0
+
+	pci_write_reg(pcie, mask, PCIEPTCTLR(win));
+}
+
+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;
+
+	pcie->root_bus_nr = -1;
+
+	/* Setup PCI resources */

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

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+	for (i = 0; i < PCI_MAX_RESOURCES; i++) {
+
+		res = &pcie->res[i];
+		if (!res->flags)
+			continue;
+
+		rcar_pcie_setup_window(i, res, pcie);
+
+		if (res->flags & IORESOURCE_IO)
+			pci_ioremap_io(nr * SZ_64K, res->start);

I'm not sure why are you not calling pci_add_resource() for I/O space...
Also, this sets up only 64 KiB of I/O ports while your device tree describes I/O space 1 MiB is size.

Quoted 7 lines
drivers/pci/host/pcie-rcar.c new file +7 -0
+		else
+			pci_add_resource(&sys->resources, res);
+	}
+	pci_add_resource(&sys->resources, &pcie->busn);
+
+	return 1;
+}

[...]

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +6 -0
+static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
+{
+	int err;
+
+	/* Begin initialization */
+	pci_write_reg(pcie, 0, PCIETCTLR);

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

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+
+	/* Set mode */
+	pci_write_reg(pcie, 1, PCIEMSR);
+
+	/*
+	 * Initial header for port config space is type 1, set the device
+	 * class to match. Hardware takes care of propagating the IDSETR
+	 * settings, so there is no need to bother with a quirk.
+	 */
+	pci_write_reg(pcie, PCI_CLASS_BRIDGE_PCI << 16, IDSETR1);

Hm, shouldn't this be a host bridge? I've noticed that the bridge's I/O
and memory base/limit registers are left uninitialized even though the BARs of the PICe devices behind this bridge are assigned.

Quoted 13 lines
drivers/pci/host/pcie-rcar.c new file +13 -0
+
+	/*
+	 * Setup Secondary Bus Number & Subordinate Bus Number, even though
+	 * they aren't used, to avoid bridge being detected as broken.
+	 */
+	rcar_rmw32(pcie, RCONF(PCI_SECONDARY_BUS), 0xff, 1);
+	rcar_rmw32(pcie, RCONF(PCI_SUBORDINATE_BUS), 0xff, 1);
+
+	/* Initialize default capabilities. */
+	rcar_rmw32(pcie, REXPCAP(0), 0, PCI_CAP_ID_EXP);
+	rcar_rmw32(pcie, REXPCAP(PCI_EXP_FLAGS),
+		PCI_EXP_FLAGS_TYPE, PCI_EXP_TYPE_ROOT_PORT << 4);
+	rcar_rmw32(pcie, RCONF(PCI_HEADER_TYPE), 0x7f,

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

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+		PCI_HEADER_TYPE_BRIDGE);
+
+	/* Enable data link layer active state reporting */
+	rcar_rmw32(pcie, REXPCAP(PCI_EXP_LNKCAP), 0, PCI_EXP_LNKCAP_DLLLARC);
+
+	/* Write out the physical slot number = 0 */
+	rcar_rmw32(pcie, REXPCAP(PCI_EXP_SLTCAP), PCI_EXP_SLTCAP_PSN, 0);
+
+	/* Set the completion timer timeout to the maximum 50ms. */
+	rcar_rmw32(pcie, TLCTLR+1, 0x3f, 50);

Missing spaces around '+'...

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

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +6 -0
+
+	/* Terminate list of capabilities (Next Capability Offset=0) */
+	rcar_rmw32(pcie, RVCCAP(0), 0xfff0, 0);
+
+	/* Enable MAC data scrambling. */
+	rcar_rmw32(pcie, MACCTLR, SCRAMBLE_DISABLE, 0);

Doesn't the comment contradict the code here?

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +6 -0
+
+	/* Finish initialization - establish a PCI Express link */
+	pci_write_reg(pcie, CFINIT, PCIETCTLR);
+
+	/* This will timeout if we don't have a link. */
+	err = rcar_pcie_wait_for_dl(pcie);

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

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+	if (err)
+		return err;
+
+	/* Enable INTx interrupts */
+	rcar_rmw32(pcie, PCIEINTXR, 0, 0xF << 8);
+
+	/* Enable slave Bus Mastering */
+	rcar_rmw32(pcie, RCONF(PCI_STATUS), PCI_STATUS_DEVSEL_MASK,
+		PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER |
+		PCI_STATUS_CAP_LIST | PCI_STATUS_DEVSEL_FAST);

Hmm, you're mixing up PCI control/status registers' bits here; they're
two 16-bit registers! So you're writing to 3 reserved LSBs of the PCI status register...

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

Quoted 7 lines
drivers/pci/host/pcie-rcar.c new file +7 -0
+static int rcar_pcie_get_resources(struct platform_device *pdev,
+				   struct rcar_pcie *pcie)
+{
+	struct resource res;
+	int err;
+
+	err = of_address_to_resource(pdev->dev.of_node, 0, &res);

BTW, you could use platfrom_get_resource() and save on your local variable
and the error check -- devm_ioremap_resource() does it.

Quoted 51 lines
drivers/pci/host/pcie-rcar.c new file +51 -0
+	if (err)
+		return err;
+
+	pcie->clk = devm_clk_get(&pdev->dev, "pcie");
+	if (IS_ERR(pcie->clk)) {
+		dev_err(pcie->dev, "cannot get platform clock\n");
+		return PTR_ERR(pcie->clk);
+	}
+	err = clk_prepare_enable(pcie->clk);
+	if (err)
+		goto fail_clk;
+
+	pcie->bus_clk = devm_clk_get(&pdev->dev, "pcie_bus");
+	if (IS_ERR(pcie->bus_clk)) {
+		dev_err(pcie->dev, "cannot get pcie bus clock\n");
+		err = PTR_ERR(pcie->bus_clk);
+		goto fail_clk;
+	}
+	err = clk_prepare_enable(pcie->bus_clk);
+	if (err)
+		goto err_map_reg;
+
+	pcie->base = devm_ioremap_resource(&pdev->dev, &res);
+	if (IS_ERR(pcie->base)) {
+		err = PTR_ERR(pcie->base);
+		goto err_map_reg;
+	}
+
+	return 0;
+
+err_map_reg:
+	clk_disable_unprepare(pcie->bus_clk);
+fail_clk:
+	clk_disable_unprepare(pcie->clk);
+
+	return err;
+}
+
+static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie,
+				    struct of_pci_range *range,
+				    int *index)
+{
+	u64 restype = range->flags;
+	u64 cpu_addr = range->cpu_addr;
+	u64 cpu_end = range->cpu_addr + range->size;
+	u64 pci_addr = range->pci_addr;
+	u32 flags = LAM_64BIT | LAR_ENABLE;
+	u64 mask;
+	u64 size;
+	int idx = *index;
+

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

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+	if (restype & IORESOURCE_PREFETCH)
+		flags |= LAM_PREFETCH;
+
+	/*
+	 * If the size of the range is larger than the alignment of the start
+	 * address, we have to use multiple entries to perform the mapping.
+	 */
+	if (cpu_addr > 0) {
+		unsigned long nr_zeros = __ffs64(cpu_addr);
+		u64 alignment = 1ULL << nr_zeros;

Missing newline...

Quoted 12 lines
drivers/pci/host/pcie-rcar.c new file +12 -0
+		size = min(range->size, alignment);
+	} else {
+		size = range->size;
+	}
+	/* Hardware supports max 4GiB inbound region */
+	size = min(size, 1ULL << 32);
+
+	mask = roundup_pow_of_two(size) - 1;
+	mask &= ~0xf;
+
+	while (cpu_addr < cpu_end) {
+		/*

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

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+		 * Set up 64-bit inbound regions as the range parser doesn't
+		 * distinguish between 32 and 64-bit types.
+		 */
+		pci_write_reg(pcie, lower_32_bits(pci_addr), PCIEPRAR(idx));
+		pci_write_reg(pcie, lower_32_bits(cpu_addr), PCIELAR(idx));
+		pci_write_reg(pcie, lower_32_bits(mask) | flags, PCIELAMR(idx));
+
+		pci_write_reg(pcie, upper_32_bits(pci_addr), PCIEPRAR(idx+1));
+		pci_write_reg(pcie, upper_32_bits(cpu_addr), PCIELAR(idx+1));
+		pci_write_reg(pcie, 0, PCIELAMR(idx+1));

Missing spaces around '+'...

Quoted 22 lines
drivers/pci/host/pcie-rcar.c new file +22 -0
+
+		pci_addr += size;
+		cpu_addr += size;
+		idx += 2;
+
+		if (idx > MAX_NR_INBOUND_MAPS) {
+			dev_err(pcie->dev, "Failed to map inbound regions!\n");
+			return -EINVAL;
+		}
+	}
+	*index = idx;
+
+	return 0;
+}
+
+static int pci_dma_range_parser_init(struct of_pci_range_parser *parser,
+				     struct device_node *node)
+{
+	const int na = 3, ns = 2;
+	int rlen;
+
+	parser->node = node;

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

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+	parser->pna = of_n_addr_cells(node);
+	parser->np = parser->pna + na + ns;
+
+	parser->range = of_get_property(node, "dma-ranges", &rlen);
+	if (!parser->range)
+		return -ENOENT;
+
+	parser->end = parser->range + rlen / sizeof(__be32);
+	return 0;
+}

Erm, AFAIK "dma-ranges" is a standard property, shouldn't its parsing be
placed in some generic place like drivers/of/address.c?

[...]

Quoted 27 lines
drivers/pci/host/pcie-rcar.c new file +27 -0
+static int rcar_pcie_probe(struct platform_device *pdev)
+{
+	struct rcar_pcie *pcie;
+	unsigned int data;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	const struct of_device_id *of_id;
+	int err, win = 0;
+	int (*hw_init_fn)(struct rcar_pcie *);
+
+	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->dev = &pdev->dev;
+	platform_set_drvdata(pdev, pcie);
+
+	/* Get the bus range */
+	if (of_pci_parse_bus_range(pdev->dev.of_node, &pcie->busn)) {
+		dev_err(&pdev->dev, "failed to parse bus-range property\n");
+		return -EINVAL;
+	}
+
+	if (of_pci_range_parser_init(&parser, pdev->dev.of_node)) {
+		dev_err(&pdev->dev, "missing ranges property\n");
+		return -EINVAL;
+	}

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

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+
+	err = rcar_pcie_get_resources(pdev, pcie);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to request resources: %d\n", err);
+		return err;
+	}
+
+	for_each_of_pci_range(&parser, &range) {
+		of_pci_range_to_resource(&range, pdev->dev.of_node,
+						&pcie->res[win++]);

This function call is probably no good here as it fetches into the 'start'
field of a 'struct resource' a CPU address instead of a PCI address...

Quoted 9 lines
drivers/pci/host/pcie-rcar.c new file +9 -0
+
+		if (win > PCI_MAX_RESOURCES)
+			break;
+	}
+
+	 err = rcar_pcie_parse_map_dma_ranges(pcie, pdev->dev.of_node);
+	 if (err)
+		return err;
+

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

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+	of_id = of_match_device(rcar_pcie_of_match, pcie->dev);
+	if (!of_id || !of_id->data)
+		return -EINVAL;
+	hw_init_fn = of_id->data;
+
+	/* Failure to get a link might just be that no cards are inserted */
+	err = hw_init_fn(pcie);
+	if (err) {
+		dev_info(&pdev->dev, "PCIe link down\n");
+		return 0;

Not quite sure why you exit normally here without enabling the hardware.
I think the internal bridge should be visible regardless of whether link is detected or not...

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

Quoted 9 lines
drivers/pci/host/pcie-rcar.c new file +9 -0
+	}
+
+	data = pci_read_reg(pcie, MACSR);
+	dev_info(&pdev->dev, "PCIe x%d: link up\n", (data >> 20) & 0x3f);
+
+	rcar_pcie_enable(pcie);
+
+	return 0;
+}

[...]

WBR, Sergei

#3 lore raw

PATCH v8 1/3 PCI: host: rcar: Add Renesas R-Car PCIe driver

Hi Sergei,

Quoted 6 lines · Sergei

On 05/12/2014 02:57 PM, Phil Edworthy wrote:

I'm investigating an imprecise external abort occurring once userland is
started when I have NetMos PCIe serial card inserted and the '8250_pci' driver
enabled and I have found some issues in this driver, while at it...

Shame they didn't come before the driver was accepted, but still, I welcome the comments. See below.

Quoted 14 lines
Quoted 4 lines

This PCIe Host driver currently does not support MSI, so cards fall back to INTx interrupts.

[...]

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +0 -0
new file mode 100644
index 0000000..3c524b9
@@ -0,0 +1,768 @@

[...]

Quoted 3 lines
Quoted 1 line

+#define PCI_MAX_RESOURCES 4

As a side note, this risks collision with <linux/pci*.h>...

True, I'll fix this.

Quoted 8 lines

drivers/pci/host/pcie-rcar.c

Quoted 7 lines
drivers/pci/host/pcie-rcar.c new file +7 -0
+static void pci_write_reg(struct rcar_pcie *pcie, unsigned long val,
+			  unsigned long reg)
+{
+	writel(val, pcie->base + reg);
+}
+
+static unsigned long pci_read_reg(struct rcar_pcie *pcie, unsigned long

reg)

Quoted 5 lines

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

Quoted 3 lines
drivers/pci/host/pcie-rcar.c new file +3 -0
+{
+	return readl(pcie->base + reg);
+}

As a side note, these functions are hardly needed, and risk collision too...

Ben mentioned this in his review and as I said then, I found them useful during development, so we agreed to leave them. Since they are static, there shouldn't be a collision risk.

Quoted 6 lines

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

Quoted 4 lines
drivers/pci/host/pcie-rcar.c new file +4 -0
+
+enum {
+	PCI_ACCESS_READ,
+	PCI_ACCESS_WRITE,

These risk collision too...

True, I'll fix this.

Quoted 6 lines

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

Quoted 2 lines
drivers/pci/host/pcie-rcar.c new file +2 -0
+static void rcar_pcie_setup_window(int win, struct resource *res,
+				   struct rcar_pcie *pcie)

As a side note, 'res' parameter is hardly needed here, as the function
always gets
called with the resources contained within 'struct rcar_pcie'...

Either I would have to pass an index to the resource in, or as I have done, a pointer to the individual resource. I found it cleaner to pass the pointer.

Quoted 21 lines
Quoted 7 lines
drivers/pci/host/pcie-rcar.c new file +7 -0
+{
+	/* Setup PCIe address space mappings for each resource */
+	resource_size_t size;
+	u32 mask;
+
+	pci_write_reg(pcie, 0x00000000, PCIEPTCTLR(win));
+

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

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+	/*
+	 * 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));

My investigation showed and printk() here confirmed that instead of a PCI
bus address here we have CPU address written to these registers:

Quoted 6 lines

rcar_pcie_setup_window: window 0, resource [io 0xfe100000-0xfe1fffff]
rcar_pcie_setup_window: window 1, resource [mem 0xfe200000-0xfe3fffff]
rcar_pcie_setup_window: window 2, resource [mem 0x30000000-0x37ffffff]
rcar_pcie_setup_window: window 3, resource [mem 0x38000000-0x3fffffff pref]
rcar-pcie fe000000.pcie: PCI host bridge to bus 0000:00

That is a good point, though for all but I/O, as you have found, we use an identity mapping for CPU-PCI addresses.

Quoted 7 lines

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

Quoted 5 lines
drivers/pci/host/pcie-rcar.c new file +5 -0
+
+	/* First resource is for IO */
+	mask = PAR_ENABLE;
+	if (res->flags & IORESOURCE_IO)
+		mask |= IO_SPACE;

For the memory space this works OK as you're identity-mapping the

Quoted 8 lines

memory
ranges in your device trees. However, for the I/O space this means that it won't work as the BARs in the PCIe devices get programmed with the PCI bus addresses but the PCIe window translation register is programmed with a CPU
address which don't at all match (given your device trees) and hence one can't
access the card's I/O mapped registers at all...

Hmm, I couldn't find any cards that supported I/O, so I wasn't able to test this. Clearly this is an issue that needs looking into.

Quoted 19 lines
Quoted 19 lines
drivers/pci/host/pcie-rcar.c new file +19 -0
+
+	pci_write_reg(pcie, mask, PCIEPTCTLR(win));
+}
+
+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;
+
+	pcie->root_bus_nr = -1;
+
+	/* Setup PCI resources */
+	for (i = 0; i < PCI_MAX_RESOURCES; i++) {
+
+		res = &pcie->res[i];
+		if (!res->flags)
+			continue;
+
Quoted 8 lines

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

Quoted 4 lines
drivers/pci/host/pcie-rcar.c new file +4 -0
+		rcar_pcie_setup_window(i, res, pcie);
+
+		if (res->flags & IORESOURCE_IO)
+			pci_ioremap_io(nr * SZ_64K, res->start);

I'm not sure why are you not calling pci_add_resource() for I/O space...
Also, this sets up only 64 KiB of I/O ports while your device tree describes I/O space 1 MiB is size.

This driver should be able to cope with multiple host controllers, so each allocated 64KiB for I/O. 64KiB is all you need for I/O, but the R-Car PCIe hardware has a 1MiB region (the smallest one) that can only be used for one type of PCIe access.

Quoted 21 lines
Quoted 7 lines
drivers/pci/host/pcie-rcar.c new file +7 -0
+		else
+			pci_add_resource(&sys->resources, res);
+	}
+	pci_add_resource(&sys->resources, &pcie->busn);
+
+	return 1;
+}

[...]

Quoted 13 lines
drivers/pci/host/pcie-rcar.c new file +13 -0
+static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
+{
+	int err;
+
+	/* Begin initialization */
+	pci_write_reg(pcie, 0, PCIETCTLR);
+
+	/* Set mode */
+	pci_write_reg(pcie, 1, PCIEMSR);
+
+	/*
+	 * Initial header for port config space is type 1, set the device
+	 * class to match. Hardware takes care of propagating the IDSETR
Quoted 8 lines

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

Quoted 3 lines
drivers/pci/host/pcie-rcar.c new file +3 -0
+	 * settings, so there is no need to bother with a quirk.
+	 */
+	pci_write_reg(pcie, PCI_CLASS_BRIDGE_PCI << 16, IDSETR1);

Hm, shouldn't this be a host bridge? I've noticed that the bridge's I/O
and memory base/limit registers are left uninitialized even though the BARs of
the PICe devices behind this bridge are assigned.

No, I am pretty sure this is correct.

Quoted 23 lines

drivers/pci/host/pcie-rcar.c

Quoted 3 lines
drivers/pci/host/pcie-rcar.c new file +3 -0
+
+	/*
+	 * Setup Secondary Bus Number & Subordinate Bus Number, even

though

Quoted 4 lines
drivers/pci/host/pcie-rcar.c new file +4 -0
+	 * they aren't used, to avoid bridge being detected as broken.
+	 */
+	rcar_rmw32(pcie, RCONF(PCI_SECONDARY_BUS), 0xff, 1);
+	rcar_rmw32(pcie, RCONF(PCI_SUBORDINATE_BUS), 0xff, 1);

drivers/pci/host/pcie-rcar.c

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+
+	/* Initialize default capabilities. */
+	rcar_rmw32(pcie, REXPCAP(0), 0, PCI_CAP_ID_EXP);
+	rcar_rmw32(pcie, REXPCAP(PCI_EXP_FLAGS),
+		PCI_EXP_FLAGS_TYPE, PCI_EXP_TYPE_ROOT_PORT << 4);
+	rcar_rmw32(pcie, RCONF(PCI_HEADER_TYPE), 0x7f,
+		PCI_HEADER_TYPE_BRIDGE);
+
+	/* Enable data link layer active state reporting */
+	rcar_rmw32(pcie, REXPCAP(PCI_EXP_LNKCAP), 0,

PCI_EXP_LNKCAP_DLLLARC);

drivers/pci/host/pcie-rcar.c

Quoted 3 lines
drivers/pci/host/pcie-rcar.c new file +3 -0
+
+	/* Write out the physical slot number = 0 */
+	rcar_rmw32(pcie, REXPCAP(PCI_EXP_SLTCAP),

PCI_EXP_SLTCAP_PSN, 0);

Quoted 5 lines

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

Quoted 3 lines
drivers/pci/host/pcie-rcar.c new file +3 -0
+
+	/* Set the completion timer timeout to the maximum 50ms. */
+	rcar_rmw32(pcie, TLCTLR+1, 0x3f, 50);

Missing spaces around '+'...

Ok

Quoted 8 lines

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

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +6 -0
+
+	/* Terminate list of capabilities (Next Capability Offset=0) */
+	rcar_rmw32(pcie, RVCCAP(0), 0xfff0, 0);
+
+	/* Enable MAC data scrambling. */
+	rcar_rmw32(pcie, MACCTLR, SCRAMBLE_DISABLE, 0);

Doesn't the comment contradict the code here?

No, the rmw32 function is read, modify, write and the SCRAMBLE_DISABLE shown here is the mask, not the value. If the last arg was 1, the call would set the scramble disable bit to 1.
Anyway, scrambling is enabled by default in the HW, so I'll remove this.

Quoted 16 lines
Quoted 5 lines
drivers/pci/host/pcie-rcar.c new file +5 -0
+
+	/* Finish initialization - establish a PCI Express link */
+	pci_write_reg(pcie, CFINIT, PCIETCTLR);
+
+	/* This will timeout if we don't have a link. */

drivers/pci/host/pcie-rcar.c

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+	err = rcar_pcie_wait_for_dl(pcie);
+	if (err)
+		return err;
+
+	/* Enable INTx interrupts */
+	rcar_rmw32(pcie, PCIEINTXR, 0, 0xF << 8);
+
+	/* Enable slave Bus Mastering */
+	rcar_rmw32(pcie, RCONF(PCI_STATUS), PCI_STATUS_DEVSEL_MASK,
+		PCI_COMMAND_IO | PCI_COMMAND_MEMORY |

PCI_COMMAND_MASTER |

Quoted 5 lines

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

Quoted 1 line
drivers/pci/host/pcie-rcar.c new file +1 -0
+		PCI_STATUS_CAP_LIST | PCI_STATUS_DEVSEL_FAST);

Hmm, you're mixing up PCI control/status registers' bits here; they're
two 16-bit registers! So you're writing to 3 reserved LSBs of the PCI status register...

The mask arg should make sure that we don't write to reserved bits. However, the bits & mask combination is clearly wrong & I'll look at this. Somewhere along the line, the use of the mask arg to the rcar_rmw32 function has clearly gone astray. I checked all the rmw calls and found another couple that are wrong.

Quoted 66 lines

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

Quoted 7 lines
drivers/pci/host/pcie-rcar.c new file +7 -0
+static int rcar_pcie_get_resources(struct platform_device *pdev,
+				   struct rcar_pcie *pcie)
+{
+	struct resource res;
+	int err;
+
+	err = of_address_to_resource(pdev->dev.of_node, 0, &res);

BTW, you could use platfrom_get_resource() and save on your local
variable
and the error check -- devm_ioremap_resource() does it.

Quoted 55 lines
drivers/pci/host/pcie-rcar.c new file +55 -0
+	if (err)
+		return err;
+
+	pcie->clk = devm_clk_get(&pdev->dev, "pcie");
+	if (IS_ERR(pcie->clk)) {
+		dev_err(pcie->dev, "cannot get platform clock\n");
+		return PTR_ERR(pcie->clk);
+	}
+	err = clk_prepare_enable(pcie->clk);
+	if (err)
+		goto fail_clk;
+
+	pcie->bus_clk = devm_clk_get(&pdev->dev, "pcie_bus");
+	if (IS_ERR(pcie->bus_clk)) {
+		dev_err(pcie->dev, "cannot get pcie bus clock\n");
+		err = PTR_ERR(pcie->bus_clk);
+		goto fail_clk;
+	}
+	err = clk_prepare_enable(pcie->bus_clk);
+	if (err)
+		goto err_map_reg;
+
+	pcie->base = devm_ioremap_resource(&pdev->dev, &res);
+	if (IS_ERR(pcie->base)) {
+		err = PTR_ERR(pcie->base);
+		goto err_map_reg;
+	}
+
+	return 0;
+
+err_map_reg:
+	clk_disable_unprepare(pcie->bus_clk);
+fail_clk:
+	clk_disable_unprepare(pcie->clk);
+
+	return err;
+}
+
+static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie,
+				    struct of_pci_range *range,
+				    int *index)
+{
+	u64 restype = range->flags;
+	u64 cpu_addr = range->cpu_addr;
+	u64 cpu_end = range->cpu_addr + range->size;
+	u64 pci_addr = range->pci_addr;
+	u32 flags = LAM_64BIT | LAR_ENABLE;
+	u64 mask;
+	u64 size;
+	int idx = *index;
+
+	if (restype & IORESOURCE_PREFETCH)
+		flags |= LAM_PREFETCH;
+
+	/*
Quoted 8 lines

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

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +6 -0
+	 * If the size of the range is larger than the alignment of the start
+	 * address, we have to use multiple entries to perform the mapping.
+	 */
+	if (cpu_addr > 0) {
+		unsigned long nr_zeros = __ffs64(cpu_addr);
+		u64 alignment = 1ULL << nr_zeros;

Missing newline...

Ok

Quoted 20 lines
Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +6 -0
+		size = min(range->size, alignment);
+	} else {
+		size = range->size;
+	}
+	/* Hardware supports max 4GiB inbound region */
+	size = min(size, 1ULL << 32);

drivers/pci/host/pcie-rcar.c

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+
+	mask = roundup_pow_of_two(size) - 1;
+	mask &= ~0xf;
+
+	while (cpu_addr < cpu_end) {
+		/*
+		 * Set up 64-bit inbound regions as the range parser doesn't
+		 * distinguish between 32 and 64-bit types.
+		 */
+		pci_write_reg(pcie, lower_32_bits(pci_addr),

PCIEPRAR(idx));

drivers/pci/host/pcie-rcar.c

Quoted 2 lines
drivers/pci/host/pcie-rcar.c new file +2 -0
+		pci_write_reg(pcie, lower_32_bits(cpu_addr), PCIELAR(idx));
+		pci_write_reg(pcie, lower_32_bits(mask) | flags,

PCIELAMR(idx));

Quoted 8 lines

drivers/pci/host/pcie-rcar.c

Quoted 2 lines
drivers/pci/host/pcie-rcar.c new file +2 -0
+
+		pci_write_reg(pcie, upper_32_bits(pci_addr),

PCIEPRAR(idx+1));

drivers/pci/host/pcie-rcar.c

Quoted 1 line
drivers/pci/host/pcie-rcar.c new file +1 -0
+		pci_write_reg(pcie, upper_32_bits(cpu_addr),

PCIELAR(idx+1));

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

Quoted 1 line
drivers/pci/host/pcie-rcar.c new file +1 -0
+		pci_write_reg(pcie, 0, PCIELAMR(idx+1));

Missing spaces around '+'...

Ok

Quoted 28 lines

drivers/pci/host/pcie-rcar.c

Quoted 7 lines
drivers/pci/host/pcie-rcar.c new file +7 -0
+
+		pci_addr += size;
+		cpu_addr += size;
+		idx += 2;
+
+		if (idx > MAX_NR_INBOUND_MAPS) {
+			dev_err(pcie->dev, "Failed to map inbound

regions!\n");

Quoted 20 lines
drivers/pci/host/pcie-rcar.c new file +20 -0
+			return -EINVAL;
+		}
+	}
+	*index = idx;
+
+	return 0;
+}
+
+static int pci_dma_range_parser_init(struct of_pci_range_parser *parser,
+				     struct device_node *node)
+{
+	const int na = 3, ns = 2;
+	int rlen;
+
+	parser->node = node;
+	parser->pna = of_n_addr_cells(node);
+	parser->np = parser->pna + na + ns;
+
+	parser->range = of_get_property(node, "dma-ranges", &rlen);
+	if (!parser->range)
Quoted 8 lines

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

Quoted 5 lines
drivers/pci/host/pcie-rcar.c new file +5 -0
+		return -ENOENT;
+
+	parser->end = parser->range + rlen / sizeof(__be32);
+	return 0;
+}

Erm, AFAIK "dma-ranges" is a standard property, shouldn't its parsing be
placed in some generic place like drivers/of/address.c?

I suppose you are right, something else to fix.

Quoted 34 lines

[...]

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+static int rcar_pcie_probe(struct platform_device *pdev)
+{
+	struct rcar_pcie *pcie;
+	unsigned int data;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	const struct of_device_id *of_id;
+	int err, win = 0;
+	int (*hw_init_fn)(struct rcar_pcie *);
+

drivers/pci/host/pcie-rcar.c

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->dev = &pdev->dev;
+	platform_set_drvdata(pdev, pcie);
+
+	/* Get the bus range */
+	if (of_pci_parse_bus_range(pdev->dev.of_node, &pcie->busn)) {
+		dev_err(&pdev->dev, "failed to parse bus-range

property\n");

drivers/pci/host/pcie-rcar.c

Quoted 11 lines
drivers/pci/host/pcie-rcar.c new file +11 -0
+		return -EINVAL;
+	}
+
+	if (of_pci_range_parser_init(&parser, pdev->dev.of_node)) {
+		dev_err(&pdev->dev, "missing ranges property\n");
+		return -EINVAL;
+	}
+
+	err = rcar_pcie_get_resources(pdev, pcie);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to request resources: %d\n",

err);

Quoted 9 lines

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

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +6 -0
+		return err;
+	}
+
+	for_each_of_pci_range(&parser, &range) {
+		of_pci_range_to_resource(&range, pdev->dev.of_node,
+						&pcie->res[win++]);

This function call is probably no good here as it fetches into the 'start'
field of a 'struct resource' a CPU address instead of a PCI address...

No, the ranges describe the CPU addresses of the PCI memory and I/O regions, so this is correct.

Quoted 15 lines
Quoted 15 lines
drivers/pci/host/pcie-rcar.c new file +15 -0
+
+		if (win > PCI_MAX_RESOURCES)
+			break;
+	}
+
+	 err = rcar_pcie_parse_map_dma_ranges(pcie, pdev->dev.of_node);
+	 if (err)
+		return err;
+
+	of_id = of_match_device(rcar_pcie_of_match, pcie->dev);
+	if (!of_id || !of_id->data)
+		return -EINVAL;
+	hw_init_fn = of_id->data;
+
+	/* Failure to get a link might just be that no cards are inserted */
Quoted 8 lines

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

Quoted 4 lines
drivers/pci/host/pcie-rcar.c new file +4 -0
+	err = hw_init_fn(pcie);
+	if (err) {
+		dev_info(&pdev->dev, "PCIe link down\n");
+		return 0;

Not quite sure why you exit normally here without enabling the hardware.
I think the internal bridge should be visible regardless of whether link is detected or not...

Why would you want to see the bridge when you can do nothing with it? Aren't you are just wasting resources?

Quoted 10 lines
Quoted 9 lines
drivers/pci/host/pcie-rcar.c new file +9 -0
+	}
+
+	data = pci_read_reg(pcie, MACSR);
+	dev_info(&pdev->dev, "PCIe x%d: link up\n", (data >> 20) & 0x3f);
+
+	rcar_pcie_enable(pcie);
+
+	return 0;
+}

[...]

Thanks
Phil

#4 lore raw

PATCH v8 1/3 PCI: host: rcar: Add Renesas R-Car PCIe driver

Hello.

Quoted 2 lines · Phil Edworthy
Quoted 2 lines

I'm investigating an imprecise external abort occurring once userland is
started when I have NetMos

Or is it MosChip now? Can't remember all their renames. :-)

Quoted 3 lines
Quoted 3 lines

PCIe serial card inserted and the '8250_pci'
driver
enabled and I have found some issues in this driver, while at it...

I should mention that the serial PCI device has both I/O port and memory
BARs; it's the driver's choice to use the I/O ports.

Quoted 1 line

Shame they didn't come before the driver was accepted,

Sorry, I don't usually review large patches -- it's very time consuming
(my review took 2+ hours and yet I haven't pointed out all issues).

Quoted 1 line

but still, I welcome the comments. See below.

    Thanks. :-)
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines

This PCIe Host driver currently does not support MSI, so cards fall back to INTx interrupts.

[...]

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +0 -0
new file mode 100644
index 0000000..3c524b9
@@ -0,0 +1,768 @@

[...]

Quoted 14 lines
Quoted 8 lines

drivers/pci/host/pcie-rcar.c

Quoted 7 lines
drivers/pci/host/pcie-rcar.c new file +7 -0
+static void pci_write_reg(struct rcar_pcie *pcie, unsigned long val,
+			  unsigned long reg)
+{
+	writel(val, pcie->base + reg);
+}
+
+static unsigned long pci_read_reg(struct rcar_pcie *pcie, unsigned long

reg)

Quoted 5 lines

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

Quoted 3 lines
drivers/pci/host/pcie-rcar.c new file +3 -0
+{
+	return readl(pcie->base + reg);
+}

As a side note, these functions are hardly needed, and risk collision too...

Quoted 3 lines

Ben mentioned this in his review and as I said then, I found them useful during development, so we agreed to leave them. Since they are static, there shouldn't be a collision risk.

You're risking clashes even at the source level, not even at object file
level...

Quoted 8 lines
Quoted 6 lines

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

Quoted 2 lines
drivers/pci/host/pcie-rcar.c new file +2 -0
+static void rcar_pcie_setup_window(int win, struct resource *res,
+				   struct rcar_pcie *pcie)

As a side note, 'res' parameter is hardly needed here, as the function
always gets
called with the resources contained within 'struct rcar_pcie'...

Either I would have to pass an index to the resource in,

But you already do pass it, 'win' is the index!

Quoted 2 lines

or as I have done, a
pointer to the individual resource. I found it cleaner to pass the pointer.

You're actually pass excess parameters, both the index and the pointer.

[...]

Quoted 16 lines
Quoted 7 lines

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

Quoted 5 lines
drivers/pci/host/pcie-rcar.c new file +5 -0
+
+	/* First resource is for IO */
+	mask = PAR_ENABLE;
+	if (res->flags & IORESOURCE_IO)
+		mask |= IO_SPACE;

For the memory space this works OK as you're identity-mapping the

Quoted 8 lines

memory
ranges in your device trees. However, for the I/O space this means that it won't work as the BARs in the PCIe devices get programmed with the PCI bus addresses but the PCIe window translation register is programmed with a CPU
address which don't at all match (given your device trees) and hence one can't
access the card's I/O mapped registers at all...

Quoted 2 lines

Hmm, I couldn't find any cards that supported I/O, so I wasn't able to test this. Clearly this is an issue that needs looking into.

Will you look into it then, or should I?

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
drivers/pci/host/pcie-rcar.c new file +17 -0
+
+	pci_write_reg(pcie, mask, PCIEPTCTLR(win));
+}
+
+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;
+
+	pcie->root_bus_nr = -1;
+
+	/* Setup PCI resources */
+	for (i = 0; i < PCI_MAX_RESOURCES; i++) {
+
+		res = &pcie->res[i];
+		if (!res->flags)
Quoted 8 lines
Quoted 8 lines

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

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +6 -0
+			continue;
+
+		rcar_pcie_setup_window(i, res, pcie);
+
+		if (res->flags & IORESOURCE_IO)
+			pci_ioremap_io(nr * SZ_64K, res->start);

I'm not sure why are you not calling pci_add_resource() for I/O space...

Sorry, did you reply to that?

Quoted 7 lines
Quoted 2 lines

Also, this sets up only 64 KiB of I/O ports while your device tree describes I/O space 1 MiB is size.

This driver should be able to cope with multiple host controllers, so each allocated 64KiB for I/O. 64KiB is all you need for I/O, but the R-Car PCIe hardware has a 1MiB region (the smallest one) that can only be used for one type of PCIe access.

[...]

Quoted 21 lines
Quoted 12 lines
Quoted 12 lines
drivers/pci/host/pcie-rcar.c new file +12 -0
+static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
+{
+	int err;
+
+	/* Begin initialization */
+	pci_write_reg(pcie, 0, PCIETCTLR);
+
+	/* Set mode */
+	pci_write_reg(pcie, 1, PCIEMSR);
+
+	/*
+	 * Initial header for port config space is type 1, set the device
Quoted 8 lines

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

Quoted 4 lines
drivers/pci/host/pcie-rcar.c new file +4 -0
+	 * class to match. Hardware takes care of propagating the IDSETR
+	 * settings, so there is no need to bother with a quirk.
+	 */
+	pci_write_reg(pcie, PCI_CLASS_BRIDGE_PCI << 16, IDSETR1);

Hm, shouldn't this be a host bridge? I've noticed that the bridge's I/O
and memory base/limit registers are left uninitialized even though the BARs of the PICe devices behind this bridge are assigned.

Quoted 1 line

No, I am pretty sure this is correct.

It just looks strange. What you actually have is clearly a host-to-PCI
bridge. Instead you have one "virtual" PCI bus consisting of only PCI-PCI bridge device, and the real PCIe bus hanging from the PCI-PCI bridge. Weird...

[...]

Quoted 5 lines
Quoted 5 lines

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

Quoted 5 lines
drivers/pci/host/pcie-rcar.c new file +5 -0
+
+	/* Terminate list of capabilities (Next Capability Offset=0) */
+	rcar_rmw32(pcie, RVCCAP(0), 0xfff0, 0);
+
+	/* Enable MAC data scrambling. */

I wonder what does MAC mean in the PCIe context...

Quoted 7 lines
Quoted 3 lines

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

Quoted 1 line
drivers/pci/host/pcie-rcar.c new file +1 -0
+	rcar_rmw32(pcie, MACCTLR, SCRAMBLE_DISABLE, 0);

Doesn't the comment contradict the code here?

No, the rmw32 function is read, modify, write and the SCRAMBLE_DISABLE shown here is the mask, not the value. If the last arg was 1, the call would set the scramble disable bit to 1.

Ah, missed that, sorry.

Quoted 1 line

Anyway, scrambling is enabled by default in the HW, so I'll remove this.

    OK.
Quoted 16 lines
Quoted 16 lines
Quoted 5 lines
drivers/pci/host/pcie-rcar.c new file +5 -0
+
+	/* Finish initialization - establish a PCI Express link */
+	pci_write_reg(pcie, CFINIT, PCIETCTLR);
+
+	/* This will timeout if we don't have a link. */

drivers/pci/host/pcie-rcar.c

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+	err = rcar_pcie_wait_for_dl(pcie);
+	if (err)
+		return err;
+
+	/* Enable INTx interrupts */
+	rcar_rmw32(pcie, PCIEINTXR, 0, 0xF << 8);
+
+	/* Enable slave Bus Mastering */
+	rcar_rmw32(pcie, RCONF(PCI_STATUS), PCI_STATUS_DEVSEL_MASK,
+		PCI_COMMAND_IO | PCI_COMMAND_MEMORY |

PCI_COMMAND_MASTER |

Quoted 5 lines
Quoted 5 lines

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

Quoted 1 line
drivers/pci/host/pcie-rcar.c new file +1 -0
+		PCI_STATUS_CAP_LIST | PCI_STATUS_DEVSEL_FAST);

Hmm, you're mixing up PCI control/status registers' bits here; they're
two 16-bit registers! So you're writing to 3 reserved LSBs of the PCI status register...

... and therefore not writing these bits to the PCI command (not control,
sorry) register. Perhaps because of that PCI-PCI bridge remains inactive...

Quoted 1 line

The mask arg should make sure that we don't write to reserved bits. However,

Look at rcar_rmw32() again -- it doesn't really do that.

Quoted 4 lines

the bits & mask combination is clearly wrong & I'll look at this. Somewhere along the line, the use of the mask arg to the rcar_rmw32 function has clearly gone astray. I checked all the rmw calls and found another couple that are wrong.

OK, please fix those.

[...]

Quoted 43 lines
Quoted 33 lines
Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+static int rcar_pcie_probe(struct platform_device *pdev)
+{
+	struct rcar_pcie *pcie;
+	unsigned int data;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	const struct of_device_id *of_id;
+	int err, win = 0;
+	int (*hw_init_fn)(struct rcar_pcie *);
+

drivers/pci/host/pcie-rcar.c

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->dev = &pdev->dev;
+	platform_set_drvdata(pdev, pcie);
+
+	/* Get the bus range */
+	if (of_pci_parse_bus_range(pdev->dev.of_node, &pcie->busn)) {
+		dev_err(&pdev->dev, "failed to parse bus-range

property\n");

drivers/pci/host/pcie-rcar.c

Quoted 11 lines
drivers/pci/host/pcie-rcar.c new file +11 -0
+		return -EINVAL;
+	}
+
+	if (of_pci_range_parser_init(&parser, pdev->dev.of_node)) {
+		dev_err(&pdev->dev, "missing ranges property\n");
+		return -EINVAL;
+	}
+
+	err = rcar_pcie_get_resources(pdev, pcie);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to request resources: %d\n",

err);

Quoted 9 lines

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

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +6 -0
+		return err;
+	}
+
+	for_each_of_pci_range(&parser, &range) {
+		of_pci_range_to_resource(&range, pdev->dev.of_node,
+						&pcie->res[win++]);

This function call is probably no good here as it fetches into the 'start'
field of a 'struct resource' a CPU address instead of a PCI address...

Quoted 2 lines

No, the ranges describe the CPU addresses of the PCI memory and I/O regions, so this is correct.

The problem actually is that you need to remember both CPU and PCI
addresses, so 'struct of_pci_range' looks more fitting here...

Quoted 24 lines
Quoted 15 lines
Quoted 15 lines
drivers/pci/host/pcie-rcar.c new file +15 -0
+
+		if (win > PCI_MAX_RESOURCES)
+			break;
+	}
+
+	 err = rcar_pcie_parse_map_dma_ranges(pcie, pdev->dev.of_node);
+	 if (err)
+		return err;
+
+	of_id = of_match_device(rcar_pcie_of_match, pcie->dev);
+	if (!of_id || !of_id->data)
+		return -EINVAL;
+	hw_init_fn = of_id->data;
+
+	/* Failure to get a link might just be that no cards are inserted */
Quoted 8 lines

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

Quoted 4 lines
drivers/pci/host/pcie-rcar.c new file +4 -0
+	err = hw_init_fn(pcie);
+	if (err) {
+		dev_info(&pdev->dev, "PCIe link down\n");
+		return 0;

Not quite sure why you exit normally here without enabling the hardware.
I think the internal bridge should be visible regardless of whether link is detected or not...

Quoted 1 line

Why would you want to see the bridge when you can do nothing with it? Aren't

Because it's the way PCI works. You have the built-in devices always
present and seen on a PCI bus. :-)

Quoted 1 line

you are just wasting resources?

I think it's rather you who are wasting resources. ;-) Why not just fail
the probe when you have no link?

WBR, Sergei

#5 lore raw

PATCH v8 1/3 PCI: host: rcar: Add Renesas R-Car PCIe driver

Hi Sergei,

Quoted 6 lines · Sergei
Quoted 2 lines · Phil Edworthy
Quoted 2 lines

I'm investigating an imprecise external abort occurring once userland is
started when I have NetMos

Or is it MosChip now? Can't remember all their renames. :-)

Do you know of somewhere I can buy a card with this chipset in the EU? I had a quick search but couldn't find anything.

[...]

Quoted 15 lines
Quoted 8 lines
Quoted 8 lines

drivers/pci/host/pcie-rcar.c

Quoted 7 lines
drivers/pci/host/pcie-rcar.c new file +7 -0
+static void pci_write_reg(struct rcar_pcie *pcie, unsigned long val,
+			  unsigned long reg)
+{
+	writel(val, pcie->base + reg);
+}
+
+static unsigned long pci_read_reg(struct rcar_pcie *pcie, unsigned long

reg)

Quoted 5 lines
Quoted 5 lines

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

Quoted 3 lines
drivers/pci/host/pcie-rcar.c new file +3 -0
+{
+	return readl(pcie->base + reg);
+}

As a side note, these functions are hardly needed, and risk collision

too...

Quoted 8 lines
Quoted 1 line

Ben mentioned this in his review and as I said then, I found them useful

during

Quoted 1 line

development, so we agreed to leave them. Since they are static, there

shouldn't

Quoted 1 line

be a collision risk.

You're risking clashes even at the source level, not even at object file
level...

Ah, yes you are correct.

Quoted 7 lines
Quoted 6 lines
Quoted 6 lines

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

Quoted 2 lines
drivers/pci/host/pcie-rcar.c new file +2 -0
+static void rcar_pcie_setup_window(int win, struct resource *res,
+				   struct rcar_pcie *pcie)

As a side note, 'res' parameter is hardly needed here, as the function
always gets
called with the resources contained within 'struct rcar_pcie'...

Quoted 8 lines
Quoted 1 line

Either I would have to pass an index to the resource in,

But you already do pass it, 'win' is the index!

Quoted 2 lines

or as I have done, a
pointer to the individual resource. I found it cleaner to pass the pointer.

You're actually pass excess parameters, both the index and the pointer.

Ha, yes I didn't notice that :)

[...]

Quoted 17 lines
Quoted 10 lines
Quoted 10 lines

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

Quoted 5 lines
drivers/pci/host/pcie-rcar.c new file +5 -0
+
+	/* First resource is for IO */
+	mask = PAR_ENABLE;
+	if (res->flags & IORESOURCE_IO)
+		mask |= IO_SPACE;

For the memory space this works OK as you're identity-mapping the
memory
ranges in your device trees. However, for the I/O space this means that it won't work as the BARs in the PCIe devices get programmed with the PCI

bus

Quoted 5 lines
Quoted 5 lines

addresses but the PCIe window translation register is programmed with a CPU
address which don't at all match (given your device trees) and hence one can't
access the card's I/O mapped registers at all...

Quoted 4 lines
Quoted 2 lines

Hmm, I couldn't find any cards that supported I/O, so I wasn't able to test this. Clearly this is an issue that needs looking into.

Will you look into it then, or should I?

I'll look at it.

Quoted 26 lines
Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
drivers/pci/host/pcie-rcar.c new file +17 -0
+
+	pci_write_reg(pcie, mask, PCIEPTCTLR(win));
+}
+
+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;
+
+	pcie->root_bus_nr = -1;
+
+	/* Setup PCI resources */
+	for (i = 0; i < PCI_MAX_RESOURCES; i++) {
+
+		res = &pcie->res[i];
+		if (!res->flags)
Quoted 8 lines
Quoted 8 lines

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

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +6 -0
+			continue;
+
+		rcar_pcie_setup_window(i, res, pcie);
+
+		if (res->flags & IORESOURCE_IO)
+			pci_ioremap_io(nr * SZ_64K, res->start);

I'm not sure why are you not calling pci_add_resource() for I/O space...

Quoted 1 line

Sorry, did you reply to that?

I used the tegra driver to inform on what I should do for this. This doesn't call pci_add_resource() for I/O space either. However, I also see that other drivers do call this. I think the simplest thing is for me to get a card that supports I/O space and properly test it.

Quoted 9 lines
Quoted 6 lines
Quoted 2 lines

Also, this sets up only 64 KiB of I/O ports while your device tree describes I/O space 1 MiB is size.

This driver should be able to cope with multiple host controllers, so each allocated 64KiB for I/O. 64KiB is all you need for I/O, but the R-Car PCIe hardware has a 1MiB region (the smallest one) that can only be used for

one

Quoted 1 line

type of PCIe access.

[...]

Quoted 22 lines
Quoted 16 lines
Quoted 16 lines
Quoted 5 lines
drivers/pci/host/pcie-rcar.c new file +5 -0
+
+	/* Finish initialization - establish a PCI Express link */
+	pci_write_reg(pcie, CFINIT, PCIETCTLR);
+
+	/* This will timeout if we don't have a link. */

drivers/pci/host/pcie-rcar.c

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+	err = rcar_pcie_wait_for_dl(pcie);
+	if (err)
+		return err;
+
+	/* Enable INTx interrupts */
+	rcar_rmw32(pcie, PCIEINTXR, 0, 0xF << 8);
+
+	/* Enable slave Bus Mastering */
+	rcar_rmw32(pcie, RCONF(PCI_STATUS), PCI_STATUS_DEVSEL_MASK,
+		PCI_COMMAND_IO | PCI_COMMAND_MEMORY |

PCI_COMMAND_MASTER |

Quoted 5 lines
Quoted 5 lines

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

Quoted 1 line
drivers/pci/host/pcie-rcar.c new file +1 -0
+		PCI_STATUS_CAP_LIST | PCI_STATUS_DEVSEL_FAST);

Hmm, you're mixing up PCI control/status registers' bits here; they're
two 16-bit registers! So you're writing to 3 reserved LSBs of the PCI status register...

Quoted 7 lines

... and therefore not writing these bits to the PCI command (not control,
sorry) register. Perhaps because of that PCI-PCI bridge remains inactive...

Quoted 1 line

The mask arg should make sure that we don't write to reserved bits.

However,

Look at rcar_rmw32() again -- it doesn't really do that.

Yeah, that's why I said it should, not does. It only clears those bits in the register's current value.

[...]

Quoted 43 lines
Quoted 42 lines
Quoted 42 lines
Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+static int rcar_pcie_probe(struct platform_device *pdev)
+{
+	struct rcar_pcie *pcie;
+	unsigned int data;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	const struct of_device_id *of_id;
+	int err, win = 0;
+	int (*hw_init_fn)(struct rcar_pcie *);
+

drivers/pci/host/pcie-rcar.c

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->dev = &pdev->dev;
+	platform_set_drvdata(pdev, pcie);
+
+	/* Get the bus range */
+	if (of_pci_parse_bus_range(pdev->dev.of_node, &pcie->busn)) {
+		dev_err(&pdev->dev, "failed to parse bus-range

property\n");

drivers/pci/host/pcie-rcar.c

Quoted 11 lines
drivers/pci/host/pcie-rcar.c new file +11 -0
+		return -EINVAL;
+	}
+
+	if (of_pci_range_parser_init(&parser, pdev->dev.of_node)) {
+		dev_err(&pdev->dev, "missing ranges property\n");
+		return -EINVAL;
+	}
+
+	err = rcar_pcie_get_resources(pdev, pcie);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to request resources: %d\n",

err);

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

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +6 -0
+		return err;
+	}
+
+	for_each_of_pci_range(&parser, &range) {
+		of_pci_range_to_resource(&range, pdev->dev.of_node,
+						&pcie->res[win++]);

This function call is probably no good here as it fetches into the 'start'
field of a 'struct resource' a CPU address instead of a PCI address...

Quoted 6 lines
Quoted 1 line

No, the ranges describe the CPU addresses of the PCI memory and I/O

regions, so

Quoted 1 line

this is correct.

The problem actually is that you need to remember both CPU and PCI
addresses, so 'struct of_pci_range' looks more fitting here...

Right, I see... this is rather a mess with all the host drivers!

Quoted 28 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
drivers/pci/host/pcie-rcar.c new file +13 -0
+
+		if (win > PCI_MAX_RESOURCES)
+			break;
+	}
+
+	 err = rcar_pcie_parse_map_dma_ranges(pcie, pdev->dev.of_node);
+	 if (err)
+		return err;
+
+	of_id = of_match_device(rcar_pcie_of_match, pcie->dev);
+	if (!of_id || !of_id->data)
+		return -EINVAL;
+	hw_init_fn = of_id->data;
Quoted 8 lines
Quoted 8 lines

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

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +6 -0
+
+	/* Failure to get a link might just be that no cards are inserted */
+	err = hw_init_fn(pcie);
+	if (err) {
+		dev_info(&pdev->dev, "PCIe link down\n");
+		return 0;

Not quite sure why you exit normally here without enabling the

hardware.

Quoted 4 lines
Quoted 2 lines

I think the internal bridge should be visible regardless of whether link is detected or not...

Why would you want to see the bridge when you can do nothing with it?

Aren't

Quoted 7 lines

Because it's the way PCI works. You have the built-in devices always
present and seen on a PCI bus. :-)

Quoted 1 line

you are just wasting resources?

I think it's rather you who are wasting resources. ;-) Why not just fail
the probe when you have no link?

Ah, so we currently have a half-way house... not failing the probe, but not enabling the HW. Either all or nothing would be preferable.

Thanks
Phil

#6 lore raw

PATCH v8 1/3 PCI: host: rcar: Add Renesas R-Car PCIe driver

Hello.

Quoted 7 lines · Phil Edworthy
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines

I'm investigating an imprecise external abort occurring once userland is
started when I have NetMos

Or is it MosChip now? Can't remember all their renames. :-)

Do you know of somewhere I can buy a card with this chipset in the EU? I had a quick search but couldn't find anything.

No. But we probably can send such card to you.

[...]

Quoted 16 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines

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

Quoted 5 lines
drivers/pci/host/pcie-rcar.c new file +5 -0
+
+	/* First resource is for IO */
+	mask = PAR_ENABLE;
+	if (res->flags & IORESOURCE_IO)
+		mask |= IO_SPACE;

For the memory space this works OK as you're identity-mapping the
memory
ranges in your device trees. However, for the I/O space this means that it won't work as the BARs in the PCIe devices get programmed with the PCI bus addresses but the PCIe window translation register is programmed with a CPU
address which don't at all match (given your device trees) and hence one can't
access the card's I/O mapped registers at all...

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

Hmm, I couldn't find any cards that supported I/O, so I wasn't able to test this. Clearly this is an issue that needs looking into.

Will you look into it then, or should I?

I'll look at it.

Thanks.

[...]

Quoted 31 lines
Quoted 30 lines
Quoted 25 lines
Quoted 16 lines
Quoted 16 lines
drivers/pci/host/pcie-rcar.c new file +16 -0
+
+		if (win > PCI_MAX_RESOURCES)
+			break;
+	}
+
+	 err = rcar_pcie_parse_map_dma_ranges(pcie, pdev->dev.of_node);
+	 if (err)
+		return err;
+
+	of_id = of_match_device(rcar_pcie_of_match, pcie->dev);
+	if (!of_id || !of_id->data)
+		return -EINVAL;
+	hw_init_fn = of_id->data;
+
+	/* Failure to get a link might just be that no cards are inserted */
+	err = hw_init_fn(pcie);
Quoted 8 lines

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

Quoted 3 lines
drivers/pci/host/pcie-rcar.c new file +3 -0
+	if (err) {
+		dev_info(&pdev->dev, "PCIe link down\n");
+		return 0;

Not quite sure why you exit normally here without enabling the
hardware.
I think the internal bridge should be visible regardless of whether link is detected or not...

Quoted 1 line

Why would you want to see the bridge when you can do nothing with it?

Aren't

Because it's the way PCI works. You have the built-in devices always
present and seen on a PCI bus. :-)

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

you are just wasting resources?

I think it's rather you who are wasting resources. ;-) Why not just fail
the probe when you have no link?

Ah, so we currently have a half-way house... not failing the probe, but not enabling the HW. Either all or nothing would be preferable.

Actually, I tried ignoring the link test and the kernel died horrible
death without any console output. :-/ Having enabled the earlyprintk support, I was able to see the reason: panic("PCI: unable to scan bus!") in arch/arm/kernel/bios32.c...

Quoted 2 lines

Thanks
Phil

WBR, Sergei

#7 lore raw

PATCH v8 1/3 PCI: host: rcar: Add Renesas R-Car PCIe driver

Hi Sergei,

Quoted 6 lines · Sergei
Quoted 1 line · Phil Edworthy
Quoted 1 line
Quoted 1 line
Quoted 1 line

I'm investigating an imprecise external abort occurring once userland

is

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

started when I have NetMos

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

Or is it MosChip now? Can't remember all their renames. :-)

Do you know of somewhere I can buy a card with this chipset in the EU? I

had a

Quoted 1 line

quick search but couldn't find anything.

No. But we probably can send such card to you.

That would be handy!

Quoted 81 lines

[...]

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

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

Quoted 5 lines
drivers/pci/host/pcie-rcar.c new file +5 -0
+
+	/* First resource is for IO */
+	mask = PAR_ENABLE;
+	if (res->flags & IORESOURCE_IO)
+		mask |= IO_SPACE;

For the memory space this works OK as you're identity-mapping the
memory
ranges in your device trees. However, for the I/O space this means that

it

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

won't work as the BARs in the PCIe devices get programmed with the

PCI bus

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

addresses but the PCIe window translation register is programmed

with a

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

CPU
address which don't at all match (given your device trees) and hence

one

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines

can't
access the card's I/O mapped registers at all...

Hmm, I couldn't find any cards that supported I/O, so I wasn't able to

test

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

this. Clearly this is an issue that needs looking into.

Will you look into it then, or should I?

I'll look at it.

Thanks.

[...]

Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

drivers/pci/host/pcie-rcar.c

Quoted 6 lines
drivers/pci/host/pcie-rcar.c new file +6 -0
+
+		if (win > PCI_MAX_RESOURCES)
+			break;
+	}
+
+	 err = rcar_pcie_parse_map_dma_ranges(pcie, pdev-

dev.of_node);

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines

drivers/pci/host/pcie-rcar.c

Quoted 9 lines
drivers/pci/host/pcie-rcar.c new file +9 -0
+	 if (err)
+		return err;
+
+	of_id = of_match_device(rcar_pcie_of_match, pcie->dev);
+	if (!of_id || !of_id->data)
+		return -EINVAL;
+	hw_init_fn = of_id->data;
+
+	/* Failure to get a link might just be that no cards are inserted

*/

Quoted 16 lines
Quoted 15 lines
Quoted 11 lines
Quoted 9 lines

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

Quoted 4 lines
drivers/pci/host/pcie-rcar.c new file +4 -0
+	err = hw_init_fn(pcie);
+	if (err) {
+		dev_info(&pdev->dev, "PCIe link down\n");
+		return 0;

Not quite sure why you exit normally here without enabling the
hardware.
I think the internal bridge should be visible regardless of whether link is detected or not...

Why would you want to see the bridge when you can do nothing with it?

Aren't

Because it's the way PCI works. You have the built-in devices always
present and seen on a PCI bus. :-)

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

you are just wasting resources?

I think it's rather you who are wasting resources. ;-) Why not just fail
the probe when you have no link?

Ah, so we currently have a half-way house... not failing the probe, but not enabling the HW. Either all or nothing would be preferable.

Actually, I tried ignoring the link test and the kernel died horrible
death without any console output. :-/ Having enabled the earlyprintk support,
I was able to see the reason: panic("PCI: unable to scan bus!") in arch/arm/kernel/bios32.c...

Quoted 4 lines
Quoted 2 lines

Thanks
Phil

WBR, Sergei

Thanks
Phil

#8 lore raw

PATCH v8 1/3 PCI: host: rcar: Add Renesas R-Car PCIe driver

In reply to Phil Edworthy

Hi Phil,

Just a question...

Quoted 21 lines · Phil Edworthy

This PCIe Host driver currently does not support MSI, so cards fall back to INTx interrupts.

---
 drivers/pci/host/Kconfig     |   6 +
 drivers/pci/host/Makefile    |   1 +
 drivers/pci/host/pcie-rcar.c | 768 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 775 insertions(+)
 create mode 100644 drivers/pci/host/pcie-rcar.c
drivers/pci/host/pcie-rcar.c new file +3 -0
new file mode 100644
index 0000000..3c524b9
 
+static void rcar_pcie_enable(struct rcar_pcie *pcie)
+{
+       struct platform_device *pdev = to_platform_device(pcie->dev);

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

Quoted 10 lines
drivers/pci/host/pcie-rcar.c new file +10 -0
+
+       rcar_pci.nr_controllers = 1;
+       rcar_pci.private_data = (void **)&pcie;
+
+       pci_common_init_dev(&pdev->dev, &rcar_pci);
+#ifdef CONFIG_PCI_DOMAINS
+       rcar_pci.domain++;
+#endif
+}
+

How does it work when you have 2 PCIe DT node ?
(because for my point of view pci_common_init_dev() can't be called twice)

void pci_common_init_dev(struct device *parent, struct hw_pci *hw)

{
    struct pci_sys_data *sys;
    LIST_HEAD(head);

    pci_add_flags(PCI_REASSIGN_ALL_RSRC);
    if (hw->preinit)
        hw->preinit();
    pcibios_init_hw(parent, hw, &head);

...

static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
struct list_head *head)

{
    struct pci_sys_data *sys = NULL;
    int ret;
    int nr, busnr;

    for (nr = busnr = 0; nr < hw->nr_controllers; nr++) {
        sys = kzalloc(sizeof(struct pci_sys_data), GFP_KERNEL);
        if (!sys)
            panic("PCI: unable to allocate sys data!");
#ifdef CONFIG_PCI_DOMAINS
        sys->domain  = hw->domain;
#endif
        sys->busnr   = busnr;

...

the issue is that sys->busnr always starts to zero ...

how did you fix this problem?

Thanks.
Best Regards

Gabriel.

#9 lore raw

PATCH v8 2/3 PCI: host: rcar: Add MSI support

In reply to Phil Edworthy

v8:
- Moved header file contents to c file
- Formatting cleaned up
- Removed unused variable in rcar_msi_free()

v7:
- No changes

v6:
- Don't check MSI irq number is valid, as upper level checks this
- Change "Unexpected MSI" msg to debug level
- Reword "Unexpected MSI" comment so that it's one line

v5:
- Return IRQ_NONE from MSI isr when there is no pending MSI
- Add additional interrupt bindings

---
 drivers/pci/host/pcie-rcar.c | 242 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 241 insertions(+), 1 deletion(-)
drivers/pci/host/pcie-rcar.c +241 -1
index 3c524b9..8e06124 100644
@@ -15,8 +15,11 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/msi.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_pci.h>
@@ -35,6 +38,7 @@
 #define PCIECDR			0x000020
 #define PCIEMSR			0x000028
 #define PCIEINTXR		0x000400
+#define PCIEMSITXR		0x000840
 
 /* Transfer control */
 #define PCIETCTLR		0x02000
@@ -43,6 +47,11 @@
 #define  DATA_LINK_ACTIVE	1
 #define PCIEERRFR		0x02020
 #define  UNSUPPORTED_REQUEST	(1 << 4)
+#define PCIEMSIFR		0x02044
+#define PCIEMSIALR		0x02048
+#define  MSIFE			1
+#define PCIEMSIAUR		0x0204c
+#define PCIEMSIIER		0x02050
 
 /* root port address */
 #define PCIEPRAR(x)		(0x02080 + ((x) * 0x4))
@@ -85,6 +94,8 @@
 #define H1_PCIEPHYDOUTR		0x040014
 #define H1_PCIEPHYSR		0x040018
 
+#define INT_PCI_MSI_NR	32
+
 #define RCONF(x)	(PCICONF(0)+(x))
 #define RPMCAP(x)	(PMCAP(0)+(x))
 #define REXPCAP(x)	(EXPCAP(0)+(x))
@@ -97,6 +108,21 @@
 #define PCI_MAX_RESOURCES 4
 #define MAX_NR_INBOUND_MAPS 6
 
+struct rcar_msi {
+	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
+	struct irq_domain *domain;
+	struct msi_chip chip;
+	unsigned long pages;
+	struct mutex lock;
+	int irq1;
+	int irq2;
+};
+
+static inline struct rcar_msi *to_rcar_msi(struct msi_chip *chip)
+{
+	return container_of(chip, struct rcar_msi, chip);
+}
+
 /* Structure representing the PCIe interface */
 struct rcar_pcie {
 	struct device		*dev;
@@ -106,6 +132,7 @@ struct rcar_pcie {
 	int			root_bus_nr;
 	struct clk		*clk;
 	struct clk		*bus_clk;
+	struct			rcar_msi msi;
 };
 
 static inline struct rcar_pcie *sys_to_pcie(struct pci_sys_data *sys)
@@ -356,10 +383,20 @@ static int rcar_pcie_setup(int nr, struct pci_sys_data *sys)
 	return 1;
 }
 
+static void rcar_pcie_add_bus(struct pci_bus *bus)
+{
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		struct rcar_pcie *pcie = sys_to_pcie(bus->sysdata);
+
+		bus->msi = &pcie->msi.chip;
+	}
+}
+
 struct hw_pci rcar_pci = {
 	.setup          = rcar_pcie_setup,
 	.map_irq        = of_irq_parse_and_map_pci,
 	.ops            = &rcar_pcie_ops,
+	.add_bus        = rcar_pcie_add_bus,
 };
 
 static void rcar_pcie_enable(struct rcar_pcie *pcie)
@@ -477,6 +514,10 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
 	/* Enable MAC data scrambling. */
 	rcar_rmw32(pcie, MACCTLR, SCRAMBLE_DISABLE, 0);
 
+	/* Enable MSI */
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		pci_write_reg(pcie, 0x101f0000, PCIEMSITXR);
+
 	/* Finish initialization - establish a PCI Express link */
 	pci_write_reg(pcie, CFINIT, PCIETCTLR);
 
@@ -530,11 +571,184 @@ static int rcar_pcie_hw_init_h1(struct rcar_pcie *pcie)
 	return -ETIMEDOUT;
 }
 
+static int rcar_msi_alloc(struct rcar_msi *chip)
+{
+	int msi;
+
+	mutex_lock(&chip->lock);
+
+	msi = find_first_zero_bit(chip->used, INT_PCI_MSI_NR);
+	if (msi < INT_PCI_MSI_NR)
+		set_bit(msi, chip->used);
+	else
+		msi = -ENOSPC;
+
+	mutex_unlock(&chip->lock);
+
+	return msi;
+}
+
+static void rcar_msi_free(struct rcar_msi *chip, unsigned long irq)
+{
+	mutex_lock(&chip->lock);
+	clear_bit(irq, chip->used);
+	mutex_unlock(&chip->lock);
+}
+
+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);
+
+	/* MSI & INTx share an interrupt - we only handle MSI here */
+	if (!reg)
+		return IRQ_NONE;
+
+	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 {
+			/* Unknown MSI, just clear it */
+			dev_dbg(pcie->dev, "unexpected MSI\n");
+		}
+
+		/* see if there's any more pending in this vector */
+		reg = pci_read_reg(pcie, PCIEMSIFR);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int rcar_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
+			      struct msi_desc *desc)
+{
+	struct rcar_msi *msi = to_rcar_msi(chip);
+	struct rcar_pcie *pcie = container_of(chip, struct rcar_pcie, msi.chip);
+	struct msi_msg msg;
+	unsigned int irq;
+	int hwirq;
+
+	hwirq = rcar_msi_alloc(msi);
+	if (hwirq < 0)
+		return hwirq;
+
+	irq = irq_create_mapping(msi->domain, hwirq);
+	if (!irq) {
+		rcar_msi_free(msi, hwirq);
+		return -EINVAL;
+	}
+
+	irq_set_msi_desc(irq, desc);
+
+	msg.address_lo = pci_read_reg(pcie, PCIEMSIALR) & ~MSIFE;
+	msg.address_hi = pci_read_reg(pcie, PCIEMSIAUR);
+	msg.data = hwirq;
+
+	write_msi_msg(irq, &msg);
+
+	return 0;
+}
+
+static void rcar_msi_teardown_irq(struct msi_chip *chip, unsigned int irq)
+{
+	struct rcar_msi *msi = to_rcar_msi(chip);
+	struct irq_data *d = irq_get_irq_data(irq);
+
+	rcar_msi_free(msi, d->hwirq);
+}
+
+static struct irq_chip rcar_msi_irq_chip = {
+	.name = "R-Car PCIe MSI",
+	.irq_enable = unmask_msi_irq,
+	.irq_disable = mask_msi_irq,
+	.irq_mask = mask_msi_irq,
+	.irq_unmask = unmask_msi_irq,
+};
+
+static int rcar_msi_map(struct irq_domain *domain, unsigned int irq,
+			irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &rcar_msi_irq_chip, handle_simple_irq);
+	irq_set_chip_data(irq, domain->host_data);
+	set_irq_flags(irq, IRQF_VALID);
+
+	return 0;
+}
+
+static const struct irq_domain_ops msi_domain_ops = {
+	.map = rcar_msi_map,
+};
+
+static int rcar_pcie_enable_msi(struct rcar_pcie *pcie)
+{
+	struct platform_device *pdev = to_platform_device(pcie->dev);
+	struct rcar_msi *msi = &pcie->msi;
+	unsigned long base;
+	int err;
+
+	mutex_init(&msi->lock);
+
+	msi->chip.dev = pcie->dev;
+	msi->chip.setup_irq = rcar_msi_setup_irq;
+	msi->chip.teardown_irq = rcar_msi_teardown_irq;
+
+	msi->domain = irq_domain_add_linear(pcie->dev->of_node, INT_PCI_MSI_NR,
+					    &msi_domain_ops, &msi->chip);
+	if (!msi->domain) {
+		dev_err(&pdev->dev, "failed to create IRQ domain\n");
+		return -ENOMEM;
+	}
+
+	/* Two irqs are for MSI, but they are also used for non-MSI irqs */
+	err = devm_request_irq(&pdev->dev, msi->irq1, rcar_pcie_msi_irq,
+			       IRQF_SHARED, rcar_msi_irq_chip.name, pcie);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to request IRQ: %d\n", err);
+		goto err;
+	}
+
+	err = devm_request_irq(&pdev->dev, msi->irq2, rcar_pcie_msi_irq,
+			       IRQF_SHARED, rcar_msi_irq_chip.name, pcie);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to request IRQ: %d\n", err);
+		goto err;
+	}
+
+	/* setup MSI data target */
+	msi->pages = __get_free_pages(GFP_KERNEL, 0);
+	base = virt_to_phys((void *)msi->pages);
+
+	pci_write_reg(pcie, base | MSIFE, PCIEMSIALR);
+	pci_write_reg(pcie, 0, PCIEMSIAUR);
+
+	/* enable all MSI interrupts */
+	pci_write_reg(pcie, 0xffffffff, PCIEMSIIER);
+
+	return 0;
+
+err:
+	irq_domain_remove(msi->domain);
+	return err;
+}
+
 static int rcar_pcie_get_resources(struct platform_device *pdev,
 				   struct rcar_pcie *pcie)
 {
 	struct resource res;
-	int err;
+	int err, i;
 
 	err = of_address_to_resource(pdev->dev.of_node, 0, &res);
 	if (err)
@@ -559,6 +773,22 @@ static int rcar_pcie_get_resources(struct platform_device *pdev,
 	if (err)
 		goto err_map_reg;
 
+	i = irq_of_parse_and_map(pdev->dev.of_node, 0);
+	if (i < 0) {
+		dev_err(pcie->dev, "cannot get platform resources for msi interrupt\n");
+		err = -ENOENT;
+		goto err_map_reg;
+	}
+	pcie->msi.irq1 = i;
+
+	i = irq_of_parse_and_map(pdev->dev.of_node, 1);
+	if (i < 0) {
+		dev_err(pcie->dev, "cannot get platform resources for msi interrupt\n");
+		err = -ENOENT;
+		goto err_map_reg;
+	}
+	pcie->msi.irq2 = i;
+
 	pcie->base = devm_ioremap_resource(&pdev->dev, &res);
 	if (IS_ERR(pcie->base)) {
 		err = PTR_ERR(pcie->base);
@@ -732,6 +962,16 @@ static int rcar_pcie_probe(struct platform_device *pdev)
 	 if (err)
 		return err;
 
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		err = rcar_pcie_enable_msi(pcie);
+		if (err < 0) {
+			dev_err(&pdev->dev,
+				"failed to enable MSI support: %d\n",
+				err);
+			return err;
+		}
+	}
+
 	of_id = of_match_device(rcar_pcie_of_match, pcie->dev);
 	if (!of_id || !of_id->data)
 		return -EINVAL;
Signature · 1 line
1.9.1
#10 lore raw

PATCH v8 3/3 dt-bindings: pci: rcar pcie device tree bindings

In reply to Phil Edworthy

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

v8:
- Split interrupt bindings into separate cells
- Add explicit bus number range

v7:
- Change binding description of clocks to 'clock specifiers'

v6:
- Correct DT bindings description for reg and clocks

v5:
- Add PCIe bus clock reference
- Add additional interrupt bindings
- Use dma-ranges property to specify inbound memory regions

---
 Documentation/devicetree/bindings/pci/rcar-pci.txt | 47 ++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci.txt
Documentation/devicetree/bindings/pci/rcar-pci.txt new file +47 -0
new file mode 100644
index 0000000..29d3b98
@@ -0,0 +1,47 @@
+* Renesas RCar PCIe interface
+
+Required properties:
+- compatible: should contain one of the following
+	"renesas,pcie-r8a7779", "renesas,pcie-r8a7790", "renesas,pcie-r8a7791"
+- reg: base address and length of the pcie controller registers.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- bus-range: PCI bus numbers covered
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions.
+- dma-ranges: ranges for the inbound memory regions.
+- interrupts: two interrupt sources for MSI interrupts, followed by interrupt
+	source for hardware related interrupts (e.g. link speed change).
+- #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: clock specifiers for the PCIe controller
+	and PCIe bus clocks.
+- clock-names: from common clock binding: should be "pcie" and "pcie_bus".
+
+Example:
+
+SoC specific DT Entry:
+
+	pcie: pcie at fe000000 {
+		compatible = "renesas,pcie-r8a7791";
+		reg = <0 0xfe000000 0 0x80000>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		bus-range = <0x00 0xff>;
+		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>;
+		dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 0x40000000
+			      0x42000000 2 0x00000000 2 0x00000000 0 0x40000000>;
+		interrupts = <0 116 4>, <0 117 4>, <0 118 4>;
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 0>;
+		interrupt-map = <0 0 0 0 &gic 0 116 4>;
+		clocks = <&mstp3_clks R8A7791_CLK_PCIE>, <&pcie_bus_clk>;
+		clock-names = "pcie", "pcie_bus";
+		status = "disabled";
+	};
Signature · 1 line
1.9.1
maintainer
#11 lore raw

v8 cover R-Car Gen2 PCIe host driver

In reply to Phil Edworthy
Quoted 2 lines · Phil Edworthy

This is version 8 of a PCIe Host driver for the R-Car Gen2 devices, i.e. R-Car H2 (r8a7790) and R-Car M2 (r8a7791).

How do we want to handle maintenance of this new file (pcie-rcar.c)? MAINTAINERS currently contains drivers/pci/host/*rcar*, which matches it and says Simon maintains it. If he wants it, I'd like an ack from him before merging it. If not, I'd like to update MAINTAINERS to show who I should look for acks from.

I don't care either way; I just don't know what to do with this right now.

Bjorn

Quoted 57 lines

v8:
- Platform changes removed from this patch set
- Moved header file contents into c file
- Formatting cleaned up
- Remove bus/dev/func range checks for config access
- Add comment about config access serialization
- Made rcar_pcie_setup_window() return void as no errors possible
- Remove unused register definitions
- Removed __init markers to fix section mismatches
- Add explicit bus number range
- Get the root bus nr from config writes instead of sys->busnr
- Use PCI domains
- Removed unused variable in rcar_msi_free()
- Split interrupt bindings into separate cells

v7:
- Change binding description of clocks to 'clock specifiers'

v6:
- Correct DT bindings description for reg and clocks
- Split device and board DT changes
- Add shmobile to subject for shmobile DT patches
- Don't check MSI irq number is valid, as upper level checks this
- Change "Unexpected MSI" msg to debug level
- Reword "Unexpected MSI" comment so that it's one line
- Remove patch that adds HAVE_ARM_ARCH_TIMER to koelsch defconfig as not needed

v5:
- Use module_platform_driver instead of subsys_initcall
- Use the of_device_id data field for HW init function
- Init hw_pci struct in declaration
- Renesas SoC compatible string has peripheral before device name
- Add PCIe bus clock reference
- Use dma-ranges property to specify inbound memory regions
- Support multiple IO windows and correct resources
- Return IRQ_NONE from MSI isr when there is no pending MSI
- Add additional interrupt bindings

v4:
- Use runtime PM properly

Phil Edworthy (3):
PCI: host: rcar: Add Renesas R-Car PCIe driver
PCI: host: rcar: Add MSI support
dt-bindings: pci: rcar pcie device tree bindings

 Documentation/devicetree/bindings/pci/rcar-pci.txt |   47 +
 drivers/pci/host/Kconfig                           |    6 +
 drivers/pci/host/Makefile                          |    1 +
 drivers/pci/host/pcie-rcar.c                       | 1008 ++++++++++++++++++++
 4 files changed, 1062 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci.txt
 create mode 100644 drivers/pci/host/pcie-rcar.c
Signature · 1 line
1.9.1
#12 lore raw

v8 cover R-Car Gen2 PCIe host driver

Quoted 11 lines · Bjorn Helgaas
Quoted 2 lines · Phil Edworthy

This is version 8 of a PCIe Host driver for the R-Car Gen2 devices, i.e. R-Car H2 (r8a7790) and R-Car M2 (r8a7791).

How do we want to handle maintenance of this new file (pcie-rcar.c)? MAINTAINERS currently contains drivers/pci/host/*rcar*, which matches it and says Simon maintains it. If he wants it, I'd like an ack from him before merging it. If not, I'd like to update MAINTAINERS to show who I should look for acks from.

I don't care either way; I just don't know what to do with this right now.

Hi Bjorn,

I'm happy for the maintenance role to default to me.

Quoted 64 lines

Bjorn

Quoted 46 lines

v8:
- Platform changes removed from this patch set
- Moved header file contents into c file
- Formatting cleaned up
- Remove bus/dev/func range checks for config access
- Add comment about config access serialization
- Made rcar_pcie_setup_window() return void as no errors possible
- Remove unused register definitions
- Removed __init markers to fix section mismatches
- Add explicit bus number range
- Get the root bus nr from config writes instead of sys->busnr
- Use PCI domains
- Removed unused variable in rcar_msi_free()
- Split interrupt bindings into separate cells

v7:
- Change binding description of clocks to 'clock specifiers'

v6:
- Correct DT bindings description for reg and clocks
- Split device and board DT changes
- Add shmobile to subject for shmobile DT patches
- Don't check MSI irq number is valid, as upper level checks this
- Change "Unexpected MSI" msg to debug level
- Reword "Unexpected MSI" comment so that it's one line
- Remove patch that adds HAVE_ARM_ARCH_TIMER to koelsch defconfig as not needed

v5:
- Use module_platform_driver instead of subsys_initcall
- Use the of_device_id data field for HW init function
- Init hw_pci struct in declaration
- Renesas SoC compatible string has peripheral before device name
- Add PCIe bus clock reference
- Use dma-ranges property to specify inbound memory regions
- Support multiple IO windows and correct resources
- Return IRQ_NONE from MSI isr when there is no pending MSI
- Add additional interrupt bindings

v4:
- Use runtime PM properly

Phil Edworthy (3):
PCI: host: rcar: Add Renesas R-Car PCIe driver
PCI: host: rcar: Add MSI support
dt-bindings: pci: rcar pcie device tree bindings

Quoted 11 lines
 Documentation/devicetree/bindings/pci/rcar-pci.txt |   47 +
 drivers/pci/host/Kconfig                           |    6 +
 drivers/pci/host/Makefile                          |    1 +
 drivers/pci/host/pcie-rcar.c                       | 1008 ++++++++++++++++++++
 4 files changed, 1062 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci.txt
 create mode 100644 drivers/pci/host/pcie-rcar.c
Signature · 1 line
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#13 lore raw

v8 cover R-Car Gen2 PCIe host driver

In reply to Phil Edworthy
Quoted 49 lines · Phil Edworthy

This is version 8 of a PCIe Host driver for the R-Car Gen2 devices, i.e. R-Car H2 (r8a7790) and R-Car M2 (r8a7791).

v8:
- Platform changes removed from this patch set
- Moved header file contents into c file
- Formatting cleaned up
- Remove bus/dev/func range checks for config access
- Add comment about config access serialization
- Made rcar_pcie_setup_window() return void as no errors possible
- Remove unused register definitions
- Removed __init markers to fix section mismatches
- Add explicit bus number range
- Get the root bus nr from config writes instead of sys->busnr
- Use PCI domains
- Removed unused variable in rcar_msi_free()
- Split interrupt bindings into separate cells

v7:
- Change binding description of clocks to 'clock specifiers'

v6:
- Correct DT bindings description for reg and clocks
- Split device and board DT changes
- Add shmobile to subject for shmobile DT patches
- Don't check MSI irq number is valid, as upper level checks this
- Change "Unexpected MSI" msg to debug level
- Reword "Unexpected MSI" comment so that it's one line
- Remove patch that adds HAVE_ARM_ARCH_TIMER to koelsch defconfig as not needed

v5:
- Use module_platform_driver instead of subsys_initcall
- Use the of_device_id data field for HW init function
- Init hw_pci struct in declaration
- Renesas SoC compatible string has peripheral before device name
- Add PCIe bus clock reference
- Use dma-ranges property to specify inbound memory regions
- Support multiple IO windows and correct resources
- Return IRQ_NONE from MSI isr when there is no pending MSI
- Add additional interrupt bindings

v4:
- Use runtime PM properly

Phil Edworthy (3):
PCI: host: rcar: Add Renesas R-Car PCIe driver
PCI: host: rcar: Add MSI support
dt-bindings: pci: rcar pcie device tree bindings

Quoted 7 lines
 Documentation/devicetree/bindings/pci/rcar-pci.txt |   47 +
 drivers/pci/host/Kconfig                           |    6 +
 drivers/pci/host/Makefile                          |    1 +
 drivers/pci/host/pcie-rcar.c                       | 1008 ++++++++++++++++++++
 4 files changed, 1062 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci.txt
 create mode 100644 drivers/pci/host/pcie-rcar.c

Applied with Simon's ack to pci/host-rcar for v3.16, thanks!

Bjorn

#14 lore raw

v8 cover R-Car Gen2 PCIe host driver

Quoted 58 lines · Bjorn Helgaas
Quoted 49 lines · Phil Edworthy

This is version 8 of a PCIe Host driver for the R-Car Gen2 devices, i.e. R-Car H2 (r8a7790) and R-Car M2 (r8a7791).

v8:
- Platform changes removed from this patch set
- Moved header file contents into c file
- Formatting cleaned up
- Remove bus/dev/func range checks for config access
- Add comment about config access serialization
- Made rcar_pcie_setup_window() return void as no errors possible
- Remove unused register definitions
- Removed __init markers to fix section mismatches
- Add explicit bus number range
- Get the root bus nr from config writes instead of sys->busnr
- Use PCI domains
- Removed unused variable in rcar_msi_free()
- Split interrupt bindings into separate cells

v7:
- Change binding description of clocks to 'clock specifiers'

v6:
- Correct DT bindings description for reg and clocks
- Split device and board DT changes
- Add shmobile to subject for shmobile DT patches
- Don't check MSI irq number is valid, as upper level checks this
- Change "Unexpected MSI" msg to debug level
- Reword "Unexpected MSI" comment so that it's one line
- Remove patch that adds HAVE_ARM_ARCH_TIMER to koelsch defconfig as not needed

v5:
- Use module_platform_driver instead of subsys_initcall
- Use the of_device_id data field for HW init function
- Init hw_pci struct in declaration
- Renesas SoC compatible string has peripheral before device name
- Add PCIe bus clock reference
- Use dma-ranges property to specify inbound memory regions
- Support multiple IO windows and correct resources
- Return IRQ_NONE from MSI isr when there is no pending MSI
- Add additional interrupt bindings

v4:
- Use runtime PM properly

Phil Edworthy (3):
PCI: host: rcar: Add Renesas R-Car PCIe driver
PCI: host: rcar: Add MSI support
dt-bindings: pci: rcar pcie device tree bindings

Quoted 7 lines
 Documentation/devicetree/bindings/pci/rcar-pci.txt |   47 +
 drivers/pci/host/Kconfig                           |    6 +
 drivers/pci/host/Makefile                          |    1 +
 drivers/pci/host/pcie-rcar.c                       | 1008 ++++++++++++++++++++
 4 files changed, 1062 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci.txt
 create mode 100644 drivers/pci/host/pcie-rcar.c
Quoted 1 line

Applied with Simon's ack to pci/host-rcar for v3.16, thanks!

Thanks!