Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6b50b9996faffe9fbddb54ed6a407b037a8509ff.camel@redhat.com>
Subject: Re: [PATCH net-next v8.1] net: phy: Add driver for Motorcomm yt8521
 gigabit ethernet phy
From:   Paolo Abeni <pabeni@redhat.com>
To:     Frank <Frank.Sae@motor-comm.com>, Peter Geis <pgwipeout@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>
Cc:     yinghong.zhang@motor-comm.com, fei.zhang@motor-comm.com,
        hua.sun@motor-comm.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Thu, 27 Oct 2022 11:23:09 +0200
In-Reply-To: <20221025064532.1595-1-Frank.Sae@motor-comm.com>
References: <20221025064532.1595-1-Frank.Sae@motor-comm.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello,

On Tue, 2022-10-25 at 14:45 +0800, Frank wrote:
[...]
> +/**
> + * yt8521_read_status_paged() -  determines the speed and duplex of one page
> + * @phydev: a pointer to a &struct phy_device
> + * @page: The reg page(YT8521_RSSR_FIBER_SPACE/YT8521_RSSR_UTP_SPACE) to
> + * operate.
> + *
> + * returns 1 (utp or fiber link),0 (no link) or negative errno code
> + */
> +static int yt8521_read_status_paged(struct phy_device *phydev, int page)
> +{
> +	int fiber_latch_val;
> +	int fiber_curr_val;
> +	int old_page;
> +	int ret = 0;
> +	int status;
> +	int link;
> +
> +	linkmode_zero(phydev->lp_advertising);
> +	phydev->duplex = DUPLEX_UNKNOWN;
> +	phydev->speed = SPEED_UNKNOWN;
> +	phydev->asym_pause = 0;
> +	phydev->pause = 0;
> +
> +	/* YT8521 has two reg space (utp/fiber) for linkup with utp/fiber
> +	 * respectively. but for utp/fiber combo mode, reg space should be
> +	 * arbitrated based on media priority. by default, utp takes
> +	 * priority. reg space should be properly set before read
> +	 * YTPHY_SPECIFIC_STATUS_REG.
> +	 */
> +
> +	page &= YT8521_RSSR_SPACE_MASK;
> +	old_page = phy_select_page(phydev, page);
> +	if (old_page < 0)
> +		goto err_restore_page;
> +
> +	/* Read YTPHY_SPECIFIC_STATUS_REG, which indicates the speed and duplex
> +	 * of the PHY is actually using.
> +	 */
> +	ret = __phy_read(phydev, YTPHY_SPECIFIC_STATUS_REG);
> +	if (ret < 0)
> +		goto err_restore_page;
> +
> +	status = ret;
> +	link = !!(status & YTPHY_SSR_LINK);
> +
> +	/* When PHY is in fiber mode, speed transferred from 1000Mbps to
> +	 * 100Mbps,there is not link down from YTPHY_SPECIFIC_STATUS_REG, so
> +	 * we need check MII_BMSR to identify such case.
> +	 */
> +	if (page == YT8521_RSSR_FIBER_SPACE) {
> +		ret = __phy_read(phydev, MII_BMSR);
> +		if (ret < 0)
> +			goto err_restore_page;
> +
> +		fiber_latch_val = ret;
> +		ret = __phy_read(phydev, MII_BMSR);
> +		if (ret < 0)
> +			goto err_restore_page;
> +
> +		fiber_curr_val = ret;
> +		if (link && fiber_latch_val != fiber_curr_val) {
> +			link = 0;
> +			phydev_info(phydev,
> +				    "%s, fiber link down detect, latch = %04x, curr = %04x\n",
> +				    __func__, fiber_latch_val, fiber_curr_val);
> +		}
> +	} else {
> +		/* Read autonegotiation status */
> +		ret = __phy_read(phydev, MII_BMSR);
> +		if (ret < 0)
> +			return ret;

You need to restore the page even on this error path.


[...]


> +/**
> + * yt8521_modify_bmcr_paged - bits modify a PHY's BMCR register of one page
> + * @phydev: the phy_device struct
> + * @page: The reg page(YT8521_RSSR_FIBER_SPACE/YT8521_RSSR_UTP_SPACE) to operate
> + * @mask: bit mask of bits to clear
> + * @set: bit mask of bits to set
> + *
> + * NOTE: Convenience function which allows a PHY's BMCR register to be
> + * modified as new register value = (old register value & ~mask) | set.
> + * YT8521 has two space (utp/fiber) and three mode (utp/fiber/poll), each space
> + * has MII_BMCR. poll mode combines utp and faber,so need do both.
> + * If it is reset, it will wait for completion.
> + *
> + * returns 0 or negative errno code
> + */
> +static int yt8521_modify_bmcr_paged(struct phy_device *phydev, int page,
> +				    u16 mask, u16 set)
> +{
> +	int max_cnt = 500; /* the max wait time of reset ~ 500 ms */
> +	int old_page;
> +	int ret = 0;
> +
> +	old_page = phy_select_page(phydev, page & YT8521_RSSR_SPACE_MASK);
> +	if (old_page < 0)
> +		goto err_restore_page;
> +
> +	ret = __phy_modify(phydev, MII_BMCR, mask, set);
> +	if (ret < 0)
> +		goto err_restore_page;
> +
> +	/* If it is reset, need to wait for the reset to complete */
> +	if (set == BMCR_RESET) {
> +		while (max_cnt--) {
> +			usleep_range(1000, 1100);
> +			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)

The above check is not needed, the loop can terminate only when such
condition is true.


Cheers,

Paolo