Hi Simon, On 1 September 2014 10:36, Simon Glass <s...@chromium.org> wrote: > Hi Jagan, > > On 28 August 2014 04:32, Jagan Teki <jagannadh.t...@gmail.com> wrote: >> On 15 July 2014 06:26, Simon Glass <s...@chromium.org> wrote: >>> This README is intended to help maintainers move their SPI drivers over to >>> driver model. It works through the required steps with an example. >>> >>> Signed-off-by: Simon Glass <s...@chromium.org> >>> --- >>> >>> doc/driver-model/spi-howto.txt | 570 >>> +++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 570 insertions(+) >>> create mode 100644 doc/driver-model/spi-howto.txt >>> >>> diff --git a/doc/driver-model/spi-howto.txt b/doc/driver-model/spi-howto.txt >>> new file mode 100644 >>> index 0000000..bb64735 >>> --- /dev/null >>> +++ b/doc/driver-model/spi-howto.txt >>> @@ -0,0 +1,570 @@ >>> +How to port a SPI driver to driver model >>> +======================================== >>> + >>> +Here is a rough step-by-step guide. It is based around converting the >>> +exynos SPI driver to driver model (DM) and the example code is based >>> +around U-Boot v2014.04 (commit dda0dbf). >>> + >>> +It is quite long since it includes actual code examples. >>> + >>> +Before driver model, SPI drivers have their own private structure which >>> +contains 'struct spi_slave'. With driver model, 'struct spi_slave' still >>> +exists, but now it is 'per-child data' for the SPI bus. Each child of the >>> +SPI bus is a SPI slave. The information that was stored in the >>> +driver-specific slave structure can now be put in private data for the >>> +SPI bus. >> >> Do you think spi_slave still require, I guess It's needed as some slave >> members >> to cs, bus are used to control the driver. > > The CS and bus are purely command-line conveniences with DM. When it > gets down to the driver the bus is determined by the SPI peripheral it > is connected to, and the CS is the control that it adjusts to enable > the chip select. The numbering of buses and chip selects is not > relevant down at the driver level anymore.
Yes - I understand but let me explain my comments. u-boot> sf probe bus:cs speed mode Unlike other buses/controllers on u-boot we're dynamically probing the slave on bus using bus and cs for example, we have a zynq_spi handling spi0, spi1 (bus's) and each one has 3 chip-selects. So u-boot> sf probe 1:1 for probing bus1 (spi1) and cs 1 u-boot> sf probe 0:1 for probing bus0 (spi0) and cs 1 The respective reg base (based on bus) and register chip-select (based on cs) zynq_spi.c will handle these in driver it self. speed and mode settings, I'm fine with dm, where it will configure using 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) { ..... ret = spi_set_speed_mode(bus, speed, mode); ...... } But how can spi-uclass.c will identifies how many bus's along with cs lines does specified controller driver support. This what usually we're doing in driver as int spi_cs_is_valid(unsigned int bus, unsigned int cs) { /* 2 bus with 3 chipselect */ return bus < 2 && cs < 3; } Please let me know if you have any questions, this is what usually doing most of drivers. > >> >> But any specific reason for removing spi_slave from exynos_spi.c? > > The driver has its own private data, there is no longer a need to > store the SPI subsystem's data there also. That is what the per-child > data is for. > >> >>> + >>> +For example, struct tegra_spi_slave looks like this: >>> + >>> +struct tegra_spi_slave { >>> + struct spi_slave slave; >>> + struct tegra_spi_ctrl *ctrl; >>> +}; >>> + >>> +In this case 'slave' will be in per-child data, and 'ctrl' will be in the >>> +SPI bus' private data. >>> + >>> + >>> +0. How long does this take? >>> + >>> +Around 2.5 hours, including some allowance for figuring out the driver >>> +model bits. >>> + >>> + >>> +1. Enable driver mode for SPI and SPI flash >>> + >>> +Add these to your board config: >>> + >>> +#define CONFIG_DM_SPI >>> +#define CONFIG_DM_SPI_FLASH >>> + >>> + >>> +2. Add the skeleton >>> + >>> +Put this code at the bottom of your existing driver file: >>> + >>> +struct spi_slave *spi_setup_slave(unsigned int busnum, unsigned int cs, >>> + unsigned int max_hz, unsigned int mode) >>> +{ >>> + return NULL; >>> +} >>> + >>> +struct spi_slave *spi_setup_slave_fdt(const void *blob, int slave_node, >>> + int spi_node) >>> +{ >>> + return NULL; >>> +} >>> + >>> +static int exynos_spi_ofdata_to_platdata(struct udevice *dev) >>> +{ >>> + return -ENODEV; >>> +} >>> + >>> +static int exynos_spi_probe(struct udevice *dev) >>> +{ >>> + return -ENODEV; >>> +} >>> + >>> +static int exynos_spi_remove(struct udevice *dev) >>> +{ >>> + return -ENODEV; >>> +} >>> + >>> +static int exynos_spi_claim_bus(struct udevice *dev) >>> +{ >>> + >>> + return -ENODEV; >>> +} >>> + >>> +static int exynos_spi_release_bus(struct udevice *dev) >>> +{ >>> + >>> + return -ENODEV; >>> +} >>> + >>> +static int exynos_spi_xfer(struct udevice *dev, unsigned int bitlen, >>> + const void *dout, void *din, unsigned long >>> flags) >>> +{ >>> + >>> + return -ENODEV; >>> +} >>> + >>> +static int exynos_spi_set_speed(struct udevice *dev, uint speed) >>> +{ >>> + return -ENODEV; >>> +} >>> + >>> +static int exynos_spi_set_mode(struct udevice *dev, uint mode) >>> +{ >>> + return -ENODEV; >>> +} >>> + >>> +static const struct dm_spi_ops exynos_spi_ops = { >>> + .claim_bus = exynos_spi_claim_bus, >>> + .release_bus = exynos_spi_release_bus, >>> + .xfer = exynos_spi_xfer, >>> + .set_speed = exynos_spi_set_speed, >>> + .set_mode = exynos_spi_set_mode, >>> +}; >>> + >>> +static const struct udevice_id exynos_spi_ids[] = { >>> + { .compatible = "samsung,exynos-spi" }, >>> + { } >>> +}; >>> + >>> +U_BOOT_DRIVER(exynos_spi) = { >>> + .name = "exynos_spi", >>> + .id = UCLASS_SPI, >>> + .of_match = exynos_spi_ids, >>> + .ops = &exynos_spi_ops, >>> + .ofdata_to_platdata = exynos_spi_ofdata_to_platdata, >>> + .probe = exynos_spi_probe, >>> + .remove = exynos_spi_remove >> >> comma missing - remove is necessary? >> >>> +}; >>> + >>> + >>> +3. Replace 'exynos' in the above code with your driver name >>> + >>> + >>> +4. #ifdef out all of the code in your driver except for the above >>> + >>> +This will allow you to get it building, which means you can work >>> +incrementally. Since all the methods return an error initially, there is >>> +less chance that you will accidentally leave something in. >>> + >>> +Also, even though your conversion is basically a rewrite, it might help >>> +reviewers if you leave functions in the same place in the file, >>> +particularly for large drivers. >>> + >>> + >>> +5. Add some includes >>> + >>> +Add these includes to your driver: >>> + >>> +#include <dm.h> >>> +#include <errno.h> >>> + >>> + >>> +6. Build >>> + >>> +At this point you should be able to build U-Boot for your board with the >>> +empty SPI driver. You still have empty methods in yur driver, but we will >> >> typo your instead of yur >> >>> +write these one by one. >>> + >>> +If you have spi_init() functions or the like that are called from your >>> +board then the build will fail. Remove these calls and make a note of the >>> +init that needs to be done. >>> + >>> + >>> +7. Set up your platform data structure >>> + >>> +This will hold the information your driver needs to operate, like its >>> +hardware address or maximum frequency. >>> + >>> +You may already have a struct like this, or you may need to create one >>> +from some of the #defines or global variables in the driver. >>> + >>> +Note that this information is not the run-time information. It should not >>> +include state that changes. It should be fixed throughout the life of >>> +U-Boot. Run-time information comes later. >>> + >>> +Here is what was in the exynos spi driver: >>> + >>> +struct spi_bus { >>> + enum periph_id periph_id; >>> + s32 frequency; /* Default clock frequency, -1 for none */ >>> + struct exynos_spi *regs; >>> + int inited; /* 1 if this bus is ready for use */ >>> + int node; >>> + uint deactivate_delay_us; /* Delay to wait after deactivate */ >>> +}; >>> + >>> +Of these, inited is handled by DM and node is the device tree node, which >>> +DM tells you. The name is not quite right. So in this case we would use: >>> + >>> +struct exynos_spi_platdata { >>> + enum periph_id periph_id; >>> + s32 frequency; /* Default clock frequency, -1 for none */ >>> + struct exynos_spi *regs; >>> + uint deactivate_delay_us; /* Delay to wait after deactivate */ >>> +}; >>> + >>> + >>> +8a. Write ofdata_to_platdata() [for device tree only] >>> + >>> +This method will convert information in the device tree node into a C >>> +structure in your driver (called platform data). If you are not using >>> +device tree, go to 8b. >>> + >>> +DM will automatically allocate the struct for us when we are using device >>> +tree, but we need to tell it the size: >>> + >>> +U_BOOT_DRIVER(spi_exynos) = { >>> +... >>> + .platdata_auto_alloc_size = sizeof(struct exynos_spi_platdata), >>> + >>> + >>> +Here is a sample function. It gets a pointer to the platform data and >>> +fills in the fields from device tree. >>> + >>> +static int exynos_spi_ofdata_to_platdata(struct udevice *dev) >>> +{ >>> + struct exynos_spi_platdata *plat = dev->platdata; >>> + const void *blob = gd->fdt_blob; >>> + int node = dev->of_offset; >>> + >>> + plat->regs = (struct exynos_spi *)fdtdec_get_addr(blob, node, >>> "reg"); >>> + plat->periph_id = pinmux_decode_periph_id(blob, node); >>> + >>> + if (plat->periph_id == PERIPH_ID_NONE) { >>> + debug("%s: Invalid peripheral ID %d\n", __func__, >>> + plat->periph_id); >>> + return -FDT_ERR_NOTFOUND; >>> + } >>> + >>> + /* Use 500KHz as a suitable default */ >>> + plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency", >>> + 500000); >>> + plat->deactivate_delay_us = fdtdec_get_int(blob, node, >>> + "spi-deactivate-delay", 0); >>> + >>> + return 0; >>> +} >>> + >>> + >>> +8b. Add the platform data [non-device-tree only] >>> + >>> +Specify this data in a U_BOOT_DEVICE() declaration in your board file: >>> + >>> +struct exynos_spi_platdata platdata_spi0 = { >>> + .periph_id = ... >>> + .frequency = ... >>> + .regs = ... >>> + .deactivate_delay_us = ... >>> +}; >>> + >>> +U_BOOT_DEVICE(board_spi0) = { >>> + .name = "exynos_spi", >>> + .platdata = &platdata_spi0, >>> +}; >>> + >>> +You will unfortunately need to put the struct into a header file in this >>> +case so that your board file can use it. >>> + >>> + >>> +9. Add the device private data >>> + >>> +Most devices have some private data which they use to keep track of things >>> +while active. This is the run-time information and needs to be stored in >>> +a structure. There is probably a structure in the driver that includes a >>> +'struct spi_slave', so you can use that. >>> + >>> +struct exynos_spi_slave { >>> + struct spi_slave slave; >>> + struct exynos_spi *regs; >>> + unsigned int freq; /* Default frequency */ >>> + unsigned int mode; >>> + enum periph_id periph_id; /* Peripheral ID for this device */ >>> + unsigned int fifo_size; >>> + int skip_preamble; >>> + struct spi_bus *bus; /* Pointer to our SPI bus info */ >>> + ulong last_transaction_us; /* Time of last transaction end */ >>> +}; >>> + >>> + >>> +We should rename this to make its purpose more obvious, and get rid of >>> +the slave structure, so we have: >>> + >>> +struct exynos_spi_priv { >>> + struct exynos_spi *regs; >>> + unsigned int freq; /* Default frequency */ >>> + unsigned int mode; >>> + enum periph_id periph_id; /* Peripheral ID for this device */ >>> + unsigned int fifo_size; >>> + int skip_preamble; >>> + ulong last_transaction_us; /* Time of last transaction end */ >>> +}; >>> + >>> + >>> +DM can auto-allocate this also: >>> + >>> +U_BOOT_DRIVER(spi_exynos) = { >>> +... >>> + .priv_auto_alloc_size = sizeof(struct exynos_spi_priv), >>> + >>> + >>> +Note that this is created before the probe() method is called, and >>> destroyed >>> +after the remove() method is called. It will be zeroed when the probe() >>> +method is called. >>> + >>> + >>> +10. Add the probe() and remove() methods >>> + >>> +Note: It's a good idea to build repeatedly as you are working, to avoid a >>> +huge amount of work getting things compiling at the end. >>> + >>> +The probe() method is supposed to set up the hardware. U-Boot used to use >>> +spi_setup_slave() to do this. So take a look at this function and see >>> +what you can copy out to set things up. >>> + >>> + >>> +static int exynos_spi_probe(struct udevice *dev) >>> +{ >>> + struct exynos_spi_platdata *pdata = dev_get_platdata(dev); >>> + struct exynos_spi_priv *priv = dev_get_priv(dev); >>> + >>> + if (pdata->periph_id == PERIPH_ID_SPI1 || >>> + pdata->periph_id == PERIPH_ID_SPI2) >>> + priv->fifo_size = 64; >>> + else >>> + priv->fifo_size = 256; >>> + >>> + priv->skip_preamble = 0; >>> + priv->last_transaction_us = timer_get_us(); >>> + priv->freq = pdata->frequency; >>> + priv->periph_id = pdata->periph_id; >>> + >>> + return 0; >>> +} >>> + >>> +This implementation doesn't actually touch the hardware, which is somewhat >>> +unusual for a driver. In this case we will do that when the device is >>> +claimed by something that wants to use the SPI bus. >>> + >>> +For remove() we could shut down the clocks, but in this case there is >>> +nothing to do. DM frees any memory that it allocated, so we can just >>> +remove exynos_spi_remove() and its reference in U_BOOT_DRIVER. >>> + >>> + >>> +11. Implement set_speed() >>> + >>> +This should set up clocks so that the SPI bus is running at the right >>> +speed. With the old API spi_claim_bus() would normally do this and several >>> +of the following functions, so let's look at that function: >>> + >>> +int spi_claim_bus(struct spi_slave *slave) >>> +{ >>> + struct exynos_spi_slave *spi_slave = to_exynos_spi(slave); >>> + struct exynos_spi *regs = spi_slave->regs; >>> + u32 reg = 0; >>> + int ret; >>> + >>> + ret = set_spi_clk(spi_slave->periph_id, >>> + spi_slave->freq); >>> + if (ret < 0) { >>> + debug("%s: Failed to setup spi clock\n", __func__); >>> + return ret; >>> + } >>> + >>> + exynos_pinmux_config(spi_slave->periph_id, PINMUX_FLAG_NONE); >>> + >>> + spi_flush_fifo(slave); >>> + >>> + reg = readl(®s->ch_cfg); >>> + reg &= ~(SPI_CH_CPHA_B | SPI_CH_CPOL_L); >>> + >>> + if (spi_slave->mode & SPI_CPHA) >>> + reg |= SPI_CH_CPHA_B; >>> + >>> + if (spi_slave->mode & SPI_CPOL) >>> + reg |= SPI_CH_CPOL_L; >>> + >>> + writel(reg, ®s->ch_cfg); >>> + writel(SPI_FB_DELAY_180, ®s->fb_clk); >>> + >>> + return 0; >>> +} >>> + >>> + >>> +It sets up the speed, mode, pinmux, feedback delay and clears the FIFOs. >>> +With DM these will happen in separate methods. >>> + >>> + >>> +Here is an example for the speed part: >>> + >>> +static int exynos_spi_set_speed(struct udevice *dev, uint speed) >>> +{ >>> + struct exynos_spi_priv *priv = dev_get_priv(dev); >>> + int ret; >>> + >>> + ret = set_spi_clk(priv->periph_id, speed); >>> + if (ret) >>> + return ret; >>> + priv->freq = speed; >>> + >>> + return 0; >>> +} >>> + >>> + >>> +12. Implement set_mode() >>> + >>> +This should adjust the SPI mode (polarity, etc.). Again this code probably >>> +comes from the old spi_claim_bus(). Here is an example: >>> + >>> + >>> +static int exynos_spi_set_mode(struct udevice *dev, uint mode) >>> +{ >>> + struct exynos_spi_priv *priv = dev_get_priv(dev); >>> + uint32_t reg; >>> + >>> + reg = readl(&priv->regs->ch_cfg); >>> + reg &= ~(SPI_CH_CPHA_B | SPI_CH_CPOL_L); >>> + >>> + if (mode & SPI_CPHA) >>> + reg |= SPI_CH_CPHA_B; >>> + >>> + if (mode & SPI_CPOL) >>> + reg |= SPI_CH_CPOL_L; >>> + >>> + writel(reg, &priv->regs->ch_cfg); >>> + >>> + return 0; >>> +} >>> + >>> + >>> +13. Implement claim_bus() >>> + >>> +This is where a client wants to make use of the bus, so claims it first. >>> +At this point we need to make sure every is set up ready for data transfer. >>> + >>> +Here again we look at the old claim function and see some code that is >>> +needed. It is anything unrelated to speed and mode: >>> + >>> +static int exynos_spi_claim_bus(struct udevice *dev) >>> +{ >>> + struct exynos_spi_priv *priv = dev_get_priv(dev); >>> + >>> + exynos_pinmux_config(priv->periph_id, PINMUX_FLAG_NONE); >>> + spi_flush_fifo(priv->regs); >>> + >>> + writel(SPI_FB_DELAY_180, &priv->regs->fb_clk); >>> + >>> + return 0; >>> +} >>> + >>> +The spi_flush_fifo() function is in the removed part of the code, so we >>> +need to expose it again (perhaps with an #endif before it and '#if 0' >>> +after it). It only needs accesss to priv->regs which is why we have >>> +passed that in: >>> + >>> +/** >>> + * Flush spi tx, rx fifos and reset the SPI controller >>> + * >>> + * @param regs Pointer to SPI registers >>> + */ >>> +static void spi_flush_fifo(struct exynos_spi *regs) >>> +{ >>> + clrsetbits_le32(®s->ch_cfg, SPI_CH_HS_EN, SPI_CH_RST); >>> + clrbits_le32(®s->ch_cfg, SPI_CH_RST); >>> + setbits_le32(®s->ch_cfg, SPI_TX_CH_ON | SPI_RX_CH_ON); >>> +} >>> + >>> + >>> +14. Implement release_bus() >>> + >>> +This releases the bus - in our example the old code in spi_release_bus() >>> +is a call to spi_flush_fifo, so we add: >>> + >>> +static int exynos_spi_release_bus(struct udevice *dev) >>> +{ >>> + struct exynos_spi_priv *priv = dev_get_priv(dev); >>> + >>> + spi_flush_fifo(priv->regs); >>> + >>> + return 0; >>> +} >>> + >>> + >>> +15. Implement xfer() >>> + >>> +This is the final method that we need to create, and it is where all the >>> +work happens. The method parameters are the same as the old spi_xfer() with >>> +the addition of a 'struct udevice' so conversion is pretty easy. Start >>> +by copying the contents of spi_xfer() to your new xfer() mthod and proceed >> >> typo - method >> >>> +from there. >>> + >>> +If (flags & SPI_XFER_BEGIN) is non-zero then xfer() normally calls an >>> +activate function, something like this: >>> + >>> +void spi_cs_activate(struct spi_slave *slave) >>> +{ >>> + struct exynos_spi_slave *spi_slave = to_exynos_spi(slave); >>> + >>> + /* If it's too soon to do another transaction, wait */ >>> + if (spi_slave->bus->deactivate_delay_us && >>> + spi_slave->last_transaction_us) { >>> + ulong delay_us; /* The delay completed so far */ >>> + delay_us = timer_get_us() - spi_slave->last_transaction_us; >>> + if (delay_us < spi_slave->bus->deactivate_delay_us) >>> + udelay(spi_slave->bus->deactivate_delay_us - >>> delay_us); >>> + } >>> + >>> + clrbits_le32(&spi_slave->regs->cs_reg, SPI_SLAVE_SIG_INACT); >>> + debug("Activate CS, bus %d\n", spi_slave->slave.bus); >>> + spi_slave->skip_preamble = spi_slave->mode & SPI_PREAMBLE; >>> +} >>> + >>> +The new version looks like this: >>> + >>> +static void spi_cs_activate(struct udevice *dev) >>> +{ >>> + struct exynos_spi_platdata *pdata = dev_get_platdata(dev); >>> + struct exynos_spi_priv *priv = dev_get_priv(dev); >>> + >>> + /* If it's too soon to do another transaction, wait */ >>> + if (pdata->deactivate_delay_us && >>> + priv->last_transaction_us) { >>> + ulong delay_us; /* The delay completed so far */ >>> + delay_us = timer_get_us() - priv->last_transaction_us; >>> + if (delay_us < pdata->deactivate_delay_us) >>> + udelay(pdata->deactivate_delay_us - delay_us); >>> + } >>> + >>> + clrbits_le32(&priv->regs->cs_reg, SPI_SLAVE_SIG_INACT); >>> + debug("Activate CS, bus '%s'\n", dev->name); >>> + priv->skip_preamble = priv->mode & SPI_PREAMBLE; >>> +} >>> + >>> +All we have really done here is change the pointers and print the device >>> name >>> +instead of the bus number. Other local static functions can be treated in >>> +the same way. >>> + >>> + >>> +16. Set up the per-child data and child_pre_probe() method >>> + >>> +To minimise the pain and complexity of the SPI subsystem while the driver >> >> typo - minimize > > English spelling (this is U-Boot). > >> >>> +model change-over is in place, struct spi_slave is used to reference a >>> +SPI bus slave, even though that slave is actually a struct udevice. In fact >>> +struct spi_slave is the device's child data. We need to make sure this is >>> +set up. It is possible to allocate more space that struct spi_slave needs, >>> +but this is the minimum. >>> + >>> +U_BOOT_DRIVER(exynos_spi) = { >>> +... >>> + .per_child_auto_alloc_size = sizeof(struct spi_slave), >>> + .child_pre_probe = exynos_spi_child_pre_probe, >>> +} >>> + >>> +int exynos_spi_child_pre_probe(struct udevice *dev) >>> +{ >>> + struct spi_slave *slave = dev_get_parentdata(dev); >>> + >>> + return spi_ofdata_to_platdata(gd->fdt_blob, dev->of_offset, slave); >>> +} >> >> So this routine will fill spi_slave members - is it? >> Why cs and bus are not part of DM? > > See above for my comments, but let me know if unsure. > >> >>> + >>> + >>> +Here our child_pre_probe() method merely calls a standard method to convert >>> +device tree data to that used in the slave. >>> + >>> + >>> +17. Test it >>> + >>> +Now that you have the code written and it compiles, try testing it using >>> +the 'sf test' command. You may need to enable CONFIG_CMD_SF_TEST for your >>> +board. >>> + >>> + >>> +18. Prepare patches and send them to the mailing lists >>> + >>> +You can use 'tools/patman/patman' to prepare, check and send patches for >>> +your work. See the README for details. >>> -- >>> 2.0.0.526.g5318336 thanks! -- Jagan. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot