Re: [PATCH] net: phy: at803x: disable delay only for RGMII mode

[Niklas Cassel <niklas.cassel@linaro.org>]

On Thu, Feb 14, 2019 at 12:49:36PM +0200, Peter Ujfalusi wrote:
> Hi Niklas,
> 
> On 13/02/2019 19.40, Niklas Cassel wrote:
> > On Wed, Feb 13, 2019 at 02:40:18PM +0100, Marc Gonzalez wrote:
> >> On 13/02/2019 14:29, Andrew Lunn wrote:
> >>
> >>>> So we have these modes:
> >>>>
> >>>> PHY_INTERFACE_MODE_RGMII: TX and RX delays disabled
> >>>> PHY_INTERFACE_MODE_RGMII_ID: TX and RX delays enabled
> >>>> PHY_INTERFACE_MODE_RGMII_RXID: RX delay enabled, TX delay disabled
> >>>> PHY_INTERFACE_MODE_RGMII_TXID: TX delay enabled, RX delay disabled
> >>>>
> >>>> What I don't like with this patch, is that if we specify phy-mode
> >>>> PHY_INTERFACE_MODE_RGMII_TXID, this patch will enable TX delay,
> >>>> but RX delay will not be explicitly set.
> >>>
> >>> That is not the behaviour we want. It is best to assume the device is
> >>> in a random state, and correctly enable/disable all delays as
> >>> requested. Only leave the hardware alone if PHY_INTERFACE_MODE_NA is
> >>> used.
> >>
> >> That's what my patch did:
> >> https://www.spinics.net/lists/netdev/msg445053.html
> >>
> >> But see Florian's remarks:
> >> https://www.spinics.net/lists/netdev/msg445133.html
> > 
> > Hello Marc,
> > 
> > I saw that comment from Florian. However that was way back in 2017.
> > Maybe the phy-modes were not as well defined back then?
> > 
> > Andrew recently suggested to fix the driver so that it conforms with the
> > phy-modes, and fix any SoC that specified an incorrect phy-mode in DT
> > and thus relied upon the broken behavior of the PHY driver:
> > https://www.spinics.net/lists/netdev/msg445133.html
> > 
> > 
> > So, I've rebased your old patch, see attachment.
> > I suggest that Peter test it on am335x-evm.
> 
> with the patch + s/rgmii-txid/rgmii-id in the am335x-evmsk.dts ethernet
> is working.
> I don't have am335x-evm to test, but it has the same PHY as evmsk.
> 

Florian's concern was that this PHY driver looked at "phy-mode" from the
perspective of the MAC rather than the PHY.
However, if s/rgmii-txid/rgmii-id is the correct fix for am335x-evm,
then this means that this PHY driver was just broken.

If the driver had misinterpreted the perspective, then the correct
fix for am335x-evm would have been s/rgmii-txid/rgmii-rxid.

So considering that this driver seems to be really broken
(rather then just inverted perspective),
perhaps we can merge the patch I attached in my previous email after all?
(Together with a s/rgmii-txid/rgmii-id in the am335x-evmsk.dts.)


Kind regards,
Niklas

> > am335x-evm appears to rely on the current broken behavior of the PHY
> > driver, so we will probably need to fix the am335x-evm according to this:
> > https://www.spinics.net/lists/netdev/msg445117.html
> > and merge that as well.
> > 
> > 
> > Andrew, Florian, do you both agree?
> > 
> > 
> > Kind regards,
> > Niklas
> > 
> 
> - Péter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki