RE: [PATCH] i2c: imx: simplify i2c_imx_dma_write() a little
Great, It's quite simpler than before. I should take more attention on make the code cleanly and efficiently. Best Regards, Yuan Yao > -Original Message- > From: Wolfram Sang [mailto:w...@the-dreams.de] > Sent: Wednesday, November 19, 2014 5:12 PM > To: linux-i2c@vger.kernel.org > Cc: Yuan Yao-B46683; Wolfram Sang > Subject: [PATCH] i2c: imx: simplify i2c_imx_dma_write() a little > > Signed-off-by: Wolfram Sang > --- > drivers/i2c/busses/i2c-imx.c | 6 +- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index d0668d0d626d..aab1f4bb9e30 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -657,11 +657,7 @@ static int i2c_imx_dma_write(struct imx_i2c_struct > *i2c_imx, > if (result) > return result; > > - result = i2c_imx_acked(i2c_imx); > - if (result) > - return result; > - > - return 0; > + return i2c_imx_acked(i2c_imx); > } > > static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx, > -- > 2.1.1 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v10 3/3] i2c: imx: add DMA support for freescale i2c driver
Hi Wolfram,
Thank you for having found the bug, I indeed missed initializing
it('orig_jiffies').
I will do more detailed review and send the v11 soon.
Best Regards,
Yuan Yao
> -Original Message-
> From: Wolfram Sang [mailto:w...@the-dreams.de]
> Sent: Tuesday, November 18, 2014 2:24 AM
> To: Yuan Yao-B46683
> Cc: ma...@denx.de; l...@karo-electronics.de; mark.rutl...@arm.com; Duan
> Fugang-B38611; shawn@linaro.org; linux-ker...@vger.kernel.org; linux-
> arm-ker...@lists.infradead.org; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH v10 3/3] i2c: imx: add DMA support for freescale i2c
> driver
>
> On Mon, Nov 17, 2014 at 06:40:36PM +0800, Yuan Yao wrote:
> > Add dma support for i2c. This function depend on DMA driver.
> > You can turn on it by write both the dmas and dma-name properties in
> dts node.
> > DMA is optional, even DMA request unsuccessfully, i2c can also work
> well.
> >
> > Signed-off-by: Yuan Yao
>
> gcc found a bug:
>
> drivers/i2c/busses/i2c-imx.c:703:7: warning: 'orig_jiffies' may be used
> uninitialized in this function [-Wuninitialized]
> drivers/i2c/busses/i2c-imx.c:672:16: note: 'orig_jiffies' was declared
> here
>
> > + result = wait_for_completion_interruptible_timeout(
> > + &i2c_imx->dma->cmd_complete,
> > + msecs_to_jiffies(DMA_TIMEOUT));
>
> And are you sure you want to use
> wait_for_completion_interruptible_timeout() instead of
> wait_for_completion_timeout() here? Unless you confirm you tested it a
> lot with signals, I really cannot recommend the *_interruptible_* version.
>
> Other than that, good. I liked the logfile of what you tested very much!
>
> Thanks,
>
>Wolfram
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v9 3/3] i2c: imx: add DMA support for freescale i2c driver
Thanks for your review.
Your suggestions is really helpful.
I will correct them in v10.
Best Regards,
Yuan Yao
> -Original Message-
> From: Wolfram Sang [mailto:w...@the-dreams.de]
> Sent: Sunday, November 09, 2014 1:35 AM
> To: Yuan Yao-B46683
> Cc: ma...@denx.de; l...@karo-electronics.de; mark.rutl...@arm.com; Duan
> Fugang-B38611; shawn@linaro.org; linux-ker...@vger.kernel.org; linux-
> arm-ker...@lists.infradead.org; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH v9 3/3] i2c: imx: add DMA support for freescale i2c
> driver
>
> Hi,
>
> mostly looking good...
>
> > +#define IMX_I2C_DMA_THRESHOLD 16
>
> Have you guessed or measured this value? A comment about this value would
> be nice.
>
> >
> > +struct imx_i2c_dma {
> > + struct dma_chan *chan_tx;
> > + struct dma_chan *chan_rx;
> > + struct dma_chan *chan_using;
> > + struct completion cmd_complete;
> > + dma_addr_t dma_buf;
> > + unsigned intdma_len;
> > + unsigned intdma_transfer_dir;
> > + unsigned intdma_data_dir;
>
> Please use proper types as there are things like 'enum
> dma_data_direction' and the likes...
>
> > + dmaengine_submit(txdesc);
>
> This can fail, too. Use dma_submit_error() to check.
>
> > + result = wait_for_completion_interruptible_timeout(
> > + &i2c_imx->dma->cmd_complete,
> > + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
>
> Have you tested signals extensively? I can't really recommend the
> _intrruptible_ here. I don't see any cleaning up to get the bus to a
> consistent state again. Most people drop the interruptible sooner or
> later.
>
> > + /* Init DMA config if support*/
> > + i2c_imx_dma_request(i2c_imx, phy_addr);
>
> Make this function void? DMA is optional anyhow.
>
> Thanks,
>
>Wolfram
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v9 3/3] i2c: imx: add DMA support for freescale i2c driver
Hi Wolfram,
I have fixed the issues in v9. Could you please help to review this patch?
Thanks & Regards
Yuan Yao
> -Original Message-
> From: linux-arm-kernel [mailto:linux-arm-kernel-
> boun...@lists.infradead.org] On Behalf Of Yuan Yao
> Sent: Saturday, October 11, 2014 6:57 PM
> To: w...@the-dreams.de; ma...@denx.de
> Cc: mark.rutl...@arm.com; linux-ker...@vger.kernel.org; linux-
> i...@vger.kernel.org; Duan Fugang-B38611; shawn@linaro.org; linux-arm-
> ker...@lists.infradead.org; l...@karo-electronics.de
> Subject: [PATCH v9 3/3] i2c: imx: add DMA support for freescale i2c
> driver
>
> Add dma support for i2c. This function depend on DMA driver.
> You can turn on it by write both the dmas and dma-name properties in dts
> node.
> DMA is optional, even DMA request unsuccessfully, i2c can also work well.
>
> Signed-off-by: Yuan Yao
> ---
> drivers/i2c/busses/i2c-imx.c | 327
> ++-
> 1 file changed, 325 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 2452b57..e58c771 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -38,7 +38,11 @@
>
> *
> **/
>
> #include
> +#include
> #include
> +#include
> +#include
> +#include
> #include
> #include
> #include
> @@ -49,6 +53,7 @@
> #include
> #include
> #include
> +#include
> #include #include
> #include @@ -63,6 +68,13 @@
> /* Default value */
> #define IMX_I2C_BIT_RATE 10 /* 100kHz */
>
> +/*
> + * Enable DMA if transfer byte size is bigger than this threshold.
> + * As the hardware request, it must bigger than 4 bytes.
> + */
> +#define IMX_I2C_DMA_THRESHOLD16
> +#define IMX_I2C_DMA_TIMEOUT 1000
> +
> /* IMX I2C registers:
> * the I2C register offset is different between SoCs,
> * to provid support for all these chips, split the @@ -88,6 +100,7 @@
> #define I2SR_IBB 0x20
> #define I2SR_IAAS0x40
> #define I2SR_ICF 0x80
> +#define I2CR_DMAEN 0x02
> #define I2CR_RSTA0x04
> #define I2CR_TXAK0x08
> #define I2CR_MTX 0x10
> @@ -174,6 +187,17 @@ struct imx_i2c_hwdata {
> unsignedi2cr_ien_opcode;
> };
>
> +struct imx_i2c_dma {
> + struct dma_chan *chan_tx;
> + struct dma_chan *chan_rx;
> + struct dma_chan *chan_using;
> + struct completion cmd_complete;
> + dma_addr_t dma_buf;
> + unsigned intdma_len;
> + unsigned intdma_transfer_dir;
> + unsigned intdma_data_dir;
> +};
> +
> struct imx_i2c_struct {
> struct i2c_adapter adapter;
> struct clk *clk;
> @@ -186,6 +210,8 @@ struct imx_i2c_struct {
> unsigned intcur_clk;
> unsigned intbitrate;
> const struct imx_i2c_hwdata *hwdata;
> +
> + struct imx_i2c_dma *dma;
> };
>
> static const struct imx_i2c_hwdata imx1_i2c_hwdata = { @@ -256,6
> +282,132 @@ static inline unsigned char imx_i2c_read_reg(struct
> imx_i2c_struct *i2c_imx,
> return readb(i2c_imx->base + (reg << i2c_imx->hwdata->regshift)); }
>
> +/* Functions for DMA support */
> +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
> + dma_addr_t phy_addr)
> +{
> + struct imx_i2c_dma *dma;
> + struct dma_slave_config dma_sconfig;
> + struct device *dev = &i2c_imx->adapter.dev;
> + int ret;
> +
> + dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
> + if (!dma)
> + return -ENOMEM;
> +
> + dma->chan_tx = dma_request_slave_channel(dev, "tx");
> + if (!dma->chan_tx) {
> + dev_dbg(dev, "can't request DMA tx channel\n");
> + ret = -ENODEV;
> + goto fail_al;
> + }
> +
> + dma_sconfig.dst_addr = phy_addr +
> + (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
> + dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> + dma_sconfig.dst_maxburst = 1;
> + dma_sconfig.direction = DMA_MEM_TO_DEV;
> + ret = dmaengine_slave_config(dma->chan_tx, &dma_sconfig);
> + if (ret < 0) {
> + dev_dbg(dev, "can't configure tx channel\n");
> + goto fail_tx;
> + }
> +
> + dma->chan_rx = dma_request_slave_channel(dev, "rx");
> + if (!dma->chan_rx) {
> + dev_dbg(dev, "can't request DMA rx channel\n");
> + ret = -ENODEV;
> + goto fail_tx;
> + }
> +
> + dma_sconfig.src_addr = phy_addr +
> + (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
> + dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> + dma_sconfig.src_maxburst = 1;
> + dma_sconfig.direction = DMA_DEV_TO_MEM;
> + ret = dmaengine_slave_config(dma->chan_rx, &dma_sconfig);
> + if (ret < 0) {
RE: [PATCH v8 1/2] i2c: imx: add DMA support for freescale i2c driver
> -Original Message-
> From: Wolfram Sang [mailto:w...@the-dreams.de]
> Sent: Friday, October 03, 2014 3:55 PM
> To: Yuan Yao-B46683
> Cc: ma...@denx.de; l...@karo-electronics.de; mark.rutl...@arm.com; Duan
> Fugang-B38611; shawn@linaro.org; linux-ker...@vger.kernel.org; linux-
> arm-ker...@lists.infradead.org; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH v8 1/2] i2c: imx: add DMA support for freescale i2c
> driver
> > -#include
> > -#include
> > -#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > #include
> > #include
> > #include
> > -#include
> > #include
> > +#include
> > #include
> > -#include
> > -#include
> > -#include
> > -#include
> > +#include
> > +#include
> > #include
> > #include
> > +#include
> > #include
> > +#include
> > +#include
> > +#include
>
> This is a seperate patch.
[Yuan Yao]
Here I just adjust the order of the include file as alphabetical order.
If it looks strange I can only add the include files about DMA.
>
>
> > + while (1) {
> > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> > + if (temp & I2SR_ICF)
> > + break;
> > + if (time_after(jiffies, orig_jiffies +
> > + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT))) {
> > + dev_dbg(dev, "<%s> Timeout\n", __func__);
> > + return -ETIMEDOUT;
> > + }
> > + schedule();
>
> That might have been asked before. Is there no interrupt for this?
>
[Yuan Yao] No, there is no interrupt.
After DMA callback, I must wait until the last byte transfer completely.
It's a very short time which less than 10us.
By the way, how about use udelay(10) instead of schedule()?
udelay(10) is waiting a appropriate time.
schedule() is waiting too long for i2c but may be good for whole system.
Can you give me some suggestion?
Thanks for your review.
Best Regards,
Yuan Yao
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v8 1/2] i2c: imx: add DMA support for freescale i2c driver
> -Original Message-
> From: Duan Fugang-B38611
> Sent: Thursday, September 25, 2014 5:25 PM
> To: Yuan Yao-B46683; w...@the-dreams.de; ma...@denx.de
> Cc: l...@karo-electronics.de; mark.rutl...@arm.com; shawn@linaro.org;
> linux-ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> linux-i2c@vger.kernel.org
> Subject: RE: [PATCH v8 1/2] i2c: imx: add DMA support for freescale i2c
> driver
>
> From: Yuan Yao Sent: Thursday, September 25,
> 2014 4:11 PM
> >To: w...@the-dreams.de; ma...@denx.de
> >Cc: l...@karo-electronics.de; mark.rutl...@arm.com; Duan Fugang-B38611;
> >shawn@linaro.org; linux-ker...@vger.kernel.org; linux-arm-
> >ker...@lists.infradead.org; linux-i2c@vger.kernel.org
> >Subject: [PATCH v8 1/2] i2c: imx: add DMA support for freescale i2c
> >driver
> >
> >Add dma support for i2c. This function depend on DMA driver.
> >You can turn on it by write both the dmas and dma-name properties in
> >dts node.
> >DMA is optional, even DMA request unsuccessfully, i2c can also work well.
> >
> >Signed-off-by: Yuan Yao
> >---
> > drivers/i2c/busses/i2c-imx.c | 352
> >+--
> > 1 file changed, 342 insertions(+), 10 deletions(-)
> >
> >diff --git a/drivers/i2c/busses/i2c-imx.c
> >b/drivers/i2c/busses/i2c-imx.c index 613069b..c643756 100644
> >--- a/drivers/i2c/busses/i2c-imx.c
> >+++ b/drivers/i2c/busses/i2c-imx.c
> >@@ -37,22 +37,27 @@
> >+ */
> >+imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> >+reinit_completion(&i2c_imx->dma->cmd_complete);
> >+result = wait_for_completion_interruptible_timeout(
> >+&i2c_imx->dma->cmd_complete,
> >+msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
> >+if (result <= 0) {
> >+dmaengine_terminate_all(dma->chan_using);
> >+if (result)
> >+return result;
> >+else
> >+return -ETIMEDOUT;
> >+}
> >+
> >+/* Waiting for Transfer complete. */
> >+while (1) {
> >+temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> >+if (temp & I2SR_ICF)
> Why not wait for IIF ?
[Yuan Yao]
IIF is often used. But IIF cannot indicate transfer completely in DMA mode.
So TCF(I2SR_ICF) is the only choice.
> >+break;
> >+if (time_after(jiffies, orig_jiffies +
> >+msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT))) {
> >+dev_dbg(dev, "<%s> Timeout\n", __func__);
> >+return -ETIMEDOUT;
> >+}
> >+schedule();
> >+}
Thanks & Best regards,
Yuan Yao
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 1/2] i2c: imx: add DMA support for freescale i2c driver
Marek Vasut wrote: > On Wednesday, September 17, 2014 at 04:50:34 PM, Yao Yuan wrote: > [...] > > > > > Would that mean that the "crashed" DMA would be running until the > > > > > next transmission is scheduled ? > > > > > > > > [Yuan Yao] No, In fact any DMA timeout will result the failure of I2C > > > > transmission and then it will turn to report the exception and wait > > > > for next transmission. > > > > > > Can you tell when the next transmission will happen? What if I issue a > > > single transmission and that one fails ? Will the DMA run until who knows > > > when ? > > > > [Yuan Yao] > > Sorry for my unclear description. In fact, During the DMA transmission if > > an error happened or time out, DMA will stop at once and be disabled. > > I just continue to route the TX and RX request to signal the DMA > > controller. Because the DMA is disabled, it will ignore those signals. > > > > In a word, I just want to block the I2C TX, RX and interrupt signal when > > DMA mode failed until the next I2C transmission start. > So the I2C block is in error state until you clean it up upon next > transmission? [Yuan Yao] No, just DMAEN bit. Other I2C error state will clean soon. > > In fact, the bit "I2CR_DMAEN" is a switch which decide whether I2C route > > the TX, RX and interrupt signal to DMA controller. > > > > > The only thing I worried about is I2C may still receive some feedbacks > > > after DMA timeout. In this case the feedbacks may lead to abnormal > > > state in PIO mode.But it will be ignored in DMA model. > > > That's why I tend to delay force-disable DMA until the next > > > transmission begin. Could you please give me some suggestion? > > > > > No, this design just seems flawed to me. You should stop the DMA > > > immediatelly if there is an error to avoid wasting resources and prevent > > > possible other adverse effects. > > > > [Yuan Yao] > > Yes, I have stopped the DMA immediately. However I keep the I2C DMA > > single route. > > > > I don't have the exact evidence to prove that my design is acceptable. > > So if you are sure it's flawed, I will change it in the next version(V8). > I'm just trying to understand it. [Yuan Yao] Both of us know that we should stop DMA immediately when issue happened. The only argument is that I want to set the DMAEN bit just before the next transmission starts. But you think when I stop the DMA I should set it at the same time. The bit is the switch which is used to decide whether Rx and Tx signal can be routed to DMA. In fact, I deeply think about what is the difference between our arguments for those days. I think the two ways are almost the same. Your way is more acceptable because people tend to clear the DMA status just after stopping it. So I think your way is better. Best regards, Yuan Yao -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 1/2] i2c: imx: add DMA support for freescale i2c driver
On Wednesday, September 17, 2014 2:17 AM, Marek Vasut wrote:
> On Wednesday, September 10, 2014 at 04:48:01 PM, Yao Yuan wrote:
> > On Friday, September 05, 2014 6:41 PM, Marek Vasut wrote:
> > > On Friday, September 05, 2014 at 12:32:40 PM, Yao Yuan wrote:
> > > [...]
> > >
> > > > > > +static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
> > > > > > + struct i2c_msg *msgs)
> > > > > > +{
> > > > > > + int result;
> > > > > > + unsigned int temp = 0;
> > > > > > + unsigned long orig_jiffies = jiffies;
> > > > > > + struct imx_i2c_dma *dma = i2c_imx->dma;
> > > > > > + struct device *dev = &i2c_imx->adapter.dev;
> > > > > > +
> > > > > > + dev_dbg(dev, "<%s> write slave address: addr=0x%x\n",
> > > > > > + __func__, msgs->addr << 1);
> > > > > > +
> > > > > > + reinit_completion(&i2c_imx->dma->cmd_complete);
> > > > > > + dma->chan_using = dma->chan_tx;
> > > > > > + dma->dma_transfer_dir = DMA_MEM_TO_DEV;
> > > > > > + dma->dma_data_dir = DMA_TO_DEVICE;
> > > > > > + dma->dma_len = msgs->len - 1;
> > > > > > + result = i2c_imx_dma_xfer(i2c_imx, msgs);
> > > > > > + if (result)
> > > > > > + return result;
> > > > > > +
> > > > > > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > > > > > + temp |= I2CR_DMAEN;
> > > > > > + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > > > > > +
> > > > > > + /*
> > > > > > +* Write slave address.
> > > > > > +* The first byte muse be transmitted by the CPU.
> > > > > > +*/
> > > > > > + imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> > > > > > + result = wait_for_completion_interruptible_timeout(
> > > > > > + &i2c_imx->dma->cmd_complete,
> > > > > > + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
> > > > > > + if (result <= 0) {
> > > > > > + dmaengine_terminate_all(dma->chan_using);
> > > > > > + if (result)
> > > > > > + return result;
> > > > > > + else
> > > > > > + return -ETIMEDOUT;
> > > > >
> > > > > Shouldn't you force-disable the DMA here somehow (like unsetting
> > > > > I2CR_DMAEN bit), if it failed or timed out?
> > > >
> > > > [Yuan Yao] Yes, I put the code for force-disable DMA in
> > > > i2c_imx_start(). In order to make sure any DMA error will not
> > > > effect the I2C.
> > > > It seems almost the same as put the code here, how about your think?
> > >
> > > Would that mean that the "crashed" DMA would be running until the
> > > next transmission is scheduled ?
> >
> > [Yuan Yao] No, In fact any DMA timeout will result the failure of I2C
> > transmission and then it will turn to report the exception and wait
> > for next transmission.
>
> Can you tell when the next transmission will happen? What if I issue a
> single transmission and that one fails ? Will the DMA run until who knows
> when ?
[Yuan Yao]
Sorry for my unclear description. In fact, During the DMA transmission if
an error happened or time out, DMA will stop at once and be disabled.
I just continue to route the TX and RX request to signal the DMA controller.
Because the DMA is disabled, it will ignore those signals.
In a word, I just want to block the I2C TX, RX and interrupt signal when
DMA mode failed until the next I2C transmission start.
In fact, the bit "I2CR_DMAEN" is a switch which decide whether I2C route
the TX, RX and interrupt signal to DMA controller.
> > The only thing I worried about is I2C may still receive some feedbacks
> > after DMA timeout. In this case the feedbacks may lead to abnormal
> > state in PIO mode.But it will be ignored in DMA model.
> > That's why I tend to delay force-disable DMA until the next
> > transmission begin. Could you please give me some suggestion?
>
> No, this design just seems flawed to me. You should stop the DMA
> immediatelly if there is an error to avoid wasting resources and prevent
> possible other adverse effects.
>
[Yuan Yao]
Yes, I have stopped the DMA immediately. However I keep the I2C DMA
single route.
I don't have the exact evidence to prove that my design is acceptable.
So if you are sure it's flawed, I will change it in the next version(V8).
Best regards,
Yuan Yao
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 1/2] i2c: imx: add DMA support for freescale i2c driver
On Friday, September 05, 2014 6:41 PM, Marek Vasut wrote:
> On Friday, September 05, 2014 at 12:32:40 PM, Yao Yuan wrote:
> [...]
> > > > +static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
> > > > + struct i2c_msg *msgs)
> > > > +{
> > > > + int result;
> > > > + unsigned int temp = 0;
> > > > + unsigned long orig_jiffies = jiffies;
> > > > + struct imx_i2c_dma *dma = i2c_imx->dma;
> > > > + struct device *dev = &i2c_imx->adapter.dev;
> > > > +
> > > > + dev_dbg(dev, "<%s> write slave address: addr=0x%x\n",
> > > > + __func__, msgs->addr << 1);
> > > > +
> > > > + reinit_completion(&i2c_imx->dma->cmd_complete);
> > > > + dma->chan_using = dma->chan_tx;
> > > > + dma->dma_transfer_dir = DMA_MEM_TO_DEV;
> > > > + dma->dma_data_dir = DMA_TO_DEVICE;
> > > > + dma->dma_len = msgs->len - 1;
> > > > + result = i2c_imx_dma_xfer(i2c_imx, msgs);
> > > > + if (result)
> > > > + return result;
> > > > +
> > > > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > > > + temp |= I2CR_DMAEN;
> > > > + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > > > +
> > > > + /*
> > > > +* Write slave address.
> > > > +* The first byte muse be transmitted by the CPU.
> > > > +*/
> > > > + imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> > > > + result = wait_for_completion_interruptible_timeout(
> > > > + &i2c_imx->dma->cmd_complete,
> > > > + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
> > > > + if (result <= 0) {
> > > > + dmaengine_terminate_all(dma->chan_using);
> > > > + if (result)
> > > > + return result;
> > > > + else
> > > > + return -ETIMEDOUT;
> > >
> > > Shouldn't you force-disable the DMA here somehow (like unsetting
> > > I2CR_DMAEN bit), if it failed or timed out?
> >
> > [Yuan Yao] Yes, I put the code for force-disable DMA in i2c_imx_start().
> > In order to make sure any DMA error will not effect the I2C.
> > It seems almost the same as put the code here, how about your think?
>
> Would that mean that the "crashed" DMA would be running until the next
> transmission is scheduled ?
>
[Yuan Yao] No, In fact any DMA timeout will result the failure of I2C
transmission
and then it will turn to report the exception and wait for next transmission.
The only thing I worried about is I2C may still receive some feedbacks after
DMA timeout.
In this case the feedbacks may lead to abnormal state in PIO mode.But it will
be ignored
in DMA model.
That's why I tend to delay force-disable DMA until the next transmission begin.
Could you please give me some suggestion?
Thanks & Best regards,
Yuan Yao--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 1/2] i2c: imx: add DMA support for freescale i2c driver
On Thursday, September 04, 2014 10:39 PM, Marek Vasut wrote:
> On Wednesday, August 13, 2014 at 11:46:54 AM, Yuan Yao wrote:
> > Add dma support for i2c. This function depend on DMA driver.
> > You can turn on it by write both the dmas and dma-name properties in
> > dts node. DMA is optional, even DMA request unsuccessfully, i2c can
> > also work well.
> >
> > Signed-off-by: Yuan Yao
>
> [...]
>
> > +/* Enable DMA if transfer byte size is bigger than this threshold.
> > + * As the hardware request, it must bigger than 4.
>
> The comment is unclear, just by reading it, I have no clue what are the
> units for this value. I can guess those would be bytes, but it would be a
> good idea to be explicit.
>
> Also, wasn't kernel comment style starting with leading /* , with the
> text starting only on the next line ?
>
[Yuan Yao] Thanks for your review. I will correct it in the next version.
> > + */
> > +#define IMX_I2C_DMA_THRESHOLD 16
> > +#define IMX_I2C_DMA_TIMEOUT1000
> > +
>
> [...]
>
> > +static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
> > + struct i2c_msg *msgs)
> > +{
> > + int result;
> > + unsigned int temp = 0;
> > + unsigned long orig_jiffies = jiffies;
> > + struct imx_i2c_dma *dma = i2c_imx->dma;
> > + struct device *dev = &i2c_imx->adapter.dev;
> > +
> > + dev_dbg(dev, "<%s> write slave address: addr=0x%x\n",
> > + __func__, msgs->addr << 1);
> > +
> > + reinit_completion(&i2c_imx->dma->cmd_complete);
> > + dma->chan_using = dma->chan_tx;
> > + dma->dma_transfer_dir = DMA_MEM_TO_DEV;
> > + dma->dma_data_dir = DMA_TO_DEVICE;
> > + dma->dma_len = msgs->len - 1;
> > + result = i2c_imx_dma_xfer(i2c_imx, msgs);
> > + if (result)
> > + return result;
> > +
> > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > + temp |= I2CR_DMAEN;
> > + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +
> > + /*
> > +* Write slave address.
> > +* The first byte muse be transmitted by the CPU.
> > +*/
> > + imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> > + result = wait_for_completion_interruptible_timeout(
> > + &i2c_imx->dma->cmd_complete,
> > + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
> > + if (result <= 0) {
> > + dmaengine_terminate_all(dma->chan_using);
> > + if (result)
> > + return result;
> > + else
> > + return -ETIMEDOUT;
>
> Shouldn't you force-disable the DMA here somehow (like unsetting
> I2CR_DMAEN bit), if it failed or timed out?
[Yuan Yao] Yes, I put the code for force-disable DMA in i2c_imx_start().
In order to make sure any DMA error will not effect the I2C.
It seems almost the same as put the code here, how about your think?
Thanks!
> > + }
> > +
> > + /* Waiting for Transfer complete. */
> > + while (1) {
> > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> > + if (temp & I2SR_ICF)
> > + break;
> > + if (time_after(jiffies, orig_jiffies +
> > + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT))) {
> > + dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n",
> > + __func__);
> > + return -ETIMEDOUT;
> > + }
> > + schedule();
> > + }
> > +
> > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > + temp &= ~I2CR_DMAEN;
> > + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +
> > + /* The last data byte must be transferred by the CPU. */
> > + imx_i2c_write_reg(msgs->buf[msgs->len-1],
> > + i2c_imx, IMX_I2C_I2DR);
> > + result = i2c_imx_trx_complete(i2c_imx);
> > + if (result)
> > + return result;
> > +
> > + result = i2c_imx_acked(i2c_imx);
> > + if (result)
> > + return result;
> > +
> > + return 0;
> > +}
> [...]
>
> Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v7 1/2] i2c: imx: add DMA support for freescale i2c driver
Hi Wolfram,
I fixed the mentioned issues before in v7. Could you please help to review this
patch?
Thanks & Regards
Yuan Yao
-Original Message-
From: linux-arm-kernel [mailto:linux-arm-kernel-boun...@lists.infradead.org] On
Behalf Of Yuan Yao
Sent: Wednesday, August 13, 2014 5:47 PM
To: w...@the-dreams.de; ma...@denx.de
Cc: mark.rutl...@arm.com; linux-ker...@vger.kernel.org;
linux-i2c@vger.kernel.org; Duan Fugang-B38611; shawn@linaro.org;
linux-arm-ker...@lists.infradead.org; l...@karo-electronics.de
Subject: [PATCH v7 1/2] i2c: imx: add DMA support for freescale i2c driver
Add dma support for i2c. This function depend on DMA driver.
You can turn on it by write both the dmas and dma-name properties in dts node.
DMA is optional, even DMA request unsuccessfully, i2c can also work well.
Signed-off-by: Yuan Yao
---
drivers/i2c/busses/i2c-imx.c | 351 +--
1 file changed, 341 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index
aa8bc14..a09bf8c 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -37,22 +37,27 @@
/** Includes
***
***/
-#include
-#include
-#include
+#include
+#include
+#include
+#include
+#include
+#include
#include
#include
#include
-#include
#include
+#include
#include
-#include
-#include
-#include
-#include
+#include
+#include
#include
#include
+#include
#include
+#include
+#include
+#include
/** Defines
***/
@@ -63,6 +68,12 @@
/* Default value */
#define IMX_I2C_BIT_RATE 10 /* 100kHz */
+/* Enable DMA if transfer byte size is bigger than this threshold.
+ * As the hardware request, it must bigger than 4.
+ */
+#define IMX_I2C_DMA_THRESHOLD 16
+#define IMX_I2C_DMA_TIMEOUT1000
+
/* IMX I2C registers:
* the I2C register offset is different between SoCs,
* to provid support for all these chips, split the @@ -88,6 +99,7 @@
#define I2SR_IBB 0x20
#define I2SR_IAAS 0x40
#define I2SR_ICF 0x80
+#define I2CR_DMAEN 0x02
#define I2CR_RSTA 0x04
#define I2CR_TXAK 0x08
#define I2CR_MTX 0x10
@@ -174,6 +186,17 @@ struct imx_i2c_hwdata {
unsignedi2cr_ien_opcode;
};
+struct imx_i2c_dma {
+ struct dma_chan *chan_tx;
+ struct dma_chan *chan_rx;
+ struct dma_chan *chan_using;
+ struct completion cmd_complete;
+ dma_addr_t dma_buf;
+ unsigned intdma_len;
+ unsigned intdma_transfer_dir;
+ unsigned intdma_data_dir;
+};
+
struct imx_i2c_struct {
struct i2c_adapter adapter;
struct clk *clk;
@@ -186,6 +209,8 @@ struct imx_i2c_struct {
unsigned intcur_clk;
unsigned intbitrate;
const struct imx_i2c_hwdata *hwdata;
+
+ struct imx_i2c_dma *dma;
};
static const struct imx_i2c_hwdata imx1_i2c_hwdata = { @@ -256,6 +281,132 @@
static inline unsigned char imx_i2c_read_reg(struct imx_i2c_struct *i2c_imx,
return readb(i2c_imx->base + (reg << i2c_imx->hwdata->regshift)); }
+/* Functions for DMA support */
+static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
+ dma_addr_t phy_addr)
+{
+ struct imx_i2c_dma *dma;
+ struct dma_slave_config dma_sconfig;
+ struct device *dev = &i2c_imx->adapter.dev;
+ int ret;
+
+ dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
+ if (!dma)
+ return -ENOMEM;
+
+ dma->chan_tx = dma_request_slave_channel(dev, "tx");
+ if (!dma->chan_tx) {
+ dev_dbg(dev, "can't request DMA tx channel\n");
+ ret = -ENODEV;
+ goto fail_al;
+ }
+
+ dma_sconfig.dst_addr = phy_addr +
+ (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
+ dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.dst_maxburst = 1;
+ dma_sconfig.direction = DMA_MEM_TO_DEV;
+ ret = dmaengine_slave_config(dma->chan_tx, &dma_sconfig);
+ if (ret < 0) {
+ dev_dbg(dev, "can't configure tx channel\n");
+ goto fail_tx;
+ }
+
+ dma->chan_rx = dma_request_slave_channel(dev, "rx");
+ if (!dma->chan_rx) {
+ dev_dbg(dev, "can't request DMA rx channel\n");
+ ret = -ENODEV;
+ goto fail_tx;
+ }
+
+ dma_sconfig.src_addr = phy_addr +
+ (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
+ dma_sconfig.
RE: [PATCH v6 1/2] i2c: imx: add DMA support for freescale i2c driver
Hi Fugang,
> >> >+ /* Waiting for Transfer complete. */
> >> >+ while (timeout--) {
> >> >+ temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> >> >+ if (temp & I2SR_ICF)
> >> >+ break;
> >> >+ udelay(10);
> >> >+ }
> >> Whether there have better method like interrupt to avoid dead wait
> >> here until timeout ?
> >
> >Can you give me more suggestion? We have discussed it with our team, It
> >seems the short query wait is necessary.
> >
> At least, you can use schdule_timeout() instead of udelay() ?
In fact, the waiting time normally is less than 10-50us, but the minimum time
interval for schdule_timeout() is 1 jiffies.
So maybe schdule_timeout() is not very necessary?
Thanks for your review.
Best Regards,
Yuan Yao
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v6 1/2] i2c: imx: add DMA support for freescale i2c driver
Hi Fugang,
Duan Fugang wrote:
[...]
>+ dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>+ dma_sconfig.dst_maxburst = 1;
> The maxburst is 1 cause very slow efficiency for DMA copy, which system
> performance even is slower Than cpu mode.
There is no FIFO for IMX i2c, so the maxburst shoud be 1 for hardware request.
[...]
> >+/* Waiting for Transfer complete. */
> >+while (timeout--) {
> >+temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> >+if (temp & I2SR_ICF)
> >+break;
> >+udelay(10);
> >+}
> Whether there have better method like interrupt to avoid dead wait here until
> timeout ?
Can you give me more suggestion? We have discussed it with our team, It seems
the short query wait is necessary.
> >+
> >+if (!timeout)
> >+return -ETIMEDOUT;
> >+temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> >+temp &= ~I2CR_DMAEN;
> >+imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> >+
> >+/* The last data byte must be transferred by the CPU. */
> >+imx_i2c_write_reg(msgs->buf[msgs->len-1],
> >+i2c_imx, IMX_I2C_I2DR);
> >+result = i2c_imx_trx_complete(i2c_imx);
> >+if (result)
> >+return result;
> I don't understand why the last data need to be transmited by cpu. I guess you
> want to get IIF interrupt ?
Yes, we need the interrupt to do some mop-up.
Also follow the hardware request as:
The following flow diagram details exactly the operation for using a DMA
controller to
transmit "n" data bytes to a slave. The first byte (the slave calling address)
is always
transmitted by the CPU. All subsequent data bytes (apart from the last data
byte) can be
transferred by the DMA controller. The last data byte must be transferred by
the CPU.
> >+
> >+result = i2c_imx_acked(i2c_imx);
> >+if (result)
> >+return result;
> >+
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v6 1/2] i2c: imx: add DMA support for freescale i2c driver
Thanks for your review. Varka Bhadram wrote: > On 08/05/2014 03:26 PM, Yuan Yao wrote: > > (...) > > +fail_rx: > > + dma_release_channel(dma->chan_rx); > > +fail_tx: > > + dma_release_channel(dma->chan_tx); > > +fail_al: > > + devm_kfree(dev, dma); > > no need to use devm_kfree() if we use devm_kzalloc()... > We have discussed it before. As Lothar Waßmann said: "The devm_kfree() is not in the failure path of the driver's probe() function, but in the function that tries to initialize the optional DMA support." So It seems need to use devm_kfree(). Do you have some other opinions? > > + dev_info(dev, "can't use DMA\n"); > > + > > + return ret; > > +} (...) > > +static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx, > > + struct i2c_msg *msgs) > > static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx, >struct i2c_msg *msgs) > > run checkpatch.pl on this patch... Sorry for my code style. I will match open parenthesis in this case. I had run this script before, just one warning. > -- > Regards, > Varka Bhadram. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v5 1/2] i2c: add DMA support for freescale i2c driver
Marek Vasut wrote: > On Wednesday, July 23, 2014 at 02:15:02 PM, Lothar Waßmann wrote: > > Hi, > > > > Varka Bhadram wrote: > > > On 07/23/2014 04:41 PM, Yao Yuan wrote: > > > > Hi, > > > > > > > > Thanks for your review. > > > > > > > > Lothar Waßmann wrote: > > > >> Yuan Yao wrote: > > > >>> Add dma support for i2c. This function depend on DMA driver. > > > >>> You can turn on it by write both the dmas and dma-name > > > >>> properties in dts node. > > > >>> > > > >>> Signed-off-by: Yuan Yao > > > >>> --- > > > >>> > > > >>> drivers/i2c/busses/i2c-imx.c | 377 > > > > > > > > [...] > > > > > > > >>> + > > > >>> +fail_rx: > > > >>> + dma_release_channel(dma->chan_rx); > > > >>> +fail_tx: > > > >>> + dma_release_channel(dma->chan_tx); > > > >>> +fail_al: > > > >>> + devm_kfree(dev, dma); > > > >> > > > >> No need for this one (that's the whole point of using devm_kzalloc())! > > > > > > > > When DMA request failed, I2C will switch to PIO mode. So if the > > > > failed reason is just like DMA channel request failed. At this > > > > time the DMA should free by devm_kfree(). Is it? > > > > > > If probe failed the memory will be freed automatically because we > > > are using devm_kzalloc()... > > > > > > If we use devm_kzalloc() ,no need to free manually on fail... > > > > Yes, but as Yuan Yao stated, the driver will still work without DMA > > but carry around the unecessary allocated imx_i2c_dma struct. > > The devm_kfree() is not in the failure path of the driver's probe() > > function, but in the function that tries to initialize the optional > > DMA support. > > If the DMA fails, I'd just make the entire probe fail. In case you cannot > probe > DMA for your hardware, which is exected to be DMA capable, it means > something is wrong anyway. Yes, but if there is something wrong in dma, I think the i2c is innocent. So I think the error message is necessary but i2c can continue work. Best regards, Yuan Yao -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v5 0/2] i2c: add DMA support for freescale i2c driver
Hi, Marek Vasut wrote: > On Wednesday, July 23, 2014 at 10:24:41 AM, Yuan Yao wrote: > > Changed in v5: > > - add "*chan_dev = dma->chan_using->device->dev" for reduce the call time. > > Did you check if the compiler generates different code ? > Sorry, I didn't compare the assembly code. It's a subtle change. As you mentioned the "noodle" before. Old: dma_map_single(dma->chan_using->device->dev, ...); dma_mapping_error(dma->chan_using->device->dev, ...); dma_unmap_single(dma->chan_using->device->dev, ...); New: struct device *chan_dev = dma->chan_using->device->dev; dma_map_single(chan_dev, ...); dma_mapping_error(chan_dev, ...); dma_unmap_single(chan_dev, ...); > > - add the test logs. > > [...] > > Best regards, > Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 1/2] i2c: add DMA support for freescale i2c driver
On Fri, May 21, 2014 at 4:01 PM +0200, Wolfram Sang wrote: > > Ping. Any updates? I think this was pretty close to inclusion? Hi, Wolfram Thanks for your concern. Sorry for my reply so late. I had on a business trip for months. At that time I have no device to debug it. Now, I'm come back and I will try my best to finish it. I have sent the patch for V5. Thanks for your review. Best regards, Yuan Yao -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v5 1/2] i2c: add DMA support for freescale i2c driver
Hi,
Thanks for your review.
Lothar Waßmann wrote:
> Yuan Yao wrote:
> > Add dma support for i2c. This function depend on DMA driver.
> > You can turn on it by write both the dmas and dma-name properties in dts
> > node.
> >
> > Signed-off-by: Yuan Yao
> > ---
> > drivers/i2c/busses/i2c-imx.c | 377
[...]
> > +
> > +fail_rx:
> > + dma_release_channel(dma->chan_rx);
> > +fail_tx:
> > + dma_release_channel(dma->chan_tx);
> > +fail_al:
> > + devm_kfree(dev, dma);
> >
> No need for this one (that's the whole point of using devm_kzalloc())!
>
When DMA request failed, I2C will switch to PIO mode. So if the failed reason
is just like DMA channel request failed. At this time the DMA should free by
devm_kfree(). Is it?
[...]
Thanks.
Yuan Yao
N�r��yb�X��ǧv�^�){.n�+{��g"��^n�r���z���h�&�� �G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�
Re: [PATCH v4 1/2] i2c: add DMA support for freescale i2c driver
On Friday, April 04, 2014 at 22:29:32 PM, Marek Vasut wrote:
> On Friday, April 04, 2014 at 04:41:11 AM, Yuan Yao wrote:
> > Add dma support for i2c. This function depend on DMA driver.
> > You can turn on it by write both the dmas and dma-name properties in dts
> > node.
> >
> > Signed-off-by: Yuan Yao
[...]
> Since you will be fixing that superfluous return 0; (I actually wonder, did
> you
> really test the driver at all before submitting it?) ...
I'm sorry for the leftover. It's a serious bug. The support by DMA will be
disabled always.
Thanks for your consideration. But subjective assume may not suitable. Testing
is a essential link .
But the bug can't be found by normal test . The testing may not be imperfect,
it's my
serious fault, but please don't doubt my work attitude.
The ugly is my work or even the ability but not the attitude, I think.
At last, Thank you for giving me a lot of help, it's very important for me.
> > +static int i2c_imx_dma_xfer(struct imx_i2c_struct *i2c_imx,
> > + struct i2c_msg *msgs)
> > +{
> > + struct imx_i2c_dma *dma = i2c_imx->dma;
> > + struct dma_async_tx_descriptor *txdesc;
> > + struct device *dev = &i2c_imx->adapter.dev;
> +
> + dma->dma_buf = dma_map_single(dma->chan_using->device->dev, msgs->buf,
[...]
> Reviewed-by: Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
On Wednesday, March 26, 2014 at 02:27:46 PM, Marek Vasut wrote:
> On Wednesday, March 26, 2014 at 06:56:34 AM, Yao Yuan wrote:
> > On Wednesday, March 26, 2014 at 11:43:27 AM, Marek Vasut wrote:
> > > On Wednesday, March 26, 2014 at 04:08:27 AM, Yao Yuan wrote:
> > >
> > > [...]
> > >
> > > > > > + i2c_imx->use_dma = false;
> > > > > > + } else if (i2c_imx_dma_request(i2c_imx,
> > > > > > +(dma_addr_t)phy_addr))
> > >
> > > {
> > >
> > > > > > + dev_info(&pdev->dev,
> > > > > > + "can't request dma chan, faild use dma.
> \n");
> > > > > > + i2c_imx->use_dma = false;
> > > > > > + } else {
> > > > > > + i2c_imx->use_dma = true;
> > > > > > + }
> > > > >
> > > > > Can you not just check if i2c_imx->dma is valid pointer or NULL
> > >
> > > pointer ?
> > >
> > > > > Then you won't need a separate variable, for this purpose ...
> right ?
> > > >
> > > > Sorry and I think what I know is just to check whether it is NULL.
> > > > Then for the second question, maybe there are some other ways, But
> > > > I think it is more tidy and easier understanding for using a
> > > > separate variable, for this purpose.
> > >
> > > You are just wasting space and duplicating data, unless I am wrong.
> >
> > Well, Do you have a better idea? Although I think it's necessary.
>
> I think we disconnected here, sorry. Why can you not use (i2c_imx->dma !=
> NULL) instead of (i2c_imx->use_dma == true) please ?
>
But there are two judge conditions. But only the "i2c_imx->dma", but also
whether "i2c_imx_dma_request" success.
"i2c_imx->use_dma == true" be equivalent to "i2c_imx->dma != NULL &&
!i2c_imx_dma_request()"
RE: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
On Wednesday, March 26, 2014 at 11:43:27 AM, Marek Vasut wrote:
> On Wednesday, March 26, 2014 at 04:08:27 AM, Yao Yuan wrote:
>
> [...]
>
> > > > + i2c_imx->use_dma = false;
> > > > + } else if (i2c_imx_dma_request(i2c_imx, (dma_addr_t)phy_addr))
> {
> > > > + dev_info(&pdev->dev,
> > > > + "can't request dma chan, faild use
> > > > dma.\n");
> > > > + i2c_imx->use_dma = false;
> > > > + } else {
> > > > + i2c_imx->use_dma = true;
> > > > + }
> > >
> > > Can you not just check if i2c_imx->dma is valid pointer or NULL
> pointer ?
> > > Then you won't need a separate variable, for this purpose ... right ?
> >
> > Sorry and I think what I know is just to check whether it is NULL.
> > Then for the second question, maybe there are some other ways, But I
> > think it is more tidy and easier understanding for using a separate
> > variable, for this purpose.
>
> You are just wasting space and duplicating data, unless I am wrong.
>
Well, Do you have a better idea? Although I think it's necessary.
Best regards,
Yuan Yao
N�r��yb�X��ǧv�^�){.n�+{��g"��^n�r���z���h�&�� �G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�
RE: [PATCH v3 1/2] i2c: add DMA support for freescale i2c driver
On Sunday, March 23, 2014 @ 11:50:00 AM, Marek Vasut wrote:
> On Thursday, March 13, 2014 at 02:47:56 AM, Yuan Yao wrote:
> > Add dma support for i2c. This function depend on DMA driver.
> > You can turn on it by write both the dmas and dma-name properties in
> > dts node.
> >
> > Signed-off-by: Yuan Yao
> > ---
> > drivers/i2c/busses/i2c-imx.c | 354
> > +-- 1 file changed, 306
> > insertions(+), 48 deletions(-)
>
> Changelog is missing.
Sorry for this, Maybe the changelog is in the junk box. It's named "[PATCH v3
0/2] i2c: add DMA support for freescale i2c driver".
> [...]
>
> > +/* Functions for DMA support */
> > +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
> > + dma_addr_t phy_addr)
> > +{
> > + struct imx_i2c_dma *dma = i2c_imx->dma;
> > + struct dma_slave_config dma_sconfig;
> > + struct device *dev = &i2c_imx->adapter.dev;
> > + int ret;
> > +
> > + dma->chan_tx = dma_request_slave_channel(dev, "tx");
> > + if (!dma->chan_tx) {
> > + dev_err(dev, "Dma tx channel request failed!\n");
>
> DMA is written in all caps, it's an abbrev. for Direct Memory Access .
> Please fix globally.
>
Ok, I will fix it globally.
> [...]
>
> > static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct
> > i2c_msg
> > *msgs) {
>
> [...]
>
> > - /* write data */
> > - for (i = 0; i < msgs->len; i++) {
> > - dev_dbg(&i2c_imx->adapter.dev,
> > - "<%s> write byte: B%d=0x%X\n",
> > - __func__, i, msgs->buf[i]);
> > - imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
> > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > + temp |= I2CR_DMAEN;
> > + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +
> > + /* write slave address */
> > + imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> > + result = wait_for_completion_interruptible_timeout(
> > + &i2c_imx->dma->cmd_complete,
> > + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
> > + if (result <= 0) {
> > + dmaengine_terminate_all(dma->chan_using);
> > + if (result)
> > + return result;
> > + else
> > + return -ETIMEDOUT;
> > + }
> > +
> > + /* waiting for Transfer complete. */
> > + while (timeout--) {
> > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> > + if (temp & I2SR_ICF)
> > + break;
> > + udelay(10);
> > + }
>
> Do you ever check if this really timed out and handle such case at all ?
> I don't see it , but I might be wrong ...
>
Oh, It's a bug, Thank you very much.
> > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> > + temp &= ~I2CR_DMAEN;
> > + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> > +
> > + /* write the last byte */
> > + imx_i2c_write_reg(msgs->buf[msgs->len-1],
> > + i2c_imx, IMX_I2C_I2DR);
> > result = i2c_imx_trx_complete(i2c_imx);
> > if (result)
> > return result;
> > +
> > result = i2c_imx_acked(i2c_imx);
> > if (result)
> > return result;
> > + } else {
> > + /* write slave address */
> > + imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> > + result = i2c_imx_trx_complete(i2c_imx);
> > + if (result)
> > + return result;
> > +
> > + result = i2c_imx_acked(i2c_imx);
> > + if (result)
> > + return result;
> > +
> > + dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__);
> > +
> > + /* write data */
> > + for (i = 0; i < msgs->len; i++) {
> > + dev_dbg(&i2c_imx->adapter.dev,
> > + "<%s> write byte: B%d=0x%X\n",
> > + __func__, i, msgs->buf[i]);
>
> Can you not just have a variable $dev here and avoid having such a long
> noodle in the dev_dbg() call ?
>
Ok, And I don't think the dev_dbg() is very helpful here now.
> > + imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
> > + result = i2c_imx_trx_complete(i2c_imx);
> > + if (result)
> > + return result;
> > + result = i2c_imx_acked(i2c_imx);
> > + if (result)
> > + return result;
> > + }
> > }
> > return 0;
> > }
> [...]
>
> > + /* waiting for Transfer complete. */
> > + while (timeout--) {
> > + temp = imx_i2c_read_reg(i2c_imx
RE: [PATCH v2 1/2] i2c: add DMA support for freescale i2c driver
On Thu, Mar 10, 2014 at 10:01:42 AM, Marek Vasut wrote:
> On Thu, Mar 06, 2014 at 12:57:42PM +0100, Marek Vasut wrote:
> > On Thursday, March 06, 2014 at 06:02:03 AM, Yao Yuan wrote:
> > > On Thu, March 06, 2014 at 12:44:14 PM, Marek Vasut wrote:
> > > > On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote:
> > > > > On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote:
> > > > > > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote:
> > > > > > > Add dma support for i2c. This function depend on DMA driver.
> > > > > > > You can turn on it by write both the dmas and dma-name
> > > > > > > properties in dts node.
> > > > > > >
> > > > > > > Signed-off-by: Yuan Yao
> > > > > > > ---
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct
> > > > > > > platform_device
> > > > > >
> > > > > > *pdev)
> > > > > >
> > > > > > > void __iomem *base;
> > > > > > > int irq, ret;
> > > > > > > u32 bitrate;
> > > > > > >
> > > > > > > + u32 phy_addr;
> > > > > > >
> > > > > > > dev_dbg(&pdev->dev, "<%s>\n", __func__);
> > > > > > >
> > > > > > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct
> > > > > > > platform_device
> > > > > >
> > > > > > *pdev)
> > > > > >
> > > > > > > }
> > > > > > >
> > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > > > >
> > > > > > > + phy_addr = res->start;
> > > > > >
> > > > > > Uh ... Shawn, I really think I am lost here. Don't you need to
> > > > > > map this memory before you can use it for DMA ? The DMA
> > > > > > mapping function should give you the physical address and is
> > > > > > the right way to go about this instead of pulling the address
> from here, no ?
> > > > > >
> > > > > > I might be wrong here, I am rather uncertain, so please help me
> out.
> > > > > > Thanks!
> > > > >
> > > > > Hi, Marek, Thanks for your suggestion.
> > > > > Here you can review the code in include/linux/ioport.h The
> > > > > resource->start describes the entity on the CPU bus as a
> > > > > resource->starting
> > > > > physical address. So I thinks it can used for dma directly.
> > > >
> > > > This doesn't feel right for some reason. If this is a register
> > > > area, you should
> > > > ioremap() it. If it's a memory area you do DMA to/from, you need
> > > > to make sure you correctly flush/invalidate caches and properly
> > > > handle the effects the write buffer might have. But I have a
> > > > feeling you actually do DMA to/from register space here ?
> > >
> > > Yes, It's a register area. But I don't know why I should ioremap()
> > > it? It's a bus address and DMA can use it directly. Is there some
> > > problem for my understanding ?
> >
> > I am not too sure here, thus I am poking someone who can clearly answer
> this.
>
> There is already a devm_ioremap_resource() call in the existing code for
> CPU to access registers in virtual address. And my understanding on
> Yuan's patch is that he needs the physical address of I2C DATA register
> for DMA from/to the controller.
>
> Shawn
Thanks you Shawn. Yes, it is. DMA need the physical address which
resource->start is.
And Hi Marek, How about you? Is there someone who can clearly answer this some
and should I send the next version?
N�r��yb�X��ǧv�^�){.n�+{��g"��^n�r���z���h�&�� �G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�
答复: [PATCH v2 1/2] i2c: add DMA support for freescale i2c driver
On Thursday, March 06, 2014 at 09:46:03 PM, Marek Vasut wrote: >On Thursday, March 06, 2014 at 06:02:03 AM, Yao Yuan wrote: > > On Thu, March 06, 2014 at 12:44:14 PM, Marek Vasut wrote: > > > On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote: > > > > On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > > > > > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > > > > > Add dma support for i2c. This function depend on DMA driver. > > > > > > You can turn on it by write both the dmas and dma-name properties > > > > > > in dts node. > > > > > > > > > > > > Signed-off-by: Yuan Yao > > > > > > --- > > > > > > > > > > [...] > > > > > > > > > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct > > > > > > platform_device > > > > > > > > > > *pdev) > > > > > > > > > > > void __iomem *base; > > > > > > int irq, ret; > > > > > > u32 bitrate; > > > > > > > > > > > >+ u32 phy_addr; > > > > > > > > > > > > dev_dbg(&pdev->dev, "<%s>\n", __func__); > > > > > > > > > > > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct > > > > > > platform_device > > > > > > > > > > *pdev) > > > > > > > > > > > } > > > > > > > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > > > > > > > > + phy_addr = res->start; > > > > > > > > > > Uh ... Shawn, I really think I am lost here. Don't you need to map > > > > > this memory before you can use it for DMA ? The DMA mapping function > > > > > should give you the physical address and is the right way to go > > > > > about this instead of pulling the address from here, no ? > > > > > > > > > > I might be wrong here, I am rather uncertain, so please help me out. > > > > > Thanks! > > > > > > > > Hi, Marek, Thanks for your suggestion. > > > > Here you can review the code in include/linux/ioport.h The > > > > resource->start describes the entity on the CPU bus as a starting > > > > physical address. So I thinks it can used for dma directly. > > > > > > This doesn't feel right for some reason. If this is a register area, you > > > should > > > ioremap() it. If it's a memory area you do DMA to/from, you need to make > > > sure you correctly flush/invalidate caches and properly handle the > > > effects the write buffer might have. But I have a feeling you actually do > > > DMA to/from register space here ? > > > > Yes, It's a register area. But I don't know why I should ioremap() it? It's > > a bus address and DMA can use it directly. Is there some problem for my > > understanding ? > > I am not too sure here, thus I am poking someone who can clearly answer this. OK, and thank you for pointing out many ugly code in my patch. It's very useful for me.
RE: [PATCH v2 1/2] i2c: add DMA support for freescale i2c driver
On Thu, March 06, 2014 at 12:44:14 PM, Marek Vasut wrote:
> On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote:
> > On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote:
> > > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote:
> > > > Add dma support for i2c. This function depend on DMA driver.
> > > > You can turn on it by write both the dmas and dma-name properties
> > > > in dts node.
> > > >
> > > > Signed-off-by: Yuan Yao
> > > > ---
> > >
> > > [...]
> > >
> > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct
> > > > platform_device
> > >
> > > *pdev)
> > >
> > > > void __iomem *base;
> > > > int irq, ret;
> > > > u32 bitrate;
> > > >
> > > > + u32 phy_addr;
> > > >
> > > > dev_dbg(&pdev->dev, "<%s>\n", __func__);
> > > >
> > > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct
> > > > platform_device
> > >
> > > *pdev)
> > >
> > > > }
> > > >
> > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > >
> > > > + phy_addr = res->start;
> > >
> > > Uh ... Shawn, I really think I am lost here. Don't you need to map
> > > this memory before you can use it for DMA ? The DMA mapping function
> > > should give you the physical address and is the right way to go
> > > about this instead of pulling the address from here, no ?
> > >
> > > I might be wrong here, I am rather uncertain, so please help me out.
> > > Thanks!
> >
> > Hi, Marek, Thanks for your suggestion.
> > Here you can review the code in include/linux/ioport.h The
> > resource->start describes the entity on the CPU bus as a starting
> > physical address. So I thinks it can used for dma directly.
>
> This doesn't feel right for some reason. If this is a register area, you
> should
> ioremap() it. If it's a memory area you do DMA to/from, you need to make
> sure you correctly flush/invalidate caches and properly handle the
> effects the write buffer might have. But I have a feeling you actually do
> DMA to/from register space here ?
>
Yes, It's a register area. But I don't know why I should ioremap() it? It's a
bus address and DMA can use it directly.
Is there some problem for my understanding ?
N�r��yb�X��ǧv�^�){.n�+{��g"��^n�r���z���h�&�� �G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�
RE: [PATCH v2 1/2] i2c: add DMA support for freescale i2c driver
On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > Add dma support for i2c. This function depend on DMA driver. > > You can turn on it by write both the dmas and dma-name properties in > > dts node. > > > > Signed-off-by: Yuan Yao > > --- > > [...] > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct platform_device > *pdev) > > void __iomem *base; > > int irq, ret; > > u32 bitrate; > > + u32 phy_addr; > > > > dev_dbg(&pdev->dev, "<%s>\n", __func__); > > > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct platform_device > *pdev) > > } > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + phy_addr = res->start; > > Uh ... Shawn, I really think I am lost here. Don't you need to map this > memory before you can use it for DMA ? The DMA mapping function should > give you the physical address and is the right way to go about this > instead of pulling the address from here, no ? > > I might be wrong here, I am rather uncertain, so please help me out. > Thanks! Hi, Marek, Thanks for your suggestion. Here you can review the code in include/linux/ioport.h The resource->start describes the entity on the CPU bus as a starting physical address. So I thinks it can used for dma directly.
RE: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
Hi, Marek
Marek Vasut wrote:
> On Thursday, February 27, 2014 at 07:05:14 AM, Yuan Yao wrote:
>
> [...]
>
> > +static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx) {
> > + struct imx_i2c_dma *dma = i2c_imx->dma;
> > + struct dma_chan *dma_chan;
> > +
> > + dma_chan = dma->chan_tx;
> > + dma->chan_tx = NULL;
> > + dma->buf_tx = 0;
> > + dma->len_tx = 0;
> > + dma_release_channel(dma_chan);
> > +
> > + dma_chan = dma->chan_rx;
> > + dma->chan_tx = NULL;
> > + dma->buf_rx = 0;
> > + dma->len_rx = 0;
> > + dma_release_channel(dma_chan);
>
> You must make _DEAD_ _SURE_ this function is not ever called while the
> DMA is still active. In your case, I have a feeling that's not handled.
>
Thanks for your attention.
This few days I look up the code for the realization of dma_release_channel().
I found that it will disable the dma request first. So it's may safe.
And drivers hadn't check whether dma is still active before
dma_release_channel() as a usually usage.
Because it will be disabled automatic.
The only problem is that, it will be forced to cancel the transfer which was
not yet completed.
Looking forward to hearing from you.
Best regards,
Yuan Yao
N�r��yb�X��ǧv�^�){.n�+{��g"��^n�r���z���h�&�� �G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�
RE: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
Hi Marek,
> On Friday, February 28, 2014 at 06:19:18 AM, Yao Yuan wrote:
>
> [...]
>
> > > > @@ -213,6 +238,7 @@ static struct imx_i2c_hwdata vf610_i2c_hwdata
> > > > = {
> > > >
> > > > .ndivs = ARRAY_SIZE(vf610_i2c_clk_div),
> > > > .i2sr_clr_opcode= I2SR_CLR_OPCODE_W1C,
> > > > .i2cr_ien_opcode= I2CR_IEN_OPCODE_0,
> > > >
> > > > + .has_dma_support= true,
> > >
> > > So why exactly don't we have a DT prop for determining whether the
> > > controller has DMA support ?
> > >
> > > What about the other controllers, do they not support DMA for some
> > > specific reason? Please elaborate on that, thank you !
> >
> > Sorry for my fault. I will modify it.
>
> I would prefer if you could explain why other controllers do have DMA
> disabled even if the hardware does support the DMA operation.
>
Well, Because of the I2C in I.MX hardware don't support the DMA operation.
But here I also think has_dma_support isn't necessary.
> > > Also, can the DMA not do full-duplex operation ? What I see here is
> > > just
> > > half- duplex operations , one for RX and the other one for TX .
> >
> > Yes, here have two dma channels, one for RX and the other one for TX.
> > When we request the channel we should determine it for TX or RX.
>
> Sorry, I don't quite understand this. If you have two DMA channels, can
> you not use them both to do full-duplex SPI transfer ?
>
Sorry, There are also hard for me. I don't understand what is full-duplex for
dma?
A DMA engine can only read or write at the same time. And a dma channel request
for only DMA_MEM_TO_DEV or DMA_DEV_TO_MEM almost.
Also i2c is a half-duplex bus.
N�r��yb�X��ǧv�^�){.n�+{��g"��^n�r���z���h�&�� �G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�
RE: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
Hi Marek,
Thank you very much for your suggestion.
> -Original Message-
> From: Marek Vasut [mailto:ma...@denx.de]
> Sent: Friday, February 28, 2014 4:40 AM
> To: linux-arm-ker...@lists.infradead.org
> Cc: Yuan Yao-B46683; w...@the-dreams.de; mark.rutl...@arm.com;
> shawn@linaro.org; linux-ker...@vger.kernel.org; linux-
> i...@vger.kernel.org
> Subject: Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
>
> On Thursday, February 27, 2014 at 07:05:14 AM, Yuan Yao wrote:
>
> [...]
>
> > */ @@ -63,6 +68,9 @@
> > /* Default value */
> > #define IMX_I2C_BIT_RATE 10 /* 100kHz */
> >
> > +/* enable DMA if transfer size is bigger than this threshold */
> > +#define IMX_I2C_DMA_THRESHOLD 16
>
> So what's the unit here , potatoes or beers or what ? I suppose it's
> bytes , but please make it explicit in the comment ...
>
Yes it's bytes. I will make it explicit in the comment.
> [...]
>
> > static const struct imx_i2c_hwdata imx1_i2c_hwdata = { @@ -193,6
> > +216,7 @@ static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
> > .ndivs = ARRAY_SIZE(imx_i2c_clk_div),
> > .i2sr_clr_opcode= I2SR_CLR_OPCODE_W0C,
> > .i2cr_ien_opcode= I2CR_IEN_OPCODE_1,
> > + .has_dma_support= false,
> >
> > };
> >
> > @@ -203,6 +227,7 @@ static const struct imx_i2c_hwdata imx21_i2c_hwdata
> =
> > { .ndivs= ARRAY_SIZE(imx_i2c_clk_div),
> > .i2sr_clr_opcode= I2SR_CLR_OPCODE_W0C,
> > .i2cr_ien_opcode= I2CR_IEN_OPCODE_1,
> > + .has_dma_support= false,
> >
> > };
> >
> > @@ -213,6 +238,7 @@ static struct imx_i2c_hwdata vf610_i2c_hwdata = {
> > .ndivs = ARRAY_SIZE(vf610_i2c_clk_div),
> > .i2sr_clr_opcode= I2SR_CLR_OPCODE_W1C,
> > .i2cr_ien_opcode= I2CR_IEN_OPCODE_0,
> > + .has_dma_support= true,
>
> So why exactly don't we have a DT prop for determining whether the
> controller has DMA support ?
>
> What about the other controllers, do they not support DMA for some
> specific reason? Please elaborate on that, thank you !
Sorry for my fault. I will modify it.
> [...]
>
> > +static void i2c_imx_dma_tx_callback(void *arg)
> [...]
> > +static int i2c_imx_dma_tx(struct imx_i2c_struct *i2c_imx, struct
> > +i2c_msg
> > *msgs) +{
> [...]
> > +static void i2c_imx_dma_rx_callback(void *arg)
> [...]
> > +static int i2c_imx_dma_rx(struct imx_i2c_struct *i2c_imx, struct
> > +i2c_msg
> > *msgs) +{
> [...]
>
> Looks like there's quite a bit of code duplication in the four functions
> above, can you not unify them ?
>
Yes, There's looks like quite a bit of code duplication in the four functions
above.
I also hate quite a bit of code duplication.
But there are many differences in fact.
If unify them we should add many conditional statements and auxiliary variable.
I think it's superfluous and will damage the readability.
So, I am very confused. And if you think unify them will be better I will
modify it.
Thanks for your suggestion and looking forward to hearing from you.
> Also, can the DMA not do full-duplex operation ? What I see here is just
> half- duplex operations , one for RX and the other one for TX .
>
Yes, here have two dma channels, one for RX and the other one for TX.
When we request the channel we should determine it for TX or RX.
> > +static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx) {
> > + struct imx_i2c_dma *dma = i2c_imx->dma;
> > + struct dma_chan *dma_chan;
> > +
> > + dma_chan = dma->chan_tx;
> > + dma->chan_tx = NULL;
> > + dma->buf_tx = 0;
> > + dma->len_tx = 0;
> > + dma_release_channel(dma_chan);
> > +
> > + dma_chan = dma->chan_rx;
> > + dma->chan_tx = NULL;
> > + dma->buf_rx = 0;
> > + dma->len_rx = 0;
> > + dma_release_channel(dma_chan);
>
> You must make _DEAD_ _SURE_ this function is not ever called while the
> DMA is still active. In your case, I have a feeling that's not handled.
>
I think this function will not called while the DMA is still
active because of the Linux synchronization mechanism - completion.
I used it in the dma function.
> > +}
> > /** Functions for IMX I2C adapter driver
> > ***
> > **
> >
> > */
> >
> > @@ -425,7 +600,8 @@ static irqreturn_t i2c_imx_isr(int irq, void
> *dev_id)
> > return IRQ_NONE;
> > }
> >
> > -static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct
> > i2c_msg
> > *msgs) +static int i2c_imx_pio_write(struct imx_i2c_struct *i2c_imx,
> > + struct i2c_msg *msgs)
> > {
> > int i, result;
> >
> > @@ -458,7 +634,56 @@ static int i2c_imx_write(struct imx_i2c_struct
> > *i2c_imx, struct i2c_msg *msgs) return 0; }
> >
> > -static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct
> > i2c_msg
> > *msgs) +static int i2c_imx_dma_write(struct imx_i2c
