Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
User-Agent: Cyrus-JMAP/3.3.0-529-g69105b1-fm-20201021.003-g69105b13
Mime-Version: 1.0
Message-Id: <3fa4fda9-1ba9-4cb6-bd7f-0200bca06f52@www.fastmail.com>
In-Reply-To: <82A00921-93ED-42A8-9B93-8F1E8025BA89@aspeedtech.com>
References: <20201012033150.21056-1-billy_tsai@aspeedtech.com>
 <20201012033150.21056-4-billy_tsai@aspeedtech.com>
 <ce3dce98-1daa-47bb-a688-0d5a743e45b2@www.fastmail.com>
 <82A00921-93ED-42A8-9B93-8F1E8025BA89@aspeedtech.com>
Date:   Mon, 26 Oct 2020 12:50:48 +1030
From:   "Andrew Jeffery" <andrew@aj.id.au>
To:     "Billy Tsai" <billy_tsai@aspeedtech.com>,
        "Rob Herring" <robh+dt@kernel.org>,
        "Joel Stanley" <joel@jms.id.au>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Linus Walleij" <linus.walleij@linaro.org>,
        "Bartosz Golaszewski" <bgolaszewski@baylibre.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>
Cc:     BMC-SW <BMC-SW@aspeedtech.com>
Subject: Re: [PATCH 3/3] pinctrl: aspeed-g6: Add sgpiom2 pinctrl setting
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On Mon, 26 Oct 2020, at 12:33, Billy Tsai wrote:
>=20
> =EF=BB=BFOn 2020/10/26, 9:27 AM, Andrew Jeffery wrote:
>    =20
>     On Mon, 12 Oct 2020, at 14:01, Billy Tsai wrote:
>     > > At ast2600a1 we change feature of master sgpio to 2 sets.
>     > > So this patch is used to add the pinctrl setting of the new=20=

> sgpio.
>     > >=20
>     > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
>     > > ---
>     > >  arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi   |  5 ++++
>     > >  drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 30=20
> +++++++++++++++++++---
>     > >  2 files changed, 31 insertions(+), 4 deletions(-)
>     > >=20
>     > > diff --git a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi=20
>     > > b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
>     > > index 7028e21bdd98..a16ecf08e307 100644
>     > > --- a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
>     > > +++ b/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi
>     > > @@ -862,6 +862,11 @@
>     > >  		groups =3D "SGPM1";
>     > >  	};
>     > > =20
>     > > +	pinctrl_sgpm2_default: sgpm2_default {
>     > > +		function =3D "SGPM2";
>     > > +		groups =3D "SGPM2";
>     > > +	};
>     > > +
>     > >  	pinctrl_sgps1_default: sgps1_default {
>     > >  		function =3D "SGPS1";
>     > >  		groups =3D "SGPS1";
>     > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c=20
>     > > b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>     > > index 34803a6c7664..b673a44ffa3b 100644
>     > > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>     > > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
>     > > @@ -46,8 +46,10 @@
>     > >  #define SCU620		0x620 /* Disable GPIO Internal Pull-Down #4 *=
/
>     > >  #define SCU634		0x634 /* Disable GPIO Internal Pull-Down #5 *=
/
>     > >  #define SCU638		0x638 /* Disable GPIO Internal Pull-Down #6 *=
/
>     > > +#define SCU690		0x690 /* Multi-function Pin Control #24 */
>     > >  #define SCU694		0x694 /* Multi-function Pin Control #25 */
>     > >  #define SCU69C		0x69C /* Multi-function Pin Control #27 */
>     > > +#define SCU6D0		0x6D0 /* Multi-function Pin Control #28 */
>     > >  #define SCUC20		0xC20 /* PCIE configuration Setting Control *=
/
>     > > =20
>     > >  #define ASPEED_G6_NR_PINS 256
>     > > @@ -81,13 +83,21 @@ FUNC_GROUP_DECL(I2C12, L26, K24);
>     > >  #define K26 4
>     > >  SIG_EXPR_LIST_DECL_SESG(K26, MACLINK1, MACLINK1,=20
> SIG_DESC_SET(SCU410, 4));
>     > >  SIG_EXPR_LIST_DECL_SESG(K26, SCL13, I2C13,=20
> SIG_DESC_SET(SCU4B0, 4));
>     > > -PIN_DECL_2(K26, GPIOA4, MACLINK1, SCL13);
>     > > +/*SGPM2 is A1 Only */
>     > > +SIG_EXPR_LIST_DECL_SESG(K26, SGPM2CLK, SGPM2,=20
> SIG_DESC_SET(SCU6D0, 4),
>     > > +			  SIG_DESC_CLEAR(SCU410, 4), SIG_DESC_CLEAR(SCU4B0, 4),
>     > > +			  SIG_DESC_CLEAR(SCU690, 4));
>     > > +PIN_DECL_3(K26, GPIOA4, SGPM2CLK, MACLINK1, SCL13);
>     > >  FUNC_GROUP_DECL(MACLINK1, K26);
>     > > =20
>     > >  #define L24 5
>     > >  SIG_EXPR_LIST_DECL_SESG(L24, MACLINK2, MACLINK2,=20
> SIG_DESC_SET(SCU410, 5));
>     > >  SIG_EXPR_LIST_DECL_SESG(L24, SDA13, I2C13,=20
> SIG_DESC_SET(SCU4B0, 5));
>     > > -PIN_DECL_2(L24, GPIOA5, MACLINK2, SDA13);
>     > > +/*SGPM2 is A1 Only */
>     > > +SIG_EXPR_LIST_DECL_SESG(L24, SGPM2LD, SGPM2,=20
> SIG_DESC_SET(SCU6D0, 5),
>     > > +			  SIG_DESC_CLEAR(SCU410, 5), SIG_DESC_CLEAR(SCU4B0, 5),
>     > > +			  SIG_DESC_CLEAR(SCU690, 5));
>     >=20
>     > A few things:
>     >=20
>     > 1. It looks like the Multi-function Pins Mapping and Control=20
> table in section 5.1 of the datasheet only tells part of the story. It=
=20
> lists SGPS2 on the pins you've modified in this patch but not SGPM2.=20=

> However, the table in section 2.1 (Pin Description) does outline SGPM2=
=20
> and SGPS2 are routed via the same pins, though this does not listed th=
e=20
> associated registers and bit fields. Can we fix the table in 5.1 so=20=

> it's easier to review this patch?

> You can see section 5.2 to find SGPIO master interface table. It lists=
=20
> balls and register setting information of the SGPIOM1 and SGPIOM2.

Yep, though typically I use the table in 5.1 to figure out the pinctrl d=
etails.=20
It appears you'd need to add another two columns to the table to capture=
 the=20
info - is Aspeed planning to do that in a future release of the datashee=
t?

>     >=20
>     > 2. We don't need to specify the _CLEAR() behaviour here as this=20=

> is implied by the process to disable the higher priority mux=20
> configurations. It should be enough to do:
>     >=20
>     > SIG_EXPR_LIST_DECL_SESG(L24, SGPM2LD, SGPM2, SIG_DESC_SET(SCU6D0=
,=20
> 5));
>     >=20
>     > However, this requires that we also define the priorities=20
> correctly, so:
>     >=20
>     > 3. Can we add both the SGPS2 and SGPM2 configurations so we have=
=20
> a complete definition for the pins?
>     >=20
> Thank you for your advice. I will complete the full configurations and=
=20
> send the V2 patch.

Thanks!

Andrew