Re: [RFC PATCH v3 3/7] interconnect: Allow inter-provider pairs to be configured

From: Chanwoo Choi <chanwoo@kernel.org>
To: "Artur Świgoń" <a.swigon@samsung.com>
Cc: devicetree <devicetree@vger.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	linux-samsung-soc <linux-samsung-soc@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Linux PM list <linux-pm@vger.kernel.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	MyungJoo Ham <myungjoo.ham@samsung.com>,
	inki.dae@samsung.com, Seung-Woo Kim <sw0312.kim@samsung.com>,
	Georgi Djakov <georgi.djakov@linaro.org>,
	Leonard Crestez <leonard.crestez@nxp.com>,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Krzysztof Kozlowski <krzk@kernel.org>
Subject: Re: [RFC PATCH v3 3/7] interconnect: Allow inter-provider pairs to be configured
Date: Mon, 23 Dec 2019 02:08:41 +0900	[thread overview]
Message-ID: <CAGTfZH0zfvPYtTv6v+5nq99Gd2PVtg+O20dwf2nbV2j1U0nxCQ@mail.gmail.com> (raw)
In-Reply-To: <20191220115653.6487-4-a.swigon@samsung.com>

Hi,

On Fri, Dec 20, 2019 at 9:03 PM Artur Świgoń <a.swigon@samsung.com> wrote:
>
> In the exynos-bus devfreq driver every bus is probed separately and is

IMHO, the patch description should specify the more general cause
why have to be changed. Actually, almost people might not know
the 'exynos-bus'. So, firstly, you have to specify the general cause
why this patch is necessary without 'exynos-bus' word and then
add the real use-case with 'exynos-bus' example.

> assigned a separate interconnect provider. However, the interconnect
> framework does not call the '->set' callback for pairs of nodes which
> belong to different providers.
>
> This patch adds support for a new boolean 'inter_set' field in struct
> icc_provider. Setting it to 'true' enables calling '->set' for
> inter-provider node pairs. All existing users of the interconnect
> framework allocate this structure with kzalloc, and are therefore
> unaffected.
>
> Signed-off-by: Artur Świgoń <a.swigon@samsung.com>
> ---
>  drivers/interconnect/core.c           | 11 +++++------
>  include/linux/interconnect-provider.h |  2 ++
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 74c68898a350..a28bd0f8a497 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -259,23 +259,22 @@ static int aggregate_requests(struct icc_node *node)
>  static int apply_constraints(struct icc_path *path)
>  {
>         struct icc_node *next, *prev = NULL;
> +       struct icc_provider *p;
>         int ret = -EINVAL;
>         int i;
>
>         for (i = 0; i < path->num_nodes; i++) {
>                 next = path->reqs[i].node;
> +               p = next->provider;
>
> -               /*
> -                * Both endpoints should be valid master-slave pairs of the
> -                * same interconnect provider that will be configured.
> -                */
> -               if (!prev || next->provider != prev->provider) {
> +               /* both endpoints should be valid master-slave pairs */
> +               if (!prev || (p != prev->provider && !p->inter_set)) {
>                         prev = next;
>                         continue;
>                 }
>
>                 /* set the constraints */
> -               ret = next->provider->set(prev, next);
> +               ret = p->set(prev, next);
>                 if (ret)
>                         goto out;
>
> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
> index cc965b8fab53..b6ae0ee686c5 100644
> --- a/include/linux/interconnect-provider.h
> +++ b/include/linux/interconnect-provider.h
> @@ -41,6 +41,7 @@ struct icc_node *of_icc_xlate_onecell(struct of_phandle_args *spec,
>   * @xlate: provider-specific callback for mapping nodes from phandle arguments
>   * @dev: the device this interconnect provider belongs to
>   * @users: count of active users
> + * @inter_set: whether inter-provider pairs will be configured with @set
>   * @data: pointer to private data
>   */
>  struct icc_provider {
> @@ -53,6 +54,7 @@ struct icc_provider {
>         struct icc_node* (*xlate)(struct of_phandle_args *spec, void *data);
>         struct device           *dev;
>         int                     users;
> +       bool                    inter_set;
>         void                    *data;
>  };
>
> --
> 2.17.1
>

-- 
Best Regards,
Chanwoo Choi