[PATCH] mtd: spi-nor: allow NULL as chip name and try to auto detect it

[PATCH] mtd: spi-nor: allow NULL as chip name and try to auto detect it

[Rafał Miłecki]

This will allow spi-nor users to plainly use JEDEC to detect flash chip.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index ee777a4..1facaac 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -937,13 +937,21 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	if (ret)
 		return ret;
 
-	id = spi_nor_match_id(name);
+	/* Try to auto-detect if chip name wasn't specified */
+	if (!name)
+		id = spi_nor_read_id(nor);
+	else
+		id = spi_nor_match_id(name);
 	if (!id)
 		return -ENOENT;
 
 	info = (void *)id->driver_data;
 
-	if (info->jedec_id) {
+	/*
+	 * If caller has specified name of flash model that can normally be
+	 * detected using JEDEC, let's verify it.
+	 */
+	if (name && info->jedec_id) {
 		const struct spi_device_id *jid;
 
 		jid = spi_nor_read_id(nor);
-- 
1.8.4.5

Re: [PATCH] mtd: spi-nor: allow NULL as chip name and try to auto detect it

[Rafał Miłecki]

On 22 October 2014 at 18:05, Rafał Miłecki <zajec5@gmail.com> wrote:
> This will allow spi-nor users to plainly use JEDEC to detect flash chip.

Ping? Can we get this pushed?

It's the first step to allow using spi-nor drivers with auto probing
(rdid), without forcing the chip name.

Re: [PATCH] mtd: spi-nor: allow NULL as chip name and try to auto detect it

[Brian Norris]

On Wed, Oct 22, 2014 at 06:05:00PM +0200, Rafał Miłecki wrote:
> This will allow spi-nor users to plainly use JEDEC to detect flash chip.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index ee777a4..1facaac 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -937,13 +937,21 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  	if (ret)
>  		return ret;
>  
> -	id = spi_nor_match_id(name);
> +	/* Try to auto-detect if chip name wasn't specified */
> +	if (!name)
> +		id = spi_nor_read_id(nor);
> +	else
> +		id = spi_nor_match_id(name);
>  	if (!id)
>  		return -ENOENT;
>  
>  	info = (void *)id->driver_data;
>  
> -	if (info->jedec_id) {
> +	/*
> +	 * If caller has specified name of flash model that can normally be
> +	 * detected using JEDEC, let's verify it.
> +	 */
> +	if (name && info->jedec_id) {

This part doesn't apply cleanly any more.

>  		const struct spi_device_id *jid;
>  
>  		jid = spi_nor_read_id(nor);

I think this is a good time to consider this question: how do we
*really* want a SPI NOR driver to interact with spi-nor.c, regarding
device detection? I like how this patch removes the string-matching
requirement, so we can just auto-detect by JEDEC RDID alone. But I don't
like how it leaves around the function parameter 'name', which really
we should really be moving to deprecate if possible.

Anyway, I can take a rebased version of this patch. But I'd like to
encourage more thought here for the future. I don't yet have a specific
proposal, so any thoughts are welcome.

Brian

[PATCH V2] mtd: spi-nor: allow NULL as chip name and try to auto detect it

[Rafał Miłecki]

This will allow spi-nor users to plainly use JEDEC to detect flash chip.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: Rebase on top of:
6d7604e mtd: spi-nor: add support for s25fl128s
d928a25 mtd: spi-nor: remove the jedec_id/ext_id
09ffafb mtd: spi-nor: add id/id_len for flash_info{}
---
 drivers/mtd/spi-nor/spi-nor.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 0f4f2ba..afc58da 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -938,13 +938,21 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	if (ret)
 		return ret;
 
-	id = spi_nor_match_id(name);
+	/* Try to auto-detect if chip name wasn't specified */
+	if (!name)
+		id = spi_nor_read_id(nor);
+	else
+		id = spi_nor_match_id(name);
 	if (!id)
 		return -ENOENT;
 
 	info = (void *)id->driver_data;
 
-	if (info->id_len) {
+	/*
+	 * If caller has specified name of flash model that can normally be
+	 * detected using JEDEC, let's verify it.
+	 */
+	if (name && info->id_len) {
 		const struct spi_device_id *jid;
 
 		jid = spi_nor_read_id(nor);
-- 
1.8.4.5

Re: [PATCH V2] mtd: spi-nor: allow NULL as chip name and try to auto detect it

[Brian Norris]

On Mon, Dec 01, 2014 at 09:42:16AM +0100, Rafał Miłecki wrote:
> This will allow spi-nor users to plainly use JEDEC to detect flash chip.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
> V2: Rebase on top of:
> 6d7604e mtd: spi-nor: add support for s25fl128s
> d928a25 mtd: spi-nor: remove the jedec_id/ext_id
> 09ffafb mtd: spi-nor: add id/id_len for flash_info{}

Applied, thanks!

Brian

Re: [PATCH] mtd: spi-nor: allow NULL as chip name and try to auto detect it

[Rafał Miłecki]

On 1 December 2014 at 09:40, Brian Norris <computersforpeace@gmail.com> wrote:
> On Wed, Oct 22, 2014 at 06:05:00PM +0200, Rafał Miłecki wrote:
>> This will allow spi-nor users to plainly use JEDEC to detect flash chip.
>>
>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>>
> I think this is a good time to consider this question: how do we
> *really* want a SPI NOR driver to interact with spi-nor.c, regarding
> device detection? I like how this patch removes the string-matching
> requirement, so we can just auto-detect by JEDEC RDID alone. But I don't
> like how it leaves around the function parameter 'name', which really
> we should really be moving to deprecate if possible.
>
> Anyway, I can take a rebased version of this patch. But I'd like to
> encourage more thought here for the future. I don't yet have a specific
> proposal, so any thoughts are welcome.

My purpose is to make use of auto-detect by RDID wherever possible. As
you noticed, it's the first step. Now we should modify m25p80 to use
it when possible.

The case with non-DT platforms is simple. There is
struct flash_platform_data
which contains "type". I was thinking about Introducing new type like
"m25p80-rdid". What do you think about this?

I'm not exactly sure how m25p80 is handled when used in DT. Does
anyone know that? m25p80 itself doesn't register as DT driver. So I
guess there is some code translating DT m25p80 entries into platform
data. Is that right? Where is this code located?

-- 
Rafał

Re: [PATCH] mtd: spi-nor: allow NULL as chip name and try to auto detect it

[Brian Norris]

On Mon, Dec 01, 2014 at 10:49:28AM +0100, Rafał Miłecki wrote:
> On 1 December 2014 at 09:40, Brian Norris <computersforpeace@gmail.com> wrote:
> > I think this is a good time to consider this question: how do we
> > *really* want a SPI NOR driver to interact with spi-nor.c, regarding
> > device detection? I like how this patch removes the string-matching
> > requirement, so we can just auto-detect by JEDEC RDID alone. But I don't
> > like how it leaves around the function parameter 'name', which really
> > we should really be moving to deprecate if possible.
> >
> > Anyway, I can take a rebased version of this patch. But I'd like to
> > encourage more thought here for the future. I don't yet have a specific
> > proposal, so any thoughts are welcome.
> 
> My purpose is to make use of auto-detect by RDID wherever possible. As
> you noticed, it's the first step. Now we should modify m25p80 to use
> it when possible.
> 
> The case with non-DT platforms is simple. There is
> struct flash_platform_data
> which contains "type". I was thinking about Introducing new type like
> "m25p80-rdid". What do you think about this?

It should reflect "JEDEC RDID" in the name; it means the flash supports
the JEDEC opcode (0x9F), unlike some of the SST flash you've looked at.
I'm also not sure why it should still be tied to the m25p80 name. How
about "spi-nor,jedec-id" or "spi-flash,jedec-id"?

> I'm not exactly sure how m25p80 is handled when used in DT. Does
> anyone know that? m25p80 itself doesn't register as DT driver. So I
> guess there is some code translating DT m25p80 entries into platform
> data. Is that right? Where is this code located?

m25p80.c is a SPI device driver, and it gets probed via the kernel bus
infrastructure. See spi_match_device().

Brian

Re: [PATCH] mtd: spi-nor: allow NULL as chip name and try to auto detect it

[Rafał Miłecki]

On 17 December 2014 at 02:15, Brian Norris <computersforpeace@gmail.com> wrote:
> On Mon, Dec 01, 2014 at 10:49:28AM +0100, Rafał Miłecki wrote:
>> On 1 December 2014 at 09:40, Brian Norris <computersforpeace@gmail.com> wrote:
>> > I think this is a good time to consider this question: how do we
>> > *really* want a SPI NOR driver to interact with spi-nor.c, regarding
>> > device detection? I like how this patch removes the string-matching
>> > requirement, so we can just auto-detect by JEDEC RDID alone. But I don't
>> > like how it leaves around the function parameter 'name', which really
>> > we should really be moving to deprecate if possible.
>> >
>> > Anyway, I can take a rebased version of this patch. But I'd like to
>> > encourage more thought here for the future. I don't yet have a specific
>> > proposal, so any thoughts are welcome.
>>
>> My purpose is to make use of auto-detect by RDID wherever possible. As
>> you noticed, it's the first step. Now we should modify m25p80 to use
>> it when possible.
>>
>> The case with non-DT platforms is simple. There is
>> struct flash_platform_data
>> which contains "type". I was thinking about Introducing new type like
>> "m25p80-rdid". What do you think about this?
>
> It should reflect "JEDEC RDID" in the name; it means the flash supports
> the JEDEC opcode (0x9F), unlike some of the SST flash you've looked at.
> I'm also not sure why it should still be tied to the m25p80 name. How
> about "spi-nor,jedec-id" or "spi-flash,jedec-id"?

OK on "jedec-id".

However I'm not sure about this "spi-nor," because spi-not is a
framework and it doesn't support all needed ops by itself. And it
wouldn't be clear if "spi-nor,jedec-id" should be handled by m25p80,
fsl-quadspi, or even some different spi-nor driver.

-- 
Rafał

Re: [PATCH] mtd: spi-nor: allow NULL as chip name and try to auto detect it

[Brian Norris]

On Wed, Dec 17, 2014 at 07:19:58AM +0100, Rafał Miłecki wrote:
> On 17 December 2014 at 02:15, Brian Norris <computersforpeace@gmail.com> wrote:
> > On Mon, Dec 01, 2014 at 10:49:28AM +0100, Rafał Miłecki wrote:
> >> The case with non-DT platforms is simple. There is
> >> struct flash_platform_data
> >> which contains "type". I was thinking about Introducing new type like
> >> "m25p80-rdid". What do you think about this?
> >
> > It should reflect "JEDEC RDID" in the name; it means the flash supports
> > the JEDEC opcode (0x9F), unlike some of the SST flash you've looked at.
> > I'm also not sure why it should still be tied to the m25p80 name. How
> > about "spi-nor,jedec-id" or "spi-flash,jedec-id"?
> 
> OK on "jedec-id".
> 
> However I'm not sure about this "spi-nor," because spi-not is a
> framework and it doesn't support all needed ops by itself.

No, SPI NOR is a (moderately) specific type of SPI device. It also
happens to be the name of the framework / library in
drivers/mtd/spi-nor/spi-nor.c and include/linux/mtd/spi-nor.h.

The point is that the name is descriptive of exactly what we expect of
it; it operates on the SPI bus, it's a NOR flash, and it supports the
JEDEC read ID command.

> And it
> wouldn't be clear if "spi-nor,jedec-id" should be handled by m25p80,
> fsl-quadspi, or even some different spi-nor driver.

fsl-quadspi (and other non-SPI flash drivers that may plug into
spi-nor.c) is not probed on a per flash device basis; it's probed based
on the controller compatibility (i.e., the "fsl,vf610-qspi" DT binding).
The driver then happens to use flash names when it calls into the SPI
NOR library.

As I see it, we only need to worry about true SPI devices when
considering this name. We'd need to be able to differentiate the kinds
of SPI devices which cannot be handled by m25p80.c + spi-nor.c. And as
far as I can see, that will only happen for devices which don't support
the JEDEC RDID opcode.

Brian