Change in coreboot[master]: ec/google/wilco: Set minimum UCSI_ACPI region length

[Bernardo Perez Priego (Code Review)]

Bernardo Perez Priego has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38414 )

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

ec/google/wilco: Set minimum UCSI_ACPI region length

In order to avoid conclict with another entity due to io mapping alignment,
we set minimum UCSI_ACPI length to be located outside of IMD small region.

BUG=144826008

Signed-off-by: Bernardo Perez Priego 
Change-Id: Id00e76dca240279773a95c8054831e05df390664
---
M src/ec/google/wilco/chip.c
1 file changed, 7 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/38414/1

diff --git a/src/ec/google/wilco/chip.c b/src/ec/google/wilco/chip.c
index 70e6124..a5fe429 100644
--- a/src/ec/google/wilco/chip.c
+++ b/src/ec/google/wilco/chip.c
@@ -27,6 +27,7 @@
 #include "ec.h"
 #include "chip.h"

+#define UCSI_MIN_ALLOC_REGION_LEN	CBMEM_SM_ROOT_SIZE
 /*
  * The UCSI fields are defined in the UCSI specification at
  * https://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-u...
@@ -175,19 +176,22 @@
 {
    struct opregion opreg;
    void *region_ptr;
+	size_t ucsi_alloc_region_len;

if (!dev->enabled)
    	return;

-	region_ptr = cbmem_add(CBMEM_ID_ACPI_UCSI, ucsi_region_len);
+	ucsi_alloc_region_len =	ucsi_region_len < UCSI_MIN_ALLOC_REGION_LEN?
+		UCSI_MIN_ALLOC_REGION_LEN : ucsi_region_len;
+	region_ptr = cbmem_add(CBMEM_ID_ACPI_UCSI, ucsi_alloc_region_len);
    if (!region_ptr)
    	return;
-	memset(region_ptr, 0, ucsi_region_len);
+	memset(region_ptr, 0, ucsi_alloc_region_len);

opreg.name = "UCSM";
    opreg.regionspace = SYSTEMMEMORY;
    opreg.regionoffset = (uintptr_t)region_ptr;
-	opreg.regionlen = ucsi_region_len;
+	opreg.regionlen = ucsi_alloc_region_len;

acpigen_write_scope(acpi_device_path_join(dev, "UCSI"));
    acpigen_write_name("_CRS");

[build bot (Jenkins) (Code Review)]

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

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38414/1/src/ec/google/wilco/chip.c 
File src/ec/google/wilco/chip.c:

https://review.coreboot.org/c/coreboot/+/38414/1/src/ec/google/wilco/chip.c@... 
PS1, Line 184: 	ucsi_alloc_region_len =	ucsi_region_len < UCSI_MIN_ALLOC_REGION_LEN?
spaces required around that '?' (ctx:VxE)

[Bernardo Perez Priego (Code Review)]

Bernardo Perez Priego has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/38414 )

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

ec/google/wilco: Set minimum UCSI_ACPI region length

In order to avoid conclict with another entity due to io mapping alignment,
we set minimum UCSI_ACPI length to be located outside of IMD small region.

BUG=144826008

Signed-off-by: Bernardo Perez Priego 
Change-Id: Id00e76dca240279773a95c8054831e05df390664
---
M src/ec/google/wilco/chip.c
1 file changed, 7 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/38414/2

[Bernardo Perez Priego (Code Review)]

Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38414 )

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38414/1/src/ec/google/wilco/chip.c 
File src/ec/google/wilco/chip.c:

https://review.coreboot.org/c/coreboot/+/38414/1/src/ec/google/wilco/chip.c@... 
PS1, Line 184: 	ucsi_alloc_region_len =	ucsi_region_len < UCSI_MIN_ALLOC_REGION_LEN?
> spaces required around that '?' (ctx:VxE)

Done

[Bernardo Perez Priego (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

ec/google/wilco: Set minimum UCSI_ACPI region length

In order to avoid conclict with another entity due to io mapping alignment,
we set minimum UCSI_ACPI length to be located outside of IMD small region.

BUG=b:144826008

Signed-off-by: Bernardo Perez Priego 
Change-Id: Id00e76dca240279773a95c8054831e05df390664
---
M src/ec/google/wilco/chip.c
1 file changed, 7 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/38414/3

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38414 )

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38414/3/src/ec/google/wilco/chip.c 
File src/ec/google/wilco/chip.c:

https://review.coreboot.org/c/coreboot/+/38414/3/src/ec/google/wilco/chip.c@... 
PS3, Line 30: UCSI_MIN_ALLOC_REGION_LEN
Could you add a comment describing why this specific allocation needs to be large enough so it doesn't end up in the IMD small region.  otherwise I'm sure this will confuse someone in the future.

https://review.coreboot.org/c/coreboot/+/38414/3/src/ec/google/wilco/chip.c@... 
PS3, Line 184: 	
looks like a tab?

[Bernardo Perez Priego (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

ec/google/wilco: Set minimum UCSI_ACPI region length

In order to avoid conclict with another entity due to io mapping alignment,
we set minimum UCSI_ACPI length to be located outside of IMD small region.

BUG=b:144826008

Signed-off-by: Bernardo Perez Priego 
Change-Id: Id00e76dca240279773a95c8054831e05df390664
---
M src/ec/google/wilco/chip.c
1 file changed, 11 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/38414/4

[Bernardo Perez Priego (Code Review)]

Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38414 )

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38414/3/src/ec/google/wilco/chip.c 
File src/ec/google/wilco/chip.c:

https://review.coreboot.org/c/coreboot/+/38414/3/src/ec/google/wilco/chip.c@... 
PS3, Line 30: UCSI_MIN_ALLOC_REGION_LEN
> Could you add a comment describing why this specific allocation needs to be large enough so it doesn […]

Done

https://review.coreboot.org/c/coreboot/+/38414/3/src/ec/google/wilco/chip.c@... 
PS3, Line 184:
> looks like a tab?

Done

[Patrick Rudolph (Code Review)]

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

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

Patch Set 4:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/38414/4//COMMIT_MSG@10 
PS4, Line 10: we set minimum UCSI_ACPI length to be located outside of IMD small region.
where is it placed then?
Why do you assume there will be no mapping problems?

[Mathew King (Code Review)]

Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38414 )

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

Patch Set 4: Code-Review+2

(1 comment)
> Patch Set 4:
>
> (1 comment)

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

https://review.coreboot.org/c/coreboot/+/38414/4//COMMIT_MSG@10 
PS4, Line 10: we set minimum UCSI_ACPI length to be located outside of IMD small region.
> where is it placed then? […]

The UCSI kernel driver maps the ACPI memory as not cached. Cache mapping is set on page boundaries and all IMD small is within the same page. If another driver maps the memory as write-back before the UCSI driver is loaded then the UCSI driver will fail to map the memory as not cached. By forcing the UCSI memory out of the IMD small it can map the memory correctly because it is in it's own page.

Bernardo, will you add some more detail to the commit message?

[Bernardo Perez Priego (Code Review)]

Hello Thejaswani Putta, AndreX Andraos, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins),

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

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

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

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

ec/google/wilco: Set minimum UCSI_ACPI region length

IMD provides support for small and large allocations. Region IMD Small memory is 1 KB
with 32 Bytes alignment, this region hold smaller entries without having to reserve a
whole 4 KB page. Remaining space is assigned to IMD Large to hold various regions with
4 KB alignment.

The UCSI kernel driver maps the UCSI_ACPI memory as not cached. Cache mapping is set
on page boundaries and all IMD Small is within the same page. If another driver maps
the memory as write-back before the UCSI driver is loaded then the UCSI driver will
fail to map the memory as not cached.

Placing UCSI_ACPI in IMD Large region will prevent this mapping issue since it will
now be located within its own page. This patch will force UCSI_ACPI region to be
located in IMD Large region instead.

BUG=b:144826008

Signed-off-by: Bernardo Perez Priego 
Change-Id: Id00e76dca240279773a95c8054831e05df390664
---
M src/ec/google/wilco/chip.c
1 file changed, 11 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/38414/5

[Bernardo Perez Priego (Code Review)]

Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38414 )

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

Patch Set 5:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/38414/4//COMMIT_MSG@10 
PS4, Line 10: we set minimum UCSI_ACPI length to be located outside of IMD small region.
> The UCSI kernel driver maps the ACPI memory as not cached. […]

Thanks for your response and feedback. I have updated patch description.

[Paul Menzel (Code Review)]

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

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

Patch Set 5:

(3 comments)

https://review.coreboot.org/c/coreboot/+/38414/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38414/5//COMMIT_MSG@10 
PS5, Line 10: hold
holds

https://review.coreboot.org/c/coreboot/+/38414/5//COMMIT_MSG@14 
PS5, Line 14: kernel
Linux kernel (what version)?

https://review.coreboot.org/c/coreboot/+/38414/5//COMMIT_MSG@21 
PS5, Line 21: instead
Remove

[Bernardo Perez Priego (Code Review)]

Hello Thejaswani Putta, AndreX Andraos, Mathew King, Selma Bensaid, Bora Guvendik, build bot (Jenkins),

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

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

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

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

ec/google/wilco: Set minimum UCSI_ACPI region length

IMD provides support for small and large allocations. Region IMD Small memory is 1 KB
with 32 Bytes alignment, this region holds smaller entries without having to reserve a
whole 4 KB page. Remaining space is assigned to IMD Large to hold various regions with
4 KB alignment.

The UCSI kernel (kernel version 4.19) driver maps the UCSI_ACPI memory as not cached.
Cache mapping is set on page boundaries and all IMD Small is within the same page.
If another driver maps the memory as write-back before the UCSI driver is loaded then
the UCSI driver will fail to map the memory as not cached.

Placing UCSI_ACPI in IMD Large region will prevent this mapping issue since it will
now be located within its own page. This patch will force UCSI_ACPI region to be
located in IMD Large region.

BUG=b:144826008

Signed-off-by: Bernardo Perez Priego 
Change-Id: Id00e76dca240279773a95c8054831e05df390664
---
M src/ec/google/wilco/chip.c
1 file changed, 11 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/38414/6

[Bernardo Perez Priego (Code Review)]

Bernardo Perez Priego has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38414 )

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

Patch Set 6:

(3 comments)

https://review.coreboot.org/c/coreboot/+/38414/5//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38414/5//COMMIT_MSG@10 
PS5, Line 10: hold
> holds

Done

https://review.coreboot.org/c/coreboot/+/38414/5//COMMIT_MSG@14 
PS5, Line 14: kernel
> Linux kernel (what version)?

Done

https://review.coreboot.org/c/coreboot/+/38414/5//COMMIT_MSG@21 
PS5, Line 21: instead
> Remove

Done

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38414 )

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

Patch Set 6: Code-Review+2

[Patrick Rudolph (Code Review)]

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

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

Patch Set 6:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/38414/4//COMMIT_MSG@10 
PS4, Line 10: we set minimum UCSI_ACPI length to be located outside of IMD small region.
> Thanks for your response and feedback. I have updated patch description.

Done

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38414 )

Change subject: ec/google/wilco: Set minimum UCSI_ACPI region length
......................................................................

ec/google/wilco: Set minimum UCSI_ACPI region length

IMD provides support for small and large allocations. Region IMD Small memory is 1 KB
with 32 Bytes alignment, this region holds smaller entries without having to reserve a
whole 4 KB page. Remaining space is assigned to IMD Large to hold various regions with
4 KB alignment.

The UCSI kernel (kernel version 4.19) driver maps the UCSI_ACPI memory as not cached.
Cache mapping is set on page boundaries and all IMD Small is within the same page.
If another driver maps the memory as write-back before the UCSI driver is loaded then
the UCSI driver will fail to map the memory as not cached.

Placing UCSI_ACPI in IMD Large region will prevent this mapping issue since it will
now be located within its own page. This patch will force UCSI_ACPI region to be
located in IMD Large region.

BUG=b:144826008

Signed-off-by: Bernardo Perez Priego 
Change-Id: Id00e76dca240279773a95c8054831e05df390664
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38414
Reviewed-by: Duncan Laurie 
Reviewed-by: Mathew King 
Tested-by: build bot (Jenkins) 
---
M src/ec/google/wilco/chip.c
1 file changed, 11 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Duncan Laurie: Looks good to me, approved
  Mathew King: Looks good to me, approved

diff --git a/src/ec/google/wilco/chip.c b/src/ec/google/wilco/chip.c
index 70e6124..b44cbd6 100644
--- a/src/ec/google/wilco/chip.c
+++ b/src/ec/google/wilco/chip.c
@@ -28,6 +28,11 @@
 #include "chip.h"

/*
+ * Setting minimum length of UCSI_ACPI will ensure this region is placed out of IMD Small.
+ * Having this region out of IMD Small will prevent any memory mapping conflicts.
+ */
+#define UCSI_MIN_ALLOC_REGION_LEN	CBMEM_SM_ROOT_SIZE
+/*
  * The UCSI fields are defined in the UCSI specification at
  * https://www.intel.com/content/www/us/en/io/universal-serial-bus/usb-type-c-u...
  * https://www.intel.com/content/www/us/en/io/universal-serial-bus/bios-impleme...
@@ -175,19 +180,22 @@
 {
    struct opregion opreg;
    void *region_ptr;
+	size_t ucsi_alloc_region_len;

if (!dev->enabled)
    	return;

-	region_ptr = cbmem_add(CBMEM_ID_ACPI_UCSI, ucsi_region_len);
+	ucsi_alloc_region_len = ucsi_region_len < UCSI_MIN_ALLOC_REGION_LEN ?
+		UCSI_MIN_ALLOC_REGION_LEN : ucsi_region_len;
+	region_ptr = cbmem_add(CBMEM_ID_ACPI_UCSI, ucsi_alloc_region_len);
    if (!region_ptr)
    	return;
-	memset(region_ptr, 0, ucsi_region_len);
+	memset(region_ptr, 0, ucsi_alloc_region_len);

opreg.name = "UCSM";
    opreg.regionspace = SYSTEMMEMORY;
    opreg.regionoffset = (uintptr_t)region_ptr;
-	opreg.regionlen = ucsi_region_len;
+	opreg.regionlen = ucsi_alloc_region_len;

acpigen_write_scope(acpi_device_path_join(dev, "UCSI"));
    acpigen_write_name("_CRS");