drivers/qcom: add RPMH communication support

Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt new file +0 -0

new file mode 100644
index 000000000000..950d56325284
@@ -0,0 +1,132 @@

Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt new file +18 -0

+
+- qcom,tcs-config:
+       Usage: required
+       Value type: <prop-encoded-array>
+       Definition: The tuple defining the configuration of TCS.
+                   Must have 2 cells which describe each TCS type.
+                   <type number_of_tcs>.
+                   The order of the TCS must match the hardware
+                   configuration.
+       - Cell #1 (TCS Type): TCS types to be specified -
+                   SLEEP_TCS
+                   WAKE_TCS
+                   ACTIVE_TCS
+                   CONTROL_TCS
+       - Cell #2 (Number of TCS): <u32>
+
+- label:
+       Usage: optional

+ interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;

Documentation/devicetree/bindings/soc/qcom/rpmh-rsc.txt new file +7 -0

+               qcom,drv-id = <2>;
+               qcom,tcs-config = <SLEEP_TCS   3>,
+                                 <WAKE_TCS    3>,
+                                 <ACTIVE_TCS  2>,
+                                 <CONTROL_TCS 1>;
+       };
+

Log sent RPMH requests and interrupt responses in FTRACE.

Cc Steven Rostedt <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

drivers/soc/qcom/rpmh.c new file +27 -0

+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;
+                       }

The global rsc_drv_list was (as far as I can tell) racy and not useful for anything.

I say it is racy because in general you need some sort of mutual exclusion for lists. If someone is adding to a list while someone else is iterating over it then you get badness.

I say it is not useful because the only user of it was get_rpmh_ctrlr() and the only thing it did was to verify that the "struct rsc_drv *" that it alrady had was in the list. How could it not be?

NOTE: After this patch get_rpmh_ctrlr() still seems a bit fishy. I'm not sure why every caller would need its own private global cache of stuff. ...but I left that part alone.

Hi,

Quoted 36 lines · Lina Iyer

drivers/soc/qcom/rpmh.c new file +36 -0

+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;
+               }
+       }

Quoted 1 line

+ 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
<https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1042883/>
and it simplifies the code a whole bunch. From that patch, my justification was:

Quoted 11 lines

The global rsc_drv_list was (as far as I can tell) racy and not useful for anything.

I say it is racy because in general you need some sort of mutual exclusion for lists. If someone is adding to a list while someone else is iterating over it then you get badness.

I say it is not useful because the only user of it was get_rpmh_ctrlr() and the only thing it did was to verify that the "struct rsc_drv *" that it alrady had was in the list. How could it not be?

Note that in v7 of your series you added a spinlock around your access of "rsc_drv_list", but this doesn't actually remove the race. Specifically I'm pretty sure that the list primitives don't support calling list_add() while someone might be iterating over the list and your spinlock isn't grabbed in rpmh_rsc_probe().

if (!rpmh_rsc[i].drv) {
    rpmh_rsc[i].drv = drv;

    spin_lock_init(&rpmh_rsc[i].lock);
    INIT_LIST_HEAD(&rpmh_rsc[i].cache);
    return &rpmh_rsc[i];
}

Hi,

Quoted 27 lines · Lina Iyer

drivers/soc/qcom/rpmh.c new file +27 -0

+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;
+                       }

drivers/soc/qcom/rpmh.c line 98

Quoted 10 lines

drivers/soc/qcom/rpmh.c new file +10 -0

+                       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
<https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1042883/>
and it simplifies the code a whole bunch. From that patch, my justification was:

Quoted 11 lines

The global rsc_drv_list was (as far as I can tell) racy and not useful for anything.

I say it is racy because in general you need some sort of mutual exclusion for lists. If someone is adding to a list while someone else is iterating over it then you get badness.

I say it is not useful because the only user of it was get_rpmh_ctrlr() and the only thing it did was to verify that the "struct rsc_drv *" that it alrady had was in the list. How could it not be?

Note that in v7 of your series you added a spinlock around your access of "rsc_drv_list", but this doesn't actually remove the race. Specifically I'm pretty sure that the list primitives don't support calling list_add() while someone might be iterating over the list and your spinlock isn't grabbed in rpmh_rsc_probe().

Note that I also say in my patch:

Quoted 3 lines

NOTE: After this patch get_rpmh_ctrlr() still seems a bit fishy. I'm not sure why every caller would need its own private global cache of stuff. ...but I left that part alone.

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.

