[PATCH 0/3] simplify Aspeed VUART SIRQ polarity DT config, add e3c246d4i BMC dts

[PATCH 0/3] simplify Aspeed VUART SIRQ polarity DT config, add e3c246d4i BMC dts

[Zev Weiss]

This patch series reworks the existing aspeed,sirq-polarity-sense DT
property as a simple boolean (aspeed,sirq-active-high) so as to
disentangle the interrupt polarity from the eSPI/LPC strapping and
updates the documentation accordingly.

The third patch adds an in-tree consumer of this property
(aspeed,sirq-polarity-sense never really had one) in the form of a
device tree for the ASRock E3C246D4I BMC, an OpenBMC target in
progress at Equinix Metal (formerly known as Packet).


Zev Weiss (3):
  drivers/tty/serial/8250: simplify Aspeed VUART SIRQ polarity DT config
  dt-bindings: serial: 8250: update for aspeed,sirq-active-high
  ARM: dts: aspeed: add ASRock E3C246D4I BMC

 .../devicetree/bindings/serial/8250.yaml      |  14 +-
 .../boot/dts/aspeed-bmc-asrock-e3c246d4i.dts  | 188 ++++++++++++++++++
 drivers/tty/serial/8250/8250_aspeed_vuart.c   |  39 +---
 drivers/tty/serial/8250/Kconfig               |   1 -
 4 files changed, 196 insertions(+), 46 deletions(-)
 create mode 100644 arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts

-- 
2.31.1

[PATCH 1/3] drivers/tty/serial/8250: simplify Aspeed VUART SIRQ polarity DT config

[Zev Weiss]

The initial implementation of this configuration conflated the SIRQ
polarity setting with the syscon eSPI/LPC strapping; this patch
disentangles them by reducing the DT config to a simple boolean.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 39 ++-------------------
 drivers/tty/serial/8250/Kconfig             |  1 -
 2 files changed, 2 insertions(+), 38 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index c33e02cbde93..b9b5fa58ab28 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -10,8 +10,6 @@
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
-#include <linux/regmap.h>
-#include <linux/mfd/syscon.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
 #include <linux/clk.h>
@@ -346,30 +344,8 @@ static int aspeed_vuart_handle_irq(struct uart_port *port)
 	return 1;
 }
 
-static void aspeed_vuart_auto_configure_sirq_polarity(
-	struct aspeed_vuart *vuart, struct device_node *syscon_np,
-	u32 reg_offset, u32 reg_mask)
-{
-	struct regmap *regmap;
-	u32 value;
-
-	regmap = syscon_node_to_regmap(syscon_np);
-	if (IS_ERR(regmap)) {
-		dev_warn(vuart->dev,
-			 "could not get regmap for aspeed,sirq-polarity-sense\n");
-		return;
-	}
-	if (regmap_read(regmap, reg_offset, &value)) {
-		dev_warn(vuart->dev, "could not read hw strap table\n");
-		return;
-	}
-
-	aspeed_vuart_set_sirq_polarity(vuart, (value & reg_mask) == 0);
-}
-
 static int aspeed_vuart_probe(struct platform_device *pdev)
 {
-	struct of_phandle_args sirq_polarity_sense_args;
 	struct uart_8250_port port;
 	struct aspeed_vuart *vuart;
 	struct device_node *np;
@@ -468,19 +444,8 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 
 	vuart->line = rc;
 
-	rc = of_parse_phandle_with_fixed_args(
-		np, "aspeed,sirq-polarity-sense", 2, 0,
-		&sirq_polarity_sense_args);
-	if (rc < 0) {
-		dev_dbg(&pdev->dev,
-			"aspeed,sirq-polarity-sense property not found\n");
-	} else {
-		aspeed_vuart_auto_configure_sirq_polarity(
-			vuart, sirq_polarity_sense_args.np,
-			sirq_polarity_sense_args.args[0],
-			BIT(sirq_polarity_sense_args.args[1]));
-		of_node_put(sirq_polarity_sense_args.np);
-	}
+	if (of_property_read_bool(np, "aspeed,sirq-active-high"))
+		aspeed_vuart_set_sirq_polarity(vuart, 1);
 
 	aspeed_vuart_set_enabled(vuart, true);
 	aspeed_vuart_set_host_tx_discard(vuart, true);
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 603137da4736..105a325bcdd1 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -254,7 +254,6 @@ config SERIAL_8250_ASPEED_VUART
 	tristate "Aspeed Virtual UART"
 	depends on SERIAL_8250
 	depends on OF
-	depends on REGMAP && MFD_SYSCON
 	help
 	  If you want to use the virtual UART (VUART) device on Aspeed
 	  BMC platforms, enable this option. This enables the 16550A-
-- 
2.31.1

[PATCH 2/3] dt-bindings: serial: 8250: update for aspeed, sirq-active-high

[Zev Weiss]

Update DT bindings documentation for the new incarnation of the
aspeed,sirq-polarity-sense property.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 Documentation/devicetree/bindings/serial/8250.yaml | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
index f54cae9ff7b2..0bbb7121f720 100644
--- a/Documentation/devicetree/bindings/serial/8250.yaml
+++ b/Documentation/devicetree/bindings/serial/8250.yaml
@@ -13,7 +13,7 @@ allOf:
   - $ref: /schemas/serial.yaml#
   - if:
       required:
-        - aspeed,sirq-polarity-sense
+        - aspeed,sirq-active-high
     then:
       properties:
         compatible:
@@ -181,13 +181,11 @@ properties:
   rng-gpios: true
   dcd-gpios: true
 
-  aspeed,sirq-polarity-sense:
-    $ref: /schemas/types.yaml#/definitions/phandle-array
+  aspeed,sirq-active-high:
+    type: boolean
     description: |
-      Phandle to aspeed,ast2500-scu compatible syscon alongside register
-      offset and bit number to identify how the SIRQ polarity should be
-      configured. One possible data source is the LPC/eSPI mode bit. Only
-      applicable to aspeed,ast2500-vuart.
+      Set to indicate that the SIRQ polarity is active-high (default
+      is active-low).  Only applicable to aspeed,ast2500-vuart.
 
 required:
   - reg
@@ -227,7 +225,7 @@ examples:
         interrupts = <8>;
         clocks = <&syscon ASPEED_CLK_APB>;
         no-loopback-test;
-        aspeed,sirq-polarity-sense = <&syscon 0x70 25>;
+        aspeed,sirq-active-high;
     };
 
 ...
-- 
2.31.1

Re: [PATCH 2/3] dt-bindings: serial: 8250: update for aspeed,sirq-active-high

[Rob Herring]

On Mon, Mar 29, 2021 at 07:23:37PM -0500, Zev Weiss wrote:
> Update DT bindings documentation for the new incarnation of the
> aspeed,sirq-polarity-sense property.

Why?

This isn't a compatible change.

> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  Documentation/devicetree/bindings/serial/8250.yaml | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> index f54cae9ff7b2..0bbb7121f720 100644
> --- a/Documentation/devicetree/bindings/serial/8250.yaml
> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> @@ -13,7 +13,7 @@ allOf:
>    - $ref: /schemas/serial.yaml#
>    - if:
>        required:
> -        - aspeed,sirq-polarity-sense
> +        - aspeed,sirq-active-high
>      then:
>        properties:
>          compatible:
> @@ -181,13 +181,11 @@ properties:
>    rng-gpios: true
>    dcd-gpios: true
>  
> -  aspeed,sirq-polarity-sense:
> -    $ref: /schemas/types.yaml#/definitions/phandle-array
> +  aspeed,sirq-active-high:
> +    type: boolean
>      description: |
> -      Phandle to aspeed,ast2500-scu compatible syscon alongside register
> -      offset and bit number to identify how the SIRQ polarity should be
> -      configured. One possible data source is the LPC/eSPI mode bit. Only
> -      applicable to aspeed,ast2500-vuart.
> +      Set to indicate that the SIRQ polarity is active-high (default
> +      is active-low).  Only applicable to aspeed,ast2500-vuart.
>  
>  required:
>    - reg
> @@ -227,7 +225,7 @@ examples:
>          interrupts = <8>;
>          clocks = <&syscon ASPEED_CLK_APB>;
>          no-loopback-test;
> -        aspeed,sirq-polarity-sense = <&syscon 0x70 25>;
> +        aspeed,sirq-active-high;
>      };
>  
>  ...
> -- 
> 2.31.1
> 

Re: [PATCH 2/3] dt-bindings: serial: 8250: update for aspeed,sirq-active-high

[Zev Weiss]

On Tue, Mar 30, 2021 at 05:39:02PM CDT, Rob Herring wrote:
>On Mon, Mar 29, 2021 at 07:23:37PM -0500, Zev Weiss wrote:
>> Update DT bindings documentation for the new incarnation of the
>> aspeed,sirq-polarity-sense property.
>
>Why?
>
>This isn't a compatible change.
>

Ah, sorry -- that was a misunderstanding on my end.  I'll resend a 
compatible v2 shortly.


Zev

[PATCH v2 0/3] simplify Aspeed VUART SIRQ polarity DT config

[Zev Weiss]

The aspeed,sirq-polarity-sense property was a bit of a design mistake
in that it ties Aspeed VUART SIRQ polarity to SCU register bits that
aren't really inherently related to it.

This patch series deprecates that property (though we hope to
eventually remove it) and adds a simple boolean property
(aspeed,sirq-active-high) to use instead.


Changes since v1:
 - deprecate and retain aspeed,sirq-polarity-sense instead of removing it
 - drop e3c246d4i dts addition from this series


Zev Weiss (3):
  dt-bindings: serial: 8250: deprecate aspeed,sirq-polarity-sense
  drivers/tty/serial/8250: add DT property for aspeed vuart sirq
    polarity
  dt-bindings: serial: 8250: add aspeed,sirq-active-high

 Documentation/devicetree/bindings/serial/8250.yaml | 14 +++++++++++---
 drivers/tty/serial/8250/8250_aspeed_vuart.c        |  3 +++
 2 files changed, 14 insertions(+), 3 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] dt-bindings: serial: 8250: deprecate aspeed, sirq-polarity-sense

[Zev Weiss]

This property ties SIRQ polarity to SCU register bits that don't
necessarily have any direct relationship to it; the only use of it
was removed in commit c82bf6e133d30e0f9172a20807814fa28aef0f67.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 Documentation/devicetree/bindings/serial/8250.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
index f54cae9ff7b2..491b9297432d 100644
--- a/Documentation/devicetree/bindings/serial/8250.yaml
+++ b/Documentation/devicetree/bindings/serial/8250.yaml
@@ -188,6 +188,7 @@ properties:
       offset and bit number to identify how the SIRQ polarity should be
       configured. One possible data source is the LPC/eSPI mode bit. Only
       applicable to aspeed,ast2500-vuart.
+    deprecated: true
 
 required:
   - reg
-- 
2.31.1

Re: [PATCH v2 1/3] dt-bindings: serial: 8250: deprecate aspeed, sirq-polarity-sense

[Joel Stanley]

On Thu, 1 Apr 2021 at 00:57, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> This property ties SIRQ polarity to SCU register bits that don't
> necessarily have any direct relationship to it; the only use of it
> was removed in commit c82bf6e133d30e0f9172a20807814fa28aef0f67.
>
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>

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

> ---
>  Documentation/devicetree/bindings/serial/8250.yaml | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> index f54cae9ff7b2..491b9297432d 100644
> --- a/Documentation/devicetree/bindings/serial/8250.yaml
> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> @@ -188,6 +188,7 @@ properties:
>        offset and bit number to identify how the SIRQ polarity should be
>        configured. One possible data source is the LPC/eSPI mode bit. Only
>        applicable to aspeed,ast2500-vuart.
> +    deprecated: true
>
>  required:
>    - reg
> --
> 2.31.1
>

[PATCH v2 2/3] drivers/tty/serial/8250: add DT property for aspeed vuart sirq polarity

[Zev Weiss]

This provides a simple boolean to use instead of the deprecated
aspeed,sirq-polarity-sense property.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index c33e02cbde93..e5ef9f957f9a 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -482,6 +482,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 		of_node_put(sirq_polarity_sense_args.np);
 	}
 
+	if (of_property_read_bool(np, "aspeed,sirq-active-high"))
+		aspeed_vuart_set_sirq_polarity(vuart, 1);
+
 	aspeed_vuart_set_enabled(vuart, true);
 	aspeed_vuart_set_host_tx_discard(vuart, true);
 	platform_set_drvdata(pdev, vuart);
-- 
2.31.1

Re: [PATCH v2 2/3] drivers/tty/serial/8250: add DT property for aspeed vuart sirq polarity

[Joel Stanley]

On Thu, 1 Apr 2021 at 00:57, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> This provides a simple boolean to use instead of the deprecated
> aspeed,sirq-polarity-sense property.
>
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  drivers/tty/serial/8250/8250_aspeed_vuart.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index c33e02cbde93..e5ef9f957f9a 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -482,6 +482,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>                 of_node_put(sirq_polarity_sense_args.np);
>         }
>
> +       if (of_property_read_bool(np, "aspeed,sirq-active-high"))
> +               aspeed_vuart_set_sirq_polarity(vuart, 1);

This assumes the default is always low, so we don't need a property to
set it to that state?

Would it make more sense to have the property describe if it's high or
low? (I'm happy for the answer to be "no", as we've gotten by for the
past few years without it).

This brings up another point. We already have the sysfs file for
setting the lpc address, from userspace. In OpenBMC land this can be
set with obmc-console-client (/etc/obmc-console.conf). Should we add
support to that application for setting the irq polarity too, and do
away with device tree descriptions?

> +
>         aspeed_vuart_set_enabled(vuart, true);
>         aspeed_vuart_set_host_tx_discard(vuart, true);
>         platform_set_drvdata(pdev, vuart);
> --
> 2.31.1
>

Re: [PATCH v2 2/3] drivers/tty/serial/8250: add DT property for aspeed vuart sirq polarity

[Zev Weiss]

On Wed, Mar 31, 2021 at 11:15:44PM CDT, Joel Stanley wrote:
>On Thu, 1 Apr 2021 at 00:57, Zev Weiss <zev@bewilderbeest.net> wrote:
>>
>> This provides a simple boolean to use instead of the deprecated
>> aspeed,sirq-polarity-sense property.
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>  drivers/tty/serial/8250/8250_aspeed_vuart.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> index c33e02cbde93..e5ef9f957f9a 100644
>> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> @@ -482,6 +482,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>>                 of_node_put(sirq_polarity_sense_args.np);
>>         }
>>
>> +       if (of_property_read_bool(np, "aspeed,sirq-active-high"))
>> +               aspeed_vuart_set_sirq_polarity(vuart, 1);
>
>This assumes the default is always low, so we don't need a property to
>set it to that state?
>
>Would it make more sense to have the property describe if it's high or
>low? (I'm happy for the answer to be "no", as we've gotten by for the
>past few years without it).
>

Yeah, that sounds like better way to approach it -- I think I'll 
rearrange as Andrew suggested in 
https://lore.kernel.org/openbmc/d66753ee-7db2-41e5-9fe5-762b1ab678bc@www.fastmail.com/

>This brings up another point. We already have the sysfs file for
>setting the lpc address, from userspace. In OpenBMC land this can be
>set with obmc-console-client (/etc/obmc-console.conf). Should we add
>support to that application for setting the irq polarity too, and do
>away with device tree descriptions?
>

I guess I might lean slightly toward keeping the DT description so that 
if for whatever reason obmc-console-server flakes out and doesn't start 
you're better positioned to try banging on /dev/ttyS* manually if you're 
desperate.  Though I suppose that in turn might imply that I'm arguing 
for adding DT properties for lpc_address and sirq too, and if you're 
really that desperate you can just fiddle with sysfs anyway, so...shrug?  
I could be convinced either way fairly easily.


Zev

Re: [PATCH v2 2/3] drivers/tty/serial/8250: add DT property for aspeed vuart sirq polarity

[Andrew Jeffery]

On Thu, 1 Apr 2021, at 15:48, Zev Weiss wrote:
> On Wed, Mar 31, 2021 at 11:15:44PM CDT, Joel Stanley wrote:
> >On Thu, 1 Apr 2021 at 00:57, Zev Weiss <zev@bewilderbeest.net> wrote:
> >>
> >> This provides a simple boolean to use instead of the deprecated
> >> aspeed,sirq-polarity-sense property.
> >>
> >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> >> ---
> >>  drivers/tty/serial/8250/8250_aspeed_vuart.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> >> index c33e02cbde93..e5ef9f957f9a 100644
> >> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> >> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> >> @@ -482,6 +482,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
> >>                 of_node_put(sirq_polarity_sense_args.np);
> >>         }
> >>
> >> +       if (of_property_read_bool(np, "aspeed,sirq-active-high"))
> >> +               aspeed_vuart_set_sirq_polarity(vuart, 1);
> >
> >This assumes the default is always low, so we don't need a property to
> >set it to that state?
> >
> >Would it make more sense to have the property describe if it's high or
> >low? (I'm happy for the answer to be "no", as we've gotten by for the
> >past few years without it).
> >
> 
> Yeah, that sounds like better way to approach it -- I think I'll 
> rearrange as Andrew suggested in 
> https://lore.kernel.org/openbmc/d66753ee-7db2-41e5-9fe5-762b1ab678bc@www.fastmail.com/
> 
> >This brings up another point. We already have the sysfs file for
> >setting the lpc address, from userspace. In OpenBMC land this can be
> >set with obmc-console-client (/etc/obmc-console.conf). Should we add
> >support to that application for setting the irq polarity too, and do
> >away with device tree descriptions?
> >
> 
> I guess I might lean slightly toward keeping the DT description so that 
> if for whatever reason obmc-console-server flakes out and doesn't start 
> you're better positioned to try banging on /dev/ttyS* manually if you're 
> desperate.  Though I suppose that in turn might imply that I'm arguing 
> for adding DT properties for lpc_address and sirq too,

Why not just adopt exactly what I've done with KCS, where we have aspeed,lpc-io-reg and aspeed,lpc-interrupts?

Andrew

Re: [PATCH v2 2/3] drivers/tty/serial/8250: add DT property for aspeed vuart sirq polarity

[Zev Weiss]

On Thu, Apr 01, 2021 at 12:34:04AM CDT, Andrew Jeffery wrote:
>
>
>On Thu, 1 Apr 2021, at 15:48, Zev Weiss wrote:
>> On Wed, Mar 31, 2021 at 11:15:44PM CDT, Joel Stanley wrote:
>> >On Thu, 1 Apr 2021 at 00:57, Zev Weiss <zev@bewilderbeest.net> wrote:
>> >>
>> >> This provides a simple boolean to use instead of the deprecated
>> >> aspeed,sirq-polarity-sense property.
>> >>
>> >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> >> ---
>> >>  drivers/tty/serial/8250/8250_aspeed_vuart.c | 3 +++
>> >>  1 file changed, 3 insertions(+)
>> >>
>> >> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> >> index c33e02cbde93..e5ef9f957f9a 100644
>> >> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> >> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
>> >> @@ -482,6 +482,9 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>> >>                 of_node_put(sirq_polarity_sense_args.np);
>> >>         }
>> >>
>> >> +       if (of_property_read_bool(np, "aspeed,sirq-active-high"))
>> >> +               aspeed_vuart_set_sirq_polarity(vuart, 1);
>> >
>> >This assumes the default is always low, so we don't need a property to
>> >set it to that state?
>> >
>> >Would it make more sense to have the property describe if it's high or
>> >low? (I'm happy for the answer to be "no", as we've gotten by for the
>> >past few years without it).
>> >
>>
>> Yeah, that sounds like better way to approach it -- I think I'll
>> rearrange as Andrew suggested in
>> https://lore.kernel.org/openbmc/d66753ee-7db2-41e5-9fe5-762b1ab678bc@www.fastmail.com/
>>
>> >This brings up another point. We already have the sysfs file for
>> >setting the lpc address, from userspace. In OpenBMC land this can be
>> >set with obmc-console-client (/etc/obmc-console.conf). Should we add
>> >support to that application for setting the irq polarity too, and do
>> >away with device tree descriptions?
>> >
>>
>> I guess I might lean slightly toward keeping the DT description so that
>> if for whatever reason obmc-console-server flakes out and doesn't start
>> you're better positioned to try banging on /dev/ttyS* manually if you're
>> desperate.  Though I suppose that in turn might imply that I'm arguing
>> for adding DT properties for lpc_address and sirq too,
>
>Why not just adopt exactly what I've done with KCS, where we have aspeed,lpc-io-reg and aspeed,lpc-interrupts?
>
>Andrew

