[PATCH net 0/1] phy: microchip_t1s: lan865x rev.b1 support

[PATCH net 0/1] phy: microchip_t1s: lan865x rev.b1 support

[Ramón Nordin Rodriguez]

Hi,
Let me first prepend this submission with 4 points:

* this is not in a merge-ready state
* some code has been copied from the ongoing oa_tc6 work by Parthiban
* this has to interop with code not yet merged (oa_tc6)
* Microchip is looking into if rev.b0 can use the rev.b1 init procedure

The ongoing work by Parthiban Veerasooran is probably gonna get at least
one more revision
(https://lore.kernel.org/netdev/20240418125648.372526-1-Parthiban.Veerasooran@microchip.com/)

I'm publishing this early as it could benefit some of the discussions in
the oa_tc6 threads, as well as giving other devs the possibility
massaging things to a state where they can use the rev.b1 chip (rev.b0
is eol).
And I need feedback on how to wrap this up.

Far as I can tell the phy-driver cannot access some of the regs necessary
for probing the hardware and performing the init/fixup without going
over the spi interface.
The MMDCTRL register (used with indirect access) can address

* PMA - mms 3
* PCS - mms 2
* Vendor specific / PLCA - mms 4

This driver needs to access mms (memory map seleector)
* mac registers - mms 1,
* vendor specific / PLCA - mms 4
* vencor specific - mms 10

Far as I can tell, mms 1 and 10 are only accessible via spi. In the
oa_tc6 patches this is enabled by the oa_tc6 framework by populating the
mdiobus->read/write_c45 funcs.

In order to access any mms I needed I added the following change in the
oa_tc6.c module

static int oa_tc6_get_phy_c45_mms(int devnum)
 {
+       if(devnum & BIT(31))
+               return devnum & GENMASK(30, 0);

Which corresponds to the 'mms | BIT(31)' snippets in this commit, this
is really not how things should be handled, and I need input on how to
proceed here.

Here we get into a weird spot, this driver will need changes in the
oa_tc6 submission, but it's weird to submit support for yet another phy
with that patchset (in my opinion).

This has been tested with a lan8650 rev.b1 chip on one end and a lan8670
usb eval board on the other end. Performance is rather lacking, the
rev.b0 reaches close to the 10Mbit/s limit, but b.1 only gets about
~4Mbit/s, with the same results when PLCA enabled or disabled.

I suggest that this patch is left to brew until the oa_tc6 changes are
accepted, at which time this is fixed up.

Ramón Nordin Rodriguez (1):
  net: phy: microchip_t1s: enable lan865x revb1

 drivers/net/phy/microchip_t1s.c | 189 ++++++++++++++++++++++++++++----
 1 file changed, 166 insertions(+), 23 deletions(-)

-- 
2.43.0

[PATCH 1/1] net: phy: microchip_t1s: enable lan865x revb1

[Ramón Nordin Rodriguez]

The microchip_t1s module is extended with support for lan865x rev.b1,
prior to this commit support for rev.b0 already exists.

Some of the operations performed in the hw probe and init of the phy
require access to registers only accessible via the macphy spi
interface (lan865x is an integrated phy), meaning the init call has
to interop with layers above, namely via read/write_c45.

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

diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index 534ca7d1b061..3c5153aa5c7a 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -5,6 +5,7 @@
  * Support: Microchip Phys:
  *  lan8670/1/2 Rev.B1
  *  lan8650/1 Rev.B0 Internal PHYs
+ *  lan8650/1 Rev.B1 Internal PHYs
  */
 
 #include <linux/kernel.h>
@@ -12,7 +13,7 @@
 #include <linux/phy.h>
 
 #define PHY_ID_LAN867X_REVB1 0x0007C162
-#define PHY_ID_LAN865X_REVB0 0x0007C1B3
+#define PHY_ID_LAN865X 0x0007C1B3
 
 #define LAN867X_REG_STS2 0x0019
 
@@ -22,6 +23,7 @@
 #define LAN865X_REG_CFGPARAM_DATA 0x00D9
 #define LAN865X_REG_CFGPARAM_CTRL 0x00DA
 #define LAN865X_REG_STS2 0x0019
+#define LAN865X_REG_DEVID 0x94
 
 #define LAN865X_CFGPARAM_READ_ENABLE BIT(1)
 
@@ -84,6 +86,65 @@ static const u16 lan865x_revb0_fixup_cfg_regs[5] = {
 	0x0084, 0x008A, 0x00AD, 0x00AE, 0x00AF
 };
 
+enum lan865x_revision {
+	REV_B0 = 0x1,
+	REV_B1 = 0x2,
+};
+
+struct lan865x_revb1_fixup_op {
+	u16 mms;
+	u16 addr;
+	u16 value;
+};
+
+static const struct lan865x_revb1_fixup_op
+lan865x_revb1_fixup_cfg[20] = {
+	{.mms = 4, .addr = 0x00d0, .value = 0x3f31},
+	{.mms = 4, .addr = 0x00e0, .value = 0xc000},
+	{.mms = 4, .addr = 0x0084, .value = 0x0000},
+	{.mms = 4, .addr = 0x008a, .value = 0x0000},
+	{.mms = 4, .addr = 0x00e9, .value = 0x9e50},
+	{.mms = 4, .addr = 0x00f5, .value = 0x1cf8},
+	{.mms = 4, .addr = 0x00f4, .value = 0xc020},
+	{.mms = 4, .addr = 0x00f8, .value = 0x9b00},
+	{.mms = 4, .addr = 0x00f9, .value = 0x4e53},
+	{.mms = 4, .addr = 0x0081, .value = 0x0080},
+	{.mms = 4, .addr = 0x0091, .value = 0x9660},
+	{.mms = 1, .addr = 0x0077, .value = 0x0028},
+	{.mms = 4, .addr = 0x0043, .value = 0x00ff},
+	{.mms = 4, .addr = 0x0044, .value = 0xffff},
+	{.mms = 4, .addr = 0x0045, .value = 0x0000},
+	{.mms = 4, .addr = 0x0053, .value = 0x00ff},
+	{.mms = 4, .addr = 0x0054, .value = 0xffff},
+	{.mms = 4, .addr = 0x0055, .value = 0x0000},
+	{.mms = 4, .addr = 0x0040, .value = 0x0002},
+	{.mms = 4, .addr = 0x0050, .value = 0x0002}
+};
+
+/* this func is copy pasted from Parthibans ongoing work with oa_tc6
+ * see https://lore.kernel.org/netdev/20240418125648.372526-7-Parthiban.Veerasooran@microchip.com/
+ */
+static int lan865x_phy_read_mmd(struct phy_device *phydev, int devnum,
+				u16 regnum)
+{
+	struct mii_bus *bus = phydev->mdio.bus;
+	int addr = phydev->mdio.addr;
+
+	return bus->read_c45(bus, addr, devnum, regnum);
+}
+
+/* this func is copy pasted from Parthibans ongoing work with oa_tc6
+ * see https://lore.kernel.org/netdev/20240418125648.372526-7-Parthiban.Veerasooran@microchip.com/
+ */
+static int lan865x_phy_write_mmd(struct phy_device *phydev, int devnum,
+				 u16 regnum, u16 val)
+{
+	struct mii_bus *bus = phydev->mdio.bus;
+	int addr = phydev->mdio.addr;
+
+	return bus->write_c45(bus, addr, devnum, regnum, val);
+}
+
 /* Pulled from AN1760 describing 'indirect read'
  *
  * write_register(0x4, 0x00D8, addr)
@@ -112,7 +173,7 @@ static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr)
 /* This is pulled straight from AN1760 from 'calculation of offset 1' &
  * 'calculation of offset 2'
  */
-static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[2])
+static int lan865x_revb0_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[2])
 {
 	const u16 fixup_regs[2] = {0x0004, 0x0008};
 	int ret;
@@ -130,7 +191,41 @@ static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[2]
 	return 0;
 }
 
