[PATCH net-next v2 0/9] net: phy: icplus: cleanups and new features

[PATCH net-next v2 0/9] net: phy: icplus: cleanups and new features

[Michael Walle]

Cleanup the PHY drivers for IPplus devices and add PHY counters and MDIX
support for the IP101A/G.

Patch 5 adds a model detection based on the behavior of the PHY.
Unfortunately, the IP101A shares the PHY ID with the IP101G. But the latter
provides more features. Try to detect the newer model by accessing the page
selection register. If it is writeable, it is assumed, that it is a IP101G.

With this detection in place, we can now access registers >= 16 in a
correct way on the IP101G; that is by first selecting the correct page.
This might previouly worked, because no one ever set another active page
before booting linux.

The last two patches add the new features.

Michael Walle (9):
  net: phy: icplus: use PHY_ID_MATCH_MODEL() macro
  net: phy: icplus: use PHY_ID_MATCH_EXACT() for IP101A/G
  net: phy: icplus: drop address operator for functions
  net: phy: icplus: use the .soft_reset() of the phy-core
  net: phy: icplus: split IP101A/G driver
  net: phy: icplus: don't set APS_EN bit on IP101G
  net: phy: icplus: fix paged register access
  net: phy: icplus: add PHY counter for IP101G
  net: phy: icplus: add MDI/MDIX support for IP101A/G

 drivers/net/phy/icplus.c | 378 ++++++++++++++++++++++++++++++++-------
 1 file changed, 317 insertions(+), 61 deletions(-)

-- 
2.20.1

[PATCH net-next v3 1/9] net: phy: icplus: use PHY_ID_MATCH_MODEL() macro

[Michael Walle]

Simpify the initializations of the structures. There is no functional
change.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
Changes since v2:
 - none

Changes since v1:
 - none

 drivers/net/phy/icplus.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index b632947cbcdf..4407b1eb1a3d 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -47,6 +47,10 @@ MODULE_LICENSE("GPL");
 #define IP101G_DIGITAL_IO_SPEC_CTRL			0x1d
 #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32		BIT(2)
 
+#define IP175C_PHY_ID 0x02430d80
+#define IP1001_PHY_ID 0x02430d90
+#define IP101A_PHY_ID 0x02430c54
+
 /* The 32-pin IP101GR package can re-configure the mode of the RXER/INTR_32 pin
  * (pin number 21). The hardware default is RXER (receive error) mode. But it
  * can be configured to interrupt mode manually.
@@ -329,9 +333,8 @@ static irqreturn_t ip101a_g_handle_interrupt(struct phy_device *phydev)
 
 static struct phy_driver icplus_driver[] = {
 {
-	.phy_id		= 0x02430d80,
+	PHY_ID_MATCH_MODEL(IP175C_PHY_ID),
 	.name		= "ICPlus IP175C",
-	.phy_id_mask	= 0x0ffffff0,
 	/* PHY_BASIC_FEATURES */
 	.config_init	= &ip175c_config_init,
 	.config_aneg	= &ip175c_config_aneg,
