Hi Takahiro,
On Thu, 30 Sept 2021 at 23:03, AKASHI Takahiro
<takahiro.aka...@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.aka...@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
