Change in ...coreboot[master]: soc/amd/stoneyridge/southbridge.c: Add new source to sb_clk_output_48Mhz

[Richard Spiegel (Code Review)]

Richard Spiegel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31386

Change subject: soc/amd/stoneyridge/southbridge.c: Add new source to sb_clk_output_48Mhz
......................................................................

soc/amd/stoneyridge/southbridge.c: Add new source to sb_clk_output_48Mhz

In preparation for board padmelon which will use a clock source different
from the one already present in sb_clk_output_48Mhz, change its code to
input the desired clock source and set appropriate register based on this
input.

BUG=b:none.
TEST=Build and boot grunt, test again later with padmelon.

Change-Id: I4b89b1e3b7963472471e34897bdd00176dbdb914
Signed-off-by: Richard Spiegel 
---
M src/soc/amd/stoneyridge/include/soc/southbridge.h
M src/soc/amd/stoneyridge/southbridge.c
2 files changed, 13 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/31386/1

diff --git a/src/soc/amd/stoneyridge/include/soc/southbridge.h b/src/soc/amd/stoneyridge/include/soc/southbridge.h
index 3ae6b4a..705fe7a 100644
--- a/src/soc/amd/stoneyridge/include/soc/southbridge.h
+++ b/src/soc/amd/stoneyridge/include/soc/southbridge.h
@@ -186,6 +186,7 @@
 #define MISC_CLK_CNTL1			0x40
 #define   CG1PLL_FBDIV_TEST		BIT(26)
 #define   OSCOUT1_CLK_OUTPUT_ENB	BIT(2)  /* 0 = Enabled, 1 = Disabled */
+#define   OSCOUT2_CLK_OUTPUT_ENB	BIT(7)  /* 0 = Enabled, 1 = Disabled */

/* XHCI_PM Registers:  0xfed81c00 */
 #define XHCI_PM_INDIRECT_INDEX		0x48
@@ -477,7 +478,7 @@
 void enable_aoac_devices(void);
 void sb_enable_rom(void);
 void configure_stoneyridge_i2c(void);
-void sb_clk_output_48Mhz(void);
+void sb_clk_output_48Mhz(u32 osc);
 void sb_disable_4dw_burst(void);
 void sb_enable(struct device *dev);
 void southbridge_final(void *chip_info);
diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c
index c8d66ac..798bcb4 100644
--- a/src/soc/amd/stoneyridge/southbridge.c
+++ b/src/soc/amd/stoneyridge/southbridge.c
@@ -389,7 +389,7 @@
    pm_write32(PM_DECODE_EN, reg | LEGACY_IO_EN);
 }

-void sb_clk_output_48Mhz(void)
+void sb_clk_output_48Mhz(u32 osc)
 {
    u32 ctrl;
    u32 *misc_clk_cntl_1_ptr = (u32 *)(uintptr_t)(MISC_MMIO_BASE
@@ -401,8 +401,16 @@
     */
    ctrl = read32(misc_clk_cntl_1_ptr);

-	/* clear the OSCOUT1_ClkOutputEnb to enable the 48 Mhz clock */
-	ctrl &= ~OSCOUT1_CLK_OUTPUT_ENB;
+	switch (osc) {
+	case 1:
+		ctrl &= ~OSCOUT1_CLK_OUTPUT_ENB;
+		break;
+	case 2:
+		ctrl &= ~OSCOUT2_CLK_OUTPUT_ENB;
+		break;
+	default:
+		return; /* do nothing if invalid */
+	}
    write32(misc_clk_cntl_1_ptr, ctrl);
 }

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/stoneyridge/southbridge.c: Add new source to sb_clk_output_48Mhz
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/31386/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31386/1//COMMIT_MSG@9 
PS1, Line 9: In preparation for board padmelon which will use a clock source different
           : from the one already present in sb_clk_output_48Mhz, change its code to
           : input the desired clock source and set appropriate register based on this
           : input.
I wouldn't bother with mentioning prep for Padmelon.  You're adding support for existing Stoney Ridge functionality.  Also, it's not a clock source or input.  It's an oscillator out.

The original patch's commit message said:

soc/amd/stoneyridge: Expand 48MHz for both osc out signals

There are typically two configurable oscillator outputs available on APUs or FCHs.  Convert the enable function to all either one.

https://review.coreboot.org/#/c/31386/1/src/soc/amd/stoneyridge/southbridge.... 
File src/soc/amd/stoneyridge/southbridge.c:

https://review.coreboot.org/#/c/31386/1/src/soc/amd/stoneyridge/southbridge.... 
PS1, Line 398: 	/*
             : 	 * Enable the X14M_25M_48M_OSC pin and leaving it at it's default so
             : 	 * 48Mhz will be on ball AP13 (FT3b package)
             : 	 */
The original work this patch is based on changed this comment to say something like, "Clear the disable for OSCOUT1 (signal typically named XnnM_25M_48M) or OSCOUT2 (USBCLK/25M_48M_OSC).  The frequency defaults to 48MHz."  We shouldn't mention the package at all; FT3b and ball AP13 shouldn't have made their way into the original ST port.

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31386 )

Change subject: soc/amd/stoneyridge/southbridge.c: Add new source to sb_clk_output_48Mhz
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/31386/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31386/1//COMMIT_MSG@9 
PS1, Line 9: In preparation for board padmelon which will use a clock source different
           : from the one already present in sb_clk_output_48Mhz, change its code to
           : input the desired clock source and set appropriate register based on this
           : input.
> I wouldn't bother with mentioning prep for Padmelon. […]

Ok, will use your message with a small change to the last sentence.

https://review.coreboot.org/#/c/31386/1/src/soc/amd/stoneyridge/southbridge.... 
File src/soc/amd/stoneyridge/southbridge.c:

https://review.coreboot.org/#/c/31386/1/src/soc/amd/stoneyridge/southbridge.... 
PS1, Line 398: 	/*
             : 	 * Enable the X14M_25M_48M_OSC pin and leaving it at it's default so
             : 	 * 48Mhz will be on ball AP13 (FT3b package)
             : 	 */
> The original work this patch is based on changed this comment to say something like, "Clear the disa […]

Ok, will change.

Change in ...coreboot[master]: soc/amd/stoneyridge: Expand 48MHz for both osc out signals

[Richard Spiegel (Code Review)]

Hello Marshall Dawson, build bot (Jenkins), Martin Roth, Patrick Georgi,

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

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

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

Change subject: soc/amd/stoneyridge: Expand 48MHz for both osc out signals
......................................................................

soc/amd/stoneyridge: Expand 48MHz for both osc out signals

There are typically two configurable oscillator outputs available on APUs
or FCHs.  Convert the enable function to work with either one.

BUG=b:none.
TEST=Build and boot grunt.

Change-Id: I4b89b1e3b7963472471e34897bdd00176dbdb914
Signed-off-by: Richard Spiegel 
---
M src/soc/amd/stoneyridge/include/soc/southbridge.h
M src/soc/amd/stoneyridge/southbridge.c
2 files changed, 15 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/31386/2

Change in ...coreboot[master]: soc/amd/stoneyridge: Expand 48MHz for both osc out signals

[Marshall Dawson (Code Review)]

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

Change subject: soc/amd/stoneyridge: Expand 48MHz for both osc out signals
......................................................................

Patch Set 2: Code-Review+2

Change in ...coreboot[master]: soc/amd/stoneyridge: Expand 48MHz for both osc out signals

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31386 )

Change subject: soc/amd/stoneyridge: Expand 48MHz for both osc out signals
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31386/2/src/soc/amd/stoneyridge/southbridge.... 
File src/soc/amd/stoneyridge/southbridge.c:

https://review.coreboot.org/#/c/31386/2/src/soc/amd/stoneyridge/southbridge.... 
PS2, Line 393: {
With the function name like this, it should really set the clock to 48MHz and not rely on previously set register value.

Change in ...coreboot[master]: soc/amd/stoneyridge: Expand 48MHz for both osc out signals

[Richard Spiegel (Code Review)]

Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31386 )

Change subject: soc/amd/stoneyridge: Expand 48MHz for both osc out signals
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31386/2/src/soc/amd/stoneyridge/southbridge.... 
File src/soc/amd/stoneyridge/southbridge.c:

https://review.coreboot.org/#/c/31386/2/src/soc/amd/stoneyridge/southbridge.... 
PS2, Line 393: {
> With the function name like this, it should really set the clock to 48MHz and not rely on previously […]

The power on reset value for these bits is 1. So they are set at reset.

Change in ...coreboot[master]: soc/amd/stoneyridge: Expand 48MHz for both osc out signals

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31386 )

Change subject: soc/amd/stoneyridge: Expand 48MHz for both osc out signals
......................................................................

soc/amd/stoneyridge: Expand 48MHz for both osc out signals

There are typically two configurable oscillator outputs available on APUs
or FCHs.  Convert the enable function to work with either one.

BUG=b:none.
TEST=Build and boot grunt.

Change-Id: I4b89b1e3b7963472471e34897bdd00176dbdb914
Signed-off-by: Richard Spiegel 
Reviewed-on: https://review.coreboot.org/c/31386
Tested-by: build bot (Jenkins) 
Reviewed-by: Marshall Dawson 
---
M src/soc/amd/stoneyridge/include/soc/southbridge.h
M src/soc/amd/stoneyridge/southbridge.c
2 files changed, 15 insertions(+), 6 deletions(-)

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

diff --git a/src/soc/amd/stoneyridge/include/soc/southbridge.h b/src/soc/amd/stoneyridge/include/soc/southbridge.h
index 3ae6b4a..705fe7a 100644
--- a/src/soc/amd/stoneyridge/include/soc/southbridge.h
+++ b/src/soc/amd/stoneyridge/include/soc/southbridge.h
@@ -186,6 +186,7 @@
 #define MISC_CLK_CNTL1			0x40
 #define   CG1PLL_FBDIV_TEST		BIT(26)
 #define   OSCOUT1_CLK_OUTPUT_ENB	BIT(2)  /* 0 = Enabled, 1 = Disabled */
+#define   OSCOUT2_CLK_OUTPUT_ENB	BIT(7)  /* 0 = Enabled, 1 = Disabled */

/* XHCI_PM Registers:  0xfed81c00 */
 #define XHCI_PM_INDIRECT_INDEX		0x48
@@ -477,7 +478,7 @@
 void enable_aoac_devices(void);
 void sb_enable_rom(void);
 void configure_stoneyridge_i2c(void);
-void sb_clk_output_48Mhz(void);
+void sb_clk_output_48Mhz(u32 osc);
 void sb_disable_4dw_burst(void);
 void sb_enable(struct device *dev);
 void southbridge_final(void *chip_info);
diff --git a/src/soc/amd/stoneyridge/southbridge.c b/src/soc/amd/stoneyridge/southbridge.c
index c8d66ac..dfbd160 100644
--- a/src/soc/amd/stoneyridge/southbridge.c
+++ b/src/soc/amd/stoneyridge/southbridge.c
@@ -389,20 +389,28 @@
    pm_write32(PM_DECODE_EN, reg | LEGACY_IO_EN);
 }

-void sb_clk_output_48Mhz(void)
+void sb_clk_output_48Mhz(u32 osc)
 {
    u32 ctrl;
    u32 *misc_clk_cntl_1_ptr = (u32 *)(uintptr_t)(MISC_MMIO_BASE
    			+ MISC_CLK_CNTL1);

/*
-	 * Enable the X14M_25M_48M_OSC pin and leaving it at it's default so
-	 * 48Mhz will be on ball AP13 (FT3b package)
+	 * Clear the disable for OSCOUT1 (signal typically named XnnM_25M_48M)
+	 * or OSCOUT2 (USBCLK/25M_48M_OSC).  The frequency defaults to 48MHz.
     */
    ctrl = read32(misc_clk_cntl_1_ptr);

-	/* clear the OSCOUT1_ClkOutputEnb to enable the 48 Mhz clock */
-	ctrl &= ~OSCOUT1_CLK_OUTPUT_ENB;
+	switch (osc) {
+	case 1:
+		ctrl &= ~OSCOUT1_CLK_OUTPUT_ENB;
+		break;
+	case 2:
+		ctrl &= ~OSCOUT2_CLK_OUTPUT_ENB;
+		break;
+	default:
+		return; /* do nothing if invalid */
+	}
    write32(misc_clk_cntl_1_ptr, ctrl);
 }