[PATCH net-next 0/4] DP83869 Enhancements

[PATCH net-next 0/4] DP83869 Enhancements

[Dan Murphy]

Hello

These are improvements to the DP83869 Ethernet PHY driver.  OP-mode and port
mirroring may be strapped on the device but the software only retrives these
settings from the device tree.  Reading the straps and initializing the
associated stored variables so when setting the PHY up and down the PHY's
configuration values will be retained.

The PHY also supports RGMII internal delays.  Implement this feature as it
was done in the DP83867 device.

Dan Murphy (4):
  net: phy: dp83869: Update port-mirroring to read straps
  net: phy: dp83869: Set opmode from straps
  dt-bindings: net: Add RGMII internal delay for DP83869
  net: dp83869: Add RGMII internal delay configuration

 .../devicetree/bindings/net/ti,dp83869.yaml   |  16 +++
 drivers/net/phy/dp83869.c                     | 120 +++++++++++++++++-
 include/dt-bindings/net/ti-dp83869.h          |  18 +++
 3 files changed, 150 insertions(+), 4 deletions(-)

-- 
2.26.2

[PATCH net-next 1/4] net: phy: dp83869: Update port-mirroring to read straps

[Dan Murphy]

The device tree may not have the property set for port mirroring
because the hardware may have it strapped. If the property is not in the
DT then check the straps and set the port mirroring bit appropriately.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/dp83869.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 7996a4aea8d2..073a0f7754a5 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -66,6 +66,7 @@
 
 /* STRAP_STS1 bits */
 #define DP83869_STRAP_STS1_RESERVED		BIT(11)
+#define DP83869_STRAP_MIRROR_ENABLED           BIT(12)
 
 /* PHYCTRL bits */
 #define DP83869_RX_FIFO_SHIFT	12
@@ -191,10 +192,18 @@ static int dp83869_of_init(struct phy_device *phydev)
 	else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
 		dp83869->io_impedance = DP83869_IO_MUX_CFG_IO_IMPEDANCE_MIN;
 
-	if (of_property_read_bool(of_node, "enet-phy-lane-swap"))
+	if (of_property_read_bool(of_node, "enet-phy-lane-swap")) {
 		dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
-	else
-		dp83869->port_mirroring = DP83869_PORT_MIRRORING_DIS;
+	} else {
+		/* If the lane swap is not in the DT then check the straps */
+		ret = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
+		if (ret < 0)
+			return ret;
+		if (ret & DP83869_STRAP_MIRROR_ENABLED)
+			dp83869->port_mirroring = DP83869_PORT_MIRRORING_EN;
+		else
+			dp83869->port_mirroring = DP83869_PORT_MIRRORING_DIS;
+	}
 
 	if (of_property_read_u32(of_node, "rx-fifo-depth",
 				 &dp83869->rx_fifo_depth))
-- 
2.26.2

[PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[Dan Murphy]

If the op-mode for the device is not set in the device tree then set
the strapped op-mode and store it for later configuration.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/dp83869.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 073a0f7754a5..64fa2d911074 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -65,6 +65,7 @@
 #define DP83869_RGMII_RX_CLK_DELAY_EN		BIT(0)
 
 /* STRAP_STS1 bits */
+#define DP83869_STRAP_OP_MODE_MASK		GENMASK(2, 0)
 #define DP83869_STRAP_STS1_RESERVED		BIT(11)
 #define DP83869_STRAP_MIRROR_ENABLED           BIT(12)
 
@@ -161,6 +162,20 @@ static int dp83869_config_port_mirroring(struct phy_device *phydev)
 					  DP83869_CFG3_PORT_MIRROR_EN);
 }
 
+static int dp83869_set_strapped_mode(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+	u16 val;
+
+	val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
+	if (val < 0)
+		return val;
+
+	dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
+
+	return 0;
+}
+
 #ifdef CONFIG_OF_MDIO
 static int dp83869_of_init(struct phy_device *phydev)
 {
@@ -185,6 +200,10 @@ static int dp83869_of_init(struct phy_device *phydev)
 		if (dp83869->mode < DP83869_RGMII_COPPER_ETHERNET ||
 		    dp83869->mode > DP83869_SGMII_COPPER_ETHERNET)
 			return -EINVAL;
+	} else {
+		ret = dp83869_set_strapped_mode(phydev);
+		if (ret)
+			return ret;
 	}
 
 	if (of_property_read_bool(of_node, "ti,max-output-impedance"))
