From: "Noralf Trønnes" <noralf@tronnes.org>
To: Stefan Wahren <stefan.wahren@i2se.com>,
Eric Anholt <eric@anholt.net>,
wsa@the-dreams.de, swarren@wwwdotorg.org
Cc: linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
linux-rpi-kernel@lists.infradead.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts
Date: Sun, 2 Oct 2016 16:19:57 +0200 [thread overview]
Message-ID: <0c943ee9-d152-79c4-2ad7-8a88c2d9c960@tronnes.org> (raw)
In-Reply-To: <1708178720.178936.b155d019-5320-4754-bbe2-4afc1ae71cd6.open-xchange@email.1und1.de>
Den 29.09.2016 07:37, skrev Stefan Wahren:
>> Noralf Trønnes <noralf@tronnes.org> hat am 29. September 2016 um 00:22
>> geschrieben:
>>
>>
>>
>> Den 29.09.2016 00:00, skrev Eric Anholt:
>>> Noralf Trønnes <noralf@tronnes.org> writes:
>>>
>>>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>>>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>>>> In this case the controller has to be disabled and the transfer
>>>> completed to avoid hang.
>>>>
>>>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>>>> the controller is disabled in the transfer function after completion.
>>>> Unite the code paths and do disabling inside the interrupt routine.
>>>>
>>>> Clear interrupt status bits in the united completion path instead of
>>>> trying to do it on every interrupt which isn't necessary.
>>>> Only CLKT, ERR and DONE can be cleared that way.
>>>>
>>>> Add the status value to the error value in case of TXW/RXR errors to
>>>> distinguish them from the other S_LEN error.
>>> I was surprised that not writing the TXW/RXR bits on handling their
>>> interrupts was OK, given that we were doing so before, but it's a level
>>> interrupt and those bits are basically ignored on write.
>>>
>>> This patch and 3, 4, and 6 are:
>>>
>>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>>
>>> Patch 5 is:
>>>
>>> Acked-by: Eric Anholt <eric@anholt.net>
>>>
>>> Note for future debug: The I2C_C_CLEAR on errors will take some time to
>>> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
>>> abort_rx flag and runs through the state machine to send a NACK and a
>>> STOP, I think. Since we're setting CLEAR without I2CEN, that NACK will
>>> be hanging around queued up for next time we start the engine.
>> Maybe you're able to explain the issues I had with reset:
>> https://github.com/raspberrypi/linux/issues/1653
>>
>> Should we put your note into the commit message?
>> It will most likely be lost if it just stays in this email.
> I prefer to have this kind of information as a code comment.
Eric, does this look good to you as a code comment:
/*
* Note about I2C_C_CLEAR on error:
* The I2C_C_CLEAR on errors will take some time to resolve -- if you
were in
* non-idle state and I2C_C_READ, it sets an abort_rx flag and runs through
* the state machine to send a NACK and a STOP. Since we're setting CLEAR
* without I2CEN, that NACK will be hanging around queued up for next time
* we start the engine.
*/
If it is, I'll resend the series with this change and add all the ack's
and r-b's.
Noralf.
next prev parent reply other threads:[~2016-10-02 14:20 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-28 17:50 [PATCH v3 0/7] i2c: bcm2835: Bring in changes from downstream Noralf Trønnes
2016-09-28 17:50 ` [PATCH v3 1/7] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes Noralf Trønnes
2016-09-28 17:50 ` [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts Noralf Trønnes
2016-09-28 22:00 ` Eric Anholt
2016-09-28 22:22 ` Noralf Trønnes
2016-09-29 5:37 ` Stefan Wahren
2016-10-02 14:19 ` Noralf Trønnes [this message]
2016-10-03 18:58 ` Eric Anholt
2016-10-03 19:42 ` Eric Anholt
2016-10-04 19:24 ` Noralf Trønnes
2016-09-28 17:50 ` [PATCH v3 3/7] i2c: bcm2835: Use dev_dbg logging on transfer errors Noralf Trønnes
2016-09-29 11:05 ` Martin Sperl
2016-09-28 17:50 ` [PATCH v3 4/7] i2c: bcm2835: Can't support I2C_M_IGNORE_NAK Noralf Trønnes
2016-09-28 17:50 ` [PATCH v3 5/7] i2c: bcm2835: Add support for Repeated Start Condition Noralf Trønnes
2016-09-28 17:50 ` [PATCH v3 6/7] i2c: bcm2835: Support i2c-dev ioctl I2C_TIMEOUT Noralf Trønnes
2016-09-28 17:50 ` [PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock Noralf Trønnes
2016-09-28 21:24 ` Eric Anholt
2016-09-28 22:10 ` Noralf Trønnes
2016-09-29 8:02 ` Martin Sperl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0c943ee9-d152-79c4-2ad7-8a88c2d9c960@tronnes.org \
--to=noralf@tronnes.org \
--cc=eric@anholt.net \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rpi-kernel@lists.infradead.org \
--cc=stefan.wahren@i2se.com \
--cc=swarren@wwwdotorg.org \
--cc=wsa@the-dreams.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).