Re: [PATCH] drm/amdgpu: Don't enable LTR if not supported

Re: [PATCH] drm/amdgpu: Don't enable LTR if not supported

[Bjorn Helgaas]

On Thu, Sep 08, 2022 at 04:42:38PM +0000, Lazar, Lijo wrote:
> I am not sure if ASPM settings can be generalized by PCIE core.
> Performance vs Power savings when ASPM is enabled will require some
> additional tuning and that will be device specific.

Can you elaborate on this?  In the universe of drivers, very few do
their own ASPM configuration, and it's usually to work around hardware
defects, e.g., L1 doesn't work on some e1000e devices, L0s doesn't
work on some iwlwifi devices, etc.

The core does know how to configure all the ASPM features defined in
the PCIe spec, e.g., L0s, L1, L1.1, L1.2, and LTR.

> In some of the other ASICs, this programming is done in VBIOS/SBIOS
> firmware. Having it in driver provides the advantage of additional
> tuning without forcing a VBIOS upgrade.

I think it's clearly the intent of the PCIe spec that ASPM
configuration be done by generic code.  Here are some things that
require a system-level view, not just an individual device view:

  - L0s, L1, and L1 Substates cannot be enabled unless both ends
    support it (PCIe r6.0, secs 5.4.1.4, 7.5.3.7, 5.5.4).

  - Devices advertise the "Acceptable Latency" they can accept for
    transitions from L0s or L1 to L0, and the actual latency depends
    on the "Exit Latencies" of all the devices in the path to the Root
    Port (sec 5.4.1.3.2).

  - LTR (required by L1.2) cannot be enabled unless it is already
    enabled in all upstream devices (sec 6.18).  This patch relies on
    "ltr_path", which works now but relies on the PCI core never
    reconfiguring the upstream path.

There might be amdgpu-specific features the driver needs to set up,
but if drivers fiddle with architected features like LTR behind the
PCI core's back, things are likely to break.

> From: Alex Deucher <alexdeucher@gmail.com>
> On Thu, Sep 8, 2022 at 12:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote:

> > Do you know why the driver configures ASPM itself?  If the PCI core is
> > doing something wrong (and I'm sure it is, ASPM support is kind of a
> > mess), I'd much prefer to fix up the core where *all* drivers can
> > benefit from it.
> 
> This is the programming sequence we get from our hardware team and it
> is used on both windows and Linux.  As far as I understand it windows
> doesn't handle this in the core, it's up to the individual drivers to
> enable it.  I'm not familiar with how this should be enabled
> generically, but at least for our hardware, it seems to have some
> variation compared to what is done in the PCI core due to stability,
> etc. It seems to me that this may need asic specific implementations
> for a lot of hardware depending on the required programming sequences.
> E.g., various asics may need hardware workaround for bugs or platform
> issues, etc.  I can ask for more details from our hardware team.

If the PCI core has stability issues, I want to fix them.  This
hardware may have its own stability issues, and I would ideally like
to have drivers use interfaces like pci_disable_link_state() to avoid
broken things.  Maybe we need new interfaces for more subtle kinds of
breakage.

Bjorn

Re: [PATCH] drm/amdgpu: Don't enable LTR if not supported

[Alex Deucher]

On Thu, Sep 8, 2022 at 1:57 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Sep 08, 2022 at 04:42:38PM +0000, Lazar, Lijo wrote:
> > I am not sure if ASPM settings can be generalized by PCIE core.
> > Performance vs Power savings when ASPM is enabled will require some
> > additional tuning and that will be device specific.
>
> Can you elaborate on this?  In the universe of drivers, very few do
> their own ASPM configuration, and it's usually to work around hardware
> defects, e.g., L1 doesn't work on some e1000e devices, L0s doesn't
> work on some iwlwifi devices, etc.
>
> The core does know how to configure all the ASPM features defined in
> the PCIe spec, e.g., L0s, L1, L1.1, L1.2, and LTR.
>
> > In some of the other ASICs, this programming is done in VBIOS/SBIOS
> > firmware. Having it in driver provides the advantage of additional
> > tuning without forcing a VBIOS upgrade.
>
> I think it's clearly the intent of the PCIe spec that ASPM
> configuration be done by generic code.  Here are some things that
> require a system-level view, not just an individual device view:
>
>   - L0s, L1, and L1 Substates cannot be enabled unless both ends
>     support it (PCIe r6.0, secs 5.4.1.4, 7.5.3.7, 5.5.4).
>
>   - Devices advertise the "Acceptable Latency" they can accept for
>     transitions from L0s or L1 to L0, and the actual latency depends
>     on the "Exit Latencies" of all the devices in the path to the Root
>     Port (sec 5.4.1.3.2).
>
>   - LTR (required by L1.2) cannot be enabled unless it is already
>     enabled in all upstream devices (sec 6.18).  This patch relies on
>     "ltr_path", which works now but relies on the PCI core never
>     reconfiguring the upstream path.
>
> There might be amdgpu-specific features the driver needs to set up,
> but if drivers fiddle with architected features like LTR behind the
> PCI core's back, things are likely to break.
>
> > From: Alex Deucher <alexdeucher@gmail.com>
> > On Thu, Sep 8, 2022 at 12:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> > > Do you know why the driver configures ASPM itself?  If the PCI core is
> > > doing something wrong (and I'm sure it is, ASPM support is kind of a
> > > mess), I'd much prefer to fix up the core where *all* drivers can
> > > benefit from it.
> >
> > This is the programming sequence we get from our hardware team and it
> > is used on both windows and Linux.  As far as I understand it windows
> > doesn't handle this in the core, it's up to the individual drivers to
> > enable it.  I'm not familiar with how this should be enabled
> > generically, but at least for our hardware, it seems to have some
> > variation compared to what is done in the PCI core due to stability,
> > etc. It seems to me that this may need asic specific implementations
> > for a lot of hardware depending on the required programming sequences.
> > E.g., various asics may need hardware workaround for bugs or platform
> > issues, etc.  I can ask for more details from our hardware team.
>
> If the PCI core has stability issues, I want to fix them.  This
> hardware may have its own stability issues, and I would ideally like
> to have drivers use interfaces like pci_disable_link_state() to avoid
> broken things.  Maybe we need new interfaces for more subtle kinds of
> breakage.

I'm not sure what, if anything is wrong with the current generic PCIe
ASPM code in Linux.  I was speaking more from a hardware validation
standpoint.  E.g., our silicon validation and hardware teams run a lot
of tests on a bunch of platforms and tune the programming sequence for
speed/power/stability.  Then they hand the programming sequence off to
the software teams as sort of a golden config or rule set for ASPM
enablement in the OS for each device.  I'm not exactly sure how far
these sequences stray from what the core PCI code does.  Will try and
find out more.

Alex

Re: [PATCH] drm/amdgpu: Don't enable LTR if not supported

[Lazar, Lijo]

