Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1209742 > unrolled thread

[PATCH v1 0/3] mailbox: hisilicon: add Hi6220 mailbox driver

Back to article view | Back to linux.kernel

Contents

  [PATCH v1 0/3] mailbox: hisilicon: add Hi6220 mailbox driver Leo Yan <leo.yan@linaro.org> - 2015-08-19 11:40 +0200
    [PATCH v1 1/3] dt-bindings: mailbox: Document Hi6220 mailbox driver Leo Yan <leo.yan@linaro.org> - 2015-08-19 11:40 +0200
      Re: [PATCH v1 1/3] dt-bindings: mailbox: Document Hi6220 mailbox  driver Sudeep Holla <sudeep.holla@arm.com> - 2015-08-25 13:20 +0200
        Re: [PATCH v1 1/3] dt-bindings: mailbox: Document Hi6220 mailbox  driver Leo Yan <leo.yan@linaro.org> - 2015-08-25 15:10 +0200
    [PATCH v1 2/3] mailbox: Hi6220: add mailbox driver Leo Yan <leo.yan@linaro.org> - 2015-08-19 11:40 +0200
    [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Leo Yan <leo.yan@linaro.org> - 2015-08-19 11:40 +0200
      Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Mark Rutland <mark.rutland@arm.com> - 2015-08-21 20:50 +0200
        Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Leo Yan <leo.yan@linaro.org> - 2015-08-22 15:40 +0200
          Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Leo Yan <leo.yan@linaro.org> - 2015-08-24 05:30 +0200
        Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Leo Yan <leo.yan@linaro.org> - 2015-08-24 11:20 +0200
          Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Mark Rutland <mark.rutland@arm.com> - 2015-08-24 12:00 +0200
            Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Haojian Zhuang <haojian.zhuang@linaro.org> - 2015-08-24 12:30 +0200
              Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Leif Lindholm <leif.lindholm@linaro.org> - 2015-08-24 13:50 +0200
                Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Haojian Zhuang <haojian.zhuang@linaro.org> - 2015-08-25 10:20 +0200
                  Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Leif Lindholm <leif.lindholm@linaro.org> - 2015-08-25 11:50 +0200
                    Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Haojian Zhuang <haojian.zhuang@linaro.org> - 2015-08-25 12:20 +0200
                      Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Leif Lindholm <leif.lindholm@linaro.org> - 2015-08-25 12:50 +0200
                      Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Mark Rutland <mark.rutland@arm.com> - 2015-08-25 12:50 +0200
                        Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Haojian Zhuang <haojian.zhuang@linaro.org> - 2015-08-25 15:50 +0200
                          Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Leif Lindholm <leif.lindholm@linaro.org> - 2015-08-25 16:30 +0200
                            Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2015-08-25 17:00 +0200
                              Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Leif Lindholm <leif.lindholm@linaro.org> - 2015-08-25 17:40 +0200
                                Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2015-08-25 17:50 +0200
                                Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Haojian Zhuang <haojian.zhuang@linaro.org> - 2015-08-26 04:50 +0200
                          Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Leo Yan <leo.yan@linaro.org> - 2015-08-25 18:10 +0200
                            Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Haojian Zhuang <haojian.zhuang@linaro.org> - 2015-08-26 03:30 +0200
                              Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Leo Yan <leo.yan@linaro.org> - 2015-08-26 09:10 +0200
                                Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Mark Rutland <mark.rutland@arm.com> - 2015-08-27 18:40 +0200
                                  Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Leo Yan <leo.yan@linaro.org> - 2015-08-28 08:40 +0200
                              Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Daniel Thompson <daniel.thompson@linaro.org> - 2015-08-27 18:00 +0200
                              Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Mark Rutland <mark.rutland@arm.com> - 2015-08-27 18:50 +0200
              Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Mark Rutland <mark.rutland@arm.com> - 2015-08-24 14:50 +0200
                Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Haojian Zhuang <haojian.zhuang@linaro.org> - 2015-08-25 10:20 +0200
                  Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Mark Rutland <mark.rutland@arm.com> - 2015-08-25 13:10 +0200
      Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Sudeep Holla <sudeep.holla@arm.com> - 2015-08-25 13:40 +0200
        Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Leo Yan <leo.yan@linaro.org> - 2015-08-25 16:10 +0200
          Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node Sudeep Holla <sudeep.holla@arm.com> - 2015-08-25 16:20 +0200

Page 1 of 2  [1] 2  Next page →

#1209742 — [PATCH v1 0/3] mailbox: hisilicon: add Hi6220 mailbox driver

Hi6220 mailbox supports up to 32 channels. Each channel is unidirectional
with a maximum message size of 8 words. I/O is performed using register
access (there is no DMA) and the cell raises an interrupt when messages
are received.

This patch series is to implement Hi6220 mailbox driver. It registers
two channels into framework for communication with MCU, one is tx channel
and another is rx channel. Now mailbox driver is used to send message to
MCU to control dynamic voltage and frequency scaling for CPU, GPU and DDR.

Changes from RFC:
* According to Jassi's review, totally remove the abstract common driver
  layer and only commit driver dedicated for Hi6220
* According to Paul Bolle's review, fix typo issue for Kconfig and remove
  unnecessary dependency with OF and fix minor for mailbox driver
* Refine a little for dts nodes

Leo Yan (3):
  dt-bindings: mailbox: Document Hi6220 mailbox driver
  mailbox: Hi6220: add mailbox driver
  arm64: dts: add Hi6220 mailbox node

 .../bindings/mailbox/hisilicon,hi6220-mailbox.txt  |  57 +++
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts     |  20 +-
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi          |   8 +
 drivers/mailbox/Kconfig                            |   8 +
 drivers/mailbox/Makefile                           |   2 +
 drivers/mailbox/hi6220-mailbox.c                   | 513 +++++++++++++++++++++
 6 files changed, 605 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/hisilicon,hi6220-mailbox.txt
 create mode 100644 drivers/mailbox/hi6220-mailbox.c

-- 
1.9.1

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

[toc] | [next] | [standalone]

#1209745 — [PATCH v1 1/3] dt-bindings: mailbox: Document Hi6220 mailbox driver

Document the new compatible for Hisilicon Hi6220 mailbox driver.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../bindings/mailbox/hisilicon,hi6220-mailbox.txt  | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/hisilicon,hi6220-mailbox.txt

diff --git a/Documentation/devicetree/bindings/mailbox/hisilicon,hi6220-mailbox.txt b/Documentation/devicetree/bindings/mailbox/hisilicon,hi6220-mailbox.txt
new file mode 100644
index 0000000..3dfb0b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/hisilicon,hi6220-mailbox.txt
@@ -0,0 +1,57 @@
+Hisilicon Hi6220 Mailbox Driver
+===============================
+
+Hisilicon Hi6220 mailbox supports up to 32 channels. Each channel
+is unidirectional with a maximum message size of 8 words. I/O is
+performed using register access (there is no DMA) and the cell
+raises an interrupt when messages are received.
+
+Mailbox Device Node:
+====================
+
+Required properties:
+--------------------
+- compatible:		Shall be "hisilicon,hi6220-mbox"
+- reg:			Contains the mailbox register address range (base
+			address and length); the first item is for IPC
+			registers, the second item is shared buffer for
+			slots.
+- #mbox-cells		Common mailbox binding property to identify the number
+			of cells required for the mailbox specifier. Should be 1.
+- interrupts:		Contains the interrupt information for the mailbox
+			device. The format is dependent on which interrupt
+			controller the SoCs use.
+
+Example:
+--------
+
+	mailbox: mailbox@F7510000 {
+		#mbox-cells = <1>;
+		compatible = "hisilicon,hi6220-mbox";
+		reg = <0x0 0xF7510000 0x0 0x1000>, /* IPC_S */
+		      <0x0 0x06DFF800 0x0 0x0800>; /* Mailbox */
+		interrupt-parent = <&gic>;
+		interrupts = <0 94 4>;
+	};
+
+
+Mailbox client
+===============
+
+"mboxes" and the optional "mbox-names" (please see
+Documentation/devicetree/bindings/mailbox/mailbox.txt for details). Each value
+of the mboxes property should contain a phandle to the mailbox controller
+device node and second argument is the channel index. It must be 0 (hardware
+support only one channel). The equivalent "mbox-names" property value can be
+used to give a name to the communication channel to be used by the client user.
+
+Example:
+--------
+
+	stub_clock: stub_clock {
+		compatible = "hisilicon,hi6220-stub-clk";
+		hisilicon,hi6220-clk-sram = <&sram>;
+		#clock-cells = <1>;
+		mbox-names = "mbox-tx";
+		mboxes = <&mailbox 1>;
+	};
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1212969 — Re: [PATCH v1 1/3] dt-bindings: mailbox: Document Hi6220 mailbox driver

On 19/08/15 10:37, Leo Yan wrote:
> Document the new compatible for Hisilicon Hi6220 mailbox driver.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>   .../bindings/mailbox/hisilicon,hi6220-mailbox.txt  | 57 ++++++++++++++++++++++
>   1 file changed, 57 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/mailbox/hisilicon,hi6220-mailbox.txt
>
> diff --git a/Documentation/devicetree/bindings/mailbox/hisilicon,hi6220-mailbox.txt b/Documentation/devicetree/bindings/mailbox/hisilicon,hi6220-mailbox.txt
> new file mode 100644
> index 0000000..3dfb0b0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/hisilicon,hi6220-mailbox.txt
> @@ -0,0 +1,57 @@
> +Hisilicon Hi6220 Mailbox Driver
> +===============================
> +
> +Hisilicon Hi6220 mailbox supports up to 32 channels. Each channel
> +is unidirectional with a maximum message size of 8 words. I/O is
> +performed using register access (there is no DMA) and the cell
> +raises an interrupt when messages are received.
> +
> +Mailbox Device Node:
> +====================
> +
> +Required properties:
> +--------------------
> +- compatible:		Shall be "hisilicon,hi6220-mbox"
> +- reg:			Contains the mailbox register address range (base
> +			address and length); the first item is for IPC
> +			registers, the second item is shared buffer for
> +			slots.

Not sure if the shared buffer needs to be part of the controller binding
as it's not related to it. It's just agreement between the endpoints of
this mailbox on particular SoC and IMO has to part of the client binding.

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

[toc] | [prev] | [next] | [standalone]

#1213030 — Re: [PATCH v1 1/3] dt-bindings: mailbox: Document Hi6220 mailbox driver

Hi Sudeep,

Thanks for review, please see below comment.

On Tue, Aug 25, 2015 at 12:17:20PM +0100, Sudeep Holla wrote:
> On 19/08/15 10:37, Leo Yan wrote:
> >Document the new compatible for Hisilicon Hi6220 mailbox driver.
> >
> >Signed-off-by: Leo Yan <leo.yan@linaro.org>
> >---
> >  .../bindings/mailbox/hisilicon,hi6220-mailbox.txt  | 57 ++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mailbox/hisilicon,hi6220-mailbox.txt
> >
> >diff --git a/Documentation/devicetree/bindings/mailbox/hisilicon,hi6220-mailbox.txt b/Documentation/devicetree/bindings/mailbox/hisilicon,hi6220-mailbox.txt
> >new file mode 100644
> >index 0000000..3dfb0b0
> >--- /dev/null
> >+++ b/Documentation/devicetree/bindings/mailbox/hisilicon,hi6220-mailbox.txt
> >@@ -0,0 +1,57 @@
> >+Hisilicon Hi6220 Mailbox Driver
> >+===============================
> >+
> >+Hisilicon Hi6220 mailbox supports up to 32 channels. Each channel
> >+is unidirectional with a maximum message size of 8 words. I/O is
> >+performed using register access (there is no DMA) and the cell
> >+raises an interrupt when messages are received.
> >+
> >+Mailbox Device Node:
> >+====================
> >+
> >+Required properties:
> >+--------------------
> >+- compatible:		Shall be "hisilicon,hi6220-mbox"
> >+- reg:			Contains the mailbox register address range (base
> >+			address and length); the first item is for IPC
> >+			registers, the second item is shared buffer for
> >+			slots.
> 
> Not sure if the shared buffer needs to be part of the controller binding
> as it's not related to it. It's just agreement between the endpoints of
> this mailbox on particular SoC and IMO has to part of the client binding.

Yes, we need distinguish the buffer is really used for channel's
management or just only used for client. Here "shared buffer" is
used for channels' state machine, mode and raw data with 8 words.

So mailbox driver just read/write raw data according to client's
requirement, client will define their specific format for data
transcation.

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

[toc] | [prev] | [next] | [standalone]

#1209746 — [PATCH v1 2/3] mailbox: Hi6220: add mailbox driver

Add driver for Hi6220 mailbox, the mailbox communicates with MCU; for
sending data, it can support two methods for low level implementation:
one is to use interrupt as acknowledge, another is automatic mode which
without any acknowledge. These two methods have been supported in the
driver. For receiving data, it will depend on the interrupt to notify
the channel has incoming message; enhance rx channel's message queue,
which is based on the code in drivers/mailbox/omap-mailbox.c.

Now mailbox driver is used to send message to MCU to control dynamic
voltage and frequency scaling for CPU, GPU and DDR.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/mailbox/Kconfig          |   8 +
 drivers/mailbox/Makefile         |   2 +
 drivers/mailbox/hi6220-mailbox.c | 513 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 523 insertions(+)
 create mode 100644 drivers/mailbox/hi6220-mailbox.c

diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index e269f08..21b71dd 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -70,4 +70,12 @@ config BCM2835_MBOX
 	  the services of the Videocore. Say Y here if you want to use the
 	  BCM2835 Mailbox.
 
+config HI6220_MBOX
+	tristate "Hi6220 Mailbox"
+	depends on ARCH_HISI
+	help
+	  An implementation of the hi6220 mailbox. It is used to send message
+	  between application processors and MCU. Say Y here if you want to build
+	  the Hi6220 mailbox controller driver.
+
 endif
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 8e6d822..4ba9f5f 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -13,3 +13,5 @@ obj-$(CONFIG_PCC)		+= pcc.o
 obj-$(CONFIG_ALTERA_MBOX)	+= mailbox-altera.o
 
 obj-$(CONFIG_BCM2835_MBOX)	+= bcm2835-mailbox.o
+
+obj-$(CONFIG_HI6220_MBOX)	+= hi6220-mailbox.o
diff --git a/drivers/mailbox/hi6220-mailbox.c b/drivers/mailbox/hi6220-mailbox.c
new file mode 100644
index 0000000..1b2dc5b
--- /dev/null
+++ b/drivers/mailbox/hi6220-mailbox.c
@@ -0,0 +1,513 @@
+/*
+ * Hisilicon's Hi6220 mailbox driver
+ *
+ * RX channel's message queue is based on the code written in
+ * drivers/mailbox/omap-mailbox.c.
+ *
+ * Copyright (c) 2015 Hisilicon Limited.
+ * Copyright (c) 2015 Linaro Limited.
+ *
+ * Author: Leo Yan <leo.yan@linaro.org>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kfifo.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+
+#define HI6220_MBOX_CHAN_MAX		32
+#define HI6220_MBOX_CHAN_NUM		2
+#define HI6220_MBOX_CHAN_SLOT_SIZE	64
+
+#define HI6220_MBOX_RX			0x0
+#define HI6220_MBOX_TX			0x1
+
+/* Mailbox message length: 32 bytes */
+#define HI6220_MBOX_MSG_LEN		32
+
+/* Mailbox kfifo size */
+#define HI6220_MBOX_MSG_FIFO_SIZE	512
+
+/* Status & Mode Register */
+#define HI6220_MBOX_MODE_REG		0x0
+
+#define HI6220_MBOX_STATUS_MASK		(0xF << 4)
+#define HI6220_MBOX_STATUS_IDLE		(0x1 << 4)
+#define HI6220_MBOX_STATUS_TX		(0x2 << 4)
+#define HI6220_MBOX_STATUS_RX		(0x4 << 4)
+#define HI6220_MBOX_STATUS_ACK		(0x8 << 4)
+#define HI6220_MBOX_ACK_CONFIG_MASK	(0x1 << 0)
+#define HI6220_MBOX_ACK_AUTOMATIC	(0x1 << 0)
+#define HI6220_MBOX_ACK_IRQ		(0x0 << 0)
+
+/* Data Registers */
+#define HI6220_MBOX_DATA_REG(i)		(0x4 + (i << 2))
+
+/* ACPU Interrupt Register */
+#define HI6220_MBOX_ACPU_INT_RAW_REG	0x400
+#define HI6220_MBOX_ACPU_INT_MSK_REG	0x404
+#define HI6220_MBOX_ACPU_INT_STAT_REG	0x408
+#define HI6220_MBOX_ACPU_INT_CLR_REG	0x40c
+#define HI6220_MBOX_ACPU_INT_ENA_REG	0x500
+#define HI6220_MBOX_ACPU_INT_DIS_REG	0x504
+
+/* MCU Interrupt Register */
+#define HI6220_MBOX_MCU_INT_RAW_REG	0x420
+
+/* Core Id */
+#define HI6220_CORE_ACPU		0x0
+#define HI6220_CORE_MCU			0x2
+
+struct hi6220_mbox_queue {
+	struct kfifo fifo;
+	struct work_struct work;
+	struct mbox_chan *chan;
+	bool full;
+};
+
+struct hi6220_mbox_chan {
+
+	/*
+	 * Description for channel's hardware info:
+	 *  - direction;
+	 *  - peer core id for communication;
+	 *  - local irq vector or number;
+	 *  - remoted irq vector or number for peer core;
+	 */
+	unsigned int dir;
+	unsigned int peer_core;
+	unsigned int remote_irq;
+	unsigned int local_irq;
+
+	/*
+	 * Slot address is cached value derived from index
+	 * within buffer for every channel
+	 */
+	void __iomem *slot;
+
+	/* For rx's fifo operations */
+	struct hi6220_mbox_queue *mq;
+
+	struct hi6220_mbox *parent;
+};
+
+struct hi6220_mbox {
+	struct device *dev;
+
+	spinlock_t lock;
+
+	unsigned int irq;
+
+	/* flag of enabling tx's irq mode */
+	bool tx_irq_mode;
+
+	/* region for ipc event */
+	void __iomem *ipc;
+
+	/* region for share mem */
+	void __iomem *buf;
+
+	unsigned int chan_num;
+	struct hi6220_mbox_chan *mchan;
+
+	void *irq_map_chan[HI6220_MBOX_CHAN_MAX];
+	struct mbox_chan *chan;
+	struct mbox_controller controller;
+};
+
+static void hi6220_mbox_set_status(struct hi6220_mbox_chan *mchan, u32 val)
+{
+	u32 status;
+
+	status = readl(mchan->slot + HI6220_MBOX_MODE_REG);
+	status &= ~HI6220_MBOX_STATUS_MASK;
+	status |= val;
+	writel(status, mchan->slot + HI6220_MBOX_MODE_REG);
+}
+
+static void hi6220_mbox_set_mode(struct hi6220_mbox_chan *mchan, u32 val)
+{
+	u32 mode;
+
+	mode = readl(mchan->slot + HI6220_MBOX_MODE_REG);
+	mode &= ~HI6220_MBOX_ACK_CONFIG_MASK;
+	mode |= val;
+	writel(mode, mchan->slot + HI6220_MBOX_MODE_REG);
+}
+
+static bool hi6220_mbox_last_tx_done(struct mbox_chan *chan)
+{
+	struct hi6220_mbox_chan *mchan = chan->con_priv;
+	struct hi6220_mbox *mbox = mchan->parent;
+	u32 status;
+
+	/* Only set idle state for polling mode */
+	BUG_ON(mbox->tx_irq_mode);
+
+	status = readl(mchan->slot + HI6220_MBOX_MODE_REG);
+	status = status & HI6220_MBOX_STATUS_MASK;
+	return (status == HI6220_MBOX_STATUS_IDLE);
+}
+
+static int hi6220_mbox_send_data(struct mbox_chan *chan, void *msg)
+{
+	struct hi6220_mbox_chan *mchan = chan->con_priv;
+	struct hi6220_mbox *mbox = mchan->parent;
+	int irq = mchan->remote_irq;
+	u32 *buf = msg;
+	unsigned long flags;
+	int i;
+
+	hi6220_mbox_set_status(mchan, HI6220_MBOX_STATUS_TX);
+
+	if (mbox->tx_irq_mode)
+		hi6220_mbox_set_mode(mchan, HI6220_MBOX_ACK_IRQ);
+	else
+		hi6220_mbox_set_mode(mchan, HI6220_MBOX_ACK_AUTOMATIC);
+
+	for (i = 0; i < (HI6220_MBOX_MSG_LEN >> 2); i++)
+		writel(buf[i], mchan->slot + HI6220_MBOX_DATA_REG(i));
+
+	/* trigger remote request */
+	spin_lock_irqsave(&mbox->lock, flags);
+	writel(1 << irq, mbox->ipc + HI6220_MBOX_MCU_INT_RAW_REG);
+	spin_unlock_irqrestore(&mbox->lock, flags);
+	return 0;
+}
+
+static void hi6220_mbox_rx_work(struct work_struct *work)
+{
+	struct hi6220_mbox_queue *mq =
+			container_of(work, struct hi6220_mbox_queue, work);
+	struct mbox_chan *chan = mq->chan;
+	struct hi6220_mbox_chan *mchan = chan->con_priv;
+	struct hi6220_mbox *mbox = mchan->parent;
+	int irq = mchan->local_irq, len;
+	u32 msg[HI6220_MBOX_MSG_LEN >> 2];
+
+	while (kfifo_len(&mq->fifo) >= sizeof(msg)) {
+		len = kfifo_out(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
+		WARN_ON(len != sizeof(msg));
+
+		mbox_chan_received_data(chan, (void *)msg);
+		spin_lock_irq(&mbox->lock);
+		if (mq->full) {
+			mq->full = false;
+			writel(1 << irq,
+				mbox->ipc + HI6220_MBOX_ACPU_INT_ENA_REG);
+		}
+		spin_unlock_irq(&mbox->lock);
+	}
+}
+
+static void hi6220_mbox_tx_interrupt(struct mbox_chan *chan)
+{
+	struct hi6220_mbox_chan *mchan = chan->con_priv;
+	struct hi6220_mbox *mbox = mchan->parent;
+	int irq = mchan->local_irq;
+
+	writel(1 << irq, mbox->ipc + HI6220_MBOX_ACPU_INT_CLR_REG);
+	hi6220_mbox_set_status(mchan, HI6220_MBOX_STATUS_IDLE);
+
+	mbox_chan_txdone(chan, 0);
+}
+
+static void hi6220_mbox_rx_interrupt(struct mbox_chan *chan)
+{
+	struct hi6220_mbox_chan *mchan = chan->con_priv;
+	struct hi6220_mbox_queue *mq = mchan->mq;
+	struct hi6220_mbox *mbox = mchan->parent;
+	int irq = mchan->local_irq;
+	int msg[HI6220_MBOX_MSG_LEN >> 2];
+	int i, len;
+
+	if (unlikely(kfifo_avail(&mq->fifo) < sizeof(msg))) {
+		writel(1 << irq, mbox->ipc + HI6220_MBOX_ACPU_INT_DIS_REG);
+		mq->full = true;
+		goto nomem;
+	}
+
+	for (i = 0; i < (HI6220_MBOX_MSG_LEN >> 2); i++)
+		msg[i] = readl(mchan->slot + HI6220_MBOX_DATA_REG(i));
+
+	/* clear IRQ source */
+	writel(1 << irq, mbox->ipc + HI6220_MBOX_ACPU_INT_CLR_REG);
+
+	hi6220_mbox_set_status(mchan, HI6220_MBOX_STATUS_IDLE);
+
+	len = kfifo_in(&mq->fifo, (unsigned char *)&msg, sizeof(msg));
+	WARN_ON(len != sizeof(msg));
+
+nomem:
+	schedule_work(&mq->work);
+}
+
+static irqreturn_t hi6220_mbox_interrupt(int irq, void *p)
+{
+	struct hi6220_mbox *mbox = p;
+	struct hi6220_mbox_chan *mchan;
+	struct mbox_chan *chan;
+	unsigned int state;
+	unsigned int intr_bit;
+
+	state = readl(mbox->ipc + HI6220_MBOX_ACPU_INT_STAT_REG);
+	if (!state) {
+		dev_warn(mbox->dev, "%s: spurious interrupt\n",
+			 __func__);
+		return IRQ_HANDLED;
+	}
+
+	while (state) {
+		intr_bit = __ffs(state);
+		state &= (state - 1);
+
+		chan = mbox->irq_map_chan[intr_bit];
+		if (!chan) {
+			dev_warn(mbox->dev, "%s: unexpected irq vector %d\n",
+				 __func__, intr_bit);
+			continue;
+		}
+
+		mchan = chan->con_priv;
+		if (mchan->dir == HI6220_MBOX_TX)
+			hi6220_mbox_tx_interrupt(chan);
+		else
+			hi6220_mbox_rx_interrupt(chan);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static struct hi6220_mbox_queue *hi6220_mbox_queue_alloc(
+		struct mbox_chan *chan,
+		void (*work)(struct work_struct *))
+{
+	struct hi6220_mbox_queue *mq;
+
+	mq = kzalloc(sizeof(struct hi6220_mbox_queue), GFP_KERNEL);
+	if (!mq)
+		return NULL;
+
+	if (kfifo_alloc(&mq->fifo, HI6220_MBOX_MSG_FIFO_SIZE, GFP_KERNEL))
+		goto error;
+
+	mq->chan = chan;
+	INIT_WORK(&mq->work, work);
+	return mq;
+
+error:
+	kfree(mq);
+	return NULL;
+}
+
+static void hi6220_mbox_queue_free(struct hi6220_mbox_queue *mq)
+{
+	kfifo_free(&mq->fifo);
+	kfree(mq);
+}
+
+static int hi6220_mbox_startup(struct mbox_chan *chan)
+{
+	struct hi6220_mbox_chan *mchan = chan->con_priv;
+	struct hi6220_mbox *mbox = mchan->parent;
+	unsigned int irq = mchan->local_irq;
+	struct hi6220_mbox_queue *mq;
+	unsigned long flags;
+
+	mq = hi6220_mbox_queue_alloc(chan, hi6220_mbox_rx_work);
+	if (!mq)
+		return -ENOMEM;
+	mchan->mq = mq;
+	mbox->irq_map_chan[irq] = (void *)chan;
+
+	/* enable interrupt */
+	spin_lock_irqsave(&mbox->lock, flags);
+	writel(1 << irq, mbox->ipc + HI6220_MBOX_ACPU_INT_ENA_REG);
+	spin_unlock_irqrestore(&mbox->lock, flags);
+	return 0;
+}
+
+static void hi6220_mbox_shutdown(struct mbox_chan *chan)
+{
+	struct hi6220_mbox_chan *mchan = chan->con_priv;
+	struct hi6220_mbox *mbox = mchan->parent;
+	unsigned int irq = mchan->local_irq;
+	unsigned long flags;
+
+	/* disable interrupt */
+	spin_lock_irqsave(&mbox->lock, flags);
+	writel(1 << irq, mbox->ipc + HI6220_MBOX_ACPU_INT_DIS_REG);
+	spin_unlock_irqrestore(&mbox->lock, flags);
+
+	mbox->irq_map_chan[irq] = NULL;
+	flush_work(&mchan->mq->work);
+	hi6220_mbox_queue_free(mchan->mq);
+}
+
+static struct mbox_chan_ops hi6220_mbox_chan_ops = {
+	.send_data    = hi6220_mbox_send_data,
+	.startup      = hi6220_mbox_startup,
+	.shutdown     = hi6220_mbox_shutdown,
+	.last_tx_done = hi6220_mbox_last_tx_done,
+};
+
+static void hi6220_mbox_init_hw(struct hi6220_mbox *mbox)
+{
+	struct hi6220_mbox_chan init_data[HI6220_MBOX_CHAN_NUM] = {
+		{ HI6220_MBOX_RX, HI6220_CORE_MCU, 1, 10 },
+		{ HI6220_MBOX_TX, HI6220_CORE_MCU, 0, 11 },
+	};
+	struct hi6220_mbox_chan *mchan = mbox->mchan;
+	int i;
+
+	for (i = 0; i < HI6220_MBOX_CHAN_NUM; i++) {
+		memcpy(&mchan[i], &init_data[i], sizeof(*mchan));
+		mchan[i].slot = mbox->buf + HI6220_MBOX_CHAN_SLOT_SIZE * i;
+		mchan[i].parent = mbox;
+	}
+
+	/* mask and clear all interrupt vectors */
+	writel(0x0,  mbox->ipc + HI6220_MBOX_ACPU_INT_MSK_REG);
+	writel(~0x0, mbox->ipc + HI6220_MBOX_ACPU_INT_CLR_REG);
+
+	/* use interrupt for tx's ack */
+	mbox->tx_irq_mode = true;
+}
+
+static const struct of_device_id hi6220_mbox_of_match[] = {
+	{ .compatible = "hisilicon,hi6220-mbox", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, hi6220_mbox_of_match);
+
+static int hi6220_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct hi6220_mbox *mbox;
+	struct resource *res;
+	int i, err;
+
+	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
+	if (!mbox)
+		return -ENOMEM;
+
+	mbox->dev = dev;
+	mbox->chan_num = HI6220_MBOX_CHAN_NUM;
+	mbox->mchan = devm_kzalloc(dev,
+		mbox->chan_num * sizeof(*mbox->mchan), GFP_KERNEL);
+	if (!mbox->mchan)
+		return -ENOMEM;
+
+	mbox->chan = devm_kzalloc(dev,
+		mbox->chan_num * sizeof(*mbox->chan), GFP_KERNEL);
+	if (!mbox->chan)
+		return -ENOMEM;
+
+	mbox->irq = platform_get_irq(pdev, 0);
+	if (mbox->irq < 0)
+		return mbox->irq;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mbox->ipc = devm_ioremap_resource(dev, res);
+	if (IS_ERR(mbox->ipc)) {
+		dev_err(dev, "ioremap ipc failed\n");
+		return PTR_ERR(mbox->ipc);
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	mbox->buf = devm_ioremap_resource(dev, res);
+	if (IS_ERR(mbox->buf)) {
+		dev_err(dev, "ioremap buffer failed\n");
+		return PTR_ERR(mbox->buf);
+	}
+
+	err = devm_request_irq(dev, mbox->irq, hi6220_mbox_interrupt, 0,
+			dev_name(dev), mbox);
+	if (err) {
+		dev_err(dev, "Failed to register a mailbox IRQ handler: %d\n",
+			err);
+		return -ENODEV;
+	}
+
+	/* init hardware parameters */
+	hi6220_mbox_init_hw(mbox);
+
+	spin_lock_init(&mbox->lock);
+
+	for (i = 0; i < mbox->chan_num; i++) {
+		mbox->chan[i].con_priv = &mbox->mchan[i];
+		mbox->irq_map_chan[i] = NULL;
+	}
+
+	mbox->controller.dev = dev;
+	mbox->controller.chans = &mbox->chan[0];
+	mbox->controller.num_chans = mbox->chan_num;
+	mbox->controller.ops = &hi6220_mbox_chan_ops;
+
+	if (mbox->tx_irq_mode)
+		mbox->controller.txdone_irq = true;
+	else {
+		mbox->controller.txdone_poll = true;
+		mbox->controller.txpoll_period = 5;
+	}
+
+	err = mbox_controller_register(&mbox->controller);
+	if (err) {
+		dev_err(dev, "Failed to register mailbox %d\n", err);
+		return err;
+	}
+
+	platform_set_drvdata(pdev, mbox);
+	dev_info(dev, "Mailbox enabled\n");
+	return 0;
+}
+
+static int hi6220_mbox_remove(struct platform_device *pdev)
+{
+	struct hi6220_mbox *mbox = platform_get_drvdata(pdev);
+
+	mbox_controller_unregister(&mbox->controller);
+	return 0;
+}
+
+static struct platform_driver hi6220_mbox_driver = {
+	.driver = {
+		.name = "hi6220-mbox",
+		.owner = THIS_MODULE,
+		.of_match_table = hi6220_mbox_of_match,
+	},
+	.probe	= hi6220_mbox_probe,
+	.remove	= hi6220_mbox_remove,
+};
+
+static int __init hi6220_mbox_init(void)
+{
+	return platform_driver_register(&hi6220_mbox_driver);
+}
+core_initcall(hi6220_mbox_init);
+
+static void __exit hi6220_mbox_exit(void)
+{
+	platform_driver_unregister(&hi6220_mbox_driver);
+}
+module_exit(hi6220_mbox_exit);
+
+MODULE_AUTHOR("Leo Yan <leo.yan@linaro.org>");
+MODULE_DESCRIPTION("Hi6220 mailbox driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1209747 — [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

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

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

This patch reserves these memory regions and add device node for
mailbox in dts.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
index e36a539..d5470d3 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"
 
 / {
@@ -28,4 +25,21 @@
 		device_type = "memory";
 		reg = <0x0 0x0 0x0 0x40000000>;
 	};
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		mcu-buf@05e00000 {
+			no-map;
+			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
+			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
+		};
+
+		mbox-buf@06dff000 {
+			no-map;
+			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
+		};
+	};
 };
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index 3f03380..9ff25bc 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -167,5 +167,13 @@
 			clocks = <&ao_ctrl 36>, <&ao_ctrl 36>;
 			clock-names = "uartclk", "apb_pclk";
 		};
+
+		mailbox: mailbox@f7510000 {
+			#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>;
+		};
 	};
 };
-- 
1.9.1

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

[toc] | [prev] | [next] | [standalone]

#1211298 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

On Wed, Aug 19, 2015 at 10:37:35AM +0100, Leo Yan wrote:
> On Hi6220, below memory regions in DDR have specific purpose:
> 
>   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
>   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
>   0x06df,f000 - 0x06df,ffff: For mailbox message data.
> 
> This patch reserves these memory regions and add device node for
> mailbox in dts.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> index e36a539..d5470d3 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"
>  
>  / {
> @@ -28,4 +25,21 @@
>  		device_type = "memory";
>  		reg = <0x0 0x0 0x0 0x40000000>;
>  	};
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		mcu-buf@05e00000 {
> +			no-map;
> +			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> +			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> +		};
> +
> +		mbox-buf@06dff000 {
> +			no-map;
> +			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
> +		};
> +	};

