[PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Heiko Thiery]

It was observed that enabling additional DM modules the configured
malloc value is not sufficient. So lets increase the value.

Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
---
v2:
 - add a more proper commit message to explan why the value was increased

 configs/kontron_pitx_imx8m_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
index 76430213e3..30c3586937 100644
--- a/configs/kontron_pitx_imx8m_defconfig
+++ b/configs/kontron_pitx_imx8m_defconfig
@@ -2,6 +2,7 @@ CONFIG_ARM=y
 CONFIG_ARCH_IMX8M=y
 CONFIG_SYS_TEXT_BASE=0x40200000
 CONFIG_SYS_MALLOC_LEN=0x600000
+CONFIG_SYS_MALLOC_F_LEN=0x10000
 CONFIG_SPL_GPIO=y
 CONFIG_SPL_LIBCOMMON_SUPPORT=y
 CONFIG_SPL_LIBGENERIC_SUPPORT=y
-- 
2.30.2

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Fabio Estevam]

Hi Heiko,

On Mon, Mar 21, 2022 at 11:26 AM Heiko Thiery <heiko.thiery@gmail.com> wrote:
>
> It was observed that enabling additional DM modules the configured
> malloc value is not sufficient. So lets increase the value.
>
> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> ---
> v2:
>  - add a more proper commit message to explan why the value was increased

Thanks for the rework:

Reviewed-by: Fabio Estevam <festevam@denx.de>

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Heinrich Schuchardt]

On 3/21/22 15:26, Heiko Thiery wrote:
> It was observed that enabling additional DM modules the configured
> malloc value is not sufficient. So lets increase the value.
>
> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> ---
> v2:
>   - add a more proper commit message to explan why the value was increased
>
>   configs/kontron_pitx_imx8m_defconfig | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> index 76430213e3..30c3586937 100644
> --- a/configs/kontron_pitx_imx8m_defconfig
> +++ b/configs/kontron_pitx_imx8m_defconfig
> @@ -2,6 +2,7 @@ CONFIG_ARM=y
>   CONFIG_ARCH_IMX8M=y
>   CONFIG_SYS_TEXT_BASE=0x40200000
>   CONFIG_SYS_MALLOC_LEN=0x600000
> +CONFIG_SYS_MALLOC_F_LEN=0x10000

@Heiko
Should we really adjust this on board level? Won't we have the same
problem on all imx8m boards?

Why don't you change the default for all i.mx8 boards in /Kconfig?

@Tom, @Simon
Shouldn't we replace the default of 0x400 by 0x2000 generally?

Best regards

Heinrich

>   CONFIG_SPL_GPIO=y
>   CONFIG_SPL_LIBCOMMON_SUPPORT=y
>   CONFIG_SPL_LIBGENERIC_SUPPORT=y

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]

On Tue, Mar 22, 2022 at 10:25:12AM +0100, Heinrich Schuchardt wrote:
> On 3/21/22 15:26, Heiko Thiery wrote:
> > It was observed that enabling additional DM modules the configured
> > malloc value is not sufficient. So lets increase the value.
> > 
> > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > ---
> > v2:
> >   - add a more proper commit message to explan why the value was increased
> > 
> >   configs/kontron_pitx_imx8m_defconfig | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> > index 76430213e3..30c3586937 100644
> > --- a/configs/kontron_pitx_imx8m_defconfig
> > +++ b/configs/kontron_pitx_imx8m_defconfig
> > @@ -2,6 +2,7 @@ CONFIG_ARM=y
> >   CONFIG_ARCH_IMX8M=y
> >   CONFIG_SYS_TEXT_BASE=0x40200000
> >   CONFIG_SYS_MALLOC_LEN=0x600000
> > +CONFIG_SYS_MALLOC_F_LEN=0x10000
> 
> @Heiko
> Should we really adjust this on board level? Won't we have the same
> problem on all imx8m boards?
> 
> Why don't you change the default for all i.mx8 boards in /Kconfig?
> 
> @Tom, @Simon
> Shouldn't we replace the default of 0x400 by 0x2000 generally?

That's well more than the default for imx8 of 0x2000 and even for the
default for sandbox of 0x4000, does it need to be that huge?  Or was
that the "I'm sick of this problem" value?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Heiko Thiery]

Hi Tom,


Am Di., 22. März 2022 um 13:25 Uhr schrieb Tom Rini <trini@konsulko.com>:
>
> On Tue, Mar 22, 2022 at 10:25:12AM +0100, Heinrich Schuchardt wrote:
> > On 3/21/22 15:26, Heiko Thiery wrote:
> > > It was observed that enabling additional DM modules the configured
> > > malloc value is not sufficient. So lets increase the value.
> > >
> > > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > > ---
> > > v2:
> > >   - add a more proper commit message to explan why the value was increased
> > >
> > >   configs/kontron_pitx_imx8m_defconfig | 1 +
> > >   1 file changed, 1 insertion(+)
> > >
> > > diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> > > index 76430213e3..30c3586937 100644
> > > --- a/configs/kontron_pitx_imx8m_defconfig
> > > +++ b/configs/kontron_pitx_imx8m_defconfig
> > > @@ -2,6 +2,7 @@ CONFIG_ARM=y
> > >   CONFIG_ARCH_IMX8M=y
> > >   CONFIG_SYS_TEXT_BASE=0x40200000
> > >   CONFIG_SYS_MALLOC_LEN=0x600000
> > > +CONFIG_SYS_MALLOC_F_LEN=0x10000
> >
> > @Heiko
> > Should we really adjust this on board level? Won't we have the same
> > problem on all imx8m boards?
> >
> > Why don't you change the default for all i.mx8 boards in /Kconfig?
> >
> > @Tom, @Simon
> > Shouldn't we replace the default of 0x400 by 0x2000 generally?
>
> That's well more than the default for imx8 of 0x2000 and even for the
> default for sandbox of 0x4000, does it need to be that huge?  Or was
> that the "I'm sick of this problem" value?
>

