Re: PCI Express support for 2.4 kernel

[Greg KH <greg@kroah.com>]

Minor code comments below:

On Tue, Dec 16, 2003 at 12:20:39PM +0200, Vladimir Kondratiev wrote:
> +
> +#ifdef CONFIG_PCI_EXPRESS
> +#define PCI_PROBE_EXP		0x0008
> +#endif
> +

If you change this to:
#ifdef CONFIG_PCI_EXPRESS
#define PCI_PROBE_EXP		0x0008
#else
#define PCI_PROBE_EXP		0x0000
#endif

You can get rid of a number of "#ifdef CONFIG_PCI_EXPRESS" statements
later on in the .c files by just always using the PCI_PROBE_EXP value.
That cleans up the patch a bit.

> -unsigned int pci_probe = PCI_PROBE_BIOS | PCI_PROBE_CONF1 | PCI_PROBE_CONF2;
> +unsigned int pci_probe = PCI_PROBE_BIOS | PCI_PROBE_CONF1 | PCI_PROBE_CONF2
> +#ifdef CONFIG_PCI_EXPRESS
> + | PCI_PROBE_EXP
> +#endif
> +;

Like right there :)

> +/**
> + * RRBAR (memory base for PCI-E config space) resides here.
> + * Initialized to default address. Actually, it is platform specific, and
> + * value may vary.
> + * I don't know how to detect it properly, it is chipset specific.
> + */
> +static u32 rrbar_phys = CONFIG_PCI_EXPRESS_BASE * 0x10000000UL;

<snip>

> +/**
> + * I don't know how to detect it properly.
> + * assume it is PCI-E, sanity_check will
> + * stop me if it is not.
> + * 
> + * Also, this function supposed to set rrbar_phys
> + */
> +static int is_pcie_platform(void)
> +{ return 1; }

Shouldn't your comments at least match your code for the rrbar_phys
statement for your first release?  :)

> +/**
> + * Initializes PCI Express method for config space access.
> + * 
> + * There is no standard method to recognize presence of PCI Express,
> + * thus we will assume it is PCI-E, and rely on sanity check to
> + * deassert PCI-E presense. If PCI-E not present,
> + * there is no physical RAM on RRBAR address, and we should read
> + * something like 0xff.
> + * 
> + * @return 1 if OK, 0 if error
> + */
> +static int pci_express_init(void)
> +{
> +	rrbar_window_virt = (void*)fix_to_virt(FIX_PCIE_CONFIG);
> +	if (!is_pcie_platform())
> +		return 0;

Call fix_to_virt() after you do the check and not before?


> +/**
> + * Shuts down PCI-E resources.
> + */
> +static inline void pci_express_fini(void)
> +{}

If this isn't needed, why have it anymore?

> +static struct pci_ops pci_express_conf = {
> +	pci_exp_read_config_byte,
> +	pci_exp_read_config_word,
> +	pci_exp_read_config_dword,
> +	pci_exp_write_config_byte,
> +	pci_exp_write_config_word,
> +	pci_exp_write_config_dword
> +};

C99 initializers here?

> +			printk(KERN_INFO "PCI-Express config at 0x%08x\n", rrbar_phys);

"%p" to show the address might be nicer.


thanks,

greg k-h