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

[Taniya Das <quic_tdas@quicinc.com>]

On 6/18/2024 6:52 PM, Konrad Dybcio wrote:
> 
> 
> 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).
> 

I understand the concern you have. I have introduced a separate regmap 
config for the LPASS resets which will have the required region size.

-- 
Thanks & Regards,
Taniya Das.