Hi, Jianmin,
On Sat, Jul 16, 2022 at 3:15 PM Jianmin Lv <lvjianmin@xxxxxxxxxxx> wrote:
>
> Hi, Huacai and Bjorn,
>
> Actually, I don't think we have to fix the MRRS issue of 7A in this
> way(change core code of PCI). The reasons are:
>
> - First, we don't know if other pci controlers have simillar issue, at
> least I don't see yet. I think if *only we* have the issue, maybe we can
> fix it in our controller driver rather than changing core code. And in
> future, if the issue is proved a common one, abstract it then by common way.
>
Keystone's pci controller has the same problem (maybe DesignWare based
controllers all have the same problem), this is discussed for a long
time.
Huacai
> - Second, even though we limit the MRRS in pcie_set_readrq() according
> to the flag set in quirk function, some drivers(e.g. in
> drivers/rapidio/devices/tsi721.c, maybe other driver do this in future,
> I dont't know.) directly call pcie_capability_clear_and_set_word to set
> MRRS, which will still break the fix.
>
> - Third, on resuming from S3, the MRRS stored in memory should be
> allowed to set to dev ctrl reg(because the reg has been reset during S3).
>
>
> Fix MMRS in our controller driver by using self-defined config_write(),
> maybe like this:
>
> static u32 handle_mrrs_limit(struct pci_bus *bus, unsigned int devfn,
> void __iomem *addr, u32 val)
> {
> u32 tmp;
> bool runtime_flag = 1;
> int pos = pci_bus_find_capability_nolock(bus, devfn, PCI_CAP_ID_EXP);
>
> #ifdef CONFIG_PM_SLEEP
> if (pm_suspend_target_state == PM_SUSPEND_ON)
> runtime_flag = 0;
> #endif
>
> if (resume_flag && pos != 0 && (pos + PCI_EXP_DEVCTL) == reg) {
> tmp = readl(addr);
> if ((val & PCI_EXP_DEVCTL_READRQ) > (tmp &
> PCI_EXP_DEVCTL_READRQ)) {
> val &= ~PCI_EXP_DEVCTL_READRQ;
> val |= (tmp & PCI_EXP_DEVCTL_READRQ);
> }
> }
> return val;
>
> }
>
>
> static int loongson_pci_config_write32(struct pci_bus *bus, unsigned int
> devfn,
> int where, int size, u32 val)
> {
> void __iomem *addr;
> u32 mask, tmp;
> int reg = where & ~3;
>
> addr = bus->ops->map_bus(bus, devfn, where & ~0x3);
> if (!addr)
> return PCIBIOS_DEVICE_NOT_FOUND;
> val = handle_mrrs_limit(bus, devfn, addr, reg, val);
> writel(val, addr);
>
> return PCIBIOS_SUCCESSFUL;
> }
>
> And I still have to emphasize on the fix in this way: It's still does
> not work for pciehp. It's only used for addressing MRRS issue of 7A
> revisions which have no pciehp support.
>
> And in future, for new revision, we just need to skip handle_mrrs_limit.
>
> The way described here is just my immature opinion, we can discuss it
> if required.
>
> Thanks.
>
> On 2022/7/14 下午8:42, Huacai Chen wrote:
> > In new revision of LS7A, some PCIe ports support larger value than 256,
> > but their maximum supported MRRS values are not detectable. Moreover,
> > the current loongson_mrrs_quirk() cannot avoid devices increasing its
> > MRRS after pci_enable_device(), and some devices (e.g. Realtek 8169)
> > will actually set a big value in its driver. So the only possible way
> > is configure MRRS of all devices in BIOS, and add a pci host bridge bit
> > flag (i.e., no_inc_mrrs) to stop the increasing MRRS operations.
> >
> > However, according to PCIe Spec, it is legal for an OS to program any
> > value for MRRS, and it is also legal for an endpoint to generate a Read
> > Request with any size up to its MRRS. As the hardware engineers say, the
> > root cause here is LS7A doesn't break up large read requests. In detail,
> > LS7A PCIe port reports CA (Completer Abort) if it receives a Memory Read
> > request with a size that's "too big" ("too big" means larger than the
> > PCIe ports can handle, which means 256 for some ports and 4096 for the
> > others, and of course this is a problem in the LS7A's hardware design).
> >
> > Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx>
> > ---
> > drivers/pci/controller/pci-loongson.c | 44 +++++++++------------------
> > drivers/pci/pci.c | 6 ++++
> > include/linux/pci.h | 1 +
> > 3 files changed, 22 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c
> > index 594653154deb..af73bb766e48 100644
> > --- a/drivers/pci/controller/pci-loongson.c
> > +++ b/drivers/pci/controller/pci-loongson.c
> > @@ -68,37 +68,23 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > DEV_LS7A_LPC, system_bus_quirk);
> >
> > -static void loongson_mrrs_quirk(struct pci_dev *dev)
> > +static void loongson_mrrs_quirk(struct pci_dev *pdev)
> > {
> > - struct pci_bus *bus = dev->bus;
> > - struct pci_dev *bridge;
> > - static const struct pci_device_id bridge_devids[] = {
> > - { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_0) },
> > - { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_1) },
> > - { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_2) },
> > - { 0, },
> > - };
> > -
> > - /* look for the matching bridge */
> > - while (!pci_is_root_bus(bus)) {
> > - bridge = bus->self;
> > - bus = bus->parent;
> > - /*
> > - * Some Loongson PCIe ports have a h/w limitation of
> > - * 256 bytes maximum read request size. They can't handle
> > - * anything larger than this. So force this limit on
> > - * any devices attached under these ports.
> > - */
> > - if (pci_match_id(bridge_devids, bridge)) {
> > - if (pcie_get_readrq(dev) > 256) {
> > - pci_info(dev, "limiting MRRS to 256\n");
> > - pcie_set_readrq(dev, 256);
> > - }
> > - break;
> > - }
> > - }
> > + /*
> > + * Some Loongson PCIe ports have h/w limitations of maximum read
> > + * request size. They can't handle anything larger than this. So
> > + * force this limit on any devices attached under these ports.
> > + */
> > + struct pci_host_bridge *bridge = pci_find_host_bridge(pdev->bus);
> > +
> > + bridge->no_inc_mrrs = 1;
> > }
> > -DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, loongson_mrrs_quirk);
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > + DEV_PCIE_PORT_0, loongson_mrrs_quirk);
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > + DEV_PCIE_PORT_1, loongson_mrrs_quirk);
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> > + DEV_PCIE_PORT_2, loongson_mrrs_quirk);
> >
> > static struct loongson_pci *pci_bus_to_loongson_pci(struct pci_bus *bus)
> > {
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index cfaf40a540a8..79157cbad835 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -6052,6 +6052,7 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
> > {
> > u16 v;
> > int ret;
> > + struct pci_host_bridge *bridge = pci_find_host_bridge(dev->bus);
> >
> > if (rq < 128 || rq > 4096 || !is_power_of_2(rq))
> > return -EINVAL;
> > @@ -6070,6 +6071,11 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
> >
> > v = (ffs(rq) - 8) << 12;
> >
> > + if (bridge->no_inc_mrrs) {
> > + if (rq > pcie_get_readrq(dev))
> > + return -EINVAL;
> > + }
> > +
> > ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
> > PCI_EXP_DEVCTL_READRQ, v);
> >
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 81a57b498f22..a9211074add6 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -569,6 +569,7 @@ struct pci_host_bridge {
> > void *release_data;
> > unsigned int ignore_reset_delay:1; /* For entire hierarchy */
> > unsigned int no_ext_tags:1; /* No Extended Tags */
> > + unsigned int no_inc_mrrs:1; /* No Increase MRRS */
> > unsigned int native_aer:1; /* OS may use PCIe AER */
> > unsigned int native_pcie_hotplug:1; /* OS may use PCIe hotplug */
> > unsigned int native_shpc_hotplug:1; /* OS may use SHPC hotplug */
> >
>
