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

[Trent Piepho]

On Mon, 2016-01-11 at 05:34 +0100, Marek Vasut wrote:
> From: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx at public.gmane.org>
> 
> 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.

I'm trying to use this driver the Alaric Devkit for Altera's Arria10
SoC.  It's not working so far.  In the course of trying to debug it,
I've found a few things with the driver in the socfpga-4.1-ltsi branch.
However most of them are in code that's not present in this driver,
which is the newest post of the code I could find to reply to.

> +					 CQSPI_REG_IRQ_WATERMARK	| \
> +					 CQSPI_REG_IRQ_UNDERFLOW)
> +
> +#define CQSPI_IRQ_STATUS_MASK		0x1FFFF
> +

Perhaps a comment.
/* waits for all bits set in mask to be zero (clear==false) or one
(clear==true) */

> +static int cqspi_wait_for_bit(void __iomem *reg, const u32 mask, bool clear)
> +{
> +	unsigned long end = jiffies + msecs_to_jiffies(CQSPI_TIMEOUT_MS);
> +	u32 val;
> +
> +	while (1) {
> +		val = readl(reg);
> +		if (clear)
> +			val = ~val;
> +		val &= mask;
> +
> +		if (val == mask)
> +			return 0;

Somewhat simpler.

if ((readl(reg) & mask) == (clear ? 0 : mask))
        return 0;

> +
> +		if (time_after(jiffies, end))
> +			return -ETIMEDOUT;

Note that there is a hypervisor/vm/long hardirq etc. bug that can happen
without timeouts like this.  What happens is after the check of the bits
fails, there is a very long delay before this task runs again.  This can
be easy if one is running under virtualization.  The the time_after call
reports the timeout expired.  But the last time it the bit was checked,
before the long delay, was well before the timeout expired.  The way to
avoid this is to always be sure to check the condition once after the
timeout expired.  This is sure to give the full timeout.


> +	}
> +}
> +

> +
> +static unsigned int calculate_ticks_for_ns(const unsigned int ref_clk_hz,
> +					   const unsigned int ns_val)
> +{
> +	unsigned int ticks;
> +
> +	ticks = ref_clk_hz / 1000;	/* kHz */
This division doesn't round up.

> +	ticks = DIV_ROUND_UP(ticks * ns_val, 1000000);
But this one does.

> +
> +	return ticks;
> +}
> +
> +static void cqspi_delay(struct spi_nor *nor, const unsigned int sclk_hz)
> +{
> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
> +	struct cqspi_st *cqspi = f_pdata->cqspi;

Isn't the sclk_hz parameter of this function already available here as
cqspi->sclk?

> +	void __iomem *iobase = cqspi->iobase;
> +	const unsigned int ref_clk_hz = cqspi->master_ref_clk_hz;
> +	unsigned int tshsl, tchsh, tslch, tsd2d;
> +	unsigned int reg;
> +	unsigned int tsclk;
> +
> +	/* calculate the number of ref ticks for one sclk tick */
> +	tsclk = (ref_clk_hz + sclk_hz - 1) / sclk_hz;

DIV_ROUND_UP(ref_clk_hz, sclk_hz);

> +
> +	tshsl = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tshsl_ns);
> +	/* this particular value must be at least one sclk */
> +	if (tshsl < tsclk)
> +		tshsl = tsclk;
> +
> +	tchsh = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tchsh_ns);
> +	tslch = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tslch_ns);
> +	tsd2d = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tsd2d_ns);
> +
> +	reg = (tshsl & CQSPI_REG_DELAY_TSHSL_MASK)
> +	       << CQSPI_REG_DELAY_TSHSL_LSB;
> +	reg |= (tchsh & CQSPI_REG_DELAY_TCHSH_MASK)
> +		<< CQSPI_REG_DELAY_TCHSH_LSB;
> +	reg |= (tslch & CQSPI_REG_DELAY_TSLCH_MASK)
> +		<< CQSPI_REG_DELAY_TSLCH_LSB;
> +	reg |= (tsd2d & CQSPI_REG_DELAY_TSD2D_MASK)
> +		<< CQSPI_REG_DELAY_TSD2D_LSB;
> +	writel(reg, iobase + CQSPI_REG_DELAY);
> +}
> +
> +static void cqspi_config_baudrate_div(struct cqspi_st *cqspi,
> +				      const unsigned int sclk_hz)
> +{

sclk_hz is available as cqspi->sclk.

> +	const unsigned int ref_clk_hz = cqspi->master_ref_clk_hz;
> +	void __iomem *reg_base = cqspi->iobase;
> +	unsigned int reg;
> +	unsigned int div;
> +
> +	reg = readl(reg_base + CQSPI_REG_CONFIG);
> +	reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK << CQSPI_REG_CONFIG_BAUD_LSB);
> +
> +	div = ref_clk_hz / sclk_hz;

Should this round up too?

> +
> +	/* Recalculate the baudrate divisor based on QSPI specification. */
> +	if (div > 32)
> +		div = 32;
> +
> +	/* Check if even number. */
> +	if (div & 1)
> +		div = (div / 2);
> +	else
> +		div = (div / 2) - 1;

Wouldn't this be the same as div = DIV_ROUND_UP(div, 2) - 1;

The entire div calculation could be done with:

   /* Register programmed with divider minus 1 */
   div = DIV_ROUND_UP(ref_clk_hz, s_clk_hz * 2) - 1;
   if (div > 15)
        div = 15;


> +
> +	div = (div & CQSPI_REG_CONFIG_BAUD_MASK) << CQSPI_REG_CONFIG_BAUD_LSB;
> +	reg |= div;
> +	writel(reg, reg_base + CQSPI_REG_CONFIG);
> +}
> +