Re: i2c and indirect vs. direct config

[Jason Thorpe <thorpej%me.com@localhost>]

> On Jun 4, 2018, at 2:47 PM, Jason Thorpe <thorpej%me.com@localhost> wrote:
> 
> 
> 
>> On Jun 3, 2018, at 7:44 AM, Jason Thorpe <thorpej%me.com@localhost> wrote:
>> 
>> Actually, I am thinking more of a set of properties that control the behavior of the generic i2c code:
> 
> To follow-up, I think I have this sorted out.  There were a couple of other issues with how indirect-config was taking place on i2c bus instances, which I fixed as part of this.
> 
> I’ll post a more complete follow-up with the details tonight or tomorrow.

OK!

So, a couple of things:

— iic_search() was structured incorrectly, really.  In indirect-config, the “search” routine is called one for each directive in the kernel config file that matches the pspec.  If that directive has non-wildcarded locators, there is zero need to scan the address space of the bus.  It wasn’t entirely wrong before (because of the “check for wildcard” in the loop), but it certainly wasted cycles.

— i2c really needs varying levels of matchy-ness, based on the quality of the match, which I have addressed in my proposed changes.

…which are thus:

— iic_search() chooses a “probe strategy” based on the "i2c-indirect-probe-strategy” property on the “iic” instance.  Valid values are "smbus-quick-write”, "smbus-receive-byte”, and “none”.  If no value is specified, the default is "smbus-quick-write”.

— If the "i2c-indirect-device-whitelist” exists on the “iic” instance, iic_search() will first check the driver name in the cfdata_t against this list, and only allow the match/probe to move forward if the cfdata_t driver name is in the list.  This is primarily to accommodate the Intel integrated memory controller neutered-i2c-thing.

— If the cfdata_t specifies a wildcard address, each address of the i2c bus will be consulted.  If the cfdata_t contains a nailed-down address, then we limit the bus scan to that specific address.

— We explicitly skip reserved / special i2c addresses, such as the All-Call address, etc.

— We introduce the notion of a “match quality” for i2c drivers.  From lowest-quality to highest-quality: matched by plausible address only, matched by plausible address and poking at the bus to see if the device looks reasonable, matched by direct-config “compatible” string, matched by direct-config “driver name” string.

— If the “match quality” is merely “plausible address only”, then iic_search() will use the probe strategy selected above to see if a device responds to that address.


None of these things impact direct-config, at least not yet.  Eventually I will change all drivers to use the match quality values in their various match routines, including those for direct-config.

I haven’t tested these on my RPI just yet — but I will try to do so tonight or tomorrow.  But would like to solicit feedback on the approach now.

