Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Monday, March 03, 2014 at 11:23:33 AM, Yao Yuan wrote:
> 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.
OK
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
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�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Monday, March 03, 2014 at 11:23:33 AM, Yao Yuan wrote:
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.
OK
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Friday, February 28, 2014 at 11:59:25 AM, Lothar Waßmann wrote: > Hi, > > Marek Vasut wrote: > > On Friday, February 28, 2014 at 06:19:18 AM, Yao Yuan wrote: > > > > [...] > > > > > 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 ? > > SPI != I2C You got me, sorry. Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Friday, February 28, 2014 at 12:36:01 PM, Yao Yuan wrote:
> 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.
OK, got it now. Thanks!
> > > > 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?
Sorry, nevermind. I was confused by this and some SPI patches review. Like
Lothar (thanks!) pointed out, using only half-duplex operation is OK.
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
Hi, Marek Vasut wrote: > On Friday, February 28, 2014 at 06:19:18 AM, Yao Yuan wrote: > > [...] > > 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 ? > SPI != I2C Lothar Waßmann -- ___ Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10 Geschäftsführer: Matthias Kaussen Handelsregistereintrag: Amtsgericht Aachen, HRB 4996 www.karo-electronics.de | i...@karo-electronics.de ___ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Friday, February 28, 2014 at 03:23:52 AM, Shawn Guo wrote:
> On Fri, Feb 28, 2014 at 10:13:02AM +0800, Shawn Guo wrote:
> > On Thu, Feb 27, 2014 at 09:39:35PM +0100, Marek Vasut wrote:
> > > > @@ -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 ?
> >
> > Using DMA or PIO is a decision that should be made by driver on its own,
> > not device tree.
>
> Sorry. I misunderstood it. Yes, we can look at the DT property 'dmas'
> to know if the controller has DMA capability.
You're right. Whether or not does the hardware support DMA transfers is a
property of the hardware, that's why it should be in DT.
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
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.
> > [...]
> >
> > > +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.
I'd say try it, the RX and TX callback look almost the same. So does the
i2c_imx_dma_rx() and i2c_imx_dma_tx() .
> > 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 ?
> > > +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.
This doesn't check whether the completion is actually finished anywhere. I
don't
quite understand how this is safe .
[...]
> > > +static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
> > > + struct i2c_msg *msgs)
> > > +{
> >
> > Looks like almost an duplication as well...
>
> Considering the symmetric with them i2c_imx_dma_write.
> i2c_imx_dma_write and i2c_imx_pio_write have many differences. So I
> separate them. But i2c_imx_dma_read and i2c_imx_pio_read is the same at
> first part. I may should unify them. But it's will not symmetric with them
> i2c_imx_dma_write if unified them. So I don't know which will be better?
> Looking forward to hearing from you.
The dma_read() looks almost like dma_write(), so I'd also try merging them
together.
> > Besides, full-duplex DMA operation is missing, please explain why.
> >
> > THanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
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.
[...]
+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.
I'd say try it, the RX and TX callback look almost the same. So does the
i2c_imx_dma_rx() and i2c_imx_dma_tx() .
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 ?
+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.
This doesn't check whether the completion is actually finished anywhere. I
don't
quite understand how this is safe .
[...]
+static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
+ struct i2c_msg *msgs)
+{
Looks like almost an duplication as well...
Considering the symmetric with them i2c_imx_dma_write.
i2c_imx_dma_write and i2c_imx_pio_write have many differences. So I
separate them. But i2c_imx_dma_read and i2c_imx_pio_read is the same at
first part. I may should unify them. But it's will not symmetric with them
i2c_imx_dma_write if unified them. So I don't know which will be better?
Looking forward to hearing from you.
The dma_read() looks almost like dma_write(), so I'd also try merging them
together.
Besides, full-duplex DMA operation is missing, please explain why.
THanks!
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Friday, February 28, 2014 at 03:23:52 AM, Shawn Guo wrote:
On Fri, Feb 28, 2014 at 10:13:02AM +0800, Shawn Guo wrote:
On Thu, Feb 27, 2014 at 09:39:35PM +0100, Marek Vasut wrote:
@@ -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 ?
Using DMA or PIO is a decision that should be made by driver on its own,
not device tree.
Sorry. I misunderstood it. Yes, we can look at the DT property 'dmas'
to know if the controller has DMA capability.
You're right. Whether or not does the hardware support DMA transfers is a
property of the hardware, that's why it should be in DT.
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
Hi, Marek Vasut wrote: On Friday, February 28, 2014 at 06:19:18 AM, Yao Yuan wrote: [...] 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 ? SPI != I2C Lothar Waßmann -- ___ Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10 Geschäftsführer: Matthias Kaussen Handelsregistereintrag: Amtsgericht Aachen, HRB 4996 www.karo-electronics.de | i...@karo-electronics.de ___ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Friday, February 28, 2014 at 12:36:01 PM, Yao Yuan wrote:
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.
OK, got it now. Thanks!
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?
Sorry, nevermind. I was confused by this and some SPI patches review. Like
Lothar (thanks!) pointed out, using only half-duplex operation is OK.
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Friday, February 28, 2014 at 11:59:25 AM, Lothar Waßmann wrote: Hi, Marek Vasut wrote: On Friday, February 28, 2014 at 06:19:18 AM, Yao Yuan wrote: [...] 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 ? SPI != I2C You got me, sorry. Best regards, Marek Vasut -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel@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;
> >
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Fri, Feb 28, 2014 at 10:13:02AM +0800, Shawn Guo wrote:
> On Thu, Feb 27, 2014 at 09:39:35PM +0100, Marek Vasut wrote:
> > > @@ -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 ?
>
> Using DMA or PIO is a decision that should be made by driver on its own,
> not device tree.
Sorry. I misunderstood it. Yes, we can look at the DT property 'dmas'
to know if the controller has DMA capability.
Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Thu, Feb 27, 2014 at 09:39:35PM +0100, Marek Vasut wrote:
> > @@ -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 ?
Using DMA or PIO is a decision that should be made by driver on its own,
not device tree.
Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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_THRESHOLD16
So what's the unit here , potatoes or beers or what ? I suppose it's bytes ,
but
please 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 !
[...]
> +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 ?
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 .
> +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.
> +}
> /** 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_struct *i2c_imx,
> + struct i2c_msg *msgs)
> +{
> + int result, timeout=1000;
> + unsigned int temp = 0;
> +
> + reinit_completion(_imx->dma->cmd_complete);
> + result = i2c_imx_dma_tx(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 */
> + imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
> + result = wait_for_completion_interruptible_timeout(
> + _imx->dma->cmd_complete,
> + msecs_to_jiffies(1000));
Pull the magic constant of 1000 out and #define it as some I2C_IMX_DMA_TIMEOUT
please .
> + if (result == 0)
> + return -ETIMEDOUT;
> +
> + /* waiting for Transfer complete. */
> + while(timeout--) {
> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + if (temp & 0x80)
> + break;
> + udelay(10);
> + }
> +
> + 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 =
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Thu, Feb 27, 2014 at 08:55:45PM +0800, Shawn Guo wrote:
> On Thu, Feb 27, 2014 at 02:05:14PM +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.
> > And you should set ".has_dma_support" as true for dma support in
> > imx_i2c_hwdata struct.
> >
> > Signed-off-by: Yuan Yao
>
> Just added a few people who might be interested.
Still missed one - Marek. Sorry.
Shawn
>
> > ---
> > drivers/i2c/busses/i2c-imx.c | 358
> > +--
> > 1 file changed, 344 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > index db895fb..6ec392b 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,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
> > +
> > /* IMX I2C registers:
> > * the I2C register offset is different between SoCs,
> > * to provid support for all these chips, split the
> > @@ -88,6 +96,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
> > @@ -172,6 +181,17 @@ struct imx_i2c_hwdata {
> > unsignedndivs;
> > unsignedi2sr_clr_opcode;
> > unsignedi2cr_ien_opcode;
> > + boolhas_dma_support;
> > +};
> > +
> > +struct imx_i2c_dma {
> > + struct dma_chan *chan_tx;
> > + struct dma_chan *chan_rx;
> > + dma_addr_t buf_tx;
> > + dma_addr_t buf_rx;
> > + unsigned intlen_tx;
> > + unsigned intlen_rx;
> > + struct completion cmd_complete;
> > };
> >
> > struct imx_i2c_struct {
> > @@ -184,6 +204,9 @@ struct imx_i2c_struct {
> > int stopped;
> > unsigned intifdr; /* IMX_I2C_IFDR */
> > const struct imx_i2c_hwdata *hwdata;
> > +
> > + booluse_dma;
> > + struct imx_i2c_dma *dma;
> > };
> >
> > 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,
> >
> > };
> >
> > @@ -254,6 +280,155 @@ 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, u32
> > phy_addr)
> > +{
> > + struct imx_i2c_dma *dma = i2c_imx->dma;
> > + struct dma_slave_config dma_sconfig;
> > + int ret;
> > +
> > + dma->chan_tx = dma_request_slave_channel(_imx->adapter.dev, "tx");
> > + if (!dma->chan_tx) {
> > + dev_err(_imx->adapter.dev,
> > + "Dma tx channel request failed!\n");
> > + return -ENODEV;
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Thu, Feb 27, 2014 at 02:05:14PM +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.
> And you should set ".has_dma_support" as true for dma support in
> imx_i2c_hwdata struct.
>
> Signed-off-by: Yuan Yao
Just added a few people who might be interested.
Shawn
> ---
> drivers/i2c/busses/i2c-imx.c | 358
> +--
> 1 file changed, 344 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index db895fb..6ec392b 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,9 @@
> /* Default value */
> #define IMX_I2C_BIT_RATE 10 /* 100kHz */
>
> +/* enable DMA if transfer size is bigger than this threshold */
> +#define IMX_I2C_DMA_THRESHOLD16
> +
> /* IMX I2C registers:
> * the I2C register offset is different between SoCs,
> * to provid support for all these chips, split the
> @@ -88,6 +96,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
> @@ -172,6 +181,17 @@ struct imx_i2c_hwdata {
> unsignedndivs;
> unsignedi2sr_clr_opcode;
> unsignedi2cr_ien_opcode;
> + boolhas_dma_support;
> +};
> +
> +struct imx_i2c_dma {
> + struct dma_chan *chan_tx;
> + struct dma_chan *chan_rx;
> + dma_addr_t buf_tx;
> + dma_addr_t buf_rx;
> + unsigned intlen_tx;
> + unsigned intlen_rx;
> + struct completion cmd_complete;
> };
>
> struct imx_i2c_struct {
> @@ -184,6 +204,9 @@ struct imx_i2c_struct {
> int stopped;
> unsigned intifdr; /* IMX_I2C_IFDR */
> const struct imx_i2c_hwdata *hwdata;
> +
> + booluse_dma;
> + struct imx_i2c_dma *dma;
> };
>
> 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,
>
> };
>
> @@ -254,6 +280,155 @@ 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, u32 phy_addr)
> +{
> + struct imx_i2c_dma *dma = i2c_imx->dma;
> + struct dma_slave_config dma_sconfig;
> + int ret;
> +
> + dma->chan_tx = dma_request_slave_channel(_imx->adapter.dev, "tx");
> + if (!dma->chan_tx) {
> + dev_err(_imx->adapter.dev,
> + "Dma tx channel request failed!\n");
> + return -ENODEV;
> + }
> +
> + dma_sconfig.dst_addr = (dma_addr_t)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 =
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Thu, Feb 27, 2014 at 02:05:14PM +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.
And you should set .has_dma_support as true for dma support in
imx_i2c_hwdata struct.
Signed-off-by: Yuan Yao yao.y...@freescale.com
Just added a few people who might be interested.
Shawn
---
drivers/i2c/busses/i2c-imx.c | 358
+--
1 file changed, 344 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index db895fb..6ec392b 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -37,22 +37,27 @@
/** Includes
***
***/
-#include linux/init.h
-#include linux/kernel.h
-#include linux/module.h
+#include linux/clk.h
+#include linux/completion.h
+#include linux/delay.h
+#include linux/dma-mapping.h
+#include linux/dmaengine.h
+#include linux/dmapool.h
#include linux/errno.h
#include linux/err.h
#include linux/interrupt.h
-#include linux/delay.h
#include linux/i2c.h
+#include linux/init.h
#include linux/io.h
-#include linux/sched.h
-#include linux/platform_device.h
-#include linux/clk.h
-#include linux/slab.h
+#include linux/kernel.h
+#include linux/module.h
#include linux/of.h
#include linux/of_device.h
+#include linux/of_dma.h
#include linux/platform_data/i2c-imx.h
+#include linux/platform_device.h
+#include linux/sched.h
+#include linux/slab.h
/** Defines
***/
@@ -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_THRESHOLD16
+
/* IMX I2C registers:
* the I2C register offset is different between SoCs,
* to provid support for all these chips, split the
@@ -88,6 +96,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
@@ -172,6 +181,17 @@ struct imx_i2c_hwdata {
unsignedndivs;
unsignedi2sr_clr_opcode;
unsignedi2cr_ien_opcode;
+ boolhas_dma_support;
+};
+
+struct imx_i2c_dma {
+ struct dma_chan *chan_tx;
+ struct dma_chan *chan_rx;
+ dma_addr_t buf_tx;
+ dma_addr_t buf_rx;
+ unsigned intlen_tx;
+ unsigned intlen_rx;
+ struct completion cmd_complete;
};
struct imx_i2c_struct {
@@ -184,6 +204,9 @@ struct imx_i2c_struct {
int stopped;
unsigned intifdr; /* IMX_I2C_IFDR */
const struct imx_i2c_hwdata *hwdata;
+
+ booluse_dma;
+ struct imx_i2c_dma *dma;
};
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,
};
@@ -254,6 +280,155 @@ 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, u32 phy_addr)
+{
+ struct imx_i2c_dma *dma = i2c_imx-dma;
+ struct dma_slave_config dma_sconfig;
+ int ret;
+
+ dma-chan_tx = dma_request_slave_channel(i2c_imx-adapter.dev, tx);
+ if (!dma-chan_tx) {
+ dev_err(i2c_imx-adapter.dev,
+ Dma tx channel request failed!\n);
+ return -ENODEV;
+ }
+
+
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Thu, Feb 27, 2014 at 08:55:45PM +0800, Shawn Guo wrote:
On Thu, Feb 27, 2014 at 02:05:14PM +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.
And you should set .has_dma_support as true for dma support in
imx_i2c_hwdata struct.
Signed-off-by: Yuan Yao yao.y...@freescale.com
Just added a few people who might be interested.
Still missed one - Marek. Sorry.
Shawn
---
drivers/i2c/busses/i2c-imx.c | 358
+--
1 file changed, 344 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index db895fb..6ec392b 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -37,22 +37,27 @@
/** Includes
***
***/
-#include linux/init.h
-#include linux/kernel.h
-#include linux/module.h
+#include linux/clk.h
+#include linux/completion.h
+#include linux/delay.h
+#include linux/dma-mapping.h
+#include linux/dmaengine.h
+#include linux/dmapool.h
#include linux/errno.h
#include linux/err.h
#include linux/interrupt.h
-#include linux/delay.h
#include linux/i2c.h
+#include linux/init.h
#include linux/io.h
-#include linux/sched.h
-#include linux/platform_device.h
-#include linux/clk.h
-#include linux/slab.h
+#include linux/kernel.h
+#include linux/module.h
#include linux/of.h
#include linux/of_device.h
+#include linux/of_dma.h
#include linux/platform_data/i2c-imx.h
+#include linux/platform_device.h
+#include linux/sched.h
+#include linux/slab.h
/** Defines
***/
@@ -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
+
/* IMX I2C registers:
* the I2C register offset is different between SoCs,
* to provid support for all these chips, split the
@@ -88,6 +96,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
@@ -172,6 +181,17 @@ struct imx_i2c_hwdata {
unsignedndivs;
unsignedi2sr_clr_opcode;
unsignedi2cr_ien_opcode;
+ boolhas_dma_support;
+};
+
+struct imx_i2c_dma {
+ struct dma_chan *chan_tx;
+ struct dma_chan *chan_rx;
+ dma_addr_t buf_tx;
+ dma_addr_t buf_rx;
+ unsigned intlen_tx;
+ unsigned intlen_rx;
+ struct completion cmd_complete;
};
struct imx_i2c_struct {
@@ -184,6 +204,9 @@ struct imx_i2c_struct {
int stopped;
unsigned intifdr; /* IMX_I2C_IFDR */
const struct imx_i2c_hwdata *hwdata;
+
+ booluse_dma;
+ struct imx_i2c_dma *dma;
};
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,
};
@@ -254,6 +280,155 @@ 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, u32
phy_addr)
+{
+ struct imx_i2c_dma *dma = i2c_imx-dma;
+ struct dma_slave_config dma_sconfig;
+ int ret;
+
+ dma-chan_tx = dma_request_slave_channel(i2c_imx-adapter.dev, tx);
+ if
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_THRESHOLD16
So what's the unit here , potatoes or beers or what ? I suppose it's bytes ,
but
please 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 !
[...]
+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 ?
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 .
+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.
+}
/** 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_struct *i2c_imx,
+ struct i2c_msg *msgs)
+{
+ int result, timeout=1000;
+ unsigned int temp = 0;
+
+ reinit_completion(i2c_imx-dma-cmd_complete);
+ result = i2c_imx_dma_tx(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 */
+ 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(1000));
Pull the magic constant of 1000 out and #define it as some I2C_IMX_DMA_TIMEOUT
please .
+ if (result == 0)
+ return -ETIMEDOUT;
+
+ /* waiting for Transfer complete. */
+ while(timeout--) {
+ temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+ if (temp 0x80)
+ break;
+ udelay(10);
+ }
+
+ 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
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Thu, Feb 27, 2014 at 09:39:35PM +0100, Marek Vasut wrote:
@@ -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 ?
Using DMA or PIO is a decision that should be made by driver on its own,
not device tree.
Shawn
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] i2c: add DMA support for freescale i2c driver
On Fri, Feb 28, 2014 at 10:13:02AM +0800, Shawn Guo wrote:
On Thu, Feb 27, 2014 at 09:39:35PM +0100, Marek Vasut wrote:
@@ -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 ?
Using DMA or PIO is a decision that should be made by driver on its own,
not device tree.
Sorry. I misunderstood it. Yes, we can look at the DT property 'dmas'
to know if the controller has DMA capability.
Shawn
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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-kernel@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_struct *i2c_imx,
+ struct i2c_msg *msgs)
+{
+ int result, timeout=1000;
+ unsigned int temp = 0;
+
+ reinit_completion(i2c_imx-dma
