Re: [linux-sunxi] [PATCH 3/3] MTD: m25p80: Add option to limit SPI transfer size.

Re: [linux-sunxi] [PATCH 3/3] MTD: m25p80: Add option to limit SPI transfer size.

[Julian Calaby]

Hi Michal,

On Thu, Apr 30, 2015 at 11:46 PM, Michal Suchanek <hramrach@gmail.com> wrote:
> My SPI driver does not support DMA so sizes larger than 64 bytes return
> an error. Add an option to limit transfer size so m25p80 can be used
> with reduced speed with such controller.

I suspect that this will be rejected as the maximum transfer size is a
property of the SPI controller, not the device and should be exported
to the device driver from the controller, not specified in the
device's binding.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [linux-sunxi] [PATCH 3/3] MTD: m25p80: Add option to limit SPI transfer size.

[Brian Norris]

On Fri, May 01, 2015 at 12:58:52AM +1000, Julian Calaby wrote:
> On Thu, Apr 30, 2015 at 11:46 PM, Michal Suchanek <hramrach@gmail.com> wrote:
> > My SPI driver does not support DMA so sizes larger than 64 bytes return
> > an error. Add an option to limit transfer size so m25p80 can be used
> > with reduced speed with such controller.
> 
> I suspect that this will be rejected as the maximum transfer size is a
> property of the SPI controller, not the device and should be exported
> to the device driver from the controller, not specified in the
> device's binding.

Absolutely correct. This does not belong in the device tree.

Brian

Re: [PATCH 0/3] Using SPI NOR flah on sunxi.

[Thomas.Betker]

Hello Michal:

> I tried to connect a SPI NOR flash to my sunxi board and due to the 
current
> sunxi SPI driver limitations it does not work.
> 
> The SPI driver returns an error when more than 64 bytes are 
> transferred at once
> due to lack of DMA support.

Wouldn't it be easier to fix the SPI driver to handle transfers larger 
than 64 bytes, filling and draining the FIFO multiple times if neccessary? 
(As far as I can tell, most SPI drivers do this.)

Just asking,
Thomas

Re: [PATCH 0/3] Using SPI NOR flah on sunxi.

[Michal Suchanek]

On 30 April 2015 at 18:30,  <Thomas.Betker@rohde-schwarz.com> wrote:
> Hello Michal:
>
>> I tried to connect a SPI NOR flash to my sunxi board and due to the
> current
>> sunxi SPI driver limitations it does not work.
>>
>> The SPI driver returns an error when more than 64 bytes are
>> transferred at once
>> due to lack of DMA support.
>
> Wouldn't it be easier to fix the SPI driver to handle transfers larger
> than 64 bytes, filling and draining the FIFO multiple times if neccessary?
> (As far as I can tell, most SPI drivers do this.)
>

Yes, the intent is to fix this by adding dma support to the driver, eventually.

The patch might be still useful for other hardware with developing SPI support.

Thanks

Michal

Re: [PATCH 0/3] Using SPI NOR flah on sunxi.

[Marek Vasut]

On Thursday, April 30, 2015 at 06:56:18 PM, Michal Suchanek wrote:
> On 30 April 2015 at 18:30,  <Thomas.Betker@rohde-schwarz.com> wrote:
> > Hello Michal:
> >> I tried to connect a SPI NOR flash to my sunxi board and due to the
> > 
> > current
> > 
> >> sunxi SPI driver limitations it does not work.
> >> 
> >> The SPI driver returns an error when more than 64 bytes are
> >> transferred at once
> >> due to lack of DMA support.
> > 
> > Wouldn't it be easier to fix the SPI driver to handle transfers larger
> > than 64 bytes, filling and draining the FIFO multiple times if
> > neccessary? (As far as I can tell, most SPI drivers do this.)
> 
> Yes, the intent is to fix this by adding dma support to the driver,
> eventually.
> 
> The patch might be still useful for other hardware with developing SPI
> support.

Please just fix the controller driver to correctly handle arbitrary transfer
lengths.

Best regards,
Marek Vasut

Re: [PATCH 0/3] Using SPI NOR flah on sunxi.

[Brian Norris]

