Re: [PATCH v11 3/6] pci:host: Add Altera PCIe host controller driver

[Ley Foon Tan]

On Fri, Oct 23, 2015 at 1:31 PM, Bjorn Helgaas  wrote:
> Hi Ley,
>
> On Thu, Oct 22, 2015 at 05:27:28PM +0800, Ley Foon Tan wrote:
>> This patch adds the Altera PCIe host controller driver.
>
>> +static void altera_pcie_fixups(struct pci_bus *bus)
>> +{
>> + struct pci_dev *dev;
>> +
>> + list_for_each_entry(dev, &bus->devices, bus_list) {
>> + altera_pcie_retrain(dev);
>> + altera_pcie_fixup_res(dev);
>> + }
>> +}
>
> I'd really like to avoid this particular fixup because it's done
> between pci_scan_root_bus() and pci_assign_unassigned_bus_resources()
> and pci_bus_add_devices().  That path is almost 100% arch-independent,
> and someday we should be able to pull all that out into one PCI core
> interface.
>
> You might be able to do the link retrain fixup as a header quirk.
> That's not really ideal either, but I don't think we have a good
> mechanism of inserting per-host bridge hooks in the enumeration path.
> There are some pcibios_*() hooks, but those are per-arch, not per-host
> bridge, so they're not really what you want here.
Okay, will change the retrain fixup to use *PCI_FIXUP* macro.
By doing this, we need [PATCH v11 2/6] pci: add Altera PCI vendor ID patch.

>
> I think other host drivers have handled the "prevent enumeration of
> root complex resources" problem by adding a similar check in the
> config accessors.
Okay, will handle this in config accessors.

>
>> +static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
>> +  int where, int size, u32 value)
>
> This needs a comment to the effect that this hardware can only generate
> 32-bit config accesses.  We also need a printk in the probe routine so
> there's a note in dmesg so we have a clue that RW1C bits in config space
> may be corrupted.
I have checked the PCIe/TLP spec, we can use the "First BE" (byte
enable) field in TLP packet to write
specific bytes only. And I have update driver to support this "First
BE" feature.
So, we don't have corrupted RW1C bit issue now.

