The diff D43811 has been split into two parts. This part consists in the splitting of the original vf_i2c driver between the FDT-specific code and the I2C controller code, as well as the introduction of ACPI-specific code. The driver files are also moved to a new recommended location. The original I2C controller logic has been kept. The FDT driver from this code should work whenever the original FDT driver worked. The second part is D44021 and it changes the I2C controller logic so it is more consistent with the reference manual for the controller.
Details
Details
-The FDT driver should work whenever the original vf_i2c driver did.
-The ACPI driver should attach for all chipsets of interest. I don't expect the I2C controller logic to work on all supported platforms
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Sorry this looks a lot but is mostly just white space.
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
|---|---|---|
| 16 | Same here. | |
| 31 | I wonder if we could leave the original reference? Maybe originally based on: ... | |
| 62 | Can we put these down before the DEBUG section and then also break the long line? | |
| 97 | Can we break these into something less long and more readable as well? #define XXX(...)<tab><tab>..<tab>\
({ \
...;<tab><tab>..<tab>\
...;<tab><tab>..<tab>\
}) | |
| 172 | Can you make this declaration\n\ndiv_reg assignment? Also spaces around the "=" | |
| 177 | remove empty line | |
| 178 | Generally people prefer a 2nd block in case the first if is also a block or at least the device_printf on the next line. Would be nice to split the two device_printfs long lines but probably no longer required (though still pre-dominant coding style). I'll leave that to you. | |
| 436–437 | I don't know the original magic number. Is it worth #define-ing it somewhere? | |
| 436–437 | Why is there a single "else" outside two FDT blocks? That cannot work on kernels w/o FDT support? Should it be an else if in the next line and then we do not need two #ifdef FDT anymore? | |
| 438 | Can you please bring it back like this to keep declaration and assignment apart? | |
| 438 | Is there a space between the tabs? git diff may show in red; Phabricator is really not good for this. | |
| 440 | Spaces around the "=" | |
| 456–457 | remove empty line. | |
| 459 | Looks like spaces and tabs mixed in Phabricator; please check in file. | |
| 463 | remove empty line | |
| 465 | and again. | |
| 469 | We can now remove this line. | |
| 589 | Can you tab-aling the 2nd parts under i2c_detach for all the DEVMETHODs? | |
| sys/dev/iicbus/controller/vybrid/vf_i2c.h | ||
| 17 | Can you please change them to match the template in /usr/share/examples/etc/bsd-style-copyright? The web version needs to be fixed. | |
| 58 | Silly thought: can we move the optional clock to the end of the struct? Keeps the rest of the layout the same this way. | |
| sys/dev/iicbus/controller/vybrid/vf_i2c_acpi.c | ||
| 16 | please also here `` '' | |
| 62 | Can we name them vf_i2c_...? | |
| 78 | vf_i2c_...? | |
| 92 | and then here too vf_i2c_...? | |
| sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c | ||
| 55 | Would call it vf_i2c_compat_data | |
| 58 | I would put the 0 there too aligned under the HW_.. | |
| 68 | and here too then | |
| 84 | and again here. | |
| 88 | remove empty line | |
| 92 | remove empty line | |
| 94 | remove empty line | |
| 99 | Where does the "magic number" come from. Should we #define it or add a comment? | |
| sys/modules/Makefile | ||
| 690 | Phabricator again makes it hard to see if this is properly tab aligned? | |
| sys/modules/vf_i2c/Makefile | ||
| 2 | remove empty line. | |
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
|---|---|---|
| 436–437 | w/o FDT, freq is necessarily 0 at this point if it was not UINT32_MAX, so div_reg should be set to the slowest setting. | |
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
|---|---|---|
| 436–437 | Ok, I had to look at the code in the plain file w/o the changes to see it. If I unroll it into two different versions I get something like: #ifndef FDT
if (sc->freq == UINT32_MAX)
return;
else
div_reg = vf610_div_table[nitems(vf610_div_table) - 1].reg_val;
#else
if (sc->freq == UINT32_MAX)
return;
if (sc->hwtype == HW_MVF600)
div_reg = 0x14;
else if (sc->freq == 0)
div_reg = vf610_div_table[nitems(vf610_div_table) - 1].reg_val;
else {
uint64_t clk_freq;
...
}
#endifGiven the first if() is a return we can just factor it out in both cases and be done with it. So we can change this into: if (sc->freq == UINT32_MAX)
return;
#ifndef FDT
div_reg = vf610_div_table[nitems(vf610_div_table) - 1].reg_val;
#else
if (sc->hwtype == HW_MVF600)
div_reg = 0x14;
else if (sc->freq == 0)
div_reg = vf610_div_table[nitems(vf610_div_table) - 1].reg_val;
else {
uint64_t clk_freq;
...
}
#endifYes this duplicates the one line but it's a lot easier to read. The question is: is this the logic in both cases what was expected? | |
| sys/dev/iicbus/controller/vybrid/vf_i2c.h | ||
|---|---|---|
| 58 | Sorry for another annoying request; given this is public now, can we also rename the softc to vf_i2c_softc like the other two controllers next are rk_i2c_soft and cdnc_i2c_softc? | |
Comment Actions
Thank you for all the changes. I think we are getting pretty close to getting this in.
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
|---|---|---|
| 444 | i am not sure if this is where it originates: This is an FDT boot (again with a previous version) complaining about a LOR: simplebus0: <memory-controller@2240000> mem 0x2240000-0x225ffff irq 14 disabled compat fsl,ifc (no driver attached) vf_i2c_fdt0: <Vybrid Family Inter-Integrated Circuit (I2C)> mem 0x2000000-0x200ffff irq 15 on simplebus0 iicbus0: <OFW I2C bus> on vf_i2c_fdt0 lock order reversal: (sleepable after non-sleepable) 1st 0xffffa0800188bf40 vf_i2c_fdt0 (I2C, sleep mutex) @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c.c:436 2nd 0xffff000000ddf508 Clock topology lock (Clock topology lock, sx) @ /usr/src/sys/dev/clk/clk.c:1181 lock order I2C -> Clock topology lock attempted at: #0 0xffff0000005289f4 at witness_checkorder+0xa78 #1 0xffff0000004be71c at _sx_slock_int+0x70 #2 0xffff00000015b4ec at clk_get_freq+0x44 #3 0xffff000000961128 at i2c_reset+0x90 #4 0xffff0000001b7918 at ofw_iicbus_attach+0x11c #5 0xffff0000004ef574 at device_attach+0x3fc #6 0xffff0000004ef0dc at device_probe_and_attach+0x80 #7 0xffff0000004f0aa4 at bus_generic_attach+0x1c #8 0xffff000000960b74 at vf_i2c_attach_common+0x128 #9 0xffff0000004ef574 at device_attach+0x3fc #10 0xffff0000004ef0dc at device_probe_and_attach+0x80 #11 0xffff0000004f16e8 at bus_generic_new_pass+0x100 #12 0xffff0000004f1698 at bus_generic_new_pass+0xb0 #13 0xffff0000004f1698 at bus_generic_new_pass+0xb0 #14 0xffff0000004f1698 at bus_generic_new_pass+0xb0 #15 0xffff0000004ebf74 at bus_set_pass+0x50 #16 0xffff0000004342d0 at mi_startup+0x1e0 #17 0xffff0000000008a8 at virtdone+0x68 | |
Comment Actions
FYI: sys/dev/clk/clk.c reports:
/* * Locking - we use three levels of locking: * - First, topology lock is taken. This one protect all lists. * - Second level is per clknode lock. It protects clknode data. * - Third level is outside of this file, it protect clock device registers. * First two levels use sleepable locks; clock device can use mutex or sx lock. */
So clk_get_freq using a sleepable lock in its implementation is expected:
CLK_TOPO_SLOCK(); rv = clknode_get_freq(clknode, freq); CLK_TOPO_UNLOCK();
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
|---|---|---|
| 444 | I think there might be a race condition between device_attach and i2c_reset. I will try using the sc->mutex to prevent it. | |
Comment Actions
Modifying vf_i2c.c, vf_i2c_fdt.c and vf_i2c_acpi.c so sc->mutex is used for the attach and detach functions as well, in order to prevent race conditions, in particular with the reset function that is called in parallel with the attach function at boot time.
| sys/dev/iicbus/controller/vybrid/vf_i2c.c | ||
|---|---|---|
| 444 | I assume I fixed it, but I am using ACPI, so if you could confirm it would be great! Thanks! | |
Comment Actions
We'll need to adjust that slightly in a different direction ...
@manu can you lend a hand? Probably a lot easier for you than for me.
vf_i2c_fdt0: <Vybrid Family Inter-Integrated Circuit (I2C)> mem 0x2000000-0x200ffff irq 15 on simplebus0
uma_zalloc_debug: zone "malloc-16" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-16" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
lock order reversal: (sleepable after non-sleepable)
1st 0xffffa08001887eb8 vf_i2c_fdt0 (I2C, sleep mutex) @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
2nd 0xffff000000de0508 Clock topology lock (Clock topology lock, sx) @ /usr/src/sys/dev/clk/clk.c:1513
lock order I2C -> Clock topology lock attempted at:
uma_zalloc_debug: zone "malloc-32" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-64" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-16" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
lock order reversal: (sleepable after non-sleepable)
1st 0xffffa08001887eb8 vf_i2c_fdt0 (I2C, sleep mutex) @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
2nd 0xffff000000fd12d8 sysctl lock (sysctl lock, sleepable rm) @ /usr/src/sys/kern/kern_sysctl.c:888
lock order I2C -> sysctl lock attempted at:
uma_zalloc_debug: zone "malloc-128" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-16" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-32" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-16" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-128" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-16" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-32" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-16" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-128" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
..
uma_zalloc_debug: zone "malloc-32" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
iicbus0: <OFW I2C bus> on vf_i2c_fdt0
uma_zalloc_debug: zone "malloc-16" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-128" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-32" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
uma_zalloc_debug: zone "malloc-32" with the following non-sleepable locks held:
exclusive sleep mutex vf_i2c_fdt0 (I2C) r = 0 (0xffffa08001887eb8) locked @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c_fdt.c:87
stack backtrace:
panic: _mtx_lock_sleep: recursed on non-recursive mutex vf_i2c_fdt0 @ /usr/src/sys/dev/iicbus/controller/vybrid/vf_i2c.c:444
cpuid = 0
time = 1
KDB: stack backtrace:
db_trace_self() at db_trace_self
db_trace_self_wrapper() at db_trace_self_wrapper+0x38
vpanic() at vpanic+0x1a4
panic() at panic+0x48
mtx_lock_sleep() at mtx_lock_sleep+0x42c
mtx_lock_flags() at mtx_lock_flags+0x138
i2c_reset() at i2c_reset+0x3c
ofw_iicbus_attach() at ofw_iicbus_attach+0x11c
device_attach() at device_attach+0x3fc
device_probe_and_attach() at device_probe_and_attach+0x80
bus_generic_attach() at bus_generic_attach+0x1c
vf_i2c_attach_common() at vf_i2c_attach_common+0xf8
vf_i2c_fdt_attach() at vf_i2c_fdt_attach+0x13c
device_attach() at device_attach+0x3fc
device_probe_and_attach() at device_probe_and_attach+0x80
bus_generic_new_pass() at bus_generic_new_pass+0x100
bus_generic_new_pass() at bus_generic_new_pass+0xb0
bus_generic_new_pass() at bus_generic_new_pass+0xb0
bus_generic_new_pass() at bus_generic_new_pass+0xb0
bus_set_pass() at bus_set_pass+0x50
mi_startup() at mi_startup+0x1e0
virtdone() at virtdone+0x68
KDB: enter: panic
[ thread pid 0 tid 100000 ]
Stopped at kdb_enter+0x4c: str xzr, [x19, #1280]
db>
Comment Actions
Based on the last couple of tests, it looks like the clk functions should not be called when a mutex is locked with mtx_lock?
Comment Actions
Trying to fix the "lock order reversal: (sleepable after non-sleepable)" error in FDT by moving back to an i2g_get_div_val function that gets called outside the sc->mutex lock with a WRITE call of the divider register inside the lock, that gets only called when the divider register value has been set.
Comment Actions
Moving back sc->mutex initialization into vf_i2c_attach_common as before, but now locking the mutex whenever it is needed to communicate with the controller. No longer locking it before calling ofw* and clk* functions in vf_i2c_fdt_attach
Comment Actions
The driver seems to be misbehaving with the latest changes. I think it is due to interference between the attach and the reset functions (the attach and reset functions are called in parallel, and i2c_get_div_val gets called by the reset function before sc->freq is set to UINT32_MAX). This issue combined with the lock restrictions for the clk* functions makes it tricky to get the driver properly initialized at boot time. I will have to take a look at it another day...
Comment Actions
Is there a way to prevent the reset function from being called before the driver is done attaching?
Comment Actions
Ok I think I know how this could be fixed. I should create a sc->initialized_freq flag which is set by the attach function once sc->freq has been set. The reset function should check the status of this flag after locking sc->mutex, and if it is not the case, it should cv_wait for a condition from attach. The attach function should cv_signal this condition after setting the flag. Does it look like the best strategy to you?
Comment Actions
This may not be based on your latest version but the general idea is: call i2c_update_div_val() before taking the lock, get the div_reg value back (maybe you can avoid passing it as pointer and checking for error by simply checking for sc->freq == UINT32_MAX in the caller (reset function) afterwards to see if you should change anything or not (just came to my mind, haven't checked all code paths). Then do the register writes all together under lock.
My assumption here is that it is the reset function is not called twice at the same time in parallel from the bus.
@@ -394,8 +392,8 @@ i2c_stop(device_t dev)
return (IIC_NOERR);
}
-static void
-i2c_update_div_val(device_t dev)
+static int
+i2c_update_div_val(device_t dev, uint8_t *divreg)
{
struct vf_i2c_softc *sc;
uint8_t div_reg;
@@ -403,7 +401,7 @@ i2c_update_div_val(device_t dev)
sc = device_get_softc(dev);
if (sc->freq == UINT32_MAX)
- return;
+ return (1);
#ifndef FDT
div_reg = vf610_div_table[nitems(vf610_div_table) - 1].reg_val;
#else
@@ -429,24 +427,36 @@ i2c_update_div_val(device_t dev)
}
}
#endif
- vf_i2c_dbg(sc, "Writing 0x%02X to clock divider register\n", div_reg);
- WRITE1(sc, I2C_IBFD, div_reg);
+ if (divreg != NULL)
+ *divreg = div_reg;
+ return (0);
}
static int
i2c_reset(device_t dev, u_char speed, u_char addr, u_char *oldadr)
{
struct vf_i2c_softc *sc;
+ int error;
+ uint8_t div_reg;
sc = device_get_softc(dev);
vf_i2c_dbg(sc, "i2c reset\n");
+ error = i2c_update_div_val(dev, &div_reg);
+ if (error == 1) {
+ /* No update to be done. */
+ return (IIC_NOERR);
+ } else if (error != 0) {
+ device_printf(dev, "%s: i2c_update_div_val error %d\n", __func__, error);
+ return (IIC_ESTATUS);
+ }
+
+ vf_i2c_dbg(sc, "Writing 0x%02X to clock divider register\n", div_reg);
+
mtx_lock(&sc->mutex);
WRITE1(sc, I2C_IBCR, IBCR_MDIS);
-
- i2c_update_div_val(dev);
+ WRITE1(sc, I2C_IBFD, div_reg);
WRITE1(sc, I2C_IBCR, 0x0); /* Enable i2c */
-
mtx_unlock(&sc->mutex);
return (IIC_NOERR);Comment Actions
What you describe is what I do in the latest version, but it does not work, since without the lock, i2c_get_div_val gets executed before the attach function has the chance to set the variables that are used by i2c_get_div_val. So I need to delay the call to i2c_get_div_val until the attach function completes. Without that I end up with the wrong divider value. Note that this problem seems to only occur at boot time and not if I load the driver as a module after booting.
Comment Actions
Do you enable interrupts before you need them? Do you need IBIC_BIIE for the initial READ?
Comment Actions
Do you think this is what could trigger the call to the reset function? I just tried setting BIIE after the initial read of the divider register, but it did not fix the issue. I will start doing more tests using the driver as a module and confirm that the wrong divider value is still being set due to the reset function being called too early. If it is still the case I will work on signaling a condition from the attach function
This comment was removed by pldrouin_gmail.com.
Comment Actions
I will do additional tests. There have been quite a few moving parts this week on my end (with the I2C tool I have been working on in addition to the driver), so I could have rushed my tests a bit too much and have drawn the wrong conclusions. I will do systematic tests starting from working versions.
Comment Actions
@bz Ok, so it turns out that the issue I had was not with the driver. So there was no issue with the reset function, and the current version of the Diff properly works for me with the ACPI version of the driver. Since the clk* functions are no longer called when sc->mutex is locked, I assume the issue regarding mutex ordering is also fixed for the FDT driver. So this means the driver should also be ready for you to test.