Date:   Thu, 24 Nov 2016 10:55:14 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Sebastian Frias <sf84@...oste.net>
Cc:     linux-amlogic@...ts.infradead.org, devicetree@...r.kernel.org,
        netdev@...r.kernel.org, davem@...emloft.net, khilman@...libre.com,
        mark.rutland@....com, robh+dt@...nel.org,
        linux-arm-kernel@...ts.infradead.org, alexandre.torgue@...com,
        peppe.cavallaro@...com, carlo@...one.org,
        Mans Rullgard <mans@...sr.com>, Andrew Lunn <andrew@...n.ch>
Subject: Re: [net-next PATCH v1 0/2] stmmac: dwmac-meson8b: configurable RGMII
 TX delay

Le 24/11/2016 à 09:05, Martin Blumenstingl a écrit :
> On Thu, Nov 24, 2016 at 4:56 PM, Jerome Brunet <jbrunet@...libre.com> wrote:
>> On Thu, 2016-11-24 at 15:34 +0100, Martin Blumenstingl wrote:
>>> Currently the dwmac-meson8b stmmac glue driver uses a hardcoded 1/4
>>> cycle TX clock delay. This seems to work fine for many boards (for
>>> example Odroid-C2 or Amlogic's reference boards) but there are some
>>> others where TX traffic is simply broken.
>>> There are probably multiple reasons why it's working on some boards
>>> while it's broken on others:
>>> - some of Amlogic's reference boards are using a Micrel PHY
>>> - hardware circuit design
>>> - maybe more...
>>>
>>> This raises a question though:
>>> Which device is supposed to enable the TX delay when both MAC and PHY
>>> support it? And should we implement it for each PHY / MAC separately
>>> or should we think about a more generic solution (currently it's not
>>> possible to disable the TX delay generated by the RTL8211F PHY via
>>> devicetree when using phy-mode "rgmii")?
>>
>> Actually you can skip the part which activate the Tx-delay on the phy
>> by setting "phy-mode = "rgmii-id" instead of "rgmii"
>>
>> phy->interface will no longer be PHY_INTERFACE_MODE_RGMII
>> but PHY_INTERFACE_MODE_RGMII_ID.
> unfortunately this is not true for RTL8211F (I did my previous tests
> with the same expectation in mind)!
> the code seems to suggest that TX-delay is disabled whenever mode !=
> PHY_INTERFACE_MODE_RGMII.
> BUT: on my device RTL8211F_TX_DELAY is set even before
> "phy_write(phydev, 0x11, reg);"!

(Adding Sebastian (and Mans, and Andrew) since he raised the same
question a while ago. I think I now understand a bit better what
Sebastian was after a couple of weeks ago)

> 
> Based on what I found it seems that rgmii-id, rgmii-txid and
> rgmii-rxid are supposed to be handled by the PHY.

Correct, the meaning of PHY_INTERFACE_MODE should be from the
perspective of the PHY device:

- PHY_INTERFACE_MODE_RGMII_TXID means that the PHY is responsible for
adding a delay when the MAC transmits (TX MAC -> PHY (delay) -> wire)
- PHY_INTERFACE_MODE_RGMII_RXID means that the PHY is responsible for
adding a delay when the MAC receives (RX MAC <- (delay) PHY) <- wire)

> That would mean that we have two problems here:
> 1) drivers/net/phy/realtek.c:rtl8211f_config_init should check for
> PHY_INTERFACE_MODE_RGMII_ID or PHY_INTERFACE_MODE_RGMII_TXID and
> enable the TX-delay in that case - otherwise explicitly disable it

Agreed.

> 2) dwmac-meson8b.c should only use the configured TX-delay for
> PHY_INTERFACE_MODE_RGMII
> @Florian: could you please share your thoughts on this (who handles
> the TX delay in which case)?

This also seems reasonable to do, provided that the PHY is also properly
configured not to add delays in both directions, and therefore assumes
that the MAC does it.

We have a fairly large problem with how RGMII delays are done in PHYLIB
and Ethernet MAC drivers (or just in general), where we can't really
intersect properly what a PHY is supporting (in terms of internal
delays), and what the MAC supports either. One possible approach could
be to update PHY drivers a list of PHY_INTERFACE_MODE_* that they
support (ideally, even with normalized nanosecond delay values), and
then intersect that with the requested phy_interface_t during
phy_{attach,connect} time, and feed this back to the MAC with a special
error code/callback, so we could gracefully try to choose another
PHY_INTERFACE_MODE_* value that the MAC supports....

A larger problem is that a number of drivers have been deployed, and
Device Trees, possibly with the meaning of "phy-mode" and
"phy-connection-type" being from the MAC perspective, and not the PHY
perspective *sigh*, good luck auditing those.

So from there, here is possibly what we could do

- submit a series of patches that update the PHYLIB documentation (there
are other things missing here) and make it clear from which entity (PHY
or MAC) does the delay apply to, document the "intersection" problem here

- have you document the configured behavior for dwmac-meson8b that we
just discussed here in v2 of this patch series

Sorry for the long post, here is a virtual potato: 0
-- 
Florian

Powered by blists - more mailing lists