| Message ID | 1503373600-20847-1-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On 22.08.2017 05:46, Bin Meng wrote: > All these places seem to inherit the codes from the MMC driver where > a FIXME was put in the comment. However the correct operation after > read should be cache invalidate, not flush. > > The underlying drivers should be responsible for the cache operation. > Remove these codes completely. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > arch/arm/cpu/armv8/fsl-layerscape/ppa.c | 15 --------------- > board/toradex/common/tdx-cfg-block.c | 2 -- > cmd/mmc.c | 2 -- > drivers/block/blk-uclass.c | 3 --- > drivers/block/blk_legacy.c | 3 --- > drivers/net/fm/fm.c | 2 -- > drivers/net/phy/cortina.c | 2 -- > drivers/qe/qe.c | 2 -- > 8 files changed, 31 deletions(-) > > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > index 24ddb5d..bbf8bba 100644 > --- a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > +++ b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > @@ -107,9 +107,6 @@ int ppa_init(void) > return -EIO; > } > > - /* flush cache after read */ > - flush_cache((ulong)fitp, cnt * 512); > - > ret = fdt_check_header(fitp); > if (ret) { > free(fitp); > @@ -134,9 +131,6 @@ int ppa_init(void) > } > debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); > > - /* flush cache after read */ > - flush_cache((ulong)ppa_hdr_ddr, cnt * 512); > - > ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; > #endif > > @@ -164,9 +158,6 @@ int ppa_init(void) > return -EIO; > } > > - /* flush cache after read */ > - flush_cache((ulong)ppa_fit_addr, cnt * 512); > - > #elif defined(CONFIG_SYS_LS_PPA_FW_IN_NAND) > struct fdt_header fit; > > @@ -208,9 +199,6 @@ int ppa_init(void) > } > debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); > > - /* flush cache after read */ > - flush_cache((ulong)ppa_hdr_ddr, fw_length); > - > ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; > #endif > > @@ -232,9 +220,6 @@ int ppa_init(void) > CONFIG_SYS_LS_PPA_FW_ADDR); > return -EIO; > } > - > - /* flush cache after read */ > - flush_cache((ulong)ppa_fit_addr, fw_length); > #else > #error "No CONFIG_SYS_LS_PPA_FW_IN_xxx defined" > #endif > diff --git a/board/toradex/common/tdx-cfg-block.c b/board/toradex/common/tdx-cfg-block.c > index 328c4c0..f850a3c 100644 > --- a/board/toradex/common/tdx-cfg-block.c > +++ b/board/toradex/common/tdx-cfg-block.c > @@ -129,8 +129,6 @@ static int tdx_cfg_block_mmc_storage(u8 *config_block, int write) > ret = -EIO; > goto out; > } > - /* Flush cache after read */ > - flush_cache((ulong)(unsigned char *)config_block, 512); > } else { > /* Just writing one 512 byte block */ > if (blk_dwrite(mmc_get_blk_desc(mmc), blk_start, 1, > diff --git a/cmd/mmc.c b/cmd/mmc.c > index 00697fc..5def4ea 100644 > --- a/cmd/mmc.c > +++ b/cmd/mmc.c > @@ -293,8 +293,6 @@ static int do_mmc_read(cmd_tbl_t *cmdtp, int flag, > curr_device, blk, cnt); > > n = blk_dread(mmc_get_blk_desc(mmc), blk, cnt, addr); > - /* flush cache after read */ > - flush_cache((ulong)addr, cnt * 512); /* FIXME */ > printf("%d blocks read: %s\n", n, (n == cnt) ? "OK" : "ERROR"); > > return (n == cnt) ? CMD_RET_SUCCESS : CMD_RET_FAILURE; > diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c > index 3aec569..e5f00dc 100644 > --- a/drivers/block/blk-uclass.c > +++ b/drivers/block/blk-uclass.c > @@ -294,9 +294,6 @@ ulong blk_read_devnum(enum if_type if_type, int devnum, lbaint_t start, > if (IS_ERR_VALUE(n)) > return n; > > - /* flush cache after read */ > - flush_cache((ulong)buffer, blkcnt * desc->blksz); > - > return n; > } > > diff --git a/drivers/block/blk_legacy.c b/drivers/block/blk_legacy.c > index 981872e..16d3bfe 100644 > --- a/drivers/block/blk_legacy.c > +++ b/drivers/block/blk_legacy.c > @@ -232,9 +232,6 @@ ulong blk_read_devnum(enum if_type if_type, int devnum, lbaint_t start, > if (IS_ERR_VALUE(n)) > return n; > > - /* flush cache after read */ > - flush_cache((ulong)buffer, blkcnt * desc->blksz); > - > return n; > } > > diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c > index 451dfde..261f1b9 100644 > --- a/drivers/net/fm/fm.c > +++ b/drivers/net/fm/fm.c > @@ -405,8 +405,6 @@ int fm_init_common(int index, struct ccsr_fman *reg) > mmc_init(mmc); > (void)mmc->block_dev.block_read(&mmc->block_dev, blk, cnt, > addr); > - /* flush cache after read */ > - flush_cache((ulong)addr, cnt * 512); > } > #elif defined(CONFIG_SYS_QE_FMAN_FW_IN_REMOTE) > void *addr = (void *)CONFIG_SYS_FMAN_FW_ADDR; > diff --git a/drivers/net/phy/cortina.c b/drivers/net/phy/cortina.c > index e0e9ed9..637d89a 100644 > --- a/drivers/net/phy/cortina.c > +++ b/drivers/net/phy/cortina.c > @@ -177,8 +177,6 @@ void cs4340_upload_firmware(struct phy_device *phydev) > mmc_init(mmc); > (void)mmc->block_dev.block_read(&mmc->block_dev, blk, cnt, > addr); > - /* flush cache after read */ > - flush_cache((ulong)addr, cnt * 512); > } > #endif > > diff --git a/drivers/qe/qe.c b/drivers/qe/qe.c > index 24e764d..931c9d9 100644 > --- a/drivers/qe/qe.c > +++ b/drivers/qe/qe.c > @@ -221,8 +221,6 @@ void u_qe_init(void) > mmc_init(mmc); > (void)mmc->block_dev.block_read(&mmc->block_dev, blk, cnt, > addr); > - /* flush cache after read */ > - flush_cache((ulong)addr, cnt * 512); > } > #endif > u_qe_upload_firmware(addr); > Did you experience any problems with the "incorrect" code in the MMC driver? Without looking into the underlying driver, your explanation makes perfect sense and flushing is wrong after a read of course. So: Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan
Hi Stefan, On Tue, Aug 22, 2017 at 2:19 PM, Stefan Roese <sr@denx.de> wrote: > On 22.08.2017 05:46, Bin Meng wrote: >> >> All these places seem to inherit the codes from the MMC driver where >> a FIXME was put in the comment. However the correct operation after >> read should be cache invalidate, not flush. >> >> The underlying drivers should be responsible for the cache operation. >> Remove these codes completely. >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> --- >> >> arch/arm/cpu/armv8/fsl-layerscape/ppa.c | 15 --------------- >> board/toradex/common/tdx-cfg-block.c | 2 -- >> cmd/mmc.c | 2 -- >> drivers/block/blk-uclass.c | 3 --- >> drivers/block/blk_legacy.c | 3 --- >> drivers/net/fm/fm.c | 2 -- >> drivers/net/phy/cortina.c | 2 -- >> drivers/qe/qe.c | 2 -- >> 8 files changed, 31 deletions(-) >> >> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c >> b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c >> index 24ddb5d..bbf8bba 100644 >> --- a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c >> +++ b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c >> @@ -107,9 +107,6 @@ int ppa_init(void) >> return -EIO; >> } >> - /* flush cache after read */ >> - flush_cache((ulong)fitp, cnt * 512); >> - >> ret = fdt_check_header(fitp); >> if (ret) { >> free(fitp); >> @@ -134,9 +131,6 @@ int ppa_init(void) >> } >> debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); >> - /* flush cache after read */ >> - flush_cache((ulong)ppa_hdr_ddr, cnt * 512); >> - >> ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; >> #endif >> @@ -164,9 +158,6 @@ int ppa_init(void) >> return -EIO; >> } >> - /* flush cache after read */ >> - flush_cache((ulong)ppa_fit_addr, cnt * 512); >> - >> #elif defined(CONFIG_SYS_LS_PPA_FW_IN_NAND) >> struct fdt_header fit; >> @@ -208,9 +199,6 @@ int ppa_init(void) >> } >> debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); >> - /* flush cache after read */ >> - flush_cache((ulong)ppa_hdr_ddr, fw_length); >> - >> ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; >> #endif >> @@ -232,9 +220,6 @@ int ppa_init(void) >> CONFIG_SYS_LS_PPA_FW_ADDR); >> return -EIO; >> } >> - >> - /* flush cache after read */ >> - flush_cache((ulong)ppa_fit_addr, fw_length); >> #else >> #error "No CONFIG_SYS_LS_PPA_FW_IN_xxx defined" >> #endif >> diff --git a/board/toradex/common/tdx-cfg-block.c >> b/board/toradex/common/tdx-cfg-block.c >> index 328c4c0..f850a3c 100644 >> --- a/board/toradex/common/tdx-cfg-block.c >> +++ b/board/toradex/common/tdx-cfg-block.c >> @@ -129,8 +129,6 @@ static int tdx_cfg_block_mmc_storage(u8 *config_block, >> int write) >> ret = -EIO; >> goto out; >> } >> - /* Flush cache after read */ >> - flush_cache((ulong)(unsigned char *)config_block, 512); >> } else { >> /* Just writing one 512 byte block */ >> if (blk_dwrite(mmc_get_blk_desc(mmc), blk_start, 1, >> diff --git a/cmd/mmc.c b/cmd/mmc.c >> index 00697fc..5def4ea 100644 >> --- a/cmd/mmc.c >> +++ b/cmd/mmc.c >> @@ -293,8 +293,6 @@ static int do_mmc_read(cmd_tbl_t *cmdtp, int flag, >> curr_device, blk, cnt); >> n = blk_dread(mmc_get_blk_desc(mmc), blk, cnt, addr); >> - /* flush cache after read */ >> - flush_cache((ulong)addr, cnt * 512); /* FIXME */ >> printf("%d blocks read: %s\n", n, (n == cnt) ? "OK" : "ERROR"); >> return (n == cnt) ? CMD_RET_SUCCESS : CMD_RET_FAILURE; >> diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c >> index 3aec569..e5f00dc 100644 >> --- a/drivers/block/blk-uclass.c >> +++ b/drivers/block/blk-uclass.c >> @@ -294,9 +294,6 @@ ulong blk_read_devnum(enum if_type if_type, int >> devnum, lbaint_t start, >> if (IS_ERR_VALUE(n)) >> return n; >> - /* flush cache after read */ >> - flush_cache((ulong)buffer, blkcnt * desc->blksz); >> - >> return n; >> } >> diff --git a/drivers/block/blk_legacy.c b/drivers/block/blk_legacy.c >> index 981872e..16d3bfe 100644 >> --- a/drivers/block/blk_legacy.c >> +++ b/drivers/block/blk_legacy.c >> @@ -232,9 +232,6 @@ ulong blk_read_devnum(enum if_type if_type, int >> devnum, lbaint_t start, >> if (IS_ERR_VALUE(n)) >> return n; >> - /* flush cache after read */ >> - flush_cache((ulong)buffer, blkcnt * desc->blksz); >> - >> return n; >> } >> diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c >> index 451dfde..261f1b9 100644 >> --- a/drivers/net/fm/fm.c >> +++ b/drivers/net/fm/fm.c >> @@ -405,8 +405,6 @@ int fm_init_common(int index, struct ccsr_fman *reg) >> mmc_init(mmc); >> (void)mmc->block_dev.block_read(&mmc->block_dev, blk, cnt, >> addr); >> - /* flush cache after read */ >> - flush_cache((ulong)addr, cnt * 512); >> } >> #elif defined(CONFIG_SYS_QE_FMAN_FW_IN_REMOTE) >> void *addr = (void *)CONFIG_SYS_FMAN_FW_ADDR; >> diff --git a/drivers/net/phy/cortina.c b/drivers/net/phy/cortina.c >> index e0e9ed9..637d89a 100644 >> --- a/drivers/net/phy/cortina.c >> +++ b/drivers/net/phy/cortina.c >> @@ -177,8 +177,6 @@ void cs4340_upload_firmware(struct phy_device *phydev) >> mmc_init(mmc); >> (void)mmc->block_dev.block_read(&mmc->block_dev, blk, cnt, >> addr); >> - /* flush cache after read */ >> - flush_cache((ulong)addr, cnt * 512); >> } >> #endif >> diff --git a/drivers/qe/qe.c b/drivers/qe/qe.c >> index 24e764d..931c9d9 100644 >> --- a/drivers/qe/qe.c >> +++ b/drivers/qe/qe.c >> @@ -221,8 +221,6 @@ void u_qe_init(void) >> mmc_init(mmc); >> (void)mmc->block_dev.block_read(&mmc->block_dev, blk, cnt, >> addr); >> - /* flush cache after read */ >> - flush_cache((ulong)addr, cnt * 512); >> } >> #endif >> u_qe_upload_firmware(addr); >> > > Did you experience any problems with the "incorrect" code in the > MMC driver? > No, since I have been mostly working on x86, and on x86 cache flush/invalidate is a nop, so no problems were seen. > Without looking into the underlying driver, your explanation makes > perfect sense and flushing is wrong after a read of course. So: > Yes, I doubt how these codes were working on other platforms where cache operations are really needed. I just noticed that when I was working on NVMe stuff. > Reviewed-by: Stefan Roese <sr@denx.de> Regards, Bin
On 08/21/2017 08:42 PM, Bin Meng wrote: > All these places seem to inherit the codes from the MMC driver where > a FIXME was put in the comment. However the correct operation after > read should be cache invalidate, not flush. > > The underlying drivers should be responsible for the cache operation. > Remove these codes completely. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > arch/arm/cpu/armv8/fsl-layerscape/ppa.c | 15 --------------- > board/toradex/common/tdx-cfg-block.c | 2 -- > cmd/mmc.c | 2 -- > drivers/block/blk-uclass.c | 3 --- > drivers/block/blk_legacy.c | 3 --- > drivers/net/fm/fm.c | 2 -- > drivers/net/phy/cortina.c | 2 -- > drivers/qe/qe.c | 2 -- > 8 files changed, 31 deletions(-) > > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > index 24ddb5d..bbf8bba 100644 > --- a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > +++ b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > @@ -107,9 +107,6 @@ int ppa_init(void) > return -EIO; > } > > - /* flush cache after read */ > - flush_cache((ulong)fitp, cnt * 512); > - NACK here. > ret = fdt_check_header(fitp); > if (ret) { > free(fitp); > @@ -134,9 +131,6 @@ int ppa_init(void) > } > debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); > > - /* flush cache after read */ > - flush_cache((ulong)ppa_hdr_ddr, cnt * 512); > - > ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; > #endif > > @@ -164,9 +158,6 @@ int ppa_init(void) > return -EIO; > } > > - /* flush cache after read */ > - flush_cache((ulong)ppa_fit_addr, cnt * 512); > - > #elif defined(CONFIG_SYS_LS_PPA_FW_IN_NAND) > struct fdt_header fit; > > @@ -208,9 +199,6 @@ int ppa_init(void) > } > debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); > > - /* flush cache after read */ > - flush_cache((ulong)ppa_hdr_ddr, fw_length); > - > ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; > #endif > > @@ -232,9 +220,6 @@ int ppa_init(void) > CONFIG_SYS_LS_PPA_FW_ADDR); > return -EIO; > } > - > - /* flush cache after read */ > - flush_cache((ulong)ppa_fit_addr, fw_length); > #else > #error "No CONFIG_SYS_LS_PPA_FW_IN_xxx defined" > #endif Hold on for ppa.c. We need to flush the cache after reading PPA image and it header after loading. These images need to be flushed into main memory. I will need to confirm with our team if we can remove flushing here. York
Hi York, On Wed, Aug 23, 2017 at 12:49 AM, York Sun <york.sun@nxp.com> wrote: > On 08/21/2017 08:42 PM, Bin Meng wrote: >> All these places seem to inherit the codes from the MMC driver where >> a FIXME was put in the comment. However the correct operation after >> read should be cache invalidate, not flush. >> >> The underlying drivers should be responsible for the cache operation. >> Remove these codes completely. >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> --- >> >> arch/arm/cpu/armv8/fsl-layerscape/ppa.c | 15 --------------- >> board/toradex/common/tdx-cfg-block.c | 2 -- >> cmd/mmc.c | 2 -- >> drivers/block/blk-uclass.c | 3 --- >> drivers/block/blk_legacy.c | 3 --- >> drivers/net/fm/fm.c | 2 -- >> drivers/net/phy/cortina.c | 2 -- >> drivers/qe/qe.c | 2 -- >> 8 files changed, 31 deletions(-) >> >> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c >> index 24ddb5d..bbf8bba 100644 >> --- a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c >> +++ b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c >> @@ -107,9 +107,6 @@ int ppa_init(void) >> return -EIO; >> } >> >> - /* flush cache after read */ >> - flush_cache((ulong)fitp, cnt * 512); >> - > > NACK here. > > >> ret = fdt_check_header(fitp); >> if (ret) { >> free(fitp); >> @@ -134,9 +131,6 @@ int ppa_init(void) >> } >> debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); >> >> - /* flush cache after read */ >> - flush_cache((ulong)ppa_hdr_ddr, cnt * 512); >> - >> ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; >> #endif >> >> @@ -164,9 +158,6 @@ int ppa_init(void) >> return -EIO; >> } >> >> - /* flush cache after read */ >> - flush_cache((ulong)ppa_fit_addr, cnt * 512); >> - >> #elif defined(CONFIG_SYS_LS_PPA_FW_IN_NAND) >> struct fdt_header fit; >> >> @@ -208,9 +199,6 @@ int ppa_init(void) >> } >> debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); >> >> - /* flush cache after read */ >> - flush_cache((ulong)ppa_hdr_ddr, fw_length); >> - >> ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; >> #endif >> >> @@ -232,9 +220,6 @@ int ppa_init(void) >> CONFIG_SYS_LS_PPA_FW_ADDR); >> return -EIO; >> } >> - >> - /* flush cache after read */ >> - flush_cache((ulong)ppa_fit_addr, fw_length); >> #else >> #error "No CONFIG_SYS_LS_PPA_FW_IN_xxx defined" >> #endif > > Hold on for ppa.c. We need to flush the cache after reading PPA image > and it header after loading. These images need to be flushed into main > memory. I will need to confirm with our team if we can remove flushing here. > For the first 3 parts in this ppa.c file, I see they are all reading PPA image from MMC while MMC driver is using DMA so this is wrong (as I mentioned in the commit message I suspect they are all copy/paste from the MMC driver). The only questionable change is the last one which is reading something from NAND and if NAND driver is not using DMA, that cache flush is needed as you pointed out. Regards, Bin
+Yangbo Best Regards Qiang Zhao > -----Original Message----- > From: York Sun > Sent: Wednesday, August 23, 2017 12:49 AM > To: Bin Meng <bmeng.cn@gmail.com>; Grygorii Strashko > <grygorii.strashko@ti.com>; Michal Simek <michal.simek@xilinx.com>; Angelo > Dureghello <angelo@sysam.it>; Kever Yang <kever.yang@rock-chips.com>; > Jaehoon Chung <jh80.chung@samsung.com>; Albert Aribaud > <albert.u.boot@aribaud.net>; Marcel Ziswiler <marcel.ziswiler@toradex.com>; > Markus Niebel <Markus.Niebel@tq-group.com>; Z.q. Hou > <zhiqiang.hou@nxp.com>; Sumit Garg <sumit.garg@nxp.com>; Joe Hershberger > <joe.hershberger@ni.com>; Stefan Roese <sr@denx.de>; Wenbin Song > <wenbin.song@nxp.com>; Qiang Zhao <qiang.zhao@nxp.com>; Simon Glass > <sjg@chromium.org>; Max Krummenacher > <max.krummenacher@toradex.com>; Masahiro Yamada > <yamada.masahiro@socionext.com>; Tomas Melin > <tomas.melin@vaisala.com>; Q.y. Gong <qianyu.gong@nxp.com>; Tom Rini > <trini@konsulko.com>; U-Boot Mailing List <u-boot@lists.denx.de> > Subject: Re: [PATCH] blk: Remove various places that do flush cache after read > > On 08/21/2017 08:42 PM, Bin Meng wrote: > > All these places seem to inherit the codes from the MMC driver where a > > FIXME was put in the comment. However the correct operation after read > > should be cache invalidate, not flush. > > > > The underlying drivers should be responsible for the cache operation. > > Remove these codes completely. > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > --- > > > > arch/arm/cpu/armv8/fsl-layerscape/ppa.c | 15 --------------- > > board/toradex/common/tdx-cfg-block.c | 2 -- > > cmd/mmc.c | 2 -- > > drivers/block/blk-uclass.c | 3 --- > > drivers/block/blk_legacy.c | 3 --- > > drivers/net/fm/fm.c | 2 -- > > drivers/net/phy/cortina.c | 2 -- > > drivers/qe/qe.c | 2 -- > > 8 files changed, 31 deletions(-) > > > > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > > b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > > index 24ddb5d..bbf8bba 100644 > > --- a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > > +++ b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > > @@ -107,9 +107,6 @@ int ppa_init(void) > > return -EIO; > > } > > > > - /* flush cache after read */ > > - flush_cache((ulong)fitp, cnt * 512); > > - > > NACK here. > > > > ret = fdt_check_header(fitp); > > if (ret) { > > free(fitp); > > @@ -134,9 +131,6 @@ int ppa_init(void) > > } > > debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); > > > > - /* flush cache after read */ > > - flush_cache((ulong)ppa_hdr_ddr, cnt * 512); > > - > > ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; > > #endif > > > > @@ -164,9 +158,6 @@ int ppa_init(void) > > return -EIO; > > } > > > > - /* flush cache after read */ > > - flush_cache((ulong)ppa_fit_addr, cnt * 512); > > - > > #elif defined(CONFIG_SYS_LS_PPA_FW_IN_NAND) > > struct fdt_header fit; > > > > @@ -208,9 +199,6 @@ int ppa_init(void) > > } > > debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); > > > > - /* flush cache after read */ > > - flush_cache((ulong)ppa_hdr_ddr, fw_length); > > - > > ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; > > #endif > > > > @@ -232,9 +220,6 @@ int ppa_init(void) > > CONFIG_SYS_LS_PPA_FW_ADDR); > > return -EIO; > > } > > - > > - /* flush cache after read */ > > - flush_cache((ulong)ppa_fit_addr, fw_length); > > #else > > #error "No CONFIG_SYS_LS_PPA_FW_IN_xxx defined" > > #endif > > Hold on for ppa.c. We need to flush the cache after reading PPA image and it > header after loading. These images need to be flushed into main memory. I will > need to confirm with our team if we can remove flushing here. > > York
cmd/mmc.c Maybe SD controller on some vendor platforms doesn't support SNOOP. flush_cache() is required to sync between DDR and cache. Should be careful to remove this. > -----Original Message----- > From: Qiang Zhao > Sent: Wednesday, August 23, 2017 11:14 AM > To: Y.b. Lu; York Sun; Bin Meng; Grygorii Strashko; Michal Simek; Angelo > Dureghello; Kever Yang; Jaehoon Chung; Albert Aribaud; Marcel Ziswiler; > Markus Niebel; Z.q. Hou; Sumit Garg; Joe Hershberger; Stefan Roese; > Wenbin Song; Simon Glass; Max Krummenacher; Masahiro Yamada; Tomas Melin; > Q.y. Gong; Tom Rini; U-Boot Mailing List > Subject: RE: [PATCH] blk: Remove various places that do flush cache after > read > > +Yangbo > > Best Regards > Qiang Zhao > > > -----Original Message----- > > From: York Sun > > Sent: Wednesday, August 23, 2017 12:49 AM > > To: Bin Meng <bmeng.cn@gmail.com>; Grygorii Strashko > > <grygorii.strashko@ti.com>; Michal Simek <michal.simek@xilinx.com>; > > Angelo Dureghello <angelo@sysam.it>; Kever Yang > > <kever.yang@rock-chips.com>; Jaehoon Chung <jh80.chung@samsung.com>; > > Albert Aribaud <albert.u.boot@aribaud.net>; Marcel Ziswiler > > <marcel.ziswiler@toradex.com>; Markus Niebel > > <Markus.Niebel@tq-group.com>; Z.q. Hou <zhiqiang.hou@nxp.com>; Sumit > > Garg <sumit.garg@nxp.com>; Joe Hershberger <joe.hershberger@ni.com>; > > Stefan Roese <sr@denx.de>; Wenbin Song <wenbin.song@nxp.com>; Qiang > > Zhao <qiang.zhao@nxp.com>; Simon Glass <sjg@chromium.org>; Max > > Krummenacher <max.krummenacher@toradex.com>; Masahiro Yamada > > <yamada.masahiro@socionext.com>; Tomas Melin > > <tomas.melin@vaisala.com>; Q.y. Gong <qianyu.gong@nxp.com>; Tom Rini > > <trini@konsulko.com>; U-Boot Mailing List <u-boot@lists.denx.de> > > Subject: Re: [PATCH] blk: Remove various places that do flush cache > > after read > > > > On 08/21/2017 08:42 PM, Bin Meng wrote: > > > All these places seem to inherit the codes from the MMC driver where > > > a FIXME was put in the comment. However the correct operation after > > > read should be cache invalidate, not flush. > > > > > > The underlying drivers should be responsible for the cache operation. > > > Remove these codes completely. > > > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > > --- > > > > > > arch/arm/cpu/armv8/fsl-layerscape/ppa.c | 15 --------------- > > > board/toradex/common/tdx-cfg-block.c | 2 -- > > > cmd/mmc.c | 2 -- > > > drivers/block/blk-uclass.c | 3 --- > > > drivers/block/blk_legacy.c | 3 --- > > > drivers/net/fm/fm.c | 2 -- > > > drivers/net/phy/cortina.c | 2 -- > > > drivers/qe/qe.c | 2 -- > > > 8 files changed, 31 deletions(-) > > > > > > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > > > b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > > > index 24ddb5d..bbf8bba 100644 > > > --- a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > > > +++ b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c > > > @@ -107,9 +107,6 @@ int ppa_init(void) > > > return -EIO; > > > } > > > > > > - /* flush cache after read */ > > > - flush_cache((ulong)fitp, cnt * 512); > > > - > > > > NACK here. > > > > > > > ret = fdt_check_header(fitp); > > > if (ret) { > > > free(fitp); > > > @@ -134,9 +131,6 @@ int ppa_init(void) > > > } > > > debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); > > > > > > - /* flush cache after read */ > > > - flush_cache((ulong)ppa_hdr_ddr, cnt * 512); > > > - > > > ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; > > > #endif > > > > > > @@ -164,9 +158,6 @@ int ppa_init(void) > > > return -EIO; > > > } > > > > > > - /* flush cache after read */ > > > - flush_cache((ulong)ppa_fit_addr, cnt * 512); > > > - > > > #elif defined(CONFIG_SYS_LS_PPA_FW_IN_NAND) > > > struct fdt_header fit; > > > > > > @@ -208,9 +199,6 @@ int ppa_init(void) > > > } > > > debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); > > > > > > - /* flush cache after read */ > > > - flush_cache((ulong)ppa_hdr_ddr, fw_length); > > > - > > > ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; > > > #endif > > > > > > @@ -232,9 +220,6 @@ int ppa_init(void) > > > CONFIG_SYS_LS_PPA_FW_ADDR); > > > return -EIO; > > > } > > > - > > > - /* flush cache after read */ > > > - flush_cache((ulong)ppa_fit_addr, fw_length); > > > #else > > > #error "No CONFIG_SYS_LS_PPA_FW_IN_xxx defined" > > > #endif > > > > Hold on for ppa.c. We need to flush the cache after reading PPA image > > and it header after loading. These images need to be flushed into main > > memory. I will need to confirm with our team if we can remove flushing > here. > > > > York
Hi Yangbo, On Wed, Aug 23, 2017 at 11:29 AM, Y.b. Lu <yangbo.lu@nxp.com> wrote: > cmd/mmc.c > > Maybe SD controller on some vendor platforms doesn't support SNOOP. > flush_cache() is required to sync between DDR and cache. > Should be careful to remove this. > Please do not top-posting. What you mentioned if snoop is not supported, flush_cache() is required, is true, however that's not the issue I am trying to fix here. The flush_cache() here after a DMA read will cause issues because stale data in the cache will overwrite the correct data in the RAM that is DMAed by hardware (in this case, MMC controller). Regards, Bin
On 08/22/2017 03:53 PM, Bin Meng wrote: > Hi York, > > On Wed, Aug 23, 2017 at 12:49 AM, York Sun <york.sun@nxp.com> wrote: >> On 08/21/2017 08:42 PM, Bin Meng wrote: >>> All these places seem to inherit the codes from the MMC driver where >>> a FIXME was put in the comment. However the correct operation after >>> read should be cache invalidate, not flush. >>> >>> The underlying drivers should be responsible for the cache operation. >>> Remove these codes completely. >>> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>> --- >>> >>> arch/arm/cpu/armv8/fsl-layerscape/ppa.c | 15 --------------- >>> board/toradex/common/tdx-cfg-block.c | 2 -- >>> cmd/mmc.c | 2 -- >>> drivers/block/blk-uclass.c | 3 --- >>> drivers/block/blk_legacy.c | 3 --- >>> drivers/net/fm/fm.c | 2 -- >>> drivers/net/phy/cortina.c | 2 -- >>> drivers/qe/qe.c | 2 -- >>> 8 files changed, 31 deletions(-) >>> >>> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c >>> index 24ddb5d..bbf8bba 100644 >>> --- a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c >>> +++ b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c >>> @@ -107,9 +107,6 @@ int ppa_init(void) >>> return -EIO; >>> } >>> >>> - /* flush cache after read */ >>> - flush_cache((ulong)fitp, cnt * 512); >>> - >> >> NACK here. >> >> >>> ret = fdt_check_header(fitp); >>> if (ret) { >>> free(fitp); >>> @@ -134,9 +131,6 @@ int ppa_init(void) >>> } >>> debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); >>> >>> - /* flush cache after read */ >>> - flush_cache((ulong)ppa_hdr_ddr, cnt * 512); >>> - >>> ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; >>> #endif >>> >>> @@ -164,9 +158,6 @@ int ppa_init(void) >>> return -EIO; >>> } >>> >>> - /* flush cache after read */ >>> - flush_cache((ulong)ppa_fit_addr, cnt * 512); >>> - >>> #elif defined(CONFIG_SYS_LS_PPA_FW_IN_NAND) >>> struct fdt_header fit; >>> >>> @@ -208,9 +199,6 @@ int ppa_init(void) >>> } >>> debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); >>> >>> - /* flush cache after read */ >>> - flush_cache((ulong)ppa_hdr_ddr, fw_length); >>> - >>> ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; >>> #endif >>> >>> @@ -232,9 +220,6 @@ int ppa_init(void) >>> CONFIG_SYS_LS_PPA_FW_ADDR); >>> return -EIO; >>> } >>> - >>> - /* flush cache after read */ >>> - flush_cache((ulong)ppa_fit_addr, fw_length); >>> #else >>> #error "No CONFIG_SYS_LS_PPA_FW_IN_xxx defined" >>> #endif >> >> Hold on for ppa.c. We need to flush the cache after reading PPA image >> and it header after loading. These images need to be flushed into main >> memory. I will need to confirm with our team if we can remove flushing here. >> > > For the first 3 parts in this ppa.c file, I see they are all reading > PPA image from MMC while MMC driver is using DMA so this is wrong (as > I mentioned in the commit message I suspect they are all copy/paste > from the MMC driver). > > The only questionable change is the last one which is reading > something from NAND and if NAND driver is not using DMA, that cache > flush is needed as you pointed out. > Bin, Our internal team agrees with the removing flushing for MMC. Give me some time to test NAND. York
On 08/21/2017 08:42 PM, Bin Meng wrote: > All these places seem to inherit the codes from the MMC driver where > a FIXME was put in the comment. However the correct operation after > read should be cache invalidate, not flush. > > The underlying drivers should be responsible for the cache operation. > Remove these codes completely. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > arch/arm/cpu/armv8/fsl-layerscape/ppa.c | 15 --------------- > board/toradex/common/tdx-cfg-block.c | 2 -- > cmd/mmc.c | 2 -- > drivers/block/blk-uclass.c | 3 --- > drivers/block/blk_legacy.c | 3 --- > drivers/net/fm/fm.c | 2 -- > drivers/net/phy/cortina.c | 2 -- > drivers/qe/qe.c | 2 -- > 8 files changed, 31 deletions(-) > Tested on LS1043ARDB with SD and NAND boot for PPA and FM. Tested-by: York Sun <york.sun@nxp.com> Reviewed-by: York Sun <york.sun@nxp.com>
> -----Original Message----- > From: Bin Meng [mailto:bmeng.cn@gmail.com] > Sent: Wednesday, August 23, 2017 1:29 PM > To: Y.b. Lu > Cc: Qiang Zhao; York Sun; Grygorii Strashko; Michal Simek; Angelo > Dureghello; Kever Yang; Jaehoon Chung; Albert Aribaud; Marcel Ziswiler; > Markus Niebel; Z.q. Hou; Sumit Garg; Joe Hershberger; Stefan Roese; > Wenbin Song; Simon Glass; Max Krummenacher; Masahiro Yamada; Tomas Melin; > Q.y. Gong; Tom Rini; U-Boot Mailing List > Subject: Re: [PATCH] blk: Remove various places that do flush cache after > read > > Hi Yangbo, > > On Wed, Aug 23, 2017 at 11:29 AM, Y.b. Lu <yangbo.lu@nxp.com> wrote: > > cmd/mmc.c > > > > Maybe SD controller on some vendor platforms doesn't support SNOOP. > > flush_cache() is required to sync between DDR and cache. > > Should be careful to remove this. > > > > Please do not top-posting. > > What you mentioned if snoop is not supported, flush_cache() is required, > is true, however that's not the issue I am trying to fix here. The > flush_cache() here after a DMA read will cause issues because stale data > in the cache will overwrite the correct data in the RAM that is DMAed by > hardware (in this case, MMC controller). > [Lu Yangbo-B47093] Sorry for my misunderstanding. It looks ok to me now. flush_cache is indeed redundant after DMA reading. > Regards, > Bin
On Mon, Aug 21, 2017 at 10:46 PM, Bin Meng <bmeng.cn@gmail.com> wrote: > All these places seem to inherit the codes from the MMC driver where > a FIXME was put in the comment. However the correct operation after > read should be cache invalidate, not flush. > > The underlying drivers should be responsible for the cache operation. > Remove these codes completely. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
On 21 August 2017 at 21:46, Bin Meng <bmeng.cn@gmail.com> wrote: > All these places seem to inherit the codes from the MMC driver where > a FIXME was put in the comment. However the correct operation after > read should be cache invalidate, not flush. > > The underlying drivers should be responsible for the cache operation. > Remove these codes completely. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > arch/arm/cpu/armv8/fsl-layerscape/ppa.c | 15 --------------- > board/toradex/common/tdx-cfg-block.c | 2 -- > cmd/mmc.c | 2 -- > drivers/block/blk-uclass.c | 3 --- > drivers/block/blk_legacy.c | 3 --- > drivers/net/fm/fm.c | 2 -- > drivers/net/phy/cortina.c | 2 -- > drivers/qe/qe.c | 2 -- > 8 files changed, 31 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org>
diff --git a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c index 24ddb5d..bbf8bba 100644 --- a/arch/arm/cpu/armv8/fsl-layerscape/ppa.c +++ b/arch/arm/cpu/armv8/fsl-layerscape/ppa.c @@ -107,9 +107,6 @@ int ppa_init(void) return -EIO; } - /* flush cache after read */ - flush_cache((ulong)fitp, cnt * 512); - ret = fdt_check_header(fitp); if (ret) { free(fitp); @@ -134,9 +131,6 @@ int ppa_init(void) } debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); - /* flush cache after read */ - flush_cache((ulong)ppa_hdr_ddr, cnt * 512); - ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; #endif @@ -164,9 +158,6 @@ int ppa_init(void) return -EIO; } - /* flush cache after read */ - flush_cache((ulong)ppa_fit_addr, cnt * 512); - #elif defined(CONFIG_SYS_LS_PPA_FW_IN_NAND) struct fdt_header fit; @@ -208,9 +199,6 @@ int ppa_init(void) } debug("Read PPA header to 0x%p\n", ppa_hdr_ddr); - /* flush cache after read */ - flush_cache((ulong)ppa_hdr_ddr, fw_length); - ppa_esbc_hdr = (uintptr_t)ppa_hdr_ddr; #endif @@ -232,9 +220,6 @@ int ppa_init(void) CONFIG_SYS_LS_PPA_FW_ADDR); return -EIO; } - - /* flush cache after read */ - flush_cache((ulong)ppa_fit_addr, fw_length); #else #error "No CONFIG_SYS_LS_PPA_FW_IN_xxx defined" #endif diff --git a/board/toradex/common/tdx-cfg-block.c b/board/toradex/common/tdx-cfg-block.c index 328c4c0..f850a3c 100644 --- a/board/toradex/common/tdx-cfg-block.c +++ b/board/toradex/common/tdx-cfg-block.c @@ -129,8 +129,6 @@ static int tdx_cfg_block_mmc_storage(u8 *config_block, int write) ret = -EIO; goto out; } - /* Flush cache after read */ - flush_cache((ulong)(unsigned char *)config_block, 512); } else { /* Just writing one 512 byte block */ if (blk_dwrite(mmc_get_blk_desc(mmc), blk_start, 1, diff --git a/cmd/mmc.c b/cmd/mmc.c index 00697fc..5def4ea 100644 --- a/cmd/mmc.c +++ b/cmd/mmc.c @@ -293,8 +293,6 @@ static int do_mmc_read(cmd_tbl_t *cmdtp, int flag, curr_device, blk, cnt); n = blk_dread(mmc_get_blk_desc(mmc), blk, cnt, addr); - /* flush cache after read */ - flush_cache((ulong)addr, cnt * 512); /* FIXME */ printf("%d blocks read: %s\n", n, (n == cnt) ? "OK" : "ERROR"); return (n == cnt) ? CMD_RET_SUCCESS : CMD_RET_FAILURE; diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c index 3aec569..e5f00dc 100644 --- a/drivers/block/blk-uclass.c +++ b/drivers/block/blk-uclass.c @@ -294,9 +294,6 @@ ulong blk_read_devnum(enum if_type if_type, int devnum, lbaint_t start, if (IS_ERR_VALUE(n)) return n; - /* flush cache after read */ - flush_cache((ulong)buffer, blkcnt * desc->blksz); - return n; } diff --git a/drivers/block/blk_legacy.c b/drivers/block/blk_legacy.c index 981872e..16d3bfe 100644 --- a/drivers/block/blk_legacy.c +++ b/drivers/block/blk_legacy.c @@ -232,9 +232,6 @@ ulong blk_read_devnum(enum if_type if_type, int devnum, lbaint_t start, if (IS_ERR_VALUE(n)) return n; - /* flush cache after read */ - flush_cache((ulong)buffer, blkcnt * desc->blksz); - return n; } diff --git a/drivers/net/fm/fm.c b/drivers/net/fm/fm.c index 451dfde..261f1b9 100644 --- a/drivers/net/fm/fm.c +++ b/drivers/net/fm/fm.c @@ -405,8 +405,6 @@ int fm_init_common(int index, struct ccsr_fman *reg) mmc_init(mmc); (void)mmc->block_dev.block_read(&mmc->block_dev, blk, cnt, addr); - /* flush cache after read */ - flush_cache((ulong)addr, cnt * 512); } #elif defined(CONFIG_SYS_QE_FMAN_FW_IN_REMOTE) void *addr = (void *)CONFIG_SYS_FMAN_FW_ADDR; diff --git a/drivers/net/phy/cortina.c b/drivers/net/phy/cortina.c index e0e9ed9..637d89a 100644 --- a/drivers/net/phy/cortina.c +++ b/drivers/net/phy/cortina.c @@ -177,8 +177,6 @@ void cs4340_upload_firmware(struct phy_device *phydev) mmc_init(mmc); (void)mmc->block_dev.block_read(&mmc->block_dev, blk, cnt, addr); - /* flush cache after read */ - flush_cache((ulong)addr, cnt * 512); } #endif diff --git a/drivers/qe/qe.c b/drivers/qe/qe.c index 24e764d..931c9d9 100644 --- a/drivers/qe/qe.c +++ b/drivers/qe/qe.c @@ -221,8 +221,6 @@ void u_qe_init(void) mmc_init(mmc); (void)mmc->block_dev.block_read(&mmc->block_dev, blk, cnt, addr); - /* flush cache after read */ - flush_cache((ulong)addr, cnt * 512); } #endif u_qe_upload_firmware(addr);
All these places seem to inherit the codes from the MMC driver where a FIXME was put in the comment. However the correct operation after read should be cache invalidate, not flush. The underlying drivers should be responsible for the cache operation. Remove these codes completely. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- arch/arm/cpu/armv8/fsl-layerscape/ppa.c | 15 --------------- board/toradex/common/tdx-cfg-block.c | 2 -- cmd/mmc.c | 2 -- drivers/block/blk-uclass.c | 3 --- drivers/block/blk_legacy.c | 3 --- drivers/net/fm/fm.c | 2 -- drivers/net/phy/cortina.c | 2 -- drivers/qe/qe.c | 2 -- 8 files changed, 31 deletions(-)