[PATCH v2 1/2] dt-bindings: net: dp83826: support TX data voltage tuning

[PATCH v2 1/2] dt-bindings: net: dp83826: support TX data voltage tuning

[Catalin Popescu]

Add properties ti,cfg-dac-minus-one-milli-percent and
ti,cfg-dac-plus-one-milli-percent to support voltage tuning
of logical levels -1/+1 of the MLT-3 encoded TX data.

Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
---
Changes in v2:
 - squash the 2 DT bindings patches in one single patch
 - drop redundant "binding" from the DT bindings patch title
 - rename DT properties and define them as percentage
 - add default value for each new DT property
---
 .../devicetree/bindings/net/ti,dp83822.yaml    | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
index db74474207ed..6bbd465e51d6 100644
--- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
+++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
@@ -62,6 +62,24 @@ properties:
        for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
        to transmit data.
 
+  ti,cfg-dac-minus-one-milli-percent:
+    description: |
+       DP83826 PHY only.
+       Sets the voltage ratio (with respect to the nominal value)
+       of the logical level -1 for the MLT-3 encoded TX data.
+    enum: [50000, 56250, 62500, 68750, 75000, 81250, 87500, 93750, 100000,
+           106250, 112500, 118750, 125000, 131250, 137500, 143750, 150000]
+    default: 100000
+
+  ti,cfg-dac-plus-one-milli-percent:
+    description: |
+       DP83826 PHY only.
+       Sets the voltage ratio (with respect to the nominal value)
+       of the logical level +1 for the MLT-3 encoded TX data.
+    enum: [50000, 56250, 62500, 68750, 75000, 81250, 87500, 93750, 100000,
+           106250, 112500, 118750, 125000, 131250, 137500, 143750, 150000]
+    default: 100000
+
 required:
   - reg
 

base-commit: 6d280f4d760e3bcb4a8df302afebf085b65ec982
prerequisite-patch-id: 0000000000000000000000000000000000000000
-- 
2.34.1

[PATCH v2 2/2] net: phy: dp83826: support TX data voltage tuning

[Catalin Popescu]

DP83826 offers the possibility to tune the voltage of logical levels
of the MLT-3 encoded TX data. This is especially interesting when the
TX 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>
---
Changes in v2:
 - remove raw values mapping tables dp83826_cfg_dac_minus_raw/
   dp83826_cfg_dac_plus_raw and replace them with functions
   dp83826_to_dac_minus_one_regval/dp83826_to_dac_plus_one_regval
 - increase readability of function dp83826_config_init
 - change return value of function dp83826_of_init from int to void
   since it never returns any error
---
 drivers/net/phy/dp83822.c | 130 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 126 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
index b7cb71817780..17070bb1a4b0 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,19 @@
 #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_MPERCENT_PER_STEP	6250
+#define DP83826_CFG_DAC_MPERCENT_DEFAULT	100000
+#define DP83826_CFG_DAC_MINUS_DEFAULT		0x30
+#define DP83826_CFG_DAC_PLUS_DEFAULT		0x10
+
 #define MII_DP83822_FIBER_ADVERTISE    (ADVERTISED_TP | ADVERTISED_MII | \
 					ADVERTISED_FIBRE | \
 					ADVERTISED_Pause | ADVERTISED_Asym_Pause)
