From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 04FBBC433F5 for ; Wed, 16 Mar 2022 06:21:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237817AbiCPGWc (ORCPT ); Wed, 16 Mar 2022 02:22:32 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59618 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237240AbiCPGWb (ORCPT ); Wed, 16 Mar 2022 02:22:31 -0400 X-Greylist: delayed 546 seconds by postgrey-1.37 at lindbergh.monkeyblade.net; Tue, 15 Mar 2022 23:21:16 PDT Received: from out4-smtp.messagingengine.com (out4-smtp.messagingengine.com [66.111.4.28]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BFE2860D95 for ; Tue, 15 Mar 2022 23:21:16 -0700 (PDT) Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id 018585C00D0; Wed, 16 Mar 2022 02:12:08 -0400 (EDT) Received: from imap49 ([10.202.2.99]) by compute3.internal (MEProxy); Wed, 16 Mar 2022 02:12:08 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h=cc :cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm3; bh=F5pL2KHL4n0aMu 847nnrHGO3av4yWeplTZO11e2ZVhw=; b=LPEJXA5oYNdEKzS0c1uxowB6yK9rhp pqp8Qy2USt9KW3o0kqtwQ3ezS13D+XYH1SE48CmE7cTeTPA+oxKOp3LI31NkTWVm Tc/mzHwT0+URQiJJAPdLHxs/4jgxD5kw2Fm4zQxYYxrm/e230v57MOH3hHYIb7vp KkOllttpPehrX/ZhCfAa4J1hZKaYcNmoAJZWj4urKzjlHpYpHqRhx6Pv794yR1IW sP9GL+SwIRb1Tui7UxK2M/o/c9y/siiiD+XBU0d/LCOF1cc7g47VFRinJd7LQ2wm AGFJQqCs0sbxg8MbkBTZTFYBAc3+tvSsYmRCrjA26gELwj18ZdK9YoGA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=F5pL2KHL4n0aMu847nnrHGO3av4yWeplTZO11e2ZV hw=; b=DxaCkxRwNIqRVw5SBSOB8W05sWyxilsuBymOfu1JqLhr2e24Xb1UL+Sys PeCLe4SmWragDWnH9gTyaLXeL89X78dbS9K/aYBu3F+y8l14+4SHG55LiEJR5WDM Rc4oY/0WMrpbb0Dbqz0igGoam3q3jdfF59XXWMbsjFZ70LKZMnqEavBLi7yWKram U2VEPEVQrXdoP8JKlQ3uKiWgMhqRzwa5jV9xXki/Xxh44a23QEnLTPFS0AD9xV0e FjLoTFlsf2gpkQh8sNgb4itUtDmiec/A3icwaX7BTeQ/b29kZ+He3o/EVfafxEf7 Ob/mfKdZHOd+smotBZhgVW3eJuUhA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvvddrudefuddgleduucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepofgfggfkjghffffhvffutgfgsehtqhertderreejnecuhfhrohhmpedftehn ughrvgifucflvghffhgvrhihfdcuoegrnhgurhgvfiesrghjrdhiugdrrghuqeenucggtf frrghtthgvrhhnpedvgeekheegfedvhfethefhudetteegueeggfeiieegueehkedugedt kefglefgheenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhroh hmpegrnhgurhgvfiesrghjrdhiugdrrghu X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id 90925F6007E; Wed, 16 Mar 2022 02:12:07 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.5.0-alpha0-4907-g25ce6f34a9-fm-20220311.001-g25ce6f34 Mime-Version: 1.0 Message-Id: <2e534b24-12bf-4bfe-ab4d-566710aa03fa@www.fastmail.com> In-Reply-To: References: <20220308003745.3930607-1-quic_jaehyoo@quicinc.com> <20220308003745.3930607-4-quic_jaehyoo@quicinc.com> <700af02b-a220-495f-861a-af10f30b482a@www.fastmail.com> <2f283c1e-adad-97c5-cc7c-2f0cf4f67150@quicinc.com> <3056f002-4d9a-461e-8a29-12b6742e99fb@www.fastmail.com> Date: Wed, 16 Mar 2022 16:41:46 +1030 From: "Andrew Jeffery" To: "Jae Hyun Yoo" , "Joel Stanley" , "Rob Herring" , "Linus Walleij" Cc: "Jamie Iles" , "Graeme Gregory" , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-aspeed@lists.ozlabs.org, "Johnny Huang" Subject: Re: [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group Content-Type: text/plain;charset=utf-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Wed, 16 Mar 2022, at 16:33, Jae Hyun Yoo wrote: > On 3/15/2022 10:45 PM, Andrew Jeffery wrote: >>=20 >>=20 >> 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 >>>>> >>>>> 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 >>>>> Signed-off-by: Jae Hyun Yoo >>>>> --- >>>>> 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(SCU43= 8, 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(SCU43= 8, 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 spec= ific >>>> group associated with the specific style of SPI bus you desire. This >>>> way you'd have a pinctrl node like: >>>> >>>> pinctrl_fwqspi_default =3D { >>>> function =3D "FWSPID"; >>>> group =3D "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 mi= xed >>>> voltage pins problem). >>>> >>>> Thoughts? >>> >>> As you said, FWSPID function in existing code is defined as two grou= ps. >>> >>> 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 se= ts >>> SCU500[3] when we select this group. Also, if we select FWQSPID grou= p, >>> 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 b= ug >>> 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 FWQSP= ID >>> 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 o= n 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=E2=80=99t enable the OTPSTRAP[2] and protect it by setting O= TPCFG30[2]=3D1 >>> and OTPCFG28[2]=3D1 if there are security concerns. >>> >>> So if we set this bit once, BMC boot path will be immediately switch= ed >>> 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. >>=20 >> Yep, this needs to be fixed. >>> >>> It's not just interface voltage level issue but also boot failure is= sue >>> if a board uses dedicated FWSPI pins (AB14, AF13, AC14, AB13). >>=20 >> Okay, I wasn't across that part :) >>=20 >>> >>> 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. >>=20 >> Okay, probably wrote what I meant in a confusing way. I understand wh= at >> 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. > > FWSPI pins are dedicated pins just for boot firmware SPI interface so > these pins don't need any pinmux setting. We just need to add pinmux > setting of AE12 and AF12 for a case when FWSPI needs QSPI support with > having these additional data pins. Do we need to define two groups > including a dummy group for the dedicated pins? Can you please share > more details of your idea? I don't see any example of dedicated pins in > the driver. Ah, yeah, if they're fixed pins then we don't need to worry about them=20 in pinmux, in which case we only have the one group. I'm getting there=20 slowly :) I'll look over the patch again. Andrew