Re: [RFC 10/22] dm: blk: add read/write interfaces with udevice

[Simon Glass <sjg@chromium.org>]

Hi Takahiro,

On Thu, 30 Sept 2021 at 23:03, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> In include/blk.h, Simon suggested:
> ===>
> /*
>  * These functions should take struct udevice instead of struct blk_desc,
>  * but this is convenient for migration to driver model. Add a 'd' prefix
>  * to the function operations, so that blk_read(), etc. can be reserved for
>  * functions with the correct arguments.
>  */
> unsigned long blk_dread(struct blk_desc *block_dev, lbaint_t start,
>                         lbaint_t blkcnt, void *buffer);
> unsigned long blk_dwrite(struct blk_desc *block_dev, lbaint_t start,
>                          lbaint_t blkcnt, const void *buffer);
> unsigned long blk_derase(struct blk_desc *block_dev, lbaint_t start,
>                          lbaint_t blkcnt);
> <===
>
> So new interfaces are provided with this patch.
>
> They are expected to be used everywhere in U-Boot at the end. The exceptions
> are block device drivers, partition drivers and efi_disk which should know
> details of blk_desc structure.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  drivers/block/blk-uclass.c | 91 ++++++++++++++++++++++++++++++++++++++
>  include/blk.h              |  6 +++
>  2 files changed, 97 insertions(+)
>
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index 6ba11a8fa7f7..8fbec8779e1e 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -482,6 +482,97 @@ unsigned long blk_derase(struct blk_desc *block_dev, lbaint_t start,
>         return ops->erase(dev, start, blkcnt);
>  }
>
> +static struct blk_desc *dev_get_blk(struct udevice *dev)
> +{
> +       struct blk_desc *block_dev;
> +
> +       switch (device_get_uclass_id(dev)) {
> +       case UCLASS_BLK:
> +               block_dev = dev_get_uclass_plat(dev);
> +               break;
> +       case UCLASS_PARTITION:
> +               block_dev = dev_get_uclass_plat(dev_get_parent(dev));
> +               break;
> +       default:
> +               block_dev = NULL;
> +               break;
> +       }
> +
> +       return block_dev;
> +}
> +
> +unsigned long blk_read(struct udevice *dev, lbaint_t start,
> +                      lbaint_t blkcnt, void *buffer)
> +{
> +       struct blk_desc *block_dev;
> +       const struct blk_ops *ops;
> +       struct disk_part *part;
> +       lbaint_t start_in_disk;
> +       ulong blks_read;
> +
> +       block_dev = dev_get_blk(dev);
> +       if (!block_dev)
> +               return -ENOSYS;

IMO blk_read() should take a block device. That is how all other DM
APIs work. I think it is too confusing to use the parent device here.

So how about changing these functions to take a blk device, then
adding new ones for what you want here, e.g.

int media_read(struct udevice *dev...
{
   struct udevice *blk;

       blk = dev_get_blk(dev);
       if (!blk)
               return -ENOSYS;

   ret = blk_read(blk, ...
}

Also can we use blk instead of block_dev?

> +
> +       ops = blk_get_ops(dev);
> +       if (!ops->read)
> +               return -ENOSYS;
> +
> +       start_in_disk = start;
> +       if (device_get_uclass_id(dev) == UCLASS_PARTITION) {
> +               part = dev_get_uclass_plat(dev);
> +               start_in_disk += part->gpt_part_info.start;
> +       }
> +
> +       if (blkcache_read(block_dev->if_type, block_dev->devnum,
> +                         start_in_disk, blkcnt, block_dev->blksz, buffer))
> +               return blkcnt;
> +       blks_read = ops->read(dev, start, blkcnt, buffer);
> +       if (blks_read == blkcnt)
> +               blkcache_fill(block_dev->if_type, block_dev->devnum,
> +                             start_in_disk, blkcnt, block_dev->blksz, buffer);
> +
> +       return blks_read;
> +}
> +
> +unsigned long blk_write(struct udevice *dev, lbaint_t start,
> +                       lbaint_t blkcnt, const void *buffer)
> +{
> +       struct blk_desc *block_dev;
> +       const struct blk_ops *ops;
> +
> +       block_dev = dev_get_blk(dev);
> +       if (!block_dev)
> +               return -ENOSYS;
> +
> +       ops = blk_get_ops(dev);
> +       if (!ops->write)
> +               return -ENOSYS;
> +
> +       blkcache_invalidate(block_dev->if_type, block_dev->devnum);
> +
> +       return ops->write(dev, start, blkcnt, buffer);
> +}
> +
> +unsigned long blk_erase(struct udevice *dev, lbaint_t start,
> +                       lbaint_t blkcnt)
> +{
> +       struct blk_desc *block_dev;
> +       const struct blk_ops *ops;
> +
> +       block_dev = dev_get_blk(dev);
> +       if (!block_dev)
> +               return -ENOSYS;
> +
> +       ops = blk_get_ops(dev);
> +       if (!ops->erase)
> +               return -ENOSYS;
> +
> +       blkcache_invalidate(block_dev->if_type, block_dev->devnum);
> +
> +       return ops->erase(dev, start, blkcnt);
> +}
> +
>  int blk_get_from_parent(struct udevice *parent, struct udevice **devp)
>  {
>         struct udevice *dev;
> diff --git a/include/blk.h b/include/blk.h
> index 3d883eb1db64..f5fdd6633a09 100644
> --- a/include/blk.h
> +++ b/include/blk.h
> @@ -284,6 +284,12 @@ unsigned long blk_dwrite(struct blk_desc *block_dev, lbaint_t start,
>  unsigned long blk_derase(struct blk_desc *block_dev, lbaint_t start,
>                          lbaint_t blkcnt);
>
> +unsigned long blk_read(struct udevice *dev, lbaint_t start,
> +                      lbaint_t blkcnt, void *buffer);
> +unsigned long blk_write(struct udevice *dev, lbaint_t start,
> +                       lbaint_t blkcnt, const void *buffer);
> +unsigned long blk_erase(struct udevice *dev, lbaint_t start,
> +                       lbaint_t blkcnt);

Please add full comments to these.

>  /**
>   * blk_find_device() - Find a block device
>   *
> --
> 2.33.0
>

Regards,
Simon