| 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: <53CEEB1C.9020202@ti.com>
Date: Tue, 22 Jul 2014 18:52:12 -0400
From: Murali Karicheri <m-karicheri2@...com>
To: Bjorn Helgaas <bhelgaas@...gle.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
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<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.
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 <m-karicheri2@...com>
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 <m-karicheri2@...com>
Signed-off-by: Bjorn Helgaas <bhelgaas@...gle.com>
CC: Russell King <linux@....linux.org.uk>
CC: Arnd Bergmann <arnd@...db.de>
CC: Jason Gunthorpe <jgunthorpe@...idianresearch.com>
CC: Santosh Shilimkar <santosh.shilimkar@...com>
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
--
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