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

[Simon Glass]

Hi Michal,

On 21 November 2016 at 12:36, Michal Simek <michal.simek at 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 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().
>
> Just a question about this. I have played with this and I haven't looked
> at usb (because I have usb ports gone on my development platform) but
> based on my understanding of our controller (ceva-sata) we support 2
> ports where each of them can have 2 different IDs and I am not quite
> sure about LUN but hardcoding it to 1 should be fine for now.
>
> But all this depends on controller setup. Number of ports should be
> possible to detect from every controller (0x0 offset - cap register 4:0
> number of ports NP).
>
> I have this code. What do you think about it?
> (There is missing loop over number of ports which I will add but
> unfortunately I don't have a HW with this setup here.

I think this is reasonable. Ideally you could avoid creating the block
device until you know it is OK, but failing that, make sure to unbind
it.

>
>  void scsi_scan(int mode)
>  {
>         unsigned char i, lun;
>         struct uclass *uc;
>          struct udevice *dev; /* SCSI controller */
>         int ret;
>
>         if (mode == 1)
>                 printf("scanning bus for devices...\n");
>
>          ret = uclass_get(UCLASS_SCSI, &uc);
>          if (ret)
>                  return;
>
>          uclass_foreach_dev(dev, uc) {
>                 struct scsi_platdata *plat; /* scsi controller platdata */
>                 struct blk_desc *bdesc; /* block device description */
>                 struct udevice *bdev; /* block device */
>                 struct udevice **devp;
>                 int dev_num = 0;
>                 int last_dev_num = -1;
>
>                 /* probe SCSI controller driver */
>                 ret = uclass_get_device_tail(dev, 0, devp);
>                 if (ret)
>                         return;
>
>                 /* Get controller platdata */
>                 plat = dev_get_platdata(dev);
>
>                 for (i = 0; i < plat->max_id; i++) {
>                         for (lun = 0; lun < plat->max_lun; lun++) {
>                                 /*
>                                  * Create only one block device and do detection
>                                  * to make sure that there won't be a lot of
>                                  * block devices created
>                                  */
>                                 if (last_dev_num != dev_num) {
>                                         char str[10];
>                                         snprintf(str, sizeof(str), "lun%d", lun);
>                                         ret = blk_create_devicef(dev, "scsi_blk", str, IF_TYPE_SCSI, -1, 512,
>                                                                 dev_num, &bdev);
>                                         if (ret) {
>                                                 printf("Cannot create block device\n");
>                                                 return;
>                                         }
>                                         last_dev_num = dev_num;
>                                         bdesc = dev_get_uclass_platdata(bdev);
>                                 }
>
>                                 scsi_init_dev_desc(bdesc, dev_num);
>                                 bdesc->lun = lun;
>                                 ret = scsi_detect_dev(i, bdesc);
>                                 if (ret)
>                                         continue;
>
>                                 if (mode == 1) {
>                                         printf("  Device %d: ", 0);
>                                         dev_print(bdesc);
>                                         dev_num++;
>                                 } /* if mode */
>                         } /* next LUN */
>                 }
>         }
>  }
>
>
> Thanks,
> Michal
>

Regards,
Simon