From: Rajan Vaja <[email protected]>
This patch series adds xilinx specific sysfs interface for below
purposes:
- Register access
- Set shutdown scope
- Set boot health status bit
Rajan Vaja (4):
firmware: xilinx: Add sysfs interface
firmware: xilinx: Add system shutdown API interface
firmware: xilinx: Add sysfs to set shutdown scope
firmware: xilinx: Add sysfs and IOCTL to set boot health status
Documentation/ABI/stable/sysfs-firmware-zynqmp | 103 +++++++++
drivers/firmware/xilinx/Makefile | 2 +-
drivers/firmware/xilinx/zynqmp-ggs.c | 284 +++++++++++++++++++++++++
drivers/firmware/xilinx/zynqmp.c | 242 +++++++++++++++++++++
include/linux/firmware/xlnx-zynqmp.h | 28 ++-
5 files changed, 657 insertions(+), 2 deletions(-)
create mode 100644 Documentation/ABI/stable/sysfs-firmware-zynqmp
create mode 100644 drivers/firmware/xilinx/zynqmp-ggs.c
--
Changes in v2:
- Removed patch #1 for register access sysfs.
- Updated kernel version in documentation.
- Used DEVICE_ATTR_* and ATTRIBUTE_GROUPS macros.
- Correct typo
- Free Kobject structure in case of error.
- Resolved smatch errors.
- Updated Signed-off-by sequence.
--
2.7.4
From: Rajan Vaja <[email protected]>
Add sysfs interface to set boot health status from user space.
Add IOCTL ID used by this interface to communicate with firmware.
If PMUFW is compiled with CHECK_HEALTHY_BOOT, it will check the
healthy bit on FPD WDT expiration. If healthy bit is set by a user
application running in Linux, PMUFW will do APU only restart. If
healthy bit is not set during FPD WDT expiration, PMUFW will do
system restart.
Signed-off-by: Rajan Vaja <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
Signed-off-by: Jolly Shah <[email protected]>
---
Changes in v2:
- Updated Linux kernel version in documentation.
- Used DEVICE_ATTR_* and ATTRIBUTE_GROUPS macros.
- Updated Signed-off-by sequence.
---
Documentation/ABI/stable/sysfs-firmware-zynqmp | 21 +++++++++++++
drivers/firmware/xilinx/zynqmp.c | 41 ++++++++++++++++++++++++++
include/linux/firmware/xlnx-zynqmp.h | 2 ++
3 files changed, 64 insertions(+)
diff --git a/Documentation/ABI/stable/sysfs-firmware-zynqmp b/Documentation/ABI/stable/sysfs-firmware-zynqmp
index f41e9c5..3f44a3c 100644
--- a/Documentation/ABI/stable/sysfs-firmware-zynqmp
+++ b/Documentation/ABI/stable/sysfs-firmware-zynqmp
@@ -80,3 +80,24 @@ Description:
# echo "subsystem" > /sys/firmware/zynqmp/shutdown_scope
Users: Xilinx
+
+What: /sys/firmware/zynqmp/health_status
+Date: April 2018
+KernelVersion: 5.5
+Contact: "Rajan Vaja" <[email protected]>
+Description:
+ This sysfs interface allows to set the health status. If PMUFW
+ is compiled with CHECK_HEALTHY_BOOT, it will check the healthy
+ bit on FPD WDT expiration. If healthy bit is set by a user
+ application running in Linux, PMUFW will do APU only restart. If
+ healthy bit is not set during FPD WDT expiration, PMUFW will do
+ system restart.
+
+ Usage:
+ Set healthy bit
+ # echo 1 > /sys/firmware/zynqmp/health_status
+
+ Unset healthy bit
+ # echo 0 > /sys/firmware/zynqmp/health_status
+
+Users: Xilinx
diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index ef7d107..2a77c90 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -477,6 +477,7 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
case IOCTL_READ_GGS:
case IOCTL_WRITE_PGGS:
case IOCTL_READ_PGGS:
+ case IOCTL_SET_BOOT_HEALTH_STATUS:
return 1;
default:
return 0;
@@ -861,8 +862,48 @@ static ssize_t shutdown_scope_store(struct device *device,
static DEVICE_ATTR_RW(shutdown_scope);
+/**
+ * health_status_store - Store health_status sysfs attribute
+ * @device: Device structure
+ * @attr: Device attribute structure
+ * @buf: User entered health_status attribute string
+ * @count: Buffer size
+ *
+ * User-space interface for setting the boot health status.
+ * Usage: echo <value> > /sys/firmware/zynqmp/health_status
+ *
+ * Value:
+ * 1 - Set healthy bit to 1
+ * 0 - Unset healthy bit
+ *
+ * Return: count argument if request succeeds, the corresponding error
+ * code otherwise
+ */
+static ssize_t health_status_store(struct device *device,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+ unsigned int value;
+
+ ret = kstrtouint(buf, 10, &value);
+ if (ret)
+ return ret;
+
+ ret = zynqmp_pm_ioctl(0, IOCTL_SET_BOOT_HEALTH_STATUS, value, 0, NULL);
+ if (ret) {
+ pr_err("unable to set healthy bit value to %u\n", value);
+ return ret;
+ }
+
+ return count;
+}
+
+static DEVICE_ATTR_WO(health_status);
+
static struct attribute *zynqmp_attrs[] = {
&dev_attr_shutdown_scope.attr,
+ &dev_attr_health_status.attr,
NULL,
};
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index e4f83c6..0554054 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -106,6 +106,8 @@ enum pm_ioctl_id {
IOCTL_READ_GGS,
IOCTL_WRITE_PGGS,
IOCTL_READ_PGGS,
+ /* Set healthy bit value */
+ IOCTL_SET_BOOT_HEALTH_STATUS = 17,
};
enum pm_query_id {
--
2.7.4
From: Rajan Vaja <[email protected]>
Add Firmware-ggs sysfs interface which provides read/write
interface to global storage registers.
Signed-off-by: Rajan Vaja <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
Signed-off-by: Jolly Shah <[email protected]>
---
Changes in v2:
- Updated Linux kernel version in documentation.
- Used DEVICE_ATTR_* and ATTRIBUTE_GROUPS macros.
- Free Kobject structure in case of error.
- Resolved smatch errors.
- Updated Signed-off-by sequence.
---
Documentation/ABI/stable/sysfs-firmware-zynqmp | 50 +++++
drivers/firmware/xilinx/Makefile | 2 +-
drivers/firmware/xilinx/zynqmp-ggs.c | 284 +++++++++++++++++++++++++
drivers/firmware/xilinx/zynqmp.c | 32 +++
include/linux/firmware/xlnx-zynqmp.h | 10 +
5 files changed, 377 insertions(+), 1 deletion(-)
create mode 100644 Documentation/ABI/stable/sysfs-firmware-zynqmp
create mode 100644 drivers/firmware/xilinx/zynqmp-ggs.c
diff --git a/Documentation/ABI/stable/sysfs-firmware-zynqmp b/Documentation/ABI/stable/sysfs-firmware-zynqmp
new file mode 100644
index 0000000..cffa2fc
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-firmware-zynqmp
@@ -0,0 +1,50 @@
+What: /sys/firmware/zynqmp/ggs*
+Date: January 2018
+KernelVersion: 5.5
+Contact: "Jolly Shah" <[email protected]>
+Description:
+ Read/Write PMU global general storage register value,
+ GLOBAL_GEN_STORAGE{0:3}.
+ Global general storage register that can be used
+ by system to pass information between masters.
+
+ The register is reset during system or power-on
+ resets. Three registers are used by the FSBL and
+ other Xilinx software products: GLOBAL_GEN_STORAGE{4:6}.
+
+ Usage:
+ # cat /sys/firmware/zynqmp/ggs0
+ # echo <mask> <value> > /sys/firmware/zynqmp/ggs0
+
+ Example:
+ # cat /sys/firmware/zynqmp/ggs0
+ # echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/ggs0
+
+Users: Xilinx
+
+What: /sys/firmware/zynqmp/pggs*
+Date: January 2018
+KernelVersion: 5.5
+Contact: "Jolly Shah" <[email protected]>
+Description:
+ Read/Write PMU persistent global general storage register
+ value, PERS_GLOB_GEN_STORAGE{0:3}.
+ Persistent global general storage register that
+ can be used by system to pass information between
+ masters.
+
+ This register is only reset by the power-on reset
+ and maintains its value through a system reset.
+ Four registers are used by the FSBL and other Xilinx
+ software products: PERS_GLOB_GEN_STORAGE{4:7}.
+ Register is reset only by a POR reset.
+
+ Usage:
+ # cat /sys/firmware/zynqmp/pggs0
+ # echo <mask> <value> > /sys/firmware/zynqmp/pggs0
+
+ Example:
+ # cat /sys/firmware/zynqmp/pggs0
+ # echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/pggs0
+
+Users: Xilinx
diff --git a/drivers/firmware/xilinx/Makefile b/drivers/firmware/xilinx/Makefile
index 875a537..1e8643c 100644
--- a/drivers/firmware/xilinx/Makefile
+++ b/drivers/firmware/xilinx/Makefile
@@ -1,5 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
# Makefile for Xilinx firmwares
-obj-$(CONFIG_ZYNQMP_FIRMWARE) += zynqmp.o
+obj-$(CONFIG_ZYNQMP_FIRMWARE) += zynqmp.o zynqmp-ggs.o
obj-$(CONFIG_ZYNQMP_FIRMWARE_DEBUG) += zynqmp-debug.o
diff --git a/drivers/firmware/xilinx/zynqmp-ggs.c b/drivers/firmware/xilinx/zynqmp-ggs.c
new file mode 100644
index 0000000..e2a6700
--- /dev/null
+++ b/drivers/firmware/xilinx/zynqmp-ggs.c
@@ -0,0 +1,284 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Xilinx Zynq MPSoC Firmware layer
+ *
+ * Copyright (C) 2014-2018 Xilinx, Inc.
+ *
+ * Jolly Shah <[email protected]>
+ * Rajan Vaja <[email protected]>
+ */
+
+#include <linux/compiler.h>
+#include <linux/of.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>
+#include <linux/firmware/xlnx-zynqmp.h>
+
+static ssize_t read_register(char *buf, u32 ioctl_id, u32 reg)
+{
+ int ret;
+ u32 ret_payload[PAYLOAD_ARG_CNT];
+ const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+ if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
+ return -EFAULT;
+
+ ret = eemi_ops->ioctl(0, ioctl_id, reg, 0, ret_payload);
+ if (ret)
+ return ret;
+
+ return sprintf(buf, "0x%x\n", ret_payload[1]);
+}
+
+static ssize_t write_register(const char *buf, size_t count, u32 read_ioctl,
+ u32 write_ioctl, u32 reg)
+{
+ char *kern_buff, *inbuf, *tok;
+ long mask, value;
+ int ret;
+ u32 ret_payload[PAYLOAD_ARG_CNT];
+ const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+ if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
+ return -EFAULT;
+
+ kern_buff = kzalloc(count, GFP_KERNEL);
+ if (!kern_buff)
+ return -ENOMEM;
+
+ ret = strlcpy(kern_buff, buf, count);
+ if (ret < 0) {
+ ret = -EFAULT;
+ goto err;
+ }
+
+ inbuf = kern_buff;
+
+ /* Read the write mask */
+ tok = strsep(&inbuf, " ");
+ if (!tok) {
+ ret = -EFAULT;
+ goto err;
+ }
+
+ ret = kstrtol(tok, 16, &mask);
+ if (ret) {
+ ret = -EFAULT;
+ goto err;
+ }
+
+ /* Read the write value */
+ tok = strsep(&inbuf, " ");
+ if (!tok) {
+ ret = -EFAULT;
+ goto err;
+ }
+
+ ret = kstrtol(tok, 16, &value);
+ if (ret) {
+ ret = -EFAULT;
+ goto err;
+ }
+
+ ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
+ if (ret) {
+ ret = -EFAULT;
+ goto err;
+ }
+ ret_payload[1] &= ~mask;
+ value &= mask;
+ value |= ret_payload[1];
+
+ ret = eemi_ops->ioctl(0, write_ioctl, reg, value, NULL);
+ if (ret)
+ ret = -EFAULT;
+
+err:
+ kfree(kern_buff);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+/**
+ * ggs_show - Show global general storage (ggs) sysfs attribute
+ * @device: Device structure
+ * @attr: Device attribute structure
+ * @buf: Requested available shutdown_scope attributes string
+ * @reg: Register number
+ *
+ * Return:Number of bytes printed into the buffer.
+ *
+ * Helper function for viewing a ggs register value.
+ *
+ * User-space interface for viewing the content of the ggs0 register.
+ * cat /sys/firmware/zynqmp/ggs0
+ */
+static ssize_t ggs_show(struct device *device,
+ struct device_attribute *attr,
+ char *buf,
+ u32 reg)
+{
+ return read_register(buf, IOCTL_READ_GGS, reg);
+}
+
+/**
+ * ggs_store - Store global general storage (ggs) sysfs attribute
+ * @device: Device structure
+ * @attr: Device attribute structure
+ * @buf: User entered shutdown_scope attribute string
+ * @count: Size of buf
+ * @reg: Register number
+ *
+ * Return: count argument if request succeeds, the corresponding
+ * error code otherwise
+ *
+ * Helper function for storing a ggs register value.
+ *
+ * For example, the user-space interface for storing a value to the
+ * ggs0 register:
+ * echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/ggs0
+ */
+static ssize_t ggs_store(struct device *device,
+ struct device_attribute *attr,
+ const char *buf, size_t count,
+ u32 reg)
+{
+ if (!device || !attr || !buf || !count || reg >= GSS_NUM_REGS)
+ return -EINVAL;
+
+ return write_register(buf, count, IOCTL_READ_GGS, IOCTL_WRITE_GGS, reg);
+}
+
+/* GGS register show functions */
+#define GGS0_SHOW(N) \
+ ssize_t ggs##N##_show(struct device *device, \
+ struct device_attribute *attr, \
+ char *buf) \
+ { \
+ return ggs_show(device, attr, buf, N); \
+ }
+
+static GGS0_SHOW(0);
+static GGS0_SHOW(1);
+static GGS0_SHOW(2);
+static GGS0_SHOW(3);
+
+/* GGS register store function */
+#define GGS0_STORE(N) \
+ ssize_t ggs##N##_store(struct device *device, \
+ struct device_attribute *attr, \
+ const char *buf, \
+ size_t count) \
+ { \
+ return ggs_store(device, attr, buf, count, N); \
+ }
+
+static GGS0_STORE(0);
+static GGS0_STORE(1);
+static GGS0_STORE(2);
+static GGS0_STORE(3);
+
+/**
+ * pggs_show - Show persistent global general storage (pggs) sysfs attribute
+ * @device: Device structure
+ * @attr: Device attribute structure
+ * @buf: Requested available shutdown_scope attributes string
+ * @reg: Register number
+ *
+ * Return:Number of bytes printed into the buffer.
+ *
+ * Helper function for viewing a pggs register value.
+ */
+static ssize_t pggs_show(struct device *device,
+ struct device_attribute *attr,
+ char *buf,
+ u32 reg)
+{
+ return read_register(buf, IOCTL_READ_PGGS, reg);
+}
+
+/**
+ * pggs_store - Store persistent global general storage (pggs) sysfs attribute
+ * @device: Device structure
+ * @attr: Device attribute structure
+ * @buf: User entered shutdown_scope attribute string
+ * @count: Size of buf
+ * @reg: Register number
+ *
+ * Return: count argument if request succeeds, the corresponding
+ * error code otherwise
+ *
+ * Helper function for storing a pggs register value.
+ */
+static ssize_t pggs_store(struct device *device,
+ struct device_attribute *attr,
+ const char *buf, size_t count,
+ u32 reg)
+{
+ return write_register(buf, count, IOCTL_READ_PGGS,
+ IOCTL_WRITE_PGGS, reg);
+}
+
+#define PGGS0_SHOW(N) \
+ ssize_t pggs##N##_show(struct device *device, \
+ struct device_attribute *attr, \
+ char *buf) \
+ { \
+ return pggs_show(device, attr, buf, N); \
+ }
+
+#define PGGS0_STORE(N) \
+ ssize_t pggs##N##_store(struct device *device, \
+ struct device_attribute *attr, \
+ const char *buf, \
+ size_t count) \
+ { \
+ return pggs_store(device, attr, buf, count, N); \
+ }
+
+/* PGGS register show functions */
+static PGGS0_SHOW(0);
+static PGGS0_SHOW(1);
+static PGGS0_SHOW(2);
+static PGGS0_SHOW(3);
+
+/* PGGS register store functions */
+static PGGS0_STORE(0);
+static PGGS0_STORE(1);
+static PGGS0_STORE(2);
+static PGGS0_STORE(3);
+
+/* GGS register attributes */
+static DEVICE_ATTR_RW(ggs0);
+static DEVICE_ATTR_RW(ggs1);
+static DEVICE_ATTR_RW(ggs2);
+static DEVICE_ATTR_RW(ggs3);
+
+/* PGGS register attributes */
+static DEVICE_ATTR_RW(pggs0);
+static DEVICE_ATTR_RW(pggs1);
+static DEVICE_ATTR_RW(pggs2);
+static DEVICE_ATTR_RW(pggs3);
+
+static struct attribute *zynqmp_ggs_attrs[] = {
+ &dev_attr_ggs0.attr,
+ &dev_attr_ggs1.attr,
+ &dev_attr_ggs2.attr,
+ &dev_attr_ggs3.attr,
+ &dev_attr_pggs0.attr,
+ &dev_attr_pggs1.attr,
+ &dev_attr_pggs2.attr,
+ &dev_attr_pggs3.attr,
+ NULL,
+};
+
+ATTRIBUTE_GROUPS(zynqmp_ggs);
+
+int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
+{
+ return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);
+}
diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index 75bdfaa..4c1117d 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
case IOCTL_GET_PLL_FRAC_MODE:
case IOCTL_SET_PLL_FRAC_DATA:
case IOCTL_GET_PLL_FRAC_DATA:
+ case IOCTL_WRITE_GGS:
+ case IOCTL_READ_GGS:
+ case IOCTL_WRITE_PGGS:
+ case IOCTL_READ_PGGS:
return 1;
default:
return 0;
@@ -704,6 +708,28 @@ const struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
}
EXPORT_SYMBOL_GPL(zynqmp_pm_get_eemi_ops);
+static int zynqmp_pm_sysfs_init(void)
+{
+ struct kobject *zynqmp_kobj;
+ int ret;
+
+ zynqmp_kobj = kobject_create_and_add("zynqmp", firmware_kobj);
+ if (!zynqmp_kobj) {
+ pr_err("zynqmp: Firmware kobj add failed.\n");
+ return -ENOMEM;
+ }
+
+ ret = zynqmp_pm_ggs_init(zynqmp_kobj);
+ if (ret) {
+ kobject_put(zynqmp_kobj);
+ pr_err("%s() GGS init fail with error %d\n",
+ __func__, ret);
+ goto err;
+ }
+err:
+ return ret;
+}
+
static int zynqmp_firmware_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -751,6 +777,12 @@ static int zynqmp_firmware_probe(struct platform_device *pdev)
/* Assign eemi_ops_table */
eemi_ops_tbl = &eemi_ops;
+ ret = zynqmp_pm_sysfs_init();
+ if (ret) {
+ pr_err("%s() sysfs init fail with error %d\n", __func__, ret);
+ return ret;
+ }
+
zynqmp_pm_api_debugfs_init();
ret = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, firmware_devs,
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index e41ad9e..534814e 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -13,6 +13,8 @@
#ifndef __FIRMWARE_ZYNQMP_H__
#define __FIRMWARE_ZYNQMP_H__
+#include <linux/device.h>
+
#define ZYNQMP_PM_VERSION_MAJOR 1
#define ZYNQMP_PM_VERSION_MINOR 0
@@ -42,6 +44,8 @@
#define ZYNQMP_PM_MAX_QOS 100U
+#define GSS_NUM_REGS (4)
+
/* Node capabilities */
#define ZYNQMP_PM_CAPABILITY_ACCESS 0x1U
#define ZYNQMP_PM_CAPABILITY_CONTEXT 0x2U
@@ -97,6 +101,10 @@ enum pm_ioctl_id {
IOCTL_GET_PLL_FRAC_MODE,
IOCTL_SET_PLL_FRAC_DATA,
IOCTL_GET_PLL_FRAC_DATA,
+ IOCTL_WRITE_GGS,
+ IOCTL_READ_GGS,
+ IOCTL_WRITE_PGGS,
+ IOCTL_READ_PGGS,
};
enum pm_query_id {
@@ -311,6 +319,8 @@ struct zynqmp_eemi_ops {
int zynqmp_pm_invoke_fn(u32 pm_api_id, u32 arg0, u32 arg1,
u32 arg2, u32 arg3, u32 *ret_payload);
+int zynqmp_pm_ggs_init(struct kobject *parent_kobj);
+
#if IS_REACHABLE(CONFIG_ARCH_ZYNQMP)
const struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void);
#else
--
2.7.4
From: Rajan Vaja <[email protected]>
Add system shutdown API interface which asks firmware to
perform system shutdown/restart.
Signed-off-by: Rajan Vaja <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
Signed-off-by: Jolly Shah <[email protected]>
---
Changes in v2:
- Updated Signed-off-by sequence.
---
drivers/firmware/xilinx/zynqmp.c | 14 ++++++++++++++
include/linux/firmware/xlnx-zynqmp.h | 4 +++-
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index 4c1117d..0f90793 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -668,6 +668,19 @@ static int zynqmp_pm_set_requirement(const u32 node, const u32 capabilities,
qos, ack, NULL);
}
+/**
+ * zynqmp_pm_system_shutdown - PM call to request a system shutdown or restart
+ * @type: Shutdown or restart? 0 for shutdown, 1 for restart
+ * @subtype: Specifies which system should be restarted or shut down
+ *
+ * Return: Returns status, either success or error+reason
+ */
+static int zynqmp_pm_system_shutdown(const u32 type, const u32 subtype)
+{
+ return zynqmp_pm_invoke_fn(PM_SYSTEM_SHUTDOWN, type, subtype,
+ 0, 0, NULL);
+}
+
static const struct zynqmp_eemi_ops eemi_ops = {
.get_api_version = zynqmp_pm_get_api_version,
.get_chipid = zynqmp_pm_get_chipid,
@@ -691,6 +704,7 @@ static const struct zynqmp_eemi_ops eemi_ops = {
.set_requirement = zynqmp_pm_set_requirement,
.fpga_load = zynqmp_pm_fpga_load,
.fpga_get_status = zynqmp_pm_fpga_get_status,
+ .system_shutdown = zynqmp_pm_system_shutdown,
};
/**
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 534814e..1fd246c 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -62,7 +62,8 @@
enum pm_api_id {
PM_GET_API_VERSION = 1,
- PM_REQUEST_NODE = 13,
+ PM_SYSTEM_SHUTDOWN = 12,
+ PM_REQUEST_NODE,
PM_RELEASE_NODE,
PM_SET_REQUIREMENT,
PM_RESET_ASSERT = 17,
@@ -314,6 +315,7 @@ struct zynqmp_eemi_ops {
const u32 capabilities,
const u32 qos,
const enum zynqmp_pm_request_ack ack);
+ int (*system_shutdown)(const u32 type, const u32 subtype);
};
int zynqmp_pm_invoke_fn(u32 pm_api_id, u32 arg0, u32 arg1,
--
2.7.4
From: Rajan Vaja <[email protected]>
The Linux shutdown functionality implemented via PSCI system_off does
not include an option to set a scope, i.e. which parts of the system to
shut down.
This patch creates sysfs that allows to set the shutdown scope for the
next shutdown request. When the next shutdown is performed, the platform
specific portion of PSCI-system_off can use the chosen shutdown scope.
Signed-off-by: Rajan Vaja <[email protected]>
Signed-off-by: Stefan Krsmanovic <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
Signed-off-by: Jolly Shah <[email protected]>
---
Changes in v2:
- Updated Linux kernel version in documentation.
- Used DEVICE_ATTR_* and ATTRIBUTE_GROUPS macros.
- Free Kobject structure in case of error.
- Updated Signed-off-by sequence.
---
Documentation/ABI/stable/sysfs-firmware-zynqmp | 32 +++++
drivers/firmware/xilinx/zynqmp.c | 155 +++++++++++++++++++++++++
include/linux/firmware/xlnx-zynqmp.h | 12 ++
3 files changed, 199 insertions(+)
diff --git a/Documentation/ABI/stable/sysfs-firmware-zynqmp b/Documentation/ABI/stable/sysfs-firmware-zynqmp
index cffa2fc..f41e9c5 100644
--- a/Documentation/ABI/stable/sysfs-firmware-zynqmp
+++ b/Documentation/ABI/stable/sysfs-firmware-zynqmp
@@ -48,3 +48,35 @@ Description:
# echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/pggs0
Users: Xilinx
+
+What: /sys/firmware/zynqmp/shutdown_scope
+Date: February 2018
+KernelVersion: 5.5
+Contact: "Jolly Shah" <[email protected]>
+Description:
+ This sysfs interface allows to set the shutdown scope for the
+ next shutdown request. When the next shutdown is performed, the
+ platform specific portion of PSCI-system_off can use the chosen
+ shutdown scope.
+
+ Following are available shutdown scopes(subtypes):
+
+ subsystem: Only the APU along with all of its peripherals
+ not used by other processing units will be
+ shut down. This may result in the FPD power
+ domain being shut down provided that no other
+ processing unit uses FPD peripherals or DRAM.
+ ps_only: The complete PS will be shut down, including the
+ RPU, PMU, etc. Only the PL domain (FPGA)
+ remains untouched.
+ system: The complete system/device is shut down.
+
+ Usage:
+ # cat /sys/firmware/zynqmp/shutdown_scope
+ # echo <scope> > /sys/firmware/zynqmp/shutdown_scope
+
+ Example:
+ # cat /sys/firmware/zynqmp/shutdown_scope
+ # echo "subsystem" > /sys/firmware/zynqmp/shutdown_scope
+
+Users: Xilinx
diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
index 0f90793..ef7d107 100644
--- a/drivers/firmware/xilinx/zynqmp.c
+++ b/drivers/firmware/xilinx/zynqmp.c
@@ -722,6 +722,152 @@ const struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
}
EXPORT_SYMBOL_GPL(zynqmp_pm_get_eemi_ops);
+/**
+ * struct zynqmp_pm_shutdown_scope - Struct for shutdown scope
+ * @subtype: Shutdown subtype
+ * @name: Matching string for scope argument
+ *
+ * This struct encapsulates mapping between shutdown scope ID and string.
+ */
+struct zynqmp_pm_shutdown_scope {
+ const enum zynqmp_pm_shutdown_subtype subtype;
+ const char *name;
+};
+
+static struct zynqmp_pm_shutdown_scope shutdown_scopes[] = {
+ [ZYNQMP_PM_SHUTDOWN_SUBTYPE_SUBSYSTEM] = {
+ .subtype = ZYNQMP_PM_SHUTDOWN_SUBTYPE_SUBSYSTEM,
+ .name = "subsystem",
+ },
+ [ZYNQMP_PM_SHUTDOWN_SUBTYPE_PS_ONLY] = {
+ .subtype = ZYNQMP_PM_SHUTDOWN_SUBTYPE_PS_ONLY,
+ .name = "ps_only",
+ },
+ [ZYNQMP_PM_SHUTDOWN_SUBTYPE_SYSTEM] = {
+ .subtype = ZYNQMP_PM_SHUTDOWN_SUBTYPE_SYSTEM,
+ .name = "system",
+ },
+};
+
+static struct zynqmp_pm_shutdown_scope *selected_scope =
+ &shutdown_scopes[ZYNQMP_PM_SHUTDOWN_SUBTYPE_SYSTEM];
+
+/**
+ * zynqmp_pm_is_shutdown_scope_valid - Check if shutdown scope string is valid
+ * @scope_string: Shutdown scope string
+ *
+ * Return: Return pointer to matching shutdown scope struct from
+ * array of available options in system if string is valid,
+ * otherwise returns NULL.
+ */
+static struct zynqmp_pm_shutdown_scope*
+ zynqmp_pm_is_shutdown_scope_valid(const char *scope_string)
+{
+ int count;
+
+ for (count = 0; count < ARRAY_SIZE(shutdown_scopes); count++)
+ if (sysfs_streq(scope_string, shutdown_scopes[count].name))
+ return &shutdown_scopes[count];
+
+ return NULL;
+}
+
+/**
+ * shutdown_scope_show - Show shutdown_scope sysfs attribute
+ * @device: Device structure
+ * @attr: Device attribute structure
+ * @buf: Requested available shutdown_scope attributes string
+ *
+ * User-space interface for viewing the available scope options for system
+ * shutdown. Scope option for next shutdown call is marked with [].
+ *
+ * Usage: cat /sys/firmware/zynqmp/shutdown_scope
+ *
+ * Return: Number of bytes printed into the buffer.
+ */
+static ssize_t shutdown_scope_show(struct device *device,
+ struct device_attribute *attr,
+ char *buf)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(shutdown_scopes); i++) {
+ if (&shutdown_scopes[i] == selected_scope) {
+ strcat(buf, "[");
+ strcat(buf, shutdown_scopes[i].name);
+ strcat(buf, "]");
+ } else {
+ strcat(buf, shutdown_scopes[i].name);
+ }
+ strcat(buf, " ");
+ }
+ strcat(buf, "\n");
+
+ return strlen(buf);
+}
+
+/**
+ * shutdown_scope_store - Store shutdown_scope sysfs attribute
+ * @device: Device structure
+ * @attr: Device attribute structure
+ * @buf: User entered shutdown_scope attribute string
+ * @count: Buffer size
+ *
+ * User-space interface for setting the scope for the next system shutdown.
+ * Usage: echo <scope> > /sys/firmware/zynqmp/shutdown_scope
+ *
+ * The Linux shutdown functionality implemented via PSCI system_off does not
+ * include an option to set a scope, i.e. which parts of the system to shut
+ * down.
+ *
+ * This API function allows to set the shutdown scope for the next shutdown
+ * request by passing it to the ATF running in EL3. When the next shutdown
+ * is performed, the platform specific portion of PSCI-system_off can use
+ * the chosen shutdown scope.
+ *
+ * subsystem: Only the APU along with all of its peripherals not used by other
+ * processing units will be shut down. This may result in the FPD
+ * power domain being shut down provided that no other processing
+ * unit uses FPD peripherals or DRAM.
+ * ps_only: The complete PS will be shut down, including the RPU, PMU, etc.
+ * Only the PL domain (FPGA) remains untouched.
+ * system: The complete system/device is shut down.
+ *
+ * Return: count argument if request succeeds, the corresponding error
+ * code otherwise
+ */
+static ssize_t shutdown_scope_store(struct device *device,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret;
+ struct zynqmp_pm_shutdown_scope *scope;
+
+ scope = zynqmp_pm_is_shutdown_scope_valid(buf);
+ if (!scope)
+ return -EINVAL;
+
+ ret = zynqmp_pm_system_shutdown(ZYNQMP_PM_SHUTDOWN_TYPE_SETSCOPE_ONLY,
+ scope->subtype);
+ if (ret) {
+ pr_err("unable to set shutdown scope %s\n", buf);
+ return ret;
+ }
+
+ selected_scope = scope;
+
+ return count;
+}
+
+static DEVICE_ATTR_RW(shutdown_scope);
+
+static struct attribute *zynqmp_attrs[] = {
+ &dev_attr_shutdown_scope.attr,
+ NULL,
+};
+
+ATTRIBUTE_GROUPS(zynqmp);
+
static int zynqmp_pm_sysfs_init(void)
{
struct kobject *zynqmp_kobj;
@@ -733,6 +879,14 @@ static int zynqmp_pm_sysfs_init(void)
return -ENOMEM;
}
+ ret = sysfs_create_group(zynqmp_kobj, zynqmp_groups[0]);
+ if (ret) {
+ kobject_put(zynqmp_kobj);
+ pr_err("%s() sysfs creation fail with error %d\n",
+ __func__, ret);
+ goto err;
+ }
+
ret = zynqmp_pm_ggs_init(zynqmp_kobj);
if (ret) {
kobject_put(zynqmp_kobj);
@@ -740,6 +894,7 @@ static int zynqmp_pm_sysfs_init(void)
__func__, ret);
goto err;
}
+
err:
return ret;
}
diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h
index 1fd246c..e4f83c6 100644
--- a/include/linux/firmware/xlnx-zynqmp.h
+++ b/include/linux/firmware/xlnx-zynqmp.h
@@ -271,6 +271,18 @@ enum tap_delay_type {
PM_TAPDELAY_OUTPUT,
};
+enum zynqmp_pm_shutdown_type {
+ ZYNQMP_PM_SHUTDOWN_TYPE_SHUTDOWN,
+ ZYNQMP_PM_SHUTDOWN_TYPE_RESET,
+ ZYNQMP_PM_SHUTDOWN_TYPE_SETSCOPE_ONLY,
+};
+
+enum zynqmp_pm_shutdown_subtype {
+ ZYNQMP_PM_SHUTDOWN_SUBTYPE_SUBSYSTEM,
+ ZYNQMP_PM_SHUTDOWN_SUBTYPE_PS_ONLY,
+ ZYNQMP_PM_SHUTDOWN_SUBTYPE_SYSTEM,
+};
+
/**
* struct zynqmp_pm_query_data - PM query data
* @qid: query ID
--
2.7.4
On Wed, Jan 08, 2020 at 03:54:20PM -0800, Jolly Shah wrote:
> From: Rajan Vaja <[email protected]>
>
> Add Firmware-ggs sysfs interface which provides read/write
> interface to global storage registers.
>
> Signed-off-by: Rajan Vaja <[email protected]>
> Signed-off-by: Michal Simek <[email protected]>
> Signed-off-by: Jolly Shah <[email protected]>
> ---
> Changes in v2:
> - Updated Linux kernel version in documentation.
> - Used DEVICE_ATTR_* and ATTRIBUTE_GROUPS macros.
> - Free Kobject structure in case of error.
> - Resolved smatch errors.
> - Updated Signed-off-by sequence.
> ---
> Documentation/ABI/stable/sysfs-firmware-zynqmp | 50 +++++
> drivers/firmware/xilinx/Makefile | 2 +-
> drivers/firmware/xilinx/zynqmp-ggs.c | 284 +++++++++++++++++++++++++
> drivers/firmware/xilinx/zynqmp.c | 32 +++
> include/linux/firmware/xlnx-zynqmp.h | 10 +
> 5 files changed, 377 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/ABI/stable/sysfs-firmware-zynqmp
> create mode 100644 drivers/firmware/xilinx/zynqmp-ggs.c
>
> diff --git a/Documentation/ABI/stable/sysfs-firmware-zynqmp b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> new file mode 100644
> index 0000000..cffa2fc
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> @@ -0,0 +1,50 @@
> +What: /sys/firmware/zynqmp/ggs*
Why are these attributes just not hanging off of the platform device for
the firmware controller? Why do you need a new subdir under "firmware"?
> +Date: January 2018
> +KernelVersion: 5.5
5.6? :)
> +Contact: "Jolly Shah" <[email protected]>
> +Description:
> + Read/Write PMU global general storage register value,
> + GLOBAL_GEN_STORAGE{0:3}.
> + Global general storage register that can be used
> + by system to pass information between masters.
> +
> + The register is reset during system or power-on
> + resets. Three registers are used by the FSBL and
> + other Xilinx software products: GLOBAL_GEN_STORAGE{4:6}.
> +
> + Usage:
> + # cat /sys/firmware/zynqmp/ggs0
> + # echo <mask> <value> > /sys/firmware/zynqmp/ggs0
> +
> + Example:
> + # cat /sys/firmware/zynqmp/ggs0
> + # echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/ggs0
> +
> +Users: Xilinx
> +
> +What: /sys/firmware/zynqmp/pggs*
> +Date: January 2018
> +KernelVersion: 5.5
> +Contact: "Jolly Shah" <[email protected]>
> +Description:
> + Read/Write PMU persistent global general storage register
> + value, PERS_GLOB_GEN_STORAGE{0:3}.
> + Persistent global general storage register that
> + can be used by system to pass information between
> + masters.
> +
> + This register is only reset by the power-on reset
> + and maintains its value through a system reset.
> + Four registers are used by the FSBL and other Xilinx
> + software products: PERS_GLOB_GEN_STORAGE{4:7}.
> + Register is reset only by a POR reset.
> +
> + Usage:
> + # cat /sys/firmware/zynqmp/pggs0
> + # echo <mask> <value> > /sys/firmware/zynqmp/pggs0
> +
> + Example:
> + # cat /sys/firmware/zynqmp/pggs0
> + # echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/pggs0
> +
> +Users: Xilinx
> diff --git a/drivers/firmware/xilinx/Makefile b/drivers/firmware/xilinx/Makefile
> index 875a537..1e8643c 100644
> --- a/drivers/firmware/xilinx/Makefile
> +++ b/drivers/firmware/xilinx/Makefile
> @@ -1,5 +1,5 @@
> # SPDX-License-Identifier: GPL-2.0
> # Makefile for Xilinx firmwares
>
> -obj-$(CONFIG_ZYNQMP_FIRMWARE) += zynqmp.o
> +obj-$(CONFIG_ZYNQMP_FIRMWARE) += zynqmp.o zynqmp-ggs.o
> obj-$(CONFIG_ZYNQMP_FIRMWARE_DEBUG) += zynqmp-debug.o
> diff --git a/drivers/firmware/xilinx/zynqmp-ggs.c b/drivers/firmware/xilinx/zynqmp-ggs.c
> new file mode 100644
> index 0000000..e2a6700
> --- /dev/null
> +++ b/drivers/firmware/xilinx/zynqmp-ggs.c
> @@ -0,0 +1,284 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Xilinx Zynq MPSoC Firmware layer
> + *
> + * Copyright (C) 2014-2018 Xilinx, Inc.
> + *
> + * Jolly Shah <[email protected]>
> + * Rajan Vaja <[email protected]>
> + */
> +
> +#include <linux/compiler.h>
> +#include <linux/of.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/firmware/xlnx-zynqmp.h>
> +
> +static ssize_t read_register(char *buf, u32 ioctl_id, u32 reg)
> +{
> + int ret;
> + u32 ret_payload[PAYLOAD_ARG_CNT];
> + const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
> +
> + if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
> + return -EFAULT;
> +
> + ret = eemi_ops->ioctl(0, ioctl_id, reg, 0, ret_payload);
> + if (ret)
> + return ret;
> +
> + return sprintf(buf, "0x%x\n", ret_payload[1]);
> +}
> +
> +static ssize_t write_register(const char *buf, size_t count, u32 read_ioctl,
> + u32 write_ioctl, u32 reg)
> +{
> + char *kern_buff, *inbuf, *tok;
> + long mask, value;
> + int ret;
> + u32 ret_payload[PAYLOAD_ARG_CNT];
> + const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
> +
> + if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
> + return -EFAULT;
> +
> + kern_buff = kzalloc(count, GFP_KERNEL);
> + if (!kern_buff)
> + return -ENOMEM;
> +
> + ret = strlcpy(kern_buff, buf, count);
> + if (ret < 0) {
> + ret = -EFAULT;
> + goto err;
> + }
> +
> + inbuf = kern_buff;
> +
> + /* Read the write mask */
> + tok = strsep(&inbuf, " ");
> + if (!tok) {
> + ret = -EFAULT;
If you just set count to the error value, no need to test the value of
ret when you exit. Not a big deal...
> + goto err;
> + }
> +
> + ret = kstrtol(tok, 16, &mask);
> + if (ret) {
> + ret = -EFAULT;
> + goto err;
> + }
> +
> + /* Read the write value */
> + tok = strsep(&inbuf, " ");
> + if (!tok) {
> + ret = -EFAULT;
> + goto err;
> + }
> +
> + ret = kstrtol(tok, 16, &value);
> + if (ret) {
> + ret = -EFAULT;
> + goto err;
> + }
> +
> + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
This feels "tricky", if you tie this to the device you have your driver
bound to, will this make it easier instead of having to go through the
ioctl callback?
> + if (ret) {
> + ret = -EFAULT;
> + goto err;
> + }
> + ret_payload[1] &= ~mask;
> + value &= mask;
> + value |= ret_payload[1];
> +
> + ret = eemi_ops->ioctl(0, write_ioctl, reg, value, NULL);
> + if (ret)
> + ret = -EFAULT;
> +
> +err:
> + kfree(kern_buff);
> + if (ret)
> + return ret;
> +
> + return count;
> +}
> +
> +/**
> + * ggs_show - Show global general storage (ggs) sysfs attribute
> + * @device: Device structure
> + * @attr: Device attribute structure
> + * @buf: Requested available shutdown_scope attributes string
> + * @reg: Register number
> + *
> + * Return:Number of bytes printed into the buffer.
> + *
> + * Helper function for viewing a ggs register value.
> + *
> + * User-space interface for viewing the content of the ggs0 register.
> + * cat /sys/firmware/zynqmp/ggs0
> + */
> +static ssize_t ggs_show(struct device *device,
> + struct device_attribute *attr,
> + char *buf,
> + u32 reg)
> +{
> + return read_register(buf, IOCTL_READ_GGS, reg);
> +}
> +
> +/**
> + * ggs_store - Store global general storage (ggs) sysfs attribute
> + * @device: Device structure
> + * @attr: Device attribute structure
> + * @buf: User entered shutdown_scope attribute string
> + * @count: Size of buf
> + * @reg: Register number
> + *
> + * Return: count argument if request succeeds, the corresponding
> + * error code otherwise
> + *
> + * Helper function for storing a ggs register value.
> + *
> + * For example, the user-space interface for storing a value to the
> + * ggs0 register:
> + * echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/ggs0
> + */
> +static ssize_t ggs_store(struct device *device,
> + struct device_attribute *attr,
> + const char *buf, size_t count,
> + u32 reg)
> +{
> + if (!device || !attr || !buf || !count || reg >= GSS_NUM_REGS)
> + return -EINVAL;
> +
> + return write_register(buf, count, IOCTL_READ_GGS, IOCTL_WRITE_GGS, reg);
> +}
> +
> +/* GGS register show functions */
> +#define GGS0_SHOW(N) \
> + ssize_t ggs##N##_show(struct device *device, \
> + struct device_attribute *attr, \
> + char *buf) \
> + { \
> + return ggs_show(device, attr, buf, N); \
> + }
> +
> +static GGS0_SHOW(0);
> +static GGS0_SHOW(1);
> +static GGS0_SHOW(2);
> +static GGS0_SHOW(3);
> +
> +/* GGS register store function */
> +#define GGS0_STORE(N) \
> + ssize_t ggs##N##_store(struct device *device, \
> + struct device_attribute *attr, \
> + const char *buf, \
> + size_t count) \
> + { \
> + return ggs_store(device, attr, buf, count, N); \
> + }
> +
> +static GGS0_STORE(0);
> +static GGS0_STORE(1);
> +static GGS0_STORE(2);
> +static GGS0_STORE(3);
> +
> +/**
> + * pggs_show - Show persistent global general storage (pggs) sysfs attribute
> + * @device: Device structure
> + * @attr: Device attribute structure
> + * @buf: Requested available shutdown_scope attributes string
> + * @reg: Register number
> + *
> + * Return:Number of bytes printed into the buffer.
> + *
> + * Helper function for viewing a pggs register value.
> + */
> +static ssize_t pggs_show(struct device *device,
> + struct device_attribute *attr,
> + char *buf,
> + u32 reg)
> +{
> + return read_register(buf, IOCTL_READ_PGGS, reg);
> +}
> +
> +/**
> + * pggs_store - Store persistent global general storage (pggs) sysfs attribute
> + * @device: Device structure
> + * @attr: Device attribute structure
> + * @buf: User entered shutdown_scope attribute string
> + * @count: Size of buf
> + * @reg: Register number
> + *
> + * Return: count argument if request succeeds, the corresponding
> + * error code otherwise
> + *
> + * Helper function for storing a pggs register value.
> + */
> +static ssize_t pggs_store(struct device *device,
> + struct device_attribute *attr,
> + const char *buf, size_t count,
> + u32 reg)
> +{
> + return write_register(buf, count, IOCTL_READ_PGGS,
> + IOCTL_WRITE_PGGS, reg);
> +}
> +
> +#define PGGS0_SHOW(N) \
> + ssize_t pggs##N##_show(struct device *device, \
> + struct device_attribute *attr, \
> + char *buf) \
> + { \
> + return pggs_show(device, attr, buf, N); \
> + }
> +
> +#define PGGS0_STORE(N) \
> + ssize_t pggs##N##_store(struct device *device, \
> + struct device_attribute *attr, \
> + const char *buf, \
> + size_t count) \
> + { \
> + return pggs_store(device, attr, buf, count, N); \
> + }
> +
> +/* PGGS register show functions */
> +static PGGS0_SHOW(0);
> +static PGGS0_SHOW(1);
> +static PGGS0_SHOW(2);
> +static PGGS0_SHOW(3);
> +
> +/* PGGS register store functions */
> +static PGGS0_STORE(0);
> +static PGGS0_STORE(1);
> +static PGGS0_STORE(2);
> +static PGGS0_STORE(3);
> +
> +/* GGS register attributes */
> +static DEVICE_ATTR_RW(ggs0);
> +static DEVICE_ATTR_RW(ggs1);
> +static DEVICE_ATTR_RW(ggs2);
> +static DEVICE_ATTR_RW(ggs3);
> +
> +/* PGGS register attributes */
> +static DEVICE_ATTR_RW(pggs0);
> +static DEVICE_ATTR_RW(pggs1);
> +static DEVICE_ATTR_RW(pggs2);
> +static DEVICE_ATTR_RW(pggs3);
> +
> +static struct attribute *zynqmp_ggs_attrs[] = {
> + &dev_attr_ggs0.attr,
> + &dev_attr_ggs1.attr,
> + &dev_attr_ggs2.attr,
> + &dev_attr_ggs3.attr,
> + &dev_attr_pggs0.attr,
> + &dev_attr_pggs1.attr,
> + &dev_attr_pggs2.attr,
> + &dev_attr_pggs3.attr,
> + NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(zynqmp_ggs);
> +
> +int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
> +{
> + return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);
You might be racing userspace here and loosing :(
> +}
> diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c
> index 75bdfaa..4c1117d 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
> case IOCTL_GET_PLL_FRAC_MODE:
> case IOCTL_SET_PLL_FRAC_DATA:
> case IOCTL_GET_PLL_FRAC_DATA:
> + case IOCTL_WRITE_GGS:
> + case IOCTL_READ_GGS:
> + case IOCTL_WRITE_PGGS:
> + case IOCTL_READ_PGGS:
Huh???
> return 1;
> default:
> return 0;
> @@ -704,6 +708,28 @@ const struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
> }
> EXPORT_SYMBOL_GPL(zynqmp_pm_get_eemi_ops);
>
> +static int zynqmp_pm_sysfs_init(void)
> +{
> + struct kobject *zynqmp_kobj;
> + int ret;
> +
> + zynqmp_kobj = kobject_create_and_add("zynqmp", firmware_kobj);
> + if (!zynqmp_kobj) {
> + pr_err("zynqmp: Firmware kobj add failed.\n");
> + return -ENOMEM;
> + }
> +
> + ret = zynqmp_pm_ggs_init(zynqmp_kobj);
> + if (ret) {
> + kobject_put(zynqmp_kobj);
> + pr_err("%s() GGS init fail with error %d\n",
> + __func__, ret);
> + goto err;
> + }
> +err:
> + return ret;
> +}
> +
> static int zynqmp_firmware_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -751,6 +777,12 @@ static int zynqmp_firmware_probe(struct platform_device *pdev)
> /* Assign eemi_ops_table */
> eemi_ops_tbl = &eemi_ops;
>
> + ret = zynqmp_pm_sysfs_init();
See, you have a platform device, hang the attributes off of that instead
of making a kobject and detatching yourself from the global device tree!
Please redo this, I think it will make it a lot simpler and more
obvious.
thanks,
greg k-h
Hi Greg,
Thanks for the review.
> -----Original Message-----
> From: Greg KH <[email protected]>
> Sent: Tuesday, January 14, 2020 6:53 AM
> To: Jolly Shah <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Michal Simek <[email protected]>; Rajan Vaja
> <[email protected]>; [email protected]; linux-
> [email protected]; Rajan Vaja <[email protected]>; Jolly Shah
> <[email protected]>
> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
>
> On Wed, Jan 08, 2020 at 03:54:20PM -0800, Jolly Shah wrote:
> > From: Rajan Vaja <[email protected]>
> >
> > Add Firmware-ggs sysfs interface which provides read/write
> > interface to global storage registers.
> >
> > Signed-off-by: Rajan Vaja <[email protected]>
> > Signed-off-by: Michal Simek <[email protected]>
> > Signed-off-by: Jolly Shah <[email protected]>
> > ---
> > Changes in v2:
> > - Updated Linux kernel version in documentation.
> > - Used DEVICE_ATTR_* and ATTRIBUTE_GROUPS macros.
> > - Free Kobject structure in case of error.
> > - Resolved smatch errors.
> > - Updated Signed-off-by sequence.
> > ---
> > Documentation/ABI/stable/sysfs-firmware-zynqmp | 50 +++++
> > drivers/firmware/xilinx/Makefile | 2 +-
> > drivers/firmware/xilinx/zynqmp-ggs.c | 284
> +++++++++++++++++++++++++
> > drivers/firmware/xilinx/zynqmp.c | 32 +++
> > include/linux/firmware/xlnx-zynqmp.h | 10 +
> > 5 files changed, 377 insertions(+), 1 deletion(-)
> > create mode 100644 Documentation/ABI/stable/sysfs-firmware-zynqmp
> > create mode 100644 drivers/firmware/xilinx/zynqmp-ggs.c
> >
> > diff --git a/Documentation/ABI/stable/sysfs-firmware-zynqmp
> b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> > new file mode 100644
> > index 0000000..cffa2fc
> > --- /dev/null
> > +++ b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> > @@ -0,0 +1,50 @@
> > +What: /sys/firmware/zynqmp/ggs*
>
> Why are these attributes just not hanging off of the platform device for
> the firmware controller? Why do you need a new subdir under "firmware"?
Firmware driver was changed later to be platform driver but these interfaces were defined
earlier and are in use.
>
> > +Date: January 2018
> > +KernelVersion: 5.5
>
> 5.6? :)
Yes. Will fix it in next version.
>
> > +Contact: "Jolly Shah" <[email protected]>
> > +Description:
> > + Read/Write PMU global general storage register value,
> > + GLOBAL_GEN_STORAGE{0:3}.
> > + Global general storage register that can be used
> > + by system to pass information between masters.
> > +
> > + The register is reset during system or power-on
> > + resets. Three registers are used by the FSBL and
> > + other Xilinx software products: GLOBAL_GEN_STORAGE{4:6}.
> > +
> > + Usage:
> > + # cat /sys/firmware/zynqmp/ggs0
> > + # echo <mask> <value> > /sys/firmware/zynqmp/ggs0
> > +
> > + Example:
> > + # cat /sys/firmware/zynqmp/ggs0
> > + # echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/ggs0
> > +
> > +Users: Xilinx
> > +
> > +What: /sys/firmware/zynqmp/pggs*
> > +Date: January 2018
> > +KernelVersion: 5.5
> > +Contact: "Jolly Shah" <[email protected]>
> > +Description:
> > + Read/Write PMU persistent global general storage register
> > + value, PERS_GLOB_GEN_STORAGE{0:3}.
> > + Persistent global general storage register that
> > + can be used by system to pass information between
> > + masters.
> > +
> > + This register is only reset by the power-on reset
> > + and maintains its value through a system reset.
> > + Four registers are used by the FSBL and other Xilinx
> > + software products: PERS_GLOB_GEN_STORAGE{4:7}.
> > + Register is reset only by a POR reset.
> > +
> > + Usage:
> > + # cat /sys/firmware/zynqmp/pggs0
> > + # echo <mask> <value> > /sys/firmware/zynqmp/pggs0
> > +
> > + Example:
> > + # cat /sys/firmware/zynqmp/pggs0
> > + # echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/pggs0
> > +
> > +Users: Xilinx
> > diff --git a/drivers/firmware/xilinx/Makefile
> b/drivers/firmware/xilinx/Makefile
> > index 875a537..1e8643c 100644
> > --- a/drivers/firmware/xilinx/Makefile
> > +++ b/drivers/firmware/xilinx/Makefile
> > @@ -1,5 +1,5 @@
> > # SPDX-License-Identifier: GPL-2.0
> > # Makefile for Xilinx firmwares
> >
> > -obj-$(CONFIG_ZYNQMP_FIRMWARE) += zynqmp.o
> > +obj-$(CONFIG_ZYNQMP_FIRMWARE) += zynqmp.o zynqmp-ggs.o
> > obj-$(CONFIG_ZYNQMP_FIRMWARE_DEBUG) += zynqmp-debug.o
> > diff --git a/drivers/firmware/xilinx/zynqmp-ggs.c
> b/drivers/firmware/xilinx/zynqmp-ggs.c
> > new file mode 100644
> > index 0000000..e2a6700
> > --- /dev/null
> > +++ b/drivers/firmware/xilinx/zynqmp-ggs.c
> > @@ -0,0 +1,284 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Xilinx Zynq MPSoC Firmware layer
> > + *
> > + * Copyright (C) 2014-2018 Xilinx, Inc.
> > + *
> > + * Jolly Shah <[email protected]>
> > + * Rajan Vaja <[email protected]>
> > + */
> > +
> > +#include <linux/compiler.h>
> > +#include <linux/of.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/slab.h>
> > +#include <linux/firmware/xlnx-zynqmp.h>
> > +
> > +static ssize_t read_register(char *buf, u32 ioctl_id, u32 reg)
> > +{
> > + int ret;
> > + u32 ret_payload[PAYLOAD_ARG_CNT];
> > + const struct zynqmp_eemi_ops *eemi_ops =
> zynqmp_pm_get_eemi_ops();
> > +
> > + if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
> > + return -EFAULT;
> > +
> > + ret = eemi_ops->ioctl(0, ioctl_id, reg, 0, ret_payload);
> > + if (ret)
> > + return ret;
> > +
> > + return sprintf(buf, "0x%x\n", ret_payload[1]);
> > +}
> > +
> > +static ssize_t write_register(const char *buf, size_t count, u32 read_ioctl,
> > + u32 write_ioctl, u32 reg)
> > +{
> > + char *kern_buff, *inbuf, *tok;
> > + long mask, value;
> > + int ret;
> > + u32 ret_payload[PAYLOAD_ARG_CNT];
> > + const struct zynqmp_eemi_ops *eemi_ops =
> zynqmp_pm_get_eemi_ops();
> > +
> > + if (IS_ERR(eemi_ops) || !eemi_ops->ioctl)
> > + return -EFAULT;
> > +
> > + kern_buff = kzalloc(count, GFP_KERNEL);
> > + if (!kern_buff)
> > + return -ENOMEM;
> > +
> > + ret = strlcpy(kern_buff, buf, count);
> > + if (ret < 0) {
> > + ret = -EFAULT;
> > + goto err;
> > + }
> > +
> > + inbuf = kern_buff;
> > +
> > + /* Read the write mask */
> > + tok = strsep(&inbuf, " ");
> > + if (!tok) {
> > + ret = -EFAULT;
>
> If you just set count to the error value, no need to test the value of
> ret when you exit. Not a big deal...
Ok. Will fix it in next version
>
> > + goto err;
> > + }
> > +
> > + ret = kstrtol(tok, 16, &mask);
> > + if (ret) {
> > + ret = -EFAULT;
> > + goto err;
> > + }
> > +
> > + /* Read the write value */
> > + tok = strsep(&inbuf, " ");
> > + if (!tok) {
> > + ret = -EFAULT;
> > + goto err;
> > + }
> > +
> > + ret = kstrtol(tok, 16, &value);
> > + if (ret) {
> > + ret = -EFAULT;
> > + goto err;
> > + }
> > +
> > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
>
> This feels "tricky", if you tie this to the device you have your driver
> bound to, will this make it easier instead of having to go through the
> ioctl callback?
>
GGS(general global storage) registers are in PMU space and linux doesn't have access to it
Hence ioctl is used.
>
> > + if (ret) {
> > + ret = -EFAULT;
> > + goto err;
> > + }
> > + ret_payload[1] &= ~mask;
> > + value &= mask;
> > + value |= ret_payload[1];
> > +
> > + ret = eemi_ops->ioctl(0, write_ioctl, reg, value, NULL);
> > + if (ret)
> > + ret = -EFAULT;
> > +
> > +err:
> > + kfree(kern_buff);
> > + if (ret)
> > + return ret;
> > +
> > + return count;
> > +}
> > +
> > +/**
> > + * ggs_show - Show global general storage (ggs) sysfs attribute
> > + * @device: Device structure
> > + * @attr: Device attribute structure
> > + * @buf: Requested available shutdown_scope attributes string
> > + * @reg: Register number
> > + *
> > + * Return:Number of bytes printed into the buffer.
> > + *
> > + * Helper function for viewing a ggs register value.
> > + *
> > + * User-space interface for viewing the content of the ggs0 register.
> > + * cat /sys/firmware/zynqmp/ggs0
> > + */
> > +static ssize_t ggs_show(struct device *device,
> > + struct device_attribute *attr,
> > + char *buf,
> > + u32 reg)
> > +{
> > + return read_register(buf, IOCTL_READ_GGS, reg);
> > +}
> > +
> > +/**
> > + * ggs_store - Store global general storage (ggs) sysfs attribute
> > + * @device: Device structure
> > + * @attr: Device attribute structure
> > + * @buf: User entered shutdown_scope attribute string
> > + * @count: Size of buf
> > + * @reg: Register number
> > + *
> > + * Return: count argument if request succeeds, the corresponding
> > + * error code otherwise
> > + *
> > + * Helper function for storing a ggs register value.
> > + *
> > + * For example, the user-space interface for storing a value to the
> > + * ggs0 register:
> > + * echo 0xFFFFFFFF 0x1234ABCD > /sys/firmware/zynqmp/ggs0
> > + */
> > +static ssize_t ggs_store(struct device *device,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count,
> > + u32 reg)
> > +{
> > + if (!device || !attr || !buf || !count || reg >= GSS_NUM_REGS)
> > + return -EINVAL;
> > +
> > + return write_register(buf, count, IOCTL_READ_GGS, IOCTL_WRITE_GGS,
> reg);
> > +}
> > +
> > +/* GGS register show functions */
> > +#define GGS0_SHOW(N) \
> > + ssize_t ggs##N##_show(struct device *device, \
> > + struct device_attribute *attr, \
> > + char *buf) \
> > + { \
> > + return ggs_show(device, attr, buf, N); \
> > + }
> > +
> > +static GGS0_SHOW(0);
> > +static GGS0_SHOW(1);
> > +static GGS0_SHOW(2);
> > +static GGS0_SHOW(3);
> > +
> > +/* GGS register store function */
> > +#define GGS0_STORE(N) \
> > + ssize_t ggs##N##_store(struct device *device, \
> > + struct device_attribute *attr, \
> > + const char *buf, \
> > + size_t count) \
> > + { \
> > + return ggs_store(device, attr, buf, count, N); \
> > + }
> > +
> > +static GGS0_STORE(0);
> > +static GGS0_STORE(1);
> > +static GGS0_STORE(2);
> > +static GGS0_STORE(3);
> > +
> > +/**
> > + * pggs_show - Show persistent global general storage (pggs) sysfs attribute
> > + * @device: Device structure
> > + * @attr: Device attribute structure
> > + * @buf: Requested available shutdown_scope attributes string
> > + * @reg: Register number
> > + *
> > + * Return:Number of bytes printed into the buffer.
> > + *
> > + * Helper function for viewing a pggs register value.
> > + */
> > +static ssize_t pggs_show(struct device *device,
> > + struct device_attribute *attr,
> > + char *buf,
> > + u32 reg)
> > +{
> > + return read_register(buf, IOCTL_READ_PGGS, reg);
> > +}
> > +
> > +/**
> > + * pggs_store - Store persistent global general storage (pggs) sysfs attribute
> > + * @device: Device structure
> > + * @attr: Device attribute structure
> > + * @buf: User entered shutdown_scope attribute string
> > + * @count: Size of buf
> > + * @reg: Register number
> > + *
> > + * Return: count argument if request succeeds, the corresponding
> > + * error code otherwise
> > + *
> > + * Helper function for storing a pggs register value.
> > + */
> > +static ssize_t pggs_store(struct device *device,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count,
> > + u32 reg)
> > +{
> > + return write_register(buf, count, IOCTL_READ_PGGS,
> > + IOCTL_WRITE_PGGS, reg);
> > +}
> > +
> > +#define PGGS0_SHOW(N) \
> > + ssize_t pggs##N##_show(struct device *device, \
> > + struct device_attribute *attr, \
> > + char *buf) \
> > + { \
> > + return pggs_show(device, attr, buf, N); \
> > + }
> > +
> > +#define PGGS0_STORE(N) \
> > + ssize_t pggs##N##_store(struct device *device, \
> > + struct device_attribute *attr, \
> > + const char *buf, \
> > + size_t count) \
> > + { \
> > + return pggs_store(device, attr, buf, count, N); \
> > + }
> > +
> > +/* PGGS register show functions */
> > +static PGGS0_SHOW(0);
> > +static PGGS0_SHOW(1);
> > +static PGGS0_SHOW(2);
> > +static PGGS0_SHOW(3);
> > +
> > +/* PGGS register store functions */
> > +static PGGS0_STORE(0);
> > +static PGGS0_STORE(1);
> > +static PGGS0_STORE(2);
> > +static PGGS0_STORE(3);
> > +
> > +/* GGS register attributes */
> > +static DEVICE_ATTR_RW(ggs0);
> > +static DEVICE_ATTR_RW(ggs1);
> > +static DEVICE_ATTR_RW(ggs2);
> > +static DEVICE_ATTR_RW(ggs3);
> > +
> > +/* PGGS register attributes */
> > +static DEVICE_ATTR_RW(pggs0);
> > +static DEVICE_ATTR_RW(pggs1);
> > +static DEVICE_ATTR_RW(pggs2);
> > +static DEVICE_ATTR_RW(pggs3);
> > +
> > +static struct attribute *zynqmp_ggs_attrs[] = {
> > + &dev_attr_ggs0.attr,
> > + &dev_attr_ggs1.attr,
> > + &dev_attr_ggs2.attr,
> > + &dev_attr_ggs3.attr,
> > + &dev_attr_pggs0.attr,
> > + &dev_attr_pggs1.attr,
> > + &dev_attr_pggs2.attr,
> > + &dev_attr_pggs3.attr,
> > + NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(zynqmp_ggs);
> > +
> > +int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
> > +{
> > + return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);
>
> You might be racing userspace here and loosing :(
Prob is called before user space is notified about sysfs so racing shouldn't happen.
Or you are referring to some other race condition?
>
> > +}
> > diff --git a/drivers/firmware/xilinx/zynqmp.c
> b/drivers/firmware/xilinx/zynqmp.c
> > index 75bdfaa..4c1117d 100644
> > --- a/drivers/firmware/xilinx/zynqmp.c
> > +++ b/drivers/firmware/xilinx/zynqmp.c
> > @@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
> > case IOCTL_GET_PLL_FRAC_MODE:
> > case IOCTL_SET_PLL_FRAC_DATA:
> > case IOCTL_GET_PLL_FRAC_DATA:
> > + case IOCTL_WRITE_GGS:
> > + case IOCTL_READ_GGS:
> > + case IOCTL_WRITE_PGGS:
> > + case IOCTL_READ_PGGS:
>
> Huh???
Sorry not sure about your concern here. These registers are in PMU space and hence
Ioctl is needed to let linux access them.
>
> > return 1;
> > default:
> > return 0;
> > @@ -704,6 +708,28 @@ const struct zynqmp_eemi_ops
> *zynqmp_pm_get_eemi_ops(void)
> > }
> > EXPORT_SYMBOL_GPL(zynqmp_pm_get_eemi_ops);
> >
> > +static int zynqmp_pm_sysfs_init(void)
> > +{
> > + struct kobject *zynqmp_kobj;
> > + int ret;
> > +
> > + zynqmp_kobj = kobject_create_and_add("zynqmp", firmware_kobj);
> > + if (!zynqmp_kobj) {
> > + pr_err("zynqmp: Firmware kobj add failed.\n");
> > + return -ENOMEM;
> > + }
> > +
> > + ret = zynqmp_pm_ggs_init(zynqmp_kobj);
> > + if (ret) {
> > + kobject_put(zynqmp_kobj);
> > + pr_err("%s() GGS init fail with error %d\n",
> > + __func__, ret);
> > + goto err;
> > + }
> > +err:
> > + return ret;
> > +}
> > +
> > static int zynqmp_firmware_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > @@ -751,6 +777,12 @@ static int zynqmp_firmware_probe(struct
> platform_device *pdev)
> > /* Assign eemi_ops_table */
> > eemi_ops_tbl = &eemi_ops;
> >
> > + ret = zynqmp_pm_sysfs_init();
>
> See, you have a platform device, hang the attributes off of that instead
> of making a kobject and detatching yourself from the global device tree!
>
> Please redo this, I think it will make it a lot simpler and more
> obvious.
Agree it will be simpler but to as firmware driver was changed to be platform driver,
to keep paths same, we used sysfs.
Thanks,
Jolly Shah
>
> thanks,
>
> greg k-h
On Thu, Jan 23, 2020 at 11:47:57PM +0000, Jolly Shah wrote:
> Hi Greg,
>
> Thanks for the review.
>
> > -----Original Message-----
> > From: Greg KH <[email protected]>
> > Sent: Tuesday, January 14, 2020 6:53 AM
> > To: Jolly Shah <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; Michal Simek <[email protected]>; Rajan Vaja
> > <[email protected]>; [email protected]; linux-
> > [email protected]; Rajan Vaja <[email protected]>; Jolly Shah
> > <[email protected]>
> > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> >
> > On Wed, Jan 08, 2020 at 03:54:20PM -0800, Jolly Shah wrote:
> > > From: Rajan Vaja <[email protected]>
> > >
> > > Add Firmware-ggs sysfs interface which provides read/write
> > > interface to global storage registers.
> > >
> > > Signed-off-by: Rajan Vaja <[email protected]>
> > > Signed-off-by: Michal Simek <[email protected]>
> > > Signed-off-by: Jolly Shah <[email protected]>
> > > ---
> > > Changes in v2:
> > > - Updated Linux kernel version in documentation.
> > > - Used DEVICE_ATTR_* and ATTRIBUTE_GROUPS macros.
> > > - Free Kobject structure in case of error.
> > > - Resolved smatch errors.
> > > - Updated Signed-off-by sequence.
> > > ---
> > > Documentation/ABI/stable/sysfs-firmware-zynqmp | 50 +++++
> > > drivers/firmware/xilinx/Makefile | 2 +-
> > > drivers/firmware/xilinx/zynqmp-ggs.c | 284
> > +++++++++++++++++++++++++
> > > drivers/firmware/xilinx/zynqmp.c | 32 +++
> > > include/linux/firmware/xlnx-zynqmp.h | 10 +
> > > 5 files changed, 377 insertions(+), 1 deletion(-)
> > > create mode 100644 Documentation/ABI/stable/sysfs-firmware-zynqmp
> > > create mode 100644 drivers/firmware/xilinx/zynqmp-ggs.c
> > >
> > > diff --git a/Documentation/ABI/stable/sysfs-firmware-zynqmp
> > b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> > > new file mode 100644
> > > index 0000000..cffa2fc
> > > --- /dev/null
> > > +++ b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> > > @@ -0,0 +1,50 @@
> > > +What: /sys/firmware/zynqmp/ggs*
> >
> > Why are these attributes just not hanging off of the platform device for
> > the firmware controller? Why do you need a new subdir under "firmware"?
>
> Firmware driver was changed later to be platform driver but these interfaces were defined
> earlier and are in use.
Defined and in use where? Not in the upstream kernel tree, right? Or
am I missing them somewhere else?
> > > + ret = kstrtol(tok, 16, &value);
> > > + if (ret) {
> > > + ret = -EFAULT;
> > > + goto err;
> > > + }
> > > +
> > > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> >
> > This feels "tricky", if you tie this to the device you have your driver
> > bound to, will this make it easier instead of having to go through the
> > ioctl callback?
> >
>
> GGS(general global storage) registers are in PMU space and linux doesn't have access to it
> Hence ioctl is used.
Why not just a "real" call to the driver to make this type of reading?
You don't have ioctls within the kernel for other drivers to call,
that's not needed at all.
> > > +int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
> > > +{
> > > + return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);
> >
> > You might be racing userspace here and loosing :(
>
> Prob is called before user space is notified about sysfs so racing shouldn't happen.
"shouldn't"? How do you know this?
> Or you are referring to some other race condition?
Your kobject was created, and notified userspace that it was present and
then sometime after that you add more attributes which userspace has no
idea are there.
If you use the proper device model interfaces, none of these problems
would be there.
>
> >
> > > +}
> > > diff --git a/drivers/firmware/xilinx/zynqmp.c
> > b/drivers/firmware/xilinx/zynqmp.c
> > > index 75bdfaa..4c1117d 100644
> > > --- a/drivers/firmware/xilinx/zynqmp.c
> > > +++ b/drivers/firmware/xilinx/zynqmp.c
> > > @@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
> > > case IOCTL_GET_PLL_FRAC_MODE:
> > > case IOCTL_SET_PLL_FRAC_DATA:
> > > case IOCTL_GET_PLL_FRAC_DATA:
> > > + case IOCTL_WRITE_GGS:
> > > + case IOCTL_READ_GGS:
> > > + case IOCTL_WRITE_PGGS:
> > > + case IOCTL_READ_PGGS:
> >
> > Huh???
>
> Sorry not sure about your concern here. These registers are in PMU space and hence
> Ioctl is needed to let linux access them.
userspace or kernelspace?
You seem to be mixing them both here.
>
> >
> > > return 1;
> > > default:
> > > return 0;
> > > @@ -704,6 +708,28 @@ const struct zynqmp_eemi_ops
> > *zynqmp_pm_get_eemi_ops(void)
> > > }
> > > EXPORT_SYMBOL_GPL(zynqmp_pm_get_eemi_ops);
> > >
> > > +static int zynqmp_pm_sysfs_init(void)
> > > +{
> > > + struct kobject *zynqmp_kobj;
> > > + int ret;
> > > +
> > > + zynqmp_kobj = kobject_create_and_add("zynqmp", firmware_kobj);
> > > + if (!zynqmp_kobj) {
> > > + pr_err("zynqmp: Firmware kobj add failed.\n");
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + ret = zynqmp_pm_ggs_init(zynqmp_kobj);
> > > + if (ret) {
> > > + kobject_put(zynqmp_kobj);
> > > + pr_err("%s() GGS init fail with error %d\n",
> > > + __func__, ret);
> > > + goto err;
> > > + }
> > > +err:
> > > + return ret;
> > > +}
> > > +
> > > static int zynqmp_firmware_probe(struct platform_device *pdev)
> > > {
> > > struct device *dev = &pdev->dev;
> > > @@ -751,6 +777,12 @@ static int zynqmp_firmware_probe(struct
> > platform_device *pdev)
> > > /* Assign eemi_ops_table */
> > > eemi_ops_tbl = &eemi_ops;
> > >
> > > + ret = zynqmp_pm_sysfs_init();
> >
> > See, you have a platform device, hang the attributes off of that instead
> > of making a kobject and detatching yourself from the global device tree!
> >
> > Please redo this, I think it will make it a lot simpler and more
> > obvious.
>
> Agree it will be simpler but to as firmware driver was changed to be platform driver,
> to keep paths same, we used sysfs.
Keep them same from what? Use the platform device as that is what it is
there for, do not go create new apis when they are not needed at all.
thanks,
greg k-h
Hi Greg,
Thanks for raising various issues that I have repeatedly asked and
constantly ignored.
On Fri, Jan 24, 2020 at 07:03:39AM +0100, Greg KH wrote:
> On Thu, Jan 23, 2020 at 11:47:57PM +0000, Jolly Shah wrote:
> > Hi Greg,
> >
[...]
> > Firmware driver was changed later to be platform driver but these
> > interfaces were defined earlier and are in use.
>
> Defined and in use where? Not in the upstream kernel tree, right? Or
> am I missing them somewhere else?
>
Exactly and they keep saying there partners are using these for 3-4 years
and they want to retain that. I have told them to change several times.
> > > > + ret = kstrtol(tok, 16, &value);
> > > > + if (ret) {
> > > > + ret = -EFAULT;
> > > > + goto err;
> > > > + }
> > > > +
> > > > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> > >
> > > This feels "tricky", if you tie this to the device you have your driver
> > > bound to, will this make it easier instead of having to go through the
> > > ioctl callback?
> > >
> >
> > GGS(general global storage) registers are in PMU space and linux doesn't have access to it
> > Hence ioctl is used.
>
> Why not just a "real" call to the driver to make this type of reading?
> You don't have ioctls within the kernel for other drivers to call,
> that's not needed at all.
>
Oh yes, this is so broken in design. This firmware is designed to abstract
the power and configuration management on their platform, but they decided
to add direct register access to some registers anyway. Weird use case,
don't even ask. But I strongly objected such interface in sysfs even if
they moved under platform device. If they need this at any cost, I have
suggested debugfs.
[...]
> >
> > Agree it will be simpler but to as firmware driver was changed to be
> > platform driver, to keep paths same, we used sysfs.
>
> Keep them same from what? Use the platform device as that is what it is
> there for, do not go create new apis when they are not needed at all.
>
+1, and please don't add any sysfs that can read/write those GGS registers
directly from userspace. Move them to debugfs if you are desperate to have
something.
--
Regards,
Sudeep
Hi Sudeep,
On 1/24/20, 3:32 AM, "Sudeep Holla" <[email protected]> wrote:
Hi Greg,
Thanks for raising various issues that I have repeatedly asked and
constantly ignored.
Sorry, never intended to ignore you. We agreed to your comments for reg access and that patch is already removed. GGS and PGGS registers are for general purpose registers(set of 4) for users. Since this registers belongs to PMU register space, interface is provided to read or write the way user wants.
On Fri, Jan 24, 2020 at 07:03:39AM +0100, Greg KH wrote:
> On Thu, Jan 23, 2020 at 11:47:57PM +0000, Jolly Shah wrote:
> > Hi Greg,
> >
[...]
> > Firmware driver was changed later to be platform driver but these
> > interfaces were defined earlier and are in use.
>
> Defined and in use where? Not in the upstream kernel tree, right? Or
> am I missing them somewhere else?
>
Exactly and they keep saying there partners are using these for 3-4 years
and they want to retain that. I have told them to change several times.
Sorry we might have missed your comments for this change. We agree to your and greg's comment and will update sysfs path.
> > > > + ret = kstrtol(tok, 16, &value);
> > > > + if (ret) {
> > > > + ret = -EFAULT;
> > > > + goto err;
> > > > + }
> > > > +
> > > > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> > >
> > > This feels "tricky", if you tie this to the device you have your driver
> > > bound to, will this make it easier instead of having to go through the
> > > ioctl callback?
> > >
> >
> > GGS(general global storage) registers are in PMU space and linux doesn't have access to it
> > Hence ioctl is used.
>
> Why not just a "real" call to the driver to make this type of reading?
> You don't have ioctls within the kernel for other drivers to call,
> that's not needed at all.
>
Oh yes, this is so broken in design. This firmware is designed to abstract
the power and configuration management on their platform, but they decided
to add direct register access to some registers anyway. Weird use case,
don't even ask. But I strongly objected such interface in sysfs even if
they moved under platform device. If they need this at any cost, I have
suggested debugfs.
As mentioned, these registers are for users and for special needs where users wants to retain values over resets. but as they belong to PMU address space, these interface APIs are provided. They don’t allow access to any other registers.
[...]
> >
> > Agree it will be simpler but to as firmware driver was changed to be
> > platform driver, to keep paths same, we used sysfs.
>
> Keep them same from what? Use the platform device as that is what it is
> there for, do not go create new apis when they are not needed at all.
>
+1, and please don't add any sysfs that can read/write those GGS registers
directly from userspace. Move them to debugfs if you are desperate to have
something.
Thanks,
Jolly Shah
--
Regards,
Sudeep
Hi Greg,
On 1/23/20, 10:03 PM, "Greg KH" <[email protected]> wrote:
On Thu, Jan 23, 2020 at 11:47:57PM +0000, Jolly Shah wrote:
> Hi Greg,
>
> Thanks for the review.
>
> > -----Original Message-----
> > From: Greg KH <[email protected]>
> > Sent: Tuesday, January 14, 2020 6:53 AM
> > To: Jolly Shah <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; Michal Simek <[email protected]>; Rajan Vaja
> > <[email protected]>; [email protected]; linux-
> > [email protected]; Rajan Vaja <[email protected]>; Jolly Shah
> > <[email protected]>
> > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> >
> > On Wed, Jan 08, 2020 at 03:54:20PM -0800, Jolly Shah wrote:
> > > From: Rajan Vaja <[email protected]>
> > >
> > > Add Firmware-ggs sysfs interface which provides read/write
> > > interface to global storage registers.
> > >
> > > Signed-off-by: Rajan Vaja <[email protected]>
> > > Signed-off-by: Michal Simek <[email protected]>
> > > Signed-off-by: Jolly Shah <[email protected]>
> > > ---
> > > Changes in v2:
> > > - Updated Linux kernel version in documentation.
> > > - Used DEVICE_ATTR_* and ATTRIBUTE_GROUPS macros.
> > > - Free Kobject structure in case of error.
> > > - Resolved smatch errors.
> > > - Updated Signed-off-by sequence.
> > > ---
> > > Documentation/ABI/stable/sysfs-firmware-zynqmp | 50 +++++
> > > drivers/firmware/xilinx/Makefile | 2 +-
> > > drivers/firmware/xilinx/zynqmp-ggs.c | 284
> > +++++++++++++++++++++++++
> > > drivers/firmware/xilinx/zynqmp.c | 32 +++
> > > include/linux/firmware/xlnx-zynqmp.h | 10 +
> > > 5 files changed, 377 insertions(+), 1 deletion(-)
> > > create mode 100644 Documentation/ABI/stable/sysfs-firmware-zynqmp
> > > create mode 100644 drivers/firmware/xilinx/zynqmp-ggs.c
> > >
> > > diff --git a/Documentation/ABI/stable/sysfs-firmware-zynqmp
> > b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> > > new file mode 100644
> > > index 0000000..cffa2fc
> > > --- /dev/null
> > > +++ b/Documentation/ABI/stable/sysfs-firmware-zynqmp
> > > @@ -0,0 +1,50 @@
> > > +What: /sys/firmware/zynqmp/ggs*
> >
> > Why are these attributes just not hanging off of the platform device for
> > the firmware controller? Why do you need a new subdir under "firmware"?
>
> Firmware driver was changed later to be platform driver but these interfaces were defined
> earlier and are in use.
Defined and in use where? Not in the upstream kernel tree, right? Or
am I missing them somewhere else?
Sorry I meant Xilinx kernel users. We will update it as per your suggestion.
> > > + ret = kstrtol(tok, 16, &value);
> > > + if (ret) {
> > > + ret = -EFAULT;
> > > + goto err;
> > > + }
> > > +
> > > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> >
> > This feels "tricky", if you tie this to the device you have your driver
> > bound to, will this make it easier instead of having to go through the
> > ioctl callback?
> >
>
> GGS(general global storage) registers are in PMU space and linux doesn't have access to it
> Hence ioctl is used.
Why not just a "real" call to the driver to make this type of reading?
You don't have ioctls within the kernel for other drivers to call,
that's not needed at all.
these registers are for users and for special needs where users wants to retain values over resets. but as they belong to PMU address space, these interface APIs are provided. They don’t allow access to any other registers.
> > > +int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
> > > +{
> > > + return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);
> >
> > You might be racing userspace here and loosing :(
>
> Prob is called before user space is notified about sysfs so racing shouldn't happen.
"shouldn't"? How do you know this?
Since firmware driver is always built-in (we don't provide support to use as module), user space won't be available before prob is complete. Correct if I am wrong.
> Or you are referring to some other race condition?
Your kobject was created, and notified userspace that it was present and
then sometime after that you add more attributes which userspace has no
idea are there.
If you use the proper device model interfaces, none of these problems
would be there.
Ok we will update it.
>
> >
> > > +}
> > > diff --git a/drivers/firmware/xilinx/zynqmp.c
> > b/drivers/firmware/xilinx/zynqmp.c
> > > index 75bdfaa..4c1117d 100644
> > > --- a/drivers/firmware/xilinx/zynqmp.c
> > > +++ b/drivers/firmware/xilinx/zynqmp.c
> > > @@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
> > > case IOCTL_GET_PLL_FRAC_MODE:
> > > case IOCTL_SET_PLL_FRAC_DATA:
> > > case IOCTL_GET_PLL_FRAC_DATA:
> > > + case IOCTL_WRITE_GGS:
> > > + case IOCTL_READ_GGS:
> > > + case IOCTL_WRITE_PGGS:
> > > + case IOCTL_READ_PGGS:
> >
> > Huh???
>
> Sorry not sure about your concern here. These registers are in PMU space and hence
> Ioctl is needed to let linux access them.
userspace or kernelspace?
You seem to be mixing them both here.
They are in Platform Management Unit register address space so it allows only secure access. Hence for linux to access it, interface APIs are provided.
>
> >
> > > return 1;
> > > default:
> > > return 0;
> > > @@ -704,6 +708,28 @@ const struct zynqmp_eemi_ops
> > *zynqmp_pm_get_eemi_ops(void)
> > > }
> > > EXPORT_SYMBOL_GPL(zynqmp_pm_get_eemi_ops);
> > >
> > > +static int zynqmp_pm_sysfs_init(void)
> > > +{
> > > + struct kobject *zynqmp_kobj;
> > > + int ret;
> > > +
> > > + zynqmp_kobj = kobject_create_and_add("zynqmp", firmware_kobj);
> > > + if (!zynqmp_kobj) {
> > > + pr_err("zynqmp: Firmware kobj add failed.\n");
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + ret = zynqmp_pm_ggs_init(zynqmp_kobj);
> > > + if (ret) {
> > > + kobject_put(zynqmp_kobj);
> > > + pr_err("%s() GGS init fail with error %d\n",
> > > + __func__, ret);
> > > + goto err;
> > > + }
> > > +err:
> > > + return ret;
> > > +}
> > > +
> > > static int zynqmp_firmware_probe(struct platform_device *pdev)
> > > {
> > > struct device *dev = &pdev->dev;
> > > @@ -751,6 +777,12 @@ static int zynqmp_firmware_probe(struct
> > platform_device *pdev)
> > > /* Assign eemi_ops_table */
> > > eemi_ops_tbl = &eemi_ops;
> > >
> > > + ret = zynqmp_pm_sysfs_init();
> >
> > See, you have a platform device, hang the attributes off of that instead
> > of making a kobject and detatching yourself from the global device tree!
> >
> > Please redo this, I think it will make it a lot simpler and more
> > obvious.
>
> Agree it will be simpler but to as firmware driver was changed to be platform driver,
> to keep paths same, we used sysfs.
Keep them same from what? Use the platform device as that is what it is
there for, do not go create new apis when they are not needed at all.
Ok we will update it in next version.
Thanks,
Jolly Shah
thanks,
greg k-h
On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
> > > > + ret = kstrtol(tok, 16, &value);
> > > > + if (ret) {
> > > > + ret = -EFAULT;
> > > > + goto err;
> > > > + }
> > > > +
> > > > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> > >
> > > This feels "tricky", if you tie this to the device you have your driver
> > > bound to, will this make it easier instead of having to go through the
> > > ioctl callback?
> > >
> >
> > GGS(general global storage) registers are in PMU space and linux doesn't have access to it
> > Hence ioctl is used.
>
> Why not just a "real" call to the driver to make this type of reading?
> You don't have ioctls within the kernel for other drivers to call,
> that's not needed at all.
>
> these registers are for users and for special needs where users wants
> to retain values over resets. but as they belong to PMU address space,
> these interface APIs are provided. They don’t allow access to any
> other registers.
That's not the issue here. The issue is you are using an "internal"
ioctl, instead just make a "real" call.
> > > > +int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
> > > > +{
> > > > + return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);
> > >
> > > You might be racing userspace here and loosing :(
> >
> > Prob is called before user space is notified about sysfs so racing shouldn't happen.
>
> "shouldn't"? How do you know this?
>
> Since firmware driver is always built-in (we don't provide support to
> use as module), user space won't be available before prob is complete.
> Correct if I am wrong.
Userspace starts earlier than you think, and also, use the correct
interfaces for this type of thing, that is why it is there. Don't
create purposfully-incorrect code :)
> > > > diff --git a/drivers/firmware/xilinx/zynqmp.c
> > > b/drivers/firmware/xilinx/zynqmp.c
> > > > index 75bdfaa..4c1117d 100644
> > > > --- a/drivers/firmware/xilinx/zynqmp.c
> > > > +++ b/drivers/firmware/xilinx/zynqmp.c
> > > > @@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
> > > > case IOCTL_GET_PLL_FRAC_MODE:
> > > > case IOCTL_SET_PLL_FRAC_DATA:
> > > > case IOCTL_GET_PLL_FRAC_DATA:
> > > > + case IOCTL_WRITE_GGS:
> > > > + case IOCTL_READ_GGS:
> > > > + case IOCTL_WRITE_PGGS:
> > > > + case IOCTL_READ_PGGS:
> > >
> > > Huh???
> >
> > Sorry not sure about your concern here. These registers are in PMU space and hence
> > Ioctl is needed to let linux access them.
>
> userspace or kernelspace?
>
> You seem to be mixing them both here.
>
> They are in Platform Management Unit register address space so it
> allows only secure access. Hence for linux to access it, interface
> APIs are provided.
Again, that's fine, but why are you creating an "internal ioctl"? Just
make a real function call.
thanks,
greg k-h
Hi Greg,
On 1/27/20, 10:28 PM, "[email protected] on behalf of Greg KH" <[email protected] on behalf of [email protected]> wrote:
On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
> > > > + ret = kstrtol(tok, 16, &value);
> > > > + if (ret) {
> > > > + ret = -EFAULT;
> > > > + goto err;
> > > > + }
> > > > +
> > > > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> > >
> > > This feels "tricky", if you tie this to the device you have your driver
> > > bound to, will this make it easier instead of having to go through the
> > > ioctl callback?
> > >
> >
> > GGS(general global storage) registers are in PMU space and linux doesn't have access to it
> > Hence ioctl is used.
>
> Why not just a "real" call to the driver to make this type of reading?
> You don't have ioctls within the kernel for other drivers to call,
> that's not needed at all.
>
> these registers are for users and for special needs where users wants
> to retain values over resets. but as they belong to PMU address space,
> these interface APIs are provided. They don’t allow access to any
> other registers.
That's not the issue here. The issue is you are using an "internal"
ioctl, instead just make a "real" call.
Sorry I am not clear. Do you mean that we should use linux standard ioctl interface instead of internal ioctl by mentioning "real" ?
> > > > +int zynqmp_pm_ggs_init(struct kobject *parent_kobj)
> > > > +{
> > > > + return sysfs_create_group(parent_kobj, zynqmp_ggs_groups[0]);
> > >
> > > You might be racing userspace here and loosing :(
> >
> > Prob is called before user space is notified about sysfs so racing shouldn't happen.
>
> "shouldn't"? How do you know this?
>
> Since firmware driver is always built-in (we don't provide support to
> use as module), user space won't be available before prob is complete.
> Correct if I am wrong.
Userspace starts earlier than you think, and also, use the correct
interfaces for this type of thing, that is why it is there. Don't
create purposfully-incorrect code :)
Sure. We will change it.
Thanks,
Jolly Shah
> > > > diff --git a/drivers/firmware/xilinx/zynqmp.c
> > > b/drivers/firmware/xilinx/zynqmp.c
> > > > index 75bdfaa..4c1117d 100644
> > > > --- a/drivers/firmware/xilinx/zynqmp.c
> > > > +++ b/drivers/firmware/xilinx/zynqmp.c
> > > > @@ -473,6 +473,10 @@ static inline int zynqmp_is_valid_ioctl(u32 ioctl_id)
> > > > case IOCTL_GET_PLL_FRAC_MODE:
> > > > case IOCTL_SET_PLL_FRAC_DATA:
> > > > case IOCTL_GET_PLL_FRAC_DATA:
> > > > + case IOCTL_WRITE_GGS:
> > > > + case IOCTL_READ_GGS:
> > > > + case IOCTL_WRITE_PGGS:
> > > > + case IOCTL_READ_PGGS:
> > >
> > > Huh???
> >
> > Sorry not sure about your concern here. These registers are in PMU space and hence
> > Ioctl is needed to let linux access them.
>
> userspace or kernelspace?
>
> You seem to be mixing them both here.
>
> They are in Platform Management Unit register address space so it
> allows only secure access. Hence for linux to access it, interface
> APIs are provided.
Again, that's fine, but why are you creating an "internal ioctl"? Just
make a real function call.
thanks,
greg k-h
On Thu, Jan 30, 2020 at 11:59:03PM +0000, Jolly Shah wrote:
> Hi Greg,
>
> On 1/27/20, 10:28 PM, "[email protected] on behalf of Greg KH" <[email protected] on behalf of [email protected]> wrote:
>
> On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
> > > > > + ret = kstrtol(tok, 16, &value);
> > > > > + if (ret) {
> > > > > + ret = -EFAULT;
> > > > > + goto err;
> > > > > + }
> > > > > +
> > > > > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> > > >
> > > > This feels "tricky", if you tie this to the device you have your driver
> > > > bound to, will this make it easier instead of having to go through the
> > > > ioctl callback?
> > > >
> > >
> > > GGS(general global storage) registers are in PMU space and linux doesn't have access to it
> > > Hence ioctl is used.
> >
> > Why not just a "real" call to the driver to make this type of reading?
> > You don't have ioctls within the kernel for other drivers to call,
> > that's not needed at all.
> >
> > these registers are for users and for special needs where users wants
> > to retain values over resets. but as they belong to PMU address space,
> > these interface APIs are provided. They don’t allow access to any
> > other registers.
>
> That's not the issue here. The issue is you are using an "internal"
> ioctl, instead just make a "real" call.
>
> Sorry I am not clear. Do you mean that we should use linux standard
> ioctl interface instead of internal ioctl by mentioning "real" ?
No, you should just make a "real" function call to the exact thing you
want to do. Not have an internal multi-plexor ioctl() call that others
then call. This isn't a microkernel :)
thanks,
greg k-h
Hi Greg,
> -----Original Message-----
> From: Greg KH <[email protected]>
> Sent: 31 January 2020 11:41 AM
> To: Jolly Shah <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Michal Simek <[email protected]>; Rajan Vaja
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
>
> EXTERNAL EMAIL
>
> On Thu, Jan 30, 2020 at 11:59:03PM +0000, Jolly Shah wrote:
> > Hi Greg,
> >
> > On 1/27/20, 10:28 PM, "[email protected] on behalf of Greg
> KH" <[email protected] on behalf of
> [email protected]> wrote:
> >
> > On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
> > > > > > + ret = kstrtol(tok, 16, &value);
> > > > > > + if (ret) {
> > > > > > + ret = -EFAULT;
> > > > > > + goto err;
> > > > > > + }
> > > > > > +
> > > > > > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> > > > >
> > > > > This feels "tricky", if you tie this to the device you have your driver
> > > > > bound to, will this make it easier instead of having to go through the
> > > > > ioctl callback?
> > > > >
> > > >
> > > > GGS(general global storage) registers are in PMU space and linux
> doesn't have access to it
> > > > Hence ioctl is used.
> > >
> > > Why not just a "real" call to the driver to make this type of reading?
> > > You don't have ioctls within the kernel for other drivers to call,
> > > that's not needed at all.
> > >
> > > these registers are for users and for special needs where users wants
> > > to retain values over resets. but as they belong to PMU address space,
> > > these interface APIs are provided. They don’t allow access to any
> > > other registers.
> >
> > That's not the issue here. The issue is you are using an "internal"
> > ioctl, instead just make a "real" call.
> >
> > Sorry I am not clear. Do you mean that we should use linux standard
> > ioctl interface instead of internal ioctl by mentioning "real" ?
>
> No, you should just make a "real" function call to the exact thing you
> want to do. Not have an internal multi-plexor ioctl() call that others
> then call. This isn't a microkernel :)
[Rajan] Sorry for multiple back and forth but as I understand, you are suggesting to create a new API for
Read/write of GGS register instead of using PM_IOCTL API (eemi_ops->ioctl) for multiple purpose. Is my understanding correct?
Thanks,
Rajan
>
> thanks,
>
> greg k-h
On Fri, Jan 31, 2020 at 09:05:15AM +0000, Rajan Vaja wrote:
> Hi Greg,
>
> > -----Original Message-----
> > From: Greg KH <[email protected]>
> > Sent: 31 January 2020 11:41 AM
> > To: Jolly Shah <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; Michal Simek <[email protected]>; Rajan Vaja
> > <[email protected]>; [email protected]; linux-
> > [email protected]
> > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> >
> > EXTERNAL EMAIL
> >
> > On Thu, Jan 30, 2020 at 11:59:03PM +0000, Jolly Shah wrote:
> > > Hi Greg,
> > >
> > > On 1/27/20, 10:28 PM, "[email protected] on behalf of Greg
> > KH" <[email protected] on behalf of
> > [email protected]> wrote:
> > >
> > > On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
> > > > > > > + ret = kstrtol(tok, 16, &value);
> > > > > > > + if (ret) {
> > > > > > > + ret = -EFAULT;
> > > > > > > + goto err;
> > > > > > > + }
> > > > > > > +
> > > > > > > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> > > > > >
> > > > > > This feels "tricky", if you tie this to the device you have your driver
> > > > > > bound to, will this make it easier instead of having to go through the
> > > > > > ioctl callback?
> > > > > >
> > > > >
> > > > > GGS(general global storage) registers are in PMU space and linux
> > doesn't have access to it
> > > > > Hence ioctl is used.
> > > >
> > > > Why not just a "real" call to the driver to make this type of reading?
> > > > You don't have ioctls within the kernel for other drivers to call,
> > > > that's not needed at all.
> > > >
> > > > these registers are for users and for special needs where users wants
> > > > to retain values over resets. but as they belong to PMU address space,
> > > > these interface APIs are provided. They don’t allow access to any
> > > > other registers.
> > >
> > > That's not the issue here. The issue is you are using an "internal"
> > > ioctl, instead just make a "real" call.
> > >
> > > Sorry I am not clear. Do you mean that we should use linux standard
> > > ioctl interface instead of internal ioctl by mentioning "real" ?
> >
> > No, you should just make a "real" function call to the exact thing you
> > want to do. Not have an internal multi-plexor ioctl() call that others
> > then call. This isn't a microkernel :)
> [Rajan] Sorry for multiple back and forth but as I understand, you are suggesting to create a new API for
> Read/write of GGS register instead of using PM_IOCTL API (eemi_ops->ioctl) for multiple purpose. Is my understanding correct?
That is correct.
Hi Greg,
> ------Original Message------
> From: 'Greg Kh' <[email protected]>
> Sent: Friday, January 31, 2020 1:36AM
> To: Rajan Vaja <[email protected]>
> Cc: Jolly Shah <[email protected]>, Ard Biesheuvel
<[email protected]>, Mingo <[email protected]>, Matt
<[email protected]>, Sudeep Holla <[email protected]>,
Hkallweit1 <[email protected]>, Keescook <[email protected]>,
Dmitry Torokhov <[email protected]>, Michal Simek
<[email protected]>, Linux-arm-kernel
<[email protected]>, Linux-kernel
<[email protected]>
> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
>
> On Fri, Jan 31, 2020 at 09:05:15AM +0000, Rajan Vaja wrote:
>> Hi Greg,
>>
>>> -----Original Message-----
>>> From: Greg KH <[email protected]>
>>> Sent: 31 January 2020 11:41 AM
>>> To: Jolly Shah <[email protected]>
>>> Cc: [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; Michal Simek <[email protected]>; Rajan Vaja
>>> <[email protected]>; [email protected]; linux-
>>> [email protected]
>>> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
>>>
>>> EXTERNAL EMAIL
>>>
>>> On Thu, Jan 30, 2020 at 11:59:03PM +0000, Jolly Shah wrote:
>>>> Hi Greg,
>>>>
>>>> On 1/27/20, 10:28 PM, "[email protected] on behalf of Greg
>>> KH" <[email protected] on behalf of
>>> [email protected]> wrote:
>>>>
>>>> On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
>>>> > > > > + ret = kstrtol(tok, 16, &value);
>>>> > > > > + if (ret) {
>>>> > > > > + ret = -EFAULT;
>>>> > > > > + goto err;
>>>> > > > > + }
>>>> > > > > +
>>>> > > > > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
>>>> > > >
>>>> > > > This feels "tricky", if you tie this to the device you have your driver
>>>> > > > bound to, will this make it easier instead of having to go through the
>>>> > > > ioctl callback?
>>>> > > >
>>>> > >
>>>> > > GGS(general global storage) registers are in PMU space and linux
>>> doesn't have access to it
>>>> > > Hence ioctl is used.
>>>> >
>>>> > Why not just a "real" call to the driver to make this type of reading?
>>>> > You don't have ioctls within the kernel for other drivers to call,
>>>> > that's not needed at all.
>>>> >
>>>> > these registers are for users and for special needs where users wants
>>>> > to retain values over resets. but as they belong to PMU address space,
>>>> > these interface APIs are provided. They don’t allow access to any
>>>> > other registers.
>>>>
>>>> That's not the issue here. The issue is you are using an "internal"
>>>> ioctl, instead just make a "real" call.
>>>>
>>>> Sorry I am not clear. Do you mean that we should use linux standard
>>>> ioctl interface instead of internal ioctl by mentioning "real" ?
>>>
>>> No, you should just make a "real" function call to the exact thing you
>>> want to do. Not have an internal multi-plexor ioctl() call that others
>>> then call. This isn't a microkernel :)
>> [Rajan] Sorry for multiple back and forth but as I understand, you are suggesting to create a new API for
>> Read/write of GGS register instead of using PM_IOCTL API (eemi_ops->ioctl) for multiple purpose. Is my understanding correct?
>
> That is correct.
Would like to clarify purpose of having ioctl API to avoid any confusion.
eemi interface apis are defined to be platform independent and allows
clock, reset, power etc management through firmware but apart from these
generic operations, there are some operations which needs secure access
through firmware. Examples are accessing some storage registers(ggs and
pggs) for inter agent communication, configuring another agent(RPU)
mode, boot device configuration etc. Those operations are covered as
ioctls as they are very platform specific. Also only whitelisted
operations are allowed through ioctl and is not exposed to user for any
random read/write operation.
Olof earlier had same concerns. We had clarified the purpose and with
his agreement, initial set of ioctls were accepted.
(https://www.lkml.org/lkml/2018/9/24/1570)
Please suggest the best approach to handle this for current and future
patches.
Thanks,
Jolly Shah
>
On Mon, Feb 10, 2020 at 04:57:01PM -0800, Jolly Shah wrote:
> Hi Greg,
>
> > ------Original Message------
> > From: 'Greg Kh' <[email protected]>
> > Sent: Friday, January 31, 2020 1:36AM
> > To: Rajan Vaja <[email protected]>
> > Cc: Jolly Shah <[email protected]>, Ard Biesheuvel
> <[email protected]>, Mingo <[email protected]>, Matt
> <[email protected]>, Sudeep Holla <[email protected]>, Hkallweit1
> <[email protected]>, Keescook <[email protected]>, Dmitry Torokhov
> <[email protected]>, Michal Simek <[email protected]>,
> Linux-arm-kernel <[email protected]>, Linux-kernel
> <[email protected]>
> > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> >
> > On Fri, Jan 31, 2020 at 09:05:15AM +0000, Rajan Vaja wrote:
> > > Hi Greg,
> > >
> > > > -----Original Message-----
> > > > From: Greg KH <[email protected]>
> > > > Sent: 31 January 2020 11:41 AM
> > > > To: Jolly Shah <[email protected]>
> > > > Cc: [email protected]; [email protected]; [email protected];
> > > > [email protected]; [email protected]; [email protected];
> > > > [email protected]; Michal Simek <[email protected]>; Rajan Vaja
> > > > <[email protected]>; [email protected]; linux-
> > > > [email protected]
> > > > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> > > >
> > > > EXTERNAL EMAIL
> > > >
> > > > On Thu, Jan 30, 2020 at 11:59:03PM +0000, Jolly Shah wrote:
> > > > > Hi Greg,
> > > > >
> > > > > On 1/27/20, 10:28 PM, "[email protected] on behalf of Greg
> > > > KH" <[email protected] on behalf of
> > > > [email protected]> wrote:
> > > > >
> > > > > On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
> > > > > > > > > + ret = kstrtol(tok, 16, &value);
> > > > > > > > > + if (ret) {
> > > > > > > > > + ret = -EFAULT;
> > > > > > > > > + goto err;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
> > > > > > > >
> > > > > > > > This feels "tricky", if you tie this to the device you have your driver
> > > > > > > > bound to, will this make it easier instead of having to go through the
> > > > > > > > ioctl callback?
> > > > > > > >
> > > > > > >
> > > > > > > GGS(general global storage) registers are in PMU space and linux
> > > > doesn't have access to it
> > > > > > > Hence ioctl is used.
> > > > > >
> > > > > > Why not just a "real" call to the driver to make this type of reading?
> > > > > > You don't have ioctls within the kernel for other drivers to call,
> > > > > > that's not needed at all.
> > > > > >
> > > > > > these registers are for users and for special needs where users wants
> > > > > > to retain values over resets. but as they belong to PMU address space,
> > > > > > these interface APIs are provided. They don’t allow access to any
> > > > > > other registers.
> > > > >
> > > > > That's not the issue here. The issue is you are using an "internal"
> > > > > ioctl, instead just make a "real" call.
> > > > >
> > > > > Sorry I am not clear. Do you mean that we should use linux standard
> > > > > ioctl interface instead of internal ioctl by mentioning "real" ?
> > > >
> > > > No, you should just make a "real" function call to the exact thing you
> > > > want to do. Not have an internal multi-plexor ioctl() call that others
> > > > then call. This isn't a microkernel :)
> > > [Rajan] Sorry for multiple back and forth but as I understand, you are suggesting to create a new API for
> > > Read/write of GGS register instead of using PM_IOCTL API (eemi_ops->ioctl) for multiple purpose. Is my understanding correct?
> >
> > That is correct.
>
>
>
> Would like to clarify purpose of having ioctl API to avoid any confusion.
> eemi interface apis are defined to be platform independent and allows clock,
> reset, power etc management through firmware but apart from these generic
> operations, there are some operations which needs secure access through
> firmware. Examples are accessing some storage registers(ggs and pggs) for
> inter agent communication, configuring another agent(RPU) mode, boot device
> configuration etc. Those operations are covered as ioctls as they are very
> platform specific. Also only whitelisted operations are allowed through
> ioctl and is not exposed to user for any random read/write operation.
>
> Olof earlier had same concerns. We had clarified the purpose and with his
> agreement, initial set of ioctls were accepted.
> (https://www.lkml.org/lkml/2018/9/24/1570)
>
> Please suggest the best approach to handle this for current and future
> patches.
Ok, in looking further at this, it's both better than I thought, and
totally worse.
This interface you all are using where you ask the firmware driver for a
pointer to a set of operation functions and then make calls through that
is indicitive of an api that is NOT what we normally use in Linux at
all.
Just make the direct call to the firmware driver, no need to muck around
with tables of function pointers. In fact, with the spectre changes,
you just made things slower than needed, and you can get back a bunch of
throughput by removing that whole middle layer.
So go do that first please, before adding any new stuff.
Now for the ioctl, yeah, that's not a "normal" pattern either. But
right now you only have 2 "different" ioctls that you call. So why not
just turn those 2 into real function calls as well that then makes the
"ioctl" call to the hardware? That makes things a lot more obvious on
the kernel driver side exactly what is going on.
If you need to add more "ioctl" like calls, just add them as more
functions, no big deal. How many more of these are you going to need
over time?
But that's not all that big of a deal right now, get rid of that whole
middle-layer first, that's more important to clean up. You will get rid
of a lot of unneeded code and indirection that way, making it simpler
and easier to understand what exactly is happening.
thanks,
greg k-h
Hi Greg,
Thanks for the response.
> ------Original Message------
> From: 'Greg Kh' <[email protected]>
> Sent: Friday, February 14, 2020 9:10AM
> To: Jolly Shah <[email protected]>
> Cc: Rajan Vaja <[email protected]>, [email protected]
<[email protected]>, [email protected] <[email protected]>,
[email protected] <[email protected]>,
[email protected] <[email protected]>, [email protected]
<[email protected]>, [email protected] <[email protected]>,
[email protected] <[email protected]>, Michal Simek
<[email protected]>, [email protected]
<[email protected]>, [email protected]
<[email protected]>
> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
>
> On Mon, Feb 10, 2020 at 04:57:01PM -0800, Jolly Shah wrote:
>> Hi Greg,
>>
>>> ------Original Message------
>>> From: 'Greg Kh' <[email protected]>
>>> Sent: Friday, January 31, 2020 1:36AM
>>> To: Rajan Vaja <[email protected]>
>>> Cc: Jolly Shah <[email protected]>, Ard Biesheuvel
>> <[email protected]>, Mingo <[email protected]>, Matt
>> <[email protected]>, Sudeep Holla <[email protected]>, Hkallweit1
>> <[email protected]>, Keescook <[email protected]>, Dmitry Torokhov
>> <[email protected]>, Michal Simek <[email protected]>,
>> Linux-arm-kernel <[email protected]>, Linux-kernel
>> <[email protected]>
>>> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
>>>
>>> On Fri, Jan 31, 2020 at 09:05:15AM +0000, Rajan Vaja wrote:
>>>> Hi Greg,
>>>>
>>>>> -----Original Message-----
>>>>> From: Greg KH <[email protected]>
>>>>> Sent: 31 January 2020 11:41 AM
>>>>> To: Jolly Shah <[email protected]>
>>>>> Cc: [email protected]; [email protected]; [email protected];
>>>>> [email protected]; [email protected]; [email protected];
>>>>> [email protected]; Michal Simek <[email protected]>; Rajan Vaja
>>>>> <[email protected]>; [email protected]; linux-
>>>>> [email protected]
>>>>> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
>>>>>
>>>>> EXTERNAL EMAIL
>>>>>
>>>>> On Thu, Jan 30, 2020 at 11:59:03PM +0000, Jolly Shah wrote:
>>>>>> Hi Greg,
>>>>>>
>>>>>> On 1/27/20, 10:28 PM, "[email protected] on behalf of Greg
>>>>> KH" <[email protected] on behalf of
>>>>> [email protected]> wrote:
>>>>>>
>>>>>> On Mon, Jan 27, 2020 at 11:01:27PM +0000, Jolly Shah wrote:
>>>>>> > > > > + ret = kstrtol(tok, 16, &value);
>>>>>> > > > > + if (ret) {
>>>>>> > > > > + ret = -EFAULT;
>>>>>> > > > > + goto err;
>>>>>> > > > > + }
>>>>>> > > > > +
>>>>>> > > > > + ret = eemi_ops->ioctl(0, read_ioctl, reg, 0, ret_payload);
>>>>>> > > >
>>>>>> > > > This feels "tricky", if you tie this to the device you have your driver
>>>>>> > > > bound to, will this make it easier instead of having to go through the
>>>>>> > > > ioctl callback?
>>>>>> > > >
>>>>>> > >
>>>>>> > > GGS(general global storage) registers are in PMU space and linux
>>>>> doesn't have access to it
>>>>>> > > Hence ioctl is used.
>>>>>> >
>>>>>> > Why not just a "real" call to the driver to make this type of reading?
>>>>>> > You don't have ioctls within the kernel for other drivers to call,
>>>>>> > that's not needed at all.
>>>>>> >
>>>>>> > these registers are for users and for special needs where users wants
>>>>>> > to retain values over resets. but as they belong to PMU address space,
>>>>>> > these interface APIs are provided. They don’t allow access to any
>>>>>> > other registers.
>>>>>>
>>>>>> That's not the issue here. The issue is you are using an "internal"
>>>>>> ioctl, instead just make a "real" call.
>>>>>>
>>>>>> Sorry I am not clear. Do you mean that we should use linux standard
>>>>>> ioctl interface instead of internal ioctl by mentioning "real" ?
>>>>>
>>>>> No, you should just make a "real" function call to the exact thing you
>>>>> want to do. Not have an internal multi-plexor ioctl() call that others
>>>>> then call. This isn't a microkernel :)
>>>> [Rajan] Sorry for multiple back and forth but as I understand, you are suggesting to create a new API for
>>>> Read/write of GGS register instead of using PM_IOCTL API (eemi_ops->ioctl) for multiple purpose. Is my understanding correct?
>>>
>>> That is correct.
>>
>>
>>
>> Would like to clarify purpose of having ioctl API to avoid any confusion.
>> eemi interface apis are defined to be platform independent and allows clock,
>> reset, power etc management through firmware but apart from these generic
>> operations, there are some operations which needs secure access through
>> firmware. Examples are accessing some storage registers(ggs and pggs) for
>> inter agent communication, configuring another agent(RPU) mode, boot device
>> configuration etc. Those operations are covered as ioctls as they are very
>> platform specific. Also only whitelisted operations are allowed through
>> ioctl and is not exposed to user for any random read/write operation.
>>
>> Olof earlier had same concerns. We had clarified the purpose and with his
>> agreement, initial set of ioctls were accepted.
>> (https://www.lkml.org/lkml/2018/9/24/1570)
>>
>> Please suggest the best approach to handle this for current and future
>> patches.
>
> Ok, in looking further at this, it's both better than I thought, and
> totally worse.
>
> This interface you all are using where you ask the firmware driver for a
> pointer to a set of operation functions and then make calls through that
> is indicitive of an api that is NOT what we normally use in Linux at
> all.
>
> Just make the direct call to the firmware driver, no need to muck around
> with tables of function pointers. In fact, with the spectre changes,
> you just made things slower than needed, and you can get back a bunch of
> throughput by removing that whole middle layer.
>
arm,scpi is doing the same way and we thought this approach will be more
acceptable than direct function calls but happy to change as suggested.
> So go do that first please, before adding any new stuff.
>
> Now for the ioctl, yeah, that's not a "normal" pattern either. But
> right now you only have 2 "different" ioctls that you call. So why not
> just turn those 2 into real function calls as well that then makes the
> "ioctl" call to the hardware? That makes things a lot more obvious on
> the kernel driver side exactly what is going on.
>
Sure as i understand firmware driver will provide real function calls to
be used by user drivers and underneath it will call ioctl for desired
operation. Please correct if I misunderstood.
Thanks,
Jolly Shah
> If you need to add more "ioctl" like calls, just add them as more
> functions, no big deal. How many more of these are you going to need
> over time?
>
> But that's not all that big of a deal right now, get rid of that whole
> middle-layer first, that's more important to clean up. You will get rid
> of a lot of unneeded code and indirection that way, making it simpler
> and easier to understand what exactly is happening.
>
> thanks,
>
> greg k-h
>
On Fri, Feb 14, 2020 at 04:37:16PM -0800, Jolly Shah wrote:
> > Just make the direct call to the firmware driver, no need to muck around
> > with tables of function pointers. In fact, with the spectre changes,
> > you just made things slower than needed, and you can get back a bunch of
> > throughput by removing that whole middle layer.
> >
>
> arm,scpi is doing the same way and we thought this approach will be more
> acceptable than direct function calls but happy to change as suggested.
Just because one random tiny thing does it the wrong way does not mean
to focus on that design pattern and ignore the thousands of other
apis/interfaces in the kernel that do not do it that way :)
> > So go do that first please, before adding any new stuff.
> >
> > Now for the ioctl, yeah, that's not a "normal" pattern either. But
> > right now you only have 2 "different" ioctls that you call. So why not
> > just turn those 2 into real function calls as well that then makes the
> > "ioctl" call to the hardware? That makes things a lot more obvious on
> > the kernel driver side exactly what is going on.
> >
>
> Sure as i understand firmware driver will provide real function calls to be
> used by user drivers and underneath it will call ioctl for desired
> operation. Please correct if I misunderstood.
You do not misunderstand.
thanks,
greg k-h
Hi Greg,
On 2/14/20, 4:58 PM, "[email protected] on behalf of 'Greg KH'" <[email protected] on behalf of [email protected]> wrote:
On Fri, Feb 14, 2020 at 04:37:16PM -0800, Jolly Shah wrote:
> > Just make the direct call to the firmware driver, no need to muck around
> > with tables of function pointers. In fact, with the spectre changes,
> > you just made things slower than needed, and you can get back a bunch of
> > throughput by removing that whole middle layer.
> >
>
> arm,scpi is doing the same way and we thought this approach will be more
> acceptable than direct function calls but happy to change as suggested.
Just because one random tiny thing does it the wrong way does not mean
to focus on that design pattern and ignore the thousands of other
apis/interfaces in the kernel that do not do it that way :)
Sure. Will change in next version.
> > So go do that first please, before adding any new stuff.
> >
> > Now for the ioctl, yeah, that's not a "normal" pattern either. But
> > right now you only have 2 "different" ioctls that you call. So why not
> > just turn those 2 into real function calls as well that then makes the
> > "ioctl" call to the hardware? That makes things a lot more obvious on
> > the kernel driver side exactly what is going on.
> >
>
> Sure as i understand firmware driver will provide real function calls to be
> used by user drivers and underneath it will call ioctl for desired
> operation. Please correct if I misunderstood.
You do not misunderstand.
Thanks for confirmation.
Thanks,
Jolly Shah
thanks,
greg k-h
Hi Greg,
> ------Original Message------
> From: 'Greg Kh' <[email protected]>
> Sent: Friday, February 14, 2020 4:52PM
> To: Jolly Shah <[email protected]>
> Cc: Rajan Vaja <[email protected]>, [email protected]
<[email protected]>, [email protected] <[email protected]>,
[email protected] <[email protected]>,
[email protected] <[email protected]>, [email protected]
<[email protected]>, [email protected] <[email protected]>,
[email protected] <[email protected]>, Michal Simek
<[email protected]>, [email protected]
<[email protected]>, [email protected]
<[email protected]>
> Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
>
> On Fri, Feb 14, 2020 at 04:37:16PM -0800, Jolly Shah wrote:
>>> Just make the direct call to the firmware driver, no need to muck around
>>> with tables of function pointers. In fact, with the spectre changes,
>>> you just made things slower than needed, and you can get back a bunch of
>>> throughput by removing that whole middle layer.
>>>
>>
>> arm,scpi is doing the same way and we thought this approach will be more
>> acceptable than direct function calls but happy to change as suggested.
>
> Just because one random tiny thing does it the wrong way does not mean
> to focus on that design pattern and ignore the thousands of other
> apis/interfaces in the kernel that do not do it that way :)
>
>>> So go do that first please, before adding any new stuff.
>>>
>>> Now for the ioctl, yeah, that's not a "normal" pattern either. But
>>> right now you only have 2 "different" ioctls that you call. So why not
>>> just turn those 2 into real function calls as well that then makes the
>>> "ioctl" call to the hardware? That makes things a lot more obvious on
>>> the kernel driver side exactly what is going on.
>>>
>>
>> Sure as i understand firmware driver will provide real function calls to be
>> used by user drivers and underneath it will call ioctl for desired
>> operation. Please correct if I misunderstood.
>
> You do not misunderstand.
Submitted v3 with required changes. Please review.
Thanks,
Jolly Shah
>
> thanks,
>
> greg k-h
>
On Fri, Mar 06, 2020 at 03:55:46PM -0800, Jolly Shah wrote:
> Hi Greg,
>
> > ------Original Message------
> > From: 'Greg Kh' <[email protected]>
> > Sent: Friday, February 14, 2020 4:52PM
> > To: Jolly Shah <[email protected]>
> > Cc: Rajan Vaja <[email protected]>, [email protected]
> <[email protected]>, [email protected] <[email protected]>,
> [email protected] <[email protected]>, [email protected]
> <[email protected]>, [email protected] <[email protected]>,
> [email protected] <[email protected]>, [email protected]
> <[email protected]>, Michal Simek <[email protected]>,
> [email protected] <[email protected]>,
> [email protected] <[email protected]>
> > Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
> >
> > On Fri, Feb 14, 2020 at 04:37:16PM -0800, Jolly Shah wrote:
> > > > Just make the direct call to the firmware driver, no need to muck around
> > > > with tables of function pointers. In fact, with the spectre changes,
> > > > you just made things slower than needed, and you can get back a bunch of
> > > > throughput by removing that whole middle layer.
> > > >
> > >
> > > arm,scpi is doing the same way and we thought this approach will be more
> > > acceptable than direct function calls but happy to change as suggested.
> >
> > Just because one random tiny thing does it the wrong way does not mean
> > to focus on that design pattern and ignore the thousands of other
> > apis/interfaces in the kernel that do not do it that way :)
> >
> > > > So go do that first please, before adding any new stuff.
> > > >
> > > > Now for the ioctl, yeah, that's not a "normal" pattern either. But
> > > > right now you only have 2 "different" ioctls that you call. So why not
> > > > just turn those 2 into real function calls as well that then makes the
> > > > "ioctl" call to the hardware? That makes things a lot more obvious on
> > > > the kernel driver side exactly what is going on.
> > > >
> > >
> > > Sure as i understand firmware driver will provide real function calls to be
> > > used by user drivers and underneath it will call ioctl for desired
> > > operation. Please correct if I misunderstood.
> >
> > You do not misunderstand.
>
> Submitted v3 with required changes. Please review.
Will do, when I get to it, relax :)