Ah -- yes, that does sound like a sensible approach.  I'll send a v3 
with that worked in.


Zev

[PATCH v2 3/3] dt-bindings: serial: 8250: add aspeed, sirq-active-high

[Zev Weiss]

This provides a simpler, more direct alternative to the deprecated
aspeed,sirq-polarity-sense property for indicating the polarity of
the Aspeed VUART's SIRQ line.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 Documentation/devicetree/bindings/serial/8250.yaml | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
index 491b9297432d..e79bb6ab9d2c 100644
--- a/Documentation/devicetree/bindings/serial/8250.yaml
+++ b/Documentation/devicetree/bindings/serial/8250.yaml
@@ -12,8 +12,9 @@ maintainers:
 allOf:
   - $ref: /schemas/serial.yaml#
   - if:
-      required:
-        - aspeed,sirq-polarity-sense
+      anyOf:
+        - required: [ aspeed,sirq-active-high ]
+        - required: [ aspeed,sirq-polarity-sense ]
     then:
       properties:
         compatible:
@@ -190,6 +191,12 @@ properties:
       applicable to aspeed,ast2500-vuart.
     deprecated: true
 
+  aspeed,sirq-active-high:
+    type: boolean
+    description: |
+      Set to indicate that the SIRQ polarity is active-high (default
+      is active-low).  Only applicable to aspeed,ast2500-vuart.
+
 required:
   - reg
   - interrupts
@@ -228,7 +235,7 @@ examples:
         interrupts = <8>;
         clocks = <&syscon ASPEED_CLK_APB>;
         no-loopback-test;
-        aspeed,sirq-polarity-sense = <&syscon 0x70 25>;
+        aspeed,sirq-active-high;
     };
 
 ...
-- 
2.31.1

Re: [PATCH v2 3/3] dt-bindings: serial: 8250: add aspeed, sirq-active-high

[Andrew Jeffery]

On Thu, 1 Apr 2021, at 11:27, Zev Weiss wrote:
> This provides a simpler, more direct alternative to the deprecated
> aspeed,sirq-polarity-sense property for indicating the polarity of
> the Aspeed VUART's SIRQ line.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  Documentation/devicetree/bindings/serial/8250.yaml | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml 
> b/Documentation/devicetree/bindings/serial/8250.yaml
> index 491b9297432d..e79bb6ab9d2c 100644
> --- a/Documentation/devicetree/bindings/serial/8250.yaml
> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> @@ -12,8 +12,9 @@ maintainers:
>  allOf:
>    - $ref: /schemas/serial.yaml#
>    - if:
> -      required:
> -        - aspeed,sirq-polarity-sense
> +      anyOf:
> +        - required: [ aspeed,sirq-active-high ]

Do you think we could make use of the approach I put forward here?

https://lore.kernel.org/openbmc/20210319062752.145730-18-andrew@aj.id.au/T/#u

Andrew

Re: [PATCH v2 3/3] dt-bindings: serial: 8250: add aspeed,sirq-active-high

[Zev Weiss]

On Wed, Mar 31, 2021 at 11:04:44PM CDT, Andrew Jeffery wrote:
>
>
>On Thu, 1 Apr 2021, at 11:27, Zev Weiss wrote:
>> This provides a simpler, more direct alternative to the deprecated
>> aspeed,sirq-polarity-sense property for indicating the polarity of
>> the Aspeed VUART's SIRQ line.
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>  Documentation/devicetree/bindings/serial/8250.yaml | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml
>> b/Documentation/devicetree/bindings/serial/8250.yaml
>> index 491b9297432d..e79bb6ab9d2c 100644
>> --- a/Documentation/devicetree/bindings/serial/8250.yaml
>> +++ b/Documentation/devicetree/bindings/serial/8250.yaml
>> @@ -12,8 +12,9 @@ maintainers:
>>  allOf:
>>    - $ref: /schemas/serial.yaml#
>>    - if:
>> -      required:
>> -        - aspeed,sirq-polarity-sense
>> +      anyOf:
>> +        - required: [ aspeed,sirq-active-high ]
>
>Do you think we could make use of the approach I put forward here?
>
>https://lore.kernel.org/openbmc/20210319062752.145730-18-andrew@aj.id.au/T/#u
>
>Andrew

If you mean using a u32 property (say aspeed,sirq-polarity) with an 
explicit IRQ_TYPE_LEVEL_{LOW,HIGH} instead of a present/absent bool,
sure, I guess that seems like a somewhat clearer, more orthogonal 
arrangement.


Zev

Re: [PATCH v2 3/3] dt-bindings: serial: 8250: add aspeed, sirq-active-high

[Rob Herring]

On Wed, 31 Mar 2021 19:57:02 -0500, Zev Weiss wrote:
> This provides a simpler, more direct alternative to the deprecated
> aspeed,sirq-polarity-sense property for indicating the polarity of
> the Aspeed VUART's SIRQ line.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  Documentation/devicetree/bindings/serial/8250.yaml | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/serial/8250.yaml:16:30: [warning] too few spaces after comma (commas)
./Documentation/devicetree/bindings/serial/8250.yaml:17:30: [warning] too few spaces after comma (commas)

dtschema/dtc warnings/errors:

See https://patchwork.ozlabs.org/patch/1460791

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 2/3] dt-bindings: serial: 8250: update for aspeed, sirq-active-high

[Joel Stanley]

On Tue, 30 Mar 2021 at 22:39, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Mar 29, 2021 at 07:23:37PM -0500, Zev Weiss wrote:
> > Update DT bindings documentation for the new incarnation of the
> > aspeed,sirq-polarity-sense property.
>
> Why?
>
> This isn't a compatible change.

We want to depreciate support for this property. It should have never
been added to the bindings; in it's current form it describes a
relationship that afaict doesn't exist ("This unrelated register over
here dictates the polarity of your virtual serial port IRQ"). See
https://lore.kernel.org/lkml/20200812112400.2406734-1-joel@jms.id.au/

The intent is to remove it from both the bindings and the code.
There's already no users of it in any device tree.

How would you like Zev to go about doing this?

Cheers,

Joel

>
> >
> > Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> > ---
> >  Documentation/devicetree/bindings/serial/8250.yaml | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> > index f54cae9ff7b2..0bbb7121f720 100644
> > --- a/Documentation/devicetree/bindings/serial/8250.yaml
> > +++ b/Documentation/devicetree/bindings/serial/8250.yaml
> > @@ -13,7 +13,7 @@ allOf:
> >    - $ref: /schemas/serial.yaml#
> >    - if:
> >        required:
> > -        - aspeed,sirq-polarity-sense
> > +        - aspeed,sirq-active-high
> >      then:
> >        properties:
> >          compatible:
> > @@ -181,13 +181,11 @@ properties:
> >    rng-gpios: true
> >    dcd-gpios: true
> >
> > -  aspeed,sirq-polarity-sense:
> > -    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +  aspeed,sirq-active-high:
> > +    type: boolean
> >      description: |
> > -      Phandle to aspeed,ast2500-scu compatible syscon alongside register
> > -      offset and bit number to identify how the SIRQ polarity should be
> > -      configured. One possible data source is the LPC/eSPI mode bit. Only
> > -      applicable to aspeed,ast2500-vuart.
> > +      Set to indicate that the SIRQ polarity is active-high (default
> > +      is active-low).  Only applicable to aspeed,ast2500-vuart.
> >
> >  required:
> >    - reg
> > @@ -227,7 +225,7 @@ examples:
> >          interrupts = <8>;
> >          clocks = <&syscon ASPEED_CLK_APB>;
> >          no-loopback-test;
> > -        aspeed,sirq-polarity-sense = <&syscon 0x70 25>;
> > +        aspeed,sirq-active-high;
> >      };
> >
> >  ...
> > --
> > 2.31.1
> >

[PATCH 3/3] ARM: dts: aspeed: add ASRock E3C246D4I BMC

[Zev Weiss]

This is a relatively low-cost AST2500-based Xeon E-2100/E-2200 series
mini-ITX board that we hope can provide a decent platform for OpenBMC
development.

This initial device-tree provides the necessary configuration for
basic BMC functionality such as host power control, serial console and
KVM support, and POST code snooping.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 .../boot/dts/aspeed-bmc-asrock-e3c246d4i.dts  | 188 ++++++++++++++++++
 1 file changed, 188 insertions(+)
 create mode 100644 arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts

diff --git a/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
new file mode 100644
index 000000000000..27b34c3cf67a
--- /dev/null
+++ b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0+
+/dts-v1/;
+
+#include "aspeed-g5.dtsi"
+#include <dt-bindings/gpio/aspeed-gpio.h>
+#include <dt-bindings/i2c/i2c.h>
+
+/{
+	model = "ASRock E3C246D4I BMC";
+	compatible = "aspeed,ast2500";
+
+	aliases {
+		serial4 = &uart5;
+	};
+
+	chosen {
+		stdout-path = &uart5;
+		bootargs = "console=tty0 console=ttyS4,115200 earlyprintk";
+	};
+
+	memory@80000000 {
+		reg = <0x80000000 0x20000000>;
+	};
+
+	leds {
+		compatible = "gpio-leds";
+
+		heartbeat {
+			/* BMC_HB_LED_N */
+			gpios = <&gpio ASPEED_GPIO(H, 6) GPIO_ACTIVE_LOW>;
+			linux,default-trigger = "timer";
+		};
+
+		system-fault {
+			/* SYSTEM_FAULT_LED_N */
+			gpios = <&gpio ASPEED_GPIO(Z, 2) GPIO_ACTIVE_LOW>;
+			panic-indicator;
+		};
+	};
+
+	gpio-keys {
+		compatible = "gpio-keys";
+
+		uid-button {
+			label = "uid-button";
+			gpios = <&gpio ASPEED_GPIO(F, 1) GPIO_ACTIVE_LOW>;
+			linux,code = <ASPEED_GPIO(F, 1)>;
+		};
+	};
+
+	iio-hwmon {
+		compatible = "iio-hwmon";
+		io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>, <&adc 4>,
+			<&adc 5>, <&adc 6>, <&adc 7>, <&adc 8>, <&adc 9>,
+			<&adc 10>, <&adc 11>, <&adc 12>;
+	};
+};
+
+&fmc {
+	status = "okay";
+	flash@0 {
+		status = "okay";
+		m25p,fast-read;
+		label = "bmc";
+		spi-max-frequency = <100000000>; /* 100 MHz */
+#include "openbmc-flash-layout.dtsi"
+	};
+};
+
+&uart5 {
+	status = "okay";
+};
+
+&vuart {
+	status = "okay";
+	aspeed,sirq-active-high;
+};
+
+&mac0 {
+	status = "okay";
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_rgmii1_default &pinctrl_mdio1_default>;
+};
+
+&i2c1 {
+	status = "okay";
+
+	/* thermal sensor, one diode run to a disconnected header */
+	w83773g@4c {
+		compatible = "nuvoton,w83773g";
+		reg = <0x4c>;
+	};
+};
+
+&i2c3 {
+	status = "okay";
+
+	/* FRU EEPROM */
+	eeprom@57 {
+		compatible = "st,24c128", "atmel,24c128";
+		reg = <0x57>;
+		pagesize = <16>;
+	};
+};
+
+&video {
+	status = "okay";
+};
+
+&vhub {
+	status = "okay";
+};
+
+&lpc_ctrl {
+	status = "okay";
+};
+
+&lpc_snoop {
+	status = "okay";
+	snoop-ports = <0x80>;
+};
+
+&gpio {
+	status = "okay";
+	gpio-line-names =
+		/*  A */ "BMC_MAC1_INTB", "BMC_MAC2_INTB", "NMI_BTN_N", "BMC_NMI",
+			"", "", "", "",
+		/*  B */ "", "", "", "", "", "IRQ_BMC_PCH_SMI_LPC_N", "", "",
+		/*  C */ "", "", "", "", "", "", "", "",
+		/*  D */ "BMC_PSIN", "BMC_PSOUT", "BMC_RESETCON", "RESETCON",
+			"", "", "", "",
+		/*  E */ "", "", "", "", "", "", "", "",
+		/*  F */ "LOCATORLED_STATUS_N", "LOCATORBTN", "", "",
+			"", "", "BMC_PCH_SCI_LPC", "BMC_NCSI_MUX_CTL",
+		/*  G */ "HWM_BAT_EN", "CHASSIS_ID0", "CHASSIS_ID1", "CHASSIS_ID2",
+			"BMC_ALERT1_N_R", "BMC_ALERT2_N_R", "BMC_ALERT3_N", "SML0ALERT",
+		/*  H */ "FM_ME_RCVR_N", "O_PWROK", "SKL_CNL_R", "D4_DIMM_EVENT_3V_N",
+			"MFG_MODE_N", "BMC_RTCRST", "BMC_HB_LED_N", "BMC_CASEOPEN",
+		/*  I */ "", "", "", "", "", "", "", "",
+		/*  J */ "BMC_READY", "BMC_PCH_BIOS_CS_N", "BMC_SMI", "",
+			"", "", "", "",
+		/*  K */ "", "", "", "", "", "", "", "",
+		/*  L */ "BMC_CTS1", "BMC_DCD1", "BMC_DSR1", "BMC_RI1",
+			"BMC_DTR1", "BMC_RTS1", "BMC_TXD1", "BMC_RXD1",
+		/*  M */ "BMC_LAN0_DIS_N", "BMC_LAN1_DIS_N", "", "",
+			"", "", "", "",
+		/*  N */ "", "", "", "", "", "", "", "",
+		/*  O */ "", "", "", "", "", "", "", "",
+		/*  P */ "", "", "", "", "", "", "", "",
+		/*  Q */ "", "", "", "",
+			"BMC_SBM_PRESENT_1_N", "BMC_SBM_PRESENT_2_N",
+			"BMC_SBM_PRESENT_3_N", "BMC_PCIE_WAKE_N",
+		/*  R */ "", "", "", "", "", "", "", "",
+		/*  S */ "PCHHOT_BMC_N", "", "RSMRST",
+			"", "", "", "", "",
+		/*  T */ "", "", "", "", "", "", "", "",
+		/*  U */ "", "", "", "", "", "", "", "",
+		/*  V */ "", "", "", "", "", "", "", "",
+		/*  W */ "PS_PWROK", /* dummy always-high signal */
+			"", "", "", "", "", "", "",
+		/*  X */ "", "", "", "", "", "", "", "",
+		/*  Y */ "SLP_S3", "SLP_S5", "", "", "", "", "", "",
+		/*  Z */ "CPU_CATERR_BMC_PCH_N", "", "SYSTEM_FAULT_LED_N", "BMC_THROTTLE_N",
+			"", "", "", "",
+		/* AA */ "CPU1_THERMTRIP_LATCH_N", "", "CPU1_PROCHOT_N", "",
+			"", "", "IRQ_SMI_ACTIVE_N", "FM_BIOS_POST_CMPLT_N",
+		/* AB */ "", "", "ME_OVERRIDE", "BMC_DMI_MODIFY",
+			"", "", "", "",
+		/* AC */ "LAD0", "LAD1", "LAD2", "LAD3",
+			"CK_33M_BMC", "LFRAME", "SERIRQ", "S_PLTRST";
+
+	/* Assert BMC_READY so BIOS doesn't sit around waiting for it */
+	bmc-ready {
+		gpio-hog;
+		gpios = <ASPEED_GPIO(J, 0) GPIO_ACTIVE_LOW>;
+		output-high;
+	};
+};
+
+&adc {
+	status = "okay";
+};
+
+&kcs3 {
+	status = "okay";
+	aspeed,lpc-io-reg = <0xca2>;
+};
-- 
2.31.1

Re: [PATCH 3/3] ARM: dts: aspeed: add ASRock E3C246D4I BMC

[Joel Stanley]

On Tue, 30 Mar 2021 at 00:25, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> This is a relatively low-cost AST2500-based Xeon E-2100/E-2200 series
> mini-ITX board that we hope can provide a decent platform for OpenBMC
> development.
>
> This initial device-tree provides the necessary configuration for
> basic BMC functionality such as host power control, serial console and
> KVM support, and POST code snooping.
>
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>

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

> ---
>  .../boot/dts/aspeed-bmc-asrock-e3c246d4i.dts  | 188 ++++++++++++++++++
>  1 file changed, 188 insertions(+)
>  create mode 100644 arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
> new file mode 100644
> index 000000000000..27b34c3cf67a
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/dts-v1/;
> +
> +#include "aspeed-g5.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +#include <dt-bindings/i2c/i2c.h>
> +
> +/{
> +       model = "ASRock E3C246D4I BMC";
> +       compatible = "aspeed,ast2500";

Convention is to add a compatible for the board. I'll add
asrock,e3c246d4Ii-bmc when I apply the patch.

> +&vuart {
> +       status = "okay";
> +       aspeed,sirq-active-high;

We don't have support for this yet, but I'll leave it in and you will
need to send a follow up if the property changes.

Cheers,

Joel

Re: [PATCH 3/3] ARM: dts: aspeed: add ASRock E3C246D4I BMC

[Joel Stanley]

On Wed, 31 Mar 2021 at 07:41, Joel Stanley <joel@jms.id.au> wrote:
>
> On Tue, 30 Mar 2021 at 00:25, Zev Weiss <zev@bewilderbeest.net> wrote:
> >
> > This is a relatively low-cost AST2500-based Xeon E-2100/E-2200 series
> > mini-ITX board that we hope can provide a decent platform for OpenBMC
> > development.
> >
> > This initial device-tree provides the necessary configuration for
> > basic BMC functionality such as host power control, serial console and
> > KVM support, and POST code snooping.
> >
> > Signed-off-by: Zev Weiss <zev@bewilderbeest.net>

> > +&vuart {
> > +       status = "okay";
> > +       aspeed,sirq-active-high;
>
> We don't have support for this yet, but I'll leave it in and you will
> need to send a follow up if the property changes.

Oh, I missed that this was part of your series to add support for that
property. Please keep the device tree out of the series when you
re-send the vuart patches. They go through different trees, so it's
easier if you send them separately in this case.

Re: [PATCH 3/3] ARM: dts: aspeed: add ASRock E3C246D4I BMC

[Joel Stanley]

On Wed, 31 Mar 2021 at 07:41, Joel Stanley <joel@jms.id.au> wrote:
>
> On Tue, 30 Mar 2021 at 00:25, Zev Weiss <zev@bewilderbeest.net> wrote:
> >
> > This is a relatively low-cost AST2500-based Xeon E-2100/E-2200 series
> > mini-ITX board that we hope can provide a decent platform for OpenBMC
> > development.
> >
> > This initial device-tree provides the necessary configuration for
> > basic BMC functionality such as host power control, serial console and
> > KVM support, and POST code snooping.
> >
> > Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> > ---
> >  .../boot/dts/aspeed-bmc-asrock-e3c246d4i.dts  | 188 ++++++++++++++++++
> >  1 file changed, 188 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
> >
> > diff --git a/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
> > new file mode 100644
> > index 000000000000..27b34c3cf67a
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts

When adding the device tree please also add it to the makefile in
arch/arm/boot/dts.

[PATCH] ARM: dts: aspeed: add ASRock E3C246D4I BMC

[Zev Weiss]

This is a relatively low-cost AST2500-based Xeon E-2100/E-2200 series
mini-ITX board that we hope can provide a decent platform for OpenBMC
development.

This initial device-tree provides the necessary configuration for
basic BMC functionality such as host power control, serial console and
KVM support, and POST code snooping.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
Reviewed-by: Joel Stanley <joel@jms.id.au>
---
 arch/arm/boot/dts/Makefile                    |   1 +
 .../boot/dts/aspeed-bmc-asrock-e3c246d4i.dts  | 202 ++++++++++++++++++
 2 files changed, 203 insertions(+)
 create mode 100644 arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 8e5d4ab4e75e..b12911262ca1 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -1406,6 +1406,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
 	aspeed-bmc-ampere-mtjade.dtb \
 	aspeed-bmc-arm-centriq2400-rep.dtb \
 	aspeed-bmc-arm-stardragon4800-rep2.dtb \
+	aspeed-bmc-asrock-e3c246d4i.dts \
 	aspeed-bmc-bytedance-g220a.dtb \
 	aspeed-bmc-facebook-cmm.dtb \
 	aspeed-bmc-facebook-galaxy100.dtb \
diff --git a/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
new file mode 100644
index 000000000000..dcab6e78dfa4
--- /dev/null
+++ b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: GPL-2.0+
+/dts-v1/;
+
+#include "aspeed-g5.dtsi"
+#include <dt-bindings/gpio/aspeed-gpio.h>
+#include <dt-bindings/i2c/i2c.h>
+
+/{
+	model = "ASRock E3C246D4I BMC";
+	compatible = "asrock,e3c246d4i-bmc", "aspeed,ast2500";
+
+	aliases {
+		serial4 = &uart5;
+	};
+
+	chosen {
+		stdout-path = &uart5;
+		bootargs = "console=tty0 console=ttyS4,115200 earlyprintk";
+	};
+
+	memory@80000000 {
+		reg = <0x80000000 0x20000000>;
+	};
+
+	leds {
+		compatible = "gpio-leds";
+
+		heartbeat {
+			/* BMC_HB_LED_N */
+			gpios = <&gpio ASPEED_GPIO(H, 6) GPIO_ACTIVE_LOW>;
+			linux,default-trigger = "timer";
+		};
+
+		system-fault {
+			/* SYSTEM_FAULT_LED_N */
+			gpios = <&gpio ASPEED_GPIO(Z, 2) GPIO_ACTIVE_LOW>;
+			panic-indicator;
+		};
+	};
+
+	gpio-keys {
+		compatible = "gpio-keys";
+
+		uid-button {
+			label = "uid-button";
+			gpios = <&gpio ASPEED_GPIO(F, 1) GPIO_ACTIVE_LOW>;
+			linux,code = <ASPEED_GPIO(F, 1)>;
+		};
+	};
+
+	iio-hwmon {
+		compatible = "iio-hwmon";
+		io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>, <&adc 4>,
+			<&adc 5>, <&adc 6>, <&adc 7>, <&adc 8>, <&adc 9>,
+			<&adc 10>, <&adc 11>, <&adc 12>;
+	};
+};
+
+&fmc {
+	status = "okay";
+	flash@0 {
+		status = "okay";
+		m25p,fast-read;
+		label = "bmc";
+		spi-max-frequency = <100000000>; /* 100 MHz */
+#include "openbmc-flash-layout.dtsi"
+	};
+};
+
+&uart5 {
+	status = "okay";
+};
+
+&vuart {
+	status = "okay";
+	aspeed,sirq-active-high;
+};
+
+&mac0 {
+	status = "okay";
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_rgmii1_default &pinctrl_mdio1_default>;
+};
+
+&i2c1 {
+	status = "okay";
+
+	/* thermal sensor, one diode run to a disconnected header */
+	w83773g@4c {
+		compatible = "nuvoton,w83773g";
+		reg = <0x4c>;
+	};
+};
+
+&i2c3 {
+	status = "okay";
+
+	/* FRU EEPROM */
+	eeprom@57 {
+		compatible = "st,24c128", "atmel,24c128";
+		reg = <0x57>;
+		pagesize = <16>;
+	};
+};
+
+&video {
+	status = "okay";
+};
+
+&vhub {
+	status = "okay";
+};
+
+&lpc_ctrl {
+	status = "okay";
+};
+
+&lpc_snoop {
+	status = "okay";
+	snoop-ports = <0x80>;
+};
+
+&gpio {
+	status = "okay";
+	gpio-line-names =
+		/*  A */ "BMC_MAC1_INTB", "BMC_MAC2_INTB", "NMI_BTN_N", "BMC_NMI",
+			"", "", "", "",
+		/*  B */ "", "", "", "", "", "IRQ_BMC_PCH_SMI_LPC_N", "", "",
+		/*  C */ "", "", "", "", "", "", "", "",
+		/*  D */ "BMC_PSIN", "BMC_PSOUT", "BMC_RESETCON", "RESETCON",
+			"", "", "", "",
+		/*  E */ "", "", "", "", "", "", "", "",
+		/*  F */ "LOCATORLED_STATUS_N", "LOCATORBTN", "", "",
+			"", "", "BMC_PCH_SCI_LPC", "BMC_NCSI_MUX_CTL",
+		/*  G */ "HWM_BAT_EN", "CHASSIS_ID0", "CHASSIS_ID1", "CHASSIS_ID2",
+			"BMC_ALERT1_N_R", "BMC_ALERT2_N_R", "BMC_ALERT3_N", "SML0ALERT",
+		/*  H */ "FM_ME_RCVR_N", "O_PWROK", "SKL_CNL_R", "D4_DIMM_EVENT_3V_N",
+			"MFG_MODE_N", "BMC_RTCRST", "BMC_HB_LED_N", "BMC_CASEOPEN",
+		/*  I */ "", "", "", "", "", "", "", "",
+		/*  J */ "BMC_READY", "BMC_PCH_BIOS_CS_N", "BMC_SMI", "",
+			"", "", "", "",
+		/*  K */ "", "", "", "", "", "", "", "",
+		/*  L */ "BMC_CTS1", "BMC_DCD1", "BMC_DSR1", "BMC_RI1",
+			"BMC_DTR1", "BMC_RTS1", "BMC_TXD1", "BMC_RXD1",
+		/*  M */ "BMC_LAN0_DIS_N", "BMC_LAN1_DIS_N", "", "",
+			"", "", "", "",
+		/*  N */ "", "", "", "", "", "", "", "",
+		/*  O */ "", "", "", "", "", "", "", "",
+		/*  P */ "", "", "", "", "", "", "", "",
+		/*  Q */ "", "", "", "",
+			"BMC_SBM_PRESENT_1_N", "BMC_SBM_PRESENT_2_N",
+			"BMC_SBM_PRESENT_3_N", "BMC_PCIE_WAKE_N",
+		/*  R */ "", "", "", "", "", "", "", "",
+		/*  S */ "PCHHOT_BMC_N", "", "RSMRST",
+			"", "", "", "", "",
+		/*  T */ "", "", "", "", "", "", "", "",
+		/*  U */ "", "", "", "", "", "", "", "",
+		/*  V */ "", "", "", "", "", "", "", "",
+		/*  W */ "PS_PWROK", /* dummy always-high signal */
+			"", "", "", "", "", "", "",
+		/*  X */ "", "", "", "", "", "", "", "",
+		/*  Y */ "SLP_S3", "SLP_S5", "", "", "", "", "", "",
+		/*  Z */ "CPU_CATERR_BMC_PCH_N", "", "SYSTEM_FAULT_LED_N", "BMC_THROTTLE_N",
+			"", "", "", "",
+		/* AA */ "CPU1_THERMTRIP_LATCH_N", "", "CPU1_PROCHOT_N", "",
+			"", "", "IRQ_SMI_ACTIVE_N", "FM_BIOS_POST_CMPLT_N",
+		/* AB */ "", "", "ME_OVERRIDE", "BMC_DMI_MODIFY",
+			"", "", "", "",
+		/* AC */ "LAD0", "LAD1", "LAD2", "LAD3",
+			"CK_33M_BMC", "LFRAME", "SERIRQ", "S_PLTRST";
+
+	/* Assert BMC_READY so BIOS doesn't sit around waiting for it */
+	bmc-ready {
+		gpio-hog;
+		gpios = <ASPEED_GPIO(J, 0) GPIO_ACTIVE_LOW>;
+		output-high;
+	};
+};
+
+&adc {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_adc0_default
+			&pinctrl_adc1_default
+			&pinctrl_adc2_default
+			&pinctrl_adc3_default
+			&pinctrl_adc4_default
+			&pinctrl_adc5_default
+			&pinctrl_adc6_default
+			&pinctrl_adc7_default
+			&pinctrl_adc8_default
+			&pinctrl_adc9_default
+			&pinctrl_adc10_default
+			&pinctrl_adc11_default
+			&pinctrl_adc12_default>;
+};
+
+&kcs3 {
+	status = "okay";
+	aspeed,lpc-io-reg = <0xca2>;
+};
-- 
2.31.1

Re: [PATCH] ARM: dts: aspeed: add ASRock E3C246D4I BMC

[Joel Stanley]

Hi Zev,

On Thu, 1 Apr 2021 at 02:57, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> This is a relatively low-cost AST2500-based Xeon E-2100/E-2200 series
> mini-ITX board that we hope can provide a decent platform for OpenBMC
> development.
>
> This initial device-tree provides the necessary configuration for
> basic BMC functionality such as host power control, serial console and
> KVM support, and POST code snooping.

The patch looks good! Some minor things below.

When sending subsequent versions, make sure to add -v N to your git
format-patch to mark it as the Nth version.

You've also set this to be threaded with a previous version of the
patch. We normally don't do that, and in this case it's doubly
confusing as you've split this patch out from the previous series.

I noticed you cc'd soc@kernel.org. We normally only do this when we
want the soc maintainers to apply a patch directly without going
through another maintainer. In this case the patch should go through
the aspeed maintainer's tree (me), so you don't need to cc that
address.

> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> ---

This spot just here is where you should put the changes between v1 and v2.

>  arch/arm/boot/dts/Makefile                    |   1 +
>  .../boot/dts/aspeed-bmc-asrock-e3c246d4i.dts  | 202 ++++++++++++++++++
>  2 files changed, 203 insertions(+)
>  create mode 100644 arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 8e5d4ab4e75e..b12911262ca1 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1406,6 +1406,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>         aspeed-bmc-ampere-mtjade.dtb \
>         aspeed-bmc-arm-centriq2400-rep.dtb \
>         aspeed-bmc-arm-stardragon4800-rep2.dtb \
> +       aspeed-bmc-asrock-e3c246d4i.dts \

This should be the output name (.dtb).

>         aspeed-bmc-bytedance-g220a.dtb \
>         aspeed-bmc-facebook-cmm.dtb \
>         aspeed-bmc-facebook-galaxy100.dtb \
> diff --git a/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
> new file mode 100644
> index 000000000000..dcab6e78dfa4
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts

The device tree itself looks good!

If you fix up the things I mentioned and send a v3 I will apply it.

Cheers,

Joel

> @@ -0,0 +1,202 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/dts-v1/;
> +
> +#include "aspeed-g5.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +#include <dt-bindings/i2c/i2c.h>
> +
> +/{
> +       model = "ASRock E3C246D4I BMC";
> +       compatible = "asrock,e3c246d4i-bmc", "aspeed,ast2500";
> +
> +       aliases {
> +               serial4 = &uart5;
> +       };
> +
> +       chosen {
> +               stdout-path = &uart5;
> +               bootargs = "console=tty0 console=ttyS4,115200 earlyprintk";
> +       };
> +
> +       memory@80000000 {
> +               reg = <0x80000000 0x20000000>;
> +       };
> +
> +       leds {
> +               compatible = "gpio-leds";
> +
> +               heartbeat {
> +                       /* BMC_HB_LED_N */
> +                       gpios = <&gpio ASPEED_GPIO(H, 6) GPIO_ACTIVE_LOW>;
> +                       linux,default-trigger = "timer";
> +               };
> +
> +               system-fault {
> +                       /* SYSTEM_FAULT_LED_N */
> +                       gpios = <&gpio ASPEED_GPIO(Z, 2) GPIO_ACTIVE_LOW>;
> +                       panic-indicator;
> +               };
> +       };
> +
> +       gpio-keys {
> +               compatible = "gpio-keys";
> +
> +               uid-button {
> +                       label = "uid-button";
> +                       gpios = <&gpio ASPEED_GPIO(F, 1) GPIO_ACTIVE_LOW>;
> +                       linux,code = <ASPEED_GPIO(F, 1)>;
> +               };
> +       };
> +
> +       iio-hwmon {
> +               compatible = "iio-hwmon";
> +               io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>, <&adc 4>,
> +                       <&adc 5>, <&adc 6>, <&adc 7>, <&adc 8>, <&adc 9>,
> +                       <&adc 10>, <&adc 11>, <&adc 12>;
> +       };
> +};
> +
> +&fmc {
> +       status = "okay";
> +       flash@0 {
> +               status = "okay";
> +               m25p,fast-read;
> +               label = "bmc";
> +               spi-max-frequency = <100000000>; /* 100 MHz */
> +#include "openbmc-flash-layout.dtsi"
> +       };
> +};
> +
> +&uart5 {
> +       status = "okay";
> +};
> +
> +&vuart {
> +       status = "okay";
> +       aspeed,sirq-active-high;
> +};
> +
> +&mac0 {
> +       status = "okay";
> +
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_rgmii1_default &pinctrl_mdio1_default>;
> +};
> +
> +&i2c1 {
> +       status = "okay";
> +
> +       /* thermal sensor, one diode run to a disconnected header */
> +       w83773g@4c {
> +               compatible = "nuvoton,w83773g";
> +               reg = <0x4c>;
> +       };
> +};
> +
> +&i2c3 {
> +       status = "okay";
> +
> +       /* FRU EEPROM */
> +       eeprom@57 {
> +               compatible = "st,24c128", "atmel,24c128";
> +               reg = <0x57>;
> +               pagesize = <16>;
> +       };
> +};
> +
> +&video {
> +       status = "okay";
> +};
> +
> +&vhub {
> +       status = "okay";
> +};
> +
> +&lpc_ctrl {
> +       status = "okay";
> +};
> +
> +&lpc_snoop {
> +       status = "okay";
> +       snoop-ports = <0x80>;
> +};
> +
> +&gpio {
> +       status = "okay";
> +       gpio-line-names =
> +               /*  A */ "BMC_MAC1_INTB", "BMC_MAC2_INTB", "NMI_BTN_N", "BMC_NMI",
> +                       "", "", "", "",
> +               /*  B */ "", "", "", "", "", "IRQ_BMC_PCH_SMI_LPC_N", "", "",
> +               /*  C */ "", "", "", "", "", "", "", "",
> +               /*  D */ "BMC_PSIN", "BMC_PSOUT", "BMC_RESETCON", "RESETCON",
> +                       "", "", "", "",
> +               /*  E */ "", "", "", "", "", "", "", "",
> +               /*  F */ "LOCATORLED_STATUS_N", "LOCATORBTN", "", "",
> +                       "", "", "BMC_PCH_SCI_LPC", "BMC_NCSI_MUX_CTL",
> +               /*  G */ "HWM_BAT_EN", "CHASSIS_ID0", "CHASSIS_ID1", "CHASSIS_ID2",
> +                       "BMC_ALERT1_N_R", "BMC_ALERT2_N_R", "BMC_ALERT3_N", "SML0ALERT",
> +               /*  H */ "FM_ME_RCVR_N", "O_PWROK", "SKL_CNL_R", "D4_DIMM_EVENT_3V_N",
> +                       "MFG_MODE_N", "BMC_RTCRST", "BMC_HB_LED_N", "BMC_CASEOPEN",
> +               /*  I */ "", "", "", "", "", "", "", "",
> +               /*  J */ "BMC_READY", "BMC_PCH_BIOS_CS_N", "BMC_SMI", "",
> +                       "", "", "", "",
> +               /*  K */ "", "", "", "", "", "", "", "",
> +               /*  L */ "BMC_CTS1", "BMC_DCD1", "BMC_DSR1", "BMC_RI1",
> +                       "BMC_DTR1", "BMC_RTS1", "BMC_TXD1", "BMC_RXD1",
> +               /*  M */ "BMC_LAN0_DIS_N", "BMC_LAN1_DIS_N", "", "",
> +                       "", "", "", "",
> +               /*  N */ "", "", "", "", "", "", "", "",
> +               /*  O */ "", "", "", "", "", "", "", "",
> +               /*  P */ "", "", "", "", "", "", "", "",
> +               /*  Q */ "", "", "", "",
> +                       "BMC_SBM_PRESENT_1_N", "BMC_SBM_PRESENT_2_N",
> +                       "BMC_SBM_PRESENT_3_N", "BMC_PCIE_WAKE_N",
> +               /*  R */ "", "", "", "", "", "", "", "",
> +               /*  S */ "PCHHOT_BMC_N", "", "RSMRST",
> +                       "", "", "", "", "",
> +               /*  T */ "", "", "", "", "", "", "", "",
> +               /*  U */ "", "", "", "", "", "", "", "",
> +               /*  V */ "", "", "", "", "", "", "", "",
> +               /*  W */ "PS_PWROK", /* dummy always-high signal */
> +                       "", "", "", "", "", "", "",
> +               /*  X */ "", "", "", "", "", "", "", "",
> +               /*  Y */ "SLP_S3", "SLP_S5", "", "", "", "", "", "",
> +               /*  Z */ "CPU_CATERR_BMC_PCH_N", "", "SYSTEM_FAULT_LED_N", "BMC_THROTTLE_N",
> +                       "", "", "", "",
> +               /* AA */ "CPU1_THERMTRIP_LATCH_N", "", "CPU1_PROCHOT_N", "",
> +                       "", "", "IRQ_SMI_ACTIVE_N", "FM_BIOS_POST_CMPLT_N",
> +               /* AB */ "", "", "ME_OVERRIDE", "BMC_DMI_MODIFY",
> +                       "", "", "", "",
> +               /* AC */ "LAD0", "LAD1", "LAD2", "LAD3",
> +                       "CK_33M_BMC", "LFRAME", "SERIRQ", "S_PLTRST";
> +
> +       /* Assert BMC_READY so BIOS doesn't sit around waiting for it */
> +       bmc-ready {
> +               gpio-hog;
> +               gpios = <ASPEED_GPIO(J, 0) GPIO_ACTIVE_LOW>;
> +               output-high;
> +       };
> +};
> +
> +&adc {
> +       status = "okay";
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_adc0_default
> +                       &pinctrl_adc1_default
> +                       &pinctrl_adc2_default
> +                       &pinctrl_adc3_default
> +                       &pinctrl_adc4_default
> +                       &pinctrl_adc5_default
> +                       &pinctrl_adc6_default
> +                       &pinctrl_adc7_default
> +                       &pinctrl_adc8_default
> +                       &pinctrl_adc9_default
> +                       &pinctrl_adc10_default
> +                       &pinctrl_adc11_default
> +                       &pinctrl_adc12_default>;
> +};
> +
> +&kcs3 {
> +       status = "okay";
> +       aspeed,lpc-io-reg = <0xca2>;
> +};
> --
> 2.31.1
>

Re: [PATCH] ARM: dts: aspeed: add ASRock E3C246D4I BMC

[Zev Weiss]

On Wed, Mar 31, 2021 at 10:51:42PM CDT, Joel Stanley wrote:
>Hi Zev,
>
>On Thu, 1 Apr 2021 at 02:57, Zev Weiss <zev@bewilderbeest.net> wrote:
>>
>> This is a relatively low-cost AST2500-based Xeon E-2100/E-2200 series
>> mini-ITX board that we hope can provide a decent platform for OpenBMC
>> development.
>>
>> This initial device-tree provides the necessary configuration for
>> basic BMC functionality such as host power control, serial console and
>> KVM support, and POST code snooping.
>
>The patch looks good! Some minor things below.
>
>When sending subsequent versions, make sure to add -v N to your git
>format-patch to mark it as the Nth version.
>
>You've also set this to be threaded with a previous version of the
>patch. We normally don't do that, and in this case it's doubly
>confusing as you've split this patch out from the previous series.
>
>I noticed you cc'd soc@kernel.org. We normally only do this when we
>want the soc maintainers to apply a patch directly without going
>through another maintainer. In this case the patch should go through
>the aspeed maintainer's tree (me), so you don't need to cc that
>address.
>

Hmm, that came from using './scripts/get_maintainer.pl --no-rolestats' 
with git send-email's --cc-cmd flag; does there happen to be a similarly 
easy alternative that wouldn't do the "wrong" thing there?

Ack on the rest, will send v3 soon.


Thanks,
Zev

>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>> ---
>
>This spot just here is where you should put the changes between v1 and v2.
>
>>  arch/arm/boot/dts/Makefile                    |   1 +
>>  .../boot/dts/aspeed-bmc-asrock-e3c246d4i.dts  | 202 ++++++++++++++++++
>>  2 files changed, 203 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index 8e5d4ab4e75e..b12911262ca1 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -1406,6 +1406,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>>         aspeed-bmc-ampere-mtjade.dtb \
>>         aspeed-bmc-arm-centriq2400-rep.dtb \
>>         aspeed-bmc-arm-stardragon4800-rep2.dtb \
>> +       aspeed-bmc-asrock-e3c246d4i.dts \
>
>This should be the output name (.dtb).
>
>>         aspeed-bmc-bytedance-g220a.dtb \
>>         aspeed-bmc-facebook-cmm.dtb \
>>         aspeed-bmc-facebook-galaxy100.dtb \
>> diff --git a/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
>> new file mode 100644
>> index 000000000000..dcab6e78dfa4
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
>
>The device tree itself looks good!
>
>If you fix up the things I mentioned and send a v3 I will apply it.
>
>Cheers,
>
>Joel
>
>> @@ -0,0 +1,202 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/dts-v1/;
>> +
>> +#include "aspeed-g5.dtsi"
>> +#include <dt-bindings/gpio/aspeed-gpio.h>
>> +#include <dt-bindings/i2c/i2c.h>
>> +
>> +/{
>> +       model = "ASRock E3C246D4I BMC";
>> +       compatible = "asrock,e3c246d4i-bmc", "aspeed,ast2500";
>> +
>> +       aliases {
>> +               serial4 = &uart5;
>> +       };
>> +
>> +       chosen {
>> +               stdout-path = &uart5;
>> +               bootargs = "console=tty0 console=ttyS4,115200 earlyprintk";
>> +       };
>> +
>> +       memory@80000000 {
>> +               reg = <0x80000000 0x20000000>;
>> +       };
>> +
>> +       leds {
>> +               compatible = "gpio-leds";
>> +
>> +               heartbeat {
>> +                       /* BMC_HB_LED_N */
>> +                       gpios = <&gpio ASPEED_GPIO(H, 6) GPIO_ACTIVE_LOW>;
>> +                       linux,default-trigger = "timer";
>> +               };
>> +
>> +               system-fault {
>> +                       /* SYSTEM_FAULT_LED_N */
>> +                       gpios = <&gpio ASPEED_GPIO(Z, 2) GPIO_ACTIVE_LOW>;
>> +                       panic-indicator;
>> +               };
>> +       };
>> +
>> +       gpio-keys {
>> +               compatible = "gpio-keys";
>> +
>> +               uid-button {
>> +                       label = "uid-button";
>> +                       gpios = <&gpio ASPEED_GPIO(F, 1) GPIO_ACTIVE_LOW>;
>> +                       linux,code = <ASPEED_GPIO(F, 1)>;
>> +               };
>> +       };
>> +
>> +       iio-hwmon {
>> +               compatible = "iio-hwmon";
>> +               io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>, <&adc 4>,
>> +                       <&adc 5>, <&adc 6>, <&adc 7>, <&adc 8>, <&adc 9>,
>> +                       <&adc 10>, <&adc 11>, <&adc 12>;
>> +       };
>> +};
>> +
>> +&fmc {
>> +       status = "okay";
>> +       flash@0 {
>> +               status = "okay";
>> +               m25p,fast-read;
>> +               label = "bmc";
>> +               spi-max-frequency = <100000000>; /* 100 MHz */
>> +#include "openbmc-flash-layout.dtsi"
>> +       };
>> +};
>> +
>> +&uart5 {
>> +       status = "okay";
>> +};
>> +
>> +&vuart {
>> +       status = "okay";
>> +       aspeed,sirq-active-high;
>> +};
>> +
>> +&mac0 {
>> +       status = "okay";
>> +
>> +       pinctrl-names = "default";
>> +       pinctrl-0 = <&pinctrl_rgmii1_default &pinctrl_mdio1_default>;
>> +};
>> +
>> +&i2c1 {
>> +       status = "okay";
>> +
>> +       /* thermal sensor, one diode run to a disconnected header */
>> +       w83773g@4c {
>> +               compatible = "nuvoton,w83773g";
>> +               reg = <0x4c>;
>> +       };
>> +};
>> +
>> +&i2c3 {
>> +       status = "okay";
>> +
>> +       /* FRU EEPROM */
>> +       eeprom@57 {
>> +               compatible = "st,24c128", "atmel,24c128";
>> +               reg = <0x57>;
>> +               pagesize = <16>;
>> +       };
>> +};
>> +
>> +&video {
>> +       status = "okay";
>> +};
>> +
>> +&vhub {
>> +       status = "okay";
>> +};
>> +
>> +&lpc_ctrl {
>> +       status = "okay";
>> +};
>> +
>> +&lpc_snoop {
>> +       status = "okay";
>> +       snoop-ports = <0x80>;
>> +};
>> +
>> +&gpio {
>> +       status = "okay";
>> +       gpio-line-names =
>> +               /*  A */ "BMC_MAC1_INTB", "BMC_MAC2_INTB", "NMI_BTN_N", "BMC_NMI",
>> +                       "", "", "", "",
>> +               /*  B */ "", "", "", "", "", "IRQ_BMC_PCH_SMI_LPC_N", "", "",
>> +               /*  C */ "", "", "", "", "", "", "", "",
>> +               /*  D */ "BMC_PSIN", "BMC_PSOUT", "BMC_RESETCON", "RESETCON",
>> +                       "", "", "", "",
>> +               /*  E */ "", "", "", "", "", "", "", "",
>> +               /*  F */ "LOCATORLED_STATUS_N", "LOCATORBTN", "", "",
>> +                       "", "", "BMC_PCH_SCI_LPC", "BMC_NCSI_MUX_CTL",
>> +               /*  G */ "HWM_BAT_EN", "CHASSIS_ID0", "CHASSIS_ID1", "CHASSIS_ID2",
>> +                       "BMC_ALERT1_N_R", "BMC_ALERT2_N_R", "BMC_ALERT3_N", "SML0ALERT",
>> +               /*  H */ "FM_ME_RCVR_N", "O_PWROK", "SKL_CNL_R", "D4_DIMM_EVENT_3V_N",
>> +                       "MFG_MODE_N", "BMC_RTCRST", "BMC_HB_LED_N", "BMC_CASEOPEN",
>> +               /*  I */ "", "", "", "", "", "", "", "",
>> +               /*  J */ "BMC_READY", "BMC_PCH_BIOS_CS_N", "BMC_SMI", "",
>> +                       "", "", "", "",
>> +               /*  K */ "", "", "", "", "", "", "", "",
>> +               /*  L */ "BMC_CTS1", "BMC_DCD1", "BMC_DSR1", "BMC_RI1",
>> +                       "BMC_DTR1", "BMC_RTS1", "BMC_TXD1", "BMC_RXD1",
>> +               /*  M */ "BMC_LAN0_DIS_N", "BMC_LAN1_DIS_N", "", "",
>> +                       "", "", "", "",
>> +               /*  N */ "", "", "", "", "", "", "", "",
>> +               /*  O */ "", "", "", "", "", "", "", "",
>> +               /*  P */ "", "", "", "", "", "", "", "",
>> +               /*  Q */ "", "", "", "",
>> +                       "BMC_SBM_PRESENT_1_N", "BMC_SBM_PRESENT_2_N",
>> +                       "BMC_SBM_PRESENT_3_N", "BMC_PCIE_WAKE_N",
>> +               /*  R */ "", "", "", "", "", "", "", "",
>> +               /*  S */ "PCHHOT_BMC_N", "", "RSMRST",
>> +                       "", "", "", "", "",
>> +               /*  T */ "", "", "", "", "", "", "", "",
>> +               /*  U */ "", "", "", "", "", "", "", "",
>> +               /*  V */ "", "", "", "", "", "", "", "",
>> +               /*  W */ "PS_PWROK", /* dummy always-high signal */
>> +                       "", "", "", "", "", "", "",
>> +               /*  X */ "", "", "", "", "", "", "", "",
>> +               /*  Y */ "SLP_S3", "SLP_S5", "", "", "", "", "", "",
>> +               /*  Z */ "CPU_CATERR_BMC_PCH_N", "", "SYSTEM_FAULT_LED_N", "BMC_THROTTLE_N",
>> +                       "", "", "", "",
>> +               /* AA */ "CPU1_THERMTRIP_LATCH_N", "", "CPU1_PROCHOT_N", "",
>> +                       "", "", "IRQ_SMI_ACTIVE_N", "FM_BIOS_POST_CMPLT_N",
>> +               /* AB */ "", "", "ME_OVERRIDE", "BMC_DMI_MODIFY",
>> +                       "", "", "", "",
>> +               /* AC */ "LAD0", "LAD1", "LAD2", "LAD3",
>> +                       "CK_33M_BMC", "LFRAME", "SERIRQ", "S_PLTRST";
>> +
>> +       /* Assert BMC_READY so BIOS doesn't sit around waiting for it */
>> +       bmc-ready {
>> +               gpio-hog;
>> +               gpios = <ASPEED_GPIO(J, 0) GPIO_ACTIVE_LOW>;
>> +               output-high;
>> +       };
>> +};
>> +
>> +&adc {
>> +       status = "okay";
>> +       pinctrl-names = "default";
>> +       pinctrl-0 = <&pinctrl_adc0_default
>> +                       &pinctrl_adc1_default
>> +                       &pinctrl_adc2_default
>> +                       &pinctrl_adc3_default
>> +                       &pinctrl_adc4_default
>> +                       &pinctrl_adc5_default
>> +                       &pinctrl_adc6_default
>> +                       &pinctrl_adc7_default
>> +                       &pinctrl_adc8_default
>> +                       &pinctrl_adc9_default
>> +                       &pinctrl_adc10_default
>> +                       &pinctrl_adc11_default
>> +                       &pinctrl_adc12_default>;
>> +};
>> +
>> +&kcs3 {
>> +       status = "okay";
>> +       aspeed,lpc-io-reg = <0xca2>;
>> +};
>> --
>> 2.31.1
>>

Re: [PATCH 3/3] ARM: dts: aspeed: add ASRock E3C246D4I BMC

[Andrew Jeffery]

On Tue, 30 Mar 2021, at 10:53, Zev Weiss wrote:
> This is a relatively low-cost AST2500-based Xeon E-2100/E-2200 series
> mini-ITX board that we hope can provide a decent platform for OpenBMC
> development.
> 
> This initial device-tree provides the necessary configuration for
> basic BMC functionality such as host power control, serial console and
> KVM support, and POST code snooping.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  .../boot/dts/aspeed-bmc-asrock-e3c246d4i.dts  | 188 ++++++++++++++++++
>  1 file changed, 188 insertions(+)
>  create mode 100644 arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
> 
> diff --git a/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts 
> b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
> new file mode 100644
> index 000000000000..27b34c3cf67a
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
> @@ -0,0 +1,188 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/dts-v1/;
> +
> +#include "aspeed-g5.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +#include <dt-bindings/i2c/i2c.h>
> +
> +/{
> +	model = "ASRock E3C246D4I BMC";
> +	compatible = "aspeed,ast2500";
> +
> +	aliases {
> +		serial4 = &uart5;
> +	};
> +
> +	chosen {
> +		stdout-path = &uart5;
> +		bootargs = "console=tty0 console=ttyS4,115200 earlyprintk";
> +	};
> +
> +	memory@80000000 {
> +		reg = <0x80000000 0x20000000>;
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		heartbeat {
> +			/* BMC_HB_LED_N */
> +			gpios = <&gpio ASPEED_GPIO(H, 6) GPIO_ACTIVE_LOW>;
> +			linux,default-trigger = "timer";
> +		};
> +
> +		system-fault {
> +			/* SYSTEM_FAULT_LED_N */
> +			gpios = <&gpio ASPEED_GPIO(Z, 2) GPIO_ACTIVE_LOW>;
> +			panic-indicator;
> +		};
> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +
> +		uid-button {
> +			label = "uid-button";
> +			gpios = <&gpio ASPEED_GPIO(F, 1) GPIO_ACTIVE_LOW>;
> +			linux,code = <ASPEED_GPIO(F, 1)>;
> +		};
> +	};
> +
> +	iio-hwmon {
> +		compatible = "iio-hwmon";
> +		io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>, <&adc 4>,
> +			<&adc 5>, <&adc 6>, <&adc 7>, <&adc 8>, <&adc 9>,
> +			<&adc 10>, <&adc 11>, <&adc 12>;
> +	};
> +};

You're hooking up the ADC lines to the iio-hwmon bridge...
> +
> +&adc {
> +	status = "okay";
> +};

But you haven't requested the ADC lines from pinmux here.

It will *happen* to work as expected because ADC is the default mux 
state for the pins, but by not requesting the lines you're leaving the 
pins available for a conflicting request, which can be annoying to 
debug.

> +
> +&kcs3 {
> +	status = "okay";
> +	aspeed,lpc-io-reg = <0xca2>;
> +};

Given you need KCS support, do you mind testing my KCS series?

https://lore.kernel.org/linux-arm-kernel/20210319062752.145730-1-andrew@aj.id.au/

The cover letter got detached, and is here:

https://lore.kernel.org/linux-arm-kernel/20210319061952.145040-1-andrew@aj.id.au/

Andrew

Re: [PATCH 3/3] ARM: dts: aspeed: add ASRock E3C246D4I BMC

[Zev Weiss]

On Wed, Mar 31, 2021 at 07:04:51PM CDT, Andrew Jeffery wrote:
>
>
>On Tue, 30 Mar 2021, at 10:53, Zev Weiss wrote:
>> This is a relatively low-cost AST2500-based Xeon E-2100/E-2200 series
>> mini-ITX board that we hope can provide a decent platform for OpenBMC
>> development.
>>
>> This initial device-tree provides the necessary configuration for
>> basic BMC functionality such as host power control, serial console and
>> KVM support, and POST code snooping.
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>  .../boot/dts/aspeed-bmc-asrock-e3c246d4i.dts  | 188 ++++++++++++++++++
>>  1 file changed, 188 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
>>
>> diff --git a/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
>> b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
>> new file mode 100644
>> index 000000000000..27b34c3cf67a
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/aspeed-bmc-asrock-e3c246d4i.dts
>> @@ -0,0 +1,188 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/dts-v1/;
>> +
>> +#include "aspeed-g5.dtsi"
>> +#include <dt-bindings/gpio/aspeed-gpio.h>
>> +#include <dt-bindings/i2c/i2c.h>
>> +
>> +/{
>> +	model = "ASRock E3C246D4I BMC";
>> +	compatible = "aspeed,ast2500";
>> +
>> +	aliases {
>> +		serial4 = &uart5;
>> +	};
>> +
>> +	chosen {
>> +		stdout-path = &uart5;
>> +		bootargs = "console=tty0 console=ttyS4,115200 earlyprintk";
>> +	};
>> +
>> +	memory@80000000 {
>> +		reg = <0x80000000 0x20000000>;
>> +	};
>> +
>> +	leds {
>> +		compatible = "gpio-leds";
>> +
>> +		heartbeat {
>> +			/* BMC_HB_LED_N */
>> +			gpios = <&gpio ASPEED_GPIO(H, 6) GPIO_ACTIVE_LOW>;
>> +			linux,default-trigger = "timer";
>> +		};
>> +
>> +		system-fault {
>> +			/* SYSTEM_FAULT_LED_N */
>> +			gpios = <&gpio ASPEED_GPIO(Z, 2) GPIO_ACTIVE_LOW>;
>> +			panic-indicator;
>> +		};
>> +	};
>> +
>> +	gpio-keys {
>> +		compatible = "gpio-keys";
>> +
>> +		uid-button {
>> +			label = "uid-button";
>> +			gpios = <&gpio ASPEED_GPIO(F, 1) GPIO_ACTIVE_LOW>;
>> +			linux,code = <ASPEED_GPIO(F, 1)>;
>> +		};
>> +	};
>> +
>> +	iio-hwmon {
>> +		compatible = "iio-hwmon";
>> +		io-channels = <&adc 0>, <&adc 1>, <&adc 2>, <&adc 3>, <&adc 4>,
>> +			<&adc 5>, <&adc 6>, <&adc 7>, <&adc 8>, <&adc 9>,
>> +			<&adc 10>, <&adc 11>, <&adc 12>;
>> +	};
>> +};
>
>You're hooking up the ADC lines to the iio-hwmon bridge...
>> +
>> +&adc {
>> +	status = "okay";
>> +};
>
>But you haven't requested the ADC lines from pinmux here.
>
>It will *happen* to work as expected because ADC is the default mux
>state for the pins, but by not requesting the lines you're leaving the
>pins available for a conflicting request, which can be annoying to
>debug.
>

Ack, thanks -- will fix & resend.

>> +
>> +&kcs3 {
>> +	status = "okay";
>> +	aspeed,lpc-io-reg = <0xca2>;
>> +};
>
>Given you need KCS support, do you mind testing my KCS series?
>
>https://lore.kernel.org/linux-arm-kernel/20210319062752.145730-1-andrew@aj.id.au/
>

Sure, I'll try to give that a shot and report back in the next day or 
two.

>The cover letter got detached, and is here:
>
>https://lore.kernel.org/linux-arm-kernel/20210319061952.145040-1-andrew@aj.id.au/
>
>Andrew