2014-07-21 17:01:34

by Karicheri, Muralidharan

[permalink] [raw]
Subject: [PATCH v7 0/5] Add Keystone PCIe controller driver

This patch series add PCIe controller driver for keystone SoCs. This is
based on v4 of the series posted to the mailing list. Keystone PCI controller
is based on version 3.65 of the DW hardware. This driver uses the DW core
functions to implement the PCI controller driver for keystone.

Testing:
=======

Testing of the driver is done on TI's K2HK EVM inserted to a Elma blu!eco
MicroTCA chassis AMC slot with JumpGen SEM-200 AMC SATA Storage and Dual
Ethernet Card Express inserted on another AMC slot. The e1000e driver available
on intel's website is patched to the kernel source tree and build. e1000e.ko
is dynamically loaded and executed ping and iperf tests to test the
functionality.

Thanks to all of you who have contributed by reviewing and offering valuable
comments. If you want me to add your "Reviewed-by", please let me know so that
I can add it to the commit log and re-send. Patch 1-3 has Acks from maintainers
and I believe this can be merged to upstream branch. Patch 4 is waiting for
Ack from DT maintainer. Please provide the same at the earliest.

Thanks

Signed-off-by: Murali Karicheri <[email protected]>

CC: Russell King <[email protected]>
CC: Grant Likely <[email protected]>
CC: Rob Herring <[email protected]>
CC: Mohit Kumar <[email protected]>
CC: Jingoo Han <[email protected]>
CC: Bjorn Helgaas <[email protected]>
CC: Pratyush Anand <[email protected]>
CC: Richard Zhu <[email protected]>
CC: Kishon Vijay Abraham I <[email protected]>
CC: Marek Vasut <[email protected]>
CC: Arnd Bergmann <[email protected]>
CC: Pawel Moll <[email protected]>
CC: Mark Rutland <[email protected]>
CC: Ian Campbell <[email protected]>
CC: Kumar Gala <[email protected]>
CC: Randy Dunlap <[email protected]>
CC: Grant Likely <[email protected]>

Changelog:

v7
- Removed ti,enable_linktrain DT property. The driver now check if
the link is up and then retrain the link if not already up. This
is as per comment from DT maintainer.
- Rebased to upstream kernel v3.16-rc6

v6
- Added Ack from Maintainer for patch 3. Patch 1-3 now have the
Acks from maintainer and can be merged to upstream branch.
Patch 4 is waiting ack from DT maintainer.
v5
- Rebased to upstream kernel v3.16-rc5
- Reworked Patch 3/6 and 4/6 into a single patch based on
maintainer comment to use a API callback model to support
the v3.65 h/w.
v4
- Addressed comments against 5/5.
- Added a patch 6/6 for Maintainer information
- Added couple of Acked-By:
- Rebased to latest Linux 3.16-rc4
v3
- DW application register handling code is now part of
Keystone PCI driver. RFC had this code part of Keystone
PCI driver, then V1 moved this to a separate file to
re-use on other platforms that uses this version of the
DW h/w. Then based on comments against v2, this is moved
back to Keystone driver.
- Keystone SerDes phy driver is removed from this series so that
this can be merged independent of that patch
- added msi_set_irq()/clear_irq() API's to support Keystone

V2
- Split the designware pcie enhancement patch to multiple
patches based on functionality added
- Remove the quirk code and add a patch to fix mps/mrss
tuning for ARM. Use kernel command line parameter
pci=pcie_bus_perf to work with Keystone PCI Controller.
Following patch addressed this.
[PATCH v1] ARM: pci: add call to pcie_bus_configure_settings()
- Add documentation for device tree bindings
- Add separate interrupt controller nodes for MSI and Legacy
IRQs and use irq map for legacy IRQ
- Use compatibility to identify v3.65 version of the DW hardware
and use it to customize the designware common code.
- Use reg property for configuration space instead of range
- Other minor updates based on code inspection.

V1
- Add an interrupt controller node for Legacy irq chip and use
interrupt map/map-mask property to map legacy IRQs A/B/C/D
- Add a Phy driver to replace the original serdes driver
- Move common application register handling code to a separate
file to allow re-use across other platforms that use older
DW PCIe h/w
- PCI quirk for maximum read request size. Check and override only
if the maximum is higher than what controller can handle.
- Converted to a module platform driver.

Murali Karicheri (5):
PCI: designware: add rd[wr]_other_conf API
PCI: designware: refactor MSI code to work with v3.65 dw hardware
PCI: designware: enhance dw_pcie_host_init() to support v3.65 DW
hardware
PCI: add PCI controller for keystone PCIe h/w
PCI: keystone: Update maintainer information

.../devicetree/bindings/pci/pci-keystone.txt | 68 +++
MAINTAINERS | 7 +
drivers/pci/host/Kconfig | 5 +
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pci-keystone-dw.c | 521 ++++++++++++++++++++
drivers/pci/host/pci-keystone.c | 386 +++++++++++++++
drivers/pci/host/pci-keystone.h | 58 +++
drivers/pci/host/pcie-designware.c | 116 +++--
drivers/pci/host/pcie-designware.h | 9 +
9 files changed, 1135 insertions(+), 36 deletions(-)
create mode 100644 Documentation/devicetree/bindings/pci/pci-keystone.txt
create mode 100644 drivers/pci/host/pci-keystone-dw.c
create mode 100644 drivers/pci/host/pci-keystone.c
create mode 100644 drivers/pci/host/pci-keystone.h

--
1.7.9.5


2014-07-21 17:00:07

by Karicheri, Muralidharan

[permalink] [raw]
Subject: [PATCH v7 1/5] PCI: designware: add rd[wr]_other_conf API

v3.65 version of the designware h/w, requires application space
registers to be configured to access the remote EP config space.
To support this, add rd[wr]_other_conf API in the pcie_host_opts

Signed-off-by: Murali Karicheri <[email protected]>
Reviewed-by: Pratyush Anand <[email protected]>
Acked-by: Mohit Kumar <[email protected]>
Acked-by: Jingoo Han <[email protected]>
Acked-by: Santosh Shilimkar <[email protected]>

CC: Russell King <[email protected]>
CC: Grant Likely <[email protected]>
CC: Rob Herring <[email protected]>
CC: Bjorn Helgaas <[email protected]>
CC: Richard Zhu <[email protected]>
CC: Kishon Vijay Abraham I <[email protected]>
CC: Marek Vasut <[email protected]>
CC: Arnd Bergmann <[email protected]>
CC: Pawel Moll <[email protected]>
CC: Mark Rutland <[email protected]>
CC: Ian Campbell <[email protected]>
CC: Kumar Gala <[email protected]>
CC: Randy Dunlap <[email protected]>
CC: Grant Likely <[email protected]>
---
drivers/pci/host/pcie-designware.c | 12 ++++++++++--
drivers/pci/host/pcie-designware.h | 4 ++++
2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 1eaf4df..d8f3af7 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -656,7 +656,11 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where,
}

if (bus->number != pp->root_bus_nr)
- ret = dw_pcie_rd_other_conf(pp, bus, devfn,
+ if (pp->ops->rd_other_conf)
+ ret = pp->ops->rd_other_conf(pp, bus, devfn,
+ where, size, val);
+ else
+ ret = dw_pcie_rd_other_conf(pp, bus, devfn,
where, size, val);
else
ret = dw_pcie_rd_own_conf(pp, where, size, val);
@@ -679,7 +683,11 @@ static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
return PCIBIOS_DEVICE_NOT_FOUND;

if (bus->number != pp->root_bus_nr)
- ret = dw_pcie_wr_other_conf(pp, bus, devfn,
+ if (pp->ops->wr_other_conf)
+ ret = pp->ops->wr_other_conf(pp, bus, devfn,
+ where, size, val);
+ else
+ ret = dw_pcie_wr_other_conf(pp, bus, devfn,
where, size, val);
else
ret = dw_pcie_wr_own_conf(pp, where, size, val);
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index 77f592f..8121901 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -61,6 +61,10 @@ struct pcie_host_ops {
u32 val, void __iomem *dbi_base);
int (*rd_own_conf)(struct pcie_port *pp, int where, int size, u32 *val);
int (*wr_own_conf)(struct pcie_port *pp, int where, int size, u32 val);
+ int (*rd_other_conf)(struct pcie_port *pp, struct pci_bus *bus,
+ unsigned int devfn, int where, int size, u32 *val);
+ int (*wr_other_conf)(struct pcie_port *pp, struct pci_bus *bus,
+ unsigned int devfn, int where, int size, u32 val);
int (*link_up)(struct pcie_port *pp);
void (*host_init)(struct pcie_port *pp);
};
--
1.7.9.5

2014-07-21 17:00:15

by Karicheri, Muralidharan

[permalink] [raw]
Subject: [PATCH v7 5/5] PCI: keystone: Update maintainer information

Update the MAINTAINERS file for the keystone PCIe driver

Signed-off-by: Murali Karicheri <[email protected]>

CC: Santosh Shilimkar <[email protected]>
CC: Russell King <[email protected]>
CC: Grant Likely <[email protected]>
CC: Rob Herring <[email protected]>
CC: Mohit Kumar <[email protected]>
CC: Jingoo Han <[email protected]>
CC: Bjorn Helgaas <[email protected]>
CC: Pratyush Anand <[email protected]>
CC: Richard Zhu <[email protected]>
CC: Kishon Vijay Abraham I <[email protected]>
CC: Marek Vasut <[email protected]>
CC: Arnd Bergmann <[email protected]>
CC: Pawel Moll <[email protected]>
CC: Mark Rutland <[email protected]>
CC: Ian Campbell <[email protected]>
CC: Kumar Gala <[email protected]>
CC: Randy Dunlap <[email protected]>
CC: Grant Likely <[email protected]>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 61a8f48..919b6b1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6805,6 +6805,13 @@ L: [email protected] (moderated for non-subscribers)
S: Maintained
F: drivers/pci/host/*imx6*

+PCI DRIVER FOR keystone
+M: Murali Karicheri <[email protected]>
+L: [email protected]
+L: [email protected] (moderated for non-subscribers)
+S: Maintained
+F: drivers/pci/host/*keystone*
+
PCI DRIVER FOR MVEBU (Marvell Armada 370 and Armada XP SOC support)
M: Thomas Petazzoni <[email protected]>
M: Jason Cooper <[email protected]>
--
1.7.9.5

2014-07-21 17:00:32

by Karicheri, Muralidharan

[permalink] [raw]
Subject: [PATCH v7 3/5] PCI: designware: enhance dw_pcie_host_init() to support v3.65 DW hardware

keystone PCI controller is based on v3.65 designware hardware. This
version differs from newer versions of the hardware in few functional
areas discussed below that makes it necessary to change dw_pcie_host_init()
to support v3.65 based PCI controller.

1. No support for ATU port. So any ATU specific resource handling code
is to be bypassed for v3.65 h/w.
2. MSI controller uses Application space to implement MSI and 32 MSI
interrupts are multiplexed over 8 IRQs to the host. Hence the code
to process MSI IRQ needs to be different. This patch allows platform
driver to provide its own irq_domain_ops ptr to irq_domain_add_linear()
through an API callback from the designware core driver.
3. MSI interrupt generation requires EP to write to the RC's application
register. So enhance the driver to allow setup of inbound access to
MSI irq register as a post scan bus API callback.

Signed-off-by: Murali Karicheri <[email protected]>
Reviewed-by: Pratyush Anand <[email protected]>
Acked-by: Mohit KUMAR <[email protected]>

CC: Santosh Shilimkar <[email protected]>
CC: Russell King <[email protected]>
CC: Grant Likely <[email protected]>
CC: Rob Herring <[email protected]>
CC: Jingoo Han <[email protected]>
CC: Bjorn Helgaas <[email protected]>
CC: Richard Zhu <[email protected]>
CC: Kishon Vijay Abraham I <[email protected]>
CC: Marek Vasut <[email protected]>
CC: Arnd Bergmann <[email protected]>
CC: Pawel Moll <[email protected]>
CC: Mark Rutland <[email protected]>
CC: Ian Campbell <[email protected]>
CC: Kumar Gala <[email protected]>
CC: Randy Dunlap <[email protected]>
CC: Grant Likely <[email protected]>
---
drivers/pci/host/pcie-designware.c | 54 +++++++++++++++++++++++-------------
drivers/pci/host/pcie-designware.h | 2 ++
2 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 905941c..35bb4af 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -420,8 +420,8 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
struct device_node *np = pp->dev->of_node;
struct of_pci_range range;
struct of_pci_range_parser parser;
+ int i, ret;
u32 val;
- int i;

if (of_pci_range_parser_init(&parser, np)) {
dev_err(pp->dev, "missing ranges property\n");
@@ -467,21 +467,26 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
}
}

- pp->cfg0_base = pp->cfg.start;
- pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
pp->mem_base = pp->mem.start;

- pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base,
- pp->config.cfg0_size);
if (!pp->va_cfg0_base) {
- dev_err(pp->dev, "error with ioremap in function\n");
- return -ENOMEM;
+ pp->cfg0_base = pp->cfg.start;
+ pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base,
+ pp->config.cfg0_size);
+ if (!pp->va_cfg0_base) {
+ dev_err(pp->dev, "error with ioremap in function\n");
+ return -ENOMEM;
+ }
}
- pp->va_cfg1_base = devm_ioremap(pp->dev, pp->cfg1_base,
- pp->config.cfg1_size);
+
if (!pp->va_cfg1_base) {
- dev_err(pp->dev, "error with ioremap\n");
- return -ENOMEM;
+ pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
+ pp->va_cfg1_base = devm_ioremap(pp->dev, pp->cfg1_base,
+ pp->config.cfg1_size);
+ if (!pp->va_cfg1_base) {
+ dev_err(pp->dev, "error with ioremap\n");
+ return -ENOMEM;
+ }
}

if (of_property_read_u32(np, "num-lanes", &pp->lanes)) {
@@ -490,16 +495,22 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
}

if (IS_ENABLED(CONFIG_PCI_MSI)) {
- pp->irq_domain = irq_domain_add_linear(pp->dev->of_node,
- MAX_MSI_IRQS, &msi_domain_ops,
- &dw_pcie_msi_chip);
- if (!pp->irq_domain) {
- dev_err(pp->dev, "irq domain init failed\n");
- return -ENXIO;
- }
+ if (!pp->ops->msi_host_init) {
+ pp->irq_domain = irq_domain_add_linear(pp->dev->of_node,
+ MAX_MSI_IRQS, &msi_domain_ops,
+ &dw_pcie_msi_chip);
+ if (!pp->irq_domain) {
+ dev_err(pp->dev, "irq domain init failed\n");
+ return -ENXIO;
+ }

- for (i = 0; i < MAX_MSI_IRQS; i++)
- irq_create_mapping(pp->irq_domain, i);
+ for (i = 0; i < MAX_MSI_IRQS; i++)
+ irq_create_mapping(pp->irq_domain, i);
+ } else {
+ ret = pp->ops->msi_host_init(pp, &dw_pcie_msi_chip);
+ if (ret < 0)
+ return ret;
+ }
}

if (pp->ops->host_init)
@@ -759,6 +770,9 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
BUG();
}

+ if (bus && pp->ops->scan_bus)
+ pp->ops->scan_bus(pp);
+
return bus;
}

diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index 387f69e..080c649 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -70,6 +70,8 @@ struct pcie_host_ops {
void (*msi_set_irq)(struct pcie_port *pp, int irq);
void (*msi_clear_irq)(struct pcie_port *pp, int irq);
u32 (*get_msi_data)(struct pcie_port *pp);
+ void (*scan_bus)(struct pcie_port *pp);
+ int (*msi_host_init)(struct pcie_port *pp, struct msi_chip *chip);
};

int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val);
--
1.7.9.5

2014-07-21 17:01:35

by Karicheri, Muralidharan

[permalink] [raw]
Subject: [PATCH v7 2/5] PCI: designware: refactor MSI code to work with v3.65 dw hardware

Keystone PCI controller is based on v3.65 version of the DW
PCI h/w that implements MSI controller registers in application
space compared to the newer version. This requires updates to
the DW core API to support the PCI controller driver based on
this old DW hardware. Add msi_irq_set()/clear() API functions to
allow Set/Clear MSI IRQ enable bit in the application register.
Also the old h/w uses MSI_IRQ register in application register
space to raise MSI IRQ to the RC from EP. Current code uses the
standard mechanism as per PCI spec. So add another API get_msi_data()
to get the address of this register so that common code can be
re-used on old h/w.

Signed-off-by: Murali Karicheri <[email protected]>
Reviewed-by: Pratyush Anand <[email protected]>
Acked-by: Mohit Kumar <[email protected]>
Acked-by: Jingoo Han <[email protected]>
Acked-by: Santosh Shilimkar <[email protected]>

CC: Russell King <[email protected]>
CC: Grant Likely <[email protected]>
CC: Rob Herring <[email protected]>
CC: Bjorn Helgaas <[email protected]>
CC: Richard Zhu <[email protected]>
CC: Kishon Vijay Abraham I <[email protected]>
CC: Marek Vasut <[email protected]>
CC: Arnd Bergmann <[email protected]>
CC: Pawel Moll <[email protected]>
CC: Mark Rutland <[email protected]>
CC: Ian Campbell <[email protected]>
CC: Kumar Gala <[email protected]>
CC: Randy Dunlap <[email protected]>
CC: Grant Likely <[email protected]>
---
drivers/pci/host/pcie-designware.c | 50 ++++++++++++++++++++++++++----------
drivers/pci/host/pcie-designware.h | 3 +++
2 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index d8f3af7..905941c 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -217,27 +217,47 @@ static int find_valid_pos0(struct pcie_port *pp, int msgvec, int pos, int *pos0)
return 0;
}

+static void dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
+{
+ unsigned int res, bit, val;
+
+ res = (irq / 32) * 12;
+ bit = irq % 32;
+ dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val);
+ val &= ~(1 << bit);
+ dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val);
+}
+
static void clear_irq_range(struct pcie_port *pp, unsigned int irq_base,
unsigned int nvec, unsigned int pos)
{
- unsigned int i, res, bit, val;
+ unsigned int i;

for (i = 0; i < nvec; i++) {
irq_set_msi_desc_off(irq_base, i, NULL);
clear_bit(pos + i, pp->msi_irq_in_use);
/* Disable corresponding interrupt on MSI controller */
- res = ((pos + i) / 32) * 12;
- bit = (pos + i) % 32;
- dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val);
- val &= ~(1 << bit);
- dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val);
+ if (pp->ops->msi_clear_irq)
+ pp->ops->msi_clear_irq(pp, pos + i);
+ else
+ dw_pcie_msi_clear_irq(pp, pos + i);
}
}

+static void dw_pcie_msi_set_irq(struct pcie_port *pp, int irq)
+{
+ unsigned int res, bit, val;
+
+ res = (irq / 32) * 12;
+ bit = irq % 32;
+ dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val);
+ val |= 1 << bit;
+ dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val);
+}
+
static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
{
- int res, bit, irq, pos0, pos1, i;
- u32 val;
+ int irq, pos0, pos1, i;
struct pcie_port *pp = sys_to_pcie(desc->dev->bus->sysdata);

if (!pp) {
@@ -281,11 +301,10 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos)
}
set_bit(pos0 + i, pp->msi_irq_in_use);
/*Enable corresponding interrupt in MSI interrupt controller */
- res = ((pos0 + i) / 32) * 12;
- bit = (pos0 + i) % 32;
- dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val);
- val |= 1 << bit;
- dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val);
+ if (pp->ops->msi_set_irq)
+ pp->ops->msi_set_irq(pp, pos0 + i);
+ else
+ dw_pcie_msi_set_irq(pp, pos0 + i);
}

*pos = pos0;
@@ -353,7 +372,10 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
*/
desc->msi_attrib.multiple = msgvec;

- msg.address_lo = virt_to_phys((void *)pp->msi_data);
+ if (pp->ops->get_msi_data)
+ msg.address_lo = pp->ops->get_msi_data(pp);
+ else
+ msg.address_lo = virt_to_phys((void *)pp->msi_data);
msg.address_hi = 0x0;
msg.data = pos;
write_msi_msg(irq, &msg);
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index 8121901..387f69e 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -67,6 +67,9 @@ struct pcie_host_ops {
unsigned int devfn, int where, int size, u32 val);
int (*link_up)(struct pcie_port *pp);
void (*host_init)(struct pcie_port *pp);
+ void (*msi_set_irq)(struct pcie_port *pp, int irq);
+ void (*msi_clear_irq)(struct pcie_port *pp, int irq);
+ u32 (*get_msi_data)(struct pcie_port *pp);
};

int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val);
--
1.7.9.5

2014-07-21 17:01:33

by Karicheri, Muralidharan

[permalink] [raw]
Subject: [PATCH v7 4/5] PCI: add PCI controller for keystone PCIe h/w

keystone PCIe controller is based on v3.65 version of the
designware h/w. Main differences are
1. No ATU support
2. Legacy and MSI irq functions are implemented in
application register space
3. MSI interrupts are multiplexed over 8 IRQ lines to the Host
side.
All of the Application register space handing code are organized into
pci-keystone-dw.c and the functions are called from pci-keystone.c
to implement PCI controller driver. Also add necessary DT documentation
for the driver.

Signed-off-by: Murali Karicheri <[email protected]>
Acked-by: Santosh Shilimkar <[email protected]>

CC: Santosh Shilimkar <[email protected]>
CC: Russell King <[email protected]>
CC: Grant Likely <[email protected]>
CC: Rob Herring <[email protected]>
CC: Jingoo Han <[email protected]>
CC: Bjorn Helgaas <[email protected]>
CC: Richard Zhu <[email protected]>
CC: Kishon Vijay Abraham I <[email protected]>
CC: Marek Vasut <[email protected]>
CC: Arnd Bergmann <[email protected]>
CC: Pawel Moll <[email protected]>
CC: Mark Rutland <[email protected]>
CC: Ian Campbell <[email protected]>
CC: Kumar Gala <[email protected]>
CC: Randy Dunlap <[email protected]>
CC: Grant Likely <[email protected]>
---
.../devicetree/bindings/pci/pci-keystone.txt | 68 +++
drivers/pci/host/Kconfig | 5 +
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pci-keystone-dw.c | 521 ++++++++++++++++++++
drivers/pci/host/pci-keystone.c | 386 +++++++++++++++
drivers/pci/host/pci-keystone.h | 58 +++
6 files changed, 1039 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/pci-keystone.txt
create mode 100644 drivers/pci/host/pci-keystone-dw.c
create mode 100644 drivers/pci/host/pci-keystone.c
create mode 100644 drivers/pci/host/pci-keystone.h

diff --git a/Documentation/devicetree/bindings/pci/pci-keystone.txt b/Documentation/devicetree/bindings/pci/pci-keystone.txt
new file mode 100644
index 0000000..1bd0476
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt
@@ -0,0 +1,68 @@
+TI Keystone PCIe interface
+
+Keystone PCI host Controller is based on Designware PCI h/w version 3.65.
+It shares common functions with PCIE Designware core driver and inherit
+common properties defined in
+Documentation/devicetree/bindings/pci/designware-pci.txt
+
+Please refer to Documentation/devicetree/bindings/pci/designware-pci.txt
+for the details of designware DT bindings. Additional properties are
+described here as well propeties that are not applicable.
+
+Required Properties:-
+
+compatibility: "ti,keystone-pcie"
+reg: index 1 is the base address and length of DW application registers.
+ index 2 is the base address and length of PCI mode configuration
+ register.
+ index 3 is the base address and length of PCI device ID register.
+
+pcie_msi_intc : Interrupt controller device node for MSI irq chip
+ interrupt-cells: should be set to 1
+ interrupt-parent: Parent interrupt controller phandle
+ interrupts: GIC interrupt lines connected to PCI MSI interrupt lines
+
+ Example:
+ pcie_msi_intc: msi-interrupt-controller {
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SPI 30 IRQ_TYPE_EDGE_RISING>,
+ <GIC_SPI 31 IRQ_TYPE_EDGE_RISING>,
+ <GIC_SPI 32 IRQ_TYPE_EDGE_RISING>,
+ <GIC_SPI 33 IRQ_TYPE_EDGE_RISING>,
+ <GIC_SPI 34 IRQ_TYPE_EDGE_RISING>,
+ <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>,
+ <GIC_SPI 36 IRQ_TYPE_EDGE_RISING>,
+ <GIC_SPI 37 IRQ_TYPE_EDGE_RISING>;
+ };
+
+pcie_intc: Interrupt controller device node for Legacy irq chip
+ interrupt-cells: should be set to 1
+ interrupt-parent: Parent interrupt controller phandle
+ interrupts: GIC interrupt lines connected to PCI Legacy interrupt lines
+
+ Example:
+ pcie_intc: legacy-interrupt-controller {
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SPI 26 IRQ_TYPE_EDGE_RISING>,
+ <GIC_SPI 27 IRQ_TYPE_EDGE_RISING>,
+ <GIC_SPI 28 IRQ_TYPE_EDGE_RISING>,
+ <GIC_SPI 29 IRQ_TYPE_EDGE_RISING>;
+ };
+
+Optional properties:-
+ phys: phandle to Generic Keystone SerDes phy for PCI
+ phy-names: name of the Generic Keystine SerDes phy for PCI
+ - If boot loader already does PCI link establishment, then phys and
+ phy-names shouldn't be present.
+
+Designware DT Properties not applicable for Keystone PCI
+
+1. pcie_bus clock-names not used. Instead, a phandle to phys is used.
+
+Note for PCI driver usage
+=========================
+Driver requires pci=pcie_bus_perf in the bootargs for proper functioning.
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 21df477..f8bc475 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -46,4 +46,9 @@ config PCI_HOST_GENERIC
Say Y here if you want to support a simple generic PCI host
controller, such as the one emulated by kvmtool.

