Re: [PATCH net-next v5] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

[Kyle Switch]

On 7/21/26 22:18, Andrew Lunn wrote:
> Just checking something here....
> 
>> +/**
>> + * ytphy_write_top_ext() - write a PHY's top extended register for YT8824
>> + * @phydev: a pointer to a &struct phy_device
>> + * @regnum: register number to write
>> + * @val: register val to write
>> + *
>> + * Returns: the value of regnum reg or negative error code
>> + */
>> +static int ytphy_write_top_ext(struct phy_device *phydev, u16 regnum,
>> +			       u16 val)
>> +{
>> +	int ret;
>> +
>> +	lockdep_assert_held(&phydev->mdio.bus->mdio_lock);
>> +	ret = __phy_package_write(phydev, 0, YTPHY_PAGE_SELECT, regnum);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return __phy_package_write(phydev, 0, YTPHY_PAGE_DATA, val);
> 
> The _top_ registers are in a different MDIO address, and are shared by
> all PHYs within one package. Correct?

Ans: yes, the understanding is correct. The top registers is shared by
     all PHYs.

> 
>> +static int yt8824_write_page(struct phy_device *phydev, int page)
>> +{
>> +	int old_page;
>> +	u16 data;
>> +
>> +	old_page = ytphy_read_top_ext(phydev, YT8521_REG_SPACE_SELECT_REG);
>> +	data = old_page & (~YT8824_RSSR_SPACE_MASK);
>> +	data |= page;
>> +
>> +	return ytphy_write_top_ext(phydev, YT8521_REG_SPACE_SELECT_REG, data);
> 
> The page register is in a top register. So when accessing a paged
> register, all other PHYs also get swapped to that page. And so it is
> necessary to block all other PHYs from accessing registers, until the
> paged access if completed?
> 
> If i have that right, that is an odd hardware design. Since this is an
> odd design, it deserves to be documented. Locking is hard, and it is
> made harder by not having good documentation about the design of the
> locking scheme. 

Ans: Yes, the current operation involves locking at the beginning of the phy8824 driver
     ops and unlocking at the end of the ops function, that is why so many __function 
     involved in this driver, but there are also some special cases that need to be reconsidered 
     such as using  phy_read_poll_timeout().Can you give me some suggestions on the lock scheme?

> 
>> +	old_page = phy_select_page(phydev, reg_space);
>> +	if (old_page < 0)
>> +		goto err_restore_page;
>> +
>> +	if (reg_space == YT8824_RSSR_UTP_SPACE) {
>> +		ret = __phy_read_mmd(phydev, 0x1,
>> +				     YT8824_UTP_TEMPLATE_MODE_CTRL);
> 
> What address spaces are paged? Every other design i've seen only has
> pages of C22 registers, since you only have 32 of them. But looking at
> this code, does this hardware also page C45?
> 

Ans: To access the mmd register of UTP, must first select the page as UTP. I'm not sure if this is 
     the answer you want.

>      Andrew