| Message ID | d1f1160c-ebea-0e7b-4d73-a27ebbd5c199@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: aquantia: extend aqr_read_status | expand |
On Mon, Feb 04, 2019 at 10:03:21PM +0100, Heiner Kallweit wrote: > Add support for speeds 10Mbps, 5Gbps, and 10Gbps. In addition don't > hardcode duplex but read it from the chip. Hi Heiner The marvell10g does this differently. It gets the local and link partner advertised link modes and from that works out what the PHY is doing. If auto-neg is not being used, it then reads the link speed from the PMA. The question is, should the Aquantia PHY do the same, or should it look an vendor registers? Apart from getting the 1G advertisement, all the Marvell code uses generic registers. So we should be able to move most of it into phy-c45 and reuse it. That is what i would prefer. Andrew
On 04.02.2019 22:28, Andrew Lunn wrote: > On Mon, Feb 04, 2019 at 10:03:21PM +0100, Heiner Kallweit wrote: >> Add support for speeds 10Mbps, 5Gbps, and 10Gbps. In addition don't >> hardcode duplex but read it from the chip. > > Hi Heiner > > The marvell10g does this differently. It gets the local and link > partner advertised link modes and from that works out what the PHY is > doing. If auto-neg is not being used, it then reads the link speed > from the PMA. > Right, it's the same mechanism we use in genphy_read_status() for clause 22. > The question is, should the Aquantia PHY do the same, or should it > look an vendor registers? Apart from getting the 1G advertisement, all > the Marvell code uses generic registers. So we should be able to move > most of it into phy-c45 and reuse it. That is what i would prefer. > I'd like to use standard registers wherever possible. This patch is meant as a quick win to improve what we do already in aqr_read_status. Once we have a generic c45 read_status function we should switch to it. However I assume that information like interface mode we still have to read from vendor registers. > Andrew > Heiner
> I'd like to use standard registers wherever possible. This patch is > meant as a quick win to improve what we do already in aqr_read_status. > Once we have a generic c45 read_status function we should switch to it. Hi Heiner I don't see much point in adding code which we know we are soon going to replace. Just replace it. > However I assume that information like interface mode we still have > to read from vendor registers. For the Aquantia PHY, yes. It appears the Marvell PHY does not have any registers which indicate this, so it uses heuristics based on the link speed. Andrew
On 04.02.2019 23:23, Andrew Lunn wrote: >> I'd like to use standard registers wherever possible. This patch is >> meant as a quick win to improve what we do already in aqr_read_status. >> Once we have a generic c45 read_status function we should switch to it. > > Hi Heiner > > I don't see much point in adding code which we know we are soon going > to replace. Just replace it. > OK, let me have a closer look at the other patches you sent me. To test them I need to get my DTU running first. And I need to check what happens if certain standard registers don't report what they should and how to deal with this. E.g. the AQCS109 according to the datasheet reports in the speed ability register that it is 10G-capable, what it is not. >> However I assume that information like interface mode we still have >> to read from vendor registers. > > For the Aquantia PHY, yes. It appears the Marvell PHY does not have > any registers which indicate this, so it uses heuristics based on the > link speed. > > Andrew > Heiner
diff --git a/drivers/net/phy/aquantia.c b/drivers/net/phy/aquantia.c index 482004efa..51ae3feea 100644 --- a/drivers/net/phy/aquantia.c +++ b/drivers/net/phy/aquantia.c @@ -133,6 +133,12 @@ static int aqr_read_status(struct phy_device *phydev) reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_STATUS1); switch (reg & MDIO_AN_TX_VEND_STATUS1_RATE_MASK) { + case MDIO_AN_TX_VEND_STATUS1_10GBASET: + phydev->speed = SPEED_10000; + break; + case MDIO_AN_TX_VEND_STATUS1_5000BASET: + phydev->speed = SPEED_5000; + break; case MDIO_AN_TX_VEND_STATUS1_2500BASET: phydev->speed = SPEED_2500; break; @@ -142,11 +148,15 @@ static int aqr_read_status(struct phy_device *phydev) case MDIO_AN_TX_VEND_STATUS1_100BASETX: phydev->speed = SPEED_100; break; + case MDIO_AN_TX_VEND_STATUS1_10BASET: + phydev->speed = SPEED_10; + break; default: - phydev->speed = SPEED_10000; + phydev->speed = SPEED_UNKNOWN; break; } - phydev->duplex = DUPLEX_FULL; + + phydev->duplex = !!(reg & MDIO_AN_TX_VEND_STATUS1_FULL_DUPLEX); return 0; }