@@ -118,6 +136,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 +253,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 +274,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 +494,43 @@ 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 val, mask;
+	int ret;
+
+	if (dp83822->cfg_dac_minus != DP83826_CFG_DAC_MINUS_DEFAULT) {
+		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));
+		mask = DP83826_VOD_CFG1_MINUS_MDIX_MASK | DP83826_VOD_CFG1_MINUS_MDI_MASK;
+		ret = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83826_VOD_CFG1, mask, val);
+		if (ret)
+			return ret;
+
+		val = FIELD_PREP(DP83826_VOD_CFG2_MINUS_MDIX_MASK,
+				 FIELD_GET(DP83826_CFG_DAC_MINUS_MDIX_3_TO_0,
+					   dp83822->cfg_dac_minus));
+		mask = DP83826_VOD_CFG2_MINUS_MDIX_MASK;
+		ret = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83826_VOD_CFG2, mask, val);
+		if (ret)
+			return ret;
+	}
+
+	if (dp83822->cfg_dac_plus != DP83826_CFG_DAC_PLUS_DEFAULT) {
+		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);
+		mask = DP83826_VOD_CFG2_PLUS_MDIX_MASK | DP83826_VOD_CFG2_PLUS_MDI_MASK;
+		ret = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83826_VOD_CFG2, mask, val);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int dp8382x_config_init(struct phy_device *phydev)
 {
 	return dp8382x_disable_wol(phydev);
@@ -509,11 +566,44 @@ static int dp83822_of_init(struct phy_device *phydev)
 
 	return 0;
 }
+
+static int dp83826_to_dac_minus_one_regval(int percent)
+{
+	int tmp = DP83826_CFG_DAC_MPERCENT_DEFAULT - percent;
+
+	return tmp / DP83826_CFG_DAC_MPERCENT_PER_STEP;
+}
+
+static int dp83826_to_dac_plus_one_regval(int percent)
+{
+	int tmp = percent - DP83826_CFG_DAC_MPERCENT_DEFAULT;
+
+	return tmp / DP83826_CFG_DAC_MPERCENT_PER_STEP;
+}
+
+static void dp83826_of_init(struct phy_device *phydev)
+{
+	struct dp83822_private *dp83822 = phydev->priv;
+	struct device *dev = &phydev->mdio.dev;
+	u32 val;
+
+	dp83822->cfg_dac_minus = DP83826_CFG_DAC_MINUS_DEFAULT;
+	if (!device_property_read_u32(dev, "ti,cfg-dac-minus-one-milli-percent", &val))
+		dp83822->cfg_dac_minus += dp83826_to_dac_minus_one_regval(val);
+
+	dp83822->cfg_dac_plus = DP83826_CFG_DAC_PLUS_DEFAULT;
+	if (!device_property_read_u32(dev, "ti,cfg-dac-plus-one-milli-percent", &val))
+		dp83822->cfg_dac_plus += dp83826_to_dac_plus_one_regval(val);
+}
 #else
 static int dp83822_of_init(struct phy_device *phydev)
 {
 	return 0;
 }
+
+static void dp83826_of_init(struct phy_device *phydev)
+{
+}
 #endif /* CONFIG_OF_MDIO */
 
 static int dp83822_read_straps(struct phy_device *phydev)
@@ -567,6 +657,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 +716,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 +750,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"),
-- 
2.34.1

Re: [PATCH v2 2/2] net: phy: dp83826: support TX data voltage tuning

[Andrew Lunn]

> +static int dp83826_config_init(struct phy_device *phydev)
> +{
> +	struct dp83822_private *dp83822 = phydev->priv;
> +	u16 val, mask;
> +	int ret;
> +
> +	if (dp83822->cfg_dac_minus != DP83826_CFG_DAC_MINUS_DEFAULT) {
> +		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));
> +		mask = DP83826_VOD_CFG1_MINUS_MDIX_MASK | DP83826_VOD_CFG1_MINUS_MDI_MASK;
> +		ret = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83826_VOD_CFG1, mask, val);
> +		if (ret)
> +			return ret;
> +
> +		val = FIELD_PREP(DP83826_VOD_CFG2_MINUS_MDIX_MASK,
> +				 FIELD_GET(DP83826_CFG_DAC_MINUS_MDIX_3_TO_0,
> +					   dp83822->cfg_dac_minus));
> +		mask = DP83826_VOD_CFG2_MINUS_MDIX_MASK;
> +		ret = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83826_VOD_CFG2, mask, val);
> +		if (ret)
> +			return ret;
> +	}

I could be reading this wrong, but it looks like
DP83826_CFG_DAC_MINUS_DEFAULT actually means leave the value
unchanged? Is there anything guaranteeing it does in fact have the
default value in the hardware?

	Andrew

Re: [PATCH v2 1/2] dt-bindings: net: dp83826: support TX data voltage tuning

[Krzysztof Kozlowski]

On 07/02/2024 18:58, Catalin Popescu wrote:
> Add properties ti,cfg-dac-minus-one-milli-percent and
> ti,cfg-dac-plus-one-milli-percent to support voltage tuning
> of logical levels -1/+1 of the MLT-3 encoded TX data.
> 
> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
> ---
> Changes in v2:
>  - squash the 2 DT bindings patches in one single patch
>  - drop redundant "binding" from the DT bindings patch title
>  - rename DT properties and define them as percentage
>  - add default value for each new DT property
> ---
>  .../devicetree/bindings/net/ti,dp83822.yaml    | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> index db74474207ed..6bbd465e51d6 100644
> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
> @@ -62,6 +62,24 @@ properties:
>         for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>         to transmit data.
>  
> +  ti,cfg-dac-minus-one-milli-percent:
> +    description: |
> +       DP83826 PHY only.
> +       Sets the voltage ratio (with respect to the nominal value)
> +       of the logical level -1 for the MLT-3 encoded TX data.
> +    enum: [50000, 56250, 62500, 68750, 75000, 81250, 87500, 93750, 100000,
> +           106250, 112500, 118750, 125000, 131250, 137500, 143750, 150000]

I see all values being multiple of basis points, so why not using -bp
suffix?


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: net: dp83826: support TX data voltage tuning

[POPESCU Catalin]

On 08.02.24 08:35, Krzysztof Kozlowski wrote:
> [You don't often get email from krzysztof.kozlowski@linaro.org. 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.
>
>
> On 07/02/2024 18:58, Catalin Popescu wrote:
>> Add properties ti,cfg-dac-minus-one-milli-percent and
>> ti,cfg-dac-plus-one-milli-percent to support voltage tuning
>> of logical levels -1/+1 of the MLT-3 encoded TX data.
>>
>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>> ---
>> Changes in v2:
>>   - squash the 2 DT bindings patches in one single patch
>>   - drop redundant "binding" from the DT bindings patch title
>>   - rename DT properties and define them as percentage
>>   - add default value for each new DT property
>> ---
>>   .../devicetree/bindings/net/ti,dp83822.yaml    | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>> index db74474207ed..6bbd465e51d6 100644
>> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>> @@ -62,6 +62,24 @@ properties:
>>          for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>>          to transmit data.
>>
>> +  ti,cfg-dac-minus-one-milli-percent:
>> +    description: |
>> +       DP83826 PHY only.
>> +       Sets the voltage ratio (with respect to the nominal value)
>> +       of the logical level -1 for the MLT-3 encoded TX data.
>> +    enum: [50000, 56250, 62500, 68750, 75000, 81250, 87500, 93750, 100000,
>> +           106250, 112500, 118750, 125000, 131250, 137500, 143750, 150000]
> I see all values being multiple of basis points, so why not using -bp
> suffix?

I can read :

   "-bp$":
     $ref: types.yaml#/definitions/int32-array
     description: basis points (1/100 of a percent)

In my case it's 1/1000 of a percent. As I'm not sure exactly what the 
author meant by "basis points", would this apply to my patch as well?

>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 1/2] dt-bindings: net: dp83826: support TX data voltage tuning

[Krzysztof Kozlowski]

