Il 09/03/22 12:47, Tinghan Shen ha scritto:
The definition of L1TCM_SRAM_PDN bits on mt8195 is different to mt8192. L1TCM_SRAM_PDN bits[3:0] control the power of mt8195 L1TCM SRAM. L1TCM_SRAM_PDN bits[7:4] control the access path to EMI for SCP. These bits have to be powered on to allow EMI access for SCP. Bits[7:4] also affect audio DSP because audio DSP and SCP are placed on the same hardware bus. If SCP cannot access EMI, audio DSP is blocked too. L1TCM_SRAM_PDN bits[31:8] are not used. This fix removes modification of bits[7:4] when power on/off mt8195 SCP L1TCM. It's because the modification introduces a short period of time blocking audio DSP to access EMI. This was not a problem until we have to load both SCP module and audio DSP module. audio DSP needs to access EMI because it has source/data on DRAM. Audio DSP will have unexpected behavior when it accesses EMI and the SCP driver blocks the EMI path at the same time. Signed-off-by: Tinghan Shen <tinghan.shen@xxxxxxxxxxxx> --- drivers/remoteproc/mtk_common.h | 4 +++ drivers/remoteproc/mtk_scp.c | 57 +++++++++++++++++++++++++++++---- 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h index 5ff3867c72f3..27e7172c926d 100644 --- a/drivers/remoteproc/mtk_common.h +++ b/drivers/remoteproc/mtk_common.h @@ -51,6 +51,10 @@ #define MT8192_CORE0_WDT_IRQ 0x10030 #define MT8192_CORE0_WDT_CFG 0x10034+#define MT8195_L1TCM_SRAM_PDN_RESERVED_RSI_BITS 0xF0
This is GENMASK(7, 4)..
+#define MT8195_L1TCM_SRAM_PDN_RESERVED_BITS \ + MT8195_L1TCM_SRAM_PDN_RESERVED_RSI_BITS +
Why are you defining the same thing twice? Please drop this.
#define SCP_FW_VER_LEN 32 #define SCP_SHARE_BUFFER_SIZE 288diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.cindex dcddb33e9997..4d75af856fd1 100644 --- a/drivers/remoteproc/mtk_scp.c +++ b/drivers/remoteproc/mtk_scp.c @@ -365,22 +365,32 @@ static int mt8183_scp_before_load(struct mtk_scp *scp) return 0; }-static void mt8192_power_on_sram(void __iomem *addr)+static void scp_sram_power_on(void __iomem *addr, u32 reserved_mask) { int i;for (i = 31; i >= 0; i--)- writel(GENMASK(i, 0), addr); + writel(GENMASK(i, 0) & ~reserved_mask, addr); writel(0, addr); }-static void mt8192_power_off_sram(void __iomem *addr)+static void scp_sram_power_off(void __iomem *addr, u32 reserved_mask) { int i;writel(0, addr);for (i = 0; i < 32; i++) - writel(GENMASK(i, 0), addr); + writel(GENMASK(i, 0) & ~reserved_mask, addr); +} + +static void mt8192_power_on_sram(void __iomem *addr) +{ + scp_sram_power_on(addr, 0); +} + +static void mt8192_power_off_sram(void __iomem *addr) +{ + scp_sram_power_off(addr, 0); }static int mt8192_scp_before_load(struct mtk_scp *scp)@@ -403,6 +413,27 @@ static int mt8192_scp_before_load(struct mtk_scp *scp) return 0; }+static int mt8195_scp_before_load(struct mtk_scp *scp)+{ + /* clear SPM interrupt, SCP2SPM_IPC_CLR */ + writel(0xff, scp->reg_base + MT8192_SCP2SPM_IPC_CLR); + + writel(1, scp->reg_base + MT8192_CORE0_SW_RSTN_SET); + + /* enable SRAM clock */ + mt8192_power_on_sram(scp->reg_base + MT8192_L2TCM_SRAM_PD_0);
At this point, you can simply use scp_sram_power_{on, off} instead of defining
a new function for just one call... I get that your intent here is to enhance
human readability, but I don't think that this is really happening with that and,
if it is, it's just about a little ignorable difference.
Please use scp_sram_power_on() and scp_sram_power_off() directly.
scp_sram_power_on(scp->reg_base + MT8192_L2TCM_SRAM_PD_1, 0);
... etc :)
+ mt8192_power_on_sram(scp->reg_base + MT8192_L2TCM_SRAM_PD_1); + mt8192_power_on_sram(scp->reg_base + MT8192_L2TCM_SRAM_PD_2); + scp_sram_power_on(scp->reg_base + MT8192_L1TCM_SRAM_PDN, + MT8195_L1TCM_SRAM_PDN_RESERVED_BITS); + mt8192_power_on_sram(scp->reg_base + MT8192_CPU0_SRAM_PD); + + /* enable MPU for all memory regions */ + writel(0xff, scp->reg_base + MT8192_CORE0_MEM_ATT_PREDEF); + + return 0; +} +
Please remember to add me to the Cc's for the next version, so that I will be able to timely give you my R-b tag for this one. Regards, Angelo
