| Message ID | 20180119000157.7380-1-ilina@codeaurora.org |
|---|---|
| Headers | show |
| Series | drivers/qcom: add RPMH communication support | expand |
I wasn't Cc'd on the rest of the patches and this wasn't Cc'd to LKML, and I'm not on any of the mailing lists that were Cc'd, I gotta just look at what I have here in this patch. On Thu, 18 Jan 2018 17:01:56 -0700 Lina Iyer <ilina@codeaurora.org> wrote: > Log sent RPMH requests and interrupt responses in FTRACE. > > Cc: Steven Rostedt <rostedt@goodmis.org> > Signed-off-by: Lina Iyer <ilina@codeaurora.org> > --- > drivers/soc/qcom/rpmh-rsc.c | 13 ++++++- > include/trace/events/rpmh.h | 89 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 101 insertions(+), 1 deletion(-) > create mode 100644 include/trace/events/rpmh.h > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > index 3e68cef5513e..424dc939b2e6 100644 > --- a/drivers/soc/qcom/rpmh-rsc.c > +++ b/drivers/soc/qcom/rpmh-rsc.c > @@ -33,6 +33,9 @@ > > #include "rpmh-internal.h" > > +#define CREATE_TRACE_POINTS > +#include <trace/events/rpmh.h> > + > #define MAX_CMDS_PER_TCS 16 > #define MAX_TCS_PER_TYPE 3 > > @@ -325,6 +328,8 @@ static irqreturn_t tcs_irq_handler(int irq, void *p) > } > } > > + trace_rpmh_notify_irq(drv->name, m, resp->msg->payload[0].addr, > + resp->err); Below, m came from resp->m, is it the same here? > 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); > @@ -351,7 +356,8 @@ static void tcs_notify_tx_done(unsigned long data) > struct rsc_drv *drv = (struct rsc_drv *)data; > struct tcs_response *resp; > unsigned long flags; > - int err; > + int err, m; > + struct tcs_mbox_msg *msg; > > do { > spin_lock_irqsave(&drv->drv_lock, flags); > @@ -364,7 +370,10 @@ static void tcs_notify_tx_done(unsigned long data) > list_del(&resp->list); > spin_unlock_irqrestore(&drv->drv_lock, flags); > err = resp->err; > + m = resp->m; > + msg = resp->msg; > free_response(resp); > + trace_rpmh_notify(drv->name, m, msg->payload[0].addr, err); The reason I ask, is that this is causing more code to be executed even when tracing is off. What about passing in resp and assigning it within the tracepoint. trace_rpmh_notify(drv->name, resp); That would keep extra code from being used within the normal code path and only executed in the tracepoint. Get rid of the m = resp->m, msg = resp->msg. Even if m is something different above, you can still just pass in: trace_rpmh_notify(drv->name, m, resp); > } while (1); > } > > @@ -393,6 +402,8 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int m, int n, > write_tcs_reg(base, RSC_DRV_CMD_MSGID, m, n + i, msgid); > write_tcs_reg(base, RSC_DRV_CMD_ADDR, m, n + i, cmd->addr); > write_tcs_reg(base, RSC_DRV_CMD_DATA, m, n + i, cmd->data); > + trace_rpmh_send_msg(drv->name, m, n + i, msgid, cmd->addr, > + cmd->data, cmd->complete); Here just pass in cmd. The less the parameters of a tracepoints, the more likely gcc wont leak code that would be executed when tracing is off. > } > > write_tcs_reg(base, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0, cmd_complete); > diff --git a/include/trace/events/rpmh.h b/include/trace/events/rpmh.h > new file mode 100644 > index 000000000000..2cc44fc5ff95 > --- /dev/null > +++ b/include/trace/events/rpmh.h > @@ -0,0 +1,89 @@ > +/* 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. > + */ > + > +#undef TRACE_SYSTEM > +#define TRACE_SYSTEM rpmh > + > +#if !defined(_TRACE_RPMH_H) || defined(TRACE_HEADER_MULTI_READ) > +#define _TRACE_RPMH_H > + > +#include <linux/tracepoint.h> > + > +DECLARE_EVENT_CLASS(rpmh_ack_recvd, > + > + TP_PROTO(const char *s, int m, u32 addr, int errno), > + > + TP_ARGS(s, m, addr, errno), Then we could just do: TP_PROTO(const char *s, struct tcs_response *resp), TP_ARGS(s, resp), > + > + TP_STRUCT__entry( > + __field(const char *, name) > + __field(int, m) > + __field(u32, addr) > + __field(int, errno) > + ), > + > + TP_fast_assign( > + __entry->name = s; > + __entry->m = m; > + __entry->addr = addr; > + __entry->errno = errno; __entry->m = resp->m; __entry->addr = resp->msg->payload[0].addr; __entry->errno = resp->err; -- Steve > + ), > + > + TP_printk("%s: ack: tcs-m:%d addr: 0x%08x errno: %d", > + __entry->name, __entry->m, __entry->addr, __entry->errno) > +); > + > +DEFINE_EVENT(rpmh_ack_recvd, rpmh_notify_irq, > + TP_PROTO(const char *s, int m, u32 addr, int err), > + TP_ARGS(s, m, addr, err) > +); > + > +DEFINE_EVENT(rpmh_ack_recvd, rpmh_notify, > + TP_PROTO(const char *s, int m, u32 addr, int err), > + TP_ARGS(s, m, addr, err) > +); > + > +TRACE_EVENT(rpmh_send_msg, > + > + TP_PROTO(const char *s, int m, int n, u32 h, u32 a, u32 v, bool c), > + > + TP_ARGS(s, m, n, h, a, v, c), > + > + TP_STRUCT__entry( > + __field(const char*, name) > + __field(int, m) > + __field(int, n) > + __field(u32, hdr) > + __field(u32, addr) > + __field(u32, data) > + __field(bool, complete) > + ), > + > + TP_fast_assign( > + __entry->name = s; > + __entry->m = m; > + __entry->n = n; > + __entry->hdr = h; > + __entry->addr = a; > + __entry->data = v; > + __entry->complete = c; > + ), > + > + TP_printk("%s: send-msg: tcs(m): %d cmd(n): %d msgid: 0x%08x addr: 0x%08x data: 0x%08x complete: %d", > + __entry->name, __entry->m, __entry->n, __entry->hdr, > + __entry->addr, __entry->data, __entry->complete) > +); > + > +#endif /* _TRACE_RPMH_H */ > + > +#define TRACE_INCLUDE_FILE rpmh > +#include <trace/define_trace.h> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Steven, On Thu, Jan 18 2018 at 01:26 +0000, Steven Rostedt wrote: > >I wasn't Cc'd on the rest of the patches and this wasn't Cc'd to LKML, >and I'm not on any of the mailing lists that were Cc'd, I gotta just >look at what I have here in this patch. > I am so sorry. Will add LKML in the future. >On Thu, 18 Jan 2018 17:01:56 -0700 >Lina Iyer <ilina@codeaurora.org> wrote: > >> @@ -325,6 +328,8 @@ static irqreturn_t tcs_irq_handler(int irq, void *p) >> } >> } >> >> + trace_rpmh_notify_irq(drv->name, m, resp->msg->payload[0].addr, >> + resp->err); > >Below, m came from resp->m, is it the same here? > Yes, they will be the same. >> 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); >> @@ -351,7 +356,8 @@ static void tcs_notify_tx_done(unsigned long data) >> struct rsc_drv *drv = (struct rsc_drv *)data; >> struct tcs_response *resp; >> unsigned long flags; >> - int err; >> + int err, m; >> + struct tcs_mbox_msg *msg; >> >> do { >> spin_lock_irqsave(&drv->drv_lock, flags); >> @@ -364,7 +370,10 @@ static void tcs_notify_tx_done(unsigned long data) >> list_del(&resp->list); >> spin_unlock_irqrestore(&drv->drv_lock, flags); >> err = resp->err; >> + m = resp->m; >> + msg = resp->msg; >> free_response(resp); >> + trace_rpmh_notify(drv->name, m, msg->payload[0].addr, err); > >The reason I ask, is that this is causing more code to be executed >even when tracing is off. What about passing in resp and assigning it >within the tracepoint. > > trace_rpmh_notify(drv->name, resp); > The free_response(resp) will free the resp object and accessing resp->m is invalid after that. >That would keep extra code from being used within the normal code path >and only executed in the tracepoint. Get rid of the m = resp->m, >msg = resp->msg. > >Even if m is something different above, you can still just pass in: > > trace_rpmh_notify(drv->name, m, resp); > > >> } while (1); >> } May be I can just move the free_response() afer the trace_rpmh_notify(). >> >> @@ -393,6 +402,8 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int m, int n, >> write_tcs_reg(base, RSC_DRV_CMD_MSGID, m, n + i, msgid); >> write_tcs_reg(base, RSC_DRV_CMD_ADDR, m, n + i, cmd->addr); >> write_tcs_reg(base, RSC_DRV_CMD_DATA, m, n + i, cmd->data); >> + trace_rpmh_send_msg(drv->name, m, n + i, msgid, cmd->addr, >> + cmd->data, cmd->complete); > >Here just pass in cmd. The less the parameters of a tracepoints, the >more likely gcc wont leak code that would be executed when tracing is >off. > OK. >> +#include <linux/tracepoint.h> >> + >> +DECLARE_EVENT_CLASS(rpmh_ack_recvd, >> + >> + TP_PROTO(const char *s, int m, u32 addr, int errno), >> + >> + TP_ARGS(s, m, addr, errno), > >Then we could just do: > > TP_PROTO(const char *s, struct tcs_response *resp), > > TP_ARGS(s, resp), > Nice. Will use this. >> + >> + TP_STRUCT__entry( >> + __field(const char *, name) >> + __field(int, m) >> + __field(u32, addr) >> + __field(int, errno) >> + ), >> + >> + TP_fast_assign( >> + __entry->name = s; >> + __entry->m = m; >> + __entry->addr = addr; >> + __entry->errno = errno; > > __entry->m = resp->m; > __entry->addr = resp->msg->payload[0].addr; > __entry->errno = resp->err; > >-- Steve > Thanks for the review. -- Lina -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 19 Jan 2018 07:35:01 +0000 Lina Iyer <ilina@codeaurora.org> wrote: > Hi Steven, > > On Thu, Jan 18 2018 at 01:26 +0000, Steven Rostedt wrote: > > > >I wasn't Cc'd on the rest of the patches and this wasn't Cc'd to LKML, > >and I'm not on any of the mailing lists that were Cc'd, I gotta just > >look at what I have here in this patch. > > > I am so sorry. Will add LKML in the future. Thanks. It's just that I had little context to review the patch from. > >> struct rsc_drv *drv = (struct rsc_drv *)data; > >> struct tcs_response *resp; > >> unsigned long flags; > >> - int err; > >> + int err, m; > >> + struct tcs_mbox_msg *msg; > >> > >> do { > >> spin_lock_irqsave(&drv->drv_lock, flags); > >> @@ -364,7 +370,10 @@ static void tcs_notify_tx_done(unsigned long data) > >> list_del(&resp->list); > >> spin_unlock_irqrestore(&drv->drv_lock, flags); > >> err = resp->err; > >> + m = resp->m; > >> + msg = resp->msg; > >> free_response(resp); > >> + trace_rpmh_notify(drv->name, m, msg->payload[0].addr, err); > > > >The reason I ask, is that this is causing more code to be executed > >even when tracing is off. What about passing in resp and assigning it > >within the tracepoint. > > > > trace_rpmh_notify(drv->name, resp); > > > The free_response(resp) will free the resp object and accessing resp->m > is invalid after that. What about placing the trace before the free? -- Steve -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 19 2018 at 14:44 +0000, Steven Rostedt wrote: >On Fri, 19 Jan 2018 07:35:01 +0000 >Lina Iyer <ilina@codeaurora.org> wrote: > >> Hi Steven, >> >> On Thu, Jan 18 2018 at 01:26 +0000, Steven Rostedt wrote: >> > >> > trace_rpmh_notify(drv->name, resp); >> > >> The free_response(resp) will free the resp object and accessing resp->m >> is invalid after that. > >What about placing the trace before the free? > Sure. Will do that. -- Lina -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 18 Jan 16:01 PST 2018, Lina Iyer wrote: > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile > index b7951ce87663..0dba46387f1c 100644 > --- a/drivers/soc/qcom/Makefile > +++ b/drivers/soc/qcom/Makefile > @@ -11,4 +11,4 @@ obj-$(CONFIG_QCOM_SMP2P) += smp2p.o > obj-$(CONFIG_QCOM_SMSM) += smsm.o > obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o > obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o > -obj-$(CONFIG_QCOM_RPMH) += rpmh-rsc.o > +obj-$(CONFIG_QCOM_RPMH) += rpmh-rsc.o rpmh.o I think it would be better if you built these two objects into the same module; obj-$(CONFIG_QCOM_RPMH) += qcom_rpmh.o qcom_rpmh-y += rpmh-rsc.o qcom_rpmh-y += rpmh.o > diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h [..] > diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c [..] > +#define RPMH_TIMEOUT msecs_to_jiffies(10000) 10 * HZ > + > +#define DEFINE_RPMH_MSG_ONSTACK(rc, s, q, c, name) \ > + struct rpmh_msg name = { \ > + .msg = { \ > + .state = s, \ > + .payload = name.cmd, \ > + .num_payload = 0, \ > + .is_complete = true, \ > + .invalidate = false, \ > + }, \ > + .cmd = { { 0 } }, \ > + .completion = q, \ > + .wait_count = c, \ > + .rc = rc, \ > + } > + > +/** > + * rpmh_msg: the message to be sent to rpmh-rsc > + * > + * @msg: the request > + * @cmd: the payload that will be part of the @msg > + * @completion: triggered when request is done > + * @wait_count: count of waiters for this completion > + * @err: err return from the controller > + */ > +struct rpmh_msg { struct rpmh_request ? > + struct tcs_mbox_msg msg; > + struct tcs_cmd cmd[MAX_RPMH_PAYLOAD]; > + struct completion *completion; > + atomic_t *wait_count; When will @wait_count > 1? As far as I can see the only purpose would be to be able to control whether you should complete @completion 0 or N times; but 0 times is covered already by not specifying a @completion. > + struct rpmh_client *rc; > + int err; > +}; > + > +/** > + * rpmh_ctrlr: our representation of the controller > + * > + * @drv: the controller instance > + */ > +struct rpmh_ctrlr { > + struct rsc_drv *drv; Is this going to grow in the future? Otherwise just drop it and reference the rsc_drv directly. (Even if it's growing it might be cleaner to introduce it at that point) > +}; > + > +/** > + * rpmh_client: the client object > + * > + * @dev: the platform device that is the owner > + * @ctrlr: the controller associated with this client. > + */ > +struct rpmh_client { > + struct device *dev; > + struct rpmh_ctrlr *ctrlr; > +}; > + > +static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_MBOXES]; > +static DEFINE_MUTEX(rpmh_ctrlr_mutex); The client needs a reference to the rsc_drv, using the rpmh_ctrlr abstraction and these global variables only seems to add unnecessary complexity. > + > +void rpmh_tx_done(struct tcs_mbox_msg *msg, int r) > +{ > + struct rpmh_msg *rpm_msg = container_of(msg, struct rpmh_msg, msg); > + atomic_t *wc = rpm_msg->wait_count; > + struct completion *compl = rpm_msg->completion; > + > + rpm_msg->err = r; > + > + if (r) > + dev_err(rpm_msg->rc->dev, > + "RPMH TX fail in msg addr 0x%x, err=%d\n", > + rpm_msg->msg.payload[0].addr, r); > + > + /* Signal the blocking thread we are done */ > + if (wc && atomic_dec_and_test(wc)) > + if (compl) > + complete(compl); I think that you should drop this function and just complete @rpm_msg->completion in the rcs driver. > +} > +EXPORT_SYMBOL(rpmh_tx_done); > + > +/** > + * wait_for_tx_done: Wait until the response is received. > + * > + * @rc: The RPMH client > + * @compl: The completion object > + * @addr: An addr that we sent in that request > + * @data: The data for the address in that request > + * > + */ > +static inline int wait_for_tx_done(struct rpmh_client *rc, > + struct completion *compl, u32 addr, u32 data) > +{ > + int ret; > + > + ret = wait_for_completion_timeout(compl, RPMH_TIMEOUT); > + if (ret) > + dev_dbg(rc->dev, > + "RPMH response received addr=0x%x data=0x%x\n", > + addr, data); > + else > + dev_err(rc->dev, > + "RPMH response timeout addr=0x%x data=0x%x\n", > + addr, data); The request can contain a number of commands and these error messages ends up printing the first one, on behalf of the client. I suspect that this isn't useful enough in most cases, causing the client to print another time. I therefor suggest that you omit these prints. > + > + return (ret > 0) ? 0 : -ETIMEDOUT; > +} > + > +/** > + * __rpmh_write: send the RPMH request > + * > + * @rc: The RPMH client > + * @state: Active/Sleep request type > + * @rpm_msg: The data that needs to be sent (payload). > + */ > +int __rpmh_write(struct rpmh_client *rc, enum rpmh_state state, > + struct rpmh_msg *rpm_msg) > +{ > + int ret = -EFAULT; > + > + rpm_msg->msg.state = state; > + > + if (state == RPMH_ACTIVE_ONLY_STATE) { > + WARN_ON(irqs_disabled()); > + ret = rpmh_rsc_send_data(rc->ctrlr->drv, &rpm_msg->msg); > + if (!ret) > + dev_dbg(rc->dev, > + "RPMH request sent addr=0x%x, data=0x%x\n", > + rpm_msg->msg.payload[0].addr, > + rpm_msg->msg.payload[0].data); > + else > + dev_warn(rc->dev, > + "Error in RPMH request addr=0x%x, data=0x%x\n", > + rpm_msg->msg.payload[0].addr, > + rpm_msg->msg.payload[0].data); Same thing here, for the user to be able to make sense of this error the client will have to print something with more context. So I think you should omit these too. tracing failing addr/data pairs might make sense though! > + } > + > + return ret; > +} > + > +/** > + * rpmh_write: Write a set of RPMH commands and block until response > + * > + * @rc: The RPMh handle got from rpmh_get_dev_channel > + * @state: Active/sleep set > + * @cmd: The payload data > + * @n: The number of elements in payload > + * > + * May sleep. Do not call from atomic contexts. > + */ > +int rpmh_write(struct rpmh_client *rc, enum rpmh_state state, > + struct tcs_cmd *cmd, int n) > +{ > + DECLARE_COMPLETION_ONSTACK(compl); > + atomic_t wait_count = ATOMIC_INIT(1); > + DEFINE_RPMH_MSG_ONSTACK(rc, state, &compl, &wait_count, rpm_msg); > + int ret; > + > + if (IS_ERR_OR_NULL(rc) || !cmd || n <= 0 || n > MAX_RPMH_PAYLOAD) > + return -EINVAL; > + > + might_sleep(); > + > + memcpy(rpm_msg.cmd, cmd, n * sizeof(*cmd)); > + rpm_msg.msg.num_payload = n; > + > + ret = __rpmh_write(rc, state, &rpm_msg); > + if (ret) > + return ret; > + > + return wait_for_tx_done(rc, &compl, cmd[0].addr, cmd[0].data); As you're returning here the completion object on the stack will be trash, so you must inform rpmh_rsc that it may no longer complete it. I suggest that you provide two functions in the rsc driver; rpmh_rsc_send_sync() and rpmh_rsc_send_async(), and move the wait-for-completion into the sync one. Also make the sync one return the msg->err (and drop the tx_done cross-call). (Or call them rpmh_rsc_send_wait() and rpmh_rsc_send_nowait()) > +} > +EXPORT_SYMBOL(rpmh_write); > + > +static struct rpmh_ctrlr *get_rpmh_ctrlr(struct platform_device *pdev) > +{ > + int i; > + struct rsc_drv *drv = dev_get_drvdata(pdev->dev.parent); > + struct rpmh_ctrlr *ctrlr = ERR_PTR(-EFAULT); > + > + if (!drv) > + return ctrlr; > + > + mutex_lock(&rpmh_ctrlr_mutex); > + for (i = 0; i < RPMH_MAX_MBOXES; i++) > + if (rpmh_rsc[i].drv == drv) { > + ctrlr = &rpmh_rsc[i]; > + goto unlock; > + } > + > + if (i == RPMH_MAX_MBOXES) > + for (i = 0; i < RPMH_MAX_MBOXES; i++) > + if (rpmh_rsc[i].drv == NULL) { > + ctrlr = &rpmh_rsc[i]; > + ctrlr->drv = drv; > + break; > + } I fail to see the reason for tracking rsc_drv references in a global array and try to find an existing one here. Just return the rsc_drv acquired from dev_get_drvdata() to the caller. > +unlock: > + mutex_unlock(&rpmh_ctrlr_mutex); > + return ctrlr; > +} > + > +/** > + * rpmh_get_client: Get the RPMh handle > + * > + * @pdev: the platform device which needs to communicate with RPM > + * accelerators > + * May sleep. > + */ > +struct rpmh_client *rpmh_get_client(struct platform_device *pdev) To make this analog to previous rpm drivers I think you should take the device * of the parent here. I.e. client does: rpmh = rpmh_get_client(&pdev->dev.parent). I recognize that this removes the possibility of providing error messages indicating which client caused the fault, but by above suggestions these functions would be moved into the rsc driver; or the error would be propagated to the client which could print these themselves. > +{ > + struct rpmh_client *rc; > + > + rc = kzalloc(sizeof(*rc), GFP_KERNEL); > + if (!rc) > + return ERR_PTR(-ENOMEM); > + > + rc->dev = &pdev->dev; > + rc->ctrlr = get_rpmh_ctrlr(pdev); > + if (IS_ERR(rc->ctrlr)) { > + kfree(rc); > + return ERR_PTR(-EFAULT); > + } > + > + return rc; > +} > +EXPORT_SYMBOL(rpmh_get_client); > + > +/** > + * rpmh_release: Release the RPMH client > + * > + * @rc: The RPMh handle to be freed. > + */ > +void rpmh_release(struct rpmh_client *rc) > +{ > + kfree(rc); If you reduce above function to just return a rsc_drv reference you don't even need a release function, which simplifies clients further. > +} > +EXPORT_SYMBOL(rpmh_release); > diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h [..] > +struct rpmh_client; > + > +#ifdef CONFIG_QCOM_RPMH I think it would be fine to just make clients depend on QCOM_RPMH. If you would prefer to get the compile testing in those drivers then make this: #if IS_ENABLED(CONFIG_QCOM_RPMH) In case someone in the future decides to make RPMH tristate. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 05 2018 at 19:50 +0000, Bjorn Andersson wrote: >On Thu 18 Jan 16:01 PST 2018, Lina Iyer wrote: >> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile >> index b7951ce87663..0dba46387f1c 100644 >> --- a/drivers/soc/qcom/Makefile >> +++ b/drivers/soc/qcom/Makefile >> @@ -11,4 +11,4 @@ obj-$(CONFIG_QCOM_SMP2P) += smp2p.o >> obj-$(CONFIG_QCOM_SMSM) += smsm.o >> obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o >> obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o >> -obj-$(CONFIG_QCOM_RPMH) += rpmh-rsc.o >> +obj-$(CONFIG_QCOM_RPMH) += rpmh-rsc.o rpmh.o > >I think it would be better if you built these two objects into the same >module; > >obj-$(CONFIG_QCOM_RPMH) += qcom_rpmh.o >qcom_rpmh-y += rpmh-rsc.o >qcom_rpmh-y += rpmh.o > Curious, how this would be better? >> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h >[..] >> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c >[..] >> +#define RPMH_TIMEOUT msecs_to_jiffies(10000) > >10 * HZ > >> + >> +#define DEFINE_RPMH_MSG_ONSTACK(rc, s, q, c, name) \ >> + struct rpmh_msg name = { \ >> + .msg = { \ >> + .state = s, \ >> + .payload = name.cmd, \ >> + .num_payload = 0, \ >> + .is_complete = true, \ >> + .invalidate = false, \ >> + }, \ >> + .cmd = { { 0 } }, \ >> + .completion = q, \ >> + .wait_count = c, \ >> + .rc = rc, \ >> + } >> + >> +/** >> + * rpmh_msg: the message to be sent to rpmh-rsc >> + * >> + * @msg: the request >> + * @cmd: the payload that will be part of the @msg >> + * @completion: triggered when request is done >> + * @wait_count: count of waiters for this completion >> + * @err: err return from the controller >> + */ >> +struct rpmh_msg { > >struct rpmh_request ? > Hmm.. ok. >> + struct tcs_mbox_msg msg; >> + struct tcs_cmd cmd[MAX_RPMH_PAYLOAD]; >> + struct completion *completion; >> + atomic_t *wait_count; > >When will @wait_count > 1? As far as I can see the only purpose would be >to be able to control whether you should complete @completion 0 or N >times; but 0 times is covered already by not specifying a @completion. > There is a patch that I haven't posted in this series. It sends a batch of requests instead of just 1. The wait count is equal to the number of requests in that batch. Sorry it is a bit ahead of its real use. Adding it later, increases unnecessary changes in the patches. >> + struct rpmh_client *rc; >> + int err; >> +}; >> + >> +/** >> + * rpmh_ctrlr: our representation of the controller >> + * >> + * @drv: the controller instance >> + */ >> +struct rpmh_ctrlr { >> + struct rsc_drv *drv; > >Is this going to grow in the future? Otherwise just drop it and >reference the rsc_drv directly. (Even if it's growing it might be >cleaner to introduce it at that point) > Will grow :) >> +}; >> + >> +/** >> + * rpmh_client: the client object >> + * >> + * @dev: the platform device that is the owner >> + * @ctrlr: the controller associated with this client. >> + */ >> +struct rpmh_client { >> + struct device *dev; >> + struct rpmh_ctrlr *ctrlr; >> +}; >> + >> +static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_MBOXES]; >> +static DEFINE_MUTEX(rpmh_ctrlr_mutex); > >The client needs a reference to the rsc_drv, using the rpmh_ctrlr >abstraction and these global variables only seems to add unnecessary >complexity. > There would be more functionality that needs these as structures and a list of RSCs and therefore the global mutex and array. Please bear with me on this. >> + >> +void rpmh_tx_done(struct tcs_mbox_msg *msg, int r) >> +{ >> + struct rpmh_msg *rpm_msg = container_of(msg, struct rpmh_msg, msg); >> + atomic_t *wc = rpm_msg->wait_count; >> + struct completion *compl = rpm_msg->completion; >> + >> + rpm_msg->err = r; >> + >> + if (r) >> + dev_err(rpm_msg->rc->dev, >> + "RPMH TX fail in msg addr 0x%x, err=%d\n", >> + rpm_msg->msg.payload[0].addr, r); >> + >> + /* Signal the blocking thread we are done */ >> + if (wc && atomic_dec_and_test(wc)) >> + if (compl) >> + complete(compl); > >I think that you should drop this function and just complete >@rpm_msg->completion in the rcs driver. > I am not sure how much of it I can upstream. Downstream I add more functionality to this function to make it easier to debug on production. If its not a bother I would like to keep it as is. Who knows, may be I can get the debug code upstream. >> +} >> +EXPORT_SYMBOL(rpmh_tx_done); >> + >> +/** >> + * wait_for_tx_done: Wait until the response is received. >> + * >> + * @rc: The RPMH client >> + * @compl: The completion object >> + * @addr: An addr that we sent in that request >> + * @data: The data for the address in that request >> + * >> + */ >> +static inline int wait_for_tx_done(struct rpmh_client *rc, >> + struct completion *compl, u32 addr, u32 data) >> +{ >> + int ret; >> + >> + ret = wait_for_completion_timeout(compl, RPMH_TIMEOUT); >> + if (ret) >> + dev_dbg(rc->dev, >> + "RPMH response received addr=0x%x data=0x%x\n", >> + addr, data); >> + else >> + dev_err(rc->dev, >> + "RPMH response timeout addr=0x%x data=0x%x\n", >> + addr, data); > >The request can contain a number of commands and these error messages >ends up printing the first one, on behalf of the client. I suspect that >this isn't useful enough in most cases, causing the client to print >another time. > >I therefor suggest that you omit these prints. > On the contrary, just the first address in the request is quite sufficient to know what request failed. The rest of the requests in dumped in the FTRACE while sending the request. That is enough to know what was sent and correlate with what failed. >> + >> + return (ret > 0) ? 0 : -ETIMEDOUT; >> +} >> + >> +/** >> + * __rpmh_write: send the RPMH request >> + * >> + * @rc: The RPMH client >> + * @state: Active/Sleep request type >> + * @rpm_msg: The data that needs to be sent (payload). >> + */ >> +int __rpmh_write(struct rpmh_client *rc, enum rpmh_state state, >> + struct rpmh_msg *rpm_msg) >> +{ >> + int ret = -EFAULT; >> + >> + rpm_msg->msg.state = state; >> + >> + if (state == RPMH_ACTIVE_ONLY_STATE) { >> + WARN_ON(irqs_disabled()); >> + ret = rpmh_rsc_send_data(rc->ctrlr->drv, &rpm_msg->msg); >> + if (!ret) >> + dev_dbg(rc->dev, >> + "RPMH request sent addr=0x%x, data=0x%x\n", >> + rpm_msg->msg.payload[0].addr, >> + rpm_msg->msg.payload[0].data); >> + else >> + dev_warn(rc->dev, >> + "Error in RPMH request addr=0x%x, data=0x%x\n", >> + rpm_msg->msg.payload[0].addr, >> + rpm_msg->msg.payload[0].data); > >Same thing here, for the user to be able to make sense of this error the >client will have to print something with more context. So I think you >should omit these too. > > >tracing failing addr/data pairs might make sense though! > See explanation above. >> + } >> + >> + return ret; >> +} >> + >> +/** >> + * rpmh_write: Write a set of RPMH commands and block until response >> + * >> + * @rc: The RPMh handle got from rpmh_get_dev_channel >> + * @state: Active/sleep set >> + * @cmd: The payload data >> + * @n: The number of elements in payload >> + * >> + * May sleep. Do not call from atomic contexts. >> + */ >> +int rpmh_write(struct rpmh_client *rc, enum rpmh_state state, >> + struct tcs_cmd *cmd, int n) >> +{ >> + DECLARE_COMPLETION_ONSTACK(compl); >> + atomic_t wait_count = ATOMIC_INIT(1); >> + DEFINE_RPMH_MSG_ONSTACK(rc, state, &compl, &wait_count, rpm_msg); >> + int ret; >> + >> + if (IS_ERR_OR_NULL(rc) || !cmd || n <= 0 || n > MAX_RPMH_PAYLOAD) >> + return -EINVAL; >> + >> + might_sleep(); >> + >> + memcpy(rpm_msg.cmd, cmd, n * sizeof(*cmd)); >> + rpm_msg.msg.num_payload = n; >> + >> + ret = __rpmh_write(rc, state, &rpm_msg); >> + if (ret) >> + return ret; >> + >> + return wait_for_tx_done(rc, &compl, cmd[0].addr, cmd[0].data); > >As you're returning here the completion object on the stack will be >trash, so you must inform rpmh_rsc that it may no longer complete it. > >I suggest that you provide two functions in the rsc driver; >rpmh_rsc_send_sync() and rpmh_rsc_send_async(), and move the >wait-for-completion into the sync one. Also make the sync one return the >msg->err (and drop the tx_done cross-call). > >(Or call them rpmh_rsc_send_wait() and rpmh_rsc_send_nowait()) > This is the sync variant. The async variant follows this patch. The wait_for_tx_done() is blocking and the compl object will not be trashed until the completion is completed or it fails. >> +} >> +EXPORT_SYMBOL(rpmh_write); >> + >> +static struct rpmh_ctrlr *get_rpmh_ctrlr(struct platform_device *pdev) >> +{ >> + int i; >> + struct rsc_drv *drv = dev_get_drvdata(pdev->dev.parent); >> + struct rpmh_ctrlr *ctrlr = ERR_PTR(-EFAULT); >> + >> + if (!drv) >> + return ctrlr; >> + >> + mutex_lock(&rpmh_ctrlr_mutex); >> + for (i = 0; i < RPMH_MAX_MBOXES; i++) >> + if (rpmh_rsc[i].drv == drv) { >> + ctrlr = &rpmh_rsc[i]; >> + goto unlock; >> + } >> + >> + if (i == RPMH_MAX_MBOXES) >> + for (i = 0; i < RPMH_MAX_MBOXES; i++) >> + if (rpmh_rsc[i].drv == NULL) { >> + ctrlr = &rpmh_rsc[i]; >> + ctrlr->drv = drv; >> + break; >> + } > >I fail to see the reason for tracking rsc_drv references in a global >array and try to find an existing one here. Just return the rsc_drv >acquired from dev_get_drvdata() to the caller. > There are multiple RSCs and clients would refer to one of them. Future patches add information to the RSC as dictated by the client (display driver is using the RSC) and therefore take action accordingly. >> +unlock: >> + mutex_unlock(&rpmh_ctrlr_mutex); >> + return ctrlr; >> +} >> + >> +/** >> + * rpmh_get_client: Get the RPMh handle >> + * >> + * @pdev: the platform device which needs to communicate with RPM >> + * accelerators >> + * May sleep. >> + */ >> +struct rpmh_client *rpmh_get_client(struct platform_device *pdev) > >To make this analog to previous rpm drivers I think you should take the >device * of the parent here. I.e. client does: > > rpmh = rpmh_get_client(&pdev->dev.parent). > > >I recognize that this removes the possibility of providing error >messages indicating which client caused the fault, but by above >suggestions these functions would be moved into the rsc driver; or the >error would be propagated to the client which could print these >themselves. > I don't see a particular reason to be analogous with RPM. But, let me think about it. >> +{ >> + struct rpmh_client *rc; >> + >> + rc = kzalloc(sizeof(*rc), GFP_KERNEL); >> + if (!rc) >> + return ERR_PTR(-ENOMEM); >> + >> + rc->dev = &pdev->dev; >> + rc->ctrlr = get_rpmh_ctrlr(pdev); >> + if (IS_ERR(rc->ctrlr)) { >> + kfree(rc); >> + return ERR_PTR(-EFAULT); >> + } >> + >> + return rc; >> +} >> +EXPORT_SYMBOL(rpmh_get_client); >> + >> +/** >> + * rpmh_release: Release the RPMH client >> + * >> + * @rc: The RPMh handle to be freed. >> + */ >> +void rpmh_release(struct rpmh_client *rc) >> +{ >> + kfree(rc); > >If you reduce above function to just return a rsc_drv reference you >don't even need a release function, which simplifies clients further. > Hmm.. not sure I understand. I need to release the allocated memory. >> +} >> +EXPORT_SYMBOL(rpmh_release); >> diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h >[..] >> +struct rpmh_client; >> + >> +#ifdef CONFIG_QCOM_RPMH > >I think it would be fine to just make clients depend on QCOM_RPMH. If >you would prefer to get the compile testing in those drivers then make >this: > >#if IS_ENABLED(CONFIG_QCOM_RPMH) > >In case someone in the future decides to make RPMH tristate. > OK. Thanks, Lina -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[].. > diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c > new file mode 100644 > index 000000000000..ad1def2c362c > --- /dev/null > +++ b/drivers/soc/qcom/rpmh.c > @@ -0,0 +1,264 @@ > +/* 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. > + * > + */ > + > +#include <linux/atomic.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/mailbox_client.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/types.h> > +#include <linux/wait.h> > + > +#include <soc/qcom/rpmh.h> > + > +#include "rpmh-internal.h" > + > +#define RPMH_MAX_MBOXES 2 > +#define RPMH_TIMEOUT msecs_to_jiffies(10000) > + > +#define DEFINE_RPMH_MSG_ONSTACK(rc, s, q, c, name) > + struct rpmh_msg name = { \ > + .msg = { \ > + .state = s, \ > + .payload = name.cmd, \ > + .num_payload = 0, \ > + .is_complete = true, \ > + .invalidate = false, \ There seems to be no field called 'invalidate' as part of the struct
On Thu, Feb 15 2018 at 08:49 +0000, Rajendra Nayak wrote: >[].. > >> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c >> new file mode 100644 >> index 000000000000..ad1def2c362c >> --- /dev/null >> +++ b/drivers/soc/qcom/rpmh.c >> @@ -0,0 +1,264 @@ >> +/* 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. >> + * >> + */ >> + >> +#include <linux/atomic.h> >> +#include <linux/interrupt.h> >> +#include <linux/kernel.h> >> +#include <linux/mailbox_client.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> +#include <linux/types.h> >> +#include <linux/wait.h> >> + >> +#include <soc/qcom/rpmh.h> >> + >> +#include "rpmh-internal.h" >> + >> +#define RPMH_MAX_MBOXES 2 >> +#define RPMH_TIMEOUT msecs_to_jiffies(10000) >> + >> +#define DEFINE_RPMH_MSG_ONSTACK(rc, s, q, c, name) >> + struct rpmh_msg name = { \ >> + .msg = { \ >> + .state = s, \ >> + .payload = name.cmd, \ >> + .num_payload = 0, \ >> + .is_complete = true, \ >> + .invalidate = false, \ > >There seems to be no field called 'invalidate' as part of the struct > Yup. Sorry, I will remove it in the next revision. I should be posting one in a day or so. Thanks, Lina -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html