[PATCH v2 2/5] pinctrl: pinctrl-aspeed-g6: remove FWQSPID group in pinctrl

[Andrew Jeffery]

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 at quicinc.com>
>>> 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 :)

>
>> 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.

>
>>>   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.

Andrew