Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1534238 > unrolled thread

Re: [PATCH v8] mtd: spi-nor: Add support for S3AN spi-nor devices

Started byMarek Vasut <marek.vasut@gmail.com>
First post2016-12-01 17:10 +0100
Last post2016-12-02 14:20 +0100
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v8] mtd: spi-nor: Add support for S3AN spi-nor devices Marek Vasut <marek.vasut@gmail.com> - 2016-12-01 17:10 +0100
    Re: [PATCH v8] mtd: spi-nor: Add support for S3AN spi-nor devices Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2016-12-01 19:00 +0100
      Re: [PATCH v8] mtd: spi-nor: Add support for S3AN spi-nor devices Marek Vasut <marek.vasut@gmail.com> - 2016-12-01 19:40 +0100
        Re: [PATCH v8] mtd: spi-nor: Add support for S3AN spi-nor devices Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2016-12-02 12:00 +0100
          Re: [PATCH v8] mtd: spi-nor: Add support for S3AN spi-nor devices Marek Vasut <marek.vasut@gmail.com> - 2016-12-02 14:20 +0100

#1534238 — Re: [PATCH v8] mtd: spi-nor: Add support for S3AN spi-nor devices

FromMarek Vasut <marek.vasut@gmail.com>
Date2016-12-01 17:10 +0100
SubjectRe: [PATCH v8] mtd: spi-nor: Add support for S3AN spi-nor devices
Message-ID<sJC5P-6gw-11@gated-at.bofh.it>
On 11/24/2016 05:56 PM, Ricardo Ribalda Delgado wrote:
> Xilinx Spartan-3AN FPGAs contain an In-System Flash where they keep
> their configuration data and (optionally) some user data.
> 
> The protocol of this flash follows most of the spi-nor standard. With
> the following differences:
> 
> - Page size might not be a power of two.
> - The address calculation (default addressing mode).
> - The spi nor commands used.
> 
> Protocol is described on Xilinx User Guide UG333
> 
> Reviewed-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Brian Norris <computersforpeace@gmail.com>
> ---

Hi, minor questions/nits below.

[...]

> -Remove inline qualifier
> -Improve documentation of Default Addressing Mode
> -Convert function callbacks into SNOR_F_
> -Fix missmatch braces
> -Improve documentation of SPI_S3AN flag
> 
>  drivers/mtd/spi-nor/spi-nor.c | 134 ++++++++++++++++++++++++++++++++++++++++--
>  include/linux/mtd/spi-nor.h   |  12 ++++
>  2 files changed, 141 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d0fc165d7d66..cf0410d4e258 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -75,6 +75,12 @@ struct flash_info {
>  					 * bit. Must be used with
>  					 * SPI_NOR_HAS_LOCK.
>  					 */
> +#define	SPI_S3AN		BIT(10)	/*
> +					 * Xilinx Spartan 3AN In-System Flash
> +					 * (MFR cannot be used for probing
> +					 * because it has the same value as
> +					 * ATMEL flashes)
> +					 */

I have possibly off-topic question. Altera has something very similar --
EPCS/EPCQ flash which cannot be detected using standard READID .
Would this patch help with supporting those degenerate flashes too?

>  };
>  
>  #define JEDEC_MFR(info)	((info)->id[0])

[...]

> @@ -1170,8 +1241,15 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  
>  	for (i = 0; i < len; ) {
>  		ssize_t written;
> +		loff_t addr = to + i;
> +
> +		if (hweight32(nor->page_size) == 1) {
> +			page_offset = addr & (nor->page_size - 1);
> +		} else {
> +			uint64_t aux = addr;
>  
> -		page_offset = (to + i) & (nor->page_size - 1);
> +			page_offset = do_div(aux, nor->page_size);
> +		}

Why is this part necessary ?

>  		WARN_ONCE(page_offset,
>  			  "Writing at offset %zu into a NOR page. Writing partial pages may decrease reliability and increase wear of NOR flash.",
>  			  page_offset);

[...]

> +static int s3an_nor_scan(const struct flash_info *info, struct spi_nor *nor)
> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = nor->read_reg(nor, SPINOR_OP_XRDSR, &val, 1);
> +	if (ret < 0) {
> +		dev_err(nor->dev, "error %d reading XRDSR\n", (int) ret);
> +		return ret;
> +	}
> +
> +	nor->erase_opcode = SPINOR_OP_XSE;
> +	nor->program_opcode = SPINOR_OP_XPP;
> +	nor->read_opcode = SPINOR_OP_READ;
> +	nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
> +
> +	/* Flash in Power of 2 mode */
> +	if (val & XSR_PAGESIZE) {
> +		nor->page_size = (nor->page_size == 264) ? 256 : 512;

264 is due to some ECC ?

> +		nor->mtd.writebufsize = nor->page_size;
> +		nor->mtd.size = 8 * nor->page_size * info->n_sectors;
> +		nor->mtd.erasesize = 8 * nor->page_size;
> +	} else {
> +		nor->flags |= SNOR_F_S3AN_ADDR_DEFAULT;
> +	}
> +
> +	return 0;
> +}

