Re: [PATCH 14/45] dm: blk: Add udevice functions

From: AKASHI Takahiro <takahiro.akashi@linaro.org>
To: Simon Glass <sjg@chromium.org>
Cc: U-Boot Mailing List <u-boot@lists.denx.de>,
	Tom Rini <trini@konsulko.com>,
	Heinrich Schuchardt <xypron.glpk@gmx.de>,
	Marek Vasut <marex@denx.de>,
	Masahisa Kojima <masahisa.kojima@linaro.org>,
	Pavel Herrmann <morpheus.ibis@gmail.com>
Subject: Re: [PATCH 14/45] dm: blk: Add udevice functions
Date: Mon, 26 Sep 2022 09:17:28 +0900	[thread overview]
Message-ID: <20220926001728.GB7925@laputa> (raw)
In-Reply-To: <20220925150248.2524421-15-sjg@chromium.org>

Hi Simon,

On Sun, Sep 25, 2022 at 09:02:17AM -0600, Simon Glass wrote:
> At present we have functions called blk_dread(), etc., which take a
> struct blk_desc * to refer to the block device. Add some functions which
> use udevice instead, since this is more in keeping with how driver model
> is supposed to work.

Unfortunately, NAK.
I have already added similar functions in disk/disk-uclass.c
with my commit 59da9d4782cd ("dm: disk: add read/write interfaces with
udevice"). dev_read()/dev_write() works well with UCLASS_BLK (as intended).

I remember that you also ack'ed that patch.

-Takahiro Akashi

> Update one of the tests to use this.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
>  drivers/block/blk-uclass.c | 51 +++++++++++++++++++++++++-------------
>  include/blk.h              | 39 ++++++++++++++++++++++++++++-
>  test/dm/usb.c              | 11 +++++---
>  3 files changed, 80 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index f8157c8d6ae..7070fd15c89 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -444,53 +444,70 @@ int blk_get_device(int uclass_id, int devnum, struct udevice **devp)
>  	return device_probe(*devp);
>  }
>  
> -unsigned long blk_dread(struct blk_desc *block_dev, lbaint_t start,
> -			lbaint_t blkcnt, void *buffer)
> +long blk_read(struct udevice *dev, lbaint_t start, lbaint_t blkcnt, void *buf)
>  {
> -	struct udevice *dev = block_dev->bdev;
> +	struct blk_desc *desc = dev_get_uclass_plat(dev);
>  	const struct blk_ops *ops = blk_get_ops(dev);
>  	ulong blks_read;
>  
>  	if (!ops->read)
>  		return -ENOSYS;
>  
> -	if (blkcache_read(block_dev->uclass_id, block_dev->devnum,
> -			  start, blkcnt, block_dev->blksz, buffer))
> +	if (blkcache_read(desc->uclass_id, desc->devnum,
> +			  start, blkcnt, desc->blksz, buf))
>  		return blkcnt;
> -	blks_read = ops->read(dev, start, blkcnt, buffer);
> +	blks_read = ops->read(dev, start, blkcnt, buf);
>  	if (blks_read == blkcnt)
> -		blkcache_fill(block_dev->uclass_id, block_dev->devnum,
> -			      start, blkcnt, block_dev->blksz, buffer);
> +		blkcache_fill(desc->uclass_id, desc->devnum, start, blkcnt,
> +			      desc->blksz, buf);
>  
>  	return blks_read;
>  }
>  
> -unsigned long blk_dwrite(struct blk_desc *block_dev, lbaint_t start,
> -			 lbaint_t blkcnt, const void *buffer)
> +long blk_write(struct udevice *dev, lbaint_t start, lbaint_t blkcnt,
> +	       const void *buf)
>  {
> -	struct udevice *dev = block_dev->bdev;
> +	struct blk_desc *desc = dev_get_uclass_plat(dev);
>  	const struct blk_ops *ops = blk_get_ops(dev);
>  
>  	if (!ops->write)
>  		return -ENOSYS;
>  
> -	blkcache_invalidate(block_dev->uclass_id, block_dev->devnum);
> -	return ops->write(dev, start, blkcnt, buffer);
> +	blkcache_invalidate(desc->uclass_id, desc->devnum);
> +
> +	return ops->write(dev, start, blkcnt, buf);
>  }
>  
> -unsigned long blk_derase(struct blk_desc *block_dev, lbaint_t start,
> -			 lbaint_t blkcnt)
> +long blk_erase(struct udevice *dev, lbaint_t start, lbaint_t blkcnt)
>  {
> -	struct udevice *dev = block_dev->bdev;
> +	struct blk_desc *desc = dev_get_uclass_plat(dev);
>  	const struct blk_ops *ops = blk_get_ops(dev);
>  
>  	if (!ops->erase)
>  		return -ENOSYS;
>  
> -	blkcache_invalidate(block_dev->uclass_id, block_dev->devnum);
> +	blkcache_invalidate(desc->uclass_id, desc->devnum);
> +
>  	return ops->erase(dev, start, blkcnt);
>  }
>  
> +ulong blk_dread(struct blk_desc *desc, lbaint_t start, lbaint_t blkcnt,
> +		void *buffer)
> +{
> +	return blk_read(desc->bdev, start, blkcnt, buffer);
> +}
> +
> +ulong blk_dwrite(struct blk_desc *desc, lbaint_t start, lbaint_t blkcnt,
> +		 const void *buffer)
> +{
> +	return blk_write(desc->bdev, start, blkcnt, buffer);
> +}
> +
> +ulong blk_derase(struct blk_desc *desc, lbaint_t start, lbaint_t blkcnt)
> +{
> +	return blk_erase(desc->bdev, 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 cdc6f0fc7eb..8614d1420ac 100644
> --- a/include/blk.h
> +++ b/include/blk.h
> @@ -273,6 +273,43 @@ 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);
>  
> +/**
> + * blk_read() - Read from a block device
> + *
> + * @dev: Device to read from
> + * @start: Start block for the read
> + * @blkcnt: Number of blocks to read
> + * @buf: Place to put the data
> + * @return number of blocks read (which may be less than @blkcnt),
> + * or -ve on error. This never returns 0 unless @blkcnt is 0
> + */
> +long blk_read(struct udevice *dev, lbaint_t start, lbaint_t blkcnt,
> +	      void *buffer);
> +
> +/**
> + * blk_write() - Write to a block device
> + *
> + * @dev: Device to write to
> + * @start: Start block for the write
> + * @blkcnt: Number of blocks to write
> + * @buf: Data to write
> + * @return number of blocks written (which may be less than @blkcnt),
> + * or -ve on error. This never returns 0 unless @blkcnt is 0
> + */
> +long blk_write(struct udevice *dev, lbaint_t start, lbaint_t blkcnt,
> +	       const void *buffer);
> +
> +/**
> + * blk_erase() - Erase part of a block device
> + *
> + * @dev: Device to erase
> + * @start: Start block for the erase
> + * @blkcnt: Number of blocks to erase
> + * @return number of blocks erased (which may be less than @blkcnt),
> + * or -ve on error. This never returns 0 unless @blkcnt is 0
> + */
> +long blk_erase(struct udevice *dev, lbaint_t start, lbaint_t blkcnt);
> +
>  /**
>   * blk_find_device() - Find a block device
>   *
> @@ -428,7 +465,7 @@ const char *blk_get_devtype(struct udevice *dev);
>  
>  /**
>   * blk_get_by_device() - Get the block device descriptor for the given device
> - * @dev:	Instance of a storage device
> + * @dev:	Instance of a storage device (the parent of the block device)
>   *
>   * Return: With block device descriptor on success , NULL if there is no such
>   *	   block device.
> diff --git a/test/dm/usb.c b/test/dm/usb.c
> index 5d6ceefce0b..445b21a560b 100644
> --- a/test/dm/usb.c
> +++ b/test/dm/usb.c
> @@ -43,19 +43,24 @@ DM_TEST(dm_test_usb_base, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
>   */
>  static int dm_test_usb_flash(struct unit_test_state *uts)
>  {
> -	struct udevice *dev;
> -	struct blk_desc *dev_desc;
> +	struct blk_desc *dev_desc, *chk;
> +	struct udevice *dev, *blk;
>  	char cmp[1024];
>  
>  	state_set_skip_delays(true);
>  	ut_assertok(usb_init());
>  	ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 0, &dev));
>  	ut_assertok(blk_get_device_by_str("usb", "0", &dev_desc));
> +	chk = blk_get_by_device(dev);
> +	ut_asserteq_ptr(chk, dev_desc);
> +
> +	ut_assertok(device_find_first_child_by_uclass(dev, UCLASS_BLK, &blk));
> +	ut_asserteq_ptr(chk, blk_get_by_device(dev));
>  
>  	/* Read a few blocks and look for the string we expect */
>  	ut_asserteq(512, dev_desc->blksz);
>  	memset(cmp, '\0', sizeof(cmp));
> -	ut_asserteq(2, blk_dread(dev_desc, 0, 2, cmp));
> +	ut_asserteq(2, blk_read(blk, 0, 2, cmp));
>  	ut_assertok(strcmp(cmp, "this is a test"));
>  	ut_assertok(usb_stop());
>  
> -- 
> 2.37.3.998.g577e59143f-goog
> 