diff mbox series

[v3,5/6] dt-bindings: net: dsa: make reset optional and add rgmii-mode to mt7531

Message ID 20220507170440.64005-6-linux@fw-web.de (mailing list archive)
State New, archived
Headers show
Series Support mt7531 on BPI-R2 Pro | expand

Commit Message

Frank Wunderlich May 7, 2022, 5:04 p.m. UTC
From: Frank Wunderlich <frank-w@public-files.de>

Make reset optional as driver already supports it, allow port 5 as
cpu-port and phy-mode rgmii for mt7531 cpu-port.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 .../devicetree/bindings/net/dsa/mediatek,mt7530.yaml          | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Krzysztof Kozlowski May 7, 2022, 8:01 p.m. UTC | #1
On 07/05/2022 19:04, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Make reset optional as driver already supports it, 

I do not see the connection between hardware needing or not needing a
reset GPIO and a driver supporting it or not... What does it mean?

> allow port 5 as
> cpu-port 

How do you allow it here?

> and phy-mode rgmii for mt7531 cpu-port.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  .../devicetree/bindings/net/dsa/mediatek,mt7530.yaml          | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> index a7696d1b4a8c..d02faed41b2a 100644
> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
> @@ -55,6 +55,7 @@ description: |
>      On mt7531:
>        - "1000base-x"
>        - "2500base-x"
> +      - "rgmii"
>        - "sgmii"


Best regards,
Krzysztof
Frank Wunderlich May 8, 2022, 6:24 a.m. UTC | #2
Am 7. Mai 2022 22:01:22 MESZ schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
>On 07/05/2022 19:04, Frank Wunderlich wrote:
>> From: Frank Wunderlich <frank-w@public-files.de>
>> 
>> Make reset optional as driver already supports it, 
>
>I do not see the connection between hardware needing or not needing a
>reset GPIO and a driver supporting it or not... What does it mean?

My board has a shared gpio-reset between gmac and switch, so both will resetted if it is asserted. Currently it is set to the gmac and is aquired exclusive. Adding it to switch results in 2 problems:

- due to exclusive and already mapped to gmac, switch driver exits as it cannot get the reset-gpio again.
- if i drop the reset from gmac and add to switch, it resets the gmac and this takes too long for switch to get up. Of course i can increase the wait time after reset,but dropping reset here was the easier way.

Using reset only on gmac side brings the switch up.

>> allow port 5 as
>> cpu-port 
>
>How do you allow it here?

Argh, seems i accidentally removed this part and have not recognized while checking :(

It should only change description of reg for ports to:

"Port address described must be 5 or 6 for CPU port and from 0 to 5 for user ports."

regards Frank
Heiko Stübner May 8, 2022, 9:41 a.m. UTC | #3
Am Sonntag, 8. Mai 2022, 08:24:37 CEST schrieb Frank Wunderlich:
> Am 7. Mai 2022 22:01:22 MESZ schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
> >On 07/05/2022 19:04, Frank Wunderlich wrote:
> >> From: Frank Wunderlich <frank-w@public-files.de>
> >> 
> >> Make reset optional as driver already supports it, 
> >
> >I do not see the connection between hardware needing or not needing a
> >reset GPIO and a driver supporting it or not... What does it mean?
> 
> My board has a shared gpio-reset between gmac and switch, so both will resetted if it is asserted. Currently it is set to the gmac and is aquired exclusive. Adding it to switch results in 2 problems:
> 
> - due to exclusive and already mapped to gmac, switch driver exits as it cannot get the reset-gpio again.
> - if i drop the reset from gmac and add to switch, it resets the gmac and this takes too long for switch to get up. Of course i can increase the wait time after reset,but dropping reset here was the easier way.
> 
> Using reset only on gmac side brings the switch up.

I think the issue is more for the description itself.

Devicetree is only meant to describe the hardware and does in general don't
care how any firmware (Linux-kernel, *BSD, etc) handles it. So going with
"the kernel does it this way" is not a valid reason for a binding change ;-) .

