[RESEND PATCH linux] aspeed: pinctrl: Allow disabling Port D and Port E loopback mode

[RESEND PATCH linux] aspeed: pinctrl: Allow disabling Port D and Port E loopback mode

[Rick Altherr]

Port D and port E GPIO loopback modes are commonly enabled via hardware
straps for use with front-panel buttons.  When the BMC is powered
off or fails to boot, the front-panel buttons are directly connected to
the host chipset via the loopback to allow direct power-on and reset
control. Once the BMC has booted, the loopback mode must be disabled for
the BMC to take over control of host power-on and reset.

Disabling these loopback modes requires writing to the hardware strap
register which violates the current design of assuming the system
designer chose the strap settings for a specific reason and they should
be treated as read-only. Only the two bits of the strap register related
to these loopback modes are allowed to be written and comments have been
added to explain why.

Signed-off-by: Rick Altherr <raltherr@google.com>
---
 drivers/pinctrl/aspeed/pinctrl-aspeed.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
index 76f62bd45f02..5b49952e5fad 100644
--- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
+++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
@@ -198,9 +198,19 @@ static int aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
 		 * them. This may mean that certain functions cannot be
 		 * deconfigured and is the reason we re-evaluate after writing
 		 * all descriptor bits.
+		 *
+		 * Port D and port E GPIO loopback modes are the only exception
+		 * as those are commonly used with front-panel buttons to allow
+		 * normal operation of the host when the BMC is powered off or
+		 * fails to boot. Once the BMC has booted, the loopback mode
+		 * must be disabled for the BMC to control host power-on and
+		 * reset.
 		 */
-		if ((desc->reg == HW_STRAP1 || desc->reg == HW_STRAP2) &&
-				desc->ip == ASPEED_IP_SCU)
+		if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP1 &&
+		    !(desc->mask & (BIT(21) | BIT(22))))
+			continue;
+
+		if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP2)
 			continue;
 
 		ret = regmap_update_bits(maps[desc->ip], desc->reg,
-- 
2.11.0.483.g087da7b7c-goog

Re: [RESEND PATCH linux] aspeed: pinctrl: Allow disabling Port D and Port E loopback mode

[Joel Stanley]

On Wed, Feb 15, 2017 at 5:25 PM, Rick Altherr <raltherr@google.com> wrote:
> Port D and port E GPIO loopback modes are commonly enabled via hardware
> straps for use with front-panel buttons.  When the BMC is powered
> off or fails to boot, the front-panel buttons are directly connected to
> the host chipset via the loopback to allow direct power-on and reset
> control. Once the BMC has booted, the loopback mode must be disabled for
> the BMC to take over control of host power-on and reset.
>
> Disabling these loopback modes requires writing to the hardware strap
> register which violates the current design of assuming the system
> designer chose the strap settings for a specific reason and they should
> be treated as read-only. Only the two bits of the strap register related
> to these loopback modes are allowed to be written and comments have been
> added to explain why.

I had to clarify this on IRC. The Aspeed soc is flexible if you want
no-passthrough when powered off and want to enable it at  runtime with
normal SCU registers.
However if you want passthrough when powered off, and then disable it
at runtime, you need to change the strap. Hence the patch.

> Signed-off-by: Rick Altherr <raltherr@google.com>

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

Cheers,

Joel

> ---
>  drivers/pinctrl/aspeed/pinctrl-aspeed.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> index 76f62bd45f02..5b49952e5fad 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> @@ -198,9 +198,19 @@ static int aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
>                  * them. This may mean that certain functions cannot be
>                  * deconfigured and is the reason we re-evaluate after writing
>                  * all descriptor bits.
> +                *
> +                * Port D and port E GPIO loopback modes are the only exception
> +                * as those are commonly used with front-panel buttons to allow
> +                * normal operation of the host when the BMC is powered off or
> +                * fails to boot. Once the BMC has booted, the loopback mode
> +                * must be disabled for the BMC to control host power-on and
> +                * reset.
>                  */
> -               if ((desc->reg == HW_STRAP1 || desc->reg == HW_STRAP2) &&
> -                               desc->ip == ASPEED_IP_SCU)
> +               if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP1 &&
> +                   !(desc->mask & (BIT(21) | BIT(22))))
> +                       continue;
> +
> +               if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP2)
>                         continue;
>
>                 ret = regmap_update_bits(maps[desc->ip], desc->reg,
> --
> 2.11.0.483.g087da7b7c-goog
>

Re: [RESEND PATCH linux] aspeed: pinctrl: Allow disabling Port D and Port E loopback mode

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2845 bytes --]

On Tue, 2017-02-14 at 22:55 -0800, Rick Altherr wrote:
> Port D and port E GPIO loopback modes are commonly enabled via hardware
> straps for use with front-panel buttons.  When the BMC is powered
> off or fails to boot, the front-panel buttons are directly connected to
> the host chipset via the loopback to allow direct power-on and reset
> control. Once the BMC has booted, the loopback mode must be disabled for
> the BMC to take over control of host power-on and reset.
> 
> Disabling these loopback modes requires writing to the hardware strap
> register which violates the current design of assuming the system
> designer chose the strap settings for a specific reason and they should
> be treated as read-only. Only the two bits of the strap register related
> to these loopback modes are allowed to be written and comments have been
> added to explain why.
> 
> Signed-off-by: Rick Altherr <raltherr@google.com>

For the record this is one of at least two awkward cases. The other is
runtime switching between the using the SoC as an SPI master, and
simply performing SPI pass-through for an external controller (e.g. to
allow either the BMC or the host to reprogram the host BIOS image).

If the list of exceptions continues to grow we should pull the tests
out to a separate function so we can keep the condition readable.

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> ---
>  drivers/pinctrl/aspeed/pinctrl-aspeed.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> index 76f62bd45f02..5b49952e5fad 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> @@ -198,9 +198,19 @@ static int aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
> >  		 * them. This may mean that certain functions cannot be
> >  		 * deconfigured and is the reason we re-evaluate after writing
> >  		 * all descriptor bits.
> > +		 *
> > +		 * Port D and port E GPIO loopback modes are the only exception
> > +		 * as those are commonly used with front-panel buttons to allow
> > +		 * normal operation of the host when the BMC is powered off or
> > +		 * fails to boot. Once the BMC has booted, the loopback mode
> > +		 * must be disabled for the BMC to control host power-on and
> > +		 * reset.
> >  		 */
> > -		if ((desc->reg == HW_STRAP1 || desc->reg == HW_STRAP2) &&
> > -				desc->ip == ASPEED_IP_SCU)
> > +		if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP1 &&
> > +		    !(desc->mask & (BIT(21) | BIT(22))))
> > +			continue;
> +
> > +		if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP2)
> >  			continue;
>  
> >  		ret = regmap_update_bits(maps[desc->ip], desc->reg,

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]