Re: [PATCH 2/4] net: mediatek: sgmii: ensure the SGMII PHY is powered down on configuration

[Paolo Abeni <pabeni@redhat.com>]

On Sun, 2022-08-21 at 00:45 +0200, Alexander Couzens wrote:
> The code expect the PHY to be in power down which is only true after reset.
> Allow changes of the SGMII parameters more than once.
> 
> Signed-off-by: Alexander Couzens <lynxis@fe80.eu>
> ---
>  drivers/net/ethernet/mediatek/mtk_sgmii.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> index a01bb20ea957..782812434367 100644
> --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
> +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> @@ -7,6 +7,7 @@
>   *
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/of.h>
>  #include <linux/phylink.h>
> @@ -24,6 +25,9 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs)
>  {
>  	unsigned int val;
>  
> +	/* PHYA power down */
> +	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);

in mtk_pcs_setup_mode_an() and in mtk_pcs_setup_mode_force() the code
carefully flips only the SGMII_PHYA_PWD bit. Is it safe to overwrite
the full register contents?

> +
>  	/* Setup the link timer and QPHY power up inside SGMIISYS */
>  	regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER,
>  		     SGMII_LINK_TIMER_DEFAULT);
> @@ -36,6 +40,10 @@ static int mtk_pcs_setup_mode_an(struct mtk_pcs *mpcs)
>  	val |= SGMII_AN_RESTART;
>  	regmap_write(mpcs->regmap, SGMSYS_PCS_CONTROL_1, val);
>  
> +	/* Release PHYA power down state
> +	 * unknown how much the QPHY needs but it is racy without a sleep
> +	 */
> +	usleep_range(50, 100);

Ouch, this looks fragile, without any related H/W specification. 

>  	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
>  
>  	return 0;
> @@ -50,6 +58,9 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs,
>  {
>  	unsigned int val;
>  
> +	/* PHYA power down */
> +	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, SGMII_PHYA_PWD);
> +
>  	regmap_read(mpcs->regmap, mpcs->ana_rgc3, &val);
>  	val &= ~RG_PHY_SPEED_MASK;
>  	if (interface == PHY_INTERFACE_MODE_2500BASEX)
> @@ -67,7 +78,10 @@ static int mtk_pcs_setup_mode_force(struct mtk_pcs *mpcs,
>  	val |= SGMII_SPEED_1000;
>  	regmap_write(mpcs->regmap, SGMSYS_SGMII_MODE, val);
>  
> -	/* Release PHYA power down state */
> +	/* Release PHYA power down state
> +	 * unknown how much the QPHY needs but it is racy without a sleep
> +	 */
> +	usleep_range(50, 100);

Same here.

Thanks!

Paolo