Re: [PATCH linux dev-5.0] ARM: dts: aspeed: Initial git pull request for Microsoft Olympus BMC

[Joel Stanley <joel@jms.id.au>]

Hi Hongwei,

Thanks for the patch. It looks good. I've made some minor suggestions
below that we should try to fix before merging.

The subject needs work. Take a look at the other patches:

git log --oneline arch/arm/boot/dts/

Something like:

ARM: dts: aspeed: Add Microsoft Olympus OCP BMC

On Thu, 25 Apr 2019 at 18:29, Hongwei Zhang <hongweiz@ami.com> wrote:
>
> Olympus is a Microsoft OCP platform equipped with Aspeed 2400 BMC
> SoC.

Nice.

> Tested: meta-olympus has been tested on an ASPEED AST2400 EVB board
>         and MT Olympus server.
>         The U-boot and kernel start and run as expected.

This is nice to know, however we don't generally put this in the
commit message for the kernel. It's up to you.

>
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> ---
>  arch/arm/boot/dts/Makefile                   |   3 +-
>  arch/arm/boot/dts/aspeed-bmc-opp-olympus.dts | 219 +++++++++++++++++++++++++++
>  2 files changed, 221 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/boot/dts/aspeed-bmc-opp-olympus.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index bd40148..34c0b7a0 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1247,4 +1247,5 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
>         aspeed-bmc-opp-witherspoon.dtb \
>         aspeed-bmc-opp-zaius.dtb \
>         aspeed-bmc-portwell-neptune.dtb \
> -       aspeed-bmc-quanta-q71l.dtb
> +       aspeed-bmc-quanta-q71l.dtb \
> +       aspeed-bmc-opp-olympus.dts

'opp' means openpower. I think Olympus is an x86 system, so it should
not have the opp prefix. The vendor (microsoft?) would be appropriate.

This list is sorted alphabetically.

> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-olympus.dts b/arch/arm/boot/dts/aspeed-bmc-opp-olympus.dts
> new file mode 100644
> index 0000000..8b4b00d0
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-olympus.dts
> @@ -0,0 +1,219 @@
> +//SPDX-License-Identifier: GPL-2.0+

Missing  space. It should look something like this:

// SPDX-License-Identifier: GPL-2.0+
// Copyright (C) Company Name 2019

> +/dts-v1/;
> +
> +#include "aspeed-g4.dtsi"
> +#include <dt-bindings/gpio/aspeed-gpio.h>
> +
> +/ {
> +       model = "Olympus BMC";
> +       compatible = "microsoft,olympus-bmc", "aspeed,ast2400";
> +
> +       chosen {
> +               stdout-path = &uart5;
> +               bootargs = "console=ttyS4,115200 earlyprintk";
> +       };
> +
> +       memory@40000000 {
> +               reg = <0x40000000 0x20000000>;
> +       };
> +
> +       reserved-memory {
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges;
> +
> +               vga_memory: framebuffer@5f000000 {
> +                       no-map;
> +                       reg = <0x5f000000 0x01000000>; /* 16M */
> +               };
> +
> +               flash_memory: region@98000000 {

See my comments below about this node.

> +                       no-map;
> +                       reg = <0x98000000 0x01000000>; /* 16MB */
> +               };
> +       };
> +

> +&i2c4 {
> +       status = "okay";
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +       clock-frequency = <100000>;
> +
> +       slave-mqueue@20 {
> +               compatible = "slave-ipmb";

Is this for a driver you have not yet submitted?

If you add device tree bindings for drivers that are not in the tree
then you run the risk of changes being made in review that cause the
two to be out of sync. I suggest submitting your driver for inclusion.

> +               reg = <0x40000020>;
> +       };
> +};

> +
> +&lpc_ctrl {
> +       status = "okay";
> +       memory-region = <&flash_memory>;

Are you sure you use this feature of the driver?

If you are adding this only to enable the lpc clocks, the driver has
recently been modified so the memory-region is not required.

> +       flash = <&spi>;
> +};

> --
> 2.7.4
>
>
> Please consider the environment before printing this email.
>
> The information contained in this message may be confidential and proprietary to American Megatrends, Inc.  This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited.  Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission.

This message doe snot make sense in an email posted to the public list.