+config PCI_KEYSTONE
+ bool "TI Keystone PCIe controller"
+ depends on ARCH_KEYSTONE
+ select PCIE_DW
+ select PCIEPORTBUS
endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 611ba4b..d1b6ce1 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -6,3 +6,4 @@ 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
obj-$(CONFIG_PCI_HOST_GENERIC) += pci-host-generic.o
+obj-$(CONFIG_PCI_KEYSTONE) += pci-keystone-dw.o pci-keystone.o
diff --git a/drivers/pci/host/pci-keystone-dw.c b/drivers/pci/host/pci-keystone-dw.c
new file mode 100644
index 0000000..701acb4
--- /dev/null
+++ b/drivers/pci/host/pci-keystone-dw.c
@@ -0,0 +1,521 @@
+/*
+ * Designware application register space functions for Keystone PCI controller
+ *
+ * Copyright (C) 2013-2014 Texas Instruments., Ltd.
+ * http://www.ti.com
+ *
+ * Author: Murali Karicheri <[email protected]>
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+
+#include "pcie-designware.h"
+#include "pci-keystone.h"
+
+/* Application register defines */
+#define LTSSM_EN_VAL 1
+#define LTSSM_STATE_MASK 0x1f
+#define LTSSM_STATE_L0 0x11
+#define DBI_CS2_EN_VAL 0x20
+#define OB_XLAT_EN_VAL 2
+
+/* Application registers */
+#define CMD_STATUS 0x004
+#define CFG_SETUP 0x008
+#define OB_SIZE 0x030
+#define CFG_PCIM_WIN_SZ_IDX 3
+#define CFG_PCIM_WIN_CNT 32
+#define SPACE0_REMOTE_CFG_OFFSET 0x1000
+#define OB_OFFSET_INDEX(n) (0x200 + (8 * n))
+#define OB_OFFSET_HI(n) (0x204 + (8 * n))
+
+/* IRQ register defines */
+#define IRQ_EOI 0x050
+#define IRQ_STATUS 0x184
+#define IRQ_ENABLE_SET 0x188
+#define IRQ_ENABLE_CLR 0x18c
+
+#define MSI_IRQ 0x054
+#define MSI0_IRQ_STATUS 0x104
+#define MSI0_IRQ_ENABLE_SET 0x108
+#define MSI0_IRQ_ENABLE_CLR 0x10c
+#define IRQ_STATUS 0x184
+#define MSI_IRQ_OFFSET 4
+
+/* Config space registers */
+#define DEBUG0 0x728
+
+#define to_keystone_pcie(x) container_of(x, struct keystone_pcie, pp)
+
+static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys)
+{
+ return sys->private_data;
+}
+
+static inline void update_reg_offset_bit_pos(u32 offset, u32 *reg_offset,
+ u32 *bit_pos)
+{
+ *reg_offset = offset % 8;
+ *bit_pos = offset >> 3;
+}
+
+u32 ks_dw_pcie_get_msi_data(struct pcie_port *pp)
+{
+ struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
+
+ return ks_pcie->app.start + MSI_IRQ;
+}
+
+void ks_dw_pcie_handle_msi_irq(struct keystone_pcie *ks_pcie , int offset)
+{
+ struct pcie_port *pp = &ks_pcie->pp;
+ u32 pending, vector;
+ int src, virq;
+
+ pending = readl(ks_pcie->va_app_base + MSI0_IRQ_STATUS + (offset << 4));
+ /*
+ * MSI0, Status bit 0-3 shows vectors 0, 8, 16, 24, MSI1 status bit
+ * shows 1, 9, 17, 25 and so forth
+ */
+ for (src = 0; src < 4; src++) {
+ if (BIT(src) & pending) {
+ vector = offset + (src << 3);
+ virq = irq_linear_revmap(pp->irq_domain, vector);
+ dev_dbg(pp->dev,
+ "irq: bit %d, vector %d, virq %d\n",
+ src, vector, virq);
+ generic_handle_irq(virq);
+ }
+ }
+}
+
+static void ks_dw_pcie_msi_irq_ack(struct irq_data *d)
+{
+ u32 offset, reg_offset, bit_pos;
+ struct keystone_pcie *ks_pcie;
+ unsigned int irq = d->irq;
+ struct msi_desc *msi;
+ struct pcie_port *pp;
+
+ msi = irq_get_msi_desc(irq);
+ pp = sys_to_pcie(msi->dev->bus->sysdata);
+ ks_pcie = to_keystone_pcie(pp);
+ offset = irq - irq_linear_revmap(pp->irq_domain, 0);
+ update_reg_offset_bit_pos(offset, &reg_offset, &bit_pos);
+
+ writel(BIT(bit_pos),
+ ks_pcie->va_app_base + MSI0_IRQ_STATUS + (reg_offset << 4));
+ writel(reg_offset + MSI_IRQ_OFFSET, ks_pcie->va_app_base + IRQ_EOI);
+}
+
+void ks_dw_pcie_msi_set_irq(struct pcie_port *pp, int irq)
+{
+ u32 reg_offset, bit_pos;
+ struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
+
+ update_reg_offset_bit_pos(irq, &reg_offset, &bit_pos);
+ writel(BIT(bit_pos),
+ ks_pcie->va_app_base + MSI0_IRQ_ENABLE_SET + (reg_offset << 4));
+}
+
+void ks_dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq)
+{
+ u32 reg_offset, bit_pos;
+ struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
+
+ update_reg_offset_bit_pos(irq, &reg_offset, &bit_pos);
+ writel(BIT(bit_pos),
+ ks_pcie->va_app_base + MSI0_IRQ_ENABLE_CLR + (reg_offset << 4));
+}
+
+static void ks_dw_pcie_msi_irq_mask(struct irq_data *d)
+{
+ struct keystone_pcie *ks_pcie;
+ unsigned int irq = d->irq;
+ struct msi_desc *msi;
+ struct pcie_port *pp;
+ u32 offset;
+
+ msi = irq_get_msi_desc(irq);
+ pp = sys_to_pcie(msi->dev->bus->sysdata);
+ ks_pcie = to_keystone_pcie(pp);
+ offset = irq - irq_linear_revmap(pp->irq_domain, 0);
+
+ /* mask the end point if PVM implemented */
+ if (IS_ENABLED(CONFIG_PCI_MSI)) {
+ if (msi->msi_attrib.maskbit)
+ mask_msi_irq(d);
+ }
+
+ ks_dw_pcie_msi_clear_irq(pp, offset);
+}
+
+static void ks_dw_pcie_msi_irq_unmask(struct irq_data *d)
+{
+ struct keystone_pcie *ks_pcie;
+ unsigned int irq = d->irq;
+ struct msi_desc *msi;
+ struct pcie_port *pp;
+ u32 offset;
+
+ msi = irq_get_msi_desc(irq);
+ pp = sys_to_pcie(msi->dev->bus->sysdata);
+ ks_pcie = to_keystone_pcie(pp);
+ offset = irq - irq_linear_revmap(pp->irq_domain, 0);
+
+ /* mask the end point if PVM implemented */
+ if (IS_ENABLED(CONFIG_PCI_MSI)) {
+ if (msi->msi_attrib.maskbit)
+ unmask_msi_irq(d);
+ }
+
+ ks_dw_pcie_msi_set_irq(pp, offset);
+}
+
+static struct irq_chip ks_dw_pcie_msi_irq_chip = {
+ .name = "Keystone-PCIe-MSI-IRQ",
+ .irq_ack = ks_dw_pcie_msi_irq_ack,
+ .irq_mask = ks_dw_pcie_msi_irq_mask,
+ .irq_unmask = ks_dw_pcie_msi_irq_unmask,
+};
+
+static int ks_dw_pcie_msi_map(struct irq_domain *domain, unsigned int irq,
+ irq_hw_number_t hwirq)
+{
+ irq_set_chip_and_handler(irq, &ks_dw_pcie_msi_irq_chip,
+ handle_level_irq);
+ irq_set_chip_data(irq, domain->host_data);
+ set_irq_flags(irq, IRQF_VALID);
+
+ return 0;
+}
+
+const struct irq_domain_ops ks_dw_pcie_msi_domain_ops = {
+ .map = ks_dw_pcie_msi_map,
+};
+
+int ks_dw_pcie_msi_host_init(struct pcie_port *pp,
+ struct msi_chip *chip)
+{
+ struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
+ int i;
+
+ pp->irq_domain = irq_domain_add_linear(ks_pcie->msi_intc_np,
+ MAX_MSI_IRQS,
+ &ks_dw_pcie_msi_domain_ops,
+ chip);
+ if (!pp->irq_domain) {
+ dev_err(pp->dev, "irq domain init failed\n");
+ return -ENXIO;
+ }
+
+ for (i = 0; i < MAX_MSI_IRQS; i++)
+ irq_create_mapping(pp->irq_domain, i);
+
+ return 0;
+}
+
+void ks_dw_pcie_enable_legacy_irqs(struct keystone_pcie *ks_pcie)
+{
+ int i;
+
+ for (i = 0; i < MAX_LEGACY_IRQS; i++)
+ writel(0x1, ks_pcie->va_app_base + IRQ_ENABLE_SET + (i << 4));
+}
+
+void ks_dw_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie, int offset)
+{
+ struct pcie_port *pp = &ks_pcie->pp;
+ u32 pending;
+ int virq;
+
+ pending = readl(ks_pcie->va_app_base + IRQ_STATUS + (offset << 4));
+
+ if (BIT(0) & pending) {
+ virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
+ dev_dbg(pp->dev,
+ ": irq: irq_offset %d, virq %d\n", offset, virq);
+ generic_handle_irq(virq);
+ }
+
+ /* EOI the INTx interrupt */
+ writel(offset, ks_pcie->va_app_base + IRQ_EOI);
+}
+
+static void ks_dw_pcie_ack_legacy_irq(struct irq_data *d)
+{
+}
+
+static void ks_dw_pcie_mask_legacy_irq(struct irq_data *d)
+{
+}
+
+static void ks_dw_pcie_unmask_legacy_irq(struct irq_data *d)
+{
+}
+
+static struct irq_chip ks_dw_pcie_legacy_irq_chip = {
+ .name = "Keystone-PCI-Legacy-IRQ",
+ .irq_ack = ks_dw_pcie_ack_legacy_irq,
+ .irq_mask = ks_dw_pcie_mask_legacy_irq,
+ .irq_unmask = ks_dw_pcie_unmask_legacy_irq,
+};
+
+static int ks_dw_pcie_init_legacy_irq_map(struct irq_domain *d,
+ unsigned int irq, irq_hw_number_t hw_irq)
+{
+ irq_set_chip_and_handler(irq, &ks_dw_pcie_legacy_irq_chip,
+ handle_level_irq);
+ irq_set_chip_data(irq, d->host_data);
+ set_irq_flags(irq, IRQF_VALID);
+
+ return 0;
+}
+
+static const struct irq_domain_ops ks_dw_pcie_legacy_irq_domian_ops = {
+ .map = ks_dw_pcie_init_legacy_irq_map,
+ .xlate = irq_domain_xlate_onetwocell,
+};
+
+/**
+ * ks_dw_pcie_set_dbi_mode() - Set DBI mode to access overlaid BAR mask registers
+ *
+ * Since modification of dbi_cs2 involves different clock domain, read the
+ * status back to ensure the transition is complete.
+ */
+static void ks_dw_pcie_set_dbi_mode(void __iomem *reg_virt)
+{
+ u32 val;
+
+ writel(DBI_CS2_EN_VAL | readl(reg_virt + CMD_STATUS),
+ reg_virt + CMD_STATUS);
+
+ do {
+ val = readl(reg_virt + CMD_STATUS);
+ } while (!(val & DBI_CS2_EN_VAL));
+}
+
+/**
+ * ks_dw_pcie_clear_dbi_mode() - Disable DBI mode
+ *
+ * Since modification of dbi_cs2 involves different clock domain, read the
+ * status back to ensure the transition is complete.
+ */
+static void ks_dw_pcie_clear_dbi_mode(void __iomem *reg_virt)
+{
+ u32 val;
+
+ writel(~DBI_CS2_EN_VAL & readl(reg_virt + CMD_STATUS),
+ reg_virt + CMD_STATUS);
+
+ do {
+ val = readl(reg_virt + CMD_STATUS);
+ } while (val & DBI_CS2_EN_VAL);
+}
+
+void ks_dw_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
+{
+ struct pcie_port *pp = &ks_pcie->pp;
+ u32 start = pp->mem.start, end = pp->mem.end;
+ int i, tr_size;
+
+ /* disable BARS for inbound access */
+ ks_dw_pcie_set_dbi_mode(ks_pcie->va_app_base);
+ writel(0, pp->dbi_base + PCI_BASE_ADDRESS_0);
+ writel(0, pp->dbi_base + PCI_BASE_ADDRESS_1);
+ ks_dw_pcie_clear_dbi_mode(ks_pcie->va_app_base);
+
+ /* Set outbound translation size per window division */
+ writel(CFG_PCIM_WIN_SZ_IDX & 0x7, ks_pcie->va_app_base + OB_SIZE);
+
+ tr_size = (1 << (CFG_PCIM_WIN_SZ_IDX & 0x7)) * SZ_1M;
+
+ /* Using Direct 1:1 mapping of RC <-> PCI memory space */
+ for (i = 0; (i < CFG_PCIM_WIN_CNT) && (start < end); i++) {
+ writel(start | 1, ks_pcie->va_app_base + OB_OFFSET_INDEX(i));
+ writel(0, ks_pcie->va_app_base + OB_OFFSET_HI(i));
+ start += tr_size;
+ }
+
+ /* Enable OB translation */
+ writel(OB_XLAT_EN_VAL | readl(ks_pcie->va_app_base + CMD_STATUS),
+ ks_pcie->va_app_base + CMD_STATUS);
+}
+
+/**
+ * ks_pcie_cfg_setup() - Set up configuration space address for a
+ * device
+ *
+ * @ks_pcie: ptr to keystone_pcie structure
+ * @bus: Bus number the device is residing on
+ * @devfn: device, function number info
+ *
+ * Forms and returns the address of configuration space mapped in PCIESS
+ * address space 0. Also configures CFG_SETUP for remote configuration space
+ * access.
+ *
+ * The address space has two regions to access configuration - local and remote.
+ * We access local region for bus 0 (as RC is attached on bus 0) and remote
+ * region for others with TYPE 1 access when bus > 1. As for device on bus = 1,
+ * we will do TYPE 0 access as it will be on our secondary bus (logical).
+ * CFG_SETUP is needed only for remote configuration access.
+ */
+static void __iomem *ks_pcie_cfg_setup(struct keystone_pcie *ks_pcie, u8 bus,
+ unsigned int devfn)
+{
+ u8 device = PCI_SLOT(devfn), function = PCI_FUNC(devfn);
+ struct pcie_port *pp = &ks_pcie->pp;
+ u32 regval;
+
+ if (bus == 0)
+ return pp->dbi_base;
+
+ regval = (bus << 16) | (device << 8) | function;
+ /*
+ * Since Bus#1 will be a virtual bus, we need to have TYPE0
+ * access only.
+ * TYPE 1
+ */
+ if (bus != 1)
+ regval |= BIT(24);
+
+ writel(regval, ks_pcie->va_app_base + CFG_SETUP);
+ return pp->va_cfg0_base;
+}
+
+int ks_dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
+ unsigned int devfn, int where, int size, u32 *val)
+{
+ struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
+ u8 bus_num = bus->number;
+ void __iomem *addr;
+ int ret;
+
+ addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn);
+ ret = dw_pcie_cfg_read(addr + (where & ~0x3), where, size, val);
+
+ return ret;
+}
+
+int ks_dw_pcie_wr_other_conf(struct pcie_port *pp,
+ struct pci_bus *bus, unsigned int devfn, int where,
+ int size, u32 val)
+{
+ struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
+ u8 bus_num = bus->number;
+ void __iomem *addr;
+
+ addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn);
+
+ return dw_pcie_cfg_write(addr + (where & ~0x3), where, size, val);
+}
+
+/**
+ * ks_dw_pcie_v3_65_scan_bus() - keystone scan_bus post initialization
+ *
+ * This sets BAR0 to enable inbound access for MSI_IRQ register
+ */
+void ks_dw_pcie_v3_65_scan_bus(struct pcie_port *pp)
+{
+ struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
+
+ /* Configure and set up BAR0 */
+ ks_dw_pcie_set_dbi_mode(ks_pcie->va_app_base);
+
+ /* Enable BAR0 */
+ writel(1, pp->dbi_base + PCI_BASE_ADDRESS_0);
+ writel(SZ_4K - 1, pp->dbi_base + PCI_BASE_ADDRESS_0);
+
+ ks_dw_pcie_clear_dbi_mode(ks_pcie->va_app_base);
+
+ /*
+ * For BAR0, just setting bus address for inbound writes (MSI) should
+ * be sufficient. Use physical address to avoid any conflicts.
+ */
+ writel(ks_pcie->app.start, pp->dbi_base + PCI_BASE_ADDRESS_0);
+}
+
+/**
+ * ks_dw_pcie_link_up() - Check if link up
+ *
+ */
+int ks_dw_pcie_link_up(struct pcie_port *pp)
+{
+ u32 val = readl(pp->dbi_base + DEBUG0);
+
+ return (val & LTSSM_STATE_MASK) == LTSSM_STATE_L0;
+}
+
+void ks_dw_pcie_initiate_link_train(struct keystone_pcie *ks_pcie)
+{
+ u32 val;
+
+ /* first disable Link training */
+ val = readl(ks_pcie->va_app_base + CMD_STATUS);
+ val &= ~LTSSM_EN_VAL;
+ writel(LTSSM_EN_VAL | val, ks_pcie->va_app_base + CMD_STATUS);
+
+ /* Initiate Link Training. */
+ val = readl(ks_pcie->va_app_base + CMD_STATUS);
+ writel(LTSSM_EN_VAL | val, ks_pcie->va_app_base + CMD_STATUS);
+}
+
+/**
+ * ks_dw_pcie_host_init() - initialize host for v3_65 dw hardware
+ *
+ * It ioremap the register resources, initialize legacy irq domain
+ * and then call dw_pcie_v3_65_host_init() API to intialize the Keystone
+ * PCI host controller.
+ *
+ */
+int __init ks_dw_pcie_host_init(struct keystone_pcie *ks_pcie,
+ struct device_node *msi_intc_np)
+{
+ struct pcie_port *pp = &ks_pcie->pp;
+ struct platform_device *pdev = to_platform_device(pp->dev);
+ struct resource *res;
+
+ /* index 0 is the config reg. space address */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ pp->dbi_base = devm_ioremap_resource(pp->dev, res);
+ if (IS_ERR(pp->dbi_base))
+ return PTR_ERR(pp->dbi_base);
+
+ /*
+ * we set these same and is used in pcie rd/wr_other_conf
+ * functions
+ */
+ pp->va_cfg0_base = pp->dbi_base + SPACE0_REMOTE_CFG_OFFSET;
+ pp->va_cfg1_base = pp->va_cfg0_base;
+
+ /* index 1 is the application reg. space address */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ ks_pcie->app = *res;
+ ks_pcie->va_app_base = devm_ioremap_resource(pp->dev, res);
+ if (IS_ERR(ks_pcie->va_app_base))
+ return PTR_ERR(ks_pcie->va_app_base);
+
+ /* create legacy irq domain */
+ ks_pcie->legacy_irq_domain =
+ irq_domain_add_linear(ks_pcie->legacy_intc_np,
+ MAX_LEGACY_IRQS,
+ &ks_dw_pcie_legacy_irq_domian_ops,
+ NULL);
+ if (!ks_pcie->legacy_irq_domain) {
+ dev_err(pp->dev, "Failed to add irq domain for legacy irqs\n");
+ return -EINVAL;
+ }
+
+ return dw_pcie_host_init(pp);
+}
diff --git a/drivers/pci/host/pci-keystone.c b/drivers/pci/host/pci-keystone.c
new file mode 100644
index 0000000..c1cfaef
--- /dev/null
+++ b/drivers/pci/host/pci-keystone.c
@@ -0,0 +1,386 @@
+/*
+ * PCIe host controller driver for Texas Instruments Keystone SoCs
+ *
+ * Copyright (C) 2013-2014 Texas Instruments., Ltd.
+ * http://www.ti.com
+ *
+ * Author: Murali Karicheri <[email protected]>
+ * Implementation based on pci-exynos.c and pcie-designware.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/irqchip/chained_irq.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/of_irq.h>
+#include <linux/of.h>
+#include <linux/of_pci.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+#include <linux/resource.h>
+#include <linux/signal.h>
+
+#include "pcie-designware.h"
+#include "pci-keystone.h"
+
+#define DRIVER_NAME "keystone-pcie"
+
+/* driver specific constants */
+#define MAX_MSI_HOST_IRQS 8
+#define MAX_LEGACY_HOST_IRQS 4
+
+/* RC mode settings masks */
+#define PCIE_RC_MODE BIT(2)
+#define PCIE_MODE_MASK (BIT(1) | BIT(2))
+
+/* DEV_STAT_CTRL */
+#define PCIE_CAP_BASE 0x70
+
+#define to_keystone_pcie(x) container_of(x, struct keystone_pcie, pp)
+
+static int ks_pcie_establish_link(struct keystone_pcie *ks_pcie)
+{
+ struct pcie_port *pp = &ks_pcie->pp;
+ int count = 200;
+
+ dw_pcie_setup_rc(pp);
+
+ if (dw_pcie_link_up(pp)) {
+ dev_err(pp->dev, "Link already up\n");
+ return 0;
+ }
+
+ ks_dw_pcie_initiate_link_train(ks_pcie);
+ /* check if the link is up or not */
+ while (!dw_pcie_link_up(pp)) {
+ usleep_range(100, 1000);
+ if (--count) {
+ ks_dw_pcie_initiate_link_train(ks_pcie);
+ continue;
+ }
+ dev_err(pp->dev, "phy link never came up\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void ks_pcie_msi_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+ struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
+ u32 offset = irq - ks_pcie->msi_host_irqs[0];
+ struct pcie_port *pp = &ks_pcie->pp;
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+
+ dev_dbg(pp->dev, "ks_pci_msi_irq_handler, irq %d\n", irq);
+
+ /*
+ * The chained irq handler installation would have replaced normal
+ * interrupt driver handler so we need to take care of mask/unmask and
+ * ack operation.
+ */
+ chained_irq_enter(chip, desc);
+ ks_dw_pcie_handle_msi_irq(ks_pcie, offset);
+ chained_irq_exit(chip, desc);
+}
+
+/**
+ * ks_pcie_legacy_irq_handler() - Handle legacy interrupt
+ * @irq: IRQ line for legacy interrupts
+ * @desc: Pointer to irq descriptor
+ *
+ * Traverse through pending legacy interrupts and invoke handler for each. Also
+ * takes care of interrupt controller level mask/ack operation.
+ */
+static void ks_pcie_legacy_irq_handler(unsigned int irq, struct irq_desc *desc)
+{
+ struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
+ struct pcie_port *pp = &ks_pcie->pp;
+ u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+
+ dev_dbg(pp->dev, ": Handling legacy irq %d\n", irq);
+
+ /*
+ * The chained irq handler installation would have replaced normal
+ * interrupt driver handler so we need to take care of mask/unmask and
+ * ack operation.
+ */
+ chained_irq_enter(chip, desc);
+ ks_dw_pcie_handle_legacy_irq(ks_pcie, irq_offset);
+ chained_irq_exit(chip, desc);
+}
+
+static int ks_pcie_get_irq_controller_info(struct keystone_pcie *ks_pcie,
+ char *controller, int *num_irqs)
+{
+ int temp, max_host_irqs, legacy = 1, *host_irqs, ret = -EINVAL;
+ struct device *dev = ks_pcie->pp.dev;
+ struct device_node *np_pcie = dev->of_node, **np_temp;
+
+ if (!strcmp(controller, "msi-interrupt-controller"))
+ legacy = 0;
+
+ if (legacy) {
+ np_temp = &ks_pcie->legacy_intc_np;
+ max_host_irqs = MAX_LEGACY_HOST_IRQS;
+ host_irqs = &ks_pcie->legacy_host_irqs[0];
+ } else {
+ np_temp = &ks_pcie->msi_intc_np;
+ max_host_irqs = MAX_MSI_HOST_IRQS;
+ host_irqs = &ks_pcie->msi_host_irqs[0];
+ }
+
+ /* interrupt controller is in a child node */
+ *np_temp = of_find_node_by_name(np_pcie, controller);
+ if (!(*np_temp)) {
+ dev_err(dev, "Node for %s is absent\n", controller);
+ goto out;
+ }
+ temp = of_irq_count(*np_temp);
+ if (!temp)
+ goto out;
+ if (temp > max_host_irqs)
+ dev_warn(dev, "Too many %s interrupts defined %u\n",
+ (legacy ? "legacy" : "MSI"), temp);
+
+ /*
+ * support upto max_host_irqs. In dt from index 0 to 3 (legacy) or 0 to
+ * 7 (MSI)
+ */
+ for (temp = 0; temp < max_host_irqs; temp++) {
+ host_irqs[temp] = irq_of_parse_and_map(*np_temp, temp);
+ if (host_irqs[temp] < 0)
+ break;
+ }
+ if (temp) {
+ *num_irqs = temp;
+ ret = 0;
+ }
+out:
+ return ret;
+}
+
+static void ks_pcie_setup_interrupts(struct keystone_pcie *ks_pcie)
+{
+ int i;
+
+ /* Legacy IRQ */
+ for (i = 0; i < ks_pcie->num_legacy_host_irqs; i++) {
+ irq_set_handler_data(ks_pcie->legacy_host_irqs[i], ks_pcie);
+ irq_set_chained_handler(ks_pcie->legacy_host_irqs[i],
+ ks_pcie_legacy_irq_handler);
+ }
+ ks_dw_pcie_enable_legacy_irqs(ks_pcie);
+
+ /* MSI IRQ */
+ if (IS_ENABLED(CONFIG_PCI_MSI)) {
+ for (i = 0; i < ks_pcie->num_msi_host_irqs; i++) {
+ irq_set_chained_handler(ks_pcie->msi_host_irqs[i],
+ ks_pcie_msi_irq_handler);
+ irq_set_handler_data(ks_pcie->msi_host_irqs[i],
+ ks_pcie);
+ }
+ }
+}
+
+/*
+ * When a PCI device does not exist during config cycles, keystone host gets a
+ * bus error instead of returning 0xffffffff. This handler always returns 0
+ * for this kind of faults.
+ */
+static int keystone_pcie_fault(unsigned long addr, unsigned int fsr,
+ struct pt_regs *regs)
+{
+ unsigned long instr = *(unsigned long *) instruction_pointer(regs);
+
+ if ((instr & 0x0e100090) == 0x00100090) {
+ int reg = (instr >> 12) & 15;
+
+ regs->uregs[reg] = -1;
+ regs->ARM_pc += 4;
+ }
+
+ return 0;
+}
+
+static void __init ks_pcie_host_init(struct pcie_port *pp)
+{
+ u32 vendor_device_id, val;
+ struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
+
+ ks_pcie_establish_link(ks_pcie);
+ ks_dw_pcie_setup_rc_app_regs(ks_pcie);
+ ks_pcie_setup_interrupts(ks_pcie);
+ writew(PCI_IO_RANGE_TYPE_32 | (PCI_IO_RANGE_TYPE_32 << 8),
+ pp->dbi_base + PCI_IO_BASE);
+
+ /* update the Vendor ID */
+ vendor_device_id = readl(ks_pcie->va_reg_pciid);
+ writew((vendor_device_id >> 16), pp->dbi_base + PCI_DEVICE_ID);
+
+ /* update the DEV_STAT_CTRL to publish right mrrs */
+ val = readl(pp->dbi_base + PCIE_CAP_BASE + PCI_EXP_DEVCTL);
+ val &= ~PCI_EXP_DEVCTL_READRQ;
+ /* set the mrrs to 256 bytes */
+ val |= BIT(12);
+ writel(val, pp->dbi_base + PCIE_CAP_BASE + PCI_EXP_DEVCTL);
+
+ /*
+ * PCIe access errors that result into OCP errors are caught by ARM as
+ * "External aborts"
+ */
+ hook_fault_code(17, keystone_pcie_fault, SIGBUS, 0,
+ "Asynchronous external abort");
+}
+
+static struct pcie_host_ops keystone_pcie_host_ops = {
+ .rd_other_conf = ks_dw_pcie_rd_other_conf,
+ .wr_other_conf = ks_dw_pcie_wr_other_conf,
+ .link_up = ks_dw_pcie_link_up,
+ .host_init = ks_pcie_host_init,
+ .msi_set_irq = ks_dw_pcie_msi_set_irq,
+ .msi_clear_irq = ks_dw_pcie_msi_clear_irq,
+ .get_msi_data = ks_dw_pcie_get_msi_data,
+ .msi_host_init = ks_dw_pcie_msi_host_init,
+ .scan_bus = ks_dw_pcie_v3_65_scan_bus,
+};
+
+static int __init ks_add_pcie_port(struct keystone_pcie *ks_pcie,
+ struct platform_device *pdev)
+{
+ struct pcie_port *pp = &ks_pcie->pp;
+ int ret;
+
+ ret = ks_pcie_get_irq_controller_info(ks_pcie,
+ "legacy-interrupt-controller",
+ &ks_pcie->num_legacy_host_irqs);
+ if (ret)
+ return ret;
+
+ if (IS_ENABLED(CONFIG_PCI_MSI)) {
+ ret = ks_pcie_get_irq_controller_info(ks_pcie,
+ "msi-interrupt-controller",
+ &ks_pcie->num_msi_host_irqs);
+ if (ret)
+ return ret;
+ }
+
+ pp->root_bus_nr = -1;
+ pp->ops = &keystone_pcie_host_ops;
+ ret = ks_dw_pcie_host_init(ks_pcie, ks_pcie->msi_intc_np);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to initialize host\n");
+ return ret;
+ }
+
+ return ret;
+}
+
+static const struct of_device_id ks_pcie_of_match[] = {
+ {
+ .type = "pci",
+ .compatible = "ti,keystone-pcie",
+ },
+ { },
+};
+MODULE_DEVICE_TABLE(of, ks_pcie_of_match);
+
+static int __exit ks_pcie_remove(struct platform_device *pdev)
+{
+ struct keystone_pcie *ks_pcie = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(ks_pcie->clk);
+
+ return 0;
+}
+
+static int __init ks_pcie_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct keystone_pcie *ks_pcie;
+ struct pcie_port *pp;
+ struct resource *res;
+ void __iomem *reg_p;
+ struct phy *phy;
+ int ret = 0;
+ u32 val;
+
+ ks_pcie = devm_kzalloc(&pdev->dev, sizeof(*ks_pcie),
+ GFP_KERNEL);
+ if (!ks_pcie) {
+ dev_err(dev, "no memory for keystone pcie\n");
+ return -ENOMEM;
+ }
+ pp = &ks_pcie->pp;
+
+ /* index 2 is the devcfg register for RC mode settings */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+ reg_p = devm_ioremap_resource(dev, res);
+ if (IS_ERR(reg_p))
+ return PTR_ERR(reg_p);
+
+ /* enable RC mode in devcfg */
+ val = readl(reg_p);
+ val &= ~PCIE_MODE_MASK;
+ val |= PCIE_RC_MODE;
+ writel(val, reg_p);
+
+ /* initialize SerDes Phy if present */
+ phy = devm_phy_get(dev, "pcie-phy");
+ if (!IS_ERR_OR_NULL(phy)) {
+ ret = phy_init(phy);
+ if (ret < 0)
+ return ret;
+ }
+
+ /* index 3 is to read PCI DEVICE_ID */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 3);
+ reg_p = devm_ioremap_resource(dev, res);
+ if (IS_ERR(reg_p))
+ return PTR_ERR(reg_p);
+ ks_pcie->va_reg_pciid = reg_p;
+
+ pp->dev = dev;
+ platform_set_drvdata(pdev, ks_pcie);
+ ks_pcie->clk = devm_clk_get(dev, "pcie");
+ if (IS_ERR(ks_pcie->clk)) {
+ dev_err(dev, "Failed to get pcie rc clock\n");
+ return PTR_ERR(ks_pcie->clk);
+ }
+ ret = clk_prepare_enable(ks_pcie->clk);
+ if (ret)
+ return ret;
+
+ ret = ks_add_pcie_port(ks_pcie, pdev);
+ if (ret < 0)
+ goto fail_clk;
+
+ return 0;
+fail_clk:
+ clk_disable_unprepare(ks_pcie->clk);
+
+ return ret;
+}
+
+static struct platform_driver ks_pcie_driver __refdata = {
+ .probe = ks_pcie_probe,
+ .remove = __exit_p(ks_pcie_remove),
+ .driver = {
+ .name = "keystone-pcie",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(ks_pcie_of_match),
+ },
+};
+
+module_platform_driver(ks_pcie_driver);
+
+MODULE_AUTHOR("Murali Karicheri <[email protected]>");
+MODULE_DESCRIPTION("Keystone PCIe host controller driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/pci/host/pci-keystone.h b/drivers/pci/host/pci-keystone.h
new file mode 100644
index 0000000..729ea7d
--- /dev/null
+++ b/drivers/pci/host/pci-keystone.h
@@ -0,0 +1,58 @@
+/*
+ * Keystone PCI Controller's common includes
+ *
+ * Copyright (C) 2013-2014 Texas Instruments., Ltd.
+ * http://www.ti.com
+ *
+ * Author: Murali Karicheri <[email protected]>
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define MAX_LEGACY_IRQS 4
+#define MAX_MSI_HOST_IRQS 8
+#define MAX_LEGACY_HOST_IRQS 4
+
+struct keystone_pcie {
+ struct clk *clk;
+ struct pcie_port pp;
+ void __iomem *va_reg_pciid;
+
+ int num_legacy_host_irqs;
+ int legacy_host_irqs[MAX_LEGACY_HOST_IRQS];
+ struct device_node *legacy_intc_np;
+
+ int num_msi_host_irqs;
+ int msi_host_irqs[MAX_MSI_HOST_IRQS];
+ struct device_node *msi_intc_np;
+ struct irq_domain *legacy_irq_domain;
+
+ /* Application register space */
+ void __iomem *va_app_base;
+ struct resource app;
+};
+
+/* Keystone DW specific MSI controller APIs/definitions */
+void ks_dw_pcie_handle_msi_irq(struct keystone_pcie *ks_pcie, int offset);
+u32 ks_dw_pcie_get_msi_data(struct pcie_port *pp);
+
+/* Keystone specific PCI controller APIs */
+void ks_dw_pcie_enable_legacy_irqs(struct keystone_pcie *ks_pcie);
+void ks_dw_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie, int offset);
+int ks_dw_pcie_host_init(struct keystone_pcie *ks_pcie,
+ struct device_node *msi_intc_np);
+int ks_dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus,
+ unsigned int devfn, int where, int size, u32 val);
+int ks_dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
+ unsigned int devfn, int where, int size, u32 *val);
+void ks_dw_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie);
+int ks_dw_pcie_link_up(struct pcie_port *pp);
+void ks_dw_pcie_initiate_link_train(struct keystone_pcie *ks_pcie);
+void ks_dw_pcie_msi_set_irq(struct pcie_port *pp, int irq);
+void ks_dw_pcie_msi_clear_irq(struct pcie_port *pp, int irq);
+void ks_dw_pcie_v3_65_scan_bus(struct pcie_port *pp);
+int ks_dw_pcie_msi_host_init(struct pcie_port *pp,
+ struct msi_chip *chip);
--
1.7.9.5

