by Srinivasa Rao Mandadapu

[permalink] [raw]

Subject: Re: [PATCH 4/7] ASoC: codecs: lpass-va-macro: Change bulk voting to individual clock voting

On 9/21/2021 2:20 PM, Srinivas Kandagatla wrote:
>
>
> On 21/09/2021 09:14, Srinivasa Rao Mandadapu wrote:
>>
>> On 9/20/2021 6:55 PM, Srinivas Kandagatla wrote:
>>>
>>>
>> Thanks for Your time Srini!!!
>>> On 20/09/2021 08:35, Srinivasa Rao Mandadapu wrote:
>>>> Change bulk clock frequency voting to individual voting.
>>>>
>>> Can you please explain why do we need to move out using clk bulk apis?
>>>
>>> Am not seeing any thing obvious behavior changing as part of this
>>> patch, more details please..
>>
>> In ADSP bypass use case, few clocks like macro and decode, are
>> optional. So is the main reason for move out.
>
>
> Have you tried using clk_bulk_get_optional()
Tried with above API. It's working fine. Do you suggest to use this
optional API?
>
> --srini
>>
>> And sometimes we are seeing bulk voting failed in Kodiak setup.
>>
>>>> Fixes: 908e6b1df26e (ASoC: codecs: lpass-va-macro: Add support to
>>>> VA Macro)
>>>
>>> Why this has Fixes tag? Are we fixing any bug with this patch?
>> Okay. As such we are not fixing any bug. Will remove this fixes tag
>> on your suggestion.
>>>
>>>>
>>>> Signed-off-by: Venkata Prasad Potturu <[email protected]>
>>>> Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
>>>> ---
>>>> sound/soc/codecs/lpass-va-macro.c | 46
>>>> ++++++++++++++++++++++++---------------
>>>> 1 file changed, 28 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/sound/soc/codecs/lpass-va-macro.c
>>>> b/sound/soc/codecs/lpass-va-macro.c
>>>> index d312a14..0ea39ae 100644
>>>> --- a/sound/soc/codecs/lpass-va-macro.c
>>>> +++ b/sound/soc/codecs/lpass-va-macro.c
>>>> @@ -193,7 +193,10 @@ struct va_macro {
>>>>       int dec_mode[VA_MACRO_NUM_DECIMATORS];
>>>>       struct regmap *regmap;
>>>> -    struct clk_bulk_data clks[VA_NUM_CLKS_MAX];
>>>> +    struct clk *mclk;
>>>> +    struct clk *macro;
>>>> +    struct clk *dcodec;
>>>> +
>>>>       struct clk_hw hw;
>>>>       s32 dmic_0_1_clk_cnt;
>>>> @@ -1321,7 +1324,7 @@ static const struct clk_ops fsgen_gate_ops = {
>>>> static int va_macro_register_fsgen_output(struct va_macro *va)
>>>> {
>>>> -    struct clk *parent = va->clks[2].clk;
>>>> +    struct clk *parent = va->mclk;
>>>>       struct device *dev = va->dev;
>>>>       struct device_node *np = dev->of_node;
>>>>       const char *parent_clk_name;
>>>> @@ -1404,15 +1407,18 @@ static int va_macro_probe(struct
>>>> platform_device *pdev)
>>>>           return -ENOMEM;
>>>>       va->dev = dev;
>>>> -    va->clks[0].id = "macro";
>>>> -    va->clks[1].id = "dcodec";
>>>> -    va->clks[2].id = "mclk";
>>>> -    ret = devm_clk_bulk_get(dev, VA_NUM_CLKS_MAX, va->clks);
>>>> -    if (ret) {
>>>> -        dev_err(dev, "Error getting VA Clocks (%d)\n", ret);
>>>> -        return ret;
>>>> -    }
>>>> +    va->macro = devm_clk_get_optional(dev, "macro");
>>>> +    if (IS_ERR(va->macro))
>>>> +        return PTR_ERR(va->macro);
>>>> +
>>>> +    va->dcodec = devm_clk_get_optional(dev, "dcodec");
>>>> +    if (IS_ERR(va->dcodec))
>>>> +        return PTR_ERR(va->dcodec);
>>>> +
>>>> +    va->mclk = devm_clk_get(dev, "mclk");
>>>> +    if (IS_ERR(va->mclk))
>>>> +        return PTR_ERR(va->mclk);
>>>>       ret = of_property_read_u32(dev->of_node,
>>>> "qcom,dmic-sample-rate",
>>>>                      &sample_rate);
>>>> @@ -1426,10 +1432,11 @@ static int va_macro_probe(struct
>>>> platform_device *pdev)
>>>>       }
>>>>       /* mclk rate */
>>>> -    clk_set_rate(va->clks[1].clk, VA_MACRO_MCLK_FREQ);
>>>> -    ret = clk_bulk_prepare_enable(VA_NUM_CLKS_MAX, va->clks);
>>>> -    if (ret)
>>>> -        return ret;
>>>> +    clk_set_rate(va->mclk, VA_MACRO_MCLK_FREQ);
>>>> +
>>>> +    clk_prepare_enable(va->mclk);
>>>> +    clk_prepare_enable(va->macro);
>>>> +    clk_prepare_enable(va->dcodec);
>>>>       base = devm_platform_ioremap_resource(pdev, 0);
>>>>       if (IS_ERR(base)) {
>>>> @@ -1457,8 +1464,9 @@ static int va_macro_probe(struct
>>>> platform_device *pdev)
>>>>       return ret;
>>>> err:
>>>> -    clk_bulk_disable_unprepare(VA_NUM_CLKS_MAX, va->clks);
>>>> -
>>>> +    clk_disable_unprepare(va->mclk);
>>>> +    clk_disable_unprepare(va->macro);
>>>> +    clk_disable_unprepare(va->dcodec);
>>>>       return ret;
>>>> }
>>>> @@ -1466,8 +1474,10 @@ static int va_macro_remove(struct
>>>> platform_device *pdev)
>>>> {
>>>>       struct va_macro *va = dev_get_drvdata(&pdev->dev);
>>>> -    clk_bulk_disable_unprepare(VA_NUM_CLKS_MAX, va->clks);
>>>> -
>>>> +    of_clk_del_provider(pdev->dev.of_node);
>>>
>>> fsgen clk is registered using devm_* variant of clk apis, so why do
>>> we need this here?
>>>
>> Okay. Will remove it and post new patch.
>>>
>>> --srini
>>>> + clk_disable_unprepare(va->mclk);
>>>> +    clk_disable_unprepare(va->macro);
>>>> +    clk_disable_unprepare(va->dcodec);
>>>>       return 0;
>>>> }
>>>>
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

2021-09-21 13:06:17

by Srinivasa Rao Mandadapu

[permalink] [raw]

Subject: Re: [PATCH 3/7] ASoC: codecs: tx-macro: Change mic control registers to volatile

On 9/21/2021 2:18 PM, Srinivas Kandagatla wrote:
>
>
> On 21/09/2021 08:30, Srinivasa Rao Mandadapu wrote:
>>
>> On 9/20/2021 6:54 PM, Srinivas Kandagatla wrote:
>> Thanks for your time Srini!!
>>>
>>> On 20/09/2021 08:35, Srinivasa Rao Mandadapu wrote:
>>>> Update amic and dmic related tx macro control registers to volatile
>>>>
>>>> Fixes: c39667ddcfc5 (ASoC: codecs: lpass-tx-macro: add support for
>>>> lpass tx macro)
>>>>
>>>> Signed-off-by: Venkata Prasad Potturu <[email protected]>
>>>> Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
>>>> ---
>>>> sound/soc/codecs/lpass-tx-macro.c | 13 +++++++++++++
>>>> 1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/sound/soc/codecs/lpass-tx-macro.c
>>>> b/sound/soc/codecs/lpass-tx-macro.c
>>>> index 9273724..e65b592 100644
>>>> --- a/sound/soc/codecs/lpass-tx-macro.c
>>>> +++ b/sound/soc/codecs/lpass-tx-macro.c
>>>> @@ -423,6 +423,13 @@ static bool tx_is_volatile_register(struct
>>>> device *dev, unsigned int reg)
>>>>       case CDC_TX_TOP_CSR_SWR_DMIC1_CTL:
>>>>       case CDC_TX_TOP_CSR_SWR_DMIC2_CTL:
>>>>       case CDC_TX_TOP_CSR_SWR_DMIC3_CTL:
>>>> +    case CDC_TX_TOP_CSR_SWR_AMIC0_CTL:
>>>> +    case CDC_TX_TOP_CSR_SWR_AMIC1_CTL:
>>>> +    case CDC_TX_CLK_RST_CTRL_MCLK_CONTROL:
>>>> +    case CDC_TX_CLK_RST_CTRL_FS_CNT_CONTROL:
>>>> +    case CDC_TX_CLK_RST_CTRL_SWR_CONTROL:
>>>> +    case CDC_TX_TOP_CSR_SWR_CTRL:
>>>> +    case CDC_TX0_TX_PATH_SEC7:
>>>
>>> Why are these marked as Volatile?
>>> Can you provide some details on the issue that you are seeing?
>>>
>>> --srini
>>
>> Without volatile these registers are not reflecting in Hardware and
>> playback and capture is not working.
>>
>> Will do recheck and keep only required registers as volatile.
>
> This sounds like a total hack to me,
>
> this might be happening in your case:
>
> The default values for this register are different to actual defaults.
> Ex: CDC_TX_TOP_CSR_SWR_AMIC0_CTL default is 0x00
> so writing 0x0 to this register will be no-op as there is no change in
> the register value as compared to default value as per regmap.
>
> In you case make sure the hardware default values are correctly
> reflected in tx_defaults array.

The default values in tx_defaults are proper. But same value is not
reflecting in Hardware, but In Cache it's reflecting set value.

>
> Then setting the desired value should work.
>
>
> --srini
>
>
>
>>
>>>
>>>
>>>>           return true;
>>>>       }
>>>>       return false;
>>>> @@ -1674,6 +1681,12 @@ static int tx_macro_component_probe(struct
>>>> snd_soc_component *comp)
>>>>       snd_soc_component_update_bits(comp, CDC_TX0_TX_PATH_SEC7,
>>>> 0x3F,
>>>>                         0x0A);
>>>> +    snd_soc_component_update_bits(comp,
>>>> CDC_TX_TOP_CSR_SWR_AMIC0_CTL, 0xFF, 0x00);
>>>> +    snd_soc_component_update_bits(comp,
>>>> CDC_TX_TOP_CSR_SWR_AMIC1_CTL, 0xFF, 0x00);
>>>> +    snd_soc_component_update_bits(comp,
>>>> CDC_TX_TOP_CSR_SWR_DMIC0_CTL, 0xFF, 0x00);
>>>> +    snd_soc_component_update_bits(comp,
>>>> CDC_TX_TOP_CSR_SWR_DMIC1_CTL, 0xFF, 0x00);
>>>> +    snd_soc_component_update_bits(comp,
>>>> CDC_TX_TOP_CSR_SWR_DMIC2_CTL, 0xFF, 0x00);
>>>> +    snd_soc_component_update_bits(comp,
>>>> CDC_TX_TOP_CSR_SWR_DMIC3_CTL, 0xFF, 0x00);
>>>>       return 0;
>>>> }
>>>>
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.,
is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.