@@ -218,7 +237,7 @@ static int dp83869_of_init(struct phy_device *phydev)
 #else
 static int dp83869_of_init(struct phy_device *phydev)
 {
-	return 0;
+	return dp83869_set_strapped_mode(phydev);
 }
 #endif /* CONFIG_OF_MDIO */
 
-- 
2.26.2

[PATCH net-next 3/4] dt-bindings: net: Add RGMII internal delay for DP83869

[Dan Murphy]

Add the internal delay values into the header and update the binding
with the internal delay properties.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../devicetree/bindings/net/ti,dp83869.yaml    | 16 ++++++++++++++++
 include/dt-bindings/net/ti-dp83869.h           | 18 ++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83869.yaml b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
index 5b69ef03bbf7..344015ab9081 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83869.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83869.yaml
@@ -64,6 +64,20 @@ properties:
        Operational mode for the PHY.  If this is not set then the operational
        mode is set by the straps. see dt-bindings/net/ti-dp83869.h for values
 
+  ti,rx-internal-delay:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description: |
+      RGMII Receive Clock Delay - see dt-bindings/net/ti-dp83869.h
+      for applicable values. Required only if interface type is
+      PHY_INTERFACE_MODE_RGMII_ID or PHY_INTERFACE_MODE_RGMII_RXID.
+
+  ti,tx-internal-delay:
+    $ref: /schemas/types.yaml#definitions/uint32
+    description: |
+      RGMII Transmit Clock Delay - see dt-bindings/net/ti-dp83869.h
+      for applicable values. Required only if interface type is
+      PHY_INTERFACE_MODE_RGMII_ID or PHY_INTERFACE_MODE_RGMII_TXID.
+
 required:
   - reg
 
@@ -80,5 +94,7 @@ examples:
         ti,op-mode = <DP83869_RGMII_COPPER_ETHERNET>;
         ti,max-output-impedance = "true";
         ti,clk-output-sel = <DP83869_CLK_O_SEL_CHN_A_RCLK>;
+        ti,rx-internal-delay = <DP83869_RGMIIDCTL_2_25_NS>;
+        ti,tx-internal-delay = <DP83869_RGMIIDCTL_2_75_NS>;
       };
     };
diff --git a/include/dt-bindings/net/ti-dp83869.h b/include/dt-bindings/net/ti-dp83869.h
index 218b1a64e975..77d104a40f1f 100644
--- a/include/dt-bindings/net/ti-dp83869.h
+++ b/include/dt-bindings/net/ti-dp83869.h
@@ -16,6 +16,24 @@
 #define DP83869_PHYCR_FIFO_DEPTH_6_B_NIB	0x02
 #define DP83869_PHYCR_FIFO_DEPTH_8_B_NIB	0x03
 
+/* RGMIIDCTL internal delay for rx and tx */
+#define	DP83869_RGMIIDCTL_250_PS	0x0
+#define	DP83869_RGMIIDCTL_500_PS	0x1
+#define	DP83869_RGMIIDCTL_750_PS	0x2
+#define	DP83869_RGMIIDCTL_1_NS		0x3
+#define	DP83869_RGMIIDCTL_1_25_NS	0x4
+#define	DP83869_RGMIIDCTL_1_50_NS	0x5
+#define	DP83869_RGMIIDCTL_1_75_NS	0x6
+#define	DP83869_RGMIIDCTL_2_00_NS	0x7
+#define	DP83869_RGMIIDCTL_2_25_NS	0x8
+#define	DP83869_RGMIIDCTL_2_50_NS	0x9
+#define	DP83869_RGMIIDCTL_2_75_NS	0xa
+#define	DP83869_RGMIIDCTL_3_00_NS	0xb
+#define	DP83869_RGMIIDCTL_3_25_NS	0xc
+#define	DP83869_RGMIIDCTL_3_50_NS	0xd
+#define	DP83869_RGMIIDCTL_3_75_NS	0xe
+#define	DP83869_RGMIIDCTL_4_00_NS	0xf
+
 /* IO_MUX_CFG - Clock output selection */
 #define DP83869_CLK_O_SEL_CHN_A_RCLK		0x0
 #define DP83869_CLK_O_SEL_CHN_B_RCLK		0x1
