lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20231219081117.GA3479@debian> Date: Tue, 19 Dec 2023 09:11:17 +0100 From: Dimitri Fedrau <dima.fedrau@...il.com> To: Stefan Eichenberger <eichest@...il.com> Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH] net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY 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@...il.com > Regards, > Stefan Regards, Dimitri
Powered by blists - more mailing lists