From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH net-next 5/9] net: stmmac: Add MDIO related functions for
 XGMAC2
Date: Thu, 2 Aug 2018 16:11:28 +0200
Message-ID: <20180802141128.GC7462@lunn.ch>
References: <cover.1533125016.git.joabreu@synopsys.com>
 <5a7f4264b01f863dbf79b4f6d5e62cd2a55c758f.1533125016.git.joabreu@synopsys.com>
 <20180801150812.GD32125@lunn.ch>
 <7b0f8758-1a77-5c30-ec2c-ee0a35f66950@synopsys.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: netdev@vger.kernel.org, "David S. Miller" <davem@davemloft.net>,
        Joao Pinto <Joao.Pinto@synopsys.com>,
        Giuseppe Cavallaro <peppe.cavallaro@st.com>,
        Alexandre Torgue <alexandre.torgue@st.com>
To: Jose Abreu <Jose.Abreu@synopsys.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from vps0.lunn.ch ([185.16.172.187]:53922 "EHLO vps0.lunn.ch"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1732617AbeHBQCz (ORCPT <rfc822;netdev@vger.kernel.org>);
        Thu, 2 Aug 2018 12:02:55 -0400
Content-Disposition: inline
In-Reply-To: <7b0f8758-1a77-5c30-ec2c-ee0a35f66950@synopsys.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Thu, Aug 02, 2018 at 09:36:41AM +0100, Jose Abreu wrote:
> Hi Andrew,
> 
> On 01-08-2018 16:08, Andrew Lunn wrote:
> > Hi Jose
> >
> >> +static int stmmac_xgmac2_mdio_read(struct stmmac_priv *priv, int phyaddr,
> >> +				   int phyreg)
> >> +{
> >> +	unsigned int mii_address = priv->hw->mii.addr;
> >> +	unsigned int mii_data = priv->hw->mii.data;
> >> +	u32 tmp, addr, value = MII_XGMAC_BUSY;
> >> +	int data;
> >> +
> >> +	if (phyreg & MII_ADDR_C45) {
> >> +		addr = ((phyreg >> 16) & 0x1f) << 21;
> >> +		addr |= (phyaddr << 16) | (phyreg & 0xffff);
> > Do you need to tell the hardware this is a C45 transfer? Normally an
> > extra bit needs setting somewhere.
> 
> The organization of addr reg is the following:
>     DA [25:21] | PA [20:16] | RA [15:0]
> 
> DA is Device Address, PA is Port Address and RA is Register Address.

Hi Jose

Please read my question. That is not what i asked.

> >
> >> +	} else {
> >> +		if (phyaddr >= 4)
> >> +			return -ENODEV;
> > Can the MDIO bus be external? If so, is there a reason why there
> > cannot be a PHY at addresses > 4. So maybe there is an Ethernet
> > switch, which needs lots of addresses? And C45 can have devices > 4
> > but C22 cannot?
> 
> Only ports 0 to 3 can be configured as C22 ports, according to
> databook.

Please add a comment about this. And EOPNOTSUP might be a better error
code.
 
> >> +	value |= MII_XGMAC_READ;
> >> +
> >> +	if (readl_poll_timeout(priv->ioaddr + mii_data, tmp,
> >> +			       !(tmp & MII_XGMAC_BUSY), 100, 10000))
> >> +		return -EBUSY;
> >> +
> >> +	writel(addr, priv->ioaddr + mii_address);
> >> +	writel(value, priv->ioaddr + mii_data);
> >> +
> >> +	if (readl_poll_timeout(priv->ioaddr + mii_data, tmp,
> >> +			       !(tmp & MII_XGMAC_BUSY), 100, 10000))
> >> +		return -EBUSY;
> >> +
> >> +	/* Read the data from the MII data register */
> >> +	data = (int)readl(priv->ioaddr + mii_data) & GENMASK(15, 0);
> > Is the cast needed? And why use GENMASK here, but not in all the other
> > places you have masks in this code?
> 
> The GENMASK is needed, notice how we set more values into
> mii_data (clk_csr, bit(18), cmd), this is not cleared by XGMAC2
> upon the completion of the operation ...

Again, please read my question. That is not what i asked.

Has C45 been tested? Does the 1G PHY you have support C45 transfers?

       Andrew