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

[Dimitri Fedrau <dima.fedrau@gmail.com>]

Am Mon, Dec 18, 2023 at 12:19:32PM +0100 schrieb 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 could add the init sequence for the 88Q2221 PHY. Then you could test
it on your side. Would this be helpful to you ? Did you already have the
chance to test the patch ?

> > > 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.
> 
You are right, but I would propose to stick to the reference init
sequence and make sure the PHYs works with our code and then work on
optimizing the code. We still can remove and/or document parts of it.

> > > 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.
> 
Are you trying with the patch I provided or your own code ? If you use
my patch you should wait until V3, because I found some problems with
it. Switching from 1000Mbit/s to 100Mbit/s in autonegotiation mode doesn't
work. I could fix it but the fix touches some code already upstreamed. So
I tried to push parts of it yesterday. I forgot to cc you, just used the
get_maintainer script. I will add you to the cc list. Until then you can
look it up here: 20231218221814.69304-2-dima.fedrau@gmail.com

> Regards,
> Stefan

Regards,
Dimitri