Hi,
Le Mon 02 Jun 25, 01:40, Andre Przywara a écrit :
> On Sun, 1 Jun 2025 17:39:43 +0200
> Paul Kocialkowski <cont...@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 <cont...@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.przyw...@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.
signature.asc
Description: PGP signature
