linux-kernel - Re: [PATCH net-next 3/3] net: phy: mediatek: add driver for built-in 2.5G ethernet PHY on MT7988

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2f0638583f9856499664659b5748056aa5f027c8.camel@mediatek.com>
Date: Fri, 14 Feb 2025 13:25:49 +0000
From: SkyLake Huang (黃啟澤)
	<SkyLake.Huang@...iatek.com>
To: "andrew@...n.ch" <andrew@...n.ch>
CC: "dqfext@...il.com" <dqfext@...il.com>,
	Steven Liu (劉人豪) <steven.liu@...iatek.com>,
	"davem@...emloft.net" <davem@...emloft.net>, AngeloGioacchino Del Regno
	<angelogioacchino.delregno@...labora.com>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "linux@...linux.org.uk"
	<linux@...linux.org.uk>, "hkallweit1@...il.com" <hkallweit1@...il.com>,
	"horms@...nel.org" <horms@...nel.org>, "daniel@...rotopia.org"
	<daniel@...rotopia.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "matthias.bgg@...il.com" <matthias.bgg@...il.com>
Subject: Re: [PATCH net-next 3/3] net: phy: mediatek: add driver for built-in
 2.5G ethernet PHY on MT7988

On Sun, 2025-01-19 at 18:31 +0100, Andrew Lunn wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > +     np = of_find_compatible_node(NULL, NULL, "mediatek,2p5gphy-
> > fw");
> > +     if (!np)
> > +             return -ENOENT;
> 
> The device tree binding need documenting.
> 
> > +     /* 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);
> 
> 0x1100 and 0x00df are magic numbers, bit 0x800e and 0x800f are
> not. Please add #defines.
> 
> 
> > +
> > +     for (i = 0; i < MT7988_2P5GE_PMB_FW_SIZE - 1; i += 4)
> > +             writel(*((uint32_t *)(fw->data + i)), pmb_addr + i);
> > +     dev_info(dev, "Firmware date code: %x/%x/%x, version:
> > %x.%x\n",
> > +              be16_to_cpu(*((__be16 *)(fw->data +
> > +                                       MT7988_2P5GE_PMB_FW_SIZE -
> > 8))),
> > +              *(fw->data + MT7988_2P5GE_PMB_FW_SIZE - 6),
> > +              *(fw->data + MT7988_2P5GE_PMB_FW_SIZE - 5),
> > +              *(fw->data + MT7988_2P5GE_PMB_FW_SIZE - 2),
> > +              *(fw->data + MT7988_2P5GE_PMB_FW_SIZE - 1));
> > +
> > +     writew(reg & ~MD32_EN, mcu_csr_base + MD32_EN_CFG);
> > +     writew(reg | MD32_EN, mcu_csr_base + MD32_EN_CFG);
> > +     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");
> 
> We generally don't spam the log for "Happy Days" conditions. Please
> only log if firmware download fails.
> 
Thanks. I'll fix all the above.

> > +static int mt798x_2p5ge_phy_get_features(struct phy_device
> > *phydev)
> > +{
> > +     int ret;
> > +
> > +     ret = genphy_c45_pma_read_abilities(phydev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* This phy can't handle collision, and neither can (XFI)MAC
> > it's
> > +      * connected to. Although it can do HDX handshake, it doesn't
> > support
> > +      * CSMA/CD that HDX requires.
> > +      */
> > +     linkmode_clear_bit(ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> > +                        phydev->supported);
> 
> So it can do 10BaseT_Half? What about 1000BaseT_Half?
> 
> As you said somewhere, 10/100/1G are not in the C45 space. So does
> genphy_c45_pma_read_abilities() report these features?
> 
>         Andrew
Nope. It can neither do 10BaseT_Half nor 1000BaseT_Half. 10 & 1000
Base_Half bits are already cleared in firmware.

BRs,
Sky