[PATCH net-next v4 0/4] DP83TD510 Single Pair 10Mbps Ethernet PHY

[PATCH net-next v4 0/4] DP83TD510 Single Pair 10Mbps Ethernet PHY

[Dan Murphy]

Hello

The DP83TD510 is an Ethernet PHY supporting single pair of twisted wires. The
PHY is capable of 10Mbps communication over long distances and exceeds the
IEEE 802.3cg 10BASE-T1L single-pair Ethernet specification.  The PHY supports
various voltage level signalling and can be forced to support a specific
voltage or allowed to perfrom auto negotiation on the voltage level. The
default for the PHY is auto negotiation but if the PHY is forced to a specific
voltage then the LP must also support the same voltage.

Add the 10BASE-T1L linkmodes for ethtool to properly advertise the PHY's
capability.

Dan

Dan Murphy (4):
  ethtool: Add 10base-T1L link mode entries
  dt-bindings: net: Add Rx/Tx output configuration for 10base T1L
  dt-bindings: dp83td510: Add binding for DP83TD510 Ethernet PHY
  net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY

 .../devicetree/bindings/net/ethernet-phy.yaml |   6 +
 .../devicetree/bindings/net/ti,dp83td510.yaml |  64 +++
 drivers/net/phy/Kconfig                       |   6 +
 drivers/net/phy/Makefile                      |   1 +
 drivers/net/phy/dp83td510.c                   | 505 ++++++++++++++++++
 drivers/net/phy/phy-core.c                    |   4 +-
 include/uapi/linux/ethtool.h                  |   2 +
 net/ethtool/common.c                          |   2 +
 net/ethtool/linkmodes.c                       |   2 +
 9 files changed, 591 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/net/ti,dp83td510.yaml
 create mode 100644 drivers/net/phy/dp83td510.c

-- 
2.29.2

[PATCH net-next v4 1/4] ethtool: Add 10base-T1L link mode entries

[Dan Murphy]

Add entries for the 10base-T1L full and half duplex supported modes.

$ ethtool eth0
        Supported ports: [ TP ]
        Supported link modes:   10baseT1L/Half 10baseT1L/Full
        Supported pause frame use: Symmetric Receive-only
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  10baseT1L/Half 10baseT1L/Full
        Advertised pause frame use: No
        Advertised auto-negotiation: No
        Advertised FEC modes: Not reported
        Speed: 10Mb/s
        Duplex: Full
        Auto-negotiation: on
        Port: MII
        PHYAD: 1
        Transceiver: external
        Supports Wake-on: gs
        Wake-on: d
        SecureOn password: 00:00:00:00:00:00
        Current message level: 0x00000000 (0)

        Link detected: yes

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/phy-core.c   | 4 +++-
 include/uapi/linux/ethtool.h | 2 ++
 net/ethtool/common.c         | 2 ++
 net/ethtool/linkmodes.c      | 2 ++
 4 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 8d333d3084ed..616fae7f0c86 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -13,7 +13,7 @@
  */
 const char *phy_speed_to_str(int speed)
 {
-	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 92,
+	BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 94,
 		"Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
 		"If a speed or mode has been added please update phy_speed_to_str "
 		"and the PHY settings array.\n");
@@ -175,6 +175,8 @@ static const struct phy_setting settings[] = {
 	/* 10M */
 	PHY_SETTING(     10, FULL,     10baseT_Full		),
 	PHY_SETTING(     10, HALF,     10baseT_Half		),
+	PHY_SETTING(     10, FULL,     10baseT1L_Full		),
+	PHY_SETTING(     10, HALF,     10baseT1L_Half		),
 };
 #undef PHY_SETTING
 
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 9ca87bc73c44..16b6ea7548d3 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1619,6 +1619,8 @@ enum ethtool_link_mode_bit_indices {
 	ETHTOOL_LINK_MODE_400000baseCR4_Full_BIT	 = 89,
 	ETHTOOL_LINK_MODE_100baseFX_Half_BIT		 = 90,
 	ETHTOOL_LINK_MODE_100baseFX_Full_BIT		 = 91,
+	ETHTOOL_LINK_MODE_10baseT1L_Half_BIT		 = 92,
+	ETHTOOL_LINK_MODE_10baseT1L_Full_BIT		 = 93,
 	/* must be last entry */
 	__ETHTOOL_LINK_MODE_MASK_NBITS
 };
diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 24036e3055a1..95f87febc742 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -194,6 +194,8 @@ const char link_mode_names[][ETH_GSTRING_LEN] = {
 	__DEFINE_LINK_MODE_NAME(400000, CR4, Full),
 	__DEFINE_LINK_MODE_NAME(100, FX, Half),
 	__DEFINE_LINK_MODE_NAME(100, FX, Full),
+	__DEFINE_LINK_MODE_NAME(10, T1L, Half),
+	__DEFINE_LINK_MODE_NAME(10, T1L, Full),
 };
 static_assert(ARRAY_SIZE(link_mode_names) == __ETHTOOL_LINK_MODE_MASK_NBITS);
 
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index c5bcb9abc8b9..a8fab6fb1b30 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -264,6 +264,8 @@ static const struct link_mode_info link_mode_params[] = {
 	__DEFINE_LINK_MODE_PARAMS(400000, CR4, Full),
 	__DEFINE_LINK_MODE_PARAMS(100, FX, Half),
 	__DEFINE_LINK_MODE_PARAMS(100, FX, Full),
+	__DEFINE_LINK_MODE_PARAMS(10, T1L, Half),
+	__DEFINE_LINK_MODE_PARAMS(10, T1L, Full),
 };
 
 const struct nla_policy ethnl_linkmodes_set_policy[] = {
-- 
2.29.2

[PATCH net-next v4 2/4] dt-bindings: net: Add Rx/Tx output configuration for 10base T1L

[Dan Murphy]

Per the 802.3cg spec the 10base T1L can operate at 2 different
differential voltages 1v p2p and 2.4v p2p. The abiility of the PHY to
drive that output is dependent on the PHY's on board power supply.
This common feature is applicable to all 10base T1L PHYs so this binding
property belongs in a top level ethernet document.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 Documentation/devicetree/bindings/net/ethernet-phy.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
index 6dd72faebd89..bda1ce51836b 100644
--- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
@@ -174,6 +174,12 @@ properties:
       PHY's that have configurable TX internal delays. If this property is
       present then the PHY applies the TX delay.
 
+  max-tx-rx-p2p-microvolt:
+    description: |
+      Configures the Tx/Rx p2p differential output voltage for 10base-T1L PHYs.
+    enum: [ 1100, 2400 ]
+    default: 2400
+
 required:
   - reg
 
-- 
2.29.2

Re: [PATCH net-next v4 2/4] dt-bindings: net: Add Rx/Tx output configuration for 10base T1L

[Andrew Lunn]

On Tue, Nov 17, 2020 at 02:15:53PM -0600, Dan Murphy wrote:
> Per the 802.3cg spec the 10base T1L can operate at 2 different
> differential voltages 1v p2p and 2.4v p2p. The abiility of the PHY to

ability

> drive that output is dependent on the PHY's on board power supply.
> This common feature is applicable to all 10base T1L PHYs so this binding
> property belongs in a top level ethernet document.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  Documentation/devicetree/bindings/net/ethernet-phy.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index 6dd72faebd89..bda1ce51836b 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -174,6 +174,12 @@ properties:
>        PHY's that have configurable TX internal delays. If this property is
>        present then the PHY applies the TX delay.
>  
> +  max-tx-rx-p2p-microvolt:
> +    description: |
> +      Configures the Tx/Rx p2p differential output voltage for 10base-T1L PHYs.

Does it configure, or does it limit? I _think_ this is a negotiation
parameter, so the PHY might decide to do 1100mV if the link peer is
near by even when max-tx-rx-p2p-microvolt has the higher value.

     Andrew

Re: [PATCH net-next v4 2/4] dt-bindings: net: Add Rx/Tx output configuration for 10base T1L

[Dan Murphy]

Andrew

On 11/17/20 2:31 PM, Andrew Lunn wrote:
> On Tue, Nov 17, 2020 at 02:15:53PM -0600, Dan Murphy wrote:
>> Per the 802.3cg spec the 10base T1L can operate at 2 different
>> differential voltages 1v p2p and 2.4v p2p. The abiility of the PHY to
> ability
Ack
>
>> drive that output is dependent on the PHY's on board power supply.
>> This common feature is applicable to all 10base T1L PHYs so this binding
>> property belongs in a top level ethernet document.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>   Documentation/devicetree/bindings/net/ethernet-phy.yaml | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
>> index 6dd72faebd89..bda1ce51836b 100644
>> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
>> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
>> @@ -174,6 +174,12 @@ properties:
>>         PHY's that have configurable TX internal delays. If this property is
>>         present then the PHY applies the TX delay.
>>   
>> +  max-tx-rx-p2p-microvolt:
>> +    description: |
>> +      Configures the Tx/Rx p2p differential output voltage for 10base-T1L PHYs.
> Does it configure, or does it limit? I _think_ this is a negotiation
> parameter, so the PHY might decide to do 1100mV if the link peer is
> near by even when max-tx-rx-p2p-microvolt has the higher value.

For this device we can configure or force it to only work at 1.1v p2p 
otherwise 2.4 is the default.

But each LP's have to be configured for the same voltage. unless auto 
negotiation is on then it negotiates the voltage.

Dan

>
>       Andrew

Re: [PATCH net-next v4 2/4] dt-bindings: net: Add Rx/Tx output configuration for 10base T1L

[Rob Herring]

On Tue, Nov 17, 2020 at 02:15:53PM -0600, Dan Murphy wrote:
> Per the 802.3cg spec the 10base T1L can operate at 2 different
> differential voltages 1v p2p and 2.4v p2p. The abiility of the PHY to

1.1V?

> drive that output is dependent on the PHY's on board power supply.
> This common feature is applicable to all 10base T1L PHYs so this binding
> property belongs in a top level ethernet document.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  Documentation/devicetree/bindings/net/ethernet-phy.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> index 6dd72faebd89..bda1ce51836b 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml
> @@ -174,6 +174,12 @@ properties:
>        PHY's that have configurable TX internal delays. If this property is
>        present then the PHY applies the TX delay.
>  
> +  max-tx-rx-p2p-microvolt:
> +    description: |

Don't need '|' if no formatting.

> +      Configures the Tx/Rx p2p differential output voltage for 10base-T1L PHYs.
> +    enum: [ 1100, 2400 ]
> +    default: 2400

Aren't you off by 1000?

> +
>  required:
>    - reg
>  
> -- 
> 2.29.2
> 

[PATCH net-next v4 3/4] dt-bindings: dp83td510: Add binding for DP83TD510 Ethernet PHY

[Dan Murphy]

The DP83TD510 is a 10M single twisted pair Ethernet PHY

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../devicetree/bindings/net/ti,dp83td510.yaml | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/ti,dp83td510.yaml

diff --git a/Documentation/devicetree/bindings/net/ti,dp83td510.yaml b/Documentation/devicetree/bindings/net/ti,dp83td510.yaml
new file mode 100644
index 000000000000..d3c97bb4d820
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/ti,dp83td510.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2020 Texas Instruments Incorporated
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/net/ti,dp83td510.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: TI DP83TD510 ethernet PHY
+
+allOf:
+  - $ref: "ethernet-controller.yaml#"
+  - $ref: "ethernet-phy.yaml#"
+
+maintainers:
+  - Dan Murphy <dmurphy@ti.com>
+
+description: |
+  The PHY is an twisted pair 10Mbps Ethernet PHY that support MII, RMII and
+  RGMII interfaces.
+
+  Specifications about the Ethernet PHY can be found at:
+    http://www.ti.com/lit/ds/symlink/dp83td510e.pdf
+
+properties:
+  reg:
+    maxItems: 1
+
+  tx-fifo-depth:
+    description: |
+       Transmitt FIFO depth for RMII mode.  The PHY only exposes 4 nibble
+       depths. The valid nibble depths are 4, 5, 6 and 8.
+    enum: [ 4, 5, 6, 8 ]
+    default: 5
+
+  rx-internal-delay-ps:
+    description: |
+       Setting this property to a non-zero number sets the RX internal delay
+       for the PHY.  The internal delay for the PHY is fixed to 30ns relative
+       to receive data.
+
+  tx-internal-delay-ps:
+    description: |
+       Setting this property to a non-zero number sets the TX internal delay
+       for the PHY.  The internal delay for the PHY has a range of -4 to 4ns
+       relative to transmit data.
+
+unevaluatedProperties: false
+
+required:
+  - reg
+
+examples:
+  - |
+    mdio0 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      ethphy0: ethernet-phy@0 {
+        reg = <0>;
+        tx-rx-output-high;
+        tx-fifo-depth = <5>;
+        rx-internal-delay-ps = <1>;
+        tx-internal-delay-ps = <1>;
+      };
+    };
-- 
2.29.2

