Messages in this thread | | | Date | Sun, 19 Apr 2020 18:56:58 +0200 | From | Michael Walle <> | Subject | Re: [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
| |