Hello Howard,
Thanks for supplying this. I have a few comments below.
On Wed, Nov 03, 2021 at 03:14:18PM +0800, Howard Chiu wrote:
> Initial introduction of Facebook Bletchley equipped with
> Aspeed 2600 BMC SoC.
>
> Signed-off-by: Howard Chiu <howard.chiu@xxxxxxxxxxxx>
> ---
> arch/arm/boot/dts/Makefile | 1 +
> .../dts/aspeed-bmc-facebook-bletchley.dts | 1160 +++++++++++++++++
> 2 files changed, 1161 insertions(+)
> create mode 100644 arch/arm/boot/dts/aspeed-bmc-facebook-bletchley.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 7e0934180724..2cc2d804e75a 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1474,6 +1474,7 @@ dtb-$(CONFIG_ARCH_ASPEED) += \
> aspeed-bmc-facebook-wedge400.dtb \
> aspeed-bmc-facebook-yamp.dtb \
> aspeed-bmc-facebook-yosemitev2.dtb \
> + aspeed-bmc-facebook-bletchley.dtb \
> aspeed-bmc-ibm-everest.dtb \
> aspeed-bmc-ibm-rainier.dtb \
> aspeed-bmc-ibm-rainier-1s4u.dtb \
I believe the preference is to keep these sorted.
> diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-bletchley.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-bletchley.dts
> new file mode 100644
> index 000000000000..af30be95fb23
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-bletchley.dts
> +
> + chosen {
> + bootargs = "console=ttyS4,115200n8";
> + };
Do we want this to be 115200 or 57600?
> + fan1_ember {
> + retain-state-shutdown;
> + default-state = "off";
> + gpios = <&fan_ioexp 13 GPIO_ACTIVE_HIGH>;
I see a number of references to 'ember'/'EMBER'. I think the intention is
'amber'.
amber: a honey-yellow color typical of amber
or a yellow light used as a cautionary signal
ember: a small piece of burning or glowing coal or wood in a dying fire.
> +&fmc {
> + status = "okay";
> + flash@0 {
> + status = "okay";
> + m25p,fast-read;
> + label = "bmc";
> + spi-max-frequency = <50000000>;
> +#include "openbmc-flash-layout-64.dtsi"
Is this board using 64MB or 128MB modules? Many of the newer systems have been
starting to use 128MB. I just want to confirm this is correct.
> + sled0_ioexp: pca9539@76 {
> + compatible = "nxp,pca9539";
> + reg = <0x76>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + gpio-controller;
> + #gpio-cells = <2>;
> +
> + gpio-line-names =
> + "","SLED0_BMC_CCG5_INT","SLED0_INA230_ALERT","SLED0_P12V_STBY_ALERT",
> + "SLED0_SSD_ALERT","SLED0_MS_DETECT","SLED0_MD_REF_PWM","",
> + "SLED0_MD_STBY_RESET","SLED0_MD_IOEXP_EN_FAULT","SLED0_MD_DIR","SLED0_MD_DECAY",
> + "SLED0_MD_MODE1","SLED0_MD_MODE2","SLED0_MD_MODE3","SLED0_AC_PWR_EN";
In general, in OpenBMC, we have a preference for the GPIOs to not be schematic
names but to be named based on their [software-oriented] function. Please take
a look at:
https://github.com/openbmc/docs/blob/master/designs/device-tree-gpio-naming.md
Any function you see that isn't documented there we should try to get documented
before fixing the GPIO name to match it.
> + gpio-line-names =
> + "SLED0_EMBER_LED","SLED0_BLUE_LED","SLED0_RST_IOEXP","",
The LEDs are ones I know are already documented in the above linked file.
> +&i2c13 {
> + multi-master;
> + aspeed,hw-timeout-ms = <1000>;
> + status = "okay";
> +};
Was this intentional to have defined a multi-master bus with nothing on it?
--
Patrick Williams
Attachment:
signature.asc
Description: PGP signature
