linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] PCI: DWC: meson: add 256 bytes MRRS quirk
@ 2021-07-27  2:30 Artem Lapkin
  2021-07-27 19:43 ` Bjorn Helgaas
  0 siblings, 1 reply; 5+ messages in thread
From: Artem Lapkin @ 2021-07-27  2:30 UTC (permalink / raw)
  To: narmstrong
  Cc: yue.wang, khilman, lorenzo.pieralisi, robh, kw, jbrunet,
	christianshewitt, martin.blumenstingl, linux-pci,
	linux-arm-kernel, linux-amlogic, linux-kernel, art, nick, gouwa

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.

Come-from: https://lkml.org/lkml/2021/6/18/160
Come-from: https://lkml.org/lkml/2021/6/19/19

It only affects PCIe in P2P, in non-P2P is will certainly affect
transfers on the internal SoC/Processor/Chip internal bus/fabric.

These quirks are currently implemented in the
controller driver and only applies when the controller has been probed
and to each endpoint detected on this particular controller.

Continue having separate quirks for each controller if the core
isn't the right place to handle MPS/MRRS.

>> Neil

Signed-off-by: Artem Lapkin <art@khadas.com>
---
 drivers/pci/controller/dwc/pci-meson.c | 31 ++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
index 686ded034..1498950de 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -466,6 +466,37 @@ static int meson_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static void meson_mrrs_limit_quirk(struct pci_dev *dev)
+{
+	struct pci_bus *bus = dev->bus;
+	int mrrs, mrrs_limit = 256;
+	static const struct pci_device_id bridge_devids[] = {
+		{ PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3) },
+		{ 0, },
+	};
+
+	/* look for the matching bridge */
+	while (!pci_is_root_bus(bus)) {
+		/*
+		 * 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, bus->self)) {
+			bus = bus->parent;
+			continue;
+		}
+
+		mrrs = pcie_get_readrq(dev);
+		if (mrrs > mrrs_limit) {
+			pci_info(dev, "limiting MRRS %d to %d\n", mrrs, mrrs_limit);
+			pcie_set_readrq(dev, mrrs_limit);
+		}
+		break;
+	}
+}
+DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, meson_mrrs_limit_quirk);
+
 static const struct of_device_id meson_pcie_of_match[] = {
 	{
 		.compatible = "amlogic,axg-pcie",
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] PCI: DWC: meson: add 256 bytes MRRS quirk
  2021-07-27  2:30 [PATCH v2] PCI: DWC: meson: add 256 bytes MRRS quirk Artem Lapkin
@ 2021-07-27 19:43 ` Bjorn Helgaas
  2021-07-28 15:08   ` Neil Armstrong
  2021-07-29  2:21   ` Art Nikpal
  0 siblings, 2 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2021-07-27 19:43 UTC (permalink / raw)
  To: Artem Lapkin
  Cc: narmstrong, yue.wang, khilman, lorenzo.pieralisi, robh, kw,
	jbrunet, christianshewitt, martin.blumenstingl, linux-pci,
	linux-arm-kernel, linux-amlogic, linux-kernel, art, nick, gouwa

On Tue, Jul 27, 2021 at 10:30:00AM +0800, Artem Lapkin wrote:
> 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.

This needs to say whether this is a functional or a performance issue.

If it's a functional issue, i.e., if meson signals an error or abort
when it receives a read request for > 256 bytes, we need to explain
exactly what happens.

If it's a performance issue, we need to explain why MRRS affects
performance and that this is an optimization.

> Come-from: https://lkml.org/lkml/2021/6/18/160
> Come-from: https://lkml.org/lkml/2021/6/19/19

Please use lore.kernel.org URLs instead.  The lore URLs are a little
uglier, but are more functional, more likely to continue working, and
avoid the ads.  These are:

  https://lore.kernel.org/r/20210618230132.GA3228427@bjorn-Precision-5520
  https://lore.kernel.org/r/20210619063952.2008746-1-art@khadas.com

> It only affects PCIe in P2P, in non-P2P is will certainly affect
> transfers on the internal SoC/Processor/Chip internal bus/fabric.

This needs to explain how a field in a PCIe TLP affects transfers on
these non-PCIe fabrics.

> These quirks are currently implemented in the
> controller driver and only applies when the controller has been probed
> and to each endpoint detected on this particular controller.
> 
> Continue having separate quirks for each controller if the core
> isn't the right place to handle MPS/MRRS.

