From: Greg KH <gregkh@linuxfoundation.org>
To: Jolly Shah <jolly.shah@xilinx.com>
Cc: ard.biesheuvel@linaro.org, mingo@kernel.org,
matt@codeblueprint.co.uk, sudeep.holla@arm.com,
hkallweit1@gmail.com, keescook@chromium.org,
dmitry.torokhov@gmail.com, michal.simek@xilinx.com,
rajanv@xilinx.com, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Rajan Vaja <rajan.vaja@xilinx.com>,
Jolly Shah <jollys@xilinx.com>
Subject: Re: [PATCH v2 1/4] firmware: xilinx: Add sysfs interface
Date: Tue, 14 Jan 2020 15:52:57 +0100 [thread overview]
Message-ID: <20200114145257.GA1910108@kroah.com> (raw)
In-Reply-To: <1578527663-10243-2-git-send-email-jolly.shah@xilinx.com>
On Wed, Jan 08, 2020 at 03:54:20PM -0800, Jolly Shah wrote:
> From: Rajan Vaja <rajan.vaja@xilinx.com>
>
> Add Firmware-ggs sysfs interface which provides read/write
> interface to global storage registers.
>
> Signed-off-by: Rajan Vaja <rajan.vaja@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> Signed-off-by: Jolly Shah <jollys@xilinx.com>
> ---
> 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" <jollys@xilinx.com>
> +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" <jollys@xilinx.com>
> +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 <jollys@xilinx.com>
> + * Rajan Vaja <rajanv@xilinx.com>
> + */
> +
> +#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
next prev parent reply other threads:[~2020-01-14 14:53 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-08 23:54 [PATCH v2 0/4] firmware: xilinx: Add xilinx specific sysfs interface Jolly Shah
2020-01-08 23:54 ` [PATCH v2 1/4] firmware: xilinx: Add " Jolly Shah
2020-01-14 14:52 ` Greg KH [this message]
2020-01-23 23:47 ` Jolly Shah
2020-01-24 6:03 ` Greg KH
2020-01-24 11:32 ` Sudeep Holla
2020-01-27 22:46 ` Jolly Shah
2020-01-27 23:01 ` Jolly Shah
2020-01-28 6:28 ` Greg KH
2020-01-30 23:59 ` Jolly Shah
2020-01-31 6:10 ` Greg KH
2020-01-31 9:05 ` Rajan Vaja
2020-01-31 9:36 ` 'Greg KH'
2020-02-11 0:57 ` Jolly Shah
2020-02-14 17:10 ` 'Greg KH'
2020-02-15 0:37 ` Jolly Shah
2020-02-15 0:52 ` 'Greg KH'
2020-02-19 22:50 ` Jolly Shah
2020-03-06 23:55 ` Jolly Shah
2020-03-07 8:47 ` 'Greg KH'
2020-01-08 23:54 ` [PATCH v2 2/4] firmware: xilinx: Add system shutdown API interface Jolly Shah
2020-01-08 23:54 ` [PATCH v2 3/4] firmware: xilinx: Add sysfs to set shutdown scope Jolly Shah
2020-01-08 23:54 ` [PATCH v2 4/4] firmware: xilinx: Add sysfs and IOCTL to set boot health status Jolly Shah
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200114145257.GA1910108@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=ard.biesheuvel@linaro.org \
--cc=dmitry.torokhov@gmail.com \
--cc=hkallweit1@gmail.com \
--cc=jolly.shah@xilinx.com \
--cc=jollys@xilinx.com \
--cc=keescook@chromium.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=matt@codeblueprint.co.uk \
--cc=michal.simek@xilinx.com \
--cc=mingo@kernel.org \
--cc=rajan.vaja@xilinx.com \
--cc=rajanv@xilinx.com \
--cc=sudeep.holla@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).