Change in coreboot[master]: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value

[CK HU (Code Review)]

CK HU has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44452 )

Change subject: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value
......................................................................

mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value

The BOOT_DEVICE_SPI_FLASH_BUS default value is 0.

Signed-off-by: CK Hu 
Change-Id: Ibc269201a34968c8400d2235e8da2ecd88114975
---
M src/mainboard/google/asurada/Kconfig
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/44452/1

diff --git a/src/mainboard/google/asurada/Kconfig b/src/mainboard/google/asurada/Kconfig
index e1c96f0..e3b30a3 100644
--- a/src/mainboard/google/asurada/Kconfig
+++ b/src/mainboard/google/asurada/Kconfig
@@ -43,7 +43,7 @@

config BOOT_DEVICE_SPI_FLASH_BUS
    int
-	default 1
+	default 0

config EC_GOOGLE_CHROMEEC_SPI_BUS
    hex

[Hung-Te Lin (Code Review)]

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

Change subject: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44452/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44452/1//COMMIT_MSG@8 
PS1, Line 8: 
           : The BOOT_DEVICE_SPI_FLASH_BUS default value is 0.
Asurada is using NOR SPI flash on bus 0.

[Hung-Te Lin (Code Review)]

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

Change subject: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

Are you sure this is really going to work?

The spi_ctrlr_bus_map works by matching bus number. If you define flash_bus to 0, then then bus_map cannot identify if you are going to init SPI NOR or SPI.

Take a look at 8173, they use the trick: (8173 uses TPM on I2C so there's only one SPI for EC):

CONFIG_SPI_BUS_NUMBER = 1
 CONFIG_BOOT_DEVICE_SPI_FLASH_BUS = 9

const struct spi_ctrlr_buses spi_ctrlr_bus_map[] = {
        {
                .ctrlr = &spi_ctrlr,
                .bus_start = 0,
                .bus_end = SPI_BUS_NUMBER - 1,
        },
        {
                .ctrlr = &spi_flash_ctrlr,
                .bus_start = CONFIG_BOOT_DEVICE_SPI_FLASH_BUS,
                .bus_end = CONFIG_BOOT_DEVICE_SPI_FLASH_BUS,
        },
 };

So the CONFIG_BOOT_DEVICE_SPI_FLASH_BUS is a virtual number for targeting SPI-NOR.

As a result, I think the right config for 8192 is

https://review.coreboot.org/c/coreboot/+/44452/1/src/mainboard/google/asurad... 
File src/mainboard/google/asurada/Kconfig:

https://review.coreboot.org/c/coreboot/+/44452/1/src/mainboard/google/asurad... 
PS1, Line 46: 0
Are you sure this is really going to work?

The spi_ctrlr_bus_map works by matching bus number. If you define flash_bus to 0, then then bus_map cannot identify if you are going to init SPI NOR or SPI.

Take a look at 8173, they use the trick: (8173 uses TPM on I2C so there's only one SPI for EC):

SPI_BUS_NUMBER = 1
 CONFIG_BOOT_DEVICE_SPI_FLASH_BUS = 9

const struct spi_ctrlr_buses spi_ctrlr_bus_map[] = {
        {
                .ctrlr = &spi_ctrlr,
                .bus_start = 0,
                .bus_end = SPI_BUS_NUMBER - 1,
        },
        {
                .ctrlr = &spi_flash_ctrlr,
                .bus_start = CONFIG_BOOT_DEVICE_SPI_FLASH_BUS,
                .bus_end = CONFIG_BOOT_DEVICE_SPI_FLASH_BUS,
        },
 };

So the CONFIG_BOOT_DEVICE_SPI_FLASH_BUS is a virtual number for targeting SPI-NOR.

As a result, I think the right config for 8192 is to follow same rule, and add a comment explaining that:

# On MT8192 the SPI flash is actually using a SPI-NOR controller with its own bus.
 # The number here should be a virtual value as (SPI_BUS_NUMBER + 1).
 default 9

[CK HU (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Yu-Ping Wu,

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

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

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

Change subject: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value
......................................................................

mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value

On MT8192 the SPI flash is actually using a SPI-NOR controller with
its own bus. The number here should be a virtual value as
(SPI_BUS_NUMBER + 1).

Signed-off-by: CK Hu 
Change-Id: Ibc269201a34968c8400d2235e8da2ecd88114975
---
M src/mainboard/google/asurada/Kconfig
1 file changed, 3 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/44452/2

[CK HU (Code Review)]

CK HU has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44452 )

Change subject: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value
......................................................................

Patch Set 2:
> Patch Set 1: Code-Review-1
>
> (1 comment)
>
> Are you sure this is really going to work?
>
> The spi_ctrlr_bus_map works by matching bus number. If you define flash_bus to 0, then then bus_map cannot identify if you are going to init SPI NOR or SPI.
>
> Take a look at 8173, they use the trick: (8173 uses TPM on I2C so there's only one SPI for EC):
>
> CONFIG_SPI_BUS_NUMBER = 1
>  CONFIG_BOOT_DEVICE_SPI_FLASH_BUS = 9
>
> const struct spi_ctrlr_buses spi_ctrlr_bus_map[] = {
>         {
>                 .ctrlr = &spi_ctrlr,
>                 .bus_start = 0,
>                 .bus_end = SPI_BUS_NUMBER - 1,
>         },
>         {
>                 .ctrlr = &spi_flash_ctrlr,
>                 .bus_start = CONFIG_BOOT_DEVICE_SPI_FLASH_BUS,
>                 .bus_end = CONFIG_BOOT_DEVICE_SPI_FLASH_BUS,
>         },
>  };
>
> So the CONFIG_BOOT_DEVICE_SPI_FLASH_BUS is a virtual number for targeting SPI-NOR.
>
> As a result, I think the right config for 8192 is

Done

[Hung-Te Lin (Code Review)]

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

Change subject: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

just one nit

https://review.coreboot.org/c/coreboot/+/44452/2/src/mainboard/google/asurad... 
File src/mainboard/google/asurada/Kconfig:

https://review.coreboot.org/c/coreboot/+/44452/2/src/mainboard/google/asurad... 
PS2, Line 45: 
            : # On MT8192 the SPI flash is actually using a SPI-NOR controller with its own bus.
            : # The number here should be a virtual value as (SPI_BUS_NUMBER + 1).
Better to put the comment above the config, e.g.

# On MT8192 ...
 # The number ...
 config BOOT_DEVICE
         int
         default 9

[CK HU (Code Review)]

Hello Hung-Te Lin, build bot (Jenkins), Yu-Ping Wu,

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

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

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

Change subject: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value
......................................................................

mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value

On MT8192 the SPI flash is actually using a SPI-NOR controller with
its own bus. The number here should be a virtual value as
(SPI_BUS_NUMBER + 1).

Signed-off-by: CK Hu 
Change-Id: Ibc269201a34968c8400d2235e8da2ecd88114975
---
M src/mainboard/google/asurada/Kconfig
1 file changed, 3 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/52/44452/3

[CK HU (Code Review)]

CK HU has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44452 )

Change subject: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44452/2/src/mainboard/google/asurad... 
File src/mainboard/google/asurada/Kconfig:

https://review.coreboot.org/c/coreboot/+/44452/2/src/mainboard/google/asurad... 
PS2, Line 45: 
            : # On MT8192 the SPI flash is actually using a SPI-NOR controller with its own bus.
            : # The number here should be a virtual value as (SPI_BUS_NUMBER + 1).
> Better to put the comment above the config, e.g. […]

Done

[CK HU (Code Review)]

CK HU has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44452 )

Change subject: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44452/1/src/mainboard/google/asurad... 
File src/mainboard/google/asurada/Kconfig:

https://review.coreboot.org/c/coreboot/+/44452/1/src/mainboard/google/asurad... 
PS1, Line 46: 0
> Are you sure this is really going to work? […]

Done

[Hung-Te Lin (Code Review)]

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

Change subject: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value
......................................................................

Patch Set 3: Code-Review+2

[Hung-Te Lin (Code Review)]

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

Change subject: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44452/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/44452/1//COMMIT_MSG@8 
PS1, Line 8: 
           : The BOOT_DEVICE_SPI_FLASH_BUS default value is 0.
> Asurada is using NOR SPI flash on bus 0.

Ack

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44452 )

Change subject: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value
......................................................................

Patch Set 3: Code-Review+1

[Hung-Te Lin (Code Review)]

Hung-Te Lin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44452 )

