[RFT PATCH 0/4] mtd: spi-nor: code refactoring and layering

[RFT PATCH 0/4] mtd: spi-nor: code refactoring and layering

[Brian Norris]

Hi,

I found a few areas where code should probably be refactored in the spi-nor
driver layering while reviewing drivers recently. I don't have hardware for
some of this, hence the Request For Test.

Potential future patch: push the MTD registration into a spi_nor_*() API. I
would have just merged it into spi_nor_scan() now, but there are occasionally a
few things that drivers want to do before exposing the MTD but after the SPI
NOR probing. Perhaps a bottom-half API, and an optional all-in-one API that
does both, similar to nand_base's nand_scan_ident(), nand_scan_tail(), and
nand_scan()?

Anyway, enough random musings. Happy reviewing/testing.

Brian

Brian Norris (4):
  mtd: fsl-quadspi: use automatic spi-nor detection
  mtd: spi-nor: assign mtd->priv in spi_nor_scan()
  mtd: spi-nor: add forward declaration for mtd_info
  mtd: spi-nor: embed struct mtd_info within struct spi_nor

 drivers/mtd/devices/m25p80.c      | 11 ++++-------
 drivers/mtd/spi-nor/fsl-quadspi.c | 19 +++++--------------
 drivers/mtd/spi-nor/nxp-spifi.c   |  7 ++-----
 drivers/mtd/spi-nor/spi-nor.c     |  9 +++++----
 include/linux/mtd/spi-nor.h       |  4 +++-
 5 files changed, 19 insertions(+), 31 deletions(-)

-- 
2.5.0.276.gf5e568e

[RFT PATCH 1/4] mtd: fsl-quadspi: use automatic spi-nor detection

[Brian Norris]