On 08/02/2024 09:48, POPESCU Catalin wrote:
> On 08.02.24 08:35, Krzysztof Kozlowski wrote:
>> [You don't often get email from krzysztof.kozlowski@linaro.org. 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.
>>
>>
>> On 07/02/2024 18:58, Catalin Popescu wrote:
>>> Add properties ti,cfg-dac-minus-one-milli-percent and
>>> ti,cfg-dac-plus-one-milli-percent to support voltage tuning
>>> of logical levels -1/+1 of the MLT-3 encoded TX data.
>>>
>>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>>> ---
>>> Changes in v2:
>>>   - squash the 2 DT bindings patches in one single patch
>>>   - drop redundant "binding" from the DT bindings patch title
>>>   - rename DT properties and define them as percentage
>>>   - add default value for each new DT property
>>> ---
>>>   .../devicetree/bindings/net/ti,dp83822.yaml    | 18 ++++++++++++++++++
>>>   1 file changed, 18 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>> index db74474207ed..6bbd465e51d6 100644
>>> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>> @@ -62,6 +62,24 @@ properties:
>>>          for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>>>          to transmit data.
>>>
>>> +  ti,cfg-dac-minus-one-milli-percent:
>>> +    description: |
>>> +       DP83826 PHY only.
>>> +       Sets the voltage ratio (with respect to the nominal value)
>>> +       of the logical level -1 for the MLT-3 encoded TX data.
>>> +    enum: [50000, 56250, 62500, 68750, 75000, 81250, 87500, 93750, 100000,
>>> +           106250, 112500, 118750, 125000, 131250, 137500, 143750, 150000]
>> I see all values being multiple of basis points, so why not using -bp
>> suffix?
> 
> I can read :
> 
>    "-bp$":
>      $ref: types.yaml#/definitions/int32-array
>      description: basis points (1/100 of a percent)
> 
> In my case it's 1/1000 of a percent. As I'm not sure exactly what the 
> author meant by "basis points", would this apply to my patch as well?

So please show me the value which does not fit in -bp.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] net: phy: dp83826: support TX data voltage tuning

[POPESCU Catalin]

On 07.02.24 19:35, Andrew Lunn wrote:
> [Some people who received this message 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.
>
>
>> +static int dp83826_config_init(struct phy_device *phydev)
>> +{
>> +     struct dp83822_private *dp83822 = phydev->priv;
>> +     u16 val, mask;
>> +     int ret;
>> +
>> +     if (dp83822->cfg_dac_minus != DP83826_CFG_DAC_MINUS_DEFAULT) {
>> +             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));
>> +             mask = DP83826_VOD_CFG1_MINUS_MDIX_MASK | DP83826_VOD_CFG1_MINUS_MDI_MASK;
>> +             ret = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83826_VOD_CFG1, mask, val);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             val = FIELD_PREP(DP83826_VOD_CFG2_MINUS_MDIX_MASK,
>> +                              FIELD_GET(DP83826_CFG_DAC_MINUS_MDIX_3_TO_0,
>> +                                        dp83822->cfg_dac_minus));
>> +             mask = DP83826_VOD_CFG2_MINUS_MDIX_MASK;
>> +             ret = phy_modify_mmd(phydev, DP83822_DEVADDR, MII_DP83826_VOD_CFG2, mask, val);
>> +             if (ret)
>> +                     return ret;
>> +     }
> I could be reading this wrong, but it looks like
> DP83826_CFG_DAC_MINUS_DEFAULT actually means leave the value
> unchanged? Is there anything guaranteeing it does in fact have the
> default value in the hardware?
>
>          Andrew

Yes, the datasheet clearly states the default/reset values of both 
registers VOD_CFG1 & VOD_CFG2 which are :
- cfg_dac_minus : 30h
- cfg_dac_plus : 10h

Re: [PATCH v2 1/2] dt-bindings: net: dp83826: support TX data voltage tuning

[POPESCU Catalin]