-static int lan865x_read_cfg_params(struct phy_device *phydev, u16 cfg_params[])
+static int lan865x_revb1_gen_cfgparams(struct phy_device *phydev, u16 params[2])
+{
+	const u16 fixup_regs[2] = {0x0004, 0x0008};
+	int ret;
+
+	// this loop attempts to de-weirdify the method described in AN1760
+	for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) {
+		// only the lower 8 bits of the results here are used
+		ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
+		if (ret < 0)
+			return ret;
+		// The AN states that the readback value is in the range [-5, 15]
+		// and that it uses 5 bits, bit wonky of a description, it really only
+		// makes sense if the read is sign extended.
+		// Since the regs are part of the reserved range there is no way
+		// of telling how this really works though.
+		if (ret & BIT(4))
+			// The AN specifies 'ret - 0x20' which is again pretty weird
+			// the addition here gives the same result on a s8 (with overflow)
+			// the assignment here are downcasting to s8 for sign extension
+			params[i] = (s8)(ret + 0xE0);
+		else
+			params[i] = (s8)ret;
+	}
+
+	// And now for the really weird part, there's no explanation to what any of this
+	// means, just that we need these results written to magic regs.
+	// Since these ops shift a lot, the previous sign extension is probably meaningless.
+	params[0] = (((params[0] + 9) & 0x3F) << 10) | ((params[0] + 14) & 0x3f) << 4 | 0x3;
+	params[1] = ((params[1] + 40) & 0x3F) << 10;
+
+	return 0;
+}
+
+static int lan865x_revb0_read_cfg_params(struct phy_device *phydev, u16 cfg_params[])
 {
 	int ret;
 
@@ -145,7 +240,7 @@ static int lan865x_read_cfg_params(struct phy_device *phydev, u16 cfg_params[])
 	return 0;
 }
 
-static int lan865x_write_cfg_params(struct phy_device *phydev, u16 cfg_params[])
+static int lan865x_revb0_write_cfg_params(struct phy_device *phydev, u16 cfg_params[])
 {
 	int ret;
 
@@ -160,18 +255,29 @@ static int lan865x_write_cfg_params(struct phy_device *phydev, u16 cfg_params[])
 	return 0;
 }
 
-static int lan865x_setup_cfgparam(struct phy_device *phydev)
+static int lan865x_revb0_setup_cfgparam(struct phy_device *phydev)
 {
 	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
 	u16 cfg_results[5];
 	s8 offsets[2];
 	int ret;
 
-	ret = lan865x_generate_cfg_offsets(phydev, offsets);
+	/* Reference to AN1760
+	 * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf
+	 */
+	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) {
+		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
+				    lan865x_revb0_fixup_registers[i],
+				    lan865x_revb0_fixup_values[i]);
+		if (ret)
+			return ret;
+	}
+
+	ret = lan865x_revb0_generate_cfg_offsets(phydev, offsets);
 	if (ret)
 		return ret;
 
-	ret = lan865x_read_cfg_params(phydev, cfg_params);
+	ret = lan865x_revb0_read_cfg_params(phydev, cfg_params);
 	if (ret)
 		return ret;
 
@@ -190,27 +296,64 @@ static int lan865x_setup_cfgparam(struct phy_device *phydev)
 			  FIELD_PREP(GENMASK(15, 8), 17 + offsets[0]) |
 			  (22 + offsets[0]);
 
-	return lan865x_write_cfg_params(phydev, cfg_results);
+	return lan865x_revb0_write_cfg_params(phydev, cfg_results);
 }
 
-static int lan865x_revb0_config_init(struct phy_device *phydev)
+static int lan865x_revb1_setup_cfgparam(struct phy_device *phydev)
 {
+	const struct lan865x_revb1_fixup_op *op;
+	u16 generated_params[2];
+	u16 fixup_val;
 	int ret;
 
-	/* Reference to AN1760
-	 * https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf
-	 */
-	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) {
-		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
-				    lan865x_revb0_fixup_registers[i],
-				    lan865x_revb0_fixup_values[i]);
+	ret = lan865x_revb1_gen_cfgparams(phydev, generated_params);
+	if (ret)
+		return ret;
+
+	for (int i = 0; i < ARRAY_SIZE(lan865x_revb1_fixup_cfg); i++) {
+		op = &lan865x_revb1_fixup_cfg[i];
+		fixup_val = op->value;
+		if (i == 2)
+			fixup_val = generated_params[0];
+		else if (i == 3)
+			fixup_val = generated_params[1];
+		/* All of the regs locatd in mms 4 could use phy_write_mmd as these are
+		 * accessible via MDIO_MMD_VEND2, but mms 1 not 'indirect accessible'.
+		 * Less conditionals are favored here by doing the set in just one way.
+		 */
+		ret = lan865x_phy_write_mmd(phydev, op->mms | BIT(31), op->addr, fixup_val);
 		if (ret)
 			return ret;
 	}
