[PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group

[Andrew Jeffery]

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 at 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 at aspeedtech.com>
>>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo at 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