Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 14 Apr 2023 03:49:08 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Daniel Golle <daniel@makrotopia.org>
Cc:     netdev@vger.kernel.org, linux-mediatek@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        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>,
        Qingfang Deng <dqfext@gmail.com>,
        SkyLake Huang <SkyLake.Huang@mediatek.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        John Crispin <john@phrozen.org>
Subject: Re: [PATCH net-next] net: phy: add driver for MediaTek SoC built-in
 GE PHYs
Message-ID: <1295d8aa-35e8-4396-b347-efc8d7557c79@lunn.ch>
References: <ZDihjfnzaZ1yh9cT@makrotopia.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZDihjfnzaZ1yh9cT@makrotopia.org>
Precedence: bulk

> +/* Registers on MDIO_MMD_VEND1 */
> +#define MTK_PHY_MIDDLE_LEVEL_SHAPPER_0TO1	0
> +#define MTK_PHY_1st_OVERSHOOT_LEVEL_0TO1	1
> +#define MTK_PHY_2nd_OVERSHOOT_LEVEL_0TO1	2
> +#define MTK_PHY_1st_OVERSHOOT_LEVEL_1TO0	4
> +#define MTK_PHY_2nd_OVERSHOOT_LEVEL_1TO0	5 /* N means negative */
> +#define MTK_PHY_1st_OVERSHOOT_LEVEL_0TON1	7
> +#define MTK_PHY_2nd_OVERSHOOT_LEVEL_0TON1	8
> +#define MTK_PHY_1st_OVERSHOOT_LEVEL_N1TO0	10
> +#define MTK_PHY_2nd_OVERSHOOT_LEVEL_N1TO0	11

Mixed case like this is very unusual.

> +static int tx_amp_fill_result(struct phy_device *phydev, u16 *buf)
> +{
> +	int i;
> +	int bias[16] = {0};
> +	const int vals_9461[16] = { 7, 1, 4, 7,
> +				    7, 1, 4, 7,
> +				    7, 1, 4, 7,
> +				    7, 1, 4, 7 };
> +	const int vals_9481[16] = { 10, 6, 6, 10,
> +				    10, 6, 6, 10,
> +				    10, 6, 6, 10,
> +				    10, 6, 6, 10 };
> +
> +	switch (phydev->drv->phy_id) {
> +	case MTK_GPHY_ID_MT7981:
> +		/* We add some calibration to efuse values
> +		 * due to board level influence.
> +		 * GBE: +7, TBT: +1, HBT: +4, TST: +7
> +		 */
> +		memcpy(bias, (const void *)vals_9461, sizeof(bias));
> +		for (i = 0; i <= 12; i += 4) {
> +			if (likely(buf[i >> 2] + bias[i] >= 32)) {
> +				bias[i] -= 13;
> +			} else {
> +				phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> +					       0x5c, 0x7 << i, bias[i] << i);
> +				bias[i + 1] += 13;
> +				bias[i + 2] += 13;
> +				bias[i + 3] += 13;

How does 13 map to GBE: +7, TBT: +1, HBT: +4, TST: +7 ?

> +static inline void mt798x_phy_common_finetune(struct phy_device *phydev)

No inline functions in .c files. Let the compiler decide. There
appears to be a number of these. And they are big function, too big to
make sense to inline.

>  static struct phy_driver mtk_gephy_driver[] = {
>  	{
> -		PHY_ID_MATCH_EXACT(0x03a29412),
> +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530),
>  		.name		= "MediaTek MT7530 PHY",
>  		.config_init	= mt7530_phy_config_init,
>  		/* Interrupts are handled by the switch, not the PHY
> @@ -84,7 +1205,7 @@ static struct phy_driver mtk_gephy_driver[] = {
>  		.write_page	= mtk_gephy_write_page,
>  	},
>  	{
> -		PHY_ID_MATCH_EXACT(0x03a29441),
> +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7531),
>  		.name		= "MediaTek MT7531 PHY",
>  		.config_init	= mt7531_phy_config_init,
>  		/* Interrupts are handled by the switch, not the PHY

Useful changes, but please put them in a separate patch.

> @@ -97,16 +1218,42 @@ static struct phy_driver mtk_gephy_driver[] = {
>  		.read_page	= mtk_gephy_read_page,
>  		.write_page	= mtk_gephy_write_page,
>  	},
> +#if IS_ENABLED(CONFIG_MEDIATEK_GE_PHY_SOC)
> +	{
> +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7981),
> +		.name		= "MediaTek MT7981 PHY",
> +		.probe		= mt7981_phy_probe,
> +		.config_intr	= genphy_no_config_intr,
> +		.handle_interrupt = genphy_handle_interrupt_no_ack,
> +		.suspend	= genphy_suspend,
> +		.resume		= genphy_resume,
> +		.read_page	= mtk_gephy_read_page,
> +		.write_page	= mtk_gephy_write_page,
> +	},
> +	{
> +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7988),
> +		.name		= "MediaTek MT7988 PHY",
> +		.probe		= mt7988_phy_probe,
> +		.config_intr	= genphy_no_config_intr,
> +		.handle_interrupt = genphy_handle_interrupt_no_ack,
> +		.suspend	= genphy_suspend,
> +		.resume		= genphy_resume,
> +		.read_page	= mtk_gephy_read_page,
> +		.write_page	= mtk_gephy_write_page,

So the only thing these two new PHYs share with the other two PHYs is
mtk_gephy_read_page and mtk_gephy_write_page?

static int mtk_gephy_read_page(struct phy_device *phydev)
{
        return __phy_read(phydev, MTK_EXT_PAGE_ACCESS);
}

static int mtk_gephy_write_page(struct phy_device *phydev, int page)
{
        return __phy_write(phydev, MTK_EXT_PAGE_ACCESS, page);
}

Given the size of the new code, maybe consider adding
mediatek-ge-soc.c and make a copy these two functions?

	Andrew