Change in coreboot[master]: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL
Rizwan Qureshi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39498 )
Change subject: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL ......................................................................
src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL
In method RAOW is assumin that the first argument is a Field object and writing to it expecting the register to get updated. However, the callers are passing in the value of the Field object instead.
This eventually is resulting the IMGCLK not getting enable/disabled on the platform.
Fix this by sendign the exact address of the register to be updated.
BUG=None BRANCH=None TEST=Build and Boot waddledoo board and verified that IMGCLKOUT for world facing camera is enabled/disabled and able to capture images.
Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Change-Id: I8b886255d5f38819502ae1f4af0851b5a0922b22 --- M src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl 1 file changed, 15 insertions(+), 70 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/39498/1
diff --git a/src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl b/src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl index ab1097e..d9d6e0b 100644 --- a/src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl +++ b/src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl @@ -17,23 +17,15 @@ #define R_ICLK_PCR_CAMERA1 0x8000 #define B_ICLK_PCR_FREQUENCY 0x1 #define B_ICLK_PCR_REQUEST 0x2 +#define B_ICLK_PCR_OFFSET 0xC
Scope (_SB.PCI0) { - /* IsCLK PCH register for clock settings */ - OperationRegion (ICLK, SystemMemory, PCRB (PID_ISCLK) + R_ICLK_PCR_CAMERA1, 0x40) - Field (ICLK, AnyAcc, Lock, Preserve) + Name (ICKB, 0) + Store (PCRB (PID_ISCLK) + R_ICLK_PCR_CAMERA1, ICKB) + + Method (OFST, 0x1, NotSerialized) { - CLK1, 8, - Offset(0x0C), - CLK2, 8, - Offset(0x18), - CLK3, 8, - Offset(0x24), - CLK4, 8, - Offset(0x30), - CLK5, 8, - Offset(0x3C), - CLK6, 8, + Return (ICKB + (Arg0 * B_ICLK_PCR_OFFSET)) }
/* @@ -44,8 +36,13 @@ */ Method (RAOW, 0x3, NotSerialized) { - Local0 = Arg0 - Arg0 = Local0 & Arg1 | Arg2 + OperationRegion (ICLK, SystemMemory, Arg0, 4) + Field (ICLK, AnyAcc, NoLock, Preserve) + { + VAL0, 32 + } + Local0 = VAL0 + VAL0 = Local0 & Arg1 | Arg2 }
/* @@ -56,33 +53,7 @@ Method(CLKC, 0x2, NotSerialized) {
- Switch (ToInteger (Arg0)) - { - Case (0) - { - RAOW (CLK1, ~B_ICLK_PCR_REQUEST, Arg1 << 1) - } - Case (1) - { - RAOW (CLK2, ~B_ICLK_PCR_REQUEST, Arg1 << 1) - } - Case (2) - { - RAOW (CLK3, ~B_ICLK_PCR_REQUEST, Arg1 << 1) - } - Case (3) - { - RAOW (CLK4, ~B_ICLK_PCR_REQUEST, Arg1 << 1) - } - Case (4) - { - RAOW (CLK5, ~B_ICLK_PCR_REQUEST, Arg1 << 1) - } - Case (5) - { - RAOW (CLK6, ~B_ICLK_PCR_REQUEST, Arg1 << 1) - } - } + RAOW (OFST(Arg0), ~B_ICLK_PCR_REQUEST, Arg1 << 1) }
/* @@ -92,33 +63,7 @@ */ Method (CLKF, 0x2, NotSerialized) { - Switch (ToInteger (Arg0)) - { - Case (0) - { - RAOW (CLK1, ~B_ICLK_PCR_FREQUENCY, Arg1) - } - Case (1) - { - RAOW (CLK2, ~B_ICLK_PCR_FREQUENCY, Arg1) - } - Case (2) - { - RAOW (CLK3, ~B_ICLK_PCR_FREQUENCY, Arg1) - } - Case (3) - { - RAOW (CLK4, ~B_ICLK_PCR_FREQUENCY, Arg1) - } - Case (4) - { - RAOW (CLK5, ~B_ICLK_PCR_FREQUENCY, Arg1) - } - Case (5) - { - RAOW (CLK6, ~B_ICLK_PCR_FREQUENCY, Arg1) - } - } + RAOW (OFST(Arg0), ~B_ICLK_PCR_FREQUENCY, Arg1) }
/*
Hello Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39498
to look at the new patch set (#2).
Change subject: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL ......................................................................
src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL
Method RAOW is assuming that the first argument is a Field object and writing to it expecting the register to get updated. However, the callers are passing in the value of the Field object instead.
This eventually is resulting the IMGCLK not getting enable/disabled on the platform.
Fix this by sending the exact address of the register to be updated.
BUG=None BRANCH=None TEST=Build and Boot waddledoo board and verified that IMGCLKOUT for world facing camera is enabled/disabled and able to capture images.
Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Change-Id: I8b886255d5f38819502ae1f4af0851b5a0922b22 --- M src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl 1 file changed, 15 insertions(+), 70 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/39498/2
Hello Varshit B Pandya, build bot (Jenkins), Furquan Shaikh, Subrata Banik, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39498
to look at the new patch set (#3).
Change subject: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL ......................................................................
src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL
Method RAOW is assuming that the first argument is a Field object and writing to it expecting the register to get updated. However, the callers are passing in the value of the Field object instead.
This eventually is resulting the IMGCLK not getting enable/disabled on the platform.
Fix this by sending the exact address of the register to be updated.
BUG=None BRANCH=None TEST=Build and Boot waddledoo board and verified that IMGCLKOUT for world facing camera is enabled/disabled and able to capture images.
Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Change-Id: I8b886255d5f38819502ae1f4af0851b5a0922b22 --- M src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl 1 file changed, 19 insertions(+), 70 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/39498/3
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39498 )
Change subject: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39498/2/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl:
https://review.coreboot.org/c/coreboot/+/39498/2/src/soc/intel/tigerlake/acp... PS2, Line 26: Method (OFST, 0x1, NotSerialized) description would help, OFST is for offset?
Hello Varshit B Pandya, build bot (Jenkins), Furquan Shaikh, Wonkyu Kim, Ravishankar Sarawadi, Subrata Banik, Nick Vaccaro, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39498
to look at the new patch set (#4).
Change subject: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL ......................................................................
src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL
Method RAOW is assuming that the first argument is a Field object and writing to it expecting the register to get updated. However, the callers are passing in the value of the Field object instead.
This eventually is resulting the IMGCLK not getting enable/disabled on the platform.
Fix this by sending the exact address of the register to be updated.
Also MCCT was setting the clock frequency in both case i.e, Clock Enable and Disable. Split the MCCT method in two, MCON and MCOF to fix the sequencing like below MCON: Set frequency Enable clock MCOF: Disable clock
BUG=None BRANCH=None TEST=Build and Boot waddledoo board and verified that IMGCLKOUT for world facing camera is enabled/disabled and able to capture images.
Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Change-Id: I8b886255d5f38819502ae1f4af0851b5a0922b22 --- M src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl 1 file changed, 34 insertions(+), 93 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/39498/4
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39498 )
Change subject: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39498/2/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl:
https://review.coreboot.org/c/coreboot/+/39498/2/src/soc/intel/tigerlake/acp... PS2, Line 26: Method (OFST, 0x1, NotSerialized)
description would help, OFST is for offset?
Yes, better to have description of helper function.
Hello Varshit B Pandya, build bot (Jenkins), Daniel Kang, Furquan Shaikh, Wonkyu Kim, Ravishankar Sarawadi, Subrata Banik, Nick Vaccaro, Aamir Bohra, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39498
to look at the new patch set (#6).
Change subject: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL ......................................................................
src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL
Method RAOW is assuming that the first argument is a Field object and writing to it expecting the register to get updated. However, the callers are passing in the value of the Field object instead.
This eventually is resulting the IMGCLK not getting enable/disabled on the platform.
Fix this by sending the exact address of the register to be updated.
Also MCCT was setting the clock frequency in both case i.e, Clock Enable and Disable. Split the MCCT method in two, MCON and MCOF to fix the sequencing like below MCON: Set frequency Enable clock MCOF: Disable clock
Also, make use of MCON and MCOF methods for camera clock control in tglrvp. This is to avoid the buildbot marking the patch unstable.
BUG=None BRANCH=None TEST=Build and Boot waddledoo board and verified that IMGCLKOUT for world facing camera is enabled/disabled and able to capture images. Build and Boot Tiger Lake RVP board and verified that IMGCLKOUT for world facing camera is enabled/disabled and able to capture images.
Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Change-Id: I8b886255d5f38819502ae1f4af0851b5a0922b22 --- M src/mainboard/intel/tglrvp/acpi/mipi_camera.asl M src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl 2 files changed, 38 insertions(+), 97 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/39498/6
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39498 )
Change subject: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39498/2/src/soc/intel/tigerlake/acp... File src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl:
https://review.coreboot.org/c/coreboot/+/39498/2/src/soc/intel/tigerlake/acp... PS2, Line 26: Method (OFST, 0x1, NotSerialized)
Yes, better to have description of helper function.
Done
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39498 )
Change subject: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL ......................................................................
Patch Set 7: Code-Review+2
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39498 )
Change subject: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL ......................................................................
Patch Set 7:
Can you add topic as TGL_UPSTREAM
Wonkyu Kim has uploaded a new patch set (#8) to the change originally created by Rizwan Qureshi. ( https://review.coreboot.org/c/coreboot/+/39498 )
Change subject: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL ......................................................................
src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL
Method RAOW is assuming that the first argument is a Field object and writing to it expecting the register to get updated. However, the callers are passing in the value of the Field object instead.
This eventually is resulting the IMGCLK not getting enable/disabled on the platform.
Fix this by sending the exact address of the register to be updated.
Also MCCT was setting the clock frequency in both case i.e, Clock Enable and Disable. Split the MCCT method in two, MCON and MCOF to fix the sequencing like below MCON: Set frequency Enable clock MCOF: Disable clock
Also, make use of MCON and MCOF methods for camera clock control in tglrvp. This is to avoid the buildbot marking the patch unstable.
BUG=None BRANCH=None TEST=Build and Boot waddledoo board and verified that IMGCLKOUT for world facing camera is enabled/disabled and able to capture images. Build and Boot Tiger Lake RVP board and verified that IMGCLKOUT for world facing camera is enabled/disabled and able to capture images.
Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Change-Id: I8b886255d5f38819502ae1f4af0851b5a0922b22 --- M src/mainboard/intel/tglrvp/acpi/mipi_camera.asl M src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl 2 files changed, 38 insertions(+), 97 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/39498/8
Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39498 )
Change subject: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL ......................................................................
Patch Set 8: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39498 )
Change subject: src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL ......................................................................
src/soc/intel/tigerlake: Fix incorrect use of Field objects in ASL
Method RAOW is assuming that the first argument is a Field object and writing to it expecting the register to get updated. However, the callers are passing in the value of the Field object instead.
This eventually is resulting the IMGCLK not getting enable/disabled on the platform.
Fix this by sending the exact address of the register to be updated.
Also MCCT was setting the clock frequency in both case i.e, Clock Enable and Disable. Split the MCCT method in two, MCON and MCOF to fix the sequencing like below MCON: Set frequency Enable clock MCOF: Disable clock
Also, make use of MCON and MCOF methods for camera clock control in tglrvp. This is to avoid the buildbot marking the patch unstable.
BUG=None BRANCH=None TEST=Build and Boot waddledoo board and verified that IMGCLKOUT for world facing camera is enabled/disabled and able to capture images. Build and Boot Tiger Lake RVP board and verified that IMGCLKOUT for world facing camera is enabled/disabled and able to capture images.
Signed-off-by: Rizwan Qureshi rizwan.qureshi@intel.com Change-Id: I8b886255d5f38819502ae1f4af0851b5a0922b22 Reviewed-on: https://review.coreboot.org/c/coreboot/+/39498 Reviewed-by: Srinidhi N Kaushik srinidhi.n.kaushik@intel.com Reviewed-by: Wonkyu Kim wonkyu.kim@intel.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/intel/tglrvp/acpi/mipi_camera.asl M src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl 2 files changed, 38 insertions(+), 97 deletions(-)
Approvals: build bot (Jenkins): Verified Srinidhi N Kaushik: Looks good to me, approved Wonkyu Kim: Looks good to me, approved
diff --git a/src/mainboard/intel/tglrvp/acpi/mipi_camera.asl b/src/mainboard/intel/tglrvp/acpi/mipi_camera.asl index c830ea1..5d42a29 100644 --- a/src/mainboard/intel/tglrvp/acpi/mipi_camera.asl +++ b/src/mainboard/intel/tglrvp/acpi/mipi_camera.asl @@ -177,7 +177,7 @@ If ((STA == Zero)) { /* Enable CLK0 with 19.2MHz */ - MCCT(0,1,1) + MCON(0,1) /* Pull PWREN(GPIO B23) high */ STXS(GPP_B23) Sleep(5) @@ -200,7 +200,7 @@ /* Pull PWREN low */ CTXS(GPP_B23) /* Disable CLK0 */ - MCCT(0,0,1) + MCOF(0) Store(0,STA) } } @@ -380,7 +380,7 @@ If ((STA == Zero)) { /* Enable CLK1 with 19.2MHz */ - MCCT(1,1,1) + MCON(1,1) /* Pull PWREN(GPIO R6) high */ STXS(GPP_R6) Sleep(5) @@ -403,7 +403,7 @@ /* Pull PWREN low */ CTXS(GPP_R6) /* Disable CLK1 */ - MCCT(1,0,1) + MCOF(1) Store(0,STA) } } diff --git a/src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl b/src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl index ab1097e..c9da977 100644 --- a/src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl +++ b/src/soc/intel/tigerlake/acpi/camera_clock_ctl.asl @@ -18,22 +18,23 @@ #define B_ICLK_PCR_FREQUENCY 0x1 #define B_ICLK_PCR_REQUEST 0x2
+/* The clock control registers for each IMGCLK are offset by 0xC */ +#define B_ICLK_PCR_OFFSET 0xC + Scope (_SB.PCI0) { - /* IsCLK PCH register for clock settings */ - OperationRegion (ICLK, SystemMemory, PCRB (PID_ISCLK) + R_ICLK_PCR_CAMERA1, 0x40) - Field (ICLK, AnyAcc, Lock, Preserve) + + /* IsCLK PCH base register for clock settings */ + Name (ICKB, 0) + Store (PCRB (PID_ISCLK) + R_ICLK_PCR_CAMERA1, ICKB) + + /* + * Arg0 : Clock Number + * Return : Offset of register to control the clock in Arg0 + * + */ + Method (OFST, 0x1, NotSerialized) { - CLK1, 8, - Offset(0x0C), - CLK2, 8, - Offset(0x18), - CLK3, 8, - Offset(0x24), - CLK4, 8, - Offset(0x30), - CLK5, 8, - Offset(0x3C), - CLK6, 8, + Return (ICKB + (Arg0 * B_ICLK_PCR_OFFSET)) }
/* @@ -42,95 +43,35 @@ * Arg1 : And data * Arg2 : Or data */ - Method (RAOW, 0x3, NotSerialized) + Method (RAOW, 0x3, Serialized) { - Local0 = Arg0 - Arg0 = Local0 & Arg1 | Arg2 - } - - /* - * Clock Control - * Arg0 - Clock number (0:IMGCLKOUT_0, etc) - * Arg1 - Desired state (0:Disable, 1:Enable) - */ - Method(CLKC, 0x2, NotSerialized) - { - - Switch (ToInteger (Arg0)) + OperationRegion (ICLK, SystemMemory, OFST(Arg0), 4) + Field (ICLK, AnyAcc, NoLock, Preserve) { - Case (0) - { - RAOW (CLK1, ~B_ICLK_PCR_REQUEST, Arg1 << 1) - } - Case (1) - { - RAOW (CLK2, ~B_ICLK_PCR_REQUEST, Arg1 << 1) - } - Case (2) - { - RAOW (CLK3, ~B_ICLK_PCR_REQUEST, Arg1 << 1) - } - Case (3) - { - RAOW (CLK4, ~B_ICLK_PCR_REQUEST, Arg1 << 1) - } - Case (4) - { - RAOW (CLK5, ~B_ICLK_PCR_REQUEST, Arg1 << 1) - } - Case (5) - { - RAOW (CLK6, ~B_ICLK_PCR_REQUEST, Arg1 << 1) - } + VAL0, 32 } - } - - /* - * Clock Frequency - * Arg0 - Clock number (0:IMGCLKOUT_0, etc) - * Arg1 - Clock frequency (0:24MHz, 1:19.2MHz) - */ - Method (CLKF, 0x2, NotSerialized) - { - Switch (ToInteger (Arg0)) - { - Case (0) - { - RAOW (CLK1, ~B_ICLK_PCR_FREQUENCY, Arg1) - } - Case (1) - { - RAOW (CLK2, ~B_ICLK_PCR_FREQUENCY, Arg1) - } - Case (2) - { - RAOW (CLK3, ~B_ICLK_PCR_FREQUENCY, Arg1) - } - Case (3) - { - RAOW (CLK4, ~B_ICLK_PCR_FREQUENCY, Arg1) - } - Case (4) - { - RAOW (CLK5, ~B_ICLK_PCR_FREQUENCY, Arg1) - } - Case (5) - { - RAOW (CLK6, ~B_ICLK_PCR_FREQUENCY, Arg1) - } - } + Local0 = VAL0 + VAL0 = Local0 & Arg1 | Arg2 }
/* * Clock control Method * Arg0: Clock source select(0: IMGCLKOUT_0, 1: IMGCLKOUT_1, 2: IMGCLKOUT_2, 3: IMGCLKOUT_3, * 4: IMGCLKOUT_4, 5: IMGCLKOUT_5) - * Arg1: Clock Enable / Disable (0: Disable, 1: Enable) - * Arg2: Select 24MHz / 19.2 MHz (0: 24MHz, 1: 19.2MHz) + * Arg1: Select 24MHz / 19.2 MHz (0: 24MHz, 1: 19.2MHz) */ - Method (MCCT, 0x3, NotSerialized) + Method (MCON, 0x2, NotSerialized) { - CLKF (Arg0, Arg2) - CLKC (Arg0, Arg1) + /* Set Clock Frequency */ + RAOW (Arg0, ~B_ICLK_PCR_FREQUENCY, Arg1) + + /* Enable Clock */ + RAOW (Arg0, ~B_ICLK_PCR_REQUEST, B_ICLK_PCR_REQUEST) + } + + Method (MCOF, 0x1, NotSerialized) + { + /* Disable Clock */ + RAOW (Arg0, ~B_ICLK_PCR_REQUEST, 0) } }
-
Aamir Bohra (Code Review) -
Patrick Georgi (Code Review)
-
Rizwan Qureshi (Code Review)
-
Srinidhi N Kaushik (Code Review)
-
Wonkyu Kim (Code Review)