@@ -339,17 +342,15 @@ static struct phy_driver icplus_driver[] = {
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
-	.phy_id		= 0x02430d90,
+	PHY_ID_MATCH_MODEL(IP1001_PHY_ID),
 	.name		= "ICPlus IP1001",
-	.phy_id_mask	= 0x0ffffff0,
 	/* PHY_GBIT_FEATURES */
 	.config_init	= &ip1001_config_init,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
-	.phy_id		= 0x02430c54,
+	PHY_ID_MATCH_MODEL(IP101A_PHY_ID),
 	.name		= "ICPlus IP101A/G",
-	.phy_id_mask	= 0x0ffffff0,
 	/* PHY_BASIC_FEATURES */
 	.probe		= ip101a_g_probe,
 	.config_intr	= ip101a_g_config_intr,
@@ -362,9 +363,9 @@ static struct phy_driver icplus_driver[] = {
 module_phy_driver(icplus_driver);
 
 static struct mdio_device_id __maybe_unused icplus_tbl[] = {
-	{ 0x02430d80, 0x0ffffff0 },
-	{ 0x02430d90, 0x0ffffff0 },
-	{ 0x02430c54, 0x0ffffff0 },
+	{ PHY_ID_MATCH_MODEL(IP175C_PHY_ID) },
+	{ PHY_ID_MATCH_MODEL(IP1001_PHY_ID) },
+	{ PHY_ID_MATCH_MODEL(IP101A_PHY_ID) },
 	{ }
 };
 
-- 
2.20.1

[PATCH net-next v3 2/9] net: phy: icplus: use PHY_ID_MATCH_EXACT() for IP101A/G

[Michael Walle]

According to the datasheet of the IP101A/G there is no revision field
and MII_PHYSID2 always reads as 0x0c54. Use PHY_ID_MATCH_EXACT() then.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
Changes since v2:
 - none

Changes since v1:
 - none

 drivers/net/phy/icplus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 4407b1eb1a3d..ae3cf61c5ac2 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -349,7 +349,7 @@ static struct phy_driver icplus_driver[] = {
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
-	PHY_ID_MATCH_MODEL(IP101A_PHY_ID),
+	PHY_ID_MATCH_EXACT(IP101A_PHY_ID),
 	.name		= "ICPlus IP101A/G",
 	/* PHY_BASIC_FEATURES */
 	.probe		= ip101a_g_probe,
@@ -365,7 +365,7 @@ module_phy_driver(icplus_driver);
 static struct mdio_device_id __maybe_unused icplus_tbl[] = {
 	{ PHY_ID_MATCH_MODEL(IP175C_PHY_ID) },
 	{ PHY_ID_MATCH_MODEL(IP1001_PHY_ID) },
-	{ PHY_ID_MATCH_MODEL(IP101A_PHY_ID) },
+	{ PHY_ID_MATCH_EXACT(IP101A_PHY_ID) },
 	{ }
 };
 
-- 
2.20.1

[PATCH net-next v3 3/9] net: phy: icplus: drop address operator for functions

[Michael Walle]

Don't sometimes use the address operator and sometimes not. Drop it and
make the code look uniform.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
Changes since v2:
 - none

Changes since v1:
 - none

 drivers/net/phy/icplus.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index ae3cf61c5ac2..43b69addc0ce 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -336,16 +336,16 @@ static struct phy_driver icplus_driver[] = {
 	PHY_ID_MATCH_MODEL(IP175C_PHY_ID),
 	.name		= "ICPlus IP175C",
 	/* PHY_BASIC_FEATURES */
-	.config_init	= &ip175c_config_init,
-	.config_aneg	= &ip175c_config_aneg,
-	.read_status	= &ip175c_read_status,
+	.config_init	= ip175c_config_init,
+	.config_aneg	= ip175c_config_aneg,
+	.read_status	= ip175c_read_status,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
 	PHY_ID_MATCH_MODEL(IP1001_PHY_ID),
 	.name		= "ICPlus IP1001",
 	/* PHY_GBIT_FEATURES */
-	.config_init	= &ip1001_config_init,
+	.config_init	= ip1001_config_init,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
@@ -355,7 +355,7 @@ static struct phy_driver icplus_driver[] = {
 	.probe		= ip101a_g_probe,
 	.config_intr	= ip101a_g_config_intr,
 	.handle_interrupt = ip101a_g_handle_interrupt,
-	.config_init	= &ip101a_g_config_init,
+	.config_init	= ip101a_g_config_init,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 } };
-- 
2.20.1

[PATCH net-next v3 4/9] net: phy: icplus: use the .soft_reset() of the phy-core

[Michael Walle]

The PHY core already resets the PHY before .config_init() if a
.soft_reset() op is registered. Drop the open-coded ip1xx_reset().

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
Changes since v2:
 - none

Changes since v1:
 - none

 drivers/net/phy/icplus.c | 32 ++------------------------------
 1 file changed, 2 insertions(+), 30 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 43b69addc0ce..036bac628b11 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -120,36 +120,10 @@ static int ip175c_config_init(struct phy_device *phydev)
 	return 0;
 }
 
-static int ip1xx_reset(struct phy_device *phydev)
-{
-	int bmcr;
-
-	/* Software Reset PHY */
-	bmcr = phy_read(phydev, MII_BMCR);
-	if (bmcr < 0)
-		return bmcr;
-	bmcr |= BMCR_RESET;
-	bmcr = phy_write(phydev, MII_BMCR, bmcr);
-	if (bmcr < 0)
-		return bmcr;
-
-	do {
-		bmcr = phy_read(phydev, MII_BMCR);
-		if (bmcr < 0)
-			return bmcr;
-	} while (bmcr & BMCR_RESET);
-
-	return 0;
-}
-
 static int ip1001_config_init(struct phy_device *phydev)
 {
 	int c;
 
-	c = ip1xx_reset(phydev);
-	if (c < 0)
-		return c;
-
 	/* Enable Auto Power Saving mode */
 	c = phy_read(phydev, IP1001_SPEC_CTRL_STATUS_2);
 	if (c < 0)
@@ -237,10 +211,6 @@ static int ip101a_g_config_init(struct phy_device *phydev)
 	struct ip101a_g_phy_priv *priv = phydev->priv;
 	int err, c;
 
-	c = ip1xx_reset(phydev);
-	if (c < 0)
-		return c;
-
 	/* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */
 	switch (priv->sel_intr32) {
 	case IP101GR_SEL_INTR32_RXER:
@@ -346,6 +316,7 @@ static struct phy_driver icplus_driver[] = {
 	.name		= "ICPlus IP1001",
 	/* PHY_GBIT_FEATURES */
 	.config_init	= ip1001_config_init,
+	.soft_reset	= genphy_soft_reset,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
@@ -356,6 +327,7 @@ static struct phy_driver icplus_driver[] = {
 	.config_intr	= ip101a_g_config_intr,
 	.handle_interrupt = ip101a_g_handle_interrupt,
 	.config_init	= ip101a_g_config_init,
+	.soft_reset	= genphy_soft_reset,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 } };
-- 
2.20.1

[PATCH net-next v3 5/9] net: phy: icplus: split IP101A/G driver

[Michael Walle]

Unfortunately, the IP101A and IP101G share the same PHY identifier.
While most of the functions are somewhat backwards compatible, there is
for example the APS_EN bit on the IP101A but on the IP101G this bit
reserved. Also, the IP101G has many more functionalities.

Deduce the model by accessing the page select register which - according
to the datasheet - is not available on the IP101A. If this register is
writable, assume we have an IP101G.

Split the combined IP101A/G driver into two separate drivers.

Signed-off-by: Michael Walle <michael@walle.cc>
---
Changes since v2:
 - dropped the PHY_BASIC_FEATURES comments as suggested by Heiner
 - converted the ternary operator to a simple comparison as suggested by
   Heiner

Changes since v1:
 - use match_phy_device() as suggested by Heiner

Andrew, I've dropped your Reviewed-by because of this.

 drivers/net/phy/icplus.c | 69 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 036bac628b11..dee4f4d988a2 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -44,6 +44,8 @@ MODULE_LICENSE("GPL");
 #define IP101A_G_IRQ_DUPLEX_CHANGE	BIT(1)
 #define IP101A_G_IRQ_LINK_CHANGE	BIT(0)
 
+#define IP101G_PAGE_CONTROL				0x14
+#define IP101G_PAGE_CONTROL_MASK			GENMASK(4, 0)
 #define IP101G_DIGITAL_IO_SPEC_CTRL			0x1d
 #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32		BIT(2)
 
@@ -301,6 +303,58 @@ static irqreturn_t ip101a_g_handle_interrupt(struct phy_device *phydev)
 	return IRQ_HANDLED;
 }
 
+static int ip101a_g_has_page_register(struct phy_device *phydev)
+{
+	int oldval, val, ret;
+
+	oldval = phy_read(phydev, IP101G_PAGE_CONTROL);
+	if (oldval < 0)
+		return oldval;
+
+	ret = phy_write(phydev, IP101G_PAGE_CONTROL, 0xffff);
+	if (ret)
+		return ret;
+
+	val = phy_read(phydev, IP101G_PAGE_CONTROL);
+	if (val < 0)
+		return val;
+
+	ret = phy_write(phydev, IP101G_PAGE_CONTROL, oldval);
+	if (ret)
+		return ret;
+
+	return val == IP101G_PAGE_CONTROL_MASK;
+}
+
+static int ip101a_g_match_phy_device(struct phy_device *phydev, bool ip101a)
+{
+	int ret;
+
+	if (phydev->phy_id != IP101A_PHY_ID)
+		return 0;
+
+	/* The IP101A and the IP101G share the same PHY identifier.The IP101G
+	 * seems to be a successor of the IP101A and implements more functions.
+	 * Amongst other things there is a page select register, which is not
+	 * available on the IP101A. Use this to distinguish these two.
+	 */
+	ret = ip101a_g_has_page_register(phydev);
+	if (ret < 0)
+		return ret;
+
+	return ip101a == !ret;
+}
+
+static int ip101a_match_phy_device(struct phy_device *phydev)
+{
+	return ip101a_g_match_phy_device(phydev, true);
+}
+
+static int ip101g_match_phy_device(struct phy_device *phydev)
+{
+	return ip101a_g_match_phy_device(phydev, false);
+}
+
 static struct phy_driver icplus_driver[] = {
 {
 	PHY_ID_MATCH_MODEL(IP175C_PHY_ID),
@@ -320,9 +374,18 @@ static struct phy_driver icplus_driver[] = {
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 }, {
-	PHY_ID_MATCH_EXACT(IP101A_PHY_ID),
-	.name		= "ICPlus IP101A/G",
-	/* PHY_BASIC_FEATURES */
+	.name		= "ICPlus IP101A",
+	.match_phy_device = ip101a_match_phy_device,
+	.probe		= ip101a_g_probe,
+	.config_intr	= ip101a_g_config_intr,
+	.handle_interrupt = ip101a_g_handle_interrupt,
+	.config_init	= ip101a_g_config_init,
+	.soft_reset	= genphy_soft_reset,
+	.suspend	= genphy_suspend,
+	.resume		= genphy_resume,
+}, {
+	.name		= "ICPlus IP101G",
+	.match_phy_device = ip101g_match_phy_device,
 	.probe		= ip101a_g_probe,
 	.config_intr	= ip101a_g_config_intr,
 	.handle_interrupt = ip101a_g_handle_interrupt,
-- 
2.20.1

[PATCH net-next v3 6/9] net: phy: icplus: don't set APS_EN bit on IP101G

[Michael Walle]

This bit is reserved as 'always-write-1'. While this is not a particular
error, because we are only setting it, guard it by checking the model to
prevent errors in the future.

Signed-off-by: Michael Walle <michael@walle.cc>
---
Changes since v2:
 - none

Changes since v1:
 - dropped the model check. Instead use two different functions.

Andrew, I've dropped your Reviewed-by because of this.

 drivers/net/phy/icplus.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index dee4f4d988a2..bc2b58061507 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -208,10 +208,10 @@ static int ip101a_g_probe(struct phy_device *phydev)
 	return 0;
 }
 
-static int ip101a_g_config_init(struct phy_device *phydev)
+static int ip101a_g_config_intr_pin(struct phy_device *phydev)
 {
 	struct ip101a_g_phy_priv *priv = phydev->priv;
-	int err, c;
+	int err;
 
 	/* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */
 	switch (priv->sel_intr32) {
@@ -241,11 +241,24 @@ static int ip101a_g_config_init(struct phy_device *phydev)
 		break;
 	}
 
+	return 0;
+}
+
+static int ip101a_config_init(struct phy_device *phydev)
+{
+	int ret;
+
 	/* Enable Auto Power Saving mode */
-	c = phy_read(phydev, IP10XX_SPEC_CTRL_STATUS);
-	c |= IP101A_G_APS_ON;
+	ret = phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, IP101A_G_APS_ON);
+	if (ret)
+		return ret;
 
-	return phy_write(phydev, IP10XX_SPEC_CTRL_STATUS, c);
+	return ip101a_g_config_intr_pin(phydev);
+}
+
+static int ip101g_config_init(struct phy_device *phydev)
+{
+	return ip101a_g_config_intr_pin(phydev);
 }
 
 static int ip101a_g_ack_interrupt(struct phy_device *phydev)
@@ -379,7 +392,7 @@ static struct phy_driver icplus_driver[] = {
 	.probe		= ip101a_g_probe,
 	.config_intr	= ip101a_g_config_intr,
 	.handle_interrupt = ip101a_g_handle_interrupt,
-	.config_init	= ip101a_g_config_init,
+	.config_init	= ip101a_config_init,
 	.soft_reset	= genphy_soft_reset,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
@@ -389,7 +402,7 @@ static struct phy_driver icplus_driver[] = {
 	.probe		= ip101a_g_probe,
 	.config_intr	= ip101a_g_config_intr,
 	.handle_interrupt = ip101a_g_handle_interrupt,
-	.config_init	= ip101a_g_config_init,
+	.config_init	= ip101g_config_init,
 	.soft_reset	= genphy_soft_reset,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
-- 
2.20.1

Re: [PATCH net-next v3 6/9] net: phy: icplus: don't set APS_EN bit on IP101G

[Andrew Lunn]

On Wed, Feb 10, 2021 at 10:08:06PM +0100, Michael Walle wrote:
> This bit is reserved as 'always-write-1'. While this is not a particular
> error, because we are only setting it, guard it by checking the model to
> prevent errors in the future.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next v3 7/9] net: phy: icplus: fix paged register access

[Michael Walle]

Registers >= 16 are paged. Be sure to set the page. It seems this was
working for now, because the default is correct for the registers used
in the driver at the moment. But this will also assume, nobody will
change the page select register before linux is started. The page select
register is _not_ reset with a soft reset of the PHY.

To ease the function reuse between the non-paged register space of the
IP101A and the IP101G, add noop read_page()/write_page() callbacks so
the IP101G functions can also be used for the IP101A.

Signed-off-by: Michael Walle <michael@walle.cc>
---
Changes since v2:
 - none

Changes since v1:
 - introduce a noop read/write_page() for the IP101A
 - also use phy_*_paged() for the interrupt status register

Andrew, I've dropped your Reviewed-by because of this.

 drivers/net/phy/icplus.c | 65 ++++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index bc2b58061507..7e0ef05b1cae 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -49,6 +49,8 @@ MODULE_LICENSE("GPL");
 #define IP101G_DIGITAL_IO_SPEC_CTRL			0x1d
 #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32		BIT(2)
 
+#define IP101G_DEFAULT_PAGE			16
+
 #define IP175C_PHY_ID 0x02430d80
 #define IP1001_PHY_ID 0x02430d90
 #define IP101A_PHY_ID 0x02430c54
@@ -211,23 +213,25 @@ static int ip101a_g_probe(struct phy_device *phydev)
 static int ip101a_g_config_intr_pin(struct phy_device *phydev)
 {
 	struct ip101a_g_phy_priv *priv = phydev->priv;
-	int err;
+	int oldpage, err;
+
+	oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);
 
 	/* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */
 	switch (priv->sel_intr32) {
 	case IP101GR_SEL_INTR32_RXER:
-		err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
-				 IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
+		err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
+				   IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
 		if (err < 0)
-			return err;
+			goto out;
 		break;
 
 	case IP101GR_SEL_INTR32_INTR:
-		err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
-				 IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
-				 IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
+		err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
+				   IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
+				   IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
 		if (err < 0)
-			return err;
+			goto out;
 		break;
 
 	default:
@@ -241,7 +245,8 @@ static int ip101a_g_config_intr_pin(struct phy_device *phydev)
 		break;
 	}
 
-	return 0;
+out:
+	return phy_restore_page(phydev, oldpage, err);
 }
 
 static int ip101a_config_init(struct phy_device *phydev)
@@ -263,8 +268,10 @@ static int ip101g_config_init(struct phy_device *phydev)
 
 static int ip101a_g_ack_interrupt(struct phy_device *phydev)
 {
-	int err = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);
+	int err;
 
+	err = phy_read_paged(phydev, IP101G_DEFAULT_PAGE,
+			     IP101A_G_IRQ_CONF_STATUS);
 	if (err < 0)
 		return err;
 
@@ -283,10 +290,12 @@ static int ip101a_g_config_intr(struct phy_device *phydev)
 
 		/* INTR pin used: Speed/link/duplex will cause an interrupt */
 		val = IP101A_G_IRQ_PIN_USED;
-		err = phy_write(phydev, IP101A_G_IRQ_CONF_STATUS, val);
+		err = phy_write_paged(phydev, IP101G_DEFAULT_PAGE,
+				      IP101A_G_IRQ_CONF_STATUS, val);
 	} else {
 		val = IP101A_G_IRQ_ALL_MASK;
-		err = phy_write(phydev, IP101A_G_IRQ_CONF_STATUS, val);
+		err = phy_write_paged(phydev, IP101G_DEFAULT_PAGE,
+				      IP101A_G_IRQ_CONF_STATUS, val);
 		if (err)
 			return err;
 
@@ -300,7 +309,8 @@ static irqreturn_t ip101a_g_handle_interrupt(struct phy_device *phydev)
 {
 	int irq_status;
 
-	irq_status = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);
+	irq_status = phy_read_paged(phydev, IP101G_DEFAULT_PAGE,
+				    IP101A_G_IRQ_CONF_STATUS);
 	if (irq_status < 0) {
 		phy_error(phydev);
 		return IRQ_NONE;
@@ -316,6 +326,31 @@ static irqreturn_t ip101a_g_handle_interrupt(struct phy_device *phydev)
 	return IRQ_HANDLED;
 }
 
+/* The IP101A doesn't really have a page register. We just pretend to have one
+ * so we can use the paged versions of the callbacks of the IP101G.
+ */
+static int ip101a_read_page(struct phy_device *phydev)
+{
+	return IP101G_DEFAULT_PAGE;
+}
+
+static int ip101a_write_page(struct phy_device *phydev, int page)
+{
+	WARN_ONCE(page != IP101G_DEFAULT_PAGE, "wrong page selected\n");
+
+	return 0;
+}
+
+static int ip101g_read_page(struct phy_device *phydev)
+{
+	return __phy_read(phydev, IP101G_PAGE_CONTROL);
+}
+
+static int ip101g_write_page(struct phy_device *phydev, int page)
+{
+	return __phy_write(phydev, IP101G_PAGE_CONTROL, page);
+}
+
 static int ip101a_g_has_page_register(struct phy_device *phydev)
 {
 	int oldval, val, ret;
@@ -390,6 +425,8 @@ static struct phy_driver icplus_driver[] = {
 	.name		= "ICPlus IP101A",
 	.match_phy_device = ip101a_match_phy_device,
 	.probe		= ip101a_g_probe,
+	.read_page	= ip101a_read_page,
+	.write_page	= ip101a_write_page,
 	.config_intr	= ip101a_g_config_intr,
 	.handle_interrupt = ip101a_g_handle_interrupt,
 	.config_init	= ip101a_config_init,
@@ -400,6 +437,8 @@ static struct phy_driver icplus_driver[] = {
 	.name		= "ICPlus IP101G",
 	.match_phy_device = ip101g_match_phy_device,
 	.probe		= ip101a_g_probe,
+	.read_page	= ip101g_read_page,
+	.write_page	= ip101g_write_page,
 	.config_intr	= ip101a_g_config_intr,
 	.handle_interrupt = ip101a_g_handle_interrupt,
 	.config_init	= ip101g_config_init,
-- 
2.20.1

Re: [PATCH net-next v3 7/9] net: phy: icplus: fix paged register access

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 11407 bytes --]

Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Michael-Walle/net-phy-icplus-cleanups-and-new-features/20210211-051702
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git de1db4a6ed6241e34cab0e5059d4b56f6bae39b9
config: powerpc64-randconfig-r004-20210211 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c9439ca36342fb6013187d0a69aef92736951476)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc64 cross compiling tool for clang build
        # apt-get install binutils-powerpc64-linux-gnu
        # https://github.com/0day-ci/linux/commit/07b7c444040f9baff7b28415b4f26be7e7a71e2e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Michael-Walle/net-phy-icplus-cleanups-and-new-features/20210211-051702
        git checkout 07b7c444040f9baff7b28415b4f26be7e7a71e2e
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   __do_insb
   ^
   arch/powerpc/include/asm/io.h:556:56: note: expanded from macro '__do_insb'
   #define __do_insb(p, b, n)      readsb((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/net/phy/icplus.c:11:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:10:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:139:1: note: expanded from here
   __do_insw
   ^
   arch/powerpc/include/asm/io.h:557:56: note: expanded from macro '__do_insw'
   #define __do_insw(p, b, n)      readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/net/phy/icplus.c:11:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:10:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:141:1: note: expanded from here
   __do_insl
   ^
   arch/powerpc/include/asm/io.h:558:56: note: expanded from macro '__do_insl'
   #define __do_insl(p, b, n)      readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
                                          ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/net/phy/icplus.c:11:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:10:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:143:1: note: expanded from here
   __do_outsb
   ^
   arch/powerpc/include/asm/io.h:559:58: note: expanded from macro '__do_outsb'
   #define __do_outsb(p, b, n)     writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/net/phy/icplus.c:11:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:10:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:145:1: note: expanded from here
   __do_outsw
   ^
   arch/powerpc/include/asm/io.h:560:58: note: expanded from macro '__do_outsw'
   #define __do_outsw(p, b, n)     writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
   In file included from drivers/net/phy/icplus.c:11:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:10:
   In file included from arch/powerpc/include/asm/hardirq.h:6:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/powerpc/include/asm/io.h:619:
   arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
   DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
                   __do_##name al;                                 \
                   ^~~~~~~~~~~~~~
   <scratch space>:147:1: note: expanded from here
   __do_outsl
   ^
   arch/powerpc/include/asm/io.h:561:58: note: expanded from macro '__do_outsl'
   #define __do_outsl(p, b, n)     writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
                                           ~~~~~~~~~~~~~~~~~~~~~^
>> drivers/net/phy/icplus.c:237:2: warning: variable 'err' is used uninitialized whenever switch default is taken [-Wsometimes-uninitialized]
           default:
           ^~~~~~~
   drivers/net/phy/icplus.c:249:43: note: uninitialized use occurs here
           return phy_restore_page(phydev, oldpage, err);
                                                    ^~~
   drivers/net/phy/icplus.c:216:18: note: initialize the variable 'err' to silence this warning
           int oldpage, err;
                           ^
                            = 0
   7 warnings generated.


vim +/err +237 drivers/net/phy/icplus.c

f2f1a847e74f61 Martin Blumenstingl 2018-11-18  212  
56ff94ca1f47d1 Michael Walle       2021-02-10  213  static int ip101a_g_config_intr_pin(struct phy_device *phydev)
034289b2d7cf29 Martin Blumenstingl 2018-11-18  214  {
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  215  	struct ip101a_g_phy_priv *priv = phydev->priv;
07b7c444040f9b Michael Walle       2021-02-10  216  	int oldpage, err;
07b7c444040f9b Michael Walle       2021-02-10  217  
07b7c444040f9b Michael Walle       2021-02-10  218  	oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);
034289b2d7cf29 Martin Blumenstingl 2018-11-18  219  
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  220  	/* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  221  	switch (priv->sel_intr32) {
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  222  	case IP101GR_SEL_INTR32_RXER:
07b7c444040f9b Michael Walle       2021-02-10  223  		err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  224  				   IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  225  		if (err < 0)
07b7c444040f9b Michael Walle       2021-02-10  226  			goto out;
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  227  		break;
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  228  
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  229  	case IP101GR_SEL_INTR32_INTR:
07b7c444040f9b Michael Walle       2021-02-10  230  		err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  231  				   IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  232  				   IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  233  		if (err < 0)
07b7c444040f9b Michael Walle       2021-02-10  234  			goto out;
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  235  		break;
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  236  
f2f1a847e74f61 Martin Blumenstingl 2018-11-18 @237  	default:
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  238  		/* Don't touch IP101G_DIGITAL_IO_SPEC_CTRL because it's not
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  239  		 * documented on IP101A and it's not clear whether this would
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  240  		 * cause problems.
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  241  		 * For the 32-pin IP101GR we simply keep the SEL_INTR32
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  242  		 * configuration as set by the bootloader when not configured
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  243  		 * to one of the special functions.
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  244  		 */
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  245  		break;
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  246  	}
f2f1a847e74f61 Martin Blumenstingl 2018-11-18  247  
07b7c444040f9b Michael Walle       2021-02-10  248  out:
07b7c444040f9b Michael Walle       2021-02-10  249  	return phy_restore_page(phydev, oldpage, err);
56ff94ca1f47d1 Michael Walle       2021-02-10  250  }
56ff94ca1f47d1 Michael Walle       2021-02-10  251  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36433 bytes --]

Re: [PATCH net-next v3 7/9] net: phy: icplus: fix paged register access

[Michael Walle]

Am 2021-02-10 22:08, schrieb Michael Walle:
> Registers >= 16 are paged. Be sure to set the page. It seems this was
> working for now, because the default is correct for the registers used
> in the driver at the moment. But this will also assume, nobody will
> change the page select register before linux is started. The page 
> select
> register is _not_ reset with a soft reset of the PHY.
> 
> To ease the function reuse between the non-paged register space of the
> IP101A and the IP101G, add noop read_page()/write_page() callbacks so
> the IP101G functions can also be used for the IP101A.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> Changes since v2:
>  - none
> 
> Changes since v1:
>  - introduce a noop read/write_page() for the IP101A
>  - also use phy_*_paged() for the interrupt status register
> 
> Andrew, I've dropped your Reviewed-by because of this.
> 
>  drivers/net/phy/icplus.c | 65 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 52 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
> index bc2b58061507..7e0ef05b1cae 100644
> --- a/drivers/net/phy/icplus.c
> +++ b/drivers/net/phy/icplus.c
> @@ -49,6 +49,8 @@ MODULE_LICENSE("GPL");
>  #define IP101G_DIGITAL_IO_SPEC_CTRL			0x1d
>  #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32		BIT(2)
> 
> +#define IP101G_DEFAULT_PAGE			16
> +
>  #define IP175C_PHY_ID 0x02430d80
>  #define IP1001_PHY_ID 0x02430d90
>  #define IP101A_PHY_ID 0x02430c54
> @@ -211,23 +213,25 @@ static int ip101a_g_probe(struct phy_device 
> *phydev)
>  static int ip101a_g_config_intr_pin(struct phy_device *phydev)
>  {
>  	struct ip101a_g_phy_priv *priv = phydev->priv;
> -	int err;
> +	int oldpage, err;

besides this being uninitialized

> +
> +	oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);

this is also missing a check for negative return values

will be fixed in v4

-michael

> 
>  	/* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */
>  	switch (priv->sel_intr32) {
>  	case IP101GR_SEL_INTR32_RXER:
> -		err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> -				 IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
> +		err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> +				   IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
>  		if (err < 0)
> -			return err;
> +			goto out;
>  		break;
> 
>  	case IP101GR_SEL_INTR32_INTR:
> -		err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> -				 IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
> -				 IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
> +		err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> +				   IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
> +				   IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
>  		if (err < 0)
> -			return err;
> +			goto out;
>  		break;
> 
>  	default:
> @@ -241,7 +245,8 @@ static int ip101a_g_config_intr_pin(struct
> phy_device *phydev)
>  		break;
>  	}
> 
> -	return 0;
> +out:
> +	return phy_restore_page(phydev, oldpage, err);
>  }
> 
>  static int ip101a_config_init(struct phy_device *phydev)
> @@ -263,8 +268,10 @@ static int ip101g_config_init(struct phy_device 
> *phydev)
> 
>  static int ip101a_g_ack_interrupt(struct phy_device *phydev)
>  {
> -	int err = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);
> +	int err;
> 
> +	err = phy_read_paged(phydev, IP101G_DEFAULT_PAGE,
> +			     IP101A_G_IRQ_CONF_STATUS);
>  	if (err < 0)
>  		return err;
> 
> @@ -283,10 +290,12 @@ static int ip101a_g_config_intr(struct phy_device 
> *phydev)
> 
>  		/* INTR pin used: Speed/link/duplex will cause an interrupt */
>  		val = IP101A_G_IRQ_PIN_USED;
> -		err = phy_write(phydev, IP101A_G_IRQ_CONF_STATUS, val);
> +		err = phy_write_paged(phydev, IP101G_DEFAULT_PAGE,
> +				      IP101A_G_IRQ_CONF_STATUS, val);
>  	} else {
>  		val = IP101A_G_IRQ_ALL_MASK;
> -		err = phy_write(phydev, IP101A_G_IRQ_CONF_STATUS, val);
> +		err = phy_write_paged(phydev, IP101G_DEFAULT_PAGE,
> +				      IP101A_G_IRQ_CONF_STATUS, val);
>  		if (err)
>  			return err;
> 
> @@ -300,7 +309,8 @@ static irqreturn_t
> ip101a_g_handle_interrupt(struct phy_device *phydev)
>  {
>  	int irq_status;
> 
> -	irq_status = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);
> +	irq_status = phy_read_paged(phydev, IP101G_DEFAULT_PAGE,
> +				    IP101A_G_IRQ_CONF_STATUS);
>  	if (irq_status < 0) {
>  		phy_error(phydev);
>  		return IRQ_NONE;
> @@ -316,6 +326,31 @@ static irqreturn_t
> ip101a_g_handle_interrupt(struct phy_device *phydev)
>  	return IRQ_HANDLED;
>  }
> 
> +/* The IP101A doesn't really have a page register. We just pretend to 
> have one
> + * so we can use the paged versions of the callbacks of the IP101G.
> + */
> +static int ip101a_read_page(struct phy_device *phydev)
> +{
> +	return IP101G_DEFAULT_PAGE;
> +}
> +
> +static int ip101a_write_page(struct phy_device *phydev, int page)
> +{
> +	WARN_ONCE(page != IP101G_DEFAULT_PAGE, "wrong page selected\n");
> +
> +	return 0;
> +}
> +
> +static int ip101g_read_page(struct phy_device *phydev)
> +{
> +	return __phy_read(phydev, IP101G_PAGE_CONTROL);
> +}
> +
> +static int ip101g_write_page(struct phy_device *phydev, int page)
> +{
> +	return __phy_write(phydev, IP101G_PAGE_CONTROL, page);
> +}
> +
>  static int ip101a_g_has_page_register(struct phy_device *phydev)
>  {
>  	int oldval, val, ret;
> @@ -390,6 +425,8 @@ static struct phy_driver icplus_driver[] = {
>  	.name		= "ICPlus IP101A",
>  	.match_phy_device = ip101a_match_phy_device,
>  	.probe		= ip101a_g_probe,
> +	.read_page	= ip101a_read_page,
> +	.write_page	= ip101a_write_page,
>  	.config_intr	= ip101a_g_config_intr,
>  	.handle_interrupt = ip101a_g_handle_interrupt,
>  	.config_init	= ip101a_config_init,
> @@ -400,6 +437,8 @@ static struct phy_driver icplus_driver[] = {
>  	.name		= "ICPlus IP101G",
>  	.match_phy_device = ip101g_match_phy_device,
>  	.probe		= ip101a_g_probe,
> +	.read_page	= ip101g_read_page,
> +	.write_page	= ip101g_write_page,
>  	.config_intr	= ip101a_g_config_intr,
>  	.handle_interrupt = ip101a_g_handle_interrupt,
>  	.config_init	= ip101g_config_init,

-- 
-michael

[PATCH net-next v3 8/9] net: phy: icplus: add PHY counter for IP101G

[Michael Walle]

The IP101G provides three counters: RX packets, CRC errors and symbol
errors. The error counters can be configured to clear automatically on
read. Unfortunately, this isn't true for the RX packet counter. Because
of this and because the RX packet counter is more likely to overflow,
than the error counters implement only support for the error counters.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
Changes since v2:
 - none

Changes since v1:
 - renamed the functions to represend a IP101G-only function
 - enable the counters in IP101G's config_init()

 drivers/net/phy/icplus.c | 75 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 7e0ef05b1cae..96e9d1d12992 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -51,6 +51,12 @@ MODULE_LICENSE("GPL");
 
 #define IP101G_DEFAULT_PAGE			16
 
+#define IP101G_P1_CNT_CTRL		17
+#define CNT_CTRL_RX_EN			BIT(13)
+#define IP101G_P8_CNT_CTRL		17
+#define CNT_CTRL_RDCLR_EN		BIT(15)
+#define IP101G_CNT_REG			18
+
 #define IP175C_PHY_ID 0x02430d80
 #define IP1001_PHY_ID 0x02430d90
 #define IP101A_PHY_ID 0x02430c54
@@ -65,8 +71,19 @@ enum ip101gr_sel_intr32 {
 	IP101GR_SEL_INTR32_RXER,
 };
 
+struct ip101g_hw_stat {
+	const char *name;
+	int page;
+};
+
+static struct ip101g_hw_stat ip101g_hw_stats[] = {
+	{ "phy_crc_errors", 1 },
+	{ "phy_symbol_errors", 11, },
+};
+
 struct ip101a_g_phy_priv {
 	enum ip101gr_sel_intr32 sel_intr32;
+	u64 stats[ARRAY_SIZE(ip101g_hw_stats)];
 };
 
 static int ip175c_config_init(struct phy_device *phydev)
@@ -263,6 +280,20 @@ static int ip101a_config_init(struct phy_device *phydev)
 
 static int ip101g_config_init(struct phy_device *phydev)
 {
+	int ret;
+
+	/* Enable the PHY counters */
+	ret = phy_modify_paged(phydev, 1, IP101G_P1_CNT_CTRL,
+			       CNT_CTRL_RX_EN, CNT_CTRL_RX_EN);
+	if (ret)
+		return ret;
+
+	/* Clear error counters on read */
+	ret = phy_modify_paged(phydev, 8, IP101G_P8_CNT_CTRL,
+			       CNT_CTRL_RDCLR_EN, CNT_CTRL_RDCLR_EN);
+	if (ret)
+		return ret;
+
 	return ip101a_g_config_intr_pin(phydev);
 }
 
@@ -403,6 +434,47 @@ static int ip101g_match_phy_device(struct phy_device *phydev)
 	return ip101a_g_match_phy_device(phydev, false);
 }
 
+static int ip101g_get_sset_count(struct phy_device *phydev)
+{
+	return ARRAY_SIZE(ip101g_hw_stats);
+}
+
+static void ip101g_get_strings(struct phy_device *phydev, u8 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ip101g_hw_stats); i++)
+		strscpy(data + i * ETH_GSTRING_LEN,
+			ip101g_hw_stats[i].name, ETH_GSTRING_LEN);
+}
+
+static u64 ip101g_get_stat(struct phy_device *phydev, int i)
+{
+	struct ip101g_hw_stat stat = ip101g_hw_stats[i];
+	struct ip101a_g_phy_priv *priv = phydev->priv;
+	int val;
+	u64 ret;
+
+	val = phy_read_paged(phydev, stat.page, IP101G_CNT_REG);
+	if (val < 0) {
+		ret = U64_MAX;
+	} else {
+		priv->stats[i] += val;
+		ret = priv->stats[i];
+	}
+
+	return ret;
+}
+
+static void ip101g_get_stats(struct phy_device *phydev,
+			     struct ethtool_stats *stats, u64 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ip101g_hw_stats); i++)
+		data[i] = ip101g_get_stat(phydev, i);
+}
+
 static struct phy_driver icplus_driver[] = {
 {
 	PHY_ID_MATCH_MODEL(IP175C_PHY_ID),
@@ -443,6 +515,9 @@ static struct phy_driver icplus_driver[] = {
 	.handle_interrupt = ip101a_g_handle_interrupt,
 	.config_init	= ip101g_config_init,
 	.soft_reset	= genphy_soft_reset,
+	.get_sset_count = ip101g_get_sset_count,
+	.get_strings	= ip101g_get_strings,
+	.get_stats	= ip101g_get_stats,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 } };
-- 
2.20.1

[PATCH net-next v3 9/9] net: phy: icplus: add MDI/MDIX support for IP101A/G

[Michael Walle]

Implement the operations to set desired mode and retrieve the current
mode.

This feature was tested with an IP101G.

Signed-off-by: Michael Walle <michael@walle.cc>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
Changes since v2:
 - none

Changes since v1:
 - none, except that the callbacks are register for both IP101A and IP101G
   PHY drivers

 drivers/net/phy/icplus.c | 93 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 96e9d1d12992..cc5b25714002 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -37,12 +37,17 @@ MODULE_LICENSE("GPL");
 #define IP1001_SPEC_CTRL_STATUS_2	20	/* IP1001 Spec. Control Reg 2 */
 #define IP1001_APS_ON			11	/* IP1001 APS Mode  bit */
 #define IP101A_G_APS_ON			BIT(1)	/* IP101A/G APS Mode bit */
+#define IP101A_G_AUTO_MDIX_DIS		BIT(11)
 #define IP101A_G_IRQ_CONF_STATUS	0x11	/* Conf Info IRQ & Status Reg */
 #define	IP101A_G_IRQ_PIN_USED		BIT(15) /* INTR pin used */
 #define IP101A_G_IRQ_ALL_MASK		BIT(11) /* IRQ's inactive */
 #define IP101A_G_IRQ_SPEED_CHANGE	BIT(2)
 #define IP101A_G_IRQ_DUPLEX_CHANGE	BIT(1)
 #define IP101A_G_IRQ_LINK_CHANGE	BIT(0)
+#define IP101A_G_PHY_STATUS		18
+#define IP101A_G_MDIX			BIT(9)
+#define IP101A_G_PHY_SPEC_CTRL		30
+#define IP101A_G_FORCE_MDIX		BIT(3)
 
 #define IP101G_PAGE_CONTROL				0x14
 #define IP101G_PAGE_CONTROL_MASK			GENMASK(4, 0)
@@ -297,6 +302,90 @@ static int ip101g_config_init(struct phy_device *phydev)
 	return ip101a_g_config_intr_pin(phydev);
 }
 
