Received-SPF: pass (google.com: domain of linux-kernel+bounces-3226-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 18 Dec 2023 10:09:32 +0100
From: Dimitri Fedrau <dima.fedrau@gmail.com>
To: Stefan Eichenberger <eichest@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: phy: marvell-88q2xxx: add driver for the Marvell
 88Q2220 PHY
Message-ID: <20231218090932.GA4319@debian>
References: <20231215213102.35994-1-dima.fedrau@gmail.com>
 <74d4b8f9-700e-45bc-af59-95a40a777b00@lunn.ch>
 <20231216221151.GA143483@debian>
 <28cc73bf-ed6d-49d8-b80b-4fbf5fa0442f@lunn.ch>
 <20231217111538.GA3591@debian>
 <ZX78ucHcNyEatXLD@eichest-laptop>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZX78ucHcNyEatXLD@eichest-laptop>

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);