Linux-SPI Archive on lore.kernel.org
 help / color / Atom feed
From: Matthias Kaehlcke <mka@chromium.org>
To: Akash Asthana <akashast@codeaurora.org>
Cc: gregkh@linuxfoundation.org, agross@kernel.org,
	bjorn.andersson@linaro.org, wsa@the-dreams.de,
	broonie@kernel.org, mark.rutland@arm.com, robh+dt@kernel.org,
	linux-i2c@vger.kernel.org, linux-spi@vger.kernel.org,
	devicetree@vger.kernel.org, swboyd@chromium.org,
	mgautam@codeaurora.org, linux-arm-msm@vger.kernel.org,
	linux-serial@vger.kernel.org, dianders@chromium.org,
	evgreen@chromium.org
Subject: Re: [PATCH V2 5/8] i2c: i2c-qcom-geni: Add interconnect support
Date: Fri, 13 Mar 2020 17:17:28 -0700
Message-ID: <20200314001728.GL144492@google.com> (raw)
In-Reply-To: <1584105134-13583-6-git-send-email-akashast@codeaurora.org>

Hi,

On Fri, Mar 13, 2020 at 06:42:11PM +0530, Akash Asthana wrote:
> Get the interconnect paths for I2C based Serial Engine device
> and vote according to the bus speed of the driver.
> 
> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> ---
> Changes in V2:
>  - As per Bjorn's comment, removed se == NULL check from geni_i2c_icc_get
>  - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure
>  - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting
>    path handle
>  - As per Matthias comment, added error handling for icc_set_bw call
> 
>  drivers/i2c/busses/i2c-qcom-geni.c | 110 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 17abf60c..33ab685 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -163,6 +163,23 @@ static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
>  	writel_relaxed(val, gi2c->se.base + SE_I2C_SCL_COUNTERS);
>  }
>  
> +static int geni_i2c_icc_get(struct geni_se *se)
> +{
> +	se->icc_path_geni_to_core = devm_of_icc_get(se->dev, "qup-core");
> +	if (IS_ERR(se->icc_path_geni_to_core))
> +		return PTR_ERR(se->icc_path_geni_to_core);
> +
> +	se->icc_path_cpu_to_geni = devm_of_icc_get(se->dev, "qup-config");
> +	if (IS_ERR(se->icc_path_cpu_to_geni))
> +		return PTR_ERR(se->icc_path_cpu_to_geni);
> +
> +	se->icc_path_geni_to_ddr = devm_of_icc_get(se->dev, "qup-memory");
> +	if (IS_ERR(se->icc_path_geni_to_ddr))
> +		return PTR_ERR(se->icc_path_geni_to_ddr);
> +
> +	return 0;
> +}
> +
>  static void geni_i2c_err_misc(struct geni_i2c_dev *gi2c)
>  {
>  	u32 m_cmd = readl_relaxed(gi2c->se.base + SE_GENI_M_CMD0);
> @@ -563,6 +580,39 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  	gi2c->adap.dev.of_node = pdev->dev.of_node;
>  	strlcpy(gi2c->adap.name, "Geni-I2C", sizeof(gi2c->adap.name));
>  
> +	ret = geni_i2c_icc_get(&gi2c->se);
> +	if (ret)
> +		return ret;
> +	/*
> +	 * Set the bus quota for core and cpu to a reasonable value for
> +	 * register access.
> +	 * Set quota for DDR based on bus speed, assume peak requirement
> +	 * as twice of avg bw.
> +	 */
> +	gi2c->se.avg_bw_core = Bps_to_icc(1000);

as commented on the UART patch, you might want to consider adding a constant
for this recurring default value.

> +	gi2c->se.peak_bw_core = Bps_to_icc(CORE_2X_100_MHZ);
> +	gi2c->se.avg_bw_cpu = Bps_to_icc(1000);
> +	gi2c->se.peak_bw_cpu = Bps_to_icc(1000);
> +	gi2c->se.avg_bw_ddr = Bps_to_icc(gi2c->clk_freq_out);
> +	gi2c->se.peak_bw_ddr = Bps_to_icc(2 * gi2c->clk_freq_out);
> +
> +	/* Vote for core clocks and CPU for register access */
> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
> +				gi2c->se.peak_bw_core);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: ICC BW voting failed for core\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, gi2c->se.avg_bw_cpu,
> +				gi2c->se.peak_bw_cpu);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: ICC BW voting failed for cpu\n",
> +			__func__);
> +		return ret;
> +	}
> +
>  	ret = geni_se_resources_on(&gi2c->se);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Error turning on resources %d\n", ret);
> @@ -584,6 +634,19 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "Error turning off resources %d\n", ret);
>  		return ret;
>  	}
> +	/* Remove vote from core clocks and CPU */
> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, 0, 0);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: ICC BW remove failed for core\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, 0, 0);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s: ICC BW remove failed for cpu\n",
> +			__func__);

