diff mbox series

[3/3] net: phy: dp83826: add support for voltage tuning of logical levels

Message ID 20240111161927.3689084-3-catalin.popescu@leica-geosystems.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [1/3] dt-bindings: net: dp83826: add ti,cfg-dac-minus binding | expand

Checks

Context Check Description
netdev/series_format warning Series does not have a cover letter
netdev/tree_selection success Guessed tree name to be net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1081 this patch: 1081
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 1108 this patch: 1108
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1108 this patch: 1108
netdev/checkpatch warning WARNING: line length of 100 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 93 exceeds 80 columns WARNING: line length of 94 exceeds 80 columns WARNING: line length of 96 exceeds 80 columns WARNING: line length of 97 exceeds 80 columns WARNING: line length of 98 exceeds 80 columns WARNING: line length of 99 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

POPESCU Catalin Jan. 11, 2024, 4:19 p.m. UTC 
DP83826 offers the possibility to tune the voltage of logical levels
of the MLT-3 encoded data. This is especially interesting when the
data path is lossy and we want to increase the voltage levels to
compensate the loss.

Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
---
 drivers/net/phy/dp83822.c | 127 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 123 insertions(+), 4 deletions(-)

Comments

Andrew Lunn Jan. 11, 2024, 4:45 p.m. UTC | #1 
> +u8 dp83826_cfg_dac_minus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x38, 0x37, 0x36, 0x35, 0x34, 0x33,
> +								0x32, 0x31, 0x30, 0x2f, 0x2e, 0x2d,
> +								0x2c, 0x2b, 0x2a, 0x29, 0x28};
> +u8 dp83826_cfg_dac_plus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d,
> +							       0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13,
> +							       0x14, 0x15, 0x16, 0x17, 0x18};

Both of these should be static const.

However, they appear pointless. Plus is just a shift. minus is some
simple arithmetic and a shift.

       Andrew
POPESCU Catalin Jan. 11, 2024, 4:54 p.m. UTC | #2 
On 11.01.24 17:45, Andrew Lunn wrote:
> [You don't often get email from andrew@lunn.ch. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
>> +u8 dp83826_cfg_dac_minus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x38, 0x37, 0x36, 0x35, 0x34, 0x33,
>> +                                                             0x32, 0x31, 0x30, 0x2f, 0x2e, 0x2d,
>> +                                                             0x2c, 0x2b, 0x2a, 0x29, 0x28};
>> +u8 dp83826_cfg_dac_plus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d,
>> +                                                            0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13,
>> +                                                            0x14, 0x15, 0x16, 0x17, 0x18};
> Both of these should be static const.
Indeed.
>
> However, they appear pointless. Plus is just a shift. minus is some
> simple arithmetic and a shift.
Well, I found it more clear to use a bit of memory than adding 
instructions to compute some raw values from other raw values used as 
reference.
>
>         Andrew
kernel test robot Jan. 13, 2024, 3:26 a.m. UTC | #3 
Hi Catalin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on net-next/main net/main linus/master v6.7 next-20240112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Catalin-Popescu/dt-bindings-net-dp83826-add-ti-cfg-dac-plus-binding/20240112-002701
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240111161927.3689084-3-catalin.popescu%40leica-geosystems.com
patch subject: [PATCH 3/3] net: phy: dp83826: add support for voltage tuning of logical levels
config: x86_64-randconfig-121-20240112 (https://download.01.org/0day-ci/archive/20240113/202401131120.vAXFTd3t-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240113/202401131120.vAXFTd3t-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401131120.vAXFTd3t-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/phy/dp83822.c:570:4: sparse: sparse: symbol 'dp83826_cfg_dac_minus_raw' was not declared. Should it be static?
>> drivers/net/phy/dp83822.c:573:4: sparse: sparse: symbol 'dp83826_cfg_dac_plus_raw' was not declared. Should it be static?

vim +/dp83826_cfg_dac_minus_raw +570 drivers/net/phy/dp83822.c

   569	
 > 570	u8 dp83826_cfg_dac_minus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x38, 0x37, 0x36, 0x35, 0x34, 0x33,
   571									0x32, 0x31, 0x30, 0x2f, 0x2e, 0x2d,
   572									0x2c, 0x2b, 0x2a, 0x29, 0x28};
 > 573	u8 dp83826_cfg_dac_plus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d,
   574								       0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13,
   575								       0x14, 0x15, 0x16, 0x17, 0x18};
   576
diff mbox series

Patch

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index b7cb71817780..e26aaacc7ea5 100644
--- a/drivers/net/phy/dp83822.c
+++ b/drivers/net/phy/dp83822.c
@@ -12,6 +12,7 @@ 
 #include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/netdevice.h>
+#include <linux/bitfield.h>
 
 #define DP83822_PHY_ID	        0x2000a240
 #define DP83825S_PHY_ID		0x2000a140
@@ -34,6 +35,10 @@ 
 #define MII_DP83822_GENCFG	0x465
 #define MII_DP83822_SOR1	0x467
 