On 2018-05-03 14:26, Doug Anderson wrote:
Hi Doug,

Quoted 70 lines

Hi,

Quoted 28 lines · Lina Iyer

drivers/soc/qcom/rpmh.c new file +27 -0

+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;
+                       }

drivers/soc/qcom/rpmh.c line 98

Quoted 10 lines

drivers/soc/qcom/rpmh.c new file +10 -0

+                       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

<https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1042883/>
and it simplifies the code a whole bunch. From that patch, my justification was:

Quoted 11 lines

The global rsc_drv_list was (as far as I can tell) racy and not useful for anything.

I say it is racy because in general you need some sort of mutual exclusion for lists. If someone is adding to a list while someone else is iterating over it then you get badness.

I say it is not useful because the only user of it was get_rpmh_ctrlr() and the only thing it did was to verify that the "struct rsc_drv *" that it alrady had was in the list. How could it not be?

Note that in v7 of your series you added a spinlock around your access of "rsc_drv_list", but this doesn't actually remove the race. Specifically I'm pretty sure that the list primitives don't support calling list_add() while someone might be iterating over the list and your spinlock isn't grabbed in rpmh_rsc_probe().

Quoted 7 lines

Note that I also say in my patch:

Quoted 3 lines

NOTE: After this patch get_rpmh_ctrlr() still seems a bit fishy. I'm not sure why every caller would need its own private global cache of stuff. ...but I left that part alone.

I am not sure I understand this.

Quoted 6 lines

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.

Hi,

Quoted 74 lines · On Tue, May 8, 2018 at 9:05 AM, <[email protected]> wrote:

On 2018-05-03 14:26, Doug Anderson wrote:
Hi Doug,

Quoted 70 lines

Hi,

Quoted 28 lines · Lina Iyer

drivers/soc/qcom/rpmh.c new file +27 -0

+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;
+                       }

drivers/soc/qcom/rpmh.c line 98

Quoted 10 lines

drivers/soc/qcom/rpmh.c new file +10 -0

+                       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

<https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1042883/>
and it simplifies the code a whole bunch. From that patch, my justification was:

Quoted 11 lines

The global rsc_drv_list was (as far as I can tell) racy and not useful for anything.

I say it is racy because in general you need some sort of mutual exclusion for lists. If someone is adding to a list while someone else is iterating over it then you get badness.

I say it is not useful because the only user of it was get_rpmh_ctrlr() and the only thing it did was to verify that the "struct rsc_drv *" that it alrady had was in the list. How could it not be?

Note that in v7 of your series you added a spinlock around your access of "rsc_drv_list", but this doesn't actually remove the race. Specifically I'm pretty sure that the list primitives don't support calling list_add() while someone might be iterating over the list and your spinlock isn't grabbed in rpmh_rsc_probe().

Quoted 8 lines

Quoted 7 lines

Note that I also say in my patch:

Quoted 3 lines

NOTE: After this patch get_rpmh_ctrlr() still seems a bit fishy. I'm not sure why every caller would need its own private global cache of stuff. ...but I left that part alone.

I am not sure I understand this.

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.

Quoted 7 lines

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 9 lines

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.

Quoted 2 lines

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.

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.

Quoted 14 lines

Quoted 12 lines

Quoted 6 lines

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.

Quoted 5 lines

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,

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.

Hi,

Quoted 9 lines · Lina Iyer

Quoted 9 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 4 lines

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?

Quoted 14 lines

Quoted 14 lines

Quoted 12 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 5 lines

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.

Quoted 4 lines

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 trying to leverage.

Sleep and wake requests are sent when the application processor subsystem of the SoC is entering deep sleep states like in suspend. These requests help lower the system power requirements when the resources are not in use.

Sleep and wake requests are written to the TCS slots but are not triggered at the time of writing. The TCS are triggered by the firmware after the last of the CPUs has executed its WFI. Since these requests may come in different batches of requests, it is the job of this controller driver to find and arrange the requests into the available TCSes.

Signed-off-by Lina Iyer <[email protected]>

Reviewed-by Evan Green <[email protected]>

---

Changes in v7:
- Bug fix in find_match()

---
 drivers/soc/qcom/rpmh-internal.h |   8 +++
 drivers/soc/qcom/rpmh-rsc.c      | 118 +++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)

drivers/soc/qcom/rpmh-internal.h +8 -0