On 9/8/2022 11:27 PM, Bjorn Helgaas wrote:
> On Thu, Sep 08, 2022 at 04:42:38PM +0000, Lazar, Lijo wrote:
>> I am not sure if ASPM settings can be generalized by PCIE core.
>> Performance vs Power savings when ASPM is enabled will require some
>> additional tuning and that will be device specific.
> 
> Can you elaborate on this?  In the universe of drivers, very few do
> their own ASPM configuration, and it's usually to work around hardware
> defects, e.g., L1 doesn't work on some e1000e devices, L0s doesn't
> work on some iwlwifi devices, etc.
> 
> The core does know how to configure all the ASPM features defined in
> the PCIe spec, e.g., L0s, L1, L1.1, L1.2, and LTR.
> 
>> In some of the other ASICs, this programming is done in VBIOS/SBIOS
>> firmware. Having it in driver provides the advantage of additional
>> tuning without forcing a VBIOS upgrade.
> 
> I think it's clearly the intent of the PCIe spec that ASPM
> configuration be done by generic code.  Here are some things that
> require a system-level view, not just an individual device view:
> 
>    - L0s, L1, and L1 Substates cannot be enabled unless both ends
>      support it (PCIe r6.0, secs 5.4.1.4, 7.5.3.7, 5.5.4).
> 
>    - Devices advertise the "Acceptable Latency" they can accept for
>      transitions from L0s or L1 to L0, and the actual latency depends
>      on the "Exit Latencies" of all the devices in the path to the Root
>      Port (sec 5.4.1.3.2).
> 
>    - LTR (required by L1.2) cannot be enabled unless it is already
>      enabled in all upstream devices (sec 6.18).  This patch relies on
>      "ltr_path", which works now but relies on the PCI core never
>      reconfiguring the upstream path.
> 
> There might be amdgpu-specific features the driver needs to set up,
> but if drivers fiddle with architected features like LTR behind the
> PCI core's back, things are likely to break.
> 

The programming is mostly related to entry conditions and spec leaves it 
to implementation.

 From r4.0 spec -
"
This specification does not dictate when a component with an Upstream 
Port must initiate a transition to the L1 state. The interoperable 
mechanisms for transitioning into and out of L1 are defined within this 
specification; however, the specific ASPM policy governing when to 
transition into L1 is left to the implementer.
...
Another approach would be for the Downstream device to initiate a 
transition to the L1 state once the Link has been idle in L0 for a set 
amount of time.
"

Some of the programming like below relates to timings for entry.

         def = data = RREG32_SOC15(NBIO, 0, regRCC_STRAP0_RCC_BIF_STRAP3);
         data |= 0x5DE0 << 
RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
         data |= 0x0010 << 
RCC_BIF_STRAP3__STRAP_VLINK_PM_L1_ENTRY_TIMER__SHIFT;
         if (def != data)
                 WREG32_SOC15(NBIO, 0, regRCC_STRAP0_RCC_BIF_STRAP3, data);

Similarly for LTR, as it provides a dynamic mechanism to report 
tolerance while in L1 substates, the tolerance timings can be tuned 
through registers though there is a threshold.

Regardless, Alex is already checking with hardware design team on 
possible improvements.

Thanks,
Lijo

>> From: Alex Deucher <alexdeucher@gmail.com>
>> On Thu, Sep 8, 2022 at 12:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
>>> Do you know why the driver configures ASPM itself?  If the PCI core is
>>> doing something wrong (and I'm sure it is, ASPM support is kind of a
>>> mess), I'd much prefer to fix up the core where *all* drivers can
>>> benefit from it.
>>
>> This is the programming sequence we get from our hardware team and it
>> is used on both windows and Linux.  As far as I understand it windows
>> doesn't handle this in the core, it's up to the individual drivers to
>> enable it.  I'm not familiar with how this should be enabled
>> generically, but at least for our hardware, it seems to have some
>> variation compared to what is done in the PCI core due to stability,
>> etc. It seems to me that this may need asic specific implementations
>> for a lot of hardware depending on the required programming sequences.
>> E.g., various asics may need hardware workaround for bugs or platform
>> issues, etc.  I can ask for more details from our hardware team.
> 
> If the PCI core has stability issues, I want to fix them.  This
> hardware may have its own stability issues, and I would ideally like
> to have drivers use interfaces like pci_disable_link_state() to avoid
> broken things.  Maybe we need new interfaces for more subtle kinds of
> breakage.
> 
> Bjorn
> 

Re: [PATCH] drm/amdgpu: Don't enable LTR if not supported

[Bjorn Helgaas]

On Fri, Sep 09, 2022 at 01:11:54PM +0530, Lazar, Lijo wrote:
> 
> 
> On 9/8/2022 11:27 PM, Bjorn Helgaas wrote:
> > On Thu, Sep 08, 2022 at 04:42:38PM +0000, Lazar, Lijo wrote:
> > > I am not sure if ASPM settings can be generalized by PCIE core.
> > > Performance vs Power savings when ASPM is enabled will require some
> > > additional tuning and that will be device specific.
> > 
> > Can you elaborate on this?  In the universe of drivers, very few do
> > their own ASPM configuration, and it's usually to work around hardware
> > defects, e.g., L1 doesn't work on some e1000e devices, L0s doesn't
> > work on some iwlwifi devices, etc.
> > 
> > The core does know how to configure all the ASPM features defined in
> > the PCIe spec, e.g., L0s, L1, L1.1, L1.2, and LTR.
> > 
> > > In some of the other ASICs, this programming is done in VBIOS/SBIOS
> > > firmware. Having it in driver provides the advantage of additional
> > > tuning without forcing a VBIOS upgrade.
> > 
> > I think it's clearly the intent of the PCIe spec that ASPM
> > configuration be done by generic code.  Here are some things that
> > require a system-level view, not just an individual device view:
> > 
> >    - L0s, L1, and L1 Substates cannot be enabled unless both ends
> >      support it (PCIe r6.0, secs 5.4.1.4, 7.5.3.7, 5.5.4).
> > 
> >    - Devices advertise the "Acceptable Latency" they can accept for
> >      transitions from L0s or L1 to L0, and the actual latency depends
> >      on the "Exit Latencies" of all the devices in the path to the Root
> >      Port (sec 5.4.1.3.2).
> > 
> >    - LTR (required by L1.2) cannot be enabled unless it is already
> >      enabled in all upstream devices (sec 6.18).  This patch relies on
> >      "ltr_path", which works now but relies on the PCI core never
> >      reconfiguring the upstream path.
> > 
> > There might be amdgpu-specific features the driver needs to set up,
> > but if drivers fiddle with architected features like LTR behind the
> > PCI core's back, things are likely to break.
> > 
> 
> The programming is mostly related to entry conditions and spec leaves it to
> implementation.
> 
> From r4.0 spec -
> "
> This specification does not dictate when a component with an Upstream Port
> must initiate a transition to the L1 state. The interoperable mechanisms for
> transitioning into and out of L1 are defined within this specification;
> however, the specific ASPM policy governing when to transition into L1 is
> left to the implementer.
> ...
> Another approach would be for the Downstream device to initiate a transition
> to the L1 state once the Link has been idle in L0 for a set amount of time.
> "
> 
> Some of the programming like below relates to timings for entry.
> 
>         def = data = RREG32_SOC15(NBIO, 0, regRCC_STRAP0_RCC_BIF_STRAP3);
>         data |= 0x5DE0 <<
> RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
>         data |= 0x0010 <<
> RCC_BIF_STRAP3__STRAP_VLINK_PM_L1_ENTRY_TIMER__SHIFT;
>         if (def != data)
>                 WREG32_SOC15(NBIO, 0, regRCC_STRAP0_RCC_BIF_STRAP3, data);
> 
> Similarly for LTR, as it provides a dynamic mechanism to report tolerance
> while in L1 substates, the tolerance timings can be tuned through registers
> though there is a threshold.

I don't object to the driver programming device-specific things,
although there might be issues if it does that after the core has
already configured and enabled ASPM -- the driver might need to
temporarily disable ASPM while it updates parameters, then re-enable
it.

