[PATCH 0/4] Update LPASS Audio clock driver for QCM6490 board

[PATCH 0/4] Update LPASS Audio clock driver for QCM6490 board

[Taniya Das]

This series updates the low pass audio clock controller driver for reset
functionality. The patches are split from the below series.
https://lore.kernel.org/all/20240318053555.20405-1-quic_tdas@quicinc.com/

The QCM6490 board requires only the reset functionality from the LPASS
subsystem. Thus separate out the driver probe to provide the same on the
QCM6490 boards.

Taniya Das (4):
  dt-bindings: clock: qcom: Add compatible for QCM6490 boards
  clk: qcom: lpassaudiocc-sc7280: Add support for LPASS resets for
    QCM6490
  arm64: dts: qcom: qcm6490-idp: Update protected clocks list
  arm64: dts: qcom: qcs6490-rb3gen2: Update the LPASS audio node

 .../clock/qcom,sc7280-lpasscorecc.yaml        |  1 +
 arch/arm64/boot/dts/qcom/qcm6490-idp.dts      | 28 ++++++++++++++++++-
 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts  |  7 ++++-
 drivers/clk/qcom/lpassaudiocc-sc7280.c        | 28 +++++++++++++++++++
 4 files changed, 62 insertions(+), 2 deletions(-)

--
2.17.1

[PATCH 1/4] dt-bindings: clock: qcom: Add compatible for QCM6490 boards

[Taniya Das]

Add the new QCM6490 compatible to support the reset functionality for
Low Power Audio subsystem.

Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
---
 .../devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml       | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml
index deee5423d66e..5d6f74cf9c43 100644
--- a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscorecc.yaml
@@ -26,6 +26,7 @@ properties:
 
   compatible:
     enum:
+      - qcom,qcm6490-lpassaudiocc
       - qcom,sc7280-lpassaoncc
       - qcom,sc7280-lpassaudiocc
       - qcom,sc7280-lpasscorecc
-- 
2.17.1

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

[Taniya Das]

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>
---
 drivers/clk/qcom/lpassaudiocc-sc7280.c | 28 ++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c
index c43d0b1af7f7..7fdfd07c111c 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>
@@ -869,10 +870,36 @@ static struct platform_driver lpass_aon_cc_sc7280_driver = {
 	},
 };
 
+static const struct of_device_id lpass_audio_cc_qcm6490_match_table[] = {
+	{ .compatible = "qcom,qcm6490-lpassaudiocc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, lpass_audio_cc_qcm6490_match_table);
+
+static int lpass_audio_cc_qcm6490_probe(struct platform_device *pdev)
+{
+	lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc_reset";
+	lpass_audio_cc_sc7280_regmap_config.max_register = 0xc8;
+
+	return qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc);
+}
+
+static struct platform_driver lpass_audio_cc_qcm6490_driver = {
+	.probe = lpass_audio_cc_qcm6490_probe,
+	.driver = {
+		.name = "lpass_audio_cc-qcm6490",
+		.of_match_table = lpass_audio_cc_qcm6490_match_table,
+	},
+};
+
 static int __init lpass_audio_cc_sc7280_init(void)
 {
 	int ret;
 
+	ret = platform_driver_register(&lpass_audio_cc_qcm6490_driver);
+	if (ret)
+		return ret;
+
 	ret = platform_driver_register(&lpass_aon_cc_sc7280_driver);
 	if (ret)
 		return ret;
@@ -885,6 +912,7 @@ static void __exit lpass_audio_cc_sc7280_exit(void)
 {
 	platform_driver_unregister(&lpass_audio_cc_sc7280_driver);
 	platform_driver_unregister(&lpass_aon_cc_sc7280_driver);
+	platform_driver_unregister(&lpass_audio_cc_qcm6490_driver);
 }
 module_exit(lpass_audio_cc_sc7280_exit);
 
-- 
2.17.1

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

[Krzysztof Kozlowski]

On 31/05/2024 12:22, 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>
> ---
>  drivers/clk/qcom/lpassaudiocc-sc7280.c | 28 ++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c
> index c43d0b1af7f7..7fdfd07c111c 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>
> @@ -869,10 +870,36 @@ static struct platform_driver lpass_aon_cc_sc7280_driver = {
>  	},
>  };
>  
> +static const struct of_device_id lpass_audio_cc_qcm6490_match_table[] = {
> +	{ .compatible = "qcom,qcm6490-lpassaudiocc" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, lpass_audio_cc_qcm6490_match_table);
> +
> +static int lpass_audio_cc_qcm6490_probe(struct platform_device *pdev)
> +{
> +	lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc_reset";
> +	lpass_audio_cc_sc7280_regmap_config.max_register = 0xc8;
> +
> +	return qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc);
> +}
> +
> +static struct platform_driver lpass_audio_cc_qcm6490_driver = {
> +	.probe = lpass_audio_cc_qcm6490_probe,
> +	.driver = {
> +		.name = "lpass_audio_cc-qcm6490",
> +		.of_match_table = lpass_audio_cc_qcm6490_match_table,
> +	},
> +};
> +
>  static int __init lpass_audio_cc_sc7280_init(void)
>  {
>  	int ret;
>  
> +	ret = platform_driver_register(&lpass_audio_cc_qcm6490_driver);
> +	if (ret)
> +		return ret;
> +
>  	ret = platform_driver_register(&lpass_aon_cc_sc7280_driver);
Why this is a new platform driver?  There should be just one driver with
different match data.

Best regards,
Krzysztof

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

[Taniya Das]

On 5/31/2024 9:56 PM, Krzysztof Kozlowski wrote:
> On 31/05/2024 12:22, 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>
>> ---
>>   drivers/clk/qcom/lpassaudiocc-sc7280.c | 28 ++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c
>> index c43d0b1af7f7..7fdfd07c111c 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>
>> @@ -869,10 +870,36 @@ static struct platform_driver lpass_aon_cc_sc7280_driver = {
>>   	},
>>   };
>>   
>> +static const struct of_device_id lpass_audio_cc_qcm6490_match_table[] = {
>> +	{ .compatible = "qcom,qcm6490-lpassaudiocc" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, lpass_audio_cc_qcm6490_match_table);
>> +
>> +static int lpass_audio_cc_qcm6490_probe(struct platform_device *pdev)
>> +{
>> +	lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc_reset";
>> +	lpass_audio_cc_sc7280_regmap_config.max_register = 0xc8;
>> +
>> +	return qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc);
>> +}
>> +
>> +static struct platform_driver lpass_audio_cc_qcm6490_driver = {
>> +	.probe = lpass_audio_cc_qcm6490_probe,
>> +	.driver = {
>> +		.name = "lpass_audio_cc-qcm6490",
>> +		.of_match_table = lpass_audio_cc_qcm6490_match_table,
>> +	},
>> +};
>> +
>>   static int __init lpass_audio_cc_sc7280_init(void)
>>   {
>>   	int ret;
>>   
>> +	ret = platform_driver_register(&lpass_audio_cc_qcm6490_driver);
>> +	if (ret)
>> +		return ret;
>> +
>>   	ret = platform_driver_register(&lpass_aon_cc_sc7280_driver);
> Why this is a new platform driver?  There should be just one driver with
> different match data.
> 

The main problem for me is that the current board(QCM6490) needs to be 
only support a subset of the entire(only resets) functionality the 
SC7280. If I redesign the probe function to pick the match data then I 
might accidentally break the existing functionalities on SC7280 boards.

Hence I thought to have a separate driver registration which looked a 
cleaner approach to go away from the "of_device_is_compatible".

> Best regards,
> Krzysztof
> 

-- 
Thanks & Regards,
Taniya Das.

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

[Krzysztof Kozlowski]

On 10/06/2024 12:03, Taniya Das wrote:
> 
> 
> On 5/31/2024 9:56 PM, Krzysztof Kozlowski wrote:
>> On 31/05/2024 12:22, 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>
>>> ---
>>>   drivers/clk/qcom/lpassaudiocc-sc7280.c | 28 ++++++++++++++++++++++++++
>>>   1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c
>>> index c43d0b1af7f7..7fdfd07c111c 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>
>>> @@ -869,10 +870,36 @@ static struct platform_driver lpass_aon_cc_sc7280_driver = {
>>>   	},
>>>   };
>>>   
>>> +static const struct of_device_id lpass_audio_cc_qcm6490_match_table[] = {
>>> +	{ .compatible = "qcom,qcm6490-lpassaudiocc" },
>>> +	{ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, lpass_audio_cc_qcm6490_match_table);
>>> +
>>> +static int lpass_audio_cc_qcm6490_probe(struct platform_device *pdev)
>>> +{
>>> +	lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc_reset";
>>> +	lpass_audio_cc_sc7280_regmap_config.max_register = 0xc8;
>>> +
>>> +	return qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc);
>>> +}
>>> +
>>> +static struct platform_driver lpass_audio_cc_qcm6490_driver = {
>>> +	.probe = lpass_audio_cc_qcm6490_probe,
>>> +	.driver = {
>>> +		.name = "lpass_audio_cc-qcm6490",
>>> +		.of_match_table = lpass_audio_cc_qcm6490_match_table,
>>> +	},
>>> +};
>>> +
>>>   static int __init lpass_audio_cc_sc7280_init(void)
>>>   {
>>>   	int ret;
>>>   
>>> +	ret = platform_driver_register(&lpass_audio_cc_qcm6490_driver);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>   	ret = platform_driver_register(&lpass_aon_cc_sc7280_driver);
>> Why this is a new platform driver?  There should be just one driver with
>> different match data.
>>
> 
> The main problem for me is that the current board(QCM6490) needs to be 
> only support a subset of the entire(only resets) functionality the 
> SC7280. If I redesign the probe function to pick the match data then I 
> might accidentally break the existing functionalities on SC7280 boards.

That's not a reason to not implement changes. Test your changes first.

> 
> Hence I thought to have a separate driver registration which looked a 
> cleaner approach to go away from the "of_device_is_compatible".

No. You over complicate simple case introducing unusual pattern.

Best regards,
Krzysztof

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

[Taniya Das]

On 6/16/2024 1:19 PM, Krzysztof Kozlowski wrote:
> On 10/06/2024 12:03, Taniya Das wrote:
>>
>>
>> On 5/31/2024 9:56 PM, Krzysztof Kozlowski wrote:
>>> On 31/05/2024 12:22, 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>
>>>> ---
>>>>    drivers/clk/qcom/lpassaudiocc-sc7280.c | 28 ++++++++++++++++++++++++++
>>>>    1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c
>>>> index c43d0b1af7f7..7fdfd07c111c 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>
>>>> @@ -869,10 +870,36 @@ static struct platform_driver lpass_aon_cc_sc7280_driver = {
>>>>    	},
>>>>    };
>>>>    
>>>> +static const struct of_device_id lpass_audio_cc_qcm6490_match_table[] = {
>>>> +	{ .compatible = "qcom,qcm6490-lpassaudiocc" },
>>>> +	{ }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, lpass_audio_cc_qcm6490_match_table);
>>>> +
>>>> +static int lpass_audio_cc_qcm6490_probe(struct platform_device *pdev)
>>>> +{
>>>> +	lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc_reset";
>>>> +	lpass_audio_cc_sc7280_regmap_config.max_register = 0xc8;
>>>> +
>>>> +	return qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc);
>>>> +}
>>>> +
>>>> +static struct platform_driver lpass_audio_cc_qcm6490_driver = {
>>>> +	.probe = lpass_audio_cc_qcm6490_probe,
>>>> +	.driver = {
>>>> +		.name = "lpass_audio_cc-qcm6490",
>>>> +		.of_match_table = lpass_audio_cc_qcm6490_match_table,
>>>> +	},
>>>> +};
>>>> +
>>>>    static int __init lpass_audio_cc_sc7280_init(void)
>>>>    {
>>>>    	int ret;
>>>>    
>>>> +	ret = platform_driver_register(&lpass_audio_cc_qcm6490_driver);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>>    	ret = platform_driver_register(&lpass_aon_cc_sc7280_driver);
>>> Why this is a new platform driver?  There should be just one driver with
>>> different match data.
>>>
>>
>> The main problem for me is that the current board(QCM6490) needs to be
>> only support a subset of the entire(only resets) functionality the
>> SC7280. If I redesign the probe function to pick the match data then I
>> might accidentally break the existing functionalities on SC7280 boards.
> 
> That's not a reason to not implement changes. Test your changes first.
> 
>>
>> Hence I thought to have a separate driver registration which looked a
>> cleaner approach to go away from the "of_device_is_compatible".
> 
> No. You over complicate simple case introducing unusual pattern.
> 

Krzysztof, now I am introducing a match data approach to differentiate 
between SC7280 and QCM6490 for adding the reset functionality only to 
the later board.

v2 series: 
https://lore.kernel.org/lkml/20240816-qcm6490-lpass-reset-v1-0-a11f33cad3c5@quicinc.com/T/#t 


> Best regards,
> Krzysztof
> 

-- 
Thanks & Regards,
Taniya Das.

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

[Konrad Dybcio]

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/

Konrad

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

[Krzysztof Kozlowski]

On 07/06/2024 11:30, 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/

So this was already sent, feedback ignored and now we have again "v1"
skipping previous talks?

Best regards,
Krzysztof

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

[Dmitry Baryshkov]

On Fri, Jun 07, 2024 at 11:34:03AM +0200, Krzysztof Kozlowski wrote:
> On 07/06/2024 11:30, 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/
> 
> So this was already sent, feedback ignored and now we have again "v1"
> skipping previous talks?

And no changelog from the previous patchset. That's really sad.


-- 
With best wishes
Dmitry

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

[Taniya Das]

On 6/7/2024 3:37 PM, Dmitry Baryshkov wrote:
> On Fri, Jun 07, 2024 at 11:34:03AM +0200, Krzysztof Kozlowski wrote:
>> On 07/06/2024 11:30, 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/
>>
>> So this was already sent, feedback ignored and now we have again "v1"
>> skipping previous talks?
> 
> And no changelog from the previous patchset. That's really sad.
> 
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.
> 

-- 
Thanks & Regards,
Taniya Das.

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

[Taniya Das]

On 6/7/2024 3:04 PM, Krzysztof Kozlowski wrote:
> On 07/06/2024 11:30, 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/
> 
> So this was already sent, feedback ignored and now we have again "v1"
> skipping previous talks?
> 

Looks like the comments in v1 came in after v2 was posted already. There 
was no intention to not respond on the comments.

> Best regards,
> Krzysztof
> 

-- 
Thanks & Regards,
Taniya Das.

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

[Taniya Das]

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.

---------------------

 > +	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.

Hope I was able to clarify your queries.


> Konrad
> 

-- 
Thanks & Regards,
Taniya Das.

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

[Dmitry Baryshkov]

On Mon, Jun 10, 2024 at 03:49:18PM +0530, 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.

I think it would have been better to respond to the original email
rather than c&psting the question. It drops the context of the
discussion.

> 
> ------------------------
> > +		/* 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.

So, first patch to fix the old code, second patch to shuffle it around,
please.

> 
> ---------------------
> 
> > +	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.
> 
> Hope I was able to clarify your queries.


-- 
With best wishes
Dmitry

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

[Konrad Dybcio]

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

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

[Taniya Das]

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.

[PATCH 3/4] arm64: dts: qcom: qcm6490-idp: Update protected clocks list

[Taniya Das]

Certain clocks are not accessible on QCM6490-IDP board,
thus mark them as protected. Update the lpassaudio node to
support the new compatible as the lpassaudio needs to support
the reset functionality on the QCM6490 board and the rest of
the Audio functionality would be provided from the LPASS
firmware.

Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
---
 arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 28 +++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
index a0668f767e4b..4eece564331a 100644
--- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
+++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: BSD-3-Clause
 /*
- * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 /dts-v1/;
@@ -688,3 +688,29 @@
 &wifi {
 	memory-region = <&wlan_fw_mem>;
 };
+
+&gcc {
+	protected-clocks = <GCC_AGGRE_NOC_PCIE_1_AXI_CLK> ,<GCC_PCIE_1_AUX_CLK>,
+			<GCC_PCIE_1_AUX_CLK_SRC>, <GCC_PCIE_1_CFG_AHB_CLK>,
+			<GCC_PCIE_1_MSTR_AXI_CLK>, <GCC_PCIE_1_PHY_RCHNG_CLK_SRC>,
+			<GCC_PCIE_1_PIPE_CLK>, <GCC_PCIE_1_PIPE_CLK_SRC>,
+			<GCC_PCIE_1_SLV_AXI_CLK>, <GCC_PCIE_1_SLV_Q2A_AXI_CLK>,
+			<GCC_QSPI_CNOC_PERIPH_AHB_CLK>, <GCC_QSPI_CORE_CLK>,
+			<GCC_QSPI_CORE_CLK_SRC>,<GCC_USB30_SEC_MASTER_CLK>,
+			<GCC_USB30_SEC_MASTER_CLK_SRC>, <GCC_USB30_SEC_MOCK_UTMI_CLK>,
+			<GCC_USB30_SEC_MOCK_UTMI_CLK_SRC>,
+			<GCC_USB30_SEC_MOCK_UTMI_POSTDIV_CLK_SRC>, <GCC_USB30_SEC_SLEEP_CLK>,
+			<GCC_USB3_SEC_PHY_AUX_CLK>, <GCC_USB3_SEC_PHY_AUX_CLK_SRC>,
+			<GCC_USB3_SEC_PHY_COM_AUX_CLK>, <GCC_USB3_SEC_PHY_PIPE_CLK>,
+			<GCC_USB3_SEC_PHY_PIPE_CLK_SRC>, <GCC_CFG_NOC_LPASS_CLK>,
+			<GCC_MSS_GPLL0_MAIN_DIV_CLK_SRC>, <GCC_MSS_CFG_AHB_CLK>,
+			<GCC_MSS_OFFLINE_AXI_CLK>, <GCC_MSS_SNOC_AXI_CLK>,
+			<GCC_MSS_Q6_MEMNOC_AXI_CLK>, <GCC_MSS_Q6SS_BOOT_CLK_SRC>,
+			<GCC_SEC_CTRL_CLK_SRC>, <GCC_WPSS_AHB_CLK>,
+			<GCC_WPSS_AHB_BDG_MST_CLK>, <GCC_WPSS_RSCP_CLK>;
+};
+
+&lpass_audiocc {
+	compatible = "qcom,qcm6490-lpassaudiocc";
+	/delete-property/ power-domains;
+};
-- 
2.17.1

Re: [PATCH 3/4] arm64: dts: qcom: qcm6490-idp: Update protected clocks list

[Dmitry Baryshkov]

On Fri, May 31, 2024 at 03:52:51PM +0530, Taniya Das wrote:
> Certain clocks are not accessible on QCM6490-IDP board,
> thus mark them as protected. Update the lpassaudio node to
> support the new compatible as the lpassaudio needs to support
> the reset functionality on the QCM6490 board and the rest of
> the Audio functionality would be provided from the LPASS
> firmware.
> 
> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 28 +++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> index a0668f767e4b..4eece564331a 100644
> --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: BSD-3-Clause
>  /*
> - * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
>  /dts-v1/;
> @@ -688,3 +688,29 @@
>  &wifi {
>  	memory-region = <&wlan_fw_mem>;
>  };
> +
> +&gcc {
> +	protected-clocks = <GCC_AGGRE_NOC_PCIE_1_AXI_CLK> ,<GCC_PCIE_1_AUX_CLK>,
> +			<GCC_PCIE_1_AUX_CLK_SRC>, <GCC_PCIE_1_CFG_AHB_CLK>,
> +			<GCC_PCIE_1_MSTR_AXI_CLK>, <GCC_PCIE_1_PHY_RCHNG_CLK_SRC>,
> +			<GCC_PCIE_1_PIPE_CLK>, <GCC_PCIE_1_PIPE_CLK_SRC>,
> +			<GCC_PCIE_1_SLV_AXI_CLK>, <GCC_PCIE_1_SLV_Q2A_AXI_CLK>,
> +			<GCC_QSPI_CNOC_PERIPH_AHB_CLK>, <GCC_QSPI_CORE_CLK>,
> +			<GCC_QSPI_CORE_CLK_SRC>,<GCC_USB30_SEC_MASTER_CLK>,
> +			<GCC_USB30_SEC_MASTER_CLK_SRC>, <GCC_USB30_SEC_MOCK_UTMI_CLK>,
> +			<GCC_USB30_SEC_MOCK_UTMI_CLK_SRC>,
> +			<GCC_USB30_SEC_MOCK_UTMI_POSTDIV_CLK_SRC>, <GCC_USB30_SEC_SLEEP_CLK>,
> +			<GCC_USB3_SEC_PHY_AUX_CLK>, <GCC_USB3_SEC_PHY_AUX_CLK_SRC>,
> +			<GCC_USB3_SEC_PHY_COM_AUX_CLK>, <GCC_USB3_SEC_PHY_PIPE_CLK>,
> +			<GCC_USB3_SEC_PHY_PIPE_CLK_SRC>, <GCC_CFG_NOC_LPASS_CLK>,
> +			<GCC_MSS_GPLL0_MAIN_DIV_CLK_SRC>, <GCC_MSS_CFG_AHB_CLK>,
> +			<GCC_MSS_OFFLINE_AXI_CLK>, <GCC_MSS_SNOC_AXI_CLK>,
> +			<GCC_MSS_Q6_MEMNOC_AXI_CLK>, <GCC_MSS_Q6SS_BOOT_CLK_SRC>,
> +			<GCC_SEC_CTRL_CLK_SRC>, <GCC_WPSS_AHB_CLK>,
> +			<GCC_WPSS_AHB_BDG_MST_CLK>, <GCC_WPSS_RSCP_CLK>;

Is there any reason why this list is significantly larger than a list
for RB3g2 or FP5?

> +};
> +
> +&lpass_audiocc {
> +	compatible = "qcom,qcm6490-lpassaudiocc";
> +	/delete-property/ power-domains;
> +};

Separate commits, please.

> -- 
> 2.17.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] arm64: dts: qcom: qcm6490-idp: Update protected clocks list

[Taniya Das]

On 5/31/2024 5:34 PM, Dmitry Baryshkov wrote:
> On Fri, May 31, 2024 at 03:52:51PM +0530, Taniya Das wrote:
>> Certain clocks are not accessible on QCM6490-IDP board,
>> thus mark them as protected. Update the lpassaudio node to
>> support the new compatible as the lpassaudio needs to support
>> the reset functionality on the QCM6490 board and the rest of
>> the Audio functionality would be provided from the LPASS
>> firmware.
>>
>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 28 +++++++++++++++++++++++-
>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
>> index a0668f767e4b..4eece564331a 100644
>> --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
>> @@ -1,6 +1,6 @@
>>   // SPDX-License-Identifier: BSD-3-Clause
>>   /*
>> - * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>> + * Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>    */
>>   
>>   /dts-v1/;
>> @@ -688,3 +688,29 @@
>>   &wifi {
>>   	memory-region = <&wlan_fw_mem>;
>>   };
>> +
>> +&gcc {
>> +	protected-clocks = <GCC_AGGRE_NOC_PCIE_1_AXI_CLK> ,<GCC_PCIE_1_AUX_CLK>,
>> +			<GCC_PCIE_1_AUX_CLK_SRC>, <GCC_PCIE_1_CFG_AHB_CLK>,
>> +			<GCC_PCIE_1_MSTR_AXI_CLK>, <GCC_PCIE_1_PHY_RCHNG_CLK_SRC>,
>> +			<GCC_PCIE_1_PIPE_CLK>, <GCC_PCIE_1_PIPE_CLK_SRC>,
>> +			<GCC_PCIE_1_SLV_AXI_CLK>, <GCC_PCIE_1_SLV_Q2A_AXI_CLK>,
>> +			<GCC_QSPI_CNOC_PERIPH_AHB_CLK>, <GCC_QSPI_CORE_CLK>,
>> +			<GCC_QSPI_CORE_CLK_SRC>,<GCC_USB30_SEC_MASTER_CLK>,
>> +			<GCC_USB30_SEC_MASTER_CLK_SRC>, <GCC_USB30_SEC_MOCK_UTMI_CLK>,
>> +			<GCC_USB30_SEC_MOCK_UTMI_CLK_SRC>,
>> +			<GCC_USB30_SEC_MOCK_UTMI_POSTDIV_CLK_SRC>, <GCC_USB30_SEC_SLEEP_CLK>,
>> +			<GCC_USB3_SEC_PHY_AUX_CLK>, <GCC_USB3_SEC_PHY_AUX_CLK_SRC>,
>> +			<GCC_USB3_SEC_PHY_COM_AUX_CLK>, <GCC_USB3_SEC_PHY_PIPE_CLK>,
>> +			<GCC_USB3_SEC_PHY_PIPE_CLK_SRC>, <GCC_CFG_NOC_LPASS_CLK>,
>> +			<GCC_MSS_GPLL0_MAIN_DIV_CLK_SRC>, <GCC_MSS_CFG_AHB_CLK>,
>> +			<GCC_MSS_OFFLINE_AXI_CLK>, <GCC_MSS_SNOC_AXI_CLK>,
>> +			<GCC_MSS_Q6_MEMNOC_AXI_CLK>, <GCC_MSS_Q6SS_BOOT_CLK_SRC>,
>> +			<GCC_SEC_CTRL_CLK_SRC>, <GCC_WPSS_AHB_CLK>,
>> +			<GCC_WPSS_AHB_BDG_MST_CLK>, <GCC_WPSS_RSCP_CLK>;
> 
> Is there any reason why this list is significantly larger than a list
> for RB3g2 or FP5?
> 

Unfortunately these are all protected on the IDP board and any access 
would cause a NoC error and then board will fail to boot up.

>> +};
>> +
>> +&lpass_audiocc {
>> +	compatible = "qcom,qcm6490-lpassaudiocc";
>> +	/delete-property/ power-domains;
>> +};
> 
> Separate commits, please.

I will separate the commits.

> 
>> -- 
>> 2.17.1
>>
> 

-- 
Thanks & Regards,
Taniya Das.

Re: [PATCH 3/4] arm64: dts: qcom: qcm6490-idp: Update protected clocks list

[Dmitry Baryshkov]

On Mon, Jun 10, 2024 at 03:57:34PM +0530, Taniya Das wrote:
> 
> 
> On 5/31/2024 5:34 PM, Dmitry Baryshkov wrote:
> > On Fri, May 31, 2024 at 03:52:51PM +0530, Taniya Das wrote:
> > > Certain clocks are not accessible on QCM6490-IDP board,
> > > thus mark them as protected. Update the lpassaudio node to
> > > support the new compatible as the lpassaudio needs to support
> > > the reset functionality on the QCM6490 board and the rest of
> > > the Audio functionality would be provided from the LPASS
> > > firmware.
> > > 
> > > Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
> > > ---
> > >   arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 28 +++++++++++++++++++++++-
> > >   1 file changed, 27 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> > > index a0668f767e4b..4eece564331a 100644
> > > --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> > > +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
> > > @@ -1,6 +1,6 @@
> > >   // SPDX-License-Identifier: BSD-3-Clause
> > >   /*
> > > - * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> > > + * Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
> > >    */
> > >   /dts-v1/;
> > > @@ -688,3 +688,29 @@
> > >   &wifi {
> > >   	memory-region = <&wlan_fw_mem>;
> > >   };
> > > +
> > > +&gcc {
> > > +	protected-clocks = <GCC_AGGRE_NOC_PCIE_1_AXI_CLK> ,<GCC_PCIE_1_AUX_CLK>,
> > > +			<GCC_PCIE_1_AUX_CLK_SRC>, <GCC_PCIE_1_CFG_AHB_CLK>,
> > > +			<GCC_PCIE_1_MSTR_AXI_CLK>, <GCC_PCIE_1_PHY_RCHNG_CLK_SRC>,
> > > +			<GCC_PCIE_1_PIPE_CLK>, <GCC_PCIE_1_PIPE_CLK_SRC>,
> > > +			<GCC_PCIE_1_SLV_AXI_CLK>, <GCC_PCIE_1_SLV_Q2A_AXI_CLK>,
> > > +			<GCC_QSPI_CNOC_PERIPH_AHB_CLK>, <GCC_QSPI_CORE_CLK>,
> > > +			<GCC_QSPI_CORE_CLK_SRC>,<GCC_USB30_SEC_MASTER_CLK>,
> > > +			<GCC_USB30_SEC_MASTER_CLK_SRC>, <GCC_USB30_SEC_MOCK_UTMI_CLK>,
> > > +			<GCC_USB30_SEC_MOCK_UTMI_CLK_SRC>,
> > > +			<GCC_USB30_SEC_MOCK_UTMI_POSTDIV_CLK_SRC>, <GCC_USB30_SEC_SLEEP_CLK>,
> > > +			<GCC_USB3_SEC_PHY_AUX_CLK>, <GCC_USB3_SEC_PHY_AUX_CLK_SRC>,
> > > +			<GCC_USB3_SEC_PHY_COM_AUX_CLK>, <GCC_USB3_SEC_PHY_PIPE_CLK>,
> > > +			<GCC_USB3_SEC_PHY_PIPE_CLK_SRC>, <GCC_CFG_NOC_LPASS_CLK>,
> > > +			<GCC_MSS_GPLL0_MAIN_DIV_CLK_SRC>, <GCC_MSS_CFG_AHB_CLK>,
> > > +			<GCC_MSS_OFFLINE_AXI_CLK>, <GCC_MSS_SNOC_AXI_CLK>,
> > > +			<GCC_MSS_Q6_MEMNOC_AXI_CLK>, <GCC_MSS_Q6SS_BOOT_CLK_SRC>,
> > > +			<GCC_SEC_CTRL_CLK_SRC>, <GCC_WPSS_AHB_CLK>,
> > > +			<GCC_WPSS_AHB_BDG_MST_CLK>, <GCC_WPSS_RSCP_CLK>;
> > 
> > Is there any reason why this list is significantly larger than a list
> > for RB3g2 or FP5?
> > 
> 
> Unfortunately these are all protected on the IDP board and any access would
> cause a NoC error and then board will fail to boot up.

Why? I mean, why does it contain the clocks that are allowed to be
touched on RB3g2 and FP5?

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] arm64: dts: qcom: qcm6490-idp: Update protected clocks list

[Taniya Das]

On 6/10/2024 11:51 PM, Dmitry Baryshkov wrote:
> On Mon, Jun 10, 2024 at 03:57:34PM +0530, Taniya Das wrote:
>>
>>
>> On 5/31/2024 5:34 PM, Dmitry Baryshkov wrote:
>>> On Fri, May 31, 2024 at 03:52:51PM +0530, Taniya Das wrote:
>>>> Certain clocks are not accessible on QCM6490-IDP board,
>>>> thus mark them as protected. Update the lpassaudio node to
>>>> support the new compatible as the lpassaudio needs to support
>>>> the reset functionality on the QCM6490 board and the rest of
>>>> the Audio functionality would be provided from the LPASS
>>>> firmware.
>>>>
>>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>>> ---
>>>>    arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 28 +++++++++++++++++++++++-
>>>>    1 file changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
>>>> index a0668f767e4b..4eece564331a 100644
>>>> --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
>>>> +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
>>>> @@ -1,6 +1,6 @@
>>>>    // SPDX-License-Identifier: BSD-3-Clause
>>>>    /*
>>>> - * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>>>> + * Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>>>     */
>>>>    /dts-v1/;
>>>> @@ -688,3 +688,29 @@
>>>>    &wifi {
>>>>    	memory-region = <&wlan_fw_mem>;
>>>>    };
>>>> +
>>>> +&gcc {
>>>> +	protected-clocks = <GCC_AGGRE_NOC_PCIE_1_AXI_CLK> ,<GCC_PCIE_1_AUX_CLK>,
>>>> +			<GCC_PCIE_1_AUX_CLK_SRC>, <GCC_PCIE_1_CFG_AHB_CLK>,
>>>> +			<GCC_PCIE_1_MSTR_AXI_CLK>, <GCC_PCIE_1_PHY_RCHNG_CLK_SRC>,
>>>> +			<GCC_PCIE_1_PIPE_CLK>, <GCC_PCIE_1_PIPE_CLK_SRC>,
>>>> +			<GCC_PCIE_1_SLV_AXI_CLK>, <GCC_PCIE_1_SLV_Q2A_AXI_CLK>,
>>>> +			<GCC_QSPI_CNOC_PERIPH_AHB_CLK>, <GCC_QSPI_CORE_CLK>,
>>>> +			<GCC_QSPI_CORE_CLK_SRC>,<GCC_USB30_SEC_MASTER_CLK>,
>>>> +			<GCC_USB30_SEC_MASTER_CLK_SRC>, <GCC_USB30_SEC_MOCK_UTMI_CLK>,
>>>> +			<GCC_USB30_SEC_MOCK_UTMI_CLK_SRC>,
>>>> +			<GCC_USB30_SEC_MOCK_UTMI_POSTDIV_CLK_SRC>, <GCC_USB30_SEC_SLEEP_CLK>,
>>>> +			<GCC_USB3_SEC_PHY_AUX_CLK>, <GCC_USB3_SEC_PHY_AUX_CLK_SRC>,
>>>> +			<GCC_USB3_SEC_PHY_COM_AUX_CLK>, <GCC_USB3_SEC_PHY_PIPE_CLK>,
>>>> +			<GCC_USB3_SEC_PHY_PIPE_CLK_SRC>, <GCC_CFG_NOC_LPASS_CLK>,
>>>> +			<GCC_MSS_GPLL0_MAIN_DIV_CLK_SRC>, <GCC_MSS_CFG_AHB_CLK>,
>>>> +			<GCC_MSS_OFFLINE_AXI_CLK>, <GCC_MSS_SNOC_AXI_CLK>,
>>>> +			<GCC_MSS_Q6_MEMNOC_AXI_CLK>, <GCC_MSS_Q6SS_BOOT_CLK_SRC>,
>>>> +			<GCC_SEC_CTRL_CLK_SRC>, <GCC_WPSS_AHB_CLK>,
>>>> +			<GCC_WPSS_AHB_BDG_MST_CLK>, <GCC_WPSS_RSCP_CLK>;
>>>
>>> Is there any reason why this list is significantly larger than a list
>>> for RB3g2 or FP5?
>>>
>>
>> Unfortunately these are all protected on the IDP board and any access would
>> cause a NoC error and then board will fail to boot up.
> 
> Why? I mean, why does it contain the clocks that are allowed to be
> touched on RB3g2 and FP5?
> 

There are some use case level and board functionality changes between 
RB3g2/FP5 vs IDP. Thus these clocks are protected and cannot be accessed.

-- 
Thanks & Regards,
Taniya Das.

Re: [PATCH 3/4] arm64: dts: qcom: qcm6490-idp: Update protected clocks list

[Dmitry Baryshkov]

On August 16, 2024 3:34:26 PM GMT+07:00, Taniya Das <quic_tdas@quicinc.com> wrote:
>
>
>On 6/10/2024 11:51 PM, Dmitry Baryshkov wrote:
>> On Mon, Jun 10, 2024 at 03:57:34PM +0530, Taniya Das wrote:
>>> 
>>> 
>>> On 5/31/2024 5:34 PM, Dmitry Baryshkov wrote:
>>>> On Fri, May 31, 2024 at 03:52:51PM +0530, Taniya Das wrote:
>>>>> Certain clocks are not accessible on QCM6490-IDP board,
>>>>> thus mark them as protected. Update the lpassaudio node to
>>>>> support the new compatible as the lpassaudio needs to support
>>>>> the reset functionality on the QCM6490 board and the rest of
>>>>> the Audio functionality would be provided from the LPASS
>>>>> firmware.
>>>>> 
>>>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
>>>>> ---
>>>>>    arch/arm64/boot/dts/qcom/qcm6490-idp.dts | 28 +++++++++++++++++++++++-
>>>>>    1 file changed, 27 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
>>>>> index a0668f767e4b..4eece564331a 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
>>>>> +++ b/arch/arm64/boot/dts/qcom/qcm6490-idp.dts
>>>>> @@ -1,6 +1,6 @@
>>>>>    // SPDX-License-Identifier: BSD-3-Clause
>>>>>    /*
>>>>> - * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
>>>>> + * Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>>>>>     */
>>>>>    /dts-v1/;
>>>>> @@ -688,3 +688,29 @@
>>>>>    &wifi {
>>>>>    	memory-region = <&wlan_fw_mem>;
>>>>>    };
>>>>> +
>>>>> +&gcc {
>>>>> +	protected-clocks = <GCC_AGGRE_NOC_PCIE_1_AXI_CLK> ,<GCC_PCIE_1_AUX_CLK>,
>>>>> +			<GCC_PCIE_1_AUX_CLK_SRC>, <GCC_PCIE_1_CFG_AHB_CLK>,
>>>>> +			<GCC_PCIE_1_MSTR_AXI_CLK>, <GCC_PCIE_1_PHY_RCHNG_CLK_SRC>,
>>>>> +			<GCC_PCIE_1_PIPE_CLK>, <GCC_PCIE_1_PIPE_CLK_SRC>,
>>>>> +			<GCC_PCIE_1_SLV_AXI_CLK>, <GCC_PCIE_1_SLV_Q2A_AXI_CLK>,
>>>>> +			<GCC_QSPI_CNOC_PERIPH_AHB_CLK>, <GCC_QSPI_CORE_CLK>,
>>>>> +			<GCC_QSPI_CORE_CLK_SRC>,<GCC_USB30_SEC_MASTER_CLK>,
>>>>> +			<GCC_USB30_SEC_MASTER_CLK_SRC>, <GCC_USB30_SEC_MOCK_UTMI_CLK>,
>>>>> +			<GCC_USB30_SEC_MOCK_UTMI_CLK_SRC>,
>>>>> +			<GCC_USB30_SEC_MOCK_UTMI_POSTDIV_CLK_SRC>, <GCC_USB30_SEC_SLEEP_CLK>,
>>>>> +			<GCC_USB3_SEC_PHY_AUX_CLK>, <GCC_USB3_SEC_PHY_AUX_CLK_SRC>,
>>>>> +			<GCC_USB3_SEC_PHY_COM_AUX_CLK>, <GCC_USB3_SEC_PHY_PIPE_CLK>,
>>>>> +			<GCC_USB3_SEC_PHY_PIPE_CLK_SRC>, <GCC_CFG_NOC_LPASS_CLK>,
>>>>> +			<GCC_MSS_GPLL0_MAIN_DIV_CLK_SRC>, <GCC_MSS_CFG_AHB_CLK>,
>>>>> +			<GCC_MSS_OFFLINE_AXI_CLK>, <GCC_MSS_SNOC_AXI_CLK>,
>>>>> +			<GCC_MSS_Q6_MEMNOC_AXI_CLK>, <GCC_MSS_Q6SS_BOOT_CLK_SRC>,
>>>>> +			<GCC_SEC_CTRL_CLK_SRC>, <GCC_WPSS_AHB_CLK>,
>>>>> +			<GCC_WPSS_AHB_BDG_MST_CLK>, <GCC_WPSS_RSCP_CLK>;
>>>> 
>>>> Is there any reason why this list is significantly larger than a list
>>>> for RB3g2 or FP5?
>>>> 
>>> 
>>> Unfortunately these are all protected on the IDP board and any access would
>>> cause a NoC error and then board will fail to boot up.
>> 
>> Why? I mean, why does it contain the clocks that are allowed to be
>> touched on RB3g2 and FP5?
>> 
>
>There are some use case level and board functionality changes between RB3g2/FP5 vs IDP. Thus these clocks are protected and cannot be accessed.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


>


-- 
With best wishes
Dmitry

[PATCH 4/4] arm64: dts: qcom: qcs6490-rb3gen2: Update the LPASS audio node

[Taniya Das]

Update the lpassaudio node to support the new compatible as the
lpassaudio needs to support the reset functionality on the
QCM6490 board and the rest of the Audio functionality would be
provided from the LPASS firmware.

Signed-off-by: Taniya Das <quic_tdas@quicinc.com>
---
 arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
index c4cde4328e3d..9d033700378d 100644
--- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
+++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: BSD-3-Clause
 /*
- * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 /dts-v1/;
@@ -726,3 +726,8 @@
 	function = "gpio";
 	bias-disable;
 };
+
+&lpass_audiocc {
+	compatible = "qcom,qcm6490-lpassaudiocc";
+	/delete-property/ power-domains;
+};
-- 
2.17.1