[U-Boot] [RFC PATCH 1/2] dm: Add support for scsi/sata based devices

Simon Glass sjg at chromium.org
Sat Sep 24 19:26:30 CEST 2016


Hi Michal,

On 8 September 2016 at 07:57, Michal Simek <michal.simek at xilinx.com> wrote:
> All sata based drivers are bind and corresponding block
> device is created. Based on this find_scsi_device() is able
> to get back block device based on scsi_curr_dev pointer.
>
> intr_scsi() is commented now but it can be replaced by calling
> find_scsi_device() and scsi_scan().
>
> scsi_dev_desc[] is commented out but common/scsi.c heavily depends on
> it. That's why CONFIG_SYS_SCSI_MAX_DEVICE is hardcoded to 1 and symbol
> is reassigned to a block description allocated by uclass.
> There is only one block description by device now but it doesn't need to
> be correct when more devices are present.
>
> scsi_bind() ensures corresponding block device creation.
> uclass post_probe (scsi_post_probe()) is doing low level init.
>
> SCSI/SATA DM based drivers requires to have 64bit base address as
> the first entry in platform data structure to setup mmio_base.
>
> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
> ---
>
>  cmd/scsi.c                  | 38 ++++++++++++++++++++++++++++++++++++++
>  common/board_r.c            |  4 ++--
>  common/scsi.c               | 17 ++++++++++++++++-
>  drivers/block/ahci-uclass.c | 38 ++++++++++++++++++++++++++++++++++++++
>  drivers/block/ahci.c        | 30 ++++++++++++++++++++++--------
>  include/ahci.h              |  2 +-
>  include/sata.h              |  3 +++
>  include/scsi.h              | 15 ++++++++++++++-
>  8 files changed, 134 insertions(+), 13 deletions(-)

Thanks for looking at this. I've taken a look and have a few comments.

It's confusing that you are changing both scsi and sata. Do you need
to add a DM_SCSI also? As far as I can see, they are separate
subsystems.

I think you need a uclass which implements the scsi_scan() function.
The existing code could be refactored so that the common parts are
called from both scsi.c and your scsi-uclass.c. It should look for
devices, and then create a block device for each. Since you don't know
how many block devices to create, I don't think you can avoid creating
them 'on the fly' in scsi_scan(). For an example, see
usb_stor_probe_device().

Also we will need a sandbox device at some point so we can run tests.

Minor point - please put #idef CONFIG_DM_SATA first and the legacy
path in the #else cause. Mostly you do this but in a few cases it is
not consistent.

A few more notes below.