I did test a pending patch that enables a DM clock driver for the
imx8mq. With that patch a huge amount of memory seems to be needed for
all that clock infrastructure.

-- 
Heiko

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]

On Tue, Mar 22, 2022 at 01:34:34PM +0100, Heiko Thiery wrote:
> Hi Tom,
> 
> 
> Am Di., 22. März 2022 um 13:25 Uhr schrieb Tom Rini <trini@konsulko.com>:
> >
> > On Tue, Mar 22, 2022 at 10:25:12AM +0100, Heinrich Schuchardt wrote:
> > > On 3/21/22 15:26, Heiko Thiery wrote:
> > > > It was observed that enabling additional DM modules the configured
> > > > malloc value is not sufficient. So lets increase the value.
> > > >
> > > > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > > > ---
> > > > v2:
> > > >   - add a more proper commit message to explan why the value was increased
> > > >
> > > >   configs/kontron_pitx_imx8m_defconfig | 1 +
> > > >   1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> > > > index 76430213e3..30c3586937 100644
> > > > --- a/configs/kontron_pitx_imx8m_defconfig
> > > > +++ b/configs/kontron_pitx_imx8m_defconfig
> > > > @@ -2,6 +2,7 @@ CONFIG_ARM=y
> > > >   CONFIG_ARCH_IMX8M=y
> > > >   CONFIG_SYS_TEXT_BASE=0x40200000
> > > >   CONFIG_SYS_MALLOC_LEN=0x600000
> > > > +CONFIG_SYS_MALLOC_F_LEN=0x10000
> > >
> > > @Heiko
> > > Should we really adjust this on board level? Won't we have the same
> > > problem on all imx8m boards?
> > >
> > > Why don't you change the default for all i.mx8 boards in /Kconfig?
> > >
> > > @Tom, @Simon
> > > Shouldn't we replace the default of 0x400 by 0x2000 generally?
> >
> > That's well more than the default for imx8 of 0x2000 and even for the
> > default for sandbox of 0x4000, does it need to be that huge?  Or was
> > that the "I'm sick of this problem" value?
> >
> 
> I did test a pending patch that enables a DM clock driver for the
> imx8mq. With that patch a huge amount of memory seems to be needed for
> all that clock infrastructure.

OK, then we should adjust upwards the default for imx8 in the toplevel
Kconfig.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Adam Ford]

On Wed, Mar 23, 2022 at 8:14 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Tue, Mar 22, 2022 at 01:34:34PM +0100, Heiko Thiery wrote:
> > Hi Tom,
> >
> >
> > Am Di., 22. März 2022 um 13:25 Uhr schrieb Tom Rini <trini@konsulko.com>:
> > >
> > > On Tue, Mar 22, 2022 at 10:25:12AM +0100, Heinrich Schuchardt wrote:
> > > > On 3/21/22 15:26, Heiko Thiery wrote:
> > > > > It was observed that enabling additional DM modules the configured
> > > > > malloc value is not sufficient. So lets increase the value.
> > > > >
> > > > > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > > > > ---
> > > > > v2:
> > > > >   - add a more proper commit message to explan why the value was increased
> > > > >
> > > > >   configs/kontron_pitx_imx8m_defconfig | 1 +
> > > > >   1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> > > > > index 76430213e3..30c3586937 100644
> > > > > --- a/configs/kontron_pitx_imx8m_defconfig
> > > > > +++ b/configs/kontron_pitx_imx8m_defconfig
> > > > > @@ -2,6 +2,7 @@ CONFIG_ARM=y
> > > > >   CONFIG_ARCH_IMX8M=y
> > > > >   CONFIG_SYS_TEXT_BASE=0x40200000
> > > > >   CONFIG_SYS_MALLOC_LEN=0x600000
> > > > > +CONFIG_SYS_MALLOC_F_LEN=0x10000
> > > >
> > > > @Heiko
> > > > Should we really adjust this on board level? Won't we have the same
> > > > problem on all imx8m boards?
> > > >
> > > > Why don't you change the default for all i.mx8 boards in /Kconfig?
> > > >
> > > > @Tom, @Simon
> > > > Shouldn't we replace the default of 0x400 by 0x2000 generally?
> > >
> > > That's well more than the default for imx8 of 0x2000 and even for the
> > > default for sandbox of 0x4000, does it need to be that huge?  Or was
> > > that the "I'm sick of this problem" value?
> > >
> >
> > I did test a pending patch that enables a DM clock driver for the
> > imx8mq. With that patch a huge amount of memory seems to be needed for
> > all that clock infrastructure.
>
> OK, then we should adjust upwards the default for imx8 in the toplevel
> Kconfig.

I have been using 0x10000 for a while without issue on both an imx8mm
and imx8mn.  When I used that value, it was more of a desire to
overshoot the minimal requirements so as features are added, we don't
have to keep adjusting the memory.

I support making it the platform default.

adam
>
> --
> Tom

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Simon Glass]

Hi Heinrich,

On Tue, 22 Mar 2022 at 03:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 3/21/22 15:26, Heiko Thiery wrote:
> > It was observed that enabling additional DM modules the configured
> > malloc value is not sufficient. So lets increase the value.
> >
> > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > ---
> > v2:
> >   - add a more proper commit message to explan why the value was increased
> >
> >   configs/kontron_pitx_imx8m_defconfig | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> > index 76430213e3..30c3586937 100644
> > --- a/configs/kontron_pitx_imx8m_defconfig
> > +++ b/configs/kontron_pitx_imx8m_defconfig
> > @@ -2,6 +2,7 @@ CONFIG_ARM=y
> >   CONFIG_ARCH_IMX8M=y
> >   CONFIG_SYS_TEXT_BASE=0x40200000
> >   CONFIG_SYS_MALLOC_LEN=0x600000
> > +CONFIG_SYS_MALLOC_F_LEN=0x10000
>
> @Heiko
> Should we really adjust this on board level? Won't we have the same
> problem on all imx8m boards?
>
> Why don't you change the default for all i.mx8 boards in /Kconfig?
>
> @Tom, @Simon
> Shouldn't we replace the default of 0x400 by 0x2000 generally?

