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

[Paul Kocialkowski <contact@paulk.fr>]

[-- Attachment #1: Type: text/plain, Size: 3590 bytes --]

Hi,

Le Mon 02 Jun 25, 01:40, Andre Przywara a écrit :
> 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/

Yeah I found it pretty odd that the implementation insists on read+write instead
of just normally building up the register. I'll do that in the next version.

And indeed with this version there would be a corner case where the bit is not
cleared. Even if it's unlikely, it's not good programming.

All the best,

Paul

> 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;
> 

-- 
Paul Kocialkowski,

Free software developer - https://www.paulk.fr/
Independent contractor - sys-base - https://www.sys-base.io/

Contributor to fully free software support for selected hardware.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]