On Tue, May 31, 2011 at 11:35 PM, Pratyush Anand
<pratyush.linux@xxxxxxxxx> wrote:
>
> On Sat, May 28, 2011 at 3:09 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 | 63 ++++++++++++++++++++++++++++++++++++++
> > drivers/pci/hotplug/pcihp_slot.c | 45 ++-------------------------
> > 2 files changed, 66 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> > index 1e2ad92..7dd3d24 100644
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -169,6 +169,59 @@ int pci_bus_add_child(struct pci_bus *bus)
> > return retval;
> > }
> >
> > +/* Program PCIE MaxPayload setting on device:
> > + * ensure parent maxpayload <= device
> > + */
> > +int pci_set_payload(struct pci_dev *dev, bool hotplug)
> > +{
> > + int pos, ppos;
> > + u16 pctl, psz;
> > + u16 dctl, dsz, dcap, dmax;
> > + struct pci_dev *parent;
> > +
> > + parent = dev->bus->self;
> > + if (!parent)
> > + return 0;
> > +
> > + 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
>
> Consider a Situation : RC->EP
> RC's Max_Payload_Size is 256 bytes.
> EP's Max_Payload_Size Supported is only 128 bytes.
> Should this system not work? It must .
> So the soultion should be "to align Max_Payload_Size for
> all devices to the minimum of the downstream tree"
There is no issue here, this is the way it currently behaves. If the
parent size is greater than the child max, its current settings are
not changed.
However, we might want to have the child payload size set to the child
max in the case (which one would assume was the current setting). I
can easily add this change now. Jordan, what do you think?
> > + * If parent payload > device payload -> set speed
> > + * If parent payload <= device payload -> do nothing
> > + */
> > + if (psz > dmax)
> > + return -1;
> > + else if (psz > dsz) {
> > + if (hotplug)
> > + dev_info(&dev->dev, "Setting MaxPayload to %d\n",
> > + 128 << psz);
> > + else
> > + dev_warn(&dev->dev, "Child MPS of %d != Parent MPS of "
> > + "%d! Most likely caused by bad BIOS. Setting"
> > + " Child MPS to %d\n", 128 << dsz, 128 << psz,
> > + 128 << psz);
> > +
> > + pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, (psz << 5) |
> > + (dctl & ~PCI_EXP_DEVCTL_PAYLOAD));
> > + }
> > + return 0;
> > +}
> > +
> > /**
> > * pci_bus_add_devices - insert newly discovered PCI devices
> > * @bus: bus to check for new devices
> > @@ -188,6 +241,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.
> > + */
>
> We should also consider about Max_Read_Request_Size.
> Max_Read_Request_Size of any of the down stream device
> should be less than or equal to Device's Max_Payload_Size.
> I mean, if an EP with Max_Read_Request_Size 256 bytes generates an
> outbound request to an RC with Max_Payload_Size 128 bytes, then RC will
> not be able to respond. In this scenerio, EP's Max_Read_Request_Size should be
> set to 128.
>
> Regards
> Pratyush
>
> > + pci_set_payload(dev, false);
> > +
> > /* 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..0038c40 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 int pci_set_payload(struct pci_dev *dev, bool hotplug);
> > +
> > 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,7 +171,7 @@ void pci_configure_slot(struct pci_dev *dev)
> > (dev->class >> 8) == PCI_CLASS_BRIDGE_PCI)))
> > return;
> >
> > - ret = pci_set_payload(dev);
> > + ret = pci_set_payload(dev, true);
> > if (ret)
> > dev_warn(&dev->dev, "could not set device max payload\n");
> >
> > --
> > 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