-- 
2.26.2

[PATCH net-next 4/4] net: dp83869: Add RGMII internal delay configuration

[Dan Murphy]

Add RGMII internal delay configuration for Rx and Tx.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/net/phy/dp83869.c | 84 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/drivers/net/phy/dp83869.c b/drivers/net/phy/dp83869.c
index 64fa2d911074..7d0b11220e47 100644
--- a/drivers/net/phy/dp83869.c
+++ b/drivers/net/phy/dp83869.c
@@ -99,6 +99,14 @@
 #define DP83869_OP_MODE_MII			BIT(5)
 #define DP83869_SGMII_RGMII_BRIDGE		BIT(6)
 
+/* RGMIIDCTL bits */
+#define DP83869_RGMII_TX_CLK_DELAY_MAX		0xf
+#define DP83869_RGMII_TX_CLK_DELAY_SHIFT	4
+#define DP83869_RGMII_TX_CLK_DELAY_INV	(DP83869_RGMII_TX_CLK_DELAY_MAX + 1)
+#define DP83869_RGMII_RX_CLK_DELAY_MAX		0xf
+#define DP83869_RGMII_RX_CLK_DELAY_SHIFT	0
+#define DP83869_RGMII_RX_CLK_DELAY_INV	(DP83869_RGMII_RX_CLK_DELAY_MAX + 1)
+
 enum {
 	DP83869_PORT_MIRRORING_KEEP,
 	DP83869_PORT_MIRRORING_EN,
@@ -108,6 +116,8 @@ enum {
 struct dp83869_private {
 	int tx_fifo_depth;
 	int rx_fifo_depth;
+	u32 rx_id_delay;
+	u32 tx_id_delay;
 	int io_impedance;
 	int port_mirroring;
 	bool rxctrl_strap_quirk;
@@ -232,6 +242,26 @@ static int dp83869_of_init(struct phy_device *phydev)
 				 &dp83869->tx_fifo_depth))
 		dp83869->tx_fifo_depth = DP83869_PHYCR_FIFO_DEPTH_4_B_NIB;
 
+	dp83869->rx_id_delay = DP83869_RGMII_RX_CLK_DELAY_INV;
+	ret = of_property_read_u32(of_node, "ti,rx-internal-delay",
+				   &dp83869->rx_id_delay);
+	if (!ret && dp83869->rx_id_delay > DP83869_RGMII_RX_CLK_DELAY_MAX) {
+		phydev_err(phydev,
+			   "ti,rx-internal-delay value of %u out of range\n",
+			   dp83869->rx_id_delay);
+		return -EINVAL;
+	}
+
+	dp83869->tx_id_delay = DP83869_RGMII_TX_CLK_DELAY_INV;
+	ret = of_property_read_u32(of_node, "ti,tx-internal-delay",
+				   &dp83869->tx_id_delay);
+	if (!ret && dp83869->tx_id_delay > DP83869_RGMII_TX_CLK_DELAY_MAX) {
+		phydev_err(phydev,
+			   "ti,tx-internal-delay value of %u out of range\n",
+			   dp83869->tx_id_delay);
+		return -EINVAL;
+	}
+
 	return ret;
 }
 #else
@@ -270,6 +300,29 @@ static int dp83869_configure_rgmii(struct phy_device *phydev,
 	return ret;
 }
 