Instead in general want to reason that there are boards without this reset
facility and thus make it optional for those.

Heiko

> >> allow port 5 as
> >> cpu-port 
> >
> >How do you allow it here?
> 
> Argh, seems i accidentally removed this part and have not recognized while checking :(
> 
> It should only change description of reg for ports to:
> 
> "Port address described must be 5 or 6 for CPU port and from 0 to 5 for user ports."
> 
> regards Frank
>
Frank Wunderlich May 8, 2022, 12:12 p.m. UTC | #4
Hi Heiko

> Gesendet: Sonntag, 08. Mai 2022 um 11:41 Uhr
> Von: "Heiko Stuebner" <heiko@sntech.de>
> Am Sonntag, 8. Mai 2022, 08:24:37 CEST schrieb Frank Wunderlich:
> > Am 7. Mai 2022 22:01:22 MESZ schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
> > >On 07/05/2022 19:04, Frank Wunderlich wrote:
> > >> From: Frank Wunderlich <frank-w@public-files.de>
> > >>
> > >> Make reset optional as driver already supports it,
> > >
> > >I do not see the connection between hardware needing or not needing a
> > >reset GPIO and a driver supporting it or not... What does it mean?
> >
> > My board has a shared gpio-reset between gmac and switch, so both will resetted if it
> > is asserted. Currently it is set to the gmac and is aquired exclusive. Adding it to switch results in 2 problems:
> >
> > - due to exclusive and already mapped to gmac, switch driver exits as it cannot get the reset-gpio again.
> > - if i drop the reset from gmac and add to switch, it resets the gmac and this takes too long for switch
> > to get up. Of course i can increase the wait time after reset,but dropping reset here was the easier way.
> >
> > Using reset only on gmac side brings the switch up.
>
> I think the issue is more for the description itself.
>
> Devicetree is only meant to describe the hardware and does in general don't
> care how any firmware (Linux-kernel, *BSD, etc) handles it. So going with
> "the kernel does it this way" is not a valid reason for a binding change ;-) .
>
> Instead in general want to reason that there are boards without this reset
> facility and thus make it optional for those.

if only the wording is the problem i try to rephrase it from hardware PoV.

maybe something like this?

https://github.com/frank-w/BPI-R2-4.14/commits/5.18-mt7531-mainline2/Documentation/devicetree/bindings/net/dsa/mediatek%2Cmt7530.yaml

Another way is maybe increasing the delay after the reset (to give more time all
come up again), but imho it is no good idea resetting the gmac/mdio-bus from the
child device.

have not looked into the gmac driver if this always  does the initial reset to
have a "clean state". In this initial reset the switch will be resetted too
and does not need an additional one which needs the gmac/mdio initialization
to be done again.

