[Letux-kernel] [PATCH 00/20] A bunch of JZ4730 fixups for letux-kernel

[H. Nikolaus Schaller]

Hi Paul,

> Am 12.01.2021 um 22:32 schrieb Paul Boddie <paul at boddie.org.uk>:
> 
> On Monday, 28 December 2020 19:03:51 CET H. Nikolaus Schaller wrote:
>> 
>> To make most of these things work I have rearranged the code so that all
>> activities are done in the irq code so that messages can be spliced together
>> with or without STA in between and with or without sending the address. And
>> it becomes possible to switch between write and read on the fly. See:
>> 
>> https://www.kernel.org/doc/Documentation/i2c/i2c-protocol
>> 
>> It requires a single wait_for_completion_timeout() for the whole set of
>> xfer messages.
>> 
>> This approach also seems to relieve from the spurious interrupts problem.
>> 
>> What I have to fight against is that the irq code now directly operates
>> on the messages array and that becomes invalid after a timeout because
>> it is memory not owned by the irq handler or the driver. Could even be
>> on stack of the caller of xfer().
>> 
>> And NACK events are also tricky to handle.
>> 
>> Otherwise the reworked code starts to work. I was already able to read
>> the hwclock (once :).
> 
> We had a discussion off list about this, and I decided to modify the amended 
> Linux driver to take into account various conditions that I think need to be 
> observed to reliably perform I2C operations. Obviously, these modifications 
> are not tested in the Linux kernel because my testing environment does not 
> permit any observations (no UART, LCD not yet working), but my L4Re 
> experiments with a working LCD and with "console" output in a window have 
> permitted some observations to be made.
> 
> One thing I looked at yesterday is that the busy state of the bus has to be 
> cleared before starting a new transaction. Waiting for an interrupt and 
> testing the busy flag seems to do the trick, with a suitable timeout of up to 
> about 10ms involved if interrupts do not occur. (Given that my L4Re program 
> will mask interrupts, this might not be as complicated in an environment where 
> interrupts are always being delivered.)
> 
> I had a look at the state machine in the amended Linux driver. What I wanted 
> to do was to reproduce some of the tests performed in my experiments that 
> guard read and write operations. For example, I found that reading needs to be 
> guarded by tests for NACK (which should stop the operation) and for DRF 
> (indicating valid data for reading). Similarly, writing needs to be guarded by 
> tests for NACK and for !DRF (indicating that data can be written).
> 
> In my L4Re code, I loop waiting for interrupts to test these conditions, but I 
> think that in the Linux driver, we might just exit the interrupt handler 
> without advancing in the state machine where such conditions are not 
> satisfied. Where NACK occurs, an error condition would be signalled instead.
> 
> I have attached the driver with my own modifications for comparison with 
> previous versions. I cannot promise that it will work, but you should be able 
> to see what I have been trying to do.

Yes, the idea is good!

What I have done is to take the latest letux-5.11-rc3 and replace the i2c
driver with yours (it seems to be based on an older version of what I had
tried last, but should not matter since interfaces are the same).

Unfortunately the only effect I can see is the kernel being stuck after
a timeout:

[    0.164164] Waiting for root device /dev/mmcblk0p2...
[    0.172036] mmc0: new high speed SDHC card at address 59b4
[    0.165852] mmcblk0: mmc0:59b4 USD   7.51 GiB 
[    0.168214]  mmcblk0: p1 p2
[    0.162872] EXT4-fs (mmcblk0p2): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
[    0.173217] VFS: Mounted root (ext4 filesystem) on device 179:2.
[    0.165201] devtmpfs: mounted
[    0.172605] Freeing unused kernel memory: 340K
[    0.177319] This architecture does not have kernel memory protection.
[    0.166508] Run /sbin/init as init process
[    0.170838]   with arguments:
[    0.173991]     /sbin/init
[    0.176890]   with environment:
[    0.180217]     HOME=/
[    0.182764]     TERM=linux
[    0.183080] process '/sbin/init' started with executable stack
Mount failed for selinuxfs on /sys/fs/selinux:  No such file or directory
INIT: version 2.88 booting
[info] Using makefile-style concurrent boot in runlevel S.
[....] Starting the hotplug events dispatcher: udevd[    0.217633] systemd-udevd[763]: starting version 215
[    0.216499] random: udevd: uninitialized urandom read (16 bytes read)
[    0.231518] random: udevd: uninitialized urandom read (16 bytes read)
^[[c.
[....] Synthesizing the initial hotplug events..
[    0.258701] minipc-mcu 0-0028: chip found, driver version 2.0
[    0.248167] i2c i2c-0: irq timeout 0
[    0.257587] random: crng init done
-- then nothing happens - no reaction on serial console etc.

What I assume from earlier experiments is that not processing an IRQ doesn't
work in this case. This makes the interrupt handler being called immediately again.

AFAIR, the kernel should throttle IRQs that are not processed too often
but the broken sched-timer may stop this function.

So unfortunately that does not work.

I think I'll try to make the LCDC work first and then I can focus again on the I2C
driver. Debugging both in parallel doesn't work well...

>  For reference, the L4Re code can be 
> found here:
> 
> https://hg.boddie.org.uk/Landfall/file/d58099373644/pkg/devices/lib/i2c/src/
> jz4730.cc#l138
> 
> The set_address function contains the testing for the busy flag. Meanwhile, 
> the read and write methods contain the other testing that happens:
> 
> https://hg.boddie.org.uk/Landfall/file/d58099373644/pkg/devices/lib/i2c/src/
> jz4730.cc#l174
> 
> I hope this is informative, at least.
> 
> Paul<i2c-jz4730.c>

BR and thanks,
Nikolaus