[U-Boot] [RFC PATCH 1/6] mtd: spi: Port SPI NOR framework from Linux

[Vignesh R]

Hi Boris,

On 29/11/18 9:34 PM, Boris Brezillon wrote:
> On Wed, 28 Nov 2018 22:56:02 +0530
> Vignesh R <vigneshr at ti.com> wrote:
> 
>> +#if defined(CONFIG_DM_SPI) && defined(CONFIG_SPI_MEM)
>> +#define spi_nor_mem_exec_op spi_mem_exec_op
>> +#else
>> +/*
>> + * This function is to support transition to DM_SPI. Will be removed
>> + * once all boards are converted to DM_SPI
>> + */
>> +
>> +static int spi_nor_mem_exec_op(struct spi_slave *slave,
>> +			       const struct spi_mem_op *op)
>> +{
>> +	unsigned int pos = 0;
>> +	const u8 *tx_buf = NULL;
>> +	u8 *rx_buf = NULL;
>> +	u8 *op_buf;
>> +	int op_len;
>> +	u32 flag;
>> +	int ret;
>> +	int i;
>> +
>> +	if (op->data.nbytes) {
>> +		if (op->data.dir == SPI_MEM_DATA_IN)
>> +			rx_buf = op->data.buf.in;
>> +		else
>> +			tx_buf = op->data.buf.out;
>> +	}
>> +
>> +	op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
>> +	op_buf = calloc(1, op_len);
>> +
>> +	ret = spi_claim_bus(slave);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	op_buf[pos++] = op->cmd.opcode;
>> +
>> +	if (op->addr.nbytes) {
>> +		for (i = 0; i < op->addr.nbytes; i++)
>> +			op_buf[pos + i] = op->addr.val >>
>> +				(8 * (op->addr.nbytes - i - 1));
>> +
>> +		pos += op->addr.nbytes;
>> +	}
>> +
>> +	if (op->dummy.nbytes)
>> +		memset(op_buf + pos, 0xff, op->dummy.nbytes);
>> +
>> +	/* 1st transfer: opcode + address + dummy cycles */
>> +	flag = SPI_XFER_BEGIN;
>> +	/* Make sure to set END bit if no tx or rx data messages follow */
>> +	if (!tx_buf && !rx_buf)
>> +		flag |= SPI_XFER_END;
>> +
>> +	ret = spi_xfer(slave, op_len * 8, op_buf, NULL, flag);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* 2nd transfer: rx or tx data path */
>> +	if (tx_buf || rx_buf) {
>> +		ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf,
>> +			       rx_buf, SPI_XFER_END);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	spi_release_bus(slave);
>> +
>> +	for (i = 0; i < pos; i++)
>> +		debug("%02x ", op_buf[i]);
>> +	debug("| [%dB %s] ",
>> +	      tx_buf || rx_buf ? op->data.nbytes : 0,
>> +	      tx_buf || rx_buf ? (tx_buf ? "out" : "in") : "-");
>> +	for (i = 0; i < op->data.nbytes; i++)
>> +		debug("%02x ", tx_buf ? tx_buf[i] : rx_buf[i]);
>> +	debug("[ret %d]\n", ret);
>> +
>> +	free(op_buf);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +#endif
> 
> I'd recommend putting this alternate spi_mem_exec_op() implementation
> in drivers/spi/. Either you create a drivers/spi/spi-mem-no-dm.c file
> or you add #ifdef CONFIG_DM_SPI #else #endif sections directly in
> drivers/spi/spi-mem.c.
> 

I thought about that, but then I saw spi-mem.c is under DM_SPI and
DM_SPI migration series by Jagan and thought that this code wont be
needing this at all. Anyways, will move this to drivers/spi next time
around.

>> +const struct flash_info spi_nor_ids[] = {
>> +#ifdef CONFIG_SPI_FLASH_ATMEL		/* ATMEL */
>> +	/* Atmel -- some are (confusingly) marketed as "DataFlash" */
>> +	{ "at26df321",  INFO(0x1f4700, 0, 64 * 1024, 64, SECT_4K) },
>> +	{ "at25df321a", INFO(0x1f4701, 0, 64 * 1024, 64, SECT_4K) },
>> +
>> +	{ "at45db011d",	INFO(0x1f2200, 0, 64 * 1024,   4, SECT_4K) },
>> +	{ "at45db021d",	INFO(0x1f2300, 0, 64 * 1024,   8, SECT_4K) },
>> +	{ "at45db041d",	INFO(0x1f2400, 0, 64 * 1024,   8, SECT_4K) },
>> +	{ "at45db081d", INFO(0x1f2500, 0, 64 * 1024,  16, SECT_4K) },
>> +	{ "at45db161d",	INFO(0x1f2600, 0, 64 * 1024,  32, SECT_4K) },
>> +	{ "at45db321d",	INFO(0x1f2700, 0, 64 * 1024,  64, SECT_4K) },
>> +	{ "at45db641d",	INFO(0x1f2800, 0, 64 * 1024, 128, SECT_4K) },
>> +	{ "at26df081a", INFO(0x1f4501, 0, 64 * 1024,  16, SECT_4K) },
>> +#endif
> 
> If you're really short in space (for the SPL build), you might want to
> consider fined-grained selection of the chips you want to support. One
> more reason to do that is that board manufacturers usually source SPI
> NOR parts from different vendors for the same design. With a
> per-manufacturer selection logic, you'll have to enable several of them
> to have an SPL that works on all variants.
> 
> I didn't try, but you might be able to place each NOR chip in its own
> section and decide which one to keep at link time (will require some
> macros to define flash_info entries + a linker script to decide which
> sections you want to discard/keep at link time).
> 

IIUC, you are proposing per board linker script that will select/choose
which flash parts would to be supported by that board's SPL?

> Note that I'd be okay to convert Linux flash_info table to the new
> macro usage if you choose to do that.
>
> Just a quick example:
> 
> #define SPI_FLASH_INFO(_name, ...)
> 	ll_entry_declare(struct flash_info, _name, spi_nor_ids) = {
> 		.name = #_name,
> 		__VAR_ARGS__
> 	};
> 



-- 
Regards
Vignesh