+static int ip101a_g_read_status(struct phy_device *phydev)
+{
+	int oldpage, ret, stat1, stat2;
+
+	ret = genphy_read_status(phydev);
+	if (ret)
+		return ret;
+
+	oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);
+
+	ret = __phy_read(phydev, IP10XX_SPEC_CTRL_STATUS);
+	if (ret < 0)
+		goto out;
+	stat1 = ret;
+
+	ret = __phy_read(phydev, IP101A_G_PHY_SPEC_CTRL);
+	if (ret < 0)
+		goto out;
+	stat2 = ret;
+
+	if (stat1 & IP101A_G_AUTO_MDIX_DIS) {
+		if (stat2 & IP101A_G_FORCE_MDIX)
+			phydev->mdix_ctrl = ETH_TP_MDI_X;
+		else
+			phydev->mdix_ctrl = ETH_TP_MDI;
+	} else {
+		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+	}
+
+	if (stat2 & IP101A_G_MDIX)
+		phydev->mdix = ETH_TP_MDI_X;
+	else
+		phydev->mdix = ETH_TP_MDI;
+
+	ret = 0;
+
+out:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
+static int ip101a_g_config_mdix(struct phy_device *phydev)
+{
+	u16 ctrl = 0, ctrl2 = 0;
+	int oldpage, ret;
+
+	switch (phydev->mdix_ctrl) {
+	case ETH_TP_MDI:
+		ctrl = IP101A_G_AUTO_MDIX_DIS;
+		break;
+	case ETH_TP_MDI_X:
+		ctrl = IP101A_G_AUTO_MDIX_DIS;
+		ctrl2 = IP101A_G_FORCE_MDIX;
+		break;
+	case ETH_TP_MDI_AUTO:
+		break;
+	default:
+		return 0;
+	}
+
+	oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);
+
+	ret = __phy_modify(phydev, IP10XX_SPEC_CTRL_STATUS,
+			   IP101A_G_AUTO_MDIX_DIS, ctrl);
+	if (ret)
+		goto out;
+
+	ret = __phy_modify(phydev, IP101A_G_PHY_SPEC_CTRL,
+			   IP101A_G_FORCE_MDIX, ctrl2);
+
+out:
+	return phy_restore_page(phydev, oldpage, ret);
+}
+
+static int ip101a_g_config_aneg(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = ip101a_g_config_mdix(phydev);
+	if (ret)
+		return ret;
+
+	return genphy_config_aneg(phydev);
+}
+
 static int ip101a_g_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -502,6 +591,8 @@ static struct phy_driver icplus_driver[] = {
 	.config_intr	= ip101a_g_config_intr,
 	.handle_interrupt = ip101a_g_handle_interrupt,
 	.config_init	= ip101a_config_init,
+	.config_aneg	= ip101a_g_config_aneg,
+	.read_status	= ip101a_g_read_status,
 	.soft_reset	= genphy_soft_reset,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
@@ -514,6 +605,8 @@ static struct phy_driver icplus_driver[] = {
 	.config_intr	= ip101a_g_config_intr,
 	.handle_interrupt = ip101a_g_handle_interrupt,
 	.config_init	= ip101g_config_init,
+	.config_aneg	= ip101a_g_config_aneg,
+	.read_status	= ip101a_g_read_status,
 	.soft_reset	= genphy_soft_reset,
 	.get_sset_count = ip101g_get_sset_count,
 	.get_strings	= ip101g_get_strings,
-- 
2.20.1

Re: [PATCH net-next v3 9/9] net: phy: icplus: add MDI/MDIX support for IP101A/G

[Michael Walle]

Am 2021-02-10 22:08, schrieb Michael Walle:
> Implement the operations to set desired mode and retrieve the current
> mode.
> 
> This feature was tested with an IP101G.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
> Changes since v2:
>  - none
> 
> Changes since v1:
>  - none, except that the callbacks are register for both IP101A and 
> IP101G
>    PHY drivers
> 
>  drivers/net/phy/icplus.c | 93 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
> 
> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
> index 96e9d1d12992..cc5b25714002 100644
> --- a/drivers/net/phy/icplus.c
> +++ b/drivers/net/phy/icplus.c
> @@ -37,12 +37,17 @@ MODULE_LICENSE("GPL");
>  #define IP1001_SPEC_CTRL_STATUS_2	20	/* IP1001 Spec. Control Reg 2 */
>  #define IP1001_APS_ON			11	/* IP1001 APS Mode  bit */
>  #define IP101A_G_APS_ON			BIT(1)	/* IP101A/G APS Mode bit */
> +#define IP101A_G_AUTO_MDIX_DIS		BIT(11)
>  #define IP101A_G_IRQ_CONF_STATUS	0x11	/* Conf Info IRQ & Status Reg */
>  #define	IP101A_G_IRQ_PIN_USED		BIT(15) /* INTR pin used */
>  #define IP101A_G_IRQ_ALL_MASK		BIT(11) /* IRQ's inactive */
>  #define IP101A_G_IRQ_SPEED_CHANGE	BIT(2)
>  #define IP101A_G_IRQ_DUPLEX_CHANGE	BIT(1)
>  #define IP101A_G_IRQ_LINK_CHANGE	BIT(0)
> +#define IP101A_G_PHY_STATUS		18
> +#define IP101A_G_MDIX			BIT(9)
> +#define IP101A_G_PHY_SPEC_CTRL		30
> +#define IP101A_G_FORCE_MDIX		BIT(3)
> 
>  #define IP101G_PAGE_CONTROL				0x14
>  #define IP101G_PAGE_CONTROL_MASK			GENMASK(4, 0)
> @@ -297,6 +302,90 @@ static int ip101g_config_init(struct phy_device 
> *phydev)
>  	return ip101a_g_config_intr_pin(phydev);
>  }
> 
> +static int ip101a_g_read_status(struct phy_device *phydev)
> +{
> +	int oldpage, ret, stat1, stat2;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret)
> +		return ret;
> +
> +	oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);

