Change in coreboot[master]: soc/intel/xeon_sp: Change the return type of get_iio_stack_info()

[Arthur Heymans (Code Review)]

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

Change subject: soc/intel/xeon_sp: Change the return type of get_iio_stack_info()
......................................................................

soc/intel/xeon_sp: Change the return type of get_iio_stack_info()

This makes the function harder to understand and the return type is
not consistently used. Use a different helper function to get the HOB
Pci64BitResourceAllocation data.

Change-Id: I9a03cbb0ebbb48cc052d4c082d359c0087aaeb3e
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/chip_common.c
M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/cpx/soc_util.c
M src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/skx/soc_util.c
5 files changed, 13 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/47298/1

diff --git a/src/soc/intel/xeon_sp/chip_common.c b/src/soc/intel/xeon_sp/chip_common.c
index 5c78780..dd37e48 100644
--- a/src/soc/intel/xeon_sp/chip_common.c
+++ b/src/soc/intel/xeon_sp/chip_common.c
@@ -403,6 +403,13 @@
    }
 }

+static uint8_t is_pci64bit_alloc(void)
+{
+	const IIO_UDS *hob = get_iio_uds();
+
+	return hob->PlatformData.Pci64BitResourceAllocation;
+}
+
 static void xeonsp_pci_domain_read_resources(struct device *dev)
 {
    struct bus *link;
@@ -425,7 +432,8 @@
    	xeonsp_pci_dev_iterator(link, xeonsp_reset_pci_op, NULL, NULL);

struct iiostack_resource stack_info = {0};
-	uint8_t pci64bit_alloc_flag = get_iiostack_info(&stack_info);
+	uint8_t pci64bit_alloc_flag = is_pci64bit_alloc();
+	get_iiostack_info(&stack_info);
    if (!pci64bit_alloc_flag) {
    	/*
    	 * Split 32 bit address space between prefetchable and
diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h
index 5f4a6f9..07c454e 100644
--- a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h
+++ b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h
@@ -11,7 +11,7 @@
    STACK_RES   res[MAX_SOCKET * MAX_LOGIC_IIO_STACK];
 };

-uint8_t get_iiostack_info(struct iiostack_resource *info);
+void get_iiostack_info(struct iiostack_resource *info);

void xeonsp_init_cpu_config(void);
 const IIO_UDS *get_iio_uds(void);
diff --git a/src/soc/intel/xeon_sp/cpx/soc_util.c b/src/soc/intel/xeon_sp/cpx/soc_util.c
index 1c5a737..9e7072e 100644
--- a/src/soc/intel/xeon_sp/cpx/soc_util.c
+++ b/src/soc/intel/xeon_sp/cpx/soc_util.c
@@ -163,7 +163,7 @@
    }
 }

-uint8_t get_iiostack_info(struct iiostack_resource *info)
+void get_iiostack_info(struct iiostack_resource *info)
 {
    const IIO_UDS *hob = get_iio_uds();

@@ -178,8 +178,6 @@
    		}
    	}
    }
-
-	return hob->PlatformData.Pci64BitResourceAllocation;
 }

/* return true if command timed out else false */
diff --git a/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
index e0d66d6..93481c9 100644
--- a/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
+++ b/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
@@ -11,7 +11,7 @@
    STACK_RES   res[CONFIG_MAX_SOCKET * MAX_IIO_STACK];
 };

-uint8_t get_iiostack_info(struct iiostack_resource *info);
+void get_iiostack_info(struct iiostack_resource *info);

void xeonsp_init_cpu_config(void);
 const IIO_UDS *get_iio_uds(void);
diff --git a/src/soc/intel/xeon_sp/skx/soc_util.c b/src/soc/intel/xeon_sp/skx/soc_util.c
index 6d511ea..f9eddb2 100644
--- a/src/soc/intel/xeon_sp/skx/soc_util.c
+++ b/src/soc/intel/xeon_sp/skx/soc_util.c
@@ -291,7 +291,7 @@
    return soc_get_num_cpus() * get_threads_per_package();
 }

-uint8_t get_iiostack_info(struct iiostack_resource *info)
+void get_iiostack_info(struct iiostack_resource *info)
 {
    const IIO_UDS *hob = get_iio_uds();

@@ -311,8 +311,6 @@
    		memcpy(&info->res[info->no_of_stacks++], ri, sizeof(STACK_RES));
    	}
    }
-
-	return hob->PlatformData.Pci64BitResourceAllocation;
 }

#if ENV_RAMSTAGE

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47298 )

Change subject: soc/intel/xeon_sp: Change the return type of get_iio_stack_info()
......................................................................

