[PATCH] ARM: dts: fix split memory bank for SSDK5440

[PATCH] ARM: dts: fix split memory bank for SSDK5440

[Kukjin Kim]

The size of memory bank should be under 256MB, because current
section size is 256MB on EXYNOS SoCs. This patch fixes it.

Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
 arch/arm/boot/dts/exynos5440-ssdk5440.dts |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos5440-ssdk5440.dts b/arch/arm/boot/dts/exynos5440-ssdk5440.dts
index 44d4d24..79d02cc 100644
--- a/arch/arm/boot/dts/exynos5440-ssdk5440.dts
+++ b/arch/arm/boot/dts/exynos5440-ssdk5440.dts
@@ -17,7 +17,14 @@
 	compatible = "samsung,ssdk5440", "samsung,exynos5440";
 
 	memory {
-		reg = <0x80000000 0x80000000>;
+		reg = <0x80000000 0x10000000
+		       0x90000000 0x10000000
+		       0xA0000000 0x10000000
+		       0xB0000000 0x10000000
+		       0xC0000000 0x10000000
+		       0xD0000000 0x10000000
+		       0xE0000000 0x10000000
+		       0xF0000000 0x10000000>;
 	};
 
 	chosen {
-- 
1.7.10.4

[PATCH] ARM: dts: fix split memory bank for SSDK5440

[Olof Johansson]

Hi,

On Thu, Dec 20, 2012 at 11:03 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> The size of memory bank should be under 256MB, because current
> section size is 256MB on EXYNOS SoCs. This patch fixes it.

This makes no sense. You don't have to split up memory ranges, the
code should be made to handle it instead.

What's the actual bug caused by this? The description is vague.


-Olof

[PATCH] ARM: dts: fix split memory bank for SSDK5440

[Tomasz Figa]

Hi Olof,

On Thursday 20 of December 2012 11:56:59 Olof Johansson wrote:
> Hi,
> 
> On Thu, Dec 20, 2012 at 11:03 AM, Kukjin Kim <kgene.kim@samsung.com> 
wrote:
> > The size of memory bank should be under 256MB, because current
> > section size is 256MB on EXYNOS SoCs. This patch fixes it.
> 
> This makes no sense. You don't have to split up memory ranges, the
> code should be made to handle it instead.

It's not Exynos code which causes the problem. Sparsemem initialization 
relies on the fact that initial amount of structures to described memory 
equals to maximum section size which is defined per arch (e.g. 
ARCH_EXYNOS).

> What's the actual bug caused by this? The description is vague.

The kernel panics early on NULL pointer dereference in memory 
initialization.

Best regards,
Tomasz Figa

[PATCH] ARM: dts: fix split memory bank for SSDK5440

[Subash Patel]

I would like to ask a question here. Do we need to have sparse even if 
the physical memory is contiguous? All the recent exynos machines come 
with physical banks without any holes, and I am thinking why not drop it 
and use flat mem instead. With LPAE these sections sizes wont be useful, 
and I dont like to keep different section sizes for different 
configurations. Any suggestions/opinions are very much helpful to me.

Regards,
Subash

On Thursday 20 December 2012 01:14 PM, Tomasz Figa wrote:
> Hi Olof,
>
> On Thursday 20 of December 2012 11:56:59 Olof Johansson wrote:
>> Hi,
>>
>> On Thu, Dec 20, 2012 at 11:03 AM, Kukjin Kim <kgene.kim@samsung.com>
> wrote:
>>> The size of memory bank should be under 256MB, because current
>>> section size is 256MB on EXYNOS SoCs. This patch fixes it.
>>
>> This makes no sense. You don't have to split up memory ranges, the
>> code should be made to handle it instead.
>
> It's not Exynos code which causes the problem. Sparsemem initialization
> relies on the fact that initial amount of structures to described memory
> equals to maximum section size which is defined per arch (e.g.
> ARCH_EXYNOS).
>
>> What's the actual bug caused by this? The description is vague.
>
> The kernel panics early on NULL pointer dereference in memory
> initialization.
>
> Best regards,
> Tomasz Figa
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH] ARM: dts: fix split memory bank for SSDK5440

[Tomasz Figa]

Hi Subash,

On Thursday 20 of December 2012 14:19:48 Subash Patel wrote:
> I would like to ask a question here. Do we need to have sparse even if
> the physical memory is contiguous? All the recent exynos machines come
> with physical banks without any holes, and I am thinking why not drop it
> and use flat mem instead. With LPAE these sections sizes wont be useful,
> and I dont like to keep different section sizes for different
> configurations. Any suggestions/opinions are very much helpful to me.

Since sparse memory is the only option available on Exynos currently in 
kernel configuration, I assume there was a reason for it. However I am not 
an expert in memory management, so please correct me if I am wrong.

Best regards,
Tomasz Figa

[PATCH] ARM: dts: fix split memory bank for SSDK5440

[Kukjin Kim]

Tomasz Figa wrote:
> 
> On Thursday 20 of December 2012 14:19:48 Subash Patel wrote:
> > I would like to ask a question here. Do we need to have sparse even if
> > the physical memory is contiguous? All the recent exynos machines come
> > with physical banks without any holes, and I am thinking why not drop it
> > and use flat mem instead. With LPAE these sections sizes wont be useful,
> > and I dont like to keep different section sizes for different
> > configurations. Any suggestions/opinions are very much helpful to me.
> 
> Since sparse memory is the only option available on Exynos currently in
> kernel configuration, I assume there was a reason for it. However I am not
> an expert in memory management, so please correct me if I am wrong.
> 
Hmm...yeah, as Subash said, it's no problem to disable sparsemem on EXYNOS
for now but memory hole can be existing on upcoming EXYNOS such as S5PV210
so keeping sparsemem would be better I think.

- Kukjin

[PATCH] ARM: dts: fix split memory bank for SSDK5440

[Russell King - ARM Linux]

On Thu, Dec 20, 2012 at 10:14:26PM +0100, Tomasz Figa wrote:
> Hi Olof,
> 
> On Thursday 20 of December 2012 11:56:59 Olof Johansson wrote:
> > Hi,
> > 
> > On Thu, Dec 20, 2012 at 11:03 AM, Kukjin Kim <kgene.kim@samsung.com> 
> wrote:
> > > The size of memory bank should be under 256MB, because current
> > > section size is 256MB on EXYNOS SoCs. This patch fixes it.
> > 
> > This makes no sense. You don't have to split up memory ranges, the
> > code should be made to handle it instead.
> 
> It's not Exynos code which causes the problem. Sparsemem initialization 
> relies on the fact that initial amount of structures to described memory 
> equals to maximum section size which is defined per arch (e.g. 
> ARCH_EXYNOS).
> 
> > What's the actual bug caused by this? The description is vague.
> 
> The kernel panics early on NULL pointer dereference in memory 
> initialization.

And of course the debugging information is included in the commit log so
that others can see what problem you're experiencing and make a decision
whether the proposed solution is the right one...

[PATCH] ARM: dts: fix split memory bank for SSDK5440

[Kukjin Kim]

Russell King - ARM Linux wrote:
> 

[...]

> >
> > > What's the actual bug caused by this? The description is vague.
> >
> > The kernel panics early on NULL pointer dereference in memory
> > initialization.
> 

(Cc'ed KyongHo Cho)

Yeah, correct. The size of memory bank should be under section size and
current section size is 256MiB on EXYNOS. So if we don't change the section
size, each memory bank should be under 256MiB. If not, as Tomasz said,
kernel panic happens in mem_init().

> And of course the debugging information is included in the commit log so
> that others can see what problem you're experiencing and make a decision
> whether the proposed solution is the right one...

Yeah, why not, I will add it in next version.

Thanks.

- Kukjin

[PATCH] ARM: dts: fix split memory bank for SSDK5440

[Olof Johansson]

On Thu, Dec 20, 2012 at 4:32 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Russell King - ARM Linux wrote:
>>
>
> [...]
>
>> >
>> > > What's the actual bug caused by this? The description is vague.
>> >
>> > The kernel panics early on NULL pointer dereference in memory
>> > initialization.
>>
>
> (Cc'ed KyongHo Cho)
>
> Yeah, correct. The size of memory bank should be under section size and
> current section size is 256MiB on EXYNOS. So if we don't change the section
> size, each memory bank should be under 256MiB. If not, as Tomasz said,
> kernel panic happens in mem_init().

That's a bug somewhere, the device tree should describe the hardware,
and not necessarily the massaged format that the kernel wants it in to
not trigger a bug.

So I don't want to see this patch merged, I want to see a proper fix
instead, please.

There are also already other exynos platforms and device trees with
more than 256MB in the memory node. Changing all of them this way
seems unreasonable.


-Olof

[PATCH] ARM: dts: fix split memory bank for SSDK5440

[Cho KyongHo]

> From: Olof Johansson [mailto:olof at lixom.net]
> Sent: Friday, December 21, 2012 9:36 AM
> 
> On Thu, Dec 20, 2012 at 4:32 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > Russell King - ARM Linux wrote:
> >>
> >
> > [...]
> >
> >> >
> >> > > What's the actual bug caused by this? The description is vague.
> >> >
> >> > The kernel panics early on NULL pointer dereference in memory
> >> > initialization.
> >>
> >
> > (Cc'ed KyongHo Cho)
> >
> > Yeah, correct. The size of memory bank should be under section size and
> > current section size is 256MiB on EXYNOS. So if we don't change the section
> > size, each memory bank should be under 256MiB. If not, as Tomasz said,
> > kernel panic happens in mem_init().
> 
> That's a bug somewhere, the device tree should describe the hardware,
> and not necessarily the massaged format that the kernel wants it in to
> not trigger a bug.
> 
> So I don't want to see this patch merged, I want to see a proper fix
> instead, please.
> 
> There are also already other exynos platforms and device trees with
> more than 256MB in the memory node. Changing all of them this way
> seems unreasonable.
> 
As Kukjin said earlier, the kernel panic may happen while gathering
memory statistics in mem_init()(arch/arm/mm/init.c). Russell King does not
agreed to modify mem_init() instead he told that a memory bank must not cross
section boundaries.