[U-Boot] [PATCH] dm: implement a cfi flash uclass

[Simon Glass <sjg@chromium.org>]

Hi Thomas,

On 8 October 2015 at 08:34, Thomas Chou <thomas@wytron.com.tw> wrote:
> Implement a cfi flash uclass to work with drivers/mtd/cfi-flash.c.
> The flash base address is extracted from device tree, and passed
> to cfi_flash_bank_addr().
>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>  common/board_r.c               |  3 ++
>  drivers/mtd/Kconfig            | 11 +++++++
>  drivers/mtd/Makefile           |  1 +
>  drivers/mtd/cfi-flash-uclass.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/cfi_flash.c        | 19 ++++++++++++
>  include/cfi-flash.h            | 27 ++++++++++++++++
>  include/dm/uclass-id.h         |  1 +
>  7 files changed, 132 insertions(+)
>  create mode 100644 drivers/mtd/cfi-flash-uclass.c
>  create mode 100644 include/cfi-flash.h
>

Can you create a sandbox driver for this so you can add a test?

> diff --git a/common/board_r.c b/common/board_r.c
> index aaf390e..86b606d 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -38,6 +38,7 @@
>  #include <miiphy.h>
>  #endif
>  #include <mmc.h>
> +#include <mtd/cfi_flash.h>
>  #include <nand.h>
>  #include <onenand_uboot.h>
>  #include <scsi.h>
> @@ -348,6 +349,8 @@ static int initr_flash(void)
>         /* update start of FLASH memory    */
>  #ifdef CONFIG_SYS_FLASH_BASE
>         bd->bi_flashstart = CONFIG_SYS_FLASH_BASE;
> +#else
> +       bd->bi_flashstart = cfi_flash_bank_addr(0);
>  #endif

Can we make this dynamic - i.e. only probe the device when it is used?
Then we could remove initr_flash() in the DM case.

>         /* size of FLASH memory (final value) */
>         bd->bi_flashsize = flash_size;
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index 59278d1..4b52894 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -1,3 +1,14 @@
> +menu "CFI Flash Support"
> +
> +config DM_CFI_FLASH
> +       bool "Enable Driver Model for CFI Flash drivers"
> +       depends on DM
> +       help
> +         Enable driver model for CFI flash access. It uses the same API as
> +         drivers/mtd/cfi_flash.c. But now implemented by the uclass.

In the help can you explain what CFI is and what it is for?

> +
> +endmenu
> +
>  source "drivers/mtd/nand/Kconfig"
>
>  source "drivers/mtd/spi/Kconfig"
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index a623f4c..d86e2c2 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -11,6 +11,7 @@ endif
>  obj-$(CONFIG_MTD_PARTITIONS) += mtdpart.o
>  obj-$(CONFIG_MTD_CONCAT) += mtdconcat.o
>  obj-$(CONFIG_HAS_DATAFLASH) += at45.o
> +obj-$(CONFIG_DM_CFI_FLASH) += cfi-flash-uclass.o
>  obj-$(CONFIG_FLASH_CFI_DRIVER) += cfi_flash.o
>  obj-$(CONFIG_FLASH_CFI_MTD) += cfi_mtd.o
>  obj-$(CONFIG_HAS_DATAFLASH) += dataflash.o
> diff --git a/drivers/mtd/cfi-flash-uclass.c b/drivers/mtd/cfi-flash-uclass.c
> new file mode 100644
> index 0000000..1db7e2f
> --- /dev/null
> +++ b/drivers/mtd/cfi-flash-uclass.c
> @@ -0,0 +1,70 @@
> +/*
> + * Copyright (C) 2015 Thomas Chou <thomas@wytron.com.tw>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <fdtdec.h>
> +#include <cfi-flash.h>
> +#include <asm/io.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/*
> + * Implement a cfi flash uclass to work with drivers/mtd/cfi-flash.c.
> + */
> +
> +phys_addr_t cfi_flash_get_base(struct udevice *dev)
> +{
> +       struct cfi_flash_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +
> +       return uc_priv->base;
> +}
> +
> +UCLASS_DRIVER(cfi_flash) = {
> +       .id             = UCLASS_CFI_FLASH,
> +       .name           = "cfi_flash",
> +       .per_device_auto_alloc_size = sizeof(struct cfi_flash_dev_priv),
> +};
> +
> +struct cfi_flash_platdata {
> +       phys_addr_t base;
> +};

Can you put this declaration at the top of the file?

> +
> +static int cfi_flash_probe(struct udevice *dev)
> +{
> +       struct cfi_flash_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +       struct cfi_flash_platdata *plat = dev->platdata;
> +
> +       uc_priv->base = plat->base;
> +
> +       return 0;
> +}
> +
> +static int cfi_flash_ofdata_to_platdata(struct udevice *dev)
> +{
> +       struct cfi_flash_platdata *plat = dev_get_platdata(dev);
> +       fdt_size_t size;
> +
> +       fdtdec_get_addr_size(gd->fdt_blob, dev->of_offset, "reg", &size);
> +       plat->base = (phys_addr_t)ioremap(dev_get_addr(dev), size);
> +
> +       return 0;
> +}
> +
> +static const struct udevice_id cfi_flash_ids[] = {
> +       { .compatible = "cfi-flash", },

Is there a binding somewhere for this?

> +       { }
> +};
> +
> +U_BOOT_DRIVER(cfi_flash) = {
> +       .name   = "cfi_flash",
> +       .id     = UCLASS_CFI_FLASH,
> +       .of_match = cfi_flash_ids,
> +       .ofdata_to_platdata = cfi_flash_ofdata_to_platdata,
> +       .platdata_auto_alloc_size = sizeof(struct cfi_flash_platdata),
> +       .probe = cfi_flash_probe,
> +};
> diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
> index 50983b8..6ec0877 100644
> --- a/drivers/mtd/cfi_flash.c
> +++ b/drivers/mtd/cfi_flash.c
> @@ -18,6 +18,9 @@
>  /* #define DEBUG       */
>
>  #include <common.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <cfi-flash.h>
>  #include <asm/processor.h>
>  #include <asm/io.h>
>  #include <asm/byteorder.h>
> @@ -87,10 +90,26 @@ static u16 cfi_flash_config_reg(int i)
>  int cfi_flash_num_flash_banks = CONFIG_SYS_MAX_FLASH_BANKS_DETECT;
>  #endif
>
> +#ifdef CONFIG_DM_CFI_FLASH
> +phys_addr_t cfi_flash_bank_addr(int i)
> +{
> +       struct udevice *dev;
> +       int ret;
> +
> +       ret = uclass_get_device(UCLASS_CFI_FLASH, i, &dev);
> +       if (ret)
> +               return ret;
> +       if (!dev)
> +               return -ENODEV;

That function will never return a NULL dev, unless it returns an
error. It is different from uclass_first_device(). Also are you sure
you want uclass_get_device() and not uclass_get_device_by_seq()?

> +
> +       return cfi_flash_get_base(dev);
> +}
> +#else
>  __weak phys_addr_t cfi_flash_bank_addr(int i)
>  {
>         return ((phys_addr_t [])CONFIG_SYS_FLASH_BANKS_LIST)[i];
>  }
> +#endif
>
>  __weak unsigned long cfi_flash_bank_size(int i)
>  {
> diff --git a/include/cfi-flash.h b/include/cfi-flash.h
> new file mode 100644
> index 0000000..0064cba
> --- /dev/null
> +++ b/include/cfi-flash.h
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (C) 2015 Thomas Chou <thomas@wytron.com.tw>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#ifndef _DM_CFI_FLASH_H_
> +#define _DM_CFI_FLASH_H_
> +
> +/*
> + * Get the cfi flash base address
> + *
> + * @dev: The cfi flash device
> + * @return: the cfi flash base address
> + */
> +phys_addr_t cfi_flash_get_base(struct udevice *dev);
> +
> +/*
> + * struct cfi_flash_dev_priv - information about a device used by the uclass
> + *
> + * @base: the cfi flash base address
> + */
> +struct cfi_flash_dev_priv {
> +       phys_addr_t base;
> +};
> +
> +#endif /* _DM_CFI_FLASH_H_ */
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index a6982ab..09e370c 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -25,6 +25,7 @@ enum uclass_id {
>         UCLASS_SIMPLE_BUS,      /* bus with child devices */
>
>         /* U-Boot uclasses start here - in alphabetical order */
> +       UCLASS_CFI_FLASH,       /* cfi flash */
>         UCLASS_CLK,             /* Clock source, e.g. used by peripherals */
>         UCLASS_CPU,             /* CPU, typically part of an SoC */
>         UCLASS_CROS_EC,         /* Chrome OS EC */
> --
> 2.1.4
>

Regards,
Simon