Date: Tue, 21 May 2024 09:07:36 +0000
From: SkyLake Huang (黃啟澤)
	<SkyLake.Huang@...iatek.com>
To: "andrew@...n.ch" <andrew@...n.ch>
CC: "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>, "linux@...linux.org.uk"
	<linux@...linux.org.uk>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"dqfext@...il.com" <dqfext@...il.com>,
	Steven Liu (劉人豪) <steven.liu@...iatek.com>,
	"matthias.bgg@...il.com" <matthias.bgg@...il.com>, "davem@...emloft.net"
	<davem@...emloft.net>, "hkallweit1@...il.com" <hkallweit1@...il.com>,
	"daniel@...rotopia.org" <daniel@...rotopia.org>,
	"angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G
 ethernet PHY on MT7988

On Mon, 2024-05-20 at 15:33 +0200, Andrew Lunn wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  > +static int mt798x_2p5ge_phy_config_init(struct phy_device
> *phydev)
> > +{
> > +struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> > +struct device *dev = &phydev->mdio.dev;
> > +const struct firmware *fw;
> > +struct pinctrl *pinctrl;
> > +int ret, i;
> > +u16 reg;
> > +
> > +if (!priv->fw_loaded) {
> > +if (!priv->md32_en_cfg_base || !priv->pmb_addr) {
> > +dev_err(dev, "MD32_EN_CFG base & PMB addresses aren't valid\n");
> > +return -EINVAL;
> > +}
> > +
> 
> https://www.kernel.org/doc/html/latest/process/coding-style.html
> 
>   6) Functions
> 
>   Functions should be short and sweet, and do just one thing. They
>   should fit on one or two screenfuls of text (the ISO/ANSI screen
>   size is 80x24, as we all know), and do one thing and do that well.
> 
> This is a big function, which does multiple things. Maybe pull the
> downloading of firmware into a helper.
> 
Agree. I'll move this part to another function.

> > +ret = request_firmware(&fw, MT7988_2P5GE_PMB, dev);
> > +if (ret) {
> > +dev_err(dev, "failed to load firmware: %s, ret: %d\n",
> > +MT7988_2P5GE_PMB, ret);
> > +return ret;
> > +}
> > +
> > +if (fw->size != MT7988_2P5GE_PMB_SIZE) {
> > +dev_err(dev, "Firmware size 0x%zx != 0x%x\n",
> > +fw->size, MT7988_2P5GE_PMB_SIZE);
> > +return -EINVAL;
> > +}
> > +
> > +reg = readw(priv->md32_en_cfg_base);
> > +if (reg & MD32_EN) {
> > +phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
> > +usleep_range(10000, 11000);
> > +}
> > +phy_set_bits(phydev, MII_BMCR, BMCR_PDOWN);
> > +
> > +/* Write magic number to safely stall MCU */
> > +phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x800e, 0x1100);
> > +phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x800f, 0x00df);
> > +
> > +for (i = 0; i < MT7988_2P5GE_PMB_SIZE - 1; i += 4)
> > +writel(*((uint32_t *)(fw->data + i)), priv->pmb_addr + i);
> > +release_firmware(fw);
> > +
> > +writew(reg & ~MD32_EN, priv->md32_en_cfg_base);
> > +writew(reg | MD32_EN, priv->md32_en_cfg_base);
> > +phy_set_bits(phydev, MII_BMCR, BMCR_RESET);
> > +/* We need a delay here to stabilize initialization of MCU */
> > +usleep_range(7000, 8000);
> > +dev_info(dev, "Firmware loading/trigger ok.\n");
> > +
> > +priv->fw_loaded = true;
> 
> So there is no way to know if this has already happened? Maybe the
> bootloader downloaded the firmware so it could TFTP boot? Linux will
> download the firmware again, which is a waste of time.
> 
Normal MTK's internal 2.5Gphy firmware loading procedure will be like:
1. request firmware
2. write firmware to correspoding memory address via sbus2pbus(faster
than MDIO)
3. Trigger MCU to run firmware by setting MD32_EN bit.

    So logically, we can determine if firmware is loaded by
bootloader(Uboot) or not by reading MD32_EN bit (step 3 above).
However, if step2 is bypassed, MD32_EN bit can still be set if CL22
reset (BMCR_RESET) is triggered by user. In this case, internal
2.5Gphy's MCU is running nothing.
    That is to say, for safety, we need to load firmware again right
atfer booting into Linux kernel. Actually, this takes just about 11ms.

> > +iounmap(priv->md32_en_cfg_base);
> > +iounmap(priv->pmb_addr);
> > +}
> > +
> > +/* Setup LED */
> > +phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED0_ON_CTRL,
> > + MTK_PHY_LED_ON_POLARITY | MTK_PHY_LED_ON_LINK10 |
> > + MTK_PHY_LED_ON_LINK100 | MTK_PHY_LED_ON_LINK1000 |
> > + MTK_PHY_LED_ON_LINK2500);
> > +phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED1_ON_CTRL,
> > + MTK_PHY_LED_ON_FDX | MTK_PHY_LED_ON_HDX);
> > +
> > +pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "i2p5gbe-
> led");
> 
> Calls to devm_pinctrl_get_select() is pretty unusual in drivers:
> 
> 
https://elixir.bootlin.com/linux/latest/C/ident/devm_pinctrl_get_select
> 
> Why is this needed? Generally, the DT file should describe the needed
> pinmux setting, without needed anything additionally.
> 
This is needed because we need to switch to i2p5gbe-led pinmux group
after we set correct polarity. Or LED may blink unexpectedly.

> > +static int mt798x_2p5ge_phy_get_features(struct phy_device
> *phydev)
> > +{
> > +int ret;
> > +
> > +ret = genphy_c45_pma_read_abilities(phydev);
> > +if (ret)
> > +return ret;
> > +
> > +/* We don't support HDX at MAC layer on mt7988. So mask phy's HDX
> capabilities here. */
> 
> So you make it clear, the MAC does not support half duplex. The MAC
> should then remove it, not the PHY.
> 
> > +linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT, phydev-
> >supported);
> > +
> > +return 0;
> > +}
> > +
> > +static int mt798x_2p5ge_phy_read_status(struct phy_device *phydev)
> > +{
> > +int ret;
> > +
> > +/* When MDIO_STAT1_LSTATUS is raised genphy_c45_read_link(), this
> phy actually
> > + * hasn't finished AN. So use CL22's link update function instead.
> > + */
> > +ret = genphy_update_link(phydev);
> > +if (ret)
> > +return ret;
> > +
> > +phydev->speed = SPEED_UNKNOWN;
> > +phydev->duplex = DUPLEX_UNKNOWN;
> > +phydev->pause = 0;
> > +phydev->asym_pause = 0;
> > +
> > +/* We'll read link speed through vendor specific registers down
> below. So remove
> > + * phy_resolve_aneg_linkmode (AN on) & genphy_c45_read_pma (AN
> off).
> > + */
> > +if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) 
> {
> > +ret = genphy_c45_read_lpa(phydev);
> > +if (ret < 0)
> > +return ret;
> > +
> > +/* Clause 45 doesn't define 1000BaseT support. Read the link
> partner's 1G
> > + * advertisement via Clause 22
> > + */
> > +ret = phy_read(phydev, MII_STAT1000);
> > +if (ret < 0)
> > +return ret;
> > +mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, ret);
> > +} else if (phydev->autoneg == AUTONEG_DISABLE) {
> > +return -EOPNOTSUPP;
> > +}
> 
> It is a bit late doing this now. The user requested this a long time
> ago, and it will be hard to understand why it now returns EOPNOTSUPP.
> You should check for AUTONEG_DISABLE in config_aneg() and return the
> error there.
> 
>       Andrew
Maybe I shouldn't return EOPNOTSUPP in config_aneg directly?
In this way, _phy_state_machine will be broken if I trigger "$ ethtool
-s ethtool eth1 autoneg off"

[  520.781368] ------------[ cut here ]------------
root@...nWrt:/# [  520.785978] _phy_start_aneg+0x0/0xa4: returned: -95
[  520.792263] WARNING: CPU: 3 PID: 423 at drivers/net/phy/phy.c:1254 _phy_state_machine+0x78/0x258
[  520.801039] Modules linked in:
[  520.804087] CPU: 3 PID: 423 Comm: kworker/u16:4 Tainted: G        W          6.8.0-rc7-next-20240306-bpi-r3+ #102
[  520.814335] Hardware name: MediaTek MT7988A Reference Board (DT)
[  520.820330] Workqueue: events_power_efficient phy_state_machine
[  520.826240] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  520.833190] pc : _phy_state_machine+0x78/0x258
[  520.837623] lr : _phy_state_machine+0x78/0x258
[  520.842056] sp : ffff800084b7bd30
[  520.845360] x29: ffff800084b7bd30 x28: 0000000000000000 x27: 0000000000000000
[  520.852487] x26: ffff000000c56900 x25: ffff000000c56980 x24: ffff000000012ac0
[  520.859613] x23: ffff00000001d005 x22: ffff000000fdf000 x21: 0000000000000001
[  520.866738] x20: 0000000000000004 x19: ffff000003a90000 x18: ffffffffffffffff
[  520.873864] x17: 0000000000000000 x16: 0000000000000000 x15: ffff800104b7b977
[  520.880988] x14: 0000000000000000 x13: 0000000000000183 x12: 00000000ffffffea
[  520.888114] x11: 0000000000000001 x10: 0000000000000001 x9 : ffff8000837222f0
[  520.895239] x8 : 0000000000017fe8 x7 : c0000000ffffefff x6 : 0000000000000001
[  520.902365] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
[  520.909490] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000004120000
[  520.916615] Call trace:
[  520.919052]  _phy_state_machine+0x78/0x258
[  520.923139]  phy_state_machine+0x2c/0x80
[  520.927051]  process_one_work+0x178/0x31c
[  520.931054]  worker_thread+0x280/0x494
[  520.934795]  kthread+0xe4/0xe8
[  520.937841]  ret_from_fork+0x10/0x20
[  520.941408] ---[ end trace 0000000000000000 ]---

Now I prefer to give a warning like this, in
mt798x_2p5ge_phy_config_aneg():
if (phydev->autoneg == AUTONEG_DISABLE) {
	dev_warn(&phydev->mdio.dev, "Once AN off is set, this phy can't
link.\n");
	return genphy_c45_pma_setup_forced(phydev);
}

Powered by blists - more mailing lists