| 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
| ||
|
Message-ID: <D35132A5-2676-4806-AF35-9110DF0CC88B@aspeedtech.com>
Date: Mon, 12 Oct 2020 05:25:29 +0000
From: Billy Tsai <billy_tsai@...eedtech.com>
To: Joel Stanley <joel@....id.au>, Jeremy Kerr <jk@...abs.org>,
Andrew Jeffery <andrew@...id.au>
CC: Rob Herring <robh+dt@...nel.org>,
devicetree <devicetree@...r.kernel.org>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
linux-aspeed <linux-aspeed@...ts.ozlabs.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] Arm: dts: aspeed-g6: Add sgpio node and pinctrl
setting
Hi Joel,
On 2020/10/8, 11:49 AM, Joel Stanley wrote:
On Thu, 8 Oct 2020 at 01:51, Billy Tsai <billy_tsai@...eedtech.com> wrote:
> >
> > This patch is used to add sgpiom and sgpios nodes and add pinctrl setting
> > for sgpiom1
>
> The code looks good Billy.
>
> Please split the change in two: device tree changes (arch/arm/dts) in
> one, and pinctrl in the second, as they go through different
> maintainers.
>
If I split the change in two, the patch of dts will have a compiler error.
Because that the sgpiom1 node needs the pinctrl symbol "&pinctrl_sgpm2_default".
> You also need to update the device tree bindings in Documentation with
> the new compatible strings:
>
> Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
>
> That should go in it's own patch too.
>
> > --- a/arch/arm/boot/dts/aspeed-g6.dtsi
> > +++ b/arch/arm/boot/dts/aspeed-g6.dtsi
> > @@ -366,6 +366,58 @@
> > #interrupt-cells = <2>;
> > };
> >
> > + sgpiom0: sgpiom@...80500 {
> > + #gpio-cells = <2>;
> > + gpio-controller;
> > + compatible = "aspeed,ast2600-sgpiom";
>
> This is interesting. I didn't realise the sgpio driver we have in the
> mainline kernel tree (drivers/gpio/gpio-aspeed-sgpio.c) is for the
> sgpio master device. It might be best to update the naming of the
> ast2400/ast2500 compatible in the future.
>
> > + reg = <0x1e780500 0x100>;
> > + interrupts = <GIC_SPI 51 IRQ_TYPE_LEVEL_HIGH>;
> > + ngpios = <128>;
> > + clocks = <&syscon ASPEED_CLK_APB2>;
> > + interrupt-controller;
> > + bus-frequency = <12000000>;
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_sgpm1_default>;
> > + status = "disabled";
> > + };
>
> > gpio1: gpio@...80800 {
> > #gpio-cells = <2>;
> > gpio-controller;
> > @@ -377,6 +429,7 @@
> > clocks = <&syscon ASPEED_CLK_APB1>;
> > interrupt-controller;
> > #interrupt-cells = <2>;
> > + status = "disabled";
>
> This should be in a different patch set, as it will break all of the
> systems that expect GPIO to be enabled (which is all of them).
>
> Considering all of them expect this gpio bank to be enabled, should we
> leave it enabled here?
>
>
> > };
> >
> > rtc: rtc@...81000 {
> > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> > index 34803a6c7664..b673a44ffa3b 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 #28 */
> > #define SCUC20 0xC20 /* PCIE configuration Setting Control */
> >
> > #define ASPEED_G6_NR_PINS 256
> > @@ -81,13 +83,21 @@ 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);
> > +/*SGPM2 is A1 Only */
> > +SIG_EXPR_LIST_DECL_SESG(K26, SGPM2CLK, SGPM2, SIG_DESC_SET(SCU6D0, 4),
> > + SIG_DESC_CLEAR(SCU410, 4), SIG_DESC_CLEAR(SCU4B0, 4),
> > + SIG_DESC_CLEAR(SCU690, 4));
> > +PIN_DECL_3(K26, GPIOA4, SGPM2CLK, MACLINK1, SCL13);
> > FUNC_GROUP_DECL(MACLINK1, K26);
> >
> > #define L24 5
> > SIG_EXPR_LIST_DECL_SESG(L24, MACLINK2, MACLINK2, SIG_DESC_SET(SCU410, 5));
> > SIG_EXPR_LIST_DECL_SESG(L24, SDA13, I2C13, SIG_DESC_SET(SCU4B0, 5));
> > -PIN_DECL_2(L24, GPIOA5, MACLINK2, SDA13);
> > +/*SGPM2 is A1 Only */
> > +SIG_EXPR_LIST_DECL_SESG(L24, SGPM2LD, SGPM2, SIG_DESC_SET(SCU6D0, 5),
> > + SIG_DESC_CLEAR(SCU410, 5), SIG_DESC_CLEAR(SCU4B0, 5),
> > + SIG_DESC_CLEAR(SCU690, 5));
> > +PIN_DECL_3(L24, GPIOA5, SGPM2LD, MACLINK2, SDA13);
> > FUNC_GROUP_DECL(MACLINK2, L24);
> >
> > FUNC_GROUP_DECL(I2C13, K26, L24);
> > @@ -95,16 +105,26 @@ FUNC_GROUP_DECL(I2C13, K26, L24);
> > #define L23 6
> > SIG_EXPR_LIST_DECL_SESG(L23, MACLINK3, MACLINK3, SIG_DESC_SET(SCU410, 6));
> > SIG_EXPR_LIST_DECL_SESG(L23, SCL14, I2C14, SIG_DESC_SET(SCU4B0, 6));
> > -PIN_DECL_2(L23, GPIOA6, MACLINK3, SCL14);
> > +/*SGPM2 is A1 Only */
> > +SIG_EXPR_LIST_DECL_SESG(L23, SGPM2O, SGPM2, SIG_DESC_SET(SCU6D0, 6),
> > + SIG_DESC_CLEAR(SCU410, 6), SIG_DESC_CLEAR(SCU4B0, 6),
> > + SIG_DESC_CLEAR(SCU690, 6));
> > +PIN_DECL_3(L23, GPIOA6, SGPM2O, MACLINK3, SCL14);
> > FUNC_GROUP_DECL(MACLINK3, L23);
> >
> > #define K25 7
> > SIG_EXPR_LIST_DECL_SESG(K25, MACLINK4, MACLINK4, SIG_DESC_SET(SCU410, 7));
> > SIG_EXPR_LIST_DECL_SESG(K25, SDA14, I2C14, SIG_DESC_SET(SCU4B0, 7));
> > -PIN_DECL_2(K25, GPIOA7, MACLINK4, SDA14);
> > +/*SGPM2 is A1 Only */
> > +SIG_EXPR_LIST_DECL_SESG(K25, SGPM2I, SGPM2, SIG_DESC_SET(SCU6D0, 7),
> > + SIG_DESC_CLEAR(SCU410, 7), SIG_DESC_CLEAR(SCU4B0, 7),
> > + SIG_DESC_CLEAR(SCU690, 7));
> > +PIN_DECL_3(K25, GPIOA7, SGPM2I, MACLINK4, SDA14);
> > FUNC_GROUP_DECL(MACLINK4, K25);
> >
> > FUNC_GROUP_DECL(I2C14, L23, K25);
> > +/*SGPM2 is A1 Only */
> > +FUNC_GROUP_DECL(SGPM2, K26, L24, L23, K25);
> >
> > #define J26 8
> > SIG_EXPR_LIST_DECL_SESG(J26, SALT1, SALT1, SIG_DESC_SET(SCU410, 8));
> > @@ -2060,6 +2080,7 @@ static const struct aspeed_pin_group aspeed_g6_groups[] = {
> > ASPEED_PINCTRL_GROUP(EMMCG4),
> > ASPEED_PINCTRL_GROUP(EMMCG8),
> > ASPEED_PINCTRL_GROUP(SGPM1),
> > + ASPEED_PINCTRL_GROUP(SGPM2),
> > ASPEED_PINCTRL_GROUP(SGPS1),
> > ASPEED_PINCTRL_GROUP(SIOONCTRL),
> > ASPEED_PINCTRL_GROUP(SIOPBI),
> > @@ -2276,6 +2297,7 @@ static const struct aspeed_pin_function aspeed_g6_functions[] = {
> > ASPEED_PINCTRL_FUNC(SD1),
> > ASPEED_PINCTRL_FUNC(SD2),
> > ASPEED_PINCTRL_FUNC(SGPM1),
> > + ASPEED_PINCTRL_FUNC(SGPM2),
> > ASPEED_PINCTRL_FUNC(SGPS1),
> > ASPEED_PINCTRL_FUNC(SIOONCTRL),
> > ASPEED_PINCTRL_FUNC(SIOPBI),
> > --
> > 2.17.1
> >
Powered by blists - more mailing lists