| Message ID | 20220207174532.362781-2-enguerrand.de-ribaucourt@savoirfairelinux.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: micrel: add Microchip KSZ 9897 Switch PHY | expand |
> + /* KSZ8081A3/KSZ8091R1 PHY and KSZ9897 switch share the same > + * exact PHY ID. However, they can be told apart by the default value > + * of the LED mode. It is 0 for the PHY, and 1 for the switch. > + */ > + ret &= (MICREL_KSZ8081_CTRL2_LED_MODE0 | MICREL_KSZ8081_CTRL2_LED_MODE1); > + if (!ksz_8081) > + return ret; > + else > + return !ret; What exactly does MICREL_KSZ8081_CTRL2_LED_MODE0 and MICREL_KSZ8081_CTRL2_LED_MODE1 mean? We have to be careful in that there could be use cases which actually wants to configure the LEDs. There have been recent discussions for two other PHYs recently where the bootloader is configuring the LEDs, to something other than their default value. Andrew
----- Original Message ----- > From: "Andrew Lunn" <andrew@lunn.ch> > To: "Enguerrand de Ribaucourt" <enguerrand.de-ribaucourt@savoirfairelinux.com> > Cc: "netdev" <netdev@vger.kernel.org>, "hkallweit1" <hkallweit1@gmail.com>, "linux" <linux@armlinux.org.uk> > Sent: Tuesday, February 8, 2022 12:28:53 AM > Subject: Re: [PATCH v2 1/2] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support > > + /* KSZ8081A3/KSZ8091R1 PHY and KSZ9897 switch share the same > > + * exact PHY ID. However, they can be told apart by the default value > > + * of the LED mode. It is 0 for the PHY, and 1 for the switch. > > + */ > > + ret &= (MICREL_KSZ8081_CTRL2_LED_MODE0 | MICREL_KSZ8081_CTRL2_LED_MODE1); > > + if (!ksz_8081) > > + return ret; > > + else > > + return !ret; > What exactly does MICREL_KSZ8081_CTRL2_LED_MODE0 and > MICREL_KSZ8081_CTRL2_LED_MODE1 mean? We have to be careful in that > there could be use cases which actually wants to configure the > LEDs. There have been recent discussions for two other PHYs recently > where the bootloader is configuring the LEDs, to something other than > their default value. Those registers configure the LED Mode according to the KSZ8081 datasheet: [00] = LED1: Speed LED0: Link/Activity [01] = LED1: Activity LED0: Link [10], [11] = Reserved default value is [00]. Indeed, if the bootloader changes them, we would match the wrong device. However, I closely examined all the registers, and there is no read-only bit that we can use to differentiate both models. The LED mode bits are the only ones that have a different default value on the KSZ8081: [00] and the KSZ9897: [01]. Also, the RMII registers are not documented in the KSZ9897 datasheet so that value is not guaranteed to be [01] even though that's what I observed. Do you think we should find another way to match KSZ8081 and KSZ9897? The good news is that I'm now confident about the phy_id emitted by both models. Thanks for your help. > Andrew
On Tue, Feb 08, 2022 at 03:38:41AM -0500, Enguerrand de Ribaucourt wrote: > ----- Original Message ----- > > From: "Andrew Lunn" <andrew@lunn.ch> > > To: "Enguerrand de Ribaucourt" <enguerrand.de-ribaucourt@savoirfairelinux.com> > > Cc: "netdev" <netdev@vger.kernel.org>, "hkallweit1" <hkallweit1@gmail.com>, "linux" <linux@armlinux.org.uk> > > Sent: Tuesday, February 8, 2022 12:28:53 AM > > Subject: Re: [PATCH v2 1/2] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support > > > > + /* KSZ8081A3/KSZ8091R1 PHY and KSZ9897 switch share the same > > > + * exact PHY ID. However, they can be told apart by the default value > > > + * of the LED mode. It is 0 for the PHY, and 1 for the switch. > > > + */ > > > + ret &= (MICREL_KSZ8081_CTRL2_LED_MODE0 | MICREL_KSZ8081_CTRL2_LED_MODE1); > > > + if (!ksz_8081) > > > + return ret; > > > + else > > > + return !ret; > > > What exactly does MICREL_KSZ8081_CTRL2_LED_MODE0 and > > MICREL_KSZ8081_CTRL2_LED_MODE1 mean? We have to be careful in that > > there could be use cases which actually wants to configure the > > LEDs. There have been recent discussions for two other PHYs recently > > where the bootloader is configuring the LEDs, to something other than > > their default value. > > Those registers configure the LED Mode according to the KSZ8081 datasheet: > [00] = LED1: Speed LED0: Link/Activity > [01] = LED1: Activity LED0: Link > [10], [11] = Reserved > default value is [00]. > > Indeed, if the bootloader changes them, we would match the wrong > device. However, I closely examined all the registers, and there is no > read-only bit that we can use to differentiate both models. The > LED mode bits are the only ones that have a different default value on the > KSZ8081: [00] and the KSZ9897: [01]. Also, the RMII registers are not > documented in the KSZ9897 datasheet so that value is not guaranteed to > be [01] even though that's what I observed. > > Do you think we should find another way to match KSZ8081 and KSZ9897? > The good news is that I'm now confident about the phy_id emitted by > both models. Lets try asking Prasanna Vengateshan, who is working on other Microchip switches and PHYs at Microchip. Andrew
On Tue, 2022-02-08 at 14:13 +0100, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Tue, Feb 08, 2022 at 03:38:41AM -0500, Enguerrand de Ribaucourt wrote: > > ----- Original Message ----- > > > From: "Andrew Lunn" <andrew@lunn.ch> > > > To: "Enguerrand de Ribaucourt" < > > > enguerrand.de-ribaucourt@savoirfairelinux.com> > > > Cc: "netdev" <netdev@vger.kernel.org>, "hkallweit1" > > > <hkallweit1@gmail.com>, "linux" <linux@armlinux.org.uk> > > > Sent: Tuesday, February 8, 2022 12:28:53 AM > > > Subject: Re: [PATCH v2 1/2] net: phy: micrel: add Microchip KSZ 9897 > > > Switch PHY support > > > > > > + /* KSZ8081A3/KSZ8091R1 PHY and KSZ9897 switch share the same > > > > + * exact PHY ID. However, they can be told apart by the default value > > > > + * of the LED mode. It is 0 for the PHY, and 1 for the switch. > > > > + */ > > > > + ret &= (MICREL_KSZ8081_CTRL2_LED_MODE0 | > > > > MICREL_KSZ8081_CTRL2_LED_MODE1); > > > > + if (!ksz_8081) > > > > + return ret; > > > > + else > > > > + return !ret; > > > > > What exactly does MICREL_KSZ8081_CTRL2_LED_MODE0 and > > > MICREL_KSZ8081_CTRL2_LED_MODE1 mean? We have to be careful in that > > > there could be use cases which actually wants to configure the > > > LEDs. There have been recent discussions for two other PHYs recently > > > where the bootloader is configuring the LEDs, to something other than > > > their default value. > > > > Those registers configure the LED Mode according to the KSZ8081 datasheet: > > [00] = LED1: Speed LED0: Link/Activity > > [01] = LED1: Activity LED0: Link > > [10], [11] = Reserved > > default value is [00]. > > > > Indeed, if the bootloader changes them, we would match the wrong > > device. However, I closely examined all the registers, and there is no > > read-only bit that we can use to differentiate both models. The > > LED mode bits are the only ones that have a different default value on the > > KSZ8081: [00] and the KSZ9897: [01]. Also, the RMII registers are not > > documented in the KSZ9897 datasheet so that value is not guaranteed to > > be [01] even though that's what I observed. > > > > Do you think we should find another way to match KSZ8081 and KSZ9897? > > The good news is that I'm now confident about the phy_id emitted by > > both models. > > Lets try asking Prasanna Vengateshan, who is working on other > Microchip switches and PHYs at Microchip. > > Andrew I have already forwarded to the team who worked on the KSZ9897 PHY and added here (part of UNGLinuxDriver). Prasanna V
On Thu, 2022-02-10 at 21:00 +0530, Prasanna Vengateshan wrote: > On Tue, 2022-02-08 at 14:13 +0100, Andrew Lunn wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > On Tue, Feb 08, 2022 at 03:38:41AM -0500, Enguerrand de Ribaucourt wrote: > > > ----- Original Message ----- > > > > From: "Andrew Lunn" <andrew@lunn.ch> > > > > To: "Enguerrand de Ribaucourt" < > > > > enguerrand.de-ribaucourt@savoirfairelinux.com> > > > > Cc: "netdev" <netdev@vger.kernel.org>, "hkallweit1" > > > > <hkallweit1@gmail.com>, "linux" <linux@armlinux.org.uk> > > > > Sent: Tuesday, February 8, 2022 12:28:53 AM > > > > Subject: Re: [PATCH v2 1/2] net: phy: micrel: add Microchip KSZ 9897 > > > > Switch PHY support > > > > > > > > + /* KSZ8081A3/KSZ8091R1 PHY and KSZ9897 switch share the same > > > > > + * exact PHY ID. However, they can be told apart by the default value > > > > > + * of the LED mode. It is 0 for the PHY, and 1 for the switch. > > > > > + */ > > > > > + ret &= (MICREL_KSZ8081_CTRL2_LED_MODE0 | > > > > > MICREL_KSZ8081_CTRL2_LED_MODE1); > > > > > + if (!ksz_8081) > > > > > + return ret; > > > > > + else > > > > > + return !ret; > > > > > > > What exactly does MICREL_KSZ8081_CTRL2_LED_MODE0 and > > > > MICREL_KSZ8081_CTRL2_LED_MODE1 mean? We have to be careful in that > > > > there could be use cases which actually wants to configure the > > > > LEDs. There have been recent discussions for two other PHYs recently > > > > where the bootloader is configuring the LEDs, to something other than > > > > their default value. > > > > > > Those registers configure the LED Mode according to the KSZ8081 datasheet: > > > [00] = LED1: Speed LED0: Link/Activity > > > [01] = LED1: Activity LED0: Link > > > [10], [11] = Reserved > > > default value is [00]. > > > > > > Indeed, if the bootloader changes them, we would match the wrong > > > device. However, I closely examined all the registers, and there is no > > > read-only bit that we can use to differentiate both models. The > > > LED mode bits are the only ones that have a different default value on the > > > KSZ8081: [00] and the KSZ9897: [01]. Also, the RMII registers are not > > > documented in the KSZ9897 datasheet so that value is not guaranteed to > > > be [01] even though that's what I observed. > > > > > > Do you think we should find another way to match KSZ8081 and KSZ9897? > > > The good news is that I'm now confident about the phy_id emitted by > > > both models. > > > > Lets try asking Prasanna Vengateshan, who is working on other > > Microchip switches and PHYs at Microchip. > > > > Andrew > > I have already forwarded to the team who worked on the KSZ9897 PHY and added > here (part of UNGLinuxDriver). > > Prasanna V Added right email id..
----- Original Message ----- > From: "Prasanna VengateshanVaradharajan" <Prasanna.VengateshanVaradharajan@microchip.com> > To: "Enguerrand de Ribaucourt" <enguerrand.de-ribaucourt@savoirfairelinux.com>, "Andrew Lunn" <andrew@lunn.ch> > Cc: "linux" <linux@armlinux.org.uk>, "netdev" <netdev@vger.kernel.org>, "hkallweit1" <hkallweit1@gmail.com>, > UNGLinuxDriver@microchip.com > Sent: Thursday, February 10, 2022 4:38:59 PM > Subject: Re: [PATCH v2 1/2] net: phy: micrel: add Microchip KSZ 9897 Switch PHY support > On Thu, 2022-02-10 at 21:00 +0530, Prasanna Vengateshan wrote: > > On Tue, 2022-02-08 at 14:13 +0100, Andrew Lunn wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > > content is safe > > > On Tue, Feb 08, 2022 at 03:38:41AM -0500, Enguerrand de Ribaucourt wrote: > > > > ----- Original Message ----- > > > > > From: "Andrew Lunn" <andrew@lunn.ch> > > > > > To: "Enguerrand de Ribaucourt" < > > > > > enguerrand.de-ribaucourt@savoirfairelinux.com> > > > > > Cc: "netdev" <netdev@vger.kernel.org>, "hkallweit1" > > > > > <hkallweit1@gmail.com>, "linux" <linux@armlinux.org.uk> > > > > > Sent: Tuesday, February 8, 2022 12:28:53 AM > > > > > Subject: Re: [PATCH v2 1/2] net: phy: micrel: add Microchip KSZ 9897 > > > > > Switch PHY support > > > > > > + /* KSZ8081A3/KSZ8091R1 PHY and KSZ9897 switch share the same > > > > > > + * exact PHY ID. However, they can be told apart by the default value > > > > > > + * of the LED mode. It is 0 for the PHY, and 1 for the switch. > > > > > > + */ > > > > > > + ret &= (MICREL_KSZ8081_CTRL2_LED_MODE0 | > > > > > > MICREL_KSZ8081_CTRL2_LED_MODE1); > > > > > > + if (!ksz_8081) > > > > > > + return ret; > > > > > > + else > > > > > > + return !ret; > > > > > What exactly does MICREL_KSZ8081_CTRL2_LED_MODE0 and > > > > > MICREL_KSZ8081_CTRL2_LED_MODE1 mean? We have to be careful in that > > > > > there could be use cases which actually wants to configure the > > > > > LEDs. There have been recent discussions for two other PHYs recently > > > > > where the bootloader is configuring the LEDs, to something other than > > > > > their default value. > > > > Those registers configure the LED Mode according to the KSZ8081 datasheet: > > > > [00] = LED1: Speed LED0: Link/Activity > > > > [01] = LED1: Activity LED0: Link > > > > [10], [11] = Reserved > > > > default value is [00]. > > > > Indeed, if the bootloader changes them, we would match the wrong > > > > device. However, I closely examined all the registers, and there is no > > > > read-only bit that we can use to differentiate both models. The > > > > LED mode bits are the only ones that have a different default value on the > > > > KSZ8081: [00] and the KSZ9897: [01]. Also, the RMII registers are not > > > > documented in the KSZ9897 datasheet so that value is not guaranteed to > > > > be [01] even though that's what I observed. > > > > Do you think we should find another way to match KSZ8081 and KSZ9897? > > > > The good news is that I'm now confident about the phy_id emitted by > > > > both models. > > > Lets try asking Prasanna Vengateshan, who is working on other > > > Microchip switches and PHYs at Microchip. > > > Andrew > > I have already forwarded to the team who worked on the KSZ9897 PHY and added > > here (part of UNGLinuxDriver). > > Prasanna V > Added right email id.. Hello Prasanna, Have you had any luck contacting the people working on the KSZ9897 PHY? As stated with more details in my previous emails, the RMII phy interface of the KSZ9897 seems to share the same phy_id as the KSZ8081. However, a different ksphy_driver struct must be used for the KSZ9897 PHY to work. Thanks, Enguerrand
On Tue, Apr 05, 2022 at 10:53:57AM -0400, Enguerrand de Ribaucourt wrote: > Hello Prasanna, > > Have you had any luck contacting the people working on the KSZ9897 > PHY? As stated with more details in my previous emails, the RMII phy interface of > the KSZ9897 seems to share the same phy_id as the KSZ8081. However, a different > ksphy_driver struct must be used for the KSZ9897 PHY to work. If there is some other way of detecting the phy device, the drivers can implement the "match_phy_device" method to do whatever it needs to differentiate between the two.
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 44a24b99c894..fc5c33194bdc 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -522,6 +522,34 @@ static int ksz8081_read_status(struct phy_device *phydev) return genphy_read_status(phydev); } +static int ksz8081_ksz9897_match_phy_device(struct phy_device *phydev, + const bool ksz_8081) +{ + int ret; + + if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8081) + return 0; + + ret = phy_read(phydev, MICREL_KSZ8081_CTRL2); + if (ret < 0) + return ret; + + /* KSZ8081A3/KSZ8091R1 PHY and KSZ9897 switch share the same + * exact PHY ID. However, they can be told apart by the default value + * of the LED mode. It is 0 for the PHY, and 1 for the switch. + */ + ret &= (MICREL_KSZ8081_CTRL2_LED_MODE0 | MICREL_KSZ8081_CTRL2_LED_MODE1); + if (!ksz_8081) + return ret; + else + return !ret; +} + +static int ksz8081_match_phy_device(struct phy_device *phydev) +{ + return ksz8081_ksz9897_match_phy_device(phydev, true); +} + static int ksz8061_config_init(struct phy_device *phydev) { int ret; @@ -1561,6 +1589,11 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev, return ret; } +static int ksz9897_match_phy_device(struct phy_device *phydev) +{ + return ksz8081_ksz9897_match_phy_device(phydev, false); +} + #define LAN_EXT_PAGE_ACCESS_CONTROL 0x16 #define LAN_EXT_PAGE_ACCESS_ADDRESS_DATA 0x17 #define LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC 0x4000 @@ -1734,6 +1767,7 @@ static struct phy_driver ksphy_driver[] = { .config_init = ksz8081_config_init, .soft_reset = genphy_soft_reset, .config_aneg = ksz8081_config_aneg, + .match_phy_device = ksz8081_match_phy_device, .read_status = ksz8081_read_status, .config_intr = kszphy_config_intr, .handle_interrupt = kszphy_handle_interrupt, @@ -1869,6 +1903,17 @@ static struct phy_driver ksphy_driver[] = { .config_init = kszphy_config_init, .suspend = genphy_suspend, .resume = genphy_resume, +}, { + .phy_id = PHY_ID_KSZ9897, + .phy_id_mask = 0x00ffffff, + .name = "Microchip KSZ9897", + /* PHY_BASIC_FEATURES */ + .config_init = kszphy_config_init, + .config_aneg = ksz8873mll_config_aneg, + .match_phy_device = ksz9897_match_phy_device, + .read_status = ksz8873mll_read_status, + .suspend = genphy_suspend, + .resume = genphy_resume, } }; module_phy_driver(ksphy_driver); diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h index 1f7c33b2f5a3..05b24bf7f75f 100644 --- a/include/linux/micrel_phy.h +++ b/include/linux/micrel_phy.h @@ -36,6 +36,7 @@ #define PHY_ID_KSZ87XX 0x00221550 #define PHY_ID_KSZ9477 0x00221631 +#define PHY_ID_KSZ9897 0x00221561 /* struct phy_device dev_flags definitions */ #define MICREL_PHY_50MHZ_CLK 0x00000001 @@ -62,4 +63,8 @@ #define KSZ886X_CTRL_MDIX_STAT BIT(4) +#define MICREL_KSZ8081_CTRL2 0x1F +#define MICREL_KSZ8081_CTRL2_LED_MODE0 BIT(4) +#define MICREL_KSZ8081_CTRL2_LED_MODE1 BIT(5) + #endif /* _MICREL_PHY_H */
Adding Microchip 9897 Phy included in KSZ9897 Switch. The KSZ9897 shares the same phy_id as some revisions of the KSZ8081. match_phy_device functions were added to distinguish them. Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com> --- drivers/net/phy/micrel.c | 45 ++++++++++++++++++++++++++++++++++++++ include/linux/micrel_phy.h | 5 +++++ 2 files changed, 50 insertions(+) -- 2.25.1