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

Hi,

On 01/11/2016 10:20 AM, Marek Vasut wrote:

On Monday, January 11, 2016 at 05:14:17 AM, R, Vignesh wrote:

quoted

Hi Marek,

Hi!

quoted

On 08/21/2015 02:50 PM, Marek Vasut wrote:

quoted

From: Graham Moore <redacted>

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.

Thanks for the patch.
I have a TI EVM with Cadence QSPI controller connected to Spansion
flash(s25fl512s), and I gave this series a try. It worked with couple of
changes, please see comments inline.

I just posted the latest greatest version I have in tree, could you give it
a spin please ?

Thanks, I will test it soon.

quoted

quoted

Signed-off-by: Graham Moore <redacted>
Signed-off-by: Marek Vasut <redacted>
Cc: Alan Tull <redacted>
Cc: Brian Norris <redacted>
Cc: David Woodhouse <redacted>
Cc: Dinh Nguyen <redacted>
Cc: Graham Moore <redacted>
Cc: Vikas MANOCHA <redacted>
Cc: Yves Vandervennet <redacted>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

[...]

quoted

quoted

+static irqreturn_t cqspi_irq_handler(int this_irq, void *dev)
+{
+	struct cqspi_st *cqspi = dev;
+	unsigned int irq_status;
+
+	/* Read interrupt status */
+	irq_status = readl(cqspi->iobase + CQSPI_REG_IRQSTATUS);
+
+	/* Clear interrupt */
+	writel(irq_status, cqspi->iobase + CQSPI_REG_IRQSTATUS);
+
+	irq_status &= CQSPI_IRQ_MASK_RD | CQSPI_IRQ_MASK_WR;
+
+	if (irq_status)
+		complete(&cqspi->transfer_complete);
+

You seem to signal completion even if the interrupt is
CQSPI_REG_IRQ_IND_(WR|RD)_OVERFLOW. Doesn't this lead to data loss on
overflow?

That's a good question. The write routine, cqspi_indirect_write_execute(),
always writes a page and then waits for completion, so the overflow should
never happen.

What would you suggest to do about write overflow interrupt anyway ? :(

My concern was signalling completion on CQSPI_REG_IRQ_IND_WR_OVERFLOW
interrupt which meant data loss.
But, I see you have disabled CQSPI_REG_IRQ_IND_WR_OVERFLOW in v10. This
addresses my concern.

quoted

quoted

+	return IRQ_HANDLED;
+}

[...]

quoted

quoted

+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);
+	}
+
+	/* Setup baudrate divisor and delays */
+	if (cqspi->sclk != sclk) {
+		cqspi->sclk = sclk;
+		cqspi_controller_disable(cqspi);
+		cqspi_config_baudrate_div(cqspi, sclk);
+		cqspi_delay(nor, sclk);
+		cqspi_readdata_capture(cqspi, 1, f_pdata->read_delay);

I see bypass field value is being hard coded to 1. Atleast on TI SoC,
bypass=0 when SPI bus frequency is  >= 50MHz. Therefore, is it possible
to provide a way to configure bypass bit from DT?
Or if bypass=0 for >=50MHz on your SoC too then, its better to configure
bypass bit based on SPI bus frequency.

You can have multiple SPI peripherals attached to the QSPI block, each running 
at different bus speed, so I think the bypass should be configured based on the 
frequency of the active peripheral, not from OF.

I agree, if the dependency is uniform across SoCs, then bypass should be
configured based on the frequency of the active peripheral.

I don't know exactly what impact configuring this bit has, the socfpga datasheet 
is not very clear about the meaning of this bit, so it'd be better if someone 
from Altera answered this question.

Have you tested the driver at SPI bus frequency >50MHz?

quoted

quoted

+		cqspi_controller_enable(cqspi);
+	}
+	return 0;
+}

[...]

[...]

quoted

quoted

+	ret = devm_request_irq(dev, irq, cqspi_irq_handler, 0,
+			       pdev->name, cqspi);
+	if (ret) {
+		dev_err(dev, "Cannot request IRQ.\n");
+		return ret;
+	}
+

pm_runtime_enable(), pm_runtime_get_sync() might be needed here as QSPI
clocks may not be enabled by default on all platforms.

Can you whip up a bit of code and test it on your platform ? I dont think
socfpga has runtime PM yet. I'd like to add it into the driver or you can
send a small patch later, whichever way you prefer.

Ok, I can send a patch once the driver is accepted.

quoted

quoted

+	cqspi_wait_idle(cqspi);
+	cqspi_controller_init(cqspi);
+	cqspi->current_cs = -1;
+	cqspi->sclk = 0;
+
+	ret = cqspi_setup_flash(cqspi, np);
+	if (ret) {
+		dev_err(dev, "Cadence QSPI NOR probe failed %d\n", ret);
+		cqspi_controller_disable(cqspi);
+	}
+
+	return ret;
+}

Thanks!

-- 
Regards
Vignesh
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html