Re: [PATCH v4 0/6] staging: mt7621-pci: re-do reset boot process

[Greg Ungerer <gerg@kernel.org>]

Hi Sergio,

On 19/3/20 11:43 pm, Sergio Paracuellos wrote:
> On Thu, Mar 19, 2020 at 1:42 PM Greg Ungerer <gerg@kernel.org> wrote:
>> On 14/3/20 6:09 am, Sergio Paracuellos wrote:
>>> Some time ago Greg Ungerer reported some random hangs using
>>> the staging mt7621-pci driver:
>>>
>>> See:
>>> * http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2019-June/134947.html
>>>
>>> Try to fix that is the main motivation of this patch series.
>>>
>>> Also in openwrt there is a driver for mt7621-pci which seems was rewritten
>>> from scratch (for kernel 4.14) by Ryder Lee and Weijie Gao from mediatek.
>>> There the approach for reset assert-deassert process is to set as 'gpio'
>>> the function for all the 'pcie' group for the pinctrl driver and use those
>>> gpio's as a reset for the end points. The driver I am talking about is still
>>> using legacy pci and legacy gpio kernel interfaces. IMHO, the correct thing
>>> to do is make this staging driver properly clean and functional and put it
>>> in its correct place in the mainline.
>>>
>>> See:
>>> * https://gist.github.com/dengqf6/7a9e9b4032d99f1a91dd9256c8a65c36
>>>
>>> Because of all of this this patch series tries to avoid random hangs of boot
>>> trying to use the 'reset-gpios' approach.
>>>
>>> Changes are being tested by openwrt people and seems to work.
>>>
>>> Hope this helps.
>>>
>>> Changes in v4:
>>> * Make use of 'devm_gpiod_get_index_optional' instead of 'devm_gpiod_get_index'.
>>> * Use 'dev_err' instead of 'dev_notice' and return ERR_PTR if
>>> 'devm_gpiod_get_index_optional' fails.
>>> * Rename pers dealy macro to PERST_DELAY_MS.
>>> * Add new patch 6 which removes function 'mt7621_reset_port' not needed anymore.
>>
>> Testing this v4 series always fails during boot with:
>>
>> ...
>> NET: Registered protocol family 17
>> NET: Registered protocol family 15
>> 8021q: 802.1Q VLAN Support v1.8
>> Loading compiled-in X.509 certificates
>> AppArmor: AppArmor sha1 policy hashing enabled
>>
>> rt2880-pinmux pinctrl: pcie is already enabled
>> mt7621-pci 1e140000.pcie: Error applying setting, reverse things back
>> mt7621-pci 1e140000.pcie: Failed to get GPIO for PCIe1
>> mt7621-pci 1e140000.pcie: Parsing DT failed
>> mt7621-pci: probe of 1e140000.pcie failed with error -16
> 
> Looks like the gpio is valid but has been assigned to anything else.
> It looks like a device-tree issue for me.
> Does your hardware follows the indications of the mediatek application note?

Yes, as per table 2-2. GPIO pins 7 and 8 are used for other purposes
on my hardware - and my devicetree has those assigned for those
other purposes. They are not available for, or used for, PCI reset.

Regards
Greg



> https://github.com/openwrt/openwrt/files/4317436/an-mt7621-pcie-application-note-v0.1.pdf
> 
> To be able to test this you can just change the device tree and set
> reset gpios to only perst-reset pin
> 
> reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>;
> 
> or avoid the "return PTR_ERR(port->gpio_rst);" after the call to
> 'devm_gpiod_get_index_optional'.
> 
> Or just make an exception if the pin is busy, which seems to be the
> problem here:
> 
>>
>> UBI error: cannot open mtd 3, error -19
>> hctosys: unable to open rtc device (rtc0)
>> cfg80211: Loading compiled-in X.509 certificates for regulatory database
>> cfg80211: Loaded X.509 cert 'sforshee: 00b28ddf47aef9cea7'
>> ...
>>
>> It never hangs, always boots up all the way. But always the same failure
>> with PCIe.
> 
> This series has been applied to the staging tree and are properly
> running for me in gnubee pc1.
> 
> You should test using all confirmed changes in staging-next branch and
> this patch which fix a wrong register usage issue:
> 
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2020-March/142472.html
> 
> 
>>
>> Regards
>> Greg
>>
>>
>>
>>> Changes in v3:
>>> * Avoid to fail if gpio descriptor fails on get.
>>> * re-do PATCH 1 commit message.
>>> * Take into account gpio low polarity on request and assert and deassert.
>>> * Review error path of driver to properly release gpio's resources.
>>>
>>> Changes in v2:
>>> * restore configuration for pers mode to GPIO.
>>> * Avoid to read FTS_NUM register in reset state.
>>> * Release gpio's patch added
>>>
>>> Best regards,
>>>       Sergio Paracuellos
>>>
>>>
>>> Sergio Paracuellos (6):
>>>     staging: mt7621-pci: use gpios for properly reset
>>>     staging: mt7621-pci: change value for 'PERST_DELAY_MS'
>>>     staging: mt7621-dts: make use of 'reset-gpios' property for pci
>>>     staging: mt7621-pci: bindings: update doc accordly to last changes
>>>     staging: mt7621-pci: release gpios after pci initialization
>>>     staging: mt7621-pci: delete no more needed 'mt7621_reset_port'
>>>
>>>    drivers/staging/mt7621-dts/mt7621.dtsi        |  11 +-
>>>    .../mt7621-pci/mediatek,mt7621-pci.txt        |   7 +-
>>>    drivers/staging/mt7621-pci/pci-mt7621.c       | 122 ++++++++++--------
>>>    3 files changed, 82 insertions(+), 58 deletions(-)
>>>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel