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

[Billy Tsai <billy_tsai@xxxxxxxxxxxxxx>]

On 2020/10/26, 10:21 AM, Andrew Jeffery wrote:

    
    
    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@xxxxxxxxxxxxxx>
    >     > > ---
    >     > >  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?
Yes, we will update the datasheet to add another two columns.    
    >     > 
    >     > 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