> > >> allow port 5 as
> > >> cpu-port
> > >
> > >How do you allow it here?
> >
> > Argh, seems i accidentally removed this part and have not recognized while checking :(
> >
> > It should only change description of reg for ports to:
> >
> > "Port address described must be 5 or 6 for CPU port and from 0 to 5 for user ports."

noticed that the target-phase is not removed but squashed in the first bindings-patch.
This was a rebasing error and not intented...will fix in next version.

regards Frank
Peter Geis May 8, 2022, 5:04 p.m. UTC | #5
On Sun, May 8, 2022 at 8:12 AM Frank Wunderlich <frank-w@public-files.de> wrote:
>
> Hi Heiko
>
> > Gesendet: Sonntag, 08. Mai 2022 um 11:41 Uhr
> > Von: "Heiko Stuebner" <heiko@sntech.de>
> > Am Sonntag, 8. Mai 2022, 08:24:37 CEST schrieb Frank Wunderlich:
> > > Am 7. Mai 2022 22:01:22 MESZ schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
> > > >On 07/05/2022 19:04, Frank Wunderlich wrote:
> > > >> From: Frank Wunderlich <frank-w@public-files.de>
> > > >>
> > > >> Make reset optional as driver already supports it,
> > > >
> > > >I do not see the connection between hardware needing or not needing a
> > > >reset GPIO and a driver supporting it or not... What does it mean?
> > >
> > > My board has a shared gpio-reset between gmac and switch, so both will resetted if it
> > > is asserted. Currently it is set to the gmac and is aquired exclusive. Adding it to switch results in 2 problems:
> > >
> > > - due to exclusive and already mapped to gmac, switch driver exits as it cannot get the reset-gpio again.
> > > - if i drop the reset from gmac and add to switch, it resets the gmac and this takes too long for switch
> > > to get up. Of course i can increase the wait time after reset,but dropping reset here was the easier way.
> > >
> > > Using reset only on gmac side brings the switch up.
> >
> > I think the issue is more for the description itself.
> >
> > Devicetree is only meant to describe the hardware and does in general don't
> > care how any firmware (Linux-kernel, *BSD, etc) handles it. So going with
> > "the kernel does it this way" is not a valid reason for a binding change ;-) .
> >
> > Instead in general want to reason that there are boards without this reset
> > facility and thus make it optional for those.
>
> if only the wording is the problem i try to rephrase it from hardware PoV.
>
> maybe something like this?
>
> https://github.com/frank-w/BPI-R2-4.14/commits/5.18-mt7531-mainline2/Documentation/devicetree/bindings/net/dsa/mediatek%2Cmt7530.yaml
>
> Another way is maybe increasing the delay after the reset (to give more time all
> come up again), but imho it is no good idea resetting the gmac/mdio-bus from the
> child device.
>
> have not looked into the gmac driver if this always  does the initial reset to
> have a "clean state". In this initial reset the switch will be resetted too
> and does not need an additional one which needs the gmac/mdio initialization
> to be done again.

For clarification, the reset gpio line is purely to reset the phy.
If having the switch driver own the reset gpio instead of the gmac
breaks initialization that means there's a bug in the gmac driver
handling phy init.
In testing I've seen issues moving the reset line to the mdio node
with other phys and the stmmac gmac driver, so I do believe this is
the case.

>
> > > >> allow port 5 as
> > > >> cpu-port
> > > >
> > > >How do you allow it here?
> > >
> > > Argh, seems i accidentally removed this part and have not recognized while checking :(
> > >
> > > It should only change description of reg for ports to:
> > >
> > > "Port address described must be 5 or 6 for CPU port and from 0 to 5 for user ports."
>
> noticed that the target-phase is not removed but squashed in the first bindings-patch.
> This was a rebasing error and not intented...will fix in next version.
>
> regards Frank
Krzysztof Kozlowski May 9, 2022, 6:48 a.m. UTC | #6
On 08/05/2022 19:04, Peter Geis wrote:
> On Sun, May 8, 2022 at 8:12 AM Frank Wunderlich <frank-w@public-files.de> wrote:
>>>
>>> I think the issue is more for the description itself.
>>>
>>> Devicetree is only meant to describe the hardware and does in general don't
>>> care how any firmware (Linux-kernel, *BSD, etc) handles it. So going with
>>> "the kernel does it this way" is not a valid reason for a binding change ;-) .

Exactly. The argument in commit msg was not matching the change, because
driver implementation should not be (mostly) a reason for such changes.

>>>
>>> Instead in general want to reason that there are boards without this reset
>>> facility and thus make it optional for those.
>>
>> if only the wording is the problem i try to rephrase it from hardware PoV.
>>
>> maybe something like this?
>>
>> https://github.com/frank-w/BPI-R2-4.14/commits/5.18-mt7531-mainline2/Documentation/devicetree/bindings/net/dsa/mediatek%2Cmt7530.yaml