2014-07-22 22:35:38

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH v7 4/5] PCI: add PCI controller for keystone PCIe h/w

On Mon, Jul 21, 2014 at 12:58:44PM -0400, Murali Karicheri wrote:
> keystone PCIe controller is based on v3.65 version of the
> designware h/w. Main differences are
> 1. No ATU support
> 2. Legacy and MSI irq functions are implemented in
> application register space
> 3. MSI interrupts are multiplexed over 8 IRQ lines to the Host
> side.
> All of the Application register space handing code are organized into
> pci-keystone-dw.c and the functions are called from pci-keystone.c
> to implement PCI controller driver. Also add necessary DT documentation
> for the driver.
>
> Signed-off-by: Murali Karicheri <[email protected]>
> Acked-by: Santosh Shilimkar <[email protected]>
> ...

> +++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt
> ...

> +Note for PCI driver usage
> +=========================
> +Driver requires pci=pcie_bus_perf in the bootargs for proper functioning.

Whoa, why is this? Special boot args should not be required.

> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 21df477..f8bc475 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -46,4 +46,9 @@ config PCI_HOST_GENERIC
> Say Y here if you want to support a simple generic PCI host
> controller, such as the one emulated by kvmtool.
>
> +config PCI_KEYSTONE
> + bool "TI Keystone PCIe controller"
> + depends on ARCH_KEYSTONE
> + select PCIE_DW
> + select PCIEPORTBUS

