Re: [PATCH v4] net: phy: Add driver for Motorcomm yt8521 gigabit ethernet

[Andrew Lunn]

On Wed, Jul 27, 2022 at 03:08:27PM +0800, Frank wrote:
> patch v4:
>  Hi Andrew,Jakub
>  Thanks very much and based on your comments we modified the patch v4 as below.
> 
>  We evaluated the Marvell 10G driver and at803x you suggested. The 2 drivers
>  implement SFP module attach/detach functions and we think these functions do
>  not help yt8521 to do UTP/Fiber register space arbitration which you may
>  concern in previous patch.
> 
>  Yt8521 can detect utp/fiber media link status automatically. For the case of
>  both media are connected, driver arbitrates the priority of the media (by
>  default, driver takes fiber as higher priority) and report the media status
>  to up layer(MAC).
> 
> patch v3:
>  Hi Andrew
>  Thanks and based on your comments we modified the patch as below.
> 
> > It is generally not that simple. Fibre, you probably want 1000BaseX,
> > unless the fibre module is actually copper, and then you want
> > SGMII. So you need something to talk to the fibre module and ask it
> > what it is. That something is phylink. Phylink does not support both
> > copper and fibre at the same time for one MAC.
> 
>  yes, you said it and for MAC, it does not support copper and Fiber at same time.
>  but from Physical Layer, you know, sometimes both Copper and Fiber cables are
>  connected. in this case, Phy driver should do arbitration and report to MAC
>  which media should be used and Link-up.
>  This is the reason that the driver has a "polling mode", and by default, also
>  this driver takes fiber as first priority which matches phy chip default behavior.
> 
> 
> patch v2:
>  Hi Andrew, Russell King, Peter,
>  Thanks and based on your comments we modified the patch as below.
>  
> > So there's only two possible pages that can be used in the extended
> >register space?
>  
>  Yes,there is only two register space (utp and fiber).
>  
> > > +/* Extended Register's Data Register */
> > > +#define YTPHY_PAGE_DATA                                0x1F
> >
> > These are defined exactly the same way as below. Please reuse code
> > where possible.
>  
>  Yes, code will be reuse, but "YT8511_PAGE" need to be rename like
>  "YTPHY_PAGE_DATA",as it is common register for yt phys.
>  
> 
> patch v1:
>  Add a driver for the motorcomm yt8521 gigabit ethernet phy. We have verified
>  the driver on StarFive VisionFive development board, which is developed by
>  Shanghai StarFive Technology Co., Ltd.. On the board, yt8521 gigabit ethernet
>  phy works in utp mode, RGMII interface, supports 1000M/100M/10M speeds, and
>  wol(magic package).
> 
> Signed-off-by: Frank <Frank.Sae@xxxxxxxxxxxxxx>
> ---
>  MAINTAINERS                 |    1 +
>  drivers/net/phy/Kconfig     |    2 +-
>  drivers/net/phy/motorcomm.c | 1170 ++++++++++++++++++++++++++++++++++-
>  3 files changed, 1170 insertions(+), 3 deletions(-)

This is not the correct way to format the commit message. All the text
you have above will end up in the commit. Please put all discussion
after the --- .


> +	/* If it is reset, need to wait for the reset to complete */
> +	if (set == BMCR_RESET) {
> +		while (max_cnt--) {
> +			/* unlock mdio bus during sleep */
> +			phy_unlock_mdio_bus(phydev);
> +			usleep_range(1000, 1100);
> +			phy_lock_mdio_bus(phydev);
> +
> +			ret = __phy_read(phydev, MII_BMCR);
> +			if (ret < 0)
> +				goto err_restore_page;
> +
> +			if (!(ret & BMCR_RESET))
> +				return phy_restore_page(phydev, old_page, 0);
> +		}
> +		if (max_cnt <= 0)
> +			ret = -ETIME;

ETIMEDOUT.

	Andrew