| Message ID | 1341850974-11977-2-git-send-email-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
Hi, On Mon, Jul 09, 2012 at 06:22:54PM +0200, Marek Vasut wrote: > This patch implements DMA support into mxs-i2c. DMA transfers are now enabled > via DT. The DMA operation is enabled by default. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Detlev Zundel <dzu@denx.de> > CC: Dong Aisheng <b29396@freescale.com> > CC: Fabio Estevam <fabio.estevam@freescale.com> > Cc: Linux ARM kernel <linux-arm-kernel@lists.infradead.org> > Cc: linux-i2c@vger.kernel.org > CC: Sascha Hauer <s.hauer@pengutronix.de> > CC: Shawn Guo <shawn.guo@linaro.org> > Cc: Stefano Babic <sbabic@denx.de> > CC: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Cc: Wolfgang Denk <wd@denx.de> > Cc: Wolfram Sang <w.sang@pengutronix.de> > --- > Documentation/devicetree/bindings/i2c/i2c-mxs.txt | 5 + > arch/arm/boot/dts/imx28.dtsi | 2 + > drivers/i2c/busses/i2c-mxs.c | 267 +++++++++++++++++++-- > 3 files changed, 252 insertions(+), 22 deletions(-) > > V2: Fixed return value from mxs_i2c_dma_setup_xfer(). > Fixed coding style nitpicks > Call mxs_i2c_dma_finish() in failpath only if DMA is active > V3: Align with changes in previous patch > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt > index 30ac3a0..45b6307 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt > +++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt > @@ -6,6 +6,10 @@ Required properties: > - interrupts: Should contain ERROR and DMA interrupts > - clock-frequency: Desired I2C bus clock frequency in Hz. > Only 100000Hz and 400000Hz modes are supported. > +- fsl,i2c-dma-channel: APBX DMA channel for the I2C > + > +Optional properties: > +- fsl,use-pio: Use PIO transfers instead of DMA, useful for debug Having PIOQUEUE (not PIO) as a fallback is good. I'd rather like to see this as a module parameter, though. For one reason, this is not a hardware property or board specific, so not a good device tree property. Also, it is implicitly deprecated somehow since either we want DMA to fully work or, even better, somewhen be able to automatically switch between PIOQUEUE and DMA depending on the i2c_msg size. Deprecated properties are also troublesome. Third, we don't really need this per instance, if somebody really has problems with DMA, it will apply to all i2c busses. Makes sense? > > Examples: > > @@ -16,4 +20,5 @@ i2c0: i2c@80058000 { > reg = <0x80058000 2000>; > interrupts = <111 68>; > clock-frequency = <100000>; > + fsl,i2c-dma-channel = <6>; > }; > + /* > + * The MXS I2C DMA mode is prefered and enabled by default. > + * The PIO mode is still supported, but should be used only PIOQUEUE > + * for debuging purposes etc. > + */ > + i2c->dma_mode = 1; > + if (of_find_property(node, "fsl,use-pio", NULL)) { > + i2c->dma_mode = 0; > + dev_info(dev, "Using PIO mode for I2C transfers!\n"); > + } > + > + /* > + * TODO: This is a temporary solution and should be changed > + * to use generic DMA binding later when the helpers get in. > + */ @Shawn: Any idea when this is going to happen? And why do we need this? AFAICT it will be always channel 6/7 on mx28? > + ret = of_property_read_u32(node, "fsl,i2c-dma-channel", > + &i2c->dma_channel); > + if (ret) { > + dev_warn(dev, "Failed to get DMA channel!\n"); > + i2c->dma_mode = 0; > + } > + Rest looks good, thanks! Wolfram
Dear Wolfram Sang, > Hi, > > On Mon, Jul 09, 2012 at 06:22:54PM +0200, Marek Vasut wrote: > > This patch implements DMA support into mxs-i2c. DMA transfers are now > > enabled via DT. The DMA operation is enabled by default. > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > Cc: Detlev Zundel <dzu@denx.de> > > CC: Dong Aisheng <b29396@freescale.com> > > CC: Fabio Estevam <fabio.estevam@freescale.com> > > Cc: Linux ARM kernel <linux-arm-kernel@lists.infradead.org> > > Cc: linux-i2c@vger.kernel.org > > CC: Sascha Hauer <s.hauer@pengutronix.de> > > CC: Shawn Guo <shawn.guo@linaro.org> > > Cc: Stefano Babic <sbabic@denx.de> > > CC: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Cc: Wolfgang Denk <wd@denx.de> > > Cc: Wolfram Sang <w.sang@pengutronix.de> > > --- > > > > Documentation/devicetree/bindings/i2c/i2c-mxs.txt | 5 + > > arch/arm/boot/dts/imx28.dtsi | 2 + > > drivers/i2c/busses/i2c-mxs.c | 267 > > +++++++++++++++++++-- 3 files changed, 252 insertions(+), 22 > > deletions(-) > > > > V2: Fixed return value from mxs_i2c_dma_setup_xfer(). > > > > Fixed coding style nitpicks > > Call mxs_i2c_dma_finish() in failpath only if DMA is active > > > > V3: Align with changes in previous patch > > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt > > b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt index > > 30ac3a0..45b6307 100644 > > --- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt > > +++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt > > > > @@ -6,6 +6,10 @@ Required properties: > > - interrupts: Should contain ERROR and DMA interrupts > > - clock-frequency: Desired I2C bus clock frequency in Hz. > > > > Only 100000Hz and 400000Hz modes are supported. > > > > +- fsl,i2c-dma-channel: APBX DMA channel for the I2C > > + > > +Optional properties: > > +- fsl,use-pio: Use PIO transfers instead of DMA, useful for debug > > Having PIOQUEUE (not PIO) as a fallback is good. I'd rather like to see > this as a module parameter, though. It'd be cool if someone complained earlier and the responses to new patches would be faster. This series has been going on for more than two months and noone complained about this part until now. Instead I was made to document this and now I have to do it in completely different way? Decide already ... > For one reason, this is not a > hardware property or board specific, so not a good device tree property. Actually, there're still people who might benefit of doing PIOq only, since they do small (eg. one byte) transfers. And this is good to have configurable per bus. > Also, it is implicitly deprecated somehow since either we want DMA to > fully work And the DMA doesn't fully work? > or, even better, somewhen be able to automatically switch > between PIOQUEUE and DMA depending on the i2c_msg size. That'd be cool. Some months ago, you promised to take a look. I tried it recently again with not much luck. > Deprecated > properties are also troublesome. Third, we don't really need this per > instance, if somebody really has problems with DMA, it will apply to all > i2c busses. Makes sense? No, it doesn't. See above about small transfers. Consider the easy situation where you have sensor on one bus (so you do PIO because you transfer small data) and you have EEPROM on other bus, where you use DMA because you transfer large data. And the mixed mode isn't there yet. > > Examples: > > @@ -16,4 +20,5 @@ i2c0: i2c@80058000 { > > > > reg = <0x80058000 2000>; > > interrupts = <111 68>; > > clock-frequency = <100000>; > > > > + fsl,i2c-dma-channel = <6>; > > > > }; > > > > + /* > > + * The MXS I2C DMA mode is prefered and enabled by default. > > + * The PIO mode is still supported, but should be used only > > PIOQUEUE > > > + * for debuging purposes etc. > > + */ > > + i2c->dma_mode = 1; > > + if (of_find_property(node, "fsl,use-pio", NULL)) { > > + i2c->dma_mode = 0; > > + dev_info(dev, "Using PIO mode for I2C transfers!\n"); > > + } > > + > > + /* > > + * TODO: This is a temporary solution and should be changed > > + * to use generic DMA binding later when the helpers get in. > > + */ > > @Shawn: Any idea when this is going to happen? And why do we need this? > AFAICT it will be always channel 6/7 on mx28? > > > + ret = of_property_read_u32(node, "fsl,i2c-dma-channel", > > + &i2c->dma_channel); > > + if (ret) { > > + dev_warn(dev, "Failed to get DMA channel!\n"); > > + i2c->dma_mode = 0; > > + } > > + > > Rest looks good, thanks! > > Wolfram Best regards, Marek Vasut
Marek, > It'd be cool if someone complained earlier and the responses to new patches > would be faster. This series has been going on for more than two months and I agree it would be cool to response faster. Maintaining being mostly my priavte fun, I can't make guarantees, though. It just happened that I didn't have much time for Linux in the last weeks. Actually, I am quite happy that I managed to work through most of the backlog for the next merge-window at all (in my holidays!). That's from my side, I can't tell why other people didn't review; all being too busy is the most likely guess. > noone complained about this part until now. Instead I was made to document this > and now I have to do it in completely different way? Decide already ... When we decided to have the fallback mode as a configuration option, the driver was still considering platform_data. That would have been no problem... > > > For one reason, this is not a > > hardware property or board specific, so not a good device tree property. > > Actually, there're still people who might benefit of doing PIOq only, since they > do small (eg. one byte) transfers. And this is good to have configurable per > bus. ... but we are now on devicetree and things are different there, sadly. "use-pio" is exposing a driver specific detail which is neither describing the hardware nor it is OS agnostic. Other drivers from other OS might not even have the seperation, because they might only use PIO mode (not even PIOQUEUE). So, the binding is not acceptable AFAIK. We can add devicetree-discuss to the CC-list. > > Also, it is implicitly deprecated somehow since either we want DMA to > > fully work > > And the DMA doesn't fully work? It probably does. I was just stating the intention. > > or, even better, somewhen be able to automatically switch > > between PIOQUEUE and DMA depending on the i2c_msg size. > > That'd be cool. Some months ago, you promised to take a look. I tried it > recently again with not much luck. Yes, I wanted to. I couldn't (see above). I do imagine that it really might not be possible to switch modes at runtime. This is why I was trying to get the patch into the next release without the mode-switching. But for that to happen, there was the binding issue. I came up with the module parameter as the easiest way to fix it. I am open to other solutions. Simply dropping the binding might be another idea if we pay attention that there are no regressions, going from PIOQUEUE to DMA. I am also still interested to check the runtime switching, but it might take another month until I can really hack on it. > > Deprecated > > properties are also troublesome. Third, we don't really need this per > > instance, if somebody really has problems with DMA, it will apply to all > > i2c busses. Makes sense? > > No, it doesn't. See above about small transfers. Consider the easy situation > where you have sensor on one bus (so you do PIO because you transfer small data) > and you have EEPROM on other bus, where you use DMA because you transfer large > data. And the mixed mode isn't there yet. I fully understand what you want to configure. I did before. Yet, devicetree bindings are not platform_data and shouldn't be used like them. Regards, Wolfram
Dear Wolfram Sang, [...] > > That'd be cool. Some months ago, you promised to take a look. I tried it > > recently again with not much luck. > > Yes, I wanted to. I couldn't (see above). I do imagine that it really > might not be possible to switch modes at runtime. This is why I was > trying to get the patch into the next release without the > mode-switching. But for that to happen, there was the binding issue. I > came up with the module parameter as the easiest way to fix it. I am > open to other solutions. Simply dropping the binding might be another > idea if we pay attention that there are no regressions, going from > PIOQUEUE to DMA. > > I am also still interested to check the runtime switching, but it might > take another month until I can really hack on it. Good, there is some bit that probably needs to be flipped to allow this switching. I managed to get this working with SPI, not with i2c though. With i2c, if I restarted the controller inbetween each transaction, it worked ... which is not what I'd like to see there. > > > Deprecated > > > properties are also troublesome. Third, we don't really need this per > > > instance, if somebody really has problems with DMA, it will apply to > > > all i2c busses. Makes sense? > > > > No, it doesn't. See above about small transfers. Consider the easy > > situation where you have sensor on one bus (so you do PIO because you > > transfer small data) and you have EEPROM on other bus, where you use DMA > > because you transfer large data. And the mixed mode isn't there yet. > > I fully understand what you want to configure. I did before. Yet, > devicetree bindings are not platform_data and shouldn't be used like > them. But then, how would you configure this detail on a per-bus basis? Well all right, screw this, let's make it impotent and go for the module parameter. This patch actually fixes a real issue, I'd like to have it in ASAP and it's been aboue three months already, which sucks. > Regards, > > Wolfram Best regards, Marek Vasut
On Fri, Jul 13, 2012 at 10:22:49AM +0200, Wolfram Sang wrote: > > + /* > > + * TODO: This is a temporary solution and should be changed > > + * to use generic DMA binding later when the helpers get in. > > + */ > > @Shawn: Any idea when this is going to happen? And why do we need this? See thread [1] for current statues. I'm not sure when it's going to happen though. > AFAICT it will be always channel 6/7 on mx28? > Yes, but it might be a different channel on mx23. Just like we define IO region and interrupt number in device tree, dma channel is just another resource of hardware block that we choose to define in device tree. Regards, Shawn [1] http://thread.gmane.org/gmane.linux.ports.arm.omap/75828
Marek, > > I am also still interested to check the runtime switching, but it might > > take another month until I can really hack on it. > > Good, there is some bit that probably needs to be flipped to allow this > switching. I managed to get this working with SPI, not with i2c though. With Ah, hearing that it works with SPI is good news. > i2c, if I restarted the controller inbetween each transaction, it worked ... > which is not what I'd like to see there. Agreed. > > > No, it doesn't. See above about small transfers. Consider the easy > > > situation where you have sensor on one bus (so you do PIO because you > > > transfer small data) and you have EEPROM on other bus, where you use DMA > > > because you transfer large data. And the mixed mode isn't there yet. > > > > I fully understand what you want to configure. I did before. Yet, > > devicetree bindings are not platform_data and shouldn't be used like > > them. > > But then, how would you configure this detail on a per-bus basis? Well all This is a question for devicetree-discuss. > patch actually fixes a real issue, I'd like to have it in ASAP and it's been > aboue three months already, which sucks. I am open to ideas improving the situation (which is: a lot more patches, but not a lot more reviewers) Thanks, Wolfram
Dear Wolfram Sang, > Marek, > > > > I am also still interested to check the runtime switching, but it might > > > take another month until I can really hack on it. > > > > Good, there is some bit that probably needs to be flipped to allow this > > switching. I managed to get this working with SPI, not with i2c though. > > With > > Ah, hearing that it works with SPI is good news. > > > i2c, if I restarted the controller inbetween each transaction, it worked > > ... which is not what I'd like to see there. > > Agreed. > > > > > No, it doesn't. See above about small transfers. Consider the easy > > > > situation where you have sensor on one bus (so you do PIO because you > > > > transfer small data) and you have EEPROM on other bus, where you use > > > > DMA because you transfer large data. And the mixed mode isn't there > > > > yet. > > > > > > I fully understand what you want to configure. I did before. Yet, > > > devicetree bindings are not platform_data and shouldn't be used like > > > them. > > > > But then, how would you configure this detail on a per-bus basis? Well > > all > > This is a question for devicetree-discuss. Did you Cc it? > > patch actually fixes a real issue, I'd like to have it in ASAP and it's > > been aboue three months already, which sucks. > > I am open to ideas improving the situation (which is: a lot more > patches, but not a lot more reviewers) > > Thanks, > > Wolfram Best regards, Marek Vasut
> > > > > No, it doesn't. See above about small transfers. Consider the easy > > > > > situation where you have sensor on one bus (so you do PIO because you > > > > > transfer small data) and you have EEPROM on other bus, where you use > > > > > DMA because you transfer large data. And the mixed mode isn't there > > > > > yet. > > > > > > > > I fully understand what you want to configure. I did before. Yet, > > > > devicetree bindings are not platform_data and shouldn't be used like > > > > them. > > > > > > But then, how would you configure this detail on a per-bus basis? Well > > > all > > > > This is a question for devicetree-discuss. > > Did you Cc it? Well, I guess you already checked the mail headers and found out yourself. The question behind the question probably is "Why didn't you CC it?". The answer to that is that I didn't have much success by adding it to $RANDOM_THREAD. It might be more efficient to answer starting a new thread with explicit "[RFC] $THE_ISSUE". Since a conclusion there won't make the next merge-window anyhow, I haven't done that now. That put aside, it might be more efficient if someone with a bigger urge might strive for a solution (yes, probably, but not necessarily you). For me, it currently is just one task which needs to be scheduled inbetween others. Regards, Wolfram
On Sun, Jul 15, 2012 at 04:17:16PM +0800, Shawn Guo wrote: > On Fri, Jul 13, 2012 at 10:22:49AM +0200, Wolfram Sang wrote: > > > + /* > > > + * TODO: This is a temporary solution and should be changed > > > + * to use generic DMA binding later when the helpers get in. > > > + */ > > > > @Shawn: Any idea when this is going to happen? And why do we need this? > > See thread [1] for current statues. I'm not sure when it's going to > happen though. Phew, [1] is a bit too much too read. I will just assume there are still issues. > > AFAICT it will be always channel 6/7 on mx28? > > > Yes, but it might be a different channel on mx23. Just like we define > IO region and interrupt number in device tree, dma channel is just > another resource of hardware block that we choose to define in device > tree. What makes me wonder now that I come to think of it (not necessarily a question for Shawn but to all): If I have an I2C slave with an interrupt line tied to something, GPIO or external IRQ from the SoC, it makes perfect sense to define that in the devicetree. Yet, if I know the compatible property for the mxs I2C driver, and also know the CPU type (be it MX23 or MX28), I can deduce from that a lot of information, including DMA channel. That is fix. Why encode it? Regards, Wolfram
Dear Wolfram Sang, > On Sun, Jul 15, 2012 at 04:17:16PM +0800, Shawn Guo wrote: > > On Fri, Jul 13, 2012 at 10:22:49AM +0200, Wolfram Sang wrote: > > > > + /* > > > > + * TODO: This is a temporary solution and should be changed > > > > + * to use generic DMA binding later when the helpers get in. > > > > + */ > > > > > > @Shawn: Any idea when this is going to happen? And why do we need this? > > > > See thread [1] for current statues. I'm not sure when it's going to > > happen though. > > Phew, [1] is a bit too much too read. I will just assume there are still > issues. > > > > AFAICT it will be always channel 6/7 on mx28? > > > > Yes, but it might be a different channel on mx23. Just like we define > > IO region and interrupt number in device tree, dma channel is just > > another resource of hardware block that we choose to define in device > > tree. > > What makes me wonder now that I come to think of it (not necessarily a > question for Shawn but to all): > > If I have an I2C slave with an interrupt line tied to something, GPIO or > external IRQ from the SoC, it makes perfect sense to define that in the > devicetree. > > Yet, if I know the compatible property for the mxs I2C driver, and also > know the CPU type (be it MX23 or MX28), I can deduce from that a lot of > information, including DMA channel. That is fix. Why encode it? You know the compatible and the "fallback compatible". From the later one, you can deduce nothing if that happens to kick in. btw. the PIO discussion on DT discuss is completely ignored. How shall we proceed, this driver is stalled for too long. > Regards, > > Wolfram Best regards, Marek Vasut
> > Yet, if I know the compatible property for the mxs I2C driver, and also > > know the CPU type (be it MX23 or MX28), I can deduce from that a lot of > > information, including DMA channel. That is fix. Why encode it? > > You know the compatible and the "fallback compatible". From the later one, you > can deduce nothing if that happens to kick in. Even if the driver was matched because of an MX23-I2C "compatible" binding, both devicetree and runtime could provide data that it actually runs on MX28. That shouldn't be a problem. > btw. the PIO discussion on DT discuss is completely ignored. How shall we > proceed, this driver is stalled for too long. IIRC I mentioned that a discussion about the bindings won't make the next merge window. That's why I proposed either module_parameter or dropping the binding entirely as possible inbetween options.
Dear Wolfram Sang, > > > Yet, if I know the compatible property for the mxs I2C driver, and also > > > know the CPU type (be it MX23 or MX28), I can deduce from that a lot of > > > information, including DMA channel. That is fix. Why encode it? > > > > You know the compatible and the "fallback compatible". From the later > > one, you can deduce nothing if that happens to kick in. > > Even if the driver was matched because of an MX23-I2C "compatible" > binding, both devicetree and runtime could provide data that it actually > runs on MX28. That shouldn't be a problem. You mean like ... cpu_is_mx28() ? We got rid of that in favor of DT. > > btw. the PIO discussion on DT discuss is completely ignored. How shall we > > proceed, this driver is stalled for too long. > > IIRC I mentioned that a discussion about the bindings won't make the > next merge window. Yet another merge window, I have to mention. And only because very long pauses inbetween reviews and very minor nitpicks. I'm being annoyed by this patch so much I'm thinking of giving up on this. I wasted too much of my free time on this and the result is as is. > That's why I proposed either module_parameter Which I explained is not a way to go. > or > dropping the binding entirely as possible inbetween options. Which is not an option either. And this discussion is only further stalling the patch. We're adding fsl,something properties all over the DT all the time, yet this one is of concern? Best regards, Marek Vasut
> > Even if the driver was matched because of an MX23-I2C "compatible" > > binding, both devicetree and runtime could provide data that it actually > > runs on MX28. That shouldn't be a problem. > > You mean like ... cpu_is_mx28() ? We got rid of that in favor of DT. Might be. But the information is probably somewhere. > > IIRC I mentioned that a discussion about the bindings won't make the > > next merge window. > > Yet another merge window, I have to mention. And only because very long pauses > inbetween reviews and very minor nitpicks. I'm being annoyed by this patch so > much I'm thinking of giving up on this. I wasted too much of my free time on > this and the result is as is. For you it might be a minor nitpick, for me (as a maintainer) it is not. You have to deal with just one binding, I have to deal with many. And since they have to be supported forever, this can easily mess up code and make the subsystem clumsy and whatnot. > > That's why I proposed either module_parameter > > Which I explained is not a way to go. That's why I called it inbetween solution so the patch could go in. It's fine if you don't like it, I prefer dropping the binding as well. > > or > > dropping the binding entirely as possible inbetween options. > > Which is not an option either. It would enable you to add the binding as an out-of-tree patch. > And this discussion is only further stalling the > patch. > We're adding fsl,something properties all over the DT all the time, yet this one > is of concern? Yes. Adding all these properties is IMO not the right way, and I have the impression they often came in because of time pressure like this. If I think it is wrong for the kernel, I have to reject a patch unless I am convinced otherwise. Which did not happen yet; as you found out discussions on devicetree-discuss are slow. Might be another indication that devictree things happen too much at the time currently. This is not specific to your patch, there are more which need discussion or had to be reworked. Regards, Wolfram
On Sat, Jul 21, 2012 at 02:44:06PM +0200, Wolfram Sang wrote: > Yet, if I know the compatible property for the mxs I2C driver, and also > know the CPU type (be it MX23 or MX28), I can deduce from that a lot of > information, including DMA channel. That is fix. Why encode it? > The driver shouldn't know the CPU type but just the IP type/version. We can definitely have "fsl,imx23-i2c" and "fsl,imx28-i2c" compatible strings for iMX23 and iMX28 respectively if the I2C controller on these two SoC differs on the IP inside aspects. But we shouldn't do that for IO region, interrupt number, DMA channel such differences at SoC integration level.
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt index 30ac3a0..45b6307 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-mxs.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-mxs.txt @@ -6,6 +6,10 @@ Required properties: - interrupts: Should contain ERROR and DMA interrupts - clock-frequency: Desired I2C bus clock frequency in Hz. Only 100000Hz and 400000Hz modes are supported. +- fsl,i2c-dma-channel: APBX DMA channel for the I2C + +Optional properties: +- fsl,use-pio: Use PIO transfers instead of DMA, useful for debug Examples: @@ -16,4 +20,5 @@ i2c0: i2c@80058000 { reg = <0x80058000 2000>; interrupts = <111 68>; clock-frequency = <100000>; + fsl,i2c-dma-channel = <6>; }; diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi index e2e9a2b..99bd037 100644 --- a/arch/arm/boot/dts/imx28.dtsi +++ b/arch/arm/boot/dts/imx28.dtsi @@ -587,6 +587,7 @@ reg = <0x80058000 2000>; interrupts = <111 68>; clock-frequency = <100000>; + fsl,i2c-dma-channel = <6>; status = "disabled"; }; @@ -597,6 +598,7 @@ reg = <0x8005a000 2000>; interrupts = <110 69>; clock-frequency = <100000>; + fsl,i2c-dma-channel = <7>; status = "disabled"; }; diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c index 877b169..20290a6 100644 --- a/drivers/i2c/busses/i2c-mxs.c +++ b/drivers/i2c/busses/i2c-mxs.c @@ -7,8 +7,6 @@ * * Copyright (C) 2009-2010 Freescale Semiconductor, Inc. All Rights Reserved. * - * TODO: add dma-support if platform-support for it is available - * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by * the Free Software Foundation; either version 2 of the License, or @@ -31,6 +29,9 @@ #include <linux/of.h> #include <linux/of_device.h> #include <linux/of_i2c.h> +#include <linux/dma-mapping.h> +#include <linux/dmaengine.h> +#include <linux/fsl/mxs-dma.h> #define DRIVER_NAME "mxs-i2c" @@ -146,6 +147,16 @@ struct mxs_i2c_dev { u32 cmd_err; struct i2c_adapter adapter; const struct mxs_i2c_speed_config *speed; + + /* DMA support components */ + bool dma_mode; + int dma_channel; + struct dma_chan *dmach; + struct mxs_dma_data dma_data; + uint32_t pio_data[2]; + uint32_t addr_data; + struct scatterlist sg_io[2]; + bool dma_read; }; static void mxs_i2c_reset(struct mxs_i2c_dev *i2c) @@ -157,7 +168,11 @@ static void mxs_i2c_reset(struct mxs_i2c_dev *i2c) writel(i2c->speed->timing2, i2c->regs + MXS_I2C_TIMING2); writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_SET); - writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE, + if (i2c->dma_mode) + writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE, + i2c->regs + MXS_I2C_QUEUECTRL_CLR); + else + writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE, i2c->regs + MXS_I2C_QUEUECTRL_SET); } @@ -248,6 +263,150 @@ static int mxs_i2c_finish_read(struct mxs_i2c_dev *i2c, u8 *buf, int len) return 0; } +static void mxs_i2c_dma_finish(struct mxs_i2c_dev *i2c) +{ + if (i2c->dma_read) { + dma_unmap_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE); + dma_unmap_sg(i2c->dev, &i2c->sg_io[1], 1, DMA_FROM_DEVICE); + } else { + dma_unmap_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE); + } +} + +static void mxs_i2c_dma_irq_callback(void *param) +{ + struct mxs_i2c_dev *i2c = param; + + complete(&i2c->cmd_complete); + mxs_i2c_dma_finish(i2c); +} + +static int mxs_i2c_dma_setup_xfer(struct i2c_adapter *adap, + struct i2c_msg *msg, uint32_t flags) +{ + struct dma_async_tx_descriptor *desc; + struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap); + + if (msg->flags & I2C_M_RD) { + i2c->dma_read = 1; + i2c->addr_data = (msg->addr << 1) | I2C_SMBUS_READ; + + /* + * SELECT command. + */ + + /* Queue the PIO register write transfer. */ + i2c->pio_data[0] = MXS_CMD_I2C_SELECT; + desc = dmaengine_prep_slave_sg(i2c->dmach, + (struct scatterlist *)&i2c->pio_data[0], + 1, DMA_TRANS_NONE, 0); + if (!desc) { + dev_err(i2c->dev, + "Failed to get PIO reg. write descriptor.\n"); + goto select_init_pio_fail; + } + + /* Queue the DMA data transfer. */ + sg_init_one(&i2c->sg_io[0], &i2c->addr_data, 1); + dma_map_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE); + desc = dmaengine_prep_slave_sg(i2c->dmach, &i2c->sg_io[0], 1, + DMA_MEM_TO_DEV, + DMA_PREP_INTERRUPT | DMA_CTRL_ACK); + if (!desc) { + dev_err(i2c->dev, + "Failed to get DMA data write descriptor.\n"); + goto select_init_dma_fail; + } + + /* + * READ command. + */ + + /* Queue the PIO register write transfer. */ + i2c->pio_data[1] = flags | MXS_CMD_I2C_READ | + MXS_I2C_CTRL0_XFER_COUNT(msg->len); + desc = dmaengine_prep_slave_sg(i2c->dmach, + (struct scatterlist *)&i2c->pio_data[1], + 1, DMA_TRANS_NONE, DMA_PREP_INTERRUPT); + if (!desc) { + dev_err(i2c->dev, + "Failed to get PIO reg. write descriptor.\n"); + goto select_init_dma_fail; + } + + /* Queue the DMA data transfer. */ + sg_init_one(&i2c->sg_io[1], msg->buf, msg->len); + dma_map_sg(i2c->dev, &i2c->sg_io[1], 1, DMA_FROM_DEVICE); + desc = dmaengine_prep_slave_sg(i2c->dmach, &i2c->sg_io[1], 1, + DMA_DEV_TO_MEM, + DMA_PREP_INTERRUPT | DMA_CTRL_ACK); + if (!desc) { + dev_err(i2c->dev, + "Failed to get DMA data write descriptor.\n"); + goto read_init_dma_fail; + } + } else { + i2c->dma_read = 0; + i2c->addr_data = (msg->addr << 1) | I2C_SMBUS_WRITE; + + /* + * WRITE command. + */ + + /* Queue the PIO register write transfer. */ + i2c->pio_data[0] = flags | MXS_CMD_I2C_WRITE | + MXS_I2C_CTRL0_XFER_COUNT(msg->len + 1); + desc = dmaengine_prep_slave_sg(i2c->dmach, + (struct scatterlist *)&i2c->pio_data[0], + 1, DMA_TRANS_NONE, 0); + if (!desc) { + dev_err(i2c->dev, + "Failed to get PIO reg. write descriptor.\n"); + goto write_init_pio_fail; + } + + /* Queue the DMA data transfer. */ + sg_init_table(i2c->sg_io, 2); + sg_set_buf(&i2c->sg_io[0], &i2c->addr_data, 1); + sg_set_buf(&i2c->sg_io[1], msg->buf, msg->len); + dma_map_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE); + desc = dmaengine_prep_slave_sg(i2c->dmach, i2c->sg_io, 2, + DMA_MEM_TO_DEV, + DMA_PREP_INTERRUPT | DMA_CTRL_ACK); + if (!desc) { + dev_err(i2c->dev, + "Failed to get DMA data write descriptor.\n"); + goto write_init_dma_fail; + } + } + + /* + * The last descriptor must have this callback, + * to finish the DMA transaction. + */ + desc->callback = mxs_i2c_dma_irq_callback; + desc->callback_param = i2c; + + /* Start the transfer. */ + dmaengine_submit(desc); + dma_async_issue_pending(i2c->dmach); + return 0; + +/* Read failpath. */ +read_init_dma_fail: + dma_unmap_sg(i2c->dev, &i2c->sg_io[1], 1, DMA_FROM_DEVICE); +select_init_dma_fail: + dma_unmap_sg(i2c->dev, &i2c->sg_io[0], 1, DMA_TO_DEVICE); +select_init_pio_fail: + return -EINVAL; + +/* Write failpath. */ +write_init_dma_fail: + dma_unmap_sg(i2c->dev, i2c->sg_io, 2, DMA_TO_DEVICE); +write_init_pio_fail: + return -EINVAL; +} + /* * Low level master read/write transaction. */ @@ -258,6 +417,8 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int ret; int flags; + flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0; + dev_dbg(i2c->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n", msg->addr, msg->len, msg->flags, stop); @@ -267,23 +428,29 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, init_completion(&i2c->cmd_complete); i2c->cmd_err = 0; - flags = stop ? MXS_I2C_CTRL0_POST_SEND_STOP : 0; - - if (msg->flags & I2C_M_RD) - mxs_i2c_pioq_setup_read(i2c, msg->addr, msg->len, flags); - else - mxs_i2c_pioq_setup_write(i2c, msg->addr, msg->buf, msg->len, - flags); + if (i2c->dma_mode) { + ret = mxs_i2c_dma_setup_xfer(adap, msg, flags); + if (ret) + return ret; + } else { + if (msg->flags & I2C_M_RD) { + mxs_i2c_pioq_setup_read(i2c, msg->addr, + msg->len, flags); + } else { + mxs_i2c_pioq_setup_write(i2c, msg->addr, msg->buf, + msg->len, flags); + } - writel(MXS_I2C_QUEUECTRL_QUEUE_RUN, + writel(MXS_I2C_QUEUECTRL_QUEUE_RUN, i2c->regs + MXS_I2C_QUEUECTRL_SET); + } ret = wait_for_completion_timeout(&i2c->cmd_complete, msecs_to_jiffies(1000)); if (ret == 0) goto timeout; - if ((!i2c->cmd_err) && (msg->flags & I2C_M_RD)) { + if (!i2c->dma_mode && !i2c->cmd_err && (msg->flags & I2C_M_RD)) { ret = mxs_i2c_finish_read(i2c, msg->buf, msg->len); if (ret) goto timeout; @@ -301,6 +468,8 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, timeout: dev_dbg(i2c->dev, "Timeout!\n"); + if (i2c->dma_mode) + mxs_i2c_dma_finish(i2c); mxs_i2c_reset(i2c); return -ETIMEDOUT; } @@ -342,11 +511,13 @@ static irqreturn_t mxs_i2c_isr(int this_irq, void *dev_id) /* MXS_I2C_CTRL1_OVERSIZE_XFER_TERM_IRQ is only for slaves */ i2c->cmd_err = -EIO; - is_last_cmd = (readl(i2c->regs + MXS_I2C_QUEUESTAT) & - MXS_I2C_QUEUESTAT_WRITE_QUEUE_CNT_MASK) == 0; + if (!i2c->dma_mode) { + is_last_cmd = (readl(i2c->regs + MXS_I2C_QUEUESTAT) & + MXS_I2C_QUEUESTAT_WRITE_QUEUE_CNT_MASK) == 0; - if (is_last_cmd || i2c->cmd_err) - complete(&i2c->cmd_complete); + if (is_last_cmd || i2c->cmd_err) + complete(&i2c->cmd_complete); + } writel(stat, i2c->regs + MXS_I2C_CTRL1_CLR); @@ -358,6 +529,21 @@ static const struct i2c_algorithm mxs_i2c_algo = { .functionality = mxs_i2c_func, }; +static bool mxs_i2c_dma_filter(struct dma_chan *chan, void *param) +{ + struct mxs_i2c_dev *i2c = param; + + if (!mxs_dma_is_apbx(chan)) + return false; + + if (chan->chan_id != i2c->dma_channel) + return false; + + chan->private = &i2c->dma_data; + + return true; +} + static int mxs_i2c_get_ofdata(struct mxs_i2c_dev *i2c) { uint32_t speed; @@ -368,6 +554,28 @@ static int mxs_i2c_get_ofdata(struct mxs_i2c_dev *i2c) if (!node) return -EINVAL; + /* + * The MXS I2C DMA mode is prefered and enabled by default. + * The PIO mode is still supported, but should be used only + * for debuging purposes etc. + */ + i2c->dma_mode = 1; + if (of_find_property(node, "fsl,use-pio", NULL)) { + i2c->dma_mode = 0; + dev_info(dev, "Using PIO mode for I2C transfers!\n"); + } + + /* + * TODO: This is a temporary solution and should be changed + * to use generic DMA binding later when the helpers get in. + */ + ret = of_property_read_u32(node, "fsl,i2c-dma-channel", + &i2c->dma_channel); + if (ret) { + dev_warn(dev, "Failed to get DMA channel!\n"); + i2c->dma_mode = 0; + } + i2c->speed = &mxs_i2c_95kHz_config; ret = of_property_read_u32(node, "clock-frequency", &speed); if (ret) @@ -388,7 +596,8 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev) struct pinctrl *pinctrl; struct resource *res; resource_size_t res_size; - int err, irq; + int err, irq, dmairq; + dma_cap_mask_t mask; pinctrl = devm_pinctrl_get_select_default(dev); if (IS_ERR(pinctrl)) @@ -399,7 +608,10 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev) return -ENOMEM; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) + irq = platform_get_irq(pdev, 0); + dmairq = platform_get_irq(pdev, 1); + + if (!res || irq < 0 || dmairq < 0) return -ENOENT; res_size = resource_size(res); @@ -410,10 +622,6 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev) if (!i2c->regs) return -EBUSY; - irq = platform_get_irq(pdev, 0); - if (irq < 0) - return irq; - err = devm_request_irq(dev, irq, mxs_i2c_isr, 0, dev_name(dev), i2c); if (err) return err; @@ -424,6 +632,18 @@ static int __devinit mxs_i2c_probe(struct platform_device *pdev) if (err) return err; + /* Setup the DMA */ + if (i2c->dma_mode) { + dma_cap_zero(mask); + dma_cap_set(DMA_SLAVE, mask); + i2c->dma_data.chan_irq = dmairq; + i2c->dmach = dma_request_channel(mask, mxs_i2c_dma_filter, i2c); + if (!i2c->dmach) { + dev_err(dev, "Failed to request dma\n"); + return -ENODEV; + } + } + platform_set_drvdata(pdev, i2c); /* Do reset to enforce correct startup after pinmuxing */ @@ -459,6 +679,9 @@ static int __devexit mxs_i2c_remove(struct platform_device *pdev) if (ret) return -EBUSY; + if (i2c->dmach) + dma_release_channel(i2c->dmach); + writel(MXS_I2C_CTRL0_SFTRST, i2c->regs + MXS_I2C_CTRL0_SET); platform_set_drvdata(pdev, NULL);
This patch implements DMA support into mxs-i2c. DMA transfers are now enabled via DT. The DMA operation is enabled by default. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Detlev Zundel <dzu@denx.de> CC: Dong Aisheng <b29396@freescale.com> CC: Fabio Estevam <fabio.estevam@freescale.com> Cc: Linux ARM kernel <linux-arm-kernel@lists.infradead.org> Cc: linux-i2c@vger.kernel.org CC: Sascha Hauer <s.hauer@pengutronix.de> CC: Shawn Guo <shawn.guo@linaro.org> Cc: Stefano Babic <sbabic@denx.de> CC: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Cc: Wolfgang Denk <wd@denx.de> Cc: Wolfram Sang <w.sang@pengutronix.de> --- Documentation/devicetree/bindings/i2c/i2c-mxs.txt | 5 + arch/arm/boot/dts/imx28.dtsi | 2 + drivers/i2c/busses/i2c-mxs.c | 267 +++++++++++++++++++-- 3 files changed, 252 insertions(+), 22 deletions(-) V2: Fixed return value from mxs_i2c_dma_setup_xfer(). Fixed coding style nitpicks Call mxs_i2c_dma_finish() in failpath only if DMA is active V3: Align with changes in previous patch