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

[Akash Asthana <akashast@codeaurora.org>]

Hi Evan,

On 3/18/2020 12:38 AM, Evan Green wrote:
> On Tue, Mar 17, 2020 at 5:13 AM Akash Asthana <akashast@codeaurora.org> wrote:
>> Hi Matthias,
>>
>> On 3/14/2020 6:28 AM, Matthias Kaehlcke wrote:
>>> Hi,
>>>
>>> On Fri, Mar 13, 2020 at 06:42:13PM +0530, Akash Asthana wrote:
>>>> Get the interconnect paths for QSPI device and vote according to the
>>>> current bus speed of the driver.
>>>>
>>>> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
>>>> ---
>>>>    - 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/spi/spi-qcom-qspi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-
>>>>    1 file changed, 45 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c
>>>> index 3c4f83b..ad48f43 100644
>>>> --- a/drivers/spi/spi-qcom-qspi.c
>>>> +++ b/drivers/spi/spi-qcom-qspi.c
>>>> @@ -2,6 +2,7 @@
>>>>    // Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
>>>>
>>>>    #include <linux/clk.h>
>>>> +#include <linux/interconnect.h>
>>>>    #include <linux/interrupt.h>
>>>>    #include <linux/io.h>
>>>>    #include <linux/module.h>
>>>> @@ -139,7 +140,10 @@ struct qcom_qspi {
>>>>       struct device *dev;
>>>>       struct clk_bulk_data *clks;
>>>>       struct qspi_xfer xfer;
>>>> -    /* Lock to protect xfer and IRQ accessed registers */
>>>> +    struct icc_path *icc_path_cpu_to_qspi;
>>>> +    unsigned int avg_bw_cpu;
>>>> +    unsigned int peak_bw_cpu;
>>> This triplet is a recurring pattern, and is probably not limited to geni SE/QSPI.
>>> On https://patchwork.kernel.org/patch/11436889/#23221925 I suggested the creation
>>> of a geni SE specific struct, however adding a generic convenience struct to
>>> 'linux/interconnect.h' might be the better solution:
>>>
>>> struct icc_client {
>>>        struct icc_path *path;
>>>        unsigned int avg_bw;
>>>        unsigned int peak_bw;
>>> };
>>>
>>> I'm sure there are better names for it, but this would be the idea.
>> Yeah, I think introducing this to ICC header would be better solution.
> +Georgi
>
> I'm not as convinced this structure is generally useful and belongs in
> the interconnect core. The thing that strikes me as weird with putting
> it in the core is now we're saving these values both inside and
> outside the interconnect core.
IIUC, you meant to say struct icc_req(inside icc_path) will be saving 
avg_bw and peak_bw so no need to save it outside icc_path?
>   In the GENI case here, we only really
> need them to undo the 0 votes we cast during suspend. If "vote for 0
> in suspend and whatever it was before at resume" is a recurring theme,
> maybe the core should give us path_disable() and path_enable() calls
> instead. I'm thinking out loud, maybe Georgi has some thoughts.
>
> Akash, for now if you want to avoid wading into a larger discussion
> maybe just refactor to a common structure local to GENI.

Ok

Thanks,

Akash

>
>
> -Evan

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