Message-ID: <ZEmPT1El342j7O8I@debian>
Date:   Wed, 26 Apr 2023 22:53:35 +0200
From:   Ramón Nordin Rodriguez 
        <ramon.nordin.rodriguez@...roamp.se>
To:     Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>
Cc:     netdev@...r.kernel.org, andrew@...n.ch, davem@...emloft.net,
        jan.huber@...rochip.com, thorsten.kummermehr@...rochip.com
Subject: Re: [PATCH net-next 2/2] net: phy: microchip_t1s: add support for
 Microchip LAN865x Rev.B0 PHYs

On Wed, Apr 26, 2023 at 05:16:55PM +0530, Parthiban Veerasooran wrote:
> This patch adds support for the Microchip LAN865x Rev.B0 10BASE-T1S
> Internal PHYs (LAN8650/1). The LAN865x combines a Media Access Controller
> (MAC) and an internal 10BASE-T1S Ethernet PHY to access 10BASE‑T1S
> networks.
> 
> Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>
> ---
>  drivers/net/phy/microchip_t1s.c | 200 ++++++++++++++++++++++++++++++--
>  1 file changed, 191 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index 793fb0210605..3d8d285b3c34 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c
> @@ -4,6 +4,7 @@
>   *
>   * Support: Microchip Phys:
>   *  lan8670/1/2 Rev.B1
> + *  lan8650/1 Rev.B0 Internal PHYs
>   */
>  
>  #include <linux/kernel.h>
> @@ -11,9 +12,10 @@
>  #include <linux/phy.h>
>  
>  #define PHY_ID_LAN867X_REVB1 0x0007C162
> +#define PHY_ID_LAN865X_REVB0 0x0007C1B3
>  
> -#define LAN867X_REG_IRQ_1_CTL 0x001C
> -#define LAN867X_REG_IRQ_2_CTL 0x001D
> +#define LAN86XX_REG_IRQ_1_CTL 0x001C
> +#define LAN86XX_REG_IRQ_2_CTL 0x001D
>  
>  /* The arrays below are pulled from the following table from AN1699
>   * Access MMD Address Value Mask
> @@ -49,6 +51,174 @@ static const int lan867x_revb1_fixup_masks[12] = {
>  	0x0600, 0x7F00, 0x2000, 0xFFFF,
>  };
>  
> +/* LAN865x Rev.B0 configuration parameters from AN1760
> + */
> +static const int lan865x_revb0_fixup_registers[28] = {
> +	0x0091, 0x0081, 0x0043, 0x0044,
> +	0x0045, 0x0053, 0x0054, 0x0055,
> +	0x0040, 0x0050, 0x00D0, 0x00E9,
> +	0x00F5, 0x00F4, 0x00F8, 0x00F9,
> +	0x00B0, 0x00B1, 0x00B2, 0x00B3,
> +	0x00B4, 0x00B5, 0x00B6, 0x00B7,
> +	0x00B8, 0x00B9, 0x00BA, 0x00BB,
> +};
> +
> +static const int lan865x_revb0_fixup_values[28] = {
> +	0x9660, 0x00C0, 0x00FF, 0xFFFF,
> +	0x0000, 0x00FF, 0xFFFF, 0x0000,
> +	0x0002, 0x0002, 0x5F21, 0x9E50,
> +	0x1CF8, 0xC020, 0x9B00, 0x4E53,
> +	0x0103, 0x0910, 0x1D26, 0x002A,
> +	0x0103, 0x070D, 0x1720, 0x0027,
> +	0x0509, 0x0E13, 0x1C25, 0x002B,
> +};
> +
> +/* indirect read pseudocode from AN1760
> + * write_register(0x4, 0x00D8, addr)
> + * write_register(0x4, 0x00DA, 0x2)
> + * return (int8)(read_register(0x4, 0x00D9))
> + */

I suggest this comment block is slightly changed to

/* Pulled from AN1760 describing 'indirect read'
 *
 * write_register(0x4, 0x00D8, addr)
 * write_register(0x4, 0x00DA, 0x2)
 * return (int8)(read_register(0x4, 0x00D9))
 *
 * 0x4 refers to memory map selector 4, which maps to MDIO_MMD_VEND2
 */

> +static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr)
> +{
> +	int ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xD8, addr);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xDA, 0x0002);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0xD9);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ret;
> +}

The func itself might get a bit more readable by naming the magic regs
and value, below is a suggestion.

static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr)
{
	int ret;
	static const u16 fixup_w0_reg = 0x00D8;
	static const u16 fixup_r0_reg = 0x00D9;
	static const u16 fixup_w1_val = 0x0002;
	static const u16 fixup_w1_reg = 0x00DA;

	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, fixup_w0_reg, addr);
	if (ret)
		return ret;

	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, fixup_w1_reg, fixup_w1_val);
	if (ret)
		return ret;

	return phy_read_mmd(phydev, MDIO_MMD_VEND2, fixup_r0_reg);
}

