Change in ...coreboot[master]: mediatek: Support GPIO based SPI CS

[You-Cheng Syu (Code Review)]

You-Cheng Syu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33165

Change subject: mediatek: Support GPIO based SPI CS
......................................................................

mediatek: Support GPIO based SPI CS

In some cases we might want to use GPIO based SPI CS. When using GPIO
based SPI CS, we need to manually make CS low/high before/after SPI
transactions.

BUG=b:132311067
TEST=Verified that b/132311067 is irreproducible now.

Change-Id: I61653fb19242b6ee6be9a45545a8b66e5c9c7cad
---
M src/soc/mediatek/common/include/soc/spi_common.h
M src/soc/mediatek/common/spi.c
2 files changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/33165/1

diff --git a/src/soc/mediatek/common/include/soc/spi_common.h b/src/soc/mediatek/common/include/soc/spi_common.h
index 162db59..2e87720 100644
--- a/src/soc/mediatek/common/include/soc/spi_common.h
+++ b/src/soc/mediatek/common/include/soc/spi_common.h
@@ -16,6 +16,7 @@
 #ifndef MTK_COMMON_SPI_H
 #define MTK_COMMON_SPI_H

+#include <soc/gpio_base.h>
 #include <spi-generic.h>

enum {
@@ -77,6 +78,7 @@
    struct mtk_spi_regs *regs;
    int initialized;
    int state;
+	gpio_t *cs_gpio;
 };

extern const struct spi_ctrlr spi_ctrlr;
diff --git a/src/soc/mediatek/common/spi.c b/src/soc/mediatek/common/spi.c
index 71ed95a..c8aadbf 100644
--- a/src/soc/mediatek/common/spi.c
+++ b/src/soc/mediatek/common/spi.c
@@ -17,6 +17,7 @@
 #include <assert.h>
 #include <console/console.h>
 #include <endian.h>
+#include <gpio.h>
 #include <stdlib.h>
 #include <soc/pll.h>
 #include <soc/spi.h>
@@ -87,6 +88,9 @@

clrsetbits_le32(&regs->spi_pad_macro_sel_reg, SPI_PAD_SEL_MASK,
    		pad_select);
+
+	if (slave->cs_gpio != NULL)
+		gpio_output(*slave->cs_gpio, 1);
 }

static void mtk_spi_dump_data(const char *name, const uint8_t *data, int size)
@@ -109,6 +113,9 @@
    setbits_le32(&regs->spi_cmd_reg, 1 << SPI_CMD_PAUSE_EN_SHIFT);
    mtk_slave->state = MTK_SPI_IDLE;

+	if (mtk_slave->cs_gpio != NULL)
+		gpio_output(*mtk_slave->cs_gpio, 0);
+
    return 0;
 }

@@ -243,6 +250,9 @@
    clrbits_le32(&regs->spi_cmd_reg, SPI_CMD_PAUSE_EN);
    spi_sw_reset(regs);
    mtk_slave->state = MTK_SPI_IDLE;
+
+	if (mtk_slave->cs_gpio != NULL)
+		gpio_output(*mtk_slave->cs_gpio, 1);
 }

static int spi_ctrlr_setup(const struct spi_slave *slave)

[You-Cheng Syu (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: mediatek: Support GPIO based SPI CS
......................................................................

mediatek: Support GPIO based SPI CS

In some cases we might want to use GPIO based SPI CS. When using GPIO
based SPI CS, we need to manually make CS low/high before/after SPI
transactions.

BUG=b:132311067
TEST=Verified that b/132311067 is irreproducible now.

Change-Id: I61653fb19242b6ee6be9a45545a8b66e5c9c7cad
Signed-off-by: You-Cheng Syu 
---
M src/soc/mediatek/common/include/soc/spi_common.h
M src/soc/mediatek/common/spi.c
2 files changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/33165/2

[Julius Werner (Code Review)]

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

Change subject: mediatek: Support GPIO based SPI CS
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/33165/2/src/soc/mediatek/common/include/soc/... 
File src/soc/mediatek/common/include/soc/spi_common.h:

https://review.coreboot.org/#/c/33165/2/src/soc/mediatek/common/include/soc/... 
PS2, Line 81: 	gpio_t *cs_gpio;
Like mentioned in the depthcharge CL, I think we should just do this unconditionally. Then this doesn't need to be a pointer, just a gpio_t.

Change in coreboot[master]: mediatek: Use GPIO based SPI CS

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has uploaded a new patch set (#3) to the change originally created by You-Cheng Syu. ( https://review.coreboot.org/c/coreboot/+/33165 )

Change subject: mediatek: Use GPIO based SPI CS
......................................................................

mediatek: Use GPIO based SPI CS

Some boards (e.g., Kukui) need GPIO based CS for SPI0. This patch
changes the pinmux and binds the pins to the correponding SPIs.

When using GPIO based SPI CS, we need to manually make CS log/high
before/after SPI transactions.

BUG=b:132311067
BRANCH=none
TEST=Verified that b/132311067 is irreproducible

Change-Id: I61653fb19242b6ee6be9a45545a8b66e5c9c7cad
Signed-off-by: Yu-Ping Wu 
---
M src/soc/mediatek/common/include/soc/spi_common.h
M src/soc/mediatek/common/spi.c
M src/soc/mediatek/mt8173/spi.c
M src/soc/mediatek/mt8183/spi.c
4 files changed, 26 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/33165/3

Change in coreboot[master]: mediatek: Use GPIO based SPI CS

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33165 )

Change subject: mediatek: Use GPIO based SPI CS
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/33165/2/src/soc/mediatek/common/inc... 
File src/soc/mediatek/common/include/soc/spi_common.h:

https://review.coreboot.org/c/coreboot/+/33165/2/src/soc/mediatek/common/inc... 
PS2, Line 81: 	gpio_t *cs_gpio;
> Like mentioned in the depthcharge CL, I think we should just do this unconditionally. […]

Done

Change in coreboot[master]: mediatek: Use GPIO based SPI CS

[Hung-Te Lin (Code Review)]

Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33165 )

Change subject: mediatek: Use GPIO based SPI CS
......................................................................

Patch Set 3: Code-Review+1

So we only need one change right?

Change in coreboot[master]: mediatek: Use GPIO based SPI CS

[Yu-Ping Wu (Code Review)]

Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33165 )

Change subject: mediatek: Use GPIO based SPI CS
......................................................................

Patch Set 3:
> Patch Set 3: Code-Review+1
>
> So we only need one change right?

Yes, just this CL.

Change in coreboot[master]: mediatek: Use GPIO based SPI CS

[Julius Werner (Code Review)]

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

Change subject: mediatek: Use GPIO based SPI CS
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: mediatek: Use GPIO based SPI CS

[Patrick Georgi (Code Review)]

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

Change subject: mediatek: Use GPIO based SPI CS
......................................................................

mediatek: Use GPIO based SPI CS

Some boards (e.g., Kukui) need GPIO based CS for SPI0. This patch
changes the pinmux and binds the pins to the correponding SPIs.

When using GPIO based SPI CS, we need to manually make CS log/high
before/after SPI transactions.

BUG=b:132311067
BRANCH=none
TEST=Verified that b/132311067 is irreproducible

Change-Id: I61653fb19242b6ee6be9a45545a8b66e5c9c7cad
Signed-off-by: Yu-Ping Wu 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33165
Tested-by: build bot (Jenkins) 
Reviewed-by: Hung-Te Lin 
Reviewed-by: Julius Werner 
---
M src/soc/mediatek/common/include/soc/spi_common.h
M src/soc/mediatek/common/spi.c
M src/soc/mediatek/mt8173/spi.c
M src/soc/mediatek/mt8183/spi.c
4 files changed, 26 insertions(+), 8 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved
  Hung-Te Lin: Looks good to me, but someone else must approve

diff --git a/src/soc/mediatek/common/include/soc/spi_common.h b/src/soc/mediatek/common/include/soc/spi_common.h
index 81a9098..1ecb94d 100644
--- a/src/soc/mediatek/common/include/soc/spi_common.h
+++ b/src/soc/mediatek/common/include/soc/spi_common.h
@@ -16,6 +16,7 @@
 #ifndef MTK_COMMON_SPI_H
 #define MTK_COMMON_SPI_H

+#include <soc/gpio_base.h>
 #include <spi-generic.h>

enum {
@@ -77,6 +78,7 @@
    struct mtk_spi_regs *regs;
    int initialized;
    int state;
+	gpio_t cs_gpio;
 };

extern const struct spi_ctrlr spi_ctrlr;
diff --git a/src/soc/mediatek/common/spi.c b/src/soc/mediatek/common/spi.c
index 40ab9b7..2a668fe 100644
--- a/src/soc/mediatek/common/spi.c
+++ b/src/soc/mediatek/common/spi.c
@@ -17,6 +17,8 @@
 #include <assert.h>
 #include <console/console.h>
 #include <endian.h>
+#include <gpio.h>
+#include <stdlib.h>
 #include <soc/pll.h>
 #include <soc/spi.h>
 #include <timer.h>
@@ -87,6 +89,8 @@

clrsetbits_le32(&regs->spi_pad_macro_sel_reg, SPI_PAD_SEL_MASK,
    		pad_select);
+
+	gpio_output(slave->cs_gpio, 1);
 }

static void mtk_spi_dump_data(const char *name, const uint8_t *data, int size)
@@ -109,6 +113,8 @@
    setbits_le32(&regs->spi_cmd_reg, 1 << SPI_CMD_PAUSE_EN_SHIFT);
    mtk_slave->state = MTK_SPI_IDLE;

+	gpio_output(mtk_slave->cs_gpio, 0);
+
    return 0;
 }

@@ -243,6 +249,8 @@
    clrbits_le32(&regs->spi_cmd_reg, SPI_CMD_PAUSE_EN);
    spi_sw_reset(regs);
    mtk_slave->state = MTK_SPI_IDLE;
+
+	gpio_output(mtk_slave->cs_gpio, 1);
 }

static int spi_ctrlr_setup(const struct spi_slave *slave)
diff --git a/src/soc/mediatek/mt8173/spi.c b/src/soc/mediatek/mt8173/spi.c
index d009441..cf2ffa2 100644
--- a/src/soc/mediatek/mt8173/spi.c
+++ b/src/soc/mediatek/mt8173/spi.c
@@ -24,6 +24,7 @@
 struct mtk_spi_bus spi_bus[SPI_BUS_NUMBER] = {
    {
    	.regs = (void *)SPI_BASE,
+		.cs_gpio = GPIO(MSDC2_CMD),
    }
 };

@@ -35,7 +36,7 @@
    gpio_set_mode(GPIO(MSDC2_DAT2), PAD_MSDC2_DAT2_FUNC_SPI_CK_1);
    gpio_set_mode(GPIO(MSDC2_DAT3), PAD_MSDC2_DAT3_FUNC_SPI_MI_1);
    gpio_set_mode(GPIO(MSDC2_CLK), PAD_MSDC2_CLK_FUNC_SPI_MO_1);
-	gpio_set_mode(GPIO(MSDC2_CMD), PAD_MSDC2_CMD_FUNC_SPI_CS_1);
+	gpio_set_mode(GPIO(MSDC2_CMD), 0);
 }

void mtk_spi_set_timing(struct mtk_spi_regs *regs, u32 sck_ticks, u32 cs_ticks,
diff --git a/src/soc/mediatek/mt8183/spi.c b/src/soc/mediatek/mt8183/spi.c
index 982f643..7672db7 100644
--- a/src/soc/mediatek/mt8183/spi.c
+++ b/src/soc/mediatek/mt8183/spi.c
@@ -24,21 +24,27 @@
 struct mtk_spi_bus spi_bus[SPI_BUS_NUMBER] = {
    {
    	.regs = (void *)SPI0_BASE,
+		.cs_gpio = GPIO(SPI_CSB),
    },
    {
    	.regs = (void *)SPI1_BASE,
+		.cs_gpio = GPIO(SPI1_CSB),
    },
    {
    	.regs = (void *)SPI2_BASE,
+		.cs_gpio = GPIO(EINT0),
    },
    {
    	.regs = (void *)SPI3_BASE,
+		.cs_gpio = GPIO(DPI_D9),
    },
    {
    	.regs = (void *)SPI4_BASE,
+		.cs_gpio = GPIO(DPI_D5),
    },
    {
    	.regs = (void *)SPI5_BASE,
+		.cs_gpio = GPIO(DPI_D1),
    }
 };

@@ -48,41 +54,42 @@
 };

#define PAD_FUNC(name, func) {PAD_##name##_ID, PAD_##name##_FUNC_##func}
+#define PAD_FUNC_GPIO(name) {PAD_##name##_ID, 0}

static const struct pad_func pad0_funcs[SPI_BUS_NUMBER][4] = {
    {
    	PAD_FUNC(SPI_MI, SPI0_MI),
-		PAD_FUNC(SPI_CSB, SPI0_CSB),
+		PAD_FUNC_GPIO(SPI_CSB),
    	PAD_FUNC(SPI_MO, SPI0_MO),
    	PAD_FUNC(SPI_CLK, SPI0_CLK),
    },
    {
    	PAD_FUNC(SPI1_MI, SPI1_A_MI),
-		PAD_FUNC(SPI1_CSB, SPI1_A_CSB),
+		PAD_FUNC_GPIO(SPI1_CSB),
    	PAD_FUNC(SPI1_MO, SPI1_A_MO),
    	PAD_FUNC(SPI1_CLK, SPI1_A_CLK),
    },
    {
    	PAD_FUNC(KPCOL1, SPI2_MI),
-		PAD_FUNC(EINT0, SPI2_CSB),
+		PAD_FUNC_GPIO(EINT0),
    	PAD_FUNC(EINT1, SPI2_MO),
    	PAD_FUNC(EINT2, SPI2_CLK),
    },
    {
    	PAD_FUNC(DPI_D8, SPI3_MI),
-		PAD_FUNC(DPI_D9, SPI3_CSB),
+		PAD_FUNC_GPIO(DPI_D9),
    	PAD_FUNC(DPI_D10, SPI3_MO),
    	PAD_FUNC(DPI_D11, SPI3_CLK),
    },
    {
    	PAD_FUNC(DPI_D4, SPI4_MI),
-		PAD_FUNC(DPI_D5, SPI4_CSB),
+		PAD_FUNC_GPIO(DPI_D5),
    	PAD_FUNC(DPI_D6, SPI4_MO),
    	PAD_FUNC(DPI_D7, SPI4_CLK),
    },
    {
    	PAD_FUNC(DPI_D0, SPI5_MI),
-		PAD_FUNC(DPI_D1, SPI5_CSB),
+		PAD_FUNC_GPIO(DPI_D1),
    	PAD_FUNC(DPI_D2, SPI5_MO),
    	PAD_FUNC(DPI_D3, SPI5_CLK),
    }
@@ -90,7 +97,7 @@

static const struct pad_func bus1_pad1_funcs[4] = {
    PAD_FUNC(EINT7, SPI1_B_MI),
-	PAD_FUNC(EINT8, SPI1_B_CSB),
+	PAD_FUNC_GPIO(EINT8),
    PAD_FUNC(EINT9, SPI1_B_MO),
    PAD_FUNC(EINT10, SPI1_B_CLK),
 };