+/* DP83826 specific registers */
+#define MII_DP83826_VOD_CFG1	0x30b
+#define MII_DP83826_VOD_CFG2	0x30c
+
 /* GENCFG */
 #define DP83822_SIG_DET_LOW	BIT(0)
 
@@ -110,6 +115,16 @@ 
 #define DP83822_RX_ER_STR_MASK	GENMASK(9, 8)
 #define DP83822_RX_ER_SHIFT	8
 
+/* DP83826: VOD_CFG1 & VOD_CFG2 */
+#define DP83826_VOD_CFG1_MINUS_MDIX_MASK	GENMASK(13, 12)
+#define DP83826_VOD_CFG1_MINUS_MDI_MASK		GENMASK(11, 6)
+#define DP83826_VOD_CFG2_MINUS_MDIX_MASK	GENMASK(15, 12)
+#define DP83826_VOD_CFG2_PLUS_MDIX_MASK		GENMASK(11, 6)
+#define DP83826_VOD_CFG2_PLUS_MDI_MASK		GENMASK(5, 0)
+#define DP83826_CFG_DAC_MINUS_MDIX_5_TO_4	GENMASK(5, 4)
+#define DP83826_CFG_DAC_MINUS_MDIX_3_TO_0	GENMASK(3, 0)
+#define DP83826_CFG_DAC_RAW_VALUES_MAX		17
+
 #define MII_DP83822_FIBER_ADVERTISE    (ADVERTISED_TP | ADVERTISED_MII | \
 					ADVERTISED_FIBRE | \
 					ADVERTISED_Pause | ADVERTISED_Asym_Pause)
@@ -118,6 +133,8 @@  struct dp83822_private {
 	bool fx_signal_det_low;
 	int fx_enabled;
 	u16 fx_sd_enable;
+	u8 cfg_dac_minus;
+	u8 cfg_dac_plus;
 };
 
 static int dp83822_set_wol(struct phy_device *phydev,
@@ -233,7 +250,7 @@  static int dp83822_config_intr(struct phy_device *phydev)
 				DP83822_ENERGY_DET_INT_EN |
 				DP83822_LINK_QUAL_INT_EN);
 
-		/* Private data pointer is NULL on DP83825/26 */
+		/* Private data pointer is NULL on DP83825 */
 		if (!dp83822 || !dp83822->fx_enabled)
 			misr_status |= DP83822_ANEG_COMPLETE_INT_EN |
 				       DP83822_DUP_MODE_CHANGE_INT_EN |
@@ -254,7 +271,7 @@  static int dp83822_config_intr(struct phy_device *phydev)
 				DP83822_PAGE_RX_INT_EN |
 				DP83822_EEE_ERROR_CHANGE_INT_EN);
 
-		/* Private data pointer is NULL on DP83825/26 */
+		/* Private data pointer is NULL on DP83825 */
 		if (!dp83822 || !dp83822->fx_enabled)
 			misr_status |= DP83822_ANEG_ERR_INT_EN |
 				       DP83822_WOL_PKT_INT_EN;
@@ -474,6 +491,46 @@  static int dp83822_config_init(struct phy_device *phydev)
 	return dp8382x_disable_wol(phydev);
 }
 
+static int dp83826_config_init(struct phy_device *phydev)
+{
+	struct dp83822_private *dp83822 = phydev->priv;
+	u16 vod_cfg1_val = 0, vod_cfg1_msk = 0, vod_cfg2_val = 0, vod_cfg2_msk = 0;
+	int ret;
+
+	if (dp83822->cfg_dac_plus) {
+		vod_cfg2_val = FIELD_PREP(DP83826_VOD_CFG2_PLUS_MDIX_MASK, dp83822->cfg_dac_plus) |
+			       FIELD_PREP(DP83826_VOD_CFG2_PLUS_MDI_MASK, dp83822->cfg_dac_plus);
+		vod_cfg2_msk = DP83826_VOD_CFG2_PLUS_MDIX_MASK | DP83826_VOD_CFG2_PLUS_MDI_MASK;
+	}
+
+	if (dp83822->cfg_dac_minus) {
+		vod_cfg2_val |= FIELD_PREP(DP83826_VOD_CFG2_MINUS_MDIX_MASK,
+				FIELD_GET(DP83826_CFG_DAC_MINUS_MDIX_3_TO_0,
+					  dp83822->cfg_dac_minus));
+		vod_cfg2_msk |= DP83826_VOD_CFG2_MINUS_MDIX_MASK;
+		vod_cfg1_val = FIELD_PREP(DP83826_VOD_CFG1_MINUS_MDI_MASK, dp83822->cfg_dac_minus) |
+			       FIELD_PREP(DP83826_VOD_CFG1_MINUS_MDIX_MASK,
+					  FIELD_GET(DP83826_CFG_DAC_MINUS_MDIX_5_TO_4,
+						    dp83822->cfg_dac_minus));
+		vod_cfg1_msk = DP83826_VOD_CFG1_MINUS_MDIX_MASK | DP83826_VOD_CFG1_MINUS_MDI_MASK;
+	}
+
+	if (vod_cfg1_msk) {
+		ret = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83826_VOD_CFG1,
+				     vod_cfg1_msk, vod_cfg1_val);
+		if (ret)
+			return ret;
+	}
+	if (vod_cfg2_msk) {
+		ret = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83826_VOD_CFG2,
+				     vod_cfg2_msk, vod_cfg2_val);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int dp8382x_config_init(struct phy_device *phydev)
 {
 	return dp8382x_disable_wol(phydev);
@@ -509,11 +566,41 @@  static int dp83822_of_init(struct phy_device *phydev)
 
 	return 0;
 }
