LKML Archive on lore.kernel.org
 help / Atom feed
From: Sibi Sankar <sibis@codeaurora.org>
To: Georgi Djakov <georgi.djakov@linaro.org>
Cc: Saravana Kannan <skannan@codeaurora.org>,
	MyungJoo Ham <myungjoo.ham@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	vincent.guittot@linaro.org, daidavid1@codeaurora.org,
	bjorn.andersson@linaro.org, linux-pm@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-kernel-owner@vger.kernel.org
Subject: Re: [PATCH v3 2/2] PM / devfreq: Add devfreq driver for interconnect bandwidth voting
Date: Tue, 11 Sep 2018 00:25:32 +0530
Message-ID: <904fb7d5f96a0c02bd57f21c50549192@codeaurora.org> (raw)
In-Reply-To: <4fcac639-cba9-1a71-9b78-326ebb9242da@linaro.org>

Hi Georgi,

This driver uses of_icc_get which is very likely to fail if it probes
before the interconnect provider. Would it be possible for icc_get to
return/differentiate both -EPROBE_DEFER and other errors to prevent the
driver to continually probe defer if the path doesn't actually exist
or just error out if it probes before the interconnect provider.

On 2018-08-23 18:30, Georgi Djakov wrote:
> Hi Saravana,
> 
> On 08/02/2018 03:57 AM, Saravana Kannan wrote:
>> This driver registers itself as a devfreq device that allows devfreq
>> governors to make bandwidth votes for an interconnect path. This 
>> allows
>> applying various policies for different interconnect paths using 
>> devfreq
>> governors.
>> 
>> Example uses:
>> * Use the devfreq performance governor to set the CPU to DDR 
>> interconnect
>>   path for maximum performance.
>> * Use the devfreq performance governor to set the GPU to DDR 
>> interconnect
>>   path for maximum performance.
>> * Use the CPU frequency to device frequency mapping governor to scale 
>> the
>>   DDR frequency based on the needs of the CPUs' current frequency.
> 
> Usually CPUs and GPUs have dedicated cpufreq/devfreq drivers and i was
> wondering if the interconnect support could be put into these drivers
> directly?
> 
>> 
>> Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> ---
>>  Documentation/devicetree/bindings/devfreq/icbw.txt |  21 ++++
>>  drivers/devfreq/Kconfig                            |  13 +++
>>  drivers/devfreq/Makefile                           |   1 +
>>  drivers/devfreq/devfreq_icbw.c                     | 116 
>> +++++++++++++++++++++
>>  4 files changed, 151 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/devfreq/icbw.txt
>>  create mode 100644 drivers/devfreq/devfreq_icbw.c
>> 
>> diff --git a/Documentation/devicetree/bindings/devfreq/icbw.txt 
>> b/Documentation/devicetree/bindings/devfreq/icbw.txt
>> new file mode 100644
>> index 0000000..36cf045
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/devfreq/icbw.txt
>> @@ -0,0 +1,21 @@
>> +Interconnect bandwidth device
>> +
>> +icbw is a device that represents an interconnect path that connects 
>> two
>> +devices. This device is typically used to vote for BW requirements 
>> between
>> +two devices. Eg: CPU to DDR, GPU to DDR, etc
>> +
>> +Required properties:
>> +- compatible:		Must be "devfreq-icbw"
>> +- interconnects:	Pairs of phandles and interconnect provider 
>> specifier
>> +			to denote the edge source and destination ports of
>> +			the interconnect path. See also:
>> +		Documentation/devicetree/bindings/interconnect/interconnect.txt
>> +- interconnect-names:	Must have one entry with the name "path".
>> +
>> +Example:
>> +
>> +	qcom,cpubw {
>> +		compatible = "devfreq-icbw";
>> +		interconnects = <&snoc MASTER_APSS_1 &bimc SLAVE_EBI_CH0>;
>> +		interconnect-names = "path";
> 
> interconnect-names is optional when there is only a single path.
> 
>> +	};
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 3d9ae68..590370e 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -121,6 +121,19 @@ config ARM_RK3399_DMC_DEVFREQ
>>            It sets the frequency for the memory controller and reads 
>> the usage counts
>>            from hardware.
>> 
>> +config DEVFREQ_ICBW
>> +	bool "DEVFREQ device for making bandwidth votes on interconnect 
>> paths"
> 
> Can this be a module?
> 
>> +	select DEVFREQ_GOV_PERFORMANCE
>> +	select DEVFREQ_GOV_POWERSAVE
>> +	select DEVFREQ_GOV_USERSPACE
>> +	default n
> 
> There's no need to specify this default. It is 'n' by default anyway.
> Also maybe you want to add something like:
> 	depends on INTERCONNECT=y
> 
> Thanks,
> Georgi
> 
>> +	help
>> +	  Different devfreq governors use this devfreq device to make
>> +	  bandwidth votes for interconnect paths between different devices
>> +	  (Eg: CPU to DDR, GPU to DDR, etc). This driver provides a generic
>> +	  interface so that the devfreq governors can be shared across SoCs
>> +	  and architectures.
>> +
>>  source "drivers/devfreq/event/Kconfig"

-- 
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-02  0:57 Saravana Kannan
     [not found] ` <CGME20180802005759epcas4p4a68d50e61c8425124a993de1917021a2@epcms1p5>
2018-08-02 10:13   ` MyungJoo Ham
2018-08-02 13:15     ` Georgi Djakov
2018-08-02 19:07     ` skannan
2018-08-07 16:51 ` Rob Herring
2018-08-07 19:31   ` skannan
2018-08-23 13:00 ` Georgi Djakov
2018-09-10 18:55   ` Sibi Sankar [this message]
2018-09-14 12:53 ` Sibi Sankar

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=904fb7d5f96a0c02bd57f21c50549192@codeaurora.org \
    --to=sibis@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=cw00.choi@samsung.com \
    --cc=daidavid1@codeaurora.org \
    --cc=devicetree@vger.kernel.org \
    --cc=georgi.djakov@linaro.org \
    --cc=kyungmin.park@samsung.com \
    --cc=linux-kernel-owner@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=myungjoo.ham@samsung.com \
    --cc=robh+dt@kernel.org \
    --cc=skannan@codeaurora.org \
    --cc=vincent.guittot@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox