| Message ID | 1347356538-23835-1-git-send-email-shubhrajyoti@ti.com |
|---|---|
| State | New |
| Headers | show |
On Tuesday 11 September 2012 03:11 PM, Shubhrajyoti D wrote: > Changes since v1: > - removed tabification on patch 6/17 > - removed dev_err() which was introduced on patch 09/17 > Changes since v2: > - do not set full fifo depth in the RDR interrupt. > - some changelog updates. > - rebase to the Wolfram's tree. > Changes since v3: > - Remove a redundant read of status register > - Read the dev->buf_len variable instead of the register > as the information of the remaining bytes is there. > Changes since v4: > - Ack the arbitration lost. > - Rebase to the i2c-embedded/for-next branch. > Changes since v5: > - Rebase to latest mainline > - Added some more cleanup patches so as have a consolidated series. > Changes since v6: > - Fix comments on setting the pdev to NULL. > - Trivial changelog update > > Previous discussions can be found here > http://www.spinics.net/lists/linux-i2c/msg09482.html Also this gives better performance With the patches: Performance counter stats for '/build/i2c/bin/i2cdump -y -f 1 0x48 b': 78.796376 task-clock # 0.453 CPUs utilized 516 context-switches # 0.007 M/sec 0 CPU-migrations # 0.000 K/sec 114 page-faults # 0.001 M/sec 0.174011183 seconds time elapsed Without the patches Performance counter stats for '/build/i2c/bin/i2cdump -y -f 1 0x48 b': 123.504640 task-clock # 0.049 CPUs utilized 337 context-switches # 0.003 M/sec 0 CPU-migrations # 0.000 K/sec 144 page-faults # 0.001 M/sec 2.534424040 seconds time elapsed The auto suspend is probably the one :-) > This is the cleanup only series. > > Tested on omap4sdp and 3430sdp. > > The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217: > > Linux 3.6-rc5 (2012-09-08 16:43:45 -0700) > > are available in the git repository at: > git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanup > > > > Felipe Balbi (22): > i2c: omap: switch to devm_* API > i2c: omap: simplify num_bytes handling > i2c: omap: decrease indentation level on data handling > i2c: omap: add blank lines > i2c: omap: simplify omap_i2c_ack_stat() > i2c: omap: split out [XR]DR and [XR]RDY > i2c: omap: improve i462 errata handling > i2c: omap: re-factor receive/transmit data loop > i2c: omap: switch over to do {} while loop > i2c: omap: ack IRQ in parts > i2c: omap: switch to platform_get_irq() > i2c: omap: bus: add a receiver flag > i2c: omap: simplify errata check > i2c: omap: always return IRQ_HANDLED > i2c: omap: simplify IRQ exit path > i2c: omap: resize fifos before each message > i2c: omap: get rid of the "complete" label > i2c: omap: always return IRQ_HANDLED > i2c: omap: switch to threaded IRQ support > i2c: omap: remove unnecessary pm_runtime_suspended check > i2c: omap: switch over to autosuspend API > i2c: omap: sanitize exit path > > Shubhrajyoti D (1): > i2c: omap: remove redundant status read > > drivers/i2c/busses/i2c-omap.c | 442 +++++++++++++++++++++++++---------------- > 1 files changed, 271 insertions(+), 171 deletions(-) >
Hi, On Tue, Sep 11, 2012 at 03:48:34PM +0530, Shubhrajyoti wrote: > On Tuesday 11 September 2012 03:11 PM, Shubhrajyoti D wrote: > > Changes since v1: > > - removed tabification on patch 6/17 > > - removed dev_err() which was introduced on patch 09/17 > > Changes since v2: > > - do not set full fifo depth in the RDR interrupt. > > - some changelog updates. > > - rebase to the Wolfram's tree. > > Changes since v3: > > - Remove a redundant read of status register > > - Read the dev->buf_len variable instead of the register > > as the information of the remaining bytes is there. > > Changes since v4: > > - Ack the arbitration lost. > > - Rebase to the i2c-embedded/for-next branch. > > Changes since v5: > > - Rebase to latest mainline > > - Added some more cleanup patches so as have a consolidated series. > > Changes since v6: > > - Fix comments on setting the pdev to NULL. > > - Trivial changelog update > > > > Previous discussions can be found here > > http://www.spinics.net/lists/linux-i2c/msg09482.html > Also this gives better performance > With the patches: > > Performance counter stats for '/build/i2c/bin/i2cdump -y -f 1 0x48 > b': > > > 78.796376 task-clock # 0.453 CPUs > utilized > 516 context-switches # 0.007 > M/sec > 0 CPU-migrations # 0.000 > K/sec > 114 page-faults # 0.001 M/sec > > 0.174011183 seconds time elapsed > > Without the patches > Performance counter stats for '/build/i2c/bin/i2cdump -y -f 1 0x48 > b': > > > 123.504640 task-clock # 0.049 CPUs > utilized > 337 context-switches # 0.003 > M/sec > 0 CPU-migrations # 0.000 > K/sec > 144 page-faults # 0.001 > M/sec > > > > 2.534424040 seconds time elapsed > > The auto suspend is probably the one :-) Awesome, I didn't go as far as checking performance counters :-)
On Tue, Sep 11, 2012 at 03:12:15PM +0530, Shubhrajyoti D wrote: > From: Felipe Balbi <balbi@ti.com> > > for OMAP2, we can easily switch over to threaded > IRQs on the I2C driver. This will allow us to > spend less time in hardirq context. > > Signed-off-by: Felipe Balbi <balbi@ti.com> > [Trivial formating changes] > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > drivers/i2c/busses/i2c-omap.c | 43 +++++++++++++++++++++++++++++++++++----- > 1 files changed, 37 insertions(+), 6 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 4a696bd..e391370 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -176,6 +176,7 @@ enum { > #define I2C_OMAP_ERRATA_I462 (1 << 1) > > struct omap_i2c_dev { > + spinlock_t lock; /* IRQ synchronization */ > struct device *dev; > void __iomem *base; /* virtual */ > int irq; > @@ -854,9 +855,30 @@ static int omap_i2c_transmit_data(struct omap_i2c_dev *dev, u8 num_bytes, > } > > static irqreturn_t > -omap_i2c_isr(int this_irq, void *dev_id) > +omap_i2c_isr(int irq, void *dev_id) > { > struct omap_i2c_dev *dev = dev_id; > + irqreturn_t ret = IRQ_HANDLED; Shouldn't that be IRQ_NONE?
On Tue, Sep 11, 2012 at 03:12:14PM +0530, Shubhrajyoti D wrote: > From: Felipe Balbi <balbi@ti.com> > > even if our clocks are disabled, we still > handled the IRQ, so we should return IRQ_HANDLED. > > Signed-off-by: Felipe Balbi <balbi@ti.com> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> Can't we just drop this patch since the code gets removed soon anyhow? Or is patch 20/23 changing something I don't see yet?
On Tue, Sep 11, 2012 at 03:12:13PM +0530, Shubhrajyoti D wrote: > Remove the redundant read of the status register. Commit message is just repeating the subject. Why was the read there and why can it be removed now? > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > drivers/i2c/busses/i2c-omap.c | 2 -- > 1 files changed, 0 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 5d4bad4..498a462 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -946,7 +946,6 @@ omap_i2c_isr(int this_irq, void *dev_id) > num_bytes = dev->buf_len; > > ret = omap_i2c_transmit_data(dev, num_bytes, true); > - stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); > if (ret < 0) > goto out; > > @@ -962,7 +961,6 @@ omap_i2c_isr(int this_irq, void *dev_id) > num_bytes = dev->threshold; > > ret = omap_i2c_transmit_data(dev, num_bytes, false); > - stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); > if (ret < 0) > goto out; > > -- > 1.7.5.4 >
On Tue, Sep 11, 2012 at 03:11:55PM +0530, Shubhrajyoti D wrote: > Changes since v1: > - removed tabification on patch 6/17 > - removed dev_err() which was introduced on patch 09/17 > Changes since v2: > - do not set full fifo depth in the RDR interrupt. > - some changelog updates. > - rebase to the Wolfram's tree. > Changes since v3: > - Remove a redundant read of status register > - Read the dev->buf_len variable instead of the register > as the information of the remaining bytes is there. > Changes since v4: > - Ack the arbitration lost. > - Rebase to the i2c-embedded/for-next branch. > Changes since v5: > - Rebase to latest mainline > - Added some more cleanup patches so as have a consolidated series. > Changes since v6: > - Fix comments on setting the pdev to NULL. > - Trivial changelog update Looks mostly good, thanks. Only a few comments. Oh, and I still get reports about a section mismatch ;)
On Wednesday 12 September 2012 03:21 AM, Wolfram Sang wrote: >> -omap_i2c_isr(int this_irq, void *dev_id) >> > +omap_i2c_isr(int irq, void *dev_id) >> > { >> > struct omap_i2c_dev *dev = dev_id; >> > + irqreturn_t ret = IRQ_HANDLED; > Shouldn't that be IRQ_NONE? Actually we are processing it so I thought it to be ok. Also a similar discussion. http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/104422.html
On Wednesday 12 September 2012 03:23 AM, Wolfram Sang wrote: >> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > Can't we just drop this patch since the code gets removed soon anyhow? > Or is patch 20/23 changing something I don't see yet? yes it is dropped will update.
On Wednesday 12 September 2012 03:24 AM, Wolfram Sang wrote: > On Tue, Sep 11, 2012 at 03:12:13PM +0530, Shubhrajyoti D wrote: >> Remove the redundant read of the status register. > Commit message is just repeating the subject. > Why was the read there and why can it be removed now? > It is read and it is not read anywhere below. >> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> >> --- >> drivers/i2c/busses/i2c-omap.c | 2 -- >> 1 files changed, 0 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >> index 5d4bad4..498a462 100644 >> --- a/drivers/i2c/busses/i2c-omap.c >> +++ b/drivers/i2c/busses/i2c-omap.c >> @@ -946,7 +946,6 @@ omap_i2c_isr(int this_irq, void *dev_id) >> num_bytes = dev->buf_len; >> >> ret = omap_i2c_transmit_data(dev, num_bytes, true); >> - stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); >> if (ret < 0) >> goto out; >> >> @@ -962,7 +961,6 @@ omap_i2c_isr(int this_irq, void *dev_id) >> num_bytes = dev->threshold; >> >> ret = omap_i2c_transmit_data(dev, num_bytes, false); >> - stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); >> if (ret < 0) >> goto out; >> >> -- >> 1.7.5.4 >>
On Wednesday 12 September 2012 03:30 AM, Wolfram Sang wrote: >> Changes since v6: >> > - Fix comments on setting the pdev to NULL. >> > - Trivial changelog update > Looks mostly good, thanks. Only a few comments. Oh, and I still get > reports about a section mismatch ;) I am using omap2plus_defconfig $ make --ver GNU Make 3.81 Copyright (C) 2006 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. $ make CONFIG_DEBUG_SECTION_MISMATCH=y uImage .... LD vmlinux.o MODPOST vmlinux.o GEN .version CHK include/generated/compile.h UPD include/generated/compile.h CC init/version.o LD init/built-in.o KSYM .tmp_kallsyms1.o KSYM .tmp_kallsyms2.o LD vmlinux SYSMAP System.map OBJCOPY arch/arm/boot/Image Kernel: arch/arm/boot/Image is ready GZIP arch/arm/boot/compressed/piggy.gzip CC arch/arm/boot/compressed/misc.o CC arch/arm/boot/compressed/decompress.o CC arch/arm/boot/compressed/string.o AS arch/arm/boot/compressed/piggy.gzip.o LD arch/arm/boot/compressed/vmlinux OBJCOPY arch/arm/boot/zImage Kernel: arch/arm/boot/zImage is ready UIMAGE arch/arm/boot/uImage Image Name: Linux-3.6.0-rc5-00022-g49fb6db Created: Wed Sep 12 15:24:21 2012 Image Type: ARM Linux Kernel Image (uncompressed) Data Size: 4074688 Bytes = 3979.19 kB = 3.89 MB Load Address: 80008000 Entry Point: 80008000 Image arch/arm/boot/uImage is ready I donot see the warning. Am I missing something?
> I donot see the warning. Am I missing something?
I deleted my logfiles already. Ignore it for now, if it comes up again
with your new series, I will give a more detailed pointer.
Thanks,
Wolfram
On Wed, Sep 12, 2012 at 11:55:39AM +0530, Shubhrajyoti wrote: > On Wednesday 12 September 2012 03:21 AM, Wolfram Sang wrote: > >> -omap_i2c_isr(int this_irq, void *dev_id) > >> > +omap_i2c_isr(int irq, void *dev_id) > >> > { > >> > struct omap_i2c_dev *dev = dev_id; > >> > + irqreturn_t ret = IRQ_HANDLED; > > Shouldn't that be IRQ_NONE? > Actually we are processing it so I thought it to be ok. Are we processing it? There is nothing acknowledged AFAICS. Anyway, we can fix that with a later patch. > Also a similar discussion. > > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-June/104422.html I totally agree to the things said there.
On Wed, Sep 12, 2012 at 12:18:50PM +0200, Wolfram Sang wrote: > > > I donot see the warning. Am I missing something? > > I deleted my logfiles already. Ignore it for now, if it comes up again > with your new series, I will give a more detailed pointer. Sorry, the section mismatch was not related to I2C it seems: WARNING: vmlinux.o(.data+0x30958): Section mismatch in reference from the variable rx51_si4713_dev to the (unknown reference) .init.data:(unknown) The variable rx51_si4713_dev references the (unknown reference) __initdata (unknown) If the reference is valid then annotate the variable with __init* or __refdata (see linux/init.h) or name the variable: *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console Got it with an "allnoconfig" and then selecting MMU and OMAP. Regards, Wolfram
On Wed, Sep 12, 2012 at 7:14 PM, Wolfram Sang <w.sang@pengutronix.de> wrote: > On Wed, Sep 12, 2012 at 12:18:50PM +0200, Wolfram Sang wrote: >> >> > I donot see the warning. Am I missing something? >> >> I deleted my logfiles already. Ignore it for now, if it comes up again >> with your new series, I will give a more detailed pointer. > > Sorry, the section mismatch was not related to I2C it seems: Thanks for the report just sent a patch fixing that. > > WARNING: vmlinux.o(.data+0x30958): Section mismatch in reference from the variable rx51_si4713_dev to the (unknown reference) .init.data:(unknown) > The variable rx51_si4713_dev references > the (unknown reference) __initdata (unknown) > If the reference is valid then annotate the > variable with __init* or __refdata (see linux/init.h) or name the variable: > *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console > > Got it with an "allnoconfig" and then selecting MMU and OMAP. > > Regards, > > Wolfram > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ | > > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.10 (GNU/Linux) > > iEYEARECAAYFAlBQkbsACgkQD27XaX1/VRtbuACgkBa0lOIN551eec9TSetVPsCE > Ew0AoKizKon3DIILpERWJIwzAXdgRVDc > =T4Yq > -----END PGP SIGNATURE----- >
Changes since v1: - removed tabification on patch 6/17 - removed dev_err() which was introduced on patch 09/17 Changes since v2: - do not set full fifo depth in the RDR interrupt. - some changelog updates. - rebase to the Wolfram's tree. Changes since v3: - Remove a redundant read of status register - Read the dev->buf_len variable instead of the register as the information of the remaining bytes is there. Changes since v4: - Ack the arbitration lost. - Rebase to the i2c-embedded/for-next branch. Changes since v5: - Rebase to latest mainline - Added some more cleanup patches so as have a consolidated series. Changes since v6: - Fix comments on setting the pdev to NULL. - Trivial changelog update Previous discussions can be found here http://www.spinics.net/lists/linux-i2c/msg09482.html This is the cleanup only series. Tested on omap4sdp and 3430sdp. The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217: Linux 3.6-rc5 (2012-09-08 16:43:45 -0700) are available in the git repository at: git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanup Felipe Balbi (22): i2c: omap: switch to devm_* API i2c: omap: simplify num_bytes handling i2c: omap: decrease indentation level on data handling i2c: omap: add blank lines i2c: omap: simplify omap_i2c_ack_stat() i2c: omap: split out [XR]DR and [XR]RDY i2c: omap: improve i462 errata handling i2c: omap: re-factor receive/transmit data loop i2c: omap: switch over to do {} while loop i2c: omap: ack IRQ in parts i2c: omap: switch to platform_get_irq() i2c: omap: bus: add a receiver flag i2c: omap: simplify errata check i2c: omap: always return IRQ_HANDLED i2c: omap: simplify IRQ exit path i2c: omap: resize fifos before each message i2c: omap: get rid of the "complete" label i2c: omap: always return IRQ_HANDLED i2c: omap: switch to threaded IRQ support i2c: omap: remove unnecessary pm_runtime_suspended check i2c: omap: switch over to autosuspend API i2c: omap: sanitize exit path Shubhrajyoti D (1): i2c: omap: remove redundant status read drivers/i2c/busses/i2c-omap.c | 442 +++++++++++++++++++++++++---------------- 1 files changed, 271 insertions(+), 171 deletions(-)