Re: [PATCH v2 2/2] net: phy: realtek: Change TX-delay setting for RGMII modes only

[Martin Blumenstingl]

 Hello Serge,

On Mon, Apr 29, 2019 at 11:12 PM Serge Semin <fancer.lancer@xxxxxxxxx> 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)

> > > > 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)

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