It'd be nice to have some help text here. I know, not everybody else does.

> +++ b/drivers/pci/host/pci-keystone-dw.c
> ...
> +void ks_dw_pcie_handle_msi_irq(struct keystone_pcie *ks_pcie , int offset)
> +{
> + struct pcie_port *pp = &ks_pcie->pp;
> + u32 pending, vector;
> + int src, virq;
> +
> + pending = readl(ks_pcie->va_app_base + MSI0_IRQ_STATUS + (offset << 4));

Blank line here (before the block comment).

> + /*
> + * MSI0, Status bit 0-3 shows vectors 0, 8, 16, 24, MSI1 status bit
> + * shows 1, 9, 17, 25 and so forth
> + */
> + for (src = 0; src < 4; src++) {
> + if (BIT(src) & pending) {
> + vector = offset + (src << 3);
> + virq = irq_linear_revmap(pp->irq_domain, vector);
> + dev_dbg(pp->dev,
> + "irq: bit %d, vector %d, virq %d\n",
> + src, vector, virq);
> + generic_handle_irq(virq);
> + }
> + }
> +}
> +
> ...

> +static void __iomem *ks_pcie_cfg_setup(struct keystone_pcie *ks_pcie, u8 bus,
> + unsigned int devfn)
> +{
> + u8 device = PCI_SLOT(devfn), function = PCI_FUNC(devfn);
> + struct pcie_port *pp = &ks_pcie->pp;
> + u32 regval;
> +
> + if (bus == 0)
> + return pp->dbi_base;
> +
> + regval = (bus << 16) | (device << 8) | function;
> + /*
> + * Since Bus#1 will be a virtual bus, we need to have TYPE0
> + * access only.
> + * TYPE 1
> + */
> + if (bus != 1)
> + regval |= BIT(24);
> +
> + writel(regval, ks_pcie->va_app_base + CFG_SETUP);
> + return pp->va_cfg0_base;
> +}
> +
> +int ks_dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> + unsigned int devfn, int where, int size, u32 *val)
> +{
> + struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
> + u8 bus_num = bus->number;
> + void __iomem *addr;
> + int ret;
> +
> + addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn);
> + ret = dw_pcie_cfg_read(addr + (where & ~0x3), where, size, val);

This *looks* like it needs a lock to protect against concurrent
ks_pcie_cfg_setup() users, since it writes a register.

> +
> + return ret;

Please use the same style as in ks_dw_pcie_wr_other_conf(), i.e., get rid
of "ret".

> +}
> +
> +int ks_dw_pcie_wr_other_conf(struct pcie_port *pp,
> + struct pci_bus *bus, unsigned int devfn, int where,
> + int size, u32 val)
> +{
> + struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
> + u8 bus_num = bus->number;
> + void __iomem *addr;
> +
> + addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn);
> +
> + return dw_pcie_cfg_write(addr + (where & ~0x3), where, size, val);
> +}

> +++ b/drivers/pci/host/pci-keystone.c
> ...

> +static struct platform_driver ks_pcie_driver __refdata = {

Why does this need to be __refdata? There are no other occurrences in
drivers/pci.

> + .probe = ks_pcie_probe,
> + .remove = __exit_p(ks_pcie_remove),
> + .driver = {
> + .name = "keystone-pcie",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(ks_pcie_of_match),
> + },
> +};

Bjorn

