Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1

From: Sergio Paracuellos <sergio.paracuellos@gmail.com>
To: "Arınç ÜNAL" <arinc.unal@arinc9.com>
Cc: Greg KH <gregkh@linuxfoundation.org>,
	Sander Vanheule <sander@svanheule.net>,
	 NeilBrown <neil@brown.name>, DENG Qingfang <dqfext@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	 erkin.bozoglu@xeront.com, linux-staging@lists.linux.dev
Subject: Re: [PATCH 2/2] staging: mt7621-dts: do not use rgmii2_pins for ethernet on GB-PC1
Date: Tue, 15 Feb 2022 16:16:07 +0100	[thread overview]
Message-ID: <CAMhs-H-bMgsssuy-Rrygr0PCVkgDWDi=-=3vDVG_cnMX4L9vOA@mail.gmail.com> (raw)
In-Reply-To: <6902e0b3-5bd3-185b-26f8-eab2e37f29cb@arinc9.com>

Hi Arinc,

On Tue, Feb 15, 2022 at 3:11 PM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
>
> On 15/02/2022 12:09, Sergio Paracuellos wrote:
> > Hi Arinc,
> >
> > On Tue, Feb 15, 2022 at 9:50 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>
> >> Hey Sergio,
> >>
> >> On 15/02/2022 11:35, Sergio Paracuellos wrote:
> >>> Hi Arinc,
> >>>
> >>> On Tue, Feb 15, 2022 at 9:18 AM Arınç ÜNAL <arinc.unal@arinc9.com> wrote:
> >>>>
> >>>> GB-PC1 uses some of the rgmii2 pins (22 - 33) as GPIO. Therefore, the
> >>>> rgmii2 bus cannot be used on this device.
> >>>> Overwrite pinctrl-0 property under the ethernet node without rgmii2_pins on
> >>>> the GB-PC1 devicetree.
> >>>>
> >>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> >>>> ---
> >>>>    drivers/staging/mt7621-dts/gbpc1.dts | 4 ++++
> >>>>    1 file changed, 4 insertions(+)
> >>>
> >>> No issues in GB-PC1. So:
> >>>
> >>> Tested-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> >>
> >> Thanks for testing it so quickly!
> >>
> >> I was wondering if you got pinctrl errors on the bootlog before applying
> >> this patch series.
> >>
> >> rgmii2 pin group is given gpio function so calling it from ethernet node
> >> would cause this on my TP-Link RE650 v1 which also uses the rgmii2_pins
> >> as GPIO.
> >>
> >> [    1.177349] rt2880-pinmux pinctrl: pin io22 already requested by
> >> pinctrl; cannot claim for 1e100000.ethernet
> >> [    1.196966] rt2880-pinmux pinctrl: pin-22 (1e100000.ethernet) status -22
> >> [    1.210312] rt2880-pinmux pinctrl: could not request pin 22 (io22)
> >> from group rgmii2  on device rt2880-pinmux
> >> [    1.230058] mtk_soc_eth 1e100000.ethernet: Error applying setting,
> >> reverse things back
> >> [    1.245853] mtk_soc_eth: probe of 1e100000.ethernet failed with error -22
> >
> > No, I was not getting any kind of error since when I test your last
> > patch series I was not experimenting any kind of regression. I don't
> > have any issues now also with your new patch series. Your new changes
> > make sense since as you have said "rgmii2" pins are requested as GPIO
> > but it seems are not really being requested? I don't have time to
> > check the datasheet now but will try to get time to see what is
> > happening there.
>
> I think this must have something to do with pinctrl on newer kernels as
> the TP-Link RE650 that I tested uses the OpenWrt master branch (Linux 5.10).

I think is this commit which I did according to a review after moving
the driver from staging into 'drivers/pincrtl/ralink':

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/drivers/pinctrl/ralink?h=staging-next&id=8a55d64c3336fc2ffd488a37d08ceab154c7b56b

You can also check other changes from where the driver was moved:

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/log/drivers/pinctrl/ralink?h=staging-next

>
> I hacked together something to run 5.17-rc1 for RE650 on OpenWrt. Here's
> what I found:
>
> When I still have rgmii2_pins on the ethernet node, I don't get any of
> the warnings above but this happens:
>
> - failsafe button lights_toggle was pressed -
> - failsafe -
>
> When I apply this patch and test again I don't get into failsafe anymore.
>
> The key for LIGHTS_TOGGLE is on GPIO 30 which is one of the rgmii2 pins.
> https://github.com/openwrt/openwrt/blob/master/target/linux/ramips/dts/mt7621_tplink_rexx0-v1.dtsi#L34
>
> What I make of this is, with newer kernels, pinctrl doesn't care and
> just claims the pin group for ethernet anyway. This would explain why
> the lights_toggle key GPIO goes from active low to high. So this patch
> is still a necessity.

Agreed, applying this patch is the correct thing to do.

Best regards,
    Sergio Paracuellos
>
> Arınç