Re: [PATCH v2 1/2] i2c: add DMA support for freescale i2c driver
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 > > > > > > > > --- > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > @@ -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(>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-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 v2 1/2] i2c: add DMA support for freescale i2c driver
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.y...@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-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 v2 1/2] i2c: add DMA support for freescale i2c driver
On Thu, Mar 10, 2014 at 10:01:42 AM, Marek Vasut wrote: > On Thu, Mar 06, 2014 at 12:57:42PM +0100, Marek Vasut wrote: > > On Thursday, March 06, 2014 at 06:02:03 AM, Yao Yuan wrote: > > > On Thu, March 06, 2014 at 12:44:14 PM, Marek Vasut wrote: > > > > On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote: > > > > > On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > > > > > > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > > > > > > Add dma support for i2c. This function depend on DMA driver. > > > > > > > You can turn on it by write both the dmas and dma-name > > > > > > > properties in dts node. > > > > > > > > > > > > > > Signed-off-by: Yuan Yao > > > > > > > --- > > > > > > > > > > > > [...] > > > > > > > > > > > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct > > > > > > > platform_device > > > > > > > > > > > > *pdev) > > > > > > > > > > > > > void __iomem *base; > > > > > > > int irq, ret; > > > > > > > u32 bitrate; > > > > > > > > > > > > > > + u32 phy_addr; > > > > > > > > > > > > > > dev_dbg(>dev, "<%s>\n", __func__); > > > > > > > > > > > > > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct > > > > > > > platform_device > > > > > > > > > > > > *pdev) > > > > > > > > > > > > > } > > > > > > > > > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > > > > > > > > > > + phy_addr = res->start; > > > > > > > > > > > > Uh ... Shawn, I really think I am lost here. Don't you need to > > > > > > map this memory before you can use it for DMA ? The DMA > > > > > > mapping function should give you the physical address and is > > > > > > the right way to go about this instead of pulling the address > from here, no ? > > > > > > > > > > > > I might be wrong here, I am rather uncertain, so please help me > out. > > > > > > Thanks! > > > > > > > > > > Hi, Marek, Thanks for your suggestion. > > > > > Here you can review the code in include/linux/ioport.h The > > > > > resource->start describes the entity on the CPU bus as a > > > > > resource->starting > > > > > physical address. So I thinks it can used for dma directly. > > > > > > > > This doesn't feel right for some reason. If this is a register > > > > area, you should > > > > ioremap() it. If it's a memory area you do DMA to/from, you need > > > > to make sure you correctly flush/invalidate caches and properly > > > > handle the effects the write buffer might have. But I have a > > > > feeling you actually do DMA to/from register space here ? > > > > > > Yes, It's a register area. But I don't know why I should ioremap() > > > it? It's a bus address and DMA can use it directly. Is there some > > > problem for my understanding ? > > > > I am not too sure here, thus I am poking someone who can clearly answer > this. > > There is already a devm_ioremap_resource() call in the existing code for > CPU to access registers in virtual address. And my understanding on > Yuan's patch is that he needs the physical address of I2C DATA register > for DMA from/to the controller. > > Shawn Thanks you Shawn. Yes, it is. DMA need the physical address which resource->start is. And Hi Marek, How about you? Is there someone who can clearly answer this some and should I send the next version? N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a��� 0��h���i
RE: [PATCH v2 1/2] i2c: add DMA support for freescale i2c driver
On Thu, Mar 10, 2014 at 10:01:42 AM, Marek Vasut wrote: On Thu, Mar 06, 2014 at 12:57:42PM +0100, Marek Vasut wrote: On Thursday, March 06, 2014 at 06:02:03 AM, Yao Yuan wrote: On Thu, March 06, 2014 at 12:44:14 PM, Marek Vasut wrote: On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote: On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: Add dma support for i2c. This function depend on DMA driver. You can turn on it by write both the dmas and dma-name properties in dts node. Signed-off-by: Yuan Yao yao.y...@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? 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 v2 1/2] i2c: add DMA support for freescale i2c driver
On Thu, Mar 06, 2014 at 12:57:42PM +0100, Marek Vasut wrote: > On Thursday, March 06, 2014 at 06:02:03 AM, Yao Yuan wrote: > > On Thu, March 06, 2014 at 12:44:14 PM, Marek Vasut wrote: > > > On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote: > > > > On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > > > > > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > > > > > Add dma support for i2c. This function depend on DMA driver. > > > > > > You can turn on it by write both the dmas and dma-name properties > > > > > > in dts node. > > > > > > > > > > > > Signed-off-by: Yuan Yao > > > > > > --- > > > > > > > > > > [...] > > > > > > > > > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct > > > > > > platform_device > > > > > > > > > > *pdev) > > > > > > > > > > > void __iomem *base; > > > > > > int irq, ret; > > > > > > u32 bitrate; > > > > > > > > > > > > + u32 phy_addr; > > > > > > > > > > > > dev_dbg(>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-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 v2 1/2] i2c: add DMA support for freescale i2c driver
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.y...@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-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 v2 1/2] i2c: add DMA support for freescale i2c driver
On Thursday, March 06, 2014 at 06:02:03 AM, Yao Yuan wrote: > On Thu, March 06, 2014 at 12:44:14 PM, Marek Vasut wrote: > > On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote: > > > On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > > > > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > > > > Add dma support for i2c. This function depend on DMA driver. > > > > > You can turn on it by write both the dmas and dma-name properties > > > > > in dts node. > > > > > > > > > > Signed-off-by: Yuan Yao > > > > > --- > > > > > > > > [...] > > > > > > > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct > > > > > platform_device > > > > > > > > *pdev) > > > > > > > > > void __iomem *base; > > > > > int irq, ret; > > > > > u32 bitrate; > > > > > > > > > > + u32 phy_addr; > > > > > > > > > > dev_dbg(>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-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 v2 1/2] i2c: add DMA support for freescale i2c driver
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.y...@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-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 v2 1/2] i2c: add DMA support for freescale i2c driver
On Thu, March 06, 2014 at 12:44:14 PM, Marek Vasut wrote: > On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote: > > On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > > > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > > > Add dma support for i2c. This function depend on DMA driver. > > > > You can turn on it by write both the dmas and dma-name properties > > > > in dts node. > > > > > > > > Signed-off-by: Yuan Yao > > > > --- > > > > > > [...] > > > > > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct > > > > platform_device > > > > > > *pdev) > > > > > > > void __iomem *base; > > > > int irq, ret; > > > > u32 bitrate; > > > > > > > > + u32 phy_addr; > > > > > > > > dev_dbg(>dev, "<%s>\n", __func__); > > > > > > > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct > > > > platform_device > > > > > > *pdev) > > > > > > > } > > > > > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > > > > + phy_addr = res->start; > > > > > > Uh ... Shawn, I really think I am lost here. Don't you need to map > > > this memory before you can use it for DMA ? The DMA mapping function > > > should give you the physical address and is the right way to go > > > about this instead of pulling the address from here, no ? > > > > > > I might be wrong here, I am rather uncertain, so please help me out. > > > Thanks! > > > > Hi, Marek, Thanks for your suggestion. > > Here you can review the code in include/linux/ioport.h The > > resource->start describes the entity on the CPU bus as a starting > > physical address. So I thinks it can used for dma directly. > > This doesn't feel right for some reason. If this is a register area, you > should > ioremap() it. If it's a memory area you do DMA to/from, you need to make > sure you correctly flush/invalidate caches and properly handle the > effects the write buffer might have. But I have a feeling you actually do > DMA to/from register space here ? > Yes, It's a register area. But I don't know why I should ioremap() it? It's a bus address and DMA can use it directly. Is there some problem for my understanding ? N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a��� 0��h���i
Re: [PATCH v2 1/2] i2c: add DMA support for freescale i2c driver
On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote: > On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > > Add dma support for i2c. This function depend on DMA driver. > > > You can turn on it by write both the dmas and dma-name properties in > > > dts node. > > > > > > Signed-off-by: Yuan Yao > > > --- > > > > [...] > > > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct platform_device > > > > *pdev) > > > > > void __iomem *base; > > > int irq, ret; > > > u32 bitrate; > > > > > > + u32 phy_addr; > > > > > > dev_dbg(>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-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 v2 1/2] i2c: add DMA support for freescale i2c driver
On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: > On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: > > Add dma support for i2c. This function depend on DMA driver. > > You can turn on it by write both the dmas and dma-name properties in > > dts node. > > > > Signed-off-by: Yuan Yao > > --- > > [...] > > > @@ -601,6 +826,7 @@ static int i2c_imx_probe(struct platform_device > *pdev) > > void __iomem *base; > > int irq, ret; > > u32 bitrate; > > + u32 phy_addr; > > > > dev_dbg(>dev, "<%s>\n", __func__); > > > > @@ -611,6 +837,7 @@ static int i2c_imx_probe(struct platform_device > *pdev) > > } > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + phy_addr = res->start; > > Uh ... Shawn, I really think I am lost here. Don't you need to map this > memory before you can use it for DMA ? The DMA mapping function should > give you the physical address and is the right way to go about this > instead of pulling the address from here, no ? > > I might be wrong here, I am rather uncertain, so please help me out. > Thanks! Hi, Marek, Thanks for your suggestion. Here you can review the code in include/linux/ioport.h The resource->start describes the entity on the CPU bus as a starting physical address. So I thinks it can used for dma directly.
Re: [PATCH v2 1/2] i2c: add DMA support for freescale i2c driver
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 > --- [...] > struct imx_i2c_struct { > struct i2c_adapter adapter; > struct clk *clk; > @@ -184,6 +205,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; 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(_imx->adapter.dev, "tx"); Why don't you define a new variable for this dev pointer ? struct device *dev = _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(_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, _sconfig); > + if (ret < 0) { > + dev_err(_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(_imx->adapter.dev, "rx"); > + if (!dma->chan_rx) { > + dev_err(_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, _sconfig); > + if (ret < 0) { > + dev_err(_imx->adapter.dev, > + "Dma slave config failed, err = %d\n", ret); > + dma_release_channel(dma->chan_rx); > + return ret; > + } > + > + init_completion(>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(>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(_imx->adapter.dev, "dma_map_single failed\n"); > + return -EINVAL; > + } > + > + txdesc = dmaengine_prep_slave_single(dma->chan_using, dma->dma_buf, > +
Re: [PATCH v2 1/2] i2c: add DMA support for freescale i2c driver
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.y...@freescale.com --- [...] struct imx_i2c_struct { struct i2c_adapter adapter; struct clk *clk; @@ -184,6 +205,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; 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, +
RE: [PATCH v2 1/2] i2c: add DMA support for freescale i2c driver
On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: Add dma support for i2c. This function depend on DMA driver. You can turn on it by write both the dmas and dma-name properties in dts node. Signed-off-by: Yuan Yao yao.y...@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.
Re: [PATCH v2 1/2] i2c: add DMA support for freescale i2c driver
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.y...@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-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 v2 1/2] i2c: add DMA support for freescale i2c driver
On Thu, March 06, 2014 at 12:44:14 PM, Marek Vasut wrote: On Thursday, March 06, 2014 at 05:36:14 AM, Yao Yuan wrote: On Thu, March 06, 2014 at 11:23:50 AM, Marek Vasut wrote: On Wednesday, March 05, 2014 at 07:52:31 AM, Yuan Yao wrote: Add dma support for i2c. This function depend on DMA driver. You can turn on it by write both the dmas and dma-name properties in dts node. Signed-off-by: Yuan Yao yao.y...@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 ? 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