From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Fainelli Subject: Re: [PATCH v3 net-next 5/9] net: stmmac: Add MDIO related functions for XGMAC2 Date: Mon, 06 Aug 2018 08:25:03 -0700 Message-ID: References: <53ac7d723d9d16a0b048433e85c2d7a8fafeef17.1533311285.git.joabreu@synopsys.com> <01fc5766-d3ad-3056-6c6c-f9a6d603f87e@gmail.com> <982b8bed-e11c-2f94-4f7d-21e358ffc0c0@synopsys.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Cc: "David S. Miller" , Joao Pinto , Giuseppe Cavallaro , Alexandre Torgue , Andrew Lunn To: Jose Abreu , netdev@vger.kernel.org Return-path: Received: from mail-oi0-f67.google.com ([209.85.218.67]:39277 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732504AbeHFRes (ORCPT ); Mon, 6 Aug 2018 13:34:48 -0400 Received: by mail-oi0-f67.google.com with SMTP id d189-v6so22871945oib.6 for ; Mon, 06 Aug 2018 08:25:12 -0700 (PDT) In-Reply-To: <982b8bed-e11c-2f94-4f7d-21e358ffc0c0@synopsys.com> Sender: netdev-owner@vger.kernel.org List-ID: On August 6, 2018 12:59:54 AM PDT, Jose Abreu = wrote: >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=2E >>> >>> Signed-off-by: Jose Abreu >>> Cc: David S=2E 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 =3D priv->hw->mii=2Edata; >>> + u32 tmp; >>> + >>> + /* HW does not support C22 addr >=3D 4 */ >>> + if (phyaddr >=3D 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 >=3D 4=2E Not a blocker, but something that could be >> improved further on=2E >> >> In premise you could even scan the MDIO bus' device tree node, and >find >> that out ahead of time=2E > >Oh, but I use phylib =2E=2E=2E I only provide the read/write callbacks >so I think we should avoid duplicating the code that's already in >the phylib =2E=2E=2E No? You can always extract the code that scans a MDIO bus into a helper functi= on and make it parametrized with a callback of some kind=2E In that case I = would be fine with you open coding the MDIO bus scan to find out if there i= s an address >=3D 4=2E > >> >>> + /* 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 =3D readl(priv->ioaddr + XGMAC_MDIO_C22P); >>> + tmp |=3D 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=2Eg: if you have an Ethernet switch, several MDIO >bus >> addresses are going to be responding=2E >> >> Your MDIO bus implementation must be able to support one transaction >> with one PHY address and the next transaction with another PHY >address , >> etc=2E=2E=2E >> >> That is something that should be easy to fix and be resubmitted as >part >> of v4=2E > >I'm not following you here=2E=2E=2E I only set/unset the bit for the >corresponding phyaddr that phylib wants to read/write=2E Why would >I clear the remaining addresses? Because this is all about transactions, the HW must be in a state that it = will be able to perform that transaction correctly=2E So here for instance = if you needed to support a C45 transaction you would have to clear that bit= for that particular PHY address=2E Since you don't appear to support those= yet then yes the code appears fine though it would not hurt if you did cle= ar all other PHY's c22 bits to make it clear what this does=2E --=20 Florian