As far as I can see, it would be simpler to simply carve these out of the
memory node.

I don't see why you need reserved-memory here, given you're not referring to
these regions by phandle anyway.

Thanks,
Mark.


>  };
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 3f03380..9ff25bc 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -167,5 +167,13 @@
>  			clocks = <&ao_ctrl 36>, <&ao_ctrl 36>;
>  			clock-names = "uartclk", "apb_pclk";
>  		};
> +
> +		mailbox: mailbox@f7510000 {
> +			#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>;
> +		};
>  	};
>  };
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1211458 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

Hi Mark,

On Fri, Aug 21, 2015 at 07:40:59PM +0100, Mark Rutland wrote:
> On Wed, Aug 19, 2015 at 10:37:35AM +0100, Leo Yan wrote:
> > On Hi6220, below memory regions in DDR have specific purpose:
> > 
> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> >   0x06df,f000 - 0x06df,ffff: For mailbox message data.
> > 
> > This patch reserves these memory regions and add device node for
> > mailbox in dts.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
> >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
> >  2 files changed, 25 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > index e36a539..d5470d3 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"
> >  
> >  / {
> > @@ -28,4 +25,21 @@
> >  		device_type = "memory";
> >  		reg = <0x0 0x0 0x0 0x40000000>;
> >  	};
> > +
> > +	reserved-memory {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +
> > +		mcu-buf@05e00000 {
> > +			no-map;
> > +			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > +			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > +		};
> > +
> > +		mbox-buf@06dff000 {
> > +			no-map;
> > +			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
> > +		};
> > +	};
> 
> As far as I can see, it would be simpler to simply carve these out of the
> memory node.