Change subject: mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value
......................................................................

mb/google/asurada: Fixup BOOT_DEVICE_SPI_FLASH_BUS default value

On MT8192 the SPI flash is actually using a SPI-NOR controller with
its own bus. The number here should be a virtual value as
(SPI_BUS_NUMBER + 1).

Signed-off-by: CK Hu 
Change-Id: Ibc269201a34968c8400d2235e8da2ecd88114975
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44452
Tested-by: build bot (Jenkins) 
Reviewed-by: Hung-Te Lin 
Reviewed-by: Yu-Ping Wu 
---
M src/mainboard/google/asurada/Kconfig
1 file changed, 3 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Hung-Te Lin: Looks good to me, approved
  Yu-Ping Wu: Looks good to me, but someone else must approve

diff --git a/src/mainboard/google/asurada/Kconfig b/src/mainboard/google/asurada/Kconfig
index e1c96f0..f5ffb3c 100644
--- a/src/mainboard/google/asurada/Kconfig
+++ b/src/mainboard/google/asurada/Kconfig
@@ -41,9 +41,11 @@
    hex
    default 0x0

+# On MT8192 the SPI flash is actually using a SPI-NOR controller with its own bus.
+# The number here should be a virtual value as (SPI_BUS_NUMBER + 1).
 config BOOT_DEVICE_SPI_FLASH_BUS
    int
-	default 1
+	default 9

config EC_GOOGLE_CHROMEEC_SPI_BUS
    hex