| Message ID | 20230921-th1520-mmc-v1-0-49f76c274fb3@baylibre.com |
|---|---|
| Headers | show |
| Series | RISC-V: Add eMMC support for TH1520 boards | expand |
在 2023-09-22星期五的 19:41 +0800，Xi Ruoyao写道： > On Thu, 2023-09-21 at 18:49 -0700, Drew Fustini wrote: > > This series adds support for the eMMC on the BeagleV Ahead and the > > Sipeed LicheePi 4A. This allows the kernel to boot with the rootfs > > on > > eMMC. > > > > I tested on top of v6.6-rc2 with this config [1]. I was able to > > boot > > both the Ahead [2] and LPi4a [3] from eMMC. The following > > prerequisites > > are required: > > > > [PATCH v2] riscv: dts: thead: set dma-noncoherent to soc bus [4] > > > > I pushed a branch [5] with this patch series and the above patch > > for > > those that find a git branch easier to test. > > > > Please note that only the MMC controller connected to the eMMC > > device > > is enabled in the device trees for these two boards. I did not yet > > attempt to configure and use the microSD card slot. My preference > > is to > > address that in a future patch series. > > > > References: > > [1] https://gist.github.com/pdp7/5fbdcf2a65eb1abdd3a29d519c19cdd2 > > [2] https://gist.github.com/pdp7/91a801a5f8d1070c53509eda9800ad78 > > [3] https://gist.github.com/pdp7/1445c3c991e88fd69c60165cef65726a > > [4] > > https://lore.kernel.org/linux-riscv/20230912072232.2455-1-jszhang@kernel.org/ > > [5] https://github.com/pdp7/linux/tree/b4/th1520-mmc > > I've tested this branch and successfully booted a rootfs on Lichee Pi > 4A > eMMC with rootdelay=10. > > Curiously is there some way to make it work without rootdelay? The answer is nearly no (although using an initrd will mitigate the need of rootdelay). MMC devices are known to be slow to probe, even on x86 devices. > > For everything except "Enable BeagleV Ahead eMMC controller": > > Tested-by: Xi Ruoyao <xry111@xry111.site> >
On Fri, 2023-09-22 at 09:23 -0700, Drew Fustini wrote: > > I've tested this branch and successfully booted a rootfs on Lichee Pi 4A > > eMMC with rootdelay=10. > > > > Curiously is there some way to make it work without rootdelay? > > Thank you for testing. > > This is the kernel command line that I am using on both the lpi4 and > the ahead: > > root=/dev/mmcblk0p3 ro rootfstype=ext4 rootwait console=ttyS0,115200 > > I seem to recall that before I used rootwait that there would be a VFS > oops because mmcblk0p3 didn't exist yet. > > Have you tried rootwait instead of the 10 second delay? I just tried rootwait several times and it works for me. (When I tried it yesterday it did not work initially, so I switched to rootdelay. I guess I'd mistyped something in the initial attempt).
On 22/09/23 04:49, Drew Fustini wrote: > Expose __sdhci_execute_tuning() so that it can be called from the > mmc host controller drivers. > > In the sdhci-of-dwcmshc driver, sdhci_dwcmshc_th1520_ops sets > platform_execute_tuning to th1520_execute_tuning(). That function has > to manipulate phy registers before tuning can be performed. To avoid > copying the code verbatim from __sdhci_execute_tuning() into > th1520_execute_tuning(), make it possible for __sdhci_execute_tuning() > to be called from sdhci-of-dwcmshc. > > Signed-off-by: Drew Fustini <dfustini@baylibre.com> > --- > drivers/mmc/host/sdhci.c | 2 +- > drivers/mmc/host/sdhci.h | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index ff41aa56564e..fd607058d176 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -2841,7 +2841,7 @@ void sdhci_send_tuning(struct sdhci_host *host, u32 opcode) > } > EXPORT_SYMBOL_GPL(sdhci_send_tuning); > > -static int __sdhci_execute_tuning(struct sdhci_host *host, u32 opcode) > +int __sdhci_execute_tuning(struct sdhci_host *host, u32 opcode) Also need EXPORT_SYMBOL_GPL(__sdhci_execute_tuning); > { > int i; > > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h > index f219bdea8f28..a20864fc0641 100644 > --- a/drivers/mmc/host/sdhci.h > +++ b/drivers/mmc/host/sdhci.h > @@ -793,6 +793,7 @@ void sdhci_set_bus_width(struct sdhci_host *host, int width); > void sdhci_reset(struct sdhci_host *host, u8 mask); > void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing); > int sdhci_execute_tuning(struct mmc_host *mmc, u32 opcode); > +int __sdhci_execute_tuning(struct sdhci_host *host, u32 opcode); > void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios); > int sdhci_start_signal_voltage_switch(struct mmc_host *mmc, > struct mmc_ios *ios); >
On Mon, Sep 25, 2023 at 01:21:05PM +0300, Adrian Hunter wrote: > On 22/09/23 04:49, Drew Fustini wrote: > > Expose __sdhci_execute_tuning() so that it can be called from the > > mmc host controller drivers. > > > > In the sdhci-of-dwcmshc driver, sdhci_dwcmshc_th1520_ops sets > > platform_execute_tuning to th1520_execute_tuning(). That function has > > to manipulate phy registers before tuning can be performed. To avoid > > copying the code verbatim from __sdhci_execute_tuning() into > > th1520_execute_tuning(), make it possible for __sdhci_execute_tuning() > > to be called from sdhci-of-dwcmshc. > > > > Signed-off-by: Drew Fustini <dfustini@baylibre.com> > > --- > > drivers/mmc/host/sdhci.c | 2 +- > > drivers/mmc/host/sdhci.h | 1 + > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > > index ff41aa56564e..fd607058d176 100644 > > --- a/drivers/mmc/host/sdhci.c > > +++ b/drivers/mmc/host/sdhci.c > > @@ -2841,7 +2841,7 @@ void sdhci_send_tuning(struct sdhci_host *host, u32 opcode) > > } > > EXPORT_SYMBOL_GPL(sdhci_send_tuning); > > > > -static int __sdhci_execute_tuning(struct sdhci_host *host, u32 opcode) > > +int __sdhci_execute_tuning(struct sdhci_host *host, u32 opcode) > > Also need > EXPORT_SYMBOL_GPL(__sdhci_execute_tuning); Thank, I will add that. I wasn't sure if making __sdhci_execute_tuning() available outside of sdhci.c was going to be seen as an acceptable solution. Do you think my apporach is acceptable (once I add EXPORT_SYMBOL_GPL)? Thanks, Drew
On Mon, Oct 02, 2023 at 09:37:44PM -0700, Drew Fustini wrote: > On Fri, Sep 22, 2023 at 05:48:21PM -0500, Robert Nelson wrote: > > On Fri, Sep 22, 2023 at 2:08 PM Robert Nelson <robertcnelson@gmail.com> wrote: > > > > > > On Thu, Sep 21, 2023 at 8:51 PM Drew Fustini <dfustini@baylibre.com> wrote: > > > > > > > > This series adds support for the eMMC on the BeagleV Ahead and the > > > > Sipeed LicheePi 4A. This allows the kernel to boot with the rootfs on > > > > eMMC. > > > > > > > > I tested on top of v6.6-rc2 with this config [1]. I was able to boot > > > > both the Ahead [2] and LPi4a [3] from eMMC. The following prerequisites > > > > are required: > > > > > > > > [PATCH v2] riscv: dts: thead: set dma-noncoherent to soc bus [4] > > > > > > > > I pushed a branch [5] with this patch series and the above patch for > > > > those that find a git branch easier to test. > > > > > > > > Please note that only the MMC controller connected to the eMMC device > > > > is enabled in the device trees for these two boards. I did not yet > > > > attempt to configure and use the microSD card slot. My preference is to > > > > address that in a future patch series. > > > > > > > > References: > > > > [1] https://gist.github.com/pdp7/5fbdcf2a65eb1abdd3a29d519c19cdd2 > > > > [2] https://gist.github.com/pdp7/91a801a5f8d1070c53509eda9800ad78 > > > > [3] https://gist.github.com/pdp7/1445c3c991e88fd69c60165cef65726a > > > > [4] https://lore.kernel.org/linux-riscv/20230912072232.2455-1-jszhang@kernel.org/ > > > > [5] https://github.com/pdp7/linux/tree/b4/th1520-mmc > > > > > > This patchset came out very nice! > > > > > > v6.6-rc2 with Last RFC v2: > > > > > > [ 4.066630] mmc0: SDHCI controller on ffe7080000.mmc > > > [ffe7080000.mmc] using PIO > > > > > > debian@BeagleV:~$ sudo hdparm -tT /dev/mmcblk0 > > > > > > /dev/mmcblk0: > > > Timing cached reads: 1516 MB in 2.00 seconds = 758.09 MB/sec > > > Timing buffered disk reads: 84 MB in 3.01 seconds = 27.94 MB/sec > > > > > > vs v6.6-rc2 with this patchset: > > > > > > [ 4.096837] mmc0: SDHCI controller on ffe7080000.mmc > > > [ffe7080000.mmc] using DMA > > > > > > debian@BeagleV:~$ sudo hdparm -tT /dev/mmcblk0 > > > > > > /dev/mmcblk0: > > > Timing cached reads: 1580 MB in 2.00 seconds = 790.97 MB/sec > > > Timing buffered disk reads: 418 MB in 3.00 seconds = 139.11 MB/sec > > > > Drew pointed out on Slack, this was not quite right.. After more > > digging by Drew, CONFIG_DMA_GLOBAL_POOL is causing a DMA limitation > > with the multiplatform defconfig. so with, > > > > ./scripts/config --disable CONFIG_ARCH_R9A07G043 > > > > (to remove CONFIG_DMA_GLOBAL_POOL)... another 2x in buffered reads.. > > > > [ 4.059242] mmc0: SDHCI controller on ffe7080000.mmc > > [ffe7080000.mmc] using ADMA 64-bit > > > > debian@BeagleV:~$ sudo hdparm -tT /dev/mmcblk0 > > > > /dev/mmcblk0: > > Timing cached reads: 1600 MB in 2.00 seconds = 800.93 MB/sec > > Timing buffered disk reads: 892 MB in 3.00 seconds = 297.06 MB/sec > > It seems CONFIG_DMA_GLOBAL_POOL=y causes ADMA buffer alloc to fail [1]: > > mmc0: Unable to allocate ADMA buffers - falling back to standard DMA > > Prabhakar's AX45MP non-coherent DMA support [2] series introduced the > selection of DMA_GLOBAL_POOL for ARCH_R9A07G043 and the riscv defconfig > selects ARCH_R9A07G043. > > Patch 5 in the series [3] states that: > > With DMA_GLOBAL_POOL enabled all DMA allocations happen from this > region and synchronization callbacks are implemented to synchronize > when doing DMA transactions. > > This example of a "shared-dma-pool" node was given: > > pma_resv0@58000000 { > compatible = "shared-dma-pool"; > reg = <0x0 0x58000000 0x0 0x08000000>; > no-map; > linux,dma-default; > }; > > I've copied that to th1520-beaglev-ahead.dts. The address of 0x58000000 > has no significance on th1520, but the existence of shared-dma-pool > seems to fix the problem. ADMA mode [4] is now working even though > CONFIG_DMA_GLOBAL_POOL=y. + Christoph, Lad IMHO, this is not TH1520 specific but a generic issue. I believe commit 484861e09f3e ("soc: renesas: Kconfig: Select the required configs for RZ/Five SoC") can cause regression on all non-dma-coherent riscv platforms with generic defconfig. This is a common issue. The logic here is: generic riscv defconfig selects ARCH_R9A07G043 which selects DMA_GLOBAL_POOL, which assumes all non-dma-coherent riscv platforms have a dma global pool, this assumption seems not correct. And I believe DMA_GLOBAL_POOL should not be selected by ARCH_SOCFAMILIY, instead, only ARCH under some specific conditions can select it globaly, for example NOMMU ARM and so on. Since this is a regression, what's proper fix? any suggestion is appreciated. Thanks > > Thanks, > Drew > > [1] https://gist.github.com/pdp7/73041ed808bbc7dd445836fb90574979 > [2] https://lore.kernel.org/linux-riscv/20230818135723.80612-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ > [3] https://lore.kernel.org/linux-riscv/20230818135723.80612-6-prabhakar.mahadev-lad.rj@bp.renesas.com/ > [4] https://gist.github.com/pdp7/91e72a663d3bb73eb28182337ad8bbcb
On 04/10/2023 2:02 pm, Lad, Prabhakar wrote: > + CC linux-mm and Robin Murphy > > On Wed, Oct 4, 2023 at 12:42 PM Jisheng Zhang <jszhang@kernel.org> wrote: >> >> On Mon, Oct 02, 2023 at 09:37:44PM -0700, Drew Fustini wrote: >>> On Fri, Sep 22, 2023 at 05:48:21PM -0500, Robert Nelson wrote: >>>> On Fri, Sep 22, 2023 at 2:08 PM Robert Nelson <robertcnelson@gmail.com> wrote: >>>>> >>>>> On Thu, Sep 21, 2023 at 8:51 PM Drew Fustini <dfustini@baylibre.com> wrote: >>>>>> >>>>>> This series adds support for the eMMC on the BeagleV Ahead and the >>>>>> Sipeed LicheePi 4A. This allows the kernel to boot with the rootfs on >>>>>> eMMC. >>>>>> >>>>>> I tested on top of v6.6-rc2 with this config [1]. I was able to boot >>>>>> both the Ahead [2] and LPi4a [3] from eMMC. The following prerequisites >>>>>> are required: >>>>>> >>>>>> [PATCH v2] riscv: dts: thead: set dma-noncoherent to soc bus [4] >>>>>> >>>>>> I pushed a branch [5] with this patch series and the above patch for >>>>>> those that find a git branch easier to test. >>>>>> >>>>>> Please note that only the MMC controller connected to the eMMC device >>>>>> is enabled in the device trees for these two boards. I did not yet >>>>>> attempt to configure and use the microSD card slot. My preference is to >>>>>> address that in a future patch series. >>>>>> >>>>>> References: >>>>>> [1] https://gist.github.com/pdp7/5fbdcf2a65eb1abdd3a29d519c19cdd2 >>>>>> [2] https://gist.github.com/pdp7/91a801a5f8d1070c53509eda9800ad78 >>>>>> [3] https://gist.github.com/pdp7/1445c3c991e88fd69c60165cef65726a >>>>>> [4] https://lore.kernel.org/linux-riscv/20230912072232.2455-1-jszhang@kernel.org/ >>>>>> [5] https://github.com/pdp7/linux/tree/b4/th1520-mmc >>>>> >>>>> This patchset came out very nice! >>>>> >>>>> v6.6-rc2 with Last RFC v2: >>>>> >>>>> [ 4.066630] mmc0: SDHCI controller on ffe7080000.mmc >>>>> [ffe7080000.mmc] using PIO >>>>> >>>>> debian@BeagleV:~$ sudo hdparm -tT /dev/mmcblk0 >>>>> >>>>> /dev/mmcblk0: >>>>> Timing cached reads: 1516 MB in 2.00 seconds = 758.09 MB/sec >>>>> Timing buffered disk reads: 84 MB in 3.01 seconds = 27.94 MB/sec >>>>> >>>>> vs v6.6-rc2 with this patchset: >>>>> >>>>> [ 4.096837] mmc0: SDHCI controller on ffe7080000.mmc >>>>> [ffe7080000.mmc] using DMA >>>>> >>>>> debian@BeagleV:~$ sudo hdparm -tT /dev/mmcblk0 >>>>> >>>>> /dev/mmcblk0: >>>>> Timing cached reads: 1580 MB in 2.00 seconds = 790.97 MB/sec >>>>> Timing buffered disk reads: 418 MB in 3.00 seconds = 139.11 MB/sec >>>> >>>> Drew pointed out on Slack, this was not quite right.. After more >>>> digging by Drew, CONFIG_DMA_GLOBAL_POOL is causing a DMA limitation >>>> with the multiplatform defconfig. so with, >>>> >>>> ./scripts/config --disable CONFIG_ARCH_R9A07G043 >>>> >>>> (to remove CONFIG_DMA_GLOBAL_POOL)... another 2x in buffered reads.. >>>> >>>> [ 4.059242] mmc0: SDHCI controller on ffe7080000.mmc >>>> [ffe7080000.mmc] using ADMA 64-bit >>>> >>>> debian@BeagleV:~$ sudo hdparm -tT /dev/mmcblk0 >>>> >>>> /dev/mmcblk0: >>>> Timing cached reads: 1600 MB in 2.00 seconds = 800.93 MB/sec >>>> Timing buffered disk reads: 892 MB in 3.00 seconds = 297.06 MB/sec >>> >>> It seems CONFIG_DMA_GLOBAL_POOL=y causes ADMA buffer alloc to fail [1]: >>> >>> mmc0: Unable to allocate ADMA buffers - falling back to standard DMA >>> >>> Prabhakar's AX45MP non-coherent DMA support [2] series introduced the >>> selection of DMA_GLOBAL_POOL for ARCH_R9A07G043 and the riscv defconfig >>> selects ARCH_R9A07G043. >>> >>> Patch 5 in the series [3] states that: >>> >>> With DMA_GLOBAL_POOL enabled all DMA allocations happen from this >>> region and synchronization callbacks are implemented to synchronize >>> when doing DMA transactions. >>> >>> This example of a "shared-dma-pool" node was given: >>> >>> pma_resv0@58000000 { >>> compatible = "shared-dma-pool"; >>> reg = <0x0 0x58000000 0x0 0x08000000>; >>> no-map; >>> linux,dma-default; >>> }; >>> >>> I've copied that to th1520-beaglev-ahead.dts. The address of 0x58000000 >>> has no significance on th1520, but the existence of shared-dma-pool >>> seems to fix the problem. ADMA mode [4] is now working even though >>> CONFIG_DMA_GLOBAL_POOL=y. >> >> + Christoph, Lad >> >> IMHO, this is not TH1520 specific but a generic issue. >> >> I believe commit 484861e09f3e ("soc: renesas: Kconfig: Select the >> required configs for RZ/Five SoC") can cause regression on all >> non-dma-coherent riscv platforms with generic defconfig. This is >> a common issue. The logic here is: generic riscv defconfig selects >> ARCH_R9A07G043 which selects DMA_GLOBAL_POOL, which assumes all >> non-dma-coherent riscv platforms have a dma global pool, this assumption >> seems not correct. And I believe DMA_GLOBAL_POOL should not be >> selected by ARCH_SOCFAMILIY, instead, only ARCH under some specific >> conditions can select it globaly, for example NOMMU ARM and so on. >> >> Since this is a regression, what's proper fix? any suggestion is >> appreciated. I think the answer is to not select DMA_GLOBAL_POOL, since that is only designed for nommu cases where non-cacheable memory lives in a fixed place in the physical address map, and regular kernel pages can't be remapped. As far as I'm aware, RISCV_DMA_NONCOHERENT is the thing you want, such that DMA_DIRECT_REMAP can dynamically provide non-cacheable coherent buffers for non-hardware-coherent devices. Thanks, Robin. >> >> Thanks >> >>> >>> Thanks, >>> Drew >>> >>> [1] https://gist.github.com/pdp7/73041ed808bbc7dd445836fb90574979 >>> [2] https://lore.kernel.org/linux-riscv/20230818135723.80612-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ >>> [3] https://lore.kernel.org/linux-riscv/20230818135723.80612-6-prabhakar.mahadev-lad.rj@bp.renesas.com/ >>> [4] https://gist.github.com/pdp7/91e72a663d3bb73eb28182337ad8bbcb
在 2023-10-04星期三的 14:49 +0100，Robin Murphy写道： > On 04/10/2023 2:02 pm, Lad, Prabhakar wrote: > > + CC linux-mm and Robin Murphy > > > > On Wed, Oct 4, 2023 at 12:42 PM Jisheng Zhang <jszhang@kernel.org> > > wrote: > > > > > > On Mon, Oct 02, 2023 at 09:37:44PM -0700, Drew Fustini wrote: > > > > On Fri, Sep 22, 2023 at 05:48:21PM -0500, Robert Nelson wrote: > > > > > On Fri, Sep 22, 2023 at 2:08 PM Robert Nelson > > > > > <robertcnelson@gmail.com> wrote: > > > > > > > > > > > > On Thu, Sep 21, 2023 at 8:51 PM Drew Fustini > > > > > > <dfustini@baylibre.com> wrote: > > > > > > > > > > > > > > This series adds support for the eMMC on the BeagleV > > > > > > > Ahead and the > > > > > > > Sipeed LicheePi 4A. This allows the kernel to boot with > > > > > > > the rootfs on > > > > > > > eMMC. > > > > > > > > > > > > > > I tested on top of v6.6-rc2 with this config [1]. I was > > > > > > > able to boot > > > > > > > both the Ahead [2] and LPi4a [3] from eMMC. The following > > > > > > > prerequisites > > > > > > > are required: > > > > > > > > > > > > > > [PATCH v2] riscv: dts: thead: set dma-noncoherent to > > > > > > > soc bus [4] > > > > > > > > > > > > > > I pushed a branch [5] with this patch series and the > > > > > > > above patch for > > > > > > > those that find a git branch easier to test. > > > > > > > > > > > > > > Please note that only the MMC controller connected to the > > > > > > > eMMC device > > > > > > > is enabled in the device trees for these two boards. I > > > > > > > did not yet > > > > > > > attempt to configure and use the microSD card slot. My > > > > > > > preference is to > > > > > > > address that in a future patch series. > > > > > > > > > > > > > > References: > > > > > > > [1] > > > > > > > https://gist.github.com/pdp7/5fbdcf2a65eb1abdd3a29d519c19cdd2 > > > > > > > [2] > > > > > > > https://gist.github.com/pdp7/91a801a5f8d1070c53509eda9800ad78 > > > > > > > [3] > > > > > > > https://gist.github.com/pdp7/1445c3c991e88fd69c60165cef65726a > > > > > > > [4] > > > > > > > https://lore.kernel.org/linux-riscv/20230912072232.2455-1-jszhang@kernel.org/ > > > > > > > [5] https://github.com/pdp7/linux/tree/b4/th1520-mmc > > > > > > > > > > > > This patchset came out very nice! > > > > > > > > > > > > v6.6-rc2 with Last RFC v2: > > > > > > > > > > > > [ 4.066630] mmc0: SDHCI controller on ffe7080000.mmc > > > > > > [ffe7080000.mmc] using PIO > > > > > > > > > > > > debian@BeagleV:~$ sudo hdparm -tT /dev/mmcblk0 > > > > > > > > > > > > /dev/mmcblk0: > > > > > > Timing cached reads: 1516 MB in 2.00 seconds = 758.09 > > > > > > MB/sec > > > > > > Timing buffered disk reads: 84 MB in 3.01 seconds = > > > > > > 27.94 MB/sec > > > > > > > > > > > > vs v6.6-rc2 with this patchset: > > > > > > > > > > > > [ 4.096837] mmc0: SDHCI controller on ffe7080000.mmc > > > > > > [ffe7080000.mmc] using DMA > > > > > > > > > > > > debian@BeagleV:~$ sudo hdparm -tT /dev/mmcblk0 > > > > > > > > > > > > /dev/mmcblk0: > > > > > > Timing cached reads: 1580 MB in 2.00 seconds = 790.97 > > > > > > MB/sec > > > > > > Timing buffered disk reads: 418 MB in 3.00 seconds = > > > > > > 139.11 MB/sec > > > > > > > > > > Drew pointed out on Slack, this was not quite right.. After > > > > > more > > > > > digging by Drew, CONFIG_DMA_GLOBAL_POOL is causing a DMA > > > > > limitation > > > > > with the multiplatform defconfig. so with, > > > > > > > > > > ./scripts/config --disable CONFIG_ARCH_R9A07G043 > > > > > > > > > > (to remove CONFIG_DMA_GLOBAL_POOL)... another 2x in buffered > > > > > reads.. > > > > > > > > > > [ 4.059242] mmc0: SDHCI controller on ffe7080000.mmc > > > > > [ffe7080000.mmc] using ADMA 64-bit > > > > > > > > > > debian@BeagleV:~$ sudo hdparm -tT /dev/mmcblk0 > > > > > > > > > > /dev/mmcblk0: > > > > > Timing cached reads: 1600 MB in 2.00 seconds = 800.93 > > > > > MB/sec > > > > > Timing buffered disk reads: 892 MB in 3.00 seconds = > > > > > 297.06 MB/sec > > > > > > > > It seems CONFIG_DMA_GLOBAL_POOL=y causes ADMA buffer alloc to > > > > fail [1]: > > > > > > > > mmc0: Unable to allocate ADMA buffers - falling back to > > > > standard DMA > > > > > > > > Prabhakar's AX45MP non-coherent DMA support [2] series > > > > introduced the > > > > selection of DMA_GLOBAL_POOL for ARCH_R9A07G043 and the riscv > > > > defconfig > > > > selects ARCH_R9A07G043. > > > > > > > > Patch 5 in the series [3] states that: > > > > > > > > With DMA_GLOBAL_POOL enabled all DMA allocations happen from > > > > this > > > > region and synchronization callbacks are implemented to > > > > synchronize > > > > when doing DMA transactions. > > > > > > > > This example of a "shared-dma-pool" node was given: > > > > > > > > pma_resv0@58000000 { > > > > compatible = "shared-dma-pool"; > > > > reg = <0x0 0x58000000 0x0 0x08000000>; > > > > no-map; > > > > linux,dma-default; > > > > }; > > > > > > > > I've copied that to th1520-beaglev-ahead.dts. The address of > > > > 0x58000000 > > > > has no significance on th1520, but the existence of shared-dma- > > > > pool > > > > seems to fix the problem. ADMA mode [4] is now working even > > > > though > > > > CONFIG_DMA_GLOBAL_POOL=y. > > > > > > + Christoph, Lad > > > > > > IMHO, this is not TH1520 specific but a generic issue. > > > > > > I believe commit 484861e09f3e ("soc: renesas: Kconfig: Select the > > > required configs for RZ/Five SoC") can cause regression on all > > > non-dma-coherent riscv platforms with generic defconfig. This is > > > a common issue. The logic here is: generic riscv defconfig > > > selects > > > ARCH_R9A07G043 which selects DMA_GLOBAL_POOL, which assumes all > > > non-dma-coherent riscv platforms have a dma global pool, this > > > assumption > > > seems not correct. And I believe DMA_GLOBAL_POOL should not be > > > selected by ARCH_SOCFAMILIY, instead, only ARCH under some > > > specific > > > conditions can select it globaly, for example NOMMU ARM and so > > > on. > > > > > > Since this is a regression, what's proper fix? any suggestion is > > > appreciated. > > I think the answer is to not select DMA_GLOBAL_POOL, since that is > only Well I think for RISC-V, it's not NOMMU only but applicable for every core that does not support Svpbmt or vendor-specific alternatives, because the original RISC-V priv spec does not define memory attributes in page table entries. For the Renesas/Andes case I think a pool is set by OpenSBI with vendor-specific M-mode facility and then passed in DT, and the S-mode (which MMU is enabled in) just sees fixed memory attributes, in this case I think DMA_GLOBAL_POOL is needed. > designed for nommu cases where non-cacheable memory lives in a fixed > place in the physical address map, and regular kernel pages can't be > remapped. As far as I'm aware, RISCV_DMA_NONCOHERENT is the thing you > want, such that DMA_DIRECT_REMAP can dynamically provide non- > cacheable > coherent buffers for non-hardware-coherent devices. > > Thanks, > Robin. > > > > > > > Thanks > > > > > > > > > > > Thanks, > > > > Drew > > > > > > > > [1] > > > > https://gist.github.com/pdp7/73041ed808bbc7dd445836fb90574979 > > > > [2] > > > > https://lore.kernel.org/linux-riscv/20230818135723.80612-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ > > > > [3] > > > > https://lore.kernel.org/linux-riscv/20230818135723.80612-6-prabhakar.mahadev-lad.rj@bp.renesas.com/ > > > > [4] > > > > https://gist.github.com/pdp7/91e72a663d3bb73eb28182337ad8bbcb
On Wed, Oct 04, 2023 at 02:49:56PM +0100, Robin Murphy wrote: > On 04/10/2023 2:02 pm, Lad, Prabhakar wrote: > > + CC linux-mm and Robin Murphy > > > > On Wed, Oct 4, 2023 at 12:42 PM Jisheng Zhang <jszhang@kernel.org> wrote: > > > > > > On Mon, Oct 02, 2023 at 09:37:44PM -0700, Drew Fustini wrote: > > > > On Fri, Sep 22, 2023 at 05:48:21PM -0500, Robert Nelson wrote: > > > > > On Fri, Sep 22, 2023 at 2:08 PM Robert Nelson <robertcnelson@gmail.com> wrote: > > > > > > > > > > > > On Thu, Sep 21, 2023 at 8:51 PM Drew Fustini <dfustini@baylibre.com> wrote: > > > > > > > > > > > > > > This series adds support for the eMMC on the BeagleV Ahead and the > > > > > > > Sipeed LicheePi 4A. This allows the kernel to boot with the rootfs on > > > > > > > eMMC. > > > > > > > > > > > > > > I tested on top of v6.6-rc2 with this config [1]. I was able to boot > > > > > > > both the Ahead [2] and LPi4a [3] from eMMC. The following prerequisites > > > > > > > are required: > > > > > > > > > > > > > > [PATCH v2] riscv: dts: thead: set dma-noncoherent to soc bus [4] > > > > > > > > > > > > > > I pushed a branch [5] with this patch series and the above patch for > > > > > > > those that find a git branch easier to test. > > > > > > > > > > > > > > Please note that only the MMC controller connected to the eMMC device > > > > > > > is enabled in the device trees for these two boards. I did not yet > > > > > > > attempt to configure and use the microSD card slot. My preference is to > > > > > > > address that in a future patch series. > > > > > > > > > > > > > > References: > > > > > > > [1] https://gist.github.com/pdp7/5fbdcf2a65eb1abdd3a29d519c19cdd2 > > > > > > > [2] https://gist.github.com/pdp7/91a801a5f8d1070c53509eda9800ad78 > > > > > > > [3] https://gist.github.com/pdp7/1445c3c991e88fd69c60165cef65726a > > > > > > > [4] https://lore.kernel.org/linux-riscv/20230912072232.2455-1-jszhang@kernel.org/ > > > > > > > [5] https://github.com/pdp7/linux/tree/b4/th1520-mmc > > > > > > > > > > > > This patchset came out very nice! > > > > > > > > > > > > v6.6-rc2 with Last RFC v2: > > > > > > > > > > > > [ 4.066630] mmc0: SDHCI controller on ffe7080000.mmc > > > > > > [ffe7080000.mmc] using PIO > > > > > > > > > > > > debian@BeagleV:~$ sudo hdparm -tT /dev/mmcblk0 > > > > > > > > > > > > /dev/mmcblk0: > > > > > > Timing cached reads: 1516 MB in 2.00 seconds = 758.09 MB/sec > > > > > > Timing buffered disk reads: 84 MB in 3.01 seconds = 27.94 MB/sec > > > > > > > > > > > > vs v6.6-rc2 with this patchset: > > > > > > > > > > > > [ 4.096837] mmc0: SDHCI controller on ffe7080000.mmc > > > > > > [ffe7080000.mmc] using DMA > > > > > > > > > > > > debian@BeagleV:~$ sudo hdparm -tT /dev/mmcblk0 > > > > > > > > > > > > /dev/mmcblk0: > > > > > > Timing cached reads: 1580 MB in 2.00 seconds = 790.97 MB/sec > > > > > > Timing buffered disk reads: 418 MB in 3.00 seconds = 139.11 MB/sec > > > > > > > > > > Drew pointed out on Slack, this was not quite right.. After more > > > > > digging by Drew, CONFIG_DMA_GLOBAL_POOL is causing a DMA limitation > > > > > with the multiplatform defconfig. so with, > > > > > > > > > > ./scripts/config --disable CONFIG_ARCH_R9A07G043 > > > > > > > > > > (to remove CONFIG_DMA_GLOBAL_POOL)... another 2x in buffered reads.. > > > > > > > > > > [ 4.059242] mmc0: SDHCI controller on ffe7080000.mmc > > > > > [ffe7080000.mmc] using ADMA 64-bit > > > > > > > > > > debian@BeagleV:~$ sudo hdparm -tT /dev/mmcblk0 > > > > > > > > > > /dev/mmcblk0: > > > > > Timing cached reads: 1600 MB in 2.00 seconds = 800.93 MB/sec > > > > > Timing buffered disk reads: 892 MB in 3.00 seconds = 297.06 MB/sec > > > > > > > > It seems CONFIG_DMA_GLOBAL_POOL=y causes ADMA buffer alloc to fail [1]: > > > > > > > > mmc0: Unable to allocate ADMA buffers - falling back to standard DMA > > > > > > > > Prabhakar's AX45MP non-coherent DMA support [2] series introduced the > > > > selection of DMA_GLOBAL_POOL for ARCH_R9A07G043 and the riscv defconfig > > > > selects ARCH_R9A07G043. > > > > > > > > Patch 5 in the series [3] states that: > > > > > > > > With DMA_GLOBAL_POOL enabled all DMA allocations happen from this > > > > region and synchronization callbacks are implemented to synchronize > > > > when doing DMA transactions. > > > > > > > > This example of a "shared-dma-pool" node was given: > > > > > > > > pma_resv0@58000000 { > > > > compatible = "shared-dma-pool"; > > > > reg = <0x0 0x58000000 0x0 0x08000000>; > > > > no-map; > > > > linux,dma-default; > > > > }; > > > > > > > > I've copied that to th1520-beaglev-ahead.dts. The address of 0x58000000 > > > > has no significance on th1520, but the existence of shared-dma-pool > > > > seems to fix the problem. ADMA mode [4] is now working even though > > > > CONFIG_DMA_GLOBAL_POOL=y. > > > > > > + Christoph, Lad > > > > > > IMHO, this is not TH1520 specific but a generic issue. > > > > > > I believe commit 484861e09f3e ("soc: renesas: Kconfig: Select the > > > required configs for RZ/Five SoC") can cause regression on all > > > non-dma-coherent riscv platforms with generic defconfig. This is > > > a common issue. The logic here is: generic riscv defconfig selects > > > ARCH_R9A07G043 which selects DMA_GLOBAL_POOL, which assumes all > > > non-dma-coherent riscv platforms have a dma global pool, this assumption > > > seems not correct. And I believe DMA_GLOBAL_POOL should not be > > > selected by ARCH_SOCFAMILIY, instead, only ARCH under some specific > > > conditions can select it globaly, for example NOMMU ARM and so on. > > > > > > Since this is a regression, what's proper fix? any suggestion is > > > appreciated. > > I think the answer is to not select DMA_GLOBAL_POOL, since that is only > designed for nommu cases where non-cacheable memory lives in a fixed place > in the physical address map, and regular kernel pages can't be remapped. As > far as I'm aware, RISCV_DMA_NONCOHERENT is the thing you want, such that > DMA_DIRECT_REMAP can dynamically provide non-cacheable coherent buffers for > non-hardware-coherent devices. Thank Robin! AFAIK, ARCH_R9A07G043 needs the dma global pool to handle its CMO. So it looks like ARCH_R9A07G043 can't be enabled in riscv generic defconfig. And we also need a special solution to prevent random config from selecting ARCH_R9A07G043 by chance for other platforms Thanks > > Thanks, > Robin. > > > > > > > Thanks > > > > > > > > > > > Thanks, > > > > Drew > > > > > > > > [1] https://gist.github.com/pdp7/73041ed808bbc7dd445836fb90574979 > > > > [2] https://lore.kernel.org/linux-riscv/20230818135723.80612-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ > > > > [3] https://lore.kernel.org/linux-riscv/20230818135723.80612-6-prabhakar.mahadev-lad.rj@bp.renesas.com/ > > > > [4] https://gist.github.com/pdp7/91e72a663d3bb73eb28182337ad8bbcb
On Wed, Oct 4, 2023 at 3:18 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 04/10/2023 3:02 pm, Icenowy Zheng wrote: > [...] > >>>> I believe commit 484861e09f3e ("soc: renesas: Kconfig: Select the > >>>> required configs for RZ/Five SoC") can cause regression on all > >>>> non-dma-coherent riscv platforms with generic defconfig. This is > >>>> a common issue. The logic here is: generic riscv defconfig > >>>> selects > >>>> ARCH_R9A07G043 which selects DMA_GLOBAL_POOL, which assumes all > >>>> non-dma-coherent riscv platforms have a dma global pool, this > >>>> assumption > >>>> seems not correct. And I believe DMA_GLOBAL_POOL should not be > >>>> selected by ARCH_SOCFAMILIY, instead, only ARCH under some > >>>> specific > >>>> conditions can select it globaly, for example NOMMU ARM and so > >>>> on. > >>>> > >>>> Since this is a regression, what's proper fix? any suggestion is > >>>> appreciated. > >> > >> I think the answer is to not select DMA_GLOBAL_POOL, since that is > >> only > > > > Well I think for RISC-V, it's not NOMMU only but applicable for every > > core that does not support Svpbmt or vendor-specific alternatives, > > because the original RISC-V priv spec does not define memory attributes > > in page table entries. > > > > For the Renesas/Andes case I think a pool is set by OpenSBI with > > vendor-specific M-mode facility and then passed in DT, and the S-mode > > (which MMU is enabled in) just sees fixed memory attributes, in this > > case I think DMA_GLOBAL_POOL is needed. > > Oh wow, is that really a thing? In that case, either you just can't > support this platform in a multi-platform kernel, or someone needs to do > some fiddly work in dma-direct to a) introduce the notion of an optional > global pool, Looking at the code [0] we do have compile time check for CONFIG_DMA_GLOBAL_POOL irrespective of this being present in DT or not, instead if we make it compile time and runtime check ie either check for DT node or see if pool is available and only then proceed for allocation form this pool. What are your thoughts on this? [0] https://elixir.bootlin.com/linux/v6.6-rc4/source/kernel/dma/direct.c#L238 > and b) make it somehow cope with DMA_DIRECT_REMAP being > enabled but non-functional. > DMA_DIRECT_REMAP config option is selected by NONCOHERENET config option anyway. Cheers, Prabhakar
On Wed, Oct 4, 2023 at 5:03 PM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > > On Wed, Oct 4, 2023 at 3:18 PM Robin Murphy <robin.murphy@arm.com> wrote: > > > > On 04/10/2023 3:02 pm, Icenowy Zheng wrote: > > [...] > > >>>> I believe commit 484861e09f3e ("soc: renesas: Kconfig: Select the > > >>>> required configs for RZ/Five SoC") can cause regression on all > > >>>> non-dma-coherent riscv platforms with generic defconfig. This is > > >>>> a common issue. The logic here is: generic riscv defconfig > > >>>> selects > > >>>> ARCH_R9A07G043 which selects DMA_GLOBAL_POOL, which assumes all > > >>>> non-dma-coherent riscv platforms have a dma global pool, this > > >>>> assumption > > >>>> seems not correct. And I believe DMA_GLOBAL_POOL should not be > > >>>> selected by ARCH_SOCFAMILIY, instead, only ARCH under some > > >>>> specific > > >>>> conditions can select it globaly, for example NOMMU ARM and so > > >>>> on. > > >>>> > > >>>> Since this is a regression, what's proper fix? any suggestion is > > >>>> appreciated. > > >> > > >> I think the answer is to not select DMA_GLOBAL_POOL, since that is > > >> only > > > > > > Well I think for RISC-V, it's not NOMMU only but applicable for every > > > core that does not support Svpbmt or vendor-specific alternatives, > > > because the original RISC-V priv spec does not define memory attributes > > > in page table entries. > > > > > > For the Renesas/Andes case I think a pool is set by OpenSBI with > > > vendor-specific M-mode facility and then passed in DT, and the S-mode > > > (which MMU is enabled in) just sees fixed memory attributes, in this > > > case I think DMA_GLOBAL_POOL is needed. > > > > Oh wow, is that really a thing? In that case, either you just can't > > support this platform in a multi-platform kernel, or someone needs to do > > some fiddly work in dma-direct to a) introduce the notion of an optional > > global pool, > Looking at the code [0] we do have compile time check for > CONFIG_DMA_GLOBAL_POOL irrespective of this being present in DT or > not, instead if we make it compile time and runtime check ie either > check for DT node or see if pool is available and only then proceed > for allocation form this pool. > > What are your thoughts on this? > Something like the below: diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h index f2fc203fb8a1..7bf41a4634a4 100644 --- a/include/linux/dma-map-ops.h +++ b/include/linux/dma-map-ops.h @@ -198,6 +198,7 @@ int dma_release_from_global_coherent(int order, void *vaddr); int dma_mmap_from_global_coherent(struct vm_area_struct *vma, void *cpu_addr, size_t size, int *ret); int dma_init_global_coherent(phys_addr_t phys_addr, size_t size); +bool dma_global_pool_available(void); #else static inline void *dma_alloc_from_global_coherent(struct device *dev, ssize_t size, dma_addr_t *dma_handle) @@ -213,6 +214,10 @@ static inline int dma_mmap_from_global_coherent(struct vm_area_struct *vma, { return 0; } +static inline bool dma_global_pool_available(void) +{ + return false; +} #endif /* CONFIG_DMA_GLOBAL_POOL */ /* diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c index c21abc77c53e..605f243b8262 100644 --- a/kernel/dma/coherent.c +++ b/kernel/dma/coherent.c @@ -277,6 +277,14 @@ int dma_mmap_from_dev_coherent(struct device *dev, struct vm_area_struct *vma, #ifdef CONFIG_DMA_GLOBAL_POOL static struct dma_coherent_mem *dma_coherent_default_memory __ro_after_init; +bool dma_global_pool_available(void) +{ + if (!dma_coherent_default_memory) + return false; + + return true; +} + void *dma_alloc_from_global_coherent(struct device *dev, ssize_t size, dma_addr_t *dma_handle) { diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index 9596ae1aa0da..a599bb731ceb 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -235,7 +235,7 @@ void *dma_direct_alloc(struct device *dev, size_t size, * If there is a global pool, always allocate from it for * non-coherent devices. */ - if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL)) + if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL) && dma_global_pool_available()) return dma_alloc_from_global_coherent(dev, size, dma_handle); Cheers, Prabhakar > [0] https://elixir.bootlin.com/linux/v6.6-rc4/source/kernel/dma/direct.c#L238 > > > and b) make it somehow cope with DMA_DIRECT_REMAP being > > enabled but non-functional. > > > DMA_DIRECT_REMAP config option is selected by NONCOHERENET config option anyway. > > Cheers, > Prabhakar
On 2023-10-04 12:16 PM, Lad, Prabhakar wrote: > On Wed, Oct 4, 2023 at 5:03 PM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: >> >> On Wed, Oct 4, 2023 at 3:18 PM Robin Murphy <robin.murphy@arm.com> wrote: >>> >>> On 04/10/2023 3:02 pm, Icenowy Zheng wrote: >>> [...] >>>>>>> I believe commit 484861e09f3e ("soc: renesas: Kconfig: Select the >>>>>>> required configs for RZ/Five SoC") can cause regression on all >>>>>>> non-dma-coherent riscv platforms with generic defconfig. This is >>>>>>> a common issue. The logic here is: generic riscv defconfig >>>>>>> selects >>>>>>> ARCH_R9A07G043 which selects DMA_GLOBAL_POOL, which assumes all >>>>>>> non-dma-coherent riscv platforms have a dma global pool, this >>>>>>> assumption >>>>>>> seems not correct. And I believe DMA_GLOBAL_POOL should not be >>>>>>> selected by ARCH_SOCFAMILIY, instead, only ARCH under some >>>>>>> specific >>>>>>> conditions can select it globaly, for example NOMMU ARM and so >>>>>>> on. >>>>>>> >>>>>>> Since this is a regression, what's proper fix? any suggestion is >>>>>>> appreciated. >>>>> >>>>> I think the answer is to not select DMA_GLOBAL_POOL, since that is >>>>> only >>>> >>>> Well I think for RISC-V, it's not NOMMU only but applicable for every >>>> core that does not support Svpbmt or vendor-specific alternatives, >>>> because the original RISC-V priv spec does not define memory attributes >>>> in page table entries. >>>> >>>> For the Renesas/Andes case I think a pool is set by OpenSBI with >>>> vendor-specific M-mode facility and then passed in DT, and the S-mode >>>> (which MMU is enabled in) just sees fixed memory attributes, in this >>>> case I think DMA_GLOBAL_POOL is needed. >>> >>> Oh wow, is that really a thing? In that case, either you just can't >>> support this platform in a multi-platform kernel, or someone needs to do >>> some fiddly work in dma-direct to a) introduce the notion of an optional >>> global pool, >> Looking at the code [0] we do have compile time check for >> CONFIG_DMA_GLOBAL_POOL irrespective of this being present in DT or >> not, instead if we make it compile time and runtime check ie either >> check for DT node or see if pool is available and only then proceed >> for allocation form this pool. >> >> What are your thoughts on this? >> > Something like the below: > > diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h > index f2fc203fb8a1..7bf41a4634a4 100644 > --- a/include/linux/dma-map-ops.h > +++ b/include/linux/dma-map-ops.h > @@ -198,6 +198,7 @@ int dma_release_from_global_coherent(int order, > void *vaddr); > int dma_mmap_from_global_coherent(struct vm_area_struct *vma, void *cpu_addr, > size_t size, int *ret); > int dma_init_global_coherent(phys_addr_t phys_addr, size_t size); > +bool dma_global_pool_available(void); > #else > static inline void *dma_alloc_from_global_coherent(struct device *dev, > ssize_t size, dma_addr_t *dma_handle) > @@ -213,6 +214,10 @@ static inline int > dma_mmap_from_global_coherent(struct vm_area_struct *vma, > { > return 0; > } > +static inline bool dma_global_pool_available(void) > +{ > + return false; > +} > #endif /* CONFIG_DMA_GLOBAL_POOL */ > > /* > diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c > index c21abc77c53e..605f243b8262 100644 > --- a/kernel/dma/coherent.c > +++ b/kernel/dma/coherent.c > @@ -277,6 +277,14 @@ int dma_mmap_from_dev_coherent(struct device > *dev, struct vm_area_struct *vma, > #ifdef CONFIG_DMA_GLOBAL_POOL > static struct dma_coherent_mem *dma_coherent_default_memory __ro_after_init; > > +bool dma_global_pool_available(void) > +{ > + if (!dma_coherent_default_memory) > + return false; > + > + return true; > +} > + > void *dma_alloc_from_global_coherent(struct device *dev, ssize_t size, > dma_addr_t *dma_handle) > { > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c > index 9596ae1aa0da..a599bb731ceb 100644 > --- a/kernel/dma/direct.c > +++ b/kernel/dma/direct.c > @@ -235,7 +235,7 @@ void *dma_direct_alloc(struct device *dev, size_t size, > * If there is a global pool, always allocate from it for > * non-coherent devices. > */ > - if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL)) > + if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL) && > dma_global_pool_available()) > return dma_alloc_from_global_coherent(dev, size, > dma_handle); dma_alloc_from_global_coherent() already checks dma_coherent_default_memory, so the solution could be even simpler: --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -232,12 +232,12 @@ void *dma_direct_alloc(struct device *dev, size_t size, attrs); /* - * If there is a global pool, always allocate from it for + * If there is a global pool, always try to allocate from it for * non-coherent devices. */ - if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL)) - return dma_alloc_from_global_coherent(dev, size, - dma_handle); + ret = dma_alloc_from_global_coherent(dev, size, dma_handle); + if (ret) + return ret; /* * Otherwise remap if the architecture is asking for it. But Regards, Samuel
On 2023-10-04 19:49, Samuel Holland wrote: > On 2023-10-04 12:16 PM, Lad, Prabhakar wrote: >> On Wed, Oct 4, 2023 at 5:03 PM Lad, Prabhakar >> <prabhakar.csengg@gmail.com> wrote: >>> >>> On Wed, Oct 4, 2023 at 3:18 PM Robin Murphy <robin.murphy@arm.com> wrote: >>>> >>>> On 04/10/2023 3:02 pm, Icenowy Zheng wrote: >>>> [...] >>>>>>>> I believe commit 484861e09f3e ("soc: renesas: Kconfig: Select the >>>>>>>> required configs for RZ/Five SoC") can cause regression on all >>>>>>>> non-dma-coherent riscv platforms with generic defconfig. This is >>>>>>>> a common issue. The logic here is: generic riscv defconfig >>>>>>>> selects >>>>>>>> ARCH_R9A07G043 which selects DMA_GLOBAL_POOL, which assumes all >>>>>>>> non-dma-coherent riscv platforms have a dma global pool, this >>>>>>>> assumption >>>>>>>> seems not correct. And I believe DMA_GLOBAL_POOL should not be >>>>>>>> selected by ARCH_SOCFAMILIY, instead, only ARCH under some >>>>>>>> specific >>>>>>>> conditions can select it globaly, for example NOMMU ARM and so >>>>>>>> on. >>>>>>>> >>>>>>>> Since this is a regression, what's proper fix? any suggestion is >>>>>>>> appreciated. >>>>>> >>>>>> I think the answer is to not select DMA_GLOBAL_POOL, since that is >>>>>> only >>>>> >>>>> Well I think for RISC-V, it's not NOMMU only but applicable for every >>>>> core that does not support Svpbmt or vendor-specific alternatives, >>>>> because the original RISC-V priv spec does not define memory attributes >>>>> in page table entries. >>>>> >>>>> For the Renesas/Andes case I think a pool is set by OpenSBI with >>>>> vendor-specific M-mode facility and then passed in DT, and the S-mode >>>>> (which MMU is enabled in) just sees fixed memory attributes, in this >>>>> case I think DMA_GLOBAL_POOL is needed. >>>> >>>> Oh wow, is that really a thing? In that case, either you just can't >>>> support this platform in a multi-platform kernel, or someone needs to do >>>> some fiddly work in dma-direct to a) introduce the notion of an optional >>>> global pool, >>> Looking at the code [0] we do have compile time check for >>> CONFIG_DMA_GLOBAL_POOL irrespective of this being present in DT or >>> not, instead if we make it compile time and runtime check ie either >>> check for DT node or see if pool is available and only then proceed >>> for allocation form this pool. >>> >>> What are your thoughts on this? >>> >> Something like the below: >> >> diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h >> index f2fc203fb8a1..7bf41a4634a4 100644 >> --- a/include/linux/dma-map-ops.h >> +++ b/include/linux/dma-map-ops.h >> @@ -198,6 +198,7 @@ int dma_release_from_global_coherent(int order, >> void *vaddr); >> int dma_mmap_from_global_coherent(struct vm_area_struct *vma, void *cpu_addr, >> size_t size, int *ret); >> int dma_init_global_coherent(phys_addr_t phys_addr, size_t size); >> +bool dma_global_pool_available(void); >> #else >> static inline void *dma_alloc_from_global_coherent(struct device *dev, >> ssize_t size, dma_addr_t *dma_handle) >> @@ -213,6 +214,10 @@ static inline int >> dma_mmap_from_global_coherent(struct vm_area_struct *vma, >> { >> return 0; >> } >> +static inline bool dma_global_pool_available(void) >> +{ >> + return false; >> +} >> #endif /* CONFIG_DMA_GLOBAL_POOL */ >> >> /* >> diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c >> index c21abc77c53e..605f243b8262 100644 >> --- a/kernel/dma/coherent.c >> +++ b/kernel/dma/coherent.c >> @@ -277,6 +277,14 @@ int dma_mmap_from_dev_coherent(struct device >> *dev, struct vm_area_struct *vma, >> #ifdef CONFIG_DMA_GLOBAL_POOL >> static struct dma_coherent_mem *dma_coherent_default_memory __ro_after_init; >> >> +bool dma_global_pool_available(void) >> +{ >> + if (!dma_coherent_default_memory) >> + return false; >> + >> + return true; >> +} >> + >> void *dma_alloc_from_global_coherent(struct device *dev, ssize_t size, >> dma_addr_t *dma_handle) >> { >> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c >> index 9596ae1aa0da..a599bb731ceb 100644 >> --- a/kernel/dma/direct.c >> +++ b/kernel/dma/direct.c >> @@ -235,7 +235,7 @@ void *dma_direct_alloc(struct device *dev, size_t size, >> * If there is a global pool, always allocate from it for >> * non-coherent devices. >> */ >> - if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL)) >> + if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL) && >> dma_global_pool_available()) >> return dma_alloc_from_global_coherent(dev, size, >> dma_handle); > > dma_alloc_from_global_coherent() already checks dma_coherent_default_memory, so > the solution could be even simpler: > > --- a/kernel/dma/direct.c > +++ b/kernel/dma/direct.c > @@ -232,12 +232,12 @@ void *dma_direct_alloc(struct device *dev, size_t size, > attrs); > > /* > - * If there is a global pool, always allocate from it for > + * If there is a global pool, always try to allocate from it for > * non-coherent devices. > */ > - if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL)) > - return dma_alloc_from_global_coherent(dev, size, > - dma_handle); > + ret = dma_alloc_from_global_coherent(dev, size, dma_handle); > + if (ret) > + return ret; So if allocation fails because the pool is full, we should go ahead and remap something that can't work? ;) The dma_global_pool_available() idea sort of works, but I'm still concerned about the case where it *should* have been available but the platform has been misconfigured, so again we fall through to DMA_DIRECT_REMAP "successfully" returning a coherent buffer that isn't, and the user's filesystem gets corrupted. Or at best, they get confused by weird errors from random devices going wrong. That's why I said it would be fiddly - the current state of DMA_GLOBAL_POOL as a binary arch-wide thing is relatively robust and easy to reason about, but attempting to generalise it further is... less so. Thanks, Robin. > > /* > * Otherwise remap if the architecture is asking for it. But > > Regards, > Samuel >
On Wed, Oct 4, 2023 at 8:38 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2023-10-04 19:49, Samuel Holland wrote: > > On 2023-10-04 12:16 PM, Lad, Prabhakar wrote: > >> On Wed, Oct 4, 2023 at 5:03 PM Lad, Prabhakar > >> <prabhakar.csengg@gmail.com> wrote: > >>> > >>> On Wed, Oct 4, 2023 at 3:18 PM Robin Murphy <robin.murphy@arm.com> wrote: > >>>> > >>>> On 04/10/2023 3:02 pm, Icenowy Zheng wrote: > >>>> [...] > >>>>>>>> I believe commit 484861e09f3e ("soc: renesas: Kconfig: Select the > >>>>>>>> required configs for RZ/Five SoC") can cause regression on all > >>>>>>>> non-dma-coherent riscv platforms with generic defconfig. This is > >>>>>>>> a common issue. The logic here is: generic riscv defconfig > >>>>>>>> selects > >>>>>>>> ARCH_R9A07G043 which selects DMA_GLOBAL_POOL, which assumes all > >>>>>>>> non-dma-coherent riscv platforms have a dma global pool, this > >>>>>>>> assumption > >>>>>>>> seems not correct. And I believe DMA_GLOBAL_POOL should not be > >>>>>>>> selected by ARCH_SOCFAMILIY, instead, only ARCH under some > >>>>>>>> specific > >>>>>>>> conditions can select it globaly, for example NOMMU ARM and so > >>>>>>>> on. > >>>>>>>> > >>>>>>>> Since this is a regression, what's proper fix? any suggestion is > >>>>>>>> appreciated. > >>>>>> > >>>>>> I think the answer is to not select DMA_GLOBAL_POOL, since that is > >>>>>> only > >>>>> > >>>>> Well I think for RISC-V, it's not NOMMU only but applicable for every > >>>>> core that does not support Svpbmt or vendor-specific alternatives, > >>>>> because the original RISC-V priv spec does not define memory attributes > >>>>> in page table entries. > >>>>> > >>>>> For the Renesas/Andes case I think a pool is set by OpenSBI with > >>>>> vendor-specific M-mode facility and then passed in DT, and the S-mode > >>>>> (which MMU is enabled in) just sees fixed memory attributes, in this > >>>>> case I think DMA_GLOBAL_POOL is needed. > >>>> > >>>> Oh wow, is that really a thing? In that case, either you just can't > >>>> support this platform in a multi-platform kernel, or someone needs to do > >>>> some fiddly work in dma-direct to a) introduce the notion of an optional > >>>> global pool, > >>> Looking at the code [0] we do have compile time check for > >>> CONFIG_DMA_GLOBAL_POOL irrespective of this being present in DT or > >>> not, instead if we make it compile time and runtime check ie either > >>> check for DT node or see if pool is available and only then proceed > >>> for allocation form this pool. > >>> > >>> What are your thoughts on this? > >>> > >> Something like the below: > >> > >> diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h > >> index f2fc203fb8a1..7bf41a4634a4 100644 > >> --- a/include/linux/dma-map-ops.h > >> +++ b/include/linux/dma-map-ops.h > >> @@ -198,6 +198,7 @@ int dma_release_from_global_coherent(int order, > >> void *vaddr); > >> int dma_mmap_from_global_coherent(struct vm_area_struct *vma, void *cpu_addr, > >> size_t size, int *ret); > >> int dma_init_global_coherent(phys_addr_t phys_addr, size_t size); > >> +bool dma_global_pool_available(void); > >> #else > >> static inline void *dma_alloc_from_global_coherent(struct device *dev, > >> ssize_t size, dma_addr_t *dma_handle) > >> @@ -213,6 +214,10 @@ static inline int > >> dma_mmap_from_global_coherent(struct vm_area_struct *vma, > >> { > >> return 0; > >> } > >> +static inline bool dma_global_pool_available(void) > >> +{ > >> + return false; > >> +} > >> #endif /* CONFIG_DMA_GLOBAL_POOL */ > >> > >> /* > >> diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c > >> index c21abc77c53e..605f243b8262 100644 > >> --- a/kernel/dma/coherent.c > >> +++ b/kernel/dma/coherent.c > >> @@ -277,6 +277,14 @@ int dma_mmap_from_dev_coherent(struct device > >> *dev, struct vm_area_struct *vma, > >> #ifdef CONFIG_DMA_GLOBAL_POOL > >> static struct dma_coherent_mem *dma_coherent_default_memory __ro_after_init; > >> > >> +bool dma_global_pool_available(void) > >> +{ > >> + if (!dma_coherent_default_memory) > >> + return false; > >> + > >> + return true; > >> +} > >> + > >> void *dma_alloc_from_global_coherent(struct device *dev, ssize_t size, > >> dma_addr_t *dma_handle) > >> { > >> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c > >> index 9596ae1aa0da..a599bb731ceb 100644 > >> --- a/kernel/dma/direct.c > >> +++ b/kernel/dma/direct.c > >> @@ -235,7 +235,7 @@ void *dma_direct_alloc(struct device *dev, size_t size, > >> * If there is a global pool, always allocate from it for > >> * non-coherent devices. > >> */ > >> - if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL)) > >> + if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL) && > >> dma_global_pool_available()) > >> return dma_alloc_from_global_coherent(dev, size, > >> dma_handle); > > > > dma_alloc_from_global_coherent() already checks dma_coherent_default_memory, so > > the solution could be even simpler: > > > > --- a/kernel/dma/direct.c > > +++ b/kernel/dma/direct.c > > @@ -232,12 +232,12 @@ void *dma_direct_alloc(struct device *dev, size_t size, > > attrs); > > > > /* > > - * If there is a global pool, always allocate from it for > > + * If there is a global pool, always try to allocate from it for > > * non-coherent devices. > > */ > > - if (IS_ENABLED(CONFIG_DMA_GLOBAL_POOL)) > > - return dma_alloc_from_global_coherent(dev, size, > > - dma_handle); > > + ret = dma_alloc_from_global_coherent(dev, size, dma_handle); > > + if (ret) > > + return ret; > > So if allocation fails because the pool is full, we should go ahead and > remap something that can't work? ;) > > The dma_global_pool_available() idea sort of works, but I'm still > concerned about the case where it *should* have been available but the > platform has been misconfigured, so again we fall through to > If the platform is misconfigured it is bound to fail anyway so should we consider that as a valid case? > DMA_DIRECT_REMAP "successfully" returning a coherent buffer that isn't, > and the user's filesystem gets corrupted. Or at best, they get confused > by weird errors from random devices going wrong. That's why I said it > would be fiddly - the current state of DMA_GLOBAL_POOL as a binary > arch-wide thing is relatively robust and easy to reason about, but > attempting to generalise it further is... less so. > > Thanks, > Robin. > Cheers, Prabhakar
This series adds support for the eMMC on the BeagleV Ahead and the Sipeed LicheePi 4A. This allows the kernel to boot with the rootfs on eMMC. I tested on top of v6.6-rc2 with this config [1]. I was able to boot both the Ahead [2] and LPi4a [3] from eMMC. The following prerequisites are required: [PATCH v2] riscv: dts: thead: set dma-noncoherent to soc bus [4] I pushed a branch [5] with this patch series and the above patch for those that find a git branch easier to test. Please note that only the MMC controller connected to the eMMC device is enabled in the device trees for these two boards. I did not yet attempt to configure and use the microSD card slot. My preference is to address that in a future patch series. References: [1] https://gist.github.com/pdp7/5fbdcf2a65eb1abdd3a29d519c19cdd2 [2] https://gist.github.com/pdp7/91a801a5f8d1070c53509eda9800ad78 [3] https://gist.github.com/pdp7/1445c3c991e88fd69c60165cef65726a [4] https://lore.kernel.org/linux-riscv/20230912072232.2455-1-jszhang@kernel.org/ [5] https://github.com/pdp7/linux/tree/b4/th1520-mmc Changes since RFC v2: - ADMA mode now works correctly due to a patch from Jisheng on the list ("riscv: dts: thead: set dma-noncoherent to soc bus") and this commit from Icenowy that is now merged: 8eb8fe67e2c8 ("riscv: errata: fix T-Head dcache.cva encoding"). - Expose __sdhci_execute_tuning from sdhci.c so that it can be called from th1520_execute_tuning() - Refactor the define macros for all the PHY related registers to make it easier to understand the bit fields that the code is manipulating - Replace magic numbers in the PHY register writes with proper defines - Replace non_removable in dwcmshc_priv with check of mmc_host.caps - Drop dt prop "thead,io-fixed-1v8" and instead check for existing properties: "mmc-ddr-1_8v", "mmc-hs200-1_8v", or "mmc-hs400-1_8v" - Rename dt prop from "thead,pull-up" to "thead,phy-pull-up" and improve the description in the dt binding - Replace pull_up_en in dwcmshc_priv with bit field in new flags field - Create th1520_set_uhs_signaling() and call dwcmshc_set_uhs_signaling() from it instead of adding th1520 code to dwcmshc_set_uhs_signaling() - Return -EIO instead of -1 upon errors in th1520_execute_tuning() Changes in RFC v2: https://lore.kernel.org/linux-riscv/20230724-th1520-emmc-v2-0-132ed2e2171e@baylibre.com/ - Expand dwcmshc_priv based on driver in the T-Head 5.10 kernel: delay_line, non_removable, pull_up_en, io_fixed_1v8 - New boolean property "thead,pull-up" indicates phy pull-up config - New boolean property "thead,io-fixed-1v8" indicates that io voltage should be set to 1.8V during reset - Add th1520_phy_1_8v_init() as voltage_switch op - Add th1520_execute_tuning() as the platform_execute_tuning op - Added th1520_sdhci_reset() as the .reset op. This function will set io voltage to 1.8V after calling the standard sdhci_reset() function. - Modified dwcmshc_set_uhs_signaling() to enable SDHCI_CTRL_VDD_180 when io_fixed_1v8 is true - Add many defines for register offsets and settings based on the mmc support in the T-Head downstream v5.10 kernel RFC v1 series: https://lore.kernel.org/r/20230724-th1520-emmc-v1-0-cca1b2533da2@baylibre.com Signed-off-by: Drew Fustini <dfustini@baylibre.com> --- Drew Fustini (6): dt-bindings: mmc: sdhci-of-dwcmhsc: Add T-Head TH1520 support mmc: sdhci: add __sdhci_execute_tuning() to header mmc: sdhci-of-dwcmshc: Add support for T-Head TH1520 riscv: dts: thead: Add TH1520 mmc controller and sdhci clock riscv: dts: thead: Enable BeagleV Ahead eMMC controller riscv: dts: thead: Enable LicheePi 4A eMMC controller .../bindings/mmc/snps,dwcmshc-sdhci.yaml | 4 + arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts | 15 + .../boot/dts/thead/th1520-lichee-module-4a.dtsi | 15 + arch/riscv/boot/dts/thead/th1520.dtsi | 15 + drivers/mmc/host/sdhci-of-dwcmshc.c | 456 +++++++++++++++++++++ drivers/mmc/host/sdhci.c | 2 +- drivers/mmc/host/sdhci.h | 1 + 7 files changed, 507 insertions(+), 1 deletion(-) --- base-commit: 3d01adbee80b2237c43e2e06d59e05aa243a0fe6 change-id: 20230921-th1520-mmc-518806aa55a8 Best regards,