Change in ...coreboot[master]: mediatek/mt8183: Add SPI GPIO driving setting

[Mengqi Zhang (Code Review)]

Mengqi Zhang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32460

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

mediatek/mt8183: Add SPI GPIO driving setting

Set SPI GPIO driving to support SPI FLASH.

BUG=b:80501386
BRANCH=none
TEST=emerge-kukui coreboot; emerge-elm coreboot

Change-Id: I95002ec71abd751c33c089185db04ed4a8686699
Signed-off-by: Mengqi Zhang 
---
M src/soc/mediatek/mt8183/gpio.c
M src/soc/mediatek/mt8183/include/soc/gpio.h
M src/soc/mediatek/mt8183/mt8183.c
3 files changed, 15 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/32460/1

diff --git a/src/soc/mediatek/mt8183/gpio.c b/src/soc/mediatek/mt8183/gpio.c
index 327d389..2bff3d0 100644
--- a/src/soc/mediatek/mt8183/gpio.c
+++ b/src/soc/mediatek/mt8183/gpio.c
@@ -20,6 +20,7 @@
    EN_OFFSET = 0x60,
    SEL_OFFSET = 0x80,
    EH_RSEL_OFFSET = 0xF0,
+	SPI_GPIO_DRV_OFFSET = 0xA0,
 };

static void gpio_set_pull_pupd(gpio_t gpio, enum pull_enable enable,
@@ -128,3 +129,14 @@
    	I2C_EH_RSL_MASK(SCL5) | I2C_EH_RSL_MASK(SDA5),
    	I2C_EH_RSL_VAL(SCL5) | I2C_EH_RSL_VAL(SDA5));
 }
+
+enum {
+	SPI_GPIO_DRV_MASK = 0xf,
+};
+
+void gpio_set_spi_driving(void)
+{
+	clrsetbits_le32((void *)IOCFG_LM_BASE + SPI_GPIO_DRV_OFFSET,
+			SPI_GPIO_DRV_MASK, 0x4);
+}
+
diff --git a/src/soc/mediatek/mt8183/include/soc/gpio.h b/src/soc/mediatek/mt8183/include/soc/gpio.h
index 5a98953..e446853 100644
--- a/src/soc/mediatek/mt8183/include/soc/gpio.h
+++ b/src/soc/mediatek/mt8183/include/soc/gpio.h
@@ -617,5 +617,6 @@

static struct gpio_regs *const mtk_gpio = (void *)(GPIO_BASE);
 void gpio_set_i2c_eh_rsel(void);
+void gpio_set_spi_driving(void);

#endif
diff --git a/src/soc/mediatek/mt8183/mt8183.c b/src/soc/mediatek/mt8183/mt8183.c
index c441980..7ceeb90 100644
--- a/src/soc/mediatek/mt8183/mt8183.c
+++ b/src/soc/mediatek/mt8183/mt8183.c
@@ -15,8 +15,10 @@

#include <soc/mt8183.h>
 #include <soc/wdt.h>
+#include <soc/gpio.h>

void mt8183_early_init(void)
 {
    mtk_wdt_init();
+	gpio_set_spi_driving();
 }

[Hung-Te Lin (Code Review)]

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32460/1/src/soc/mediatek/mt8183/gpio.c 
File src/soc/mediatek/mt8183/gpio.c:

https://review.coreboot.org/#/c/32460/1/src/soc/mediatek/mt8183/gpio.c@142 
PS1, Line 142: 
remove one blank line to prevent build check failure.

[Mengqi Zhang (Code Review)]

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

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

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

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

mediatek/mt8183: Add SPI GPIO driving setting

Set SPI GPIO driving to support SPI FLASH.

BUG=b:80501386
BRANCH=none
TEST=emerge-kukui coreboot; emerge-elm coreboot

Change-Id: I95002ec71abd751c33c089185db04ed4a8686699
Signed-off-by: Mengqi Zhang 
---
M src/soc/mediatek/mt8183/gpio.c
M src/soc/mediatek/mt8183/include/soc/gpio.h
M src/soc/mediatek/mt8183/mt8183.c
3 files changed, 14 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/32460/2

[Mengqi Zhang (Code Review)]

Mengqi Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32460 )

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32460/1/src/soc/mediatek/mt8183/gpio.c 
File src/soc/mediatek/mt8183/gpio.c:

https://review.coreboot.org/#/c/32460/1/src/soc/mediatek/mt8183/gpio.c@142 
PS1, Line 142:
> remove one blank line to prevent build check failure.

Done

[Hung-Te Lin (Code Review)]

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 2: Code-Review+1

@Julius do you want to have a final confirm?

[Julius Werner (Code Review)]

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/gpio.c 
File src/soc/mediatek/mt8183/gpio.c:

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/gpio.c@140 
PS2, Line 140: 0x4
Where does this 0x4 come from? Is this a drive strength? Or a bit field? A comment about what exactly this does and why would be nice. (If it's a drive strength, are we sure 0x4 will always be the right value for all boards, or should this be passed in from mainboard code?)

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/mt8183.c 
File src/soc/mediatek/mt8183/mt8183.c:

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/mt8183.c@23 
PS2, Line 23: 	gpio_set_spi_driving();
If this is required for SPI flash access, don't we need to set it in the bootblock? This function runs in verstage, we've already read stuff from SPI flash by this point.

[Julius Werner (Code Review)]

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/gpio.c 
File src/soc/mediatek/mt8183/gpio.c:

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/gpio.c@140 
PS2, Line 140: 0x4
> Where does this 0x4 come from? Is this a drive strength? Or a bit field? A comment about what exactl […]

Also, does this affects all SPI controllers or only one?

[Mengqi Zhang (Code Review)]

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

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

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

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

mediatek/mt8183: Add SPI GPIO driving setting

Set SPI GPIO driving to support SPI FLASH.

BUG=b:80501386
BRANCH=none
TEST=emerge-kukui coreboot; emerge-elm coreboot

Change-Id: I95002ec71abd751c33c089185db04ed4a8686699
Signed-off-by: Mengqi Zhang 
---
M src/mainboard/google/kukui/bootblock.c
M src/soc/mediatek/mt8183/gpio.c
M src/soc/mediatek/mt8183/include/soc/gpio.h
M src/soc/mediatek/mt8183/mt8183.c
4 files changed, 52 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/32460/3

[Mengqi Zhang (Code Review)]

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

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

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

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

mediatek/mt8183: Add SPI GPIO driving setting

Set SPI GPIO driving to support SPI FLASH.

BUG=b:80501386
BRANCH=none
TEST=emerge-kukui coreboot; emerge-elm coreboot

Change-Id: I95002ec71abd751c33c089185db04ed4a8686699
Signed-off-by: Mengqi Zhang 
---
M src/mainboard/google/kukui/bootblock.c
M src/soc/mediatek/mt8183/gpio.c
M src/soc/mediatek/mt8183/include/soc/gpio.h
M src/soc/mediatek/mt8183/mt8183.c
4 files changed, 52 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/32460/4

[Mengqi Zhang (Code Review)]

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

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

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

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

mediatek/mt8183: Add SPI GPIO driving setting

Set SPI GPIO driving to support SPI FLASH.

BUG=b:80501386
BRANCH=none
TEST=emerge-kukui coreboot; emerge-elm coreboot

Change-Id: I95002ec71abd751c33c089185db04ed4a8686699
Signed-off-by: Mengqi Zhang 
---
M src/mainboard/google/kukui/bootblock.c
M src/soc/mediatek/mt8183/gpio.c
M src/soc/mediatek/mt8183/include/soc/gpio.h
3 files changed, 51 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/32460/5

[Mengqi Zhang (Code Review)]

Mengqi Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32460 )

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/gpio.c 
File src/soc/mediatek/mt8183/gpio.c:

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/gpio.c@140 
PS2, Line 140: 0x4
> Where does this 0x4 come from? Is this a drive strength? Or a bit field? A comment about what exactl […]

0x4 means 10mA drive strength. This drive strength value depends on the real board design, so it can't be suitable all boards.I have modified this interface, please review the latest patch.

[Mengqi Zhang (Code Review)]

Mengqi Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32460 )

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/mt8183.c 
File src/soc/mediatek/mt8183/mt8183.c:

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/mt8183.c@23 
PS2, Line 23: 	gpio_set_spi_driving();
> If this is required for SPI flash access, don't we need to set it in the bootblock? This function ru […]

Yes, I will call this function in mainboard/kukui/bootblock.

[Julius Werner (Code Review)]

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 5:

(3 comments)

https://review.coreboot.org/#/c/32460/5/src/soc/mediatek/mt8183/gpio.c 
File src/soc/mediatek/mt8183/gpio.c:

https://review.coreboot.org/#/c/32460/5/src/soc/mediatek/mt8183/gpio.c@135 
PS5, Line 135: driving
So this parameter is in milliamps now? Please just call it 'milliamps' or 'drive_milliamps' or something like that to make that clear.

https://review.coreboot.org/#/c/32460/5/src/soc/mediatek/mt8183/gpio.c@137 
PS5, Line 137: 	if (driving < 2 || driving > 16)
This should just be an assert()

https://review.coreboot.org/#/c/32460/5/src/soc/mediatek/mt8183/gpio.c@140 
PS5, Line 140: 	if (bus == 0)
Please use a switch-statement for 'bus'

[Hung-Te Lin (Code Review)]

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/mt8183.c 
File src/soc/mediatek/mt8183/mt8183.c:

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/mt8183.c@23 
PS2, Line 23: 	gpio_set_spi_driving();
> If this is required for SPI flash access, don't we need to set it in the bootblock? This function ru […]

I'm probably fine if this is set only after verstage if that helps reducing bootblock size. We read much more data after that...

If you're moving this to bootblock can you also double check if the size has been increased and how much?

[Mengqi Zhang (Code Review)]

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

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

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

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

mediatek/mt8183: Add SPI GPIO driving setting

Set SPI GPIO driving to support SPI FLASH.

BUG=b:80501386
BRANCH=none
TEST=emerge-kukui coreboot; emerge-elm coreboot

Change-Id: I95002ec71abd751c33c089185db04ed4a8686699
Signed-off-by: Mengqi Zhang 
---
M src/mainboard/google/kukui/bootblock.c
M src/soc/mediatek/mt8183/gpio.c
M src/soc/mediatek/mt8183/include/soc/gpio.h
3 files changed, 59 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/32460/6

[Mengqi Zhang (Code Review)]

Mengqi Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32460 )

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 5:

(3 comments)

https://review.coreboot.org/#/c/32460/5/src/soc/mediatek/mt8183/gpio.c 
File src/soc/mediatek/mt8183/gpio.c:

https://review.coreboot.org/#/c/32460/5/src/soc/mediatek/mt8183/gpio.c@135 
PS5, Line 135: driving
> So this parameter is in milliamps now? Please just call it 'milliamps' or 'drive_milliamps' or somet […]

Done

https://review.coreboot.org/#/c/32460/5/src/soc/mediatek/mt8183/gpio.c@137 
PS5, Line 137: 	if (driving < 2 || driving > 16)
> This should just be an assert()

Done

https://review.coreboot.org/#/c/32460/5/src/soc/mediatek/mt8183/gpio.c@140 
PS5, Line 140: 	if (bus == 0)
> Please use a switch-statement for 'bus'

Done

[Mengqi Zhang (Code Review)]

Mengqi Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32460 )

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/mt8183.c 
File src/soc/mediatek/mt8183/mt8183.c:

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/mt8183.c@23 
PS2, Line 23: 	gpio_set_spi_driving();
> I'm probably fine if this is set only after verstage if that helps reducing bootblock size. […]

I'm trying to reduce the code size, please review the latest patch.

[Julius Werner (Code Review)]

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/#/c/32460/6/src/soc/mediatek/mt8183/gpio.c 
File src/soc/mediatek/mt8183/gpio.c:

https://review.coreboot.org/#/c/32460/6/src/soc/mediatek/mt8183/gpio.c@170 
PS6, Line 170: 	clrsetbits_le32(reg, 0xf << offset, reg_val << offset);
I don't think this works. You would still be running this statement for SPI_PAD1 or bus 2, and in those cases, reg is NULL here.

You could put the code for those special cases directly into the switch-statement and then just return immediately to avoid this problem.

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/mt8183.c 
File src/soc/mediatek/mt8183/mt8183.c:

https://review.coreboot.org/#/c/32460/2/src/soc/mediatek/mt8183/mt8183.c@23 
PS2, Line 23: 	gpio_set_spi_driving();
> I'm trying to reduce the code size, please review the latest patch.

Uhh... unless I'm misunderstanding something here, this is a reliability thing, not a speed thing. Increasing your drive strength doesn't make the SPI run faster, but running it with too low drive strength may make it flip a bit somewhere in every one out of a thousand boots. So no, please really do put this in the bootblock. ;)

If you are really worried about code size, one option would be to throw the whole function into the header and make it static inline. That would be ugly, but it would reduce the size to almost nothing (because the compiler can drop all the stuff for busses it doesn't care about).

[Mengqi Zhang (Code Review)]

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

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

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

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

mediatek/mt8183: Add SPI GPIO driving setting

Set SPI GPIO driving to support SPI FLASH.

BUG=b:80501386
BRANCH=none
TEST=emerge-kukui coreboot; emerge-elm coreboot

Change-Id: I95002ec71abd751c33c089185db04ed4a8686699
Signed-off-by: Mengqi Zhang 
---
M src/mainboard/google/kukui/bootblock.c
M src/soc/mediatek/mt8183/gpio.c
M src/soc/mediatek/mt8183/include/soc/gpio.h
3 files changed, 59 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/32460/7

[Mengqi Zhang (Code Review)]

Mengqi Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32460 )

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/#/c/32460/6/src/soc/mediatek/mt8183/gpio.c 
File src/soc/mediatek/mt8183/gpio.c:

https://review.coreboot.org/#/c/32460/6/src/soc/mediatek/mt8183/gpio.c@170 
PS6, Line 170: 	clrsetbits_le32(reg, 0xf << offset, reg_val << offset);
> I don't think this works. […]

Done

[Mengqi Zhang (Code Review)]

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

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

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

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

mediatek/mt8183: Add SPI GPIO driving setting

Set SPI GPIO driving to support SPI FLASH.

BUG=b:80501386
BRANCH=none
TEST=emerge-kukui coreboot; emerge-elm coreboot

Change-Id: I95002ec71abd751c33c089185db04ed4a8686699
Signed-off-by: Mengqi Zhang 
---
M src/mainboard/google/kukui/bootblock.c
M src/soc/mediatek/mt8183/gpio.c
M src/soc/mediatek/mt8183/include/soc/gpio.h
3 files changed, 60 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/32460/8

[Julius Werner (Code Review)]

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 8: Code-Review+2

[Tristan Hsieh (Code Review)]

Tristan Hsieh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32460 )

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32460/8/src/soc/mediatek/mt8183/gpio.c 
File src/soc/mediatek/mt8183/gpio.c:

PS8: 
Don't change file mode.

[Mengqi Zhang (Code Review)]

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

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

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

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

mediatek/mt8183: Add SPI GPIO driving setting

Set SPI GPIO driving to support SPI FLASH.

BUG=b:80501386
BRANCH=none
TEST=emerge-kukui coreboot; emerge-elm coreboot

Change-Id: I95002ec71abd751c33c089185db04ed4a8686699
Signed-off-by: Mengqi Zhang 
---
M src/mainboard/google/kukui/bootblock.c
M src/soc/mediatek/mt8183/gpio.c
M src/soc/mediatek/mt8183/include/soc/gpio.h
3 files changed, 60 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/32460/9

[Hung-Te Lin (Code Review)]

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 9:

(6 comments)

https://review.coreboot.org/#/c/32460/9/src/mainboard/google/kukui/bootblock... 
File src/mainboard/google/kukui/bootblock.c:

https://review.coreboot.org/#/c/32460/9/src/mainboard/google/kukui/bootblock... 
PS9, Line 24: gpio_set_spi_driving(CONFIG_BOOT_DEVICE_SPI_FLASH_BUS, SPI_PAD0_MASK,
            : 			     10);
please move kukui-specific changes to a standalone one unless if that will break something.

i.e., have one CL to support setting SPI driving for 8183, and another CL to set higher driver in kukui.

also, maybe add some explain for how the 10 is calculated.

https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c 
File src/soc/mediatek/mt8183/gpio.c:

https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c@136 
PS9, Line 136: gpio_set_spi_driving
Is this a per-board design or per-soc logic?

If the settings below are only for Kukui then we should move it to mainboard/kukui.

But if it can be applied to all 8183 family then yes we can put it here.

https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c@143 
PS9, Line 143: ((milliamps >= 2) && (milliamps <= 16));
&& has lower precedence so you don't need to quote, i.e..

assert(milliamps >= 2 && milliamps <= 16)

https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c@162 
PS9, Line 162: 	
should we do some assert here if pad_select is something else?

https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c@181 
PS9, Line 181: }
should we assert on any other values for bus?

https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/include/soc/... 
File src/soc/mediatek/mt8183/include/soc/gpio.h:

https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/include/soc/... 
PS9, Line 622: driving
driving or milliamps ? the prototype should be consistent.

[Hung-Te Lin (Code Review)]

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 9:

@MTK/Mengqi ping?

[Mengqi Zhang (Code Review)]

Mengqi Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32460 )

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 9:

(4 comments)

https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c 
File src/soc/mediatek/mt8183/gpio.c:

https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c@136 
PS9, Line 136: gpio_set_spi_driving
> Is this a per-board design or per-soc logic? […]

This is a per-soc logic, it can be applied to all 8183.

https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c@143 
PS9, Line 143: ((milliamps >= 2) && (milliamps <= 16));
> && has lower precedence so you don't need to quote, i.e.. […]

OK, I will fix it.

https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c@162 
PS9, Line 162:
> should we do some assert here if pad_select is something else?

If we need do this, I think it's better to put it to line144 with other assert together.

https://review.coreboot.org/#/c/32460/9/src/soc/mediatek/mt8183/gpio.c@181 
PS9, Line 181: }
> should we assert on any other values for bus?

I have do it at line142.

[Mengqi Zhang (Code Review)]

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

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

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

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

mediatek/mt8183: Add SPI GPIO driving setting

Set SPI GPIO driving to support SPI FLASH.

BUG=b:80501386
BRANCH=none
TEST=emerge-kukui coreboot; emerge-elm coreboot

Change-Id: I95002ec71abd751c33c089185db04ed4a8686699
Signed-off-by: Mengqi Zhang 
---
M src/mainboard/google/kukui/bootblock.c
M src/soc/mediatek/mt8183/gpio.c
M src/soc/mediatek/mt8183/include/soc/gpio.h
3 files changed, 61 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/32460/10

[Hung-Te Lin (Code Review)]

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 10: Code-Review+1

@Julius do you want to take a final look?

[Julius Werner (Code Review)]

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

Patch Set 10: Code-Review+2

[Patrick Georgi (Code Review)]

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

Change subject: mediatek/mt8183: Add SPI GPIO driving setting
......................................................................

mediatek/mt8183: Add SPI GPIO driving setting

Set SPI GPIO driving to support SPI FLASH.

BUG=b:80501386
BRANCH=none
TEST=emerge-kukui coreboot; emerge-elm coreboot

Change-Id: I95002ec71abd751c33c089185db04ed4a8686699
Signed-off-by: Mengqi Zhang 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32460
Reviewed-by: Hung-Te Lin 
Reviewed-by: Julius Werner 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/google/kukui/bootblock.c
M src/soc/mediatek/mt8183/gpio.c
M src/soc/mediatek/mt8183/include/soc/gpio.h
3 files changed, 61 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/mainboard/google/kukui/bootblock.c b/src/mainboard/google/kukui/bootblock.c
index 9d6c38b..9a7e71c 100644
--- a/src/mainboard/google/kukui/bootblock.c
+++ b/src/mainboard/google/kukui/bootblock.c
@@ -15,9 +15,12 @@

#include <bootblock_common.h>
 #include <soc/spi.h>
+#include <soc/gpio.h>

void bootblock_mainboard_init(void)
 {
    mtk_spi_init(CONFIG_EC_GOOGLE_CHROMEEC_SPI_BUS, SPI_PAD0_MASK, 6 * MHz);
    mtk_spi_init(CONFIG_BOOT_DEVICE_SPI_FLASH_BUS, SPI_PAD0_MASK, 26 * MHz);
+	gpio_set_spi_driving(CONFIG_BOOT_DEVICE_SPI_FLASH_BUS, SPI_PAD0_MASK,
+			     10);
 }
diff --git a/src/soc/mediatek/mt8183/gpio.c b/src/soc/mediatek/mt8183/gpio.c
index 327d389..3eccfbd 100644
--- a/src/soc/mediatek/mt8183/gpio.c
+++ b/src/soc/mediatek/mt8183/gpio.c
@@ -15,11 +15,15 @@

#include <device/mmio.h>
 #include <gpio.h>
+#include <assert.h>
+#include <soc/spi.h>

enum {
    EN_OFFSET = 0x60,
    SEL_OFFSET = 0x80,
    EH_RSEL_OFFSET = 0xF0,
+	GPIO_DRV0_OFFSET = 0xA0,
+	GPIO_DRV1_OFFSET = 0XB0,
 };

static void gpio_set_pull_pupd(gpio_t gpio, enum pull_enable enable,
@@ -128,3 +132,54 @@
    	I2C_EH_RSL_MASK(SCL5) | I2C_EH_RSL_MASK(SDA5),
    	I2C_EH_RSL_VAL(SCL5) | I2C_EH_RSL_VAL(SDA5));
 }
+
+void gpio_set_spi_driving(unsigned int bus, enum spi_pad_mask pad_select,
+			  unsigned int milliamps)
+{
+	void *reg = NULL;
+	unsigned int reg_val = milliamps / 2 - 1, offset = 0;
+
+	assert(bus < SPI_BUS_NUMBER);
+	assert(milliamps >= 2 && milliamps <= 16);
+	assert(pad_select <= SPI_PAD1_MASK);
+
+	switch (bus) {
+	case 0:
+		reg = (void *)(IOCFG_RB_BASE + GPIO_DRV1_OFFSET);
+		offset = 0;
+		break;
+	case 1:
+		if (pad_select == SPI_PAD0_MASK) {
+			reg = (void *)(IOCFG_LM_BASE + GPIO_DRV0_OFFSET);
+			offset = 0;
+		} else if (pad_select == SPI_PAD1_MASK) {
+			clrsetbits_le32((void *)IOCFG_RM_BASE +
+					GPIO_DRV0_OFFSET, 0xf | 0xf << 20,
+					reg_val | reg_val << 20);
+			clrsetbits_le32((void *)IOCFG_RM_BASE +
+					GPIO_DRV1_OFFSET, 0xf << 16,
+					reg_val << 16);
+			return;
+		}
+		break;
+	case 2:
+		clrsetbits_le32((void *)IOCFG_RM_BASE + GPIO_DRV0_OFFSET,
+				0xf << 8 | 0xf << 12,
+				reg_val << 8 | reg_val << 12);
+		return;
+	case 3:
+		reg = (void *)(IOCFG_LM_BASE + GPIO_DRV0_OFFSET);
+		offset = 16;
+		break;
+	case 4:
+		reg = (void *)(IOCFG_LM_BASE + GPIO_DRV0_OFFSET);
+		offset = 12;
+		break;
+	case 5:
+		reg = (void *)(IOCFG_LM_BASE + GPIO_DRV0_OFFSET);
+		offset = 8;
+		break;
+	}
+
+	clrsetbits_le32(reg, 0xf << offset, reg_val << offset);
+}
diff --git a/src/soc/mediatek/mt8183/include/soc/gpio.h b/src/soc/mediatek/mt8183/include/soc/gpio.h
index 5a98953..a0d6262 100644
--- a/src/soc/mediatek/mt8183/include/soc/gpio.h
+++ b/src/soc/mediatek/mt8183/include/soc/gpio.h
@@ -19,6 +19,7 @@
 #include <soc/addressmap.h>
 #include <soc/gpio_common.h>
 #include <types.h>
+#include <soc/spi_common.h>

enum {
    MAX_GPIO_REG_BITS = 32,
@@ -617,5 +618,7 @@

static struct gpio_regs *const mtk_gpio = (void *)(GPIO_BASE);
 void gpio_set_i2c_eh_rsel(void);
+void gpio_set_spi_driving(unsigned int bus, enum spi_pad_mask pad_select,
+			  unsigned int milliamps);

#endif