Add properties ti,cfg-dac-minus-one-bp/ti,cfg-dac-plus-one-bp
to support voltage tuning of logical levels -1/+1 of the MLT-3
encoded TX data.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Catalin Popescu <[email protected]>
---
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
Changes in v3:
- rename properties to "-bp" and change their admissible values
accordingly
Changes in v4:
- add reviewed-by tag
---
.../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..8f4350be689c 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-bp:
+ 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: [5000, 5625, 6250, 6875, 7500, 8125, 8750, 9375, 10000,
+ 10625, 11250, 11875, 12500, 13125, 13750, 14375, 15000]
+ default: 10000
+
+ ti,cfg-dac-plus-one-bp:
+ 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: [5000, 5625, 6250, 6875, 7500, 8125, 8750, 9375, 10000,
+ 10625, 11250, 11875, 12500, 13125, 13750, 14375, 15000]
+ default: 10000
+
required:
- reg
base-commit: 1f719a2f3fa67665578c759ac34fd3d3690c1a20
prerequisite-patch-id: 0000000000000000000000000000000000000000
--
2.34.1
DP83826 offers the possibility to tune the voltage of logical
levels of the MLT-3 encoded TX data. This is useful when there
is a voltage drop in between the PHY and the connector and we
want to increase the voltage levels to compensate for that drop.
Prior to PHY configuration, the driver SW resets the PHY which has
the same effect as the HW reset pin according to the datasheet.
Hence, there's no need to force update the VOD_CFG registers to make
sure they hold their reset values. VOD_CFG registers need to be
updated only if the DT has been configured with values other than
the reset ones.
Signed-off-by: Catalin Popescu <[email protected]>
---
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
Changes in v3:
- rename DT properties to "-bp"
- rename DP83826_CFG_DAC_MPERCENT_DEFAULT/DP83826_CFG_DAC_MPERCENT_PER_STEP
to DP83826_CFG_DAC_PERCENT_DEFAULT/DP83826_CFG_DAC_PERCENT_PER_STEP and
update their values to reflect the new unit "basis point"
- update commit message with explanation about the registers reset
values
Changes in v4:
- update commit message to be more meaningful
---
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..30f2616ab1c2 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_PERCENT_PER_STEP 625
+#define DP83826_CFG_DAC_PERCENT_DEFAULT 10000
+#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_PERCENT_DEFAULT - percent;
+
+ return tmp / DP83826_CFG_DAC_PERCENT_PER_STEP;
+}
+
+static int dp83826_to_dac_plus_one_regval(int percent)
+{
+ int tmp = percent - DP83826_CFG_DAC_PERCENT_DEFAULT;
+
+ return tmp / DP83826_CFG_DAC_PERCENT_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-bp", &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-bp", &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
I just figured out that I forgot to disable WOL in the callback config_init.
It looks the PHY driver should explicitly disable WOL feature at init,
and leave it to ethtool to be enabled.
I will provide a v5 ASAP to fix that.
On 12.02.24 08:46, Catalin Popescu wrote:
> DP83826 offers the possibility to tune the voltage of logical
> levels of the MLT-3 encoded TX data. This is useful when there
> is a voltage drop in between the PHY and the connector and we
> want to increase the voltage levels to compensate for that drop.
>
> Prior to PHY configuration, the driver SW resets the PHY which has
> the same effect as the HW reset pin according to the datasheet.
> Hence, there's no need to force update the VOD_CFG registers to make
> sure they hold their reset values. VOD_CFG registers need to be
> updated only if the DT has been configured with values other than
> the reset ones.
>
> Signed-off-by: Catalin Popescu <[email protected]>
> ---
> 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
>
> Changes in v3:
> - rename DT properties to "-bp"
> - rename DP83826_CFG_DAC_MPERCENT_DEFAULT/DP83826_CFG_DAC_MPERCENT_PER_STEP
> to DP83826_CFG_DAC_PERCENT_DEFAULT/DP83826_CFG_DAC_PERCENT_PER_STEP and
> update their values to reflect the new unit "basis point"
> - update commit message with explanation about the registers reset
> values
>
> Changes in v4:
> - update commit message to be more meaningful
> ---
> 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..30f2616ab1c2 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_PERCENT_PER_STEP 625
> +#define DP83826_CFG_DAC_PERCENT_DEFAULT 10000
> +#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_PERCENT_DEFAULT - percent;
> +
> + return tmp / DP83826_CFG_DAC_PERCENT_PER_STEP;
> +}
> +
> +static int dp83826_to_dac_plus_one_regval(int percent)
> +{
> + int tmp = percent - DP83826_CFG_DAC_PERCENT_DEFAULT;
> +
> + return tmp / DP83826_CFG_DAC_PERCENT_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-bp", &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-bp", &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"),
On 12.02.24 15:37, Andrew Lunn wrote:
> [Some people who received this message don't often get email from [email protected]. 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 Mon, Feb 12, 2024 at 02:15:47PM +0000, POPESCU Catalin wrote:
>> I just figured out that I forgot to disable WOL in the callback config_init.
>> It looks the PHY driver should explicitly disable WOL feature at init,
>> and leave it to ethtool to be enabled.
>> I will provide a v5 ASAP to fix that.
> WoL is a bit murky. On x86, it can be the BIOS which configures WoL,
> behind the back of Linux. That is not something i would actually
> recommend, so disabling it at boot does make sense. But consider
> suspend and resume. If the PHY is used for WoL, the WoL settings
> should be kept through suspend/resume. So you need to be careful where
> you disable it, so its only disables on boot, not resume.
>
> Andrew
I see. Then, I should not disable WOL in config_init, since this
callback is also called on the resume path :
mdio_bus_phy_resume->phy_init_hw->config_init.
However, this contrasts with the rest of the driver and my patch is not
anymore about just adding support for tuning of TX voltage levels ...
> I see. Then, I should not disable WOL in config_init, since this
> callback is also called on the resume path :
> mdio_bus_phy_resume->phy_init_hw->config_init.
> However, this contrasts with the rest of the driver and my patch is not
> anymore about just adding support for tuning of TX voltage levels ...
So wait until your patch is merged and then follow up with another
patch disabling WoL. Patches get merged pretty quickly, so there is
plenty of time before the next merge window.
Andrew
On Mon, Feb 12, 2024 at 02:15:47PM +0000, POPESCU Catalin wrote:
> I just figured out that I forgot to disable WOL in the callback config_init.
> It looks the PHY driver should explicitly disable WOL feature at init,
> and leave it to ethtool to be enabled.
> I will provide a v5 ASAP to fix that.
WoL is a bit murky. On x86, it can be the BIOS which configures WoL,
behind the back of Linux. That is not something i would actually
recommend, so disabling it at boot does make sense. But consider
suspend and resume. If the PHY is used for WoL, the WoL settings
should be kept through suspend/resume. So you need to be careful where
you disable it, so its only disables on boot, not resume.
Andrew
On Mon, Feb 12, 2024 at 08:46:49AM +0100, Catalin Popescu wrote:
> DP83826 offers the possibility to tune the voltage of logical
> levels of the MLT-3 encoded TX data. This is useful when there
> is a voltage drop in between the PHY and the connector and we
> want to increase the voltage levels to compensate for that drop.
>
> Prior to PHY configuration, the driver SW resets the PHY which has
> the same effect as the HW reset pin according to the datasheet.
> Hence, there's no need to force update the VOD_CFG registers to make
> sure they hold their reset values. VOD_CFG registers need to be
> updated only if the DT has been configured with values other than
> the reset ones.
>
> Signed-off-by: Catalin Popescu <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
FYI: The WoL discussion should not prevent this from being merged.
Andrew