[RFC PATCH 00/11] Simple QoS for exynos-bus driver using interconnect

[RFC PATCH 00/11] Simple QoS for exynos-bus driver using interconnect

[Artur Świgoń]

The following patchset adds interconnect[1][2] framework support to the
exynos-bus devfreq driver. Extending the devfreq driver with interconnect
capabilities started as a response to the issue referenced in [3]. The
patches can be subdivided into four logical groups:

(a) Refactoring the existing devfreq driver in order to improve readability
and accommodate for adding new code (patches 01--04/11).

(b) Tweaking the interconnect framework to support the exynos-bus use case
(patches 05--07/11). Exporting of_icc_get_from_provider() allows us to
avoid hardcoding every single graph edge in the DT or driver source, and
relaxing the requirement contained in that function removes the need to
provide dummy node IDs in the DT. Adjusting the logic in
apply_constraints() (drivers/interconnect/core.c) accounts for the fact
that every bus is a separate entity and therefore a separate interconnect
provider, albeit constituting a part of a larger hierarchy.

(c) Implementing interconnect providers in the exynos-bus devfreq driver
and adding required DT properties for one selected platform, namely
Exynos4412 (patches 08--09/11). Due to the fact that this aims to be a
generic driver for various Exynos SoCs, node IDs are generated dynamically
rather than hardcoded. This has been determined to be a simpler approach,
but depends on changes described in (b).

(d) Implementing a sample interconnect consumer for exynos-mixer targeted
at the issue referenced in [3], again with DT info only for Exynos4412
(patches 10--11/11).

Integration of devfreq and interconnect functionalities comes down to one
extra line in the devfreq target() callback, which selects either the
frequency calculated by the devfreq governor, or the one requested with the
interconnect API, whichever is higher. All new code works equally well when
CONFIG_INTERCONNECT is 'n' (as in exynos_defconfig) in which case all
interconnect API functions are no-ops.

---
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics

---
References:
[1] Documentation/interconnect/interconnect.rst
[2] Documentation/devicetree/bindings/interconnect/interconnect.txt
[3] https://patchwork.kernel.org/patch/10861757

Artur Świgoń (10):
  devfreq: exynos-bus: Extract exynos_bus_profile_init()
  devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()
  devfreq: exynos-bus: Change goto-based logic to if-else logic
  devfreq: exynos-bus: Clean up code
  icc: Export of_icc_get_from_provider()
  icc: Relax requirement in of_icc_get_from_provider()
  icc: Relax condition in apply_constraints()
  arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412
  devfreq: exynos-bus: Add interconnect functionality to exynos-bus
  arm: dts: exynos: Add interconnects to Exynos4412 mixer

Marek Szyprowski (1):
  drm: exynos: mixer: Add interconnect support

 .../boot/dts/exynos4412-odroid-common.dtsi    |   1 +
 arch/arm/boot/dts/exynos4412.dtsi             |  10 +
 drivers/devfreq/exynos-bus.c                  | 296 ++++++++++++++----
 drivers/gpu/drm/exynos/exynos_mixer.c         |  68 +++-
 drivers/interconnect/core.c                   |  12 +-
 include/linux/interconnect-provider.h         |   6 +
 6 files changed, 314 insertions(+), 79 deletions(-)

-- 
2.17.1

[RFC PATCH 01/11] devfreq: exynos-bus: Extract exynos_bus_profile_init()

[Artur Świgoń]

This patch adds a new static function, exynos_bus_profile_init(), extracted
from exynos_bus_probe().

Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
 drivers/devfreq/exynos-bus.c | 106 ++++++++++++++++++++---------------
 1 file changed, 60 insertions(+), 46 deletions(-)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index d9f377912c10..d8f1efaf2d49 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -372,12 +372,69 @@ static int exynos_bus_parse_of(struct device_node *np,
 	return ret;
 }
 
+static int exynos_bus_profile_init(struct exynos_bus *bus,
+				   struct devfreq_dev_profile *profile)
+{
+	struct device *dev = bus->dev;
+	struct devfreq_simple_ondemand_data *ondemand_data;
+	int ret;
+
+	/* Initialize the struct profile and governor data for parent device */
+	profile->polling_ms = 50;
+	profile->target = exynos_bus_target;
+	profile->get_dev_status = exynos_bus_get_dev_status;
+	profile->exit = exynos_bus_exit;
+
+	ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
+	if (!ondemand_data) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	ondemand_data->upthreshold = 40;
+	ondemand_data->downdifferential = 5;
+
+	/* Add devfreq device to monitor and handle the exynos bus */
+	bus->devfreq = devm_devfreq_add_device(dev, profile,
+						DEVFREQ_GOV_SIMPLE_ONDEMAND,
+						ondemand_data);
+	if (IS_ERR(bus->devfreq)) {
+		dev_err(dev, "failed to add devfreq device\n");
+		ret = PTR_ERR(bus->devfreq);
+		goto err;
+	}
+
+	/* Register opp_notifier to catch the change of OPP  */
+	ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
+	if (ret < 0) {
+		dev_err(dev, "failed to register opp notifier\n");
+		goto err;
+	}
+
+	/*
+	 * Enable devfreq-event to get raw data which is used to determine
+	 * current bus load.
+	 */
+	ret = exynos_bus_enable_edev(bus);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable devfreq-event devices\n");
+		goto err;
+	}
+
+	ret = exynos_bus_set_event(bus);
+	if (ret < 0) {
+		dev_err(dev, "failed to set event to devfreq-event devices\n");
+		goto err;
+	}
+
+err:
+	return ret;
+}
+
 static int exynos_bus_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node, *node;
 	struct devfreq_dev_profile *profile;
-	struct devfreq_simple_ondemand_data *ondemand_data;
 	struct devfreq_passive_data *passive_data;
 	struct devfreq *parent_devfreq;
 	struct exynos_bus *bus;
@@ -418,52 +475,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err;
 
-	/* Initialize the struct profile and governor data for parent device */
-	profile->polling_ms = 50;
-	profile->target = exynos_bus_target;
-	profile->get_dev_status = exynos_bus_get_dev_status;
-	profile->exit = exynos_bus_exit;
-
-	ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
-	if (!ondemand_data) {
-		ret = -ENOMEM;
+	ret = exynos_bus_profile_init(bus, profile);
+	if (ret < 0)
 		goto err;
-	}
-	ondemand_data->upthreshold = 40;
-	ondemand_data->downdifferential = 5;
-
-	/* Add devfreq device to monitor and handle the exynos bus */
-	bus->devfreq = devm_devfreq_add_device(dev, profile,
-						DEVFREQ_GOV_SIMPLE_ONDEMAND,
-						ondemand_data);
-	if (IS_ERR(bus->devfreq)) {
-		dev_err(dev, "failed to add devfreq device\n");
-		ret = PTR_ERR(bus->devfreq);
-		goto err;
-	}
-
-	/* Register opp_notifier to catch the change of OPP  */
-	ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
-	if (ret < 0) {
-		dev_err(dev, "failed to register opp notifier\n");
-		goto err;
-	}
-
-	/*
-	 * Enable devfreq-event to get raw data which is used to determine
-	 * current bus load.
-	 */
-	ret = exynos_bus_enable_edev(bus);
-	if (ret < 0) {
-		dev_err(dev, "failed to enable devfreq-event devices\n");
-		goto err;
-	}
-
-	ret = exynos_bus_set_event(bus);
-	if (ret < 0) {
-		dev_err(dev, "failed to set event to devfreq-event devices\n");
-		goto err;
-	}
 
 	goto out;
 passive:
-- 
2.17.1

Re: [RFC PATCH 01/11] devfreq: exynos-bus: Extract exynos_bus_profile_init()

[Krzysztof Kozlowski]

On Tue, Jul 23, 2019 at 02:20:06PM +0200, Artur Świgoń wrote:
> This patch adds a new static function, exynos_bus_profile_init(), extracted
> from exynos_bus_probe().
> 
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 106 ++++++++++++++++++++---------------
>  1 file changed, 60 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index d9f377912c10..d8f1efaf2d49 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -372,12 +372,69 @@ static int exynos_bus_parse_of(struct device_node *np,
>  	return ret;
>  }
>  
> +static int exynos_bus_profile_init(struct exynos_bus *bus,
> +				   struct devfreq_dev_profile *profile)
> +{
> +	struct device *dev = bus->dev;
> +	struct devfreq_simple_ondemand_data *ondemand_data;
> +	int ret;
> +
> +	/* Initialize the struct profile and governor data for parent device */
> +	profile->polling_ms = 50;
> +	profile->target = exynos_bus_target;
> +	profile->get_dev_status = exynos_bus_get_dev_status;
> +	profile->exit = exynos_bus_exit;
> +
> +	ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> +	if (!ondemand_data) {
> +		ret = -ENOMEM;
> +		goto err;

Just return proper error code. Less lines, obvious code since you do not
have any cleanup in error path.

> +	}
> +	ondemand_data->upthreshold = 40;
> +	ondemand_data->downdifferential = 5;
> +
> +	/* Add devfreq device to monitor and handle the exynos bus */
> +	bus->devfreq = devm_devfreq_add_device(dev, profile,
> +						DEVFREQ_GOV_SIMPLE_ONDEMAND,
> +						ondemand_data);
> +	if (IS_ERR(bus->devfreq)) {
> +		dev_err(dev, "failed to add devfreq device\n");
> +		ret = PTR_ERR(bus->devfreq);
> +		goto err;
> +	}
> +
> +	/* Register opp_notifier to catch the change of OPP  */
> +	ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register opp notifier\n");
> +		goto err;

The same - return err.

Best regards,
Krzysztof

Re: [RFC PATCH 01/11] devfreq: exynos-bus: Extract exynos_bus_profile_init()

[Artur Świgoń]

Hi,

On Wed, 2019-07-24 at 21:07 +0200, Krzysztof Kozlowski wrote:
> On Tue, Jul 23, 2019 at 02:20:06PM +0200, Artur Świgoń wrote:
> > This patch adds a new static function, exynos_bus_profile_init(), extracted
> > from exynos_bus_probe().
> > 
> > Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> > ---
> >  drivers/devfreq/exynos-bus.c | 106 ++++++++++++++++++++---------------
> >  1 file changed, 60 insertions(+), 46 deletions(-)
> > 
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index d9f377912c10..d8f1efaf2d49 100644
> > --- a/drivers/devfreq/exynos-bus.c
> > +++ b/drivers/devfreq/exynos-bus.c
> > @@ -372,12 +372,69 @@ static int exynos_bus_parse_of(struct device_node *np,
> >  	return ret;
> >  }
> >  
> > +static int exynos_bus_profile_init(struct exynos_bus *bus,
> > +				   struct devfreq_dev_profile *profile)
> > +{
> > +	struct device *dev = bus->dev;
> > +	struct devfreq_simple_ondemand_data *ondemand_data;
> > +	int ret;
> > +
> > +	/* Initialize the struct profile and governor data for parent device */
> > +	profile->polling_ms = 50;
> > +	profile->target = exynos_bus_target;
> > +	profile->get_dev_status = exynos_bus_get_dev_status;
> > +	profile->exit = exynos_bus_exit;
> > +
> > +	ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> > +	if (!ondemand_data) {
> > +		ret = -ENOMEM;
> > +		goto err;
> 
> Just return proper error code. Less lines, obvious code since you do not
> have any cleanup in error path.

I was advised to avoid modifying code being moved (in one patch). I do make
changes in these places in patch 04/11, i.e. change the original label 'err' to
'out'. What's your opinion on making the proposed changes to patches 01 and 02
(s/goto err/return ret/) in patch 04 instead?

> > +
> > +	/* Register opp_notifier to catch the change of OPP  */
> > +	ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to register opp notifier\n");
> > +		goto err;
> 
> The same - return err.
> 
> Best regards,
> Krzysztof

Best regards,
-- 
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics

Re: [RFC PATCH 01/11] devfreq: exynos-bus: Extract exynos_bus_profile_init()

[Krzysztof Kozlowski]

On Wed, 31 Jul 2019 at 15:00, Artur Świgoń <a.swigon@partner.samsung.com> wrote:
>
> Hi,
>
> On Wed, 2019-07-24 at 21:07 +0200, Krzysztof Kozlowski wrote:
> > On Tue, Jul 23, 2019 at 02:20:06PM +0200, Artur Świgoń wrote:
> > > This patch adds a new static function, exynos_bus_profile_init(), extracted
> > > from exynos_bus_probe().
> > >
> > > Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> > > ---
> > >  drivers/devfreq/exynos-bus.c | 106 ++++++++++++++++++++---------------
> > >  1 file changed, 60 insertions(+), 46 deletions(-)
> > >
> > > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > > index d9f377912c10..d8f1efaf2d49 100644
> > > --- a/drivers/devfreq/exynos-bus.c
> > > +++ b/drivers/devfreq/exynos-bus.c
> > > @@ -372,12 +372,69 @@ static int exynos_bus_parse_of(struct device_node *np,
> > >     return ret;
> > >  }
> > >
> > > +static int exynos_bus_profile_init(struct exynos_bus *bus,
> > > +                              struct devfreq_dev_profile *profile)
> > > +{
> > > +   struct device *dev = bus->dev;
> > > +   struct devfreq_simple_ondemand_data *ondemand_data;
> > > +   int ret;
> > > +
> > > +   /* Initialize the struct profile and governor data for parent device */
> > > +   profile->polling_ms = 50;
> > > +   profile->target = exynos_bus_target;
> > > +   profile->get_dev_status = exynos_bus_get_dev_status;
> > > +   profile->exit = exynos_bus_exit;
> > > +
> > > +   ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> > > +   if (!ondemand_data) {
> > > +           ret = -ENOMEM;
> > > +           goto err;
> >
> > Just return proper error code. Less lines, obvious code since you do not
> > have any cleanup in error path.
>
> I was advised to avoid modifying code being moved (in one patch). I do make
> changes in these places in patch 04/11, i.e. change the original label 'err' to
> 'out'. What's your opinion on making the proposed changes to patches 01 and 02
> (s/goto err/return ret/) in patch 04 instead?

Yes, you're right. I also prefer not to touch moved code.

Best regards,
Krzysztof

Re: [RFC PATCH 01/11] devfreq: exynos-bus: Extract exynos_bus_profile_init()

[Chanwoo Choi]

2019년 7월 24일 (수) 오전 8:09, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
>
> This patch adds a new static function, exynos_bus_profile_init(), extracted
> from exynos_bus_probe().
>
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 106 ++++++++++++++++++++---------------
>  1 file changed, 60 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index d9f377912c10..d8f1efaf2d49 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -372,12 +372,69 @@ static int exynos_bus_parse_of(struct device_node *np,
>         return ret;
>  }
>
> +static int exynos_bus_profile_init(struct exynos_bus *bus,
> +                                  struct devfreq_dev_profile *profile)
> +{
> +       struct device *dev = bus->dev;
> +       struct devfreq_simple_ondemand_data *ondemand_data;
> +       int ret;
> +
> +       /* Initialize the struct profile and governor data for parent device */
> +       profile->polling_ms = 50;
> +       profile->target = exynos_bus_target;
> +       profile->get_dev_status = exynos_bus_get_dev_status;
> +       profile->exit = exynos_bus_exit;
> +
> +       ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> +       if (!ondemand_data) {
> +               ret = -ENOMEM;
> +               goto err;
> +       }
> +       ondemand_data->upthreshold = 40;
> +       ondemand_data->downdifferential = 5;
> +
> +       /* Add devfreq device to monitor and handle the exynos bus */
> +       bus->devfreq = devm_devfreq_add_device(dev, profile,
> +                                               DEVFREQ_GOV_SIMPLE_ONDEMAND,
> +                                               ondemand_data);
> +       if (IS_ERR(bus->devfreq)) {
> +               dev_err(dev, "failed to add devfreq device\n");
> +               ret = PTR_ERR(bus->devfreq);
> +               goto err;
> +       }
> +
> +       /* Register opp_notifier to catch the change of OPP  */
> +       ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to register opp notifier\n");
> +               goto err;
> +       }
> +
> +       /*
> +        * Enable devfreq-event to get raw data which is used to determine
> +        * current bus load.
> +        */
> +       ret = exynos_bus_enable_edev(bus);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to enable devfreq-event devices\n");
> +               goto err;
> +       }
> +
> +       ret = exynos_bus_set_event(bus);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to set event to devfreq-event devices\n");
> +               goto err;
> +       }
> +
> +err:
> +       return ret;
> +}
> +
>  static int exynos_bus_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
>         struct device_node *np = dev->of_node, *node;
>         struct devfreq_dev_profile *profile;
> -       struct devfreq_simple_ondemand_data *ondemand_data;
>         struct devfreq_passive_data *passive_data;
>         struct devfreq *parent_devfreq;
>         struct exynos_bus *bus;
> @@ -418,52 +475,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
>         if (ret < 0)
>                 goto err;
>
> -       /* Initialize the struct profile and governor data for parent device */
> -       profile->polling_ms = 50;
> -       profile->target = exynos_bus_target;
> -       profile->get_dev_status = exynos_bus_get_dev_status;
> -       profile->exit = exynos_bus_exit;
> -
> -       ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> -       if (!ondemand_data) {
> -               ret = -ENOMEM;
> +       ret = exynos_bus_profile_init(bus, profile);
> +       if (ret < 0)
>                 goto err;
> -       }
> -       ondemand_data->upthreshold = 40;
> -       ondemand_data->downdifferential = 5;
> -
> -       /* Add devfreq device to monitor and handle the exynos bus */
> -       bus->devfreq = devm_devfreq_add_device(dev, profile,
> -                                               DEVFREQ_GOV_SIMPLE_ONDEMAND,
> -                                               ondemand_data);
> -       if (IS_ERR(bus->devfreq)) {
> -               dev_err(dev, "failed to add devfreq device\n");
> -               ret = PTR_ERR(bus->devfreq);
> -               goto err;
> -       }
> -
> -       /* Register opp_notifier to catch the change of OPP  */
> -       ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to register opp notifier\n");
> -               goto err;
> -       }
> -
> -       /*
> -        * Enable devfreq-event to get raw data which is used to determine
> -        * current bus load.
> -        */
> -       ret = exynos_bus_enable_edev(bus);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to enable devfreq-event devices\n");
> -               goto err;
> -       }
> -
> -       ret = exynos_bus_set_event(bus);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to set event to devfreq-event devices\n");
> -               goto err;
> -       }
>
>         goto out;
>  passive:
> --
> 2.17.1
>

NACK.

It has not any benefit and I don't understand reason why it is necessary.
I don't agree. Please drop it.

-- 
Best Regards,
Chanwoo Choi

Re: [RFC PATCH 01/11] devfreq: exynos-bus: Extract exynos_bus_profile_init()

[Krzysztof Kozlowski]

