diff mbox series

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

Message ID 20250219083910.2255981-2-SkyLake.Huang@mediatek.com (mailing list archive)
State New
Headers show
Series Add 2.5G ethernet phy support on MT7988 | expand

Commit Message

SkyLake Huang (黃啟澤) Feb. 19, 2025, 8:39 a.m. UTC 
From: Sky Huang <skylake.huang@mediatek.com>

Add 2.5Gphy firmware dt-bindings and dts node since mtk-2p5ge
driver requires firmware to run. Also, update MAINTAINERS for
MediaTek's built-in 2.5Gphy dt-bindings and change MAINTAINER's name.

Signed-off-by: Sky Huang <skylake.huang@mediatek.com>
---
 .../bindings/net/mediatek,2p5gphy-fw.yaml     | 37 +++++++++++++++++++
 MAINTAINERS                                   |  3 +-
 2 files changed, 39 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml

Comments

Krzysztof Kozlowski Feb. 19, 2025, 12:26 p.m. UTC | #1 
On 19/02/2025 09:39, Sky Huang wrote:
> From: Sky Huang <skylake.huang@mediatek.com>
> 
> Add 2.5Gphy firmware dt-bindings and dts node since mtk-2p5ge
> driver requires firmware to run. Also, update MAINTAINERS for
> MediaTek's built-in 2.5Gphy dt-bindings and change MAINTAINER's name.
> 
> Signed-off-by: Sky Huang <skylake.huang@mediatek.com>

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>


> ---
>  .../bindings/net/mediatek,2p5gphy-fw.yaml     | 37 +++++++++++++++++++
>  MAINTAINERS                                   |  3 +-
>  2 files changed, 39 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml b/Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
> new file mode 100644
> index 000000000000..56ebe88b8921
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/mediatek,2p5gphy-fw.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Built-in 2.5G Ethernet PHY
> +
> +maintainers:
> +  - Sky Huang <SkyLake.Huang@mediatek.com>
> +
> +description: |
> +  MediaTek Built-in 2.5G Ethernet PHY needs to load firmware so it can
> +  run correctly.
> +
> +properties:
> +  compatible:
> +    const: "mediatek,2p5gphy-fw"


Not tested.

I have doubts that's a real device... Model name looks exactly like 2.5G
phy. "FW" suggests you do it for driver.

Read writing and submitting bindings documents.

Best regards,
Krzysztof
Andrew Lunn Feb. 19, 2025, 3:26 p.m. UTC | #2 
> +description: |
> +  MediaTek Built-in 2.5G Ethernet PHY needs to load firmware so it can
> +  run correctly.
> +
> +properties:
> +  compatible:
> +    const: "mediatek,2p5gphy-fw"
> +
> +  reg:
> +    items:
> +      - description: pmb firmware load address
> +      - description: firmware trigger register
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    phyfw: phy-firmware@f000000 {
> +      compatible = "mediatek,2p5gphy-fw";
> +      reg = <0 0x0f100000 0 0x20000>,
> +            <0 0x0f0f0018 0 0x20>;
> +    };

This is not a device in itself is it? There is no driver for this.

It seems like these should be properties in the PHY node, since it is
the PHY driver which make use of them. This cannot be the first SoC
device which is both on some sort of serial bus, but also has memory
mapped registers. Please look around and find the correct way to do
this.

	Andrew
Daniel Golle Feb. 19, 2025, 3:30 p.m. UTC | #3 
On Wed, Feb 19, 2025 at 04:26:21PM +0100, Andrew Lunn wrote:
> > +description: |
> > +  MediaTek Built-in 2.5G Ethernet PHY needs to load firmware so it can
> > +  run correctly.
> > +
> > +properties:
> > +  compatible:
> > +    const: "mediatek,2p5gphy-fw"
> > +
> > +  reg:
> > +    items:
> > +      - description: pmb firmware load address
> > +      - description: firmware trigger register
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    phyfw: phy-firmware@f000000 {
> > +      compatible = "mediatek,2p5gphy-fw";
> > +      reg = <0 0x0f100000 0 0x20000>,
> > +            <0 0x0f0f0018 0 0x20>;
> > +    };
> 
> This is not a device in itself is it? There is no driver for this.
> 
> It seems like these should be properties in the PHY node, since it is
> the PHY driver which make use of them. This cannot be the first SoC
> device which is both on some sort of serial bus, but also has memory
> mapped registers.

I'm afraid it is...

> Please look around and find the correct way to do this.

Would using a 'reserved-memory' region be an option maybe?
SkyLake Huang (黃啟澤) Feb. 25, 2025, 10:59 a.m. UTC | #4 
On Wed, 2025-02-19 at 15:30 +0000, Daniel Golle wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Wed, Feb 19, 2025 at 04:26:21PM +0100, Andrew Lunn wrote:
> > > +description: |
> > > +  MediaTek Built-in 2.5G Ethernet PHY needs to load firmware so
> > > it can
> > > +  run correctly.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: "mediatek,2p5gphy-fw"
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: pmb firmware load address
> > > +      - description: firmware trigger register
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    phyfw: phy-firmware@f000000 {
> > > +      compatible = "mediatek,2p5gphy-fw";
> > > +      reg = <0 0x0f100000 0 0x20000>,
> > > +            <0 0x0f0f0018 0 0x20>;
> > > +    };
> > 
> > This is not a device in itself is it? There is no driver for this.
> > 
> > It seems like these should be properties in the PHY node, since it
> > is
> > the PHY driver which make use of them. This cannot be the first SoC
> > device which is both on some sort of serial bus, but also has
> > memory
> > mapped registers.
> 
> I'm afraid it is...
> 
It's actually MCU's memory mapped registers. This MCU will control all
of this built-in the 2.5Gphy's behaviors and it provides faster bus
access speed than MDC/MDIO.

> > Please look around and find the correct way to do this.
> 
> Would using a 'reserved-memory' region be an option maybe?
Or maybe just leave those mapped registers' addresses in driver code
(mtk-2p5ge.c)? Like:
#define MT7988_2P5GE_PMB_BASE (0x0f100000)
#define MT7988_2P5GE_PMB_LEN  (0x20000)

I'm not sure which is more Linux upstream style.

BRs,
Sky
Andrew Lunn Feb. 25, 2025, 1:51 p.m. UTC | #5 
> > Would using a 'reserved-memory' region be an option maybe?
> Or maybe just leave those mapped registers' addresses in driver code
> (mtk-2p5ge.c)? Like:
> #define MT7988_2P5GE_PMB_BASE (0x0f100000)
> #define MT7988_2P5GE_PMB_LEN  (0x20000)

The problem with hard coding them is you need some way to know which
set of hard coded values to use, because the hardware engineers will
not guarantee to never move them, or change the bit layout for the
next generation of devices.

PHYs don't use compatibles because they have an ID in register 2 and
3. Is this ID specific to the MT7988?

	Andrew
SkyLake Huang (黃啟澤) Feb. 26, 2025, 4:14 a.m. UTC | #6 
On Tue, 2025-02-25 at 14:51 +0100, Andrew Lunn wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > > Would using a 'reserved-memory' region be an option maybe?
> > Or maybe just leave those mapped registers' addresses in driver
> > code
> > (mtk-2p5ge.c)? Like:
> > #define MT7988_2P5GE_PMB_BASE (0x0f100000)
> > #define MT7988_2P5GE_PMB_LEN  (0x20000)
> 
> The problem with hard coding them is you need some way to know which
> set of hard coded values to use, because the hardware engineers will
> not guarantee to never move them, or change the bit layout for the
> next generation of devices.
> 
> PHYs don't use compatibles because they have an ID in register 2 and
> 3. Is this ID specific to the MT7988?
> 
>         Andrew
Do you mean that "are MT7988_2P5GE_PMB_BASE & MT7988_2P5GE_PMB_LEN
specific to MT7988"? There'a another our new platform, MT7987, has
almost the same built-in 2.5Gphy. It will use the same "PMB" base
address to load firmware.

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@0f100000 {
		reg = <0 0x0f010000 0 0x1e0024>;
		no-map;
	};
};

Reserve a memory region in mt7987.dtsi
reserved-memory {
	#address-cells = <2>;
	#size-celss = <2>;
	ranges;

	i2p5g: i2p5g@0f100000 {
		reg = <0 0x0f010000 0 0x1e0024>;
		no-map;
	};

	/* For built-in 2.5Gphy's top reset */
	i2p5g_apb: i2p5g_apb@11c30000 {
		reg = <0 0x11c30000 0 0x10c>;
		no-map;
	};
};

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)
#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);
...
}

BRs,
Sky
Andrew Lunn Feb. 26, 2025, 1:26 p.m. UTC | #7 
> 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@0f100000 {
> 		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
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml b/Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
new file mode 100644
index 000000000000..56ebe88b8921
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
@@ -0,0 +1,37 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/mediatek,2p5gphy-fw.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek Built-in 2.5G Ethernet PHY
+
+maintainers:
+  - Sky Huang <SkyLake.Huang@mediatek.com>
+
+description: |
+  MediaTek Built-in 2.5G Ethernet PHY needs to load firmware so it can
+  run correctly.
+
+properties:
+  compatible:
+    const: "mediatek,2p5gphy-fw"
+
+  reg:
+    items:
+      - description: pmb firmware load address
+      - description: firmware trigger register
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    phyfw: phy-firmware@f000000 {
+      compatible = "mediatek,2p5gphy-fw";
+      reg = <0 0x0f100000 0 0x20000>,
+            <0 0x0f0f0018 0 0x20>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index de81a3d68396..42ec8b8d03bf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14716,9 +14716,10 @@  F:	include/linux/pcs/pcs-mtk-lynxi.h
 MEDIATEK ETHERNET PHY DRIVERS
 M:	Daniel Golle <daniel@makrotopia.org>
 M:	Qingfang Deng <dqfext@gmail.com>
-M:	SkyLake Huang <SkyLake.Huang@mediatek.com>
+M:	Sky Huang <SkyLake.Huang@mediatek.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/net/mediatek,2p5gphy-fw.yaml
 F:	drivers/net/phy/mediatek/mtk-ge-soc.c
 F:	drivers/net/phy/mediatek/mtk-phy-lib.c
 F:	drivers/net/phy/mediatek/mtk-ge.c