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

Andrew Jeffery andrew at aj.id.au
Fri Nov 4 12:18:14 AEDT 2016


On Thu, 2016-11-03 at 13:54 -0700, Xo Wang wrote:
> On Wed, Nov 2, 2016 at 9:06 PM, Andrew Jeffery <andrew at aj.id.au> wrote:
> > 
> > On Wed, 2016-11-02 at 20:47 -0700, Kun Yi wrote:
> > > 
> > > Thanks for reviewing, Andrew!
> > No worries!
> > 
> > > 
> > > 
> > > On Wed, Nov 2, 2016 at 6:33 PM, Andrew Jeffery <andrew at aj.id.au> wrote:
> > > > 
> > > > 
> > > > 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.
> > > > 
> > > > 
> > > Good point. I will change this and the next mask comparisons in case
> > > only one of strap bit 12/13 is used as desc->mask.
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > >                       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
> > > > 
> > > HW_STRAP1_CLEAR does sound better, will rename in v2.
> > > Since SIG_DESC_TO_REG() is removed in your v2 patch, should I directly
> > > define it as this?
> > > 
> > > #define HW_STRAP1_CLEAR      0x7C
> > Yes, that would then work in either case (with or without being applied
> > atop v2).
> > 
> > Thanks,
> > 
> > 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 */
> > > 
> Since the SPI1 strap bits will be mutable by the pinctrl driver,
> should we take the hard-coding out of the mach-aspeed AST2500 fixup?
> 
> xo

Yep, I've replied as such to Joel's patch[1].

Cheers,

Andrew

[1] https://lists.ozlabs.org/pipermail/openbmc/2016-November/005419.htm
l
-------------- 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/20161104/37e28f9c/attachment-0001.sig>


More information about the openbmc mailing list