I *do* object to the driver programming PCIe-generic things that the
PCI core thinks it owns.  It's especially annoying if the driver uses
device-specific #defines and access methods for generic PCIe things
because then we can't even find potential conflicts.

> > > From: Alex Deucher <alexdeucher@gmail.com>
> > > On Thu, Sep 8, 2022 at 12:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > 
> > > > Do you know why the driver configures ASPM itself?  If the PCI core is
> > > > doing something wrong (and I'm sure it is, ASPM support is kind of a
> > > > mess), I'd much prefer to fix up the core where *all* drivers can
> > > > benefit from it.
> > > 
> > > This is the programming sequence we get from our hardware team and it
> > > is used on both windows and Linux.  As far as I understand it windows
> > > doesn't handle this in the core, it's up to the individual drivers to
> > > enable it.  I'm not familiar with how this should be enabled
> > > generically, but at least for our hardware, it seems to have some
> > > variation compared to what is done in the PCI core due to stability,
> > > etc. It seems to me that this may need asic specific implementations
> > > for a lot of hardware depending on the required programming sequences.
> > > E.g., various asics may need hardware workaround for bugs or platform
> > > issues, etc.  I can ask for more details from our hardware team.
> > 
> > If the PCI core has stability issues, I want to fix them.  This
> > hardware may have its own stability issues, and I would ideally like
> > to have drivers use interfaces like pci_disable_link_state() to avoid
> > broken things.  Maybe we need new interfaces for more subtle kinds of
> > breakage.
> > 
> > Bjorn
> > 

[PATCH] drm/amdgpu: Don't enable LTR if not supported

[Lijo Lazar]

As per PCIE Base Spec r4.0 Section 6.18
'Software must not enable LTR in an Endpoint unless the Root Complex
and all intermediate Switches indicate support for LTR.'

This fixes the Unsupported Request error reported through AER during
ASPM enablement.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=216455

The error was unnoticed before and got visible because of the commit
referenced below. This doesn't fix anything in the commit below, rather
fixes the issue in amdgpu exposed by the commit. The reference is only
to associate this commit with below one so that both go together.

Fixes: 8795e182b02d ("PCI/portdrv: Don't disable AER reporting in get_port_device_capability()")

Reported-by: Gustaw Smolarczyk <wielkiegie@gmail.com>
Signed-off-by: Lijo Lazar <lijo.lazar@amd.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 9 ++++++++-
 drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c | 9 ++++++++-
 drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c | 9 ++++++++-
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
index b465baa26762..aa761ff3a5fa 100644
--- a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
+++ b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
@@ -380,6 +380,7 @@ static void nbio_v2_3_enable_aspm(struct amdgpu_device *adev,
 		WREG32_PCIE(smnPCIE_LC_CNTL, data);
 }
 
+#ifdef CONFIG_PCIEASPM
 static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
 {
 	uint32_t def, data;
@@ -401,9 +402,11 @@ static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
 	if (def != data)
 		WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
 }
+#endif
 
 static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
 {
+#ifdef CONFIG_PCIEASPM
 	uint32_t def, data;
 
 	def = data = RREG32_PCIE(smnPCIE_LC_CNTL);
@@ -459,7 +462,10 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
 	if (def != data)
 		WREG32_PCIE(smnPCIE_LC_CNTL6, data);
 
-	nbio_v2_3_program_ltr(adev);
+	/* Don't bother about LTR if LTR is not enabled
+	 * in the path */
+	if (adev->pdev->ltr_path)
+		nbio_v2_3_program_ltr(adev);
 
 	def = data = RREG32_SOC15(NBIO, 0, mmRCC_BIF_STRAP3);
 	data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
@@ -483,6 +489,7 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
 	data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
 	if (def != data)
 		WREG32_PCIE(smnPCIE_LC_CNTL3, data);
+#endif
 }
 
 static void nbio_v2_3_apply_lc_spc_mode_wa(struct amdgpu_device *adev)
diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
index f7f6ddebd3e4..37615a77287b 100644
--- a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
@@ -282,6 +282,7 @@ static void nbio_v6_1_init_registers(struct amdgpu_device *adev)
 			mmBIF_BX_DEV0_EPF0_VF0_HDP_MEM_COHERENCY_FLUSH_CNTL) << 2;
 }
 
+#ifdef CONFIG_PCIEASPM
 static void nbio_v6_1_program_ltr(struct amdgpu_device *adev)
 {
 	uint32_t def, data;
@@ -303,9 +304,11 @@ static void nbio_v6_1_program_ltr(struct amdgpu_device *adev)
 	if (def != data)
 		WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
 }
+#endif
 
 static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
 {
+#ifdef CONFIG_PCIEASPM
 	uint32_t def, data;
 
 	def = data = RREG32_PCIE(smnPCIE_LC_CNTL);
@@ -361,7 +364,10 @@ static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
 	if (def != data)
 		WREG32_PCIE(smnPCIE_LC_CNTL6, data);
 
-	nbio_v6_1_program_ltr(adev);
+	/* Don't bother about LTR if LTR is not enabled
+	 * in the path */
+	if (adev->pdev->ltr_path)
+		nbio_v6_1_program_ltr(adev);
 
 	def = data = RREG32_PCIE(smnRCC_BIF_STRAP3);
 	data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
@@ -385,6 +391,7 @@ static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
 	data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
 	if (def != data)
 		WREG32_PCIE(smnPCIE_LC_CNTL3, data);
+#endif
 }
 
 const struct amdgpu_nbio_funcs nbio_v6_1_funcs = {
diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
index 11848d1e238b..19455a725939 100644
--- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
+++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
@@ -673,6 +673,7 @@ struct amdgpu_nbio_ras nbio_v7_4_ras = {
 };
 
 
+#ifdef CONFIG_PCIEASPM
 static void nbio_v7_4_program_ltr(struct amdgpu_device *adev)
 {
 	uint32_t def, data;
@@ -694,9 +695,11 @@ static void nbio_v7_4_program_ltr(struct amdgpu_device *adev)
 	if (def != data)
 		WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
 }
+#endif
 
 static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
 {
+#ifdef CONFIG_PCIEASPM
 	uint32_t def, data;
 
 	if (adev->ip_versions[NBIO_HWIP][0] == IP_VERSION(7, 4, 4))
@@ -755,7 +758,10 @@ static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
 	if (def != data)
 		WREG32_PCIE(smnPCIE_LC_CNTL6, data);
 
-	nbio_v7_4_program_ltr(adev);
+	/* Don't bother about LTR if LTR is not enabled
+	 * in the path */
+	if (adev->pdev->ltr_path)
+		nbio_v7_4_program_ltr(adev);
 
 	def = data = RREG32_PCIE(smnRCC_BIF_STRAP3);
 	data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
@@ -779,6 +785,7 @@ static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
 	data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
 	if (def != data)
 		WREG32_PCIE(smnPCIE_LC_CNTL3, data);
+#endif
 }
 
 const struct amdgpu_nbio_funcs nbio_v7_4_funcs = {
-- 
2.25.1

Re: [PATCH] drm/amdgpu: Don't enable LTR if not supported

[Alex Deucher]

On Wed, Sep 7, 2022 at 11:24 PM Lijo Lazar <lijo.lazar@amd.com> wrote:
>
> As per PCIE Base Spec r4.0 Section 6.18
> 'Software must not enable LTR in an Endpoint unless the Root Complex
> and all intermediate Switches indicate support for LTR.'
>
> This fixes the Unsupported Request error reported through AER during
> ASPM enablement.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216455
>
> The error was unnoticed before and got visible because of the commit
> referenced below. This doesn't fix anything in the commit below, rather
> fixes the issue in amdgpu exposed by the commit. The reference is only
> to associate this commit with below one so that both go together.
>
> Fixes: 8795e182b02d ("PCI/portdrv: Don't disable AER reporting in get_port_device_capability()")
>
> Reported-by: Gustaw Smolarczyk <wielkiegie@gmail.com>
> Signed-off-by: Lijo Lazar <lijo.lazar@amd.com>
> Cc: stable@vger.kernel.org
> ---

Even though the ASPM code in si.c, cik.c, and vi.c doesn't mess with
LTR, it still sets up ASPM so shouldn't it be protected with
CONFIG_PCIEASPM as well?

Alex

>  drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 9 ++++++++-
>  drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c | 9 ++++++++-
>  drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c | 9 ++++++++-
>  3 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> index b465baa26762..aa761ff3a5fa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> @@ -380,6 +380,7 @@ static void nbio_v2_3_enable_aspm(struct amdgpu_device *adev,
>                 WREG32_PCIE(smnPCIE_LC_CNTL, data);
>  }
>
> +#ifdef CONFIG_PCIEASPM
>  static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
>  {
>         uint32_t def, data;
> @@ -401,9 +402,11 @@ static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
>         if (def != data)
>                 WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
>  }
> +#endif
>
>  static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
>  {
> +#ifdef CONFIG_PCIEASPM
>         uint32_t def, data;
>
>         def = data = RREG32_PCIE(smnPCIE_LC_CNTL);
> @@ -459,7 +462,10 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
>         if (def != data)
>                 WREG32_PCIE(smnPCIE_LC_CNTL6, data);
>
> -       nbio_v2_3_program_ltr(adev);
> +       /* Don't bother about LTR if LTR is not enabled
> +        * in the path */
> +       if (adev->pdev->ltr_path)
> +               nbio_v2_3_program_ltr(adev);
>
>         def = data = RREG32_SOC15(NBIO, 0, mmRCC_BIF_STRAP3);
>         data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
> @@ -483,6 +489,7 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
>         data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
>         if (def != data)
>                 WREG32_PCIE(smnPCIE_LC_CNTL3, data);
> +#endif
>  }
>
>  static void nbio_v2_3_apply_lc_spc_mode_wa(struct amdgpu_device *adev)
> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
> index f7f6ddebd3e4..37615a77287b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
> @@ -282,6 +282,7 @@ static void nbio_v6_1_init_registers(struct amdgpu_device *adev)
>                         mmBIF_BX_DEV0_EPF0_VF0_HDP_MEM_COHERENCY_FLUSH_CNTL) << 2;
>  }
>
> +#ifdef CONFIG_PCIEASPM
>  static void nbio_v6_1_program_ltr(struct amdgpu_device *adev)
>  {
>         uint32_t def, data;
> @@ -303,9 +304,11 @@ static void nbio_v6_1_program_ltr(struct amdgpu_device *adev)
>         if (def != data)
>                 WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
>  }
> +#endif
>
>  static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
>  {
> +#ifdef CONFIG_PCIEASPM
>         uint32_t def, data;
>
>         def = data = RREG32_PCIE(smnPCIE_LC_CNTL);
> @@ -361,7 +364,10 @@ static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
>         if (def != data)
>                 WREG32_PCIE(smnPCIE_LC_CNTL6, data);
>
> -       nbio_v6_1_program_ltr(adev);
> +       /* Don't bother about LTR if LTR is not enabled
> +        * in the path */
> +       if (adev->pdev->ltr_path)
> +               nbio_v6_1_program_ltr(adev);
>
>         def = data = RREG32_PCIE(smnRCC_BIF_STRAP3);
>         data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
> @@ -385,6 +391,7 @@ static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
>         data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
>         if (def != data)
>                 WREG32_PCIE(smnPCIE_LC_CNTL3, data);
> +#endif
>  }
>
>  const struct amdgpu_nbio_funcs nbio_v6_1_funcs = {
> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
> index 11848d1e238b..19455a725939 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
> @@ -673,6 +673,7 @@ struct amdgpu_nbio_ras nbio_v7_4_ras = {
>  };
>
>
> +#ifdef CONFIG_PCIEASPM
>  static void nbio_v7_4_program_ltr(struct amdgpu_device *adev)
>  {
>         uint32_t def, data;
> @@ -694,9 +695,11 @@ static void nbio_v7_4_program_ltr(struct amdgpu_device *adev)
>         if (def != data)
>                 WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
>  }
> +#endif
>
>  static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
>  {
> +#ifdef CONFIG_PCIEASPM
>         uint32_t def, data;
>
>         if (adev->ip_versions[NBIO_HWIP][0] == IP_VERSION(7, 4, 4))
> @@ -755,7 +758,10 @@ static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
>         if (def != data)
>                 WREG32_PCIE(smnPCIE_LC_CNTL6, data);
>
> -       nbio_v7_4_program_ltr(adev);
> +       /* Don't bother about LTR if LTR is not enabled
> +        * in the path */
> +       if (adev->pdev->ltr_path)
> +               nbio_v7_4_program_ltr(adev);
>
>         def = data = RREG32_PCIE(smnRCC_BIF_STRAP3);
>         data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
> @@ -779,6 +785,7 @@ static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
>         data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
>         if (def != data)
>                 WREG32_PCIE(smnPCIE_LC_CNTL3, data);
> +#endif
>  }
>
>  const struct amdgpu_nbio_funcs nbio_v7_4_funcs = {
> --
> 2.25.1
>

Re: [PATCH] drm/amdgpu: Don't enable LTR if not supported

[Lazar, Lijo]

On 9/8/2022 8:58 AM, Alex Deucher wrote:
> On Wed, Sep 7, 2022 at 11:24 PM Lijo Lazar <lijo.lazar@amd.com> wrote:
>>
>> As per PCIE Base Spec r4.0 Section 6.18
>> 'Software must not enable LTR in an Endpoint unless the Root Complex
>> and all intermediate Switches indicate support for LTR.'
>>
>> This fixes the Unsupported Request error reported through AER during
>> ASPM enablement.
>>
>> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D216455&amp;data=05%7C01%7Clijo.lazar%40amd.com%7Cc190635e13f047625b4508da914a47a5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637982045476774989%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=mNq7A7oT2VwZVc7WtYyWj0BRAXV5MzNLir0o4%2BKiWYU%3D&amp;reserved=0
>>
>> The error was unnoticed before and got visible because of the commit
>> referenced below. This doesn't fix anything in the commit below, rather
>> fixes the issue in amdgpu exposed by the commit. The reference is only
>> to associate this commit with below one so that both go together.
>>
>> Fixes: 8795e182b02d ("PCI/portdrv: Don't disable AER reporting in get_port_device_capability()")
>>
>> Reported-by: Gustaw Smolarczyk <wielkiegie@gmail.com>
>> Signed-off-by: Lijo Lazar <lijo.lazar@amd.com>
>> Cc: stable@vger.kernel.org
>> ---
> 
> Even though the ASPM code in si.c, cik.c, and vi.c doesn't mess with
> LTR, it still sets up ASPM so shouldn't it be protected with
> CONFIG_PCIEASPM as well?
> 

Yes, but it is only a compilation improvement and unrelated to this 
patch. We don't access any ASPM related kernel variables in those 
sequences. ltr_path variable used under this patch is declared under 
ASPM config.

Runtime protection is already there -
0ab5d711ec74 (drm/amd: Refactor `amdgpu_aspm` to be evaluated per device)

Thanks,
Lijo

> Alex
> 
>>   drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 9 ++++++++-
>>   drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c | 9 ++++++++-
>>   drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c | 9 ++++++++-
>>   3 files changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
>> index b465baa26762..aa761ff3a5fa 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
>> @@ -380,6 +380,7 @@ static void nbio_v2_3_enable_aspm(struct amdgpu_device *adev,
>>                  WREG32_PCIE(smnPCIE_LC_CNTL, data);
>>   }
>>
>> +#ifdef CONFIG_PCIEASPM
>>   static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
>>   {
>>          uint32_t def, data;
>> @@ -401,9 +402,11 @@ static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
>>          if (def != data)
>>                  WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
>>   }
>> +#endif
>>
>>   static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
>>   {
>> +#ifdef CONFIG_PCIEASPM
>>          uint32_t def, data;
>>
>>          def = data = RREG32_PCIE(smnPCIE_LC_CNTL);
>> @@ -459,7 +462,10 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
>>          if (def != data)
>>                  WREG32_PCIE(smnPCIE_LC_CNTL6, data);
>>
>> -       nbio_v2_3_program_ltr(adev);
>> +       /* Don't bother about LTR if LTR is not enabled
>> +        * in the path */
>> +       if (adev->pdev->ltr_path)
>> +               nbio_v2_3_program_ltr(adev);
>>
>>          def = data = RREG32_SOC15(NBIO, 0, mmRCC_BIF_STRAP3);
>>          data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
>> @@ -483,6 +489,7 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
>>          data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
>>          if (def != data)
>>                  WREG32_PCIE(smnPCIE_LC_CNTL3, data);
>> +#endif
>>   }
>>
>>   static void nbio_v2_3_apply_lc_spc_mode_wa(struct amdgpu_device *adev)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
>> index f7f6ddebd3e4..37615a77287b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
>> @@ -282,6 +282,7 @@ static void nbio_v6_1_init_registers(struct amdgpu_device *adev)
>>                          mmBIF_BX_DEV0_EPF0_VF0_HDP_MEM_COHERENCY_FLUSH_CNTL) << 2;
>>   }
>>
>> +#ifdef CONFIG_PCIEASPM
>>   static void nbio_v6_1_program_ltr(struct amdgpu_device *adev)
>>   {
>>          uint32_t def, data;
>> @@ -303,9 +304,11 @@ static void nbio_v6_1_program_ltr(struct amdgpu_device *adev)
>>          if (def != data)
>>                  WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
>>   }
>> +#endif
>>
>>   static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
>>   {
>> +#ifdef CONFIG_PCIEASPM
>>          uint32_t def, data;
>>
>>          def = data = RREG32_PCIE(smnPCIE_LC_CNTL);
>> @@ -361,7 +364,10 @@ static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
>>          if (def != data)
>>                  WREG32_PCIE(smnPCIE_LC_CNTL6, data);
>>
>> -       nbio_v6_1_program_ltr(adev);
>> +       /* Don't bother about LTR if LTR is not enabled
>> +        * in the path */
>> +       if (adev->pdev->ltr_path)
>> +               nbio_v6_1_program_ltr(adev);
>>
>>          def = data = RREG32_PCIE(smnRCC_BIF_STRAP3);
>>          data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
>> @@ -385,6 +391,7 @@ static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
>>          data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
>>          if (def != data)
>>                  WREG32_PCIE(smnPCIE_LC_CNTL3, data);
>> +#endif
>>   }
>>
>>   const struct amdgpu_nbio_funcs nbio_v6_1_funcs = {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
>> index 11848d1e238b..19455a725939 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
>> @@ -673,6 +673,7 @@ struct amdgpu_nbio_ras nbio_v7_4_ras = {
>>   };
>>
>>
>> +#ifdef CONFIG_PCIEASPM
>>   static void nbio_v7_4_program_ltr(struct amdgpu_device *adev)
>>   {
>>          uint32_t def, data;
>> @@ -694,9 +695,11 @@ static void nbio_v7_4_program_ltr(struct amdgpu_device *adev)
>>          if (def != data)
>>                  WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
>>   }
>> +#endif
>>
>>   static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
>>   {
>> +#ifdef CONFIG_PCIEASPM
>>          uint32_t def, data;
>>
>>          if (adev->ip_versions[NBIO_HWIP][0] == IP_VERSION(7, 4, 4))
>> @@ -755,7 +758,10 @@ static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
>>          if (def != data)
>>                  WREG32_PCIE(smnPCIE_LC_CNTL6, data);
>>
>> -       nbio_v7_4_program_ltr(adev);
>> +       /* Don't bother about LTR if LTR is not enabled
>> +        * in the path */
>> +       if (adev->pdev->ltr_path)
>> +               nbio_v7_4_program_ltr(adev);
>>
>>          def = data = RREG32_PCIE(smnRCC_BIF_STRAP3);
>>          data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
>> @@ -779,6 +785,7 @@ static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
>>          data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
>>          if (def != data)
>>                  WREG32_PCIE(smnPCIE_LC_CNTL3, data);
>> +#endif
>>   }
>>
>>   const struct amdgpu_nbio_funcs nbio_v7_4_funcs = {
>> --
>> 2.25.1
>>

Re: [PATCH] drm/amdgpu: Don't enable LTR if not supported

[Alex Deucher]

On Wed, Sep 7, 2022 at 11:40 PM Lazar, Lijo <lijo.lazar@amd.com> wrote:
>
>
>
> On 9/8/2022 8:58 AM, Alex Deucher wrote:
> > On Wed, Sep 7, 2022 at 11:24 PM Lijo Lazar <lijo.lazar@amd.com> wrote:
> >>
> >> As per PCIE Base Spec r4.0 Section 6.18
> >> 'Software must not enable LTR in an Endpoint unless the Root Complex
> >> and all intermediate Switches indicate support for LTR.'
> >>
> >> This fixes the Unsupported Request error reported through AER during
> >> ASPM enablement.
> >>
> >> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D216455&amp;data=05%7C01%7Clijo.lazar%40amd.com%7Cc190635e13f047625b4508da914a47a5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637982045476774989%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=mNq7A7oT2VwZVc7WtYyWj0BRAXV5MzNLir0o4%2BKiWYU%3D&amp;reserved=0
> >>
> >> The error was unnoticed before and got visible because of the commit
> >> referenced below. This doesn't fix anything in the commit below, rather
> >> fixes the issue in amdgpu exposed by the commit. The reference is only
> >> to associate this commit with below one so that both go together.
> >>
> >> Fixes: 8795e182b02d ("PCI/portdrv: Don't disable AER reporting in get_port_device_capability()")
> >>
> >> Reported-by: Gustaw Smolarczyk <wielkiegie@gmail.com>
> >> Signed-off-by: Lijo Lazar <lijo.lazar@amd.com>
> >> Cc: stable@vger.kernel.org
> >> ---
> >
> > Even though the ASPM code in si.c, cik.c, and vi.c doesn't mess with
> > LTR, it still sets up ASPM so shouldn't it be protected with
> > CONFIG_PCIEASPM as well?
> >
>
> Yes, but it is only a compilation improvement and unrelated to this
> patch. We don't access any ASPM related kernel variables in those
> sequences. ltr_path variable used under this patch is declared under
> ASPM config.

Thanks for the information.

Reviewed-by: Alex Deucher <alexander.deucher@amd.com?

>
> Runtime protection is already there -
> 0ab5d711ec74 (drm/amd: Refactor `amdgpu_aspm` to be evaluated per device)
>
> Thanks,
> Lijo
>
> > Alex
> >
> >>   drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 9 ++++++++-
> >>   drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c | 9 ++++++++-
> >>   drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c | 9 ++++++++-
> >>   3 files changed, 24 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> >> index b465baa26762..aa761ff3a5fa 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> >> @@ -380,6 +380,7 @@ static void nbio_v2_3_enable_aspm(struct amdgpu_device *adev,
> >>                  WREG32_PCIE(smnPCIE_LC_CNTL, data);
> >>   }
> >>
> >> +#ifdef CONFIG_PCIEASPM
> >>   static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
> >>   {
> >>          uint32_t def, data;
> >> @@ -401,9 +402,11 @@ static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
> >>          if (def != data)
> >>                  WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
> >>   }
> >> +#endif
> >>
> >>   static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
> >>   {
> >> +#ifdef CONFIG_PCIEASPM
> >>          uint32_t def, data;
> >>
> >>          def = data = RREG32_PCIE(smnPCIE_LC_CNTL);
> >> @@ -459,7 +462,10 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
> >>          if (def != data)
> >>                  WREG32_PCIE(smnPCIE_LC_CNTL6, data);
> >>
> >> -       nbio_v2_3_program_ltr(adev);
> >> +       /* Don't bother about LTR if LTR is not enabled
> >> +        * in the path */
> >> +       if (adev->pdev->ltr_path)
> >> +               nbio_v2_3_program_ltr(adev);
> >>
> >>          def = data = RREG32_SOC15(NBIO, 0, mmRCC_BIF_STRAP3);
> >>          data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
> >> @@ -483,6 +489,7 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
> >>          data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
> >>          if (def != data)
> >>                  WREG32_PCIE(smnPCIE_LC_CNTL3, data);
> >> +#endif
> >>   }
> >>
> >>   static void nbio_v2_3_apply_lc_spc_mode_wa(struct amdgpu_device *adev)
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
> >> index f7f6ddebd3e4..37615a77287b 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
> >> @@ -282,6 +282,7 @@ static void nbio_v6_1_init_registers(struct amdgpu_device *adev)
> >>                          mmBIF_BX_DEV0_EPF0_VF0_HDP_MEM_COHERENCY_FLUSH_CNTL) << 2;
> >>   }
> >>
> >> +#ifdef CONFIG_PCIEASPM
> >>   static void nbio_v6_1_program_ltr(struct amdgpu_device *adev)
> >>   {
> >>          uint32_t def, data;
> >> @@ -303,9 +304,11 @@ static void nbio_v6_1_program_ltr(struct amdgpu_device *adev)
> >>          if (def != data)
> >>                  WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
> >>   }
> >> +#endif
> >>
> >>   static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
> >>   {
> >> +#ifdef CONFIG_PCIEASPM
> >>          uint32_t def, data;
> >>
> >>          def = data = RREG32_PCIE(smnPCIE_LC_CNTL);
> >> @@ -361,7 +364,10 @@ static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
> >>          if (def != data)
> >>                  WREG32_PCIE(smnPCIE_LC_CNTL6, data);
> >>
> >> -       nbio_v6_1_program_ltr(adev);
> >> +       /* Don't bother about LTR if LTR is not enabled
> >> +        * in the path */
> >> +       if (adev->pdev->ltr_path)
> >> +               nbio_v6_1_program_ltr(adev);
> >>
> >>          def = data = RREG32_PCIE(smnRCC_BIF_STRAP3);
> >>          data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
> >> @@ -385,6 +391,7 @@ static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
> >>          data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
> >>          if (def != data)
> >>                  WREG32_PCIE(smnPCIE_LC_CNTL3, data);
> >> +#endif
> >>   }
> >>
> >>   const struct amdgpu_nbio_funcs nbio_v6_1_funcs = {
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
> >> index 11848d1e238b..19455a725939 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
> >> @@ -673,6 +673,7 @@ struct amdgpu_nbio_ras nbio_v7_4_ras = {
> >>   };
> >>
> >>
> >> +#ifdef CONFIG_PCIEASPM
> >>   static void nbio_v7_4_program_ltr(struct amdgpu_device *adev)
> >>   {
> >>          uint32_t def, data;
> >> @@ -694,9 +695,11 @@ static void nbio_v7_4_program_ltr(struct amdgpu_device *adev)
> >>          if (def != data)
> >>                  WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
> >>   }
> >> +#endif
> >>
> >>   static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
> >>   {
> >> +#ifdef CONFIG_PCIEASPM
> >>          uint32_t def, data;
> >>
> >>          if (adev->ip_versions[NBIO_HWIP][0] == IP_VERSION(7, 4, 4))
> >> @@ -755,7 +758,10 @@ static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
> >>          if (def != data)
> >>                  WREG32_PCIE(smnPCIE_LC_CNTL6, data);
> >>
> >> -       nbio_v7_4_program_ltr(adev);
> >> +       /* Don't bother about LTR if LTR is not enabled
> >> +        * in the path */
> >> +       if (adev->pdev->ltr_path)
> >> +               nbio_v7_4_program_ltr(adev);
> >>
> >>          def = data = RREG32_PCIE(smnRCC_BIF_STRAP3);
> >>          data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
> >> @@ -779,6 +785,7 @@ static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
> >>          data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
> >>          if (def != data)
> >>                  WREG32_PCIE(smnPCIE_LC_CNTL3, data);
> >> +#endif
> >>   }
> >>
> >>   const struct amdgpu_nbio_funcs nbio_v7_4_funcs = {
> >> --
> >> 2.25.1
> >>

Re: [PATCH] drm/amdgpu: Don't enable LTR if not supported

[Alex Deucher]

On Wed, Sep 7, 2022 at 11:53 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Wed, Sep 7, 2022 at 11:40 PM Lazar, Lijo <lijo.lazar@amd.com> wrote:
> >
> >
> >
> > On 9/8/2022 8:58 AM, Alex Deucher wrote:
> > > On Wed, Sep 7, 2022 at 11:24 PM Lijo Lazar <lijo.lazar@amd.com> wrote:
> > >>
> > >> As per PCIE Base Spec r4.0 Section 6.18
> > >> 'Software must not enable LTR in an Endpoint unless the Root Complex
> > >> and all intermediate Switches indicate support for LTR.'
> > >>
> > >> This fixes the Unsupported Request error reported through AER during
> > >> ASPM enablement.
> > >>
> > >> Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D216455&amp;data=05%7C01%7Clijo.lazar%40amd.com%7Cc190635e13f047625b4508da914a47a5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637982045476774989%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=mNq7A7oT2VwZVc7WtYyWj0BRAXV5MzNLir0o4%2BKiWYU%3D&amp;reserved=0
> > >>
> > >> The error was unnoticed before and got visible because of the commit
> > >> referenced below. This doesn't fix anything in the commit below, rather
> > >> fixes the issue in amdgpu exposed by the commit. The reference is only
> > >> to associate this commit with below one so that both go together.
> > >>
> > >> Fixes: 8795e182b02d ("PCI/portdrv: Don't disable AER reporting in get_port_device_capability()")
> > >>
> > >> Reported-by: Gustaw Smolarczyk <wielkiegie@gmail.com>
> > >> Signed-off-by: Lijo Lazar <lijo.lazar@amd.com>
> > >> Cc: stable@vger.kernel.org
> > >> ---
> > >
> > > Even though the ASPM code in si.c, cik.c, and vi.c doesn't mess with
> > > LTR, it still sets up ASPM so shouldn't it be protected with
> > > CONFIG_PCIEASPM as well?
> > >
> >
> > Yes, but it is only a compilation improvement and unrelated to this
> > patch. We don't access any ASPM related kernel variables in those
> > sequences. ltr_path variable used under this patch is declared under
> > ASPM config.
>
> Thanks for the information.
>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com?

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

>
> >
> > Runtime protection is already there -
> > 0ab5d711ec74 (drm/amd: Refactor `amdgpu_aspm` to be evaluated per device)
> >
> > Thanks,
> > Lijo
> >
> > > Alex
> > >
> > >>   drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 9 ++++++++-
> > >>   drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c | 9 ++++++++-
> > >>   drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c | 9 ++++++++-
> > >>   3 files changed, 24 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> > >> index b465baa26762..aa761ff3a5fa 100644
> > >> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> > >> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> > >> @@ -380,6 +380,7 @@ static void nbio_v2_3_enable_aspm(struct amdgpu_device *adev,
> > >>                  WREG32_PCIE(smnPCIE_LC_CNTL, data);
> > >>   }
> > >>
> > >> +#ifdef CONFIG_PCIEASPM
> > >>   static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
> > >>   {
> > >>          uint32_t def, data;
> > >> @@ -401,9 +402,11 @@ static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
> > >>          if (def != data)
> > >>                  WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
> > >>   }
> > >> +#endif
> > >>
> > >>   static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
> > >>   {
> > >> +#ifdef CONFIG_PCIEASPM
> > >>          uint32_t def, data;
> > >>
> > >>          def = data = RREG32_PCIE(smnPCIE_LC_CNTL);
> > >> @@ -459,7 +462,10 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
> > >>          if (def != data)
> > >>                  WREG32_PCIE(smnPCIE_LC_CNTL6, data);
> > >>
> > >> -       nbio_v2_3_program_ltr(adev);
> > >> +       /* Don't bother about LTR if LTR is not enabled
> > >> +        * in the path */
> > >> +       if (adev->pdev->ltr_path)
> > >> +               nbio_v2_3_program_ltr(adev);
> > >>
> > >>          def = data = RREG32_SOC15(NBIO, 0, mmRCC_BIF_STRAP3);
> > >>          data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
> > >> @@ -483,6 +489,7 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
> > >>          data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
> > >>          if (def != data)
> > >>                  WREG32_PCIE(smnPCIE_LC_CNTL3, data);
> > >> +#endif
> > >>   }
> > >>
> > >>   static void nbio_v2_3_apply_lc_spc_mode_wa(struct amdgpu_device *adev)
> > >> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
> > >> index f7f6ddebd3e4..37615a77287b 100644
> > >> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
> > >> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c
> > >> @@ -282,6 +282,7 @@ static void nbio_v6_1_init_registers(struct amdgpu_device *adev)
> > >>                          mmBIF_BX_DEV0_EPF0_VF0_HDP_MEM_COHERENCY_FLUSH_CNTL) << 2;
> > >>   }
> > >>
> > >> +#ifdef CONFIG_PCIEASPM
> > >>   static void nbio_v6_1_program_ltr(struct amdgpu_device *adev)
> > >>   {
> > >>          uint32_t def, data;
> > >> @@ -303,9 +304,11 @@ static void nbio_v6_1_program_ltr(struct amdgpu_device *adev)
> > >>          if (def != data)
> > >>                  WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
> > >>   }
> > >> +#endif
> > >>
> > >>   static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
> > >>   {
> > >> +#ifdef CONFIG_PCIEASPM
> > >>          uint32_t def, data;
> > >>
> > >>          def = data = RREG32_PCIE(smnPCIE_LC_CNTL);
> > >> @@ -361,7 +364,10 @@ static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
> > >>          if (def != data)
> > >>                  WREG32_PCIE(smnPCIE_LC_CNTL6, data);
> > >>
> > >> -       nbio_v6_1_program_ltr(adev);
> > >> +       /* Don't bother about LTR if LTR is not enabled
> > >> +        * in the path */
> > >> +       if (adev->pdev->ltr_path)
> > >> +               nbio_v6_1_program_ltr(adev);
> > >>
> > >>          def = data = RREG32_PCIE(smnRCC_BIF_STRAP3);
> > >>          data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
> > >> @@ -385,6 +391,7 @@ static void nbio_v6_1_program_aspm(struct amdgpu_device *adev)
> > >>          data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
> > >>          if (def != data)
> > >>                  WREG32_PCIE(smnPCIE_LC_CNTL3, data);
> > >> +#endif
> > >>   }
> > >>
> > >>   const struct amdgpu_nbio_funcs nbio_v6_1_funcs = {
> > >> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
> > >> index 11848d1e238b..19455a725939 100644
> > >> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
> > >> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c
> > >> @@ -673,6 +673,7 @@ struct amdgpu_nbio_ras nbio_v7_4_ras = {
> > >>   };
> > >>
> > >>
> > >> +#ifdef CONFIG_PCIEASPM
> > >>   static void nbio_v7_4_program_ltr(struct amdgpu_device *adev)
> > >>   {
> > >>          uint32_t def, data;
> > >> @@ -694,9 +695,11 @@ static void nbio_v7_4_program_ltr(struct amdgpu_device *adev)
> > >>          if (def != data)
> > >>                  WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
> > >>   }
> > >> +#endif
> > >>
> > >>   static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
> > >>   {
> > >> +#ifdef CONFIG_PCIEASPM
> > >>          uint32_t def, data;
> > >>
> > >>          if (adev->ip_versions[NBIO_HWIP][0] == IP_VERSION(7, 4, 4))
> > >> @@ -755,7 +758,10 @@ static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
> > >>          if (def != data)
> > >>                  WREG32_PCIE(smnPCIE_LC_CNTL6, data);
> > >>
> > >> -       nbio_v7_4_program_ltr(adev);
> > >> +       /* Don't bother about LTR if LTR is not enabled
> > >> +        * in the path */
> > >> +       if (adev->pdev->ltr_path)
> > >> +               nbio_v7_4_program_ltr(adev);
> > >>
> > >>          def = data = RREG32_PCIE(smnRCC_BIF_STRAP3);
> > >>          data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
> > >> @@ -779,6 +785,7 @@ static void nbio_v7_4_program_aspm(struct amdgpu_device *adev)
> > >>          data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
> > >>          if (def != data)
> > >>                  WREG32_PCIE(smnPCIE_LC_CNTL3, data);
> > >> +#endif
> > >>   }
> > >>
> > >>   const struct amdgpu_nbio_funcs nbio_v7_4_funcs = {
> > >> --
> > >> 2.25.1
> > >>

Re: [PATCH] drm/amdgpu: Don't enable LTR if not supported

[Bjorn Helgaas]

[+cc Evan, author of 62f8f5c3bfc2 ("drm/amdgpu: enable ASPM support
for PCIE 7.4.0/7.6.0")]

On Thu, Sep 08, 2022 at 08:53:44AM +0530, Lijo Lazar wrote:
> As per PCIE Base Spec r4.0 Section 6.18
> 'Software must not enable LTR in an Endpoint unless the Root Complex
> and all intermediate Switches indicate support for LTR.'
> 
> This fixes the Unsupported Request error reported through AER during
> ASPM enablement.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216455
> 
> The error was unnoticed before and got visible because of the commit
> referenced below. This doesn't fix anything in the commit below, rather
> fixes the issue in amdgpu exposed by the commit. The reference is only
> to associate this commit with below one so that both go together.
> 
> Fixes: 8795e182b02d ("PCI/portdrv: Don't disable AER reporting in get_port_device_capability()")
> 
> Reported-by: Gustaw Smolarczyk <wielkiegie@gmail.com>
> Signed-off-by: Lijo Lazar <lijo.lazar@amd.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 9 ++++++++-
>  drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c | 9 ++++++++-
>  drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c | 9 ++++++++-

nbio_v4_3_program_ltr() checks pdev->ltr_path itself instead of doing
it in *_program_aspm().  It'd be nice to use the same approach for all
versions.

I really don't like the fact that amdgpu does all this ASPM fiddling
in the driver in the first place.  ASPM should be configured by the
PCI core, not by each individual driver.  ASPM has all sorts of
requirements that relate to upstream devices, which I think amdgpu
ignores, but the core pays attention to.

Do you know why the driver configures ASPM itself?  If the PCI core is
doing something wrong (and I'm sure it is, ASPM support is kind of a
mess), I'd much prefer to fix up the core where *all* drivers can
benefit from it.

> diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> index b465baa26762..aa761ff3a5fa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> @@ -380,6 +380,7 @@ static void nbio_v2_3_enable_aspm(struct amdgpu_device *adev,
>  		WREG32_PCIE(smnPCIE_LC_CNTL, data);
>  }
>  
> +#ifdef CONFIG_PCIEASPM
>  static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
>  {
>  	uint32_t def, data;
> @@ -401,9 +402,11 @@ static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
>  	if (def != data)
>  		WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
>  }
> +#endif
>  
>  static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
>  {
> +#ifdef CONFIG_PCIEASPM
>  	uint32_t def, data;
>  
>  	def = data = RREG32_PCIE(smnPCIE_LC_CNTL);
> @@ -459,7 +462,10 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
>  	if (def != data)
>  		WREG32_PCIE(smnPCIE_LC_CNTL6, data);
>  
> -	nbio_v2_3_program_ltr(adev);
> +	/* Don't bother about LTR if LTR is not enabled
> +	 * in the path */
> +	if (adev->pdev->ltr_path)
> +		nbio_v2_3_program_ltr(adev);
>  
>  	def = data = RREG32_SOC15(NBIO, 0, mmRCC_BIF_STRAP3);
>  	data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
> @@ -483,6 +489,7 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
>  	data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
>  	if (def != data)
>  		WREG32_PCIE(smnPCIE_LC_CNTL3, data);
> +#endif
>  }

Re: [PATCH] drm/amdgpu: Don't enable LTR if not supported

[Alex Deucher]

On Thu, Sep 8, 2022 at 12:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Evan, author of 62f8f5c3bfc2 ("drm/amdgpu: enable ASPM support
> for PCIE 7.4.0/7.6.0")]
>
> On Thu, Sep 08, 2022 at 08:53:44AM +0530, Lijo Lazar wrote:
> > As per PCIE Base Spec r4.0 Section 6.18
> > 'Software must not enable LTR in an Endpoint unless the Root Complex
> > and all intermediate Switches indicate support for LTR.'
> >
> > This fixes the Unsupported Request error reported through AER during
> > ASPM enablement.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216455
> >
> > The error was unnoticed before and got visible because of the commit
> > referenced below. This doesn't fix anything in the commit below, rather
> > fixes the issue in amdgpu exposed by the commit. The reference is only
> > to associate this commit with below one so that both go together.
> >
> > Fixes: 8795e182b02d ("PCI/portdrv: Don't disable AER reporting in get_port_device_capability()")
> >
> > Reported-by: Gustaw Smolarczyk <wielkiegie@gmail.com>
> > Signed-off-by: Lijo Lazar <lijo.lazar@amd.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c | 9 ++++++++-
> >  drivers/gpu/drm/amd/amdgpu/nbio_v6_1.c | 9 ++++++++-
> >  drivers/gpu/drm/amd/amdgpu/nbio_v7_4.c | 9 ++++++++-
>
> nbio_v4_3_program_ltr() checks pdev->ltr_path itself instead of doing
> it in *_program_aspm().  It'd be nice to use the same approach for all
> versions.
>
> I really don't like the fact that amdgpu does all this ASPM fiddling
> in the driver in the first place.  ASPM should be configured by the
> PCI core, not by each individual driver.  ASPM has all sorts of
> requirements that relate to upstream devices, which I think amdgpu
> ignores, but the core pays attention to.
>
> Do you know why the driver configures ASPM itself?  If the PCI core is
> doing something wrong (and I'm sure it is, ASPM support is kind of a
> mess), I'd much prefer to fix up the core where *all* drivers can
> benefit from it.

This is the programming sequence we get from our hardware team and it
is used on both windows and Linux.  As far as I understand it windows
doesn't handle this in the core, it's up to the individual drivers to
enable it.  I'm not familiar with how this should be enabled
generically, but at least for our hardware, it seems to have some
variation compared to what is done in the PCI core due to stability,
etc. It seems to me that this may need asic specific implementations
for a lot of hardware depending on the required programming sequences.
E.g., various asics may need hardware workaround for bugs or platform
issues, etc.  I can ask for more details from our hardware team.

Alex

>
> > diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> > index b465baa26762..aa761ff3a5fa 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
> > @@ -380,6 +380,7 @@ static void nbio_v2_3_enable_aspm(struct amdgpu_device *adev,
> >               WREG32_PCIE(smnPCIE_LC_CNTL, data);
> >  }
> >
> > +#ifdef CONFIG_PCIEASPM
> >  static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
> >  {
> >       uint32_t def, data;
> > @@ -401,9 +402,11 @@ static void nbio_v2_3_program_ltr(struct amdgpu_device *adev)
> >       if (def != data)
> >               WREG32_PCIE(smnBIF_CFG_DEV0_EPF0_DEVICE_CNTL2, data);
> >  }
> > +#endif
> >
> >  static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
> >  {
> > +#ifdef CONFIG_PCIEASPM
> >       uint32_t def, data;
> >
> >       def = data = RREG32_PCIE(smnPCIE_LC_CNTL);
> > @@ -459,7 +462,10 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
> >       if (def != data)
> >               WREG32_PCIE(smnPCIE_LC_CNTL6, data);
> >
> > -     nbio_v2_3_program_ltr(adev);
> > +     /* Don't bother about LTR if LTR is not enabled
> > +      * in the path */
> > +     if (adev->pdev->ltr_path)
> > +             nbio_v2_3_program_ltr(adev);
> >
> >       def = data = RREG32_SOC15(NBIO, 0, mmRCC_BIF_STRAP3);
> >       data |= 0x5DE0 << RCC_BIF_STRAP3__STRAP_VLINK_ASPM_IDLE_TIMER__SHIFT;
> > @@ -483,6 +489,7 @@ static void nbio_v2_3_program_aspm(struct amdgpu_device *adev)
> >       data &= ~PCIE_LC_CNTL3__LC_DSC_DONT_ENTER_L23_AFTER_PME_ACK_MASK;
> >       if (def != data)
> >               WREG32_PCIE(smnPCIE_LC_CNTL3, data);
> > +#endif
> >  }