Change in coreboot[master]: drivers/ipmi: make IPMI KCS status and command register offset config...

[Name of user not set (Code Review)]

Name of user not set #1002517 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34835 )

Change subject: drivers/ipmi: make IPMI KCS status and command register offset configurable
......................................................................

drivers/ipmi: make IPMI KCS status and command register offset configurable

Add offset options for 1 (BYTE), 2 (WORD) and 4 (DWORD).

Tested on Mono Lake

Change-Id: I47412c32e6db8f58b4fde8150adcbce349ca18a7
Signed-off-by: Johnny Lin 
---
M src/drivers/ipmi/Kconfig
M src/drivers/ipmi/ipmi_kcs.c
2 files changed, 33 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34835/1

diff --git a/src/drivers/ipmi/Kconfig b/src/drivers/ipmi/Kconfig
index 5851438..cb4d256 100644
--- a/src/drivers/ipmi/Kconfig
+++ b/src/drivers/ipmi/Kconfig
@@ -1,3 +1,34 @@
 config IPMI_KCS
    bool
    default n
+
+choice
+	prompt "IPMI KCS register address offset"
+	default KCS_REGISTER_OFFSET_1
+
+config KCS_REGISTER_OFFSET_1
+	bool "IPMI KCS register address offset 1 (BYTE)"
+	help
+	  Set IPMI KCS register address offset to 1 (BYTE).
+
+config KCS_REGISTER_OFFSET_2
+	bool "IPMI KCS register address offset 2 (WORD)"
+	help
+	  Set IPMI KCS register address offset to 2 (WORD).
+
+config KCS_REGISTER_OFFSET_4
+	bool "IPMI KCS register address offset 4 (DWORD)"
+	help
+	  Set IPMI KCS register address offset to 4 (DWORD).
+
+endchoice
+
+config KCS_REGISTER_OFFSET
+	int
+	default 1 if KCS_REGISTER_OFFSET_1
+	default 2 if KCS_REGISTER_OFFSET_2
+	default 4 if KCS_REGISTER_OFFSET_4
+	depends on IPMI_KCS
+	help
+	  KCS status and command register IO port address offset
+
diff --git a/src/drivers/ipmi/ipmi_kcs.c b/src/drivers/ipmi/ipmi_kcs.c
index 397a800..8975f75 100644
--- a/src/drivers/ipmi/ipmi_kcs.c
+++ b/src/drivers/ipmi/ipmi_kcs.c
@@ -36,9 +36,9 @@
 #define IPMI_KCS_STATE_WRITE 0x02
 #define IPMI_KCS_STATE_ERROR 0x03

-#define IPMI_CMD(_x) ((_x) + 1)
+#define IPMI_CMD(_x) ((_x) + CONFIG_KCS_REGISTER_OFFSET)
 #define IPMI_DATA(_x) ((_x))
-#define IPMI_STAT(_x) ((_x) + 1)
+#define IPMI_STAT(_x) ((_x) + CONFIG_KCS_REGISTER_OFFSET)

static unsigned char ipmi_kcs_status(int port)
 {

[Name of user not set (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: drivers/ipmi: make IPMI KCS status and command register offset configurable
......................................................................

drivers/ipmi: make IPMI KCS status and command register offset configurable

Add offset options for 1 (BYTE), 2 (WORD) and 4 (DWORD).

Tested on Mono Lake

Change-Id: I47412c32e6db8f58b4fde8150adcbce349ca18a7
Signed-off-by: Johnny Lin 
---
M src/drivers/ipmi/Kconfig
M src/drivers/ipmi/ipmi_kcs.c
2 files changed, 32 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34835/2

[Name of user not set (Code Review)]

Name of user not set #1002517 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34835 )

Change subject: drivers/ipmi: make IPMI KCS status and command register offset configurable
......................................................................

Patch Set 2:

On OCP Mono Lake it needs to be configured as offset 4.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34835 )

Change subject: drivers/ipmi: make IPMI KCS status and command register offset configurable
......................................................................

Patch Set 2: Code-Review-1

(2 comments)

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig 
File src/drivers/ipmi/Kconfig:

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig@5 
PS2, Line 5: choice
why is it user configurable?
Isn't that board/BMC specific?

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig@15 
PS2, Line 15: 	bool "IPMI KCS register address offset 2 (WORD)"
that's not part of the spec.
It says:
"either on byte, 32-bit, or 16-byte boundaries"

[Name of user not set (Code Review)]

Name of user not set #1002517 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34835 )

Change subject: drivers/ipmi: make IPMI KCS status and command register offset configurable
......................................................................

Patch Set 2:

(2 comments)
> Patch Set 2: Code-Review-1
>
> (2 comments)

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig 
File src/drivers/ipmi/Kconfig:

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig@5 
PS2, Line 5: choice
> why is it user configurable? […]

Yes it is board/BMC specific, but coreboot needs to modify the offset to match BMC's configuration, for Mono Lake the offset is 4 (32-bit boundaries).

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig@15 
PS2, Line 15: 	bool "IPMI KCS register address offset 2 (WORD)"
> that's not part of the spec. […]

Yes you are right, I will remove this part in my next patch set. Because I don't have device that can verify 16-byte boundaries so in this commit I will only add options for byte and 32-bit boundaries.

[Name of user not set (Code Review)]

Hello Patrick Rudolph, David Hendricks, Jonathan Zhang, Philipp Deppenwiese, build bot (Jenkins),

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

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

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

Change subject: drivers/ipmi: make IPMI KCS status and command register offset configurable
......................................................................

drivers/ipmi: make IPMI KCS status and command register offset configurable

Add offset options for 1 (byte) and 4 (32-bit).

Tested on Mono Lake

Change-Id: I47412c32e6db8f58b4fde8150adcbce349ca18a7
Signed-off-by: Johnny Lin 
---
M src/drivers/ipmi/Kconfig
M src/drivers/ipmi/ipmi_kcs.c
2 files changed, 26 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34835/3

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34835 )

Change subject: drivers/ipmi: make IPMI KCS status and command register offset configurable
......................................................................

Patch Set 3:

(4 comments)

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig 
File src/drivers/ipmi/Kconfig:

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig@5 
PS2, Line 5: choice
> Yes it is board/BMC specific, but coreboot needs to modify the offset to match BMC's configuration,  […]

please remove the choice

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig@26 
PS2, Line 26: config KCS_REGISTER_OFFSET
The spec uses the name "spacing" instead of offset. I'd prefer IPMI_KCS_REGISTER_SPACING.

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig@28 
PS2, Line 28: 	default 1 if KCS_REGISTER_OFFSET_1
default 1

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig@31 
PS2, Line 31: 	depends on IPMI_KCS
in monolake mainboard Kconfig do:

config IPMI_KCS_REGISTER_SPACING
  default 4

Change in coreboot[master]: drivers/ipmi: make IPMI KCS status and command register spacing confi...

[Johnny Lin (Code Review)]

Hello Patrick Rudolph, Sven Schnelle, David Hendricks, Jonathan Zhang, Philipp Deppenwiese, build bot (Jenkins), Andrey Petrov,

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

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

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

Change subject: drivers/ipmi: make IPMI KCS status and command register spacing configurable
......................................................................

drivers/ipmi: make IPMI KCS status and command register spacing configurable

The default is 1 (byte) spacing.

Tested on Mono Lake with 4 (32-bit) spacing

Change-Id: I47412c32e6db8f58b4fde8150adcbce349ca18a7
Signed-off-by: Johnny Lin 
---
M src/drivers/ipmi/Kconfig
M src/drivers/ipmi/ipmi_kcs.c
2 files changed, 9 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/35/34835/4

Change in coreboot[master]: drivers/ipmi: make IPMI KCS status and command register spacing confi...

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34835 )

Change subject: drivers/ipmi: make IPMI KCS status and command register spacing configurable
......................................................................

Patch Set 4:

(5 comments)
> Patch Set 3:
>
> (4 comments)

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig 
File src/drivers/ipmi/Kconfig:

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig@5 
PS2, Line 5: choice
> please remove the choice

Done

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig@15 
PS2, Line 15: 	bool "IPMI KCS register address offset 2 (WORD)"
> Yes you are right, I will remove this part in my next patch set. […]

Done

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig@26 
PS2, Line 26: config KCS_REGISTER_OFFSET
> The spec uses the name "spacing" instead of offset. I'd prefer IPMI_KCS_REGISTER_SPACING.

Done

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig@28 
PS2, Line 28: 	default 1 if KCS_REGISTER_OFFSET_1
> default 1

Done

https://review.coreboot.org/c/coreboot/+/34835/2/src/drivers/ipmi/Kconfig@31 
PS2, Line 31: 	depends on IPMI_KCS
> in monolake mainboard Kconfig do: […]

Sure I will put this in another change.

Change in coreboot[master]: drivers/ipmi: make IPMI KCS status and command register spacing confi...

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34835 )

Change subject: drivers/ipmi: make IPMI KCS status and command register spacing configurable
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: drivers/ipmi: make IPMI KCS status and command register spacing confi...

[Patrick Rudolph (Code Review)]

Patrick Rudolph has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34835 )

Change subject: drivers/ipmi: make IPMI KCS status and command register spacing configurable
......................................................................

drivers/ipmi: make IPMI KCS status and command register spacing configurable

The default is 1 (byte) spacing.

Tested on Mono Lake with 4 (32-bit) spacing

Change-Id: I47412c32e6db8f58b4fde8150adcbce349ca18a7
Signed-off-by: Johnny Lin 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34835
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
---
M src/drivers/ipmi/Kconfig
M src/drivers/ipmi/ipmi_kcs.c
2 files changed, 9 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Rudolph: Looks good to me, approved

diff --git a/src/drivers/ipmi/Kconfig b/src/drivers/ipmi/Kconfig
index 5851438..0f7152d 100644
--- a/src/drivers/ipmi/Kconfig
+++ b/src/drivers/ipmi/Kconfig
@@ -1,3 +1,10 @@
 config IPMI_KCS
    bool
    default n
+
+config IPMI_KCS_REGISTER_SPACING
+	int
+	default 1
+	depends on IPMI_KCS
+	help
+	  KCS status and command register IO port address spacing
diff --git a/src/drivers/ipmi/ipmi_kcs.c b/src/drivers/ipmi/ipmi_kcs.c
index 397a800..4d1e3e1 100644
--- a/src/drivers/ipmi/ipmi_kcs.c
+++ b/src/drivers/ipmi/ipmi_kcs.c
@@ -36,9 +36,9 @@
 #define IPMI_KCS_STATE_WRITE 0x02
 #define IPMI_KCS_STATE_ERROR 0x03

-#define IPMI_CMD(_x) ((_x) + 1)
+#define IPMI_CMD(_x) ((_x) + CONFIG_IPMI_KCS_REGISTER_SPACING)
 #define IPMI_DATA(_x) ((_x))
-#define IPMI_STAT(_x) ((_x) + 1)
+#define IPMI_STAT(_x) ((_x) + CONFIG_IPMI_KCS_REGISTER_SPACING)

static unsigned char ipmi_kcs_status(int port)
 {