Should this return an error as for the core clocks?

> +	}
>  
>  	dev_dbg(&pdev->dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
>  
> @@ -629,6 +692,28 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>  		gi2c->suspended = 1;
>  	}
>  
> +	/* Remove BW votes */
> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, 0, 0);
> +	if (ret) {
> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for core\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, 0, 0);
> +	if (ret) {
> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for cpu\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_ddr, 0, 0);
> +	if (ret) {
> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for ddr\n",
> +			__func__);
> +		return ret;
> +	}

you could consider putting this and the ICC voting in _runtime_resume() in
separate functions and call them from here and _probe(). The only difference
in _probe() is that it doesn't vote for the DDR bandwidth, but that slight
one-time overhead might be worth for consolidating the code.

Actually all this looks very similar for UART, I2C, SPI and QSPI. It seems it
should be possible to consolidate this further by having functions like these
in the geni SE driver:

int geni_icc_get(struct geni_se *se, const char *icc_core, const char *icc_cpu,
	const char *icc_ddr)
{
	if (icc_core) {
		se->icc_path_geni_to_core = devm_of_icc_get(se->dev, icc_core);
		if (IS_ERR(se->icc_path_geni_to_core))
			return PTR_ERR(se->icc_path_geni_to_core);
	}

	if (icc_cpu) {

	...
}

int geni_icc_vote_on(struct geni_se *se)
{
	if (gi2c->se.icc_path_geni_to_core) {
		ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
			gi2c->se.peak_bw_core);
		if (ret) {
			dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for core\n",
				__func__);
			return ret;
		}
	}

	/* or to reduce nesting:
	if (!gi2c->se.icc_path_geni_to_core)
		goto vote_cpu;

	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
			gi2c->se.peak_bw_core);
	if (ret) {
	...

vote_cpu:
	*/

	if (gi2c->se.icc_path_cpu_to_geni) {

	...
}

int geni_icc_vote_off(struct geni_se *se)
{
	if (gi2c->se.icc_path_geni_to_core) {
		ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, 0, 0);
		if (ret) {
			dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW remove failed for core\n",
				__func__);
			return ret;
		}
	}

	if (gi2c->se.icc_path_cpu_to_geni) {

	...
}

