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 [off-list ref] wrote:
[...]

quoted

quoted

quoted

quoted

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.

quoted

quoted

quoted

quoted

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.

quoted

quoted

quoted

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.

[...]

quoted

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