-	/* Function to calculate and write the configuration parameters in the
-	 * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760)
+	return 0;
+}
+
+static int lan865x_config_init(struct phy_device *phydev)
+{
+	u32 rev_mask = GENMASK(1, 0);
+	enum lan865x_revision rev;
+	u32 mms10 = 10;
+	u32 devid;
+
+	/* The standard reg OA_PHYID (mms 0, addr 0x1) does not contain any
+	 * information that distinguishes hardware revisions.
+	 * HW rev can be read from the vendor specific DEVID reg mms 10, addr 0x94
 	 */
-	return lan865x_setup_cfgparam(phydev);
+	devid = lan865x_phy_read_mmd(phydev, mms10 | BIT(31), LAN865X_REG_DEVID);
+
+	if (devid < 0)
+		return devid;
+
+	rev = devid & rev_mask;
+	switch (rev) {
+	case REV_B0:
+		return lan865x_revb0_setup_cfgparam(phydev);
+	case REV_B1:
+		return lan865x_revb1_setup_cfgparam(phydev);
+	}
+
+	dev_err(&phydev->mdio.dev, "unsupported chip rev: 0x%x", rev);
+	return -ENODEV;
 }
 
 static int lan867x_revb1_config_init(struct phy_device *phydev)
@@ -280,10 +423,10 @@ static struct phy_driver microchip_t1s_driver[] = {
 		.get_plca_status    = genphy_c45_plca_get_status,
 	},
 	{
-		PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0),
-		.name               = "LAN865X Rev.B0 Internal Phy",
+		PHY_ID_MATCH_EXACT(PHY_ID_LAN865X),
+		.name               = "LAN865X Rev.B0/1 Internal Phy",
 		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
-		.config_init        = lan865x_revb0_config_init,
+		.config_init        = lan865x_config_init,
 		.read_status        = lan86xx_read_status,
 		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
 		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
@@ -295,7 +438,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_LAN865X_REVB0) },
+	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN865X) },
 	{ }
 };
 
-- 
2.43.0

Re: [PATCH 1/1] net: phy: microchip_t1s: enable lan865x revb1

[Russell King (Oracle)]

On Fri, May 24, 2024 at 04:07:06PM +0200, Ramón Nordin Rodriguez wrote:
> +/* this func is copy pasted from Parthibans ongoing work with oa_tc6
> + * see https://lore.kernel.org/netdev/20240418125648.372526-7-Parthiban.Veerasooran@microchip.com/
> + */

Oh dear...

> +static int lan865x_phy_read_mmd(struct phy_device *phydev, int devnum,
> +				u16 regnum)
> +{
> +	struct mii_bus *bus = phydev->mdio.bus;
> +	int addr = phydev->mdio.addr;
> +
> +	return bus->read_c45(bus, addr, devnum, regnum);
> +}
> +
> +/* this func is copy pasted from Parthibans ongoing work with oa_tc6
> + * see https://lore.kernel.org/netdev/20240418125648.372526-7-Parthiban.Veerasooran@microchip.com/
> + */
> +static int lan865x_phy_write_mmd(struct phy_device *phydev, int devnum,
> +				 u16 regnum, u16 val)
> +{
> +	struct mii_bus *bus = phydev->mdio.bus;
> +	int addr = phydev->mdio.addr;
> +
> +	return bus->write_c45(bus, addr, devnum, regnum, val);
> +}

So you need both of these to (dangerously) read and write the C45
address using a devnum with bit 31 set. Please at the very least
take the MDIO bus lock so the locking of these operations remains
consistent.

I'm not sure that abusing the interface by setting bit 31 in the
device address, which is supposed to be between 0 and 31 is
something we're prepared to entertain.

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

RE: [EXTERNAL] [PATCH 1/1] net: phy: microchip_t1s: enable lan865x revb1

[Sai Krishna Gajula]

> -----Original Message-----
> From: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
> Sent: Friday, May 24, 2024 7:37 PM
> To: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: parthiban.veerasooran@microchip.com; Ramón Nordin Rodriguez
> <ramon.nordin.rodriguez@ferroamp.se>
> Subject: [EXTERNAL] [PATCH 1/1] net: phy: microchip_t1s: enable lan865x
> revb1
> 
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
> 
> ----------------------------------------------------------------------
> The microchip_t1s module is extended with support for lan865x rev.b1, prior
> to this commit support for rev.b0 already exists.
> 
> Some of the operations performed in the hw probe and init of the phy require
> access to registers only accessible via the macphy spi interface (lan865x is an
> integrated phy), meaning the init call has to interop with layers above, namely
> via read/write_c45.
> 
> Signed-off-by: Ramón Nordin Rodriguez
> <ramon.nordin.rodriguez@ferroamp.se>

Fixes tag is required if the patch is for NET tree. Also, it seems like build for 32 bit seems to be failing. Please check.

> ---
>  drivers/net/phy/microchip_t1s.c | 189 ++++++++++++++++++++++++++++-
> ---
>  1 file changed, 166 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/phy/microchip_t1s.c
> b/drivers/net/phy/microchip_t1s.c index 534ca7d1b061..3c5153aa5c7a
> 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c
> @@ -5,6 +5,7 @@
>   * Support: Microchip Phys:
>   *  lan8670/1/2 Rev.B1
>   *  lan8650/1 Rev.B0 Internal PHYs
> + *  lan8650/1 Rev.B1 Internal PHYs
>   */
> 
>  #include <linux/kernel.h>
> @@ -12,7 +13,7 @@
>  #include <linux/phy.h>
> 
>  #define PHY_ID_LAN867X_REVB1 0x0007C162 -#define
> PHY_ID_LAN865X_REVB0 0x0007C1B3
> +#define PHY_ID_LAN865X 0x0007C1B3
> 
>  #define LAN867X_REG_STS2 0x0019
> 
> @@ -22,6 +23,7 @@
>  #define LAN865X_REG_CFGPARAM_DATA 0x00D9  #define
> LAN865X_REG_CFGPARAM_CTRL 0x00DA  #define LAN865X_REG_STS2
> 0x0019
> +#define LAN865X_REG_DEVID 0x94
> 
>  #define LAN865X_CFGPARAM_READ_ENABLE BIT(1)
> 
> @@ -84,6 +86,65 @@ static const u16 lan865x_revb0_fixup_cfg_regs[5] = {
>  	0x0084, 0x008A, 0x00AD, 0x00AE, 0x00AF  };
> 
> +enum lan865x_revision {
> +	REV_B0 = 0x1,
> +	REV_B1 = 0x2,
> +};
> +
> +struct lan865x_revb1_fixup_op {
> +	u16 mms;
> +	u16 addr;
> +	u16 value;
> +};
> +
> +static const struct lan865x_revb1_fixup_op lan865x_revb1_fixup_cfg[20]
> += {
> +	{.mms = 4, .addr = 0x00d0, .value = 0x3f31},
> +	{.mms = 4, .addr = 0x00e0, .value = 0xc000},
> +	{.mms = 4, .addr = 0x0084, .value = 0x0000},
> +	{.mms = 4, .addr = 0x008a, .value = 0x0000},
> +	{.mms = 4, .addr = 0x00e9, .value = 0x9e50},
> +	{.mms = 4, .addr = 0x00f5, .value = 0x1cf8},
> +	{.mms = 4, .addr = 0x00f4, .value = 0xc020},
> +	{.mms = 4, .addr = 0x00f8, .value = 0x9b00},
> +	{.mms = 4, .addr = 0x00f9, .value = 0x4e53},
> +	{.mms = 4, .addr = 0x0081, .value = 0x0080},
> +	{.mms = 4, .addr = 0x0091, .value = 0x9660},
> +	{.mms = 1, .addr = 0x0077, .value = 0x0028},
> +	{.mms = 4, .addr = 0x0043, .value = 0x00ff},
> +	{.mms = 4, .addr = 0x0044, .value = 0xffff},
> +	{.mms = 4, .addr = 0x0045, .value = 0x0000},
> +	{.mms = 4, .addr = 0x0053, .value = 0x00ff},
> +	{.mms = 4, .addr = 0x0054, .value = 0xffff},
> +	{.mms = 4, .addr = 0x0055, .value = 0x0000},
> +	{.mms = 4, .addr = 0x0040, .value = 0x0002},
> +	{.mms = 4, .addr = 0x0050, .value = 0x0002} };
> +
> +/* this func is copy pasted from Parthibans ongoing work with oa_tc6
> + * see
> +https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_ne
> +tdev_20240418125648.372526-2D7-2DParthiban.Veerasooran-
> 40microchip.com_
> +&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=c3MsgrR-U-
> HFhmFd6R4MWRZG-8QeikJn5P
> +kjqMTpBSg&m=XjkUpKCoT2lM06i5fF3KdqFf-notCV0B-
> LT0mcLVMbPDQ29Vs7R3Ek2zHA7
> +0BlwP&s=rNsk2zP8m89VjgtwQo3jL-STsPKU5S7Ig8TGr0IsZTY&e=
> + */
> +static int lan865x_phy_read_mmd(struct phy_device *phydev, int devnum,
> +				u16 regnum)
> +{
> +	struct mii_bus *bus = phydev->mdio.bus;
> +	int addr = phydev->mdio.addr;
> +
> +	return bus->read_c45(bus, addr, devnum, regnum); }
> +
> +/* this func is copy pasted from Parthibans ongoing work with oa_tc6
> + * see
> +https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_ne
> +tdev_20240418125648.372526-2D7-2DParthiban.Veerasooran-
> 40microchip.com_
> +&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=c3MsgrR-U-
> HFhmFd6R4MWRZG-8QeikJn5P
> +kjqMTpBSg&m=XjkUpKCoT2lM06i5fF3KdqFf-notCV0B-
> LT0mcLVMbPDQ29Vs7R3Ek2zHA7
> +0BlwP&s=rNsk2zP8m89VjgtwQo3jL-STsPKU5S7Ig8TGr0IsZTY&e=
> + */
> +static int lan865x_phy_write_mmd(struct phy_device *phydev, int devnum,
> +				 u16 regnum, u16 val)
> +{
> +	struct mii_bus *bus = phydev->mdio.bus;
> +	int addr = phydev->mdio.addr;
> +
> +	return bus->write_c45(bus, addr, devnum, regnum, val); }
> +
>  /* Pulled from AN1760 describing 'indirect read'
>   *
>   * write_register(0x4, 0x00D8, addr)
> @@ -112,7 +173,7 @@ static int lan865x_revb0_indirect_read(struct
> phy_device *phydev, u16 addr)
>  /* This is pulled straight from AN1760 from 'calculation of offset 1' &
>   * 'calculation of offset 2'
>   */
> -static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8
> offsets[2])
> +static int lan865x_revb0_generate_cfg_offsets(struct phy_device
> +*phydev, s8 offsets[2])
>  {
>  	const u16 fixup_regs[2] = {0x0004, 0x0008};
>  	int ret;
> @@ -130,7 +191,41 @@ static int lan865x_generate_cfg_offsets(struct
> phy_device *phydev, s8 offsets[2]
>  	return 0;
>  }
> 
> -static int lan865x_read_cfg_params(struct phy_device *phydev, u16
> cfg_params[])
> +static int lan865x_revb1_gen_cfgparams(struct phy_device *phydev, u16
> +params[2]) {
> +	const u16 fixup_regs[2] = {0x0004, 0x0008};
> +	int ret;
> +
> +	// this loop attempts to de-weirdify the method described in AN1760
> +	for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) {
> +		// only the lower 8 bits of the results here are used
> +		ret = lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
> +		if (ret < 0)
> +			return ret;
> +		// The AN states that the readback value is in the range [-5,
> 15]
> +		// and that it uses 5 bits, bit wonky of a description, it really
> only
> +		// makes sense if the read is sign extended.
> +		// Since the regs are part of the reserved range there is no way
> +		// of telling how this really works though.
> +		if (ret & BIT(4))
> +			// The AN specifies 'ret - 0x20' which is again pretty
> weird
> +			// the addition here gives the same result on a s8
> (with overflow)
> +			// the assignment here are downcasting to s8 for sign
> extension
> +			params[i] = (s8)(ret + 0xE0);
> +		else
> +			params[i] = (s8)ret;
> +	}
> +
> +	// And now for the really weird part, there's no explanation to what
> any of this
> +	// means, just that we need these results written to magic regs.
> +	// Since these ops shift a lot, the previous sign extension is probably
> meaningless.
> +	params[0] = (((params[0] + 9) & 0x3F) << 10) | ((params[0] + 14) &
> 0x3f) << 4 | 0x3;
> +	params[1] = ((params[1] + 40) & 0x3F) << 10;
> +
> +	return 0;
> +}
> +
> +static int lan865x_revb0_read_cfg_params(struct phy_device *phydev, u16
> +cfg_params[])
>  {
>  	int ret;
> 
> @@ -145,7 +240,7 @@ static int lan865x_read_cfg_params(struct
> phy_device *phydev, u16 cfg_params[])
>  	return 0;
>  }
> 
> -static int lan865x_write_cfg_params(struct phy_device *phydev, u16
> cfg_params[])
> +static int lan865x_revb0_write_cfg_params(struct phy_device *phydev,
> +u16 cfg_params[])
>  {
>  	int ret;
> 
> @@ -160,18 +255,29 @@ static int lan865x_write_cfg_params(struct
> phy_device *phydev, u16 cfg_params[])
>  	return 0;
>  }
> 
> -static int lan865x_setup_cfgparam(struct phy_device *phydev)
> +static int lan865x_revb0_setup_cfgparam(struct phy_device *phydev)
>  {
>  	u16 cfg_params[ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs)];
>  	u16 cfg_results[5];
>  	s8 offsets[2];
>  	int ret;
> 
> -	ret = lan865x_generate_cfg_offsets(phydev, offsets);
> +	/* Reference to AN1760
> +	 * https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__ww1.microchip.com_downloads_aemDocuments_documents_AIS_Prod
> uctDocuments_SupportingCollateral_AN-2DLAN8650-2D1-2DConfiguration-
> 2D60001760.pdf&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=c3MsgrR-U-
> HFhmFd6R4MWRZG-8QeikJn5PkjqMTpBSg&m=XjkUpKCoT2lM06i5fF3KdqFf-
> notCV0B-
> LT0mcLVMbPDQ29Vs7R3Ek2zHA70BlwP&s=9lbQcePQzdtdyNZYMB_GAniwOj
> qhUp3KA0VBMaGmi6M&e=
> +	 */
> +	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) {
> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
> +				    lan865x_revb0_fixup_registers[i],
> +				    lan865x_revb0_fixup_values[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = lan865x_revb0_generate_cfg_offsets(phydev, offsets);
>  	if (ret)
>  		return ret;
> 
> -	ret = lan865x_read_cfg_params(phydev, cfg_params);
> +	ret = lan865x_revb0_read_cfg_params(phydev, cfg_params);
>  	if (ret)
>  		return ret;
> 
> @@ -190,27 +296,64 @@ static int lan865x_setup_cfgparam(struct
> phy_device *phydev)
>  			  FIELD_PREP(GENMASK(15, 8), 17 + offsets[0]) |
>  			  (22 + offsets[0]);
> 
> -	return lan865x_write_cfg_params(phydev, cfg_results);
> +	return lan865x_revb0_write_cfg_params(phydev, cfg_results);
>  }
> 
> -static int lan865x_revb0_config_init(struct phy_device *phydev)
> +static int lan865x_revb1_setup_cfgparam(struct phy_device *phydev)
>  {
> +	const struct lan865x_revb1_fixup_op *op;
> +	u16 generated_params[2];
> +	u16 fixup_val;
>  	int ret;
> 
> -	/* Reference to AN1760
> -	 * https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__ww1.microchip.com_downloads_aemDocuments_documents_AIS_Prod
> uctDocuments_SupportingCollateral_AN-2DLAN8650-2D1-2DConfiguration-
> 2D60001760.pdf&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=c3MsgrR-U-
> HFhmFd6R4MWRZG-8QeikJn5PkjqMTpBSg&m=XjkUpKCoT2lM06i5fF3KdqFf-
> notCV0B-
> LT0mcLVMbPDQ29Vs7R3Ek2zHA70BlwP&s=9lbQcePQzdtdyNZYMB_GAniwOj
> qhUp3KA0VBMaGmi6M&e=
> -	 */
> -	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) {
> -		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2,
> -				    lan865x_revb0_fixup_registers[i],
> -				    lan865x_revb0_fixup_values[i]);
> +	ret = lan865x_revb1_gen_cfgparams(phydev, generated_params);
> +	if (ret)
> +		return ret;
> +
> +	for (int i = 0; i < ARRAY_SIZE(lan865x_revb1_fixup_cfg); i++) {
> +		op = &lan865x_revb1_fixup_cfg[i];
> +		fixup_val = op->value;
> +		if (i == 2)
> +			fixup_val = generated_params[0];
> +		else if (i == 3)
> +			fixup_val = generated_params[1];
> +		/* All of the regs locatd in mms 4 could use phy_write_mmd
> as these are
> +		 * accessible via MDIO_MMD_VEND2, but mms 1 not 'indirect
> accessible'.
> +		 * Less conditionals are favored here by doing the set in just
> one way.
> +		 */
> +		ret = lan865x_phy_write_mmd(phydev, op->mms | BIT(31),
> op->addr,
> +fixup_val);
>  		if (ret)
>  			return ret;
>  	}
> -	/* Function to calculate and write the configuration parameters in the
> -	 * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from
> AN1760)
> +	return 0;
> +}
> +
> +static int lan865x_config_init(struct phy_device *phydev) {
> +	u32 rev_mask = GENMASK(1, 0);
> +	enum lan865x_revision rev;
> +	u32 mms10 = 10;
> +	u32 devid;
> +
> +	/* The standard reg OA_PHYID (mms 0, addr 0x1) does not contain
> any
> +	 * information that distinguishes hardware revisions.
> +	 * HW rev can be read from the vendor specific DEVID reg mms 10,
> addr
> +0x94
>  	 */
> -	return lan865x_setup_cfgparam(phydev);
> +	devid = lan865x_phy_read_mmd(phydev, mms10 | BIT(31),
> +LAN865X_REG_DEVID);
> +
> +	if (devid < 0)
> +		return devid;
> +
> +	rev = devid & rev_mask;
> +	switch (rev) {
> +	case REV_B0:
> +		return lan865x_revb0_setup_cfgparam(phydev);
> +	case REV_B1:
> +		return lan865x_revb1_setup_cfgparam(phydev);
> +	}
> +
> +	dev_err(&phydev->mdio.dev, "unsupported chip rev: 0x%x", rev);
> +	return -ENODEV;
>  }
> 
>  static int lan867x_revb1_config_init(struct phy_device *phydev) @@ -280,10
> +423,10 @@ static struct phy_driver microchip_t1s_driver[] = {
>  		.get_plca_status    = genphy_c45_plca_get_status,
>  	},
>  	{
> -		PHY_ID_MATCH_EXACT(PHY_ID_LAN865X_REVB0),
> -		.name               = "LAN865X Rev.B0 Internal Phy",
> +		PHY_ID_MATCH_EXACT(PHY_ID_LAN865X),
> +		.name               = "LAN865X Rev.B0/1 Internal Phy",
>  		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
> -		.config_init        = lan865x_revb0_config_init,
> +		.config_init        = lan865x_config_init,
>  		.read_status        = lan86xx_read_status,
>  		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
>  		.set_plca_cfg	    = genphy_c45_plca_set_cfg,
> @@ -295,7 +438,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_LAN865X_REVB0) },
> +	{ PHY_ID_MATCH_EXACT(PHY_ID_LAN865X) },
>  	{ }
>  };
> 
> --
> 2.43.0
> 