optionally you could even reduce the code further by having an array of
'struct geni_icc_path' (as suggested on https://patchwork.kernel.org/patch/11436889/#23221925)
and iterate over the array instead of spelling everything out for the 3
ICC paths.

> +
>  	return 0;
>  }
>  
> @@ -637,6 +722,31 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>  	int ret;
>  	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>  
> +	/* Vote on Core, CPU and DDR path respectively */
> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_core, gi2c->se.avg_bw_core,
> +		gi2c->se.peak_bw_core);
> +	if (ret) {
> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for core\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(gi2c->se.icc_path_cpu_to_geni, gi2c->se.avg_bw_cpu,
> +		gi2c->se.peak_bw_cpu);
> +	if (ret) {
> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for cpu\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = icc_set_bw(gi2c->se.icc_path_geni_to_ddr, gi2c->se.avg_bw_ddr,
> +		gi2c->se.peak_bw_ddr);
> +	if (ret) {
> +		dev_err_ratelimited(gi2c->se.dev, "%s: ICC BW voting failed for ddr\n",
> +			__func__);
> +		return ret;
> +	}
> +

as per above this would be just:

	ret = geni_icc_vote_on(&gi2c->se);
	if (ret)
		return ret;

with the same benefit for the other 3 drivers.

  reply index

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-13 13:12 [PATCH V2 0/8] Add interconnect support to QSPI and QUP drivers Akash Asthana
     [not found] ` <1584105134-13583-1-git-send-email-akashast-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-13 13:12   ` [PATCH V2 1/8] interconnect: Add devm_of_icc_get() as exported API for users Akash Asthana
2020-03-13 16:26     ` Matthias Kaehlcke
2020-03-27 23:02     ` Bjorn Andersson
2020-03-13 13:12   ` [PATCH V2 4/8] tty: serial: qcom_geni_serial: Add interconnect support Akash Asthana
     [not found]     ` <1584105134-13583-5-git-send-email-akashast-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-13 21:28       ` Matthias Kaehlcke
     [not found]         ` <20200313212833.GK144492-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-17 11:48           ` Akash Asthana
2020-03-17 19:08             ` Matthias Kaehlcke
2020-03-18 12:23               ` Akash Asthana
     [not found]                 ` <e95bd6e0-d1fd-9d13-47df-f7d08b443e37-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-19 20:42                   ` Matthias Kaehlcke
     [not found]                     ` <20200319204248.GA204494-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-20 10:35                       ` Akash Asthana
2020-03-13 13:12   ` [PATCH V2 5/8] i2c: i2c-qcom-geni: " Akash Asthana
2020-03-14  0:17     ` Matthias Kaehlcke [this message]
2020-03-17 11:51       ` Akash Asthana
2020-03-13 13:12   ` [PATCH V2 6/8] spi: spi-geni-qcom: " Akash Asthana
     [not found]     ` <1584105134-13583-7-git-send-email-akashast-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-13 13:16       ` Mark Brown
2020-03-17  9:35         ` Akash Asthana
     [not found]           ` <aa197568-3bac-6962-d39d-3261f68c0514-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-17 13:06             ` Mark Brown
2020-03-20 13:52               ` Akash Asthana
2020-03-14  0:41     ` Matthias Kaehlcke
     [not found]       ` <20200314004106.GM144492-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-17 12:11         ` Akash Asthana
2020-03-13 13:12 ` [PATCH V2 2/8] soc: qcom: geni: Support for ICC voting Akash Asthana
2020-03-13 16:42   ` Matthias Kaehlcke
     [not found]     ` <20200313164207.GH144492-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-17  9:58       ` Akash Asthana
2020-03-17 19:06   ` Evan Green
     [not found]     ` <74851dda-296d-cdc5-2449-b9ec59bbc057@codeaurora.org>
2020-03-20 16:45       ` Evan Green
2020-03-27  5:33         ` Akash Asthana
2020-03-13 13:12 ` [PATCH V2 3/8] soc: qcom-geni-se: Add interconnect support to fix earlycon crash Akash Asthana
2020-03-13 20:44   ` Matthias Kaehlcke
     [not found]     ` <20200313204441.GJ144492-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-17 10:57       ` Akash Asthana
     [not found]         ` <1f86fdf0-df7c-4e4a-d4d8-8b0162e52cb4-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-17 18:29           ` Matthias Kaehlcke
2020-03-18  8:54             ` Akash Asthana
2020-03-19 19:43               ` Matthias Kaehlcke
     [not found]                 ` <20200319194332.GA60149-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-20 10:22                   ` Akash Asthana
2020-03-20 16:30                     ` Evan Green
2020-03-27  5:04                       ` Akash Asthana
2020-03-27 23:23                       ` Bjorn Andersson
2020-03-31 10:55                         ` Akash Asthana
2020-03-17 19:08           ` Evan Green
2020-03-17 19:46             ` Doug Anderson
     [not found]             ` <CAE=gft5GcOeQ5kh1bGen_P0J98g2XaAJ7NrDsxkirDoLtL4GWg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2020-03-18 10:57               ` Akash Asthana
2020-03-18 16:22                 ` Evan Green
2020-03-13 13:12 ` [PATCH V2 7/8] spi: spi-qcom-qspi: Add interconnect support Akash Asthana
2020-03-14  0:58   ` Matthias Kaehlcke
     [not found]     ` <20200314005817.GN144492-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-03-17 12:13       ` Akash Asthana
2020-03-17 19:08         ` Evan Green
2020-03-18 13:48           ` Akash Asthana
     [not found]             ` <e2ee1a60-a379-5c78-355a-64aad451a944-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2020-03-18 16:30               ` Evan Green
2020-03-20  5:35                 ` Akash Asthana
2020-03-13 13:12 ` [PATCH V2 8/8] arm64: dts: sc7180: Add interconnect for QUP and QSPI Akash Asthana

Reply instructions:

You may reply publicly 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=20200314001728.GL144492@google.com \
    --to=mka@chromium.org \
    --cc=agross@kernel.org \
    --cc=akashast@codeaurora.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=evgreen@chromium.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mgautam@codeaurora.org \
    --cc=robh+dt@kernel.org \
    --cc=swboyd@chromium.org \
    --cc=wsa@the-dreams.de \
    /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

Linux-SPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-spi/0 linux-spi/git/0.git

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

Example config snippet for mirrors

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


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