I don't think that is a good idea. That is a lot of memory! Many
platforms don't need that much.

I wonder what is driving this large amount. Is it pinctrl?

Anyway, changing it for imx8 seem right to me, as Tom says.

Regards,
Simon

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Heiko Thiery]

Hi Simon,

Am Mi., 23. März 2022 um 19:04 Uhr schrieb Simon Glass <sjg@chromium.org>:
>
> Hi Heinrich,
>
> On Tue, 22 Mar 2022 at 03:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 3/21/22 15:26, Heiko Thiery wrote:
> > > It was observed that enabling additional DM modules the configured
> > > malloc value is not sufficient. So lets increase the value.
> > >
> > > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > > ---
> > > v2:
> > >   - add a more proper commit message to explan why the value was increased
> > >
> > >   configs/kontron_pitx_imx8m_defconfig | 1 +
> > >   1 file changed, 1 insertion(+)
> > >
> > > diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> > > index 76430213e3..30c3586937 100644
> > > --- a/configs/kontron_pitx_imx8m_defconfig
> > > +++ b/configs/kontron_pitx_imx8m_defconfig
> > > @@ -2,6 +2,7 @@ CONFIG_ARM=y
> > >   CONFIG_ARCH_IMX8M=y
> > >   CONFIG_SYS_TEXT_BASE=0x40200000
> > >   CONFIG_SYS_MALLOC_LEN=0x600000
> > > +CONFIG_SYS_MALLOC_F_LEN=0x10000
> >
> > @Heiko
> > Should we really adjust this on board level? Won't we have the same
> > problem on all imx8m boards?
> >
> > Why don't you change the default for all i.mx8 boards in /Kconfig?
> >
> > @Tom, @Simon
> > Shouldn't we replace the default of 0x400 by 0x2000 generally?
>
> I don't think that is a good idea. That is a lot of memory! Many
> platforms don't need that much.
>
> I wonder what is driving this large amount. Is it pinctrl?

The increase comes from the introduction of a clock driver for the
imx8mq platform.

>
> Anyway, changing it for imx8 seem right to me, as Tom says.
>
> Regards,
> Simon

-- 
Heiko

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Sean Anderson]

On 3/23/22 2:26 PM, Heiko Thiery wrote:
> Hi Simon,
> 
> Am Mi., 23. März 2022 um 19:04 Uhr schrieb Simon Glass <sjg@chromium.org>:
>>
>> Hi Heinrich,
>>
>> On Tue, 22 Mar 2022 at 03:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>
>>> On 3/21/22 15:26, Heiko Thiery wrote:
>>>> It was observed that enabling additional DM modules the configured
>>>> malloc value is not sufficient. So lets increase the value.
>>>>
>>>> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
>>>> ---
>>>> v2:
>>>>    - add a more proper commit message to explan why the value was increased
>>>>
>>>>    configs/kontron_pitx_imx8m_defconfig | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
>>>> index 76430213e3..30c3586937 100644
>>>> --- a/configs/kontron_pitx_imx8m_defconfig
>>>> +++ b/configs/kontron_pitx_imx8m_defconfig
>>>> @@ -2,6 +2,7 @@ CONFIG_ARM=y
>>>>    CONFIG_ARCH_IMX8M=y
>>>>    CONFIG_SYS_TEXT_BASE=0x40200000
>>>>    CONFIG_SYS_MALLOC_LEN=0x600000
>>>> +CONFIG_SYS_MALLOC_F_LEN=0x10000
>>>
>>> @Heiko
>>> Should we really adjust this on board level? Won't we have the same
>>> problem on all imx8m boards?
>>>
>>> Why don't you change the default for all i.mx8 boards in /Kconfig?
>>>
>>> @Tom, @Simon
>>> Shouldn't we replace the default of 0x400 by 0x2000 generally?
>>
>> I don't think that is a good idea. That is a lot of memory! Many
>> platforms don't need that much.
>>
>> I wonder what is driving this large amount. Is it pinctrl?
> 
> The increase comes from the introduction of a clock driver for the
> imx8mq platform.

Yes, the problem is that CCF creates a udevice+clk+private data for
every clock. This runs about 150-200 bytes per clock on a 64-bit
platform. In addition, many physical clocks are modeled as several
logical clocks plus a composite. This means a platform with maybe
20-30 physical clocks can easily allocate 10k-20k to create
the clock tree.

--Sean

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2854 bytes --]