Will modify for MCU firmware buffer and section.

> I don't see why you need reserved-memory here, given you're not referring to
> these regions by phandle anyway.

mbox-buf is used by below mailbox's node, but the start address has
been truncated with 4KB alignment; so should keep it, right?

Thanks,
Leo Yan

> >  };
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > index 3f03380..9ff25bc 100644
> > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > @@ -167,5 +167,13 @@
> >  			clocks = <&ao_ctrl 36>, <&ao_ctrl 36>;
> >  			clock-names = "uartclk", "apb_pclk";
> >  		};
> > +
> > +		mailbox: mailbox@f7510000 {
> > +			#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>;
> > +		};
> >  	};
> >  };
> > -- 
> > 1.9.1
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1211745 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

Hi Mark,

On Sat, Aug 22, 2015 at 09:30:50PM +0800, Leo Yan wrote:
> On Fri, Aug 21, 2015 at 07:40:59PM +0100, Mark Rutland wrote:
> > On Wed, Aug 19, 2015 at 10:37:35AM +0100, Leo Yan wrote:
> > > On Hi6220, below memory regions in DDR have specific purpose:
> > > 
> > >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> > >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> > >   0x06df,f000 - 0x06df,ffff: For mailbox message data.
> > > 
> > > This patch reserves these memory regions and add device node for
> > > mailbox in dts.
> > > 
> > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
> > >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
> > >  2 files changed, 25 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > index e36a539..d5470d3 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"
> > >  
> > >  / {
> > > @@ -28,4 +25,21 @@
> > >  		device_type = "memory";
> > >  		reg = <0x0 0x0 0x0 0x40000000>;
> > >  	};
> > > +
> > > +	reserved-memory {
> > > +		#address-cells = <2>;
> > > +		#size-cells = <2>;
> > > +		ranges;
> > > +
> > > +		mcu-buf@05e00000 {
> > > +			no-map;
> > > +			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > > +			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > > +		};
> > > +
> > > +		mbox-buf@06dff000 {
> > > +			no-map;
> > > +			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
> > > +		};
> > > +	};
> > 
> > As far as I can see, it would be simpler to simply carve these out of the
> > memory node.
> 
> Will modify for MCU firmware buffer and section.
> 
> > I don't see why you need reserved-memory here, given you're not referring to
> > these regions by phandle anyway.
> 
> mbox-buf is used by below mailbox's node, but the start address has
> been truncated with 4KB alignment; so should keep it, right?

