Change in coreboot[master]: Revert "soc/intel/common: Set controller state to active in uart init"

[Christian Walter (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Meera Ravindranath, Usha P, Aamir Bohra, V Sowmya, build bot (Jenkins), Furquan Shaikh,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Revert "soc/intel/common: Set controller state to active in uart init"

This reverts commit 46445155ea21b0aa9106e12a00b9b1d89887a461.

Reason for revert: Breaks coreboot. Either no UART working or the complete boot process stops.

Change-Id: If581f42e423caa76deb4ecf67296a7c2f1f7705d
---
M src/soc/intel/common/block/uart/uart.c
1 file changed, 3 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/34307/1

diff --git a/src/soc/intel/common/block/uart/uart.c b/src/soc/intel/common/block/uart/uart.c
index 82e5df4..9d820ff 100644
--- a/src/soc/intel/common/block/uart/uart.c
+++ b/src/soc/intel/common/block/uart/uart.c
@@ -33,11 +33,8 @@
 extern const struct uart_gpio_pad_config uart_gpio_pads[];
 extern const int uart_max_index;

-static void uart_lpss_init(struct device *dev, uintptr_t baseaddr)
+static void uart_lpss_init(uintptr_t baseaddr)
 {
-	 /* Ensure controller is in D0 state */
-	lpss_set_power_state(dev, STATE_D0);
-
    /* Take UART out of reset */
    lpss_reset_release(baseaddr);

@@ -84,7 +81,7 @@
    /* Enable memory access and bus master */
    pci_write_config32(dev, PCI_COMMAND, UART_PCI_ENABLE);

-	uart_lpss_init(device, baseaddr);
+	uart_lpss_init(baseaddr);
 }

struct device *uart_get_device(void)
@@ -227,7 +224,7 @@

base = pci_read_config32(dev, PCI_BASE_ADDRESS_0) & ~0xFFF;
    	if (base)
-			uart_lpss_init(dev, base);
+			uart_lpss_init(base);
    }
 }

[Christian Walter (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Meera Ravindranath, Usha P, Aamir Bohra, V Sowmya, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Revert "soc/intel/common: Set controller state to active in uart init"

This reverts commit 46445155ea21b0aa9106e12a00b9b1d89887a461.

Reason for revert: Breaks coreboot. Either no UART working or the complete boot process stops.

Platform: Intel Apollolake, on Up Squared

Change-Id: If581f42e423caa76deb4ecf67296a7c2f1f7705d
---
M src/soc/intel/common/block/uart/uart.c
1 file changed, 3 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/34307/2

[Christian Walter (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Meera Ravindranath, Usha P, Aamir Bohra, V Sowmya, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Revert "soc/intel/common: Set controller state to active in uart init"

This reverts commit 46445155ea21b0aa9106e12a00b9b1d89887a461.

Reason for revert: Breaks coreboot. Either no UART working or the complete boot process stops.

Platform: Intel Apollolake, on Up Squared

Change-Id: If581f42e423caa76deb4ecf67296a7c2f1f7705d
Signed-off-by: Christian Walter 
---
M src/soc/intel/common/block/uart/uart.c
1 file changed, 3 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/34307/3

[Christian Walter (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Meera Ravindranath, Usha P, Aamir Bohra, V Sowmya, build bot (Jenkins), Furquan Shaikh,

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

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

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

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Revert "soc/intel/common: Set controller state to active in uart init"

This reverts commit 46445155ea21b0aa9106e12a00b9b1d89887a461.

Reason for revert: Breaks coreboot. Either no UART working or the complete boot process stops.

Platform: Intel Apollolake, tested on Up Squared

Change-Id: If581f42e423caa76deb4ecf67296a7c2f1f7705d
Signed-off-by: Christian Walter 
---
M src/soc/intel/common/block/uart/uart.c
1 file changed, 3 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/34307/4

[Patrick Rudolph (Code Review)]

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

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Patch Set 4: Code-Review+2

[Patrick Rudolph (Code Review)]

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

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Patch Set 4:

Good job Intel, not.

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34307 )

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Patch Set 4:

The original CL should  not have caused the UART to break. The LC was verified on CML and ICL.  The All SOC including APL have same register offset and definitions.  The UART init sequence is also same.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34307 )

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Patch Set 4: Code-Review+2

(1 comment)
> The original CL should  not have caused the UART to break. The LC was verified on CML and ICL.  The All SOC including APL have same register offset and definitions.  The UART init sequence is also same.

Please don't verify your CLs, please review them more carefully
instead. Unless you want to test *all* paths that the affected
code can take, especially untested paths need review. If somebody
writes that a patch was verified, that is a reason to check every-
thing twice! not to check less. I assume it wasn't "verified" that
the PCI config write was actually effective in the bootblock with
CONFIG_INTEL_LPSS_UART_FOR_CONSOLE.

+2 as the reverted patch introduced undefined behaviour. Not really
the fault of that patch, as typing is not taken serious in soc/intel/.

NB. I don't think we should build further on this code, soc/intel/
deserves a rewrite. This time with actual reviews.

https://review.coreboot.org/c/coreboot/+/34307/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/uart/uart.c:

https://review.coreboot.org/c/coreboot/+/34307/4/src/soc/intel/common/block/... 
PS4, Line 74: 
This is proof that the type of `device` is wrong.

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34307 )

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Patch Set 4: Code-Review+2

I can confirm that the reverted patch breaks mc_apl1 board as well.
And the bad taste here again is a merged patch though an active -1 review was there. We should really sort this out some day as it kind of becomes more and more common.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34307 )

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34307/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/uart/uart.c:

https://review.coreboot.org/c/coreboot/+/34307/4/src/soc/intel/common/block/... 
PS4, Line 74:
> This is proof that the type of `device` is wrong.

sorry, this was not meant to be unresolved

[Paul Menzel (Code Review)]

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

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Patch Set 4:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/34307/4//COMMIT_MSG@11 
PS4, Line 11: Reason for revert: Breaks coreboot. Either no UART working or the complete boot process stops.
Please wrap the line after 75 characters.

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34307 )

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Patch Set 4:

Acknowledge the issue. The device reference was not set correct. Will make the required changes in SOC code and push a CL in sometime.

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34307 )

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Patch Set 4:
> Patch Set 4:
>
> Acknowledge the issue. The device reference was not set correct. Will make the required changes in SOC code and push a CL in sometime.

please go ahead and merge this change.
We will fix the broken code and push it after validating across the platforms.

[Aamir Bohra (Code Review)]

Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34307 )

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34307/4/src/soc/intel/common/block/... 
File src/soc/intel/common/block/uart/uart.c:

https://review.coreboot.org/c/coreboot/+/34307/4/src/soc/intel/common/block/... 
PS4, Line 74:
> sorry, this was not meant to be unresolved

Uploaded a CL to address this:
https://review.coreboot.org/c/coreboot/+/34582

Please review.

[Christian Walter (Code Review)]

Hello Werner Zeh, Patrick Rudolph, Subrata Banik, Meera Ravindranath, Aamir Bohra, Usha P, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh,

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

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

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

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Revert "soc/intel/common: Set controller state to active in uart init"

This reverts commit 46445155ea21b0aa9106e12a00b9b1d89887a461.

Reason for revert: Breaks coreboot. Either no UART working or the complete
boot process stops.

Platform: Intel Apollolake, tested on Up Squared

Change-Id: If581f42e423caa76deb4ecf67296a7c2f1f7705d
Signed-off-by: Christian Walter 
---
M src/soc/intel/common/block/uart/uart.c
1 file changed, 3 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/34307/5

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34307 )

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Patch Set 5:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/34307/4//COMMIT_MSG@11 
PS4, Line 11: Reason for revert: Breaks coreboot. Either no UART working or the complete boot process stops.
> Please wrap the line after 75 characters.

Ack

[Christian Walter (Code Review)]

Hello Werner Zeh, Patrick Rudolph, Subrata Banik, Meera Ravindranath, Aamir Bohra, Usha P, V Sowmya, build bot (Jenkins), Nico Huber, Furquan Shaikh,

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

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

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

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Revert "soc/intel/common: Set controller state to active in uart init"

This reverts commit 46445155ea21b0aa9106e12a00b9b1d89887a461.

Reason for revert: Breaks coreboot. Either no UART working or the
complete boot process stops.

Platform: Intel Apollolake, tested on Up Squared

Change-Id: If581f42e423caa76deb4ecf67296a7c2f1f7705d
Signed-off-by: Christian Walter 
---
M src/soc/intel/common/block/uart/uart.c
1 file changed, 3 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/34307/6

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34307 )

Change subject: Revert "soc/intel/common: Set controller state to active in uart init"
......................................................................

Revert "soc/intel/common: Set controller state to active in uart init"

This reverts commit 46445155ea21b0aa9106e12a00b9b1d89887a461.

Reason for revert: Breaks coreboot. Either no UART working or the
complete boot process stops.

Platform: Intel Apollolake, tested on Up Squared

Change-Id: If581f42e423caa76deb4ecf67296a7c2f1f7705d
Signed-off-by: Christian Walter 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34307
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
Reviewed-by: Nico Huber 
Reviewed-by: Werner Zeh 
---
M src/soc/intel/common/block/uart/uart.c
1 file changed, 3 insertions(+), 6 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Werner Zeh: Looks good to me, approved
  Patrick Rudolph: Looks good to me, approved

diff --git a/src/soc/intel/common/block/uart/uart.c b/src/soc/intel/common/block/uart/uart.c
index 82e5df4..9d820ff 100644
--- a/src/soc/intel/common/block/uart/uart.c
+++ b/src/soc/intel/common/block/uart/uart.c
@@ -33,11 +33,8 @@
 extern const struct uart_gpio_pad_config uart_gpio_pads[];
 extern const int uart_max_index;

-static void uart_lpss_init(struct device *dev, uintptr_t baseaddr)
+static void uart_lpss_init(uintptr_t baseaddr)
 {
-	 /* Ensure controller is in D0 state */
-	lpss_set_power_state(dev, STATE_D0);
-
    /* Take UART out of reset */
    lpss_reset_release(baseaddr);

@@ -84,7 +81,7 @@
    /* Enable memory access and bus master */
    pci_write_config32(dev, PCI_COMMAND, UART_PCI_ENABLE);

-	uart_lpss_init(device, baseaddr);
+	uart_lpss_init(baseaddr);
 }

struct device *uart_get_device(void)
@@ -227,7 +224,7 @@

base = pci_read_config32(dev, PCI_BASE_ADDRESS_0) & ~0xFFF;
    	if (base)
-			uart_lpss_init(dev, base);
+			uart_lpss_init(base);
    }
 }