On Thu, 25 Jul 2019 at 14:44, Chanwoo Choi <cwchoi00@gmail.com> wrote:
>
> 2019년 7월 24일 (수) 오전 8:09, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
> >
> > This patch adds a new static function, exynos_bus_profile_init(), extracted
> > from exynos_bus_probe().
> >
> > Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> > ---
> >  drivers/devfreq/exynos-bus.c | 106 ++++++++++++++++++++---------------
> >  1 file changed, 60 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index d9f377912c10..d8f1efaf2d49 100644
> > --- a/drivers/devfreq/exynos-bus.c
> > +++ b/drivers/devfreq/exynos-bus.c
> > @@ -372,12 +372,69 @@ static int exynos_bus_parse_of(struct device_node *np,
> >         return ret;
> >  }
> >
> > +static int exynos_bus_profile_init(struct exynos_bus *bus,
> > +                                  struct devfreq_dev_profile *profile)
> > +{
> > +       struct device *dev = bus->dev;
> > +       struct devfreq_simple_ondemand_data *ondemand_data;
> > +       int ret;
> > +
> > +       /* Initialize the struct profile and governor data for parent device */
> > +       profile->polling_ms = 50;
> > +       profile->target = exynos_bus_target;
> > +       profile->get_dev_status = exynos_bus_get_dev_status;
> > +       profile->exit = exynos_bus_exit;
> > +
> > +       ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> > +       if (!ondemand_data) {
> > +               ret = -ENOMEM;
> > +               goto err;
> > +       }
> > +       ondemand_data->upthreshold = 40;
> > +       ondemand_data->downdifferential = 5;
> > +
> > +       /* Add devfreq device to monitor and handle the exynos bus */
> > +       bus->devfreq = devm_devfreq_add_device(dev, profile,
> > +                                               DEVFREQ_GOV_SIMPLE_ONDEMAND,
> > +                                               ondemand_data);
> > +       if (IS_ERR(bus->devfreq)) {
> > +               dev_err(dev, "failed to add devfreq device\n");
> > +               ret = PTR_ERR(bus->devfreq);
> > +               goto err;
> > +       }
> > +
> > +       /* Register opp_notifier to catch the change of OPP  */
> > +       ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
> > +       if (ret < 0) {
> > +               dev_err(dev, "failed to register opp notifier\n");
> > +               goto err;
> > +       }
> > +
> > +       /*
> > +        * Enable devfreq-event to get raw data which is used to determine
> > +        * current bus load.
> > +        */
> > +       ret = exynos_bus_enable_edev(bus);
> > +       if (ret < 0) {
> > +               dev_err(dev, "failed to enable devfreq-event devices\n");
> > +               goto err;
> > +       }
> > +
> > +       ret = exynos_bus_set_event(bus);
> > +       if (ret < 0) {
> > +               dev_err(dev, "failed to set event to devfreq-event devices\n");
> > +               goto err;
> > +       }
> > +
> > +err:
> > +       return ret;
> > +}
> > +
> >  static int exynos_bus_probe(struct platform_device *pdev)
> >  {
> >         struct device *dev = &pdev->dev;
> >         struct device_node *np = dev->of_node, *node;
> >         struct devfreq_dev_profile *profile;
> > -       struct devfreq_simple_ondemand_data *ondemand_data;
> >         struct devfreq_passive_data *passive_data;
> >         struct devfreq *parent_devfreq;
> >         struct exynos_bus *bus;
> > @@ -418,52 +475,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
> >         if (ret < 0)
> >                 goto err;
> >
> > -       /* Initialize the struct profile and governor data for parent device */
> > -       profile->polling_ms = 50;
> > -       profile->target = exynos_bus_target;
> > -       profile->get_dev_status = exynos_bus_get_dev_status;
> > -       profile->exit = exynos_bus_exit;
> > -
> > -       ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> > -       if (!ondemand_data) {
> > -               ret = -ENOMEM;
> > +       ret = exynos_bus_profile_init(bus, profile);
> > +       if (ret < 0)
> >                 goto err;
> > -       }
> > -       ondemand_data->upthreshold = 40;
> > -       ondemand_data->downdifferential = 5;
> > -
> > -       /* Add devfreq device to monitor and handle the exynos bus */
> > -       bus->devfreq = devm_devfreq_add_device(dev, profile,
> > -                                               DEVFREQ_GOV_SIMPLE_ONDEMAND,
> > -                                               ondemand_data);
> > -       if (IS_ERR(bus->devfreq)) {
> > -               dev_err(dev, "failed to add devfreq device\n");
> > -               ret = PTR_ERR(bus->devfreq);
> > -               goto err;
> > -       }
> > -
> > -       /* Register opp_notifier to catch the change of OPP  */
> > -       ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
> > -       if (ret < 0) {
> > -               dev_err(dev, "failed to register opp notifier\n");
> > -               goto err;
> > -       }
> > -
> > -       /*
> > -        * Enable devfreq-event to get raw data which is used to determine
> > -        * current bus load.
> > -        */
> > -       ret = exynos_bus_enable_edev(bus);
> > -       if (ret < 0) {
> > -               dev_err(dev, "failed to enable devfreq-event devices\n");
> > -               goto err;
> > -       }
> > -
> > -       ret = exynos_bus_set_event(bus);
> > -       if (ret < 0) {
> > -               dev_err(dev, "failed to set event to devfreq-event devices\n");
> > -               goto err;
> > -       }
> >
> >         goto out;
> >  passive:
> > --
> > 2.17.1
> >
>
> NACK.
>
> It has not any benefit and I don't understand reason why it is necessary.
> I don't agree. Please drop it.

The probe has 12 local variables and around 140 lines of code (so much
more than coding style recommendations). Therefore splitting some
logical part out of probe to make code better organized and more
readable is pretty obvious benefit.

Best regards,
Krzysztof

Re: [RFC PATCH 01/11] devfreq: exynos-bus: Extract exynos_bus_profile_init()

[Chanwoo Choi]

On 19. 7. 26. 오후 7:42, Krzysztof Kozlowski wrote:
> On Thu, 25 Jul 2019 at 14:44, Chanwoo Choi <cwchoi00@gmail.com> wrote:
>>
>> 2019년 7월 24일 (수) 오전 8:09, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
>>>
>>> This patch adds a new static function, exynos_bus_profile_init(), extracted
>>> from exynos_bus_probe().
>>>
>>> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
>>> ---
>>>  drivers/devfreq/exynos-bus.c | 106 ++++++++++++++++++++---------------
>>>  1 file changed, 60 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>> index d9f377912c10..d8f1efaf2d49 100644
>>> --- a/drivers/devfreq/exynos-bus.c
>>> +++ b/drivers/devfreq/exynos-bus.c
>>> @@ -372,12 +372,69 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>         return ret;
>>>  }
>>>
>>> +static int exynos_bus_profile_init(struct exynos_bus *bus,
>>> +                                  struct devfreq_dev_profile *profile)
>>> +{
>>> +       struct device *dev = bus->dev;
>>> +       struct devfreq_simple_ondemand_data *ondemand_data;
>>> +       int ret;
>>> +
>>> +       /* Initialize the struct profile and governor data for parent device */
>>> +       profile->polling_ms = 50;
>>> +       profile->target = exynos_bus_target;
>>> +       profile->get_dev_status = exynos_bus_get_dev_status;
>>> +       profile->exit = exynos_bus_exit;
>>> +
>>> +       ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
>>> +       if (!ondemand_data) {
>>> +               ret = -ENOMEM;
>>> +               goto err;
>>> +       }
>>> +       ondemand_data->upthreshold = 40;
>>> +       ondemand_data->downdifferential = 5;
>>> +
>>> +       /* Add devfreq device to monitor and handle the exynos bus */
>>> +       bus->devfreq = devm_devfreq_add_device(dev, profile,
>>> +                                               DEVFREQ_GOV_SIMPLE_ONDEMAND,
>>> +                                               ondemand_data);
>>> +       if (IS_ERR(bus->devfreq)) {
>>> +               dev_err(dev, "failed to add devfreq device\n");
>>> +               ret = PTR_ERR(bus->devfreq);
>>> +               goto err;
>>> +       }
>>> +
>>> +       /* Register opp_notifier to catch the change of OPP  */
>>> +       ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
>>> +       if (ret < 0) {
>>> +               dev_err(dev, "failed to register opp notifier\n");
>>> +               goto err;
>>> +       }
>>> +
>>> +       /*
>>> +        * Enable devfreq-event to get raw data which is used to determine
>>> +        * current bus load.
>>> +        */
>>> +       ret = exynos_bus_enable_edev(bus);
>>> +       if (ret < 0) {
>>> +               dev_err(dev, "failed to enable devfreq-event devices\n");
>>> +               goto err;
>>> +       }
>>> +
>>> +       ret = exynos_bus_set_event(bus);
>>> +       if (ret < 0) {
>>> +               dev_err(dev, "failed to set event to devfreq-event devices\n");
>>> +               goto err;
>>> +       }
>>> +
>>> +err:
>>> +       return ret;
>>> +}
>>> +
>>>  static int exynos_bus_probe(struct platform_device *pdev)
>>>  {
>>>         struct device *dev = &pdev->dev;
>>>         struct device_node *np = dev->of_node, *node;
>>>         struct devfreq_dev_profile *profile;
>>> -       struct devfreq_simple_ondemand_data *ondemand_data;
>>>         struct devfreq_passive_data *passive_data;
>>>         struct devfreq *parent_devfreq;
>>>         struct exynos_bus *bus;
>>> @@ -418,52 +475,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>         if (ret < 0)
>>>                 goto err;
>>>
>>> -       /* Initialize the struct profile and governor data for parent device */
>>> -       profile->polling_ms = 50;
>>> -       profile->target = exynos_bus_target;
>>> -       profile->get_dev_status = exynos_bus_get_dev_status;
>>> -       profile->exit = exynos_bus_exit;
>>> -
>>> -       ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
>>> -       if (!ondemand_data) {
>>> -               ret = -ENOMEM;
>>> +       ret = exynos_bus_profile_init(bus, profile);
>>> +       if (ret < 0)
>>>                 goto err;
>>> -       }
>>> -       ondemand_data->upthreshold = 40;
>>> -       ondemand_data->downdifferential = 5;
>>> -
>>> -       /* Add devfreq device to monitor and handle the exynos bus */
>>> -       bus->devfreq = devm_devfreq_add_device(dev, profile,
>>> -                                               DEVFREQ_GOV_SIMPLE_ONDEMAND,
>>> -                                               ondemand_data);
>>> -       if (IS_ERR(bus->devfreq)) {
>>> -               dev_err(dev, "failed to add devfreq device\n");
>>> -               ret = PTR_ERR(bus->devfreq);
>>> -               goto err;
>>> -       }
>>> -
>>> -       /* Register opp_notifier to catch the change of OPP  */
>>> -       ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
>>> -       if (ret < 0) {
>>> -               dev_err(dev, "failed to register opp notifier\n");
>>> -               goto err;
>>> -       }
>>> -
>>> -       /*
>>> -        * Enable devfreq-event to get raw data which is used to determine
>>> -        * current bus load.
>>> -        */
>>> -       ret = exynos_bus_enable_edev(bus);
>>> -       if (ret < 0) {
>>> -               dev_err(dev, "failed to enable devfreq-event devices\n");
>>> -               goto err;
>>> -       }
>>> -
>>> -       ret = exynos_bus_set_event(bus);
>>> -       if (ret < 0) {
>>> -               dev_err(dev, "failed to set event to devfreq-event devices\n");
>>> -               goto err;
>>> -       }
>>>
>>>         goto out;
>>>  passive:
>>> --
>>> 2.17.1
>>>
>>
>> NACK.
>>
>> It has not any benefit and I don't understand reason why it is necessary.
>> I don't agree. Please drop it.
> 
> The probe has 12 local variables and around 140 lines of code (so much
> more than coding style recommendations). Therefore splitting some
> logical part out of probe to make code better organized and more
> readable is pretty obvious benefit.

After checked the patch3, I changed my opinion. It seems more simple than before
and I replied on patch3. But, I think that can merge patch1/2/2 to one patch.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[RFC PATCH 02/11] devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()

[Artur Świgoń]

This patch adds a new static function, exynos_bus_profile_init_passive(),
extracted from exynos_bus_probe().

Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
 drivers/devfreq/exynos-bus.c | 70 +++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 28 deletions(-)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index d8f1efaf2d49..cf6f6cbd0f55 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -430,13 +430,51 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
 	return ret;
 }
 
+static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
+					   struct devfreq_dev_profile *profile)
+{
+	struct device *dev = bus->dev;
+	struct devfreq *parent_devfreq;
+	struct devfreq_passive_data *passive_data;
+	int ret = 0;
+
+	/* Initialize the struct profile and governor data for passive device */
+	profile->target = exynos_bus_passive_target;
+	profile->exit = exynos_bus_passive_exit;
+
+	/* Get the instance of parent devfreq device */
+	parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (IS_ERR(parent_devfreq)) {
+		ret = -EPROBE_DEFER;
+		goto err;
+	}
+
+	passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
+	if (!passive_data) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	passive_data->parent = parent_devfreq;
+
+	/* Add devfreq device for exynos bus with passive governor */
+	bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
+						passive_data);
+	if (IS_ERR(bus->devfreq)) {
+		dev_err(dev,
+			"failed to add devfreq dev with passive governor\n");
+		ret = PTR_ERR(bus->devfreq);
+		goto err;
+	}
+
+err:
+	return ret;
+}
+
 static int exynos_bus_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node, *node;
 	struct devfreq_dev_profile *profile;
-	struct devfreq_passive_data *passive_data;
-	struct devfreq *parent_devfreq;
 	struct exynos_bus *bus;
 	int ret, max_state;
 	unsigned long min_freq, max_freq;
@@ -481,33 +519,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
 
 	goto out;
 passive:
-	/* Initialize the struct profile and governor data for passive device */
-	profile->target = exynos_bus_passive_target;
-	profile->exit = exynos_bus_passive_exit;
-
-	/* Get the instance of parent devfreq device */
-	parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
-	if (IS_ERR(parent_devfreq)) {
-		ret = -EPROBE_DEFER;
+	ret = exynos_bus_profile_init_passive(bus, profile);
+	if (ret < 0)
 		goto err;
-	}
-
-	passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
-	if (!passive_data) {
-		ret = -ENOMEM;
-		goto err;
-	}
-	passive_data->parent = parent_devfreq;
-
-	/* Add devfreq device for exynos bus with passive governor */
-	bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
-						passive_data);
-	if (IS_ERR(bus->devfreq)) {
-		dev_err(dev,
-			"failed to add devfreq dev with passive governor\n");
-		ret = PTR_ERR(bus->devfreq);
-		goto err;
-	}
 
 out:
 	max_state = bus->devfreq->profile->max_state;
-- 
2.17.1

Re: [RFC PATCH 02/11] devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()

[Krzysztof Kozlowski]

On Tue, Jul 23, 2019 at 02:20:07PM +0200, Artur Świgoń wrote:
> This patch adds a new static function, exynos_bus_profile_init_passive(),
> extracted from exynos_bus_probe().
> 
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 70 +++++++++++++++++++++---------------
>  1 file changed, 42 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index d8f1efaf2d49..cf6f6cbd0f55 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -430,13 +430,51 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>  	return ret;
>  }
>  
> +static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> +					   struct devfreq_dev_profile *profile)
> +{
> +	struct device *dev = bus->dev;
> +	struct devfreq *parent_devfreq;
> +	struct devfreq_passive_data *passive_data;
> +	int ret = 0;
> +
> +	/* Initialize the struct profile and governor data for passive device */
> +	profile->target = exynos_bus_passive_target;
> +	profile->exit = exynos_bus_passive_exit;
> +
> +	/* Get the instance of parent devfreq device */
> +	parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
> +	if (IS_ERR(parent_devfreq)) {
> +		ret = -EPROBE_DEFER;
> +		goto err;

Same as in previous patch - no need for error goto.

Best regards,
Krzysztof

Re: [RFC PATCH 02/11] devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()

[Chanwoo Choi]

2019년 7월 24일 (수) 오전 8:07, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
>
> This patch adds a new static function, exynos_bus_profile_init_passive(),
> extracted from exynos_bus_probe().
>
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 70 +++++++++++++++++++++---------------
>  1 file changed, 42 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index d8f1efaf2d49..cf6f6cbd0f55 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -430,13 +430,51 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>         return ret;
>  }
>
> +static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> +                                          struct devfreq_dev_profile *profile)
> +{
> +       struct device *dev = bus->dev;
> +       struct devfreq *parent_devfreq;
> +       struct devfreq_passive_data *passive_data;
> +       int ret = 0;
> +
> +       /* Initialize the struct profile and governor data for passive device */
> +       profile->target = exynos_bus_passive_target;
> +       profile->exit = exynos_bus_passive_exit;
> +
> +       /* Get the instance of parent devfreq device */
> +       parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
> +       if (IS_ERR(parent_devfreq)) {
> +               ret = -EPROBE_DEFER;
> +               goto err;
> +       }
> +
> +       passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
> +       if (!passive_data) {
> +               ret = -ENOMEM;
> +               goto err;
> +       }
> +       passive_data->parent = parent_devfreq;
> +
> +       /* Add devfreq device for exynos bus with passive governor */
> +       bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
> +                                               passive_data);
> +       if (IS_ERR(bus->devfreq)) {
> +               dev_err(dev,
> +                       "failed to add devfreq dev with passive governor\n");
> +               ret = PTR_ERR(bus->devfreq);
> +               goto err;
> +       }
> +
> +err:
> +       return ret;
> +}
> +
>  static int exynos_bus_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
>         struct device_node *np = dev->of_node, *node;
>         struct devfreq_dev_profile *profile;
> -       struct devfreq_passive_data *passive_data;
> -       struct devfreq *parent_devfreq;
>         struct exynos_bus *bus;
>         int ret, max_state;
>         unsigned long min_freq, max_freq;
> @@ -481,33 +519,9 @@ static int exynos_bus_probe(struct platform_device *pdev)
>
>         goto out;
>  passive:
> -       /* Initialize the struct profile and governor data for passive device */
> -       profile->target = exynos_bus_passive_target;
> -       profile->exit = exynos_bus_passive_exit;
> -
> -       /* Get the instance of parent devfreq device */
> -       parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
> -       if (IS_ERR(parent_devfreq)) {
> -               ret = -EPROBE_DEFER;
> +       ret = exynos_bus_profile_init_passive(bus, profile);
> +       if (ret < 0)
>                 goto err;
> -       }
> -
> -       passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
> -       if (!passive_data) {
> -               ret = -ENOMEM;
> -               goto err;
> -       }
> -       passive_data->parent = parent_devfreq;
> -
> -       /* Add devfreq device for exynos bus with passive governor */
> -       bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
> -                                               passive_data);
> -       if (IS_ERR(bus->devfreq)) {
> -               dev_err(dev,
> -                       "failed to add devfreq dev with passive governor\n");
> -               ret = PTR_ERR(bus->devfreq);
> -               goto err;
> -       }
>
>  out:
>         max_state = bus->devfreq->profile->max_state;
> --
> 2.17.1
>

Actually, it is not necessary. It has no any benefit.
Please drop it as I commented on patch1.


--
Best Regards,
Chanwoo Choi

[RFC PATCH 03/11] devfreq: exynos-bus: Change goto-based logic to if-else logic

[Artur Świgoń]

This patch improves code readability by changing the following construct:

>    if (cond)
>        goto passive;
>    foo();
>    goto out;
>passive:
>    bar();
>out:

into this:

>    if (cond)
>        bar();
>    else
>        foo();

Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
 drivers/devfreq/exynos-bus.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index cf6f6cbd0f55..4bb83b945bf7 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -505,25 +505,19 @@ static int exynos_bus_probe(struct platform_device *pdev)
 	node = of_parse_phandle(dev->of_node, "devfreq", 0);
 	if (node) {
 		of_node_put(node);
-		goto passive;
+		ret = exynos_bus_profile_init_passive(bus, profile);
+		if (ret < 0)
+			goto err;
 	} else {
 		ret = exynos_bus_parent_parse_of(np, bus);
+		if (ret < 0)
+			goto err;
+
+		ret = exynos_bus_profile_init(bus, profile);
+		if (ret < 0)
+			goto err;
 	}
 
-	if (ret < 0)
-		goto err;
-
-	ret = exynos_bus_profile_init(bus, profile);
-	if (ret < 0)
-		goto err;
-
-	goto out;
-passive:
-	ret = exynos_bus_profile_init_passive(bus, profile);
-	if (ret < 0)
-		goto err;
-
-out:
 	max_state = bus->devfreq->profile->max_state;
 	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
 	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
-- 
2.17.1

Re: [RFC PATCH 03/11] devfreq: exynos-bus: Change goto-based logic to if-else logic

[Krzysztof Kozlowski]

On Tue, Jul 23, 2019 at 02:20:08PM +0200, Artur Świgoń wrote:
> This patch improves code readability by changing the following construct:
> 
> >    if (cond)
> >        goto passive;
> >    foo();
> >    goto out;
> >passive:
> >    bar();
> >out:
> 
> into this:
> 
> >    if (cond)
> >        bar();
> >    else
> >        foo();
> 
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)

Code looks much better:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [RFC PATCH 03/11] devfreq: exynos-bus: Change goto-based logic to if-else logic

[Chanwoo Choi]

2019년 7월 24일 (수) 오전 8:08, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
>
> This patch improves code readability by changing the following construct:
>
> >    if (cond)
> >        goto passive;
> >    foo();
> >    goto out;
> >passive:
> >    bar();
> >out:
>
> into this:
>
> >    if (cond)
> >        bar();
> >    else
> >        foo();
>
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index cf6f6cbd0f55..4bb83b945bf7 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -505,25 +505,19 @@ static int exynos_bus_probe(struct platform_device *pdev)
>         node = of_parse_phandle(dev->of_node, "devfreq", 0);
>         if (node) {
>                 of_node_put(node);
> -               goto passive;
> +               ret = exynos_bus_profile_init_passive(bus, profile);
> +               if (ret < 0)
> +                       goto err;
>         } else {
>                 ret = exynos_bus_parent_parse_of(np, bus);
> +               if (ret < 0)
> +                       goto err;
> +
> +               ret = exynos_bus_profile_init(bus, profile);
> +               if (ret < 0)
> +                       goto err;
>         }
>
> -       if (ret < 0)
> -               goto err;
> -
> -       ret = exynos_bus_profile_init(bus, profile);
> -       if (ret < 0)
> -               goto err;
> -
> -       goto out;
> -passive:
> -       ret = exynos_bus_profile_init_passive(bus, profile);
> -       if (ret < 0)
> -               goto err;
> -
> -out:
>         max_state = bus->devfreq->profile->max_state;
>         min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
>         max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
> --
> 2.17.1
>

It seems more simple than before.
Instead, please merge patch1/2/3 to one patch. and drop the patch4.

-- 
Best Regards,
Chanwoo Choi

Re: [RFC PATCH 03/11] devfreq: exynos-bus: Change goto-based logic to if-else logic

[Chanwoo Choi]

2019년 7월 25일 (목) 오후 9:56, Chanwoo Choi <cwchoi00@gmail.com>님이 작성:
>
> 2019년 7월 24일 (수) 오전 8:08, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
> >
> > This patch improves code readability by changing the following construct:
> >
> > >    if (cond)
> > >        goto passive;
> > >    foo();
> > >    goto out;
> > >passive:
> > >    bar();
> > >out:
> >
> > into this:
> >
> > >    if (cond)
> > >        bar();
> > >    else
> > >        foo();
> >
> > Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> > ---
> >  drivers/devfreq/exynos-bus.c | 24 +++++++++---------------
> >  1 file changed, 9 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index cf6f6cbd0f55..4bb83b945bf7 100644
> > --- a/drivers/devfreq/exynos-bus.c
> > +++ b/drivers/devfreq/exynos-bus.c
> > @@ -505,25 +505,19 @@ static int exynos_bus_probe(struct platform_device *pdev)
> >         node = of_parse_phandle(dev->of_node, "devfreq", 0);
> >         if (node) {
> >                 of_node_put(node);
> > -               goto passive;
> > +               ret = exynos_bus_profile_init_passive(bus, profile);
> > +               if (ret < 0)
> > +                       goto err;
> >         } else {
> >                 ret = exynos_bus_parent_parse_of(np, bus);
> > +               if (ret < 0)
> > +                       goto err;
> > +
> > +               ret = exynos_bus_profile_init(bus, profile);
> > +               if (ret < 0)
> > +                       goto err;
> >         }
> >
> > -       if (ret < 0)
> > -               goto err;
> > -
> > -       ret = exynos_bus_profile_init(bus, profile);
> > -       if (ret < 0)
> > -               goto err;
> > -
> > -       goto out;
> > -passive:
> > -       ret = exynos_bus_profile_init_passive(bus, profile);
> > -       if (ret < 0)
> > -               goto err;
> > -
> > -out:
> >         max_state = bus->devfreq->profile->max_state;
> >         min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
> >         max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
> > --
> > 2.17.1
> >
>
> It seems more simple than before.
> Instead, please merge patch1/2/3 to one patch. and drop the patch4.

But, I think that you better to drop the cleanup patch from this series
because the series[1] touch the exynos-bus.c driver for coupled regulator.
[1] https://www.spinics.net/lists/arm-kernel/msg741971.html

I recommend that you send the cleanup patch with my comment
either after reviewing the Kamil's patch[1] or rebase this series base
on patch[1].

>
> --
> Best Regards,
> Chanwoo Choi



-- 
Best Regards,
Chanwoo Choi

[RFC PATCH 04/11] devfreq: exynos-bus: Clean up code

[Artur Świgoń]

This patch adds minor improvements to the exynos-bus driver.

Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
 drivers/devfreq/exynos-bus.c | 49 ++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 27 deletions(-)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 4bb83b945bf7..412511ca7703 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -15,11 +15,10 @@
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/module.h>
-#include <linux/of_device.h>
+#include <linux/of.h>
 #include <linux/pm_opp.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
-#include <linux/slab.h>
 
 #define DEFAULT_SATURATION_RATIO	40
 #define DEFAULT_VOLTAGE_TOLERANCE	2
@@ -256,7 +255,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
 					struct exynos_bus *bus)
 {
 	struct device *dev = bus->dev;
-	int i, ret, count, size;
+	int i, ret, count;
 
 	/* Get the regulator to provide each bus with the power */
 	bus->regulator = devm_regulator_get(dev, "vdd");
@@ -283,8 +282,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
 	}
 	bus->edev_count = count;
 
