Hi Evan,
On 11/19/19 00:42, Evan Green wrote:
> Hi Sibi,
>
> On Mon, Nov 18, 2019 at 7:45 AM Sibi Sankar <sibis@xxxxxxxxxxxxxx> wrote:
>>
>> On some Qualcomm SoCs, Operating State Manager (OSM) controls the
>> resources of scaling L3 caches. Add a driver to handle bandwidth
>> requests to OSM L3 from CPU/GPU.
>>
>> Signed-off-by: Sibi Sankar <sibis@xxxxxxxxxxxxxx>
>> ---
>> drivers/interconnect/qcom/Kconfig | 7 +
>> drivers/interconnect/qcom/Makefile | 2 +
>> drivers/interconnect/qcom/osm-l3.c | 284 +++++++++++++++++++++++++++++
>> 3 files changed, 293 insertions(+)
>> create mode 100644 drivers/interconnect/qcom/osm-l3.c
>>
[..]
>> +static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
>> + u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
>> +{
>> + *agg_avg += avg_bw;
>> + *agg_peak = max_t(u32, *agg_peak, peak_bw);
>> +
>> + return 0;
>> +}
>
> Georgi, I wonder if it's a good idea to make a small collection of
> "std" aggregate functions in the interconnect core that a driver can
> just point to if it's doing something super standard like this (ie
> driver->aggregate = icc_std_aggregate;). This is probably fine as-is
> for now, but if we see a lot more copy/pastes of this function we
> should think about it.
Sure, thanks for the suggestion! Will submit a patch for this.
>
>> +
>> +static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>> +{
[..]
>> +
>> +static int qcom_osm_l3_remove(struct platform_device *pdev)
>> +{
>> + struct qcom_osm_l3_icc_provider *qp = platform_get_drvdata(pdev);
>> + struct icc_provider *provider = &qp->provider;
>> + struct icc_node *n;
>> +
>> + list_for_each_entry(n, &provider->nodes, node_list) {
>
> There was a comment on one of the other threads that we've been
> copy/pasting this snippet around and it's wrong because it doesn't use
> the _safe variant of the macro. So we end up destroying the list we're
> iterating over.
>
> Georgi, did you have a plan to refactor this, or should we just change
> this to be the _safe version?
I have fixes that convert this to the _safe variant (for stable) and also
factor this out into icc_nodes_remove() function. Will post them.
Thanks,
Georgi
>
>> + icc_node_del(n);
>> + icc_node_destroy(n->id);
>> + }
>> +
>> + return icc_provider_del(provider);
>> +}
