Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel: Make GPIO ASL helper function unified across SoC ......................................................................
soc/intel: Make GPIO ASL helper function unified across SoC
List of changes: 1. Make CNL gpio_op.asl unified as TGL gpio_op.asl 2. Refer GPIO state macros from intelblocks/gpio_defs.h 3. Delete unused gpio_common.h from CNL SoC 4. Make ASL helper function like GRXS, GTXS, STXS, CTXS unified across ICL, JSL, SKL
TEST=Able to build and boot CNL and CML platform. 1) Dump and disassemble DSDT, verify unified methods like GRXS, GTXS etc. are there. 2) Verify no ACPI error seen while running 'dmesg` from console.
Change-Id: I78d712eeba56b9c098dc6a6f11e4e51cb2529b10 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/acpi/gpio_op.asl D src/soc/intel/cannonlake/include/soc/gpio_common.h M src/soc/intel/cannonlake/include/soc/gpio_defs.h M src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h M src/soc/intel/icelake/acpi/gpio.asl M src/soc/intel/icelake/include/soc/gpio_defs.h M src/soc/intel/jasperlake/acpi/gpio_op.asl M src/soc/intel/skylake/acpi/gpio.asl M src/soc/intel/tigerlake/acpi/gpio_op.asl 9 files changed, 27 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/45654/1
diff --git a/src/soc/intel/cannonlake/acpi/gpio_op.asl b/src/soc/intel/cannonlake/acpi/gpio_op.asl index 3c0ed66..8af1e0c 100644 --- a/src/soc/intel/cannonlake/acpi/gpio_op.asl +++ b/src/soc/intel/cannonlake/acpi/gpio_op.asl @@ -1,4 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <intelblocks/gpio_defs.h>
/* * Get GPIO Value @@ -11,7 +12,7 @@ { VAL0, 32 } - And (GPIORXSTATE_MASK, ShiftRight (VAL0, GPIORXSTATE_SHIFT), Local0) + Local0 = PAD_CFG0_RX_STATE & (VAL0 >> PAD_CFG0_RX_STATE_BIT)
Return (Local0) } @@ -27,7 +28,7 @@ { VAL0, 32 } - And (GPIOTXSTATE_MASK, VAL0, Local0) + Local0 = PAD_CFG0_TX_STATE & VAL0
Return (Local0) } @@ -43,7 +44,7 @@ { VAL0, 32 } - Or (GPIOTXSTATE_MASK, VAL0, VAL0) + VAL0 = PAD_CFG0_TX_STATE | VAL0 }
/* @@ -57,7 +58,7 @@ { VAL0, 32 } - And (Not (GPIOTXSTATE_MASK), VAL0, VAL0) + VAL0 = ~PAD_CFG0_TX_STATE & VAL0 }
/* @@ -76,10 +77,9 @@ { VAL0, 32 } - Store (VAL0, Local0) - And (Not (GPIOPADMODE_MASK), Local0, Local0) - And (ShiftLeft (Arg1, GPIOPADMODE_SHIFT, Arg1), GPIOPADMODE_MASK, Arg1) - Or (Local0, Arg1, VAL0) + Local0 = ~PAD_CFG0_MODE_MASK & VAL0 + Arg1 = (Arg1 << PAD_CFG0_MODE_SHIFT) & PAD_CFG0_MODE_MASK + VAL0 = Local0 | Arg1 }
/* @@ -97,10 +97,10 @@ VAL0, 32 }
- If (LEqual (Arg1, 1)) { - And (Not (GPIOTXBUFDIS_MASK), VAL0, VAL0) - } ElseIf (LEqual (Arg1, 0)){ - Or (GPIOTXBUFDIS_MASK, VAL0, VAL0) + If (Arg1 == 1) { + VAL0 = ~PAD_CFG0_TX_DISABLE & VAL0 + } ElseIf (Arg1 == 0){ + VAL0 = PAD_CFG0_TX_DISABLE | VAL0 } }
@@ -119,9 +119,9 @@ VAL0, 32 }
- If (LEqual (Arg1, 1)) { - And (Not (GPIORXBUFDIS_MASK), VAL0, VAL0) - } ElseIf (LEqual (Arg1, 0)){ - Or (GPIORXBUFDIS_MASK, VAL0, VAL0) + If (Arg1 == 1) { + VAL0 = ~PAD_CFG0_RX_DISABLE & VAL0 + } ElseIf (Arg1 == 0){ + VAL0 = PAD_CFG0_RX_DISABLE | VAL0 } } diff --git a/src/soc/intel/cannonlake/include/soc/gpio_common.h b/src/soc/intel/cannonlake/include/soc/gpio_common.h deleted file mode 100644 index c11ef50..0000000 --- a/src/soc/intel/cannonlake/include/soc/gpio_common.h +++ /dev/null @@ -1,13 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -#ifndef _SOC_CANNONLAKE_GPIO_COMMON_H_ -#define _SOC_CANNONLAKE_GPIO_COMMON_H_ - -#define GPIORXSTATE_MASK 0x1 -#define GPIORXSTATE_SHIFT 1 -#define GPIOTXSTATE_MASK 0x1 -#define GPIOPADMODE_MASK 0xC00 -#define GPIOPADMODE_SHIFT 10 -#define GPIOTXBUFDIS_MASK 0x100 -#define GPIORXBUFDIS_MASK 0x200 - -#endif diff --git a/src/soc/intel/cannonlake/include/soc/gpio_defs.h b/src/soc/intel/cannonlake/include/soc/gpio_defs.h index 9b1690e..e7769b5 100644 --- a/src/soc/intel/cannonlake/include/soc/gpio_defs.h +++ b/src/soc/intel/cannonlake/include/soc/gpio_defs.h @@ -6,7 +6,6 @@ #ifndef __ACPI__ #include <stddef.h> #endif -#include <soc/gpio_common.h> #include <soc/gpio_soc_defs.h>
#define GPIO_NUM_PAD_CFG_REGS 4 /* DW0, DW1, DW2, DW3 */ diff --git a/src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h b/src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h index a1f51d1..bd68b04 100644 --- a/src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h +++ b/src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h @@ -6,7 +6,6 @@ #ifndef __ACPI__ #include <stddef.h> #endif -#include <soc/gpio_common.h> #include <soc/gpio_soc_defs_cnp_h.h>
#define GPIO_NUM_PAD_CFG_REGS 4 /* DW0, DW1, DW2, DW3 */ diff --git a/src/soc/intel/icelake/acpi/gpio.asl b/src/soc/intel/icelake/acpi/gpio.asl index 43aa83c..823f9cc 100644 --- a/src/soc/intel/icelake/acpi/gpio.asl +++ b/src/soc/intel/icelake/acpi/gpio.asl @@ -1,4 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <intelblocks/gpio_defs.h> #include <soc/gpio_defs.h> #include <soc/irq.h> #include <soc/pcr_ids.h> @@ -114,7 +115,7 @@ { VAL0, 32 } - And (GPIORXSTATE_MASK, ShiftRight (VAL0, GPIORXSTATE_SHIFT), Local0) + Local0 = PAD_CFG0_RX_STATE & (VAL0 >> PAD_CFG0_RX_STATE_BIT)
Return (Local0) } diff --git a/src/soc/intel/icelake/include/soc/gpio_defs.h b/src/soc/intel/icelake/include/soc/gpio_defs.h index 57701e1..577ca5f 100644 --- a/src/soc/intel/icelake/include/soc/gpio_defs.h +++ b/src/soc/intel/icelake/include/soc/gpio_defs.h @@ -257,6 +257,4 @@ #define GPI_SMI_EN_0 0x1A0 #define PAD_CFG_BASE 0x600
-#define GPIORXSTATE_MASK 0x1 -#define GPIORXSTATE_SHIFT 1 #endif diff --git a/src/soc/intel/jasperlake/acpi/gpio_op.asl b/src/soc/intel/jasperlake/acpi/gpio_op.asl index 683686f..8af1e0c 100644 --- a/src/soc/intel/jasperlake/acpi/gpio_op.asl +++ b/src/soc/intel/jasperlake/acpi/gpio_op.asl @@ -1,4 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <intelblocks/gpio_defs.h>
/* * Get GPIO Value @@ -76,9 +77,8 @@ { VAL0, 32 } - Local0 = VAL0 - Local0 = ~PAD_CFG0_MODE_MASK & Local0 - Arg1 = (Arg1 <<= PAD_CFG0_MODE_SHIFT) & PAD_CFG0_MODE_MASK + Local0 = ~PAD_CFG0_MODE_MASK & VAL0 + Arg1 = (Arg1 << PAD_CFG0_MODE_SHIFT) & PAD_CFG0_MODE_MASK VAL0 = Local0 | Arg1 }
diff --git a/src/soc/intel/skylake/acpi/gpio.asl b/src/soc/intel/skylake/acpi/gpio.asl index 60e1cf5..5dba1bb 100644 --- a/src/soc/intel/skylake/acpi/gpio.asl +++ b/src/soc/intel/skylake/acpi/gpio.asl @@ -1,9 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <intelblocks/gpio_defs.h> #include <soc/gpio.h>
-#define GPIOTXSTATE_MASK 0x1 -#define GPIORXSTATE_MASK 0x1 - Device (GPIO) { Name (_HID, "INT344B") @@ -119,7 +117,7 @@ { VAL0, 32 } - And (GPIORXSTATE_MASK, ShiftRight (VAL0, PAD_CFG0_RX_STATE_BIT), Local0) + Local0 = PAD_CFG0_RX_STATE & (VAL0 >> PAD_CFG0_RX_STATE_BIT)
Return (Local0) } @@ -135,7 +133,7 @@ { VAL0, 32 } - And (GPIOTXSTATE_MASK, ShiftRight (VAL0, PAD_CFG0_TX_STATE_BIT), Local0) + Local0 = PAD_CFG0_TX_STATE & VAL0
Return (Local0) } @@ -151,7 +149,7 @@ { VAL0, 32 } - Or (GPIOTXSTATE_MASK, VAL0, VAL0) + VAL0 = PAD_CFG0_TX_STATE | VAL0 }
/* @@ -165,5 +163,5 @@ { VAL0, 32 } - And (Not (GPIOTXSTATE_MASK), VAL0, VAL0) + VAL0 = ~PAD_CFG0_TX_STATE & VAL0 } diff --git a/src/soc/intel/tigerlake/acpi/gpio_op.asl b/src/soc/intel/tigerlake/acpi/gpio_op.asl index f7332aa..8af1e0c 100644 --- a/src/soc/intel/tigerlake/acpi/gpio_op.asl +++ b/src/soc/intel/tigerlake/acpi/gpio_op.asl @@ -1,4 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <intelblocks/gpio_defs.h>
/* * Get GPIO Value
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel: Make GPIO ASL helper function unified across SoC ......................................................................
Patch Set 1: Code-Review+1
(5 comments)
The changes look good, but they could be split up to ease review and verification through reproducible builds.
I should probably write a tutorial about reproducible builds. For now, this long comment will have to do:
=== What are reproducible builds? ===
Reproducible builds are coreboot images stripped of things like version, timestamps... This allows binaries to be easily compared with e.g. `diff`. To build reproducible binaries, one can run `make BUILD_TIMELESS=1 -j$(nproc)`. Abuild also has a `--timeless` option to build reproducible images.
=== Why are reproducible builds awesome? ===
Reproducible builds provide a mechanism to verify commits that do not intend to change the behavior of the code. This is extremely useful to verify code rewrites, factoring out common code and cosmetic cleanups. When a commit does not change the resulting coreboot image, boot-testing it isn't necessary: if the board was working before the commit, it will work after the commit.
I usually try to change a small thing and check if the coreboot image has changed. A one-liner I use is `make BUILD_TIMELESS=1 -j$(nproc) && diff -s -q before.rom build/coreboot.rom`. Before changing anything, I build a timeless coreboot image and rename it to `before.rom`. Checking every few changes makes it easy to spot where reproducibility broke.
=== Which kinds of changes can be verified with reproducible builds? ===
For example, cleaning up cosmetics, moving things between headers, renaming macros, replacing bare register offsets with macros, rewriting gpio arrays with macros, replacing raw Azalia verbs with macros... Folding mainboards into variants and conversions to overridetrees should also be reproducible, but I have little experience with them.
One thing to consider is that, by default, the .config file is included in coreboot images. This is controllable by Kconfig. I generally disable this, especially when the changes I want to test are expected to result in different config files. For example, when folding multiple mainboards together using the variants mechanism, it is possible to obtain an identical binary even when the config has changed.
Converting files to ASL 2.0 syntax is usually reproducible, but there can be some edge cases where IASL realizes it can optimize something differently.
There can be corner cases where something isn't reproducible for some reason. I isolate the problematic cases and handle them in a separate commit, either before or after the reproducible commit.
Moving function definitions between compilation units is usually not reproducible. If I have to factor out such cases, I usually resort to temporarily including a .c file from other .c files, then fix it in a follow-up. The first commit has a big diffstat but is reproducible, whereas the second one changes the binary but is tiny.
I've refactored Sandy Bridge native raminit several times with the help of timeless builds. I think the craziest (ab)use of timeless builds I've done is when I introduced an API to program sequences of DRAM commands into the IOSAV registers.
Here's the chronologically-sorted list of commits: CB:40972 CB:40973 CB:40976 CB:40971 CB:40977 CB:40978 CB:40982 CB:40983 CB:40981 CB:40980 CB:40984 CB:41003
https://review.coreboot.org/c/coreboot/+/45654/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45654/1//COMMIT_MSG@9 PS1, Line 9: List When you have a list of changes in a commit message, you should be suspicious: Shouldn't this be a list of commits instead, each doing one thing? 😊
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio_op.asl:
PS1: I think the conversion to ASL 2.0 syntax can be done in its own commit. It should result in a reproducible build (using BUILD_TIMELESS=1), too.
And please, one commit per platform. It may seem like a waste of commits, but commits are fungible material (like soldering tin or thermal paste). Plus, I (and possibly others) can +2 small, localized commits much more easily than large commits touching multiple platforms. 😉
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/gpio_common.h:
PS1: Dropping this in favor of common definitions is something best done in its own commit. It should even be a reproducible change.
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/jasperlake/ac... File src/soc/intel/jasperlake/acpi/gpio_op.asl:
PS1: Same here, the conversion to ASL 2.0 syntax would be best done in a separate commit.
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/skylake/acpi/... File src/soc/intel/skylake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/skylake/acpi/... PS1, Line 120: Local0 = PAD_CFG0_RX_STATE & (VAL0 >> PAD_CFG0_RX_STATE_BIT) Same here, the conversion to ASL 2.0 syntax would be best done in a separate commit.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45654
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Make CNL gpio_op.asl unified as TGL ......................................................................
soc/intel/cannonlake: Make CNL gpio_op.asl unified as TGL
Also delete unused gpio_common.h from CNL SoC
TEST=Able to build and boot CNL and CML platform. 1) Dump and disassemble DSDT, verify unified methods like GRXS, GTXS etc. are there. 2) Verify no ACPI error seen while running 'dmesg` from console.
Change-Id: I78d712eeba56b9c098dc6a6f11e4e51cb2529b10 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/acpi/gpio_op.asl D src/soc/intel/cannonlake/include/soc/gpio_common.h M src/soc/intel/cannonlake/include/soc/gpio_defs.h M src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h 4 files changed, 11 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/45654/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Make CNL gpio_op.asl unified as TGL ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/45654/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45654/1//COMMIT_MSG@9 PS1, Line 9: List
When you have a list of changes in a commit message, you should be suspicious: Shouldn't this be a l […]
Ack
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio_op.asl:
PS1:
I think the conversion to ASL 2.0 syntax can be done in its own commit. […]
Ack
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/gpio_common.h:
PS1:
Dropping this in favor of common definitions is something best done in its own commit. […]
Ack
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/jasperlake/ac... File src/soc/intel/jasperlake/acpi/gpio_op.asl:
PS1:
Same here, the conversion to ASL 2.0 syntax would be best done in a separate commit.
Ack
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/skylake/acpi/... File src/soc/intel/skylake/acpi/gpio.asl:
https://review.coreboot.org/c/coreboot/+/45654/1/src/soc/intel/skylake/acpi/... PS1, Line 120: Local0 = PAD_CFG0_RX_STATE & (VAL0 >> PAD_CFG0_RX_STATE_BIT)
Same here, the conversion to ASL 2.0 syntax would be best done in a separate commit.
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Make CNL gpio_op.asl unified as TGL ......................................................................
Patch Set 2: Code-Review+2
(3 comments)
LGTM, thank you! Just a few nits in the commit message.
https://review.coreboot.org/c/coreboot/+/45654/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45654/2//COMMIT_MSG@7 PS2, Line 7: Make CNL gpio_op.asl unified as TGL Align gpio_op.asl with TGL
https://review.coreboot.org/c/coreboot/+/45654/2//COMMIT_MSG@9 PS2, Line 9: Also delete unused gpio_common.h from CNL SoC Maybe:
Also drop gpio_common.h in favor of intelblocks/gpio_defs.h macros.
https://review.coreboot.org/c/coreboot/+/45654/2//COMMIT_MSG@14 PS2, Line 14: 'dmesg` nit: use matching quotes? left one is a single quote, right one is a backtick
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45654
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
soc/intel/cannonlake: Align gpio_op.asl with TGL
Also drop gpio_common.h in favor of intelblocks/gpio_defs.h macros.
TEST=Able to build and boot CNL and CML platform. 1) Dump and disassemble DSDT, verify unified methods like GRXS, GTXS etc. are there. 2) Verify no ACPI error seen while running 'dmesg' from console.
Change-Id: I78d712eeba56b9c098dc6a6f11e4e51cb2529b10 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/acpi/gpio_op.asl D src/soc/intel/cannonlake/include/soc/gpio_common.h M src/soc/intel/cannonlake/include/soc/gpio_defs.h M src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h 4 files changed, 11 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/45654/3
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45654/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45654/2//COMMIT_MSG@7 PS2, Line 7: Make CNL gpio_op.asl unified as TGL
Align gpio_op. […]
Ack
https://review.coreboot.org/c/coreboot/+/45654/2//COMMIT_MSG@9 PS2, Line 9: Also delete unused gpio_common.h from CNL SoC
Maybe: […]
Ack
https://review.coreboot.org/c/coreboot/+/45654/2//COMMIT_MSG@14 PS2, Line 14: 'dmesg`
nit: use matching quotes? left one is a single quote, right one is a backtick
Ack
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45654/3/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio_op.asl:
https://review.coreboot.org/c/coreboot/+/45654/3/src/soc/intel/cannonlake/ac... PS3, Line 47: VAL0 = PAD_CFG0_TX_STATE | VAL0 VAL0 |= PAD_CFG0_TX_STATE
https://review.coreboot.org/c/coreboot/+/45654/3/src/soc/intel/cannonlake/ac... PS3, Line 61: VAL0 = ~PAD_CFG0_TX_STATE & VAL0 VAL0 &= ~PAD_CFG0_TX_STATE
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45654/3/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio_op.asl:
https://review.coreboot.org/c/coreboot/+/45654/3/src/soc/intel/cannonlake/ac... PS3, Line 47: VAL0 = PAD_CFG0_TX_STATE | VAL0
VAL0 |= PAD_CFG0_TX_STATE
Ack
https://review.coreboot.org/c/coreboot/+/45654/3/src/soc/intel/cannonlake/ac... PS3, Line 61: VAL0 = ~PAD_CFG0_TX_STATE & VAL0
VAL0 &= ~PAD_CFG0_TX_STATE
Ack
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45654
to look at the new patch set (#4).
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
soc/intel/cannonlake: Align gpio_op.asl with TGL
Also drop gpio_common.h in favor of intelblocks/gpio_defs.h macros.
TEST=Able to build and boot CNL and CML platform. 1) Dump and disassemble DSDT, verify unified methods like GRXS, GTXS etc. are there. 2) Verify no ACPI error seen while running 'dmesg' from console.
Change-Id: I78d712eeba56b9c098dc6a6f11e4e51cb2529b10 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/acpi/gpio_op.asl D src/soc/intel/cannonlake/include/soc/gpio_common.h M src/soc/intel/cannonlake/include/soc/gpio_defs.h M src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h 4 files changed, 11 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/45654/4
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 5:
I think this one is redundant now with CB:45566
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 5:
Patch Set 5:
I think this one is redundant now with CB:45566
Yeah, kind off but Angel has good point that lets get rid of other stuffs before we heading towards CB:45566. Do you agree too? like dropping unused GPIO headers from LP and H etc..
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 5:
Tim, Furquan, Angel,
if you can take a look into this CL then we are good in patch trend for submission and end of the day?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 5: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 5:
Thanks Angel.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45654/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio_op.asl:
https://review.coreboot.org/c/coreboot/+/45654/5/src/soc/intel/cannonlake/ac... PS5, Line 15: PAD_CFG0_RX_STATE & (VAL0 >> PAD_CFG0_RX_STATE_BIT) This is not correct. It should be: Local0 = (PAD_CFG0_RX_STATE & VAL0) >> PAD_CFG0_RX_STATE_BIT
Since PAD_CFG0_RX_STATE is 1 << PAD_CFG0_RX_STATE_BIT: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45654/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio_op.asl:
https://review.coreboot.org/c/coreboot/+/45654/5/src/soc/intel/cannonlake/ac... PS5, Line 15: PAD_CFG0_RX_STATE & (VAL0 >> PAD_CFG0_RX_STATE_BIT)
This is not correct. It should be: […]
Yes Furquan, you are right. i have to submit a correction CL for all chipset looks like at first ? or i should just clean at final CL https://review.coreboot.org/c/coreboot/+/45566 ? then don't need a correction CL?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45654/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio_op.asl:
https://review.coreboot.org/c/coreboot/+/45654/5/src/soc/intel/cannonlake/ac... PS5, Line 15: PAD_CFG0_RX_STATE & (VAL0 >> PAD_CFG0_RX_STATE_BIT)
Yes Furquan, you are right. […]
I think we should add a separate CL to fix the incorrect behavior. This ensures that we don't add incorrect code for cannonlake and also the move to common gpio.asl would be just a no functional change (makes it easier to review).
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45654/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio_op.asl:
https://review.coreboot.org/c/coreboot/+/45654/5/src/soc/intel/cannonlake/ac... PS5, Line 15: PAD_CFG0_RX_STATE & (VAL0 >> PAD_CFG0_RX_STATE_BIT)
I think we should add a separate CL to fix the incorrect behavior. […]
make sense, will submit one
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45654/5/src/soc/intel/cannonlake/ac... File src/soc/intel/cannonlake/acpi/gpio_op.asl:
https://review.coreboot.org/c/coreboot/+/45654/5/src/soc/intel/cannonlake/ac... PS5, Line 15: PAD_CFG0_RX_STATE & (VAL0 >> PAD_CFG0_RX_STATE_BIT)
make sense, will submit one
https://review.coreboot.org/c/coreboot/+/45738
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45654
to look at the new patch set (#6).
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
soc/intel/cannonlake: Align gpio_op.asl with TGL
Also drop gpio_common.h in favor of intelblocks/gpio_defs.h macros.
TEST=Able to build and boot CNL and CML platform. 1) Dump and disassemble DSDT, verify unified methods like GRXS, GTXS etc. are there. 2) Verify no ACPI error seen while running 'dmesg' from console.
Change-Id: I78d712eeba56b9c098dc6a6f11e4e51cb2529b10 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/acpi/gpio_op.asl D src/soc/intel/cannonlake/include/soc/gpio_common.h M src/soc/intel/cannonlake/include/soc/gpio_defs.h M src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h 4 files changed, 11 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/45654/6
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 6:
This needs to be rebased on its parent CL.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45654
to look at the new patch set (#7).
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
soc/intel/cannonlake: Align gpio_op.asl with TGL
Also drop gpio_common.h in favor of intelblocks/gpio_defs.h macros.
TEST=Able to build and boot CNL and CML platform. 1) Dump and disassemble DSDT, verify unified methods like GRXS, GTXS etc. are there. 2) Verify no ACPI error seen while running 'dmesg' from console.
Change-Id: I78d712eeba56b9c098dc6a6f11e4e51cb2529b10 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/acpi/gpio_op.asl D src/soc/intel/cannonlake/include/soc/gpio_common.h M src/soc/intel/cannonlake/include/soc/gpio_defs.h M src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h 4 files changed, 11 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/45654/7
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45654/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45654/7//COMMIT_MSG@14 PS7, Line 14: 2) Verify no ACPI error seen while running 'dmesg' from console. It would be very helpful to use abuild with timeless option to ensure there are no other functional changes.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45654/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45654/7//COMMIT_MSG@14 PS7, Line 14: 2) Verify no ACPI error seen while running 'dmesg' from console.
It would be very helpful to use abuild with timeless option to ensure there are no other functional […]
Do you recommend me to add this line in test msg ? as i have ensured no changes with or without this CL?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45654/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45654/7//COMMIT_MSG@14 PS7, Line 14: 2) Verify no ACPI error seen while running 'dmesg' from console.
Do you recommend me to add this line in test msg ? as i have ensured no changes with or without this […]
Yeah. I think that is generally helpful to know what tests have been run. Also, helpful when bisecting issues.
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45654
to look at the new patch set (#8).
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
soc/intel/cannonlake: Align gpio_op.asl with TGL
Also drop gpio_common.h in favor of intelblocks/gpio_defs.h macros.
TEST=Able to build and boot CNL and CML platform. 1) Dump and disassemble DSDT, verify unified methods like GRXS, GTXS etc. are there. 2) Verify no ACPI error seen while running 'dmesg' from console. 3) abuild --timeless to ensure there are no other functional changes.
Change-Id: I78d712eeba56b9c098dc6a6f11e4e51cb2529b10 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/acpi/gpio_op.asl D src/soc/intel/cannonlake/include/soc/gpio_common.h M src/soc/intel/cannonlake/include/soc/gpio_defs.h M src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h 4 files changed, 11 insertions(+), 25 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/45654/8
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45654/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45654/7//COMMIT_MSG@14 PS7, Line 14: 2) Verify no ACPI error seen while running 'dmesg' from console.
Yeah. I think that is generally helpful to know what tests have been run. […]
Ack
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45654 )
Change subject: soc/intel/cannonlake: Align gpio_op.asl with TGL ......................................................................
soc/intel/cannonlake: Align gpio_op.asl with TGL
Also drop gpio_common.h in favor of intelblocks/gpio_defs.h macros.
TEST=Able to build and boot CNL and CML platform. 1) Dump and disassemble DSDT, verify unified methods like GRXS, GTXS etc. are there. 2) Verify no ACPI error seen while running 'dmesg' from console. 3) abuild --timeless to ensure there are no other functional changes.
Change-Id: I78d712eeba56b9c098dc6a6f11e4e51cb2529b10 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45654 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/cannonlake/acpi/gpio_op.asl D src/soc/intel/cannonlake/include/soc/gpio_common.h M src/soc/intel/cannonlake/include/soc/gpio_defs.h M src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h 4 files changed, 11 insertions(+), 25 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/intel/cannonlake/acpi/gpio_op.asl b/src/soc/intel/cannonlake/acpi/gpio_op.asl index 7f2a40c..9fa3dc4 100644 --- a/src/soc/intel/cannonlake/acpi/gpio_op.asl +++ b/src/soc/intel/cannonlake/acpi/gpio_op.asl @@ -1,4 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <intelblocks/gpio_defs.h>
/* * Get GPIO Value @@ -11,7 +12,7 @@ { VAL0, 32 } - Local0 = GPIORXSTATE_MASK & (VAL0 >> GPIORXSTATE_SHIFT) + Local0 = (PAD_CFG0_RX_STATE & VAL0) >> PAD_CFG0_RX_STATE_BIT
Return (Local0) } @@ -27,7 +28,7 @@ { VAL0, 32 } - Local0 = GPIOTXSTATE_MASK & VAL0 + Local0 = PAD_CFG0_TX_STATE & VAL0
Return (Local0) } @@ -43,7 +44,7 @@ { VAL0, 32 } - VAL0 |= GPIOTXSTATE_MASK + VAL0 |= PAD_CFG0_TX_STATE }
/* @@ -57,7 +58,7 @@ { VAL0, 32 } - VAL0 &= ~GPIOTXSTATE_MASK + VAL0 &= ~PAD_CFG0_TX_STATE }
/* @@ -76,8 +77,8 @@ { VAL0, 32 } - Local0 = ~GPIOPADMODE_MASK & VAL0 - Arg1 = (Arg1 << GPIOPADMODE_SHIFT) & GPIOPADMODE_MASK + Local0 = ~PAD_CFG0_MODE_MASK & VAL0 + Arg1 = (Arg1 << PAD_CFG0_MODE_SHIFT) & PAD_CFG0_MODE_MASK VAL0 = Local0 | Arg1 }
@@ -97,9 +98,9 @@ }
If (Arg1 == 1) { - VAL0 &= ~GPIOTXBUFDIS_MASK + VAL0 &= ~PAD_CFG0_TX_DISABLE } ElseIf (Arg1 == 0){ - VAL0 |= GPIOTXBUFDIS_MASK + VAL0 |= PAD_CFG0_TX_DISABLE } }
@@ -119,8 +120,8 @@ }
If (Arg1 == 1) { - VAL0 &= ~GPIORXBUFDIS_MASK + VAL0 &= ~PAD_CFG0_RX_DISABLE } ElseIf (Arg1 == 0){ - VAL0 |= GPIORXBUFDIS_MASK + VAL0 |= PAD_CFG0_RX_DISABLE } } diff --git a/src/soc/intel/cannonlake/include/soc/gpio_common.h b/src/soc/intel/cannonlake/include/soc/gpio_common.h deleted file mode 100644 index c11ef50..0000000 --- a/src/soc/intel/cannonlake/include/soc/gpio_common.h +++ /dev/null @@ -1,13 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0-only */ -#ifndef _SOC_CANNONLAKE_GPIO_COMMON_H_ -#define _SOC_CANNONLAKE_GPIO_COMMON_H_ - -#define GPIORXSTATE_MASK 0x1 -#define GPIORXSTATE_SHIFT 1 -#define GPIOTXSTATE_MASK 0x1 -#define GPIOPADMODE_MASK 0xC00 -#define GPIOPADMODE_SHIFT 10 -#define GPIOTXBUFDIS_MASK 0x100 -#define GPIORXBUFDIS_MASK 0x200 - -#endif diff --git a/src/soc/intel/cannonlake/include/soc/gpio_defs.h b/src/soc/intel/cannonlake/include/soc/gpio_defs.h index 9b1690e..e7769b5 100644 --- a/src/soc/intel/cannonlake/include/soc/gpio_defs.h +++ b/src/soc/intel/cannonlake/include/soc/gpio_defs.h @@ -6,7 +6,6 @@ #ifndef __ACPI__ #include <stddef.h> #endif -#include <soc/gpio_common.h> #include <soc/gpio_soc_defs.h>
#define GPIO_NUM_PAD_CFG_REGS 4 /* DW0, DW1, DW2, DW3 */ diff --git a/src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h b/src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h index a1f51d1..bd68b04 100644 --- a/src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h +++ b/src/soc/intel/cannonlake/include/soc/gpio_defs_cnp_h.h @@ -6,7 +6,6 @@ #ifndef __ACPI__ #include <stddef.h> #endif -#include <soc/gpio_common.h> #include <soc/gpio_soc_defs_cnp_h.h>
#define GPIO_NUM_PAD_CFG_REGS 4 /* DW0, DW1, DW2, DW3 */
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
HAOUAS Elyes (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)