linux-kernel - Re: [PATCH v1 3/3] pinctrl: pinctrl-aspeed-g6: Add sgpio pinctrl settings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <b38cf7e0-6cce-4cb3-8bb7-d711b1fe6eb2@www.fastmail.com>
Date:   Tue, 25 May 2021 13:12:59 +0930
From:   "Andrew Jeffery" <andrew@...id.au>
To:     "Steven Lee" <steven_lee@...eedtech.com>
Cc:     "Linus Walleij" <linus.walleij@...aro.org>,
        "Rob Herring" <robh+dt@...nel.org>,
        "Joel Stanley" <joel@....id.au>,
        "moderated list:ASPEED PINCTRL DRIVERS" 
        <linux-aspeed@...ts.ozlabs.org>,
        "moderated list:ASPEED PINCTRL DRIVERS" <openbmc@...ts.ozlabs.org>,
        "open list:ASPEED PINCTRL DRIVERS" <linux-gpio@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "moderated list:ARM/ASPEED MACHINE SUPPORT" 
        <linux-arm-kernel@...ts.infradead.org>,
        "open list" <linux-kernel@...r.kernel.org>,
        "Hongwei Zhang" <Hongweiz@....com>,
        "Ryan Chen" <ryan_chen@...eedtech.com>,
        "Billy Tsai" <billy_tsai@...eedtech.com>
Subject: Re: [PATCH v1 3/3] pinctrl: pinctrl-aspeed-g6: Add sgpio pinctrl settings



On Tue, 25 May 2021, at 12:32, Steven Lee wrote:
> The 05/25/2021 08:54, Andrew Jeffery wrote:
> > Hi Steven,
> > 
> > On Mon, 24 May 2021, at 20:43, Steven Lee wrote:
> > > AST2600 supports 2 SGPIO master interfaces and 2 SGPIO slave interfaces.
> > > Current pinctrl driver only define the first sgpio master and slave
> > > interfaces.
> > > The sencond SGPIO master and slave interfaces should be added in
> > > pinctrl driver as well.
> > > 
> > > Signed-off-by: Steven Lee <steven_lee@...eedtech.com>
> > > ---
> > >  drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 24 ++++++++++++++++++----
> > >  drivers/pinctrl/aspeed/pinmux-aspeed.h     |  9 ++++++++
> > >  2 files changed, 29 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c 
> > > b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> > > index 5c1a109842a7..d0e9ab9d1a9c 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 #29 */
> > >  #define SCUC20		0xC20 /* PCIE configuration Setting Control */
> > >  
> > >  #define ASPEED_G6_NR_PINS 256
> > > @@ -81,13 +83,17 @@ 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);
> > > +SIG_EXPR_LIST_DECL_SESG(K26, SGPS2CK, SGPS2, SIG_DESC_SET(SCU690, 4));
> > > +SIG_EXPR_LIST_DECL_SESG(K26, SGPM2CLK, SGPM2, SIG_DESC_SET(SCU6D0, 4));
> > > +PIN_DECL_4(K26, GPIOA4, SGPM2CLK, SGPS2CK, MACLINK1, SCL13);
> > 
> > Is this the right priority order? Looking at the Multi-Function Pin 
> > Mapping and Control table, function 1 is MACLINK1,
> > function 2 is SCL13, function 3 is SGPS2CK, and I assume function 4 
> > would be SGPM2CLK, except it's not documented in the table in v9 of the 
> > datasheet (I hope it will be documented?).
> > 
> > If function 1 is the highest priority (which is what all the Aspeed 
> > pinctrl drivers assume), then this should be:
> > 
> > PIN_DECL_4(K26, GPIOA4, MACLINK1, SCL13, SGPS2CK, SGPM2CLK);
> > 
> > Anyway, one of several things could be at fault here:
> > 
> > 1. I've made a wrong assumption about the priority order in how I've 
> > implemented pinctrl support for Aspeed SoCs
> > 
> > 2. The Multi-Function Pin Mapping and Control table is out of date and 
> > needs to be fixed (which it already does as it doesn't list SGPM2CLK).
> > 
> > 3. The patch needs to align with the assumptions of the Aspeed pinctrl 
> > support.
> > 
> > I don't think it's 1 as I haven't heard of any issues where we are 
> > getting incorrect behaviour because of pinmux. I don't think it's 2 as 
> > this patch makes a non-linear change to the ordering. So my hunch is
> > the issue is 3, that the patch is putting the signals in the wrong order.
> > In this case, you want the PIN_DECL_4(...) I outlined above.
> > 
> 
> Yes, you are right. Per discussion with the designer, the priority order is
> MACLINK1 > SCL13 > SGPS2CK > SGPM2CLK
> 
> We will add the information in the v10 datasheet.

Great. Thanks Steven.

Andrew