Questionable design of drivers/io/io_block.c

[masahir0y]

Haojian,

I need to load an FIP image from my eMMC device, so I took a look at drivers/io/io_block.c written by you.

I think this design is weird because it usually expects much bigger buffer than a block size.

I found Hikey uses 512MB buffer for io_block.c for eMMC.

static const io_block_dev_spec_t emmc_dev_spec = { / It's used as temp buffer in block driver. /

if IMAGE_BL1

    .buffer = {
            .offset = HIKEY_BL1_MMC_DATA_BASE,
            .length = HIKEY_BL1_MMC_DATA_SIZE,
    },

else

    .buffer = {
            .offset = HIKEY_MMC_DATA_BASE,
            .length = HIKEY_MMC_DATA_SIZE,
    },

endif

    .ops = {
            .read = emmc_read_blocks,
            .write = emmc_write_blocks,
    },
    .block_size = EMMC_BLOCK_SIZE,

};

define HIKEY_MMC_DATA_BASE (DDR_BASE + 0x10000000)

define HIKEY_MMC_DATA_SIZE 0x20000000

I am assuming your intention is like this:

[1] load an image onto a buffer that is large enough to accommodate the whole of the image. [2] Do memcpy() from the buffer to your final destination. (this is done by CPU, so slow)

This is not the block-layer abstraction, but just relocating the image.

If you really want to do image relocation, you can achieve it as follows:

[1] transfer the FIP to a large enough buffer with your SoC-specific way [2] Use drivers/io/io_memmap.c as the backend of the io_fip

I understand your motivation for the relocation; each component in the FIP image is not aligned.

Please take a look my pull-request #799. I guess it is what you want.

My patch allows each component (BL2, BL31, ...) in the FIP aligned by a given byte.

If each offset to BL* is aligned by the block size, you do not need relocation. Just make the DMA engine transfer the image to its load address directly.

I think drivers/io/io_block.c should be abstraction for block devices and it should be implemented like this:

Have a buffer as large as the block-size. (512 byte for eMMC) If the destination is not aligned or transfer size is smaller than the block-size, use the bounce buffer (partial read), otherwise, transfer the data to the destination directly.

[danh-arm]

See https://github.com/ARM-software/arm-trusted-firmware/pull/799

[danh-arm]

@hzhuang1 - have you seen this?

[hzhuang1]

The main issue is BL image just after FIP header. Did your alignment patch fix this issue? Could you help to rebase the PR#799 first? It can't be merged now. Then I could check the io block driver again.

I use the large block buffer since I need to download images into buffer by fastboot protocol. It a bit special case. But it's too small to use only 512 byte buffer since only single block operation could be supported. It only impacts the performance of eMMC.

[masahir0y]

Hi @hzhuang1 .

The main issue is BL image just after FIP header. Did your alignment patch fix this issue?

Yes. This is the root cause of our problems, and my patch aims to fix it. It has been merged into the integration branch. Please check commit 1c75d5dfb03181bc481fe021dc5a8ab29c0f7557.

But it's too small to use only 512 byte buffer since only single block operation could be supported. It only impacts the performance of eMMC.

Right. If you use the new option --align for the fiptool, BL images are nicely aligned, so the eMMC controller will be able to directly transfer images to their final destination.