On Wed, Mar 23, 2022 at 02:33:12PM -0400, Sean Anderson wrote:
> On 3/23/22 2:26 PM, Heiko Thiery wrote:
> > Hi Simon,
> > 
> > Am Mi., 23. März 2022 um 19:04 Uhr schrieb Simon Glass <sjg@chromium.org>:
> > > 
> > > Hi Heinrich,
> > > 
> > > On Tue, 22 Mar 2022 at 03:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > 
> > > > On 3/21/22 15:26, Heiko Thiery wrote:
> > > > > It was observed that enabling additional DM modules the configured
> > > > > malloc value is not sufficient. So lets increase the value.
> > > > > 
> > > > > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > > > > ---
> > > > > v2:
> > > > >    - add a more proper commit message to explan why the value was increased
> > > > > 
> > > > >    configs/kontron_pitx_imx8m_defconfig | 1 +
> > > > >    1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> > > > > index 76430213e3..30c3586937 100644
> > > > > --- a/configs/kontron_pitx_imx8m_defconfig
> > > > > +++ b/configs/kontron_pitx_imx8m_defconfig
> > > > > @@ -2,6 +2,7 @@ CONFIG_ARM=y
> > > > >    CONFIG_ARCH_IMX8M=y
> > > > >    CONFIG_SYS_TEXT_BASE=0x40200000
> > > > >    CONFIG_SYS_MALLOC_LEN=0x600000
> > > > > +CONFIG_SYS_MALLOC_F_LEN=0x10000
> > > > 
> > > > @Heiko
> > > > Should we really adjust this on board level? Won't we have the same
> > > > problem on all imx8m boards?
> > > > 
> > > > Why don't you change the default for all i.mx8 boards in /Kconfig?
> > > > 
> > > > @Tom, @Simon
> > > > Shouldn't we replace the default of 0x400 by 0x2000 generally?
> > > 
> > > I don't think that is a good idea. That is a lot of memory! Many
> > > platforms don't need that much.
> > > 
> > > I wonder what is driving this large amount. Is it pinctrl?
> > 
> > The increase comes from the introduction of a clock driver for the
> > imx8mq platform.
> 
> Yes, the problem is that CCF creates a udevice+clk+private data for
> every clock. This runs about 150-200 bytes per clock on a 64-bit
> platform. In addition, many physical clocks are modeled as several
> logical clocks plus a composite. This means a platform with maybe
> 20-30 physical clocks can easily allocate 10k-20k to create
> the clock tree.

And to cycle back to what I mentioned on IRC at some point, I disagree
with the notion many platforms don't need "that" might.  Yes, 0x10000
tends to be on the higher end, but there's larger still boards, but we
should likely set a new default X if DM (and similar, default Y if
SPL_DM, and Z for TPL) and bump the boards up that are below that but
more than current default, to that default.  A value of 0x400 is
unreasonably small for DM and likely anything less than 0x4000 (what
sandbox picks) is going to be problematic.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Simon Glass]

Hi Sean,

On Wed, 23 Mar 2022 at 12:33, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 3/23/22 2:26 PM, Heiko Thiery wrote:
> > Hi Simon,
> >
> > Am Mi., 23. März 2022 um 19:04 Uhr schrieb Simon Glass <sjg@chromium.org>:
> >>
> >> Hi Heinrich,
> >>
> >> On Tue, 22 Mar 2022 at 03:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>
> >>> On 3/21/22 15:26, Heiko Thiery wrote:
> >>>> It was observed that enabling additional DM modules the configured
> >>>> malloc value is not sufficient. So lets increase the value.
> >>>>
> >>>> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> >>>> ---
> >>>> v2:
> >>>>    - add a more proper commit message to explan why the value was increased
> >>>>
> >>>>    configs/kontron_pitx_imx8m_defconfig | 1 +
> >>>>    1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> >>>> index 76430213e3..30c3586937 100644
> >>>> --- a/configs/kontron_pitx_imx8m_defconfig
> >>>> +++ b/configs/kontron_pitx_imx8m_defconfig
> >>>> @@ -2,6 +2,7 @@ CONFIG_ARM=y
> >>>>    CONFIG_ARCH_IMX8M=y
> >>>>    CONFIG_SYS_TEXT_BASE=0x40200000
> >>>>    CONFIG_SYS_MALLOC_LEN=0x600000
> >>>> +CONFIG_SYS_MALLOC_F_LEN=0x10000
> >>>
> >>> @Heiko
> >>> Should we really adjust this on board level? Won't we have the same
> >>> problem on all imx8m boards?
> >>>
> >>> Why don't you change the default for all i.mx8 boards in /Kconfig?
> >>>
> >>> @Tom, @Simon
> >>> Shouldn't we replace the default of 0x400 by 0x2000 generally?
> >>
> >> I don't think that is a good idea. That is a lot of memory! Many
> >> platforms don't need that much.
> >>
> >> I wonder what is driving this large amount. Is it pinctrl?
> >
> > The increase comes from the introduction of a clock driver for the
> > imx8mq platform.
>
> Yes, the problem is that CCF creates a udevice+clk+private data for
> every clock. This runs about 150-200 bytes per clock on a 64-bit
> platform. In addition, many physical clocks are modeled as several
> logical clocks plus a composite. This means a platform with maybe
> 20-30 physical clocks can easily allocate 10k-20k to create
> the clock tree.

With the new DM tag support we could move uncommon fields in struct
udevice to tags, such as parent_plat_, uclass_plat_, driver_data,
uclass_priv_, parent_priv_, dma_offset. It would add a small amount of
code but save data. We could call this DM_TINY_DATA and It might save
64 bytes per device.

Also the #ifdef CONFIG_DEVRES should use CONFIG_IS_ENABLED(DEVRES) so
that devres_head is not included in SPL.

Regards,
Simon

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Simon Glass]

Hi Tom,

On Wed, 23 Mar 2022 at 12:49, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Mar 23, 2022 at 02:33:12PM -0400, Sean Anderson wrote:
> > On 3/23/22 2:26 PM, Heiko Thiery wrote:
> > > Hi Simon,
> > >
> > > Am Mi., 23. März 2022 um 19:04 Uhr schrieb Simon Glass <sjg@chromium.org>:
> > > >
> > > > Hi Heinrich,
> > > >
> > > > On Tue, 22 Mar 2022 at 03:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >
> > > > > On 3/21/22 15:26, Heiko Thiery wrote:
> > > > > > It was observed that enabling additional DM modules the configured
> > > > > > malloc value is not sufficient. So lets increase the value.
> > > > > >
> > > > > > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > > > > > ---
> > > > > > v2:
> > > > > >    - add a more proper commit message to explan why the value was increased
> > > > > >
> > > > > >    configs/kontron_pitx_imx8m_defconfig | 1 +
> > > > > >    1 file changed, 1 insertion(+)
> > > > > >
> > > > > > diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> > > > > > index 76430213e3..30c3586937 100644
> > > > > > --- a/configs/kontron_pitx_imx8m_defconfig
> > > > > > +++ b/configs/kontron_pitx_imx8m_defconfig
> > > > > > @@ -2,6 +2,7 @@ CONFIG_ARM=y
> > > > > >    CONFIG_ARCH_IMX8M=y
> > > > > >    CONFIG_SYS_TEXT_BASE=0x40200000
> > > > > >    CONFIG_SYS_MALLOC_LEN=0x600000
> > > > > > +CONFIG_SYS_MALLOC_F_LEN=0x10000
> > > > >
> > > > > @Heiko
> > > > > Should we really adjust this on board level? Won't we have the same
> > > > > problem on all imx8m boards?
> > > > >
> > > > > Why don't you change the default for all i.mx8 boards in /Kconfig?
> > > > >
> > > > > @Tom, @Simon
> > > > > Shouldn't we replace the default of 0x400 by 0x2000 generally?
> > > >
> > > > I don't think that is a good idea. That is a lot of memory! Many
> > > > platforms don't need that much.
> > > >
> > > > I wonder what is driving this large amount. Is it pinctrl?
> > >
> > > The increase comes from the introduction of a clock driver for the
> > > imx8mq platform.
> >
> > Yes, the problem is that CCF creates a udevice+clk+private data for
> > every clock. This runs about 150-200 bytes per clock on a 64-bit
> > platform. In addition, many physical clocks are modeled as several
> > logical clocks plus a composite. This means a platform with maybe
> > 20-30 physical clocks can easily allocate 10k-20k to create
> > the clock tree.
>
> And to cycle back to what I mentioned on IRC at some point, I disagree
> with the notion many platforms don't need "that" might.  Yes, 0x10000
> tends to be on the higher end, but there's larger still boards, but we
> should likely set a new default X if DM (and similar, default Y if
> SPL_DM, and Z for TPL) and bump the boards up that are below that but
> more than current default, to that default.  A value of 0x400 is
> unreasonably small for DM and likely anything less than 0x4000 (what
> sandbox picks) is going to be problematic.

Perhaps we can set the default for something that doesn't use CCF, to
start? It seems that CCF can really blow things up, so we could have a
larger default when that is used.

Regards,
Simon

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3789 bytes --]

On Wed, Mar 23, 2022 at 12:53:59PM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 23 Mar 2022 at 12:49, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Mar 23, 2022 at 02:33:12PM -0400, Sean Anderson wrote:
> > > On 3/23/22 2:26 PM, Heiko Thiery wrote:
> > > > Hi Simon,
> > > >
> > > > Am Mi., 23. März 2022 um 19:04 Uhr schrieb Simon Glass <sjg@chromium.org>:
> > > > >
> > > > > Hi Heinrich,
> > > > >
> > > > > On Tue, 22 Mar 2022 at 03:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >
> > > > > > On 3/21/22 15:26, Heiko Thiery wrote:
> > > > > > > It was observed that enabling additional DM modules the configured
> > > > > > > malloc value is not sufficient. So lets increase the value.
> > > > > > >
> > > > > > > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > > > > > > ---
> > > > > > > v2:
> > > > > > >    - add a more proper commit message to explan why the value was increased
> > > > > > >
> > > > > > >    configs/kontron_pitx_imx8m_defconfig | 1 +
> > > > > > >    1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> > > > > > > index 76430213e3..30c3586937 100644
> > > > > > > --- a/configs/kontron_pitx_imx8m_defconfig
> > > > > > > +++ b/configs/kontron_pitx_imx8m_defconfig
> > > > > > > @@ -2,6 +2,7 @@ CONFIG_ARM=y
> > > > > > >    CONFIG_ARCH_IMX8M=y
> > > > > > >    CONFIG_SYS_TEXT_BASE=0x40200000
> > > > > > >    CONFIG_SYS_MALLOC_LEN=0x600000
> > > > > > > +CONFIG_SYS_MALLOC_F_LEN=0x10000
> > > > > >
> > > > > > @Heiko
> > > > > > Should we really adjust this on board level? Won't we have the same
> > > > > > problem on all imx8m boards?
> > > > > >
> > > > > > Why don't you change the default for all i.mx8 boards in /Kconfig?
> > > > > >
> > > > > > @Tom, @Simon
> > > > > > Shouldn't we replace the default of 0x400 by 0x2000 generally?
> > > > >
> > > > > I don't think that is a good idea. That is a lot of memory! Many
> > > > > platforms don't need that much.
> > > > >
> > > > > I wonder what is driving this large amount. Is it pinctrl?
> > > >
> > > > The increase comes from the introduction of a clock driver for the
> > > > imx8mq platform.
> > >
> > > Yes, the problem is that CCF creates a udevice+clk+private data for
> > > every clock. This runs about 150-200 bytes per clock on a 64-bit
> > > platform. In addition, many physical clocks are modeled as several
> > > logical clocks plus a composite. This means a platform with maybe
> > > 20-30 physical clocks can easily allocate 10k-20k to create
> > > the clock tree.
> >
> > And to cycle back to what I mentioned on IRC at some point, I disagree
> > with the notion many platforms don't need "that" might.  Yes, 0x10000
> > tends to be on the higher end, but there's larger still boards, but we
> > should likely set a new default X if DM (and similar, default Y if
> > SPL_DM, and Z for TPL) and bump the boards up that are below that but
> > more than current default, to that default.  A value of 0x400 is
> > unreasonably small for DM and likely anything less than 0x4000 (what
> > sandbox picks) is going to be problematic.
> 
> Perhaps we can set the default for something that doesn't use CCF, to
> start? It seems that CCF can really blow things up, so we could have a
> larger default when that is used.

I think this is just the example of the day of the more general issue.
There's 369 boards setting a non-default value for
CONFIG_SYS_MALLOC_F_LEN.  A bunch of them are overriding their already
non-0x400 default provided in ./Kconfig.  More attention and thought
about what to do here is needed.  And probably bumping the default for
DM cases up.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Sean Anderson]