I think i got your point, all these nodes can be removed and just use
memory node to carve them out; but currently i saw the memory node
cannot be passed correctly from UEFI to kernel, we will check for
this. So will follow your suggestion if without any unknown reason.

Thanks,
Leo Yan

> > >  };
> > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > > index 3f03380..9ff25bc 100644
> > > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> > > @@ -167,5 +167,13 @@
> > >  			clocks = <&ao_ctrl 36>, <&ao_ctrl 36>;
> > >  			clock-names = "uartclk", "apb_pclk";
> > >  		};
> > > +
> > > +		mailbox: mailbox@f7510000 {
> > > +			#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>;
> > > +		};
> > >  	};
> > >  };
> > > -- 
> > > 1.9.1
> > > 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1211961 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

Hi Mark,

On Fri, Aug 21, 2015 at 07:40:59PM +0100, Mark Rutland wrote:
> On Wed, Aug 19, 2015 at 10:37:35AM +0100, Leo Yan wrote:
> > On Hi6220, below memory regions in DDR have specific purpose:
> > 
> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> >   0x06df,f000 - 0x06df,ffff: For mailbox message data.
> > 
> > This patch reserves these memory regions and add device node for
> > mailbox in dts.
> > 
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
> >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
> >  2 files changed, 25 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > index e36a539..d5470d3 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"
> >  
> >  / {
> > @@ -28,4 +25,21 @@
> >  		device_type = "memory";
> >  		reg = <0x0 0x0 0x0 0x40000000>;
> >  	};
> > +
> > +	reserved-memory {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +
> > +		mcu-buf@05e00000 {
> > +			no-map;
> > +			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > +			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > +		};
> > +
> > +		mbox-buf@06dff000 {
> > +			no-map;
> > +			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
> > +		};
> > +	};
> 
> As far as I can see, it would be simpler to simply carve these out of the
> memory node.
> 
> I don't see why you need reserved-memory here, given you're not referring to
> these regions by phandle anyway.

- Now we have enabled EFI_STUB, so the memory node will be removed in
  kernel:
    efi_entry()
      \-> allocate_new_fdt_and_exit_boot()
            \-> update_fdt();

  Finally in kernel it cannot use memory node to carve out reseved
  memory regions.

- On the other hand, DTS's the memory node is to "describes the
  physical memory layout for the system"; so it's better to use it only
  to describe the hardware info for memory. We can use reserved-memory
  to help manage the memory regions which are reserved from software
  perspective.

According to upper info, we still need to use reserved-memory node to
depict the reserved memory regions. i have no knowledge about EFI_STUB,
so please confirm or correct as needed.

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

[toc] | [prev] | [next] | [standalone]

#1212049 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

On Mon, Aug 24, 2015 at 10:18:45AM +0100, Leo Yan wrote:
> Hi Mark,
> 
> On Fri, Aug 21, 2015 at 07:40:59PM +0100, Mark Rutland wrote:
> > On Wed, Aug 19, 2015 at 10:37:35AM +0100, Leo Yan wrote:
> > > On Hi6220, below memory regions in DDR have specific purpose:
> > > 
> > >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> > >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> > >   0x06df,f000 - 0x06df,ffff: For mailbox message data.
> > > 
> > > This patch reserves these memory regions and add device node for
> > > mailbox in dts.
> > > 
> > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
> > >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
> > >  2 files changed, 25 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > index e36a539..d5470d3 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"
> > >  
> > >  / {
> > > @@ -28,4 +25,21 @@
> > >  		device_type = "memory";
> > >  		reg = <0x0 0x0 0x0 0x40000000>;
> > >  	};
> > > +
> > > +	reserved-memory {
> > > +		#address-cells = <2>;
> > > +		#size-cells = <2>;
> > > +		ranges;
> > > +
> > > +		mcu-buf@05e00000 {
> > > +			no-map;
> > > +			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > > +			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > > +		};
> > > +
> > > +		mbox-buf@06dff000 {
> > > +			no-map;
> > > +			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
> > > +		};
> > > +	};
> > 
> > As far as I can see, it would be simpler to simply carve these out of the
> > memory node.
> > 
> > I don't see why you need reserved-memory here, given you're not referring to
> > these regions by phandle anyway.
> 
> - Now we have enabled EFI_STUB, so the memory node will be removed in
>   kernel:
>     efi_entry()
>       \-> allocate_new_fdt_and_exit_boot()
>             \-> update_fdt();
> 
>   Finally in kernel it cannot use memory node to carve out reseved
>   memory regions.
> 
> - On the other hand, DTS's the memory node is to "describes the
>   physical memory layout for the system"; so it's better to use it only
>   to describe the hardware info for memory. We can use reserved-memory
>   to help manage the memory regions which are reserved from software
>   perspective.

