[U-Boot] [EXT] Re: [PATCH 1/7] scsi: move base, max_lun and max_id to uclass plat data

[Simon Glass]

Hi Ken,

On 5 April 2017 at 02:38, Ken Ma <make at marvell.com> wrote:
> Hi Simon
>
> Please see my inline reply, thanks a lot!
>
> -----Original Message-----
> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
> Sent: 2017年4月1日 12:22
> To: Ken Ma
> Cc: U-Boot Mailing List; Stefan Roese; Michal Simek
> Subject: [EXT] Re: [PATCH 1/7] scsi: move base, max_lun and max_id to uclass plat data
>
> External Email
>
> ----------------------------------------------------------------------
> Hi Ken,
>
> On 23 March 2017 at 03:29,  <make at marvell.com> wrote:
>> From: Ken Ma <make at marvell.com>
>>
>> - The members in scsi_platdata(base, max_lun and max_id) are generic,
>>   so now they are taken from fdt by the uclass_platdata instead of
>>   platdata code upon call to post bind callback.
>>
>> Signed-off-by: Ken Ma <make at marvell.com>
>> Cc: Simon Glass <sjg at chromium.org>
>> Cc: Stefan Roese <sr at denx.de>
>> Cc: Michal Simek <michal.simek at xilinx.com>
>> Reviewed-on: http://vgitil04.il.marvell.com:8080/35304
>> Tested-by: iSoC Platform CI <ykjenk at marvell.com>
>> Reviewed-by: Omri Itach <omrii at marvell.com>
>> Reviewed-by: Kostya Porotchkin <kostap at marvell.com>
>> ---
>>  common/scsi.c               |  2 +-
>>  drivers/block/ahci.c        |  2 +-
>>  drivers/block/scsi-uclass.c | 29 +++++++++++++++++++++++++++++
>>  3 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/scsi.c b/common/scsi.c index fb5b407..117c682
>> 100644
>> --- a/common/scsi.c
>> +++ b/common/scsi.c
>> @@ -574,7 +574,7 @@ int scsi_scan(int mode)
>>                         return ret;
>>
>>                 /* Get controller platdata */
>> -               plat = dev_get_platdata(dev);
>> +               plat = dev_get_uclass_platdata(dev);
>>
>>                 for (i = 0; i < plat->max_id; i++) {
>>                         for (lun = 0; lun < plat->max_lun; lun++) {
>> diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c index
>> 3fa14a7..368816e 100644
>> --- a/drivers/block/ahci.c
>> +++ b/drivers/block/ahci.c
>> @@ -479,7 +479,7 @@ static int ahci_init_one(pci_dev_t dev)
>>                 pci_write_config_byte(dev, 0x41, 0xa1);  #endif  #else
>> -       struct scsi_platdata *plat = dev_get_platdata(dev);
>> +       struct scsi_platdata *plat = dev_get_uclass_platdata(dev);
>>         probe_ent->mmio_base = (void *)plat->base;  #endif
>>
>> diff --git a/drivers/block/scsi-uclass.c b/drivers/block/scsi-uclass.c
>> index 05da6cd..3bf026b 100644
>> --- a/drivers/block/scsi-uclass.c
>> +++ b/drivers/block/scsi-uclass.c
>> @@ -11,8 +11,11 @@
>>
>>  #include <common.h>
>>  #include <dm.h>
>> +#include <dm/device-internal.h>
>>  #include <scsi.h>
>>
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>>  static int scsi_post_probe(struct udevice *dev)  {
>>         debug("%s: device %p\n", __func__, dev); @@ -20,8 +23,34 @@
>> static int scsi_post_probe(struct udevice *dev)
>>         return 0;
>>  }
>>
>> +static void scsi_ofdata_to_uclass_platdata(struct udevice *dev)
>
> Please make this return an int since functions that decode the DT should return an error code.
>
>> +{
>> +       struct scsi_platdata *plat = dev_get_uclass_platdata(dev);
>> +       const void *blob = gd->fdt_blob;
>> +       int node = dev->of_offset;
>> +
>> +       plat->base = (unsigned long)dev_get_addr_ptr(dev);
>> +       plat->max_id = fdtdec_get_uint(blob,
>> +                                      node,
>> +                                      "max-id",
>> +                                      CONFIG_SYS_SCSI_MAX_SCSI_ID);
>> +       plat->max_lun = fdtdec_get_uint(blob,
>> +                                       node,
>> +                                       "max-lun",
>> +                                       CONFIG_SYS_SCSI_MAX_LUN);
>> +       return;
>
> return 0
> [Ken] got it.
>
>> +}
>> +
>> +static int scsi_post_bind(struct udevice *dev) {
>> +       /* Get uclass plat data from fdt */
>> +       scsi_ofdata_to_uclass_platdata(dev);
>
> Do we need to have this info in bind(), or could it wait until of_to_platdata()?
> [Ken] Stefan shows me a patch https://patchwork.ozlabs.org/patch/743160/ , it fills the two field members(max_lun and max_id) in ahci's scsi_low_level_init()
> As below, I think it's OK to get active link port number in ahci_host_init(), and in my opinion, it's better to be a default way to get max_lun and max_id
> in ahci driver if the two members are not set in fdt since actually max_lun * max_id = ffs(linkmap) and we can also set max_lun = 2 and max_id = fls(linkmap)/2;
> And another question is whther ffs() or fls() for max_id?

OK well since it doesn't take any time, I am OK with it. I'm not sure
about your last question.

>
>> +       if (plat) {
>> +               plat->max_lun = 1;
>> +               plat->max_id = ffs(linkmap);
>> +       }
>
>
> Also, return the error code here.
> [Ken] got it.
>
>> +}
>> +
>>  UCLASS_DRIVER(scsi) = {
>>         .id             = UCLASS_SCSI,
>>         .name           = "scsi",
>> +       .post_bind      = scsi_post_bind,
>>         .post_probe      = scsi_post_probe,
>> +       .per_device_platdata_auto_alloc_size = sizeof(struct
>> + scsi_platdata),
>>  };
>> --
>> 1.9.1

Regards,
Simon