Re: [PATCH net 0/1] phy: microchip_t1s: lan865x rev.b1 support

[Andrew Lunn]

> Far as I can tell the phy-driver cannot access some of the regs necessary
> for probing the hardware and performing the init/fixup without going
> over the spi interface.
> The MMDCTRL register (used with indirect access) can address
> 
> * PMA - mms 3
> * PCS - mms 2
> * Vendor specific / PLCA - mms 4
> 
> This driver needs to access mms (memory map seleector)
> * mac registers - mms 1,
> * vendor specific / PLCA - mms 4
> * vencor specific - mms 10

In general, a MAC should not be touching the PHY, and the PHY should
not be touching the MAC. This rule is because you should not assume
you have a specific MAC+PHY pair. However, this is one blob of
silicon, so we can relax that a bit if needed.

So it sounds like Microchip have mixed up the register address spaces
:-(

I guess this also means there is no discrete version of this PHY,
because where would these registers be?

Do any of the registers in the wrong address space need to be poked at
runtime? By that i mean config_aneg(), read_status(). Or are they only
needed around the time the PHY is probed?

How critical is the ordering? Could we have the Microchip MAC driver
probe. It instantiates the TC6 framework which registers the MDIO bus
and probes the PHY. Can the MAC driver then complete the PHY setup
using the registers in the wrong address space? Does it need to access
any PHY registers in the correct address space? The MAC driver should
be able to do this before phy_start()

Does MMS 0 register 1 "PHY Identification Register" give enough
information to know it is a B1 PHY? The standard suggests it is a
straight copy of PHY registers 2 and 3. So the MAC driver does not
need to touch PHY registers, we are not totally violating the
layering...

	Andrew

Re: [PATCH net 0/1] phy: microchip_t1s: lan865x rev.b1 support

[Parthiban.Veerasooran]

Hi Andrew,

On 24/05/24 8:20 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> Far as I can tell the phy-driver cannot access some of the regs necessary
>> for probing the hardware and performing the init/fixup without going
>> over the spi interface.
>> The MMDCTRL register (used with indirect access) can address
>>
>> * PMA - mms 3
>> * PCS - mms 2
>> * Vendor specific / PLCA - mms 4
>>
>> This driver needs to access mms (memory map seleector)
>> * mac registers - mms 1,
>> * vendor specific / PLCA - mms 4
>> * vencor specific - mms 10
> 
> In general, a MAC should not be touching the PHY, and the PHY should
> not be touching the MAC. This rule is because you should not assume
> you have a specific MAC+PHY pair. However, this is one blob of
> silicon, so we can relax that a bit if needed.
> 
> So it sounds like Microchip have mixed up the register address spaces
> :-(
> 
> I guess this also means there is no discrete version of this PHY,
> because where would these registers be?
> 
> Do any of the registers in the wrong address space need to be poked at
> runtime? By that i mean config_aneg(), read_status(). Or are they only
> needed around the time the PHY is probed?
> 
> How critical is the ordering? Could we have the Microchip MAC driver
> probe. It instantiates the TC6 framework which registers the MDIO bus
> and probes the PHY. Can the MAC driver then complete the PHY setup
> using the registers in the wrong address space? Does it need to access
> any PHY registers in the correct address space? The MAC driver should
> be able to do this before phy_start()
> 
> Does MMS 0 register 1 "PHY Identification Register" give enough
> information to know it is a B1 PHY? The standard suggests it is a
> straight copy of PHY registers 2 and 3. So the MAC driver does not
> need to touch PHY registers, we are not totally violating the
> layering...
I completely agree with all your above points. As I told already, I am 
in talk with our design team about this complications by the time this 
Rev.B1 support has been posted. Will try to get the clarity as soon as 
possible. Sorry for the inconvenience.

So I would recommend to go with Rev.B0 support now as "CD disable if 
PLCA is enabled" fix which gives stable performance until we get the 
clarity on B1. So that we can evaluate the TC6 framework (oa_tc6.c) to 
have a initial/basic version in the mainline first.

Best regards,
Parthiban V
> 
>          Andrew
> 

Re: [PATCH net 0/1] phy: microchip_t1s: lan865x rev.b1 support

[Russell King (Oracle)]

On Fri, May 24, 2024 at 04:07:05PM +0200, Ramón Nordin Rodriguez wrote:
> Hi,
> Let me first prepend this submission with 4 points:
> 
> * this is not in a merge-ready state
> * some code has been copied from the ongoing oa_tc6 work by Parthiban
> * this has to interop with code not yet merged (oa_tc6)
> * Microchip is looking into if rev.b0 can use the rev.b1 init procedure
> 
> The ongoing work by Parthiban Veerasooran is probably gonna get at least
> one more revision
> (https://lore.kernel.org/netdev/20240418125648.372526-1-Parthiban.Veerasooran@microchip.com/)
> 
> I'm publishing this early as it could benefit some of the discussions in
> the oa_tc6 threads, as well as giving other devs the possibility
> massaging things to a state where they can use the rev.b1 chip (rev.b0
> is eol).
> And I need feedback on how to wrap this up.
> 
> Far as I can tell the phy-driver cannot access some of the regs necessary
> for probing the hardware and performing the init/fixup without going
> over the spi interface.
> The MMDCTRL register (used with indirect access) can address
> 
> * PMA - mms 3
> * PCS - mms 2
> * Vendor specific / PLCA - mms 4
> 
> This driver needs to access mms (memory map seleector)
> * mac registers - mms 1,
> * vendor specific / PLCA - mms 4
> * vencor specific - mms 10
> 
> Far as I can tell, mms 1 and 10 are only accessible via spi. In the
> oa_tc6 patches this is enabled by the oa_tc6 framework by populating the
> mdiobus->read/write_c45 funcs.
> 
> In order to access any mms I needed I added the following change in the
> oa_tc6.c module
> 
> static int oa_tc6_get_phy_c45_mms(int devnum)
>  {
> +       if(devnum & BIT(31))
> +               return devnum & GENMASK(30, 0);
> 
> Which corresponds to the 'mms | BIT(31)' snippets in this commit, this
> is really not how things should be handled, and I need input on how to
> proceed here.

So if bit 31 of the devnum is set, then the other bits specify the
MMS instead of the MMD.

I'm not sure we want to overload the PHY interface in this way. We
have been down this path before with the MDIO bus read/write methods
being used for both C22 and C45 accesses, and it created problems,
so I don't think we want to repeat that mistake by doing the same
thing here.

There's a comment in the original patches etc about the PHY being
discovered via C22, and then not using the direct accesses to the
C45 register space. I'm wondering whether we should split
phydev->is_c45 to be phydev->probed_c45 / phydev->use_c45.

The former gets used during bus scanning and probe time to determine
how we match the device driver to the phydev. The latter gets used
_only_ to determine whether the read/write_mmd ops use direct mode
or indirect mode.

Before the driver probe is called, we should do:

	phydev->use_mmd = phydev->probed_c45;

to ensure that todays behaviour is preserved. Then, provide a
function, maybe, phy_use_direct_c45(phydev) which will set this
phydev->use_mmd flag, and phy_use_indirect_c45(phydev) which will
clear it.

phy_use_direct_c45() should also check whether the MDIO bus that
is attached supports C45 access, and return an error if not.

That will give you the ability to use the direct access method
where necessary.

There's a comment in the referred to code:

+	/* OPEN Alliance 10BASE-T1x compliance MAC-PHYs will have both C22 and
+	 * C45 registers space. If the PHY is discovered via C22 bus protocol it
+	 * assumes it uses C22 protocol and always uses C22 registers indirect
+	 * access to access C45 registers. This is because, we don't have a
+	 * clean separation between C22/C45 register space and C22/C45 MDIO bus
+	 * protocols. Resulting, PHY C45 registers direct access can't be used
+	 * which can save multiple SPI bus access. To support this feature, PHY
+	 * drivers can set .read_mmd/.write_mmd in the PHY driver to call
+	 * .read_c45/.write_c45. Ex: drivers/net/phy/microchip_t1s.c
+	 */

which I don't really understand. It claims that C45 direct access
"saves" multiple SPI bus accesses. However, C45 indirect access
requires:

1. A C22 write to the MMD control register
2. A C22 write to the MMD data register
3. Another C22 write to the MMD control register
4. A c22 read or write to access the actual data.

Do four C22 bus transactions over SPI require more or less SPI bus
accesses than a single C45 bus transaction over SPI? I suspect not,
which makes the comment above factually incorrect.

If we have direct C45 access working, does that remove the need to
have this special bit-31 to signal MMS access requirement?

Thanks.

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

Re: [PATCH net 0/1] phy: microchip_t1s: lan865x rev.b1 support

[Parthiban.Veerasooran]

Hi,

On 24/05/24 9:00 pm, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, May 24, 2024 at 04:07:05PM +0200, Ramón Nordin Rodriguez wrote:
>> Hi,
>> Let me first prepend this submission with 4 points:
>>
>> * this is not in a merge-ready state
>> * some code has been copied from the ongoing oa_tc6 work by Parthiban
>> * this has to interop with code not yet merged (oa_tc6)
>> * Microchip is looking into if rev.b0 can use the rev.b1 init procedure
>>
>> The ongoing work by Parthiban Veerasooran is probably gonna get at least
>> one more revision
>> (https://lore.kernel.org/netdev/20240418125648.372526-1-Parthiban.Veerasooran@microchip.com/)
>>
>> I'm publishing this early as it could benefit some of the discussions in
>> the oa_tc6 threads, as well as giving other devs the possibility
>> massaging things to a state where they can use the rev.b1 chip (rev.b0
>> is eol).
>> And I need feedback on how to wrap this up.
>>
>> Far as I can tell the phy-driver cannot access some of the regs necessary
>> for probing the hardware and performing the init/fixup without going
>> over the spi interface.
>> The MMDCTRL register (used with indirect access) can address
>>
>> * PMA - mms 3
>> * PCS - mms 2
>> * Vendor specific / PLCA - mms 4
>>
>> This driver needs to access mms (memory map seleector)
>> * mac registers - mms 1,
>> * vendor specific / PLCA - mms 4
>> * vencor specific - mms 10
>>
>> Far as I can tell, mms 1 and 10 are only accessible via spi. In the
>> oa_tc6 patches this is enabled by the oa_tc6 framework by populating the
>> mdiobus->read/write_c45 funcs.
>>
>> In order to access any mms I needed I added the following change in the
>> oa_tc6.c module
>>
>> static int oa_tc6_get_phy_c45_mms(int devnum)
>>   {
>> +       if(devnum & BIT(31))
>> +               return devnum & GENMASK(30, 0);
>>
>> Which corresponds to the 'mms | BIT(31)' snippets in this commit, this
>> is really not how things should be handled, and I need input on how to
>> proceed here.
> 
> So if bit 31 of the devnum is set, then the other bits specify the
> MMS instead of the MMD.
> 
> I'm not sure we want to overload the PHY interface in this way. We
> have been down this path before with the MDIO bus read/write methods
> being used for both C22 and C45 accesses, and it created problems,
> so I don't think we want to repeat that mistake by doing the same
> thing here.
> 
> There's a comment in the original patches etc about the PHY being
> discovered via C22, and then not using the direct accesses to the
> C45 register space. I'm wondering whether we should split
> phydev->is_c45 to be phydev->probed_c45 / phydev->use_c45.
> 
> The former gets used during bus scanning and probe time to determine
> how we match the device driver to the phydev. The latter gets used
> _only_ to determine whether the read/write_mmd ops use direct mode
> or indirect mode.
> 
> Before the driver probe is called, we should do:
> 
>          phydev->use_mmd = phydev->probed_c45;
> 
> to ensure that todays behaviour is preserved. Then, provide a
> function, maybe, phy_use_direct_c45(phydev) which will set this
> phydev->use_mmd flag, and phy_use_indirect_c45(phydev) which will
> clear it.
> 
> phy_use_direct_c45() should also check whether the MDIO bus that
> is attached supports C45 access, and return an error if not.
> 
> That will give you the ability to use the direct access method
> where necessary.
> 
> There's a comment in the referred to code:
> 
> +       /* OPEN Alliance 10BASE-T1x compliance MAC-PHYs will have both C22 and
> +        * C45 registers space. If the PHY is discovered via C22 bus protocol it
> +        * assumes it uses C22 protocol and always uses C22 registers indirect
> +        * access to access C45 registers. This is because, we don't have a
> +        * clean separation between C22/C45 register space and C22/C45 MDIO bus
> +        * protocols. Resulting, PHY C45 registers direct access can't be used
> +        * which can save multiple SPI bus access. To support this feature, PHY
> +        * drivers can set .read_mmd/.write_mmd in the PHY driver to call
> +        * .read_c45/.write_c45. Ex: drivers/net/phy/microchip_t1s.c
> +        */
> 
> which I don't really understand. It claims that C45 direct access
> "saves" multiple SPI bus accesses. However, C45 indirect access
Sorry for the misunderstanding here. Probably I should have used "avoid" 
in the place of "save" might clear the things properly. The intention of 
this comment is, PHY C45 direct access will be faster than PHY C45 
indirect access as PHY C45 indirect access needs 4 SPI bus access.

If you agree, I will correct it in the next version.

Best regards,
Parthiban V
> requires:
> 
> 1. A C22 write to the MMD control register
> 2. A C22 write to the MMD data register
> 3. Another C22 write to the MMD control register
> 4. A c22 read or write to access the actual data.
> 
> Do four C22 bus transactions over SPI require more or less SPI bus
> accesses than a single C45 bus transaction over SPI? I suspect not,
> which makes the comment above factually incorrect.
> 
> If we have direct C45 access working, does that remove the need to
> have this special bit-31 to signal MMS access requirement?
> 
> Thanks.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net 0/1] phy: microchip_t1s: lan865x rev.b1 support

[Parthiban.Veerasooran]

Hi Ramon,

On 24/05/24 7:37 pm, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi,
> Let me first prepend this submission with 4 points:
> 
> * this is not in a merge-ready state
> * some code has been copied from the ongoing oa_tc6 work by Parthiban
> * this has to interop with code not yet merged (oa_tc6)
> * Microchip is looking into if rev.b0 can use the rev.b1 init procedure
I will try to get the info on this as soon as possible. I have asked for 
it to avoid the below complications only. I don't want to proceed with 
these patches until getting a clarity to avoid unnecessary confusions.

Best regards,
Parthiban V
> 
> The ongoing work by Parthiban Veerasooran is probably gonna get at least
> one more revision
> (https://lore.kernel.org/netdev/20240418125648.372526-1-Parthiban.Veerasooran@microchip.com/)
> 
> I'm publishing this early as it could benefit some of the discussions in
> the oa_tc6 threads, as well as giving other devs the possibility
> massaging things to a state where they can use the rev.b1 chip (rev.b0
> is eol).
> And I need feedback on how to wrap this up.
> 
> Far as I can tell the phy-driver cannot access some of the regs necessary
> for probing the hardware and performing the init/fixup without going
> over the spi interface.
> The MMDCTRL register (used with indirect access) can address
> 
> * PMA - mms 3
> * PCS - mms 2
> * Vendor specific / PLCA - mms 4
> 
> This driver needs to access mms (memory map seleector)
> * mac registers - mms 1,
> * vendor specific / PLCA - mms 4
> * vencor specific - mms 10
> 
> Far as I can tell, mms 1 and 10 are only accessible via spi. In the
> oa_tc6 patches this is enabled by the oa_tc6 framework by populating the
> mdiobus->read/write_c45 funcs.
> 
> In order to access any mms I needed I added the following change in the
> oa_tc6.c module
> 
> static int oa_tc6_get_phy_c45_mms(int devnum)
>   {
> +       if(devnum & BIT(31))
> +               return devnum & GENMASK(30, 0);
> 
> Which corresponds to the 'mms | BIT(31)' snippets in this commit, this
> is really not how things should be handled, and I need input on how to
> proceed here.
> 
> Here we get into a weird spot, this driver will need changes in the
> oa_tc6 submission, but it's weird to submit support for yet another phy
> with that patchset (in my opinion).
> 
> This has been tested with a lan8650 rev.b1 chip on one end and a lan8670
> usb eval board on the other end. Performance is rather lacking, the
> rev.b0 reaches close to the 10Mbit/s limit, but b.1 only gets about
> ~4Mbit/s, with the same results when PLCA enabled or disabled.
> 
> I suggest that this patch is left to brew until the oa_tc6 changes are
> accepted, at which time this is fixed up.
> 
> Ramón Nordin Rodriguez (1):
>    net: phy: microchip_t1s: enable lan865x revb1
> 
>   drivers/net/phy/microchip_t1s.c | 189 ++++++++++++++++++++++++++++----
>   1 file changed, 166 insertions(+), 23 deletions(-)
> 
> --
> 2.43.0
>