Re: [PATCH v3 2/4] clk: qcom: lpassaudiocc-sc7280: Add support for LPASS resets for QCM6490

[Taniya Das]

On 2/13/2025 1:30 AM, Dmitry Baryshkov wrote:
> On Wed, 12 Feb 2025 at 19:15, Taniya Das <quic_tdas@xxxxxxxxxxx> wrote:
>>
>>
>>
>> On 2/12/2025 4:39 PM, Dmitry Baryshkov wrote:
>>> On Wed, Feb 12, 2025 at 01:52:20PM +0530, Taniya Das wrote:
>>>> On the QCM6490 boards the LPASS firmware controls the complete clock
>>>> controller functionalities. But the LPASS resets are required to be
>>>> controlled from the high level OS. The Audio SW driver should be able to
>>>> assert/deassert the audio resets as required. Thus in clock driver add
>>>> support for the resets.
>>>>
>>>> Signed-off-by: Taniya Das <quic_tdas@xxxxxxxxxxx>
>>>> ---
>>>>  drivers/clk/qcom/lpassaudiocc-sc7280.c | 23 +++++++++++++++++++----
>>>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c
>>>> index 45e7264770866f929a3f4663c477330f0bf7aa84..b6439308926371891cc5f9a5e0d4e8393641560d 100644
>>>> --- a/drivers/clk/qcom/lpassaudiocc-sc7280.c
>>>> +++ b/drivers/clk/qcom/lpassaudiocc-sc7280.c
>>>> @@ -1,6 +1,7 @@
>>>>  // SPDX-License-Identifier: GPL-2.0-only
>>>>  /*
>>>>   * Copyright (c) 2021, The Linux Foundation. All rights reserved.
>>>> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
>>>>   */
>>>>
>>>>  #include <linux/clk-provider.h>
>>>> @@ -713,14 +714,24 @@ static const struct qcom_reset_map lpass_audio_cc_sc7280_resets[] = {
>>>>      [LPASS_AUDIO_SWR_WSA_CGCR] = { 0xb0, 1 },
>>>>  };
>>>>
>>>> +static const struct regmap_config lpass_audio_cc_sc7280_reset_regmap_config = {
>>>> +    .name = "lpassaudio_cc_reset",
>>>> +    .reg_bits = 32,
>>>> +    .reg_stride = 4,
>>>> +    .val_bits = 32,
>>>> +    .fast_io = true,
>>>> +    .max_register = 0xc8,
>>>> +};
>>>> +
>>>>  static const struct qcom_cc_desc lpass_audio_cc_reset_sc7280_desc = {
>>>> -    .config = &lpass_audio_cc_sc7280_regmap_config,
>>>> +    .config = &lpass_audio_cc_sc7280_reset_regmap_config,
>>>>      .resets = lpass_audio_cc_sc7280_resets,
>>>>      .num_resets = ARRAY_SIZE(lpass_audio_cc_sc7280_resets),
>>>>  };
>>>>
>>>>  static const struct of_device_id lpass_audio_cc_sc7280_match_table[] = {
>>>> -    { .compatible = "qcom,sc7280-lpassaudiocc" },
>>>> +    { .compatible = "qcom,qcm6490-lpassaudiocc", .data = &lpass_audio_cc_reset_sc7280_desc },
>>>> +    { .compatible = "qcom,sc7280-lpassaudiocc", .data = &lpass_audio_cc_sc7280_desc },
>>>>      { }
>>>>  };
>>>>  MODULE_DEVICE_TABLE(of, lpass_audio_cc_sc7280_match_table);
>>>> @@ -752,13 +763,17 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev)
>>>>      struct regmap *regmap;
>>>>      int ret;
>>>>
>>>> +    desc = device_get_match_data(&pdev->dev);
>>>> +
>>>> +    if (desc->num_resets)
>>>> +            return qcom_cc_probe_by_index(pdev, 1, desc);
>>>
>>> Won't this break SC7280 support by causing an early return?
>>>
>>
>> The resets are not defined for SC7280.
>> static const struct qcom_cc_desc lpass_audio_cc_sc7280_desc = {
>>         .config = &lpass_audio_cc_sc7280_regmap_config,
>>         .clks = lpass_audio_cc_sc7280_clocks,
>>         .num_clks = ARRAY_SIZE(lpass_audio_cc_sc7280_clocks),
>> };
>>
>> The reset get registered for SC7280 after the clocks are registered.
>> qcom_cc_probe_by_index(pdev, 1,  &lpass_audio_cc_reset_sc7280_desc);
> 
> Could you please make this condition more obvious and error-prone
> rather than checking one particular non-obvious property?
> 

Dmitry, we had earlier tried [1], but seems like we could not align on
this patchset.

If you are aligned, please let me know I can fall back on the approach.

[1]:
https://lore.kernel.org/all/20240318053555.20405-3-quic_tdas@xxxxxxxxxxx/

Do you have any suggestions that we could consider?

>>
>>>> +
>>>>      ret = lpass_audio_setup_runtime_pm(pdev);
>>>>      if (ret)
>>>>              return ret;
>>>>
>>>>      lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc";
>>>>      lpass_audio_cc_sc7280_regmap_config.max_register = 0x2f000;
>>>> -    desc = &lpass_audio_cc_sc7280_desc;
>>>>
>>>>      regmap = qcom_cc_map(pdev, desc);
>>>>      if (IS_ERR(regmap)) {
>>>> @@ -772,7 +787,7 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev)
>>>>      regmap_write(regmap, 0x4, 0x3b);
>>>>      regmap_write(regmap, 0x8, 0xff05);
>>>>
>>>> -    ret = qcom_cc_really_probe(&pdev->dev, &lpass_audio_cc_sc7280_desc, regmap);
>>>> +    ret = qcom_cc_really_probe(&pdev->dev, desc, regmap);
>>>>      if (ret) {
>>>>              dev_err(&pdev->dev, "Failed to register LPASS AUDIO CC clocks\n");
>>>>              goto exit;
>>>>
>>>> --
>>>> 2.45.2
>>>>
>>>
>>
> 
>