[PATCH 0/7] IP101GR: devicetree based configuration of SEL_INTR32

[PATCH 0/7] IP101GR: devicetree based configuration of SEL_INTR32

[Martin Blumenstingl]

The IP101GR is a 32-pin QFN package variant of the IP101G/IP101GA
Ethernet PHY. Due to it's limited amount of pins the RXER (receive
error) and INTR32 (interrupt) functions share pin 21.

The goal of this series is:
- some small cleanups in patches 3, 4 and 5
- allowing the kernel to detect IRQ floods on boards where the IP101GR
  is configured in RXER mode but the RXER line is configured on the
  host SoC as interrupt line (patch 6)
- configuration of the SEL_INTR32 register so we can use the interrupt
  function on boards where the RXER/INTR32 pin (pin 21) is routed to
  one of the host SoC's interrupt inputs (patches 1, 2, 7)

A use-case where this is needed is the Endless Mini (EC-100). I have
tested my changes on that board. This also confirms that Heiner
Kallweit's recent icplus.c PHY driver changes are working (at least on
my setup).

This series is based on net-next commit 9c549a6b057386d ("selftests:
add explicit test for multiple concurrent GRO sockets")


Martin Blumenstingl (7):
  dt-bindings: vendor-prefix: add prefix for IC Plus Corp.
  dt-bindings: net: phy: add bindings for the IC Plus Corp. IP101A/G
    PHYs
  net: phy: icplus: keep all ip101a_g functions together
  net: phy: icplus: use the BIT macro where possible
  net: phy: icplus: rename IP101A_G_NO_IRQ to IP101A_G_IRQ_ALL_MASK
  net: phy: icplus: implement .did_interrupt for IP101A/G
  net: phy: icplus: allow configuring the interrupt function on IP101GR

 .../bindings/net/icplus-ip101ag.txt           |  19 +++
 .../devicetree/bindings/vendor-prefixes.txt   |   1 +
 drivers/net/phy/icplus.c                      | 125 +++++++++++++++---
 3 files changed, 125 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/icplus-ip101ag.txt

-- 
2.19.1

[PATCH 1/7] dt-bindings: vendor-prefix: add prefix for IC Plus Corp.

[Martin Blumenstingl]

IC Plus Corp. has various Ethernet related products such as Ethernet
transceivers, Ethernet controllers, Ethernet switches, etc.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 4b1a2a8fcc16..cc6b2c0d3b49 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -170,6 +170,7 @@ holtek	Holtek Semiconductor, Inc.
 hwacom	HwaCom Systems Inc.
 i2se	I2SE GmbH
 ibm	International Business Machines (IBM)
+icplus	IC Plus Corp.
 idt	Integrated Device Technologies, Inc.
 ifi	Ingenieurburo Fur Ic-Technologie (I/F/I)
 ilitek	ILI Technology Corporation (ILITEK)
-- 
2.19.1

[PATCH 2/7] dt-bindings: net: phy: add bindings for the IC Plus Corp. IP101A/G PHYs

[Martin Blumenstingl]

The IP101A and IP101G series both have various models. Depending on the
board implementation we need a special property for the IP101GR (32-pin
LQFP package) PHY:
pin 21 ("RXER/INTR_32") outputs the "receive error" signal by default
(LOW means "normal operation", HIGH means that there's either a decoding
error of the received signal or that the PHY is receiving LPI). This pin
can also be switched to INTR32 mode, where the interrupt signal is
routed to this pin. The other PHYs don't need this special handling
because they have more pins available so the interrupt function gets a
dedicated pin.

This adds two properties to either select the "receive error" or
"interrupt" function of pin 21. Not specifying any function means that
the default set by the bootloader is used. This is required because the
IP101GR cannot be differentiated between other IP101 PHYs as the PHY
identification registers on all of these is 0x02430c54.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../bindings/net/icplus-ip101ag.txt           | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/icplus-ip101ag.txt

diff --git a/Documentation/devicetree/bindings/net/icplus-ip101ag.txt b/Documentation/devicetree/bindings/net/icplus-ip101ag.txt
new file mode 100644
index 000000000000..a784592bbb15
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/icplus-ip101ag.txt
@@ -0,0 +1,19 @@
+IC Plus Corp. IP101A / IP101G Ethernet PHYs
+
+There are different models of the IP101G Ethernet PHY:
+- IP101GR (32-pin QFN package)
+- IP101G (die only, no package)
+- IP101GA (48-pin LQFP package)
+
+There are different models of the IP101A Ethernet PHY (which is the
+predecessor of the IP101G):
+- IP101A (48-pin LQFP package)
+- IP101AH (48-pin LQFP package)
+
+Optional properties for the IP101GR (32-pin QFN package):
+
+- icplus,select-rx-error:
+  pin 21 ("RXER/INTR_32") will output the receive error status.
+  interrupts are not routed outside the PHY in this mode.
+- icplus,select-interrupt:
+  pin 21 ("RXER/INTR_32") will output the interrupt signal.
-- 
2.19.1

[PATCH 3/7] net: phy: icplus: keep all ip101a_g functions together

[Martin Blumenstingl]

This simply moves ip101a_g_config_init right above
ip101a_g_config_intr so all functions for the ICPlus IP101A/G PHYs are
grouped together.
No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/icplus.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index ad87bd3280d7..3d3e9134c762 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -162,21 +162,6 @@ static int ip1001_config_init(struct phy_device *phydev)
 	return 0;
 }
 
-static int ip101a_g_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, IP10XX_SPEC_CTRL_STATUS);
-	c |= IP101A_G_APS_ON;
-
-	return phy_write(phydev, IP10XX_SPEC_CTRL_STATUS, c);
-}
-
 static int ip175c_read_status(struct phy_device *phydev)
 {
 	if (phydev->mdio.addr == 4) /* WAN port */
@@ -196,6 +181,21 @@ static int ip175c_config_aneg(struct phy_device *phydev)
 	return 0;
 }
 
+static int ip101a_g_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, IP10XX_SPEC_CTRL_STATUS);
+	c |= IP101A_G_APS_ON;
+
+	return phy_write(phydev, IP10XX_SPEC_CTRL_STATUS, c);
+}
+
 static int ip101a_g_config_intr(struct phy_device *phydev)
 {
 	u16 val;
-- 
2.19.1

[PATCH 4/7] net: phy: icplus: use the BIT macro where possible

[Martin Blumenstingl]

This makes the code consistent by using the BIT() macro instead of
manual bit-shifting for some of the fields. No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/icplus.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 3d3e9134c762..3ec470adde3d 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -36,11 +36,11 @@ MODULE_LICENSE("GPL");
 
 /* IP101A/G - IP1001 */
 #define IP10XX_SPEC_CTRL_STATUS		16	/* Spec. Control Register */
-#define IP1001_RXPHASE_SEL		(1<<0)	/* Add delay on RX_CLK */
-#define IP1001_TXPHASE_SEL		(1<<1)	/* Add delay on TX_CLK */
+#define IP1001_RXPHASE_SEL		BIT(0)	/* Add delay on RX_CLK */
+#define IP1001_TXPHASE_SEL		BIT(1)	/* Add delay on TX_CLK */
 #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			2	/* IP101A/G APS Mode bit */
+#define IP101A_G_APS_ON			BIT(1)	/* IP101A/G APS Mode bit */
 #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_NO_IRQ			BIT(11) /* IRQ's inactive */
-- 
2.19.1

[PATCH 5/7] net: phy: icplus: rename IP101A_G_NO_IRQ to IP101A_G_IRQ_ALL_MASK

[Martin Blumenstingl]

The datasheet uses the name "All Mask" for this bit. Change the name of
our #define to be consistent with the datasheet. While here also replace
the tab between the #define and IP101A_G_IRQ_ALL_MASK with a space.
No functional changes.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 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 3ec470adde3d..c9489ec77cef 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -43,7 +43,7 @@ MODULE_LICENSE("GPL");
 #define IP101A_G_APS_ON			BIT(1)	/* IP101A/G APS Mode bit */
 #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_NO_IRQ			BIT(11) /* IRQ's inactive */
+#define IP101A_G_IRQ_ALL_MASK		BIT(11) /* IRQ's inactive */
 
 static int ip175c_config_init(struct phy_device *phydev)
 {
@@ -204,7 +204,7 @@ 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;
 	else
-		val = IP101A_G_NO_IRQ;
+		val = IP101A_G_IRQ_ALL_MASK;
 
 	return phy_write(phydev, IP101A_G_IRQ_CONF_STATUS, val);
 }
-- 
2.19.1

[PATCH 6/7] net: phy: icplus: implement .did_interrupt for IP101A/G

[Martin Blumenstingl]

The IP101A_G_IRQ_CONF_STATUS register has bits to detect which
interrupts have fired. Implement the .did_interrupt callback to let the
PHY core know whether the interrupt was for this specific PHY.

This is useful for debugging interrupt problems with 32-pin IP101GR PHYs
where the interrupt line is shared with the RX_ERR (receive error
status) signal. The default values are:
- RX_ERR is enabled by default (LOW means that there is no receive
  error)
- the PHY's interrupt line is configured "active low" by default

Without any additional changes there is a flood of interrupts if the
RX_ERR/INTR32 signal is configured in RX_ERR mode (which is the
default). Having a did_interrupt ensures that the PHY core returns
IRQ_NONE instead of endlessly triggering the PHY state machine.
Additionally the kernel will report this after a while:
  irq 28: nobody cared (try booting with the "irqpoll" option)

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/icplus.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index c9489ec77cef..3dc8bbbe746b 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -44,6 +44,9 @@ MODULE_LICENSE("GPL");
 #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)
 
 static int ip175c_config_init(struct phy_device *phydev)
 {
@@ -209,6 +212,18 @@ static int ip101a_g_config_intr(struct phy_device *phydev)
 	return phy_write(phydev, IP101A_G_IRQ_CONF_STATUS, val);
 }
 
+static int ip101a_g_did_interrupt(struct phy_device *phydev)
+{
+	int val = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);
+
+	if (val < 0)
+		return 0;
+
+	return val & (IP101A_G_IRQ_SPEED_CHANGE |
+		      IP101A_G_IRQ_DUPLEX_CHANGE |
+		      IP101A_G_IRQ_LINK_CHANGE);
+}
+
 static int ip101a_g_ack_interrupt(struct phy_device *phydev)
 {
 	int err = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);
