| Message ID | 20190324175002.28969-9-kernel@martin.sperl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: bcm2835aux: bug fixes and improvements | expand |
Hi Martin, Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org: > From: Martin Sperl <kernel@martin.sperl.org> > > To estimate efficiency add statistics on transfer types > (polling and interrupt) used to debugfs. > > Signed-off-by: Martin Sperl <kernel@martin.sperl.org> > --- > drivers/spi/spi-bcm2835aux.c | 61 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c > index dd0446968da6..d2b58060b333 100644 > --- a/drivers/spi/spi-bcm2835aux.c > +++ b/drivers/spi/spi-bcm2835aux.c > @@ -21,6 +21,7 @@ > > #include <linux/clk.h> > #include <linux/completion.h> > +#include <linux/debugfs.h> > #include <linux/delay.h> > #include <linux/err.h> > #include <linux/interrupt.h> > @@ -102,8 +103,57 @@ struct bcm2835aux_spi { > int tx_len; > int rx_len; > int pending; > + > +#if defined(CONFIG_DEBUG_FS) > +#define BCM2835_AUX_INCR(field) ((field)++) > + u64 count_transfer_polling; > + u64 count_transfer_irq; > + u64 count_transfer_irq_after_poll; > + > + struct dentry *debugfs_dir; > +#else > +#define BCM2835_AUX_INCR(field) > +#endif /* CONFIG_DEBUG_FS */ > }; > is there a chance to avoid these ifdefs CONFIG_DEBUG_FS?
> On 25.03.2019, at 10:28, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > Hi Martin, > > Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org: >> + >> +#if defined(CONFIG_DEBUG_FS) >> +#define BCM2835_AUX_INCR(field) ((field)++) >> + u64 count_transfer_polling; >> + u64 count_transfer_irq; >> + u64 count_transfer_irq_after_poll; >> + >> + struct dentry *debugfs_dir; >> +#else >> +#define BCM2835_AUX_INCR(field) >> +#endif /* CONFIG_DEBUG_FS */ >> }; >> > is there a chance to avoid these ifdefs CONFIG_DEBUG_FS? > The idea was not to add those stats at all when debugfs is disabled. Other groups (e.g net/can) prefer it this way… I do not care either way - your preference... Martin
Am 25.03.19 um 10:52 schrieb kernel@martin.sperl.org: >> On 25.03.2019, at 10:28, Stefan Wahren <stefan.wahren@i2se.com> wrote: >> >> Hi Martin, >> >> Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org: >>> + >>> +#if defined(CONFIG_DEBUG_FS) >>> +#define BCM2835_AUX_INCR(field) ((field)++) >>> + u64 count_transfer_polling; >>> + u64 count_transfer_irq; >>> + u64 count_transfer_irq_after_poll; >>> + >>> + struct dentry *debugfs_dir; >>> +#else >>> +#define BCM2835_AUX_INCR(field) >>> +#endif /* CONFIG_DEBUG_FS */ >>> }; >>> >> is there a chance to avoid these ifdefs CONFIG_DEBUG_FS? >> > The idea was not to add those stats at all when debugfs is disabled. > Other groups (e.g net/can) prefer it this way… Yes, this is always a balance between size reduction and code readability. My biggest concern was about BCM2835_AUX_INCR
> On 25.03.2019, at 11:06, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > Am 25.03.19 um 10:52 schrieb kernel@martin.sperl.org: >>> On 25.03.2019, at 10:28, Stefan Wahren <stefan.wahren@i2se.com> wrote: >>> >>> Hi Martin, >>> >>> Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org: >>>> + >>>> +#if defined(CONFIG_DEBUG_FS) >>>> +#define BCM2835_AUX_INCR(field) ((field)++) >>>> + u64 count_transfer_polling; >>>> + u64 count_transfer_irq; >>>> + u64 count_transfer_irq_after_poll; >>>> + >>>> + struct dentry *debugfs_dir; >>>> +#else >>>> +#define BCM2835_AUX_INCR(field) >>>> +#endif /* CONFIG_DEBUG_FS */ >>>> }; >>>> >>> is there a chance to avoid these ifdefs CONFIG_DEBUG_FS? >>> >> The idea was not to add those stats at all when debugfs is disabled. >> Other groups (e.g net/can) prefer it this way… > > Yes, this is always a balance between size reduction and code readability. > > My biggest concern was about BCM2835_AUX_INCR > just tell me which way you prefer it - anyway: disabling DEBUG is quite hard to do in the first place (too many components select it automatically) so I am always wondering why I need to hop thtu those loops. If you agree I will remove those specific ifdefs and macros. Martin
Am 25.03.19 um 11:11 schrieb kernel@martin.sperl.org: >> On 25.03.2019, at 11:06, Stefan Wahren <stefan.wahren@i2se.com> wrote: >> >> Am 25.03.19 um 10:52 schrieb kernel@martin.sperl.org: >>>> On 25.03.2019, at 10:28, Stefan Wahren <stefan.wahren@i2se.com> wrote: >>>> >>>> Hi Martin, >>>> >>>> Am 24.03.19 um 18:50 schrieb kernel@martin.sperl.org: >>>>> + >>>>> +#if defined(CONFIG_DEBUG_FS) >>>>> +#define BCM2835_AUX_INCR(field) ((field)++) >>>>> + u64 count_transfer_polling; >>>>> + u64 count_transfer_irq; >>>>> + u64 count_transfer_irq_after_poll; >>>>> + >>>>> + struct dentry *debugfs_dir; >>>>> +#else >>>>> +#define BCM2835_AUX_INCR(field) >>>>> +#endif /* CONFIG_DEBUG_FS */ >>>>> }; >>>>> >>>> is there a chance to avoid these ifdefs CONFIG_DEBUG_FS? >>>> >>> The idea was not to add those stats at all when debugfs is disabled. >>> Other groups (e.g net/can) prefer it this way… >> Yes, this is always a balance between size reduction and code readability. >> >> My biggest concern was about BCM2835_AUX_INCR >> > just tell me which way you prefer it - anyway: disabling DEBUG is quite hard > to do in the first place (too many components select it automatically) > so I am always wondering why I need to hop thtu those loops. > > If you agree I will remove those specific ifdefs and macros. Please remove it for the quoted part above
diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c index dd0446968da6..d2b58060b333 100644 --- a/drivers/spi/spi-bcm2835aux.c +++ b/drivers/spi/spi-bcm2835aux.c @@ -21,6 +21,7 @@ #include <linux/clk.h> #include <linux/completion.h> +#include <linux/debugfs.h> #include <linux/delay.h> #include <linux/err.h> #include <linux/interrupt.h> @@ -102,8 +103,57 @@ struct bcm2835aux_spi { int tx_len; int rx_len; int pending; + +#if defined(CONFIG_DEBUG_FS) +#define BCM2835_AUX_INCR(field) ((field)++) + u64 count_transfer_polling; + u64 count_transfer_irq; + u64 count_transfer_irq_after_poll; + + struct dentry *debugfs_dir; +#else +#define BCM2835_AUX_INCR(field) +#endif /* CONFIG_DEBUG_FS */ }; +#if defined(CONFIG_DEBUG_FS) +static void bcm2835aux_debugfs_create(struct bcm2835aux_spi *bs, + const char *dname) +{ + char name[64]; + struct dentry *dir; + + /* get full name */ + snprintf(name, sizeof(name), "spi-bcm2835aux-%s", dname); + + /* the base directory */ + dir = debugfs_create_dir(name, NULL); + bs->debugfs_dir = dir; + + /* the counters */ + debugfs_create_u64("count_transfer_polling", 0444, dir, + &bs->count_transfer_polling); + debugfs_create_u64("count_transfer_irq", 0444, dir, + &bs->count_transfer_irq); + debugfs_create_u64("count_transfer_irq_after_poll", 0444, dir, + &bs->count_transfer_irq_after_poll); +} + +static void bcm2835aux_debugfs_remove(struct bcm2835aux_spi *bs) +{ + debugfs_remove_recursive(bs->debugfs_dir); + bs->debugfs_dir = NULL; +} +#else +static void bcm2835aux_debugfs_create(struct bcm2835aux_spi *bs) +{ +} + +static void bcm2835aux_debugfs_remove(struct bcm2835aux_spi *bs) +{ +} +#endif /* CONFIG_DEBUG_FS */ + static inline u32 bcm2835aux_rd(struct bcm2835aux_spi *bs, unsigned reg) { return readl(bs->regs + reg); @@ -242,6 +292,9 @@ static int bcm2835aux_spi_transfer_one_irq(struct spi_master *master, { struct bcm2835aux_spi *bs = spi_master_get_devdata(master); + /* update statistics */ + BCM2835_AUX_INCR(bs->count_transfer_irq); + /* fill in registers and fifos before enabling interrupts */ bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]); bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]); @@ -265,6 +318,9 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master, struct bcm2835aux_spi *bs = spi_master_get_devdata(master); unsigned long timeout; + /* update statistics */ + BCM2835_AUX_INCR(bs->count_transfer_polling); + /* configure spi */ bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL1, bs->cntl[1]); bcm2835aux_wr(bs, BCM2835_AUX_SPI_CNTL0, bs->cntl[0]); @@ -285,6 +341,7 @@ static int bcm2835aux_spi_transfer_one_poll(struct spi_master *master, jiffies - timeout, bs->tx_len, bs->rx_len); /* forward to interrupt handler */ + BCM2835_AUX_INCR(bs->count_transfer_irq_after_poll); return __bcm2835aux_spi_transfer_one_irq(master, spi, tfr); } @@ -531,6 +588,8 @@ static int bcm2835aux_spi_probe(struct platform_device *pdev) goto out_clk_disable; } + bcm2835aux_debugfs_create(bs, dev_name(&pdev->dev)); + return 0; out_clk_disable: @@ -545,6 +604,8 @@ static int bcm2835aux_spi_remove(struct platform_device *pdev) struct spi_master *master = platform_get_drvdata(pdev); struct bcm2835aux_spi *bs = spi_master_get_devdata(master); + bcm2835aux_debugfs_remove(bs); + bcm2835aux_spi_reset_hw(bs); /* disable the HW block by releasing the clock */