Index: dev/i2c/i2c.c
===================================================================
RCS file: /cvsroot/src/sys/dev/i2c/i2c.c,v
retrieving revision 1.58
diff -u -p -r1.58 i2c.c
--- dev/i2c/i2c.c	15 May 2018 02:02:18 -0000	1.58
+++ dev/i2c/i2c.c	5 Jun 2018 03:59:26 -0000
@@ -67,6 +67,7 @@ __KERNEL_RCSID(0, "$NetBSD: i2c.c,v 1.58
 #endif
 
 struct iic_softc {
+	device_t sc_dev;
 	i2c_tag_t sc_tag;
 	int sc_type;
 	device_t sc_devices[I2C_MAX_ADDR + 1];
@@ -127,21 +128,159 @@ iic_print(void *aux, const char *pnp)
 	return UNCONF;
 }
 
+static bool
+iic_is_special_address(i2c_addr_t addr)
+{
+
+	/*
+	 * See: https://www.i2c-bus.org/addressing/
+	 */
+
+	/* General Call (read) / Start Byte (write) */
+	if (addr == 0x00)
+		return (true);
+
+	/* CBUS Addresses */
+	if (addr == 0x01)
+		return (true);
+
+	/* Reserved for Different Bus Formats */
+	if (addr == 0x02)
+		return (true);
+
+	/* Reserved for future purposes */
+	if (addr == 0x03)
+		return (true);
+
+	/* High Speed Master Code */
+	if ((addr & 0x7c) == 0x04)
+		return (true);
+
+	/* 10-bit Slave Addressing prefix */
+	if ((addr & 0x7c) == 0x78)
+		return (true);
+	
+	/* Reserved for future purposes */
+	if ((addr & 0x7c) == 0x7c)
+		return (true);
+	
+	return (false);
+}
+
+static int
+iic_probe_none(struct iic_softc *sc,
+	       const struct i2c_attach_args *ia, int flags)
+{
+
+	return (0);
+}
+
+static int
+iic_probe_smbus_quick_write(struct iic_softc *sc,
+			    const struct i2c_attach_args *ia, int flags)
+{
+	int error;
+
+	if ((error = iic_acquire_bus(ia->ia_tag, flags)) == 0)
+		error = iic_smbus_quick_write(ia->ia_tag, ia->ia_addr, flags);
+	(void) iic_release_bus(ia->ia_tag, flags);
+
+	return (error);
+}
+
+static int
+iic_probe_smbus_receive_byte(struct iic_softc *sc,
+			     const struct i2c_attach_args *ia, int flags)
+{
+	int error;
+
+	if ((error = iic_acquire_bus(ia->ia_tag, flags)) == 0) {
+		uint8_t dummy;
+
+		error = iic_smbus_receive_byte(ia->ia_tag, ia->ia_addr,
+					       &dummy, flags);
+	}
+	(void) iic_release_bus(ia->ia_tag, flags);
+
+	return (error);
+}
+
+static bool
+iic_indirect_driver_is_whitelisted(struct iic_softc *sc, cfdata_t cf)
+{
+	prop_object_iterator_t iter;
+	prop_array_t whitelist;
+	prop_string_t pstr;
+	prop_type_t ptype;
+	bool rv = false;
+
+	whitelist = prop_dictionary_get(device_properties(sc->sc_dev),
+					I2C_PROP_INDIRECT_DEVICE_WHITELIST);
+	if (whitelist == NULL) {
+		/* No whitelist -> everything allowed */
+		return (true);
+	}
+
+	if ((ptype = prop_object_type(whitelist)) != PROP_TYPE_ARRAY) {
+		aprint_error_dev(sc->sc_dev,
+		    "invalid property type (%d) for '%s'; must be array (%d)\n",
+		    ptype, I2C_PROP_INDIRECT_DEVICE_WHITELIST, PROP_TYPE_ARRAY);
+		return (false);
+	}
+
+	iter = prop_array_iterator(whitelist);
+	while ((pstr = prop_object_iterator_next(iter)) != NULL) {
+		if (prop_string_equals_cstring(pstr, cf->cf_name)) {
+			rv = true;
+			break;
+		}
+	}
+	prop_object_iterator_release(iter);
+
+	return (rv);
+}
+
 static int
 iic_search(device_t parent, cfdata_t cf, const int *ldesc, void *aux)
 {
 	struct iic_softc *sc = device_private(parent);
 	struct i2c_attach_args ia;
+	int (*probe_func)(struct iic_softc *,
+			  const struct i2c_attach_args *, int);
+	prop_string_t pstr;
+	i2c_addr_t first_addr, last_addr;
 
 	/*
-	 * I2C doesn't have any regular probing capability.  If we
-	 * encounter a cfdata with a wild-carded address or a wild-
-	 * carded parent spec, we skip them because they can only
-	 * be used for direct-coniguration.
+	 * Before we do any more work, consult the allowed-driver
+	 * white-list for this bus (if any).
 	 */
-	if (cf->cf_loc[IICCF_ADDR] == IICCF_ADDR_DEFAULT ||
-	    cf->cf_pspec->cfp_unit == DVUNIT_ANY)
-		return 0;
+	if (iic_indirect_driver_is_whitelisted(sc, cf) == false)
+		return (0);
+
+	/* default to "quick write". */
+	probe_func = iic_probe_smbus_quick_write;
+
+	pstr = prop_dictionary_get(device_properties(sc->sc_dev),
+				   I2C_PROP_INDIRECT_PROBE_STRATEGY);
+	if (pstr == NULL) {
+		/* Use the default. */
+	} else if (prop_string_equals_cstring(pstr,
+					I2C_PROBE_STRATEGY_QUICK_WRITE)) {
+		probe_func = iic_probe_smbus_quick_write;
+	} else if (prop_string_equals_cstring(pstr,
+					I2C_PROBE_STRATEGY_RECEIVE_BYTE)) {
+		probe_func = iic_probe_smbus_receive_byte;
+	} else if (prop_string_equals_cstring(pstr,
+					I2C_PROBE_STRATEGY_NONE)) {
+		probe_func = iic_probe_none;
+	} else {
+		aprint_error_dev(sc->sc_dev,
+			"unknown probe strategy '%s'; defaulting to '%s'\n",
+			prop_string_cstring_nocopy(pstr),
+			I2C_PROBE_STRATEGY_QUICK_WRITE);
+
+		/* Use the default. */
+	}
 
 	ia.ia_tag = sc->sc_tag;
 	ia.ia_size = cf->cf_loc[IICCF_SIZE];
@@ -152,16 +291,74 @@ iic_search(device_t parent, cfdata_t cf,
 	ia.ia_compat = NULL;
 	ia.ia_prop = NULL;
 
-	for (ia.ia_addr = 0; ia.ia_addr <= I2C_MAX_ADDR; ia.ia_addr++) {
+	if (cf->cf_loc[IICCF_ADDR] == IICCF_ADDR_DEFAULT) {
+		/*
+		 * This particular config directive has
+		 * wildcarded the address, so we will
+		 * scan the entire bus for it.
+		 */
+		first_addr = 0;
+		last_addr = I2C_MAX_ADDR;
+	} else {
+		/*
+		 * This config directive hard-wires the i2c
+		 * bus address for the device, so there is
+		 * no need to go poking around at any other
+		 * addresses.
+		 */
+		if (cf->cf_loc[IICCF_ADDR] < 0 ||
+		    cf->cf_loc[IICCF_ADDR] > I2C_MAX_ADDR) {
+			/* Invalid config directive! */
+			return (0);
+		}
+		first_addr = last_addr = cf->cf_loc[IICCF_ADDR];
+	}
+
+	for (ia.ia_addr = first_addr; ia.ia_addr <= last_addr; ia.ia_addr++) {
+		int error, match_result;
+
+		/*
+		 * Skip I2C addresses that are reserved for
+		 * special purposes.
+		 */
+		if (iic_is_special_address(ia.ia_addr))
+			continue;
+
+		/*
+		 * Skip addresses where a device is already attached.
+		 */
 		if (sc->sc_devices[ia.ia_addr] != NULL)
 			continue;
 
-		if (cf->cf_loc[IICCF_ADDR] != ia.ia_addr)
+		/*
+		 * Call the "match" routine for the device.  If that
+		 * returns success, then call the probe strategy
+		 * function.
+		 *
+		 * We do it in this order because i2c devices tend
+		 * to be found at a small number of possible addresses
+		 * (e.g. read-time clocks that are only ever found at
+		 * 0x68).  This gives the driver a chance to skip any
+		 * address that are not valid for the device, saving
+		 * us from having to poke at the bus to see if anything
+		 * is there.
+		 */
+		match_result = config_match(parent, cf, &ia);
+		if (match_result <= 0)
+			continue;
+
+		/*
+		 * If the quality of the match by the driver was low
+		 * (i.e. matched on being a valid address only, didn't
+		 * perform any hardware probe), invoke our probe routine
+		 * to see if it looks like something is really there.
+		 */
+		if (match_result == I2C_MATCH_ADDRESS_ONLY &&
+		    (error = (*probe_func)(sc, &ia, I2C_F_POLL)) != 0)
 			continue;
 
-		if (config_match(parent, cf, &ia) > 0)
-			sc->sc_devices[ia.ia_addr] =
-			    config_attach(parent, cf, &ia, iic_print);
+		sc->sc_devices[ia.ia_addr] =
+		    config_attach(parent, cf, &ia, iic_print);
 	}
 
 	return 0;
@@ -209,6 +406,7 @@ iic_attach(device_t parent, device_t sel
 	aprint_naive("\n");
 	aprint_normal(": I2C bus\n");
 
+	sc->sc_dev = self;
 	sc->sc_tag = iba->iba_tag;
 	sc->sc_type = iba->iba_type;
 	ic = sc->sc_tag;
Index: dev/i2c/i2cvar.h
===================================================================
RCS file: /cvsroot/src/sys/dev/i2c/i2cvar.h,v
retrieving revision 1.10
diff -u -p -r1.10 i2cvar.h
--- dev/i2c/i2cvar.h	10 Dec 2017 16:53:32 -0000	1.10
+++ dev/i2c/i2cvar.h	5 Jun 2018 03:59:26 -0000
@@ -49,6 +49,20 @@
 #define	I2C_F_POLL		0x08	/* poll, don't sleep */
 #define	I2C_F_PEC		0x10	/* smbus packet error checking */
 
+/* i2c bus instance properties */
+#define	I2C_PROP_INDIRECT_PROBE_STRATEGY	\
+				"i2c-indirect-probe-strategy"
+#define	I2C_PROBE_STRATEGY_QUICK_WRITE		\
+				"smbus-quick-write"
+#define	I2C_PROBE_STRATEGY_RECEIVE_BYTE		\
+				"smbus-receive-byte"
+#define	I2C_PROBE_STRATEGY_NONE			\
+				"none"
+
+#define	I2C_PROP_INDIRECT_DEVICE_WHITELIST	\
+				"i2c-indirect-device-whitelist"
+	/* value is a prop_array of prop_strings */
+
 struct ic_intr_list {
 	LIST_ENTRY(ic_intr_list) il_next;
 	int (*il_intr)(void *);
@@ -147,6 +161,23 @@ struct i2c_attach_args {
 int	iicbus_print(void *, const char *);
 int	iic_compat_match(struct i2c_attach_args*, const char **);
 
+/*
+ * Constants to indicate the quality of a match made by a driver's
+ * match routine, from lowest to higest:
+ *
+ *	-- Address only; no other checks were made.
+ *
+ *	-- Address + device probed and recognized.
+ *
+ *	-- Direct-config match by "compatible" string.
+ *
+ *	-- Direct-config match by specific driver name.
+ */
+#define	I2C_MATCH_ADDRESS_ONLY		1
+#define	I2C_MATCH_ADDRESS_AND_PROBE	2
+#define	I2C_MATCH_DIRECT_COMPATIBLE	10
+#define	I2C_MATCH_DIRECT_SPECIFIC	50
+
 #ifdef _I2C_PRIVATE
 /*
  * Macros used internally by the i2c framework.


-- thorpej