+
+u8 dp83826_cfg_dac_minus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x38, 0x37, 0x36, 0x35, 0x34, 0x33,
+								0x32, 0x31, 0x30, 0x2f, 0x2e, 0x2d,
+								0x2c, 0x2b, 0x2a, 0x29, 0x28};
+u8 dp83826_cfg_dac_plus_raw[DP83826_CFG_DAC_RAW_VALUES_MAX] = {0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d,
+							       0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13,
+							       0x14, 0x15, 0x16, 0x17, 0x18};
+
+static int dp83826_of_init(struct phy_device *phydev)
+{
+	struct dp83822_private *dp83822 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	u32 val;
+	int ret;
+
+	ret = device_property_read_u32(dev, "ti,cfg-dac-minus", &val);
+	if (!ret && val < DP83826_CFG_DAC_RAW_VALUES_MAX)
+		dp83822->cfg_dac_minus = dp83826_cfg_dac_minus_raw[val];
+
+	ret = device_property_read_u32(dev, "ti,cfg-dac-plus", &val);
+	if (!ret && val < DP83826_CFG_DAC_RAW_VALUES_MAX)
+		dp83822->cfg_dac_plus = dp83826_cfg_dac_plus_raw[val];
+
+	return 0;
+}
 #else
 static int dp83822_of_init(struct phy_device *phydev)
 {
 	return 0;
 }
+
+static int dp83826_of_init(struct phy_device *phydev)
+{
+	return 0;
+}
 #endif /* CONFIG_OF_MDIO */
 
 static int dp83822_read_straps(struct phy_device *phydev)
@@ -567,6 +654,22 @@  static int dp83822_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int dp83826_probe(struct phy_device *phydev)
+{
+	struct dp83822_private *dp83822;
+
+	dp83822 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83822),
+			       GFP_KERNEL);
+	if (!dp83822)
+		return -ENOMEM;
+
+	phydev->priv = dp83822;
+
+	dp83826_of_init(phydev);
+
+	return 0;
+}
+
 static int dp83822_suspend(struct phy_device *phydev)
 {
 	int value;
@@ -610,6 +713,22 @@  static int dp83822_resume(struct phy_device *phydev)
 		.resume = dp83822_resume,			\
 	}
 
+#define DP83826_PHY_DRIVER(_id, _name)				\
+	{							\
+		PHY_ID_MATCH_MODEL(_id),			\
+		.name		= (_name),			\
+		/* PHY_BASIC_FEATURES */			\
+		.probe          = dp83826_probe,		\
+		.soft_reset	= dp83822_phy_reset,		\
+		.config_init	= dp83826_config_init,		\
+		.get_wol = dp83822_get_wol,			\
+		.set_wol = dp83822_set_wol,			\
+		.config_intr = dp83822_config_intr,		\
+		.handle_interrupt = dp83822_handle_interrupt,	\
+		.suspend = dp83822_suspend,			\
+		.resume = dp83822_resume,			\
+	}
+
 #define DP8382X_PHY_DRIVER(_id, _name)				\
 	{							\
 		PHY_ID_MATCH_MODEL(_id),			\
@@ -628,8 +747,8 @@  static int dp83822_resume(struct phy_device *phydev)
 static struct phy_driver dp83822_driver[] = {
 	DP83822_PHY_DRIVER(DP83822_PHY_ID, "TI DP83822"),
 	DP8382X_PHY_DRIVER(DP83825I_PHY_ID, "TI DP83825I"),
-	DP8382X_PHY_DRIVER(DP83826C_PHY_ID, "TI DP83826C"),
-	DP8382X_PHY_DRIVER(DP83826NC_PHY_ID, "TI DP83826NC"),
+	DP83826_PHY_DRIVER(DP83826C_PHY_ID, "TI DP83826C"),
+	DP83826_PHY_DRIVER(DP83826NC_PHY_ID, "TI DP83826NC"),
 	DP8382X_PHY_DRIVER(DP83825S_PHY_ID, "TI DP83825S"),
 	DP8382X_PHY_DRIVER(DP83825CM_PHY_ID, "TI DP83825M"),
 	DP8382X_PHY_DRIVER(DP83825CS_PHY_ID, "TI DP83825CS"),