[U-Boot] of-platdata address map decode for 64-bits

[U-Boot] of-platdata address map decode for 64-bits

[Kever Yang]

Hi Simon,

     For my rk3399(64bit) dts, the address is described as below:
#address-cells = <2>;
reg = <0x0 0xfe320000 0x0 0x4000>;

not like 32-bit soc which reg address describe like:
reg = <0xfe320000 0x4000>;

     There should have some update for this dts decode, right?
like:
map_sysmem()
regmap_init_mem_platdata()

     Do you have a idea to update all these kind of function call?
It will be very great if you can send a patch for it.

Thanks,
- Kever

[U-Boot] of-platdata address map decode for 64-bits

[Simon Glass]

Hi Kever,

On 17 January 2017 at 23:37, Kever Yang <kever.yang@rock-chips.com> wrote:
> Hi Simon,
>
>     For my rk3399(64bit) dts, the address is described as below:
> #address-cells = <2>;
> reg = <0x0 0xfe320000 0x0 0x4000>;
>
> not like 32-bit soc which reg address describe like:
> reg = <0xfe320000 0x4000>;
>
>     There should have some update for this dts decode, right?
> like:
> map_sysmem()
> regmap_init_mem_platdata()
>
>     Do you have a idea to update all these kind of function call?
> It will be very great if you can send a patch for it.
>
> Thanks,
> - Kever
>

Yes I think so.

But first I'd like to understand why we need to use of-platdata on
this very modern SoC. Can you tell me the maximum SPL size you can
support on this SoC? I thought you had removed the 30KB limit?

Using of-platdata only saves 3-4KB.

Regards,
Simon

[U-Boot] of-platdata address map decode for 64-bits

[Kever Yang]

Hi Simon,

     For rk3399, the data for sdram driver in dts is big, I don't want 
to do the copy for it,
so I think use of-platdata is a good method, and for image size, its 
smaller, then it's sooner to load.

Thanks,
- Kever
On 01/26/2017 10:22 PM, Simon Glass wrote:
> Hi Kever,
>
> On 17 January 2017 at 23:37, Kever Yang <kever.yang@rock-chips.com> wrote:
>> Hi Simon,
>>
>>      For my rk3399(64bit) dts, the address is described as below:
>> #address-cells = <2>;
>> reg = <0x0 0xfe320000 0x0 0x4000>;
>>
>> not like 32-bit soc which reg address describe like:
>> reg = <0xfe320000 0x4000>;
>>
>>      There should have some update for this dts decode, right?
>> like:
>> map_sysmem()
>> regmap_init_mem_platdata()
>>
>>      Do you have a idea to update all these kind of function call?
>> It will be very great if you can send a patch for it.
>>
>> Thanks,
>> - Kever
>>
> Yes I think so.
>
> But first I'd like to understand why we need to use of-platdata on
> this very modern SoC. Can you tell me the maximum SPL size you can
> support on this SoC? I thought you had removed the 30KB limit?
>
> Using of-platdata only saves 3-4KB.
>
> Regards,
> Simon
>
>
>

[U-Boot] of-platdata address map decode for 64-bits

[Simon Glass]

Hi Kever,

On 4 February 2017 at 16:30, Kever Yang <kever.yang@rock-chips.com> wrote:
> Hi Simon,
>
>     For rk3399, the data for sdram driver in dts is big, I don't want to do
> the copy for it,

Are you referring to the call to fdtdec_get_int_array()? Don't you
have heaps of SRAM though? So why does it matter? If you are really
worried about it you could loop through the data word by word instead.
But why?

> so I think use of-platdata is a good method, and for image size, its
> smaller, then it's sooner to load.

Yes it reduces the image size, but only by a small amount. I have not
checked it on ARM64 but it should be around 5KB. I don't think it is
very important.

Regards,
Simon


>
> Thanks,
> - Kever
>
> On 01/26/2017 10:22 PM, Simon Glass wrote:
>>
>> Hi Kever,
>>
>> On 17 January 2017 at 23:37, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>
>>> Hi Simon,
>>>
>>>      For my rk3399(64bit) dts, the address is described as below:
>>> #address-cells = <2>;
>>> reg = <0x0 0xfe320000 0x0 0x4000>;
>>>
>>> not like 32-bit soc which reg address describe like:
>>> reg = <0xfe320000 0x4000>;
>>>
>>>      There should have some update for this dts decode, right?
>>> like:
>>> map_sysmem()
>>> regmap_init_mem_platdata()
>>>
>>>      Do you have a idea to update all these kind of function call?
>>> It will be very great if you can send a patch for it.
>>>
>>> Thanks,
>>> - Kever
>>>
>> Yes I think so.
>>
>> But first I'd like to understand why we need to use of-platdata on
>> this very modern SoC. Can you tell me the maximum SPL size you can
>> support on this SoC? I thought you had removed the 30KB limit?
>>
>> Using of-platdata only saves 3-4KB.
>>
>> Regards,
>> Simon
>>
>>
>>
>
>

