On 19 November 2015 at 12:35, Mugunthan V N mugunthanvnm@ti.com wrote:

spi bus can support dual and quad wire data transfers for tx and rx. So defining dual and quad modes for both tx and rx. Also add support to parse bus width used for spi tx and rx transfers.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Tom Rini trini@konsulko.com

---

drivers/spi/spi-uclass.c | 33 +++++++++++++++++++++++++++++++++ include/spi.h | 4 ++++ 2 files changed, 37 insertions(+)

diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index 58388ef..62d8da7 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -349,6 +349,7 @@ int spi_slave_ofdata_to_platdata(const void *blob, int node, struct dm_spi_slave_platdata *plat) { int mode = 0;

•   int value;
  
    plat->cs = fdtdec_get_int(blob, node, "reg", -1);
    plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency", 0);
  

@@ -360,6 +361,38 @@ int spi_slave_ofdata_to_platdata(const void *blob, int node, mode |= SPI_CS_HIGH; if (fdtdec_get_bool(blob, node, "spi-half-duplex")) mode |= SPI_PREAMBLE;

•   /* Device DUAL/QUAD mode */
  

•   value = fdtdec_get_uint(blob, node, "spi-tx-bus-width", 1);
  

•   switch (value) {
  

•   case 1:
  

•           break;
  

•   case 2:
  

•           mode |= SPI_TX_DUAL;
  

•           break;
  

•   case 4:
  

•           mode |= SPI_TX_QUAD;
  

•           break;
  

•   default:
  

•           error("spi-tx-bus-width %d not supported\n", value);
  

•           break;
  

•   }
  

•   value = fdtdec_get_uint(blob, node, "spi-rx-bus-width", 1);
  

•   switch (value) {
  

•   case 1:
  

•           break;
  

•   case 2:
  

•           mode |= SPI_RX_DUAL;
  

•           break;
  

•   case 4:
  

•           mode |= SPI_RX_QUAD;
  

•           break;
  

•   default:
  

•           error("spi-rx-bus-width %d not supported\n", value);
  

•           break;
  

•   }
  

I think these are similar to SPI TX/RX operation modes in spi.h and I understand this is similar approach as with Linux but before this we need to do many changes on u-boot as well.

    plat->mode = mode;

    return 0;

diff --git a/include/spi.h b/include/spi.h index b4d2723..40dbb4d 100644 --- a/include/spi.h +++ b/include/spi.h @@ -23,6 +23,10 @@ #define SPI_LOOP 0x20 /* loopback mode */ #define SPI_SLAVE 0x40 /* slave mode */ #define SPI_PREAMBLE 0x80 /* Skip preamble bytes */ +#define SPI_TX_DUAL 0x100 /* transmit with 2 wires */ +#define SPI_TX_QUAD 0x200 /* transmit with 4 wires */ +#define SPI_RX_DUAL 0x400 /* receive with 2 wires */ +#define SPI_RX_QUAD 0x800 /* receive with 4 wires */

/* SPI transfer flags */

#define SPI_XFER_BEGIN 0x01 /* Assert CS before transfer */

2.6.2.280.g74301d6

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot

On 6 December 2015 at 11:33, Mugunthan V N mugunthanvnm@ti.com wrote:

On Thursday 03 December 2015 09:37 PM, Jagan Teki wrote:

...

On 19 November 2015 at 12:35, Mugunthan V N mugunthanvnm@ti.com wrote:

...

spi bus can support dual and quad wire data transfers for tx and rx. So defining dual and quad modes for both tx and rx. Also add support to parse bus width used for spi tx and rx transfers.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Tom Rini trini@konsulko.com

---

drivers/spi/spi-uclass.c | 33 +++++++++++++++++++++++++++++++++ include/spi.h | 4 ++++ 2 files changed, 37 insertions(+)

diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index 58388ef..62d8da7 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -349,6 +349,7 @@ int spi_slave_ofdata_to_platdata(const void *blob, int node, struct dm_spi_slave_platdata *plat) { int mode = 0;

•   int value;
  
    plat->cs = fdtdec_get_int(blob, node, "reg", -1);
    plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency", 0);
  

@@ -360,6 +361,38 @@ int spi_slave_ofdata_to_platdata(const void *blob, int node, mode |= SPI_CS_HIGH; if (fdtdec_get_bool(blob, node, "spi-half-duplex")) mode |= SPI_PREAMBLE;

•   /* Device DUAL/QUAD mode */
  

•   value = fdtdec_get_uint(blob, node, "spi-tx-bus-width", 1);
  

•   switch (value) {
  

•   case 1:
  

•           break;
  

•   case 2:
  

•           mode |= SPI_TX_DUAL;
  

•           break;
  

•   case 4:
  

•           mode |= SPI_TX_QUAD;
  

•           break;
  

•   default:
  

•           error("spi-tx-bus-width %d not supported\n", value);
  

•           break;
  

•   }
  

•   value = fdtdec_get_uint(blob, node, "spi-rx-bus-width", 1);
  

•   switch (value) {
  

•   case 1:
  

•           break;
  

•   case 2:
  

•           mode |= SPI_RX_DUAL;
  

•           break;
  

•   case 4:
  

•           mode |= SPI_RX_QUAD;
  

•           break;
  

•   default:
  

•           error("spi-rx-bus-width %d not supported\n", value);
  

•           break;
  

•   }
  

I think these are similar to SPI TX/RX operation modes in spi.h and I understand this is similar approach as with Linux but before this we need to do many changes on u-boot as well.

I agree that op_mode_rx/op_mode_tx can be used. I just tried to follow Linux way of implementation to have consistency between Linux and

Agreed but the way these mode or flags handling in Linux vs u-boot is different.

U-Boot. If you feel that using op_mode_(rx/tx) will be the best approach, I can repost the series with the changes.

If this is a dependent patch wrt series pls- do otherwise bypass this with your series now once we handle these mode/flags with Linux way will add till now will mark it as "Under review" on patchwork.

thanks!

On 8 December 2015 at 20:35, Mugunthan V N mugunthanvnm@ti.com wrote:

On Sunday 06 December 2015 08:13 PM, Jagan Teki wrote:

...

On 6 December 2015 at 11:33, Mugunthan V N mugunthanvnm@ti.com wrote:

...

On Thursday 03 December 2015 09:37 PM, Jagan Teki wrote:

...

On 19 November 2015 at 12:35, Mugunthan V N mugunthanvnm@ti.com wrote:

...

spi bus can support dual and quad wire data transfers for tx and rx. So defining dual and quad modes for both tx and rx. Also add support to parse bus width used for spi tx and rx transfers.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Tom Rini trini@konsulko.com

---

drivers/spi/spi-uclass.c | 33 +++++++++++++++++++++++++++++++++ include/spi.h | 4 ++++ 2 files changed, 37 insertions(+)

diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index 58388ef..62d8da7 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -349,6 +349,7 @@ int spi_slave_ofdata_to_platdata(const void *blob, int node, struct dm_spi_slave_platdata *plat) { int mode = 0;

•   int value;
  
    plat->cs = fdtdec_get_int(blob, node, "reg", -1);
    plat->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency", 0);
  

@@ -360,6 +361,38 @@ int spi_slave_ofdata_to_platdata(const void *blob, int node, mode |= SPI_CS_HIGH; if (fdtdec_get_bool(blob, node, "spi-half-duplex")) mode |= SPI_PREAMBLE;

•   /* Device DUAL/QUAD mode */
  

•   value = fdtdec_get_uint(blob, node, "spi-tx-bus-width", 1);
  

•   switch (value) {
  

•   case 1:
  

•           break;
  

•   case 2:
  

•           mode |= SPI_TX_DUAL;
  

•           break;
  

•   case 4:
  

•           mode |= SPI_TX_QUAD;
  

•           break;
  

•   default:
  

•           error("spi-tx-bus-width %d not supported\n", value);
  

•           break;
  

•   }
  

•   value = fdtdec_get_uint(blob, node, "spi-rx-bus-width", 1);
  

•   switch (value) {
  

•   case 1:
  

•           break;
  

•   case 2:
  

•           mode |= SPI_RX_DUAL;
  

•           break;
  

•   case 4:
  

•           mode |= SPI_RX_QUAD;
  

•           break;
  

•   default:
  

•           error("spi-rx-bus-width %d not supported\n", value);
  

•           break;
  

•   }
  

I think these are similar to SPI TX/RX operation modes in spi.h and I understand this is similar approach as with Linux but before this we need to do many changes on u-boot as well.

I agree that op_mode_rx/op_mode_tx can be used. I just tried to follow Linux way of implementation to have consistency between Linux and

Agreed but the way these mode or flags handling in Linux vs u-boot is different.

...

U-Boot. If you feel that using op_mode_(rx/tx) will be the best approach, I can repost the series with the changes.

If this is a dependent patch wrt series pls- do otherwise bypass this with your series now once we handle these mode/flags with Linux way will add till now will mark it as "Under review" on patchwork.

08/16 patch is dependent on this patch where the mode is used to select dual/quad mode. I will respin 05 (this patch) and 08 patch as a follow-up patch.

Then go-ahead and reuse the existing macro's.

thanks!

adopt ti_qspi driver to device driver model

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Tom Rini trini@konsulko.com ---

Changes from v3->v4: * Use op_mode_{t/r}x for flash tx/rx operation modes.

This has been tested on am437x-sk evm logs [1] and pushed a branch for others to test [2]

[1] - http://pastebin.ubuntu.com/14024895/ [2] - git://git.ti.com/~mugunthanvnm/ti-u-boot/mugunth-ti-u-boot.git dm-qspi-v4

--- drivers/spi/ti_qspi.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 187 insertions(+)

diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c index 41add38..5bacd7a 100644 --- a/drivers/spi/ti_qspi.c +++ b/drivers/spi/ti_qspi.c @@ -11,11 +11,14 @@ #include <asm/arch/omap.h> #include <malloc.h> #include <spi.h> +#include <dm.h> #include <asm/gpio.h> #include <asm/omap_gpio.h> #include <asm/omap_common.h> #include <asm/ti-common/ti-edma3.h>

+DECLARE_GLOBAL_DATA_PTR; + /* ti qpsi register bit masks */ #define QSPI_TIMEOUT 2000000 #define QSPI_FCLK 192000000 @@ -48,12 +51,14 @@ #define CORE_CTRL_IO 0x4a002558

#define QSPI_CMD_READ (0x3 << 0) +#define QSPI_CMD_READ_DUAL (0x6b << 0) #define QSPI_CMD_READ_QUAD (0x6b << 0) #define QSPI_CMD_READ_FAST (0x0b << 0) #define QSPI_SETUP0_NUM_A_BYTES (0x2 << 8) #define QSPI_SETUP0_NUM_D_BYTES_NO_BITS (0x0 << 10) #define QSPI_SETUP0_NUM_D_BYTES_8_BITS (0x1 << 10) #define QSPI_SETUP0_READ_NORMAL (0x0 << 12) +#define QSPI_SETUP0_READ_DUAL (0x1 << 12) #define QSPI_SETUP0_READ_QUAD (0x3 << 12) #define QSPI_CMD_WRITE (0x2 << 16) #define QSPI_NUM_DUMMY_BITS (0x0 << 24) @@ -87,7 +92,13 @@ struct ti_qspi_regs {

/* ti qspi priv */ struct ti_qspi_priv { +#ifndef CONFIG_DM_SPI struct spi_slave slave; +#else + void *memory_map; + uint max_hz; + u32 num_cs; +#endif struct ti_qspi_regs *base; void *ctrl_mod_mmap; unsigned int mode; @@ -288,6 +299,8 @@ void spi_flash_copy_mmap(void *data, void *offset, size_t len) } #endif

+#ifndef CONFIG_DM_SPI + static inline struct ti_qspi_priv *to_ti_qspi_priv(struct spi_slave *slave) { return container_of(slave, struct ti_qspi_priv, slave); @@ -401,3 +414,177 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, priv->slave.bus, priv->slave.cs, bitlen, flags); return __ti_qspi_xfer(priv, bitlen, dout, din, flags, priv->slave.cs); } + +#else /* CONFIG_DM_SPI */ + +static void __ti_qspi_setup_memorymap(struct ti_qspi_priv *priv, + struct spi_slave *slave, + bool enable) +{ + u32 memval; + u32 mode = slave->op_mode_rx & (SPI_OPM_RX_QUAD | SPI_OPM_RX_DUAL); + + if (!enable) { + writel(0, &priv->base->setup0); + return; + } + + memval = QSPI_SETUP0_NUM_A_BYTES | QSPI_CMD_WRITE | QSPI_NUM_DUMMY_BITS; + + switch (mode) { + case SPI_OPM_RX_QUAD: + memval |= QSPI_CMD_READ_QUAD; + memval |= QSPI_SETUP0_NUM_D_BYTES_8_BITS; + memval |= QSPI_SETUP0_READ_QUAD; + slave->op_mode_rx = SPI_OPM_RX_QOF; + break; + case SPI_OPM_RX_DUAL: + memval |= QSPI_CMD_READ_DUAL; + memval |= QSPI_SETUP0_NUM_D_BYTES_8_BITS; + memval |= QSPI_SETUP0_READ_DUAL; + break; + default: + memval |= QSPI_CMD_READ; + memval |= QSPI_SETUP0_NUM_D_BYTES_NO_BITS; + memval |= QSPI_SETUP0_READ_NORMAL; + break; + } + + writel(memval, &priv->base->setup0); +} + + +static int ti_qspi_set_speed(struct udevice *bus, uint max_hz) +{ + struct ti_qspi_priv *priv = dev_get_priv(bus); + + ti_spi_set_speed(priv, max_hz); + + return 0; +} + +static int ti_qspi_set_mode(struct udevice *bus, uint mode) +{ + struct ti_qspi_priv *priv = dev_get_priv(bus); + return __ti_qspi_set_mode(priv, mode); +} + +static int ti_qspi_claim_bus(struct udevice *dev) +{ + struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev); + struct spi_slave *slave = dev_get_parent_priv(dev); + struct ti_qspi_priv *priv; + struct udevice *bus; + + bus = dev->parent; + priv = dev_get_priv(bus); + + if (slave_plat->cs > priv->num_cs) { + debug("invalid qspi chip select\n"); + return -EINVAL; + } + + __ti_qspi_setup_memorymap(priv, slave, true); + + return __ti_qspi_claim_bus(priv, slave_plat->cs); +} + +static int ti_qspi_release_bus(struct udevice *dev) +{ + struct spi_slave *slave = dev_get_parent_priv(dev); + struct ti_qspi_priv *priv; + struct udevice *bus; + + bus = dev->parent; + priv = dev_get_priv(bus); + + __ti_qspi_setup_memorymap(priv, slave, false); + __ti_qspi_release_bus(priv); + + return 0; +} + +static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen, + const void *dout, void *din, unsigned long flags) +{ + struct dm_spi_slave_platdata *slave = dev_get_parent_platdata(dev); + struct ti_qspi_priv *priv; + struct udevice *bus; + + bus = dev->parent; + priv = dev_get_priv(bus); + + if (slave->cs > priv->num_cs) { + debug("invalid qspi chip select\n"); + return -EINVAL; + } + + return __ti_qspi_xfer(priv, bitlen, dout, din, flags, slave->cs); +} + +static int ti_qspi_probe(struct udevice *bus) +{ + /* Nothing to do in probe */ + return 0; +} + +static int ti_qspi_ofdata_to_platdata(struct udevice *bus) +{ + struct ti_qspi_priv *priv = dev_get_priv(bus); + const void *blob = gd->fdt_blob; + int node = bus->of_offset; + fdt_addr_t addr; + + priv->base = (struct ti_qspi_regs *)dev_get_addr(bus); + priv->memory_map = (void *)dev_get_addr_index(bus, 1); + addr = dev_get_addr_index(bus, 2); + priv->ctrl_mod_mmap = (addr == FDT_ADDR_T_NONE) ? NULL : (void *)addr; + + priv->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency", -1); + if (priv->max_hz < 0) { + debug("Error: Max frequency missing\n"); + return -ENODEV; + } + priv->num_cs = fdtdec_get_int(blob, node, "num-cs", 4); + + debug("%s: regs=<0x%x>, max-frequency=%d\n", __func__, + (int)priv->base, priv->max_hz); + + return 0; +} + +static int ti_qspi_child_pre_probe(struct udevice *dev) +{ + struct spi_slave *slave = dev_get_parent_priv(dev); + struct udevice *bus = dev_get_parent(dev); + struct ti_qspi_priv *priv = dev_get_priv(bus); + + slave->memory_map = priv->memory_map; + return 0; +} + +static const struct dm_spi_ops ti_qspi_ops = { + .claim_bus = ti_qspi_claim_bus, + .release_bus = ti_qspi_release_bus, + .xfer = ti_qspi_xfer, + .set_speed = ti_qspi_set_speed, + .set_mode = ti_qspi_set_mode, +}; + +static const struct udevice_id ti_qspi_ids[] = { + { .compatible = "ti,dra7xxx-qspi" }, + { .compatible = "ti,am4372-qspi" }, + { } +}; + +U_BOOT_DRIVER(ti_qspi) = { + .name = "ti_qspi", + .id = UCLASS_SPI, + .of_match = ti_qspi_ids, + .ops = &ti_qspi_ops, + .ofdata_to_platdata = ti_qspi_ofdata_to_platdata, + .priv_auto_alloc_size = sizeof(struct ti_qspi_priv), + .probe = ti_qspi_probe, + .child_pre_probe = ti_qspi_child_pre_probe, +}; +#endif /* CONFIG_DM_SPI */

Jagan

On Thursday 19 November 2015 03:40 PM, Jagan Teki wrote:

On 19 November 2015 at 12:35, Mugunthan V N mugunthanvnm@ti.com wrote:

...

Add compatible for spansion 32MiB spi flash s25fl256s1.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

drivers/mtd/spi/sf_probe.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index bc05d30..ffbce59 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -515,6 +515,7 @@ static const struct dm_spi_flash_ops spi_flash_std_ops = {

static const struct udevice_id spi_flash_std_ids[] = { { .compatible = "spi-flash" },

•   { .compatible = "s25fl256s1" },
    { }
  

Never compare with spi-flash vs s25fl256s1 here former is driver compatible string and later one is probed flash string name. we don't need to add them in compatible list, better to add it on dts node otherwise no issue the connected flash will detect while 'sf probe'

I agree that spi flash will be probed even without addition of compatibles.

If there is no matching compatible between the dtb and driver, the dt properties like spi-max-frequency, spi clock phase/polarity and spi bus-width are not parsed from DT as the of_offset of the spi device will be '-1'. This check and dt parse is done in spi_child_post_bind() in spi-uclass driver.

Since spi bus-width is not read from DT, by default spi transfers are done in one wire mode and read throughput drops to 5.3Mbps. With Quad mode read throughput is 16.6Mbps in DRA74x EVM.

Adding spi-splash to DT is not possible now as it is not documented in Kernel binding documentation. Even if "spi-flash" compatible is added to u-boot DT then it will show diff when syncing the dts files from Kernel to U-boot and it has to be taken care for every sync. Tom Rini also has agreed to this [1].

[1]: http://patchwork.ozlabs.org/patch/539780/

Regards Mugunthan V N

On Friday 20 November 2015 05:57 PM, Jagan Teki wrote:

On 20 November 2015 at 11:31, Mugunthan V N mugunthanvnm@ti.com wrote:

...

Jagan

On Thursday 19 November 2015 03:40 PM, Jagan Teki wrote:

...

On 19 November 2015 at 12:35, Mugunthan V N mugunthanvnm@ti.com wrote:

...

Add compatible for spansion 32MiB spi flash s25fl256s1.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

drivers/mtd/spi/sf_probe.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index bc05d30..ffbce59 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -515,6 +515,7 @@ static const struct dm_spi_flash_ops spi_flash_std_ops = {

static const struct udevice_id spi_flash_std_ids[] = { { .compatible = "spi-flash" },

•   { .compatible = "s25fl256s1" },
    { }
  

Never compare with spi-flash vs s25fl256s1 here former is driver compatible string and later one is probed flash string name. we don't need to add them in compatible list, better to add it on dts node otherwise no issue the connected flash will detect while 'sf probe'

I agree that spi flash will be probed even without addition of compatibles.

If there is no matching compatible between the dtb and driver, the dt properties like spi-max-frequency, spi clock phase/polarity and spi bus-width are not parsed from DT as the of_offset of the spi device will be '-1'. This check and dt parse is done in spi_child_post_bind() in spi-uclass driver.

Since spi bus-width is not read from DT, by default spi transfers are done in one wire mode and read throughput drops to 5.3Mbps. With Quad mode read throughput is 16.6Mbps in DRA74x EVM.

Why can't we try something like this [1] [2] I do agree with documentation missing, may be we add add that well.

[1] arch/arm/dts/socfpga_cyclone5_sockit.dts

This dts file is not in sync with kernel dts. There is no qspi support in kernel. It is going to be really tough time to sync this dts between kernel and u-boot ;)

[2] arch/arm/dts/zynqmp-ep108.dts

I don't see compatible string "spi-flash" in zynqmp-ep108.dts

Adding spi-flash compatible will be tough as spi-flash works with the existing compatibles and convincing spi maintainer (Mark Brown) for adding a generic compatible will be difficult.

If you still insist to add spi-flash compatible to the dts file and *Tom Rini* is also fine, I will re-spin the series with compatible changes to the dts files.

Regards Mugunthan V N

Hi Mugunthan,

On 23 November 2015 at 18:42, Jagan Teki jteki@openedev.com wrote:

On 23 November 2015 at 16:59, Mugunthan V N mugunthanvnm@ti.com wrote:

...

On Friday 20 November 2015 05:57 PM, Jagan Teki wrote:

...

On 20 November 2015 at 11:31, Mugunthan V N mugunthanvnm@ti.com wrote:

...

Jagan

On Thursday 19 November 2015 03:40 PM, Jagan Teki wrote:

...

On 19 November 2015 at 12:35, Mugunthan V N mugunthanvnm@ti.com wrote:

...

Add compatible for spansion 32MiB spi flash s25fl256s1.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

drivers/mtd/spi/sf_probe.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index bc05d30..ffbce59 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -515,6 +515,7 @@ static const struct dm_spi_flash_ops spi_flash_std_ops = {

static const struct udevice_id spi_flash_std_ids[] = { { .compatible = "spi-flash" },

•   { .compatible = "s25fl256s1" },
    { }
  

Never compare with spi-flash vs s25fl256s1 here former is driver compatible string and later one is probed flash string name. we don't need to add them in compatible list, better to add it on dts node otherwise no issue the connected flash will detect while 'sf probe'

I agree that spi flash will be probed even without addition of compatibles.

If there is no matching compatible between the dtb and driver, the dt properties like spi-max-frequency, spi clock phase/polarity and spi bus-width are not parsed from DT as the of_offset of the spi device will be '-1'. This check and dt parse is done in spi_child_post_bind() in spi-uclass driver.

Since spi bus-width is not read from DT, by default spi transfers are done in one wire mode and read throughput drops to 5.3Mbps. With Quad mode read throughput is 16.6Mbps in DRA74x EVM.

Why can't we try something like this [1] [2] I do agree with documentation missing, may be we add add that well.

[1] arch/arm/dts/socfpga_cyclone5_sockit.dts

This dts file is not in sync with kernel dts. There is no qspi support in kernel. It is going to be really tough time to sync this dts between kernel and u-boot ;)

Since we are in starting point of adding dts files (unlike Linux) these dts files may not sync with Linux, we started adding these to make dm or related drivers need to work. Yes I agreed with your point and we really need to work on that as well.

...

...

[2] arch/arm/dts/zynqmp-ep108.dts

I don't see compatible string "spi-flash" in zynqmp-ep108.dts

Yes, cant you add "s25fl256s1" like n25q512a11 added in this dts.

...

Adding spi-flash compatible will be tough as spi-flash works with the existing compatibles and convincing spi maintainer (Mark Brown) for adding a generic compatible will be difficult.

This is what you pointed out - id_table.

static struct spi_driver m25p80_driver = { .driver = { .name = "m25p80", .of_match_table = m25p_of_table, }, .id_table = m25p_ids,

Comments?

...

If you still insist to add spi-flash compatible to the dts file and *Tom Rini* is also fine, I will re-spin the series with compatible changes to the dts files.

I'm saying like s25fl256s1 few of compatible strings were added already on existing dts files, probably we may proceed with the same for now. will trimming up later.

Any comments? proceed with above suggestion.

thanks!

On Mon, Nov 23, 2015 at 04:59:12PM +0530, Mugunthan V N wrote:

On Friday 20 November 2015 05:57 PM, Jagan Teki wrote:

...

On 20 November 2015 at 11:31, Mugunthan V N mugunthanvnm@ti.com wrote:

...

Jagan

On Thursday 19 November 2015 03:40 PM, Jagan Teki wrote:

...

On 19 November 2015 at 12:35, Mugunthan V N mugunthanvnm@ti.com wrote:

...

Add compatible for spansion 32MiB spi flash s25fl256s1.

Signed-off-by: Mugunthan V N mugunthanvnm@ti.com

drivers/mtd/spi/sf_probe.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c index bc05d30..ffbce59 100644 --- a/drivers/mtd/spi/sf_probe.c +++ b/drivers/mtd/spi/sf_probe.c @@ -515,6 +515,7 @@ static const struct dm_spi_flash_ops spi_flash_std_ops = {

static const struct udevice_id spi_flash_std_ids[] = { { .compatible = "spi-flash" },

•   { .compatible = "s25fl256s1" },
    { }
  

Never compare with spi-flash vs s25fl256s1 here former is driver compatible string and later one is probed flash string name. we don't need to add them in compatible list, better to add it on dts node otherwise no issue the connected flash will detect while 'sf probe'

I agree that spi flash will be probed even without addition of compatibles.

If there is no matching compatible between the dtb and driver, the dt properties like spi-max-frequency, spi clock phase/polarity and spi bus-width are not parsed from DT as the of_offset of the spi device will be '-1'. This check and dt parse is done in spi_child_post_bind() in spi-uclass driver.

Since spi bus-width is not read from DT, by default spi transfers are done in one wire mode and read throughput drops to 5.3Mbps. With Quad mode read throughput is 16.6Mbps in DRA74x EVM.

Why can't we try something like this [1] [2] I do agree with documentation missing, may be we add add that well.

[1] arch/arm/dts/socfpga_cyclone5_sockit.dts

This dts file is not in sync with kernel dts. There is no qspi support in kernel. It is going to be really tough time to sync this dts between kernel and u-boot ;)

True, but you're pushing that too, right? I suppose the high level take-away here is that when you're posting the bindings for Linux you need to keep some of the U-Boot folks in CC as well. So first up, where / how far along is the kernel support today?