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

["SkyLake Huang (黃啟澤)" <SkyLake.Huang@mediatek.com>]

On Wed, 2025-02-26 at 14:26 +0100, Andrew Lunn wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> > 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.

Agree. I'll remove this.

> 
> > 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.
> 
These PCS registers are in different domain with USXGMII's PCS on
MT7988. These PCS registers are only used by built-in 2.5Gphy when
loading firmware. I'll submit MT7987's built-in 2.5Gphy driver later
and we can check if another PCS driver is needed or not.

> > #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

OK. I'll submit v3 in this way.

BRs,
Sky