>
>> +{
>> + struct altera_pcie *pcie = bus->sysdata;
>> + u32 data32;
>> + u32 shift = 8 * (where & 3);
>> + u8 byte_en;
>> +
>> + if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
>> + return PCIBIOS_DEVICE_NOT_FOUND;
>> +
>> + switch (size) {
>> + case 1:
>> + data32 = (value & 0xff) << shift;
>> + byte_en = 1 << (where & 3);
>> + break;
>> + case 2:
>> + data32 = (value & 0x) << shift;
>> + byte_en = 3 << (where & 3);
>> + break;
>> + default:
>> + data32 = value;
>> + byte_en = 0xf;
>> + break;
>> + }
>> +
>> + return tlp_cfg_dword_write(pcie, bus->number, devfn,
>> + (where & ~DWORD_MASK), byte_en, data32);
>> +}
>
>> +static void altera_pcie_isr(struct irq_desc *desc)
>> +{
>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>> + struct altera_pcie *pcie;
>> + unsigned long status;
>> + u32 bit;
>> + u32 virq;
>> +
>> + chained_irq_enter(chip, desc);
>> + pcie = irq_desc_get_handler_data(desc);
>> +
>> + while ((status = cra_readl(pcie, P2A_INT_STATUS)
>> + & P2A_INT_STS_ALL) != 0) {
>> + for_each_set_bit(bit, &status, INTX_NUM) {
>> + /* clear interrupts */
>> + cra_writel(pcie, 1 << bit, P2A_INT_STATUS);
>> +
>> + virq = irq_find_mapping(pcie->irq_domain, bit + 1);
>> + if (virq)
>> + generic_handle_irq(virq);
>> + else
>> + dev_err(&pcie->pdev->dev, "unexpected IRQ\n");
>
> Include the bit number here.  A printk string with no % substitutions
> is rarely as useful as it could be.
Okay.
>
>> ...
>> + bus = pci_scan_root_bus(&pdev->dev, pcie->root_bus_nr, 
>> &altera_pcie_ops,
>> + pcie, &pcie->resources);
>> + if (!bus)
>> + return -ENOMEM;
>> +
>> + altera_pcie_fixups(bus);
>> + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
>> + pci_assign_unassigned_bus_resources(bus);
>> + pci_bus_add_devices(bus);
>> +
>> + /* Configure PCI Express setting. */
>> + list_for_each_entry(child, &bus->children, node)
>> + pcie_bus_configure_settings(child);
>
> This loop should be before pci_bus_add_devices().  When we call
> pci_bus_add_devices(), drivers may claim devices immediately, and the
> PCI core shouldn't be changing device configuration while a driver
> owns the device.
Okay, will move this before pci_bus_add_devices().

Thanks.

Regards
Ley Foon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v11 3/6] pci:host: Add Altera PCIe host controller driver

[Bjorn Helgaas]

Hi Ley,

On Thu, Oct 22, 2015 at 05:27:28PM +0800, Ley Foon Tan wrote:
> This patch adds the Altera PCIe host controller driver.

> +static void altera_pcie_fixups(struct pci_bus *bus)
> +{
> + struct pci_dev *dev;
> +
> + list_for_each_entry(dev, &bus->devices, bus_list) {
> + altera_pcie_retrain(dev);
> + altera_pcie_fixup_res(dev);
> + }
> +}

I'd really like to avoid this particular fixup because it's done
between pci_scan_root_bus() and pci_assign_unassigned_bus_resources()
and pci_bus_add_devices().  That path is almost 100% arch-independent,
and someday we should be able to pull all that out into one PCI core
interface.

You might be able to do the link retrain fixup as a header quirk.
That's not really ideal either, but I don't think we have a good
mechanism of inserting per-host bridge hooks in the enumeration path.
There are some pcibios_*() hooks, but those are per-arch, not per-host
bridge, so they're not really what you want here.

I think other host drivers have handled the "prevent enumeration of
root complex resources" problem by adding a similar check in the
config accessors.

> +static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
> +  int where, int size, u32 value)

This needs a comment to the effect that this hardware can only generate
32-bit config accesses.  We also need a printk in the probe routine so
there's a note in dmesg so we have a clue that RW1C bits in config space
may be corrupted.

> +{
> + struct altera_pcie *pcie = bus->sysdata;
> + u32 data32;
> + u32 shift = 8 * (where & 3);
> + u8 byte_en;
> +
> + if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
> + return PCIBIOS_DEVICE_NOT_FOUND;
> +
> + switch (size) {
> + case 1:
> + data32 = (value & 0xff) << shift;
> + byte_en = 1 << (where & 3);
> + break;
> + case 2:
> + data32 = (value & 0x) << shift;
> + byte_en = 3 << (where & 3);
> + break;
> + default:
> + data32 = value;
> + byte_en = 0xf;
> + break;
> + }
> +
> + return tlp_cfg_dword_write(pcie, bus->number, devfn,
> + (where & ~DWORD_MASK), byte_en, data32);
> +}

> +static void altera_pcie_isr(struct irq_desc *desc)
> +{
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct altera_pcie *pcie;
> + unsigned long status;
> + u32 bit;
> + u32 virq;
> +
> + chained_irq_enter(chip, desc);
> + pcie = irq_desc_get_handler_data(desc);
> +
> + while ((status = cra_readl(pcie, P2A_INT_STATUS)
> + & P2A_INT_STS_ALL) != 0) {
> + for_each_set_bit(bit, &status, INTX_NUM) {
> + /* clear interrupts */
> + cra_writel(pcie, 1 << bit, P2A_INT_STATUS);
> +
> + virq = irq_find_mapping(pcie->irq_domain, bit + 1);
> + if (virq)
> + generic_handle_irq(virq);
> + else
> + dev_err(&pcie->pdev->dev, "unexpected IRQ\n");

Include the bit number here.  A printk string with no % substitutions
is rarely as useful as it could be.

> ...
> + bus = pci_scan_root_bus(&pdev->dev, pcie->root_bus_nr, &altera_pcie_ops,
> + pcie, &pcie->resources);
> + if (!bus)
> + return -ENOMEM;
> +
> + altera_pcie_fixups(bus);
> + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> + pci_assign_unassigned_bus_resources(bus);
> + pci_bus_add_devices(bus);
> +
> + /* Configure PCI Express setting. */
> + list_for_each_entry(child, &bus->children, node)
> + pcie_bus_configure_settings(child);

This loop should be before pci_bus_add_devices().  When we call
pci_bus_add_devices(), drivers may claim devices immediately, and the
PCI core shouldn't be changing device configuration while a driver
owns the device.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v11 3/6] pci:host: Add Altera PCIe host controller driver

[Ley Foon Tan]

This patch adds the Altera PCIe host controller driver.

Signed-off-by: Ley Foon Tan 
Reviewed-by: Marc Zyngier 
---
 drivers/pci/host/Kconfig   |   8 +
 drivers/pci/host/Makefile  |   1 +
 drivers/pci/host/pcie-altera.c | 579 +
 3 files changed, 588 insertions(+)
 create mode 100644 drivers/pci/host/pcie-altera.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index d5e58ba..a67c9de 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -145,4 +145,12 @@ config PCIE_IPROC_BCMA
  Say Y here if you want to use the Broadcom iProc PCIe controller
  through the BCMA bus interface
 
+config PCIE_ALTERA
+   tristate "Altera PCIe controller"
+   depends on ARM || NIOS2
+   select PCI_DOMAINS
+   help
+ Say Y here if you want to enable PCIe controller support on Altera
+ FPGA.
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 140d66f..6954f76 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
 obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
 obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
 obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
+obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
new file mode 100644
index 000..3503d9c
--- /dev/null
+++ b/drivers/pci/host/pcie-altera.c
@@ -0,0 +1,579 @@
+/*
+ * Copyright Altera Corporation (C) 2013-2015. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define RP_TX_REG0 0x2000
+#define RP_TX_REG1 0x2004
+#define RP_TX_CNTRL0x2008
+#define RP_TX_EOP  0x2
+#define RP_TX_SOP  0x1
+#define RP_RXCPL_STATUS0x2010
+#define RP_RXCPL_EOP   0x2
+#define RP_RXCPL_SOP   0x1
+#define RP_RXCPL_REG0  0x2014
+#define RP_RXCPL_REG1  0x2018
+#define P2A_INT_STATUS 0x3060
+#define P2A_INT_STS_ALL0xf
+#define P2A_INT_ENABLE 0x3070
+#define P2A_INT_ENA_ALL0xf
+#define RP_LTSSM   0x3c64
+#define LTSSM_L0   0xf
+
+/* TLP configuration type 0 and 1 */
+#define TLP_FMTTYPE_CFGRD0 0x04/* Configuration Read Type 0 */
+#define TLP_FMTTYPE_CFGWR0 0x44/* Configuration Write Type 0 */
+#define TLP_FMTTYPE_CFGRD1 0x05/* Configuration Read Type 1 */
+#define TLP_FMTTYPE_CFGWR1 0x45/* Configuration Write Type 1 */
+#define TLP_PAYLOAD_SIZE   0x01
+#define TLP_READ_TAG   0x1d
+#define TLP_WRITE_TAG  0x10
+#define TLP_CFG_DW0(fmttype)   (((fmttype) << 24) | TLP_PAYLOAD_SIZE)
+#define TLP_CFG_DW1(reqid, tag, be)(((reqid) << 16) | (tag << 8) | (be))
+#define TLP_CFG_DW2(bus, devfn, offset)\
+   (((bus) << 24) | ((devfn) << 16) | (offset))
+#define TLP_REQ_ID(bus, devfn) (((bus) << 8) | (devfn))
+#define TLP_HDR_SIZE   3
+#define TLP_LOOP   500
+
+#define INTX_NUM   4
+
+#define DWORD_MASK 3
+
+struct altera_pcie {
+   struct platform_device  *pdev;
+   void __iomem*cra_base;
+   int irq;
+   u8  root_bus_nr;
+   struct irq_domain   *irq_domain;
+   struct resource bus_range;
+   struct list_headresources;
+};
+
+struct tlp_rp_regpair_t {
+   u32 ctrl;
+   u32 reg0;
+   u32 reg1;
+};
+
+static void altera_pcie_retrain(struct pci_dev *dev)
+{
+   u16 linkcap, linkstat;
+
+   /*
+* Set the retrain bit if the PCIe rootport support > 2.5GB/s, but
+* current speed is 2.5 GB/s.
+*/
+   pcie_capability_read_word(dev, PCI_EXP_LNKCAP, &linkcap);
+
+   if ((linkcap & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB)
+   return;
+
+   pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &linkstat);
+