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

[Serge Semin]

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

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