| Message ID | 20240326233502.2625015-1-jonas@kwiboo.se |
|---|---|
| State | Superseded |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | mmc: rockchip_sdhci: Revert 4 blocks PIO mode read limit for RK35xx | expand |
Hi Jonas, On 3/27/24 00:35, Jonas Karlman wrote: > The commit 2cc6cde647e2 ("mmc: rockchip_sdhci: Limit number of blocks > read in a single command") introduced a limit of number of blocks to > read to fix a Data End Bit Error on RK3568 and RK3588. This had a side > affect of significant slowing down reading FIT from eMMC. > > After the commit 6de9d7b2f13c ("rockchip: rk35xx: Enable eMMC HS200 mode > by default") the limit of number of blocks to read workaround is no > longer necessary and a Data End Bit Error is no longer happening using > PIO mode. > > Revert this limitation to allow reading more than 4 blocks with a single > CMD18 command in PIO mode and speed up reading FIT from eMMC. > Should we instead keep this code but surround it with #if !IS_ENABLED(MMC_HS200_SUPPORT) ? After all, HS200 is only implied for RK35xx boards, so one could disable it. Also wondering whether we should do this dynamically based on the MMC mode that could be negotiated with the chip? E.g. I think it's not guaranteed that with hs200 support built and enabled in DT that it'll be negotiated as HS200? Cheers, Quentin
On 2024-03-27 09:56, Quentin Schulz wrote: > On 3/27/24 00:35, Jonas Karlman wrote: >> The commit 2cc6cde647e2 ("mmc: rockchip_sdhci: Limit number of blocks >> read in a single command") introduced a limit of number of blocks to >> read to fix a Data End Bit Error on RK3568 and RK3588. This had a side >> affect of significant slowing down reading FIT from eMMC. >> >> After the commit 6de9d7b2f13c ("rockchip: rk35xx: Enable eMMC HS200 >> mode >> by default") the limit of number of blocks to read workaround is no >> longer necessary and a Data End Bit Error is no longer happening using >> PIO mode. >> >> Revert this limitation to allow reading more than 4 blocks with a >> single >> CMD18 command in PIO mode and speed up reading FIT from eMMC. > > Should we instead keep this code but surround it with > #if !IS_ENABLED(MMC_HS200_SUPPORT) > ? > > After all, HS200 is only implied for RK35xx boards, so one could > disable it. > > Also wondering whether we should do this dynamically based on the MMC > mode that could be negotiated with the chip? E.g. I think it's not > guaranteed that with hs200 support built and enabled in DT that it'll > be negotiated as HS200? Totally agreed with Quentin's remarks.
Hi Quentin, On 2024-03-27 09:56, Quentin Schulz wrote: > Hi Jonas, > > On 3/27/24 00:35, Jonas Karlman wrote: >> The commit 2cc6cde647e2 ("mmc: rockchip_sdhci: Limit number of blocks >> read in a single command") introduced a limit of number of blocks to >> read to fix a Data End Bit Error on RK3568 and RK3588. This had a side >> affect of significant slowing down reading FIT from eMMC. >> >> After the commit 6de9d7b2f13c ("rockchip: rk35xx: Enable eMMC HS200 mode >> by default") the limit of number of blocks to read workaround is no >> longer necessary and a Data End Bit Error is no longer happening using >> PIO mode. >> >> Revert this limitation to allow reading more than 4 blocks with a single >> CMD18 command in PIO mode and speed up reading FIT from eMMC. >> > > Should we instead keep this code but surround it with > #if !IS_ENABLED(MMC_HS200_SUPPORT) > ? > > After all, HS200 is only implied for RK35xx boards, so one could disable it. I do not think this will be necessary, for RK3568 the PIO mode is not needed at all and SDMA is used. And not 100% sure if it was the HS200 or bss address change, or a combo of both that fixes the Data End Bit Error. Or could also be that it was the DDR52 mode that caused issues. Will re-try with normal HS mode and see if any hack is still needed at all. > > Also wondering whether we should do this dynamically based on the MMC > mode that could be negotiated with the chip? E.g. I think it's not > guaranteed that with hs200 support built and enabled in DT that it'll be > negotiated as HS200? Last time I checked this there was no easy way to change number of blocks to read at time using current mmc/sdhci ops, and I do not think there is enough need to add new ops. However, if we still need a workaround it should be changed to use a bounce buffer instead of limit number of blocks to not affect performance, similar to [1]. [1] https://patchwork.ozlabs.org/patch/1895377/ Regards, Jonas > > Cheers, > Quentin
diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c index 706fb1235796..45587222a230 100644 --- a/drivers/mmc/rockchip_sdhci.c +++ b/drivers/mmc/rockchip_sdhci.c @@ -598,16 +598,6 @@ static int rockchip_sdhci_probe(struct udevice *dev) dev_read_bool(dev, "u-boot,spl-fifo-mode")) host->flags &= ~USE_DMA; - /* - * Reading more than 4 blocks with a single CMD18 command in PIO mode - * triggers Data End Bit Error on RK3568 and RK3588. Limit to reading - * max 4 blocks in one command when using PIO mode. - */ - if (!(host->flags & USE_DMA) && - (device_is_compatible(dev, "rockchip,rk3568-dwcmshc") || - device_is_compatible(dev, "rockchip,rk3588-dwcmshc"))) - cfg->b_max = 4; - return sdhci_probe(dev); }
The commit 2cc6cde647e2 ("mmc: rockchip_sdhci: Limit number of blocks read in a single command") introduced a limit of number of blocks to read to fix a Data End Bit Error on RK3568 and RK3588. This had a side affect of significant slowing down reading FIT from eMMC. After the commit 6de9d7b2f13c ("rockchip: rk35xx: Enable eMMC HS200 mode by default") the limit of number of blocks to read workaround is no longer necessary and a Data End Bit Error is no longer happening using PIO mode. Revert this limitation to allow reading more than 4 blocks with a single CMD18 command in PIO mode and speed up reading FIT from eMMC. Signed-off-by: Jonas Karlman <jonas@kwiboo.se> --- This is a replacement for the prior [1] "mmc: rockchip_sdhci: Use bounce buffer in SPL to fix read performance" patch. [1] https://patchwork.ozlabs.org/patch/1895377/ --- drivers/mmc/rockchip_sdhci.c | 10 ---------- 1 file changed, 10 deletions(-)