@@ -243,6 +258,7 @@ static struct phy_driver icplus_driver[] = {
 	.phy_id_mask	= 0x0ffffff0,
 	.features	= PHY_BASIC_FEATURES,
 	.config_intr	= ip101a_g_config_intr,
+	.did_interrupt	= ip101a_g_did_interrupt,
 	.ack_interrupt	= ip101a_g_ack_interrupt,
 	.config_init	= &ip101a_g_config_init,
 	.suspend	= genphy_suspend,
-- 
2.19.1

[PATCH 7/7] net: phy: icplus: allow configuring the interrupt function on IP101GR

[Martin Blumenstingl]

The IP101GR is a 32-pin QFN package variant of the IP101G/IP101GA
Ethernet PHY. Due to it's limited amount of pins the RXER (receive
error) and INTR32 (interrupt) functions share pin 21.
By default the PHY is configured to output the "receive error" status on
pin 21. Depending on the board layout and requirements we may want to
re-configure the PHY to output the interrupt signal there.

The mode of pin 21 can be configured in the "Digital I/O Specific
Control Register" (register 29), bit 2:
- 0 = RXER function
- 1 = INTR(32) function

Depending on the devicetree configuration we will now:
- change the mode to either ther RXER or INTR32 function
- keep the SEL_INTR32 value set by the bootloader (default) if no
  configuration is provided (to ensure that we're not breaking existing
  boards)

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/net/phy/icplus.c | 71 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 3dc8bbbe746b..a27e15cb3366 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -25,6 +25,7 @@
 #include <linux/mii.h>
 #include <linux/ethtool.h>
 #include <linux/phy.h>
