Message-ID: <402780af-9d12-45dd-e435-e7279f1b9263@gmail.com>
Date:   Fri, 26 Nov 2021 12:57:33 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.2
Content-Language: en-US
To:     Horatiu Vultur <horatiu.vultur@microchip.com>, andrew@lunn.ch,
        linux@armlinux.org.uk, davem@davemloft.net, kuba@kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20211126103833.3609945-1-horatiu.vultur@microchip.com>
From:   Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: [PATCH net-next] net: phy: micrel: Add config_init for LAN8814
In-Reply-To: <20211126103833.3609945-1-horatiu.vultur@microchip.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@microchip.com>
> ---
>  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,
>