Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Build hwm.c for all stages ......................................................................
superio/nuvoton: Build hwm.c for all stages
There's no reason to restrict `nuvoton_hwm_select_bank` to ramstage.
Change-Id: I103a4ea4cef24844d382854c9358bbb37d229e04 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/superio/nuvoton/Makefile.inc 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/42130/1
diff --git a/src/superio/nuvoton/Makefile.inc b/src/superio/nuvoton/Makefile.inc index 054c7ba..0d5d0f5 100644 --- a/src/superio/nuvoton/Makefile.inc +++ b/src/superio/nuvoton/Makefile.inc @@ -5,7 +5,7 @@ romstage-$(CONFIG_SUPERIO_NUVOTON_COMMON_PRE_RAM) += common/early_serial.c
## include generic Nuvoton HWM driver -ramstage-$(CONFIG_SUPERIO_NUVOTON_COMMON_HWM) += common/hwm.c +all-$(CONFIG_SUPERIO_NUVOTON_COMMON_HWM) += common/hwm.c
subdirs-$(CONFIG_SUPERIO_NUVOTON_WPCM450) += wpcm450 subdirs-$(CONFIG_SUPERIO_NUVOTON_NCT5104D) += nct5104d
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Build hwm.c for all stages ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42130/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42130/1//COMMIT_MSG@9 PS1, Line 9: There's no reason Please, is there a reason to use it elsewhere?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Build hwm.c for all stages ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42130/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42130/1//COMMIT_MSG@9 PS1, Line 9: There's no reason
Please, is there a reason to use it elsewhere?
Yes, see the follow-up commits. Among others, I want to use it on bootblock and romstage as well.
Or, since it is a small function, I might as well make it inline. Which do you prefer?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Build hwm.c for all stages ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42130/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42130/1//COMMIT_MSG@9 PS1, Line 9: There's no reason
Yes, see the follow-up commits. Among others, I want to use it on bootblock and romstage as well. […]
IMO, HWM configuration is best done in the ramstage via devicetree. Any particular reason to do it early?
I've peeked ahead and the code looks almost ready for that, all you'd need is a little shim between dt and your functions in some .init function.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Build hwm.c for all stages ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42130/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42130/1//COMMIT_MSG@9 PS1, Line 9: There's no reason
IMO, HWM configuration is best done in the ramstage via devicetree. Any […]
The original idea was to ramp up the fans to full speed until raminit is done, then lower them to half speed and engage automatic control. That makes this function necessary in stages other than ramstage. Another idea was to ramp up the fans to full speed if coreboot dies, and since coreboot can die in any stage...
Note that I'm currently trying things out, so it's not a definite approach yet. For fan control stuff, I wanted to allow configurability (e.g. with a CBFS file that contains the fan control parameters), which is not really doable if putting values in the devicetree. And yes, configurability is needed: not everyone will use this B85M Pro4 with an [AVC DA09025T12U] as CPU fan.
[AVC DA09025T12U]: A 12V 0.70A fan, which sounds like a hair dryer at full speed: https://www.dhresource.com/0x0/f2/albu/g8/M01/D4/25/rBVaV1yQtRWAORKLAATr_7Qe...
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Build hwm.c for all stages ......................................................................
Patch Set 5:
This change is ready for review.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42130
to look at the new patch set (#11).
Change subject: superio/nuvoton: Build hwm.c for all stages ......................................................................
superio/nuvoton: Build hwm.c for all stages
There's no reason to restrict `nuvoton_hwm_select_bank` to ramstage.
Change-Id: I103a4ea4cef24844d382854c9358bbb37d229e04 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/superio/nuvoton/Makefile.inc 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/42130/11
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Build hwm.c for all stages ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42130/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42130/1//COMMIT_MSG@9 PS1, Line 9: There's no reason
The original idea was to ramp up the fans to full speed until raminit is done, then lower them to ha […]
Might be easier to retarget this patch: Make this function static inline.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Build hwm.c for all stages ......................................................................
Patch Set 12:
since the only function in nuvoton/common/hwm.c is a function wrapping another one, it might be a good idea to move that one into the header file as an inline function. if more or higher level functionality that is common for all winbond/nuvoton sio hwm units, those should be in the c file and only be linked in ramstage
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42130
to look at the new patch set (#14).
Change subject: superio/nuvoton: Inline `nuvoton_hwm_select_bank` ......................................................................
superio/nuvoton: Inline `nuvoton_hwm_select_bank`
There's no need to place a single-line function in its own compilation unit, and then guard it behind a Kconfig symbol. This also allows using this function in stages other than ramstage.
Change-Id: I103a4ea4cef24844d382854c9358bbb37d229e04 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/ibase/mb899/Kconfig M src/mainboard/kontron/986lcd-m/Kconfig M src/superio/nuvoton/Makefile.inc D src/superio/nuvoton/common/hwm.c M src/superio/nuvoton/common/hwm.h 5 files changed, 7 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/42130/14
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Inline `nuvoton_hwm_select_bank` ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42130/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42130/1//COMMIT_MSG@9 PS1, Line 9: There's no reason
Might be easier to retarget this patch: Make this function static inline.
Ack
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42130
to look at the new patch set (#15).
Change subject: superio/nuvoton: Inline `nuvoton_hwm_select_bank` ......................................................................
superio/nuvoton: Inline `nuvoton_hwm_select_bank`
There's no need to place a single-line function in its own compilation unit, and then guard it behind a Kconfig symbol. This also allows using this function in stages other than ramstage.
Change-Id: I103a4ea4cef24844d382854c9358bbb37d229e04 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/ibase/mb899/Kconfig M src/mainboard/kontron/986lcd-m/Kconfig M src/superio/nuvoton/Makefile.inc D src/superio/nuvoton/common/hwm.c M src/superio/nuvoton/common/hwm.h 5 files changed, 7 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/42130/15
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Inline `nuvoton_hwm_select_bank` ......................................................................
Patch Set 15: Code-Review+1
looks good to me, but I haven't checked if there are other boards that have a Winbond SIO and use the HWM
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Inline `nuvoton_hwm_select_bank` ......................................................................
Patch Set 16:
have you checked if no other board with a winbond SIO uses the HWM? if so, please comment on that and i'll +2 the patch
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Inline `nuvoton_hwm_select_bank` ......................................................................
Patch Set 16:
Patch Set 16:
have you checked if no other board with a winbond SIO uses the HWM? if so, please comment on that and i'll +2 the patch
Only the two boards I removed the SUPERIO_NUVOTON_COMMON_HWM symbol from.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Inline `nuvoton_hwm_select_bank` ......................................................................
Patch Set 16: Code-Review+2
ok
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Inline `nuvoton_hwm_select_bank` ......................................................................
superio/nuvoton: Inline `nuvoton_hwm_select_bank`
There's no need to place a single-line function in its own compilation unit, and then guard it behind a Kconfig symbol. This also allows using this function in stages other than ramstage.
Change-Id: I103a4ea4cef24844d382854c9358bbb37d229e04 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42130 Reviewed-by: Felix Held felix-coreboot@felixheld.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/ibase/mb899/Kconfig M src/mainboard/kontron/986lcd-m/Kconfig M src/superio/nuvoton/Makefile.inc D src/superio/nuvoton/common/hwm.c M src/superio/nuvoton/common/hwm.h 5 files changed, 7 insertions(+), 20 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/mainboard/ibase/mb899/Kconfig b/src/mainboard/ibase/mb899/Kconfig index 94c4e23..66d85f3 100644 --- a/src/mainboard/ibase/mb899/Kconfig +++ b/src/mainboard/ibase/mb899/Kconfig @@ -8,7 +8,6 @@ select CHECK_SLFRCS_ON_RESUME select SOUTHBRIDGE_INTEL_I82801GX select SUPERIO_WINBOND_W83627EHG - select SUPERIO_NUVOTON_COMMON_HWM # Nuvoton is a Winbond spin-off select HAVE_ACPI_TABLES select HAVE_PIRQ_TABLE select HAVE_MP_TABLE diff --git a/src/mainboard/kontron/986lcd-m/Kconfig b/src/mainboard/kontron/986lcd-m/Kconfig index 006837a..1a527d6 100644 --- a/src/mainboard/kontron/986lcd-m/Kconfig +++ b/src/mainboard/kontron/986lcd-m/Kconfig @@ -8,7 +8,6 @@ select CHECK_SLFRCS_ON_RESUME select SOUTHBRIDGE_INTEL_I82801GX select SUPERIO_WINBOND_W83627THG - select SUPERIO_NUVOTON_COMMON_HWM # Nuvoton is a Winbond spin-off select HAVE_ACPI_TABLES select HAVE_PIRQ_TABLE select HAVE_MP_TABLE diff --git a/src/superio/nuvoton/Makefile.inc b/src/superio/nuvoton/Makefile.inc index 054c7ba..e9ac2e3 100644 --- a/src/superio/nuvoton/Makefile.inc +++ b/src/superio/nuvoton/Makefile.inc @@ -4,9 +4,6 @@ bootblock-$(CONFIG_SUPERIO_NUVOTON_COMMON_PRE_RAM) += common/early_serial.c romstage-$(CONFIG_SUPERIO_NUVOTON_COMMON_PRE_RAM) += common/early_serial.c
-## include generic Nuvoton HWM driver -ramstage-$(CONFIG_SUPERIO_NUVOTON_COMMON_HWM) += common/hwm.c - subdirs-$(CONFIG_SUPERIO_NUVOTON_WPCM450) += wpcm450 subdirs-$(CONFIG_SUPERIO_NUVOTON_NCT5104D) += nct5104d subdirs-$(CONFIG_SUPERIO_NUVOTON_NCT5539D) += nct5539d diff --git a/src/superio/nuvoton/common/hwm.c b/src/superio/nuvoton/common/hwm.c deleted file mode 100644 index fb7b79c..0000000 --- a/src/superio/nuvoton/common/hwm.c +++ /dev/null @@ -1,14 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-or-later */ - -/* Nuvoton is a Winbond spin-off, so this code is for both */ - -#include <stdint.h> -#include <superio/hwm5_conf.h> -#include "hwm.h" - -#define HWM_BANK_SELECT 0x4e - -void nuvoton_hwm_select_bank(const u16 base, const u8 bank) -{ - pnp_write_hwm5_index(base, HWM_BANK_SELECT, bank); -} diff --git a/src/superio/nuvoton/common/hwm.h b/src/superio/nuvoton/common/hwm.h index 47d4e04..42e0f4a 100644 --- a/src/superio/nuvoton/common/hwm.h +++ b/src/superio/nuvoton/common/hwm.h @@ -6,7 +6,13 @@ /* Nuvoton is a Winbond spin-off, so this code is for both */
#include <stdint.h> +#include <superio/hwm5_conf.h>
-void nuvoton_hwm_select_bank(const u16 base, const u8 bank); +#define HWM_BANK_SELECT 0x4e + +static inline void nuvoton_hwm_select_bank(const u16 base, const u8 bank) +{ + pnp_write_hwm5_index(base, HWM_BANK_SELECT, bank); +}
#endif /* SUPERIO_NUVOTON_COMMON_HWM_H */
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42130 )
Change subject: superio/nuvoton: Inline `nuvoton_hwm_select_bank` ......................................................................
Patch Set 17:
Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19945 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19944 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19943 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19942 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19941 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19949 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19948 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19947 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19946
Please note: This test is under development and might not be accurate at all!