Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions
On Fri, May 11 2018 at 14:14 -0600, Doug Anderson wrote: Hi, On Fri, May 11, 2018 at 8:06 AM, Lina Iyer wrote: As I've said I haven't reviewed RPMh in any amount of detail and so perhaps I don't understand something. OK, I dug a little more and coded up something for you. Basically you're doing a whole bunch of iteration / extra work here to try to come up with a way to associate an extra bit of data with each "struct rsc_drv". Rather than that, just add an extra field into "struct rsc_drv". Problem solved. See http://crosreview.com/1054646 for what I mean. I tried to avoid such pointer references and keep it object oriented with this approach. I agree that we run through a list of 2 (at the max) RSC to get the drv* from the rpmh_ctrlr. It is not going to be expensive. Even if you wanted to keep things "object oriented" then IMHO your code still should change. Sure, it's not computationally expensive to iterate through this list, but it adds an extra level of complexity that doesn't seem justified. If you _really_ needed an abstraction barrier then at least add a "void *client_data" to "struct rsc_drv.c". At least you'd get rid of the ugly global list and store your pointer directly in the correct structure rather than creating an external entity. Now it becomes 100% obvious that there is exactly 1 "struct rpmh_ctrlr" for each controller. ...but IMO there's enough intertwining between "rpmh.c" and "rpmh-rsc.c" that it would just be a waste because now you'd need to do extra memory allocation and freeing. ...and if you just allocated the pointer in get_rpmh_ctrlr() it would also seem non-ideal because this one-time allocation (that affects _all_ RPMh clients) happens whenever one client happens to do the first access. This is one-time init so it makes sense to do it at init time. I say that there's intertwining between "rpmh.c" and "rpmh-rsc.c" because both C files call directly into one another and have intimate knowledge of how the other works. They aren't really separate things. Specifically I see that "rpmh-rsc" directly calls into "rpmh.c" when it calls rpmh_tx_done(), and of coruse "rpmh-rsc.c" directly calls into "rpmh.c". OK, so I've been browsing through the source code more so I can be a little more informed here. As far as I can tell "rpmh.c"'s goal is: 1. Put a simpler API atop "rpmh-rsc.c". ...but why not just put that API directly into "rpmh-rsc.c" anyway? If there was someone that needed the more complex API then having a "simpler" wrapper makes sense, but that's not the case, is it? 2. Add caching atop "rpmh-rsc" I'll respond to some of your other patches too, but I think that the amount of code for caching is not very much. I don't see the benefit of trying to split the code into two files. Put them into one and then delete all the extra code you needed just the try to maintain some abstraction. Another things this helps with is that, if the driver is not a child of the RSC nodes in DT, then the drvdata of the parent would not a RSC node and accessing that would result in a crash. This offers a cleaner exit path for the error case. Why would the driver not be a child of the RSC nodes? That's kinda like saying "if you try to instantiate an i2c device as a platform device then its probe will crash". Yeah, it will. Doctor, it hurts if I poke myself in my eye with this sharp stick, do you have any medicine that can help fix that? I'll try to dig into this more so I could just be confused, but in general it seems really odd to have a spinlock and something called a "cache" at this level. If we need some sort of mutual exclusion or caching it seems like it should be stored in memory directly associated with the RPMh device, not some external global. The idea behind the locking is not to avoid the race between rpmh.c and rpmh-rsc.c. From the DT, the devices that are dependent on the RSCs are probed following the probe of the controller. And init is not that we are worried about. The condition here is to prevent the rpmh_rsc[] from being modified concurrently by drivers. OK, I see the point of the locking now, but not the list still. Sounds like Matthias agrees with me that the list isn't useful. Seems like you should squash my patch at http://crosreview.com/1042883 into yours. I saw your approach. I am okay with it for your tree, I'm not okay with it for the Chrome OS tree. We need to match upstream, not fork for style reasons. I'd rather take a driver that I think it overly complex but matches upstream than a private driver. my approach comes out of experiences in qcom platforms and how things tend to shape up in the future. I would want you to consider my reasoning as well, before we go forward. I suppose we can get advice from others who have worked in qcom platforms and see what they think. My opinions come out of years of experience working with Linux drivers, so I guess we both have some experience under our belts that we're try
Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions
Hi, On Fri, May 11, 2018 at 8:06 AM, Lina Iyer wrote: >> As I've said I haven't reviewed RPMh in any amount of detail and so >> perhaps I don't understand something. >> >> OK, I dug a little more and coded up something for you. Basically >> you're doing a whole bunch of iteration / extra work here to try to >> come up with a way to associate an extra bit of data with each "struct >> rsc_drv". Rather than that, just add an extra field into "struct >> rsc_drv". Problem solved. >> >> See http://crosreview.com/1054646 for what I mean. >> > I tried to avoid such pointer references and keep it object oriented > with this approach. I agree that we run through a list of 2 (at the max) > RSC to get the drv* from the rpmh_ctrlr. It is not going to be > expensive. Even if you wanted to keep things "object oriented" then IMHO your code still should change. Sure, it's not computationally expensive to iterate through this list, but it adds an extra level of complexity that doesn't seem justified. If you _really_ needed an abstraction barrier then at least add a "void *client_data" to "struct rsc_drv.c". At least you'd get rid of the ugly global list and store your pointer directly in the correct structure rather than creating an external entity. Now it becomes 100% obvious that there is exactly 1 "struct rpmh_ctrlr" for each controller. ...but IMO there's enough intertwining between "rpmh.c" and "rpmh-rsc.c" that it would just be a waste because now you'd need to do extra memory allocation and freeing. ...and if you just allocated the pointer in get_rpmh_ctrlr() it would also seem non-ideal because this one-time allocation (that affects _all_ RPMh clients) happens whenever one client happens to do the first access. This is one-time init so it makes sense to do it at init time. I say that there's intertwining between "rpmh.c" and "rpmh-rsc.c" because both C files call directly into one another and have intimate knowledge of how the other works. They aren't really separate things. Specifically I see that "rpmh-rsc" directly calls into "rpmh.c" when it calls rpmh_tx_done(), and of coruse "rpmh-rsc.c" directly calls into "rpmh.c". OK, so I've been browsing through the source code more so I can be a little more informed here. As far as I can tell "rpmh.c"'s goal is: 1. Put a simpler API atop "rpmh-rsc.c". ...but why not just put that API directly into "rpmh-rsc.c" anyway? If there was someone that needed the more complex API then having a "simpler" wrapper makes sense, but that's not the case, is it? 2. Add caching atop "rpmh-rsc" I'll respond to some of your other patches too, but I think that the amount of code for caching is not very much. I don't see the benefit of trying to split the code into two files. Put them into one and then delete all the extra code you needed just the try to maintain some abstraction. > Another things this helps with is that, if the driver is not a child of > the RSC nodes in DT, then the drvdata of the parent would not a RSC node > and accessing that would result in a crash. This offers a cleaner exit > path for the error case. Why would the driver not be a child of the RSC nodes? That's kinda like saying "if you try to instantiate an i2c device as a platform device then its probe will crash". Yeah, it will. Doctor, it hurts if I poke myself in my eye with this sharp stick, do you have any medicine that can help fix that? I'll try to dig into this more so I could just be confused, but in general it seems really odd to have a spinlock and something called a "cache" at this level. If we need some sort of mutual exclusion or caching it seems like it should be stored in memory directly associated with the RPMh device, not some external global. >>> The idea behind the locking is not to avoid the race between rpmh.c and >>> rpmh-rsc.c. From the DT, the devices that are dependent on the RSCs are >>> probed following the probe of the controller. And init is not that we are >>> worried about. >>> The condition here is to prevent the rpmh_rsc[] from being modified >>> concurrently by drivers. >> >> >> OK, I see the point of the locking now, but not the list still. >> Sounds like Matthias agrees with me that the list isn't useful. Seems >> like you should squash my patch at http://crosreview.com/1042883 into >> yours. >> > I saw your approach. I am okay with it for your tree, I'm not okay with it for the Chrome OS tree. We need to match upstream, not fork for style reasons. I'd rather take a driver that I think it overly complex but matches upstream than a private driver. > my approach comes > out of experiences in qcom platforms and how things tend to shape up in > the future. I would want you to consider my reasoning as well, before we > go forward. I suppose we can get advice from others who have worked in qcom platforms and see what they think. My opinions come out of years of experience working with Linux drivers, so I guess we
Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions
Hi Doug,
On Thu, May 10 2018 at 16:37 -0600, Doug Anderson wrote:
Hi,
On Tue, May 8, 2018 at 9:05 AM, wrote:
On 2018-05-03 14:26, Doug Anderson wrote:
Hi Doug,
Hi,
On Wed, May 2, 2018 at 12:37 PM, Lina Iyer wrote:
+static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
+static DEFINE_SPINLOCK(rpmh_rsc_lock);
+
+static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
+{
+ int i;
+ struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
+ struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
+ unsigned long flags;
+
+ if (!drv)
+ return ctrlr;
+
+ for (i = 0; i < RPMH_MAX_CTRLR; i++) {
+ if (rpmh_rsc[i].drv == drv) {
+ ctrlr = &rpmh_rsc[i];
+ return ctrlr;
+ }
+ }
+
+ spin_lock_irqsave(&rpmh_rsc_lock, flags);
+ list_for_each_entry(p, &rsc_drv_list, list) {
+ if (drv == p) {
+ for (i = 0; i < RPMH_MAX_CTRLR; i++) {
+ if (!rpmh_rsc[i].drv)
+ break;
+ }
+ if (i == RPMH_MAX_CTRLR) {
+ ctrlr = ERR_PTR(-ENOMEM);
+ break;
+ }
+ rpmh_rsc[i].drv = drv;
+ ctrlr = &rpmh_rsc[i];
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&rpmh_rsc_lock, flags);
I may have missed something, but to me it appears that this whole
"rsc_drv_list" is pretty pointless. I wrote up a patch atop your
series to remove it at
Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions
Hi,
On Tue, May 8, 2018 at 9:05 AM, wrote:
> On 2018-05-03 14:26, Doug Anderson wrote:
> Hi Doug,
>
>
>> Hi,
>>
>> On Wed, May 2, 2018 at 12:37 PM, Lina Iyer wrote:
>>>
>>> +static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
>>> +static DEFINE_SPINLOCK(rpmh_rsc_lock);
>>> +
>>> +static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
>>> +{
>>> + int i;
>>> + struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
>>> + struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
>>> + unsigned long flags;
>>> +
>>> + if (!drv)
>>> + return ctrlr;
>>> +
>>> + for (i = 0; i < RPMH_MAX_CTRLR; i++) {
>>> + if (rpmh_rsc[i].drv == drv) {
>>> + ctrlr = &rpmh_rsc[i];
>>> + return ctrlr;
>>> + }
>>> + }
>>> +
>>> + spin_lock_irqsave(&rpmh_rsc_lock, flags);
>>> + list_for_each_entry(p, &rsc_drv_list, list) {
>>> + if (drv == p) {
>>> + for (i = 0; i < RPMH_MAX_CTRLR; i++) {
>>> + if (!rpmh_rsc[i].drv)
>>> + break;
>>> + }
>>> + if (i == RPMH_MAX_CTRLR) {
>>> + ctrlr = ERR_PTR(-ENOMEM);
>>> + break;
>>> + }
>>> + rpmh_rsc[i].drv = drv;
>>> + ctrlr = &rpmh_rsc[i];
>>> + break;
>>> + }
>>> + }
>>> + spin_unlock_irqrestore(&rpmh_rsc_lock, flags);
>>
>>
>> I may have missed something, but to me it appears that this whole
>> "rsc_drv_list" is pretty pointless. I wrote up a patch atop your
>> series to remove it at
>>
>>
Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions
On 2018-05-03 14:26, Doug Anderson wrote:
Hi Doug,
Hi,
On Wed, May 2, 2018 at 12:37 PM, Lina Iyer
wrote:
+static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
+static DEFINE_SPINLOCK(rpmh_rsc_lock);
+
+static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
+{
+ int i;
+ struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
+ struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
+ unsigned long flags;
+
+ if (!drv)
+ return ctrlr;
+
+ for (i = 0; i < RPMH_MAX_CTRLR; i++) {
+ if (rpmh_rsc[i].drv == drv) {
+ ctrlr = &rpmh_rsc[i];
+ return ctrlr;
+ }
+ }
+
+ spin_lock_irqsave(&rpmh_rsc_lock, flags);
+ list_for_each_entry(p, &rsc_drv_list, list) {
+ if (drv == p) {
+ for (i = 0; i < RPMH_MAX_CTRLR; i++) {
+ if (!rpmh_rsc[i].drv)
+ break;
+ }
+ if (i == RPMH_MAX_CTRLR) {
+ ctrlr = ERR_PTR(-ENOMEM);
+ break;
+ }
+ rpmh_rsc[i].drv = drv;
+ ctrlr = &rpmh_rsc[i];
+ break;
+ }
+ }
+ spin_unlock_irqrestore(&rpmh_rsc_lock, flags);
I may have missed something, but to me it appears that this whole
"rsc_drv_list" is pretty pointless. I wrote up a patch atop your
series to remove it at
Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions
On Thu, May 03, 2018 at 01:26:07PM -0700, Doug Anderson wrote:
> Hi,
>
> On Wed, May 2, 2018 at 12:37 PM, Lina Iyer wrote:
> > +static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
> > +static DEFINE_SPINLOCK(rpmh_rsc_lock);
> > +
> > +static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
> > +{
> > + int i;
> > + struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
> > + struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
> > + unsigned long flags;
> > +
> > + if (!drv)
> > + return ctrlr;
> > +
> > + for (i = 0; i < RPMH_MAX_CTRLR; i++) {
> > + if (rpmh_rsc[i].drv == drv) {
> > + ctrlr = &rpmh_rsc[i];
> > + return ctrlr;
> > + }
> > + }
> > +
> > + spin_lock_irqsave(&rpmh_rsc_lock, flags);
> > + list_for_each_entry(p, &rsc_drv_list, list) {
> > + if (drv == p) {
> > + for (i = 0; i < RPMH_MAX_CTRLR; i++) {
> > + if (!rpmh_rsc[i].drv)
> > + break;
> > + }
> > + if (i == RPMH_MAX_CTRLR) {
> > + ctrlr = ERR_PTR(-ENOMEM);
> > + break;
> > + }
> > + rpmh_rsc[i].drv = drv;
> > + ctrlr = &rpmh_rsc[i];
> > + break;
> > + }
> > + }
> > + spin_unlock_irqrestore(&rpmh_rsc_lock, flags);
>
> I may have missed something, but to me it appears that this whole
> "rsc_drv_list" is pretty pointless. I wrote up a patch atop your
> series to remove it at
>
Re: [PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions
Hi,
On Wed, May 2, 2018 at 12:37 PM, Lina Iyer wrote:
> +static struct rpmh_ctrlr rpmh_rsc[RPMH_MAX_CTRLR];
> +static DEFINE_SPINLOCK(rpmh_rsc_lock);
> +
> +static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
> +{
> + int i;
> + struct rsc_drv *p, *drv = dev_get_drvdata(dev->parent);
> + struct rpmh_ctrlr *ctrlr = ERR_PTR(-EINVAL);
> + unsigned long flags;
> +
> + if (!drv)
> + return ctrlr;
> +
> + for (i = 0; i < RPMH_MAX_CTRLR; i++) {
> + if (rpmh_rsc[i].drv == drv) {
> + ctrlr = &rpmh_rsc[i];
> + return ctrlr;
> + }
> + }
> +
> + spin_lock_irqsave(&rpmh_rsc_lock, flags);
> + list_for_each_entry(p, &rsc_drv_list, list) {
> + if (drv == p) {
> + for (i = 0; i < RPMH_MAX_CTRLR; i++) {
> + if (!rpmh_rsc[i].drv)
> + break;
> + }
> + if (i == RPMH_MAX_CTRLR) {
> + ctrlr = ERR_PTR(-ENOMEM);
> + break;
> + }
> + rpmh_rsc[i].drv = drv;
> + ctrlr = &rpmh_rsc[i];
> + break;
> + }
> + }
> + spin_unlock_irqrestore(&rpmh_rsc_lock, flags);
I may have missed something, but to me it appears that this whole
"rsc_drv_list" is pretty pointless. I wrote up a patch atop your
series to remove it at
[PATCH v7 04/10] drivers: qcom: rpmh: add RPMH helper functions
Sending RPMH requests and waiting for response from the controller
through a callback is common functionality across all platform drivers.
To simplify drivers, add a library functions to create RPMH client and
send resource state requests.
rpmh_write() is a synchronous blocking call that can be used to send
active state requests.
Signed-off-by: Lina Iyer
---
Changes in v7:
- Optimization and locking fixes
Changes in v6:
- replace rpmh_client with device
- inline wait_for_tx_done()
Changes in v4:
- use const struct tcs_cmd in API
- remove wait count from this patch
- changed -EFAULT to -EINVAL
---
drivers/soc/qcom/Makefile| 4 +-
drivers/soc/qcom/rpmh-internal.h | 6 ++
drivers/soc/qcom/rpmh-rsc.c | 8 ++
drivers/soc/qcom/rpmh.c | 176 +++
include/soc/qcom/rpmh.h | 25 +
5 files changed, 218 insertions(+), 1 deletion(-)
create mode 100644 drivers/soc/qcom/rpmh.c
create mode 100644 include/soc/qcom/rpmh.h
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index cb6300f6a8e9..bb395c3202ca 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -7,7 +7,9 @@ obj-$(CONFIG_QCOM_PM) += spm.o
obj-$(CONFIG_QCOM_QMI_HELPERS) += qmi_helpers.o
qmi_helpers-y += qmi_encdec.o qmi_interface.o
obj-$(CONFIG_QCOM_RMTFS_MEM) += rmtfs_mem.o
-obj-$(CONFIG_QCOM_RPMH)+= rpmh-rsc.o
+obj-$(CONFIG_QCOM_RPMH)+= qcom_rpmh.o
+qcom_rpmh-y+= rpmh-rsc.o
+qcom_rpmh-y+= rpmh.o
obj-$(CONFIG_QCOM_SMD_RPM) += smd-rpm.o
obj-$(CONFIG_QCOM_SMEM) += smem.o
obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index cc29176f1303..d9a21726e568 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -14,6 +14,7 @@
#define MAX_CMDS_PER_TCS 16
#define MAX_TCS_PER_TYPE 3
#define MAX_TCS_NR (MAX_TCS_PER_TYPE * TCS_TYPE_NR)
+#define RPMH_MAX_CTRLR 2
struct rsc_drv;
@@ -52,6 +53,7 @@ struct tcs_group {
* @tcs:TCS groups
* @tcs_in_use: s/w state of the TCS
* @lock: synchronize state of the controller
+ * @list: element in list of drv
*/
struct rsc_drv {
const char *name;
@@ -61,9 +63,13 @@ struct rsc_drv {
struct tcs_group tcs[TCS_TYPE_NR];
DECLARE_BITMAP(tcs_in_use, MAX_TCS_NR);
spinlock_t lock;
+ struct list_head list;
};
+extern struct list_head rsc_drv_list;
int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
+void rpmh_tx_done(const struct tcs_request *msg, int r);
+
#endif /* __RPM_INTERNAL_H__ */
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index b9ad2e7b1dea..33270b1d5991 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -61,6 +61,8 @@
#define CMD_STATUS_ISSUED BIT(8)
#define CMD_STATUS_COMPL BIT(16)
+LIST_HEAD(rsc_drv_list);
+
static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
{
return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id +
@@ -176,6 +178,8 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
spin_lock(&drv->lock);
clear_bit(i, drv->tcs_in_use);
spin_unlock(&drv->lock);
+ if (req)
+ rpmh_tx_done(req, err);
}
return IRQ_HANDLED;
@@ -467,6 +471,10 @@ static int rpmh_rsc_probe(struct platform_device *pdev)
/* Enable the active TCS to send requests immediately */
write_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0, drv->tcs[ACTIVE_TCS].mask);
+ INIT_LIST_HEAD(&drv->list);
+ list_add(&drv->list, &rsc_drv_list);
+ dev_set_drvdata(&pdev->dev, drv);
+
return devm_of_platform_populate(&pdev->dev);
}
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
new file mode 100644
index ..74bb82339b01
--- /dev/null
+++ b/drivers/soc/qcom/rpmh.c
@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#include "rpmh-internal.h"
+
+#define RPMH_TIMEOUT_MSmsecs_to_jiffies(1)
+
+#define DEFINE_RPMH_MSG_ONSTACK(dev, s, q, name) \
+ struct rpmh_request name = {\
+ .msg = {\
+ .state = s, \
+ .cmds = name.cmd, \
+ .num_cmds = 0, \
+ .wait_for_compl = true, \
+ },