-	size = sizeof(*bus->edev) * count;
-	bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
+	bus->edev = devm_kcalloc(dev, count, sizeof(*bus->edev), GFP_KERNEL);
 	if (!bus->edev) {
 		ret = -ENOMEM;
 		goto err_regulator;
@@ -388,7 +386,7 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
 	ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
 	if (!ondemand_data) {
 		ret = -ENOMEM;
-		goto err;
+		goto out;
 	}
 	ondemand_data->upthreshold = 40;
 	ondemand_data->downdifferential = 5;
@@ -400,14 +398,14 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
 	if (IS_ERR(bus->devfreq)) {
 		dev_err(dev, "failed to add devfreq device\n");
 		ret = PTR_ERR(bus->devfreq);
-		goto err;
+		goto out;
 	}
 
 	/* Register opp_notifier to catch the change of OPP  */
 	ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
 	if (ret < 0) {
 		dev_err(dev, "failed to register opp notifier\n");
-		goto err;
+		goto out;
 	}
 
 	/*
@@ -417,16 +415,16 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
 	ret = exynos_bus_enable_edev(bus);
 	if (ret < 0) {
 		dev_err(dev, "failed to enable devfreq-event devices\n");
-		goto err;
+		goto out;
 	}
 
 	ret = exynos_bus_set_event(bus);
 	if (ret < 0) {
 		dev_err(dev, "failed to set event to devfreq-event devices\n");
-		goto err;
+		goto out;
 	}
 
-err:
+out:
 	return ret;
 }
 
@@ -446,27 +444,28 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
 	parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
 	if (IS_ERR(parent_devfreq)) {
 		ret = -EPROBE_DEFER;
-		goto err;
+		goto out;
 	}
 
 	passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
 	if (!passive_data) {
 		ret = -ENOMEM;
-		goto err;
+		goto out;
 	}
 	passive_data->parent = parent_devfreq;
 
 	/* Add devfreq device for exynos bus with passive governor */
-	bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
+	bus->devfreq = devm_devfreq_add_device(dev, profile,
+						DEVFREQ_GOV_PASSIVE,
 						passive_data);
 	if (IS_ERR(bus->devfreq)) {
 		dev_err(dev,
 			"failed to add devfreq dev with passive governor\n");
 		ret = PTR_ERR(bus->devfreq);
-		goto err;
+		goto out;
 	}
 
-err:
+out:
 	return ret;
 }
 
@@ -484,11 +483,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
+	bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
 	if (!bus)
 		return -ENOMEM;
 	mutex_init(&bus->lock);
-	bus->dev = &pdev->dev;
+	bus->dev = dev;
 	platform_set_drvdata(pdev, bus);
 
 	/* Parse the device-tree to get the resource information */
@@ -502,7 +501,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	node = of_parse_phandle(dev->of_node, "devfreq", 0);
+	node = of_parse_phandle(np, "devfreq", 0);
 	if (node) {
 		of_node_put(node);
 		ret = exynos_bus_profile_init_passive(bus, profile);
@@ -547,12 +546,10 @@ static int exynos_bus_resume(struct device *dev)
 	int ret;
 
 	ret = exynos_bus_enable_edev(bus);
-	if (ret < 0) {
+	if (ret < 0)
 		dev_err(dev, "failed to enable the devfreq-event devices\n");
-		return ret;
-	}
 
-	return 0;
+	return ret;
 }
 
 static int exynos_bus_suspend(struct device *dev)
@@ -561,12 +558,10 @@ static int exynos_bus_suspend(struct device *dev)
 	int ret;
 
 	ret = exynos_bus_disable_edev(bus);
-	if (ret < 0) {
+	if (ret < 0)
 		dev_err(dev, "failed to disable the devfreq-event devices\n");
-		return ret;
-	}
 
-	return 0;
+	return ret;
 }
 #endif
 
-- 
2.17.1

Re: [RFC PATCH 04/11] devfreq: exynos-bus: Clean up code

[Krzysztof Kozlowski]

On Tue, Jul 23, 2019 at 02:20:09PM +0200, Artur Świgoń wrote:
> This patch adds minor improvements to the exynos-bus driver.
> 
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 49 ++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 27 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [RFC PATCH 04/11] devfreq: exynos-bus: Clean up code

[Chanwoo Choi]

2019년 7월 24일 (수) 오전 8:07, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
>
> This patch adds minor improvements to the exynos-bus driver.
>
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 49 ++++++++++++++++--------------------
>  1 file changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 4bb83b945bf7..412511ca7703 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -15,11 +15,10 @@
>  #include <linux/device.h>
>  #include <linux/export.h>
>  #include <linux/module.h>
> -#include <linux/of_device.h>
> +#include <linux/of.h>
>  #include <linux/pm_opp.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
> -#include <linux/slab.h>
>
>  #define DEFAULT_SATURATION_RATIO       40
>  #define DEFAULT_VOLTAGE_TOLERANCE      2
> @@ -256,7 +255,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>                                         struct exynos_bus *bus)
>  {
>         struct device *dev = bus->dev;
> -       int i, ret, count, size;
> +       int i, ret, count;
>
>         /* Get the regulator to provide each bus with the power */
>         bus->regulator = devm_regulator_get(dev, "vdd");
> @@ -283,8 +282,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>         }
>         bus->edev_count = count;
>
> -       size = sizeof(*bus->edev) * count;
> -       bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
> +       bus->edev = devm_kcalloc(dev, count, sizeof(*bus->edev), GFP_KERNEL);
>         if (!bus->edev) {
>                 ret = -ENOMEM;
>                 goto err_regulator;
> @@ -388,7 +386,7 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>         ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
>         if (!ondemand_data) {
>                 ret = -ENOMEM;
> -               goto err;
> +               goto out;
>         }
>         ondemand_data->upthreshold = 40;
>         ondemand_data->downdifferential = 5;
> @@ -400,14 +398,14 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>         if (IS_ERR(bus->devfreq)) {
>                 dev_err(dev, "failed to add devfreq device\n");
>                 ret = PTR_ERR(bus->devfreq);
> -               goto err;
> +               goto out;
>         }
>
>         /* Register opp_notifier to catch the change of OPP  */
>         ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
>         if (ret < 0) {
>                 dev_err(dev, "failed to register opp notifier\n");
> -               goto err;
> +               goto out;
>         }
>
>         /*
> @@ -417,16 +415,16 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>         ret = exynos_bus_enable_edev(bus);
>         if (ret < 0) {
>                 dev_err(dev, "failed to enable devfreq-event devices\n");
> -               goto err;
> +               goto out;
>         }
>
>         ret = exynos_bus_set_event(bus);
>         if (ret < 0) {
>                 dev_err(dev, "failed to set event to devfreq-event devices\n");
> -               goto err;
> +               goto out;
>         }
>
> -err:
> +out:
>         return ret;
>  }
>
> @@ -446,27 +444,28 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>         parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
>         if (IS_ERR(parent_devfreq)) {
>                 ret = -EPROBE_DEFER;
> -               goto err;
> +               goto out;
>         }
>
>         passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
>         if (!passive_data) {
>                 ret = -ENOMEM;
> -               goto err;
> +               goto out;
>         }
>         passive_data->parent = parent_devfreq;
>
>         /* Add devfreq device for exynos bus with passive governor */
> -       bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
> +       bus->devfreq = devm_devfreq_add_device(dev, profile,
> +                                               DEVFREQ_GOV_PASSIVE,
>                                                 passive_data);
>         if (IS_ERR(bus->devfreq)) {
>                 dev_err(dev,
>                         "failed to add devfreq dev with passive governor\n");
>                 ret = PTR_ERR(bus->devfreq);
> -               goto err;
> +               goto out;
>         }
>
> -err:
> +out:
>         return ret;
>  }
>
> @@ -484,11 +483,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>                 return -EINVAL;
>         }
>
> -       bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> +       bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
>         if (!bus)
>                 return -ENOMEM;
>         mutex_init(&bus->lock);
> -       bus->dev = &pdev->dev;
> +       bus->dev = dev;
>         platform_set_drvdata(pdev, bus);
>
>         /* Parse the device-tree to get the resource information */
> @@ -502,7 +501,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>                 goto err;
>         }
>
> -       node = of_parse_phandle(dev->of_node, "devfreq", 0);
> +       node = of_parse_phandle(np, "devfreq", 0);
>         if (node) {
>                 of_node_put(node);
>                 ret = exynos_bus_profile_init_passive(bus, profile);
> @@ -547,12 +546,10 @@ static int exynos_bus_resume(struct device *dev)
>         int ret;
>
>         ret = exynos_bus_enable_edev(bus);
> -       if (ret < 0) {
> +       if (ret < 0)
>                 dev_err(dev, "failed to enable the devfreq-event devices\n");
> -               return ret;
> -       }
>
> -       return 0;
> +       return ret;
>  }
>
>  static int exynos_bus_suspend(struct device *dev)
> @@ -561,12 +558,10 @@ static int exynos_bus_suspend(struct device *dev)
>         int ret;
>

NACK.
>         ret = exynos_bus_disable_edev(bus);
> -       if (ret < 0) {
> +       if (ret < 0)
>                 dev_err(dev, "failed to disable the devfreq-event devices\n");
> -               return ret;
> -       }
>
> -       return 0;
> +       return ret;
>  }
>  #endif
>
> --
> 2.17.1
>

NACK.

As I already commented, It has never any benefit.
I think that it is not usful. Please drop it.



-- 
Best Regards,
Chanwoo Choi

Re: [RFC PATCH 04/11] devfreq: exynos-bus: Clean up code

[Krzysztof Kozlowski]

On Thu, 25 Jul 2019 at 14:51, Chanwoo Choi <cwchoi00@gmail.com> wrote:
>
> 2019년 7월 24일 (수) 오전 8:07, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
> >
> > This patch adds minor improvements to the exynos-bus driver.
> >
> > Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> > ---
> >  drivers/devfreq/exynos-bus.c | 49 ++++++++++++++++--------------------
> >  1 file changed, 22 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index 4bb83b945bf7..412511ca7703 100644
> > --- a/drivers/devfreq/exynos-bus.c
> > +++ b/drivers/devfreq/exynos-bus.c
> > @@ -15,11 +15,10 @@
> >  #include <linux/device.h>
> >  #include <linux/export.h>
> >  #include <linux/module.h>
> > -#include <linux/of_device.h>
> > +#include <linux/of.h>
> >  #include <linux/pm_opp.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regulator/consumer.h>
> > -#include <linux/slab.h>
> >
> >  #define DEFAULT_SATURATION_RATIO       40
> >  #define DEFAULT_VOLTAGE_TOLERANCE      2
> > @@ -256,7 +255,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> >                                         struct exynos_bus *bus)
> >  {
> >         struct device *dev = bus->dev;
> > -       int i, ret, count, size;
> > +       int i, ret, count;
> >
> >         /* Get the regulator to provide each bus with the power */
> >         bus->regulator = devm_regulator_get(dev, "vdd");
> > @@ -283,8 +282,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> >         }
> >         bus->edev_count = count;
> >
> > -       size = sizeof(*bus->edev) * count;
> > -       bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
> > +       bus->edev = devm_kcalloc(dev, count, sizeof(*bus->edev), GFP_KERNEL);
> >         if (!bus->edev) {
> >                 ret = -ENOMEM;
> >                 goto err_regulator;
> > @@ -388,7 +386,7 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
> >         ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
> >         if (!ondemand_data) {
> >                 ret = -ENOMEM;
> > -               goto err;
> > +               goto out;
> >         }
> >         ondemand_data->upthreshold = 40;
> >         ondemand_data->downdifferential = 5;
> > @@ -400,14 +398,14 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
> >         if (IS_ERR(bus->devfreq)) {
> >                 dev_err(dev, "failed to add devfreq device\n");
> >                 ret = PTR_ERR(bus->devfreq);
> > -               goto err;
> > +               goto out;
> >         }
> >
> >         /* Register opp_notifier to catch the change of OPP  */
> >         ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
> >         if (ret < 0) {
> >                 dev_err(dev, "failed to register opp notifier\n");
> > -               goto err;
> > +               goto out;
> >         }
> >
> >         /*
> > @@ -417,16 +415,16 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
> >         ret = exynos_bus_enable_edev(bus);
> >         if (ret < 0) {
> >                 dev_err(dev, "failed to enable devfreq-event devices\n");
> > -               goto err;
> > +               goto out;
> >         }
> >
> >         ret = exynos_bus_set_event(bus);
> >         if (ret < 0) {
> >                 dev_err(dev, "failed to set event to devfreq-event devices\n");
> > -               goto err;
> > +               goto out;
> >         }
> >
> > -err:
> > +out:
> >         return ret;
> >  }
> >
> > @@ -446,27 +444,28 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
> >         parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
> >         if (IS_ERR(parent_devfreq)) {
> >                 ret = -EPROBE_DEFER;
> > -               goto err;
> > +               goto out;
> >         }
> >
> >         passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
> >         if (!passive_data) {
> >                 ret = -ENOMEM;
> > -               goto err;
> > +               goto out;
> >         }
> >         passive_data->parent = parent_devfreq;
> >
> >         /* Add devfreq device for exynos bus with passive governor */
> > -       bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
> > +       bus->devfreq = devm_devfreq_add_device(dev, profile,
> > +                                               DEVFREQ_GOV_PASSIVE,
> >                                                 passive_data);
> >         if (IS_ERR(bus->devfreq)) {
> >                 dev_err(dev,
> >                         "failed to add devfreq dev with passive governor\n");
> >                 ret = PTR_ERR(bus->devfreq);
> > -               goto err;
> > +               goto out;
> >         }
> >
> > -err:
> > +out:
> >         return ret;
> >  }
> >
> > @@ -484,11 +483,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
> >                 return -EINVAL;
> >         }
> >
> > -       bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> > +       bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
> >         if (!bus)
> >                 return -ENOMEM;
> >         mutex_init(&bus->lock);
> > -       bus->dev = &pdev->dev;
> > +       bus->dev = dev;
> >         platform_set_drvdata(pdev, bus);
> >
> >         /* Parse the device-tree to get the resource information */
> > @@ -502,7 +501,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
> >                 goto err;
> >         }
> >
> > -       node = of_parse_phandle(dev->of_node, "devfreq", 0);
> > +       node = of_parse_phandle(np, "devfreq", 0);
> >         if (node) {
> >                 of_node_put(node);
> >                 ret = exynos_bus_profile_init_passive(bus, profile);
> > @@ -547,12 +546,10 @@ static int exynos_bus_resume(struct device *dev)
> >         int ret;
> >
> >         ret = exynos_bus_enable_edev(bus);
> > -       if (ret < 0) {
> > +       if (ret < 0)
> >                 dev_err(dev, "failed to enable the devfreq-event devices\n");
> > -               return ret;
> > -       }
> >
> > -       return 0;
> > +       return ret;
> >  }
> >
> >  static int exynos_bus_suspend(struct device *dev)
> > @@ -561,12 +558,10 @@ static int exynos_bus_suspend(struct device *dev)
> >         int ret;
> >
>
> NACK.

Instead of simple nack you should explain what is wrong with proposed
approach. The existing code could be improved and this patch clearly
improves the readability. "err" label is confusing if it is used for
correct exit path. The last "if() return" block is subjective - but
then explain exactly why you prefer one over another. No just "nack"
for peoples contributions.

> >         ret = exynos_bus_disable_edev(bus);
> > -       if (ret < 0) {
> > +       if (ret < 0)
> >                 dev_err(dev, "failed to disable the devfreq-event devices\n");
> > -               return ret;
> > -       }
> >
> > -       return 0;
> > +       return ret;
> >  }
> >  #endif
> >
> > --
> > 2.17.1
> >
>
> NACK.
>
> As I already commented, It has never any benefit.
> I think that it is not usful. Please drop it.

The benefit for me is clear - makes the code easier to understand.

Best regards,
Krzysztof

Re: [RFC PATCH 04/11] devfreq: exynos-bus: Clean up code

[Chanwoo Choi]

On 19. 7. 26. 오후 7:45, Krzysztof Kozlowski wrote:
> On Thu, 25 Jul 2019 at 14:51, Chanwoo Choi <cwchoi00@gmail.com> wrote:
>>
>> 2019년 7월 24일 (수) 오전 8:07, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
>>>
>>> This patch adds minor improvements to the exynos-bus driver.
>>>
>>> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
>>> ---
>>>  drivers/devfreq/exynos-bus.c | 49 ++++++++++++++++--------------------
>>>  1 file changed, 22 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>> index 4bb83b945bf7..412511ca7703 100644
>>> --- a/drivers/devfreq/exynos-bus.c
>>> +++ b/drivers/devfreq/exynos-bus.c
>>> @@ -15,11 +15,10 @@
>>>  #include <linux/device.h>
>>>  #include <linux/export.h>
>>>  #include <linux/module.h>
>>> -#include <linux/of_device.h>
>>> +#include <linux/of.h>
>>>  #include <linux/pm_opp.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/regulator/consumer.h>
>>> -#include <linux/slab.h>
>>>
>>>  #define DEFAULT_SATURATION_RATIO       40
>>>  #define DEFAULT_VOLTAGE_TOLERANCE      2
>>> @@ -256,7 +255,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>                                         struct exynos_bus *bus)
>>>  {
>>>         struct device *dev = bus->dev;
>>> -       int i, ret, count, size;
>>> +       int i, ret, count;
>>>
>>>         /* Get the regulator to provide each bus with the power */
>>>         bus->regulator = devm_regulator_get(dev, "vdd");
>>> @@ -283,8 +282,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>         }
>>>         bus->edev_count = count;
>>>
>>> -       size = sizeof(*bus->edev) * count;
>>> -       bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>> +       bus->edev = devm_kcalloc(dev, count, sizeof(*bus->edev), GFP_KERNEL);
>>>         if (!bus->edev) {
>>>                 ret = -ENOMEM;
>>>                 goto err_regulator;
>>> @@ -388,7 +386,7 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>>>         ondemand_data = devm_kzalloc(dev, sizeof(*ondemand_data), GFP_KERNEL);
>>>         if (!ondemand_data) {
>>>                 ret = -ENOMEM;
>>> -               goto err;
>>> +               goto out;
>>>         }
>>>         ondemand_data->upthreshold = 40;
>>>         ondemand_data->downdifferential = 5;
>>> @@ -400,14 +398,14 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>>>         if (IS_ERR(bus->devfreq)) {
>>>                 dev_err(dev, "failed to add devfreq device\n");
>>>                 ret = PTR_ERR(bus->devfreq);
>>> -               goto err;
>>> +               goto out;
>>>         }
>>>
>>>         /* Register opp_notifier to catch the change of OPP  */
>>>         ret = devm_devfreq_register_opp_notifier(dev, bus->devfreq);
>>>         if (ret < 0) {
>>>                 dev_err(dev, "failed to register opp notifier\n");
>>> -               goto err;
>>> +               goto out;
>>>         }
>>>
>>>         /*
>>> @@ -417,16 +415,16 @@ static int exynos_bus_profile_init(struct exynos_bus *bus,
>>>         ret = exynos_bus_enable_edev(bus);
>>>         if (ret < 0) {
>>>                 dev_err(dev, "failed to enable devfreq-event devices\n");
>>> -               goto err;
>>> +               goto out;
>>>         }
>>>
>>>         ret = exynos_bus_set_event(bus);
>>>         if (ret < 0) {
>>>                 dev_err(dev, "failed to set event to devfreq-event devices\n");
>>> -               goto err;
>>> +               goto out;
>>>         }
>>>
>>> -err:
>>> +out:
>>>         return ret;
>>>  }
>>>
>>> @@ -446,27 +444,28 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>>>         parent_devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
>>>         if (IS_ERR(parent_devfreq)) {
>>>                 ret = -EPROBE_DEFER;
>>> -               goto err;
>>> +               goto out;
>>>         }
>>>
>>>         passive_data = devm_kzalloc(dev, sizeof(*passive_data), GFP_KERNEL);
>>>         if (!passive_data) {
>>>                 ret = -ENOMEM;
>>> -               goto err;
>>> +               goto out;
>>>         }
>>>         passive_data->parent = parent_devfreq;
>>>
>>>         /* Add devfreq device for exynos bus with passive governor */
>>> -       bus->devfreq = devm_devfreq_add_device(dev, profile, DEVFREQ_GOV_PASSIVE,
>>> +       bus->devfreq = devm_devfreq_add_device(dev, profile,
>>> +                                               DEVFREQ_GOV_PASSIVE,
>>>                                                 passive_data);
>>>         if (IS_ERR(bus->devfreq)) {
>>>                 dev_err(dev,
>>>                         "failed to add devfreq dev with passive governor\n");
>>>                 ret = PTR_ERR(bus->devfreq);
>>> -               goto err;
>>> +               goto out;
>>>         }
>>>
>>> -err:
>>> +out:
>>>         return ret;
>>>  }
>>>
>>> @@ -484,11 +483,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>                 return -EINVAL;
>>>         }
>>>
>>> -       bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>> +       bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
>>>         if (!bus)
>>>                 return -ENOMEM;
>>>         mutex_init(&bus->lock);
>>> -       bus->dev = &pdev->dev;
>>> +       bus->dev = dev;
>>>         platform_set_drvdata(pdev, bus);
>>>
>>>         /* Parse the device-tree to get the resource information */
>>> @@ -502,7 +501,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>                 goto err;
>>>         }
>>>
>>> -       node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>> +       node = of_parse_phandle(np, "devfreq", 0);
>>>         if (node) {
>>>                 of_node_put(node);
>>>                 ret = exynos_bus_profile_init_passive(bus, profile);
>>> @@ -547,12 +546,10 @@ static int exynos_bus_resume(struct device *dev)
>>>         int ret;
>>>
>>>         ret = exynos_bus_enable_edev(bus);
>>> -       if (ret < 0) {
>>> +       if (ret < 0)
>>>                 dev_err(dev, "failed to enable the devfreq-event devices\n");
>>> -               return ret;
>>> -       }
>>>
>>> -       return 0;
>>> +       return ret;
>>>  }
>>>
>>>  static int exynos_bus_suspend(struct device *dev)
>>> @@ -561,12 +558,10 @@ static int exynos_bus_suspend(struct device *dev)
>>>         int ret;
>>>
>>
>> NACK.
> 
> Instead of simple nack you should explain what is wrong with proposed
> approach. The existing code could be improved and this patch clearly
> improves the readability. "err" label is confusing if it is used for
> correct exit path. The last "if() return" block is subjective - but
> then explain exactly why you prefer one over another. No just "nack"
> for peoples contributions.

OK. Sorry for my lack comment.

Actually, I prefer 'err' instead of 'out' because jump to 'err'
statement point when failed to call some functions. In my case,
'err' is proper without any problem.


> 
>>>         ret = exynos_bus_disable_edev(bus);
>>> -       if (ret < 0) {
>>> +       if (ret < 0)
>>>                 dev_err(dev, "failed to disable the devfreq-event devices\n");
>>> -               return ret;
>>> -       }

When happen error by function call, I think that have to print
the error log, just return or jump some point like 'err'
for restoring the previous state.

But, in this case, even if happen the error, the exception handling
of exynos_bus_disable_edev() just prints the error log without return.

I knew that this it is possible because the exynos_bus_disable_edev(bus)
was called at the end of function. But, I want to keep the same style
for exception handling if there are no any critical benefits.

>>>
>>> -       return 0;
>>> +       return ret;
>>>  }
>>>  #endif
>>>
>>> --
>>> 2.17.1
>>>
>>
>> NACK.
>>
>> As I already commented, It has never any benefit.
>> I think that it is not usful. Please drop it.
> 
> The benefit for me is clear - makes the code easier to understand.
> 
> Best regards,
> Krzysztof
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[RFC PATCH 05/11] icc: Export of_icc_get_from_provider()

[Artur Świgoń]

This patch makes the above function public (for use in exynos-bus devfreq
driver).

Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
 drivers/interconnect/core.c           | 3 ++-
 include/linux/interconnect-provider.h | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 871eb4bc4efc..d566ae0b66c0 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -274,7 +274,7 @@ EXPORT_SYMBOL_GPL(of_icc_xlate_onecell);
  * Returns a valid pointer to struct icc_node on success or ERR_PTR()
  * on failure.
  */
-static struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
+struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
 {
 	struct icc_node *node = ERR_PTR(-EPROBE_DEFER);
 	struct icc_provider *provider;
@@ -293,6 +293,7 @@ static struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
 
 	return node;
 }
+EXPORT_SYMBOL_GPL(of_icc_get_from_provider);
 
 /**
  * of_icc_get() - get a path handle from a DT node based on name
diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
index 63caccadc2db..9ecfc518b952 100644
--- a/include/linux/interconnect-provider.h
+++ b/include/linux/interconnect-provider.h
@@ -97,6 +97,7 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider);
 void icc_node_del(struct icc_node *node);
 int icc_provider_add(struct icc_provider *provider);
 int icc_provider_del(struct icc_provider *provider);
+struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec);
 
 #else
 
@@ -137,6 +138,11 @@ static inline int icc_provider_del(struct icc_provider *provider)
 	return -ENOTSUPP;
 }
 
+struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
 #endif /* CONFIG_INTERCONNECT */
 
 #endif /* __LINUX_INTERCONNECT_PROVIDER_H */
-- 
2.17.1

Re: [RFC PATCH 05/11] icc: Export of_icc_get_from_provider()

[Krzysztof Kozlowski]

On Tue, Jul 23, 2019 at 02:20:10PM +0200, Artur Świgoń wrote:
> This patch makes the above function public (for use in exynos-bus devfreq
> driver).
> 
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---
>  drivers/interconnect/core.c           | 3 ++-
>  include/linux/interconnect-provider.h | 6 ++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

[RFC PATCH 06/11] icc: Relax requirement in of_icc_get_from_provider()

[Artur Świgoń]

This patch relaxes the condition in of_icc_get_from_provider() so that it
is no longer required to set #interconnect-cells = <1> in the DT. In case
of the devfreq driver for exynos-bus, #interconnect-cells is always zero.

Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
 drivers/interconnect/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index d566ae0b66c0..2556fd6d1863 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -279,7 +279,7 @@ struct icc_node *of_icc_get_from_provider(struct of_phandle_args *spec)
 	struct icc_node *node = ERR_PTR(-EPROBE_DEFER);
 	struct icc_provider *provider;
 
-	if (!spec || spec->args_count != 1)
+	if (!spec)
 		return ERR_PTR(-EINVAL);
 
 	mutex_lock(&icc_lock);
-- 
2.17.1

Re: [RFC PATCH 06/11] icc: Relax requirement in of_icc_get_from_provider()

[Krzysztof Kozlowski]

On Tue, Jul 23, 2019 at 02:20:11PM +0200, Artur Świgoń wrote:
> This patch relaxes the condition in of_icc_get_from_provider() so that it
> is no longer required to set #interconnect-cells = <1> in the DT. In case
> of the devfreq driver for exynos-bus, #interconnect-cells is always zero.
> 
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---

Makes sense to me:
Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

[RFC PATCH 07/11] icc: Relax condition in apply_constraints()

[Artur Świgoń]

The exynos-bus devfreq driver is extended with interconnect functionality
by a subsequent patch. This patch removes a check from the interconnect
framework that prevents interconnect from working on exynos-bus, in which
every bus is a separate interconnect provider.

Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
 drivers/interconnect/core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 2556fd6d1863..bb55565d190c 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -219,11 +219,8 @@ static int apply_constraints(struct icc_path *path)
 	for (i = 0; i < path->num_nodes; i++) {
 		next = path->reqs[i].node;
 
-		/*
-		 * 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) {
 			prev = next;
 			continue;
 		}
-- 
2.17.1

[RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

[Artur Świgoń]

This patch adds two fields tp the Exynos4412 DTS:
  - parent: to declare connections between nodes that are not in a
    parent-child relation in devfreq;
  - #interconnect-cells: required by the interconnect framework.

Please note that #interconnect-cells is always zero and node IDs are not
hardcoded anywhere.

Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
 arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
 arch/arm/boot/dts/exynos4412.dtsi               | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
index ea55f377d17c..bdd61ae86103 100644
--- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
+++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
@@ -106,6 +106,7 @@
 &bus_leftbus {
 	devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
 	vdd-supply = <&buck3_reg>;
+	parent = <&bus_dmc>;
 	status = "okay";
 };
 
diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
index d20db2dfe8e2..a70a671acacd 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -390,6 +390,7 @@
 			clocks = <&clock CLK_DIV_DMC>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_dmc_opp_table>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
@@ -398,6 +399,7 @@
 			clocks = <&clock CLK_DIV_ACP>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_acp_opp_table>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
@@ -406,6 +408,7 @@
 			clocks = <&clock CLK_DIV_C2C>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_dmc_opp_table>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
@@ -459,6 +462,7 @@
 			clocks = <&clock CLK_DIV_GDL>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_leftbus_opp_table>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
@@ -467,6 +471,7 @@
 			clocks = <&clock CLK_DIV_GDR>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_leftbus_opp_table>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
@@ -475,6 +480,7 @@
 			clocks = <&clock CLK_ACLK160>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_display_opp_table>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
@@ -483,6 +489,7 @@
 			clocks = <&clock CLK_ACLK133>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_fsys_opp_table>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
@@ -491,6 +498,7 @@
 			clocks = <&clock CLK_ACLK100>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_peri_opp_table>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
@@ -499,6 +507,7 @@
 			clocks = <&clock CLK_SCLK_MFC>;
 			clock-names = "bus";
 			operating-points-v2 = <&bus_leftbus_opp_table>;
+			#interconnect-cells = <0>;
 			status = "disabled";
 		};
 
-- 
2.17.1

Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

[Krzysztof Kozlowski]

On Tue, Jul 23, 2019 at 02:20:13PM +0200, Artur Świgoń wrote:
> This patch adds two fields tp the Exynos4412 DTS:

tp->to

>   - parent: to declare connections between nodes that are not in a
>     parent-child relation in devfreq;

Is it a standard property?
The explanation needs some improvement... why are you adding parent to a
devices which are not child-parent?

Best regards,
Krzysztof

>   - #interconnect-cells: required by the interconnect framework.
> 
> Please note that #interconnect-cells is always zero and node IDs are not
> hardcoded anywhere.
> 
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---
>  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
>  arch/arm/boot/dts/exynos4412.dtsi               | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> index ea55f377d17c..bdd61ae86103 100644
> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -106,6 +106,7 @@
>  &bus_leftbus {
>  	devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>  	vdd-supply = <&buck3_reg>;
> +	parent = <&bus_dmc>;
>  	status = "okay";
>  };
>  
> diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
> index d20db2dfe8e2..a70a671acacd 100644
> --- a/arch/arm/boot/dts/exynos4412.dtsi
> +++ b/arch/arm/boot/dts/exynos4412.dtsi
> @@ -390,6 +390,7 @@
>  			clocks = <&clock CLK_DIV_DMC>;
>  			clock-names = "bus";
>  			operating-points-v2 = <&bus_dmc_opp_table>;
> +			#interconnect-cells = <0>;
>  			status = "disabled";
>  		};
>  
> @@ -398,6 +399,7 @@
>  			clocks = <&clock CLK_DIV_ACP>;
>  			clock-names = "bus";
>  			operating-points-v2 = <&bus_acp_opp_table>;
> +			#interconnect-cells = <0>;
>  			status = "disabled";
>  		};
>  
> @@ -406,6 +408,7 @@
>  			clocks = <&clock CLK_DIV_C2C>;
>  			clock-names = "bus";
>  			operating-points-v2 = <&bus_dmc_opp_table>;
> +			#interconnect-cells = <0>;
>  			status = "disabled";
>  		};
>  
> @@ -459,6 +462,7 @@
>  			clocks = <&clock CLK_DIV_GDL>;
>  			clock-names = "bus";
>  			operating-points-v2 = <&bus_leftbus_opp_table>;
> +			#interconnect-cells = <0>;
>  			status = "disabled";
>  		};
>  
> @@ -467,6 +471,7 @@
>  			clocks = <&clock CLK_DIV_GDR>;
>  			clock-names = "bus";
>  			operating-points-v2 = <&bus_leftbus_opp_table>;
> +			#interconnect-cells = <0>;
>  			status = "disabled";
>  		};
>  
> @@ -475,6 +480,7 @@
>  			clocks = <&clock CLK_ACLK160>;
>  			clock-names = "bus";
>  			operating-points-v2 = <&bus_display_opp_table>;
> +			#interconnect-cells = <0>;
>  			status = "disabled";
>  		};
>  
> @@ -483,6 +489,7 @@
>  			clocks = <&clock CLK_ACLK133>;
>  			clock-names = "bus";
>  			operating-points-v2 = <&bus_fsys_opp_table>;
> +			#interconnect-cells = <0>;
>  			status = "disabled";
>  		};
>  
> @@ -491,6 +498,7 @@
>  			clocks = <&clock CLK_ACLK100>;
>  			clock-names = "bus";
>  			operating-points-v2 = <&bus_peri_opp_table>;
> +			#interconnect-cells = <0>;
>  			status = "disabled";
>  		};
>  
> @@ -499,6 +507,7 @@
>  			clocks = <&clock CLK_SCLK_MFC>;
>  			clock-names = "bus";
>  			operating-points-v2 = <&bus_leftbus_opp_table>;
> +			#interconnect-cells = <0>;
>  			status = "disabled";
>  		};
>  
> -- 
> 2.17.1
> 

Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

[Artur Świgoń]

On Wed, 2019-07-24 at 21:24 +0200, Krzysztof Kozlowski wrote:
> On Tue, Jul 23, 2019 at 02:20:13PM +0200, Artur Świgoń wrote:
> > This patch adds two fields tp the Exynos4412 DTS:
> 
> tp->to

Fixed. Thanks for catching the typo :)

> >   - parent: to declare connections between nodes that are not in a
> >     parent-child relation in devfreq;
> 
> Is it a standard property?
> The explanation needs some improvement... why are you adding parent to a
> devices which are not child-parent?

This is not a standard property. I actually wanted to call it 'neighbor' at first. If you take a look at [1] and search for 'Exynos4x12', you will see that there are two power lines, and each has exactly one parent block (the rest are modelled in devfreq as its children). In [2], the parent of each child is indicated using the 'devfreq' property, e.g.,

&bus_display {
	devfreq = <&bus_leftbus>;
	status = "okay";
};

The single piece missing to make this topology a connected graph (for
interconnect QoS purposes) is the 'parent' property I proposed in this patch.
bus_leftbus is dependent on bus_dmc, therefore using the term 'parent' is
justified IMHO.

The explanation could be improved by adding what Chanwoo Choi <
cw00.choi@samsung.com> expressed in a parallel reply to this patch:
> - If 'devfreq' property is used between buses,
>   it indicates that there are requirement of sharing of power line.
> - If 'parent' property is used between buses,
>   it indicates that there are requirement of interconnect connection.

[1] Documentation/devicetree/bindings/devfreq/exynos-bus.txt
[2] arch/arm/boot/dts/exynos4412-odroid-common.dtsi (subject of this patch)

Best regards,
-- 
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics

Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

[Chanwoo Choi]

2019년 7월 24일 (수) 오전 8:07, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
>
> This patch adds two fields tp the Exynos4412 DTS:
>   - parent: to declare connections between nodes that are not in a
>     parent-child relation in devfreq;
>   - #interconnect-cells: required by the interconnect framework.
>
> Please note that #interconnect-cells is always zero and node IDs are not
> hardcoded anywhere.
>
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---
>  arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
>  arch/arm/boot/dts/exynos4412.dtsi               | 9 +++++++++
>  2 files changed, 10 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> index ea55f377d17c..bdd61ae86103 100644
> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -106,6 +106,7 @@
>  &bus_leftbus {
>         devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>         vdd-supply = <&buck3_reg>;
> +       parent = <&bus_dmc>;

It is wrong. 'bus_leftbus' has not any h/w dependency of 'bus_dmc'
and 'bus_leftbus' is not child of 'bus_dmc'.

Even it there are some PMQoS requirement between them,
it it not proper to tie both 'bus_leftbus' and 'bus_dmc'.

>         status = "okay";
>  };
>
> diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
> index d20db2dfe8e2..a70a671acacd 100644
> --- a/arch/arm/boot/dts/exynos4412.dtsi
> +++ b/arch/arm/boot/dts/exynos4412.dtsi
> @@ -390,6 +390,7 @@
>                         clocks = <&clock CLK_DIV_DMC>;
>                         clock-names = "bus";
>                         operating-points-v2 = <&bus_dmc_opp_table>;
> +                       #interconnect-cells = <0>;
>                         status = "disabled";
>                 };
>
> @@ -398,6 +399,7 @@
>                         clocks = <&clock CLK_DIV_ACP>;
>                         clock-names = "bus";
>                         operating-points-v2 = <&bus_acp_opp_table>;
> +                       #interconnect-cells = <0>;
>                         status = "disabled";
>                 };
>
> @@ -406,6 +408,7 @@
>                         clocks = <&clock CLK_DIV_C2C>;
>                         clock-names = "bus";
>                         operating-points-v2 = <&bus_dmc_opp_table>;
> +                       #interconnect-cells = <0>;
>                         status = "disabled";
>                 };
>
> @@ -459,6 +462,7 @@
>                         clocks = <&clock CLK_DIV_GDL>;
>                         clock-names = "bus";
>                         operating-points-v2 = <&bus_leftbus_opp_table>;
> +                       #interconnect-cells = <0>;
>                         status = "disabled";
>                 };
>
> @@ -467,6 +471,7 @@
>                         clocks = <&clock CLK_DIV_GDR>;
>                         clock-names = "bus";
>                         operating-points-v2 = <&bus_leftbus_opp_table>;
> +                       #interconnect-cells = <0>;
>                         status = "disabled";
>                 };
>
> @@ -475,6 +480,7 @@
>                         clocks = <&clock CLK_ACLK160>;
>                         clock-names = "bus";
>                         operating-points-v2 = <&bus_display_opp_table>;
> +                       #interconnect-cells = <0>;
>                         status = "disabled";
>                 };
>
> @@ -483,6 +489,7 @@
>                         clocks = <&clock CLK_ACLK133>;
>                         clock-names = "bus";
>                         operating-points-v2 = <&bus_fsys_opp_table>;
> +                       #interconnect-cells = <0>;
>                         status = "disabled";
>                 };
>
> @@ -491,6 +498,7 @@
>                         clocks = <&clock CLK_ACLK100>;
>                         clock-names = "bus";
>                         operating-points-v2 = <&bus_peri_opp_table>;
> +                       #interconnect-cells = <0>;
>                         status = "disabled";
>                 };
>
> @@ -499,6 +507,7 @@
>                         clocks = <&clock CLK_SCLK_MFC>;
>                         clock-names = "bus";
>                         operating-points-v2 = <&bus_leftbus_opp_table>;
> +                       #interconnect-cells = <0>;
>                         status = "disabled";
>                 };
>
> --
> 2.17.1
>


-- 
Best Regards,
Chanwoo Choi

Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

[Marek Szyprowski]

Hi

On 2019-07-25 15:13, Chanwoo Choi wrote:
> 2019년 7월 24일 (수) 오전 8:07, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
>> This patch adds two fields tp the Exynos4412 DTS:
>>    - parent: to declare connections between nodes that are not in a
>>      parent-child relation in devfreq;
>>    - #interconnect-cells: required by the interconnect framework.
>>
>> Please note that #interconnect-cells is always zero and node IDs are not
>> hardcoded anywhere.
>>
>> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
>> ---
>>   arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
>>   arch/arm/boot/dts/exynos4412.dtsi               | 9 +++++++++
>>   2 files changed, 10 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>> index ea55f377d17c..bdd61ae86103 100644
>> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>> @@ -106,6 +106,7 @@
>>   &bus_leftbus {
>>          devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>>          vdd-supply = <&buck3_reg>;
>> +       parent = <&bus_dmc>;
> It is wrong. 'bus_leftbus' has not any h/w dependency of 'bus_dmc'
> and 'bus_leftbus' is not child of 'bus_dmc'.
>
> Even it there are some PMQoS requirement between them,
> it it not proper to tie both 'bus_leftbus' and 'bus_dmc'.

There is strict dependency between them. DMC bus running at frequency 
lower than left (or right) bus really doesn't make much sense, because 
it will limit the left bus performance. This dependency should be 
modeled somehow.

>>          status = "okay";
>>   };
>>
>> diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
>> index d20db2dfe8e2..a70a671acacd 100644
>> --- a/arch/arm/boot/dts/exynos4412.dtsi
>> +++ b/arch/arm/boot/dts/exynos4412.dtsi
>> @@ -390,6 +390,7 @@
>>                          clocks = <&clock CLK_DIV_DMC>;
>>                          clock-names = "bus";
>>                          operating-points-v2 = <&bus_dmc_opp_table>;
>> +                       #interconnect-cells = <0>;
>>                          status = "disabled";
>>                  };
>>
>> @@ -398,6 +399,7 @@
>>                          clocks = <&clock CLK_DIV_ACP>;
>>                          clock-names = "bus";
>>                          operating-points-v2 = <&bus_acp_opp_table>;
>> +                       #interconnect-cells = <0>;
>>                          status = "disabled";
>>                  };
>>
>> @@ -406,6 +408,7 @@
>>                          clocks = <&clock CLK_DIV_C2C>;
>>                          clock-names = "bus";
>>                          operating-points-v2 = <&bus_dmc_opp_table>;
>> +                       #interconnect-cells = <0>;
>>                          status = "disabled";
>>                  };
>>
>> @@ -459,6 +462,7 @@
>>                          clocks = <&clock CLK_DIV_GDL>;
>>                          clock-names = "bus";
>>                          operating-points-v2 = <&bus_leftbus_opp_table>;
>> +                       #interconnect-cells = <0>;
>>                          status = "disabled";
>>                  };
>>
>> @@ -467,6 +471,7 @@
>>                          clocks = <&clock CLK_DIV_GDR>;
>>                          clock-names = "bus";
>>                          operating-points-v2 = <&bus_leftbus_opp_table>;
>> +                       #interconnect-cells = <0>;
>>                          status = "disabled";
>>                  };
>>
>> @@ -475,6 +480,7 @@
>>                          clocks = <&clock CLK_ACLK160>;
>>                          clock-names = "bus";
>>                          operating-points-v2 = <&bus_display_opp_table>;
>> +                       #interconnect-cells = <0>;
>>                          status = "disabled";
>>                  };
>>
>> @@ -483,6 +489,7 @@
>>                          clocks = <&clock CLK_ACLK133>;
>>                          clock-names = "bus";
>>                          operating-points-v2 = <&bus_fsys_opp_table>;
>> +                       #interconnect-cells = <0>;
>>                          status = "disabled";
>>                  };
>>
>> @@ -491,6 +498,7 @@
>>                          clocks = <&clock CLK_ACLK100>;
>>                          clock-names = "bus";
>>                          operating-points-v2 = <&bus_peri_opp_table>;
>> +                       #interconnect-cells = <0>;
>>                          status = "disabled";
>>                  };
>>
>> @@ -499,6 +507,7 @@
>>                          clocks = <&clock CLK_SCLK_MFC>;
>>                          clock-names = "bus";
>>                          operating-points-v2 = <&bus_leftbus_opp_table>;
>> +                       #interconnect-cells = <0>;
>>                          status = "disabled";
>>                  };
>>
>> --
>> 2.17.1
>>
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [RFC PATCH 08/11] arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412

[Chanwoo Choi]

Hi,

On 19. 7. 26. 오후 9:02, Marek Szyprowski wrote:
> Hi
> 
> On 2019-07-25 15:13, Chanwoo Choi wrote:
>> 2019년 7월 24일 (수) 오전 8:07, Artur Świgoń <a.swigon@partner.samsung.com>님이 작성:
>>> This patch adds two fields tp the Exynos4412 DTS:
>>>    - parent: to declare connections between nodes that are not in a
>>>      parent-child relation in devfreq;
>>>    - #interconnect-cells: required by the interconnect framework.
>>>
>>> Please note that #interconnect-cells is always zero and node IDs are not
>>> hardcoded anywhere.
>>>
>>> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
>>> ---
>>>   arch/arm/boot/dts/exynos4412-odroid-common.dtsi | 1 +
>>>   arch/arm/boot/dts/exynos4412.dtsi               | 9 +++++++++
>>>   2 files changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>>> index ea55f377d17c..bdd61ae86103 100644
>>> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>>> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
>>> @@ -106,6 +106,7 @@
>>>   &bus_leftbus {
>>>          devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
>>>          vdd-supply = <&buck3_reg>;
>>> +       parent = <&bus_dmc>;
>> It is wrong. 'bus_leftbus' has not any h/w dependency of 'bus_dmc'
>> and 'bus_leftbus' is not child of 'bus_dmc'.
>>
>> Even it there are some PMQoS requirement between them,
>> it it not proper to tie both 'bus_leftbus' and 'bus_dmc'.
> 
> There is strict dependency between them. DMC bus running at frequency 
> lower than left (or right) bus really doesn't make much sense, because 
> it will limit the left bus performance. This dependency should be 
> modeled somehow.

I misunderstood new 'parent' prototype as the existing 'devfreq' property.
I didn't understand why use the 'devfreq' property because 'bus_dmc' and
'bus_leftbus' don't share the power line. Please ignore my previous comment.

Basically, I agree that it is necessary to support the QoS requirement
between buses or if possible, between bus and gpu.

To support the interconnect between bus_dmc, bus_leftbus and bus_display,
it used the either 'devfreq' or 'parent' properties to connect them.

In order to catch the meaning of 'devfreq' and 'parent' properties,
If possible, it would be separate the usage role of between 'devfreq'
or 'parent' properties. Because it is possible to connect the 'buses'
with only using 'parent' property if all buses in the path uses
the devfreq governors except for 'passive' governor.

- If 'devfreq' property is used between buses,
  it indicates that there are requirement of shading of power line.
- If 'parent' property is used between buses,
  it indicates that there are requirement of interconnect connection.

> 
>>>          status = "okay";
>>>   };
>>>
>>> diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
>>> index d20db2dfe8e2..a70a671acacd 100644
>>> --- a/arch/arm/boot/dts/exynos4412.dtsi
>>> +++ b/arch/arm/boot/dts/exynos4412.dtsi
>>> @@ -390,6 +390,7 @@
>>>                          clocks = <&clock CLK_DIV_DMC>;
>>>                          clock-names = "bus";
>>>                          operating-points-v2 = <&bus_dmc_opp_table>;
>>> +                       #interconnect-cells = <0>;
>>>                          status = "disabled";
>>>                  };
>>>
>>> @@ -398,6 +399,7 @@
>>>                          clocks = <&clock CLK_DIV_ACP>;
>>>                          clock-names = "bus";
>>>                          operating-points-v2 = <&bus_acp_opp_table>;
>>> +                       #interconnect-cells = <0>;
>>>                          status = "disabled";
>>>                  };
>>>
>>> @@ -406,6 +408,7 @@
>>>                          clocks = <&clock CLK_DIV_C2C>;
>>>                          clock-names = "bus";
>>>                          operating-points-v2 = <&bus_dmc_opp_table>;
>>> +                       #interconnect-cells = <0>;
>>>                          status = "disabled";
>>>                  };
>>>
>>> @@ -459,6 +462,7 @@
>>>                          clocks = <&clock CLK_DIV_GDL>;
>>>                          clock-names = "bus";
>>>                          operating-points-v2 = <&bus_leftbus_opp_table>;
>>> +                       #interconnect-cells = <0>;
>>>                          status = "disabled";
>>>                  };
>>>
>>> @@ -467,6 +471,7 @@
>>>                          clocks = <&clock CLK_DIV_GDR>;
>>>                          clock-names = "bus";
>>>                          operating-points-v2 = <&bus_leftbus_opp_table>;
>>> +                       #interconnect-cells = <0>;
>>>                          status = "disabled";
>>>                  };
>>>
>>> @@ -475,6 +480,7 @@
>>>                          clocks = <&clock CLK_ACLK160>;
>>>                          clock-names = "bus";
>>>                          operating-points-v2 = <&bus_display_opp_table>;
>>> +                       #interconnect-cells = <0>;
>>>                          status = "disabled";
>>>                  };
>>>
>>> @@ -483,6 +489,7 @@
>>>                          clocks = <&clock CLK_ACLK133>;
>>>                          clock-names = "bus";
>>>                          operating-points-v2 = <&bus_fsys_opp_table>;
>>> +                       #interconnect-cells = <0>;
>>>                          status = "disabled";
>>>                  };
>>>
>>> @@ -491,6 +498,7 @@
>>>                          clocks = <&clock CLK_ACLK100>;
>>>                          clock-names = "bus";
>>>                          operating-points-v2 = <&bus_peri_opp_table>;
>>> +                       #interconnect-cells = <0>;
>>>                          status = "disabled";
>>>                  };
>>>
>>> @@ -499,6 +507,7 @@
>>>                          clocks = <&clock CLK_SCLK_MFC>;
>>>                          clock-names = "bus";
>>>                          operating-points-v2 = <&bus_leftbus_opp_table>;
>>> +                       #interconnect-cells = <0>;
>>>                          status = "disabled";
>>>                  };
>>>
>>> --
>>> 2.17.1
>>>
>>
> Best regards
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Artur Świgoń]

This patch adds interconnect functionality to the exynos-bus devfreq
driver.

The SoC topology is a graph (or, more specifically, a tree) and most of its
edges are taken from the devfreq parent-child hierarchy (cf.
Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
patch adds missing edges to the DT (under the name 'parent'). Due to
unspecified relative probing order, -EPROBE_DEFER may be propagated to
guarantee that a child is probed before its parent.

Each bus is now an interconnect provider and an interconnect node as well
(cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
itself as a node. Node IDs are not hardcoded but rather assigned at
runtime, in probing order (subject to the above-mentioned exception
regarding relative order). This approach allows for using this driver with
various Exynos SoCs.

The devfreq target() callback provided by exynos-bus now selects either the
frequency calculated by the devfreq governor or the frequency requested via
the interconnect API for the given node, whichever is higher.

Please note that it is not an error when CONFIG_INTERCONNECT is 'n', in
which case all interconnect API functions are no-op.

Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
 drivers/devfreq/exynos-bus.c | 145 +++++++++++++++++++++++++++++++++++
 1 file changed, 145 insertions(+)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 412511ca7703..12fb7c84ae50 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -14,6 +14,7 @@
 #include <linux/devfreq-event.h>
 #include <linux/device.h>
 #include <linux/export.h>
+#include <linux/interconnect-provider.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pm_opp.h>
@@ -23,6 +24,8 @@
 #define DEFAULT_SATURATION_RATIO	40
 #define DEFAULT_VOLTAGE_TOLERANCE	2
 
+#define icc_units_to_hz(x) ((x) * 1000UL / 8)
+
 struct exynos_bus {
 	struct device *dev;
 
@@ -31,12 +34,17 @@ struct exynos_bus {
 	unsigned int edev_count;
 	struct mutex lock;
 
+	unsigned long min_freq;
 	unsigned long curr_freq;
 
 	struct regulator *regulator;
 	struct clk *clk;
 	unsigned int voltage_tolerance;
 	unsigned int ratio;
+
+	/* One provider per bus, one node per provider */
+	struct icc_provider provider;
+	struct icc_node *node;
 };
 
 /*
@@ -61,6 +69,13 @@ exynos_bus_ops_edev(enable_edev);
 exynos_bus_ops_edev(disable_edev);
 exynos_bus_ops_edev(set_event);
 
+static int exynos_bus_next_id(void)
+{
+	static int exynos_bus_node_id;
+
+	return exynos_bus_node_id++;
+}
+
 static int exynos_bus_get_event(struct exynos_bus *bus,
 				struct devfreq_event_data *edata)
 {
@@ -98,6 +113,8 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
 	unsigned long old_freq, new_freq, new_volt, tol;
 	int ret = 0;
 
+	*freq = max(*freq, bus->min_freq);
+
 	/* Get new opp-bus instance according to new bus clock */
 	new_opp = devfreq_recommended_opp(dev, freq, flags);
 	if (IS_ERR(new_opp)) {
@@ -208,6 +225,8 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
 	unsigned long old_freq, new_freq;
 	int ret = 0;
 
+	*freq = max(*freq, bus->min_freq);
+
 	/* Get new opp-bus instance according to new bus clock */
 	new_opp = devfreq_recommended_opp(dev, freq, flags);
 	if (IS_ERR(new_opp)) {
@@ -251,6 +270,35 @@ static void exynos_bus_passive_exit(struct device *dev)
 	clk_disable_unprepare(bus->clk);
 }
 
+static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
+{
+	struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
+
+	src_bus->min_freq = icc_units_to_hz(src->peak_bw);
+	dst_bus->min_freq = icc_units_to_hz(dst->peak_bw);
+
+	return 0;
+}
+
+static int exynos_bus_icc_aggregate(struct icc_node *node, u32 avg_bw,
+				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
+{
+	*agg_peak = *agg_avg = peak_bw;
+
+	return 0;
+}
+
+static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
+					     void *data)
+{
+	struct exynos_bus *bus = data;
+
+	if (spec->np != bus->dev->of_node)
+		return ERR_PTR(-EINVAL);
+
+	return bus->node;
+}
+
 static int exynos_bus_parent_parse_of(struct device_node *np,
 					struct exynos_bus *bus)
 {
@@ -469,6 +517,95 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
 	return ret;
 }
 
+static int exynos_bus_icc_connect(struct exynos_bus *bus)
+{
+	struct device_node *np = bus->dev->of_node;
+	struct devfreq *parent_devfreq;
+	struct icc_node *parent_node = NULL;
+	struct of_phandle_args args;
+	int ret = 0;
+
+	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
+	if (!IS_ERR(parent_devfreq)) {
+		struct exynos_bus *parent_bus;
+
+		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
+		parent_node = parent_bus->node;
+	} else {
+		/* Look for parent in DT */
+		int num = of_count_phandle_with_args(np, "parent",
+						     "#interconnect-cells");
+		if (num != 1)
+			goto out;
+
+		ret = of_parse_phandle_with_args(np, "parent",
+						 "#interconnect-cells",
+						 0, &args);
+		if (ret < 0)
+			goto out;
+
+		of_node_put(args.np);
+
+		parent_node = of_icc_get_from_provider(&args);
+		if (IS_ERR(parent_node)) {
+			/* May be -EPROBE_DEFER */
+			ret = PTR_ERR(parent_node);
+			goto out;
+		}
+	}
+
+	ret = icc_link_create(bus->node, parent_node->id);
+
+out:
+	return ret;
+}
+
+static int exynos_bus_icc_init(struct exynos_bus *bus)
+{
+	struct device *dev = bus->dev;
+	struct icc_provider *provider = &bus->provider;
+	struct icc_node *node;
+	int id, ret;
+
+	/* Initialize the interconnect provider */
+	provider->set = exynos_bus_icc_set;
+	provider->aggregate = exynos_bus_icc_aggregate;
+	provider->xlate = exynos_bus_icc_xlate;
+	provider->dev = dev;
+	provider->data = bus;
+
+	ret = icc_provider_add(provider);
+	if (ret < 0)
+		goto out;
+
+	id = exynos_bus_next_id();
+	node = icc_node_create(id);
+	if (IS_ERR(node)) {
+		ret = PTR_ERR(node);
+		goto err_node;
+	}
+
+	bus->node = node;
+	node->name = dev->of_node->name;
+	node->data = bus;
+	icc_node_add(node, provider);
+
+	ret = exynos_bus_icc_connect(bus);
+	if (ret < 0)
+		goto err_connect;
+
+out:
+	return ret;
+
+err_connect:
+	icc_node_del(node);
+	icc_node_destroy(id);
+err_node:
+	icc_provider_del(provider);
+
+	return ret;
+}
+
 static int exynos_bus_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -517,6 +654,14 @@ static int exynos_bus_probe(struct platform_device *pdev)
 			goto err;
 	}
 
