* Re: [PATCH] pci: avoid unsync of LTR mechanism configuration
@ 2021-01-12 21:36 ` Bjorn Helgaas
0 siblings, 0 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2021-01-12 21:36 UTC (permalink / raw)
To: mingchuang.qiao
Cc: kerun.zhu, linux-pci, lambert.wang, linux-kernel, matthias.bgg,
linux-mediatek, haijun.liu, bhelgaas, linux-arm-kernel
Note subject line tips at https://lore.kernel.org/r/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com
On Tue, Jan 12, 2021 at 03:27:39PM +0800, mingchuang.qiao@mediatek.com wrote:
> From: Mingchuang Qiao <mingchuang.qiao@mediatek.com>
>
> In pci bus scan flow, the LTR mechanism enable bit of DEVCTL2 register
> is configured in pci_configure_ltr(). If device and it's bridge both
> support LTR mechanism, LTR mechanism of device and it's bridge will
> be enabled in DEVCTL2 register. And the flag pci_dev->ltr_path will
> be set as 1.
s/it's/its/ twice above.
It's == It is.
Its == belonging to 'it'.
Weird, I know, but that's English for you :)
> For some pcie products, pcie link becomes down when device reset. And then
> the LTR mechanism enable bit of bridge will become 0 based on description
> in PCIE r4.0, sec 7.8.16. However, the pci_dev->ltr_path value of bridge
> is still 1. Remove and rescan flow could be triggered to recover after
> device reset. In the bus rescan flow, the LTR mechanism of device will be
> enabled in pci_configure_ltr() due to ltr_path of its bridge is still 1.
s/pcie/PCIe/ twice above.
s/PCIE/PCIe/; also reference r5.0 instead of r4.0 if possible.
This sounds like a general problem of most device config bits being
cleared by reset. Usually these are restored by pci_restore_state().
Is that function missing a restore for PCI_EXP_DEVCTL2? I'd rather
have a general-purpose way of restoring all the config state than
little pieces scattered all over.
> When device's LTR mechanism is enabled, device will send LTR message to
> bridge. Bridge receives the device's LTR message and found bridge's LTR
> mechanism is disabled. Then the bridge will generate unsupported request
> and other error handling flow will be triggered such as AER Non-Fatal
> error handling.
>
> This patch is used to avoid this unsupported request and make the bridge's
> ltr_path value is aligned with DEVCTL2 register value. Check bridge
> register value if aligned with ltr_path in pci_configure_ltr(). If
> register value is disable and the ltr_path is 1, we need configure
> the register value as enable.
>
> Signed-off-by: Mingchuang Qiao <mingchuang.qiao@mediatek.com>
> ---
> drivers/pci/probe.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 953f15abc850..49355cf4af54 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2132,9 +2132,21 @@ static void pci_configure_ltr(struct pci_dev *dev)
> * Complex and all intermediate Switches indicate support for LTR.
> * PCIe r4.0, sec 6.18.
> */
> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> - ((bridge = pci_upstream_bridge(dev)) &&
> - bridge->ltr_path)) {
> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> + pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
> + PCI_EXP_DEVCTL2_LTR_EN);
> + dev->ltr_path = 1;
> + return;
> + }
> +
> + bridge = pci_upstream_bridge(dev);
> + if (bridge && bridge->ltr_path) {
> + pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, &ctl);
> + if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) {
> + pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
> + PCI_EXP_DEVCTL2_LTR_EN);
> + }
> +
> pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
> PCI_EXP_DEVCTL2_LTR_EN);
> dev->ltr_path = 1;
> --
> 2.18.0
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] pci: avoid unsync of LTR mechanism configuration
@ 2021-01-12 21:36 ` Bjorn Helgaas
0 siblings, 0 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2021-01-12 21:36 UTC (permalink / raw)
To: mingchuang.qiao
Cc: kerun.zhu, linux-pci, lambert.wang, linux-kernel, matthias.bgg,
linux-mediatek, haijun.liu, bhelgaas, linux-arm-kernel
Note subject line tips at https://lore.kernel.org/r/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com
On Tue, Jan 12, 2021 at 03:27:39PM +0800, mingchuang.qiao@mediatek.com wrote:
> From: Mingchuang Qiao <mingchuang.qiao@mediatek.com>
>
> In pci bus scan flow, the LTR mechanism enable bit of DEVCTL2 register
> is configured in pci_configure_ltr(). If device and it's bridge both
> support LTR mechanism, LTR mechanism of device and it's bridge will
> be enabled in DEVCTL2 register. And the flag pci_dev->ltr_path will
> be set as 1.
s/it's/its/ twice above.
It's == It is.
Its == belonging to 'it'.
Weird, I know, but that's English for you :)
> For some pcie products, pcie link becomes down when device reset. And then
> the LTR mechanism enable bit of bridge will become 0 based on description
> in PCIE r4.0, sec 7.8.16. However, the pci_dev->ltr_path value of bridge
> is still 1. Remove and rescan flow could be triggered to recover after
> device reset. In the bus rescan flow, the LTR mechanism of device will be
> enabled in pci_configure_ltr() due to ltr_path of its bridge is still 1.
s/pcie/PCIe/ twice above.
s/PCIE/PCIe/; also reference r5.0 instead of r4.0 if possible.
This sounds like a general problem of most device config bits being
cleared by reset. Usually these are restored by pci_restore_state().
Is that function missing a restore for PCI_EXP_DEVCTL2? I'd rather
have a general-purpose way of restoring all the config state than
little pieces scattered all over.
> When device's LTR mechanism is enabled, device will send LTR message to
> bridge. Bridge receives the device's LTR message and found bridge's LTR
> mechanism is disabled. Then the bridge will generate unsupported request
> and other error handling flow will be triggered such as AER Non-Fatal
> error handling.
>
> This patch is used to avoid this unsupported request and make the bridge's
> ltr_path value is aligned with DEVCTL2 register value. Check bridge
> register value if aligned with ltr_path in pci_configure_ltr(). If
> register value is disable and the ltr_path is 1, we need configure
> the register value as enable.
>
> Signed-off-by: Mingchuang Qiao <mingchuang.qiao@mediatek.com>
> ---
> drivers/pci/probe.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 953f15abc850..49355cf4af54 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2132,9 +2132,21 @@ static void pci_configure_ltr(struct pci_dev *dev)
> * Complex and all intermediate Switches indicate support for LTR.
> * PCIe r4.0, sec 6.18.
> */
> - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> - ((bridge = pci_upstream_bridge(dev)) &&
> - bridge->ltr_path)) {
> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> + pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
> + PCI_EXP_DEVCTL2_LTR_EN);
> + dev->ltr_path = 1;
> + return;
> + }
> +
> + bridge = pci_upstream_bridge(dev);
> + if (bridge && bridge->ltr_path) {
> + pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, &ctl);
> + if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) {
> + pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
> + PCI_EXP_DEVCTL2_LTR_EN);
> + }
> +
> pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
> PCI_EXP_DEVCTL2_LTR_EN);
> dev->ltr_path = 1;
> --
> 2.18.0
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] pci: avoid unsync of LTR mechanism configuration
2021-01-12 21:36 ` Bjorn Helgaas
(?)
@ 2021-01-18 2:55 ` Mingchuang Qiao
-1 siblings, 0 replies; 9+ messages in thread
From: Mingchuang Qiao @ 2021-01-18 2:55 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: bhelgaas, matthias.bgg, kerun.zhu, linux-pci, lambert.wang,
linux-kernel, linux-mediatek, haijun.liu, linux-arm-kernel
On Tue, 2021-01-12 at 15:36 -0600, Bjorn Helgaas wrote:
> Note subject line tips at https://lore.kernel.org/r/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com
>
> On Tue, Jan 12, 2021 at 03:27:39PM +0800, mingchuang.qiao@mediatek.com wrote:
> > From: Mingchuang Qiao <mingchuang.qiao@mediatek.com>
> >
> > In pci bus scan flow, the LTR mechanism enable bit of DEVCTL2 register
> > is configured in pci_configure_ltr(). If device and it's bridge both
> > support LTR mechanism, LTR mechanism of device and it's bridge will
> > be enabled in DEVCTL2 register. And the flag pci_dev->ltr_path will
> > be set as 1.
>
> s/it's/its/ twice above.
> It's == It is.
> Its == belonging to 'it'.
> Weird, I know, but that's English for you :)
>
> > For some pcie products, pcie link becomes down when device reset. And then
> > the LTR mechanism enable bit of bridge will become 0 based on description
> > in PCIE r4.0, sec 7.8.16. However, the pci_dev->ltr_path value of bridge
> > is still 1. Remove and rescan flow could be triggered to recover after
> > device reset. In the bus rescan flow, the LTR mechanism of device will be
> > enabled in pci_configure_ltr() due to ltr_path of its bridge is still 1.
>
> s/pcie/PCIe/ twice above.
> s/PCIE/PCIe/; also reference r5.0 instead of r4.0 if possible.
>
Sorry for the misspelling and inconvenience. Thanks for the correction
and I will follow the suggestion in later patch.
> This sounds like a general problem of most device config bits being
> cleared by reset. Usually these are restored by pci_restore_state().
> Is that function missing a restore for PCI_EXP_DEVCTL2? I'd rather
> have a general-purpose way of restoring all the config state than
> little pieces scattered all over.
>
Actually it’s not PCI_EXP_DEVCTL2 restore missing issue. The
PCI_EXP_DEVCTL2 is correctly saved/restored during bridge runtime
suspend/resume.
It’s the issue that ltr_path value of the bridge mismatches the actual
value in bridge’s PCI_EXP_DEVCTL2.
Here is the scenario for the issue:
1.PCI bus scan done
-For both PCIe Device and bridge:
-"LTR Mechanism Enable" bit in PCI_EXP_DEVCTL2 is 1
- ltr_path value is 1
2.Device resets and PCIe link is down, then "LTR Mechanism Enable" bit
of bridge changes to 0 according to PCIe r5.0, sec 7.5.3.16.
-The ltr_path value of bridge is still 1 but the "LTR Mechanism
Enable" bit within PCI_EXP_DEVCTL2 changed to 0.
3.Trigger device removal and bridge enters runtime suspend flow.
-"LTR Mechanism Enable" bit of bridge is 0 and saved by
pci_save_state().
4.Trigger bus rescan and bridge enters runtime resume flow.
-"LTR Mechanism Enable" bit of bridge is restored by
pci_restore_state() and the value is 0.
5.Scan device and configure device's LTR in pci_configure_ltr().
-"LTR Mechanism Enable" bit of device is configured as 1 due to
bridge's ltr_path value is 1.
6.The "LTR Mechanism Enable" bit of device and bridge is different now.
-When device sends LTR Message, bridge will treat the Message as
Unsupported Request according to PCIe r5.0, sec 6.18.
This patch is used to make bridge's ltr_path value match "LTR Mechanism
Enable" bit within DEVCTL2 register and avoid the Unsupported Request.
> > When device's LTR mechanism is enabled, device will send LTR message to
> > bridge. Bridge receives the device's LTR message and found bridge's LTR
> > mechanism is disabled. Then the bridge will generate unsupported request
> > and other error handling flow will be triggered such as AER Non-Fatal
> > error handling.
> >
> > This patch is used to avoid this unsupported request and make the bridge's
> > ltr_path value is aligned with DEVCTL2 register value. Check bridge
> > register value if aligned with ltr_path in pci_configure_ltr(). If
> > register value is disable and the ltr_path is 1, we need configure
> > the register value as enable.
> >
> > Signed-off-by: Mingchuang Qiao <mingchuang.qiao@mediatek.com>
> > ---
> > drivers/pci/probe.c | 18 +++++++++++++++---
> > 1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 953f15abc850..49355cf4af54 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2132,9 +2132,21 @@ static void pci_configure_ltr(struct pci_dev *dev)
> > * Complex and all intermediate Switches indicate support for LTR.
> > * PCIe r4.0, sec 6.18.
> > */
> > - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> > - ((bridge = pci_upstream_bridge(dev)) &&
> > - bridge->ltr_path)) {
> > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> > + pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
> > + PCI_EXP_DEVCTL2_LTR_EN);
> > + dev->ltr_path = 1;
> > + return;
> > + }
> > +
> > + bridge = pci_upstream_bridge(dev);
> > + if (bridge && bridge->ltr_path) {
> > + pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, &ctl);
> > + if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) {
> > + pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
> > + PCI_EXP_DEVCTL2_LTR_EN);
> > + }
> > +
> > pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
> > PCI_EXP_DEVCTL2_LTR_EN);
> > dev->ltr_path = 1;
> > --
> > 2.18.0
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] pci: avoid unsync of LTR mechanism configuration
@ 2021-01-18 2:55 ` Mingchuang Qiao
0 siblings, 0 replies; 9+ messages in thread
From: Mingchuang Qiao @ 2021-01-18 2:55 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: kerun.zhu, linux-pci, lambert.wang, linux-kernel, matthias.bgg,
linux-mediatek, haijun.liu, bhelgaas, linux-arm-kernel
On Tue, 2021-01-12 at 15:36 -0600, Bjorn Helgaas wrote:
> Note subject line tips at https://lore.kernel.org/r/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com
>
> On Tue, Jan 12, 2021 at 03:27:39PM +0800, mingchuang.qiao@mediatek.com wrote:
> > From: Mingchuang Qiao <mingchuang.qiao@mediatek.com>
> >
> > In pci bus scan flow, the LTR mechanism enable bit of DEVCTL2 register
> > is configured in pci_configure_ltr(). If device and it's bridge both
> > support LTR mechanism, LTR mechanism of device and it's bridge will
> > be enabled in DEVCTL2 register. And the flag pci_dev->ltr_path will
> > be set as 1.
>
> s/it's/its/ twice above.
> It's == It is.
> Its == belonging to 'it'.
> Weird, I know, but that's English for you :)
>
> > For some pcie products, pcie link becomes down when device reset. And then
> > the LTR mechanism enable bit of bridge will become 0 based on description
> > in PCIE r4.0, sec 7.8.16. However, the pci_dev->ltr_path value of bridge
> > is still 1. Remove and rescan flow could be triggered to recover after
> > device reset. In the bus rescan flow, the LTR mechanism of device will be
> > enabled in pci_configure_ltr() due to ltr_path of its bridge is still 1.
>
> s/pcie/PCIe/ twice above.
> s/PCIE/PCIe/; also reference r5.0 instead of r4.0 if possible.
>
Sorry for the misspelling and inconvenience. Thanks for the correction
and I will follow the suggestion in later patch.
> This sounds like a general problem of most device config bits being
> cleared by reset. Usually these are restored by pci_restore_state().
> Is that function missing a restore for PCI_EXP_DEVCTL2? I'd rather
> have a general-purpose way of restoring all the config state than
> little pieces scattered all over.
>
Actually it’s not PCI_EXP_DEVCTL2 restore missing issue. The
PCI_EXP_DEVCTL2 is correctly saved/restored during bridge runtime
suspend/resume.
It’s the issue that ltr_path value of the bridge mismatches the actual
value in bridge’s PCI_EXP_DEVCTL2.
Here is the scenario for the issue:
1.PCI bus scan done
-For both PCIe Device and bridge:
-"LTR Mechanism Enable" bit in PCI_EXP_DEVCTL2 is 1
- ltr_path value is 1
2.Device resets and PCIe link is down, then "LTR Mechanism Enable" bit
of bridge changes to 0 according to PCIe r5.0, sec 7.5.3.16.
-The ltr_path value of bridge is still 1 but the "LTR Mechanism
Enable" bit within PCI_EXP_DEVCTL2 changed to 0.
3.Trigger device removal and bridge enters runtime suspend flow.
-"LTR Mechanism Enable" bit of bridge is 0 and saved by
pci_save_state().
4.Trigger bus rescan and bridge enters runtime resume flow.
-"LTR Mechanism Enable" bit of bridge is restored by
pci_restore_state() and the value is 0.
5.Scan device and configure device's LTR in pci_configure_ltr().
-"LTR Mechanism Enable" bit of device is configured as 1 due to
bridge's ltr_path value is 1.
6.The "LTR Mechanism Enable" bit of device and bridge is different now.
-When device sends LTR Message, bridge will treat the Message as
Unsupported Request according to PCIe r5.0, sec 6.18.
This patch is used to make bridge's ltr_path value match "LTR Mechanism
Enable" bit within DEVCTL2 register and avoid the Unsupported Request.
> > When device's LTR mechanism is enabled, device will send LTR message to
> > bridge. Bridge receives the device's LTR message and found bridge's LTR
> > mechanism is disabled. Then the bridge will generate unsupported request
> > and other error handling flow will be triggered such as AER Non-Fatal
> > error handling.
> >
> > This patch is used to avoid this unsupported request and make the bridge's
> > ltr_path value is aligned with DEVCTL2 register value. Check bridge
> > register value if aligned with ltr_path in pci_configure_ltr(). If
> > register value is disable and the ltr_path is 1, we need configure
> > the register value as enable.
> >
> > Signed-off-by: Mingchuang Qiao <mingchuang.qiao@mediatek.com>
> > ---
> > drivers/pci/probe.c | 18 +++++++++++++++---
> > 1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 953f15abc850..49355cf4af54 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2132,9 +2132,21 @@ static void pci_configure_ltr(struct pci_dev *dev)
> > * Complex and all intermediate Switches indicate support for LTR.
> > * PCIe r4.0, sec 6.18.
> > */
> > - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> > - ((bridge = pci_upstream_bridge(dev)) &&
> > - bridge->ltr_path)) {
> > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> > + pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
> > + PCI_EXP_DEVCTL2_LTR_EN);
> > + dev->ltr_path = 1;
> > + return;
> > + }
> > +
> > + bridge = pci_upstream_bridge(dev);
> > + if (bridge && bridge->ltr_path) {
> > + pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, &ctl);
> > + if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) {
> > + pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
> > + PCI_EXP_DEVCTL2_LTR_EN);
> > + }
> > +
> > pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
> > PCI_EXP_DEVCTL2_LTR_EN);
> > dev->ltr_path = 1;
> > --
> > 2.18.0
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] pci: avoid unsync of LTR mechanism configuration
@ 2021-01-18 2:55 ` Mingchuang Qiao
0 siblings, 0 replies; 9+ messages in thread
From: Mingchuang Qiao @ 2021-01-18 2:55 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: kerun.zhu, linux-pci, lambert.wang, linux-kernel, matthias.bgg,
linux-mediatek, haijun.liu, bhelgaas, linux-arm-kernel
On Tue, 2021-01-12 at 15:36 -0600, Bjorn Helgaas wrote:
> Note subject line tips at https://lore.kernel.org/r/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com
>
> On Tue, Jan 12, 2021 at 03:27:39PM +0800, mingchuang.qiao@mediatek.com wrote:
> > From: Mingchuang Qiao <mingchuang.qiao@mediatek.com>
> >
> > In pci bus scan flow, the LTR mechanism enable bit of DEVCTL2 register
> > is configured in pci_configure_ltr(). If device and it's bridge both
> > support LTR mechanism, LTR mechanism of device and it's bridge will
> > be enabled in DEVCTL2 register. And the flag pci_dev->ltr_path will
> > be set as 1.
>
> s/it's/its/ twice above.
> It's == It is.
> Its == belonging to 'it'.
> Weird, I know, but that's English for you :)
>
> > For some pcie products, pcie link becomes down when device reset. And then
> > the LTR mechanism enable bit of bridge will become 0 based on description
> > in PCIE r4.0, sec 7.8.16. However, the pci_dev->ltr_path value of bridge
> > is still 1. Remove and rescan flow could be triggered to recover after
> > device reset. In the bus rescan flow, the LTR mechanism of device will be
> > enabled in pci_configure_ltr() due to ltr_path of its bridge is still 1.
>
> s/pcie/PCIe/ twice above.
> s/PCIE/PCIe/; also reference r5.0 instead of r4.0 if possible.
>
Sorry for the misspelling and inconvenience. Thanks for the correction
and I will follow the suggestion in later patch.
> This sounds like a general problem of most device config bits being
> cleared by reset. Usually these are restored by pci_restore_state().
> Is that function missing a restore for PCI_EXP_DEVCTL2? I'd rather
> have a general-purpose way of restoring all the config state than
> little pieces scattered all over.
>
Actually it’s not PCI_EXP_DEVCTL2 restore missing issue. The
PCI_EXP_DEVCTL2 is correctly saved/restored during bridge runtime
suspend/resume.
It’s the issue that ltr_path value of the bridge mismatches the actual
value in bridge’s PCI_EXP_DEVCTL2.
Here is the scenario for the issue:
1.PCI bus scan done
-For both PCIe Device and bridge:
-"LTR Mechanism Enable" bit in PCI_EXP_DEVCTL2 is 1
- ltr_path value is 1
2.Device resets and PCIe link is down, then "LTR Mechanism Enable" bit
of bridge changes to 0 according to PCIe r5.0, sec 7.5.3.16.
-The ltr_path value of bridge is still 1 but the "LTR Mechanism
Enable" bit within PCI_EXP_DEVCTL2 changed to 0.
3.Trigger device removal and bridge enters runtime suspend flow.
-"LTR Mechanism Enable" bit of bridge is 0 and saved by
pci_save_state().
4.Trigger bus rescan and bridge enters runtime resume flow.
-"LTR Mechanism Enable" bit of bridge is restored by
pci_restore_state() and the value is 0.
5.Scan device and configure device's LTR in pci_configure_ltr().
-"LTR Mechanism Enable" bit of device is configured as 1 due to
bridge's ltr_path value is 1.
6.The "LTR Mechanism Enable" bit of device and bridge is different now.
-When device sends LTR Message, bridge will treat the Message as
Unsupported Request according to PCIe r5.0, sec 6.18.
This patch is used to make bridge's ltr_path value match "LTR Mechanism
Enable" bit within DEVCTL2 register and avoid the Unsupported Request.
> > When device's LTR mechanism is enabled, device will send LTR message to
> > bridge. Bridge receives the device's LTR message and found bridge's LTR
> > mechanism is disabled. Then the bridge will generate unsupported request
> > and other error handling flow will be triggered such as AER Non-Fatal
> > error handling.
> >
> > This patch is used to avoid this unsupported request and make the bridge's
> > ltr_path value is aligned with DEVCTL2 register value. Check bridge
> > register value if aligned with ltr_path in pci_configure_ltr(). If
> > register value is disable and the ltr_path is 1, we need configure
> > the register value as enable.
> >
> > Signed-off-by: Mingchuang Qiao <mingchuang.qiao@mediatek.com>
> > ---
> > drivers/pci/probe.c | 18 +++++++++++++++---
> > 1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 953f15abc850..49355cf4af54 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2132,9 +2132,21 @@ static void pci_configure_ltr(struct pci_dev *dev)
> > * Complex and all intermediate Switches indicate support for LTR.
> > * PCIe r4.0, sec 6.18.
> > */
> > - if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> > - ((bridge = pci_upstream_bridge(dev)) &&
> > - bridge->ltr_path)) {
> > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> > + pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
> > + PCI_EXP_DEVCTL2_LTR_EN);
> > + dev->ltr_path = 1;
> > + return;
> > + }
> > +
> > + bridge = pci_upstream_bridge(dev);
> > + if (bridge && bridge->ltr_path) {
> > + pcie_capability_read_dword(bridge, PCI_EXP_DEVCTL2, &ctl);
> > + if (!(ctl & PCI_EXP_DEVCTL2_LTR_EN)) {
> > + pcie_capability_set_word(bridge, PCI_EXP_DEVCTL2,
> > + PCI_EXP_DEVCTL2_LTR_EN);
> > + }
> > +
> > pcie_capability_set_word(dev, PCI_EXP_DEVCTL2,
> > PCI_EXP_DEVCTL2_LTR_EN);
> > dev->ltr_path = 1;
> > --
> > 2.18.0
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 9+ messages in thread