[PATCHv2 0/3] arm64 xilinx zynqmp firmware interface

[PATCHv2 0/3] arm64 xilinx zynqmp firmware interface

[Michal Simek]

Hi,

xilinx is using this interface for very long time and we can't merge our
driver changes to Linux because of missing communication layer with
firmware. This interface was developed before scpi and scmi was
available. In connection to power management scpi and scmi are missing
pieces which we already use. There is a separate discussion how to
extend scmi to support all our use cases.
This simply patch is not adding any power management features but only
adding minimum functionality which are needed for drivers.

Edgar's comment on having HVC:
"We have patches for Xen that implement a power-management mediator.
That implementation handles PM calls over both SMC and HVC insns.
Patches are on the Xen mailing list.

Other hypervisors may work differently.

I think we should support both but I don't have a strong opinion on
it."

Thanks,
Michal

Changes in v2:
- Move to bindings/firmware and also add it to firmware node
  Reported-by Rob
- Add zynqmp firmware node to firmware node
- Change commit message
- Make zynqmp_pm_ret_code, invoke_pm_fn static
  Reported by Arnd
- Remove pm_api_version function - no need for interface
- Move driver to drivers/firmware/
  Reported by Arnd and Rob

Michal Simek (1):
  soc: xilinx: zynqmp: Add firmware interface

Soren Brinkmann (2):
  dt: xilinx: zynqmp: Add bindings for PM firmware
  arm64: zynqmp: dt: Add PM firmware node

 .../bindings/firmware/xlnx,zynqmp-pm.txt           |  22 ++
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi             |   9 +
 drivers/firmware/Kconfig                           |   1 +
 drivers/firmware/Makefile                          |   1 +
 drivers/firmware/xilinx/Kconfig                    |   6 +
 drivers/firmware/xilinx/Makefile                   |   1 +
 drivers/firmware/xilinx/zynqmp/firmware.c          | 382 +++++++++++++++++++++
 include/linux/firmware/xilinx/zynqmp/firmware.h    | 232 +++++++++++++
 8 files changed, 654 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
 create mode 100644 drivers/firmware/xilinx/Kconfig
 create mode 100644 drivers/firmware/xilinx/Makefile
 create mode 100644 drivers/firmware/xilinx/zynqmp/firmware.c
 create mode 100644 include/linux/firmware/xilinx/zynqmp/firmware.h

-- 
1.9.1

[PATCHv2 1/3] dt: xilinx: zynqmp: Add bindings for PM firmware

[Michal Simek]

From: Soren Brinkmann <soren.brinkmann@xilinx.com>

Document the DT bindings for the Zynq UltraScale+ PM Firmware.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2:
- Move to bindings/firmware and also add it to firmware node
  Reported-by Rob

 .../bindings/firmware/xlnx,zynqmp-pm.txt           | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt

diff --git a/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt b/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
new file mode 100644
index 000000000000..7de0c82758b3
--- /dev/null
+++ b/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
@@ -0,0 +1,22 @@
+Xilinx Zynq MPSoC Firmware Device Tree Bindings
+
+The zynqmp-pm node describes the interface to platform firmware.
+
+Required properties:
+ - compatible:	Must contain:  "xlnx,zynqmp-pm"
+ - method:	The method of calling the PM-API firmware layer.
+		Permitted values are:
+		 - "smc" : To be used in configurations without a hypervisor
+		 - "hvc" : To be used when hypervisor is present
+ - interrupts:	Interrupt specifier
+
+Examples:
+
+	firmware {
+		zynqmp-firmware {
+			compatible = "xlnx,zynqmp-pm";
+			method = "smc";
+			interrupt-parent = <&gic>;
+			interrupts = <0 35 4>;
+		};
+	};
-- 
1.9.1

[PATCHv2 2/3] arm64: zynqmp: dt: Add PM firmware node

[Michal Simek]

From: Soren Brinkmann <soren.brinkmann@xilinx.com>

Add node for the firmware interface.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2:
- Add zynqmp firmware node to firmware node
- Change commit message

 arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index 54dc28351c8c..41c0034fd905 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -49,6 +49,15 @@
 		};
 	};
 
+	firmware {
+		zynqmp-firmware {
+			compatible = "xlnx,zynqmp-pm";
+			method = "smc";
+			interrupt-parent = <&gic>;
+			interrupts = <0 35 4>;
+		};
+	};
+
 	pmu {
 		compatible = "arm,armv8-pmuv3";
 		interrupt-parent = <&gic>;
-- 
1.9.1

[PATCHv2 3/3] soc: xilinx: zynqmp: Add firmware interface

[Michal Simek]

This patch is adding communication layer with firmware.
The most of request are coming thought ATF to PMUFW (platform management
unit).

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2:
- Make zynqmp_pm_ret_code, invoke_pm_fn static
  Reported by Arnd
- Remove pm_api_version function - no need for interface
- Move driver to drivers/firmware/
  Reported by Arnd and Rob

 drivers/firmware/Kconfig                        |   1 +
 drivers/firmware/Makefile                       |   1 +
 drivers/firmware/xilinx/Kconfig                 |   6 +
 drivers/firmware/xilinx/Makefile                |   1 +
 drivers/firmware/xilinx/zynqmp/firmware.c       | 382 ++++++++++++++++++++++++
 include/linux/firmware/xilinx/zynqmp/firmware.h | 232 ++++++++++++++
 6 files changed, 623 insertions(+)
 create mode 100644 drivers/firmware/xilinx/Kconfig
 create mode 100644 drivers/firmware/xilinx/Makefile
 create mode 100644 drivers/firmware/xilinx/zynqmp/firmware.c
 create mode 100644 include/linux/firmware/xilinx/zynqmp/firmware.h

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 6e4ed5a9c6fd..ebdf3c341899 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -238,5 +238,6 @@ source "drivers/firmware/google/Kconfig"
 source "drivers/firmware/efi/Kconfig"
 source "drivers/firmware/meson/Kconfig"
 source "drivers/firmware/tegra/Kconfig"
+source "drivers/firmware/xilinx/Kconfig"
 
 endmenu
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index a37f12e8d137..ebdce418dca0 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_GOOGLE_FIRMWARE)	+= google/
 obj-$(CONFIG_EFI)		+= efi/
 obj-$(CONFIG_UEFI_CPER)		+= efi/
 obj-y				+= tegra/
