On Mon, Feb 05 2018 at 19:19 +0000, Bjorn Andersson wrote:
On Thu 18 Jan 16:01 PST 2018, Lina Iyer wrote:
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
new file mode 100644
index 000000000000..3e68cef5513e
--- /dev/null
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -0,0 +1,674 @@
+/* Copyright (c) 2016-2018, 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.
+ *
+ */
Please use SPDX headers for new files.
// SPDX-License-Identifier: GPL-2.0
/*
* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
*/
OK.
+
+#define pr_fmt(fmt) "%s " fmt, KBUILD_MODNAME
+
+#include <linux/atomic.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
Unused?
Will remove.
+/**
+ * tcs_mbox: group of TCSes for a request state
+ *
+ * @type: type of the TCS in this group - active, sleep, wake
+ * @tcs_mask: mask of the TCSes relative to all the TCSes in the RSC
+ * @tcs_offset: start of the TCS group relative to the TCSes in the RSC
+ * @num_tcs: number of TCSes in this type
+ * @ncpt: number of commands in each TCS
+ * @tcs_lock: lock for synchronizing this TCS writes
+ * @responses: response objects for requests sent from each TCS
+ */
+struct tcs_mbox {
struct tcs_group ?
Hmm.. I am okay with that.
+ struct rsc_drv *drv;
+ int type;
+ u32 tcs_mask;
+ u32 tcs_offset;
+ int num_tcs;
+ int ncpt;
+ spinlock_t tcs_lock;
+ struct tcs_response *responses[MAX_TCS_PER_TYPE];
+};
+
+/**
+ * rsc_drv: the RSC controller
+ *
+ * @pdev: platform device of this controller
+ * @name: controller identifier
+ * @base: start address of the controller
+ * @reg_base: start address of the registers in this controller
+ * @drv_id: instance id in the controller (DRV)
+ * @num_tcs: number of TCSes in this DRV
+ * @tasklet: handle responses, off-load work from IRQ handler
+ * @response_pending: list of responses that needs to be sent to caller
+ * @tcs: TCS groups
+ * @tcs_in_use: s/w state of the TCS
+ * @drv_lock: synchronize state of the controller
+ */
+struct rsc_drv {
+ struct platform_device *pdev;
This is not used outside rpmh_rsc_probe() and I presume you would like
to be able to use it for dev_prints(), if so store the device * instead.
I don't need it. Will remove.
+ const char *name;
+ void __iomem *base;
+ void __iomem *reg_base;
+ int drv_id;
@base and @drv_id are local to rpmh_rsc_probe(), so you can make it them
local variables.
They are used in the other functions and are needed in this structure.
See IRQ handler and tcs_is_free().
+ int num_tcs;
+ struct tasklet_struct tasklet;
+ struct list_head response_pending;
+ struct tcs_mbox tcs[TCS_TYPE_NR];
+ atomic_t tcs_in_use[MAX_TCS_NR];
Does this really need to be an atomic_t? Seems like it's protected by
the drv_lock.
Not in the irq handler.
+ spinlock_t drv_lock;
+};
+
+static inline struct tcs_mbox *get_tcs_from_index(struct rsc_drv *drv, int m)
get_tcs_group_by_index(), or maybe even get_tcs_group_by_tcs()? To
clarify that this resolves a group and not a single tcs.
Also, please drop the "inline".
Ok.
+{
+ struct tcs_mbox *tcs = NULL;
+ int i;
+
+ for (i = 0; i < drv->num_tcs; i++) {
+ tcs = &drv->tcs[i];
+ if (tcs->tcs_mask & (u32)BIT(m))
I don't think you need this cast.
OK.
+ break;
+ }
+
+ if (i == drv->num_tcs) {
+ WARN(1, "Incorrect TCS index %d", m);
+ tcs = NULL;
+ }
+
+ return tcs;
+}
+
+static struct tcs_response *setup_response(struct rsc_drv *drv,
+ struct tcs_mbox_msg *msg, int m)
+{
+ struct tcs_response *resp;
+ struct tcs_mbox *tcs;
+
+ resp = kcalloc(1, sizeof(*resp), GFP_ATOMIC);
+ if (!resp)
+ return ERR_PTR(-ENOMEM);
Rather than allocating a response for each write request and freeing it
in the tasklet I think you should just allocate MAX_TCS_PER_TYPE
responses when you're probing the driver.
I am keeping this simple for now. Ideally, a pool of responses would be
more helpful and instead of this. I will probably bring that in later.
+
+ resp->drv = drv;
+ resp->msg = msg;
+ resp->err = 0;
+
+ tcs = get_tcs_from_index(drv, m);
+ if (!tcs)
+ return ERR_PTR(-EINVAL);
+
+ /*
+ * We should have been called from a TCS-type locked context, and
+ * we overwrite the previously saved response.
+ */
I assume that this overwrite wouldn't happen because we're only writing
to the tcs if it's not in use? Wouldn't overwriting a valid response
lead to us leaking the previously allocated resp?
No. The freeing of the response is handled in the tasklet. Here we just
start using a new response object while the old one is still being
processed.
+ tcs->responses[m - tcs->tcs_offset] = resp;
+
+ return resp;
+}
+
+static void free_response(struct tcs_response *resp)
+{
+ kfree(resp);
+}
+
+static inline struct tcs_response *get_response(struct rsc_drv *drv, u32 m)
+{
+ struct tcs_mbox *tcs = get_tcs_from_index(drv, m);
+
+ return tcs->responses[m - tcs->tcs_offset];
+}
+
+static inline u32 read_drv_config(void __iomem *base)
+{
+ return le32_to_cpu(readl_relaxed(base + DRV_PRNT_CHLD_CONFIG));
readl_relaxed() on arm64 is defined as le32_to_cpu(__raw_readl(c)), so I
would suggest that you drop the le32_to_cpu() and just inline your
readl() directly in rpmh_rsc_probe().
Ok.
+}
+
+static inline u32 read_tcs_reg(void __iomem *base, int reg, int m, int n)
+{
I would suggest that you pass the rsc_drv to these functions, so that
it's obvious which "base" we're operating on here.
Hmm. Ok. I used the void * because they are simple register functions
and have no bearing on the DRV they operate on.
But sure.
+ return le32_to_cpu(readl_relaxed(base + reg +
+ RSC_DRV_TCS_OFFSET * m + RSC_DRV_CMD_OFFSET * n));
No le32_to_cpu()
Ok.
+}
+
+static inline void write_tcs_reg(void __iomem *base, int reg, int m, int n,
+ u32 data)
+{
+ writel_relaxed(cpu_to_le32(data), base + reg +
+ RSC_DRV_TCS_OFFSET * m + RSC_DRV_CMD_OFFSET * n);
As with readl, writel does cpu_to_le32()
Ok
+}
+
+static inline void write_tcs_reg_sync(void __iomem *base, int reg, int m, int n,
+ u32 data)
+{
+ do {
+ write_tcs_reg(base, reg, m, n, data);
+ if (data == read_tcs_reg(base, reg, m, n))
+ break;
+ udelay(1);
+ } while (1);
+}
+
+static inline bool tcs_is_free(struct rsc_drv *drv, int m)
+{
+ void __iomem *base = drv->reg_base;
+
+ return read_tcs_reg(base, RSC_DRV_STATUS, m, 0) &&
+ !atomic_read(&drv->tcs_in_use[m]);
If you flip these conditions around you won't do the io access when
tcs_in_use[m].
Sure.
What are the cases when tcs_in_use will tell you that the tcs is free,
but the hardware will say it's not?
In the case of display RSC, which could be directly used by the display
driver, my state in tcs_in_use() would not reflect the state of the
hardware. This should not happen, but I would like to ensure that it
doesn't.
+}
+
+static inline struct tcs_mbox *get_tcs_of_type(struct rsc_drv *drv, int type)
+{
+ int i;
+ struct tcs_mbox *tcs;
+
+ for (i = 0; i < TCS_TYPE_NR; i++)
+ if (type == drv->tcs[i].type)
+ break;
+
+ if (i == TCS_TYPE_NR)
+ return ERR_PTR(-EINVAL);
+
+ tcs = &drv->tcs[i];
+ if (!tcs->num_tcs)
+ return ERR_PTR(-EINVAL);
+
+ return tcs;
+}
+
+static inline struct tcs_mbox *get_tcs_for_msg(struct rsc_drv *drv,
+ struct tcs_mbox_msg *msg)
+{
+ int type = -1;
+
+ switch (msg->state) {
+ case RPMH_ACTIVE_ONLY_STATE:
+ type = ACTIVE_TCS;
+ break;
+ default:
+ break;
+ }
+
+ if (type < 0)
+ return ERR_PTR(-EINVAL);
afaict you can put this return in the default case.
Ok.
+
+ return get_tcs_of_type(drv, type);
+}
+
+static inline void send_tcs_response(struct tcs_response *resp)
+{
+ struct rsc_drv *drv = resp->drv;
+ unsigned long flags;
+
+ spin_lock_irqsave(&drv->drv_lock, flags);
+ INIT_LIST_HEAD(&resp->list);
+ list_add_tail(&resp->list, &drv->response_pending);
+ spin_unlock_irqrestore(&drv->drv_lock, flags);
+
+ tasklet_schedule(&drv->tasklet);
+}
Is there any particular reason why this uses a tasklet, instead of just
handling the tx_done event from the interrupt handler directly?
Its a bit too heavy for the interrupt handler to do all this locking and
respond back and signal completion (done in the next patches). Hence the
work is offloaded to the tasklet, we can be faster in responding to
other requests that have received the responses.
+
+/**
+ * tcs_irq_handler: TX Done interrupt handler
+ */
+static irqreturn_t tcs_irq_handler(int irq, void *p)
+{
+ struct rsc_drv *drv = p;
+ void __iomem *base = drv->reg_base;
+ int m, i;
+ u32 irq_status, sts;
+ struct tcs_response *resp;
+ struct tcs_cmd *cmd;
+ int err;
+
+ irq_status = read_tcs_reg(base, RSC_DRV_IRQ_STATUS, 0, 0);
+
+ for (m = 0; m < drv->num_tcs; m++) {
+ if (!(irq_status & (u32)BIT(m)))
+ continue;
+
+ err = 0;
+ resp = get_response(drv, m);
+ if (!resp) {
This conditional section dereferences resp, so I suspect that the
conditional is backwards.
Jeez, how did it not fail. Thanks for catching this..
+ for (i = 0; i < resp->msg->num_payload; i++) {
+ cmd = &resp->msg->payload[i];
+ sts = read_tcs_reg(base, RSC_DRV_CMD_STATUS,
+ m, i);
+ if ((!(sts & CMD_STATUS_ISSUED)) ||
+ ((resp->msg->is_complete ||
+ cmd->complete) &&
+ (!(sts & CMD_STATUS_COMPL)))) {
+ resp->err = -EIO;
+ break;
As this completes the response, aren't we leaking the tcs_in_use?
No, we would break out of this loop and set the tcs_in_use. We have no
use of this TCS anymore. The h/w status is also reset.
+ }
+ }
+ }
+
+ write_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0, 0);
+ write_tcs_reg(base, RSC_DRV_IRQ_CLEAR, 0, 0, BIT(m));
+ atomic_set(&drv->tcs_in_use[m], 0);
+
+ if (resp) {
+ resp->err = err;
+ send_tcs_response(resp);
+ }
+ }
+
+ return IRQ_HANDLED;
+}
+
+/**
+ * tcs_notify_tx_done: TX Done for requests that got a response
+ *
+ * @data: the tasklet argument
+ *
+ * Tasklet function to notify MBOX that we are done with the request.
+ * Handles all pending reponses whenever run.
+ */
+static void tcs_notify_tx_done(unsigned long data)
I think you should drop the tasklet and just invoke the handling of
responses directly from the irq context.
I think, it will affect performance in multi-threaded usecases if we do
that. It is performance critical that we free up the TCS as soon as
possible. We only have 2 of them and there could be multiple clients
competing for that at the same time. Any work offloaded from the IRQ
handler will help move the pipe faster.
+{
+ struct rsc_drv *drv = (struct rsc_drv *)data;
+ struct tcs_response *resp;
+ unsigned long flags;
+ int err;
+
+ do {
+ spin_lock_irqsave(&drv->drv_lock, flags);
+ if (list_empty(&drv->response_pending)) {
+ spin_unlock_irqrestore(&drv->drv_lock, flags);
+ break;
+ }
+ resp = list_first_entry(&drv->response_pending,
+ struct tcs_response, list);
+ list_del(&resp->list);
+ spin_unlock_irqrestore(&drv->drv_lock, flags);
+ err = resp->err;
+ free_response(resp);
+ } while (1);
Use for (;;) {} or while (1) {}, rather than do {} while(1)
OK.
+ curr_enabled = read_tcs_reg(base, RSC_DRV_CMD_ENABLE, m, 0);
+
+ for (j = 0; j < MAX_CMDS_PER_TCS; j++) {
+ if (!(curr_enabled & (u32)BIT(j)))
+ continue;
Consider using for_each_set_bit() here.
Ok
+/**
+ * rpmh_rsc_send_data: Validate the incoming message and write to the
+ * appropriate TCS block.
+ *
+ * @drv: the controller
+ * @msg: the data to be sent
+ *
+ * Returns a negative error for invalid message structure and invalid
+ * message combination, -EBUSY if there is an other active request for
+ * the channel in process, otherwise bubbles up internal error.
Kernel-doc style is "Return: a negative..."
The function never returns -EBUSY, as you're looping while this is being
returned.
Also, it would be nice if the doc mentions that 0 is returned on
success, so perhaps just "Return: 0 on success, negative errno on
error"?
Sure.
+ */
+int rpmh_rsc_send_data(struct rsc_drv *drv, struct tcs_mbox_msg *msg)
+{
+ int ret = 0;
+
+ if (!msg || !msg->payload || !msg->num_payload ||
+ msg->num_payload > MAX_RPMH_PAYLOAD)
+ return -EINVAL;
+
+ do {
+ ret = tcs_mbox_write(drv, msg);
+ if (ret == -EBUSY) {
+ pr_info_ratelimited(
+ "TCS Busy, retrying RPMH message send: addr=0x%x\n",
+ msg->payload[0].addr);
Indent wrapped lines to the start parenthesis.
It was hard with the message overflowing the 80 char limit. Will try to
clean it up.
+ udelay(10);
+ }
+ } while (ret == -EBUSY);
+
+ return ret;
+}
+EXPORT_SYMBOL(rpmh_rsc_send_data);
+
+static int rpmh_rsc_probe(struct platform_device *pdev)
+{
+ struct device_node *dn = pdev->dev.of_node;
+ struct rsc_drv *drv;
+ struct tcs_mbox *tcs;
+ int irq;
+ u32 val[8] = { 0 };
8 here should be TCS_TYPE_NR * 2, but I like Tomas(?) suggestion in the
PDC driver of representing arrays of tuples like this as a structured
type.
OK.
+ int st = 0;
+ int i, ret, nelem;
+ u32 config, max_tcs, ncpt;
+ int tcs_type_count[TCS_TYPE_NR] = { 0 };
+ struct resource *res;
+
+ drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
+ if (!drv)
+ return -ENOMEM;
+
+ ret = of_property_read_u32(dn, "qcom,drv-id", &drv->drv_id);
+ if (ret)
+ return ret;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -EINVAL;
+ drv->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(drv->base))
+ return PTR_ERR(drv->base);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!res)
+ return -EINVAL;
+ drv->reg_base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(drv->reg_base))
+ return PTR_ERR(drv->reg_base);
+
+ config = read_drv_config(drv->base);
+ max_tcs = config & (DRV_NUM_TCS_MASK <<
+ (DRV_NUM_TCS_SHIFT * drv->drv_id));
+ max_tcs = max_tcs >> (DRV_NUM_TCS_SHIFT * drv->drv_id);
+ ncpt = config & (DRV_NCPT_MASK << DRV_NCPT_SHIFT);
+ ncpt = ncpt >> DRV_NCPT_SHIFT;
+
+ nelem = of_property_count_elems_of_size(dn, "qcom,tcs-config",
+ sizeof(u32));
+ if (!nelem || (nelem % 2) || (nelem > 2 * TCS_TYPE_NR))
+ return -EINVAL;
+
+ ret = of_property_read_u32_array(dn, "qcom,tcs-config", val, nelem);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < (nelem / 2); i++) {
+ if (val[2 * i] >= TCS_TYPE_NR)
+ return -EINVAL;
+ tcs_type_count[val[2 * i]]++;
+ if (tcs_type_count[val[2 * i]] > 1)
+ return -EINVAL;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(tcs_type_count); i++)
+ if (!tcs_type_count[i])
+ return -EINVAL;
I think it's better if replace these two loops by adding a check in
below loop to ensure that tcs->drv is NULL and that tcs->type is valid
and then follow up with a loop checking that each drv->tcs was
initialized.
That way you don't need to do the counting of the various types.
I need to count to ensure that all types are mentioned in the DT.
+
+ for (i = 0; i < (nelem / 2); i++) {
+ tcs = &drv->tcs[val[2 * i]];
+ tcs->drv = drv;
+ tcs->type = val[2 * i];
+ tcs->num_tcs = val[2 * i + 1];
+ tcs->ncpt = ncpt;
+ spin_lock_init(&tcs->tcs_lock);
+
+ if (tcs->num_tcs <= 0 || tcs->type == CONTROL_TCS)
+ continue;
+
+ if (tcs->num_tcs > MAX_TCS_PER_TYPE ||
+ st + tcs->num_tcs > max_tcs ||
+ st + tcs->num_tcs >= 8 * sizeof(tcs->tcs_mask))
+ return -EINVAL;
+
+ tcs->tcs_mask = ((1 << tcs->num_tcs) - 1) << st;
+ tcs->tcs_offset = st;
+ st += tcs->num_tcs;
+ }
+
+ drv->num_tcs = st;
+ drv->pdev = pdev;
+ INIT_LIST_HEAD(&drv->response_pending);
+ spin_lock_init(&drv->drv_lock);
+ tasklet_init(&drv->tasklet, tcs_notify_tx_done, (unsigned long)drv);
+
+ drv->name = of_get_property(pdev->dev.of_node, "label", NULL);
+ if (!drv->name)
+ drv->name = dev_name(&pdev->dev);
+
+ irq = of_irq_get(dn, 0);
+ if (irq < 0)
+ return irq;
+
+ ret = devm_request_irq(&pdev->dev, irq, tcs_irq_handler,
+ IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND, drv->name, drv);
+ if (ret)
+ return ret;
+
+ write_tcs_reg(drv->reg_base, RSC_DRV_IRQ_ENABLE, 0, 0,
+ drv->tcs[ACTIVE_TCS].tcs_mask);
+
+ for (i = 0; i < ARRAY_SIZE(drv->tcs_in_use); i++)
+ atomic_set(&drv->tcs_in_use[i], 0);
Perhaps do this before requesting the irq? If nothing else that saves
future readers from wondering if it makes any difference...
Ok. No difference to me.
+
+ dev_set_drvdata(&pdev->dev, drv);
This isn't used until the next patch, so move it there to clarify its
purpose.
OK.
+
+ return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+}
+
+static const struct of_device_id rpmh_drv_match[] = {
+ { .compatible = "qcom,rpmh-rsc", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, rpm_drv_match);
+
+static struct platform_driver rpmh_driver = {
+ .probe = rpmh_rsc_probe,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = rpmh_drv_match,
As we don't want to handle unbind, add:
.suppress_bind_attrs = true,
OK.
+ },
+};
+
diff --git a/include/dt-bindings/soc/qcom,rpmh-rsc.h b/include/dt-bindings/soc/qcom,rpmh-rsc.h
new file mode 100644
index 000000000000..24a56a5cf096
--- /dev/null
+++ b/include/dt-bindings/soc/qcom,rpmh-rsc.h
+struct tcs_cmd {
+ u32 addr; /* slv_id:18:16 | offset:0:15 */
+ u32 data; /* data for resource (or read response) */
+ bool complete; /* wait for completion before sending next */
+};
I think you should use kernel-doc to describe the structs.
Ok.
+
+enum rpmh_state {
+ RPMH_ACTIVE_ONLY_STATE, /* Active only (= AMC) */
+ RPMH_WAKE_ONLY_STATE, /* Wake only */
+ RPMH_SLEEP_STATE, /* Sleep */
+};
+
+struct tcs_mbox_msg {
struct tcs_request ?
Hmm.. ok
+ enum rpmh_state state; /* request state */
+ bool is_complete; /* wait for resp from accelerator */
+ u32 num_payload; /* Limited to MAX_RPMH_PAYLOAD in one msg */
+ struct tcs_cmd *payload;/* array of tcs_cmds */
+};
+
+#endif /* __SOC_QCOM_TCS_H__ */
Thanks for the review Bjorn.
-- Lina
--
To unsubscribe from this list: send the line "unsubscribe linux-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