index d9a21726e568..6e19fe458c31 100644
@@ -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 MAX_TCS_SLOTS			(MAX_CMDS_PER_TCS * MAX_TCS_PER_TYPE)
 #define RPMH_MAX_CTRLR			2
 
 struct rsc_drv;

@@ -30,6 +31,8 @@ struct rsc_drv;
  * @ncpt:      number of commands in each TCS
  * @lock:      lock for synchronizing this TCS writes
  * @req:       requests that are sent from the TCS
+ * @cmd_cache: flattened cache of cmds in sleep/wake TCS
+ * @slots:     indicates which of @cmd_addr are occupied
  */
 struct tcs_group {
 	struct rsc_drv *drv;

@@ -40,6 +43,8 @@ struct tcs_group {
 	int ncpt;
 	spinlock_t lock;
 	const struct tcs_request *req[MAX_TCS_PER_TYPE];
+	u32 *cmd_cache;
+	DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
 };
 
 /**

@@ -69,6 +74,9 @@ struct rsc_drv {
 extern struct list_head rsc_drv_list;
 
 int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
+int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv,
+			     const struct tcs_request *msg);
+int rpmh_rsc_invalidate(struct rsc_drv *drv);
 
 void rpmh_tx_done(const struct tcs_request *msg, int r);
 

drivers/soc/qcom/rpmh-rsc.c +7 -0

index 33270b1d5991..4e2144a14c31 100644
@@ -113,6 +113,12 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
 	case RPMH_ACTIVE_ONLY_STATE:
 		type = ACTIVE_TCS;
 		break;
+	case RPMH_WAKE_ONLY_STATE:
+		type = WAKE_TCS;
+		break;
+	case RPMH_SLEEP_STATE:
+		type = SLEEP_TCS;
+		break;
 	default:
 		return ERR_PTR(-EINVAL);
 	}

@@ -353,6 +359,105 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
 }
 EXPORT_SYMBOL(rpmh_rsc_send_data);
 
+static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,

Hi Lina,

Quoted 89 lines · Lina Iyer

Sleep and wake requests are sent when the application processor subsystem of the SoC is entering deep sleep states like in suspend. These requests help lower the system power requirements when the resources are not in use.

Sleep and wake requests are written to the TCS slots but are not triggered at the time of writing. The TCS are triggered by the firmware after the last of the CPUs has executed its WFI. Since these requests may come in different batches of requests, it is the job of this controller driver to find and arrange the requests into the available TCSes.

Signed-off-by Lina Iyer <[email protected]>

Reviewed-by Evan Green <[email protected]>

---

Changes in v7:
- Bug fix in find_match()

---
 drivers/soc/qcom/rpmh-internal.h |   8 +++
 drivers/soc/qcom/rpmh-rsc.c      | 118 
+++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)

drivers/soc/qcom/rpmh-internal.h +8 -0

index d9a21726e568..6e19fe458c31 100644
@@ -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 MAX_TCS_SLOTS			(MAX_CMDS_PER_TCS * MAX_TCS_PER_TYPE)
 #define RPMH_MAX_CTRLR			2
 
 struct rsc_drv;

@@ -30,6 +31,8 @@ struct rsc_drv;
  * @ncpt:      number of commands in each TCS
  * @lock:      lock for synchronizing this TCS writes
  * @req:       requests that are sent from the TCS
+ * @cmd_cache: flattened cache of cmds in sleep/wake TCS
+ * @slots:     indicates which of @cmd_addr are occupied
  */
 struct tcs_group {
 	struct rsc_drv *drv;

@@ -40,6 +43,8 @@ struct tcs_group {
 	int ncpt;
 	spinlock_t lock;
 	const struct tcs_request *req[MAX_TCS_PER_TYPE];
+	u32 *cmd_cache;
+	DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
 };
 
 /**

@@ -69,6 +74,9 @@ struct rsc_drv {
 extern struct list_head rsc_drv_list;
 
 int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request 
 *msg);
+int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv,
+			     const struct tcs_request *msg);
+int rpmh_rsc_invalidate(struct rsc_drv *drv);
 
 void rpmh_tx_done(const struct tcs_request *msg, int r);

drivers/soc/qcom/rpmh-rsc.c +6 -0

index 33270b1d5991..4e2144a14c31 100644
@@ -113,6 +113,12 @@ static struct tcs_group *get_tcs_for_msg(struct 
 rsc_drv *drv,
 	case RPMH_ACTIVE_ONLY_STATE:
 		type = ACTIVE_TCS;
 		break;
+	case RPMH_WAKE_ONLY_STATE:
+		type = WAKE_TCS;
+		break;
+	case RPMH_SLEEP_STATE:
+		type = SLEEP_TCS;
+		break;
 	default:
 		return ERR_PTR(-EINVAL);

drivers/soc/qcom/rpmh-rsc.c +0 -0

 	}

@@ -353,6 +359,105 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, 
 const struct tcs_request *msg)
 }
 EXPORT_SYMBOL(rpmh_rsc_send_data);
 

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 372

Quoted 12 lines

drivers/soc/qcom/rpmh-rsc.c +11 -0

+static int find_match(const struct tcs_group *tcs, const struct 
 tcs_cmd *cmd,
+		      int len)
+{
+	int i, j;
+
+	/* Check for already cached commands */
+	for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
+		if (tcs->cmd_cache[i] != cmd[0].addr)
+			continue;
+		for (j = 0; j < len; j++) {
+			if (tcs->cmd_cache[i + j] != cmd[j].addr) {

As mentioned in the review of v6, the code could read memory beyond tcs->cmd_cache if 'i + len > ARRAY_SIZE(tcs->cmd_cache)'

drivers/soc/qcom/rpmh-rsc.c int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg) · line 384

Quoted 13 lines

drivers/soc/qcom/rpmh-rsc.c +12 -0

+				WARN(1, "Message does not match previous sequence.\n");
+				return -EINVAL;
+			}
+		}
+		return i;
+	}
+
+	return -ENODATA;
+}
+
+static int find_slots(struct tcs_group *tcs, const struct tcs_request 
 *msg,
+		      int *tcs_id, int *cmd_id)