2014-07-22 22:53:32

by Karicheri, Muralidharan

[permalink] [raw]
Subject: Re: [PATCH v7 4/5] PCI: add PCI controller for keystone PCIe h/w

Bjorn,

On 07/22/2014 06:35 PM, Bjorn Helgaas wrote:
> On Mon, Jul 21, 2014 at 12:58:44PM -0400, Murali Karicheri wrote:
>> keystone PCIe controller is based on v3.65 version of the
>> designware h/w. Main differences are
>> 1. No ATU support
>> 2. Legacy and MSI irq functions are implemented in
>> application register space
>> 3. MSI interrupts are multiplexed over 8 IRQ lines to the Host
>> side.
>> All of the Application register space handing code are organized into
>> pci-keystone-dw.c and the functions are called from pci-keystone.c
>> to implement PCI controller driver. Also add necessary DT documentation
>> for the driver.
>>
>> Signed-off-by: Murali Karicheri<[email protected]>
>> Acked-by: Santosh Shilimkar<[email protected]>
>> ...
>
>> +++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt
>> ...
>
>> +Note for PCI driver usage
>> +=========================
>> +Driver requires pci=pcie_bus_perf in the bootargs for proper functioning.
>
> Whoa, why is this? Special boot args should not be required.

This was discussed initially and I had added following commit to get
this working instead of a PCI quirk. To get some background please see
the thread for commit below that you also had signed off as well.

commit 8b5742ad156d30ee38486652cdbd152e2d6ebbcc
Author: Murali Karicheri <[email protected]>
Date: Wed May 28 13:14:53 2014 -0400

ARM/PCI: Call pcie_bus_configure_settings() to set MPS

Call pcie_bus_configure_settings() on ARM, like for other platforms.
pcie_bus_configure_settings() makes sure the MPS across the bus is
uniform
and provides the ability to tune the MRSS and MPS to higher performance
values. This is particularly important for embedded where there is no
firmware to program these PCIe settings for the OS.

Signed-off-by: Murali Karicheri <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
CC: Russell King <[email protected]>
CC: Arnd Bergmann <[email protected]>
CC: Jason Gunthorpe <[email protected]>
CC: Santosh Shilimkar <[email protected]>

This was added as a preparatory patch to support keystone and
avoid a PCI quirk to do the same. Keystone has MRSS limitation
of 256 bytes. So adding a bootargs flag was suggested a better option
than a PCI quirk.

I will look into the rest of the comments and possibly try to address
them or discuss.

BTW, please apply patch 1-3 that has already got ack from maintainers
and is indepdent of this patch.

Thanks

Murali

>
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index 21df477..f8bc475 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -46,4 +46,9 @@ config PCI_HOST_GENERIC
>> Say Y here if you want to support a simple generic PCI host
>> controller, such as the one emulated by kvmtool.
>>
>> +config PCI_KEYSTONE
>> + bool "TI Keystone PCIe controller"
>> + depends on ARCH_KEYSTONE
>> + select PCIE_DW
>> + select PCIEPORTBUS
>
> It'd be nice to have some help text here. I know, not everybody else does.
>
>> +++ b/drivers/pci/host/pci-keystone-dw.c
>> ...
>> +void ks_dw_pcie_handle_msi_irq(struct keystone_pcie *ks_pcie , int offset)
>> +{
>> + struct pcie_port *pp =&ks_pcie->pp;
>> + u32 pending, vector;
>> + int src, virq;
>> +
>> + pending = readl(ks_pcie->va_app_base + MSI0_IRQ_STATUS + (offset<< 4));
>
> Blank line here (before the block comment).
>
>> + /*
>> + * MSI0, Status bit 0-3 shows vectors 0, 8, 16, 24, MSI1 status bit
>> + * shows 1, 9, 17, 25 and so forth
>> + */
>> + for (src = 0; src< 4; src++) {
>> + if (BIT(src)& pending) {
>> + vector = offset + (src<< 3);
>> + virq = irq_linear_revmap(pp->irq_domain, vector);
>> + dev_dbg(pp->dev,
>> + "irq: bit %d, vector %d, virq %d\n",
>> + src, vector, virq);
>> + generic_handle_irq(virq);
>> + }
>> + }
>> +}
>> +
>> ...
>
>> +static void __iomem *ks_pcie_cfg_setup(struct keystone_pcie *ks_pcie, u8 bus,
>> + unsigned int devfn)
>> +{
>> + u8 device = PCI_SLOT(devfn), function = PCI_FUNC(devfn);
>> + struct pcie_port *pp =&ks_pcie->pp;
>> + u32 regval;
>> +
>> + if (bus == 0)
>> + return pp->dbi_base;
>> +
>> + regval = (bus<< 16) | (device<< 8) | function;
>> + /*
>> + * Since Bus#1 will be a virtual bus, we need to have TYPE0
>> + * access only.
>> + * TYPE 1
>> + */
>> + if (bus != 1)
>> + regval |= BIT(24);
>> +
>> + writel(regval, ks_pcie->va_app_base + CFG_SETUP);
>> + return pp->va_cfg0_base;
>> +}
>> +
>> +int ks_dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
>> + unsigned int devfn, int where, int size, u32 *val)
>> +{
>> + struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
>> + u8 bus_num = bus->number;
>> + void __iomem *addr;
>> + int ret;
>> +
>> + addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn);
>> + ret = dw_pcie_cfg_read(addr + (where& ~0x3), where, size, val);
>
> This *looks* like it needs a lock to protect against concurrent
> ks_pcie_cfg_setup() users, since it writes a register.
>
>> +
>> + return ret;
>
> Please use the same style as in ks_dw_pcie_wr_other_conf(), i.e., get rid
> of "ret".
>
>> +}
>> +
>> +int ks_dw_pcie_wr_other_conf(struct pcie_port *pp,
>> + struct pci_bus *bus, unsigned int devfn, int where,
>> + int size, u32 val)
>> +{
>> + struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
>> + u8 bus_num = bus->number;
>> + void __iomem *addr;
>> +
>> + addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn);
>> +
>> + return dw_pcie_cfg_write(addr + (where& ~0x3), where, size, val);
>> +}
>
>> +++ b/drivers/pci/host/pci-keystone.c
>> ...
>
>> +static struct platform_driver ks_pcie_driver __refdata = {
>
> Why does this need to be __refdata? There are no other occurrences in
> drivers/pci.
>
>> + .probe = ks_pcie_probe,
>> + .remove = __exit_p(ks_pcie_remove),
>> + .driver = {
>> + .name = "keystone-pcie",
>> + .owner = THIS_MODULE,
>> + .of_match_table = of_match_ptr(ks_pcie_of_match),
>> + },
>> +};
>
> Bjorn

2014-07-22 22:57:16

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH v7 0/5] Add Keystone PCIe controller driver

On Mon, Jul 21, 2014 at 12:58:40PM -0400, Murali Karicheri wrote:
> This patch series add PCIe controller driver for keystone SoCs. This is
> based on v4 of the series posted to the mailing list. Keystone PCI controller
> is based on version 3.65 of the DW hardware. This driver uses the DW core
> functions to implement the PCI controller driver for keystone.
>
> Testing:
> =======
>
> Testing of the driver is done on TI's K2HK EVM inserted to a Elma blu!eco
> MicroTCA chassis AMC slot with JumpGen SEM-200 AMC SATA Storage and Dual
> Ethernet Card Express inserted on another AMC slot. The e1000e driver available
> on intel's website is patched to the kernel source tree and build. e1000e.ko
> is dynamically loaded and executed ping and iperf tests to test the
> functionality.
>
> Thanks to all of you who have contributed by reviewing and offering valuable
> comments. If you want me to add your "Reviewed-by", please let me know so that
> I can add it to the commit log and re-send. Patch 1-3 has Acks from maintainers
> and I believe this can be merged to upstream branch. Patch 4 is waiting for
> Ack from DT maintainer. Please provide the same at the earliest.
>
> Thanks
>
> Signed-off-by: Murali Karicheri <[email protected]>
>
> CC: Russell King <[email protected]>
> CC: Grant Likely <[email protected]>
> CC: Rob Herring <[email protected]>
> CC: Mohit Kumar <[email protected]>
> CC: Jingoo Han <[email protected]>
> CC: Bjorn Helgaas <[email protected]>
> CC: Pratyush Anand <[email protected]>
> CC: Richard Zhu <[email protected]>
> CC: Kishon Vijay Abraham I <[email protected]>
> CC: Marek Vasut <[email protected]>
> CC: Arnd Bergmann <[email protected]>
> CC: Pawel Moll <[email protected]>
> CC: Mark Rutland <[email protected]>
> CC: Ian Campbell <[email protected]>
> CC: Kumar Gala <[email protected]>
> CC: Randy Dunlap <[email protected]>
> CC: Grant Likely <[email protected]>
>
> Changelog:
>
> v7
> - Removed ti,enable_linktrain DT property. The driver now check if
> the link is up and then retrain the link if not already up. This
> is as per comment from DT maintainer.
> - Rebased to upstream kernel v3.16-rc6
>
> v6
> - Added Ack from Maintainer for patch 3. Patch 1-3 now have the
> Acks from maintainer and can be merged to upstream branch.
> Patch 4 is waiting ack from DT maintainer.
> v5
> - Rebased to upstream kernel v3.16-rc5
> - Reworked Patch 3/6 and 4/6 into a single patch based on
> maintainer comment to use a API callback model to support
> the v3.65 h/w.
> v4
> - Addressed comments against 5/5.
> - Added a patch 6/6 for Maintainer information
> - Added couple of Acked-By:
> - Rebased to latest Linux 3.16-rc4
> v3
> - DW application register handling code is now part of
> Keystone PCI driver. RFC had this code part of Keystone
> PCI driver, then V1 moved this to a separate file to
> re-use on other platforms that uses this version of the
> DW h/w. Then based on comments against v2, this is moved
> back to Keystone driver.
> - Keystone SerDes phy driver is removed from this series so that
> this can be merged independent of that patch
> - added msi_set_irq()/clear_irq() API's to support Keystone
>
> V2
> - Split the designware pcie enhancement patch to multiple
> patches based on functionality added
> - Remove the quirk code and add a patch to fix mps/mrss
> tuning for ARM. Use kernel command line parameter
> pci=pcie_bus_perf to work with Keystone PCI Controller.
> Following patch addressed this.
> [PATCH v1] ARM: pci: add call to pcie_bus_configure_settings()
> - Add documentation for device tree bindings
> - Add separate interrupt controller nodes for MSI and Legacy
> IRQs and use irq map for legacy IRQ
> - Use compatibility to identify v3.65 version of the DW hardware
> and use it to customize the designware common code.
> - Use reg property for configuration space instead of range
> - Other minor updates based on code inspection.
>
> V1
> - Add an interrupt controller node for Legacy irq chip and use
> interrupt map/map-mask property to map legacy IRQs A/B/C/D
> - Add a Phy driver to replace the original serdes driver
> - Move common application register handling code to a separate
> file to allow re-use across other platforms that use older
> DW PCIe h/w
> - PCI quirk for maximum read request size. Check and override only
> if the maximum is higher than what controller can handle.
> - Converted to a module platform driver.
>
> Murali Karicheri (5):
> PCI: designware: add rd[wr]_other_conf API
> PCI: designware: refactor MSI code to work with v3.65 dw hardware

I applied the above two to my pci/host-designware branch. The rest didn't
apply cleanly because I applied Kishon's DRA7xx changes first, and there
are several conflicts. Can you rebase the rest of them on top of
pci/host-designware?

> PCI: designware: enhance dw_pcie_host_init() to support v3.65 DW
> hardware
> PCI: add PCI controller for keystone PCIe h/w
> PCI: keystone: Update maintainer information

You can squash the MAINTAINERS update into the driver addition. They're
logically part of the same change.

Bjorn

> .../devicetree/bindings/pci/pci-keystone.txt | 68 +++
> MAINTAINERS | 7 +
> drivers/pci/host/Kconfig | 5 +
> drivers/pci/host/Makefile | 1 +
> drivers/pci/host/pci-keystone-dw.c | 521 ++++++++++++++++++++
> drivers/pci/host/pci-keystone.c | 386 +++++++++++++++
> drivers/pci/host/pci-keystone.h | 58 +++
> drivers/pci/host/pcie-designware.c | 116 +++--
> drivers/pci/host/pcie-designware.h | 9 +
> 9 files changed, 1135 insertions(+), 36 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/pci/pci-keystone.txt
> create mode 100644 drivers/pci/host/pci-keystone-dw.c
> create mode 100644 drivers/pci/host/pci-keystone.c
> create mode 100644 drivers/pci/host/pci-keystone.h
>
> --
> 1.7.9.5
>

2014-07-22 23:52:07

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH v7 4/5] PCI: add PCI controller for keystone PCIe h/w

