| Message ID | 1435355419-23602-7-git-send-email-bjorn.andersson@sonymobile.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 26 Jun 2015, bjorn@kryo.se wrote: > From: Bjorn Andersson <bjorn.andersson@sonymobile.com> > > Driver for the Resource Power Manager (RPM) found in Qualcomm 8974 based > devices. > The driver exposes resources that child drivers can operate on; to > implementing regulator, clock and bus frequency drivers. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> > --- > drivers/mfd/Kconfig | 14 +++ > drivers/mfd/Makefile | 1 + > drivers/mfd/qcom-smd-rpm.c | 236 +++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/qcom-smd-rpm.h | 35 ++++++ > 4 files changed, 286 insertions(+) > create mode 100644 drivers/mfd/qcom-smd-rpm.c > create mode 100644 include/linux/mfd/qcom-smd-rpm.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 6538159..666c812 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -656,6 +656,20 @@ config MFD_QCOM_RPM > Say M here if you want to include support for the Qualcomm RPM as a > module. This will build a module called "qcom_rpm". > > +config MFD_QCOM_SMD_RPM > + tristate "Qualcomm Resource Power Manager (RPM) over SMD" > + depends on QCOM_SMD && OF > + help > + If you say yes to this option, support will be included for the > + Resource Power Manager system found in the Qualcomm 8974 based > + devices. > + > + This is required to access many regulators, clocks and bus > + frequencies controlled by the RPM on these devices. > + > + Say M here if you want to include support for the Qualcomm RPM as a > + module. This will build a module called "qcom-smd-rpm". I'm not exactly sure what makes this an MFD device. > config MFD_SPMI_PMIC > tristate "Qualcomm SPMI PMICs" > depends on ARCH_QCOM || COMPILE_TEST > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index ea40e07..289bd24 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -156,6 +156,7 @@ obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o > obj-$(CONFIG_MFD_OMAP_USB_HOST) += omap-usb-host.o omap-usb-tll.o > obj-$(CONFIG_MFD_PM8921_CORE) += pm8921-core.o ssbi.o > obj-$(CONFIG_MFD_QCOM_RPM) += qcom_rpm.o > +obj-$(CONFIG_MFD_QCOM_SMD_RPM) += qcom-smd-rpm.o > obj-$(CONFIG_MFD_SPMI_PMIC) += qcom-spmi-pmic.o > obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o > obj-$(CONFIG_MFD_TPS65090) += tps65090.o > diff --git a/drivers/mfd/qcom-smd-rpm.c b/drivers/mfd/qcom-smd-rpm.c > new file mode 100644 > index 0000000..d6faf85 > --- /dev/null > +++ b/drivers/mfd/qcom-smd-rpm.c > @@ -0,0 +1,236 @@ > +/* > + * Copyright (c) 2015, Sony Mobile Communications AB. > + * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/of_platform.h> > +#include <linux/io.h> > +#include <linux/interrupt.h> > + > +#include <linux/soc/qcom/smd.h> > +#include <linux/mfd/qcom-smd-rpm.h> > + > +#include <dt-bindings/mfd/qcom-smd-rpm.h> > + > +#define RPM_REQUEST_TIMEOUT (5 * HZ) > + > +/** > + * struct qcom_smd_rpm - state of the rpm device driver > + * @rpm_channel: reference to the smd channel > + * @ack: completion for acks > + * @lock: mutual exclusion around the send/complete pair > + * @ack_status: result of the rpm request > + */ > +struct qcom_smd_rpm { > + struct qcom_smd_channel *rpm_channel; > + > + struct completion ack; > + struct mutex lock; > + int ack_status; > +}; > + > +/** > + * struct qcom_rpm_header - header for all rpm requests and responses > + * @service_type: identifier of the service > + * @length: length of the payload > + */ > +struct qcom_rpm_header { > + u32 service_type; > + u32 length; > +}; > + > +/** > + * struct qcom_rpm_request - request message to the rpm > + * @msg_id: identifier of the outgoing message > + * @flags: active/sleep state flags > + * @type: resource type > + * @id: resource id > + * @data_len: length of the payload following this header > + */ > +struct qcom_rpm_request { > + u32 msg_id; > + u32 flags; > + u32 type; > + u32 id; > + u32 data_len; > +}; > + > +/** > + * struct qcom_rpm_message - response message from the rpm > + * @msg_type: indicator of the type of message > + * @length: the size of this message, including the message header > + * @msg_id: message id > + * @message: textual message from the rpm > + * > + * Multiple of these messages can be stacked in an rpm message. > + */ > +struct qcom_rpm_message { > + u32 msg_type; > + u32 length; > + union { > + u32 msg_id; > + u8 message[0]; > + }; > +}; > + > +#define RPM_SERVICE_TYPE_REQUEST 0x00716572 /* "req\0" */ > + > +#define RPM_MSG_TYPE_ERR 0x00727265 /* "err\0" */ > +#define RPM_MSG_TYPE_MSG_ID 0x2367736d /* "msg#" */ > + > +/** > + * qcom_rpm_smd_write - write @buf to @type:@id > + * @rpm: rpm handle > + * @type: resource type > + * @id: resource identifier > + * @buf: the data to be written > + * @count: number of bytes in @buf > + */ > +int qcom_rpm_smd_write(struct qcom_smd_rpm *rpm, > + int state, > + u32 type, u32 id, > + void *buf, > + size_t count) > +{ > + static unsigned msg_id = 1; > + int left; > + int ret; > + > + struct { > + struct qcom_rpm_header hdr; > + struct qcom_rpm_request req; > + u8 payload[count]; > + } pkt; > + > + /* SMD packets to the RPM may not exceed 256 bytes */ > + if (WARN_ON(sizeof(pkt) >= 256)) > + return -EINVAL; > + > + mutex_lock(&rpm->lock); > + > + pkt.hdr.service_type = RPM_SERVICE_TYPE_REQUEST; > + pkt.hdr.length = sizeof(struct qcom_rpm_request) + count; > + > + pkt.req.msg_id = msg_id++; > + pkt.req.flags = BIT(state); > + pkt.req.type = type; > + pkt.req.id = id; > + pkt.req.data_len = count; > + memcpy(pkt.payload, buf, count); > + > + ret = qcom_smd_send(rpm->rpm_channel, &pkt, sizeof(pkt)); > + if (ret) > + goto out; > + > + left = wait_for_completion_timeout(&rpm->ack, RPM_REQUEST_TIMEOUT); > + if (!left) > + ret = -ETIMEDOUT; > + else > + ret = rpm->ack_status; > + > +out: > + mutex_unlock(&rpm->lock); > + return ret; > +} > +EXPORT_SYMBOL(qcom_rpm_smd_write); > + > +#define RPM_ERR_INVALID_RESOURCE "resource does not exist" I don't like this at all. > +static int qcom_smd_rpm_callback(struct qcom_smd_device *qsdev, > + const void *data, > + size_t count) > +{ > + const struct qcom_rpm_header *hdr = data; > + const struct qcom_rpm_message *msg; > + const size_t inv_res_len = sizeof(RPM_ERR_INVALID_RESOURCE) - 1; > + struct qcom_smd_rpm *rpm = dev_get_drvdata(&qsdev->dev); > + const u8 *buf = data + sizeof(struct qcom_rpm_header); > + const u8 *end = buf + hdr->length; > + int status = 0; > + > + if (hdr->service_type != RPM_SERVICE_TYPE_REQUEST || > + hdr->length < sizeof(struct qcom_rpm_message)) { > + dev_err(&qsdev->dev, "invalid request\n"); > + return 0; > + } > + > + while (buf < end) { > + msg = (struct qcom_rpm_message *)buf; > + switch (msg->msg_type) { > + case RPM_MSG_TYPE_MSG_ID: > + break; > + case RPM_MSG_TYPE_ERR: > + if (msg->length == inv_res_len && > + !memcmp(msg->message, > + RPM_ERR_INVALID_RESOURCE, > + inv_res_len)) strncpy(msg->message, "resource does not exist", 23); > + status = -ENXIO; > + else > + status = -EIO; > + break; > + } > + > + buf = PTR_ALIGN(buf + 2 * sizeof(u32) + msg->length, 4); > + } > + > + rpm->ack_status = status; > + complete(&rpm->ack); > + return 0; > +} > + > +static int qcom_smd_rpm_probe(struct qcom_smd_device *sdev) > +{ > + struct qcom_smd_rpm *rpm; > + > + rpm = devm_kzalloc(&sdev->dev, sizeof(*rpm), GFP_KERNEL); > + if (!rpm) > + return -ENOMEM; > + > + mutex_init(&rpm->lock); > + init_completion(&rpm->ack); > + > + rpm->rpm_channel = sdev->channel; > + > + dev_set_drvdata(&sdev->dev, rpm); > + > + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev); > +} > + > +static void qcom_smd_rpm_remove(struct qcom_smd_device *sdev) > +{ > + of_platform_depopulate(&sdev->dev); > +} > + > +static const struct of_device_id qcom_smd_rpm_of_match[] = { > + { .compatible = "qcom,rpm-msm8974" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, qcom_smd_rpm_of_match); > + > +static struct qcom_smd_driver qcom_smd_rpm_driver = { > + .probe = qcom_smd_rpm_probe, > + .remove = qcom_smd_rpm_remove, > + .callback = qcom_smd_rpm_callback, > + .driver = { > + .name = "qcom_smd_rpm", > + .owner = THIS_MODULE, Remove this line. > + .of_match_table = qcom_smd_rpm_of_match, > + }, > +}; > + > +module_qcom_smd_driver(qcom_smd_rpm_driver); > + > +MODULE_AUTHOR("Bjorn Andersson <bjorn.andersson@sonymobile.com>"); > +MODULE_DESCRIPTION("Qualcomm SMD backed RPM driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/mfd/qcom-smd-rpm.h b/include/linux/mfd/qcom-smd-rpm.h > new file mode 100644 > index 0000000..2a53dca > --- /dev/null > +++ b/include/linux/mfd/qcom-smd-rpm.h > @@ -0,0 +1,35 @@ > +#ifndef __QCOM_SMD_RPM_H__ > +#define __QCOM_SMD_RPM_H__ > + > +struct qcom_smd_rpm; > + > +#define QCOM_SMD_RPM_ACTIVE_STATE 0 > +#define QCOM_SMD_RPM_SLEEP_STATE 1 > + > +/* > + * Constants used for addressing resources in the RPM. > + */ > +#define QCOM_SMD_RPM_BOOST 0x61747362 > +#define QCOM_SMD_RPM_BUS_CLK 0x316b6c63 > +#define QCOM_SMD_RPM_BUS_MASTER 0x73616d62 > +#define QCOM_SMD_RPM_BUS_SLAVE 0x766c7362 > +#define QCOM_SMD_RPM_CLK_BUF_A 0x616B6C63 > +#define QCOM_SMD_RPM_LDOA 0x616f646c > +#define QCOM_SMD_RPM_LDOB 0x626F646C > +#define QCOM_SMD_RPM_MEM_CLK 0x326b6c63 > +#define QCOM_SMD_RPM_MISC_CLK 0x306b6c63 > +#define QCOM_SMD_RPM_NCPA 0x6170636E > +#define QCOM_SMD_RPM_NCPB 0x6270636E > +#define QCOM_SMD_RPM_OCMEM_PWR 0x706d636f > +#define QCOM_SMD_RPM_QPIC_CLK 0x63697071 > +#define QCOM_SMD_RPM_SMPA 0x61706d73 > +#define QCOM_SMD_RPM_SMPB 0x62706d73 > +#define QCOM_SMD_RPM_SPDM 0x63707362 > +#define QCOM_SMD_RPM_VSA 0x00617376 > + > +int qcom_rpm_smd_write(struct qcom_smd_rpm *rpm, > + int state, > + u32 resource_type, u32 resource_id, > + void *buf, size_t count); > + > +#endif
On Tue 07 Jul 05:37 PDT 2015, Lee Jones wrote: > On Fri, 26 Jun 2015, bjorn@kryo.se wrote: > > > From: Bjorn Andersson <bjorn.andersson@sonymobile.com> [..] > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig [..] > > +config MFD_QCOM_SMD_RPM > > + tristate "Qualcomm Resource Power Manager (RPM) over SMD" > > + depends on QCOM_SMD && OF > > + help > > + If you say yes to this option, support will be included for the > > + Resource Power Manager system found in the Qualcomm 8974 based > > + devices. > > + > > + This is required to access many regulators, clocks and bus > > + frequencies controlled by the RPM on these devices. > > + > > + Say M here if you want to include support for the Qualcomm RPM as a > > + module. This will build a module called "qcom-smd-rpm". > > I'm not exactly sure what makes this an MFD device. > It represents a piece of hardware (a micro-controller) that exposes control of a multitude of regulators and clocks in the Qualcomm platforms. It's basically just a successor of the qcom_rpm driver - same functionality but a new communication method is used. [..] > > diff --git a/drivers/mfd/qcom-smd-rpm.c b/drivers/mfd/qcom-smd-rpm.c [..] > > + > > +#define RPM_ERR_INVALID_RESOURCE "resource does not exist" > > I don't like this at all. > Which part of it? It should probably be a static const char *, inlined in the function below. Would that be to your liking? > > +static int qcom_smd_rpm_callback(struct qcom_smd_device *qsdev, > > + const void *data, > > + size_t count) > > +{ > > + const struct qcom_rpm_header *hdr = data; > > + const struct qcom_rpm_message *msg; > > + const size_t inv_res_len = sizeof(RPM_ERR_INVALID_RESOURCE) - 1; > > + struct qcom_smd_rpm *rpm = dev_get_drvdata(&qsdev->dev); > > + const u8 *buf = data + sizeof(struct qcom_rpm_header); > > + const u8 *end = buf + hdr->length; > > + int status = 0; > > + > > + if (hdr->service_type != RPM_SERVICE_TYPE_REQUEST || > > + hdr->length < sizeof(struct qcom_rpm_message)) { > > + dev_err(&qsdev->dev, "invalid request\n"); > > + return 0; > > + } > > + > > + while (buf < end) { > > + msg = (struct qcom_rpm_message *)buf; > > + switch (msg->msg_type) { > > + case RPM_MSG_TYPE_MSG_ID: > > + break; > > + case RPM_MSG_TYPE_ERR: > > + if (msg->length == inv_res_len && > > + !memcmp(msg->message, > > + RPM_ERR_INVALID_RESOURCE, > > + inv_res_len)) > > strncpy(msg->message, "resource does not exist", 23); > No, I want to compare the content of msg->message with the string "resource does not exist" - as that's the only way to know what type of error we got. This is unfortunately how the protocol looks :/ > > + status = -ENXIO; > > + else > > + status = -EIO; > > + break; > > + } > > + > > + buf = PTR_ALIGN(buf + 2 * sizeof(u32) + msg->length, 4); > > + } > > + > > + rpm->ack_status = status; > > + complete(&rpm->ack); > > + return 0; > > +} > > + [..] > > + > > +static struct qcom_smd_driver qcom_smd_rpm_driver = { > > + .probe = qcom_smd_rpm_probe, > > + .remove = qcom_smd_rpm_remove, > > + .callback = qcom_smd_rpm_callback, > > + .driver = { > > + .name = "qcom_smd_rpm", > > + .owner = THIS_MODULE, > > Remove this line. > The module_qcom_smd_driver does not initialize the .owner, but to follow the general direction of the kernel I can add that to the macro... > > + .of_match_table = qcom_smd_rpm_of_match, > > + }, > > +}; > > + > > +module_qcom_smd_driver(qcom_smd_rpm_driver); > > + > > +MODULE_AUTHOR("Bjorn Andersson <bjorn.andersson@sonymobile.com>"); > > +MODULE_DESCRIPTION("Qualcomm SMD backed RPM driver"); > > +MODULE_LICENSE("GPL v2"); Thanks, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 13 Jul 2015, Bjorn Andersson wrote: > On Tue 07 Jul 05:37 PDT 2015, Lee Jones wrote: > > > On Fri, 26 Jun 2015, bjorn@kryo.se wrote: > > > > > From: Bjorn Andersson <bjorn.andersson@sonymobile.com> > [..] > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > [..] > > > > +config MFD_QCOM_SMD_RPM > > > + tristate "Qualcomm Resource Power Manager (RPM) over SMD" > > > + depends on QCOM_SMD && OF > > > + help > > > + If you say yes to this option, support will be included for the > > > + Resource Power Manager system found in the Qualcomm 8974 based > > > + devices. > > > + > > > + This is required to access many regulators, clocks and bus > > > + frequencies controlled by the RPM on these devices. > > > + > > > + Say M here if you want to include support for the Qualcomm RPM as a > > > + module. This will build a module called "qcom-smd-rpm". > > > > I'm not exactly sure what makes this an MFD device. > > > > It represents a piece of hardware (a micro-controller) that exposes > control of a multitude of regulators and clocks in the Qualcomm > platforms. > > It's basically just a successor of the qcom_rpm driver - same > functionality but a new communication method is used. My point still stands. Please investigate moving this (and the qcom_rpm driver if it's the same) into either drivers/soc or drivers/platform. The support in these two directories _seem_ to be pretty similar. > > > diff --git a/drivers/mfd/qcom-smd-rpm.c b/drivers/mfd/qcom-smd-rpm.c > > [..] > > > > + > > > +#define RPM_ERR_INVALID_RESOURCE "resource does not exist" > > > > I don't like this at all. > > > > Which part of it? > > It should probably be a static const char *, inlined in the function > below. Would that be to your liking? It would be better, but I never really see the point in initialising variables with these types of messages. I'd get rid of the superfluous chuff and just do: memcmp(msg->message, "resource does not exist", 23); > > > +static int qcom_smd_rpm_callback(struct qcom_smd_device *qsdev, > > > + const void *data, > > > + size_t count) > > > +{ > > > + const struct qcom_rpm_header *hdr = data; > > > + const struct qcom_rpm_message *msg; > > > + const size_t inv_res_len = sizeof(RPM_ERR_INVALID_RESOURCE) - 1; > > > + struct qcom_smd_rpm *rpm = dev_get_drvdata(&qsdev->dev); > > > + const u8 *buf = data + sizeof(struct qcom_rpm_header); > > > + const u8 *end = buf + hdr->length; > > > + int status = 0; > > > + > > > + if (hdr->service_type != RPM_SERVICE_TYPE_REQUEST || > > > + hdr->length < sizeof(struct qcom_rpm_message)) { > > > + dev_err(&qsdev->dev, "invalid request\n"); > > > + return 0; > > > + } > > > + > > > + while (buf < end) { > > > + msg = (struct qcom_rpm_message *)buf; > > > + switch (msg->msg_type) { > > > + case RPM_MSG_TYPE_MSG_ID: > > > + break; > > > + case RPM_MSG_TYPE_ERR: > > > + if (msg->length == inv_res_len && > > > + !memcmp(msg->message, > > > + RPM_ERR_INVALID_RESOURCE, > > > + inv_res_len)) > > > > strncpy(msg->message, "resource does not exist", 23); > > > > No, I want to compare the content of msg->message with the string Yes, I just noticed that. > "resource does not exist" - as that's the only way to know what type of > error we got. > > This is unfortunately how the protocol looks :/ What about either my memcmp suggestion above or this then: strncmp(msg->message, "resource does not exist", 23); > > > + status = -ENXIO; > > > + else > > > + status = -EIO; > > > + break; > > > + } > > > + > > > + buf = PTR_ALIGN(buf + 2 * sizeof(u32) + msg->length, 4); > > > + } > > > + > > > + rpm->ack_status = status; > > > + complete(&rpm->ack); > > > + return 0; > > > +} > > > + > > [..] > > > > + > > > +static struct qcom_smd_driver qcom_smd_rpm_driver = { > > > + .probe = qcom_smd_rpm_probe, > > > + .remove = qcom_smd_rpm_remove, > > > + .callback = qcom_smd_rpm_callback, > > > + .driver = { > > > + .name = "qcom_smd_rpm", > > > + .owner = THIS_MODULE, > > > > Remove this line. Still not 100% sure why you need your own 'special' driver struct. If it's for the .callback, there are other ways to do this without having to invent your own bus. > The module_qcom_smd_driver does not initialize the .owner, but to follow > the general direction of the kernel I can add that to the macro... > > > > + .of_match_table = qcom_smd_rpm_of_match, > > > + }, > > > +}; > > > + > > > +module_qcom_smd_driver(qcom_smd_rpm_driver); > > > + > > > +MODULE_AUTHOR("Bjorn Andersson <bjorn.andersson@sonymobile.com>"); > > > +MODULE_DESCRIPTION("Qualcomm SMD backed RPM driver"); > > > +MODULE_LICENSE("GPL v2"); > > Thanks, > Bjorn
On Thu 23 Jul 06:22 PDT 2015, Lee Jones wrote: > On Mon, 13 Jul 2015, Bjorn Andersson wrote: > > > On Tue 07 Jul 05:37 PDT 2015, Lee Jones wrote: > > > > > On Fri, 26 Jun 2015, bjorn@kryo.se wrote: > > > > > > > From: Bjorn Andersson <bjorn.andersson@sonymobile.com> > > [..] > > > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > > > [..] > > > > > > +config MFD_QCOM_SMD_RPM > > > > + tristate "Qualcomm Resource Power Manager (RPM) over SMD" > > > > + depends on QCOM_SMD && OF > > > > + help > > > > + If you say yes to this option, support will be included for the > > > > + Resource Power Manager system found in the Qualcomm 8974 based > > > > + devices. > > > > + > > > > + This is required to access many regulators, clocks and bus > > > > + frequencies controlled by the RPM on these devices. > > > > + > > > > + Say M here if you want to include support for the Qualcomm RPM as a > > > > + module. This will build a module called "qcom-smd-rpm". > > > > > > I'm not exactly sure what makes this an MFD device. > > > > > > > It represents a piece of hardware (a micro-controller) that exposes > > control of a multitude of regulators and clocks in the Qualcomm > > platforms. > > > > It's basically just a successor of the qcom_rpm driver - same > > functionality but a new communication method is used. > > My point still stands. Please investigate moving this (and the > qcom_rpm driver if it's the same) into either drivers/soc or > drivers/platform. The support in these two directories _seem_ to be > pretty similar. > We had this exact discussion last year and I argued that a piece of hardware that exposes regulators and clocks - like most PMICs - is a mfd and you agreed and picked the driver. I will have a word with Andy about moving this and the qcom_rpm driver out of mfd. > > > > diff --git a/drivers/mfd/qcom-smd-rpm.c b/drivers/mfd/qcom-smd-rpm.c > > > > [..] > > > > > > + > > > > +#define RPM_ERR_INVALID_RESOURCE "resource does not exist" > > > > > > I don't like this at all. > > > > > > > Which part of it? > > > > It should probably be a static const char *, inlined in the function > > below. Would that be to your liking? > > It would be better, but I never really see the point in initialising > variables with these types of messages. I'd get rid of the > superfluous chuff and just do: > > memcmp(msg->message, "resource does not exist", 23); > The point was simply to not have to write: if (msg->length == 23 && memcmp(msg->message, ..., 23); Simply because I don't like the first part of the expression. I'll rewrite it... > > > > +static int qcom_smd_rpm_callback(struct qcom_smd_device *qsdev, > > > > + const void *data, > > > > + size_t count) > > > > +{ > > > > + const struct qcom_rpm_header *hdr = data; > > > > + const struct qcom_rpm_message *msg; > > > > + const size_t inv_res_len = sizeof(RPM_ERR_INVALID_RESOURCE) - 1; > > > > + struct qcom_smd_rpm *rpm = dev_get_drvdata(&qsdev->dev); > > > > + const u8 *buf = data + sizeof(struct qcom_rpm_header); > > > > + const u8 *end = buf + hdr->length; > > > > + int status = 0; > > > > + > > > > + if (hdr->service_type != RPM_SERVICE_TYPE_REQUEST || > > > > + hdr->length < sizeof(struct qcom_rpm_message)) { > > > > + dev_err(&qsdev->dev, "invalid request\n"); > > > > + return 0; > > > > + } > > > > + > > > > + while (buf < end) { > > > > + msg = (struct qcom_rpm_message *)buf; > > > > + switch (msg->msg_type) { > > > > + case RPM_MSG_TYPE_MSG_ID: > > > > + break; > > > > + case RPM_MSG_TYPE_ERR: > > > > + if (msg->length == inv_res_len && > > > > + !memcmp(msg->message, > > > > + RPM_ERR_INVALID_RESOURCE, > > > > + inv_res_len)) > > > > > > strncpy(msg->message, "resource does not exist", 23); > > > > > > > No, I want to compare the content of msg->message with the string > > Yes, I just noticed that. > > > "resource does not exist" - as that's the only way to know what type of > > error we got. > > > > This is unfortunately how the protocol looks :/ > > What about either my memcmp suggestion above or this then: > > strncmp(msg->message, "resource does not exist", 23); > That would require the string to be 0-terminated. [..] > > > > +static struct qcom_smd_driver qcom_smd_rpm_driver = { > > > > + .probe = qcom_smd_rpm_probe, > > > > + .remove = qcom_smd_rpm_remove, > > > > + .callback = qcom_smd_rpm_callback, > > > > + .driver = { > > > > + .name = "qcom_smd_rpm", > > > > + .owner = THIS_MODULE, > > > > > > Remove this line. > > Still not 100% sure why you need your own 'special' driver struct. If > it's for the .callback, there are other ways to do this without having > to invent your own bus. > Because the life cycle of these components are much like, say, USB - they can come and go. As such e.g. a platform_driver is not a good fit. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 23 Jul 2015, Bjorn Andersson wrote: > On Thu 23 Jul 06:22 PDT 2015, Lee Jones wrote: > > > On Mon, 13 Jul 2015, Bjorn Andersson wrote: > > > > > On Tue 07 Jul 05:37 PDT 2015, Lee Jones wrote: > > > > > > > On Fri, 26 Jun 2015, bjorn@kryo.se wrote: > > > > > > > > > From: Bjorn Andersson <bjorn.andersson@sonymobile.com> > > > [..] > > > > > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > > > > > [..] > > > > > > > > +config MFD_QCOM_SMD_RPM > > > > > + tristate "Qualcomm Resource Power Manager (RPM) over SMD" > > > > > + depends on QCOM_SMD && OF > > > > > + help > > > > > + If you say yes to this option, support will be included for the > > > > > + Resource Power Manager system found in the Qualcomm 8974 based > > > > > + devices. > > > > > + > > > > > + This is required to access many regulators, clocks and bus > > > > > + frequencies controlled by the RPM on these devices. > > > > > + > > > > > + Say M here if you want to include support for the Qualcomm RPM as a > > > > > + module. This will build a module called "qcom-smd-rpm". > > > > > > > > I'm not exactly sure what makes this an MFD device. > > > > > > > > > > It represents a piece of hardware (a micro-controller) that exposes > > > control of a multitude of regulators and clocks in the Qualcomm > > > platforms. > > > > > > It's basically just a successor of the qcom_rpm driver - same > > > functionality but a new communication method is used. > > > > My point still stands. Please investigate moving this (and the > > qcom_rpm driver if it's the same) into either drivers/soc or > > drivers/platform. The support in these two directories _seem_ to be > > pretty similar. > > > > We had this exact discussion last year and I argued that a piece of > hardware that exposes regulators and clocks - like most PMICs - is a > mfd and you agreed and picked the driver. I've become stricter since then. An IC which only does power management should either live in drivers/power, or more recently they have been described as platform specific drivers which have subsequently been moved to drivers/platform. Particularly if they have their own special, platform specific communication method/bus. > I will have a word with Andy about moving this and the qcom_rpm driver > out of mfd. Thanks. > > > > > diff --git a/drivers/mfd/qcom-smd-rpm.c b/drivers/mfd/qcom-smd-rpm.c > > > > > > [..] > > > > > > > > + > > > > > +#define RPM_ERR_INVALID_RESOURCE "resource does not exist" > > > > > > > > I don't like this at all. > > > > > > > > > > Which part of it? > > > > > > It should probably be a static const char *, inlined in the function > > > below. Would that be to your liking? > > > > It would be better, but I never really see the point in initialising > > variables with these types of messages. I'd get rid of the > > superfluous chuff and just do: > > > > memcmp(msg->message, "resource does not exist", 23); > > > > The point was simply to not have to write: > > if (msg->length == 23 && memcmp(msg->message, ..., 23); > > Simply because I don't like the first part of the expression. I'll > rewrite it... All this cruft just to avoid that? Just define '23', then code looks good and problem vaporises. > > > > > +static int qcom_smd_rpm_callback(struct qcom_smd_device *qsdev, > > > > > + const void *data, > > > > > + size_t count) > > > > > +{ > > > > > + const struct qcom_rpm_header *hdr = data; > > > > > + const struct qcom_rpm_message *msg; > > > > > + const size_t inv_res_len = sizeof(RPM_ERR_INVALID_RESOURCE) - 1; > > > > > + struct qcom_smd_rpm *rpm = dev_get_drvdata(&qsdev->dev); > > > > > + const u8 *buf = data + sizeof(struct qcom_rpm_header); > > > > > + const u8 *end = buf + hdr->length; > > > > > + int status = 0; > > > > > + > > > > > + if (hdr->service_type != RPM_SERVICE_TYPE_REQUEST || > > > > > + hdr->length < sizeof(struct qcom_rpm_message)) { > > > > > + dev_err(&qsdev->dev, "invalid request\n"); > > > > > + return 0; > > > > > + } > > > > > + > > > > > + while (buf < end) { > > > > > + msg = (struct qcom_rpm_message *)buf; > > > > > + switch (msg->msg_type) { > > > > > + case RPM_MSG_TYPE_MSG_ID: > > > > > + break; > > > > > + case RPM_MSG_TYPE_ERR: > > > > > + if (msg->length == inv_res_len && > > > > > + !memcmp(msg->message, > > > > > + RPM_ERR_INVALID_RESOURCE, > > > > > + inv_res_len)) > > > > > > > > strncpy(msg->message, "resource does not exist", 23); > > > > > > > > > > No, I want to compare the content of msg->message with the string > > > > Yes, I just noticed that. > > > > > "resource does not exist" - as that's the only way to know what type of > > > error we got. > > > > > > This is unfortunately how the protocol looks :/ > > > > What about either my memcmp suggestion above or this then: > > > > strncmp(msg->message, "resource does not exist", 23); > > > > That would require the string to be 0-terminated. No it doesn't. strNcmp, only compares the first N characters. > > > > > +static struct qcom_smd_driver qcom_smd_rpm_driver = { > > > > > + .probe = qcom_smd_rpm_probe, > > > > > + .remove = qcom_smd_rpm_remove, > > > > > + .callback = qcom_smd_rpm_callback, > > > > > + .driver = { > > > > > + .name = "qcom_smd_rpm", > > > > > + .owner = THIS_MODULE, > > > > > > > > Remove this line. > > > > Still not 100% sure why you need your own 'special' driver struct. If > > it's for the .callback, there are other ways to do this without having > > to invent your own bus. > > Because the life cycle of these components are much like, say, USB - > they can come and go. As such e.g. a platform_driver is not a good fit. You mean they are hot-swappable? Don't we have any platform devices which support that already?
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 6538159..666c812 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -656,6 +656,20 @@ config MFD_QCOM_RPM Say M here if you want to include support for the Qualcomm RPM as a module. This will build a module called "qcom_rpm". +config MFD_QCOM_SMD_RPM + tristate "Qualcomm Resource Power Manager (RPM) over SMD" + depends on QCOM_SMD && OF + help + If you say yes to this option, support will be included for the + Resource Power Manager system found in the Qualcomm 8974 based + devices. + + This is required to access many regulators, clocks and bus + frequencies controlled by the RPM on these devices. + + Say M here if you want to include support for the Qualcomm RPM as a + module. This will build a module called "qcom-smd-rpm". + config MFD_SPMI_PMIC tristate "Qualcomm SPMI PMICs" depends on ARCH_QCOM || COMPILE_TEST diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index ea40e07..289bd24 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -156,6 +156,7 @@ obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o obj-$(CONFIG_MFD_OMAP_USB_HOST) += omap-usb-host.o omap-usb-tll.o obj-$(CONFIG_MFD_PM8921_CORE) += pm8921-core.o ssbi.o obj-$(CONFIG_MFD_QCOM_RPM) += qcom_rpm.o +obj-$(CONFIG_MFD_QCOM_SMD_RPM) += qcom-smd-rpm.o obj-$(CONFIG_MFD_SPMI_PMIC) += qcom-spmi-pmic.o obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o obj-$(CONFIG_MFD_TPS65090) += tps65090.o diff --git a/drivers/mfd/qcom-smd-rpm.c b/drivers/mfd/qcom-smd-rpm.c new file mode 100644 index 0000000..d6faf85 --- /dev/null +++ b/drivers/mfd/qcom-smd-rpm.c @@ -0,0 +1,236 @@ +/* + * Copyright (c) 2015, Sony Mobile Communications AB. + * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/of_platform.h> +#include <linux/io.h> +#include <linux/interrupt.h> + +#include <linux/soc/qcom/smd.h> +#include <linux/mfd/qcom-smd-rpm.h> + +#include <dt-bindings/mfd/qcom-smd-rpm.h> + +#define RPM_REQUEST_TIMEOUT (5 * HZ) + +/** + * struct qcom_smd_rpm - state of the rpm device driver + * @rpm_channel: reference to the smd channel + * @ack: completion for acks + * @lock: mutual exclusion around the send/complete pair + * @ack_status: result of the rpm request + */ +struct qcom_smd_rpm { + struct qcom_smd_channel *rpm_channel; + + struct completion ack; + struct mutex lock; + int ack_status; +}; + +/** + * struct qcom_rpm_header - header for all rpm requests and responses + * @service_type: identifier of the service + * @length: length of the payload + */ +struct qcom_rpm_header { + u32 service_type; + u32 length; +}; + +/** + * struct qcom_rpm_request - request message to the rpm + * @msg_id: identifier of the outgoing message + * @flags: active/sleep state flags + * @type: resource type + * @id: resource id + * @data_len: length of the payload following this header + */ +struct qcom_rpm_request { + u32 msg_id; + u32 flags; + u32 type; + u32 id; + u32 data_len; +}; + +/** + * struct qcom_rpm_message - response message from the rpm + * @msg_type: indicator of the type of message + * @length: the size of this message, including the message header + * @msg_id: message id + * @message: textual message from the rpm + * + * Multiple of these messages can be stacked in an rpm message. + */ +struct qcom_rpm_message { + u32 msg_type; + u32 length; + union { + u32 msg_id; + u8 message[0]; + }; +}; + +#define RPM_SERVICE_TYPE_REQUEST 0x00716572 /* "req\0" */ + +#define RPM_MSG_TYPE_ERR 0x00727265 /* "err\0" */ +#define RPM_MSG_TYPE_MSG_ID 0x2367736d /* "msg#" */ + +/** + * qcom_rpm_smd_write - write @buf to @type:@id + * @rpm: rpm handle + * @type: resource type + * @id: resource identifier + * @buf: the data to be written + * @count: number of bytes in @buf + */ +int qcom_rpm_smd_write(struct qcom_smd_rpm *rpm, + int state, + u32 type, u32 id, + void *buf, + size_t count) +{ + static unsigned msg_id = 1; + int left; + int ret; + + struct { + struct qcom_rpm_header hdr; + struct qcom_rpm_request req; + u8 payload[count]; + } pkt; + + /* SMD packets to the RPM may not exceed 256 bytes */ + if (WARN_ON(sizeof(pkt) >= 256)) + return -EINVAL; + + mutex_lock(&rpm->lock); + + pkt.hdr.service_type = RPM_SERVICE_TYPE_REQUEST; + pkt.hdr.length = sizeof(struct qcom_rpm_request) + count; + + pkt.req.msg_id = msg_id++; + pkt.req.flags = BIT(state); + pkt.req.type = type; + pkt.req.id = id; + pkt.req.data_len = count; + memcpy(pkt.payload, buf, count); + + ret = qcom_smd_send(rpm->rpm_channel, &pkt, sizeof(pkt)); + if (ret) + goto out; + + left = wait_for_completion_timeout(&rpm->ack, RPM_REQUEST_TIMEOUT); + if (!left) + ret = -ETIMEDOUT; + else + ret = rpm->ack_status; + +out: + mutex_unlock(&rpm->lock); + return ret; +} +EXPORT_SYMBOL(qcom_rpm_smd_write); + +#define RPM_ERR_INVALID_RESOURCE "resource does not exist" + +static int qcom_smd_rpm_callback(struct qcom_smd_device *qsdev, + const void *data, + size_t count) +{ + const struct qcom_rpm_header *hdr = data; + const struct qcom_rpm_message *msg; + const size_t inv_res_len = sizeof(RPM_ERR_INVALID_RESOURCE) - 1; + struct qcom_smd_rpm *rpm = dev_get_drvdata(&qsdev->dev); + const u8 *buf = data + sizeof(struct qcom_rpm_header); + const u8 *end = buf + hdr->length; + int status = 0; + + if (hdr->service_type != RPM_SERVICE_TYPE_REQUEST || + hdr->length < sizeof(struct qcom_rpm_message)) { + dev_err(&qsdev->dev, "invalid request\n"); + return 0; + } + + while (buf < end) { + msg = (struct qcom_rpm_message *)buf; + switch (msg->msg_type) { + case RPM_MSG_TYPE_MSG_ID: + break; + case RPM_MSG_TYPE_ERR: + if (msg->length == inv_res_len && + !memcmp(msg->message, + RPM_ERR_INVALID_RESOURCE, + inv_res_len)) + status = -ENXIO; + else + status = -EIO; + break; + } + + buf = PTR_ALIGN(buf + 2 * sizeof(u32) + msg->length, 4); + } + + rpm->ack_status = status; + complete(&rpm->ack); + return 0; +} + +static int qcom_smd_rpm_probe(struct qcom_smd_device *sdev) +{ + struct qcom_smd_rpm *rpm; + + rpm = devm_kzalloc(&sdev->dev, sizeof(*rpm), GFP_KERNEL); + if (!rpm) + return -ENOMEM; + + mutex_init(&rpm->lock); + init_completion(&rpm->ack); + + rpm->rpm_channel = sdev->channel; + + dev_set_drvdata(&sdev->dev, rpm); + + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev); +} + +static void qcom_smd_rpm_remove(struct qcom_smd_device *sdev) +{ + of_platform_depopulate(&sdev->dev); +} + +static const struct of_device_id qcom_smd_rpm_of_match[] = { + { .compatible = "qcom,rpm-msm8974" }, + {} +}; +MODULE_DEVICE_TABLE(of, qcom_smd_rpm_of_match); + +static struct qcom_smd_driver qcom_smd_rpm_driver = { + .probe = qcom_smd_rpm_probe, + .remove = qcom_smd_rpm_remove, + .callback = qcom_smd_rpm_callback, + .driver = { + .name = "qcom_smd_rpm", + .owner = THIS_MODULE, + .of_match_table = qcom_smd_rpm_of_match, + }, +}; + +module_qcom_smd_driver(qcom_smd_rpm_driver); + +MODULE_AUTHOR("Bjorn Andersson <bjorn.andersson@sonymobile.com>"); +MODULE_DESCRIPTION("Qualcomm SMD backed RPM driver"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/mfd/qcom-smd-rpm.h b/include/linux/mfd/qcom-smd-rpm.h new file mode 100644 index 0000000..2a53dca --- /dev/null +++ b/include/linux/mfd/qcom-smd-rpm.h @@ -0,0 +1,35 @@ +#ifndef __QCOM_SMD_RPM_H__ +#define __QCOM_SMD_RPM_H__ + +struct qcom_smd_rpm; + +#define QCOM_SMD_RPM_ACTIVE_STATE 0 +#define QCOM_SMD_RPM_SLEEP_STATE 1 + +/* + * Constants used for addressing resources in the RPM. + */ +#define QCOM_SMD_RPM_BOOST 0x61747362 +#define QCOM_SMD_RPM_BUS_CLK 0x316b6c63 +#define QCOM_SMD_RPM_BUS_MASTER 0x73616d62 +#define QCOM_SMD_RPM_BUS_SLAVE 0x766c7362 +#define QCOM_SMD_RPM_CLK_BUF_A 0x616B6C63 +#define QCOM_SMD_RPM_LDOA 0x616f646c +#define QCOM_SMD_RPM_LDOB 0x626F646C +#define QCOM_SMD_RPM_MEM_CLK 0x326b6c63 +#define QCOM_SMD_RPM_MISC_CLK 0x306b6c63 +#define QCOM_SMD_RPM_NCPA 0x6170636E +#define QCOM_SMD_RPM_NCPB 0x6270636E +#define QCOM_SMD_RPM_OCMEM_PWR 0x706d636f +#define QCOM_SMD_RPM_QPIC_CLK 0x63697071 +#define QCOM_SMD_RPM_SMPA 0x61706d73 +#define QCOM_SMD_RPM_SMPB 0x62706d73 +#define QCOM_SMD_RPM_SPDM 0x63707362 +#define QCOM_SMD_RPM_VSA 0x00617376 + +int qcom_rpm_smd_write(struct qcom_smd_rpm *rpm, + int state, + u32 resource_type, u32 resource_id, + void *buf, size_t count); + +#endif