+obj-y				+= xilinx/
diff --git a/drivers/firmware/xilinx/Kconfig b/drivers/firmware/xilinx/Kconfig
new file mode 100644
index 000000000000..3bda5377f5e6
--- /dev/null
+++ b/drivers/firmware/xilinx/Kconfig
@@ -0,0 +1,6 @@
+#
+# Xilinx ZYNQ MPSoC configuration
+#
+menuconfig SOC_XILINX_ZYNQMP
+	bool "Xilinx Zynq MPSoC Firmware driver support"
+	depends on ARCH_ZYNQMP
diff --git a/drivers/firmware/xilinx/Makefile b/drivers/firmware/xilinx/Makefile
new file mode 100644
index 000000000000..954696bf9aec
--- /dev/null
+++ b/drivers/firmware/xilinx/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SOC_XILINX_ZYNQMP) += zynqmp/firmware.o
diff --git a/drivers/firmware/xilinx/zynqmp/firmware.c b/drivers/firmware/xilinx/zynqmp/firmware.c
new file mode 100644
index 000000000000..af30f78b5653
--- /dev/null
+++ b/drivers/firmware/xilinx/zynqmp/firmware.c
@@ -0,0 +1,382 @@
+/*
+ * Xilinx Zynq MPSoC Firware layer
+ *
+ *  Copyright (C) 2014-2017 Xilinx, Inc.
+ *
+ *  Michal Simek <michal.simek@xilinx.com>
+ *  Davorin Mista <davorin.mista@aggios.com>
+ *
+ * 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, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/compiler.h>
+#include <linux/arm-smccc.h>
+#include <linux/of.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <linux/uaccess.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+
+#include <linux/firmware/xilinx/zynqmp/firmware.h>
+
+/**
+ * zynqmp_pm_ret_code - Convert PMU-FW error codes to Linux error codes
+ * @ret_status:		PMUFW return code
+ *
+ * Return:		corresponding Linux error code
+ */
+static int zynqmp_pm_ret_code(u32 ret_status)
+{
+	switch (ret_status) {
+	case XST_PM_SUCCESS:
+	case XST_PM_DOUBLE_REQ:
+		return 0;
+	case XST_PM_NO_ACCESS:
+		return -EACCES;
+	case XST_PM_ABORT_SUSPEND:
+		return -ECANCELED;
+	case XST_PM_INTERNAL:
+	case XST_PM_CONFLICT:
+	case XST_PM_INVALID_NODE:
+	default:
+		return -EINVAL;
+	}
+}
+
+static noinline int do_fw_call_fail(u64 arg0, u64 arg1, u64 arg2,
+				    u32 *ret_payload)
+{
+	return -ENODEV;
+}
+
+/*
+ * PM function call wrapper
+ * Invoke do_fw_call_smc or do_fw_call_hvc, depending on the configuration
+ */
+static int (*do_fw_call)(u64, u64, u64, u32 *ret_payload) = do_fw_call_fail;
+
+/**
+ * do_fw_call_smc - Call system-level power management layer (SMC)
+ * @arg0:		Argument 0 to SMC call
+ * @arg1:		Argument 1 to SMC call
+ * @arg2:		Argument 2 to SMC call
+ * @ret_payload:	Returned value array
+ *
+ * Return:		Returns status, either success or error+reason
+ *
+ * Invoke power management function via SMC call (no hypervisor present)
+ */
+static noinline int do_fw_call_smc(u64 arg0, u64 arg1, u64 arg2,
+				   u32 *ret_payload)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_smc(arg0, arg1, arg2, 0, 0, 0, 0, 0, &res);
+
+	if (ret_payload) {
+		ret_payload[0] = (u32)res.a0;
+		ret_payload[1] = (u32)(res.a0 >> 32);
+		ret_payload[2] = (u32)res.a1;
+		ret_payload[3] = (u32)(res.a1 >> 32);
+		ret_payload[4] = (u32)res.a2;
+	}
+
+	return zynqmp_pm_ret_code((enum pm_ret_status)res.a0);
+}
+
+/**
+ * do_fw_call_hvc - Call system-level power management layer (HVC)
+ * @arg0:		Argument 0 to HVC call
+ * @arg1:		Argument 1 to HVC call
+ * @arg2:		Argument 2 to HVC call
+ * @ret_payload:	Returned value array
+ *
+ * Return:		Returns status, either success or error+reason
+ *
+ * Invoke power management function via HVC
+ * HVC-based for communication through hypervisor
+ * (no direct communication with ATF)
+ */
+static noinline int do_fw_call_hvc(u64 arg0, u64 arg1, u64 arg2,
+				   u32 *ret_payload)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_hvc(arg0, arg1, arg2, 0, 0, 0, 0, 0, &res);
+
+	if (ret_payload) {
+		ret_payload[0] = (u32)res.a0;
+		ret_payload[1] = (u32)(res.a0 >> 32);
+		ret_payload[2] = (u32)res.a1;
+		ret_payload[3] = (u32)(res.a1 >> 32);
+		ret_payload[4] = (u32)res.a2;
+	}
+
+	return zynqmp_pm_ret_code((enum pm_ret_status)res.a0);
+}
+
+/**
+ * invoke_pm_fn - Invoke the system-level power management layer caller
+ *			function depending on the configuration
+ * @pm_api_id:         Requested PM-API call
+ * @arg0:              Argument 0 to requested PM-API call
+ * @arg1:              Argument 1 to requested PM-API call
+ * @arg2:              Argument 2 to requested PM-API call
+ * @arg3:              Argument 3 to requested PM-API call
+ * @ret_payload:       Returned value array
+ *
+ * Return:             Returns status, either success or error+reason
+ *
+ * Invoke power management function for SMC or HVC call, depending on
+ * configuration
+ * Following SMC Calling Convention (SMCCC) for SMC64:
+ * Pm Function Identifier,
+ * PM_SIP_SVC + PM_API_ID =
+ *     ((SMC_TYPE_FAST << FUNCID_TYPE_SHIFT)
+ *     ((SMC_64) << FUNCID_CC_SHIFT)
+ *     ((SIP_START) << FUNCID_OEN_SHIFT)
+ *     ((PM_API_ID) & FUNCID_NUM_MASK))
+ *
+ * PM_SIP_SVC  - Registered ZynqMP SIP Service Call
+ * PM_API_ID   - Power Management API ID
+ */
+static int invoke_pm_fn(u32 pm_api_id, u32 arg0, u32 arg1, u32 arg2, u32 arg3,
+			u32 *ret_payload)
+{
+	/*
+	 * Added SIP service call Function Identifier
+	 * Make sure to stay in x0 register
+	 */
+	u64 smc_arg[4];
+
+	smc_arg[0] = PM_SIP_SVC | pm_api_id;
+	smc_arg[1] = ((u64)arg1 << 32) | arg0;
+	smc_arg[2] = ((u64)arg3 << 32) | arg2;
+
+	return do_fw_call(smc_arg[0], smc_arg[1], smc_arg[2], ret_payload);
+}
+
+/**
+ * zynqmp_pm_get_chipid - Get silicon ID registers
+ * @idcode:	IDCODE register
+ * @version:	version register
+ *
+ * Return:	Returns the status of the operation and the idcode and version
+ *		registers in @idcode and @version.
+ */
+int zynqmp_pm_get_chipid(u32 *idcode, u32 *version)
+{
+	u32 ret_payload[PAYLOAD_ARG_CNT];
+
+	if (!idcode || !version)
+		return -EINVAL;
+
+	invoke_pm_fn(GET_CHIPID, 0, 0, 0, 0, ret_payload);
+	*idcode = ret_payload[1];
+	*version = ret_payload[2];
+
+	return zynqmp_pm_ret_code((enum pm_ret_status)ret_payload[0]);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_get_chipid);
+
+/**
+ * get_set_conduit_method - Choose SMC or HVC based communication
+ * @np:	Pointer to the device_node structure
+ *
+ * Use SMC or HVC-based functions to communicate with EL2/EL3
+ */
+static void get_set_conduit_method(struct device_node *np)
+{
+	const char *method;
+	struct device *dev;
+
+	dev = container_of(&np, struct device, of_node);
+
+	if (of_property_read_string(np, "method", &method)) {
+		dev_warn(dev, "%s Missing \"method\" property - defaulting to smc\n",
+			 __func__);
+		do_fw_call = do_fw_call_smc;
+		return;
+	}
+
+	if (!strcmp("hvc", method)) {
+		do_fw_call = do_fw_call_hvc;
+
+	} else if (!strcmp("smc", method)) {
+		do_fw_call = do_fw_call_smc;
+	} else {
+		dev_warn(dev, "%s Invalid \"method\" property: %s - defaulting to smc\n",
+			 __func__, method);
+		do_fw_call = do_fw_call_smc;
+	}
+}
+
+/**
+ * zynqmp_pm_reset_assert - Request setting of reset (1 - assert, 0 - release)
+ * @reset:		Reset to be configured
+ * @assert_flag:	Flag stating should reset be asserted (1) or
+ *			released (0)
+ *
+ * Return:		Returns status, either success or error+reason
+ */
+int zynqmp_pm_reset_assert(const enum zynqmp_pm_reset reset,
+			   const enum zynqmp_pm_reset_action assert_flag)
+{
+	return invoke_pm_fn(RESET_ASSERT, reset, assert_flag, 0, 0, NULL);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_reset_assert);
+
+/**
+ * zynqmp_pm_reset_get_status - Get status of the reset
+ * @reset:	Reset whose status should be returned
+ * @status:	Returned status
+ *
+ * Return:	Returns status, either success or error+reason
+ */
+int zynqmp_pm_reset_get_status(const enum zynqmp_pm_reset reset, u32 *status)
+{
+	u32 ret_payload[PAYLOAD_ARG_CNT];
+
+	if (!status)
+		return zynqmp_pm_ret_code(XST_PM_CONFLICT);
+
+	invoke_pm_fn(RESET_GET_STATUS, reset, 0, 0, 0, ret_payload);
+	*status = ret_payload[1];
+
+	return zynqmp_pm_ret_code((enum pm_ret_status)ret_payload[0]);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_reset_get_status);
+
+/**
+ * zynqmp_pm_mmio_write - Perform write to protected mmio
+ * @address:	Address to write to
+ * @mask:	Mask to apply
+ * @value:	Value to write
+ *
+ * This function provides access to PM-related control registers
+ * that may not be directly accessible by a particular PU.
+ *
+ * Return:	Returns status, either success or error+reason
+ */
+int zynqmp_pm_mmio_write(const u32 address,
+			 const u32 mask,
+				     const u32 value)
+{
+	return invoke_pm_fn(MMIO_WRITE, address, mask, value, 0, NULL);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_mmio_write);
+
+/**
+ * zynqmp_pm_mmio_read - Read value from protected mmio
+ * @address:	Address to write to
+ * @value:	Value to read
+ *
+ * This function provides access to PM-related control registers
+ * that may not be directly accessible by a particular PU.
+ *
+ * Return:	Returns status, either success or error+reason
+ */
+int zynqmp_pm_mmio_read(const u32 address, u32 *value)
+{
+	u32 ret_payload[PAYLOAD_ARG_CNT];
+
+	if (!value)
+		return -EINVAL;
+
+	invoke_pm_fn(MMIO_READ, address, 0, 0, 0, ret_payload);
+	*value = ret_payload[1];
+
+	return zynqmp_pm_ret_code((enum pm_ret_status)ret_payload[0]);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_mmio_read);
+
+/**
+ * zynqmp_pm_fpga_load - Perform the fpga load
+ * @address:    Address to write to
+ * @size:       pl bitstream size
+ * @flags:
+ *	BIT(0) - Bit-stream type.
+ *		 0 - Full Bit-stream.
+ *		 1 - Partial Bit-stream.
+ *	BIT(1) - Authentication.
+ *		 1 - Enable.
+ *		 0 - Disable.
+ *	BIT(2) - Encryption.
+ *		 1 - Enable.
+ *		 0 - Disable.
+ * NOTE -
+ *	The current implementation supports only Full Bit-stream.
+ *
+ * This function provides access to xilfpga library to transfer
+ * the required bitstream into PL.
+ *
+ * Return:      Returns status, either success or error+reason
+ */
+int zynqmp_pm_fpga_load(const u64 address, const u32 size, const u32 flags)
+{
+	return invoke_pm_fn(FPGA_LOAD, (u32)address,
+			((u32)(address >> 32)), size, flags, NULL);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_fpga_load);
+
+/**
+ * zynqmp_pm_fpga_get_status - Read value from PCAP status register
+ * @value:      Value to read
+ *
+ *This function provides access to the xilfpga library to get
+ *the PCAP status
+ *
+ * Return:      Returns status, either success or error+reason
+ */
+int zynqmp_pm_fpga_get_status(u32 *value)
+{
+	u32 ret_payload[PAYLOAD_ARG_CNT];
+
+	if (!value)
+		return -EINVAL;
+
+	invoke_pm_fn(FPGA_GET_STATUS, 0, 0, 0, 0, ret_payload);
+	*value = ret_payload[1];
+
+	return zynqmp_pm_ret_code((enum pm_ret_status)ret_payload[0]);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_fpga_get_status);
+
+static int __init zynqmp_plat_init(void)
+{
+	struct device_node *np;
+	int ret = 0;
+
+	np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp");
+	if (!np)
+		return 0;
+	of_node_put(np);
+
+	/* We're running on a ZynqMP machine, the PM node is mandatory. */
+	np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp-pm");
+	if (!np)
+		panic("%s: pm node not found\n", __func__);
+
+	get_set_conduit_method(np);
+
+	pr_info("%s ZynqMP firmware interface up\n", __func__);
+
+	of_node_put(np);
+	return ret;
+}
+
+early_initcall(zynqmp_plat_init);
diff --git a/include/linux/firmware/xilinx/zynqmp/firmware.h b/include/linux/firmware/xilinx/zynqmp/firmware.h
new file mode 100644
index 000000000000..b6f941fc5f91
--- /dev/null
+++ b/include/linux/firmware/xilinx/zynqmp/firmware.h
@@ -0,0 +1,232 @@
+/*
+ * Xilinx Zynq MPSoC Firmware layer
+ *
+ *  Copyright (C) 2014-2017 Xilinx
+ *
+ *  Michal Simek <michal.simek@xilinx.com>
+ *  Davorin Mista <davorin.mista@aggios.com>
+ *
+ * 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, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __SOC_ZYNQMP_FIRMWARE_H__
+#define __SOC_ZYNQMP_FIRMWARE_H__
+
+/* SMC SIP service Call Function Identifier Prefix */
+#define PM_SIP_SVC	0xC2000000
+#define GET_CALLBACK_DATA 0xa01
+#define SET_SUSPEND_MODE  0xa02
+
+/* Number of 32bits values in payload */
+#define PAYLOAD_ARG_CNT	5U
+
+/* Number of arguments for a callback */
+#define CB_ARG_CNT	4
+
+/* Payload size (consists of callback API ID + arguments) */
+#define CB_PAYLOAD_SIZE	(CB_ARG_CNT + 1)
+
+/* Global general storage register base address */
+#define GGS_BASEADDR	(0xFFD80030U)
+#define GSS_NUM_REGS	(4)
+
+/* Persistent global general storage register base address */
+#define PGGS_BASEADDR	(0xFFD80050U)
+#define PGSS_NUM_REGS	(4)
+
+enum pm_api_id {
+	/* Miscellaneous API functions: */
+	GET_API_VERSION = 1,
+	SET_CONFIGURATION,
+	GET_NODE_STATUS,
+	GET_OPERATING_CHARACTERISTIC,
+	REGISTER_NOTIFIER,
+	/* API for suspending of PUs: */
+	REQUEST_SUSPEND,
+	SELF_SUSPEND,
+	FORCE_POWERDOWN,
+	ABORT_SUSPEND,
+	REQUEST_WAKEUP,
+	SET_WAKEUP_SOURCE,
+	SYSTEM_SHUTDOWN,
+	/* API for managing PM slaves: */
+	REQUEST_NODE,
+	RELEASE_NODE,
+	SET_REQUIREMENT,
+	SET_MAX_LATENCY,
+	/* Direct control API functions: */
+	RESET_ASSERT,
+	RESET_GET_STATUS,
+	MMIO_WRITE,
+	MMIO_READ,
+	PM_INIT_FINALIZE,
+	FPGA_LOAD,
+	FPGA_GET_STATUS,
+	GET_CHIPID,
+};
+
+/* PMU-FW return status codes */
+enum pm_ret_status {
+	XST_PM_SUCCESS = 0,
+	XST_PM_INTERNAL	= 2000,
+	XST_PM_CONFLICT,
+	XST_PM_NO_ACCESS,
+	XST_PM_INVALID_NODE,
+	XST_PM_DOUBLE_REQ,
+	XST_PM_ABORT_SUSPEND,
+};
+
+enum zynqmp_pm_reset_action {
+	PM_RESET_ACTION_RELEASE,
+	PM_RESET_ACTION_ASSERT,
+	PM_RESET_ACTION_PULSE,
+};
+
+enum zynqmp_pm_reset {
+	ZYNQMP_PM_RESET_START = 999,
+	ZYNQMP_PM_RESET_PCIE_CFG,
+	ZYNQMP_PM_RESET_PCIE_BRIDGE,
+	ZYNQMP_PM_RESET_PCIE_CTRL,
+	ZYNQMP_PM_RESET_DP,
+	ZYNQMP_PM_RESET_SWDT_CRF,
+	ZYNQMP_PM_RESET_AFI_FM5,
+	ZYNQMP_PM_RESET_AFI_FM4,
+	ZYNQMP_PM_RESET_AFI_FM3,
+	ZYNQMP_PM_RESET_AFI_FM2,
+	ZYNQMP_PM_RESET_AFI_FM1,
+	ZYNQMP_PM_RESET_AFI_FM0,
+	ZYNQMP_PM_RESET_GDMA,
+	ZYNQMP_PM_RESET_GPU_PP1,
+	ZYNQMP_PM_RESET_GPU_PP0,
+	ZYNQMP_PM_RESET_GPU,
+	ZYNQMP_PM_RESET_GT,
+	ZYNQMP_PM_RESET_SATA,
+	ZYNQMP_PM_RESET_ACPU3_PWRON,
+	ZYNQMP_PM_RESET_ACPU2_PWRON,
+	ZYNQMP_PM_RESET_ACPU1_PWRON,
+	ZYNQMP_PM_RESET_ACPU0_PWRON,
+	ZYNQMP_PM_RESET_APU_L2,
+	ZYNQMP_PM_RESET_ACPU3,
+	ZYNQMP_PM_RESET_ACPU2,
+	ZYNQMP_PM_RESET_ACPU1,
+	ZYNQMP_PM_RESET_ACPU0,
+	ZYNQMP_PM_RESET_DDR,
+	ZYNQMP_PM_RESET_APM_FPD,
+	ZYNQMP_PM_RESET_SOFT,
+	ZYNQMP_PM_RESET_GEM0,
+	ZYNQMP_PM_RESET_GEM1,
+	ZYNQMP_PM_RESET_GEM2,
+	ZYNQMP_PM_RESET_GEM3,
+	ZYNQMP_PM_RESET_QSPI,
+	ZYNQMP_PM_RESET_UART0,
+	ZYNQMP_PM_RESET_UART1,
+	ZYNQMP_PM_RESET_SPI0,
+	ZYNQMP_PM_RESET_SPI1,
+	ZYNQMP_PM_RESET_SDIO0,
+	ZYNQMP_PM_RESET_SDIO1,
+	ZYNQMP_PM_RESET_CAN0,
+	ZYNQMP_PM_RESET_CAN1,
+	ZYNQMP_PM_RESET_I2C0,
+	ZYNQMP_PM_RESET_I2C1,
+	ZYNQMP_PM_RESET_TTC0,
+	ZYNQMP_PM_RESET_TTC1,
+	ZYNQMP_PM_RESET_TTC2,
+	ZYNQMP_PM_RESET_TTC3,
+	ZYNQMP_PM_RESET_SWDT_CRL,
+	ZYNQMP_PM_RESET_NAND,
+	ZYNQMP_PM_RESET_ADMA,
+	ZYNQMP_PM_RESET_GPIO,
+	ZYNQMP_PM_RESET_IOU_CC,
+	ZYNQMP_PM_RESET_TIMESTAMP,
+	ZYNQMP_PM_RESET_RPU_R50,
+	ZYNQMP_PM_RESET_RPU_R51,
+	ZYNQMP_PM_RESET_RPU_AMBA,
+	ZYNQMP_PM_RESET_OCM,
+	ZYNQMP_PM_RESET_RPU_PGE,
+	ZYNQMP_PM_RESET_USB0_CORERESET,
+	ZYNQMP_PM_RESET_USB1_CORERESET,
+	ZYNQMP_PM_RESET_USB0_HIBERRESET,
+	ZYNQMP_PM_RESET_USB1_HIBERRESET,
+	ZYNQMP_PM_RESET_USB0_APB,
+	ZYNQMP_PM_RESET_USB1_APB,
+	ZYNQMP_PM_RESET_IPI,
+	ZYNQMP_PM_RESET_APM_LPD,
+	ZYNQMP_PM_RESET_RTC,
+	ZYNQMP_PM_RESET_SYSMON,
+	ZYNQMP_PM_RESET_AFI_FM6,
+	ZYNQMP_PM_RESET_LPD_SWDT,
+	ZYNQMP_PM_RESET_FPD,
+	ZYNQMP_PM_RESET_RPU_DBG1,
+	ZYNQMP_PM_RESET_RPU_DBG0,
+	ZYNQMP_PM_RESET_DBG_LPD,
+	ZYNQMP_PM_RESET_DBG_FPD,
+	ZYNQMP_PM_RESET_APLL,
+	ZYNQMP_PM_RESET_DPLL,
+	ZYNQMP_PM_RESET_VPLL,
+	ZYNQMP_PM_RESET_IOPLL,
+	ZYNQMP_PM_RESET_RPLL,
+	ZYNQMP_PM_RESET_GPO3_PL_0,
+	ZYNQMP_PM_RESET_GPO3_PL_1,
+	ZYNQMP_PM_RESET_GPO3_PL_2,
+	ZYNQMP_PM_RESET_GPO3_PL_3,
+	ZYNQMP_PM_RESET_GPO3_PL_4,
+	ZYNQMP_PM_RESET_GPO3_PL_5,
+	ZYNQMP_PM_RESET_GPO3_PL_6,
+	ZYNQMP_PM_RESET_GPO3_PL_7,
+	ZYNQMP_PM_RESET_GPO3_PL_8,
+	ZYNQMP_PM_RESET_GPO3_PL_9,
+	ZYNQMP_PM_RESET_GPO3_PL_10,
+	ZYNQMP_PM_RESET_GPO3_PL_11,
+	ZYNQMP_PM_RESET_GPO3_PL_12,
+	ZYNQMP_PM_RESET_GPO3_PL_13,
+	ZYNQMP_PM_RESET_GPO3_PL_14,
+	ZYNQMP_PM_RESET_GPO3_PL_15,
+	ZYNQMP_PM_RESET_GPO3_PL_16,
+	ZYNQMP_PM_RESET_GPO3_PL_17,
+	ZYNQMP_PM_RESET_GPO3_PL_18,
+	ZYNQMP_PM_RESET_GPO3_PL_19,
+	ZYNQMP_PM_RESET_GPO3_PL_20,
+	ZYNQMP_PM_RESET_GPO3_PL_21,
+	ZYNQMP_PM_RESET_GPO3_PL_22,
+	ZYNQMP_PM_RESET_GPO3_PL_23,
+	ZYNQMP_PM_RESET_GPO3_PL_24,
+	ZYNQMP_PM_RESET_GPO3_PL_25,
+	ZYNQMP_PM_RESET_GPO3_PL_26,
+	ZYNQMP_PM_RESET_GPO3_PL_27,
+	ZYNQMP_PM_RESET_GPO3_PL_28,
+	ZYNQMP_PM_RESET_GPO3_PL_29,
+	ZYNQMP_PM_RESET_GPO3_PL_30,
+	ZYNQMP_PM_RESET_GPO3_PL_31,
+	ZYNQMP_PM_RESET_RPU_LS,
+	ZYNQMP_PM_RESET_PS_ONLY,
+	ZYNQMP_PM_RESET_PL,
+	ZYNQMP_PM_RESET_END
+};
+
+/* Miscellaneous API functions */
+int zynqmp_pm_get_chipid(u32 *idcode, u32 *version);
+
+/* Direct-Control API functions */
+int zynqmp_pm_reset_assert(const enum zynqmp_pm_reset reset,
+				const enum zynqmp_pm_reset_action assert_flag);
+int zynqmp_pm_reset_get_status(const enum zynqmp_pm_reset reset,
+					u32 *status);
+int zynqmp_pm_mmio_write(const u32 address,
+				     const u32 mask,
+				     const u32 value);
+int zynqmp_pm_mmio_read(const u32 address, u32 *value);
+int zynqmp_pm_fpga_load(const u64 address, const u32 size, const u32 flags);
+int zynqmp_pm_fpga_get_status(u32 *value);
+
+#endif /* __SOC_ZYNQMP_FIRMWARE_H__ */
-- 
1.9.1

Re: [PATCHv2 0/3] arm64 xilinx zynqmp firmware interface

[Marc Zyngier]

On 16/08/17 13:24, Michal Simek wrote:
> Hi,
> 
> xilinx is using this interface for very long time and we can't merge our
> driver changes to Linux because of missing communication layer with
> firmware. This interface was developed before scpi and scmi was
> available. In connection to power management scpi and scmi are missing
> pieces which we already use. There is a separate discussion how to
> extend scmi to support all our use cases.

So maybe we should wait and see where this discussion is going before we
merge yet another firmware interface?

> This simply patch is not adding any power management features but only
> adding minimum functionality which are needed for drivers.

I don't get it. 600 lines of firmware interface that isn't used by
anything? Or is it? Needed by which driver?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCHv2 1/3] dt: xilinx: zynqmp: Add bindings for PM firmware

[Sudeep Holla]

On 16/08/17 13:24, Michal Simek wrote:
> From: Soren Brinkmann <soren.brinkmann@xilinx.com>
> 
> Document the DT bindings for the Zynq UltraScale+ PM Firmware.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> Changes in v2:
> - Move to bindings/firmware and also add it to firmware node
>   Reported-by Rob
> 
>  .../bindings/firmware/xlnx,zynqmp-pm.txt           | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
> 
> diff --git a/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt b/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
> new file mode 100644
> index 000000000000..7de0c82758b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
> @@ -0,0 +1,22 @@
> +Xilinx Zynq MPSoC Firmware Device Tree Bindings
> +
> +The zynqmp-pm node describes the interface to platform firmware.
> +
> +Required properties:
> + - compatible:	Must contain:  "xlnx,zynqmp-pm"
> + - method:	The method of calling the PM-API firmware layer.
> +		Permitted values are:
> +		 - "smc" : To be used in configurations without a hypervisor
> +		 - "hvc" : To be used when hypervisor is present
> + - interrupts:	Interrupt specifier

