Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1491968 > unrolled thread

[PATCH 0/8] pinctrl: aspeed: Fixes for core and g5, implement remaining pins

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/8] pinctrl: aspeed: Fixes for core and g5, implement remaining pins Andrew Jeffery <andrew@aj.id.au> - 2016-09-27 17:00 +0200
    [PATCH 1/8] pinctrl: aspeed: "Not enabled" is a significant mux state Andrew Jeffery <andrew@aj.id.au> - 2016-09-27 17:00 +0200
      Re: [PATCH 1/8] pinctrl: aspeed: "Not enabled" is a significant mux state Joel Stanley <joel@jms.id.au> - 2016-09-29 03:00 +0200
    [PATCH 2/8] pinctrl: aspeed-g5: Fix names of GPID2 pins Andrew Jeffery <andrew@aj.id.au> - 2016-09-27 17:00 +0200
      Re: [PATCH 2/8] pinctrl: aspeed-g5: Fix names of GPID2 pins Linus Walleij <linus.walleij@linaro.org> - 2016-10-10 10:00 +0200
    [PATCH 5/8] pinctrl: aspeed: Enable capture of off-SCU pinmux state Andrew Jeffery <andrew@aj.id.au> - 2016-09-27 17:00 +0200
      Re: [PATCH 5/8] pinctrl: aspeed: Enable capture of off-SCU pinmux state Joel Stanley <joel@jms.id.au> - 2016-09-29 08:50 +0200
        Re: [PATCH 5/8] pinctrl: aspeed: Enable capture of off-SCU pinmux  state Andrew Jeffery <andrew@aj.id.au> - 2016-09-29 10:00 +0200
    Re: [PATCH 7/8] pinctrl: aspeed-g4: Add mux configuration for all pins Joel Stanley <joel@jms.id.au> - 2016-09-29 03:00 +0200
    Re: [PATCH 7/8] pinctrl: aspeed-g4: Add mux configuration for all  pins Rob Herring <robh@kernel.org> - 2016-10-03 21:10 +0200
      Re: [PATCH 7/8] pinctrl: aspeed-g4: Add mux configuration for all  pins Andrew Jeffery <andrew@aj.id.au> - 2016-10-04 03:10 +0200
    Re: [PATCH 8/8] pinctrl: aspeed-g5: Add mux configuration for all  pins Rob Herring <robh@kernel.org> - 2016-10-10 03:00 +0200
    Re: [PATCH 0/8] pinctrl: aspeed: Fixes for core and g5, implement  remaining pins Linus Walleij <linus.walleij@linaro.org> - 2016-10-10 10:10 +0200
      Re: [PATCH 0/8] pinctrl: aspeed: Fixes for core and g5, implement  remaining pins Andrew Jeffery <andrew@aj.id.au> - 2016-10-11 01:40 +0200

#1491968 — [PATCH 0/8] pinctrl: aspeed: Fixes for core and g5, implement remaining pins

Hi all,

The initial Aspeed pinctrl patches implemented a subset of pins for each of the
g4 and g5 SoCs. This series provides a number of fixes to the initial patches,
mostly for issues identified in the g5 driver. The fixes account for the first
half of the series (up to and including "pinctrl: aspeed-g5: Fix pin
association of SPI1 function") and should be applied for 4.9.

The second half, from "pinctrl: aspeed: Enable capture of off-SCU pinmux
state", implements some additional functionality in the core engine for the
Aspeed SoCs and follows up with patches implementing mux configuration tables
for all remaining pins. Given the significant additions in the last few
patches, their lateness in the cycle and the light testing they have received
they are best left for 4.10, but I'm keen to get them out for review.

Cheers,

Andrew

Andrew Jeffery (8):
  pinctrl: aspeed: "Not enabled" is a significant mux state
  pinctrl: aspeed-g5: Fix names of GPID2 pins
  pinctrl: aspeed-g5: Fix GPIOE1 typo
  pinctrl: aspeed-g5: Fix pin association of SPI1 function
  pinctrl: aspeed: Enable capture of off-SCU pinmux state
  pinctrl: aspeed-g4: Capture SuperIO pinmux dependency
  pinctrl: aspeed-g4: Add mux configuration for all pins
  pinctrl: aspeed-g5: Add mux configuration for all pins

 Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt |   36 +-
 drivers/pinctrl/aspeed/pinctrl-aspeed-g4.c                   | 1098 ++++-
 drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c                   | 1574 ++++++-
 drivers/pinctrl/aspeed/pinctrl-aspeed.c                      |   65 +-
 drivers/pinctrl/aspeed/pinctrl-aspeed.h                      |   19 +-
 5 files changed, 2737 insertions(+), 55 deletions(-)

base-commit: 8d0a0ac0abcdba5b5d52726055c95f1f6234e85e
-- 
git-series 0.8.10

[toc] | [next] | [standalone]

#1491969 — [PATCH 1/8] pinctrl: aspeed: "Not enabled" is a significant mux state

Consider a scenario with one pin P that has two signals A and B, where A
is defined to be higher priority than B: That is, if the mux IP is in a
state that would consider both A and B to be active on P, then A will be
the active signal.

To instead configure B as the active signal we must configure the mux so
that A is inactive. The mux state for signals can be described by
logical operations on one or more bits from one or more registers (a
"signal expression"), which in some cases leads to aliased mux states for
a particular signal. Further, signals described by multi-bit bitfields
often do not only need to record the states that would make them active
(the "enable" expressions), but also the states that makes them inactive
(the "disable" expressions). All of this combined leads to four possible
states for a signal:

         1. A signal is active with respect to an "enable" expression
         2. A signal is not active with respect to an "enable" expression
         3. A signal is inactive with respect to a "disable" expression
         4. A signal is not inactive with respect to a "disable" expression

In the case of P, if we are looking to activate B without explicitly
having configured A it's enough to consider A inactive if all of A's
"enable" signal expressions evaluate to "not active". If any evaluate to
"active" then the corresponding "disable" states must be applied so it
becomes inactive.

For example, on the AST2400 the pins composing GPIO bank H provide
signals ROMD8 through ROMD15 (high priority) and those for UART6 (low
priority). The mux states for ROMD8 through ROMD15 are aliased, i.e.
there are two mux states that result in the respective signals being
configured:

         A. SCU90[6]=1
         B. Strap[4,1:0]=100

Further, the second mux state is a 3-bit bitfield that explicitly
defines the enabled state but the disabled state is implicit, i.e. if
Strap[4,1:0] is not exactly "100" then ROMD8 through ROMD15 are not
considered active. This requires the mux function evaluation logic to
use approach 2. above, however the existing code was using approach 3.
The problem was brought to light on the Palmetto machines where the
strap register value is 0x120ce416, and prevented GPIO requests in bank
H from succeeding despite the hardware being in a position to allow
them.

Fixes: 318398c09a8d ("pinctrl: Add core pinctrl support for Aspeed SoCs")
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/pinctrl/aspeed/pinctrl-aspeed.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
index 0391f9f13f3e..49aeba912531 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
@@ -166,13 +166,9 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
 				bool enable, struct regmap *map)
 {
 	int i;
-	bool ret;
-
-	ret = aspeed_sig_expr_eval(expr, enable, map);
-	if (ret)
-		return ret;
 
 	for (i = 0; i < expr->ndescs; i++) {
+		bool ret;
 		const struct aspeed_sig_desc *desc = &expr->descs[i];
 		u32 pattern = enable ? desc->enable : desc->disable;
 
@@ -199,12 +195,18 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
 static bool aspeed_sig_expr_enable(const struct aspeed_sig_expr *expr,
 				   struct regmap *map)
 {
+	if (aspeed_sig_expr_eval(expr, true, map))
+		return true;
+
 	return aspeed_sig_expr_set(expr, true, map);
 }
 
 static bool aspeed_sig_expr_disable(const struct aspeed_sig_expr *expr,
 				    struct regmap *map)
 {
+	if (!aspeed_sig_expr_eval(expr, true, map))
+		return true;
+
 	return aspeed_sig_expr_set(expr, false, map);
 }
 
-- 
git-series 0.8.10

[toc] | [prev] | [next] | [standalone]

#1493075 — Re: [PATCH 1/8] pinctrl: aspeed: "Not enabled" is a significant mux state

On Wed, Sep 28, 2016 at 12:20 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> Consider a scenario with one pin P that has two signals A and B, where A
> is defined to be higher priority than B: That is, if the mux IP is in a
> state that would consider both A and B to be active on P, then A will be
> the active signal.
>
> To instead configure B as the active signal we must configure the mux so
> that A is inactive. The mux state for signals can be described by
> logical operations on one or more bits from one or more registers (a
> "signal expression"), which in some cases leads to aliased mux states for
> a particular signal. Further, signals described by multi-bit bitfields
> often do not only need to record the states that would make them active
> (the "enable" expressions), but also the states that makes them inactive
> (the "disable" expressions). All of this combined leads to four possible
> states for a signal:
>
>          1. A signal is active with respect to an "enable" expression
>          2. A signal is not active with respect to an "enable" expression
>          3. A signal is inactive with respect to a "disable" expression
>          4. A signal is not inactive with respect to a "disable" expression
>
> In the case of P, if we are looking to activate B without explicitly
> having configured A it's enough to consider A inactive if all of A's
> "enable" signal expressions evaluate to "not active". If any evaluate to
> "active" then the corresponding "disable" states must be applied so it
> becomes inactive.
>
> For example, on the AST2400 the pins composing GPIO bank H provide
> signals ROMD8 through ROMD15 (high priority) and those for UART6 (low
> priority). The mux states for ROMD8 through ROMD15 are aliased, i.e.
> there are two mux states that result in the respective signals being
> configured:
>
>          A. SCU90[6]=1
>          B. Strap[4,1:0]=100
>
> Further, the second mux state is a 3-bit bitfield that explicitly
> defines the enabled state but the disabled state is implicit, i.e. if
> Strap[4,1:0] is not exactly "100" then ROMD8 through ROMD15 are not
> considered active. This requires the mux function evaluation logic to
> use approach 2. above, however the existing code was using approach 3.
> The problem was brought to light on the Palmetto machines where the
> strap register value is 0x120ce416, and prevented GPIO requests in bank
> H from succeeding despite the hardware being in a position to allow
> them.
>
> Fixes: 318398c09a8d ("pinctrl: Add core pinctrl support for Aspeed SoCs")
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

That's a decent commit message.

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

> ---
>  drivers/pinctrl/aspeed/pinctrl-aspeed.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> index 0391f9f13f3e..49aeba912531 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> @@ -166,13 +166,9 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
>                                 bool enable, struct regmap *map)
>  {
>         int i;
> -       bool ret;
> -
> -       ret = aspeed_sig_expr_eval(expr, enable, map);
> -       if (ret)
> -               return ret;
>
>         for (i = 0; i < expr->ndescs; i++) {
> +               bool ret;
>                 const struct aspeed_sig_desc *desc = &expr->descs[i];
>                 u32 pattern = enable ? desc->enable : desc->disable;
>
> @@ -199,12 +195,18 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
>  static bool aspeed_sig_expr_enable(const struct aspeed_sig_expr *expr,
>                                    struct regmap *map)
>  {
> +       if (aspeed_sig_expr_eval(expr, true, map))
> +               return true;
> +
>         return aspeed_sig_expr_set(expr, true, map);
>  }
>
>  static bool aspeed_sig_expr_disable(const struct aspeed_sig_expr *expr,
>                                     struct regmap *map)
>  {
> +       if (!aspeed_sig_expr_eval(expr, true, map))
> +               return true;
> +
>         return aspeed_sig_expr_set(expr, false, map);
>  }
>
> --
> git-series 0.8.10

[toc] | [prev] | [next] | [standalone]

#1491972 — [PATCH 2/8] pinctrl: aspeed-g5: Fix names of GPID2 pins

Fixes simple typos in the initial commit. There is no behavioural
change.

Fixes: 56e57cb6c07f (pinctrl: Add pinctrl-aspeed-g5 driver)
Reported-by: Xo Wang <xow@google.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c
index e1ab864e1a7f..14639834a5eb 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c
@@ -151,21 +151,21 @@ FUNC_GROUP_DECL(GPID0, F19, E21);
 
 #define GPID2_DESC      SIG_DESC_SET(SCU8C, 9)
 
-#define D20 26
+#define F20 26
 SIG_EXPR_LIST_DECL_SINGLE(SD2DAT0, SD2, SD2_DESC);
 SIG_EXPR_DECL(GPID2IN, GPID2, GPID2_DESC);
 SIG_EXPR_DECL(GPID2IN, GPID, GPID_DESC);
 SIG_EXPR_LIST_DECL_DUAL(GPID2IN, GPID2, GPID);
-MS_PIN_DECL(D20, GPIOD2, SD2DAT0, GPID2IN);
+MS_PIN_DECL(F20, GPIOD2, SD2DAT0, GPID2IN);
 
-#define D21 27
+#define D20 27
 SIG_EXPR_LIST_DECL_SINGLE(SD2DAT1, SD2, SD2_DESC);
 SIG_EXPR_DECL(GPID2OUT, GPID2, GPID2_DESC);
 SIG_EXPR_DECL(GPID2OUT, GPID, GPID_DESC);
 SIG_EXPR_LIST_DECL_DUAL(GPID2OUT, GPID2, GPID);
-MS_PIN_DECL(D21, GPIOD3, SD2DAT1, GPID2OUT);
+MS_PIN_DECL(D20, GPIOD3, SD2DAT1, GPID2OUT);
 
-FUNC_GROUP_DECL(GPID2, D20, D21);
+FUNC_GROUP_DECL(GPID2, F20, D20);
 
 #define GPIE_DESC	SIG_DESC_SET(HW_STRAP1, 21)
 #define GPIE0_DESC	SIG_DESC_SET(SCU8C, 12)
@@ -614,7 +614,6 @@ static struct pinctrl_pin_desc aspeed_g5_pins[ASPEED_G5_NR_PINS] = {
 	ASPEED_PINCTRL_PIN(D10),
 	ASPEED_PINCTRL_PIN(D2),
 	ASPEED_PINCTRL_PIN(D20),
-	ASPEED_PINCTRL_PIN(D21),
 	ASPEED_PINCTRL_PIN(D4),
 	ASPEED_PINCTRL_PIN(D5),
 	ASPEED_PINCTRL_PIN(D6),
@@ -630,6 +629,7 @@ static struct pinctrl_pin_desc aspeed_g5_pins[ASPEED_G5_NR_PINS] = {
 	ASPEED_PINCTRL_PIN(E7),
 	ASPEED_PINCTRL_PIN(E9),
 	ASPEED_PINCTRL_PIN(F19),
+	ASPEED_PINCTRL_PIN(F20),
 	ASPEED_PINCTRL_PIN(F9),
 	ASPEED_PINCTRL_PIN(H20),
 	ASPEED_PINCTRL_PIN(L1),
-- 
git-series 0.8.10

[toc] | [prev] | [next] | [standalone]

#1498154 — Re: [PATCH 2/8] pinctrl: aspeed-g5: Fix names of GPID2 pins

On Tue, Sep 27, 2016 at 4:50 PM, Andrew Jeffery <andrew@aj.id.au> wrote:

> Fixes simple typos in the initial commit. There is no behavioural
> change.
>
> Fixes: 56e57cb6c07f (pinctrl: Add pinctrl-aspeed-g5 driver)
> Reported-by: Xo Wang <xow@google.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

Patch applied for fixes.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1491973 — [PATCH 5/8] pinctrl: aspeed: Enable capture of off-SCU pinmux state

The System Control Unit IP in the Aspeed SoCs is typically where the
pinmux configuration is found.

But not always.

On the AST2400 and AST2500 a number of pins depend on state in one of
the SIO, LPC or GFX IP blocks, so add support to at least capture what
that state is. The pinctrl engine for the Aspeed SoCs doesn't try to
inspect or modify the state of the off-SCU IP blocks. Instead, it logs
the state requirement with the expectation that the platform
designer/maintainer arranges for the appropriate configuration to be
applied through the associated drivers.

The IP block of interest is encoded in the reg member of struct
aspeed_sig_desc. For compatibility with the existing code, the SCU is
defined to have an IP value of 0.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/pinctrl/aspeed/pinctrl-aspeed.c | 53 +++++++++++++++++++++++---
 drivers/pinctrl/aspeed/pinctrl-aspeed.h | 16 +++++++-
 2 files changed, 61 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
index 49aeba912531..21ef195d586f 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
@@ -14,6 +14,8 @@
 #include "../core.h"
 #include "pinctrl-aspeed.h"
 
+const char *const aspeed_pinmux_ips[] = { "SCU", "SIO", "GFX", "LPC" };
+
 int aspeed_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
 {
 	struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
@@ -78,7 +80,9 @@ int aspeed_pinmux_get_fn_groups(struct pinctrl_dev *pctldev,
 static inline void aspeed_sig_desc_print_val(
 		const struct aspeed_sig_desc *desc, bool enable, u32 rv)
 {
-	pr_debug("SCU%x[0x%08x]=0x%x, got 0x%x from 0x%08x\n", desc->reg,
+	pr_debug("Want %s%lX[0x%08X]=0x%X, got 0x%X from 0x%08X\n",
+			aspeed_pinmux_ips[SIG_DESC_IP_FROM_REG(desc->reg)],
+			SIG_DESC_OFFSET_FROM_REG(desc->reg),
 			desc->mask, enable ? desc->enable : desc->disable,
 			(rv & desc->mask) >> __ffs(desc->mask), rv);
 }
@@ -105,6 +109,8 @@ static bool aspeed_sig_desc_eval(const struct aspeed_sig_desc *desc,
 	unsigned int raw;
 	u32 want;
 
+	WARN_ON(SIG_DESC_IP_FROM_REG(desc->reg) != ASPEED_IP_SCU);
+
 	if (regmap_read(map, desc->reg, &raw) < 0)
 		return false;
 
@@ -142,9 +148,19 @@ static bool aspeed_sig_expr_eval(const struct aspeed_sig_expr *expr,
 
 	for (i = 0; i < expr->ndescs; i++) {
 		const struct aspeed_sig_desc *desc = &expr->descs[i];
+		size_t ip = SIG_DESC_IP_FROM_REG(desc->reg);
+
+		if (ip == ASPEED_IP_SCU) {
+			if (!aspeed_sig_desc_eval(desc, enabled, map))
+				return false;
+		} else {
+			size_t offset = SIG_DESC_OFFSET_FROM_REG(desc->reg);
+			const char *ip_name = aspeed_pinmux_ips[ip];
+
+			pr_debug("Ignoring configuration of field %s%X[0x%08X]\n",
+				 ip_name, offset, desc->mask);
+		}
 
-		if (!aspeed_sig_desc_eval(desc, enabled, map))
-			return false;
 	}
 
 	return true;
@@ -170,7 +186,14 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
 	for (i = 0; i < expr->ndescs; i++) {
 		bool ret;
 		const struct aspeed_sig_desc *desc = &expr->descs[i];
+
+		size_t offset = SIG_DESC_OFFSET_FROM_REG(desc->reg);
+		size_t ip = SIG_DESC_IP_FROM_REG(desc->reg);
+		bool is_scu = (ip == ASPEED_IP_SCU);
+		const char *ip_name = aspeed_pinmux_ips[ip];
+
 		u32 pattern = enable ? desc->enable : desc->disable;
+		u32 val = (pattern << __ffs(desc->mask));
 
 		/*
 		 * Strap registers are configured in hardware or by early-boot
@@ -179,11 +202,27 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
 		 * deconfigured and is the reason we re-evaluate after writing
 		 * all descriptor bits.
 		 */
-		if (desc->reg == HW_STRAP1 || desc->reg == HW_STRAP2)
+		if (is_scu && (offset == HW_STRAP1 || offset == HW_STRAP2))
 			continue;
 
-		ret = regmap_update_bits(map, desc->reg, desc->mask,
-				pattern << __ffs(desc->mask)) == 0;
+		/*
+		 * Sometimes we need help from IP outside the SCU to activate a
+		 * mux request. Report that we need its cooperation.
+		 */
+		if (enable && !is_scu) {
+			pr_debug("Pinmux request for %s requires cooperation of %s IP: Need (%s%X[0x%08X] = 0x%08X\n",
+				expr->function, ip_name, ip_name, offset,
+				desc->mask, val);
+		}
+
+		/* And only read/write SCU registers */
+		if (!is_scu) {
+			pr_debug("Skipping configuration of field %s%X[0x%08X]\n",
+					ip_name, offset, desc->mask);
+			continue;
+		}
+
+		ret = regmap_update_bits(map, desc->reg, desc->mask, val) == 0;
 
 		if (!ret)
 			return ret;
@@ -343,6 +382,8 @@ int aspeed_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
 		const struct aspeed_sig_expr **funcs;
 		const struct aspeed_sig_expr ***prios;
 
+		pr_debug("Muxing pin %d for %s\n", pin, pfunc->name);
+
 		if (!pdesc)
 			return -EINVAL;
 
diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.h b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
index 3e72ef8c54bf..4384407d77fb 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed.h
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
@@ -232,6 +232,15 @@
  * group.
  */
 
+#define ASPEED_IP_SCU	0
+#define ASPEED_IP_SIO	1
+#define ASPEED_IP_GFX	2
+#define ASPEED_IP_LPC	3
+
+#define SIG_DESC_TO_REG(ip, offset)	(((ip) << 24) | (offset))
+#define SIG_DESC_IP_FROM_REG(reg)	(((reg) >> 24) & GENMASK(7, 0))
+#define SIG_DESC_OFFSET_FROM_REG(reg)	((reg) & GENMASK(11, 0))
+
 /*
  * The "Multi-function Pins Mapping and Control" table in the SoC datasheet
  * references registers by the device/offset mnemonic. The register macros
@@ -261,7 +270,10 @@
   * A signal descriptor, which describes the register, bits and the
   * enable/disable values that should be compared or written.
   *
-  * @reg: The register offset from base in bytes
+  * @reg: Split into three fields:
+  *       31:24: IP selector
+  *       23:12: Reserved
+  *       11:0: Register offset
   * @mask: The mask to apply to the register. The lowest set bit of the mask is
   *        used to derive the shift value.
   * @enable: The value that enables the function. Value should be in the LSBs,
@@ -270,7 +282,7 @@
   *           LSBs, not at the position of the mask.
   */
 struct aspeed_sig_desc {
-	unsigned int reg;
+	u32 reg;
 	u32 mask;
 	u32 enable;
 	u32 disable;
-- 
git-series 0.8.10

[toc] | [prev] | [next] | [standalone]

#1493166 — Re: [PATCH 5/8] pinctrl: aspeed: Enable capture of off-SCU pinmux state

On Wed, Sep 28, 2016 at 12:20 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> The System Control Unit IP in the Aspeed SoCs is typically where the
> pinmux configuration is found.
>
> But not always.
>
> On the AST2400 and AST2500 a number of pins depend on state in one of
> the SIO, LPC or GFX IP blocks, so add support to at least capture what
> that state is. The pinctrl engine for the Aspeed SoCs doesn't try to
> inspect or modify the state of the off-SCU IP blocks. Instead, it logs
> the state requirement with the expectation that the platform
> designer/maintainer arranges for the appropriate configuration to be
> applied through the associated drivers.

This is unfortunate.

This patch kicks the can down the road, but doesn't solve the problem
for a user who wants to configure some functionality that depends on
the non-SCU bits. Because of this I'm not sure if we want to put it in
the tree.

However, I'm not sure what a proper solution would look like. Perhaps
Linus can point out another SoC that has a similar problem?

Cheers,

Joel

>
> The IP block of interest is encoded in the reg member of struct
> aspeed_sig_desc. For compatibility with the existing code, the SCU is
> defined to have an IP value of 0.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/pinctrl/aspeed/pinctrl-aspeed.c | 53 +++++++++++++++++++++++---
>  drivers/pinctrl/aspeed/pinctrl-aspeed.h | 16 +++++++-
>  2 files changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> index 49aeba912531..21ef195d586f 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> @@ -14,6 +14,8 @@
>  #include "../core.h"
>  #include "pinctrl-aspeed.h"
>
> +const char *const aspeed_pinmux_ips[] = { "SCU", "SIO", "GFX", "LPC" };
> +
>  int aspeed_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
>  {
>         struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
> @@ -78,7 +80,9 @@ int aspeed_pinmux_get_fn_groups(struct pinctrl_dev *pctldev,
>  static inline void aspeed_sig_desc_print_val(
>                 const struct aspeed_sig_desc *desc, bool enable, u32 rv)
>  {
> -       pr_debug("SCU%x[0x%08x]=0x%x, got 0x%x from 0x%08x\n", desc->reg,
> +       pr_debug("Want %s%lX[0x%08X]=0x%X, got 0x%X from 0x%08X\n",
> +                       aspeed_pinmux_ips[SIG_DESC_IP_FROM_REG(desc->reg)],
> +                       SIG_DESC_OFFSET_FROM_REG(desc->reg),
>                         desc->mask, enable ? desc->enable : desc->disable,
>                         (rv & desc->mask) >> __ffs(desc->mask), rv);
>  }
> @@ -105,6 +109,8 @@ static bool aspeed_sig_desc_eval(const struct aspeed_sig_desc *desc,
>         unsigned int raw;
>         u32 want;
>
> +       WARN_ON(SIG_DESC_IP_FROM_REG(desc->reg) != ASPEED_IP_SCU);
> +
>         if (regmap_read(map, desc->reg, &raw) < 0)
>                 return false;
>
> @@ -142,9 +148,19 @@ static bool aspeed_sig_expr_eval(const struct aspeed_sig_expr *expr,
>
>         for (i = 0; i < expr->ndescs; i++) {
>                 const struct aspeed_sig_desc *desc = &expr->descs[i];
> +               size_t ip = SIG_DESC_IP_FROM_REG(desc->reg);
> +
> +               if (ip == ASPEED_IP_SCU) {
> +                       if (!aspeed_sig_desc_eval(desc, enabled, map))
> +                               return false;
> +               } else {
> +                       size_t offset = SIG_DESC_OFFSET_FROM_REG(desc->reg);
> +                       const char *ip_name = aspeed_pinmux_ips[ip];
> +
> +                       pr_debug("Ignoring configuration of field %s%X[0x%08X]\n",
> +                                ip_name, offset, desc->mask);
> +               }
>
> -               if (!aspeed_sig_desc_eval(desc, enabled, map))
> -                       return false;
>         }
>
>         return true;
> @@ -170,7 +186,14 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
>         for (i = 0; i < expr->ndescs; i++) {
>                 bool ret;
>                 const struct aspeed_sig_desc *desc = &expr->descs[i];
> +
> +               size_t offset = SIG_DESC_OFFSET_FROM_REG(desc->reg);
> +               size_t ip = SIG_DESC_IP_FROM_REG(desc->reg);
> +               bool is_scu = (ip == ASPEED_IP_SCU);
> +               const char *ip_name = aspeed_pinmux_ips[ip];
> +
>                 u32 pattern = enable ? desc->enable : desc->disable;
> +               u32 val = (pattern << __ffs(desc->mask));
>
>                 /*
>                  * Strap registers are configured in hardware or by early-boot
> @@ -179,11 +202,27 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
>                  * deconfigured and is the reason we re-evaluate after writing
>                  * all descriptor bits.
>                  */
> -               if (desc->reg == HW_STRAP1 || desc->reg == HW_STRAP2)
> +               if (is_scu && (offset == HW_STRAP1 || offset == HW_STRAP2))
>                         continue;
>
> -               ret = regmap_update_bits(map, desc->reg, desc->mask,
> -                               pattern << __ffs(desc->mask)) == 0;
> +               /*
> +                * Sometimes we need help from IP outside the SCU to activate a
> +                * mux request. Report that we need its cooperation.
> +                */
> +               if (enable && !is_scu) {
> +                       pr_debug("Pinmux request for %s requires cooperation of %s IP: Need (%s%X[0x%08X] = 0x%08X\n",
> +                               expr->function, ip_name, ip_name, offset,
> +                               desc->mask, val);
> +               }
> +
> +               /* And only read/write SCU registers */
> +               if (!is_scu) {
> +                       pr_debug("Skipping configuration of field %s%X[0x%08X]\n",
> +                                       ip_name, offset, desc->mask);
> +                       continue;
> +               }
> +
> +               ret = regmap_update_bits(map, desc->reg, desc->mask, val) == 0;
>
>                 if (!ret)
>                         return ret;
> @@ -343,6 +382,8 @@ int aspeed_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
>                 const struct aspeed_sig_expr **funcs;
>                 const struct aspeed_sig_expr ***prios;
>
> +               pr_debug("Muxing pin %d for %s\n", pin, pfunc->name);
> +
>                 if (!pdesc)
>                         return -EINVAL;
>
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.h b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> index 3e72ef8c54bf..4384407d77fb 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> @@ -232,6 +232,15 @@
>   * group.
>   */
>
> +#define ASPEED_IP_SCU  0
> +#define ASPEED_IP_SIO  1
> +#define ASPEED_IP_GFX  2
> +#define ASPEED_IP_LPC  3
> +
> +#define SIG_DESC_TO_REG(ip, offset)    (((ip) << 24) | (offset))
> +#define SIG_DESC_IP_FROM_REG(reg)      (((reg) >> 24) & GENMASK(7, 0))
> +#define SIG_DESC_OFFSET_FROM_REG(reg)  ((reg) & GENMASK(11, 0))
> +
>  /*
>   * The "Multi-function Pins Mapping and Control" table in the SoC datasheet
>   * references registers by the device/offset mnemonic. The register macros
> @@ -261,7 +270,10 @@
>    * A signal descriptor, which describes the register, bits and the
>    * enable/disable values that should be compared or written.
>    *
> -  * @reg: The register offset from base in bytes
> +  * @reg: Split into three fields:
> +  *       31:24: IP selector
> +  *       23:12: Reserved
> +  *       11:0: Register offset
>    * @mask: The mask to apply to the register. The lowest set bit of the mask is
>    *        used to derive the shift value.
>    * @enable: The value that enables the function. Value should be in the LSBs,
> @@ -270,7 +282,7 @@
>    *           LSBs, not at the position of the mask.
>    */
>  struct aspeed_sig_desc {
> -       unsigned int reg;
> +       u32 reg;
>         u32 mask;
>         u32 enable;
>         u32 disable;
> --
> git-series 0.8.10

[toc] | [prev] | [next] | [standalone]

#1493183 — Re: [PATCH 5/8] pinctrl: aspeed: Enable capture of off-SCU pinmux state

[Multipart message — attachments visible in raw view] — view raw

On Thu, 2016-09-29 at 16:15 +0930, Joel Stanley wrote:
> On Wed, Sep 28, 2016 at 12:20 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > 
> > The System Control Unit IP in the Aspeed SoCs is typically where the
> > pinmux configuration is found.
> > 
> > But not always.
> > 
> > On the AST2400 and AST2500 a number of pins depend on state in one of
> > the SIO, LPC or GFX IP blocks, so add support to at least capture what
> > that state is. The pinctrl engine for the Aspeed SoCs doesn't try to
> > inspect or modify the state of the off-SCU IP blocks. Instead, it logs
> > the state requirement with the expectation that the platform
> > designer/maintainer arranges for the appropriate configuration to be
> > applied through the associated drivers.
> This is unfortunate.
> 
> This patch kicks the can down the road, but doesn't solve the problem
> for a user who wants to configure some functionality that depends on
> the non-SCU bits. Because of this I'm not sure if we want to put it in
> the tree.

I agree that there's not much functionality from a user's perspective,
but the "kicking the can down the road" assessment might be a little
harsh. Given the lack of user functionality it becomes more difficult
to argue for the patch's inclusion given the additional complexity, but
it does mean that the g4/g5 drivers can completely specify their
dependencies and not have the aspeed pinctrl core do the wrong thing
when it encounters the non-SCU IP offsets. It gets us half-way to
having the pinctrl driver actually configure the state (knowing what it
needs to configure), which I feel is more than a kick-the-can-down-the-
road boondoggle.

> 
> However, I'm not sure what a proper solution would look like.

So if we accept that a proper solution includes specifying the off-SCU
dependencies, the remaining question is how do we tastefully apply the
desired state on register-spaces the pinctrl driver doesn't own.

>  Perhaps
> Linus can point out another SoC that has a similar problem?

Or failing that, an approach that is acceptable...

Cheers,

Andrew

[toc] | [prev] | [next] | [standalone]

#1493074 — Re: [PATCH 7/8] pinctrl: aspeed-g4: Add mux configuration for all pins

On Wed, Sep 28, 2016 at 12:20 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> The patch introducing the g4 pinctrl driver implemented a smattering of
> pins to flesh out the implementation of the core and provide bare-bones
> support for some OpenPOWER platforms. Now, update the bindings document
> to reflect the complete functionality and implement the necessary pin
> configuration tables in the driver.
>
> Cc: Timothy Pearson <tpearson@raptorengineering.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

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

[toc] | [prev] | [next] | [standalone]

#1495003 — Re: [PATCH 7/8] pinctrl: aspeed-g4: Add mux configuration for all pins

On Wed, Sep 28, 2016 at 12:20:19AM +0930, Andrew Jeffery wrote:
> The patch introducing the g4 pinctrl driver implemented a smattering of
> pins to flesh out the implementation of the core and provide bare-bones
> support for some OpenPOWER platforms. Now, update the bindings document
> to reflect the complete functionality and implement the necessary pin
> configuration tables in the driver.

We prefer bindings to be complete if possible where as drivers can be 
expanded over time.

> 
> Cc: Timothy Pearson <tpearson@raptorengineering.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt |   19 +-

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/pinctrl/aspeed/pinctrl-aspeed-g4.c                   | 1094 ++++++-
>  2 files changed, 1093 insertions(+), 20 deletions(-)

[toc] | [prev] | [next] | [standalone]

#1495129 — Re: [PATCH 7/8] pinctrl: aspeed-g4: Add mux configuration for all pins

[Multipart message — attachments visible in raw view] — view raw

On Mon, 2016-10-03 at 14:08 -0500, Rob Herring wrote:
> On Wed, Sep 28, 2016 at 12:20:19AM +0930, Andrew Jeffery wrote:
> > 
> > The patch introducing the g4 pinctrl driver implemented a smattering of
> > pins to flesh out the implementation of the core and provide bare-bones
> > support for some OpenPOWER platforms. Now, update the bindings document
> > to reflect the complete functionality and implement the necessary pin
> > configuration tables in the driver.
> We prefer bindings to be complete if possible where as drivers can be 
> expanded over time.

Noted.

> 
> > 
> > 
> > Cc: Timothy Pearson <tpearson@raptorengineering.com>
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt |   19 +-
> Acked-by: Rob Herring <robh@kernel.org>

Thanks,

Andrew

[toc] | [prev] | [next] | [standalone]

#1498051 — Re: [PATCH 8/8] pinctrl: aspeed-g5: Add mux configuration for all pins

On Wed, Sep 28, 2016 at 12:20:20AM +0930, Andrew Jeffery wrote:
> The patch introducing the g5 pinctrl driver implemented a smattering of
> pins to flesh out the implementation of the core and provide bare-bones
> support for some OpenPOWER platforms and the AST2500 evaluation board.
> Now, update the bindings document to reflect the complete functionality
> and implement the necessary pin configuration tables in the driver.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt |   17 +-

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c                   | 1476 ++++++-
>  drivers/pinctrl/aspeed/pinctrl-aspeed.h                      |    1 +-
>  3 files changed, 1487 insertions(+), 7 deletions(-)

[toc] | [prev] | [next] | [standalone]

#1498158 — Re: [PATCH 0/8] pinctrl: aspeed: Fixes for core and g5, implement remaining pins

On Tue, Sep 27, 2016 at 4:50 PM, Andrew Jeffery <andrew@aj.id.au> wrote:

> The initial Aspeed pinctrl patches implemented a subset of pins for each of the
> g4 and g5 SoCs. This series provides a number of fixes to the initial patches,
> mostly for issues identified in the g5 driver. The fixes account for the first
> half of the series (up to and including "pinctrl: aspeed-g5: Fix pin
> association of SPI1 function") and should be applied for 4.9.

Those are applied for fixes.

> The second half, from "pinctrl: aspeed: Enable capture of off-SCU pinmux
> state", implements some additional functionality in the core engine for the
> Aspeed SoCs and follows up with patches implementing mux configuration tables
> for all remaining pins. Given the significant additions in the last few
> patches, their lateness in the cycle and the light testing they have received
> they are best left for 4.10, but I'm keen to get them out for review.

I'm holding these back until v4.9-rc1 is out.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1498562 — Re: [PATCH 0/8] pinctrl: aspeed: Fixes for core and g5, implement remaining pins

[Multipart message — attachments visible in raw view] — view raw

On Mon, 2016-10-10 at 09:59 +0200, Linus Walleij wrote:
> On Tue, Sep 27, 2016 at 4:50 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> 
> > 
> > The initial Aspeed pinctrl patches implemented a subset of pins for each of the
> > g4 and g5 SoCs. This series provides a number of fixes to the initial patches,
> > mostly for issues identified in the g5 driver. The fixes account for the first
> > half of the series (up to and including "pinctrl: aspeed-g5: Fix pin
> > association of SPI1 function") and should be applied for 4.9.
> Those are applied for fixes.

Thanks!

> 
> > 
> > The second half, from "pinctrl: aspeed: Enable capture of off-SCU pinmux
> > state", implements some additional functionality in the core engine for the
> > Aspeed SoCs and follows up with patches implementing mux configuration tables
> > for all remaining pins. Given the significant additions in the last few
> > patches, their lateness in the cycle and the light testing they have received
> > they are best left for 4.10, but I'm keen to get them out for review.
> I'm holding these back until v4.9-rc1 is out.

No worries; they need some discussion when you have the time.

Cheers,

Andrew

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web