On 08.02.24 09:50, Krzysztof Kozlowski wrote:
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On 08/02/2024 09:48, POPESCU Catalin wrote:
>> On 08.02.24 08:35, Krzysztof Kozlowski wrote:
>>> [You don't often get email from krzysztof.kozlowski@linaro.org. 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.
>>>
>>>
>>> On 07/02/2024 18:58, Catalin Popescu wrote:
>>>> Add properties ti,cfg-dac-minus-one-milli-percent and
>>>> ti,cfg-dac-plus-one-milli-percent to support voltage tuning
>>>> of logical levels -1/+1 of the MLT-3 encoded TX data.
>>>>
>>>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>>>> ---
>>>> Changes in v2:
>>>>    - squash the 2 DT bindings patches in one single patch
>>>>    - drop redundant "binding" from the DT bindings patch title
>>>>    - rename DT properties and define them as percentage
>>>>    - add default value for each new DT property
>>>> ---
>>>>    .../devicetree/bindings/net/ti,dp83822.yaml    | 18 ++++++++++++++++++
>>>>    1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>> index db74474207ed..6bbd465e51d6 100644
>>>> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>> @@ -62,6 +62,24 @@ properties:
>>>>           for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>>>>           to transmit data.
>>>>
>>>> +  ti,cfg-dac-minus-one-milli-percent:
>>>> +    description: |
>>>> +       DP83826 PHY only.
>>>> +       Sets the voltage ratio (with respect to the nominal value)
>>>> +       of the logical level -1 for the MLT-3 encoded TX data.
>>>> +    enum: [50000, 56250, 62500, 68750, 75000, 81250, 87500, 93750, 100000,
>>>> +           106250, 112500, 118750, 125000, 131250, 137500, 143750, 150000]
>>> I see all values being multiple of basis points, so why not using -bp
>>> suffix?
>> I can read :
>>
>>     "-bp$":
>>       $ref: types.yaml#/definitions/int32-array
>>       description: basis points (1/100 of a percent)
>>
>> In my case it's 1/1000 of a percent. As I'm not sure exactly what the
>> author meant by "basis points", would this apply to my patch as well?
> So please show me the value which does not fit in -bp.

"Basis points" doesn't mean anything to me, that's why I was asking for 
confirmation :)
I don't have any issue to use "-bp" at all, I'm not pusing against.

>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 1/2] dt-bindings: net: dp83826: support TX data voltage tuning

[Krzysztof Kozlowski]

On 08/02/2024 10:08, POPESCU Catalin wrote:
> On 08.02.24 09:50, Krzysztof Kozlowski wrote:
>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>>
>>
>> On 08/02/2024 09:48, POPESCU Catalin wrote:
>>> On 08.02.24 08:35, Krzysztof Kozlowski wrote:
>>>> [You don't often get email from krzysztof.kozlowski@linaro.org. 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.
>>>>
>>>>
>>>> On 07/02/2024 18:58, Catalin Popescu wrote:
>>>>> Add properties ti,cfg-dac-minus-one-milli-percent and
>>>>> ti,cfg-dac-plus-one-milli-percent to support voltage tuning
>>>>> of logical levels -1/+1 of the MLT-3 encoded TX data.
>>>>>
>>>>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>>>>> ---
>>>>> Changes in v2:
>>>>>    - squash the 2 DT bindings patches in one single patch
>>>>>    - drop redundant "binding" from the DT bindings patch title
>>>>>    - rename DT properties and define them as percentage
>>>>>    - add default value for each new DT property
>>>>> ---
>>>>>    .../devicetree/bindings/net/ti,dp83822.yaml    | 18 ++++++++++++++++++
>>>>>    1 file changed, 18 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>> index db74474207ed..6bbd465e51d6 100644
>>>>> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>> @@ -62,6 +62,24 @@ properties:
>>>>>           for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>>>>>           to transmit data.
>>>>>
>>>>> +  ti,cfg-dac-minus-one-milli-percent:
>>>>> +    description: |
>>>>> +       DP83826 PHY only.
>>>>> +       Sets the voltage ratio (with respect to the nominal value)
>>>>> +       of the logical level -1 for the MLT-3 encoded TX data.
>>>>> +    enum: [50000, 56250, 62500, 68750, 75000, 81250, 87500, 93750, 100000,
>>>>> +           106250, 112500, 118750, 125000, 131250, 137500, 143750, 150000]
>>>> I see all values being multiple of basis points, so why not using -bp
>>>> suffix?
>>> I can read :
>>>
>>>     "-bp$":
>>>       $ref: types.yaml#/definitions/int32-array
>>>       description: basis points (1/100 of a percent)
>>>
>>> In my case it's 1/1000 of a percent. As I'm not sure exactly what the
>>> author meant by "basis points", would this apply to my patch as well?
>> So please show me the value which does not fit in -bp.
> 
> "Basis points" doesn't mean anything to me, that's why I was asking for 
> confirmation :)
> I don't have any issue to use "-bp" at all, I'm not pusing against.

https://en.wikipedia.org/wiki/Basis_point

Looks like all your values fit there, at least for these devices. Maybe
you would need to be sure other devices do not require mpercent after all.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: net: dp83826: support TX data voltage tuning

[POPESCU Catalin]

On 08.02.24 10:12, Krzysztof Kozlowski wrote:
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
> On 08/02/2024 10:08, POPESCU Catalin wrote:
>> On 08.02.24 09:50, Krzysztof Kozlowski wrote:
>>> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>>>
>>>
>>> On 08/02/2024 09:48, POPESCU Catalin wrote:
>>>> On 08.02.24 08:35, Krzysztof Kozlowski wrote:
>>>>> [You don't often get email from krzysztof.kozlowski@linaro.org. 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.
>>>>>
>>>>>
>>>>> On 07/02/2024 18:58, Catalin Popescu wrote:
>>>>>> Add properties ti,cfg-dac-minus-one-milli-percent and
>>>>>> ti,cfg-dac-plus-one-milli-percent to support voltage tuning
>>>>>> of logical levels -1/+1 of the MLT-3 encoded TX data.
>>>>>>
>>>>>> Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>>     - squash the 2 DT bindings patches in one single patch
>>>>>>     - drop redundant "binding" from the DT bindings patch title
>>>>>>     - rename DT properties and define them as percentage
>>>>>>     - add default value for each new DT property
>>>>>> ---
>>>>>>     .../devicetree/bindings/net/ti,dp83822.yaml    | 18 ++++++++++++++++++
>>>>>>     1 file changed, 18 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/ti,dp83822.yaml b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>>> index db74474207ed..6bbd465e51d6 100644
>>>>>> --- a/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/net/ti,dp83822.yaml
>>>>>> @@ -62,6 +62,24 @@ properties:
>>>>>>            for the PHY.  The internal delay for the PHY is fixed to 3.5ns relative
>>>>>>            to transmit data.
>>>>>>
>>>>>> +  ti,cfg-dac-minus-one-milli-percent:
>>>>>> +    description: |
>>>>>> +       DP83826 PHY only.
>>>>>> +       Sets the voltage ratio (with respect to the nominal value)
>>>>>> +       of the logical level -1 for the MLT-3 encoded TX data.
>>>>>> +    enum: [50000, 56250, 62500, 68750, 75000, 81250, 87500, 93750, 100000,
>>>>>> +           106250, 112500, 118750, 125000, 131250, 137500, 143750, 150000]
>>>>> I see all values being multiple of basis points, so why not using -bp
>>>>> suffix?
>>>> I can read :
>>>>
>>>>      "-bp$":
>>>>        $ref: types.yaml#/definitions/int32-array
>>>>        description: basis points (1/100 of a percent)
>>>>
>>>> In my case it's 1/1000 of a percent. As I'm not sure exactly what the
>>>> author meant by "basis points", would this apply to my patch as well?
>>> So please show me the value which does not fit in -bp.
>> "Basis points" doesn't mean anything to me, that's why I was asking for
>> confirmation :)
>> I don't have any issue to use "-bp" at all, I'm not pusing against.
> https://en.wikipedia.org/wiki/Basis_point
>
> Looks like all your values fit there, at least for these devices. Maybe
> you would need to be sure other devices do not require mpercent after all.
Got it! Indeed, "-bp" completely fits my need.
> Best regards,
> Krzysztof
>

Re: [PATCH v2 2/2] net: phy: dp83826: support TX data voltage tuning

[Andrew Lunn]

> > I could be reading this wrong, but it looks like
> > DP83826_CFG_DAC_MINUS_DEFAULT actually means leave the value
> > unchanged? Is there anything guaranteeing it does in fact have the
> > default value in the hardware?
> >
> >          Andrew
> 
> Yes, the datasheet clearly states the default/reset values of both 
> registers VOD_CFG1 & VOD_CFG2 which are :
> - cfg_dac_minus : 30h
> - cfg_dac_plus : 10h

And the device is actually and always reset by Linux when the driver
loads? Anything the bootloader has done, or a previous kernel, will be
cleared?

Please add this explanation to the commit message.

I'm being pedantic because we have had problems like this in the past.
If a register was not actually set back to the default value, the
bootloader set it to some other value, the board can work fine. Then a
board can came along which the bootloader set the wrong value, and the
default is actually needed. Fixing the driver to actually enforce the
default breaks boards...

	 Andrew

Re: [PATCH v2 2/2] net: phy: dp83826: support TX data voltage tuning

[POPESCU Catalin]

Since my previous message has been rejected due to HTML content, I'm 
resending it. Sorry, for the inconvenience.

On 08.02.24 14:56, Andrew Lunn wrote:
> [Some people who received this message 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.
>
>
>>> I could be reading this wrong, but it looks like
>>> DP83826_CFG_DAC_MINUS_DEFAULT actually means leave the value
>>> unchanged? Is there anything guaranteeing it does in fact have the
>>> default value in the hardware?
>>>
>>>           Andrew
>> Yes, the datasheet clearly states the default/reset values of both
>> registers VOD_CFG1 & VOD_CFG2 which are :
>> - cfg_dac_minus : 30h
>> - cfg_dac_plus : 10h
> And the device is actually and always reset by Linux when the driver
> loads? Anything the bootloader has done, or a previous kernel, will be
> cleared?
Now, I understand your question 🙂
To answer, DP83826_CFG_DAC_MINUS_DEFAULT will indeed leave the register 
unchanged. However, dp83822 driver exports a PHY callback soft_reset 
which does a SW reset which actually has the same effect as the HW reset 
pin according to the datasheet. Since the PAL enforces the call to 
soft_reset before config_init, in dp83826_config_init we can rely on the 
registers reset value.
> Please add this explanation to the commit message.
>
> I'm being pedantic because we have had problems like this in the past.
> If a register was not actually set back to the default value, the
> bootloader set it to some other value, the board can work fine. Then a
> board can came along which the bootloader set the wrong value, and the
> default is actually needed. Fixing the driver to actually enforce the
> default breaks boards...
>
>           Andrew

Re: [PATCH v2 2/2] net: phy: dp83826: support TX data voltage tuning

[Andrew Lunn]

> Now, I understand your question 🙂
> To answer, DP83826_CFG_DAC_MINUS_DEFAULT will indeed leave the register 
> unchanged. However, dp83822 driver exports a PHY callback soft_reset 
> which does a SW reset which actually has the same effect as the HW reset 
> pin according to the datasheet. Since the PAL enforces the call to 
> soft_reset before config_init, in dp83826_config_init we can rely on the 
> registers reset value.

Great. Please add a version of this to the commit message. That shows
we did our due diligence and we don't expect any surprises in the
future.

    Andrew

---
pw-bot: cr

Re: [PATCH v2 2/2] net: phy: dp83826: support TX data voltage tuning

[POPESCU Catalin]

On 08.02.24 17:50, Andrew Lunn wrote:
> [Some people who received this message 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.
>
>
>> Now, I understand your question 🙂
>> To answer, DP83826_CFG_DAC_MINUS_DEFAULT will indeed leave the register
>> unchanged. However, dp83822 driver exports a PHY callback soft_reset
>> which does a SW reset which actually has the same effect as the HW reset
>> pin according to the datasheet. Since the PAL enforces the call to
>> soft_reset before config_init, in dp83826_config_init we can rely on the
>> registers reset value.
> Great. Please add a version of this to the commit message. That shows
> we did our due diligence and we don't expect any surprises in the
> future.
Sure, I'll update the commit message in v3.
>      Andrew
>
> ---
> pw-bot: cr