Re: [PATCH net-next v4 3/4] dt-bindings: dp83td510: Add binding for DP83TD510 Ethernet PHY

[Rob Herring]

On Tue, Nov 17, 2020 at 02:15:54PM -0600, Dan Murphy wrote:
> The DP83TD510 is a 10M single twisted pair Ethernet PHY
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  .../devicetree/bindings/net/ti,dp83td510.yaml | 64 +++++++++++++++++++
>  1 file changed, 64 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/ti,dp83td510.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,dp83td510.yaml b/Documentation/devicetree/bindings/net/ti,dp83td510.yaml
> new file mode 100644
> index 000000000000..d3c97bb4d820
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/ti,dp83td510.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2020 Texas Instruments Incorporated
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/net/ti,dp83td510.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: TI DP83TD510 ethernet PHY
> +
> +allOf:
> +  - $ref: "ethernet-controller.yaml#"
> +  - $ref: "ethernet-phy.yaml#"
> +
> +maintainers:
> +  - Dan Murphy <dmurphy@ti.com>
> +
> +description: |
> +  The PHY is an twisted pair 10Mbps Ethernet PHY that support MII, RMII and
> +  RGMII interfaces.
> +
> +  Specifications about the Ethernet PHY can be found at:
> +    http://www.ti.com/lit/ds/symlink/dp83td510e.pdf
> +
> +properties:
> +  reg:
> +    maxItems: 1
> +
> +  tx-fifo-depth:
> +    description: |
> +       Transmitt FIFO depth for RMII mode.  The PHY only exposes 4 nibble
> +       depths. The valid nibble depths are 4, 5, 6 and 8.
> +    enum: [ 4, 5, 6, 8 ]
> +    default: 5
> +
> +  rx-internal-delay-ps:
> +    description: |
> +       Setting this property to a non-zero number sets the RX internal delay
> +       for the PHY.  The internal delay for the PHY is fixed to 30ns relative
> +       to receive data.

I'm confused. The delay is 30ns +/- whatever is set here?

> +
> +  tx-internal-delay-ps:
> +    description: |
> +       Setting this property to a non-zero number sets the TX internal delay
> +       for the PHY.  The internal delay for the PHY has a range of -4 to 4ns
> +       relative to transmit data.

Sounds like constraints?

We do have a problem handling negative values though. Addressing in dtc 
was rejected, so we'll need to fixup the schema with unsigned values. 
But here it should just be negative values.

> +
> +unevaluatedProperties: false
> +
> +required:
> +  - reg
> +
> +examples:
> +  - |
> +    mdio0 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      ethphy0: ethernet-phy@0 {
> +        reg = <0>;
> +        tx-rx-output-high;
> +        tx-fifo-depth = <5>;
> +        rx-internal-delay-ps = <1>;
> +        tx-internal-delay-ps = <1>;
> +      };
> +    };
> -- 
> 2.29.2
> 

[PATCH net-next v4 4/4] net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY

[Dan Murphy]

The DP83TD510E is an ultra-low power Ethernet physical layer transceiver
that supports 10M single pair cable.

The device supports both 2.4-V p2p and 1-V p2p output voltage as defined
by IEEE 802.3cg 10Base-T1L specfications. These modes can be forced via
the device tree or the device is defaulted to auto negotiation to
determine the proper p2p voltage.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---

v4 - Considerable rework of the code after secondary test setup was created.
This version also uses the handle_interrupt call back and reduces the
configuration arrays as it was determined that 80% of the array was the same.

 drivers/net/phy/Kconfig     |   6 +
 drivers/net/phy/Makefile    |   1 +
 drivers/net/phy/dp83td510.c | 505 ++++++++++++++++++++++++++++++++++++
 3 files changed, 512 insertions(+)
 create mode 100644 drivers/net/phy/dp83td510.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 698bea312adc..017252e1504c 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -302,6 +302,12 @@ config DP83869_PHY
 	  Currently supports the DP83869 PHY.  This PHY supports copper and
 	  fiber connections.
 
+config DP83TD510_PHY
+	tristate "Texas Instruments DP83TD510 10M Single Pair Ethernet PHY"
+	help
+	  Support for the DP83TD510 Ethernet PHY. This PHY supports a 10M single
+	  pair Ethernet connection.
+
 config VITESSE_PHY
 	tristate "Vitesse PHYs"
 	help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index a13e402074cf..bf62ce211eb4 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -56,6 +56,7 @@ obj-$(CONFIG_DP83848_PHY)	+= dp83848.o
 obj-$(CONFIG_DP83867_PHY)	+= dp83867.o
 obj-$(CONFIG_DP83869_PHY)	+= dp83869.o
 obj-$(CONFIG_DP83TC811_PHY)	+= dp83tc811.o
+obj-$(CONFIG_DP83TD510_PHY)	+= dp83td510.o
 obj-$(CONFIG_FIXED_PHY)		+= fixed_phy.o
 obj-$(CONFIG_ICPLUS_PHY)	+= icplus.o
 obj-$(CONFIG_INTEL_XWAY_PHY)	+= intel-xway.o
