Linux-Renesas-SoC Archive on lore.kernel.org
 help / Atom feed
* [PATCH 1/2] spi: sh-msiof: fix *info pointer in request_dma()
@ 2019-01-11  8:58 Nguyen An Hoan
  2019-01-11  8:58 ` [PATCH 2/2] spi: sh-msiof: Use DMA if possible Nguyen An Hoan
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Nguyen An Hoan @ 2019-01-11  8:58 UTC (permalink / raw)
  To: linux-renesas-soc, broonie, geert+renesas
  Cc: magnus.damm, linux-spi, kuninori.morimoto.gx,
	yoshihiro.shimoda.uh, h-inayoshi, nv-dung, na-hoan, cv-dong

From: Hoan Nguyen An <na-hoan@jinso.co.jp>

msiof_spi_infor struct pointer was initialized in the probe() function,
no need to get back and keep consistency.

Signed-off-by: Hoan Nguyen An <na-hoan@jinso.co.jp>
---
 drivers/spi/spi-sh-msiof.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index d14b407..64adf34 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -1206,7 +1206,7 @@ static int sh_msiof_request_dma(struct sh_msiof_spi_priv *p)
 {
 	struct platform_device *pdev = p->pdev;
 	struct device *dev = &pdev->dev;
-	const struct sh_msiof_spi_info *info = dev_get_platdata(dev);
+	const struct sh_msiof_spi_info *info = p->info;
 	unsigned int dma_tx_id, dma_rx_id;
 	const struct resource *res;
 	struct spi_master *master;
@@ -1469,3 +1469,4 @@ MODULE_DESCRIPTION("SuperH MSIOF SPI Master Interface Driver");
 MODULE_AUTHOR("Magnus Damm");
 MODULE_LICENSE("GPL v2");
 MODULE_ALIAS("platform:spi_sh_msiof");
+
-- 
2.7.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] spi: sh-msiof: Use DMA if possible
  2019-01-11  8:58 [PATCH 1/2] spi: sh-msiof: fix *info pointer in request_dma() Nguyen An Hoan
@ 2019-01-11  8:58 ` Nguyen An Hoan
  2019-01-11  9:09   ` Geert Uytterhoeven
  2019-01-11  9:01 ` [PATCH 1/2] spi: sh-msiof: fix *info pointer in request_dma() Geert Uytterhoeven
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Nguyen An Hoan @ 2019-01-11  8:58 UTC (permalink / raw)
  To: linux-renesas-soc, broonie, geert+renesas
  Cc: magnus.damm, linux-spi, kuninori.morimoto.gx,
	yoshihiro.shimoda.uh, h-inayoshi, nv-dung, na-hoan, cv-dong

From: Hoan Nguyen An <na-hoan@jinso.co.jp>

Currently, this driver only supports feature for DMA 32-bits.
In this case, only if the data length is divisible by 4 to use
DMA, otherwise PIO will be used. This patch will suggest use
the DMA 32-bits with 4bytes of words, then the remaining data
will be transmitted by PIO mode.

Signed-off-by: Hoan Nguyen An <na-hoan@jinso.co.jp>
---
 drivers/spi/spi-sh-msiof.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
index 64adf34..241cc6e 100644
--- a/drivers/spi/spi-sh-msiof.c
+++ b/drivers/spi/spi-sh-msiof.c
@@ -937,17 +937,13 @@ static int sh_msiof_transfer_one(struct spi_master *master,
 		unsigned int l = 0;
 
 		if (tx_buf)
-			l = min(len, p->tx_fifo_size * 4);
+			l = min(len - len % 4, p->tx_fifo_size * 4);
 		if (rx_buf)
-			l = min(len, p->rx_fifo_size * 4);
+			l = min(len - len % 4, p->rx_fifo_size * 4);
 
 		if (bits <= 8) {
-			if (l & 3)
-				break;
 			copy32 = copy_bswap32;
 		} else if (bits <= 16) {
-			if (l & 3)
-				break;
 			copy32 = copy_wswap32;
 		} else {
 			copy32 = copy_plain32;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] spi: sh-msiof: fix *info pointer in request_dma()
  2019-01-11  8:58 [PATCH 1/2] spi: sh-msiof: fix *info pointer in request_dma() Nguyen An Hoan
  2019-01-11  8:58 ` [PATCH 2/2] spi: sh-msiof: Use DMA if possible Nguyen An Hoan
@ 2019-01-11  9:01 ` Geert Uytterhoeven
  2019-01-11 10:15 ` Sergei Shtylyov
  2019-01-11 13:26 ` Mark Brown
  3 siblings, 0 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2019-01-11  9:01 UTC (permalink / raw)
  To: Nguyen An Hoan
  Cc: Linux-Renesas, Mark Brown, Geert Uytterhoeven, Magnus Damm,
	linux-spi, Kuninori Morimoto, Yoshihiro Shimoda, 稲吉,
	Dung:人ソ,
	カオ・ヴァン・ドン

Hi Hoan-san,

On Fri, Jan 11, 2019 at 9:59 AM Nguyen An Hoan <na-hoan@jinso.co.jp> wrote:
> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
>
> msiof_spi_infor struct pointer was initialized in the probe() function,
> no need to get back and keep consistency.
>
> Signed-off-by: Hoan Nguyen An <na-hoan@jinso.co.jp>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
One nit below.

> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -1206,7 +1206,7 @@ static int sh_msiof_request_dma(struct sh_msiof_spi_priv *p)
>  {
>         struct platform_device *pdev = p->pdev;
>         struct device *dev = &pdev->dev;
> -       const struct sh_msiof_spi_info *info = dev_get_platdata(dev);
> +       const struct sh_msiof_spi_info *info = p->info;
>         unsigned int dma_tx_id, dma_rx_id;
>         const struct resource *res;
>         struct spi_master *master;
> @@ -1469,3 +1469,4 @@ MODULE_DESCRIPTION("SuperH MSIOF SPI Master Interface Driver");
>  MODULE_AUTHOR("Magnus Damm");
>  MODULE_LICENSE("GPL v2");
>  MODULE_ALIAS("platform:spi_sh_msiof");
> +

This change is unrelated, please drop it.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] spi: sh-msiof: Use DMA if possible
  2019-01-11  8:58 ` [PATCH 2/2] spi: sh-msiof: Use DMA if possible Nguyen An Hoan
@ 2019-01-11  9:09   ` Geert Uytterhoeven
  2019-01-11  9:27     ` Hoan
  0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2019-01-11  9:09 UTC (permalink / raw)
  To: Nguyen An Hoan
  Cc: Linux-Renesas, Mark Brown, Geert Uytterhoeven, Magnus Damm,
	linux-spi, Kuninori Morimoto, Yoshihiro Shimoda, 稲吉,
	Dung:人ソ,
	カオ・ヴァン・ドン

Hi Hoan-san,

On Fri, Jan 11, 2019 at 9:59 AM Nguyen An Hoan <na-hoan@jinso.co.jp> wrote:
> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
>
> Currently, this driver only supports feature for DMA 32-bits.
> In this case, only if the data length is divisible by 4 to use
> DMA, otherwise PIO will be used. This patch will suggest use
> the DMA 32-bits with 4bytes of words, then the remaining data
> will be transmitted by PIO mode.
>
> Signed-off-by: Hoan Nguyen An <na-hoan@jinso.co.jp>

Thanks for your patch, that's a great idea!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
with a small suggestion below.

> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
> @@ -937,17 +937,13 @@ static int sh_msiof_transfer_one(struct spi_master *master,
>                 unsigned int l = 0;
>
>                 if (tx_buf)
> -                       l = min(len, p->tx_fifo_size * 4);
> +                       l = min(len - len % 4, p->tx_fifo_size * 4);

I think this would be easier to read when written as:

    l = min(round_down(len, 4), p->tx_fifo_size * 4);

>                 if (rx_buf)
> -                       l = min(len, p->rx_fifo_size * 4);
> +                       l = min(len - len % 4, p->rx_fifo_size * 4);

Likewise,

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] spi: sh-msiof: Use DMA if possible
  2019-01-11  9:09   ` Geert Uytterhoeven
@ 2019-01-11  9:27     ` Hoan
  2019-01-11 10:05       ` Geert Uytterhoeven
  0 siblings, 1 reply; 9+ messages in thread
From: Hoan @ 2019-01-11  9:27 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linux-Renesas, Mark Brown, Geert Uytterhoeven, Magnus Damm,
	linux-spi, Kuninori Morimoto, Yoshihiro Shimoda, 稲吉,
	Dung:人ソ,
	カオ・ヴァン・ドン

Dear Geert-san

Thank you very much for your comments!

On 2019/01/11 18:09, Geert Uytterhoeven wrote:
> Hi Hoan-san,
>
> On Fri, Jan 11, 2019 at 9:59 AM Nguyen An Hoan <na-hoan@jinso.co.jp> wrote:
>> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
>>
>> Currently, this driver only supports feature for DMA 32-bits.
>> In this case, only if the data length is divisible by 4 to use
>> DMA, otherwise PIO will be used. This patch will suggest use
>> the DMA 32-bits with 4bytes of words, then the remaining data
>> will be transmitted by PIO mode.
>>
>> Signed-off-by: Hoan Nguyen An <na-hoan@jinso.co.jp>
> Thanks for your patch, that's a great idea!
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> with a small suggestion below.
Currently, I only test the transmit-receive spi with AT25C eeprom,
because the maximum length of data I test with eeprom is only 18 bytes,
so with this patch, there will be times when clock timing time and pulse
of data transmission (I Observed by  sigrock logic analyzer) will not be
beautiful compared to PIO mode.
I worry if other devices using this driver also limit the maximum length
of transmission/reception to 18bytes.

But I still want to patch up to ask for your opinion.


>> --- a/drivers/spi/spi-sh-msiof.c
>> +++ b/drivers/spi/spi-sh-msiof.c
>> @@ -937,17 +937,13 @@ static int sh_msiof_transfer_one(struct spi_master *master,
>>                  unsigned int l = 0;
>>
>>                  if (tx_buf)
>> -                       l = min(len, p->tx_fifo_size * 4);
>> +                       l = min(len - len % 4, p->tx_fifo_size * 4);
> I think this would be easier to read when written as:
>
>      l = min(round_down(len, 4), p->tx_fifo_size * 4);
>
If you do not have any problems with the above,
I will update as you want here.


Thank you for your helps!

Hoan.

>>                  if (rx_buf)
>> -                       l = min(len, p->rx_fifo_size * 4);
>> +                       l = min(len - len % 4, p->rx_fifo_size * 4);
> Likewise,
>
> Gr{oetje,eeting}s,
>
>                          Geert
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] spi: sh-msiof: Use DMA if possible
  2019-01-11  9:27     ` Hoan
@ 2019-01-11 10:05       ` Geert Uytterhoeven
  2019-01-18  8:19         ` Hoan
  0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2019-01-11 10:05 UTC (permalink / raw)
  To: Hoan
  Cc: Linux-Renesas, Mark Brown, Geert Uytterhoeven, Magnus Damm,
	linux-spi, Kuninori Morimoto, Yoshihiro Shimoda, 稲吉,
	Dung:人ソ,
	カオ・ヴァン・ドン

Hi Hoan,

On Fri, Jan 11, 2019 at 10:27 AM Hoan <na-hoan@jinso.co.jp> wrote:
> On 2019/01/11 18:09, Geert Uytterhoeven wrote:
> > On Fri, Jan 11, 2019 at 9:59 AM Nguyen An Hoan <na-hoan@jinso.co.jp> wrote:
> >> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
> >>
> >> Currently, this driver only supports feature for DMA 32-bits.
> >> In this case, only if the data length is divisible by 4 to use
> >> DMA, otherwise PIO will be used. This patch will suggest use
> >> the DMA 32-bits with 4bytes of words, then the remaining data
> >> will be transmitted by PIO mode.
> >>
> >> Signed-off-by: Hoan Nguyen An <na-hoan@jinso.co.jp>
> > Thanks for your patch, that's a great idea!
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > with a small suggestion below.
> Currently, I only test the transmit-receive spi with AT25C eeprom,
> because the maximum length of data I test with eeprom is only 18 bytes,
> so with this patch, there will be times when clock timing time and pulse
> of data transmission (I Observed by  sigrock logic analyzer) will not be
> beautiful compared to PIO mode.
> I worry if other devices using this driver also limit the maximum length
> of transmission/reception to 18bytes.

One other issue is that MSIOF drops the hardware chip select in between
transfers.  Before, transfers that fully fit in the FIFO were sent in a
single transfer, keeping the hardware chip select asserted during the
full transfer.
If transfers are split, and you need the chip select to be asserted
during the full transfer, you have to use a GPIO chip select instead.
I believe the split can already happen since your commit 916d9802e4b0723c
("spi: sh-msiof: Reduce the number of times write to and perform the
transmission from FIFO")?

However, for most SPI slave devices (e.g. AT25), that issue is moot, as
they need a GPIO chip select anyway, to avoid dropping the chip select
in between transfers belonging to the same message.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] spi: sh-msiof: fix *info pointer in request_dma()
  2019-01-11  8:58 [PATCH 1/2] spi: sh-msiof: fix *info pointer in request_dma() Nguyen An Hoan
  2019-01-11  8:58 ` [PATCH 2/2] spi: sh-msiof: Use DMA if possible Nguyen An Hoan
  2019-01-11  9:01 ` [PATCH 1/2] spi: sh-msiof: fix *info pointer in request_dma() Geert Uytterhoeven
@ 2019-01-11 10:15 ` Sergei Shtylyov
  2019-01-11 13:26 ` Mark Brown
  3 siblings, 0 replies; 9+ messages in thread
From: Sergei Shtylyov @ 2019-01-11 10:15 UTC (permalink / raw)
  To: Nguyen An Hoan, linux-renesas-soc, broonie, geert+renesas
  Cc: magnus.damm, linux-spi, kuninori.morimoto.gx,
	yoshihiro.shimoda.uh, h-inayoshi, nv-dung, cv-dong

Hello!

On 01/11/2019 11:58 AM, Nguyen An Hoan wrote:

> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
> 
> msiof_spi_infor struct pointer was initialized in the probe() function,

   infor?

> no need to get back and keep consistency.
> 
> Signed-off-by: Hoan Nguyen An <na-hoan@jinso.co.jp>
> ---
>  drivers/spi/spi-sh-msiof.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c
> index d14b407..64adf34 100644
> --- a/drivers/spi/spi-sh-msiof.c
> +++ b/drivers/spi/spi-sh-msiof.c
[...]
> @@ -1469,3 +1469,4 @@ MODULE_DESCRIPTION("SuperH MSIOF SPI Master Interface Driver");
>  MODULE_AUTHOR("Magnus Damm");
>  MODULE_LICENSE("GPL v2");
>  MODULE_ALIAS("platform:spi_sh_msiof");
> +
> 

   Huh?

MBR, Sergei

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] spi: sh-msiof: fix *info pointer in request_dma()
  2019-01-11  8:58 [PATCH 1/2] spi: sh-msiof: fix *info pointer in request_dma() Nguyen An Hoan
                   ` (2 preceding siblings ...)
  2019-01-11 10:15 ` Sergei Shtylyov
@ 2019-01-11 13:26 ` Mark Brown
  3 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2019-01-11 13:26 UTC (permalink / raw)
  To: Nguyen An Hoan
  Cc: linux-renesas-soc, geert+renesas, magnus.damm, linux-spi,
	kuninori.morimoto.gx, yoshihiro.shimoda.uh, h-inayoshi, nv-dung,
	cv-dong

[-- Attachment #1: Type: text/plain, Size: 307 bytes --]

On Fri, Jan 11, 2019 at 05:58:28PM +0900, Nguyen An Hoan wrote:
> From: Hoan Nguyen An <na-hoan@jinso.co.jp>
> 
> msiof_spi_infor struct pointer was initialized in the probe() function,
> no need to get back and keep consistency.

These look good to me with the changes Geert and Sergei pointed out.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] spi: sh-msiof: Use DMA if possible
  2019-01-11 10:05       ` Geert Uytterhoeven
@ 2019-01-18  8:19         ` Hoan
  0 siblings, 0 replies; 9+ messages in thread
From: Hoan @ 2019-01-18  8:19 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Linux-Renesas, Mark Brown, Geert Uytterhoeven, Magnus Damm,
	linux-spi, Kuninori Morimoto, Yoshihiro Shimoda, 稲吉,
	Dung:人ソ,
	カオ・ヴァン・ドン

Dear Geert-san

Thanks you for your comments!

On 2019/01/11 19:05, Geert Uytterhoeven wrote:
> Hi Hoan,
>
> On Fri, Jan 11, 2019 at 10:27 AM Hoan <na-hoan@jinso.co.jp> wrote:
>> ....
>> Currently, I only test the transmit-receive spi with AT25C eeprom,
>> because the maximum length of data I test with eeprom is only 18 bytes,
>> so with this patch, there will be times when clock timing time and pulse
>> of data transmission (I Observed by  sigrock logic analyzer) will not be
>> beautiful compared to PIO mode.
>> I worry if other devices using this driver also limit the maximum length
>> of transmission/reception to 18bytes.
> One other issue is that MSIOF drops the hardware chip select in between
> transfers.  Before, transfers that fully fit in the FIFO were sent in a
> single transfer, keeping the hardware chip select asserted during the
> full transfer.
> If transfers are split, and you need the chip select to be asserted
> during the full transfer, you have to use a GPIO chip select instead.
> I believe the split can already happen since your commit 916d9802e4b0723c
> ("spi: sh-msiof: Reduce the number of times write to and perform the
> transmission from FIFO")?

That's right, aligning the maximum message length of 18-bytes current 
with AT25 eeprom.

> However, for most SPI slave devices (e.g. AT25), that issue is moot, as
> they need a GPIO chip select anyway, to avoid dropping the chip select
> in between transfers belonging to the same message.
But I worry here is whether other devices using spi also align data? I 
think not.
So with SPI-MSIOF as SPI driver, we serve all devices using SPI, not 
only EEPROM,
and we don't care what Slave is. As far as code and theory is concerned,
I think there is no problem.
So I will update this patch.


Thanks you very much!

Hoan.


> Gr{oetje,eeting}s,
>
>                          Geert
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-11  8:58 [PATCH 1/2] spi: sh-msiof: fix *info pointer in request_dma() Nguyen An Hoan
2019-01-11  8:58 ` [PATCH 2/2] spi: sh-msiof: Use DMA if possible Nguyen An Hoan
2019-01-11  9:09   ` Geert Uytterhoeven
2019-01-11  9:27     ` Hoan
2019-01-11 10:05       ` Geert Uytterhoeven
2019-01-18  8:19         ` Hoan
2019-01-11  9:01 ` [PATCH 1/2] spi: sh-msiof: fix *info pointer in request_dma() Geert Uytterhoeven
2019-01-11 10:15 ` Sergei Shtylyov
2019-01-11 13:26 ` Mark Brown

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
	public-inbox-index linux-renesas-soc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox