| Message ID | 1394002352-8380-2-git-send-email-yao.yuan@freescale.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > Add dma support for i2c. This function depend on DMA driver. > You can turn on it by write both the dmas and dma-name properties in dts > node. > > Signed-off-by: Yuan Yao <yao.yuan@freescale.com> > --- [...] > struct imx_i2c_struct { > struct i2c_adapter adapter; > struct clk *clk; > @@ -184,6 +205,9 @@ struct imx_i2c_struct { > int stopped; > unsigned int ifdr; /* IMX_I2C_IFDR */ > const struct imx_i2c_hwdata *hwdata; > + > + bool use_dma; > + struct imx_i2c_dma *dma; I'd suggest you check how big this structure would be if you swapped the 'bool...' and 'struct imx_i2c_dma...' entries and compare it to the original. I think due to natural alignment, having the bool first would create more slop [1]. > }; > > static const struct imx_i2c_hwdata imx1_i2c_hwdata = { > @@ -254,9 +278,120 @@ 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 > ************************************************ > +************************************************************************* > *****/ I can't say I am too fond of this style of comment, especially since it violates coding style (it's not a kernel-doc style comment either). Just use /* * Functions for DMA */ if you really want to make the comment big. > +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"); Why don't you define a new variable for this dev pointer ? struct device *dev = &i2c_imx->adapter.dev; Then use this 'dev' all around isntead of spelling the entire path everywhere. That'd trim the amount of code down too, not to mention you'd be able to avoid multi-line function args more often. > + if (!dma->chan_tx) { > + dev_err(&i2c_imx->adapter.dev, > + "Dma tx channel request failed!\n"); > + return -ENODEV; > + } > + > + dma_sconfig.dst_addr = (dma_addr_t)phy_addr + Why don't you pass the 'phy_addr' as 'dma_addr_t' into this function in the first place ? > + (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_err(&i2c_imx->adapter.dev, > + "Dma slave config failed, err = %d\n", ret); > + dma_release_channel(dma->chan_tx); Use 'goto fail' or such here to implement a failpath. Then handle the release of tx channel in the failpath. You do duplicate this call below. > + return ret; > + } > + > + dma->chan_rx = dma_request_slave_channel(&i2c_imx->adapter.dev, "rx"); > + if (!dma->chan_rx) { > + dev_err(&i2c_imx->adapter.dev, > + "Dma rx channel request failed!\n"); > + return -ENODEV; > + } > + > + dma_sconfig.src_addr = (dma_addr_t)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_err(&i2c_imx->adapter.dev, > + "Dma slave config failed, err = %d\n", ret); > + dma_release_channel(dma->chan_rx); > + return ret; > + } > + > + init_completion(&dma->cmd_complete); > + > + return 0; > +} > + > +static void i2c_imx_dma_callback(void *arg) > +{ > + struct imx_i2c_struct *i2c_imx = (struct imx_i2c_struct *)arg; > + struct imx_i2c_dma *dma = i2c_imx->dma; > + > + dma_unmap_single(dma->chan_using->device->dev, dma->dma_buf, > + dma->dma_len, dma->dma_data_dir); > + complete(&dma->cmd_complete); > +} > + > +static int i2c_imx_dma_xfer(struct imx_i2c_struct *i2c_imx, > + struct i2c_msg *msgs) > +{ > + struct imx_i2c_dma *dma = i2c_imx->dma; > + struct dma_async_tx_descriptor *txdesc; > + > + dma->dma_buf = dma_map_single(dma->chan_using->device->dev, msgs->buf, > + dma->dma_len, dma->dma_data_dir); > + if (dma_mapping_error(dma->chan_using->device->dev, dma->dma_buf)) { > + dev_err(&i2c_imx->adapter.dev, "dma_map_single failed\n"); > + return -EINVAL; > + } > + > + txdesc = dmaengine_prep_slave_single(dma->chan_using, dma->dma_buf, > + dma->dma_len, dma->dma_transfer_dir, > + DMA_PREP_INTERRUPT | DMA_CTRL_ACK); > + if (!txdesc) { > + dev_err(&i2c_imx->adapter.dev, > + "Not able to get desc for dma xfer\n"); > + return -EINVAL; If we fail here, we will still have dma_buf mapped , right ? > + } > + > + txdesc->callback = i2c_imx_dma_callback; > + txdesc->callback_param = i2c_imx; > + dmaengine_submit(txdesc); > + dma_async_issue_pending(dma->chan_using); > + > + return 0; > +} > + > +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->dma_buf = 0; > + dma->dma_len = 0; > + > + dma_chan = dma->chan_tx; > + dma_release_channel(dma_chan); > + dma->chan_tx = NULL; Here you can just drop the special variable. > + dma_chan = dma->chan_rx; > + dma_release_channel(dma_chan); > + dma->chan_rx = NULL; > + > + dma->chan_using = NULL; > +} > + > /** Functions for IMX I2C adapter driver > *************************************** > ************************************************************************** > *****/ - > static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy) > { > unsigned long orig_jiffies = jiffies; > @@ -427,46 +562,98 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id) > > static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg > *msgs) { > - int i, result; > + int i, result, timeout = IMX_I2C_DMA_TIMEOUT; > + unsigned int temp = 0; > + struct imx_i2c_dma *dma = i2c_imx->dma; > > dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n", > __func__, msgs->addr << 1); > > - /* write slave address */ > - imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR); > - result = i2c_imx_trx_complete(i2c_imx); > - if (result) > - return result; > - result = i2c_imx_acked(i2c_imx); > - if (result) > - return result; > - dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__); > + if (i2c_imx->use_dma && msgs->len >= IMX_I2C_DMA_THRESHOLD) { > + 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; > > - /* write data */ > - for (i = 0; i < msgs->len; i++) { > - dev_dbg(&i2c_imx->adapter.dev, > - "<%s> write byte: B%d=0x%X\n", > - __func__, i, msgs->buf[i]); > - imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR); > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > + temp |= I2CR_DMAEN; > + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > + > + /* write slave address */ > + imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR); > + result = wait_for_completion_interruptible_timeout( > + &i2c_imx->dma->cmd_complete, > + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT)); > + if (result == 0) > + return -ETIMEDOUT; You need to handle result < 0 as well, since that happens if this was interrupted. > + > + /* waiting for Transfer complete. */ > + while (timeout--) { > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR); > + if (temp & 0x80) What's this magic value of 0x80 here ? > + 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 (result) > return result; > + } else { > + /* write slave address */ > + imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR); > + result = i2c_imx_trx_complete(i2c_imx); > + if (result) > + return result; > + > + result = i2c_imx_acked(i2c_imx); > + if (result) > + return result; > + > + dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__); Define and use 'dev' variable here, as mentioned above. > + /* write data */ > + for (i = 0; i < msgs->len; i++) { > + dev_dbg(&i2c_imx->adapter.dev, > + "<%s> write byte: B%d=0x%X\n", > + __func__, i, msgs->buf[i]); > + imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR); > + result = i2c_imx_trx_complete(i2c_imx); > + if (result) > + return result; > + result = i2c_imx_acked(i2c_imx); > + if (result) > + return result; > + } > } > return 0; > } > > static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg > *msgs) { > - int i, result; > + int i, result, timeout = IMX_I2C_DMA_TIMEOUT; > unsigned int temp; > + struct imx_i2c_dma *dma = i2c_imx->dma; > > dev_dbg(&i2c_imx->adapter.dev, > "<%s> write slave address: addr=0x%x\n", > __func__, (msgs->addr << 1) | 0x01); > > + > /* write slave address */ > imx_i2c_write_reg((msgs->addr << 1) | 0x01, i2c_imx, IMX_I2C_I2DR); > result = i2c_imx_trx_complete(i2c_imx); > @@ -488,33 +675,71 @@ static int i2c_imx_read(struct imx_i2c_struct > *i2c_imx, struct i2c_msg *msgs) > > dev_dbg(&i2c_imx->adapter.dev, "<%s> read data\n", __func__); > > - /* read data */ > - for (i = 0; i < msgs->len; i++) { > - result = i2c_imx_trx_complete(i2c_imx); > + if (i2c_imx->use_dma && msgs->len >= IMX_I2C_DMA_THRESHOLD) { > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > + temp |= I2CR_DMAEN; > + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > + > + reinit_completion(&i2c_imx->dma->cmd_complete); > + dma->chan_using = dma->chan_rx; > + dma->dma_transfer_dir = DMA_DEV_TO_MEM; > + dma->dma_data_dir = DMA_FROM_DEVICE; > + dma->dma_len = msgs->len - 2; > + result = i2c_imx_dma_xfer(i2c_imx, msgs); > if (result) > return result; > - if (i == (msgs->len - 1)) { > - /* It must generate STOP before read I2DR to prevent > - controller from generating another clock cycle */ > - dev_dbg(&i2c_imx->adapter.dev, > - "<%s> clear MSTA\n", __func__); > - temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > - temp &= ~(I2CR_MSTA | I2CR_MTX); > - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > - i2c_imx_bus_busy(i2c_imx, 0); > - i2c_imx->stopped = 1; > - } else if (i == (msgs->len - 2)) { > + > + result = wait_for_completion_interruptible_timeout( > + &i2c_imx->dma->cmd_complete, > + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT)); > + if (result == 0) > + return -ETIMEDOUT; > + > + /* waiting for Transfer complete. */ > + while (timeout--) { > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR); > + if (temp & 0x80) Magic value here, please fix. > + 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); > + } else { > + /* read data */ > + for (i = 0; i < msgs->len - 2; i++) { > + result = i2c_imx_trx_complete(i2c_imx); > + if (result) > + return result; > + msgs->buf[i] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); > dev_dbg(&i2c_imx->adapter.dev, > - "<%s> set TXAK\n", __func__); > - temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > - temp |= I2CR_TXAK; > - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > + "<%s> read byte: B%d=0x%X\n", > + __func__, i, msgs->buf[i]); > } > - msgs->buf[i] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); > - dev_dbg(&i2c_imx->adapter.dev, > - "<%s> read byte: B%d=0x%X\n", > - __func__, i, msgs->buf[i]); > + result = i2c_imx_trx_complete(i2c_imx); > } > + > + /* read n-1 byte data */ > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > + temp |= I2CR_TXAK; > + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > + > + msgs->buf[msgs->len-2] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); > + /* read n byte data */ > + result = i2c_imx_trx_complete(i2c_imx); > + if (result) > + return result; > + > + /* It must generate STOP before read I2DR to prevent > + controller from generating another clock cycle */ Again, comment style is wrong. For multiline comments, use this: /* * foo bar * baz quux */ > + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > + temp &= ~(I2CR_MSTA | I2CR_MTX); > + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); > + i2c_imx_bus_busy(i2c_imx, 0); > + i2c_imx->stopped = 1; > + msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); > + > return 0; > } > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct platform_device *pdev) > void __iomem *base; > int irq, ret; > u32 bitrate; > + u32 phy_addr; > > dev_dbg(&pdev->dev, "<%s>\n", __func__); > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct platform_device *pdev) > } > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + phy_addr = res->start; Uh ... Shawn, I really think I am lost here. Don't you need to map this memory before you can use it for DMA ? The DMA mapping function should give you the physical address and is the right way to go about this instead of pulling the address from here, no ? I might be wrong here, I am rather uncertain, so please help me out. Thanks! > base = devm_ioremap_resource(&pdev->dev, res); > if (IS_ERR(base)) > return PTR_ERR(base); > @@ -696,6 +923,21 @@ static int i2c_imx_probe(struct platform_device *pdev) > i2c_imx->adapter.name); > dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n"); > > + /* Init DMA config if support*/ > + i2c_imx->dma = devm_kzalloc(&pdev->dev, sizeof(struct imx_i2c_dma), > + GFP_KERNEL); > + if (!i2c_imx->dma) { > + dev_info(&pdev->dev, > + "can't allocate dma struct faild use dma.\n"); > + i2c_imx->use_dma = false; > + } else if (i2c_imx_dma_request(i2c_imx, phy_addr)) { > + dev_info(&pdev->dev, > + "can't request dma chan, faild use dma.\n"); > + i2c_imx->use_dma = false; > + } else { > + i2c_imx->use_dma = true; > + } > + > return 0; /* Return OK */ > } > > @@ -707,6 +949,9 @@ static int i2c_imx_remove(struct platform_device *pdev) > dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n"); > i2c_del_adapter(&i2c_imx->adapter); > > + if (i2c_imx->use_dma) > + i2c_imx_dma_free(i2c_imx); > + > /* setup chip registers to defaults */ > imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR); > imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR); [1] http://www.catb.org/esr/structure-packing/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > Add dma support for i2c. This function depend on DMA driver. > > You can turn on it by write both the dmas and dma-name properties in > > dts node. > > > > Signed-off-by: Yuan Yao <yao.yuan@freescale.com> > > --- > > [...] > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct platform_device > *pdev) > > void __iomem *base; > > int irq, ret; > > u32 bitrate; > > + u32 phy_addr; > > > > dev_dbg(&pdev->dev, "<%s>\n", __func__); > > > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct platform_device > *pdev) > > } > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + phy_addr = res->start; > > Uh ... Shawn, I really think I am lost here. Don't you need to map this > memory before you can use it for DMA ? The DMA mapping function should > give you the physical address and is the right way to go about this > instead of pulling the address from here, no ? > > I might be wrong here, I am rather uncertain, so please help me out. > Thanks! Hi, Marek, Thanks for your suggestion. Here you can review the code in include/linux/ioport.h The resource->start describes the entity on the CPU bus as a starting physical address. So I thinks it can used for dma directly.
On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote: > On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > > Add dma support for i2c. This function depend on DMA driver. > > > You can turn on it by write both the dmas and dma-name properties in > > > dts node. > > > > > > Signed-off-by: Yuan Yao <yao.yuan@freescale.com> > > > --- > > > > [...] > > > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct platform_device > > > > *pdev) > > > > > void __iomem *base; > > > int irq, ret; > > > u32 bitrate; > > > > > > + u32 phy_addr; > > > > > > dev_dbg(&pdev->dev, "<%s>\n", __func__); > > > > > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct platform_device > > > > *pdev) > > > > > } > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > > + phy_addr = res->start; > > > > Uh ... Shawn, I really think I am lost here. Don't you need to map this > > memory before you can use it for DMA ? The DMA mapping function should > > give you the physical address and is the right way to go about this > > instead of pulling the address from here, no ? > > > > I might be wrong here, I am rather uncertain, so please help me out. > > Thanks! > > Hi, Marek, Thanks for your suggestion. > Here you can review the code in include/linux/ioport.h > The resource->start describes the entity on the CPU bus as a starting > physical address. So I thinks it can used for dma directly. This doesn't feel right for some reason. If this is a register area, you should ioremap() it. If it's a memory area you do DMA to/from, you need to make sure you correctly flush/invalidate caches and properly handle the effects the write buffer might have. But I have a feeling you actually do DMA to/from register space here ? Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
T24gVGh1LCBNYXJjaCAwNiwgMjAxNCBhdCAxMjo0NDoxNCBQTSwgTWFyZWsgVmFzdXQgd3JvdGU6 DQo+IE9uIFRodXJzZGF5LCBNYXJjaCAwNiwgMjAxNCBhdCAwNTozNjoxNCBBTSwgWWFvIFl1YW4g d3JvdGU6DQo+ID4gT24gVGh1LCBNYXJjaCAwNiwgMjAxNCBhdCAxMToyMzo1MCBBTSwgTWFyZWsg VmFzdXQgd3JvdGU6DQo+ID4gPiBPbiBXZWRuZXNkYXksIE1hcmNoIDA1LCAyMDE0IGF0IDA3OjUy OjMxIEFNLCBZdWFuIFlhbyB3cm90ZToNCj4gPiA+ID4gQWRkIGRtYSBzdXBwb3J0IGZvciBpMmMu IFRoaXMgZnVuY3Rpb24gZGVwZW5kIG9uIERNQSBkcml2ZXIuDQo+ID4gPiA+IFlvdSBjYW4gdHVy biBvbiBpdCBieSB3cml0ZSBib3RoIHRoZSBkbWFzIGFuZCBkbWEtbmFtZSBwcm9wZXJ0aWVzDQo+ ID4gPiA+IGluIGR0cyBub2RlLg0KPiA+ID4gPg0KPiA+ID4gPiBTaWduZWQtb2ZmLWJ5OiBZdWFu IFlhbyA8eWFvLnl1YW5AZnJlZXNjYWxlLmNvbT4NCj4gPiA+ID4gLS0tDQo+ID4gPg0KPiA+ID4g Wy4uLl0NCj4gPiA+DQo+ID4gPiA+IEBAIC02MDEsNiArODI2LDcgQEAgc3RhdGljIGludCBpMmNf aW14X3Byb2JlKHN0cnVjdA0KPiA+ID4gPiBwbGF0Zm9ybV9kZXZpY2UNCj4gPiA+DQo+ID4gPiAq cGRldikNCj4gPiA+DQo+ID4gPiA+ICAJdm9pZCBfX2lvbWVtICpiYXNlOw0KPiA+ID4gPiAgCWlu dCBpcnEsIHJldDsNCj4gPiA+ID4gIAl1MzIgYml0cmF0ZTsNCj4gPiA+ID4NCj4gPiA+ID4gKwl1 MzIgcGh5X2FkZHI7DQo+ID4gPiA+DQo+ID4gPiA+ICAJZGV2X2RiZygmcGRldi0+ZGV2LCAiPCVz PlxuIiwgX19mdW5jX18pOw0KPiA+ID4gPg0KPiA+ID4gPiBAQCAtNjExLDYgKzgzNyw3IEBAIHN0 YXRpYyBpbnQgaTJjX2lteF9wcm9iZShzdHJ1Y3QNCj4gPiA+ID4gcGxhdGZvcm1fZGV2aWNlDQo+ ID4gPg0KPiA+ID4gKnBkZXYpDQo+ID4gPg0KPiA+ID4gPiAgCX0NCj4gPiA+ID4NCj4gPiA+ID4g IAlyZXMgPSBwbGF0Zm9ybV9nZXRfcmVzb3VyY2UocGRldiwgSU9SRVNPVVJDRV9NRU0sIDApOw0K PiA+ID4gPg0KPiA+ID4gPiArCXBoeV9hZGRyID0gcmVzLT5zdGFydDsNCj4gPiA+DQo+ID4gPiBV aCAuLi4gU2hhd24sIEkgcmVhbGx5IHRoaW5rIEkgYW0gbG9zdCBoZXJlLiBEb24ndCB5b3UgbmVl ZCB0byBtYXANCj4gPiA+IHRoaXMgbWVtb3J5IGJlZm9yZSB5b3UgY2FuIHVzZSBpdCBmb3IgRE1B ID8gVGhlIERNQSBtYXBwaW5nIGZ1bmN0aW9uDQo+ID4gPiBzaG91bGQgZ2l2ZSB5b3UgdGhlIHBo eXNpY2FsIGFkZHJlc3MgYW5kIGlzIHRoZSByaWdodCB3YXkgdG8gZ28NCj4gPiA+IGFib3V0IHRo aXMgaW5zdGVhZCBvZiBwdWxsaW5nIHRoZSBhZGRyZXNzIGZyb20gaGVyZSwgbm8gPw0KPiA+ID4N Cj4gPiA+IEkgbWlnaHQgYmUgd3JvbmcgaGVyZSwgSSBhbSByYXRoZXIgdW5jZXJ0YWluLCBzbyBw bGVhc2UgaGVscCBtZSBvdXQuDQo+ID4gPiBUaGFua3MhDQo+ID4NCj4gPiBIaSwgTWFyZWssIFRo YW5rcyBmb3IgeW91ciBzdWdnZXN0aW9uLg0KPiA+IEhlcmUgeW91IGNhbiByZXZpZXcgdGhlIGNv ZGUgaW4gaW5jbHVkZS9saW51eC9pb3BvcnQuaCBUaGUNCj4gPiByZXNvdXJjZS0+c3RhcnQgZGVz Y3JpYmVzIHRoZSBlbnRpdHkgb24gdGhlIENQVSBidXMgYXMgYSBzdGFydGluZw0KPiA+IHBoeXNp Y2FsIGFkZHJlc3MuIFNvIEkgdGhpbmtzIGl0IGNhbiB1c2VkIGZvciBkbWEgZGlyZWN0bHkuDQo+ IA0KPiBUaGlzIGRvZXNuJ3QgZmVlbCByaWdodCBmb3Igc29tZSByZWFzb24uIElmIHRoaXMgaXMg YSByZWdpc3RlciBhcmVhLCB5b3UNCj4gc2hvdWxkDQo+IGlvcmVtYXAoKSBpdC4gSWYgaXQncyBh IG1lbW9yeSBhcmVhIHlvdSBkbyBETUEgdG8vZnJvbSwgeW91IG5lZWQgdG8gbWFrZQ0KPiBzdXJl IHlvdSBjb3JyZWN0bHkgZmx1c2gvaW52YWxpZGF0ZSBjYWNoZXMgYW5kIHByb3Blcmx5IGhhbmRs ZSB0aGUNCj4gZWZmZWN0cyB0aGUgd3JpdGUgYnVmZmVyIG1pZ2h0IGhhdmUuIEJ1dCBJIGhhdmUg YSBmZWVsaW5nIHlvdSBhY3R1YWxseSBkbw0KPiBETUEgdG8vZnJvbSByZWdpc3RlciBzcGFjZSBo ZXJlID8NCj4gDQpZZXMsIEl0J3MgYSByZWdpc3RlciBhcmVhLiBCdXQgSSBkb24ndCBrbm93IHdo eSBJIHNob3VsZCBpb3JlbWFwKCkgaXQ/IEl0J3MgYSBidXMgYWRkcmVzcyBhbmQgRE1BIGNhbiB1 c2UgaXQgZGlyZWN0bHkuDQpJcyB0aGVyZSBzb21lIHByb2JsZW0gZm9yIG15IHVuZGVyc3RhbmRp bmcgPw0K -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, March 06, 2014 at 06:02:03 AM, Yao Yuan wrote: > On Thu, March 06, 2014 at 12:44:14 PM, Marek Vasut wrote: > > On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote: > > > On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > > > > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > > > > Add dma support for i2c. This function depend on DMA driver. > > > > > You can turn on it by write both the dmas and dma-name properties > > > > > in dts node. > > > > > > > > > > Signed-off-by: Yuan Yao <yao.yuan@freescale.com> > > > > > --- > > > > > > > > [...] > > > > > > > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct > > > > > platform_device > > > > > > > > *pdev) > > > > > > > > > void __iomem *base; > > > > > int irq, ret; > > > > > u32 bitrate; > > > > > > > > > > + u32 phy_addr; > > > > > > > > > > dev_dbg(&pdev->dev, "<%s>\n", __func__); > > > > > > > > > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct > > > > > platform_device > > > > > > > > *pdev) > > > > > > > > > } > > > > > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > > > > > > + phy_addr = res->start; > > > > > > > > Uh ... Shawn, I really think I am lost here. Don't you need to map > > > > this memory before you can use it for DMA ? The DMA mapping function > > > > should give you the physical address and is the right way to go > > > > about this instead of pulling the address from here, no ? > > > > > > > > I might be wrong here, I am rather uncertain, so please help me out. > > > > Thanks! > > > > > > Hi, Marek, Thanks for your suggestion. > > > Here you can review the code in include/linux/ioport.h The > > > resource->start describes the entity on the CPU bus as a starting > > > physical address. So I thinks it can used for dma directly. > > > > This doesn't feel right for some reason. If this is a register area, you > > should > > ioremap() it. If it's a memory area you do DMA to/from, you need to make > > sure you correctly flush/invalidate caches and properly handle the > > effects the write buffer might have. But I have a feeling you actually do > > DMA to/from register space here ? > > Yes, It's a register area. But I don't know why I should ioremap() it? It's > a bus address and DMA can use it directly. Is there some problem for my > understanding ? I am not too sure here, thus I am poking someone who can clearly answer this. Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, March 06, 2014 at 09:46:03 PM, Marek Vasut wrote: >On Thursday, March 06, 2014 at 06:02:03 AM, Yao Yuan wrote: > > On Thu, March 06, 2014 at 12:44:14 PM, Marek Vasut wrote: > > > On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote: > > > > On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > > > > > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > > > > > Add dma support for i2c. This function depend on DMA driver. > > > > > > You can turn on it by write both the dmas and dma-name properties > > > > > > in dts node. > > > > > > > > > > > > Signed-off-by: Yuan Yao <yao.yuan@freescale.com> > > > > > > --- > > > > > > > > > > [...] > > > > > > > > > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct > > > > > > platform_device > > > > > > > > > > *pdev) > > > > > > > > > > > void __iomem *base; > > > > > > int irq, ret; > > > > > > u32 bitrate; > > > > > > > > > > > >+ u32 phy_addr; > > > > > > > > > > > > dev_dbg(&pdev->dev, "<%s>\n", __func__); > > > > > > > > > > > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct > > > > > > platform_device > > > > > > > > > > *pdev) > > > > > > > > > > > } > > > > > > > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > > > > > > > > + phy_addr = res->start; > > > > > > > > > > Uh ... Shawn, I really think I am lost here. Don't you need to map > > > > > this memory before you can use it for DMA ? The DMA mapping function > > > > > should give you the physical address and is the right way to go > > > > > about this instead of pulling the address from here, no ? > > > > > > > > > > I might be wrong here, I am rather uncertain, so please help me out. > > > > > Thanks! > > > > > > > > Hi, Marek, Thanks for your suggestion. > > > > Here you can review the code in include/linux/ioport.h The > > > > resource->start describes the entity on the CPU bus as a starting > > > > physical address. So I thinks it can used for dma directly. > > > > > > This doesn't feel right for some reason. If this is a register area, you > > > should > > > ioremap() it. If it's a memory area you do DMA to/from, you need to make > > > sure you correctly flush/invalidate caches and properly handle the > > > effects the write buffer might have. But I have a feeling you actually do > > > DMA to/from register space here ? > > > > Yes, It's a register area. But I don't know why I should ioremap() it? It's > > a bus address and DMA can use it directly. Is there some problem for my > > understanding ? > > I am not too sure here, thus I am poking someone who can clearly answer this. OK, and thank you for pointing out many ugly code in my patch. It's very useful for me.
On Thu, Mar 06, 2014 at 12:57:42PM +0100, Marek Vasut wrote: > On Thursday, March 06, 2014 at 06:02:03 AM, Yao Yuan wrote: > > On Thu, March 06, 2014 at 12:44:14 PM, Marek Vasut wrote: > > > On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote: > > > > On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > > > > > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > > > > > Add dma support for i2c. This function depend on DMA driver. > > > > > > You can turn on it by write both the dmas and dma-name properties > > > > > > in dts node. > > > > > > > > > > > > Signed-off-by: Yuan Yao <yao.yuan@freescale.com> > > > > > > --- > > > > > > > > > > [...] > > > > > > > > > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct > > > > > > platform_device > > > > > > > > > > *pdev) > > > > > > > > > > > void __iomem *base; > > > > > > int irq, ret; > > > > > > u32 bitrate; > > > > > > > > > > > > + u32 phy_addr; > > > > > > > > > > > > dev_dbg(&pdev->dev, "<%s>\n", __func__); > > > > > > > > > > > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct > > > > > > platform_device > > > > > > > > > > *pdev) > > > > > > > > > > > } > > > > > > > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > > > > > > > > + phy_addr = res->start; > > > > > > > > > > Uh ... Shawn, I really think I am lost here. Don't you need to map > > > > > this memory before you can use it for DMA ? The DMA mapping function > > > > > should give you the physical address and is the right way to go > > > > > about this instead of pulling the address from here, no ? > > > > > > > > > > I might be wrong here, I am rather uncertain, so please help me out. > > > > > Thanks! > > > > > > > > Hi, Marek, Thanks for your suggestion. > > > > Here you can review the code in include/linux/ioport.h The > > > > resource->start describes the entity on the CPU bus as a starting > > > > physical address. So I thinks it can used for dma directly. > > > > > > This doesn't feel right for some reason. If this is a register area, you > > > should > > > ioremap() it. If it's a memory area you do DMA to/from, you need to make > > > sure you correctly flush/invalidate caches and properly handle the > > > effects the write buffer might have. But I have a feeling you actually do > > > DMA to/from register space here ? > > > > Yes, It's a register area. But I don't know why I should ioremap() it? It's > > a bus address and DMA can use it directly. Is there some problem for my > > understanding ? > > I am not too sure here, thus I am poking someone who can clearly answer this. There is already a devm_ioremap_resource() call in the existing code for CPU to access registers in virtual address. And my understanding on Yuan's patch is that he needs the physical address of I2C DATA register for DMA from/to the controller. Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
T24gVGh1LCBNYXIgMTAsIDIwMTQgYXQgMTA6MDE6NDIgQU0sIE1hcmVrIFZhc3V0IHdyb3RlOg0K PiBPbiBUaHUsIE1hciAwNiwgMjAxNCBhdCAxMjo1Nzo0MlBNICswMTAwLCBNYXJlayBWYXN1dCB3 cm90ZToNCj4gPiBPbiBUaHVyc2RheSwgTWFyY2ggMDYsIDIwMTQgYXQgMDY6MDI6MDMgQU0sIFlh byBZdWFuIHdyb3RlOg0KPiA+ID4gT24gVGh1LCBNYXJjaCAwNiwgMjAxNCBhdCAxMjo0NDoxNCBQ TSwgTWFyZWsgVmFzdXQgd3JvdGU6DQo+ID4gPiA+IE9uIFRodXJzZGF5LCBNYXJjaCAwNiwgMjAx NCBhdCAwNTozNjoxNCBBTSwgWWFvIFl1YW4gd3JvdGU6DQo+ID4gPiA+ID4gT24gVGh1LCBNYXJj aCAwNiwgMjAxNCBhdCAxMToyMzo1MCBBTSwgTWFyZWsgVmFzdXQgd3JvdGU6DQo+ID4gPiA+ID4g PiBPbiBXZWRuZXNkYXksIE1hcmNoIDA1LCAyMDE0IGF0IDA3OjUyOjMxIEFNLCBZdWFuIFlhbyB3 cm90ZToNCj4gPiA+ID4gPiA+ID4gQWRkIGRtYSBzdXBwb3J0IGZvciBpMmMuIFRoaXMgZnVuY3Rp b24gZGVwZW5kIG9uIERNQSBkcml2ZXIuDQo+ID4gPiA+ID4gPiA+IFlvdSBjYW4gdHVybiBvbiBp dCBieSB3cml0ZSBib3RoIHRoZSBkbWFzIGFuZCBkbWEtbmFtZQ0KPiA+ID4gPiA+ID4gPiBwcm9w ZXJ0aWVzIGluIGR0cyBub2RlLg0KPiA+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gPiBTaWduZWQt b2ZmLWJ5OiBZdWFuIFlhbyA8eWFvLnl1YW5AZnJlZXNjYWxlLmNvbT4NCj4gPiA+ID4gPiA+ID4g LS0tDQo+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gWy4uLl0NCj4gPiA+ID4gPiA+DQo+ID4gPiA+ ID4gPiA+IEBAIC02MDEsNiArODI2LDcgQEAgc3RhdGljIGludCBpMmNfaW14X3Byb2JlKHN0cnVj dA0KPiA+ID4gPiA+ID4gPiBwbGF0Zm9ybV9kZXZpY2UNCj4gPiA+ID4gPiA+DQo+ID4gPiA+ID4g PiAqcGRldikNCj4gPiA+ID4gPiA+DQo+ID4gPiA+ID4gPiA+ICAJdm9pZCBfX2lvbWVtICpiYXNl Ow0KPiA+ID4gPiA+ID4gPiAgCWludCBpcnEsIHJldDsNCj4gPiA+ID4gPiA+ID4gIAl1MzIgYml0 cmF0ZTsNCj4gPiA+ID4gPiA+ID4NCj4gPiA+ID4gPiA+ID4gKwl1MzIgcGh5X2FkZHI7DQo+ID4g PiA+ID4gPiA+DQo+ID4gPiA+ID4gPiA+ICAJZGV2X2RiZygmcGRldi0+ZGV2LCAiPCVzPlxuIiwg X19mdW5jX18pOw0KPiA+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gPiBAQCAtNjExLDYgKzgzNyw3 IEBAIHN0YXRpYyBpbnQgaTJjX2lteF9wcm9iZShzdHJ1Y3QNCj4gPiA+ID4gPiA+ID4gcGxhdGZv cm1fZGV2aWNlDQo+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gKnBkZXYpDQo+ID4gPiA+ID4gPg0K PiA+ID4gPiA+ID4gPiAgCX0NCj4gPiA+ID4gPiA+ID4NCj4gPiA+ID4gPiA+ID4gIAlyZXMgPSBw bGF0Zm9ybV9nZXRfcmVzb3VyY2UocGRldiwgSU9SRVNPVVJDRV9NRU0sIDApOw0KPiA+ID4gPiA+ ID4gPg0KPiA+ID4gPiA+ID4gPiArCXBoeV9hZGRyID0gcmVzLT5zdGFydDsNCj4gPiA+ID4gPiA+ DQo+ID4gPiA+ID4gPiBVaCAuLi4gU2hhd24sIEkgcmVhbGx5IHRoaW5rIEkgYW0gbG9zdCBoZXJl LiBEb24ndCB5b3UgbmVlZCB0bw0KPiA+ID4gPiA+ID4gbWFwIHRoaXMgbWVtb3J5IGJlZm9yZSB5 b3UgY2FuIHVzZSBpdCBmb3IgRE1BID8gVGhlIERNQQ0KPiA+ID4gPiA+ID4gbWFwcGluZyBmdW5j dGlvbiBzaG91bGQgZ2l2ZSB5b3UgdGhlIHBoeXNpY2FsIGFkZHJlc3MgYW5kIGlzDQo+ID4gPiA+ ID4gPiB0aGUgcmlnaHQgd2F5IHRvIGdvIGFib3V0IHRoaXMgaW5zdGVhZCBvZiBwdWxsaW5nIHRo ZSBhZGRyZXNzDQo+IGZyb20gaGVyZSwgbm8gPw0KPiA+ID4gPiA+ID4NCj4gPiA+ID4gPiA+IEkg bWlnaHQgYmUgd3JvbmcgaGVyZSwgSSBhbSByYXRoZXIgdW5jZXJ0YWluLCBzbyBwbGVhc2UgaGVs cCBtZQ0KPiBvdXQuDQo+ID4gPiA+ID4gPiBUaGFua3MhDQo+ID4gPiA+ID4NCj4gPiA+ID4gPiBI aSwgTWFyZWssIFRoYW5rcyBmb3IgeW91ciBzdWdnZXN0aW9uLg0KPiA+ID4gPiA+IEhlcmUgeW91 IGNhbiByZXZpZXcgdGhlIGNvZGUgaW4gaW5jbHVkZS9saW51eC9pb3BvcnQuaCBUaGUNCj4gPiA+ ID4gPiByZXNvdXJjZS0+c3RhcnQgZGVzY3JpYmVzIHRoZSBlbnRpdHkgb24gdGhlIENQVSBidXMg YXMgYQ0KPiA+ID4gPiA+IHJlc291cmNlLT5zdGFydGluZw0KPiA+ID4gPiA+IHBoeXNpY2FsIGFk ZHJlc3MuIFNvIEkgdGhpbmtzIGl0IGNhbiB1c2VkIGZvciBkbWEgZGlyZWN0bHkuDQo+ID4gPiA+ DQo+ID4gPiA+IFRoaXMgZG9lc24ndCBmZWVsIHJpZ2h0IGZvciBzb21lIHJlYXNvbi4gSWYgdGhp cyBpcyBhIHJlZ2lzdGVyDQo+ID4gPiA+IGFyZWEsIHlvdSBzaG91bGQNCj4gPiA+ID4gaW9yZW1h cCgpIGl0LiBJZiBpdCdzIGEgbWVtb3J5IGFyZWEgeW91IGRvIERNQSB0by9mcm9tLCB5b3UgbmVl ZA0KPiA+ID4gPiB0byBtYWtlIHN1cmUgeW91IGNvcnJlY3RseSBmbHVzaC9pbnZhbGlkYXRlIGNh Y2hlcyBhbmQgcHJvcGVybHkNCj4gPiA+ID4gaGFuZGxlIHRoZSBlZmZlY3RzIHRoZSB3cml0ZSBi dWZmZXIgbWlnaHQgaGF2ZS4gQnV0IEkgaGF2ZSBhDQo+ID4gPiA+IGZlZWxpbmcgeW91IGFjdHVh bGx5IGRvIERNQSB0by9mcm9tIHJlZ2lzdGVyIHNwYWNlIGhlcmUgPw0KPiA+ID4NCj4gPiA+IFll cywgSXQncyBhIHJlZ2lzdGVyIGFyZWEuIEJ1dCBJIGRvbid0IGtub3cgd2h5IEkgc2hvdWxkIGlv cmVtYXAoKQ0KPiA+ID4gaXQ/IEl0J3MgYSBidXMgYWRkcmVzcyBhbmQgRE1BIGNhbiB1c2UgaXQg ZGlyZWN0bHkuIElzIHRoZXJlIHNvbWUNCj4gPiA+IHByb2JsZW0gZm9yIG15IHVuZGVyc3RhbmRp bmcgPw0KPiA+DQo+ID4gSSBhbSBub3QgdG9vIHN1cmUgaGVyZSwgdGh1cyBJIGFtIHBva2luZyBz b21lb25lIHdobyBjYW4gY2xlYXJseSBhbnN3ZXINCj4gdGhpcy4NCj4gDQo+IFRoZXJlIGlzIGFs cmVhZHkgYSBkZXZtX2lvcmVtYXBfcmVzb3VyY2UoKSBjYWxsIGluIHRoZSBleGlzdGluZyBjb2Rl IGZvcg0KPiBDUFUgdG8gYWNjZXNzIHJlZ2lzdGVycyBpbiB2aXJ0dWFsIGFkZHJlc3MuICBBbmQg bXkgdW5kZXJzdGFuZGluZyBvbg0KPiBZdWFuJ3MgcGF0Y2ggaXMgdGhhdCBoZSBuZWVkcyB0aGUg cGh5c2ljYWwgYWRkcmVzcyBvZiBJMkMgREFUQSByZWdpc3Rlcg0KPiBmb3IgRE1BIGZyb20vdG8g dGhlIGNvbnRyb2xsZXIuDQo+IA0KPiBTaGF3bg0KDQpUaGFua3MgeW91IFNoYXduLiBZZXMsIGl0 IGlzLiBETUEgbmVlZCB0aGUgcGh5c2ljYWwgYWRkcmVzcyB3aGljaCByZXNvdXJjZS0+c3RhcnQg aXMuDQoNCkFuZCBIaSBNYXJlaywgSG93IGFib3V0IHlvdT8gSXMgdGhlcmUgc29tZW9uZSB3aG8g Y2FuIGNsZWFybHkgYW5zd2VyIHRoaXMgc29tZSBhbmQgc2hvdWxkIEkgc2VuZCB0aGUgbmV4dCB2 ZXJzaW9uPyANCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, March 11, 2014 at 06:27:39 AM, Yao Yuan wrote: > On Thu, Mar 10, 2014 at 10:01:42 AM, Marek Vasut wrote: > > On Thu, Mar 06, 2014 at 12:57:42PM +0100, Marek Vasut wrote: > > > On Thursday, March 06, 2014 at 06:02:03 AM, Yao Yuan wrote: > > > > On Thu, March 06, 2014 at 12:44:14 PM, Marek Vasut wrote: > > > > > On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote: > > > > > > On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > > > > > > > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > > > > > > > Add dma support for i2c. This function depend on DMA driver. > > > > > > > > You can turn on it by write both the dmas and dma-name > > > > > > > > properties in dts node. > > > > > > > > > > > > > > > > Signed-off-by: Yuan Yao <yao.yuan@freescale.com> > > > > > > > > --- > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct > > > > > > > > platform_device > > > > > > > > > > > > > > *pdev) > > > > > > > > > > > > > > > void __iomem *base; > > > > > > > > int irq, ret; > > > > > > > > u32 bitrate; > > > > > > > > > > > > > > > > + u32 phy_addr; > > > > > > > > > > > > > > > > dev_dbg(&pdev->dev, "<%s>\n", __func__); > > > > > > > > > > > > > > > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct > > > > > > > > platform_device > > > > > > > > > > > > > > *pdev) > > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > > > > > > > > > > > > + phy_addr = res->start; > > > > > > > > > > > > > > Uh ... Shawn, I really think I am lost here. Don't you need to > > > > > > > map this memory before you can use it for DMA ? The DMA > > > > > > > mapping function should give you the physical address and is > > > > > > > the right way to go about this instead of pulling the address > > > > from here, no ? > > > > > > > > > I might be wrong here, I am rather uncertain, so please help me > > > > out. > > > > > > > > > Thanks! > > > > > > > > > > > > Hi, Marek, Thanks for your suggestion. > > > > > > Here you can review the code in include/linux/ioport.h The > > > > > > resource->start describes the entity on the CPU bus as a > > > > > > resource->starting > > > > > > physical address. So I thinks it can used for dma directly. > > > > > > > > > > This doesn't feel right for some reason. If this is a register > > > > > area, you should > > > > > ioremap() it. If it's a memory area you do DMA to/from, you need > > > > > to make sure you correctly flush/invalidate caches and properly > > > > > handle the effects the write buffer might have. But I have a > > > > > feeling you actually do DMA to/from register space here ? > > > > > > > > Yes, It's a register area. But I don't know why I should ioremap() > > > > it? It's a bus address and DMA can use it directly. Is there some > > > > problem for my understanding ? > > > > > > I am not too sure here, thus I am poking someone who can clearly answer > > > > this. > > > > There is already a devm_ioremap_resource() call in the existing code for > > CPU to access registers in virtual address. And my understanding on > > Yuan's patch is that he needs the physical address of I2C DATA register > > for DMA from/to the controller. > > > > Shawn > > Thanks you Shawn. Yes, it is. DMA need the physical address which > resource->start is. > > And Hi Marek, How about you? Is there someone who can clearly answer this > some and should I send the next version? While I don't quite understand what Shawn wanted to say by the above, please go ahead and send V2 . -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index db895fb..c8d5e04 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,10 @@ /* Default value */ #define IMX_I2C_BIT_RATE 100000 /* 100kHz */ +/* enable DMA if transfer byte size is bigger than this threshold */ +#define IMX_I2C_DMA_THRESHOLD 16 +#define IMX_I2C_DMA_TIMEOUT 1000 + /* IMX I2C registers: * the I2C register offset is different between SoCs, * to provid support for all these chips, split the @@ -88,6 +97,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 +184,17 @@ struct imx_i2c_hwdata { unsigned i2cr_ien_opcode; }; +struct imx_i2c_dma { + struct dma_chan *chan_tx; + struct dma_chan *chan_rx; + struct dma_chan *chan_using; + dma_addr_t dma_buf; + unsigned int dma_len; + unsigned int dma_transfer_dir; + unsigned int dma_data_dir; + struct completion cmd_complete; +}; + struct imx_i2c_struct { struct i2c_adapter adapter; struct clk *clk; @@ -184,6 +205,9 @@ struct imx_i2c_struct { int stopped; unsigned int ifdr; /* IMX_I2C_IFDR */ const struct imx_i2c_hwdata *hwdata; + + bool use_dma; + struct imx_i2c_dma *dma; }; static const struct imx_i2c_hwdata imx1_i2c_hwdata = { @@ -254,9 +278,120 @@ 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; + } + + 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 = dmaengine_slave_config(dma->chan_tx, &dma_sconfig); + if (ret < 0) { + dev_err(&i2c_imx->adapter.dev, + "Dma slave config failed, err = %d\n", ret); + dma_release_channel(dma->chan_tx); + return ret; + } + + dma->chan_rx = dma_request_slave_channel(&i2c_imx->adapter.dev, "rx"); + if (!dma->chan_rx) { + dev_err(&i2c_imx->adapter.dev, + "Dma rx channel request failed!\n"); + return -ENODEV; + } + + dma_sconfig.src_addr = (dma_addr_t)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_err(&i2c_imx->adapter.dev, + "Dma slave config failed, err = %d\n", ret); + dma_release_channel(dma->chan_rx); + return ret; + } + + init_completion(&dma->cmd_complete); + + return 0; +} + +static void i2c_imx_dma_callback(void *arg) +{ + struct imx_i2c_struct *i2c_imx = (struct imx_i2c_struct *)arg; + struct imx_i2c_dma *dma = i2c_imx->dma; + + dma_unmap_single(dma->chan_using->device->dev, dma->dma_buf, + dma->dma_len, dma->dma_data_dir); + complete(&dma->cmd_complete); +} + +static int i2c_imx_dma_xfer(struct imx_i2c_struct *i2c_imx, + struct i2c_msg *msgs) +{ + struct imx_i2c_dma *dma = i2c_imx->dma; + struct dma_async_tx_descriptor *txdesc; + + dma->dma_buf = dma_map_single(dma->chan_using->device->dev, msgs->buf, + dma->dma_len, dma->dma_data_dir); + if (dma_mapping_error(dma->chan_using->device->dev, dma->dma_buf)) { + dev_err(&i2c_imx->adapter.dev, "dma_map_single failed\n"); + return -EINVAL; + } + + txdesc = dmaengine_prep_slave_single(dma->chan_using, dma->dma_buf, + dma->dma_len, dma->dma_transfer_dir, + DMA_PREP_INTERRUPT | DMA_CTRL_ACK); + if (!txdesc) { + dev_err(&i2c_imx->adapter.dev, + "Not able to get desc for dma xfer\n"); + return -EINVAL; + } + + txdesc->callback = i2c_imx_dma_callback; + txdesc->callback_param = i2c_imx; + dmaengine_submit(txdesc); + dma_async_issue_pending(dma->chan_using); + + return 0; +} + +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->dma_buf = 0; + dma->dma_len = 0; + + dma_chan = dma->chan_tx; + dma_release_channel(dma_chan); + dma->chan_tx = NULL; + + dma_chan = dma->chan_rx; + dma_release_channel(dma_chan); + dma->chan_rx = NULL; + + dma->chan_using = NULL; +} + /** Functions for IMX I2C adapter driver *************************************** *******************************************************************************/ - static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy) { unsigned long orig_jiffies = jiffies; @@ -427,46 +562,98 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id) static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs) { - int i, result; + int i, result, timeout = IMX_I2C_DMA_TIMEOUT; + unsigned int temp = 0; + struct imx_i2c_dma *dma = i2c_imx->dma; dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n", __func__, msgs->addr << 1); - /* write slave address */ - imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR); - result = i2c_imx_trx_complete(i2c_imx); - if (result) - return result; - result = i2c_imx_acked(i2c_imx); - if (result) - return result; - dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__); + if (i2c_imx->use_dma && msgs->len >= IMX_I2C_DMA_THRESHOLD) { + 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; - /* write data */ - for (i = 0; i < msgs->len; i++) { - dev_dbg(&i2c_imx->adapter.dev, - "<%s> write byte: B%d=0x%X\n", - __func__, i, msgs->buf[i]); - imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR); + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); + temp |= I2CR_DMAEN; + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); + + /* write slave address */ + imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR); + result = wait_for_completion_interruptible_timeout( + &i2c_imx->dma->cmd_complete, + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT)); + if (result == 0) + 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 (result) return result; + } else { + /* write slave address */ + imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR); + result = i2c_imx_trx_complete(i2c_imx); + if (result) + return result; + + result = i2c_imx_acked(i2c_imx); + if (result) + return result; + + dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__); + + /* write data */ + for (i = 0; i < msgs->len; i++) { + dev_dbg(&i2c_imx->adapter.dev, + "<%s> write byte: B%d=0x%X\n", + __func__, i, msgs->buf[i]); + imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR); + result = i2c_imx_trx_complete(i2c_imx); + if (result) + return result; + result = i2c_imx_acked(i2c_imx); + if (result) + return result; + } } return 0; } static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs) { - int i, result; + int i, result, timeout = IMX_I2C_DMA_TIMEOUT; unsigned int temp; + struct imx_i2c_dma *dma = i2c_imx->dma; dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n", __func__, (msgs->addr << 1) | 0x01); + /* write slave address */ imx_i2c_write_reg((msgs->addr << 1) | 0x01, i2c_imx, IMX_I2C_I2DR); result = i2c_imx_trx_complete(i2c_imx); @@ -488,33 +675,71 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs) dev_dbg(&i2c_imx->adapter.dev, "<%s> read data\n", __func__); - /* read data */ - for (i = 0; i < msgs->len; i++) { - result = i2c_imx_trx_complete(i2c_imx); + if (i2c_imx->use_dma && msgs->len >= IMX_I2C_DMA_THRESHOLD) { + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); + temp |= I2CR_DMAEN; + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); + + reinit_completion(&i2c_imx->dma->cmd_complete); + dma->chan_using = dma->chan_rx; + dma->dma_transfer_dir = DMA_DEV_TO_MEM; + dma->dma_data_dir = DMA_FROM_DEVICE; + dma->dma_len = msgs->len - 2; + result = i2c_imx_dma_xfer(i2c_imx, msgs); if (result) return result; - if (i == (msgs->len - 1)) { - /* It must generate STOP before read I2DR to prevent - controller from generating another clock cycle */ - dev_dbg(&i2c_imx->adapter.dev, - "<%s> clear MSTA\n", __func__); - temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); - temp &= ~(I2CR_MSTA | I2CR_MTX); - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); - i2c_imx_bus_busy(i2c_imx, 0); - i2c_imx->stopped = 1; - } else if (i == (msgs->len - 2)) { + + result = wait_for_completion_interruptible_timeout( + &i2c_imx->dma->cmd_complete, + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT)); + 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); + } else { + /* read data */ + for (i = 0; i < msgs->len - 2; i++) { + result = i2c_imx_trx_complete(i2c_imx); + if (result) + return result; + msgs->buf[i] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); dev_dbg(&i2c_imx->adapter.dev, - "<%s> set TXAK\n", __func__); - temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); - temp |= I2CR_TXAK; - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); + "<%s> read byte: B%d=0x%X\n", + __func__, i, msgs->buf[i]); } - msgs->buf[i] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); - dev_dbg(&i2c_imx->adapter.dev, - "<%s> read byte: B%d=0x%X\n", - __func__, i, msgs->buf[i]); + result = i2c_imx_trx_complete(i2c_imx); } + + /* read n-1 byte data */ + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); + temp |= I2CR_TXAK; + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); + + msgs->buf[msgs->len-2] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); + /* read n byte data */ + result = i2c_imx_trx_complete(i2c_imx); + if (result) + return result; + + /* It must generate STOP before read I2DR to prevent + controller from generating another clock cycle */ + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); + temp &= ~(I2CR_MSTA | I2CR_MTX); + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); + i2c_imx_bus_busy(i2c_imx, 0); + i2c_imx->stopped = 1; + msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); + return 0; } @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct platform_device *pdev) void __iomem *base; int irq, ret; u32 bitrate; + u32 phy_addr; dev_dbg(&pdev->dev, "<%s>\n", __func__); @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct platform_device *pdev) } res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + phy_addr = res->start; base = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(base)) return PTR_ERR(base); @@ -696,6 +923,21 @@ static int i2c_imx_probe(struct platform_device *pdev) i2c_imx->adapter.name); dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n"); + /* Init DMA config if support*/ + i2c_imx->dma = devm_kzalloc(&pdev->dev, sizeof(struct imx_i2c_dma), + GFP_KERNEL); + if (!i2c_imx->dma) { + dev_info(&pdev->dev, + "can't allocate dma struct faild use dma.\n"); + i2c_imx->use_dma = false; + } else if (i2c_imx_dma_request(i2c_imx, phy_addr)) { + dev_info(&pdev->dev, + "can't request dma chan, faild use dma.\n"); + i2c_imx->use_dma = false; + } else { + i2c_imx->use_dma = true; + } + return 0; /* Return OK */ } @@ -707,6 +949,9 @@ static int i2c_imx_remove(struct platform_device *pdev) dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n"); i2c_del_adapter(&i2c_imx->adapter); + if (i2c_imx->use_dma) + i2c_imx_dma_free(i2c_imx); + /* setup chip registers to defaults */ imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR); imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
Add dma support for i2c. This function depend on DMA driver. You can turn on it by write both the dmas and dma-name properties in dts node. Signed-off-by: Yuan Yao <yao.yuan@freescale.com> --- drivers/i2c/busses/i2c-imx.c | 341 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 293 insertions(+), 48 deletions(-)