From: Bjorn Helgaas <bhelgaas@google.com>
To: Murali Karicheri <m-karicheri2@ti.com>
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
Santosh Shilimkar <santosh.shilimkar@ti.com>,
Russell King <linux@arm.linux.org.uk>,
Grant Likely <grant.likely@linaro.org>,
Rob Herring <robh+dt@kernel.org>,
Jingoo Han <jg1.han@samsung.com>,
Richard Zhu <r65037@freescale.com>,
Kishon Vijay Abraham I <kishon@ti.com>,
Marek Vasut <marex@denx.de>, Arnd Bergmann <arnd@arndb.de>,
Pawel Moll <pawel.moll@arm.com>,
Mark Rutland <mark.rutland@arm.com>,
Ian Campbell <ijc+devicetree@hellion.org.uk>,
Kumar Gala <galak@codeaurora.org>,
Randy Dunlap <rdunlap@infradead.org>
Subject: Re: [PATCH v7 4/5] PCI: add PCI controller for keystone PCIe h/w
Date: Tue, 22 Jul 2014 16:35:27 -0600 [thread overview]
Message-ID: <20140722223527.GA27965@google.com> (raw)
In-Reply-To: <1405961925-27248-5-git-send-email-m-karicheri2@ti.com>
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@ti.com>
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.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
next prev parent reply other threads:[~2014-07-22 22:35 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-21 16:58 [PATCH v7 0/5] Add Keystone PCIe controller driver Murali Karicheri
2014-07-21 16:58 ` [PATCH v7 1/5] PCI: designware: add rd[wr]_other_conf API Murali Karicheri
2014-07-21 16:58 ` [PATCH v7 2/5] PCI: designware: refactor MSI code to work with v3.65 dw hardware Murali Karicheri
2014-07-21 16:58 ` [PATCH v7 3/5] PCI: designware: enhance dw_pcie_host_init() to support v3.65 DW hardware Murali Karicheri
2014-07-23 1:27 ` Jingoo Han
2014-07-21 16:58 ` [PATCH v7 4/5] PCI: add PCI controller for keystone PCIe h/w Murali Karicheri
2014-07-22 22:35 ` Bjorn Helgaas [this message]
2014-07-22 22:52 ` Murali Karicheri
2014-07-22 23:52 ` Bjorn Helgaas
2014-07-23 17:42 ` Jason Gunthorpe
2014-07-30 19:34 ` Murali Karicheri
2014-07-30 20:05 ` Jason Gunthorpe
2014-08-06 14:38 ` Murali Karicheri
2014-07-21 16:58 ` [PATCH v7 5/5] PCI: keystone: Update maintainer information Murali Karicheri
2014-07-22 22:57 ` [PATCH v7 0/5] Add Keystone PCIe controller driver Bjorn Helgaas
2014-07-23 15:27 ` Murali Karicheri
2014-07-23 16:43 ` Bjorn Helgaas
2014-07-23 16:56 ` Murali Karicheri
2014-08-18 14:58 ` Murali Karicheri
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140722223527.GA27965@google.com \
--to=bhelgaas@google.com \
--cc=arnd@arndb.de \
--cc=galak@codeaurora.org \
--cc=grant.likely@linaro.org \
--cc=ijc+devicetree@hellion.org.uk \
--cc=jg1.han@samsung.com \
--cc=kishon@ti.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux@arm.linux.org.uk \
--cc=m-karicheri2@ti.com \
--cc=marex@denx.de \
--cc=mark.rutland@arm.com \
--cc=pawel.moll@arm.com \
--cc=r65037@freescale.com \
--cc=rdunlap@infradead.org \
--cc=robh+dt@kernel.org \
--cc=santosh.shilimkar@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).