On Thu, Apr 30, 2015 at 08:34:36PM +0200, Marek Vasut wrote:
> On Thursday, April 30, 2015 at 06:56:18 PM, Michal Suchanek wrote:
> > On 30 April 2015 at 18:30,  <Thomas.Betker@rohde-schwarz.com> wrote:
> > > Hello Michal:
> > >> I tried to connect a SPI NOR flash to my sunxi board and due to the
> > > 
> > > current
> > > 
> > >> sunxi SPI driver limitations it does not work.
> > >> 
> > >> The SPI driver returns an error when more than 64 bytes are
> > >> transferred at once
> > >> due to lack of DMA support.
> > > 
> > > Wouldn't it be easier to fix the SPI driver to handle transfers larger
> > > than 64 bytes, filling and draining the FIFO multiple times if
> > > neccessary? (As far as I can tell, most SPI drivers do this.)
> > 
> > Yes, the intent is to fix this by adding dma support to the driver,
> > eventually.
> > 
> > The patch might be still useful for other hardware with developing SPI
> > support.
> 
> Please just fix the controller driver to correctly handle arbitrary transfer
> lengths.

Just noticed this. Yes, that would definitely be a better option!

Brian

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Marek Vasut]

On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote:
> The size of written data was added to user supplied value rather than
> written at the provided address.

You might want to work on the commit message a little, something like
the following, but feel free to reword as seen fit.

The 'retlen' points to a variable representing the number of data bytes 
written/read (see include/linux/mtd/mtd.h) by the current invocation of
the function. This variable must be set, not incremented.

Otherwise, the patch is correct I believe:

Acked-by: Marek Vasut <marex@denx.de>

> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  drivers/mtd/devices/m25p80.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 7c8b169..0b2bc21 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -102,7 +102,7 @@ static void m25p80_write(struct spi_nor *nor, loff_t
> to, size_t len,
> 
>  	spi_sync(spi, &m);
> 
> -	*retlen += m.actual_length - cmd_sz;
> +	*retlen = m.actual_length - cmd_sz;
>  }
> 
>  static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)

Best regards,
Marek Vasut

Re: [PATCH 1/3] MTD: m25p80: fix write return value.

[Michal Suchanek]

On 30 April 2015 at 20:43, Marek Vasut <marex@denx.de> wrote:
> On Thursday, April 30, 2015 at 03:33:47 PM, Michal Suchanek wrote:
>> The size of written data was added to user supplied value rather than
>> written at the provided address.
>
> You might want to work on the commit message a little, something like
> the following, but feel free to reword as seen fit.
>
> The 'retlen' points to a variable representing the number of data bytes
> written/read (see include/linux/mtd/mtd.h) by the current invocation of
> the function. This variable must be set, not incremented.
>
> Otherwise, the patch is correct I believe:
>
> Acked-by: Marek Vasut <marex@denx.de>
>

ok, I will send an updated version.

Thanks

Michal

Re: [linux-sunxi] Re: [PATCH 0/3] Using SPI NOR flah on sunxi.

[Michal Suchanek]

Hello,

On 1 May 2015 at 14:27, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>> The SPI driver returns an error when more than 64 bytes are
>> transferred at once due to lack of DMA support.
>
> Have you tried the dmaengine patch and make the SPI driver use it?
>

The dmaengine is already merged or queued in sunxi-wip but the SPI
glue fell through the cracks. I found it after digging through the
archives so I will see if it works for me.

Thanks

Michal

Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.

[Brian Norris]

+ linux-spi, Mark

On Thu, Apr 30, 2015 at 03:38:50PM +0200, Michal Suchanek wrote:
> My SPI controller driver does not support DMA so writes are truncated to
> FIFO size.

Which SPI master driver?

> Check the amount of data actually written by the driver.

I'm not sure if we should just reactively use the retlen, or if we
should be communicating such a limitation via the SPI API. Thoughts?

Brian

> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 42 +++++++++++++++++-------------------------
>  1 file changed, 17 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 14a5d23..75904b5 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -807,7 +807,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  	size_t *retlen, const u_char *buf)
>  {
>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> -	u32 page_offset, page_size, i;
> +	u32 page_offset, page_remainder, page_size, i;
>  	int ret;
>  
>  	dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len);
> @@ -816,36 +816,28 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  	if (ret)
>  		return ret;
>  
> -	write_enable(nor);
> -
> -	page_offset = to & (nor->page_size - 1);
> -
> -	/* do all the bytes fit onto one page? */
> -	if (page_offset + len <= nor->page_size) {
> -		nor->write(nor, to, len, retlen, buf);
> -	} else {
> -		/* the size of data remaining on the first page */
> -		page_size = nor->page_size - page_offset;
> -		nor->write(nor, to, page_size, retlen, buf);
> +	/* write everything in nor->page_size chunks */
> +	/* check the size of data actually written */
> +	for (i = 0; i < len; i += *retlen) {
> +		page_size = len - i;
> +		page_offset = (to + i) & (nor->page_size - 1);
> +		page_remainder = nor->page_size - page_offset;
> +		if (page_size > nor->page_size)
> +			page_size = nor->page_size;
> +		if (page_remainder && (page_size > page_remainder))
> +			page_size = page_remainder;
>  
> -		/* write everything in nor->page_size chunks */
> -		for (i = page_size; i < len; i += page_size) {
> -			page_size = len - i;
> -			if (page_size > nor->page_size)
> -				page_size = nor->page_size;
> -
> -			ret = spi_nor_wait_till_ready(nor);
> -			if (ret)
> -				goto write_err;
> +		write_enable(nor);
>  
> -			write_enable(nor);
> +		nor->write(nor, to + i, page_size, retlen, buf + i);
>  
> -			nor->write(nor, to + i, page_size, retlen, buf + i);
> -		}
> +		ret = spi_nor_wait_till_ready(nor);
> +		if (ret)
> +			goto write_err;
>  	}
>  
> -	ret = spi_nor_wait_till_ready(nor);
>  write_err:
> +	*retlen = i;
>  	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE);
>  	return ret;
>  }

Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.

[Michal Suchanek]

On 21 May 2015 at 01:38, Brian Norris <computersforpeace@gmail.com> wrote:
> + linux-spi, Mark
>
> On Thu, Apr 30, 2015 at 03:38:50PM +0200, Michal Suchanek wrote:
>> My SPI controller driver does not support DMA so writes are truncated to
>> FIFO size.
>
> Which SPI master driver?

I am using sunxi SPI driver. The dmaengine support for sunxi is not
yet in mainline kernel so the SPI master functionality is limited.

>
>> Check the amount of data actually written by the driver.
>
> I'm not sure if we should just reactively use the retlen, or if we
> should be communicating such a limitation via the SPI API. Thoughts?
>

Is there any driver that would break if the SPI master truncated
writes when the message is too long rather than returning an error an
refusing the transfer?

m25p80 as is would because it assumes all data is always written.

Some display driver I tried earlier has an option to limit transfer
size actively in the driver.

Thanks

Michal

Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.

[Mark Brown]

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

On Thu, May 21, 2015 at 10:39:13AM +0200, Michal Suchanek wrote:
> On 21 May 2015 at 01:38, Brian Norris <computersforpeace@gmail.com> wrote:

Why is this thread about SPI error handling CCed to quite so many
people?

> >> Check the amount of data actually written by the driver.

> > I'm not sure if we should just reactively use the retlen, or if we
> > should be communicating such a limitation via the SPI API. Thoughts?

> Is there any driver that would break if the SPI master truncated
> writes when the message is too long rather than returning an error an
> refusing the transfer?

