Re: [PATCH v4 4/8] rockchip: block: blk-uclass: disable bounce buffer support for rkmtd

[Johan Jonker <jbx6244@gmail.com>]

On 10/16/23 23:54, Simon Glass wrote:
> Hi Johan,
> 
> On Sun, 15 Oct 2023 at 16:33, Johan Jonker <jbx6244@gmail.com> wrote:
>>
>> Disable bounce buffer support for rkmtd.
>>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>
>> Changed V3:
>>   New patch
>> ---
>>  drivers/block/blk-uclass.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c
>> index 30ad5bbb0024..ac1b43d757d8 100644
>> --- a/drivers/block/blk-uclass.c
>> +++ b/drivers/block/blk-uclass.c
>> @@ -415,7 +415,7 @@ struct blk_bounce_buffer {
>>
>>  static int blk_buffer_aligned(struct bounce_buffer *state)
>>  {
>> -#if IS_ENABLED(CONFIG_BOUNCE_BUFFER)
>> +#if IS_ENABLED(CONFIG_BOUNCE_BUFFER) && !IS_ENABLED(CONFIG_RKMTD)
> 

> We should not have arch-specific code in a generic file. Can you make
> BOUNCE_BUFFER depend on !RKMTD ?

Hi Simon, Marek,

No that doesn't work that way.

On Rockchip mainline boards MMC_DW is always our boot device BOOT_DEVICE_MMCx.
Therefore BOUNCE_BUFFER is also standard enabled.

The solution in this patch below is wrong as it assumes that if BOUNCE_BUFFER is enabled it must be used all over the place.

blk: Add bounce buffer support to read/write operations
https://source.denx.de/u-boot/u-boot/-/commit/75191f75bce45f3b9aff607c88f17778d3805c61

All implementations of bounce_buffer_start and bounce_buffer_start_extalign are located in the individual drivers  

https://elixir.bootlin.com/u-boot/latest/C/ident/bounce_buffer_start
https://elixir.bootlin.com/u-boot/latest/C/ident/bounce_buffer_start_extalign

The use of a bounce_buffer in block devices should only be allowed if needed.
Lack of "ops->buffer_aligned" does not prevent the creation of a bounce_buffer and is not a good selector yet.

	if (ops->buffer_aligned)
		return ops->buffer_aligned(dev, state);

Current blk_ops has no option to communicate the needed resources.
Please advise how to select the creation of a bounce buffer per device or other solution.

Johan

===
config MMC_DW
	bool "Synopsys DesignWare Memory Card Interface"
	select BOUNCE_BUFFER

Without BOUNCE_BUFFER selected /drivers/mmc/dw_mmc.c does not compile even rk3066 only uses "fifo_mode".

arm-linux-gnueabihf-ld.bfd: drivers/mmc/dw_mmc.o: in function `dwmci_send_cmd':
/drivers/mmc/dw_mmc.c:285: undefined reference to `bounce_buffer_start'


		if (host->fifo_mode) {
			dwmci_writel(host, DWMCI_BLKSIZ, data->blocksize);
			dwmci_writel(host, DWMCI_BYTCNT,
				     data->blocksize * data->blocks);
			dwmci_wait_reset(host, DWMCI_CTRL_FIFO_RESET);
		} else {
			if (data->flags == MMC_DATA_READ) {
				ret = bounce_buffer_start(&bbstate,
						(void*)data->dest,
						data->blocksize *
						data->blocks, GEN_BB_WRITE);
			} else {
				ret = bounce_buffer_start(&bbstate,
						(void*)data->src,
						data->blocksize *
						data->blocks, GEN_BB_READ);
			}
[..]
		}

arm-linux-gnueabihf-ld.bfd: /drivers/mmc/dw_mmc.c:393: undefined reference to `bounce_buffer_stop'


		/* only dma mode need it */
		if (!host->fifo_mode) {
[..]
			bounce_buffer_stop(&bbstate);
		}

> 
>>         struct blk_bounce_buffer *bbstate =
>>                 container_of(state, struct blk_bounce_buffer, state);
>>         struct udevice *dev = bbstate->dev;
>> @@ -441,7 +441,7 @@ long blk_read(struct udevice *dev, lbaint_t start, lbaint_t blkcnt, void *buf)
>>                           start, blkcnt, desc->blksz, buf))
>>                 return blkcnt;
>>
>> -       if (IS_ENABLED(CONFIG_BOUNCE_BUFFER)) {
>> +       if (IS_ENABLED(CONFIG_BOUNCE_BUFFER) && !IS_ENABLED(CONFIG_RKMTD)) {
>>                 struct blk_bounce_buffer bbstate = { .dev = dev };
>>                 int ret;
>>
>> @@ -478,7 +478,7 @@ long blk_write(struct udevice *dev, lbaint_t start, lbaint_t blkcnt,
>>
>>         blkcache_invalidate(desc->uclass_id, desc->devnum);
>>
>> -       if (IS_ENABLED(CONFIG_BOUNCE_BUFFER)) {
>> +       if (IS_ENABLED(CONFIG_BOUNCE_BUFFER) && !IS_ENABLED(CONFIG_RKMTD)) {
>>                 struct blk_bounce_buffer bbstate = { .dev = dev };
>>                 int ret;
>>
>> --
>> 2.39.2
>>
> 
> Regards,
> Simon