lkml.org 
[lkml]   [2020]   [Apr]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH net-next v2 2/3] net: phy: add Broadcom BCM54140 support
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

\
 
 \ /
  Last update: 2020-04-19 18:58    [W:0.072 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site