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

[Jae Hyun Yoo <quic_jaehyoo@quicinc.com>]

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