+#include <linux/property.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -48,6 +49,23 @@ MODULE_LICENSE("GPL");
 #define IP101A_G_IRQ_DUPLEX_CHANGE	BIT(1)
 #define IP101A_G_IRQ_LINK_CHANGE	BIT(0)
 
+#define IP101G_DIGITAL_IO_SPEC_CTRL			0x1d
+#define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32		BIT(2)
+
+/* 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.
+ */
+enum ip101gr_sel_intr32 {
+	IP101GR_SEL_INTR32_KEEP,
+	IP101GR_SEL_INTR32_INTR,
+	IP101GR_SEL_INTR32_RXER,
+};
+
+struct ip101a_g_phy_priv {
+	enum ip101gr_sel_intr32 sel_intr32;
+};
+
 static int ip175c_config_init(struct phy_device *phydev)
 {
 	int err, i;
@@ -184,14 +202,64 @@ static int ip175c_config_aneg(struct phy_device *phydev)
 	return 0;
 }
 
+static int ip101a_g_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct ip101a_g_phy_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	if (device_property_read_bool(dev, "icplus,select-rx-error"))
+		priv->sel_intr32 = IP101GR_SEL_INTR32_RXER;
+	else if (device_property_read_bool(dev, "icplus,select-interrupt"))
+		priv->sel_intr32 = IP101GR_SEL_INTR32_INTR;
+	else
+		priv->sel_intr32 = IP101GR_SEL_INTR32_KEEP;
+
+	phydev->priv = priv;
+
+	return 0;
+}
+
 static int ip101a_g_config_init(struct phy_device *phydev)
 {
-	int c;
+	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:
+		err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
+				 IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
+		if (err < 0)
+			return err;
+		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);
+		if (err < 0)
+			return err;
+		break;
+
+	default:
+		/* Don't touch IP101G_DIGITAL_IO_SPEC_CTRL because it's not
+		 * documented on IP101A and it's not clear whether this would
+		 * cause problems.
+		 * For the 32-pin IP101GR we simply keep the SEL_INTR32
+		 * configuration as set by the bootloader when not configured
+		 * to one of the special functions.
+		 */
+		break;
+	}
+
 	/* Enable Auto Power Saving mode */
 	c = phy_read(phydev, IP10XX_SPEC_CTRL_STATUS);
 	c |= IP101A_G_APS_ON;
