lkml.org 
[lkml]   [2023]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/2] i2c: designware: fix __i2c_dw_disable in case master is holding SCL low
From
Hi,

Le 11/08/2023 à 15:59, Andy Shevchenko a écrit :
> On Fri, Aug 11, 2023 at 02:46:23PM +0200, Yann Sionneau wrote:
>> From: Yann Sionneau <ysionneau@kalray.eu>
>>
>> The designware IP can be synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN
> DesignWare
Ack!
>
>> parameter.
>> In which case, if the TX FIFO gets empty and the last command didn't have
> "In this case when the..."
Ack!
>
>> the STOP bit (IC_DATA_CMD[9]), the dw_apb_i2c will hold SCL low until
> "the controller will..."
Ack!
>
>> a new command is pushed into the TX FIFO or the transfer is aborted.
>>
>> When the dw_apb_i2c is holding SCL low, it cannot be disabled.
> "When the controller..."
Ack!
>
>> The transfer must first be aborted.
>> Also, the bus recover won't work because SCL is held low by the master.
>>
>> This patch checks if the master is holding SCL low in __i2c_dw_disable
> Grep for "This patch" in the Submitting Patches document and fix this
> accordingly.
Ok I didn't know, ack!
>
> __i2c_dw_disable()
>
>> before trying to disable the IP.
>> If SCL is held low, an abort is initiated.
>> When the abort is done, the disabling can then proceed.
>>
>> This whole situation can happen for instance during SMBUS read data block
>> if the slave just responds with "byte count == 0".
>> This puts the master in an unrecoverable state, holding SCL low and the
>> current __i2c_dw_disable procedure is not working. In this situation
> __i2c_dw_disable()
>
>> only a Linux reboot can fix the i2c bus.
> If reboot helps, what magic does it do that Linux OS can't repeat in software?
> Please, elaborate more.
Sorry I was not very clear. In fact I meant a SoC reset, not a Linux
reboot. It's just that on our SoC with boot-from-flash a reset will also
reboot the Linux. But indeed what fixes the issue is the reset of the SoC.
>
> ...
>
>> int timeout = 100;
>> u32 status;
>> + u32 raw_intr_stats;
>> + u32 enable;
>> + bool abort_needed;
>> + bool abort_done = false;
> Perhaps reversed xmas tree order?
Oh, I didn't know about this, thanks, ack!
>
> bool abort_done = false;
> bool abort_needed;
> u32 raw_intr_stats;
> int timeout = 100;
> u32 status;
> u32 enable;
>
> ...
>
>> + abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
>> + if (abort_needed)
>> + regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
>>
>> do {
>> + if (abort_needed && !abort_done) {
>> + regmap_read(dev->map, DW_IC_ENABLE, &enable);
>> + abort_done = !(enable & DW_IC_ENABLE_ABORT);
>> + continue;
> This will exhaust the timeout and below can be run at most once,
> is it a problem?
I was also wondering about this... I can propose to extract this in 2
loops. First loop to wait for the abort to finish, with its own timeout.
Then the untouched second loop that waits for the disabling to finish.
>
> Also it's a tight busyloop, are you sure it's what you need?

I don't know, I would expect that this would not take much time, I
already have a V2 for this patch with all your remarks taken into
account, including the splitting into 2 loops (previous comment).

I am waiting before sending it to have the opportunity to test it on the
real device, it will be done on the August 21st since I am in holidays
for now.

I will print the number of iterations it takes for the abort to finish.
If the abort is quick, maybe there is no need for sleeping. I didn't see
any info about the time it takes inside the IP documentation.

Thanks for the review!

I'll get back to you when I have done the testing of the V2 patch :) (or
maybe you want it on the mailing list now as an RFC ?)

--

Yann

\
 
 \ /
  Last update: 2023-08-17 16:43    [W:0.048 / U:0.412 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site