[U-Boot] of-platdata address map decode for 64-bits

[Kever Yang]

Hi Simon,

On 02/08/2017 01:10 PM, Simon Glass wrote:
> Hi Kever,
>
> On 4 February 2017 at 16:30, Kever Yang <kever.yang@rock-chips.com> wrote:
>> Hi Simon,
>>
>>      For rk3399, the data for sdram driver in dts is big, I don't want to do
>> the copy for it,
> Are you referring to the call to fdtdec_get_int_array()? Don't you
> have heaps of SRAM though? So why does it matter? If you are really
> worried about it you could loop through the data word by word instead.

I don't understand how to "loop through the data word by word".
> But why?
>
>> so I think use of-platdata is a good method, and for image size, its
>> smaller, then it's sooner to load.
> Yes it reduces the image size, but only by a small amount. I have not
> checked it on ARM64 but it should be around 5KB. I don't think it is
> very important.

Here is why I insist on no more extra operation and extra code in SPL,
- The CPU in bootrom usually running in pretty low frequency, the copy 
may be slow;
- The eMMC in bootrom is running in pretty low frequency, the read is slow;
- Not like kernel or U-Boot, I don't think the dts values much for SPL,
     Most of driver used in SPL is one time init, and may totally 
different in
different SoC, including clock/cru driver, pinctrl/grf driver, dram 
driver, only
storage driver is shared in different SoC and different boot stage.
IMO, the SPL suppose to be simple, fast to init dram and load next stage 
image(if
we do not go back to bootrom).

For production, I have do many times to speed up the boot duration in 
different SoCs,
so I hope the upstream U-Boot for Rockchip SoC is not only for 
developers. We should
care about the size even if the size is enough for boot, the size design 
is not only consider
the boot, but also for some application in system suspend which dram 
suspend.

Thanks,
- Kever
>
> Regards,
> Simon
>
>
>> Thanks,
>> - Kever
>>
>> On 01/26/2017 10:22 PM, Simon Glass wrote:
>>> Hi Kever,
>>>
>>> On 17 January 2017 at 23:37, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>> Hi Simon,
>>>>
>>>>       For my rk3399(64bit) dts, the address is described as below:
>>>> #address-cells = <2>;
>>>> reg = <0x0 0xfe320000 0x0 0x4000>;
>>>>
>>>> not like 32-bit soc which reg address describe like:
>>>> reg = <0xfe320000 0x4000>;
>>>>
>>>>       There should have some update for this dts decode, right?
>>>> like:
>>>> map_sysmem()
>>>> regmap_init_mem_platdata()
>>>>
>>>>       Do you have a idea to update all these kind of function call?
>>>> It will be very great if you can send a patch for it.
>>>>
>>>> Thanks,
>>>> - Kever
>>>>
>>> Yes I think so.
>>>
>>> But first I'd like to understand why we need to use of-platdata on
>>> this very modern SoC. Can you tell me the maximum SPL size you can
>>> support on this SoC? I thought you had removed the 30KB limit?
>>>
>>> Using of-platdata only saves 3-4KB.
>>>
>>> Regards,
>>> Simon
>>>
>>>
>>>
>>
>
>

[U-Boot] of-platdata address map decode for 64-bits

[Simon Glass]

Hi Kever,

On 13 February 2017 at 01:13, Kever Yang <kever.yang@rock-chips.com> wrote:
> Hi Simon,
>
> On 02/08/2017 01:10 PM, Simon Glass wrote:
>>
>> Hi Kever,
>>
>> On 4 February 2017 at 16:30, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>
>>> Hi Simon,
>>>
>>>      For rk3399, the data for sdram driver in dts is big, I don't want to
>>> do
>>> the copy for it,
>>
>> Are you referring to the call to fdtdec_get_int_array()? Don't you
>> have heaps of SRAM though? So why does it matter? If you are really
>> worried about it you could loop through the data word by word instead.
>
>
> I don't understand how to "loop through the data word by word".

u32 *ptr;
u32 *reg = <address of memory controller>

ptr = fdt_getprop(...., &len);
len /= sizeof(u32);
for (i = 0; i < len)
   writel(fdt32_to_cpu(ptr[i]), reg[i]);


>>
>> But why?
>>
>>> so I think use of-platdata is a good method, and for image size, its
>>> smaller, then it's sooner to load.
>>
>> Yes it reduces the image size, but only by a small amount. I have not
>> checked it on ARM64 but it should be around 5KB. I don't think it is
>> very important.
>
>
> Here is why I insist on no more extra operation and extra code in SPL,
> - The CPU in bootrom usually running in pretty low frequency, the copy may
> be slow;
> - The eMMC in bootrom is running in pretty low frequency, the read is slow;
> - Not like kernel or U-Boot, I don't think the dts values much for SPL,
>     Most of driver used in SPL is one time init, and may totally different
> in
> different SoC, including clock/cru driver, pinctrl/grf driver, dram driver,
> only
> storage driver is shared in different SoC and different boot stage.
> IMO, the SPL suppose to be simple, fast to init dram and load next stage
> image(if
> we do not go back to bootrom).
>
> For production, I have do many times to speed up the boot duration in
> different SoCs,
> so I hope the upstream U-Boot for Rockchip SoC is not only for developers.
> We should
> care about the size even if the size is enough for boot, the size design is
> not only consider
> the boot, but also for some application in system suspend which dram
> suspend.

I am very much in agreement about your focus on boot time. I have no
interest in having a rockchip upstream U-Boot that is anything less
than the best in terms of boot time.

However, does it really matter? Can you benchmark the boot time with
and without of-platdata and show me that the difference is
significant?

So often I see optimisation of things that don't really matter.
Remember that of-platdata is going to be a pretty significant burden
on your development, so it needs to be worth it.

That said, in the meantime, let's continue with this path
(of-platdata). We can decide whether to enable it or disable it based
on the results.

Regards,
Simon


>
> Thanks,
> - Kever
>
>>
>> Regards,
>> Simon
>>
>>
>>> Thanks,
>>> - Kever
>>>
>>> On 01/26/2017 10:22 PM, Simon Glass wrote:
>>>>
>>>> Hi Kever,
>>>>
>>>> On 17 January 2017 at 23:37, Kever Yang <kever.yang@rock-chips.com>
>>>> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>>       For my rk3399(64bit) dts, the address is described as below:
>>>>> #address-cells = <2>;
>>>>> reg = <0x0 0xfe320000 0x0 0x4000>;
>>>>>
>>>>> not like 32-bit soc which reg address describe like:
>>>>> reg = <0xfe320000 0x4000>;
>>>>>
>>>>>       There should have some update for this dts decode, right?
>>>>> like:
>>>>> map_sysmem()
>>>>> regmap_init_mem_platdata()
>>>>>
>>>>>       Do you have a idea to update all these kind of function call?
>>>>> It will be very great if you can send a patch for it.
>>>>>
>>>>> Thanks,
>>>>> - Kever
>>>>>
>>>> Yes I think so.
>>>>
>>>> But first I'd like to understand why we need to use of-platdata on
>>>> this very modern SoC. Can you tell me the maximum SPL size you can
>>>> support on this SoC? I thought you had removed the 30KB limit?
>>>>
>>>> Using of-platdata only saves 3-4KB.
>>>>
>>>> Regards,
>>>> Simon
>>>>
>>>>
>>>>
>>>
>>
>>
>
>

[U-Boot] of-platdata address map decode for 64-bits

[Kever Yang]

Hi Simon,

On 02/14/2017 01:31 PM, Simon Glass wrote:
> Hi Kever,
>
> On 13 February 2017 at 01:13, Kever Yang <kever.yang@rock-chips.com> wrote:
>> Hi Simon,
>>
>> On 02/08/2017 01:10 PM, Simon Glass wrote:
>>> Hi Kever,
>>>
>>> On 4 February 2017 at 16:30, Kever Yang <kever.yang@rock-chips.com> wrote:
>>>> Hi Simon,
>>>>
>>>>       For rk3399, the data for sdram driver in dts is big, I don't want to
>>>> do
>>>> the copy for it,
>>> Are you referring to the call to fdtdec_get_int_array()? Don't you
>>> have heaps of SRAM though? So why does it matter? If you are really
>>> worried about it you could loop through the data word by word instead.
>>
>> I don't understand how to "loop through the data word by word".
> u32 *ptr;
> u32 *reg = <address of memory controller>
>
> ptr = fdt_getprop(...., &len);
> len /= sizeof(u32);
> for (i = 0; i < len)
>     writel(fdt32_to_cpu(ptr[i]), reg[i]);

I think the fdt32_to_cpu() may be one thing cost CPU, it's done by the tool
when of-platadata is enabled while CPU needs to do it when of-platdata 
is not enabled.

>
>
>>> But why?
>>>
>>>> so I think use of-platdata is a good method, and for image size, its
>>>> smaller, then it's sooner to load.
>>> Yes it reduces the image size, but only by a small amount. I have not
>>> checked it on ARM64 but it should be around 5KB. I don't think it is
>>> very important.
>>
>> Here is why I insist on no more extra operation and extra code in SPL,
>> - The CPU in bootrom usually running in pretty low frequency, the copy may
>> be slow;
>> - The eMMC in bootrom is running in pretty low frequency, the read is slow;
>> - Not like kernel or U-Boot, I don't think the dts values much for SPL,
>>      Most of driver used in SPL is one time init, and may totally different
>> in
>> different SoC, including clock/cru driver, pinctrl/grf driver, dram driver,
>> only
>> storage driver is shared in different SoC and different boot stage.
>> IMO, the SPL suppose to be simple, fast to init dram and load next stage
>> image(if
>> we do not go back to bootrom).
>>
>> For production, I have do many times to speed up the boot duration in
>> different SoCs,
>> so I hope the upstream U-Boot for Rockchip SoC is not only for developers.
>> We should
>> care about the size even if the size is enough for boot, the size design is
>> not only consider
>> the boot, but also for some application in system suspend which dram
>> suspend.
> I am very much in agreement about your focus on boot time. I have no
> interest in having a rockchip upstream U-Boot that is anything less
> than the best in terms of boot time.
>
> However, does it really matter? Can you benchmark the boot time with
> and without of-platdata and show me that the difference is
> significant?
>
> So often I see optimisation of things that don't really matter.
> Remember that of-platdata is going to be a pretty significant burden
> on your development, so it needs to be worth it.
>
> That said, in the meantime, let's continue with this path
> (of-platdata). We can decide whether to enable it or disable it based
> on the results.

Here is my test result:
I enable a 24M counter at print the counter at rk3399 arch_cpu_init(),
then we can compare the current value of this counter when we enter
U-Boot:
- with of-platdata, the value is about 0xe577c7;
- without of-platdata, the value is about 0xf730ba,
This result in about 48ms difference, not including the time difference
when loading SPL, there should also have some difference especially when
we using SPI boot.

The CPU is running at 400MHz at bootrom, if we speedup the cpu to 600M,
the value will be:
- with of-platdata, counter value about 0xd1f0c4,
- without of-platdata, counter value about 0xd84c8d,
The difference is about 17ms.

Anyway, I will support both of-platdata and non of-platdata feature in 
my next
version patch, and with of-platdata enabled by default.

Thanks,
- Kever
>
> Regards,
> Simon
>
>
>> Thanks,
>> - Kever
>>
>>> Regards,
>>> Simon
>>>
>>>
>>>> Thanks,
>>>> - Kever
>>>>
>>>> On 01/26/2017 10:22 PM, Simon Glass wrote:
>>>>> Hi Kever,
>>>>>
>>>>> On 17 January 2017 at 23:37, Kever Yang <kever.yang@rock-chips.com>
>>>>> wrote:
>>>>>> Hi Simon,
>>>>>>
>>>>>>        For my rk3399(64bit) dts, the address is described as below:
>>>>>> #address-cells = <2>;
>>>>>> reg = <0x0 0xfe320000 0x0 0x4000>;
>>>>>>
>>>>>> not like 32-bit soc which reg address describe like:
>>>>>> reg = <0xfe320000 0x4000>;
>>>>>>
>>>>>>        There should have some update for this dts decode, right?
>>>>>> like:
>>>>>> map_sysmem()
>>>>>> regmap_init_mem_platdata()
>>>>>>
>>>>>>        Do you have a idea to update all these kind of function call?
>>>>>> It will be very great if you can send a patch for it.
>>>>>>
>>>>>> Thanks,
>>>>>> - Kever
>>>>>>
>>>>> Yes I think so.
>>>>>
>>>>> But first I'd like to understand why we need to use of-platdata on
>>>>> this very modern SoC. Can you tell me the maximum SPL size you can
>>>>> support on this SoC? I thought you had removed the 30KB limit?
>>>>>
>>>>> Using of-platdata only saves 3-4KB.
>>>>>
>>>>> Regards,
>>>>> Simon
>>>>>
>>>>>
>>>>>
>>>
>>
>
>

[U-Boot] of-platdata address map decode for 64-bits

[Simon Glass]

Hi Kever,

