Merge pull request #345 from DrDoof/fix/sfp-i2c-error

sfp: notice when an I2C read fails

cmd_parser.c

@@ -759,18 +759,25 @@ void parse_mtu(void)
 	write_char('\n');
 }
 
-void sfp_print_measurements(uint8_t sfp)
+bool sfp_print_measurements(uint8_t sfp)
 {
-	print_string("Options: "); print_byte(sfp_read_reg(sfp, 92)); write_char('\n');
+	if (!sfp_read_block(sfp, 92, 1))
+		return false;
+
+	print_string("Options: "); print_byte(sfp_buf[0]); write_char('\n');
 	if (!(sfp_options[sfp] & 0x40))
-		return;
+		return true;
-	print_string("Temp: "); print_byte(sfp_read_reg(sfp, 224)); print_byte(sfp_read_reg(sfp, 225)); write_char('\n');
+	if (!sfp_read_block(sfp, 224, 16))
-	print_string("Vcc: "); print_byte(sfp_read_reg(sfp, 226)); print_byte(sfp_read_reg(sfp, 227)); write_char('\n');
+		return false;
-	print_string("TX Bias: "); print_byte(sfp_read_reg(sfp, 228)); print_byte(sfp_read_reg(sfp, 229)); write_char('\n');
+	print_string("Temp: "); print_byte(sfp_buf[0]); print_byte(sfp_buf[1]); write_char('\n');
-	print_string("TX Power: "); print_byte(sfp_read_reg(sfp, 230)); print_byte(sfp_read_reg(sfp, 231)); write_char('\n');
+	print_string("Vcc: "); print_byte(sfp_buf[2]); print_byte(sfp_buf[3]); write_char('\n');
-	print_string("RX Power: "); print_byte(sfp_read_reg(sfp, 232)); print_byte(sfp_read_reg(sfp, 233)); write_char('\n');
+	print_string("TX Bias: "); print_byte(sfp_buf[4]); print_byte(sfp_buf[5]); write_char('\n');
-	print_string("Laser: "); print_byte(sfp_read_reg(sfp, 234)); print_byte(sfp_read_reg(sfp, 235)); write_char('\n');
+	print_string("TX Power: "); print_byte(sfp_buf[6]); print_byte(sfp_buf[7]); write_char('\n');
-	print_string("State: "); print_byte(sfp_read_reg(sfp, 238)); write_char('\n');
+	print_string("RX Power: "); print_byte(sfp_buf[8]); print_byte(sfp_buf[9]); write_char('\n');
+	print_string("Laser: "); print_byte(sfp_buf[10]); print_byte(sfp_buf[11]); write_char('\n');
+	print_string("State: "); print_byte(sfp_buf[14]); write_char('\n');
+
+	return true;
 }
 
 
@@ -788,11 +795,15 @@ void parse_sfp(void)
 				print_string(" - empty\n");
 				continue;
 			}
-			print_string(" - Rate: "); print_byte(sfp_read_reg(slot, 12));
+			if (!sfp_read_block(slot, 11, 2)) {
-			print_string("  Encoding: "); print_byte(sfp_read_reg(slot, 11));
+				print_string(" - I2C read failed on this slot\n");
+				continue;
+			}
+			print_string(" - Rate: "); print_byte(sfp_buf[1]);
+			print_string("  Encoding: "); print_byte(sfp_buf[0]);
 			write_char('\n');
-			sfp_print_info(slot);
+			if (!sfp_print_info(slot) || !sfp_print_measurements(slot))
-			sfp_print_measurements(slot);
+				print_string("I2C read failed on this slot\n");
 		}
 		return;
 	}

httpd/page_impl.c

