On Tue, Feb 20, 2018 at 11:21:05AM -0800, Jolly Shah wrote:

This patch is adding communication layer with firmware.
Firmware driver provides an interface to firmware APIs.
Interface APIs can be used by any driver to communicate to
PMUFW(Platform Management Unit). All requests go through ATF.

Signed-off-by: Jolly Shah <redacted>
Signed-off-by: Rajan Vaja <redacted>
---
 arch/arm64/Kconfig.platforms                    |    1 +
 drivers/firmware/Kconfig                        |    1 +
 drivers/firmware/Makefile                       |    1 +
 drivers/firmware/xilinx/Kconfig                 |    4 +
 drivers/firmware/xilinx/Makefile                |    4 +
 drivers/firmware/xilinx/zynqmp/Kconfig          |   16 +
 drivers/firmware/xilinx/zynqmp/Makefile         |    4 +
 drivers/firmware/xilinx/zynqmp/firmware.c       | 1051 +++++++++++++++++++++++

Why are you 2 levels deep?  Why not just drivers/firmware/zynqmp.c?  Or
at the worst:
	drivers/firmware/xilinx/zynqmp.c
Don't over do it, if we really get too many firmware drivers, we can
always move things around in the future.

 include/linux/firmware/xilinx/zynqmp/firmware.h |  590 +++++++++++++

I still think this include directly depth is crazy.  What's wrong with:
	include/linux/firmware/zynqmp.h
?

Select and not depends?

2+ for a Kconfig file?

2+ for a Makefile?

Again...

And again?

And I have to ask, you really mean "any future version"?

+/*
+ * Xilinx Zynq MPSoC Firmware layer
+ *
+ *  Copyright (C) 2014-2018 Xilinx, Inc.
+ *
+ *  Michal Simek [off-list ref]
+ *  Davorin Mista [off-list ref]
+ *  Jolly Shah [off-list ref]
+ *  Rajan Vaja [off-list ref]
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/compiler.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+#include <linux/firmware/xilinx/zynqmp/firmware.h>

Why do you need a file in linux/firmware anyway?

+struct zynqmp_eemi_ops {
+	int (*get_api_version)(u32 *version);
+	int (*get_chipid)(u32 *idcode, u32 *version);
+	int (*reset_assert)(const enum zynqmp_pm_reset reset,
+			    const enum zynqmp_pm_reset_action assert_flag);
+	int (*reset_get_status)(const enum zynqmp_pm_reset reset, u32 *status);
+	int (*fpga_load)(const u64 address, const u32 size, const u32 flags);
+	int (*fpga_get_status)(u32 *value);
+	int (*sha_hash)(const u64 address, const u32 size, const u32 flags);
+	int (*rsa)(const u64 address, const u32 size, const u32 flags);
+	int (*request_suspend)(const u32 node,
+			       const enum zynqmp_pm_request_ack ack,
+			       const u32 latency,
+			       const u32 state);
+	int (*force_powerdown)(const u32 target,
+			       const enum zynqmp_pm_request_ack ack);
+	int (*request_wakeup)(const u32 node,
+			      const bool set_addr,
+			      const u64 address,
+			      const enum zynqmp_pm_request_ack ack);
+	int (*set_wakeup_source)(const u32 target,
+				 const u32 wakeup_node,
+				 const u32 enable);
+	int (*system_shutdown)(const u32 type, const u32 subtype);
+	int (*request_node)(const u32 node,
+			    const u32 capabilities,
+			    const u32 qos,
+			    const enum zynqmp_pm_request_ack ack);
+	int (*release_node)(const u32 node);
+	int (*set_requirement)(const u32 node,
+			       const u32 capabilities,
+			       const u32 qos,
+			       const enum zynqmp_pm_request_ack ack);
+	int (*set_max_latency)(const u32 node, const u32 latency);
+	int (*set_configuration)(const u32 physical_addr);
+	int (*get_node_status)(const u32 node, u32 *const status,
+			       u32 *const requirements, u32 *const usage);
+	int (*get_operating_characteristic)(const u32 node,
+					    const enum zynqmp_pm_opchar_type
+					    type, u32 *const result);
+	int (*init_finalize)(void);
+	int (*set_suspend_mode)(u32 mode);
+	int (*ioctl)(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2, u32 *out);
+	int (*query_data)(struct zynqmp_pm_query_data qdata, u32 *out);
+	int (*pinctrl_request)(const u32 pin);
+	int (*pinctrl_release)(const u32 pin);
+	int (*pinctrl_get_function)(const u32 pin, u32 *id);
+	int (*pinctrl_set_function)(const u32 pin, const u32 id);
+	int (*pinctrl_get_config)(const u32 pin, const u32 param, u32 *value);
+	int (*pinctrl_set_config)(const u32 pin, const u32 param, u32 value);
+	int (*clock_enable)(u32 clock_id);
+	int (*clock_disable)(u32 clock_id);
+	int (*clock_getstate)(u32 clock_id, u32 *state);
+	int (*clock_setdivider)(u32 clock_id, u32 divider);
+	int (*clock_getdivider)(u32 clock_id, u32 *divider);
+	int (*clock_setrate)(u32 clock_id, u64 rate);
+	int (*clock_getrate)(u32 clock_id, u64 *rate);
+	int (*clock_setparent)(u32 clock_id, u32 parent_id);
+	int (*clock_getparent)(u32 clock_id, u32 *parent_id);
+};

Why do you need this huge structure of pointers to functions, when you
only ever set them to 1 value, and no one even calls them?

Why not just export the needed symbols and call them directly?  What is
the indirection getting you here?

thanks,

greg k-h