[PATCH net-next v2] net: micrel: Add support for lan8841 PHY

[PATCH net-next v2] net: micrel: Add support for lan8841 PHY

[Horatiu Vultur]

The LAN8841 is completely integrated triple-speed (10BASE-T/ 100BASE-TX/
1000BASE-T) Ethernet physical layer transceivers for transmission and
reception of data on standard CAT-5, as well as CAT-5e and CAT-6,
unshielded twisted pair (UTP) cables.
The LAN8841 offers the industry-standard GMII/MII as well as the RGMII.
Some of the features of the PHY are:
- Wake on LAN
- Auto-MDIX
- IEEE 1588-2008 (V2)
- LinkMD Capable diagnosis

Currently the patch offers support only for link configuration.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
v1->v2:
- Remove hardcoded values
- Fix typo in commit message
---
 drivers/net/phy/micrel.c   | 227 +++++++++++++++++++++++++++++++++++--
 include/linux/micrel_phy.h |   1 +
 2 files changed, 219 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index d5b80c31ab91c..53c37ac66c343 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -26,6 +26,7 @@
 #include <linux/phy.h>
 #include <linux/micrel_phy.h>
 #include <linux/of.h>
+#include <linux/irq.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/ptp_clock_kernel.h>
@@ -268,6 +269,9 @@ struct kszphy_type {
 	u16 interrupt_level_mask;
 	u16 cable_diag_reg;
 	unsigned long pair_mask;
+	u16 disable_dll_tx_bit;
+	u16 disable_dll_rx_bit;
+	u16 disable_dll_mask;
 	bool has_broadcast_disable;
 	bool has_nand_tree_disable;
 	bool has_rmii_ref_clk_sel;
@@ -364,6 +368,19 @@ static const struct kszphy_type ksz9021_type = {
 	.interrupt_level_mask	= BIT(14),
 };
 
+static const struct kszphy_type ksz9131_type = {
+	.interrupt_level_mask	= BIT(14),
+	.disable_dll_tx_bit	= BIT(12),
+	.disable_dll_rx_bit	= BIT(12),
+	.disable_dll_mask	= BIT_MASK(12),
+};
+
+static const struct kszphy_type lan8841_type = {
+	.disable_dll_tx_bit	= BIT(14),
+	.disable_dll_rx_bit	= BIT(14),
+	.disable_dll_mask	= BIT_MASK(14),
+};
+
 static int kszphy_extended_write(struct phy_device *phydev,
 				u32 regnum, u16 val)
 {
@@ -1172,19 +1189,18 @@ static int ksz9131_of_load_skew_values(struct phy_device *phydev,
 #define KSZ9131RN_MMD_COMMON_CTRL_REG	2
 #define KSZ9131RN_RXC_DLL_CTRL		76
 #define KSZ9131RN_TXC_DLL_CTRL		77
-#define KSZ9131RN_DLL_CTRL_BYPASS	BIT_MASK(12)
 #define KSZ9131RN_DLL_ENABLE_DELAY	0
-#define KSZ9131RN_DLL_DISABLE_DELAY	BIT(12)
 
 static int ksz9131_config_rgmii_delay(struct phy_device *phydev)
 {
+	const struct kszphy_type *type = phydev->drv->driver_data;
 	u16 rxcdll_val, txcdll_val;
 	int ret;
 
 	switch (phydev->interface) {
 	case PHY_INTERFACE_MODE_RGMII:
-		rxcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
-		txcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
+		rxcdll_val = type->disable_dll_rx_bit;
+		txcdll_val = type->disable_dll_tx_bit;
 		break;
 	case PHY_INTERFACE_MODE_RGMII_ID:
 		rxcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
@@ -1192,10 +1208,10 @@ static int ksz9131_config_rgmii_delay(struct phy_device *phydev)
 		break;
 	case PHY_INTERFACE_MODE_RGMII_RXID:
 		rxcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
-		txcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
+		txcdll_val = type->disable_dll_tx_bit;
 		break;
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		rxcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
+		rxcdll_val = type->disable_dll_rx_bit;
 		txcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
 		break;
 	default:
@@ -1203,13 +1219,13 @@ static int ksz9131_config_rgmii_delay(struct phy_device *phydev)
 	}
 
 	ret = phy_modify_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
-			     KSZ9131RN_RXC_DLL_CTRL, KSZ9131RN_DLL_CTRL_BYPASS,
+			     KSZ9131RN_RXC_DLL_CTRL, type->disable_dll_mask,
 			     rxcdll_val);
 	if (ret < 0)
 		return ret;
 
 	return phy_modify_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
-			      KSZ9131RN_TXC_DLL_CTRL, KSZ9131RN_DLL_CTRL_BYPASS,
+			      KSZ9131RN_TXC_DLL_CTRL, type->disable_dll_mask,
 			      txcdll_val);
 }
 
@@ -3152,6 +3168,183 @@ static int lan8814_probe(struct phy_device *phydev)
 	return 0;
 }
 
+#define LAN8841_MMD_TIMER_REG			0
+#define LAN8841_MMD0_REGISTER_17		17
+#define LAN8841_MMD0_REGISTER_17_DROP_OPT(x)	((x) & 0x3)
+#define LAN8841_MMD0_REGISTER_17_XMIT_TOG_TX_DIS	BIT(3)
+#define LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG	2
+#define LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG_MAGJACK	BIT(14)
+#define LAN8841_MMD_ANALOG_REG			28
+#define LAN8841_ANALOG_CONTROL_1		1
+#define LAN8841_ANALOG_CONTROL_1_PLL_TRIM(x)	(((x) & 0x3) << 5)
+#define LAN8841_ANALOG_CONTROL_10		13
+#define LAN8841_ANALOG_CONTROL_10_PLL_DIV(x)	((x) & 0x3)
+#define LAN8841_ANALOG_CONTROL_11		14
+#define LAN8841_ANALOG_CONTROL_11_LDO_REF(x)	(((x) & 0x7) << 12)
+#define LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT	69
+#define LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT_VAL 0xbffc
+#define LAN8841_BTRX_POWER_DOWN			70
+#define LAN8841_BTRX_POWER_DOWN_QBIAS_CH_A	BIT(0)
+#define LAN8841_BTRX_POWER_DOWN_BTRX_CH_A	BIT(1)
+#define LAN8841_BTRX_POWER_DOWN_QBIAS_CH_B	BIT(2)
+#define LAN8841_BTRX_POWER_DOWN_BTRX_CH_B	BIT(3)
+#define LAN8841_BTRX_POWER_DOWN_BTRX_CH_C	BIT(5)
+#define LAN8841_BTRX_POWER_DOWN_BTRX_CH_D	BIT(7)
+#define LAN8841_ADC_CHANNEL_MASK		198
+static int lan8841_config_init(struct phy_device *phydev)
+{
+	char *rx_data_skews[4] = {"rxd0-skew-psec", "rxd1-skew-psec",
+				  "rxd2-skew-psec", "rxd3-skew-psec"};
+	char *tx_data_skews[4] = {"txd0-skew-psec", "txd1-skew-psec",
+				  "txd2-skew-psec", "txd3-skew-psec"};
+	char *clk_skews[2] = {"rxc-skew-psec", "txc-skew-psec"};
+	struct device_node *of_node;
+	int ret;
+
+	if (phy_interface_is_rgmii(phydev)) {
+		ret = ksz9131_config_rgmii_delay(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
+	of_node = phydev->mdio.dev.of_node;
+	if (!of_node)
+		goto hw_init;
+
+	ret = ksz9131_of_load_skew_values(phydev, of_node,
+					  MII_KSZ9031RN_CLK_PAD_SKEW, 5,
+					  clk_skews, 2);
+	if (ret < 0)
+		return ret;
+
+	ret = ksz9131_of_load_skew_values(phydev, of_node,
+					  MII_KSZ9031RN_RX_DATA_PAD_SKEW, 4,
+					  rx_data_skews, 4);
+	if (ret < 0)
+		return ret;
+
+	ret = ksz9131_of_load_skew_values(phydev, of_node,
+					  MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4,
+					  tx_data_skews, 4);
+	if (ret < 0)
+		return ret;
+
+hw_init:
+	/* 100BT Clause 40 improvenent errata */
+	phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
+		      LAN8841_ANALOG_CONTROL_1,
+		      LAN8841_ANALOG_CONTROL_1_PLL_TRIM(0x2));
+	phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
+		      LAN8841_ANALOG_CONTROL_10,
+		      LAN8841_ANALOG_CONTROL_10_PLL_DIV(0x1));
+
+	/* 10M/100M Ethernet Signal Tuning Errata for Shorted-Center Tap
+	 * Magnetics
+	 */
+	ret = phy_read_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
+			   LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG);
+	if (ret & LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG_MAGJACK) {
+		phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
+			      LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT,
+			      LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT_VAL);
+		phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
+			      LAN8841_BTRX_POWER_DOWN,
+			      LAN8841_BTRX_POWER_DOWN_QBIAS_CH_A |
+			      LAN8841_BTRX_POWER_DOWN_BTRX_CH_A |
+			      LAN8841_BTRX_POWER_DOWN_QBIAS_CH_B |
+			      LAN8841_BTRX_POWER_DOWN_BTRX_CH_B |
+			      LAN8841_BTRX_POWER_DOWN_BTRX_CH_C |
+			      LAN8841_BTRX_POWER_DOWN_BTRX_CH_D);
+	}
+
+	/* LDO Adjustment errata */
+	phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
+		      LAN8841_ANALOG_CONTROL_11,
+		      LAN8841_ANALOG_CONTROL_11_LDO_REF(1));
+
+	/* 100BT RGMII latency tuning errata */
+	phy_write_mmd(phydev, MDIO_MMD_PMAPMD,
+		      LAN8841_ADC_CHANNEL_MASK, 0x0);
+	phy_write_mmd(phydev, LAN8841_MMD_TIMER_REG,
+		      LAN8841_MMD0_REGISTER_17,
+		      LAN8841_MMD0_REGISTER_17_DROP_OPT(2) |
+		      LAN8841_MMD0_REGISTER_17_XMIT_TOG_TX_DIS);
+
+	return 0;
+}
+
+#define LAN8841_OUTPUT_CTRL			25
+#define LAN8841_OUTPUT_CTRL_INT_BUFFER		BIT(14)
+#define LAN8841_CTRL				31
+#define LAN8841_CTRL_INTR_POLARITY		BIT(14)
+static int lan8841_config_intr(struct phy_device *phydev)
+{
+	struct irq_data *irq_data;
+	int temp = 0;
+
+	irq_data = irq_get_irq_data(phydev->irq);
+	if (!irq_data)
+		return 0;
+
+	if (irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_HIGH) {
+		/* Change polarity of the interrupt */
+		phy_modify(phydev, LAN8841_OUTPUT_CTRL,
+			   LAN8841_OUTPUT_CTRL_INT_BUFFER,
+			   LAN8841_OUTPUT_CTRL_INT_BUFFER);
+		phy_modify(phydev, LAN8841_CTRL,
+			   LAN8841_CTRL_INTR_POLARITY,
+			   LAN8841_CTRL_INTR_POLARITY);
+	} else {
+		/* It is enough to set INT buffer to open-drain because then
+		 * the interrupt will be active low.
+		 */
+		phy_modify(phydev, LAN8841_OUTPUT_CTRL,
+			   LAN8841_OUTPUT_CTRL_INT_BUFFER, 0);
+	}
+
+	/* enable / disable interrupts */
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		temp = LAN8814_INT_LINK;
+
+	return phy_write(phydev, LAN8814_INTC, temp);
+}
+
+static irqreturn_t lan8841_handle_interrupt(struct phy_device *phydev)
+{
+	int irq_status;
+
+	irq_status = phy_read(phydev, LAN8814_INTS);
+	if (irq_status < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	if (irq_status & LAN8814_INT_LINK) {
+		phy_trigger_machine(phydev);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+#define LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER 3
+#define LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER_STRAP_RGMII_EN BIT(0)
+static int lan8841_probe(struct phy_device *phydev)
+{
+	int err;
+
+	err = kszphy_probe(phydev);
+	if (err)
+		return err;
+
+	if (phy_read_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
+			 LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER) &
+	    LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER_STRAP_RGMII_EN)
+		phydev->interface = PHY_INTERFACE_MODE_RGMII_RXID;
+
+	return 0;
+}
+
 static struct phy_driver ksphy_driver[] = {
 {
 	.phy_id		= PHY_ID_KS8737,
@@ -3361,13 +3554,28 @@ static struct phy_driver ksphy_driver[] = {
 	.resume		= kszphy_resume,
 	.config_intr	= lan8804_config_intr,
 	.handle_interrupt = lan8804_handle_interrupt,
+}, {
+	.phy_id		= PHY_ID_LAN8841,
+	.phy_id_mask	= MICREL_PHY_ID_MASK,
+	.name		= "Microchip LAN8841 Gigabit PHY",
+	.driver_data	= &lan8841_type,
+	.config_init	= lan8841_config_init,
+	.probe		= lan8841_probe,
+	.soft_reset	= genphy_soft_reset,
+	.config_intr	= lan8841_config_intr,
+	.handle_interrupt = lan8841_handle_interrupt,
+	.get_sset_count = kszphy_get_sset_count,
+	.get_strings	= kszphy_get_strings,
+	.get_stats	= kszphy_get_stats,
+	.suspend	= genphy_suspend,
+	.resume		= genphy_resume,
 }, {
 	.phy_id		= PHY_ID_KSZ9131,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Microchip KSZ9131 Gigabit PHY",
 	/* PHY_GBIT_FEATURES */
 	.flags		= PHY_POLL_CABLE_TEST,
-	.driver_data	= &ksz9021_type,
+	.driver_data	= &ksz9131_type,
 	.probe		= kszphy_probe,
 	.config_init	= ksz9131_config_init,
 	.config_intr	= kszphy_config_intr,
@@ -3446,6 +3654,7 @@ static struct mdio_device_id __maybe_unused micrel_tbl[] = {
 	{ PHY_ID_KSZ886X, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
+	{ PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
 	{ }
 };
 
diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
index 771e050883db7..8bef1ab62bba3 100644
--- a/include/linux/micrel_phy.h
+++ b/include/linux/micrel_phy.h
@@ -31,6 +31,7 @@
 #define PHY_ID_KSZ9131		0x00221640
 #define PHY_ID_LAN8814		0x00221660
 #define PHY_ID_LAN8804		0x00221670
+#define PHY_ID_LAN8841		0x00221650
 
 #define PHY_ID_KSZ886X		0x00221430
 #define PHY_ID_KSZ8863		0x00221435
-- 
2.38.0

Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY

[Heiner Kallweit]

On 03.02.2023 13:25, Horatiu Vultur wrote:
> The LAN8841 is completely integrated triple-speed (10BASE-T/ 100BASE-TX/
> 1000BASE-T) Ethernet physical layer transceivers for transmission and
> reception of data on standard CAT-5, as well as CAT-5e and CAT-6,
> unshielded twisted pair (UTP) cables.
> The LAN8841 offers the industry-standard GMII/MII as well as the RGMII.
> Some of the features of the PHY are:
> - Wake on LAN
> - Auto-MDIX
> - IEEE 1588-2008 (V2)
> - LinkMD Capable diagnosis
> 
> Currently the patch offers support only for link configuration.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
> v1->v2:
> - Remove hardcoded values
> - Fix typo in commit message
> ---
>  drivers/net/phy/micrel.c   | 227 +++++++++++++++++++++++++++++++++++--
>  include/linux/micrel_phy.h |   1 +
>  2 files changed, 219 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index d5b80c31ab91c..53c37ac66c343 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -26,6 +26,7 @@
>  #include <linux/phy.h>
>  #include <linux/micrel_phy.h>
>  #include <linux/of.h>
> +#include <linux/irq.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/ptp_clock_kernel.h>
> @@ -268,6 +269,9 @@ struct kszphy_type {
>  	u16 interrupt_level_mask;
>  	u16 cable_diag_reg;
>  	unsigned long pair_mask;
> +	u16 disable_dll_tx_bit;
> +	u16 disable_dll_rx_bit;
> +	u16 disable_dll_mask;
>  	bool has_broadcast_disable;
>  	bool has_nand_tree_disable;
>  	bool has_rmii_ref_clk_sel;
> @@ -364,6 +368,19 @@ static const struct kszphy_type ksz9021_type = {
>  	.interrupt_level_mask	= BIT(14),
>  };
>  
> +static const struct kszphy_type ksz9131_type = {
> +	.interrupt_level_mask	= BIT(14),
> +	.disable_dll_tx_bit	= BIT(12),
> +	.disable_dll_rx_bit	= BIT(12),
> +	.disable_dll_mask	= BIT_MASK(12),
> +};
> +
> +static const struct kszphy_type lan8841_type = {
> +	.disable_dll_tx_bit	= BIT(14),
> +	.disable_dll_rx_bit	= BIT(14),
> +	.disable_dll_mask	= BIT_MASK(14),
> +};
> +
>  static int kszphy_extended_write(struct phy_device *phydev,
>  				u32 regnum, u16 val)
>  {
> @@ -1172,19 +1189,18 @@ static int ksz9131_of_load_skew_values(struct phy_device *phydev,
>  #define KSZ9131RN_MMD_COMMON_CTRL_REG	2
>  #define KSZ9131RN_RXC_DLL_CTRL		76
>  #define KSZ9131RN_TXC_DLL_CTRL		77
> -#define KSZ9131RN_DLL_CTRL_BYPASS	BIT_MASK(12)
>  #define KSZ9131RN_DLL_ENABLE_DELAY	0
> -#define KSZ9131RN_DLL_DISABLE_DELAY	BIT(12)
>  
>  static int ksz9131_config_rgmii_delay(struct phy_device *phydev)
>  {
> +	const struct kszphy_type *type = phydev->drv->driver_data;
>  	u16 rxcdll_val, txcdll_val;
>  	int ret;
>  
>  	switch (phydev->interface) {
>  	case PHY_INTERFACE_MODE_RGMII:
> -		rxcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> -		txcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> +		rxcdll_val = type->disable_dll_rx_bit;
> +		txcdll_val = type->disable_dll_tx_bit;
>  		break;
>  	case PHY_INTERFACE_MODE_RGMII_ID:
>  		rxcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> @@ -1192,10 +1208,10 @@ static int ksz9131_config_rgmii_delay(struct phy_device *phydev)
>  		break;
>  	case PHY_INTERFACE_MODE_RGMII_RXID:
>  		rxcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> -		txcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> +		txcdll_val = type->disable_dll_tx_bit;
>  		break;
>  	case PHY_INTERFACE_MODE_RGMII_TXID:
> -		rxcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> +		rxcdll_val = type->disable_dll_rx_bit;
>  		txcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
>  		break;
>  	default:
> @@ -1203,13 +1219,13 @@ static int ksz9131_config_rgmii_delay(struct phy_device *phydev)
>  	}
>  
>  	ret = phy_modify_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
> -			     KSZ9131RN_RXC_DLL_CTRL, KSZ9131RN_DLL_CTRL_BYPASS,
> +			     KSZ9131RN_RXC_DLL_CTRL, type->disable_dll_mask,
>  			     rxcdll_val);
>  	if (ret < 0)
>  		return ret;
>  
>  	return phy_modify_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
> -			      KSZ9131RN_TXC_DLL_CTRL, KSZ9131RN_DLL_CTRL_BYPASS,
> +			      KSZ9131RN_TXC_DLL_CTRL, type->disable_dll_mask,
>  			      txcdll_val);
>  }
>  
> @@ -3152,6 +3168,183 @@ static int lan8814_probe(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +#define LAN8841_MMD_TIMER_REG			0
> +#define LAN8841_MMD0_REGISTER_17		17
> +#define LAN8841_MMD0_REGISTER_17_DROP_OPT(x)	((x) & 0x3)
> +#define LAN8841_MMD0_REGISTER_17_XMIT_TOG_TX_DIS	BIT(3)
> +#define LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG	2
> +#define LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG_MAGJACK	BIT(14)
> +#define LAN8841_MMD_ANALOG_REG			28
> +#define LAN8841_ANALOG_CONTROL_1		1
> +#define LAN8841_ANALOG_CONTROL_1_PLL_TRIM(x)	(((x) & 0x3) << 5)
> +#define LAN8841_ANALOG_CONTROL_10		13
> +#define LAN8841_ANALOG_CONTROL_10_PLL_DIV(x)	((x) & 0x3)
> +#define LAN8841_ANALOG_CONTROL_11		14
> +#define LAN8841_ANALOG_CONTROL_11_LDO_REF(x)	(((x) & 0x7) << 12)
> +#define LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT	69
> +#define LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT_VAL 0xbffc
> +#define LAN8841_BTRX_POWER_DOWN			70
> +#define LAN8841_BTRX_POWER_DOWN_QBIAS_CH_A	BIT(0)
> +#define LAN8841_BTRX_POWER_DOWN_BTRX_CH_A	BIT(1)
> +#define LAN8841_BTRX_POWER_DOWN_QBIAS_CH_B	BIT(2)
> +#define LAN8841_BTRX_POWER_DOWN_BTRX_CH_B	BIT(3)
> +#define LAN8841_BTRX_POWER_DOWN_BTRX_CH_C	BIT(5)
> +#define LAN8841_BTRX_POWER_DOWN_BTRX_CH_D	BIT(7)
> +#define LAN8841_ADC_CHANNEL_MASK		198
> +static int lan8841_config_init(struct phy_device *phydev)
> +{
> +	char *rx_data_skews[4] = {"rxd0-skew-psec", "rxd1-skew-psec",
> +				  "rxd2-skew-psec", "rxd3-skew-psec"};
> +	char *tx_data_skews[4] = {"txd0-skew-psec", "txd1-skew-psec",
> +				  "txd2-skew-psec", "txd3-skew-psec"};
> +	char *clk_skews[2] = {"rxc-skew-psec", "txc-skew-psec"};
> +	struct device_node *of_node;
> +	int ret;
> +
> +	if (phy_interface_is_rgmii(phydev)) {
> +		ret = ksz9131_config_rgmii_delay(phydev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	of_node = phydev->mdio.dev.of_node;
> +	if (!of_node)
> +		goto hw_init;
> +
> +	ret = ksz9131_of_load_skew_values(phydev, of_node,
> +					  MII_KSZ9031RN_CLK_PAD_SKEW, 5,
> +					  clk_skews, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ksz9131_of_load_skew_values(phydev, of_node,
> +					  MII_KSZ9031RN_RX_DATA_PAD_SKEW, 4,
> +					  rx_data_skews, 4);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ksz9131_of_load_skew_values(phydev, of_node,
> +					  MII_KSZ9031RN_TX_DATA_PAD_SKEW, 4,
> +					  tx_data_skews, 4);
> +	if (ret < 0)
> +		return ret;
> +
> +hw_init:
> +	/* 100BT Clause 40 improvenent errata */
> +	phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> +		      LAN8841_ANALOG_CONTROL_1,
> +		      LAN8841_ANALOG_CONTROL_1_PLL_TRIM(0x2));
> +	phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> +		      LAN8841_ANALOG_CONTROL_10,
> +		      LAN8841_ANALOG_CONTROL_10_PLL_DIV(0x1));
> +
> +	/* 10M/100M Ethernet Signal Tuning Errata for Shorted-Center Tap
> +	 * Magnetics
> +	 */
> +	ret = phy_read_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
> +			   LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG);
> +	if (ret & LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG_MAGJACK) {
> +		phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> +			      LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT,
> +			      LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT_VAL);
> +		phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> +			      LAN8841_BTRX_POWER_DOWN,
> +			      LAN8841_BTRX_POWER_DOWN_QBIAS_CH_A |
> +			      LAN8841_BTRX_POWER_DOWN_BTRX_CH_A |
> +			      LAN8841_BTRX_POWER_DOWN_QBIAS_CH_B |
> +			      LAN8841_BTRX_POWER_DOWN_BTRX_CH_B |
> +			      LAN8841_BTRX_POWER_DOWN_BTRX_CH_C |
> +			      LAN8841_BTRX_POWER_DOWN_BTRX_CH_D);
> +	}
> +
> +	/* LDO Adjustment errata */
> +	phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> +		      LAN8841_ANALOG_CONTROL_11,
> +		      LAN8841_ANALOG_CONTROL_11_LDO_REF(1));
> +
> +	/* 100BT RGMII latency tuning errata */
> +	phy_write_mmd(phydev, MDIO_MMD_PMAPMD,
> +		      LAN8841_ADC_CHANNEL_MASK, 0x0);
> +	phy_write_mmd(phydev, LAN8841_MMD_TIMER_REG,
> +		      LAN8841_MMD0_REGISTER_17,
> +		      LAN8841_MMD0_REGISTER_17_DROP_OPT(2) |
> +		      LAN8841_MMD0_REGISTER_17_XMIT_TOG_TX_DIS);
> +
> +	return 0;
> +}
> +
> +#define LAN8841_OUTPUT_CTRL			25
> +#define LAN8841_OUTPUT_CTRL_INT_BUFFER		BIT(14)
> +#define LAN8841_CTRL				31
> +#define LAN8841_CTRL_INTR_POLARITY		BIT(14)
> +static int lan8841_config_intr(struct phy_device *phydev)
> +{
> +	struct irq_data *irq_data;
> +	int temp = 0;
> +
> +	irq_data = irq_get_irq_data(phydev->irq);
> +	if (!irq_data)
> +		return 0;
> +
> +	if (irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_HIGH) {
> +		/* Change polarity of the interrupt */

Why this a little bit esoteric logic? Can't you set the interrupt
to level-low in the chip (like most other ones), and then define
the polarity the usual way e.g. in DT?

> +		phy_modify(phydev, LAN8841_OUTPUT_CTRL,
> +			   LAN8841_OUTPUT_CTRL_INT_BUFFER,
> +			   LAN8841_OUTPUT_CTRL_INT_BUFFER);
> +		phy_modify(phydev, LAN8841_CTRL,
> +			   LAN8841_CTRL_INTR_POLARITY,
> +			   LAN8841_CTRL_INTR_POLARITY);
> +	} else {
> +		/* It is enough to set INT buffer to open-drain because then
> +		 * the interrupt will be active low.
> +		 */
> +		phy_modify(phydev, LAN8841_OUTPUT_CTRL,
> +			   LAN8841_OUTPUT_CTRL_INT_BUFFER, 0);
> +	}
> +
> +	/* enable / disable interrupts */
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> +		temp = LAN8814_INT_LINK;
> +
> +	return phy_write(phydev, LAN8814_INTC, temp);
> +}
> +
> +static irqreturn_t lan8841_handle_interrupt(struct phy_device *phydev)
> +{
> +	int irq_status;
> +
> +	irq_status = phy_read(phydev, LAN8814_INTS);
> +	if (irq_status < 0) {
> +		phy_error(phydev);
> +		return IRQ_NONE;
> +	}
> +
> +	if (irq_status & LAN8814_INT_LINK) {
> +		phy_trigger_machine(phydev);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +#define LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER 3
> +#define LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER_STRAP_RGMII_EN BIT(0)
> +static int lan8841_probe(struct phy_device *phydev)
> +{
> +	int err;
> +
> +	err = kszphy_probe(phydev);
> +	if (err)
> +		return err;
> +
> +	if (phy_read_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
> +			 LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER) &
> +	    LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER_STRAP_RGMII_EN)
> +		phydev->interface = PHY_INTERFACE_MODE_RGMII_RXID;
> +
> +	return 0;
> +}
> +
>  static struct phy_driver ksphy_driver[] = {
>  {
>  	.phy_id		= PHY_ID_KS8737,
> @@ -3361,13 +3554,28 @@ static struct phy_driver ksphy_driver[] = {
>  	.resume		= kszphy_resume,
>  	.config_intr	= lan8804_config_intr,
>  	.handle_interrupt = lan8804_handle_interrupt,
> +}, {
> +	.phy_id		= PHY_ID_LAN8841,
> +	.phy_id_mask	= MICREL_PHY_ID_MASK,
> +	.name		= "Microchip LAN8841 Gigabit PHY",
> +	.driver_data	= &lan8841_type,
> +	.config_init	= lan8841_config_init,
> +	.probe		= lan8841_probe,
> +	.soft_reset	= genphy_soft_reset,
> +	.config_intr	= lan8841_config_intr,
> +	.handle_interrupt = lan8841_handle_interrupt,
> +	.get_sset_count = kszphy_get_sset_count,
> +	.get_strings	= kszphy_get_strings,
> +	.get_stats	= kszphy_get_stats,
> +	.suspend	= genphy_suspend,
> +	.resume		= genphy_resume,
>  }, {
>  	.phy_id		= PHY_ID_KSZ9131,
>  	.phy_id_mask	= MICREL_PHY_ID_MASK,
>  	.name		= "Microchip KSZ9131 Gigabit PHY",
>  	/* PHY_GBIT_FEATURES */
>  	.flags		= PHY_POLL_CABLE_TEST,
> -	.driver_data	= &ksz9021_type,
> +	.driver_data	= &ksz9131_type,
>  	.probe		= kszphy_probe,
>  	.config_init	= ksz9131_config_init,
>  	.config_intr	= kszphy_config_intr,
> @@ -3446,6 +3654,7 @@ static struct mdio_device_id __maybe_unused micrel_tbl[] = {
>  	{ PHY_ID_KSZ886X, MICREL_PHY_ID_MASK },
>  	{ PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
>  	{ PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
> +	{ PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
>  	{ }
>  };
>  
> diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
> index 771e050883db7..8bef1ab62bba3 100644
> --- a/include/linux/micrel_phy.h
> +++ b/include/linux/micrel_phy.h
> @@ -31,6 +31,7 @@
>  #define PHY_ID_KSZ9131		0x00221640
>  #define PHY_ID_LAN8814		0x00221660
>  #define PHY_ID_LAN8804		0x00221670
> +#define PHY_ID_LAN8841		0x00221650
>  
>  #define PHY_ID_KSZ886X		0x00221430
>  #define PHY_ID_KSZ8863		0x00221435

Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY

[Andrew Lunn]

> +{
> +	char *rx_data_skews[4] = {"rxd0-skew-psec", "rxd1-skew-psec",
> +				  "rxd2-skew-psec", "rxd3-skew-psec"};
> +	char *tx_data_skews[4] = {"txd0-skew-psec", "txd1-skew-psec",
> +				  "txd2-skew-psec", "txd3-skew-psec"};

Please take a read of
Documentation/devicetree/bindings/net/micrel-ksz90x1.txt and then add
a section which describes what these properties mean for this PHY,
since nearly every microchip PHY has a different meaning :-(

      Andrew

Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY

[Horatiu Vultur]

The 02/03/2023 14:55, Heiner Kallweit wrote:

Hi Heiner,

> 
> On 03.02.2023 13:25, Horatiu Vultur wrote:

...

> > +
> > +#define LAN8841_OUTPUT_CTRL                  25
> > +#define LAN8841_OUTPUT_CTRL_INT_BUFFER               BIT(14)
> > +#define LAN8841_CTRL                         31
> > +#define LAN8841_CTRL_INTR_POLARITY           BIT(14)
> > +static int lan8841_config_intr(struct phy_device *phydev)
> > +{
> > +     struct irq_data *irq_data;
> > +     int temp = 0;
> > +
> > +     irq_data = irq_get_irq_data(phydev->irq);
> > +     if (!irq_data)
> > +             return 0;
> > +
> > +     if (irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_HIGH) {
> > +             /* Change polarity of the interrupt */
> 
> Why this a little bit esoteric logic? Can't you set the interrupt
> to level-low in the chip (like most other ones), and then define
> the polarity the usual way e.g. in DT?

To set the interrupt to level-low it needs to be set to open-drain and
in that case I can't use the polarity register, because doesn't have any
effect on the interrupt. So I can't set the interrupt to level low and
then use the polarity to select if it is high or low.
That is the reason why I have these checks.

> 
> > +             phy_modify(phydev, LAN8841_OUTPUT_CTRL,
> > +                        LAN8841_OUTPUT_CTRL_INT_BUFFER,
> > +                        LAN8841_OUTPUT_CTRL_INT_BUFFER);
> > +             phy_modify(phydev, LAN8841_CTRL,
> > +                        LAN8841_CTRL_INTR_POLARITY,
> > +                        LAN8841_CTRL_INTR_POLARITY);
> > +     } else {
> > +             /* It is enough to set INT buffer to open-drain because then
> > +              * the interrupt will be active low.
> > +              */
> > +             phy_modify(phydev, LAN8841_OUTPUT_CTRL,
> > +                        LAN8841_OUTPUT_CTRL_INT_BUFFER, 0);
> > +     }
> > +
> > +     /* enable / disable interrupts */
> > +     if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> > +             temp = LAN8814_INT_LINK;
> > +
> > +     return phy_write(phydev, LAN8814_INTC, temp);
> > +}
> > +
> > +static irqreturn_t lan8841_handle_interrupt(struct phy_device *phydev)
> > +{
> > +     int irq_status;
> > +
> > +     irq_status = phy_read(phydev, LAN8814_INTS);
> > +     if (irq_status < 0) {
> > +             phy_error(phydev);
> > +             return IRQ_NONE;
> > +     }
> > +
> > +     if (irq_status & LAN8814_INT_LINK) {
> > +             phy_trigger_machine(phydev);
> > +             return IRQ_HANDLED;
> > +     }
> > +
> > +     return IRQ_NONE;
> > +}
> > +

-- 
/Horatiu

Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY

[Horatiu Vultur]

The 02/03/2023 15:22, Andrew Lunn wrote:

Hi Andrew,

> 
> > +{
> > +     char *rx_data_skews[4] = {"rxd0-skew-psec", "rxd1-skew-psec",
> > +                               "rxd2-skew-psec", "rxd3-skew-psec"};
> > +     char *tx_data_skews[4] = {"txd0-skew-psec", "txd1-skew-psec",
> > +                               "txd2-skew-psec", "txd3-skew-psec"};
> 
> Please take a read of
> Documentation/devicetree/bindings/net/micrel-ksz90x1.txt and then add
> a section which describes what these properties mean for this PHY,
> since nearly every microchip PHY has a different meaning :-(

I had a closer look at the datasheet of this PHY, and these properties
for lan8841 are the same for ksz9131, so actually I can reuse the
function 'ksz9131_config_init', to remove some of the duplicated code.

In this case maybe is enough to add the following change in
'micrel-ksz90x1.txt' not to create a totally new section.

---
diff --git a/Documentation/devicetree/bindings/net/micrel-ksz90x1.txt b/Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
index df9e844dd6bc6..2681168777a1e 100644
--- a/Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
+++ b/Documentation/devicetree/bindings/net/micrel-ksz90x1.txt
@@ -158,6 +158,7 @@ KSZ9031:
         no link will be established.

 KSZ9131:
+LAN8841:

   All skew control options are specified in picoseconds. The increment
   step is 100ps. Unlike KSZ9031, the values represent picoseccond delays.

---

> 
>       Andrew

-- 
/Horatiu

Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY

[Andrew Lunn]

On Fri, Feb 03, 2023 at 04:25:48PM +0100, Horatiu Vultur wrote:
> The 02/03/2023 15:22, Andrew Lunn wrote:
> 
> Hi Andrew,
> 
> > 
> > > +{
> > > +     char *rx_data_skews[4] = {"rxd0-skew-psec", "rxd1-skew-psec",
> > > +                               "rxd2-skew-psec", "rxd3-skew-psec"};
> > > +     char *tx_data_skews[4] = {"txd0-skew-psec", "txd1-skew-psec",
> > > +                               "txd2-skew-psec", "txd3-skew-psec"};
> > 
> > Please take a read of
> > Documentation/devicetree/bindings/net/micrel-ksz90x1.txt and then add
> > a section which describes what these properties mean for this PHY,
> > since nearly every microchip PHY has a different meaning :-(
> 
> I had a closer look at the datasheet of this PHY, and these properties
> for lan8841 are the same for ksz9131, so actually I can reuse the
> function 'ksz9131_config_init', to remove some of the duplicated code.

Great.

> In this case maybe is enough to add the following change in
> 'micrel-ksz90x1.txt' not to create a totally new section.
> 
>  KSZ9131:
> +LAN8841:

Yes, that is good, and KSZ9131 is about the only one which actually
gets this right, so it is a good you can reuse it.

     Andrew

Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY

[Heiner Kallweit]

On 03.02.2023 16:10, Horatiu Vultur wrote:
> The 02/03/2023 14:55, Heiner Kallweit wrote:
> 
> Hi Heiner,
> 
>>
>> On 03.02.2023 13:25, Horatiu Vultur wrote:
> 
> ...
> 
>>> +
>>> +#define LAN8841_OUTPUT_CTRL                  25
>>> +#define LAN8841_OUTPUT_CTRL_INT_BUFFER               BIT(14)
>>> +#define LAN8841_CTRL                         31
>>> +#define LAN8841_CTRL_INTR_POLARITY           BIT(14)
>>> +static int lan8841_config_intr(struct phy_device *phydev)
>>> +{
>>> +     struct irq_data *irq_data;
>>> +     int temp = 0;
>>> +
>>> +     irq_data = irq_get_irq_data(phydev->irq);
>>> +     if (!irq_data)
>>> +             return 0;
>>> +
>>> +     if (irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_HIGH) {
>>> +             /* Change polarity of the interrupt */
>>
>> Why this a little bit esoteric logic? Can't you set the interrupt
>> to level-low in the chip (like most other ones), and then define
>> the polarity the usual way e.g. in DT?
> 
> To set the interrupt to level-low it needs to be set to open-drain and
> in that case I can't use the polarity register, because doesn't have any
> effect on the interrupt. So I can't set the interrupt to level low and
> then use the polarity to select if it is high or low.
> That is the reason why I have these checks.
> 
To me this still doesn't look right. After checking the datasheet I'd say:
At first open-drain should be preferred because only in this mode the
interrupt line can be shared.
And if you use level-low and open-drain, why would you want to fiddle
with the polarity? Level-low and open-drain is the only mode supported by
most PHY's and it's totally fine. Or do you have a special use case where
you want to connect the interrupt pin to an interrupt controller that
only supports level-high and has no programmable inverter in its path?

>>
>>> +             phy_modify(phydev, LAN8841_OUTPUT_CTRL,
>>> +                        LAN8841_OUTPUT_CTRL_INT_BUFFER,
>>> +                        LAN8841_OUTPUT_CTRL_INT_BUFFER);
>>> +             phy_modify(phydev, LAN8841_CTRL,
>>> +                        LAN8841_CTRL_INTR_POLARITY,
>>> +                        LAN8841_CTRL_INTR_POLARITY);
>>> +     } else {
>>> +             /* It is enough to set INT buffer to open-drain because then
>>> +              * the interrupt will be active low.
>>> +              */
>>> +             phy_modify(phydev, LAN8841_OUTPUT_CTRL,
>>> +                        LAN8841_OUTPUT_CTRL_INT_BUFFER, 0);
>>> +     }
>>> +
>>> +     /* enable / disable interrupts */
>>> +     if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
>>> +             temp = LAN8814_INT_LINK;
>>> +
>>> +     return phy_write(phydev, LAN8814_INTC, temp);
>>> +}
>>> +
>>> +static irqreturn_t lan8841_handle_interrupt(struct phy_device *phydev)
>>> +{
>>> +     int irq_status;
>>> +
>>> +     irq_status = phy_read(phydev, LAN8814_INTS);
>>> +     if (irq_status < 0) {
>>> +             phy_error(phydev);
>>> +             return IRQ_NONE;
>>> +     }
>>> +
>>> +     if (irq_status & LAN8814_INT_LINK) {
>>> +             phy_trigger_machine(phydev);
>>> +             return IRQ_HANDLED;
>>> +     }
>>> +
>>> +     return IRQ_NONE;
>>> +}
>>> +
> 

Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY

[Horatiu Vultur]

The 02/03/2023 22:57, Heiner Kallweit wrote:
> 
> On 03.02.2023 16:10, Horatiu Vultur wrote:
> > The 02/03/2023 14:55, Heiner Kallweit wrote:
> >
> > Hi Heiner,
> >
> >>
> >> On 03.02.2023 13:25, Horatiu Vultur wrote:
> >
> > ...
> >
> >>> +
> >>> +#define LAN8841_OUTPUT_CTRL                  25
> >>> +#define LAN8841_OUTPUT_CTRL_INT_BUFFER               BIT(14)
> >>> +#define LAN8841_CTRL                         31
> >>> +#define LAN8841_CTRL_INTR_POLARITY           BIT(14)
> >>> +static int lan8841_config_intr(struct phy_device *phydev)
> >>> +{
> >>> +     struct irq_data *irq_data;
> >>> +     int temp = 0;
> >>> +
> >>> +     irq_data = irq_get_irq_data(phydev->irq);
> >>> +     if (!irq_data)
> >>> +             return 0;
> >>> +
> >>> +     if (irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_HIGH) {
> >>> +             /* Change polarity of the interrupt */
> >>
> >> Why this a little bit esoteric logic? Can't you set the interrupt
> >> to level-low in the chip (like most other ones), and then define
> >> the polarity the usual way e.g. in DT?
> >
> > To set the interrupt to level-low it needs to be set to open-drain and
> > in that case I can't use the polarity register, because doesn't have any
> > effect on the interrupt. So I can't set the interrupt to level low and
> > then use the polarity to select if it is high or low.
> > That is the reason why I have these checks.
> >
> To me this still doesn't look right. After checking the datasheet I'd say:
> At first open-drain should be preferred because only in this mode the
> interrupt line can be shared.

Agree.

> And if you use level-low and open-drain, why would you want to fiddle
> with the polarity?

In this case, I don't fiddle with the polarity. That case is on the else
branch of this if condition. I play with the polarity only when using
push-pull.

> Level-low and open-drain is the only mode supported by
> most PHY's and it's totally fine.
>
> Or do you have a special use case where
> you want to connect the interrupt pin to an interrupt controller that
> only supports level-high and has no programmable inverter in its path?

I have two cases:
1. When lan966x is connected to this lan8841. In this case the interrupt
controller supports both level-low and level-high. But in this case I
can test only the level-low.

2. When lan7431 is connected to this lan8841 and using x86. If I
remember correctly (I don't have the setup to test it anymore and will
take a some time to get it again) this worked only with level-high
interrupts. To get this working I had some changes in the lan7431 driver
to enable interrupts from the external PHY.

Maybe a better approach would be for now, just to set the interrupt to
open-drain in the lan8841. And only when I add the changes to lan7431
also add the changes to lan8841 to support level-high interrupts if it
is still needed.

> 
> >>
> >>> +             phy_modify(phydev, LAN8841_OUTPUT_CTRL,
> >>> +                        LAN8841_OUTPUT_CTRL_INT_BUFFER,
> >>> +                        LAN8841_OUTPUT_CTRL_INT_BUFFER);
> >>> +             phy_modify(phydev, LAN8841_CTRL,
> >>> +                        LAN8841_CTRL_INTR_POLARITY,
> >>> +                        LAN8841_CTRL_INTR_POLARITY);
> >>> +     } else {
> >>> +             /* It is enough to set INT buffer to open-drain because then
> >>> +              * the interrupt will be active low.
> >>> +              */
> >>> +             phy_modify(phydev, LAN8841_OUTPUT_CTRL,
> >>> +                        LAN8841_OUTPUT_CTRL_INT_BUFFER, 0);
> >>> +     }
> >>> +
> >>> +     /* enable / disable interrupts */
> >>> +     if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> >>> +             temp = LAN8814_INT_LINK;
> >>> +
> >>> +     return phy_write(phydev, LAN8814_INTC, temp);
> >>> +}
> >>> +
> >>> +static irqreturn_t lan8841_handle_interrupt(struct phy_device *phydev)
> >>> +{
> >>> +     int irq_status;
> >>> +
> >>> +     irq_status = phy_read(phydev, LAN8814_INTS);
> >>> +     if (irq_status < 0) {
> >>> +             phy_error(phydev);
> >>> +             return IRQ_NONE;
> >>> +     }
> >>> +
> >>> +     if (irq_status & LAN8814_INT_LINK) {
> >>> +             phy_trigger_machine(phydev);
> >>> +             return IRQ_HANDLED;
> >>> +     }
> >>> +
> >>> +     return IRQ_NONE;
> >>> +}
> >>> +
> >
> 

-- 
/Horatiu

Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY

[Heiner Kallweit]

On 04.02.2023 11:12, Horatiu Vultur wrote:
> The 02/03/2023 22:57, Heiner Kallweit wrote:
>>
>> On 03.02.2023 16:10, Horatiu Vultur wrote:
>>> The 02/03/2023 14:55, Heiner Kallweit wrote:
>>>
>>> Hi Heiner,
>>>
>>>>
>>>> On 03.02.2023 13:25, Horatiu Vultur wrote:
>>>
>>> ...
>>>
>>>>> +
>>>>> +#define LAN8841_OUTPUT_CTRL                  25
>>>>> +#define LAN8841_OUTPUT_CTRL_INT_BUFFER               BIT(14)
>>>>> +#define LAN8841_CTRL                         31
>>>>> +#define LAN8841_CTRL_INTR_POLARITY           BIT(14)
>>>>> +static int lan8841_config_intr(struct phy_device *phydev)
>>>>> +{
>>>>> +     struct irq_data *irq_data;
>>>>> +     int temp = 0;
>>>>> +
>>>>> +     irq_data = irq_get_irq_data(phydev->irq);
>>>>> +     if (!irq_data)
>>>>> +             return 0;
>>>>> +
>>>>> +     if (irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_HIGH) {
>>>>> +             /* Change polarity of the interrupt */
>>>>
>>>> Why this a little bit esoteric logic? Can't you set the interrupt
>>>> to level-low in the chip (like most other ones), and then define
>>>> the polarity the usual way e.g. in DT?
>>>
>>> To set the interrupt to level-low it needs to be set to open-drain and
>>> in that case I can't use the polarity register, because doesn't have any
>>> effect on the interrupt. So I can't set the interrupt to level low and
>>> then use the polarity to select if it is high or low.
>>> That is the reason why I have these checks.
>>>
>> To me this still doesn't look right. After checking the datasheet I'd say:
>> At first open-drain should be preferred because only in this mode the
>> interrupt line can be shared.
> 
> Agree.
> 
>> And if you use level-low and open-drain, why would you want to fiddle
>> with the polarity?
> 
> In this case, I don't fiddle with the polarity. That case is on the else
> branch of this if condition. I play with the polarity only when using
> push-pull.
> 
>> Level-low and open-drain is the only mode supported by
>> most PHY's and it's totally fine.
>>
>> Or do you have a special use case where
>> you want to connect the interrupt pin to an interrupt controller that
>> only supports level-high and has no programmable inverter in its path?
> 
> I have two cases:
> 1. When lan966x is connected to this lan8841. In this case the interrupt
> controller supports both level-low and level-high. But in this case I
> can test only the level-low.
> 
> 2. When lan7431 is connected to this lan8841 and using x86. If I
> remember correctly (I don't have the setup to test it anymore and will
> take a some time to get it again) this worked only with level-high
> interrupts. To get this working I had some changes in the lan7431 driver
> to enable interrupts from the external PHY.
> 
Looking at the datasheet for LAN7431 (document AN2948, page 76) in register
GPIO_WAKE you can configure the polarity for the PHY interrupt (to be exact,
for all GPIO-triggered interrupts).
Therefore level-low should be fine also with LAN7431.

GPIO Polarity 0-11 (GPIOPOL[11:0])
When clear, the pin functions as a GPIO.
0 = Wakeup/interrupt is triggered when GPIO is driven low.
1 = Wakeup/interrupt is triggered when GPIO is driven high

> Maybe a better approach would be for now, just to set the interrupt to
> open-drain in the lan8841. And only when I add the changes to lan7431
> also add the changes to lan8841 to support level-high interrupts if it
> is still needed.
> 
Agree.

>>
>>>>
>>>>> +             phy_modify(phydev, LAN8841_OUTPUT_CTRL,
>>>>> +                        LAN8841_OUTPUT_CTRL_INT_BUFFER,
>>>>> +                        LAN8841_OUTPUT_CTRL_INT_BUFFER);
>>>>> +             phy_modify(phydev, LAN8841_CTRL,
>>>>> +                        LAN8841_CTRL_INTR_POLARITY,
>>>>> +                        LAN8841_CTRL_INTR_POLARITY);
>>>>> +     } else {
>>>>> +             /* It is enough to set INT buffer to open-drain because then
>>>>> +              * the interrupt will be active low.
>>>>> +              */
>>>>> +             phy_modify(phydev, LAN8841_OUTPUT_CTRL,
>>>>> +                        LAN8841_OUTPUT_CTRL_INT_BUFFER, 0);
>>>>> +     }
>>>>> +
>>>>> +     /* enable / disable interrupts */
>>>>> +     if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
>>>>> +             temp = LAN8814_INT_LINK;
>>>>> +
>>>>> +     return phy_write(phydev, LAN8814_INTC, temp);
>>>>> +}
>>>>> +
>>>>> +static irqreturn_t lan8841_handle_interrupt(struct phy_device *phydev)
>>>>> +{
>>>>> +     int irq_status;
>>>>> +
>>>>> +     irq_status = phy_read(phydev, LAN8814_INTS);
>>>>> +     if (irq_status < 0) {
>>>>> +             phy_error(phydev);
>>>>> +             return IRQ_NONE;
>>>>> +     }
>>>>> +
>>>>> +     if (irq_status & LAN8814_INT_LINK) {
>>>>> +             phy_trigger_machine(phydev);
>>>>> +             return IRQ_HANDLED;
>>>>> +     }
>>>>> +
>>>>> +     return IRQ_NONE;
>>>>> +}
>>>>> +
>>>
>>
> 

Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY

[Russell King (Oracle)]

On Fri, Feb 03, 2023 at 01:25:42PM +0100, Horatiu Vultur wrote:
> +hw_init:
> +	/* 100BT Clause 40 improvenent errata */
> +	phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> +		      LAN8841_ANALOG_CONTROL_1,
> +		      LAN8841_ANALOG_CONTROL_1_PLL_TRIM(0x2));
> +	phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> +		      LAN8841_ANALOG_CONTROL_10,
> +		      LAN8841_ANALOG_CONTROL_10_PLL_DIV(0x1));
> +
> +	/* 10M/100M Ethernet Signal Tuning Errata for Shorted-Center Tap
> +	 * Magnetics
> +	 */
> +	ret = phy_read_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
> +			   LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG);

Error handling? If this returns a negative error code, then the if()
statement likely becomes true... although the writes below may also
error out.

> +	if (ret & LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG_MAGJACK) {
> +		phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> +			      LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT,
> +			      LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT_VAL);
> +		phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> +			      LAN8841_BTRX_POWER_DOWN,
> +			      LAN8841_BTRX_POWER_DOWN_QBIAS_CH_A |
> +			      LAN8841_BTRX_POWER_DOWN_BTRX_CH_A |
> +			      LAN8841_BTRX_POWER_DOWN_QBIAS_CH_B |
> +			      LAN8841_BTRX_POWER_DOWN_BTRX_CH_B |
> +			      LAN8841_BTRX_POWER_DOWN_BTRX_CH_C |
> +			      LAN8841_BTRX_POWER_DOWN_BTRX_CH_D);
> +	}
> +
> +	/* LDO Adjustment errata */
> +	phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> +		      LAN8841_ANALOG_CONTROL_11,
> +		      LAN8841_ANALOG_CONTROL_11_LDO_REF(1));
> +
> +	/* 100BT RGMII latency tuning errata */
> +	phy_write_mmd(phydev, MDIO_MMD_PMAPMD,
> +		      LAN8841_ADC_CHANNEL_MASK, 0x0);
> +	phy_write_mmd(phydev, LAN8841_MMD_TIMER_REG,
> +		      LAN8841_MMD0_REGISTER_17,
> +		      LAN8841_MMD0_REGISTER_17_DROP_OPT(2) |
> +		      LAN8841_MMD0_REGISTER_17_XMIT_TOG_TX_DIS);
> +
> +	return 0;

This function is always succesful, even if the writes fail?

> +}
> +
> +#define LAN8841_OUTPUT_CTRL			25
> +#define LAN8841_OUTPUT_CTRL_INT_BUFFER		BIT(14)
> +#define LAN8841_CTRL				31
> +#define LAN8841_CTRL_INTR_POLARITY		BIT(14)
> +static int lan8841_config_intr(struct phy_device *phydev)
> +{
> +	struct irq_data *irq_data;
> +	int temp = 0;
> +
> +	irq_data = irq_get_irq_data(phydev->irq);
> +	if (!irq_data)
> +		return 0;
> +
> +	if (irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_HIGH) {
> +		/* Change polarity of the interrupt */
> +		phy_modify(phydev, LAN8841_OUTPUT_CTRL,
> +			   LAN8841_OUTPUT_CTRL_INT_BUFFER,
> +			   LAN8841_OUTPUT_CTRL_INT_BUFFER);
> +		phy_modify(phydev, LAN8841_CTRL,
> +			   LAN8841_CTRL_INTR_POLARITY,
> +			   LAN8841_CTRL_INTR_POLARITY);
> +	} else {
> +		/* It is enough to set INT buffer to open-drain because then
> +		 * the interrupt will be active low.
> +		 */
> +		phy_modify(phydev, LAN8841_OUTPUT_CTRL,
> +			   LAN8841_OUTPUT_CTRL_INT_BUFFER, 0);
> +	}
> +
> +	/* enable / disable interrupts */
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> +		temp = LAN8814_INT_LINK;
> +
> +	return phy_write(phydev, LAN8814_INTC, temp);
> +}
> +
> +static irqreturn_t lan8841_handle_interrupt(struct phy_device *phydev)
> +{
> +	int irq_status;
> +
> +	irq_status = phy_read(phydev, LAN8814_INTS);
> +	if (irq_status < 0) {
> +		phy_error(phydev);
> +		return IRQ_NONE;
> +	}
> +
> +	if (irq_status & LAN8814_INT_LINK) {
> +		phy_trigger_machine(phydev);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +#define LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER 3
> +#define LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER_STRAP_RGMII_EN BIT(0)
> +static int lan8841_probe(struct phy_device *phydev)
> +{
> +	int err;
> +
> +	err = kszphy_probe(phydev);
> +	if (err)
> +		return err;
> +
> +	if (phy_read_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
> +			 LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER) &
> +	    LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER_STRAP_RGMII_EN)
> +		phydev->interface = PHY_INTERFACE_MODE_RGMII_RXID;

I'm not entirely sure what this code is trying to do here, as many
drivers just pass into phy_attach_direct() the interface mode that
was configured in firmware or by the ethernet driver's platform
data, and that will override phydev->interface.

There are a few corner cases in DSA where we do make use of the
phy's ->interface as set at probe() time but that's rather
exceptional.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY

[Horatiu Vultur]

The 02/09/2023 16:37, Russell King (Oracle) wrote:

Hi Russell,

Thanks for the review. The latest version of this patch series (v4) was
already accepted. But your comments will still apply.

> 
> On Fri, Feb 03, 2023 at 01:25:42PM +0100, Horatiu Vultur wrote:
> > +hw_init:
> > +     /* 100BT Clause 40 improvenent errata */
> > +     phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> > +                   LAN8841_ANALOG_CONTROL_1,
> > +                   LAN8841_ANALOG_CONTROL_1_PLL_TRIM(0x2));
> > +     phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> > +                   LAN8841_ANALOG_CONTROL_10,
> > +                   LAN8841_ANALOG_CONTROL_10_PLL_DIV(0x1));
> > +
> > +     /* 10M/100M Ethernet Signal Tuning Errata for Shorted-Center Tap
> > +      * Magnetics
> > +      */
> > +     ret = phy_read_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
> > +                        LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG);
> 
> Error handling? If this returns a negative error code, then the if()
> statement likely becomes true... although the writes below may also
> error out.

Yes, I missed that. I will add that.

> 
> > +     if (ret & LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG_MAGJACK) {
> > +             phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> > +                           LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT,
> > +                           LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT_VAL);
> > +             phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> > +                           LAN8841_BTRX_POWER_DOWN,
> > +                           LAN8841_BTRX_POWER_DOWN_QBIAS_CH_A |
> > +                           LAN8841_BTRX_POWER_DOWN_BTRX_CH_A |
> > +                           LAN8841_BTRX_POWER_DOWN_QBIAS_CH_B |
> > +                           LAN8841_BTRX_POWER_DOWN_BTRX_CH_B |
> > +                           LAN8841_BTRX_POWER_DOWN_BTRX_CH_C |
> > +                           LAN8841_BTRX_POWER_DOWN_BTRX_CH_D);
> > +     }
> > +
> > +     /* LDO Adjustment errata */
> > +     phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> > +                   LAN8841_ANALOG_CONTROL_11,
> > +                   LAN8841_ANALOG_CONTROL_11_LDO_REF(1));
> > +
> > +     /* 100BT RGMII latency tuning errata */
> > +     phy_write_mmd(phydev, MDIO_MMD_PMAPMD,
> > +                   LAN8841_ADC_CHANNEL_MASK, 0x0);
> > +     phy_write_mmd(phydev, LAN8841_MMD_TIMER_REG,
> > +                   LAN8841_MMD0_REGISTER_17,
> > +                   LAN8841_MMD0_REGISTER_17_DROP_OPT(2) |
> > +                   LAN8841_MMD0_REGISTER_17_XMIT_TOG_TX_DIS);
> > +
> > +     return 0;
> 
> This function is always succesful, even if the writes fail?

What is the rule of thumb here? Do we need to check the return value of
the writes and reads? Because I can see in the micrel.c this is not
really the case.
> 
> > +}
> > +
> > +#define LAN8841_OUTPUT_CTRL                  25
> > +#define LAN8841_OUTPUT_CTRL_INT_BUFFER               BIT(14)
> > +#define LAN8841_CTRL                         31
> > +#define LAN8841_CTRL_INTR_POLARITY           BIT(14)
> > +static int lan8841_config_intr(struct phy_device *phydev)
> > +{
> > +     struct irq_data *irq_data;
> > +     int temp = 0;
> > +
> > +     irq_data = irq_get_irq_data(phydev->irq);
> > +     if (!irq_data)
> > +             return 0;
> > +
> > +     if (irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_HIGH) {
> > +             /* Change polarity of the interrupt */
> > +             phy_modify(phydev, LAN8841_OUTPUT_CTRL,
> > +                        LAN8841_OUTPUT_CTRL_INT_BUFFER,
> > +                        LAN8841_OUTPUT_CTRL_INT_BUFFER);
> > +             phy_modify(phydev, LAN8841_CTRL,
> > +                        LAN8841_CTRL_INTR_POLARITY,
> > +                        LAN8841_CTRL_INTR_POLARITY);
> > +     } else {
> > +             /* It is enough to set INT buffer to open-drain because then
> > +              * the interrupt will be active low.
> > +              */
> > +             phy_modify(phydev, LAN8841_OUTPUT_CTRL,
> > +                        LAN8841_OUTPUT_CTRL_INT_BUFFER, 0);
> > +     }
> > +
> > +     /* enable / disable interrupts */
> > +     if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> > +             temp = LAN8814_INT_LINK;
> > +
> > +     return phy_write(phydev, LAN8814_INTC, temp);
> > +}
> > +
> > +static irqreturn_t lan8841_handle_interrupt(struct phy_device *phydev)
> > +{
> > +     int irq_status;
> > +
> > +     irq_status = phy_read(phydev, LAN8814_INTS);
> > +     if (irq_status < 0) {
> > +             phy_error(phydev);
> > +             return IRQ_NONE;
> > +     }
> > +
> > +     if (irq_status & LAN8814_INT_LINK) {
> > +             phy_trigger_machine(phydev);
> > +             return IRQ_HANDLED;
> > +     }
> > +
> > +     return IRQ_NONE;
> > +}
> > +
> > +#define LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER 3
> > +#define LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER_STRAP_RGMII_EN BIT(0)
> > +static int lan8841_probe(struct phy_device *phydev)
> > +{
> > +     int err;
> > +
> > +     err = kszphy_probe(phydev);
> > +     if (err)
> > +             return err;
> > +
> > +     if (phy_read_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
> > +                      LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER) &
> > +         LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER_STRAP_RGMII_EN)
> > +             phydev->interface = PHY_INTERFACE_MODE_RGMII_RXID;
> 
> I'm not entirely sure what this code is trying to do here, as many
> drivers just pass into phy_attach_direct() the interface mode that
> was configured in firmware or by the ethernet driver's platform
> data, and that will override phydev->interface.

This was something when the lan8841 is connected with lan743x. The
changes to lan743x were not upstream. In that case actually it was
passing phy's->interface to the phy_attach_direct. So the interface was
not overridden.
Like I suggested to Heiner, maybe I should drop this and add back this
only when adding the changes to lan743x if it is still needed.

> 
> There are a few corner cases in DSA where we do make use of the
> phy's ->interface as set at probe() time but that's rather
> exceptional.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

-- 
/Horatiu