From 1d1e33f4d5008d645df4b5906beff7a61f66a193 Mon Sep 17 00:00:00 2001
From: d00f <8052722+DrDoof@users.noreply.github.com>
Date: Sat, 15 Aug 2026 17:25:51 +0200
Subject: [PATCH] sfp: notice when an I2C read fails

sfp_read_reg() waited for the transfer to finish and then read the output
register whatever the outcome, so an address nothing acknowledged came back
as an ordinary byte and no caller could tell it apart from data. The vendor
SDK looks at bit 1 of the control register for exactly this, and we did
not.

A failure now sets sfp_i2c_fail and the read returns 0xff, which is already
the value sfp_apply_quirks() reads as either a failed transfer or a voltage
the spec does not allow, so that test starts being true when it should be.
The insertion path and the sfp command clear the flag first and say so
afterwards, rather than presenting the bytes as though they came from the
module.

What this deliberately does not do is act on the failure. Skipping
sds_config() when the rate read failed is the obvious next step, but a
module that raises the bit spuriously would then never be configured at
all, which is worse than what happens today, and I have no way to judge how
often the bit is right. That decision belongs with someone holding the
board.

It also leaves the other half of the rewrite alone, reading and writing up
to sixteen bytes per transaction. doc/sfp.md describes only the single byte
path and does not name a length field, and guessing at a register I cannot
test is how the last attempt at this function went wrong.

40 bytes of the common segment, 51 of BANK2 and 1 of xdata, nothing in
BANK1 or internal RAM. Built for SWTGW218AS and KP_9000_6XHML_X2 on sdcc
4.5.0. Not tested on hardware: shorting the clock line, as in #342, should
now print the failure line instead of a plausible looking byte.
---
 cmd_parser.c     |  3 +++
 rtl837x_common.h |  1 +
 rtlplayground.c  | 12 ++++++++++++
 3 files changed, 16 insertions(+)

diff --git a/cmd_parser.c b/cmd_parser.c
index 9e2be41..2b13edd 100644
--- a/cmd_parser.c
+++ b/cmd_parser.c
@@ -777,11 +777,14 @@ void parse_sfp(void)
 				print_string(" - empty\n");
 				continue;
 			}
+			sfp_i2c_fail = 0;
 			print_string(" - Rate: "); print_byte(sfp_read_reg(slot, 12));
 			print_string("  Encoding: "); print_byte(sfp_read_reg(slot, 11));
 			write_char('\n');
 			sfp_print_info(slot);
 			sfp_print_measurements(slot);
+			if (sfp_i2c_fail)
+				print_string("I2C read failed on this slot\n");
 		}
 		return;
 	}
diff --git a/rtl837x_common.h b/rtl837x_common.h
index 66df88a..98202b8 100644
--- a/rtl837x_common.h
+++ b/rtl837x_common.h
@@ -150,6 +150,7 @@ 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);
+extern __xdata uint8_t sfp_i2c_fail;
 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);
diff --git a/rtlplayground.c b/rtlplayground.c
index bdb8ab3..f8cd721 100644
--- a/rtlplayground.c
+++ b/rtlplayground.c
@@ -139,6 +139,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_i2c_fail;	/* set by sfp_read_reg() when the controller flags a failed transfer */
 __xdata uint8_t sfp_speed[2];
 __xdata uint8_t sfp_quirks[2];
 __xdata bool button_last;
@@ -1061,6 +1062,14 @@ uint8_t sfp_read_reg(uint8_t slot, uint8_t reg)
 		reg_read_m(RTL837X_REG_I2C_CTRL);
 	} while (sfr_data[3] & 0x1);
 
+	/* Bit 1 is the controller's own failure indication, which the vendor SDK
+	 * looks at and this did not. Without it an unacknowledged address comes
+	 * back as an ordinary byte and the caller cannot tell it from data. */
+	if (sfr_data[3] & 0x2) {
+		sfp_i2c_fail = 1;
+		return 0xff;
+	}
+
 	reg_read_m(RTL837X_REG_I2C_OUT);
 	return sfr_data[3];
 }
@@ -1300,6 +1309,7 @@ void handle_sfp(void)
 				// 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
+				sfp_i2c_fail = 0;
 				uint8_t rate = sfp_read_reg(sfp, 12);
 				if (sfp_speed[sfp] == SFP_SPEED_100M)
 					rate = 0x1;
@@ -1316,6 +1326,8 @@ void handle_sfp(void)
 				sfp_options[sfp] = sfp_read_reg(sfp, 92);
 				sfp_get_info(sfp);
 				sfp_apply_quirks(sfp);
+				if (sfp_i2c_fail)
+					print_string("SFP: an I2C read failed, the module data above may be wrong\n");
 				sds_config(machine.sfp_port[sfp].sds, sfp_rate_to_sds_config(rate));
 			}
 		} else {