Introduce driver model support for SPI, SPI flash, cros_ec

At present sandbox has its own table of supported SPI flash chips. Now that the SPI flash system is fully consolidated and has its own list, sandbox should use that.

This enables us to expand the number of chips that sandbox supports.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/mtd/spi/sandbox.c | 114 ++++++++++++++++------------------------------
 1 file changed, 38 insertions(+), 76 deletions(-)

drivers/mtd/spi/sandbox.c +38 -77

index a62ef4c..98e0a34 100644
@@ -51,46 +51,7 @@ static const char *sandbox_sf_state_name(enum sandbox_sf_state state)
 /* Assume all SPI flashes have 3 byte addresses since they do atm */
 #define SF_ADDR_LEN    3
 
-struct sandbox_spi_flash_erase_commands {
-       u8 cmd;
-       u32 size;
-};
-#define IDCODE_LEN 5
-#define MAX_ERASE_CMDS 3
-struct sandbox_spi_flash_data {
-       const char *name;
-       u8 idcode[IDCODE_LEN];
-       u32 size;
-       const struct sandbox_spi_flash_erase_commands
-                                               erase_cmds[MAX_ERASE_CMDS];
-};
-
-/* Structure describing all the flashes we know how to emulate */
-static const struct sandbox_spi_flash_data sandbox_sf_flashes[] = {
-       {
-               "M25P16", { 0x20, 0x20, 0x15 }, (2 << 20),
-               {       /* erase commands */
-                       { 0xd8, (64 << 10), }, /* sector */
-                       { 0xc7, (2 << 20), }, /* bulk */
-               },
-       },
-       {
-               "W25Q32", { 0xef, 0x40, 0x16 }, (4 << 20),
-               {       /* erase commands */
-                       { 0x20, (4 << 10), }, /* 4KB */
-                       { 0xd8, (64 << 10), }, /* sector */
-                       { 0xc7, (4 << 20), }, /* bulk */
-               },
-       },
-       {
-               "W25Q128", { 0xef, 0x40, 0x18 }, (16 << 20),
-               {       /* erase commands */
-                       { 0x20, (4 << 10), }, /* 4KB */
-                       { 0xd8, (64 << 10), }, /* sector */
-                       { 0xc7, (16 << 20), }, /* bulk */
-               },
-       },
-};
+#define IDCODE_LEN 3
 
 /* Used to quickly bulk erase backing store */
 static u8 sandbox_sf_0xff[0x1000];

@@ -109,7 +70,8 @@ struct sandbox_spi_flash {
         */
        enum sandbox_sf_state state;
        uint cmd;
-       const void *cmd_data;
+       /* Erase size of current erase command */
+       uint erase_size;
        /* Current position in the flash; used when reading/writing/etc... */
        uint off;
        /* How many address bytes we've consumed */

@@ -117,7 +79,7 @@ struct sandbox_spi_flash {
        /* The current flash status (see STAT_XXX defines above) */
        u16 status;
        /* Data describing the flash we're emulating */
-       const struct sandbox_spi_flash_data *data;
+       const struct spi_flash_params *data;
        /* The file on disk to serv up data from */
        int fd;
 };

@@ -127,8 +89,8 @@ static int sandbox_sf_setup(void **priv, const char *spec)
        /* spec = idcode:file */
        struct sandbox_spi_flash *sbsf;
        const char *file;
-       size_t i, len, idname_len;
-       const struct sandbox_spi_flash_data *data;
+       size_t len, idname_len;
+       const struct spi_flash_params *data;
 
        file = strchr(spec, ':');
        if (!file) {

@@ -138,15 +100,14 @@ static int sandbox_sf_setup(void **priv, const char *spec)
        idname_len = file - spec;
        ++file;
 
-       for (i = 0; i < ARRAY_SIZE(sandbox_sf_flashes); ++i) {
-               data = &sandbox_sf_flashes[i];
+       for (data = spi_flash_params_table; data->name; data++) {
                len = strlen(data->name);
                if (idname_len != len)
                        continue;
                if (!memcmp(spec, data->name, len))
                        break;
        }
-       if (i == ARRAY_SIZE(sandbox_sf_flashes)) {
+       if (!data->name) {
                printf("sandbox_sf: unknown flash '%*s'\n", (int)idname_len,
                       spec);
                goto error;

@@ -223,7 +184,6 @@ static int sandbox_sf_process_cmd(struct sandbox_spi_flash *sbsf, const u8 *rx,
                sbsf->pad_addr_bytes = 1;
        case CMD_READ_ARRAY_SLOW:
        case CMD_PAGE_PROGRAM:
- state_addr:
                sbsf->state = SF_ADDR;
                break;
        case CMD_WRITE_DISABLE:

@@ -241,24 +201,25 @@ static int sandbox_sf_process_cmd(struct sandbox_spi_flash *sbsf, const u8 *rx,
                sbsf->status |= STAT_WEL;
                break;
        default: {
-               size_t i;
-
-               /* handle erase commands first */
-               for (i = 0; i < MAX_ERASE_CMDS; ++i) {
-                       const struct sandbox_spi_flash_erase_commands *
-                               erase_cmd = &sbsf->data->erase_cmds[i];
-
-                       if (erase_cmd->cmd == 0x00)
-                               continue;
-                       if (sbsf->cmd != erase_cmd->cmd)
-                               continue;
-
-                       sbsf->cmd_data = erase_cmd;
-                       goto state_addr;
+               int flags = sbsf->data->flags;
+
+               /* we only support erase here */
+               if (sbsf->cmd == CMD_ERASE_CHIP) {
+                       sbsf->erase_size = sbsf->data->sector_size *
+                               sbsf->data->nr_sectors;
+               } else if (sbsf->cmd == CMD_ERASE_4K && (flags & SECT_4K)) {
+                       sbsf->erase_size = 4 << 10;
+               } else if (sbsf->cmd == CMD_ERASE_32K && (flags & SECT_32K)) {
+                       sbsf->erase_size = 32 << 10;
+               } else if (sbsf->cmd == CMD_ERASE_64K &&
+                          !(flags & (SECT_4K | SECT_32K))) {
+                       sbsf->erase_size = 64 << 10;
+               } else {
+                       debug(" cmd unknown: %#x\n", sbsf->cmd);
+                       return 1;
                }
-
-               debug(" cmd unknown: %#x\n", sbsf->cmd);
-               return 1;
+               sbsf->state = SF_ADDR;
+               break;
        }
        }
 

@@ -309,11 +270,14 @@ static int sandbox_sf_xfer(void *priv, const u8 *rx, u8 *tx,
                        u8 id;
 
                        debug(" id: off:%u tx:", sbsf->off);
-                       if (sbsf->off < IDCODE_LEN)
-                               id = sbsf->data->idcode[sbsf->off];
-                       else
+                       if (sbsf->off < IDCODE_LEN) {
+                               /* Extract correct byte from ID 0x00aabbcc */
+                               id = sbsf->data->jedec >>
+                                       (8 * (IDCODE_LEN - 1 - sbsf->off));
+                       } else {
                                id = 0;
-                       debug("%02x\n", id);
+                       }
+                       debug("%d %02x\n", sbsf->off, id);
                        tx[pos++] = id;
                        ++sbsf->off;
                        break;

@@ -406,24 +370,22 @@ static int sandbox_sf_xfer(void *priv, const u8 *rx, u8 *tx,
                        break;
                case SF_ERASE:
  case_sf_erase: {
-                       const struct sandbox_spi_flash_erase_commands *
-                                               erase_cmd = sbsf->cmd_data;
-
                        if (!(sbsf->status & STAT_WEL)) {
                                puts("sandbox_sf: write enable not set before erase\n");
                                goto done;
                        }
 
                        /* verify address is aligned */
-                       if (sbsf->off & (erase_cmd->size - 1)) {
+                       if (sbsf->off & (sbsf->erase_size - 1)) {
                                debug(" sector erase: cmd:%#x needs align:%#x, but we got %#x\n",
-                                     erase_cmd->cmd, erase_cmd->size,
+                                     sbsf->cmd, sbsf->erase_size,
                                      sbsf->off);
                                sbsf->status &= ~STAT_WEL;
                                goto done;
                        }
 
-                       debug(" sector erase addr: %u\n", sbsf->off);
+                       debug(" sector erase addr: %u, size: %u\n", sbsf->off,
+                             sbsf->erase_size);
 
                        cnt = bytes - pos;
                        sandbox_spi_tristate(&tx[pos], cnt);

@@ -433,7 +395,7 @@ static int sandbox_sf_xfer(void *priv, const u8 *rx, u8 *tx,
                         * TODO(vapier at gentoo.org): latch WIP in status, and
                         * delay before clearing it ?
                         */
-                       ret = sandbox_erase_part(sbsf, erase_cmd->size);
+                       ret = sandbox_erase_part(sbsf, sbsf->erase_size);
                        sbsf->status &= ~STAT_WEL;
                        if (ret) {
                                debug("sandbox_sf: Erase failed\n");
--

2.0.0.526.g5318336

Sandbox may as well support everything. This increases the amount of code that is built/tested by sandbox, and also provides access to all the supported SPI flash devices.

Signed-off-by Simon Glass <[email protected]>

---

 include/configs/sandbox.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

include/configs/sandbox.h +7 -2

index f5fa4b3..5e4e5fc 100644
@@ -94,7 +94,7 @@
 #define CONFIG_ENV_SIZE                8192
 #define CONFIG_ENV_IS_NOWHERE
 
-/* SPI */
+/* SPI - enable all SPI flash types for testing purposes */
 #define CONFIG_SANDBOX_SPI
 #define CONFIG_CMD_SF
 #define CONFIG_CMD_SF_TEST

@@ -102,7 +102,13 @@
 #define CONFIG_SPI_FLASH
 #define CONFIG_OF_SPI
 #define CONFIG_OF_SPI_FLASH
+#define CONFIG_SPI_FLASH_ATMEL
+#define CONFIG_SPI_FLASH_EON
+#define CONFIG_SPI_FLASH_GIGADEVICE
+#define CONFIG_SPI_FLASH_MACRONIX
 #define CONFIG_SPI_FLASH_SANDBOX
+#define CONFIG_SPI_FLASH_SPANSION
+#define CONFIG_SPI_FLASH_SST
 #define CONFIG_SPI_FLASH_STMICRO
 #define CONFIG_SPI_FLASH_WINBOND
 
--

2.0.0.526.g5318336

Add a SPI device which can be used for testing SPI flash features in sandbox.

Also add a cros_ec device since with driver model the Chrome OS EC emulation will not otherwise be available.

Signed-off-by Simon Glass <[email protected]>

---

 arch/sandbox/dts/sandbox.dts                | 25 +++++++++++++++++++++++++
 doc/device-tree-bindings/mtd/spi/m25p80.txt | 29 +++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)
 create mode 100644 doc/device-tree-bindings/mtd/spi/m25p80.txt

arch/sandbox/dts/sandbox.dts +25 -0

index daf5e73..fad1fae 100644
@@ -1,6 +1,9 @@
 /dts-v1/;
 
 / {
+       #address-cells = <1>;
+       #size-cells = <0>;
+
        aliases {
                console = "/serial";
        };

@@ -131,5 +134,27 @@
                num-gpios = <20>;
        };
 
+       spi at 0 {
+               #address-cells = <1>;
+               #size-cells = <0>;
+               reg = <0>;
+               compatible = "sandbox,spi";
+               cs-gpios = <0>, <&gpio_a 0>;
+               flash at 0 {
+                       reg = <0>;
+                       compatible = "spansion,m25p16", "sandbox,spi-flash";
+                       spi-max-frequency = <40000000>;
+                       sandbox,filename = "spi.bin";
+               };
+       };
+
+       cros-ec at 0 {
+               compatible = "google,cros-ec";
+               #address-cells = <1>;
+               #size-cells = <1>;
+               firmware_storage_spi: flash at 0 {
+                       reg = <0 0x400000>;
+               };
+       };
 
 };

doc/device-tree-bindings/mtd/spi/m25p80.txt new file +29 -1

new file mode 100644
index 0000000..6d3d576
@@ -0,0 +1,29 @@
+* MTD SPI driver for ST M25Pxx (and similar) serial flash chips
+
+Required properties:
+- #address-cells, #size-cells : Must be present if the device has sub-nodes
+  representing partitions.
+- compatible : Should be the manufacturer and the name of the chip. Bear in mind
+               the DT binding is not Linux-only, but in case of Linux, see the
+               "m25p_ids" table in drivers/mtd/devices/m25p80.c for the list of
+               supported chips.
+- reg : Chip-Select number
+- spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at
+
+Optional properties:
+- m25p,fast-read : Use the "fast read" opcode to read data from the chip instead
+                   of the usual "read" opcode. This opcode is not supported by
+                   all chips and support for it can not be detected at runtime.
+                   Refer to your chips' datasheet to check if this is supported
+                   by your chip.
+
+Example:
+
+       flash: m25p80 at 0 {
+               #address-cells = <1>;
+               #size-cells = <1>;
+               compatible = "spansion,m25p80";
+               reg = <0>;
+               spi-max-frequency = <40000000>;
+               m25p,fast-read;
+       };
--

2.0.0.526.g5318336

In preparation for changing the error handling in this code for driver model, move it into its own function.

Signed-off-by Simon Glass <[email protected]>

---

 common/cmd_spi.c | 53 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 21 deletions(-)

common/cmd_spi.c +32 -22

index 3c8e913..be5709c 100644
@@ -11,6 +11,7 @@
 
 #include <common.h>
 #include <command.h>
+#include <errno.h>
 #include <spi.h>
 
 /*-----------------------------------------------------------------------

@@ -38,6 +39,35 @@ static int                   bitlen;
 static uchar           dout[MAX_SPI_BYTES];
 static uchar           din[MAX_SPI_BYTES];
 
+static int do_spi_xfer(int bus, int cs)
+{
+       struct spi_slave *slave;
+       int rcode = 0;
+
+       slave = spi_setup_slave(bus, cs, 1000000, mode);
+       if (!slave) {
+               printf("Invalid device %d:%d\n", bus, cs);
+               return -EINVAL;
+       }
+
+       spi_claim_bus(slave);
+       if (spi_xfer(slave, bitlen, dout, din,
+                    SPI_XFER_BEGIN | SPI_XFER_END) != 0) {
+               printf("Error during SPI transaction\n");
+               rcode = -EIO;
+       } else {
+               int j;
+
+               for (j = 0; j < ((bitlen + 7) / 8); j++)
+                       printf("%02X", din[j]);
+               printf("\n");
+       }
+       spi_release_bus(slave);
+       spi_free_slave(slave);
+
+       return rcode;
+}
+
 /*
  * SPI read/write
  *

@@ -51,11 +81,9 @@ static uchar                 din[MAX_SPI_BYTES];
 
 int do_spi (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
-       struct spi_slave *slave;
        char  *cp = 0;
        uchar tmp;
        int   j;
-       int   rcode = 0;
 
        /*
         * We use the last specified parameters, unless new ones are

@@ -103,27 +131,10 @@ int do_spi (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
                return 1;
        }
 
-       slave = spi_setup_slave(bus, cs, 1000000, mode);
-       if (!slave) {
-               printf("Invalid device %d:%d\n", bus, cs);
+       if (do_spi_xfer(bus, cs))
                return 1;
-       }
-
-       spi_claim_bus(slave);
-       if(spi_xfer(slave, bitlen, dout, din,
-                               SPI_XFER_BEGIN | SPI_XFER_END) != 0) {
-               printf("Error during SPI transaction\n");
-               rcode = 1;
-       } else {
-               for(j = 0; j < ((bitlen + 7) / 8); j++) {
-                       printf("%02X", din[j]);
-               }
-               printf("\n");
-       }
-       spi_release_bus(slave);
-       spi_free_slave(slave);
 
-       return rcode;
+       return 0;
 }
 
 /***************************************************/
--

2.0.0.526.g5318336

Quoted 3 lines · Simon Glass

In preparation for changing the error handling in this code for driver model, move it into its own function.

Quoted 6 lines

Signed-off-by Simon Glass <[email protected]>

---

 common/cmd_spi.c | 53 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 32 insertions(+), 21 deletions(-)

Reviewed-by Jagannadha Sutradharudu Teki <[email protected]>

Some of the #defines in spi.h are not bracketed. To avoid future mistakes add brackets. Also add an explanatory comment for SPI_CONN_DUAL_...

Signed-off-by Simon Glass <[email protected]>

---

 include/spi.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

include/spi.h +12 -13

index ffd6647..b673be2 100644
@@ -30,24 +30,24 @@
 #define SPI_XFER_MMAP          0x08    /* Memory Mapped start */
 #define SPI_XFER_MMAP_END      0x10    /* Memory Mapped End */
 #define SPI_XFER_ONCE          (SPI_XFER_BEGIN | SPI_XFER_END)
-#define SPI_XFER_U_PAGE                (1 << 5)
+#define SPI_XFER_U_PAGE        (1 << 5)
 
 /* SPI TX operation modes */
-#define SPI_OPM_TX_QPP         1 << 0
+#define SPI_OPM_TX_QPP         (1 << 0)
 
 /* SPI RX operation modes */
-#define SPI_OPM_RX_AS          1 << 0
-#define SPI_OPM_RX_DOUT                1 << 1
-#define SPI_OPM_RX_DIO         1 << 2
-#define SPI_OPM_RX_QOF         1 << 3
-#define SPI_OPM_RX_QIOF                1 << 4
-#define SPI_OPM_RX_EXTN                SPI_OPM_RX_AS | SPI_OPM_RX_DOUT | \
+#define SPI_OPM_RX_AS          (1 << 0)
+#define SPI_OPM_RX_DOUT        (1 << 1)
+#define SPI_OPM_RX_DIO         (1 << 2)
+#define SPI_OPM_RX_QOF         (1 << 3)
+#define SPI_OPM_RX_QIOF        (1 << 4)
+#define SPI_OPM_RX_EXTN        (SPI_OPM_RX_AS | SPI_OPM_RX_DOUT | \
                                SPI_OPM_RX_DIO | SPI_OPM_RX_QOF | \
-                               SPI_OPM_RX_QIOF
+                               SPI_OPM_RX_QIOF)
 
-/* SPI bus connection options */
-#define SPI_CONN_DUAL_SHARED   1 << 0
-#define SPI_CONN_DUAL_SEPARATED        1 << 1
+/* SPI bus connection options - see enum spi_dual_flash */
+#define SPI_CONN_DUAL_SHARED           (1 << 0)
+#define SPI_CONN_DUAL_SEPARATED        (1 << 1)
 
 /* Header byte that marks the start of the message */
 #define SPI_PREAMBLE_END_BYTE  0xec
--

2.0.0.526.g5318336

Add a uclass which provides access to SPI buses and includes operations required by SPI.

For a time driver model will need to co-exist with the legacy SPI interface so some parts of the header file are changed depending on which is in use. The exports are adjusted also since some functions are not available with driver model.

Boards must define CONFIG_DM_SPI to use driver model for SPI.

Signed-off-by Simon Glass <[email protected]>

---

...

Hi

Quoted 13 lines · On Monday 14 of July 2014 18:56:13 Simon Glass wrote:

Add a uclass which provides access to SPI buses and includes operations required by SPI.

For a time driver model will need to co-exist with the legacy SPI interface so some parts of the header file are changed depending on which is in use. The exports are adjusted also since some functions are not available with driver model.

Boards must define CONFIG_DM_SPI to use driver model for SPI.

Signed-off-by Simon Glass <[email protected]>

---

drivers/spi/spi-uclass.c line 86

Quoted 6 lines

...

drivers/spi/spi-uclass.c new file +5 -0

+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
+          const void *dout, void *din, unsigned long flags)
+{
+     struct udevice *dev = slave->dev;
+     struct udevice *bus = dev->parent;

is this the best interface here?
I think it would be cleaner if bus drivers had interfaces which follow a certain template, such as
bus_ops(struct udevice *bus, struct udevice *child, ...)

struct spi_slave would be a prime candidate to have in child->parentdata (which should only be accessed by the parent IIUC)

Hi Pavel,

Quoted 19 lines · Pavel Herrmann

Hi

Quoted 16 lines · On Monday 14 of July 2014 18:56:13 Simon Glass wrote:

Add a uclass which provides access to SPI buses and includes operations required by SPI.

For a time driver model will need to co-exist with the legacy SPI interface
so some parts of the header file are changed depending on which is in use.
The exports are adjusted also since some functions are not available with
driver model.

Boards must define CONFIG_DM_SPI to use driver model for SPI.

Signed-off-by Simon Glass <[email protected]>

---

Quoted 11 lines

drivers/spi/spi-uclass.c line 86

Quoted 6 lines

...

drivers/spi/spi-uclass.c new file +5 -0

+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
+          const void *dout, void *din, unsigned long flags)
+{
+     struct udevice *dev = slave->dev;
+     struct udevice *bus = dev->parent;

is this the best interface here?
I think it would be cleaner if bus drivers had interfaces which follow a certain template, such as
bus_ops(struct udevice *bus, struct udevice *child, ...)

Thanks for your comments.

Well I thought about that long and hard. Clearly in a pure driver-model work we would pass a udevice, not a spi_slave.

Quoted 2 lines

struct spi_slave would be a prime candidate to have in child->parentdata (which should only be accessed by the parent IIUC)

Yes, it is. In other words, 'struct spi_slave' is the child's parent data. The only reason that spi_xfer() is passed a spi_slave rather than a udevice is to maintain compatibility with the existing SPI system. I tried various other approachs, such as '#define spi_slave udevice' and the like. At some point we can change it, but it is really painful to have two completely different APIs co-existing in U-Boot.

This way, driver model becomes a fairly soft transition, the amount of rewriting needed is reduced, and I think it is much more likely that people will use it. As an example, one of the reasons that the generic board change has been relatively painless is that people can just define CONFIG_SYS_GENERIC_BOARD in their board header file, and in most cases it just works. If we require people to rewrite things it will take longer, etc. There is already enough rewriting required in individual drivers to keep people busy. It will be a relatively easy change to do in the future if we want to.

Re passing both the bus and the device, really, what's the point? The only valid bus to pass to the function is dev->parent. If you pass anything else it is an error. It is redundant and therefore just introduces the possibility of error.

Hi

Quoted 50 lines · On Wednesday 16 of July 2014 23:39:44 Simon Glass wrote:

Hi Pavel,

Quoted 19 lines · Pavel Herrmann

Hi

Quoted 16 lines · On Monday 14 of July 2014 18:56:13 Simon Glass wrote:

Add a uclass which provides access to SPI buses and includes operations required by SPI.

For a time driver model will need to co-exist with the legacy SPI interface
so some parts of the header file are changed depending on which is in use.
The exports are adjusted also since some functions are not available with
driver model.

Boards must define CONFIG_DM_SPI to use driver model for SPI.

Signed-off-by Simon Glass <[email protected]>

---

Quoted 11 lines

drivers/spi/spi-uclass.c line 86

Quoted 6 lines

...

drivers/spi/spi-uclass.c new file +5 -0

+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
+          const void *dout, void *din, unsigned long flags)
+{
+     struct udevice *dev = slave->dev;
+     struct udevice *bus = dev->parent;

is this the best interface here?
I think it would be cleaner if bus drivers had interfaces which follow a certain template, such as
bus_ops(struct udevice *bus, struct udevice *child, ...)

Thanks for your comments.

Well I thought about that long and hard. Clearly in a pure driver-model work we would pass a udevice, not a spi_slave.

Quoted 2 lines

struct spi_slave would be a prime candidate to have in child->parentdata (which should only be accessed by the parent IIUC)

Yes, it is. In other words, 'struct spi_slave' is the child's parent data. The only reason that spi_xfer() is passed a spi_slave rather than a udevice is to maintain compatibility with the existing SPI system. I tried various other approachs, such as '#define spi_slave udevice' and the like. At some point we can change it, but it is really painful to have two completely different APIs co-existing in U-Boot.

Quoted 9 lines

This way, driver model becomes a fairly soft transition, the amount of rewriting needed is reduced, and I think it is much more likely that people will use it. As an example, one of the reasons that the generic board change has been relatively painless is that people can just define CONFIG_SYS_GENERIC_BOARD in their board header file, and in most cases it just works. If we require people to rewrite things it will take longer, etc. There is already enough rewriting required in individual drivers to keep people busy. It will be a relatively easy change to do in the future if we want to.

OK, that reason I understand.

However, what you are doing now is limiting what parentdata a SPI bus controller can use.
This means that each bus driver has its parentdata defined by what uclass it belongs to. Are you sure this won't be a limitation? I mean that you could end up with uclasses that have the same calls but a slightly different parentdata.

Yes, you could have a shared parentdata from the uclass (that makes sense for all controllers, because of the way the bus works), and a controller-specific parentdata as an extension (read "void *private" at the end of the parentdata structure)

Quoted 4 lines

Re passing both the bus and the device, really, what's the point? The only valid bus to pass to the function is dev->parent. If you pass anything else it is an error. It is redundant and therefore just introduces the possibility of error.

Well, yes, sort of.

Consider a bus with transparent bridges, like USB or PCI.

If you were to represent these bridges as udevices, you would end up with something in between the actual devices and the bus controller, that forwards requests with no or minimal change.
in case of USB specifically (IIRC), hubs are visible during initialization, but when the device is up it has its own descriptor that is used to
in case of PCI, the device address actually contains the bus number, but the device itself doesn't really care about it, and is only used to select which bus the command goes to.

consider the following scenario:

------     ----------     ---------     ---------     ------------
|root| --- |ehci_hcd| --- |usb_hub| --- |usb_hub| --- |usb_device|
------     ----------     ---------     ---------     ------------

If you were to flatten the bus, the udevice tree would not really correspond to how the bus looks like in reality, and it might give you some hurdles with initialization.

note that in these cases you cannot pass a child udevice to the bus controller as a part of the upcall, since it is not necessarily its child. this is in contradiction to what I wrote previously, simply because I didn't think hard enough to find these counter examples.

Hi Pavel,

Quoted 66 lines · Pavel Herrmann

Hi

Quoted 53 lines · On Wednesday 16 of July 2014 23:39:44 Simon Glass wrote:

Hi Pavel,

Quoted 20 lines · Pavel Herrmann

Hi

Quoted 17 lines · On Monday 14 of July 2014 18:56:13 Simon Glass wrote:

Add a uclass which provides access to SPI buses and includes operations
required by SPI.

For a time driver model will need to co-exist with the legacy SPI interface
so some parts of the header file are changed depending on which is in use.
The exports are adjusted also since some functions are not available with
driver model.

Boards must define CONFIG_DM_SPI to use driver model for SPI.

Signed-off-by Simon Glass <[email protected]>

---

Quoted 12 lines

drivers/spi/spi-uclass.c line 86

Quoted 6 lines

...

drivers/spi/spi-uclass.c new file +5 -0

+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
+          const void *dout, void *din, unsigned long flags)
+{
+     struct udevice *dev = slave->dev;
+     struct udevice *bus = dev->parent;

is this the best interface here?
I think it would be cleaner if bus drivers had interfaces which follow a
certain template, such as
bus_ops(struct udevice *bus, struct udevice *child, ...)

Thanks for your comments.

Well I thought about that long and hard. Clearly in a pure driver-model work we would pass a udevice, not a spi_slave.

Quoted 3 lines

struct spi_slave would be a prime candidate to have in child->parentdata
(which should only be accessed by the parent IIUC)

Yes, it is. In other words, 'struct spi_slave' is the child's parent data. The only reason that spi_xfer() is passed a spi_slave rather than a udevice is to maintain compatibility with the existing SPI system. I tried various other approachs, such as '#define spi_slave udevice' and the like. At some point we can change it, but it is really painful to have two completely different APIs co-existing in U-Boot.

Quoted 9 lines

This way, driver model becomes a fairly soft transition, the amount of rewriting needed is reduced, and I think it is much more likely that people will use it. As an example, one of the reasons that the generic board change has been relatively painless is that people can just define CONFIG_SYS_GENERIC_BOARD in their board header file, and in most cases it just works. If we require people to rewrite things it will take longer, etc. There is already enough rewriting required in individual drivers to keep people busy. It will be a relatively easy change to do in the future if we want to.

Quoted 8 lines

OK, that reason I understand.

However, what you are doing now is limiting what parentdata a SPI bus controller can use.
This means that each bus driver has its parentdata defined by what uclass it belongs to. Are you sure this won't be a limitation? I mean that you could end up with uclasses that have the same calls but a slightly different parentdata.

No it is defined by the driver of the bus. Since it is possible to have (for example) 3 different USB drivers in a system, each can have its own idea of what child data it wants. I definitely agree that setting the parentdata by the child's uclass, or even the bus's uclass would be limiting. In the case of SPI I have elected to use struct spi_slave for reasons I explained earlier.

Quoted 31 lines

Yes, you could have a shared parentdata from the uclass (that makes sense for all controllers, because of the way the bus works), and a controller-specific parentdata as an extension (read "void *private" at the end of the parentdata structure)

Quoted 4 lines

Re passing both the bus and the device, really, what's the point? The only valid bus to pass to the function is dev->parent. If you pass anything else it is an error. It is redundant and therefore just introduces the possibility of error.

Well, yes, sort of.

Consider a bus with transparent bridges, like USB or PCI.

If you were to represent these bridges as udevices, you would end up with something in between the actual devices and the bus controller, that forwards requests with no or minimal change.
in case of USB specifically (IIRC), hubs are visible during initialization, but when the device is up it has its own descriptor that is used to
in case of PCI, the device address actually contains the bus number, but the device itself doesn't really care about it, and is only used to select which bus the command goes to.

consider the following scenario:
------ ---------- --------- --------- ------------

|root| --- |ehci_hcd| --- |usb_hub| --- |usb_hub| --- |usb_device|

------ ---------- --------- --------- ------------

Quoted 8 lines

If you were to flatten the bus, the udevice tree would not really correspond to how the bus looks like in reality, and it might give you some hurdles with initialization.

note that in these cases you cannot pass a child udevice to the bus controller as a part of the upcall, since it is not necessarily its child. this is in contradiction to what I wrote previously, simply because I didn't think hard enough to find these counter examples.

I think you are referring to setting up a bus such that it becomes transparent. But even when it is, I'm not sure that driver model needs to rearrange its data structures to suit. Why would you flatten the bus? If we keep the data structures the same (strict parent-child relationships) then we always have a parent for the child, rather than trying to be too clever. Still, the child can obtain direct bus access through some mechanism delegated by the parent if we like. In that case there is even less reason to access the parent udevice IMO.

I think I'm missing something from your example, so please let me know.

Hi

Quoted 82 lines · On Thursday 17 of July 2014 09:26:47 Simon Glass wrote:

Hi Pavel,

Quoted 71 lines · Pavel Herrmann

Hi

Quoted 56 lines · On Wednesday 16 of July 2014 23:39:44 Simon Glass wrote:

Hi Pavel,

Quoted 22 lines · Pavel Herrmann

Hi

Quoted 19 lines · On Monday 14 of July 2014 18:56:13 Simon Glass wrote:

Add a uclass which provides access to SPI buses and includes operations
required by SPI.

For a time driver model will need to co-exist with the legacy SPI interface
so some parts of the header file are changed depending on which is in
use.
The exports are adjusted also since some functions are not available
with
driver model.

Boards must define CONFIG_DM_SPI to use driver model for SPI.

Signed-off-by Simon Glass <[email protected]>

---

Quoted 13 lines

drivers/spi/spi-uclass.c line 86

Quoted 6 lines

...

drivers/spi/spi-uclass.c new file +5 -0

+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
+          const void *dout, void *din, unsigned long flags)
+{
+     struct udevice *dev = slave->dev;
+     struct udevice *bus = dev->parent;

is this the best interface here?
I think it would be cleaner if bus drivers had interfaces which follow
a
certain template, such as
bus_ops(struct udevice *bus, struct udevice *child, ...)

Thanks for your comments.

Well I thought about that long and hard. Clearly in a pure driver-model work we would pass a udevice, not a spi_slave.

Quoted 3 lines

struct spi_slave would be a prime candidate to have in child->parentdata
(which should only be accessed by the parent IIUC)

Yes, it is. In other words, 'struct spi_slave' is the child's parent data. The only reason that spi_xfer() is passed a spi_slave rather than a udevice is to maintain compatibility with the existing SPI system. I tried various other approachs, such as '#define spi_slave udevice' and the like. At some point we can change it, but it is really painful to have two completely different APIs co-existing in U-Boot.

Quoted 9 lines

This way, driver model becomes a fairly soft transition, the amount of rewriting needed is reduced, and I think it is much more likely that people will use it. As an example, one of the reasons that the generic board change has been relatively painless is that people can just define CONFIG_SYS_GENERIC_BOARD in their board header file, and in most cases it just works. If we require people to rewrite things it will take longer, etc. There is already enough rewriting required in individual drivers to keep people busy. It will be a relatively easy change to do in the future if we want to.

OK, that reason I understand.

Quoted 7 lines

However, what you are doing now is limiting what parentdata a SPI bus controller can use.
This means that each bus driver has its parentdata defined by what uclass
it belongs to. Are you sure this won't be a limitation? I mean that you could end up with uclasses that have the same calls but a slightly different parentdata.

Quoted 6 lines

No it is defined by the driver of the bus. Since it is possible to have (for example) 3 different USB drivers in a system, each can have its own idea of what child data it wants. I definitely agree that setting the parentdata by the child's uclass, or even the bus's uclass would be limiting. In the case of SPI I have elected to use struct spi_slave for reasons I explained earlier.

OK, so you would have some bus uclasses that pass typecasted parentdata, because it is the same for all bus drivers, and some that pass the childs udevice*, because the parentdata type is not known beforehands? what happens if suddenly you need to add a bus controller that needs a little more per- child data than the existing ones?

wouldn't it be better to unify this somehow?

Quoted 51 lines

Quoted 33 lines

Yes, you could have a shared parentdata from the uclass (that makes sense
for all controllers, because of the way the bus works), and a controller-specific parentdata as an extension (read "void *private" at the end of the parentdata structure)

Quoted 4 lines

Re passing both the bus and the device, really, what's the point? The only valid bus to pass to the function is dev->parent. If you pass anything else it is an error. It is redundant and therefore just introduces the possibility of error.

Well, yes, sort of.

Consider a bus with transparent bridges, like USB or PCI.

If you were to represent these bridges as udevices, you would end up with
something in between the actual devices and the bus controller, that forwards requests with no or minimal change.
in case of USB specifically (IIRC), hubs are visible during initialization, but when the device is up it has its own descriptor that is used to
in case of PCI, the device address actually contains the bus number, but the device itself doesn't really care about it, and is only used to select which bus the command goes to.

consider the following scenario:
------ ---------- --------- --------- ------------

|root| --- |ehci_hcd| --- |usb_hub| --- |usb_hub| --- |usb_device|

------ ---------- --------- --------- ------------

Quoted 8 lines

If you were to flatten the bus, the udevice tree would not really correspond to how the bus looks like in reality, and it might give you some hurdles with initialization.

note that in these cases you cannot pass a child udevice to the bus controller as a part of the upcall, since it is not necessarily its child. this is in contradiction to what I wrote previously, simply because I didn't think hard enough to find these counter examples.

I think you are referring to setting up a bus such that it becomes transparent. But even when it is, I'm not sure that driver model needs to rearrange its data structures to suit. Why would you flatten the bus? If we keep the data structures the same (strict parent-child relationships) then we always have a parent for the child, rather than trying to be too clever. Still, the child can obtain direct bus access through some mechanism delegated by the parent if we like. In that case there is even less reason to access the parent udevice IMO.

Quoted 1 line

I think I'm missing something from your example, so please let me know.

ok, lets try some code examples

lets say a usb_hub is implemented as a USB bus that itself sits in a USB bus. lets say USB bus uclass has a function usb_bulk_msg() usb_hub's implementation of such function would look like this:

usb_bulk_msg(udevice *hub, usb_descriptor *child, ...)

{
	return usb_bulk_msg(hub->parent, child, ...);
}

your USB device would then call
usb_bulk_msg(my_dev->parent, my_desc,...)

this would work recursively through all hubs, and the end result you would like is to call usb_bulk_msg() of the host bus controller, without changing any of the parameters. (except the first, which is used for recursion)

However, if you got rid of the first parameter, you would need a different variable to control your recursion, or you would need the device descriptor to hold pointer to the udevice of the host controller, which would effectively flatten the bus.

this is the case where I believe the best way to implement is to have a shared parentdata based on parent uclass (the usb_descriptor in this case), with a driver-specific extension, in case the bus driver need something extra over what the "standard" is. the shared parentdata would then be used as a device descriptor/address/whatever-you-call-that in the bus calls.

One would also need to relax the semantics a bit, so that host bus controller can fill the parentdata for a udevice that is not its direct child.

I'm not saying your code is wrong or anything, I just think you should have a look at a more complex bus like USB or PCI, and design the "bus uclass interface template" in a way that would support such a bus. Otherwise you might end up either reworking your simpler busses later, or having inconsistent bus uclass interfaces (which we did when we tried this the first time).

I understand that API compatibility is an issue, and I agree that it is fine to cut some corners at this point, but we need to keep in mind to fix them once all the drivers have been converted.

regards
Pavel

Quoted 176 lines · On Thursday 17 of July 2014 20:01:29 Pavel Herrmann wrote:

Hi

Quoted 82 lines · On Thursday 17 of July 2014 09:26:47 Simon Glass wrote:

Hi Pavel,

Quoted 71 lines · Pavel Herrmann

Hi

Quoted 56 lines · On Wednesday 16 of July 2014 23:39:44 Simon Glass wrote:

Hi Pavel,

Quoted 22 lines · Pavel Herrmann

Hi

Quoted 19 lines · On Monday 14 of July 2014 18:56:13 Simon Glass wrote:

Add a uclass which provides access to SPI buses and includes operations
required by SPI.

For a time driver model will need to co-exist with the legacy SPI interface
so some parts of the header file are changed depending on which is in
use.
The exports are adjusted also since some functions are not available
with
driver model.

Boards must define CONFIG_DM_SPI to use driver model for SPI.

Signed-off-by Simon Glass <[email protected]>

---

Quoted 13 lines

Review

Quoted 6 lines

...

(quoted diff) +5 -0

+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
+          const void *dout, void *din, unsigned long flags)
+{
+     struct udevice *dev = slave->dev;
+     struct udevice *bus = dev->parent;

is this the best interface here?
I think it would be cleaner if bus drivers had interfaces which follow
a
certain template, such as
bus_ops(struct udevice *bus, struct udevice *child, ...)

Thanks for your comments.

Well I thought about that long and hard. Clearly in a pure driver-model work we would pass a udevice, not a spi_slave.

Quoted 3 lines

struct spi_slave would be a prime candidate to have in child->parentdata
(which should only be accessed by the parent IIUC)

Yes, it is. In other words, 'struct spi_slave' is the child's parent data. The only reason that spi_xfer() is passed a spi_slave rather than a udevice is to maintain compatibility with the existing SPI system. I tried various other approachs, such as '#define spi_slave udevice' and the like. At some point we can change it, but it is really painful to have two completely different APIs co-existing in U-Boot.

Quoted 9 lines

This way, driver model becomes a fairly soft transition, the amount of rewriting needed is reduced, and I think it is much more likely that people will use it. As an example, one of the reasons that the generic board change has been relatively painless is that people can just define CONFIG_SYS_GENERIC_BOARD in their board header file, and in most cases it just works. If we require people to rewrite things it will take longer, etc. There is already enough rewriting required in individual drivers to keep people busy. It will be a relatively easy change to do in the future if we want to.

OK, that reason I understand.

Quoted 7 lines

However, what you are doing now is limiting what parentdata a SPI bus controller can use.
This means that each bus driver has its parentdata defined by what uclass
it belongs to. Are you sure this won't be a limitation? I mean that you could end up with uclasses that have the same calls but a slightly different parentdata.

Quoted 6 lines

No it is defined by the driver of the bus. Since it is possible to have (for example) 3 different USB drivers in a system, each can have its own idea of what child data it wants. I definitely agree that setting the parentdata by the child's uclass, or even the bus's uclass would be limiting. In the case of SPI I have elected to use struct spi_slave for reasons I explained earlier.

OK, so you would have some bus uclasses that pass typecasted parentdata, because it is the same for all bus drivers, and some that pass the childs udevice*, because the parentdata type is not known beforehands? what happens if suddenly you need to add a bus controller that needs a little more per- child data than the existing ones?

wouldn't it be better to unify this somehow?

Quoted 51 lines

Quoted 33 lines

Yes, you could have a shared parentdata from the uclass (that makes sense
for all controllers, because of the way the bus works), and a controller-specific parentdata as an extension (read "void *private" at the end of the parentdata structure)

Quoted 4 lines

Re passing both the bus and the device, really, what's the point? The only valid bus to pass to the function is dev->parent. If you pass anything else it is an error. It is redundant and therefore just introduces the possibility of error.

Well, yes, sort of.

Consider a bus with transparent bridges, like USB or PCI.

If you were to represent these bridges as udevices, you would end up with
something in between the actual devices and the bus controller, that forwards requests with no or minimal change.
in case of USB specifically (IIRC), hubs are visible during initialization, but when the device is up it has its own descriptor that is used to
in case of PCI, the device address actually contains the bus number, but the device itself doesn't really care about it, and is only used to select which bus the command goes to.

consider the following scenario:
------ ---------- --------- --------- ------------

|root| --- |ehci_hcd| --- |usb_hub| --- |usb_hub| --- |usb_device|

------ ---------- --------- --------- ------------

Quoted 8 lines

If you were to flatten the bus, the udevice tree would not really correspond to how the bus looks like in reality, and it might give you some hurdles with initialization.

note that in these cases you cannot pass a child udevice to the bus controller as a part of the upcall, since it is not necessarily its child. this is in contradiction to what I wrote previously, simply because I didn't think hard enough to find these counter examples.

I think you are referring to setting up a bus such that it becomes transparent. But even when it is, I'm not sure that driver model needs to rearrange its data structures to suit. Why would you flatten the bus? If we keep the data structures the same (strict parent-child relationships) then we always have a parent for the child, rather than trying to be too clever. Still, the child can obtain direct bus access through some mechanism delegated by the parent if we like. In that case there is even less reason to access the parent udevice IMO.

Quoted 1 line

I think I'm missing something from your example, so please let me know.

ok, lets try some code examples

lets say a usb_hub is implemented as a USB bus that itself sits in a USB bus. lets say USB bus uclass has a function usb_bulk_msg() usb_hub's implementation of such function would look like this:

usb_bulk_msg(udevice *hub, usb_descriptor *child, ...)

{
      return usb_bulk_msg(hub->parent, child, ...);
}

your USB device would then call
usb_bulk_msg(my_dev->parent, my_desc,...)

this would work recursively through all hubs, and the end result you would like is to call usb_bulk_msg() of the host bus controller, without changing any of the parameters. (except the first, which is used for recursion)

However, if you got rid of the first parameter, you would need a different variable to control your recursion, or you would need the device descriptor to hold pointer to the udevice of the host controller, which would effectively flatten the bus.

Quoted 5 lines

this is the case where I believe the best way to implement is to have a shared parentdata based on parent uclass (the usb_descriptor in this case), with a driver-specific extension, in case the bus driver need something extra over what the "standard" is. the shared parentdata would then be used as a device descriptor/address/whatever-you-call-that in the bus calls.

I would like to add that passing the childs udevice* should work equally as well, if you relax the bus == child->parent assumption.

Hi

Quoted 77 lines · On Thursday 17 of July 2014 09:26:47 Simon Glass wrote:

Hi Pavel,

Quoted 66 lines · Pavel Herrmann

Hi

Quoted 53 lines · On Wednesday 16 of July 2014 23:39:44 Simon Glass wrote:

Hi Pavel,

Quoted 20 lines · Pavel Herrmann

Hi

Quoted 17 lines · On Monday 14 of July 2014 18:56:13 Simon Glass wrote:

Add a uclass which provides access to SPI buses and includes operations
required by SPI.

For a time driver model will need to co-exist with the legacy SPI interface
so some parts of the header file are changed depending on which is in use.
The exports are adjusted also since some functions are not available with
driver model.

Boards must define CONFIG_DM_SPI to use driver model for SPI.

Signed-off-by Simon Glass <[email protected]>

---

Quoted 12 lines

drivers/spi/spi-uclass.c line 86

Quoted 6 lines

...

drivers/spi/spi-uclass.c new file +5 -0

+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
+          const void *dout, void *din, unsigned long flags)
+{
+     struct udevice *dev = slave->dev;
+     struct udevice *bus = dev->parent;

is this the best interface here?
I think it would be cleaner if bus drivers had interfaces which follow a
certain template, such as
bus_ops(struct udevice *bus, struct udevice *child, ...)

Thanks for your comments.

Well I thought about that long and hard. Clearly in a pure driver-model work we would pass a udevice, not a spi_slave.

Quoted 3 lines

struct spi_slave would be a prime candidate to have in child->parentdata
(which should only be accessed by the parent IIUC)

Yes, it is. In other words, 'struct spi_slave' is the child's parent data. The only reason that spi_xfer() is passed a spi_slave rather than a udevice is to maintain compatibility with the existing SPI system. I tried various other approachs, such as '#define spi_slave udevice' and the like. At some point we can change it, but it is really painful to have two completely different APIs co-existing in U-Boot.

Quoted 9 lines

This way, driver model becomes a fairly soft transition, the amount of rewriting needed is reduced, and I think it is much more likely that people will use it. As an example, one of the reasons that the generic board change has been relatively painless is that people can just define CONFIG_SYS_GENERIC_BOARD in their board header file, and in most cases it just works. If we require people to rewrite things it will take longer, etc. There is already enough rewriting required in individual drivers to keep people busy. It will be a relatively easy change to do in the future if we want to.

Quoted 8 lines

OK, that reason I understand.

However, what you are doing now is limiting what parentdata a SPI bus controller can use.
This means that each bus driver has its parentdata defined by what uclass it belongs to. Are you sure this won't be a limitation? I mean that you could end up with uclasses that have the same calls but a slightly different parentdata.

Quoted 6 lines

No it is defined by the driver of the bus. Since it is possible to have (for example) 3 different USB drivers in a system, each can have its own idea of what child data it wants. I definitely agree that setting the parentdata by the child's uclass, or even the bus's uclass would be limiting. In the case of SPI I have elected to use struct spi_slave for reasons I explained earlier.

OK, so you would have some bus uclasses that pass typecasted parentdata, because it is the same for all bus drivers, and some that pass the childs udevice*, because the parentdata type is not known beforehands? what happens if suddenly you need to add a bus controller that needs a little more per- child data than the existing ones?

wouldn't it be better to unify this somehow?

Quoted 50 lines

Quoted 32 lines

Yes, you could have a shared parentdata from the uclass (that makes sense for all controllers, because of the way the bus works), and a controller-specific parentdata as an extension (read "void *private" at the end of the parentdata structure)

Quoted 4 lines

Re passing both the bus and the device, really, what's the point? The only valid bus to pass to the function is dev->parent. If you pass anything else it is an error. It is redundant and therefore just introduces the possibility of error.

Well, yes, sort of.

Consider a bus with transparent bridges, like USB or PCI.

If you were to represent these bridges as udevices, you would end up with something in between the actual devices and the bus controller, that forwards requests with no or minimal change.
in case of USB specifically (IIRC), hubs are visible during initialization, but when the device is up it has its own descriptor that is used to
in case of PCI, the device address actually contains the bus number, but the device itself doesn't really care about it, and is only used to select which bus the command goes to.

consider the following scenario:
------ ---------- --------- --------- ------------

|root| --- |ehci_hcd| --- |usb_hub| --- |usb_hub| --- |usb_device|

------ ---------- --------- --------- ------------

Quoted 8 lines

If you were to flatten the bus, the udevice tree would not really correspond to how the bus looks like in reality, and it might give you some hurdles with initialization.

note that in these cases you cannot pass a child udevice to the bus controller as a part of the upcall, since it is not necessarily its child. this is in contradiction to what I wrote previously, simply because I didn't think hard enough to find these counter examples.

I think you are referring to setting up a bus such that it becomes transparent. But even when it is, I'm not sure that driver model needs to rearrange its data structures to suit. Why would you flatten the bus? If we keep the data structures the same (strict parent-child relationships) then we always have a parent for the child, rather than trying to be too clever. Still, the child can obtain direct bus access through some mechanism delegated by the parent if we like. In that case there is even less reason to access the parent udevice IMO.

Quoted 1 line

I think I'm missing something from your example, so please let me know.

ok, lets try some code examples

lets say a usb_hub is implemented as a USB bus that itself sits in a USB bus. lets say USB bus uclass has a function usb_bulk_msg()
usb_hub's implementation of such function would look like this:

usb_bulk_msg(udevice *hub, usb_descriptor *child, ...)

{
        return usb_bulk_msg(hub->parent, child, ...);
}

your USB device would then call
usb_bulk_msg(my_dev->parent, my_desc,...)

this would work recursively through all hubs, and the end result you would like is to call usb_bulk_msg() of the host bus controller, without changing any of the parameters. (except the first, which is used for recursion)

However, if you got rid of the first parameter, you would need a different variable to control your recursion, or you would need the device descriptor to hold pointer to the udevice of the host controller, which would effectively flatten the bus.

this is the case where I believe the best way to implement is to have a shared parentdata based on parent uclass (the usb_descriptor in this case), with a driver-specific extension, in case the bus driver need something extra over what the "standard" is. the shared parentdata would then be used as a device descriptor/address/whatever-you-call-that in the bus calls.

One would also need to relax the semantics a bit, so that host bus controller can fill the parentdata for a udevice that is not its direct child.

I'm not saying your code is wrong or anything, I just think you should have a look at a more complex bus like USB or PCI, and design the "bus uclass interface template" in a way that would support such a bus. Otherwise you might end up either reworking your simpler busses later, or having inconsistent bus uclass interfaces (which we did when we tried this the first time).

I understand that API compatibility is an issue, and I agree that it is fine to cut some corners at this point, but we need to keep in mind to fix them once all the drivers have been converted.

Add a uclass which provides access to SPI buses and includes operations required by SPI.

For a time driver model will need to co-exist with the legacy SPI interface so some parts of the header file are changed depending on which is in use. The exports are adjusted also since some functions are not available with driver model.

Boards must define CONFIG_DM_SPI to use driver model for SPI.

Signed-off-by Simon Glass <[email protected]>

---

 common/exports.c         |   4 +-
 drivers/spi/Makefile     |   4 +
 drivers/spi/spi-uclass.c | 253 +++++++++++++++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h   |   1 +
 include/spi.h            | 140 ++++++++++++++++++++++++++
 5 files changed, 401 insertions(+), 1 deletion(-)
 create mode 100644 drivers/spi/spi-uclass.c

drivers/spi/spi-uclass.c new file +16 -0

@@ -0,0 +1,253 @@
+/*
+ * Copyright (c) 2014 Google, Inc
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <fdtdec.h>
+#include <spi.h>
+#include <dm/device-internal.h>
+#include <dm/uclass-internal.h>
+#include <dm/root.h>
+#include <dm/lists.h>
+

drivers/spi/spi-uclass.c new file +33 -0

+	else
+		ret = -EINVAL;
+	if (ret) {
+		printf("Cannot set mode (err=%d)\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+int spi_claim_bus(struct spi_slave *slave)
+{
+	struct udevice *dev = slave->dev;
+	struct udevice *bus = dev->parent;
+	struct dm_spi_ops *ops = spi_get_ops(bus);
+	struct dm_spi_bus *spi = bus->uclass_priv;
+	int speed;
+	int ret;
+
+	speed = slave->max_hz;
+	if (spi->max_hz) {
+		if (speed)
+			speed = min(speed, spi->max_hz);
+		else
+			speed = spi->max_hz;
+	}
+	if (!speed)
+		speed = 100000;
+	ret = spi_set_speed_mode(bus, speed, slave->mode);
+	if (ret)
+		return ret;
+
+	return ops->claim_bus ? ops->claim_bus(bus) : 0;

drivers/spi/spi-uclass.c new file +174 -0

+}
+
+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
+	     const void *dout, void *din, unsigned long flags)
+{
+	struct udevice *dev = slave->dev;
+	struct udevice *bus = dev->parent;
+
+	if (bus->uclass->uc_drv->id != UCLASS_SPI)
+		return -EOPNOTSUPP;
+
+	return spi_get_ops(bus)->xfer(bus, dev, bitlen, dout, din, flags);
+}
+
+int spi_post_bind(struct udevice *dev)
+{
+	/* Scan the bus for devices */
+	return dm_scan_fdt_node(dev, gd->fdt_blob, dev->of_offset, false);
+}
+
+int spi_post_probe(struct udevice *dev)
+{
+	struct dm_spi_bus *spi = dev->uclass_priv;
+
+	spi->max_hz = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
+				     "spi-max-frequency", 0);
+
+	return 0;
+}
+
+int spi_bind_device(struct udevice *bus, int cs, const char *drv_name,
+		    const char *dev_name, struct udevice **slavep)
+{
+	struct driver *drv;
+	int ret;
+
+	drv = lists_driver_lookup_name(drv_name);
+	if (!drv) {
+		puts("Cannot find spi_flash_std driver\n");
+		return -ENOENT;
+	}
+	ret = device_bind(bus, drv, dev_name, NULL, -1, slavep);
+	if (ret) {
+		printf("Cannot create device named '%s' (err=%d)\n",
+		       dev_name, ret);
+		return ret;
+	}
+	(*slavep)->req_seq = cs;
+
+	return 0;
+}
+
+int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp,
+			struct udevice **devp)
+{
+	struct udevice *bus, *dev;
+	int ret;
+
+	ret = uclass_find_device_by_seq(UCLASS_SPI, busnum, false, &bus);
+	if (ret)
+		return ret;
+	ret = device_find_child_by_seq(bus, cs, false, &dev);
+	if (ret)
+		return ret;
+	*busp = bus;
+	*devp = dev;
+
+	return ret;
+}
+
+int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
+		       const char *drv_name, const char *dev_name,
+		       struct udevice **devp, struct spi_slave **slavep)
+{
+	struct udevice *bus, *dev;
+	int ret;
+
+	ret = uclass_get_device_by_seq(UCLASS_SPI, busnum, &bus);
+	if (ret) {
+		printf("Invalid bus %d (err=%d)\n", busnum, ret);
+		return ret;
+	}
+	ret = device_get_child_by_seq(bus, cs, &dev);
+
+	/**
+	 * If there is no such device, create one automatically. This means
+	 * that we don't need a device tree node or platform data for the
+	 * SPI flash chip - we will bind to the correct driver.
+	 */
+	if (ret == -ENODEV && drv_name) {
+		ret = spi_bind_device(bus, cs, drv_name, dev_name, &dev);
+		if (ret)
+			return ret;
+	}
+	if (ret) {
+		printf("Invalid chip select %d:%d (err=%d)\n", busnum, cs,
+		       ret);
+		return ret;
+	}
+
+	ret = spi_set_speed_mode(bus, speed, mode);
+	if (ret)
+		return ret;
+
+	*devp = bus;
+	*slavep = dev_get_parentdata(dev);
+
+	return 0;
+}
+
+/* Compatibility function - to be removed */
+struct spi_slave *spi_setup_slave_fdt(const void *blob, int node,
+				      int bus_node)
+{
+	struct udevice *bus, *dev;
+	int ret;
+
+	ret = uclass_get_device_by_of_offset(UCLASS_SPI, bus_node, &bus);
+	if (ret)
+		return NULL;
+	ret = device_get_child_by_of_offset(bus, node, &dev);
+	if (ret)
+		return NULL;
+	return dev_get_parentdata(dev);
+}
+
+/* Compatibility function - to be removed */
+struct spi_slave *spi_setup_slave(unsigned int busnum, unsigned int cs,
+				  unsigned int speed, unsigned int mode)
+{
+	struct spi_slave *slave;
+	struct udevice *dev;
+	int ret;
+
+	ret = spi_get_bus_and_cs(busnum, cs, speed, mode, NULL, 0, &dev,
+				  &slave);
+	if (ret)
+		return NULL;
+
+	return slave;
+}
+
+void spi_free_slave(struct spi_slave *slave)
+{
+	device_remove(slave->dev);
+	slave->dev = NULL;
+}
+
+int spi_ofdata_to_platdata(const void *blob, int node,
+			   struct spi_slave *spi)
+{
+	int mode = 0;
+
+	spi->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency", 0);
+	if (fdtdec_get_bool(blob, node, "spi-cpol"))
+		mode |= SPI_CPOL;
+	if (fdtdec_get_bool(blob, node, "spi-cpha"))
+		mode |= SPI_CPHA;
+	if (fdtdec_get_bool(blob, node, "spi-cs-high"))
+		mode |= SPI_CS_HIGH;
+	if (fdtdec_get_bool(blob, node, "spi-half-duplex"))
+		mode |= SPI_PREAMBLE;
+	spi->mode = mode;
+
+	return 0;
+}
+
+UCLASS_DRIVER(spi) = {
+	.id		= UCLASS_SPI,
+	.name		= "spi",
+	.post_bind	= spi_post_bind,
+	.post_probe	= spi_post_probe,
+	.per_device_auto_alloc_size = sizeof(struct dm_spi_bus),
+};

Add a uclass which provides access to SPI buses and includes operations required by SPI.

For a time driver model will need to co-exist with the legacy SPI interface so some parts of the header file are changed depending on which is in use. The exports are adjusted also since some functions are not available with driver model.

Boards must define CONFIG_DM_SPI to use driver model for SPI.

Signed-off-by Simon Glass <[email protected]>

---

 common/exports.c         |   4 +-
 drivers/spi/Makefile     |   4 +
 drivers/spi/spi-uclass.c | 253 +++++++++++++++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h   |   1 +
 include/spi.h            | 140 ++++++++++++++++++++++++++
 5 files changed, 401 insertions(+), 1 deletion(-)
 create mode 100644 drivers/spi/spi-uclass.c

common/exports.c +3 -1

index b97ca48..88fcfc8 100644
@@ -27,10 +27,12 @@ unsigned long get_version(void)
 # define i2c_write         dummy
 # define i2c_read          dummy
 #endif
-#ifndef CONFIG_CMD_SPI
+#if !defined(CONFIG_CMD_SPI) || defined(CONFIG_DM_SPI)
 # define spi_init          dummy
 # define spi_setup_slave   dummy
 # define spi_free_slave    dummy
+#endif
+#ifndef CONFIG_CMD_SPI
 # define spi_claim_bus     dummy
 # define spi_release_bus   dummy
 # define spi_xfer          dummy

drivers/spi/Makefile +4 -0

index f02c35a..d1f1dd0 100644
@@ -6,7 +6,11 @@
 #
 
 # There are many options which enable SPI, so make this library available
+ifdef CONFIG_DM_SPI
+obj-y += spi-uclass.o
+else
 obj-y += spi.o
+endif
 
 obj-$(CONFIG_EP93XX_SPI) += ep93xx_spi.o
 obj-$(CONFIG_ALTERA_SPI) += altera_spi.o

drivers/spi/spi-uclass.c new file +82 -0

new file mode 100644
index 0000000..4057bce
@@ -0,0 +1,253 @@
+/*
+ * Copyright (c) 2014 Google, Inc
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <fdtdec.h>
+#include <spi.h>
+#include <dm/device-internal.h>
+#include <dm/uclass-internal.h>
+#include <dm/root.h>
+#include <dm/lists.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static int spi_set_speed_mode(struct udevice *bus, int speed, int mode)
+{
+       struct dm_spi_ops *ops;
+       int ret;
+
+       ops = spi_get_ops(bus);
+       if (ops->set_speed)
+               ret = (*ops->set_speed)(bus, speed);
+       else
+               ret = -EINVAL;
+       if (ret) {
+               printf("Cannot set speed (err=%d)\n", ret);
+               return ret;
+       }
+
+       ops = spi_get_ops(bus);
+       if (ops->set_mode)
+               ret = (*ops->set_mode)(bus, mode);
+       else
+               ret = -EINVAL;
+       if (ret) {
+               printf("Cannot set mode (err=%d)\n", ret);
+               return ret;
+       }
+
+       return 0;
+}
+
+int spi_claim_bus(struct spi_slave *slave)
+{
+       struct udevice *dev = slave->dev;
+       struct udevice *bus = dev->parent;
+       struct dm_spi_ops *ops = spi_get_ops(bus);
+       struct dm_spi_bus *spi = bus->uclass_priv;
+       int speed;
+       int ret;
+
+       speed = slave->max_hz;
+       if (spi->max_hz) {
+               if (speed)
+                       speed = min(speed, spi->max_hz);
+               else
+                       speed = spi->max_hz;
+       }
+       if (!speed)
+               speed = 100000;
+       ret = spi_set_speed_mode(bus, speed, slave->mode);
+       if (ret)
+               return ret;
+
+       return ops->claim_bus ? ops->claim_bus(bus) : 0;
+}
+
+void spi_release_bus(struct spi_slave *slave)
+{
+       struct udevice *dev = slave->dev;
+       struct udevice *bus = dev->parent;
+       struct dm_spi_ops *ops = spi_get_ops(bus);
+
+       if (ops->release_bus)
+               spi_get_ops(bus)->release_bus(bus);
+}
+
+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,

drivers/spi/spi-uclass.c new file +6 -0

+
+int spi_post_bind(struct udevice *dev)
+{
+       /* Scan the bus for devices */
+       return dm_scan_fdt_node(dev, gd->fdt_blob, dev->of_offset, false);
+}

drivers/spi/spi-uclass.c new file +70 -0

+
+int spi_bind_device(struct udevice *bus, int cs, const char *drv_name,
+                   const char *dev_name, struct udevice **slavep)
+{
+       struct driver *drv;
+       int ret;
+
+       drv = lists_driver_lookup_name(drv_name);
+       if (!drv) {
+               puts("Cannot find spi_flash_std driver\n");
+               return -ENOENT;
+       }
+       ret = device_bind(bus, drv, dev_name, NULL, -1, slavep);
+       if (ret) {
+               printf("Cannot create device named '%s' (err=%d)\n",
+                      dev_name, ret);
+               return ret;
+       }
+       (*slavep)->req_seq = cs;
+
+       return 0;
+}
+
+int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp,
+                       struct udevice **devp)
+{
+       struct udevice *bus, *dev;
+       int ret;
+
+       ret = uclass_find_device_by_seq(UCLASS_SPI, busnum, false, &bus);
+       if (ret)
+               return ret;
+       ret = device_find_child_by_seq(bus, cs, false, &dev);
+       if (ret)
+               return ret;
+       *busp = bus;
+       *devp = dev;
+
+       return ret;
+}
+
+int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
+                      const char *drv_name, const char *dev_name,
+                      struct udevice **devp, struct spi_slave **slavep)
+{
+       struct udevice *bus, *dev;
+       int ret;
+
+       ret = uclass_get_device_by_seq(UCLASS_SPI, busnum, &bus);
+       if (ret) {
+               printf("Invalid bus %d (err=%d)\n", busnum, ret);
+               return ret;
+       }
+       ret = device_get_child_by_seq(bus, cs, &dev);
+
+       /**
+        * If there is no such device, create one automatically. This means
+        * that we don't need a device tree node or platform data for the
+        * SPI flash chip - we will bind to the correct driver.
+        */
+       if (ret == -ENODEV && drv_name) {
+               ret = spi_bind_device(bus, cs, drv_name, dev_name, &dev);
+               if (ret)
+                       return ret;
+       }
+       if (ret) {
+               printf("Invalid chip select %d:%d (err=%d)\n", busnum, cs,
+                      ret);
+               return ret;
+       }

drivers/spi/spi-uclass.c new file +65 -0

+
+/* Compatibility function - to be removed */
+struct spi_slave *spi_setup_slave_fdt(const void *blob, int node,
+                                     int bus_node)
+{
+       struct udevice *bus, *dev;
+       int ret;
+
+       ret = uclass_get_device_by_of_offset(UCLASS_SPI, bus_node, &bus);
+       if (ret)
+               return NULL;
+       ret = device_get_child_by_of_offset(bus, node, &dev);
+       if (ret)
+               return NULL;
+       return dev_get_parentdata(dev);
+}
+
+/* Compatibility function - to be removed */
+struct spi_slave *spi_setup_slave(unsigned int busnum, unsigned int cs,
+                                 unsigned int speed, unsigned int mode)
+{
+       struct spi_slave *slave;
+       struct udevice *dev;
+       int ret;
+
+       ret = spi_get_bus_and_cs(busnum, cs, speed, mode, NULL, 0, &dev,
+                                 &slave);
+       if (ret)
+               return NULL;
+
+       return slave;
+}
+
+void spi_free_slave(struct spi_slave *slave)
+{
+       device_remove(slave->dev);
+       slave->dev = NULL;
+}
+
+int spi_ofdata_to_platdata(const void *blob, int node,
+                          struct spi_slave *spi)
+{
+       int mode = 0;
+
+       spi->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency", 0);
+       if (fdtdec_get_bool(blob, node, "spi-cpol"))
+               mode |= SPI_CPOL;
+       if (fdtdec_get_bool(blob, node, "spi-cpha"))
+               mode |= SPI_CPHA;
+       if (fdtdec_get_bool(blob, node, "spi-cs-high"))
+               mode |= SPI_CS_HIGH;
+       if (fdtdec_get_bool(blob, node, "spi-half-duplex"))
+               mode |= SPI_PREAMBLE;
+       spi->mode = mode;
+
+       return 0;
+}
+
+UCLASS_DRIVER(spi) = {
+       .id             = UCLASS_SPI,
+       .name           = "spi",
+       .post_bind      = spi_post_bind,
+       .post_probe     = spi_post_probe,
+       .per_device_auto_alloc_size = sizeof(struct dm_spi_bus),
+};

include/dm/uclass-id.h +1 -0

index 7f0e37b..8207483 100644
@@ -22,6 +22,7 @@ enum uclass_id {
        /* U-Boot uclasses start here */
        UCLASS_GPIO,            /* Bank of general-purpose I/O pins */
        UCLASS_SERIAL,          /* Serial UART */
+       UCLASS_SPI,             /* SPI bus */
 
        UCLASS_COUNT,
        UCLASS_INVALID = -1,

include/spi.h +0 -0

index b673be2..b5e9347 100644

include/spi.h +132 -1

  * @bus:               ID of the bus that the slave is attached to.
  * @cs:                        ID of the chip select connected to the slave.
  * @op_mode_rx:                SPI RX operation mode.

@@ -71,8 +79,14 @@
  * @flags:             Indication of SPI flags.
  */
 struct spi_slave {
+#ifdef CONFIG_DM_SPI
+       struct udevice *dev;    /* struct spi_slave is dev->parentdata */
+       uint max_hz;
+       uint mode;
+#else
        unsigned int bus;
        unsigned int cs;
+#endif
        u8 op_mode_rx;
        u8 op_mode_tx;
        unsigned int wordlen;

@@ -220,6 +234,8 @@ int spi_set_wordlen(struct spi_slave *slave, unsigned int wordlen);
 int  spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
                void *din, unsigned long flags);
 
+#ifndef CONFIG_DM_SPI
+
 /**
  * Determine if a SPI chipselect is valid.
  * This function is provided by the board if the low-level SPI driver

@@ -255,6 +271,7 @@ void spi_cs_deactivate(struct spi_slave *slave);
  * @hz:                The transfer speed
  */
 void spi_set_speed(struct spi_slave *slave, uint hz);
+#endif
 
 /**
  * Write 8 bits, then read 8 bits.

@@ -305,4 +322,127 @@ struct spi_slave *spi_setup_slave_fdt(const void *blob, int slave_node,
 struct spi_slave *spi_base_setup_slave_fdt(const void *blob, int busnum,
                                           int node);
 
+#ifdef CONFIG_DM_SPI
+
+/**
+ * struct struct dm_spi_ops - Driver model SPI operations
+ *
+ * The uclass interface is implemented by all SPI devices which use
+ * driver model.
+ */
+struct dm_spi_ops {
+       /**
+        * Claim the bus and prepare it for communication.
+        *
+        * The device privided is the slave device. It's parent controller
+        * will be used to provide the communication.
+        *
+        * This must be called before doing any transfers with a SPI slave. It
+        * will enable and initialize any SPI hardware as necessary, and make
+        * sure that the SCK line is in the correct idle state. It is not
+        * allowed to claim the same bus for several slaves without releasing
+        * the bus in between.
+        *
+        * @device:     The SPI slave
+        *
+        * Returns: 0 if the bus was claimed successfully, or a negative value
+        * if it wasn't.
+        */
+       int (*claim_bus)(struct udevice *device);
+
+       /**
+        * Release the SPI bus
+        *
+        * This must be called once for every call to spi_claim_bus() after
+        * all transfers have finished. It may disable any SPI hardware as
+        * appropriate.
+        *
+        * @device:     The SPI slave
+        */
+       int (*release_bus)(struct udevice *device);
+
+       /**
+        * Set the word length for SPI transactions
+        *
+        * Set the word length (number of bits per word) for SPI transactions.
+        *
+        * @device:     The SPI slave
+        * @wordlen:    The number of bits in a word
+        *
+        * Returns: 0 on success, -ve on failure.
+        */
+       int (*set_wordlen)(struct udevice *deiuce, unsigned int wordlen);
+
+       /**
+        * SPI transfer
+        *
+        * This writes "bitlen" bits out the SPI MOSI port and simultaneously
+        * clocks "bitlen" bits in the SPI MISO port.  That's just the way SPI
+        * works.
+        *
+        * The source of the outgoing bits is the "dout" parameter and the
+        * destination of the input bits is the "din" parameter.  Note that
+        * "dout" and "din" can point to the same memory location, in which
+        * case the input data overwrites the output data (since both are
+        * buffered by temporary variables, this is OK).
+        *
+        * spi_xfer() interface:
+        * @device:     The SPI bus which will be sending/receiving the data.
+        * @slave:      The slave device to communicate with
+        * @bitlen:     How many bits to write and read.
+        * @dout:       Pointer to a string of bits to send out.  The bits are
+        *              held in a byte array and are sent MSB first.
+        * @din:        Pointer to a string of bits that will be filled in.
+        * @flags:      A bitwise combination of SPI_XFER_* flags.
+        *
+        * Returns: 0 on success, not -1 on failure
+        */
+       int (*xfer)(struct udevice *device, struct udevice *slave,
+                   unsigned int bitlen, const void *dout, void *din,
+                   unsigned long flags);
+
+       /**
+        * Set transfer speed.
+        * This sets a new speed to be applied for next spi_xfer().
+        * @slave:      The SPI slave
+        * @hz:         The transfer speed
+        * @return 0 if OK, -ve on error
+        */
+       int (*set_speed)(struct udevice *device, uint hz);
+
+       /**
+        * Set the SPI mode/flags
+        *
+        * It is unclear if we want to set speed and mode together instead
+        * of separately.
+        *
+        * @slave:      The SPI slave
+        * @mode:       Requested SPI mode (SPI_... flags)
+        * @return 0 if OK, -ve on error
+        */
+       int (*set_mode)(struct udevice *device, uint mode);
+};
+
+int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp,
+                       struct udevice **devp);
+
+int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
+                       const char *drv_name, const char *dev_name,
+                       struct udevice **devp, struct spi_slave **slavep);
+
+int spi_bind_device(struct udevice *bus, int cs, const char *drv_name,
+                   const char *dev_name, struct udevice **slavep);
+
+int spi_ofdata_to_platdata(const void *blob, int node,
+                          struct spi_slave *spi);
+
+struct sandbox_state;
+int sandbox_spi_get_emul(struct sandbox_state *state,
+                        struct udevice *bus, struct udevice *slave,
+                        struct udevice **emulp);
+
+/* Access the serial operations for a device */
+#define spi_get_ops(dev)       ((struct dm_spi_ops *)(dev)->driver->ops)
+#endif /* CONFIG_DM_SPI */
+
 #endif /* _SPI_H_ */
--

2.0.0.526.g5318336

Quoted 150 lines · Simon Glass

Add a uclass which provides access to SPI buses and includes operations required by SPI.

For a time driver model will need to co-exist with the legacy SPI interface so some parts of the header file are changed depending on which is in use. The exports are adjusted also since some functions are not available with driver model.

Boards must define CONFIG_DM_SPI to use driver model for SPI.

Signed-off-by Simon Glass <[email protected]>

---

 common/exports.c         |   4 +-
 drivers/spi/Makefile     |   4 +
 drivers/spi/spi-uclass.c | 253 +++++++++++++++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h   |   1 +
 include/spi.h            | 140 ++++++++++++++++++++++++++
 5 files changed, 401 insertions(+), 1 deletion(-)
 create mode 100644 drivers/spi/spi-uclass.c

common/exports.c +3 -1

index b97ca48..88fcfc8 100644
@@ -27,10 +27,12 @@ unsigned long get_version(void)
 # define i2c_write         dummy
 # define i2c_read          dummy
 #endif
-#ifndef CONFIG_CMD_SPI
+#if !defined(CONFIG_CMD_SPI) || defined(CONFIG_DM_SPI)
 # define spi_init          dummy
 # define spi_setup_slave   dummy
 # define spi_free_slave    dummy
+#endif
+#ifndef CONFIG_CMD_SPI
 # define spi_claim_bus     dummy
 # define spi_release_bus   dummy
 # define spi_xfer          dummy

drivers/spi/Makefile +4 -0

index f02c35a..d1f1dd0 100644
@@ -6,7 +6,11 @@
 #
 
 # There are many options which enable SPI, so make this library available
+ifdef CONFIG_DM_SPI
+obj-y += spi-uclass.o
+else
 obj-y += spi.o
+endif
 
 obj-$(CONFIG_EP93XX_SPI) += ep93xx_spi.o
 obj-$(CONFIG_ALTERA_SPI) += altera_spi.o

drivers/spi/spi-uclass.c new file +89 -0

new file mode 100644
index 0000000..4057bce
@@ -0,0 +1,253 @@
+/*
+ * Copyright (c) 2014 Google, Inc
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <errno.h>
+#include <fdtdec.h>
+#include <spi.h>
+#include <dm/device-internal.h>
+#include <dm/uclass-internal.h>
+#include <dm/root.h>
+#include <dm/lists.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static int spi_set_speed_mode(struct udevice *bus, int speed, int mode)
+{
+       struct dm_spi_ops *ops;
+       int ret;
+
+       ops = spi_get_ops(bus);
+       if (ops->set_speed)
+               ret = (*ops->set_speed)(bus, speed);
+       else
+               ret = -EINVAL;
+       if (ret) {
+               printf("Cannot set speed (err=%d)\n", ret);
+               return ret;
+       }
+
+       ops = spi_get_ops(bus);
+       if (ops->set_mode)
+               ret = (*ops->set_mode)(bus, mode);
+       else
+               ret = -EINVAL;
+       if (ret) {
+               printf("Cannot set mode (err=%d)\n", ret);
+               return ret;
+       }
+
+       return 0;
+}
+
+int spi_claim_bus(struct spi_slave *slave)
+{
+       struct udevice *dev = slave->dev;
+       struct udevice *bus = dev->parent;
+       struct dm_spi_ops *ops = spi_get_ops(bus);
+       struct dm_spi_bus *spi = bus->uclass_priv;
+       int speed;
+       int ret;
+
+       speed = slave->max_hz;
+       if (spi->max_hz) {
+               if (speed)
+                       speed = min(speed, spi->max_hz);
+               else
+                       speed = spi->max_hz;
+       }
+       if (!speed)
+               speed = 100000;
+       ret = spi_set_speed_mode(bus, speed, slave->mode);
+       if (ret)
+               return ret;
+
+       return ops->claim_bus ? ops->claim_bus(bus) : 0;
+}
+
+void spi_release_bus(struct spi_slave *slave)
+{
+       struct udevice *dev = slave->dev;
+       struct udevice *bus = dev->parent;
+       struct dm_spi_ops *ops = spi_get_ops(bus);
+
+       if (ops->release_bus)
+               spi_get_ops(bus)->release_bus(bus);
+}
+
+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
+            const void *dout, void *din, unsigned long flags)
+{
+       struct udevice *dev = slave->dev;
+       struct udevice *bus = dev->parent;
+
+       if (bus->uclass->uc_drv->id != UCLASS_SPI)
+               return -EOPNOTSUPP;

drivers/spi/spi-uclass.c line 92

Quoted 3 lines

drivers/spi/spi-uclass.c new file +3 -0

+
+       return spi_get_ops(bus)->xfer(bus, dev, bitlen, dout, din, flags);
+}

Cleared all these calls, as individual drivers/spi* will setup their ops and fills
the priv data. So this uclass will call accordingly - correct? add if I'm missing anything.

Quoted 13 lines

drivers/spi/spi-uclass.c new file +12 -0

+
+int spi_post_bind(struct udevice *dev)
+{
+       /* Scan the bus for devices */
+       return dm_scan_fdt_node(dev, gd->fdt_blob, dev->of_offset, false);
+}
+
+int spi_post_probe(struct udevice *dev)
+{
+       struct dm_spi_bus *spi = dev->uclass_priv;
+
+       spi->max_hz = fdtdec_get_int(gd->fdt_blob, dev->of_offset,

drivers/spi/spi-uclass.c line 108

Quoted 4 lines

drivers/spi/spi-uclass.c new file +4 -0

+                                    "spi-max-frequency", 0);
+
+       return 0;
+}

Anyway each spi driver will fills the data from dtb - like spi-max-frequency, why the spi
core (uclass) will do the same.

Can't we reuse the spi_slave, why dm_spi_bus?

Quoted 70 lines

drivers/spi/spi-uclass.c new file +70 -0

+
+int spi_bind_device(struct udevice *bus, int cs, const char *drv_name,
+                   const char *dev_name, struct udevice **slavep)
+{
+       struct driver *drv;
+       int ret;
+
+       drv = lists_driver_lookup_name(drv_name);
+       if (!drv) {
+               puts("Cannot find spi_flash_std driver\n");
+               return -ENOENT;
+       }
+       ret = device_bind(bus, drv, dev_name, NULL, -1, slavep);
+       if (ret) {
+               printf("Cannot create device named '%s' (err=%d)\n",
+                      dev_name, ret);
+               return ret;
+       }
+       (*slavep)->req_seq = cs;
+
+       return 0;
+}
+
+int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp,
+                       struct udevice **devp)
+{
+       struct udevice *bus, *dev;
+       int ret;
+
+       ret = uclass_find_device_by_seq(UCLASS_SPI, busnum, false, &bus);
+       if (ret)
+               return ret;
+       ret = device_find_child_by_seq(bus, cs, false, &dev);
+       if (ret)
+               return ret;
+       *busp = bus;
+       *devp = dev;
+
+       return ret;
+}
+
+int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
+                      const char *drv_name, const char *dev_name,
+                      struct udevice **devp, struct spi_slave **slavep)
+{
+       struct udevice *bus, *dev;
+       int ret;
+
+       ret = uclass_get_device_by_seq(UCLASS_SPI, busnum, &bus);
+       if (ret) {
+               printf("Invalid bus %d (err=%d)\n", busnum, ret);
+               return ret;
+       }
+       ret = device_get_child_by_seq(bus, cs, &dev);
+
+       /**
+        * If there is no such device, create one automatically. This means
+        * that we don't need a device tree node or platform data for the
+        * SPI flash chip - we will bind to the correct driver.
+        */
+       if (ret == -ENODEV && drv_name) {
+               ret = spi_bind_device(bus, cs, drv_name, dev_name, &dev);
+               if (ret)
+                       return ret;
+       }
+       if (ret) {
+               printf("Invalid chip select %d:%d (err=%d)\n", busnum, cs,
+                      ret);
+               return ret;
+       }

drivers/spi/spi-uclass.c line 188

Quoted 10 lines

drivers/spi/spi-uclass.c new file +10 -0

+
+       ret = spi_set_speed_mode(bus, speed, mode);
+       if (ret)
+               return ret;
+
+       *devp = bus;
+       *slavep = dev_get_parentdata(dev);
+
+       return 0;
+}

I guess this is one of the replacement for spi_setup_slave() in spi drivers, so how bus and cs member will populate to drivers.

Usually bus and cs members from spi_slave will populate to drivers through "sf probe" so spi_cs_activate will set the respective chip-select and along with
reg_base in driver select through bus value.

spi_setup_slave() {
...
zslave->base = get_zynq_spi_base(bus);
...
}

How these will handling with dm?

Quoted 83 lines

drivers/spi/spi-uclass.c new file +65 -0

+
+/* Compatibility function - to be removed */
+struct spi_slave *spi_setup_slave_fdt(const void *blob, int node,
+                                     int bus_node)
+{
+       struct udevice *bus, *dev;
+       int ret;
+
+       ret = uclass_get_device_by_of_offset(UCLASS_SPI, bus_node, &bus);
+       if (ret)
+               return NULL;
+       ret = device_get_child_by_of_offset(bus, node, &dev);
+       if (ret)
+               return NULL;
+       return dev_get_parentdata(dev);
+}
+
+/* Compatibility function - to be removed */
+struct spi_slave *spi_setup_slave(unsigned int busnum, unsigned int cs,
+                                 unsigned int speed, unsigned int mode)
+{
+       struct spi_slave *slave;
+       struct udevice *dev;
+       int ret;
+
+       ret = spi_get_bus_and_cs(busnum, cs, speed, mode, NULL, 0, &dev,
+                                 &slave);
+       if (ret)
+               return NULL;
+
+       return slave;
+}
+
+void spi_free_slave(struct spi_slave *slave)
+{
+       device_remove(slave->dev);
+       slave->dev = NULL;
+}
+
+int spi_ofdata_to_platdata(const void *blob, int node,
+                          struct spi_slave *spi)
+{
+       int mode = 0;
+
+       spi->max_hz = fdtdec_get_int(blob, node, "spi-max-frequency", 0);
+       if (fdtdec_get_bool(blob, node, "spi-cpol"))
+               mode |= SPI_CPOL;
+       if (fdtdec_get_bool(blob, node, "spi-cpha"))
+               mode |= SPI_CPHA;
+       if (fdtdec_get_bool(blob, node, "spi-cs-high"))
+               mode |= SPI_CS_HIGH;
+       if (fdtdec_get_bool(blob, node, "spi-half-duplex"))
+               mode |= SPI_PREAMBLE;
+       spi->mode = mode;
+
+       return 0;
+}
+
+UCLASS_DRIVER(spi) = {
+       .id             = UCLASS_SPI,
+       .name           = "spi",
+       .post_bind      = spi_post_bind,
+       .post_probe     = spi_post_probe,
+       .per_device_auto_alloc_size = sizeof(struct dm_spi_bus),
+};

include/dm/uclass-id.h +1 -0

index 7f0e37b..8207483 100644
@@ -22,6 +22,7 @@ enum uclass_id {
        /* U-Boot uclasses start here */
        UCLASS_GPIO,            /* Bank of general-purpose I/O pins */
        UCLASS_SERIAL,          /* Serial UART */
+       UCLASS_SPI,             /* SPI bus */
 
        UCLASS_COUNT,
        UCLASS_INVALID = -1,

include/spi.h +0 -0

index b673be2..b5e9347 100644

include/spi.h line 63

Quoted 10 lines

include/spi.h +7 -0

@@ -54,11 +54,19 @@
 
 #define SPI_DEFAULT_WORDLEN 8
 
+#ifdef CONFIG_DM_SPI
+struct dm_spi_bus {
+       uint max_hz;
+};
+
+#endif /* CONFIG_DM_SPI */
+

No idea why this should require, max_hz is already a part of spi_slave and handling of these like getting from dtb and process and fill on priv data should be part of individual drivers and filling spi_slave member should be part of uclass - based on my understanding.

Any comments?

include/spi.h line 69

Quoted 8 lines

include/spi.h +1 -0

 /**
  * struct spi_slave - Representation of a SPI slave
  *
  * Drivers are expected to extend this with controller-specific data.
  *
+ * @dev:               SPI slave device

Missing comments for max_hz and mode

Quoted 162 lines

include/spi.h +132 -0

  * @bus:               ID of the bus that the slave is attached to.
  * @cs:                        ID of the chip select connected to the slave.
  * @op_mode_rx:                SPI RX operation mode.

@@ -71,8 +79,14 @@
  * @flags:             Indication of SPI flags.
  */
 struct spi_slave {
+#ifdef CONFIG_DM_SPI
+       struct udevice *dev;    /* struct spi_slave is dev->parentdata */
+       uint max_hz;
+       uint mode;
+#else
        unsigned int bus;
        unsigned int cs;
+#endif
        u8 op_mode_rx;
        u8 op_mode_tx;
        unsigned int wordlen;

@@ -220,6 +234,8 @@ int spi_set_wordlen(struct spi_slave *slave, unsigned int wordlen);
 int  spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
                void *din, unsigned long flags);
 
+#ifndef CONFIG_DM_SPI
+
 /**
  * Determine if a SPI chipselect is valid.
  * This function is provided by the board if the low-level SPI driver

@@ -255,6 +271,7 @@ void spi_cs_deactivate(struct spi_slave *slave);
  * @hz:                The transfer speed
  */
 void spi_set_speed(struct spi_slave *slave, uint hz);
+#endif
 
 /**
  * Write 8 bits, then read 8 bits.

@@ -305,4 +322,127 @@ struct spi_slave *spi_setup_slave_fdt(const void *blob, int slave_node,
 struct spi_slave *spi_base_setup_slave_fdt(const void *blob, int busnum,
                                           int node);
 
+#ifdef CONFIG_DM_SPI
+
+/**
+ * struct struct dm_spi_ops - Driver model SPI operations
+ *
+ * The uclass interface is implemented by all SPI devices which use
+ * driver model.
+ */
+struct dm_spi_ops {
+       /**
+        * Claim the bus and prepare it for communication.
+        *
+        * The device privided is the slave device. It's parent controller
+        * will be used to provide the communication.
+        *
+        * This must be called before doing any transfers with a SPI slave. It
+        * will enable and initialize any SPI hardware as necessary, and make
+        * sure that the SCK line is in the correct idle state. It is not
+        * allowed to claim the same bus for several slaves without releasing
+        * the bus in between.
+        *
+        * @device:     The SPI slave
+        *
+        * Returns: 0 if the bus was claimed successfully, or a negative value
+        * if it wasn't.
+        */
+       int (*claim_bus)(struct udevice *device);
+
+       /**
+        * Release the SPI bus
+        *
+        * This must be called once for every call to spi_claim_bus() after
+        * all transfers have finished. It may disable any SPI hardware as
+        * appropriate.
+        *
+        * @device:     The SPI slave
+        */
+       int (*release_bus)(struct udevice *device);
+
+       /**
+        * Set the word length for SPI transactions
+        *
+        * Set the word length (number of bits per word) for SPI transactions.
+        *
+        * @device:     The SPI slave
+        * @wordlen:    The number of bits in a word
+        *
+        * Returns: 0 on success, -ve on failure.
+        */
+       int (*set_wordlen)(struct udevice *deiuce, unsigned int wordlen);
+
+       /**
+        * SPI transfer
+        *
+        * This writes "bitlen" bits out the SPI MOSI port and simultaneously
+        * clocks "bitlen" bits in the SPI MISO port.  That's just the way SPI
+        * works.
+        *
+        * The source of the outgoing bits is the "dout" parameter and the
+        * destination of the input bits is the "din" parameter.  Note that
+        * "dout" and "din" can point to the same memory location, in which
+        * case the input data overwrites the output data (since both are
+        * buffered by temporary variables, this is OK).
+        *
+        * spi_xfer() interface:
+        * @device:     The SPI bus which will be sending/receiving the data.
+        * @slave:      The slave device to communicate with
+        * @bitlen:     How many bits to write and read.
+        * @dout:       Pointer to a string of bits to send out.  The bits are
+        *              held in a byte array and are sent MSB first.
+        * @din:        Pointer to a string of bits that will be filled in.
+        * @flags:      A bitwise combination of SPI_XFER_* flags.
+        *
+        * Returns: 0 on success, not -1 on failure
+        */
+       int (*xfer)(struct udevice *device, struct udevice *slave,
+                   unsigned int bitlen, const void *dout, void *din,
+                   unsigned long flags);
+
+       /**
+        * Set transfer speed.
+        * This sets a new speed to be applied for next spi_xfer().
+        * @slave:      The SPI slave
+        * @hz:         The transfer speed
+        * @return 0 if OK, -ve on error
+        */
+       int (*set_speed)(struct udevice *device, uint hz);
+
+       /**
+        * Set the SPI mode/flags
+        *
+        * It is unclear if we want to set speed and mode together instead
+        * of separately.
+        *
+        * @slave:      The SPI slave
+        * @mode:       Requested SPI mode (SPI_... flags)
+        * @return 0 if OK, -ve on error
+        */
+       int (*set_mode)(struct udevice *device, uint mode);
+};
+
+int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp,
+                       struct udevice **devp);
+
+int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
+                       const char *drv_name, const char *dev_name,
+                       struct udevice **devp, struct spi_slave **slavep);
+
+int spi_bind_device(struct udevice *bus, int cs, const char *drv_name,
+                   const char *dev_name, struct udevice **slavep);
+
+int spi_ofdata_to_platdata(const void *blob, int node,
+                          struct spi_slave *spi);
+
+struct sandbox_state;
+int sandbox_spi_get_emul(struct sandbox_state *state,
+                        struct udevice *bus, struct udevice *slave,
+                        struct udevice **emulp);
+
+/* Access the serial operations for a device */
+#define spi_get_ops(dev)       ((struct dm_spi_ops *)(dev)->driver->ops)
+#endif /* CONFIG_DM_SPI */
+

Quoted 4 lines

include/spi.h +0 -1

 #endif /* _SPI_H_ */
--

2.0.0.526.g5318336

thanks!
--
Jagan.