Change in coreboot[master]: soc/amd/picassso/acpi: increase MMIO region size of GPIO controller

[Felix Held (Code Review)]

Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48270 )

Change subject: soc/amd/picassso/acpi: increase MMIO region size of GPIO controller
......................................................................

soc/amd/picassso/acpi: increase MMIO region size of GPIO controller

The GPIO controller on Picasso has 4 banks of GPIOs with a size of 256
bytes each, so increase the reserved size to match the hardware.

Change-Id: I453f1c531d612a0e82ee0d91762fec6cdb2b8556
Signed-off-by: Felix Held 
---
M src/soc/amd/picasso/acpi/sb_fch.asl
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/48270/1

diff --git a/src/soc/amd/picasso/acpi/sb_fch.asl b/src/soc/amd/picasso/acpi/sb_fch.asl
index 6cbfc5f..0401313 100644
--- a/src/soc/amd/picasso/acpi/sb_fch.asl
+++ b/src/soc/amd/picasso/acpi/sb_fch.asl
@@ -34,7 +34,7 @@
    			ActiveLow,
    			Exclusive, , , IRQR)
    		{ 0 }
-			Memory32Fixed (ReadWrite, 0xFED81500, 0x300)
+			Memory32Fixed (ReadWrite, 0xFED81500, 0x400)
    	}
    	CreateDWordField (Local0, IRQR._INT, IRQN)
    	If (PMOD) {
@@ -44,7 +44,7 @@
    	}
    	If (IRQN == 0x1f) {
    		Return (ResourceTemplate() {
-				Memory32Fixed (ReadWrite, 0xFED81500, 0x300)
+				Memory32Fixed (ReadWrite, 0xFED81500, 0x400)
    		})
    	} Else {
    		Return (Local0)

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48270 )

Change subject: soc/amd/picassso/acpi: increase MMIO region size of GPIO controller
......................................................................

Patch Set 1: Code-Review+2

(2 comments)

https://review.coreboot.org/c/coreboot/+/48270/1/src/soc/amd/picasso/acpi/sb... 
File src/soc/amd/picasso/acpi/sb_fch.asl:

https://review.coreboot.org/c/coreboot/+/48270/1/src/soc/amd/picasso/acpi/sb... 
PS1, Line 37: 0xFED81500
ACPIMMIO_GPIO0_BASE

https://review.coreboot.org/c/coreboot/+/48270/1/src/soc/amd/picasso/acpi/sb... 
PS1, Line 47: 0xFED81500
ACPIMMIO_GPIO0_BASE

[Felix Held (Code Review)]

Hello build bot (Jenkins), Jason Glenesk, Furquan Shaikh, Martin Roth, Marshall Dawson,

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

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

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

Change subject: soc/amd/picassso/acpi: increase MMIO region size of GPIO controller
......................................................................

soc/amd/picassso/acpi: increase MMIO region size of GPIO controller

The GPIO controller on Picasso has 4 banks of GPIOs with a size of 256
bytes each, so increase the reserved size to match the hardware.

Also replace the base GPIO address with the corresponding define.

Change-Id: I453f1c531d612a0e82ee0d91762fec6cdb2b8556
Signed-off-by: Felix Held 
---
M src/soc/amd/picasso/acpi/sb_fch.asl
1 file changed, 2 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/48270/2

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48270 )

Change subject: soc/amd/picassso/acpi: increase MMIO region size of GPIO controller
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/48270/1/src/soc/amd/picasso/acpi/sb... 
File src/soc/amd/picasso/acpi/sb_fch.asl:

https://review.coreboot.org/c/coreboot/+/48270/1/src/soc/amd/picasso/acpi/sb... 
PS1, Line 37: 0xFED81500
> ACPIMMIO_GPIO0_BASE

good point. fixed

https://review.coreboot.org/c/coreboot/+/48270/1/src/soc/amd/picasso/acpi/sb... 
PS1, Line 47: 0xFED81500
> ACPIMMIO_GPIO0_BASE

Done

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48270 )

Change subject: soc/amd/picassso/acpi: increase MMIO region size of GPIO controller
......................................................................