On 21 February 2017 at 06:08, Kever Yang <kever.yang@rock-chips.com> wrote:
> Hi Simon,
>
> On 02/14/2017 01:31 PM, Simon Glass wrote:
>>
>> Hi Kever,
>>
>> On 13 February 2017 at 01:13, Kever Yang <kever.yang@rock-chips.com>
>> wrote:
>>>
>>> Hi Simon,
>>>
>>> On 02/08/2017 01:10 PM, Simon Glass wrote:
>>>>
>>>> Hi Kever,
>>>>
>>>> On 4 February 2017 at 16:30, Kever Yang <kever.yang@rock-chips.com>
>>>> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>>       For rk3399, the data for sdram driver in dts is big, I don't want
>>>>> to
>>>>> do
>>>>> the copy for it,
>>>>
>>>> Are you referring to the call to fdtdec_get_int_array()? Don't you
>>>> have heaps of SRAM though? So why does it matter? If you are really
>>>> worried about it you could loop through the data word by word instead.
>>>
>>>
>>> I don't understand how to "loop through the data word by word".
>>
>> u32 *ptr;
>> u32 *reg = <address of memory controller>
>>
>> ptr = fdt_getprop(...., &len);
>> len /= sizeof(u32);
>> for (i = 0; i < len)
>>     writel(fdt32_to_cpu(ptr[i]), reg[i]);
>
>
> I think the fdt32_to_cpu() may be one thing cost CPU, it's done by the tool
> when of-platadata is enabled while CPU needs to do it when of-platdata is
> not enabled.
>
>
>>
>>
>>>> But why?
>>>>
>>>>> so I think use of-platdata is a good method, and for image size, its
>>>>> smaller, then it's sooner to load.
>>>>
>>>> Yes it reduces the image size, but only by a small amount. I have not
>>>> checked it on ARM64 but it should be around 5KB. I don't think it is
>>>> very important.
>>>
>>>
>>> Here is why I insist on no more extra operation and extra code in SPL,
>>> - The CPU in bootrom usually running in pretty low frequency, the copy
>>> may
>>> be slow;
>>> - The eMMC in bootrom is running in pretty low frequency, the read is
>>> slow;
>>> - Not like kernel or U-Boot, I don't think the dts values much for SPL,
>>>      Most of driver used in SPL is one time init, and may totally
>>> different
>>> in
>>> different SoC, including clock/cru driver, pinctrl/grf driver, dram
>>> driver,
>>> only
>>> storage driver is shared in different SoC and different boot stage.
>>> IMO, the SPL suppose to be simple, fast to init dram and load next stage
>>> image(if
>>> we do not go back to bootrom).
>>>
>>> For production, I have do many times to speed up the boot duration in
>>> different SoCs,
>>> so I hope the upstream U-Boot for Rockchip SoC is not only for
>>> developers.
>>> We should
>>> care about the size even if the size is enough for boot, the size design
>>> is
>>> not only consider
>>> the boot, but also for some application in system suspend which dram
>>> suspend.
>>
>> I am very much in agreement about your focus on boot time. I have no
>> interest in having a rockchip upstream U-Boot that is anything less
>> than the best in terms of boot time.
>>
>> However, does it really matter? Can you benchmark the boot time with
>> and without of-platdata and show me that the difference is
>> significant?
>>
>> So often I see optimisation of things that don't really matter.
>> Remember that of-platdata is going to be a pretty significant burden
>> on your development, so it needs to be worth it.
>>
>> That said, in the meantime, let's continue with this path
>> (of-platdata). We can decide whether to enable it or disable it based
>> on the results.
>
>
> Here is my test result:
> I enable a 24M counter at print the counter at rk3399 arch_cpu_init(),
> then we can compare the current value of this counter when we enter
> U-Boot:
> - with of-platdata, the value is about 0xe577c7;
> - without of-platdata, the value is about 0xf730ba,
> This result in about 48ms difference, not including the time difference
> when loading SPL, there should also have some difference especially when
> we using SPI boot.
>
> The CPU is running at 400MHz at bootrom, if we speedup the cpu to 600M,
> the value will be:
> - with of-platdata, counter value about 0xd1f0c4,
> - without of-platdata, counter value about 0xd84c8d,
> The difference is about 17ms.
>
> Anyway, I will support both of-platdata and non of-platdata feature in my
> next
> version patch, and with of-platdata enabled by default.

This is excellent information, thank you.

I suspect we can speed this up and reduce the difference to a new ms,
but let's go with your plan. Supporting both is fine. Now that we know
the tie difference we have something to work with.

BTW I suspect for some platforms we will want to increase to maximum
speed in SPL, assuming there is space to allow programming the PMIC,
etc.

Regards,
Simon