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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux