Hi Vladimir,

On Thu, May 2, 2019 at 1:03 AM Vladimir Oltean [off-list ref] wrote:

On Wed, 1 May 2019 at 00:16, Martin Blumenstingl
[off-list ref] wrote:

quoted

 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)

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)

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

Hi Martin, Sergey,

I had another look and it seems that Realtek has designated the same
PHY ID for the RTL8211F and RTL8211FS. However the F is a 40-pin
package and the FS is a 48-pin package. The datasheet for the F
doesn't mention about the MDIO bit for TXDLY being implemented,
whereas for FS it does. That can mean anything, really, but as I only
have access to a board with the FS chip, I can't easily check. Maybe
Martin can confirm that his chip's designation is really not FS.

I just checked two of my boards:
- the PHY on my Hardkernel Odroid-C1+ is marked as "RTL8211F"
- the PHY on my Khadas VIM2 is marked as "RTL8211FDI"

I have not heard of a RTL8211FS chip / package before.

But my point still remains, though. The F and FS share the same PHY
ID, and one supports only RGMII while the other can be configured for
SGMII as well. Good luck being ultra-correct in the phy-mode checking
when you can't distinguish the chip. But in general DT description is
chief and should not be contradicted. Perhaps an argument could be
made for the RGMII delays as they constitute an exception to the "HW
description" rule.

PHY ID being shared across various PHY revisions and packages is also
the case for some ICPlus IP101A / IP101G PHYs, see [0]
The (32-pin QFN) IP101GR uses a register to switch a pin function
between "interrupt" and "receive error".
There's no rule in the driver to enforce that this only applies to
IP101GR PHYs, it's only described in the documentation (I don't even
know if we can enforce it in software because I'm not sure we can
detect the different variants of the IP101A / IP101G PHYs)


Martin


[0] https://github.com/torvalds/linux/blob/fdc13a9effd5359ae00705708c8c846b1cb2b69c/Documentation/devicetree/bindings/net/icplus-ip101ag.txt