[PATCH V6 02/17] ST SPEAr13xx: Added PCIe host controller base driver support.

[pratyush]

Hello Arnd,

On 3/1/2011 8:40 PM, Arnd Bergmann wrote:
> On Tuesday 01 March 2011, Viresh Kumar wrote:
>> From: Pratyush Anand <pratyush.anand at st.com>
> 
> Hi Viresh and Pratyush,
> 
> The code looks really nice, I have just a few comments, mostly
> about pointer type conversion.
>  
>> SPEAr13xx family contains Synopsys designware PCIe version 3.30a. This
>> patch adds support for this PCIe module for spear platform.
> 
> If this is a standard PCIe controller, why add it to the platform
> code instead of a common place like arch/arch/common or arch/arm/kernel ?
> 

Yes, It is for Synopsys IP , but there are ST specific changes for accessing
HW resources.

>> diff --git a/arch/arm/mach-spear13xx/include/mach/hardware.h b/arch/arm/mach-spear13xx/include/mach/hardware.h
>> index fd8c2dc..c3fb454 100644
>> --- a/arch/arm/mach-spear13xx/include/mach/hardware.h
>> +++ b/arch/arm/mach-spear13xx/include/mach/hardware.h
>> @@ -28,4 +28,11 @@
>>  /* typesafe io address */
>>  #define __io_address(n)		__io(IO_ADDRESS(n))
> 
> I could not find the definition for __io() here, but I suspect this is
> wrong. If __io_address() is what you use for accessing the direct-mapped
> MMIO registers, it cannot also be what you use to access the PCIe PIO
> ports, so most likely one of the two is broken. Can you explain?
> 
>> +#if defined(CONFIG_PCI)
>> +#define PCIBIOS_MIN_IO		0
>> +#define PCIBIOS_MIN_MEM		0
>> +#define pcibios_assign_all_busses()	0
>> +#endif
>> +
> 
> No need for the #ifdef here, if you have no PCI, there won't be a conflict.
> 
> Better make PCIBIOS_MIN_IO 0x1000, in order to avoid stepping over
> ISA port ranges.
> 

In fact , I do not need these defines. I will remove them.

>> +static u32 spr_pcie_base[NUM_PCIE_PORTS] = {
>> +	SPEAR13XX_PCIE0_BASE,
>> +	SPEAR13XX_PCIE1_BASE,
>> +	SPEAR13XX_PCIE2_BASE,
>> +};
>> +static u32 spr_pcie_app_base[NUM_PCIE_PORTS] = {
>> +	SPEAR13XX_PCIE0_APP_BASE,
>> +	SPEAR13XX_PCIE1_APP_BASE,
>> +	SPEAR13XX_PCIE2_APP_BASE,
>> +};
> 
> I think these should be __iomem pointers, not u32 tokens, since you
> are listing virtual addresses. If you unroll the loop in
> spear13xx_pcie_init() that uses these, you can actually get rid
> of the two arrays, and simplify the code used there at the
> same time.
> 

will do it.

>> +#ifdef CONFIG_PCI_MSI
>> +static DECLARE_BITMAP(msi_irq_in_use[NUM_PCIE_PORTS], SPEAR_NUM_MSI_IRQS);
>> +static unsigned int spear_msi_data[NUM_PCIE_PORTS];
>> +
>> +static void spear13xx_msi_init(struct pcie_port *pp);
>> +#endif
>> +
>> +static void spear_pcie_int_handler(unsigned int irq, struct irq_desc *desc);
> 
> It would be nice if you could avoid the forward declarations by reordering
> the functions if possible.
> 

will do it.

>> +static int __init spear13xx_pcie_setup(int nr, struct pci_sys_data *sys)
>> +{
>> +	struct pcie_port *pp;
>> +	u32 val = 0;
>> +
>> +	if (nr >= NUM_PCIE_PORTS)
>> +		return 0;
>> +
>> +	if ((*pcie_port_is_host)(nr) != 1)
>> +		return 0;
>> +
>> +	pp = &pcie_port[nr];
>> +	if (!spear13xx_pcie_link_up((void __iomem *)pp->va_app_base))
>> +		return 0;
> 
> No need for the cast, va_app_base is already (void __iomem *).
> 

ok.

>> +static void __init add_pcie_port(int port, u32 base, u32 app_base)
>> +{
>> +	struct pcie_port *pp = &pcie_port[port];
>> +	struct pcie_app_reg *app_reg;
>> +
>> +	pp->port = port;
>> +	pp->root_bus_nr = -1;
>> +	pp->base = (void __iomem *)base;
>> +	pp->app_base = (void __iomem *)app_base;
>> +	pp->va_app_base = (void __iomem *) ioremap(app_base, 0x200);
>> +	if (!pp->va_app_base) {
>> +		pr_err("error with ioremap in function %s\n", __func__);
>> +		return;
>> +	}
>> +	pp->va_dbi_base = (void __iomem *) ioremap(base, 0x2000);
>> +	if (!pp->va_dbi_base) {
>> +		pr_err("error with ioremap in function %s\n", __func__);
>> +		return;
>> +	}
> 
> Please remove all these casts. Some are unneeded, some can go away
> after the things I mention above.
> 

ok.

>> +	spin_lock_init(&pp->conf_lock);
>> +	memset(pp->res, 0, sizeof(pp->res));
>> +	pr_info("spear13xx PCIe port %d\n", port);
>> +	if (spear13xx_pcie_link_up((void __iomem *)pp->va_app_base)) {
>> +		pr_info("link up in bios\n");
>> +	} else {
>> +		pr_info("link down in bios\n");
>> +		spear13xx_pcie_host_init(pp);
>> +		spear13xx_int_init(pp);
>> +		app_reg = (struct pcie_app_reg *)pp->va_app_base;
> 
> This cast looks invalid, you cast from __iomem to a regular pointer,
> 

will typecast it with (struct pcie_app_reg __iomem*)

>> +		pp->va_cfg0_base = (void __iomem *)
>> +			ioremap(app_reg->in_cfg0_addr_start, IN_CFG0_SIZE);
> 
> which breaks here when you pass the value to ioremap without doing a readl.
> 

will modify.

>> +#ifdef CONFIG_PCI_MSI
>> +/* MSI int handler
>> + */
>> +static void handle_msi(struct pcie_port *pp)
>> +{
>> +	unsigned long val;
>> +	int i, pos;
>> +
>> +	for (i = 0; i < 8; i++) {
>> +		spear_dbi_read_reg(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
>> +				(u32 *)&val);
>> +		if (val) {
>> +			pos = 0;
>> +			while ((pos = find_next_bit(&val, 32, pos)) != 32) {
>> +				generic_handle_irq(SPEAR_MSI0_INT_BASE
>> +					+ pp->port * SPEAR_NUM_MSI_IRQS
>> +					+ (i * 32) + pos);
>> +				pos++;
>> +			}
>> +		}
>> +		spear_dbi_write_reg(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4, val);
>> +	}
>> +}
>> +#else
>> +static void handle_msi(struct pcie_port *pp)
>> +{
>> +}
>> +#endif
> 
> The MSI code is not big, but I'd still recommend moving it to a separate
> file, which gets compiled only when CONFIG_PCI_MSI is set. You can have
> the #ifdef and inline NOP alternative in a header for this.
> 

will put it in a separate file.

Regards
Pratyush


> 	Arnd
> .
>