Re: [PATCH 2/2] spi: Add driver for IMG SPFI controller

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3390 bytes --]


On Wed, Nov 12, 2014 at 01:37:54PM -0800, Andrew Bresticker wrote:
> Add support for the Synchronous Peripheral Flash Interface (SPFI) master
> controller found on IMG SoCs.  The SPFI controller supports 5 chip-select
> lines and single/dual/quad mode SPI transfers.

>  drivers/spi/spi-img.c | 703 ++++++++++++++++++++++++++++++++++++++++++++++++++

How about spi-img-spfi?  That way if someone makes another SPI
controller (say a more generic one, this one seems flash specialized)
there won't be a collision.

> +static void spfi_flush_tx_fifo(struct img_spfi *spfi)
> +{
> +	spfi_writel(spfi, SPFI_INTERRUPT_SDE, SPFI_INTERRUPT_CLEAR);
> +	while (!(spfi_readl(spfi, SPFI_INTERRUPT_STATUS) &
> +		 SPFI_INTERRUPT_SDE))
> +		cpu_relax();
> +}

This will busy loop for ever if we don't get the response we want from
the hardware...  some cap on the number of spins would be good.

> +	while ((tx_bytes > 0 || rx_bytes > 0) &&
> +	       time_before(jiffies, timeout)) {
> +		unsigned int tx_count, rx_count;
> +
> +		if (xfer->bits_per_word == 32) {
> +			tx_count = spfi_pio_write32(spfi, tx_buf, tx_bytes);
> +			rx_count = spfi_pio_read32(spfi, rx_buf, rx_bytes);
> +		} else {
> +			tx_count = spfi_pio_write8(spfi, tx_buf, tx_bytes);
> +			rx_count = spfi_pio_read8(spfi, rx_buf, rx_bytes);
> +		}

switch statement please (here and elsewhere).  Apart from the
defensivenes it means that we'll do the right thing if someone decides
to add 16 bit support to the hardware.

> +		tx_buf += tx_count;
> +		rx_buf += rx_count;
> +		tx_bytes -= tx_count;
> +		rx_bytes -= rx_count;

No errors possible?

> +
> +		cpu_relax();

Seems random - we already relax in the data transfer?

> +	if (tx_buf)
> +		spfi_flush_tx_fifo(spfi);
> +	spfi_disable(spfi);

What does the enable and disable actually do?  Should this be runtime
PM?

> +       if (xfer->tx_nbits == SPI_NBITS_DUAL ||
> +           xfer->rx_nbits == SPI_NBITS_DUAL)
> +               val |= SPFI_CONTROL_TMODE_DUAL << SPFI_CONTROL_TMODE_SHIFT;
> +       else if (xfer->tx_nbits == SPI_NBITS_QUAD ||
> +                xfer->rx_nbits == SPI_NBITS_QUAD)
> +               val |= SPFI_CONTROL_TMODE_QUAD << SPFI_CONTROL_TMODE_SHIFT;

This suggests that dual and quad mode must be symmetric but doesn't
enforce that; I rather suspect that in reality these modes are only
supported on the transmit side.

> +static irqreturn_t img_spfi_irq(int irq, void *dev_id)
> +{
> +	struct img_spfi *spfi = (struct img_spfi *)dev_id;
> +	u32 status;
> +
> +	status = spfi_readl(spfi, SPFI_INTERRUPT_STATUS);
> +	if (status & SPFI_INTERRUPT_IACCESS) {
> +		spfi_writel(spfi, SPFI_INTERRUPT_IACCESS, SPFI_INTERRUPT_CLEAR);
> +		dev_err(spfi->dev, "Illegal access interrupt");
> +	}
> +
> +	return IRQ_HANDLED;
> +}

This will unconditionally claim to have handled an interrupt even if it
didn't get any interrupt status it has ever heard of.  At the very least
it should log unknown interrupts, ideally return IRQ_NONE though since
it seems to be a clear on read interrupt that's a bit misleading.

> +	ret = clk_prepare_enable(spfi->sys_clk);
> +	if (ret)
> +		goto put_spi;
> +	ret = clk_prepare_enable(spfi->spfi_clk);
> +	if (ret)
> +		goto disable_pclk;

We should have runtime PM callbacks to enable these clocks only when the
controller is active, this will improve power consumption slightly - the
core can manage the runtime PM for you.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]