Change in coreboot[master]: mb/pcengines/apu1/mainboard.c: fix apu1 serial number counting
Piotr Kleinschmidt has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: fix apu1 serial number counting ......................................................................
mb/pcengines/apu1/mainboard.c: fix apu1 serial number counting
PC Engines apu1 platform returns serial number value as -64. It is caused by faulty read of NIC's registers. This patch adjust NIC's bus number to valid value, so NIC's registers can be correctly read and hence NIC's MAC address.
TEST=`dmidecode -t 2` command in Linux Debian
Signed-off-by: Piotr Kleinschmidt piotr.kleinschmidt@3mdeb.com Change-Id: I5e8690d100b38ac7889395d375c0ff32bdefda0b --- M src/mainboard/pcengines/apu1/mainboard.c 1 file changed, 10 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/42512/1
diff --git a/src/mainboard/pcengines/apu1/mainboard.c b/src/mainboard/pcengines/apu1/mainboard.c index a2a78c5..b297061 100644 --- a/src/mainboard/pcengines/apu1/mainboard.c +++ b/src/mainboard/pcengines/apu1/mainboard.c @@ -299,9 +299,18 @@ if (!dev) return serial;
+ /* FIXME: dev->bus->secondary has 0x100 value, while it should + * has 0x001 value, as it is on PCI bridge 1. + * Without this workaround pci_read_config32(dev, 0x18) returns + * incorrect data and hence platform's serial number is incorrect. + * However, shifting dev->bus->secondary 8 bits right is not + * reliable solution, as the problem probably lies earlier. + */ + dev->bus->secondary >>= 8; + /* Read in the last 3 bytes of NIC's MAC address. */ bar18 = pci_read_config32(dev, 0x18); - bar18 &= 0xFFFFFC00; + bar18 &= 0xFFFFFFF0; for (i = 3; i < 6; i++) { mac_addr <<= 8; mac_addr |= read8((u8 *)bar18 + i);
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: fix apu1 serial number counting ......................................................................
Patch Set 1:
The same approach to obtaining the correct NIC PCI device is used in PC Engines apu2. Why the same code doesn't work here? Any ideas?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: fix apu1 serial number counting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42512/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42512/1/src/mainboard/pcengines/apu... PS1, Line 303: has have
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: fix apu1 serial number counting ......................................................................
Patch Set 1:
While this woraround might work, it seems to be solving the wrong problem and being possibly unreliable to me. Might this be related to the resource allocator changes?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: fix apu1 serial number counting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42512/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42512/1/src/mainboard/pcengines/apu... PS1, Line 4: #include <AGESA.h> git grep "Porting.h"
/* warning: Porting.h includes an open #pragma pack(1) */
If at all possible, avoid <AGESA.h> and <AMD.h> includes. If you need it, place them as the last include.
6ebb739 mb/pcengines/apu1/mainboard.c: Add SMBIOS type 16 and 17 entries
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: fix apu1 serial number counting ......................................................................
Patch Set 1:
Patch Set 1:
While this woraround might work, it seems to be solving the wrong problem and being possibly unreliable to me. Might this be related to the resource allocator changes?
To be honest, I don't know what is the source of original problem, so this workaround is done only to fix serial number issue. I will just add that every NIC shows this problem - secondary bus number is shifted 8 bits left. That's why I suggested that it might be related to enumeration phase already. What exactly do you mean by resource allocator changes?
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: fix apu1 serial number counting ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
While this woraround might work, it seems to be solving the wrong problem and being possibly unreliable to me. Might this be related to the resource allocator changes?
To be honest, I don't know what is the source of original problem, so this workaround is done only to fix serial number issue. I will just add that every NIC shows this problem - secondary bus number is shifted 8 bits left. That's why I suggested that it might be related to enumeration phase already. What exactly do you mean by resource allocator changes?
Many changes has been introduce to the coreboot's resource allocator (the code that enumerates devices, assigng resources for MMIO, IO, etc.). I would try to move the AGESA header includes as Kyösti suggests to see if it helps.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: fix apu1 serial number counting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42512/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42512/1/src/mainboard/pcengines/apu... PS1, Line 4: #include <AGESA.h>
git grep "Porting.h" […]
Piotr, try this out to see if it helps
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: fix apu1 serial number counting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42512/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42512/1/src/mainboard/pcengines/apu... PS1, Line 4: #include <AGESA.h>
Piotr, try this out to see if it helps
Tried this approach, but it doesn't even compile then. No matter if it is deleted or moved at the end of all includes.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: fix apu1 serial number counting ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42512/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42512/1/src/mainboard/pcengines/apu... PS1, Line 4: #include <AGESA.h>
Tried this approach, but it doesn't even compile then. […]
Could you track what is it used for?
Hello build bot (Jenkins), Nico Huber, Michał Żygowski, Angel Pons, Arthur Heymans, Aaron Durbin, Felix Held, Piotr Król,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42512
to look at the new patch set (#2).
Change subject: mb/pcengines/apu1/mainboard.c: fix building order and resource allocation ......................................................................
mb/pcengines/apu1/mainboard.c: fix building order and resource allocation
Originally, there was problem with PC Engines apu1 platform which returned serial number value as -64. It was caused by faulty resource allocation and/or enumeration. This patch reorder includes and put <AGESA.h> and <AMD.h> at the end of list. They can't be deleted completely. As a result bus number value in device's structure is correct and hence serial number.
TEST=`dmidecode -t 2` command in Linux Debian
Signed-off-by: Piotr Kleinschmidt piotr.kleinschmidt@3mdeb.com Change-Id: I5e8690d100b38ac7889395d375c0ff32bdefda0b --- M src/mainboard/pcengines/apu1/mainboard.c 1 file changed, 5 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/42512/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: fix building order and resource allocation ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42512/2//COMMIT_MSG@11 PS2, Line 11: resource allocation and/or enumeration. Nothing wrong with the resource allocation and enumeration. Please mention the #pragma Porting.h I mentioned earlier.
What happened is mainboard.c file used different binary layout of 'struct device' because of the attribute((packed)) and the reference dev->bus->secondary landed at wrong memory address.
Hello build bot (Jenkins), Nico Huber, Michał Żygowski, Angel Pons, Arthur Heymans, Aaron Durbin, Felix Held, Piotr Król,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42512
to look at the new patch set (#3).
Change subject: mb/pcengines/apu1/mainboard.c: reorder includes ......................................................................
mb/pcengines/apu1/mainboard.c: reorder includes
Originally, there was problem with PC Engines apu1 platform which returned serial number value as -64. It was caused by wrong value of dev->bus->secondary. Source of the problem is in Porting.h header file. It contains '#pragma pack(1)' which affects struct device. As mainboard.c uses different binary layout because of this attribute, reference dev->bus->secondary lands at wrong memory address. This patch reorder includes and put <AGESA.h> and <AMD.h> at the end of list, making struct device consistent. As a result bus number value in device's structure is correct and hence serial number.
TEST=`dmidecode -t 2` command in Linux Debian
Signed-off-by: Piotr Kleinschmidt piotr.kleinschmidt@3mdeb.com Change-Id: I5e8690d100b38ac7889395d375c0ff32bdefda0b --- M src/mainboard/pcengines/apu1/mainboard.c 1 file changed, 5 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/42512/3
Piotr Kleinschmidt has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: reorder includes ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42512/2//COMMIT_MSG@11 PS2, Line 11: resource allocation and/or enumeration.
Nothing wrong with the resource allocation and enumeration. Please mention the #pragma Porting. […]
I have reworded commit message, as you suggested.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: reorder includes ......................................................................
Patch Set 3: Code-Review+2
(3 comments)
https://review.coreboot.org/c/coreboot/+/42512/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42512/2//COMMIT_MSG@11 PS2, Line 11: resource allocation and/or enumeration.
I have reworded commit message, as you suggested.
Ack
https://review.coreboot.org/c/coreboot/+/42512/1/src/mainboard/pcengines/apu... File src/mainboard/pcengines/apu1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/42512/1/src/mainboard/pcengines/apu... PS1, Line 4: #include <AGESA.h>
Could you track what is it used for?
Ack
https://review.coreboot.org/c/coreboot/+/42512/1/src/mainboard/pcengines/apu... PS1, Line 303: has
have
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: reorder includes ......................................................................
Patch Set 3: Code-Review+1
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42512 )
Change subject: mb/pcengines/apu1/mainboard.c: reorder includes ......................................................................
mb/pcengines/apu1/mainboard.c: reorder includes
Originally, there was problem with PC Engines apu1 platform which returned serial number value as -64. It was caused by wrong value of dev->bus->secondary. Source of the problem is in Porting.h header file. It contains '#pragma pack(1)' which affects struct device. As mainboard.c uses different binary layout because of this attribute, reference dev->bus->secondary lands at wrong memory address. This patch reorder includes and put <AGESA.h> and <AMD.h> at the end of list, making struct device consistent. As a result bus number value in device's structure is correct and hence serial number.
TEST=`dmidecode -t 2` command in Linux Debian
Signed-off-by: Piotr Kleinschmidt piotr.kleinschmidt@3mdeb.com Change-Id: I5e8690d100b38ac7889395d375c0ff32bdefda0b Reviewed-on: https://review.coreboot.org/c/coreboot/+/42512 Reviewed-by: Michał Żygowski michal.zygowski@3mdeb.com Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/pcengines/apu1/mainboard.c 1 file changed, 5 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, but someone else must approve Michał Żygowski: Looks good to me, approved
diff --git a/src/mainboard/pcengines/apu1/mainboard.c b/src/mainboard/pcengines/apu1/mainboard.c index a2a78c5..45d326a 100644 --- a/src/mainboard/pcengines/apu1/mainboard.c +++ b/src/mainboard/pcengines/apu1/mainboard.c @@ -1,8 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <amdblocks/acpimmio.h> -#include <AGESA.h> -#include <AMD.h> + #include <console/console.h> #include <device/device.h> #include <device/mmio.h> @@ -11,12 +10,14 @@ #include <southbridge/amd/common/amd_pci_util.h> #include <smbios.h> #include <string.h> -#include <southbridge/amd/cimx/sb800/SBPLATFORM.h> #include <southbridge/amd/cimx/sb800/pci_devs.h> #include <northbridge/amd/agesa/agesa_helper.h> #include <northbridge/amd/agesa/family14/pci_devs.h> #include <superio/nuvoton/nct5104d/nct5104d.h> #include "gpio_ftns.h" +#include <AGESA.h> +#include <AMD.h> +#include <southbridge/amd/cimx/sb800/SBPLATFORM.h>
/*********************************************************** * These arrays set up the FCH PCI_INTR registers 0xC00/0xC01. @@ -301,7 +302,7 @@
/* Read in the last 3 bytes of NIC's MAC address. */ bar18 = pci_read_config32(dev, 0x18); - bar18 &= 0xFFFFFC00; + bar18 &= 0xFFFFFFF0; for (i = 3; i < 6; i++) { mac_addr <<= 8; mac_addr |= read8((u8 *)bar18 + i);
-
Felix Held (Code Review) -
Kyösti Mälkki (Code Review)
-
Michał Żygowski (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)
-
Piotr Kleinschmidt (Code Review)