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

[Andrew Lunn]

On Thu, Jul 23, 2026 at 05:28:39PM +0800, Kyle Switch wrote:
> 
> 
> On 7/22/26 21:18, Andrew Lunn wrote:
> > On Wed, Jul 22, 2026 at 04:06:31PM +0800, Kyle Switch wrote:
> >>
> >>
> >> 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?
> > 
> > So, that is a really bad design. As a software engineer you should go
> > talk to your hardware engineers and tell them what is wrong with this
> > design. Hopefully they will then fix it for the next generation, move
> > the page register into the PHYs own address space, a page register per
> > PHY so removing this locking.
> > 
> >>>> +	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.
> > 
> > So it sounds like each MDIO address on the bus actually has two full
> > PHYs on it, and this page register allows you to swap the accesses
> > between the two. Are there two sets of MDI pins, one for each PHY?
> > Can both FIBER and UTP be used at the same time? Could i configure
> > both copper and fibre parts, see which gets link first, and then
> > declare that the winner? Maybe you should actually be exposing both
> > PHYs, with a mux between the MAC and the PHYs. This is something
> > Maxime has been working on.
> > 
> > This whole concept of one shared register controlling page access to
> > all PHYs within the package sounds so wrong, i would go talk to the
> > hardware engineers. Maybe you are missing something, the software is
> > not doing what the hardware designer intended? Because this is so
> > ugly.
> > 
> 
> Ans: Perhaps there is indeed a misunderstanding here. The working mode 
>      of phy8824 is"mac <---> usxgmii ----UTP". The panel ports of 
>      PHY8824 is always UTP.

So what is all this code about YT8824_RSSR_FIBER_SPACE for?

I'm assuming this device can be used similar to the marvell
m88e1111. It can either be used with Base-T UTP, or it can be
connected to an SFP cage. It uses different pages to configure UTP vs
fibre. Your driver seems to have a similar structure.

       Andrew