| Message ID | 20230203122542.436305-1-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: micrel: Add support for lan8841 PHY | expand |
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
> +{ > + 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
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; > > +} > > +
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
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
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; >>> +} >>> + >
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; > >>> +} > >>> + > > >
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; >>>>> +} >>>>> + >>> >> >
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.
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!
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
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(-)