+static int dp83869_verify_rgmii_cfg(struct phy_device *phydev)
+{
+	struct dp83869_private *dp83869 = phydev->priv;
+
+	/* RX delay *must* be specified if internal delay of RX is used. */
+	if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	     phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) &&
+	     dp83869->rx_id_delay == DP83869_RGMII_RX_CLK_DELAY_INV) {
+		phydev_err(phydev, "ti,rx-internal-delay must be specified\n");
+		return -EINVAL;
+	}
+
+	/* TX delay *must* be specified if internal delay of TX is used. */
+	if ((phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	     phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) &&
+	     dp83869->tx_id_delay == DP83869_RGMII_TX_CLK_DELAY_INV) {
+		phydev_err(phydev, "ti,tx-internal-delay must be specified\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int dp83869_configure_mode(struct phy_device *phydev,
 				  struct dp83869_private *dp83869)
 {
@@ -371,6 +424,11 @@ static int dp83869_config_init(struct phy_device *phydev)
 {
 	struct dp83869_private *dp83869 = phydev->priv;
 	int ret, val;
+	u16 delay;
+
+	ret = dp83869_verify_rgmii_cfg(phydev);
+	if (ret)
+		return ret;
 
 	ret = dp83869_configure_mode(phydev, dp83869);
 	if (ret)
@@ -394,6 +452,32 @@ static int dp83869_config_init(struct phy_device *phydev)
 				     dp83869->clk_output_sel <<
 				     DP83869_IO_MUX_CFG_CLK_O_SEL_SHIFT);
 
+	if (phy_interface_is_rgmii(phydev)) {
+		val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL);
+
+		val &= ~(DP83869_RGMII_TX_CLK_DELAY_EN | DP83869_RGMII_RX_CLK_DELAY_EN);
+		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
+			val |= (DP83869_RGMII_TX_CLK_DELAY_EN | DP83869_RGMII_RX_CLK_DELAY_EN);
+
+		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
+			val |= DP83869_RGMII_TX_CLK_DELAY_EN;
+
+		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
+			val |= DP83869_RGMII_RX_CLK_DELAY_EN;
+
+		phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIICTL, val);
+
+		delay = 0;
+		if (dp83869->rx_id_delay != DP83869_RGMII_RX_CLK_DELAY_INV)
+			delay |= dp83869->rx_id_delay;
+		if (dp83869->tx_id_delay != DP83869_RGMII_TX_CLK_DELAY_INV)
+			delay |= dp83869->tx_id_delay <<
+				 DP83869_RGMII_TX_CLK_DELAY_SHIFT;
+
+		phy_write_mmd(phydev, DP83869_DEVADDR, DP83869_RGMIIDCTL,
+			      delay);
+	}
+
 	return ret;
 }
 
-- 
2.26.2

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[Jakub Kicinski]

On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
> If the op-mode for the device is not set in the device tree then set
> the strapped op-mode and store it for later configuration.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
  171 |  if (val < 0)
      |          ^

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[kbuild test robot]

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

Hi Dan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200518]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh 

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

All warnings (new ones prefixed by >>, old ones prefixed by <<):

drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode':
>> drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
171 |  if (val < 0)
|          ^

vim +171 drivers/net/phy/dp83869.c

   164	
   165	static int dp83869_set_strapped_mode(struct phy_device *phydev)
   166	{
   167		struct dp83869_private *dp83869 = phydev->priv;
   168		u16 val;
   169	
   170		val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
 > 171		if (val < 0)
   172			return val;
   173	
   174		dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
   175	
   176		return 0;
   177	}
   178	

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

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[kbuild test robot]

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

Hi Dan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200518]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh 

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

All warnings (new ones prefixed by >>, old ones prefixed by <<):

drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode':
>> drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
171 |  if (val < 0)
|          ^

vim +171 drivers/net/phy/dp83869.c

   164	
   165	static int dp83869_set_strapped_mode(struct phy_device *phydev)
   166	{
   167		struct dp83869_private *dp83869 = phydev->priv;
   168		u16 val;
   169	
   170		val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
 > 171		if (val < 0)
   172			return val;
   173	
   174		dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
   175	
   176		return 0;
   177	}
   178	

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

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

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[Dan Murphy]

Jakub

On 5/19/20 11:58 AM, Jakub Kicinski wrote:
> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
>> If the op-mode for the device is not set in the device tree then set
>> the strapped op-mode and store it for later configuration.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
>    171 |  if (val < 0)
>        |          ^

This looks to be a false positive.

phy_read_mmd will return an errno or a value from 0->15

So if errno is returned then this will be true.

Unless I have to do IS_ERR.

I did not get that warning.  But I am using 9.2-gcc.

What compiler are you using?

Dan

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[Dan Murphy]

kbuild

On 5/19/20 12:19 PM, kbuild test robot wrote:
> Hi Dan,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on net-next/master]
> [also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200518]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b
> config: sh-allmodconfig (attached as .config)
> compiler: sh4-linux-gcc (GCC) 9.3.0
> reproduce:
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # save the attached .config to linux build tree
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>, old ones prefixed by <<):
>
> drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode':
>>> drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
> 171 |  if (val < 0)

