Re: [PATCH v3 3/4] soc: qcom: geni-se: Do not keep GPIOs to sleep state for shared SE usecase

Mukesh Kumar Savaliya
2024-11-13T16:07:40Z

Thanks Konrad for the review.

On 10/26/2024 12:23 AM, Konrad Dybcio wrote:
> On 27.09.2024 8:31 AM, Mukesh Kumar Savaliya wrote:
>> Currently the driver provides a function called geni_serial_resources_off()
>> to turn off resources like clocks and pinctrl.
>>
>> For shared SE between two SS, we don't need to keep pinctrl to sleep state
>> as other SS may be actively transferring data over SE. Hence,bypass keeping
>> pinctrl to sleep state conditionally using shared_geni_se flag.
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>> drivers/soc/qcom/qcom-geni-se.c | 14 ++++++++++----
>> include/linux/soc/qcom/geni-se.h | 3 +++
>> 2 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>> index 2e8f24d5da80..89cf18699336 100644
>> --- a/drivers/soc/qcom/qcom-geni-se.c
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -1,5 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0
>> // Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> +// Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>
>> /* Disable MMIO tracing to prevent excessive logging of unwanted MMIO traces */
>> #define __DISABLE_TRACE_MMIO__
>> @@ -503,10 +504,15 @@ int geni_se_resources_off(struct geni_se *se)
>>
>> if (has_acpi_companion(se->dev))
>> return 0;
>> -
>> - ret = pinctrl_pm_select_sleep_state(se->dev);
>> - if (ret)
>> - return ret;
>> + /* Keep pinctrl to sleep state only for regular usecase.
>> + * Do not sleep pinctrl for shared SE because other SS(subsystems)
>> + * may continueto perform transfer.
>> + */
>
> /*
> * Don't alter pin states on shared SEs to avoid potentially
> * interrupting transfers started by other subsystems
> */
>
Done
>
>> + if (se->shared_geni_se == false) {
>
> if (!se->shared_geni_se)
Done
>
>> + ret = pinctrl_pm_select_sleep_state(se->dev);
>> + if (ret)
>> + return ret;
>> + }
>>
>> geni_se_clks_off(se);
>
> Should the clocks be turned off?
>
Yes, it's required to be turned off.
> Konrad