Change in ...coreboot[master]: src/mainboard/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFO...

[Mike Banon (Code Review)]

Mike Banon has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33919

Change subject: src/mainboard/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

src/mainboard/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP

Original AMD_PLATFORM_MOBILE is incorrect because this board is a desktop one.

Signed-off-by: Mike Banon 
Change-Id: I02adedffe8624c38e7b93fadd0449ddf094388fd
---
M src/mainboard/asus/am1i-a/buildOpts.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/33919/1

diff --git a/src/mainboard/asus/am1i-a/buildOpts.c b/src/mainboard/asus/am1i-a/buildOpts.c
index cba0894..98bc620 100644
--- a/src/mainboard/asus/am1i-a/buildOpts.c
+++ b/src/mainboard/asus/am1i-a/buildOpts.c
@@ -115,7 +115,7 @@
 #define BLDCFG_CORE_LEVELING_MODE                CORE_LEVEL_LOWEST
 #define BLDCFG_MEM_INIT_PSTATE                   0

-#define BLDCFG_AMD_PLATFORM_TYPE                  AMD_PLATFORM_MOBILE
+#define BLDCFG_AMD_PLATFORM_TYPE                  AMD_PLATFORM_DESKTOP

#define BLDCFG_MEMORY_BUS_FREQUENCY_LIMIT         DDR1600_FREQUENCY
 #define BLDCFG_MEMORY_MODE_UNGANGED               TRUE

[Mike Banon (Code Review)]

Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33919 )

Change subject: src/mainboard/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

Patch Set 1:

am1i-a/buildOpts.c patch series [7/8] : 1st = CB:33913 , 2nd = CB:33914 , 3rd = CB:33915 , 4th = CB:33916 , 5th = CB:33917 , 6th = CB:33918 ; 7th = CB:33919 <-- you are here ; 8th = CB:33920

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33919 )

Change subject: src/mainboard/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

Patch Set 1: Code-Review+1

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33919 )

Change subject: src/mainboard/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33919/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33919/1//COMMIT_MSG@9 
PS1, Line 9: incorrect
This option seems to have some effect in the AGESA code on PCIe configuration (look for GfxCardWorkaround). Can you state why you think this change is needed, since it's not a cosmetic change?

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33919 )

Change subject: src/mainboard/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33919/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33919/1//COMMIT_MSG@9 
PS1, Line 9: desktop
But a mobile processor, right?

[Mike Banon (Code Review)]

Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33919 )

Change subject: src/mainboard/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/33919/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/33919/1//COMMIT_MSG@9 
PS1, Line 9: incorrect
> This option seems to have some effect in the AGESA code on PCIe configuration (look for GfxCardWorka […]

Most mobile boards do not have a PCIe slot (except a MiniPCIe x1 usually reserved for WiFi card) so this workaround is disabled for them because they don't need it. But this MiniITX desktop board has a PCIe slot, so it might benefit from this workaround. Changing to AMD_PLATFORM_DESKTOP will enable some workaround code at ./src/vendorcode/amd/agesa/f16kb/Proc/GNB/Modules/GnbPcieTrainingV2/PcieTrainingV2.c , line 500 :  https://review.coreboot.org/cgit/coreboot.git/tree/src/vendorcode/amd/agesa/...

https://review.coreboot.org/#/c/33919/1//COMMIT_MSG@9 
PS1, Line 9: desktop
> But a mobile processor, right?

CPU itself is a desktop one, Athlon 5370. It has a low TDP of 25W but couldn't be found at laptops.

Change in coreboot[master]: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP

[Mike Banon (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP

Original AMD_PLATFORM_MOBILE is incorrect because this board is a desktop one.

Signed-off-by: Mike Banon 
Change-Id: I02adedffe8624c38e7b93fadd0449ddf094388fd
---
M src/mainboard/asus/am1i-a/buildOpts.c
1 file changed, 1 insertion(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/33919/6

Change in coreboot[master]: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP

[Mike Banon (Code Review)]

Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33919 )

Change subject: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

Patch Set 6:

Different SHA256 as expected.

Change in coreboot[master]: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33919 )

Change subject: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/33919/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/33919/6//COMMIT_MSG@9 
PS6, Line 9: Original AMD_PLATFORM_MOBILE is incorrect because this board is a desktop one.
If this is incorrect, what does changing it improve?

Change in coreboot[master]: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP

[Mike Banon (Code Review)]

Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33919 )

Change subject: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/33919/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/33919/6//COMMIT_MSG@9 
PS6, Line 9: Original AMD_PLATFORM_MOBILE is incorrect because this board is a desktop one.
> If this is incorrect, what does changing it improve?

There is AMD vendorcode which is active only if AMD_PLATFORM_MOBILE is enabled:

./src/vendorcode/amd/agesa/f16kb/Proc/GNB/Modules/GnbInitKB/GfxIntegratedInfoTableKB.c:467:        // GPUCAPINFO_DFS_BYPASS_ENABLE should be enabled by default for MOBILE systems
    if ((Gfx->AmdPlatformType & AMD_PLATFORM_MOBILE) != 0) {
      SystemInfoTableV3.sIntegratedSysInfo.ulGPUCapInfo |= GPUCAPINFO_DFS_BYPASS_ENABLE;
    }

./src/vendorcode/amd/agesa/f16kb/Proc/GNB/Modules/GnbPcieConfig/PcieConfigData.c:253:    if ((UserOptions.CfgAmdPlatformType  & AMD_PLATFORM_MOBILE) != 0) {
    Pcie->GfxCardWorkaround = GfxWorkaroundDisable;
  }

./src/vendorcode/amd/agesa/f16kb/Proc/GNB/Modules/GnbGfxIntTableV3/GfxPwrPlayTable.c:1058:    if ((Gfx->AmdPlatformType & AMD_PLATFORM_MOBILE) != 0) {
    PpWorkspace.PpTable->ulPlatformCaps |= ATOM_PP_PLATFORM_CAP_POWERPLAY;
  }

Change in coreboot[master]: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33919 )

