Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Ard Biesheuvel <ardb@kernel.org>
To: Andrew Lunn <andrew@lunn.ch>
Cc: "Arnd Bergmann" <arnd@kernel.org>,
	"Jernej Škrabec" <jernej.skrabec@gmail.com>,
	"Daniel Thompson" <daniel.thompson@linaro.org>,
	"Sumit Garg" <sumit.garg@linaro.org>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Masami Hiramatsu" <mhiramat@kernel.org>,
	"Steve McIntyre" <steve@einval.com>,
	"open list:BPF JIT for MIPS (32-BIT AND 64-BIT)"
	<netdev@vger.kernel.org>, "Willy Liu" <willy.liu@realtek.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Sasha Levin" <sashal@kernel.org>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	"Masahisa Kojima" <masahisa.kojima@linaro.org>,
	"Ilias Apalodimas" <ilias.apalodimas@linaro.org>
Subject: Re: Re: realtek PHY commit bbc4d71d63549 causes regression
Date: Fri, 13 Nov 2020 23:49:39 +0100
Message-ID: <CAMj1kXGnfsX1pH8m1eO-B1nAqL=vMeuw6fpYdeA1RqMpSrg66Q@mail.gmail.com> (raw)
In-Reply-To: <20201113224301.GU1480543@lunn.ch>

On Fri, 13 Nov 2020 at 23:43, Andrew Lunn <andrew@lunn.ch> wrote:
>
> Hi Arnd
>
> > Something of that sort. I also see a similar patch in KSZ9031
> > now, see 7dd8f0ba88fc ("arm: dts: imx6qdl-udoo: fix rgmii phy-mode
> > for ksz9031 phy")
> >
> > As this exact mismatch between rgmii and rgmii-id mode is apparently
> > a more widespread problem, the best workaround I can think of
> > is that we redefine the phy-mode="rgmii" property to actually mean
> > "use rgmii mode and let the phy driver decide the delay configuration",
>
> The problem is, the PHY driver has no idea what the delay
> configuration should be. That is the whole point of the DT property.
>
> The MAC and the PHY have to work together to ensure one of them
> inserts the delay. In most cases, the MAC driver reads the property
> and passes it unmodified to the PHY. The PHY then does what it is
> told. In some cases, the MAC decides to add the delay, it changes the
> rgmii-id to rgmii before passing it onto the PHY. The PHY does as it
> is told, and it works. And a very small number of boards simply have
> longer clock lines than signal lines, so the PCB adds the delay. It is
> not clearly defined how that should be described in DT, but it works
> so far because most MAC drivers don't add delays, pass the 'rgmii'
> from DT to the PHY and it does as it is told and does not add delays.
>
> There is one more case, which is not used very often. The PHY is
> passed the NA values, which means, don't touch, something else has set
> it up. So when the straps are doing the correct thing, you could pass
> NA. However, some MAC drivers look at the phy mode, see it is one of
> the 4 rgmii modes, and configure their end to rgmii, instead of gmii,
> mii, sgmii, etc. How networking does ACPI is still very undefined, but
> i think we need to push for ACPI to pass NA, and the firmware does all
> the setup. That seems to be ACPI way.
>
> > with a new string to mean specifically rgmii mode with no delay.
>
> As you said, we have phy-mode="rgmii" 235 times. How many of those are
> going to break when you change the definition of rgmii?  I have no
> idea, but my gut feeling is more than the number of boards which are
> currently broken because of the problem with this PHY.
>
> And, as i said above, some MAC drivers look for one of the 4 RGMII
> modes in order to configure their side. If you add a new string, you
> need to review all the MAC drivers and make sure they check for all 5
> strings, not 4. Some of that is easy, modify
> phy_interface_mode_is_rgmii(), but not all MAC use it, and it is no
> help in a switch statement.
>
> And we are potentially going to get into the same problem
> again. History has shown, we cannot get 4 properties right. Do you
> think we will do any better getting 5 properties right? Especially
> when phy-mode="rgmii" does not mean rgmii, but do whatever you think
> might be correct?
>
> Having suffered the pain from the Atheros PHY, this is something i
> review much more closely, so hopefully we are getting better at
> this. But PHY drivers live for a long time, ksz9031 was added 7 years
> ago, well before we started looking closely at delays. I expect more
> similar problems will keep being found over the next decade.
>
> To some extent, we actually need DT writers to properly test their
> DT. If both rgmii and rgmii-id works, there is a 50% chance whatever
> they pick is wrong. And it would be nice if they told the networking
> people so we can fix the PHY.
>

One question that still has not been answered is how many actual
platforms were fixed by backporting Realtek's follow up fix to
-stable. My suspicion is none. That by itself should be enough
justification to revert the backport of that change.

I do agree that we should fix this properly going forward, and if we
do manage to fix this in a backwards compatible way, we should
backport that fix. But letting the current situation exist because
nobody can be bothered to fix it properly is not the right solution
IMHO.

  reply index

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-17 14:20 Ard Biesheuvel
2020-10-17 14:44 ` Andrew Lunn
2020-10-17 14:46   ` Ard Biesheuvel
2020-10-17 15:11     ` Andrew Lunn
2020-10-17 15:18       ` Ard Biesheuvel
2020-10-17 16:14         ` Ilias Apalodimas
2020-10-17 16:17           ` Ard Biesheuvel
2020-10-17 16:21             ` Ilias Apalodimas
2020-10-17 18:04             ` Andrew Lunn
2020-10-17 18:11               ` Ard Biesheuvel
2020-10-17 18:17                 ` Ard Biesheuvel
2020-10-17 18:27                 ` Andrew Lunn
2020-10-17 18:55                   ` Ard Biesheuvel
2020-10-17 19:49                     ` Andrew Lunn
2020-10-17 22:19                       ` Ard Biesheuvel
2020-10-17 23:02                         ` Andrew Lunn
2020-10-18 10:35                           ` Ard Biesheuvel
2020-10-18 10:56                             ` Ard Biesheuvel
2020-10-18 15:45                               ` Andrew Lunn
2020-10-25 14:16                                 ` Ard Biesheuvel
2020-10-25 14:28                                   ` Andrew Lunn
2020-10-25 14:34                                     ` Ard Biesheuvel
2020-10-25 14:42                                       ` Andrew Lunn
2020-10-29 14:21                                         ` Ilias Apalodimas
2020-10-29 14:39                                           ` Andrew Lunn
2020-10-29 14:42                                             ` Ard Biesheuvel
2020-10-29 14:50                                               ` Andrew Lunn
2020-10-29 14:46                                             ` Ilias Apalodimas
2020-11-05 17:31                                               ` Jernej Škrabec
2020-11-13 13:50                                                 ` Arnd Bergmann
2020-11-13 14:44                                                   ` Andrew Lunn
2020-11-13 15:33                                                     ` Arnd Bergmann
2020-11-13 16:56                                                       ` Andrew Lunn
2020-11-13 21:26                                                         ` Arnd Bergmann
2020-11-13 22:43                                                           ` Andrew Lunn
2020-11-13 22:49                                                             ` Ard Biesheuvel [this message]
2020-11-14  0:40                                                               ` Andrew Lunn
2020-11-14 10:09                                                                 ` Ard Biesheuvel
2020-10-18 15:38                             ` Andrew Lunn
2020-10-18 14:20                         ` Masami Hiramatsu
2020-10-17 18:01           ` Andrew Lunn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMj1kXGnfsX1pH8m1eO-B1nAqL=vMeuw6fpYdeA1RqMpSrg66Q@mail.gmail.com' \
    --to=ardb@kernel.org \
    --cc=alex.bennee@linaro.org \
    --cc=andrew@lunn.ch \
    --cc=arnd@kernel.org \
    --cc=daniel.thompson@linaro.org \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=ilias.apalodimas@linaro.org \
    --cc=jernej.skrabec@gmail.com \
    --cc=kuba@kernel.org \
    --cc=masahisa.kojima@linaro.org \
    --cc=mhiramat@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=sashal@kernel.org \
    --cc=steve@einval.com \
    --cc=sumit.garg@linaro.org \
    --cc=willy.liu@realtek.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git