On Tue, Jul 22, 2014 at 06:52:12PM -0400, Murali Karicheri wrote:
> Bjorn,
>
> On 07/22/2014 06:35 PM, Bjorn Helgaas wrote:
> >On Mon, Jul 21, 2014 at 12:58:44PM -0400, Murali Karicheri wrote:
> >>keystone PCIe controller is based on v3.65 version of the
> >>designware h/w. Main differences are
> >> 1. No ATU support
> >> 2. Legacy and MSI irq functions are implemented in
> >> application register space
> >> 3. MSI interrupts are multiplexed over 8 IRQ lines to the Host
> >> side.
> >>All of the Application register space handing code are organized into
> >>pci-keystone-dw.c and the functions are called from pci-keystone.c
> >>to implement PCI controller driver. Also add necessary DT documentation
> >>for the driver.
> >>
> >>Signed-off-by: Murali Karicheri<[email protected]>
> >>Acked-by: Santosh Shilimkar<[email protected]>
> >>...
> >
> >>+++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt
> >>...
> >
> >>+Note for PCI driver usage
> >>+=========================
> >>+Driver requires pci=pcie_bus_perf in the bootargs for proper functioning.
> >
> >Whoa, why is this? Special boot args should not be required.
>
> This was discussed initially and I had added following commit to get
> this working instead of a PCI quirk. To get some background please
> see the thread for commit below that you also had signed off as
> well.

I applied 8b5742ad156d because it's something all arches should do
(actually, we *should* do it in the PCI core, but nobody's gotten
around to doing that yet). It has nothing to do with Keystone
support, and it doesn't mean I'm in favor of a boot argument.

I think the discussion you mentioned is [1]. I see hints that there
might be a Keystone hardware defect related to MRSS, but I don't see a
clear description of it. If you have a hardware erratum document,
those usually contain pretty good descriptions.

If there is a hardware defect, a PCI quirk is a reasonable way to work
around it, since that's the main purpose of quirks. fixup_mpss_256()
is an example of something that sounds superficially similar.

I don't think there's a way for a device to advertise the maximum MRSS
value it supports. MRSS only controls the maximum Read Request size
the device can generate, and I wouldn't think there's much to go wrong
there, because the request doesn't contain any data, so MRSS doesn't
affect the packet size of the *request*.

I think it's more likely that a hardware problem would affect the
*response*, where, e.g., a device might advertise (via the Device
Capabilities Max_Payload_Size_Supported field) that it can support an
MPS of 1024, but it can't actually handle a TLP that big. Software
would have to work around that by artificially limiting the MPS to
something smaller than the MPSS advertised by the device. This is
what fixup_mpss_256() is doing.

If there is a hardware problem with MRSS specifically, you can
probably still do a quirk, but it might also involve a little work in
the PCI core to add something similar to pcie_mpss to support the
quirk.

Bjorn

[1] http://lkml.kernel.org/r/[email protected]

> commit 8b5742ad156d30ee38486652cdbd152e2d6ebbcc
> Author: Murali Karicheri <[email protected]>
> Date: Wed May 28 13:14:53 2014 -0400
>
> ARM/PCI: Call pcie_bus_configure_settings() to set MPS
>
> Call pcie_bus_configure_settings() on ARM, like for other platforms.
> pcie_bus_configure_settings() makes sure the MPS across the bus
> is uniform
> and provides the ability to tune the MRSS and MPS to higher performance
> values. This is particularly important for embedded where there is no
> firmware to program these PCIe settings for the OS.
>
> Signed-off-by: Murali Karicheri <[email protected]>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> CC: Russell King <[email protected]>
> CC: Arnd Bergmann <[email protected]>
> CC: Jason Gunthorpe <[email protected]>
> CC: Santosh Shilimkar <[email protected]>
>
> This was added as a preparatory patch to support keystone and
> avoid a PCI quirk to do the same. Keystone has MRSS limitation
> of 256 bytes. So adding a bootargs flag was suggested a better
> option than a PCI quirk.
>
> I will look into the rest of the comments and possibly try to
> address them or discuss.
>
> BTW, please apply patch 1-3 that has already got ack from maintainers
> and is indepdent of this patch.
>
> Thanks
>
> Murali
>
> >
> >>diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> >>index 21df477..f8bc475 100644
> >>--- a/drivers/pci/host/Kconfig
> >>+++ b/drivers/pci/host/Kconfig
> >>@@ -46,4 +46,9 @@ config PCI_HOST_GENERIC
> >> Say Y here if you want to support a simple generic PCI host
> >> controller, such as the one emulated by kvmtool.
> >>
> >>+config PCI_KEYSTONE
> >>+ bool "TI Keystone PCIe controller"
> >>+ depends on ARCH_KEYSTONE
> >>+ select PCIE_DW
> >>+ select PCIEPORTBUS
> >
> >It'd be nice to have some help text here. I know, not everybody else does.
> >
> >>+++ b/drivers/pci/host/pci-keystone-dw.c
> >>...
> >>+void ks_dw_pcie_handle_msi_irq(struct keystone_pcie *ks_pcie , int offset)
> >>+{
> >>+ struct pcie_port *pp =&ks_pcie->pp;
> >>+ u32 pending, vector;
> >>+ int src, virq;
> >>+
> >>+ pending = readl(ks_pcie->va_app_base + MSI0_IRQ_STATUS + (offset<< 4));
> >
> >Blank line here (before the block comment).
> >
> >>+ /*
> >>+ * MSI0, Status bit 0-3 shows vectors 0, 8, 16, 24, MSI1 status bit
> >>+ * shows 1, 9, 17, 25 and so forth
> >>+ */
> >>+ for (src = 0; src< 4; src++) {
> >>+ if (BIT(src)& pending) {
> >>+ vector = offset + (src<< 3);
> >>+ virq = irq_linear_revmap(pp->irq_domain, vector);
> >>+ dev_dbg(pp->dev,
> >>+ "irq: bit %d, vector %d, virq %d\n",
> >>+ src, vector, virq);
> >>+ generic_handle_irq(virq);
> >>+ }
> >>+ }
> >>+}
> >>+
> >>...
> >
> >>+static void __iomem *ks_pcie_cfg_setup(struct keystone_pcie *ks_pcie, u8 bus,
> >>+ unsigned int devfn)
> >>+{
> >>+ u8 device = PCI_SLOT(devfn), function = PCI_FUNC(devfn);
> >>+ struct pcie_port *pp =&ks_pcie->pp;
> >>+ u32 regval;
> >>+
> >>+ if (bus == 0)
> >>+ return pp->dbi_base;
> >>+
> >>+ regval = (bus<< 16) | (device<< 8) | function;
> >>+ /*
> >>+ * Since Bus#1 will be a virtual bus, we need to have TYPE0
> >>+ * access only.
> >>+ * TYPE 1
> >>+ */
> >>+ if (bus != 1)
> >>+ regval |= BIT(24);
> >>+
> >>+ writel(regval, ks_pcie->va_app_base + CFG_SETUP);
> >>+ return pp->va_cfg0_base;
> >>+}
> >>+
> >>+int ks_dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
> >>+ unsigned int devfn, int where, int size, u32 *val)
> >>+{
> >>+ struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
> >>+ u8 bus_num = bus->number;
> >>+ void __iomem *addr;
> >>+ int ret;
> >>+
> >>+ addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn);
> >>+ ret = dw_pcie_cfg_read(addr + (where& ~0x3), where, size, val);
> >
> >This *looks* like it needs a lock to protect against concurrent
> >ks_pcie_cfg_setup() users, since it writes a register.
> >
> >>+
> >>+ return ret;
> >
> >Please use the same style as in ks_dw_pcie_wr_other_conf(), i.e., get rid
> >of "ret".
> >
> >>+}
> >>+
> >>+int ks_dw_pcie_wr_other_conf(struct pcie_port *pp,
> >>+ struct pci_bus *bus, unsigned int devfn, int where,
> >>+ int size, u32 val)
> >>+{
> >>+ struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
> >>+ u8 bus_num = bus->number;
> >>+ void __iomem *addr;
> >>+
> >>+ addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn);
> >>+
> >>+ return dw_pcie_cfg_write(addr + (where& ~0x3), where, size, val);
> >>+}
> >
> >>+++ b/drivers/pci/host/pci-keystone.c
> >>...
> >
> >>+static struct platform_driver ks_pcie_driver __refdata = {
> >
> >Why does this need to be __refdata? There are no other occurrences in
> >drivers/pci.
> >
> >>+ .probe = ks_pcie_probe,
> >>+ .remove = __exit_p(ks_pcie_remove),
> >>+ .driver = {
> >>+ .name = "keystone-pcie",
> >>+ .owner = THIS_MODULE,
> >>+ .of_match_table = of_match_ptr(ks_pcie_of_match),
> >>+ },
> >>+};
> >
> >Bjorn
>

2014-07-23 01:27:34

by Jingoo Han

[permalink] [raw]
Subject: Re: [PATCH v7 3/5] PCI: designware: enhance dw_pcie_host_init() to support v3.65 DW hardware

On Tuesday, July 22, 2014 1:59 AM, Murali Karicheri wrote:
>
> keystone PCI controller is based on v3.65 designware hardware. This
> version differs from newer versions of the hardware in few functional
> areas discussed below that makes it necessary to change dw_pcie_host_init()
> to support v3.65 based PCI controller.
>
> 1. No support for ATU port. So any ATU specific resource handling code
> is to be bypassed for v3.65 h/w.
> 2. MSI controller uses Application space to implement MSI and 32 MSI
> interrupts are multiplexed over 8 IRQs to the host. Hence the code
> to process MSI IRQ needs to be different. This patch allows platform
> driver to provide its own irq_domain_ops ptr to irq_domain_add_linear()
> through an API callback from the designware core driver.
> 3. MSI interrupt generation requires EP to write to the RC's application
> register. So enhance the driver to allow setup of inbound access to
> MSI irq register as a post scan bus API callback.
>
> Signed-off-by: Murali Karicheri <[email protected]>
> Reviewed-by: Pratyush Anand <[email protected]>
> Acked-by: Mohit KUMAR <[email protected]>
>
> CC: Santosh Shilimkar <[email protected]>
> CC: Russell King <[email protected]>
> CC: Grant Likely <[email protected]>
> CC: Rob Herring <[email protected]>
> CC: Jingoo Han <[email protected]>

Acked-by: Jingoo Han <[email protected]>

Best regards,
Jingoo Han

> CC: Bjorn Helgaas <[email protected]>
> CC: Richard Zhu <[email protected]>
> CC: Kishon Vijay Abraham I <[email protected]>
> CC: Marek Vasut <[email protected]>
> CC: Arnd Bergmann <[email protected]>
> CC: Pawel Moll <[email protected]>
> CC: Mark Rutland <[email protected]>
> CC: Ian Campbell <[email protected]>
> CC: Kumar Gala <[email protected]>
> CC: Randy Dunlap <[email protected]>
> CC: Grant Likely <[email protected]>
> ---
> drivers/pci/host/pcie-designware.c | 54 +++++++++++++++++++++++-------------
> drivers/pci/host/pcie-designware.h | 2 ++
> 2 files changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 905941c..35bb4af 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -420,8 +420,8 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> struct device_node *np = pp->dev->of_node;
> struct of_pci_range range;
> struct of_pci_range_parser parser;
> + int i, ret;
> u32 val;
> - int i;
>
> if (of_pci_range_parser_init(&parser, np)) {
> dev_err(pp->dev, "missing ranges property\n");
> @@ -467,21 +467,26 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> }
> }
>
> - pp->cfg0_base = pp->cfg.start;
> - pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
> pp->mem_base = pp->mem.start;
>
> - pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base,
> - pp->config.cfg0_size);
> if (!pp->va_cfg0_base) {
> - dev_err(pp->dev, "error with ioremap in function\n");
> - return -ENOMEM;
> + pp->cfg0_base = pp->cfg.start;
> + pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base,
> + pp->config.cfg0_size);
> + if (!pp->va_cfg0_base) {
> + dev_err(pp->dev, "error with ioremap in function\n");
> + return -ENOMEM;
> + }
> }
> - pp->va_cfg1_base = devm_ioremap(pp->dev, pp->cfg1_base,
> - pp->config.cfg1_size);
> +
> if (!pp->va_cfg1_base) {
> - dev_err(pp->dev, "error with ioremap\n");
> - return -ENOMEM;
> + pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
> + pp->va_cfg1_base = devm_ioremap(pp->dev, pp->cfg1_base,
> + pp->config.cfg1_size);
> + if (!pp->va_cfg1_base) {
> + dev_err(pp->dev, "error with ioremap\n");
> + return -ENOMEM;
> + }
> }
>
> if (of_property_read_u32(np, "num-lanes", &pp->lanes)) {
> @@ -490,16 +495,22 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> }
>
> if (IS_ENABLED(CONFIG_PCI_MSI)) {
> - pp->irq_domain = irq_domain_add_linear(pp->dev->of_node,
> - MAX_MSI_IRQS, &msi_domain_ops,
> - &dw_pcie_msi_chip);
> - if (!pp->irq_domain) {
> - dev_err(pp->dev, "irq domain init failed\n");
> - return -ENXIO;
> - }
> + if (!pp->ops->msi_host_init) {
> + pp->irq_domain = irq_domain_add_linear(pp->dev->of_node,
> + MAX_MSI_IRQS, &msi_domain_ops,
> + &dw_pcie_msi_chip);
> + if (!pp->irq_domain) {
> + dev_err(pp->dev, "irq domain init failed\n");
> + return -ENXIO;
> + }
>
> - for (i = 0; i < MAX_MSI_IRQS; i++)
> - irq_create_mapping(pp->irq_domain, i);
> + for (i = 0; i < MAX_MSI_IRQS; i++)
> + irq_create_mapping(pp->irq_domain, i);
> + } else {
> + ret = pp->ops->msi_host_init(pp, &dw_pcie_msi_chip);
> + if (ret < 0)
> + return ret;
> + }
> }
>
> if (pp->ops->host_init)
> @@ -759,6 +770,9 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> BUG();
> }
>
> + if (bus && pp->ops->scan_bus)
> + pp->ops->scan_bus(pp);
> +
> return bus;
> }
>
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index 387f69e..080c649 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -70,6 +70,8 @@ struct pcie_host_ops {
> void (*msi_set_irq)(struct pcie_port *pp, int irq);
> void (*msi_clear_irq)(struct pcie_port *pp, int irq);
> u32 (*get_msi_data)(struct pcie_port *pp);
> + void (*scan_bus)(struct pcie_port *pp);
> + int (*msi_host_init)(struct pcie_port *pp, struct msi_chip *chip);
> };
>
> int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val);
> --
> 1.7.9.5