Any such driver is buggy, the actual length transferred has always been
a part of the SPI API.  We should probably expose limitations so clients
can query in advance (we don't at the minute) but it'd be a while before
clients could rely on that information being useful and it's still
possible things can be truncated by error.

With modern drivers using transfer_one() where we have control of the
chip select we do also have the ability to add support to the core for
chopping large transfers up into smaller ones that the hardware can
handle transparently.  That won't for everything though since it depends
on us being able to manually control the chip select which not all
hardware can do.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.

[Brian Norris]

On Thu, May 21, 2015 at 11:28:02AM +0100, Mark Brown wrote:
> On Thu, May 21, 2015 at 10:39:13AM +0200, Michal Suchanek wrote:
> > On 21 May 2015 at 01:38, Brian Norris <computersforpeace@gmail.com> wrote:
> 
> Why is this thread about SPI error handling CCed to quite so many
> people?

I can't really speak for the original patch author, who created the CC
list. I added you for comment on the SPI API (thanks BTW).

This is part of a series that included an ill-conceived DT binding for
recording a "max transfer length" property in the flash node. That's one
step that easily blows up the CC list for the series, as there are 5 DT
maintainers and a mailing list. Others are contributors to the m25p80 /
spi-nor drivers. (All in all, you can probably trace this back to
scripts/get_maintainer.pl.)

> > >> Check the amount of data actually written by the driver.
> 
> > > I'm not sure if we should just reactively use the retlen, or if we
> > > should be communicating such a limitation via the SPI API. Thoughts?
> 
> > Is there any driver that would break if the SPI master truncated
> > writes when the message is too long rather than returning an error an
> > refusing the transfer?
> 
> Any such driver is buggy, the actual length transferred has always been
> a part of the SPI API.

OK, so m25p80.c currently checks the retlen
(spi_message::actual_length), but it doesn't actually handle it well if
the SPI master can't write a full page-size chunk in one go. It looks
like we'd leave holes in the programmed data right now.

So that qualifies as buggy, and that's part of what Michal is trying to
fix, I think. Admittedly, as he's using an out-of-tree driver, I'm not
sure I know exactly what failure modes he is hitting yet.

> We should probably expose limitations so clients
> can query in advance (we don't at the minute) but it'd be a while before
> clients could rely on that information being useful and it's still
> possible things can be truncated by error.

That might help in the long run. In this case, I think we might be able
to get by (for a properly-functioning SPI driver with a limited transfer
size) with the current API, and handling the 'return length < message
length' case better.

BTW, one extra note for Michal regarding your SPI controller/driver
transfer length limitation: you would do well to try as much as possible
to allow full NOR pages to be programmed in one SPINOR_OP_PP sequence. I
know of some SPI NOR that, though they are byte addressable, actually
have opaque internal ECC which is encoded on page boundaries, so you
get better flash lifetime if you program on page boundaries, rather than
on whatever smaller chunk size your SPI driver supports. So that might
require a little more work on your SPI driver.

Brian

Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.

[Brian Norris]

(trimming CC a little this time, though it's still a bit large)

On Fri, May 22, 2015 at 12:17:27AM -0700, Brian Norris wrote:
> Admittedly, as he's using an out-of-tree driver, I'm not
> sure I know exactly what failure modes he is hitting yet.

Sorry, I realized I misread here. He's using spi-sunxi. Given that...

... is this code even valid?

static int sun6i_spi_transfer_one(struct spi_master *master,
                                  struct spi_device *spi,
                                  struct spi_transfer *tfr)
{
...
	/* We don't support transfer larger than the FIFO */
	if (tfr->len > SUN6I_FIFO_DEPTH)
		return -EINVAL;

Seems like it should be looping over the transfer in multiple chunks
instead.

Brian

Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.

[Mark Brown]

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

On Fri, May 22, 2015 at 12:25:05AM -0700, Brian Norris wrote:
> On Fri, May 22, 2015 at 12:17:27AM -0700, Brian Norris wrote:

> > Admittedly, as he's using an out-of-tree driver, I'm not
> > sure I know exactly what failure modes he is hitting yet.

> Sorry, I realized I misread here. He's using spi-sunxi. Given that...

> ... is this code even valid?

> static int sun6i_spi_transfer_one(struct spi_master *master,
>                                   struct spi_device *spi,
>                                   struct spi_transfer *tfr)
> {
> ...
> 	/* We don't support transfer larger than the FIFO */
> 	if (tfr->len > SUN6I_FIFO_DEPTH)
> 		return -EINVAL;

> Seems like it should be looping over the transfer in multiple chunks
> instead.

Well, it's not ideal.  Like I say I think that logic probably belongs in
the core rather than individual drivers then we minimise the problems,
if I remember correctly there was the suggestion that the DMA code was
going to follow soon and be used for larger transfers when the original
driver was merged.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]