linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Serge Semin <fancer.lancer@gmail.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: Vladimir Oltean <olteanv@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Serge Semin <Sergey.Semin@t-platforms.ru>,
	netdev <netdev@vger.kernel.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] net: phy: realtek: Change TX-delay setting for RGMII modes only
Date: Mon, 6 May 2019 17:39:07 +0300	[thread overview]
Message-ID: <20190506143906.o3tublcxr5ge46rg@mobilestation> (raw)
In-Reply-To: <CAFBinCBxgMr6ZkOSGfXZ9VwJML=GnzrL+FSo5jMpN27L2o5+JA@mail.gmail.com>

Hello Martin.

On Tue, Apr 30, 2019 at 11:16:21PM +0200, Martin Blumenstingl wrote:
>  Hello Serge,
> 
> On Mon, Apr 29, 2019 at 11:12 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> [...]
> > > > > Apparently the current config_init method doesn't support RXID setting.
> > > > > The patch introduced current function code was submitted by
> > > > > Martin Blumenstingl in 2016:
> > > > > https://patchwork.kernel.org/patch/9447581/
> > > > > and was reviewed by Florian. So we'd better ask him why it was ok to mark
> > > > > the RGMII_ID as supported while only TX-delay could be set.
> > > > > I also failed to find anything regarding programmatic rtl8211f delays setting
> > > > > in the Internet. So at this point we can set TX-delay only for f-model of the PHY.
> let me give you a bit of context on that patch:
> most boards (SBCs and TV boxes) with an Amlogic SoC and a Gigabit
> Ethernet PHY use a Realtek RTL8211F PHY. we were seeing high packet
> loss when transmitting from the board to another device.
> it took us very long to understand that a combination of different
> hardware and driver pieces lead to this issue:
> - in the MAC driver we enabled a 2ns TX delay by default, like Amlogic
> does it in their vendor (BSP) kernel
> - we used the upstream Realtek RTL8211F PHY driver which only enabled
> the TX delay if requested (it never disabled the TX delay)
> - hardware defaults or pin strapping of the Realtek RTL8211F PHY
> enabled the TX delay in the PHY
> 
> This means that the TX delay was applied twice: once at the MAC and
> once at the PHY.
> That lead to high packet loss when transmitting data.
> To solve that I wrote the patch you mentioned, which has since been
> ported over to u-boot (for a non-Amlogic related board)
> 

Yeah. This is a standard problem if you ever worked with a hardware just
designed, when you try to make MAC+PHY working together. If you experienced
packets loss and it's RGMII, then most likely the problem with delays.

> > > > > Anyway lets clarify the situation before to proceed further. You are suggesting
> > > > > to return an error in case if either RGMII_ID or RGMII_RXID interface mode is
> > > > > requested to be enabled for the PHY. It's fair seeing the driver can't fully
> > > > > support either of them.
> I don't have any datasheet for the Realtek RTL8211F PHY and I'm not in
> the position to get one (company contracts seem to be required for
> this).
> Linux is not my main job, I do driver development in my spare time.
> 
> there may or may not be a register or pin strapping to configure the RX delay.
> due to this I decided to leave the RX delay behavior "not defined"
> instead of rejecting RGMII_RXID and RGMII_ID.
> 
> > > > That is how I read Andrew's suggestion and it is reasonable. WRT to the
> > > > original changes from Martin, he is probably the one you would want to
> > > > add to this conversation in case there are any RX delay control knobs
> > > > available, I certainly don't have the datasheet, and Martin's change
> > > > looks and looked reasonable, seemingly independent of the direction of
> > > > this very conversation we are having.
> the changes in patch 1 are looking good to me (except that I would use
> phy_modify_paged instead of open-coding it, functionally it's
> identical with what you have already)
> 

Nah, this isn't going to work since the config register is placed on an extension
page. So in order to reach the register first I needed to enable a standard page,
then select an extended page, then modify the register bits.

> I'm not sure about patch 2:
> personally I would wait for someone to come up with the requirement to
> use RGMII_RXID with a RTL8211F PHY.
> that person will then a board to test the changes and (hopefully) a
> datasheet to explain the RX delay situation with that PHY.
> that way we only change the RGMII_RXID behavior once (when someone
> requests support for it) instead of twice (now with your change, later
> on when someone needs RGMII_RXID support in the RTL8211F driver)
> 
> that said, the change in patch 2 itself looks fine on Amlogic boards
> (because all upstream .dts let the MAC generate the TX delay). I
> haven't runtime-tested your patch there yet.
> but there seem to be other boards (than the Amlogic ones, the RTL8211F
> PHY driver discussion in u-boot was not related to an Amlogic board)
> out there with a RTL8211F PHY (these may or may not be supported in
> mainline Linux or u-boot and may or may not use RGMII_RXID where you
> are now changing the behavior). that's not a problem by itself, but
> you should be aware of this.
> 
> [...]
> > rtl8211(e|f) TX/RX delays can be configured either by external pins
> > strapping or via software registers. This is one of the clue to provide
> > a proper config_init method code. But not all rtl8211f phys provide
> > that software register, and if they do it only concerns TX-delay (as we
> > aware of). So we need to take this into account when creating the updated
> > versions of these functions.
> >
> > (Martin, I also Cc'ed you in this discussion, so if you have anything to
> > say in this matter, please don't hesitate to comment.)
> Amlogic boards, such as the Hardkernel Odroid-C1 and Odroid-C2 as well
> as the Khadas VIM2 use a "RTL8211F" RGMII PHY. I don't know whether
> there are multiple versions of this PHY. all RTL8211F I have seen so
> far did behave exactly the same.
> 
> I also don't know whether the RX delay is configurable (by pin
> strapping or some register) on RTL8211F PHYs because I don't have
> access to the datasheet.
> 
> 
> Martin

Ok. Thanks for the comments. I am sure the RX-delay is configurable at list
via external RXD pin strapping at the chip powering up procedure. The only
problem with a way of software to change the setting.

I don't think there is going to be anyone revealing that realtek black boxed
registers layout anytime soon. So as I see it it's better to leave the
rtl8211f-part as is for now.

-Sergey

  parent reply	other threads:[~2019-05-06 15:03 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-26  9:30 [PATCH] net: phy: realtek: Add rtl8211e rx/tx delays config Serge Semin
2019-04-26 13:28 ` Andrew Lunn
2019-04-26 19:19   ` Serge Semin
2019-04-26 20:05     ` Andrew Lunn
2019-04-26 20:28       ` Serge Semin
2019-04-26 17:17 ` Heiner Kallweit
2019-04-26 20:26   ` Serge Semin
2019-04-26 21:21 ` [PATCH v2 1/2] " Serge Semin
2019-04-26 21:40   ` Andrew Lunn
2019-04-26 23:45     ` Serge Semin
2019-04-27  3:11       ` Florian Fainelli
2019-04-27  7:44         ` Serge Semin
2019-04-27 15:21           ` Andrew Lunn
2019-04-28 19:19             ` Serge Semin
2019-04-27 19:20           ` Florian Fainelli
2019-05-08  1:29   ` [PATCH v3 0/2] net: phy: realtek: Fix RGMII TX/RX-delays initial config of rtl8211(e|f) Serge Semin
2019-05-08  1:29     ` [PATCH v3 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config Serge Semin
2019-05-08  1:29     ` [PATCH v3 2/2] net: phy: realtek: Change TX-delay setting for RGMII modes only Serge Semin
2019-05-08 16:37     ` [PATCH v3 0/2] net: phy: realtek: Fix RGMII TX/RX-delays initial config of rtl8211(e|f) David Miller
2019-05-08 21:51     ` [PATCH v4 " Serge Semin
2019-05-08 21:51       ` [PATCH v4 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config Serge Semin
2019-05-08 21:51       ` [PATCH v4 2/2] net: phy: realtek: Change TX-delay setting for RGMII modes only Serge Semin
2019-05-08 23:31       ` [PATCH v4 0/2] net: phy: realtek: Fix RGMII TX/RX-delays initial config of rtl8211(e|f) David Miller
2019-05-13  5:41   ` [PATCH v2 1/2] net: phy: realtek: Add rtl8211e rx/tx delays config Guenter Roeck
2019-05-13 10:37     ` Serge Semin
2019-04-26 21:21 ` [PATCH v2 2/2] net: phy: realtek: Change TX-delay setting for RGMII modes only Serge Semin
2019-04-26 21:46   ` Andrew Lunn
2019-04-26 23:35     ` Serge Semin
2019-04-29 17:37       ` Florian Fainelli
2019-04-29 18:29         ` Vladimir Oltean
2019-04-29 21:12           ` Serge Semin
2019-04-29 22:36             ` Vladimir Oltean
2019-04-30 12:54               ` Serge Semin
2019-04-30 20:44               ` Martin Blumenstingl
2019-05-08  0:48                 ` Serge Semin
2019-04-30 21:16             ` Martin Blumenstingl
2019-05-01 23:03               ` Vladimir Oltean
2019-05-03 17:29                 ` Martin Blumenstingl
2019-05-06 14:39               ` Serge Semin [this message]
2019-05-06 17:21                 ` Martin Blumenstingl
2019-05-07 17:37                   ` Heiner Kallweit
2019-05-07 20:09                     ` Martin Blumenstingl

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=20190506143906.o3tublcxr5ge46rg@mobilestation \
    --to=fancer.lancer@gmail.com \
    --cc=Sergey.Semin@t-platforms.ru \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=martin.blumenstingl@googlemail.com \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).