Re: [PATCH wireless-drivers 1/2] mt76: mt76x2e: disable pcie_aspm by default

[Heiner Kallweit <hkallweit1@gmail.com>]

On 24.10.2019 23:54, Lorenzo Bianconi wrote:
>> On 24.10.2019 00:23, Lorenzo Bianconi wrote:
>>> On same device (e.g. U7612E-H1) PCIE_ASPM causes continuous mcu hangs and
>>> instability and so let's disable PCIE_ASPM by default. This patch has
>>> been successfully tested on U7612E-H1 mini-pice card
>>>
>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>>> ---
>>>  drivers/net/wireless/mediatek/mt76/mmio.c     | 47 +++++++++++++++++++
>>>  drivers/net/wireless/mediatek/mt76/mt76.h     |  1 +
>>>  .../net/wireless/mediatek/mt76/mt76x2/pci.c   |  2 +
>>>  3 files changed, 50 insertions(+)
>>>
> 
> [...]
> 
>>> +
>>> +	if (parent)
>>> +		pcie_capability_clear_word(parent, PCI_EXP_LNKCTL,
>>> +					   aspm_conf);
>>
>> + linux-pci mailing list
> 
> Hi Heiner,
> 
>>
>> All this seems to be legacy code copied from e1000e.
>> Fiddling with the low-level PCI(e) registers should be left to the
>> PCI core. It shouldn't be needed here, a simple call to
>> pci_disable_link_state() should be sufficient. Note that this function
>> has a return value meanwhile that you can check instead of reading
>> back low-level registers.
> 
> ack, I will add it to v2
> 
>> If BIOS forbids that OS changes ASPM settings, then this should be
>> respected (like PCI core does). Instead the network chip may provide
>> the option to configure whether it activates certain ASPM (sub-)states
>> or not. We went through a similar exercise with the r8169 driver,
>> you can check how it's done there.
> 
> looking at the vendor sdk (at least in the version I currently have) there are
> no particular ASPM configurations, it just optionally disables it writing directly
> in pci registers.
> Moreover there are multiple drivers that are currently using this approach:
> - ath9k in ath_pci_aspm_init()
> - tg3 in tg3_chip_reset()
> - e1000e in __e1000e_disable_aspm()
> - r8169 in rtl_enable_clock_request()/rtl_disable_clock_request()
> 
All these drivers include quite some legacy code. I can mainly speak for r8169:
First versions of the driver are almost as old as Linux. And even though I
refactored most of the driver still some legacy code for older chip versions
(like the two functions you mentioned) is included.

> Is disabling the ASPM for the system the only option to make this minipcie
> work?
> 

No. What we do in r8169:

- call pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1)
- If it returns 0, then ASPM (including the L1 sub-states) is disabled.
- If it returns an errno, then disabling ASPM failed (most likely due to
  BIOS forbidding ASPM changes - pci_disable_link_state will spit out
  a related warning). In this case r8169 configures the chip to not initiate
  transitions to L0s/L1 (the other end of the link may still try to enter
  ASPM states). See rtl_hw_aspm_clkreq_enable(). That's sufficient
  to avoid the ASPM-related problems with certain versions of this chip.
  Maybe your HW provides similar functionality.

> Regards,
> Lorenzo
> 
Heiner

>>
>>> +}
>>> +EXPORT_SYMBOL_GPL(mt76_mmio_disable_aspm);
>>> +
>>>  void mt76_mmio_init(struct mt76_dev *dev, void __iomem *regs)
>>>  {
>>>  	static const struct mt76_bus_ops mt76_mmio_ops = {
>>> diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
>>> index 570c159515a0..962812b6247d 100644
>>> --- a/drivers/net/wireless/mediatek/mt76/mt76.h
>>> +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
>>> @@ -578,6 +578,7 @@ bool __mt76_poll_msec(struct mt76_dev *dev, u32 offset, u32 mask, u32 val,
>>>  #define mt76_poll_msec(dev, ...) __mt76_poll_msec(&((dev)->mt76), __VA_ARGS__)
>>>  
>>>  void mt76_mmio_init(struct mt76_dev *dev, void __iomem *regs);
>>> +void mt76_mmio_disable_aspm(struct pci_dev *pdev);
>>>  
>>>  static inline u16 mt76_chip(struct mt76_dev *dev)
>>>  {
>>> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/pci.c b/drivers/net/wireless/mediatek/mt76/mt76x2/pci.c
>>> index 73c3104f8858..264bef87e5c7 100644
>>> --- a/drivers/net/wireless/mediatek/mt76/mt76x2/pci.c
>>> +++ b/drivers/net/wireless/mediatek/mt76/mt76x2/pci.c
>>> @@ -81,6 +81,8 @@ mt76pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>>  	/* RG_SSUSB_CDR_BR_PE1D = 0x3 */
>>>  	mt76_rmw_field(dev, 0x15c58, 0x3 << 6, 0x3);
>>>  
>>> +	mt76_mmio_disable_aspm(pdev);
>>> +
>>>  	return 0;
>>>  
>>>  error:
>>>
>>