2019-12-26 08:55:44

by Andrew Peng

[permalink] [raw]
Subject: [PATCH v1 1/1] ARM: dts: aspeed: update Hr855xg2 device tree

Update i2c aliases.
Change flash_memory mapping address and size.
Add in a gpio-keys section.
Enable vhub, vuart, spi1 and spi2.
Add raa228006, ir38164 and sn1701022 hwmon sensors.
Remove some unuse gpio from gpio section.

Signed-off-by: Andrew Peng <[email protected]>
Signed-off-by: Derek Lin <[email protected]>
Signed-off-by: Yonghui Liu <[email protected]>
---
v1: initial version

arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts | 557 ++++++++++++++++-------
1 file changed, 382 insertions(+), 175 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts b/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
index 8193fad..e1386d4 100644
--- a/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
@@ -15,14 +15,21 @@
compatible = "lenovo,hr855xg2-bmc", "aspeed,ast2500";

aliases {
- i2c14 = &i2c_riser1;
- i2c15 = &i2c_riser2;
- i2c16 = &i2c_riser3;
- i2c17 = &i2c_M2;
- i2c18 = &channel_0;
- i2c19 = &channel_1;
- i2c20 = &channel_2;
- i2c21 = &channel_3;
+ i2c14 = &pcie_slot8;
+ i2c15 = &pcie_slot9;
+ i2c16 = &pcie_slot10;
+ i2c17 = &pcie_slot11;
+ i2c18 = &pcie_slot12;
+ i2c19 = &switch0_i2c5;
+ i2c22 = &switch1_i2c0;
+ i2c23 = &pcie_slot6;
+ i2c24 = &pcie_slot7;
+ i2c30 = &pcie_slot1;
+ i2c31 = &pcie_slot2;
+ i2c32 = &pcie_slot3;
+ i2c33 = &pcie_slot4;
+ i2c34 = &pcie_slot5;
+ i2c35 = &switch2_i2c5;
};

chosen {
@@ -40,9 +47,9 @@
#size-cells = <1>;
ranges;

- flash_memory: region@98000000 {
+ flash_memory: region@9EFF0000 {
no-map;
- reg = <0x98000000 0x00100000>; /* 1M */
+ reg = <0x9EFF0000 0x00010000>; /* 64K */
};

gfx_memory: framebuffer {
@@ -78,6 +85,87 @@
io-channels = <&adc 15>;
};

+ gpio-keys {
+ compatible = "gpio-keys";
+
+ id-button {
+ label = "id-button";
+ gpios = <&gpio ASPEED_GPIO(Y, 2) GPIO_ACTIVE_LOW>;
+ linux,code = <ASPEED_GPIO(Y, 2)>;
+ };
+
+ pwr-button {
+ label = "pwr-button";
+ gpios = <&gpio ASPEED_GPIO(I, 1) GPIO_ACTIVE_LOW>;
+ linux,code = <ASPEED_GPIO(I, 1)>;
+ };
+
+ cpu-caterr {
+ label = "cpu-caterr";
+ gpios = <&gpio ASPEED_GPIO(G, 1) GPIO_ACTIVE_LOW>;
+ linux,code = <ASPEED_GPIO(G, 1)>;
+ };
+
+ int-fpga-bmc {
+ label = "int-fpga-bmc";
+ gpios = <&gpio ASPEED_GPIO(F, 5) GPIO_ACTIVE_LOW>;
+ linux,code = <ASPEED_GPIO(F, 5)>;
+ };
+
+ pdb-alt-n {
+ label = "pdb-alt-n";
+ gpios = <&gpio ASPEED_GPIO(AA, 1) GPIO_ACTIVE_LOW>;
+ linux,code = <ASPEED_GPIO(AA, 1)>;
+ };
+
+ p12v-aux1-alert1-n {
+ label = "p12v-aux1-alert1-n";
+ gpios = <&gpio ASPEED_GPIO(AA, 7) GPIO_ACTIVE_LOW>;
+ linux,code = <ASPEED_GPIO(AA, 7)>;
+ };
+
+ p12v-aux2-alert1-n {
+ label = "p12v-aux2-alert1-n";
+ gpios = <&gpio ASPEED_GPIO(J, 0) GPIO_ACTIVE_LOW>;
+ linux,code = <ASPEED_GPIO(J, 0)>;
+ };
+
+ p12v-aux3-alert1-n {
+ label = "p12v-aux3-alert1-n";
+ gpios = <&gpio ASPEED_GPIO(G, 5) GPIO_ACTIVE_LOW>;
+ linux,code = <ASPEED_GPIO(G, 5)>;
+ };
+
+ ddr-vr-bmc-alert-n {
+ label = "ddr-vr-bmc-alert-n";
+ gpios = <&gpio ASPEED_GPIO(L, 7) GPIO_ACTIVE_LOW>;
+ linux,code = <ASPEED_GPIO(L, 7)>;
+ };
+
+ cpu-vr-bmc-alert-n {
+ label = "cpu-vr-bmc-alert-n";
+ gpios = <&gpio ASPEED_GPIO(L, 6) GPIO_ACTIVE_LOW>;
+ linux,code = <ASPEED_GPIO(L, 6)>;
+ };
+
+ riser1-vr-al-r {
+ label = "riser1-vr-al-r";
+ gpios = <&gpio ASPEED_GPIO(AB, 1) GPIO_ACTIVE_LOW>;
+ linux,code = <ASPEED_GPIO(AB, 1)>;
+ };
+
+ riser2-vr-al-r {
+ label = "riser2-vr-al-r";
+ gpios = <&gpio ASPEED_GPIO(F, 1) GPIO_ACTIVE_LOW>;
+ linux,code = <ASPEED_GPIO(F, 1)>;
+ };
+
+ riser3-vr-al-r {
+ label = "riser3-vr-al-r";
+ gpios = <&gpio ASPEED_GPIO(A, 1) GPIO_ACTIVE_LOW>;
+ linux,code = <ASPEED_GPIO(A, 1)>;
+ };
+ };
};

&fmc {
@@ -91,10 +179,13 @@
};
};

+&vhub {
+ status = "okay";
+};
+
&lpc_ctrl {
status = "okay";
memory-region = <&flash_memory>;
- flash = <&spi1>;
};

&lpc_snoop {
@@ -102,11 +193,39 @@
snoop-ports = <0x80>;
};

-&uart1 {
+&spi1 {
+ status = "okay";
+ flash@0 {
+ status = "okay";
+ m25p,fast-read;
+ label = "pnor";
+ spi-max-frequency = <40000000>;
+ };
+};
+
+&spi2 {
status = "okay";
pinctrl-names = "default";
- pinctrl-0 = <&pinctrl_txd1_default
- &pinctrl_rxd1_default>;
+ pinctrl-0 = <&pinctrl_spi2ck_default
+ &pinctrl_spi2cs0_default
+ &pinctrl_spi2miso_default
+ &pinctrl_spi2mosi_default>;
+
+ spidev@0 {
+ status = "okay";
+ compatible = "aspeed,spidev";
+ reg = < 0 >;
+ spi-max-frequency = <50000000>;
+ };
+
+ flash@0 {
+ compatible = "jedec,spi-nor";
+ m25p,fast-read;
+ label = "fpga";
+ reg = < 0 >;
+ spi-max-frequency = <50000000>;
+ status = "okay";
+ };
};

&uart2 {
@@ -123,12 +242,14 @@
&pinctrl_nri2_default>;
};

-&uart3 {
+&uart5 {
status = "okay";
};

-&uart5 {
+&vuart {
status = "okay";
+ auto-flow-control;
+ espi-enabled = <&syscon 0x70 25>;
};

&ibt {
@@ -140,7 +261,7 @@
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_rmii1_default>;
clocks = <&syscon ASPEED_CLK_GATE_MAC1CLK>,
- <&syscon ASPEED_CLK_GATE_MAC1RCLK>;
+ <&syscon ASPEED_CLK_MAC1RCLK>;
clock-names = "MACCLK", "RCLK";
use-ncsi;
};
@@ -172,37 +293,77 @@
&pinctrl_adc15_default>;
};

+&peci0 {
+ status = "okay";
+ peci-client@30 {
+ compatible = "intel,peci-client";
+ reg = <0x30>;
+ };
+
+ peci-client@31 {
+ compatible = "intel,peci-client";
+ reg = <0x31>;
+ };
+
+ peci-client@32 {
+ compatible = "intel,peci-client";
+ reg = <0x32>;
+ };
+
+ peci-client@33 {
+ compatible = "intel,peci-client";
+ reg = <0x33>;
+ };
+};
+
&i2c0 {
status = "okay";

- i2c-switch@70 {
- compatible = "nxp,pca9545";
- reg = <0x70>;
+ i2c-switch@71 {
+ compatible = "nxp,pca9548";
#address-cells = <1>;
#size-cells = <0>;
+ reg = <0x71>;

- i2c_riser1: i2c@0 {
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <0>;
+ pcie_slot8: i2c@0{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
};

- i2c_riser2: i2c@1 {
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <1>;
+ pcie_slot9: i2c@1{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
};

- i2c_riser3: i2c@2 {
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <2>;
+ pcie_slot10: i2c@2{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
};

- i2c_M2: i2c@3 {
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <3>;
+ pcie_slot11: i2c@3{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+ };
+
+ pcie_slot12: i2c@4{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <4>;
+ };
+
+ switch0_i2c5:i2c@5{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <5>;
+ eeprom@54 {
+ compatible = "atmel,24c04";
+ pagesize = <16>;
+ reg = <0x54>;
+ };
};
};
};
@@ -216,13 +377,43 @@
};

VR@45 {
- compatible = "pmbus";
+ compatible = "raa228006";
reg = <0x45>;
};
};

&i2c2 {
status = "okay";
+
+ i2c-switch@71 {
+ compatible = "nxp,pca9545";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x71>;
+
+ switch1_i2c0:i2c@0{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+ eeprom@54 {
+ compatible = "atmel,24c04";
+ pagesize = <16>;
+ reg = <0x54>;
+ };
+ };
+
+ pcie_slot6:i2c@1{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+ };
+
+ pcie_slot7:i2c@2{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+ };
+ };
};

&i2c3 {
@@ -265,6 +456,66 @@

&i2c5 {
status = "okay";
+
+ CPU0_VCCIN@60 {
+ compatible = "raa228006";
+ reg = <0x60>;
+ };
+
+ CPU1_VCCIN@62 {
+ compatible = "raa228006";
+ reg = <0x62>;
+ };
+
+ CPU2_VCCIN@64 {
+ compatible = "raa228006";
+ reg = <0x64>;
+ };
+
+ CPU3_VCCIN@66 {
+ compatible = "raa228006";
+ reg = <0x66>;
+ };
+
+ CPU0_VCCSA@46 {
+ compatible = "ir38164";
+ reg = <0x46>;
+ };
+
+ CPU1_VCCSA@47 {
+ compatible = "ir38164";
+ reg = <0x47>;
+ };
+
+ CPU2_VCCSA@48 {
+ compatible = "ir38164";
+ reg = <0x48>;
+ };
+
+ CPU3_VCCSA@49 {
+ compatible = "ir38164";
+ reg = <0x49>;
+ };
+
+ CPU0_VCCIO@41 {
+ compatible = "ir38164";
+ reg = <0x41>;
+ };
+
+ CPU1_VCCIO@42 {
+ compatible = "ir38164";
+ reg = <0x42>;
+ };
+
+ CPU2_VCCIO@43 {
+ compatible = "ir38164";
+ reg = <0x43>;
+ };
+
+ CPU3_VCCIO@44 {
+ compatible = "ir38164";
+ reg = <0x44>;
+ };
};

&i2c6 {
@@ -284,7 +535,7 @@
eeprom@54 {
compatible = "atmel,24c256";
reg = <0x54>;
- pagesize = <16>;
+ pagesize = <64>;
};
};

@@ -306,10 +557,99 @@

&i2c11 {
status = "okay";
+
+ i2c-switch@71 {
+ compatible = "nxp,pca9548";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x71>;
+
+ pcie_slot1: i2c@0{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+ };
+
+ pcie_slot2: i2c@1{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+ };
+
+ pcie_slot3: i2c@2{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+ };
+
+ pcie_slot4: i2c@3{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+ };
+
+ pcie_slot5: i2c@4{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <4>;
+ };
+
+ switch2_i2c5:i2c@5{
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <5>;
+ eeprom@54 {
+ compatible = "atmel,24c04";
+ pagesize = <16>;
+ reg = <0x54>;
+ };
+ };
+ };
};

&i2c13 {
status = "okay";
+
+ CPU0_VDDQ_ABC@58 {
+ compatible = "sn1701022";
+ reg = <0x58>;
+ };
+
+ CPU0_VDDQ_DEF@5a {
+ compatible = "sn1701022";
+ reg = <0x5a>;
+ };
+
+ CPU1_VDDQ_ABC@5c {
+ compatible = "sn1701022";
+ reg = <0x5c>;
+ };
+
+ CPU1_VDDQ_DEF@5e {
+ compatible = "sn1701022";
+ reg = <0x5e>;
+ };
+
+ CPU2_VDDQ_ABC@68 {
+ compatible = "sn1701022";
+ reg = <0x68>;
+ };
+
+ CPU2_VDDQ_DEF@6a {
+ compatible = "sn1701022";
+ reg = <0x6a>;
+ };
+
+ CPU3_VDDQ_ABC@6c {
+ compatible = "sn1701022";
+ reg = <0x6c>;
+ };
+
+ CPU3_VDDQ_DEF@6e {
+ compatible = "sn1701022";
+ reg = <0x6e>;
+ };
+
};

&ehci1 {
@@ -425,20 +765,6 @@

&gpio {

- pin_gpio_a1 {
- gpio-hog;
- gpios = <ASPEED_GPIO(A, 1) GPIO_ACTIVE_LOW>;
- output-high;
- line-name = "BMC_EMMC_RST_N";
- };
-
- pin_gpio_a3 {
- gpio-hog;
- gpios = <ASPEED_GPIO(A, 3) GPIO_ACTIVE_LOW>;
- output-high;
- line-name = "PCH_PWROK_BMC_FPGA";
- };
-
pin_gpio_b5 {
gpio-hog;
gpios = <ASPEED_GPIO(B, 5) GPIO_ACTIVE_HIGH>;
@@ -453,27 +779,6 @@
line-name = "CPU_SM_WP";
};

- pin_gpio_e0 {
- gpio-hog;
- gpios = <ASPEED_GPIO(E, 0) GPIO_ACTIVE_HIGH>;
- input;
- line-name = "PDB_PSU_SEL";
- };
-
- pin_gpio_e2 {
- gpio-hog;
- gpios = <ASPEED_GPIO(E, 2) GPIO_ACTIVE_HIGH>;
- output-high;
- line-name = "LOCATOR_LED_N";
- };
-
- pin_gpio_e5 {
- gpio-hog;
- gpios = <ASPEED_GPIO(E, 5) GPIO_ACTIVE_HIGH>;
- output-high;
- line-name = "FM_BMC_DBP_PRESENT_R1_N";
- };
-
pin_gpio_e6 {
gpio-hog;
gpios = <ASPEED_GPIO(E, 6) GPIO_ACTIVE_HIGH>;
@@ -481,18 +786,11 @@
line-name = "BMC_ME_SECURITY_OVERRIDE_N";
};

- pin_gpio_f0 {
+ pin_gpio_g7 {
gpio-hog;
- gpios = <ASPEED_GPIO(F, 0) GPIO_ACTIVE_HIGH>;
+ gpios = <ASPEED_GPIO(G, 7) GPIO_ACTIVE_HIGH>;
output-high;
- line-name = "IRQ_BMC_PCH_NMI_R";
- };
-
- pin_gpio_f1 {
- gpio-hog;
- gpios = <ASPEED_GPIO(F, 1) GPIO_ACTIVE_HIGH>;
- input;
- line-name = "CPU2_PROCDIS_BMC_N";
+ line-name = "BMC_PCIE_I2C_MUX_RST_N";
};

pin_gpio_f2 {
@@ -516,34 +814,6 @@
line-name = "BMC_FORCE_NM_THROTTLE_N";
};

- pin_gpio_f6 {
- gpio-hog;
- gpios = <ASPEED_GPIO(F, 6) GPIO_ACTIVE_HIGH>;
- output-high;
- line-name = "FM_BMC_CPU_PWR_DEBUG_N";
- };
-
- pin_gpio_g7 {
- gpio-hog;
- gpios = <ASPEED_GPIO(G, 7) GPIO_ACTIVE_HIGH>;
- output-high;
- line-name = "BMC_PCIE_I2C_MUX_RST_N";
- };
-
- pin_gpio_h6 {
- gpio-hog;
- gpios = <ASPEED_GPIO(H, 6) GPIO_ACTIVE_HIGH>;
- output-high;
- line-name = "FM_BMC_DBP_PRESENT_R2_N";
- };
-
- pin_gpio_i3 {
- gpio-hog;
- gpios = <ASPEED_GPIO(I, 3) GPIO_ACTIVE_HIGH>;
- output-high;
- line-name = "SPI_BMC_BIOS_WP_N";
- };
-
pin_gpio_j1 {
gpio-hog;
gpios = <ASPEED_GPIO(J, 1) GPIO_ACTIVE_HIGH>;
@@ -565,20 +835,6 @@
line-name = "SPI_BMC_BIOS_HOLD_N";
};

- pin_gpio_l0 {
- gpio-hog;
- gpios = <ASPEED_GPIO(L, 0) GPIO_ACTIVE_HIGH>;
- output-high;
- line-name = "PDB_FAN_TACH_SEL";
- };
-
- pin_gpio_l1 {
- gpio-hog;
- gpios = <ASPEED_GPIO(L, 1) GPIO_ACTIVE_HIGH>;
- output-high;
- line-name = "SYS_RESET_BMC_FPGA_N";
- };
-
pin_gpio_l4 {
gpio-hog;
gpios = <ASPEED_GPIO(L, 4) GPIO_ACTIVE_HIGH>;
@@ -593,27 +849,6 @@
line-name = "FM_EFUSE_FAN_G2_EN";
};

- pin_gpio_r6 {
- gpio-hog;
- gpios = <ASPEED_GPIO(R, 6) GPIO_ACTIVE_HIGH>;
- input;
- line-name = "CPU3_PROCDIS_BMC_N";
- };
-
- pin_gpio_r7 {
- gpio-hog;
- gpios = <ASPEED_GPIO(R, 7) GPIO_ACTIVE_HIGH>;
- input;
- line-name = "CPU4_PROCDIS_BMC_N";
- };
-
- pin_gpio_s1 {
- gpio-hog;
- gpios = <ASPEED_GPIO(S, 1) GPIO_ACTIVE_HIGH>;
- output-low;
- line-name = "DBP_SYSPWROK_BMC";
- };
-
pin_gpio_s2 {
gpio-hog;
gpios = <ASPEED_GPIO(S, 2) GPIO_ACTIVE_HIGH>;
@@ -621,13 +856,6 @@
line-name = "PCH_RST_RSMRST_N";
};

- pin_gpio_s6 {
- gpio-hog;
- gpios = <ASPEED_GPIO(S, 6) GPIO_ACTIVE_HIGH>;
- output-high;
- line-name = "BMC_HW_STRAP_5";
- };
-
pin_gpio_z3 {
gpio-hog;
gpios = <ASPEED_GPIO(Z, 3) GPIO_ACTIVE_HIGH>;
@@ -638,29 +866,8 @@
pin_gpio_aa0 {
gpio-hog;
gpios = <ASPEED_GPIO(AA, 0) GPIO_ACTIVE_HIGH>;
- output-low;
- line-name = "FW_PSU_ALERT_EN_N";
- };
-
- pin_gpio_aa4 {
- gpio-hog;
- gpios = <ASPEED_GPIO(AA, 4) GPIO_ACTIVE_HIGH>;
output-high;
- line-name = "DBP_CPU_PREQ_N";
- };
-
- pin_gpio_ab3 {
- gpio-hog;
- gpios = <ASPEED_GPIO(AB, 3) GPIO_ACTIVE_HIGH>;
- output-low;
- line-name = "BMC_WDTRST";
- };
-
- pin_gpio_ac6 {
- gpio-hog;
- gpios = <ASPEED_GPIO(AC, 6) GPIO_ACTIVE_HIGH>;
- output-high;
- line-name = "ESPI_BMC_ALERT_N";
+ line-name = "FW_PSU_ALERT_EN_N";
};

};
--
2.7.4


2020-01-06 06:48:55

by Joel Stanley

[permalink] [raw]
Subject: Re: [PATCH v1 1/1] ARM: dts: aspeed: update Hr855xg2 device tree

On Thu, 26 Dec 2019 at 08:54, Andrew Peng <[email protected]> wrote:
>

When you have a list of things like below, it's sometimes a good hint
that you should be sending one patch for each bullet point. This makes
it easier to review.

> Update i2c aliases.
> Change flash_memory mapping address and size.
> Add in a gpio-keys section.
> Enable vhub, vuart, spi1 and spi2.
> Add raa228006, ir38164 and sn1701022 hwmon sensors.
> Remove some unuse gpio from gpio section.

unused?

>
> Signed-off-by: Andrew Peng <[email protected]>
> Signed-off-by: Derek Lin <[email protected]>
> Signed-off-by: Yonghui Liu <[email protected]>

I got two copies of this. I think they are the same.

> ---
> v1: initial version
>
> arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts | 557 ++++++++++++++++-------
> 1 file changed, 382 insertions(+), 175 deletions(-)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts b/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
> index 8193fad..e1386d4 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
> @@ -15,14 +15,21 @@
> compatible = "lenovo,hr855xg2-bmc", "aspeed,ast2500";
>

> - flash_memory: region@98000000 {
> + flash_memory: region@9EFF0000 {
> no-map;
> - reg = <0x98000000 0x00100000>; /* 1M */
> + reg = <0x9EFF0000 0x00010000>; /* 64K */

Do you really use 64K here, or was this a workaround for the lpc-ctlr
driver requiring a memory region?

If it's a workaround you can now drop the memory region phandle, as
the driver works without it.

> +&spi2 {
> status = "okay";
> pinctrl-names = "default";
> - pinctrl-0 = <&pinctrl_txd1_default
> - &pinctrl_rxd1_default>;
> + pinctrl-0 = <&pinctrl_spi2ck_default
> + &pinctrl_spi2cs0_default
> + &pinctrl_spi2miso_default
> + &pinctrl_spi2mosi_default>;
> +
> + spidev@0 {
> + status = "okay";
> + compatible = "aspeed,spidev";
> + reg = < 0 >;
> + spi-max-frequency = <50000000>;
> + };

This is for an out of tree driver? We discourage that, and prefer you
submit the driver upstream for review before adding it to the device
tree.

Please drop the sbidev bit from your next version.

> +
> + flash@0 {
> + compatible = "jedec,spi-nor";
> + m25p,fast-read;
> + label = "fpga";
> + reg = < 0 >;
> + spi-max-frequency = <50000000>;
> + status = "okay";
> + };
> };

> +&vuart {
> status = "okay";
> + auto-flow-control;
> + espi-enabled = <&syscon 0x70 25>;

Is this the same as the upstreamed aspeed,sirq-polarity-sense?

Please review https://git.kernel.org/torvalds/c/8d310c9107a2a3f19dc7bb54dd50f70c65ef5409.
I think you will find you can drop the espi-enabled property as
aspeed-g5.dtsi now contains the same information.

> + pcie_slot12: i2c@4{
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <4>;
> + };
> +
> + switch0_i2c5:i2c@5{

a space after the :

> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <5>;
> + eeprom@54 {
> + compatible = "atmel,24c04";
> + pagesize = <16>;
> + reg = <0x54>;
> + };
> };
> };
> };
> @@ -216,13 +377,43 @@
> };
>
> VR@45 {
> - compatible = "pmbus";
> + compatible = "raa228006";

Please send this change once you've had your pmbus driver accepted by
Guneter. In the mean time I suggest dropping it from v2 so we can
merge the other changes.

> reg = <0x45>;
> };
> };
>

> + CPU0_VCCIN@60 {

Convention is to use lower case for node names.

> + compatible = "raa228006";
> + reg = <0x60>;
> + };
> +

2020-01-09 08:08:31

by Andrew Peng

[permalink] [raw]
Subject: 答复: [External] Re: [PATCH v1 1/1] ARM: d ts: aspeed: update Hr855xg2 device tree

Hi Joel,

Please help to check below my comments. Thanks.

Regards,
Andrew Peng

> -----邮件原件-----
> 发件人: Joel Stanley <[email protected]>
> 发送时间: 2020年1月6日 14:48
> 收件人: Andrew MS1 Peng <[email protected]>
> 抄送: Rob Herring <[email protected]>; Mark Rutland
> <[email protected]>; Andrew Jeffery <[email protected]>; devicetree
> <[email protected]>; Linux ARM
> <[email protected]>; linux-aspeed
> <[email protected]>; Linux Kernel Mailing List
> <[email protected]>; Benjamin Fair <[email protected]>;
> OpenBMC Maillist <[email protected]>; Derek Lin23
> <[email protected]>; Yonghui YH21 Liu <[email protected]>
> 主题: [External] Re: [PATCH v1 1/1] ARM: dts: aspeed: update Hr855xg2
> device tree
>
> On Thu, 26 Dec 2019 at 08:54, Andrew Peng <[email protected]> wrote:
> >
>
> When you have a list of things like below, it's sometimes a good hint that you
> should be sending one patch for each bullet point. This makes it easier to
> review.
>

Should I separate below changes to six patches for next patch version?
For example: [PATCH v2 0/5] [PATCH v2 1/5] ...etc

> > Update i2c aliases.
> > Change flash_memory mapping address and size.
> > Add in a gpio-keys section.
> > Enable vhub, vuart, spi1 and spi2.
> > Add raa228006, ir38164 and sn1701022 hwmon sensors.
> > Remove some unuse gpio from gpio section.
>
> unused?
>

It was my mistake, the correct sentence should be "Remove gpio from gpio section since it controlled by user space."

> >
> > Signed-off-by: Andrew Peng <[email protected]>
> > Signed-off-by: Derek Lin <[email protected]>
> > Signed-off-by: Yonghui Liu <[email protected]>
>
> I got two copies of this. I think they are the same.
>

I apologize to send twice.

> > ---
> > v1: initial version
> >
> > arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts | 557
> > ++++++++++++++++-------
> > 1 file changed, 382 insertions(+), 175 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
> > b/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
> > index 8193fad..e1386d4 100644
> > --- a/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
> > +++ b/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
> > @@ -15,14 +15,21 @@
> > compatible = "lenovo,hr855xg2-bmc", "aspeed,ast2500";
> >
>
> > - flash_memory: region@98000000 {
> > + flash_memory: region@9EFF0000 {
> > no-map;
> > - reg = <0x98000000 0x00100000>; /* 1M */
> > + reg = <0x9EFF0000 0x00010000>; /* 64K */
>
> Do you really use 64K here, or was this a workaround for the lpc-ctlr driver
> requiring a memory region?
>

We reserve 64K for L2A In-Band Firmware Update (phosphor-ipmi-flash).

> If it's a workaround you can now drop the memory region phandle, as the
> driver works without it.
>
> > +&spi2 {
> > status = "okay";
> > pinctrl-names = "default";
> > - pinctrl-0 = <&pinctrl_txd1_default
> > - &pinctrl_rxd1_default>;
> > + pinctrl-0 = <&pinctrl_spi2ck_default
> > + &pinctrl_spi2cs0_default
> > + &pinctrl_spi2miso_default
> > + &pinctrl_spi2mosi_default>;
> > +
> > + spidev@0 {
> > + status = "okay";
> > + compatible = "aspeed,spidev";
> > + reg = < 0 >;
> > + spi-max-frequency = <50000000>;
> > + };
>
> This is for an out of tree driver? We discourage that, and prefer you submit the
> driver upstream for review before adding it to the device tree.
>
> Please drop the sbidev bit from your next version.
>

I will remove spidev@0 property in the next version.

> > +
> > + flash@0 {
> > + compatible = "jedec,spi-nor";
> > + m25p,fast-read;
> > + label = "fpga";
> > + reg = < 0 >;
> > + spi-max-frequency = <50000000>;
> > + status = "okay";
> > + };
> > };
>
> > +&vuart {
> > status = "okay";
> > + auto-flow-control;
> > + espi-enabled = <&syscon 0x70 25>;
>
> Is this the same as the upstreamed aspeed,sirq-polarity-sense?
>

Yes, it is used for sirq-polarity-sense.

> Please review
> https://git.kernel.org/torvalds/c/8d310c9107a2a3f19dc7bb54dd50f70c65ef5
> 409.
> I think you will find you can drop the espi-enabled property as aspeed-g5.dtsi
> now contains the same information.
>

I will remove espi-enabled property in the next version.

> > + pcie_slot12: i2c@4{
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + reg = <4>;
> > + };
> > +
> > + switch0_i2c5:i2c@5{
>
> a space after the :
>

I will revise it.

> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + reg = <5>;
> > + eeprom@54 {
> > + compatible =
> "atmel,24c04";
> > + pagesize =
> <16>;
> > + reg = <0x54>;
> > + };
> > };
> > };
> > };
> > @@ -216,13 +377,43 @@
> > };
> >
> > VR@45 {
> > - compatible = "pmbus";
> > + compatible = "raa228006";
>
> Please send this change once you've had your pmbus driver accepted by
> Guneter. In the mean time I suggest dropping it from v2 so we can merge the
> other changes.
>

I will remove it in the next version.

> > reg = <0x45>;
> > };
> > };
> >
>
> > + CPU0_VCCIN@60 {
>
> Convention is to use lower case for node names.
>

I will drop CPUXX_VCCXX relative property in the next version since it use new pmbus driver, and I will remember to use lower case for node names.

> > + compatible = "raa228006";
> > + reg = <0x60>;
> > + };
> > +

2020-01-22 00:27:55

by Joel Stanley

[permalink] [raw]
Subject: Re: [External] Re: [PATCH v1 1/1] ARM: dts: aspeed: update Hr855xg2 device tree

On Thu, 9 Jan 2020 at 08:07, Andrew MS1 Peng <[email protected]> wrote:
>
> Hi Joel,
>
> Please help to check below my comments. Thanks.
>
> Regards,
> Andrew Peng
>
> > -----邮件原件-----
> > 发件人: Joel Stanley <[email protected]>
> > 发送时间: 2020年1月6日 14:48
> > 收件人: Andrew MS1 Peng <[email protected]>
> > 抄送: Rob Herring <[email protected]>; Mark Rutland
> > <[email protected]>; Andrew Jeffery <[email protected]>; devicetree
> > <[email protected]>; Linux ARM
> > <[email protected]>; linux-aspeed
> > <[email protected]>; Linux Kernel Mailing List
> > <[email protected]>; Benjamin Fair <[email protected]>;
> > OpenBMC Maillist <[email protected]>; Derek Lin23
> > <[email protected]>; Yonghui YH21 Liu <[email protected]>
> > 主题: [External] Re: [PATCH v1 1/1] ARM: dts: aspeed: update Hr855xg2
> > device tree
> >
> > On Thu, 26 Dec 2019 at 08:54, Andrew Peng <[email protected]> wrote:
> > >
> >
> > When you have a list of things like below, it's sometimes a good hint that you
> > should be sending one patch for each bullet point. This makes it easier to
> > review.
> >
>
> Should I separate below changes to six patches for next patch version?
> For example: [PATCH v2 0/5] [PATCH v2 1/5] ...etc

It's up to you.

I do not require it.

>
> > > Update i2c aliases.
> > > Change flash_memory mapping address and size.
> > > Add in a gpio-keys section.
> > > Enable vhub, vuart, spi1 and spi2.
> > > Add raa228006, ir38164 and sn1701022 hwmon sensors.
> > > Remove some unuse gpio from gpio section.
> >
> > unused?
> >
>
> It was my mistake, the correct sentence should be "Remove gpio from gpio section since it controlled by user space."
>
> > >
> > > Signed-off-by: Andrew Peng <[email protected]>
> > > Signed-off-by: Derek Lin <[email protected]>
> > > Signed-off-by: Yonghui Liu <[email protected]>
> >
> > I got two copies of this. I think they are the same.
> >
>
> I apologize to send twice.
>
> > > ---
> > > v1: initial version
> > >
> > > arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts | 557
> > > ++++++++++++++++-------
> > > 1 file changed, 382 insertions(+), 175 deletions(-)
> > >
> > > diff --git a/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
> > > b/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
> > > index 8193fad..e1386d4 100644
> > > --- a/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
> > > +++ b/arch/arm/boot/dts/aspeed-bmc-lenovo-hr855xg2.dts
> > > @@ -15,14 +15,21 @@
> > > compatible = "lenovo,hr855xg2-bmc", "aspeed,ast2500";
> > >
> >
> > > - flash_memory: region@98000000 {
> > > + flash_memory: region@9EFF0000 {
> > > no-map;
> > > - reg = <0x98000000 0x00100000>; /* 1M */
> > > + reg = <0x9EFF0000 0x00010000>; /* 64K */
> >
> > Do you really use 64K here, or was this a workaround for the lpc-ctlr driver
> > requiring a memory region?
> >
>
> We reserve 64K for L2A In-Band Firmware Update (phosphor-ipmi-flash).

Okay, thanks for clarifying.

I am happy with rest of your responses. Please send v2 with these things fixed.

Cheers,

Joel