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

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: add IP101A/IP101G model detection
  net: phy: icplus: don't set APS_EN bit on IP101G
  net: phy: icplus: select page before writing control register
  net: phy: icplus: add PHY counter for IP101G
  net: phy: icplus: add MDI/MDIX support for IP101A/G

 drivers/net/phy/icplus.c | 328 ++++++++++++++++++++++++++++++++-------
 1 file changed, 272 insertions(+), 56 deletions(-)

-- 
2.20.1

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

Signed-off-by: Michael Walle <redacted>
---
 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

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 <redacted>
---
 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

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 <redacted>
---
 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

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

Signed-off-by: Michael Walle <redacted>
---
 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

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.

Signed-off-by: Michael Walle <redacted>
---
 drivers/net/phy/icplus.c | 43 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 036bac628b11..189a9a34ed5f 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)
 

@@ -61,8 +63,14 @@ enum ip101gr_sel_intr32 {
 	IP101GR_SEL_INTR32_RXER,
 };
 
+enum ip101_model {
+	IP101A,
+	IP101G,
+};
+
 struct ip101a_g_phy_priv {
 	enum ip101gr_sel_intr32 sel_intr32;
+	enum ip101_model model;
 };
 
 static int ip175c_config_init(struct phy_device *phydev)

@@ -175,6 +183,39 @@ static int ip175c_config_aneg(struct phy_device *phydev)
 	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 a page select register, which is not available on the IP101. Use this
+ * to distinguish these two.
+ */
+static int ip101a_g_detect_model(struct phy_device *phydev)
+{
+	struct ip101a_g_phy_priv *priv = phydev->priv;
+	int oldval, 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;
+
+	ret = phy_read(phydev, IP101G_PAGE_CONTROL);
+	if (ret < 0)
+		return ret;
+
+	if (ret == IP101G_PAGE_CONTROL_MASK)
+		priv->model = IP101G;
+	else
+		priv->model = IP101A;
+
+	phydev_dbg(phydev, "Detected %s\n",
+		   priv->model == IP101G ? "IP101G" : "IP101A");
+
+	return phy_write(phydev, IP101G_PAGE_CONTROL, oldval);
+}
+
 static int ip101a_g_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;

@@ -203,7 +244,7 @@ static int ip101a_g_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
-	return 0;
+	return ip101a_g_detect_model(phydev);
 }
 
 static int ip101a_g_config_init(struct phy_device *phydev)

-- 
2.20.1

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 <redacted>
---
 drivers/net/phy/icplus.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 189a9a34ed5f..a6e1c7611f15 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c

@@ -250,7 +250,7 @@ static int ip101a_g_probe(struct phy_device *phydev)
 static int ip101a_g_config_init(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) {

@@ -280,11 +280,16 @@ static int ip101a_g_config_init(struct phy_device *phydev)
 		break;
 	}
 
-	/* Enable Auto Power Saving mode */
-	c = phy_read(phydev, IP10XX_SPEC_CTRL_STATUS);
-	c |= IP101A_G_APS_ON;
+	/* Enable Auto Power Saving mode on IP101A, on IP101G this bit is
+	 * reserved as 'write-one'.
+	 */
+	if (priv->model == IP101A) {
+		err = phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, IP101A_G_APS_ON);
+		if (err)
+			return err;
+	}
 
-	return phy_write(phydev, IP10XX_SPEC_CTRL_STATUS, c);
+	return 0;
 }
 
 static int ip101a_g_ack_interrupt(struct phy_device *phydev)

-- 
2.20.1

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 <redacted>
---
 drivers/net/phy/icplus.c | 78 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 858b9326a72d..d1b57d81f281 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

@@ -70,9 +76,20 @@ enum ip101_model {
 	IP101G,
 };
 
+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;
 	enum ip101_model model;
+	u64 stats[ARRAY_SIZE(ip101g_hw_stats)];
 };
 
 static int ip175c_config_init(struct phy_device *phydev)

@@ -254,6 +271,18 @@ static int ip101a_g_config_init(struct phy_device *phydev)
 	struct ip101a_g_phy_priv *priv = phydev->priv;
 	int oldpage, err;
 