Patch Set 2: Code-Review+2

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48270 )

Change subject: soc/amd/picassso/acpi: increase MMIO region size of GPIO controller
......................................................................

Patch Set 2:

I'm not seeing it yet.  Can you let me know (offline's OK) what doc you're looking at?

[Furquan Shaikh (Code Review)]

Furquan Shaikh has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/48270 )

Change subject: soc/amd/picassso/acpi: increase MMIO region size of GPIO controller
......................................................................

Removed Code-Review+2 by Furquan Shaikh

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48270 )

Change subject: soc/amd/picassso/acpi: increase MMIO region size of GPIO controller
......................................................................

Patch Set 2:

waiting for confirmation from Marshall/Felix based on latest comment.

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48270 )

Change subject: soc/amd/picassso/acpi: increase MMIO region size of GPIO controller
......................................................................

Patch Set 2:
> Patch Set 2:
>
> I'm not seeing it yet.  Can you let me know (offline's OK) what doc you're looking at?

see the chapter FCH/registers/GPIO pin control registers/GPIO registers of #55570. There are 4 banks of GPIOs (0-3) and they start 0x100 bytes apart from each other, so 0x400 bytes in total

[Marshall Dawson (Code Review)]

Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48270 )

Change subject: soc/amd/picassso/acpi: increase MMIO region size of GPIO controller
......................................................................

Patch Set 2: Code-Review+2
> Patch Set 2:
> > Patch Set 2:
> >
> > I'm not seeing it yet.  Can you let me know (offline's OK) what doc you're looking at?
>
> see the chapter FCH/registers/GPIO pin control registers/GPIO registers of #55570. There are 4 banks of GPIOs (0-3) and they start 0x100 bytes apart from each other, so 0x400 bytes in total

I assumed you were right, of course.  I've always referred to the PMx000 descriptions for the mappings (See "ACPI MMIO Space Allocation"), aka PMx00 in previous generations.  Not sure when we added the ability to support a 4th bank, but it looks like someone forgot to update the table.

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48270 )

Change subject: soc/amd/picassso/acpi: increase MMIO region size of GPIO controller
......................................................................

soc/amd/picassso/acpi: increase MMIO region size of GPIO controller

The GPIO controller on Picasso has 4 banks of GPIOs with a size of 256
bytes each, so increase the reserved size to match the hardware.

Also replace the base GPIO address with the corresponding define.

Change-Id: I453f1c531d612a0e82ee0d91762fec6cdb2b8556
Signed-off-by: Felix Held 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48270
Tested-by: build bot (Jenkins) 
Reviewed-by: Marshall Dawson 
---
M src/soc/amd/picasso/acpi/sb_fch.asl
1 file changed, 2 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Marshall Dawson: Looks good to me, approved

diff --git a/src/soc/amd/picasso/acpi/sb_fch.asl b/src/soc/amd/picasso/acpi/sb_fch.asl
index 6cbfc5f..c4dffed 100644
--- a/src/soc/amd/picasso/acpi/sb_fch.asl
+++ b/src/soc/amd/picasso/acpi/sb_fch.asl
@@ -34,7 +34,7 @@
    			ActiveLow,
    			Exclusive, , , IRQR)
    		{ 0 }
-			Memory32Fixed (ReadWrite, 0xFED81500, 0x300)
+			Memory32Fixed (ReadWrite, ACPIMMIO_GPIO0_BASE, 0x400)
    	}
    	CreateDWordField (Local0, IRQR._INT, IRQN)
    	If (PMOD) {
@@ -44,7 +44,7 @@
    	}
    	If (IRQN == 0x1f) {
    		Return (ResourceTemplate() {
-				Memory32Fixed (ReadWrite, 0xFED81500, 0x300)
+				Memory32Fixed (ReadWrite, ACPIMMIO_GPIO0_BASE, 0x400)
    		})
    	} Else {
    		Return (Local0)

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48270 )

Change subject: soc/amd/picassso/acpi: increase MMIO region size of GPIO controller
......................................................................

Patch Set 3:

Stoneyridge only has 3 GPIO banks, so I guess that was just copy-pasted from there