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:

quoted

Am Sun, Dec 17, 2023 at 02:50:49PM +0100 schrieb Stefan Eichenberger:

quoted

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 ?

quoted

quoted

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.

quoted

quoted

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