Hi Joel,
Please help to check below my comments. Thanks.
Regards,
Andrew Peng
> -----邮件原件-----
> 发件人: Joel Stanley <joel@xxxxxxxxx>
> 发送时间: 2020年1月6日 14:48
> 收件人: Andrew MS1 Peng <pengms1@xxxxxxxxxx>
> 抄送: Rob Herring <robh+dt@xxxxxxxxxx>; Mark Rutland
> <mark.rutland@xxxxxxx>; Andrew Jeffery <andrew@xxxxxxxx>; devicetree
> <devicetree@xxxxxxxxxxxxxxx>; Linux ARM
> <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; linux-aspeed
> <linux-aspeed@xxxxxxxxxxxxxxxx>; Linux Kernel Mailing List
> <linux-kernel@xxxxxxxxxxxxxxx>; Benjamin Fair <benjaminfair@xxxxxxxxxx>;
> OpenBMC Maillist <openbmc@xxxxxxxxxxxxxxxx>; Derek Lin23
> <dlin23@xxxxxxxxxx>; Yonghui YH21 Liu <liuyh21@xxxxxxxxxx>
> 主题: [External] Re: [PATCH v1 1/1] ARM: dts: aspeed: update Hr855xg2
> device tree
>
> On Thu, 26 Dec 2019 at 08:54, Andrew Peng <pengms1@xxxxxxxxxx> 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 <pengms1@xxxxxxxxxx>
> > Signed-off-by: Derek Lin <dlin23@xxxxxxxxxx>
> > Signed-off-by: Yonghui Liu <liuyh21@xxxxxxxxxx>
>
> 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>;
> > + };
> > +
