Re: [PATCH v7 1/2] i2c: imx: add DMA support for freescale i2c driver
On Thursday, September 18, 2014 at 05:46:04 PM, Yao Yuan wrote: > 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. OK, I am a bit lost, so let's see V8 and then go with that one I'd say. 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 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 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? > 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. 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 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 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 ?
> 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.
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 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
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 ?
[...]
--
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, 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 ?
> + */
> +#define IMX_I2C_DMA_THRESHOLD16
> +#define IMX_I2C_DMA_TIMEOUT 1000
> +
[...]
> +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?
> + }
> +
> + /* 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;
+
[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.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) {
+ dev_dbg(dev, "can't configure rx channel\n");
+ goto fail_rx;
+ }
+
+ i2c_imx->dma = dma;
+ init_completion(&dma->cmd_complete);
+ dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n",
+ dma_chan_name(dma->chan_tx), dma_chan_name(dma->chan_rx));
+
+ return 0;
+
+fail_rx:
+ dma_release_channel(
[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(struct imx_i2c_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) {
+ dev_dbg(dev, "can't configure rx channel\n");
+ goto fail_rx;
+ }
+
+ i2c_imx->dma = dma;
+ init_completion(&dma->cmd_complete);
+ dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n",
+ dma_chan_name(dma->chan_tx), dma_chan_name(dma->chan_rx));
+
+ return 0;
+
+fail_rx:
+ dma_re
