[PATCH 0/4] arm64: Hi6220: enable CPU idle states

[PATCH 0/4] arm64: Hi6220: enable CPU idle states

[Leo Yan]

This patch series is to enable CPU idle states for Hi6220.

Hi6220 uses PSCIv0.2 compliance interface, so directly use ARM's generic
CPUIdle driver. Patch 1 is to reserve memory regions so make sure MCU can
work well to handle power controlling; Patch 2/3 enable sp804 timer as
broadcast timer during idle states; Patch 4 registers CPU power down state
and cluster power down state.


Leo Yan (4):
  arm64: dts: Reserve memory regions for hi6220
  arm64: Kconfig: select sp804 timer for ARCH_HISI
  arm64: dts: add sp804 timer node for Hi6220
  arm64: dts: enable idle states for Hi6220

 arch/arm64/Kconfig.platforms                   |  1 +
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++---
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi      | 40 ++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Leo Yan]

On Hi6220, below memory regions in DDR have specific purpose:

  0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
  0x06df,f000 - 0x06df,ffff: For mailbox message data;
  0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
  0x3e00,0000 - 0x3fff,ffff: For OP-TEE.

This patch reserves these memory regions in DT.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index e36a539..e3f4cb3 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
@@ -7,9 +7,6 @@
 
 /dts-v1/;
 
-/*Reserved 1MB memory for MCU*/
-/memreserve/ 0x05e00000 0x00100000;
-
 #include "hi6220.dtsi"
 
 / {
@@ -24,8 +21,19 @@
 		stdout-path = "serial0:115200n8";
 	};
 
+	/*
+	 * Reserve below regions from memory node:
+	 *
+	 *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
+	 *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
+	 *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
+	 *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
+	 */
 	memory at 0 {
 		device_type = "memory";
-		reg = <0x0 0x0 0x0 0x40000000>;
+		reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
+		      <0x00000000 0x05f00000 0x00000000 0x00eff000>,
+		      <0x00000000 0x06e00000 0x00000000 0x0060f000>,
+		      <0x00000000 0x07410000 0x00000000 0x36bf0000>;
 	};
 };
-- 
1.9.1

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Rob Herring]

On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@linaro.org> wrote:
> On Hi6220, below memory regions in DDR have specific purpose:
>
>   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
>   0x06df,f000 - 0x06df,ffff: For mailbox message data;
>   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
>   0x3e00,0000 - 0x3fff,ffff: For OP-TEE.
>
> This patch reserves these memory regions in DT.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> index e36a539..e3f4cb3 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> @@ -7,9 +7,6 @@
>
>  /dts-v1/;
>
> -/*Reserved 1MB memory for MCU*/
> -/memreserve/ 0x05e00000 0x00100000;
> -

Why does memreserve not work for you? You can have multiple entries.

>  #include "hi6220.dtsi"
>
>  / {
> @@ -24,8 +21,19 @@
>                 stdout-path = "serial0:115200n8";
>         };
>
> +       /*
> +        * Reserve below regions from memory node:
> +        *
> +        *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
> +        *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
> +        *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
> +        *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
> +        */
>         memory at 0 {
>                 device_type = "memory";
> -               reg = <0x0 0x0 0x0 0x40000000>;
> +               reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> +                     <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> +                     <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> +                     <0x00000000 0x07410000 0x00000000 0x36bf0000>;

No, don't do this. Please use memreserve or reserved-memory binding[1]
or combination of both. Probably reserved-memory if you need the
kernel to access some of these regions.

Rob

[1] Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Mark Rutland]

On Fri, Oct 09, 2015 at 08:17:16AM -0500, Rob Herring wrote:
> On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@linaro.org> wrote:
> > On Hi6220, below memory regions in DDR have specific purpose:
> >
> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> >   0x06df,f000 - 0x06df,ffff: For mailbox message data;
> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> >   0x3e00,0000 - 0x3fff,ffff: For OP-TEE.
> >
> > This patch reserves these memory regions in DT.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > index e36a539..e3f4cb3 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > @@ -7,9 +7,6 @@
> >
> >  /dts-v1/;
> >
> > -/*Reserved 1MB memory for MCU*/
> > -/memreserve/ 0x05e00000 0x00100000;
> > -
> 
> Why does memreserve not work for you? You can have multiple entries.
> 
> >  #include "hi6220.dtsi"
> >
> >  / {
> > @@ -24,8 +21,19 @@
> >                 stdout-path = "serial0:115200n8";
> >         };
> >
> > +       /*
> > +        * Reserve below regions from memory node:
> > +        *
> > +        *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
> > +        *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
> > +        *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
> > +        *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
> > +        */
> >         memory at 0 {
> >                 device_type = "memory";
> > -               reg = <0x0 0x0 0x0 0x40000000>;
> > +               reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> > +                     <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> > +                     <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> > +                     <0x00000000 0x07410000 0x00000000 0x36bf0000>;
> 
> No, don't do this. Please use memreserve or reserved-memory binding[1]
> or combination of both. Probably reserved-memory if you need the
> kernel to access some of these regions.

I disagree at least for those portions owned by the secure world. The
kernel shouldn't map those at all, so memreserve isn't appropriate. That
covers OP-TEE and the MCU firmware regions, and I'd expec the EFI memory
map to not list those as available to the kernel.

For the mailbox memory reserved-memory should be OK.

Thanks,
Mark.

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Rob Herring]

On Fri, Oct 9, 2015 at 8:30 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Oct 09, 2015 at 08:17:16AM -0500, Rob Herring wrote:
>> On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@linaro.org> wrote:
>> > On Hi6220, below memory regions in DDR have specific purpose:
>> >
>> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
>> >   0x06df,f000 - 0x06df,ffff: For mailbox message data;
>> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
>> >   0x3e00,0000 - 0x3fff,ffff: For OP-TEE.
>> >
>> > This patch reserves these memory regions in DT.
>> >
>> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> > ---
>> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
>> >  1 file changed, 12 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
>> > index e36a539..e3f4cb3 100644
>> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
>> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
>> > @@ -7,9 +7,6 @@
>> >
>> >  /dts-v1/;
>> >
>> > -/*Reserved 1MB memory for MCU*/
>> > -/memreserve/ 0x05e00000 0x00100000;
>> > -
>>
>> Why does memreserve not work for you? You can have multiple entries.
>>
>> >  #include "hi6220.dtsi"
>> >
>> >  / {
>> > @@ -24,8 +21,19 @@
>> >                 stdout-path = "serial0:115200n8";
>> >         };
>> >
>> > +       /*
>> > +        * Reserve below regions from memory node:
>> > +        *
>> > +        *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
>> > +        *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
>> > +        *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
>> > +        *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
>> > +        */
>> >         memory at 0 {
>> >                 device_type = "memory";
>> > -               reg = <0x0 0x0 0x0 0x40000000>;
>> > +               reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
>> > +                     <0x00000000 0x05f00000 0x00000000 0x00eff000>,
>> > +                     <0x00000000 0x06e00000 0x00000000 0x0060f000>,
>> > +                     <0x00000000 0x07410000 0x00000000 0x36bf0000>;
>>
>> No, don't do this. Please use memreserve or reserved-memory binding[1]
>> or combination of both. Probably reserved-memory if you need the
>> kernel to access some of these regions.
>
> I disagree at least for those portions owned by the secure world. The
> kernel shouldn't map those at all, so memreserve isn't appropriate. That
> covers OP-TEE and the MCU firmware regions, and I'd expec the EFI memory
> map to not list those as available to the kernel.

I'm fine carving out the beginning or end, but otherwise think memory
should correspond to the physical memory. We have a way to describe
holes to keep out, so we should use them. If secure world uses the DT,
then it would either want to know its region in memory or add the DT
data to say what it is using. We need that to be easy to find or easy
to set, respectively. The size secure world needs could vary as well.

The fact that the kernel maps the memory is the kernel's problem, not
a DT problem.

>
> For the mailbox memory reserved-memory should be OK.

That only gets us from 4 regions to 3.

Rob

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Leo Yan]

Hi Rob,

On Fri, Oct 09, 2015 at 08:50:13AM -0500, Rob Herring wrote:
> On Fri, Oct 9, 2015 at 8:30 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Fri, Oct 09, 2015 at 08:17:16AM -0500, Rob Herring wrote:
> >> On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@linaro.org> wrote:
> >> > On Hi6220, below memory regions in DDR have specific purpose:
> >> >
> >> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> >> >   0x06df,f000 - 0x06df,ffff: For mailbox message data;
> >> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> >> >   0x3e00,0000 - 0x3fff,ffff: For OP-TEE.
> >> >
> >> > This patch reserves these memory regions in DT.
> >> >
> >> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> >> > ---
> >> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
> >> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >> >
> >> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> >> > index e36a539..e3f4cb3 100644
> >> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> >> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> >> > @@ -7,9 +7,6 @@
> >> >
> >> >  /dts-v1/;
> >> >
> >> > -/*Reserved 1MB memory for MCU*/
> >> > -/memreserve/ 0x05e00000 0x00100000;
> >> > -
> >>
> >> Why does memreserve not work for you? You can have multiple entries.
> >>
> >> >  #include "hi6220.dtsi"
> >> >
> >> >  / {
> >> > @@ -24,8 +21,19 @@
> >> >                 stdout-path = "serial0:115200n8";
> >> >         };
> >> >
> >> > +       /*
> >> > +        * Reserve below regions from memory node:
> >> > +        *
> >> > +        *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
> >> > +        *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
> >> > +        *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
> >> > +        *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
> >> > +        */
> >> >         memory at 0 {
> >> >                 device_type = "memory";
> >> > -               reg = <0x0 0x0 0x0 0x40000000>;
> >> > +               reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> >> > +                     <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> >> > +                     <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> >> > +                     <0x00000000 0x07410000 0x00000000 0x36bf0000>;
> >>
> >> No, don't do this. Please use memreserve or reserved-memory binding[1]
> >> or combination of both. Probably reserved-memory if you need the
> >> kernel to access some of these regions.
> >
> > I disagree at least for those portions owned by the secure world. The
> > kernel shouldn't map those at all, so memreserve isn't appropriate. That
> > covers OP-TEE and the MCU firmware regions, and I'd expec the EFI memory
> > map to not list those as available to the kernel.
> 
> I'm fine carving out the beginning or end, but otherwise think memory
> should correspond to the physical memory. We have a way to describe
> holes to keep out, so we should use them. If secure world uses the DT,
> then it would either want to know its region in memory or add the DT
> data to say what it is using. We need that to be easy to find or easy
> to set, respectively. The size secure world needs could vary as well.
> 
> The fact that the kernel maps the memory is the kernel's problem, not
> a DT problem.
> 

Just give more input here. In previous time, we have long discussion [1];
So actually your suggestion is exactly same what my old patch.

>From previous discussion, i think here have an assumtion: Use UEFI as
bootloader, the kernel will ignore (or remove) memreserve and reserved-memory
nodes, so just like Mark said "the EFI memory map to not list those
as available to the kernel". My new patch is just to follow this and
also make sure they have same behavior for different bootloader
(between UEFI and uboot).

[1] http://archive.arm.linux.org.uk/lurker/thread/20150819.093735.59724a58.en.html#i20150819.093735.59724a58

Thanks,
Leo Yan

> >
> > For the mailbox memory reserved-memory should be OK.
> 
> That only gets us from 4 regions to 3.
> 
> Rob

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Rob Herring]

On Fri, Oct 9, 2015 at 9:20 AM, Leo Yan <leo.yan@linaro.org> wrote:
> Hi Rob,
>
> On Fri, Oct 09, 2015 at 08:50:13AM -0500, Rob Herring wrote:
>> On Fri, Oct 9, 2015 at 8:30 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Fri, Oct 09, 2015 at 08:17:16AM -0500, Rob Herring wrote:
>> >> On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@linaro.org> wrote:
>> >> > On Hi6220, below memory regions in DDR have specific purpose:
>> >> >
>> >> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
>> >> >   0x06df,f000 - 0x06df,ffff: For mailbox message data;
>> >> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
>> >> >   0x3e00,0000 - 0x3fff,ffff: For OP-TEE.
>> >> >
>> >> > This patch reserves these memory regions in DT.
>> >> >
>> >> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> >> > ---
>> >> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
>> >> >  1 file changed, 12 insertions(+), 4 deletions(-)
>> >> >
>> >> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
>> >> > index e36a539..e3f4cb3 100644
>> >> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
>> >> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
>> >> > @@ -7,9 +7,6 @@
>> >> >
>> >> >  /dts-v1/;
>> >> >
>> >> > -/*Reserved 1MB memory for MCU*/
>> >> > -/memreserve/ 0x05e00000 0x00100000;
>> >> > -
>> >>
>> >> Why does memreserve not work for you? You can have multiple entries.
>> >>
>> >> >  #include "hi6220.dtsi"
>> >> >
>> >> >  / {
>> >> > @@ -24,8 +21,19 @@
>> >> >                 stdout-path = "serial0:115200n8";
>> >> >         };
>> >> >
>> >> > +       /*
>> >> > +        * Reserve below regions from memory node:
>> >> > +        *
>> >> > +        *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
>> >> > +        *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
>> >> > +        *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
>> >> > +        *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
>> >> > +        */
>> >> >         memory at 0 {
>> >> >                 device_type = "memory";
>> >> > -               reg = <0x0 0x0 0x0 0x40000000>;
>> >> > +               reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
>> >> > +                     <0x00000000 0x05f00000 0x00000000 0x00eff000>,
>> >> > +                     <0x00000000 0x06e00000 0x00000000 0x0060f000>,
>> >> > +                     <0x00000000 0x07410000 0x00000000 0x36bf0000>;
>> >>
>> >> No, don't do this. Please use memreserve or reserved-memory binding[1]
>> >> or combination of both. Probably reserved-memory if you need the
>> >> kernel to access some of these regions.
>> >
>> > I disagree at least for those portions owned by the secure world. The
>> > kernel shouldn't map those at all, so memreserve isn't appropriate. That
>> > covers OP-TEE and the MCU firmware regions, and I'd expec the EFI memory
>> > map to not list those as available to the kernel.
>>
>> I'm fine carving out the beginning or end, but otherwise think memory
>> should correspond to the physical memory. We have a way to describe
>> holes to keep out, so we should use them. If secure world uses the DT,
>> then it would either want to know its region in memory or add the DT
>> data to say what it is using. We need that to be easy to find or easy
>> to set, respectively. The size secure world needs could vary as well.
>>
>> The fact that the kernel maps the memory is the kernel's problem, not
>> a DT problem.
>>
>
> Just give more input here. In previous time, we have long discussion [1];
> So actually your suggestion is exactly same what my old patch.
>
> From previous discussion, i think here have an assumtion: Use UEFI as
> bootloader, the kernel will ignore (or remove) memreserve and reserved-memory
> nodes, so just like Mark said "the EFI memory map to not list those
> as available to the kernel". My new patch is just to follow this and
> also make sure they have same behavior for different bootloader
> (between UEFI and uboot).

I've read thru the thread and see 2 main conclusions. Using
reserved-memory is problematic since things like grub don't support
that. That is fine and we should stick with /mem-reserve/ for now. The
other thing is the desire to have the memory presented to the kernel
be the same whether it comes from UEFI or DT structures. I can see why
there is some desire to have that alignment, but that doesn't really
buy us anything. We can't eliminate some code path in the kernel doing
so. So I still think that the memory node should reflect all of memory
as defined by the h/w and mem-reserve should be used for any software
defined reserved regions.

Rob

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Leo Yan]

On Wed, Oct 28, 2015 at 11:32:29PM -0500, Rob Herring wrote:
> On Fri, Oct 9, 2015 at 9:20 AM, Leo Yan <leo.yan@linaro.org> wrote:
> > On Fri, Oct 09, 2015 at 08:50:13AM -0500, Rob Herring wrote:
> >> On Fri, Oct 9, 2015 at 8:30 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > On Fri, Oct 09, 2015 at 08:17:16AM -0500, Rob Herring wrote:
> >> >> On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@linaro.org> wrote:
> >> >> > On Hi6220, below memory regions in DDR have specific purpose:
> >> >> >
> >> >> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> >> >> >   0x06df,f000 - 0x06df,ffff: For mailbox message data;
> >> >> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> >> >> >   0x3e00,0000 - 0x3fff,ffff: For OP-TEE.
> >> >> >
> >> >> > This patch reserves these memory regions in DT.
> >> >> >
> >> >> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> >> >> > ---
> >> >> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
> >> >> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >> >> >
> >> >> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> >> >> > index e36a539..e3f4cb3 100644
> >> >> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> >> >> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> >> >> > @@ -7,9 +7,6 @@
> >> >> >
> >> >> >  /dts-v1/;
> >> >> >
> >> >> > -/*Reserved 1MB memory for MCU*/
> >> >> > -/memreserve/ 0x05e00000 0x00100000;
> >> >> > -
> >> >>
> >> >> Why does memreserve not work for you? You can have multiple entries.
> >> >>
> >> >> >  #include "hi6220.dtsi"
> >> >> >
> >> >> >  / {
> >> >> > @@ -24,8 +21,19 @@
> >> >> >                 stdout-path = "serial0:115200n8";
> >> >> >         };
> >> >> >
> >> >> > +       /*
> >> >> > +        * Reserve below regions from memory node:
> >> >> > +        *
> >> >> > +        *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
> >> >> > +        *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
> >> >> > +        *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
> >> >> > +        *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
> >> >> > +        */
> >> >> >         memory at 0 {
> >> >> >                 device_type = "memory";
> >> >> > -               reg = <0x0 0x0 0x0 0x40000000>;
> >> >> > +               reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> >> >> > +                     <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> >> >> > +                     <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> >> >> > +                     <0x00000000 0x07410000 0x00000000 0x36bf0000>;
> >> >>
> >> >> No, don't do this. Please use memreserve or reserved-memory binding[1]
> >> >> or combination of both. Probably reserved-memory if you need the
> >> >> kernel to access some of these regions.
> >> >
> >> > I disagree at least for those portions owned by the secure world. The
> >> > kernel shouldn't map those at all, so memreserve isn't appropriate. That
> >> > covers OP-TEE and the MCU firmware regions, and I'd expec the EFI memory
> >> > map to not list those as available to the kernel.
> >>
> >> I'm fine carving out the beginning or end, but otherwise think memory
> >> should correspond to the physical memory. We have a way to describe
> >> holes to keep out, so we should use them. If secure world uses the DT,
> >> then it would either want to know its region in memory or add the DT
> >> data to say what it is using. We need that to be easy to find or easy
> >> to set, respectively. The size secure world needs could vary as well.
> >>
> >> The fact that the kernel maps the memory is the kernel's problem, not
> >> a DT problem.
> >>
> >
> > Just give more input here. In previous time, we have long discussion [1];
> > So actually your suggestion is exactly same what my old patch.
> >
> > From previous discussion, i think here have an assumtion: Use UEFI as
> > bootloader, the kernel will ignore (or remove) memreserve and reserved-memory
> > nodes, so just like Mark said "the EFI memory map to not list those
> > as available to the kernel". My new patch is just to follow this and
> > also make sure they have same behavior for different bootloader
> > (between UEFI and uboot).
> 
> I've read thru the thread and see 2 main conclusions. Using
> reserved-memory is problematic since things like grub don't support
> that. That is fine and we should stick with /mem-reserve/ for now.

Thanks for reviewing, Rob.

One thing should note: after booting with UEFI, /memreserve/ nodes
will be deleted by UEFI stub; and _ONLY_ can use /reserved-memory/
node to reserve memory regions.

Ard have another patch [1], after applied this patch, then all
/memreserve/ nodes and /reserved-memory/ nodes nodes will be ignored
to scan after booting with UEFI stub.

This is make sense, that means UEFI need provide exactly correct memory
map info by self and totally not depend on DT structures.

Another minor difference between /memreserve/ node and /reserved-memory/
node is: we can add property "no-map" for /reserved-memory/; so that
means it will totally remove region from memory block. it's more safe
for the memory region will NOT be mapped twice with different mapping
attribution.

[1] http://archive.arm.linux.org.uk/lurker/message/20150922.002128.46757034.en.html

> The other thing is the desire to have the memory presented to the kernel
> be the same whether it comes from UEFI or DT structures. I can see why
> there is some desire to have that alignment, but that doesn't really
> buy us anything. We can't eliminate some code path in the kernel doing
> so. So I still think that the memory node should reflect all of memory
> as defined by the h/w and mem-reserve should be used for any software
> defined reserved regions.

i think before we engaged much thinking for UEFI, that's meaningful for
we found what's correct implementation for UEFI. We need make sure UEFI
will do correct thing for itself.

If only consider purly from DT's usage, i have no strong opinion to
stick to use memory node to carve memory regions out. It's okay for me to
go back to use /reserved-memory/ to reserved regions.

Mark, do you agree with this?

Thanks,
Leo Yan

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Leo Yan]

On Thu, Oct 29, 2015 at 04:33:01PM +0800, Leo Yan wrote:
> On Wed, Oct 28, 2015 at 11:32:29PM -0500, Rob Herring wrote:
> > On Fri, Oct 9, 2015 at 9:20 AM, Leo Yan <leo.yan@linaro.org> wrote:
> > > On Fri, Oct 09, 2015 at 08:50:13AM -0500, Rob Herring wrote:
> > >> On Fri, Oct 9, 2015 at 8:30 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > >> > On Fri, Oct 09, 2015 at 08:17:16AM -0500, Rob Herring wrote:
> > >> >> On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@linaro.org> wrote:
> > >> >> > On Hi6220, below memory regions in DDR have specific purpose:
> > >> >> >
> > >> >> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> > >> >> >   0x06df,f000 - 0x06df,ffff: For mailbox message data;
> > >> >> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> > >> >> >   0x3e00,0000 - 0x3fff,ffff: For OP-TEE.
> > >> >> >
> > >> >> > This patch reserves these memory regions in DT.
> > >> >> >
> > >> >> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > >> >> > ---
> > >> >> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
> > >> >> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > >> >> >
> > >> >> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > >> >> > index e36a539..e3f4cb3 100644
> > >> >> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > >> >> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > >> >> > @@ -7,9 +7,6 @@
> > >> >> >
> > >> >> >  /dts-v1/;
> > >> >> >
> > >> >> > -/*Reserved 1MB memory for MCU*/
> > >> >> > -/memreserve/ 0x05e00000 0x00100000;
> > >> >> > -
> > >> >>
> > >> >> Why does memreserve not work for you? You can have multiple entries.
> > >> >>
> > >> >> >  #include "hi6220.dtsi"
> > >> >> >
> > >> >> >  / {
> > >> >> > @@ -24,8 +21,19 @@
> > >> >> >                 stdout-path = "serial0:115200n8";
> > >> >> >         };
> > >> >> >
> > >> >> > +       /*
> > >> >> > +        * Reserve below regions from memory node:
> > >> >> > +        *
> > >> >> > +        *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
> > >> >> > +        *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
> > >> >> > +        *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
> > >> >> > +        *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
> > >> >> > +        */
> > >> >> >         memory at 0 {
> > >> >> >                 device_type = "memory";
> > >> >> > -               reg = <0x0 0x0 0x0 0x40000000>;
> > >> >> > +               reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> > >> >> > +                     <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> > >> >> > +                     <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> > >> >> > +                     <0x00000000 0x07410000 0x00000000 0x36bf0000>;
> > >> >>
> > >> >> No, don't do this. Please use memreserve or reserved-memory binding[1]
> > >> >> or combination of both. Probably reserved-memory if you need the
> > >> >> kernel to access some of these regions.
> > >> >
> > >> > I disagree at least for those portions owned by the secure world. The
> > >> > kernel shouldn't map those at all, so memreserve isn't appropriate. That
> > >> > covers OP-TEE and the MCU firmware regions, and I'd expec the EFI memory
> > >> > map to not list those as available to the kernel.
> > >>
> > >> I'm fine carving out the beginning or end, but otherwise think memory
> > >> should correspond to the physical memory. We have a way to describe
> > >> holes to keep out, so we should use them. If secure world uses the DT,
> > >> then it would either want to know its region in memory or add the DT
> > >> data to say what it is using. We need that to be easy to find or easy
> > >> to set, respectively. The size secure world needs could vary as well.
> > >>
> > >> The fact that the kernel maps the memory is the kernel's problem, not
> > >> a DT problem.
> > >>
> > >
> > > Just give more input here. In previous time, we have long discussion [1];
> > > So actually your suggestion is exactly same what my old patch.
> > >
> > > From previous discussion, i think here have an assumtion: Use UEFI as
> > > bootloader, the kernel will ignore (or remove) memreserve and reserved-memory
> > > nodes, so just like Mark said "the EFI memory map to not list those
> > > as available to the kernel". My new patch is just to follow this and
> > > also make sure they have same behavior for different bootloader
> > > (between UEFI and uboot).
> > 
> > I've read thru the thread and see 2 main conclusions. Using
> > reserved-memory is problematic since things like grub don't support
> > that. That is fine and we should stick with /mem-reserve/ for now.
> 
> Thanks for reviewing, Rob.
> 
> One thing should note: after booting with UEFI, /memreserve/ nodes
> will be deleted by UEFI stub; and _ONLY_ can use /reserved-memory/
> node to reserve memory regions.
> 
> Ard have another patch [1], after applied this patch, then all
> /memreserve/ nodes and /reserved-memory/ nodes nodes will be ignored
> to scan after booting with UEFI stub.
> 
> This is make sense, that means UEFI need provide exactly correct memory
> map info by self and totally not depend on DT structures.
> 
> Another minor difference between /memreserve/ node and /reserved-memory/
> node is: we can add property "no-map" for /reserved-memory/; so that
> means it will totally remove region from memory block. it's more safe
> for the memory region will NOT be mapped twice with different mapping
> attribution.
> 
> [1] http://archive.arm.linux.org.uk/lurker/message/20150922.002128.46757034.en.html
> 
> > The other thing is the desire to have the memory presented to the kernel
> > be the same whether it comes from UEFI or DT structures. I can see why
> > there is some desire to have that alignment, but that doesn't really
> > buy us anything. We can't eliminate some code path in the kernel doing
> > so. So I still think that the memory node should reflect all of memory
> > as defined by the h/w and mem-reserve should be used for any software
> > defined reserved regions.
> 
> i think before we engaged much thinking for UEFI, that's meaningful for
> we found what's correct implementation for UEFI. We need make sure UEFI
> will do correct thing for itself.
> 
> If only consider purly from DT's usage, i have no strong opinion to
> stick to use memory node to carve memory regions out. It's okay for me to
> go back to use /reserved-memory/ to reserved regions.
> 
> Mark, do you agree with this?

Ping ...

Hi Mark,

Could u help confirm for this? i'm planning to resend new version
patch series in tommorrow, but it's better can get your feedback
firstly.

Thanks,
Leo Yan

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Mark Rutland]

On Thu, Nov 05, 2015 at 09:54:50PM +0800, Leo Yan wrote:
> On Thu, Oct 29, 2015 at 04:33:01PM +0800, Leo Yan wrote:
> > On Wed, Oct 28, 2015 at 11:32:29PM -0500, Rob Herring wrote:
> > > On Fri, Oct 9, 2015 at 9:20 AM, Leo Yan <leo.yan@linaro.org> wrote:
> > > > On Fri, Oct 09, 2015 at 08:50:13AM -0500, Rob Herring wrote:
> > > >> On Fri, Oct 9, 2015 at 8:30 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > > >> > On Fri, Oct 09, 2015 at 08:17:16AM -0500, Rob Herring wrote:
> > > >> >> On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@linaro.org> wrote:
> > > >> >> > On Hi6220, below memory regions in DDR have specific purpose:
> > > >> >> >
> > > >> >> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> > > >> >> >   0x06df,f000 - 0x06df,ffff: For mailbox message data;
> > > >> >> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> > > >> >> >   0x3e00,0000 - 0x3fff,ffff: For OP-TEE.
> > > >> >> >
> > > >> >> > This patch reserves these memory regions in DT.
> > > >> >> >
> > > >> >> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > >> >> > ---
> > > >> >> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
> > > >> >> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > >> >> >
> > > >> >> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > >> >> > index e36a539..e3f4cb3 100644
> > > >> >> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > >> >> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > >> >> > @@ -7,9 +7,6 @@
> > > >> >> >
> > > >> >> >  /dts-v1/;
> > > >> >> >
> > > >> >> > -/*Reserved 1MB memory for MCU*/
> > > >> >> > -/memreserve/ 0x05e00000 0x00100000;
> > > >> >> > -
> > > >> >>
> > > >> >> Why does memreserve not work for you? You can have multiple entries.
> > > >> >>
> > > >> >> >  #include "hi6220.dtsi"
> > > >> >> >
> > > >> >> >  / {
> > > >> >> > @@ -24,8 +21,19 @@
> > > >> >> >                 stdout-path = "serial0:115200n8";
> > > >> >> >         };
> > > >> >> >
> > > >> >> > +       /*
> > > >> >> > +        * Reserve below regions from memory node:
> > > >> >> > +        *
> > > >> >> > +        *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
> > > >> >> > +        *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
> > > >> >> > +        *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
> > > >> >> > +        *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
> > > >> >> > +        */
> > > >> >> >         memory at 0 {
> > > >> >> >                 device_type = "memory";
> > > >> >> > -               reg = <0x0 0x0 0x0 0x40000000>;
> > > >> >> > +               reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> > > >> >> > +                     <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> > > >> >> > +                     <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> > > >> >> > +                     <0x00000000 0x07410000 0x00000000 0x36bf0000>;
> > > >> >>
> > > >> >> No, don't do this. Please use memreserve or reserved-memory binding[1]
> > > >> >> or combination of both. Probably reserved-memory if you need the
> > > >> >> kernel to access some of these regions.
> > > >> >
> > > >> > I disagree at least for those portions owned by the secure world. The
> > > >> > kernel shouldn't map those at all, so memreserve isn't appropriate. That
> > > >> > covers OP-TEE and the MCU firmware regions, and I'd expec the EFI memory
> > > >> > map to not list those as available to the kernel.
> > > >>
> > > >> I'm fine carving out the beginning or end, but otherwise think memory
> > > >> should correspond to the physical memory. We have a way to describe
> > > >> holes to keep out, so we should use them. If secure world uses the DT,
> > > >> then it would either want to know its region in memory or add the DT
> > > >> data to say what it is using. We need that to be easy to find or easy
> > > >> to set, respectively. The size secure world needs could vary as well.
> > > >>
> > > >> The fact that the kernel maps the memory is the kernel's problem, not
> > > >> a DT problem.
> > > >>
> > > >
> > > > Just give more input here. In previous time, we have long discussion [1];
> > > > So actually your suggestion is exactly same what my old patch.
> > > >
> > > > From previous discussion, i think here have an assumtion: Use UEFI as
> > > > bootloader, the kernel will ignore (or remove) memreserve and reserved-memory
> > > > nodes, so just like Mark said "the EFI memory map to not list those
> > > > as available to the kernel". My new patch is just to follow this and
> > > > also make sure they have same behavior for different bootloader
> > > > (between UEFI and uboot).
> > > 
> > > I've read thru the thread and see 2 main conclusions. Using
> > > reserved-memory is problematic since things like grub don't support
> > > that. That is fine and we should stick with /mem-reserve/ for now.
> > 
> > Thanks for reviewing, Rob.
> > 
> > One thing should note: after booting with UEFI, /memreserve/ nodes
> > will be deleted by UEFI stub; and _ONLY_ can use /reserved-memory/
> > node to reserve memory regions.
> > 
> > Ard have another patch [1], after applied this patch, then all
> > /memreserve/ nodes and /reserved-memory/ nodes nodes will be ignored
> > to scan after booting with UEFI stub.
> > 
> > This is make sense, that means UEFI need provide exactly correct memory
> > map info by self and totally not depend on DT structures.
> > 
> > Another minor difference between /memreserve/ node and /reserved-memory/
> > node is: we can add property "no-map" for /reserved-memory/; so that
> > means it will totally remove region from memory block. it's more safe
> > for the memory region will NOT be mapped twice with different mapping
> > attribution.
> > 
> > [1] http://archive.arm.linux.org.uk/lurker/message/20150922.002128.46757034.en.html
> > 
> > > The other thing is the desire to have the memory presented to the kernel
> > > be the same whether it comes from UEFI or DT structures. I can see why
> > > there is some desire to have that alignment, but that doesn't really
> > > buy us anything. We can't eliminate some code path in the kernel doing
> > > so. So I still think that the memory node should reflect all of memory
> > > as defined by the h/w and mem-reserve should be used for any software
> > > defined reserved regions.
> > 
> > i think before we engaged much thinking for UEFI, that's meaningful for
> > we found what's correct implementation for UEFI. We need make sure UEFI
> > will do correct thing for itself.
> > 
> > If only consider purly from DT's usage, i have no strong opinion to
> > stick to use memory node to carve memory regions out. It's okay for me to
> > go back to use /reserved-memory/ to reserved regions.
> > 
> > Mark, do you agree with this?
> 
> Ping ...
> 
> Hi Mark,
> 
> Could u help confirm for this? i'm planning to resend new version
> patch series in tommorrow, but it's better can get your feedback
> firstly.

Sorry for the delay.

I'm still of the opinion that given the kernel has no business even
reading this memory, it does not make sense to use a memreserve.

Given that, and the points about other software not knowing aobut
/reserved-memory/, I don't think it makes sense to describe the region
in the memory nodes.

Thanks,
Mark.

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Leo Yan]

On Thu, Nov 05, 2015 at 04:13:15PM +0000, Mark Rutland wrote:
> On Thu, Nov 05, 2015 at 09:54:50PM +0800, Leo Yan wrote:
> > On Thu, Oct 29, 2015 at 04:33:01PM +0800, Leo Yan wrote:
> > > On Wed, Oct 28, 2015 at 11:32:29PM -0500, Rob Herring wrote:
> > > > On Fri, Oct 9, 2015 at 9:20 AM, Leo Yan <leo.yan@linaro.org> wrote:
> > > > > On Fri, Oct 09, 2015 at 08:50:13AM -0500, Rob Herring wrote:
> > > > >> On Fri, Oct 9, 2015 at 8:30 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > >> > On Fri, Oct 09, 2015 at 08:17:16AM -0500, Rob Herring wrote:
> > > > >> >> On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@linaro.org> wrote:
> > > > >> >> > On Hi6220, below memory regions in DDR have specific purpose:
> > > > >> >> >
> > > > >> >> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> > > > >> >> >   0x06df,f000 - 0x06df,ffff: For mailbox message data;
> > > > >> >> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> > > > >> >> >   0x3e00,0000 - 0x3fff,ffff: For OP-TEE.
> > > > >> >> >
> > > > >> >> > This patch reserves these memory regions in DT.
> > > > >> >> >
> > > > >> >> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > > >> >> > ---
> > > > >> >> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
> > > > >> >> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > > >> >> >
> > > > >> >> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > > >> >> > index e36a539..e3f4cb3 100644
> > > > >> >> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > > >> >> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > > >> >> > @@ -7,9 +7,6 @@
> > > > >> >> >
> > > > >> >> >  /dts-v1/;
> > > > >> >> >
> > > > >> >> > -/*Reserved 1MB memory for MCU*/
> > > > >> >> > -/memreserve/ 0x05e00000 0x00100000;
> > > > >> >> > -
> > > > >> >>
> > > > >> >> Why does memreserve not work for you? You can have multiple entries.
> > > > >> >>
> > > > >> >> >  #include "hi6220.dtsi"
> > > > >> >> >
> > > > >> >> >  / {
> > > > >> >> > @@ -24,8 +21,19 @@
> > > > >> >> >                 stdout-path = "serial0:115200n8";
> > > > >> >> >         };
> > > > >> >> >
> > > > >> >> > +       /*
> > > > >> >> > +        * Reserve below regions from memory node:
> > > > >> >> > +        *
> > > > >> >> > +        *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
> > > > >> >> > +        *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
> > > > >> >> > +        *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
> > > > >> >> > +        *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
> > > > >> >> > +        */
> > > > >> >> >         memory at 0 {
> > > > >> >> >                 device_type = "memory";
> > > > >> >> > -               reg = <0x0 0x0 0x0 0x40000000>;
> > > > >> >> > +               reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> > > > >> >> > +                     <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> > > > >> >> > +                     <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> > > > >> >> > +                     <0x00000000 0x07410000 0x00000000 0x36bf0000>;
> > > > >> >>
> > > > >> >> No, don't do this. Please use memreserve or reserved-memory binding[1]
> > > > >> >> or combination of both. Probably reserved-memory if you need the
> > > > >> >> kernel to access some of these regions.
> > > > >> >
> > > > >> > I disagree at least for those portions owned by the secure world. The
> > > > >> > kernel shouldn't map those at all, so memreserve isn't appropriate. That
> > > > >> > covers OP-TEE and the MCU firmware regions, and I'd expec the EFI memory
> > > > >> > map to not list those as available to the kernel.
> > > > >>
> > > > >> I'm fine carving out the beginning or end, but otherwise think memory
> > > > >> should correspond to the physical memory. We have a way to describe
> > > > >> holes to keep out, so we should use them. If secure world uses the DT,
> > > > >> then it would either want to know its region in memory or add the DT
> > > > >> data to say what it is using. We need that to be easy to find or easy
> > > > >> to set, respectively. The size secure world needs could vary as well.
> > > > >>
> > > > >> The fact that the kernel maps the memory is the kernel's problem, not
> > > > >> a DT problem.
> > > > >>
> > > > >
> > > > > Just give more input here. In previous time, we have long discussion [1];
> > > > > So actually your suggestion is exactly same what my old patch.
> > > > >
> > > > > From previous discussion, i think here have an assumtion: Use UEFI as
> > > > > bootloader, the kernel will ignore (or remove) memreserve and reserved-memory
> > > > > nodes, so just like Mark said "the EFI memory map to not list those
> > > > > as available to the kernel". My new patch is just to follow this and
> > > > > also make sure they have same behavior for different bootloader
> > > > > (between UEFI and uboot).
> > > > 
> > > > I've read thru the thread and see 2 main conclusions. Using
> > > > reserved-memory is problematic since things like grub don't support
> > > > that. That is fine and we should stick with /mem-reserve/ for now.
> > > 
> > > Thanks for reviewing, Rob.
> > > 
> > > One thing should note: after booting with UEFI, /memreserve/ nodes
> > > will be deleted by UEFI stub; and _ONLY_ can use /reserved-memory/
> > > node to reserve memory regions.
> > > 
> > > Ard have another patch [1], after applied this patch, then all
> > > /memreserve/ nodes and /reserved-memory/ nodes nodes will be ignored
> > > to scan after booting with UEFI stub.
> > > 
> > > This is make sense, that means UEFI need provide exactly correct memory
> > > map info by self and totally not depend on DT structures.
> > > 
> > > Another minor difference between /memreserve/ node and /reserved-memory/
> > > node is: we can add property "no-map" for /reserved-memory/; so that
> > > means it will totally remove region from memory block. it's more safe
> > > for the memory region will NOT be mapped twice with different mapping
> > > attribution.
> > > 
> > > [1] http://archive.arm.linux.org.uk/lurker/message/20150922.002128.46757034.en.html
> > > 
> > > > The other thing is the desire to have the memory presented to the kernel
> > > > be the same whether it comes from UEFI or DT structures. I can see why
> > > > there is some desire to have that alignment, but that doesn't really
> > > > buy us anything. We can't eliminate some code path in the kernel doing
> > > > so. So I still think that the memory node should reflect all of memory
> > > > as defined by the h/w and mem-reserve should be used for any software
> > > > defined reserved regions.
> > > 
> > > i think before we engaged much thinking for UEFI, that's meaningful for
> > > we found what's correct implementation for UEFI. We need make sure UEFI
> > > will do correct thing for itself.
> > > 
> > > If only consider purly from DT's usage, i have no strong opinion to
> > > stick to use memory node to carve memory regions out. It's okay for me to
> > > go back to use /reserved-memory/ to reserved regions.
> > > 
> > > Mark, do you agree with this?
> > 
> > Ping ...
> > 
> > Hi Mark,
> > 
> > Could u help confirm for this? i'm planning to resend new version
> > patch series in tommorrow, but it's better can get your feedback
> > firstly.
> 
> Sorry for the delay.
> 
> I'm still of the opinion that given the kernel has no business even
> reading this memory, it does not make sense to use a memreserve.
> 
> Given that, and the points about other software not knowing aobut
> /reserved-memory/, I don't think it makes sense to describe the region
> in the memory nodes.

Here "other software" means other OS but not Linux, right? i
rememebered before you meantioned /reserved-memory/ is dedicated for
Linux kernel and i checked ePAPR has not defined this property. If we
cannot use /reserved-memory/ currently just because other OS don't
know it, then we should firstly commit one patch to totally remove it
from Linux kernel; Haojian also brought up this question at very early
time.

Basically i'm reluctant to engage UEFI anymore for this discussion :),
but if you are meaning "other software" as UEFI, IMOH, this is also
not make sense to cannot use /reserved-memory/. Because after last
time's discussion, Haojian has implemented UEFI memory map and we
already verified it on Hikey. UEFI will create memory map for itself
and it's no matter with DT's memory node and have no dependancy with
/reserved-memory/.

But suppose /reserved-memory/ will kept in Linux kernel and and now
i'm just committing patches for Linux kernel. So i'd like to summary
some rules for memory reservasion from my own understanding (also have
posted on IRC):

- Memory node is just to describe physical memory layout; Before there
  have many discussion for DT binding should describe the hardware, in
  a fashion which is completely OS-agnostic[1]. i think it also can
  apply to memory node, we just use memory node to describe physical
  memory layout, but not describe software's usage.
- /memreserve/ is used to allow kernel to map linear virtual kernel
  address, usually it will not be used by driver; otherwise kernel
  will map the memory region twice with different memory attribution;
- /reserved-memory/ is used to the memory regions, which will be
  remove from memory block and can be mapped by driver with ioremap.

This will be easily for us follow up these rules and use it.

[1] https://lwn.net/Articles/560523/

Thanks,
Leo Yan

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Leo Yan]

On Fri, Nov 06, 2015 at 09:19:37AM +0800, Leo Yan wrote:
> On Thu, Nov 05, 2015 at 04:13:15PM +0000, Mark Rutland wrote:
> > On Thu, Nov 05, 2015 at 09:54:50PM +0800, Leo Yan wrote:
> > > On Thu, Oct 29, 2015 at 04:33:01PM +0800, Leo Yan wrote:
> > > > On Wed, Oct 28, 2015 at 11:32:29PM -0500, Rob Herring wrote:
> > > > > On Fri, Oct 9, 2015 at 9:20 AM, Leo Yan <leo.yan@linaro.org> wrote:
> > > > > > On Fri, Oct 09, 2015 at 08:50:13AM -0500, Rob Herring wrote:
> > > > > >> On Fri, Oct 9, 2015 at 8:30 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > > >> > On Fri, Oct 09, 2015 at 08:17:16AM -0500, Rob Herring wrote:
> > > > > >> >> On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@linaro.org> wrote:
> > > > > >> >> > On Hi6220, below memory regions in DDR have specific purpose:
> > > > > >> >> >
> > > > > >> >> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> > > > > >> >> >   0x06df,f000 - 0x06df,ffff: For mailbox message data;
> > > > > >> >> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> > > > > >> >> >   0x3e00,0000 - 0x3fff,ffff: For OP-TEE.
> > > > > >> >> >
> > > > > >> >> > This patch reserves these memory regions in DT.
> > > > > >> >> >
> > > > > >> >> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > > > >> >> > ---
> > > > > >> >> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
> > > > > >> >> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > > > >> >> >
> > > > > >> >> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > > > >> >> > index e36a539..e3f4cb3 100644
> > > > > >> >> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > > > >> >> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > > > >> >> > @@ -7,9 +7,6 @@
> > > > > >> >> >
> > > > > >> >> >  /dts-v1/;
> > > > > >> >> >
> > > > > >> >> > -/*Reserved 1MB memory for MCU*/
> > > > > >> >> > -/memreserve/ 0x05e00000 0x00100000;
> > > > > >> >> > -
> > > > > >> >>
> > > > > >> >> Why does memreserve not work for you? You can have multiple entries.
> > > > > >> >>
> > > > > >> >> >  #include "hi6220.dtsi"
> > > > > >> >> >
> > > > > >> >> >  / {
> > > > > >> >> > @@ -24,8 +21,19 @@
> > > > > >> >> >                 stdout-path = "serial0:115200n8";
> > > > > >> >> >         };
> > > > > >> >> >
> > > > > >> >> > +       /*
> > > > > >> >> > +        * Reserve below regions from memory node:
> > > > > >> >> > +        *
> > > > > >> >> > +        *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
> > > > > >> >> > +        *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
> > > > > >> >> > +        *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
> > > > > >> >> > +        *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
> > > > > >> >> > +        */
> > > > > >> >> >         memory at 0 {
> > > > > >> >> >                 device_type = "memory";
> > > > > >> >> > -               reg = <0x0 0x0 0x0 0x40000000>;
> > > > > >> >> > +               reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> > > > > >> >> > +                     <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> > > > > >> >> > +                     <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> > > > > >> >> > +                     <0x00000000 0x07410000 0x00000000 0x36bf0000>;
> > > > > >> >>

[...]

> > > > > I've read thru the thread and see 2 main conclusions. Using
> > > > > reserved-memory is problematic since things like grub don't support
> > > > > that. That is fine and we should stick with /mem-reserve/ for now.
> > > > >
> > > > > The other thing is the desire to have the memory presented to the kernel
> > > > > be the same whether it comes from UEFI or DT structures. I can see why
> > > > > there is some desire to have that alignment, but that doesn't really
> > > > > buy us anything. We can't eliminate some code path in the kernel doing
> > > > > so. So I still think that the memory node should reflect all of memory
> > > > > as defined by the h/w and mem-reserve should be used for any software
> > > > > defined reserved regions.

[...]

> > Sorry for the delay.
> > 
> > I'm still of the opinion that given the kernel has no business even
> > reading this memory, it does not make sense to use a memreserve.
> > 
> > Given that, and the points about other software not knowing aobut
> > /reserved-memory/, I don't think it makes sense to describe the region
> > in the memory nodes.

[...]

Hi Rob, Mark,

Sorry its taken me so long to bump this thread.

So I can have more confidence discussing this I have reviewed the
existing files in arch/arm/boot and arch/arm64/boot and done a bit of
analysis. My summary is below:

In arch/arm/boot:

- In arch/arm, Using /reserved-memory/ and /memreserve/ are very common
  case to reserve memory regions;
- /memreserve/ is mainly used to reserve memory for spin-table which
  used by SMP booting;
- /reserved-memory/ are used to reserve memory regions for security
  (TrustZone, secure RAM, etc), message transferring; So we can say
  usually use this method to reserve memory for device driver or other
  modules which maps these memory regions later.
- There also have some DT files using memory node to carve out memory
  regions, but from roughly analysis we can see the most cases are
  introduced by memory controller for discontinuous memory space for
  DDR; or there have some silicon bug so some regions are not stable
  so need get rid of them (Armada-xp);

In arch/arm64/boot:

- There have no any file using /reserved-memory/;
- Several files are using /memreserve/ for spin-table boot method;
- ARM platforms (Juno, FVP) use memory nodes to carve out memory regions
  for security;
- DT files in arch/arm64 are quite consistent with Mark's suggestion.

In my review I haven't really found anything that pushes me in one
direction or the other so, without consensus from the DT maintainers, I
really don't know how to progress here.

Thanks,
Leo Yan

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Rob Herring]

On Mon, Jan 11, 2016 at 9:40 AM, Leo Yan <leo.yan@linaro.org> wrote:
> On Fri, Nov 06, 2015 at 09:19:37AM +0800, Leo Yan wrote:
>> On Thu, Nov 05, 2015 at 04:13:15PM +0000, Mark Rutland wrote:
>> > On Thu, Nov 05, 2015 at 09:54:50PM +0800, Leo Yan wrote:
>> > > On Thu, Oct 29, 2015 at 04:33:01PM +0800, Leo Yan wrote:
>> > > > On Wed, Oct 28, 2015 at 11:32:29PM -0500, Rob Herring wrote:
>> > > > > On Fri, Oct 9, 2015 at 9:20 AM, Leo Yan <leo.yan@linaro.org> wrote:
>> > > > > > On Fri, Oct 09, 2015 at 08:50:13AM -0500, Rob Herring wrote:
>> > > > > >> On Fri, Oct 9, 2015 at 8:30 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > > > > >> > On Fri, Oct 09, 2015 at 08:17:16AM -0500, Rob Herring wrote:
>> > > > > >> >> On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@linaro.org> wrote:
>> > > > > >> >> > On Hi6220, below memory regions in DDR have specific purpose:
>> > > > > >> >> >
>> > > > > >> >> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
>> > > > > >> >> >   0x06df,f000 - 0x06df,ffff: For mailbox message data;
>> > > > > >> >> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
>> > > > > >> >> >   0x3e00,0000 - 0x3fff,ffff: For OP-TEE.
>> > > > > >> >> >
>> > > > > >> >> > This patch reserves these memory regions in DT.
>> > > > > >> >> >
>> > > > > >> >> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
>> > > > > >> >> > ---
>> > > > > >> >> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
>> > > > > >> >> >  1 file changed, 12 insertions(+), 4 deletions(-)
>> > > > > >> >> >
>> > > > > >> >> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
>> > > > > >> >> > index e36a539..e3f4cb3 100644
>> > > > > >> >> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
>> > > > > >> >> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
>> > > > > >> >> > @@ -7,9 +7,6 @@
>> > > > > >> >> >
>> > > > > >> >> >  /dts-v1/;
>> > > > > >> >> >
>> > > > > >> >> > -/*Reserved 1MB memory for MCU*/
>> > > > > >> >> > -/memreserve/ 0x05e00000 0x00100000;
>> > > > > >> >> > -
>> > > > > >> >>
>> > > > > >> >> Why does memreserve not work for you? You can have multiple entries.
>> > > > > >> >>
>> > > > > >> >> >  #include "hi6220.dtsi"
>> > > > > >> >> >
>> > > > > >> >> >  / {
>> > > > > >> >> > @@ -24,8 +21,19 @@
>> > > > > >> >> >                 stdout-path = "serial0:115200n8";
>> > > > > >> >> >         };
>> > > > > >> >> >
>> > > > > >> >> > +       /*
>> > > > > >> >> > +        * Reserve below regions from memory node:
>> > > > > >> >> > +        *
>> > > > > >> >> > +        *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
>> > > > > >> >> > +        *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
>> > > > > >> >> > +        *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
>> > > > > >> >> > +        *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
>> > > > > >> >> > +        */
>> > > > > >> >> >         memory at 0 {
>> > > > > >> >> >                 device_type = "memory";
>> > > > > >> >> > -               reg = <0x0 0x0 0x0 0x40000000>;
>> > > > > >> >> > +               reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
>> > > > > >> >> > +                     <0x00000000 0x05f00000 0x00000000 0x00eff000>,
>> > > > > >> >> > +                     <0x00000000 0x06e00000 0x00000000 0x0060f000>,
>> > > > > >> >> > +                     <0x00000000 0x07410000 0x00000000 0x36bf0000>;
>> > > > > >> >>
>
> [...]
>
>> > > > > I've read thru the thread and see 2 main conclusions. Using
>> > > > > reserved-memory is problematic since things like grub don't support
>> > > > > that. That is fine and we should stick with /mem-reserve/ for now.
>> > > > >
>> > > > > The other thing is the desire to have the memory presented to the kernel
>> > > > > be the same whether it comes from UEFI or DT structures. I can see why
>> > > > > there is some desire to have that alignment, but that doesn't really
>> > > > > buy us anything. We can't eliminate some code path in the kernel doing
>> > > > > so. So I still think that the memory node should reflect all of memory
>> > > > > as defined by the h/w and mem-reserve should be used for any software
>> > > > > defined reserved regions.
>
> [...]
>
>> > Sorry for the delay.
>> >
>> > I'm still of the opinion that given the kernel has no business even
>> > reading this memory, it does not make sense to use a memreserve.
>> >
>> > Given that, and the points about other software not knowing aobut
>> > /reserved-memory/, I don't think it makes sense to describe the region
>> > in the memory nodes.
>
> [...]
>
> Hi Rob, Mark,
>
> Sorry its taken me so long to bump this thread.
>
> So I can have more confidence discussing this I have reviewed the
> existing files in arch/arm/boot and arch/arm64/boot and done a bit of
> analysis. My summary is below:
>
> In arch/arm/boot:
>
> - In arch/arm, Using /reserved-memory/ and /memreserve/ are very common
>   case to reserve memory regions;
> - /memreserve/ is mainly used to reserve memory for spin-table which
>   used by SMP booting;
> - /reserved-memory/ are used to reserve memory regions for security
>   (TrustZone, secure RAM, etc), message transferring; So we can say
>   usually use this method to reserve memory for device driver or other
>   modules which maps these memory regions later.
>
> - There also have some DT files using memory node to carve out memory
>   regions, but from roughly analysis we can see the most cases are
>   introduced by memory controller for discontinuous memory space for
>   DDR; or there have some silicon bug so some regions are not stable
>   so need get rid of them (Armada-xp);
>
> In arch/arm64/boot:
>
> - There have no any file using /reserved-memory/;
> - Several files are using /memreserve/ for spin-table boot method;
> - ARM platforms (Juno, FVP) use memory nodes to carve out memory regions
>   for security;
> - DT files in arch/arm64 are quite consistent with Mark's suggestion.
>
> In my review I haven't really found anything that pushes me in one
> direction or the other so, without consensus from the DT maintainers, I
> really don't know how to progress here.

While I still think regions should be marked as reserved rather than
carving up the memory node, it's not really worth holding up this
platform further. The kernel can support either way.

However, Mark's argument AIUI was regions that are never accessed
should be done the way you have done it. I don't think the mailbox
region falls in this category. I'm not sure about OP-TEE, but we
should have consistency for it if the OP-TEE driver needs to find its
memory region. Cc'ing Joakim for comment.

Rob

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Mark Rutland]

On Mon, Jan 18, 2016 at 09:07:14AM -0600, Rob Herring wrote:
> On Mon, Jan 11, 2016 at 9:40 AM, Leo Yan <leo.yan@linaro.org> wrote:
> > On Fri, Nov 06, 2015 at 09:19:37AM +0800, Leo Yan wrote:
> >> On Thu, Nov 05, 2015 at 04:13:15PM +0000, Mark Rutland wrote:
> >> > On Thu, Nov 05, 2015 at 09:54:50PM +0800, Leo Yan wrote:
> >> > > On Thu, Oct 29, 2015 at 04:33:01PM +0800, Leo Yan wrote:
> >> > > > On Wed, Oct 28, 2015 at 11:32:29PM -0500, Rob Herring wrote:
> >> > > > > On Fri, Oct 9, 2015 at 9:20 AM, Leo Yan <leo.yan@linaro.org> wrote:
> >> > > > > > On Fri, Oct 09, 2015 at 08:50:13AM -0500, Rob Herring wrote:
> >> > > > > >> On Fri, Oct 9, 2015 at 8:30 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > > > > >> > On Fri, Oct 09, 2015 at 08:17:16AM -0500, Rob Herring wrote:
> >> > > > > >> >> On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@linaro.org> wrote:
> >> > > > > >> >> > On Hi6220, below memory regions in DDR have specific purpose:
> >> > > > > >> >> >
> >> > > > > >> >> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> >> > > > > >> >> >   0x06df,f000 - 0x06df,ffff: For mailbox message data;
> >> > > > > >> >> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> >> > > > > >> >> >   0x3e00,0000 - 0x3fff,ffff: For OP-TEE.
> >> > > > > >> >> >
> >> > > > > >> >> > This patch reserves these memory regions in DT.
> >> > > > > >> >> >
> >> > > > > >> >> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> >> > > > > >> >> > ---
> >> > > > > >> >> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
> >> > > > > >> >> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >> > > > > >> >> >
> >> > > > > >> >> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> >> > > > > >> >> > index e36a539..e3f4cb3 100644
> >> > > > > >> >> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> >> > > > > >> >> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> >> > > > > >> >> > @@ -7,9 +7,6 @@
> >> > > > > >> >> >
> >> > > > > >> >> >  /dts-v1/;
> >> > > > > >> >> >
> >> > > > > >> >> > -/*Reserved 1MB memory for MCU*/
> >> > > > > >> >> > -/memreserve/ 0x05e00000 0x00100000;
> >> > > > > >> >> > -
> >> > > > > >> >>
> >> > > > > >> >> Why does memreserve not work for you? You can have multiple entries.
> >> > > > > >> >>
> >> > > > > >> >> >  #include "hi6220.dtsi"
> >> > > > > >> >> >
> >> > > > > >> >> >  / {
> >> > > > > >> >> > @@ -24,8 +21,19 @@
> >> > > > > >> >> >                 stdout-path = "serial0:115200n8";
> >> > > > > >> >> >         };
> >> > > > > >> >> >
> >> > > > > >> >> > +       /*
> >> > > > > >> >> > +        * Reserve below regions from memory node:
> >> > > > > >> >> > +        *
> >> > > > > >> >> > +        *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
> >> > > > > >> >> > +        *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
> >> > > > > >> >> > +        *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
> >> > > > > >> >> > +        *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
> >> > > > > >> >> > +        */
> >> > > > > >> >> >         memory at 0 {
> >> > > > > >> >> >                 device_type = "memory";
> >> > > > > >> >> > -               reg = <0x0 0x0 0x0 0x40000000>;
> >> > > > > >> >> > +               reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> >> > > > > >> >> > +                     <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> >> > > > > >> >> > +                     <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> >> > > > > >> >> > +                     <0x00000000 0x07410000 0x00000000 0x36bf0000>;
> >> > > > > >> >>
> >
> > [...]
> >
> >> > > > > I've read thru the thread and see 2 main conclusions. Using
> >> > > > > reserved-memory is problematic since things like grub don't support
> >> > > > > that. That is fine and we should stick with /mem-reserve/ for now.
> >> > > > >
> >> > > > > The other thing is the desire to have the memory presented to the kernel
> >> > > > > be the same whether it comes from UEFI or DT structures. I can see why
> >> > > > > there is some desire to have that alignment, but that doesn't really
> >> > > > > buy us anything. We can't eliminate some code path in the kernel doing
> >> > > > > so. So I still think that the memory node should reflect all of memory
> >> > > > > as defined by the h/w and mem-reserve should be used for any software
> >> > > > > defined reserved regions.
> >
> > [...]
> >
> >> > Sorry for the delay.
> >> >
> >> > I'm still of the opinion that given the kernel has no business even
> >> > reading this memory, it does not make sense to use a memreserve.
> >> >
> >> > Given that, and the points about other software not knowing aobut
> >> > /reserved-memory/, I don't think it makes sense to describe the region
> >> > in the memory nodes.
> >
> > [...]
> >
> > Hi Rob, Mark,
> >
> > Sorry its taken me so long to bump this thread.
> >
> > So I can have more confidence discussing this I have reviewed the
> > existing files in arch/arm/boot and arch/arm64/boot and done a bit of
> > analysis. My summary is below:
> >
> > In arch/arm/boot:
> >
> > - In arch/arm, Using /reserved-memory/ and /memreserve/ are very common
> >   case to reserve memory regions;
> > - /memreserve/ is mainly used to reserve memory for spin-table which
> >   used by SMP booting;
> > - /reserved-memory/ are used to reserve memory regions for security
> >   (TrustZone, secure RAM, etc), message transferring; So we can say
> >   usually use this method to reserve memory for device driver or other
> >   modules which maps these memory regions later.
> >
> > - There also have some DT files using memory node to carve out memory
> >   regions, but from roughly analysis we can see the most cases are
> >   introduced by memory controller for discontinuous memory space for
> >   DDR; or there have some silicon bug so some regions are not stable
> >   so need get rid of them (Armada-xp);
> >
> > In arch/arm64/boot:
> >
> > - There have no any file using /reserved-memory/;
> > - Several files are using /memreserve/ for spin-table boot method;
> > - ARM platforms (Juno, FVP) use memory nodes to carve out memory regions
> >   for security;
> > - DT files in arch/arm64 are quite consistent with Mark's suggestion.
> >
> > In my review I haven't really found anything that pushes me in one
> > direction or the other so, without consensus from the DT maintainers, I
> > really don't know how to progress here.
> 
> While I still think regions should be marked as reserved rather than
> carving up the memory node, it's not really worth holding up this
> platform further. The kernel can support either way.
> 
> However, Mark's argument AIUI was regions that are never accessed
> should be done the way you have done it. I don't think the mailbox
> region falls in this category. I'm not sure about OP-TEE, but we
> should have consistency for it if the OP-TEE driver needs to find its
> memory region. Cc'ing Joakim for comment.

My understanding was that the OP-TEE memory was that reserved for use by
the secure side (rather than a shared region). To avoid (non-secure)
speculative accesses triggering aborts from some trustzone controller,
and other issues, no memory reserved for secure side use should be
mapped by the non-secure side, and hence a memreserve is insufficient.
Hopefully Joakim can shed light on that.

There is a general problem with memreserve in that it is currently
ill-defined for arm and arm64, and its implications are not well
understood (by the nature of the ARM cache architecture, and problems
like mismatched aliases being not well understood). Due to this, I think
that its use should be avoided unless strictly necessary.

Having an alias as part of the linear mapping creates a number of
problems which are avoided most simply by never having that alias (i.e.
not describing the region as memory at all). Establishing that pattern
now saves us pain in future.

One major reason for memreserve being used was to allow the linear
mapping to be used to access objects. Practically every case has moved
over to using a dynamic mapping (fixmap, or {io,mem}remap), as that's
required to handle a number of edge cases.

For data structures (e.g. the DTB) that are only expected to be accessed
by the kernel, a memreserve is fine provided sufficient cache
maintenance has been performed already. For everything else (e.g.
anything shared with or owned by another agent), its use is questionable
at best (and I think the spin-table story is an unfortunate mistake that
we have to live with).

Thanks,
Mark.

[PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

[Leo Yan]

Add Victor as well.

On Mon, Jan 18, 2016 at 03:42:59PM +0000, Mark Rutland wrote:

[...]

> > While I still think regions should be marked as reserved rather than
> > carving up the memory node, it's not really worth holding up this
> > platform further. The kernel can support either way.
> > 
> > However, Mark's argument AIUI was regions that are never accessed
> > should be done the way you have done it. I don't think the mailbox
> > region falls in this category. I'm not sure about OP-TEE, but we
> > should have consistency for it if the OP-TEE driver needs to find its
> > memory region. Cc'ing Joakim for comment.
> 
> My understanding was that the OP-TEE memory was that reserved for use by
> the secure side (rather than a shared region). To avoid (non-secure)
> speculative accesses triggering aborts from some trustzone controller,
> and other issues, no memory reserved for secure side use should be
> mapped by the non-secure side, and hence a memreserve is insufficient.
> Hopefully Joakim can shed light on that.

I checked with Victor and OP-TEE memory region is reserved for below
purpose:

3f00,0000 - 3fff,ffff: OP-TEE's execution region
3ef0,0000 - 3eff,ffff: OP-TEE's shared memory, so this region will be
                       mapped both for secure world and normal world.
3e00,0000 - 3eef,ffff: Reserved for future using...

Not sure if this is same with ARM's Juno/TC2 platform? will leave
to Joakim for confirmation.

Here just wander if can directly carve out whole region at one time?
Or should distinguish the regions will be accessed by kernel later and
use different way to reserve them? Especially readed below Mark
brought cache alias issue, the most safe way is totally to avoid
linear mapping.

Thanks,
Leo Yan

> There is a general problem with memreserve in that it is currently
> ill-defined for arm and arm64, and its implications are not well
> understood (by the nature of the ARM cache architecture, and problems
> like mismatched aliases being not well understood). Due to this, I think
> that its use should be avoided unless strictly necessary.
> 
> Having an alias as part of the linear mapping creates a number of
> problems which are avoided most simply by never having that alias (i.e.
> not describing the region as memory at all). Establishing that pattern
> now saves us pain in future.
> 
> One major reason for memreserve being used was to allow the linear
> mapping to be used to access objects. Practically every case has moved
> over to using a dynamic mapping (fixmap, or {io,mem}remap), as that's
> required to handle a number of edge cases.
> 
> For data structures (e.g. the DTB) that are only expected to be accessed
> by the kernel, a memreserve is fine provided sufficient cache
> maintenance has been performed already. For everything else (e.g.
> anything shared with or owned by another agent), its use is questionable
> at best (and I think the spin-table story is an unfortunate mistake that
> we have to live with).
> 
> Thanks,
> Mark.

[PATCH 2/4] arm64: Kconfig: select sp804 timer for ARCH_HISI

[Leo Yan]

Select sp804 timer for ARCH_HISI, which is used as broadcast timer.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/Kconfig.platforms | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 23800a1..6d730fb 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -35,6 +35,7 @@ config ARCH_FSL_LS2085A
 
 config ARCH_HISI
 	bool "Hisilicon SoC Family"
+	select ARM_TIMER_SP804
 	help
 	  This enables support for Hisilicon ARMv8 SoC family
 
-- 
1.9.1

[PATCH 3/4] arm64: dts: add sp804 timer node for Hi6220

[Leo Yan]

Add sp804 timer for hi6220, so it can be used as broadcast timer.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index 3f03380..7edbe42 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -167,5 +167,14 @@
 			clocks = <&ao_ctrl 36>, <&ao_ctrl 36>;
 			clock-names = "uartclk", "apb_pclk";
 		};
+
+		dual_timer0: dual_timer at f8008000 {
+			compatible = "arm,sp804", "arm,primecell";
+			reg = <0x0 0xf8008000 0x0 0x1000>;
+			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ao_ctrl 27>, <&ao_ctrl 27>;
+			clock-names = "apb_pclk", "apb_pclk";
+		};
 	};
 };
-- 
1.9.1

[PATCH 3/4] arm64: dts: add sp804 timer node for Hi6220

[Rob Herring]

On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@linaro.org> wrote:
> Add sp804 timer for hi6220, so it can be used as broadcast timer.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 3f03380..7edbe42 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -167,5 +167,14 @@
>                         clocks = <&ao_ctrl 36>, <&ao_ctrl 36>;
>                         clock-names = "uartclk", "apb_pclk";
>                 };
> +
> +               dual_timer0: dual_timer at f8008000 {
> +                       compatible = "arm,sp804", "arm,primecell";
> +                       reg = <0x0 0xf8008000 0x0 0x1000>;
> +                       interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>,
> +                                    <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&ao_ctrl 27>, <&ao_ctrl 27>;
> +                       clock-names = "apb_pclk", "apb_pclk";

These should not have the same name and should be 3 clocks. The
binding doc is not clear what the names should be, but follow the
example.

The vexpress-v2p-ca9, hi3620 and hip04 are wrong too (in different ways).

Rob

> +               };
>         };
>  };
> --
> 1.9.1
>

[PATCH 4/4] arm64: dts: enable idle states for Hi6220

[Leo Yan]

Add cpu and cluster level's low power state for Hi6220.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index 7edbe42..e83802a 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -52,11 +52,35 @@
 			};
 		};
 
+		idle-states {
+			entry-method = "arm,psci";
+
+			CPU_SLEEP: cpu-sleep {
+				compatible = "arm,idle-state";
+				local-timer-stop;
+				arm,psci-suspend-param = <0x0010000>;
+				entry-latency-us = <700>;
+				exit-latency-us = <250>;
+				min-residency-us = <1000>;
+			};
+
+			CLUSTER_SLEEP: cluster-sleep {
+				compatible = "arm,idle-state";
+				local-timer-stop;
+				arm,psci-suspend-param = <0x1010000>;
+				entry-latency-us = <1000>;
+				exit-latency-us = <700>;
+				min-residency-us = <2700>;
+				wakeup-latency-us = <1500>;
+			};
+		};
+
 		cpu0: cpu at 0 {
 			compatible = "arm,cortex-a53", "arm,armv8";
 			device_type = "cpu";
 			reg = <0x0 0x0>;
 			enable-method = "psci";
+			cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
 		};
 
 		cpu1: cpu at 1 {
@@ -64,6 +88,7 @@
 			device_type = "cpu";
 			reg = <0x0 0x1>;
 			enable-method = "psci";
+			cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
 		};
 
 		cpu2: cpu at 2 {
@@ -71,6 +96,7 @@
 			device_type = "cpu";
 			reg = <0x0 0x2>;
 			enable-method = "psci";
+			cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
 		};
 
 		cpu3: cpu at 3 {
@@ -78,6 +104,7 @@
 			device_type = "cpu";
 			reg = <0x0 0x3>;
 			enable-method = "psci";
+			cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
 		};
 
 		cpu4: cpu at 100 {
@@ -85,6 +112,7 @@
 			device_type = "cpu";
 			reg = <0x0 0x100>;
 			enable-method = "psci";
+			cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
 		};
 
 		cpu5: cpu at 101 {
@@ -92,6 +120,7 @@
 			device_type = "cpu";
 			reg = <0x0 0x101>;
 			enable-method = "psci";
+			cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
 		};
 
 		cpu6: cpu at 102 {
@@ -99,6 +128,7 @@
 			device_type = "cpu";
 			reg = <0x0 0x102>;
 			enable-method = "psci";
+			cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
 		};
 
 		cpu7: cpu at 103 {
@@ -106,6 +136,7 @@
 			device_type = "cpu";
 			reg = <0x0 0x103>;
 			enable-method = "psci";
+			cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
 		};
 	};
 
-- 
1.9.1

[PATCH 4/4] arm64: dts: enable idle states for Hi6220

[Sudeep Holla]

On 09/10/15 05:36, Leo Yan wrote:
> Add cpu and cluster level's low power state for Hi6220.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>   arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 7edbe42..e83802a 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -52,11 +52,35 @@
>   			};
>   		};
>
> +		idle-states {
> +			entry-method = "arm,psci";

Please refer the bindings:           ^ should be just "psci"

Otherwise looks good.
Acked-by: Sudeep Holla <sudeep.holla@arm.com>

Regards,
Sudeep

[PATCH 4/4] arm64: dts: enable idle states for Hi6220

[Leo Yan]

Hi Sudeep,

On Fri, Oct 09, 2015 at 09:48:17AM +0100, Sudeep Holla wrote:
> 
> 
> On 09/10/15 05:36, Leo Yan wrote:
> >Add cpu and cluster level's low power state for Hi6220.
> >
> >Signed-off-by: Leo Yan <leo.yan@linaro.org>
> >---
> >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> >diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> >index 7edbe42..e83802a 100644
> >--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> >+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> >@@ -52,11 +52,35 @@
> >  			};
> >  		};
> >
> >+		idle-states {
> >+			entry-method = "arm,psci";
> 
> Please refer the bindings:           ^ should be just "psci"
> 
> Otherwise looks good.
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>

Will fix it, thanks for review.

Thanks,
Leo Yan