@@ -257,6 +325,7 @@ static struct phy_driver icplus_driver[] = {
 	.name		= "ICPlus IP101A/G",
 	.phy_id_mask	= 0x0ffffff0,
 	.features	= PHY_BASIC_FEATURES,
+	.probe		= ip101a_g_probe,
 	.config_intr	= ip101a_g_config_intr,
 	.did_interrupt	= ip101a_g_did_interrupt,
 	.ack_interrupt	= ip101a_g_ack_interrupt,
-- 
2.19.1

Re: [PATCH 2/7] dt-bindings: net: phy: add bindings for the IC Plus Corp. IP101A/G PHYs

[Andrew Lunn]

On Sat, Nov 17, 2018 at 07:20:02PM +0100, Martin Blumenstingl wrote:
> The IP101A and IP101G series both have various models. Depending on the
> board implementation we need a special property for the IP101GR (32-pin
> LQFP package) PHY:
> pin 21 ("RXER/INTR_32") outputs the "receive error" signal by default
> (LOW means "normal operation", HIGH means that there's either a decoding
> error of the received signal or that the PHY is receiving LPI). This pin
> can also be switched to INTR32 mode, where the interrupt signal is
> routed to this pin. The other PHYs don't need this special handling
> because they have more pins available so the interrupt function gets a
> dedicated pin.
> 
> This adds two properties to either select the "receive error" or
> "interrupt" function of pin 21. Not specifying any function means that
> the default set by the bootloader is used. This is required because the
> IP101GR cannot be differentiated between other IP101 PHYs as the PHY
> identification registers on all of these is 0x02430c54.

Hi Martin

Not being able to identify the device is a real problem here.

I did wonder about adding a property which tells you if this is the R
variant, but the binding you suggests seems equally good.

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

    Andrew

Re: [PATCH 3/7] net: phy: icplus: keep all ip101a_g functions together

[Andrew Lunn]

On Sat, Nov 17, 2018 at 07:20:03PM +0100, Martin Blumenstingl wrote:
> This simply moves ip101a_g_config_init right above
> ip101a_g_config_intr so all functions for the ICPlus IP101A/G PHYs are
> grouped together.
> No functional changes.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

    Andrew

Re: [PATCH 4/7] net: phy: icplus: use the BIT macro where possible

[Andrew Lunn]

On Sat, Nov 17, 2018 at 07:20:04PM +0100, Martin Blumenstingl wrote:
> This makes the code consistent by using the BIT() macro instead of
> manual bit-shifting for some of the fields. No functional changes.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

    Andrew

Re: [PATCH 5/7] net: phy: icplus: rename IP101A_G_NO_IRQ to IP101A_G_IRQ_ALL_MASK

[Andrew Lunn]

On Sat, Nov 17, 2018 at 07:20:05PM +0100, Martin Blumenstingl wrote:
> The datasheet uses the name "All Mask" for this bit. Change the name of
> our #define to be consistent with the datasheet. While here also replace
> the tab between the #define and IP101A_G_IRQ_ALL_MASK with a space.
> No functional changes.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

I think this is the bit which confused Florian. Nice change, it makes
it a lot clearer what is going on.

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

    Andrew

Re: [PATCH 6/7] net: phy: icplus: implement .did_interrupt for IP101A/G

[Andrew Lunn]

On Sat, Nov 17, 2018 at 07:20:06PM +0100, Martin Blumenstingl wrote:
> The IP101A_G_IRQ_CONF_STATUS register has bits to detect which
> interrupts have fired. Implement the .did_interrupt callback to let the
> PHY core know whether the interrupt was for this specific PHY.
> 
> This is useful for debugging interrupt problems with 32-pin IP101GR PHYs
> where the interrupt line is shared with the RX_ERR (receive error
> status) signal. The default values are:
> - RX_ERR is enabled by default (LOW means that there is no receive
>   error)
> - the PHY's interrupt line is configured "active low" by default
> 
> Without any additional changes there is a flood of interrupts if the
> RX_ERR/INTR32 signal is configured in RX_ERR mode (which is the
> default). Having a did_interrupt ensures that the PHY core returns
> IRQ_NONE instead of endlessly triggering the PHY state machine.
> Additionally the kernel will report this after a while:
>   irq 28: nobody cared (try booting with the "irqpoll" option)

That is a useful hint.

> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

    Andrew

Re: [PATCH 7/7] net: phy: icplus: allow configuring the interrupt function on IP101GR

[Andrew Lunn]

Hi Martin

> +static int ip101a_g_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct ip101a_g_phy_priv *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	if (device_property_read_bool(dev, "icplus,select-rx-error"))
> +		priv->sel_intr32 = IP101GR_SEL_INTR32_RXER;
> +	else if (device_property_read_bool(dev, "icplus,select-interrupt"))
> +		priv->sel_intr32 = IP101GR_SEL_INTR32_INTR;
> +	else
> +		priv->sel_intr32 = IP101GR_SEL_INTR32_KEEP;

It would be good to return -EINVAL if both properties are found.

This looks good otherwise.

     Andrew

Re: [PATCH 2/7] dt-bindings: net: phy: add bindings for the IC Plus Corp. IP101A/G PHYs

[Martin Blumenstingl]

Hi Andrew,

On Sun, Nov 18, 2018 at 6:03 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sat, Nov 17, 2018 at 07:20:02PM +0100, Martin Blumenstingl wrote:
> > The IP101A and IP101G series both have various models. Depending on the
> > board implementation we need a special property for the IP101GR (32-pin
> > LQFP package) PHY:
> > pin 21 ("RXER/INTR_32") outputs the "receive error" signal by default
> > (LOW means "normal operation", HIGH means that there's either a decoding
> > error of the received signal or that the PHY is receiving LPI). This pin
> > can also be switched to INTR32 mode, where the interrupt signal is
> > routed to this pin. The other PHYs don't need this special handling
> > because they have more pins available so the interrupt function gets a
> > dedicated pin.
> >
> > This adds two properties to either select the "receive error" or
> > "interrupt" function of pin 21. Not specifying any function means that
> > the default set by the bootloader is used. This is required because the
> > IP101GR cannot be differentiated between other IP101 PHYs as the PHY
> > identification registers on all of these is 0x02430c54.
>
> Hi Martin
>
> Not being able to identify the device is a real problem here.
indeed, even some extra "custom version" register would have helped

> I did wonder about adding a property which tells you if this is the R
> variant, but the binding you suggests seems equally good.
I decided against that because I *believe* (I have no evidence though)
that the IP101G could have the same problem in theory.
according to the datasheet I have the IP101G is a "Dice" (which I'm
interpreting as "raw die, no package")
<some company> could make a multi-chip package with the IP101G and
only route the RXER/INTR_32 to the outside
due to lack of evidence I decided not to document this in the
dt-bindings themselves, but if you want I can add this theory to the
patch description

apart from that: thank you for reviewing this series!


Regards
Martin

Re: [PATCH 7/7] net: phy: icplus: allow configuring the interrupt function on IP101GR

[Martin Blumenstingl]

Hi Andrew,

On Sun, Nov 18, 2018 at 6:13 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Hi Martin
>
> > +static int ip101a_g_probe(struct phy_device *phydev)
> > +{
> > +     struct device *dev = &phydev->mdio.dev;
> > +     struct ip101a_g_phy_priv *priv;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     if (device_property_read_bool(dev, "icplus,select-rx-error"))
> > +             priv->sel_intr32 = IP101GR_SEL_INTR32_RXER;
> > +     else if (device_property_read_bool(dev, "icplus,select-interrupt"))
> > +             priv->sel_intr32 = IP101GR_SEL_INTR32_INTR;
> > +     else
> > +             priv->sel_intr32 = IP101GR_SEL_INTR32_KEEP;
>
> It would be good to return -EINVAL if both properties are found.
that makes sense
I'll wait a few days for more feedback and re-send this series with
that issue fixed

> This looks good otherwise.
great, thanks for looking into it!


Regards
Martin

Re: [PATCH 7/7] net: phy: icplus: allow configuring the interrupt function on IP101GR

[Andrew Lunn]

> I'll wait a few days for more feedback and re-send this series with
> that issue fixed

Hi Martin

In networking world, you should expect feedback within 3 days. So i
was actually a bit slow. If i'd of waited much longer, David would of
merged the patches very soon.

So feel free to respin the patchset now if you want.

   Andrew

Re: [PATCH 7/7] net: phy: icplus: allow configuring the interrupt function on IP101GR

[Martin Blumenstingl]

On Sun, Nov 18, 2018 at 6:45 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > I'll wait a few days for more feedback and re-send this series with
> > that issue fixed
>
> Hi Martin
>
> In networking world, you should expect feedback within 3 days. So i
> was actually a bit slow. If i'd of waited much longer, David would of
> merged the patches very soon.
good to know, thanks
for me as "someone who sends patches" (and as someone who is impatient
sometimes... ;)) this is great because I don't have to wait ages for
feedback!

> So feel free to respin the patchset now if you want.
I just sent v2


Regards
Martin