@@ -177,10 +177,12 @@ void reg_to_html_long(register uint16_t reg)
 void send_sfp_info(uint8_t sfp)
 {
 	// This loops over the Vendor-name, Vendor OUI, Vendor PN and Vendor rev ASCII fields
-	for (uint8_t i = 20; i < 60; i++) {
+	for (uint8_t i = 16; i < 64; i++) {
-		if (i >= 36 && i < 40) // Skip Non-ASCII codes
+		if (!(i & 0xf))
+			sfp_read_block(sfp, i, 16);
+		if (i < 20 || i >= 60 || (i >= 36 && i < 40)) // Skip Non-ASCII codes
 			continue;
-		uint8_t c = sfp_read_reg(sfp, i);
+		uint8_t c = sfp_buf[i & 0xf];
 		if (c && c != 0xa0) // a0 is the byte read from a non-existant I2C EEPROM
 			char_to_html(c);
 	}
@@ -193,32 +195,10 @@ void sfp_send_data(uint8_t slot, uint8_t reg, uint8_t len)
 	if (len > 16)
 		return;
 
-	if (reg & 0x80) {	// Configure SFP readings address (0x51) as I2C device address
+	sfp_read_block(slot, reg, len);
-		reg &= 0x7f;
-		REG_WRITE(RTL837X_REG_I2C_CTRL, 0x00, 0x1 << (I2C_MEM_ADDR_WIDTH-16) | (len - 1) & 0xf,  0x51 >> 5, (0x51 << 3) & 0xff);
-	} else {
-		REG_WRITE(RTL837X_REG_I2C_CTRL, 0x00, 0x1 << (I2C_MEM_ADDR_WIDTH-16) | (len - 1) & 0xf,  0x50 >> 5, (0x50 << 3) & 0xff);
-	}
 
-	reg_read_m(RTL837X_REG_I2C_CTRL);
+	for (uint8_t i = 0; i < len; i++)
-	sfr_mask_data(1, 0xfc, i2c_bus_from_scl_pin(machine.sfp_port[slot].i2c.scl) << 5 | i2c_bus_from_sda_pin(machine.sfp_port[slot].i2c.sda) << 2);
+		byte_to_html(sfp_buf[i]);
-	reg_write_m(RTL837X_REG_I2C_CTRL);
-
-	REG_WRITE(RTL837X_REG_I2C_IN, 0, 0, 0, reg);
-
-	// Execute I2C Read
-	reg_bit_set(RTL837X_REG_I2C_CTRL, 0);
-
-	// Wait for execution to finish
-	do {
-		reg_read_m(RTL837X_REG_I2C_CTRL);
-	} while (sfr_data[3] & 0x1);
-
-	for (uint8_t i = 0; i < len; i++) {
-		if (!(i & 0x3))
-			reg_read_m(RTL837X_REG_I2C_OUT + i);
-		byte_to_html(sfr_data[3 - (i & 0x3)]);
-	}
 }
 
 

rtl837x_common.h

@@ -150,7 +150,8 @@ void sleep(uint16_t t);
 void write_char_no_syslog(char c);
 void write_char(char c);
 void print_reg(uint16_t reg);
-uint8_t sfp_read_reg(uint8_t slot, uint8_t reg);
+bool sfp_read_block(uint8_t slot, uint8_t reg, uint8_t len) __banked __reentrant;
+extern __xdata uint8_t sfp_buf[16];
 void reg_bit_set(uint16_t reg_addr, char bit);
 void reg_bit_clear(uint16_t reg_addr, char bit);
 uint8_t reg_bit_test(uint16_t reg_addr, char bit);
@@ -171,7 +172,7 @@ void tcpip_output(void);
 uint8_t read_flash(uint8_t bank, __code uint8_t *addr);
 void get_random_32(void);
 void read_reg_timer(__xdata uint32_t * tmr);
-void sfp_print_info(uint8_t sfp);
+bool sfp_print_info(uint8_t sfp);
 bool gpio_pin_test(uint8_t pin);
 void set_sys_led_state(uint8_t state);
 void sds_read(uint8_t sds_id, uint8_t page, uint8_t reg);

rtl837x_pins.c

@@ -1,6 +1,11 @@
 #include "rtl837x_pins.h"
 #include "rtl837x_common.h"
+#include "rtl837x_sfr.h"
 #include "rtl837x_regs.h"
+#include "machine.h"
+
+extern __code const struct machine machine;
+extern __xdata uint8_t sfr_data[4];
 
 #pragma codeseg BANK2
 #pragma constseg BANK2
@@ -121,3 +126,56 @@ void gpio_output_setup(uint8_t pin, __xdata uint8_t initial_val) __banked{
 
 	reg_bit_set(gpio_direction_reg(pin), (pin % 32));
 }
+
+
+/*
+ * Read up to 16 consecutive registers of the EEPROM via I2C into sfp_buf
+ */
+bool sfp_read_block(uint8_t slot, uint8_t reg, uint8_t len) __banked __reentrant
+{
+	uint8_t dev;
+	uint8_t val;
+
+	len--;
+	if (len > 15)
+		return false;
+
+	dev = (reg & 0x80) ? 0x51 : 0x50;	// 0x51 holds the diagnostics, 0x50 the module data
+	reg &= 0x7f;
+
+	REG_WRITE(RTL837X_REG_I2C_IN, 0, 0, 0, reg);
+
+	REG_WRITE(RTL837X_REG_I2C_CTRL, 0x00,
+		  0x1 << (I2C_MEM_ADDR_WIDTH - 16) | len,
+		  (dev >> 5) | i2c_bus_from_scl_pin(machine.sfp_port[slot].i2c.scl) << 5
+		  | i2c_bus_from_sda_pin(machine.sfp_port[slot].i2c.sda) << 2,
+		  ((dev << 3) & 0xff) | 0x1);
+
+	do {
+		reg_read(RTL837X_REG_I2C_CTRL);
+	} while (SFR_DATA_0 & 0x1);
+
+	if (SFR_DATA_0 & 0x2)
+		return false;
+
+	for (uint8_t i = 0; i <= len; i++) {
+		switch (i & 0x3) {
+		case 0:
+			reg_read(RTL837X_REG_I2C_OUT + i);
+			val = SFR_DATA_0;
+			break;
+		case 1:
+			val = SFR_DATA_8;
+			break;
+		case 2:
+			val = SFR_DATA_16;
+			break;
+		default:
+			val = SFR_DATA_24;
+			break;
+		}
+		sfp_buf[i] = val;
+	}
+
+	return true;
+}

rtlplayground.c

@@ -140,6 +140,7 @@ __xdata char sfp_module_vendor[2][17];
 __xdata char sfp_module_model[2][17];
 __xdata char sfp_module_serial[2][17];
 __xdata uint8_t sfp_options[2];
+__xdata uint8_t sfp_buf[16];	/* scratch for one I2C transaction, the controller reads at most 16 bytes */
 __xdata uint8_t sfp_speed[2];
 __xdata uint8_t sfp_quirks[2];
 __xdata bool button_last;
@@ -1068,37 +1069,6 @@ void sds_config(uint8_t sds, uint8_t mode)
 }
 
 
