Message-ID: <20150827163108.GA31748@leverpostej>
Date:	Thu, 27 Aug 2015 17:31:09 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	Leo Yan <leo.yan@...aro.org>
Cc:	Haojian Zhuang <haojian.zhuang@...aro.org>,
	Leif Lindholm <leif.lindholm@...aro.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <Pawel.Moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Catalin Marinas <Catalin.Marinas@....com>,
	Will Deacon <Will.Deacon@....com>,
	Jassi Brar <jassisinghbrar@...il.com>,
	Bintian Wang <bintian.wang@...wei.com>,
	Yiping Xu <xuyiping@...ilicon.com>,
	Wei Xu <xuwei5@...ilicon.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"guodong.xu@...aro.org" <guodong.xu@...aro.org>,
	Jian Zhang <zhangjian001@...ilicon.com>,
	Zhenwei Wang <Zhenwei.wang@...ilicon.com>,
	Haoju Mo <mohaoju@...ilicon.com>,
	Dan Zhao <dan.zhao@...ilicon.com>,
	"kongfei@...ilicon.com" <kongfei@...ilicon.com>,
	Guangyue Zeng <zengguangyue@...ilicon.com>
Subject: Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

On Wed, Aug 26, 2015 at 07:59:50AM +0100, Leo Yan wrote:
> Hi Haojian,
> 
> On Wed, Aug 26, 2015 at 09:25:41AM +0800, Haojian Zhuang wrote:
> > On Wed, 2015-08-26 at 00:00 +0800, Leo Yan wrote:
> > > On Tue, Aug 25, 2015 at 09:43:14PM +0800, Haojian Zhuang wrote:
> > > > On Tue, 2015-08-25 at 11:42 +0100, Mark Rutland wrote:
> > > > > > > Are you then going to hack GRUB, release a special HiKey version of
> > > > > > > GRUB, not support any other versions, and still can your firmware
> > > > > > > UEFI?
> > > > > > 
> > > > > > I don't need to hack GRUB at all.
> > > > > 
> > > > > Then it is working for you by pure chance alone.
> > > > > 
> > > > > Please listen to the advice you are being given here; we're trying to
> > > > > ensure that your platform functions (and continues to function) as best
> > > > > it can.
> > > > 
> > > > Since we discussed a lot on this, let's make a conclusion on it.
> > > > 
> > > > 1. UEFI could append the reserved buffer in it's memory mapping.
> > > > 2. These reserved buffer must be declared in DT, since we also need to
> > > >    support non-UEFI (uboot) at the same time.
> > > > 3. Mailbox node should reference reserved buffer by phandle in DT. Then
> > > >    map the buffer as non-cacheable in driver.
> > > > 4. These reserved buffer must use "no-map" property since it should be
> > > >    non-cacheable in driver.
> > > 
> > > For more specific discussion for DTS, i list two options at here;
> > > 
> > > - Option 1: just simply reserve memory regions through memory node,
> > >   and mailbox node will directly use the buffer through reg ranges;
> > > 
> > > - Option 2: use reserved-memory and mailbox node will refer phandle
> > >   of reserved-memory;
> > > 
> > > These two options both can work well with UEFI and Uboot, but option 1
> > > is more simple and straightforward; so i personally prefer it. But
> > > look forwarding your guys' suggestion.
> > > 
> > > Option 1:
> > > 
> > > 	memory@0 {
> > > 		device_type = "memory";
> > > 		reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> > > 		      <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> > > 		      <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> > > 		      <0x00000000 0x07410000 0x00000000 0x38bf0000>;
> > > 	};
> > > 
> > >         [...]
> > > 
> > > 	mailbox: mailbox@...10000 {
> > > 		#mbox-cells = <1>;
> > > 		compatible = "hisilicon,hi6220-mbox";
> > > 		reg = <0x0 0xf7510000 0x0 0x1000>, /* IPC_S */
> > > 		      <0x0 0x06dff800 0x0 0x0800>; /* Mailbox buffer */
> > > 		interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> > > 	};
> > > 
> > > Option 2:
> > > 
> > > 	memory@0 {
> > > 		device_type = "memory";
> > > 		reg = <0x0 0x0 0x0 0x40000000>;
> > > 	};
> > > 
> > > 	reserved-memory {
> > > 		#address-cells = <2>;
> > > 		#size-cells = <2>;
> > > 		ranges;
> > > 
> > > 		mcu_reserved: mcu_reserved@...ff000 {
> > > 			no-map;
> > > 			reg = <0x0 0x06dff000 0x0 0x00001000>,	/* MCU mailbox buffer */
> > > 			      <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > > 			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > > 		};
> > > 	};
> > > 
> > >         [...]
> > > 
> > > 	mailbox: mailbox@...10000 {
> > > 		#mbox-cells = <1>;
> > > 		compatible = "hisilicon,hi6220-mbox";
> > > 		reg = <0x0 0xf7510000 0x0 0x1000>; /* IPC_S */
> > > 		memory-region = <&mcu_reserved>;   /* Mailbox buffer */
> > > 		interrupts = <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>;
> > > 	};
> > 
> > I prefer the second one. From my view, memory node should only describe
> > the hardware information of memory.
> 
> Yes, option 2 will be more simple for memory node.
> 
> But option 2 also will introduce complexity for mailbox node, due mailbox
> driver need use property "reg" and "memory-region" to sepeately depict
> the regions for mailbox's ipc and slots. If later mailbox is designed to
> use SRAM for both ipc and slots, then it will no matter with DDR anymore,
> in this case option 1 will easily switch to support it.
> 
> Another question is a general question: for Linux kernel, which is the
> best method to reserve memory regions? According to previous discussion,
> we can use /memory/ node or /reseved-memory/ node to reserve memory
> regions.

If the memory is truly reserved for a purpose and cannot be used for
anything else, I don't think it should be in the memory node at all, and
should be carved out. That aligns with what you'd do in UEFI (either not
listing the region in the memory map, or listing it with attributes such
that it may not be mapped and/or used).

I don't see much of a reason for /memreserve/, as it can cause issues
(by allowing the OS to map the region with cacheable attributes), and is
not as rigorously specified for ARM as it is for Power in ePAPR.

I understand that reserved-memory is for carving out (potentially
reusable) memory pools for devices or other special uses (perhaps a
panic log). Usually such memory may also be used by the kernel for its
own purposes if not presently required by the device.

Having an entry in reserved-memory does not necessitate the region also
appears in memory nodes, and if a region cannot be used by an OS (or
other software) for other purposes, I would not expect it to be describe
in any memory node. That will prevent other software (e.g. bootloaders)
from erroneously using the memory.

If you have a region described with no-map, I would expect that this
doesn't exist in any memory node or the UEFI memory map, and is only
under reserved-memory so it may be referred to by phandle in a
consistent manner.

> when review Juno's dts, i also see there have reserved 16MB DDR for secure
> world. If so, looks like /reserved-memory/ node is unnecessary. if have some
> specific scenarios will we use reserved-memory node to help reserve memory
> regions?

I'd expect shared DMA pools to appear in reserved-memory. The OS can
choose to use these or ignore them if it chooses (or is otherwise forced
to, e.g. were it loaded over one).

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists