Re: [PATCH V3 7/8] spi: spi-qcom-qspi: Add interconnect support

From: Akash Asthana <akashast@codeaurora.org>
To: Mark Brown <broonie@kernel.org>
Cc: gregkh@linuxfoundation.org, agross@kernel.org,
	bjorn.andersson@linaro.org, wsa@the-dreams.de,
	mark.rutland@arm.com, robh+dt@kernel.org,
	georgi.djakov@linaro.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,
	mka@chromium.org, dianders@chromium.org, evgreen@chromium.org
Subject: Re: [PATCH V3 7/8] spi: spi-qcom-qspi: Add interconnect support
Date: Wed, 8 Apr 2020 17:47:25 +0530	[thread overview]
Message-ID: <48c60fdf-03c6-650a-2671-b8f7cc1e5c82@codeaurora.org> (raw)
In-Reply-To: <20200407105542.GA5247@sirena.org.uk>

Hi Mark, Evan, Georgi,

On 4/7/2020 4:25 PM, Mark Brown wrote:
> On Tue, Apr 07, 2020 at 03:24:42PM +0530, Akash Asthana wrote:
>> On 3/31/2020 4:53 PM, Mark Brown wrote:
>>>> +	ctrl->avg_bw_cpu = Bps_to_icc(speed_hz);
>>>> +	ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz);
>>> I thought you were going to factor this best guess handling of peak
>>> bandwidth out into the core?
>> I can centralize this for SPI, I2C and UART  in Common driver(QUP wrapper)
>> but still for QSPI I have to keep this piece of code as is because It is not
>> child of QUP wrapper(it doesn't use common code).
> Why not?
>
>> I am not sure whether I can move this " Assume peak_bw as twice of avg_bw if
>> nothing is mentioned explicitly" to ICC core because the factor of 2 is
>> chosen randomly by me.
> That's the whole point - if this is just a random number then we may as
> well at least be consistently random.

Can we centralize below logic of peak_bw selection for all the clients 
to ICC core?

"Assume peak_bw requirement as twice of avg_bw, if it is not mentioned 
explicitly"

===========================================================================
int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
{
         struct icc_node *node;
         u32 old_avg, old_peak;
         size_t i;
         int ret;

         if (!path)
                 return 0;

         if (WARN_ON(IS_ERR(path) || !path->num_nodes))
                 return -EINVAL;

+       /*
+        * Assume peak_bw requirement as twice of avg_bw, if it is not
+        * mentioned explicitly
+        */
+       peak_bw = peak_bw ? peak_bw : 2 * avg_bw;
===========================================================================

In case if some client really don't want to put peak requirement they 
can pass avg_bw = peak_bw. As peak_bw <= avg_bw is kind of no-ops.

Regards,

Akash

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project