same here, missing return code check, will be fixed in v4

> +
> +	ret = __phy_read(phydev, IP10XX_SPEC_CTRL_STATUS);
> +	if (ret < 0)
> +		goto out;
> +	stat1 = ret;
> +
> +	ret = __phy_read(phydev, IP101A_G_PHY_SPEC_CTRL);
> +	if (ret < 0)
> +		goto out;
> +	stat2 = ret;
> +
> +	if (stat1 & IP101A_G_AUTO_MDIX_DIS) {
> +		if (stat2 & IP101A_G_FORCE_MDIX)
> +			phydev->mdix_ctrl = ETH_TP_MDI_X;
> +		else
> +			phydev->mdix_ctrl = ETH_TP_MDI;
> +	} else {
> +		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> +	}
> +
> +	if (stat2 & IP101A_G_MDIX)
> +		phydev->mdix = ETH_TP_MDI_X;
> +	else
> +		phydev->mdix = ETH_TP_MDI;
> +
> +	ret = 0;
> +
> +out:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +
> +static int ip101a_g_config_mdix(struct phy_device *phydev)
> +{
> +	u16 ctrl = 0, ctrl2 = 0;
> +	int oldpage, ret;
> +
> +	switch (phydev->mdix_ctrl) {
> +	case ETH_TP_MDI:
> +		ctrl = IP101A_G_AUTO_MDIX_DIS;
> +		break;
> +	case ETH_TP_MDI_X:
> +		ctrl = IP101A_G_AUTO_MDIX_DIS;
> +		ctrl2 = IP101A_G_FORCE_MDIX;
> +		break;
> +	case ETH_TP_MDI_AUTO:
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);

dito

-michael

> +
> +	ret = __phy_modify(phydev, IP10XX_SPEC_CTRL_STATUS,
> +			   IP101A_G_AUTO_MDIX_DIS, ctrl);
> +	if (ret)
> +		goto out;
> +
> +	ret = __phy_modify(phydev, IP101A_G_PHY_SPEC_CTRL,
> +			   IP101A_G_FORCE_MDIX, ctrl2);
> +
> +out:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +
> +static int ip101a_g_config_aneg(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = ip101a_g_config_mdix(phydev);
> +	if (ret)
> +		return ret;
> +
> +	return genphy_config_aneg(phydev);
> +}
> +
>  static int ip101a_g_ack_interrupt(struct phy_device *phydev)
>  {
>  	int err;
> @@ -502,6 +591,8 @@ static struct phy_driver icplus_driver[] = {
>  	.config_intr	= ip101a_g_config_intr,
>  	.handle_interrupt = ip101a_g_handle_interrupt,
>  	.config_init	= ip101a_config_init,
> +	.config_aneg	= ip101a_g_config_aneg,
> +	.read_status	= ip101a_g_read_status,
>  	.soft_reset	= genphy_soft_reset,
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
> @@ -514,6 +605,8 @@ static struct phy_driver icplus_driver[] = {
>  	.config_intr	= ip101a_g_config_intr,
>  	.handle_interrupt = ip101a_g_handle_interrupt,
>  	.config_init	= ip101g_config_init,
> +	.config_aneg	= ip101a_g_config_aneg,
> +	.read_status	= ip101a_g_read_status,
>  	.soft_reset	= genphy_soft_reset,
>  	.get_sset_count = ip101g_get_sset_count,
>  	.get_strings	= ip101g_get_strings,

[PATCH net-next v2 0/9] net: phy: icplus: cleanups and new features

[Michael Walle]

Cleanup the PHY drivers for IPplus devices and add PHY counters and MDIX
support for the IP101A/G.

Patch 5 adds a model detection based on the behavior of the PHY.
Unfortunately, the IP101A shares the PHY ID with the IP101G. But the latter
provides more features. Try to detect the newer model by accessing the page
selection register. If it is writeable, it is assumed, that it is a IP101G.

With this detection in place, we can now access registers >= 16 in a
correct way on the IP101G; that is by first selecting the correct page.
This might previouly worked, because no one ever set another active page
before booting linux.

The last two patches add the new features.

Michael Walle (9):
  net: phy: icplus: use PHY_ID_MATCH_MODEL() macro
  net: phy: icplus: use PHY_ID_MATCH_EXACT() for IP101A/G
  net: phy: icplus: drop address operator for functions
  net: phy: icplus: use the .soft_reset() of the phy-core
  net: phy: icplus: split IP101A/G driver
  net: phy: icplus: don't set APS_EN bit on IP101G
  net: phy: icplus: fix paged register access
  net: phy: icplus: add PHY counter for IP101G
  net: phy: icplus: add MDI/MDIX support for IP101A/G

 drivers/net/phy/icplus.c | 378 ++++++++++++++++++++++++++++++++-------
 1 file changed, 318 insertions(+), 60 deletions(-)

-- 
2.20.1