All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] Fix AST2600 quad mode SPI pinmux settings
@ 2022-03-08  0:37 ` Jae Hyun Yoo
  0 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-08  0:37 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Andrew Jeffery, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Jae Hyun Yoo

I’m sending this patch series to fix current issues in AST2600 pinmux
settings while enabling quad mode SPI support.

FWSPI18 pins are basically 1.8v logic pins that are different from the
dedicated FWSPI pins that provide 3.3v logic level, so FWSPI18 pins can’t
be grouped with FWSPIDQ2 and FWSPIDQ3, so this series fix the issue.

Also, fixes QSPI1 and QSPI2 function settings in AST2600 pinctrl dtsi to
make it able to enable quad mode on SPI1 and SPI2 interfaces.

With this series, quad mode pinmux can be set like below.

FW SPI:
&fmc {
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_fwqspi_default>;
}

SPI1:
&spi1 {
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_qspi1_default>;
}

SPI2:
&spi1 {
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_qspi1_default>;
}

Please review.

Thanks,
Jae


Jae Hyun Yoo (3):
  ARM: dts: aspeed-g6: remove FWQSPID group in pinctrl dtsi
  pinctrl: pinctrl-aspeed-g6: remove FWQSPID group in pinctrl dtsi
  ARM: dts: aspeed-g6: fix SPI1/SPI2 quad pin group

Johnny Huang (2):
  pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
  ARM: dts: aspeed-g6: add FWQSPI group in pinctrl dtsi

 arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi   | 10 +++++-----
 drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 17 ++++++++---------
 2 files changed, 13 insertions(+), 14 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 0/5] Fix AST2600 quad mode SPI pinmux settings
@ 2022-03-08  0:37 ` Jae Hyun Yoo
  0 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-08  0:37 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Andrew Jeffery, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Jae Hyun Yoo

I’m sending this patch series to fix current issues in AST2600 pinmux
settings while enabling quad mode SPI support.

FWSPI18 pins are basically 1.8v logic pins that are different from the
dedicated FWSPI pins that provide 3.3v logic level, so FWSPI18 pins can’t
be grouped with FWSPIDQ2 and FWSPIDQ3, so this series fix the issue.

Also, fixes QSPI1 and QSPI2 function settings in AST2600 pinctrl dtsi to
make it able to enable quad mode on SPI1 and SPI2 interfaces.

With this series, quad mode pinmux can be set like below.

FW SPI:
&fmc {
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_fwqspi_default>;
}

SPI1:
&spi1 {
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_qspi1_default>;
}

SPI2:
&spi1 {
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_qspi1_default>;
}

Please review.

Thanks,
Jae


Jae Hyun Yoo (3):
  ARM: dts: aspeed-g6: remove FWQSPID group in pinctrl dtsi
  pinctrl: pinctrl-aspeed-g6: remove FWQSPID group in pinctrl dtsi
  ARM: dts: aspeed-g6: fix SPI1/SPI2 quad pin group

Johnny Huang (2):
  pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
  ARM: dts: aspeed-g6: add FWQSPI group in pinctrl dtsi

 arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi   | 10 +++++-----
 drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 17 ++++++++---------
 2 files changed, 13 insertions(+), 14 deletions(-)

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH 1/5] ARM: dts: aspeed-g6: remove FWQSPID group in pinctrl dtsi
  2022-03-08  0:37 ` Jae Hyun Yoo
@ 2022-03-08  0:37   ` Jae Hyun Yoo
  -1 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-08  0:37 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Andrew Jeffery, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Jae Hyun Yoo

FWSPIDQ2 and FWSPIDQ3 are not part of FWSPI18 interface so remove
FWQSPID group in pinctrl dtsi. These pins must be used with the
FWSPI pins that are dedicated for boot SPI interface which provides
same 3.3v logic level.

Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
Fixes: f510f04c8c83 ("ARM: dts: aspeed: Add AST2600 pinmux nodes")
---
 arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
index 6dde51c2aed3..06d60a8540e9 100644
--- a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
@@ -117,11 +117,6 @@ pinctrl_fwspid_default: fwspid_default {
 		groups = "FWSPID";
 	};
 
-	pinctrl_fwqspid_default: fwqspid_default {
-		function = "FWQSPID";
-		groups = "FWQSPID";
-	};
-
 	pinctrl_fwspiwp_default: fwspiwp_default {
 		function = "FWSPIWP";
 		groups = "FWSPIWP";
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 1/5] ARM: dts: aspeed-g6: remove FWQSPID group in pinctrl dtsi
@ 2022-03-08  0:37   ` Jae Hyun Yoo
  0 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-08  0:37 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Andrew Jeffery, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Jae Hyun Yoo

FWSPIDQ2 and FWSPIDQ3 are not part of FWSPI18 interface so remove
FWQSPID group in pinctrl dtsi. These pins must be used with the
FWSPI pins that are dedicated for boot SPI interface which provides
same 3.3v logic level.

Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
Fixes: f510f04c8c83 ("ARM: dts: aspeed: Add AST2600 pinmux nodes")
---
 arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
index 6dde51c2aed3..06d60a8540e9 100644
--- a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
@@ -117,11 +117,6 @@ pinctrl_fwspid_default: fwspid_default {
 		groups = "FWSPID";
 	};
 
-	pinctrl_fwqspid_default: fwqspid_default {
-		function = "FWQSPID";
-		groups = "FWQSPID";
-	};
-
 	pinctrl_fwspiwp_default: fwspiwp_default {
 		function = "FWSPIWP";
 		groups = "FWSPIWP";
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 2/5] pinctrl: pinctrl-aspeed-g6: remove FWQSPID group in pinctrl dtsi
  2022-03-08  0:37 ` Jae Hyun Yoo
@ 2022-03-08  0:37   ` Jae Hyun Yoo
  -1 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-08  0:37 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Andrew Jeffery, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Jae Hyun Yoo

FWSPIDQ2 and FWSPIDQ3 are not part of FWSPI18 interface so remove
FWQSPID group in pinctrl dtsi. These pins must be used with the
FWSPI pins that are dedicated for boot SPI interface which provides
same 3.3v logic level.

Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
Fixes: 2eda1cdec49f ("pinctrl: aspeed: Add AST2600 pinmux support")
---
 drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
index a3fa03bcd9a3..54064714d73f 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
@@ -1236,18 +1236,12 @@ FUNC_GROUP_DECL(SALT8, AA12);
 FUNC_GROUP_DECL(WDTRST4, AA12);
 
 #define AE12 196
-SIG_EXPR_LIST_DECL_SEMG(AE12, FWSPIDQ2, FWQSPID, FWSPID,
-			SIG_DESC_SET(SCU438, 4));
 SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
-PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIDQ2),
-	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
+PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
 
 #define AF12 197
-SIG_EXPR_LIST_DECL_SEMG(AF12, FWSPIDQ3, FWQSPID, FWSPID,
-			SIG_DESC_SET(SCU438, 5));
 SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
-PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIDQ3),
-	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
+PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
 
 #define AC12 198
 SSSF_PIN_DECL(AC12, GPIOY6, FWSPIABR, SIG_DESC_SET(SCU438, 6));
@@ -1520,9 +1514,8 @@ SIG_EXPR_LIST_DECL_SEMG(Y4, EMMCDAT7, EMMCG8, EMMC, SIG_DESC_SET(SCU404, 3));
 PIN_DECL_3(Y4, GPIO18E3, FWSPIDMISO, VBMISO, EMMCDAT7);
 
 GROUP_DECL(FWSPID, Y1, Y2, Y3, Y4);
-GROUP_DECL(FWQSPID, Y1, Y2, Y3, Y4, AE12, AF12);
 GROUP_DECL(EMMCG8, AB4, AA4, AC4, AA5, Y5, AB5, AB6, AC5, Y1, Y2, Y3, Y4);
-FUNC_DECL_2(FWSPID, FWSPID, FWQSPID);
+FUNC_DECL_1(FWSPID, FWSPID);
 FUNC_GROUP_DECL(VB, Y1, Y2, Y3, Y4);
 FUNC_DECL_3(EMMC, EMMCG1, EMMCG4, EMMCG8);
 /*
@@ -1918,7 +1911,6 @@ static const struct aspeed_pin_group aspeed_g6_groups[] = {
 	ASPEED_PINCTRL_GROUP(FSI2),
 	ASPEED_PINCTRL_GROUP(FWSPIABR),
 	ASPEED_PINCTRL_GROUP(FWSPID),
-	ASPEED_PINCTRL_GROUP(FWQSPID),
 	ASPEED_PINCTRL_GROUP(FWSPIWP),
 	ASPEED_PINCTRL_GROUP(GPIT0),
 	ASPEED_PINCTRL_GROUP(GPIT1),
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 2/5] pinctrl: pinctrl-aspeed-g6: remove FWQSPID group in pinctrl dtsi
@ 2022-03-08  0:37   ` Jae Hyun Yoo
  0 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-08  0:37 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Andrew Jeffery, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Jae Hyun Yoo

FWSPIDQ2 and FWSPIDQ3 are not part of FWSPI18 interface so remove
FWQSPID group in pinctrl dtsi. These pins must be used with the
FWSPI pins that are dedicated for boot SPI interface which provides
same 3.3v logic level.

Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
Fixes: 2eda1cdec49f ("pinctrl: aspeed: Add AST2600 pinmux support")
---
 drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
index a3fa03bcd9a3..54064714d73f 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
@@ -1236,18 +1236,12 @@ FUNC_GROUP_DECL(SALT8, AA12);
 FUNC_GROUP_DECL(WDTRST4, AA12);
 
 #define AE12 196
-SIG_EXPR_LIST_DECL_SEMG(AE12, FWSPIDQ2, FWQSPID, FWSPID,
-			SIG_DESC_SET(SCU438, 4));
 SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
-PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIDQ2),
-	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
+PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
 
 #define AF12 197
-SIG_EXPR_LIST_DECL_SEMG(AF12, FWSPIDQ3, FWQSPID, FWSPID,
-			SIG_DESC_SET(SCU438, 5));
 SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
-PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIDQ3),
-	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
+PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
 
 #define AC12 198
 SSSF_PIN_DECL(AC12, GPIOY6, FWSPIABR, SIG_DESC_SET(SCU438, 6));
@@ -1520,9 +1514,8 @@ SIG_EXPR_LIST_DECL_SEMG(Y4, EMMCDAT7, EMMCG8, EMMC, SIG_DESC_SET(SCU404, 3));
 PIN_DECL_3(Y4, GPIO18E3, FWSPIDMISO, VBMISO, EMMCDAT7);
 
 GROUP_DECL(FWSPID, Y1, Y2, Y3, Y4);
-GROUP_DECL(FWQSPID, Y1, Y2, Y3, Y4, AE12, AF12);
 GROUP_DECL(EMMCG8, AB4, AA4, AC4, AA5, Y5, AB5, AB6, AC5, Y1, Y2, Y3, Y4);
-FUNC_DECL_2(FWSPID, FWSPID, FWQSPID);
+FUNC_DECL_1(FWSPID, FWSPID);
 FUNC_GROUP_DECL(VB, Y1, Y2, Y3, Y4);
 FUNC_DECL_3(EMMC, EMMCG1, EMMCG4, EMMCG8);
 /*
@@ -1918,7 +1911,6 @@ static const struct aspeed_pin_group aspeed_g6_groups[] = {
 	ASPEED_PINCTRL_GROUP(FSI2),
 	ASPEED_PINCTRL_GROUP(FWSPIABR),
 	ASPEED_PINCTRL_GROUP(FWSPID),
-	ASPEED_PINCTRL_GROUP(FWQSPID),
 	ASPEED_PINCTRL_GROUP(FWSPIWP),
 	ASPEED_PINCTRL_GROUP(GPIT0),
 	ASPEED_PINCTRL_GROUP(GPIT1),
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
  2022-03-08  0:37 ` Jae Hyun Yoo
@ 2022-03-08  0:37   ` Jae Hyun Yoo
  -1 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-08  0:37 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Andrew Jeffery, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang, Jae Hyun Yoo

From: Johnny Huang <johnny_huang@aspeedtech.com>

Add FWSPIDQ2 (AE12) and FWSPIDQ3 (AF12) function-group to support
AST2600 FW SPI quad mode. These pins can be used with dedicated FW
SPI pins - FWSPICS0# (AB14), FWSPICK (AF13), FWSPIMOSI (AC14)
and FWSPIMISO (AB13).

Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
---
 drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
index 54064714d73f..80838dc54b3a 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
@@ -1236,12 +1236,17 @@ FUNC_GROUP_DECL(SALT8, AA12);
 FUNC_GROUP_DECL(WDTRST4, AA12);
 
 #define AE12 196
+SIG_EXPR_LIST_DECL_SESG(AE12, FWSPIQ2, FWQSPI, SIG_DESC_SET(SCU438, 4));
 SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
-PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
+PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIQ2),
+	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
 
 #define AF12 197
+SIG_EXPR_LIST_DECL_SESG(AF12, FWSPIQ3, FWQSPI, SIG_DESC_SET(SCU438, 5));
 SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
-PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
+PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIQ3),
+	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
+FUNC_GROUP_DECL(FWQSPI, AE12, AF12);
 
 #define AC12 198
 SSSF_PIN_DECL(AC12, GPIOY6, FWSPIABR, SIG_DESC_SET(SCU438, 6));
@@ -1911,6 +1916,7 @@ static const struct aspeed_pin_group aspeed_g6_groups[] = {
 	ASPEED_PINCTRL_GROUP(FSI2),
 	ASPEED_PINCTRL_GROUP(FWSPIABR),
 	ASPEED_PINCTRL_GROUP(FWSPID),
+	ASPEED_PINCTRL_GROUP(FWQSPI),
 	ASPEED_PINCTRL_GROUP(FWSPIWP),
 	ASPEED_PINCTRL_GROUP(GPIT0),
 	ASPEED_PINCTRL_GROUP(GPIT1),
@@ -2152,6 +2158,7 @@ static const struct aspeed_pin_function aspeed_g6_functions[] = {
 	ASPEED_PINCTRL_FUNC(FSI2),
 	ASPEED_PINCTRL_FUNC(FWSPIABR),
 	ASPEED_PINCTRL_FUNC(FWSPID),
+	ASPEED_PINCTRL_FUNC(FWQSPI),
 	ASPEED_PINCTRL_FUNC(FWSPIWP),
 	ASPEED_PINCTRL_FUNC(GPIT0),
 	ASPEED_PINCTRL_FUNC(GPIT1),
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
@ 2022-03-08  0:37   ` Jae Hyun Yoo
  0 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-08  0:37 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Andrew Jeffery, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang, Jae Hyun Yoo

From: Johnny Huang <johnny_huang@aspeedtech.com>

Add FWSPIDQ2 (AE12) and FWSPIDQ3 (AF12) function-group to support
AST2600 FW SPI quad mode. These pins can be used with dedicated FW
SPI pins - FWSPICS0# (AB14), FWSPICK (AF13), FWSPIMOSI (AC14)
and FWSPIMISO (AB13).

Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
---
 drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
index 54064714d73f..80838dc54b3a 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
@@ -1236,12 +1236,17 @@ FUNC_GROUP_DECL(SALT8, AA12);
 FUNC_GROUP_DECL(WDTRST4, AA12);
 
 #define AE12 196
+SIG_EXPR_LIST_DECL_SESG(AE12, FWSPIQ2, FWQSPI, SIG_DESC_SET(SCU438, 4));
 SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
-PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
+PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIQ2),
+	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
 
 #define AF12 197
+SIG_EXPR_LIST_DECL_SESG(AF12, FWSPIQ3, FWQSPI, SIG_DESC_SET(SCU438, 5));
 SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
-PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
+PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIQ3),
+	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
+FUNC_GROUP_DECL(FWQSPI, AE12, AF12);
 
 #define AC12 198
 SSSF_PIN_DECL(AC12, GPIOY6, FWSPIABR, SIG_DESC_SET(SCU438, 6));
@@ -1911,6 +1916,7 @@ static const struct aspeed_pin_group aspeed_g6_groups[] = {
 	ASPEED_PINCTRL_GROUP(FSI2),
 	ASPEED_PINCTRL_GROUP(FWSPIABR),
 	ASPEED_PINCTRL_GROUP(FWSPID),
+	ASPEED_PINCTRL_GROUP(FWQSPI),
 	ASPEED_PINCTRL_GROUP(FWSPIWP),
 	ASPEED_PINCTRL_GROUP(GPIT0),
 	ASPEED_PINCTRL_GROUP(GPIT1),
@@ -2152,6 +2158,7 @@ static const struct aspeed_pin_function aspeed_g6_functions[] = {
 	ASPEED_PINCTRL_FUNC(FSI2),
 	ASPEED_PINCTRL_FUNC(FWSPIABR),
 	ASPEED_PINCTRL_FUNC(FWSPID),
+	ASPEED_PINCTRL_FUNC(FWQSPI),
 	ASPEED_PINCTRL_FUNC(FWSPIWP),
 	ASPEED_PINCTRL_FUNC(GPIT0),
 	ASPEED_PINCTRL_FUNC(GPIT1),
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 4/5] ARM: dts: aspeed-g6: add FWQSPI group in pinctrl dtsi
  2022-03-08  0:37 ` Jae Hyun Yoo
@ 2022-03-08  0:37   ` Jae Hyun Yoo
  -1 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-08  0:37 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Andrew Jeffery, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang, Jae Hyun Yoo

From: Johnny Huang <johnny_huang@aspeedtech.com>

Add FWSPIDQ2 and FWSPIDQ3 group to support AST2600 FW SPI quad mode.
These pins can be used with dedicated FW SPI pins - FWSPICS0#,
FWSPICK, FWSPIMOSI and FWSPIMISO.

Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
---
 arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
index 06d60a8540e9..47c3fb137cbc 100644
--- a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
@@ -117,6 +117,11 @@ pinctrl_fwspid_default: fwspid_default {
 		groups = "FWSPID";
 	};
 
+	pinctrl_fwqspi_default: fwqspi_default {
+		function = "FWQSPI";
+		groups = "FWQSPI";
+	};
+
 	pinctrl_fwspiwp_default: fwspiwp_default {
 		function = "FWSPIWP";
 		groups = "FWSPIWP";
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 4/5] ARM: dts: aspeed-g6: add FWQSPI group in pinctrl dtsi
@ 2022-03-08  0:37   ` Jae Hyun Yoo
  0 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-08  0:37 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Andrew Jeffery, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang, Jae Hyun Yoo

From: Johnny Huang <johnny_huang@aspeedtech.com>

Add FWSPIDQ2 and FWSPIDQ3 group to support AST2600 FW SPI quad mode.
These pins can be used with dedicated FW SPI pins - FWSPICS0#,
FWSPICK, FWSPIMOSI and FWSPIMISO.

Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
---
 arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
index 06d60a8540e9..47c3fb137cbc 100644
--- a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
@@ -117,6 +117,11 @@ pinctrl_fwspid_default: fwspid_default {
 		groups = "FWSPID";
 	};
 
+	pinctrl_fwqspi_default: fwqspi_default {
+		function = "FWQSPI";
+		groups = "FWQSPI";
+	};
+
 	pinctrl_fwspiwp_default: fwspiwp_default {
 		function = "FWSPIWP";
 		groups = "FWSPIWP";
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 5/5] ARM: dts: aspeed-g6: fix SPI1/SPI2 quad pin group
  2022-03-08  0:37 ` Jae Hyun Yoo
@ 2022-03-08  0:37   ` Jae Hyun Yoo
  -1 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-08  0:37 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Andrew Jeffery, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Jae Hyun Yoo

Fix incorrect function mappings in pinctrl_qspi1_default and
pinctrl_qspi2_default since there function should be SPI1 and
SPI2 respectively.

Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
Fixes: f510f04c8c83 ("ARM: dts: aspeed: Add AST2600 pinmux nodes")
---
 arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
index 47c3fb137cbc..7cd4f075e325 100644
--- a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
@@ -653,12 +653,12 @@ pinctrl_pwm9g1_default: pwm9g1_default {
 	};
 
 	pinctrl_qspi1_default: qspi1_default {
-		function = "QSPI1";
+		function = "SPI1";
 		groups = "QSPI1";
 	};
 
 	pinctrl_qspi2_default: qspi2_default {
-		function = "QSPI2";
+		function = "SPI2";
 		groups = "QSPI2";
 	};
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH 5/5] ARM: dts: aspeed-g6: fix SPI1/SPI2 quad pin group
@ 2022-03-08  0:37   ` Jae Hyun Yoo
  0 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-08  0:37 UTC (permalink / raw)
  To: Joel Stanley, Rob Herring, Andrew Jeffery, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Jae Hyun Yoo

Fix incorrect function mappings in pinctrl_qspi1_default and
pinctrl_qspi2_default since there function should be SPI1 and
SPI2 respectively.

Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
Fixes: f510f04c8c83 ("ARM: dts: aspeed: Add AST2600 pinmux nodes")
---
 arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
index 47c3fb137cbc..7cd4f075e325 100644
--- a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
@@ -653,12 +653,12 @@ pinctrl_pwm9g1_default: pwm9g1_default {
 	};
 
 	pinctrl_qspi1_default: qspi1_default {
-		function = "QSPI1";
+		function = "SPI1";
 		groups = "QSPI1";
 	};
 
 	pinctrl_qspi2_default: qspi2_default {
-		function = "QSPI2";
+		function = "SPI2";
 		groups = "QSPI2";
 	};
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
  2022-03-08  0:37   ` Jae Hyun Yoo
@ 2022-03-16  3:33     ` Andrew Jeffery
  -1 siblings, 0 replies; 28+ messages in thread
From: Andrew Jeffery @ 2022-03-16  3:33 UTC (permalink / raw)
  To: Jae Hyun Yoo, Joel Stanley, Rob Herring, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang



On Tue, 8 Mar 2022, at 11:07, Jae Hyun Yoo wrote:
> From: Johnny Huang <johnny_huang@aspeedtech.com>
>
> Add FWSPIDQ2 (AE12) and FWSPIDQ3 (AF12) function-group to support
> AST2600 FW SPI quad mode. These pins can be used with dedicated FW
> SPI pins - FWSPICS0# (AB14), FWSPICK (AF13), FWSPIMOSI (AC14)
> and FWSPIMISO (AB13).
>
> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
> ---
>  drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c 
> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> index 54064714d73f..80838dc54b3a 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> @@ -1236,12 +1236,17 @@ FUNC_GROUP_DECL(SALT8, AA12);
>  FUNC_GROUP_DECL(WDTRST4, AA12);
> 
>  #define AE12 196
> +SIG_EXPR_LIST_DECL_SESG(AE12, FWSPIQ2, FWQSPI, SIG_DESC_SET(SCU438, 4));
>  SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
> -PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
> +PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIQ2),
> +	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
> 
>  #define AF12 197
> +SIG_EXPR_LIST_DECL_SESG(AF12, FWSPIQ3, FWQSPI, SIG_DESC_SET(SCU438, 5));
>  SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
> -PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
> +PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIQ3),
> +	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
> +FUNC_GROUP_DECL(FWQSPI, AE12, AF12);

The idea behind the quad group was not to define a function for it 
specifically, but to re-use the FWSPID function and select the specific 
group associated with the specific style of SPI bus you desire. This 
way you'd have a pinctrl node like:

pinctrl_fwqspi_default = {
    function = "FWSPID";
    group = "FWQSPI";
};

(note the lack of 'Q' in the function name)

Maybe that's an abuse of groups, but I don't see a need for the 
function name to match the group name here, we're still doing SPI.

This can be seen in the DTS fix that Joel sent (disregarding the mixed 
voltage pins problem).

Thoughts?

Andrew

> 
>  #define AC12 198
>  SSSF_PIN_DECL(AC12, GPIOY6, FWSPIABR, SIG_DESC_SET(SCU438, 6));
> @@ -1911,6 +1916,7 @@ static const struct aspeed_pin_group 
> aspeed_g6_groups[] = {
>  	ASPEED_PINCTRL_GROUP(FSI2),
>  	ASPEED_PINCTRL_GROUP(FWSPIABR),
>  	ASPEED_PINCTRL_GROUP(FWSPID),
> +	ASPEED_PINCTRL_GROUP(FWQSPI),
>  	ASPEED_PINCTRL_GROUP(FWSPIWP),
>  	ASPEED_PINCTRL_GROUP(GPIT0),
>  	ASPEED_PINCTRL_GROUP(GPIT1),
> @@ -2152,6 +2158,7 @@ static const struct aspeed_pin_function 
> aspeed_g6_functions[] = {
>  	ASPEED_PINCTRL_FUNC(FSI2),
>  	ASPEED_PINCTRL_FUNC(FWSPIABR),
>  	ASPEED_PINCTRL_FUNC(FWSPID),
> +	ASPEED_PINCTRL_FUNC(FWQSPI),
>  	ASPEED_PINCTRL_FUNC(FWSPIWP),
>  	ASPEED_PINCTRL_FUNC(GPIT0),
>  	ASPEED_PINCTRL_FUNC(GPIT1),
> -- 
> 2.25.1

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
@ 2022-03-16  3:33     ` Andrew Jeffery
  0 siblings, 0 replies; 28+ messages in thread
From: Andrew Jeffery @ 2022-03-16  3:33 UTC (permalink / raw)
  To: Jae Hyun Yoo, Joel Stanley, Rob Herring, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang



On Tue, 8 Mar 2022, at 11:07, Jae Hyun Yoo wrote:
> From: Johnny Huang <johnny_huang@aspeedtech.com>
>
> Add FWSPIDQ2 (AE12) and FWSPIDQ3 (AF12) function-group to support
> AST2600 FW SPI quad mode. These pins can be used with dedicated FW
> SPI pins - FWSPICS0# (AB14), FWSPICK (AF13), FWSPIMOSI (AC14)
> and FWSPIMISO (AB13).
>
> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
> ---
>  drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c 
> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> index 54064714d73f..80838dc54b3a 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> @@ -1236,12 +1236,17 @@ FUNC_GROUP_DECL(SALT8, AA12);
>  FUNC_GROUP_DECL(WDTRST4, AA12);
> 
>  #define AE12 196
> +SIG_EXPR_LIST_DECL_SESG(AE12, FWSPIQ2, FWQSPI, SIG_DESC_SET(SCU438, 4));
>  SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
> -PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
> +PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIQ2),
> +	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
> 
>  #define AF12 197
> +SIG_EXPR_LIST_DECL_SESG(AF12, FWSPIQ3, FWQSPI, SIG_DESC_SET(SCU438, 5));
>  SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
> -PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
> +PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIQ3),
> +	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
> +FUNC_GROUP_DECL(FWQSPI, AE12, AF12);

The idea behind the quad group was not to define a function for it 
specifically, but to re-use the FWSPID function and select the specific 
group associated with the specific style of SPI bus you desire. This 
way you'd have a pinctrl node like:

pinctrl_fwqspi_default = {
    function = "FWSPID";
    group = "FWQSPI";
};

(note the lack of 'Q' in the function name)

Maybe that's an abuse of groups, but I don't see a need for the 
function name to match the group name here, we're still doing SPI.

This can be seen in the DTS fix that Joel sent (disregarding the mixed 
voltage pins problem).

Thoughts?

Andrew

> 
>  #define AC12 198
>  SSSF_PIN_DECL(AC12, GPIOY6, FWSPIABR, SIG_DESC_SET(SCU438, 6));
> @@ -1911,6 +1916,7 @@ static const struct aspeed_pin_group 
> aspeed_g6_groups[] = {
>  	ASPEED_PINCTRL_GROUP(FSI2),
>  	ASPEED_PINCTRL_GROUP(FWSPIABR),
>  	ASPEED_PINCTRL_GROUP(FWSPID),
> +	ASPEED_PINCTRL_GROUP(FWQSPI),
>  	ASPEED_PINCTRL_GROUP(FWSPIWP),
>  	ASPEED_PINCTRL_GROUP(GPIT0),
>  	ASPEED_PINCTRL_GROUP(GPIT1),
> @@ -2152,6 +2158,7 @@ static const struct aspeed_pin_function 
> aspeed_g6_functions[] = {
>  	ASPEED_PINCTRL_FUNC(FSI2),
>  	ASPEED_PINCTRL_FUNC(FWSPIABR),
>  	ASPEED_PINCTRL_FUNC(FWSPID),
> +	ASPEED_PINCTRL_FUNC(FWQSPI),
>  	ASPEED_PINCTRL_FUNC(FWSPIWP),
>  	ASPEED_PINCTRL_FUNC(GPIT0),
>  	ASPEED_PINCTRL_FUNC(GPIT1),
> -- 
> 2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
  2022-03-16  3:33     ` Andrew Jeffery
@ 2022-03-16  5:05       ` Jae Hyun Yoo
  -1 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-16  5:05 UTC (permalink / raw)
  To: Andrew Jeffery, Joel Stanley, Rob Herring, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang

Hi Andrew,

On 3/15/2022 8:33 PM, Andrew Jeffery wrote:
> 
> 
> On Tue, 8 Mar 2022, at 11:07, Jae Hyun Yoo wrote:
>> From: Johnny Huang <johnny_huang@aspeedtech.com>
>>
>> Add FWSPIDQ2 (AE12) and FWSPIDQ3 (AF12) function-group to support
>> AST2600 FW SPI quad mode. These pins can be used with dedicated FW
>> SPI pins - FWSPICS0# (AB14), FWSPICK (AF13), FWSPIMOSI (AC14)
>> and FWSPIMISO (AB13).
>>
>> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
>> ---
>>   drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>> index 54064714d73f..80838dc54b3a 100644
>> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>> @@ -1236,12 +1236,17 @@ FUNC_GROUP_DECL(SALT8, AA12);
>>   FUNC_GROUP_DECL(WDTRST4, AA12);
>>
>>   #define AE12 196
>> +SIG_EXPR_LIST_DECL_SESG(AE12, FWSPIQ2, FWQSPI, SIG_DESC_SET(SCU438, 4));
>>   SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
>> -PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>> +PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIQ2),
>> +	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>
>>   #define AF12 197
>> +SIG_EXPR_LIST_DECL_SESG(AF12, FWSPIQ3, FWQSPI, SIG_DESC_SET(SCU438, 5));
>>   SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
>> -PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>> +PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIQ3),
>> +	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>> +FUNC_GROUP_DECL(FWQSPI, AE12, AF12);
> 
> The idea behind the quad group was not to define a function for it
> specifically, but to re-use the FWSPID function and select the specific
> group associated with the specific style of SPI bus you desire. This
> way you'd have a pinctrl node like:
> 
> pinctrl_fwqspi_default = {
>      function = "FWSPID";
>      group = "FWQSPI";
> };
> 
> (note the lack of 'Q' in the function name)
> 
> Maybe that's an abuse of groups, but I don't see a need for the
> function name to match the group name here, we're still doing SPI.
> 
> This can be seen in the DTS fix that Joel sent (disregarding the mixed
> voltage pins problem).
> 
> Thoughts?

As you said, FWSPID function in existing code is defined as two groups.

GROUP_DECL(FWSPID, Y1, Y2, Y3, Y4);
GROUP_DECL(FWQSPID, Y1, Y2, Y3, Y4, AE12, AF12);

In case of the FWSPID group, it defines Y1, Y2, Y3 and Y4 pin pads as
FWSPI18 pins which can be multiplexed with eMMC so pinctrl driver sets
SCU500[3] when we select this group. Also, if we select FWQSPID group,
it additionally set AE12 and AF12 pin pads as FWSPIDQ2 and FWSPIDQ3 but
these two pins are actually part of FWSPI function group that are
exposed as dedicated pins on AST2600 SoC.

Joel's patch can fix below pin mux setting error since there was a bug
in aspeed-g6-pinctrl.dtsi.

[    0.742963] aspeed-g6-pinctrl 1e6e2000.syscon:pinctrl: invalid 
function FWQSPID in map table

But it doesn't fix an improper group selection in pinctrl-aspeed-g6
driver.

As we saw above, SCU500[3] bit will be set even when we select FWQSPID
group, and it's described in datasheets like below.

SCU500[3]
   Boot from debug SPI (OTPSTRAP[2])
   0: Disable (default)
   1: Enable
   Enable this bit will set CPU to boot from SPI that is attached on pins
   FWSPI18*. This strap will not work when secure boot or boot from Uart5
   is enabled. This bit is for verification and testing only. Please
   don’t enable the OTPSTRAP[2] and protect it by setting OTPCFG30[2]=1
   and OTPCFG28[2]=1 if there are security concerns.

So if we set this bit once, BMC boot path will be immediately switched
to FWSPI18 pins when we don't enable secure boot, and it breaks BMC
booting. I observed this issue in my board and AST2600 EVB too.

It's not just interface voltage level issue but also boot failure issue
if a board uses dedicated FWSPI pins (AB14, AF13, AC14, AB13).

To fix the issue, this commit removes FWQSPID group from FWSPID
function, and adds FWQSPI function and group to enable just AE12 and
AF12 as FWSPIDQ2 and FWSPIDQ3 to use them with FWSPICS#, FWSPICK,
FWSPIMOSI and FWSPIMISO pins.

Thanks,

-Jae

> Andrew
> 
>>
>>   #define AC12 198
>>   SSSF_PIN_DECL(AC12, GPIOY6, FWSPIABR, SIG_DESC_SET(SCU438, 6));
>> @@ -1911,6 +1916,7 @@ static const struct aspeed_pin_group
>> aspeed_g6_groups[] = {
>>   	ASPEED_PINCTRL_GROUP(FSI2),
>>   	ASPEED_PINCTRL_GROUP(FWSPIABR),
>>   	ASPEED_PINCTRL_GROUP(FWSPID),
>> +	ASPEED_PINCTRL_GROUP(FWQSPI),
>>   	ASPEED_PINCTRL_GROUP(FWSPIWP),
>>   	ASPEED_PINCTRL_GROUP(GPIT0),
>>   	ASPEED_PINCTRL_GROUP(GPIT1),
>> @@ -2152,6 +2158,7 @@ static const struct aspeed_pin_function
>> aspeed_g6_functions[] = {
>>   	ASPEED_PINCTRL_FUNC(FSI2),
>>   	ASPEED_PINCTRL_FUNC(FWSPIABR),
>>   	ASPEED_PINCTRL_FUNC(FWSPID),
>> +	ASPEED_PINCTRL_FUNC(FWQSPI),
>>   	ASPEED_PINCTRL_FUNC(FWSPIWP),
>>   	ASPEED_PINCTRL_FUNC(GPIT0),
>>   	ASPEED_PINCTRL_FUNC(GPIT1),
>> -- 
>> 2.25.1

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
@ 2022-03-16  5:05       ` Jae Hyun Yoo
  0 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-16  5:05 UTC (permalink / raw)
  To: Andrew Jeffery, Joel Stanley, Rob Herring, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang

Hi Andrew,

On 3/15/2022 8:33 PM, Andrew Jeffery wrote:
> 
> 
> On Tue, 8 Mar 2022, at 11:07, Jae Hyun Yoo wrote:
>> From: Johnny Huang <johnny_huang@aspeedtech.com>
>>
>> Add FWSPIDQ2 (AE12) and FWSPIDQ3 (AF12) function-group to support
>> AST2600 FW SPI quad mode. These pins can be used with dedicated FW
>> SPI pins - FWSPICS0# (AB14), FWSPICK (AF13), FWSPIMOSI (AC14)
>> and FWSPIMISO (AB13).
>>
>> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
>> ---
>>   drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>> index 54064714d73f..80838dc54b3a 100644
>> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>> @@ -1236,12 +1236,17 @@ FUNC_GROUP_DECL(SALT8, AA12);
>>   FUNC_GROUP_DECL(WDTRST4, AA12);
>>
>>   #define AE12 196
>> +SIG_EXPR_LIST_DECL_SESG(AE12, FWSPIQ2, FWQSPI, SIG_DESC_SET(SCU438, 4));
>>   SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
>> -PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>> +PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIQ2),
>> +	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>
>>   #define AF12 197
>> +SIG_EXPR_LIST_DECL_SESG(AF12, FWSPIQ3, FWQSPI, SIG_DESC_SET(SCU438, 5));
>>   SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
>> -PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>> +PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIQ3),
>> +	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>> +FUNC_GROUP_DECL(FWQSPI, AE12, AF12);
> 
> The idea behind the quad group was not to define a function for it
> specifically, but to re-use the FWSPID function and select the specific
> group associated with the specific style of SPI bus you desire. This
> way you'd have a pinctrl node like:
> 
> pinctrl_fwqspi_default = {
>      function = "FWSPID";
>      group = "FWQSPI";
> };
> 
> (note the lack of 'Q' in the function name)
> 
> Maybe that's an abuse of groups, but I don't see a need for the
> function name to match the group name here, we're still doing SPI.
> 
> This can be seen in the DTS fix that Joel sent (disregarding the mixed
> voltage pins problem).
> 
> Thoughts?

As you said, FWSPID function in existing code is defined as two groups.

GROUP_DECL(FWSPID, Y1, Y2, Y3, Y4);
GROUP_DECL(FWQSPID, Y1, Y2, Y3, Y4, AE12, AF12);

In case of the FWSPID group, it defines Y1, Y2, Y3 and Y4 pin pads as
FWSPI18 pins which can be multiplexed with eMMC so pinctrl driver sets
SCU500[3] when we select this group. Also, if we select FWQSPID group,
it additionally set AE12 and AF12 pin pads as FWSPIDQ2 and FWSPIDQ3 but
these two pins are actually part of FWSPI function group that are
exposed as dedicated pins on AST2600 SoC.

Joel's patch can fix below pin mux setting error since there was a bug
in aspeed-g6-pinctrl.dtsi.

[    0.742963] aspeed-g6-pinctrl 1e6e2000.syscon:pinctrl: invalid 
function FWQSPID in map table

But it doesn't fix an improper group selection in pinctrl-aspeed-g6
driver.

As we saw above, SCU500[3] bit will be set even when we select FWQSPID
group, and it's described in datasheets like below.

SCU500[3]
   Boot from debug SPI (OTPSTRAP[2])
   0: Disable (default)
   1: Enable
   Enable this bit will set CPU to boot from SPI that is attached on pins
   FWSPI18*. This strap will not work when secure boot or boot from Uart5
   is enabled. This bit is for verification and testing only. Please
   don’t enable the OTPSTRAP[2] and protect it by setting OTPCFG30[2]=1
   and OTPCFG28[2]=1 if there are security concerns.

So if we set this bit once, BMC boot path will be immediately switched
to FWSPI18 pins when we don't enable secure boot, and it breaks BMC
booting. I observed this issue in my board and AST2600 EVB too.

It's not just interface voltage level issue but also boot failure issue
if a board uses dedicated FWSPI pins (AB14, AF13, AC14, AB13).

To fix the issue, this commit removes FWQSPID group from FWSPID
function, and adds FWQSPI function and group to enable just AE12 and
AF12 as FWSPIDQ2 and FWSPIDQ3 to use them with FWSPICS#, FWSPICK,
FWSPIMOSI and FWSPIMISO pins.

Thanks,

-Jae

> Andrew
> 
>>
>>   #define AC12 198
>>   SSSF_PIN_DECL(AC12, GPIOY6, FWSPIABR, SIG_DESC_SET(SCU438, 6));
>> @@ -1911,6 +1916,7 @@ static const struct aspeed_pin_group
>> aspeed_g6_groups[] = {
>>   	ASPEED_PINCTRL_GROUP(FSI2),
>>   	ASPEED_PINCTRL_GROUP(FWSPIABR),
>>   	ASPEED_PINCTRL_GROUP(FWSPID),
>> +	ASPEED_PINCTRL_GROUP(FWQSPI),
>>   	ASPEED_PINCTRL_GROUP(FWSPIWP),
>>   	ASPEED_PINCTRL_GROUP(GPIT0),
>>   	ASPEED_PINCTRL_GROUP(GPIT1),
>> @@ -2152,6 +2158,7 @@ static const struct aspeed_pin_function
>> aspeed_g6_functions[] = {
>>   	ASPEED_PINCTRL_FUNC(FSI2),
>>   	ASPEED_PINCTRL_FUNC(FWSPIABR),
>>   	ASPEED_PINCTRL_FUNC(FWSPID),
>> +	ASPEED_PINCTRL_FUNC(FWQSPI),
>>   	ASPEED_PINCTRL_FUNC(FWSPIWP),
>>   	ASPEED_PINCTRL_FUNC(GPIT0),
>>   	ASPEED_PINCTRL_FUNC(GPIT1),
>> -- 
>> 2.25.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
  2022-03-16  5:05       ` Jae Hyun Yoo
@ 2022-03-16  5:45         ` Andrew Jeffery
  -1 siblings, 0 replies; 28+ messages in thread
From: Andrew Jeffery @ 2022-03-16  5:45 UTC (permalink / raw)
  To: Jae Hyun Yoo, Joel Stanley, Rob Herring, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang



On Wed, 16 Mar 2022, at 15:35, Jae Hyun Yoo wrote:
> Hi Andrew,
>
> On 3/15/2022 8:33 PM, Andrew Jeffery wrote:
>> 
>> 
>> On Tue, 8 Mar 2022, at 11:07, Jae Hyun Yoo wrote:
>>> From: Johnny Huang <johnny_huang@aspeedtech.com>
>>>
>>> Add FWSPIDQ2 (AE12) and FWSPIDQ3 (AF12) function-group to support
>>> AST2600 FW SPI quad mode. These pins can be used with dedicated FW
>>> SPI pins - FWSPICS0# (AB14), FWSPICK (AF13), FWSPIMOSI (AC14)
>>> and FWSPIMISO (AB13).
>>>
>>> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
>>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
>>> ---
>>>   drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 11 +++++++++--
>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>> index 54064714d73f..80838dc54b3a 100644
>>> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>> @@ -1236,12 +1236,17 @@ FUNC_GROUP_DECL(SALT8, AA12);
>>>   FUNC_GROUP_DECL(WDTRST4, AA12);
>>>
>>>   #define AE12 196
>>> +SIG_EXPR_LIST_DECL_SESG(AE12, FWSPIQ2, FWQSPI, SIG_DESC_SET(SCU438, 4));
>>>   SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
>>> -PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>> +PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIQ2),
>>> +	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>>
>>>   #define AF12 197
>>> +SIG_EXPR_LIST_DECL_SESG(AF12, FWSPIQ3, FWQSPI, SIG_DESC_SET(SCU438, 5));
>>>   SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
>>> -PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>>> +PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIQ3),
>>> +	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>>> +FUNC_GROUP_DECL(FWQSPI, AE12, AF12);
>> 
>> The idea behind the quad group was not to define a function for it
>> specifically, but to re-use the FWSPID function and select the specific
>> group associated with the specific style of SPI bus you desire. This
>> way you'd have a pinctrl node like:
>> 
>> pinctrl_fwqspi_default = {
>>      function = "FWSPID";
>>      group = "FWQSPI";
>> };
>> 
>> (note the lack of 'Q' in the function name)
>> 
>> Maybe that's an abuse of groups, but I don't see a need for the
>> function name to match the group name here, we're still doing SPI.
>> 
>> This can be seen in the DTS fix that Joel sent (disregarding the mixed
>> voltage pins problem).
>> 
>> Thoughts?
>
> As you said, FWSPID function in existing code is defined as two groups.
>
> GROUP_DECL(FWSPID, Y1, Y2, Y3, Y4);
> GROUP_DECL(FWQSPID, Y1, Y2, Y3, Y4, AE12, AF12);
>
> In case of the FWSPID group, it defines Y1, Y2, Y3 and Y4 pin pads as
> FWSPI18 pins which can be multiplexed with eMMC so pinctrl driver sets
> SCU500[3] when we select this group. Also, if we select FWQSPID group,
> it additionally set AE12 and AF12 pin pads as FWSPIDQ2 and FWSPIDQ3 but
> these two pins are actually part of FWSPI function group that are
> exposed as dedicated pins on AST2600 SoC.
>
> Joel's patch can fix below pin mux setting error since there was a bug
> in aspeed-g6-pinctrl.dtsi.
>
> [    0.742963] aspeed-g6-pinctrl 1e6e2000.syscon:pinctrl: invalid 
> function FWQSPID in map table
>
> But it doesn't fix an improper group selection in pinctrl-aspeed-g6
> driver.
>
> As we saw above, SCU500[3] bit will be set even when we select FWQSPID
> group, and it's described in datasheets like below.
>
> SCU500[3]
>    Boot from debug SPI (OTPSTRAP[2])
>    0: Disable (default)
>    1: Enable
>    Enable this bit will set CPU to boot from SPI that is attached on pins
>    FWSPI18*. This strap will not work when secure boot or boot from Uart5
>    is enabled. This bit is for verification and testing only. Please
>    don’t enable the OTPSTRAP[2] and protect it by setting OTPCFG30[2]=1
>    and OTPCFG28[2]=1 if there are security concerns.
>
> So if we set this bit once, BMC boot path will be immediately switched
> to FWSPI18 pins when we don't enable secure boot, and it breaks BMC
> booting. I observed this issue in my board and AST2600 EVB too.

Yep, this needs to be fixed.
>
> It's not just interface voltage level issue but also boot failure issue
> if a board uses dedicated FWSPI pins (AB14, AF13, AC14, AB13).

Okay, I wasn't across that part :)

>
> To fix the issue, this commit removes FWQSPID group from FWSPID
> function, and adds FWQSPI function and group to enable just AE12 and
> AF12 as FWSPIDQ2 and FWSPIDQ3 to use them with FWSPICS#, FWSPICK,
> FWSPIMOSI and FWSPIMISO pins.

Okay, probably wrote what I meant in a confusing way. I understand what 
you've described, but what I was trying to suggest was instead of 
creating a "FWQSPI" function and group was to instead have just the 
"FWSPI" function to be used with both the "FWSPI" and "FWQSPI" groups. 
This aligns with how the FWSPID function/groups work.

Andrew

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
@ 2022-03-16  5:45         ` Andrew Jeffery
  0 siblings, 0 replies; 28+ messages in thread
From: Andrew Jeffery @ 2022-03-16  5:45 UTC (permalink / raw)
  To: Jae Hyun Yoo, Joel Stanley, Rob Herring, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang



On Wed, 16 Mar 2022, at 15:35, Jae Hyun Yoo wrote:
> Hi Andrew,
>
> On 3/15/2022 8:33 PM, Andrew Jeffery wrote:
>> 
>> 
>> On Tue, 8 Mar 2022, at 11:07, Jae Hyun Yoo wrote:
>>> From: Johnny Huang <johnny_huang@aspeedtech.com>
>>>
>>> Add FWSPIDQ2 (AE12) and FWSPIDQ3 (AF12) function-group to support
>>> AST2600 FW SPI quad mode. These pins can be used with dedicated FW
>>> SPI pins - FWSPICS0# (AB14), FWSPICK (AF13), FWSPIMOSI (AC14)
>>> and FWSPIMISO (AB13).
>>>
>>> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
>>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
>>> ---
>>>   drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 11 +++++++++--
>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>> index 54064714d73f..80838dc54b3a 100644
>>> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>> @@ -1236,12 +1236,17 @@ FUNC_GROUP_DECL(SALT8, AA12);
>>>   FUNC_GROUP_DECL(WDTRST4, AA12);
>>>
>>>   #define AE12 196
>>> +SIG_EXPR_LIST_DECL_SESG(AE12, FWSPIQ2, FWQSPI, SIG_DESC_SET(SCU438, 4));
>>>   SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
>>> -PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>> +PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIQ2),
>>> +	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>>
>>>   #define AF12 197
>>> +SIG_EXPR_LIST_DECL_SESG(AF12, FWSPIQ3, FWQSPI, SIG_DESC_SET(SCU438, 5));
>>>   SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
>>> -PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>>> +PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIQ3),
>>> +	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>>> +FUNC_GROUP_DECL(FWQSPI, AE12, AF12);
>> 
>> The idea behind the quad group was not to define a function for it
>> specifically, but to re-use the FWSPID function and select the specific
>> group associated with the specific style of SPI bus you desire. This
>> way you'd have a pinctrl node like:
>> 
>> pinctrl_fwqspi_default = {
>>      function = "FWSPID";
>>      group = "FWQSPI";
>> };
>> 
>> (note the lack of 'Q' in the function name)
>> 
>> Maybe that's an abuse of groups, but I don't see a need for the
>> function name to match the group name here, we're still doing SPI.
>> 
>> This can be seen in the DTS fix that Joel sent (disregarding the mixed
>> voltage pins problem).
>> 
>> Thoughts?
>
> As you said, FWSPID function in existing code is defined as two groups.
>
> GROUP_DECL(FWSPID, Y1, Y2, Y3, Y4);
> GROUP_DECL(FWQSPID, Y1, Y2, Y3, Y4, AE12, AF12);
>
> In case of the FWSPID group, it defines Y1, Y2, Y3 and Y4 pin pads as
> FWSPI18 pins which can be multiplexed with eMMC so pinctrl driver sets
> SCU500[3] when we select this group. Also, if we select FWQSPID group,
> it additionally set AE12 and AF12 pin pads as FWSPIDQ2 and FWSPIDQ3 but
> these two pins are actually part of FWSPI function group that are
> exposed as dedicated pins on AST2600 SoC.
>
> Joel's patch can fix below pin mux setting error since there was a bug
> in aspeed-g6-pinctrl.dtsi.
>
> [    0.742963] aspeed-g6-pinctrl 1e6e2000.syscon:pinctrl: invalid 
> function FWQSPID in map table
>
> But it doesn't fix an improper group selection in pinctrl-aspeed-g6
> driver.
>
> As we saw above, SCU500[3] bit will be set even when we select FWQSPID
> group, and it's described in datasheets like below.
>
> SCU500[3]
>    Boot from debug SPI (OTPSTRAP[2])
>    0: Disable (default)
>    1: Enable
>    Enable this bit will set CPU to boot from SPI that is attached on pins
>    FWSPI18*. This strap will not work when secure boot or boot from Uart5
>    is enabled. This bit is for verification and testing only. Please
>    don’t enable the OTPSTRAP[2] and protect it by setting OTPCFG30[2]=1
>    and OTPCFG28[2]=1 if there are security concerns.
>
> So if we set this bit once, BMC boot path will be immediately switched
> to FWSPI18 pins when we don't enable secure boot, and it breaks BMC
> booting. I observed this issue in my board and AST2600 EVB too.

Yep, this needs to be fixed.
>
> It's not just interface voltage level issue but also boot failure issue
> if a board uses dedicated FWSPI pins (AB14, AF13, AC14, AB13).

Okay, I wasn't across that part :)

>
> To fix the issue, this commit removes FWQSPID group from FWSPID
> function, and adds FWQSPI function and group to enable just AE12 and
> AF12 as FWSPIDQ2 and FWSPIDQ3 to use them with FWSPICS#, FWSPICK,
> FWSPIMOSI and FWSPIMISO pins.

Okay, probably wrote what I meant in a confusing way. I understand what 
you've described, but what I was trying to suggest was instead of 
creating a "FWQSPI" function and group was to instead have just the 
"FWSPI" function to be used with both the "FWSPI" and "FWQSPI" groups. 
This aligns with how the FWSPID function/groups work.

Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
  2022-03-16  5:45         ` Andrew Jeffery
@ 2022-03-16  6:03           ` Jae Hyun Yoo
  -1 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-16  6:03 UTC (permalink / raw)
  To: Andrew Jeffery, Joel Stanley, Rob Herring, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang

On 3/15/2022 10:45 PM, Andrew Jeffery wrote:
> 
> 
> On Wed, 16 Mar 2022, at 15:35, Jae Hyun Yoo wrote:
>> Hi Andrew,
>>
>> On 3/15/2022 8:33 PM, Andrew Jeffery wrote:
>>>
>>>
>>> On Tue, 8 Mar 2022, at 11:07, Jae Hyun Yoo wrote:
>>>> From: Johnny Huang <johnny_huang@aspeedtech.com>
>>>>
>>>> Add FWSPIDQ2 (AE12) and FWSPIDQ3 (AF12) function-group to support
>>>> AST2600 FW SPI quad mode. These pins can be used with dedicated FW
>>>> SPI pins - FWSPICS0# (AB14), FWSPICK (AF13), FWSPIMOSI (AC14)
>>>> and FWSPIMISO (AB13).
>>>>
>>>> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
>>>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
>>>> ---
>>>>    drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 11 +++++++++--
>>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>> index 54064714d73f..80838dc54b3a 100644
>>>> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>> @@ -1236,12 +1236,17 @@ FUNC_GROUP_DECL(SALT8, AA12);
>>>>    FUNC_GROUP_DECL(WDTRST4, AA12);
>>>>
>>>>    #define AE12 196
>>>> +SIG_EXPR_LIST_DECL_SESG(AE12, FWSPIQ2, FWQSPI, SIG_DESC_SET(SCU438, 4));
>>>>    SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
>>>> -PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>>> +PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIQ2),
>>>> +	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>>>
>>>>    #define AF12 197
>>>> +SIG_EXPR_LIST_DECL_SESG(AF12, FWSPIQ3, FWQSPI, SIG_DESC_SET(SCU438, 5));
>>>>    SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
>>>> -PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>>>> +PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIQ3),
>>>> +	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>>>> +FUNC_GROUP_DECL(FWQSPI, AE12, AF12);
>>>
>>> The idea behind the quad group was not to define a function for it
>>> specifically, but to re-use the FWSPID function and select the specific
>>> group associated with the specific style of SPI bus you desire. This
>>> way you'd have a pinctrl node like:
>>>
>>> pinctrl_fwqspi_default = {
>>>       function = "FWSPID";
>>>       group = "FWQSPI";
>>> };
>>>
>>> (note the lack of 'Q' in the function name)
>>>
>>> Maybe that's an abuse of groups, but I don't see a need for the
>>> function name to match the group name here, we're still doing SPI.
>>>
>>> This can be seen in the DTS fix that Joel sent (disregarding the mixed
>>> voltage pins problem).
>>>
>>> Thoughts?
>>
>> As you said, FWSPID function in existing code is defined as two groups.
>>
>> GROUP_DECL(FWSPID, Y1, Y2, Y3, Y4);
>> GROUP_DECL(FWQSPID, Y1, Y2, Y3, Y4, AE12, AF12);
>>
>> In case of the FWSPID group, it defines Y1, Y2, Y3 and Y4 pin pads as
>> FWSPI18 pins which can be multiplexed with eMMC so pinctrl driver sets
>> SCU500[3] when we select this group. Also, if we select FWQSPID group,
>> it additionally set AE12 and AF12 pin pads as FWSPIDQ2 and FWSPIDQ3 but
>> these two pins are actually part of FWSPI function group that are
>> exposed as dedicated pins on AST2600 SoC.
>>
>> Joel's patch can fix below pin mux setting error since there was a bug
>> in aspeed-g6-pinctrl.dtsi.
>>
>> [    0.742963] aspeed-g6-pinctrl 1e6e2000.syscon:pinctrl: invalid
>> function FWQSPID in map table
>>
>> But it doesn't fix an improper group selection in pinctrl-aspeed-g6
>> driver.
>>
>> As we saw above, SCU500[3] bit will be set even when we select FWQSPID
>> group, and it's described in datasheets like below.
>>
>> SCU500[3]
>>     Boot from debug SPI (OTPSTRAP[2])
>>     0: Disable (default)
>>     1: Enable
>>     Enable this bit will set CPU to boot from SPI that is attached on pins
>>     FWSPI18*. This strap will not work when secure boot or boot from Uart5
>>     is enabled. This bit is for verification and testing only. Please
>>     don’t enable the OTPSTRAP[2] and protect it by setting OTPCFG30[2]=1
>>     and OTPCFG28[2]=1 if there are security concerns.
>>
>> So if we set this bit once, BMC boot path will be immediately switched
>> to FWSPI18 pins when we don't enable secure boot, and it breaks BMC
>> booting. I observed this issue in my board and AST2600 EVB too.
> 
> Yep, this needs to be fixed.
>>
>> It's not just interface voltage level issue but also boot failure issue
>> if a board uses dedicated FWSPI pins (AB14, AF13, AC14, AB13).
> 
> Okay, I wasn't across that part :)
> 
>>
>> To fix the issue, this commit removes FWQSPID group from FWSPID
>> function, and adds FWQSPI function and group to enable just AE12 and
>> AF12 as FWSPIDQ2 and FWSPIDQ3 to use them with FWSPICS#, FWSPICK,
>> FWSPIMOSI and FWSPIMISO pins.
> 
> Okay, probably wrote what I meant in a confusing way. I understand what
> you've described, but what I was trying to suggest was instead of
> creating a "FWQSPI" function and group was to instead have just the
> "FWSPI" function to be used with both the "FWSPI" and "FWQSPI" groups.
> This aligns with how the FWSPID function/groups work.

FWSPI pins are dedicated pins just for boot firmware SPI interface so
these pins don't need any pinmux setting. We just need to add pinmux
setting of AE12 and AF12 for a case when FWSPI needs QSPI support with
having these additional data pins. Do we need to define two groups
including a dummy group for the dedicated pins? Can you please share
more details of your idea? I don't see any example of dedicated pins in
the driver.

- Jae

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
@ 2022-03-16  6:03           ` Jae Hyun Yoo
  0 siblings, 0 replies; 28+ messages in thread
From: Jae Hyun Yoo @ 2022-03-16  6:03 UTC (permalink / raw)
  To: Andrew Jeffery, Joel Stanley, Rob Herring, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang

On 3/15/2022 10:45 PM, Andrew Jeffery wrote:
> 
> 
> On Wed, 16 Mar 2022, at 15:35, Jae Hyun Yoo wrote:
>> Hi Andrew,
>>
>> On 3/15/2022 8:33 PM, Andrew Jeffery wrote:
>>>
>>>
>>> On Tue, 8 Mar 2022, at 11:07, Jae Hyun Yoo wrote:
>>>> From: Johnny Huang <johnny_huang@aspeedtech.com>
>>>>
>>>> Add FWSPIDQ2 (AE12) and FWSPIDQ3 (AF12) function-group to support
>>>> AST2600 FW SPI quad mode. These pins can be used with dedicated FW
>>>> SPI pins - FWSPICS0# (AB14), FWSPICK (AF13), FWSPIMOSI (AC14)
>>>> and FWSPIMISO (AB13).
>>>>
>>>> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
>>>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
>>>> ---
>>>>    drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 11 +++++++++--
>>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>> index 54064714d73f..80838dc54b3a 100644
>>>> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>> @@ -1236,12 +1236,17 @@ FUNC_GROUP_DECL(SALT8, AA12);
>>>>    FUNC_GROUP_DECL(WDTRST4, AA12);
>>>>
>>>>    #define AE12 196
>>>> +SIG_EXPR_LIST_DECL_SESG(AE12, FWSPIQ2, FWQSPI, SIG_DESC_SET(SCU438, 4));
>>>>    SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
>>>> -PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>>> +PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIQ2),
>>>> +	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>>>
>>>>    #define AF12 197
>>>> +SIG_EXPR_LIST_DECL_SESG(AF12, FWSPIQ3, FWQSPI, SIG_DESC_SET(SCU438, 5));
>>>>    SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
>>>> -PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>>>> +PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIQ3),
>>>> +	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>>>> +FUNC_GROUP_DECL(FWQSPI, AE12, AF12);
>>>
>>> The idea behind the quad group was not to define a function for it
>>> specifically, but to re-use the FWSPID function and select the specific
>>> group associated with the specific style of SPI bus you desire. This
>>> way you'd have a pinctrl node like:
>>>
>>> pinctrl_fwqspi_default = {
>>>       function = "FWSPID";
>>>       group = "FWQSPI";
>>> };
>>>
>>> (note the lack of 'Q' in the function name)
>>>
>>> Maybe that's an abuse of groups, but I don't see a need for the
>>> function name to match the group name here, we're still doing SPI.
>>>
>>> This can be seen in the DTS fix that Joel sent (disregarding the mixed
>>> voltage pins problem).
>>>
>>> Thoughts?
>>
>> As you said, FWSPID function in existing code is defined as two groups.
>>
>> GROUP_DECL(FWSPID, Y1, Y2, Y3, Y4);
>> GROUP_DECL(FWQSPID, Y1, Y2, Y3, Y4, AE12, AF12);
>>
>> In case of the FWSPID group, it defines Y1, Y2, Y3 and Y4 pin pads as
>> FWSPI18 pins which can be multiplexed with eMMC so pinctrl driver sets
>> SCU500[3] when we select this group. Also, if we select FWQSPID group,
>> it additionally set AE12 and AF12 pin pads as FWSPIDQ2 and FWSPIDQ3 but
>> these two pins are actually part of FWSPI function group that are
>> exposed as dedicated pins on AST2600 SoC.
>>
>> Joel's patch can fix below pin mux setting error since there was a bug
>> in aspeed-g6-pinctrl.dtsi.
>>
>> [    0.742963] aspeed-g6-pinctrl 1e6e2000.syscon:pinctrl: invalid
>> function FWQSPID in map table
>>
>> But it doesn't fix an improper group selection in pinctrl-aspeed-g6
>> driver.
>>
>> As we saw above, SCU500[3] bit will be set even when we select FWQSPID
>> group, and it's described in datasheets like below.
>>
>> SCU500[3]
>>     Boot from debug SPI (OTPSTRAP[2])
>>     0: Disable (default)
>>     1: Enable
>>     Enable this bit will set CPU to boot from SPI that is attached on pins
>>     FWSPI18*. This strap will not work when secure boot or boot from Uart5
>>     is enabled. This bit is for verification and testing only. Please
>>     don’t enable the OTPSTRAP[2] and protect it by setting OTPCFG30[2]=1
>>     and OTPCFG28[2]=1 if there are security concerns.
>>
>> So if we set this bit once, BMC boot path will be immediately switched
>> to FWSPI18 pins when we don't enable secure boot, and it breaks BMC
>> booting. I observed this issue in my board and AST2600 EVB too.
> 
> Yep, this needs to be fixed.
>>
>> It's not just interface voltage level issue but also boot failure issue
>> if a board uses dedicated FWSPI pins (AB14, AF13, AC14, AB13).
> 
> Okay, I wasn't across that part :)
> 
>>
>> To fix the issue, this commit removes FWQSPID group from FWSPID
>> function, and adds FWQSPI function and group to enable just AE12 and
>> AF12 as FWSPIDQ2 and FWSPIDQ3 to use them with FWSPICS#, FWSPICK,
>> FWSPIMOSI and FWSPIMISO pins.
> 
> Okay, probably wrote what I meant in a confusing way. I understand what
> you've described, but what I was trying to suggest was instead of
> creating a "FWQSPI" function and group was to instead have just the
> "FWSPI" function to be used with both the "FWSPI" and "FWQSPI" groups.
> This aligns with how the FWSPID function/groups work.

FWSPI pins are dedicated pins just for boot firmware SPI interface so
these pins don't need any pinmux setting. We just need to add pinmux
setting of AE12 and AF12 for a case when FWSPI needs QSPI support with
having these additional data pins. Do we need to define two groups
including a dummy group for the dedicated pins? Can you please share
more details of your idea? I don't see any example of dedicated pins in
the driver.

- Jae

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
  2022-03-16  6:03           ` Jae Hyun Yoo
@ 2022-03-16  6:11             ` Andrew Jeffery
  -1 siblings, 0 replies; 28+ messages in thread
From: Andrew Jeffery @ 2022-03-16  6:11 UTC (permalink / raw)
  To: Jae Hyun Yoo, Joel Stanley, Rob Herring, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang



On Wed, 16 Mar 2022, at 16:33, Jae Hyun Yoo wrote:
> On 3/15/2022 10:45 PM, Andrew Jeffery wrote:
>> 
>> 
>> On Wed, 16 Mar 2022, at 15:35, Jae Hyun Yoo wrote:
>>> Hi Andrew,
>>>
>>> On 3/15/2022 8:33 PM, Andrew Jeffery wrote:
>>>>
>>>>
>>>> On Tue, 8 Mar 2022, at 11:07, Jae Hyun Yoo wrote:
>>>>> From: Johnny Huang <johnny_huang@aspeedtech.com>
>>>>>
>>>>> Add FWSPIDQ2 (AE12) and FWSPIDQ3 (AF12) function-group to support
>>>>> AST2600 FW SPI quad mode. These pins can be used with dedicated FW
>>>>> SPI pins - FWSPICS0# (AB14), FWSPICK (AF13), FWSPIMOSI (AC14)
>>>>> and FWSPIMISO (AB13).
>>>>>
>>>>> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
>>>>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
>>>>> ---
>>>>>    drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 11 +++++++++--
>>>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>>> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>>> index 54064714d73f..80838dc54b3a 100644
>>>>> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>>> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>>> @@ -1236,12 +1236,17 @@ FUNC_GROUP_DECL(SALT8, AA12);
>>>>>    FUNC_GROUP_DECL(WDTRST4, AA12);
>>>>>
>>>>>    #define AE12 196
>>>>> +SIG_EXPR_LIST_DECL_SESG(AE12, FWSPIQ2, FWQSPI, SIG_DESC_SET(SCU438, 4));
>>>>>    SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
>>>>> -PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>>>> +PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIQ2),
>>>>> +	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>>>>
>>>>>    #define AF12 197
>>>>> +SIG_EXPR_LIST_DECL_SESG(AF12, FWSPIQ3, FWQSPI, SIG_DESC_SET(SCU438, 5));
>>>>>    SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
>>>>> -PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>>>>> +PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIQ3),
>>>>> +	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>>>>> +FUNC_GROUP_DECL(FWQSPI, AE12, AF12);
>>>>
>>>> The idea behind the quad group was not to define a function for it
>>>> specifically, but to re-use the FWSPID function and select the specific
>>>> group associated with the specific style of SPI bus you desire. This
>>>> way you'd have a pinctrl node like:
>>>>
>>>> pinctrl_fwqspi_default = {
>>>>       function = "FWSPID";
>>>>       group = "FWQSPI";
>>>> };
>>>>
>>>> (note the lack of 'Q' in the function name)
>>>>
>>>> Maybe that's an abuse of groups, but I don't see a need for the
>>>> function name to match the group name here, we're still doing SPI.
>>>>
>>>> This can be seen in the DTS fix that Joel sent (disregarding the mixed
>>>> voltage pins problem).
>>>>
>>>> Thoughts?
>>>
>>> As you said, FWSPID function in existing code is defined as two groups.
>>>
>>> GROUP_DECL(FWSPID, Y1, Y2, Y3, Y4);
>>> GROUP_DECL(FWQSPID, Y1, Y2, Y3, Y4, AE12, AF12);
>>>
>>> In case of the FWSPID group, it defines Y1, Y2, Y3 and Y4 pin pads as
>>> FWSPI18 pins which can be multiplexed with eMMC so pinctrl driver sets
>>> SCU500[3] when we select this group. Also, if we select FWQSPID group,
>>> it additionally set AE12 and AF12 pin pads as FWSPIDQ2 and FWSPIDQ3 but
>>> these two pins are actually part of FWSPI function group that are
>>> exposed as dedicated pins on AST2600 SoC.
>>>
>>> Joel's patch can fix below pin mux setting error since there was a bug
>>> in aspeed-g6-pinctrl.dtsi.
>>>
>>> [    0.742963] aspeed-g6-pinctrl 1e6e2000.syscon:pinctrl: invalid
>>> function FWQSPID in map table
>>>
>>> But it doesn't fix an improper group selection in pinctrl-aspeed-g6
>>> driver.
>>>
>>> As we saw above, SCU500[3] bit will be set even when we select FWQSPID
>>> group, and it's described in datasheets like below.
>>>
>>> SCU500[3]
>>>     Boot from debug SPI (OTPSTRAP[2])
>>>     0: Disable (default)
>>>     1: Enable
>>>     Enable this bit will set CPU to boot from SPI that is attached on pins
>>>     FWSPI18*. This strap will not work when secure boot or boot from Uart5
>>>     is enabled. This bit is for verification and testing only. Please
>>>     don’t enable the OTPSTRAP[2] and protect it by setting OTPCFG30[2]=1
>>>     and OTPCFG28[2]=1 if there are security concerns.
>>>
>>> So if we set this bit once, BMC boot path will be immediately switched
>>> to FWSPI18 pins when we don't enable secure boot, and it breaks BMC
>>> booting. I observed this issue in my board and AST2600 EVB too.
>> 
>> Yep, this needs to be fixed.
>>>
>>> It's not just interface voltage level issue but also boot failure issue
>>> if a board uses dedicated FWSPI pins (AB14, AF13, AC14, AB13).
>> 
>> Okay, I wasn't across that part :)
>> 
>>>
>>> To fix the issue, this commit removes FWQSPID group from FWSPID
>>> function, and adds FWQSPI function and group to enable just AE12 and
>>> AF12 as FWSPIDQ2 and FWSPIDQ3 to use them with FWSPICS#, FWSPICK,
>>> FWSPIMOSI and FWSPIMISO pins.
>> 
>> Okay, probably wrote what I meant in a confusing way. I understand what
>> you've described, but what I was trying to suggest was instead of
>> creating a "FWQSPI" function and group was to instead have just the
>> "FWSPI" function to be used with both the "FWSPI" and "FWQSPI" groups.
>> This aligns with how the FWSPID function/groups work.
>
> FWSPI pins are dedicated pins just for boot firmware SPI interface so
> these pins don't need any pinmux setting. We just need to add pinmux
> setting of AE12 and AF12 for a case when FWSPI needs QSPI support with
> having these additional data pins. Do we need to define two groups
> including a dummy group for the dedicated pins? Can you please share
> more details of your idea? I don't see any example of dedicated pins in
> the driver.

Ah, yeah, if they're fixed pins then we don't need to worry about them 
in pinmux, in which case we only have the one group. I'm getting there 
slowly :)

I'll look over the patch again.

Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group
@ 2022-03-16  6:11             ` Andrew Jeffery
  0 siblings, 0 replies; 28+ messages in thread
From: Andrew Jeffery @ 2022-03-16  6:11 UTC (permalink / raw)
  To: Jae Hyun Yoo, Joel Stanley, Rob Herring, Linus Walleij
  Cc: Jamie Iles, Graeme Gregory, devicetree, linux-arm-kernel,
	linux-aspeed, Johnny Huang



On Wed, 16 Mar 2022, at 16:33, Jae Hyun Yoo wrote:
> On 3/15/2022 10:45 PM, Andrew Jeffery wrote:
>> 
>> 
>> On Wed, 16 Mar 2022, at 15:35, Jae Hyun Yoo wrote:
>>> Hi Andrew,
>>>
>>> On 3/15/2022 8:33 PM, Andrew Jeffery wrote:
>>>>
>>>>
>>>> On Tue, 8 Mar 2022, at 11:07, Jae Hyun Yoo wrote:
>>>>> From: Johnny Huang <johnny_huang@aspeedtech.com>
>>>>>
>>>>> Add FWSPIDQ2 (AE12) and FWSPIDQ3 (AF12) function-group to support
>>>>> AST2600 FW SPI quad mode. These pins can be used with dedicated FW
>>>>> SPI pins - FWSPICS0# (AB14), FWSPICK (AF13), FWSPIMOSI (AC14)
>>>>> and FWSPIMISO (AB13).
>>>>>
>>>>> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
>>>>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
>>>>> ---
>>>>>    drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 11 +++++++++--
>>>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>>> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>>> index 54064714d73f..80838dc54b3a 100644
>>>>> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>>> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>>>>> @@ -1236,12 +1236,17 @@ FUNC_GROUP_DECL(SALT8, AA12);
>>>>>    FUNC_GROUP_DECL(WDTRST4, AA12);
>>>>>
>>>>>    #define AE12 196
>>>>> +SIG_EXPR_LIST_DECL_SESG(AE12, FWSPIQ2, FWQSPI, SIG_DESC_SET(SCU438, 4));
>>>>>    SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
>>>>> -PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>>>> +PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIQ2),
>>>>> +	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
>>>>>
>>>>>    #define AF12 197
>>>>> +SIG_EXPR_LIST_DECL_SESG(AF12, FWSPIQ3, FWQSPI, SIG_DESC_SET(SCU438, 5));
>>>>>    SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
>>>>> -PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>>>>> +PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIQ3),
>>>>> +	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
>>>>> +FUNC_GROUP_DECL(FWQSPI, AE12, AF12);
>>>>
>>>> The idea behind the quad group was not to define a function for it
>>>> specifically, but to re-use the FWSPID function and select the specific
>>>> group associated with the specific style of SPI bus you desire. This
>>>> way you'd have a pinctrl node like:
>>>>
>>>> pinctrl_fwqspi_default = {
>>>>       function = "FWSPID";
>>>>       group = "FWQSPI";
>>>> };
>>>>
>>>> (note the lack of 'Q' in the function name)
>>>>
>>>> Maybe that's an abuse of groups, but I don't see a need for the
>>>> function name to match the group name here, we're still doing SPI.
>>>>
>>>> This can be seen in the DTS fix that Joel sent (disregarding the mixed
>>>> voltage pins problem).
>>>>
>>>> Thoughts?
>>>
>>> As you said, FWSPID function in existing code is defined as two groups.
>>>
>>> GROUP_DECL(FWSPID, Y1, Y2, Y3, Y4);
>>> GROUP_DECL(FWQSPID, Y1, Y2, Y3, Y4, AE12, AF12);
>>>
>>> In case of the FWSPID group, it defines Y1, Y2, Y3 and Y4 pin pads as
>>> FWSPI18 pins which can be multiplexed with eMMC so pinctrl driver sets
>>> SCU500[3] when we select this group. Also, if we select FWQSPID group,
>>> it additionally set AE12 and AF12 pin pads as FWSPIDQ2 and FWSPIDQ3 but
>>> these two pins are actually part of FWSPI function group that are
>>> exposed as dedicated pins on AST2600 SoC.
>>>
>>> Joel's patch can fix below pin mux setting error since there was a bug
>>> in aspeed-g6-pinctrl.dtsi.
>>>
>>> [    0.742963] aspeed-g6-pinctrl 1e6e2000.syscon:pinctrl: invalid
>>> function FWQSPID in map table
>>>
>>> But it doesn't fix an improper group selection in pinctrl-aspeed-g6
>>> driver.
>>>
>>> As we saw above, SCU500[3] bit will be set even when we select FWQSPID
>>> group, and it's described in datasheets like below.
>>>
>>> SCU500[3]
>>>     Boot from debug SPI (OTPSTRAP[2])
>>>     0: Disable (default)
>>>     1: Enable
>>>     Enable this bit will set CPU to boot from SPI that is attached on pins
>>>     FWSPI18*. This strap will not work when secure boot or boot from Uart5
>>>     is enabled. This bit is for verification and testing only. Please
>>>     don’t enable the OTPSTRAP[2] and protect it by setting OTPCFG30[2]=1
>>>     and OTPCFG28[2]=1 if there are security concerns.
>>>
>>> So if we set this bit once, BMC boot path will be immediately switched
>>> to FWSPI18 pins when we don't enable secure boot, and it breaks BMC
>>> booting. I observed this issue in my board and AST2600 EVB too.
>> 
>> Yep, this needs to be fixed.
>>>
>>> It's not just interface voltage level issue but also boot failure issue
>>> if a board uses dedicated FWSPI pins (AB14, AF13, AC14, AB13).
>> 
>> Okay, I wasn't across that part :)
>> 
>>>
>>> To fix the issue, this commit removes FWQSPID group from FWSPID
>>> function, and adds FWQSPI function and group to enable just AE12 and
>>> AF12 as FWSPIDQ2 and FWSPIDQ3 to use them with FWSPICS#, FWSPICK,
>>> FWSPIMOSI and FWSPIMISO pins.
>> 
>> Okay, probably wrote what I meant in a confusing way. I understand what
>> you've described, but what I was trying to suggest was instead of
>> creating a "FWQSPI" function and group was to instead have just the
>> "FWSPI" function to be used with both the "FWSPI" and "FWQSPI" groups.
>> This aligns with how the FWSPID function/groups work.
>
> FWSPI pins are dedicated pins just for boot firmware SPI interface so
> these pins don't need any pinmux setting. We just need to add pinmux
> setting of AE12 and AF12 for a case when FWSPI needs QSPI support with
> having these additional data pins. Do we need to define two groups
> including a dummy group for the dedicated pins? Can you please share
> more details of your idea? I don't see any example of dedicated pins in
> the driver.

Ah, yeah, if they're fixed pins then we don't need to worry about them 
in pinmux, in which case we only have the one group. I'm getting there 
slowly :)

I'll look over the patch again.

Andrew

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 0/5] Fix AST2600 quad mode SPI pinmux settings
  2022-03-08  0:37 ` Jae Hyun Yoo
@ 2022-03-17  1:11   ` Linus Walleij
  -1 siblings, 0 replies; 28+ messages in thread
From: Linus Walleij @ 2022-03-17  1:11 UTC (permalink / raw)
  To: Jae Hyun Yoo
  Cc: Joel Stanley, Rob Herring, Andrew Jeffery, Jamie Iles,
	Graeme Gregory, devicetree, linux-arm-kernel, linux-aspeed

On Tue, Mar 8, 2022 at 1:37 AM Jae Hyun Yoo <quic_jaehyoo@quicinc.com> wrote:

> I’m sending this patch series to fix current issues in AST2600 pinmux
> settings while enabling quad mode SPI support.

Patches 2 & 3 applied to the pinctrl tree.

Please funnel the DTS patches through the SoC tree.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 0/5] Fix AST2600 quad mode SPI pinmux settings
@ 2022-03-17  1:11   ` Linus Walleij
  0 siblings, 0 replies; 28+ messages in thread
From: Linus Walleij @ 2022-03-17  1:11 UTC (permalink / raw)
  To: Jae Hyun Yoo
  Cc: Joel Stanley, Rob Herring, Andrew Jeffery, Jamie Iles,
	Graeme Gregory, devicetree, linux-arm-kernel, linux-aspeed

On Tue, Mar 8, 2022 at 1:37 AM Jae Hyun Yoo <quic_jaehyoo@quicinc.com> wrote:

> I’m sending this patch series to fix current issues in AST2600 pinmux
> settings while enabling quad mode SPI support.

Patches 2 & 3 applied to the pinctrl tree.

Please funnel the DTS patches through the SoC tree.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 0/5] Fix AST2600 quad mode SPI pinmux settings
  2022-03-17  1:11   ` Linus Walleij
@ 2022-03-17  1:17     ` Joel Stanley
  -1 siblings, 0 replies; 28+ messages in thread
From: Joel Stanley @ 2022-03-17  1:17 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Jae Hyun Yoo, Rob Herring, Andrew Jeffery, Jamie Iles,
	Graeme Gregory, devicetree, Linux ARM, linux-aspeed

On Thu, 17 Mar 2022 at 01:11, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Mar 8, 2022 at 1:37 AM Jae Hyun Yoo <quic_jaehyoo@quicinc.com> wrote:
>
> > I’m sending this patch series to fix current issues in AST2600 pinmux
> > settings while enabling quad mode SPI support.
>
> Patches 2 & 3 applied to the pinctrl tree.
>
> Please funnel the DTS patches through the SoC tree.

Thanks for jumping on this Linus. We're not sure that this is the
correct fix, Andrew is still reviewing (see the comments on patch 3):

 https://lore.kernel.org/linux-arm-kernel/CACRpkdbFNLLve1-JntNW=eMT9ivZTZHBk-xpwK6w-kE0fczr+g@mail.gmail.com/T/#m2cdf4f8b55427d6040f5c13eb85dd656f3579c48

If you haven't pushed it out then please hold off. If you have, I'll
let Andrew jump in and recommend the best course of action.

>
> Yours,
> Linus Walleij

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 0/5] Fix AST2600 quad mode SPI pinmux settings
@ 2022-03-17  1:17     ` Joel Stanley
  0 siblings, 0 replies; 28+ messages in thread
From: Joel Stanley @ 2022-03-17  1:17 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Jae Hyun Yoo, Rob Herring, Andrew Jeffery, Jamie Iles,
	Graeme Gregory, devicetree, Linux ARM, linux-aspeed

On Thu, 17 Mar 2022 at 01:11, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Mar 8, 2022 at 1:37 AM Jae Hyun Yoo <quic_jaehyoo@quicinc.com> wrote:
>
> > I’m sending this patch series to fix current issues in AST2600 pinmux
> > settings while enabling quad mode SPI support.
>
> Patches 2 & 3 applied to the pinctrl tree.
>
> Please funnel the DTS patches through the SoC tree.

Thanks for jumping on this Linus. We're not sure that this is the
correct fix, Andrew is still reviewing (see the comments on patch 3):

 https://lore.kernel.org/linux-arm-kernel/CACRpkdbFNLLve1-JntNW=eMT9ivZTZHBk-xpwK6w-kE0fczr+g@mail.gmail.com/T/#m2cdf4f8b55427d6040f5c13eb85dd656f3579c48

If you haven't pushed it out then please hold off. If you have, I'll
let Andrew jump in and recommend the best course of action.

>
> Yours,
> Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 0/5] Fix AST2600 quad mode SPI pinmux settings
  2022-03-17  1:17     ` Joel Stanley
@ 2022-03-24 19:11       ` Linus Walleij
  -1 siblings, 0 replies; 28+ messages in thread
From: Linus Walleij @ 2022-03-24 19:11 UTC (permalink / raw)
  To: Joel Stanley
  Cc: Jae Hyun Yoo, Rob Herring, Andrew Jeffery, Jamie Iles,
	Graeme Gregory, devicetree, Linux ARM, linux-aspeed

On Thu, Mar 17, 2022 at 2:18 AM Joel Stanley <joel@jms.id.au> wrote:
> On Thu, 17 Mar 2022 at 01:11, Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > On Tue, Mar 8, 2022 at 1:37 AM Jae Hyun Yoo <quic_jaehyoo@quicinc.com> wrote:
> >
> > > I’m sending this patch series to fix current issues in AST2600 pinmux
> > > settings while enabling quad mode SPI support.
> >
> > Patches 2 & 3 applied to the pinctrl tree.
> >
> > Please funnel the DTS patches through the SoC tree.
>
> Thanks for jumping on this Linus. We're not sure that this is the
> correct fix, Andrew is still reviewing (see the comments on patch 3):
>
>  https://lore.kernel.org/linux-arm-kernel/CACRpkdbFNLLve1-JntNW=eMT9ivZTZHBk-xpwK6w-kE0fczr+g@mail.gmail.com/T/#m2cdf4f8b55427d6040f5c13eb85dd656f3579c48
>
> If you haven't pushed it out then please hold off. If you have, I'll
> let Andrew jump in and recommend the best course of action.

I just dropped them again.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH 0/5] Fix AST2600 quad mode SPI pinmux settings
@ 2022-03-24 19:11       ` Linus Walleij
  0 siblings, 0 replies; 28+ messages in thread
From: Linus Walleij @ 2022-03-24 19:11 UTC (permalink / raw)
  To: Joel Stanley
  Cc: Jae Hyun Yoo, Rob Herring, Andrew Jeffery, Jamie Iles,
	Graeme Gregory, devicetree, Linux ARM, linux-aspeed

On Thu, Mar 17, 2022 at 2:18 AM Joel Stanley <joel@jms.id.au> wrote:
> On Thu, 17 Mar 2022 at 01:11, Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > On Tue, Mar 8, 2022 at 1:37 AM Jae Hyun Yoo <quic_jaehyoo@quicinc.com> wrote:
> >
> > > I’m sending this patch series to fix current issues in AST2600 pinmux
> > > settings while enabling quad mode SPI support.
> >
> > Patches 2 & 3 applied to the pinctrl tree.
> >
> > Please funnel the DTS patches through the SoC tree.
>
> Thanks for jumping on this Linus. We're not sure that this is the
> correct fix, Andrew is still reviewing (see the comments on patch 3):
>
>  https://lore.kernel.org/linux-arm-kernel/CACRpkdbFNLLve1-JntNW=eMT9ivZTZHBk-xpwK6w-kE0fczr+g@mail.gmail.com/T/#m2cdf4f8b55427d6040f5c13eb85dd656f3579c48
>
> If you haven't pushed it out then please hold off. If you have, I'll
> let Andrew jump in and recommend the best course of action.

I just dropped them again.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2022-03-24 19:12 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-08  0:37 [PATCH 0/5] Fix AST2600 quad mode SPI pinmux settings Jae Hyun Yoo
2022-03-08  0:37 ` Jae Hyun Yoo
2022-03-08  0:37 ` [PATCH 1/5] ARM: dts: aspeed-g6: remove FWQSPID group in pinctrl dtsi Jae Hyun Yoo
2022-03-08  0:37   ` Jae Hyun Yoo
2022-03-08  0:37 ` [PATCH 2/5] pinctrl: pinctrl-aspeed-g6: " Jae Hyun Yoo
2022-03-08  0:37   ` Jae Hyun Yoo
2022-03-08  0:37 ` [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group Jae Hyun Yoo
2022-03-08  0:37   ` Jae Hyun Yoo
2022-03-16  3:33   ` Andrew Jeffery
2022-03-16  3:33     ` Andrew Jeffery
2022-03-16  5:05     ` Jae Hyun Yoo
2022-03-16  5:05       ` Jae Hyun Yoo
2022-03-16  5:45       ` Andrew Jeffery
2022-03-16  5:45         ` Andrew Jeffery
2022-03-16  6:03         ` Jae Hyun Yoo
2022-03-16  6:03           ` Jae Hyun Yoo
2022-03-16  6:11           ` Andrew Jeffery
2022-03-16  6:11             ` Andrew Jeffery
2022-03-08  0:37 ` [PATCH 4/5] ARM: dts: aspeed-g6: add FWQSPI group in pinctrl dtsi Jae Hyun Yoo
2022-03-08  0:37   ` Jae Hyun Yoo
2022-03-08  0:37 ` [PATCH 5/5] ARM: dts: aspeed-g6: fix SPI1/SPI2 quad pin group Jae Hyun Yoo
2022-03-08  0:37   ` Jae Hyun Yoo
2022-03-17  1:11 ` [PATCH 0/5] Fix AST2600 quad mode SPI pinmux settings Linus Walleij
2022-03-17  1:11   ` Linus Walleij
2022-03-17  1:17   ` Joel Stanley
2022-03-17  1:17     ` Joel Stanley
2022-03-24 19:11     ` Linus Walleij
2022-03-24 19:11       ` Linus Walleij

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.