| 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
| ||
|
Message-ID: <CAK+owogXdnvoiVNBn_6uBZgqoHrkiQmVU58ip1Wqd6v8VX5f6A@mail.gmail.com>
Date: Thu, 15 May 2025 19:23:53 +0200
From: Stefano Radaelli <stefano.radaelli21@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Heiner Kallweit <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Xu Liang <lxu@...linear.com>
Subject: Re: [PATCH] net: phy: add driver for MaxLinear MxL86110 PHY
Hi Andrew,
> If there is only one page, does these make any sense?
You're totally right: the MxL86110 does not have a traditional page
mechanism like other PHYs. The so-called "page" access via register 30
is simply the standard MaxLinear mechanism to access extended
registers: reg 30 is used to select the register address, and reg 31 is used
for the data. So in this case, I can drop both 'phy_select_page()' and the
'read_page'/'write_page' functions entirely, and just keep the
'mxl86110_{read,write}_extended_reg()' helpers.
> This is confusing. It looks identical to mxl86110_write_page(). So
> regnum is actually page?
Exactly; as mentioned above, regnum isn't a page, it's the actual address
of the extended register I'm trying to access. The name `regnum` is
still accurate
because it corresponds to the extended register map defined by MaxLinear
and written to reg 30 before accessing reg 31.
> So does the value 1 here mean 8ns? 0 would be 2ns?
Setting RXDLY_ENABLE = 1 enables the internal fixed RX_CLK delay
provided by the PHY, but the actual delay value depends on the
RX clock frequency: approximately 2 ns at 125 MHz, and ~8 ns at 25 or 2.5 MHz.
I'm not explicitly selecting 2 or 8 ns, it's applied automatically by the PHY
based on clock rate.
Since this delay is additive with the configurable digital RX delay
set in `CFG1_REG`, I only configure 150 ps in the digital field to
avoid over-delaying.
That said, if you prefer, I can disable `RXDLY_ENABLE` and set to 1950 ps
directly in the digital delay field. Just let me know what you'd prefer here.
Thanks again,
Stefano
Il giorno gio 15 mag 2025 alle ore 18:33 Andrew Lunn <andrew@...n.ch>
ha scritto:
>
> > +/* only 1 page for MXL86110 */
> > +#define MXL86110_DEFAULT_PAGE 0
>
> > +/**
> > + * mxl86110_read_page - Read current page number
> > + * @phydev: Pointer to the PHY device
> > + *
> > + * Return: The currently selected page number, or negative errno on failure.
> > + */
> > +static int mxl86110_read_page(struct phy_device *phydev)
> > +{
> > + return __phy_read(phydev, MXL86110_EXTD_REG_ADDR_OFFSET);
> > +};
>
> If there is only one page, does these make any sense?
>
> > +static int mxl86110_write_page(struct phy_device *phydev, int page)
> > +{
> > + return __phy_write(phydev, MXL86110_EXTD_REG_ADDR_OFFSET, page);
> > +};
> > +
> > +/**
> > + * mxl86110_write_extended_reg() - write to a PHY's extended register
> > + * @phydev: pointer to a &struct phy_device
> > + * @regnum: register number to write
> > + * @val: value to write to @regnum
> > + *
> > + * NOTE: This function assumes the caller already holds the MDIO bus lock
> > + * or otherwise has exclusive access to the PHY. If exclusive access
> > + * cannot be guaranteed, please use mxl86110_locked_write_extended_reg()
> > + * which handles locking internally.
> > + *
> > + * returns 0 or negative error code
> > + */
> > +static int mxl86110_write_extended_reg(struct phy_device *phydev, u16 regnum, u16 val)
> > +{
> > + int ret;
> > +
> > + ret = __phy_write(phydev, MXL86110_EXTD_REG_ADDR_OFFSET, regnum);
>
> This is confusing. It looks identical to mxl86110_write_page(). So
> regnum is actually page?
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + return __phy_write(phydev, MXL86110_EXTD_REG_ADDR_DATA, val);
>
> And within that page, there is a single register at address
> MXL86110_EXTD_REG_ADDR_DATA?
>
> If you keep the write_page() and read_page(), it looks like you can
> replace this with
>
> return phy_write_paged(phydev, regnum,
> MXL86110_EXTD_REG_ADDR_DATA,
> val);
>
> > +static int mxl86110_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
> > +{
> > + struct net_device *netdev;
> > + int page_to_restore;
> > + const u8 *mac;
> > + int ret = 0;
> > +
> > + if (wol->wolopts & WAKE_MAGIC) {
> > + netdev = phydev->attached_dev;
> > + if (!netdev)
> > + return -ENODEV;
> > +
> > + page_to_restore = phy_select_page(phydev, MXL86110_DEFAULT_PAGE);
> > + if (page_to_restore < 0)
> > + goto error;
>
> If there is only one page, i think this can be removed. And everywhere
> else in the driver.
>
> > + /*
> > + * RX_CLK delay (RXDLY) enabled via CHIP_CFG register causes a fixed
> > + * delay of approximately 2 ns at 125 MHz or 8 ns at 25/2.5 MHz.
> > + * Digital delays in RGMII_CFG1 register are additive
> > + */
> > + switch (phydev->interface) {
> > + case PHY_INTERFACE_MODE_RGMII:
> > + val = 0;
> > + break;
> > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > + val = MXL86110_EXT_RGMII_CFG1_RX_DELAY_150PS;
>
> This should be 2000ps, or the nearest you can get to it.
>
> > + break;
> > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > + val = MXL86110_EXT_RGMII_CFG1_TX_1G_DELAY_2250PS |
> > + MXL86110_EXT_RGMII_CFG1_TX_10MB_100MB_DELAY_2250PS;
> > + break;
> > + case PHY_INTERFACE_MODE_RGMII_ID:
> > + val = MXL86110_EXT_RGMII_CFG1_TX_1G_DELAY_2250PS |
> > + MXL86110_EXT_RGMII_CFG1_TX_10MB_100MB_DELAY_2250PS;
> > + val |= MXL86110_EXT_RGMII_CFG1_RX_DELAY_150PS;
>
> Same here.
>
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + goto err_restore_page;
> > + }
> > + ret = mxl86110_modify_extended_reg(phydev, MXL86110_EXT_RGMII_CFG1_REG,
> > + MXL86110_EXT_RGMII_CFG1_FULL_MASK, val);
> > + if (ret < 0)
> > + goto err_restore_page;
> > +
> > + /* Configure RXDLY (RGMII Rx Clock Delay) to keep the default
> > + * delay value on RX_CLK (2 ns for 125 MHz, 8 ns for 25 MHz/2.5 MHz)
> > + */
> > + ret = mxl86110_modify_extended_reg(phydev, MXL86110_EXT_CHIP_CFG_REG,
> > + MXL86110_EXT_CHIP_CFG_RXDLY_ENABLE, 1);
>
> So does the value 1 here mean 8ns? 0 would be 2ns?
>
> Andrew
>
> ---
> pw-bot: cr
Powered by blists - more mailing lists