I see similar code in dwc/pci-keystone.c.  Does this problem actually
affect *all* DesignWare-based controllers?

If so, we should put the workaround in the common dwc code, e.g.,
pcie-designware.c or similar.  

It also seems to affect pci-loongson.c (not DesignWare-based).  Is
there some reason it has the same problem, e.g., does loongson contain
DesignWare IP, or does it use the same non-PCIe fabric?

> >> Neil
> 
> Signed-off-by: Artem Lapkin <art@khadas.com>
> ---
>  drivers/pci/controller/dwc/pci-meson.c | 31 ++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> index 686ded034..1498950de 100644
> --- a/drivers/pci/controller/dwc/pci-meson.c
> +++ b/drivers/pci/controller/dwc/pci-meson.c
> @@ -466,6 +466,37 @@ static int meson_pcie_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static void meson_mrrs_limit_quirk(struct pci_dev *dev)
> +{
> +	struct pci_bus *bus = dev->bus;
> +	int mrrs, mrrs_limit = 256;
> +	static const struct pci_device_id bridge_devids[] = {
> +		{ PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3) },

I don't really believe that PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3 is the
only device affected here.  Is this related to the Meson root port, or
is it related to a PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3 on a plug-in card?
I guess the former, since you're searching upward for a root port.

So why is this limited to PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3?

> +		{ 0, },
> +	};
> +
> +	/* look for the matching bridge */
> +	while (!pci_is_root_bus(bus)) {
> +		/*
> +		 * 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, bus->self)) {
> +			bus = bus->parent;
> +			continue;
> +		}
> +
> +		mrrs = pcie_get_readrq(dev);
> +		if (mrrs > mrrs_limit) {
> +			pci_info(dev, "limiting MRRS %d to %d\n", mrrs, mrrs_limit);
> +			pcie_set_readrq(dev, mrrs_limit);
> +		}
> +		break;
> +	}
> +}
> +DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, meson_mrrs_limit_quirk);
> +
>  static const struct of_device_id meson_pcie_of_match[] = {
>  	{
>  		.compatible = "amlogic,axg-pcie",
> -- 
> 2.25.1
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] PCI: DWC: meson: add 256 bytes MRRS quirk
  2021-07-27 19:43 ` Bjorn Helgaas
@ 2021-07-28 15:08   ` Neil Armstrong
  2021-07-29  2:06     ` Art Nikpal
  2021-07-29  2:21   ` Art Nikpal
  1 sibling, 1 reply; 5+ messages in thread
From: Neil Armstrong @ 2021-07-28 15:08 UTC (permalink / raw)
  To: Bjorn Helgaas, Artem Lapkin
  Cc: yue.wang, khilman, lorenzo.pieralisi, robh, kw, jbrunet,
	christianshewitt, martin.blumenstingl, linux-pci,
	linux-arm-kernel, linux-amlogic, linux-kernel, art, nick, gouwa

Hi,


On 27/07/2021 21:43, Bjorn Helgaas wrote:
> On Tue, Jul 27, 2021 at 10:30:00AM +0800, Artem Lapkin wrote:
>> 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.
> 
> This needs to say whether this is a functional or a performance issue.
> 
> If it's a functional issue, i.e., if meson signals an error or abort
> when it receives a read request for > 256 bytes, we need to explain
> exactly what happens.
> 
> If it's a performance issue, we need to explain why MRRS affects
> performance and that this is an optimization.
> 
>> Come-from: https://lkml.org/lkml/2021/6/18/160
>> Come-from: https://lkml.org/lkml/2021/6/19/19
> 
> Please use lore.kernel.org URLs instead.  The lore URLs are a little
> uglier, but are more functional, more likely to continue working, and
> avoid the ads.  These are:
> 
>   https://lore.kernel.org/r/20210618230132.GA3228427@bjorn-Precision-5520
>   https://lore.kernel.org/r/20210619063952.2008746-1-art@khadas.com
> 
>> It only affects PCIe in P2P, in non-P2P is will certainly affect
>> transfers on the internal SoC/Processor/Chip internal bus/fabric.
> 
> This needs to explain how a field in a PCIe TLP affects transfers on
> these non-PCIe fabrics.
> 
>> These quirks are currently implemented in the
>> controller driver and only applies when the controller has been probed
>> and to each endpoint detected on this particular controller.
>>
>> Continue having separate quirks for each controller if the core
>> isn't the right place to handle MPS/MRRS.
> 
> I see similar code in dwc/pci-keystone.c.  Does this problem actually
> affect *all* DesignWare-based controllers?
> 
> If so, we should put the workaround in the common dwc code, e.g.,
> pcie-designware.c or similar.  
> 
> It also seems to affect pci-loongson.c (not DesignWare-based).  Is
> there some reason it has the same problem, e.g., does loongson contain
> DesignWare IP, or does it use the same non-PCIe fabric?

As my reply on the previous thread, the Synopsys IP can be configured with a
maximum TLP packet to AXI transaction size, which is hardcoded AFAIK Amlogic
doesn't explicit it. And it doesn't seem we can read the value.

This means is a TPL size if higher than this maximum packet size, the IP will
do multiple AXI transactions, and this can reduce the system overall performance.

The problem is that it affects the P2P transactions aswell, which can support any MPS/MRRS.
But honestly, it's not a big deal on a PCIe 2.0 1x system only designed for NVMe and basic
PCIe devices.

The fun part is that the pci=pcie_bus_perf kerne cmdline solves this already,
isn't there any possibility to force pcie_bus_perf for a particular root port ?

Neil

> 
>>>> Neil
>>
>> Signed-off-by: Artem Lapkin <art@khadas.com>
>> ---
>>  drivers/pci/controller/dwc/pci-meson.c | 31 ++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
>> index 686ded034..1498950de 100644
>> --- a/drivers/pci/controller/dwc/pci-meson.c
>> +++ b/drivers/pci/controller/dwc/pci-meson.c
>> @@ -466,6 +466,37 @@ static int meson_pcie_probe(struct platform_device *pdev)
>>  	return ret;
>>  }
>>  
>> +static void meson_mrrs_limit_quirk(struct pci_dev *dev)
>> +{
>> +	struct pci_bus *bus = dev->bus;
>> +	int mrrs, mrrs_limit = 256;
>> +	static const struct pci_device_id bridge_devids[] = {
>> +		{ PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3) },
> 
> I don't really believe that PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3 is the
> only device affected here.  Is this related to the Meson root port, or
> is it related to a PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3 on a plug-in card?
> I guess the former, since you're searching upward for a root port.
> 
> So why is this limited to PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3?
> 
>> +		{ 0, },
>> +	};
>> +
>> +	/* look for the matching bridge */
>> +	while (!pci_is_root_bus(bus)) {
>> +		/*
>> +		 * 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, bus->self)) {
>> +			bus = bus->parent;
>> +			continue;
>> +		}
>> +
>> +		mrrs = pcie_get_readrq(dev);
>> +		if (mrrs > mrrs_limit) {
>> +			pci_info(dev, "limiting MRRS %d to %d\n", mrrs, mrrs_limit);
>> +			pcie_set_readrq(dev, mrrs_limit);
>> +		}
>> +		break;
>> +	}
>> +}
>> +DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, meson_mrrs_limit_quirk);
>> +
>>  static const struct of_device_id meson_pcie_of_match[] = {
>>  	{
>>  		.compatible = "amlogic,axg-pcie",
>> -- 
>> 2.25.1
>>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] PCI: DWC: meson: add 256 bytes MRRS quirk
  2021-07-28 15:08   ` Neil Armstrong
@ 2021-07-29  2:06     ` Art Nikpal
  0 siblings, 0 replies; 5+ messages in thread
From: Art Nikpal @ 2021-07-29  2:06 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Bjorn Helgaas, Yue Wang, Kevin Hilman, Lorenzo Pieralisi,
	Rob Herring, Krzysztof Wilczynski, Jerome Brunet,
	Christian Hewitt, Martin Blumenstingl, PCI, linux-arm-kernel,
	open list:ARM/Amlogic Meson...,
	LKML, Artem Lapkin, Nick Xie, Gouwa Wang

> The fun part is that the pci=pcie_bus_perf kerne cmdline solves this already,

yes pci=pcie_bus_perf works only because setup same MRRS=256 in this case
same like in this patch pcie_set_readrq(dev, 256 )

some details about pci=pcie_bus_perf

[    4.665327] pci 0000:00:00.0: [16c3:abcd] type 01 class 0x060400
[    4.671166] pci 0000:00:00.0: reg 0x38: [mem 0x00000000-0x0000ffff pref]
[    4.677793] pci 0000:00:00.0: supports D1
[    4.683028] pci 0000:00:00.0: PME# supported from D0 D1 D3hot D3cold
[    4.691667] pci 0000:01:00.0: [2646:2263] type 00 class 0x010802
[    4.697469] pci 0000:01:00.0: reg 0x10: [mem 0x00000000-0x00003fff 64bit]
[    4.704218] pci 0000:01:00.0: pcie_get_mps - 128
[    4.709607] pci 0000:00:00.0: pcie_get_mps - 256
[    4.715145] pci 0000:01:00.0: 4.000 Gb/s available PCIe bandwidth,
limited by 5.0 GT/s PCIe x1 link at 0000:00:00.0 (capable of 31.504
Gb/s with 8.0 GT/s PCIe x4 link)
[    4.748582] pci 0000:00:00.0: BAR 14: assigned [mem 0xfc700000-0xfc7fffff]
[    4.755575] pci 0000:00:00.0: BAR 6: assigned [mem
0xfc800000-0xfc80ffff pref]
[    4.762731] pci 0000:01:00.0: BAR 0: assigned [mem
0xfc700000-0xfc703fff 64bit]
[    4.769849] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
[    4.775195] pci 0000:00:00.0:   bridge window [mem 0xfc700000-0xfc7fffff]
[    4.781741] pci 0000:00:00.0: pcie_get_mps - 256
[    4.787118] pci 0000:00:00.0: pcie_set_mps - 256 (32)
[    4.792404] pci 0000:00:00.0: pcie_get_mps - 256
[    4.797637] pci 0000:00:00.0: pcie_get_mps - 256
[    4.802674] pci 0000:00:00.0: Max Payload Size set to  256/ 256
(was  256), Max Read Rq  256
[    4.810602] pci 0000:01:00.0: pcie_get_mps - 128
[    4.815590] pci 0000:00:00.0: pcie_get_mps - 256
[    4.820422] pci 0000:01:00.0: pcie_set_mps - 256 (32)
[    4.825274] pci 0000:01:00.0: pcie_get_mps - 256
[    4.829946] pci 0000:01:00.0: pcie_set_readrq - 256
[    4.834609] pci 0000:01:00.0: pcie_get_mps - 256
[    4.839174] pci 0000:01:00.0: pcie_get_mps - 256
[    4.843582] pci 0000:01:00.0: Max Payload Size set to  256/ 256
(was  128), Max Read Rq  256

and pci=pcie_bus_perf works for whole system and i think its not good
idea use or adoptate this option for this case

Artem

On Wed, Jul 28, 2021 at 11:08 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> Hi,
>
>
> On 27/07/2021 21:43, Bjorn Helgaas wrote:
> > On Tue, Jul 27, 2021 at 10:30:00AM +0800, Artem Lapkin wrote:
> >> 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.
> >
> > This needs to say whether this is a functional or a performance issue.
> >
> > If it's a functional issue, i.e., if meson signals an error or abort
> > when it receives a read request for > 256 bytes, we need to explain
> > exactly what happens.
> >
> > If it's a performance issue, we need to explain why MRRS affects
> > performance and that this is an optimization.
> >
> >> Come-from: https://lkml.org/lkml/2021/6/18/160
> >> Come-from: https://lkml.org/lkml/2021/6/19/19
> >
> > Please use lore.kernel.org URLs instead.  The lore URLs are a little
> > uglier, but are more functional, more likely to continue working, and
> > avoid the ads.  These are:
> >
> >   https://lore.kernel.org/r/20210618230132.GA3228427@bjorn-Precision-5520
> >   https://lore.kernel.org/r/20210619063952.2008746-1-art@khadas.com
> >
> >> It only affects PCIe in P2P, in non-P2P is will certainly affect
> >> transfers on the internal SoC/Processor/Chip internal bus/fabric.
> >
> > This needs to explain how a field in a PCIe TLP affects transfers on
> > these non-PCIe fabrics.
> >
> >> These quirks are currently implemented in the
> >> controller driver and only applies when the controller has been probed
> >> and to each endpoint detected on this particular controller.
> >>
> >> Continue having separate quirks for each controller if the core
> >> isn't the right place to handle MPS/MRRS.
> >
> > I see similar code in dwc/pci-keystone.c.  Does this problem actually
> > affect *all* DesignWare-based controllers?
> >
> > If so, we should put the workaround in the common dwc code, e.g.,
> > pcie-designware.c or similar.
> >
> > It also seems to affect pci-loongson.c (not DesignWare-based).  Is
> > there some reason it has the same problem, e.g., does loongson contain
> > DesignWare IP, or does it use the same non-PCIe fabric?
>
> As my reply on the previous thread, the Synopsys IP can be configured with a
> maximum TLP packet to AXI transaction size, which is hardcoded AFAIK Amlogic
> doesn't explicit it. And it doesn't seem we can read the value.
>
> This means is a TPL size if higher than this maximum packet size, the IP will
> do multiple AXI transactions, and this can reduce the system overall performance.
>
> The problem is that it affects the P2P transactions aswell, which can support any MPS/MRRS.
> But honestly, it's not a big deal on a PCIe 2.0 1x system only designed for NVMe and basic
> PCIe devices.
>
> The fun part is that the pci=pcie_bus_perf kerne cmdline solves this already,
> isn't there any possibility to force pcie_bus_perf for a particular root port ?
>
> Neil
>
> >
> >>>> Neil
> >>
> >> Signed-off-by: Artem Lapkin <art@khadas.com>
> >> ---
> >>  drivers/pci/controller/dwc/pci-meson.c | 31 ++++++++++++++++++++++++++
> >>  1 file changed, 31 insertions(+)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> >> index 686ded034..1498950de 100644
> >> --- a/drivers/pci/controller/dwc/pci-meson.c
> >> +++ b/drivers/pci/controller/dwc/pci-meson.c
> >> @@ -466,6 +466,37 @@ static int meson_pcie_probe(struct platform_device *pdev)
> >>      return ret;
> >>  }
> >>
> >> +static void meson_mrrs_limit_quirk(struct pci_dev *dev)
> >> +{
> >> +    struct pci_bus *bus = dev->bus;
> >> +    int mrrs, mrrs_limit = 256;
> >> +    static const struct pci_device_id bridge_devids[] = {
> >> +            { PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3) },
> >
> > I don't really believe that PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3 is the
> > only device affected here.  Is this related to the Meson root port, or
> > is it related to a PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3 on a plug-in card?
> > I guess the former, since you're searching upward for a root port.
> >
> > So why is this limited to PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3?
> >
> >> +            { 0, },
> >> +    };
> >> +
> >> +    /* look for the matching bridge */
> >> +    while (!pci_is_root_bus(bus)) {
> >> +            /*
> >> +             * 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, bus->self)) {
> >> +                    bus = bus->parent;
> >> +                    continue;
> >> +            }
> >> +
> >> +            mrrs = pcie_get_readrq(dev);
> >> +            if (mrrs > mrrs_limit) {
> >> +                    pci_info(dev, "limiting MRRS %d to %d\n", mrrs, mrrs_limit);
> >> +                    pcie_set_readrq(dev, mrrs_limit);
> >> +            }
> >> +            break;
> >> +    }
> >> +}
> >> +DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, meson_mrrs_limit_quirk);
> >> +
> >>  static const struct of_device_id meson_pcie_of_match[] = {
> >>      {
> >>              .compatible = "amlogic,axg-pcie",
> >> --
> >> 2.25.1
> >>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] PCI: DWC: meson: add 256 bytes MRRS quirk
  2021-07-27 19:43 ` Bjorn Helgaas
  2021-07-28 15:08   ` Neil Armstrong
@ 2021-07-29  2:21   ` Art Nikpal
  1 sibling, 0 replies; 5+ messages in thread
From: Art Nikpal @ 2021-07-29  2:21 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Neil Armstrong, Yue Wang, Kevin Hilman, Lorenzo Pieralisi,
	Rob Herring, Krzysztof Wilczynski, Jerome Brunet,
	Christian Hewitt, Martin Blumenstingl, PCI, linux-arm-kernel,
	open list:ARM/Amlogic Meson...,
	LKML, Artem Lapkin, Nick Xie, Gouwa Wang

> This needs to say whether this is a functional or a performance issue.

Looks like not a performance issue , this is a functional issue.

We detect just one problem(may be exist another i dont know ) for
writing(reading works fine) any data to NVME devices with MRRS != 256
we have scrambled HDMI display

> Does this problem actually affect *all* DesignWare-based controllers?
> So why is this limited to PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3?

i can say only for VIM3 device which use this controller and have this problem

On Wed, Jul 28, 2021 at 3:43 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Jul 27, 2021 at 10:30:00AM +0800, Artem Lapkin wrote:
> > 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.
>
> This needs to say whether this is a functional or a performance issue.
>
> If it's a functional issue, i.e., if meson signals an error or abort
> when it receives a read request for > 256 bytes, we need to explain
> exactly what happens.
>
> If it's a performance issue, we need to explain why MRRS affects
> performance and that this is an optimization.
>
> > Come-from: https://lkml.org/lkml/2021/6/18/160
> > Come-from: https://lkml.org/lkml/2021/6/19/19
>
> Please use lore.kernel.org URLs instead.  The lore URLs are a little
> uglier, but are more functional, more likely to continue working, and
> avoid the ads.  These are:
>
>   https://lore.kernel.org/r/20210618230132.GA3228427@bjorn-Precision-5520
>   https://lore.kernel.org/r/20210619063952.2008746-1-art@khadas.com
>
> > It only affects PCIe in P2P, in non-P2P is will certainly affect
> > transfers on the internal SoC/Processor/Chip internal bus/fabric.
>
> This needs to explain how a field in a PCIe TLP affects transfers on
> these non-PCIe fabrics.
>
> > These quirks are currently implemented in the
> > controller driver and only applies when the controller has been probed
> > and to each endpoint detected on this particular controller.
> >
> > Continue having separate quirks for each controller if the core
> > isn't the right place to handle MPS/MRRS.
>
> I see similar code in dwc/pci-keystone.c.  Does this problem actually
> affect *all* DesignWare-based controllers?
>
> If so, we should put the workaround in the common dwc code, e.g.,
> pcie-designware.c or similar.
>
> It also seems to affect pci-loongson.c (not DesignWare-based).  Is
> there some reason it has the same problem, e.g., does loongson contain
> DesignWare IP, or does it use the same non-PCIe fabric?
>
> > >> Neil
> >
> > Signed-off-by: Artem Lapkin <art@khadas.com>
> > ---
> >  drivers/pci/controller/dwc/pci-meson.c | 31 ++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> > index 686ded034..1498950de 100644
> > --- a/drivers/pci/controller/dwc/pci-meson.c
> > +++ b/drivers/pci/controller/dwc/pci-meson.c
> > @@ -466,6 +466,37 @@ static int meson_pcie_probe(struct platform_device *pdev)
> >       return ret;
> >  }
> >
> > +static void meson_mrrs_limit_quirk(struct pci_dev *dev)
> > +{
> > +     struct pci_bus *bus = dev->bus;
> > +     int mrrs, mrrs_limit = 256;
> > +     static const struct pci_device_id bridge_devids[] = {
> > +             { PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3) },
>
> I don't really believe that PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3 is the
> only device affected here.  Is this related to the Meson root port, or
> is it related to a PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3 on a plug-in card?
> I guess the former, since you're searching upward for a root port.
>
> So why is this limited to PCI_DEVICE_ID_SYNOPSYS_HAPSUSB3?
>
> > +             { 0, },
> > +     };
> > +
> > +     /* look for the matching bridge */
> > +     while (!pci_is_root_bus(bus)) {
> > +             /*
> > +              * 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, bus->self)) {
> > +                     bus = bus->parent;
> > +                     continue;
> > +             }
> > +
> > +             mrrs = pcie_get_readrq(dev);
> > +             if (mrrs > mrrs_limit) {
> > +                     pci_info(dev, "limiting MRRS %d to %d\n", mrrs, mrrs_limit);
> > +                     pcie_set_readrq(dev, mrrs_limit);
> > +             }
> > +             break;
> > +     }
> > +}
> > +DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, meson_mrrs_limit_quirk);
> > +
> >  static const struct of_device_id meson_pcie_of_match[] = {
> >       {
> >               .compatible = "amlogic,axg-pcie",
> > --
> > 2.25.1
> >

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-07-29  2:21 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27  2:30 [PATCH v2] PCI: DWC: meson: add 256 bytes MRRS quirk Artem Lapkin
2021-07-27 19:43 ` Bjorn Helgaas
2021-07-28 15:08   ` Neil Armstrong
2021-07-29  2:06     ` Art Nikpal
2021-07-29  2:21   ` Art Nikpal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).