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