[U-Boot,v3,05/29] dm: spi: Add a uclass for SPI

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 <sjg@chromium.org>
---

Changes in v3:
- Add a cs_info() method to the driver model SPI API
- Add a uclass for a generic SPI device (for use with the 'sspi' command)
- Add missing comments to spi.h
- Correct typo where 'slave' should say 'bus'
- Fix two comment typos
- Put the cs member back into spi_slave
- Use an explicit chip select value instead of reusing device sequence number

Changes in v2:
- Add missing comments for struct spi_slave
- Fix code nits from Daniel Schwierzeck
- Use 'bus' instead of 'dev' to make the API clearer

 common/exports.c         |   4 +-
 drivers/spi/Makefile     |   4 +
 drivers/spi/spi-uclass.c | 390 +++++++++++++++++++++++++++++++++++++++++++++++
 include/dm/uclass-id.h   |   2 +
 include/spi.h            | 254 +++++++++++++++++++++++++++++-
 5 files changed, 650 insertions(+), 4 deletions(-)
 create mode 100644 drivers/spi/spi-uclass.c

Hi Simon,

Can you please see my comments below, I'm relating more about cs and bus
valid scenario which is available on current drivers wrt dm-spi.

On 30 September 2014 01:05, Simon Glass <sjg@chromium.org> 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 <sjg@chromium.org>
> ---
>
> Changes in v3:
> - Add a cs_info() method to the driver model SPI API

I think this gives the enough info regarding cs on particular controller what
about the bus usage - which is also specific to controller it self, right?

Say - spi controller on a particular soc, have spi0, spi1 means two buses
and 4 cs lines, cs0-cs3. How is this works in that case?

And also I saw tegra20_sflash.c validates only cs

int tegra20_sflash_cs_info(struct udevice *bus, unsigned int cs,
                           struct spi_cs_info *info)

What about bus number check this particular tegra spi controller is concern.

> - Add a uclass for a generic SPI device (for use with the 'sspi' command)
> - Add missing comments to spi.h
> - Correct typo where 'slave' should say 'bus'
> - Fix two comment typos
> - Put the cs member back into spi_slave
> - Use an explicit chip select value instead of reusing device sequence number
>
> Changes in v2:
> - Add missing comments for struct spi_slave
> - Fix code nits from Daniel Schwierzeck
> - Use 'bus' instead of 'dev' to make the API clearer
>
>  common/exports.c         |   4 +-
>  drivers/spi/Makefile     |   4 +
>  drivers/spi/spi-uclass.c | 390 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/dm/uclass-id.h   |   2 +
>  include/spi.h            | 254 +++++++++++++++++++++++++++++-
>  5 files changed, 650 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/spi/spi-uclass.c
>
> diff --git a/common/exports.c b/common/exports.c
> index b97ca48..88fcfc8 100644
> --- a/common/exports.c
> +++ b/common/exports.c
> @@ -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
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index f02c35a..d1f1dd0 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -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
> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c
> new file mode 100644
> index 0000000..13c6b77
> --- /dev/null
> +++ b/drivers/spi/spi-uclass.c
> @@ -0,0 +1,390 @@
> +/*
> + * Copyright (c) 2014 Google, Inc
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <fdtdec.h>
> +#include <malloc.h>
> +#include <spi.h>
> +#include <dm/device-internal.h>
> +#include <dm/uclass-internal.h>
> +#include <dm/root.h>
> +#include <dm/lists.h>
> +#include <dm/util.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;
> +       }
> +
> +       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)
> +               ops->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;
> +
> +       return spi_get_ops(bus)->xfer(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_chip_select(struct udevice *dev)
> +{
> +       struct spi_slave *slave = dev_get_parentdata(dev);
> +
> +       return slave ? slave->cs : -ENOENT;
> +}
> +
> +/**
> + * spi_find_chip_select() - Find the slave attached to chip select
> + *
> + * @bus:       SPI bus to search
> + * @cs:                Chip select to look for
> + * @devp:      Returns the slave device if found
> + * @return 0 if found, -ENODEV on error
> + */
> +static int spi_find_chip_select(struct udevice *bus, int cs,
> +                               struct udevice **devp)
> +{
> +       struct udevice *dev;
> +
> +       for (device_find_first_child(bus, &dev); dev;
> +            device_find_next_child(&dev)) {
> +               struct spi_slave store;
> +               struct spi_slave *slave = dev_get_parentdata(dev);
> +
> +               if (!slave)  {
> +                       slave = &store;
> +                       spi_ofdata_to_platdata(gd->fdt_blob, dev->of_offset,
> +                                              slave);
> +               }
> +               debug("%s: slave=%p, cs=%d\n", __func__, slave,
> +                     slave ? slave->cs : -1);
> +               if (slave && slave->cs == cs) {
> +                       *devp = dev;
> +                       return 0;
> +               }
> +       }
> +
> +       return -ENODEV;
> +}
> +
> +int spi_cs_is_valid(unsigned int busnum, unsigned int cs)

Who is the caller for this?

