diff mbox series

[RESEND,v3,4/4] clk: qcom: lpasscc-sc7280: Add resets for audioreach

Message ID 1672849297-3116-5-git-send-email-quic_srivasam@quicinc.com
State New
Headers
Series Add resets for ADSP based audio clock controller driver |

Commit Message

Srinivasa Rao Mandadapu Jan. 4, 2023, 4:21 p.m. UTC 
  The clock gating control for TX/RX/WSA core bus clocks would be required
to be reset(moved from hardware control) from audio core driver. Thus
add the support for the reset clocks in audioreach based clock driver.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
---
 drivers/clk/qcom/lpasscc-sc7280.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Stephen Boyd Jan. 11, 2023, 9:24 p.m. UTC | #1 
Quoting Srinivasa Rao Mandadapu (2023-01-04 08:21:37)
> The clock gating control for TX/RX/WSA core bus clocks would be required
> to be reset(moved from hardware control) from audio core driver. Thus
> add the support for the reset clocks in audioreach based clock driver.
>
> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
> ---
>  drivers/clk/qcom/lpasscc-sc7280.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/clk/qcom/lpasscc-sc7280.c b/drivers/clk/qcom/lpasscc-sc7280.c
> index 85dd5b9..1efb72d 100644
> --- a/drivers/clk/qcom/lpasscc-sc7280.c
> +++ b/drivers/clk/qcom/lpasscc-sc7280.c
> @@ -12,10 +12,12 @@
>  #include <linux/regmap.h>
>
>  #include <dt-bindings/clock/qcom,lpass-sc7280.h>
> +#include <dt-bindings/clock/qcom,lpassaudiocc-sc7280.h>
>
>  #include "clk-regmap.h"
>  #include "clk-branch.h"
>  #include "common.h"
> +#include "reset.h"
>
>  static struct clk_branch lpass_top_cc_lpi_q6_axim_hs_clk = {
>         .halt_reg = 0x0,
> @@ -102,6 +104,18 @@ static const struct qcom_cc_desc lpass_qdsp6ss_sc7280_desc = {
>         .num_clks = ARRAY_SIZE(lpass_qdsp6ss_sc7280_clocks),
>  };
>
> +static const struct qcom_reset_map lpass_cc_sc7280_resets[] = {
> +       [LPASS_AUDIO_SWR_RX_CGCR] =  { 0xa0, 1 },
> +       [LPASS_AUDIO_SWR_TX_CGCR] =  { 0xa8, 1 },
> +       [LPASS_AUDIO_SWR_WSA_CGCR] = { 0xb0, 1 },

Why are we adding these resets again? These are already exposed in
lpassaudiocc-sc7280.c
Srinivasa Rao Mandadapu Jan. 12, 2023, 7:53 a.m. UTC | #2 
On 1/12/2023 2:54 AM, Stephen Boyd wrote:
Thanks for your time Stephen!!!
> Quoting Srinivasa Rao Mandadapu (2023-01-04 08:21:37)
>> The clock gating control for TX/RX/WSA core bus clocks would be required
>> to be reset(moved from hardware control) from audio core driver. Thus
>> add the support for the reset clocks in audioreach based clock driver.
>>
>> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
>> Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com>
>> ---
>>   drivers/clk/qcom/lpasscc-sc7280.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/clk/qcom/lpasscc-sc7280.c b/drivers/clk/qcom/lpasscc-sc7280.c
>> index 85dd5b9..1efb72d 100644
>> --- a/drivers/clk/qcom/lpasscc-sc7280.c
>> +++ b/drivers/clk/qcom/lpasscc-sc7280.c
>> @@ -12,10 +12,12 @@
>>   #include <linux/regmap.h>
>>
>>   #include <dt-bindings/clock/qcom,lpass-sc7280.h>
>> +#include <dt-bindings/clock/qcom,lpassaudiocc-sc7280.h>
>>
>>   #include "clk-regmap.h"
>>   #include "clk-branch.h"
>>   #include "common.h"
>> +#include "reset.h"
>>
>>   static struct clk_branch lpass_top_cc_lpi_q6_axim_hs_clk = {
>>          .halt_reg = 0x0,
>> @@ -102,6 +104,18 @@ static const struct qcom_cc_desc lpass_qdsp6ss_sc7280_desc = {
>>          .num_clks = ARRAY_SIZE(lpass_qdsp6ss_sc7280_clocks),
>>   };
>>
>> +static const struct qcom_reset_map lpass_cc_sc7280_resets[] = {
>> +       [LPASS_AUDIO_SWR_RX_CGCR] =  { 0xa0, 1 },
>> +       [LPASS_AUDIO_SWR_TX_CGCR] =  { 0xa8, 1 },
>> +       [LPASS_AUDIO_SWR_WSA_CGCR] = { 0xb0, 1 },
> Why are we adding these resets again? These are already exposed in
> lpassaudiocc-sc7280.c

As explained in previous versions, legacy path nodes are not being used 
in ADSP based platforms, due to conflicts.

Hence lpasscc node alone being used exclusively in ADSP based solution, 
resets are added.

In probe also, these reset controls are enabled based on 
"qcom,adsp-pil-mode" property.
Stephen Boyd Jan. 12, 2023, 7:35 p.m. UTC | #3 
Quoting Srinivasa Rao Mandadapu (2023-01-11 23:53:23)
>
> On 1/12/2023 2:54 AM, Stephen Boyd wrote:
> Thanks for your time Stephen!!!
> > Quoting Srinivasa Rao Mandadapu (2023-01-04 08:21:37)
> >> diff --git a/drivers/clk/qcom/lpasscc-sc7280.c b/drivers/clk/qcom/lpasscc-sc7280.c
> >> index 85dd5b9..1efb72d 100644
> >> --- a/drivers/clk/qcom/lpasscc-sc7280.c
> >> +++ b/drivers/clk/qcom/lpasscc-sc7280.c
> >> @@ -102,6 +104,18 @@ static const struct qcom_cc_desc lpass_qdsp6ss_sc7280_desc = {
> >>          .num_clks = ARRAY_SIZE(lpass_qdsp6ss_sc7280_clocks),
> >>   };
> >>
> >> +static const struct qcom_reset_map lpass_cc_sc7280_resets[] = {
> >> +       [LPASS_AUDIO_SWR_RX_CGCR] =  { 0xa0, 1 },
> >> +       [LPASS_AUDIO_SWR_TX_CGCR] =  { 0xa8, 1 },
> >> +       [LPASS_AUDIO_SWR_WSA_CGCR] = { 0xb0, 1 },
> > Why are we adding these resets again? These are already exposed in
> > lpassaudiocc-sc7280.c
>
> As explained in previous versions, legacy path nodes are not being used
> in ADSP based platforms, due to conflicts.

What is legacy path nodes?

>
> Hence lpasscc node alone being used exclusively in ADSP based solution,
> resets are added.

I think I understand..

>
> In probe also, these reset controls are enabled based on
> "qcom,adsp-pil-mode" property.
>

but now I'm super confused! Please help me! We shouldn't have two
different device nodes for the same physical hardware registers.
Instead, we should have one node. The "qcom,adsp-pil-mode" property was
supposed to indicate the different mode of operation.

Maybe the audio clk and reset drivers on sc7280 are duplicating each
other and one of them can be removed?
Srinivasa Rao Mandadapu Jan. 19, 2023, 6:02 a.m. UTC | #4 
On 1/13/2023 1:05 AM, Stephen Boyd wrote:
> Quoting Srinivasa Rao Mandadapu (2023-01-11 23:53:23)
>> On 1/12/2023 2:54 AM, Stephen Boyd wrote:
>> Thanks for your time Stephen!!!
>>> Quoting Srinivasa Rao Mandadapu (2023-01-04 08:21:37)
>>>> diff --git a/drivers/clk/qcom/lpasscc-sc7280.c b/drivers/clk/qcom/lpasscc-sc7280.c
>>>> index 85dd5b9..1efb72d 100644
>>>> --- a/drivers/clk/qcom/lpasscc-sc7280.c
>>>> +++ b/drivers/clk/qcom/lpasscc-sc7280.c
>>>> @@ -102,6 +104,18 @@ static const struct qcom_cc_desc lpass_qdsp6ss_sc7280_desc = {
>>>>           .num_clks = ARRAY_SIZE(lpass_qdsp6ss_sc7280_clocks),
>>>>    };
>>>>
>>>> +static const struct qcom_reset_map lpass_cc_sc7280_resets[] = {
>>>> +       [LPASS_AUDIO_SWR_RX_CGCR] =  { 0xa0, 1 },
>>>> +       [LPASS_AUDIO_SWR_TX_CGCR] =  { 0xa8, 1 },
>>>> +       [LPASS_AUDIO_SWR_WSA_CGCR] = { 0xb0, 1 },
>>> Why are we adding these resets again? These are already exposed in
>>> lpassaudiocc-sc7280.c
>> As explained in previous versions, legacy path nodes are not being used
>> in ADSP based platforms, due to conflicts.
> What is legacy path nodes?
Legacy path nodes are for ADSP bypass use case such as nodes 
lpass_audiocc, lpass_core, etc.
>
>> Hence lpasscc node alone being used exclusively in ADSP based solution,
>> resets are added.
> I think I understand..
>
>> In probe also, these reset controls are enabled based on
>> "qcom,adsp-pil-mode" property.
>>
> but now I'm super confused! Please help me! We shouldn't have two
> different device nodes for the same physical hardware registers.
> Instead, we should have one node. The "qcom,adsp-pil-mode" property was
> supposed to indicate the different mode of operation.
>
> Maybe the audio clk and reset drivers on sc7280 are duplicating each
> other and one of them can be removed?

Yes agreed, that for controlling same registers from two different 
drivers is not good solution.

But, when we are validating ADSP solution, we have seen issues like ADSP 
is not coming out of reset with the existing bypass mode

clock drivers(lpassaudiocc_sc7280.c and lpasscoreecc_sc7280.c) enabled.

As per your suggestion, will try to address that  issues with 
"qcom,adsp-pil-mode" property and avoid duplicating reset control code

in lpasscc driver(lpasscc_sc7280.c).
diff mbox series

Patch

diff --git a/drivers/clk/qcom/lpasscc-sc7280.c b/drivers/clk/qcom/lpasscc-sc7280.c
index 85dd5b9..1efb72d 100644
--- a/drivers/clk/qcom/lpasscc-sc7280.c
+++ b/drivers/clk/qcom/lpasscc-sc7280.c
@@ -12,10 +12,12 @@ 
 #include <linux/regmap.h>
 
 #include <dt-bindings/clock/qcom,lpass-sc7280.h>
+#include <dt-bindings/clock/qcom,lpassaudiocc-sc7280.h>
 
 #include "clk-regmap.h"
 #include "clk-branch.h"
 #include "common.h"
+#include "reset.h"
 
 static struct clk_branch lpass_top_cc_lpi_q6_axim_hs_clk = {
 	.halt_reg = 0x0,
@@ -102,6 +104,18 @@  static const struct qcom_cc_desc lpass_qdsp6ss_sc7280_desc = {
 	.num_clks = ARRAY_SIZE(lpass_qdsp6ss_sc7280_clocks),
 };
 
+static const struct qcom_reset_map lpass_cc_sc7280_resets[] = {
+	[LPASS_AUDIO_SWR_RX_CGCR] =  { 0xa0, 1 },
+	[LPASS_AUDIO_SWR_TX_CGCR] =  { 0xa8, 1 },
+	[LPASS_AUDIO_SWR_WSA_CGCR] = { 0xb0, 1 },
+};
+
+static const struct qcom_cc_desc lpass_audio_cc_reset_sc7280_desc = {
+	.config = &lpass_regmap_config,
+	.resets = lpass_cc_sc7280_resets,
+	.num_resets = ARRAY_SIZE(lpass_cc_sc7280_resets),
+};
+
 static int lpass_cc_sc7280_probe(struct platform_device *pdev)
 {
 	const struct qcom_cc_desc *desc;
@@ -134,6 +148,15 @@  static int lpass_cc_sc7280_probe(struct platform_device *pdev)
 	if (ret)
 		goto destroy_pm_clk;
 
+	if (of_property_read_bool(pdev->dev.of_node, "qcom,adsp-pil-mode")) {
+		lpass_regmap_config.name = "reset_cgcr";
+		desc = &lpass_audio_cc_reset_sc7280_desc;
+
+		ret = qcom_cc_probe_by_index(pdev, 2, desc);
+		if (ret)
+			goto destroy_pm_clk;
+	}
+
 	return 0;
 
 destroy_pm_clk: