| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140722223527.GA27965@google.com>
Date: Tue, 22 Jul 2014 16:35:27 -0600
From: Bjorn Helgaas <bhelgaas@...gle.com>
To: Murali Karicheri <m-karicheri2@...com>
Cc: linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
Santosh Shilimkar <santosh.shilimkar@...com>,
Russell King <linux@....linux.org.uk>,
Grant Likely <grant.likely@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Jingoo Han <jg1.han@...sung.com>,
Richard Zhu <r65037@...escale.com>,
Kishon Vijay Abraham I <kishon@...com>,
Marek Vasut <marex@...x.de>, Arnd Bergmann <arnd@...db.de>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Randy Dunlap <rdunlap@...radead.org>
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 <m-karicheri2@...com>
> Acked-by: Santosh Shilimkar <santosh.shilimkar@...com>
> ...
> +++ 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists