Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
mb/supermicro/x9scl: Select IPMI_KCS
Needed for `chip drivers/ipmi` in the devicetree.
Change-Id: Ice70aab7cedaeb91a33dd90d763c5a487f190b8f Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/supermicro/x9scl/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/41687/1
diff --git a/src/mainboard/supermicro/x9scl/Kconfig b/src/mainboard/supermicro/x9scl/Kconfig index df6308e..c2e3a37 100644 --- a/src/mainboard/supermicro/x9scl/Kconfig +++ b/src/mainboard/supermicro/x9scl/Kconfig @@ -13,6 +13,7 @@ select SUPERIO_NUVOTON_NCT6776_COM_A select SUPERIO_NUVOTON_WPCM450 select MAINBOARD_USES_IFD_GBE_REGION + select IPMI_KCS
config MAINBOARD_DIR string
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 1: Code-Review+1
Jonathan Kollasch has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 1: Code-Review-1
Have you tested this on a X9SCL or X9SCM (i.e. one without BMC features)?
The reason I didn't enable this already was that it caused rather long delays in boot time (on the order of like 30 seconds or more, IIRC) on a X9SCL, as the unimplemented BMC never came ready. That said, it did appear to work OK when I tried it on the X9SCM-F.
I think there's a board config strap GPIO that indicates the presence of BMC features, but I have not identified it exactly.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
Have you tested this on a X9SCL or X9SCM (i.e. one without BMC features)?
The reason I didn't enable this already was that it caused rather long delays in boot time (on the order of like 30 seconds or more, IIRC) on a X9SCL, as the unimplemented BMC never came ready. That said, it did appear to work OK when I tried it on the X9SCM-F.
I think there's a board config strap GPIO that indicates the presence of BMC features, but I have not identified it exactly.
No, that's why I added you as a reviewer. If you don't want to use IPMI in coreboot for this board, then I would drop the devicetree entry for IPMI.
Jonathan Kollasch has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 1: Code-Review+1
I re-tested this just now. As long as we keep 'device pnp ca2.0 off end' in the devicetree.cb, no excruciating delays are introduced on a non-'-F' X9SCL.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+1
I re-tested this just now. As long as we keep 'device pnp ca2.0 off end' in the devicetree.cb, no excruciating delays are introduced on a non-'-F' X9SCL.
What about the "wait_for_bmc" thing? Also, the timeout is set at 60 (seconds?) so it's pretty large
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 3: Code-Review-1
This is the X9SCL, not X9SCL-F which is a variant of X9SCL with BMC.
Supermicro provides multiple variants of each board, where the board itself matches but some components like BMC are missing: X9SCL = !BMC, X9SCL-F = BMC; X11SSM = !BMC, X11SSM-F = BMC
My suggestion on this one is to add the X9SCL-F as variant of X9SCL/X9SCM
Jonathan Kollasch has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 3: -Code-Review
Patch Set 3: Code-Review-1
This is the X9SCL, not X9SCL-F which is a variant of X9SCL with BMC.
Supermicro provides multiple variants of each board, where the board itself matches but some components like BMC are missing: X9SCL = !BMC, X9SCL-F = BMC; X11SSM = !BMC, X11SSM-F = BMC
My suggestion on this one is to add the X9SCL-F as variant of X9SCL/X9SCM
I'll point out that Supermicro's firmware update image is identical for all of X9SCL, X9SCL-F, X9SCL+-F, X9SCM, X9SCM-F, X9SCM-iiF. I believe this holds true for the BMC firmware image (where applicable) too. I can probably figure out whatever GPIO pin or whatnot is used by the Supermicro firmware to differentiate boards with BMC from boards without.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 3:
Patch Set 3: -Code-Review
Patch Set 3: Code-Review-1
This is the X9SCL, not X9SCL-F which is a variant of X9SCL with BMC.
Supermicro provides multiple variants of each board, where the board itself matches but some components like BMC are missing: X9SCL = !BMC, X9SCL-F = BMC; X11SSM = !BMC, X11SSM-F = BMC
My suggestion on this one is to add the X9SCL-F as variant of X9SCL/X9SCM
I'll point out that Supermicro's firmware update image is identical for all of X9SCL, X9SCL-F, X9SCL+-F, X9SCM, X9SCM-F, X9SCM-iiF. I believe this holds true for the BMC firmware image (where applicable) too. I can probably figure out whatever GPIO pin or whatnot is used by the Supermicro firmware to differentiate boards with BMC from boards without.
If we find a way doing this, we should document somehow that BMC is detected automagically if present. Probably we also should rename the boards then: X9SCL(-F)/X9SCM(-F), X11SSM(-F)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3: -Code-Review
Patch Set 3: Code-Review-1
This is the X9SCL, not X9SCL-F which is a variant of X9SCL with BMC.
Supermicro provides multiple variants of each board, where the board itself matches but some components like BMC are missing: X9SCL = !BMC, X9SCL-F = BMC; X11SSM = !BMC, X11SSM-F = BMC
My suggestion on this one is to add the X9SCL-F as variant of X9SCL/X9SCM
I'll point out that Supermicro's firmware update image is identical for all of X9SCL, X9SCL-F, X9SCL+-F, X9SCM, X9SCM-F, X9SCM-iiF. I believe this holds true for the BMC firmware image (where applicable) too. I can probably figure out whatever GPIO pin or whatnot is used by the Supermicro firmware to differentiate boards with BMC from boards without.
If we find a way doing this, we should document somehow that BMC is detected automagically if present. Probably we also should rename the boards then: X9SCL(-F)/X9SCM(-F), X11SSM(-F)
Two possibilities are matching VGA pci device id or hardware id via superio (if we don't find a more generic way)
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 3:
Oh, looks like we can't really generalize it... - X11SSM has AST1400, while X11SSM-F has AST2400 - detection done by SCU7C register (hw id and revision) in X11SSMPeiDriver - X8SIE-F/X8SIL-F and X9SCL-F/X9SCM-F have WPCM450, while the non-F variants don't have the chip populated -> detection probably by some strap/gpio/id/...
Ofc we could do some runtime detection in both cases. While I don't expect any problems on the X11 series (where the pads are connected in both cases), I do expect GPIO floating due to unconnected pins on the non-F variants. -> The non-F variants need a different GPIO config. We still could make that possible by switching between two configs in `mainboard.c`.
I am not sure if we really want do make it that complicated in contrast of making the BMC boards variants.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 3:
This is dependency to CB:35086 so please come up with some plan.
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 3:
Patch Set 3:
This is dependency to CB:35086 so please come up with some plan.
Why is this a dependency? I dont see any code in here where CB:35086 is dependent on. What about rebasing CB:35086 and removing this from the relation chain?
Jonathan Kollasch has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 3: Code-Review+1
Patch Set 3:
Oh, looks like we can't really generalize it...
- X11SSM has AST1400, while X11SSM-F has AST2400 - detection done by SCU7C register (hw id and revision) in X11SSMPeiDriver
- X8SIE-F/X8SIL-F and X9SCL-F/X9SCM-F have WPCM450, while the non-F variants don't have the chip populated -> detection probably by some strap/gpio/id/...
Ofc we could do some runtime detection in both cases. While I don't expect any problems on the X11 series (where the pads are connected in both cases), I do expect GPIO floating due to unconnected pins on the non-F variants. -> The non-F variants need a different GPIO config. We still could make that possible by switching between two configs in `mainboard.c`.
I am not sure if we really want do make it that complicated in contrast of making the BMC boards variants.
I looked around a bit. I have strong indication that PCH GPIO 57 (GP_LVL2 bit 25 aka mask 0x02000000) is used on the X9SCL/X9SCM series to indicate the presence of a WPCM450 BMC w/ firmware (bit is 0), vs. a WPCM150 GPU (bit is 1). This is already configured as a GPIO in x9scl/gpio.c. I've confirmed by reading GP_LVL2 on both a X9SCM-F and X9SCL. I have some confidence that this GPIO pin is configured by differences in the bills of materials for the different X9SCL/X9SCM variants, and as such there's not much reason to worry that the GPIO is floating.
As I said before: as we're not (yet) turning the ipmi device on in the x9scl device tree, this all is not actually a problem, and does not block this change for me.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 3:
Patch Set 3: Code-Review+1
Patch Set 3:
Oh, looks like we can't really generalize it...
- X11SSM has AST1400, while X11SSM-F has AST2400 - detection done by SCU7C register (hw id and revision) in X11SSMPeiDriver
- X8SIE-F/X8SIL-F and X9SCL-F/X9SCM-F have WPCM450, while the non-F variants don't have the chip populated -> detection probably by some strap/gpio/id/...
Ofc we could do some runtime detection in both cases. While I don't expect any problems on the X11 series (where the pads are connected in both cases), I do expect GPIO floating due to unconnected pins on the non-F variants. -> The non-F variants need a different GPIO config. We still could make that possible by switching between two configs in `mainboard.c`.
I am not sure if we really want do make it that complicated in contrast of making the BMC boards variants.
I looked around a bit. I have strong indication that PCH GPIO 57 (GP_LVL2 bit 25 aka mask 0x02000000) is used on the X9SCL/X9SCM series to indicate the presence of a WPCM450 BMC w/ firmware (bit is 0), vs. a WPCM150 GPU (bit is 1). This is already configured as a GPIO in x9scl/gpio.c. I've confirmed by reading GP_LVL2 on both a X9SCM-F and X9SCL. I have some confidence that this GPIO pin is configured by differences in the bills of materials for the different X9SCL/X9SCM variants,
Well, nice that we have a detection method for the X9 boards! Since that driver is not specific for WPCM450 but generic for many BMC/IPMI chips, having only one detection method is not enough. We have to have one for each possible BMC in the coreboot tree. Additionally this has to be maintained in the future, when new BMCs or new boards with different detection methods (different gpio for example) get added.
Even when we now have a runtime detection mechanism for X9 and X11 series, I personally wouldn't add that overhead when a simple variant with overridetree is enough.
and as such there's not much reason to worry that the GPIO is floating.
Oh, I just realized I looked at the wrong place! The BMC is always populated either with full BMC or with graphics only, right? Can you provide a inteltool gpio log from both boards, please?
As I said before: as we're not (yet) turning the ipmi device on in the x9scl device tree, this all is not actually a problem, and does not block this change for me.
Yeah sure, the X9SCL non-F shouldn't have it enabled.
Kyösti Mälkki has uploaded a new patch set (#4) to the change originally created by Angel Pons. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: [REJECTED] mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
[REJECTED] mb/supermicro/x9scl: Select IPMI_KCS
Needed for `chip drivers/ipmi` in the devicetree.
Change-Id: Ice70aab7cedaeb91a33dd90d763c5a487f190b8f Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/supermicro/x9scl/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/41687/4
Kyösti Mälkki has uploaded a new patch set (#5) to the change originally created by Angel Pons. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: [REJECTED] mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
[REJECTED] mb/supermicro/x9scl: Select IPMI_KCS
Needed for `chip drivers/ipmi` in the devicetree.
Change-Id: Ice70aab7cedaeb91a33dd90d763c5a487f190b8f Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/supermicro/x9scl/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/41687/5
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: [REJECTED] mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 5:
This board is now the only board blocking CB:35086
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: [REJECTED] mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 5:
Patch Set 3:
Patch Set 3:
This is dependency to CB:35086 so please come up with some plan.
Why is this a dependency? I dont see any code in here where CB:35086 is dependent on. What about rebasing CB:35086 and removing this from the relation chain?
CB:35086 removes weak declarations for devicetree ops. So, adding a "chip" entry without building the associated driver becomes a build-time error. This change is to make this mainboard build after CB:35086 lands, as it has a `chip drivers/ipmi` entry in the devicetree, but does not build in the IPMI KCS driver.
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: [REJECTED] mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Abandoned
Prefer CB:43085
Angel Pons has restored this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: [REJECTED] mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Restored
Hello build bot (Jenkins), Nico Huber, Jonathan Kollasch, Paul Menzel, Michael Niewöhner, Kyösti Mälkki,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41687
to look at the new patch set (#6).
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
mb/supermicro/x9scl: Select IPMI_KCS
Needed for `chip drivers/ipmi` in the devicetree.
Change-Id: Ice70aab7cedaeb91a33dd90d763c5a487f190b8f Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/supermicro/x9scl/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/41687/6
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 6: Code-Review+2
I'm not happy with this but since multiple attempts to really fix this were not successfull due to people unwilling to provide dumps... just merge it to unblock other peoples work which is way more important than this.
Jonathan Kollasch has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 6: Code-Review+1
Jonathan Kollasch has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 6:
Patch Set 6: Code-Review+2
I'm not happy with this but since multiple attempts to really fix this were not successfull due to people unwilling to provide dumps... just merge it to unblock other peoples work which is way more important than this.
I do my best to be helpful, but sometimes I just don't manage to do it in a timely manner. 😞
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 7:
Swapped the order to have this before CB:35086.
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
mb/supermicro/x9scl: Select IPMI_KCS
Needed for `chip drivers/ipmi` in the devicetree.
Change-Id: Ice70aab7cedaeb91a33dd90d763c5a487f190b8f Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/41687 Reviewed-by: Michael Niewöhner Reviewed-by: Jonathan Kollasch jakllsch@kollasch.net Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/supermicro/x9scl/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Jonathan Kollasch: Looks good to me, but someone else must approve Michael Niewöhner: Looks good to me, approved
diff --git a/src/mainboard/supermicro/x9scl/Kconfig b/src/mainboard/supermicro/x9scl/Kconfig index fb73683..9968d34 100644 --- a/src/mainboard/supermicro/x9scl/Kconfig +++ b/src/mainboard/supermicro/x9scl/Kconfig @@ -13,6 +13,7 @@ select SUPERIO_NUVOTON_NCT6776_COM_A select SUPERIO_NUVOTON_WPCM450 select MAINBOARD_USES_IFD_GBE_REGION + select IPMI_KCS
config MAINBOARD_DIR string
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41687 )
Change subject: mb/supermicro/x9scl: Select IPMI_KCS ......................................................................
Patch Set 7:
Patch Set 6:
Patch Set 6: Code-Review+2
I'm not happy with this but since multiple attempts to really fix this were not successfull due to people unwilling to provide dumps... just merge it to unblock other peoples work which is way more important than this.
I do my best to be helpful, but sometimes I just don't manage to do it in a timely manner. 😞
A short notice on IRC would have made that clear and we could have tried to find a quick fix to not delay the other changes. I tried to reach out to you multiple times :P Just ping me on irc when you had the time to dump the stuff :-)
-
Angel Pons (Code Review) -
Christian Walter (Code Review)
-
Jonathan Kollasch (Code Review)
-
Kyösti Mälkki (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)