Re: [PATCH 6/6] net: sun8i-emac: Add support for active-low leds with internal PHY

[Andre Przywara <andre.przywara@arm.com>]

On Sun,  1 Jun 2025 17:39:43 +0200
Paul Kocialkowski <contact@paulk.fr> wrote:

Hi,

> A device-tree property is already defined to indicate that the internal
> PHY should be used with active-low leds, which corresponds to a
> specific bit in the dedicated syscon register.
> 
> Add support for setting this bit when the property is present.
> 
> Signed-off-by: Paul Kocialkowski <contact@paulk.fr>
> ---
>  drivers/net/sun8i_emac.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/sun8i_emac.c b/drivers/net/sun8i_emac.c
> index 8433e7db2654..990a184e4b1f 100644
> --- a/drivers/net/sun8i_emac.c
> +++ b/drivers/net/sun8i_emac.c
> @@ -176,6 +176,7 @@ struct sun8i_eth_pdata {
>  	u32 reset_delays[3];
>  	int tx_delay_ps;
>  	int rx_delay_ps;
> +	bool leds_active_low;
>  };
>  
>  static int sun8i_mdio_read(struct mii_dev *bus, int addr, int devad, int reg)
> @@ -287,7 +288,8 @@ static void sun8i_adjust_link(struct emac_eth_dev *priv,
>  	writel(v, priv->mac_reg + EMAC_CTL0);
>  }
>  
> -static u32 sun8i_emac_set_syscon_ephy(struct emac_eth_dev *priv, u32 reg)
> +static u32 sun8i_emac_set_syscon_ephy(struct sun8i_eth_pdata *pdata,
> +				      struct emac_eth_dev *priv, u32 reg)
>  {
>  	if (priv->use_internal_phy) {
>  		/* H3 based SoC's that has an Internal 100MBit PHY
> @@ -295,6 +297,10 @@ static u32 sun8i_emac_set_syscon_ephy(struct emac_eth_dev *priv, u32 reg)
>  		*/
>  		reg &= ~H3_EPHY_DEFAULT_MASK;
>  		reg |=  H3_EPHY_DEFAULT_VALUE;
> +
> +		if (pdata->leds_active_low)
> +			reg |= H3_EPHY_LED_POL;

That might be nitpicking, since it worked before, but I wonder if we
should either explicitly clear the bit in an else branch, or follow the
recent Linux change to build the register up from scratch:
https://lore.kernel.org/linux-sunxi/20250423095222.1517507-1-andre.przywara@arm.com/

Cheers,
Andre

> +
>  		reg |= priv->phyaddr << H3_EPHY_ADDR_SHIFT;
>  		reg &= ~H3_EPHY_SHUTDOWN;
>  		return reg | H3_EPHY_SELECT;
> @@ -314,7 +320,7 @@ static int sun8i_emac_set_syscon(struct sun8i_eth_pdata *pdata,
>  
>  	reg = readl(priv->sysctl_reg);
>  
> -	reg = sun8i_emac_set_syscon_ephy(priv, reg);
> +	reg = sun8i_emac_set_syscon_ephy(pdata, priv, reg);
>  
>  	reg &= ~(SC_ETCS_MASK | SC_EPIT);
>  	if (priv->variant->support_rmii)
> @@ -859,6 +865,10 @@ static int sun8i_emac_eth_of_to_plat(struct udevice *dev)
>  		printf("%s: Invalid RX delay value %d\n", __func__,
>  		       sun8i_pdata->rx_delay_ps);
>  
> +	sun8i_pdata->leds_active_low =
> +		fdtdec_get_bool(gd->fdt_blob, dev_of_offset(dev),
> +				"allwinner,leds-active-low");
> +
>  	if (fdtdec_get_bool(gd->fdt_blob, dev_of_offset(dev),
>  			    "snps,reset-active-low"))
>  		reset_flags |= GPIOD_ACTIVE_LOW;