Ricardo Ribalda has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
mb/google/poppy: Fix race condition in acpi camera_pmic
Newer kernels can re-schedule new acpi command calls during a Sleep().
This causes that the following trace fails to detect the cameras: [ 15.764725] drivers/acpi/power.c:358 Power resource [OVFI] turned on start [ 15.772180] drivers/acpi/power.c:358 Power resource [OVTH] turned on start [ 15.834970] drivers/acpi/power.c:362 Power resource [OVFI] turned on start [ 15.852456] drivers/acpi/power.c:415 Power resource [OVFI] turned off start [ 15.955987] drivers/acpi/power.c:420 Power resource [OVFI] turned off end ERROR!! [ 16.030896] drivers/acpi/power.c:362 Power resource [OVTH] turned on end
Which can be triggered more frequently if the Sleep() commands in OVTH _ON Method are increased.
To avoid the race condition, we create a new Power Resource that handles the common resources of both cameras and make both cameras depend on that resource. This also simplifies the acpi table by removing a Mutex.
BUG=c:171955583 TEST=while true; do if ssh $DUT "dmesg | grep "failed to find sensor" "; then break; fi; ssh $DUT reboot; sleep 30 ; done Signed-off-by: Ricardo Ribalda ribalda@chromium.org
Change-Id: I25df0225699759c1828b8791c5bdee66529858a7 --- M src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl M src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl M src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl 3 files changed, 78 insertions(+), 96 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/48631/1
diff --git a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl index 8f3ae53..2d5dd0a 100644 --- a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl +++ b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl @@ -23,8 +23,8 @@ ) })
- Name (_PR0, Package () { ^^I2C2.PMIC.OVTH }) - Name (_PR3, Package () { ^^I2C2.PMIC.OVTH }) + Name (_PR0, Package () {^^I2C2.PMIC.OVCM, ^^I2C2.PMIC.OVTH}) + Name (_PR3, Package () {^^I2C2.PMIC.OVCM, ^^I2C2.PMIC.OVTH})
/* Port0 of CAM0 is connected to port0 of CIO2 device */ Name (_DSD, Package () { diff --git a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl index 7e20575..c41ebca 100644 --- a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl +++ b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl @@ -23,8 +23,8 @@ ) })
- Name (_PR0, Package () { ^^I2C2.PMIC.OVFI }) - Name (_PR3, Package () { ^^I2C2.PMIC.OVFI }) + Name (_PR0, Package () {^^I2C2.PMIC.OVCM, ^^I2C2.PMIC.OVFI}) + Name (_PR3, Package () {^^I2C2.PMIC.OVCM, ^^I2C2.PMIC.OVFI})
/* Port0 of CAM1 is connected to port1 of CIO2 device */ Name (_DSD, Package () { diff --git a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl index 5ad10ef..3ce5555 100644 --- a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl +++ b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl @@ -313,76 +313,50 @@ PODV, 32, }
- /* CLE0 and CLE1 are used to determine if both the clocks - are enabled or disabled. */ - Mutex (MUTC, 0) - Name (CLE0, 0) - Name (CLE1, 0) - Method (CLKE, 0, Serialized) { - /* save Acquire result so we can check for - Mutex acquired */ - Store (Acquire (MUTC, 1000), Local0) - /* check for Mutex acquired */ - If (LEqual (Local0, Zero)) { - /* Enable clocks only when a sensor is turned on and - both the clocks are disabled */ - If (LNot (LOr (CLE0, CLE1))) { - /* Set boost clock divider */ - BODI = 3 - /* Set buck clock divider */ - BUDI = 2 - /* Set the PLL_REF_CLK cyles */ - PSWR = 19 - /* Set the reference crystal divider */ - XTDV = 170 - /* Set PLL feedback divider */ - PLDV = 32 - /* Set PLL output divider for HCLK_A */ - PODV = 1 - /* Set PLL output divider for HCLK_B */ - PDV2 = 1 - /* Enable clocks for both the sensors - * CFG1: output selection for HCLK_A and - * HCLK_B. - * CFG2: set drive strength for HCLK_A - * and HCLK_B. - */ - CFG2 = 5 - CFG1 = 10 - /* Enable PLL output, crystal oscillator - * input capacitance control and set - * Xtal oscillator as clock source. - */ - PCTL = 209 - Sleep(1) - } - Release (MUTC) - } - } - - /* Clocks need to be disabled only if both the sensors are - turned off */ - Method (CLKD, 0, Serialized) { - /* save Acquire result so we can check for - Mutex acquired */ - Store (Acquire (MUTC, 1000), Local0) - /* check for Mutex acquired */ - If (LEqual (Local0, Zero)) { - If (LNot (LOr (CLE0, CLE1))) { - BODI = 0 - BUDI = 0 - PSWR = 0 - XTDV = 0 - PLDV = 0 - PODV = 0 - PDV2 = 0 - /* Disable clocks for both the - sensors */ - CFG2 = 0 - CFG1 = 0 - PCTL = 0 - } - Release (MUTC) + Method (CLK, 1, Serialized) { + If (LEqual (Arg0, Zero)) { + BODI = 0 + BUDI = 0 + PSWR = 0 + XTDV = 0 + PLDV = 0 + PODV = 0 + PDV2 = 0 + /* Disable clocks for both the + sensors */ + CFG2 = 0 + CFG1 = 0 + PCTL = 0 + Sleep(1) + } ElseIf (LEqual (Arg0, 1)) { + /* Set boost clock divider */ + BODI = 3 + /* Set buck clock divider */ + BUDI = 2 + /* Set the PLL_REF_CLK cyles */ + PSWR = 19 + /* Set the reference crystal divider */ + XTDV = 170 + /* Set PLL feedback divider */ + PLDV = 32 + /* Set PLL output divider for HCLK_A */ + PODV = 1 + /* Set PLL output divider for HCLK_B */ + PDV2 = 1 + /* Enable clocks for both the sensors + * CFG1: output selection for HCLK_A and + * HCLK_B. + * CFG2: set drive strength for HCLK_A + * and HCLK_B. + */ + CFG2 = 5 + CFG1 = 10 + /* Enable PLL output, crystal oscillator + * input capacitance control and set + * Xtal oscillator as clock source. + */ + PCTL = 209 + Sleep(1) } }
@@ -425,6 +399,36 @@ } }
+ /* Power resource methods for both CAMs */ + PowerResource (OVCM, 0, 0) { + Name (STA, 0) + Method (_ON, 0, Serialized) { + If (LEqual (AVBL, 1)) { + If (LEqual (STA, 0)) { + /* Enable VSIO regulator + + daisy chain */ + DOVD(1) + + CLK(1) + STA = 1 + } + } + } + Method (_OFF, 0, Serialized) { + If (LEqual (AVBL, 1)) { + If (LEqual (STA, 1)) { + CLK(0) + Sleep(2) + DOVD(0) + } + } + STA = 0 + } + Method (_STA, 0, NotSerialized) { + Return (STA) + } + } + /* Power resource methods for CAM0 */ PowerResource (OVTH, 0, 0) { Name (STA, 0) @@ -432,10 +436,6 @@ /* TODO: Read Voltage and Sleep values from Sensor Obj */ If (LEqual (AVBL, 1)) { If (LEqual (STA, 0)) { - /* Enable VSIO regulator + - daisy chain */ - DOVD(1) - _SB.PCI0.I2C2.PMIC.CGP1() _SB.PCI0.I2C2.PMIC.CGP2()
@@ -447,9 +447,6 @@ DCVA = 12 VDCT = 1
- _SB.PCI0.I2C2.PMIC.CLKE() - CLE0 = 1 - /* * Wait for all regulator * outputs to settle. @@ -473,16 +470,12 @@ If (LEqual (AVBL, 1)) { If (LEqual (STA, 1)) { Sleep(2) - CLE0 = 0 - _SB.PCI0.I2C2.PMIC.CLKD() - Sleep(2) _SB.PCI0.I2C2.PMIC.CRST(0) Sleep(3) VDCT = 0 Sleep(3) VACT = 0 Sleep(1) - DOVD(0) } } STA = 0 @@ -499,10 +492,6 @@ /* TODO: Read Voltage and Sleep values from Sensor Obj */ If (LEqual (AVBL, 1)) { If (LEqual (STA, 0)) { - /* Enable VSIO regulator + - daisy chain */ - DOVD(1) - /* Set VAUX2 as 1.8006 V */ AX2V = 52 VAX2 = 1 /* Enable VAUX2 */ @@ -522,9 +511,6 @@ /* Wait for VDD to settle. */ Sleep(1)
- _SB.PCI0.I2C2.PMIC.CLKE() - CLE1 = 1 - _SB.PCI0.I2C2.PMIC.CGP5(1) /* * Ensure 10 ms between @@ -540,9 +526,6 @@ If (LEqual (AVBL, 1)) { If (LEqual (STA, 1)) { Sleep(2) - CLE1 = 0 - _SB.PCI0.I2C2.PMIC.CLKD() - Sleep(2) _SB.PCI0.I2C2.PMIC.CGP5(0) Sleep(3) VAX1 = 0 @@ -551,7 +534,6 @@ Sleep(1) VAX2 = 0 Sleep(1) - DOVD(0) } STA = 0 }
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
Patch Set 2: Code-Review+2
(3 comments)
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG@9 PS2, Line 9: Newer kernels can re-schedule new acpi command calls during a Sleep(). Is there a pointer to this change?
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG@28 PS2, Line 28: c b
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG@29 PS2, Line 29: TEST=while true; do if ssh $DUT "dmesg | grep "failed to find sensor" "; then break; fi; ssh $DUT reboot; sleep 30 ; done BRANCH=poppy
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48631/2/src/mainboard/google/poppy/... File src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl:
https://review.coreboot.org/c/coreboot/+/48631/2/src/mainboard/google/poppy/... PS2, Line 403: 0 Do you need to set the resourceorder for OVCM and OVTH to guarantee a specific order?
Hello build bot (Jenkins), Furquan Shaikh, Rizwan Qureshi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48631
to look at the new patch set (#3).
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
mb/google/poppy: Fix race condition in acpi camera_pmic
Newer kernels can re-schedule new acpi command calls during a Sleep().
This causes that the following trace fails to detect the cameras: [ 15.764725] drivers/acpi/power.c:358 Power resource [OVFI] turned on start [ 15.772180] drivers/acpi/power.c:358 Power resource [OVTH] turned on start [ 15.834970] drivers/acpi/power.c:362 Power resource [OVFI] turned on start [ 15.852456] drivers/acpi/power.c:415 Power resource [OVFI] turned off start [ 15.955987] drivers/acpi/power.c:420 Power resource [OVFI] turned off end ERROR!! [ 16.030896] drivers/acpi/power.c:362 Power resource [OVTH] turned on end
Which can be triggered more frequently if the Sleep() commands in OVTH _ON Method are increased.
To avoid the race condition, we create a new Power Resource that handles the common resources of both cameras and make both cameras depend on that resource. This also simplifies the acpi table by removing a Mutex.
BRANCH=poppy BUG=b:171955583 TEST=while true; do if ssh $DUT "dmesg | grep "failed to find sensor" "; then break; fi; ssh $DUT reboot; sleep 30 ; done Signed-off-by: Ricardo Ribalda ribalda@chromium.org
Change-Id: I25df0225699759c1828b8791c5bdee66529858a7 --- M src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl M src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl M src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl 3 files changed, 78 insertions(+), 96 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/48631/3
Ricardo Ribalda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG@9 PS2, Line 9: Newer kernels can re-schedule new acpi command calls during a Sleep().
Is there a pointer to this change?
I would love to blame this:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
but it might be too old...
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG@9 PS2, Line 9: Newer kernels can re-schedule new acpi command calls during a Sleep().
Is there a pointer to this change?
Done
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG@28 PS2, Line 28: c
b
Done
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG@29 PS2, Line 29: TEST=while true; do if ssh $DUT "dmesg | grep "failed to find sensor" "; then break; fi; ssh $DUT reboot; sleep 30 ; done
BRANCH=poppy
Done
Ricardo Ribalda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48631/2/src/mainboard/google/poppy/... File src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl:
https://review.coreboot.org/c/coreboot/+/48631/2/src/mainboard/google/poppy/... PS2, Line 403: 0
Do you need to set the resourceorder for OVCM and OVTH to guarantee a specific order?
I need the OVCM to be set before the other PR, and clear after the others are ready. I thought that was enforced by this order:
Name (_PR0, Package () {^^I2C2.PMIC.OVCM, ^^I2C2.PMIC.OVFI})
But reading: 7.2 https://www.uefi.org/sites/default/files/resources/ACPI%206_2_A_Sept29.pdf probes that I am worng. Thanks!!
Ricardo Ribalda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG@29 PS2, Line 29: TEST=while true; do if ssh $DUT "dmesg | grep "failed to find sensor" "; then break; fi; ssh $DUT reboot; sleep 30 ; done
Done
Is this field automatically parsed?
Hello build bot (Jenkins), Furquan Shaikh, Rizwan Qureshi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/48631
to look at the new patch set (#4).
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
mb/google/poppy: Fix race condition in acpi camera_pmic
Newer kernels can re-schedule new acpi command calls during a Sleep().
This causes that the following trace fails to detect the cameras: [ 15.764725] drivers/acpi/power.c:358 Power resource [OVFI] turned on start [ 15.772180] drivers/acpi/power.c:358 Power resource [OVTH] turned on start [ 15.834970] drivers/acpi/power.c:362 Power resource [OVFI] turned on start [ 15.852456] drivers/acpi/power.c:415 Power resource [OVFI] turned off start [ 15.955987] drivers/acpi/power.c:420 Power resource [OVFI] turned off end ERROR!! [ 16.030896] drivers/acpi/power.c:362 Power resource [OVTH] turned on end
Which can be triggered more frequently if the Sleep() commands in OVTH _ON Method are increased.
To avoid the race condition, we create a new Power Resource that handles the common resources of both cameras and make both cameras depend on that resource. This also simplifies the acpi table by removing a Mutex.
BRANCH=poppy BUG=b:171955583 TEST=while true; do if ssh $DUT "dmesg | grep "failed to find sensor" "; then break; fi; ssh $DUT reboot; sleep 30 ; done Signed-off-by: Ricardo Ribalda ribalda@chromium.org
Change-Id: I25df0225699759c1828b8791c5bdee66529858a7 --- M src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl M src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl M src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl 3 files changed, 78 insertions(+), 96 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/48631/4
Ricardo Ribalda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48631/2/src/mainboard/google/poppy/... File src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl:
https://review.coreboot.org/c/coreboot/+/48631/2/src/mainboard/google/poppy/... PS2, Line 403: 0
I need the OVCM to be set before the other PR, and clear after the others are ready. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG@9 PS2, Line 9: Newer kernels can re-schedule new acpi command calls during a Sleep().
I would love to blame this: […]
Yeah, that seems pretty old. I am confident that we did not see this issue with 4.4 or even 4.19. In fact, we had this issue that resume takes super long because each Sleep() results in all CPUs waiting idle. It would be interesting to see what the change in behavior is because there might be other cases that would probably break.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
Patch Set 4: Code-Review+2
Ricardo Ribalda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG@9 PS2, Line 9: Newer kernels can re-schedule new acpi command calls during a Sleep().
Yeah, that seems pretty old. I am confident that we did not see this issue with 4.4 or even 4.19. […]
Although unlikely, it could also be that this bug has been un-noticeable for a while. On an unpatched soraka it fails around 1/10 reboots. And I know Tomasz could not even replicate it in some other boards...
We can try to bisect when this started to fail... but it could be quite a long job. Also I am not sure that all the path from 4.4 to 5.4 is bisectable.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
Patch Set 4:
(1 comment)
If you need to roll this out to shipped devices, following needs to happen: 1. Change needs to be cherry-picked to poppy firmware branch 2. FW qual needs to be requested
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/48631/2//COMMIT_MSG@9 PS2, Line 9: Newer kernels can re-schedule new acpi command calls during a Sleep().
Although unlikely, it could also be that this bug has been un-noticeable for a while. […]
That's fine. I was mostly curious since you asserted that newer kernels change some behavior w.r.t. Sleep() in ACPI. The change in its current form looks okay and should work fine irrespective of kernel behavior.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
Patch Set 4: Code-Review+2
Thanks for fixing this!
Ricardo Ribalda has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
Patch Set 4:
Patch Set 4:
(1 comment)
If you need to roll this out to shipped devices, following needs to happen:
- Change needs to be cherry-picked to poppy firmware branch
- FW qual needs to be requested
What is the process for requesting FW qual?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
(1 comment)
If you need to roll this out to shipped devices, following needs to happen:
- Change needs to be cherry-picked to poppy firmware branch
- FW qual needs to be requested
What is the process for requesting FW qual?
Details are captured here: https://g3doc.corp.google.com/company/teams/chrome/ops/chromeos/engprod/firm...
Let me know if you need help with it. Thanks!
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48631 )
Change subject: mb/google/poppy: Fix race condition in acpi camera_pmic ......................................................................
mb/google/poppy: Fix race condition in acpi camera_pmic
Newer kernels can re-schedule new acpi command calls during a Sleep().
This causes that the following trace fails to detect the cameras: [ 15.764725] drivers/acpi/power.c:358 Power resource [OVFI] turned on start [ 15.772180] drivers/acpi/power.c:358 Power resource [OVTH] turned on start [ 15.834970] drivers/acpi/power.c:362 Power resource [OVFI] turned on start [ 15.852456] drivers/acpi/power.c:415 Power resource [OVFI] turned off start [ 15.955987] drivers/acpi/power.c:420 Power resource [OVFI] turned off end ERROR!! [ 16.030896] drivers/acpi/power.c:362 Power resource [OVTH] turned on end
Which can be triggered more frequently if the Sleep() commands in OVTH _ON Method are increased.
To avoid the race condition, we create a new Power Resource that handles the common resources of both cameras and make both cameras depend on that resource. This also simplifies the acpi table by removing a Mutex.
BRANCH=poppy BUG=b:171955583 TEST=while true; do if ssh $DUT "dmesg | grep "failed to find sensor" "; then break; fi; ssh $DUT reboot; sleep 30 ; done Signed-off-by: Ricardo Ribalda ribalda@chromium.org
Change-Id: I25df0225699759c1828b8791c5bdee66529858a7 Reviewed-on: https://review.coreboot.org/c/coreboot/+/48631 Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Rizwan Qureshi rizwan.qureshi@intel.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl M src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl M src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl 3 files changed, 78 insertions(+), 96 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Rizwan Qureshi: Looks good to me, approved
diff --git a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl index 8f3ae53..2d5dd0a 100644 --- a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl +++ b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl @@ -23,8 +23,8 @@ ) })
- Name (_PR0, Package () { ^^I2C2.PMIC.OVTH }) - Name (_PR3, Package () { ^^I2C2.PMIC.OVTH }) + Name (_PR0, Package () {^^I2C2.PMIC.OVCM, ^^I2C2.PMIC.OVTH}) + Name (_PR3, Package () {^^I2C2.PMIC.OVCM, ^^I2C2.PMIC.OVTH})
/* Port0 of CAM0 is connected to port0 of CIO2 device */ Name (_DSD, Package () { diff --git a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl index 7e20575..c41ebca 100644 --- a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl +++ b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl @@ -23,8 +23,8 @@ ) })
- Name (_PR0, Package () { ^^I2C2.PMIC.OVFI }) - Name (_PR3, Package () { ^^I2C2.PMIC.OVFI }) + Name (_PR0, Package () {^^I2C2.PMIC.OVCM, ^^I2C2.PMIC.OVFI}) + Name (_PR3, Package () {^^I2C2.PMIC.OVCM, ^^I2C2.PMIC.OVFI})
/* Port0 of CAM1 is connected to port1 of CIO2 device */ Name (_DSD, Package () { diff --git a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl index 5ad10ef..842005a 100644 --- a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl +++ b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/camera_pmic.asl @@ -313,76 +313,50 @@ PODV, 32, }
- /* CLE0 and CLE1 are used to determine if both the clocks - are enabled or disabled. */ - Mutex (MUTC, 0) - Name (CLE0, 0) - Name (CLE1, 0) - Method (CLKE, 0, Serialized) { - /* save Acquire result so we can check for - Mutex acquired */ - Store (Acquire (MUTC, 1000), Local0) - /* check for Mutex acquired */ - If (LEqual (Local0, Zero)) { - /* Enable clocks only when a sensor is turned on and - both the clocks are disabled */ - If (LNot (LOr (CLE0, CLE1))) { - /* Set boost clock divider */ - BODI = 3 - /* Set buck clock divider */ - BUDI = 2 - /* Set the PLL_REF_CLK cyles */ - PSWR = 19 - /* Set the reference crystal divider */ - XTDV = 170 - /* Set PLL feedback divider */ - PLDV = 32 - /* Set PLL output divider for HCLK_A */ - PODV = 1 - /* Set PLL output divider for HCLK_B */ - PDV2 = 1 - /* Enable clocks for both the sensors - * CFG1: output selection for HCLK_A and - * HCLK_B. - * CFG2: set drive strength for HCLK_A - * and HCLK_B. - */ - CFG2 = 5 - CFG1 = 10 - /* Enable PLL output, crystal oscillator - * input capacitance control and set - * Xtal oscillator as clock source. - */ - PCTL = 209 - Sleep(1) - } - Release (MUTC) - } - } - - /* Clocks need to be disabled only if both the sensors are - turned off */ - Method (CLKD, 0, Serialized) { - /* save Acquire result so we can check for - Mutex acquired */ - Store (Acquire (MUTC, 1000), Local0) - /* check for Mutex acquired */ - If (LEqual (Local0, Zero)) { - If (LNot (LOr (CLE0, CLE1))) { - BODI = 0 - BUDI = 0 - PSWR = 0 - XTDV = 0 - PLDV = 0 - PODV = 0 - PDV2 = 0 - /* Disable clocks for both the - sensors */ - CFG2 = 0 - CFG1 = 0 - PCTL = 0 - } - Release (MUTC) + Method (CLK, 1, Serialized) { + If (LEqual (Arg0, Zero)) { + BODI = 0 + BUDI = 0 + PSWR = 0 + XTDV = 0 + PLDV = 0 + PODV = 0 + PDV2 = 0 + /* Disable clocks for both the + sensors */ + CFG2 = 0 + CFG1 = 0 + PCTL = 0 + Sleep(1) + } ElseIf (LEqual (Arg0, 1)) { + /* Set boost clock divider */ + BODI = 3 + /* Set buck clock divider */ + BUDI = 2 + /* Set the PLL_REF_CLK cyles */ + PSWR = 19 + /* Set the reference crystal divider */ + XTDV = 170 + /* Set PLL feedback divider */ + PLDV = 32 + /* Set PLL output divider for HCLK_A */ + PODV = 1 + /* Set PLL output divider for HCLK_B */ + PDV2 = 1 + /* Enable clocks for both the sensors + * CFG1: output selection for HCLK_A and + * HCLK_B. + * CFG2: set drive strength for HCLK_A + * and HCLK_B. + */ + CFG2 = 5 + CFG1 = 10 + /* Enable PLL output, crystal oscillator + * input capacitance control and set + * Xtal oscillator as clock source. + */ + PCTL = 209 + Sleep(1) } }
@@ -425,17 +399,43 @@ } }
- /* Power resource methods for CAM0 */ - PowerResource (OVTH, 0, 0) { + /* Power resource methods for both CAMs */ + PowerResource (OVCM, 0, 0) { Name (STA, 0) Method (_ON, 0, Serialized) { - /* TODO: Read Voltage and Sleep values from Sensor Obj */ If (LEqual (AVBL, 1)) { If (LEqual (STA, 0)) { /* Enable VSIO regulator + daisy chain */ DOVD(1)
+ CLK(1) + STA = 1 + } + } + } + Method (_OFF, 0, Serialized) { + If (LEqual (AVBL, 1)) { + If (LEqual (STA, 1)) { + CLK(0) + Sleep(2) + DOVD(0) + } + } + STA = 0 + } + Method (_STA, 0, NotSerialized) { + Return (STA) + } + } + + /* Power resource methods for CAM0 */ + PowerResource (OVTH, 0, 1) { + Name (STA, 0) + Method (_ON, 0, Serialized) { + /* TODO: Read Voltage and Sleep values from Sensor Obj */ + If (LEqual (AVBL, 1)) { + If (LEqual (STA, 0)) { _SB.PCI0.I2C2.PMIC.CGP1() _SB.PCI0.I2C2.PMIC.CGP2()
@@ -447,9 +447,6 @@ DCVA = 12 VDCT = 1
- _SB.PCI0.I2C2.PMIC.CLKE() - CLE0 = 1 - /* * Wait for all regulator * outputs to settle. @@ -473,16 +470,12 @@ If (LEqual (AVBL, 1)) { If (LEqual (STA, 1)) { Sleep(2) - CLE0 = 0 - _SB.PCI0.I2C2.PMIC.CLKD() - Sleep(2) _SB.PCI0.I2C2.PMIC.CRST(0) Sleep(3) VDCT = 0 Sleep(3) VACT = 0 Sleep(1) - DOVD(0) } } STA = 0 @@ -493,16 +486,12 @@ }
/* Power resource methods for CAM1 */ - PowerResource (OVFI, 0, 0) { + PowerResource (OVFI, 0, 1) { Name (STA, 0) Method (_ON, 0, Serialized) { /* TODO: Read Voltage and Sleep values from Sensor Obj */ If (LEqual (AVBL, 1)) { If (LEqual (STA, 0)) { - /* Enable VSIO regulator + - daisy chain */ - DOVD(1) - /* Set VAUX2 as 1.8006 V */ AX2V = 52 VAX2 = 1 /* Enable VAUX2 */ @@ -522,9 +511,6 @@ /* Wait for VDD to settle. */ Sleep(1)
- _SB.PCI0.I2C2.PMIC.CLKE() - CLE1 = 1 - _SB.PCI0.I2C2.PMIC.CGP5(1) /* * Ensure 10 ms between @@ -540,9 +526,6 @@ If (LEqual (AVBL, 1)) { If (LEqual (STA, 1)) { Sleep(2) - CLE1 = 0 - _SB.PCI0.I2C2.PMIC.CLKD() - Sleep(2) _SB.PCI0.I2C2.PMIC.CGP5(0) Sleep(3) VAX1 = 0 @@ -551,7 +534,6 @@ Sleep(1) VAX2 = 0 Sleep(1) - DOVD(0) } STA = 0 }
-
Furquan Shaikh (Code Review) -
Ricardo Ribalda (Code Review)
-
Rizwan Qureshi (Code Review)