mbox series

[V2,0/3] Fix the memory layout and add sgpio node for aspeed g6

Message ID 20201012033150.21056-1-billy_tsai@aspeedtech.com
Headers show
Series Fix the memory layout and add sgpio node for aspeed g6 | expand

Message

Billy Tsai Oct. 12, 2020, 3:31 a.m. UTC 
This patch series is used to add sgpiom and sgpios nodes and add pinctrl 
setting for sgpiom1

v2:
  - Split the change of dts and pinctrl to two commit.
  - Add the compatible string for aspeed,ast2600-sgpiom. 
    aspeed,ast2600-sgpios will implement in the future.

Billy Tsai (3):
  Arm: dts: aspeed-g6: Fix the register range of gpio
  Arm: dts: aspeed-g6: Add sgpio node
  pinctrl: aspeed-g6: Add sgpiom2 pinctrl setting

 .../devicetree/bindings/gpio/sgpio-aspeed.txt |  8 +--
 arch/arm/boot/dts/aspeed-g6-pinctrl.dtsi      |  5 ++
 arch/arm/boot/dts/aspeed-g6.dtsi              | 54 ++++++++++++++++++-
 drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c    | 30 +++++++++--
 4 files changed, 89 insertions(+), 8 deletions(-)

Comments

Joel Stanley Oct. 12, 2020, 4:30 a.m. UTC | #1 
On Mon, 12 Oct 2020 at 03:32, Billy Tsai <billy_tsai@aspeedtech.com> wrote:
>
> This patch is used to fix the memory range of gpio0
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  arch/arm/boot/dts/aspeed-g6.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed-g6.dtsi
> index 97ca743363d7..ad19dce038ea 100644
> --- a/arch/arm/boot/dts/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g6.dtsi
> @@ -357,7 +357,7 @@
>                                 #gpio-cells = <2>;
>                                 gpio-controller;
>                                 compatible = "aspeed,ast2600-gpio";
> -                               reg = <0x1e780000 0x800>;
> +                               reg = <0x1e780000 0x400>;
>                                 interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
>                                 gpio-ranges = <&pinctrl 0 0 208>;
>                                 ngpios = <208>;
> --
> 2.17.1
>
Joel Stanley Oct. 12, 2020, 4:36 a.m. UTC | #2 
On Mon, 12 Oct 2020 at 03:32, Billy Tsai <billy_tsai@aspeedtech.com> 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>

Reviewed-by: Joel Stanley <joel@jms.id.au>

Linus, can you take this through the pinctrl tree? The patch to the
will be fine to come through your tree as we rarely update that file.

> ---
>  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));
> +PIN_DECL_3(L24, GPIOA5, SGPM2LD, MACLINK2, SDA13);
>  FUNC_GROUP_DECL(MACLINK2, L24);
>
>  FUNC_GROUP_DECL(I2C13, K26, L24);
> @@ -95,16 +105,26 @@ FUNC_GROUP_DECL(I2C13, K26, L24);
>  #define L23 6
>  SIG_EXPR_LIST_DECL_SESG(L23, MACLINK3, MACLINK3, SIG_DESC_SET(SCU410, 6));
>  SIG_EXPR_LIST_DECL_SESG(L23, SCL14, I2C14, SIG_DESC_SET(SCU4B0, 6));
> -PIN_DECL_2(L23, GPIOA6, MACLINK3, SCL14);
> +/*SGPM2 is A1 Only */
> +SIG_EXPR_LIST_DECL_SESG(L23, SGPM2O, SGPM2, SIG_DESC_SET(SCU6D0, 6),
> +                         SIG_DESC_CLEAR(SCU410, 6), SIG_DESC_CLEAR(SCU4B0, 6),
> +                         SIG_DESC_CLEAR(SCU690, 6));
> +PIN_DECL_3(L23, GPIOA6, SGPM2O, MACLINK3, SCL14);
>  FUNC_GROUP_DECL(MACLINK3, L23);
>
>  #define K25 7
>  SIG_EXPR_LIST_DECL_SESG(K25, MACLINK4, MACLINK4, SIG_DESC_SET(SCU410, 7));
>  SIG_EXPR_LIST_DECL_SESG(K25, SDA14, I2C14, SIG_DESC_SET(SCU4B0, 7));
> -PIN_DECL_2(K25, GPIOA7, MACLINK4, SDA14);
> +/*SGPM2 is A1 Only */
> +SIG_EXPR_LIST_DECL_SESG(K25, SGPM2I, SGPM2, SIG_DESC_SET(SCU6D0, 7),
> +                         SIG_DESC_CLEAR(SCU410, 7), SIG_DESC_CLEAR(SCU4B0, 7),
> +                         SIG_DESC_CLEAR(SCU690, 7));
> +PIN_DECL_3(K25, GPIOA7, SGPM2I, MACLINK4, SDA14);
>  FUNC_GROUP_DECL(MACLINK4, K25);
>
>  FUNC_GROUP_DECL(I2C14, L23, K25);
> +/*SGPM2 is A1 Only */
> +FUNC_GROUP_DECL(SGPM2, K26, L24, L23, K25);
>
>  #define J26 8
>  SIG_EXPR_LIST_DECL_SESG(J26, SALT1, SALT1, SIG_DESC_SET(SCU410, 8));
> @@ -2060,6 +2080,7 @@ static const struct aspeed_pin_group aspeed_g6_groups[] = {
>         ASPEED_PINCTRL_GROUP(EMMCG4),
>         ASPEED_PINCTRL_GROUP(EMMCG8),
>         ASPEED_PINCTRL_GROUP(SGPM1),
> +       ASPEED_PINCTRL_GROUP(SGPM2),
>         ASPEED_PINCTRL_GROUP(SGPS1),
>         ASPEED_PINCTRL_GROUP(SIOONCTRL),
>         ASPEED_PINCTRL_GROUP(SIOPBI),
> @@ -2276,6 +2297,7 @@ static const struct aspeed_pin_function aspeed_g6_functions[] = {
>         ASPEED_PINCTRL_FUNC(SD1),
>         ASPEED_PINCTRL_FUNC(SD2),
>         ASPEED_PINCTRL_FUNC(SGPM1),
> +       ASPEED_PINCTRL_FUNC(SGPM2),
>         ASPEED_PINCTRL_FUNC(SGPS1),
>         ASPEED_PINCTRL_FUNC(SIOONCTRL),
>         ASPEED_PINCTRL_FUNC(SIOPBI),
> --
> 2.17.1
>
Andrew Jeffery Oct. 26, 2020, 1:05 a.m. UTC | #3 
On Mon, 12 Oct 2020, at 14:01, Billy Tsai wrote:
> This patch is used to fix the memory range of gpio0
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
Andrew Jeffery Oct. 26, 2020, 1:26 a.m. UTC | #4 
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?

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?

Cheers,

Andrew
Billy Tsai Oct. 26, 2020, 2:03 a.m. UTC | #5 
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.
    > 
    > 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.
    > Cheers,
    > 
    > Andrew

Best Regards,
Billy Tsai
Andrew Jeffery Oct. 26, 2020, 2:20 a.m. UTC | #6 
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
Billy Tsai Oct. 26, 2020, 2:56 a.m. UTC | #7 
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@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?
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
Joel Stanley Oct. 28, 2020, 5:12 a.m. UTC | #8 
On Mon, 26 Oct 2020 at 01:05, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Mon, 12 Oct 2020, at 14:01, Billy Tsai wrote:
> > This patch is used to fix the memory range of gpio0
> >
> > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

I've applied this with:

Fixes: 8dbcb5b709b9 ("ARM: dts: aspeed-g6: Add gpio devices")

Cheers,

Joel