This looks to be a false positive.

phy_read_mmd will return an errno or a value from 0->15

So if errno is returned then this will be true.

Unless I have to do IS_ERR.

Dan

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[Dan Murphy]

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

kbuild

On 5/19/20 12:19 PM, kbuild test robot wrote:
> Hi Dan,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on net-next/master]
> [also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200518]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b
> config: sh-allmodconfig (attached as .config)
> compiler: sh4-linux-gcc (GCC) 9.3.0
> reproduce:
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # save the attached .config to linux build tree
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>, old ones prefixed by <<):
>
> drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode':
>>> drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
> 171 |  if (val < 0)

This looks to be a false positive.

phy_read_mmd will return an errno or a value from 0->15

So if errno is returned then this will be true.

Unless I have to do IS_ERR.

Dan

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[Dan Murphy]

Jakub

On 5/19/20 12:40 PM, Dan Murphy wrote:
> Jakub
>
> On 5/19/20 11:58 AM, Jakub Kicinski wrote:
>> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
>>> If the op-mode for the device is not set in the device tree then set
>>> the strapped op-mode and store it for later configuration.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
>> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always 
>> false due to limited range of data type [-Wtype-limits]
>>    171 |  if (val < 0)
>>        |          ^
>
> This looks to be a false positive.
>
> phy_read_mmd will return an errno or a value from 0->15
>
> So if errno is returned then this will be true.
>
> Unless I have to do IS_ERR.
>
> I did not get that warning.  But I am using 9.2-gcc.
>
> What compiler are you using?
>
I see what the issue is val needs to be an int not a u16

I will fix it

Dan


> Dan
>

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[Andrew Lunn]

On Tue, May 19, 2020 at 09:58:18AM -0700, Jakub Kicinski wrote:
> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
> > If the op-mode for the device is not set in the device tree then set
> > the strapped op-mode and store it for later configuration.
> > 
> > Signed-off-by: Dan Murphy <dmurphy@ti.com>
> 
> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
>   171 |  if (val < 0)
>       |          ^

Hi Jakub

This happens a lot with PHY drivers. The register being read is a u16,
so that is what people use.

Is this now a standard GCC warning? Or have you turned on extra
checking?

	Andrew

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[Dan Murphy]

Andrew

On 5/19/20 1:29 PM, Andrew Lunn wrote:
> On Tue, May 19, 2020 at 09:58:18AM -0700, Jakub Kicinski wrote:
>> On Tue, 19 May 2020 09:18:11 -0500 Dan Murphy wrote:
>>> If the op-mode for the device is not set in the device tree then set
>>> the strapped op-mode and store it for later configuration.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ../drivers/net/phy/dp83869.c: In function0 dp83869_set_strapped_mode:
>> ../drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
>>    171 |  if (val < 0)
>>        |          ^
> Hi Jakub
>
> This happens a lot with PHY drivers. The register being read is a u16,
> so that is what people use.

Yes this is what happened but phy_read_mmd returns an int so the 
declaration of val should be an int.

I will update that in v2


> Is this now a standard GCC warning? Or have you turned on extra
> checking?
I still was not able to reproduce this warning with gcc-9.2.  I would 
like to know the same


Dan

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[Jakub Kicinski]

On Tue, 19 May 2020 13:41:40 -0500 Dan Murphy wrote:
> > Is this now a standard GCC warning? Or have you turned on extra
> > checking?  
> I still was not able to reproduce this warning with gcc-9.2.  I would 
> like to know the same

W=1 + gcc-10 here, also curious to know which one of the two makes 
the difference :)

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[Dan Murphy]

Jakub

On 5/19/20 1:48 PM, Jakub Kicinski wrote:
> On Tue, 19 May 2020 13:41:40 -0500 Dan Murphy wrote:
>>> Is this now a standard GCC warning? Or have you turned on extra
>>> checking?
>> I still was not able to reproduce this warning with gcc-9.2.  I would
>> like to know the same
> W=1 + gcc-10 here, also curious to know which one of the two makes
> the difference :)

W=1 made the difference I got the warning with gcc-9.2

Dan

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[Andrew Lunn]

On Tue, May 19, 2020 at 01:59:16PM -0500, Dan Murphy wrote:
> Jakub
> 
> On 5/19/20 1:48 PM, Jakub Kicinski wrote:
> > On Tue, 19 May 2020 13:41:40 -0500 Dan Murphy wrote:
> > > > Is this now a standard GCC warning? Or have you turned on extra
> > > > checking?
> > > I still was not able to reproduce this warning with gcc-9.2.  I would
> > > like to know the same
> > W=1 + gcc-10 here, also curious to know which one of the two makes
> > the difference :)
> 
> W=1 made the difference I got the warning with gcc-9.2

I wonder if we should turn on this specific warning by default in
drivers/net/phy? I keep making the same mistake, and it would be nice
if GCC actually told me.

   Andrew

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[kbuild test robot]

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

Hi Dan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200519]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b
config: sparc64-randconfig-s001-20200519 (attached as .config)
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-193-gb8fad4bc-dirty
        # save the attached .config to linux build tree
        make C=1 ARCH=sparc64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

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

All warnings (new ones prefixed by >>, old ones prefixed by <<):

In file included from include/linux/build_bug.h:5,
from include/linux/bits.h:23,
from include/linux/bitops.h:5,
from include/linux/bitmap.h:8,
from include/linux/ethtool.h:16,
from drivers/net/phy/dp83869.c:6:
drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode':
drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
171 |  if (val < 0)
|          ^
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
|                                                    ^~~~
>> drivers/net/phy/dp83869.c:171:2: note: in expansion of macro 'if'
171 |  if (val < 0)
|  ^~
drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
171 |  if (val < 0)
|          ^
include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
|                                                             ^~~~
>> drivers/net/phy/dp83869.c:171:2: note: in expansion of macro 'if'
171 |  if (val < 0)
|  ^~
drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
171 |  if (val < 0)
|          ^
include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
69 |  (cond) ?              |   ^~~~
include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
|                            ^~~~~~~~~~~~~~
>> drivers/net/phy/dp83869.c:171:2: note: in expansion of macro 'if'
171 |  if (val < 0)
|  ^~

vim +/if +171 drivers/net/phy/dp83869.c

   164	
   165	static int dp83869_set_strapped_mode(struct phy_device *phydev)
   166	{
   167		struct dp83869_private *dp83869 = phydev->priv;
   168		u16 val;
   169	
   170		val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
 > 171		if (val < 0)
   172			return val;
   173	
   174		dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
   175	
   176		return 0;
   177	}
   178	

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

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[kbuild test robot]

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

Hi Dan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]
[also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200519]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b
config: sparc64-randconfig-s001-20200519 (attached as .config)
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-193-gb8fad4bc-dirty
        # save the attached .config to linux build tree
        make C=1 ARCH=sparc64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

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

All warnings (new ones prefixed by >>, old ones prefixed by <<):

In file included from include/linux/build_bug.h:5,
from include/linux/bits.h:23,
from include/linux/bitops.h:5,
from include/linux/bitmap.h:8,
from include/linux/ethtool.h:16,
from drivers/net/phy/dp83869.c:6:
drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode':
drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
171 |  if (val < 0)
|          ^
include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
|                                                    ^~~~
>> drivers/net/phy/dp83869.c:171:2: note: in expansion of macro 'if'
171 |  if (val < 0)
|  ^~
drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
171 |  if (val < 0)
|          ^
include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
|                                                             ^~~~
>> drivers/net/phy/dp83869.c:171:2: note: in expansion of macro 'if'
171 |  if (val < 0)
|  ^~
drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
171 |  if (val < 0)
|          ^
include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
69 |  (cond) ?              |   ^~~~
include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
|                            ^~~~~~~~~~~~~~
>> drivers/net/phy/dp83869.c:171:2: note: in expansion of macro 'if'
171 |  if (val < 0)
|  ^~

vim +/if +171 drivers/net/phy/dp83869.c

   164	
   165	static int dp83869_set_strapped_mode(struct phy_device *phydev)
   166	{
   167		struct dp83869_private *dp83869 = phydev->priv;
   168		u16 val;
   169	
   170		val = phy_read_mmd(phydev, DP83869_DEVADDR, DP83869_STRAP_STS1);
 > 171		if (val < 0)
   172			return val;
   173	
   174		dp83869->mode = val & DP83869_STRAP_OP_MODE_MASK;
   175	
   176		return 0;
   177	}
   178	

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

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

