Re: APU1 ethernet LEDs
On 2016/04/19 18:14, Stuart Henderson wrote:
> On 2016/04/19 18:57, Christian Weisgerber wrote:
> > Stuart Henderson:
> >
> > > Looking at unique "RTL8168E/8111E (0x2c00)" entries from dmesglog
> > > back to Feb 2013, there are 7 APUs (=21 NICs), and 20 non-APUs.
> > > Do we care if we change led state for those others too? We could
> > > check the MAC vendor for 00:0d:b9, but I think this is unnecessary
> > > complexity (and who knows, maybe it's an improvement for some of
> > > those too).
> >
> > Checking the MAC address is brittle since it can be changed (ifconfig
> > lladdr). Instead, I suggest checking hw_vendor and hw_prod; see
> > for instance what skgpio(4) does. I don't think blindly reprogramming
> > the LED state for all chips of this type is acceptable.
>
> Ah that's not a bad idea, I'll take a look. I'll need a tester for
> that as the board I was using is in production now.
Done in diff below. I switched to one led for link, one flashing
on for activity as I think the majority of people who commented
preferred that over having indication of gig link.
Whether or not this goes in, I'll be using some variant of this
for remote boxes using this board, there's too much risk of people
thinking the box is broken and trying to power-cycle to "fix" it
otherwise.
textdatabss dec hex
19112 32 0 19144 4ac8re.o
19287 32 0 19319 4b77re.o
(obviously it's larger with the strcmp).
Index: re.c
===
RCS file: /cvs/src/sys/dev/ic/re.c,v
retrieving revision 1.191
diff -u -p -r1.191 re.c
--- re.c13 Apr 2016 10:49:26 - 1.191
+++ re.c19 Apr 2016 20:22:03 -
@@ -198,6 +198,8 @@ struct cfdriver re_cd = {
0, "re", DV_IFNET
};
+extern char *hw_vendor, *hw_prod;
+
#define EE_SET(x) \
CSR_WRITE_1(sc, RL_EECMD, \
CSR_READ_1(sc, RL_EECMD) | x)
@@ -1940,6 +1942,21 @@ re_init(struct ifnet *ifp)
htole32(*(u_int32_t *)(&eaddr.eaddr[4])));
CSR_WRITE_4(sc, RL_IDR0,
htole32(*(u_int32_t *)(&eaddr.eaddr[0])));
+ /*
+* Default on PC Engines APU1 is to have all LEDs off unless
+* there is network activity. Override to provide a link status
+* LED.
+*/
+ if (sc->sc_hwrev == RL_HWREV_8168E &&
+ hw_vendor != NULL && hw_prod != NULL &&
+ strcmp(hw_vendor, "PC Engines") == 0 &&
+ strcmp(hw_prod, "APU") == 0) {
+ CSR_SETBIT_1(sc, RL_CFG4, RL_CFG4_CUSTOM_LED);
+ CSR_WRITE_1(sc, RL_LEDSEL, RL_LED_LINK | RL_LED_ACT << 4);
+ }
+ /*
+* Protect config register again
+*/
CSR_WRITE_1(sc, RL_EECMD, RL_EEMODE_OFF);
if ((sc->rl_flags & RL_FLAG_JUMBOV2) != 0)
Index: rtl81x9reg.h
===
RCS file: /cvs/src/sys/dev/ic/rtl81x9reg.h,v
retrieving revision 1.97
diff -u -p -r1.97 rtl81x9reg.h
--- rtl81x9reg.h28 Dec 2015 05:49:15 - 1.97
+++ rtl81x9reg.h19 Apr 2016 20:22:03 -
@@ -153,6 +153,13 @@
#define RL_MISC0x00F0
/*
+ * Register used on RTL8111E
+ */
+#define RL_LEDSEL 0x0018
+#define RL_LED_LINK0x7 /* link at any speed */
+#define RL_LED_ACT 0x8
+
+/*
* TX config register bits
*/
#define RL_TXCFG_CLRABRT 0x0001 /* retransmit aborted pkt */
@@ -449,6 +456,7 @@
/*
* Config 4 register
*/
+#define RL_CFG4_CUSTOM_LED 0x40
#define RL_CFG4_LWPTN 0x04
#define RL_CFG4_LWPME 0x10
#define RL_CFG4_JUMBO_EN1 0x02
Re: APU1 ethernet LEDs
> Date: Tue, 19 Apr 2016 18:57:47 +0200 > From: Christian Weisgerber > > Stuart Henderson: > > > Looking at unique "RTL8168E/8111E (0x2c00)" entries from dmesglog > > back to Feb 2013, there are 7 APUs (=21 NICs), and 20 non-APUs. > > Do we care if we change led state for those others too? We could > > check the MAC vendor for 00:0d:b9, but I think this is unnecessary > > complexity (and who knows, maybe it's an improvement for some of > > those too). > > Checking the MAC address is brittle since it can be changed (ifconfig > lladdr). Instead, I suggest checking hw_vendor and hw_prod; see > for instance what skgpio(4) does. I don't think blindly reprogramming > the LED state for all chips of this type is acceptable. > > Ideally, Pascal should fix this in the BIOS. :-( To be honest, I don't think we should bloat our kernel with workarounds for cosmetic issues like this.
Re: APU1 ethernet LEDs
> > Ideally, Pascal should fix this in the BIOS. :-( > > Ideally it would have been found/fixed during beta - I'm not sure if > I'd be massively happy about the BIOS reprogramming the eeprom (which > the user might have already modified). If I were him I wouldn't want > to do more than providing a bootable OS image that people can run > to do it.. Ideally. Maybe next time
Re: APU1 ethernet LEDs
On 2016/04/19 18:57, Christian Weisgerber wrote: > Stuart Henderson: > > > Looking at unique "RTL8168E/8111E (0x2c00)" entries from dmesglog > > back to Feb 2013, there are 7 APUs (=21 NICs), and 20 non-APUs. > > Do we care if we change led state for those others too? We could > > check the MAC vendor for 00:0d:b9, but I think this is unnecessary > > complexity (and who knows, maybe it's an improvement for some of > > those too). > > Checking the MAC address is brittle since it can be changed (ifconfig > lladdr). Instead, I suggest checking hw_vendor and hw_prod; see > for instance what skgpio(4) does. I don't think blindly reprogramming > the LED state for all chips of this type is acceptable. Ah that's not a bad idea, I'll take a look. I'll need a tester for that as the board I was using is in production now. > Ideally, Pascal should fix this in the BIOS. :-( Ideally it would have been found/fixed during beta - I'm not sure if I'd be massively happy about the BIOS reprogramming the eeprom (which the user might have already modified). If I were him I wouldn't want to do more than providing a bootable OS image that people can run to do it..
Re: APU1 ethernet LEDs
> Ideally, Pascal should fix this in the BIOS. :-( I encourage that direction.
Re: APU1 ethernet LEDs
Stuart Henderson: > Looking at unique "RTL8168E/8111E (0x2c00)" entries from dmesglog > back to Feb 2013, there are 7 APUs (=21 NICs), and 20 non-APUs. > Do we care if we change led state for those others too? We could > check the MAC vendor for 00:0d:b9, but I think this is unnecessary > complexity (and who knows, maybe it's an improvement for some of > those too). Checking the MAC address is brittle since it can be changed (ifconfig lladdr). Instead, I suggest checking hw_vendor and hw_prod; see for instance what skgpio(4) does. I don't think blindly reprogramming the LED state for all chips of this type is acceptable. Ideally, Pascal should fix this in the BIOS. :-( -- Christian "naddy" Weisgerber na...@mips.inka.de
Re: APU1 ethernet LEDs
On 2016/04/12 14:31, Chris Cappuccio wrote: > Did you bring this up to Pascal Dornier ? I think he would rather fix > the eeprom... No, but it came up a few times on the forums and, assuming he's logging in as "support" there, he knows about it: http://www.pcengines.info/forums/?page=post&id=1B5B7F52-95CF-4AEE-AC35-BC6C9F96CB35&fid=DF5ACB70-99C4-4C61-AFA6-4C0E0DB05B2A&cachecommand=bypass&pageindex=3 http://www.pcengines.info/forums/?page=post&id=BB38BE76-C8E9-4227-A0B5-A165EE1BD245&fid=DF5ACB70-99C4-4C61-AFA6-4C0E0DB05B2A > This is specific to the hardware layout, the eeprom is the right place, > not the driver! Right, but that wouldn't help existing boards unless something was added (to bios?) to reprogramme the eeprom, which seems more complicated/risky than I'd choose to do in his position.. > On the other hand, I think the CSR_WRITE_1 is perfectly clear > > Stuart Henderson [s...@spacehopper.org] wrote: > > The re(4) variant on the PC Engines APU1, RTL8111E, has a flexible LED > > configuration. The nic's default config seems to be intended for > > controlling 3 LEDs (or one single-colour and a bi-colour LED with > > different colours to indicate link speed) however the ethernet ports > > on the APU1 only have two LEDs (one green one amber). > > > > Defaults can be changed in eeprom but (intentionally or not) this > > hasn't been done on the APU1 so in normal situations with a 1GB link, > > you only ever see one lit LED, and even that is normally off, only > > blinking for activity. I don't know of any APU users who like > > the default setup. > > > > Section 6.2.6 of the RTL8111E-VB-GR / -VB-CG / -VC-CG datasheet > > tells us how to reprogramme it. Do we want to change it? > > > > Without diff: > > > > green: normally off, blink on for activity. > > amber: on solid for 100MB link, off for 1GB link. > > > > With diff: > > > > green: normally on if link at any speed. blink off for activity. > > amber: on solid for 1GB link, otherwise off. > > > > Some other settings are possible but I think this gives the best > > combination of information under "normal" conditions given only > > 2 LEDs. > > > > Looking at unique "RTL8168E/8111E (0x2c00)" entries from dmesglog > > back to Feb 2013, there are 7 APUs (=21 NICs), and 20 non-APUs. > > Do we care if we change led state for those others too? We could > > check the MAC vendor for 00:0d:b9, but I think this is unnecessary > > complexity (and who knows, maybe it's an improvement for some of > > those too). > > > > Any comments/OKs? (I am not 100% happy with the clarity of the new > > CSR_WRITE_1 line, but my earlier iterations were worse ;) > > >
Re: APU1 ethernet LEDs
Did you bring this up to Pascal Dornier ? I think he would rather fix the eeprom... This is specific to the hardware layout, the eeprom is the right place, not the driver! On the other hand, I think the CSR_WRITE_1 is perfectly clear Stuart Henderson [s...@spacehopper.org] wrote: > The re(4) variant on the PC Engines APU1, RTL8111E, has a flexible LED > configuration. The nic's default config seems to be intended for > controlling 3 LEDs (or one single-colour and a bi-colour LED with > different colours to indicate link speed) however the ethernet ports > on the APU1 only have two LEDs (one green one amber). > > Defaults can be changed in eeprom but (intentionally or not) this > hasn't been done on the APU1 so in normal situations with a 1GB link, > you only ever see one lit LED, and even that is normally off, only > blinking for activity. I don't know of any APU users who like > the default setup. > > Section 6.2.6 of the RTL8111E-VB-GR / -VB-CG / -VC-CG datasheet > tells us how to reprogramme it. Do we want to change it? > > Without diff: > > green: normally off, blink on for activity. > amber: on solid for 100MB link, off for 1GB link. > > With diff: > > green: normally on if link at any speed. blink off for activity. > amber: on solid for 1GB link, otherwise off. > > Some other settings are possible but I think this gives the best > combination of information under "normal" conditions given only > 2 LEDs. > > Looking at unique "RTL8168E/8111E (0x2c00)" entries from dmesglog > back to Feb 2013, there are 7 APUs (=21 NICs), and 20 non-APUs. > Do we care if we change led state for those others too? We could > check the MAC vendor for 00:0d:b9, but I think this is unnecessary > complexity (and who knows, maybe it's an improvement for some of > those too). > > Any comments/OKs? (I am not 100% happy with the clarity of the new > CSR_WRITE_1 line, but my earlier iterations were worse ;) >
APU1 ethernet LEDs
The re(4) variant on the PC Engines APU1, RTL8111E, has a flexible LED
configuration. The nic's default config seems to be intended for
controlling 3 LEDs (or one single-colour and a bi-colour LED with
different colours to indicate link speed) however the ethernet ports
on the APU1 only have two LEDs (one green one amber).
Defaults can be changed in eeprom but (intentionally or not) this
hasn't been done on the APU1 so in normal situations with a 1GB link,
you only ever see one lit LED, and even that is normally off, only
blinking for activity. I don't know of any APU users who like
the default setup.
Section 6.2.6 of the RTL8111E-VB-GR / -VB-CG / -VC-CG datasheet
tells us how to reprogramme it. Do we want to change it?
Without diff:
green: normally off, blink on for activity.
amber: on solid for 100MB link, off for 1GB link.
With diff:
green: normally on if link at any speed. blink off for activity.
amber: on solid for 1GB link, otherwise off.
Some other settings are possible but I think this gives the best
combination of information under "normal" conditions given only
2 LEDs.
Looking at unique "RTL8168E/8111E (0x2c00)" entries from dmesglog
back to Feb 2013, there are 7 APUs (=21 NICs), and 20 non-APUs.
Do we care if we change led state for those others too? We could
check the MAC vendor for 00:0d:b9, but I think this is unnecessary
complexity (and who knows, maybe it's an improvement for some of
those too).
Any comments/OKs? (I am not 100% happy with the clarity of the new
CSR_WRITE_1 line, but my earlier iterations were worse ;)
Index: re.c
===
RCS file: /cvs/src/sys/dev/ic/re.c,v
retrieving revision 1.190
diff -u -p -r1.190 re.c
--- re.c19 Mar 2016 11:34:22 - 1.190
+++ re.c12 Apr 2016 21:09:53 -
@@ -1941,6 +1941,18 @@ re_init(struct ifnet *ifp)
htole32(*(u_int32_t *)(&eaddr.eaddr[4])));
CSR_WRITE_4(sc, RL_IDR0,
htole32(*(u_int32_t *)(&eaddr.eaddr[0])));
+ /*
+* Programme LEDs on 8168E/8111E to avoid poor default on
+* PC Engines APU1.
+*/
+ if (sc->sc_hwrev == RL_HWREV_8168E) {
+ CSR_SETBIT_1(sc, RL_CFG4, RL_CFG4_CUSTOM_LED);
+ CSR_WRITE_1(sc, RL_LEDSEL, RL_LED_ACT | RL_LED_LINK1000 |
+ RL_LED_LINK << 4);
+ }
+ /*
+* Protect config register again
+*/
CSR_WRITE_1(sc, RL_EECMD, RL_EEMODE_OFF);
if ((sc->rl_flags & RL_FLAG_JUMBOV2) != 0)
Index: rtl81x9reg.h
===
RCS file: /cvs/src/sys/dev/ic/rtl81x9reg.h,v
retrieving revision 1.97
diff -u -p -r1.97 rtl81x9reg.h
--- rtl81x9reg.h28 Dec 2015 05:49:15 - 1.97
+++ rtl81x9reg.h12 Apr 2016 21:09:53 -
@@ -153,6 +153,16 @@
#define RL_MISC0x00F0
/*
+ * Register used on RTL8111E
+ */
+#define RL_LEDSEL 0x0018
+#define RL_LED_LINK10 0x1
+#define RL_LED_LINK100 0x2
+#define RL_LED_LINK10000x4
+#define RL_LED_LINKRL_LED_LINK10 | RL_LED_LINK100 | RL_LED_LINK1000
+#define RL_LED_ACT 0x8
+
+/*
* TX config register bits
*/
#define RL_TXCFG_CLRABRT 0x0001 /* retransmit aborted pkt */
@@ -449,6 +459,7 @@
/*
* Config 4 register
*/
+#define RL_CFG4_CUSTOM_LED 0x40
#define RL_CFG4_LWPTN 0x04
#define RL_CFG4_LWPME 0x10
#define RL_CFG4_JUMBO_EN1 0x02