On 3/23/22 2:53 PM, Simon Glass wrote:
> Hi Sean,
> 
> On Wed, 23 Mar 2022 at 12:33, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 3/23/22 2:26 PM, Heiko Thiery wrote:
>>> Hi Simon,
>>>
>>> Am Mi., 23. März 2022 um 19:04 Uhr schrieb Simon Glass <sjg@chromium.org>:
>>>>
>>>> Hi Heinrich,
>>>>
>>>> On Tue, 22 Mar 2022 at 03:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>
>>>>> On 3/21/22 15:26, Heiko Thiery wrote:
>>>>>> It was observed that enabling additional DM modules the configured
>>>>>> malloc value is not sufficient. So lets increase the value.
>>>>>>
>>>>>> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
>>>>>> ---
>>>>>> v2:
>>>>>>     - add a more proper commit message to explan why the value was increased
>>>>>>
>>>>>>     configs/kontron_pitx_imx8m_defconfig | 1 +
>>>>>>     1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
>>>>>> index 76430213e3..30c3586937 100644
>>>>>> --- a/configs/kontron_pitx_imx8m_defconfig
>>>>>> +++ b/configs/kontron_pitx_imx8m_defconfig
>>>>>> @@ -2,6 +2,7 @@ CONFIG_ARM=y
>>>>>>     CONFIG_ARCH_IMX8M=y
>>>>>>     CONFIG_SYS_TEXT_BASE=0x40200000
>>>>>>     CONFIG_SYS_MALLOC_LEN=0x600000
>>>>>> +CONFIG_SYS_MALLOC_F_LEN=0x10000
>>>>>
>>>>> @Heiko
>>>>> Should we really adjust this on board level? Won't we have the same
>>>>> problem on all imx8m boards?
>>>>>
>>>>> Why don't you change the default for all i.mx8 boards in /Kconfig?
>>>>>
>>>>> @Tom, @Simon
>>>>> Shouldn't we replace the default of 0x400 by 0x2000 generally?
>>>>
>>>> I don't think that is a good idea. That is a lot of memory! Many
>>>> platforms don't need that much.
>>>>
>>>> I wonder what is driving this large amount. Is it pinctrl?
>>>
>>> The increase comes from the introduction of a clock driver for the
>>> imx8mq platform.
>>
>> Yes, the problem is that CCF creates a udevice+clk+private data for
>> every clock. This runs about 150-200 bytes per clock on a 64-bit
>> platform. In addition, many physical clocks are modeled as several
>> logical clocks plus a composite. This means a platform with maybe
>> 20-30 physical clocks can easily allocate 10k-20k to create
>> the clock tree.
> 
> With the new DM tag support we could move uncommon fields in struct
> udevice to tags, such as parent_plat_, uclass_plat_, driver_data,
> uclass_priv_, parent_priv_, dma_offset. It would add a small amount of
> code but save data. We could call this DM_TINY_DATA and It might save
> 64 bytes per device.
> 
> Also the #ifdef CONFIG_DEVRES should use CONFIG_IS_ENABLED(DEVRES) so
> that devres_head is not included in SPL.

Personally, I think we could get away with a much smaller amount of data
per-clock, and not make every clock a separate device. The primary reason
each CCF clock gets its own device is that we cache things like clock
rates. Because of this, we need to keep track of children so we can
invalidate their rates correctly. But caching rates is probably not that
much faster than just recalculating (especially since most of the time
it's just a division). We also don't try and find the best way to achieve
a clock speed like Linux does. If we don't cache clock rates, we don't
need to keep track of a clock's children.

Further, we could likely also eliminate tracking enable/disable counts.
We don't have fine-grained low power states like Linux does, so generally
when someone disables a clock we're getting in the way by trying to keep
track of the count.

This is all a rather big change, so we're stuck with the status quo for
now, but it's the direction I would like to move in going ahead. I know
Marek would rather just not create as many clocks in SPL/pre-reloc.

--Sean

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Simon Glass]

Hi Tom,

