Re: [PATCH] mmc: sdhci-xenon: Fix 2G limitation on AC5 SoC

[Adrian Hunter]

On 1/08/22 12:30, Vadym Kochan wrote:
> Hi Florian,
> 
> On Wed, Jul 27, 2022 at 07:45:32PM +0300, Vadym Kochan wrote:
>> Hi Florian,
>>
>> On Tue, Jul 26, 2022 at 10:37:46AM -0700, Florian Fainelli wrote:
>>> On 7/26/22 10:07, Vadym Kochan wrote:
>>>> From: Elad Nachman <enachman@xxxxxxxxxxx>
>>>>
>>>> There is a limitation on AC5 SoC that mmc controller
>>>> can't have DMA access over 2G memory.
>>>>
>>>> Signed-off-by: Elad Nachman <enachman@xxxxxxxxxxx>
>>>> Signed-off-by: Vadym Kochan <vadym.kochan@xxxxxxxxxxx>
>>>> ---
>>>>  drivers/mmc/host/sdhci-xenon.c | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
>>>> index 08e838400b52..666d06b58564 100644
>>>> --- a/drivers/mmc/host/sdhci-xenon.c
>>>> +++ b/drivers/mmc/host/sdhci-xenon.c
>>>> @@ -18,6 +18,7 @@
>>>>  #include <linux/of.h>
>>>>  #include <linux/pm.h>
>>>>  #include <linux/pm_runtime.h>
>>>> +#include <linux/mm.h>
>>>>  
>>>>  #include "sdhci-pltfm.h"
>>>>  #include "sdhci-xenon.h"
>>>> @@ -422,6 +423,8 @@ static int xenon_probe_params(struct platform_device *pdev)
>>>>  	struct xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>>>>  	u32 sdhc_id, nr_sdhc;
>>>>  	u32 tuning_count;
>>>> +	struct device_node *np = pdev->dev.of_node;
>>>> +	struct sysinfo si;
>>>>  
>>>>  	/* Disable HS200 on Armada AP806 */
>>>>  	if (priv->hw_version == XENON_AP806)
>>>> @@ -450,6 +453,15 @@ static int xenon_probe_params(struct platform_device *pdev)
>>>>  	}
>>>>  	priv->tuning_count = tuning_count;
>>>>  
>>>> +	si_meminfo(&si);
>>>> +
>>>> +	if (of_device_is_compatible(np, "marvell,ac5-sdhci") &&
>>>> +	    ((si.totalram * si.mem_unit) > 0x80000000 /*2G*/)) {
>>>
>>> Why not limit the DMA mask of the device and ensure, that bounce buffers get used so you can still do DMA?
>>>
>>> Also, you ought to be able to describe that limitation using Device Tree (assuming this is an option) and declaring a dedicated bus node for the SDHCI controller and providing a suitable dma-ranges property, see: arch/arm/boot/dts/bcm2711.dtsi and the 'soc' node for such examples.
>>>
>>>
>>
> 
> I could use DMA only in 2 ways:
> 
> #1 Use sdhci bounce buffer with SDMA mode
> 
>     But there was the issue that SDMA requires that SDHCI v4 mode should
>     be enabled, and when I enable it via sdhci_enable_v4_mode(host)
>     then I got error that EXT_CSD can't be recognized.
> 
>     But if I comment this line in sdhci.c:
>     
>     int sdhci_setup_host(struct sdhci_host *host)
>     {
>         ...
> 
> 	/* SDMA does not support 64-bit DMA if v4 mode not set */
> 	if ((host->flags & SDHCI_USE_64_BIT_DMA) && !host->v4_mode) {
> 		pr_info("XXX SDMA does not support 64-bit DMA if v4 mode not set\n");
> 		host->flags &= ~SDHCI_USE_SDMA;
> 	}
> 
>        ...
>     }
> 
>     then everything is OK.
> 
> #2 Use restricted-dma-pool in device-tree
> 
>    But I am not sure if it is good solution compared to #1.
> 
> Setting only DMA mask did not help because after some time I got
> "DMA overflow address" error stack-traces.

AFAICT using a DMA mask is the correct way to solve this.  If that
reveals another issue then that must be resolved also. Can you show
the stack traces and what kernel they are from?

> 
>> Do I understand correctly that the swiotlb will be used in case DMA
>> engine could not map the page in the specified range (limited by dma-ranges or
>> dma-mask) ?
>>
>>>
>>>> +		host->quirks |= SDHCI_QUIRK_BROKEN_DMA;
>>>> +		host->quirks |= SDHCI_QUIRK_BROKEN_ADMA;
>>>> +		dev_info(mmc_dev(mmc), "Disabling DMA because of 2GB DMA access limit.\n");
>>>> +	}
>>>> +
>>>>  	return xenon_phy_parse_params(dev, host);
>>>>  }
>>>>  
>>>> @@ -682,6 +694,7 @@ static const struct of_device_id sdhci_xenon_dt_ids[] = {
>>>>  	{ .compatible = "marvell,armada-ap807-sdhci", .data = (void *)XENON_AP807},
>>>>  	{ .compatible = "marvell,armada-cp110-sdhci", .data =  (void *)XENON_CP110},
>>>>  	{ .compatible = "marvell,armada-3700-sdhci", .data =  (void *)XENON_A3700},
>>>> +	{ .compatible = "marvell,ac5-sdhci", .data = (void *)XENON_AP806},
>>>>  	{}
>>>>  };
>>>>  MODULE_DEVICE_TABLE(of, sdhci_xenon_dt_ids);
>>>
>>>
>>> -- 
>>> Florian
>>
>> Thanks,
>>
> 
> Regards,