> On May 30, 2018, at 12:47 AM, Martin Husemann <mar...@duskware.de> 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
