[PATCH] mtd: spi-nor: use _lock_and_prep() in _read_sfdp()

[Cyrille Pitchen]

Hi Yogesh,

Le 22/12/2017 à 09:35, Yogesh Gaur a écrit :
> spi_nor_read_sfdp() calls nor->read() to read the SFDP data.
> But first needs to call nor->prepare() to prepare the READ command before
> calling nor->read().
>

Currently, I don't agree with this: spi_nor_read_sfdp() is called only from
spi_nor_scan(). Before the SFDP patch, spi_nor_scan() has already been
calling for years nor->read_reg() and nor->write_reg() without calling
spi_nor_lock_and_prep() first.

Indeed, still being in spi_nor_scan(), the SPI NOR memory should have not
been registered yet to the above MTD layer. So there should be no need to
lock nor->lock at this point.

Then, about calling nor->prepare(), IMHO, it's look a little bit odd that
neither nor->read_reg() nor nor->write_reg() require nor->prepare() to be
called first whereas nor->read() does need it.

You didn't describe what issue or bug you actually noticed so now I assume
it deals with the fsl-quadspi.c driver. So let's have a look at its
implementation of nor->prepare() / fsl_qspi_prep():

+static int fsl_qspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct fsl_qspi *q = nor->priv;
+	int ret;
+
+	mutex_lock(&q->lock);

It was not needed before by fsl_qspi_read_reg() or fls_qspi_write_reg(),
why would it be needed now by fls_qspi_read()?

Then I guess, this is not the issue.

Anyway, if actually needed or safer, the mutex can be locked just after
mutex_init() and before for_each_available_child_of_node() in
fsl_qspi_probe().

+
+	ret = fsl_qspi_clk_prep_enable(q);

Already called from fls_qpsi_probe() before calling spi_nor_scan():
should not be the issue neither.

+	if (ret)
+		goto err_mutex;
+
+	fsl_qspi_set_base_addr(q, nor);

Also called from fsl_qspi_probe() right before calling spi_nor_scan():

		/* set the chip address for READID */
		fsl_qspi_set_base_addr(q, nor);

		ret = spi_nor_scan(nor, NULL, &hwcaps);
		if (ret)
			goto mutex_failed;

		ret = mtd_device_register(mtd, NULL, 0);
		if (ret)
			goto mutex_failed;

		/* Set the correct NOR size now. */
		if (q->nor_size == 0) {
			q->nor_size = mtd->size;

			/* Map the SPI NOR to accessiable address */
			fsl_qspi_set_map_addr(q);
		}

q->nor_size is only updated after spi_nor_scan() hence I don't see why
calling nor->prepare() hence fsl_qpsi_set_base_addr() one more time now
during spi_nor_scan() would actually change anything.

Please note that all I said was based on the assumption that you are trying
to fix an issue in the fsl-quadspi.c driver (based on your email address:
@nxp.com) as the nxp-spifi.c driver doesn't provide any implementation for
the optional nor->prepare().

Then could you please describe a little bit more details about the actual
issue? What does this patch fix? Which drivers are involved? 

Indeed, you would like to have a precise understanding of the issue and
also avoid modifying spi-nor.c rather than the single fsl-quadspi.c driver
or whatever driver is concerned since calling nor->preprare() from
spi_nor_scan() has never been done before and may have unwanted side effects
on other SPI flash controller drivers.

Actually, this is the reason why spi_nor_read_sfdp() directly calls
nor->read() instead of spi_nor_read().

Best regards,

Cyrille

+	return 0;
+
+err_mutex:
+	mutex_unlock(&q->lock);
+	return ret;
+}

 
> Patch adds call to spi_nor_lock_and_prep() to prepare the READ command
> and add subsequent call to spi_nor_unlock_and_unprep() to release lock
> and call nor->unprepare().
> 
> Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur at nxp.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 2203d6ef..9fb85ae 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1807,6 +1807,10 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
>  	nor->addr_width = 3;
>  	nor->read_dummy = 8;
>  
> +	ret = spi_nor_lock_and_prep(nor, SPI_NOR_OPS_READ);
> +	if (ret)
> +		goto lock_err;
> +
>  	while (len) {
>  		ret = nor->read(nor, addr, len, (u8 *)buf);
>  		if (!ret || ret > len) {
> @@ -1822,6 +1826,8 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
>  	}
>  	ret = 0;
>  
> +	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ);
> +lock_err:
>  read_err:
>  	nor->read_opcode = read_opcode;
>  	nor->addr_width = addr_width;
>