netdev - Re: [PATCH net-next v2 1/3] net: phy: mediatek: Add 2.5Gphy firmware dt-bindings and dts node

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: <8bc68f1a-5abd-478c-9b9d-2c8baa6bb36a@lunn.ch>
Date: Wed, 26 Feb 2025 14:26:16 +0100
From: Andrew Lunn <andrew@...n.ch>
To: SkyLake Huang (黃啟澤) <SkyLake.Huang@...iatek.com>
Cc: Steven Liu (劉人豪) <steven.liu@...iatek.com>,
	"dqfext@...il.com" <dqfext@...il.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"linux@...linux.org.uk" <linux@...linux.org.uk>,
	"hkallweit1@...il.com" <hkallweit1@...il.com>,
	"horms@...nel.org" <horms@...nel.org>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"daniel@...rotopia.org" <daniel@...rotopia.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 v2 1/3] net: phy: mediatek: Add 2.5Gphy firmware
 dt-bindings and dts node

> So I guess I can do the following according to the previous discussion:
> 1) Reserve a memory region in mt7988.dtsi
> reserved-memory {
> 	#address-cells = <2>;
> 	#size-celss = <2>;
> 	ranges;
> 
> 	/* 0x0f0100000~0x0f1f0024 are specific for built-in 2.5Gphy.
> 	 * In this range, it includes "PMB_FW_BASE"(0x0f100000)
> 	 * and "MCU_CSR_BASE"(0x0f0f0000)
> 	 */
> 	i2p5g: i2p5g@...00000 {
> 		reg = <0 0x0f010000 0 0x1e0024>;
> 		no-map;
> 	};
> };

Do you even need these? I assume this is in the IO space, not DRAM. So
the kernel is not going to use it by default. That is why you need to
specifically ioremap() it.

> 2) Since PHYs don't use compatibles, hardcode address in mtk-2p5ge.c:
> /* MTK_ prefix means that the macro is used for both MT7988 & MT7987*/
> #define MTK_2P5GPHY_PMB_FW_BASE		(0x0f100000)
> #define MT7988_2P5GE_PMB_FW_LEN		(0x20000)
> #define MT7987_2P5GE_PMB_FW_LEN		(0x18000)
> #define MTK_2P5GPHY_MCU_CSR_BASE	(0x0f0f0000)
> #define MTK_2P5GPHY_MCU_CSR_LEN		(0x20)
> 
> /* On MT7987, we separate firmware bin to 2 files and total size
>  * is decreased from 128KB(mediatek/mt7988/i2p5ge-phy-pmb.bin) to
>  * 96KB(mediatek/mt7987/i2p5ge-phy-pmb.bin)+
>  * 28KB(mediatek/mt7987/i2p5ge-phy-DSPBitTb.bin)
>  * And i2p5ge-phy-DSPBitTb.bin will be loaded to
>  * MT7987_2P5GE_XBZ_PMA_RX_BASE
>  */
> #define MT7987_2P5GE_XBZ_PMA_RX_BASE	(0x0f080000)
> #define MT7987_2P5GE_XBZ_PMA_RX_LEN	(0x5228)
> #define MT7987_2P5GE_DSPBITTB_SIZE	(0x7000)
> 
> /* MT7987 requires these base addresses to manipulate some
>  * registers before loading firmware.
>  */
> #define MT7987_2P5GE_APB_BASE		(0x11c30000)
> #define MT7987_2P5GE_APB_LEN		(0x9c)
> #define MT7987_2P5GE_PMD_REG_BASE	(0x0f010000)
> #define MT7987_2P5GE_PMD_REG_LEN	(0x210)
> #define MT7987_2P5GE_XBZ_PCS_REG_BASE	(0x0f030000)
> #define MT7987_2P5GE_XBZ_PCS_REG_LEN	(0x844)

Should the PCS registers actually be in the PCS driver, not the PHY
driver? Hard to say until we know what these registers actually are.

> #define MT7987_2P5GE_CHIP_SCU_BASE	(0x0f0cf800)
> #define MT7987_2P5GE_CHIP_SCU_LEN	(0x12c)
> 
> static int mt7988_2p5ge_phy_load_fw(struct phy_device *phydev)
> {
> 	struct mtk_i2p5ge_phy_priv *priv = phydev->priv;
> 	void __iomem *mcu_csr_base, *pmb_addr;
> 	struct device *dev = &phydev->mdio.dev;
> 	const struct firmware *fw;
> 	int ret, i;
> 	u32 reg;
> 
> 	if (priv->fw_loaded)
> 		return 0;
> 
> 	pmb_addr = ioremap(MTK_2P5GPHY_PMB_FW_BASE,
> 			   MT7988_2P5GE_PMB_FW_LEN);
> 	if (!pmb_addr)
> 		return -ENOMEM;
> 	mcu_csr_base = ioremap(MTK_2P5GPHY_MCU_CSR_BASE,
> 			       MTK_2P5GPHY_MCU_CSR_LEN);
> 	if (!mcu_csr_base) {
> 		ret = -ENOMEM;
> 		goto free_pmb;
> 	}
> ...
> free:
> 	iounmap(mcu_csr_base);
> free_pmb:
> 	iounmap(pmb_addr);
> ...
> }

This looks O.K. It is basically what we did before device tree was
used.

	Andrew