Change in ...coreboot[master]: mediatek/mt8183: Add option MT8183_MINIMIZE_BOOTBLOCK

[You-Cheng Syu (Code Review)]

You-Cheng Syu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31024

Change subject: mediatek/mt8183: Add option MT8183_MINIMIZE_BOOTBLOCK
......................................................................

mediatek/mt8183: Add option MT8183_MINIMIZE_BOOTBLOCK

MT8183 only allows booting from eMMC, so we have to do eMMC emulation
from an external source, for example EC, which makes the size of
bootblock very important.

This CL adds a new option MT8183_MINIMIZE_BOOTBLOCK in Kconfig. When
enabled, initializations that be can postponed in bootblock will be
removed.

Change-Id: I35d7ab875395da913b967ae1f7b72359be3e744a
Signed-off-by: You-Cheng Syu 
---
M src/soc/mediatek/mt8183/Kconfig
M src/soc/mediatek/mt8183/bootblock.c
2 files changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/31024/1

diff --git a/src/soc/mediatek/mt8183/Kconfig b/src/soc/mediatek/mt8183/Kconfig
index 6582e4e..b8c4dc0 100644
--- a/src/soc/mediatek/mt8183/Kconfig
+++ b/src/soc/mediatek/mt8183/Kconfig
@@ -28,4 +28,8 @@
    bool
    default y

+config MT8183_MINIMIZE_BOOTBLOCK
+	bool
+	default n
+
 endif
diff --git a/src/soc/mediatek/mt8183/bootblock.c b/src/soc/mediatek/mt8183/bootblock.c
index d7d5c2d..809826f 100644
--- a/src/soc/mediatek/mt8183/bootblock.c
+++ b/src/soc/mediatek/mt8183/bootblock.c
@@ -20,5 +20,7 @@
 void bootblock_soc_init(void)
 {
    mt_pll_init();
-	mtk_wdt_init();
+	if (!IS_ENABLED(CONFIG_MT8183_MINIMIZE_BOOTBLOCK)) {
+		mtk_wdt_init();
+	}
 }

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31024 )

Change subject: mediatek/mt8183: Add option MT8183_MINIMIZE_BOOTBLOCK
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31024/1/src/soc/mediatek/mt8183/bootblock.c 
File src/soc/mediatek/mt8183/bootblock.c:

https://review.coreboot.org/#/c/31024/1/src/soc/mediatek/mt8183/bootblock.c@... 
PS1, Line 23: 	if (!IS_ENABLED(CONFIG_MT8183_MINIMIZE_BOOTBLOCK)) {
braces {} are not necessary for single statement blocks

[You-Cheng Syu (Code Review)]

Hello Julius Werner, Tristan Hsieh, Hung-Te Lin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31024

to look at the new patch set (#2).

Change subject: mediatek/mt8183: Add option MT8183_MINIMIZE_BOOTBLOCK
......................................................................

mediatek/mt8183: Add option MT8183_MINIMIZE_BOOTBLOCK

MT8183 only allows booting from eMMC, so we have to do eMMC emulation
from an external source, for example EC, which makes the size of
bootblock very important.

This CL adds a new option MT8183_MINIMIZE_BOOTBLOCK in Kconfig. When
enabled, initializations that be can postponed in bootblock will be
removed.

Change-Id: I35d7ab875395da913b967ae1f7b72359be3e744a
Signed-off-by: You-Cheng Syu 
---
M src/soc/mediatek/mt8183/Kconfig
M src/soc/mediatek/mt8183/bootblock.c
2 files changed, 6 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/31024/2

[You-Cheng Syu (Code Review)]

Hello Julius Werner, Tristan Hsieh, Hung-Te Lin, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31024

to look at the new patch set (#3).

Change subject: mediatek/mt8183: Add option MT8183_MINIMIZE_BOOTBLOCK
......................................................................

mediatek/mt8183: Add option MT8183_MINIMIZE_BOOTBLOCK

MT8183 only allows booting from eMMC, so we have to do eMMC emulation
from an external source, for example EC, which makes the size of
bootblock very important.

This CL adds a new option MT8183_MINIMIZE_BOOTBLOCK in Kconfig. When
enabled, initializations that be can postponed in bootblock will be
removed.

BRANCH=none
BUG=b:120588396
TEST=manually boot into kernel

Change-Id: I35d7ab875395da913b967ae1f7b72359be3e744a
Signed-off-by: You-Cheng Syu 
---
M src/soc/mediatek/mt8183/Kconfig
M src/soc/mediatek/mt8183/bootblock.c
2 files changed, 6 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/31024/3

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31024 )

Change subject: mediatek/mt8183: Add option MT8183_MINIMIZE_BOOTBLOCK
......................................................................

Patch Set 3:

please check build failure

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31024 )

Change subject: mediatek/mt8183: Add option MT8183_MINIMIZE_BOOTBLOCK
......................................................................

Patch Set 4:

FWIW, I don't think this needs to be an explicit option. I think it's fine to just say that mtk_wdt_init() must be called by mainboard code at the appropriate point. If you think that's too obscure, you could wrap it in an SoC function like mt8183_early_init() or something like that to make it look more structured.

If anything, really, the appropriate thing to do would probably be to make sure that Kukui works both with and without CONFIG_VBOOT (since that's a user-facing option that can be turned off). So you should wrap all the verstage_mainboard_init() stuff into another function (e.g. mainboard_early_init() or something like that) and make sure it is also called in the !CONFIG_VBOOT case (e.g. from romstage... see the Nyan board which did something similar with early_mainboard_init()).

Change in ...coreboot[master]: mediatek/mt8183: Move some initialization into mt8183_early_init

[You-Cheng Syu (Code Review)]

Hello Julius Werner, Tristan Hsieh, Hung-Te Lin, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31024

to look at the new patch set (#5).

Change subject: mediatek/mt8183: Move some initialization into mt8183_early_init
......................................................................

mediatek/mt8183: Move some initialization into mt8183_early_init

MT8183 only allows booting from eMMC, so we have to do eMMC emulation
from an external source, for example EC, which makes the size of
bootblock very important.

This CL adds a new function mt8183_early_init, which includes all
initializations that should be done in early stages. All mainboards
using MT8183 should manually call it in either bootblock or verstage.

BRANCH=none
BUG=b:120588396
TEST=manually boot into kernel

Change-Id: I35d7ab875395da913b967ae1f7b72359be3e744a
Signed-off-by: You-Cheng Syu 
---
M src/mainboard/google/kukui/bootblock.c
M src/soc/mediatek/mt8183/Makefile.inc
M src/soc/mediatek/mt8183/bootblock.c
A src/soc/mediatek/mt8183/include/soc/mt8183.h
A src/soc/mediatek/mt8183/mt8183.c
5 files changed, 50 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/31024/5

Change in ...coreboot[master]: mediatek/mt8183: Move some initialization into mt8183_early_init

[You-Cheng Syu (Code Review)]

You-Cheng Syu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31024 )

Change subject: mediatek/mt8183: Move some initialization into mt8183_early_init
......................................................................

Patch Set 5:
> Patch Set 4:
>
> FWIW, I don't think this needs to be an explicit option. I think it's fine to just say that mtk_wdt_init() must be called by mainboard code at the appropriate point. If you think that's too obscure, you could wrap it in an SoC function like mt8183_early_init() or something like that to make it look more structured.
>
> If anything, really, the appropriate thing to do would probably be to make sure that Kukui works both with and without CONFIG_VBOOT (since that's a user-facing option that can be turned off). So you should wrap all the verstage_mainboard_init() stuff into another function (e.g. mainboard_early_init() or something like that) and make sure it is also called in the !CONFIG_VBOOT case (e.g. from romstage... see the Nyan board which did something similar with early_mainboard_init()).

Done

Change in ...coreboot[master]: mediatek/mt8183: Move some initialization into mt8183_early_init

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31024 )

Change subject: mediatek/mt8183: Move some initialization into mt8183_early_init
......................................................................

Patch Set 5: Code-Review+2

Change in ...coreboot[master]: mediatek/mt8183: Move some initialization into mt8183_early_init

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31024 )

Change subject: mediatek/mt8183: Move some initialization into mt8183_early_init
......................................................................

mediatek/mt8183: Move some initialization into mt8183_early_init

MT8183 only allows booting from eMMC, so we have to do eMMC emulation
from an external source, for example EC, which makes the size of
bootblock very important.

This CL adds a new function mt8183_early_init, which includes all
initializations that should be done in early stages. All mainboards
using MT8183 should manually call it in either bootblock or verstage.

BRANCH=none
BUG=b:120588396
TEST=manually boot into kernel

Change-Id: I35d7ab875395da913b967ae1f7b72359be3e744a
Signed-off-by: You-Cheng Syu 
Reviewed-on: https://review.coreboot.org/c/31024
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
---
M src/mainboard/google/kukui/bootblock.c
M src/soc/mediatek/mt8183/Makefile.inc
M src/soc/mediatek/mt8183/bootblock.c
A src/soc/mediatek/mt8183/include/soc/mt8183.h
A src/soc/mediatek/mt8183/mt8183.c
5 files changed, 50 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved

diff --git a/src/mainboard/google/kukui/bootblock.c b/src/mainboard/google/kukui/bootblock.c
index 0728588..ab537d4 100644
--- a/src/mainboard/google/kukui/bootblock.c
+++ b/src/mainboard/google/kukui/bootblock.c
@@ -16,6 +16,7 @@
 #include <bootblock_common.h>
 #include <gpio.h>
 #include <soc/gpio.h>
+#include <soc/mt8183.h>
 #include <soc/spi.h>

#include "gpio.h"
@@ -25,6 +26,8 @@

void bootblock_mainboard_init(void)
 {
+	mt8183_early_init();
+
    setup_chromeos_gpios();

/* Turn on real eMMC. */
diff --git a/src/soc/mediatek/mt8183/Makefile.inc b/src/soc/mediatek/mt8183/Makefile.inc
index 9aa1733..f24fdcc 100644
--- a/src/soc/mediatek/mt8183/Makefile.inc
+++ b/src/soc/mediatek/mt8183/Makefile.inc
@@ -5,6 +5,7 @@
 bootblock-y += ../common/gpio.c gpio.c
 bootblock-y += ../common/pll.c pll.c
 bootblock-$(CONFIG_SPI_FLASH) += ../common/spi.c spi.c
+bootblock-y += mt8183.c
 bootblock-y += ../common/timer.c
 bootblock-y += ../common/uart.c
 bootblock-y += ../common/wdt.c
@@ -16,6 +17,7 @@
 verstage-y += auxadc.c
 verstage-y += ../common/gpio.c gpio.c
 verstage-$(CONFIG_SPI_FLASH) += ../common/spi.c spi.c
+verstage-y += mt8183.c
 verstage-y += ../common/timer.c
 verstage-y += ../common/uart.c
 verstage-y += ../common/wdt.c
diff --git a/src/soc/mediatek/mt8183/bootblock.c b/src/soc/mediatek/mt8183/bootblock.c
index d7d5c2d..e4c331e 100644
--- a/src/soc/mediatek/mt8183/bootblock.c
+++ b/src/soc/mediatek/mt8183/bootblock.c
@@ -15,10 +15,8 @@

#include <bootblock_common.h>
 #include <soc/pll.h>
-#include <soc/wdt.h>

void bootblock_soc_init(void)
 {
    mt_pll_init();
-	mtk_wdt_init();
 }
diff --git a/src/soc/mediatek/mt8183/include/soc/mt8183.h b/src/soc/mediatek/mt8183/include/soc/mt8183.h
new file mode 100644
index 0000000..5591ffd
--- /dev/null
+++ b/src/soc/mediatek/mt8183/include/soc/mt8183.h
@@ -0,0 +1,23 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright 2019 Google 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_H__
+#define __SOC_MEDIATEK_MT8183_H__
+
+/* Mainboards should manually call this function in early stages (bootblock or
+ * verstage). */
+void mt8183_early_init(void);
+
+#endif /* __SOC_MEDIATEK_MT8183_H__ */
diff --git a/src/soc/mediatek/mt8183/mt8183.c b/src/soc/mediatek/mt8183/mt8183.c
new file mode 100644
index 0000000..c441980
--- /dev/null
+++ b/src/soc/mediatek/mt8183/mt8183.c
@@ -0,0 +1,22 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright 2019 Google 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 <soc/mt8183.h>
+#include <soc/wdt.h>
+
+void mt8183_early_init(void)
+{
+	mtk_wdt_init();
+}