diff --git a/drivers/net/phy/dp83td510.c b/drivers/net/phy/dp83td510.c
new file mode 100644
index 000000000000..a4456e0da447
--- /dev/null
+++ b/drivers/net/phy/dp83td510.c
@@ -0,0 +1,505 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Driver for the Texas Instruments DP83TD510 PHY
+ * Copyright (C) 2020 Texas Instruments Incorporated - https://www.ti.com/
+ */
+
+#include <linux/ethtool.h>
+#include <linux/etherdevice.h>
+#include <linux/kernel.h>
+#include <linux/mii.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy.h>
+#include <linux/netdevice.h>
+
+#define DP83TD510E_PHY_ID	0x20000180
+#define DP83TD510_DEVADDR_AN	0x7
+#define DP83TD510_DEVADDR	0x1f
+#define DP83TD510_PMD_DEVADDR	0x1
+
+#define DP83TD510_PHY_STAT	0x10
+#define DP83TD510_GEN_CFG	0x11
+#define DP83TD510_INT_REG1	0x12
+#define DP83TD510_INT_REG2	0x13
+#define DP83TD510_MAC_CFG_1	0x17
+#define DP83TD510_CTRL_REG	0x1f
+
+#define DP83TD510_ANEG_CTRL	0x200
+#define DP83TD510_PMD_CTRL	0x834
+#define DP83TD510_M_S_CTRL	0x8f6
+
+#define DP83TD510_SOR_1		0x467
+
+#define DP83TD510_HW_RESET	BIT(15)
+#define DP83TD510_SW_RESET	BIT(14)
+
+#define DP83TD510_LINK_STS	BIT(0)
+
+/* GEN CFG bits */
+#define DP83TD510_INT_OE	BIT(0)
+#define DP83TD510_INT_EN	BIT(1)
+
+/* INT REG 1 bits */
+#define DP83TD510_INT1_ESD_EN	BIT(3)
+#define DP83TD510_INT1_LINK_EN	BIT(5)
+#define DP83TD510_INT1_RHF_EN	BIT(7)
+#define DP83TD510_INT1_ESD	BIT(11)
+#define DP83TD510_INT1_LINK	BIT(13)
+#define DP83TD510_INT1_RHF	BIT(15)
+
+/* INT REG 2 bits */
+#define DP83TD510_INT2_POR_EN	BIT(0)
+#define DP83TD510_INT2_POL_EN	BIT(1)
+#define DP83TD510_INT2_PAGE_EN	BIT(5)
+#define DP83TD510_INT2_POR	BIT(8)
+#define DP83TD510_INT2_POL	BIT(9)
+#define DP83TD510_INT2_PAGE	BIT(13)
+
+/* MAC CFG bits */
+#define DP83TD510_RX_CLK_SHIFT	BIT(12)
+#define DP83TD510_TX_CLK_SHIFT	BIT(11)
+
+#define DP83TD510_MASTER_MODE	BIT(14)
+#define DP83TD510_AUTO_NEG_EN	BIT(12)
+#define DP83TD510_RGMII		BIT(8)
+
+#define DP83TD510_FIFO_DEPTH_MASK	GENMASK(6, 5)
+#define DP83TD510_FIFO_DEPTH_4_B_NIB	0
+#define DP83TD510_FIFO_DEPTH_5_B_NIB	BIT(5)
+#define DP83TD510_FIFO_DEPTH_6_B_NIB	BIT(6)
+#define DP83TD510_FIFO_DEPTH_8_B_NIB	(BIT(5) | BIT(6))
+
+#define DP83TD510_2_4V		BIT(12)
+#define DP83TD510_2_4V_P2P	2400
+#define DP83TD510_1_1V_P2P	1100
+#define DP83TD510_AUTO_NEG_P2P	0
+
+const int dp83td510_feature_array[4] = {
+	ETHTOOL_LINK_MODE_Autoneg_BIT,
+	ETHTOOL_LINK_MODE_10baseT1L_Half_BIT,
+	ETHTOOL_LINK_MODE_10baseT1L_Full_BIT,
+	ETHTOOL_LINK_MODE_TP_BIT,
+};
+
+struct dp83td510_private {
+	u32 hi_diff_output;
+	u32 tx_fifo_depth;
+	u32 rgmii_delay;
+	bool is_rgmii;
+};
+
+struct dp83td510_init_reg {
+	int reg;
+	int val;
+};
+
+static struct dp83td510_init_reg dp83td510_errata[] = {
+	{ 0x608, 0x003b }, /* disable_0_transition */
+	{ 0x862, 0x39f8 }, /* AGC Gain during Autoneg */
+	{ 0x81a, 0x67c0 }, /* deq offset for 1V swing */
+	{ 0x81c, 0xfb62 }, /* deq offset for 2.4V swing */
+	{ 0x830, 0x05a3 }, /* Enable energy lost fallback */
+	{ 0x855, 0x1b55 }, /* MSE Threshold change */
+	{ 0x831, 0x0403 }, /* energy detect threshold */
+	{ 0x856, 0x1800 }, /* good1 MSE threshold change */
+	{ 0x857, 0x8fa0 }, /* Enable fallback to phase 1 on watchdog trigger */
+	{ 0x871, 0x000c }, /* TED input changed to slicer_in without FFE */
+	{ 0x883, 0x022e }, /* Enable Rx Filter for Short Cable detection */
+	{ 0x402, 0x1800 }, /* Adjust LD swing */
+	{ 0x878, 0x2248 }, /* Change PI up/down polarity */
+	{ 0x10c, 0x0008 }, /* tx filter coefficient */
+	{ 0x112, 0x1212 }, /* tx filter scaling factor */
+	{ 0x809, 0x5c80 }, /* AGC retrain */
+	{ 0x803, 0x1529 }, /* Master Ph1 Back-off */
+	{ 0x804, 0x1a33 }, /* Master Ph1 Back-off */
+	{ 0x805, 0x1f3d }, /* Master Ph1 Back-off */
+	{ 0x850, 0x045b }, /* hybrid gain & delay */
+	{ 0x874, 0x6967 }, /* kp step 0 for master */
+	{ 0x852, 0x7800 }, /* FAGC init gain */
+	{ 0x806, 0x1e1e }, /* Master/Slave Ph2 Back-off */
+	{ 0x807, 0x2525 }, /* Master/Slave Ph2 Back-off */
+	{ 0x808, 0x2c2c }, /* Master/Slave Ph2 Back-off */
+	{ 0x850, 0x0590 }, /* Hybrid Gain/Delay Code */
+	{ 0x827, 0x4000 }, /* Echo Fixed Delay */
+	{ 0x849, 0x0fe4 }, /* Hybrid Cal enable */
+	{ 0x84b, 0x04b5 }, /* Echo Score Sel */
+	{ 0x018, 0x0043 }, /* CRS/RX_DV pin as RX_DV for RMII repeater mode */
+};
+
+static int dp83td510_ack_interrupt(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read(phydev, DP83TD510_INT_REG1);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_read(phydev, DP83TD510_INT_REG2);
+	if (ret < 0)
+		return ret;
+
+	phy_trigger_machine(phydev);
+
+	return 0;
+}
+
+static irqreturn_t dp83td510_handle_interrupt(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = dp83td510_ack_interrupt(phydev);
+	if (ret)
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static int dp83td510_config_intr(struct phy_device *phydev)
+{
+	int int_status;
+	int gen_cfg_val;
+	int ret;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		int_status = phy_read(phydev, DP83TD510_INT_REG1);
+		if (int_status < 0)
+			return int_status;
+
+		int_status = (DP83TD510_INT1_ESD_EN | DP83TD510_INT1_LINK_EN |
+			      DP83TD510_INT1_RHF_EN);
+
+		ret = phy_write(phydev, DP83TD510_INT_REG1, int_status);
+		if (ret)
+			return ret;
+
+		int_status = phy_read(phydev, DP83TD510_INT_REG2);
+		if (int_status < 0)
+			return int_status;
+
+		int_status = (DP83TD510_INT2_POR | DP83TD510_INT2_POL |
+				DP83TD510_INT2_PAGE);
+
+		ret = phy_write(phydev, DP83TD510_INT_REG2, int_status);
+		if (ret)
+			return ret;
+
+		gen_cfg_val = phy_read(phydev, DP83TD510_GEN_CFG);
+		if (gen_cfg_val < 0)
+			return gen_cfg_val;
+
+		gen_cfg_val |= DP83TD510_INT_OE | DP83TD510_INT_EN;
+
+	} else {
+		ret = phy_write(phydev, DP83TD510_INT_REG1, 0);
+		if (ret)
+			return ret;
+
+		ret = phy_write(phydev, DP83TD510_INT_REG2, 0);
+		if (ret)
+			return ret;
+
+		gen_cfg_val = phy_read(phydev, DP83TD510_GEN_CFG);
+		if (gen_cfg_val < 0)
+			return gen_cfg_val;
+
+		gen_cfg_val &= ~DP83TD510_INT_EN;
+	}
+
+	ret = phy_write(phydev, DP83TD510_GEN_CFG, gen_cfg_val);
+	if (ret)
+		return ret;
+
+	return dp83td510_ack_interrupt(phydev);
+}
+
+static int dp83td510_configure_mode(struct phy_device *phydev, int pmd_ctrl)
+{
+	struct dp83td510_private *dp83td510 = phydev->priv;
+	int ret;
+	int i;
+
+	ret = phy_set_bits(phydev, DP83TD510_CTRL_REG, DP83TD510_HW_RESET);
+	if (ret < 0)
+		return ret;
+
+	if (dp83td510->hi_diff_output == DP83TD510_2_4V_P2P)
+		ret = phy_write_mmd(phydev, DP83TD510_DEVADDR,
+				    DP83TD510_M_S_CTRL, DP83TD510_2_4V);
+	else
+		ret = phy_write_mmd(phydev, DP83TD510_DEVADDR,
+				    DP83TD510_M_S_CTRL, 0);
+
+	if (phydev->autoneg) {
+		ret = phy_modify_mmd(phydev, DP83TD510_DEVADDR_AN,
+				     DP83TD510_ANEG_CTRL,
+				     DP83TD510_AUTO_NEG_EN,
+				     DP83TD510_AUTO_NEG_EN);
+		if (ret)
+			return ret;
+	} else {
+		ret = phy_modify_mmd(phydev, DP83TD510_DEVADDR_AN,
+				     DP83TD510_ANEG_CTRL,
+				     DP83TD510_AUTO_NEG_EN, 0);
+		if (ret)
+			return ret;
+	}
+
+	ret = phy_modify_mmd(phydev, DP83TD510_PMD_DEVADDR,
+			     DP83TD510_PMD_CTRL,
+			     DP83TD510_MASTER_MODE, pmd_ctrl);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ARRAY_SIZE(dp83td510_errata); i++) {
+		ret = phy_write_mmd(phydev, DP83TD510_DEVADDR,
+				    dp83td510_errata[i].reg,
+				    dp83td510_errata[i].val);
+
+		if (ret)
+			return ret;
+	}
+
+	return phy_set_bits(phydev, DP83TD510_CTRL_REG, DP83TD510_SW_RESET);
+}
+
+static int dp83td510_set_master_slave(struct phy_device *phydev)
+{
+	int mst_slave_cfg;
+
+	mst_slave_cfg = phy_read_mmd(phydev, DP83TD510_PMD_DEVADDR,
+				     DP83TD510_PMD_CTRL);
+	if (mst_slave_cfg < 0)
+		return mst_slave_cfg;
+
+	if (mst_slave_cfg & DP83TD510_MASTER_MODE) {
+		if (phydev->autoneg) {
+			phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_PREFERRED;
+			phydev->master_slave_set = MASTER_SLAVE_CFG_MASTER_PREFERRED;
+		} else {
+			phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
+			phydev->master_slave_set = MASTER_SLAVE_CFG_MASTER_FORCE;
+		}
+	} else {
+		if (phydev->autoneg) {
+			phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_PREFERRED;
+			phydev->master_slave_set = MASTER_SLAVE_CFG_SLAVE_PREFERRED;
+		} else {
+			phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_FORCE;
+			phydev->master_slave_set = MASTER_SLAVE_CFG_SLAVE_FORCE;
+		}
+	}
+
+	return 0;
+}
+
+static int dp83td510_read_status(struct phy_device *phydev)
+{
+	int phy_status;
+	int phy_bmcr;
+
+	phy_status = phy_read(phydev, DP83TD510_PHY_STAT);
+	if (phy_status < 0)
+		return phy_status;
+
+	phy_bmcr = phy_read(phydev, MII_BMCR);
+	if (phy_bmcr < 0)
+		return phy_bmcr;
+
+	phydev->link = phy_status & DP83TD510_LINK_STS;
+	if (phydev->link) {
+		phydev->duplex = phy_bmcr & BMCR_FULLDPLX ? DUPLEX_FULL : DUPLEX_HALF;
+		phydev->speed = SPEED_10;
+	} else {
+		phydev->speed = SPEED_UNKNOWN;
+		phydev->duplex = DUPLEX_UNKNOWN;
+	}
+
+	return dp83td510_set_master_slave(phydev);
+}
+
+static int dp83td510_get_features(struct phy_device *phydev)
+{
+	linkmode_set_bit_array(dp83td510_feature_array,
+			       ARRAY_SIZE(dp83td510_feature_array),
+			       phydev->supported);
+
+	return 0;
+}
+
+static int dp83td510_config_aneg(struct phy_device *phydev)
+{
+	int pmd_ctrl;
+
+	switch (phydev->master_slave_set) {
+	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
+	case MASTER_SLAVE_CFG_MASTER_FORCE:
+		pmd_ctrl = DP83TD510_MASTER_MODE;
+		break;
+	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
+	case MASTER_SLAVE_CFG_SLAVE_FORCE:
+		pmd_ctrl = 0;
+		break;
+	case MASTER_SLAVE_CFG_UNKNOWN:
+	case MASTER_SLAVE_CFG_UNSUPPORTED:
+	default:
+		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+		return -ENOTSUPP;
+	}
+
+	return dp83td510_configure_mode(phydev, pmd_ctrl);
+}
+
+static int dp83td510_config_init(struct phy_device *phydev)
+{
+	struct dp83td510_private *dp83td510 = phydev->priv;
+	int ret = 0;
+
+	if (phy_interface_is_rgmii(phydev)) {
+		if (dp83td510->rgmii_delay) {
+			ret = phy_set_bits_mmd(phydev, DP83TD510_DEVADDR,
+					       DP83TD510_MAC_CFG_1,
+					       dp83td510->rgmii_delay);
+			if (ret)
+				return ret;
+		}
+	}
+
+	if (phydev->interface == PHY_INTERFACE_MODE_RMII) {
+		ret = phy_modify(phydev, DP83TD510_GEN_CFG,
+				 DP83TD510_FIFO_DEPTH_MASK,
+				 dp83td510->tx_fifo_depth);
+		if (ret)
+			return ret;
+	}
+
+	return dp83td510_set_master_slave(phydev);
+}
+
+static int dp83td510_phy_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_set_bits(phydev, DP83TD510_CTRL_REG, DP83TD510_SW_RESET);
+	if (ret < 0)
+		return ret;
+
+	usleep_range(10, 20);
+
+	return dp83td510_config_init(phydev);
+}
+
+#if IS_ENABLED(CONFIG_OF_MDIO)
+static int dp83td510_of_init(struct phy_device *phydev)
+{
+	struct dp83td510_private *dp83td510 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	struct device_node *of_node = dev->of_node;
+	s32 rx_int_delay;
+	s32 tx_int_delay;
+	int ret;
+
+	if (!of_node)
+		return -ENODEV;
+
+	ret = of_property_read_u32(phydev->mdio.dev.of_node,
+				   "max-tx-rx-p2p-microvolt",
+				   &dp83td510->hi_diff_output);
+	if (ret)
+		dp83td510->hi_diff_output = DP83TD510_2_4V_P2P;
+
+	if (dp83td510->hi_diff_output != DP83TD510_2_4V_P2P &&
+	    dp83td510->hi_diff_output != DP83TD510_1_1V_P2P)
+		return -EINVAL;
+
+	if (of_property_read_u32(phydev->mdio.dev.of_node, "tx-fifo-depth",
+				 &dp83td510->tx_fifo_depth))
+		dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB;
+
+	switch (dp83td510->tx_fifo_depth) {
+	case 4:
+		dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_4_B_NIB;
+		break;
+	case 6:
+		dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_6_B_NIB;
+		break;
+	case 8:
+		dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_8_B_NIB;
+		break;
+	case 5:
+	default:
+		dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB;
+	}
+
+	rx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0, true);
+	if (rx_int_delay <= 0)
+		dp83td510->rgmii_delay = DP83TD510_RX_CLK_SHIFT;
+	else
+		dp83td510->rgmii_delay = 0;
+
+	tx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0, false);
+	if (tx_int_delay <= 0)
+		dp83td510->rgmii_delay |= DP83TD510_TX_CLK_SHIFT;
+	else
+		dp83td510->rgmii_delay &= ~DP83TD510_TX_CLK_SHIFT;
+
+	return 0;
+}
+#else
+static int dp83869_of_init(struct phy_device *phydev)
+{
+	dp83td510->hi_diff_output = DP83TD510_2_4V_P2P
+	dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB
+	return 0;
+}
+#endif /* CONFIG_OF_MDIO */
+
+static int dp83td510_probe(struct phy_device *phydev)
+{
+	struct dp83td510_private *dp83td510;
+	int ret;
+
+	dp83td510 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83td510),
+				 GFP_KERNEL);
+	if (!dp83td510)
+		return -ENOMEM;
+
+	phydev->priv = dp83td510;
+
+	ret = dp83td510_of_init(phydev);
+	if (ret)
+		return ret;
+
+	return dp83td510_config_init(phydev);
+}
+
+static struct phy_driver dp83td510_driver[] = {
+	{
+		PHY_ID_MATCH_MODEL(DP83TD510E_PHY_ID),
+		.name		= "TI DP83TD510E",
+		.probe          = dp83td510_probe,
+		.config_init	= dp83td510_config_init,
+		.soft_reset	= dp83td510_phy_reset,
+
+		/* IRQ related */
+		.handle_interrupt = dp83td510_handle_interrupt,
+		.config_intr	= dp83td510_config_intr,
+		.config_aneg	= dp83td510_config_aneg,
+		.read_status	= dp83td510_read_status,
+
+		.get_features	= dp83td510_get_features,
+
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+	},
+};
+module_phy_driver(dp83td510_driver);
+
+static struct mdio_device_id __maybe_unused dp83td510_tbl[] = {
+	{ PHY_ID_MATCH_MODEL(DP83TD510E_PHY_ID) },
+	{ }
+};
+MODULE_DEVICE_TABLE(mdio, dp83td510_tbl);
+
+MODULE_DESCRIPTION("Texas Instruments DP83TD510E PHY driver");
+MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com");
+MODULE_LICENSE("GPL v2");
-- 
2.29.2

Re: [PATCH net-next v4 4/4] net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY

[Ioana Ciornei]

On Tue, Nov 17, 2020 at 02:15:55PM -0600, Dan Murphy wrote:
> The DP83TD510E is an ultra-low power Ethernet physical layer transceiver
> that supports 10M single pair cable.
> 
> The device supports both 2.4-V p2p and 1-V p2p output voltage as defined
> by IEEE 802.3cg 10Base-T1L specfications. These modes can be forced via
> the device tree or the device is defaulted to auto negotiation to
> determine the proper p2p voltage.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v4 - Considerable rework of the code after secondary test setup was created.
> This version also uses the handle_interrupt call back and reduces the
> configuration arrays as it was determined that 80% of the array was the same.
> 
>  drivers/net/phy/Kconfig     |   6 +
>  drivers/net/phy/Makefile    |   1 +
>  drivers/net/phy/dp83td510.c | 505 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 512 insertions(+)
>  create mode 100644 drivers/net/phy/dp83td510.c
> 

[snip]

> +static int dp83td510_ack_interrupt(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy_read(phydev, DP83TD510_INT_REG1);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_read(phydev, DP83TD510_INT_REG2);
> +	if (ret < 0)
> +		return ret;
> +
> +	phy_trigger_machine(phydev);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t dp83td510_handle_interrupt(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = dp83td510_ack_interrupt(phydev);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	return IRQ_HANDLED;
> +}

From what I can see in the datasheet, the INT_REG1 and INT_REG2 are used
for both interrupt configuration and interrupt status.

If this is the case, the state machine should only be triggered if the
interrupt was triggered (eg DP83TD510_INT1_LINK is set), not if _any_
bit from the register is set. This is broken since anytime you have
interrupts enabled, the lower half of the register will be non-zero
since that contains you interrupt enabled bits.

The .handle_interrupt() should look something like:

	ret = phy_read(phydev, DP83TD510_INT_REG1);
	if (ret < 0)
		return ret;
	
	if (!(ret & (DP83TD510_INT1_ESD | DP83TD510_INT1_LINK | DP83TD510_INT1_RHF)))
		return IRQ_NONE;

	ret = phy_read(phydev, DP83TD510_INT_REG2);
	if (ret < 0)
		return ret;
	
	if (!(ret & (DP83TD510_INT2_POR | DP83TD510_INT2_POL | DP83TD510_INT2_PAGE)))
		return IRQ_NONE;

	phy_trigger_machine(phydev);

	return IRQ_HANDLED;

> +
> +static int dp83td510_config_intr(struct phy_device *phydev)
> +{
> +	int int_status;
> +	int gen_cfg_val;
> +	int ret;
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +		int_status = phy_read(phydev, DP83TD510_INT_REG1);
> +		if (int_status < 0)
> +			return int_status;
> +
> +		int_status = (DP83TD510_INT1_ESD_EN | DP83TD510_INT1_LINK_EN |
> +			      DP83TD510_INT1_RHF_EN);
> +
> +		ret = phy_write(phydev, DP83TD510_INT_REG1, int_status);
> +		if (ret)
> +			return ret;
> +
> +		int_status = phy_read(phydev, DP83TD510_INT_REG2);
> +		if (int_status < 0)
> +			return int_status;
> +
> +		int_status = (DP83TD510_INT2_POR | DP83TD510_INT2_POL |
> +				DP83TD510_INT2_PAGE);
> +

Shouldn't you use DP83TD510_INT2_POR_EN, DP83TD510_INT2_POL_EN etc here?
It seems that you are setting up the bits corresponding with the
interrupt status and not the interrupt enable.

Ioana

Re: [PATCH net-next v4 4/4] net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY

[kernel test robot]

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

Hi Dan,

I love your patch! Yet something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83TD510-Single-Pair-10Mbps-Ethernet-PHY/20201118-041918
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 72308ecbf33b145641aba61071be31a85ebfd92c
config: arm-randconfig-m031-20201118 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
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
        # https://github.com/0day-ci/linux/commit/ec556cedbd640ecfa25713eadf48e5b7ee74fda3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Dan-Murphy/DP83TD510-Single-Pair-10Mbps-Ethernet-PHY/20201118-041918
        git checkout ec556cedbd640ecfa25713eadf48e5b7ee74fda3
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

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

All errors (new ones prefixed by >>):

   drivers/net/phy/dp83td510.c: In function 'dp83869_of_init':
>> drivers/net/phy/dp83td510.c:450:2: error: 'dp83td510' undeclared (first use in this function)
     450 |  dp83td510->hi_diff_output = DP83TD510_2_4V_P2P
         |  ^~~~~~~~~
   drivers/net/phy/dp83td510.c:450:2: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/net/phy/dp83td510.c:451:2: error: expected ';' before 'dp83td510'
     451 |  dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB
         |  ^~~~~~~~~
   drivers/net/phy/dp83td510.c:453:1: error: no return statement in function returning non-void [-Werror=return-type]
     453 | }
         | ^
   drivers/net/phy/dp83td510.c: In function 'dp83td510_probe':
>> drivers/net/phy/dp83td510.c:468:8: error: implicit declaration of function 'dp83td510_of_init'; did you mean 'dp83td510_config_init'? [-Werror=implicit-function-declaration]
     468 |  ret = dp83td510_of_init(phydev);
         |        ^~~~~~~~~~~~~~~~~
         |        dp83td510_config_init
   At top level:
   drivers/net/phy/dp83td510.c:448:12: warning: 'dp83869_of_init' defined but not used [-Wunused-function]
     448 | static int dp83869_of_init(struct phy_device *phydev)
         |            ^~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/dp83td510 +450 drivers/net/phy/dp83td510.c

   390	
   391	#if IS_ENABLED(CONFIG_OF_MDIO)
   392	static int dp83td510_of_init(struct phy_device *phydev)
   393	{
   394		struct dp83td510_private *dp83td510 = phydev->priv;
   395		struct device *dev = &phydev->mdio.dev;
   396		struct device_node *of_node = dev->of_node;
   397		s32 rx_int_delay;
   398		s32 tx_int_delay;
   399		int ret;
   400	
   401		if (!of_node)
   402			return -ENODEV;
   403	
   404		ret = of_property_read_u32(phydev->mdio.dev.of_node,
   405					   "max-tx-rx-p2p-microvolt",
   406					   &dp83td510->hi_diff_output);
   407		if (ret)
   408			dp83td510->hi_diff_output = DP83TD510_2_4V_P2P;
   409	
   410		if (dp83td510->hi_diff_output != DP83TD510_2_4V_P2P &&
   411		    dp83td510->hi_diff_output != DP83TD510_1_1V_P2P)
   412			return -EINVAL;
   413	
   414		if (of_property_read_u32(phydev->mdio.dev.of_node, "tx-fifo-depth",
   415					 &dp83td510->tx_fifo_depth))
   416			dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB;
   417	
   418		switch (dp83td510->tx_fifo_depth) {
   419		case 4:
   420			dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_4_B_NIB;
   421			break;
   422		case 6:
   423			dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_6_B_NIB;
   424			break;
   425		case 8:
   426			dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_8_B_NIB;
   427			break;
   428		case 5:
   429		default:
   430			dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB;
   431		}
   432	
   433		rx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0, true);
   434		if (rx_int_delay <= 0)
   435			dp83td510->rgmii_delay = DP83TD510_RX_CLK_SHIFT;
   436		else
   437			dp83td510->rgmii_delay = 0;
   438	
   439		tx_int_delay = phy_get_internal_delay(phydev, dev, NULL, 0, false);
   440		if (tx_int_delay <= 0)
   441			dp83td510->rgmii_delay |= DP83TD510_TX_CLK_SHIFT;
   442		else
   443			dp83td510->rgmii_delay &= ~DP83TD510_TX_CLK_SHIFT;
   444	
   445		return 0;
   446	}
   447	#else
   448	static int dp83869_of_init(struct phy_device *phydev)
   449	{
 > 450		dp83td510->hi_diff_output = DP83TD510_2_4V_P2P
 > 451		dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB
   452		return 0;
   453	}
   454	#endif /* CONFIG_OF_MDIO */
   455	
   456	static int dp83td510_probe(struct phy_device *phydev)
   457	{
   458		struct dp83td510_private *dp83td510;
   459		int ret;
   460	
   461		dp83td510 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83td510),
   462					 GFP_KERNEL);
   463		if (!dp83td510)
   464			return -ENOMEM;
   465	
   466		phydev->priv = dp83td510;
   467	
 > 468		ret = dp83td510_of_init(phydev);
   469		if (ret)
   470			return ret;
   471	
   472		return dp83td510_config_init(phydev);
   473	}
   474	

---
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: 34524 bytes --]

Re: [PATCH net-next v4 4/4] net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY

[Andrew Lunn]

> +static int dp83td510_config_init(struct phy_device *phydev)
> +{
> +	struct dp83td510_private *dp83td510 = phydev->priv;
> +	int ret = 0;
> +
> +	if (phy_interface_is_rgmii(phydev)) {

> +		if (dp83td510->rgmii_delay) {
> +			ret = phy_set_bits_mmd(phydev, DP83TD510_DEVADDR,
> +					       DP83TD510_MAC_CFG_1,
> +					       dp83td510->rgmii_delay);

Just to be safe, you should always write rgmii_delay, even if it is
zero. We have had too many bugs with RGMII delays which cause bad
backwards compatibility problems, so i would prefer to do a write
which might be unneeded, that find a bug here in a few years time.

> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	if (phydev->interface == PHY_INTERFACE_MODE_RMII) {
> +		ret = phy_modify(phydev, DP83TD510_GEN_CFG,
> +				 DP83TD510_FIFO_DEPTH_MASK,
> +				 dp83td510->tx_fifo_depth);

So there is no need to set the FIFO depth for the other three RGMII
modes? Or should this also be phy_interface_is_rgmii(phydev)?

> +#if IS_ENABLED(CONFIG_OF_MDIO)
> +static int dp83td510_of_init(struct phy_device *phydev)
> +{
> +	struct dp83td510_private *dp83td510 = phydev->priv;
> +	struct device *dev = &phydev->mdio.dev;
> +	struct device_node *of_node = dev->of_node;

You need to move this assignment to later in order to keep with
reverse christmas tree.

> +#else
> +static int dp83869_of_init(struct phy_device *phydev)
> +{
> +	dp83td510->hi_diff_output = DP83TD510_2_4V_P2P
> +	dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB

You don't have DT, so there is no fine control, but you still need to
do the basic 2ns delay as indicated by the phydev->interface value. So
i think you still need to set dp83td510->rgmii_delay depending on
which RGMII mode is requested.

      Andrew

Re: [PATCH net-next v4 4/4] net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY

[Dan Murphy]

Andrew

On 11/19/20 7:49 PM, Andrew Lunn wrote:
>> +static int dp83td510_config_init(struct phy_device *phydev)
>> +{
>> +	struct dp83td510_private *dp83td510 = phydev->priv;
>> +	int ret = 0;
>> +
>> +	if (phy_interface_is_rgmii(phydev)) {
>> +		if (dp83td510->rgmii_delay) {
>> +			ret = phy_set_bits_mmd(phydev, DP83TD510_DEVADDR,
>> +					       DP83TD510_MAC_CFG_1,
>> +					       dp83td510->rgmii_delay);
> Just to be safe, you should always write rgmii_delay, even if it is
> zero. We have had too many bugs with RGMII delays which cause bad
> backwards compatibility problems, so i would prefer to do a write
> which might be unneeded, that find a bug here in a few years time.

OK.


>
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
>> +
>> +	if (phydev->interface == PHY_INTERFACE_MODE_RMII) {
>> +		ret = phy_modify(phydev, DP83TD510_GEN_CFG,
>> +				 DP83TD510_FIFO_DEPTH_MASK,
>> +				 dp83td510->tx_fifo_depth);
> So there is no need to set the FIFO depth for the other three RGMII
> modes? Or should this also be phy_interface_is_rgmii(phydev)?

According to the data sheet the FIFO depth is for RMII.

"Fifo depth for RMII Tx fifo"

But I will ask the HW team for clarification.


>
>> +#if IS_ENABLED(CONFIG_OF_MDIO)
>> +static int dp83td510_of_init(struct phy_device *phydev)
>> +{
>> +	struct dp83td510_private *dp83td510 = phydev->priv;
>> +	struct device *dev = &phydev->mdio.dev;
>> +	struct device_node *of_node = dev->of_node;
> You need to move this assignment to later in order to keep with
> reverse christmas tree.
Well this is only used once so I will just remove the of_node declaration
>
>> +#else
>> +static int dp83869_of_init(struct phy_device *phydev)
>> +{
>> +	dp83td510->hi_diff_output = DP83TD510_2_4V_P2P
>> +	dp83td510->tx_fifo_depth = DP83TD510_FIFO_DEPTH_5_B_NIB
> You don't have DT, so there is no fine control, but you still need to
> do the basic 2ns delay as indicated by the phydev->interface value. So
> i think you still need to set dp83td510->rgmii_delay depending on
> which RGMII mode is requested.

The RGMII delay is fixed in the PHY.  The user can either turn it on or 
off. The default is 'off' which is 0.

I can explicitly set the rgmii_delay to 0 in non-OF cases.

Dan