[U-Boot] [PATCH] Tegra210: add QSPI driver

[Tom Warren]

Stephen,

-----Original Message-----
From: Stephen Warren [mailto:swarren at wwwdotorg.org] 
Sent: Friday, October 09, 2015 11:51 AM
To: Tom Warren <TWarren at nvidia.com>
Cc: u-boot at lists.denx.de; Jagan Teki <jteki at openedev.com>; Stephen Warren <swarren at nvidia.com>; tomcwarren3959 at gmail.com
Subject: Re: [U-Boot] [PATCH] Tegra210: add QSPI driver

On 10/09/2015 12:20 PM, Tom Warren wrote:
> From: Yen Lin <yelin at nvidia.com>
>
> Tested on P2571 board with Winbond SPI ROM.
> sf probe 2:0 to see SPI chip. Flashed U-Boot image to SPI, rebooted, read back data to RAM with sf read, then verified crc against original binary file.

>   board/nvidia/p2571/pinmux-config-p2571.h |  14 +-
>   configs/p2571_defconfig                  |   1 +

As a general comment, driver changes should be in separate patches to board changes to enable the driver. The two things are logically separate, and this also helps when cherry-picking patches between branches that might not have p2571 support.

True, my bad. I'll resubmit.

> diff --git a/board/nvidia/p2571/pinmux-config-p2571.h 
> b/board/nvidia/p2571/pinmux-config-p2571.h

> -	PINCFG(QSPI_SCK_PEE0,        RSVD1,    DOWN,   TRISTATE, OUTPUT,  DISABLE, DEFAULT),
> -	PINCFG(QSPI_CS_N_PEE1,       RSVD1,    DOWN,   TRISTATE, OUTPUT,  DISABLE, DEFAULT),
> -	PINCFG(QSPI_IO0_PEE2,        RSVD1,    DOWN,   TRISTATE, OUTPUT,  DISABLE, DEFAULT),
> -	PINCFG(QSPI_IO1_PEE3,        RSVD1,    DOWN,   TRISTATE, OUTPUT,  DISABLE, DEFAULT),
> -	PINCFG(QSPI_IO2_PEE4,        RSVD1,    DOWN,   TRISTATE, OUTPUT,  DISABLE, DEFAULT),
> -	PINCFG(QSPI_IO3_PEE5,        RSVD1,    DOWN,   TRISTATE, OUTPUT,  DISABLE, DEFAULT),
> +
> +	PINCFG(QSPI_SCK_PEE0,        QSPI,     NORMAL, NORMAL,   INPUT,   DISABLE, DEFAULT),
> +	PINCFG(QSPI_CS_N_PEE1,       QSPI,     NORMAL, NORMAL,   INPUT,   DISABLE, DEFAULT),
> +	PINCFG(QSPI_IO0_PEE2,        QSPI,     NORMAL, NORMAL,   INPUT,   DISABLE, DEFAULT),
> +	PINCFG(QSPI_IO1_PEE3,        QSPI,     NORMAL, NORMAL,   INPUT,   DISABLE, DEFAULT),
> +	PINCFG(QSPI_IO2_PEE4,        QSPI,     UP,     TRISTATE, OUTPUT,  DISABLE, DEFAULT),
> +	PINCFG(QSPI_IO3_PEE5,        QSPI,     UP,     TRISTATE, OUTPUT,  DISABLE, DEFAULT),

The current pinmux matches the syseng-supplied spreadsheet. If the spreadsheet is wrong, please get syseng to update the spreadsheet first. 
The pinmux files must not be manually edited, so we ensure that the mandated pinmux is used.

Valid point, I'm still used to changing the pinmux as desired to get new devices up. I'll pull down the latest P2571 spreadsheet and have it changed if it hasn't already been. Once it's corrected, I assume you can generate a new pinmux header and post a patch that I can put in ahead of the QSPI patches.

> diff --git a/drivers/spi/tegra210_qspi.c b/drivers/spi/tegra210_qspi.c 
> new file mode 100644 index 0000000..d827667


> +static int tegra210_qspi_ofdata_to_platdata(struct udevice *bus) {
> +	struct tegra_spi_platdata *plat = bus->platdata;
> +	const void *blob = gd->fdt_blob;
> +	int node = bus->of_offset;
> +
> +	plat->base = fdtdec_get_addr(blob, node, "reg");

I think that should use dev_get_addr() now, since that does the correct thing w.r.t. #address-cells/#size-cells.

Ok, I'll take a look. Thanks.

> +static int tegra210_qspi_claim_bus(struct udevice *bus) {

> +	/* Clear stale status here */
> +	setbits_le32(&regs->fifo_status,
> +		     QSPI_FIFO_STS_ERR		|
> +		     QSPI_FIFO_STS_TX_FIFO_OVF	|
> +		     QSPI_FIFO_STS_TX_FIFO_UNR	|
> +		     QSPI_FIFO_STS_RX_FIFO_OVF	|
> +		     QSPI_FIFO_STS_RX_FIFO_UNR	|
> +		     QSPI_FIFO_STS_TX_FIFO_FULL	|
> +		     QSPI_FIFO_STS_TX_FIFO_EMPTY	|
> +		     QSPI_FIFO_STS_RX_FIFO_FULL	|
> +		     QSPI_FIFO_STS_RX_FIFO_EMPTY);
> +	debug("%s: FIFO STATUS = %08x\n", __func__, 
> +readl(&regs->fifo_status));

Indentation issue.

I would expect at least that setbits_le32() to happen inside the transfer function so that every transfer is immune to stale status, not just the first transfer after the bus is cleared.

Actually, it looks like the transfer function already does this, so the above code can simply be dropped.

Perhaps the other HW-touching code might be moved to, but it's probably fine to leave it here.

Yen wrote and debugged this driver awhile back - I don't know why he wasn't included by 'send-email', but I've added him to the CC list.

Thanks,

Tom
--
nvpublic