* [PATCH 1/2] spi: Add support for dma_min_bytes configuration. @ 2010-04-06 10:39 Roman Tereshonkov [not found] ` <1270550389-30392-1-git-send-email-roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> 0 siblings, 1 reply; 13+ messages in thread From: Roman Tereshonkov @ 2010-04-06 10:39 UTC (permalink / raw) To: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f; +Cc: Roman Tereshonkov This parameters defines the minimum number of bytes when dma is used. Signed-off-by: Roman Tereshonkov <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> --- drivers/spi/spi.c | 1 + include/linux/spi/spi.h | 3 +++ 2 files changed, 4 insertions(+), 0 deletions(-) diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index b76f246..5bf7992 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -347,6 +347,7 @@ struct spi_device *spi_new_device(struct spi_master *master, proxy->max_speed_hz = chip->max_speed_hz; proxy->mode = chip->mode; proxy->irq = chip->irq; + proxy->dma_min_bytes = chip->dma_min_bytes; strlcpy(proxy->modalias, chip->modalias, sizeof(proxy->modalias)); proxy->dev.platform_data = (void *) chip->platform_data; proxy->controller_data = chip->controller_data; diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 97b60b3..4e9961d 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -84,6 +84,7 @@ struct spi_device { #define SPI_NO_CS 0x40 /* 1 dev/bus, no chipselect */ #define SPI_READY 0x80 /* slave pulls low to pause */ u8 bits_per_word; + int dma_min_bytes; int irq; void *controller_state; void *controller_data; @@ -727,6 +728,8 @@ struct spi_board_info { */ u8 mode; + /* dma_min_bytes defines minimum bytes when dma is used */ + u32 dma_min_bytes; /* ... may need additional spi_device chip config data here. * avoid stuff protocol drivers can set; but include stuff * needed to behave without being bound to a driver: -- 1.6.2.rc1.3.g81d3f ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev ^ permalink raw reply related [flat|nested] 13+ messages in thread
[parent not found: <1270550389-30392-1-git-send-email-roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>]
* [PATCH 2/2] omap2_mcspi: Use dma_min_bytes parameter when it is configured. [not found] ` <1270550389-30392-1-git-send-email-roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> @ 2010-04-06 10:39 ` Roman Tereshonkov 2010-04-08 6:26 ` [PATCH 1/2] spi: Add support for dma_min_bytes configuration Grant Likely 1 sibling, 0 replies; 13+ messages in thread From: Roman Tereshonkov @ 2010-04-06 10:39 UTC (permalink / raw) To: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f; +Cc: Roman Tereshonkov When dma_min_bytes parameter is set as non-zero from device configuration it overlaps the global one DMA_MIN_BYTES. Signed-off-by: Roman Tereshonkov <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> --- drivers/spi/omap2_mcspi.c | 12 ++++++++++-- 1 files changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c index a9d58ce..1befdf8 100644 --- a/drivers/spi/omap2_mcspi.c +++ b/drivers/spi/omap2_mcspi.c @@ -793,6 +793,7 @@ static void omap2_mcspi_work(struct work_struct *work) int par_override = 0; int status = 0; u32 chconf; + u32 dma_min_bytes = DMA_MIN_BYTES; m = container_of(mcspi->msg_queue.next, struct spi_message, queue); @@ -803,6 +804,9 @@ static void omap2_mcspi_work(struct work_struct *work) spi = m->spi; cs = spi->controller_state; + if (spi->dma_min_bytes) + dma_min_bytes = spi->dma_min_bytes; + omap2_mcspi_set_enable(spi, 1); list_for_each_entry(t, &m->transfers, transfer_list) { if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) { @@ -839,7 +843,7 @@ static void omap2_mcspi_work(struct work_struct *work) __raw_writel(0, cs->base + OMAP2_MCSPI_TX0); - if (m->is_dma_mapped || t->len >= DMA_MIN_BYTES) + if (m->is_dma_mapped || t->len >= dma_min_bytes) count = omap2_mcspi_txrx_dma(spi, t); else count = omap2_mcspi_txrx_pio(spi, t); @@ -889,6 +893,7 @@ static int omap2_mcspi_transfer(struct spi_device *spi, struct spi_message *m) struct omap2_mcspi *mcspi; unsigned long flags; struct spi_transfer *t; + u32 dma_min_bytes; m->actual_length = 0; m->status = 0; @@ -896,6 +901,9 @@ static int omap2_mcspi_transfer(struct spi_device *spi, struct spi_message *m) /* reject invalid messages and transfers */ if (list_empty(&m->transfers) || !m->complete) return -EINVAL; + + dma_min_bytes = spi->dma_min_bytes ? spi->dma_min_bytes : DMA_MIN_BYTES; + list_for_each_entry(t, &m->transfers, transfer_list) { const void *tx_buf = t->tx_buf; void *rx_buf = t->rx_buf; @@ -921,7 +929,7 @@ static int omap2_mcspi_transfer(struct spi_device *spi, struct spi_message *m) return -EINVAL; } - if (m->is_dma_mapped || len < DMA_MIN_BYTES) + if (m->is_dma_mapped || len < dma_min_bytes) continue; /* Do DMA mapping "early" for better error reporting and -- 1.6.2.rc1.3.g81d3f ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration. [not found] ` <1270550389-30392-1-git-send-email-roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> 2010-04-06 10:39 ` [PATCH 2/2] omap2_mcspi: Use dma_min_bytes parameter when it is configured Roman Tereshonkov @ 2010-04-08 6:26 ` Grant Likely [not found] ` <z2rfa686aa41004072326z5ff8c044l46330b85dfd7ae74-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 1 sibling, 1 reply; 13+ messages in thread From: Grant Likely @ 2010-04-08 6:26 UTC (permalink / raw) To: Roman Tereshonkov; +Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f Hi Roman, On Tue, Apr 6, 2010 at 4:39 AM, Roman Tereshonkov <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: > This parameters defines the minimum number of bytes when dma is used. > > Signed-off-by: Roman Tereshonkov <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> The intent of this feature is... ? Your patch needs a better description as to why it is needed; especially considering that it changes common code. But, inferring from the code that is written, I can guess what you're trying to do, if not why. Why would spi_devices care what the minimum size of a DMA transfer is? The SPI bus driver is in a far better position to make that determination. Also, since this essentially adds a new 'knob' for spi_devices to twiddle, what is the expected behaviour for SPI bus controllers that don't support it? Is it a required feature for spi bus drivers to implement if they support DMA? Cheers, g. > --- > drivers/spi/spi.c | 1 + > include/linux/spi/spi.h | 3 +++ > 2 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index b76f246..5bf7992 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -347,6 +347,7 @@ struct spi_device *spi_new_device(struct spi_master *master, > proxy->max_speed_hz = chip->max_speed_hz; > proxy->mode = chip->mode; > proxy->irq = chip->irq; > + proxy->dma_min_bytes = chip->dma_min_bytes; > strlcpy(proxy->modalias, chip->modalias, sizeof(proxy->modalias)); > proxy->dev.platform_data = (void *) chip->platform_data; > proxy->controller_data = chip->controller_data; > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index 97b60b3..4e9961d 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -84,6 +84,7 @@ struct spi_device { > #define SPI_NO_CS 0x40 /* 1 dev/bus, no chipselect */ > #define SPI_READY 0x80 /* slave pulls low to pause */ > u8 bits_per_word; > + int dma_min_bytes; > int irq; > void *controller_state; > void *controller_data; > @@ -727,6 +728,8 @@ struct spi_board_info { > */ > u8 mode; > > + /* dma_min_bytes defines minimum bytes when dma is used */ > + u32 dma_min_bytes; > /* ... may need additional spi_device chip config data here. > * avoid stuff protocol drivers can set; but include stuff > * needed to behave without being bound to a driver: > -- > 1.6.2.rc1.3.g81d3f > > -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <z2rfa686aa41004072326z5ff8c044l46330b85dfd7ae74-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration. [not found] ` <z2rfa686aa41004072326z5ff8c044l46330b85dfd7ae74-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2010-04-08 10:33 ` roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w [not found] ` <E1C7579D1379824DAE67858071C8103828208BC339-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org> 0 siblings, 1 reply; 13+ messages in thread From: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w @ 2010-04-08 10:33 UTC (permalink / raw) To: grant.likely-s3s/WqlpOiPyB63q8FvJNQ Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f Hi, >-----Original Message----- >From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On >Behalf Of ext Grant Likely >Sent: 08 April, 2010 09:27 >To: Tereshonkov Roman (Nokia-D/Helsinki) >Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org >Subject: Re: [PATCH 1/2] spi: Add support for dma_min_bytes >configuration. > >Hi Roman, > >On Tue, Apr 6, 2010 at 4:39 AM, Roman Tereshonkov ><roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: >> This parameters defines the minimum number of bytes when dma is used. >> >> Signed-off-by: Roman Tereshonkov <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> > >The intent of this feature is... ? Your patch needs a better >description as to why it is needed; especially considering that it >changes common code. > >But, inferring from the code that is written, I can guess what you're >trying to do, if not why. Why would spi_devices care what the minimum >size of a DMA transfer is? The SPI bus driver is in a far better >position to make that determination. > >Also, since this essentially adds a new 'knob' for spi_devices to >twiddle, what is the expected behaviour for SPI bus controllers that >don't support it? Is it a required feature for spi bus drivers to >implement if they support DMA? The spi transactions can be handled in two ways: dma and pio. For the best perfomance the minimun number of bytes when dma is used can be found experimentaly and passed through the platform board config files. Now I will talk about omap2/3 spi only. If the mentioned parameter is not set then the static default one is used (as it is nowdays). This exludes the patch influence on other spi devices controlled by the same omap2/3 spi master. I think you might be right. The better way would be to pass it through the controller_data field of the spi_board_info. Then I do not need to touch spi.h and spi.c. I will create a new patch. But now I wonder about patches syncronization. The first patch should go to linux-omap tree as it is for arch/arm/plat-omap/include/plat/mcspi.h. The second patch applied after the first one is for driver/spi/omap2_mcspi.h. Or can you handle both patches? Regards Roman > >Cheers, >g. > >> --- >> drivers/spi/spi.c | 1 + >> include/linux/spi/spi.h | 3 +++ >> 2 files changed, 4 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c >> index b76f246..5bf7992 100644 >> --- a/drivers/spi/spi.c >> +++ b/drivers/spi/spi.c >> @@ -347,6 +347,7 @@ struct spi_device *spi_new_device(struct >spi_master *master, >> proxy->max_speed_hz = chip->max_speed_hz; >> proxy->mode = chip->mode; >> proxy->irq = chip->irq; >> + proxy->dma_min_bytes = chip->dma_min_bytes; >> strlcpy(proxy->modalias, chip->modalias, >sizeof(proxy->modalias)); >> proxy->dev.platform_data = (void *) chip->platform_data; >> proxy->controller_data = chip->controller_data; >> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h >> index 97b60b3..4e9961d 100644 >> --- a/include/linux/spi/spi.h >> +++ b/include/linux/spi/spi.h >> @@ -84,6 +84,7 @@ struct spi_device { >> #define SPI_NO_CS 0x40 /* 1 >dev/bus, no chipselect */ >> #define SPI_READY 0x80 /* >slave pulls low to pause */ >> u8 bits_per_word; >> + int dma_min_bytes; >> int irq; >> void *controller_state; >> void *controller_data; >> @@ -727,6 +728,8 @@ struct spi_board_info { >> */ >> u8 mode; >> >> + /* dma_min_bytes defines minimum bytes when dma is used */ >> + u32 dma_min_bytes; >> /* ... may need additional spi_device chip config data here. >> * avoid stuff protocol drivers can set; but include stuff >> * needed to behave without being bound to a driver: >> -- >> 1.6.2.rc1.3.g81d3f >> >> > > > >-- >Grant Likely, B.Sc., P.Eng. >Secret Lab Technologies Ltd. > ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <E1C7579D1379824DAE67858071C8103828208BC339-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>]
* Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration. [not found] ` <E1C7579D1379824DAE67858071C8103828208BC339-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org> @ 2010-04-08 15:08 ` Grant Likely [not found] ` <s2vfa686aa41004080808o443dd6b7ge4d23c513a1f80a8-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 13+ messages in thread From: Grant Likely @ 2010-04-08 15:08 UTC (permalink / raw) To: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Samuel Ortiz On Thu, Apr 8, 2010 at 4:33 AM, <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: > > Hi, > > >>-----Original Message----- >>From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On >>Behalf Of ext Grant Likely >>Sent: 08 April, 2010 09:27 >>To: Tereshonkov Roman (Nokia-D/Helsinki) >>Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org >>Subject: Re: [PATCH 1/2] spi: Add support for dma_min_bytes >>configuration. >> >>Hi Roman, >> >>On Tue, Apr 6, 2010 at 4:39 AM, Roman Tereshonkov >><roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: >>> This parameters defines the minimum number of bytes when dma is used. >>> >>> Signed-off-by: Roman Tereshonkov <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> >> >>The intent of this feature is... ? Your patch needs a better >>description as to why it is needed; especially considering that it >>changes common code. >> >>But, inferring from the code that is written, I can guess what you're >>trying to do, if not why. Why would spi_devices care what the minimum >>size of a DMA transfer is? The SPI bus driver is in a far better >>position to make that determination. >> >>Also, since this essentially adds a new 'knob' for spi_devices to >>twiddle, what is the expected behaviour for SPI bus controllers that >>don't support it? Is it a required feature for spi bus drivers to >>implement if they support DMA? > > The spi transactions can be handled in two ways: dma and pio. > For the best perfomance the minimun number of bytes when dma is used > can be found experimentaly and passed through the platform board config files. > > Now I will talk about omap2/3 spi only. > If the mentioned parameter is not set then the static default one > is used (as it is nowdays). > This exludes the patch influence on other spi devices controlled > by the same omap2/3 spi master. > > I think you might be right. The better way would be to pass it > through the controller_data field of the spi_board_info. > Then I do not need to touch spi.h and spi.c. The question that must be asked, will this new optimization option actually be profiled for most boards? Should it really be a board-specific parameter? Or even an SoC specific parameter? Or to be even more specific, do *you* have two different boards that need a different value for the minimum dma bytes? If the answer is no, then I recommend profiling your platform and floating out a patch that changes the default value to what you find is best. If there is no opposition to the value you choose, then there is no need to make it a tunable until someone else comes along who needs it to be something different. The current value of DMA_MIN_BYTES hasn't changed since the driver was first submitted. I suspect that it has not been optimized. What does Samuel have to say about it? g. > > I will create a new patch. > But now I wonder about patches syncronization. > The first patch should go to linux-omap tree as it is for arch/arm/plat-omap/include/plat/mcspi.h. > The second patch applied after the first one is for driver/spi/omap2_mcspi.h. > Or can you handle both patches? > > > Regards > Roman ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <s2vfa686aa41004080808o443dd6b7ge4d23c513a1f80a8-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration. [not found] ` <s2vfa686aa41004080808o443dd6b7ge4d23c513a1f80a8-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2010-04-08 15:25 ` roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w [not found] ` <E1C7579D1379824DAE67858071C8103828208BC539-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org> 0 siblings, 1 reply; 13+ messages in thread From: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w @ 2010-04-08 15:25 UTC (permalink / raw) To: grant.likely-s3s/WqlpOiPyB63q8FvJNQ Cc: juuso.oikarinen-xNZwKgViW5gAvxtiuMwx3w, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, samuel.ortiz-EmnPodGKVbzby3iVrkZq2A The others from wlan team need DMA_MIN_BYTES to be adjustable. Juuso can comment this. Just changing DMA_MIN_BYTES might influence on other spi devices controlled by omap2/3 spi controller driver. Roman >-----Original Message----- >From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On >Behalf Of ext Grant Likely >Sent: 08 April, 2010 18:09 >To: Tereshonkov Roman (Nokia-D/Helsinki) >Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Samuel Ortiz >Subject: Re: [PATCH 1/2] spi: Add support for dma_min_bytes >configuration. > >On Thu, Apr 8, 2010 at 4:33 AM, <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: >> >> Hi, >> >> >>>-----Original Message----- >>>From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On >>>Behalf Of ext Grant Likely >>>Sent: 08 April, 2010 09:27 >>>To: Tereshonkov Roman (Nokia-D/Helsinki) >>>Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org >>>Subject: Re: [PATCH 1/2] spi: Add support for dma_min_bytes >>>configuration. >>> >>>Hi Roman, >>> >>>On Tue, Apr 6, 2010 at 4:39 AM, Roman Tereshonkov >>><roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: >>>> This parameters defines the minimum number of bytes when >dma is used. >>>> >>>> Signed-off-by: Roman Tereshonkov <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> >>> >>>The intent of this feature is... ? Your patch needs a better >>>description as to why it is needed; especially considering that it >>>changes common code. >>> >>>But, inferring from the code that is written, I can guess what you're >>>trying to do, if not why. Why would spi_devices care what >the minimum >>>size of a DMA transfer is? The SPI bus driver is in a far better >>>position to make that determination. >>> >>>Also, since this essentially adds a new 'knob' for spi_devices to >>>twiddle, what is the expected behaviour for SPI bus controllers that >>>don't support it? Is it a required feature for spi bus drivers to >>>implement if they support DMA? >> >> The spi transactions can be handled in two ways: dma and pio. >> For the best perfomance the minimun number of bytes when dma is used >> can be found experimentaly and passed through the platform >board config files. >> >> Now I will talk about omap2/3 spi only. >> If the mentioned parameter is not set then the static default one >> is used (as it is nowdays). >> This exludes the patch influence on other spi devices controlled >> by the same omap2/3 spi master. >> >> I think you might be right. The better way would be to pass it >> through the controller_data field of the spi_board_info. >> Then I do not need to touch spi.h and spi.c. > >The question that must be asked, will this new optimization option >actually be profiled for most boards? Should it really be a >board-specific parameter? Or even an SoC specific parameter? Or to >be even more specific, do *you* have two different boards that need a >different value for the minimum dma bytes? > >If the answer is no, then I recommend profiling your platform and >floating out a patch that changes the default value to what you find >is best. If there is no opposition to the value you choose, then >there is no need to make it a tunable until someone else comes along >who needs it to be something different. > >The current value of DMA_MIN_BYTES hasn't changed since the driver was >first submitted. I suspect that it has not been optimized. What does >Samuel have to say about it? > >g. > >> >> I will create a new patch. >> But now I wonder about patches syncronization. >> The first patch should go to linux-omap tree as it is for >arch/arm/plat-omap/include/plat/mcspi.h. >> The second patch applied after the first one is for >driver/spi/omap2_mcspi.h. >> Or can you handle both patches? >> >> >> Regards >> Roman > ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <E1C7579D1379824DAE67858071C8103828208BC539-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>]
* Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration. [not found] ` <E1C7579D1379824DAE67858071C8103828208BC539-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org> @ 2010-04-08 16:27 ` Grant Likely 2010-04-09 4:05 ` Juuso Oikarinen 1 sibling, 0 replies; 13+ messages in thread From: Grant Likely @ 2010-04-08 16:27 UTC (permalink / raw) To: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w Cc: juuso.oikarinen-xNZwKgViW5gAvxtiuMwx3w, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, samuel.ortiz-EmnPodGKVbzby3iVrkZq2A On Thu, Apr 8, 2010 at 9:25 AM, <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: > > The others from wlan team need DMA_MIN_BYTES to be adjustable. > Juuso can comment this. Fair enough. I'm happy to either take both parts of the patch through the spi tree, or for the changes to go in via the omap tree. There is no sense in splitting them up. g. ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration. [not found] ` <E1C7579D1379824DAE67858071C8103828208BC539-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org> 2010-04-08 16:27 ` Grant Likely @ 2010-04-09 4:05 ` Juuso Oikarinen [not found] ` <1270785939.10120.1141.camel-gZysiALljqf+W+uuys5jPoH6Mc4MB0Vx@public.gmane.org> 1 sibling, 1 reply; 13+ messages in thread From: Juuso Oikarinen @ 2010-04-09 4:05 UTC (permalink / raw) To: Tereshonkov Roman (Nokia-D/Helsinki) Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Samuel Ortiz Hi, The wl1271 driver does not actually require this value to be adjustable - i.e. the driver does not need to know the value. In fact, the wl1271 does not know about DMA at all, instead the SPI controller driver makes the choice on how to perform the transfer most optimally. So we - or the wl1271 driver - just need the selection between DMA and PIO to be optimal in point of view of throughput and CPU load. The old threshold value of 8 is far from optimal - something like 160-200 is in the correct range. AFAIK performing the SPI transfer optimally should not affect other SPI clients in a negative way, and AFAIK there are no other SPI clients in Dali than the wl1271, so I guess this new threshold value could also be hard-coded into the omap2_mcspi driver. -Juuso On Thu, 2010-04-08 at 17:25 +0200, Tereshonkov Roman (Nokia-D/Helsinki) wrote: > The others from wlan team need DMA_MIN_BYTES to be adjustable. > Juuso can comment this. > > Just changing DMA_MIN_BYTES might influence on other spi devices > controlled by omap2/3 spi controller driver. > > Roman > > >-----Original Message----- > >From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On > >Behalf Of ext Grant Likely > >Sent: 08 April, 2010 18:09 > >To: Tereshonkov Roman (Nokia-D/Helsinki) > >Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Samuel Ortiz > >Subject: Re: [PATCH 1/2] spi: Add support for dma_min_bytes > >configuration. > > > >On Thu, Apr 8, 2010 at 4:33 AM, <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: > >> > >> Hi, > >> > >> > >>>-----Original Message----- > >>>From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On > >>>Behalf Of ext Grant Likely > >>>Sent: 08 April, 2010 09:27 > >>>To: Tereshonkov Roman (Nokia-D/Helsinki) > >>>Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org > >>>Subject: Re: [PATCH 1/2] spi: Add support for dma_min_bytes > >>>configuration. > >>> > >>>Hi Roman, > >>> > >>>On Tue, Apr 6, 2010 at 4:39 AM, Roman Tereshonkov > >>><roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: > >>>> This parameters defines the minimum number of bytes when > >dma is used. > >>>> > >>>> Signed-off-by: Roman Tereshonkov <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> > >>> > >>>The intent of this feature is... ? Your patch needs a better > >>>description as to why it is needed; especially considering that it > >>>changes common code. > >>> > >>>But, inferring from the code that is written, I can guess what you're > >>>trying to do, if not why. Why would spi_devices care what > >the minimum > >>>size of a DMA transfer is? The SPI bus driver is in a far better > >>>position to make that determination. > >>> > >>>Also, since this essentially adds a new 'knob' for spi_devices to > >>>twiddle, what is the expected behaviour for SPI bus controllers that > >>>don't support it? Is it a required feature for spi bus drivers to > >>>implement if they support DMA? > >> > >> The spi transactions can be handled in two ways: dma and pio. > >> For the best perfomance the minimun number of bytes when dma is used > >> can be found experimentaly and passed through the platform > >board config files. > >> > >> Now I will talk about omap2/3 spi only. > >> If the mentioned parameter is not set then the static default one > >> is used (as it is nowdays). > >> This exludes the patch influence on other spi devices controlled > >> by the same omap2/3 spi master. > >> > >> I think you might be right. The better way would be to pass it > >> through the controller_data field of the spi_board_info. > >> Then I do not need to touch spi.h and spi.c. > > > >The question that must be asked, will this new optimization option > >actually be profiled for most boards? Should it really be a > >board-specific parameter? Or even an SoC specific parameter? Or to > >be even more specific, do *you* have two different boards that need a > >different value for the minimum dma bytes? > > > >If the answer is no, then I recommend profiling your platform and > >floating out a patch that changes the default value to what you find > >is best. If there is no opposition to the value you choose, then > >there is no need to make it a tunable until someone else comes along > >who needs it to be something different. > > > >The current value of DMA_MIN_BYTES hasn't changed since the driver was > >first submitted. I suspect that it has not been optimized. What does > >Samuel have to say about it? > > > >g. > > > >> > >> I will create a new patch. > >> But now I wonder about patches syncronization. > >> The first patch should go to linux-omap tree as it is for > >arch/arm/plat-omap/include/plat/mcspi.h. > >> The second patch applied after the first one is for > >driver/spi/omap2_mcspi.h. > >> Or can you handle both patches? > >> > >> > >> Regards > >> Roman > > ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <1270785939.10120.1141.camel-gZysiALljqf+W+uuys5jPoH6Mc4MB0Vx@public.gmane.org>]
* Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration. [not found] ` <1270785939.10120.1141.camel-gZysiALljqf+W+uuys5jPoH6Mc4MB0Vx@public.gmane.org> @ 2010-04-09 4:56 ` Grant Likely 2010-04-09 7:49 ` roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w 1 sibling, 0 replies; 13+ messages in thread From: Grant Likely @ 2010-04-09 4:56 UTC (permalink / raw) To: Juuso Oikarinen Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Tereshonkov Roman (Nokia-D/Helsinki), Samuel Ortiz On Thu, Apr 8, 2010 at 10:05 PM, Juuso Oikarinen <juuso.oikarinen-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: > AFAIK performing the SPI transfer optimally should not affect other SPI > clients in a negative way, and AFAIK there are no other SPI clients in > Dali than the wl1271, so I guess this new threshold value could also be > hard-coded into the omap2_mcspi driver. Okay, good. Then I recommend preparing and posting a patch to change the default value. Include the rational behind the change, how you came up with the final number, and be sure to cc: the original developer of that driver. g. ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration. [not found] ` <1270785939.10120.1141.camel-gZysiALljqf+W+uuys5jPoH6Mc4MB0Vx@public.gmane.org> 2010-04-09 4:56 ` Grant Likely @ 2010-04-09 7:49 ` roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w [not found] ` <E1C7579D1379824DAE67858071C8103828208BC90A-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org> 1 sibling, 1 reply; 13+ messages in thread From: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w @ 2010-04-09 7:49 UTC (permalink / raw) To: juuso.oikarinen-xNZwKgViW5gAvxtiuMwx3w Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, samuel.ortiz-EmnPodGKVbzby3iVrkZq2A >-----Original Message----- >From: Oikarinen Juuso (Nokia-D/Tampere) >Sent: 09 April, 2010 07:06 >To: Tereshonkov Roman (Nokia-D/Helsinki) >Cc: ext Grant Likely; spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; >Samuel Ortiz >Subject: RE: [PATCH 1/2] spi: Add support for dma_min_bytes >configuration. > >Hi, > >The wl1271 driver does not actually require this value to be adjustable >- i.e. the driver does not need to know the value. In fact, the wl1271 >does not know about DMA at all, instead the SPI controller driver makes >the choice on how to perform the transfer most optimally. > >So we - or the wl1271 driver - just need the selection between DMA and >PIO to be optimal in point of view of throughput and CPU load. The old >threshold value of 8 is far from optimal - something like 160-200 is in >the correct range. > >AFAIK performing the SPI transfer optimally should not affect other SPI >clients in a negative way, and AFAIK there are no other SPI clients in >Dali than the wl1271, so I guess this new threshold value could also be >hard-coded into the omap2_mcspi driver. What about omap2 based projects which use the same driver? Roman > >-Juuso > >On Thu, 2010-04-08 at 17:25 +0200, Tereshonkov Roman (Nokia-D/Helsinki) >wrote: >> The others from wlan team need DMA_MIN_BYTES to be adjustable. >> Juuso can comment this. >> >> Just changing DMA_MIN_BYTES might influence on other spi devices >> controlled by omap2/3 spi controller driver. >> >> Roman >> >> >-----Original Message----- >> >From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On >> >Behalf Of ext Grant Likely >> >Sent: 08 April, 2010 18:09 >> >To: Tereshonkov Roman (Nokia-D/Helsinki) >> >Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Samuel Ortiz >> >Subject: Re: [PATCH 1/2] spi: Add support for dma_min_bytes >> >configuration. >> > >> >On Thu, Apr 8, 2010 at 4:33 AM, ><roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: >> >> >> >> Hi, >> >> >> >> >> >>>-----Original Message----- >> >>>From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On >> >>>Behalf Of ext Grant Likely >> >>>Sent: 08 April, 2010 09:27 >> >>>To: Tereshonkov Roman (Nokia-D/Helsinki) >> >>>Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org >> >>>Subject: Re: [PATCH 1/2] spi: Add support for dma_min_bytes >> >>>configuration. >> >>> >> >>>Hi Roman, >> >>> >> >>>On Tue, Apr 6, 2010 at 4:39 AM, Roman Tereshonkov >> >>><roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: >> >>>> This parameters defines the minimum number of bytes when >> >dma is used. >> >>>> >> >>>> Signed-off-by: Roman Tereshonkov <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> >> >>> >> >>>The intent of this feature is... ? Your patch needs a better >> >>>description as to why it is needed; especially considering that it >> >>>changes common code. >> >>> >> >>>But, inferring from the code that is written, I can guess >what you're >> >>>trying to do, if not why. Why would spi_devices care what >> >the minimum >> >>>size of a DMA transfer is? The SPI bus driver is in a far better >> >>>position to make that determination. >> >>> >> >>>Also, since this essentially adds a new 'knob' for spi_devices to >> >>>twiddle, what is the expected behaviour for SPI bus >controllers that >> >>>don't support it? Is it a required feature for spi bus drivers to >> >>>implement if they support DMA? >> >> >> >> The spi transactions can be handled in two ways: dma and pio. >> >> For the best perfomance the minimun number of bytes when >dma is used >> >> can be found experimentaly and passed through the platform >> >board config files. >> >> >> >> Now I will talk about omap2/3 spi only. >> >> If the mentioned parameter is not set then the static default one >> >> is used (as it is nowdays). >> >> This exludes the patch influence on other spi devices controlled >> >> by the same omap2/3 spi master. >> >> >> >> I think you might be right. The better way would be to pass it >> >> through the controller_data field of the spi_board_info. >> >> Then I do not need to touch spi.h and spi.c. >> > >> >The question that must be asked, will this new optimization option >> >actually be profiled for most boards? Should it really be a >> >board-specific parameter? Or even an SoC specific parameter? Or to >> >be even more specific, do *you* have two different boards >that need a >> >different value for the minimum dma bytes? >> > >> >If the answer is no, then I recommend profiling your platform and >> >floating out a patch that changes the default value to what you find >> >is best. If there is no opposition to the value you choose, then >> >there is no need to make it a tunable until someone else comes along >> >who needs it to be something different. >> > >> >The current value of DMA_MIN_BYTES hasn't changed since the >driver was >> >first submitted. I suspect that it has not been optimized. > What does >> >Samuel have to say about it? >> > >> >g. >> > >> >> >> >> I will create a new patch. >> >> But now I wonder about patches syncronization. >> >> The first patch should go to linux-omap tree as it is for >> >arch/arm/plat-omap/include/plat/mcspi.h. >> >> The second patch applied after the first one is for >> >driver/spi/omap2_mcspi.h. >> >> Or can you handle both patches? >> >> >> >> >> >> Regards >> >> Roman >> > > > > ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <E1C7579D1379824DAE67858071C8103828208BC90A-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>]
* Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration. [not found] ` <E1C7579D1379824DAE67858071C8103828208BC90A-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org> @ 2010-04-09 7:52 ` Juuso Oikarinen [not found] ` <1270799541.10120.1146.camel-gZysiALljqf+W+uuys5jPoH6Mc4MB0Vx@public.gmane.org> 0 siblings, 1 reply; 13+ messages in thread From: Juuso Oikarinen @ 2010-04-09 7:52 UTC (permalink / raw) To: Tereshonkov Roman (Nokia-D/Helsinki) Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Samuel Ortiz On Fri, 2010-04-09 at 09:49 +0200, Tereshonkov Roman (Nokia-D/Helsinki) wrote: > > >-----Original Message----- > >From: Oikarinen Juuso (Nokia-D/Tampere) > >Sent: 09 April, 2010 07:06 > >To: Tereshonkov Roman (Nokia-D/Helsinki) > >Cc: ext Grant Likely; spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; > >Samuel Ortiz > >Subject: RE: [PATCH 1/2] spi: Add support for dma_min_bytes > >configuration. > > > >Hi, > > > >The wl1271 driver does not actually require this value to be adjustable > >- i.e. the driver does not need to know the value. In fact, the wl1271 > >does not know about DMA at all, instead the SPI controller driver makes > >the choice on how to perform the transfer most optimally. > > > >So we - or the wl1271 driver - just need the selection between DMA and > >PIO to be optimal in point of view of throughput and CPU load. The old > >threshold value of 8 is far from optimal - something like 160-200 is in > >the correct range. > > > >AFAIK performing the SPI transfer optimally should not affect other SPI > >clients in a negative way, and AFAIK there are no other SPI clients in > >Dali than the wl1271, so I guess this new threshold value could also be > >hard-coded into the omap2_mcspi driver. > > > > What about omap2 based projects which use the same driver? > The threshold, I think, is ultimately something that depends on how long it takes to arm DMA. The longer it takes to do that, the larger the optimal threshold. Is it conceivable, that the time it takes to arm the DMA in those potential platforms varies? If it does, the threshold should be configurable. -Juuso > > > Roman > > > > > >-Juuso > > > >On Thu, 2010-04-08 at 17:25 +0200, Tereshonkov Roman (Nokia-D/Helsinki) > >wrote: > >> The others from wlan team need DMA_MIN_BYTES to be adjustable. > >> Juuso can comment this. > >> > >> Just changing DMA_MIN_BYTES might influence on other spi devices > >> controlled by omap2/3 spi controller driver. > >> > >> Roman > >> > >> >-----Original Message----- > >> >From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On > >> >Behalf Of ext Grant Likely > >> >Sent: 08 April, 2010 18:09 > >> >To: Tereshonkov Roman (Nokia-D/Helsinki) > >> >Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Samuel Ortiz > >> >Subject: Re: [PATCH 1/2] spi: Add support for dma_min_bytes > >> >configuration. > >> > > >> >On Thu, Apr 8, 2010 at 4:33 AM, > ><roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: > >> >> > >> >> Hi, > >> >> > >> >> > >> >>>-----Original Message----- > >> >>>From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On > >> >>>Behalf Of ext Grant Likely > >> >>>Sent: 08 April, 2010 09:27 > >> >>>To: Tereshonkov Roman (Nokia-D/Helsinki) > >> >>>Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org > >> >>>Subject: Re: [PATCH 1/2] spi: Add support for dma_min_bytes > >> >>>configuration. > >> >>> > >> >>>Hi Roman, > >> >>> > >> >>>On Tue, Apr 6, 2010 at 4:39 AM, Roman Tereshonkov > >> >>><roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: > >> >>>> This parameters defines the minimum number of bytes when > >> >dma is used. > >> >>>> > >> >>>> Signed-off-by: Roman Tereshonkov <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> > >> >>> > >> >>>The intent of this feature is... ? Your patch needs a better > >> >>>description as to why it is needed; especially considering that it > >> >>>changes common code. > >> >>> > >> >>>But, inferring from the code that is written, I can guess > >what you're > >> >>>trying to do, if not why. Why would spi_devices care what > >> >the minimum > >> >>>size of a DMA transfer is? The SPI bus driver is in a far better > >> >>>position to make that determination. > >> >>> > >> >>>Also, since this essentially adds a new 'knob' for spi_devices to > >> >>>twiddle, what is the expected behaviour for SPI bus > >controllers that > >> >>>don't support it? Is it a required feature for spi bus drivers to > >> >>>implement if they support DMA? > >> >> > >> >> The spi transactions can be handled in two ways: dma and pio. > >> >> For the best perfomance the minimun number of bytes when > >dma is used > >> >> can be found experimentaly and passed through the platform > >> >board config files. > >> >> > >> >> Now I will talk about omap2/3 spi only. > >> >> If the mentioned parameter is not set then the static default one > >> >> is used (as it is nowdays). > >> >> This exludes the patch influence on other spi devices controlled > >> >> by the same omap2/3 spi master. > >> >> > >> >> I think you might be right. The better way would be to pass it > >> >> through the controller_data field of the spi_board_info. > >> >> Then I do not need to touch spi.h and spi.c. > >> > > >> >The question that must be asked, will this new optimization option > >> >actually be profiled for most boards? Should it really be a > >> >board-specific parameter? Or even an SoC specific parameter? Or to > >> >be even more specific, do *you* have two different boards > >that need a > >> >different value for the minimum dma bytes? > >> > > >> >If the answer is no, then I recommend profiling your platform and > >> >floating out a patch that changes the default value to what you find > >> >is best. If there is no opposition to the value you choose, then > >> >there is no need to make it a tunable until someone else comes along > >> >who needs it to be something different. > >> > > >> >The current value of DMA_MIN_BYTES hasn't changed since the > >driver was > >> >first submitted. I suspect that it has not been optimized. > > What does > >> >Samuel have to say about it? > >> > > >> >g. > >> > > >> >> > >> >> I will create a new patch. > >> >> But now I wonder about patches syncronization. > >> >> The first patch should go to linux-omap tree as it is for > >> >arch/arm/plat-omap/include/plat/mcspi.h. > >> >> The second patch applied after the first one is for > >> >driver/spi/omap2_mcspi.h. > >> >> Or can you handle both patches? > >> >> > >> >> > >> >> Regards > >> >> Roman > >> > > > > > > > ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <1270799541.10120.1146.camel-gZysiALljqf+W+uuys5jPoH6Mc4MB0Vx@public.gmane.org>]
* Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration. [not found] ` <1270799541.10120.1146.camel-gZysiALljqf+W+uuys5jPoH6Mc4MB0Vx@public.gmane.org> @ 2010-04-09 8:38 ` roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w [not found] ` <E1C7579D1379824DAE67858071C8103828208BC96F-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org> 0 siblings, 1 reply; 13+ messages in thread From: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w @ 2010-04-09 8:38 UTC (permalink / raw) To: juuso.oikarinen-xNZwKgViW5gAvxtiuMwx3w Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, samuel.ortiz-EmnPodGKVbzby3iVrkZq2A >-----Original Message----- >From: Oikarinen Juuso (Nokia-D/Tampere) >Sent: 09 April, 2010 10:52 >To: Tereshonkov Roman (Nokia-D/Helsinki) >Cc: ext Grant Likely; spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; >Samuel Ortiz >Subject: RE: [PATCH 1/2] spi: Add support for dma_min_bytes >configuration. > >On Fri, 2010-04-09 at 09:49 +0200, Tereshonkov Roman (Nokia-D/Helsinki) >wrote: >> >> >-----Original Message----- >> >From: Oikarinen Juuso (Nokia-D/Tampere) >> >Sent: 09 April, 2010 07:06 >> >To: Tereshonkov Roman (Nokia-D/Helsinki) >> >Cc: ext Grant Likely; spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; >> >Samuel Ortiz >> >Subject: RE: [PATCH 1/2] spi: Add support for dma_min_bytes >> >configuration. >> > >> >Hi, >> > >> >The wl1271 driver does not actually require this value to >be adjustable >> >- i.e. the driver does not need to know the value. In fact, >the wl1271 >> >does not know about DMA at all, instead the SPI controller >driver makes >> >the choice on how to perform the transfer most optimally. >> > >> >So we - or the wl1271 driver - just need the selection >between DMA and >> >PIO to be optimal in point of view of throughput and CPU >load. The old >> >threshold value of 8 is far from optimal - something like >160-200 is in >> >the correct range. >> > >> >AFAIK performing the SPI transfer optimally should not >affect other SPI >> >clients in a negative way, and AFAIK there are no other SPI >clients in >> >Dali than the wl1271, so I guess this new threshold value >could also be >> >hard-coded into the omap2_mcspi driver. >> >> >> >> What about omap2 based projects which use the same driver? >> > >The threshold, I think, is ultimately something that depends >on how long >it takes to arm DMA. The longer it takes to do that, the larger the >optimal threshold. > >Is it conceivable, that the time it takes to arm the DMA in those >potential platforms varies? If it does, the threshold should be >configurable. There are wl1251 and wl1271 spi clients in omap3. And there are two omap2 spi clients, touchscreen ads7846 and wlan p54spi, found in arch/arm/mach-omap2. All of them use now the default DMA_MIN_BYTES set to 8. When we change this value for our needs it might influence also on all of them. Having the same spi clock but different cpu clock for omap2 and omap3 I suspect that optimal DMA_MIN_VALUE found for omap3 will be different for omap2. Roman > >-Juuso > >> >> >> Roman >> >> >> > >> >-Juuso >> > >> >On Thu, 2010-04-08 at 17:25 +0200, Tereshonkov Roman >(Nokia-D/Helsinki) >> >wrote: >> >> The others from wlan team need DMA_MIN_BYTES to be adjustable. >> >> Juuso can comment this. >> >> >> >> Just changing DMA_MIN_BYTES might influence on other spi devices >> >> controlled by omap2/3 spi controller driver. >> >> >> >> Roman >> >> >> >> >-----Original Message----- >> >> >From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On >> >> >Behalf Of ext Grant Likely >> >> >Sent: 08 April, 2010 18:09 >> >> >To: Tereshonkov Roman (Nokia-D/Helsinki) >> >> >Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Samuel Ortiz >> >> >Subject: Re: [PATCH 1/2] spi: Add support for dma_min_bytes >> >> >configuration. >> >> > >> >> >On Thu, Apr 8, 2010 at 4:33 AM, >> ><roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: >> >> >> >> >> >> Hi, >> >> >> >> >> >> >> >> >>>-----Original Message----- >> >> >>>From: glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On >> >> >>>Behalf Of ext Grant Likely >> >> >>>Sent: 08 April, 2010 09:27 >> >> >>>To: Tereshonkov Roman (Nokia-D/Helsinki) >> >> >>>Cc: spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org >> >> >>>Subject: Re: [PATCH 1/2] spi: Add support for dma_min_bytes >> >> >>>configuration. >> >> >>> >> >> >>>Hi Roman, >> >> >>> >> >> >>>On Tue, Apr 6, 2010 at 4:39 AM, Roman Tereshonkov >> >> >>><roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: >> >> >>>> This parameters defines the minimum number of bytes when >> >> >dma is used. >> >> >>>> >> >> >>>> Signed-off-by: Roman Tereshonkov ><roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> >> >> >>> >> >> >>>The intent of this feature is... ? Your patch needs a better >> >> >>>description as to why it is needed; especially >considering that it >> >> >>>changes common code. >> >> >>> >> >> >>>But, inferring from the code that is written, I can guess >> >what you're >> >> >>>trying to do, if not why. Why would spi_devices care what >> >> >the minimum >> >> >>>size of a DMA transfer is? The SPI bus driver is in a >far better >> >> >>>position to make that determination. >> >> >>> >> >> >>>Also, since this essentially adds a new 'knob' for >spi_devices to >> >> >>>twiddle, what is the expected behaviour for SPI bus >> >controllers that >> >> >>>don't support it? Is it a required feature for spi >bus drivers to >> >> >>>implement if they support DMA? >> >> >> >> >> >> The spi transactions can be handled in two ways: dma and pio. >> >> >> For the best perfomance the minimun number of bytes when >> >dma is used >> >> >> can be found experimentaly and passed through the platform >> >> >board config files. >> >> >> >> >> >> Now I will talk about omap2/3 spi only. >> >> >> If the mentioned parameter is not set then the static >default one >> >> >> is used (as it is nowdays). >> >> >> This exludes the patch influence on other spi devices >controlled >> >> >> by the same omap2/3 spi master. >> >> >> >> >> >> I think you might be right. The better way would be to pass it >> >> >> through the controller_data field of the spi_board_info. >> >> >> Then I do not need to touch spi.h and spi.c. >> >> > >> >> >The question that must be asked, will this new >optimization option >> >> >actually be profiled for most boards? Should it really be a >> >> >board-specific parameter? Or even an SoC specific >parameter? Or to >> >> >be even more specific, do *you* have two different boards >> >that need a >> >> >different value for the minimum dma bytes? >> >> > >> >> >If the answer is no, then I recommend profiling your platform and >> >> >floating out a patch that changes the default value to >what you find >> >> >is best. If there is no opposition to the value you choose, then >> >> >there is no need to make it a tunable until someone else >comes along >> >> >who needs it to be something different. >> >> > >> >> >The current value of DMA_MIN_BYTES hasn't changed since the >> >driver was >> >> >first submitted. I suspect that it has not been optimized. >> > What does >> >> >Samuel have to say about it? >> >> > >> >> >g. >> >> > >> >> >> >> >> >> I will create a new patch. >> >> >> But now I wonder about patches syncronization. >> >> >> The first patch should go to linux-omap tree as it is for >> >> >arch/arm/plat-omap/include/plat/mcspi.h. >> >> >> The second patch applied after the first one is for >> >> >driver/spi/omap2_mcspi.h. >> >> >> Or can you handle both patches? >> >> >> >> >> >> >> >> >> Regards >> >> >> Roman >> >> > >> > >> > >> > > > > ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <E1C7579D1379824DAE67858071C8103828208BC96F-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org>]
* Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration. [not found] ` <E1C7579D1379824DAE67858071C8103828208BC96F-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org> @ 2010-04-09 14:52 ` Grant Likely 0 siblings, 0 replies; 13+ messages in thread From: Grant Likely @ 2010-04-09 14:52 UTC (permalink / raw) To: roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w Cc: juuso.oikarinen-xNZwKgViW5gAvxtiuMwx3w, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, samuel.ortiz-EmnPodGKVbzby3iVrkZq2A On Fri, Apr 9, 2010 at 2:38 AM, <roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> wrote: > > >>-----Original Message----- >>From: Oikarinen Juuso (Nokia-D/Tampere) >>Sent: 09 April, 2010 10:52 >>To: Tereshonkov Roman (Nokia-D/Helsinki) >>Cc: ext Grant Likely; spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; >>Samuel Ortiz >>Subject: RE: [PATCH 1/2] spi: Add support for dma_min_bytes >>configuration. >> >>On Fri, 2010-04-09 at 09:49 +0200, Tereshonkov Roman (Nokia-D/Helsinki) >>wrote: >>> >>> >-----Original Message----- >>> >From: Oikarinen Juuso (Nokia-D/Tampere) >>> >Sent: 09 April, 2010 07:06 >>> >To: Tereshonkov Roman (Nokia-D/Helsinki) >>> >Cc: ext Grant Likely; spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; >>> >Samuel Ortiz >>> >Subject: RE: [PATCH 1/2] spi: Add support for dma_min_bytes >>> >configuration. >>> > >>> >Hi, >>> > >>> >The wl1271 driver does not actually require this value to >>be adjustable >>> >- i.e. the driver does not need to know the value. In fact, >>the wl1271 >>> >does not know about DMA at all, instead the SPI controller >>driver makes >>> >the choice on how to perform the transfer most optimally. >>> > >>> >So we - or the wl1271 driver - just need the selection >>between DMA and >>> >PIO to be optimal in point of view of throughput and CPU >>load. The old >>> >threshold value of 8 is far from optimal - something like >>160-200 is in >>> >the correct range. >>> > >>> >AFAIK performing the SPI transfer optimally should not >>affect other SPI >>> >clients in a negative way, and AFAIK there are no other SPI >>clients in >>> >Dali than the wl1271, so I guess this new threshold value >>could also be >>> >hard-coded into the omap2_mcspi driver. >>> >>> >>> >>> What about omap2 based projects which use the same driver? >>> >> >>The threshold, I think, is ultimately something that depends >>on how long >>it takes to arm DMA. The longer it takes to do that, the larger the >>optimal threshold. >> >>Is it conceivable, that the time it takes to arm the DMA in those >>potential platforms varies? If it does, the threshold should be >>configurable. > > There are wl1251 and wl1271 spi clients in omap3. And there are two > omap2 spi clients, touchscreen ads7846 and wlan p54spi, found in > arch/arm/mach-omap2. All of them use now the default DMA_MIN_BYTES > set to 8. When we change this value for our needs it might influence > also on all of them. Yup. Change the default, post the patch, and solicit omap2 users for feedback. > Having the same spi clock but different cpu clock for omap2 and omap3 > I suspect that optimal DMA_MIN_VALUE found for omap3 > will be different for omap2. Most likely, but I would hold off merging a new tunable parameter until someone actually takes on the work of tuning for omap2 vs. omap3. No sense adding a dynamic tunable if it never gets used. g. ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-04-09 14:52 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-04-06 10:39 [PATCH 1/2] spi: Add support for dma_min_bytes configuration Roman Tereshonkov [not found] ` <1270550389-30392-1-git-send-email-roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org> 2010-04-06 10:39 ` [PATCH 2/2] omap2_mcspi: Use dma_min_bytes parameter when it is configured Roman Tereshonkov 2010-04-08 6:26 ` [PATCH 1/2] spi: Add support for dma_min_bytes configuration Grant Likely [not found] ` <z2rfa686aa41004072326z5ff8c044l46330b85dfd7ae74-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2010-04-08 10:33 ` roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w [not found] ` <E1C7579D1379824DAE67858071C8103828208BC339-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org> 2010-04-08 15:08 ` Grant Likely [not found] ` <s2vfa686aa41004080808o443dd6b7ge4d23c513a1f80a8-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2010-04-08 15:25 ` roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w [not found] ` <E1C7579D1379824DAE67858071C8103828208BC539-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org> 2010-04-08 16:27 ` Grant Likely 2010-04-09 4:05 ` Juuso Oikarinen [not found] ` <1270785939.10120.1141.camel-gZysiALljqf+W+uuys5jPoH6Mc4MB0Vx@public.gmane.org> 2010-04-09 4:56 ` Grant Likely 2010-04-09 7:49 ` roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w [not found] ` <E1C7579D1379824DAE67858071C8103828208BC90A-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org> 2010-04-09 7:52 ` Juuso Oikarinen [not found] ` <1270799541.10120.1146.camel-gZysiALljqf+W+uuys5jPoH6Mc4MB0Vx@public.gmane.org> 2010-04-09 8:38 ` roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w [not found] ` <E1C7579D1379824DAE67858071C8103828208BC96F-xJW1crHCIS+8kqYwC468Frtp2NbXvJi8gfoxzgwHRXE@public.gmane.org> 2010-04-09 14:52 ` Grant Likely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).