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

[Jae Hyun Yoo]

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 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 :)

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