Re: i2c and indirect vs. direct config

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

> On May 30, 2018, at 12:47 AM, Martin Husemann <martin%duskware.de@localhost> wrote:
> 
> To avoid this indirect config fallback, Jared disabled this 
> for all i2c controllers FDT knows about, assuming it would be
> easy to just fix FDT instead.

To be clear — Indirect-config wasn’t disabled by setting the i2c-indirect-config property to false, but rather the i2c-child-devices property was set to an empty array which had the **side-effect** of stopping indirect-config.

> Recently Jason ran into a problem with this (and I am not sure I properly
> understood it). While fixing things, he also disabled indirect config
> for wildcard i2c instances.

First of all, it rendered the RPI kernel config file somewhat broken — it has commented-out examples of e.g. RTC devices and whatnot on the i2c bus, and enabling them didn’t work due to this issue.  This is how I first discovered it… while bread-boarding a prototype of a project that included a RTC chip.  “WTF didn’t the dsrtc driver attach?”

The issue basically boiled down to the inability to add a driver to the kernel config file and have it work as expected (and indeed how it does on other platforms).

The reason for the follow-up change that requires concrete locators is the following:  Imagine a system that has more than one i2c interface.  Now let’s imagine that you have the following in your kernel config file:

dsrtc* at iic? addr 0x68 flags 1307

In the case of the dsrtc driver, what that says is: “Look for a dsrtc device on any i2c bus instance at address 0x68, and when you find one, it will be a DS1307”.

The DS1307 RTC (and the other Dallas Semiconductor variants) will only ever exist at i2c address 0x68 (they are hard-wired there).  It’s not really possible to probe for them in a meaningful way because it’s practically impossible to distinguish between “no device” and “device is there but contains no valid data”.  Writing to it to elicit some reaction only results in modifying data, which isn’t very neighborly.  In the case of this device, *maybe* it would be OK, but different flavors of the device have different things at different offsets, and while there isn’t likely to be another device on the bus that is at 0x68 instead of a DS1307-alike, it could happen (because not all RTCs are at 0x68).

So, the result of that config file directive?  A dsrtc instance attached to each i2c bus that the system has… of course, only one of them actually works.  This was the reason to require a concrete parent locator.  With i2c, you sort of have to know what you’re doing.

> Unfortunately this totally breaks i2c on i386/amd64, and it is not practically
> possible to hardwire "dbcool* at i2c2" or something in an x86 GENERIC kernel.
> 
> I don't understand why Jason needed indirect config at all - to me it sounds
> the proper solution would have been to enhance the FDT for that particular
> machine and only use direct config.

There’s really two problems with that answer, though :-)  First of all, it’s really non-obvious that just adding it to the kernel config file like you’d do on any other embedded system platform won’t work, the tools for modifying the FDT are way more complicated than the kernel config file, and you have to do the kernel config file anyway.

Secondly, constantly fiddling with the FDT while you’re prototyping is annoying, and to complicate matters, I don’t think all loaders support FDT overlays, meaning have to build a complete FDT for the device.  Ick.

> But in general Jason is right: indirect config on i2c *is* dangerous.

But of course, there are exceptions!  Gotta keep things interesting, right?? :-)

spdmem is a great example — Even the name gives it away “Serial Presence Detect” — you’re probing to see if the memory modules are there, as well as their parameters.  I think HDMI’s i2c interface for EDID / DisplayID might be another example where “probing” is OK.

> A possible solution would be to have config flags (or device
> attributes) per i2c bus, which controll:
> 
> - whether to use the strict indirect config semantic we have now
> - not to use any indirect config at all (a bus that has a full
>   description in OF/fdt would set this)
> - allow arbitrary wildcard parent matches (amd64 GENERIC would
>   use that) 

A couple of thoughts on this:

— If the firmware / FDT for a device knows about all of the i2c devices present and it’s not possible to add additional ones, they should set i2c-indirect-config to false, and NOT rely on the presence of i2c-child-devices to disable indirect-config.

— The i2c code should do configuration in 2 passes:
	— If i2c-child-devices is specified, enumerate that array and attach those devices.
	— If i2c-indirect-config is not false, then perform indirect-config by scanning for kernel config directives.

…and then perhaps there should be a new property “i2c-requires-concrete-locators” that, if set to true, enables that behavior and we can default it to unset / false.

> It would be nice to move more machines over to direct config (I am not
> sure how much data ACPI or UEFI provide for that), but still we need working
> arbitrary indirect config matches for machines with no means of direct
> config.

In the case of ACPI, there fact of the matter is that we shouldn’t be attaching discrete i2c drivers at all, really.  The whole point of ACPI is to use the ACPI methods to talk to the sensor hardware, because the vendor that built the board populated the tables and plopped in the correct AML to talk to those devices.

-- thorpej