On Wed, 23 Mar 2022 at 12:59, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Mar 23, 2022 at 12:53:59PM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 23 Mar 2022 at 12:49, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Mar 23, 2022 at 02:33:12PM -0400, Sean Anderson wrote:
> > > > On 3/23/22 2:26 PM, Heiko Thiery wrote:
> > > > > Hi Simon,
> > > > >
> > > > > Am Mi., 23. März 2022 um 19:04 Uhr schrieb Simon Glass <sjg@chromium.org>:
> > > > > >
> > > > > > Hi Heinrich,
> > > > > >
> > > > > > On Tue, 22 Mar 2022 at 03:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > >
> > > > > > > On 3/21/22 15:26, Heiko Thiery wrote:
> > > > > > > > It was observed that enabling additional DM modules the configured
> > > > > > > > malloc value is not sufficient. So lets increase the value.
> > > > > > > >
> > > > > > > > Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> > > > > > > > ---
> > > > > > > > v2:
> > > > > > > >    - add a more proper commit message to explan why the value was increased
> > > > > > > >
> > > > > > > >    configs/kontron_pitx_imx8m_defconfig | 1 +
> > > > > > > >    1 file changed, 1 insertion(+)
> > > > > > > >
> > > > > > > > diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> > > > > > > > index 76430213e3..30c3586937 100644
> > > > > > > > --- a/configs/kontron_pitx_imx8m_defconfig
> > > > > > > > +++ b/configs/kontron_pitx_imx8m_defconfig
> > > > > > > > @@ -2,6 +2,7 @@ CONFIG_ARM=y
> > > > > > > >    CONFIG_ARCH_IMX8M=y
> > > > > > > >    CONFIG_SYS_TEXT_BASE=0x40200000
> > > > > > > >    CONFIG_SYS_MALLOC_LEN=0x600000
> > > > > > > > +CONFIG_SYS_MALLOC_F_LEN=0x10000
> > > > > > >
> > > > > > > @Heiko
> > > > > > > Should we really adjust this on board level? Won't we have the same
> > > > > > > problem on all imx8m boards?
> > > > > > >
> > > > > > > Why don't you change the default for all i.mx8 boards in /Kconfig?
> > > > > > >
> > > > > > > @Tom, @Simon
> > > > > > > Shouldn't we replace the default of 0x400 by 0x2000 generally?
> > > > > >
> > > > > > I don't think that is a good idea. That is a lot of memory! Many
> > > > > > platforms don't need that much.
> > > > > >
> > > > > > I wonder what is driving this large amount. Is it pinctrl?
> > > > >
> > > > > The increase comes from the introduction of a clock driver for the
> > > > > imx8mq platform.
> > > >
> > > > Yes, the problem is that CCF creates a udevice+clk+private data for
> > > > every clock. This runs about 150-200 bytes per clock on a 64-bit
> > > > platform. In addition, many physical clocks are modeled as several
> > > > logical clocks plus a composite. This means a platform with maybe
> > > > 20-30 physical clocks can easily allocate 10k-20k to create
> > > > the clock tree.
> > >
> > > And to cycle back to what I mentioned on IRC at some point, I disagree
> > > with the notion many platforms don't need "that" might.  Yes, 0x10000
> > > tends to be on the higher end, but there's larger still boards, but we
> > > should likely set a new default X if DM (and similar, default Y if
> > > SPL_DM, and Z for TPL) and bump the boards up that are below that but
> > > more than current default, to that default.  A value of 0x400 is
> > > unreasonably small for DM and likely anything less than 0x4000 (what
> > > sandbox picks) is going to be problematic.
> >
> > Perhaps we can set the default for something that doesn't use CCF, to
> > start? It seems that CCF can really blow things up, so we could have a
> > larger default when that is used.
>
> I think this is just the example of the day of the more general issue.
> There's 369 boards setting a non-default value for
> CONFIG_SYS_MALLOC_F_LEN.  A bunch of them are overriding their already
> non-0x400 default provided in ./Kconfig.  More attention and thought
> about what to do here is needed.  And probably bumping the default for
> DM cases up.

Yes, makes sense. Increasing the default from 0x400 should help,
although if something is tight then it won't boot.

We sort-of have a way to discover the effective config values with the
moveconfig tool now, and the -i option allows looking up values that
are implied (although I have not tried it with a 'hex' config.

Also, if there is plenty of RAM on imx8 (or some other SoC), then we
may as well use it, at least by default.

I'm avoiding any new endeavours while so much is outstanding, so I
hope someone else can step up to look at this.

Regards,
Simon

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Simon Glass]

Hi Sean,

On Wed, 23 Mar 2022 at 13:19, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 3/23/22 2:53 PM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Wed, 23 Mar 2022 at 12:33, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> On 3/23/22 2:26 PM, Heiko Thiery wrote:
> >>> Hi Simon,
> >>>
> >>> Am Mi., 23. März 2022 um 19:04 Uhr schrieb Simon Glass <sjg@chromium.org>:
> >>>>
> >>>> Hi Heinrich,
> >>>>
> >>>> On Tue, 22 Mar 2022 at 03:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>
> >>>>> On 3/21/22 15:26, Heiko Thiery wrote:
> >>>>>> It was observed that enabling additional DM modules the configured
> >>>>>> malloc value is not sufficient. So lets increase the value.
> >>>>>>
> >>>>>> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> >>>>>> ---
> >>>>>> v2:
> >>>>>>     - add a more proper commit message to explan why the value was increased
> >>>>>>
> >>>>>>     configs/kontron_pitx_imx8m_defconfig | 1 +
> >>>>>>     1 file changed, 1 insertion(+)
> >>>>>>
> >>>>>> diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> >>>>>> index 76430213e3..30c3586937 100644
> >>>>>> --- a/configs/kontron_pitx_imx8m_defconfig
> >>>>>> +++ b/configs/kontron_pitx_imx8m_defconfig
> >>>>>> @@ -2,6 +2,7 @@ CONFIG_ARM=y
> >>>>>>     CONFIG_ARCH_IMX8M=y
> >>>>>>     CONFIG_SYS_TEXT_BASE=0x40200000
> >>>>>>     CONFIG_SYS_MALLOC_LEN=0x600000
> >>>>>> +CONFIG_SYS_MALLOC_F_LEN=0x10000
> >>>>>
> >>>>> @Heiko
> >>>>> Should we really adjust this on board level? Won't we have the same
> >>>>> problem on all imx8m boards?
> >>>>>
> >>>>> Why don't you change the default for all i.mx8 boards in /Kconfig?
> >>>>>
> >>>>> @Tom, @Simon
> >>>>> Shouldn't we replace the default of 0x400 by 0x2000 generally?
> >>>>
> >>>> I don't think that is a good idea. That is a lot of memory! Many
> >>>> platforms don't need that much.
> >>>>
> >>>> I wonder what is driving this large amount. Is it pinctrl?
> >>>
> >>> The increase comes from the introduction of a clock driver for the
> >>> imx8mq platform.
> >>
> >> Yes, the problem is that CCF creates a udevice+clk+private data for
> >> every clock. This runs about 150-200 bytes per clock on a 64-bit
> >> platform. In addition, many physical clocks are modeled as several
> >> logical clocks plus a composite. This means a platform with maybe
> >> 20-30 physical clocks can easily allocate 10k-20k to create
> >> the clock tree.
> >
> > With the new DM tag support we could move uncommon fields in struct
> > udevice to tags, such as parent_plat_, uclass_plat_, driver_data,
> > uclass_priv_, parent_priv_, dma_offset. It would add a small amount of
> > code but save data. We could call this DM_TINY_DATA and It might save
> > 64 bytes per device.
> >
> > Also the #ifdef CONFIG_DEVRES should use CONFIG_IS_ENABLED(DEVRES) so
> > that devres_head is not included in SPL.
>
> Personally, I think we could get away with a much smaller amount of data
> per-clock, and not make every clock a separate device. The primary reason
> each CCF clock gets its own device is that we cache things like clock
> rates. Because of this, we need to keep track of children so we can
> invalidate their rates correctly. But caching rates is probably not that
> much faster than just recalculating (especially since most of the time
> it's just a division). We also don't try and find the best way to achieve
> a clock speed like Linux does. If we don't cache clock rates, we don't
> need to keep track of a clock's children.
>
> Further, we could likely also eliminate tracking enable/disable counts.
> We don't have fine-grained low power states like Linux does, so generally
> when someone disables a clock we're getting in the way by trying to keep
> track of the count.
>
> This is all a rather big change, so we're stuck with the status quo for
> now, but it's the direction I would like to move in going ahead. I know
> Marek would rather just not create as many clocks in SPL/pre-reloc.

This all makes sense. But updating 'struct udevice' would benefit
every subsystem, particular arm64. The struct is just too damn big.

Regards,
Simon

Re: [PATCH v2] board: kontron: increase the CONFIG_SYS_MALLOC_F_LEN

[Adam Ford]

On Wed, Mar 23, 2022 at 2:19 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> On 3/23/22 2:53 PM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Wed, 23 Mar 2022 at 12:33, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> On 3/23/22 2:26 PM, Heiko Thiery wrote:
> >>> Hi Simon,
> >>>
> >>> Am Mi., 23. März 2022 um 19:04 Uhr schrieb Simon Glass <sjg@chromium.org>:
> >>>>
> >>>> Hi Heinrich,
> >>>>
> >>>> On Tue, 22 Mar 2022 at 03:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>
> >>>>> On 3/21/22 15:26, Heiko Thiery wrote:
> >>>>>> It was observed that enabling additional DM modules the configured
> >>>>>> malloc value is not sufficient. So lets increase the value.
> >>>>>>
> >>>>>> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
> >>>>>> ---
> >>>>>> v2:
> >>>>>>     - add a more proper commit message to explan why the value was increased
> >>>>>>
> >>>>>>     configs/kontron_pitx_imx8m_defconfig | 1 +
> >>>>>>     1 file changed, 1 insertion(+)
> >>>>>>
> >>>>>> diff --git a/configs/kontron_pitx_imx8m_defconfig b/configs/kontron_pitx_imx8m_defconfig
> >>>>>> index 76430213e3..30c3586937 100644
> >>>>>> --- a/configs/kontron_pitx_imx8m_defconfig
> >>>>>> +++ b/configs/kontron_pitx_imx8m_defconfig
> >>>>>> @@ -2,6 +2,7 @@ CONFIG_ARM=y
> >>>>>>     CONFIG_ARCH_IMX8M=y
> >>>>>>     CONFIG_SYS_TEXT_BASE=0x40200000
> >>>>>>     CONFIG_SYS_MALLOC_LEN=0x600000
> >>>>>> +CONFIG_SYS_MALLOC_F_LEN=0x10000
> >>>>>
> >>>>> @Heiko
> >>>>> Should we really adjust this on board level? Won't we have the same
> >>>>> problem on all imx8m boards?
> >>>>>
> >>>>> Why don't you change the default for all i.mx8 boards in /Kconfig?
> >>>>>
> >>>>> @Tom, @Simon
> >>>>> Shouldn't we replace the default of 0x400 by 0x2000 generally?
> >>>>
> >>>> I don't think that is a good idea. That is a lot of memory! Many
> >>>> platforms don't need that much.
> >>>>
> >>>> I wonder what is driving this large amount. Is it pinctrl?
> >>>
> >>> The increase comes from the introduction of a clock driver for the
> >>> imx8mq platform.
> >>
> >> Yes, the problem is that CCF creates a udevice+clk+private data for
> >> every clock. This runs about 150-200 bytes per clock on a 64-bit
> >> platform. In addition, many physical clocks are modeled as several
> >> logical clocks plus a composite. This means a platform with maybe
> >> 20-30 physical clocks can easily allocate 10k-20k to create
> >> the clock tree.
> >
> > With the new DM tag support we could move uncommon fields in struct
> > udevice to tags, such as parent_plat_, uclass_plat_, driver_data,
> > uclass_priv_, parent_priv_, dma_offset. It would add a small amount of
> > code but save data. We could call this DM_TINY_DATA and It might save
> > 64 bytes per device.
> >
> > Also the #ifdef CONFIG_DEVRES should use CONFIG_IS_ENABLED(DEVRES) so
> > that devres_head is not included in SPL.
>
> Personally, I think we could get away with a much smaller amount of data
> per-clock, and not make every clock a separate device. The primary reason
> each CCF clock gets its own device is that we cache things like clock
> rates. Because of this, we need to keep track of children so we can
> invalidate their rates correctly. But caching rates is probably not that
> much faster than just recalculating (especially since most of the time
> it's just a division). We also don't try and find the best way to achieve
> a clock speed like Linux does. If we don't cache clock rates, we don't
> need to keep track of a clock's children.
>
> Further, we could likely also eliminate tracking enable/disable counts.
> We don't have fine-grained low power states like Linux does, so generally
> when someone disables a clock we're getting in the way by trying to keep
> track of the count.
>
> This is all a rather big change, so we're stuck with the status quo for
> now, but it's the direction I would like to move in going ahead. I know
> Marek would rather just not create as many clocks in SPL/pre-reloc.

We have config options to enable/disable peripherals.  For the imx8m
CCF, what if we inserted a series of ifdefs for the various
peripherals.  If peripheral-X is configured, enable the corresponding
clocks.  It doesn't necessarily make sense to me to load a bunch of
clocks in SPL if the corresponding peripherals are not used, but in
U-Boot they might be.  It would make the CCF a bit more ugly, but it
could help conserve some RAM in SPL/TPL.

adam
>
> --Sean
>