Re: [PATCH net-next 1/4] net: phy: dp83869: Update port-mirroring to read straps

[Florian Fainelli]

On 5/19/2020 7:18 AM, Dan Murphy wrote:
> The device tree may not have the property set for port mirroring
> because the hardware may have it strapped. If the property is not in the
> DT then check the straps and set the port mirroring bit appropriately.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [kbuild-all] Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[Philip Li]

On Tue, May 19, 2020 at 12:40:37PM -0500, Dan Murphy wrote:
> kbuild
> 
> On 5/19/20 12:19 PM, kbuild test robot wrote:
> > Hi Dan,
> > 
> > I love your patch! Perhaps something to improve:
> > 
> > [auto build test WARNING on net-next/master]
> > [also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200518]
> > [if your patch is applied to the wrong git tree, please drop us a note to help
> > improve the system. BTW, we also suggest to use '--base' option to specify the
> > base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b
> > config: sh-allmodconfig (attached as .config)
> > compiler: sh4-linux-gcc (GCC) 9.3.0
> > reproduce:
> >          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >          chmod +x ~/bin/make.cross
> >          # save the attached .config to linux build tree
> >          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
> > 
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kbuild test robot <lkp@intel.com>
> > 
> > All warnings (new ones prefixed by >>, old ones prefixed by <<):
> > 
> > drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode':
> > > > drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
> > 171 |  if (val < 0)
> 
> This looks to be a false positive.
> 
> phy_read_mmd will return an errno or a value from 0->15
thanks, here because val is defined as "u16 val", the comparison
to < 0 can not work as expected. Any err returned from phy_read_mmd,
which is int, will be converted to u16.

> 
> So if errno is returned then this will be true.
> 
> Unless I have to do IS_ERR.
> 
> Dan
> _______________________________________________
> kbuild-all mailing list -- kbuild-all@lists.01.org
> To unsubscribe send an email to kbuild-all-leave@lists.01.org

Re: [PATCH net-next 2/4] net: phy: dp83869: Set opmode from straps

[Philip Li]

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

On Tue, May 19, 2020 at 12:40:37PM -0500, Dan Murphy wrote:
> kbuild
> 
> On 5/19/20 12:19 PM, kbuild test robot wrote:
> > Hi Dan,
> > 
> > I love your patch! Perhaps something to improve:
> > 
> > [auto build test WARNING on net-next/master]
> > [also build test WARNING on robh/for-next sparc-next/master net/master linus/master v5.7-rc6 next-20200518]
> > [if your patch is applied to the wrong git tree, please drop us a note to help
> > improve the system. BTW, we also suggest to use '--base' option to specify the
> > base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> > 
> > url:    https://github.com/0day-ci/linux/commits/Dan-Murphy/DP83869-Enhancements/20200519-222047
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 5cdfe8306631b2224e3f81fc5a1e2721c7a1948b
> > config: sh-allmodconfig (attached as .config)
> > compiler: sh4-linux-gcc (GCC) 9.3.0
> > reproduce:
> >          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >          chmod +x ~/bin/make.cross
> >          # save the attached .config to linux build tree
> >          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
> > 
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kbuild test robot <lkp@intel.com>
> > 
> > All warnings (new ones prefixed by >>, old ones prefixed by <<):
> > 
> > drivers/net/phy/dp83869.c: In function 'dp83869_set_strapped_mode':
> > > > drivers/net/phy/dp83869.c:171:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]
> > 171 |  if (val < 0)
> 
> This looks to be a false positive.
> 
> phy_read_mmd will return an errno or a value from 0->15
thanks, here because val is defined as "u16 val", the comparison
to < 0 can not work as expected. Any err returned from phy_read_mmd,
which is int, will be converted to u16.

> 
> So if errno is returned then this will be true.
> 
> Unless I have to do IS_ERR.
> 
> Dan
> _______________________________________________
> kbuild-all mailing list -- kbuild-all(a)lists.01.org
> To unsubscribe send an email to kbuild-all-leave(a)lists.01.org