+	/* Enable the PHY counters */
+	err = phy_modify_paged(phydev, 1, IP101G_P1_CNT_CTRL,
+			       CNT_CTRL_RX_EN, CNT_CTRL_RX_EN);
+	if (err)
+		return err;
+
+	/* Clear error counters on read */
+	err = phy_modify_paged(phydev, 8, IP101G_P8_CNT_CTRL,
+			       CNT_CTRL_RDCLR_EN, CNT_CTRL_RDCLR_EN);
+	if (err)
+		return err;
+
 	oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);
 
 	/* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */

@@ -373,6 +402,52 @@ static int ip101a_g_write_page(struct phy_device *phydev, int page)
 	return __phy_write(phydev, IP101G_PAGE_CONTROL, page);
 }
 
+static int ip101a_g_get_sset_count(struct phy_device *phydev)
+{
+	struct ip101a_g_phy_priv *priv = phydev->priv;
+
+	if (priv->model == IP101A)
+		return -EOPNOTSUPP;
+
+	return ARRAY_SIZE(ip101g_hw_stats);
+}
+
+static void ip101a_g_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 ip101a_g_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 ip101a_g_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] = ip101a_g_get_stat(phydev, i);
+}
+
 static struct phy_driver icplus_driver[] = {
 {
 	PHY_ID_MATCH_MODEL(IP175C_PHY_ID),

@@ -402,6 +477,9 @@ static struct phy_driver icplus_driver[] = {
 	.read_page	= ip101a_g_read_page,
 	.write_page	= ip101a_g_write_page,
 	.soft_reset	= genphy_soft_reset,
+	.get_sset_count = ip101a_g_get_sset_count,
+	.get_strings	= ip101a_g_get_strings,
+	.get_stats	= ip101a_g_get_stats,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 } };

-- 
2.20.1

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.

Add read_page()/write_page() support for the IP101G and use it
accordingly.

Signed-off-by: Michael Walle <redacted>
---
 drivers/net/phy/icplus.c | 50 +++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index a6e1c7611f15..858b9326a72d 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

@@ -250,23 +252,25 @@ static int ip101a_g_probe(struct phy_device *phydev)
 static int ip101a_g_config_init(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:

@@ -284,12 +288,14 @@ static int ip101a_g_config_init(struct phy_device *phydev)
 	 * reserved as 'write-one'.
 	 */
 	if (priv->model == IP101A) {
-		err = phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, IP101A_G_APS_ON);
+		err = __phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS,
+				     IP101A_G_APS_ON);
 		if (err)
-			return err;
+			goto out;
 	}
 
-	return 0;
+out:
+	return phy_restore_page(phydev, oldpage, err);
 }
 
 static int ip101a_g_ack_interrupt(struct phy_device *phydev)

@@ -347,6 +353,26 @@ static irqreturn_t ip101a_g_handle_interrupt(struct phy_device *phydev)
 	return IRQ_HANDLED;
 }
 
+static int ip101a_g_read_page(struct phy_device *phydev)
+{
+	struct ip101a_g_phy_priv *priv = phydev->priv;
+
+	if (priv->model == IP101A)
+		return 0;
+
+	return __phy_read(phydev, IP101G_PAGE_CONTROL);
+}
+
+static int ip101a_g_write_page(struct phy_device *phydev, int page)
+{
+	struct ip101a_g_phy_priv *priv = phydev->priv;
+
+	if (priv->model == IP101A)
+		return 0;
+
+	return __phy_write(phydev, IP101G_PAGE_CONTROL, page);
+}
+
 static struct phy_driver icplus_driver[] = {
 {
 	PHY_ID_MATCH_MODEL(IP175C_PHY_ID),

@@ -373,6 +399,8 @@ static struct phy_driver icplus_driver[] = {
 	.config_intr	= ip101a_g_config_intr,
 	.handle_interrupt = ip101a_g_handle_interrupt,
 	.config_init	= ip101a_g_config_init,
+	.read_page	= ip101a_g_read_page,
+	.write_page	= ip101a_g_write_page,
 	.soft_reset	= genphy_soft_reset,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,

-- 
2.20.1

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

This feature was tested with an IP101G.

Signed-off-by: Michael Walle <redacted>
---
 drivers/net/phy/icplus.c | 91 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index d1b57d81f281..a2fee2d08ec2 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)

@@ -327,6 +332,90 @@ static int ip101a_g_config_init(struct phy_device *phydev)
 	return phy_restore_page(phydev, oldpage, err);
 }
 
+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 = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);

