Re: [PATCH net-next v2 2/3] net: phy: add Broadcom BCM54140 support

[Michael Walle]

Am 2020-04-19 18:49, schrieb Andrew Lunn:
> On Sun, Apr 19, 2020 at 06:33:40PM +0200, Michael Walle wrote:
> > >
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int bcm54140_config_init(struct phy_device *phydev)
> > > > +{
> > > > +	u16 reg = 0xffff;
> > > > +	int ret;
> > > > +
> > > > +	/* Apply hardware errata */
> > > > +	ret = bcm54140_b0_workaround(phydev);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/* Unmask events we are interested in. */
> > > > +	reg &= ~(BCM54140_RDB_INT_DUPLEX |
> > > > +		 BCM54140_RDB_INT_SPEED |
> > > > +		 BCM54140_RDB_INT_LINK);
> > > > +	ret = bcm_phy_write_rdb(phydev, BCM54140_RDB_IMR, reg);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	/* LED1=LINKSPD[1], LED2=LINKSPD[2], LED3=ACTIVITY */
> > > > +	ret = bcm_phy_modify_rdb(phydev, BCM54140_RDB_SPARE1,
> > > > +				 0, BCM54140_RDB_SPARE1_LSLM);
> > > > +	if (ret)
> > > > +		return ret;
> > >
> > > What are the reset default for LEDs? Can the LEDs be configured via
> > > strapping pins? There is currently no good solution for this. Whatever
> > > you pick will be wrong for somebody else. At minimum, strapping pins,
> > > if they exist, should not be overridden.
> >
> > Fair enough. There are no strapping options, just the "default 
> > behaviour",
> > where LED1/2 indicates the speed, and LED3 just activity (no link
> > indication). And I just noticed that in this case the comment above is
> > wrong, because it is actually link/activity. Further, there are myriad
> > configuration options which I didn't want to encode altogether. So 
> > I've
> > just chosen the typical one (which actually matches our hardware), ie.
> > to have the "activity/led mode". The application note mentions some 
> > other
> > concrete modes, but I don't know if its worth implementing them. Maybe 
> > we
> > can have a enumeration of some distinct modes? Ie.
> >
> >    broadcom,led-mode = <BCM54140_NO_CHANGE>;
> >    broadcom,led-mode = <BCM54140_ACT_LINK_MODE>;
>
> Configuring LEDs is a mess at the moment. No two PHYs do it the
> same. For a long time i've had a TODO item to make PHY LEDs work just
> like every other LED in linux, and you can set trigger actions which
> are then implemented in hardware.
>
> We have been pushing back on adding DT properties, it just makes the
> problem worse. If reset defaults are good enough for you, please leave
> it at that.

Unfortunately not. We need the link/act, which I presume will also be
used by most other users, thus the driver enables this setting. I don't
know any board which just have an activity led, that is just off and
blinks for a short time if there is RX or TX, which is the reset default
setting of this PHY.

-michael