Patch Set 1:

(1 comment)

Yes, I didn't like the introduction of that return type. I'd also like to de-dup this function, but they are slightly different and haven't confirmed the change for skx. This will need a rebase since several functions have moved out of soc_util.c

https://review.coreboot.org/c/coreboot/+/47298/1/src/soc/intel/xeon_sp/chip_... 
File src/soc/intel/xeon_sp/chip_common.c:

https://review.coreboot.org/c/coreboot/+/47298/1/src/soc/intel/xeon_sp/chip_... 
PS1, Line 437: pci64bit_alloc_flag
It could be called it here. No need for a flag variable.

[Arthur Heymans (Code Review)]

Hello Marc Jones, build bot (Jenkins), Jonathan Zhang, Patrick Rudolph,

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

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

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

Change subject: soc/intel/xeon_sp: Change the return type of get_iio_stack_info()
......................................................................

soc/intel/xeon_sp: Change the return type of get_iio_stack_info()

This makes the function harder to understand and the return type is
not consistently used. Use a different helper function to get the HOB
Pci64BitResourceAllocation data.

Change-Id: I9a03cbb0ebbb48cc052d4c082d359c0087aaeb3e
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/chip_common.c
M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/cpx/soc_util.c
M src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/skx/soc_util.c
5 files changed, 13 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/47298/2

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47298 )

Change subject: soc/intel/xeon_sp: Change the return type of get_iio_stack_info()
......................................................................

Patch Set 2: Code-Review+2

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Marc Jones, Jonathan Zhang, Patrick Rudolph,

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

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

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

Change subject: soc/intel/xeon_sp: Change the return type of get_iio_stack_info()
......................................................................

soc/intel/xeon_sp: Change the return type of get_iio_stack_info()

This makes the function harder to understand and the return type is
not consistently used. Use a different helper function to get the HOB
Pci64BitResourceAllocation data.

Change-Id: I9a03cbb0ebbb48cc052d4c082d359c0087aaeb3e
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/chip_common.c
M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/cpx/soc_util.c
M src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
4 files changed, 12 insertions(+), 7 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/47298/4

[Paul Menzel (Code Review)]

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

Change subject: soc/intel/xeon_sp: Change the return type of get_iio_stack_info()
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47298/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47298/4//COMMIT_MSG@9 
PS4, Line 9: This
The current code?

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Marc Jones, Jonathan Zhang, Patrick Rudolph,

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

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

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

Change subject: soc/intel/xeon_sp: Change the return type of get_iio_stack_info()
......................................................................

soc/intel/xeon_sp: Change the return type of get_iio_stack_info()

The somewhat unrelated return value makes the function harder to
understand and the return type is not consistently used. Use a
different helper function to get the HOB Pci64BitResourceAllocation
data.

Change-Id: I9a03cbb0ebbb48cc052d4c082d359c0087aaeb3e
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/chip_common.c
M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/cpx/soc_util.c
M src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/skx/soc_util.c
5 files changed, 13 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/47298/5

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Marc Jones, Jonathan Zhang, Patrick Rudolph,

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

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

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

Change subject: soc/intel/xeon_sp: Change the return type of get_iio_stack_info()
......................................................................

soc/intel/xeon_sp: Change the return type of get_iio_stack_info()

The somewhat unrelated return value makes the function harder to
understand and the return type is not consistently used. Use a
different helper function to get the HOB Pci64BitResourceAllocation
data.

Change-Id: I9a03cbb0ebbb48cc052d4c082d359c0087aaeb3e
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/chip_common.c
M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/cpx/soc_util.c
M src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/skx/soc_util.c
5 files changed, 13 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/47298/6

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47298 )

Change subject: soc/intel/xeon_sp: Change the return type of get_iio_stack_info()
......................................................................

Patch Set 6: Code-Review+2

[Arthur Heymans (Code Review)]

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

Change subject: soc/intel/xeon_sp: Change the return type of get_iio_stack_info()
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47298/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47298/4//COMMIT_MSG@9 
PS4, Line 9: This
> The current code?

Done

https://review.coreboot.org/c/coreboot/+/47298/1/src/soc/intel/xeon_sp/chip_... 
File src/soc/intel/xeon_sp/chip_common.c:

https://review.coreboot.org/c/coreboot/+/47298/1/src/soc/intel/xeon_sp/chip_... 
PS1, Line 437: pci64bit_alloc_flag
> It could be called it here. No need for a flag variable.

Done

[Arthur Heymans (Code Review)]

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

Change subject: soc/intel/xeon_sp: Change the return type of get_iio_stack_info()
......................................................................

soc/intel/xeon_sp: Change the return type of get_iio_stack_info()

The somewhat unrelated return value makes the function harder to
understand and the return type is not consistently used. Use a
different helper function to get the HOB Pci64BitResourceAllocation
data.

Change-Id: I9a03cbb0ebbb48cc052d4c082d359c0087aaeb3e
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47298
Tested-by: build bot (Jenkins) 
Reviewed-by: Marc Jones 
---
M src/soc/intel/xeon_sp/chip_common.c
M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/cpx/soc_util.c
M src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
M src/soc/intel/xeon_sp/skx/soc_util.c
5 files changed, 13 insertions(+), 10 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Marc Jones: Looks good to me, approved

diff --git a/src/soc/intel/xeon_sp/chip_common.c b/src/soc/intel/xeon_sp/chip_common.c
index 5c78780..47d8f5c 100644
--- a/src/soc/intel/xeon_sp/chip_common.c
+++ b/src/soc/intel/xeon_sp/chip_common.c
@@ -403,6 +403,13 @@
    }
 }

+static uint8_t is_pci64bit_alloc(void)
+{
+	const IIO_UDS *hob = get_iio_uds();
+
+	return hob->PlatformData.Pci64BitResourceAllocation;
+}
+
 static void xeonsp_pci_domain_read_resources(struct device *dev)
 {
    struct bus *link;
@@ -425,8 +432,8 @@
    	xeonsp_pci_dev_iterator(link, xeonsp_reset_pci_op, NULL, NULL);

struct iiostack_resource stack_info = {0};
-	uint8_t pci64bit_alloc_flag = get_iiostack_info(&stack_info);
-	if (!pci64bit_alloc_flag) {
+	get_iiostack_info(&stack_info);
+	if (!is_pci64bit_alloc()) {
    	/*
    	 * Split 32 bit address space between prefetchable and
    	 * non-prefetchable windows
diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h
index 649e6b5..e3c971d 100644
--- a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h
+++ b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h
@@ -11,7 +11,7 @@
    STACK_RES   res[MAX_SOCKET * MAX_LOGIC_IIO_STACK];
 };

-uint8_t get_iiostack_info(struct iiostack_resource *info);
+void get_iiostack_info(struct iiostack_resource *info);

const struct SystemMemoryMapHob *get_system_memory_map(void);

diff --git a/src/soc/intel/xeon_sp/cpx/soc_util.c b/src/soc/intel/xeon_sp/cpx/soc_util.c
index d2d12d1..242fcfe 100644
--- a/src/soc/intel/xeon_sp/cpx/soc_util.c
+++ b/src/soc/intel/xeon_sp/cpx/soc_util.c
@@ -27,7 +27,7 @@
    return *memmap_addr;
 }

-uint8_t get_iiostack_info(struct iiostack_resource *info)
+void get_iiostack_info(struct iiostack_resource *info)
 {
    const IIO_UDS *hob = get_iio_uds();

@@ -42,8 +42,6 @@
    		}
    	}
    }
-
-	return hob->PlatformData.Pci64BitResourceAllocation;
 }

uint32_t get_socket_stack_busno(uint32_t socket, uint32_t stack)
diff --git a/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
index b42aec0..0f52811 100644
--- a/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
+++ b/src/soc/intel/xeon_sp/skx/include/soc/soc_util.h
@@ -11,7 +11,7 @@
    STACK_RES   res[CONFIG_MAX_SOCKET * MAX_IIO_STACK];
 };

-uint8_t get_iiostack_info(struct iiostack_resource *info);
+void get_iiostack_info(struct iiostack_resource *info);

void config_reset_cpl3_csrs(void);

diff --git a/src/soc/intel/xeon_sp/skx/soc_util.c b/src/soc/intel/xeon_sp/skx/soc_util.c
index af6dfcc3..d5e1ae1 100644
--- a/src/soc/intel/xeon_sp/skx/soc_util.c
+++ b/src/soc/intel/xeon_sp/skx/soc_util.c
@@ -55,7 +55,7 @@
    return memmap_addr;
 }

-uint8_t get_iiostack_info(struct iiostack_resource *info)
+void get_iiostack_info(struct iiostack_resource *info)
 {
    const IIO_UDS *hob = get_iio_uds();

@@ -71,8 +71,6 @@
    		memcpy(&info->res[info->no_of_stacks++], ri, sizeof(STACK_RES));
    	}
    }
-
-	return hob->PlatformData.Pci64BitResourceAllocation;
 }

uint32_t get_socket_stack_busno(uint32_t socket, uint32_t stack)