Re: [PATCH] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY

[Stefan Eichenberger]

Hi Dimitri,

On Mon, Dec 18, 2023 at 10:09:32AM +0100, Dimitri Fedrau wrote:
> Am Sun, Dec 17, 2023 at 02:50:49PM +0100 schrieb Stefan Eichenberger:
> > I also tried to make the 88Q2221 work but didn't find the time yet to
> > write a clean version yet. My last minimal patch looks as attached
> > bellow.
> > 
> I probably will also get a 88Q2221 PHY, but it could take some time.
> When looking at the reference code the only difference for the 88Q2220
> and 88Q2221 seems to be an additional init sequence with 28 register writes.
> Remaining code seems to be identical. Am I right ? If yes we can use the
> same code base here. Besides that it seems that both PHYs share the same
> PHY id and are only distinguished by the "Secondary ID Register".

I think the init sequence is the same for both PHYs. At least they share
the same reference manual and the API User Guide.

> > I think the main thing to make the PHY work is to call this
> > sequence to set the master/slave detection threshold:
> > 
> > /* Set detection threshold slave master */
> > phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020);
> > phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0a28);
> > phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0x0c28);
> > 
> > Without this sequence the PHY does not work. I was also wondering as
> > Andrew wrote why we write twice to the same register. My assumption is
> > that 0x8032 is some kind of selector for a subregister while 0x8031 will
> > set a 32 bit value. Unforunately, I also didn't get that information
> > from Marvell and it is just a wild guess. Please also note that calling
> > the sequence in the probe function (as I do it in the example below) is
> > definitely wrong, it was just a quick and dirty test I did because I
> > wanted to know if it is enough to call it only once.
> > 
> You are maybe right about your guess. Without the init sequence at all I
> was able to get the PHY work with a fixed setting (100Mbit/Master).
> Maybe it was due to bootstrapping the pins of the PHY. But still I'm not
> getting the point. What are we going to do ? Do we want to strip down or
> generalize the init sequence ? There is probably a reason for such an
> annoying large undocumented series of register writes.

The documentation is really annoying, I agree. I would propose to try to
keep the driver as minimal as possible. If we see that something is not
working, we can still add it later on. Maybe this helps to get a better
understanding of what the registers do. Further, they always do a full
initialization when they switch e.g. from 100MBit/s to 1GBit/s. This
definitely seems to be unnecessary.

> > Are you able to test everyting with the upstream kernel? I'm asking
> > because I have to backport a lot of stuff to a downstream kernel 5.15
> > from NXP to test the 88Q2221. 
> > 
> I'm testing with the upstream kernel, no problems so far. Didn't have to
> backport anything or test on another kernel. First version of my driver
> was also on NXPs 5.15 kernel. Gladly you already upstreamed some T1
> specific code which helped me a lot to reduce code size.

That's good to hear. The 88Q2221 in my setup is connected to an S32G274A
from NXP and unfortunately that one doesn't have proper upstream support
yet.

> > Further, are you able to verify that autonegotion works? Somehow for me
> > this never really worked even when using the example sequence from
> > Marvell.
> > 
> 
> Autonegotiation works fine, didn't have any problems. I'm using the
> 88Q2220M rev B0. I test it with a Media Converter, the NETLion1000 C2T and
> with another 88Q2220M PHY. What do you use for testing ?

I have to try it again. I'm using the Goepel Media Converter (EasyCON)
and I'm pretty sure autoneg works on the Media Converter but somehow not
on the PHY side. It could be that this is because of one of this
undocumented registers.

Regards,
Stefan