@@ -474,6 +563,8 @@ static struct phy_driver icplus_driver[] = {
 	.config_intr	= ip101a_g_config_intr,
 	.handle_interrupt = ip101a_g_handle_interrupt,
 	.config_init	= ip101a_g_config_init,
+	.config_aneg	= ip101a_g_config_aneg,
+	.read_status	= ip101a_g_read_status,
 	.read_page	= ip101a_g_read_page,
 	.write_page	= ip101a_g_write_page,
 	.soft_reset	= genphy_soft_reset,

-- 
2.20.1

On 09.02.2021 17:40, Michael Walle wrote:

You could also implement the match_phy_device callback. Then you can
have separate PHY drivers for IP101A/IP101G. Would be cleaner I think.
See the Realtek PHY driver for an example.

On Tue, Feb 09, 2021 at 05:40:43PM +0100, Michael Walle wrote:

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

Signed-off-by: Michael Walle <redacted>

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

    Andrew

On Tue, Feb 09, 2021 at 05:40:44PM +0100, Michael Walle wrote:

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 <redacted>

Lets hope the datasheet is correct and up to date, because this could
cause a regression if wrong.

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

    Andrew

On Tue, Feb 09, 2021 at 05:40:45PM +0100, Michael Walle wrote:

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

Signed-off-by: Michael Walle <redacted>

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

    Andrew

On Tue, Feb 09, 2021 at 05:40:46PM +0100, Michael Walle wrote:

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 <redacted>

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

    Andrew

On Tue, Feb 09, 2021 at 05:40:48PM +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 <redacted>

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

    Andrew

On Tue, Feb 09, 2021 at 05:40:49PM +0100, Michael Walle wrote:

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.

Add read_page()/write_page() support for the IP101G and use it
accordingly.

Signed-off-by: Michael Walle <redacted>

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

    Andrew

On Tue, Feb 09, 2021 at 05:40:50PM +0100, Michael Walle wrote:

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 <redacted>

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

    Andrew

On Tue, Feb 09, 2021 at 05:40:51PM +0100, Michael Walle wrote:

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

This feature was tested with an IP101G.

Signed-off-by: Michael Walle <redacted>

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

    Andrew

On 09.02.2021 17:40, Michael Walle wrote:

If a random page was set before entering config_init, do we actually want
to restore it? Or wouldn't it be better to set the default page as part
of initialization?

Hi,

Am 2021-02-10 08:03, schrieb Heiner Kallweit:

On 09.02.2021 17:40, Michael Walle wrote:

quoted

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.

Add read_page()/write_page() support for the IP101G and use it
accordingly.

Signed-off-by: Michael Walle <redacted>
---
 drivers/net/phy/icplus.c | 50 
+++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index a6e1c7611f15..858b9326a72d 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

@@ -250,23 +252,25 @@ static int ip101a_g_probe(struct phy_device 

*phydev)
 static int ip101a_g_config_init(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:

@@ -284,12 +288,14 @@ static int ip101a_g_config_init(struct 

phy_device *phydev)
 	 * reserved as 'write-one'.
 	 */
 	if (priv->model == IP101A) {
-		err = phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, 
IP101A_G_APS_ON);
+		err = __phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS,
+				     IP101A_G_APS_ON);
 		if (err)
-			return err;
+			goto out;
 	}

-	return 0;
+out:
+	return phy_restore_page(phydev, oldpage, err);

If a random page was set before entering config_init, do we actually 
want
to restore it? Or wouldn't it be better to set the default page as part
of initialization?

First, I want to convert this to the match_phy_device() and while at it,
I noticed that there is this one "problem". Short summary: the IP101A 
isn't
paged, the IP101G has serveral and if page 16 is selected it is more or
less compatible with the IP101A. My problem here is now how to share the
functions for both PHYs without duplicating all the code. Eg. the IP101A
will phy_read/phy_write/phy_modify(), that is, all the locked versions.
For the IP101G I'd either need the _paged() versions or the __phy ones
which don't take the mdio_bus lock.

Here is what I came up with:
(1) provide a common function which uses the __phy ones, then the
     callback for the A version will take the mdio_bus lock and calls
     the common one. The G version will use phy_{select,restore}_page().
(2) the phy_driver ops for A will also provde a .read/write_page()
     callback which is just a no-op. So A can just use the G versions.
(3) What Heiner mentioned here, just set the default page in
     config_init().

(1) will still bloat the code; (3) has the disadvantage, that the
userspace might fiddle around with the page register and then the
whole PHY driver goes awry. I don't know if we have to respect that
use case in general. I know there is an API to read/write the PHY
registers and it could happen.

That being said, I'm either fine with (2) and (3) but I'm preferring
(2).

BTW, this patch is still missing read/writes to the interrupt status
and control registers which is also paged.

-michael

On 10.02.2021 09:25, Michael Walle wrote:

Hi,

Am 2021-02-10 08:03, schrieb Heiner Kallweit:

quoted

On 09.02.2021 17:40, Michael Walle wrote:

quoted

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.

Add read_page()/write_page() support for the IP101G and use it
accordingly.

Signed-off-by: Michael Walle <redacted>
---
 drivers/net/phy/icplus.c | 50 +++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index a6e1c7611f15..858b9326a72d 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

@@ -250,23 +252,25 @@ static int ip101a_g_probe(struct phy_device *phydev)

 static int ip101a_g_config_init(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:

@@ -284,12 +288,14 @@ static int ip101a_g_config_init(struct phy_device *phydev)

      * reserved as 'write-one'.
      */
     if (priv->model == IP101A) {
-        err = phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS, IP101A_G_APS_ON);
+        err = __phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS,
+                     IP101A_G_APS_ON);
         if (err)
-            return err;
+            goto out;
     }

-    return 0;
+out:
+    return phy_restore_page(phydev, oldpage, err);

If a random page was set before entering config_init, do we actually want
to restore it? Or wouldn't it be better to set the default page as part
of initialization?

First, I want to convert this to the match_phy_device() and while at it,
I noticed that there is this one "problem". Short summary: the IP101A isn't
paged, the IP101G has serveral and if page 16 is selected it is more or
less compatible with the IP101A. My problem here is now how to share the
functions for both PHYs without duplicating all the code. Eg. the IP101A
will phy_read/phy_write/phy_modify(), that is, all the locked versions.
For the IP101G I'd either need the _paged() versions or the __phy ones
which don't take the mdio_bus lock.

Here is what I came up with:
(1) provide a common function which uses the __phy ones, then the
    callback for the A version will take the mdio_bus lock and calls
    the common one. The G version will use phy_{select,restore}_page().
(2) the phy_driver ops for A will also provde a .read/write_page()
    callback which is just a no-op. So A can just use the G versions.
(3) What Heiner mentioned here, just set the default page in
    config_init().

(1) will still bloat the code; (3) has the disadvantage, that the
userspace might fiddle around with the page register and then the
whole PHY driver goes awry. I don't know if we have to respect that
use case in general. I know there is an API to read/write the PHY
registers and it could happen.

The potential issue you mention here we have with all PHY's using
pages. As one example, the genphy functions rely on the PHY being
set to the default page. In general userspace can write PHY register
values that break processing, independent of paging.
I'm not aware of any complaints regarding this behavior, therefore
wouldn't be too concerned here.

Regarding (2) I'd like to come back to my proposal from yesterday,
implement match_phy_device to completely decouple the A and G versions.
Did you consider this option?

That being said, I'm either fine with (2) and (3) but I'm preferring
(2).

BTW, this patch is still missing read/writes to the interrupt status
and control registers which is also paged.

-michael

Heiner

Hi,

Am 2021-02-10 10:03, schrieb Heiner Kallweit:
[..]

quoted

quoted

quoted

+    return phy_restore_page(phydev, oldpage, err);

If a random page was set before entering config_init, do we actually 
want
to restore it? Or wouldn't it be better to set the default page as 
part
of initialization?

First, I want to convert this to the match_phy_device() and while at 
it,
I noticed that there is this one "problem". Short summary: the IP101A 
isn't
paged, the IP101G has serveral and if page 16 is selected it is more 
or
less compatible with the IP101A. My problem here is now how to share 
the
functions for both PHYs without duplicating all the code. Eg. the 
IP101A
will phy_read/phy_write/phy_modify(), that is, all the locked 
versions.
For the IP101G I'd either need the _paged() versions or the __phy ones
which don't take the mdio_bus lock.

Here is what I came up with:
(1) provide a common function which uses the __phy ones, then the
    callback for the A version will take the mdio_bus lock and calls
    the common one. The G version will use 
