[PATCH linux dev-4.7 v1 1/1] pinctrl: aspeed: Enable ASPEED pinctrl driver to modify SPI1 strap bits

[Andrew Jeffery]

Hi Kun,

On Wed, 2016-11-02 at 15:39 -0700, Kun Yi wrote:
> Current pinctrl driver treats HWSTRAP1/HWSTRAP2 bits as read-only. Changing
> SPI1 mode requires pinctrl to modify bit 12 and 13 of HWSTRAP1, and keep
> other strap bits as read-only.
> 
> Note: with this change devicetree default pinctrl settings for spi1 will
> effectively override any previous mode set by physical strapping or
> aspeed.c when Aspeed smc driver is probed.
> 
> Signed-off-by: Kun Yi <kunyi at google.com>
> ---
>  drivers/pinctrl/aspeed/pinctrl-aspeed.c | 22 ++++++++++++++++++++--
>  drivers/pinctrl/aspeed/pinctrl-aspeed.h |  2 +-
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> index 21ef195..c647503 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> @@ -16,6 +16,8 @@
>  
>  const char *const aspeed_pinmux_ips[] = { "SCU", "SIO", "GFX", "LPC" };
>  
> +#define SPI1_REG_MASK   0x3000
> +
>  int aspeed_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
>  {
>  	struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
> @@ -178,6 +180,7 @@ static bool aspeed_sig_expr_eval(const struct aspeed_sig_expr *expr,
>   *
>   * @return true if the expression is configured as requested, false otherwise
>   */
> +
>  static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
>  				bool enable, struct regmap *map)
>  {
> @@ -197,15 +200,30 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
>  
>  		/*
>  		 * Strap registers are configured in hardware or by early-boot
> -		 * firmware. Treat them as read-only despite that we can write
> +		 * firmware. With the exception of SPI1 interface bits, treat
> +		 * them as read-only despite that we can write
>  		 * them. This may mean that certain functions cannot be
>  		 * deconfigured and is the reason we re-evaluate after writing
>  		 * all descriptor bits.
>  		 */
> -		if (is_scu && (offset == HW_STRAP1 || offset == HW_STRAP2))
> +		if (is_scu && (offset == HW_STRAP2 ||
> +			offset == HW_STRAP1 && desc->mask != SPI1_REG_MASK))

Maybe the mask comparison would be safer by rewriting as:


> +		if (is_scu && (offset == HW_STRAP2 ||
> +			offset == HW_STRAP1 && !(desc->mask & SPI1_REG_MASK)))

That way if (for whatever reason) we change the approach used in the
-g4 and -g5 implementations we still get the expected outcome.


>  			continue;
>  
>  		/*
> +		 * HW_STRAP1 bits can only be set to 0 by writing 1 into
> +		 * bits of same offset in SCU7C. To configure different SPI1
> +		 * modes, we write 1 to SCU7C[13:12] to clear SPI1 mask to make
> +		 * sure later write to strap register can take effect.
> +		 */
> +		if (is_scu && offset == HW_STRAP1 &&
> +		    desc->mask == SPI1_REG_MASK) {
> +			ret = regmap_write(map, HW_STRAP1_RESET, desc->mask);
> +			if (ret)
> +				return false;
> +		};
> +
> +		/*
>  		 * Sometimes we need help from IP outside the SCU to activate a
>  		 * mux request. Report that we need its cooperation.
>  		 */
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.h b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> index 68315a8..46ba16d 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> @@ -268,6 +268,7 @@
>  #define HW_STRAP2       0xD0 /* Strapping */
>  
>  #define SIORD30		SIG_DESC_TO_REG(ASPEED_IP_SIO, 0x30)
> +#define HW_STRAP1_RESET	SIG_DESC_TO_REG(ASPEED_IP_SCU, 0x7C)

Maybe HW_STRAP1_CLEAR would be ... clearer? Also in this case the
SIG_DESC_TO_REG() macro is not entirely necessary as we're talking
about the SCU, but being explicit is good. As a note the macro(s) have
gone away in the v2 of the series I've recently sent upstream due to
feedback from the maintainer:

https://www.spinics.net/lists/arm-kernel/msg540082.html

Otherwise this looks good to me. Thanks for looking at the problem!

Andrew

>  
>   /**
>    * A signal descriptor, which describes the register, bits and the
> @@ -580,5 +581,4 @@ int aspeed_gpio_request_enable(struct pinctrl_dev *pctldev,
>  int aspeed_pinctrl_probe(struct platform_device *pdev,
>  		struct pinctrl_desc *pdesc,
>  		struct aspeed_pinctrl_data *pdata);
> -
>  #endif /* PINCTRL_ASPEED */
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20161103/0b15e79b/attachment.sig>