netdev - Re: [PATCH net-next 5/5] net: lan743x: Add Clause-45 MDIO access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20220131181457.v6sqhvv2jrewisxm@microsemi.com>
Date:   Mon, 31 Jan 2022 23:44:57 +0530
From:   Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     <netdev@...r.kernel.org>, <davem@...emloft.net>, <kuba@...nel.org>,
        <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net-next 5/5] net: lan743x: Add Clause-45 MDIO access

Hi Andrew,

Thank you for review comments.

The 01/27/2022 23:23, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Jan 27, 2022 at 11:00:55PM +0530, Raju Lakkaraju wrote:
> > PCI1A011/PCI1A041 chip support the MDIO Clause-45 access
> >
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
> > ---
> >  drivers/net/ethernet/microchip/lan743x_main.c | 110 +++++++++++++++++-
> >  drivers/net/ethernet/microchip/lan743x_main.h |  16 +++
> >  2 files changed, 123 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> > index 6f6655eb6438..98d346aaf627 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> > @@ -793,6 +793,95 @@ static int lan743x_mdiobus_write(struct mii_bus *bus,
> >       return ret;
> >  }
> >
> > +static u32 lan743x_mac_mmd_access(int id, int index, int op, u8 freq)
> > +{
> > +     u16 dev_addr;
> > +     u32 ret;
> > +
> > +     dev_addr = (index >> 16) & 0x1f;
> > +     ret = (id << MAC_MII_ACC_PHY_ADDR_SHIFT_) &
> > +             MAC_MII_ACC_PHY_ADDR_MASK_;
> > +     ret |= (dev_addr << MAC_MII_ACC_MIIMMD_SHIFT_) &
> > +             MAC_MII_ACC_MIIMMD_MASK_;
> > +     if (freq)
> > +             ret |= (freq << MAC_MII_ACC_MDC_CYCLE_SHIFT_) &
> > +                     MAC_MII_ACC_MDC_CYCLE_MASK_;
> 
> All callers of this function appear to pass freq as 0. So you can
> remove this.
> 

Accepted.
Yes. Currently frequency is not programming.
I will change.

> > +     if (op == 1)
> > +             ret |= MAC_MII_ACC_MIICMD_WRITE_;
> > +     else if (op == 2)
> > +             ret |= MAC_MII_ACC_MIICMD_READ_;
> > +     else if (op == 3)
> > +             ret |= MAC_MII_ACC_MIICMD_READ_INC_;
> > +     else
> > +             ret |= MAC_MII_ACC_MIICMD_ADDR_;
> 
> > +             mmd_access = lan743x_mac_mmd_access(phy_id, index, 0, 0);
> 
> It is pretty opaque what the 0 means here. How about you actually pass
> MAC_MII_ACC_MIICMD_ values?
> 
> lan743x_mac_mmd_access(phy_id, index, );
> 

Accepted. I will change

> > +             if (adapter->mdiobus->probe_capabilities == MDIOBUS_C45)
> > +                     phydev->c45_ids.devices_in_package &= ~BIT(0);
> >       }
> 
> A MAC driver should not be modifying the phydev structure.
> 

Accepted. I will remove this change.

> >       /* MAC doesn't support 1000T Half */
> > @@ -2822,12 +2914,14 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
> >                       sgmii_ctl &= ~SGMII_CTL_SGMII_POWER_DN_;
> >                       lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
> >                       netif_info(adapter, drv, adapter->netdev, "SGMII operation\n");
> > +                     adapter->mdiobus->probe_capabilities = MDIOBUS_C22_C45;
> >               } else {
> >                       sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
> >                       sgmii_ctl &= ~SGMII_CTL_SGMII_ENABLE_;
> >                       sgmii_ctl |= SGMII_CTL_SGMII_POWER_DN_;
> >                       lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
> >                       netif_info(adapter, drv, adapter->netdev, "GMII operation\n");
> > +                     adapter->mdiobus->probe_capabilities = MDIOBUS_C22;
> >               }
> >       } else {
> >               chip_ver = lan743x_csr_read(adapter, STRAP_READ);
> > @@ -2839,19 +2933,29 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
> >                       sgmii_ctl &= ~SGMII_CTL_SGMII_POWER_DN_;
> >                       lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
> >                       netif_info(adapter, drv, adapter->netdev, "SGMII operation\n");
> > +                     adapter->mdiobus->probe_capabilities = MDIOBUS_C22_C45;
> >               } else {
> >                       sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
> >                       sgmii_ctl &= ~SGMII_CTL_SGMII_ENABLE_;
> >                       sgmii_ctl |= SGMII_CTL_SGMII_POWER_DN_;
> >                       lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
> >                       netif_info(adapter, drv, adapter->netdev, "GMII operation\n");
> > +                     adapter->mdiobus->probe_capabilities = MDIOBUS_C22;
> 
> This manipulation of adapter->mdiobus->probe_capabilities based on
> SGMII vs RGMII makes no sense. It should be set based on what the bus
> master can actually do. I assume the PCI1A011/PCI1A041 can do both C22
> and C45. So it should look at the reg value and either do a C45
> transaction, or a C22 transaction. Do the older chips not support C45?
> In that case, return -EOPNOTSUPP if asked to do a C45 transaction.
> 

Yes, Older chip does not suuport C45.
I will change code such that without upate the
"adapter->mdiobus->probe_capabilities" variable, assign the read/write
functions based chip id.

>         Andrew

-- 

Thanks,
Raju