phy_{select,restore}_page().
(2) the phy_driver ops for A will also provde a .read/write_page()
    callback which is just a no-op. So A can just use the G versions.
(3) What Heiner mentioned here, just set the default page in
    config_init().

(1) will still bloat the code; (3) has the disadvantage, that the
userspace might fiddle around with the page register and then the
whole PHY driver goes awry. I don't know if we have to respect that
use case in general. I know there is an API to read/write the PHY
registers and it could happen.

The potential issue you mention here we have with all PHY's using
pages. As one example, the genphy functions rely on the PHY being
set to the default page. In general userspace can write PHY register
values that break processing, independent of paging.
I'm not aware of any complaints regarding this behavior, therefore
wouldn't be too concerned here.

I'm fine with that, that will make the driver easier.

Regarding (2) I'd like to come back to my proposal from yesterday,
implement match_phy_device to completely decouple the A and G versions.
Did you consider this option?

Yes, that is what I was talking about above: "First, I want to convert
this to the match_phy_device()" ;) And then I stumbled across that 
problem
I was describing above.

It will likely go away if I just set the page to the default page.

quoted

That being said, I'm either fine with (2) and (3) but I'm preferring
(2).

BTW, this patch is still missing read/writes to the interrupt status
and control registers which is also paged.

-- 
-michael

On Wed, Feb 10, 2021 at 08:03:07AM +0100, Heiner Kallweit wrote:

On 09.02.2021 17:40, Michael Walle wrote:

quoted

+out:
+	return phy_restore_page(phydev, oldpage, err);

If a random page was set before entering config_init, do we actually want
to restore it? Or wouldn't it be better to set the default page as part
of initialization?

I think you've missed asking one key question: does the paging on this
PHY affect the standardised registers at 0..15 inclusive, or does it
only affect registers 16..31?

If it doesn't affect the standardised registers, then the genphy_*
functions don't care which page is selected.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Am 2021-02-10 11:30, schrieb Russell King - ARM Linux admin:

On Wed, Feb 10, 2021 at 08:03:07AM +0100, Heiner Kallweit wrote:

quoted

On 09.02.2021 17:40, Michael Walle wrote:

quoted

+out:
+	return phy_restore_page(phydev, oldpage, err);

If a random page was set before entering config_init, do we actually 
want
to restore it? Or wouldn't it be better to set the default page as 
part
of initialization?

I think you've missed asking one key question: does the paging on this
PHY affect the standardised registers at 0..15 inclusive, or does it
only affect registers 16..31?

For this PHY it affects only registers >=16. But that doesn't invaldiate
the point that for other PHYs this might affect all regsisters. Eg. ones
where you could select between fiber and copper pages, right?

If it doesn't affect the standardised registers, then the genphy_*
functions don't care which page is selected.

-- 
-michael

On Wed, Feb 10, 2021 at 11:38:18AM +0100, Michael Walle wrote:

Am 2021-02-10 11:30, schrieb Russell King - ARM Linux admin:

quoted

On Wed, Feb 10, 2021 at 08:03:07AM +0100, Heiner Kallweit wrote:

quoted

On 09.02.2021 17:40, Michael Walle wrote:

quoted

+out:
+	return phy_restore_page(phydev, oldpage, err);

If a random page was set before entering config_init, do we actually
want
to restore it? Or wouldn't it be better to set the default page as
part
of initialization?

I think you've missed asking one key question: does the paging on this
PHY affect the standardised registers at 0..15 inclusive, or does it
only affect registers 16..31?

For this PHY it affects only registers >=16. But that doesn't invaldiate
the point that for other PHYs this might affect all regsisters. Eg. ones
where you could select between fiber and copper pages, right?

You are modifying the code using ip101a_g_* functions, which is only
used for the IP101A and IP101G PHYs. Do these devices support fiber
in a way that change the first 16 registers?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Am 2021-02-10 11:49, schrieb Russell King - ARM Linux admin:

On Wed, Feb 10, 2021 at 11:38:18AM +0100, Michael Walle wrote:

quoted

Am 2021-02-10 11:30, schrieb Russell King - ARM Linux admin:

quoted

On Wed, Feb 10, 2021 at 08:03:07AM +0100, Heiner Kallweit wrote:

quoted

On 09.02.2021 17:40, Michael Walle wrote:

quoted

+out:
+	return phy_restore_page(phydev, oldpage, err);

If a random page was set before entering config_init, do we actually
want
to restore it? Or wouldn't it be better to set the default page as
part
of initialization?

I think you've missed asking one key question: does the paging on this
PHY affect the standardised registers at 0..15 inclusive, or does it
only affect registers 16..31?

For this PHY it affects only registers >=16. But that doesn't 
invaldiate
the point that for other PHYs this might affect all regsisters. Eg. 
ones
where you could select between fiber and copper pages, right?

You are modifying the code using ip101a_g_* functions, which is only
used for the IP101A and IP101G PHYs. Do these devices support fiber
in a way that change the first 16 registers?

The PHY doesn't support fiber and register 0..15 are always available
regardless of the selected page for the IP101G.

genphy_() stuff will work, but the IP101G PHY driver specific functions,
like interrupt and mdix will break if someone is messing with the page
register from userspace.

So Heiner's point was, that there are other PHY drivers which
also break when a user changes registers from userspace and no one
seemed to cared about that for now.

I guess it boils down to: how hard should we try to get the driver
behave correctly if the user is changing registers. Or can we
just make the assumption that if the PHY driver sets the page
selection to its default, all the other callbacks will work
on this page.

-michael

On Wed, Feb 10, 2021 at 12:14:35PM +0100, Michael Walle wrote:

Am 2021-02-10 11:49, schrieb Russell King - ARM Linux admin:
The PHY doesn't support fiber and register 0..15 are always available
regardless of the selected page for the IP101G.

genphy_() stuff will work, but the IP101G PHY driver specific functions,
like interrupt and mdix will break if someone is messing with the page
register from userspace.

So Heiner's point was, that there are other PHY drivers which
also break when a user changes registers from userspace and no one
seemed to cared about that for now.

I guess it boils down to: how hard should we try to get the driver
behave correctly if the user is changing registers. Or can we
just make the assumption that if the PHY driver sets the page
selection to its default, all the other callbacks will work
on this page.

Provided the PHY driver uses the paged accessors for all paged
registers, userspace can't break the PHY driver because we have proper
locking in the paged accessors (I wrote them.) Userspace can access the
paged registers too, but there will be no locking other than on each
individual access. This can't be fixed without augmenting the kernel/
user API, and in any case is not a matter for the PHY driver.

So, let's stop worrying about the userspace paged access problem for
driver reviews; that's a core phylib and userspace API issue.

The paged accessor API is designed to allow the driver author to access
registers in the most efficient manner. There are two parts to it.

1) the phy_*_paged() accessors switch the page before accessing the
   register, and restore it afterwards. If you need to access a lot
   of paged registers, this can be inefficient.

2) phy_save_page()..phy_restore_page() allows wrapping of __phy_*
   accessors to access paged registers.

3) phy_select_page()..phy_restore_page() also allows wrapping of
   __phy_* accessors to access paged registers.

phy_save_page() and phy_select_page() must /always/ be paired with
a call to phy_restore_page(), since the former pair takes the bus lock
and the latter releases it.

(2) and (3) allow multiple accesses to either a single page without
constantly saving and restoring the page, and can also be used to
select other pages without the save/restore and locking steps. We
could export __phy_read_page() and __phy_write_page() if required.

While the bus lock is taken, userspace can't access any PHY on the bus.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Am 2021-02-10 12:48, schrieb Russell King - ARM Linux admin:

On Wed, Feb 10, 2021 at 12:14:35PM +0100, Michael Walle wrote:

quoted

Am 2021-02-10 11:49, schrieb Russell King - ARM Linux admin:
The PHY doesn't support fiber and register 0..15 are always available
regardless of the selected page for the IP101G.

genphy_() stuff will work, but the IP101G PHY driver specific 
functions,
like interrupt and mdix will break if someone is messing with the page
register from userspace.

So Heiner's point was, that there are other PHY drivers which
also break when a user changes registers from userspace and no one
seemed to cared about that for now.

I guess it boils down to: how hard should we try to get the driver
behave correctly if the user is changing registers. Or can we
just make the assumption that if the PHY driver sets the page
selection to its default, all the other callbacks will work
on this page.

