Re: [PATCH 0/2] PCI-X/PCI-Express read control interfaces

[Andrew Vasquez <andrew.vasquez@qlogic.com>]

On Tue, 15 May 2007, Andrew Morton wrote:

> On Tue, 15 May 2007 13:50:27 +0200
> "Peter Oruba" <peter.oruba@amd.com> wrote:
> 
> > This patch set introduces a PCI-X / PCI-Express read byte count control 
> > interface. Instead of letting every driver to directly read/write to PCI 
> > config space for that, an interface is provided. The interface functions then 
> > can be used for quirks since some PCI bridges require that read byte count 
> > values are set by the BIOS and left unchanged by device drivers.
> 
> Some of the patches were wordwrapped, which I fixed.
> 
> The way we would merge a feature like this is
> 
> - get maintainers to review-and-ack the change

This is definetly good cleanup, and I ACK the QLogic changes.

I do though have some questions on call prerequisites given the
driver-changes, most in the form of:

> diff -uprN -X linux-2.6.22-rc1.orig/Documentation/dontdiff 
> linux-2.6.22-rc1.orig/drivers/infiniband/hw/mthca/mthca_main.c 
> linux-2.6.22-rc1/drivers/infiniband/hw/mthca/mthca_main.c
> --- linux-2.6.22-rc1.orig/drivers/infiniband/hw/mthca/mthca_main.c	2007-05-14 
> 11:29:29.358547000 +0200
> +++ linux-2.6.22-rc1/drivers/infiniband/hw/mthca/mthca_main.c	2007-05-15 
> 10:55:24.954074000 +0200
> @@ -137,45 +137,27 @@ static const char mthca_version[] __devi
>  
>  static int mthca_tune_pci(struct mthca_dev *mdev)
>  {
> -	int cap;
> -	u16 val;
> -
>  	if (!tune_pci)
>  		return 0;
>  
>  	/* First try to max out Read Byte Count */
> -	cap = pci_find_capability(mdev->pdev, PCI_CAP_ID_PCIX);
> -	if (cap) {
> -		if (pci_read_config_word(mdev->pdev, cap + PCI_X_CMD, &val)) {
> -			mthca_err(mdev, "Couldn't read PCI-X command register, "
> -				  "aborting.\n");
> -			return -ENODEV;
> -		}
> -		val = (val & ~PCI_X_CMD_MAX_READ) | (3 << 2);
> -		if (pci_write_config_word(mdev->pdev, cap + PCI_X_CMD, val)) {
> -			mthca_err(mdev, "Couldn't write PCI-X command register, "
> -				  "aborting.\n");
> +	if (pci_find_capability(mdev->pdev, PCI_CAP_ID_PCIX)) {
> +		if (pcix_set_mmrbc(mdev->pdev, pcix_get_max_mmrbc(mdev->pdev))) {
> +			mthca_err(mdev, "Couldn't set PCI-X max read count, "
> +				"aborting.\n");
...
> -	cap = pci_find_capability(mdev->pdev, PCI_CAP_ID_EXP);
> -	if (cap) {
> -		if (pci_read_config_word(mdev->pdev, cap + PCI_EXP_DEVCTL, &val)) {
> -			mthca_err(mdev, "Couldn't read PCI Express device control "
> -				  "register, aborting.\n");
> -			return -ENODEV;
> -		}
> -		val = (val & ~PCI_EXP_DEVCTL_READRQ) | (5 << 12);
> -		if (pci_write_config_word(mdev->pdev, cap + PCI_EXP_DEVCTL, val)) {
> -			mthca_err(mdev, "Couldn't write PCI Express device control "
> -				  "register, aborting.\n");
> +	if (pci_find_capability(mdev->pdev, PCI_CAP_ID_EXP)) {
> +		if (pcie_set_readrq(mdev->pdev, 4096)) {
> +			mthca_err(mdev, "Couldn't write PCI Express read request, "
> +				"aborting.\n");


In general, if PCI-[Xe] capability structure exists do set-
mmrbc()/readrq(), yet each of those pre-condition checks are already
present in the pcix_set_mmrbc() and pcie_set_readrq().

At least for the qla2xxx case, the patch could easily distill down from:

	...
	/* PCIe -- adjust Maximum Read Request Size (2048). */
	pcie_dctl_reg = pci_find_capability(ha->pdev, PCI_CAP_ID_EXP);
	if (pcie_dctl_reg)
		if (pcie_set_readrq(ha->pdev, 2048))
			DEBUG2(printk("Couldn't write PCI Express read request\n"));

to:

	...
	pcie_set_readrq(ha->pdev, 2048);


Whatever the decision, I can fold this into my next patchset for
qla2xxx and submit.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/