+	/*
+	 * Initialize interconnect provider. A return value of -ENOTSUPP means
+	 * that CONFIG_INTERCONNECT is disabled.
+	 */
+	ret = exynos_bus_icc_init(bus);
+	if (ret < 0 && ret != -ENOTSUPP)
+		goto err;
+
 	max_state = bus->devfreq->profile->max_state;
 	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
 	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
-- 
2.17.1

Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Krzysztof Kozlowski]

On Tue, Jul 23, 2019 at 02:20:14PM +0200, Artur Świgoń wrote:
> This patch adds interconnect functionality to the exynos-bus devfreq
> driver.
> 
> The SoC topology is a graph (or, more specifically, a tree) and most of its
> edges are taken from the devfreq parent-child hierarchy (cf.
> Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
> patch adds missing edges to the DT (under the name 'parent'). Due to

Do not refer to DT patches. They will come through different tree so
"previous" will not be correct anymore. You mentioned dependencies in
cover letter so it is sufficient.

> unspecified relative probing order, -EPROBE_DEFER may be propagated to
> guarantee that a child is probed before its parent.
> 
> Each bus is now an interconnect provider and an interconnect node as well
> (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> itself as a node. Node IDs are not hardcoded but rather assigned at
> runtime, in probing order (subject to the above-mentioned exception
> regarding relative order). This approach allows for using this driver with
> various Exynos SoCs.
> 
> The devfreq target() callback provided by exynos-bus now selects either the
> frequency calculated by the devfreq governor or the frequency requested via
> the interconnect API for the given node, whichever is higher.
> 
> Please note that it is not an error when CONFIG_INTERCONNECT is 'n', in
> which case all interconnect API functions are no-op.
> 
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 145 +++++++++++++++++++++++++++++++++++
>  1 file changed, 145 insertions(+)
> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 412511ca7703..12fb7c84ae50 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -14,6 +14,7 @@
>  #include <linux/devfreq-event.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> +#include <linux/interconnect-provider.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/pm_opp.h>
> @@ -23,6 +24,8 @@
>  #define DEFAULT_SATURATION_RATIO	40
>  #define DEFAULT_VOLTAGE_TOLERANCE	2
>  
> +#define icc_units_to_hz(x) ((x) * 1000UL / 8)
> +
>  struct exynos_bus {
>  	struct device *dev;
>  
> @@ -31,12 +34,17 @@ struct exynos_bus {
>  	unsigned int edev_count;
>  	struct mutex lock;
>  
> +	unsigned long min_freq;
>  	unsigned long curr_freq;
>  
>  	struct regulator *regulator;
>  	struct clk *clk;
>  	unsigned int voltage_tolerance;
>  	unsigned int ratio;
> +
> +	/* One provider per bus, one node per provider */
> +	struct icc_provider provider;
> +	struct icc_node *node;
>  };
>  
>  /*
> @@ -61,6 +69,13 @@ exynos_bus_ops_edev(enable_edev);
>  exynos_bus_ops_edev(disable_edev);
>  exynos_bus_ops_edev(set_event);
>  
> +static int exynos_bus_next_id(void)
> +{
> +	static int exynos_bus_node_id;
> +
> +	return exynos_bus_node_id++;

This does not look robust. Use IDR for IDs. 

> +}
> +
>  static int exynos_bus_get_event(struct exynos_bus *bus,
>  				struct devfreq_event_data *edata)
>  {
> @@ -98,6 +113,8 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>  	unsigned long old_freq, new_freq, new_volt, tol;
>  	int ret = 0;
>  
> +	*freq = max(*freq, bus->min_freq);
> +
>  	/* Get new opp-bus instance according to new bus clock */
>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>  	if (IS_ERR(new_opp)) {
> @@ -208,6 +225,8 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>  	unsigned long old_freq, new_freq;
>  	int ret = 0;
>  
> +	*freq = max(*freq, bus->min_freq);
> +
>  	/* Get new opp-bus instance according to new bus clock */
>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>  	if (IS_ERR(new_opp)) {
> @@ -251,6 +270,35 @@ static void exynos_bus_passive_exit(struct device *dev)
>  	clk_disable_unprepare(bus->clk);
>  }
>  
> +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> +	struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
> +
> +	src_bus->min_freq = icc_units_to_hz(src->peak_bw);
> +	dst_bus->min_freq = icc_units_to_hz(dst->peak_bw);
> +
> +	return 0;
> +}
> +
> +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 avg_bw,
> +				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> +{
> +	*agg_peak = *agg_avg = peak_bw;
> +
> +	return 0;
> +}
> +
> +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
> +					     void *data)
> +{
> +	struct exynos_bus *bus = data;
> +
> +	if (spec->np != bus->dev->of_node)
> +		return ERR_PTR(-EINVAL);
> +
> +	return bus->node;
> +}
> +
>  static int exynos_bus_parent_parse_of(struct device_node *np,
>  					struct exynos_bus *bus)
>  {
> @@ -469,6 +517,95 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>  	return ret;
>  }
>  
> +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> +{
> +	struct device_node *np = bus->dev->of_node;
> +	struct devfreq *parent_devfreq;
> +	struct icc_node *parent_node = NULL;
> +	struct of_phandle_args args;
> +	int ret = 0;
> +
> +	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> +	if (!IS_ERR(parent_devfreq)) {
> +		struct exynos_bus *parent_bus;

What if someone unbinds this parent devfreq? I guess everything in
devfreq starts exploding... however it's not the problem of this patch.

Do you also need suspend/resume order (device links)? I guess the other
side, e.g.  mixer, should resume before the bus?

> +
> +		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> +		parent_node = parent_bus->node;
> +	} else {
> +		/* Look for parent in DT */
> +		int num = of_count_phandle_with_args(np, "parent",
> +						     "#interconnect-cells");
> +		if (num != 1)

You will return here 0 but isn't it an error?

Best regards,
Krzysztof

Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Artur Świgoń]

On Wed, 2019-07-24 at 20:36 +0200, Krzysztof Kozlowski wrote:
> On Tue, Jul 23, 2019 at 02:20:14PM +0200, Artur Świgoń wrote:
> > This patch adds interconnect functionality to the exynos-bus devfreq
> > driver.
> > 
> > The SoC topology is a graph (or, more specifically, a tree) and most of its
> > edges are taken from the devfreq parent-child hierarchy (cf.
> > Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
> > patch adds missing edges to the DT (under the name 'parent'). Due to
> 
> Do not refer to DT patches. They will come through different tree so
> "previous" will not be correct anymore. You mentioned dependencies in
> cover letter so it is sufficient.

OK.
 
> >  /*
> > @@ -61,6 +69,13 @@ exynos_bus_ops_edev(enable_edev);
> >  exynos_bus_ops_edev(disable_edev);
> >  exynos_bus_ops_edev(set_event);
> >  
> > +static int exynos_bus_next_id(void)
> > +{
> > +	static int exynos_bus_node_id;
> > +
> > +	return exynos_bus_node_id++;
> 
> This does not look robust. Use IDR for IDs. 

OK.

> > +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> > +{
> > +	struct device_node *np = bus->dev->of_node;
> > +	struct devfreq *parent_devfreq;
> > +	struct icc_node *parent_node = NULL;
> > +	struct of_phandle_args args;
> > +	int ret = 0;
> > +
> > +	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> > +	if (!IS_ERR(parent_devfreq)) {
> > +		struct exynos_bus *parent_bus;
> 
> What if someone unbinds this parent devfreq? I guess everything in
> devfreq starts exploding... however it's not the problem of this patch.
> 
> Do you also need suspend/resume order (device links)? I guess the other
> side, e.g.  mixer, should resume before the bus?

Actually, I think that the bus (devfreq) should resume before mixer. However,
suspend/resume order is another issue that applies to this driver regardless of
using the interconnect framework, although device links could probably also be
implemented in the interconnect framework itself.

> > +		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> > +		parent_node = parent_bus->node;
> > +	} else {
> > +		/* Look for parent in DT */
> > +		int num = of_count_phandle_with_args(np, "parent",
> > +						     "#interconnect-cells");
> > +		if (num != 1)
> 
> You will return here 0 but isn't it an error?

It is definitely not an error when 'parent' does not exist in DT (for buses that
are parents themselves). I can extend the comment in the code to explicitly
state that.

Best regards,
-- 
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics

Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Georgi Djakov]

Hi Artur,

On 7/23/19 15:20, Artur Świgoń wrote:
> This patch adds interconnect functionality to the exynos-bus devfreq
> driver.
> 
> The SoC topology is a graph (or, more specifically, a tree) and most of its
> edges are taken from the devfreq parent-child hierarchy (cf.
> Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
> patch adds missing edges to the DT (under the name 'parent'). Due to
> unspecified relative probing order, -EPROBE_DEFER may be propagated to
> guarantee that a child is probed before its parent.
> 
> Each bus is now an interconnect provider and an interconnect node as well
> (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> itself as a node. Node IDs are not hardcoded but rather assigned at
> runtime, in probing order (subject to the above-mentioned exception
> regarding relative order). This approach allows for using this driver with
> various Exynos SoCs.

I am not familiar with the Exynos bus topology, but it seems to me that it's not
represented correctly. An interconnect provider with just a single node (port)
is odd. I would expect that each provider consists of multiple master and slave
nodes. This data would be used by a framework to understand what are the links
and how the traffic flows between the IP blocks and through which buses.

> The devfreq target() callback provided by exynos-bus now selects either the
> frequency calculated by the devfreq governor or the frequency requested via
> the interconnect API for the given node, whichever is higher.

This completely makes sense. We just need to be sure that the interconnect
framework is used correctly.

Thanks,
Georgi

Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Artur Świgoń]

Hi Georgi,

On Fri, 2019-07-26 at 11:05 +0300, Georgi Djakov wrote:
> Hi Artur,
> 
> On 7/23/19 15:20, Artur Świgoń wrote:
> > This patch adds interconnect functionality to the exynos-bus devfreq
> > driver.
> > 
> > The SoC topology is a graph (or, more specifically, a tree) and most of its
> > edges are taken from the devfreq parent-child hierarchy (cf.
> > Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
> > patch adds missing edges to the DT (under the name 'parent'). Due to
> > unspecified relative probing order, -EPROBE_DEFER may be propagated to
> > guarantee that a child is probed before its parent.
> > 
> > Each bus is now an interconnect provider and an interconnect node as well
> > (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> > itself as a node. Node IDs are not hardcoded but rather assigned at
> > runtime, in probing order (subject to the above-mentioned exception
> > regarding relative order). This approach allows for using this driver with
> > various Exynos SoCs.
> 
> I am not familiar with the Exynos bus topology, but it seems to me that it's not
> represented correctly. An interconnect provider with just a single node (port)
> is odd. I would expect that each provider consists of multiple master and slave
> nodes. This data would be used by a framework to understand what are the links
> and how the traffic flows between the IP blocks and through which buses.

To summarize the exynos-bus topology[1] used by the devfreq driver: There are
many data buses for data transfer in Samsung Exynos SoC. Every bus has its own
clock. Buses often share power lines, in which case one of the buses on the
power line is referred to as 'parent' (or as 'devfreq' in the DT). In the
particular case of Exynos4412[1][2], the topology can be expressed as follows:

bus_dmc
-- bus_acp
-- bus_c2c

bus_leftbus
-- bus_rightbus
-- bus_display
-- bus_fsys
-- bus_peri
-- bus_mfc

Where bus_dmc and bus_leftbus probably could be referred to as masters, and the
following indented nodes as slaves. Patch 08/11 of this RFC additionally adds
the following to the DT:

bus_dmc
-- bus_leftbus

Which makes the topology a valid tree.

The exynos-bus concept in devfreq[3] is designed in such a way that every bus is
probed separately as a platform device, and is a largely independent entity.
This RFC proposes an extension to the existing devfreq driver that basically
provides a simple QoS to ensure minimum clock frequency for selected buses
(possibly overriding devfreq governor calculations) using the interconnect
framework.

The hierarchy is modelled in such a way that every bus is an interconnect node.
On the other hand, what is considered an interconnect provider here is quite
arbitrary, but for the reasons mentioned in the above paragraph, this RFC
assumes that every bus is a provider of itself as a node. Using an alternative
singleton provider approach was deemed more complicated since the 'dev' field in
'struct icc_provider' has to be set to something meaningful and we are tied to
the 'samsung,exynos-bus' compatible string in the driver (and multiple instances
of exynos-bus probed in indeterminate relative order).

I'm looking forward to hearing any additional thoughts you may have on this
topic.

Best regards,
-- 
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics

[1] Documentation/devicetree/bindings/devfreq/exynos-bus.txt
[2]
arch/arm/boot/dts/exynos4412-odroid-common.dtsi
[3] drivers/devfreq/exynos-bus.c
(subject of this patch)

Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Georgi Djakov]

Hi Artur,

On 8/1/19 10:59, Artur Świgoń wrote:
> Hi Georgi,
> 
> On Fri, 2019-07-26 at 11:05 +0300, Georgi Djakov wrote:
>> Hi Artur,
>>
>> On 7/23/19 15:20, Artur Świgoń wrote:
>>> This patch adds interconnect functionality to the exynos-bus devfreq
>>> driver.
>>>
>>> The SoC topology is a graph (or, more specifically, a tree) and most of its
>>> edges are taken from the devfreq parent-child hierarchy (cf.
>>> Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
>>> patch adds missing edges to the DT (under the name 'parent'). Due to
>>> unspecified relative probing order, -EPROBE_DEFER may be propagated to
>>> guarantee that a child is probed before its parent.
>>>
>>> Each bus is now an interconnect provider and an interconnect node as well
>>> (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
>>> itself as a node. Node IDs are not hardcoded but rather assigned at
>>> runtime, in probing order (subject to the above-mentioned exception
>>> regarding relative order). This approach allows for using this driver with
>>> various Exynos SoCs.
>>
>> I am not familiar with the Exynos bus topology, but it seems to me that it's not
>> represented correctly. An interconnect provider with just a single node (port)
>> is odd. I would expect that each provider consists of multiple master and slave
>> nodes. This data would be used by a framework to understand what are the links
>> and how the traffic flows between the IP blocks and through which buses.
> 
> To summarize the exynos-bus topology[1] used by the devfreq driver: There are
> many data buses for data transfer in Samsung Exynos SoC. Every bus has its own
> clock. Buses often share power lines, in which case one of the buses on the
> power line is referred to as 'parent' (or as 'devfreq' in the DT). In the
> particular case of Exynos4412[1][2], the topology can be expressed as follows:
> 
> bus_dmc
> -- bus_acp
> -- bus_c2c
> 
> bus_leftbus
> -- bus_rightbus
> -- bus_display
> -- bus_fsys
> -- bus_peri
> -- bus_mfc
> 
> Where bus_dmc and bus_leftbus probably could be referred to as masters, and the
> following indented nodes as slaves. Patch 08/11 of this RFC additionally adds
> the following to the DT:
> 
> bus_dmc
> -- bus_leftbus
> 
> Which makes the topology a valid tree.
> 
> The exynos-bus concept in devfreq[3] is designed in such a way that every bus is
> probed separately as a platform device, and is a largely independent entity.
>
> This RFC proposes an extension to the existing devfreq driver that basically
> provides a simple QoS to ensure minimum clock frequency for selected buses
> (possibly overriding devfreq governor calculations) using the interconnect
> framework.
> 
> The hierarchy is modelled in such a way that every bus is an interconnect node.
> On the other hand, what is considered an interconnect provider here is quite
> arbitrary, but for the reasons mentioned in the above paragraph, this RFC
> assumes that every bus is a provider of itself as a node. Using an alternative

IIUC, in case we want to transfer data between the display and the memory
controller, the path would look like this:

display --> bus_display --> bus_leftbus --> bus_dmc --> memory

But the bus_display for example would have not one, but two nodes (ports),
right?  One representing the link to the display controller and another one
representing the link to bus_leftbus? So i think that all the buses should
have at least two nodes, to represent each end of the wire.

> singleton provider approach was deemed more complicated since the 'dev' field in
> 'struct icc_provider' has to be set to something meaningful and we are tied to
> the 'samsung,exynos-bus' compatible string in the driver (and multiple instances
> of exynos-bus probed in indeterminate relative order).
> 

Sure, the rest makes sense to me.

Thanks,
Georgi

Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Artur Świgoń]

Hi,

On Wed, 2019-08-07 at 17:21 +0300, Georgi Djakov wrote:
> Hi Artur,
> 
> On 8/1/19 10:59, Artur Świgoń wrote:
> > Hi Georgi,
> > 
> > On Fri, 2019-07-26 at 11:05 +0300, Georgi Djakov wrote:
> > > Hi Artur,
> > > 
> > > On 7/23/19 15:20, Artur Świgoń wrote:
> > > > This patch adds interconnect functionality to the exynos-bus devfreq
> > > > driver.
> > > > 
> > > > The SoC topology is a graph (or, more specifically, a tree) and most of
> > > > its
> > > > edges are taken from the devfreq parent-child hierarchy (cf.
> > > > Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
> > > > patch adds missing edges to the DT (under the name 'parent'). Due to
> > > > unspecified relative probing order, -EPROBE_DEFER may be propagated to
> > > > guarantee that a child is probed before its parent.
> > > > 
> > > > Each bus is now an interconnect provider and an interconnect node as
> > > > well
> > > > (cf. Documentation/interconnect/interconnect.rst), i.e. every bus
> > > > registers
> > > > itself as a node. Node IDs are not hardcoded but rather assigned at
> > > > runtime, in probing order (subject to the above-mentioned exception
> > > > regarding relative order). This approach allows for using this driver
> > > > with
> > > > various Exynos SoCs.
> > > 
> > > I am not familiar with the Exynos bus topology, but it seems to me that
> > > it's not
> > > represented correctly. An interconnect provider with just a single node
> > > (port)
> > > is odd. I would expect that each provider consists of multiple master and
> > > slave
> > > nodes. This data would be used by a framework to understand what are the
> > > links
> > > and how the traffic flows between the IP blocks and through which buses.
> > 
> > To summarize the exynos-bus topology[1] used by the devfreq driver: There
> > are
> > many data buses for data transfer in Samsung Exynos SoC. Every bus has its
> > own
> > clock. Buses often share power lines, in which case one of the buses on the
> > power line is referred to as 'parent' (or as 'devfreq' in the DT). In the
> > particular case of Exynos4412[1][2], the topology can be expressed as
> > follows:
> > 
> > bus_dmc
> > -- bus_acp
> > -- bus_c2c
> > 
> > bus_leftbus
> > -- bus_rightbus
> > -- bus_display
> > -- bus_fsys
> > -- bus_peri
> > -- bus_mfc
> > 
> > Where bus_dmc and bus_leftbus probably could be referred to as masters, and
> > the
> > following indented nodes as slaves. Patch 08/11 of this RFC additionally
> > adds
> > the following to the DT:
> > 
> > bus_dmc
> > -- bus_leftbus
> > 
> > Which makes the topology a valid tree.
> > 
> > The exynos-bus concept in devfreq[3] is designed in such a way that every
> > bus is
> > probed separately as a platform device, and is a largely independent entity.
> > 
> > This RFC proposes an extension to the existing devfreq driver that basically
> > provides a simple QoS to ensure minimum clock frequency for selected buses
> > (possibly overriding devfreq governor calculations) using the interconnect
> > framework.
> > 
> > The hierarchy is modelled in such a way that every bus is an interconnect
> > node.
> > On the other hand, what is considered an interconnect provider here is quite
> > arbitrary, but for the reasons mentioned in the above paragraph, this RFC
> > assumes that every bus is a provider of itself as a node. Using an
> > alternative
> 
> IIUC, in case we want to transfer data between the display and the memory
> controller, the path would look like this:
> 
> display --> bus_display --> bus_leftbus --> bus_dmc --> memory
> 
> But the bus_display for example would have not one, but two nodes (ports),
> right?  One representing the link to the display controller and another one
> representing the link to bus_leftbus? So i think that all the buses should
> have at least two nodes, to represent each end of the wire.

I do not think we really need that for our simple tree hierarchy. Of course, I
can split every tree node into two nodes/ports (e.g., 'in' for children and
'out' for parent), but neither 'display' nor 'memory' from your diagram above
are registered with the interconnect framework (only buses are). The devfreq
devices used in the driver are virtual anyway.

> > singleton provider approach was deemed more complicated since the 'dev'
> > field in
> > 'struct icc_provider' has to be set to something meaningful and we are tied
> > to
> > the 'samsung,exynos-bus' compatible string in the driver (and multiple
> > instances
> > of exynos-bus probed in indeterminate relative order).
> > 
> 
> Sure, the rest makes sense to me.
> 
> Thanks,
> Georgi

Regards,
-- 
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics

Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Chanwoo Choi]

Hi,

On 19. 7. 23. 오후 9:20, Artur Świgoń wrote:
> This patch adds interconnect functionality to the exynos-bus devfreq
> driver.
> 
> The SoC topology is a graph (or, more specifically, a tree) and most of its
> edges are taken from the devfreq parent-child hierarchy (cf.
> Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
> patch adds missing edges to the DT (under the name 'parent'). Due to
> unspecified relative probing order, -EPROBE_DEFER may be propagated to
> guarantee that a child is probed before its parent.
> 
> Each bus is now an interconnect provider and an interconnect node as well
> (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> itself as a node. Node IDs are not hardcoded but rather assigned at
> runtime, in probing order (subject to the above-mentioned exception
> regarding relative order). This approach allows for using this driver with
> various Exynos SoCs.
> 
> The devfreq target() callback provided by exynos-bus now selects either the
> frequency calculated by the devfreq governor or the frequency requested via
> the interconnect API for the given node, whichever is higher.

Basically, I agree to support the QoS requirement between devices.
But, I think that need to consider the multiple cases.


1. When changing the devfreq governor by user,
For example of the connection between bus_dmc/leftbus/display on patch8,
there are possible multiple cases with various devfreq governor
which is changed on the runtime by user through sysfs interface.

If users changes the devfreq governor as following:
Before,
- bus_dmc (simple_ondemand, available frequency 100/200/300/400 MHz)
--> bus_leftbus(simple_ondemand, available frequency 100/200/300/400 MHz)
----> bus_display(passive)

After changed governor of bus_dmc,
if the min_freq by interconnect requirement is 400Mhz,
- bus_dmc (powersave) : min_freq and max_freq and cur_freq is 100MHz
--> bus_leftbus(simple_ondemand) : cur_freq is 400Mhz
----> bus_display(passive)

The final frequency is 400MHz of bus_dmc
even if the min_freq/max_freq/cur_freq is 100MHz.
It cannot show the correct min_freq/max_freq through
devfreq sysfs interface.


2. When disabling the some frequency by devfreq-thermal throttling,
This patch checks the min_freq of interconnect requirement
in the exynos_bus_target() and exynos_bus_passive_target().
Also, it cannot show the correct min_freq/max_freq through
devfreq sysfs interface.

For example of bus_dmc bus,
- The available frequencies are 100MHz, 200MHz, 300MHz, 400MHz
- Disable 400MHz by devfreq-thermal throttling 
- min_freq is 100MHz
- max_freq is 300MHz
- min_freq of interconnect is 400MHz

In result, the final frequency is 400MHz by exynos_bus_target()
There are no problem for working. But, the user cannot know
reason why cur_freq is 400MHz even if max_freq is 300MHz.

Basically, update_devfreq() considers the all constraints
of min_freq/max_freq to decide the proper target frequency.


3.
I think that the exynos_bus_passive_target() is used for devfreq device
using 'passive' governor. The frequency already depends on the parent device.

If already the parent devfreq device like bus_leftbus consider
the minimum frequency of QoS requirement like interconnect,
it is not necessary. The next frequency of devfreq device
with 'passive' governor, it will apply the QoS requirement
without any additional code.

> 
> Please note that it is not an error when CONFIG_INTERCONNECT is 'n', in
> which case all interconnect API functions are no-op.
> 
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 145 +++++++++++++++++++++++++++++++++++
>  1 file changed, 145 insertions(+)
> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 412511ca7703..12fb7c84ae50 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -14,6 +14,7 @@
>  #include <linux/devfreq-event.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> +#include <linux/interconnect-provider.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/pm_opp.h>
> @@ -23,6 +24,8 @@
>  #define DEFAULT_SATURATION_RATIO	40
>  #define DEFAULT_VOLTAGE_TOLERANCE	2
>  
> +#define icc_units_to_hz(x) ((x) * 1000UL / 8)
> +
>  struct exynos_bus {
>  	struct device *dev;
>  
> @@ -31,12 +34,17 @@ struct exynos_bus {
>  	unsigned int edev_count;
>  	struct mutex lock;
>  
> +	unsigned long min_freq;
>  	unsigned long curr_freq;
>  
>  	struct regulator *regulator;
>  	struct clk *clk;
>  	unsigned int voltage_tolerance;
>  	unsigned int ratio;
> +
> +	/* One provider per bus, one node per provider */
> +	struct icc_provider provider;
> +	struct icc_node *node;
>  };
>  
>  /*
> @@ -61,6 +69,13 @@ exynos_bus_ops_edev(enable_edev);
>  exynos_bus_ops_edev(disable_edev);
>  exynos_bus_ops_edev(set_event);
>  
> +static int exynos_bus_next_id(void)
> +{
> +	static int exynos_bus_node_id;
> +
> +	return exynos_bus_node_id++;
> +}
> +
>  static int exynos_bus_get_event(struct exynos_bus *bus,
>  				struct devfreq_event_data *edata)
>  {
> @@ -98,6 +113,8 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>  	unsigned long old_freq, new_freq, new_volt, tol;
>  	int ret = 0;
>  
> +	*freq = max(*freq, bus->min_freq);
> +
>  	/* Get new opp-bus instance according to new bus clock */
>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>  	if (IS_ERR(new_opp)) {
> @@ -208,6 +225,8 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>  	unsigned long old_freq, new_freq;
>  	int ret = 0;
>  
> +	*freq = max(*freq, bus->min_freq);
> +
>  	/* Get new opp-bus instance according to new bus clock */
>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>  	if (IS_ERR(new_opp)) {
> @@ -251,6 +270,35 @@ static void exynos_bus_passive_exit(struct device *dev)
>  	clk_disable_unprepare(bus->clk);
>  }
>  
> +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> +	struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
> +
> +	src_bus->min_freq = icc_units_to_hz(src->peak_bw);
> +	dst_bus->min_freq = icc_units_to_hz(dst->peak_bw);
> +
> +	return 0;
> +}
> +
> +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 avg_bw,
> +				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> +{
> +	*agg_peak = *agg_avg = peak_bw;
> +
> +	return 0;
> +}
> +
> +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
> +					     void *data)
> +{
> +	struct exynos_bus *bus = data;
> +
> +	if (spec->np != bus->dev->of_node)
> +		return ERR_PTR(-EINVAL);
> +
> +	return bus->node;
> +}
> +
>  static int exynos_bus_parent_parse_of(struct device_node *np,
>  					struct exynos_bus *bus)
>  {
> @@ -469,6 +517,95 @@ static int exynos_bus_profile_init_passive(struct exynos_bus *bus,
>  	return ret;
>  }
>  
> +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> +{
> +	struct device_node *np = bus->dev->of_node;
> +	struct devfreq *parent_devfreq;
> +	struct icc_node *parent_node = NULL;
> +	struct of_phandle_args args;
> +	int ret = 0;
> +
> +	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> +	if (!IS_ERR(parent_devfreq)) {
> +		struct exynos_bus *parent_bus;
> +
> +		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> +		parent_node = parent_bus->node;
> +	} else {
> +		/* Look for parent in DT */
> +		int num = of_count_phandle_with_args(np, "parent",
> +						     "#interconnect-cells");
> +		if (num != 1)
> +			goto out;
> +
> +		ret = of_parse_phandle_with_args(np, "parent",
> +						 "#interconnect-cells",
> +						 0, &args);
> +		if (ret < 0)
> +			goto out;
> +
> +		of_node_put(args.np);
> +
> +		parent_node = of_icc_get_from_provider(&args);
> +		if (IS_ERR(parent_node)) {
> +			/* May be -EPROBE_DEFER */
> +			ret = PTR_ERR(parent_node);
> +			goto out;
> +		}
> +	}
> +
> +	ret = icc_link_create(bus->node, parent_node->id);
> +
> +out:
> +	return ret;
> +}
> +
> +static int exynos_bus_icc_init(struct exynos_bus *bus)
> +{
> +	struct device *dev = bus->dev;
> +	struct icc_provider *provider = &bus->provider;
> +	struct icc_node *node;
> +	int id, ret;
> +
> +	/* Initialize the interconnect provider */
> +	provider->set = exynos_bus_icc_set;
> +	provider->aggregate = exynos_bus_icc_aggregate;
> +	provider->xlate = exynos_bus_icc_xlate;
> +	provider->dev = dev;
> +	provider->data = bus;
> +
> +	ret = icc_provider_add(provider);
> +	if (ret < 0)
> +		goto out;
> +
> +	id = exynos_bus_next_id();
> +	node = icc_node_create(id);
> +	if (IS_ERR(node)) {
> +		ret = PTR_ERR(node);
> +		goto err_node;
> +	}
> +
> +	bus->node = node;
> +	node->name = dev->of_node->name;
> +	node->data = bus;
> +	icc_node_add(node, provider);
> +
> +	ret = exynos_bus_icc_connect(bus);
> +	if (ret < 0)
> +		goto err_connect;
> +
> +out:
> +	return ret;
> +
> +err_connect:
> +	icc_node_del(node);
> +	icc_node_destroy(id);
> +err_node:
> +	icc_provider_del(provider);
> +
> +	return ret;
> +}
> +
>  static int exynos_bus_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -517,6 +654,14 @@ static int exynos_bus_probe(struct platform_device *pdev)
>  			goto err;
>  	}
>  
> +	/*
> +	 * Initialize interconnect provider. A return value of -ENOTSUPP means
> +	 * that CONFIG_INTERCONNECT is disabled.
> +	 */
> +	ret = exynos_bus_icc_init(bus);
> +	if (ret < 0 && ret != -ENOTSUPP)
> +		goto err;
> +
>  	max_state = bus->devfreq->profile->max_state;
>  	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
>  	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Artur Świgoń]

Hi,

Thank you for your remarks. I will take them into account while preparing RFCv2.

On Mon, 2019-07-29 at 10:52 +0900, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 7. 23. 오후 9:20, Artur Świgoń wrote:
> > This patch adds interconnect functionality to the exynos-bus devfreq
> > driver.
> > 
> > The SoC topology is a graph (or, more specifically, a tree) and most of its
> > edges are taken from the devfreq parent-child hierarchy (cf.
> > Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
> > patch adds missing edges to the DT (under the name 'parent'). Due to
> > unspecified relative probing order, -EPROBE_DEFER may be propagated to
> > guarantee that a child is probed before its parent.
> > 
> > Each bus is now an interconnect provider and an interconnect node as well
> > (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
> > itself as a node. Node IDs are not hardcoded but rather assigned at
> > runtime, in probing order (subject to the above-mentioned exception
> > regarding relative order). This approach allows for using this driver with
> > various Exynos SoCs.
> > 
> > The devfreq target() callback provided by exynos-bus now selects either the
> > frequency calculated by the devfreq governor or the frequency requested via
> > the interconnect API for the given node, whichever is higher.
> 
> Basically, I agree to support the QoS requirement between devices.
> But, I think that need to consider the multiple cases.
> 
> 
> 1. When changing the devfreq governor by user,
> For example of the connection between bus_dmc/leftbus/display on patch8,
> there are possible multiple cases with various devfreq governor
> which is changed on the runtime by user through sysfs interface.
> 
> If users changes the devfreq governor as following:
> Before,
> - bus_dmc (simple_ondemand, available frequency 100/200/300/400 MHz)
> --> bus_leftbus(simple_ondemand, available frequency 100/200/300/400 MHz)
> ----> bus_display(passive)
> 
> After changed governor of bus_dmc,
> if the min_freq by interconnect requirement is 400Mhz,
> - bus_dmc (powersave) : min_freq and max_freq and cur_freq is 100MHz
> --> bus_leftbus(simple_ondemand) : cur_freq is 400Mhz
> ----> bus_display(passive)
> 
> The final frequency is 400MHz of bus_dmc
> even if the min_freq/max_freq/cur_freq is 100MHz.
> It cannot show the correct min_freq/max_freq through
> devfreq sysfs interface.
> 
> 
> 2. When disabling the some frequency by devfreq-thermal throttling,
> This patch checks the min_freq of interconnect requirement
> in the exynos_bus_target() and exynos_bus_passive_target().
> Also, it cannot show the correct min_freq/max_freq through
> devfreq sysfs interface.
> 
> For example of bus_dmc bus,
> - The available frequencies are 100MHz, 200MHz, 300MHz, 400MHz
> - Disable 400MHz by devfreq-thermal throttling 
> - min_freq is 100MHz
> - max_freq is 300MHz
> - min_freq of interconnect is 400MHz
> 
> In result, the final frequency is 400MHz by exynos_bus_target()
> There are no problem for working. But, the user cannot know
> reason why cur_freq is 400MHz even if max_freq is 300MHz.
> 
> Basically, update_devfreq() considers the all constraints
> of min_freq/max_freq to decide the proper target frequency.
> 
> 
> 3.
> I think that the exynos_bus_passive_target() is used for devfreq device
> using 'passive' governor. The frequency already depends on the parent device.
> 
> If already the parent devfreq device like bus_leftbus consider
> the minimum frequency of QoS requirement like interconnect,
> it is not necessary. The next frequency of devfreq device
> with 'passive' governor, it will apply the QoS requirement
> without any additional code.
> 
> > 
> > Please note that it is not an error when CONFIG_INTERCONNECT is 'n', in
> > which case all interconnect API functions are no-op.
> > 
> > Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> > ---
> >  drivers/devfreq/exynos-bus.c | 145 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 145 insertions(+)
> > 
> > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> > index 412511ca7703..12fb7c84ae50 100644
> > --- a/drivers/devfreq/exynos-bus.c
> > +++ b/drivers/devfreq/exynos-bus.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/devfreq-event.h>
> >  #include <linux/device.h>
> >  #include <linux/export.h>
> > +#include <linux/interconnect-provider.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/pm_opp.h>
> > @@ -23,6 +24,8 @@
> >  #define DEFAULT_SATURATION_RATIO	40
> >  #define DEFAULT_VOLTAGE_TOLERANCE	2
> >  
> > +#define icc_units_to_hz(x) ((x) * 1000UL / 8)
> > +
> >  struct exynos_bus {
> >  	struct device *dev;
> >  
> > @@ -31,12 +34,17 @@ struct exynos_bus {
> >  	unsigned int edev_count;
> >  	struct mutex lock;
> >  
> > +	unsigned long min_freq;
> >  	unsigned long curr_freq;
> >  
> >  	struct regulator *regulator;
> >  	struct clk *clk;
> >  	unsigned int voltage_tolerance;
> >  	unsigned int ratio;
> > +
> > +	/* One provider per bus, one node per provider */
> > +	struct icc_provider provider;
> > +	struct icc_node *node;
> >  };
> >  
> >  /*
> > @@ -61,6 +69,13 @@ exynos_bus_ops_edev(enable_edev);
> >  exynos_bus_ops_edev(disable_edev);
> >  exynos_bus_ops_edev(set_event);
> >  
> > +static int exynos_bus_next_id(void)
> > +{
> > +	static int exynos_bus_node_id;
> > +
> > +	return exynos_bus_node_id++;
> > +}
> > +
> >  static int exynos_bus_get_event(struct exynos_bus *bus,
> >  				struct devfreq_event_data *edata)
> >  {
> > @@ -98,6 +113,8 @@ static int exynos_bus_target(struct device *dev, unsigned
> > long *freq, u32 flags)
> >  	unsigned long old_freq, new_freq, new_volt, tol;
> >  	int ret = 0;
> >  
> > +	*freq = max(*freq, bus->min_freq);
> > +
> >  	/* Get new opp-bus instance according to new bus clock */
> >  	new_opp = devfreq_recommended_opp(dev, freq, flags);
> >  	if (IS_ERR(new_opp)) {
> > @@ -208,6 +225,8 @@ static int exynos_bus_passive_target(struct device *dev,
> > unsigned long *freq,
> >  	unsigned long old_freq, new_freq;
> >  	int ret = 0;
> >  
> > +	*freq = max(*freq, bus->min_freq);
> > +
> >  	/* Get new opp-bus instance according to new bus clock */
> >  	new_opp = devfreq_recommended_opp(dev, freq, flags);
> >  	if (IS_ERR(new_opp)) {
> > @@ -251,6 +270,35 @@ static void exynos_bus_passive_exit(struct device *dev)
> >  	clk_disable_unprepare(bus->clk);
> >  }
> >  
> > +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
> > +{
> > +	struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
> > +
> > +	src_bus->min_freq = icc_units_to_hz(src->peak_bw);
> > +	dst_bus->min_freq = icc_units_to_hz(dst->peak_bw);
> > +
> > +	return 0;
> > +}
> > +
> > +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 avg_bw,
> > +				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> > +{
> > +	*agg_peak = *agg_avg = peak_bw;
> > +
> > +	return 0;
> > +}
> > +
> > +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
> > +					     void *data)
> > +{
> > +	struct exynos_bus *bus = data;
> > +
> > +	if (spec->np != bus->dev->of_node)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	return bus->node;
> > +}
> > +
> >  static int exynos_bus_parent_parse_of(struct device_node *np,
> >  					struct exynos_bus *bus)
> >  {
> > @@ -469,6 +517,95 @@ static int exynos_bus_profile_init_passive(struct
> > exynos_bus *bus,
> >  	return ret;
> >  }
> >  
> > +static int exynos_bus_icc_connect(struct exynos_bus *bus)
> > +{
> > +	struct device_node *np = bus->dev->of_node;
> > +	struct devfreq *parent_devfreq;
> > +	struct icc_node *parent_node = NULL;
> > +	struct of_phandle_args args;
> > +	int ret = 0;
> > +
> > +	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
> > +	if (!IS_ERR(parent_devfreq)) {
> > +		struct exynos_bus *parent_bus;
> > +
> > +		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
> > +		parent_node = parent_bus->node;
> > +	} else {
> > +		/* Look for parent in DT */
> > +		int num = of_count_phandle_with_args(np, "parent",
> > +						     "#interconnect-cells");
> > +		if (num != 1)
> > +			goto out;
> > +
> > +		ret = of_parse_phandle_with_args(np, "parent",
> > +						 "#interconnect-cells",
> > +						 0, &args);
> > +		if (ret < 0)
> > +			goto out;
> > +
> > +		of_node_put(args.np);
> > +
> > +		parent_node = of_icc_get_from_provider(&args);
> > +		if (IS_ERR(parent_node)) {
> > +			/* May be -EPROBE_DEFER */
> > +			ret = PTR_ERR(parent_node);
> > +			goto out;
> > +		}
> > +	}
> > +
> > +	ret = icc_link_create(bus->node, parent_node->id);
> > +
> > +out:
> > +	return ret;
> > +}
> > +
> > +static int exynos_bus_icc_init(struct exynos_bus *bus)
> > +{
> > +	struct device *dev = bus->dev;
> > +	struct icc_provider *provider = &bus->provider;
> > +	struct icc_node *node;
> > +	int id, ret;
> > +
> > +	/* Initialize the interconnect provider */
> > +	provider->set = exynos_bus_icc_set;
> > +	provider->aggregate = exynos_bus_icc_aggregate;
> > +	provider->xlate = exynos_bus_icc_xlate;
> > +	provider->dev = dev;
> > +	provider->data = bus;
> > +
> > +	ret = icc_provider_add(provider);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	id = exynos_bus_next_id();
> > +	node = icc_node_create(id);
> > +	if (IS_ERR(node)) {
> > +		ret = PTR_ERR(node);
> > +		goto err_node;
> > +	}
> > +
> > +	bus->node = node;
> > +	node->name = dev->of_node->name;
> > +	node->data = bus;
> > +	icc_node_add(node, provider);
> > +
> > +	ret = exynos_bus_icc_connect(bus);
> > +	if (ret < 0)
> > +		goto err_connect;
> > +
> > +out:
> > +	return ret;
> > +
> > +err_connect:
> > +	icc_node_del(node);
> > +	icc_node_destroy(id);
> > +err_node:
> > +	icc_provider_del(provider);
> > +
> > +	return ret;
> > +}
> > +
> >  static int exynos_bus_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -517,6 +654,14 @@ static int exynos_bus_probe(struct platform_device
> > *pdev)
> >  			goto err;
> >  	}
> >  
> > +	/*
> > +	 * Initialize interconnect provider. A return value of -ENOTSUPP means
> > +	 * that CONFIG_INTERCONNECT is disabled.
> > +	 */
> > +	ret = exynos_bus_icc_init(bus);
> > +	if (ret < 0 && ret != -ENOTSUPP)
> > +		goto err;
> > +
> >  	max_state = bus->devfreq->profile->max_state;
> >  	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
> >  	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
> > 
> 
> 
-- 
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics

Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Chanwoo Choi]

Hi,

We need to discuss how to change or do refactoring on v2.
Actually, I don't know the your opinion and how to do it on v2.
You have to reply the answer and then after finished the discussion,
I recommend that you would rework and resend the v2 patches.

On 19. 8. 8. 오후 10:18, Artur Świgoń wrote:
> Hi,
> 
> Thank you for your remarks. I will take them into account while preparing RFCv2.
> 
> On Mon, 2019-07-29 at 10:52 +0900, Chanwoo Choi wrote:
>> Hi,
>>
>> On 19. 7. 23. 오후 9:20, Artur Świgoń wrote:
>>> This patch adds interconnect functionality to the exynos-bus devfreq
>>> driver.
>>>
>>> The SoC topology is a graph (or, more specifically, a tree) and most of its
>>> edges are taken from the devfreq parent-child hierarchy (cf.
>>> Documentation/devicetree/bindings/devfreq/exynos-bus.txt). The previous
>>> patch adds missing edges to the DT (under the name 'parent'). Due to
>>> unspecified relative probing order, -EPROBE_DEFER may be propagated to
>>> guarantee that a child is probed before its parent.
>>>
>>> Each bus is now an interconnect provider and an interconnect node as well
>>> (cf. Documentation/interconnect/interconnect.rst), i.e. every bus registers
>>> itself as a node. Node IDs are not hardcoded but rather assigned at
>>> runtime, in probing order (subject to the above-mentioned exception
>>> regarding relative order). This approach allows for using this driver with
>>> various Exynos SoCs.
>>>
>>> The devfreq target() callback provided by exynos-bus now selects either the
>>> frequency calculated by the devfreq governor or the frequency requested via
>>> the interconnect API for the given node, whichever is higher.
>>
>> Basically, I agree to support the QoS requirement between devices.
>> But, I think that need to consider the multiple cases.
>>
>>
>> 1. When changing the devfreq governor by user,
>> For example of the connection between bus_dmc/leftbus/display on patch8,
>> there are possible multiple cases with various devfreq governor
>> which is changed on the runtime by user through sysfs interface.
>>
>> If users changes the devfreq governor as following:
>> Before,
>> - bus_dmc (simple_ondemand, available frequency 100/200/300/400 MHz)
>> --> bus_leftbus(simple_ondemand, available frequency 100/200/300/400 MHz)
>> ----> bus_display(passive)
>>
>> After changed governor of bus_dmc,
>> if the min_freq by interconnect requirement is 400Mhz,
>> - bus_dmc (powersave) : min_freq and max_freq and cur_freq is 100MHz
>> --> bus_leftbus(simple_ondemand) : cur_freq is 400Mhz
>> ----> bus_display(passive)
>>
>> The final frequency is 400MHz of bus_dmc
>> even if the min_freq/max_freq/cur_freq is 100MHz.
>> It cannot show the correct min_freq/max_freq through
>> devfreq sysfs interface.
>>
>>
>> 2. When disabling the some frequency by devfreq-thermal throttling,
>> This patch checks the min_freq of interconnect requirement
>> in the exynos_bus_target() and exynos_bus_passive_target().
>> Also, it cannot show the correct min_freq/max_freq through
>> devfreq sysfs interface.
>>
>> For example of bus_dmc bus,
>> - The available frequencies are 100MHz, 200MHz, 300MHz, 400MHz
>> - Disable 400MHz by devfreq-thermal throttling 
>> - min_freq is 100MHz
>> - max_freq is 300MHz
>> - min_freq of interconnect is 400MHz
>>
>> In result, the final frequency is 400MHz by exynos_bus_target()
>> There are no problem for working. But, the user cannot know
>> reason why cur_freq is 400MHz even if max_freq is 300MHz.
>>
>> Basically, update_devfreq() considers the all constraints
>> of min_freq/max_freq to decide the proper target frequency.
>>
>>
>> 3.
>> I think that the exynos_bus_passive_target() is used for devfreq device
>> using 'passive' governor. The frequency already depends on the parent device.
>>
>> If already the parent devfreq device like bus_leftbus consider
>> the minimum frequency of QoS requirement like interconnect,
>> it is not necessary. The next frequency of devfreq device
>> with 'passive' governor, it will apply the QoS requirement
>> without any additional code.
>>
>>>
>>> Please note that it is not an error when CONFIG_INTERCONNECT is 'n', in
>>> which case all interconnect API functions are no-op.
>>>
>>> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
>>> ---
>>>  drivers/devfreq/exynos-bus.c | 145 +++++++++++++++++++++++++++++++++++
>>>  1 file changed, 145 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>> index 412511ca7703..12fb7c84ae50 100644
>>> --- a/drivers/devfreq/exynos-bus.c
>>> +++ b/drivers/devfreq/exynos-bus.c
>>> @@ -14,6 +14,7 @@
>>>  #include <linux/devfreq-event.h>
>>>  #include <linux/device.h>
>>>  #include <linux/export.h>
>>> +#include <linux/interconnect-provider.h>
>>>  #include <linux/module.h>
>>>  #include <linux/of.h>
>>>  #include <linux/pm_opp.h>
>>> @@ -23,6 +24,8 @@
>>>  #define DEFAULT_SATURATION_RATIO	40
>>>  #define DEFAULT_VOLTAGE_TOLERANCE	2
>>>  
>>> +#define icc_units_to_hz(x) ((x) * 1000UL / 8)
>>> +
>>>  struct exynos_bus {
>>>  	struct device *dev;
>>>  
>>> @@ -31,12 +34,17 @@ struct exynos_bus {
>>>  	unsigned int edev_count;
>>>  	struct mutex lock;
>>>  
>>> +	unsigned long min_freq;
>>>  	unsigned long curr_freq;
>>>  
>>>  	struct regulator *regulator;
>>>  	struct clk *clk;
>>>  	unsigned int voltage_tolerance;
>>>  	unsigned int ratio;
>>> +
>>> +	/* One provider per bus, one node per provider */
>>> +	struct icc_provider provider;
>>> +	struct icc_node *node;
>>>  };
>>>  
>>>  /*
>>> @@ -61,6 +69,13 @@ exynos_bus_ops_edev(enable_edev);
>>>  exynos_bus_ops_edev(disable_edev);
>>>  exynos_bus_ops_edev(set_event);
>>>  
>>> +static int exynos_bus_next_id(void)
>>> +{
>>> +	static int exynos_bus_node_id;
>>> +
>>> +	return exynos_bus_node_id++;
>>> +}
>>> +
>>>  static int exynos_bus_get_event(struct exynos_bus *bus,
>>>  				struct devfreq_event_data *edata)
>>>  {
>>> @@ -98,6 +113,8 @@ static int exynos_bus_target(struct device *dev, unsigned
>>> long *freq, u32 flags)
>>>  	unsigned long old_freq, new_freq, new_volt, tol;
>>>  	int ret = 0;
>>>  
>>> +	*freq = max(*freq, bus->min_freq);
>>> +
>>>  	/* Get new opp-bus instance according to new bus clock */
>>>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>  	if (IS_ERR(new_opp)) {
>>> @@ -208,6 +225,8 @@ static int exynos_bus_passive_target(struct device *dev,
>>> unsigned long *freq,
>>>  	unsigned long old_freq, new_freq;
>>>  	int ret = 0;
>>>  
>>> +	*freq = max(*freq, bus->min_freq);
>>> +
>>>  	/* Get new opp-bus instance according to new bus clock */
>>>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>  	if (IS_ERR(new_opp)) {
>>> @@ -251,6 +270,35 @@ static void exynos_bus_passive_exit(struct device *dev)
>>>  	clk_disable_unprepare(bus->clk);
>>>  }
>>>  
>>> +static int exynos_bus_icc_set(struct icc_node *src, struct icc_node *dst)
>>> +{
>>> +	struct exynos_bus *src_bus = src->data, *dst_bus = dst->data;
>>> +
>>> +	src_bus->min_freq = icc_units_to_hz(src->peak_bw);
>>> +	dst_bus->min_freq = icc_units_to_hz(dst->peak_bw);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 avg_bw,
>>> +				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
>>> +{
>>> +	*agg_peak = *agg_avg = peak_bw;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct icc_node *exynos_bus_icc_xlate(struct of_phandle_args *spec,
>>> +					     void *data)
>>> +{
>>> +	struct exynos_bus *bus = data;
>>> +
>>> +	if (spec->np != bus->dev->of_node)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	return bus->node;
>>> +}
>>> +
>>>  static int exynos_bus_parent_parse_of(struct device_node *np,
>>>  					struct exynos_bus *bus)
>>>  {
>>> @@ -469,6 +517,95 @@ static int exynos_bus_profile_init_passive(struct
>>> exynos_bus *bus,
>>>  	return ret;
>>>  }
>>>  
>>> +static int exynos_bus_icc_connect(struct exynos_bus *bus)
>>> +{
>>> +	struct device_node *np = bus->dev->of_node;
>>> +	struct devfreq *parent_devfreq;
>>> +	struct icc_node *parent_node = NULL;
>>> +	struct of_phandle_args args;
>>> +	int ret = 0;
>>> +
>>> +	parent_devfreq = devfreq_get_devfreq_by_phandle(bus->dev, 0);
>>> +	if (!IS_ERR(parent_devfreq)) {
>>> +		struct exynos_bus *parent_bus;
>>> +
>>> +		parent_bus = dev_get_drvdata(parent_devfreq->dev.parent);
>>> +		parent_node = parent_bus->node;
>>> +	} else {
>>> +		/* Look for parent in DT */
>>> +		int num = of_count_phandle_with_args(np, "parent",
>>> +						     "#interconnect-cells");
>>> +		if (num != 1)
>>> +			goto out;
>>> +
>>> +		ret = of_parse_phandle_with_args(np, "parent",
>>> +						 "#interconnect-cells",
>>> +						 0, &args);
>>> +		if (ret < 0)
>>> +			goto out;
>>> +
>>> +		of_node_put(args.np);
>>> +
>>> +		parent_node = of_icc_get_from_provider(&args);
>>> +		if (IS_ERR(parent_node)) {
>>> +			/* May be -EPROBE_DEFER */
>>> +			ret = PTR_ERR(parent_node);
>>> +			goto out;
>>> +		}
>>> +	}
>>> +
>>> +	ret = icc_link_create(bus->node, parent_node->id);
>>> +
>>> +out:
>>> +	return ret;
>>> +}
>>> +
>>> +static int exynos_bus_icc_init(struct exynos_bus *bus)
>>> +{
>>> +	struct device *dev = bus->dev;
>>> +	struct icc_provider *provider = &bus->provider;
>>> +	struct icc_node *node;
>>> +	int id, ret;
>>> +
>>> +	/* Initialize the interconnect provider */
>>> +	provider->set = exynos_bus_icc_set;
>>> +	provider->aggregate = exynos_bus_icc_aggregate;
>>> +	provider->xlate = exynos_bus_icc_xlate;
>>> +	provider->dev = dev;
>>> +	provider->data = bus;
>>> +
>>> +	ret = icc_provider_add(provider);
>>> +	if (ret < 0)
>>> +		goto out;
>>> +
>>> +	id = exynos_bus_next_id();
>>> +	node = icc_node_create(id);
>>> +	if (IS_ERR(node)) {
>>> +		ret = PTR_ERR(node);
>>> +		goto err_node;
>>> +	}
>>> +
>>> +	bus->node = node;
>>> +	node->name = dev->of_node->name;
>>> +	node->data = bus;
>>> +	icc_node_add(node, provider);
>>> +
>>> +	ret = exynos_bus_icc_connect(bus);
>>> +	if (ret < 0)
>>> +		goto err_connect;
>>> +
>>> +out:
>>> +	return ret;
>>> +
>>> +err_connect:
>>> +	icc_node_del(node);
>>> +	icc_node_destroy(id);
>>> +err_node:
>>> +	icc_provider_del(provider);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>  static int exynos_bus_probe(struct platform_device *pdev)
>>>  {
>>>  	struct device *dev = &pdev->dev;
>>> @@ -517,6 +654,14 @@ static int exynos_bus_probe(struct platform_device
>>> *pdev)
>>>  			goto err;
>>>  	}
>>>  
>>> +	/*
>>> +	 * Initialize interconnect provider. A return value of -ENOTSUPP means
>>> +	 * that CONFIG_INTERCONNECT is disabled.
>>> +	 */
>>> +	ret = exynos_bus_icc_init(bus);
>>> +	if (ret < 0 && ret != -ENOTSUPP)
>>> +		goto err;
>>> +
>>>  	max_state = bus->devfreq->profile->max_state;
>>>  	min_freq = (bus->devfreq->profile->freq_table[0] / 1000);
>>>  	max_freq = (bus->devfreq->profile->freq_table[max_state - 1] / 1000);
>>>
>>
>>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Leonard Crestez]

On 29.07.2019 04:49, Chanwoo Choi wrote:
> On 19. 7. 23. 오후 9:20, Artur Świgoń wrote:
>> This patch adds interconnect functionality to the exynos-bus devfreq
>> driver.
>>
>> The devfreq target() callback provided by exynos-bus now selects either the
>> frequency calculated by the devfreq governor or the frequency requested via
>> the interconnect API for the given node, whichever is higher.
> 
> Basically, I agree to support the QoS requirement between devices.
> But, I think that need to consider the multiple cases.
> 
> 1. When changing the devfreq governor by user,
> For example of the connection between bus_dmc/leftbus/display on patch8,
> there are possible multiple cases with various devfreq governor
> which is changed on the runtime by user through sysfs interface.
> 
> If users changes the devfreq governor as following:
> Before,
> - bus_dmc (simple_ondemand, available frequency 100/200/300/400 MHz)
> --> bus_leftbus(simple_ondemand, available frequency 100/200/300/400 MHz)
> ----> bus_display(passive)
> 
> After changed governor of bus_dmc,
> if the min_freq by interconnect requirement is 400Mhz,
> - bus_dmc (powersave) : min_freq and max_freq and cur_freq is 100MHz
> --> bus_leftbus(simple_ondemand) : cur_freq is 400Mhz
> ----> bus_display(passive)
> 
> The final frequency is 400MHz of bus_dmc
> even if the min_freq/max_freq/cur_freq is 100MHz.
> It cannot show the correct min_freq/max_freq through
> devfreq sysfs interface.
> 
> 
> 2. When disabling the some frequency by devfreq-thermal throttling,
> This patch checks the min_freq of interconnect requirement
> in the exynos_bus_target() and exynos_bus_passive_target().
> Also, it cannot show the correct min_freq/max_freq through
> devfreq sysfs interface.
> 
> For example of bus_dmc bus,
> - The available frequencies are 100MHz, 200MHz, 300MHz, 400MHz
> - Disable 400MHz by devfreq-thermal throttling
> - min_freq is 100MHz
> - max_freq is 300MHz
> - min_freq of interconnect is 400MHz
> 
> In result, the final frequency is 400MHz by exynos_bus_target()
> There are no problem for working. But, the user cannot know
> reason why cur_freq is 400MHz even if max_freq is 300MHz.
> 
> Basically, update_devfreq() considers the all constraints
> of min_freq/max_freq to decide the proper target frequency.

I think that applying the interconnect min_freq via dev_pm_qos can help 
with many of these concerns: update_devfreq controls all the min/max 
handling, sysfs is accurate and better decisions can be made regarding 
thermal. Enforcing constraints in the core is definitely better.

This wouldn't even be a very big change, you don't need to actually move 
the interconnect code outside of devfreq. Just make every devfreq/icc 
node register a dev_pm_qos_request for itself during registration and 
call dev_pm_qos_update_request inside exynos_bus_icc_set.

See: https://patchwork.kernel.org/patch/11084279/

--
Regards,
Leonard

Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Chanwoo Choi]

Hi Artur and Leonard,

On 19. 8. 9. 오전 12:00, Leonard Crestez wrote:
> On 29.07.2019 04:49, Chanwoo Choi wrote:
>> On 19. 7. 23. 오후 9:20, Artur Świgoń wrote:
>>> This patch adds interconnect functionality to the exynos-bus devfreq
>>> driver.
>>>
>>> The devfreq target() callback provided by exynos-bus now selects either the
>>> frequency calculated by the devfreq governor or the frequency requested via
>>> the interconnect API for the given node, whichever is higher.
>>
>> Basically, I agree to support the QoS requirement between devices.
>> But, I think that need to consider the multiple cases.
>>
>> 1. When changing the devfreq governor by user,
>> For example of the connection between bus_dmc/leftbus/display on patch8,
>> there are possible multiple cases with various devfreq governor
>> which is changed on the runtime by user through sysfs interface.
>>
>> If users changes the devfreq governor as following:
>> Before,
>> - bus_dmc (simple_ondemand, available frequency 100/200/300/400 MHz)
>> --> bus_leftbus(simple_ondemand, available frequency 100/200/300/400 MHz)
>> ----> bus_display(passive)
>>
>> After changed governor of bus_dmc,
>> if the min_freq by interconnect requirement is 400Mhz,
>> - bus_dmc (powersave) : min_freq and max_freq and cur_freq is 100MHz
>> --> bus_leftbus(simple_ondemand) : cur_freq is 400Mhz
>> ----> bus_display(passive)
>>
>> The final frequency is 400MHz of bus_dmc
>> even if the min_freq/max_freq/cur_freq is 100MHz.
>> It cannot show the correct min_freq/max_freq through
>> devfreq sysfs interface.
>>
>>
>> 2. When disabling the some frequency by devfreq-thermal throttling,
>> This patch checks the min_freq of interconnect requirement
>> in the exynos_bus_target() and exynos_bus_passive_target().
>> Also, it cannot show the correct min_freq/max_freq through
>> devfreq sysfs interface.
>>
>> For example of bus_dmc bus,
>> - The available frequencies are 100MHz, 200MHz, 300MHz, 400MHz
>> - Disable 400MHz by devfreq-thermal throttling
>> - min_freq is 100MHz
>> - max_freq is 300MHz
>> - min_freq of interconnect is 400MHz
>>
>> In result, the final frequency is 400MHz by exynos_bus_target()
>> There are no problem for working. But, the user cannot know
>> reason why cur_freq is 400MHz even if max_freq is 300MHz.
>>
>> Basically, update_devfreq() considers the all constraints
>> of min_freq/max_freq to decide the proper target frequency.
> 
> I think that applying the interconnect min_freq via dev_pm_qos can help 
> with many of these concerns: update_devfreq controls all the min/max 
> handling, sysfs is accurate and better decisions can be made regarding 
> thermal. Enforcing constraints in the core is definitely better.
> 
> This wouldn't even be a very big change, you don't need to actually move 
> the interconnect code outside of devfreq. Just make every devfreq/icc 
> node register a dev_pm_qos_request for itself during registration and 
> call dev_pm_qos_update_request inside exynos_bus_icc_set.
> 
> See: https://patchwork.kernel.org/patch/11084279/

I agree this approach of Leonard. If we use the dev_pm_qos[1] feature,
it resolve the issue of my comment1/2.

In summary, when receive the minimum frequency requirement
from interconnect path, the each bus uses the dev_pm_qos interface
in order to inform the minimum frequency from interconnect to devfreq.
And then the devfreq core will execute the update_devfreq()
with all frequency requirements as following:
- the user input (min/max frequency though devfreq sysfs
- the target frequency provided by devfreq governor
- the minimum frequency from interconnect path

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Leonard Crestez]

On 23.07.2019 15:21, Artur Świgoń wrote:

> +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 avg_bw,
> +				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> +{
> +	*agg_peak = *agg_avg = peak_bw;
> +
> +	return 0;
> +}

The only current provider aggregates "avg" with "sum" and "peak" with 
"max", any particular reason to do something different? This function 
doesn't even really do aggregation, if there is a second request for "0" 
then the first request is lost.

I didn't find any docs but my interpretation of avg/peak is that "avg" 
is for constant traffic like a display or VPU pushing pixels and "peak" 
is for variable traffic like networking. I assume devices which make 
"peak" requests are aggregated with max because they're not expected to 
all max-out together.

In PATCH 11 you're making a bandwidth request based on resolution, that 
traffic is constant and guaranteed to happend while the display is on so 
it would make sense to request it as an "avg" and aggregate it with "sum".

--
Regards,
Leonard

Re: [RFC PATCH 09/11] devfreq: exynos-bus: Add interconnect functionality to exynos-bus

[Artur Świgoń]

Hi,

Thank you for your comments.

On Tue, 2019-08-06 at 13:41 +0000, Leonard Crestez wrote:
> On 23.07.2019 15:21, Artur Świgoń wrote:
> 
> > +static int exynos_bus_icc_aggregate(struct icc_node *node, u32 avg_bw,
> > +				    u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> > +{
> > +	*agg_peak = *agg_avg = peak_bw;
> > +
> > +	return 0;
> > +}
> 
> The only current provider aggregates "avg" with "sum" and "peak" with 
> "max", any particular reason to do something different? This function 
> doesn't even really do aggregation, if there is a second request for "0" 
> then the first request is lost.

Yes, you're right. I adopted an oversimplified solution for the purpose of this
RFC (please bear in mind that currently only one icc_path is used, so there is
no aggregation anyway).

> I didn't find any docs but my interpretation of avg/peak is that "avg" 
> is for constant traffic like a display or VPU pushing pixels and "peak" 
> is for variable traffic like networking. I assume devices which make 
> "peak" requests are aggregated with max because they're not expected to 
> all max-out together.

That's correct (according to my understanding).

> In PATCH 11 you're making a bandwidth request based on resolution, that 
> traffic is constant and guaranteed to happend while the display is on so 
> it would make sense to request it as an "avg" and aggregate it with "sum".
> 
> --
> Regards,
> Leonard
> 
-- 
Artur Świgoń
Samsung R&D Institute Poland
Samsung Electronics

[RFC PATCH 10/11] arm: dts: exynos: Add interconnects to Exynos4412 mixer

[Artur Świgoń]

This patch adds an 'interconnects' property to Exynos4412 DTS in order to
declare the interconnect path used by the mixer. Please note that the
'interconnect-names' property is not needed when there is only one path in
'interconnects', in which case calling of_icc_get() with a NULL name simply
returns the right path.

Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
---
 arch/arm/boot/dts/exynos4412.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
index a70a671acacd..faaec6c40412 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -789,6 +789,7 @@
 	clock-names = "mixer", "hdmi", "sclk_hdmi", "vp";
 	clocks = <&clock CLK_MIXER>, <&clock CLK_HDMI>,
 		 <&clock CLK_SCLK_HDMI>, <&clock CLK_VP>;
+	interconnects = <&bus_display &bus_dmc>;
 };
 
 &pmu {
-- 
2.17.1

[RFC PATCH 11/11] drm: exynos: mixer: Add interconnect support

[Artur Świgoń]

From: Marek Szyprowski <m.szyprowski@samsung.com>

This patch adds interconnect support to exynos-mixer. Please note that the
mixer works the same as before when CONFIG_INTERCONNECT is 'n'.

Co-developed-by: Artur Świgoń <a.swigon@partner.samsung.com>
Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 68 +++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 7b24338fad3c..fb763854b300 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -13,6 +13,7 @@
 #include <linux/component.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/interconnect.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/kernel.h>
@@ -97,6 +98,7 @@ struct mixer_context {
 	struct exynos_drm_crtc	*crtc;
 	struct exynos_drm_plane	planes[MIXER_WIN_NR];
 	unsigned long		flags;
+	struct icc_path		*soc_path;
 
 	int			irq;
 	void __iomem		*mixer_regs;
@@ -931,6 +933,37 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
 	mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
 }
 
+static void mixer_set_memory_bandwidth(struct exynos_drm_crtc *crtc)
+{
+	struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
+	struct mixer_context *ctx = crtc->ctx;
+	unsigned long bw, bandwidth = 0;
+	int i, j, sub;
+
+	for (i = 0; i < MIXER_WIN_NR; i++) {
+		struct drm_plane *plane = &ctx->planes[i].base;
+		const struct drm_format_info *format;
+
+		if (plane->state && plane->state->crtc && plane->state->fb) {
+			format = plane->state->fb->format;
+			bw = mode->hdisplay * mode->vdisplay *
+							drm_mode_vrefresh(mode);
+			if (mode->flags & DRM_MODE_FLAG_INTERLACE)
+				bw /= 2;
+			for (j = 0; j < format->num_planes; j++) {
+				sub = j ? (format->vsub * format->hsub) : 1;
+				bandwidth += format->cpp[j] * bw / sub;
+			}
+		}
+	}
+
+	/* add 20% safety margin */
+	bandwidth = 5UL * bandwidth / 4;
+
+	pr_info("exynos-mixer: safe bandwidth %ld Bps\n", bandwidth);
+	icc_set_bw(ctx->soc_path, 0, Bps_to_icc(bandwidth));
+}
+
 static void mixer_atomic_begin(struct exynos_drm_crtc *crtc)
 {
 	struct mixer_context *ctx = crtc->ctx;
@@ -982,6 +1015,7 @@ static void mixer_atomic_flush(struct exynos_drm_crtc *crtc)
 	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
 		return;
 
+	mixer_set_memory_bandwidth(crtc);
 	mixer_enable_sync(mixer_ctx);
 	exynos_crtc_handle_event(crtc);
 }
@@ -1029,6 +1063,7 @@ static void mixer_disable(struct exynos_drm_crtc *crtc)
 	for (i = 0; i < MIXER_WIN_NR; i++)
 		mixer_disable_plane(crtc, &ctx->planes[i]);
 
+	mixer_set_memory_bandwidth(crtc);
 	exynos_drm_pipe_clk_enable(crtc, false);
 
 	pm_runtime_put(ctx->dev);
@@ -1220,12 +1255,22 @@ static int mixer_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	const struct mixer_drv_data *drv;
 	struct mixer_context *ctx;
+	struct icc_path *path;
 	int ret;
 
+	/*
+	 * Returns NULL if CONFIG_INTERCONNECT is disabled.
+	 * May return ERR_PTR(-EPROBE_DEFER).
+	 */
+	path = of_icc_get(dev, NULL);
+	if (IS_ERR(path))
+		return PTR_ERR(path);
+
 	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx) {
 		DRM_DEV_ERROR(dev, "failed to alloc mixer context.\n");
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	drv = of_device_get_match_data(dev);
@@ -1233,6 +1278,7 @@ static int mixer_probe(struct platform_device *pdev)
 	ctx->pdev = pdev;
 	ctx->dev = dev;
 	ctx->mxr_ver = drv->version;
+	ctx->soc_path = path;
 
 	if (drv->is_vp_enabled)
 		__set_bit(MXR_BIT_VP_ENABLED, &ctx->flags);
@@ -1242,17 +1288,29 @@ static int mixer_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, ctx);
 
 	ret = component_add(&pdev->dev, &mixer_component_ops);
-	if (!ret)
-		pm_runtime_enable(dev);
+	if (ret < 0)
+		goto err;
+
+	pm_runtime_enable(dev);
+
+	return 0;
+
+err:
+	icc_put(path);
 
 	return ret;
 }
 
 static int mixer_remove(struct platform_device *pdev)
 {
-	pm_runtime_disable(&pdev->dev);
+	struct device *dev = &pdev->dev;
+	struct mixer_context *ctx = dev_get_drvdata(dev);
 
-	component_del(&pdev->dev, &mixer_component_ops);
+	pm_runtime_disable(dev);
+
+	component_del(dev, &mixer_component_ops);
+
+	icc_put(ctx->soc_path);
 
 	return 0;
 }
-- 
2.17.1

Re: [RFC PATCH 11/11] drm: exynos: mixer: Add interconnect support

[Krzysztof Kozlowski]

On Tue, Jul 23, 2019 at 02:20:16PM +0200, Artur Świgoń wrote:
> From: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> This patch adds interconnect support to exynos-mixer. Please note that the
> mixer works the same as before when CONFIG_INTERCONNECT is 'n'.
> 
> Co-developed-by: Artur Świgoń <a.swigon@partner.samsung.com>
> Signed-off-by: Artur Świgoń <a.swigon@partner.samsung.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 68 +++++++++++++++++++++++++--
>  1 file changed, 63 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 7b24338fad3c..fb763854b300 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -13,6 +13,7 @@
>  #include <linux/component.h>
>  #include <linux/delay.h>
>  #include <linux/i2c.h>
> +#include <linux/interconnect.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
> @@ -97,6 +98,7 @@ struct mixer_context {
>  	struct exynos_drm_crtc	*crtc;
>  	struct exynos_drm_plane	planes[MIXER_WIN_NR];
>  	unsigned long		flags;
> +	struct icc_path		*soc_path;
>  
>  	int			irq;
>  	void __iomem		*mixer_regs;
> @@ -931,6 +933,37 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
>  	mixer_reg_writemask(mixer_ctx, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
>  }
>  
> +static void mixer_set_memory_bandwidth(struct exynos_drm_crtc *crtc)
> +{
> +	struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
> +	struct mixer_context *ctx = crtc->ctx;
> +	unsigned long bw, bandwidth = 0;
> +	int i, j, sub;
> +

Early exit if !ctx->soc_path, no need to figure out the bandwidth.
Optionally check it before calling mixer_set_memory_bandwidth() - should
not hurt readability.

> +	for (i = 0; i < MIXER_WIN_NR; i++) {
> +		struct drm_plane *plane = &ctx->planes[i].base;
> +		const struct drm_format_info *format;
> +
> +		if (plane->state && plane->state->crtc && plane->state->fb) {
> +			format = plane->state->fb->format;
> +			bw = mode->hdisplay * mode->vdisplay *
> +							drm_mode_vrefresh(mode);
> +			if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> +				bw /= 2;
> +			for (j = 0; j < format->num_planes; j++) {
> +				sub = j ? (format->vsub * format->hsub) : 1;
> +				bandwidth += format->cpp[j] * bw / sub;
> +			}
> +		}
> +	}
> +
> +	/* add 20% safety margin */
> +	bandwidth = 5UL * bandwidth / 4;
> +
> +	pr_info("exynos-mixer: safe bandwidth %ld Bps\n", bandwidth);

dev_dbg()

Best regards,
Krzysztof

Re: [RFC PATCH 00/11] Simple QoS for exynos-bus driver using interconnect

[Krzysztof Kozlowski]

On Tue, Jul 23, 2019 at 02:20:05PM +0200, Artur Świgoń wrote:
> The following patchset adds interconnect[1][2] framework support to the
> exynos-bus devfreq driver. Extending the devfreq driver with interconnect
> capabilities started as a response to the issue referenced in [3]. The
> patches can be subdivided into four logical groups:

Nice work! Good to see proper solution :)

Best regards,
Krzysztof

Re: [RFC PATCH 00/11] Simple QoS for exynos-bus driver using interconnect

[Chanwoo Choi]

Hi Artur.

The patch1-4 in this series depend on other patches[1] on mainline.
On next v2 version, please make some patches based on patches[1]
in order to prevent the merge conflict. 

[1] [RESEND PATCH v5 0/4] add coupled regulators for Exynos5422/5800
- https://lkml.org/lkml/2019/8/8/217

Also, as I commented, we better to discuss it before sending the v2.

On 19. 7. 23. 오후 9:20, Artur Świgoń wrote:
> The following patchset adds interconnect[1][2] framework support to the
> exynos-bus devfreq driver. Extending the devfreq driver with interconnect
> capabilities started as a response to the issue referenced in [3]. The
> patches can be subdivided into four logical groups:
> 
> (a) Refactoring the existing devfreq driver in order to improve readability
> and accommodate for adding new code (patches 01--04/11).
> 
> (b) Tweaking the interconnect framework to support the exynos-bus use case
> (patches 05--07/11). Exporting of_icc_get_from_provider() allows us to
> avoid hardcoding every single graph edge in the DT or driver source, and
> relaxing the requirement contained in that function removes the need to
> provide dummy node IDs in the DT. Adjusting the logic in
> apply_constraints() (drivers/interconnect/core.c) accounts for the fact
> that every bus is a separate entity and therefore a separate interconnect
> provider, albeit constituting a part of a larger hierarchy.
> 
> (c) Implementing interconnect providers in the exynos-bus devfreq driver
> and adding required DT properties for one selected platform, namely
> Exynos4412 (patches 08--09/11). Due to the fact that this aims to be a
> generic driver for various Exynos SoCs, node IDs are generated dynamically
> rather than hardcoded. This has been determined to be a simpler approach,
> but depends on changes described in (b).
> 
> (d) Implementing a sample interconnect consumer for exynos-mixer targeted
> at the issue referenced in [3], again with DT info only for Exynos4412
> (patches 10--11/11).
> 
> Integration of devfreq and interconnect functionalities comes down to one
> extra line in the devfreq target() callback, which selects either the
> frequency calculated by the devfreq governor, or the one requested with the
> interconnect API, whichever is higher. All new code works equally well when
> CONFIG_INTERCONNECT is 'n' (as in exynos_defconfig) in which case all
> interconnect API functions are no-ops.
> 
> ---
> Artur Świgoń
> Samsung R&D Institute Poland
> Samsung Electronics
> 
> ---
> References:
> [1] Documentation/interconnect/interconnect.rst
> [2] Documentation/devicetree/bindings/interconnect/interconnect.txt
> [3] https://patchwork.kernel.org/patch/10861757
> 
> Artur Świgoń (10):
>   devfreq: exynos-bus: Extract exynos_bus_profile_init()
>   devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()
>   devfreq: exynos-bus: Change goto-based logic to if-else logic
>   devfreq: exynos-bus: Clean up code
>   icc: Export of_icc_get_from_provider()
>   icc: Relax requirement in of_icc_get_from_provider()
>   icc: Relax condition in apply_constraints()
>   arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412
>   devfreq: exynos-bus: Add interconnect functionality to exynos-bus
>   arm: dts: exynos: Add interconnects to Exynos4412 mixer
> 
> Marek Szyprowski (1):
>   drm: exynos: mixer: Add interconnect support
> 
>  .../boot/dts/exynos4412-odroid-common.dtsi    |   1 +
>  arch/arm/boot/dts/exynos4412.dtsi             |  10 +
>  drivers/devfreq/exynos-bus.c                  | 296 ++++++++++++++----
>  drivers/gpu/drm/exynos/exynos_mixer.c         |  68 +++-
>  drivers/interconnect/core.c                   |  12 +-
>  include/linux/interconnect-provider.h         |   6 +
>  6 files changed, 314 insertions(+), 79 deletions(-)
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [RFC PATCH 00/11] Simple QoS for exynos-bus driver using interconnect

[Chanwoo Choi]

Hi Artur,

On 19. 8. 13. 오후 3:17, Chanwoo Choi wrote:
> Hi Artur.
> 
> The patch1-4 in this series depend on other patches[1] on mainline.
> On next v2 version, please make some patches based on patches[1]
> in order to prevent the merge conflict. 
> 
> [1] [RESEND PATCH v5 0/4] add coupled regulators for Exynos5422/5800
> - https://protect2.fireeye.com/url?k=4f35944fb07b6ba2.4f341f00-9498e831e3c86bfb&u=https://lkml.org/lkml/2019/8/8/217

Add correct reference url as following:
- https://lkml.org/lkml/2019/8/8/217

> 
> Also, as I commented, we better to discuss it before sending the v2.
> 
> On 19. 7. 23. 오후 9:20, Artur Świgoń wrote:
>> The following patchset adds interconnect[1][2] framework support to the
>> exynos-bus devfreq driver. Extending the devfreq driver with interconnect
>> capabilities started as a response to the issue referenced in [3]. The
>> patches can be subdivided into four logical groups:
>>
>> (a) Refactoring the existing devfreq driver in order to improve readability
>> and accommodate for adding new code (patches 01--04/11).
>>
>> (b) Tweaking the interconnect framework to support the exynos-bus use case
>> (patches 05--07/11). Exporting of_icc_get_from_provider() allows us to
>> avoid hardcoding every single graph edge in the DT or driver source, and
>> relaxing the requirement contained in that function removes the need to
>> provide dummy node IDs in the DT. Adjusting the logic in
>> apply_constraints() (drivers/interconnect/core.c) accounts for the fact
>> that every bus is a separate entity and therefore a separate interconnect
>> provider, albeit constituting a part of a larger hierarchy.
>>
>> (c) Implementing interconnect providers in the exynos-bus devfreq driver
>> and adding required DT properties for one selected platform, namely
>> Exynos4412 (patches 08--09/11). Due to the fact that this aims to be a
>> generic driver for various Exynos SoCs, node IDs are generated dynamically
>> rather than hardcoded. This has been determined to be a simpler approach,
>> but depends on changes described in (b).
>>
>> (d) Implementing a sample interconnect consumer for exynos-mixer targeted
>> at the issue referenced in [3], again with DT info only for Exynos4412
>> (patches 10--11/11).
>>
>> Integration of devfreq and interconnect functionalities comes down to one
>> extra line in the devfreq target() callback, which selects either the
>> frequency calculated by the devfreq governor, or the one requested with the
>> interconnect API, whichever is higher. All new code works equally well when
>> CONFIG_INTERCONNECT is 'n' (as in exynos_defconfig) in which case all
>> interconnect API functions are no-ops.
>>
>> ---
>> Artur Świgoń
>> Samsung R&D Institute Poland
>> Samsung Electronics
>>
>> ---
>> References:
>> [1] Documentation/interconnect/interconnect.rst
>> [2] Documentation/devicetree/bindings/interconnect/interconnect.txt
>> [3] https://patchwork.kernel.org/patch/10861757
>>
>> Artur Świgoń (10):
>>   devfreq: exynos-bus: Extract exynos_bus_profile_init()
>>   devfreq: exynos-bus: Extract exynos_bus_profile_init_passive()
>>   devfreq: exynos-bus: Change goto-based logic to if-else logic
>>   devfreq: exynos-bus: Clean up code
>>   icc: Export of_icc_get_from_provider()
>>   icc: Relax requirement in of_icc_get_from_provider()
>>   icc: Relax condition in apply_constraints()
>>   arm: dts: exynos: Add parents and #interconnect-cells to Exynos4412
>>   devfreq: exynos-bus: Add interconnect functionality to exynos-bus
>>   arm: dts: exynos: Add interconnects to Exynos4412 mixer
>>
>> Marek Szyprowski (1):
>>   drm: exynos: mixer: Add interconnect support
>>
>>  .../boot/dts/exynos4412-odroid-common.dtsi    |   1 +
>>  arch/arm/boot/dts/exynos4412.dtsi             |  10 +
>>  drivers/devfreq/exynos-bus.c                  | 296 ++++++++++++++----
>>  drivers/gpu/drm/exynos/exynos_mixer.c         |  68 +++-
>>  drivers/interconnect/core.c                   |  12 +-
>>  include/linux/interconnect-provider.h         |   6 +
>>  6 files changed, 314 insertions(+), 79 deletions(-)
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics