| Message ID | 20231215213102.35994-1-dima.fedrau@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: marvell-88q2xxx: add driver for the Marvell 88Q2220 PHY | expand |
> +static int mv88q222x_config_aneg_gbit(struct phy_device *phydev) > +{ > + int ret; > + > + /* send_s detection threshold, slave and master */ > + ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020); > + if (ret < 0) > + return ret; > + > + ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xa28); > + if (ret < 0) > + return ret; > + > + ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xc28); > + if (ret < 0) > + return ret; Same register with two different values? There are a lot of magic values here. Does the datasheet names these registers? Does it define the bits? Adding #defines would be good. > +static int mv88q222x_config_aneg_preinit(struct phy_device *phydev) > +{ > + int ret, val, i; > + > + /* Enable txdac */ > + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8033, 0x6801); > + if (ret < 0) > + return ret; > + > + /* Disable ANEG */ > + ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_CTRL, 0x0); > + if (ret < 0) > + return ret; > + > + /* Set IEEE power down */ > + ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x840); 0x800 is MDIO_CTRL1_LPOWER. What is the other? It seems like a speed selection bit? Andrew
Am Sat, Dec 16, 2023 at 05:46:32PM +0100 schrieb Andrew Lunn: > > +static int mv88q222x_config_aneg_gbit(struct phy_device *phydev) > > +{ > > + int ret; > > + > > + /* send_s detection threshold, slave and master */ > > + ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020); > > + if (ret < 0) > > + return ret; > > + > > + ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xa28); > > + if (ret < 0) > > + return ret; > > + > > + ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xc28); > > + if (ret < 0) > > + return ret; > > Same register with two different values? > Just copied the init sequence from sample code provided by Marvell. I don't know if its a mistake. There is no documentation on this. > There are a lot of magic values here. Does the datasheet names these > registers? Does it define the bits? Adding #defines would be good. > Datasheet is not naming them. I once asked Marvell Support for documentation on the init sequence and what purpose each register has. Just got the answer to use the sample code as it is. > > +static int mv88q222x_config_aneg_preinit(struct phy_device *phydev) > > +{ > > + int ret, val, i; > > + > > + /* Enable txdac */ > > + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8033, 0x6801); > > + if (ret < 0) > > + return ret; > > + > > + /* Disable ANEG */ > > + ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_CTRL, 0x0); > > + if (ret < 0) > > + return ret; > > + > > + /* Set IEEE power down */ > > + ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x840); > > 0x800 is MDIO_CTRL1_LPOWER. What is the other? It seems like a speed > selection bit? > The other is MDIO_PMA_CTRL1_SPEED1000. Will fix this in V2. > Andrew
> > > + /* Set IEEE power down */ > > > + ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x840); > > > > 0x800 is MDIO_CTRL1_LPOWER. What is the other? It seems like a speed > > selection bit? > > > The other is MDIO_PMA_CTRL1_SPEED1000. Will fix this in V2. It seems odd to set a speed, and power it down. But i guess you have blindly copied the reference code, so have no idea why? Andrew
Am Sun, Dec 17, 2023 at 10:22:54AM +0100 schrieb Andrew Lunn: > > > > + /* Set IEEE power down */ > > > > + ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x840); > > > > > > 0x800 is MDIO_CTRL1_LPOWER. What is the other? It seems like a speed > > > selection bit? > > > > > The other is MDIO_PMA_CTRL1_SPEED1000. Will fix this in V2. > > It seems odd to set a speed, and power it down. But i guess you have > blindly copied the reference code, so have no idea why? > I agree, absolutely no idea. I already asked the Marvell support for any document describing the init sequence, but they couldn't help me. So I have to stick to the reference code. At least I copied the comments that were part of the init sequence, trying to give some meaning to it. > Andrew Dimitri
Hi Dimitri, On Sun, Dec 17, 2023 at 12:15:38PM +0100, Dimitri Fedrau wrote: > Am Sun, Dec 17, 2023 at 10:22:54AM +0100 schrieb Andrew Lunn: > > > > > + /* Set IEEE power down */ > > > > > + ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x840); > > > > > > > > 0x800 is MDIO_CTRL1_LPOWER. What is the other? It seems like a speed > > > > selection bit? > > > > > > > The other is MDIO_PMA_CTRL1_SPEED1000. Will fix this in V2. > > > > It seems odd to set a speed, and power it down. But i guess you have > > blindly copied the reference code, so have no idea why? > > > I agree, absolutely no idea. I already asked the Marvell support for > any document describing the init sequence, but they couldn't help me. > So I have to stick to the reference code. At least I copied the comments > that were part of the init sequence, trying to give some meaning to it. 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 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. 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. Further, are you able to verify that autonegotion works? Somehow for me this never really worked even when using the example sequence from Marvell. Best regards, Stefan diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c index 94a8c99b58da..15e82e8ff8f4 100644 --- a/drivers/net/phy/marvell-88q2xxx.c +++ b/drivers/net/phy/marvell-88q2xxx.c @@ -208,17 +214,26 @@ static int mv88q2xxxx_get_sqi(struct phy_device *phydev) ret = ret >> 12; } else { - /* Read from vendor specific registers, they are not documented - * but can be found in the Software Initialization Guide. Only - * revisions >= A0 are supported. - */ - ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, 0xFC5D, 0x00FF, 0x00AC); - if (ret < 0) - return ret; + if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2221) { + /* Read from vendor specific register, they can be + * found in the sample source code of the Q222X API + */ + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfcd9); + if (ret < 0) + return ret; + } else { + /* Read from vendor specific registers, they are not documented + * but can be found in the Software Initialization Guide. Only + * revisions >= A0 are supported. + */ + ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, 0xFC5D, 0x00FF, 0x00AC); + if (ret < 0) + return ret; - ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfc88); - if (ret < 0) - return ret; + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfc88); + if (ret < 0) + return ret; + } } return ret & 0x0F; @@ -229,6 +244,16 @@ static int mv88q2xxxx_get_sqi_max(struct phy_device *phydev) return 15; } +static int mv88q2221_probe(struct phy_device *phydev) +{ + /* 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); + + return 0; +} + static struct phy_driver mv88q2xxx_driver[] = { { .phy_id = MARVELL_PHY_ID_88Q2110, @@ -243,12 +268,27 @@ static struct phy_driver mv88q2xxx_driver[] = { .get_sqi = mv88q2xxxx_get_sqi, .get_sqi_max = mv88q2xxxx_get_sqi_max, }, + { + .phy_id = MARVELL_PHY_ID_88Q2221, + .phy_id_mask = MARVELL_PHY_ID_MASK, + .name = "mv88q2221", + .get_features = mv88q2xxx_get_features, + .config_aneg = mv88q2xxx_config_aneg, + .config_init = mv88q2xxx_config_init, + .read_status = mv88q2xxx_read_status, + .soft_reset = mv88q2xxx_soft_reset, + .set_loopback = genphy_c45_loopback, + .get_sqi = mv88q2xxxx_get_sqi, + .get_sqi_max = mv88q2xxxx_get_sqi_max, + .probe = mv88q2221_probe, + }, }; module_phy_driver(mv88q2xxx_driver); static struct mdio_device_id __maybe_unused mv88q2xxx_tbl[] = { { MARVELL_PHY_ID_88Q2110, MARVELL_PHY_ID_MASK }, + { MARVELL_PHY_ID_88Q2221, MARVELL_PHY_ID_MASK }, { /*sentinel*/ } }; MODULE_DEVICE_TABLE(mdio, mv88q2xxx_tbl);
Am Sun, Dec 17, 2023 at 02:50:49PM +0100 schrieb Stefan Eichenberger: > Hi Dimitri, > Hi Stefan, > On Sun, Dec 17, 2023 at 12:15:38PM +0100, Dimitri Fedrau wrote: > > Am Sun, Dec 17, 2023 at 10:22:54AM +0100 schrieb Andrew Lunn: > > > > > > + /* Set IEEE power down */ > > > > > > + ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x840); > > > > > > > > > > 0x800 is MDIO_CTRL1_LPOWER. What is the other? It seems like a speed > > > > > selection bit? > > > > > > > > > The other is MDIO_PMA_CTRL1_SPEED1000. Will fix this in V2. > > > > > > It seems odd to set a speed, and power it down. But i guess you have > > > blindly copied the reference code, so have no idea why? > > > > > I agree, absolutely no idea. I already asked the Marvell support for > > any document describing the init sequence, but they couldn't help me. > > So I have to stick to the reference code. At least I copied the comments > > that were part of the init sequence, trying to give some meaning to it. > > 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 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. > 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. > 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 ? > Best regards, > Stefan > Best regards, Dimitri > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c > index 94a8c99b58da..15e82e8ff8f4 100644 > --- a/drivers/net/phy/marvell-88q2xxx.c > +++ b/drivers/net/phy/marvell-88q2xxx.c > @@ -208,17 +214,26 @@ static int mv88q2xxxx_get_sqi(struct phy_device *phydev) > > ret = ret >> 12; > } else { > - /* Read from vendor specific registers, they are not documented > - * but can be found in the Software Initialization Guide. Only > - * revisions >= A0 are supported. > - */ > - ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, 0xFC5D, 0x00FF, 0x00AC); > - if (ret < 0) > - return ret; > + if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2221) { > + /* Read from vendor specific register, they can be > + * found in the sample source code of the Q222X API > + */ > + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfcd9); > + if (ret < 0) > + return ret; > + } else { > + /* Read from vendor specific registers, they are not documented > + * but can be found in the Software Initialization Guide. Only > + * revisions >= A0 are supported. > + */ > + ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, 0xFC5D, 0x00FF, 0x00AC); > + if (ret < 0) > + return ret; > > - ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfc88); > - if (ret < 0) > - return ret; > + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfc88); > + if (ret < 0) > + return ret; > + } > } > > return ret & 0x0F; > @@ -229,6 +244,16 @@ static int mv88q2xxxx_get_sqi_max(struct phy_device *phydev) > return 15; > } > > +static int mv88q2221_probe(struct phy_device *phydev) > +{ > + /* 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); > + > + return 0; > +} > + > static struct phy_driver mv88q2xxx_driver[] = { > { > .phy_id = MARVELL_PHY_ID_88Q2110, > @@ -243,12 +268,27 @@ static struct phy_driver mv88q2xxx_driver[] = { > .get_sqi = mv88q2xxxx_get_sqi, > .get_sqi_max = mv88q2xxxx_get_sqi_max, > }, > + { > + .phy_id = MARVELL_PHY_ID_88Q2221, > + .phy_id_mask = MARVELL_PHY_ID_MASK, > + .name = "mv88q2221", > + .get_features = mv88q2xxx_get_features, > + .config_aneg = mv88q2xxx_config_aneg, > + .config_init = mv88q2xxx_config_init, > + .read_status = mv88q2xxx_read_status, > + .soft_reset = mv88q2xxx_soft_reset, > + .set_loopback = genphy_c45_loopback, > + .get_sqi = mv88q2xxxx_get_sqi, > + .get_sqi_max = mv88q2xxxx_get_sqi_max, > + .probe = mv88q2221_probe, > + }, > }; > > module_phy_driver(mv88q2xxx_driver); > > static struct mdio_device_id __maybe_unused mv88q2xxx_tbl[] = { > { MARVELL_PHY_ID_88Q2110, MARVELL_PHY_ID_MASK }, > + { MARVELL_PHY_ID_88Q2221, MARVELL_PHY_ID_MASK }, > { /*sentinel*/ } > }; > MODULE_DEVICE_TABLE(mdio, mv88q2xxx_tbl);
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
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
On Tue, Dec 19, 2023 at 09:11:17AM +0100, Dimitri Fedrau wrote: > 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 ? Unfortunately I haven't had time to test it yet. I will try to do it on Thursday, otherwise sadly it will be next year. > 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. I am not sure that it will be accepted by the maintainers if you use a lot of registers that are not documented. For this reason, keeping it to a minimum might increase the chances of it being accepted. > 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 I used my own code so far but I will try again with your patches. Maybe send this and the other patches as a whole series so that it gets clear why you need the changes as Andrew wrote. Regards, Stefan
Am Tue, Dec 19, 2023 at 10:19:41AM +0100 schrieb Stefan Eichenberger: > On Tue, Dec 19, 2023 at 09:11:17AM +0100, Dimitri Fedrau wrote: > > > 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 ? > > Unfortunately I haven't had time to test it yet. I will try to do it on > Thursday, otherwise sadly it will be next year. > Ok. > > 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. > > I am not sure that it will be accepted by the maintainers if you use a > lot of registers that are not documented. For this reason, keeping it to > a minimum might increase the chances of it being accepted. > Ok. Will try to reduce them. > > 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 > > I used my own code so far but I will try again with your patches. Maybe > send this and the other patches as a whole series so that it gets clear > why you need the changes as Andrew wrote. > Ok. Will send an V3 including all patches. > Regards, > Stefan Regards, Dimitri
> I am not sure that it will be accepted by the maintainers if you use a > lot of registers that are not documented. Sometimes there is no choice, there is no documentation except the vendor crap driver which we try to clean up as much as possible, but we still end up with lots of magic numbers. Andrew
Am Tue, Dec 19, 2023 at 04:57:50PM +0100 schrieb Andrew Lunn: > > I am not sure that it will be accepted by the maintainers if you use a > > lot of registers that are not documented. > > Sometimes there is no choice, there is no documentation except the > vendor crap driver which we try to clean up as much as possible, but > we still end up with lots of magic numbers. > Hi Andrew, hi Stefan, tried to reduce the init sequence. This worked for me: static int mv88q222x_config_init(struct phy_device *phydev) { int ret; /* send_s detection threshold, slave and master */ ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020); if (ret < 0) return ret; ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xa28); if (ret < 0) return ret; ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xc28); if (ret < 0) return ret; ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc); if (ret < 0) return ret; return mv88q2xxx_config_init(phydev); } The four register writes were required to make the PHY work in 1000Mbit forced mode. When using autonegotiation or 100Mbit forced mode they weren't needed. It was enough to write them once in mv88q222x_config_init as you can see. Thanks Stefan for the hint with the first three register writes, it helped a lot. > Andrew Best regards, Dimitri
On Fri, Jan 05, 2024 at 01:42:21PM +0100, Dimitri Fedrau wrote: > Am Tue, Dec 19, 2023 at 04:57:50PM +0100 schrieb Andrew Lunn: > > > I am not sure that it will be accepted by the maintainers if you use a > > > lot of registers that are not documented. > > > > Sometimes there is no choice, there is no documentation except the > > vendor crap driver which we try to clean up as much as possible, but > > we still end up with lots of magic numbers. > > > > Hi Andrew, hi Stefan, > > tried to reduce the init sequence. This worked for me: > > static int mv88q222x_config_init(struct phy_device *phydev) > { > int ret; > > /* send_s detection threshold, slave and master */ > ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020); > if (ret < 0) > return ret; > > ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xa28); > if (ret < 0) > return ret; > > ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xc28); > if (ret < 0) > return ret; > > ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc); > if (ret < 0) > return ret; > > return mv88q2xxx_config_init(phydev); > } > > The four register writes were required to make the PHY work in 1000Mbit forced > mode. When using autonegotiation or 100Mbit forced mode they weren't needed. > It was enough to write them once in mv88q222x_config_init as you can > see. Thanks Stefan for the hint with the first three register writes, it > helped a lot. Hi Dimitri Do we need to reduce the init sequence? Since this is all undocumented magic which nobody understands, it would be safer to just keep with the Marvell vendor crap code dump. Unless we really do need to change it. Andrew
Am Fri, Jan 05, 2024 at 03:00:58PM +0100 schrieb Andrew Lunn: > On Fri, Jan 05, 2024 at 01:42:21PM +0100, Dimitri Fedrau wrote: > > Am Tue, Dec 19, 2023 at 04:57:50PM +0100 schrieb Andrew Lunn: > > > > I am not sure that it will be accepted by the maintainers if you use a > > > > lot of registers that are not documented. > > > > > > Sometimes there is no choice, there is no documentation except the > > > vendor crap driver which we try to clean up as much as possible, but > > > we still end up with lots of magic numbers. > > > > > > > Hi Andrew, hi Stefan, > > > > tried to reduce the init sequence. This worked for me: > > > > static int mv88q222x_config_init(struct phy_device *phydev) > > { > > int ret; > > > > /* send_s detection threshold, slave and master */ > > ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020); > > if (ret < 0) > > return ret; > > > > ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xa28); > > if (ret < 0) > > return ret; > > > > ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xc28); > > if (ret < 0) > > return ret; > > > > ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc); > > if (ret < 0) > > return ret; > > > > return mv88q2xxx_config_init(phydev); > > } > > > > The four register writes were required to make the PHY work in 1000Mbit forced > > mode. When using autonegotiation or 100Mbit forced mode they weren't needed. > > It was enough to write them once in mv88q222x_config_init as you can > > see. Thanks Stefan for the hint with the first three register writes, it > > helped a lot. > > Hi Dimitri > Hi Andrew, > Do we need to reduce the init sequence? Since this is all undocumented > magic which nobody understands, it would be safer to just keep with > the Marvell vendor crap code dump. Unless we really do need to change > it. > You are right, it would be safer to use the vendor code. But when looking at the vendor code, the init sequence changed a lot from rev. B0 to rev. B1 of the PHY. There are some additional register writes, but mostly the order of the register writes changed. I don't know if this is going to be worse in the future. Maintaining different revisions will probably take some effort or at least result in bloated code. We probably don't need all of the init sequence. I'm not sure how to deal with it, keeping the init sequence at a minimum is probably a good idea. > Andrew Best regards, Dimitri
> Hi Andrew, > > > Do we need to reduce the init sequence? Since this is all undocumented > > magic which nobody understands, it would be safer to just keep with > > the Marvell vendor crap code dump. Unless we really do need to change > > it. > > > You are right, it would be safer to use the vendor code. But when > looking at the vendor code, the init sequence changed a lot from rev. B0 > to rev. B1 of the PHY. There are some additional register writes, but > mostly the order of the register writes changed. I don't know if this is > going to be worse in the future. Maintaining different revisions will > probably take some effort or at least result in bloated code. We probably > don't need all of the init sequence. I'm not sure how to deal with it, > keeping the init sequence at a minimum is probably a good idea. Is the revision in the lower nibble of the ID register? We can handle them as different PHYs, each gets its own init code, and share what can be shared in helper functions. Andrew
Am Fri, Jan 05, 2024 at 05:06:53PM +0100 schrieb Andrew Lunn: > > Hi Andrew, > > > > > Do we need to reduce the init sequence? Since this is all undocumented > > > magic which nobody understands, it would be safer to just keep with > > > the Marvell vendor crap code dump. Unless we really do need to change > > > it. > > > > > You are right, it would be safer to use the vendor code. But when > > looking at the vendor code, the init sequence changed a lot from rev. B0 > > to rev. B1 of the PHY. There are some additional register writes, but > > mostly the order of the register writes changed. I don't know if this is > > going to be worse in the future. Maintaining different revisions will > > probably take some effort or at least result in bloated code. We probably > > don't need all of the init sequence. I'm not sure how to deal with it, > > keeping the init sequence at a minimum is probably a good idea. > > Is the revision in the lower nibble of the ID register? We can handle > them as different PHYs, each gets its own init code, and share what > can be shared in helper functions. > Yes, lowest four bits. Handling them as different PHYs would definitely help maintaining PHY revisions. Still there is the problem with this huge undocumented init sequence. Is this going to be accepted ? Didn't see such a long undocumented init sequence in any other phy driver. > Andrew Best regards, Dimitri
diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c index 1c3ff77de56b..1b79f2ea5ed7 100644 --- a/drivers/net/phy/marvell-88q2xxx.c +++ b/drivers/net/phy/marvell-88q2xxx.c @@ -166,7 +166,9 @@ static int mv88q2xxx_get_features(struct phy_device *phydev) * sequence provided by Marvell. Disable it for now until a proper * workaround is found or a new PHY revision is released. */ - linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported); + if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2110) + linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, + phydev->supported); return 0; } @@ -192,6 +194,9 @@ static int mv88q2xxx_config_init(struct phy_device *phydev) */ phydev->pma_extable = MDIO_PMA_EXTABLE_BT1; + if (phydev->drv->phy_id == MARVELL_PHY_ID_88Q2220) + return 0; + /* Read the current PHY configuration */ ret = genphy_c45_read_pma(phydev); if (ret) @@ -235,6 +240,242 @@ static int mv88q2xxxx_get_sqi_max(struct phy_device *phydev) return 15; } +static int mv88q222x_soft_reset(struct phy_device *phydev) +{ + int ret; + + /* Enable RESET of DCL */ + if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000) { + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48); + if (ret < 0) + return ret; + } + + /* Soft reset */ + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_CTRL, + MDIO_PCS_1000BT1_CTRL_RESET); + if (ret < 0) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0xc); + if (ret < 0) + return ret; + + /* Disable RESET of DCL */ + if (phydev->autoneg == AUTONEG_ENABLE || phydev->speed == SPEED_1000) + return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58); + else + return ret; +} + +static int mv88q222x_config_aneg_gbit(struct phy_device *phydev) +{ + int ret; + + /* send_s detection threshold, slave and master */ + ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8032, 0x2020); + if (ret < 0) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xa28); + if (ret < 0) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_AN, 0x8031, 0xc28); + if (ret < 0) + return ret; + + /* Disable DCL calibratin during tear down */ + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffdb, 0xfc10); + if (ret < 0) + return ret; + + /* Disable RESET of DCL*/ + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x58); + if (ret < 0) + return ret; + + /* Turn CM Clamp ON */ + return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe79, 0x4); +} + +static int mv88q222x_config_aneg_100m(struct phy_device *phydev) +{ + int ret; + + /* Update Initial FFE Coefficients */ + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfbba, 0xcb2); + if (ret < 0) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfbbb, 0xc4a); + if (ret < 0) + return ret; + + /* Turn CM Clamp ON */ + return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe79, 0x4); +} + +static int mv88q222x_config_aneg_preinit(struct phy_device *phydev) +{ + int ret, val, i; + + /* Enable txdac */ + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0x8033, 0x6801); + if (ret < 0) + return ret; + + /* Disable ANEG */ + ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_CTRL, 0x0); + if (ret < 0) + return ret; + + /* Set IEEE power down */ + ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x840); + if (ret < 0) + return ret; + + /* Exit standby state(internal state) */ + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe1b, 0x48); + if (ret < 0) + return ret; + + /* Set power management state breakpoint (internal state) */ + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xffe4, 0x6b6); + if (ret < 0) + return ret; + + /* Exit IEEE power down */ + ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, 0x0); + if (ret < 0) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, 0x0); + if (ret < 0) + return ret; + + /* Wait up to 5ms to enter to power management state, if we do not meet + * the target value, it is still ok to proceed + */ + for (i = 0; i < 5; i++) { + val = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xffe4); + if (val == 0x6ba) + break; + + usleep_range(1000, 2000); + } + + /* Turn CM Clamp OFF */ + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe79, 0x0); + if (ret < 0) + return ret; + + /* mdi vcm */ + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe07, 0x125a); + if (ret < 0) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe09, 0x1288); + if (ret < 0) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe08, 0x2588); + if (ret < 0) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe11, 0x1105); + if (ret < 0) + return ret; + + /* aux_boost */ + return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe72, 0x042c); +} + +static int mv88q222x_config_aneg_init_b0(struct phy_device *phydev) +{ + int ret; + + ret = mv88q222x_config_aneg_preinit(phydev); + if (ret < 0) + return ret; + + if (phydev->autoneg == AUTONEG_DISABLE) { + if (phydev->speed == SPEED_100) + return mv88q222x_config_aneg_100m(phydev); + else + return mv88q222x_config_aneg_gbit(phydev); + } + + ret = mv88q222x_config_aneg_100m(phydev); + if (ret) + return ret; + + ret = mv88q222x_config_aneg_gbit(phydev); + if (ret) + return ret; + + ret = phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe5f, 0xe8); + if (ret) + return ret; + + return phy_write_mmd(phydev, MDIO_MMD_PCS, 0xfe05, 0x755c); +} + +static int mv88q222x_config_aneg(struct phy_device *phydev) +{ + int ret; + + ret = mv88q222x_config_aneg_init_b0(phydev); + if (ret < 0) + return ret; + + ret = genphy_c45_config_aneg(phydev); + if (ret) + return ret; + + return mv88q222x_soft_reset(phydev); +} + +static int mv88q222x_set_loopback(struct phy_device *phydev, bool enable) +{ + return phy_modify_mmd(phydev, + MDIO_MMD_PCS, + MDIO_PCS_1000BT1_CTRL, + MDIO_PCS_CTRL1_LOOPBACK, + enable ? MDIO_PCS_CTRL1_LOOPBACK : 0); +} + +static int mv88q222x_get_sqi(struct phy_device *phydev) +{ + int ret; + + if (phydev->speed == SPEED_100) { + /* Read the SQI from the vendor specific receiver status + * register + */ + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8230); + if (ret < 0) + return ret; + + ret = (ret & 0xe0000) >> 13; + } else { + /* Read the SQI from the vendor specific signal quality index + * register + */ + + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfcd8); + if (ret < 0) + return ret; + } + + return ret & 0x0F; +} + +static int mv88q222x_get_sqi_max(struct phy_device *phydev) +{ + return 7; +} + static struct phy_driver mv88q2xxx_driver[] = { { .phy_id = MARVELL_PHY_ID_88Q2110, @@ -249,12 +490,27 @@ static struct phy_driver mv88q2xxx_driver[] = { .get_sqi = mv88q2xxxx_get_sqi, .get_sqi_max = mv88q2xxxx_get_sqi_max, }, + { + .phy_id = MARVELL_PHY_ID_88Q2220, + .phy_id_mask = MARVELL_PHY_ID_MASK, + .name = "mv88q2220", + .get_features = mv88q2xxx_get_features, + .config_aneg = mv88q222x_config_aneg, + .aneg_done = genphy_c45_aneg_done, + .config_init = mv88q2xxx_config_init, + .read_status = mv88q2xxx_read_status, + .soft_reset = mv88q222x_soft_reset, + .set_loopback = mv88q222x_set_loopback, + .get_sqi = mv88q222x_get_sqi, + .get_sqi_max = mv88q222x_get_sqi_max, + }, }; module_phy_driver(mv88q2xxx_driver); static struct mdio_device_id __maybe_unused mv88q2xxx_tbl[] = { { MARVELL_PHY_ID_88Q2110, MARVELL_PHY_ID_MASK }, + { MARVELL_PHY_ID_88Q2220, MARVELL_PHY_ID_MASK }, { /*sentinel*/ } }; MODULE_DEVICE_TABLE(mdio, mv88q2xxx_tbl); diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h index 9b54c4f0677f..693eba9869e4 100644 --- a/include/linux/marvell_phy.h +++ b/include/linux/marvell_phy.h @@ -26,6 +26,7 @@ #define MARVELL_PHY_ID_88E2110 0x002b09b0 #define MARVELL_PHY_ID_88X2222 0x01410f10 #define MARVELL_PHY_ID_88Q2110 0x002b0980 +#define MARVELL_PHY_ID_88Q2220 0x002b0b20 /* Marvel 88E1111 in Finisar SFP module with modified PHY ID */ #define MARVELL_PHY_ID_88E1111_FINISAR 0x01ff0cc0
Add a driver for the Marvell 88Q2220. This driver allows to detect the link, switch between 100BASE-T1 and 1000BASE-T1 and switch between master and slave mode and autonegotiation. Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> --- drivers/net/phy/marvell-88q2xxx.c | 258 +++++++++++++++++++++++++++++- include/linux/marvell_phy.h | 1 + 2 files changed, 258 insertions(+), 1 deletion(-)