2014-07-23 15:28:23

by Karicheri, Muralidharan

[permalink] [raw]
Subject: Re: [PATCH v7 0/5] Add Keystone PCIe controller driver


Bjorn,

On 07/22/2014 06:57 PM, Bjorn Helgaas wrote:
> On Mon, Jul 21, 2014 at 12:58:40PM -0400, Murali Karicheri wrote:

>> Murali Karicheri (5):
>> PCI: designware: add rd[wr]_other_conf API
>> PCI: designware: refactor MSI code to work with v3.65 dw hardware
>
> I applied the above two to my pci/host-designware branch. The rest didn't
> apply cleanly because I applied Kishon's DRA7xx changes first, and there
> are several conflicts. Can you rebase the rest of them on top of
> pci/host-designware?

Thanks for applying 1 and 2.

I will fix up 3 based on your branch and send you updated patch 3 and 4
today. Hope you can apply this updated patch so that I don't have to
address any rebase issues. Regarding the MRSS comment, I will discuss it
on that thread and send you a fix for removing the bootargs depedency
based on what comes out of that discussion. Is that fine?

Regards,

Murali

>
>> PCI: designware: enhance dw_pcie_host_init() to support v3.65 DW
>> hardware
>> PCI: add PCI controller for keystone PCIe h/w
>> PCI: keystone: Update maintainer information
>
> You can squash the MAINTAINERS update into the driver addition. They're
> logically part of the same change.
>
> Bjorn
>
>> .../devicetree/bindings/pci/pci-keystone.txt | 68 +++
>> MAINTAINERS | 7 +
>> drivers/pci/host/Kconfig | 5 +
>> drivers/pci/host/Makefile | 1 +
>> drivers/pci/host/pci-keystone-dw.c | 521 ++++++++++++++++++++
>> drivers/pci/host/pci-keystone.c | 386 +++++++++++++++
>> drivers/pci/host/pci-keystone.h | 58 +++
>> drivers/pci/host/pcie-designware.c | 116 +++--
>> drivers/pci/host/pcie-designware.h | 9 +
>> 9 files changed, 1135 insertions(+), 36 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/pci/pci-keystone.txt
>> create mode 100644 drivers/pci/host/pci-keystone-dw.c
>> create mode 100644 drivers/pci/host/pci-keystone.c
>> create mode 100644 drivers/pci/host/pci-keystone.h
>>
>> --
>> 1.7.9.5
>>

2014-07-23 16:44:10

by Bjorn Helgaas

[permalink] [raw]
Subject: Re: [PATCH v7 0/5] Add Keystone PCIe controller driver

On Wed, Jul 23, 2014 at 9:27 AM, Murali Karicheri <[email protected]> wrote:
>
> Bjorn,
>
>
> On 07/22/2014 06:57 PM, Bjorn Helgaas wrote:
>>
>> On Mon, Jul 21, 2014 at 12:58:40PM -0400, Murali Karicheri wrote:
>
>
>>> Murali Karicheri (5):
>>> PCI: designware: add rd[wr]_other_conf API
>>> PCI: designware: refactor MSI code to work with v3.65 dw hardware
>>
>>
>> I applied the above two to my pci/host-designware branch. The rest didn't
>> apply cleanly because I applied Kishon's DRA7xx changes first, and there
>> are several conflicts. Can you rebase the rest of them on top of
>> pci/host-designware?
>
>
> Thanks for applying 1 and 2.
>
> I will fix up 3 based on your branch and send you updated patch 3 and 4
> today. Hope you can apply this updated patch so that I don't have to
> address any rebase issues. Regarding the MRSS comment, I will discuss it on
> that thread and send you a fix for removing the bootargs depedency based on
> what comes out of that discussion. Is that fine?

Yep, sounds reasonable. I'm about to leave for vacation, so I won't
be able to apply these until mid-August, but I think we can still get
them into v3.17 since it's new hardware that shouldn't affect anything
else.

For MRSS, I'll probably open a bugzilla and reference it in the
changelog just so we don't forget about it.

>>> PCI: designware: enhance dw_pcie_host_init() to support v3.65 DW
>>> hardware
>>> PCI: add PCI controller for keystone PCIe h/w
>>> PCI: keystone: Update maintainer information
>>
>>
>> You can squash the MAINTAINERS update into the driver addition. They're
>> logically part of the same change.
>>
>> Bjorn
>>
>>> .../devicetree/bindings/pci/pci-keystone.txt | 68 +++
>>> MAINTAINERS | 7 +
>>> drivers/pci/host/Kconfig | 5 +
>>> drivers/pci/host/Makefile | 1 +
>>> drivers/pci/host/pci-keystone-dw.c | 521
>>> ++++++++++++++++++++
>>> drivers/pci/host/pci-keystone.c | 386
>>> +++++++++++++++
>>> drivers/pci/host/pci-keystone.h | 58 +++
>>> drivers/pci/host/pcie-designware.c | 116 +++--
>>> drivers/pci/host/pcie-designware.h | 9 +
>>> 9 files changed, 1135 insertions(+), 36 deletions(-)
>>> create mode 100644
>>> Documentation/devicetree/bindings/pci/pci-keystone.txt
>>> create mode 100644 drivers/pci/host/pci-keystone-dw.c
>>> create mode 100644 drivers/pci/host/pci-keystone.c
>>> create mode 100644 drivers/pci/host/pci-keystone.h
>>>
>>> --
>>> 1.7.9.5
>>>
>

2014-07-23 16:57:54

by Karicheri, Muralidharan

[permalink] [raw]
Subject: Re: [PATCH v7 0/5] Add Keystone PCIe controller driver

On 07/23/2014 12:43 PM, Bjorn Helgaas wrote:
> On Wed, Jul 23, 2014 at 9:27 AM, Murali Karicheri<[email protected]> wrote:
>>
>> Bjorn,
>>
>>
>> On 07/22/2014 06:57 PM, Bjorn Helgaas wrote:
>>>
>>> On Mon, Jul 21, 2014 at 12:58:40PM -0400, Murali Karicheri wrote:
>>
>>
>>>> Murali Karicheri (5):
>>>> PCI: designware: add rd[wr]_other_conf API
>>>> PCI: designware: refactor MSI code to work with v3.65 dw hardware
>>>
>>>
>>> I applied the above two to my pci/host-designware branch. The rest didn't
>>> apply cleanly because I applied Kishon's DRA7xx changes first, and there
>>> are several conflicts. Can you rebase the rest of them on top of
>>> pci/host-designware?
>>
>>
>> Thanks for applying 1 and 2.
>>
>> I will fix up 3 based on your branch and send you updated patch 3 and 4
>> today. Hope you can apply this updated patch so that I don't have to
>> address any rebase issues. Regarding the MRSS comment, I will discuss it on
>> that thread and send you a fix for removing the bootargs depedency based on
>> what comes out of that discussion. Is that fine?
>
> Yep, sounds reasonable. I'm about to leave for vacation, so I won't
> be able to apply these until mid-August, but I think we can still get
> them into v3.17 since it's new hardware that shouldn't affect anything
> else.
>
Ok Thanks
> For MRSS, I'll probably open a bugzilla and reference it in the
> changelog just so we don't forget about it.
Not done it before. Will check and do this.

Regards,

Murali

>
>>>> PCI: designware: enhance dw_pcie_host_init() to support v3.65 DW
>>>> hardware
>>>> PCI: add PCI controller for keystone PCIe h/w
>>>> PCI: keystone: Update maintainer information
>>>
>>>
>>> You can squash the MAINTAINERS update into the driver addition. They're
>>> logically part of the same change.
>>>
>>> Bjorn
>>>
>>>> .../devicetree/bindings/pci/pci-keystone.txt | 68 +++
>>>> MAINTAINERS | 7 +
>>>> drivers/pci/host/Kconfig | 5 +
>>>> drivers/pci/host/Makefile | 1 +
>>>> drivers/pci/host/pci-keystone-dw.c | 521
>>>> ++++++++++++++++++++
>>>> drivers/pci/host/pci-keystone.c | 386
>>>> +++++++++++++++
>>>> drivers/pci/host/pci-keystone.h | 58 +++
>>>> drivers/pci/host/pcie-designware.c | 116 +++--
>>>> drivers/pci/host/pcie-designware.h | 9 +
>>>> 9 files changed, 1135 insertions(+), 36 deletions(-)
>>>> create mode 100644
>>>> Documentation/devicetree/bindings/pci/pci-keystone.txt
>>>> create mode 100644 drivers/pci/host/pci-keystone-dw.c
>>>> create mode 100644 drivers/pci/host/pci-keystone.c
>>>> create mode 100644 drivers/pci/host/pci-keystone.h
>>>>
>>>> --
>>>> 1.7.9.5
>>>>
>>

2014-07-23 17:43:08

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH v7 4/5] PCI: add PCI controller for keystone PCIe h/w

On Tue, Jul 22, 2014 at 05:52:00PM -0600, Bjorn Helgaas wrote:
> If there is a hardware defect, a PCI quirk is a reasonable way to work
> around it, since that's the main purpose of quirks. fixup_mpss_256()
> is an example of something that sounds superficially similar.

It was my suggestion to engage the PCI-E tuning code. By my
understanding the HW bug is that read response segmentation at the
host bridge does not work - so all read requests from any downstream
device must have responses that fit within a single packet.

This is completely against how the spec envisions things working,
segmentation is a mandatory function. As you point out there is no
parameter bounding the maximum read request size that a completer will
accept.

So, the only fix is that every downstream device must always have a
MRSS set to less than the MPS of the host bridge.

Which means the tuning code must be involved somehow, as that code
controls the MRSS of unrelated devices...

Regards,
Jason

2014-07-30 19:36:10

by Karicheri, Muralidharan

[permalink] [raw]
Subject: Re: [PATCH v7 4/5] PCI: add PCI controller for keystone PCIe h/w

On 07/23/2014 01:42 PM, Jason Gunthorpe wrote:
> On Tue, Jul 22, 2014 at 05:52:00PM -0600, Bjorn Helgaas wrote:
>> If there is a hardware defect, a PCI quirk is a reasonable way to work
>> around it, since that's the main purpose of quirks. fixup_mpss_256()
>> is an example of something that sounds superficially similar.
>
> It was my suggestion to engage the PCI-E tuning code. By my
> understanding the HW bug is that read response segmentation at the
> host bridge does not work - so all read requests from any downstream
> device must have responses that fit within a single packet.
>

In the case of Keystone PCI, when I set the MRSS to 256 in the EP, PCI
controller is able to function properly. Keystone spec says.

? Maximum outbound payload size of 128 bytes
? Maximum inbound payload size of 256 bytes
? Maximum remote read request size of 256 bytes

I am interpreting the "Maximum remote read request size" to indicate it
can's handle if it exceeds the limit. It has an outbound payload size of
128 bytes. So in this case a read request would results in 2 completion
packets. So it seems to be able to segment up to maximum 256 bytes of
read request. Where do I find the requirement in PCI spec that "read
response segmentation at the host bridge does not work" ?

> This is completely against how the spec envisions things working,
> segmentation is a mandatory function. As you point out there is no
> parameter bounding the maximum read request size that a completer will
> accept.
>
> So, the only fix is that every downstream device must always have a
> MRSS set to less than the MPS of the host bridge.

Why this can't be the default behavior in the PCI core? Any cons?

Murali
>
> Which means the tuning code must be involved somehow, as that code
> controls the MRSS of unrelated devices...
>
> Regards,
> Jason

2014-07-30 20:05:56

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH v7 4/5] PCI: add PCI controller for keystone PCIe h/w

On Wed, Jul 30, 2014 at 03:34:44PM -0400, Murali Karicheri wrote:

> • Maximum remote read request size of 256 bytes

The PCI spec simply does not permit a completer to impose this
limitation.

See spec 2.3.1.1

It is not an option to error a read request because it is too
long. All requests must be completed. All completions must be segmented
according to the RCB and Max_Payload_Size.

> completion packets. So it seems to be able to segment up to maximum
> 256 bytes of read request. Where do I find the requirement in PCI
> spec that "read response segmentation at the host bridge does not
> work" ?

You just said it. Segmenting up to only 256 bytes is not a limitation
the completer can impose. Such a device is non-conformant.

> >So, the only fix is that every downstream device must always have a
> >MRSS set to less than the MPS of the host bridge.
>
> Why this can't be the default behavior in the PCI core? Any cons?

It increases read request traffic and read response latency on the bus
for large transfers.

The MRSS is intented as a performance tuning knob, it is not something
that ever needs to be set properly for correct bus operation.

Jason