The change from m/n => tcs_id/cmd_id in v7 greatly improves readability, thanks!

Matthias

PS: Please remember to include reviewers of earlier revisions to cc:

Allow sleep and wake commands to be cleared from the respective TCSes, so that they can be re-populated.

Signed-off-by Lina Iyer <[email protected]>

---

Changes in v7:
- Move bitmap_zero() outside the loop

Changes in v6:
- remove unnecessary locks around __tcs_invalidate
- rename function to tcs_invaldiate

Changes in v4:
- refactored the rphm_rsc_invalidate()

---
 drivers/soc/qcom/rpmh-rsc.c | 45 +++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

drivers/soc/qcom/rpmh-rsc.c +0 -0

index 4e2144a14c31..42aedf2d80fe 100644

drivers/soc/qcom/rpmh-rsc.c +10 -0

+	struct tcs_group *tcs;
+
+	tcs = get_tcs_of_type(drv, type);
+	if (IS_ERR(tcs))
+		return PTR_ERR(tcs);
+
+	spin_lock(&tcs->lock);
+	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
+		spin_unlock(&tcs->lock);
+		return 0;

Hi Lina,

Quoted 29 lines · Lina Iyer

Allow sleep and wake commands to be cleared from the respective TCSes, so that they can be re-populated.

Signed-off-by Lina Iyer <[email protected]>

---

Changes in v7:
- Move bitmap_zero() outside the loop

Changes in v6:
- remove unnecessary locks around __tcs_invalidate
- rename function to tcs_invaldiate

Changes in v4:
- refactored the rphm_rsc_invalidate()

---
 drivers/soc/qcom/rpmh-rsc.c | 45 
+++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

drivers/soc/qcom/rpmh-rsc.c +0 -0

index 4e2144a14c31..42aedf2d80fe 100644
@@ -104,6 +104,51 @@ static struct tcs_group *get_tcs_of_type(struct 
 rsc_drv *drv, int type)
 	return &drv->tcs[type];
 }
 

drivers/soc/qcom/rpmh-rsc.c static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type) · line 109

Quoted 3 lines

drivers/soc/qcom/rpmh-rsc.c +3 -0

+static int tcs_invalidate(struct rsc_drv *drv, int type)
+{
+	int m;

nit: use tcs_id?

Quoted 10 lines

drivers/soc/qcom/rpmh-rsc.c +10 -0

+	struct tcs_group *tcs;
+
+	tcs = get_tcs_of_type(drv, type);
+	if (IS_ERR(tcs))
+		return PTR_ERR(tcs);
+
+	spin_lock(&tcs->lock);
+	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
+		spin_unlock(&tcs->lock);
+		return 0;

drivers/soc/qcom/rpmh-rsc.c static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type) · line 129

