Erin Lo has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31516
Change subject: google/kukui: boot up sspm ......................................................................
google/kukui: boot up sspm
Load sspm firmware form cbfs and bring up it.
BUG=b:80501386 BRANCH=none Test=Boots correctly on Kukui.
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/mainboard/google/kukui/Kconfig M src/mainboard/google/kukui/Makefile.inc M src/mainboard/google/kukui/mainboard.c M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 7 files changed, 94 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/1
diff --git a/src/mainboard/google/kukui/Kconfig b/src/mainboard/google/kukui/Kconfig index 9f477e5..0dcdb20 100644 --- a/src/mainboard/google/kukui/Kconfig +++ b/src/mainboard/google/kukui/Kconfig @@ -65,4 +65,8 @@ default "KUKUI TEST 9847" if BOARD_GOOGLE_KUKUI default "FLAPJACK TEST 4147" if BOARD_GOOGLE_FLAPJACK
+config SSPM_BIN_FILE + string "SSPM BIN FILE" + default "" + endif diff --git a/src/mainboard/google/kukui/Makefile.inc b/src/mainboard/google/kukui/Makefile.inc index a0556c1..565c3f7 100644 --- a/src/mainboard/google/kukui/Makefile.inc +++ b/src/mainboard/google/kukui/Makefile.inc @@ -25,3 +25,8 @@ ramstage-y += mainboard.c ramstage-y += memlayout.ld ramstage-y += reset.c + +cbfs-files-y += sspm +sspm-file := $(call strip_quotes,$(CONFIG_SSPM_BIN_FILE)) +sspm-type := raw +sspm-compression :=$(CBFS_COMPRESS_FLAG) diff --git a/src/mainboard/google/kukui/mainboard.c b/src/mainboard/google/kukui/mainboard.c index e1d8f5f..5aaab8b 100644 --- a/src/mainboard/google/kukui/mainboard.c +++ b/src/mainboard/google/kukui/mainboard.c @@ -13,10 +13,13 @@ * GNU General Public License for more details. */
+#include <console/console.h> +#include <cbfs.h> #include <device/device.h> #include <soc/gpio.h> #include <soc/mmu_operations.h> #include <soc/usb.h> +#include <soc/sspm.h>
static void configure_emmc(void) { @@ -37,10 +40,29 @@ setup_usb_host(); }
+#define BUF_SIZE (64 * KiB) +unsigned char buf[BUF_SIZE]; + +static void sspm_boot(void) +{ + size_t fw_size = cbfs_boot_load_file("sspm", buf, sizeof(buf), + CBFS_TYPE_RAW); + + if (fw_size == 0) + printk(BIOS_DEBUG, "no sspm\n"); + else + printk(BIOS_DEBUG, "sspm[0]=%#x, [%zd]=%#x\n", + buf[0], fw_size - 1, buf[fw_size - 1]); + + sspm_init(buf, BUF_SIZE); + +} + static void mainboard_init(struct device *dev) { configure_emmc(); configure_usb(); + sspm_boot(); }
static void mainboard_enable(struct device *dev) diff --git a/src/soc/mediatek/mt8183/Makefile.inc b/src/soc/mediatek/mt8183/Makefile.inc index 5770a83..199b22d 100644 --- a/src/soc/mediatek/mt8183/Makefile.inc +++ b/src/soc/mediatek/mt8183/Makefile.inc @@ -49,6 +49,7 @@ ramstage-y += ../common/uart.c ramstage-y += ../common/usb.c ramstage-y += ../common/wdt.c +ramstage-y += sspm.c
CPPFLAGS_common += -Isrc/soc/mediatek/mt8183/include CPPFLAGS_common += -Isrc/soc/mediatek/common/include diff --git a/src/soc/mediatek/mt8183/include/soc/addressmap.h b/src/soc/mediatek/mt8183/include/soc/addressmap.h index d41b2b9..f812224 100644 --- a/src/soc/mediatek/mt8183/include/soc/addressmap.h +++ b/src/soc/mediatek/mt8183/include/soc/addressmap.h @@ -34,6 +34,7 @@ EMI_BASE = IO_PHYS + 0x00219000, EMI_MPU_BASE = IO_PHYS + 0x00226000, DRAMC_CH_BASE = IO_PHYS + 0x00228000, + SSPM_BASE = IO_PHYS + 0x00440000, AUXADC_BASE = IO_PHYS + 0x01001000, UART0_BASE = IO_PHYS + 0x01002000, SPI0_BASE = IO_PHYS + 0x0100A000, diff --git a/src/soc/mediatek/mt8183/include/soc/sspm.h b/src/soc/mediatek/mt8183/include/soc/sspm.h new file mode 100644 index 0000000..f006ca2 --- /dev/null +++ b/src/soc/mediatek/mt8183/include/soc/sspm.h @@ -0,0 +1,28 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 MediaTek Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef SOC_MEDIATEK_MT8183_SSPM_H +#define SOC_MEDIATEK_MT8183_SSPM_H + +#include <soc/addressmap.h> +#include <types.h> + +struct mt8183_sspm_regs { + u32 sw_rstn; +}; +static struct mt8183_sspm_regs *const mt8183_sspm = (void *)SSPM_BASE; +#define CFG_SSPM_SRAM 0x10400000 +s32 sspm_init(unsigned char *buf, int len); +#endif /* SOC_MEDIATEK_MT8183_SSPM_H */ diff --git a/src/soc/mediatek/mt8183/sspm.c b/src/soc/mediatek/mt8183/sspm.c new file mode 100644 index 0000000..a42a0f5 --- /dev/null +++ b/src/soc/mediatek/mt8183/sspm.c @@ -0,0 +1,33 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 MediaTek Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <arch/barrier.h> +#include <arch/io.h> +#include <soc/gpio.h> +#include <soc/sspm.h> +#include <string.h> + +#define SSPM_UART 1 +s32 sspm_init(unsigned char *buf, int len) +{ + memcpy((void*)CFG_SSPM_SRAM, buf, len); +#if SSPM_UART + gpio_set_mode(GPIO(EINT4), PAD_EINT4_FUNC_SSPM_UTXD_AO); + gpio_set_mode(GPIO(EINT5), PAD_EINT5_FUNC_SSPM_URXD_AO); +#endif + mb(); + write32(&mt8183_sspm->sw_rstn, 0x1); + return 0; +}
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 49: CBFS_TYPE_RAW); code indent should use tabs where possible
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 49: CBFS_TYPE_RAW); please, no spaces at the start of a line
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 55: buf[0], fw_size - 1, buf[fw_size - 1]); code indent should use tabs where possible
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 55: buf[0], fw_size - 1, buf[fw_size - 1]); please, no spaces at the start of a line
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@25 PS1, Line 25: memcpy((void*)CFG_SSPM_SRAM, buf, len); "(foo*)" should be "(foo *)"
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@30 PS1, Line 30: mb(); memory barrier without comment
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/Makefile.... File src/mainboard/google/kukui/Makefile.inc:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/Makefile.... PS1, Line 32: =$ insert a space character
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 16: #include <console/console.h> : #include <cbfs.h> : #include <device/device.h> : #include <soc/gpio.h> : #include <soc/mmu_operations.h> : #include <soc/usb.h> : #include <soc/sspm.h> sort include lines
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 44: unsigned char buf[BUF_SIZE]; move this array into sspm_boot()
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 48: "sspm" Don't directly use string literal as file name.
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/Makefile.inc File src/soc/mediatek/mt8183/Makefile.inc:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/Makefile.inc... PS1, Line 52: ramstage-y += sspm.c move this line in between line 47~48
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/sspm.h:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... PS1, Line 26: #define CFG_SSPM_SRAM 0x10400000 move this line in between line 20~22
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 65: sspm_boot configure_sspm, to this is more consistent with other functions in same file.
Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/addressmap.h:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... PS1, Line 37: tab
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/sspm.h:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... PS1, Line 26: #define CFG_SSPM_SRAM 0x10400000
move this line in between line 20~22
How about we move it to addresmap.h?
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... PS1, Line 27: s32 Would it be better to return int?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@7 PS1, Line 7: google/kukui: boot up sspm please split into two patches, one for 8183/SSPM, and then another to add SSPM for kukui.
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@9 PS1, Line 9: cbfs CBFS
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@9 PS1, Line 9: bring up it enables SSPM
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@9 PS1, Line 9: sspm SSPM
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@22 PS1, Line 22: #define SSPM_UART 1 this should be a Kconfig value
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@31 PS1, Line 31: write32 a command for what this triggers - resetting SSPM ? or also loads the CFG_SSPM_SRAM etc?
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 48: "sspm"
Don't directly use string literal as file name.
Could I use sspm.img?
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/Makefile.inc File src/soc/mediatek/mt8183/Makefile.inc:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/Makefile.inc... PS1, Line 52: ramstage-y += sspm.c
move this line in between line 47~48
Why? I think it is in-order in line 52 ?
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/sspm.h:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... PS1, Line 26: #define CFG_SSPM_SRAM 0x10400000
How about we move it to addresmap. […]
I will move it to addresmap.h, and re-name to SSPM_SRAM_BASE
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@22 PS1, Line 22: #define SSPM_UART 1
this should be a Kconfig value
It is about pin selection depends on platform and just for debug. I will remove it in formal code.
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@31 PS1, Line 31: write32
a command for what this triggers - resetting SSPM ? or also loads the CFG_SSPM_SRAM etc?
resetting SSPM
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 1:
(5 comments)
Where will the firmware be available?
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@9 PS1, Line 9: Load sspm firmware form cbfs and bring up it. bring it up
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@9 PS1, Line 9: form from
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@10 PS1, Line 10: Please describe what SSPM is, or where it is documented.
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@13 PS1, Line 13: Boots correctly on Kukui. How do you verify that SSPM firmware was loaded and works?
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/Kconfig File src/mainboard/google/kukui/Kconfig:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/Kconfig@6... PS1, Line 69: string "SSPM BIN FILE" Please add a help text.
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 44: unsigned char buf[BUF_SIZE];
move this array into sspm_boot()
If move to sspm_boot, it will cause build error. "error: stack usage is 65552 bytes"
If use malloc(), the memory allocates fail . "memalign(boundary=8, size=65536): failed: Tried to round up free_mem_ptr 000000004021be38 to 000000004022be38 but free_mem_end_ptr is 000000004021f038 Error! memalign: Out of memory (free_mem_ptr >= free_mem_end_ptr)"
How could I fix it?
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins), Chun-ta Lin, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31516
to look at the new patch set (#2).
Change subject: google/kukui: boot up sspm ......................................................................
google/kukui: boot up sspm
Load SSPM firmware from CBFS and enables SSPM.
BUG=b:80501386 BRANCH=none Test=Boots correctly on Kukui.
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/mainboard/google/kukui/Kconfig M src/mainboard/google/kukui/Makefile.inc M src/mainboard/google/kukui/mainboard.c M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 7 files changed, 96 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/31516/2/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/2/src/soc/mediatek/mt8183/sspm.c@26 PS2, Line 26: * before we release the reset pin. code indent should use tabs where possible
https://review.coreboot.org/#/c/31516/2/src/soc/mediatek/mt8183/sspm.c@27 PS2, Line 27: */ code indent should use tabs where possible
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 2:
(14 comments)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/Makefile.... File src/mainboard/google/kukui/Makefile.inc:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/Makefile.... PS1, Line 32: =$
insert a space character
Done
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 16: #include <console/console.h> : #include <cbfs.h> : #include <device/device.h> : #include <soc/gpio.h> : #include <soc/mmu_operations.h> : #include <soc/usb.h> : #include <soc/sspm.h>
sort include lines
Done
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 49: CBFS_TYPE_RAW);
code indent should use tabs where possible
Done
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 49: CBFS_TYPE_RAW);
please, no spaces at the start of a line
Done
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 55: buf[0], fw_size - 1, buf[fw_size - 1]);
please, no spaces at the start of a line
Done
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 55: buf[0], fw_size - 1, buf[fw_size - 1]);
code indent should use tabs where possible
Done
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 65: sspm_boot
configure_sspm, to this is more consistent with other functions in same file.
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/addressmap.h:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... PS1, Line 37:
tab
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/sspm.h:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... PS1, Line 26: #define CFG_SSPM_SRAM 0x10400000
I will move it to addresmap. […]
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... PS1, Line 27: s32
Would it be better to return int?
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@22 PS1, Line 22: #define SSPM_UART 1
It is about pin selection depends on platform and just for debug. […]
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@25 PS1, Line 25: memcpy((void*)CFG_SSPM_SRAM, buf, len);
"(foo*)" should be "(foo *)"
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@30 PS1, Line 30: mb();
memory barrier without comment
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@31 PS1, Line 31: write32
resetting SSPM
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 44: unsigned char buf[BUF_SIZE];
If move to sspm_boot, it will cause build error.
I think you mean declare that as auto variable inside function, which uses stack and would definitely run out of space.
If use malloc(), the memory allocates fail .
Like it said, I think heap reserved there is less than 64K. There are few solutions:
1. If we think the sspm image file won't be compressed in future, just use cbfs_boot_map_with_leak .
2. If we think it'll save a lot of space by applying compression (and should be compressed), increase the heap in ramstage (src/soc/mediatek/mt8183/include/soc/memlayout.ld) may help.
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 44: unsigned char buf[BUF_SIZE];
If move to sspm_boot, it will cause build error. […]
Actually the main reason I don't want this array be put here is that: This array is not 'static' (able to be linked with other files) and with a really common name. So if we do want to compress it, I think you can also move it into sspm_boot() and make it static.
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 48: "sspm"
Could I use sspm. […]
The point is to declare a 'const char*' / macro for the filename.
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/Makefile.inc File src/soc/mediatek/mt8183/Makefile.inc:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/Makefile.inc... PS1, Line 52: ramstage-y += sspm.c
Why? I think it is in-order in line 52 ?
I think we sort these lines by their basename.
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 44: unsigned char buf[BUF_SIZE];
Actually the main reason I don't want this array be put here is that: […]
Besides, is it possible that we directly load the file into SSPM_SRAM_BASE?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 44: unsigned char buf[BUF_SIZE];
Besides, is it possible that we directly load the file into SSPM_SRAM_BASE?
That's an interesting idea... yes I think that may also work...?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/31516/2/src/mainboard/google/kukui/Kconfig File src/mainboard/google/kukui/Kconfig:
https://review.coreboot.org/#/c/31516/2/src/mainboard/google/kukui/Kconfig@6... PS2, Line 68: config SSPM_BIN_FILE Why is this a Kconfig? This should just be a well-known name (e.g. 3rdparty/blobs/soc/mediatek/mt8183/sspm.bin). Then commit the binary to https://review.coreboot.org/cgit/blobs.git.
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 44: unsigned char buf[BUF_SIZE];
That's an interesting idea... yes I think that may also work... […]
Yeah, that's definitely the right thing to do, don't copy 64KB around more than necessary.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31516/2/src/mainboard/google/kukui/Kconfig File src/mainboard/google/kukui/Kconfig:
https://review.coreboot.org/#/c/31516/2/src/mainboard/google/kukui/Kconfig@6... PS2, Line 68: config SSPM_BIN_FILE
Why is this a Kconfig? This should just be a well-known name (e.g. […]
@Julius I requested this to be a Kconfig, in case if we need to create build target-specific SSPM files in future, or even devices that do not have SSPM image files. What do you think?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31516/2/src/mainboard/google/kukui/Kconfig File src/mainboard/google/kukui/Kconfig:
https://review.coreboot.org/#/c/31516/2/src/mainboard/google/kukui/Kconfig@6... PS2, Line 68: config SSPM_BIN_FILE
@Julius I requested this to be a Kconfig, in case if we need to create build target-specific SSPM fi […]
Okay, maybe I misunderstood the purpose of the SSPM. Are there board-specific bits encoded in the binary? I was hoping it would always be the same for the whole SoC? Can you explain what we expect here?
If the binary is board-specific, it would probably still make more sense to keep a well-known name, just make it 3rdparty/blobs/src/mainboard/... instead. Unless there are even differences between variants of the same board (e.g. flapjack and kukui)? Then, yes, it needs to be a Kconfig. Although maybe it should still just be the base filename in the Kconfig then.
Note that if you're just worried about some weird edge case where we'd want to have different SSPM versions between Chrome OS boards to isolate releases or something, we can always do that at the ebuild level. They should only be separate in upstream coreboot if we really think they'll need to be different long-term. (And if we're just not sure yet, we can always put it at the SoC level now and change later if necessary.)
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 44: unsigned char buf[BUF_SIZE];
Yeah, that's definitely the right thing to do, don't copy 64KB around more than necessary.
SSPM_SRAM_BASE is accessed through APB box. It can't support byte access only supports word access. Does it touch any limitation in cbfs_boot_load_file()? I found "lzma: Decoding error = 1" when directly load the file into SSPM_SRAM_BASE.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 44: unsigned char buf[BUF_SIZE];
SSPM_SRAM_BASE is accessed through APB box. It can't support byte access only supports word access. […]
Can you map the area as cacheable? That might prevent that problem because the cache will combine writes. Caching is also important for decompression speed (because the decompression algorithm does many back-references to already decompressed parts).
If you can only map it as device memory, then yeah, you'll probably not get around a static bounce buffer here. It shouldn't be a huge deal because ramstage size is usually not limited. (I'd hide all of that in a separate file then, though... in fact, why is any of this called from mainboard.c? Can't you put it in soc.c?)
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins), Chun-ta Lin, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31516
to look at the new patch set (#3).
Change subject: google/kukui: boot up sspm ......................................................................
google/kukui: boot up sspm
Load SSPM firmware from CBFS and enables SSPM.
BUG=b:80501386 BRANCH=none Test=We can see "sspm is alive" in ATF stage if SSPM enabled and ipi success But now! the loaded contents are worng by using cbfs_boot_map_with_leak(), need to debug
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/mainboard/google/kukui/Kconfig M src/mainboard/google/kukui/Makefile.inc M src/mainboard/google/kukui/mainboard.c M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 7 files changed, 96 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/3
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 3:
(10 comments)
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@9 PS1, Line 9: Load sspm firmware form cbfs and bring up it.
bring it up
I modify to "enable"
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@9 PS1, Line 9: cbfs
CBFS
Done
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@9 PS1, Line 9: form
from
Done
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@9 PS1, Line 9: sspm
SSPM
Done
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@9 PS1, Line 9: bring up it
enables SSPM
Done
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@13 PS1, Line 13: Boots correctly on Kukui.
How do you verify that SSPM firmware was loaded and works?
Done
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 44: unsigned char buf[BUF_SIZE];
Can you map the area as cacheable? That might prevent that problem because the cache will combine wr […]
The area can't set as cacheable. We don't compress it now. I have move it to src/soc/mediatek/mt8183/sspm.c and use cbfs_boot_map_with_leak(). But it seems the loaded contents are wrong.
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 48: "sspm"
The point is to declare a 'const char*' / macro for the filename.
Done
https://review.coreboot.org/#/c/31516/2/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/2/src/soc/mediatek/mt8183/sspm.c@26 PS2, Line 26: * before we release the reset pin.
code indent should use tabs where possible
Done
https://review.coreboot.org/#/c/31516/2/src/soc/mediatek/mt8183/sspm.c@27 PS2, Line 27: */
code indent should use tabs where possible
Done
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins), Chun-ta Lin, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31516
to look at the new patch set (#4).
Change subject: google/kukui: boot up sspm ......................................................................
google/kukui: boot up sspm
Load SSPM firmware from CBFS and enables SSPM.
BUG=b:80501386 BRANCH=none Test=We can see "sspm is alive" in ATF stage if SSPM enabled and ipi success But now! the loaded contents are worng by using cbfs_boot_map_with_leak(), need to debug
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/mainboard/google/kukui/Kconfig M src/mainboard/google/kukui/Makefile.inc M src/mainboard/google/kukui/mainboard.c M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 7 files changed, 97 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/4
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@9 PS1, Line 9: Load sspm firmware form cbfs and bring up it.
I modify to "enable"
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/Makefile.inc File src/soc/mediatek/mt8183/Makefile.inc:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/Makefile.inc... PS1, Line 52: ramstage-y += sspm.c
I think we sort these lines by their basename.
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 4:
(8 comments)
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG@7 PS4, Line 7: boot up sspm Boot up SSPM
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG@9 PS4, Line 9: enables enable
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG@14 PS4, Line 14: worng wrong
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig File src/mainboard/google/kukui/Kconfig:
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig@7... PS4, Line 71: ---help--- That syntax seems new.
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig@7... PS4, Line 72: SSPM is Secure System Power Manager for power control in secure domain. Please add a dot at the end.
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@23 PS4, Line 23: void Make that bool?
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@35 PS4, Line 35: printk(BIOS_DEBUG, "no sspm\n"); No SSPM firmware(?) found.
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@35 PS4, Line 35: BIOS_DEBUG Make that INFO?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig File src/mainboard/google/kukui/Kconfig:
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig@6... PS4, Line 68: config SSPM_BIN_FILE Still think this shouldn't be a Kconfig...
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Makefile.... File src/mainboard/google/kukui/Makefile.inc:
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Makefile.... PS4, Line 32: sspm-compression := $(CBFS_COMPRESS_FLAG) Odd that you said you weren't compressing it anymore now? Because this definitely looks like you do (and you should, compression is almost certainly a much bigger speed gain than what a memcpy or two would cost).
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/mainboard... PS4, Line 41: static void configure_sspm(void) nit: You don't really need single line functions, can just put sspm_init() into mainboard_init().
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@23 PS4, Line 23: void
Make that bool?
Probably better to just die() if something goes wrong, assuming the board cannot really boot without this anyway?
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@35 PS4, Line 35: BIOS_DEBUG
Make that INFO?
I'd say at least ERR, but probably a die()
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@38 PS4, Line 38: "sspm[0]=%#x, [%zd]=%#x\n", nit: I'm not sure this is useful enough to keep in here after you're done debugging?
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Makefile.... File src/mainboard/google/kukui/Makefile.inc:
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Makefile.... PS4, Line 32: sspm-compression := $(CBFS_COMPRESS_FLAG)
Odd that you said you weren't compressing it anymore now? Because this definitely looks like you do […]
Sorry, I didn't recognize it compressed or not decided here. Do you mean you suggest keep it in compressed? And then we can't use cbfs_boot_map_with_leak()?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Makefile.... File src/mainboard/google/kukui/Makefile.inc:
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Makefile.... PS4, Line 32: sspm-compression := $(CBFS_COMPRESS_FLAG)
Sorry, I didn't recognize it compressed or not decided here. […]
Oh... yes, actually, that's the cause of your problem. I thought cbfs_boot_map_with_leak() would also transparently decompress things, but on closer inspection it doesn't.
I think you should just go back to having a big static uint8_t[] buffer in sspm.c and then cbfs_boot_load_file() into there (that will transparently decompress), then memcpy().
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins), Chun-ta Lin, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31516
to look at the new patch set (#5).
Change subject: google/kukui: Boot up SSPM ......................................................................
google/kukui: Boot up SSPM
Load SSPM firmware from CBFS and enable SSPM.
BUG=b:80501386 BRANCH=none Test=We can see "sspm is alive" in ATF stage if SSPM enabled and ipi success
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/mainboard/google/kukui/Makefile.inc M src/mainboard/google/kukui/mainboard.c M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 6 files changed, 78 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/5
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: Boot up SSPM ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31516/5/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/5/src/soc/mediatek/mt8183/sspm.c@29 PS5, Line 29: size_t fw_size = cbfs_boot_load_file(file_name, sspm_bin, sizeof(sspm_bin), line over 80 characters
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins), Chun-ta Lin, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31516
to look at the new patch set (#6).
Change subject: google/kukui: Boot up SSPM ......................................................................
google/kukui: Boot up SSPM
Load SSPM firmware from CBFS and enable SSPM.
BUG=b:80501386 BRANCH=none Test=We can see "sspm is alive" in ATF stage if SSPM enabled and ipi success
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/mainboard/google/kukui/Makefile.inc M src/mainboard/google/kukui/mainboard.c M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 6 files changed, 80 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/6
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: Boot up SSPM ......................................................................
Patch Set 7:
(12 comments)
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG@7 PS4, Line 7: boot up sspm
Boot up SSPM
Done
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG@9 PS4, Line 9: enables
enable
Done
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG@14 PS4, Line 14: worng
wrong
Removed it
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig File src/mainboard/google/kukui/Kconfig:
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig@6... PS4, Line 68: config SSPM_BIN_FILE
Still think this shouldn't be a Kconfig...
Removed it
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig@7... PS4, Line 71: ---help---
That syntax seems new.
Removed it
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig@7... PS4, Line 72: SSPM is Secure System Power Manager for power control in secure domain.
Please add a dot at the end.
Removed it
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Makefile.... File src/mainboard/google/kukui/Makefile.inc:
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Makefile.... PS4, Line 32: sspm-compression := $(CBFS_COMPRESS_FLAG)
Oh... yes, actually, that's the cause of your problem. […]
Done
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/mainboard... PS4, Line 41: static void configure_sspm(void)
nit: You don't really need single line functions, can just put sspm_init() into mainboard_init().
Done
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@23 PS4, Line 23: void
Probably better to just die() if something goes wrong, assuming the board cannot really boot without […]
Done
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@35 PS4, Line 35: printk(BIOS_DEBUG, "no sspm\n");
No SSPM firmware(?) found.
Done
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@35 PS4, Line 35: BIOS_DEBUG
I'd say at least ERR, but probably a die()
Done
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@38 PS4, Line 38: "sspm[0]=%#x, [%zd]=%#x\n",
nit: I'm not sure this is useful enough to keep in here after you're done debugging?
Done
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: Boot up SSPM ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG@14 PS4, Line 14: worng
Removed it
Done
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig File src/mainboard/google/kukui/Kconfig:
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig@6... PS4, Line 68: config SSPM_BIN_FILE
Removed it
Done
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig@7... PS4, Line 71: ---help---
Removed it
Done
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig@7... PS4, Line 72: SSPM is Secure System Power Manager for power control in secure domain.
Removed it
Done
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: Boot up SSPM ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/Kconfig File src/mainboard/google/kukui/Kconfig:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/Kconfig@6... PS1, Line 69: string "SSPM BIN FILE"
Please add a help text.
Done Removed this config
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 44: unsigned char buf[BUF_SIZE];
The area can't set as cacheable. […]
Done
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: Boot up SSPM ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31516/2/src/mainboard/google/kukui/Kconfig File src/mainboard/google/kukui/Kconfig:
https://review.coreboot.org/#/c/31516/2/src/mainboard/google/kukui/Kconfig@6... PS2, Line 68: config SSPM_BIN_FILE
Okay, maybe I misunderstood the purpose of the SSPM. […]
Done Remove this config
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: Boot up SSPM ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
LGTM, but you'll need to submit the binary to blobs.git first, obviously.
https://review.coreboot.org/#/c/31516/7/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/7/src/soc/mediatek/mt8183/sspm.c@24 PS7, Line 24: static uint8_t sspm_bin[BUF_SIZE]; nit: I don't exactly remember the default alignment rules in effect here, but you may wanna add an __aligned(8) here just to make sure it will always be well-aligned (because the compiler just sees uint8_t with a 1-byte alignment requirement here).
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: Boot up SSPM ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31516/7/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/7/src/soc/mediatek/mt8183/sspm.c@24 PS7, Line 24: static uint8_t sspm_bin[BUF_SIZE];
nit: I don't exactly remember the default alignment rules in effect here, but you may wanna add an _ […]
Maybe I need to implement memory copy by myself since destination sram need word aligned access? And source dram should be __aligned(4)?
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: Boot up SSPM ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31516/7/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/7/src/soc/mediatek/mt8183/sspm.c@24 PS7, Line 24: static uint8_t sspm_bin[BUF_SIZE];
Maybe I need to implement memory copy by myself since destination sram need word aligned access? […]
You need to make sure your sspm_bin[] is aligned so that memcpy will do word aligned access. See: https://github.com/coreboot/coreboot/blob/master/payloads/libpayload/libc/me...
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: Boot up SSPM ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31516/7/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/7/src/soc/mediatek/mt8183/sspm.c@24 PS7, Line 24: static uint8_t sspm_bin[BUF_SIZE];
You need to make sure your sspm_bin[] is aligned so that memcpy will do word aligned access. […]
That's the wrong memcpy, you'll be using this one: https://github.com/coreboot/coreboot/blob/master/src/arch/arm64/memcpy.S
But as long as you add __aligned(8) here and your SRAM buffer is aligned anyway, you should be fine using that. (I assume when you say word-aligned access, 8-byte stores will also be fine? Or does it have to be 4-byte stores?)
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: Boot up SSPM ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31516/7/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/7/src/soc/mediatek/mt8183/sspm.c@24 PS7, Line 24: static uint8_t sspm_bin[BUF_SIZE];
That's the wrong memcpy, you'll be using this one: https://github. […]
Sure, I can use __aligned(8).
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins), Chun-ta Lin, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31516
to look at the new patch set (#8).
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
mediatek/mt8183: Add SSPM init support
SSPM is a Secure System Power Manager for power control in secure domain. The init flow is load SSPM firmware to SSPM's sram and enable SSPM.
BUG=b:80501386 BRANCH=none Test=Build pass
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 4 files changed, 73 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/8
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@7 PS1, Line 7: google/kukui: boot up sspm
please split into two patches, one for 8183/SSPM, and then another to add SSPM for kukui.
Done
https://review.coreboot.org/#/c/31516/1//COMMIT_MSG@10 PS1, Line 10:
Please describe what SSPM is, or where it is documented.
Done I describe in commit message.
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/#/c/31516/7/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/7/src/soc/mediatek/mt8183/sspm.c@24 PS7, Line 24: static uint8_t sspm_bin[BUF_SIZE];
Sure, I can use __aligned(8).
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 8: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31516/8/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/8/src/soc/mediatek/mt8183/sspm.c@24 PS8, Line 24: __attribute__((aligned(8))) nit: please use the __aligned(8) shortcut macro (from <commonlib/compiler.h>, should be automatically included by build system).
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins), Chun-ta Lin, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31516
to look at the new patch set (#9).
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
mediatek/mt8183: Add SSPM init support
SSPM is a Secure System Power Manager for power control in secure domain. The init flow is load SSPM firmware to SSPM's sram and enable SSPM.
BUG=b:80501386 BRANCH=none Test=Build pass
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 4 files changed, 74 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/9
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/31516/8/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/8/src/soc/mediatek/mt8183/sspm.c@24 PS8, Line 24: __attribute__((aligned(8)))
nit: please use the __aligned(8) shortcut macro (from <commonlib/compiler. […]
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 9: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31516/9/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/9/src/soc/mediatek/mt8183/sspm.c@18 PS9, Line 18: #include <commonlib/compiler.h> nit: You don't actually need to write this #include, the Makefile adds it automatically to all files.
Hung-Te Lin has uploaded a new patch set (#10) to the change originally created by Erin Lo. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
mediatek/mt8183: Add SSPM init support
SSPM is a Secure System Power Manager for power control in secure domain. The init flow is load SSPM firmware to SSPM's sram and enable SSPM.
BUG=b:80501386 BRANCH=none Test=Build pass
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 4 files changed, 73 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/10
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 10:
(1 comment)
Can we merge this now?
https://review.coreboot.org/#/c/31516/9/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/9/src/soc/mediatek/mt8183/sspm.c@18 PS9, Line 18: #include <commonlib/compiler.h>
nit: You don't actually need to write this #include, the Makefile adds it automatically to all files […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 10:
(6 comments)
https://review.coreboot.org/#/c/31516/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31516/10//COMMIT_MSG@9 PS10, Line 9: SSPM is a Secure System Power Manager for : power control in secure domain. Please use the full text width.
https://review.coreboot.org/#/c/31516/10//COMMIT_MSG@11 PS10, Line 11: is load is to load
https://review.coreboot.org/#/c/31516/10//COMMIT_MSG@11 PS10, Line 11: SSPM firmware Is that already in the 3rdparty repository?
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c@35 PS10, Line 35: die("SSPM file not found."); As the name is in a variable, please print the name in the error message too.
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c@38 PS10, Line 38: are is
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c@38 PS10, Line 38: /* Memory barrier to ensure that all fw code are loaded : * before we release the reset pin. : */ Please follow the coding style.
/* … … */
[1]: https://doc.coreboot.org/coding_style.html?highlight=coding#commenting
You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c@19 PS10, Line 19: <arch/io.h> This header file has been renamed. Use "<arch/mmio.h>" instead.
Hung-Te Lin has uploaded a new patch set (#11) to the change originally created by Erin Lo. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
mediatek/mt8183: Add SSPM init support
SSPM is a Secure System Power Manager for power control in secure domain. The init flow is load SSPM firmware to SSPM's sram and enable SSPM.
BUG=b:80501386 BRANCH=none Test=Build pass
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 4 files changed, 73 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/11
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins), Chun-ta Lin, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31516
to look at the new patch set (#12).
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
mediatek/mt8183: Add SSPM init support
SSPM is a Secure System Power Manager for power control in secure domain. The init flow is to load SSPM firmware to SSPM's sram and enable SSPM.
BUG=b:80501386 BRANCH=none Test=Build pass
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 4 files changed, 72 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/12
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/#/c/31516/12/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/12/src/soc/mediatek/mt8183/sspm.c@35 PS12, Line 35: die("SSPM file :%s not found.",file_name); space required after that ',' (ctx:VxV)
Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 12:
(7 comments)
Patch Set 10:
(1 comment)
https://review.coreboot.org/#/c/31516/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31516/10//COMMIT_MSG@9 PS10, Line 9: SSPM is a Secure System Power Manager for : power control in secure domain.
Please use the full text width.
Done
https://review.coreboot.org/#/c/31516/10//COMMIT_MSG@11 PS10, Line 11: is load
is to load
Done
https://review.coreboot.org/#/c/31516/10//COMMIT_MSG@11 PS10, Line 11: SSPM firmware
Is that already in the 3rdparty repository?
https://review.coreboot.org/c/blobs/+/32698
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c@19 PS10, Line 19: <arch/io.h>
This header file has been renamed. Use "<arch/mmio.h>" instead.
Done
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c@35 PS10, Line 35: die("SSPM file not found.");
As the name is in a variable, please print the name in the error message too.
Done
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c@38 PS10, Line 38: are
is
Done
https://review.coreboot.org/#/c/31516/10/src/soc/mediatek/mt8183/sspm.c@38 PS10, Line 38: /* Memory barrier to ensure that all fw code are loaded : * before we release the reset pin. : */
Please follow the coding style. […]
Done
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins), Chun-ta Lin, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31516
to look at the new patch set (#13).
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
mediatek/mt8183: Add SSPM init support
SSPM is a Secure System Power Manager for power control in secure domain. The init flow is to load SSPM firmware to SSPM's sram and enable SSPM.
BUG=b:80501386 BRANCH=none Test=Build pass
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 4 files changed, 72 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/13
Hello Julius Werner, You-Cheng Syu, Hung-Te Lin, build bot (Jenkins), Chun-ta Lin, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31516
to look at the new patch set (#14).
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
mediatek/mt8183: Add SSPM init support
SSPM is a Secure System Power Manager for power control in secure domain. The init flow is to load SSPM firmware to SSPM's sram and enable SSPM.
BUG=b:80501386 BRANCH=none Test=Build pass
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 4 files changed, 72 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/14
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 15: Code-Review+2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 15:
Not sure what's wrong here, Jenkins already garbage collected the CI results. If you rebase, it should run again.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 15: Code-Review+1
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: mediatek/mt8183: Add SSPM init support ......................................................................
Patch Set 16:
Oh okay, this is just failing because of the Arm TF stuff in the other patches that hasn't landed yet. If you want you can reorder these patches so the SSPM stuff comes first and then we can land it already.
Hung-Te Lin has uploaded a new patch set (#18) to the change originally created by Erin Lo. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: soc/mediatek/mt8183: Support SSPM ......................................................................
soc/mediatek/mt8183: Support SSPM
SSPM is "Secure System Power Manager" that provides power control in secure domain. The initialization flow is to load SSPM firmware to its SRAM space and then enable.
BUG=b:80501386 BRANCH=none Test=Build pass
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com --- M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 4 files changed, 72 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/31516/18
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: soc/mediatek/mt8183: Support SSPM ......................................................................
Patch Set 18: Code-Review+1
Build should pass now
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: soc/mediatek/mt8183: Support SSPM ......................................................................
Patch Set 19:
Build passes, time to merge?
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: soc/mediatek/mt8183: Support SSPM ......................................................................
Patch Set 19:
Patch Set 19:
Build passes, time to merge?
Sure, it still needs a +2, and all of the outstanding comments in the code need to be marked as resolved.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: soc/mediatek/mt8183: Support SSPM ......................................................................
Patch Set 19: Code-Review+2
(2 comments)
I hope this comment resolving thing will become useful in the long term but it's certainly annoying for the transition period...
https://review.coreboot.org/c/coreboot/+/31516/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/31516/1//COMMIT_MSG@10 PS1, Line 10:
Done […]
Done
https://review.coreboot.org/c/coreboot/+/31516/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/31516/10//COMMIT_MSG@11 PS10, Line 11: SSPM firmware
Done
Julius Werner has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: soc/mediatek/mt8183: Support SSPM ......................................................................
soc/mediatek/mt8183: Support SSPM
SSPM is "Secure System Power Manager" that provides power control in secure domain. The initialization flow is to load SSPM firmware to its SRAM space and then enable.
BUG=b:80501386 BRANCH=none Test=Build pass
Change-Id: I4ae6034454326f5115cd3948819adc448b67fb1c Signed-off-by: Erin Lo erin.lo@mediatek.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31516 Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Hung-Te Lin hungte@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/mediatek/mt8183/Makefile.inc M src/soc/mediatek/mt8183/include/soc/addressmap.h A src/soc/mediatek/mt8183/include/soc/sspm.h A src/soc/mediatek/mt8183/sspm.c 4 files changed, 72 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved Hung-Te Lin: Looks good to me, but someone else must approve
diff --git a/src/soc/mediatek/mt8183/Makefile.inc b/src/soc/mediatek/mt8183/Makefile.inc index d35a07e..0a79ed7 100644 --- a/src/soc/mediatek/mt8183/Makefile.inc +++ b/src/soc/mediatek/mt8183/Makefile.inc @@ -49,6 +49,7 @@ ramstage-y += ../common/rtc.c rtc.c ramstage-y += soc.c ramstage-$(CONFIG_SPI_FLASH) += ../common/spi.c spi.c +ramstage-y += sspm.c ramstage-y += ../common/timer.c ramstage-y += ../common/uart.c ramstage-y += ../common/usb.c diff --git a/src/soc/mediatek/mt8183/include/soc/addressmap.h b/src/soc/mediatek/mt8183/include/soc/addressmap.h index e9f80d1..0f085b2 100644 --- a/src/soc/mediatek/mt8183/include/soc/addressmap.h +++ b/src/soc/mediatek/mt8183/include/soc/addressmap.h @@ -34,6 +34,8 @@ EMI_BASE = IO_PHYS + 0x00219000, EMI_MPU_BASE = IO_PHYS + 0x00226000, DRAMC_CH_BASE = IO_PHYS + 0x00228000, + SSPM_SRAM_BASE = IO_PHYS + 0x00400000, + SSPM_CFG_BASE = IO_PHYS + 0x00440000, AUXADC_BASE = IO_PHYS + 0x01001000, UART0_BASE = IO_PHYS + 0x01002000, SPI0_BASE = IO_PHYS + 0x0100A000, diff --git a/src/soc/mediatek/mt8183/include/soc/sspm.h b/src/soc/mediatek/mt8183/include/soc/sspm.h new file mode 100644 index 0000000..627088f --- /dev/null +++ b/src/soc/mediatek/mt8183/include/soc/sspm.h @@ -0,0 +1,27 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 MediaTek Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef SOC_MEDIATEK_MT8183_SSPM_H +#define SOC_MEDIATEK_MT8183_SSPM_H + +#include <soc/addressmap.h> +#include <types.h> + +struct mt8183_sspm_regs { + u32 sw_rstn; +}; +static struct mt8183_sspm_regs *const mt8183_sspm = (void *)SSPM_CFG_BASE; +void sspm_init(void); +#endif /* SOC_MEDIATEK_MT8183_SSPM_H */ diff --git a/src/soc/mediatek/mt8183/sspm.c b/src/soc/mediatek/mt8183/sspm.c new file mode 100644 index 0000000..559034e --- /dev/null +++ b/src/soc/mediatek/mt8183/sspm.c @@ -0,0 +1,42 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2019 MediaTek Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <arch/barrier.h> +#include <cbfs.h> +#include <console/console.h> +#include <arch/mmio.h> +#include <soc/sspm.h> +#include <string.h> + +#define BUF_SIZE (64 * KiB) +static uint8_t sspm_bin[BUF_SIZE] __aligned(8); + +void sspm_init(void) +{ + const char *file_name = "sspm.bin"; + size_t fw_size = cbfs_boot_load_file(file_name, + sspm_bin, + sizeof(sspm_bin), + CBFS_TYPE_RAW); + + if (fw_size == 0) + die("SSPM file :sspm.bin not found."); + + memcpy((void *)SSPM_SRAM_BASE, sspm_bin, fw_size); + /* Memory barrier to ensure that all fw code is loaded + before we release the reset pin. */ + mb(); + write32(&mt8183_sspm->sw_rstn, 0x1); +}
-
build bot (Jenkins) (Code Review) -
Erin Lo (Code Review)
-
Hung-Te Lin (Code Review)
-
Julius Werner (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)
-
Tristan Hsieh (Code Review)
-
You-Cheng Syu (Code Review)
-
Yu-Ping Wu (Code Review)