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

[Simon Glass <sjg@chromium.org>]

Hi Michal,

On 26 September 2016 at 05:06, Michal Simek <michal.simek@xilinx.com> wrote:
> On 24.9.2016 19:26, Simon Glass wrote:
>> Hi Michal,
>>
>> On 8 September 2016 at 07:57, Michal Simek <michal.simek@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@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.
>
> TBH I am confused with that too. This is ceva sata driver
> but we use scsi subsystem to work with it.
> From my look sata is mostly copied from scsi but I don't know history of
> it.
> I will look at using just one interface - sata or scsi to see how this
> will look like.

Here is my understanding. I may have it wrong.

SCSI should be a uclass
SATA should be a uclass (called AHCI)

SCSI provide a library of functions which can be called by SCSI or
SATA code. This is distinct from the uclass so really should be in
some sort of common file.

>
>
>> 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().
>
> Will look.
>
>>
>> Also we will need a sandbox device at some point so we can run tests.
>
> This can be added later.
>
>>
>> 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.
>
> ok. Will look at it.
>
>>
>> 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?
>
> It is mostly because I didn't want to create another copy of
> the same functions. We are allocating all stuff on fly that's why
> we are working with one device at time.

I'd rather not use these options if they are not useful.

>
>>
>>> +#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?
>
> yes a lot. It is pointer to device description. For non DM case it is
> statically described based on selected number of devices.

But I can't see scsi_dev_desc in your function.

>
>>
>>>  #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@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?
>
> PCI is using dm_pci_bus_find_bdf() to find out udevice.
> I know it from post_probe function that's why I don't need to look for it.
> Not sure why PCI is using this function.

I think you might need to do DM_SCSI first to separate the SCSI code
from the SCSI helper functions (the ones that deal with SCSI
messages). Then DM_SATA after that?

>
>>
>>> -
>>> +#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
>
> Thanks,
> Michal
>

Regards,
Simon