The fact that you have no-map means that the memory should not be
described to the kernel as mappable in the first place. It's wrong to
place such memory in the memory node, even if listed in reserved-memory.

If your EFI memory map describes the memory as mappable, it is wrong.

> According to upper info, we still need to use reserved-memory node to
> depict the reserved memory regions. i have no knowledge about EFI_STUB,
> so please confirm or correct as needed.

If the memory shouldn't be mapped, it should neither be in the memory
node nor EFI memory map (with attributes allowing it to be mapped) to
begin with.

As far as I can see you do not need to use reserved-memory.

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

[toc] | [prev] | [next] | [standalone]

#1212065 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

On Mon, 2015-08-24 at 10:51 +0100, Mark Rutland wrote:
> On Mon, Aug 24, 2015 at 10:18:45AM +0100, Leo Yan wrote:
> > Hi Mark,
> > 
> > On Fri, Aug 21, 2015 at 07:40:59PM +0100, Mark Rutland wrote:
> > > On Wed, Aug 19, 2015 at 10:37:35AM +0100, Leo Yan wrote:
> > > > On Hi6220, below memory regions in DDR have specific purpose:
> > > > 
> > > >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> > > >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> > > >   0x06df,f000 - 0x06df,ffff: For mailbox message data.
> > > > 
> > > > This patch reserves these memory regions and add device node for
> > > > mailbox in dts.
> > > > 
> > > > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > > > ---
> > > >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 20 +++++++++++++++++---
> > > >  arch/arm64/boot/dts/hisilicon/hi6220.dtsi      |  8 ++++++++
> > > >  2 files changed, 25 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > > index e36a539..d5470d3 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"
> > > >  
> > > >  / {
> > > > @@ -28,4 +25,21 @@
> > > >  		device_type = "memory";
> > > >  		reg = <0x0 0x0 0x0 0x40000000>;
> > > >  	};
> > > > +
> > > > +	reserved-memory {
> > > > +		#address-cells = <2>;
> > > > +		#size-cells = <2>;
> > > > +		ranges;
> > > > +
> > > > +		mcu-buf@05e00000 {
> > > > +			no-map;
> > > > +			reg = <0x0 0x05e00000 0x0 0x00100000>,	/* MCU firmware buffer */
> > > > +			      <0x0 0x0740f000 0x0 0x00001000>;	/* MCU firmware section */
> > > > +		};
> > > > +
> > > > +		mbox-buf@06dff000 {
> > > > +			no-map;
> > > > +			reg = <0x0 0x06dff000 0x0 0x00001000>;	/* Mailbox message buf */
> > > > +		};
> > > > +	};
> > > 
> > > As far as I can see, it would be simpler to simply carve these out of the
> > > memory node.
> > > 
> > > I don't see why you need reserved-memory here, given you're not referring to
> > > these regions by phandle anyway.
> > 
> > - Now we have enabled EFI_STUB, so the memory node will be removed in
> >   kernel:
> >     efi_entry()
> >       \-> allocate_new_fdt_and_exit_boot()
> >             \-> update_fdt();
> > 
> >   Finally in kernel it cannot use memory node to carve out reseved
> >   memory regions.
> > 
> > - On the other hand, DTS's the memory node is to "describes the
> >   physical memory layout for the system"; so it's better to use it only
> >   to describe the hardware info for memory. We can use reserved-memory
> >   to help manage the memory regions which are reserved from software
> >   perspective.
> 
> The fact that you have no-map means that the memory should not be
> described to the kernel as mappable in the first place. It's wrong to
> place such memory in the memory node, even if listed in reserved-memory.
> 
> If your EFI memory map describes the memory as mappable, it is wrong.

When kernel is working, kernel will create its own page table based on
UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
be moved to reserved memblock. Why is it wrong?

In the second, UEFI is firmware. When it's stable, nobody should change
it without any reason. These reserved memory are used in mailbox driver.
Look. It's driver, so it could be changed at any time. Why do you want
to UEFI knowing this memory range? Do you hope UEFI to change when
mailbox driver is changed?

> 
> > According to upper info, we still need to use reserved-memory node to
> > depict the reserved memory regions. i have no knowledge about EFI_STUB,
> > so please confirm or correct as needed.
> 
> If the memory shouldn't be mapped, it should neither be in the memory
> node nor EFI memory map (with attributes allowing it to be mapped) to
> begin with.

As I said above, kernel will create its own page table. When kernel's
page table is working, UEFI's page table is destroying. So the memory
won't be mapped twice at the same time. What's wrong?
> 
> As far as I can see you do not need to use reserved-memory.

1. Are we talking on the same thing? Leo already mentioned that all
memory node in DTB will be destroyed by kernel when EFI_STUB is enabled
on arm. Did you read the source code after his reply?
And you suggested that Leo to use discrete memory region in DTB. It is
really wrong. Kernel only gets memory map information from UEFI, not
DTB.

2. The working flow is in below.
   a. Kernel gets memory map information from UEFI.
   b. Kernel loads the memory reserved information from DTB.

3. Do you mean the reserved-memory is totally wrong? If it's wrong,
please submit patches to remove all reserved-memory in linux kernel
first.

4. Again and again. Memory node should be only used to describe the
RAM information.

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

[toc] | [prev] | [next] | [standalone]

#1212092 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

On Mon, Aug 24, 2015 at 06:19:56PM +0800, Haojian Zhuang wrote:
> > If your EFI memory map describes the memory as mappable, it is wrong.
> 
> When kernel is working, kernel will create its own page table based on
> UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
> be moved to reserved memblock. Why is it wrong?
> 
> In the second, UEFI is firmware. When it's stable, nobody should change
> it without any reason.

Much like the memory map.

> These reserved memory are used in mailbox driver.
> Look. It's driver, so it could be changed at any time.

No, it is a set of regions of memory set aside for use by a different
master in the system as well as communications with that master.

The fact that there is a driver somewhere that is aware of this is
entirely beside the point. All agents in the system must adher to this
protocol.

> Why do you want
> to UEFI knowing this memory range? Do you hope UEFI to change when
> mailbox driver is changed?

Yes.

UEFI is a runtime environment. Having random magic areas not to be
touched will cause random pieces of software running under it to break
horribly or break other things horribly.
Unless you mark them as reserved in the UEFI memory map.
At which point the Linux kernel will automatically ignore them, and
the proposed patch is redundant.

So, yes, if you want a system that can boot reliably, run testsuites
(like SCT or FWTS), run applications (like fastboot ... or the EFI
stub kernel itself), then any memory regions that is reserved for
mailbox communication (or other masters in the system) _must_ be
marked in the EFI memory map.

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

[toc] | [prev] | [next] | [standalone]

#1212797 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

On Mon, 2015-08-24 at 12:49 +0100, Leif Lindholm wrote:
> On Mon, Aug 24, 2015 at 06:19:56PM +0800, Haojian Zhuang wrote:
> > > If your EFI memory map describes the memory as mappable, it is wrong.
> > 
> > When kernel is working, kernel will create its own page table based on
> > UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
> > be moved to reserved memblock. Why is it wrong?
> > 
> > In the second, UEFI is firmware. When it's stable, nobody should change
> > it without any reason.
> 
> Much like the memory map.
> 
> > These reserved memory are used in mailbox driver.
> > Look. It's driver, so it could be changed at any time.
> 
> No, it is a set of regions of memory set aside for use by a different
> master in the system as well as communications with that master.
> 
> The fact that there is a driver somewhere that is aware of this is
> entirely beside the point. All agents in the system must adher to this
> protocol.
> 
> > Why do you want
> > to UEFI knowing this memory range? Do you hope UEFI to change when
> > mailbox driver is changed?
> 
> Yes.
> 
> UEFI is a runtime environment. Having random magic areas not to be
> touched will cause random pieces of software running under it to break
> horribly or break other things horribly.
> Unless you mark them as reserved in the UEFI memory map.
> At which point the Linux kernel will automatically ignore them, and
> the proposed patch is redundant.
> 
> So, yes, if you want a system that can boot reliably, run testsuites
> (like SCT or FWTS), run applications (like fastboot ... or the EFI
> stub kernel itself), then any memory regions that is reserved for
> mailbox communication (or other masters in the system) _must_ be
> marked in the EFI memory map.

1. We need support both UEFI and uboot. So the reserved buffer have to
be declared in DTB since they are used by kernel driver, not UEFI.

2. UEFI just loads grub. It's no time to run any other custom EFI
application.

Regards
Haojian

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

[toc] | [prev] | [next] | [standalone]

#1212916 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

On Tue, Aug 25, 2015 at 04:13:47PM +0800, Haojian Zhuang wrote:
> On Mon, 2015-08-24 at 12:49 +0100, Leif Lindholm wrote:
> > On Mon, Aug 24, 2015 at 06:19:56PM +0800, Haojian Zhuang wrote:
> > > > If your EFI memory map describes the memory as mappable, it is wrong.
> > > 
> > > When kernel is working, kernel will create its own page table based on
> > > UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
> > > be moved to reserved memblock. Why is it wrong?
> > > 
> > > In the second, UEFI is firmware. When it's stable, nobody should change
> > > it without any reason.
> > 
> > Much like the memory map.
> > 
> > > These reserved memory are used in mailbox driver.
> > > Look. It's driver, so it could be changed at any time.
> > 
> > No, it is a set of regions of memory set aside for use by a different
> > master in the system as well as communications with that master.
> > 
> > The fact that there is a driver somewhere that is aware of this is
> > entirely beside the point. All agents in the system must adher to this
> > protocol.
> > 
> > > Why do you want
> > > to UEFI knowing this memory range? Do you hope UEFI to change when
> > > mailbox driver is changed?
> > 
> > Yes.
> > 
> > UEFI is a runtime environment. Having random magic areas not to be
> > touched will cause random pieces of software running under it to break
> > horribly or break other things horribly.
> > Unless you mark them as reserved in the UEFI memory map.
> > At which point the Linux kernel will automatically ignore them, and
> > the proposed patch is redundant.
> > 
> > So, yes, if you want a system that can boot reliably, run testsuites
> > (like SCT or FWTS), run applications (like fastboot ... or the EFI
> > stub kernel itself), then any memory regions that is reserved for
> > mailbox communication (or other masters in the system) _must_ be
> > marked in the EFI memory map.
> 
> 1. We need support both UEFI and uboot. So the reserved buffer have to
> be declared in DTB since they are used by kernel driver, not UEFI.

The buffer may need to be declared in DTB also, but it most certanily
needs to be declared in UEFI.

And for the U-Boot case, since it is not memory available to Linux, it
should not be declared as "memory".

> 2. UEFI just loads grub. It's no time to run any other custom EFI
> application.

Apart from being completely irrelevant, how are you intending to
validate that GRUB never touches these memory regions?

Build a version once, test it, and hope the results remain valid
forever? And then when you move the regions and the previously working
GRUB now tramples all over them? Or when something changes in upstream
GRUB and its memory allocations drifts into the secretly untouchable
regions?

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?

Repeat again and again for any other UEFI applications - including
fastboot, SCT, FWTS and the UEFI stub kernel.

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

[toc] | [prev] | [next] | [standalone]

#1212928 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

On Tue, 2015-08-25 at 10:46 +0100, Leif Lindholm wrote:
> On Tue, Aug 25, 2015 at 04:13:47PM +0800, Haojian Zhuang wrote:
> > On Mon, 2015-08-24 at 12:49 +0100, Leif Lindholm wrote:
> > > On Mon, Aug 24, 2015 at 06:19:56PM +0800, Haojian Zhuang wrote:
> > > > > If your EFI memory map describes the memory as mappable, it is wrong.
> > > > 
> > > > When kernel is working, kernel will create its own page table based on
> > > > UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
> > > > be moved to reserved memblock. Why is it wrong?
> > > > 
> > > > In the second, UEFI is firmware. When it's stable, nobody should change
> > > > it without any reason.
> > > 
> > > Much like the memory map.
> > > 
> > > > These reserved memory are used in mailbox driver.
> > > > Look. It's driver, so it could be changed at any time.
> > > 
> > > No, it is a set of regions of memory set aside for use by a different
> > > master in the system as well as communications with that master.
> > > 
> > > The fact that there is a driver somewhere that is aware of this is
> > > entirely beside the point. All agents in the system must adher to this
> > > protocol.
> > > 
> > > > Why do you want
> > > > to UEFI knowing this memory range? Do you hope UEFI to change when
> > > > mailbox driver is changed?
> > > 
> > > Yes.
> > > 
> > > UEFI is a runtime environment. Having random magic areas not to be
> > > touched will cause random pieces of software running under it to break
> > > horribly or break other things horribly.
> > > Unless you mark them as reserved in the UEFI memory map.
> > > At which point the Linux kernel will automatically ignore them, and
> > > the proposed patch is redundant.
> > > 
> > > So, yes, if you want a system that can boot reliably, run testsuites
> > > (like SCT or FWTS), run applications (like fastboot ... or the EFI
> > > stub kernel itself), then any memory regions that is reserved for
> > > mailbox communication (or other masters in the system) _must_ be
> > > marked in the EFI memory map.
> > 
> > 1. We need support both UEFI and uboot. So the reserved buffer have to
> > be declared in DTB since they are used by kernel driver, not UEFI.
> 
> The buffer may need to be declared in DTB also, but it most certanily
> needs to be declared in UEFI.
> 
> And for the U-Boot case, since it is not memory available to Linux, it
> should not be declared as "memory".

Something are messed at here. We have these buffer are used in mailbox.
They should be allocated as non-cacheable.

If these buffers are contained in memory memblock in kernel, it means
that they exist in kernel page table with cachable property. When it's
used in mailbox driver with non-cachable property, it'll only cause
cache maintenance issue. So Leo declared these buffers as reserved
in DT with "no-map" property. It's the key. It could avoid the cache
maintenance issue.

> 
> > 2. UEFI just loads grub. It's no time to run any other custom EFI
> > application.
> 
> Apart from being completely irrelevant, how are you intending to
> validate that GRUB never touches these memory regions?
> 

GRUB is just a part of bootloader. When linux kernel is running,
who cares GRUB? GRUB's lifetime is already finished.

By the way, UEFI code region is at [0x3Dxx_xxxx, 0x3DFF_FFFF]. Those
mailbox buffer is in [0x05e0_xxxx, 0x06f0_xxxx]. Then I can make sure
UEFI won't touch the reserved buffer. Even if UEFI touched the reserved
buffer, is it an issue? Definitely it's not. UEFI's lifetime is end
when linux kernel is running at hikey. Even if UEFI runtime service
is enabled, the runtime data area is at [0x38xx_xxxx, 0x38xx_xxxx].

> Build a version once, test it, and hope the results remain valid
> forever? And then when you move the regions and the previously working
> GRUB now tramples all over them? Or when something changes in upstream
> GRUB and its memory allocations drifts into the secretly untouchable
> regions?

As I said above, UEFI won't touch it. And even UEFI touch it, kernel
doesn't care since UEFI's lifetime is end.

> 
> 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.

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

[toc] | [prev] | [next] | [standalone]

#1212945 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

On Tue, Aug 25, 2015 at 06:15:10PM +0800, Haojian Zhuang wrote:
> > > 1. We need support both UEFI and uboot. So the reserved buffer have to
> > > be declared in DTB since they are used by kernel driver, not UEFI.
> > 
> > The buffer may need to be declared in DTB also, but it most certanily
> > needs to be declared in UEFI.
> > 
> > And for the U-Boot case, since it is not memory available to Linux, it
> > should not be declared as "memory".
> 
> Something are messed at here. We have these buffer are used in mailbox.
> They should be allocated as non-cacheable.

That is a completely different issue, and if that is not currently
possible, then we need to fix that. But it needs to be fixed in the
right place.

> If these buffers are contained in memory memblock in kernel, it means
> that they exist in kernel page table with cachable property. When it's
> used in mailbox driver with non-cachable property, it'll only cause
> cache maintenance issue. So Leo declared these buffers as reserved
> in DT with "no-map" property. It's the key. It could avoid the cache
> maintenance issue.

Yes, when not booting with UEFI.

> > > 2. UEFI just loads grub. It's no time to run any other custom EFI
> > > application.
> > 
> > Apart from being completely irrelevant, how are you intending to
> > validate that GRUB never touches these memory regions?
> 
> GRUB is just a part of bootloader. When linux kernel is running,
> who cares GRUB? GRUB's lifetime is already finished.

We don't care once Linux is running - we care between UEFI boot
services starting and Linux memblock being initialised.

> By the way, UEFI code region is at [0x3Dxx_xxxx, 0x3DFF_FFFF]. Those
> mailbox buffer is in [0x05e0_xxxx, 0x06f0_xxxx]. Then I can make sure
> UEFI won't touch the reserved buffer.

And if a UEFI application explicitly requests to map an area
elsewhere, will your UEFI reject that request? How will it do that
without having information in its memory map about areas it must not
access?

> Even if UEFI touched the reserved
> buffer, is it an issue? Definitely it's not. UEFI's lifetime is end
> when linux kernel is running at hikey. Even if UEFI runtime service
> is enabled, the runtime data area is at [0x38xx_xxxx, 0x38xx_xxxx].

The runtime data area is currently, in your current image, at
[0x38xx_xxxx, 0x38xx_xxxx].

What happens if a UEFI application registers a configuration table?
Or registers a protocol for use at runtime?

Areas of memory that are not available for UEFI _must_ be marked as
such in the UEFI memory map. Once they are, we can deal with them in
the kernel. If this is not currently being done, that is a bug that
needs fixing.

> > Build a version once, test it, and hope the results remain valid
> > forever? And then when you move the regions and the previously working
> > GRUB now tramples all over them? Or when something changes in upstream
> > GRUB and its memory allocations drifts into the secretly untouchable
> > regions?
> 
> As I said above, UEFI won't touch it. And even UEFI touch it, kernel
> doesn't care since UEFI's lifetime is end.

UEFI's lifetime doesn't end until reset.

> > 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.

You will if you're running it under a "UEFI" which has areas you can't
touch and aren't telling it about that.

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

[toc] | [prev] | [next] | [standalone]

#1212951 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

On Tue, Aug 25, 2015 at 11:15:10AM +0100, Haojian Zhuang wrote:
> On Tue, 2015-08-25 at 10:46 +0100, Leif Lindholm wrote:
> > On Tue, Aug 25, 2015 at 04:13:47PM +0800, Haojian Zhuang wrote:
> > > On Mon, 2015-08-24 at 12:49 +0100, Leif Lindholm wrote:
> > > > On Mon, Aug 24, 2015 at 06:19:56PM +0800, Haojian Zhuang wrote:
> > > > > > If your EFI memory map describes the memory as mappable, it is wrong.
> > > > > 
> > > > > When kernel is working, kernel will create its own page table based on
> > > > > UEFI memory map. Since it's reserved in DTS file as Leo's patch, it'll
> > > > > be moved to reserved memblock. Why is it wrong?
> > > > > 
> > > > > In the second, UEFI is firmware. When it's stable, nobody should change
> > > > > it without any reason.
> > > > 
> > > > Much like the memory map.
> > > > 
> > > > > These reserved memory are used in mailbox driver.
> > > > > Look. It's driver, so it could be changed at any time.
> > > > 
> > > > No, it is a set of regions of memory set aside for use by a different
> > > > master in the system as well as communications with that master.
> > > > 
> > > > The fact that there is a driver somewhere that is aware of this is
> > > > entirely beside the point. All agents in the system must adher to this
> > > > protocol.
> > > > 
> > > > > Why do you want
> > > > > to UEFI knowing this memory range? Do you hope UEFI to change when
> > > > > mailbox driver is changed?
> > > > 
> > > > Yes.
> > > > 
> > > > UEFI is a runtime environment. Having random magic areas not to be
> > > > touched will cause random pieces of software running under it to break
> > > > horribly or break other things horribly.
> > > > Unless you mark them as reserved in the UEFI memory map.
> > > > At which point the Linux kernel will automatically ignore them, and
> > > > the proposed patch is redundant.
> > > > 
> > > > So, yes, if you want a system that can boot reliably, run testsuites
> > > > (like SCT or FWTS), run applications (like fastboot ... or the EFI
> > > > stub kernel itself), then any memory regions that is reserved for
> > > > mailbox communication (or other masters in the system) _must_ be
> > > > marked in the EFI memory map.
> > > 
> > > 1. We need support both UEFI and uboot. So the reserved buffer have to
> > > be declared in DTB since they are used by kernel driver, not UEFI.
> > 
> > The buffer may need to be declared in DTB also, but it most certanily
> > needs to be declared in UEFI.
> > 
> > And for the U-Boot case, since it is not memory available to Linux, it
> > should not be declared as "memory".
> 
> Something are messed at here. We have these buffer are used in mailbox.
> They should be allocated as non-cacheable.
> 
> If these buffers are contained in memory memblock in kernel, it means
> that they exist in kernel page table with cachable property. When it's
> used in mailbox driver with non-cachable property, it'll only cause
> cache maintenance issue. So Leo declared these buffers as reserved
> in DT with "no-map" property. It's the key. It could avoid the cache
> maintenance issue.

The better solution is to never describe the memory to the kernel as
memory, by never placing it in a memory node, and ensuring that if it is
in the UEFI memory map, its attributes do not allow it to be mapped.

That way a driver can map it as non-cacheable if it wishes, but nothing
else can possibly touch that memory.

That is all you need to do.

> > > 2. UEFI just loads grub. It's no time to run any other custom EFI
> > > application.
> > 
> > Apart from being completely irrelevant, how are you intending to
> > validate that GRUB never touches these memory regions?
> > 
> 
> GRUB is just a part of bootloader. When linux kernel is running,
> who cares GRUB? GRUB's lifetime is already finished.

If GRUB temporarily maps memory as cacheable, or hands it to a device,
then your statements above about cache maintenance are broken.

An EFI application like GRUB might leave something resident in memory
after it's done (consider the UEFI shim), or it could even load the
kernel into the region that you care about having reserved, because as
far as it's concerned it's just memory. That could leave you with a
conflict for that region of memory.

You _must_ care about GRUB (and other EFI applications) doing the right
thing. To get them to avoid a region of memory, it must not be described
as being usable by them in the UEFI memory map.

> By the way, UEFI code region is at [0x3Dxx_xxxx, 0x3DFF_FFFF]. Those
> mailbox buffer is in [0x05e0_xxxx, 0x06f0_xxxx]. Then I can make sure
> UEFI won't touch the reserved buffer. Even if UEFI touched the reserved
> buffer, is it an issue? Definitely it's not.

It definitely is, due to the possibility of stale cache lines being left
in the region from when UEFI may have mapped it with cacheable
attributes.

> > Build a version once, test it, and hope the results remain valid
> > forever? And then when you move the regions and the previously working
> > GRUB now tramples all over them? Or when something changes in upstream
> > GRUB and its memory allocations drifts into the secretly untouchable
> > regions?
> 
> As I said above, UEFI won't touch it. And even UEFI touch it, kernel
> doesn't care since UEFI's lifetime is end.

If EFI touches it there may be stale cache lines left around, which you
don't seem to expect.

> > 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.

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

[toc] | [prev] | [next] | [standalone]

#1213045 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

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.
5. A patch is necessary in kernel. If efi stub feature is enabled,
   arm kernel should not parse memory node or reserved memory buffer in
   DT any more. Arm kernel should either fetch memory information from 
   efi or DT. Currently arm kernel fetch both efi memory information and
   reserved buffer from DTB at the same time.

Do you agree on these points?

Regards
Haojian

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

[toc] | [prev] | [next] | [standalone]

#1213081 — Re: [PATCH v1 3/3] arm64: dts: add Hi6220 mailbox node

On Tue, Aug 25, 2015 at 09:43:14PM +0800, Haojian Zhuang wrote:
> 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.

Yes. It needs to.

(I will let Mark comment on points 2-4.)

> 5. A patch is necessary in kernel. If efi stub feature is enabled,
>    arm kernel should not parse memory node or reserved memory buffer in
>    DT any more.

This is already the case. The stub deletes any present memory nodes and
reserved entries in drivers/firmware/efi/libstub/fdt.c:update_fdt().

Then, during setup_arch(), arch/arm64/kernel/efi.c:efi_init() calls
reserve_regions(), which adds only those memory regions available for
use by Linux as RAM to memblock.

>    Arm kernel should either fetch memory information from 
>    efi or DT.

Absolutely.

>    Currently arm kernel fetch both efi memory information and
>    reserved buffer from DTB at the same time.

No, it does not.

Regards,

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

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web