> +{
> +       struct spi_cs_info info;
> +       struct udevice *bus;
> +       int ret;
> +
> +       ret = uclass_find_device_by_seq(UCLASS_SPI, busnum, false, &bus);
> +       if (ret) {
> +               debug("%s: No bus %d\n", __func__, busnum);
> +               return ret;
> +       }
> +
> +       return spi_cs_info(bus, cs, &info);
> +}
> +
> +int spi_cs_info(struct udevice *bus, uint cs, struct spi_cs_info *info)
> +{
> +       struct spi_cs_info local_info;
> +       struct dm_spi_ops *ops;
> +       int ret;
> +
> +       if (!info)
> +               info = &local_info;
> +
> +       /* If there is a device attached, return it */
> +       info->dev = NULL;
> +       ret = spi_find_chip_select(bus, cs, &info->dev);
> +       if (!ret)
> +               return 0;
> +
> +       /*
> +        * Otherwise ask the driver. For the moment we don't have CS info.
> +        * When we do we could provide the driver with a helper function
> +        * to figure out what chip selects are valid, or just handle the
> +        * request.
> +        */
> +       ops = spi_get_ops(bus);
> +       if (ops->cs_info)
> +               return ops->cs_info(bus, cs, info);
> +
> +       /*
> +        * We could assume there is at least one valid chip select, but best
> +        * to be sure and return an error in this case. The driver didn't
> +        * care enough to tell us.
> +        */
> +       return -ENODEV;
> +}
> +
> +int spi_bind_device(struct udevice *bus, int cs, const char *drv_name,
> +                   const char *dev_name, struct udevice **devp)
> +{
> +       struct driver *drv;
> +       int ret;
> +
> +       drv = lists_driver_lookup_name(drv_name);
> +       if (!drv) {
> +               printf("Cannot find driver '%s'\n", drv_name);
> +               return -ENOENT;
> +       }
> +       ret = device_bind(bus, drv, dev_name, NULL, -1, devp);
> +       if (ret) {
> +               printf("Cannot create device named '%s' (err=%d)\n",
> +                      dev_name, ret);
> +               return ret;
> +       }
> +
> +       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) {
> +               debug("%s: No bus %d\n", __func__, busnum);
> +               return ret;
> +       }
> +       ret = spi_find_chip_select(bus, cs, &dev);
> +       if (ret) {
> +               debug("%s: No cs %d\n", __func__, cs);
> +               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 **busp, struct spi_slave **devp)
> +{
> +       struct udevice *bus, *dev;
> +       struct spi_slave *slave;
> +       bool created = false;
> +       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 = spi_find_chip_select(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) {
> +               debug("%s: Binding new device '%s', busnum=%d, cs=%d, driver=%s\n",
> +                     __func__, dev_name, busnum, cs, drv_name);
> +               ret = spi_bind_device(bus, cs, drv_name, dev_name, &dev);
> +               if (ret)
> +                       return ret;
> +               created = true;
> +       } else if (ret) {
> +               printf("Invalid chip select %d:%d (err=%d)\n", busnum, cs,
> +                      ret);
> +               return ret;
> +       }
> +
> +       if (!device_active(dev)) {
> +               slave = (struct spi_slave *)calloc(1,
> +                                                  sizeof(struct spi_slave));
> +               if (!slave) {
> +                       ret = -ENOMEM;
> +                       goto err;
> +               }
> +
> +               ret = spi_ofdata_to_platdata(gd->fdt_blob, dev->of_offset,
> +                                            slave);
> +               if (ret)
> +                       goto err;
> +               slave->cs = cs;
> +               slave->dev = dev;
> +               ret = device_probe_child(dev, slave);
> +               free(slave);
> +               if (ret)
> +                       goto err;
> +       }
> +
> +       ret = spi_set_speed_mode(bus, speed, mode);
> +       if (ret)
> +               goto err;
> +
> +       *busp = bus;
> +       *devp = dev_get_parentdata(dev);
> +       debug("%s: bus=%p, slave=%p\n", __func__, bus, *devp);
> +
> +       return 0;
> +
> +err:
> +       if (created) {
> +               device_remove(dev);
> +               device_unbind(dev);
> +       }
> +
> +       return ret;
> +}
> +
> +/* 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->cs = fdtdec_get_int(blob, node, "reg", -1);
> +       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),
> +};
> +
> +UCLASS_DRIVER(spi_generic) = {
> +       .id             = UCLASS_SPI_GENERIC,
> +       .name           = "spi_generic",
> +};
> +
> +U_BOOT_DRIVER(spi_generic_drv) = {
> +       .name           = "spi_generic_drv",
> +       .id             = UCLASS_SPI_GENERIC,
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 7f0e37b..0afdc75 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -22,6 +22,8 @@ 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_SPI_GENERIC,     /* Generic SPI flash target */
>
>         UCLASS_COUNT,
>         UCLASS_INVALID = -1,
> diff --git a/include/spi.h b/include/spi.h
> index b673be2..89949b1 100644
> --- a/include/spi.h
> +++ b/include/spi.h
> @@ -54,12 +54,31 @@
>
>  #define SPI_DEFAULT_WORDLEN 8
>
> +#ifdef CONFIG_DM_SPI
> +struct dm_spi_bus {
> +       uint max_hz;
> +};
> +
> +#endif /* CONFIG_DM_SPI */
> +
>  /**
>   * struct spi_slave - Representation of a SPI slave
>   *
> - * Drivers are expected to extend this with controller-specific data.
> + * For driver model this is the per-child data used by the SPI bus. It can
> + * be accessed using dev_get_parentdata() on the slave device. Each SPI
> + * driver should define this child data in its U_BOOT_DRIVER() definition:
> + *
> + *     .per_child_auto_alloc_size      = sizeof(struct spi_slave),
>   *
> - * @bus:               ID of the bus that the slave is attached to.
> + * If not using driver model, drivers are expected to extend this with
> + * controller-specific data.
> + *
> + * @dev:               SPI slave device
> + * @max_hz:            Maximum speed for this slave
> + * @mode:              SPI mode to use for this slave (see SPI mode flags)
> + * @bus:               ID of the bus that the slave is attached to. For
> + *                     driver model this is the sequence number of the SPI
> + *                     bus (bus->seq) so does not need to be stored
>   * @cs:                        ID of the chip select connected to the slave.
>   * @op_mode_rx:                SPI RX operation mode.
>   * @op_mode_tx:                SPI TX operation mode.
> @@ -71,7 +90,13 @@
>   * @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;
> +#endif
>         unsigned int cs;
>         u8 op_mode_rx;
>         u8 op_mode_tx;
> @@ -228,8 +253,9 @@ int  spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
>   * Returns: 1 if bus:cs identifies a valid chip on this board, 0
>   * otherwise.
>   */
> -int  spi_cs_is_valid(unsigned int bus, unsigned int cs);
> +int spi_cs_is_valid(unsigned int bus, unsigned int cs);
>
> +#ifndef CONFIG_DM_SPI
>  /**
>   * Activate a SPI chipselect.
>   * This function is provided by the board code when using a driver
> @@ -255,6 +281,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 +332,225 @@ 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 spi_cs_info - Information about a bus chip select
> + *
> + * @dev:       Connected device, or NULL if none
> + */
> +struct spi_cs_info {
> +       struct udevice *dev;
> +};
> +
> +/**
> + * 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 provided 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.
> +        *
> +        * @bus:        The SPI slave
> +        *
> +        * Returns: 0 if the bus was claimed successfully, or a negative value
> +        * if it wasn't.
> +        */
> +       int (*claim_bus)(struct udevice *bus);
> +
> +       /**
> +        * 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.
> +        *
> +        * @bus:        The SPI slave
> +        */
> +       int (*release_bus)(struct udevice *bus);
> +
> +       /**
> +        * Set the word length for SPI transactions
> +        *
> +        * Set the word length (number of bits per word) for SPI transactions.
> +        *
> +        * @bus:        The SPI slave
> +        * @wordlen:    The number of bits in a word
> +        *
> +        * Returns: 0 on success, -ve on failure.
> +        */
> +       int (*set_wordlen)(struct udevice *bus, 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:
> +        * @dev:        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 *dev, 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().
> +        * @bus:        The SPI bus
> +        * @hz:         The transfer speed
> +        * @return 0 if OK, -ve on error
> +        */
> +       int (*set_speed)(struct udevice *bus, uint hz);
> +
> +       /**
> +        * Set the SPI mode/flags
> +        *
> +        * It is unclear if we want to set speed and mode together instead
> +        * of separately.
> +        *
> +        * @bus:        The SPI bus
> +        * @mode:       Requested SPI mode (SPI_... flags)
> +        * @return 0 if OK, -ve on error
> +        */
> +       int (*set_mode)(struct udevice *bus, uint mode);
> +
> +       /**
> +        * Get information on a chip select
> +        *
> +        * This is only called when the SPI uclass does not know about a
> +        * chip select, i.e. it has no attached device. It gives the driver
> +        * a chance to allow activity on that chip select even so.
> +        *
> +        * @bus:        The SPI bus
> +        * @cs:         The chip select (0..n-1)
> +        * @info:       Returns information about the chip select, if valid.
> +        *              On entry info->dev is NULL
> +        * @return 0 if OK (and @info is set up), -ENODEV if the chip select
> +        *         is invalid, other -ve value on error
> +        */
> +       int (*cs_info)(struct udevice *bus, uint cs, struct spi_cs_info *info);
> +};
> +
> +/**
> + * spi_find_bus_and_cs() - Find bus and slave devices by number
> + *
> + * Given a bus number and chip select, this finds the corresponding bus
> + * device and slave device. Neither device is activated by this function,
> + * although they may have been activated previously.
> + *
> + * @busnum:    SPI bus number
> + * @cs:                Chip select to look for
> + * @busp:      Returns bus device
> + * @devp:      Return slave device
> + * @return 0 if found, -ENODEV on error
> + */
> +int spi_find_bus_and_cs(int busnum, int cs, struct udevice **busp,
> +                       struct udevice **devp);
> +
> +/**
> + * spi_get_bus_and_cs() - Find and activate bus and slave devices by number
> + *
> + * Given a bus number and chip select, this finds the corresponding bus
> + * device and slave device.
> + *
> + * If no such slave exists, and drv_name is not NULL, then a new slave device
> + * is automatically bound on this chip select.
> + *
> + * Ths new slave device is probed ready for use with the given speed and mode.
> + *
> + * @busnum:    SPI bus number
> + * @cs:                Chip select to look for
> + * @speed:     SPI speed to use for this slave
> + * @mode:      SPI mode to use for this slave
> + * @drv_name:  Name of driver to attach to this chip select
> + * @dev_name:  Name of the new device thus created
> + * @busp:      Returns bus device
> + * @devp:      Return slave device
> + * @return 0 if found, -ve on error
> + */
> +int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode,
> +                       const char *drv_name, const char *dev_name,
> +                       struct udevice **busp, struct spi_slave **devp);
> +
> +/**
> + * spi_chip_select() - Get the chip select for a slave
> + *
> + * @return the chip select this slave is attached to
> + */
> +int spi_chip_select(struct udevice *slave);
> +
> +/**
> + * spi_bind_device() - bind a device to a bus's chip select
> + *
> + * This binds a new device to an given chip select (which must be unused).
> + *
> + * @bus:       SPI bus to search
> + * @cs:                Chip select to attach to
> + * @drv_name:  Name of driver to attach to this chip select
> + * @dev_name:  Name of the new device thus created
> + * @devp:      Returns the newly bound device
> + */
> +int spi_bind_device(struct udevice *bus, int cs, const char *drv_name,
> +                   const char *dev_name, struct udevice **devp);
> +
> +/**
> + * spi_ofdata_to_platdata() - decode standard SPI platform data
> + *
> + * This decodes the speed and mode from a device tree node and puts it into
> + * the spi_slave structure.
> + *
> + * @blob:      Device tree blob
> + * @node:      Node offset to read from
> + * @spi:       Place to put the decoded information
> + */
> +int spi_ofdata_to_platdata(const void *blob, int node, struct spi_slave *spi);
> +
> +/**
> + * spi_cs_info() - Check information on a chip select
> + *
> + * This checks a particular chip select on a bus to see if it has a device
> + * attached, or is even valid.
> + *
> + * @bus:       The SPI bus
> + * @cs:                The chip select (0..n-1)
> + * @info:      Returns information about the chip select, if valid
> + * @return 0 if OK (and @info is set up), -ENODEV if the chip select
> + *        is invalid, other -ve value on error
> + */
> +int spi_cs_info(struct udevice *bus, uint cs, struct spi_cs_info *info);
> +
> +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.1.0.rc2.206.gedb03e5
>

thanks!

Hi Jagan,

On 10 October 2014 11:31, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> Hi Simon,
>
> Can you please see my comments below, I'm relating more about cs and bus
> valid scenario which is available on current drivers wrt dm-spi.
>
> On 30 September 2014 01:05, Simon Glass <sjg@chromium.org> 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 <sjg@chromium.org>
>> ---
>>
>> Changes in v3:
>> - Add a cs_info() method to the driver model SPI API
>
> I think this gives the enough info regarding cs on particular controller what
> about the bus usage - which is also specific to controller it self, right?
>
> Say - spi controller on a particular soc, have spi0, spi1 means two buses
> and 4 cs lines, cs0-cs3. How is this works in that case?
>
> And also I saw tegra20_sflash.c validates only cs
>
> int tegra20_sflash_cs_info(struct udevice *bus, unsigned int cs,
>                            struct spi_cs_info *info)
>
> What about bus number check this particular tegra spi controller is concern.

The bus device is passed in as a pointer. The bus number gets looked
up to find the device for that bus number. See spi_get_bus_and_cs()
for that.

This is good because drivers no longer need to validate the bus number.

>
>> - Add a uclass for a generic SPI device (for use with the 'sspi' command)
>> - Add missing comments to spi.h
>> - Correct typo where 'slave' should say 'bus'
>> - Fix two comment typos
>> - Put the cs member back into spi_slave
>> - Use an explicit chip select value instead of reusing device sequence number
>>
>> Changes in v2:
>> - Add missing comments for struct spi_slave
>> - Fix code nits from Daniel Schwierzeck
>> - Use 'bus' instead of 'dev' to make the API clearer
>>
>>  common/exports.c         |   4 +-
>>  drivers/spi/Makefile     |   4 +
>>  drivers/spi/spi-uclass.c | 390 +++++++++++++++++++++++++++++++++++++++++++++++
>>  include/dm/uclass-id.h   |   2 +
>>  include/spi.h            | 254 +++++++++++++++++++++++++++++-
>>  5 files changed, 650 insertions(+), 4 deletions(-)
>>  create mode 100644 drivers/spi/spi-uclass.c
>>


>> +int spi_cs_is_valid(unsigned int busnum, unsigned int cs)
>
> Who is the caller for this?

This can be called by boards, or anywhere really. It replaces the
function with the same purpose in the old SPI framework.

Regards,
Simon

Hi Simon,

On 10 October 2014 23:35, Simon Glass <sjg@chromium.org> wrote:
> Hi Jagan,
>
> On 10 October 2014 11:31, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>> Hi Simon,
>>
>> Can you please see my comments below, I'm relating more about cs and bus
>> valid scenario which is available on current drivers wrt dm-spi.
>>
>> On 30 September 2014 01:05, Simon Glass <sjg@chromium.org> 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 <sjg@chromium.org>
>>> ---
>>>
>>> Changes in v3:
>>> - Add a cs_info() method to the driver model SPI API
>>
>> I think this gives the enough info regarding cs on particular controller what
>> about the bus usage - which is also specific to controller it self, right?
>>
>> Say - spi controller on a particular soc, have spi0, spi1 means two buses
>> and 4 cs lines, cs0-cs3. How is this works in that case?
>>
>> And also I saw tegra20_sflash.c validates only cs
>>
>> int tegra20_sflash_cs_info(struct udevice *bus, unsigned int cs,
>>                            struct spi_cs_info *info)
>>
>> What about bus number check this particular tegra spi controller is concern.
>
> The bus device is passed in as a pointer. The bus number gets looked
> up to find the device for that bus number. See spi_get_bus_and_cs()
> for that.
>
> This is good because drivers no longer need to validate the bus number.

Agreed, but for controlling "sf probe bus:cs " options bus number
should get from
the respective driver.

I have a suggestion, like in spi_find_bus_and_cs() or spi_get_bus_and_cs()
we have a function calls for uclass_get_device_by_seq() and
spi_find_bus_and_cs()
along with that ops->cs_info(bus, cs, info) by adding bus number check
for the driver.

So-that there is no requirement to call spi_cs_is_valid from any other location
as bus and cs validates through commands itself.

BTW: in tegra20_sflash.c, how does tegra20_sflash_cs_info() called?

>
>>
>>> - Add a uclass for a generic SPI device (for use with the 'sspi' command)
>>> - Add missing comments to spi.h
>>> - Correct typo where 'slave' should say 'bus'
>>> - Fix two comment typos
>>> - Put the cs member back into spi_slave
>>> - Use an explicit chip select value instead of reusing device sequence number
>>>
>>> Changes in v2:
>>> - Add missing comments for struct spi_slave
>>> - Fix code nits from Daniel Schwierzeck
>>> - Use 'bus' instead of 'dev' to make the API clearer
>>>
>>>  common/exports.c         |   4 +-
>>>  drivers/spi/Makefile     |   4 +
>>>  drivers/spi/spi-uclass.c | 390 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/dm/uclass-id.h   |   2 +
>>>  include/spi.h            | 254 +++++++++++++++++++++++++++++-
>>>  5 files changed, 650 insertions(+), 4 deletions(-)
>>>  create mode 100644 drivers/spi/spi-uclass.c
>>>
>
>
>>> +int spi_cs_is_valid(unsigned int busnum, unsigned int cs)
>>
>> Who is the caller for this?
>
> This can be called by boards, or anywhere really. It replaces the
> function with the same purpose in the old SPI framework.

Please see above.

thanks!

Hi Jagan,

On 10 October 2014 12:37, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> Hi Simon,
>
> On 10 October 2014 23:35, Simon Glass <sjg@chromium.org> wrote:
>> Hi Jagan,
>>
>> On 10 October 2014 11:31, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> Can you please see my comments below, I'm relating more about cs and bus
>>> valid scenario which is available on current drivers wrt dm-spi.
>>>
>>> On 30 September 2014 01:05, Simon Glass <sjg@chromium.org> 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 <sjg@chromium.org>
>>>> ---
>>>>
>>>> Changes in v3:
>>>> - Add a cs_info() method to the driver model SPI API
>>>
>>> I think this gives the enough info regarding cs on particular controller what
>>> about the bus usage - which is also specific to controller it self, right?
>>>
>>> Say - spi controller on a particular soc, have spi0, spi1 means two buses
>>> and 4 cs lines, cs0-cs3. How is this works in that case?
>>>
>>> And also I saw tegra20_sflash.c validates only cs
>>>
>>> int tegra20_sflash_cs_info(struct udevice *bus, unsigned int cs,
>>>                            struct spi_cs_info *info)
>>>
>>> What about bus number check this particular tegra spi controller is concern.
>>
>> The bus device is passed in as a pointer. The bus number gets looked
>> up to find the device for that bus number. See spi_get_bus_and_cs()
>> for that.
>>
>> This is good because drivers no longer need to validate the bus number.
>
> Agreed, but for controlling "sf probe bus:cs " options bus number
> should get from
> the respective driver.

I wonder if the issue here is a difference of understanding about bus
and chip select. In my mind, bus is just a device. We use the bus
number to specify which device we mean, but it is just a a
convenience. If there were some other way of finding the device we
would use it (as we do with device tree, for example).

Similarly, chip select is just a device. We look at the bus device,
find the appropriate chip select number, and end up with a child
device which is referred to by that chip select.

>
> I have a suggestion, like in spi_find_bus_and_cs() or spi_get_bus_and_cs()
> we have a function calls for uclass_get_device_by_seq() and
> spi_find_bus_and_cs()
> along with that ops->cs_info(bus, cs, info) by adding bus number check
> for the driver.
>
> So-that there is no requirement to call spi_cs_is_valid from any other location
> as bus and cs validates through commands itself.

At present spi_cs_info() calls spi_find_chip_select(). I think I see
what you are getting at - you don't want a device to be created unless
cs_info() allows it, right? I will take a look.

>
> BTW: in tegra20_sflash.c, how does tegra20_sflash_cs_info() called?

Only if someone calls spi_cs_info(). I don't think there are any
callers at present. With tegra using device tree I'm not sure it would
have any purpose - the chip selects and buses are all known.

>
>>
>>>
>>>> - Add a uclass for a generic SPI device (for use with the 'sspi' command)
>>>> - Add missing comments to spi.h
>>>> - Correct typo where 'slave' should say 'bus'
>>>> - Fix two comment typos
>>>> - Put the cs member back into spi_slave
>>>> - Use an explicit chip select value instead of reusing device sequence number
>>>>
>>>> Changes in v2:
>>>> - Add missing comments for struct spi_slave
>>>> - Fix code nits from Daniel Schwierzeck
>>>> - Use 'bus' instead of 'dev' to make the API clearer
>>>>
>>>>  common/exports.c         |   4 +-
>>>>  drivers/spi/Makefile     |   4 +
>>>>  drivers/spi/spi-uclass.c | 390 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/dm/uclass-id.h   |   2 +
>>>>  include/spi.h            | 254 +++++++++++++++++++++++++++++-
>>>>  5 files changed, 650 insertions(+), 4 deletions(-)
>>>>  create mode 100644 drivers/spi/spi-uclass.c
>>>>
>>
>>
>>>> +int spi_cs_is_valid(unsigned int busnum, unsigned int cs)
>>>
>>> Who is the caller for this?
>>
>> This can be called by boards, or anywhere really. It replaces the
>> function with the same purpose in the old SPI framework.
>
> Please see above.
>
OK

Regards,
Simon

Hi Simon,

On 11 October 2014 04:56, Simon Glass <sjg@chromium.org> wrote:
> Hi Jagan,
>
> On 10 October 2014 12:37, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>> Hi Simon,
>>
>> On 10 October 2014 23:35, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Jagan,
>>>
>>> On 10 October 2014 11:31, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>>> Hi Simon,
>>>>
>>>> Can you please see my comments below, I'm relating more about cs and bus
>>>> valid scenario which is available on current drivers wrt dm-spi.
>>>>
>>>> On 30 September 2014 01:05, Simon Glass <sjg@chromium.org> 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 <sjg@chromium.org>
>>>>> ---
>>>>>
>>>>> Changes in v3:
>>>>> - Add a cs_info() method to the driver model SPI API
>>>>
>>>> I think this gives the enough info regarding cs on particular controller what
>>>> about the bus usage - which is also specific to controller it self, right?
>>>>
>>>> Say - spi controller on a particular soc, have spi0, spi1 means two buses
>>>> and 4 cs lines, cs0-cs3. How is this works in that case?
>>>>
>>>> And also I saw tegra20_sflash.c validates only cs
>>>>
>>>> int tegra20_sflash_cs_info(struct udevice *bus, unsigned int cs,
>>>>                            struct spi_cs_info *info)
>>>>
>>>> What about bus number check this particular tegra spi controller is concern.
>>>
>>> The bus device is passed in as a pointer. The bus number gets looked
>>> up to find the device for that bus number. See spi_get_bus_and_cs()
>>> for that.
>>>
>>> This is good because drivers no longer need to validate the bus number.
>>
>> Agreed, but for controlling "sf probe bus:cs " options bus number
>> should get from
>> the respective driver.
>
> I wonder if the issue here is a difference of understanding about bus
> and chip select. In my mind, bus is just a device. We use the bus
> number to specify which device we mean, but it is just a a
> convenience. If there were some other way of finding the device we
> would use it (as we do with device tree, for example).

Yes, I do believe that by mentioning bus spi0, spi1 on the devicetree
so the respective bus get's bind'ed till that fine. But as we use runtime
probe using "sf probe bus:cs" where I would mention
# sf probe 2:0
selecting spi2 which is of third bus on the same controller driver will that
spi_cs_is_valid() will show spi2 is invalid for this particular driver?

>
> Similarly, chip select is just a device. We look at the bus device,
> find the appropriate chip select number, and end up with a child
> device which is referred to by that chip select.

Just like validating supported cs, through cs_info I assume the same
works for bus as well as this is a pure driver specific theory.

>
>>
>> I have a suggestion, like in spi_find_bus_and_cs() or spi_get_bus_and_cs()
>> we have a function calls for uclass_get_device_by_seq() and
>> spi_find_bus_and_cs()
>> along with that ops->cs_info(bus, cs, info) by adding bus number check
>> for the driver.
>>
>> So-that there is no requirement to call spi_cs_is_valid from any other location
>> as bus and cs validates through commands itself.
>
> At present spi_cs_info() calls spi_find_chip_select(). I think I see
> what you are getting at - you don't want a device to be created unless
> cs_info() allows it, right? I will take a look.

Yes ie my point, along with that instead of a separate caller for spi_cs_info()
why can't we call in the probe time, either from command or at the time of
bus_cs binding.

Once the bus_cs binding done, and validate the mentioned bus and cs from
command or devicetree and return.

Does this make sense, please comment?

>
>>
>> BTW: in tegra20_sflash.c, how does tegra20_sflash_cs_info() called?
>
> Only if someone calls spi_cs_info(). I don't think there are any
> callers at present. With tegra using device tree I'm not sure it would
> have any purpose - the chip selects and buses are all known.

Yes devicetree will help to take but validating should be require as driver
only knows how many bus's with respective supported chip-selects based
on this command user may verify the same. what do think?

>
>>
>>>
>>>>
>>>>> - Add a uclass for a generic SPI device (for use with the 'sspi' command)
>>>>> - Add missing comments to spi.h
>>>>> - Correct typo where 'slave' should say 'bus'
>>>>> - Fix two comment typos
>>>>> - Put the cs member back into spi_slave
>>>>> - Use an explicit chip select value instead of reusing device sequence number
>>>>>
>>>>> Changes in v2:
>>>>> - Add missing comments for struct spi_slave
>>>>> - Fix code nits from Daniel Schwierzeck
>>>>> - Use 'bus' instead of 'dev' to make the API clearer
>>>>>
>>>>>  common/exports.c         |   4 +-
>>>>>  drivers/spi/Makefile     |   4 +
>>>>>  drivers/spi/spi-uclass.c | 390 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  include/dm/uclass-id.h   |   2 +
>>>>>  include/spi.h            | 254 +++++++++++++++++++++++++++++-
>>>>>  5 files changed, 650 insertions(+), 4 deletions(-)
>>>>>  create mode 100644 drivers/spi/spi-uclass.c
>>>>>
>>>
>>>
>>>>> +int spi_cs_is_valid(unsigned int busnum, unsigned int cs)
>>>>
>>>> Who is the caller for this?
>>>
>>> This can be called by boards, or anywhere really. It replaces the
>>> function with the same purpose in the old SPI framework.
>>
>> Please see above.
>>
> OK

thanks!

Hi Jagan,

On 11 October 2014 11:33, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> Hi Simon,
>
> On 11 October 2014 04:56, Simon Glass <sjg@chromium.org> wrote:
>> Hi Jagan,
>>
>> On 10 October 2014 12:37, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On 10 October 2014 23:35, Simon Glass <sjg@chromium.org> wrote:
>>>> Hi Jagan,
>>>>
>>>> On 10 October 2014 11:31, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>>>> Hi Simon,
>>>>>
>>>>> Can you please see my comments below, I'm relating more about cs and bus
>>>>> valid scenario which is available on current drivers wrt dm-spi.
>>>>>
>>>>> On 30 September 2014 01:05, Simon Glass <sjg@chromium.org> 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 <sjg@chromium.org>
>>>>>> ---
>>>>>>
>>>>>> Changes in v3:
>>>>>> - Add a cs_info() method to the driver model SPI API
>>>>>
>>>>> I think this gives the enough info regarding cs on particular controller what
>>>>> about the bus usage - which is also specific to controller it self, right?
>>>>>
>>>>> Say - spi controller on a particular soc, have spi0, spi1 means two buses
>>>>> and 4 cs lines, cs0-cs3. How is this works in that case?
>>>>>
>>>>> And also I saw tegra20_sflash.c validates only cs
>>>>>
>>>>> int tegra20_sflash_cs_info(struct udevice *bus, unsigned int cs,
>>>>>                            struct spi_cs_info *info)
>>>>>
>>>>> What about bus number check this particular tegra spi controller is concern.
>>>>
>>>> The bus device is passed in as a pointer. The bus number gets looked
>>>> up to find the device for that bus number. See spi_get_bus_and_cs()
>>>> for that.
>>>>
>>>> This is good because drivers no longer need to validate the bus number.
>>>
>>> Agreed, but for controlling "sf probe bus:cs " options bus number
>>> should get from
>>> the respective driver.
>>
>> I wonder if the issue here is a difference of understanding about bus
>> and chip select. In my mind, bus is just a device. We use the bus
>> number to specify which device we mean, but it is just a a
>> convenience. If there were some other way of finding the device we
>> would use it (as we do with device tree, for example).
>
> Yes, I do believe that by mentioning bus spi0, spi1 on the devicetree
> so the respective bus get's bind'ed till that fine. But as we use runtime
> probe using "sf probe bus:cs" where I would mention
> # sf probe 2:0
> selecting spi2 which is of third bus on the same controller driver will that
> spi_cs_is_valid() will show spi2 is invalid for this particular driver?

Run-time binding of SPI buses is supported by driver model (just call
device_bind()) but not by the sf probe command. We should know what
SPI buses exist through device tree or platform data.

So no, the that method is intended to validate a chip select, not a bus.

>
>>
>> Similarly, chip select is just a device. We look at the bus device,
>> find the appropriate chip select number, and end up with a child
>> device which is referred to by that chip select.
>
> Just like validating supported cs, through cs_info I assume the same
> works for bus as well as this is a pure driver specific theory.

Not at present. In fact I feel that both bus and cs should be known in
advance, but to keep compatibility (and since there is no platform
data available for chip selects) I have added the auto-bind used by
the 'sf probe' and 'sspi' commands.

>
>>
>>>
>>> I have a suggestion, like in spi_find_bus_and_cs() or spi_get_bus_and_cs()
>>> we have a function calls for uclass_get_device_by_seq() and
>>> spi_find_bus_and_cs()
>>> along with that ops->cs_info(bus, cs, info) by adding bus number check
>>> for the driver.
>>>
>>> So-that there is no requirement to call spi_cs_is_valid from any other location
>>> as bus and cs validates through commands itself.
>>
>> At present spi_cs_info() calls spi_find_chip_select(). I think I see
>> what you are getting at - you don't want a device to be created unless
>> cs_info() allows it, right? I will take a look.
>
> Yes ie my point, along with that instead of a separate caller for spi_cs_info()
> why can't we call in the probe time, either from command or at the time of
> bus_cs binding.
>
> Once the bus_cs binding done, and validate the mentioned bus and cs from
> command or devicetree and return.
>
> Does this make sense, please comment?

I'm starting to feel that (beyond the idea of using cs_info() more)
what you are referring to here is best handled in follow-up discussion
and patches once this initial support is merged. There seem to be
several questions that are hard to answer until we actually come to
implementation:

- qspi (could be implemented in sandbox, I don't have a qspi board at present)
- chip selects via GPIO (I am planning to look at this before long)
- platform data for chip selects (likely will be used for at91 or
something like that

>
>>
>>>
>>> BTW: in tegra20_sflash.c, how does tegra20_sflash_cs_info() called?
>>
>> Only if someone calls spi_cs_info(). I don't think there are any
>> callers at present. With tegra using device tree I'm not sure it would
>> have any purpose - the chip selects and buses are all known.
>
> Yes devicetree will help to take but validating should be require as driver
> only knows how many bus's with respective supported chip-selects based
> on this command user may verify the same. what do think?

I'm not sure what you are saying here. Can you please restate this?

The device tree should be able to completely specify the bus and chip
selects. I can't see a reason not to do this.

Regards,
Simon

>
>>
>>>
>>>>
>>>>>
>>>>>> - Add a uclass for a generic SPI device (for use with the 'sspi' command)
>>>>>> - Add missing comments to spi.h
>>>>>> - Correct typo where 'slave' should say 'bus'
>>>>>> - Fix two comment typos
>>>>>> - Put the cs member back into spi_slave
>>>>>> - Use an explicit chip select value instead of reusing device sequence number
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Add missing comments for struct spi_slave
>>>>>> - Fix code nits from Daniel Schwierzeck
>>>>>> - Use 'bus' instead of 'dev' to make the API clearer
>>>>>>
>>>>>>  common/exports.c         |   4 +-
>>>>>>  drivers/spi/Makefile     |   4 +
>>>>>>  drivers/spi/spi-uclass.c | 390 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  include/dm/uclass-id.h   |   2 +
>>>>>>  include/spi.h            | 254 +++++++++++++++++++++++++++++-
>>>>>>  5 files changed, 650 insertions(+), 4 deletions(-)
>>>>>>  create mode 100644 drivers/spi/spi-uclass.c
>>>>>>
>>>>
>>>>
>>>>>> +int spi_cs_is_valid(unsigned int busnum, unsigned int cs)
>>>>>
>>>>> Who is the caller for this?
>>>>
>>>> This can be called by boards, or anywhere really. It replaces the
>>>> function with the same purpose in the old SPI framework.
>>>
>>> Please see above.
>>>
>> OK
>
> thanks!
> --
> Jagan.

Hi Simon,

On 11 October 2014 23:13, Simon Glass <sjg@chromium.org> wrote:
> Hi Jagan,
>
> On 11 October 2014 11:33, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>> Hi Simon,
>>
>> On 11 October 2014 04:56, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Jagan,
>>>
>>> On 10 October 2014 12:37, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On 10 October 2014 23:35, Simon Glass <sjg@chromium.org> wrote:
>>>>> Hi Jagan,
>>>>>
>>>>> On 10 October 2014 11:31, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>>>>> Hi Simon,
>>>>>>
>>>>>> Can you please see my comments below, I'm relating more about cs and bus
>>>>>> valid scenario which is available on current drivers wrt dm-spi.
>>>>>>
>>>>>> On 30 September 2014 01:05, Simon Glass <sjg@chromium.org> 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 <sjg@chromium.org>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v3:
>>>>>>> - Add a cs_info() method to the driver model SPI API
>>>>>>
>>>>>> I think this gives the enough info regarding cs on particular controller what
>>>>>> about the bus usage - which is also specific to controller it self, right?
>>>>>>
>>>>>> Say - spi controller on a particular soc, have spi0, spi1 means two buses
>>>>>> and 4 cs lines, cs0-cs3. How is this works in that case?
>>>>>>
>>>>>> And also I saw tegra20_sflash.c validates only cs
>>>>>>
>>>>>> int tegra20_sflash_cs_info(struct udevice *bus, unsigned int cs,
>>>>>>                            struct spi_cs_info *info)
>>>>>>
>>>>>> What about bus number check this particular tegra spi controller is concern.
>>>>>
>>>>> The bus device is passed in as a pointer. The bus number gets looked
>>>>> up to find the device for that bus number. See spi_get_bus_and_cs()
>>>>> for that.
>>>>>
>>>>> This is good because drivers no longer need to validate the bus number.
>>>>
>>>> Agreed, but for controlling "sf probe bus:cs " options bus number
>>>> should get from
>>>> the respective driver.
>>>
>>> I wonder if the issue here is a difference of understanding about bus
>>> and chip select. In my mind, bus is just a device. We use the bus
>>> number to specify which device we mean, but it is just a a
>>> convenience. If there were some other way of finding the device we
>>> would use it (as we do with device tree, for example).
>>
>> Yes, I do believe that by mentioning bus spi0, spi1 on the devicetree
>> so the respective bus get's bind'ed till that fine. But as we use runtime
>> probe using "sf probe bus:cs" where I would mention
>> # sf probe 2:0
>> selecting spi2 which is of third bus on the same controller driver will that
>> spi_cs_is_valid() will show spi2 is invalid for this particular driver?
>
> Run-time binding of SPI buses is supported by driver model (just call
> device_bind()) but not by the sf probe command. We should know what
> SPI buses exist through device tree or platform data.
>
> So no, the that method is intended to validate a chip select, not a bus.
>
>>
>>>
>>> Similarly, chip select is just a device. We look at the bus device,
>>> find the appropriate chip select number, and end up with a child
>>> device which is referred to by that chip select.
>>
>> Just like validating supported cs, through cs_info I assume the same
>> works for bus as well as this is a pure driver specific theory.
>
> Not at present. In fact I feel that both bus and cs should be known in
> advance, but to keep compatibility (and since there is no platform
> data available for chip selects) I have added the auto-bind used by
> the 'sf probe' and 'sspi' commands.
>
>>
>>>
>>>>
>>>> I have a suggestion, like in spi_find_bus_and_cs() or spi_get_bus_and_cs()
>>>> we have a function calls for uclass_get_device_by_seq() and
>>>> spi_find_bus_and_cs()
>>>> along with that ops->cs_info(bus, cs, info) by adding bus number check
>>>> for the driver.
>>>>
>>>> So-that there is no requirement to call spi_cs_is_valid from any other location
>>>> as bus and cs validates through commands itself.
>>>
>>> At present spi_cs_info() calls spi_find_chip_select(). I think I see
>>> what you are getting at - you don't want a device to be created unless
>>> cs_info() allows it, right? I will take a look.
>>
>> Yes ie my point, along with that instead of a separate caller for spi_cs_info()
>> why can't we call in the probe time, either from command or at the time of
>> bus_cs binding.
>>
>> Once the bus_cs binding done, and validate the mentioned bus and cs from
>> command or devicetree and return.
>>
>> Does this make sense, please comment?
>
> I'm starting to feel that (beyond the idea of using cs_info() more)
> what you are referring to here is best handled in follow-up discussion
> and patches once this initial support is merged. There seem to be
> several questions that are hard to answer until we actually come to
> implementation:
>
> - qspi (could be implemented in sandbox, I don't have a qspi board at present)
> - chip selects via GPIO (I am planning to look at this before long)
> - platform data for chip selects (likely will be used for at91 or
> something like that

OK then - will start moving with this initial stuff update accordingly
in follow-up
changes/discussions.

>
>>
>>>
>>>>
>>>> BTW: in tegra20_sflash.c, how does tegra20_sflash_cs_info() called?
>>>
>>> Only if someone calls spi_cs_info(). I don't think there are any
>>> callers at present. With tegra using device tree I'm not sure it would
>>> have any purpose - the chip selects and buses are all known.
>>
>> Yes devicetree will help to take but validating should be require as driver
>> only knows how many bus's with respective supported chip-selects based
>> on this command user may verify the same. what do think?
>
> I'm not sure what you are saying here. Can you please restate this?
>
> The device tree should be able to completely specify the bus and chip
> selects. I can't see a reason not to do this.

I just stated, cs and bus numbers we can get it from devicetree but
for validating
how many number of bus's and cs's for a specific controller we can get through
driver specific call through cs_info().[of course bus is not
validating as of now]

>
> Regards,
> Simon
>
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> - Add a uclass for a generic SPI device (for use with the 'sspi' command)
>>>>>>> - Add missing comments to spi.h
>>>>>>> - Correct typo where 'slave' should say 'bus'
>>>>>>> - Fix two comment typos
>>>>>>> - Put the cs member back into spi_slave
>>>>>>> - Use an explicit chip select value instead of reusing device sequence number
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>> - Add missing comments for struct spi_slave
>>>>>>> - Fix code nits from Daniel Schwierzeck
>>>>>>> - Use 'bus' instead of 'dev' to make the API clearer
>>>>>>>
>>>>>>>  common/exports.c         |   4 +-
>>>>>>>  drivers/spi/Makefile     |   4 +
>>>>>>>  drivers/spi/spi-uclass.c | 390 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>  include/dm/uclass-id.h   |   2 +
>>>>>>>  include/spi.h            | 254 +++++++++++++++++++++++++++++-
>>>>>>>  5 files changed, 650 insertions(+), 4 deletions(-)
>>>>>>>  create mode 100644 drivers/spi/spi-uclass.c
>>>>>>>
>>>>>
>>>>>
>>>>>>> +int spi_cs_is_valid(unsigned int busnum, unsigned int cs)
>>>>>>
>>>>>> Who is the caller for this?
>>>>>
>>>>> This can be called by boards, or anywhere really. It replaces the
>>>>> function with the same purpose in the old SPI framework.
>>>>
>>>> Please see above.
>>>>
>>> OK
>>

thanks!

Hi Jagan,

On 11 October 2014 12:06, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> Hi Simon,
>
> On 11 October 2014 23:13, Simon Glass <sjg@chromium.org> wrote:
>> Hi Jagan,
>>
>> On 11 October 2014 11:33, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On 11 October 2014 04:56, Simon Glass <sjg@chromium.org> wrote:
>>>> Hi Jagan,
>>>>
>>>> On 10 October 2014 12:37, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>>>> Hi Simon,
>>>>>
>>>>> On 10 October 2014 23:35, Simon Glass <sjg@chromium.org> wrote:
>>>>>> Hi Jagan,
>>>>>>
>>>>>> On 10 October 2014 11:31, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>>>>>> Hi Simon,
>>>>>>>
>>>>>>> Can you please see my comments below, I'm relating more about cs and bus
>>>>>>> valid scenario which is available on current drivers wrt dm-spi.
>>>>>>>
>>>>>>> On 30 September 2014 01:05, Simon Glass <sjg@chromium.org> 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 <sjg@chromium.org>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes in v3:
>>>>>>>> - Add a cs_info() method to the driver model SPI API
>>>>>>>
>>>>>>> I think this gives the enough info regarding cs on particular controller what
>>>>>>> about the bus usage - which is also specific to controller it self, right?
>>>>>>>
>>>>>>> Say - spi controller on a particular soc, have spi0, spi1 means two buses
>>>>>>> and 4 cs lines, cs0-cs3. How is this works in that case?
>>>>>>>
>>>>>>> And also I saw tegra20_sflash.c validates only cs
>>>>>>>
>>>>>>> int tegra20_sflash_cs_info(struct udevice *bus, unsigned int cs,
>>>>>>>                            struct spi_cs_info *info)
>>>>>>>
>>>>>>> What about bus number check this particular tegra spi controller is concern.
>>>>>>
>>>>>> The bus device is passed in as a pointer. The bus number gets looked
>>>>>> up to find the device for that bus number. See spi_get_bus_and_cs()
>>>>>> for that.
>>>>>>
>>>>>> This is good because drivers no longer need to validate the bus number.
>>>>>
>>>>> Agreed, but for controlling "sf probe bus:cs " options bus number
>>>>> should get from
>>>>> the respective driver.
>>>>
>>>> I wonder if the issue here is a difference of understanding about bus
>>>> and chip select. In my mind, bus is just a device. We use the bus
>>>> number to specify which device we mean, but it is just a a
>>>> convenience. If there were some other way of finding the device we
>>>> would use it (as we do with device tree, for example).
>>>
>>> Yes, I do believe that by mentioning bus spi0, spi1 on the devicetree
>>> so the respective bus get's bind'ed till that fine. But as we use runtime
>>> probe using "sf probe bus:cs" where I would mention
>>> # sf probe 2:0
>>> selecting spi2 which is of third bus on the same controller driver will that
>>> spi_cs_is_valid() will show spi2 is invalid for this particular driver?
>>
>> Run-time binding of SPI buses is supported by driver model (just call
>> device_bind()) but not by the sf probe command. We should know what
>> SPI buses exist through device tree or platform data.
>>
>> So no, the that method is intended to validate a chip select, not a bus.
>>
>>>
>>>>
>>>> Similarly, chip select is just a device. We look at the bus device,
>>>> find the appropriate chip select number, and end up with a child
>>>> device which is referred to by that chip select.
>>>
>>> Just like validating supported cs, through cs_info I assume the same
>>> works for bus as well as this is a pure driver specific theory.
>>
>> Not at present. In fact I feel that both bus and cs should be known in
>> advance, but to keep compatibility (and since there is no platform
>> data available for chip selects) I have added the auto-bind used by
>> the 'sf probe' and 'sspi' commands.
>>
>>>
>>>>
>>>>>
>>>>> I have a suggestion, like in spi_find_bus_and_cs() or spi_get_bus_and_cs()
>>>>> we have a function calls for uclass_get_device_by_seq() and
>>>>> spi_find_bus_and_cs()
>>>>> along with that ops->cs_info(bus, cs, info) by adding bus number check
>>>>> for the driver.
>>>>>
>>>>> So-that there is no requirement to call spi_cs_is_valid from any other location
>>>>> as bus and cs validates through commands itself.
>>>>
>>>> At present spi_cs_info() calls spi_find_chip_select(). I think I see
>>>> what you are getting at - you don't want a device to be created unless
>>>> cs_info() allows it, right? I will take a look.
>>>
>>> Yes ie my point, along with that instead of a separate caller for spi_cs_info()
>>> why can't we call in the probe time, either from command or at the time of
>>> bus_cs binding.
>>>
>>> Once the bus_cs binding done, and validate the mentioned bus and cs from
>>> command or devicetree and return.
>>>
>>> Does this make sense, please comment?
>>
>> I'm starting to feel that (beyond the idea of using cs_info() more)
>> what you are referring to here is best handled in follow-up discussion
>> and patches once this initial support is merged. There seem to be
>> several questions that are hard to answer until we actually come to
>> implementation:
>>
>> - qspi (could be implemented in sandbox, I don't have a qspi board at present)
>> - chip selects via GPIO (I am planning to look at this before long)
>> - platform data for chip selects (likely will be used for at91 or
>> something like that
>
> OK then - will start moving with this initial stuff update accordingly
> in follow-up
> changes/discussions.

OK.

>
>>
>>>
>>>>
>>>>>
>>>>> BTW: in tegra20_sflash.c, how does tegra20_sflash_cs_info() called?
>>>>
>>>> Only if someone calls spi_cs_info(). I don't think there are any
>>>> callers at present. With tegra using device tree I'm not sure it would
>>>> have any purpose - the chip selects and buses are all known.
>>>
>>> Yes devicetree will help to take but validating should be require as driver
>>> only knows how many bus's with respective supported chip-selects based
>>> on this command user may verify the same. what do think?
>>
>> I'm not sure what you are saying here. Can you please restate this?
>>
>> The device tree should be able to completely specify the bus and chip
>> selects. I can't see a reason not to do this.
>
> I just stated, cs and bus numbers we can get it from devicetree but
> for validating
> how many number of bus's and cs's for a specific controller we can get through
> driver specific call through cs_info().[of course bus is not
> validating as of now]

Let's pick this up when we have a specific problem to resolve, likely
platform data.

Regards,
Simon

>>>>>
>>>>>>
>>>>>>>
>>>>>>>> - Add a uclass for a generic SPI device (for use with the 'sspi' command)
>>>>>>>> - Add missing comments to spi.h
>>>>>>>> - Correct typo where 'slave' should say 'bus'
>>>>>>>> - Fix two comment typos
>>>>>>>> - Put the cs member back into spi_slave
>>>>>>>> - Use an explicit chip select value instead of reusing device sequence number
>>>>>>>>
>>>>>>>> Changes in v2:
>>>>>>>> - Add missing comments for struct spi_slave
>>>>>>>> - Fix code nits from Daniel Schwierzeck
>>>>>>>> - Use 'bus' instead of 'dev' to make the API clearer
>>>>>>>>
>>>>>>>>  common/exports.c         |   4 +-
>>>>>>>>  drivers/spi/Makefile     |   4 +
>>>>>>>>  drivers/spi/spi-uclass.c | 390 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>  include/dm/uclass-id.h   |   2 +
>>>>>>>>  include/spi.h            | 254 +++++++++++++++++++++++++++++-
>>>>>>>>  5 files changed, 650 insertions(+), 4 deletions(-)
>>>>>>>>  create mode 100644 drivers/spi/spi-uclass.c
>>>>>>>>
>>>>>>
>>>>>>
>>>>>>>> +int spi_cs_is_valid(unsigned int busnum, unsigned int cs)
>>>>>>>
>>>>>>> Who is the caller for this?
>>>>>>
>>>>>> This can be called by boards, or anywhere really. It replaces the
>>>>>> function with the same purpose in the old SPI framework.
>>>>>
>>>>> Please see above.
>>>>>
>>>> OK
>>>
>
> thanks!
> --
> Jagan.