We don't really need the flash information from the device tree here.
Let's stick with autodetection here instead.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/fsl-quadspi.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index d32b7e04ccca..2a17ec6269ff 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -1006,8 +1006,6 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 
 	/* iterate the subnodes. */
 	for_each_available_child_of_node(dev->of_node, np) {
-		char modalias[40];
-
 		/* skip the holes */
 		if (!q->has_second_chip)
 			i *= 2;
@@ -1030,10 +1028,6 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 		nor->prepare = fsl_qspi_prep;
 		nor->unprepare = fsl_qspi_unprep;
 
-		ret = of_modalias_node(np, modalias, sizeof(modalias));
-		if (ret < 0)
-			goto mutex_failed;
-
 		ret = of_property_read_u32(np, "spi-max-frequency",
 				&q->clk_rate);
 		if (ret < 0)
@@ -1042,7 +1036,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 		/* set the chip address for READID */
 		fsl_qspi_set_base_addr(q, nor);
 
-		ret = spi_nor_scan(nor, modalias, SPI_NOR_QUAD);
+		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
 		if (ret)
 			goto mutex_failed;
 
-- 
2.5.0.276.gf5e568e

[RFT PATCH 2/4] mtd: spi-nor: assign mtd->priv in spi_nor_scan()

[Brian Norris]

Layering suggests that the SPI NOR layer (not the hardware driver)
should be initializing the MTD layer.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/devices/m25p80.c      | 1 -
 drivers/mtd/spi-nor/fsl-quadspi.c | 1 -
 drivers/mtd/spi-nor/nxp-spifi.c   | 1 -
 drivers/mtd/spi-nor/spi-nor.c     | 1 +
 4 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 9cd3631170ef..24965ae9f7aa 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -205,7 +205,6 @@ static int m25p_probe(struct spi_device *spi)
 	nor->priv = flash;
 
 	spi_set_drvdata(spi, flash);
-	flash->mtd.priv = nor;
 	flash->spi = spi;
 
 	if (spi->mode & SPI_RX_QUAD)
diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index 2a17ec6269ff..cefc9636e551 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -1016,7 +1016,6 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 		nor->mtd = mtd;
 		nor->dev = dev;
 		nor->priv = q;
-		mtd->priv = nor;
 
 		/* fill the hooks */
 		nor->read_reg = fsl_qspi_read_reg;
diff --git a/drivers/mtd/spi-nor/nxp-spifi.c b/drivers/mtd/spi-nor/nxp-spifi.c
index 9ad1dd0896c0..ce6a478a02a5 100644
--- a/drivers/mtd/spi-nor/nxp-spifi.c
+++ b/drivers/mtd/spi-nor/nxp-spifi.c
@@ -331,7 +331,6 @@ static int nxp_spifi_setup_flash(struct nxp_spifi *spifi,
 
 	writel(ctrl, spifi->io_base + SPIFI_CTRL);
 
-	spifi->mtd.priv  = &spifi->nor;
 	spifi->nor.mtd   = &spifi->mtd;
 	spifi->nor.dev   = spifi->dev;
 	spifi->nor.priv  = spifi;
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 47516d3af015..cba513373d5e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1066,6 +1066,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 
 	if (!mtd->name)
 		mtd->name = dev_name(dev);
+	mtd->priv = nor;
 	mtd->type = MTD_NORFLASH;
 	mtd->writesize = 1;
 	mtd->flags = MTD_CAP_NORFLASH;
-- 
2.5.0.276.gf5e568e

[RFT PATCH 3/4] mtd: spi-nor: add forward declaration for mtd_info

[Brian Norris]

This header can't actually stand alone, as it relies on the declaration
(but not definition) of struct mtd_info. Let's fix that.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 include/linux/mtd/spi-nor.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index e5409524bb0a..85a24baea093 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -127,6 +127,8 @@ enum spi_nor_option_flags {
 	SNOR_F_USE_FSR		= BIT(0),
 };
 
+struct mtd_info;
+
 /**
  * struct spi_nor - Structure for defining a the SPI NOR layer
  * @mtd:		point to a mtd_info structure
-- 
2.5.0.276.gf5e568e

[RFT PATCH 4/4] mtd: spi-nor: embed struct mtd_info within struct spi_nor

[Brian Norris]

This reflects the proper layering, so let's do it.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/devices/m25p80.c      | 10 ++++------
 drivers/mtd/spi-nor/fsl-quadspi.c | 10 ++++------
 drivers/mtd/spi-nor/nxp-spifi.c   |  6 ++----
 drivers/mtd/spi-nor/spi-nor.c     |  8 ++++----
 include/linux/mtd/spi-nor.h       |  2 +-
 5 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 24965ae9f7aa..b6bc9a2a5f87 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -31,7 +31,6 @@
 struct m25p {
 	struct spi_device	*spi;
 	struct spi_nor		spi_nor;
-	struct mtd_info		mtd;
 	u8			command[MAX_CMD_SIZE];
 };
 
@@ -159,7 +158,7 @@ static int m25p80_erase(struct spi_nor *nor, loff_t offset)
 	struct m25p *flash = nor->priv;
 
 	dev_dbg(nor->dev, "%dKiB at 0x%08x\n",
-		flash->mtd.erasesize / 1024, (u32)offset);
+		flash->spi_nor.mtd.erasesize / 1024, (u32)offset);
 
 	/* Set up command buffer. */
 	flash->command[0] = nor->erase_opcode;
@@ -201,7 +200,6 @@ static int m25p_probe(struct spi_device *spi)
 	nor->read_reg = m25p80_read_reg;
 
 	nor->dev = &spi->dev;
-	nor->mtd = &flash->mtd;
 	nor->priv = flash;
 
 	spi_set_drvdata(spi, flash);
@@ -213,7 +211,7 @@ static int m25p_probe(struct spi_device *spi)
 		mode = SPI_NOR_DUAL;
 
 	if (data && data->name)
-		flash->mtd.name = data->name;
+		nor->mtd.name = data->name;
 
 	/* For some (historical?) reason many platforms provide two different
 	 * names in flash_platform_data: "name" and "type". Quite often name is
@@ -231,7 +229,7 @@ static int m25p_probe(struct spi_device *spi)
 
 	ppdata.of_node = spi->dev.of_node;
 
-	return mtd_device_parse_register(&flash->mtd, NULL, &ppdata,
+	return mtd_device_parse_register(&nor->mtd, NULL, &ppdata,
 			data ? data->parts : NULL,
 			data ? data->nr_parts : 0);
 }
@@ -242,7 +240,7 @@ static int m25p_remove(struct spi_device *spi)
 	struct m25p	*flash = spi_get_drvdata(spi);
 
 	/* Clean up MTD stuff. */
-	return mtd_device_unregister(&flash->mtd);
+	return mtd_device_unregister(&flash->spi_nor.mtd);
 }
 
 /*
diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index cefc9636e551..f28dcc1cd63f 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -259,7 +259,6 @@ static struct fsl_qspi_devtype_data imx6ul_data = {
 
 #define FSL_QSPI_MAX_CHIP	4
 struct fsl_qspi {
-	struct mtd_info mtd[FSL_QSPI_MAX_CHIP];
 	struct spi_nor nor[FSL_QSPI_MAX_CHIP];
 	void __iomem *iobase;
 	void __iomem *ahb_addr;
@@ -888,7 +887,7 @@ static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs)
 	int ret;
 
 	dev_dbg(nor->dev, "%dKiB at 0x%08x:0x%08x\n",
-		nor->mtd->erasesize / 1024, q->chip_base_addr, (u32)offs);
+		nor->mtd.erasesize / 1024, q->chip_base_addr, (u32)offs);
 
 	ret = fsl_qspi_runcmd(q, nor->erase_opcode, offs, 0);
 	if (ret)
@@ -1011,9 +1010,8 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 			i *= 2;
 
 		nor = &q->nor[i];
-		mtd = &q->mtd[i];
+		mtd = &nor->mtd;
 
-		nor->mtd = mtd;
 		nor->dev = dev;
 		nor->priv = q;
 
@@ -1080,7 +1078,7 @@ last_init_failed:
 		/* skip the holes */
 		if (!q->has_second_chip)
 			i *= 2;
-		mtd_device_unregister(&q->mtd[i]);
+		mtd_device_unregister(&q->nor[i].mtd);
 	}
 mutex_failed:
 	mutex_destroy(&q->lock);
@@ -1100,7 +1098,7 @@ static int fsl_qspi_remove(struct platform_device *pdev)
 		/* skip the holes */
 		if (!q->has_second_chip)
 			i *= 2;
-		mtd_device_unregister(&q->mtd[i]);
+		mtd_device_unregister(&q->nor[i].mtd);
 	}
 
 	/* disable the hardware */
diff --git a/drivers/mtd/spi-nor/nxp-spifi.c b/drivers/mtd/spi-nor/nxp-spifi.c
index ce6a478a02a5..0f6b452574bd 100644
--- a/drivers/mtd/spi-nor/nxp-spifi.c
+++ b/drivers/mtd/spi-nor/nxp-spifi.c
@@ -60,7 +60,6 @@ struct nxp_spifi {
 	struct clk *clk_reg;
 	void __iomem *io_base;
 	void __iomem *flash_base;
-	struct mtd_info mtd;
 	struct spi_nor nor;
 	bool memory_mode;
 	u32 mcmd;
@@ -331,7 +330,6 @@ static int nxp_spifi_setup_flash(struct nxp_spifi *spifi,
 
 	writel(ctrl, spifi->io_base + SPIFI_CTRL);
 
-	spifi->nor.mtd   = &spifi->mtd;
 	spifi->nor.dev   = spifi->dev;
 	spifi->nor.priv  = spifi;
 	spifi->nor.read  = nxp_spifi_read;
@@ -364,7 +362,7 @@ static int nxp_spifi_setup_flash(struct nxp_spifi *spifi,
 	}
 
 	ppdata.of_node = np;
-	ret = mtd_device_parse_register(&spifi->mtd, NULL, &ppdata, NULL, 0);
+	ret = mtd_device_parse_register(&spifi->nor.mtd, NULL, &ppdata, NULL, 0);
 	if (ret) {
 		dev_err(spifi->dev, "mtd device parse failed\n");
 		return ret;
@@ -453,7 +451,7 @@ static int nxp_spifi_remove(struct platform_device *pdev)
 {
 	struct nxp_spifi *spifi = platform_get_drvdata(pdev);
 
-	mtd_device_unregister(&spifi->mtd);
+	mtd_device_unregister(&spifi->nor.mtd);
 	clk_disable_unprepare(spifi->clk_spifi);
 	clk_disable_unprepare(spifi->clk_reg);
 
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index cba513373d5e..f911be6eadcd 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -263,7 +263,7 @@ static int spi_nor_wait_till_ready(struct spi_nor *nor)
  */
 static int erase_chip(struct spi_nor *nor)
 {
-	dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd->size >> 10));
+	dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd.size >> 10));
 
 	return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0, 0);
 }
@@ -371,7 +371,7 @@ erase_err:
 
 static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 {
-	struct mtd_info *mtd = nor->mtd;
+	struct mtd_info *mtd = &nor->mtd;
 	uint32_t offset = ofs;
 	uint8_t status_old, status_new;
 	int ret = 0;
@@ -405,7 +405,7 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 
 static int stm_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
 {
-	struct mtd_info *mtd = nor->mtd;
+	struct mtd_info *mtd = &nor->mtd;
 	uint32_t offset = ofs;
 	uint8_t status_old, status_new;
 	int ret = 0;
@@ -1006,7 +1006,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	const struct spi_device_id	*id = NULL;
 	struct flash_info		*info;
 	struct device *dev = nor->dev;
-	struct mtd_info *mtd = nor->mtd;
+	struct mtd_info *mtd = &nor->mtd;
 	struct device_node *np = dev->of_node;
 	int ret;
 	int i;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 85a24baea093..495433d3f56d 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -162,7 +162,7 @@ struct mtd_info;
  * @priv:		the private data
  */
 struct spi_nor {
-	struct mtd_info		*mtd;
+	struct mtd_info		mtd;
 	struct mutex		lock;
 	struct device		*dev;
 	u32			page_size;
-- 
2.5.0.276.gf5e568e

Re: [RFT PATCH 1/4] mtd: fsl-quadspi: use automatic spi-nor detection

[Marek Vasut]

On Friday, August 14, 2015 at 12:46:02 AM, Brian Norris wrote:
> We don't really need the flash information from the device tree here.
> Let's stick with autodetection here instead.

Thanks for keeping me in the loop.

> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/spi-nor/fsl-quadspi.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c
> b/drivers/mtd/spi-nor/fsl-quadspi.c index d32b7e04ccca..2a17ec6269ff
> 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -1006,8 +1006,6 @@ static int fsl_qspi_probe(struct platform_device
> *pdev)
> 
>  	/* iterate the subnodes. */
>  	for_each_available_child_of_node(dev->of_node, np) {
> -		char modalias[40];
> -
>  		/* skip the holes */
>  		if (!q->has_second_chip)
>  			i *= 2;
> @@ -1030,10 +1028,6 @@ static int fsl_qspi_probe(struct platform_device
> *pdev) nor->prepare = fsl_qspi_prep;
>  		nor->unprepare = fsl_qspi_unprep;
> 
> -		ret = of_modalias_node(np, modalias, sizeof(modalias));
> -		if (ret < 0)
> -			goto mutex_failed;
> -
>  		ret = of_property_read_u32(np, "spi-max-frequency",
>  				&q->clk_rate);
>  		if (ret < 0)
> @@ -1042,7 +1036,7 @@ static int fsl_qspi_probe(struct platform_device
> *pdev) /* set the chip address for READID */
>  		fsl_qspi_set_base_addr(q, nor);
> 
> -		ret = spi_nor_scan(nor, modalias, SPI_NOR_QUAD);
> +		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);

This is something I don't quite understand. So we have a SPI NOR controller,
which as it's own struct device instance.

This controller can have multiple SPI NORs on it. Each SPI NOR has it's own
struct spi_nor instance and struct mtd_info instance, right?

But, all of the SPI NORs share the same struct device as the controller. Do
I understand that correctly ? Does that make sense to NOT allocate a new
struct device for each of the SPI NORs ?

Best regards,
Marek Vasut

Re: [RFT PATCH 1/4] mtd: fsl-quadspi: use automatic spi-nor detection

[Brian Norris]

On Fri, Aug 14, 2015 at 01:09:14AM +0200, Marek Vasut wrote:
> This is something I don't quite understand. So we have a SPI NOR controller,
> which as it's own struct device instance.

Right.

> This controller can have multiple SPI NORs on it. Each SPI NOR has it's own
> struct spi_nor instance and struct mtd_info instance, right?

Right.

> But, all of the SPI NORs share the same struct device as the controller. Do
> I understand that correctly ?

Mostly yes. But your next statement doesn't quite follow in my mind, so
maybe you've missed something.

There is typically a single platform device (and associated struct
device) that represents the fsl-quadspi controller.

(Pedantic side point: each MTD actually creates one or more struct
device objects; one for the master MTD, and one for each partition that
might be created. But I don't think you're asking about these struct
device's.)

But none of this is super-relevant to this patch; I'm not talking about
struct devices (think kobjects, Linux driver model), I'm dealing with
struct device_node (think device tree, of_*() APIs, etc.).

Now, each flash connected to the controller has its own device_node. All
this patch is saying is that we don't need to know much about that node;
as long as it responds to the READ ID command properly, spi_nor_scan()
can autodetect it.

> Does that make sense to NOT allocate a new
> struct device for each of the SPI NORs ?

I don't quite see how this question relates. Are you suggesting that we
should be using fewer *device_node* structs? That would involve
rewriting the device tree, I believe.

Or perhaps I've misunderstood your train of thought.

Brian

Re: [RFT PATCH 1/4] mtd: fsl-quadspi: use automatic spi-nor detection

[Marek Vasut]

On Friday, August 14, 2015 at 01:24:47 AM, Brian Norris wrote:
> On Fri, Aug 14, 2015 at 01:09:14AM +0200, Marek Vasut wrote:
> > This is something I don't quite understand. So we have a SPI NOR
> > controller, which as it's own struct device instance.
> 
> Right.
> 
> > This controller can have multiple SPI NORs on it. Each SPI NOR has it's
> > own struct spi_nor instance and struct mtd_info instance, right?
> 
> Right.
> 
> > But, all of the SPI NORs share the same struct device as the controller.
> > Do I understand that correctly ?
> 
> Mostly yes. But your next statement doesn't quite follow in my mind, so
> maybe you've missed something.

I think maybe, just maybe, I should've asked this in an entirely separate
thread, sorry.

> There is typically a single platform device (and associated struct
> device) that represents the fsl-quadspi controller.
> 
> (Pedantic side point: each MTD actually creates one or more struct
> device objects; one for the master MTD, and one for each partition that
> might be created. But I don't think you're asking about these struct
> device's.)

Right.

> But none of this is super-relevant to this patch; I'm not talking about
> struct devices (think kobjects, Linux driver model), I'm dealing with
> struct device_node (think device tree, of_*() APIs, etc.).
> 
> Now, each flash connected to the controller has its own device_node. All
> this patch is saying is that we don't need to know much about that node;
> as long as it responds to the READ ID command properly, spi_nor_scan()
> can autodetect it.

Right.

> > Does that make sense to NOT allocate a new
> > struct device for each of the SPI NORs ?
> 
> I don't quite see how this question relates. Are you suggesting that we
> should be using fewer *device_node* structs? That would involve
> rewriting the device tree, I believe.
> 
> Or perhaps I've misunderstood your train of thought.

Sorry, I should've asked about this in a separate thread.

Best regards,
Marek Vasut

Re: [RFT PATCH 1/4] mtd: fsl-quadspi: use automatic spi-nor detection

[Michal Suchanek]

On 14 August 2015 at 01:24, Brian Norris <computersforpeace@gmail.com> wrote:
> On Fri, Aug 14, 2015 at 01:09:14AM +0200, Marek Vasut wrote:
>> This is something I don't quite understand. So we have a SPI NOR controller,
>> which as it's own struct device instance.
>
> Right.
>
>> This controller can have multiple SPI NORs on it. Each SPI NOR has it's own
>> struct spi_nor instance and struct mtd_info instance, right?
>
> Right.
>
>> But, all of the SPI NORs share the same struct device as the controller. Do
>> I understand that correctly ?
>
> Mostly yes. But your next statement doesn't quite follow in my mind, so
> maybe you've missed something.
>
> There is typically a single platform device (and associated struct
> device) that represents the fsl-quadspi controller.
>
> (Pedantic side point: each MTD actually creates one or more struct
> device objects; one for the master MTD, and one for each partition that
> might be created. But I don't think you're asking about these struct
> device's.)
>
> But none of this is super-relevant to this patch; I'm not talking about
> struct devices (think kobjects, Linux driver model), I'm dealing with
> struct device_node (think device tree, of_*() APIs, etc.).
>
> Now, each flash connected to the controller has its own device_node. All
> this patch is saying is that we don't need to know much about that node;
> as long as it responds to the READ ID command properly, spi_nor_scan()
> can autodetect it.
>

And if there was suppor for a flash chip that does not respond to READ
ID (or uses a different opcode for it) this patch would break it,
right?

Thanks

Michal

Re: [RFT PATCH 2/4] mtd: spi-nor: assign mtd->priv in spi_nor_scan()

[Joachim Eastwood]

On 14 August 2015 at 00:46, Brian Norris <computersforpeace@gmail.com> wrote:
> Layering suggests that the SPI NOR layer (not the hardware driver)
> should be initializing the MTD layer.
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Boots fine on Hitex LPC4350 Evaluation Board (lpc18xx) with SPI-NOR
NXP SPIFI and JFFS2 root on s25sl064p.

Tested-by: Joachim Eastwood <manabian@gmail.com>


regards,
Joachim Eastwood

Re: [RFT PATCH 3/4] mtd: spi-nor: add forward declaration for mtd_info

[Joachim Eastwood]

On 14 August 2015 at 00:46, Brian Norris <computersforpeace@gmail.com> wrote:
> This header can't actually stand alone, as it relies on the declaration
> (but not definition) of struct mtd_info. Let's fix that.
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Not much to test here I guess, but it was a part of the patch set I
applied so FWIW:

Tested-by: Joachim Eastwood <manabian@gmail.com>

regards,
Joachim Eastwood

Re: [RFT PATCH 4/4] mtd: spi-nor: embed struct mtd_info within struct spi_nor

[Joachim Eastwood]

On 14 August 2015 at 00:46, Brian Norris <computersforpeace@gmail.com> wrote:
> This reflects the proper layering, so let's do it.
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Boots fine on Hitex LPC4350 Evaluation Board (lpc18xx) with SPI-NOR
NXP SPIFI and JFFS2 root on s25sl064p.

Tested-by: Joachim Eastwood <manabian@gmail.com>


regards,
Joachim Eastwood

Re: [RFT PATCH 1/4] mtd: fsl-quadspi: use automatic spi-nor detection

[Brian Norris]

On Fri, Aug 14, 2015 at 07:40:27AM +0200, Michal Suchanek wrote:
> On 14 August 2015 at 01:24, Brian Norris <computersforpeace@gmail.com> wrote:
> > Now, each flash connected to the controller has its own device_node. All
> > this patch is saying is that we don't need to know much about that node;
> > as long as it responds to the READ ID command properly, spi_nor_scan()
> > can autodetect it.
> >
> 
> And if there was suppor for a flash chip that does not respond to READ
> ID (or uses a different opcode for it) this patch would break it,
> right?

For the latter: this already doesn't support chips that use different
opcodes.

For the former: we're only talking about the "*-nonjedec" and similar,
right? I'm not confident those were supported well by this driver in the
first place. (And "*-nonjedec" should really die; if it's needed,
support should be added by design, not by accident.)

Perhaps Huang can comment.

Brian

Re: [RFT PATCH 1/4] mtd: fsl-quadspi: use automatic spi-nor detection

[Han Xu]

Seems Huang has changed the e-mail address.

On Mon, Aug 17, 2015 at 9:56 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Fri, Aug 14, 2015 at 07:40:27AM +0200, Michal Suchanek wrote:
>> On 14 August 2015 at 01:24, Brian Norris <computersforpeace@gmail.com> wrote:
>> > Now, each flash connected to the controller has its own device_node. All
>> > this patch is saying is that we don't need to know much about that node;
>> > as long as it responds to the READ ID command properly, spi_nor_scan()
>> > can autodetect it.
>> >
>>
>> And if there was suppor for a flash chip that does not respond to READ
>> ID (or uses a different opcode for it) this patch would break it,
>> right?
>
> For the latter: this already doesn't support chips that use different
> opcodes.
>
> For the former: we're only talking about the "*-nonjedec" and similar,
> right? I'm not confident those were supported well by this driver in the
> first place. (And "*-nonjedec" should really die; if it's needed,
> support should be added by design, not by accident.)
>
> Perhaps Huang can comment.
>
> Brian
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFT PATCH 1/4] mtd: fsl-quadspi: use automatic spi-nor detection

[Huang Shijie]

On Mon, Aug 17, 2015 at 10:15:50PM -0500, Han Xu wrote:
Hi Brian & Han,

> Seems Huang has changed the e-mail address.
> 
> On Mon, Aug 17, 2015 at 9:56 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> > On Fri, Aug 14, 2015 at 07:40:27AM +0200, Michal Suchanek wrote:
> >> On 14 August 2015 at 01:24, Brian Norris <computersforpeace@gmail.com> wrote:
> >> > Now, each flash connected to the controller has its own device_node. All
> >> > this patch is saying is that we don't need to know much about that node;
> >> > as long as it responds to the READ ID command properly, spi_nor_scan()
> >> > can autodetect it.
> >> >
> >>
> >> And if there was suppor for a flash chip that does not respond to READ
> >> ID (or uses a different opcode for it) this patch would break it,
> >> right?

Do not worry about the quadspi controller :)

The controller only uses the best NOR flash (or maybe the best NOR flash), I remember
that the NOR flash can support the QUAD DDR read which is not supported not.

So it will not break the driver. 

Btw: @Han Xu : I think you can test this patch.

> >
> > For the latter: this already doesn't support chips that use different
> > opcodes.
> >
> > For the former: we're only talking about the "*-nonjedec" and similar,
> > right? I'm not confident those were supported well by this driver in the
> > first place. (And "*-nonjedec" should really die; if it's needed,
> > support should be added by design, not by accident.)
> >
> > Perhaps Huang can comment.

I think this patch is okay. But please wait for Han's test result.


thanks
Huang Shijie

Re: [RFT PATCH 1/4] mtd: fsl-quadspi: use automatic spi-nor detection

[Brian Norris]

On Tue, Aug 18, 2015 at 04:09:27PM +0800, Huang Shijie wrote:
> On Mon, Aug 17, 2015 at 10:15:50PM -0500, Han Xu wrote:
> > Seems Huang has changed the e-mail address.

Thanks, noted.

> > On Mon, Aug 17, 2015 at 9:56 PM, Brian Norris
> > <computersforpeace@gmail.com> wrote:
> > > On Fri, Aug 14, 2015 at 07:40:27AM +0200, Michal Suchanek wrote:
> > >> On 14 August 2015 at 01:24, Brian Norris <computersforpeace@gmail.com> wrote:
> > >> > Now, each flash connected to the controller has its own device_node. All
> > >> > this patch is saying is that we don't need to know much about that node;
> > >> > as long as it responds to the READ ID command properly, spi_nor_scan()
> > >> > can autodetect it.
> > >> >
> > >>
> > >> And if there was suppor for a flash chip that does not respond to READ
> > >> ID (or uses a different opcode for it) this patch would break it,
> > >> right?
> 
> Do not worry about the quadspi controller :)
> 
> The controller only uses the best NOR flash (or maybe the best NOR flash), I remember
> that the NOR flash can support the QUAD DDR read which is not supported not.
> 
> So it will not break the driver. 
> 
> Btw: @Han Xu : I think you can test this patch.
> 
> > >
> > > For the latter: this already doesn't support chips that use different
> > > opcodes.
> > >
> > > For the former: we're only talking about the "*-nonjedec" and similar,
> > > right? I'm not confident those were supported well by this driver in the
> > > first place. (And "*-nonjedec" should really die; if it's needed,
> > > support should be added by design, not by accident.)
> > >
> > > Perhaps Huang can comment.
> 
> I think this patch is okay. But please wait for Han's test result.

OK. Pushed patches 2-4 to l2-mtd.git/next. Will wait on this one.

Re: [RFT PATCH 1/4] mtd: fsl-quadspi: use automatic spi-nor detection

[Han Xu]

On Thu, Aug 13, 2015 at 5:46 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> We don't really need the flash information from the device tree here.
> Let's stick with autodetection here instead.
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/spi-nor/fsl-quadspi.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index d32b7e04ccca..2a17ec6269ff 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -1006,8 +1006,6 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>
>         /* iterate the subnodes. */
>         for_each_available_child_of_node(dev->of_node, np) {
> -               char modalias[40];
> -
>                 /* skip the holes */
>                 if (!q->has_second_chip)
>                         i *= 2;
> @@ -1030,10 +1028,6 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>                 nor->prepare = fsl_qspi_prep;
>                 nor->unprepare = fsl_qspi_unprep;
>
> -               ret = of_modalias_node(np, modalias, sizeof(modalias));
> -               if (ret < 0)
> -                       goto mutex_failed;
> -
>                 ret = of_property_read_u32(np, "spi-max-frequency",
>                                 &q->clk_rate);
>                 if (ret < 0)
> @@ -1042,7 +1036,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
>                 /* set the chip address for READID */
>                 fsl_qspi_set_base_addr(q, nor);
>
> -               ret = spi_nor_scan(nor, modalias, SPI_NOR_QUAD);
> +               ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
>                 if (ret)
>                         goto mutex_failed;
>

Acked-by: Han xu <han.xu@freescale.com>
Tested-by: Han xu <han.xu@freescale.com>

> --
> 2.5.0.276.gf5e568e
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFT PATCH 1/4] mtd: fsl-quadspi: use automatic spi-nor detection

[Brian Norris]

On Tue, Sep 01, 2015 at 06:40:17PM -0500, Han Xu wrote:
> On Thu, Aug 13, 2015 at 5:46 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> > We don't really need the flash information from the device tree here.
> > Let's stick with autodetection here instead.
> >
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> >  drivers/mtd/spi-nor/fsl-quadspi.c | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> > index d32b7e04ccca..2a17ec6269ff 100644
> > --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> > @@ -1006,8 +1006,6 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> >
> >         /* iterate the subnodes. */
> >         for_each_available_child_of_node(dev->of_node, np) {
> > -               char modalias[40];
> > -
> >                 /* skip the holes */
> >                 if (!q->has_second_chip)
> >                         i *= 2;
> > @@ -1030,10 +1028,6 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> >                 nor->prepare = fsl_qspi_prep;
> >                 nor->unprepare = fsl_qspi_unprep;
> >
> > -               ret = of_modalias_node(np, modalias, sizeof(modalias));
> > -               if (ret < 0)
> > -                       goto mutex_failed;
> > -
> >                 ret = of_property_read_u32(np, "spi-max-frequency",
> >                                 &q->clk_rate);
> >                 if (ret < 0)
> > @@ -1042,7 +1036,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
> >                 /* set the chip address for READID */
> >                 fsl_qspi_set_base_addr(q, nor);
> >
> > -               ret = spi_nor_scan(nor, modalias, SPI_NOR_QUAD);
> > +               ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
> >                 if (ret)
> >                         goto mutex_failed;
> >
> 
> Acked-by: Han xu <han.xu@freescale.com>
> Tested-by: Han xu <han.xu@freescale.com>

Thanks, applied to l2-mtd.git/next