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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAA8EJpon5+R5s0HXUmoikjtuyEf3sQUqBVYvWrxuh14h2DvjQg@mail.gmail.com>
Date: Thu, 13 Feb 2025 16:28:32 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Taniya Das <quic_tdas@...cinc.com>
Cc: Bjorn Andersson <andersson@...nel.org>, Michael Turquette <mturquette@...libre.com>, 
	Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Konrad Dybcio <konradybcio@...nel.org>, Ajit Pandey <quic_ajipan@...cinc.com>, 
	Imran Shaik <quic_imrashai@...cinc.com>, Jagadeesh Kona <quic_jkona@...cinc.com>, 
	linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/4] clk: qcom: lpassaudiocc-sc7280: Add support for
 LPASS resets for QCM6490

On Thu, 13 Feb 2025 at 08:52, Taniya Das <quic_tdas@...cinc.com> wrote:
>
>
>
> On 2/13/2025 1:30 AM, Dmitry Baryshkov wrote:
> > On Wed, 12 Feb 2025 at 19:15, Taniya Das <quic_tdas@...cinc.com> 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@...cinc.com>
> >>>> ---
> >>>>  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.

You have been using of_device_is_compatible(). Krzysztof suggested
using mach data. Both approaches are fine with me (I'm sorry,
Krzysztof, this is a clock driver for a single platform, it doesn't
need to scale).

You've settled on the second one. So far so good.

But! The problem is in readability. Checking for desc->num_resets is a
_hidden_ or cryptic way of checking whether to register only a first
controller or both.

BTW: the commit message also tells nothing about the dropped power
domain and skipped PM code. Is it not required anymore? Is it handled
automatically by the firmware? But I see that audio codecs still use
that power domain.

>
> [1]:
> https://lore.kernel.org/all/20240318053555.20405-3-quic_tdas@quicinc.com/
>
> 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
> >>>>
> >>>
> >>
> >
> >
>


-- 
With best wishes
Dmitry