On 3/28/2022 4:01 PM, Andrew Jeffery wrote:
On Tue, 29 Mar 2022, at 01:11, Jae Hyun Yoo wrote:On 3/27/2022 8:18 PM, Andrew Jeffery wrote:On Sat, 26 Mar 2022, at 02:10, Jae Hyun Yoo wrote:FWSPIDQ2 and FWSPIDQ3 are not part of FWSPI18 interface so remove FWQSPID group in pinctrl. 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@xxxxxxxxxxx> Fixes: 2eda1cdec49f ("pinctrl: aspeed: Add AST2600 pinmux support") --- Changes in v2: * None. 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);Really this is the FWSPI18 group now? The FWSPID name never made sense. I'm not sure what I was thinking.Yes, it's now the FWSPI18 which is described as 'debug SPI' in the datasheet. Corresponding SCU500[3] bit is also described as that 'the bit is for verification and testing only'. Probably, you was thinking 'D' as in 'Debug' for the FWSPID naming.I suspect it was also to do with some lack of detail in the early data sheets :)
Right, there were lots of lack of detail in the early version of AST2600 datasheets and FWSPI18 description is still insufficient even in the latest version, IMO.
Actually, I think it's worth squashing this with 3/5 so it's a proper fix rather than separate remove/add?Two reasons I separated them. 1. Author is different. 2. 2/5 is a bug fix and 3/5 introduces a new pinmux.Okay, I'm not terribly fussed.
Thanks! I'll keep them separated.
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),We should also remove the function (not just the group).Still worth to keep FWSPID to support SCU500[3] - Boot from debug SPI. FWSPID would work on single and dual data mode only.I guess, yeah, though the only use case I see for it is a temporary devicetree change to account for someone setting the strap pin into the debug state. I don't see a reason to support it beyond that, but that said we still need to support it for that use case.
Great! I'll keep FWSPID as supported. I'll send v3 soon. Thanks a lot for your review! -Jae
