Re: [PATCH v2 1/3] i2c: s3c24xx: fix read transfers in polling mode

[Marek Szyprowski <m.szyprowski@samsung.com>]

On 02.11.2023 01:49, Andi Shyti wrote:
> On Tue, Oct 31, 2023 at 02:49:04PM +0100, Marek Szyprowski wrote:
>> On 27.10.2023 15:39, Andi Shyti wrote:
>>> On Wed, Oct 25, 2023 at 02:17:23PM +0200, Marek Szyprowski wrote:
>>>> To properly handle read transfers in polling mode, no waiting for the ACK
>>>> state is needed as it will never come. Just wait a bit to ensure start
>>>> state is on the bus and continue processing next bytes.
>>>>
>>>> Fixes: 117053f77a5a ("i2c: s3c2410: Add polling mode support")
>>>> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com>
>>>> ---
>>>>    drivers/i2c/busses/i2c-s3c2410.c | 9 +++++++--
>>>>    1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
>>>> index 127eb3805fac..f9dcb1112a61 100644
>>>> --- a/drivers/i2c/busses/i2c-s3c2410.c
>>>> +++ b/drivers/i2c/busses/i2c-s3c2410.c
>>>> @@ -216,8 +216,13 @@ static bool is_ack(struct s3c24xx_i2c *i2c)
>>>>    	int tries;
>>>>    
>>>>    	for (tries = 50; tries; --tries) {
>>>> -		if (readl(i2c->regs + S3C2410_IICCON)
>>>> -			& S3C2410_IICCON_IRQPEND) {
>>>> +		unsigned long tmp = readl(i2c->regs + S3C2410_IICCON);
>>>> +
>>>> +		if (!(tmp & S3C2410_IICCON_ACKEN)) {
>>>> +			usleep_range(100, 200);
>>>> +			return true;
>>> What is the real issue here? Is the value of S3C2410_IICCON_ACKEN
>>> enabling/disabling irq's?
>> It is not about the enabling/disabling interrupts, but controlling the
>> bus state. This bit is named as 'Acknowledge generation / I2C-bus
>> acknowledge enable bit' in Exynos reference manual:
>>
>> In Tx mode, the I2CSDA is idle in the ACK time.
>>
>> In Rx mode, the I2CSDA is low in the ACK time.
>>
>> So it is a part of proper controlling the bus state, not the reported
>> interrupts, although the S3C2410_IICCON_ACKEN name is a bit misleading
>> in this case.
> Yes, correct, but I still don't understand this sequence in the
> message_start:
>
>   - enable ACKEN in IICCON (enable_ack())
>   - read IICCON (iiccon = readl(...))
>   - write what you read in IICCON (writel(iiccon, ...))
>   - if ACKEN is disabled in IICCON (from your patch)
>
> Where is supposed ACKEN to be disabled?

It must be cleared by the hardware when read mode is set. Frankly 
speaking I didn't dig much into the driver internals and respective i2c 
bus states. I just logged all the call paths and driver states from the 
driver operating with interrupts enabled and fixed the polling mode to 
match what I have captured with interrupts enabled. After that changes, 
all my test boards finally booted properly with polling mode enabled 
unconditionally. This ACK related logic is a bit strange, but I don't 
really want to change driver logic and risk other regressions, so my 
changes were limited only to the code reachable during polling mode.

>>> Besides, if we use polling mode, shouldn't we disable the acks
>>> already in probe (even though they are disabled by default),
>>> never enable them before starting the message and avoid checking
>>> here everytime?
>> I assume that this polling mode is a special case, so there is no point
>> in optimizing it much. It is used only by the i2c core for some special
>> transfers to the PMIC during system reboot/shutdown or by the s3c24xx
>> i2c controller embedded in SoC for controlling some PHYs. Till now only
>> the second case was actually used. There were only a few single writes
>> done this way, so noone even noticed that the other types of transfers
>> (multi message or read) were broken... I found all those issues by
>> enabling polling mode unconditionally and fixing it to make all my test
>> systems working again.
> Yeah, I understand your point here.
>
> It would be nice to have a pure polling mode supported though.

Well, it is now with this patchset. It is just a matter of core to 
select it.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland