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.
Patch 1/2: PCI subsystem
Patch 2/2: affected drivers
Both patches are based on work by Stephen Hemminger, please see
http://lkml.org/lkml/2006/12/8/222
Regards,
Peter Oruba
--
AMD Saxony Limited Liability Company & Co. KG
Operating System Research Center
Wilschdorfer Landstr. 101, 01109 Dresden, Germany
Register Court Dresden: HRA 4896
General Partner authorized to represent:
AMD Saxony LLC (Wilmington, Delaware, US)
General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy
This patch introduces an interface to read and write PCI-X / PCI-Express
maximum read byte count values from PCI config space. There is a second
function that returns the maximum _designed_ read byte count, which marks the
maximum value for a device, since some drivers try to set MMRBC to the
highest allowed value and rely on such a function.
Signed-off by: Peter Oruba <[email protected]>
Based on patch set by Stephen Hemminger <[email protected]>
---
diff -uprN -X linux-2.6.22-rc1.orig/Documentation/dontdiff
linux-2.6.22-rc1.orig/drivers/pci/pci.c linux-2.6.22-rc1/drivers/pci/pci.c
--- linux-2.6.22-rc1.orig/drivers/pci/pci.c 2007-05-14 11:29:39.473498000
+0200
+++ linux-2.6.22-rc1/drivers/pci/pci.c 2007-05-15 11:44:33.804254000 +0200
@@ -1375,6 +1375,170 @@ pci_set_consistent_dma_mask(struct pci_d
#endif
/**
+ * pcix_get_max_mmrbc - get PCI-X maximum designed memory read byte count
+ * @dev: PCI device to query
+ *
+ * Returns mmrbc: maximum designed memory read count in bytes
+ * or appropriate error value.
+ */
+int
+pcix_get_max_mmrbc(struct pci_dev *dev)
+{
+ int ret, err, cap;
+ u32 stat;
+
+ cap = pci_find_capability(dev, PCI_CAP_ID_PCIX);
+ if (!cap)
+ return -EINVAL;
+
+ err = pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat);
+ if (err)
+ return -EINVAL;
+
+ ret = (stat & PCI_X_STATUS_MAX_READ) >> 12;
+
+ return ret;
+}
+EXPORT_SYMBOL(pcix_get_max_mmrbc);
+
+/**
+ * pcix_get_mmrbc - get PCI-X maximum memory read byte count
+ * @dev: PCI device to query
+ *
+ * Returns mmrbc: maximum memory read count in bytes
+ * or appropriate error value.
+ */
+int
+pcix_get_mmrbc(struct pci_dev *dev)
+{
+ int ret, cap;
+ u32 cmd;
+
+ cap = pci_find_capability(dev, PCI_CAP_ID_PCIX);
+ if (!cap)
+ return -EINVAL;
+
+ ret = pci_read_config_dword(dev, cap + PCI_X_CMD, &cmd);
+ if (!ret)
+ ret = 512 << ((cmd & PCI_X_CMD_MAX_READ) >> 2);
+
+ return ret;
+}
+EXPORT_SYMBOL(pcix_get_mmrbc);
+
+/**
+ * pcix_set_mmrbc - set PCI-X maximum memory read byte count
+ * @dev: PCI device to query
+ * @mmrbc: maximum memory read count in bytes
+ * valid values are 512, 1024, 2048, 4096
+ *
+ * If possible sets maximum memory read byte count, some bridges have erratas
+ * that prevent this.
+ */
+int
+pcix_set_mmrbc(struct pci_dev *dev, int mmrbc)
+{
+ int cap, err = -EINVAL;
+ u32 stat, cmd, v, o;
+
+ if (mmrbc < 512 || mmrbc > 4096 || (mmrbc & (mmrbc-1)))
+ goto out;
+
+ v = ffs(mmrbc) - 10;
+
+ cap = pci_find_capability(dev, PCI_CAP_ID_PCIX);
+ if (!cap)
+ goto out;
+
+ err = pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat);
+ if (err)
+ goto out;
+
+ if (v > (stat & PCI_X_STATUS_MAX_READ) >> 21)
+ return -E2BIG;
+
+ err = pci_read_config_dword(dev, cap + PCI_X_CMD, &cmd);
+ if (err)
+ goto out;
+
+ o = (cmd & PCI_X_CMD_MAX_READ) >> 2;
+ if (o != v) {
+ if (v > o && dev->bus &&
+ (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MMRBC))
+ return -EIO;
+
+ cmd &= ~PCI_X_CMD_MAX_READ;
+ cmd |= v << 2;
+ err = pci_write_config_dword(dev, cap + PCI_X_CMD, cmd);
+ }
+out:
+ return err;
+}
+EXPORT_SYMBOL(pcix_set_mmrbc);
+
+/**
+ * pcie_get_readrq - get PCI Express read request size
+ * @dev: PCI device to query
+ *
+ * Returns maximum memory read request in bytes
+ * or appropriate error value.
+ */
+int pcie_get_readrq(struct pci_dev *dev)
+{
+ int ret, cap;
+ u16 ctl;
+
+ cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
+ if (!cap)
+ return -EINVAL;
+
+ ret = pci_read_config_word(dev, cap + PCI_EXP_DEVCTL, &ctl);
+ if (!ret)
+ ret = 128 << ((ctl & PCI_EXP_DEVCTL_READRQ) >> 12);
+
+ return ret;
+}
+EXPORT_SYMBOL(pcie_get_readrq);
+
+/**
+ * pcie_set_readrq - set PCI Express maximum memory read request
+ * @dev: PCI device to query
+ * @count: maximum memory read count in bytes
+ * valid values are 128, 256, 512, 1024, 2048, 4096
+ *
+ * If possible sets maximum read byte count
+ */
+int
+pcie_set_readrq(struct pci_dev *dev, int rq)
+{
+ int cap, err = -EINVAL;
+ u16 ctl, v;
+
+ if (rq < 128 || rq > 4096 || (rq & (rq-1)))
+ goto out;
+
+ v = (ffs(rq) - 8) << 12;
+
+ cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
+ if (!cap)
+ goto out;
+
+ err = pci_read_config_word(dev, cap + PCI_EXP_DEVCTL, &ctl);
+ if (err)
+ goto out;
+
+ if ((ctl & PCI_EXP_DEVCTL_READRQ) != v) {
+ ctl &= ~PCI_EXP_DEVCTL_READRQ;
+ ctl |= v;
+ err = pci_write_config_dword(dev, cap + PCI_EXP_DEVCTL, ctl);
+ }
+
+out:
+ return err;
+}
+EXPORT_SYMBOL(pcie_set_readrq);
+
+/**
* pci_select_bars - Make BAR mask from the type of resource
* @dev: the PCI device for which BAR mask is made
* @flags: resource type mask to be selected
diff -uprN -X linux-2.6.22-rc1.orig/Documentation/dontdiff
linux-2.6.22-rc1.orig/drivers/pci/quirks.c
linux-2.6.22-rc1/drivers/pci/quirks.c
--- linux-2.6.22-rc1.orig/drivers/pci/quirks.c 2007-05-14 11:29:39.596501000
+0200
+++ linux-2.6.22-rc1/drivers/pci/quirks.c 2007-05-15 10:07:43.342427000 +0200
@@ -627,6 +627,22 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AM
DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_8131_BRIDGE,
quirk_amd_8131_ioapic);
#endif /* CONFIG_X86_IO_APIC */
+/*
+ * Some settings of MMRBC can lead to data corruption so block changes.
+ * See AMD 8131 HyperTransport PCI-X Tunnel Revision Guide
+ */
+static void __init quirk_amd_8131_mmrbc(struct pci_dev *dev)
+{
+ unsigned char revid;
+
+ pci_read_config_byte(dev, PCI_REVISION_ID, &revid);
+ if (dev->subordinate && revid <= 0x12) {
+ printk(KERN_INFO "AMD8131 rev %x detected, disabling PCI-X MMRBC\n",
+ revid);
+ dev->subordinate->bus_flags |= PCI_BUS_FLAGS_NO_MMRBC;
+ }
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_8131_BRIDGE,
quirk_amd_8131_mmrbc);
/*
* FIXME: it is questionable that quirk_via_acpi
diff -uprN -X linux-2.6.22-rc1.orig/Documentation/dontdiff
linux-2.6.22-rc1.orig/include/linux/pci.h
linux-2.6.22-rc1/include/linux/pci.h
--- linux-2.6.22-rc1.orig/include/linux/pci.h 2007-05-14 11:29:55.025995000
+0200
+++ linux-2.6.22-rc1/include/linux/pci.h 2007-05-15 11:34:26.602039000 +0200
@@ -111,7 +111,8 @@ enum pcie_reset_state {
typedef unsigned short __bitwise pci_bus_flags_t;
enum pci_bus_flags {
- PCI_BUS_FLAGS_NO_MSI = (__force pci_bus_flags_t) 1,
+ PCI_BUS_FLAGS_NO_MSI = (__force pci_bus_flags_t) 1,
+ PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
};
struct pci_cap_saved_state {
@@ -549,6 +550,10 @@ void pci_intx(struct pci_dev *dev, int e
void pci_msi_off(struct pci_dev *dev);
int pci_set_dma_mask(struct pci_dev *dev, u64 mask);
int pci_set_consistent_dma_mask(struct pci_dev *dev, u64 mask);
+int pcix_get_max_mmrbc(struct pci_dev *dev);
+int pcix_get_mmrbc(struct pci_dev *dev);
+int pcix_set_mmrbc(struct pci_dev *dev, int mmrbc);
+int pcie_set_readrq(struct pci_dev *dev, int rq);
void pci_update_resource(struct pci_dev *dev, struct resource *res, int
resno);
int __must_check pci_assign_resource(struct pci_dev *dev, int i);
int __must_check pci_assign_resource_fixed(struct pci_dev *dev, int i);
---
--
AMD Saxony Limited Liability Company & Co. KG
Operating System Research Center
Wilschdorfer Landstr. 101, 01109 Dresden, Germany
Register Court Dresden: HRA 4896
General Partner authorized to represent:
AMD Saxony LLC (Wilmington, Delaware, US)
General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy
These driver changes incorporate the proposed PCI-X / PCI-Express read byte
count interface. Reading and setting those valuse doesn't take
place "manually", instead wrapping functions are called to allow quirks for
some PCI bridges.
Signed-off by: Peter Oruba <[email protected]>
Based on work by Stephen Hemminger <[email protected]>
---
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");
return -ENODEV;
}
} else if (!(mdev->mthca_flags & MTHCA_FLAG_PCIE))
mthca_info(mdev, "No PCI-X capability, not setting RBC.\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");
return -ENODEV;
}
- } else if (mdev->mthca_flags & MTHCA_FLAG_PCIE)
- mthca_info(mdev, "No PCI Express capability, "
- "not setting Max Read Request Size.\n");
+ } else if (!(mdev->mthca_flags & MTHCA_FLAG_PCIE))
+ mthca_info(mdev, "No PCI-X capability, not setting RBC.\n");
return 0;
}
diff -uprN -X linux-2.6.22-rc1.orig/Documentation/dontdiff
linux-2.6.22-rc1.orig/drivers/net/e1000/e1000_hw.c
linux-2.6.22-rc1/drivers/net/e1000/e1000_hw.c
--- linux-2.6.22-rc1.orig/drivers/net/e1000/e1000_hw.c 2007-04-26
05:08:32.000000000 +0200
+++ linux-2.6.22-rc1/drivers/net/e1000/e1000_hw.c 2007-05-15
11:07:24.726991000 +0200
@@ -865,10 +865,6 @@ e1000_init_hw(struct e1000_hw *hw)
uint32_t ctrl;
uint32_t i;
int32_t ret_val;
- uint16_t pcix_cmd_word;
- uint16_t pcix_stat_hi_word;
- uint16_t cmd_mmrbc;
- uint16_t stat_mmrbc;
uint32_t mta_size;
uint32_t reg_data;
uint32_t ctrl_ext;
@@ -958,24 +954,9 @@ e1000_init_hw(struct e1000_hw *hw)
break;
default:
/* Workaround for PCI-X problem when BIOS sets MMRBC incorrectly. */
- if (hw->bus_type == e1000_bus_type_pcix) {
- e1000_read_pci_cfg(hw, PCIX_COMMAND_REGISTER, &pcix_cmd_word);
- e1000_read_pci_cfg(hw, PCIX_STATUS_REGISTER_HI,
- &pcix_stat_hi_word);
- cmd_mmrbc = (pcix_cmd_word & PCIX_COMMAND_MMRBC_MASK) >>
- PCIX_COMMAND_MMRBC_SHIFT;
- stat_mmrbc = (pcix_stat_hi_word & PCIX_STATUS_HI_MMRBC_MASK) >>
- PCIX_STATUS_HI_MMRBC_SHIFT;
- if (stat_mmrbc == PCIX_STATUS_HI_MMRBC_4K)
- stat_mmrbc = PCIX_STATUS_HI_MMRBC_2K;
- if (cmd_mmrbc > stat_mmrbc) {
- pcix_cmd_word &= ~PCIX_COMMAND_MMRBC_MASK;
- pcix_cmd_word |= stat_mmrbc << PCIX_COMMAND_MMRBC_SHIFT;
- e1000_write_pci_cfg(hw, PCIX_COMMAND_REGISTER,
- &pcix_cmd_word);
- }
- }
- break;
+ if (hw->bus_type == e1000_bus_type_pcix && e1000_pcix_get_mmrbc(hw) > 2048)
+ e1000_pcix_set_mmrbc(hw, 2048);
+ break;
}
/* More time needed for PHY to initialize */
diff -uprN -X linux-2.6.22-rc1.orig/Documentation/dontdiff
linux-2.6.22-rc1.orig/drivers/net/e1000/e1000_hw.h
linux-2.6.22-rc1/drivers/net/e1000/e1000_hw.h
--- linux-2.6.22-rc1.orig/drivers/net/e1000/e1000_hw.h 2007-04-26
05:08:32.000000000 +0200
+++ linux-2.6.22-rc1/drivers/net/e1000/e1000_hw.h 2007-05-15
11:10:17.115782000 +0200
@@ -424,6 +424,8 @@ void e1000_pci_clear_mwi(struct e1000_hw
void e1000_read_pci_cfg(struct e1000_hw *hw, uint32_t reg, uint16_t * value);
void e1000_write_pci_cfg(struct e1000_hw *hw, uint32_t reg, uint16_t *
value);
int32_t e1000_read_pcie_cap_reg(struct e1000_hw *hw, uint32_t reg, uint16_t
*value);
+void e1000_pcix_set_mmrbc(struct e1000_hw *hw, int mmrbc);
+int e1000_pcix_get_mmrbc(struct e1000_hw *hw);
/* Port I/O is only supported on 82544 and newer */
void e1000_io_write(struct e1000_hw *hw, unsigned long port, uint32_t value);
int32_t e1000_disable_pciex_master(struct e1000_hw *hw);
diff -uprN -X linux-2.6.22-rc1.orig/Documentation/dontdiff
linux-2.6.22-rc1.orig/drivers/net/e1000/e1000_main.c
linux-2.6.22-rc1/drivers/net/e1000/e1000_main.c
--- linux-2.6.22-rc1.orig/drivers/net/e1000/e1000_main.c 2007-05-14
11:29:35.324875000 +0200
+++ linux-2.6.22-rc1/drivers/net/e1000/e1000_main.c 2007-05-15
11:10:18.358679000 +0200
@@ -4912,6 +4912,20 @@ e1000_write_pci_cfg(struct e1000_hw *hw,
pci_write_config_word(adapter->pdev, reg, *value);
}
+int
+e1000_pcix_get_mmrbc(struct e1000_hw *hw)
+{
+ struct e1000_adapter *adapter = hw->back;
+ return pcix_get_mmrbc(adapter->pdev);
+}
+
+void
+e1000_pcix_set_mmrbc(struct e1000_hw *hw, int mmrbc)
+{
+ struct e1000_adapter *adapter = hw->back;
+ pcix_set_mmrbc(adapter->pdev, mmrbc);
+}
+
int32_t
e1000_read_pcie_cap_reg(struct e1000_hw *hw, uint32_t reg, uint16_t *value)
{
diff -uprN -X linux-2.6.22-rc1.orig/Documentation/dontdiff
linux-2.6.22-rc1.orig/drivers/net/myri10ge/myri10ge.c
linux-2.6.22-rc1/drivers/net/myri10ge/myri10ge.c
--- linux-2.6.22-rc1.orig/drivers/net/myri10ge/myri10ge.c 2007-05-14
11:29:36.235781000 +0200
+++ linux-2.6.22-rc1/drivers/net/myri10ge/myri10ge.c 2007-05-15
11:10:18.974579000 +0200
@@ -2873,8 +2873,7 @@ static int myri10ge_probe(struct pci_dev
status);
goto abort_with_netdev;
}
- val = (val & ~PCI_EXP_DEVCTL_READRQ) | (5 << 12);
- status = pci_write_config_word(pdev, cap + PCI_EXP_DEVCTL, val);
+ status = pcie_set_readrq(pdev, 4096);
if (status != 0) {
dev_err(&pdev->dev, "Error %d writing PCI_EXP_DEVCTL\n",
status);
diff -uprN -X linux-2.6.22-rc1.orig/Documentation/dontdiff
linux-2.6.22-rc1.orig/drivers/scsi/qla2xxx/qla_init.c
linux-2.6.22-rc1/drivers/scsi/qla2xxx/qla_init.c
--- linux-2.6.22-rc1.orig/drivers/scsi/qla2xxx/qla_init.c 2007-05-14
11:29:41.561311000 +0200
+++ linux-2.6.22-rc1/drivers/scsi/qla2xxx/qla_init.c 2007-05-15
10:39:29.636747000 +0200
@@ -281,15 +281,9 @@ qla24xx_pci_config(scsi_qla_host_t *ha)
/* PCIe -- adjust Maximum Read Request Size (2048). */
pcie_dctl_reg = pci_find_capability(ha->pdev, PCI_CAP_ID_EXP);
- if (pcie_dctl_reg) {
- uint16_t pcie_dctl;
-
- pcie_dctl_reg += PCI_EXP_DEVCTL;
- pci_read_config_word(ha->pdev, pcie_dctl_reg, &pcie_dctl);
- pcie_dctl &= ~PCI_EXP_DEVCTL_READRQ;
- pcie_dctl |= 0x4000;
- pci_write_config_word(ha->pdev, pcie_dctl_reg, pcie_dctl);
- }
+ if (pcie_dctl_reg)
+ if (pcie_set_readrq(ha->pdev, 2048))
+ DEBUG2(printk("Couldn't write PCI Express read request\n"));
/* Reset expansion ROM address decode enable */
pci_read_config_dword(ha->pdev, PCI_ROM_ADDRESS, &d);
---
--
AMD Saxony Limited Liability Company & Co. KG
Operating System Research Center
Wilschdorfer Landstr. 101, 01109 Dresden, Germany
Register Court Dresden: HRA 4896
General Partner authorized to represent:
AMD Saxony LLC (Wilmington, Delaware, US)
General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy
On Tue, 15 May 2007 13:50:27 +0200
"Peter Oruba" <[email protected]> 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
- merge the core patch into Greg's PCI tree and later into
mainline.
- Once the base infrastructure is in mainline, feed the per-driver
changes into the tree via the appropriate maintainers.
This takes, umm, months and consumes quite a bit of my time. I'm becoming
inclined just to slam stuff like this straight in as you've proposed, but
for now, let's play the game - I split the patches up appropriately. I
don't think there's any particular urgency behind this, is there?
On 5/15/07, Peter Oruba <[email protected]> wrote:
> These driver changes incorporate the proposed PCI-X / PCI-Express read byte
> count interface. Reading and setting those valuse doesn't take
> place "manually", instead wrapping functions are called to allow quirks for
> some PCI bridges.
>
> Signed-off by: Peter Oruba <[email protected]>
> Based on work by Stephen Hemminger <[email protected]>
>
Ack.
--
Cheers,
Jeff Kirsher
Andrew Morton wrote:
> On Tue, 15 May 2007 13:50:27 +0200
> "Peter Oruba" <[email protected]> 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
>
> - merge the core patch into Greg's PCI tree and later into
> mainline.
>
> - Once the base infrastructure is in mainline, feed the per-driver
> changes into the tree via the appropriate maintainers.
>
> This takes, umm, months and consumes quite a bit of my time. I'm becoming
> inclined just to slam stuff like this straight in as you've proposed, but
> for now, let's play the game - I split the patches up appropriately. I
> don't think there's any particular urgency behind this, is there?
While Jeff just Acked the changes to e1000-specific for me, I think this is a
good approach and would appreciate it. I'll be happy to pull the change and push
it through in due time.
Auke
On Tue, 15 May 2007, Andrew Morton wrote:
> On Tue, 15 May 2007 13:50:27 +0200
> "Peter Oruba" <[email protected]> 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.
Am Dienstag, 15. Mai 2007 21:37:21 schrieb Andrew Morton:
> On Tue, 15 May 2007 13:50:27 +0200
>
> "Peter Oruba" <[email protected]> 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
>
> - merge the core patch into Greg's PCI tree and later into
> mainline.
>
> - Once the base infrastructure is in mainline, feed the per-driver
> changes into the tree via the appropriate maintainers.
>
> This takes, umm, months and consumes quite a bit of my time. I'm becoming
> inclined just to slam stuff like this straight in as you've proposed, but
> for now, let's play the game - I split the patches up appropriately. I
> don't think there's any particular urgency behind this, is there?
I cannot tell how common AM8131 based systems in combination with one of these
three devices actually are. However, this patch set is essential for those
systems.
--
AMD Saxony Limited Liability Company & Co. KG
Operating System Research Center
Wilschdorfer Landstr. 101, 01109 Dresden, Germany
Register Court Dresden: HRA 4896
General Partner authorized to represent:
AMD Saxony LLC (Wilmington, Delaware, US)
General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy
Am Dienstag, 15. Mai 2007 23:35:31 schrieb Andrew Vasquez:
> 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.
>
>
>
Sorry, I missed out an essential part in the qla2xxx driver, namely the MMRBC configuration for PCI-X. The submitted patch only did that for PCIe. This is the corrected version
The pre-condition check is there to not blindly call the mmrbc-functions, but to check for the bus type first. Indeed, the first patch version made this check look unnecessary.
Signed-off by: Peter Oruba <[email protected]>
Based on patch set by Stephen Hemminger <[email protected]>
---
--- linux-2.6.22-rc1.orig/drivers/scsi/qla2xxx/qla_init.c 2007-05-14 11:29:41.561311000 +0200
+++ linux-2.6.22-rc1/drivers/scsi/qla2xxx/qla_init.c 2007-05-16 16:08:47.811810000 +0200
@@ -269,27 +269,15 @@ qla24xx_pci_config(scsi_qla_host_t *ha)
/* PCI-X -- adjust Maximum Memory Read Byte Count (2048). */
pcix_cmd_reg = pci_find_capability(ha->pdev, PCI_CAP_ID_PCIX);
- if (pcix_cmd_reg) {
- uint16_t pcix_cmd;
-
- pcix_cmd_reg += PCI_X_CMD;
- pci_read_config_word(ha->pdev, pcix_cmd_reg, &pcix_cmd);
- pcix_cmd &= ~PCI_X_CMD_MAX_READ;
- pcix_cmd |= 0x0008;
- pci_write_config_word(ha->pdev, pcix_cmd_reg, pcix_cmd);
- }
+ if (pcix_cmd_reg)
+ if (pcix_set_mmrbc(ha->pdev, 2048))
+ DEBUG2(printk("Couldn't write PCI-X read request\n"));
/* PCIe -- adjust Maximum Read Request Size (2048). */
pcie_dctl_reg = pci_find_capability(ha->pdev, PCI_CAP_ID_EXP);
- if (pcie_dctl_reg) {
- uint16_t pcie_dctl;
-
- pcie_dctl_reg += PCI_EXP_DEVCTL;
- pci_read_config_word(ha->pdev, pcie_dctl_reg, &pcie_dctl);
- pcie_dctl &= ~PCI_EXP_DEVCTL_READRQ;
- pcie_dctl |= 0x4000;
- pci_write_config_word(ha->pdev, pcie_dctl_reg, pcie_dctl);
- }
+ if (pcie_dctl_reg)
+ if (pcie_set_readrq(ha->pdev, 2048))
+ DEBUG2(printk("Couldn't write PCI Express read request\n"));
/* Reset expansion ROM address decode enable */
pci_read_config_dword(ha->pdev, PCI_ROM_ADDRESS, &d);
---
--
AMD Saxony Limited Liability Company & Co. KG
Operating System Research Center
Wilschdorfer Landstr. 101, 01109 Dresden, Germany
Register Court Dresden: HRA 4896
General Partner authorized to represent:
AMD Saxony LLC (Wilmington, Delaware, US)
General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy
On Tue, 15 May 2007 13:59:13 +0200 Peter Oruba wrote:
> This patch introduces an interface to read and write PCI-X / PCI-Express
> maximum read byte count values from PCI config space. There is a second
> function that returns the maximum _designed_ read byte count, which marks the
> maximum value for a device, since some drivers try to set MMRBC to the
> highest allowed value and rely on such a function.
>
> Signed-off by: Peter Oruba <[email protected]>
> Based on patch set by Stephen Hemminger <[email protected]>
>
> ---
> diff -uprN -X linux-2.6.22-rc1.orig/Documentation/dontdiff
> linux-2.6.22-rc1.orig/drivers/pci/pci.c linux-2.6.22-rc1/drivers/pci/pci.c
> --- linux-2.6.22-rc1.orig/drivers/pci/pci.c 2007-05-14 11:29:39.473498000
> +0200
> +++ linux-2.6.22-rc1/drivers/pci/pci.c 2007-05-15 11:44:33.804254000 +0200
> @@ -1375,6 +1375,170 @@ pci_set_consistent_dma_mask(struct pci_d
> #endif
>
> /**
> + * pcix_get_max_mmrbc - get PCI-X maximum designed memory read byte count
> + * @dev: PCI device to query
> + *
> + * Returns mmrbc: maximum designed memory read count in bytes
> + * or appropriate error value.
> + */
> +int
> +pcix_get_max_mmrbc(struct pci_dev *dev)
Kernel style is like so (referring to multiple occurrences):
int pcix_get_max_mmrbc(struct pci_dev *dev)
Yes, that source file does it both ways (and needs to be Fixed).
> +{
> + int ret, err, cap;
> + u32 stat;
> +
> + cap = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> + if (!cap)
> + return -EINVAL;
> +
> + err = pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat);
> + if (err)
> + return -EINVAL;
> +
> + ret = (stat & PCI_X_STATUS_MAX_READ) >> 12;
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(pcix_get_max_mmrbc);
> +
> +
> +/**
> + * pcix_set_mmrbc - set PCI-X maximum memory read byte count
> + * @dev: PCI device to query
> + * @mmrbc: maximum memory read count in bytes
> + * valid values are 512, 1024, 2048, 4096
> + *
> + * If possible sets maximum memory read byte count, some bridges have erratas
"errata" is plural.
> + * that prevent this.
> + */
> +int
> +pcix_set_mmrbc(struct pci_dev *dev, int mmrbc)
> +{
...
> +}
> +EXPORT_SYMBOL(pcix_set_mmrbc);
> +
> +/**
> + * pcie_get_readrq - get PCI Express read request size
> + * @dev: PCI device to query
> + *
> + * Returns maximum memory read request in bytes
> + * or appropriate error value.
> + */
> +int pcie_get_readrq(struct pci_dev *dev)
:)
> +{
> + int ret, cap;
> + u16 ctl;
> +
> + cap = pci_find_capability(dev, PCI_CAP_ID_EXP);
> + if (!cap)
> + return -EINVAL;
> +
> + ret = pci_read_config_word(dev, cap + PCI_EXP_DEVCTL, &ctl);
> + if (!ret)
> + ret = 128 << ((ctl & PCI_EXP_DEVCTL_READRQ) >> 12);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(pcie_get_readrq);
> +
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***