CK HU has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
soc/mediatek/mt8192: Add usb host support
Add usb host function support
Signed-off-by: Tianping Fang tianping.fang@mediatek.com Signed-off-by: Zhanyong Wang zhanyong.wang@mediatek.com Change-Id: Icb4cc304654b5fb7cf20b96ab83a22663bfeab63 --- M src/soc/mediatek/mt8192/Makefile.inc A src/soc/mediatek/mt8192/include/soc/usb.h A src/soc/mediatek/mt8192/usb.c 3 files changed, 39 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/45396/1
diff --git a/src/soc/mediatek/mt8192/Makefile.inc b/src/soc/mediatek/mt8192/Makefile.inc old mode 100644 new mode 100755 index c7dbe51..533eae2 --- a/src/soc/mediatek/mt8192/Makefile.inc +++ b/src/soc/mediatek/mt8192/Makefile.inc @@ -32,6 +32,7 @@ ramstage-y += soc.c ramstage-y += ../common/timer.c ramstage-y += ../common/uart.c +ramstage-y += ../common/usb.c usb.c
CPPFLAGS_common += -Isrc/soc/mediatek/mt8192/include CPPFLAGS_common += -Isrc/soc/mediatek/common/include diff --git a/src/soc/mediatek/mt8192/include/soc/usb.h b/src/soc/mediatek/mt8192/include/soc/usb.h new file mode 100755 index 0000000..3537ba2 --- /dev/null +++ b/src/soc/mediatek/mt8192/include/soc/usb.h @@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef SOC_MEDIATEK_MT8192_USB_H +#define SOC_MEDIATEK_MT8192_USB_H + +#include <soc/usb_common.h> + +struct ssusb_sif_port { + struct sif_u2_phy_com u2phy; + u32 reserved0[64*5]; + struct sif_u3phyd u3phyd; + u32 reserved1[64]; + struct sif_u3phya u3phya; + struct sif_u3phya_da u3phya_da; + u32 reserved2[64 * 3]; +}; +check_member(ssusb_sif_port, u3phyd, 0x600); +check_member(ssusb_sif_port, u3phya, 0x800); +check_member(ssusb_sif_port, u3phya_da, 0x900); +check_member(ssusb_sif_port, reserved2, 0xa00); + +#define USB_PORT_NUMBER 2 + +#endif diff --git a/src/soc/mediatek/mt8192/usb.c b/src/soc/mediatek/mt8192/usb.c new file mode 100755 index 0000000..4beab9c --- /dev/null +++ b/src/soc/mediatek/mt8192/usb.c @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#include <soc/addressmap.h> +#include <device/mmio.h> +#include <soc/usb.h> + +#define REG_SPM_POWERON_CONFIG_EN (void *)(SPM_BASE + 0x000) +#define REG_SPM_SSPM_PWR_CON (void *)(SPM_BASE + 0x390) + +void mtk_usb_prepare(void) +{ + /* power on SSUSB SRAM FIFO */ + setbits32(REG_SPM_POWERON_CONFIG_EN, 0xB160001); + clrbits32(REG_SPM_SSPM_PWR_CON, 0x000001FF); +} +
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
Patch Set 1:
File src/soc/mediatek/mt8192/Makefile.inc has one or more executable bits set in the file permissions. File src/soc/mediatek/mt8192/include/soc/usb.h has one or more executable bits set in the file permissions. File src/soc/mediatek/mt8192/usb.c has one or more executable bits set in the file permissions.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/usb... File src/soc/mediatek/mt8192/usb.c:
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/usb... PS1, Line 12: setbits32 set or clrset?
I think you probably want to change these to SET32_BITFIELDS APIs so it'll be easier to read and understand.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/inc... File src/soc/mediatek/mt8192/include/soc/usb.h:
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/inc... PS1, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */ Please add a blank line below.
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/usb... File src/soc/mediatek/mt8192/usb.c:
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/usb... PS1, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */ Ditto.
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/usb... PS1, Line 16: Please remove the blank line at the end of the file.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45396/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45396/1//COMMIT_MSG@9 PS1, Line 9: Add usb host function support Please mention the datasheet name and revision, and maybe elaborate that the common code can be used.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/Mak... File src/soc/mediatek/mt8192/Makefile.inc:
PS1: Please remove the execute permission.
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/inc... File src/soc/mediatek/mt8192/include/soc/usb.h:
PS1: Please remove the execute permission.
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/usb... File src/soc/mediatek/mt8192/usb.c:
PS1: Please remove the execute permission.
Yidi Lin has uploaded a new patch set (#2) to the change originally created by CK HU. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
soc/mediatek/mt8192: Add usb host support
Add usb host function support
Signed-off-by: Tianping Fang tianping.fang@mediatek.com Signed-off-by: Zhanyong Wang zhanyong.wang@mediatek.com Change-Id: Icb4cc304654b5fb7cf20b96ab83a22663bfeab63 --- M src/soc/mediatek/mt8192/Makefile.inc A src/soc/mediatek/mt8192/include/soc/usb.h A src/soc/mediatek/mt8192/usb.c 3 files changed, 40 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/45396/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45396/2/src/soc/mediatek/mt8192/usb... File src/soc/mediatek/mt8192/usb.c:
https://review.coreboot.org/c/coreboot/+/45396/2/src/soc/mediatek/mt8192/usb... PS2, Line 15: } adding a line without newline at end of file
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
Patch Set 2:
(7 comments)
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/Mak... File src/soc/mediatek/mt8192/Makefile.inc:
PS1:
Please remove the execute permission.
Done
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/inc... File src/soc/mediatek/mt8192/include/soc/usb.h:
PS1:
Please remove the execute permission.
Done
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/inc... PS1, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */
Please add a blank line below.
Done
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/usb... File src/soc/mediatek/mt8192/usb.c:
PS1:
Please remove the execute permission.
Done
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/usb... PS1, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */
Ditto.
Done
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/usb... PS1, Line 16:
Please remove the blank line at the end of the file.
Done
https://review.coreboot.org/c/coreboot/+/45396/2/src/soc/mediatek/mt8192/usb... File src/soc/mediatek/mt8192/usb.c:
https://review.coreboot.org/c/coreboot/+/45396/2/src/soc/mediatek/mt8192/usb... PS2, Line 15: }
adding a line without newline at end of file
Please fix this.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45396/3/src/soc/mediatek/mt8192/usb... File src/soc/mediatek/mt8192/usb.c:
https://review.coreboot.org/c/coreboot/+/45396/3/src/soc/mediatek/mt8192/usb... PS3, Line 15: } adding a line without newline at end of file
Yidi Lin has uploaded a new patch set (#4) to the change originally created by CK HU. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
soc/mediatek/mt8192: Add usb host support
Add usb host function support
Signed-off-by: Tianping Fang tianping.fang@mediatek.com Signed-off-by: Zhanyong Wang zhanyong.wang@mediatek.com Change-Id: Icb4cc304654b5fb7cf20b96ab83a22663bfeab63 --- M src/soc/mediatek/mt8192/Makefile.inc A src/soc/mediatek/mt8192/include/soc/usb.h A src/soc/mediatek/mt8192/usb.c 3 files changed, 41 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/45396/4
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45396/2/src/soc/mediatek/mt8192/usb... File src/soc/mediatek/mt8192/usb.c:
https://review.coreboot.org/c/coreboot/+/45396/2/src/soc/mediatek/mt8192/usb... PS2, Line 15: }
Please fix this.
done
https://review.coreboot.org/c/coreboot/+/45396/3/src/soc/mediatek/mt8192/usb... File src/soc/mediatek/mt8192/usb.c:
https://review.coreboot.org/c/coreboot/+/45396/3/src/soc/mediatek/mt8192/usb... PS3, Line 15: }
adding a line without newline at end of file
done
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45396/4/src/soc/mediatek/mt8192/usb... File src/soc/mediatek/mt8192/usb.c:
https://review.coreboot.org/c/coreboot/+/45396/4/src/soc/mediatek/mt8192/usb... PS4, Line 16: Remove one blank line.
Yidi Lin has uploaded a new patch set (#5) to the change originally created by CK HU. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
soc/mediatek/mt8192: Add usb host support
Add usb host function support
Signed-off-by: Tianping Fang tianping.fang@mediatek.com Signed-off-by: Zhanyong Wang zhanyong.wang@mediatek.com Change-Id: Icb4cc304654b5fb7cf20b96ab83a22663bfeab63 --- M src/soc/mediatek/mt8192/Makefile.inc A src/soc/mediatek/mt8192/include/soc/usb.h A src/soc/mediatek/mt8192/usb.c 3 files changed, 40 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/45396/5
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45396/4/src/soc/mediatek/mt8192/usb... File src/soc/mediatek/mt8192/usb.c:
https://review.coreboot.org/c/coreboot/+/45396/4/src/soc/mediatek/mt8192/usb... PS4, Line 16:
Remove one blank line.
done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
Patch Set 5: Code-Review+2
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Add usb host support ......................................................................
Patch Set 5:
There are still two unresolved comments
Tianping Fang has uploaded a new patch set (#6) to the change originally created by CK HU. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Refactor USB code among similar SoCs ......................................................................
soc/mediatek/mt8192: Refactor USB code among similar SoCs
Refactor USB code which will be reused among similar SoCs
Signed-off-by: Tianping Fang tianping.fang@mediatek.com Signed-off-by: Zhanyong Wang zhanyong.wang@mediatek.com Change-Id: Icb4cc304654b5fb7cf20b96ab83a22663bfeab63 --- M src/soc/mediatek/mt8192/Makefile.inc A src/soc/mediatek/mt8192/include/soc/usb.h A src/soc/mediatek/mt8192/usb.c 3 files changed, 40 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/45396/6
Yidi Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Refactor USB code among similar SoCs ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/usb... File src/soc/mediatek/mt8192/usb.c:
https://review.coreboot.org/c/coreboot/+/45396/1/src/soc/mediatek/mt8192/usb... PS1, Line 12: setbits32
set or clrset? […]
Ack
Tianping Fang has uploaded a new patch set (#7) to the change originally created by CK HU. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Refactor USB code among similar SoCs ......................................................................
soc/mediatek/mt8192: Refactor USB code among similar SoCs
Adjust ssusb register layout offset accroding mt8192 Soc, then refactor USB code which will be reused among similar SoCs
Signed-off-by: Tianping Fang tianping.fang@mediatek.com Signed-off-by: Zhanyong Wang zhanyong.wang@mediatek.com Change-Id: Icb4cc304654b5fb7cf20b96ab83a22663bfeab63 --- M src/soc/mediatek/mt8192/Makefile.inc A src/soc/mediatek/mt8192/include/soc/usb.h A src/soc/mediatek/mt8192/usb.c 3 files changed, 40 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/45396/7
Tianping Fang has uploaded a new patch set (#8) to the change originally created by CK HU. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Refactor USB code among similar SoCs ......................................................................
soc/mediatek/mt8192: Refactor USB code among similar SoCs
Adjust ssusb register layout offset accroding mt8192 Soc then refactor USB code which will be reused among similar SoCs
Signed-off-by: Tianping Fang tianping.fang@mediatek.com Signed-off-by: Zhanyong Wang zhanyong.wang@mediatek.com Change-Id: Icb4cc304654b5fb7cf20b96ab83a22663bfeab63 --- M src/soc/mediatek/mt8192/Makefile.inc A src/soc/mediatek/mt8192/include/soc/usb.h A src/soc/mediatek/mt8192/usb.c 3 files changed, 40 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/45396/8
Tianping Fang has uploaded a new patch set (#9) to the change originally created by CK HU. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Refactor USB code among similar SoCs ......................................................................
soc/mediatek/mt8192: Refactor USB code among similar SoCs
Adjust ssusb register layout and offset accroding mt8192 Soc then refactor USB code which will be reused among similar SoCs
Signed-off-by: Tianping Fang tianping.fang@mediatek.com Signed-off-by: Zhanyong Wang zhanyong.wang@mediatek.com Change-Id: Icb4cc304654b5fb7cf20b96ab83a22663bfeab63 --- M src/soc/mediatek/mt8192/Makefile.inc A src/soc/mediatek/mt8192/include/soc/usb.h A src/soc/mediatek/mt8192/usb.c 3 files changed, 40 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/45396/9
Tianping Fang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Refactor USB code among similar SoCs ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45396/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45396/1//COMMIT_MSG@9 PS1, Line 9: Add usb host function support
Please mention the datasheet name and revision, and maybe elaborate that the common code can be used […]
Ack
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Refactor USB code among similar SoCs ......................................................................
Patch Set 9:
unit test failed junit.xml.[failed-to-read]
@Patrick do you know if that's an infra issue?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Refactor USB code among similar SoCs ......................................................................
Patch Set 9:
Patch Set 9:
unit test failed junit.xml.[failed-to-read]
@Patrick do you know if that's an infra issue?
Not really an infra issue, but since the build has been aborted (because a newer patch set appeared), the junit files were incomplete: they're written incrementally and were likely lacking the closing xml tags.
tl;dr: when a build is aborted, look at the reason (the main other reason is timeout if the build takes more than 60 minutes) but ignore any other reported problems.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45396 )
Change subject: soc/mediatek/mt8192: Refactor USB code among similar SoCs ......................................................................
soc/mediatek/mt8192: Refactor USB code among similar SoCs
Adjust ssusb register layout and offset accroding mt8192 Soc then refactor USB code which will be reused among similar SoCs
Signed-off-by: Tianping Fang tianping.fang@mediatek.com Signed-off-by: Zhanyong Wang zhanyong.wang@mediatek.com Change-Id: Icb4cc304654b5fb7cf20b96ab83a22663bfeab63 Reviewed-on: https://review.coreboot.org/c/coreboot/+/45396 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Hung-Te Lin hungte@chromium.org --- M src/soc/mediatek/mt8192/Makefile.inc A src/soc/mediatek/mt8192/include/soc/usb.h A src/soc/mediatek/mt8192/usb.c 3 files changed, 40 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Hung-Te Lin: Looks good to me, approved
diff --git a/src/soc/mediatek/mt8192/Makefile.inc b/src/soc/mediatek/mt8192/Makefile.inc index c7dbe51..533eae2 100644 --- a/src/soc/mediatek/mt8192/Makefile.inc +++ b/src/soc/mediatek/mt8192/Makefile.inc @@ -32,6 +32,7 @@ ramstage-y += soc.c ramstage-y += ../common/timer.c ramstage-y += ../common/uart.c +ramstage-y += ../common/usb.c usb.c
CPPFLAGS_common += -Isrc/soc/mediatek/mt8192/include CPPFLAGS_common += -Isrc/soc/mediatek/common/include diff --git a/src/soc/mediatek/mt8192/include/soc/usb.h b/src/soc/mediatek/mt8192/include/soc/usb.h new file mode 100644 index 0000000..c988270 --- /dev/null +++ b/src/soc/mediatek/mt8192/include/soc/usb.h @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef SOC_MEDIATEK_MT8192_USB_H +#define SOC_MEDIATEK_MT8192_USB_H + +#include <soc/usb_common.h> + +struct ssusb_sif_port { + struct sif_u2_phy_com u2phy; + u32 reserved0[64*5]; + struct sif_u3phyd u3phyd; + u32 reserved1[64]; + struct sif_u3phya u3phya; + struct sif_u3phya_da u3phya_da; + u32 reserved2[64 * 3]; +}; +check_member(ssusb_sif_port, u3phyd, 0x600); +check_member(ssusb_sif_port, u3phya, 0x800); +check_member(ssusb_sif_port, u3phya_da, 0x900); +check_member(ssusb_sif_port, reserved2, 0xa00); + +#define USB_PORT_NUMBER 2 + +#endif diff --git a/src/soc/mediatek/mt8192/usb.c b/src/soc/mediatek/mt8192/usb.c new file mode 100644 index 0000000..44f2f15 --- /dev/null +++ b/src/soc/mediatek/mt8192/usb.c @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <soc/addressmap.h> +#include <device/mmio.h> +#include <soc/usb.h> + +#define REG_SPM_POWERON_CONFIG_EN (void *)(SPM_BASE + 0x000) +#define REG_SPM_SSPM_PWR_CON (void *)(SPM_BASE + 0x390) + +void mtk_usb_prepare(void) +{ + /* power on SSUSB SRAM FIFO */ + setbits32(REG_SPM_POWERON_CONFIG_EN, 0xB160001); + clrbits32(REG_SPM_SSPM_PWR_CON, 0x000001FF); +}
-
build bot (Jenkins) (Code Review) -
CK HU (Code Review)
-
Hung-Te Lin (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Tianping Fang (Code Review)
-
Yidi Lin (Code Review)
-
Yu-Ping Wu (Code Review)