[...]

-- 
Best regards,
Marek Vasut

[toc] | [next] | [standalone]

#1534340

FromRicardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Date2016-12-01 19:00 +0100
Message-ID<sJDOi-7Cs-5@gated-at.bofh.it>
In reply to#1534238
Hi Marek

Thanks for your review

On Thu, Dec 1, 2016 at 5:05 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 11/24/2016 05:56 PM, Ricardo Ribalda Delgado wrote:

>> +#define      SPI_S3AN                BIT(10) /*
>> +                                      * Xilinx Spartan 3AN In-System Flash
>> +                                      * (MFR cannot be used for probing
>> +                                      * because it has the same value as
>> +                                      * ATMEL flashes)
>> +                                      */
>
> I have possibly off-topic question. Altera has something very similar --
> EPCS/EPCQ flash which cannot be detected using standard READID .
> Would this patch help with supporting those degenerate flashes too?
>
>>  };
>>

I dont know, but I love the term degenerated flash, please let me use it :)

> >       for (i = 0; i < len; ) {
> >               ssize_t written;
> > +             loff_t addr = to + i;
> > +
> > +             if (hweight32(nor->page_size) == 1) {
> > +                     page_offset = addr & (nor->page_size - 1);
> > +             } else {
> > +                     uint64_t aux = addr;
> >
> > -             page_offset = (to + i) & (nor->page_size - 1);
> > +                     page_offset = do_div(aux, nor->page_size);
> > +             }
>
> Why is this part necessary ?

If page_size is not a power of 2 (264,528),  the & (size-1) operation
for getting the offset  does not work, you need to do a real modulus.

> > +
> > +     /* Flash in Power of 2 mode */
> > +     if (val & XSR_PAGESIZE) {
> > +             nor->page_size = (nor->page_size == 264) ? 256 : 512;
>
> 264 is due to some ECC ?

The flash can be in standard mode or in power of two mode. You need to
check the status register to know if the chip is in one mode or the
other.

The flash is in standard mode from factory, you can change the mode to
power of two, but the data is corrupted and you cannot change  back to
standard mode.

I guess they are using some bits reserved to ECC for data and that way
you can squeeze some bits for user data.

>
> > +             nor->mtd.writebufsize = nor->page_size;
> > +             nor->mtd.size = 8 * nor->page_size * info->n_sectors;
> > +             nor->mtd.erasesize = 8 * nor->page_size;
> > +     } else {
> > +             nor->flags |= SNOR_F_S3AN_ADDR_DEFAULT;
> > +     }
> > +
> > +     return 0;
> > +}
>
> [...]
>
> --
> Best regards,
> Marek Vasut

Thanks Again




-- 
Ricardo Ribalda

[toc] | [prev] | [next] | [standalone]

#1534375

FromMarek Vasut <marek.vasut@gmail.com>
Date2016-12-01 19:40 +0100
Message-ID<sJEqZ-8du-5@gated-at.bofh.it>
In reply to#1534340
On 12/01/2016 06:52 PM, Ricardo Ribalda Delgado wrote:
> Hi Marek

Hi,

> Thanks for your review
> 
> On Thu, Dec 1, 2016 at 5:05 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> On 11/24/2016 05:56 PM, Ricardo Ribalda Delgado wrote:
> 
>>> +#define      SPI_S3AN                BIT(10) /*
>>> +                                      * Xilinx Spartan 3AN In-System Flash
>>> +                                      * (MFR cannot be used for probing
>>> +                                      * because it has the same value as
>>> +                                      * ATMEL flashes)
>>> +                                      */
>>
>> I have possibly off-topic question. Altera has something very similar --
>> EPCS/EPCQ flash which cannot be detected using standard READID .
>> Would this patch help with supporting those degenerate flashes too?
>>
>>>  };
>>>
> 
> I dont know, but I love the term degenerated flash, please let me use it :)

Hehe. It'd be great to know whether we don't have a possibility for a
generic usecase here. Can you briefly check that ?

