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

[Ken Ma <make@marvell.com>]

Hi Simon

Please see my inline reply, thanks a lot!

-----Original Message-----
From: sjg@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@marvell.com> wrote:
> From: Ken Ma <make@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@marvell.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Reviewed-on: http://vgitil04.il.marvell.com:8080/35304
> Tested-by: iSoC Platform CI <ykjenk@marvell.com>
> Reviewed-by: Omri Itach <omrii@marvell.com>
> Reviewed-by: Kostya Porotchkin <kostap@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?

> +       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