On 07-08-2018 14:35, Jose Abreu wrote: > Hi Florian, > > On 06-08-2018 16:25, Florian Fainelli wrote: >> On August 6, 2018 12:59:54 AM PDT, Jose Abreu <jose.ab...@synopsys.com> >> 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. >>>>> >>>>> Signed-off-by: Jose Abreu <joab...@synopsys.com> >>>>> Cc: David S. Miller <da...@davemloft.net> >>>>> Cc: Joao Pinto <jpi...@synopsys.com> >>>>> Cc: Giuseppe Cavallaro <peppe.cavall...@st.com> >>>>> Cc: Alexandre Torgue <alexandre.tor...@st.com> >>>>> Cc: Andrew Lunn <and...@lunn.ch> >>>>> --- >>>>> +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? >> You can always extract the code that scans a MDIO bus into a helper function >> and make it parametrized with a callback of some kind. In that case I would >> be fine with you open coding the MDIO bus scan to find out if there is an >> address >= 4. > Sorry but I dont' think thats the best solution because > of_mdiobus_register() already scans the bus.
My mistake. Its stmmac thats scanning the bus. See this: 359 for (addr = 0; addr < PHY_MAX_ADDR; addr++) { 360 struct phy_device *phydev = mdiobus_get_phy(new_bus, addr); 361 362 if (!phydev) 363 continue; So, its just cycling through all possible phys (0..31) ... Do you think it would be okay if I just limit the cycle max to 4 for this xgmac2 for now ? I would maintain the if in the stmmac_xgmac2_c22_format though, as safe-guard. Thanks and Best Regards, Jose Miguel Abreu > Duplicating this > should be avoided, no? > > Note all of this is probably never needed because stmmac just > picks the first phy it finds, if phy_addr is not specified ... > >>>>> + /* 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? >> Because this is all about transactions, the HW must be in a state that it >> will be able to perform that transaction correctly. So here for instance if >> you needed to support a C45 transaction you would have to clear that bit for >> that particular PHY address. Since you don't appear to support those yet >> then yes the code appears fine though it would not hurt if you did clear all >> other PHY's c22 bits to make it clear what this does. > I can't test C45 right now but I will in a near future. In that > case then we will need to support C22 and C45 so I may want to > only set one bit for a specific phy that only supports c22. > > Thanks and Best Regards, > Jose Miguel Abreu >