From: "Frkuska, Joshua" <joshua_frkuska@mentor.com> To: Maxim Syrchin <syrchin@dev.rtsoft.ru>, <linux-i2c@vger.kernel.org> Cc: <wsa@the-dreams.de>, <peda@axentia.se>, <Jiada_Wang@mentor.com>, <linux-kernel@vger.kernel.org>, "Zapolskiy, Vladimir" <vladimir_zapolskiy@mentor.com>, "Baxter, Jim" <Jim_Baxter@mentor.com> Subject: Re: [PATCH] i2c: imx: add slave support. v2 status Date: Thu, 1 Dec 2016 17:11:04 +0900 [thread overview] Message-ID: <31603684-e75a-a245-03d6-5447e9ba7f6b@mentor.com> (raw) In-Reply-To: <4553f82f-d5aa-8c31-5a6a-108582810b14@dev.rtsoft.ru> Hello Maxim, I have a few questions for you. Please see my comments inline below. In addition, I have modified your patch-set slightly and I would like to progress it to merger if you do not have any issues with this. I would like to sync with you here before moving forward and submitting a new patch. Thank you and best regards, Joshua On 11/01/2016 03:21 AM, Maxim Syrchin wrote: > Hello, > > Please find some comments below. > > > 31.10.2016 5:14, Frkuska, Joshua пишет: >> Hello Maxim, >> >> Thank you very much for the intermediate patch. I am in the process >> of reviewing it. Please let me clarify a few questions I have. >> >> 1. What alternative to "bus busy/bus free/IBB" polling do you have in >> mind? This seems like a reasonable approach to me. > We didn't find any suitable alternatives. The only one we're > considered was using timeout on receive (which is kind of polling of > course) >> 2. What are the major points you consider in need of refactoring? > As you can see we have implemented FSM in slave thread. > - Due to lack of time all master functionality had not been included > in State Machine. Currently there seems to not be a problem of entirely handling master functionality in the slave state machine as it is handled outside of it. Do you feel everything should be handled in the slave state machine? I dont see any holes in the logic currently. > - wait_event_timeout() calls are used in every event handler (obviosly > it is better to have only one wait function) It is possible to have a single wait_event_timeout call at the expense of a bit of conditional logic in i2c_imx_slave_threadfn but this brings me to my next comment > - Need to review state switching code I have reviewed all states in a state transition table and all of them seem well defined. My only question here is in regards to the STATE_SP state. I would like to understand your motivation for it. To me it seems like this can also be handled in STATE_IDLE but I would like to get your reasoning behind it >> 3. You mention race conditions being fixed in this version relating >> to bus-locking by the slave and breaking slave transactions by the >> master. Does this mean mixed slave/master mode works to your >> satisfaction? If not, what work still needs to be done here? > Yes mixed slave/master mode works ok. It had passed long-lasting > stress tests (async message exchange of two imx6 boards connected > together by i2c bus ) >> 4. You mention the need for a slave locking test and a work-around >> (checking IMX_I2C_I2DR and IBB) being in-place. Why is this >> work-around not sufficient? > By the time we discovered I2DR workaround we went far from version 2 > of driver and it wasn't tested. I'm sure that I2DR workaround will > improve stability, but I do not know if it will fix all issues (i.e. > passing of stress tests ) > > > Best Regards, > Maxim Syrchin >> Thanks again, >> >> Joshua >> >> >> On 10/28/2016 04:38 AM, Maxim Syrchin wrote: >>> Hi, >>> Sorry for huge delay in answering. Unfortunately we don't have >>> enough resources now to prepare clean enough patch to be accepted by >>> community. >>> Please find the latest version attached. Driver has passed stress >>> tests, but looks like it need seriuos refactoring (it is >>> unnecessarily complicated). >>> We still have polling in slave code. Since imx doesn't generate >>> interrupt on "bus busy"/"bus free" events we have to test IBB bit in >>> polling loop. >>> Comparing to v2 version several race conditions were fixed (bus >>> locking by slave, breaking slave transaction by starting master >>> xfer). v2 is working pretty good in slave-only or master-only mode. >>> It is reasonable to add slave locking test - sometimes imx6 hw >>> doesn't release data line. As workaround we use dummy reading of >>> IMX_I2C_I2DR if driver is in slave mode and IBB bit is set for a >>> long time. >>> >>> Thanks, >>> Maxim Syrchin >>> >>> >>> 27.10.2016 10:31, Frkuska, Joshua пишет: >>>> Hi Maxim, Dmitriy, Wolfram, >>>> >>>> If there is no immediate plan for a third release of the below >>>> patch set, would it be possible to continue with merging v2 after >>>> addressing the remaining concerns? >>>> >>>> >>>> Thank you and regards, >>>> >>>> Joshua >>>>> Hi Maxim, >>>>> >>>>> On 2016-03-04 11:06:10 in the thread "Re: [PATCH] i2c: imx: add >>>>> slave support. v2" >>>>> referenced here: https://patchwork.ozlabs.org/patch/573353/ you said: >>>>>> Hi Wolfram, >>>>>> I'm now working on creating new driver version. I think I'll be >>>>>> able to >>>>>> sent it soon. >>>>> Do you still plan to release a driver update for an i2c imx driver >>>>> slave support? >>>>> >>>>> Best regards, >>>>> Jim Baxter >>>>> >>> >> >
WARNING: multiple messages have this Message-ID (diff)
From: "Frkuska, Joshua" <joshua_frkuska@mentor.com> To: Maxim Syrchin <syrchin@dev.rtsoft.ru>, linux-i2c@vger.kernel.org Cc: wsa@the-dreams.de, peda@axentia.se, Jiada_Wang@mentor.com, linux-kernel@vger.kernel.org, "Zapolskiy, Vladimir" <vladimir_zapolskiy@mentor.com>, "Baxter, Jim" <Jim_Baxter@mentor.com> Subject: Re: [PATCH] i2c: imx: add slave support. v2 status Date: Thu, 1 Dec 2016 17:11:04 +0900 [thread overview] Message-ID: <31603684-e75a-a245-03d6-5447e9ba7f6b@mentor.com> (raw) In-Reply-To: <4553f82f-d5aa-8c31-5a6a-108582810b14@dev.rtsoft.ru> Hello Maxim, I have a few questions for you. Please see my comments inline below. In addition, I have modified your patch-set slightly and I would like to progress it to merger if you do not have any issues with this. I would like to sync with you here before moving forward and submitting a new patch. Thank you and best regards, Joshua On 11/01/2016 03:21 AM, Maxim Syrchin wrote: > Hello, > > Please find some comments below. > > > 31.10.2016 5:14, Frkuska, Joshua пишет: >> Hello Maxim, >> >> Thank you very much for the intermediate patch. I am in the process >> of reviewing it. Please let me clarify a few questions I have. >> >> 1. What alternative to "bus busy/bus free/IBB" polling do you have in >> mind? This seems like a reasonable approach to me. > We didn't find any suitable alternatives. The only one we're > considered was using timeout on receive (which is kind of polling of > course) >> 2. What are the major points you consider in need of refactoring? > As you can see we have implemented FSM in slave thread. > - Due to lack of time all master functionality had not been included > in State Machine. Currently there seems to not be a problem of entirely handling master functionality in the slave state machine as it is handled outside of it. Do you feel everything should be handled in the slave state machine? I dont see any holes in the logic currently. > - wait_event_timeout() calls are used in every event handler (obviosly > it is better to have only one wait function) It is possible to have a single wait_event_timeout call at the expense of a bit of conditional logic in i2c_imx_slave_threadfn but this brings me to my next comment > - Need to review state switching code I have reviewed all states in a state transition table and all of them seem well defined. My only question here is in regards to the STATE_SP state. I would like to understand your motivation for it. To me it seems like this can also be handled in STATE_IDLE but I would like to get your reasoning behind it >> 3. You mention race conditions being fixed in this version relating >> to bus-locking by the slave and breaking slave transactions by the >> master. Does this mean mixed slave/master mode works to your >> satisfaction? If not, what work still needs to be done here? > Yes mixed slave/master mode works ok. It had passed long-lasting > stress tests (async message exchange of two imx6 boards connected > together by i2c bus ) >> 4. You mention the need for a slave locking test and a work-around >> (checking IMX_I2C_I2DR and IBB) being in-place. Why is this >> work-around not sufficient? > By the time we discovered I2DR workaround we went far from version 2 > of driver and it wasn't tested. I'm sure that I2DR workaround will > improve stability, but I do not know if it will fix all issues (i.e. > passing of stress tests ) > > > Best Regards, > Maxim Syrchin >> Thanks again, >> >> Joshua >> >> >> On 10/28/2016 04:38 AM, Maxim Syrchin wrote: >>> Hi, >>> Sorry for huge delay in answering. Unfortunately we don't have >>> enough resources now to prepare clean enough patch to be accepted by >>> community. >>> Please find the latest version attached. Driver has passed stress >>> tests, but looks like it need seriuos refactoring (it is >>> unnecessarily complicated). >>> We still have polling in slave code. Since imx doesn't generate >>> interrupt on "bus busy"/"bus free" events we have to test IBB bit in >>> polling loop. >>> Comparing to v2 version several race conditions were fixed (bus >>> locking by slave, breaking slave transaction by starting master >>> xfer). v2 is working pretty good in slave-only or master-only mode. >>> It is reasonable to add slave locking test - sometimes imx6 hw >>> doesn't release data line. As workaround we use dummy reading of >>> IMX_I2C_I2DR if driver is in slave mode and IBB bit is set for a >>> long time. >>> >>> Thanks, >>> Maxim Syrchin >>> >>> >>> 27.10.2016 10:31, Frkuska, Joshua пишет: >>>> Hi Maxim, Dmitriy, Wolfram, >>>> >>>> If there is no immediate plan for a third release of the below >>>> patch set, would it be possible to continue with merging v2 after >>>> addressing the remaining concerns? >>>> >>>> >>>> Thank you and regards, >>>> >>>> Joshua >>>>> Hi Maxim, >>>>> >>>>> On 2016-03-04 11:06:10 in the thread "Re: [PATCH] i2c: imx: add >>>>> slave support. v2" >>>>> referenced here: https://patchwork.ozlabs.org/patch/573353/ you said: >>>>>> Hi Wolfram, >>>>>> I'm now working on creating new driver version. I think I'll be >>>>>> able to >>>>>> sent it soon. >>>>> Do you still plan to release a driver update for an i2c imx driver >>>>> slave support? >>>>> >>>>> Best regards, >>>>> Jim Baxter >>>>> >>> >> >
next prev parent reply other threads:[~2016-12-01 8:11 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-10-27 7:31 Re: [PATCH] i2c: imx: add slave support. v2 status Frkuska, Joshua 2016-10-27 7:31 ` Frkuska, Joshua 2016-10-27 19:38 ` Maxim Syrchin 2016-10-31 2:14 ` Frkuska, Joshua 2016-10-31 2:14 ` Frkuska, Joshua 2016-10-31 18:21 ` Maxim Syrchin 2016-12-01 8:11 ` Frkuska, Joshua [this message] 2016-12-01 8:11 ` Frkuska, Joshua
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=31603684-e75a-a245-03d6-5447e9ba7f6b@mentor.com \ --to=joshua_frkuska@mentor.com \ --cc=Jiada_Wang@mentor.com \ --cc=Jim_Baxter@mentor.com \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=peda@axentia.se \ --cc=syrchin@dev.rtsoft.ru \ --cc=vladimir_zapolskiy@mentor.com \ --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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.