> +
> +static int lan865x_setup_cfgparam(struct phy_device *phydev)
> +{
> +	s8 offset1;
> +	s8 offset2;
> +	s8 value;
> +	u16 cfgparam;
> +	int ret;
> +
> +	ret = lan865x_revb0_indirect_read(phydev, 0x0004);
> +	if (ret < 0)
> +		return ret;
> +	value = (s8)ret;
> +	/* Calculation of configuration offset 1 from AN1760
> +	 */
> +	if ((value & 0x10) != 0)
> +		offset1 = value | 0xE0;
> +	else
> +		offset1 = value;
> +
> +	ret = lan865x_revb0_indirect_read(phydev, 0x0008);
> +	if (ret < 0)
> +		return ret;
> +	value = (s8)ret;
> +	/* Calculation of configuration offset 2 from AN1760
> +	 */
> +	if ((value & 0x10) != 0)
> +		offset2 = value | 0xE0;
> +	else
> +		offset2 = value;
> +
> +	/* calculate and write the configuration parameters in the
> +	 * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760)
> +	 */
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x0084);
> +	if (ret < 0)
> +		return ret;
> +	cfgparam = (ret & 0xF) | (((9 + offset1) << 10) |
> +		    ((14 + offset1) << 4));
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x84, cfgparam);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x008A);
> +	if (ret < 0)
> +		return ret;
> +	cfgparam = (ret & 0x3FF) | ((40 + offset2) << 10);
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x8A, cfgparam);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AD);
> +	if (ret < 0)
> +		return ret;
> +	cfgparam = (ret & 0xC0C0) | (((5 + offset1) << 8) | (9 + offset1));
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAD, cfgparam);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AE);
> +	if (ret < 0)
> +		return ret;
> +	cfgparam = (ret & 0xC0C0) | (((9 + offset1) << 8) | (14 + offset1));
> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAE, cfgparam);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0x00AF);
> +	if (ret < 0)
> +		return ret;
> +	cfgparam = (ret & 0xC0C0) | (((17 + offset1) << 8) | (22 + offset1));
> +	return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xAF, cfgparam);
> +}
> +

I suggest this func is broken up into multiple funcs, for the sake of
readability. My suggestion is less efficient and would require
allocating some arrays which might not be an ideal solution, but then
readability would be my primary concern here.

//this array should be placed at the top of the file with the other arrs
static const u16 lan865x_revb0_fixup_cfg_regs[5] = { 0x84, 0x8A, 0xAD, 0xAE, 0xAF, };

/* This is pulled straigt from AN1760 from 'calulation of offset 1' & 'calculation of offset 2'
 */
static int lan865x_generate_cfg_offsets(struct phy_device *phydev, s8 offsets[2])
{
	u16 fixup_regs[2] = {0x0004, 0x0008};
	int val;

	for (int i = 0; i < ARRAY_SIZE(fixup_regs); i++) {
		val = (s8)lan865x_revb0_indirect_read(phydev, fixup_regs[i]);
		if (val < 0)
			return val;
		if ((val & 0x10) != 0)
			offsets[i] = val | 0xE0;
		else
			offsets[i] = val;
	}

	return 0;
}

static int lan865x_read_cfg_params(struct phy_device *phydev, s16 cfg_params[5])
{
	int err;

	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs); i++) {
		err = phy_read_mmd(phydev, MDIO_MMD_VEND2, lan865x_revb0_fixup_cfg_regs[i]);
		if (err < 0)
			return err;
		cfg_params[i] = (u16)err;
	}

	return 0;
}

static int lan865x_write_cfg_params(struct phy_device *phydev, u16 cfg_params[5])
{
	int err;

	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_cfg_regs); i++) {
		err = phy_write_mmd(phydev, MDIO_MMD_VEND2, lan865x_revb0_fixup_cfg_regs[i], cfg_params[i]);
		if (err < 0)
			return err;
	}

	return 0;
}

static int lan865x_setup_cfgparam(struct phy_device *phydev)
{
	u16 cfg_results[5];
	u16 cfg_params[5];
	s8 offsets[2];
	int err;

	err = lan865x_generate_cfg_offsets(phydev, offsets);
	if (err < 0)
		return err;
	err = lan865x_read_cfg_params(phydev, cfg_params);
	if (err < 0)
		return err;

	// This block computes the magic values that goes into the magic cfg regs.
	// Maybe there is some way of compacting this into a loop, but this seems like
	// as readable as it's going to get.
	cfg_results[0] = (cfg_params[0] & 0xF) | (((9 + offsets[0]) << 10) |
		    ((14 + offsets[0]) << 4));
	cfg_results[1] = (cfg_params[1] & 0x3FF) | ((40 + offsets[1]) << 10);
	cfg_results[2] = (cfg_params[2] & 0xC0C0) | (((5 + offsets[0]) << 8) | (9 + offsets[0]));
	cfg_results[3] = (cfg_params[3] & 0xC0C0) | (((9 + offsets[0]) << 8) | (14 + offsets[0]));
	cfg_results[4] = (cfg_params[4] & 0xC0C0) | (((17 + offsets[0]) << 8) | (22 + offsets[0]));

	return lan865x_write_cfg_params(phydev, cfg_results);
}

