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

Marek Vasut
2021-02-27T12:57:59Z

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; …