From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jose Abreu <Jose.Abreu@synopsys.com>
Subject: Re: [PATCH net-next 5/9] net: stmmac: Add MDIO related functions for
 XGMAC2
Date: Thu, 2 Aug 2018 09:36:41 +0100
Message-ID: <7b0f8758-1a77-5c30-ec2c-ee0a35f66950@synopsys.com>
References: <cover.1533125016.git.joabreu@synopsys.com>
 <5a7f4264b01f863dbf79b4f6d5e62cd2a55c758f.1533125016.git.joabreu@synopsys.com>
 <20180801150812.GD32125@lunn.ch>
Mime-Version: 1.0
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
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: Andrew Lunn <andrew@lunn.ch>, Jose Abreu <Jose.Abreu@synopsys.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from smtprelay2.synopsys.com ([198.182.60.111]:58425 "EHLO
        smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1726224AbeHBK0v (ORCPT
        <rfc822;netdev@vger.kernel.org>); Thu, 2 Aug 2018 06:26:51 -0400
In-Reply-To: <20180801150812.GD32125@lunn.ch>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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.

>
>> +	} 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. This for MDIO bus trough controller.

>
>> +		writel(~0x0, priv->ioaddr + 0x220);
>> +		addr = (phyaddr << 16) | (phyreg & 0x1f);
>> +	}
>> +
>> +	value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
>> +		& priv->hw->mii.clk_csr_mask;
>> +	value |= BIT(18);
> Please add a #define for this bit.

Ok.

>
>> +	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 ...

>
>>  /**
>>   * stmmac_mdio_read
>>   * @bus: points to the mii_bus structure
>> @@ -59,6 +141,9 @@ static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
>>  	int data;
>>  	u32 value = MII_BUSY;
>>  
>> +	if (priv->plat->has_xgmac)
>> +		return stmmac_xgmac2_mdio_read(priv, phyaddr, phyreg);
> It would be cleaner to instead do this in stmmac_mdio_register() when
> setting new_bus->read.

Makes sense! Thanks!

Thanks and Best Regards,
Jose Miguel Abreu

>
> 	Andrew