[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 at 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 at mailbox.org>
> ---
> Cc: Abdellatif El Khlifi <abdellatif.elkhlifi at arm.com>
> Cc: Bin Meng <bmeng.cn at gmail.com>
> Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>
> Cc: Mattijs Korpershoek <mkorpershoek at baylibre.com>
> Cc: Michal Suchanek <msuchanek at suse.de>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Tobias Waldekranz <tobias at 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