> +static int lan865x_revb0_config_init(struct phy_device *phydev)
> +{
> +	int addr;
> +	int value;
> +	int ret;
> +
> +	/* As per AN1760, the below configuration applies only to the LAN8650/1
> +	 * hardware revision Rev.B0.
> +	 */

I think this is implied by having it the device specific init func, you
can probably drop this comment.

> +	for (int i = 0; i < ARRAY_SIZE(lan865x_revb0_fixup_registers); i++) {
> +		addr = lan865x_revb0_fixup_registers[i];
> +		value = lan865x_revb0_fixup_values[i];
> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, addr, value);
> +		if (ret)
> +			return ret;
> +	}
> +	/* function to calculate and write the configuration parameters in the
> +	 * 0x0084, 0x008A, 0x00AD, 0x00AE and 0x00AF registers (from AN1760)
> +	 */
> +	ret = lan865x_setup_cfgparam(phydev);
> +	if (ret < 0)
> +		return ret;

The loop and the call to lan865x_setup_cfgparam deviates from AN1760, in
the AN the writes to the cfg regs are mixed into the writes to the fixup
regs. Is this really intended and safe?
I'm guessing this is done out of convenience, if so I'd suggest adding a
comment along the lines

// The writes to the fixup and cfg regs deviate from the recommendation
// in AN1760, where the writes are intermixed. This is done out of
// convenience but works

I'm guessing you've tested it :)

> +
> +	/* disable all the interrupts
> +	 */

Maybe a motivation about why would be helpful here

> +	ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF);
> +	if (ret)
> +		return ret;
> +	return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF);
> +}
> +
> +static int lan865x_revb0_plca_set_cfg(struct phy_device *phydev,
> +				      const struct phy_plca_cfg *plca_cfg)
> +{
> +	int ret;
> +
> +	ret = genphy_c45_plca_set_cfg(phydev, plca_cfg);
> +	if (ret)
> +		return ret;
> +
> +	/* Disable the collision detection when PLCA is enabled and enable
> +	 * collision detection when CSMA/CD mode is enabled.
> +	 */
> +	if (plca_cfg->enabled)
> +		return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0000);
> +	else
> +		return phy_write_mmd(phydev, MDIO_MMD_VEND2, 0x0087, 0x0083);

This register is marked as reserved in the datasheet, is this the
intended reg?
If it is intended a more detailed comment about what it does would be
nice, since no one outside microchip will be able to verify it.
Also is something similar relevant for the lan867x phys?

> +}
> +
>  static int lan867x_revb1_config_init(struct phy_device *phydev)
>  {
>  	/* HW quirk: Microchip states in the application note (AN1699) for the phy
> @@ -90,13 +260,13 @@ static int lan867x_revb1_config_init(struct phy_device *phydev)
>  	 * for it either.
>  	 * So we'll just disable all interrupts on the chip.
>  	 */
> -	err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_1_CTL, 0xFFFF);
> +	err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_1_CTL, 0xFFFF);
>  	if (err != 0)
>  		return err;
> -	return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_2_CTL, 0xFFFF);
> +	return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN86XX_REG_IRQ_2_CTL, 0xFFFF);
>  }
>  
> -static int lan867x_read_status(struct phy_device *phydev)
> +static int lan86xx_read_status(struct phy_device *phydev)
>  {
>  	/* The phy has some limitations, namely:
>  	 *  - always reports link up
> @@ -111,23 +281,34 @@ static int lan867x_read_status(struct phy_device *phydev)
>  	return 0;
>  }
>  
> -static struct phy_driver lan867x_revb1_driver[] = {
> +static struct phy_driver lan86xx_driver[] = {
>  	{
>  		PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1),
>  		.name               = "LAN867X Rev.B1",
>  		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
>  		.config_init        = lan867x_revb1_config_init,
> -		.read_status        = lan867x_read_status,
> +		.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",
> +		.features           = PHY_BASIC_T1S_P2MP_FEATURES,
> +		.config_init        = lan865x_revb0_config_init,
> +		.read_status        = lan86xx_read_status,
> +		.get_plca_cfg	    = genphy_c45_plca_get_cfg,
> +		.set_plca_cfg	    = lan865x_revb0_plca_set_cfg,
> +		.get_plca_status    = genphy_c45_plca_get_status,
> +	},
>  };
>  
> -module_phy_driver(lan867x_revb1_driver);
> +module_phy_driver(lan86xx_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) },
>  	{ }
>  };
>  
> @@ -135,4 +316,5 @@ MODULE_DEVICE_TABLE(mdio, tbl);
>  
>  MODULE_DESCRIPTION("Microchip 10BASE-T1S Phy driver");
>  MODULE_AUTHOR("Ramón Nordin Rodriguez");
> +MODULE_AUTHOR("Parthiban Veerasooran <parthiban.veerasooran@...rochip.com>");
>  MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists