Re: [PATCH] i2c: imx: add slave support. v2 status
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
Re: [PATCH] i2c: imx: add slave support. v2 status
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
Re: [PATCH] i2c: imx: add slave support. v2 status
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. - wait_event_timeout() calls are used in every event handler (obviosly it is better to have only one wait function) - Need to review state switching code 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
Re: [PATCH] i2c: imx: add slave support. v2 status
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. - wait_event_timeout() calls are used in every event handler (obviosly it is better to have only one wait function) - Need to review state switching code 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
Re: [PATCH] i2c: imx: add slave support. v2 status
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. 2. What are the major points you consider in need of refactoring? 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? 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? 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 -- ___ Joshua Frkuska | Embedded Software Engineer Mentor Graphics Japan Co., ltd. | +81-3-6866-7611 PRIVACY AND CONFIDENTIALITY NOTICE This email and any attachments may contain confidential or privileged information for the sole use of the intended recipient. Any review, reliance or distribution by others or forwarding without express permission is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Re: [PATCH] i2c: imx: add slave support. v2 status
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. 2. What are the major points you consider in need of refactoring? 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? 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? 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 -- ___ Joshua Frkuska | Embedded Software Engineer Mentor Graphics Japan Co., ltd. | +81-3-6866-7611 PRIVACY AND CONFIDENTIALITY NOTICE This email and any attachments may contain confidential or privileged information for the sole use of the intended recipient. Any review, reliance or distribution by others or forwarding without express permission is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Re: [PATCH] i2c: imx: add slave support. v2 status
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 From 61ae34268d78eb284bf8ee0cbe8f9f0c5e7df074 Mon Sep 17 00:00:00 2001 From: Maxim Syrchin <msyrc...@dev.rtsoft.ru> Date: Thu, 27 Oct 2016 17:37:56 +0300 Subject: [PATCH] i2c: imx: add slave support. v3 Add I2C slave provider using the generic slave interface. It also supports master transactions when the slave in the idle mode. Signed-off-by: Maxim Syrchin <msyrc...@dev.rtsoft.ru> --- drivers/i2c/busses/i2c-imx.c | 682 +-- 1 file changed, 653 insertions(+), 29 deletions(-) diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 592a8f2..11a2292 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -53,6 +53,7 @@ #include #include #include +#include /* This will be the driver name the kernel reports */ #define DRIVER_NAME "imx-i2c" @@ -171,6 +172,82 @@ enum imx_i2c_type { VF610_I2C, }; +enum i2c_imx_state { + STATE_IDLE = 0, + STATE_SLAVE, + STATE_MASTER, + STATE_SP +}; + +#define MAX_EVENTS (1<<4) +#define EUNDEFINED 4000 + +enum i2c_imx_events { + EVT_AL = 0, + EVT_SI, + EVT_START, + EVT_STOP, + EVT_POLL, + EVT_INVALID, + EVT_ENTRY +}; + +typedef struct evt_queue { + atomic_t count; + atomic_t ir; + atomic_t iw; + unsigned int evt[MAX_EVENTS]; +} evt_queue; + +static inline int evt_find_next_idx(atomic_t *v) +{ + return atomic_inc_return(v) & (MAX_EVENTS - 1); +} + +static unsigned int evt_put(evt_queue *stk, unsigned int evt) +{ + int count = atomic_inc_return(>count); + int idx; + if (count < MAX_EVENTS) + { + idx = evt_find_next_idx(>iw); + stk->evt[idx] = evt; + + return 0; + } else { + atomic_dec(>count); + return EVT_INVALID; + } +} + +static unsigned int evt_get(evt_queue *stk) +{ + int count = atomic_dec_return(>count); + int idx; + + if (count >= 0) + { + idx = evt_find_next_idx(>ir); + return stk->evt[idx]; + } else { + atomic_inc(>count); + return EVT_INVALID; + } +} + +static unsigned int evt_is_empty(evt_queue *stk) +{ + int ret = atomic_read(>count); + return (ret <= 0); +} + +static void evt_init(evt_queue *stk) +{ + atomic_set(>count,0); + atomic_set(>iw,0); + atomic_set(>ir,0); +} + struct imx_i2c_hwdata { enum imx_i2c_type devtype; unsignedregshift; @@ -193,6 +270,7 @@ struct imx_i2c_dma { struct imx_i2c_struct { struct i2c_adapter adapter; + struct i2c_client *slave; struct clk *clk; void __iomem*base; wait_queue_head_t queue; @@ -210,6 +288,18 @@ struct imx_i2c_struct { struct pinctrl_state *pinctrl_pins_gpio; struct imx_i2c_dma *dma; + + unsigned intstate; + evt_queue events; + atomic_tlast_error; + + int master_interrupt; + int start_retry_cnt; + int slave_poll_cnt; + + struct task_struct *slave_task; + wait_queue_head_t sta
Re: [PATCH] i2c: imx: add slave support. v2 status
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 From 61ae34268d78eb284bf8ee0cbe8f9f0c5e7df074 Mon Sep 17 00:00:00 2001 From: Maxim Syrchin Date: Thu, 27 Oct 2016 17:37:56 +0300 Subject: [PATCH] i2c: imx: add slave support. v3 Add I2C slave provider using the generic slave interface. It also supports master transactions when the slave in the idle mode. Signed-off-by: Maxim Syrchin --- drivers/i2c/busses/i2c-imx.c | 682 +-- 1 file changed, 653 insertions(+), 29 deletions(-) diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 592a8f2..11a2292 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -53,6 +53,7 @@ #include #include #include +#include /* This will be the driver name the kernel reports */ #define DRIVER_NAME "imx-i2c" @@ -171,6 +172,82 @@ enum imx_i2c_type { VF610_I2C, }; +enum i2c_imx_state { + STATE_IDLE = 0, + STATE_SLAVE, + STATE_MASTER, + STATE_SP +}; + +#define MAX_EVENTS (1<<4) +#define EUNDEFINED 4000 + +enum i2c_imx_events { + EVT_AL = 0, + EVT_SI, + EVT_START, + EVT_STOP, + EVT_POLL, + EVT_INVALID, + EVT_ENTRY +}; + +typedef struct evt_queue { + atomic_t count; + atomic_t ir; + atomic_t iw; + unsigned int evt[MAX_EVENTS]; +} evt_queue; + +static inline int evt_find_next_idx(atomic_t *v) +{ + return atomic_inc_return(v) & (MAX_EVENTS - 1); +} + +static unsigned int evt_put(evt_queue *stk, unsigned int evt) +{ + int count = atomic_inc_return(>count); + int idx; + if (count < MAX_EVENTS) + { + idx = evt_find_next_idx(>iw); + stk->evt[idx] = evt; + + return 0; + } else { + atomic_dec(>count); + return EVT_INVALID; + } +} + +static unsigned int evt_get(evt_queue *stk) +{ + int count = atomic_dec_return(>count); + int idx; + + if (count >= 0) + { + idx = evt_find_next_idx(>ir); + return stk->evt[idx]; + } else { + atomic_inc(>count); + return EVT_INVALID; + } +} + +static unsigned int evt_is_empty(evt_queue *stk) +{ + int ret = atomic_read(>count); + return (ret <= 0); +} + +static void evt_init(evt_queue *stk) +{ + atomic_set(>count,0); + atomic_set(>iw,0); + atomic_set(>ir,0); +} + struct imx_i2c_hwdata { enum imx_i2c_type devtype; unsignedregshift; @@ -193,6 +270,7 @@ struct imx_i2c_dma { struct imx_i2c_struct { struct i2c_adapter adapter; + struct i2c_client *slave; struct clk *clk; void __iomem*base; wait_queue_head_t queue; @@ -210,6 +288,18 @@ struct imx_i2c_struct { struct pinctrl_state *pinctrl_pins_gpio; struct imx_i2c_dma *dma; + + unsigned intstate; + evt_queue events; + atomic_tlast_error; + + int master_interrupt; + int start_retry_cnt; + int slave_poll_cnt; + + struct task_struct *slave_task; + wait_queue_head_t state_queue; + }; static const struct imx_i2c_hwdata imx1_i2c_hwdata
Re: Re: [PATCH] i2c: imx: add slave support. v2 status
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
Re: Re: [PATCH] i2c: imx: add slave support. v2 status
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
Re: [PATCH] i2c: imx: add slave support. v2
Hi Wolfram, I'm now working on creating new driver version. I think I'll be able to sent it soon. 04.03.2016 0:35, Wolfram Sang пишет: There are might be race conditions. Can you name them Most of races are fixed already. There were some issues with interrupt latencies - sometimes slave interrupt appears in process of starting master xfer. +enum imx_i2c_slave_state { + I2C_IMX_SLAVE_IDLE, + I2C_IMX_SLAVE_IRQ, + I2C_IMX_SLAVE_POLLING Highlevel question first: Why do you have polling? Why would anyone not want to use interrupts here? Since imx doesn't generate interrupt on "bus stop" condition we'd had to implement polling scheme. Interrupts are used for starting polling and for waking polling loop on new slave request. Without polling we can't handle "end-of-packet" event correctly. In current version states are: I2C_IMX_SLAVE_IDLE // default state. slave process is waiting for interrupt I2C_IMX_SLAVE_POLLING // slave transfer is in process. I2C_IMX_MASTER // master transfer is in process.
Re: [PATCH] i2c: imx: add slave support. v2
Hi Wolfram, I'm now working on creating new driver version. I think I'll be able to sent it soon. 04.03.2016 0:35, Wolfram Sang пишет: There are might be race conditions. Can you name them Most of races are fixed already. There were some issues with interrupt latencies - sometimes slave interrupt appears in process of starting master xfer. +enum imx_i2c_slave_state { + I2C_IMX_SLAVE_IDLE, + I2C_IMX_SLAVE_IRQ, + I2C_IMX_SLAVE_POLLING Highlevel question first: Why do you have polling? Why would anyone not want to use interrupts here? Since imx doesn't generate interrupt on "bus stop" condition we'd had to implement polling scheme. Interrupts are used for starting polling and for waking polling loop on new slave request. Without polling we can't handle "end-of-packet" event correctly. In current version states are: I2C_IMX_SLAVE_IDLE // default state. slave process is waiting for interrupt I2C_IMX_SLAVE_POLLING // slave transfer is in process. I2C_IMX_MASTER // master transfer is in process.
Re: [PATCH] i2c: imx: add slave support. v2
On Tue, Jan 26, 2016 at 07:14:40PM +0300, Dmitriy Baranov wrote: > Add I2C slave provider using the generic slave interface. > It also supports master transactions when the slave in the idle mode. > > Issues: > Changes work only in PIO mode (when driver doesn`t use DMA) This is fine with me. We don't support block transfers currently in the slave core anyhow. > There are might be race conditions. Can you name them > +enum imx_i2c_slave_state { > + I2C_IMX_SLAVE_IDLE, > + I2C_IMX_SLAVE_IRQ, > + I2C_IMX_SLAVE_POLLING Highlevel question first: Why do you have polling? Why would anyone not want to use interrupts here? signature.asc Description: PGP signature
Re: [PATCH] i2c: imx: add slave support. v2
On Tue, Jan 26, 2016 at 07:14:40PM +0300, Dmitriy Baranov wrote: > Add I2C slave provider using the generic slave interface. > It also supports master transactions when the slave in the idle mode. > > Issues: > Changes work only in PIO mode (when driver doesn`t use DMA) This is fine with me. We don't support block transfers currently in the slave core anyhow. > There are might be race conditions. Can you name them > +enum imx_i2c_slave_state { > + I2C_IMX_SLAVE_IDLE, > + I2C_IMX_SLAVE_IRQ, > + I2C_IMX_SLAVE_POLLING Highlevel question first: Why do you have polling? Why would anyone not want to use interrupts here? signature.asc Description: PGP signature
[PATCH] i2c: imx: add slave support. v2
Add I2C slave provider using the generic slave interface. It also supports master transactions when the slave in the idle mode. Issues: Changes work only in PIO mode (when driver doesn`t use DMA) It weren`t tested with DMA is enabled (in PIO mode it works well) There are might be race conditions. We hope that these changes will be helpfull. Thank you. Signed-off-by: Maxim Syrchin Signed-off-by: Dmitriy Baranov --- drivers/i2c/busses/Kconfig | 1 + drivers/i2c/busses/i2c-imx.c | 311 --- 2 files changed, 291 insertions(+), 21 deletions(-) diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 0299dfa..bc7cbfd 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -587,6 +587,7 @@ config I2C_IMG config I2C_IMX tristate "IMX I2C interface" depends on ARCH_MXC || ARCH_LAYERSCAPE + select I2C_SLAVE help Say Y here if you want to use the IIC bus controller on the Freescale i.MX/MXC or Layerscape processors. diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index a2b132c..3c286f1 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -53,6 +53,7 @@ #include #include #include +#include /* This will be the driver name the kernel reports */ #define DRIVER_NAME "imx-i2c" @@ -171,6 +172,18 @@ enum imx_i2c_type { VF610_I2C, }; +enum imx_i2c_mode { + I2C_IMX_SLAVE, + I2C_IMX_MASTER, + I2C_IMX_UNDEFINED +}; + +enum imx_i2c_slave_state { + I2C_IMX_SLAVE_IDLE, + I2C_IMX_SLAVE_IRQ, + I2C_IMX_SLAVE_POLLING +}; + struct imx_i2c_hwdata { enum imx_i2c_type devtype; unsignedregshift; @@ -193,10 +206,12 @@ struct imx_i2c_dma { struct imx_i2c_struct { struct i2c_adapter adapter; + struct i2c_client *slave; struct clk *clk; void __iomem*base; wait_queue_head_t queue; unsigned long i2csr; + unsigned long i2csr_slave; unsigned intdisable_delay; int stopped; unsigned intifdr; /* IMX_I2C_IFDR */ @@ -210,6 +225,11 @@ struct imx_i2c_struct { struct pinctrl_state *pinctrl_pins_gpio; struct imx_i2c_dma *dma; + + enum imx_i2c_mode dev_mode; + atomic_tslave_state; + struct task_struct *slave_task; + wait_queue_head_t slave_queue; }; static const struct imx_i2c_hwdata imx1_i2c_hwdata = { @@ -510,39 +530,97 @@ static void i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx) #endif } -static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) +static int i2c_imx_configure_clock(struct imx_i2c_struct *i2c_imx) { - unsigned int temp = 0; int result; - dev_dbg(_imx->adapter.dev, "<%s>\n", __func__); - i2c_imx_set_clk(i2c_imx); - imx_i2c_write_reg(i2c_imx->ifdr, i2c_imx, IMX_I2C_IFDR); - /* Enable I2C controller */ - imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, IMX_I2C_I2SR); - imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode, i2c_imx, IMX_I2C_I2CR); + result = clk_prepare_enable(i2c_imx->clk); + if (result == 0) + imx_i2c_write_reg(i2c_imx->ifdr, i2c_imx, IMX_I2C_IFDR); + + return result; +} + +static void i2c_imx_enable_i2c_controller(struct imx_i2c_struct *i2c_imx) +{ + imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, + IMX_I2C_I2SR); + imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode, i2c_imx, + IMX_I2C_I2CR); /* Wait controller to be stable */ udelay(50); +} + +static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) +{ + unsigned int temp = 0; + int result; + + i2c_imx->dev_mode = I2C_IMX_UNDEFINED; + + dev_dbg(_imx->adapter.dev, "<%s>\n", __func__); + + result = i2c_imx_configure_clock(i2c_imx); + if (result != 0) + return result; + + i2c_imx_enable_i2c_controller(i2c_imx); /* Start I2C transaction */ temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); temp |= I2CR_MSTA; imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); + result = i2c_imx_bus_busy(i2c_imx, 1); if (result) return result; i2c_imx->stopped = 0; + i2c_imx->dev_mode = I2C_IMX_MASTER; + temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK; temp &= ~I2CR_DMAEN; imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); return result; } -static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) +static int i2c_imx_start_slave_mode(struct imx_i2c_struct *i2c_imx, bool enable) +{ + unsigned int temp; + int result; + + dev_dbg(_imx->adapter.dev, "<%s>\n", __func__); + + i2c_imx->dev_mode =
[PATCH] i2c: imx: add slave support. v2
Add I2C slave provider using the generic slave interface. It also supports master transactions when the slave in the idle mode. Issues: Changes work only in PIO mode (when driver doesn`t use DMA) It weren`t tested with DMA is enabled (in PIO mode it works well) There are might be race conditions. We hope that these changes will be helpfull. Thank you. Signed-off-by: Maxim SyrchinSigned-off-by: Dmitriy Baranov --- drivers/i2c/busses/Kconfig | 1 + drivers/i2c/busses/i2c-imx.c | 311 --- 2 files changed, 291 insertions(+), 21 deletions(-) diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 0299dfa..bc7cbfd 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -587,6 +587,7 @@ config I2C_IMG config I2C_IMX tristate "IMX I2C interface" depends on ARCH_MXC || ARCH_LAYERSCAPE + select I2C_SLAVE help Say Y here if you want to use the IIC bus controller on the Freescale i.MX/MXC or Layerscape processors. diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index a2b132c..3c286f1 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -53,6 +53,7 @@ #include #include #include +#include /* This will be the driver name the kernel reports */ #define DRIVER_NAME "imx-i2c" @@ -171,6 +172,18 @@ enum imx_i2c_type { VF610_I2C, }; +enum imx_i2c_mode { + I2C_IMX_SLAVE, + I2C_IMX_MASTER, + I2C_IMX_UNDEFINED +}; + +enum imx_i2c_slave_state { + I2C_IMX_SLAVE_IDLE, + I2C_IMX_SLAVE_IRQ, + I2C_IMX_SLAVE_POLLING +}; + struct imx_i2c_hwdata { enum imx_i2c_type devtype; unsignedregshift; @@ -193,10 +206,12 @@ struct imx_i2c_dma { struct imx_i2c_struct { struct i2c_adapter adapter; + struct i2c_client *slave; struct clk *clk; void __iomem*base; wait_queue_head_t queue; unsigned long i2csr; + unsigned long i2csr_slave; unsigned intdisable_delay; int stopped; unsigned intifdr; /* IMX_I2C_IFDR */ @@ -210,6 +225,11 @@ struct imx_i2c_struct { struct pinctrl_state *pinctrl_pins_gpio; struct imx_i2c_dma *dma; + + enum imx_i2c_mode dev_mode; + atomic_tslave_state; + struct task_struct *slave_task; + wait_queue_head_t slave_queue; }; static const struct imx_i2c_hwdata imx1_i2c_hwdata = { @@ -510,39 +530,97 @@ static void i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx) #endif } -static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) +static int i2c_imx_configure_clock(struct imx_i2c_struct *i2c_imx) { - unsigned int temp = 0; int result; - dev_dbg(_imx->adapter.dev, "<%s>\n", __func__); - i2c_imx_set_clk(i2c_imx); - imx_i2c_write_reg(i2c_imx->ifdr, i2c_imx, IMX_I2C_IFDR); - /* Enable I2C controller */ - imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, IMX_I2C_I2SR); - imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode, i2c_imx, IMX_I2C_I2CR); + result = clk_prepare_enable(i2c_imx->clk); + if (result == 0) + imx_i2c_write_reg(i2c_imx->ifdr, i2c_imx, IMX_I2C_IFDR); + + return result; +} + +static void i2c_imx_enable_i2c_controller(struct imx_i2c_struct *i2c_imx) +{ + imx_i2c_write_reg(i2c_imx->hwdata->i2sr_clr_opcode, i2c_imx, + IMX_I2C_I2SR); + imx_i2c_write_reg(i2c_imx->hwdata->i2cr_ien_opcode, i2c_imx, + IMX_I2C_I2CR); /* Wait controller to be stable */ udelay(50); +} + +static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) +{ + unsigned int temp = 0; + int result; + + i2c_imx->dev_mode = I2C_IMX_UNDEFINED; + + dev_dbg(_imx->adapter.dev, "<%s>\n", __func__); + + result = i2c_imx_configure_clock(i2c_imx); + if (result != 0) + return result; + + i2c_imx_enable_i2c_controller(i2c_imx); /* Start I2C transaction */ temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); temp |= I2CR_MSTA; imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); + result = i2c_imx_bus_busy(i2c_imx, 1); if (result) return result; i2c_imx->stopped = 0; + i2c_imx->dev_mode = I2C_IMX_MASTER; + temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK; temp &= ~I2CR_DMAEN; imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); return result; } -static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) +static int i2c_imx_start_slave_mode(struct imx_i2c_struct *i2c_imx, bool enable) +{ + unsigned int temp; + int result; + + dev_dbg(_imx->adapter.dev, "<%s>\n",