Page MenuHomeFreeBSD

Splitting the existing Vybrid I2C Controller Driver to Add ACPI Support
ClosedPublic

Authored by pldrouin_gmail.com on Feb 22 2024, 2:11 AM.
Referenced Files
F81335291: D44020.id135447.diff
Sun, Apr 14, 5:41 PM
F81335290: D44020.id135446.diff
Sun, Apr 14, 5:41 PM
F81335289: D44020.id135444.diff
Sun, Apr 14, 5:41 PM
F81327682: D44020.id135006.diff
Sun, Apr 14, 3:07 PM
F81323013: D44020.id134926.diff
Sun, Apr 14, 12:53 PM
Unknown Object (File)
Mon, Apr 8, 11:51 PM
Unknown Object (File)
Mon, Apr 8, 10:39 PM
Unknown Object (File)
Mon, Apr 8, 10:39 PM

Details

Summary

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.

Test Plan

-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

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
pldrouin_gmail.com edited the summary of this revision. (Show Details)

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?
Also spaces around the "="

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.

pldrouin_gmail.com marked 33 inline comments as done.

Implementing all but one of bz's recommendations, pending feedback from comment.

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.
Below is based on the previous version still with manual adjustments to the whitespace (so ignore the whitespace this time ;-)

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;
                ...
        }
#endif

Given 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;
                ...
        }
#endif

Yes 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?
Or did I miss something or screw it up?

Modifying the code to make it more readable

Yes it is equivalent. I updated the code.

bz requested changes to this revision.Feb 26 2024, 7:08 AM
bz added inline comments.
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?

This revision now requires changes to proceed.Feb 26 2024, 7:08 AM
  • Renaming driver's data structure
pldrouin_gmail.com marked an inline comment as done.

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

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.

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.

pldrouin_gmail.com added inline comments.
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!

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();

Thanks, it was helpful!

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>

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?

Please ignore this revision

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.

I think I will have to make additional changes...

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

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...

Is there a way to prevent the reset function from being called before the driver is done attaching?

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?

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?

I think the fix may be easier; give me a few minutes.

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);
In D44020#1009469, @bz wrote:

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.

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.

In D44020#1009469, @bz wrote:

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.

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.

Do you enable interrupts before you need them? Do you need IBIC_BIIE for the initial READ?

In D44020#1009471, @bz wrote:
In D44020#1009469, @bz wrote:

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.

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.

Do you enable interrupts before you need them? Do you need IBIC_BIIE for the initial READ?

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

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.

@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.

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Mar 29, 11:14 PM
This revision was automatically updated to reflect the committed changes.