diff mbox

[V10,2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

Message ID 570563D3.9080704@denx.de
State Superseded
Headers show

Commit Message

Marek Vasut April 6, 2016, 7:30 p.m. UTC 
On 04/06/2016 06:55 PM, R, Vignesh wrote:
> Hi Marek,

Hi!

> I encountered a issue with this driver while testing.

Try with the attached patches, I am planning to use them for V11
submission. I think you're hitting the problem with missing buslock.

> On 1/11/2016 10:04 AM, Marek Vasut wrote:
>> From: Graham Moore <grmoore@opensource.altera.com>
>>
>> Add support for the Cadence QSPI controller. This controller is
>> present in the Altera SoCFPGA SoCs and this driver has been tested
>> on the Cyclone V SoC.
> 
> 
>> +static void cqspi_switch_cs(struct spi_nor *nor)
>> +{
>> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +	struct cqspi_st *cqspi = f_pdata->cqspi;
>> +	void __iomem *iobase = cqspi->iobase;
>> +	unsigned int reg;
>> +
>> +	cqspi_controller_enable(cqspi, 0);
>> +
>> +	/* configure page size and block size. */
>> +	reg = readl(iobase + CQSPI_REG_SIZE);
>> +	reg &= ~(CQSPI_REG_SIZE_PAGE_MASK << CQSPI_REG_SIZE_PAGE_LSB);
>> +	reg &= ~(CQSPI_REG_SIZE_BLOCK_MASK << CQSPI_REG_SIZE_BLOCK_LSB);
>> +	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
>> +	reg |= (nor->page_size << CQSPI_REG_SIZE_PAGE_LSB);
>> +	reg |= (ilog2(nor->mtd.erasesize) << CQSPI_REG_SIZE_BLOCK_LSB);
>> +	reg |= (nor->addr_width - 1);
>> +	writel(reg, iobase + CQSPI_REG_SIZE);
>> +
> 
> Page size and block size are configured here...
> 
>> +	/* configure the chip select */
>> +	cqspi_chipselect(nor);
>> +
>> +	cqspi_controller_enable(cqspi, 1);
>> +}
>> +
>> +static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>> +{
>> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +	struct cqspi_st *cqspi = f_pdata->cqspi;
>> +	const unsigned int sclk = f_pdata->clk_rate;
>> +
>> +	/* Switch chip select. */
>> +	if (cqspi->current_cs != f_pdata->cs) {
>> +		cqspi->current_cs = f_pdata->cs;
>> +		cqspi_switch_cs(nor);
> 
> cqspi_switch_cs(nor) is called from cqspi_prep(). And is called only
> once for a given slave (assuming only one slave on the QSPI bus)
> 
>> +	}
>> +
>> +	/* Setup baudrate divisor and delays */
>> +	if (cqspi->sclk != sclk) {
>> +		cqspi->sclk = sclk;
>> +		cqspi_controller_enable(cqspi, 0);
>> +		cqspi_config_baudrate_div(cqspi, sclk);
>> +		cqspi_delay(nor, sclk);
>> +		cqspi_readdata_capture(cqspi, 1, f_pdata->read_delay);
>> +		cqspi_controller_enable(cqspi, 1);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int cqspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>> +{
>> +	int ret;
>> +
>> +	ret = cqspi_set_protocol(nor, nor->reg_proto);
>> +	if (ret)
>> +		return ret;
>> +
>> +	cqspi_prep(nor, SPI_NOR_OPS_READ);
> 
> cqspi_read_reg() is first called to read JEDEC ID, this calls
> cqspi_prep() for first time. But nor->page_size, nor->mtd.erasesize are
> not yet populated. Therefore cqspi_switch_cs() will not populate
> CQSPI_REG_SIZE with correct values.
> I think its better to configure this register each time during
> read/write ops.
> 
> Regards
> Vignesh
>

Comments

Raghavendra, Vignesh April 7, 2016, 4:55 a.m. UTC | #1 
On 04/07/2016 01:00 AM, Marek Vasut wrote:
> On 04/06/2016 06:55 PM, R, Vignesh wrote:
>> Hi Marek,
> 
> Hi!
> 
>> I encountered a issue with this driver while testing.
> 
> Try with the attached patches, I am planning to use them for V11
> submission. I think you're hitting the problem with missing buslock.
> 

Thanks for the patches.
But I am pretty sure that's not the problem at my end, because I have
only one flash device on QSPI bus.

The problem is cqspi_switch_cs() is called only once ie when JEDEC ID is
being read(during autodetect of chip), but at that instance,
nor->page_size and nor->mtd.erasesize are not yet initialized (They are
initialized only after JEDEC ID is looked up in the table and page_size
and erasesize are known).
Therefore if nor->page_size is printed during cqspi_switch_cs() then its
zero. But nor->page_size reports 256 when printed in cqspi_flash_setup()
after spi_nor_scan(). Therefore CQSPI_REG_SIZE register has to be
configured only after spi_nor struct is fully populated (i.e after
spi_nor_scan() has recognized the slave after JEDEC ID read).
Marek Vasut April 13, 2016, 10:27 a.m. UTC | #2 
On 04/07/2016 06:55 AM, Vignesh R wrote:
> 
> 
> On 04/07/2016 01:00 AM, Marek Vasut wrote:
>> On 04/06/2016 06:55 PM, R, Vignesh wrote:
>>> Hi Marek,
>>
>> Hi!
>>
>>> I encountered a issue with this driver while testing.
>>
>> Try with the attached patches, I am planning to use them for V11
>> submission. I think you're hitting the problem with missing buslock.
>>
> 
> Thanks for the patches.
> But I am pretty sure that's not the problem at my end, because I have
> only one flash device on QSPI bus.

OK, sorry for the delayed reply. I will go through the rest of the QSPI
mail later, probably this week.

> The problem is cqspi_switch_cs() is called only once ie when JEDEC ID is
> being read(during autodetect of chip), but at that instance,
> nor->page_size and nor->mtd.erasesize are not yet initialized (They are
> initialized only after JEDEC ID is looked up in the table and page_size
> and erasesize are known).
> Therefore if nor->page_size is printed during cqspi_switch_cs() then its
> zero. But nor->page_size reports 256 when printed in cqspi_flash_setup()
> after spi_nor_scan(). Therefore CQSPI_REG_SIZE register has to be
> configured only after spi_nor struct is fully populated (i.e after
> spi_nor_scan() has recognized the slave after JEDEC ID read).
> 
>
diff mbox

Patch

>From e680fa497cf0e2f41fe1b56c7d6b868596d8e420 Mon Sep 17 00:00:00 2001
From: Marek Vasut <marex@denx.de>
Date: Wed, 23 Mar 2016 08:30:47 +0100
Subject: [PATCH 3/3] mtd: spi-nor: cqspi: Add bus lock

Lock the whole bus in .prepare callback and unlock the whole bus
in the .unprepare callback. This is necessary to prevent a race
condition when accessing two flashes, each of which has it's own
instance of struct spi_nor, in parallel. The SPI NOR framework
only locks the mutex in struct spi_nor and therefore the locking
happens with per-flash granularity, which is not enough to prevent
concurrent access to the SPI NOR controller registers.

Signed-off-by: Marek Vasut <marex@denx.de>
---
 drivers/mtd/spi-nor/cadence-quadspi.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index ee309cb..b086609 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -63,6 +63,7 @@  struct cqspi_st {
 	void __iomem		*iobase;
 	void __iomem		*ahb_base;
 	struct completion	transfer_complete;
+	struct mutex		bus_mutex;
 
 	int			current_cs;
 	unsigned long		master_ref_clk_hz;
@@ -919,7 +920,7 @@  static void cqspi_switch_cs(struct spi_nor *nor)
 	cqspi_chipselect(nor);
 }
 
-static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
+static int cqspi_prep_unlocked(struct spi_nor *nor, enum spi_nor_ops ops)
 {
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
 	struct cqspi_st *cqspi = f_pdata->cqspi;
@@ -950,31 +951,51 @@  static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
 	return 0;
 }
 
+static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+
+	mutex_lock(&cqspi->bus_mutex);
+
+	return cqspi_prep_unlocked(nor, ops);
+}
+
+static void cqspi_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+
+	mutex_unlock(&cqspi->bus_mutex);
+}
+
 static int cqspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 {
 	int ret;
 
 	ret = cqspi_set_protocol(nor, nor->reg_proto);
 	if (ret)
-		return ret;
+		goto exit;
 
-	cqspi_prep(nor, SPI_NOR_OPS_READ);
+	cqspi_prep_unlocked(nor, SPI_NOR_OPS_READ);
 
 	ret = cqspi_command_read(nor, &opcode, 1, buf, len);
+exit:
 	return ret;
 }
 
 static int cqspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 {
-	int ret = 0;
+	int ret;
 
 	ret = cqspi_set_protocol(nor, nor->reg_proto);
 	if (ret)
-		return ret;
+		goto exit;
 
-	cqspi_prep(nor, SPI_NOR_OPS_WRITE);
+	cqspi_prep_unlocked(nor, SPI_NOR_OPS_WRITE);
 
 	ret = cqspi_command_write(nor, opcode, buf, len);
+exit:
 	return ret;
 }
 
@@ -1113,6 +1134,7 @@  static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
 		nor->write = cqspi_write;
 		nor->erase = cqspi_erase;
 		nor->prepare = cqspi_prep;
+		nor->unprepare = cqspi_unprep;
 
 		mtd->name = kasprintf(GFP_KERNEL, "%s.%d", dev_name(dev), cs);
 		if (!mtd->name) {
@@ -1154,6 +1176,7 @@  static int cqspi_probe(struct platform_device *pdev)
 	if (!cqspi)
 		return -ENOMEM;
 
+	mutex_init(&cqspi->bus_mutex);
 	cqspi->pdev = pdev;
 	platform_set_drvdata(pdev, cqspi);
 
-- 
2.7.0