[PATCH 1/2] spi: Add support for dma_min_bytes configuration.

[PATCH 1/2] spi: Add support for dma_min_bytes configuration.

[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&#174; 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

[PATCH 2/2] omap2_mcspi: Use dma_min_bytes parameter when it is configured.

[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&#174; 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

Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration.

[Grant Likely]

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&#174; 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

Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration.

[roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w]

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&#174; 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

Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration.

[Grant Likely]

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&#174; 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

Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration.

[roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w]

 
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&#174; 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

Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration.

[Grant Likely]

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&#174; 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

Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration.

[Juuso Oikarinen]

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&#174; 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

Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration.

[Grant Likely]

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&#174; 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

Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration.

[roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w]

 

>-----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&#174; 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

Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration.

[Juuso Oikarinen]

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&#174; 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

Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration.

[roman.tereshonkov-xNZwKgViW5gAvxtiuMwx3w]

 

>-----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&#174; 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

Re: [PATCH 1/2] spi: Add support for dma_min_bytes configuration.

[Grant Likely]

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&#174; 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