[PATCH 1/2] blk: Add bounce buffer support to read/write operations

[PATCH 1/2] blk: Add bounce buffer support to read/write operations

[Marek Vasut]

Some devices have limited DMA capabilities and require that the
buffers passed to them fit specific properties. Add new optional
callback which can be used at driver level to indicate whether a
buffer alignment is suitable for the device DMA or not, and
trigger use of generic bounce buffer implementation to help use
of unsuitable buffers at the expense of performance degradation.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Cc: Michal Suchanek <msuchanek@suse.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/block/blk-uclass.c | 62 ++++++++++++++++++++++++++++++++++++--
 include/blk.h              | 19 ++++++++++++
 2 files changed, 79 insertions(+), 2 deletions(-)

diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
index 6aac92d9962..885513893f6 100644
--- a/drivers/block/blk-uclass.c
+++ b/drivers/block/blk-uclass.c
@@ -446,6 +446,26 @@ int blk_get_device(int uclass_id, int devnum, struct udevice **devp)
 	return device_probe(*devp);
 }
 
+struct blk_bounce_buffer {
+	struct udevice		*dev;
+	struct bounce_buffer	state;
+};
+
+static int blk_buffer_aligned(struct bounce_buffer *state)
+{
+#if IS_ENABLED(CONFIG_BOUNCE_BUFFER)
+	struct blk_bounce_buffer *bbstate =
+		container_of(state, struct blk_bounce_buffer, state);
+	struct udevice *dev = bbstate->dev;
+	const struct blk_ops *ops = blk_get_ops(dev);
+
+	if (ops->buffer_aligned)
+		return ops->buffer_aligned(dev, state);
+#endif	/* CONFIG_BOUNCE_BUFFER */
+
+	return 1;	/* Default, any buffer is OK */
+}
+
 long blk_read(struct udevice *dev, lbaint_t start, lbaint_t blkcnt, void *buf)
 {
 	struct blk_desc *desc = dev_get_uclass_plat(dev);
@@ -458,7 +478,25 @@ long blk_read(struct udevice *dev, lbaint_t start, lbaint_t blkcnt, void *buf)
 	if (blkcache_read(desc->uclass_id, desc->devnum,
 			  start, blkcnt, desc->blksz, buf))
 		return blkcnt;
-	blks_read = ops->read(dev, start, blkcnt, buf);
+
+	if (IS_ENABLED(CONFIG_BOUNCE_BUFFER)) {
+		struct blk_bounce_buffer bbstate = { .dev = dev };
+		int ret;
+
+		ret = bounce_buffer_start_extalign(&bbstate.state, buf,
+						   blkcnt * desc->blksz,
+						   GEN_BB_WRITE, desc->blksz,
+						   blk_buffer_aligned);
+		if (ret)
+			return ret;
+
+		blks_read = ops->read(dev, start, blkcnt, bbstate.state.bounce_buffer);
+
+		bounce_buffer_stop(&bbstate.state);
+	} else {
+		blks_read = ops->read(dev, start, blkcnt, buf);
+	}
+
 	if (blks_read == blkcnt)
 		blkcache_fill(desc->uclass_id, desc->devnum, start, blkcnt,
 			      desc->blksz, buf);
@@ -471,13 +509,33 @@ long blk_write(struct udevice *dev, lbaint_t start, lbaint_t blkcnt,
 {
 	struct blk_desc *desc = dev_get_uclass_plat(dev);
 	const struct blk_ops *ops = blk_get_ops(dev);
+	long blks_written;
 
 	if (!ops->write)
 		return -ENOSYS;
 
 	blkcache_invalidate(desc->uclass_id, desc->devnum);
 
-	return ops->write(dev, start, blkcnt, buf);
+	if (IS_ENABLED(CONFIG_BOUNCE_BUFFER)) {
+		struct blk_bounce_buffer bbstate = { .dev = dev };
+		int ret;
+
+		ret = bounce_buffer_start_extalign(&bbstate.state, (void *)buf,
+						   blkcnt * desc->blksz,
+						   GEN_BB_READ, desc->blksz,
+						   blk_buffer_aligned);
+		if (ret)
+			return ret;
+
+		blks_written = ops->write(dev, start, blkcnt,
+					  bbstate.state.bounce_buffer);
+
+		bounce_buffer_stop(&bbstate.state);
+	} else {
+		blks_written = ops->write(dev, start, blkcnt, buf);
+	}
+
+	return blks_written;
 }
 
 long blk_erase(struct udevice *dev, lbaint_t start, lbaint_t blkcnt)
diff --git a/include/blk.h b/include/blk.h
index 8986e953e5a..b819f97c2f1 100644
--- a/include/blk.h
+++ b/include/blk.h
@@ -7,6 +7,7 @@
 #ifndef BLK_H
 #define BLK_H
 
+#include <bouncebuf.h>
 #include <dm/uclass-id.h>
 #include <efi.h>
 
@@ -260,6 +261,24 @@ struct blk_ops {
 	 * @return 0 if OK, -ve on error
 	 */
 	int (*select_hwpart)(struct udevice *dev, int hwpart);
+
+#if IS_ENABLED(CONFIG_BOUNCE_BUFFER)
+	/**
+	 * buffer_aligned() - test memory alignment of block operation buffer
+	 *
+	 * Some devices have limited DMA capabilities and require that the
+	 * buffers passed to them fit specific properties. This optional
+	 * callback can be used to indicate whether a buffer alignment is
+	 * suitable for the device DMA or not, and trigger use of generic
+	 * bounce buffer implementation to help use of unsuitable buffers
+	 * at the expense of performance degradation.
+	 *
+	 * @dev:	Block device associated with the request
+	 * @state:	Bounce buffer state
+	 * @return 1 if OK, 0 if unaligned
+	 */
+	int (*buffer_aligned)(struct udevice *dev, struct bounce_buffer *state);
+#endif	/* CONFIG_BOUNCE_BUFFER */
 };
 
 /*
-- 
2.40.1

[PATCH 2/2] scsi: Add buffer_aligned check pass-through

[Marek Vasut]

Some devices have limited DMA capabilities and require that the
buffers passed to them fit specific properties. Add new optional
callback which can be used at driver level to indicate whether a
buffer alignment is suitable for the device DMA or not. This is
a pass-through callback from block uclass to drivers.

Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Cc: Michal Suchanek <msuchanek@suse.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tobias Waldekranz <tobias@waldekranz.com>
---
 drivers/scsi/scsi.c | 15 +++++++++++++++
 include/scsi.h      | 19 +++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 6498f998ad6..7411660d465 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -274,6 +274,18 @@ static ulong scsi_write(struct udevice *dev, lbaint_t blknr, lbaint_t blkcnt,
 	      __func__, start, smallblks, buf_addr);
 	return blkcnt;
 }
+
+#if IS_ENABLED(CONFIG_BOUNCE_BUFFER)
+static int scsi_buffer_aligned(struct udevice *dev, struct bounce_buffer *state)
+{
+	struct scsi_ops *ops = scsi_get_ops(dev->parent);
+
+	if (ops->buffer_aligned)
+		return ops->buffer_aligned(dev->parent, state);
+
+	return 1;
+}
+#endif	/* CONFIG_BOUNCE_BUFFER */
 #endif
 
 #if defined(CONFIG_PCI) && !defined(CONFIG_SCSI_AHCI_PLAT) && \
@@ -720,6 +732,9 @@ int scsi_scan(bool verbose)
 static const struct blk_ops scsi_blk_ops = {
 	.read	= scsi_read,
 	.write	= scsi_write,
+#if IS_ENABLED(CONFIG_BOUNCE_BUFFER)
+	.buffer_aligned	= scsi_buffer_aligned,
+#endif	/* CONFIG_BOUNCE_BUFFER */
 };
 
 U_BOOT_DRIVER(scsi_blk) = {
diff --git a/include/scsi.h b/include/scsi.h
index 9efefea99bb..ee9d622680d 100644
--- a/include/scsi.h
+++ b/include/scsi.h
@@ -7,6 +7,7 @@
  #define _SCSI_H
 
 #include <asm/cache.h>
+#include <bouncebuf.h>
 #include <linux/dma-direction.h>
 
 /* Fix this to the maximum */
@@ -298,6 +299,24 @@ struct scsi_ops {
 	 * @return 0 if OK, -ve on error
 	 */
 	int (*bus_reset)(struct udevice *dev);
+
+#if IS_ENABLED(CONFIG_BOUNCE_BUFFER)
+	/**
+	 * buffer_aligned() - test memory alignment of block operation buffer
+	 *
+	 * Some devices have limited DMA capabilities and require that the
+	 * buffers passed to them fit specific properties. This optional
+	 * callback can be used to indicate whether a buffer alignment is
+	 * suitable for the device DMA or not, and trigger use of generic
+	 * bounce buffer implementation to help use of unsuitable buffers
+	 * at the expense of performance degradation.
+	 *
+	 * @dev:	Block device associated with the request
+	 * @state:	Bounce buffer state
+	 * @return 1 if OK, 0 if unaligned
+	 */
+	int (*buffer_aligned)(struct udevice *dev, struct bounce_buffer *state);
+#endif	/* CONFIG_BOUNCE_BUFFER */
 };
 
 #define scsi_get_ops(dev)        ((struct scsi_ops *)(dev)->driver->ops)
-- 
2.40.1

Re: [PATCH 1/2] blk: Add bounce buffer support to read/write operations

[Simon Glass]

Hi Marek,

On Sun, 13 Aug 2023 at 17:50, Marek Vasut
<marek.vasut+renesas@mailbox.org> wrote:
>
> Some devices have limited DMA capabilities and require that the
> buffers passed to them fit specific properties. Add new optional
> callback which can be used at driver level to indicate whether a
> buffer alignment is suitable for the device DMA or not, and
> trigger use of generic bounce buffer implementation to help use
> of unsuitable buffers at the expense of performance degradation.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Michal Suchanek <msuchanek@suse.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  drivers/block/blk-uclass.c | 62 ++++++++++++++++++++++++++++++++++++--
>  include/blk.h              | 19 ++++++++++++
>  2 files changed, 79 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
> index 6aac92d9962..885513893f6 100644
> --- a/drivers/block/blk-uclass.c
> +++ b/drivers/block/blk-uclass.c
> @@ -446,6 +446,26 @@ int blk_get_device(int uclass_id, int devnum, struct udevice **devp)
>         return device_probe(*devp);
>  }
>
> +struct blk_bounce_buffer {
> +       struct udevice          *dev;
> +       struct bounce_buffer    state;
> +};
> +
> +static int blk_buffer_aligned(struct bounce_buffer *state)
> +{
> +#if IS_ENABLED(CONFIG_BOUNCE_BUFFER)

I think it is worth adding an accessor in the header file to allow you
to convert this #if to if

> +       struct blk_bounce_buffer *bbstate =
> +               container_of(state, struct blk_bounce_buffer, state);
> +       struct udevice *dev = bbstate->dev;
> +       const struct blk_ops *ops = blk_get_ops(dev);
> +
> +       if (ops->buffer_aligned)
> +               return ops->buffer_aligned(dev, state);
> +#endif /* CONFIG_BOUNCE_BUFFER */
> +
> +       return 1;       /* Default, any buffer is OK */
> +}
> +
>  long blk_read(struct udevice *dev, lbaint_t start, lbaint_t blkcnt, void *buf)
>  {
>         struct blk_desc *desc = dev_get_uclass_plat(dev);
> @@ -458,7 +478,25 @@ long blk_read(struct udevice *dev, lbaint_t start, lbaint_t blkcnt, void *buf)
>         if (blkcache_read(desc->uclass_id, desc->devnum,
>                           start, blkcnt, desc->blksz, buf))
>                 return blkcnt;
> -       blks_read = ops->read(dev, start, blkcnt, buf);
> +
> +       if (IS_ENABLED(CONFIG_BOUNCE_BUFFER)) {
> +               struct blk_bounce_buffer bbstate = { .dev = dev };
> +               int ret;
> +
> +               ret = bounce_buffer_start_extalign(&bbstate.state, buf,
> +                                                  blkcnt * desc->blksz,
> +                                                  GEN_BB_WRITE, desc->blksz,
> +                                                  blk_buffer_aligned);
> +               if (ret)
> +                       return ret;
> +
> +               blks_read = ops->read(dev, start, blkcnt, bbstate.state.bounce_buffer);
> +
> +               bounce_buffer_stop(&bbstate.state);
> +       } else {
> +               blks_read = ops->read(dev, start, blkcnt, buf);
> +       }
> +
>         if (blks_read == blkcnt)
>                 blkcache_fill(desc->uclass_id, desc->devnum, start, blkcnt,
>                               desc->blksz, buf);
> @@ -471,13 +509,33 @@ long blk_write(struct udevice *dev, lbaint_t start, lbaint_t blkcnt,
>  {
>         struct blk_desc *desc = dev_get_uclass_plat(dev);
>         const struct blk_ops *ops = blk_get_ops(dev);
> +       long blks_written;
>
>         if (!ops->write)
>                 return -ENOSYS;
>
>         blkcache_invalidate(desc->uclass_id, desc->devnum);
>
> -       return ops->write(dev, start, blkcnt, buf);
> +       if (IS_ENABLED(CONFIG_BOUNCE_BUFFER)) {
> +               struct blk_bounce_buffer bbstate = { .dev = dev };
> +               int ret;
> +
> +               ret = bounce_buffer_start_extalign(&bbstate.state, (void *)buf,
> +                                                  blkcnt * desc->blksz,
> +                                                  GEN_BB_READ, desc->blksz,
> +                                                  blk_buffer_aligned);
> +               if (ret)
> +                       return ret;
> +
> +               blks_written = ops->write(dev, start, blkcnt,
> +                                         bbstate.state.bounce_buffer);
> +
> +               bounce_buffer_stop(&bbstate.state);
> +       } else {
> +               blks_written = ops->write(dev, start, blkcnt, buf);
> +       }
> +
> +       return blks_written;
>  }
>
>  long blk_erase(struct udevice *dev, lbaint_t start, lbaint_t blkcnt)
> diff --git a/include/blk.h b/include/blk.h
> index 8986e953e5a..b819f97c2f1 100644
> --- a/include/blk.h
> +++ b/include/blk.h
> @@ -7,6 +7,7 @@
>  #ifndef BLK_H
>  #define BLK_H
>
> +#include <bouncebuf.h>
>  #include <dm/uclass-id.h>
>  #include <efi.h>
>
> @@ -260,6 +261,24 @@ struct blk_ops {
>          * @return 0 if OK, -ve on error
>          */
>         int (*select_hwpart)(struct udevice *dev, int hwpart);
> +
> +#if IS_ENABLED(CONFIG_BOUNCE_BUFFER)
> +       /**
> +        * buffer_aligned() - test memory alignment of block operation buffer
> +        *
> +        * Some devices have limited DMA capabilities and require that the
> +        * buffers passed to them fit specific properties. This optional
> +        * callback can be used to indicate whether a buffer alignment is
> +        * suitable for the device DMA or not, and trigger use of generic
> +        * bounce buffer implementation to help use of unsuitable buffers
> +        * at the expense of performance degradation.
> +        *
> +        * @dev:        Block device associated with the request
> +        * @state:      Bounce buffer state
> +        * @return 1 if OK, 0 if unaligned

We typically return 0 for OK, so that we can use -ve values for
errors. Perhaps flip this to check_aligned() that returns either 0 or
-EINVAL ? I'm just worried that it will be confusing since it is
different from nearly every other DM API.

> +        */
> +       int (*buffer_aligned)(struct udevice *dev, struct bounce_buffer *state);
> +#endif /* CONFIG_BOUNCE_BUFFER */
>  };
>
>  /*
> --
> 2.40.1
>

Regards,
Simon

Re: [PATCH 1/2] blk: Add bounce buffer support to read/write operations

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 580 bytes --]

On Mon, Aug 14, 2023 at 01:49:59AM +0200, Marek Vasut wrote:

> Some devices have limited DMA capabilities and require that the
> buffers passed to them fit specific properties. Add new optional
> callback which can be used at driver level to indicate whether a
> buffer alignment is suitable for the device DMA or not, and
> trigger use of generic bounce buffer implementation to help use
> of unsuitable buffers at the expense of performance degradation.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/2] scsi: Add buffer_aligned check pass-through

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

On Mon, Aug 14, 2023 at 01:50:00AM +0200, Marek Vasut wrote:

> Some devices have limited DMA capabilities and require that the
> buffers passed to them fit specific properties. Add new optional
> callback which can be used at driver level to indicate whether a
> buffer alignment is suitable for the device DMA or not. This is
> a pass-through callback from block uclass to drivers.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]