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

[Bjorn Helgaas <bhelgaas@google.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