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

[Paul Boddie]

On Monday, 28 December 2020 19:03:51 CET H. Nikolaus Schaller wrote:
> 
> Yes, that worked. What I have found is that interrupts are only generated
> if I2CSR.BUSY is set. This is activated by sending a STA condition and ends
> with sending the STO.
> 
> The other status bits have no direct influence. Or I can't see it.
> To further analyse it would probably be necessary to slow down the i2c bus
> speed to the minimum. So that sending a STA bit or data byte takes longer
> than the printk. But that is far beyond my time budget.

If I can get to investigating with the Minibook then I will try and write some 
tests. Currently, I'm occupied with some other catching-up activities.

> Anyways it might be possible to implement I2C_M_NOSTART and I2C_M_STOP
> except for the first and the last message in an xfer(). If this is tried,
> the interrupts will probably lock up...
> 
> It also appears as if the 1.5.3 Read Operation flowchart is a little
> incomplete that one should send an STO at the end...

It occurred to me that there might have been a missing operation involved, and 
given that the JZ4730 controls the bus, it would surely be its responsibility 
to issue the stop condition. Otherwise, I can imagine that the CPU asserts a 
repeated start condition which will obviously keep the bus busy.

> And what is also not clear if the interrupt/state machine can stall if
> I2C_M_REV_DIR_ADDR is set and both, the chip and the jz4730 are sending or
> receiving.
> 
> 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.

This sounds reasonable enough.

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

I suppose that is progress. :-)

> I still have the printk sched_clock timer issue. Which also may have
> an influence since I don't know if it is used by
> wait_for_completion_timeout() and triggers "early" timeout. Then I would
> try to debug this in the i2c driver...

[...]

> To me the pattern looks like integer overflow during
> multiplication / division or expansion from 32 bit
> to 64 bit time value.
> 
> Question: how does sched_clock() work on the jz4730?
> 
> Obviously it does not use the simple jiffies based clock:
> 
> https://elixir.bootlin.com/linux/latest/source/kernel/sched/clock.c#L64
> 
> It likely does use
> 
> https://elixir.bootlin.com/linux/latest/source/kernel/time/sched_clock.c#L82
> 
> But I could not identify a call to sched_clock_register()
> or how the setup works. And which registers sched_clock() reads.
> 
> So we probably should focus on that first before I
> continue with the i2c driver.

So, among the other things I have been looking at is this. I went searching 
for timer/counter details starting at time_init (arch/mips/kernel/time.c). 
This led me to some likely definitions applicable to the JZ4730 such as 
init_r4k_clocksource (arch/mips/kernel/csrc-r4k.c) which uses a variable 
called mips_hpt_frequency that is defined in plat_time_init (arch/mips/
generic/init.c).

It is in the plat_time_init function that an attempt is made to get a clock, 
but we haven't defined one in the device tree for the JZ4730. So, with the 
assumption that this might make a difference, I have made a patch that should 
add a CPU node with a reference to the appropriate clock.

Paul
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-missing-CPU-node-to-make-timer-available.patch
Type: text/x-patch
Size: 981 bytes
Desc: not available
URL: <http://lists.goldelico.com/pipermail/letux-kernel/attachments/20201228/db2c657b/attachment.bin>