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

[Michael Walle <michael@walle.cc>]

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