From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jose Abreu Subject: Re: [PATCH v3 net-next 5/9] net: stmmac: Add MDIO related functions for XGMAC2 Date: Mon, 6 Aug 2018 08:59:54 +0100 Message-ID: <982b8bed-e11c-2f94-4f7d-21e358ffc0c0@synopsys.com> References: <53ac7d723d9d16a0b048433e85c2d7a8fafeef17.1533311285.git.joabreu@synopsys.com> <01fc5766-d3ad-3056-6c6c-f9a6d603f87e@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Cc: "David S. Miller" , Joao Pinto , Giuseppe Cavallaro , Alexandre Torgue , Andrew Lunn To: Florian Fainelli , Jose Abreu , Return-path: Received: from smtprelay4.synopsys.com ([198.182.47.9]:55365 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725985AbeHFKHu (ORCPT ); Mon, 6 Aug 2018 06:07:50 -0400 In-Reply-To: <01fc5766-d3ad-3056-6c6c-f9a6d603f87e@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On 03-08-2018 20:06, Florian Fainelli wrote: > On 08/03/2018 08:50 AM, Jose Abreu wrote: >> Add the MDIO related funcionalities for the new IP block XGMAC2. >> >> Signed-off-by: Jose Abreu >> Cc: David S. Miller >> Cc: Joao Pinto >> Cc: Giuseppe Cavallaro >> Cc: Alexandre Torgue >> Cc: Andrew Lunn >> --- >> +satic int stmmac_xgmac2_c22_format(struct stmmac_priv *priv, int phyaddr, >> + int phyreg, u32 *hw_addr) >> +{ >> + unsigned int mii_data = priv->hw->mii.data; >> + u32 tmp; >> + >> + /* HW does not support C22 addr >= 4 */ >> + if (phyaddr >= 4) >> + return -ENODEV; > It would be nice if this could be moved at probe time so you don't have > to wait until you connect to the PHY, read its PHY OUI and find out it > has a MDIO address >= 4. Not a blocker, but something that could be > improved further on. > > In premise you could even scan the MDIO bus' device tree node, and find > that out ahead of time. Oh, but I use phylib ... I only provide the read/write callbacks so I think we should avoid duplicating the code that's already in the phylib ... No? > >> + /* Wait until any existing MII operation is complete */ >> + if (readl_poll_timeout(priv->ioaddr + mii_data, tmp, >> + !(tmp & MII_XGMAC_BUSY), 100, 10000)) >> + return -EBUSY; >> + >> + /* Set port as Clause 22 */ >> + tmp = readl(priv->ioaddr + XGMAC_MDIO_C22P); >> + tmp |= BIT(phyaddr); > Since the registers are being Read/Modify/Write here, don't you need to > clear the previous address bits as well? > > You probably did not encounter any problems in your testing if you had > only one PHY on the MDIO bus, but this is not something that is > necessarily true, e.g: if you have an Ethernet switch, several MDIO bus > addresses are going to be responding. > > Your MDIO bus implementation must be able to support one transaction > with one PHY address and the next transaction with another PHY address , > etc... > > That is something that should be easy to fix and be resubmitted as part > of v4. I'm not following you here... I only set/unset the bit for the corresponding phyaddr that phylib wants to read/write. Why would I clear the remaining addresses? Thanks and Best Regards, Jose Miguel Abreu