Yanjie Jiang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32666 )
Change subject: mediatek/mt8183: Add md power-off flow ......................................................................
Patch Set 10:
(87 comments)
https://review.coreboot.org/c/coreboot/+/32666/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32666/1//COMMIT_MSG@2 PS1, Line 2: yanjie.jiang
Please spell it Yanjie Jiang. […]
Done
https://review.coreboot.org/c/coreboot/+/32666/1//COMMIT_MSG@8 PS1, Line 8:
Why prefix with coreboot? Please look at `git log --oneline` how to format the summaries. […]
Done
https://review.coreboot.org/c/coreboot/+/32666/1//COMMIT_MSG@10 PS1, Line 10:
Signed-off-by line is missing.
Done
https://review.coreboot.org/c/coreboot/+/32666/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32666/5//COMMIT_MSG@10 PS5, Line 10: no
not
Done
https://review.coreboot.org/c/coreboot/+/32666/5//COMMIT_MSG@17 PS5, Line 17: yanjie.jiang
Please update this string too.
Done
https://review.coreboot.org/c/coreboot/+/32666/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32666/6//COMMIT_MSG@9 PS6, Line 9: 26M clock will be hold by srcclkena
SRCCLKENA holds 26M clock
Done
https://review.coreboot.org/c/coreboot/+/32666/6//COMMIT_MSG@9 PS6, Line 9: make suspend fail.
fail suspend/resume.
Done
https://review.coreboot.org/c/coreboot/+/32666/6//COMMIT_MSG@10 PS6, Line 10: , so we release it.
so we can simply release it for suspend/resume to work.
Done
https://review.coreboot.org/c/coreboot/+/32666/6//COMMIT_MSG@10 PS6, Line 10: srcclkena
SRCCLKENA
Done
https://review.coreboot.org/c/coreboot/+/32666/6//COMMIT_MSG@10 PS6, Line 10: on
by
Done
https://review.coreboot.org/c/coreboot/+/32666/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32666/8//COMMIT_MSG@7 PS8, Line 7: power off
power-off
Done
https://review.coreboot.org/c/coreboot/+/32666/8//COMMIT_MSG@10 PS8, Line 10: The SRCCLKENA is not used by mt8183, : so we can simply release it for suspend/resume to work.
Please do not break the, just because there is a comma.
Done
https://review.coreboot.org/c/coreboot/+/32666/8//COMMIT_MSG@15 PS8, Line 15: Test=Boots correctly on Kukui, suspend test.
Yes, will fail suspend/resume.
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/md_ctrl.h:
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 1: /*
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 2: * Copyright (C) 2019 MediaTek Inc.
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 3: *
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 4: * This program is free software: you can redistribute it and/or modify
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 5: * it under the terms of the GNU General Public License version 2 as
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 6: * published by the Free Software Foundation.
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 7: *
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 8: * This program is distributed in the hope that it will be useful,
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 9: * but WITHOUT ANY WARRANTY; without even the implied warranty of
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 10: * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 11: * GNU General Public License for more details.
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 12: */
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 13:
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 14: #ifndef __SOC_MEDIATEK_MD_POWER_H__
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 15: #define __SOC_MEDIATEK_MD_POWER_H__
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 16:
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 17: void mtk_md_early_init(void);
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 18:
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/inc... PS1, Line 19: #endif
adding a line without newline at end of file
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... File src/soc/mediatek/mt8183/md_ctrl.c:
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 1: /*
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 2: * Copyright (C) 2019 MediaTek Inc.
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 3: *
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 4: * This program is free software: you can redistribute it and/or modify
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 5: * it under the terms of the GNU General Public License version 2 as
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 6: * published by the Free Software Foundation.
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 7: *
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 8: * This program is distributed in the hope that it will be useful,
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 9: * but WITHOUT ANY WARRANTY; without even the implied warranty of
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 10: * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 11: * GNU General Public License for more details.
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 12: */
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 13:
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 13:
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 14: #include <device/mmio.h>
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 15: #include <console/console.h>
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 16: #include <soc/addressmap.h>
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 17: #include <soc/infracfg.h>
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 18: #include <soc/pll.h>
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 19: #include <soc/md_ctrl.h>
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 20:
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 21: static void internal_md1_power_down(void)
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 22: {
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 23: unsigned int reg_value;
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 24:
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 25: /* 1. md clock setting: gating */
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 26: reg_value = read32(&mtk_topckgen->clk_mode);
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 27: reg_value |= ((1<<8)|(1<<9));
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 28: write32(&mtk_topckgen->clk_mode, reg_value);
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 29:
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 30: /* 2. mixedsys topsm init, for release srcclkena in kernel */
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 31: reg_value = read32(&mt8183_infracfg->infra_misc2);
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 32: reg_value &= ~0xFF;
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 33: write32(&mt8183_infracfg->infra_misc2, reg_value);
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 34:
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 35: printk(BIOS_INFO, "[ccci-off]src clk ena = 0x%X\n",
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 36: read32(&mt8183_infracfg->infra_misc2));
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 37: }
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 38:
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 39: void mtk_md_early_init(void)
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 40: {
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 41: internal_md1_power_down();
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 42: }
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/1/src/soc/mediatek/mt8183/md_... PS1, Line 43:
DOS line endings
Done
https://review.coreboot.org/c/coreboot/+/32666/2/src/soc/mediatek/mt8183/md_... File src/soc/mediatek/mt8183/md_ctrl.c:
https://review.coreboot.org/c/coreboot/+/32666/2/src/soc/mediatek/mt8183/md_... PS2, Line 13:
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/32666/2/src/soc/mediatek/mt8183/md_... PS2, Line 13:
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/32666/2/src/soc/mediatek/mt8183/md_... PS2, Line 23: setbits_le32(&mtk_topckgen->clk_mode, ((1<<8)|(1<<9)));
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/32666/2/src/soc/mediatek/mt8183/md_... PS2, Line 23: setbits_le32(&mtk_topckgen->clk_mode, ((1<<8)|(1<<9)));
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/32666/6/src/soc/mediatek/mt8183/md_... File src/soc/mediatek/mt8183/md_ctrl.c:
https://review.coreboot.org/c/coreboot/+/32666/6/src/soc/mediatek/mt8183/md_... PS6, Line 23: (1 << 8) | (1 << 9)
Do these have good reg constant names? […]
Done
https://review.coreboot.org/c/coreboot/+/32666/6/src/soc/mediatek/mt8183/md_... PS6, Line 25: 0xFF
also make a constant for this.
Done
https://review.coreboot.org/c/coreboot/+/32666/7/src/soc/mediatek/mt8183/md_... File src/soc/mediatek/mt8183/md_ctrl.c:
https://review.coreboot.org/c/coreboot/+/32666/7/src/soc/mediatek/mt8183/md_... PS7, Line 28: (
no need to quote since this is one single arg. […]
Done
https://review.coreboot.org/c/coreboot/+/32666/8/src/soc/mediatek/mt8183/md_... File src/soc/mediatek/mt8183/md_ctrl.c:
https://review.coreboot.org/c/coreboot/+/32666/8/src/soc/mediatek/mt8183/md_... PS8, Line 26: /* Gating MD clock. */
deleted
Done
https://review.coreboot.org/c/coreboot/+/32666/8/src/soc/mediatek/mt8183/md_... PS8, Line 29: /* Release SRCCLKENA. */
Ditto.
Done
https://review.coreboot.org/c/coreboot/+/32666/8/src/soc/mediatek/mt8183/md_... PS8, Line 31: INFRA_MISC2_SRCCLKENA_RELEASE);
clrbits_le32(&mt8183_infracfg->infra_misc2, INFRA_MISC2_SRCCLKENA_RELEASE); […]
Ack
https://review.coreboot.org/c/coreboot/+/32666/6/src/soc/mediatek/mt8183/soc... File src/soc/mediatek/mt8183/soc.c:
https://review.coreboot.org/c/coreboot/+/32666/6/src/soc/mediatek/mt8183/soc... PS6, Line 20: #include <soc/md_ctrl.h>
order header files (i.e., move to before mmu_operations. […]
Done