Looks ok.

>>
>> Another way is maybe increasing the delay after the reset (to give more time all
>> come up again), but imho it is no good idea resetting the gmac/mdio-bus from the
>> child device.
>>
>> have not looked into the gmac driver if this always  does the initial reset to
>> have a "clean state". In this initial reset the switch will be resetted too
>> and does not need an additional one which needs the gmac/mdio initialization
>> to be done again.
> 
> For clarification, the reset gpio line is purely to reset the phy.
> If having the switch driver own the reset gpio instead of the gmac
> breaks initialization that means there's a bug in the gmac driver
> handling phy init.
> In testing I've seen issues moving the reset line to the mdio node
> with other phys and the stmmac gmac driver, so I do believe this is
> the case.

Yes, this seems reasonable, although Frank mentioned that reset is
shared with gmac, so it resets some part of it as well?




Best regards,
Krzysztof
Peter Geis May 9, 2022, 11:22 a.m. UTC | #7
On Mon, May 9, 2022 at 2:48 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 08/05/2022 19:04, Peter Geis wrote:
> > On Sun, May 8, 2022 at 8:12 AM Frank Wunderlich <frank-w@public-files.de> wrote:
> >>>
> >>> I think the issue is more for the description itself.
> >>>
> >>> Devicetree is only meant to describe the hardware and does in general don't
> >>> care how any firmware (Linux-kernel, *BSD, etc) handles it. So going with
> >>> "the kernel does it this way" is not a valid reason for a binding change ;-) .
>
> Exactly. The argument in commit msg was not matching the change, because
> driver implementation should not be (mostly) a reason for such changes.
>
> >>>
> >>> Instead in general want to reason that there are boards without this reset
> >>> facility and thus make it optional for those.
> >>
> >> if only the wording is the problem i try to rephrase it from hardware PoV.
> >>
> >> maybe something like this?
> >>
> >> https://github.com/frank-w/BPI-R2-4.14/commits/5.18-mt7531-mainline2/Documentation/devicetree/bindings/net/dsa/mediatek%2Cmt7530.yaml
>
> Looks ok.
>
> >>
> >> Another way is maybe increasing the delay after the reset (to give more time all
> >> come up again), but imho it is no good idea resetting the gmac/mdio-bus from the
> >> child device.
> >>
> >> have not looked into the gmac driver if this always  does the initial reset to
> >> have a "clean state". In this initial reset the switch will be resetted too
> >> and does not need an additional one which needs the gmac/mdio initialization
> >> to be done again.
> >
> > For clarification, the reset gpio line is purely to reset the phy.
> > If having the switch driver own the reset gpio instead of the gmac
> > breaks initialization that means there's a bug in the gmac driver
> > handling phy init.
> > In testing I've seen issues moving the reset line to the mdio node
> > with other phys and the stmmac gmac driver, so I do believe this is
> > the case.
>
> Yes, this seems reasonable, although Frank mentioned that reset is
> shared with gmac, so it resets some part of it as well?

No, the gpio-reset line is purely to reset the phy. The stmmac gmac
driver handles it because it seems initialization failures occur if
it's handled by the mdio drivers. I suspect this is due to a
difference between when the driver initializes the phy vs when the
driver triggers the reset.
They had tried to attach the gpio binding to both the gmac node and
the mdio node at the same time when only one can own it. Having it
owned by the switch driver on the mdio node leads to the same
initialization failures we see in other mdio drivers.

>
>
>
>
> Best regards,
> Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
index a7696d1b4a8c..d02faed41b2a 100644
--- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml
@@ -55,6 +55,7 @@  description: |
     On mt7531:
       - "1000base-x"
       - "2500base-x"
+      - "rgmii"
       - "sgmii"
 
 
@@ -159,9 +160,6 @@  allOf:
       required:
         - resets
         - reset-names
-    else:
-      required:
-        - reset-gpios
 
   - if:
       required: