[EXT] [PATCH] ARM: imx: Add OCRAM_S into iMX8M MMU tables

[Marek Vasut]

On 2/27/21 7:26 AM, Ye Li wrote:
> Hi Marek,

Hi,

[...]
>>>> diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-
>>>> imx/imx8m/soc.c
>>>> index 5456c10fb17..225e4e12500 100644
>>>> --- a/arch/arm/mach-imx/imx8m/soc.c
>>>> +++ b/arch/arm/mach-imx/imx8m/soc.c
>>>> @@ -104,6 +104,13 @@ static struct mm_region imx8m_mem_map[] = {
>>>>                   .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |
>>>>                            PTE_BLOCK_NON_SHARE |
>>>>                            PTE_BLOCK_PXN | PTE_BLOCK_UXN
>>>> +       }, {
>>>> +               /* OCRAM_S */
>>>> +               .virt = 0x180000UL,
>>>> +               .phys = 0x180000UL,
>>>> +               .size = 0x8000UL,
>>>> +               .attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |
>>>> +                        PTE_BLOCK_OUTER_SHARE
>>>>           }, {
>>>>                   /* TCM */
>>>>                   .virt = 0x7C0000UL,
>>>> --
>>>> 2.30.0
>>>>
>>> OCRAM_S is used by ATF and SPL to pass DDR CSR data.
>> Where is this implemented ?
> 
> See below definition in drivers/ddr/imx/imx8m/Kconfig
> 
> config SAVED_DRAM_TIMING_BASE
> 	hex "Define the base address for saved dram timing"
> 	help
> 	  after DRAM is trained, need to save the dram related timming
> 	  info into memory for low power use. OCRAM_S is used for this
> 	  purpose on i.MX8MM.
> 	default 0x180000

So, this just defines some sort of value. Note that some boards do 
override this value to NOT point into OCRAM_S .

I think what you wanted to point me to is drivers/ddr/imx/imx8m/ddr_init.c:
248         dram_config_save(dram_timing, CONFIG_SAVED_DRAM_TIMING_BASE);
which writes regular memory. And that regular memory can very well be 
cacheable.

>>> It is better not
>>> use it in u-boot to avoid any DDR issue.
>> The MMU table entry does not trigger any IO to the OCRAM_S , it
>> merely
>> makes it cacheable .
>>
> That's fine to add a map, just remind to use it carefully since it
> already used by ATF.

Not necessarily, use git grep SAVED_DRAM_TIMING_BASE configs/ and see 
how some boards change the default in configs/ .

>>> And this imx8m_mem_map will be modified at runtime to get rid of
>>> optee
>>> memory. When OCRAM_S is added, the index used in enable_caches and
>>> dram_init need update as well.
>> I'm not sure I understand this. What kind of modification are you
>> talking about ? The DRAM entry offset should be determined
>> automatically, so there shouldn't be any need to hand-tune ad-hoc
>> offsets.
> 
> You also need below change, the index for DRAM1 is used in codes to
> help remove the OPTEE space from MMU table.
> 
> --- a/arch/arm/mach-imx/imx8m/soc.c
> +++ b/arch/arm/mach-imx/imx8m/soc.c
> @@ -167,10 +167,10 @@ void enable_caches(void)
>                   * please make sure that entry initial value matches
>                   * imx8m_mem_map for DRAM1
>                   */
> -               int entry = 5;
> +               int entry = 6;

OK, then this hard-coding of random offset is nasty and should be fixed.
For example iterate over imx8m_mem_map[] and detect entry which has PA 
 >= 0x40000000 , that's your DRAM.

>                  u64 attrs = imx8m_mem_map[entry].attrs;
>   
> -               while (i < CONFIG_NR_DRAM_BANKS && entry < 8) {
> +               while (i < CONFIG_NR_DRAM_BANKS && entry < 9) {
>                          if (gd->bd->bi_dram[i].start == 0)
>                                  break;
>                          imx8m_mem_map[entry].phys = gd->bd-
>> bi_dram[i].start;
> @@ -212,7 +212,7 @@ int dram_init(void)
>                  gd->ram_size = sdram_size;
>   
>          /* also update the SDRAM size in the mem_map used externally */
> -       imx8m_mem_map[5].size = sdram_size;
> +       imx8m_mem_map[6].size = sdram_size;
>   
>   #ifdef PHYS_SDRAM_2_SIZE
>          gd->ram_size += PHYS_SDRAM_2_SIZE;

[...]