-/*
- * Read a register of the EEPROM via I2C
- */
-uint8_t sfp_read_reg(uint8_t slot, uint8_t reg)
-{
-	if (reg & 0x80) {	// Configure SFP readings address (0x51) as I2C device address
-		reg &= 0x7f;
-		REG_WRITE(RTL837X_REG_I2C_CTRL, 0x00, 0x1 << (I2C_MEM_ADDR_WIDTH-16) | 0,  0x51 >> 5, (0x51 << 3) & 0xff);
-	} else {
-		REG_WRITE(RTL837X_REG_I2C_CTRL, 0x00, 0x1 << (I2C_MEM_ADDR_WIDTH-16) | 0,  0x50 >> 5, (0x50 << 3) & 0xff);
-	}
-
-	reg_read_m(RTL837X_REG_I2C_CTRL);
-	sfr_mask_data(1, 0xfc, i2c_bus_from_scl_pin(machine.sfp_port[slot].i2c.scl) << 5 | i2c_bus_from_sda_pin(machine.sfp_port[slot].i2c.sda) << 2);
-	reg_write_m(RTL837X_REG_I2C_CTRL);
-
-	REG_WRITE(RTL837X_REG_I2C_IN, 0, 0, 0, reg);
-
-	// Execute I2C Read
-	reg_bit_set(RTL837X_REG_I2C_CTRL, 0);
-
-	// Wait for execution to finish
-	do {
-		reg_read_m(RTL837X_REG_I2C_CTRL);
-	} while (sfr_data[3] & 0x1);
-
-	reg_read_m(RTL837X_REG_I2C_OUT);
-	return sfr_data[3];
-}
-
-
 /*
  * Adds TX Header to uip_buf and calls nic_tx_packet to send the packet
  * over the wire
@@ -1251,36 +1221,46 @@ static inline uint8_t sfp_rate_to_sds_config(register uint8_t rate)
 }
 
 
-void sfp_print_info(uint8_t sfp)
+bool sfp_print_info(uint8_t sfp)
 {
 	// This loops over the Vendor-name, Vendor OUI, Vendor PN and Vendor rev ASCII fields
-	for (uint8_t i = 20; i < 60; i++) {
+	for (uint8_t i = 16; i < 64; i++) {
-		if (i >= 36 && i < 40) // Skip Non-ASCII codes
+		if (!(i & 0xf) && !sfp_read_block(sfp, i, 16))
+			return false;
+		if (i < 20 || i >= 60 || (i >= 36 && i < 40)) // Skip Non-ASCII codes
 			continue;
-		uint8_t c = sfp_read_reg(sfp, i);
+		uint8_t c = sfp_buf[i & 0xf];
 		if (c)
 			write_char(c);
 	}
 	print_string("\n");
+
+	return true;
 }
 
 // Normalize strings from EEPROM by removing any trailing spaces; this allows simpler comparisons
-void sfp_read_field(__xdata char *dst, uint8_t sfp, uint8_t start, uint8_t length) __reentrant
+bool sfp_read_field(__xdata char *dst, uint8_t sfp, uint8_t start, uint8_t length) __reentrant
 {
-	dst[length] = '\0';
+	if (!sfp_read_block(sfp, start, length))
+		return false;
 
-	for (uint8_t i = 0; i < length; i++)
+	dst[length] = '\0';
-		dst[i] = sfp_read_reg(sfp, start + i);
+	memcpy(dst, sfp_buf, length);
 
 	while (length > 0 && dst[--length] == ' ')
 		dst[length] = '\0';
+
+	return true;
 }
 
-void sfp_get_info(uint8_t sfp)
+bool sfp_get_info(uint8_t sfp)
 {
-	sfp_read_field(sfp_module_vendor[sfp], sfp, 20, 16);
+	if (!sfp_read_field(sfp_module_vendor[sfp], sfp, 20, 16))
-	sfp_read_field(sfp_module_model[sfp], sfp, 40, 16);
+		return false;
-	sfp_read_field(sfp_module_serial[sfp], sfp, 68, 16);
+	if (!sfp_read_field(sfp_module_model[sfp], sfp, 40, 16))
+		return false;
+
+	return sfp_read_field(sfp_module_serial[sfp], sfp, 68, 16);
 }
 
 void sfp_apply_quirks(uint8_t sfp) __reentrant
@@ -1299,7 +1279,7 @@ void sfp_apply_quirks(uint8_t sfp) __reentrant
 		if (!(sfp_options[sfp] & 0x40)) {
 			// The module reports that DDM is not implemented, but try a dummy read to confirm
 			// 0xff would mean a failed I2C read or an impossible (per spec) voltage greater than 6.5V
-			if (sfp_read_reg(sfp, 226) != 0xff) {
+			if (sfp_read_block(sfp, 226, 1) && sfp_buf[0] != 0xff) {
 				sfp_options[sfp] |= 0x40;
 			}
 		}
@@ -1323,6 +1303,45 @@ void setup_sfp_gpio(void)
 	}
 }
 
+static bool sfp_module_read(uint8_t sfp)
+{
+	uint8_t rate;
+
+	// Read Reg 11: Encoding, see SFF-8472 and SFF-8024
+	// Read Reg 12: Signalling rate (including overhead) in 100Mbit: 0xd: 1Gbit, 0x67:10Gbit
+	delay(100); // Delay, because some modules need time to wake up
+	if (!sfp_read_block(sfp, 11, 2))
+		return false;
+
+	rate = sfp_buf[1];
+	if (sfp_speed[sfp] == SFP_SPEED_100M)
+		rate = 0x1;
+	else if (sfp_speed[sfp] == SFP_SPEED_1G)
+		rate = 0xc;
+	else if (sfp_speed[sfp] == SFP_SPEED_2G5)
+		rate = 0x19;
+	else if (sfp_speed[sfp] == SFP_SPEED_10G)
+		rate = 0x69;
+	print_string("  Rate: "); print_byte(rate);  // Normally 1, but 0 for DAC, can be ignored?
+	print_string("  Encoding: "); print_byte(sfp_buf[0]);
+	print_string("  Module: ");
+	if (!sfp_print_info(sfp))
+		return false;
+	print_string("\n");
+
+	if (!sfp_read_block(sfp, 92, 1))
+		return false;
+	sfp_options[sfp] = sfp_buf[0];
+	if (!sfp_get_info(sfp))
+		return false;
+
+	sfp_apply_quirks(sfp);
+	sds_config(machine.sfp_port[sfp].sds, sfp_rate_to_sds_config(rate));
+
+	return true;
+}
+
+
 void handle_sfp(void)
 {
 	for (uint8_t sfp = 0; sfp < machine.n_sfp; sfp++) {
@@ -1330,26 +1349,10 @@ void handle_sfp(void)
 			if (sfp_pins_last & (0x1 << (sfp << 2))) {
 				sfp_pins_last &= ~(0x01 << (sfp << 2));
 				print_string("\n<MODULE INSERTED>  Slot: "); write_char('1' + sfp);
-				// Read Reg 11: Encoding, see SFF-8472 and SFF-8024
+				if (!sfp_module_read(sfp)) {
-				// Read Reg 12: Signalling rate (including overhead) in 100Mbit: 0xd: 1Gbit, 0x67:10Gbit
+					print_string("SFP: an I2C read failed, retrying on the next poll\n");
-				delay(100); // Delay, because some modules need time to wake up
+					sfp_pins_last |= 0x01 << (sfp << 2);
-				uint8_t rate = sfp_read_reg(sfp, 12);
+				}
-				if (sfp_speed[sfp] == SFP_SPEED_100M)
-					rate = 0x1;
-				else if (sfp_speed[sfp] == SFP_SPEED_1G)
-					rate = 0xc;
-				else if (sfp_speed[sfp] == SFP_SPEED_2G5)
-					rate = 0x19;
-				else if (sfp_speed[sfp] == SFP_SPEED_10G)
-					rate = 0x69;
-				print_string("  Rate: "); print_byte(rate);  // Normally 1, but 0 for DAC, can be ignored?
-				print_string("  Encoding: "); print_byte(sfp_read_reg(sfp, 11));
-				print_string("  Module: "); sfp_print_info(sfp);
-				print_string("\n");
-				sfp_options[sfp] = sfp_read_reg(sfp, 92);
-				sfp_get_info(sfp);
-				sfp_apply_quirks(sfp);
-				sds_config(machine.sfp_port[sfp].sds, sfp_rate_to_sds_config(rate));
 			}
 		} else {
 			if (!(sfp_pins_last & (0x1 << (sfp << 2)))) {