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@xxxxxxxx>
>>> Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
>>> ---
>>> 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:
>>>
>>
