Re: [PATCH 4/5] mtd: spi-nor: Add driver for Adaptrum Anarion QSPI controller

[Alexandru Gagniuc]

Hi Marek,

Me again!

On 07/29/2017 02:34 AM, Marek Vasut wrote:
> On 07/29/2017 12:07 AM, Alexandru Gagniuc wrote:
> > +static void aspi_drain_fifo(struct anarion_qspi *aspi, uint8_t *buf, size_t len)
> > +{
> > +	uint32_t data;
>
> Is this stuff below something like ioread32_rep() ?
[snip]
> > +	aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
> > +	while (len >= 4) {
> > +		memcpy(&data, buf, sizeof(data));
> > +		aspi_write_reg(spi, ASPI_REG_DATA1, data);
>
> iowrite32_rep ?
>
> > +		buf += 4;
> > +		len -= 4;
> > +	}

I looked at using io(read|write)32_rep in these two places, and I've run 
into some issues.

First, I'm seeing unaligned MMIO accesses, which are not supported on 
ARC. Note that 'buf' has an alignment of 1, while the register requires 
an alignment of 4. The memcpy() in-between takes care of that, which was 
the original intent.

Other than that, we still need to break off the tail because we need to 
update ASPI_REG_BYTE_COUNT before writing/reading any more data from the 
FIFO. We have to keep track of the remainder, so we're not really saving 
any SLOC.

I'd like to keep the original version as I find it to be much more 
symmetrical and readable.

Thanks,
Alex

> > +
> > +	if (len) {
> > +		aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, len);
> > +		memcpy(&data, buf, len);
> > +		aspi_write_reg(spi, ASPI_REG_DATA1, data);
> > +	}
> > +}


Exhibit A: aspi_seed_fifo with writesl()

static void aspi_seed_fifo(struct anarion_qspi *spi,
			   const uint8_t *buf, size_t len)
{
	uint32_t data;
	void __iomem *data_reg = (void *)(spi->regbase + ASPI_REG_DATA1);

	aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
	writesl(data_reg, buf, len / 4);
	buf += len & ~0x03;
	len &= 0x03;

	if (len) {
		aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, len);
		memcpy(&data, buf, len);
		aspi_write_reg(spi, ASPI_REG_DATA1, data);
	}
}