Change subject: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/33919/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/33919/6//COMMIT_MSG@9 
PS6, Line 9: Original AMD_PLATFORM_MOBILE is incorrect because this board is a desktop one.
> There is AMD vendorcode which is active only if AMD_PLATFORM_MOBILE is enabled: […]

Is there a noticeable improvement, though? Sorry if I wasn't clear enough before.

Change in coreboot[master]: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP

[Michał Żygowski (Code Review)]

Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33919 )

Change subject: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

Patch Set 8: Code-Review+2

Change in coreboot[master]: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP

[Mike Banon (Code Review)]

Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33919 )

Change subject: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/33919/6//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/33919/6//COMMIT_MSG@9 
PS6, Line 9: Original AMD_PLATFORM_MOBILE is incorrect because this board is a desktop one.
> Is there a noticeable improvement, though? Sorry if I wasn't clear enough before.

Improvement could be the higher performance of integrated graphics at the expense of slightly higher power consumption, if AMD PowerPlay turns out as disabled, but since this is a mini desktop board with APU's TDP 25W - this improvement is hardly noticeable to be honest.

There's also GfxCardWorkaround code at ./coreboot/src/vendorcode/amd/agesa/f16kb/Proc/GNB/Modules/PcieTrainingV2.c and ./GnbPcieTrainingV2/PcieWorkaroundsV2.c , which seems to issue PCIe reset up to 5 times if there's init problem - but my iGPU is working fine even without it.

Change in coreboot[master]: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/33919 )

Change subject: mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP
......................................................................

mb/asus/am1i-a/buildOpts.c: set a board type to AMD_PLATFORM_DESKTOP

Original AMD_PLATFORM_MOBILE is incorrect because this board is a desktop one.

Signed-off-by: Mike Banon 
Change-Id: I02adedffe8624c38e7b93fadd0449ddf094388fd
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33919
Reviewed-by: Michał Żygowski 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/asus/am1i-a/buildOpts.c
1 file changed, 1 insertion(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Michał Żygowski: Looks good to me, approved

diff --git a/src/mainboard/asus/am1i-a/buildOpts.c b/src/mainboard/asus/am1i-a/buildOpts.c
index 71b1815f..71fedaa 100644
--- a/src/mainboard/asus/am1i-a/buildOpts.c
+++ b/src/mainboard/asus/am1i-a/buildOpts.c
@@ -99,7 +99,7 @@
 #define BLDCFG_CORE_LEVELING_MODE                CORE_LEVEL_LOWEST
 #define BLDCFG_MEM_INIT_PSTATE                   0

-#define BLDCFG_AMD_PLATFORM_TYPE                  AMD_PLATFORM_MOBILE
+#define BLDCFG_AMD_PLATFORM_TYPE                  AMD_PLATFORM_DESKTOP

#define BLDCFG_MEMORY_BUS_FREQUENCY_LIMIT         DDR1600_FREQUENCY
 #define BLDCFG_MEMORY_MODE_UNGANGED               TRUE