Re: [PATCH] ASoC: SOF: mediatek: mt8186 modify dram type as non-cache

["Trevor Wu (吳文良)" <Trevor.Wu@mediatek.com>]

On Thu, 2023-08-03 at 11:04 +0300, Eugen Hristev wrote:
> Hi Trevor,
> 
> On 8/3/23 10:50, Trevor Wu wrote:
> > To prevent incorrect access between the host and DSP sides, we need
> > to
> > modify DRAM as a non-cache memory type. Additionally, we can
> > retrieve
> > the size of shared DMA from the device tree.
> > 
> > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com>
> > Reviewed-by: Yaochun Hung <yc.hung@mediatek.com>
> > Reviewed-by: Kuan-Hsun Cheng <Allen-KH.Cheng@mediatek.com>
> > ---
> >   sound/soc/sof/mediatek/mt8186/mt8186.c | 40 +++++++++++++++----
> > -------
> >   1 file changed, 23 insertions(+), 17 deletions(-)
> > 
> > diff --git a/sound/soc/sof/mediatek/mt8186/mt8186.c
> > b/sound/soc/sof/mediatek/mt8186/mt8186.c
> > index 3e0ea0e109e2..f587edf9e0a7 100644
> > --- a/sound/soc/sof/mediatek/mt8186/mt8186.c
> > +++ b/sound/soc/sof/mediatek/mt8186/mt8186.c
> > @@ -111,6 +111,14 @@ static int platform_parse_resource(struct
> > platform_device *pdev, void *data)
> >   
> >   	dev_dbg(dev, "DMA %pR\n", &res);
> >   
> > +	adsp->pa_shared_dram = (phys_addr_t)res.start;
> > +	adsp->shared_size = resource_size(&res);
> > +	if (adsp->pa_shared_dram & DRAM_REMAP_MASK) {
> > +		dev_err(dev, "adsp shared dma memory(%#x) is not 4K-
> > aligned\n",
> > +			(u32)adsp->pa_shared_dram);
> > +		return -EINVAL;
> > +	}
> > +
> 
> Would it be better to just realign to the next 4k boundary ?
> Or, isn't it more usual to use dma_coerce_mask_and_coherent ?

Hi Eugen,
 
The 4k boundary checking serves as a sentinel to ensure that the memory
we assign on the AP side can be utilized on the DSP side. If we handle
it in the way you suggested, it would mean that some reserved memory
will never be used. 

> >   	ret = of_reserved_mem_device_init(dev);
> >   	if (ret) {
> >   		dev_err(dev, "of_reserved_mem_device_init failed\n");
> > @@ -244,23 +252,18 @@ static int adsp_shared_base_ioremap(struct
> > platform_device *pdev, void *data)
> >   {
> >   	struct device *dev = &pdev->dev;
> >   	struct mtk_adsp_chip_info *adsp = data;
> > -	u32 shared_size;
> >   
> >   	/* remap shared-dram base to be non-cachable */
> > -	shared_size = TOTAL_SIZE_SHARED_DRAM_FROM_TAIL;
> > -	adsp->pa_shared_dram = adsp->pa_dram + adsp->dramsize -
> > shared_size;
> > -	if (adsp->va_dram) {
> > -		adsp->shared_dram = adsp->va_dram + DSP_DRAM_SIZE -
> > shared_size;
> > -	} else {
> > -		adsp->shared_dram = devm_ioremap(dev, adsp-
> > >pa_shared_dram,
> > -						 shared_size);
> > -		if (!adsp->shared_dram) {
> > -			dev_err(dev, "ioremap failed for shared
> > DRAM\n");
> > -			return -ENOMEM;
> > -		}
> > +	adsp->shared_dram = devm_ioremap(dev, adsp->pa_shared_dram,
> > +					 adsp->shared_size);
> 
> You cannot use dma_alloc_coherent ? This should take care of all the 
> cache maintainance for you.

The purpose of this patch is to align the implementation on mt8195[1].
In fact, the usage of "adsp->shared_dram" has been discontinued. I will
create a follow-up patch to remove this part.

> 
> > +	if (!adsp->shared_dram) {
> > +		dev_err(dev, "failed to ioremap base %pa size %#x\n",
> > +			adsp->shared_dram, adsp->shared_size);
> > +		return -ENOMEM;
> >   	}
> > -	dev_dbg(dev, "shared-dram vbase=%p, phy addr :%pa, size=%#x\n",
> > -		adsp->shared_dram, &adsp->pa_shared_dram, shared_size);
> > +
> > +	dev_dbg(dev, "shared-dram vbase=%p, phy addr
> > :%pa,  size=%#x\n",
> > +		adsp->shared_dram, &adsp->pa_shared_dram, adsp-
> > >shared_size);
> >   
> >   	return 0;
> >   }
> > @@ -307,9 +310,12 @@ static int mt8186_dsp_probe(struct snd_sof_dev
> > *sdev)
> >   		return -ENOMEM;
> >   	}
> >   
> > -	sdev->bar[SOF_FW_BLK_TYPE_SRAM] = devm_ioremap_wc(sdev->dev,
> > -							  priv->adsp-
> > >pa_dram,
> > -							  priv->adsp-
> > >dramsize);
> > +	priv->adsp->va_sram = sdev->bar[SOF_FW_BLK_TYPE_IRAM];
> > +
> > +	sdev->bar[SOF_FW_BLK_TYPE_SRAM] = devm_ioremap(sdev->dev,
> > +						       priv->adsp-
> > >pa_dram,
> > +						       priv->adsp-
> > >dramsize);
> > +
> 
> Same here

There are two separate memories. However, the memory being referred to
here is not the one we set for of_reserved_mem_device_init().

[1]
https://lore.kernel.org/all/20220606210212.146626-5-pierre-louis.bossart@linux.intel.com/

Thanks,
Trevor