Change in coreboot[master]: superio/*: Standardise config state entry/exit prototype in pnp_ops.h

[Keith Hui (Code Review)]

Keith Hui has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40962 )

Change subject: superio/*: Standardise config state entry/exit prototype in pnp_ops.h
......................................................................

superio/*: Standardise config state entry/exit prototype in pnp_ops.h

pnp_{entry,exit}_conf_state() is declared and implemented under each
super I/O with not that much variation. Place them to a central
location along with other pre-RAM pnp functions.

This provides a standard pre-RAM PNP API, with the eventual goal of
consolidating all implementations, with only one implementation
per actual known config state entry/exit sequence.

Remove the prototype from winbond/common. All code that use pre-RAM
pnp include pnp_ops.h and all basic functions will be brought in. The
correct implementation is selected at compile time via Makefile.inc.

Change-Id: If4e742edb17ca73c01ff7b552e00e18acc6779dd
Signed-off-by: Keith Hui 
---
M src/include/device/pnp_ops.h
M src/superio/fintek/common/fintek.h
M src/superio/winbond/common/winbond.h
3 files changed, 3 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40962/1

diff --git a/src/include/device/pnp_ops.h b/src/include/device/pnp_ops.h
index 93a5dc8..f43cb33 100644
--- a/src/include/device/pnp_ops.h
+++ b/src/include/device/pnp_ops.h
@@ -10,6 +10,9 @@

#if ENV_PNP_SIMPLE_DEVICE

+void pnp_enter_conf_state(pnp_devfn_t dev);
+void pnp_exit_conf_state(pnp_devfn_t dev);
+
 static __always_inline void pnp_write_config(
    pnp_devfn_t dev, uint8_t reg, uint8_t value)
 {
diff --git a/src/superio/fintek/common/fintek.h b/src/superio/fintek/common/fintek.h
index 81e8e67..915c148 100644
--- a/src/superio/fintek/common/fintek.h
+++ b/src/superio/fintek/common/fintek.h
@@ -9,7 +9,4 @@

void fintek_enable_serial(pnp_devfn_t dev, u16 iobase);

-void pnp_enter_conf_state(pnp_devfn_t dev);
-void pnp_exit_conf_state(pnp_devfn_t dev);
-
 #endif /* SUPERIO_FINTEK_COMMON_PRE_RAM_H */
diff --git a/src/superio/winbond/common/winbond.h b/src/superio/winbond/common/winbond.h
index 58297e5..9816db0 100644
--- a/src/superio/winbond/common/winbond.h
+++ b/src/superio/winbond/common/winbond.h
@@ -11,7 +11,4 @@
 void winbond_set_pinmux(pnp_devfn_t dev, uint8_t offset, uint8_t mask, uint8_t state);
 void winbond_set_clksel_48(pnp_devfn_t dev);

-void pnp_enter_conf_state(pnp_devfn_t dev);
-void pnp_exit_conf_state(pnp_devfn_t dev);
-
 #endif /* SUPERIO_WINBOND_COMMON_PRE_RAM_H */

[Keith Hui (Code Review)]

Hello build bot (Jenkins), Felix Held,

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

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

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

Change subject: superio/*: Standardise config state entry/exit prototype in pnp_ops.h
......................................................................

superio/*: Standardise config state entry/exit prototype in pnp_ops.h

pnp_{entry,exit}_conf_state() is declared and implemented under each
super I/O with not that much variation. Move the identical prototypes
to a central location along with other pre-RAM pnp functions.

All code that use pre-RAM pnp include pnp_ops.h and all basic functions
will be brought in. The correct implementation is selected at compile
time via Makefile.inc.

This provides a standard pre-RAM PNP API, with the eventual goal of
consolidating all implementations, with only one implementation
per actual known config state entry/exit sequence.

Change-Id: If4e742edb17ca73c01ff7b552e00e18acc6779dd
Signed-off-by: Keith Hui 
---
M src/include/device/pnp_ops.h
M src/superio/aspeed/common/aspeed.h
M src/superio/fintek/common/fintek.h
M src/superio/ite/common/ite.h
M src/superio/smsc/lpc47m10x/lpc47m10x.h
M src/superio/smsc/lpc47n227/lpc47n227.h
M src/superio/winbond/common/winbond.h
7 files changed, 3 insertions(+), 17 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40962/2

[Keith Hui (Code Review)]

Hello build bot (Jenkins), Felix Held,

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

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

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

Change subject: superio/*: Standardise config state entry/exit prototype in pnp_ops.h
......................................................................

superio/*: Standardise config state entry/exit prototype in pnp_ops.h

pnp_{entry,exit}_conf_state() is declared and implemented under each
super I/O with not that much variation. Move the identical prototypes
to a central location along with other pre-RAM pnp functions.

All code that use pre-RAM pnp include pnp_ops.h and all basic functions
will be brought in. The correct implementation is selected at compile
time via Makefile.inc.

This provides a standard pre-RAM PNP API, with the eventual goal of
consolidating all implementations, with only one implementation
per actual known config state entry/exit sequence.

TEST=abuild

Change-Id: If4e742edb17ca73c01ff7b552e00e18acc6779dd
Signed-off-by: Keith Hui 
---
M src/include/device/pnp_ops.h
M src/superio/aspeed/common/aspeed.h
M src/superio/fintek/common/fintek.h
M src/superio/ite/common/ite.h
M src/superio/smsc/kbc1100/early_init.c
M src/superio/smsc/lpc47m10x/lpc47m10x.h
M src/superio/smsc/lpc47m15x/early_serial.c
M src/superio/smsc/lpc47n217/early_serial.c
M src/superio/smsc/lpc47n227/lpc47n227.h
M src/superio/winbond/common/winbond.h
10 files changed, 9 insertions(+), 23 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40962/3

[Paul Menzel (Code Review)]

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

Change subject: superio/*: Standardise config state entry/exit prototype in pnp_ops.h
......................................................................

Patch Set 3:

This change is ready for review.

[Keith Hui (Code Review)]

Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40962 )

Change subject: superio/*: Standardise config state entry/exit prototype in pnp_ops.h
......................................................................

Patch Set 3:

This change is ready for review.

[Keith Hui (Code Review)]

Hello build bot (Jenkins), Felix Held,

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

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

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

Change subject: superio/*: Standardise config state entry/exit prototype in pnp_ops.h
......................................................................

superio/*: Standardise config state entry/exit prototype in pnp_ops.h

pnp_{entry,exit}_conf_state() is declared and implemented under each
super I/O with not that much variation. Move the identical prototypes
to a central location along with other pre-RAM pnp functions.

All code that use pre-RAM pnp include pnp_ops.h and all basic functions
will be brought in. The correct implementation is selected at compile
time via Makefile.inc.

This provides a standard pre-RAM PNP API, with the eventual goal of
consolidating all implementations, with only one implementation
per actual known config state entry/exit sequence.

TEST=abuild

Change-Id: If4e742edb17ca73c01ff7b552e00e18acc6779dd
Signed-off-by: Keith Hui 
---
M src/ec/google/wilco/bootblock.c
M src/include/device/pnp_ops.h
M src/superio/aspeed/common/aspeed.h
M src/superio/fintek/common/fintek.h
M src/superio/ite/common/ite.h
M src/superio/smsc/kbc1100/early_init.c
M src/superio/smsc/lpc47m10x/lpc47m10x.h
M src/superio/smsc/lpc47m15x/early_serial.c
M src/superio/smsc/lpc47n217/early_serial.c
M src/superio/smsc/lpc47n227/lpc47n227.h
M src/superio/smsc/smscsuperio/early_serial.c
M src/superio/winbond/common/winbond.h
12 files changed, 13 insertions(+), 27 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40962/5

[Keith Hui (Code Review)]

Hello build bot (Jenkins), Felix Held,

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

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

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

Change subject: superio/*: Standardise config state entry/exit prototype in pnp_ops.h
......................................................................

superio/*: Standardise config state entry/exit prototype in pnp_ops.h

pnp_{entry,exit}_conf_state() is declared and implemented under each
super I/O with not that much variation. Move the identical prototypes
to a central location along with other pre-RAM pnp functions.

All code that use pre-RAM pnp include pnp_ops.h and all basic functions
will be brought in. The correct implementation is selected at compile
time via Makefile.inc.

This provides a standard pre-RAM PNP API, with the eventual goal of
consolidating all implementations, with only one implementation
per actual known config state entry/exit sequence.

TEST=abuild

Change-Id: If4e742edb17ca73c01ff7b552e00e18acc6779dd
Signed-off-by: Keith Hui 
---
M src/ec/google/wilco/bootblock.c
M src/include/device/pnp_ops.h
M src/superio/aspeed/common/aspeed.h
M src/superio/fintek/common/fintek.h
M src/superio/ite/common/ite.h
M src/superio/smsc/kbc1100/early_init.c
M src/superio/smsc/lpc47m10x/lpc47m10x.h
M src/superio/smsc/lpc47m15x/early_serial.c
M src/superio/smsc/lpc47n217/early_serial.c
M src/superio/smsc/lpc47n227/lpc47n227.h
M src/superio/smsc/sch5545/sch5545_early_init.c
M src/superio/smsc/smscsuperio/early_serial.c
M src/superio/winbond/common/winbond.h
13 files changed, 15 insertions(+), 29 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/40962/7

[Paul Menzel (Code Review)]

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

Change subject: superio/*: Standardise config state entry/exit prototype in pnp_ops.h
......................................................................

Patch Set 7: Code-Review+1

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40962 )

Change subject: superio/*: Standardise config state entry/exit prototype in pnp_ops.h
......................................................................

Patch Set 7:

The problem I see with this and the next patch (40963) is that it bases on the assumption that never more than one type of super i/o comfiguration enter/exit sequences can be linked in the pre-ram stages of a board. An example of a board where this assumption is not valid would be the Supermicro X10SLM+-F that only passes the build test after the two patches, because it only initializes the Nuvoton super I/O in bootblock/romstage and doesn't do anything with the ASpeed chip before ramstage.

I'd suggest adding functions in a common place with names like pnp_enter_conf_mode_55 that take pnp_devfn_t as dev parameter type instead of struct device *. Then just add the calls to the corresponding config mode enter or exit functions in the pre-ram-code.

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40962 )

Change subject: superio/*: Standardise config state entry/exit prototype in pnp_ops.h
......................................................................

Patch Set 7:
> Patch Set 7:
>
> The problem I see with this and the next patch (40963) is that it bases on the assumption that never more than one type of super i/o comfiguration enter/exit sequences can be linked in the pre-ram stages of a board. An example of a board where this assumption is not valid would be the Supermicro X10SLM+-F that only passes the build test after the two patches, because it only initializes the Nuvoton super I/O in bootblock/romstage and doesn't do anything with the ASpeed chip before ramstage.
>
> I'd suggest adding functions in a common place with names like pnp_enter_conf_mode_55 that take pnp_devfn_t as dev parameter type instead of struct device *. Then just add the calls to the corresponding config mode enter or exit functions in the pre-ram-code.

If you look at some generated mb/static.c file, we already have PNP device nodes in romstage, and I think you can constify struct device * everywhere in pnp_enter/exit_xx declarations?

Longterm I think SIMPLE_DEVICE and pci_devfn_t and pnp_devfn_t could go away.

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40962 )

Change subject: superio/*: Standardise config state entry/exit prototype in pnp_ops.h
......................................................................

Patch Set 7:
> If you look at some generated mb/static.c file, we already have PNP device nodes in romstage, and I think you can constify struct device * everywhere in pnp_enter/exit_xx declarations?

That doesn't solve the problem for bootblock and the serial console usually gets configured in bootblock.
> Longterm I think SIMPLE_DEVICE and pci_devfn_t and pnp_devfn_t could go away.

I'd like to see that, but I'm not sure how feasible this would be. Haven't had a look how difficult it would be to solve this for bootblock.

[S] Change in coreboot[master]: superio/*: Standardise config state entry/exit prototype in pnp_ops.h

[Stefan Reinauer (Code Review)]

Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40962?usp=email )

Change subject: superio/*: Standardise config state entry/exit prototype in pnp_ops.h
......................................................................

Abandoned