What exactly is this interrupt ? I see it's not SGI, so if it's SPI,
where does this originate from ? How is that dealt ?

-- 
Regards,
Sudeep

Re: [PATCHv2 3/3] soc: xilinx: zynqmp: Add firmware interface

[Mark Rutland]

On Wed, Aug 16, 2017 at 02:24:58PM +0200, Michal Simek wrote:
> This patch is adding communication layer with firmware.

Which is used for what, exactly?

There are no users of this API introduced in this series.

> The most of request are coming thought ATF to PMUFW (platform management
> unit).

[...]

> +/**
> + * get_set_conduit_method - Choose SMC or HVC based communication
> + * @np:	Pointer to the device_node structure
> + *
> + * Use SMC or HVC-based functions to communicate with EL2/EL3
> + */
> +static void get_set_conduit_method(struct device_node *np)
> +{
> +	const char *method;
> +	struct device *dev;
> +
> +	dev = container_of(&np, struct device, of_node);
> +
> +	if (of_property_read_string(np, "method", &method)) {
> +		dev_warn(dev, "%s Missing \"method\" property - defaulting to smc\n",
> +			 __func__);
> +		do_fw_call = do_fw_call_smc;
> +		return;
> +	}

This is a mandatory property. Don't guess if it's not present.

[...]

> +/**
> + * zynqmp_pm_reset_assert - Request setting of reset (1 - assert, 0 - release)
> + * @reset:		Reset to be configured
> + * @assert_flag:	Flag stating should reset be asserted (1) or
> + *			released (0)
> + *
> + * Return:		Returns status, either success or error+reason
> + */
> +int zynqmp_pm_reset_assert(const enum zynqmp_pm_reset reset,
> +			   const enum zynqmp_pm_reset_action assert_flag)
> +{
> +	return invoke_pm_fn(RESET_ASSERT, reset, assert_flag, 0, 0, NULL);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_reset_assert);

Where is the reset nr expected to come from?

Nothing in this series defined bindings for those.

> +/**
> + * zynqmp_pm_mmio_write - Perform write to protected mmio
> + * @address:	Address to write to
> + * @mask:	Mask to apply
> + * @value:	Value to write
> + *
> + * This function provides access to PM-related control registers
> + * that may not be directly accessible by a particular PU.
> + *
> + * Return:	Returns status, either success or error+reason
> + */
> +int zynqmp_pm_mmio_write(const u32 address,
> +			 const u32 mask,
> +				     const u32 value)
> +{
> +	return invoke_pm_fn(MMIO_WRITE, address, mask, value, 0, NULL);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_mmio_write);
> +
> +/**
> + * zynqmp_pm_mmio_read - Read value from protected mmio
> + * @address:	Address to write to
> + * @value:	Value to read
> + *
> + * This function provides access to PM-related control registers
> + * that may not be directly accessible by a particular PU.
> + *
> + * Return:	Returns status, either success or error+reason
> + */
> +int zynqmp_pm_mmio_read(const u32 address, u32 *value)
> +{
> +	u32 ret_payload[PAYLOAD_ARG_CNT];
> +
> +	if (!value)
> +		return -EINVAL;
> +
> +	invoke_pm_fn(MMIO_READ, address, 0, 0, 0, ret_payload);
> +	*value = ret_payload[1];
> +
> +	return zynqmp_pm_ret_code((enum pm_ret_status)ret_payload[0]);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_mmio_read);

I guess these are to secure memory?

How are these safe?

What validation is done on the secure side?

> +
> +/**
> + * zynqmp_pm_fpga_load - Perform the fpga load
> + * @address:    Address to write to
> + * @size:       pl bitstream size
> + * @flags:
> + *	BIT(0) - Bit-stream type.
> + *		 0 - Full Bit-stream.
> + *		 1 - Partial Bit-stream.
> + *	BIT(1) - Authentication.
> + *		 1 - Enable.
> + *		 0 - Disable.
> + *	BIT(2) - Encryption.
> + *		 1 - Enable.
> + *		 0 - Disable.
> + * NOTE -
> + *	The current implementation supports only Full Bit-stream.
> + *
> + * This function provides access to xilfpga library to transfer
> + * the required bitstream into PL.
> + *
> + * Return:      Returns status, either success or error+reason
> + */
> +int zynqmp_pm_fpga_load(const u64 address, const u32 size, const u32 flags)
> +{
> +	return invoke_pm_fn(FPGA_LOAD, (u32)address,
> +			((u32)(address >> 32)), size, flags, NULL);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_fpga_load);

What space is the address in?

*which* FPGA instance is this? Surely this needs to be linked to a node
in the DT by some means?

[...]

> +static int __init zynqmp_plat_init(void)
> +{
> +	struct device_node *np;
> +	int ret = 0;
> +
> +	np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp");
> +	if (!np)
> +		return 0;
> +	of_node_put(np);
> +
> +	/* We're running on a ZynqMP machine, the PM node is mandatory. */
> +	np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp-pm");
> +	if (!np)
> +		panic("%s: pm node not found\n", __func__);

NAK to this panic(). It breaks existing DTBs.

[...]

> +enum pm_api_id {
> +	/* Miscellaneous API functions: */
> +	GET_API_VERSION = 1,
> +	SET_CONFIGURATION,
> +	GET_NODE_STATUS,
> +	GET_OPERATING_CHARACTERISTIC,
> +	REGISTER_NOTIFIER,
> +	/* API for suspending of PUs: */
> +	REQUEST_SUSPEND,
> +	SELF_SUSPEND,
> +	FORCE_POWERDOWN,
> +	ABORT_SUSPEND,
> +	REQUEST_WAKEUP,
> +	SET_WAKEUP_SOURCE,
> +	SYSTEM_SHUTDOWN,

What are these? They sound like they overlap with PSCI.

Thanks,
Mark.

Re: [PATCHv2 1/3] dt: xilinx: zynqmp: Add bindings for PM firmware

[Mark Rutland]

On Wed, Aug 16, 2017 at 02:24:56PM +0200, Michal Simek wrote:
> From: Soren Brinkmann <soren.brinkmann@xilinx.com>
> 
> Document the DT bindings for the Zynq UltraScale+ PM Firmware.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> Changes in v2:
> - Move to bindings/firmware and also add it to firmware node
>   Reported-by Rob
> 
>  .../bindings/firmware/xlnx,zynqmp-pm.txt           | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
> 
> diff --git a/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt b/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
> new file mode 100644
> index 000000000000..7de0c82758b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
> @@ -0,0 +1,22 @@
> +Xilinx Zynq MPSoC Firmware Device Tree Bindings
> +
> +The zynqmp-pm node describes the interface to platform firmware.

Is there any documentation for this? I appreciate that the answer
might be "no".

> +Required properties:
> + - compatible:	Must contain:  "xlnx,zynqmp-pm"
> + - method:	The method of calling the PM-API firmware layer.
> +		Permitted values are:
> +		 - "smc" : To be used in configurations without a hypervisor
> +		 - "hvc" : To be used when hypervisor is present
> + - interrupts:	Interrupt specifier

For what, exactly?

Is this an interrupt for FW to signal the OS somehow?

If so, is it an SGI?

Thanks,
Mark.

Re: [PATCHv2 0/3] arm64 xilinx zynqmp firmware interface

[Michal Simek]

On 16.8.2017 17:39, Marc Zyngier wrote:
> On 16/08/17 13:24, Michal Simek wrote:
>> Hi,
>>
>> xilinx is using this interface for very long time and we can't merge our
>> driver changes to Linux because of missing communication layer with
>> firmware. This interface was developed before scpi and scmi was
>> available. In connection to power management scpi and scmi are missing
>> pieces which we already use. There is a separate discussion how to
>> extend scmi to support all our use cases.
> 
> So maybe we should wait and see where this discussion is going before we
> merge yet another firmware interface?

It will take a lot of time when this discussion ends and I can't see any
benefit to hold all


>> This simply patch is not adding any power management features but only
>> adding minimum functionality which are needed for drivers.
> 
> I don't get it. 600 lines of firmware interface that isn't used by
> anything? Or is it? Needed by which driver?

I can send that drivers for review. pinctrl, fpga manager, nvmem driver,
clock, serdes phy and reset drivers.
But this driver need to come first because they depend on this.

Thanks,
Michal

Re: [PATCHv2 1/3] dt: xilinx: zynqmp: Add bindings for PM firmware

[Michal Simek]

On 16.8.2017 18:00, Mark Rutland wrote:
> On Wed, Aug 16, 2017 at 02:24:56PM +0200, Michal Simek wrote:
>> From: Soren Brinkmann <soren.brinkmann@xilinx.com>
>>
>> Document the DT bindings for the Zynq UltraScale+ PM Firmware.
>>
>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> Changes in v2:
>> - Move to bindings/firmware and also add it to firmware node
>>   Reported-by Rob
>>
>>  .../bindings/firmware/xlnx,zynqmp-pm.txt           | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt b/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
>> new file mode 100644
>> index 000000000000..7de0c82758b3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
>> @@ -0,0 +1,22 @@
>> +Xilinx Zynq MPSoC Firmware Device Tree Bindings
>> +
>> +The zynqmp-pm node describes the interface to platform firmware.
> 
> Is there any documentation for this? I appreciate that the answer
> might be "no".

We have pm documentation somewhere but not sure if this firmware
interface is the part of it. Let me ask around and see what I can find out.

> 
>> +Required properties:
>> + - compatible:	Must contain:  "xlnx,zynqmp-pm"
>> + - method:	The method of calling the PM-API firmware layer.
>> +		Permitted values are:
>> +		 - "smc" : To be used in configurations without a hypervisor
>> +		 - "hvc" : To be used when hypervisor is present
>> + - interrupts:	Interrupt specifier
> 
> For what, exactly?
> 
> Is this an interrupt for FW to signal the OS somehow?
> 
> If so, is it an SGI?

This is SPI. There is a dedicated hardware on the chip for communication
with APU.

https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
page 275

Anyway I don't need this for this driver that's why I will remove it.

Thanks,
Michal

Re: [PATCHv2 1/3] dt: xilinx: zynqmp: Add bindings for PM firmware

[Michal Simek]

On 16.8.2017 17:45, Sudeep Holla wrote:
> 
> 
> On 16/08/17 13:24, Michal Simek wrote:
>> From: Soren Brinkmann <soren.brinkmann@xilinx.com>
>>
>> Document the DT bindings for the Zynq UltraScale+ PM Firmware.
>>
>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> Changes in v2:
>> - Move to bindings/firmware and also add it to firmware node
>>   Reported-by Rob
>>
>>  .../bindings/firmware/xlnx,zynqmp-pm.txt           | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt b/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
>> new file mode 100644
>> index 000000000000..7de0c82758b3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/firmware/xlnx,zynqmp-pm.txt
>> @@ -0,0 +1,22 @@
>> +Xilinx Zynq MPSoC Firmware Device Tree Bindings
>> +
>> +The zynqmp-pm node describes the interface to platform firmware.
>> +
>> +Required properties:
>> + - compatible:	Must contain:  "xlnx,zynqmp-pm"
>> + - method:	The method of calling the PM-API firmware layer.
>> +		Permitted values are:
>> +		 - "smc" : To be used in configurations without a hypervisor
>> +		 - "hvc" : To be used when hypervisor is present
>> + - interrupts:	Interrupt specifier
> 
> What exactly is this interrupt ? I see it's not SGI, so if it's SPI,
> where does this originate from ? How is that dealt ?
> 

Please look at my response to Mark for more details.

Thanks,
Michal

Re: [PATCHv2 3/3] soc: xilinx: zynqmp: Add firmware interface

[Michal Simek]

On 16.8.2017 17:58, Mark Rutland wrote:
> On Wed, Aug 16, 2017 at 02:24:58PM +0200, Michal Simek wrote:
>> This patch is adding communication layer with firmware.
> 
> Which is used for what, exactly?

pinctrl, fpga manager, nvmem driver, clock, serdes phy and reset drivers.

> There are no users of this API introduced in this series.

But this driver need to come first because they depend on this.
If you want to look at that drivers please take a look at
https://github.com/Xilinx/linux-xlnx

> 
>> The most of request are coming thought ATF to PMUFW (platform management
>> unit).
> 
> [...]
> 
>> +/**
>> + * get_set_conduit_method - Choose SMC or HVC based communication
>> + * @np:	Pointer to the device_node structure
>> + *
>> + * Use SMC or HVC-based functions to communicate with EL2/EL3
>> + */
>> +static void get_set_conduit_method(struct device_node *np)
>> +{
>> +	const char *method;
>> +	struct device *dev;
>> +
>> +	dev = container_of(&np, struct device, of_node);
>> +
>> +	if (of_property_read_string(np, "method", &method)) {
>> +		dev_warn(dev, "%s Missing \"method\" property - defaulting to smc\n",
>> +			 __func__);
>> +		do_fw_call = do_fw_call_smc;
>> +		return;
>> +	}
> 
> This is a mandatory property. Don't guess if it's not present.

Fixed in next version

> 
> [...]
> 
>> +/**
>> + * zynqmp_pm_reset_assert - Request setting of reset (1 - assert, 0 - release)
>> + * @reset:		Reset to be configured
>> + * @assert_flag:	Flag stating should reset be asserted (1) or
>> + *			released (0)
>> + *
>> + * Return:		Returns status, either success or error+reason
>> + */
>> +int zynqmp_pm_reset_assert(const enum zynqmp_pm_reset reset,
>> +			   const enum zynqmp_pm_reset_action assert_flag)
>> +{
>> +	return invoke_pm_fn(RESET_ASSERT, reset, assert_flag, 0, 0, NULL);
>> +}
>> +EXPORT_SYMBOL_GPL(zynqmp_pm_reset_assert);
> 
> Where is the reset nr expected to come from?
> 
> Nothing in this series defined bindings for those.


Numbers will be the part of reset driver binding but numbers are coming
from DT.


This is the usage.

		serdes: zynqmp_phy@fd400000 {
			compatible = "xlnx,zynqmp-psgtr";
			status = "disabled";
			reg = <0x0 0xfd400000 0x0 0x40000>,
			      <0x0 0xfd3d0000 0x0 0x1000>,
			      <0x0 0xff5e0000 0x0 0x1000>;
			reg-names = "serdes", "siou", "lpd";
			nvmem-cells = <&soc_revision>;
			nvmem-cell-names = "soc_revision";
			resets = <&rst 16>, <&rst 59>, <&rst 60>,
				 <&rst 61>, <&rst 62>, <&rst 63>,
				 <&rst 64>, <&rst 3>, <&rst 29>,
				 <&rst 30>, <&rst 31>, <&rst 32>;
			reset-names = "sata_rst", "usb0_crst", "usb1_crst",
				      "usb0_hibrst", "usb1_hibrst", "usb0_apbrst",
				      "usb1_apbrst", "dp_rst", "gem0_rst",
				      "gem1_rst", "gem2_rst", "gem3_rst";
			lane0: lane0 {
				#phy-cells = <4>;
			};
			lane1: lane1 {
				#phy-cells = <4>;
			};
			lane2: lane2 {
				#phy-cells = <4>;
			};
			lane3: lane3 {
				#phy-cells = <4>;
			};
		};


If you think that this shouldn't be the part of this patch, I can remove
this part and send it as the part of reset driver.

> 
>> +/**
>> + * zynqmp_pm_mmio_write - Perform write to protected mmio
>> + * @address:	Address to write to
>> + * @mask:	Mask to apply
>> + * @value:	Value to write
>> + *
>> + * This function provides access to PM-related control registers
>> + * that may not be directly accessible by a particular PU.
>> + *
>> + * Return:	Returns status, either success or error+reason
>> + */
>> +int zynqmp_pm_mmio_write(const u32 address,
>> +			 const u32 mask,
>> +				     const u32 value)
>> +{
>> +	return invoke_pm_fn(MMIO_WRITE, address, mask, value, 0, NULL);
>> +}
>> +EXPORT_SYMBOL_GPL(zynqmp_pm_mmio_write);
>> +
>> +/**
>> + * zynqmp_pm_mmio_read - Read value from protected mmio
>> + * @address:	Address to write to
>> + * @value:	Value to read
>> + *
>> + * This function provides access to PM-related control registers
>> + * that may not be directly accessible by a particular PU.
>> + *
>> + * Return:	Returns status, either success or error+reason
>> + */
>> +int zynqmp_pm_mmio_read(const u32 address, u32 *value)
>> +{
>> +	u32 ret_payload[PAYLOAD_ARG_CNT];
>> +
>> +	if (!value)
>> +		return -EINVAL;
>> +
>> +	invoke_pm_fn(MMIO_READ, address, 0, 0, 0, ret_payload);
>> +	*value = ret_payload[1];
>> +
>> +	return zynqmp_pm_ret_code((enum pm_ret_status)ret_payload[0]);
>> +}
>> +EXPORT_SYMBOL_GPL(zynqmp_pm_mmio_read);
> 
> I guess these are to secure memory?

yep.

> 
> How are these safe?
> 
> What validation is done on the secure side?

firmware is limiting access based on system setup.


> 
>> +
>> +/**
>> + * zynqmp_pm_fpga_load - Perform the fpga load
>> + * @address:    Address to write to
>> + * @size:       pl bitstream size
>> + * @flags:
>> + *	BIT(0) - Bit-stream type.
>> + *		 0 - Full Bit-stream.
>> + *		 1 - Partial Bit-stream.
>> + *	BIT(1) - Authentication.
>> + *		 1 - Enable.
>> + *		 0 - Disable.
>> + *	BIT(2) - Encryption.
>> + *		 1 - Enable.
>> + *		 0 - Disable.
>> + * NOTE -
>> + *	The current implementation supports only Full Bit-stream.
>> + *
>> + * This function provides access to xilfpga library to transfer
>> + * the required bitstream into PL.
>> + *
>> + * Return:      Returns status, either success or error+reason
>> + */
>> +int zynqmp_pm_fpga_load(const u64 address, const u32 size, const u32 flags)
>> +{
>> +	return invoke_pm_fn(FPGA_LOAD, (u32)address,
>> +			((u32)(address >> 32)), size, flags, NULL);
>> +}
>> +EXPORT_SYMBOL_GPL(zynqmp_pm_fpga_load);
> 
> What space is the address in?
> 
> *which* FPGA instance is this? Surely this needs to be linked to a node
> in the DT by some means?
> 
> [...]

It is the same case as reset functions above. If you think that this
should be the part of fpga series I am happy to remove this part for
this patch.

https://github.com/Xilinx/linux-xlnx/blob/master/drivers/fpga/zynqmp-fpga.c

> 
>> +static int __init zynqmp_plat_init(void)
>> +{
>> +	struct device_node *np;
>> +	int ret = 0;
>> +
>> +	np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp");
>> +	if (!np)
>> +		return 0;
>> +	of_node_put(np);
>> +
>> +	/* We're running on a ZynqMP machine, the PM node is mandatory. */
>> +	np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp-pm");
>> +	if (!np)
>> +		panic("%s: pm node not found\n", __func__);
> 
> NAK to this panic(). It breaks existing DTBs.

Ok. I have fixed this.

> 
> [...]
> 
>> +enum pm_api_id {
>> +	/* Miscellaneous API functions: */
>> +	GET_API_VERSION = 1,
>> +	SET_CONFIGURATION,
>> +	GET_NODE_STATUS,
>> +	GET_OPERATING_CHARACTERISTIC,
>> +	REGISTER_NOTIFIER,
>> +	/* API for suspending of PUs: */
>> +	REQUEST_SUSPEND,
>> +	SELF_SUSPEND,
>> +	FORCE_POWERDOWN,
>> +	ABORT_SUSPEND,
>> +	REQUEST_WAKEUP,
>> +	SET_WAKEUP_SOURCE,
>> +	SYSTEM_SHUTDOWN,
> 
> What are these? They sound like they overlap with PSCI.

PM guys can comment this more than I but AFAIK PSCI is not providing
enough functionality for our chip that's why additional things needs to
be done. I can remove them as is above with reset/fpga stuff.

Thanks,
Michal

Re: [PATCHv2 0/3] arm64 xilinx zynqmp firmware interface

[Marc Zyngier]

On 17/08/17 07:10, Michal Simek wrote:
> On 16.8.2017 17:39, Marc Zyngier wrote:
>> On 16/08/17 13:24, Michal Simek wrote:
>>> Hi,
>>>
>>> xilinx is using this interface for very long time and we can't merge our
>>> driver changes to Linux because of missing communication layer with
>>> firmware. This interface was developed before scpi and scmi was
>>> available. In connection to power management scpi and scmi are missing
>>> pieces which we already use. There is a separate discussion how to
>>> extend scmi to support all our use cases.
>>
>> So maybe we should wait and see where this discussion is going before we
>> merge yet another firmware interface?
> 
> It will take a lot of time when this discussion ends and I can't see any
> benefit to hold all

Well, so far, the benefit of this series is exactly nil, as the code it
brings is *unused*. It is impossible to review in isolation.

In the meantime, you can continue finding out how *not* to have to merge
this code, and instead focus on using the infrastructure we already
have, or at least influence the infrastructure that is being designed.
It will be much better than dumping yet another slab of "I'm so
different" code that is going to ultimately bitrot.

> 
> 
>>> This simply patch is not adding any power management features but only
>>> adding minimum functionality which are needed for drivers.
>>
>> I don't get it. 600 lines of firmware interface that isn't used by
>> anything? Or is it? Needed by which driver?
> 
> I can send that drivers for review. pinctrl, fpga manager, nvmem driver,
> clock, serdes phy and reset drivers.
> But this driver need to come first because they depend on this.
My take is: no users, no use.

And if you need to hack all these drivers to hook into your specific
firmware interface, I feel that you're doing it wrong. We have common
APIs for device drivers. If these APIs are not good enough, we extend
them. But designing drivers for a given firmware interface?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCHv2 0/3] arm64 xilinx zynqmp firmware interface

[Michal Simek]

On 17.8.2017 09:52, Marc Zyngier wrote:
> On 17/08/17 07:10, Michal Simek wrote:
>> On 16.8.2017 17:39, Marc Zyngier wrote:
>>> On 16/08/17 13:24, Michal Simek wrote:
>>>> Hi,
>>>>
>>>> xilinx is using this interface for very long time and we can't merge our
>>>> driver changes to Linux because of missing communication layer with
>>>> firmware. This interface was developed before scpi and scmi was
>>>> available. In connection to power management scpi and scmi are missing
>>>> pieces which we already use. There is a separate discussion how to
>>>> extend scmi to support all our use cases.
>>>
>>> So maybe we should wait and see where this discussion is going before we
>>> merge yet another firmware interface?
>>
>> It will take a lot of time when this discussion ends and I can't see any
>> benefit to hold all
> 
> Well, so far, the benefit of this series is exactly nil, as the code it
> brings is *unused*. It is impossible to review in isolation.
> 

Ok. I will add others drivers to this series that's not a problem.

> In the meantime, you can continue finding out how *not* to have to merge
> this code, and instead focus on using the infrastructure we already
> have, or at least influence the infrastructure that is being designed.
> It will be much better than dumping yet another slab of "I'm so
> different" code that is going to ultimately bitrot.

I am happy to look the better proposed way. SCPI is ancient and SCMI is
replacement and not merged yet. We already had a call with arm and
Sudeep was on it too where outcome from that was that we can't use that
because it doesn't support what we need to support now.

This code is compatible with current ARM SMC calling convention which
allocate range for vendors.

0xC2000000-0xC200FFFF SMC64: SiP Service Calls

Provides interfaces to SoC implementation-specific services on this
platform, for example secure platform initialization, configuration
, and some power control services.

> 
>>
>>
>>>> This simply patch is not adding any power management features but only
>>>> adding minimum functionality which are needed for drivers.
>>>
>>> I don't get it. 600 lines of firmware interface that isn't used by
>>> anything? Or is it? Needed by which driver?
>>
>> I can send that drivers for review. pinctrl, fpga manager, nvmem driver,
>> clock, serdes phy and reset drivers.
>> But this driver need to come first because they depend on this.
> My take is: no users, no use.
> 
> And if you need to hack all these drivers to hook into your specific
> firmware interface, I feel that you're doing it wrong. We have common
> APIs for device drivers. If these APIs are not good enough, we extend
> them. But designing drivers for a given firmware interface?

Let me send that users of this.

Thanks,
Michal

Re: [PATCHv2 0/3] arm64 xilinx zynqmp firmware interface

[Sudeep Holla]

On 17/08/17 09:42, Michal Simek wrote:
> On 17.8.2017 09:52, Marc Zyngier wrote:
>> On 17/08/17 07:10, Michal Simek wrote:
>>> On 16.8.2017 17:39, Marc Zyngier wrote:
>>>> On 16/08/17 13:24, Michal Simek wrote:
>>>>> Hi,
>>>>>
>>>>> xilinx is using this interface for very long time and we can't merge our
>>>>> driver changes to Linux because of missing communication layer with
>>>>> firmware. This interface was developed before scpi and scmi was
>>>>> available. In connection to power management scpi and scmi are missing
>>>>> pieces which we already use. There is a separate discussion how to
>>>>> extend scmi to support all our use cases.
>>>>
>>>> So maybe we should wait and see where this discussion is going before we
>>>> merge yet another firmware interface?
>>>
>>> It will take a lot of time when this discussion ends and I can't see any
>>> benefit to hold all
>>
>> Well, so far, the benefit of this series is exactly nil, as the code it
>> brings is *unused*. It is impossible to review in isolation.
>>
> 
> Ok. I will add others drivers to this series that's not a problem.
> 
>> In the meantime, you can continue finding out how *not* to have to merge
>> this code, and instead focus on using the infrastructure we already
>> have, or at least influence the infrastructure that is being designed.
>> It will be much better than dumping yet another slab of "I'm so
>> different" code that is going to ultimately bitrot.
> 
> I am happy to look the better proposed way. SCPI is ancient and SCMI is
> replacement and not merged yet. We already had a call with arm and
> Sudeep was on it too where outcome from that was that we can't use that
> because it doesn't support what we need to support now.
> 

OK, none of the specifics were discussed in the meeting to conclude that
SCMI can't be used. My takeaway from the meeting was Xilinx has this
interface for long and being deployed in various systems. I would like
to get into specifics before discarding SCMI as unusable. What bothers
me more is that why was that not raised during the specification review
which was quite a long period IMO ? I tend to think Xilinx never
bothered to look/review the specification as this f/w interface was
already there.

However I still can't see why this was posted once we started pushing
out SCMI patches especially given that this f/w interface was there for
long and no attempts were made in past to upstream this.

Also I am not dismissing the series yet, but if I find that SCMI can be
used(after getting specifics from this series myself), I will at-least
argue against the "SCMI can't be used" argument.

-- 
Regards,
Sudeep

Re: [PATCHv2 0/3] arm64 xilinx zynqmp firmware interface

[Michal Simek]

On 17.8.2017 11:12, Sudeep Holla wrote:
> 
> 
> On 17/08/17 09:42, Michal Simek wrote:
>> On 17.8.2017 09:52, Marc Zyngier wrote:
>>> On 17/08/17 07:10, Michal Simek wrote:
>>>> On 16.8.2017 17:39, Marc Zyngier wrote:
>>>>> On 16/08/17 13:24, Michal Simek wrote:
>>>>>> Hi,
>>>>>>
>>>>>> xilinx is using this interface for very long time and we can't merge our
>>>>>> driver changes to Linux because of missing communication layer with
>>>>>> firmware. This interface was developed before scpi and scmi was
>>>>>> available. In connection to power management scpi and scmi are missing
>>>>>> pieces which we already use. There is a separate discussion how to
>>>>>> extend scmi to support all our use cases.
>>>>>
>>>>> So maybe we should wait and see where this discussion is going before we
>>>>> merge yet another firmware interface?
>>>>
>>>> It will take a lot of time when this discussion ends and I can't see any
>>>> benefit to hold all
>>>
>>> Well, so far, the benefit of this series is exactly nil, as the code it
>>> brings is *unused*. It is impossible to review in isolation.
>>>
>>
>> Ok. I will add others drivers to this series that's not a problem.
>>
>>> In the meantime, you can continue finding out how *not* to have to merge
>>> this code, and instead focus on using the infrastructure we already
>>> have, or at least influence the infrastructure that is being designed.
>>> It will be much better than dumping yet another slab of "I'm so
>>> different" code that is going to ultimately bitrot.
>>
>> I am happy to look the better proposed way. SCPI is ancient and SCMI is
>> replacement and not merged yet. We already had a call with arm and
>> Sudeep was on it too where outcome from that was that we can't use that
>> because it doesn't support what we need to support now.
>>
> 
> OK, none of the specifics were discussed in the meeting to conclude that
> SCMI can't be used. My takeaway from the meeting was Xilinx has this
> interface for long and being deployed in various systems. I would like
> to get into specifics before discarding SCMI as unusable. What bothers
> me more is that why was that not raised during the specification review
> which was quite a long period IMO ? I tend to think Xilinx never
> bothered to look/review the specification as this f/w interface was
> already there.

Xilinx is using this interface from Aug 2015. I am not aware about any
invitation to spec review. And not sure who was there from xilinx.

> 
> However I still can't see why this was posted once we started pushing
> out SCMI patches especially given that this f/w interface was there for
> long and no attempts were made in past to upstream this.

The reason is simple which is upstream our code which depends on this
communication layer. I don't think there is quick path to move to
different interface than this one.

> 
> Also I am not dismissing the series yet, but if I find that SCMI can be
> used(after getting specifics from this series myself), I will at-least
> argue against the "SCMI can't be used" argument.

This is not my argument that we can't use SCMI. This is what was my
understanding from that meeting we had. And definitely there is no quick
path for us to switch to SCMI and breaks all current existing customers.

And this interface is just in the same position as current SCPI. It
means you have SCPI already merged and you are adding new one. SCMI
could be maybe also just SCPIv2.
And when it is merged you are going to move all targets to this new
interface. The same is for us but instead of scpi it is this interface
we use and test.

Thanks,
Michal

Re: [PATCHv2 0/3] arm64 xilinx zynqmp firmware interface

[Sudeep Holla]

On 17/08/17 11:32, Michal Simek wrote:
> On 17.8.2017 11:12, Sudeep Holla wrote:
>> 
>> 
>> On 17/08/17 09:42, Michal Simek wrote:
>>> On 17.8.2017 09:52, Marc Zyngier wrote:
>>>> On 17/08/17 07:10, Michal Simek wrote:
>>>>> On 16.8.2017 17:39, Marc Zyngier wrote:
>>>>>> On 16/08/17 13:24, Michal Simek wrote:
>>>>>>> Hi,
>>>>>>> 
>>>>>>> xilinx is using this interface for very long time and we
>>>>>>> can't merge our driver changes to Linux because of
>>>>>>> missing communication layer with firmware. This interface
>>>>>>> was developed before scpi and scmi was available. In
>>>>>>> connection to power management scpi and scmi are missing 
>>>>>>> pieces which we already use. There is a separate
>>>>>>> discussion how to extend scmi to support all our use
>>>>>>> cases.
>>>>>> 
>>>>>> So maybe we should wait and see where this discussion is
>>>>>> going before we merge yet another firmware interface?
>>>>> 
>>>>> It will take a lot of time when this discussion ends and I
>>>>> can't see any benefit to hold all
>>>> 
>>>> Well, so far, the benefit of this series is exactly nil, as the
>>>> code it brings is *unused*. It is impossible to review in
>>>> isolation.
>>>> 
>>> 
>>> Ok. I will add others drivers to this series that's not a
>>> problem.
>>> 
>>>> In the meantime, you can continue finding out how *not* to have
>>>> to merge this code, and instead focus on using the
>>>> infrastructure we already have, or at least influence the
>>>> infrastructure that is being designed. It will be much better
>>>> than dumping yet another slab of "I'm so different" code that
>>>> is going to ultimately bitrot.
>>> 
>>> I am happy to look the better proposed way. SCPI is ancient and
>>> SCMI is replacement and not merged yet. We already had a call
>>> with arm and Sudeep was on it too where outcome from that was
>>> that we can't use that because it doesn't support what we need to
>>> support now.
>>> 
>> 
>> OK, none of the specifics were discussed in the meeting to conclude
>> that SCMI can't be used. My takeaway from the meeting was Xilinx
>> has this interface for long and being deployed in various systems.
>> I would like to get into specifics before discarding SCMI as
>> unusable. What bothers me more is that why was that not raised
>> during the specification review which was quite a long period IMO ?
>> I tend to think Xilinx never bothered to look/review the
>> specification as this f/w interface was already there.
> 
> Xilinx is using this interface from Aug 2015. I am not aware about
> any invitation to spec review. And not sure who was there from
> xilinx.
> 

Sure, I can understand and that's not a problem but Xilinx was involved.

>> 
>> However I still can't see why this was posted once we started
>> pushing out SCMI patches especially given that this f/w interface
>> was there for long and no attempts were made in past to upstream
>> this.
> 
> The reason is simple which is upstream our code which depends on
> this communication layer. I don't think there is quick path to move
> to different interface than this one.
> 

Do you mean "smc" when you refer communication layer ? If so, that's
fine. You can use "smc" as transport with SCMI if you want,
specification doesn't prevent that.

>> 
>> Also I am not dismissing the series yet, but if I find that SCMI
>> can be used(after getting specifics from this series myself), I
>> will at-least argue against the "SCMI can't be used" argument.
> 
> This is not my argument that we can't use SCMI. This is what was my 
> understanding from that meeting we had. And definitely there is no
> quick path for us to switch to SCMI and breaks all current existing
> customers.
> 

I understand the latter and I mentioned the same earlier, but I disagree
with the former. That meeting was mostly introduction(and informal IMO)
and didn't involve anything at the technical level.

> And this interface is just in the same position as current SCPI. It 
> means you have SCPI already merged and you are adding new one. SCMI 
> could be maybe also just SCPIv2.

Agreed, but it was posted as soon as the specification is out and so is
the SCMI. I am not arguing it as a point, but just mentioning that this
post was simply bad timing :)

-- 
Regards,
Sudeep

Re: [PATCHv2 0/3] arm64 xilinx zynqmp firmware interface

[Michal Simek]

On 17.8.2017 13:18, Sudeep Holla wrote:
> 
> 
> On 17/08/17 11:32, Michal Simek wrote:
>> On 17.8.2017 11:12, Sudeep Holla wrote:
>>>
>>>
>>> On 17/08/17 09:42, Michal Simek wrote:
>>>> On 17.8.2017 09:52, Marc Zyngier wrote:
>>>>> On 17/08/17 07:10, Michal Simek wrote:
>>>>>> On 16.8.2017 17:39, Marc Zyngier wrote:
>>>>>>> On 16/08/17 13:24, Michal Simek wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> xilinx is using this interface for very long time and we
>>>>>>>> can't merge our driver changes to Linux because of
>>>>>>>> missing communication layer with firmware. This interface
>>>>>>>> was developed before scpi and scmi was available. In
>>>>>>>> connection to power management scpi and scmi are missing 
>>>>>>>> pieces which we already use. There is a separate
>>>>>>>> discussion how to extend scmi to support all our use
>>>>>>>> cases.
>>>>>>>
>>>>>>> So maybe we should wait and see where this discussion is
>>>>>>> going before we merge yet another firmware interface?
>>>>>>
>>>>>> It will take a lot of time when this discussion ends and I
>>>>>> can't see any benefit to hold all
>>>>>
>>>>> Well, so far, the benefit of this series is exactly nil, as the
>>>>> code it brings is *unused*. It is impossible to review in
>>>>> isolation.
>>>>>
>>>>
>>>> Ok. I will add others drivers to this series that's not a
>>>> problem.
>>>>
>>>>> In the meantime, you can continue finding out how *not* to have
>>>>> to merge this code, and instead focus on using the
>>>>> infrastructure we already have, or at least influence the
>>>>> infrastructure that is being designed. It will be much better
>>>>> than dumping yet another slab of "I'm so different" code that
>>>>> is going to ultimately bitrot.
>>>>
>>>> I am happy to look the better proposed way. SCPI is ancient and
>>>> SCMI is replacement and not merged yet. We already had a call
>>>> with arm and Sudeep was on it too where outcome from that was
>>>> that we can't use that because it doesn't support what we need to
>>>> support now.
>>>>
>>>
>>> OK, none of the specifics were discussed in the meeting to conclude
>>> that SCMI can't be used. My takeaway from the meeting was Xilinx
>>> has this interface for long and being deployed in various systems.
>>> I would like to get into specifics before discarding SCMI as
>>> unusable. What bothers me more is that why was that not raised
>>> during the specification review which was quite a long period IMO ?
>>> I tend to think Xilinx never bothered to look/review the
>>> specification as this f/w interface was already there.
>>
>> Xilinx is using this interface from Aug 2015. I am not aware about
>> any invitation to spec review. And not sure who was there from
>> xilinx.
>>
> 
> Sure, I can understand and that's not a problem but Xilinx was involved.
> 
>>>
>>> However I still can't see why this was posted once we started
>>> pushing out SCMI patches especially given that this f/w interface
>>> was there for long and no attempts were made in past to upstream
>>> this.
>>
>> The reason is simple which is upstream our code which depends on
>> this communication layer. I don't think there is quick path to move
>> to different interface than this one.
>>
> 
> Do you mean "smc" when you refer communication layer ? If so, that's
> fine. You can use "smc" as transport with SCMI if you want,
> specification doesn't prevent that.

yep, just smc or hvc.


>>>
>>> Also I am not dismissing the series yet, but if I find that SCMI
>>> can be used(after getting specifics from this series myself), I
>>> will at-least argue against the "SCMI can't be used" argument.
>>
>> This is not my argument that we can't use SCMI. This is what was my 
>> understanding from that meeting we had. And definitely there is no
>> quick path for us to switch to SCMI and breaks all current existing
>> customers.
>>
> 
> I understand the latter and I mentioned the same earlier, but I disagree
> with the former. That meeting was mostly introduction(and informal IMO)
> and didn't involve anything at the technical level.

Definitely the first one for me.


>> And this interface is just in the same position as current SCPI. It 
>> means you have SCPI already merged and you are adding new one. SCMI 
>> could be maybe also just SCPIv2.
> 
> Agreed, but it was posted as soon as the specification is out and so is
> the SCMI. I am not arguing it as a point, but just mentioning that this
> post was simply bad timing :)

Definitely not purpose just coincidence. SCMI v1 June 26, SCMI v2 Aug 4.

It is quite clear if Xilinx decides to use SCMI that there will be
transition time between what we use for couple of years to new interface.

Thanks,
Michal