mbox series

[0/6] RISC-V: Add eMMC support for TH1520 boards

Message ID 20230921-th1520-mmc-v1-0-49f76c274fb3@baylibre.com
Headers show
Series RISC-V: Add eMMC support for TH1520 boards | expand

Message

Drew Fustini Sept. 22, 2023, 1:49 a.m. UTC
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,

Comments

Xi Ruoyao Sept. 22, 2023, 11:41 a.m. UTC | #1
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?

For everything except "Enable BeagleV Ahead eMMC controller":

Tested-by: Xi Ruoyao <xry111@xry111.site>
Drew Fustini Sept. 22, 2023, 4:23 p.m. UTC | #2
On Fri, Sep 22, 2023 at 07:41:37PM +0800, Xi Ruoyao wrote:
> 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?

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 imagine an enforced delay would be very annoying. With "rootwait", I
don't notice any delay, it boots to the login prompt faster than I can
read the text scrolling by during boot. (my rootfs is a simple
buildroot).

thanks,
drew
Robert Nelson Sept. 22, 2023, 7:08 p.m. UTC | #3
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

Regards,
Robert Nelson Sept. 22, 2023, 10:48 p.m. UTC | #4
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

Regards,
Icenowy Zheng Sept. 23, 2023, 12:11 a.m. UTC | #5
在 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>
>
Xi Ruoyao Sept. 23, 2023, 4:33 p.m. UTC | #6
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).
Adrian Hunter Sept. 25, 2023, 10:21 a.m. UTC | #7
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);
>
Xi Ruoyao Sept. 25, 2023, 10:23 a.m. UTC | #8
On Mon, 2023-09-25 at 13:21 +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);
> 
> >  {
> >  	int i;

By the way should we rename this function?  I think
"__sdhci_execute_tuning" vs "sdhci_execute_tuning" might be confusing
when we export both.
Adrian Hunter Sept. 25, 2023, 10:31 a.m. UTC | #9
On 25/09/23 13:23, Xi Ruoyao wrote:
> On Mon, 2023-09-25 at 13:21 +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);
>>
>>>  {
>>>  	int i;
> 
> By the way should we rename this function?  I think
> "__sdhci_execute_tuning" vs "sdhci_execute_tuning" might be confusing
> when we export both.
> 

'name()' and '__name()' is not a particularly rare paradigm in the kernel,
so it seems ok.
Adrian Hunter Sept. 25, 2023, 10:35 a.m. UTC | #10
On 22/09/23 04:49, Drew Fustini wrote:
> Add support for the mmc controller in the T-Head TH1520 with the new
> compatible "thead,th1520-dwcmshc". Implement custom sdhci_ops for
> set_uhs_signaling, reset, voltage_switch, and platform_execute_tuning.
> 
> Signed-off-by: Drew Fustini <dfustini@baylibre.com>
> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 456 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 456 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 3a3bae6948a8..7294bf1afb7d 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -35,6 +35,26 @@
>  #define DWCMSHC_CARD_IS_EMMC		BIT(0)
>  #define DWCMSHC_ENHANCED_STROBE		BIT(8)
>  #define DWCMSHC_EMMC_ATCTRL		0x40
> +/* Tuning and auto-tuning fields in AT_CTRL_R control register */
> +#define AT_CTRL_AT_EN			0x1 /* autotuning is enabled */
> +#define AT_CTRL_CI_SEL_SHIFT		0x1 /* bit 1 */
> +#define AT_CTRL_CI_SEL			0x1 /* interval to drive center phase select */
> +#define AT_CTRL_SWIN_TH_EN_SHIFT	0x2 /* bit 2 */
> +#define AT_CTRL_SWIN_TH_EN		0x1 /* sampling window threshold enable */
> +#define AT_CTRL_RPT_TUNE_ERR_SHIFT	0x3 /* bit 3 */
> +#define AT_CTRL_RPT_TUNE_ERR		0x1 /* enable reporting framing errors */
> +#define AT_CTRL_SW_TUNE_EN_SHIFT	0x4 /* bit 4 */
> +#define AT_CTRL_SW_TUNE_EN		0x1 /* enable software managed tuning */
> +#define AT_CTRL_WIN_EDGE_SEL_SHIFT	0x8 /* bits [11:8] */
> +#define AT_CTRL_WIN_EDGE_SEL		0xf /* sampling window edge select */
> +#define AT_CTRL_TUNE_CLK_STOP_EN_SHIFT	0x10 /* bit 16 */
> +#define AT_CTRL_TUNE_CLK_STOP_EN	0x1  /* clocks stopped during phase code change */
> +#define AT_CTRL_PRE_CHANGE_DLY_SHIFT	0x11 /* bits [18:17] */
> +#define AT_CTRL_PRE_CHANGE_DLY		0x1  /* 2-cycle latency */
> +#define AT_CTRL_POST_CHANGE_DLY_SHIFT	0x13 /* bits [20:19] */
> +#define AT_CTRL_POST_CHANGE_DLY		0x3  /* 4-cycle latency */
> +#define AT_CTRL_SWIN_TH_VAL_SHIFT	0x18 /* bits [31:24] */
> +#define AT_CTRL_SWIN_TH_VAL		0x9  /* sampling window threshold */

Here and elsewhere, please try to make use of BIT(), GENMASK(),
FIELD_PREP(), FIELD_GET()
Drew Fustini Sept. 25, 2023, 2:41 p.m. UTC | #11
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
Drew Fustini Sept. 25, 2023, 2:43 p.m. UTC | #12
On Mon, Sep 25, 2023 at 01:35:36PM +0300, Adrian Hunter wrote:
> On 22/09/23 04:49, Drew Fustini wrote:
> > Add support for the mmc controller in the T-Head TH1520 with the new
> > compatible "thead,th1520-dwcmshc". Implement custom sdhci_ops for
> > set_uhs_signaling, reset, voltage_switch, and platform_execute_tuning.
> > 
> > Signed-off-by: Drew Fustini <dfustini@baylibre.com>
> > ---
> >  drivers/mmc/host/sdhci-of-dwcmshc.c | 456 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 456 insertions(+)
> > 
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > index 3a3bae6948a8..7294bf1afb7d 100644
> > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > @@ -35,6 +35,26 @@
> >  #define DWCMSHC_CARD_IS_EMMC		BIT(0)
> >  #define DWCMSHC_ENHANCED_STROBE		BIT(8)
> >  #define DWCMSHC_EMMC_ATCTRL		0x40
> > +/* Tuning and auto-tuning fields in AT_CTRL_R control register */
> > +#define AT_CTRL_AT_EN			0x1 /* autotuning is enabled */
> > +#define AT_CTRL_CI_SEL_SHIFT		0x1 /* bit 1 */
> > +#define AT_CTRL_CI_SEL			0x1 /* interval to drive center phase select */
> > +#define AT_CTRL_SWIN_TH_EN_SHIFT	0x2 /* bit 2 */
> > +#define AT_CTRL_SWIN_TH_EN		0x1 /* sampling window threshold enable */
> > +#define AT_CTRL_RPT_TUNE_ERR_SHIFT	0x3 /* bit 3 */
> > +#define AT_CTRL_RPT_TUNE_ERR		0x1 /* enable reporting framing errors */
> > +#define AT_CTRL_SW_TUNE_EN_SHIFT	0x4 /* bit 4 */
> > +#define AT_CTRL_SW_TUNE_EN		0x1 /* enable software managed tuning */
> > +#define AT_CTRL_WIN_EDGE_SEL_SHIFT	0x8 /* bits [11:8] */
> > +#define AT_CTRL_WIN_EDGE_SEL		0xf /* sampling window edge select */
> > +#define AT_CTRL_TUNE_CLK_STOP_EN_SHIFT	0x10 /* bit 16 */
> > +#define AT_CTRL_TUNE_CLK_STOP_EN	0x1  /* clocks stopped during phase code change */
> > +#define AT_CTRL_PRE_CHANGE_DLY_SHIFT	0x11 /* bits [18:17] */
> > +#define AT_CTRL_PRE_CHANGE_DLY		0x1  /* 2-cycle latency */
> > +#define AT_CTRL_POST_CHANGE_DLY_SHIFT	0x13 /* bits [20:19] */
> > +#define AT_CTRL_POST_CHANGE_DLY		0x3  /* 4-cycle latency */
> > +#define AT_CTRL_SWIN_TH_VAL_SHIFT	0x18 /* bits [31:24] */
> > +#define AT_CTRL_SWIN_TH_VAL		0x9  /* sampling window threshold */
> 
> Here and elsewhere, please try to make use of BIT(), GENMASK(),
> FIELD_PREP(), FIELD_GET()

Thank you for the advice.  FIELD_PREP() and FIELD_GET() look like they
can make the code simpler.  I'll make use of those macros in the next
version.

thanks,
drew
Drew Fustini Oct. 3, 2023, 4:37 a.m. UTC | #13
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.

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
Jisheng Zhang Oct. 4, 2023, 11:30 a.m. UTC | #14
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
Prabhakar Oct. 4, 2023, 1:02 p.m. UTC | #15
+ 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.
>
> 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
Robin Murphy Oct. 4, 2023, 1:49 p.m. UTC | #16
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
Icenowy Zheng Oct. 4, 2023, 2:02 p.m. UTC | #17
在 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
Jisheng Zhang Oct. 4, 2023, 2:06 p.m. UTC | #18
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
Robin Murphy Oct. 4, 2023, 2:18 p.m. UTC | #19
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, and b) make it somehow cope with DMA_DIRECT_REMAP being 
enabled but non-functional.

Thanks,
Robin.
Icenowy Zheng Oct. 4, 2023, 2:46 p.m. UTC | #20
在 2023-10-04星期三的 15:18 +0100,Robin Murphy写道:
> 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 

Emmmm thus RZ/Five should `depends on NONPORTABLE`?

> some fiddly work in dma-direct to a) introduce the notion of an
> optional 
> global pool, and b) make it somehow cope with DMA_DIRECT_REMAP being 
> enabled but non-functional.
> 
> Thanks,
> Robin.
Icenowy Zheng Oct. 4, 2023, 2:58 p.m. UTC | #21
在 2023-10-04星期三的 15:18 +0100,Robin Murphy写道:
> 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 

Well, considering RZ/Five enables some spec-non-conformant local memory
(which bypasses MMU) that makes even running generic user space
binaries not so viable (PIE ones may still run, but those built to be
on the default fixed location of binutils will conflict with the MMU-
bypassing local memory), not supporting it in a multi-platform kernel
doesn't look like a big deal.

> some fiddly work in dma-direct to a) introduce the notion of an
> optional 
> global pool, and b) make it somehow cope with DMA_DIRECT_REMAP being 
> enabled but non-functional.
> 
> Thanks,
> Robin.
Geert Uytterhoeven Oct. 4, 2023, 3:27 p.m. UTC | #22
Hi Jisheng,

On Wed, Oct 4, 2023 at 4:18 PM Jisheng Zhang <jszhang@kernel.org> wrote:
> 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

There will be a similar issue with e.g. Starlight, as ERRATA_STARFIVE_JH7100
(not yet upstream, but in esmil/visionfive) also selects DMA_GLOBAL_POOL.

Gr{oetje,eeting}s,

                        Geert
Prabhakar Oct. 4, 2023, 4:03 p.m. UTC | #23
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
Jisheng Zhang Oct. 4, 2023, 4:19 p.m. UTC | #24
On Thu, Sep 21, 2023 at 06:49:50PM -0700, Drew Fustini wrote:
> Add support for the mmc controller in the T-Head TH1520 with the new
> compatible "thead,th1520-dwcmshc". Implement custom sdhci_ops for
> set_uhs_signaling, reset, voltage_switch, and platform_execute_tuning.
> 
> Signed-off-by: Drew Fustini <dfustini@baylibre.com>

Hi Drew,
Thanks for doing this and sorry for being late for this code review. Some
comments below.

> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 456 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 456 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 3a3bae6948a8..7294bf1afb7d 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -35,6 +35,26 @@
>  #define DWCMSHC_CARD_IS_EMMC		BIT(0)
>  #define DWCMSHC_ENHANCED_STROBE		BIT(8)
>  #define DWCMSHC_EMMC_ATCTRL		0x40
> +/* Tuning and auto-tuning fields in AT_CTRL_R control register */
> +#define AT_CTRL_AT_EN			0x1 /* autotuning is enabled */
> +#define AT_CTRL_CI_SEL_SHIFT		0x1 /* bit 1 */
> +#define AT_CTRL_CI_SEL			0x1 /* interval to drive center phase select */
> +#define AT_CTRL_SWIN_TH_EN_SHIFT	0x2 /* bit 2 */
> +#define AT_CTRL_SWIN_TH_EN		0x1 /* sampling window threshold enable */
> +#define AT_CTRL_RPT_TUNE_ERR_SHIFT	0x3 /* bit 3 */
> +#define AT_CTRL_RPT_TUNE_ERR		0x1 /* enable reporting framing errors */
> +#define AT_CTRL_SW_TUNE_EN_SHIFT	0x4 /* bit 4 */
> +#define AT_CTRL_SW_TUNE_EN		0x1 /* enable software managed tuning */
> +#define AT_CTRL_WIN_EDGE_SEL_SHIFT	0x8 /* bits [11:8] */
> +#define AT_CTRL_WIN_EDGE_SEL		0xf /* sampling window edge select */
> +#define AT_CTRL_TUNE_CLK_STOP_EN_SHIFT	0x10 /* bit 16 */
> +#define AT_CTRL_TUNE_CLK_STOP_EN	0x1  /* clocks stopped during phase code change */
> +#define AT_CTRL_PRE_CHANGE_DLY_SHIFT	0x11 /* bits [18:17] */
> +#define AT_CTRL_PRE_CHANGE_DLY		0x1  /* 2-cycle latency */
> +#define AT_CTRL_POST_CHANGE_DLY_SHIFT	0x13 /* bits [20:19] */
> +#define AT_CTRL_POST_CHANGE_DLY		0x3  /* 4-cycle latency */
> +#define AT_CTRL_SWIN_TH_VAL_SHIFT	0x18 /* bits [31:24] */
> +#define AT_CTRL_SWIN_TH_VAL		0x9  /* sampling window threshold */
>  
>  /* Rockchip specific Registers */
>  #define DWCMSHC_EMMC_DLL_CTRL		0x800
> @@ -72,6 +92,84 @@
>  	(((x) & DWCMSHC_EMMC_DLL_TIMEOUT) == 0))
>  #define RK35xx_MAX_CLKS 3
>  
> +/* PHY register area pointer */
> +#define DWC_MSHC_PTR_PHY_R	0x300
> +
> +/* PHY general configuration */
> +#define PHY_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x00)
> +#define PHY_CNFG_RSTN_DEASSERT	0x1  /* Deassert PHY reset */
> +#define PHY_CNFG_PAD_SP_SHIFT	0x10 /* bits [16:9] */
> +#define PHY_CNFG_PAD_SP_VALUE	0x0c /* PMOS TX drive strength */
> +#define PHY_CNFG_PAD_SN_SHIFT	0x14 /* bits [23:20] */
> +#define PHY_CNFG_PAD_SN_VALUE	0x0c /* NMOS TX drive strength */
> +
> +/* PHY command/response pad settings */
> +#define PHY_CMDPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x04)
> +
> +/* PHY data pad settings */
> +#define PHY_DATAPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x06)
> +
> +/* PHY clock pad settings */
> +#define PHY_CLKPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x08)
> +
> +/* PHY strobe pad settings */
> +#define PHY_STBPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0a)
> +
> +/* PHY reset pad settings */
> +#define PHY_RSTNPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0c)
> +
> +/* Bitfields are common for all pad settings */
> +#define PHY_PAD_RXSEL_1V8		0x1 /* Receiver type select for 1.8V */
> +#define PHY_PAD_RXSEL_3V3		0x2 /* Receiver type select for 3.3V */
> +
> +#define PHY_PAD_WEAKPULL_SHIFT		0x3 /* bits [4:3] */
> +#define PHY_PAD_WEAKPULL_PULLUP		0x1 /* Weak pull down enabled */
> +#define PHY_PAD_WEAKPULL_PULLDOWN	0x2 /* Weak pull down enabled */
> +
> +#define PHY_PAD_TXSLEW_CTRL_P_SHIFT	0x5 /* bits [8:5] */
> +#define PHY_PAD_TXSLEW_CTRL_P_VALUE	0x3 /* Slew control for P-Type pad TX */
> +#define PHY_PAD_TXSLEW_CTRL_N_SHIFT	0x9 /* bits [12:9] */
> +#define PHY_PAD_TXSLEW_CTRL_N_VALUE	0x3 /* Slew control for N-Type pad TX */
> +
> +/* PHY CLK delay line settings */
> +#define PHY_SDCLKDL_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x1d)
> +#define PHY_SDCLKDL_CNFG_UPDATE_SHIFT	0x4 /* bit 4 */
> +#define PHY_SDCLKDL_CNFG_UPDATE_DC	0x1 /* set before writing to SDCLKDL_DC */
> +
> +/* PHY CLK delay line delay code */
> +#define PHY_SDCLKDL_DC_R		(DWC_MSHC_PTR_PHY_R + 0x1e)
> +#define PHY_SDCLKDL_DC_INITIAL		0x40 /* initial delay code */
> +#define PHY_SDCLKDL_DC_DEFAULT		0x32 /* default delay code */
> +#define PHY_SDCLKDL_DC_HS400		0x18 /* delay code for HS400 mode */
> +
> +/* PHY drift_cclk_rx delay line configuration setting */
> +#define PHY_ATDL_CNFG_R			(DWC_MSHC_PTR_PHY_R + 0x21)
> +#define PHY_ATDL_CNFG_INPSEL_SHIFT	0x2 /* bits [3:2] */
> +#define PHY_ATDL_CNFG_INPSEL_VALUE	0x3 /* delay line input source */
> +
> +/* PHY DLL control settings */
> +#define PHY_DLL_CTRL_R			(DWC_MSHC_PTR_PHY_R + 0x24)
> +#define PHY_DLL_CTRL_DISABLE		0x0 /* PHY DLL is enabled */
> +#define PHY_DLL_CTRL_ENABLE		0x1 /* PHY DLL is disabled */
> +
> +/* PHY DLL  configuration register 1 */
> +#define PHY_DLL_CNFG1_R			(DWC_MSHC_PTR_PHY_R + 0x25)
> +#define PHY_DLL_CNFG1_SLVDLY_SHIFT	0x4 /* bits [5:4] */
> +#define PHY_DLL_CNFG1_SLVDLY_VALUE	0x2 /* DLL slave update delay input */
> +#define PHY_DLL_CNFG1_WAITCYCLE		0x5 /* DLL wait cycle input */
> +
> +/* PHY DLL configuration register 2 */
> +#define PHY_DLL_CNFG2_R			(DWC_MSHC_PTR_PHY_R + 0x26)
> +#define PHY_DLL_CNFG2_JUMPSTEP		0xa /* DLL jump step input */
> +
> +/* PHY DLL master and slave delay line configuration settings */
> +#define PHY_DLLDL_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x28)
> +#define PHY_DLLDL_CNFG_SLV_INPSEL_SHIFT	0x5 /* bits [6:5] */
> +#define PHY_DLLDL_CNFG_SLV_INPSEL_VALUE	0x3 /* clock source select for slave DL */
> +
> +#define FLAG_PULL_UP_EN		BIT(0)
> +#define FLAG_IO_FIXED_1V8	BIT(1)
> +
>  #define BOUNDARY_OK(addr, len) \
>  	((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
>  
> @@ -92,6 +190,8 @@ struct dwcmshc_priv {
>  	struct clk	*bus_clk;
>  	int vendor_specific_area1; /* P_VENDOR_SPECIFIC_AREA reg */
>  	void *priv; /* pointer to SoC private stuff */
> +	u16 delay_line;
> +	u16 flags;
>  };
>  
>  /*
> @@ -157,6 +257,206 @@ static void dwcmshc_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  	sdhci_request(mmc, mrq);
>  }
>  
> +static void th1520_phy_1_8v_init_no_pull(struct sdhci_host *host)
> +{
> +	u32 val;
> +
> +	/* deassert phy reset */
> +	sdhci_writel(host, PHY_CNFG_RSTN_DEASSERT, PHY_CNFG_R);
> +
> +	/* disable delay line */
> +	sdhci_writeb(host, PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT,
> +		     PHY_SDCLKDL_CNFG_R);
> +
> +	/* set delay line */
> +	sdhci_writeb(host, PHY_SDCLKDL_DC_INITIAL, PHY_SDCLKDL_DC_R);
> +
> +	/* enable delay lane */
> +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> +	val &= ~(PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT);
> +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> +
> +	/* configure phy pads */
> +	val = sdhci_readw(host, PHY_CMDPAD_CNFG_R);
> +	sdhci_writew(host, val | PHY_PAD_RXSEL_1V8, PHY_CMDPAD_CNFG_R);
> +
> +	val = sdhci_readw(host, PHY_DATAPAD_CNFG_R);
> +	sdhci_writew(host, val | PHY_PAD_RXSEL_1V8, PHY_DATAPAD_CNFG_R);
> +
> +	val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R);
> +	sdhci_writew(host, val | PHY_PAD_RXSEL_1V8, PHY_RSTNPAD_CNFG_R);
> +
> +	val = sdhci_readw(host, PHY_STBPAD_CNFG_R);
> +	sdhci_writew(host, val | PHY_PAD_RXSEL_1V8, PHY_STBPAD_CNFG_R);
> +
> +	/* enable phy dll */
> +	val = sdhci_readb(host, PHY_DLL_CTRL_R);
> +	sdhci_writeb(host, val | PHY_DLL_CTRL_ENABLE, PHY_DLL_CTRL_R);
> +}
> +
> +static void th1520_phy_3_3v_init_no_pull(struct sdhci_host *host)
> +{
> +	u32 val;
> +
> +	/* deassert phy reset */
> +	sdhci_writel(host, PHY_CNFG_RSTN_DEASSERT, PHY_CNFG_R);
> +
> +	/* disable delay line */
> +	sdhci_writeb(host, PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT,
> +		     PHY_SDCLKDL_CNFG_R);
> +
> +	/* set delay line */
> +	sdhci_writeb(host, PHY_SDCLKDL_DC_INITIAL, PHY_SDCLKDL_DC_R);
> +
> +	/* enable delay lane */
> +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> +	val &= ~(PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT);
> +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> +
> +	/* configure phy pads */
> +	val = sdhci_readw(host, PHY_CMDPAD_CNFG_R);
> +	sdhci_writew(host, val | PHY_PAD_RXSEL_3V3, PHY_CMDPAD_CNFG_R);
> +
> +	val = sdhci_readw(host, PHY_DATAPAD_CNFG_R);
> +	sdhci_writew(host, val | PHY_PAD_RXSEL_3V3, PHY_DATAPAD_CNFG_R);
> +
> +	val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R);
> +	sdhci_writew(host, val | PHY_PAD_RXSEL_3V3, PHY_RSTNPAD_CNFG_R);
> +
> +	val = sdhci_readw(host, PHY_STBPAD_CNFG_R);
> +	sdhci_writew(host, val | PHY_PAD_RXSEL_3V3, PHY_STBPAD_CNFG_R);
> +
> +	/* enable phy dll */
> +	val = sdhci_readb(host, PHY_DLL_CTRL_R);
> +	sdhci_writeb(host, val | PHY_DLL_CTRL_ENABLE, PHY_DLL_CTRL_R);
> +}
> +
> +static void th1520_phy_1_8v_init(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	u32 val;
> +
> +	if (!priv)
> +		return;
> +
> +	if (!(priv->flags & FLAG_PULL_UP_EN)) {
> +		th1520_phy_1_8v_init_no_pull(host);
> +		return;
> +	}
> +
> +	/* deassert phy reset & set tx drive strength */
> +	sdhci_writel(host, PHY_CNFG_RSTN_DEASSERT |
> +		    (PHY_CNFG_PAD_SP_VALUE << PHY_CNFG_PAD_SP_SHIFT) |
> +		    (PHY_CNFG_PAD_SN_VALUE << PHY_CNFG_PAD_SN_SHIFT),
> +		    PHY_CNFG_R);
> +
> +	/* disable delay line */
> +	sdhci_writeb(host, PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT,
> +		     PHY_SDCLKDL_CNFG_R);
> +
> +	/* set delay line */
> +	sdhci_writeb(host, priv->delay_line, PHY_SDCLKDL_DC_R);
> +	sdhci_writeb(host, PHY_DLL_CNFG2_JUMPSTEP, PHY_DLL_CNFG2_R);
> +
> +	/* enable delay lane */
> +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> +	val &= ~(PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT);
> +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> +
> +	/* configure phy pads */
> +	val = PHY_PAD_RXSEL_1V8 | (PHY_PAD_WEAKPULL_PULLUP << PHY_PAD_WEAKPULL_SHIFT) |
> +	      (PHY_PAD_TXSLEW_CTRL_P_VALUE << PHY_PAD_TXSLEW_CTRL_P_SHIFT) |
> +	      (PHY_PAD_TXSLEW_CTRL_N_VALUE << PHY_PAD_TXSLEW_CTRL_N_SHIFT);
> +	sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
> +	sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
> +	sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
> +
> +	val = (PHY_PAD_TXSLEW_CTRL_P_VALUE << PHY_PAD_TXSLEW_CTRL_P_SHIFT) |
> +	      (PHY_PAD_TXSLEW_CTRL_N_VALUE << PHY_PAD_TXSLEW_CTRL_N_SHIFT);
> +	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> +
> +	val = PHY_PAD_RXSEL_1V8 | (PHY_PAD_WEAKPULL_PULLDOWN << PHY_PAD_WEAKPULL_SHIFT) |
> +	      (PHY_PAD_TXSLEW_CTRL_P_VALUE << PHY_PAD_TXSLEW_CTRL_P_SHIFT) |
> +	      (PHY_PAD_TXSLEW_CTRL_N_VALUE << PHY_PAD_TXSLEW_CTRL_N_SHIFT);
> +	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> +
> +	/* enable data strobe mode */
> +	sdhci_writeb(host, PHY_DLLDL_CNFG_SLV_INPSEL_VALUE << PHY_DLLDL_CNFG_SLV_INPSEL_SHIFT,
> +		     PHY_DLLDL_CNFG_R);
> +
> +	/* enable phy dll */
> +	sdhci_writeb(host, PHY_DLL_CTRL_ENABLE,  PHY_DLL_CTRL_R);
> +}
> +
> +static void th1520_phy_3_3v_init(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	u32 val;
> +
> +	if (!(priv->flags & FLAG_PULL_UP_EN)) {
> +		th1520_phy_3_3v_init_no_pull(host);
> +		return;
> +	}
> +
> +	/* deassert phy reset & set tx drive strength */
> +	sdhci_writel(host, PHY_CNFG_RSTN_DEASSERT |
> +		    (PHY_CNFG_PAD_SP_VALUE << PHY_CNFG_PAD_SP_SHIFT) |
> +		    (PHY_CNFG_PAD_SN_VALUE << PHY_CNFG_PAD_SN_SHIFT),
> +		    PHY_CNFG_R);
> +
> +	/* disable delay line */
> +	sdhci_writeb(host, PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT,
> +		     PHY_SDCLKDL_CNFG_R);
> +
> +	/* set delay line */
> +	sdhci_writeb(host, priv->delay_line, PHY_SDCLKDL_DC_R);
> +	sdhci_writeb(host, PHY_DLL_CNFG2_JUMPSTEP, PHY_DLL_CNFG2_R);
> +
> +	/* enable delay lane */
> +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> +	val &= ~(PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT);
> +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> +
> +	/* configure phy pads */
> +	val = PHY_PAD_RXSEL_3V3 | (PHY_PAD_WEAKPULL_PULLUP << PHY_PAD_WEAKPULL_SHIFT) |
> +	      (PHY_PAD_TXSLEW_CTRL_P_VALUE << PHY_PAD_TXSLEW_CTRL_P_SHIFT) |
> +	      (PHY_PAD_TXSLEW_CTRL_N_VALUE << PHY_PAD_TXSLEW_CTRL_N_SHIFT);
> +	sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
> +	sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
> +	sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
> +
> +	val = (PHY_PAD_TXSLEW_CTRL_P_VALUE << PHY_PAD_TXSLEW_CTRL_P_SHIFT) |
> +	      (PHY_PAD_TXSLEW_CTRL_N_VALUE << PHY_PAD_TXSLEW_CTRL_N_SHIFT);
> +	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> +
> +	val = PHY_PAD_RXSEL_3V3 | (PHY_PAD_WEAKPULL_PULLDOWN << PHY_PAD_WEAKPULL_SHIFT) |
> +	      (PHY_PAD_TXSLEW_CTRL_P_VALUE << PHY_PAD_TXSLEW_CTRL_P_SHIFT) |
> +	      (PHY_PAD_TXSLEW_CTRL_N_VALUE << PHY_PAD_TXSLEW_CTRL_N_SHIFT);
> +	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> +}
> +
> +static void th1520_sdhci_set_phy(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	u16 emmc_ctrl;
> +
> +	/* Before power on, set PHY configs */
> +	if (host->mmc->caps & MMC_CAP_NONREMOVABLE) {

check flag & FLAG_IO_FIXED_1V8 instead? Because I didn't see why
3.3v can't be non-removeable. I know you only brought up emmc
now, but we can also prepare for sdio/sd support at the same time.

> +		th1520_phy_1_8v_init(host);
> +		emmc_ctrl = sdhci_readw(host, priv->vendor_specific_area1 + DWCMSHC_EMMC_CONTROL);
> +		emmc_ctrl |= DWCMSHC_CARD_IS_EMMC;

can we check host->mmc->caps2 to know whether this is eMMC or not, then
set this bit for eMMC after that.

And setting DWCMSHC_CARD_IS_EMMC should be orthogonalized with 3.3v/1.8v
etc.

> +		sdhci_writew(host, emmc_ctrl, priv->vendor_specific_area1 + DWCMSHC_EMMC_CONTROL);
> +	} else {
> +		th1520_phy_3_3v_init(host);

> +	}
> +
> +	sdhci_writeb(host, (PHY_DLL_CNFG1_SLVDLY_VALUE << PHY_DLL_CNFG1_SLVDLY_SHIFT) |

I'm not sure whether we need to get the delay value from DT.
Do emmc, sd and sdio share the same delay value?

> +		     PHY_DLL_CNFG1_WAITCYCLE, PHY_DLL_CNFG1_R);
> +}
> +
>  static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
>  				      unsigned int timing)
>  {
> @@ -189,9 +489,30 @@ static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
>  		ctrl_2 |= DWCMSHC_CTRL_HS400;
>  	}
>  
> +	if (priv->flags & FLAG_IO_FIXED_1V8)
> +		ctrl_2 |= SDHCI_CTRL_VDD_180;
>  	sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>  }
>  
> +static void th1520_set_uhs_signaling(struct sdhci_host *host,
> +				     unsigned int timing)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	u32 reg;
> +
> +	dwcmshc_set_uhs_signaling(host, timing);
> +	if (timing == MMC_TIMING_MMC_HS400) {
> +		reg = sdhci_readl(host, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
> +		reg &= ~AT_CTRL_AT_EN;
> +		sdhci_writel(host, reg, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);

can we move the auto tuning bit setting to tuning routine?

> +		priv->delay_line = PHY_SDCLKDL_DC_HS400;

this delay line may need to be gotten from DT?

> +		th1520_sdhci_set_phy(host);
> +	} else {
> +		sdhci_writeb(host, 0, PHY_DLLDL_CNFG_R);
> +	}
> +}
> +
>  static void dwcmshc_hs400_enhanced_strobe(struct mmc_host *mmc,
>  					  struct mmc_ios *ios)
>  {
> @@ -338,6 +659,91 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
>  	sdhci_reset(host, mask);
>  }
>  
> +static int th1520_execute_tuning(struct sdhci_host *host, u32 opcode)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	u32 val = 0;
> +
> +	if (host->flags & SDHCI_HS400_TUNING)
> +		return 0;
> +
> +	sdhci_writeb(host, PHY_ATDL_CNFG_INPSEL_VALUE << PHY_ATDL_CNFG_INPSEL_SHIFT,
> +		     PHY_ATDL_CNFG_R);
> +	val = sdhci_readl(host, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
> +
> +	/*
> +	 * configure tuning settings:
> +	 *  - center phase select code driven in block gap interval
> +	 *  - disable reporting of framing errors
> +	 *  - disable software managed tuning
> +	 *  - disable user selection of sampling window edges,
> +	 *    instead tuning calculated edges are used
> +	 */
> +	val &= ~((AT_CTRL_CI_SEL << AT_CTRL_CI_SEL_SHIFT) |
> +		(AT_CTRL_RPT_TUNE_ERR << AT_CTRL_RPT_TUNE_ERR_SHIFT) |
> +		(AT_CTRL_SW_TUNE_EN << AT_CTRL_SW_TUNE_EN_SHIFT) |
> +		(AT_CTRL_WIN_EDGE_SEL << AT_CTRL_WIN_EDGE_SEL_SHIFT));
> +
> +	/*
> +	 * configure tuning settings:
> +	 *  - enable auto-tuning
> +	 *  - enable sampling window threshold
> +	 *  - stop clocks during phase code change
> +	 *  - set max latency in cycles between tx and rx clocks
> +	 *  - set max latency in cycles to switch output phase
> +	 *  - set max sampling window threshold value
> +	 */
> +	val |= AT_CTRL_AT_EN | (AT_CTRL_SWIN_TH_EN << AT_CTRL_SWIN_TH_EN_SHIFT) |
> +		(AT_CTRL_TUNE_CLK_STOP_EN << AT_CTRL_TUNE_CLK_STOP_EN_SHIFT) |
> +		(AT_CTRL_PRE_CHANGE_DLY << AT_CTRL_PRE_CHANGE_DLY_SHIFT) |
> +		(AT_CTRL_POST_CHANGE_DLY << AT_CTRL_POST_CHANGE_DLY_SHIFT) |
> +		(AT_CTRL_SWIN_TH_VAL << AT_CTRL_SWIN_TH_VAL_SHIFT);
> +
> +	sdhci_writel(host, val, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
> +	val = sdhci_readl(host, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
> +
> +	/* check if is possible to enable auto-tuning */
> +	if (!(val & AT_CTRL_AT_EN)) {
> +		dev_err(mmc_dev(host->mmc), "failed to enable auto tuning\n");
> +		return -EIO;
> +	}
> +
> +	/* disable auto tuning */
> +	val &= ~AT_CTRL_AT_EN;
> +	sdhci_writel(host, val, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);

AFAIK, the HW support auto tuning, can we enable it?

> +
> +	/* perform tuning */
> +	sdhci_start_tuning(host);
> +	host->tuning_err = __sdhci_execute_tuning(host, opcode);
> +	if (host->tuning_err) {
> +		val &= ~AT_CTRL_AT_EN;

It looks like auto tunning is never enabled, so why do clear it here?

> +		sdhci_writel(host, val, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
> +		dev_err(mmc_dev(host->mmc), "tuning failed: %d\n", host->tuning_err);
> +		return -EIO;
> +	}
> +	sdhci_end_tuning(host);
> +
> +	return 0;
> +}
> +
> +static void th1520_sdhci_reset(struct sdhci_host *host, u8 mask)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	u16 ctrl_2;
> +
> +	sdhci_reset(host, mask);
> +
> +	if (priv->flags & FLAG_IO_FIXED_1V8) {
> +		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +		if (!(ctrl_2 & SDHCI_CTRL_VDD_180)) {
> +			ctrl_2 |= SDHCI_CTRL_VDD_180;
> +			sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> +		}
> +	}
> +}
> +
>  static const struct sdhci_ops sdhci_dwcmshc_ops = {
>  	.set_clock		= sdhci_set_clock,
>  	.set_bus_width		= sdhci_set_bus_width,
> @@ -356,6 +762,17 @@ static const struct sdhci_ops sdhci_dwcmshc_rk35xx_ops = {
>  	.adma_write_desc	= dwcmshc_adma_write_desc,
>  };
>  
> +static const struct sdhci_ops sdhci_dwcmshc_th1520_ops = {
> +	.set_clock		= sdhci_set_clock,
> +	.set_bus_width		= sdhci_set_bus_width,
> +	.set_uhs_signaling	= th1520_set_uhs_signaling,
> +	.get_max_clock		= dwcmshc_get_max_clock,
> +	.reset			= th1520_sdhci_reset,
> +	.adma_write_desc	= dwcmshc_adma_write_desc,
> +	.voltage_switch		= th1520_phy_1_8v_init,
> +	.platform_execute_tuning = &th1520_execute_tuning,
> +};
> +
>  static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {
>  	.ops = &sdhci_dwcmshc_ops,
>  	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> @@ -379,6 +796,12 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_rk35xx_pdata = {
>  		   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
>  };
>  
> +static const struct sdhci_pltfm_data sdhci_dwcmshc_th1520_pdata = {
> +	.ops = &sdhci_dwcmshc_th1520_ops,
> +	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> +};
> +
>  static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
>  {
>  	int err;
> @@ -447,6 +870,10 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
>  		.compatible = "snps,dwcmshc-sdhci",
>  		.data = &sdhci_dwcmshc_pdata,
>  	},
> +	{
> +		.compatible = "thead,th1520-dwcmshc",
> +		.data = &sdhci_dwcmshc_th1520_pdata,
> +	},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, sdhci_dwcmshc_dt_ids);
> @@ -542,6 +969,35 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  			goto err_clk;
>  	}
>  
> +	if (pltfm_data == &sdhci_dwcmshc_th1520_pdata) {
> +		priv->delay_line = PHY_SDCLKDL_DC_DEFAULT;

Ditto. 

> +
> +		if (device_property_present(&pdev->dev, "thead,phy-pull-up"))
> +			priv->flags |= FLAG_PULL_UP_EN;
> +		else
> +			priv->flags &= ~FLAG_PULL_UP_EN;
> +
> +		if ((device_property_read_bool(dev, "mmc-ddr-1_8v")) |
> +		    (device_property_read_bool(dev, "mmc-hs200-1_8v")) |
> +		    (device_property_read_bool(dev, "mmc-hs400-1_8v")))
> +			priv->flags |= FLAG_IO_FIXED_1V8;
> +		else
> +			priv->flags &= ~FLAG_IO_FIXED_1V8;
> +
> +		/*
> +		 * start_signal_voltage_switch() will try 3.3V first
> +		 * then 1.8V. Use SDHCI_SIGNALING_180 ranther than
> +		 * SDHCI_SIGNALING_330 to avoid setting voltage to 3.3V
> +		 * in sdhci_start_signal_voltage_switch().
> +		 */
> +		if (priv->flags & FLAG_IO_FIXED_1V8) {
> +			host->flags &= ~SDHCI_SIGNALING_330;
> +			host->flags |=  SDHCI_SIGNALING_180;
> +		}
> +
> +		sdhci_enable_v4_mode(host);
> +	}
> +
>  #ifdef CONFIG_ACPI
>  	if (pltfm_data == &sdhci_dwcmshc_bf3_pdata)
>  		sdhci_enable_v4_mode(host);
> 
> -- 
> 2.34.1
>
Drew Fustini Oct. 4, 2023, 7:12 p.m. UTC | #25
On Thu, Oct 05, 2023 at 12:19:03AM +0800, Jisheng Zhang wrote:
> On Thu, Sep 21, 2023 at 06:49:50PM -0700, Drew Fustini wrote:
> > Add support for the mmc controller in the T-Head TH1520 with the new
> > compatible "thead,th1520-dwcmshc". Implement custom sdhci_ops for
> > set_uhs_signaling, reset, voltage_switch, and platform_execute_tuning.
> > 
> > Signed-off-by: Drew Fustini <dfustini@baylibre.com>
> 
> Hi Drew,
> Thanks for doing this and sorry for being late for this code review. Some
> comments below.
> 
> > ---
> >  drivers/mmc/host/sdhci-of-dwcmshc.c | 456 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 456 insertions(+)
> > 
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > index 3a3bae6948a8..7294bf1afb7d 100644
> > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > @@ -35,6 +35,26 @@
> >  #define DWCMSHC_CARD_IS_EMMC		BIT(0)
> >  #define DWCMSHC_ENHANCED_STROBE		BIT(8)
> >  #define DWCMSHC_EMMC_ATCTRL		0x40
> > +/* Tuning and auto-tuning fields in AT_CTRL_R control register */
> > +#define AT_CTRL_AT_EN			0x1 /* autotuning is enabled */
> > +#define AT_CTRL_CI_SEL_SHIFT		0x1 /* bit 1 */
> > +#define AT_CTRL_CI_SEL			0x1 /* interval to drive center phase select */
> > +#define AT_CTRL_SWIN_TH_EN_SHIFT	0x2 /* bit 2 */
> > +#define AT_CTRL_SWIN_TH_EN		0x1 /* sampling window threshold enable */
> > +#define AT_CTRL_RPT_TUNE_ERR_SHIFT	0x3 /* bit 3 */
> > +#define AT_CTRL_RPT_TUNE_ERR		0x1 /* enable reporting framing errors */
> > +#define AT_CTRL_SW_TUNE_EN_SHIFT	0x4 /* bit 4 */
> > +#define AT_CTRL_SW_TUNE_EN		0x1 /* enable software managed tuning */
> > +#define AT_CTRL_WIN_EDGE_SEL_SHIFT	0x8 /* bits [11:8] */
> > +#define AT_CTRL_WIN_EDGE_SEL		0xf /* sampling window edge select */
> > +#define AT_CTRL_TUNE_CLK_STOP_EN_SHIFT	0x10 /* bit 16 */
> > +#define AT_CTRL_TUNE_CLK_STOP_EN	0x1  /* clocks stopped during phase code change */
> > +#define AT_CTRL_PRE_CHANGE_DLY_SHIFT	0x11 /* bits [18:17] */
> > +#define AT_CTRL_PRE_CHANGE_DLY		0x1  /* 2-cycle latency */
> > +#define AT_CTRL_POST_CHANGE_DLY_SHIFT	0x13 /* bits [20:19] */
> > +#define AT_CTRL_POST_CHANGE_DLY		0x3  /* 4-cycle latency */
> > +#define AT_CTRL_SWIN_TH_VAL_SHIFT	0x18 /* bits [31:24] */
> > +#define AT_CTRL_SWIN_TH_VAL		0x9  /* sampling window threshold */
> >  
> >  /* Rockchip specific Registers */
> >  #define DWCMSHC_EMMC_DLL_CTRL		0x800
> > @@ -72,6 +92,84 @@
> >  	(((x) & DWCMSHC_EMMC_DLL_TIMEOUT) == 0))
> >  #define RK35xx_MAX_CLKS 3
> >  
> > +/* PHY register area pointer */
> > +#define DWC_MSHC_PTR_PHY_R	0x300
> > +
> > +/* PHY general configuration */
> > +#define PHY_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x00)
> > +#define PHY_CNFG_RSTN_DEASSERT	0x1  /* Deassert PHY reset */
> > +#define PHY_CNFG_PAD_SP_SHIFT	0x10 /* bits [16:9] */
> > +#define PHY_CNFG_PAD_SP_VALUE	0x0c /* PMOS TX drive strength */
> > +#define PHY_CNFG_PAD_SN_SHIFT	0x14 /* bits [23:20] */
> > +#define PHY_CNFG_PAD_SN_VALUE	0x0c /* NMOS TX drive strength */
> > +
> > +/* PHY command/response pad settings */
> > +#define PHY_CMDPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x04)
> > +
> > +/* PHY data pad settings */
> > +#define PHY_DATAPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x06)
> > +
> > +/* PHY clock pad settings */
> > +#define PHY_CLKPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x08)
> > +
> > +/* PHY strobe pad settings */
> > +#define PHY_STBPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0a)
> > +
> > +/* PHY reset pad settings */
> > +#define PHY_RSTNPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0c)
> > +
> > +/* Bitfields are common for all pad settings */
> > +#define PHY_PAD_RXSEL_1V8		0x1 /* Receiver type select for 1.8V */
> > +#define PHY_PAD_RXSEL_3V3		0x2 /* Receiver type select for 3.3V */
> > +
> > +#define PHY_PAD_WEAKPULL_SHIFT		0x3 /* bits [4:3] */
> > +#define PHY_PAD_WEAKPULL_PULLUP		0x1 /* Weak pull down enabled */
> > +#define PHY_PAD_WEAKPULL_PULLDOWN	0x2 /* Weak pull down enabled */
> > +
> > +#define PHY_PAD_TXSLEW_CTRL_P_SHIFT	0x5 /* bits [8:5] */
> > +#define PHY_PAD_TXSLEW_CTRL_P_VALUE	0x3 /* Slew control for P-Type pad TX */
> > +#define PHY_PAD_TXSLEW_CTRL_N_SHIFT	0x9 /* bits [12:9] */
> > +#define PHY_PAD_TXSLEW_CTRL_N_VALUE	0x3 /* Slew control for N-Type pad TX */
> > +
> > +/* PHY CLK delay line settings */
> > +#define PHY_SDCLKDL_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x1d)
> > +#define PHY_SDCLKDL_CNFG_UPDATE_SHIFT	0x4 /* bit 4 */
> > +#define PHY_SDCLKDL_CNFG_UPDATE_DC	0x1 /* set before writing to SDCLKDL_DC */
> > +
> > +/* PHY CLK delay line delay code */
> > +#define PHY_SDCLKDL_DC_R		(DWC_MSHC_PTR_PHY_R + 0x1e)
> > +#define PHY_SDCLKDL_DC_INITIAL		0x40 /* initial delay code */
> > +#define PHY_SDCLKDL_DC_DEFAULT		0x32 /* default delay code */
> > +#define PHY_SDCLKDL_DC_HS400		0x18 /* delay code for HS400 mode */
> > +
> > +/* PHY drift_cclk_rx delay line configuration setting */
> > +#define PHY_ATDL_CNFG_R			(DWC_MSHC_PTR_PHY_R + 0x21)
> > +#define PHY_ATDL_CNFG_INPSEL_SHIFT	0x2 /* bits [3:2] */
> > +#define PHY_ATDL_CNFG_INPSEL_VALUE	0x3 /* delay line input source */
> > +
> > +/* PHY DLL control settings */
> > +#define PHY_DLL_CTRL_R			(DWC_MSHC_PTR_PHY_R + 0x24)
> > +#define PHY_DLL_CTRL_DISABLE		0x0 /* PHY DLL is enabled */
> > +#define PHY_DLL_CTRL_ENABLE		0x1 /* PHY DLL is disabled */
> > +
> > +/* PHY DLL  configuration register 1 */
> > +#define PHY_DLL_CNFG1_R			(DWC_MSHC_PTR_PHY_R + 0x25)
> > +#define PHY_DLL_CNFG1_SLVDLY_SHIFT	0x4 /* bits [5:4] */
> > +#define PHY_DLL_CNFG1_SLVDLY_VALUE	0x2 /* DLL slave update delay input */
> > +#define PHY_DLL_CNFG1_WAITCYCLE		0x5 /* DLL wait cycle input */
> > +
> > +/* PHY DLL configuration register 2 */
> > +#define PHY_DLL_CNFG2_R			(DWC_MSHC_PTR_PHY_R + 0x26)
> > +#define PHY_DLL_CNFG2_JUMPSTEP		0xa /* DLL jump step input */
> > +
> > +/* PHY DLL master and slave delay line configuration settings */
> > +#define PHY_DLLDL_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x28)
> > +#define PHY_DLLDL_CNFG_SLV_INPSEL_SHIFT	0x5 /* bits [6:5] */
> > +#define PHY_DLLDL_CNFG_SLV_INPSEL_VALUE	0x3 /* clock source select for slave DL */
> > +
> > +#define FLAG_PULL_UP_EN		BIT(0)
> > +#define FLAG_IO_FIXED_1V8	BIT(1)
> > +
> >  #define BOUNDARY_OK(addr, len) \
> >  	((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
> >  
> > @@ -92,6 +190,8 @@ struct dwcmshc_priv {
> >  	struct clk	*bus_clk;
> >  	int vendor_specific_area1; /* P_VENDOR_SPECIFIC_AREA reg */
> >  	void *priv; /* pointer to SoC private stuff */
> > +	u16 delay_line;
> > +	u16 flags;
> >  };
> >  
> >  /*
> > @@ -157,6 +257,206 @@ static void dwcmshc_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >  	sdhci_request(mmc, mrq);
> >  }
> >  
> > +static void th1520_phy_1_8v_init_no_pull(struct sdhci_host *host)
> > +{
> > +	u32 val;
> > +
> > +	/* deassert phy reset */
> > +	sdhci_writel(host, PHY_CNFG_RSTN_DEASSERT, PHY_CNFG_R);
> > +
> > +	/* disable delay line */
> > +	sdhci_writeb(host, PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT,
> > +		     PHY_SDCLKDL_CNFG_R);
> > +
> > +	/* set delay line */
> > +	sdhci_writeb(host, PHY_SDCLKDL_DC_INITIAL, PHY_SDCLKDL_DC_R);
> > +
> > +	/* enable delay lane */
> > +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > +	val &= ~(PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT);
> > +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > +
> > +	/* configure phy pads */
> > +	val = sdhci_readw(host, PHY_CMDPAD_CNFG_R);
> > +	sdhci_writew(host, val | PHY_PAD_RXSEL_1V8, PHY_CMDPAD_CNFG_R);
> > +
> > +	val = sdhci_readw(host, PHY_DATAPAD_CNFG_R);
> > +	sdhci_writew(host, val | PHY_PAD_RXSEL_1V8, PHY_DATAPAD_CNFG_R);
> > +
> > +	val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R);
> > +	sdhci_writew(host, val | PHY_PAD_RXSEL_1V8, PHY_RSTNPAD_CNFG_R);
> > +
> > +	val = sdhci_readw(host, PHY_STBPAD_CNFG_R);
> > +	sdhci_writew(host, val | PHY_PAD_RXSEL_1V8, PHY_STBPAD_CNFG_R);
> > +
> > +	/* enable phy dll */
> > +	val = sdhci_readb(host, PHY_DLL_CTRL_R);
> > +	sdhci_writeb(host, val | PHY_DLL_CTRL_ENABLE, PHY_DLL_CTRL_R);
> > +}
> > +
> > +static void th1520_phy_3_3v_init_no_pull(struct sdhci_host *host)
> > +{
> > +	u32 val;
> > +
> > +	/* deassert phy reset */
> > +	sdhci_writel(host, PHY_CNFG_RSTN_DEASSERT, PHY_CNFG_R);
> > +
> > +	/* disable delay line */
> > +	sdhci_writeb(host, PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT,
> > +		     PHY_SDCLKDL_CNFG_R);
> > +
> > +	/* set delay line */
> > +	sdhci_writeb(host, PHY_SDCLKDL_DC_INITIAL, PHY_SDCLKDL_DC_R);
> > +
> > +	/* enable delay lane */
> > +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > +	val &= ~(PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT);
> > +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > +
> > +	/* configure phy pads */
> > +	val = sdhci_readw(host, PHY_CMDPAD_CNFG_R);
> > +	sdhci_writew(host, val | PHY_PAD_RXSEL_3V3, PHY_CMDPAD_CNFG_R);
> > +
> > +	val = sdhci_readw(host, PHY_DATAPAD_CNFG_R);
> > +	sdhci_writew(host, val | PHY_PAD_RXSEL_3V3, PHY_DATAPAD_CNFG_R);
> > +
> > +	val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R);
> > +	sdhci_writew(host, val | PHY_PAD_RXSEL_3V3, PHY_RSTNPAD_CNFG_R);
> > +
> > +	val = sdhci_readw(host, PHY_STBPAD_CNFG_R);
> > +	sdhci_writew(host, val | PHY_PAD_RXSEL_3V3, PHY_STBPAD_CNFG_R);
> > +
> > +	/* enable phy dll */
> > +	val = sdhci_readb(host, PHY_DLL_CTRL_R);
> > +	sdhci_writeb(host, val | PHY_DLL_CTRL_ENABLE, PHY_DLL_CTRL_R);
> > +}
> > +
> > +static void th1520_phy_1_8v_init(struct sdhci_host *host)
> > +{
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +	u32 val;
> > +
> > +	if (!priv)
> > +		return;
> > +
> > +	if (!(priv->flags & FLAG_PULL_UP_EN)) {
> > +		th1520_phy_1_8v_init_no_pull(host);
> > +		return;
> > +	}
> > +
> > +	/* deassert phy reset & set tx drive strength */
> > +	sdhci_writel(host, PHY_CNFG_RSTN_DEASSERT |
> > +		    (PHY_CNFG_PAD_SP_VALUE << PHY_CNFG_PAD_SP_SHIFT) |
> > +		    (PHY_CNFG_PAD_SN_VALUE << PHY_CNFG_PAD_SN_SHIFT),
> > +		    PHY_CNFG_R);
> > +
> > +	/* disable delay line */
> > +	sdhci_writeb(host, PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT,
> > +		     PHY_SDCLKDL_CNFG_R);
> > +
> > +	/* set delay line */
> > +	sdhci_writeb(host, priv->delay_line, PHY_SDCLKDL_DC_R);
> > +	sdhci_writeb(host, PHY_DLL_CNFG2_JUMPSTEP, PHY_DLL_CNFG2_R);
> > +
> > +	/* enable delay lane */
> > +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > +	val &= ~(PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT);
> > +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > +
> > +	/* configure phy pads */
> > +	val = PHY_PAD_RXSEL_1V8 | (PHY_PAD_WEAKPULL_PULLUP << PHY_PAD_WEAKPULL_SHIFT) |
> > +	      (PHY_PAD_TXSLEW_CTRL_P_VALUE << PHY_PAD_TXSLEW_CTRL_P_SHIFT) |
> > +	      (PHY_PAD_TXSLEW_CTRL_N_VALUE << PHY_PAD_TXSLEW_CTRL_N_SHIFT);
> > +	sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
> > +	sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
> > +	sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
> > +
> > +	val = (PHY_PAD_TXSLEW_CTRL_P_VALUE << PHY_PAD_TXSLEW_CTRL_P_SHIFT) |
> > +	      (PHY_PAD_TXSLEW_CTRL_N_VALUE << PHY_PAD_TXSLEW_CTRL_N_SHIFT);
> > +	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> > +
> > +	val = PHY_PAD_RXSEL_1V8 | (PHY_PAD_WEAKPULL_PULLDOWN << PHY_PAD_WEAKPULL_SHIFT) |
> > +	      (PHY_PAD_TXSLEW_CTRL_P_VALUE << PHY_PAD_TXSLEW_CTRL_P_SHIFT) |
> > +	      (PHY_PAD_TXSLEW_CTRL_N_VALUE << PHY_PAD_TXSLEW_CTRL_N_SHIFT);
> > +	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> > +
> > +	/* enable data strobe mode */
> > +	sdhci_writeb(host, PHY_DLLDL_CNFG_SLV_INPSEL_VALUE << PHY_DLLDL_CNFG_SLV_INPSEL_SHIFT,
> > +		     PHY_DLLDL_CNFG_R);
> > +
> > +	/* enable phy dll */
> > +	sdhci_writeb(host, PHY_DLL_CTRL_ENABLE,  PHY_DLL_CTRL_R);
> > +}
> > +
> > +static void th1520_phy_3_3v_init(struct sdhci_host *host)
> > +{
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +	u32 val;
> > +
> > +	if (!(priv->flags & FLAG_PULL_UP_EN)) {
> > +		th1520_phy_3_3v_init_no_pull(host);
> > +		return;
> > +	}
> > +
> > +	/* deassert phy reset & set tx drive strength */
> > +	sdhci_writel(host, PHY_CNFG_RSTN_DEASSERT |
> > +		    (PHY_CNFG_PAD_SP_VALUE << PHY_CNFG_PAD_SP_SHIFT) |
> > +		    (PHY_CNFG_PAD_SN_VALUE << PHY_CNFG_PAD_SN_SHIFT),
> > +		    PHY_CNFG_R);
> > +
> > +	/* disable delay line */
> > +	sdhci_writeb(host, PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT,
> > +		     PHY_SDCLKDL_CNFG_R);
> > +
> > +	/* set delay line */
> > +	sdhci_writeb(host, priv->delay_line, PHY_SDCLKDL_DC_R);
> > +	sdhci_writeb(host, PHY_DLL_CNFG2_JUMPSTEP, PHY_DLL_CNFG2_R);
> > +
> > +	/* enable delay lane */
> > +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > +	val &= ~(PHY_SDCLKDL_CNFG_UPDATE_DC << PHY_SDCLKDL_CNFG_UPDATE_SHIFT);
> > +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > +
> > +	/* configure phy pads */
> > +	val = PHY_PAD_RXSEL_3V3 | (PHY_PAD_WEAKPULL_PULLUP << PHY_PAD_WEAKPULL_SHIFT) |
> > +	      (PHY_PAD_TXSLEW_CTRL_P_VALUE << PHY_PAD_TXSLEW_CTRL_P_SHIFT) |
> > +	      (PHY_PAD_TXSLEW_CTRL_N_VALUE << PHY_PAD_TXSLEW_CTRL_N_SHIFT);
> > +	sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
> > +	sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
> > +	sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
> > +
> > +	val = (PHY_PAD_TXSLEW_CTRL_P_VALUE << PHY_PAD_TXSLEW_CTRL_P_SHIFT) |
> > +	      (PHY_PAD_TXSLEW_CTRL_N_VALUE << PHY_PAD_TXSLEW_CTRL_N_SHIFT);
> > +	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> > +
> > +	val = PHY_PAD_RXSEL_3V3 | (PHY_PAD_WEAKPULL_PULLDOWN << PHY_PAD_WEAKPULL_SHIFT) |
> > +	      (PHY_PAD_TXSLEW_CTRL_P_VALUE << PHY_PAD_TXSLEW_CTRL_P_SHIFT) |
> > +	      (PHY_PAD_TXSLEW_CTRL_N_VALUE << PHY_PAD_TXSLEW_CTRL_N_SHIFT);
> > +	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> > +}
> > +
> > +static void th1520_sdhci_set_phy(struct sdhci_host *host)
> > +{
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +	u16 emmc_ctrl;
> > +
> > +	/* Before power on, set PHY configs */
> > +	if (host->mmc->caps & MMC_CAP_NONREMOVABLE) {
> 
> check flag & FLAG_IO_FIXED_1V8 instead? Because I didn't see why
> 3.3v can't be non-removeable. I know you only brought up emmc
> now, but we can also prepare for sdio/sd support at the same time.

Okay, that does seem to make more sense to check for the 1.8V flag when
deciding whether to call th1520_phy_1_8v_init().

> 
> > +		th1520_phy_1_8v_init(host);
> > +		emmc_ctrl = sdhci_readw(host, priv->vendor_specific_area1 + DWCMSHC_EMMC_CONTROL);
> > +		emmc_ctrl |= DWCMSHC_CARD_IS_EMMC;
> 
> can we check host->mmc->caps2 to know whether this is eMMC or not, then
> set this bit for eMMC after that.
> 
> And setting DWCMSHC_CARD_IS_EMMC should be orthogonalized with 3.3v/1.8v
> etc.

Okay, I'll move the write to emmc_ctrl outside of this if() block. I'll
put it in a new if() block directly after. There is MMC_CAP_NONREMOVABLE
but that is mmc_host->caps and not mmc_host->caps2. Is that what you
meant?

> 
> > +		sdhci_writew(host, emmc_ctrl, priv->vendor_specific_area1 + DWCMSHC_EMMC_CONTROL);
> > +	} else {
> > +		th1520_phy_3_3v_init(host);
> 
> > +	}
> > +
> > +	sdhci_writeb(host, (PHY_DLL_CNFG1_SLVDLY_VALUE << PHY_DLL_CNFG1_SLVDLY_SHIFT) |
> 
> I'm not sure whether we need to get the delay value from DT.
> Do emmc, sd and sdio share the same delay value?

The PHY_DLL_CNFG1_SLVDLY_VALUE ("DLL slave update delay input") field
appeared to be the same for all instances in the T-Head SDK.

Whereas, the PHY_SDCLKDL_DC register ("CLK delay line delay code") does
have different value depending on whether HS400 mode is used.

> 
> > +		     PHY_DLL_CNFG1_WAITCYCLE, PHY_DLL_CNFG1_R);
> > +}
> > +
> >  static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
> >  				      unsigned int timing)
> >  {
> > @@ -189,9 +489,30 @@ static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
> >  		ctrl_2 |= DWCMSHC_CTRL_HS400;
> >  	}
> >  
> > +	if (priv->flags & FLAG_IO_FIXED_1V8)
> > +		ctrl_2 |= SDHCI_CTRL_VDD_180;
> >  	sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> >  }
> >  
> > +static void th1520_set_uhs_signaling(struct sdhci_host *host,
> > +				     unsigned int timing)
> > +{
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +	u32 reg;
> > +
> > +	dwcmshc_set_uhs_signaling(host, timing);
> > +	if (timing == MMC_TIMING_MMC_HS400) {
> > +		reg = sdhci_readl(host, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
> > +		reg &= ~AT_CTRL_AT_EN;
> > +		sdhci_writel(host, reg, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
> 
> can we move the auto tuning bit setting to tuning routine?

It is unclear to me why the T-Head's version manipulated AT_CTRL_AT_EN
in the th1520_set_uhs_signaling(). I can try removing it and seeing if
there is any effect.

> 
> > +		priv->delay_line = PHY_SDCLKDL_DC_HS400;
> 
> this delay line may need to be gotten from DT?

There were only 3 different values used in the T-Head SDK so I am not
sure a DT property is needed. The three values for the CLK delayline
delay code (PHY_SDCLKDL_DC) are:
  
  PHY_SDCLKDL_DC_INITIAL          0x40
  PHY_SDCLKDL_DC_DEFAULT          0x32
  PHY_SDCLKDL_DC_HS400            0x18

PHY_SDCLKDL_DC_INITIAL is only used in th1520_phy_1_8v_init_no_pull()
and th1520_phy_3_3v_init_no_pull(). Based on earlier discussion in this
thread, I will be removing those functions in the next revision as they
are unused.

th1520_phy_1_8v_init() and th1520_phy_3_3v_init() both use
dwcmshc_priv.delay_line to program the PHY_SDCLKDL_DC register.

dwcmshc_probe() sets delay_line to PHY_SDCLKDL_DC_DEFAULT. If HS400 is
used, then th1520_set_uhs_signaling() will change delay_line to
PHY_SDCLKDL_DC_HS400.

Based on the above, I don't think DT needs direct control of the
delay_line value, but I can add a delay line DT prop if you prefer.

> 
> > +		th1520_sdhci_set_phy(host);
> > +	} else {
> > +		sdhci_writeb(host, 0, PHY_DLLDL_CNFG_R);
> > +	}
> > +}
> > +
> >  static void dwcmshc_hs400_enhanced_strobe(struct mmc_host *mmc,
> >  					  struct mmc_ios *ios)
> >  {
> > @@ -338,6 +659,91 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
> >  	sdhci_reset(host, mask);
> >  }
> >  
> > +static int th1520_execute_tuning(struct sdhci_host *host, u32 opcode)
> > +{
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +	u32 val = 0;
> > +
> > +	if (host->flags & SDHCI_HS400_TUNING)
> > +		return 0;
> > +
> > +	sdhci_writeb(host, PHY_ATDL_CNFG_INPSEL_VALUE << PHY_ATDL_CNFG_INPSEL_SHIFT,
> > +		     PHY_ATDL_CNFG_R);
> > +	val = sdhci_readl(host, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
> > +
> > +	/*
> > +	 * configure tuning settings:
> > +	 *  - center phase select code driven in block gap interval
> > +	 *  - disable reporting of framing errors
> > +	 *  - disable software managed tuning
> > +	 *  - disable user selection of sampling window edges,
> > +	 *    instead tuning calculated edges are used
> > +	 */
> > +	val &= ~((AT_CTRL_CI_SEL << AT_CTRL_CI_SEL_SHIFT) |
> > +		(AT_CTRL_RPT_TUNE_ERR << AT_CTRL_RPT_TUNE_ERR_SHIFT) |
> > +		(AT_CTRL_SW_TUNE_EN << AT_CTRL_SW_TUNE_EN_SHIFT) |
> > +		(AT_CTRL_WIN_EDGE_SEL << AT_CTRL_WIN_EDGE_SEL_SHIFT));
> > +
> > +	/*
> > +	 * configure tuning settings:
> > +	 *  - enable auto-tuning
> > +	 *  - enable sampling window threshold
> > +	 *  - stop clocks during phase code change
> > +	 *  - set max latency in cycles between tx and rx clocks
> > +	 *  - set max latency in cycles to switch output phase
> > +	 *  - set max sampling window threshold value
> > +	 */
> > +	val |= AT_CTRL_AT_EN | (AT_CTRL_SWIN_TH_EN << AT_CTRL_SWIN_TH_EN_SHIFT) |
> > +		(AT_CTRL_TUNE_CLK_STOP_EN << AT_CTRL_TUNE_CLK_STOP_EN_SHIFT) |
> > +		(AT_CTRL_PRE_CHANGE_DLY << AT_CTRL_PRE_CHANGE_DLY_SHIFT) |
> > +		(AT_CTRL_POST_CHANGE_DLY << AT_CTRL_POST_CHANGE_DLY_SHIFT) |
> > +		(AT_CTRL_SWIN_TH_VAL << AT_CTRL_SWIN_TH_VAL_SHIFT);
> > +
> > +	sdhci_writel(host, val, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
> > +	val = sdhci_readl(host, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
> > +
> > +	/* check if is possible to enable auto-tuning */
> > +	if (!(val & AT_CTRL_AT_EN)) {
> > +		dev_err(mmc_dev(host->mmc), "failed to enable auto tuning\n");
> > +		return -EIO;
> > +	}
> > +
> > +	/* disable auto tuning */
> > +	val &= ~AT_CTRL_AT_EN;
> > +	sdhci_writel(host, val, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
> 
> AFAIK, the HW support auto tuning, can we enable it?

I am not sure why the version of the driver in the T-Head SDK disables
auto tuning.

The AT_EN field in the AT_CTRL register is supposed to be enabled by
default if the controller was designed to support Mode3 retuning. I am
not sure if this is the case for this SoC.

AT_EN is meant to be cleared when software wishes to disable Mode3
retuning which I believe is also known as Mode1.

I suppose the best thing for me to do is experiement with AT_EN and see
if I can in fact turn on auto-tuning.

> 
> > +
> > +	/* perform tuning */
> > +	sdhci_start_tuning(host);
> > +	host->tuning_err = __sdhci_execute_tuning(host, opcode);
> > +	if (host->tuning_err) {
> > +		val &= ~AT_CTRL_AT_EN;
> 
> It looks like auto tunning is never enabled, so why do clear it here?

I was confused by this also. I copied it from the T-Head SDK but I agree
it doesn't seem to do anything useful.

> 
> > +		sdhci_writel(host, val, priv->vendor_specific_area1 + DWCMSHC_EMMC_ATCTRL);
> > +		dev_err(mmc_dev(host->mmc), "tuning failed: %d\n", host->tuning_err);
> > +		return -EIO;
> > +	}
> > +	sdhci_end_tuning(host);
> > +
> > +	return 0;
> > +}
> > +
> > +static void th1520_sdhci_reset(struct sdhci_host *host, u8 mask)
> > +{
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +	u16 ctrl_2;
> > +
> > +	sdhci_reset(host, mask);
> > +
> > +	if (priv->flags & FLAG_IO_FIXED_1V8) {
> > +		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> > +		if (!(ctrl_2 & SDHCI_CTRL_VDD_180)) {
> > +			ctrl_2 |= SDHCI_CTRL_VDD_180;
> > +			sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> > +		}
> > +	}
> > +}
> > +
> >  static const struct sdhci_ops sdhci_dwcmshc_ops = {
> >  	.set_clock		= sdhci_set_clock,
> >  	.set_bus_width		= sdhci_set_bus_width,
> > @@ -356,6 +762,17 @@ static const struct sdhci_ops sdhci_dwcmshc_rk35xx_ops = {
> >  	.adma_write_desc	= dwcmshc_adma_write_desc,
> >  };
> >  
> > +static const struct sdhci_ops sdhci_dwcmshc_th1520_ops = {
> > +	.set_clock		= sdhci_set_clock,
> > +	.set_bus_width		= sdhci_set_bus_width,
> > +	.set_uhs_signaling	= th1520_set_uhs_signaling,
> > +	.get_max_clock		= dwcmshc_get_max_clock,
> > +	.reset			= th1520_sdhci_reset,
> > +	.adma_write_desc	= dwcmshc_adma_write_desc,
> > +	.voltage_switch		= th1520_phy_1_8v_init,
> > +	.platform_execute_tuning = &th1520_execute_tuning,
> > +};
> > +
> >  static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {
> >  	.ops = &sdhci_dwcmshc_ops,
> >  	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> > @@ -379,6 +796,12 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_rk35xx_pdata = {
> >  		   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
> >  };
> >  
> > +static const struct sdhci_pltfm_data sdhci_dwcmshc_th1520_pdata = {
> > +	.ops = &sdhci_dwcmshc_th1520_ops,
> > +	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> > +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> > +};
> > +
> >  static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
> >  {
> >  	int err;
> > @@ -447,6 +870,10 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
> >  		.compatible = "snps,dwcmshc-sdhci",
> >  		.data = &sdhci_dwcmshc_pdata,
> >  	},
> > +	{
> > +		.compatible = "thead,th1520-dwcmshc",
> > +		.data = &sdhci_dwcmshc_th1520_pdata,
> > +	},
> >  	{},
> >  };
> >  MODULE_DEVICE_TABLE(of, sdhci_dwcmshc_dt_ids);
> > @@ -542,6 +969,35 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >  			goto err_clk;
> >  	}
> >  
> > +	if (pltfm_data == &sdhci_dwcmshc_th1520_pdata) {
> > +		priv->delay_line = PHY_SDCLKDL_DC_DEFAULT;
> 
> Ditto. 

There seems to only be two values used for the CLK delayline delay code,
one for default and one for HS400. However, I could make it a dt
property if you prefer.

> 
> > +
> > +		if (device_property_present(&pdev->dev, "thead,phy-pull-up"))
> > +			priv->flags |= FLAG_PULL_UP_EN;
> > +		else
> > +			priv->flags &= ~FLAG_PULL_UP_EN;
> > +
> > +		if ((device_property_read_bool(dev, "mmc-ddr-1_8v")) |
> > +		    (device_property_read_bool(dev, "mmc-hs200-1_8v")) |
> > +		    (device_property_read_bool(dev, "mmc-hs400-1_8v")))
> > +			priv->flags |= FLAG_IO_FIXED_1V8;
> > +		else
> > +			priv->flags &= ~FLAG_IO_FIXED_1V8;
> > +
> > +		/*
> > +		 * start_signal_voltage_switch() will try 3.3V first
> > +		 * then 1.8V. Use SDHCI_SIGNALING_180 ranther than
> > +		 * SDHCI_SIGNALING_330 to avoid setting voltage to 3.3V
> > +		 * in sdhci_start_signal_voltage_switch().
> > +		 */
> > +		if (priv->flags & FLAG_IO_FIXED_1V8) {
> > +			host->flags &= ~SDHCI_SIGNALING_330;
> > +			host->flags |=  SDHCI_SIGNALING_180;
> > +		}
> > +
> > +		sdhci_enable_v4_mode(host);
> > +	}
> > +
> >  #ifdef CONFIG_ACPI
> >  	if (pltfm_data == &sdhci_dwcmshc_bf3_pdata)
> >  		sdhci_enable_v4_mode(host);
> > 
> > -- 
> > 2.34.1
> > 

Thank you for the review,
Drew
Robin Murphy Oct. 4, 2023, 7:38 p.m. UTC | #26
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
>
Prabhakar Oct. 4, 2023, 8:47 p.m. UTC | #27
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
Christoph Hellwig Oct. 5, 2023, 6:57 a.m. UTC | #28
No.  The global pool is a last resort only for nommu and other very
limited setups.  It should never be enable in a general-purpose kernel,
and if your hardware requires it is a failed designed and should not
be supported in a general purpose kernel build.