Re: [PATCH net-next] net: phy: micrel: Add config_init for LAN8814

[Heiner Kallweit]

On 26.11.2021 11:38, Horatiu Vultur wrote:
> Add config_init for LAN8814. This function is required for the following
> reasons:
> - we need to make sure that the PHY is reset,
> - disable ANEG with QSGMII PCS Host side
> - swap the MDI-X A,B transmit so that there will not be any link flip-flaps
>   when the PHY gets a link.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
> ---
>  drivers/net/phy/micrel.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 44a24b99c894..f080312032cf 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -1565,6 +1565,14 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev,
>  #define LAN_EXT_PAGE_ACCESS_ADDRESS_DATA		0x17
>  #define LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC		0x4000
>  
> +#define LAN8814_QSGMII_SOFT_RESET			0x43
> +#define LAN8814_QSGMII_SOFT_RESET_BIT			BIT(0)
> +#define LAN8814_QSGMII_PCS1G_ANEG_CONFIG		0x13
> +#define LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA	BIT(3)
> +#define LAN8814_ALIGN_SWAP				0x4a
> +#define LAN8814_ALIGN_TX_A_B_SWAP			0x1
> +#define LAN8814_ALIGN_TX_A_B_SWAP_MASK			GENMASK(2, 0)
> +
>  #define LAN8804_ALIGN_SWAP				0x4a
>  #define LAN8804_ALIGN_TX_A_B_SWAP			0x1
>  #define LAN8804_ALIGN_TX_A_B_SWAP_MASK			GENMASK(2, 0)
> @@ -1601,6 +1609,29 @@ static int lanphy_write_page_reg(struct phy_device *phydev, int page, u16 addr,
>  	return 0;
>  }
>  
> +static int lan8814_config_init(struct phy_device *phydev)
> +{
> +	int val;
> +
> +	/* Reset the PHY */
> +	val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
> +	val |= LAN8814_QSGMII_SOFT_RESET_BIT;
> +	lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);
> +
> +	/* Disable ANEG with QSGMII PCS Host side */
> +	val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
> +	val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
> +	lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val);
> +
> +	/* MDI-X setting for swap A,B transmit */
> +	val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP);
> +	val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK;
> +	val |= LAN8814_ALIGN_TX_A_B_SWAP;
> +	lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val);
> +

Not directly related to just this patch:
Did you consider implementing the read_page and write_page PHY driver
callbacks? Then you could use phylib functions like phy_modify_paged et al
and you wouldn't have to open-code the paged register operations.

I think write_page would just be
phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page);
phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC));

and read_page
phy_read(phydev, LAN_EXT_PAGE_ACCESS_CONTROL);

> +	return 0;
> +}
> +
>  static int lan8804_config_init(struct phy_device *phydev)
>  {
>  	int val;
> @@ -1793,6 +1824,7 @@ static struct phy_driver ksphy_driver[] = {
>  	.phy_id		= PHY_ID_LAN8814,
>  	.phy_id_mask	= MICREL_PHY_ID_MASK,
>  	.name		= "Microchip INDY Gigabit Quad PHY",
> +	.config_init	= lan8814_config_init,
>  	.driver_data	= &ksz9021_type,
>  	.probe		= kszphy_probe,
>  	.soft_reset	= genphy_soft_reset,
>