Change in coreboot[master]: soc/intel/common/block/p2sb: Add hpet BDF functions

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47531 )

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

soc/intel/common/block/p2sb: Add hpet BDF functions

This allows to get/set the HPET bus device function.

Change-Id: I8d72da8bc392aa144d167d31cde30cc71cd1396e
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/common/block/include/intelblocks/p2sb.h
M src/soc/intel/common/block/p2sb/p2sb.c
2 files changed, 28 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/47531/1

diff --git a/src/soc/intel/common/block/include/intelblocks/p2sb.h b/src/soc/intel/common/block/include/intelblocks/p2sb.h
index 71f9c62..7cf0c48 100644
--- a/src/soc/intel/common/block/include/intelblocks/p2sb.h
+++ b/src/soc/intel/common/block/include/intelblocks/p2sb.h
@@ -29,6 +29,18 @@
 void p2sb_enable_bar(void);
 void p2sb_configure_hpet(void);

+union p2sb_bdf {
+	uint16_t raw;
+	struct {
+		uint16_t fn  : 3;
+		uint16_t dev : 5;
+		uint16_t bus : 8;
+	};
+};
+
+union p2sb_bdf p2sb_get_hpet_bdf(void);
+void p2sb_set_hpet_bdf(union p2sb_bdf bdf);
+
 /* SOC overrides */
 /*
  * Each SoC should implement EP Mask register to disable SB access
diff --git a/src/soc/intel/common/block/p2sb/p2sb.c b/src/soc/intel/common/block/p2sb/p2sb.c
index b5f9a07..93f711f 100644
--- a/src/soc/intel/common/block/p2sb/p2sb.c
+++ b/src/soc/intel/common/block/p2sb/p2sb.c
@@ -56,6 +56,22 @@
    pci_write_config8(PCH_DEV_P2SB, HPTC_OFFSET, HPTC_ADDR_ENABLE_BIT);
 }

+union p2sb_bdf p2sb_get_hpet_bdf(void)
+{
+	union p2sb_bdf bdf;
+	bool p2sb_state = p2sb_is_hiden();
+	p2sb_unhide();
+        bdf.raw = pci_read_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF);
+	if (p2sb_state)
+		p2sb_hide();
+	return bdf;
+}
+
+void p2sb_set_hpet_bdf(union p2sb_bdf bdf)
+{
+	pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, bdf.raw);
+}
+
 static void p2sb_set_hide_bit(int hide)
 {
    const uint16_t reg = PCH_P2SB_E0 + 1;

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47531 )

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47531/1/src/soc/intel/common/block/... 
File src/soc/intel/common/block/p2sb/p2sb.c:

https://review.coreboot.org/c/coreboot/+/47531/1/src/soc/intel/common/block/... 
PS1, Line 64:         bdf.raw = pci_read_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF);
code indent should use tabs where possible

https://review.coreboot.org/c/coreboot/+/47531/1/src/soc/intel/common/block/... 
PS1, Line 64:         bdf.raw = pci_read_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF);
please, no spaces at the start of a line

[Arthur Heymans (Code Review)]

Hello Patrick Rudolph,

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

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

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

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

soc/intel/common/block/p2sb: Add hpet BDF functions

This allows to get/set the HPET bus device function.

Change-Id: I8d72da8bc392aa144d167d31cde30cc71cd1396e
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/common/block/include/intelblocks/p2sb.h
M src/soc/intel/common/block/p2sb/p2sb.c
2 files changed, 28 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/47531/2

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

soc/intel/common/block/p2sb: Add hpet BDF functions

This allows to get/set the HPET bus device function.

Change-Id: I8d72da8bc392aa144d167d31cde30cc71cd1396e
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/common/block/include/intelblocks/p2sb.h
M src/soc/intel/common/block/p2sb/p2sb.c
2 files changed, 28 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/47531/4

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47531 )

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

Patch Set 6: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
File src/soc/intel/common/block/p2sb/p2sb.c:

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
PS6, Line 61: p2sb_unhide
Do we always need to call this? Or only when hidden?

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47531 )

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
File src/soc/intel/common/block/p2sb/p2sb.c:

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
PS6, Line 61: p2sb_unhide
> Do we always need to call this? Or only when hidden?

It's only necessary when it's hidden.

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47531 )

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
File src/soc/intel/common/block/p2sb/p2sb.c:

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
PS6, Line 61: p2sb_unhide
> > Do we always need to call this? Or only when hidden? […]

if (p2sb_state)
    p2sb_unhide();

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47531 )

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
File src/soc/intel/common/block/p2sb/p2sb.c:

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
PS6, Line 33: 	pci_write_config32(PCH_DEV_P2SB, PCI_BASE_ADDRESS_0, P2SB_BAR);
nit: put `raw` after the struct?

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
PS6, Line 59: 	union p2sb_bdf bdf;
I'd make these variables constant:

const bool was_hidden = p2sb_is_hidden();
 p2sb_unhide();
 const union p2sb_bdf bdf = { .raw = pci_read_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF) };
 if (was_hidden)
    p2sb_hide();
 return bdf;

Also, please rename `p2sb_state`. Thinking of the P2SB state as "true" or "false" makes my brain hurt. I've suggested `was_hidden`.

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Marc Jones, Christian Walter, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

soc/intel/common/block/p2sb: Add hpet BDF functions

This allows to get/set the HPET bus device function.

Change-Id: I8d72da8bc392aa144d167d31cde30cc71cd1396e
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/common/block/include/intelblocks/p2sb.h
M src/soc/intel/common/block/p2sb/p2sb.c
2 files changed, 31 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/47531/7

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47531 )

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

Patch Set 7:

(3 comments)

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
File src/soc/intel/common/block/p2sb/p2sb.c:

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
PS6, Line 33: 	pci_write_config32(PCH_DEV_P2SB, PCI_BASE_ADDRESS_0, P2SB_BAR);
> nit: put `raw` after the struct?

You mean in the union definition?

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
PS6, Line 59: 	union p2sb_bdf bdf;
> I'd make these variables constant: […]

Done

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
PS6, Line 61: p2sb_unhide
> if (p2sb_state) […]

Done

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47531 )

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

Patch Set 7: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
File src/soc/intel/common/block/p2sb/p2sb.c:

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
PS6, Line 33: 	pci_write_config32(PCH_DEV_P2SB, PCI_BASE_ADDRESS_0, P2SB_BAR);
> > nit: put `raw` after the struct? […]

Yes. No idea how this comment ended up here.

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47531 )

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

Patch Set 7: Code-Review+2

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Marc Jones, Christian Walter, Angel Pons, Patrick Rudolph,

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

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

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

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

soc/intel/common/block/p2sb: Add hpet BDF functions

This allows to get/set the HPET bus device function.

Change-Id: I8d72da8bc392aa144d167d31cde30cc71cd1396e
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/common/block/include/intelblocks/p2sb.h
M src/soc/intel/common/block/p2sb/p2sb.c
2 files changed, 31 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/47531/8

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47531 )

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
File src/soc/intel/common/block/p2sb/p2sb.c:

https://review.coreboot.org/c/coreboot/+/47531/6/src/soc/intel/common/block/... 
PS6, Line 33: 	pci_write_config32(PCH_DEV_P2SB, PCI_BASE_ADDRESS_0, P2SB_BAR);
> Yes. No idea how this comment ended up here.

Done

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47531 )

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

Patch Set 8: Code-Review+2

[Arthur Heymans (Code Review)]

Arthur Heymans has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47531 )

Change subject: soc/intel/common/block/p2sb: Add hpet BDF functions
......................................................................

soc/intel/common/block/p2sb: Add hpet BDF functions

This allows to get/set the HPET bus device function.

Change-Id: I8d72da8bc392aa144d167d31cde30cc71cd1396e
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47531
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/common/block/include/intelblocks/p2sb.h
M src/soc/intel/common/block/p2sb/p2sb.c
2 files changed, 31 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/common/block/include/intelblocks/p2sb.h b/src/soc/intel/common/block/include/intelblocks/p2sb.h
index 71f9c62..819b940 100644
--- a/src/soc/intel/common/block/include/intelblocks/p2sb.h
+++ b/src/soc/intel/common/block/include/intelblocks/p2sb.h
@@ -29,6 +29,18 @@
 void p2sb_enable_bar(void);
 void p2sb_configure_hpet(void);

+union p2sb_bdf {
+	struct {
+		uint16_t fn  : 3;
+		uint16_t dev : 5;
+		uint16_t bus : 8;
+	};
+	uint16_t raw;
+};
+
+union p2sb_bdf p2sb_get_hpet_bdf(void);
+void p2sb_set_hpet_bdf(union p2sb_bdf bdf);
+
 /* SOC overrides */
 /*
  * Each SoC should implement EP Mask register to disable SB access
diff --git a/src/soc/intel/common/block/p2sb/p2sb.c b/src/soc/intel/common/block/p2sb/p2sb.c
index 9673a2c..fd54a08 100644
--- a/src/soc/intel/common/block/p2sb/p2sb.c
+++ b/src/soc/intel/common/block/p2sb/p2sb.c
@@ -54,6 +54,25 @@
    pci_write_config8(PCH_DEV_P2SB, HPTC_OFFSET, HPTC_ADDR_ENABLE_BIT);
 }

+union p2sb_bdf p2sb_get_hpet_bdf(void)
+{
+	const bool was_hidden = p2sb_is_hidden();
+	if (was_hidden)
+		p2sb_unhide();
+
+	union p2sb_bdf bdf = { .raw = pci_read_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF) };
+
+	if (was_hidden)
+		p2sb_hide();
+
+	return bdf;
+}
+
+void p2sb_set_hpet_bdf(union p2sb_bdf bdf)
+{
+	pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, bdf.raw);
+}
+
 static void p2sb_set_hide_bit(int hide)
 {
    const uint16_t reg = PCH_P2SB_E0 + 1;