From: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>

This commit adds support for yet another Microchip T1S lan867x phy
revision. The only meaningful difference between Rev.B that already is
supported and Rev.C is the init change where other undocumented regs
needs writes with unexplained values.
The changes introduced here attempts to follow the manufacturer
recommendations in AN1699.

Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
---
 drivers/net/phy/microchip_t1s.c | 96 +++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index ace2bf35a18a..db84d850b165 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -12,6 +12,7 @@
 #include <linux/phy.h>
 
 #define PHY_ID_LAN867X_REVB1 0x0007C162
+#define PHY_ID_LAN867X_REVC1 0x0007C164
 #define PHY_ID_LAN865X_REVB0 0x0007C1B3
 
 #define LAN867X_REG_STS2 0x0019
@@ -59,6 +60,22 @@ static const u16 lan867x_revb1_fixup_masks[12] = {
 	0x0600, 0x7F00, 0x2000, 0xFFFF,
 };
 
+static const u16 lan867x_revc1_fixup_registers[12] = {
+	0x00D0, 0x00E0, 0x0084, 0x008A,
+	0x00E9, 0x00F5, 0x00F4, 0x00F8,
+	0x00F9, 0x0081, 0x0091, 0x0093,
+};
+
+/* Index 2 & 3 will not be used, these are runtime populated/calculated.
+ * It makes the code a lot easier to read having this arr the same len
+ * as the _fixup_registers arr though
+ */
+static const u16 lan867x_revc1_fixup_values[12] = {
+	0x3F31, 0xC000, 0xFFFF, 0xFFFF,
+	0x9E50, 0x1CF8, 0xC020, 0x9B00,
+	0x4E53, 0x0080, 0x9660, 0x06E9,
+};
+
 /* LAN865x Rev.B0 configuration parameters from AN1760 */
 static const u32 lan865x_revb0_fixup_registers[28] = {
 	0x0091, 0x0081, 0x0043, 0x0044,
@@ -263,6 +280,74 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int lan867x_revc1_read_fixup_value(struct phy_device *phydev, u16 addr)
+{
+	int regval;
+	/* The AN pretty much just states 'trust us' regarding these magic vals */
+	const u16 magic_or = 0xE0;
+	const u16 magic_reg_mask = 0x1F;
+	const u16 magic_check_mask = 0x10;
+
+	regval = lan865x_revb0_indirect_read(phydev, addr);
+	if (regval < 0)
+		return regval;
+
+	regval &= magic_reg_mask;
+
+	return (regval & magic_check_mask) ? regval | magic_or : regval;
+}
+
+static int lan867x_revc1_config_init(struct phy_device *phydev)
+{
+	int err;
+	int regval;
+	u16 override0;
+	u16 override1;
+	const u16 override_addr0 = 0x4;
+	const u16 override_addr1 = 0x8;
+	const u8 index_to_override0 = 2;
+	const u8 index_to_override1 = 3;
+
+	err = lan867x_wait_for_reset_complete(phydev);
+	if (err)
+		return err;
+
+	/* The application note specifies a super convenient process
+	 * where 2 of the fixup regs needs a write with a value that is
+	 * a modified result of another reg read.
+	 * Enjoy the magic show.
+	 */
+	regval = lan867x_revc1_read_fixup_value(phydev, override_addr0);
+	if (regval < 0)
+		return regval;
+	override0 = ((regval + 9) << 10) | ((regval + 14) << 4) | 0x3;
+
+	regval = lan867x_revc1_read_fixup_value(phydev, override_addr1);
+	if (regval < 0)
+		return regval;
+	override1 = (regval + 40) << 10;
+
+	for (int i = 0; i < ARRAY_SIZE(lan867x_revc1_fixup_registers); i++) {
+		/* The hardcoded  */
+		if (i == index_to_override0)
+			err = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+					    lan867x_revc1_fixup_registers[i],
+					    override0);
+		else if (i == index_to_override1)
+			err = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+					    lan867x_revc1_fixup_registers[i],
+					    override1);
+		else
+			err = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+					    lan867x_revc1_fixup_registers[i],
+					    lan867x_revc1_fixup_values[i]);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static int lan86xx_read_status(struct phy_device *phydev)
 {
 	/* The phy has some limitations, namely:
@@ -289,6 +374,16 @@ static struct phy_driver microchip_t1s_driver[] = {
 		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
 		.get_plca_status    = genphy_c45_plca_get_status,
 	},
+	{
+		PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVC1),
+		.name               = "LAN867X Rev.C1",
+		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
+		.config_init        = lan867x_revc1_config_init,
+		.read_status        = lan86xx_read_status,
+		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
+		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
+		.get_plca_status    = genphy_c45_plca_get_status,
+	},
 	{
 		PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0),
 		.name               = "LAN865X Rev.B0 Internal Phy",
@@ -305,6 +400,7 @@ module_phy_driver(microchip_t1s_driver);
 
 static struct mdio_device_id __maybe_unused tbl[] = {
 	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1) },
+	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVC1) },
 	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0) },
 	{ }
 };
-- 
2.40.1

>  #define PHY_ID_LAN867X_REVB1 0x0007C162
> +#define PHY_ID_LAN867X_REVC1 0x0007C164

So there is a gap in the revisions. Maybe a B2 exists?

> +static int lan867x_revc1_read_fixup_value(struct phy_device *phydev, u16 addr)
> +{
> +	int regval;
> +	/* The AN pretty much just states 'trust us' regarding these magic vals */
> +	const u16 magic_or = 0xE0;
> +	const u16 magic_reg_mask = 0x1F;
> +	const u16 magic_check_mask = 0x10;

Reverse christmass tree please. Longest first, shorted last.

> +	regval = lan865x_revb0_indirect_read(phydev, addr);
> +	if (regval < 0)
> +		return regval;
> +
> +	regval &= magic_reg_mask;
> +
> +	return (regval & magic_check_mask) ? regval | magic_or : regval;
> +}
> +
> +static int lan867x_revc1_config_init(struct phy_device *phydev)
> +{
> +	int err;
> +	int regval;
> +	u16 override0;
> +	u16 override1;
> +	const u16 override_addr0 = 0x4;
> +	const u16 override_addr1 = 0x8;
> +	const u8 index_to_override0 = 2;
> +	const u8 index_to_override1 = 3;

Same here.

> +
> +	err = lan867x_wait_for_reset_complete(phydev);
> +	if (err)
> +		return err;
> +
> +	/* The application note specifies a super convenient process
> +	 * where 2 of the fixup regs needs a write with a value that is
> +	 * a modified result of another reg read.
> +	 * Enjoy the magic show.
> +	 */

I really do hope that by revision D1 they get the firmware sorted out
so none of this undocumented magic is needed.

	Andrew

On Mon, Nov 27, 2023 at 02:37:41PM +0100, Andrew Lunn wrote:
> >  #define PHY_ID_LAN867X_REVB1 0x0007C162
> > +#define PHY_ID_LAN867X_REVC1 0x0007C164
> 
> So there is a gap in the revisions. Maybe a B2 exists?

The datasheet lists A0, B1 and C1, seems like Microchip removes the
application notes for old revisions, so no way that I can see to add the
init-fixup for A0.

I'm guessing there is a rev.c0 that was never released to the public.

> > +	const u16 magic_or = 0xE0;
> > +	const u16 magic_reg_mask = 0x1F;
> > +	const u16 magic_check_mask = 0x10;
> 
> Reverse christmass tree please. Longest first, shorted last.

My bad, I was just thinking 'christmas tree' forgot about the reverse.
I'll fix that.

> > +	int err;
> > +	int regval;
> > +	u16 override0;
> > +	u16 override1;
> > +	const u16 override_addr0 = 0x4;
> > +	const u16 override_addr1 = 0x8;
> > +	const u8 index_to_override0 = 2;
> > +	const u8 index_to_override1 = 3;
> 
> Same here.
I'll fix this.

> 
> > +
> > +	err = lan867x_wait_for_reset_complete(phydev);
> > +	if (err)
> > +		return err;
> > +
> > +	/* The application note specifies a super convenient process
> > +	 * where 2 of the fixup regs needs a write with a value that is
> > +	 * a modified result of another reg read.
> > +	 * Enjoy the magic show.
> > +	 */
> 
> I really do hope that by revision D1 they get the firmware sorted out
> so none of this undocumented magic is needed.
> 
> 	Andrew

Really do hope so..