On 1/22/20 11:38 PM, Madalin Bucur (OSS) wrote:

quoted

-----Original Message-----
From: Florian Fainelli <f.fainelli@gmail.com>
Sent: Wednesday, January 22, 2020 7:58 PM
To: Madalin Bucur (OSS) <redacted>; davem@davemloft.net
Cc: andrew@lunn.ch; hkallweit1@gmail.com; netdev@vger.kernel.org;
ykaukab@suse.de
Subject: Re: [PATCH net-next 1/2] net: phy: aquantia: add rate_adaptation
indication

On 1/22/20 5:59 AM, Madalin Bucur wrote:

quoted

The AQR PHYs are able to perform rate adaptation between
the system interface and the line interfaces. When such
a PHY is deployed, the ethernet driver should not limit
the modes supported or advertised by the PHY. This patch
introduces the bit that allows checking for this feature
in the phy_device structure and its use for the Aquantia
PHYs.

Signed-off-by: Madalin Bucur <redacted>
---
 drivers/net/phy/aquantia_main.c | 3 +++
 include/linux/phy.h             | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/phy/aquantia_main.c

b/drivers/net/phy/aquantia_main.c

quoted

index 975789d9349d..36fdd523b758 100644

--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c

@@ -209,6 +209,9 @@ static int aqr_config_aneg(struct phy_device

*phydev)

quoted

 	u16 reg;
 	int ret;

+	/* add here as this is called for all devices */
+	phydev->rate_adaptation = 1;

How about introducing a new PHY_SUPPORTS_RATE_ADAPTATION flag and you
set that directly from the phy_driver entry? using the "flags" bitmask
instead of adding another structure member to phy_device?

I've looked at the phydev->dev_flags use, it seemed to me that mostly it
is used to convey configuration options towards the PHY.

You read me incorrectly, I am suggesting using the phy_driver::flags
member, not the phy_device::dev_flags entry, please re-consider your
position.

Another problem
is that it's meaning seems to be opaque,  PHY specific. I wanted to avoid
trampling on a certain PHY hardcoded value and I turned my attention to
the bit fields in the phy_device. I noticed that there are already 12 bits
so due to alignment, the added bit is not adding extra size to the struct.
 

quoted

quoted

+
 	if (phydev->autoneg == AUTONEG_DISABLE)
 		return genphy_c45_pma_setup_forced(phydev);

diff --git a/include/linux/phy.h b/include/linux/phy.h
index dd4a91f1feaa..2a5c202333fc 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -387,6 +387,9 @@ struct phy_device {
 	/* Interrupts are enabled */
 	unsigned interrupts:1;

+	/* Rate adaptation in the PHY */
+	unsigned rate_adaptation:1;
+
 	enum phy_state state;

 	u32 dev_flags;

--
Florian

-- 
Florian