Quoted 10 lines

drivers/soc/qcom/rpmh-rsc.c +10 -0

+	}
+
+	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
+		if (!tcs_is_free(drv, m)) {
+			spin_unlock(&tcs->lock);
+			return -EAGAIN;
+		}
+		write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
+	}
+	bitmap_zero(tcs->slots, MAX_TCS_SLOTS);

You didn't reply to (or address) my comment on v6:

It could occur that one or more TCSes are disabled, then tcs_is_free() returns false for the next TCS and the function returns without having updated tcs->slots for the TCSes that have been disabled.

On 2018-05-03 16:06, Matthias Kaehlcke wrote:
Hi Matthias,

Quoted 32 lines

Hi Lina,

Quoted 29 lines · Lina Iyer

Allow sleep and wake commands to be cleared from the respective TCSes, so that they can be re-populated.

Signed-off-by Lina Iyer <[email protected]>

---

Changes in v7:
- Move bitmap_zero() outside the loop

Changes in v6:
- remove unnecessary locks around __tcs_invalidate
- rename function to tcs_invaldiate

Changes in v4:
- refactored the rphm_rsc_invalidate()

---
 drivers/soc/qcom/rpmh-rsc.c | 45
+++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

drivers/soc/qcom/rpmh-rsc.c +0 -0

index 4e2144a14c31..42aedf2d80fe 100644
@@ -104,6 +104,51 @@ static struct tcs_group *get_tcs_of_type(struct
 rsc_drv *drv, int type)
 	return &drv->tcs[type];
 }
 

Quoted 6 lines

drivers/soc/qcom/rpmh-rsc.c static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type) · line 109

Quoted 3 lines

drivers/soc/qcom/rpmh-rsc.c +3 -0

+static int tcs_invalidate(struct rsc_drv *drv, int type)
+{
+	int m;

nit: use tcs_id?

It's an iterator. Hence didn't change this.

Quoted 23 lines

Quoted 10 lines

drivers/soc/qcom/rpmh-rsc.c +10 -0

+	struct tcs_group *tcs;
+
+	tcs = get_tcs_of_type(drv, type);
+	if (IS_ERR(tcs))
+		return PTR_ERR(tcs);
+
+	spin_lock(&tcs->lock);
+	if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
+		spin_unlock(&tcs->lock);
+		return 0;

drivers/soc/qcom/rpmh-rsc.c static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type) · line 129

Quoted 10 lines

drivers/soc/qcom/rpmh-rsc.c +10 -0

+	}
+
+	for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
+		if (!tcs_is_free(drv, m)) {
+			spin_unlock(&tcs->lock);
+			return -EAGAIN;
+		}
+		write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
+	}
+	bitmap_zero(tcs->slots, MAX_TCS_SLOTS);

You didn't reply to (or address) my comment on v6:

Quoted 3 lines

It could occur that one or more TCSes are disabled, then tcs_is_free() returns false for the next TCS and the function returns without having updated tcs->slots for the TCSes that have been disabled.

How do you mean TCS are disabled? I think I asked that question in my mail. Sorry, if I forgot to ask.

TCSes are either available for sending requests or they are busy sending requests. They cannot be disabled if they are present. Individual commands, however are enabled or disabled based on whether they have active requests or not.

What we are trying to do here is to cleanup the TCSes of their existing requests. Generally, sleep and wake TCSes are not used to send active state requests, they are sent through AMC/Active TCS. So they will be free. However, in the case of the Display RSC, there is no explicit TCS available for sending active state requests. So we overload the wake TCS to send the active state requests. Even in that case, TCS would have finished and should be free when this function is called. The tcs_is_free() check is addition to make sure the requirement is not violated.

Active state requests are sent immediately to the RSC controller, while sleep and wake state requests are cached in this driver to avoid taxing the RSC controller repeatedly. The cached values will be sent to the controller when the rpmh_flush() is called.

Generally, flushing is a system PM activity and may be called from the system PM drivers when the system is entering suspend or deeper sleep modes during cpuidle.

Also allow invalidating the cached requests, so they may be re-populated again.

Signed-off-by Lina Iyer <[email protected]>

Reviewed-by Evan Green <[email protected]>