[PATCH net-next] net: phy: add flag PHY_QUIRK_NO_ESTATEN

[PATCH net-next] net: phy: add flag PHY_QUIRK_NO_ESTATEN

[Heiner Kallweit]

We have a Xilinx GBit PHY that doesn't have BMSR_ESTATEN set
(what violates the Clause 22 standard). Instead of having the PHY
driver to implement almost identical copies of few generic functions
let's add a flag for this quirk to phylib.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c |  6 +++---
 include/linux/phy.h          | 16 +++++++++++++---
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2c879ba01..7593ebebf 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1595,7 +1595,7 @@ static int genphy_config_advert(struct phy_device *phydev)
 	 * 1000Mbits/sec capable PHYs shall have the BMSR_ESTATEN bit set to a
 	 * logical 1.
 	 */
-	if (!(bmsr & BMSR_ESTATEN))
+	if (!(bmsr & BMSR_ESTATEN) && !phy_quirk_no_estaten(phydev))
 		return changed;
 
 	/* Configure gigabit if it's supported */
@@ -1919,7 +1919,7 @@ int genphy_config_init(struct phy_device *phydev)
 	if (val & BMSR_10HALF)
 		linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, features);
 
-	if (val & BMSR_ESTATEN) {
+	if (val & BMSR_ESTATEN || phy_quirk_no_estaten(phydev)) {
 		val = phy_read(phydev, MII_ESTATUS);
 		if (val < 0)
 			return val;
@@ -1972,7 +1972,7 @@ int genphy_read_abilities(struct phy_device *phydev)
 	linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT_Half_BIT, phydev->supported,
 			 val & BMSR_10HALF);
 
-	if (val & BMSR_ESTATEN) {
+	if (val & BMSR_ESTATEN || phy_quirk_no_estaten(phydev)) {
 		val = phy_read(phydev, MII_ESTATUS);
 		if (val < 0)
 			return val;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index dc4b51060..b2ffd82b1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -73,9 +73,10 @@ extern const int phy_10gbit_features_array[1];
 #define PHY_POLL		-1
 #define PHY_IGNORE_INTERRUPT	-2
 
-#define PHY_IS_INTERNAL		0x00000001
-#define PHY_RST_AFTER_CLK_EN	0x00000002
-#define MDIO_DEVICE_IS_PHY	0x80000000
+#define PHY_IS_INTERNAL		BIT(0)
+#define PHY_RST_AFTER_CLK_EN	BIT(1)
+#define PHY_QUIRK_NO_ESTATEN	BIT(2)
+#define MDIO_DEVICE_IS_PHY	BIT(31)
 
 /* Interface Mode definitions */
 typedef enum {
@@ -677,6 +678,15 @@ size_t phy_speeds(unsigned int *speeds, size_t size,
 void of_set_phy_supported(struct phy_device *phydev);
 void of_set_phy_eee_broken(struct phy_device *phydev);
 
+/**
+ * phy_quirk_no_estaten - Helper to check for flag PHY_QUIRK_NO_ESTATEN
+ * @phydev: The phy_device struct
+ */
+static inline bool phy_quirk_no_estaten(struct phy_device *phydev)
+{
+	return phydev->drv && phydev->drv->flags & PHY_QUIRK_NO_ESTATEN;
+}
+
 /**
  * phy_is_started - Convenience function to check whether PHY is started
  * @phydev: The phy_device struct
-- 
2.21.0

Re: [PATCH net-next] net: phy: add flag PHY_QUIRK_NO_ESTATEN

[Andrew Lunn]

On Tue, Jun 04, 2019 at 08:10:50AM +0200, Heiner Kallweit wrote:
> We have a Xilinx GBit PHY that doesn't have BMSR_ESTATEN set
> (what violates the Clause 22 standard). Instead of having the PHY
> driver to implement almost identical copies of few generic functions
> let's add a flag for this quirk to phylib.

Hi Heiner

It is a bit of a personal preference, but i would prefer the Xilinx
driver worked around broken hardware, not scatter quirks in the core.
Keep the core clean.

If we had multiple PHYs broken in the same way, then maybe a quirk.

	Andrew

Re: [PATCH net-next] net: phy: add flag PHY_QUIRK_NO_ESTATEN

[Heiner Kallweit]

On 04.06.2019 15:00, Andrew Lunn wrote:
> On Tue, Jun 04, 2019 at 08:10:50AM +0200, Heiner Kallweit wrote:
>> We have a Xilinx GBit PHY that doesn't have BMSR_ESTATEN set
>> (what violates the Clause 22 standard). Instead of having the PHY
>> driver to implement almost identical copies of few generic functions
>> let's add a flag for this quirk to phylib.
> 
> Hi Heiner
> 
> It is a bit of a personal preference, but i would prefer the Xilinx
> driver worked around broken hardware, not scatter quirks in the core.
> Keep the core clean.
> 
> If we had multiple PHYs broken in the same way, then maybe a quirk.
> 
Yes, I was expecting that there may be different opinions on whether
this should be handled in the core or by the driver.
But this is obsolete now anyway after Robert figured out that his
problem actually was another one.

> 	Andrew
> 
Heiner