Re: [PATCH 3/3] pinctrl: aspeed-g6: Add sgpiom2 pinctrl setting

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
Date: Mon, 26 Oct 2020 12:50:48 +1030	[thread overview]
Message-ID: <3fa4fda9-1ba9-4cb6-bd7f-0200bca06f52@www.fastmail.com> (raw)
In-Reply-To: <82A00921-93ED-42A8-9B93-8F1E8025BA89@aspeedtech.com>

On Mon, 26 Oct 2020, at 12:33, Billy Tsai wrote:
> 
> On 2020/10/26, 9:27 AM, Andrew Jeffery wrote:
>     
>     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 
> sgpio.
>     > > 
>     > > 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 
> +++++++++++++++++++---
>     > >  2 files changed, 31 insertions(+), 4 deletions(-)
>     > > 
>     > > diff --git a/arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi 
>     > > 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 = "SGPM1";
>     > >  	};
>     > >  
>     > > +	pinctrl_sgpm2_default: sgpm2_default {
>     > > +		function = "SGPM2";
>     > > +		groups = "SGPM2";
>     > > +	};
>     > > +
>     > >  	pinctrl_sgps1_default: sgps1_default {
>     > >  		function = "SGPS1";
>     > >  		groups = "SGPS1";
>     > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c 
>     > > 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 */
>     > >  
>     > >  #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, 
> SIG_DESC_SET(SCU410, 4));
>     > >  SIG_EXPR_LIST_DECL_SESG(K26, SCL13, I2C13, 
> SIG_DESC_SET(SCU4B0, 4));
>     > > -PIN_DECL_2(K26, GPIOA4, MACLINK1, SCL13);
>     > > +/*SGPM2 is A1 Only */
>     > > +SIG_EXPR_LIST_DECL_SESG(K26, SGPM2CLK, SGPM2, 
> 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);
>     > >  
>     > >  #define L24 5
>     > >  SIG_EXPR_LIST_DECL_SESG(L24, MACLINK2, MACLINK2, 
> SIG_DESC_SET(SCU410, 5));
>     > >  SIG_EXPR_LIST_DECL_SESG(L24, SDA13, I2C13, 
> SIG_DESC_SET(SCU4B0, 5));
>     > > -PIN_DECL_2(L24, GPIOA5, MACLINK2, SDA13);
>     > > +/*SGPM2 is A1 Only */
>     > > +SIG_EXPR_LIST_DECL_SESG(L24, SGPM2LD, SGPM2, 
> SIG_DESC_SET(SCU6D0, 5),
>     > > +			  SIG_DESC_CLEAR(SCU410, 5), SIG_DESC_CLEAR(SCU4B0, 5),
>     > > +			  SIG_DESC_CLEAR(SCU690, 5));
>     > 
>     > A few things:
>     > 
>     > 1. It looks like the Multi-function Pins Mapping and Control 
> table in section 5.1 of the datasheet only tells part of the story. It 
> lists SGPS2 on the pins you've modified in this patch but not SGPM2. 
> However, the table in section 2.1 (Pin Description) does outline SGPM2 
> and SGPS2 are routed via the same pins, though this does not listed the 
> associated registers and bit fields. Can we fix the table in 5.1 so 
> it's easier to review this patch?

> You can see section 5.2 to find SGPIO master interface table. It lists 
> 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 details. 
It appears you'd need to add another two columns to the table to capture the 
info - is Aspeed planning to do that in a future release of the datasheet?

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

Thanks!

Andrew