Provided the PHY driver uses the paged accessors for all paged
registers, userspace can't break the PHY driver because we have proper
locking in the paged accessors (I wrote them.) Userspace can access the
paged registers too, but there will be no locking other than on each
individual access. This can't be fixed without augmenting the kernel/
user API, and in any case is not a matter for the PHY driver.

So, let's stop worrying about the userspace paged access problem for
driver reviews; that's a core phylib and userspace API issue.

The paged accessor API is designed to allow the driver author to access
registers in the most efficient manner. There are two parts to it.

1) the phy_*_paged() accessors switch the page before accessing the
   register, and restore it afterwards. If you need to access a lot
   of paged registers, this can be inefficient.

2) phy_save_page()..phy_restore_page() allows wrapping of __phy_*
   accessors to access paged registers.

3) phy_select_page()..phy_restore_page() also allows wrapping of
   __phy_* accessors to access paged registers.

phy_save_page() and phy_select_page() must /always/ be paired with
a call to phy_restore_page(), since the former pair takes the bus lock
and the latter releases it.

(2) and (3) allow multiple accesses to either a single page without
constantly saving and restoring the page, and can also be used to
select other pages without the save/restore and locking steps. We
could export __phy_read_page() and __phy_write_page() if required.

While the bus lock is taken, userspace can't access any PHY on the bus.

That was how the v1 of this series was written. But Heiner's review
comment was, what if we just set the default page and don't
use phy_save_page()..phy_restore_page() for the registers which are
behind the default page. And in this case, userspace _can_ break
access to the paged registers.

-michael

On 10.02.2021 13:17, Michael Walle wrote:

Am 2021-02-10 12:48, schrieb Russell King - ARM Linux admin:

quoted

On Wed, Feb 10, 2021 at 12:14:35PM +0100, Michael Walle wrote:

quoted

Am 2021-02-10 11:49, schrieb Russell King - ARM Linux admin:
The PHY doesn't support fiber and register 0..15 are always available
regardless of the selected page for the IP101G.

genphy_() stuff will work, but the IP101G PHY driver specific functions,
like interrupt and mdix will break if someone is messing with the page
register from userspace.

So Heiner's point was, that there are other PHY drivers which
also break when a user changes registers from userspace and no one
seemed to cared about that for now.

I guess it boils down to: how hard should we try to get the driver
behave correctly if the user is changing registers. Or can we
just make the assumption that if the PHY driver sets the page
selection to its default, all the other callbacks will work
on this page.

Provided the PHY driver uses the paged accessors for all paged
registers, userspace can't break the PHY driver because we have proper
locking in the paged accessors (I wrote them.) Userspace can access the
paged registers too, but there will be no locking other than on each
individual access. This can't be fixed without augmenting the kernel/
user API, and in any case is not a matter for the PHY driver.

So, let's stop worrying about the userspace paged access problem for
driver reviews; that's a core phylib and userspace API issue.

The paged accessor API is designed to allow the driver author to access
registers in the most efficient manner. There are two parts to it.

1) the phy_*_paged() accessors switch the page before accessing the
   register, and restore it afterwards. If you need to access a lot
   of paged registers, this can be inefficient.

2) phy_save_page()..phy_restore_page() allows wrapping of __phy_*
   accessors to access paged registers.

3) phy_select_page()..phy_restore_page() also allows wrapping of
   __phy_* accessors to access paged registers.

phy_save_page() and phy_select_page() must /always/ be paired with
a call to phy_restore_page(), since the former pair takes the bus lock
and the latter releases it.

(2) and (3) allow multiple accesses to either a single page without
constantly saving and restoring the page, and can also be used to
select other pages without the save/restore and locking steps. We
could export __phy_read_page() and __phy_write_page() if required.

While the bus lock is taken, userspace can't access any PHY on the bus.

That was how the v1 of this series was written. But Heiner's review
comment was, what if we just set the default page and don't
use phy_save_page()..phy_restore_page() for the registers which are
behind the default page. And in this case, userspace _can_ break
access to the paged registers.

The comment was assuming that paging also applies to register 0..15,
like it is the case for Realtek PHY's. That's not the case for your
PHY, therefore the situation is slightly different.

-michael

I guess it boils down to: how hard should we try to get the driver
behave correctly if the user is changing registers.

All bets are off if root starts messing with the PHY state from
userspace. Its a foot gun, don't be surprised with what happens when
you use it.

    Andrew