>>>       for (i = 0; i < len; ) {
>>>               ssize_t written;
>>> +             loff_t addr = to + i;
>>> +
>>> +             if (hweight32(nor->page_size) == 1) {
>>> +                     page_offset = addr & (nor->page_size - 1);
>>> +             } else {
>>> +                     uint64_t aux = addr;
>>>
>>> -             page_offset = (to + i) & (nor->page_size - 1);
>>> +                     page_offset = do_div(aux, nor->page_size);
>>> +             }
>>
>> Why is this part necessary ?
> 
> If page_size is not a power of 2 (264,528),  the & (size-1) operation
> for getting the offset  does not work, you need to do a real modulus.

Aaah, now it makes sense. Ew, but all right, thanks for clarifying.
This could use a comment, somewhere along these lines.

>>> +
>>> +     /* Flash in Power of 2 mode */
>>> +     if (val & XSR_PAGESIZE) {
>>> +             nor->page_size = (nor->page_size == 264) ? 256 : 512;
>>
>> 264 is due to some ECC ?
> 
> The flash can be in standard mode or in power of two mode. You need to
> check the status register to know if the chip is in one mode or the
> other.
> 
> The flash is in standard mode from factory, you can change the mode to
> power of two, but the data is corrupted and you cannot change  back to
> standard mode.
> 
> I guess they are using some bits reserved to ECC for data and that way
> you can squeeze some bits for user data.

OK, comment could help clarify this, so please add one.

>>> +             nor->mtd.writebufsize = nor->page_size;
>>> +             nor->mtd.size = 8 * nor->page_size * info->n_sectors;
>>> +             nor->mtd.erasesize = 8 * nor->page_size;
>>> +     } else {
>>> +             nor->flags |= SNOR_F_S3AN_ADDR_DEFAULT;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>
>> [...]
>>
>> --
>> Best regards,
>> Marek Vasut
> 
> Thanks Again
> 
> 
> 
> 


-- 
Best regards,
Marek Vasut

[toc] | [prev] | [next] | [standalone]

#1534837

FromRicardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Date2016-12-02 12:00 +0100
Message-ID<sJTJo-2Mb-21@gated-at.bofh.it>
In reply to#1534375
Hi Marek

On Thu, Dec 1, 2016 at 7:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 12/01/2016 06:52 PM, Ricardo Ribalda Delgado wrote:
>> Hi Marek
>
> Hi,
>
>> Thanks for your review
>>
>> On Thu, Dec 1, 2016 at 5:05 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>
>>> On 11/24/2016 05:56 PM, Ricardo Ribalda Delgado wrote:
>>
>>>> +#define      SPI_S3AN                BIT(10) /*
>>>> +                                      * Xilinx Spartan 3AN In-System Flash
>>>> +                                      * (MFR cannot be used for probing
>>>> +                                      * because it has the same value as
>>>> +                                      * ATMEL flashes)
>>>> +                                      */
>>>
>>> I have possibly off-topic question. Altera has something very similar --
>>> EPCS/EPCQ flash which cannot be detected using standard READID .
>>> Would this patch help with supporting those degenerate flashes too?
>>>
>>>>  };
>>>>
>>
>> I dont know, but I love the term degenerated flash, please let me use it :)
>
> Hehe. It'd be great to know whether we don't have a possibility for a
> generic usecase here. Can you briefly check that ?

I have taken a brief look to
https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/hb/cfg/cfg_cf52012.pdf

and they seem different enough to not reuse the flag :(.

>> I guess they are using some bits reserved to ECC for data and that way
>> you can squeeze some bits for user data.
>
> OK, comment could help clarify this, so please add one.

Will send a v9

Thanks!

[toc] | [prev] | [next] | [standalone]

#1534907

FromMarek Vasut <marek.vasut@gmail.com>
Date2016-12-02 14:20 +0100
Message-ID<sJVUS-4jk-51@gated-at.bofh.it>
In reply to#1534837
On 12/02/2016 11:52 AM, Ricardo Ribalda Delgado wrote:
> Hi Marek

Hi,

> On Thu, Dec 1, 2016 at 7:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 12/01/2016 06:52 PM, Ricardo Ribalda Delgado wrote:
>>> Hi Marek
>>
>> Hi,
>>
>>> Thanks for your review
>>>
>>> On Thu, Dec 1, 2016 at 5:05 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>
>>>> On 11/24/2016 05:56 PM, Ricardo Ribalda Delgado wrote:
>>>
>>>>> +#define      SPI_S3AN                BIT(10) /*
>>>>> +                                      * Xilinx Spartan 3AN In-System Flash
>>>>> +                                      * (MFR cannot be used for probing
>>>>> +                                      * because it has the same value as
>>>>> +                                      * ATMEL flashes)
>>>>> +                                      */
>>>>
>>>> I have possibly off-topic question. Altera has something very similar --
>>>> EPCS/EPCQ flash which cannot be detected using standard READID .
>>>> Would this patch help with supporting those degenerate flashes too?
>>>>
>>>>>  };
>>>>>
>>>
>>> I dont know, but I love the term degenerated flash, please let me use it :)
>>
>> Hehe. It'd be great to know whether we don't have a possibility for a
>> generic usecase here. Can you briefly check that ?
> 
> I have taken a brief look to
> https://www.altera.com/content/dam/altera-www/global/en_US/pdfs/literature/hb/cfg/cfg_cf52012.pdf
> 
> and they seem different enough to not reuse the flag :(.

OK, fine, thanks for checking.

>>> I guess they are using some bits reserved to ECC for data and that way
>>> you can squeeze some bits for user data.
>>
>> OK, comment could help clarify this, so please add one.
> 
> Will send a v9

Thanks!

-- 
Best regards,
Marek Vasut

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web