On Wed, Jun 1, 2011 at 2:39 PM, Pratyush Anand <pratyush.linux@xxxxxxxxx> wrote:
> On Thu, Jun 2, 2011 at 12:40 AM, Jon Mason <jon.mason@xxxxxxxx> wrote:
>> This is required to work around buggy BIOS revisions found on various
>> whitebox motherboards which do not configure mps beyond one level below
>> the root port. Ensure that the max payload size of children bridges and
>> devices are the same as the parent bus.
>>
>> Signed-off-by: Jon Mason <mason@xxxxxxxx>
>> ---
>> drivers/pci/bus.c | 57 ++++++++++++++++++++++++++++++++++++++
>> drivers/pci/hotplug/pcihp_slot.c | 47 ++-----------------------------
>> 2 files changed, 60 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>> index 1e2ad92..a8be7e4 100644
>> --- a/drivers/pci/bus.c
>> +++ b/drivers/pci/bus.c
>> @@ -169,6 +169,53 @@ int pci_bus_add_child(struct pci_bus *bus)
>> return retval;
>> }
>>
>> +/* Program PCIE MaxPayload setting on device:
>> + * ensure parent maxpayload <= device
>> + */
>> +void pci_set_payload(struct pci_dev *dev)
>> +{
>> + int pos, ppos;
>> + u16 pctl, psz;
>> + u16 dctl, dsz, dcap, dmax;
>> + struct pci_dev *parent;
>> +
>> + parent = dev->bus->self;
>> + if (!parent)
>> + return;
>> +
>> + pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
>> + if (!pos)
>> + return;
>> +
>> + /* Read Device MaxPayload capability and setting */
>> + pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &dctl);
>> + pci_read_config_word(dev, pos + PCI_EXP_DEVCAP, &dcap);
>> + dsz = (dctl & PCI_EXP_DEVCTL_PAYLOAD) >> 5;
>> + dmax = (dcap & PCI_EXP_DEVCAP_PAYLOAD);
>> +
>> + /* Read Parent MaxPayload setting */
>> + ppos = pci_find_capability(parent, PCI_CAP_ID_EXP);
>> + if (!ppos)
>> + return;
>> +
>> + pci_read_config_word(parent, ppos + PCI_EXP_DEVCTL, &pctl);
>> + psz = (pctl & PCI_EXP_DEVCTL_PAYLOAD) >> 5;
>> +
>
> I am new to PCIe, So my understanding may not be correct.
> Still, what I understand is as under, please correct me if I am wrong.
>
> consider:
>
> Parent (RC) - Max_Payload_Size 256 => psz = 512
> Child (EP) - Max_Payload_Size_capability 256 => dmax = 256
> Child (EP) - Max_Payload_Size 128 => dsz = 128
>
>> + /* If parent payload > device max -> set to device max */
>> + if (psz > dmax)
>> + psz = dmax;
>> +
>
> => psz = dmax = 256
> dsz = 128
>
>> + /* If parent payload > device payload -> set speed
>> + * If parent payload <= device payload -> do nothing
>> + */
>> + if (psz > dsz) {
>> + dev_info(&dev->dev, "Setting MaxPayload to %d\n", 128 << psz);
>> +
>> + pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, (psz << 5) |
>> + (dctl & ~PCI_EXP_DEVCTL_PAYLOAD));
>
> => dsz = 256
> psz = 256
>
> now, what I understand that RC will not always send 256 byte TLP until
> this psz (=256) is
> programmed into its (parents) EXP_DEVCTL. It might send 512 byte TLP, which this
> EP will not be able to receive and will return UR.
> so we need to program parents payload size to the min of downstream tree.
> is my understanding correct?
You are correct. I was only thinking about the DMA write. The entire
fabric needs have each device/bridge MPS set to the smallest MPS
found, otherwise DMA reads could be too large for a device on the
fabric and cause an PCI unrecoverable error.
So the correct solution is to find the smallest MPS per fabric while
walking the bus, then walk the bus again setting each device/bridge's
MPS to the smallest MPS found.
This patch just got much more complicated. Damn you crappy BIOS developers!
>
>> + }
>> +}
>> +
>> /**
>> * pci_bus_add_devices - insert newly discovered PCI devices
>> * @bus: bus to check for new devices
>> @@ -188,6 +235,16 @@ void pci_bus_add_devices(const struct pci_bus *bus)
>> int retval;
>>
>> list_for_each_entry(dev, &bus->devices, bus_list) {
>> +
>> + /*
>> + * This is required to work around buggy BIOS revisions found on
>> + * various whitebox motherboards which do not configure mps
>> + * beyond one level below the root port. Ensure that the max
>> + * payload size of children bridges and devices are the same as
>> + * the parent bus.
>> + */
>> + pci_set_payload(dev);
>> +
>> /* Skip already-added devices */
>> if (dev->is_added)
>> continue;
>> diff --git a/drivers/pci/hotplug/pcihp_slot.c b/drivers/pci/hotplug/pcihp_slot.c
>> index 749fdf0..cb3c67c 100644
>> --- a/drivers/pci/hotplug/pcihp_slot.c
>> +++ b/drivers/pci/hotplug/pcihp_slot.c
>> @@ -26,6 +26,8 @@
>> #include <linux/pci.h>
>> #include <linux/pci_hotplug.h>
>>
>> +extern void pci_set_payload(struct pci_dev *dev);
>> +
>> static struct hpp_type0 pci_default_type0 = {
>> .revision = 1,
>> .cache_line_size = 8,
>> @@ -158,47 +160,6 @@ static void program_hpp_type2(struct pci_dev *dev, struct hpp_type2 *hpp)
>> */
>> }
>>
>> -/* Program PCIE MaxPayload setting on device: ensure parent maxpayload <= device */
>> -static int pci_set_payload(struct pci_dev *dev)
>> -{
>> - int pos, ppos;
>> - u16 pctl, psz;
>> - u16 dctl, dsz, dcap, dmax;
>> - struct pci_dev *parent;
>> -
>> - parent = dev->bus->self;
>> - pos = pci_find_capability(dev, PCI_CAP_ID_EXP);
>> - if (!pos)
>> - return 0;
>> -
>> - /* Read Device MaxPayload capability and setting */
>> - pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &dctl);
>> - pci_read_config_word(dev, pos + PCI_EXP_DEVCAP, &dcap);
>> - dsz = (dctl & PCI_EXP_DEVCTL_PAYLOAD) >> 5;
>> - dmax = (dcap & PCI_EXP_DEVCAP_PAYLOAD);
>> -
>> - /* Read Parent MaxPayload setting */
>> - ppos = pci_find_capability(parent, PCI_CAP_ID_EXP);
>> - if (!ppos)
>> - return 0;
>> - pci_read_config_word(parent, ppos + PCI_EXP_DEVCTL, &pctl);
>> - psz = (pctl & PCI_EXP_DEVCTL_PAYLOAD) >> 5;
>> -
>> - /* If parent payload > device max payload -> error
>> - * If parent payload > device payload -> set speed
>> - * If parent payload <= device payload -> do nothing
>> - */
>> - if (psz > dmax)
>> - return -1;
>> - else if (psz > dsz) {
>> - dev_info(&dev->dev, "Setting MaxPayload to %d\n", 128 << psz);
>> - pci_write_config_word(dev, pos + PCI_EXP_DEVCTL,
>> - (dctl & ~PCI_EXP_DEVCTL_PAYLOAD) +
>> - (psz << 5));
>> - }
>> - return 0;
>> -}
>> -
>> void pci_configure_slot(struct pci_dev *dev)
>> {
>> struct pci_dev *cdev;
>> @@ -210,9 +171,7 @@ void pci_configure_slot(struct pci_dev *dev)
>> (dev->class >> 8) == PCI_CLASS_BRIDGE_PCI)))
>> return;
>>
>> - ret = pci_set_payload(dev);
>> - if (ret)
>> - dev_warn(&dev->dev, "could not set device max payload\n");
>> + pci_set_payload(dev);
>>
>> memset(&hpp, 0, sizeof(hpp));
>> ret = pci_get_hp_params(dev, &hpp);
>> --
>> 1.7.4.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
