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

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 6/10/24 12:19, Taniya Das wrote:
> 
> 
> On 6/7/2024 3:00 PM, Konrad Dybcio wrote:
>> On 31.05.2024 12:22 PM, 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 same.
>>>
>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>> ---
>>
>> Please stop ignoring my comments without responding.
>>
>> https://lore.kernel.org/all/c1d07eff-4832-47d9-8598-aa6709b465ff@linaro.org/
>>
> 
> Sorry about that, it was not intentional. I had posted the v2 and decided to split as it was delaying the other changes in the older series which had more functional fixes.
> 
> 
> Picking your comments from the old series.
> 
> ---------------------------------
>  > -    clk_zonda_pll_configure(&lpass_audio_cc_pll, regmap, &lpass_audio_cc_pll_config);
>  > +    if (!of_property_read_bool(pdev->dev.of_node, "qcom,adsp-skip-pll")) {
> 
> Big no-no.
> --------------------------------
> 
> Yes, I have already moved away from it and introduced a new probe to support the subset of functionality on QCM6490.
> 
> 
> ------------------------
>  > +        /* PLL settings */
>  > +        regmap_write(regmap, 0x4, 0x3b);
>  > +        regmap_write(regmap, 0x8, 0xff05);
> 
> Model these properly and use the abstracted clock (re)configuration functions.
> Add the unreachable clocks to `protected-clocks = <>` and make sure that the
> aforementioned configure calls check if the PLL was really registered.
> ---------------------------
> 
> These were made for alignment of code, but existing approach was not touched.

That's not purely cosmetic, this now falls into the compatible-specific
if-condition, which was my issue.

> 
> ---------------------
> 
>  > +    lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc_reset";
> 
> Ugh.. are these really not contiguous, or were the register ranges misrepresented from
> the start?
> 
>  > +    lpass_audio_cc_sc7280_regmap_config.max_register = 0xc8;
> 
> Provide the real size of the block in .max_register instead, unconditionally
> -----------------
> 
> This had a little history behind this approach. During the driver development the ask was to avoid duplicating same descriptors and update runtime what is possible. That is the reason to update it runtime. The max register size is 0xC8 for resets functionality usage for High level OS.

What I mean is that, the register region size is constant for a given piece of
hardware. Whether Linux can safely access it or not, doesn't matter. The
regmap_size value can just reflect the width of the region (and so should the
device tree).

Konrad