>
> diff --git a/cmd/scsi.c b/cmd/scsi.c
> index 387ca1a262ab..dc1176610672 100644
> --- a/cmd/scsi.c
> +++ b/cmd/scsi.c
> @@ -10,6 +10,7 @@
>   */
>  #include <common.h>
>  #include <command.h>
> +#include <dm.h>
>  #include <scsi.h>
>
>  static int scsi_curr_dev; /* current device */
> @@ -25,6 +26,23 @@ int do_scsiboot(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>  /*
>   * scsi command intepreter
>   */
> +#ifdef CONFIG_DM_SATA
> +struct udevice *find_scsi_device(int dev_num)
> +{
> +       struct udevice *bdev;
> +       int ret;
> +
> +       ret = blk_get_device(IF_TYPE_SCSI, dev_num, &bdev);
> +
> +       if (ret) {
> +               printf("SCSI Device %d not found\n", dev_num);
> +               return NULL;
> +       }
> +
> +       return bdev;
> +}
> +#endif
> +
>  int do_scsi(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>  {
>         switch (argc) {
> @@ -35,7 +53,18 @@ int do_scsi(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>                 if (strncmp(argv[1], "res", 3) == 0) {
>                         printf("\nReset SCSI\n");
>                         scsi_bus_reset();
> +
> +#if defined(CONFIG_DM_SATA)
> +                       struct udevice *bdev;
> +
> +                       bdev = find_scsi_device(scsi_curr_dev);
> +                       if (!bdev)
> +                               return CMD_RET_FAILURE;
> +
> +                       scsi_scan(1, bdev);
> +#else
>                         scsi_scan(1);
> +#endif
>                         return 0;
>                 }
>                 if (strncmp(argv[1], "inf", 3) == 0) {
> @@ -51,7 +80,16 @@ int do_scsi(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
>                         return 0;
>                 }
>                 if (strncmp(argv[1], "scan", 4) == 0) {
> +#if defined(CONFIG_DM_SATA)
> +                       struct udevice *bdev;
> +
> +                       bdev = find_scsi_device(scsi_curr_dev);
> +                       if (!bdev)
> +                               return CMD_RET_FAILURE;
> +                       scsi_scan(1, bdev);
> +#else
>                         scsi_scan(1);
> +#endif
>                         return 0;
>                 }
>                 if (strncmp(argv[1], "part", 4) == 0) {
> diff --git a/common/board_r.c b/common/board_r.c
> index d959ad3c6f90..f3ea457507de 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -620,7 +620,7 @@ static int initr_ambapp_print(void)
>  }
>  #endif
>
> -#if defined(CONFIG_SCSI)
> +#if defined(CONFIG_SCSI) && !defined(CONFIG_DM_SATA)
>  static int initr_scsi(void)
>  {
>         puts("SCSI:  ");
> @@ -923,7 +923,7 @@ init_fnc_t init_sequence_r[] = {
>         initr_ambapp_print,
>  #endif
>  #endif
> -#ifdef CONFIG_SCSI
> +#if defined(CONFIG_SCSI) && !defined(CONFIG_DM_SATA)
>         INIT_FUNC_WATCHDOG_RESET
>         initr_scsi,
>  #endif
> diff --git a/common/scsi.c b/common/scsi.c
> index dbbf4043b22a..1726898b06e2 100644
> --- a/common/scsi.c
> +++ b/common/scsi.c
> @@ -26,7 +26,7 @@
>  #define SCSI_VEND_ID 0x10b9
>  #define SCSI_DEV_ID  0x5288
>
> -#elif !defined(CONFIG_SCSI_AHCI_PLAT)
> +#elif !defined(CONFIG_SCSI_AHCI_PLAT) && !defined(CONFIG_DM_SATA)
>  #error no scsi device defined
>  #endif
>  #define SCSI_DEV_LIST {SCSI_VEND_ID, SCSI_DEV_ID}
> @@ -43,7 +43,13 @@ static int scsi_max_devs; /* number of highest available scsi device */
>
>  static int scsi_curr_dev; /* current device */
>
> +#if !defined(CONFIG_DM_SATA)
>  static struct blk_desc scsi_dev_desc[CONFIG_SYS_SCSI_MAX_DEVICE];
> +#else
> +#undef CONFIG_SYS_SCSI_MAX_DEVICE
> +#define CONFIG_SYS_SCSI_MAX_DEVICE     1
> +#define CONFIG_SYS_SCSI_MAX_LUN        1

Do we need these with driver model, or can we scan?

> +#endif
>
>  /* almost the maximum amount of the scsi_ext command.. */
>  #define SCSI_MAX_READ_BLK 0xFFFF
> @@ -160,6 +166,7 @@ static ulong scsi_read(struct blk_desc *block_dev, lbaint_t blknr,
>  {
>  #ifdef CONFIG_BLK
>         struct blk_desc *block_dev = dev_get_uclass_platdata(dev);
> +       struct blk_desc *scsi_dev_desc = block_dev;

Is this actually used?

>  #endif
>         int device = block_dev->devnum;
>         lbaint_t start, blks;
> @@ -235,6 +242,7 @@ static ulong scsi_write(struct blk_desc *block_dev, lbaint_t blknr,
>  {
>  #ifdef CONFIG_BLK
>         struct blk_desc *block_dev = dev_get_uclass_platdata(dev);
> +       struct blk_desc *scsi_dev_desc = block_dev;
>  #endif
>         int device = block_dev->devnum;
>         lbaint_t start, blks;
> @@ -462,13 +470,20 @@ void scsi_setup_test_unit_ready(ccb *pccb)
>   * (re)-scan the scsi bus and reports scsi device info
>   * to the user if mode = 1
>   */
> +#ifdef CONFIG_DM_SATA
> +void scsi_scan(int mode, struct udevice *bdev)
> +#else
>  void scsi_scan(int mode)
> +#endif
>  {
>         unsigned char i, perq, modi, lun;
>         lbaint_t capacity;
>         unsigned long blksz;
>         ccb *pccb = (ccb *)&tempccb;
>
> +#if defined(CONFIG_DM_SATA)
> +       struct blk_desc *scsi_dev_desc = dev_get_uclass_platdata(bdev);
> +#endif
>         if (mode == 1)
>                 printf("scanning bus for devices...\n");
>         for (i = 0; i < CONFIG_SYS_SCSI_MAX_DEVICE; i++) {
> diff --git a/drivers/block/ahci-uclass.c b/drivers/block/ahci-uclass.c
> index 7b8c32699f53..f25b1bd336ae 100644
> --- a/drivers/block/ahci-uclass.c
> +++ b/drivers/block/ahci-uclass.c
> @@ -1,14 +1,52 @@
>  /*
>   * Copyright (c) 2015 Google, Inc
>   * Written by Simon Glass <sjg at chromium.org>
> + * Copyright (c) 2016 Xilinx, Inc
> + * Written by Michal Simek
>   *
>   * SPDX-License-Identifier:    GPL-2.0+
>   */
>
>  #include <common.h>
>  #include <dm.h>
> +#include <scsi.h>
> +
> +#if defined(CONFIG_DM_SATA)
> +int scsi_bind(struct udevice *dev)
> +{
> +       struct udevice *bdev;
> +       struct blk_desc *bdesc;
> +       int ret;
> +
> +       /*
> +        * FIXME - maybe loop over CONFIG_SYS_SCSI_MAX_SCSI_ID
> +        * here to support more ports
> +        */
> +       ret = blk_create_devicef(dev, "scsi_blk", "blk", IF_TYPE_SCSI, -1, 512,
> +                                0, &bdev);
> +       if (ret) {
> +               printf("Cannot create block device\n");
> +               return ret;
> +       }
> +       bdesc = dev_get_uclass_platdata(bdev);
> +       debug("device %p, block device %p, block description %p\n",
> +             dev, bdev, bdesc);
> +
> +       return 0;
> +}
> +
> +static int scsi_post_probe(struct udevice *dev)
> +{
> +       debug("%s: device %p\n", __func__, dev);
> +       scsi_low_level_init(0, dev);
> +       return 0;
> +}
> +#endif
>
>  UCLASS_DRIVER(ahci) = {
>         .id             = UCLASS_AHCI,
>         .name           = "ahci",
> +#if defined(CONFIG_DM_SATA)
> +       .post_probe      = scsi_post_probe,
> +#endif
>  };
> diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c
> index e3e783a74cfd..03a95179eaa4 100644
> --- a/drivers/block/ahci.c
> +++ b/drivers/block/ahci.c
> @@ -168,7 +168,7 @@ int ahci_reset(void __iomem *base)
>
>  static int ahci_host_init(struct ahci_probe_ent *probe_ent)
>  {
> -#ifndef CONFIG_SCSI_AHCI_PLAT
> +#if !defined(CONFIG_SCSI_AHCI_PLAT) && !defined(CONFIG_DM_SATA)
>  # ifdef CONFIG_DM_PCI
>         struct udevice *dev = probe_ent->dev;
>         struct pci_child_platdata *pplat = dev_get_parent_platdata(dev);
> @@ -198,7 +198,7 @@ static int ahci_host_init(struct ahci_probe_ent *probe_ent)
>         writel(cap_save, mmio + HOST_CAP);
>         writel_with_flush(0xf, mmio + HOST_PORTS_IMPL);
>
> -#ifndef CONFIG_SCSI_AHCI_PLAT
> +#if !defined(CONFIG_SCSI_AHCI_PLAT) && !defined(CONFIG_DM_SATA)
>  # ifdef CONFIG_DM_PCI
>         if (pplat->vendor == PCI_VENDOR_ID_INTEL) {
>                 u16 tmp16;
> @@ -327,6 +327,7 @@ static int ahci_host_init(struct ahci_probe_ent *probe_ent)
>         writel(tmp | HOST_IRQ_EN, mmio + HOST_CTL);
>         tmp = readl(mmio + HOST_CTL);
>         debug("HOST_CTL 0x%x\n", tmp);
> +#if !defined(CONFIG_DM_SATA)
>  #ifndef CONFIG_SCSI_AHCI_PLAT
>  # ifdef CONFIG_DM_PCI
>         dm_pci_read_config16(dev, PCI_COMMAND, &tmp16);
> @@ -338,14 +339,15 @@ static int ahci_host_init(struct ahci_probe_ent *probe_ent)
>         pci_write_config_word(pdev, PCI_COMMAND, tmp16);
>  # endif
>  #endif
> +#endif
>         return 0;
>  }
>
>
>  static void ahci_print_info(struct ahci_probe_ent *probe_ent)
>  {
> -#ifndef CONFIG_SCSI_AHCI_PLAT
> -# ifdef CONFIG_DM_PCI
> +#if !defined(CONFIG_SCSI_AHCI_PLAT) && !defined(CONFIG_DM_SATA)
> +# if defined(CONFIG_DM_PCI) || defined(CONFIG_DM_SATA)
>         struct udevice *dev = probe_ent->dev;
>  # else
>         pci_dev_t pdev = probe_ent->dev;
> @@ -372,7 +374,7 @@ static void ahci_print_info(struct ahci_probe_ent *probe_ent)
>         else
>                 speed_s = "?";
>
> -#ifdef CONFIG_SCSI_AHCI_PLAT
> +#if defined(CONFIG_SCSI_AHCI_PLAT) || defined(CONFIG_DM_SATA)
>         scc_s = "SATA";
>  #else
>  # ifdef CONFIG_DM_PCI
> @@ -424,13 +426,15 @@ static void ahci_print_info(struct ahci_probe_ent *probe_ent)
>  }
>
>  #ifndef CONFIG_SCSI_AHCI_PLAT
> -# ifdef CONFIG_DM_PCI
> +# if defined(CONFIG_DM_PCI) || defined(CONFIG_DM_SATA)
>  static int ahci_init_one(struct udevice *dev)
>  # else
>  static int ahci_init_one(pci_dev_t dev)
>  # endif
>  {
> +#if defined(CONFIG_DM_PCI)
>         u16 vendor;
> +#endif
>         int rc;
>
>         probe_ent = malloc(sizeof(struct ahci_probe_ent));
> @@ -450,6 +454,7 @@ static int ahci_init_one(pci_dev_t dev)
>         probe_ent->pio_mask = 0x1f;
>         probe_ent->udma_mask = 0x7f;    /*Fixme,assume to support UDMA6 */
>
> +#if !defined(CONFIG_DM_SATA)
>  #ifdef CONFIG_DM_PCI
>         probe_ent->mmio_base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_5,
>                                               PCI_REGION_MEM);
> @@ -473,6 +478,10 @@ static int ahci_init_one(pci_dev_t dev)
>         if (vendor == 0x197b)
>                 pci_write_config_byte(dev, 0x41, 0xa1);
>  #endif
> +#else
> +       struct scsi_platdata *plat = dev_get_platdata(dev);
> +       probe_ent->mmio_base = (void *)plat->base;
> +#endif
>
>         debug("ahci mmio_base=0x%p\n", probe_ent->mmio_base);
>         /* initialize adapter */
> @@ -954,14 +963,17 @@ int scsi_exec(ccb *pccb)
>
>  }
>
> -
> +#if defined(CONFIG_DM_SATA)
> +void scsi_low_level_init(int busdevfunc, struct udevice *dev)
> +#else
>  void scsi_low_level_init(int busdevfunc)
> +#endif
>  {
>         int i;
>         u32 linkmap;
>
>  #ifndef CONFIG_SCSI_AHCI_PLAT
> -# ifdef CONFIG_DM_PCI
> +# if defined(CONFIG_DM_PCI)
>         struct udevice *dev;
>         int ret;
>
> @@ -969,6 +981,8 @@ void scsi_low_level_init(int busdevfunc)
>         if (ret)
>                 return;
>         ahci_init_one(dev);
> +# elif defined(CONFIG_DM_SATA)
> +       ahci_init_one(dev);
>  # else
>         ahci_init_one(busdevfunc);
>  # endif
> diff --git a/include/ahci.h b/include/ahci.h
> index a956c6ff5df7..e740273de804 100644
> --- a/include/ahci.h
> +++ b/include/ahci.h
> @@ -145,7 +145,7 @@ struct ahci_ioports {
>  };
>
>  struct ahci_probe_ent {
> -#ifdef CONFIG_DM_PCI
> +#if defined(CONFIG_DM_PCI) || defined(CONFIG_DM_SATA)
>         struct udevice *dev;
>  #else
>         pci_dev_t       dev;
> diff --git a/include/sata.h b/include/sata.h
> index b35359aa5a19..26c8de730399 100644
> --- a/include/sata.h
> +++ b/include/sata.h
> @@ -2,6 +2,8 @@
>  #define __SATA_H__
>  #include <part.h>
>
> +#if !defined(CONFIG_DM_SATA)
> +
>  int init_sata(int dev);
>  int reset_sata(int dev);
>  int scan_sata(int dev);
> @@ -15,5 +17,6 @@ int __sata_stop(void);
>  int sata_port_status(int dev, int port);
>
>  extern struct blk_desc sata_dev_desc[];
> +#endif
>
>  #endif
> diff --git a/include/scsi.h b/include/scsi.h
> index 7e3759140b34..22d6bd0d02a7 100644
> --- a/include/scsi.h
> +++ b/include/scsi.h
> @@ -166,14 +166,27 @@ typedef struct SCSI_cmd_block{
>  void scsi_print_error(ccb *pccb);
>  int scsi_exec(ccb *pccb);
>  void scsi_bus_reset(void);
> +#if !defined(CONFIG_DM_SATA)
>  void scsi_low_level_init(int busdevfunc);

What will happen to these functions?

> -
> +#else
> +void scsi_low_level_init(int busdevfunc, struct udevice *dev);
> +#endif
>
>  /***************************************************************************
>   * functions residing inside cmd_scsi.c
>   */
>  void scsi_init(void);
> +#if !defined(CONFIG_DM_SATA)
>  void scsi_scan(int mode);
> +#else
> +
> +struct scsi_platdata {
> +       unsigned long base;
> +};
> +
> +void scsi_scan(int mode, struct udevice *bdev);
> +int scsi_bind(struct udevice *dev);
> +#endif
>
>  /** @return the number of scsi disks */
>  int scsi_get_disk_count(void);
> --
> 1.9.1
>

Regards,
Simon


More information about the U-Boot mailing list