Yidi Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
soc/mediatek/common: A common API for loading the blob file
load_blob_file is added for loading the blog file to the specified memory address. This function also measures the load time and the blob size.
Signed-off-by: Yidi Lin yidi.lin@mediatek.com Change-Id: Ie94001bbda25fe015f43172e92a1006e059de223 --- A src/soc/mediatek/common/include/soc/mtlib_common.h A src/soc/mediatek/common/mtlib.c M src/soc/mediatek/mt8192/Makefile.inc 3 files changed, 38 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/46930/1
diff --git a/src/soc/mediatek/common/include/soc/mtlib_common.h b/src/soc/mediatek/common/include/soc/mtlib_common.h new file mode 100644 index 0000000..3210468 --- /dev/null +++ b/src/soc/mediatek/common/include/soc/mtlib_common.h @@ -0,0 +1,8 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef SOC_MEDIATEK_MTLIB_COMMON_H +#define SOC_MEDIATEK_MTLIB_COMMON_H + +int load_blob_file(const char *name, unsigned long addr); + +#endif /* SOC_MEDIATEK_MTLIB_COMMON_H */ diff --git a/src/soc/mediatek/common/mtlib.c b/src/soc/mediatek/common/mtlib.c new file mode 100644 index 0000000..cdb748b --- /dev/null +++ b/src/soc/mediatek/common/mtlib.c @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <cbfs.h> +#include <console/console.h> +#include <soc/mtlib_common.h> +#include <timer.h> + +#define BUF_SIZE (128 * KiB) +static u8 blob_data[BUF_SIZE] __aligned(8); + +int load_blob_file(const char *name, unsigned long addr) +{ + struct stopwatch sw; + size_t blob_bytes; + + stopwatch_init(&sw); + + blob_bytes = cbfs_boot_load_file(name, blob_data, sizeof(blob_data), CBFS_TYPE_RAW); + if (blob_bytes == 0) { + printk(BIOS_ERR, "binary %s not found\n", name); + return CB_ERR; + } + + memcpy((void *)addr, blob_data, blob_bytes); + printk(BIOS_DEBUG, "%s: Load %s in %ld msecs, size %ld bytes\n", + __func__, name, stopwatch_duration_msecs(&sw), blob_bytes); + + return CB_SUCCESS; +} diff --git a/src/soc/mediatek/mt8192/Makefile.inc b/src/soc/mediatek/mt8192/Makefile.inc index 6890910..bebb0c4 100755 --- a/src/soc/mediatek/mt8192/Makefile.inc +++ b/src/soc/mediatek/mt8192/Makefile.inc @@ -52,6 +52,7 @@ ramstage-y += devapc.c ramstage-y += ufs.c ramstage-y += mcupm.c +ramstage-y += ../common/mtlib.c ramstage-y += ../common/timer.c ramstage-y += ../common/uart.c ramstage-y += ../common/usb.c usb.c
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46930/1/src/soc/mediatek/common/inc... File src/soc/mediatek/common/include/soc/mtlib_common.h:
PS1: Could we use this API for spm and sspm for mt8183 in *this* patch?
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46930/1/src/soc/mediatek/common/inc... File src/soc/mediatek/common/include/soc/mtlib_common.h:
PS1:
Could we use this API for spm and sspm for mt8183 in *this* patch?
Only sspm is applicable. spm requires additional format handling. So, is it suggested to move this patch to mt8192 folder ?
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46930/1/src/soc/mediatek/common/inc... File src/soc/mediatek/common/include/soc/mtlib_common.h:
PS1:
Only sspm is applicable. spm requires additional format handling. […]
Then let's use this API for mt8183 sspm.
Hello Hung-Te Lin, Xi Chen, build bot (Jenkins), Roger Lu, Patrick Georgi, Martin Roth, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46930
to look at the new patch set (#7).
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
soc/mediatek/common: A common API for loading the blob file
load_blob_file is added for loading the blog file to the specified memory address. This function also measures the load time and the blob size.
Signed-off-by: Yidi Lin yidi.lin@mediatek.com Change-Id: Ie94001bbda25fe015f43172e92a1006e059de223 --- A src/soc/mediatek/common/include/soc/mtlib_common.h A src/soc/mediatek/common/mtlib.c M src/soc/mediatek/mt8192/Makefile.inc M src/soc/mediatek/mt8192/include/soc/memlayout.ld 4 files changed, 46 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/46930/7
Hello Hung-Te Lin, Xi Chen, build bot (Jenkins), Roger Lu, Patrick Georgi, Martin Roth, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46930
to look at the new patch set (#11).
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
soc/mediatek/common: A common API for loading the blob file
load_blob_file is added for loading the blog file to the specified memory address. This function also measures the load time and the blob size.
Signed-off-by: Yidi Lin yidi.lin@mediatek.com Change-Id: Ie94001bbda25fe015f43172e92a1006e059de223 --- A src/soc/mediatek/common/include/soc/mtlib_common.h A src/soc/mediatek/common/mtlib.c M src/soc/mediatek/mt8192/Makefile.inc M src/soc/mediatek/mt8192/include/soc/memlayout.ld 4 files changed, 46 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/46930/11
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
Patch Set 14:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 21: binary "%s: Failed to load %s.\n", __func__, name
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 25: memcpy((void *)addr, blob_data, blob_bytes); One question. If we will do mb(), why can't we just use addr?
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 27: /* Memory barrier to ensure that all firmware code is loaded : * before we release the reset pin. : */ given this is a generic function now maybe:
Memory barrier to ensure data is flushed so we can initiate DMA on return.
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 25: memcpy((void *)addr, blob_data, blob_bytes);
One question. […]
Hi Hung-te, do you mean feed the addr to cbfs_boot_load_file() ?
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 27: /* Memory barrier to ensure that all firmware code is loaded : * before we release the reset pin. : */
given this is a generic function now maybe: […]
Let me clarify how those blobs are loaded. 1. For 2 dpm binaries and SSPM binary, AP copies the binary to the target SRAM(addr). 2. For SPM binary, AP can't touch the SPM's SRAM. We can only use SPM's DMA hardware to move the binary from DRAM(non-cacheable buffer) to its SRAM.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
Patch Set 14:
(8 comments)
Very nice.
https://review.coreboot.org/c/coreboot/+/46930/14//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46930/14//COMMIT_MSG@7 PS14, Line 7: A common API for loading the blob file Please use statements by adding a verb (in imperative mood):
Add common API to load blob file
https://review.coreboot.org/c/coreboot/+/46930/14//COMMIT_MSG@9 PS14, Line 9: blog blob
https://review.coreboot.org/c/coreboot/+/46930/14//COMMIT_MSG@11 PS14, Line 11: load time and the blob size. Please also mention, why the ramstage size is increased.
https://review.coreboot.org/c/coreboot/+/46930/14//COMMIT_MSG@12 PS14, Line 12: Maybe paste the log output with one example from the new debug messages.
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/in... File src/soc/mediatek/common/include/soc/mtlib_common.h:
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/in... PS14, Line 6: unsigned long Should `addr` be `uintptr_t` or something similar? Or, as it’s passed to `memcpy()`, why not `void *`?
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 9: #define BUF_SIZE (192 * KiB) Is that the size of the biggest blob?
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 27: /* Memory barrier to ensure that all firmware code is loaded : * before we release the reset pin. : */ Please use format the comment according to the [coding style][1].
[1]: https://doc.coreboot.org/coding_style.html#commenting
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 32: %ld The length modifier for `size_t` is `z`.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 25: memcpy((void *)addr, blob_data, blob_bytes);
Hi Hung-te, do you mean feed the addr to cbfs_boot_load_file() ?
Yes
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 27: /* Memory barrier to ensure that all firmware code is loaded : * before we release the reset pin. : */
Let me clarify how those blobs are loaded. […]
You can change the description, but I think the point is, from this function <load_blob_file> people can't understand why there's some code related to 'all firmware code', also 'release reset pin'. For example, maybe you can change the function to
load_and_run_mcu(const char *fw_name, void *addr, size_t size, int (*reset_mcu)(void *));
So it's very clear we'll cbfs load, mb, and then invoke reset_mcu callback. The SPMDMA thing can be part of reset_mcu. Or if you think there'll be more components need extra DMA flow, then we can also refactor that.
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 25: memcpy((void *)addr, blob_data, blob_bytes);
Yes
CBFS: Found 'dpm.dm' @0x1d980 size 0x1a load_blob_file: Load dpm.dm in 13 msecs, size 40 bytes CBFS: Found 'dpm.pm' @0x1da00 size 0x1f8d read SPI 0x41fa38 0x1f8d: 892 us, 9054 KB/s, 72.432 Mbps lzma: Decoding error = 1
I encounter this error when feeding SRAM address to cbfs_boot_load_file with larger size. I will reuse dram_dma region as CBFS buffer for now (assume that all blobs are loaded after DRAM is ready.)
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
Patch Set 15:
I create 3 WIP patches for API refactoring. Please review CB:47895 CB:47896 CB:47897.
I will squash those changes to correlated patches once review is done.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
Patch Set 16:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46930/16/src/soc/mediatek/common/in... File src/soc/mediatek/common/include/soc/mtlib_common.h:
https://review.coreboot.org/c/coreboot/+/46930/16/src/soc/mediatek/common/in... PS16, Line 6: int load_blob_file(const char *name, unsigned long addr); Please use this API for mt8183 sspm in this patch.
https://review.coreboot.org/c/coreboot/+/46930/1/src/soc/mediatek/common/inc... File src/soc/mediatek/common/include/soc/mtlib_common.h:
PS1:
Then let's use this API for mt8183 sspm.
Left another comment in PS16.
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: A common API for loading the blob file ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46930/16/src/soc/mediatek/common/in... File src/soc/mediatek/common/include/soc/mtlib_common.h:
https://review.coreboot.org/c/coreboot/+/46930/16/src/soc/mediatek/common/in... PS16, Line 6: int load_blob_file(const char *name, unsigned long addr);
Please use this API for mt8183 sspm in this patch.
Hi Yu-Ping,
Could I squash CB:47895 to this patch and use mtlib_load_and_run() for mt8183 ?
Hello Hung-Te Lin, Xi Chen, build bot (Jenkins), Roger Lu, Patrick Georgi, Martin Roth, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46930
to look at the new patch set (#17).
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
soc/mediatek/common: Add common API for loading firmwares
Add mtlib_load_and_run to load the firmware to the specified memory address and run the firmware. This function also measures the load time and the blob size. For example:
mtlib_load_and_run: Load dpm.pm in 13 msecs (14004 bytes)
Signed-off-by: Yidi Lin yidi.lin@mediatek.com Change-Id: Ie94001bbda25fe015f43172e92a1006e059de223 --- A src/soc/mediatek/common/include/soc/mtlib_common.h A src/soc/mediatek/common/mtlib.c M src/soc/mediatek/mt8192/Makefile.inc 3 files changed, 46 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/46930/17
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 17:
(11 comments)
https://review.coreboot.org/c/coreboot/+/46930/14//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46930/14//COMMIT_MSG@7 PS14, Line 7: A common API for loading the blob file
Please use statements by adding a verb (in imperative mood): […]
Done
https://review.coreboot.org/c/coreboot/+/46930/14//COMMIT_MSG@9 PS14, Line 9: blog
blob
Done
https://review.coreboot.org/c/coreboot/+/46930/14//COMMIT_MSG@11 PS14, Line 11: load time and the blob size.
Please also mention, why the ramstage size is increased.
The change is no longer needed.
https://review.coreboot.org/c/coreboot/+/46930/14//COMMIT_MSG@12 PS14, Line 12:
Maybe paste the log output with one example from the new debug messages.
Done
https://review.coreboot.org/c/coreboot/+/46930/16/src/soc/mediatek/common/in... File src/soc/mediatek/common/include/soc/mtlib_common.h:
https://review.coreboot.org/c/coreboot/+/46930/16/src/soc/mediatek/common/in... PS16, Line 6: int load_blob_file(const char *name, unsigned long addr);
Hi Yu-Ping, […]
please review CB:48232 and CB:48233
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/in... File src/soc/mediatek/common/include/soc/mtlib_common.h:
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/in... PS14, Line 6: unsigned long
Should `addr` be `uintptr_t` or something similar? Or, as it’s passed to `memcpy()`, why not `void * […]
Done
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 9: #define BUF_SIZE (192 * KiB)
Is that the size of the biggest blob?
Done
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 21: binary
"%s: Failed to load %s. […]
Done
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 25: memcpy((void *)addr, blob_data, blob_bytes);
CBFS: Found 'dpm.dm' @0x1d980 size 0x1a […]
use _dram_dma as CBFS buffer and copy to target SRAM address.
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 27: /* Memory barrier to ensure that all firmware code is loaded : * before we release the reset pin. : */
You can change the description, but I think the point is, from this function <load_blob_file> people […]
Done
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 32: %ld
The length modifier for `size_t` is `z`.
Done
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 25: memcpy((void *)addr, blob_data, blob_bytes);
use _dram_dma as CBFS buffer and copy to target SRAM address.
Done
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/14/src/soc/mediatek/common/mt... PS14, Line 27: /* Memory barrier to ensure that all firmware code is loaded : * before we release the reset pin. : */
Please use format the comment according to the [coding style][1]. […]
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 17:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46930/17/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/17/src/soc/mediatek/common/mt... PS17, Line 27: we release the reset pin resetting MCU
https://review.coreboot.org/c/coreboot/+/46930/17/src/soc/mediatek/common/mt... PS17, Line 30: if (reset_mcu != NULL) if (reset_mcu)
https://review.coreboot.org/c/coreboot/+/46930/17/src/soc/mediatek/common/mt... PS17, Line 31: reset_mcu should we change reset_mcu to return int for error code?
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46930/17/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/17/src/soc/mediatek/common/mt... PS17, Line 31: reset_mcu
should we change reset_mcu to return int for error code?
In most cases, reset function only writes 1 to reset bit and assumes firmware running successfully.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46930/16/src/soc/mediatek/common/in... File src/soc/mediatek/common/include/soc/mtlib_common.h:
https://review.coreboot.org/c/coreboot/+/46930/16/src/soc/mediatek/common/in... PS16, Line 6: int load_blob_file(const char *name, unsigned long addr);
please review CB:48232 and CB:48233
Ack
https://review.coreboot.org/c/coreboot/+/46930/17/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/17/src/soc/mediatek/common/mt... PS17, Line 31: reset_mcu
In most cases, reset function only writes 1 to reset bit and assumes firmware running successfully.
Okay, let's keep it as is for now.
Hello Hung-Te Lin, Xi Chen, build bot (Jenkins), Roger Lu, Patrick Georgi, Martin Roth, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46930
to look at the new patch set (#18).
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
soc/mediatek/common: Add common API for loading firmwares
Add mtlib_load_and_run to load the firmware to the specified memory address and run the firmware. This function also measures the load time and the blob size. For example:
mtlib_load_and_run: Load dpm.pm in 13 msecs (14004 bytes)
Signed-off-by: Yidi Lin yidi.lin@mediatek.com Change-Id: Ie94001bbda25fe015f43172e92a1006e059de223 --- A src/soc/mediatek/common/include/soc/mtlib_common.h A src/soc/mediatek/common/mtlib.c M src/soc/mediatek/mt8192/Makefile.inc 3 files changed, 46 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/46930/18
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 18:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46930/17/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/17/src/soc/mediatek/common/mt... PS17, Line 27: we release the reset pin
resetting MCU
Done
https://review.coreboot.org/c/coreboot/+/46930/17/src/soc/mediatek/common/mt... PS17, Line 30: if (reset_mcu != NULL)
if (reset_mcu)
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46930/18/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/18/src/soc/mediatek/common/mt... PS18, Line 18: cbfs_boot_load_file I think this API has been already renamed. Can you rebase and build again?
Hello Hung-Te Lin, Xi Chen, build bot (Jenkins), Roger Lu, Patrick Georgi, Martin Roth, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46930
to look at the new patch set (#19).
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
soc/mediatek/common: Add common API for loading firmwares
Add mtlib_init_mcu to load the firmware to the specified memory address and run the firmware. This function also measures the load time and the blob size. For example:
mtlib_init_mcu: Load dpm.pm in 13 msecs (14004 bytes)
Signed-off-by: Yidi Lin yidi.lin@mediatek.com Change-Id: Ie94001bbda25fe015f43172e92a1006e059de223 --- A src/soc/mediatek/common/include/soc/mtlib_common.h A src/soc/mediatek/common/mtlib.c M src/soc/mediatek/mt8192/Makefile.inc 3 files changed, 57 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/46930/19
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 19:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46930/18/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/18/src/soc/mediatek/common/mt... PS18, Line 18: cbfs_boot_load_file
I think this API has been already renamed. […]
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 19: Code-Review+2
(1 comment)
just one nit
https://review.coreboot.org/c/coreboot/+/46930/19/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/19/src/soc/mediatek/common/mt... PS19, Line 34: Load Loaded (and reset)
Hello Hung-Te Lin, Xi Chen, build bot (Jenkins), Roger Lu, Patrick Georgi, Martin Roth, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46930
to look at the new patch set (#20).
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
soc/mediatek/common: Add common API for loading firmwares
Add mtlib_init_mcu to load the firmware to the specified memory address and run the firmware. This function also measures the load time and the blob size. For example:
mtlib_init_mcu: Load dpm.pm in 13 msecs (14004 bytes)
Signed-off-by: Yidi Lin yidi.lin@mediatek.com Change-Id: Ie94001bbda25fe015f43172e92a1006e059de223 --- A src/soc/mediatek/common/include/soc/mtlib_common.h A src/soc/mediatek/common/mtlib.c M src/soc/mediatek/mt8192/Makefile.inc 3 files changed, 57 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/46930/20
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 20:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46930/19/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/19/src/soc/mediatek/common/mt... PS19, Line 34: Load
Loaded (and reset)
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 20: Code-Review+2
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 20:
(6 comments)
https://review.coreboot.org/c/coreboot/+/46930/20//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46930/20//COMMIT_MSG@10 PS20, Line 10: memory Can be moved to the previous line.
https://review.coreboot.org/c/coreboot/+/46930/20//COMMIT_MSG@13 PS20, Line 13: mtlib_init_mcu: Load dpm.pm in 13 msecs (14004 bytes) Please update the format.
https://review.coreboot.org/c/coreboot/+/46930/20/src/soc/mediatek/common/in... File src/soc/mediatek/common/include/soc/mtlib_common.h:
https://review.coreboot.org/c/coreboot/+/46930/20/src/soc/mediatek/common/in... PS20, Line 8: * One space before "*/"
https://review.coreboot.org/c/coreboot/+/46930/20/src/soc/mediatek/common/in... PS20, Line 8: Only one space. Same below.
https://review.coreboot.org/c/coreboot/+/46930/20/src/soc/mediatek/common/in... PS20, Line 9: * One space before "*/"
https://review.coreboot.org/c/coreboot/+/46930/20/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/20/src/soc/mediatek/common/mt... PS20, Line 21: . Remove "."
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 20: Code-Review+1
Hello Hung-Te Lin, Xi Chen, build bot (Jenkins), Roger Lu, Patrick Georgi, Martin Roth, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46930
to look at the new patch set (#21).
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
soc/mediatek/common: Add common API for loading firmwares
Add mtlib_init_mcu to load the firmware to the specified memory address and run the firmware. This function also measures the load time and the blob size. For example:
mtlib_init_mcu: Loaded (and reset) dpm.pm in 15 msecs (14004 bytes)
Signed-off-by: Yidi Lin yidi.lin@mediatek.com Change-Id: Ie94001bbda25fe015f43172e92a1006e059de223 --- A src/soc/mediatek/common/include/soc/mtlib_common.h A src/soc/mediatek/common/mtlib.c M src/soc/mediatek/mt8192/Makefile.inc 3 files changed, 57 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/46930/21
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 21:
(6 comments)
https://review.coreboot.org/c/coreboot/+/46930/20//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46930/20//COMMIT_MSG@10 PS20, Line 10: memory
Can be moved to the previous line.
Done
https://review.coreboot.org/c/coreboot/+/46930/20//COMMIT_MSG@13 PS20, Line 13: mtlib_init_mcu: Load dpm.pm in 13 msecs (14004 bytes)
Please update the format.
Done
https://review.coreboot.org/c/coreboot/+/46930/20/src/soc/mediatek/common/in... File src/soc/mediatek/common/include/soc/mtlib_common.h:
https://review.coreboot.org/c/coreboot/+/46930/20/src/soc/mediatek/common/in... PS20, Line 8: *
One space before "*/"
Done
https://review.coreboot.org/c/coreboot/+/46930/20/src/soc/mediatek/common/in... PS20, Line 8:
Only one space. Same below.
Done
https://review.coreboot.org/c/coreboot/+/46930/20/src/soc/mediatek/common/in... PS20, Line 9: *
One space before "*/"
Done
https://review.coreboot.org/c/coreboot/+/46930/20/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtlib.c:
https://review.coreboot.org/c/coreboot/+/46930/20/src/soc/mediatek/common/mt... PS20, Line 21: .
Remove ". […]
Done
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 21: Code-Review+2
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 21:
One last minor thing: do you think we'll have lots of common APIs in the mtlib?
If not maybe it's better to rename the file to 'mcu.c' and 'mcu.h', and the APIs also don't need to have prefix mtlib
No need to change this if you do plan to add something into mtlib.
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 21:
Patch Set 21:
One last minor thing: do you think we'll have lots of common APIs in the mtlib?
If not maybe it's better to rename the file to 'mcu.c' and 'mcu.h', and the APIs also don't need to have prefix mtlib
No need to change this if you do plan to add something into mtlib.
Could you suggest the struct naming if mtlib_ prefix removed ?
struct mtlib_mcu *mcu
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 21:
Could you suggest the struct naming if mtlib_ prefix removed ? struct mtlib_mcu *mcu
assuming we have mcu_common.h and common/mcu.c, I'll probably name them
struct mtk_mcu;
int mtk_init_mcu(struct mtk_mcu);
or mt_mcu (I think we already have lots of mtk/mt prefix API and structs)
Hello Hung-Te Lin, Xi Chen, build bot (Jenkins), Roger Lu, Patrick Georgi, Martin Roth, Julius Werner, Yu-Ping Wu,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46930
to look at the new patch set (#22).
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
soc/mediatek/common: Add common API for loading firmwares
Add mtk_init_mcu to load the firmware to the specified memory address and run the firmware. This function also measures the load time and the blob size. For example:
mtk_init_mcu: Loaded (and reset) dpm.pm in 15 msecs (14004 bytes)
Signed-off-by: Yidi Lin yidi.lin@mediatek.com Change-Id: Ie94001bbda25fe015f43172e92a1006e059de223 --- A src/soc/mediatek/common/include/soc/mcu_common.h A src/soc/mediatek/common/mcu.c M src/soc/mediatek/mt8192/Makefile.inc 3 files changed, 57 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/46930/22
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 22:
Patch Set 21:
Could you suggest the struct naming if mtlib_ prefix removed ? struct mtlib_mcu *mcu
assuming we have mcu_common.h and common/mcu.c, I'll probably name them
struct mtk_mcu;
int mtk_init_mcu(struct mtk_mcu);
or mt_mcu (I think we already have lots of mtk/mt prefix API and structs)
please review whole series.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
Patch Set 22: Code-Review+2
Thanks so much!
Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46930 )
Change subject: soc/mediatek/common: Add common API for loading firmwares ......................................................................
soc/mediatek/common: Add common API for loading firmwares
Add mtk_init_mcu to load the firmware to the specified memory address and run the firmware. This function also measures the load time and the blob size. For example:
mtk_init_mcu: Loaded (and reset) dpm.pm in 15 msecs (14004 bytes)
Signed-off-by: Yidi Lin yidi.lin@mediatek.com Change-Id: Ie94001bbda25fe015f43172e92a1006e059de223 Reviewed-on: https://review.coreboot.org/c/coreboot/+/46930 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Hung-Te Lin hungte@chromium.org --- A src/soc/mediatek/common/include/soc/mcu_common.h A src/soc/mediatek/common/mcu.c M src/soc/mediatek/mt8192/Makefile.inc 3 files changed, 57 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Hung-Te Lin: Looks good to me, approved
diff --git a/src/soc/mediatek/common/include/soc/mcu_common.h b/src/soc/mediatek/common/include/soc/mcu_common.h new file mode 100644 index 0000000..974da52 --- /dev/null +++ b/src/soc/mediatek/common/include/soc/mcu_common.h @@ -0,0 +1,18 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef SOC_MEDIATEK_MTLIB_COMMON_H +#define SOC_MEDIATEK_MTLIB_COMMON_H + +struct mtk_mcu { + const char *firmware_name; /* The firmware file name in CBFS */ + void *run_address; /* The address for running the firmware */ + size_t run_size; /* The buffer for loading the firmware */ + void *load_buffer; /* The buffer size */ + size_t buffer_size; /* The firmware real size */ + void *priv; /* The additional data required by the reset callback */ + void (*reset)(struct mtk_mcu *mcu); /* The reset callback */ +}; + +int mtk_init_mcu(struct mtk_mcu *mcu); + +#endif /* SOC_MEDIATEK_MTLIB_COMMON_H */ diff --git a/src/soc/mediatek/common/mcu.c b/src/soc/mediatek/common/mcu.c new file mode 100644 index 0000000..d0a1107 --- /dev/null +++ b/src/soc/mediatek/common/mcu.c @@ -0,0 +1,38 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <arch/barrier.h> +#include <cbfs.h> +#include <console/console.h> +#include <soc/mcu_common.h> +#include <soc/symbols.h> +#include <timer.h> + +int mtk_init_mcu(struct mtk_mcu *mcu) +{ + struct stopwatch sw; + + if (!mcu) + return CB_ERR_ARG; + + stopwatch_init(&sw); + + mcu->run_size = cbfs_load(mcu->firmware_name, mcu->load_buffer, mcu->buffer_size); + if (mcu->run_size == 0) { + printk(BIOS_ERR, "%s: Failed to load %s\n", __func__, mcu->firmware_name); + return CB_ERR; + } + + if (mcu->run_address) { + memcpy(mcu->run_address, mcu->load_buffer, mcu->run_size); + /* Memory barrier to ensure data is flushed before resetting MCU. */ + mb(); + } + + if (mcu->reset) + mcu->reset(mcu); + + printk(BIOS_DEBUG, "%s: Loaded (and reset) %s in %ld msecs (%zd bytes)\n", + __func__, mcu->firmware_name, stopwatch_duration_msecs(&sw), mcu->run_size); + + return CB_SUCCESS; +} diff --git a/src/soc/mediatek/mt8192/Makefile.inc b/src/soc/mediatek/mt8192/Makefile.inc index 421968d..8c7fe0d 100644 --- a/src/soc/mediatek/mt8192/Makefile.inc +++ b/src/soc/mediatek/mt8192/Makefile.inc @@ -39,6 +39,7 @@ ramstage-y += ../common/gpio.c gpio.c ramstage-y += emi.c ramstage-$(CONFIG_SPI_FLASH) += ../common/spi.c spi.c +ramstage-y += ../common/mcu.c ramstage-y += ../common/mmu_operations.c mmu_operations.c ramstage-y += ../common/mtcmos.c mtcmos.c ramstage-y += soc.c
-
Hung-Te Lin (Code Review) -
Paul Menzel (Code Review)
-
Yidi Lin (Code Review)
-
Yu-Ping Wu (Code Review)