| Message ID | 20250212-lpass_qcm6490_resets-v3-2-0b1cfb35b38e@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Update LPASS Audio clock driver for QCM6490 board | expand |
On Wed, Feb 12, 2025 at 01:52:20PM +0530, 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 resets. > > Signed-off-by: Taniya Das <quic_tdas@quicinc.com> > --- > drivers/clk/qcom/lpassaudiocc-sc7280.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c > index 45e7264770866f929a3f4663c477330f0bf7aa84..b6439308926371891cc5f9a5e0d4e8393641560d 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> > @@ -713,14 +714,24 @@ static const struct qcom_reset_map lpass_audio_cc_sc7280_resets[] = { > [LPASS_AUDIO_SWR_WSA_CGCR] = { 0xb0, 1 }, > }; > > +static const struct regmap_config lpass_audio_cc_sc7280_reset_regmap_config = { > + .name = "lpassaudio_cc_reset", > + .reg_bits = 32, > + .reg_stride = 4, > + .val_bits = 32, > + .fast_io = true, > + .max_register = 0xc8, > +}; > + > static const struct qcom_cc_desc lpass_audio_cc_reset_sc7280_desc = { > - .config = &lpass_audio_cc_sc7280_regmap_config, > + .config = &lpass_audio_cc_sc7280_reset_regmap_config, > .resets = lpass_audio_cc_sc7280_resets, > .num_resets = ARRAY_SIZE(lpass_audio_cc_sc7280_resets), > }; > > static const struct of_device_id lpass_audio_cc_sc7280_match_table[] = { > - { .compatible = "qcom,sc7280-lpassaudiocc" }, > + { .compatible = "qcom,qcm6490-lpassaudiocc", .data = &lpass_audio_cc_reset_sc7280_desc }, > + { .compatible = "qcom,sc7280-lpassaudiocc", .data = &lpass_audio_cc_sc7280_desc }, > { } > }; > MODULE_DEVICE_TABLE(of, lpass_audio_cc_sc7280_match_table); > @@ -752,13 +763,17 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) > struct regmap *regmap; > int ret; > > + desc = device_get_match_data(&pdev->dev); > + > + if (desc->num_resets) > + return qcom_cc_probe_by_index(pdev, 1, desc); Won't this break SC7280 support by causing an early return? > + > ret = lpass_audio_setup_runtime_pm(pdev); > if (ret) > return ret; > > lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc"; > lpass_audio_cc_sc7280_regmap_config.max_register = 0x2f000; > - desc = &lpass_audio_cc_sc7280_desc; > > regmap = qcom_cc_map(pdev, desc); > if (IS_ERR(regmap)) { > @@ -772,7 +787,7 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) > regmap_write(regmap, 0x4, 0x3b); > regmap_write(regmap, 0x8, 0xff05); > > - ret = qcom_cc_really_probe(&pdev->dev, &lpass_audio_cc_sc7280_desc, regmap); > + ret = qcom_cc_really_probe(&pdev->dev, desc, regmap); > if (ret) { > dev_err(&pdev->dev, "Failed to register LPASS AUDIO CC clocks\n"); > goto exit; > > -- > 2.45.2 >
On 2/12/2025 4:39 PM, Dmitry Baryshkov wrote: > On Wed, Feb 12, 2025 at 01:52:20PM +0530, 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 resets. >> >> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >> --- >> drivers/clk/qcom/lpassaudiocc-sc7280.c | 23 +++++++++++++++++++---- >> 1 file changed, 19 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c >> index 45e7264770866f929a3f4663c477330f0bf7aa84..b6439308926371891cc5f9a5e0d4e8393641560d 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> >> @@ -713,14 +714,24 @@ static const struct qcom_reset_map lpass_audio_cc_sc7280_resets[] = { >> [LPASS_AUDIO_SWR_WSA_CGCR] = { 0xb0, 1 }, >> }; >> >> +static const struct regmap_config lpass_audio_cc_sc7280_reset_regmap_config = { >> + .name = "lpassaudio_cc_reset", >> + .reg_bits = 32, >> + .reg_stride = 4, >> + .val_bits = 32, >> + .fast_io = true, >> + .max_register = 0xc8, >> +}; >> + >> static const struct qcom_cc_desc lpass_audio_cc_reset_sc7280_desc = { >> - .config = &lpass_audio_cc_sc7280_regmap_config, >> + .config = &lpass_audio_cc_sc7280_reset_regmap_config, >> .resets = lpass_audio_cc_sc7280_resets, >> .num_resets = ARRAY_SIZE(lpass_audio_cc_sc7280_resets), >> }; >> >> static const struct of_device_id lpass_audio_cc_sc7280_match_table[] = { >> - { .compatible = "qcom,sc7280-lpassaudiocc" }, >> + { .compatible = "qcom,qcm6490-lpassaudiocc", .data = &lpass_audio_cc_reset_sc7280_desc }, >> + { .compatible = "qcom,sc7280-lpassaudiocc", .data = &lpass_audio_cc_sc7280_desc }, >> { } >> }; >> MODULE_DEVICE_TABLE(of, lpass_audio_cc_sc7280_match_table); >> @@ -752,13 +763,17 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) >> struct regmap *regmap; >> int ret; >> >> + desc = device_get_match_data(&pdev->dev); >> + >> + if (desc->num_resets) >> + return qcom_cc_probe_by_index(pdev, 1, desc); > > Won't this break SC7280 support by causing an early return? > The resets are not defined for SC7280. static const struct qcom_cc_desc lpass_audio_cc_sc7280_desc = { .config = &lpass_audio_cc_sc7280_regmap_config, .clks = lpass_audio_cc_sc7280_clocks, .num_clks = ARRAY_SIZE(lpass_audio_cc_sc7280_clocks), }; The reset get registered for SC7280 after the clocks are registered. qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc); >> + >> ret = lpass_audio_setup_runtime_pm(pdev); >> if (ret) >> return ret; >> >> lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc"; >> lpass_audio_cc_sc7280_regmap_config.max_register = 0x2f000; >> - desc = &lpass_audio_cc_sc7280_desc; >> >> regmap = qcom_cc_map(pdev, desc); >> if (IS_ERR(regmap)) { >> @@ -772,7 +787,7 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) >> regmap_write(regmap, 0x4, 0x3b); >> regmap_write(regmap, 0x8, 0xff05); >> >> - ret = qcom_cc_really_probe(&pdev->dev, &lpass_audio_cc_sc7280_desc, regmap); >> + ret = qcom_cc_really_probe(&pdev->dev, desc, regmap); >> if (ret) { >> dev_err(&pdev->dev, "Failed to register LPASS AUDIO CC clocks\n"); >> goto exit; >> >> -- >> 2.45.2 >> >
On Wed, 12 Feb 2025 at 19:15, Taniya Das <quic_tdas@quicinc.com> wrote: > > > > On 2/12/2025 4:39 PM, Dmitry Baryshkov wrote: > > On Wed, Feb 12, 2025 at 01:52:20PM +0530, 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 resets. > >> > >> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> > >> --- > >> drivers/clk/qcom/lpassaudiocc-sc7280.c | 23 +++++++++++++++++++---- > >> 1 file changed, 19 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c > >> index 45e7264770866f929a3f4663c477330f0bf7aa84..b6439308926371891cc5f9a5e0d4e8393641560d 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> > >> @@ -713,14 +714,24 @@ static const struct qcom_reset_map lpass_audio_cc_sc7280_resets[] = { > >> [LPASS_AUDIO_SWR_WSA_CGCR] = { 0xb0, 1 }, > >> }; > >> > >> +static const struct regmap_config lpass_audio_cc_sc7280_reset_regmap_config = { > >> + .name = "lpassaudio_cc_reset", > >> + .reg_bits = 32, > >> + .reg_stride = 4, > >> + .val_bits = 32, > >> + .fast_io = true, > >> + .max_register = 0xc8, > >> +}; > >> + > >> static const struct qcom_cc_desc lpass_audio_cc_reset_sc7280_desc = { > >> - .config = &lpass_audio_cc_sc7280_regmap_config, > >> + .config = &lpass_audio_cc_sc7280_reset_regmap_config, > >> .resets = lpass_audio_cc_sc7280_resets, > >> .num_resets = ARRAY_SIZE(lpass_audio_cc_sc7280_resets), > >> }; > >> > >> static const struct of_device_id lpass_audio_cc_sc7280_match_table[] = { > >> - { .compatible = "qcom,sc7280-lpassaudiocc" }, > >> + { .compatible = "qcom,qcm6490-lpassaudiocc", .data = &lpass_audio_cc_reset_sc7280_desc }, > >> + { .compatible = "qcom,sc7280-lpassaudiocc", .data = &lpass_audio_cc_sc7280_desc }, > >> { } > >> }; > >> MODULE_DEVICE_TABLE(of, lpass_audio_cc_sc7280_match_table); > >> @@ -752,13 +763,17 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) > >> struct regmap *regmap; > >> int ret; > >> > >> + desc = device_get_match_data(&pdev->dev); > >> + > >> + if (desc->num_resets) > >> + return qcom_cc_probe_by_index(pdev, 1, desc); > > > > Won't this break SC7280 support by causing an early return? > > > > The resets are not defined for SC7280. > static const struct qcom_cc_desc lpass_audio_cc_sc7280_desc = { > .config = &lpass_audio_cc_sc7280_regmap_config, > .clks = lpass_audio_cc_sc7280_clocks, > .num_clks = ARRAY_SIZE(lpass_audio_cc_sc7280_clocks), > }; > > The reset get registered for SC7280 after the clocks are registered. > qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc); Could you please make this condition more obvious and error-prone rather than checking one particular non-obvious property? > > >> + > >> ret = lpass_audio_setup_runtime_pm(pdev); > >> if (ret) > >> return ret; > >> > >> lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc"; > >> lpass_audio_cc_sc7280_regmap_config.max_register = 0x2f000; > >> - desc = &lpass_audio_cc_sc7280_desc; > >> > >> regmap = qcom_cc_map(pdev, desc); > >> if (IS_ERR(regmap)) { > >> @@ -772,7 +787,7 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) > >> regmap_write(regmap, 0x4, 0x3b); > >> regmap_write(regmap, 0x8, 0xff05); > >> > >> - ret = qcom_cc_really_probe(&pdev->dev, &lpass_audio_cc_sc7280_desc, regmap); > >> + ret = qcom_cc_really_probe(&pdev->dev, desc, regmap); > >> if (ret) { > >> dev_err(&pdev->dev, "Failed to register LPASS AUDIO CC clocks\n"); > >> goto exit; > >> > >> -- > >> 2.45.2 > >> > > >
On 2/13/2025 1:30 AM, Dmitry Baryshkov wrote: > On Wed, 12 Feb 2025 at 19:15, Taniya Das <quic_tdas@quicinc.com> wrote: >> >> >> >> On 2/12/2025 4:39 PM, Dmitry Baryshkov wrote: >>> On Wed, Feb 12, 2025 at 01:52:20PM +0530, 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 resets. >>>> >>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >>>> --- >>>> drivers/clk/qcom/lpassaudiocc-sc7280.c | 23 +++++++++++++++++++---- >>>> 1 file changed, 19 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c >>>> index 45e7264770866f929a3f4663c477330f0bf7aa84..b6439308926371891cc5f9a5e0d4e8393641560d 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> >>>> @@ -713,14 +714,24 @@ static const struct qcom_reset_map lpass_audio_cc_sc7280_resets[] = { >>>> [LPASS_AUDIO_SWR_WSA_CGCR] = { 0xb0, 1 }, >>>> }; >>>> >>>> +static const struct regmap_config lpass_audio_cc_sc7280_reset_regmap_config = { >>>> + .name = "lpassaudio_cc_reset", >>>> + .reg_bits = 32, >>>> + .reg_stride = 4, >>>> + .val_bits = 32, >>>> + .fast_io = true, >>>> + .max_register = 0xc8, >>>> +}; >>>> + >>>> static const struct qcom_cc_desc lpass_audio_cc_reset_sc7280_desc = { >>>> - .config = &lpass_audio_cc_sc7280_regmap_config, >>>> + .config = &lpass_audio_cc_sc7280_reset_regmap_config, >>>> .resets = lpass_audio_cc_sc7280_resets, >>>> .num_resets = ARRAY_SIZE(lpass_audio_cc_sc7280_resets), >>>> }; >>>> >>>> static const struct of_device_id lpass_audio_cc_sc7280_match_table[] = { >>>> - { .compatible = "qcom,sc7280-lpassaudiocc" }, >>>> + { .compatible = "qcom,qcm6490-lpassaudiocc", .data = &lpass_audio_cc_reset_sc7280_desc }, >>>> + { .compatible = "qcom,sc7280-lpassaudiocc", .data = &lpass_audio_cc_sc7280_desc }, >>>> { } >>>> }; >>>> MODULE_DEVICE_TABLE(of, lpass_audio_cc_sc7280_match_table); >>>> @@ -752,13 +763,17 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) >>>> struct regmap *regmap; >>>> int ret; >>>> >>>> + desc = device_get_match_data(&pdev->dev); >>>> + >>>> + if (desc->num_resets) >>>> + return qcom_cc_probe_by_index(pdev, 1, desc); >>> >>> Won't this break SC7280 support by causing an early return? >>> >> >> The resets are not defined for SC7280. >> static const struct qcom_cc_desc lpass_audio_cc_sc7280_desc = { >> .config = &lpass_audio_cc_sc7280_regmap_config, >> .clks = lpass_audio_cc_sc7280_clocks, >> .num_clks = ARRAY_SIZE(lpass_audio_cc_sc7280_clocks), >> }; >> >> The reset get registered for SC7280 after the clocks are registered. >> qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc); > > Could you please make this condition more obvious and error-prone > rather than checking one particular non-obvious property? > Dmitry, we had earlier tried [1], but seems like we could not align on this patchset. If you are aligned, please let me know I can fall back on the approach. [1]: https://lore.kernel.org/all/20240318053555.20405-3-quic_tdas@quicinc.com/ Do you have any suggestions that we could consider? >> >>>> + >>>> ret = lpass_audio_setup_runtime_pm(pdev); >>>> if (ret) >>>> return ret; >>>> >>>> lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc"; >>>> lpass_audio_cc_sc7280_regmap_config.max_register = 0x2f000; >>>> - desc = &lpass_audio_cc_sc7280_desc; >>>> >>>> regmap = qcom_cc_map(pdev, desc); >>>> if (IS_ERR(regmap)) { >>>> @@ -772,7 +787,7 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) >>>> regmap_write(regmap, 0x4, 0x3b); >>>> regmap_write(regmap, 0x8, 0xff05); >>>> >>>> - ret = qcom_cc_really_probe(&pdev->dev, &lpass_audio_cc_sc7280_desc, regmap); >>>> + ret = qcom_cc_really_probe(&pdev->dev, desc, regmap); >>>> if (ret) { >>>> dev_err(&pdev->dev, "Failed to register LPASS AUDIO CC clocks\n"); >>>> goto exit; >>>> >>>> -- >>>> 2.45.2 >>>> >>> >> > >
On Thu, 13 Feb 2025 at 08:52, Taniya Das <quic_tdas@quicinc.com> wrote: > > > > On 2/13/2025 1:30 AM, Dmitry Baryshkov wrote: > > On Wed, 12 Feb 2025 at 19:15, Taniya Das <quic_tdas@quicinc.com> wrote: > >> > >> > >> > >> On 2/12/2025 4:39 PM, Dmitry Baryshkov wrote: > >>> On Wed, Feb 12, 2025 at 01:52:20PM +0530, 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 resets. > >>>> > >>>> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> > >>>> --- > >>>> drivers/clk/qcom/lpassaudiocc-sc7280.c | 23 +++++++++++++++++++---- > >>>> 1 file changed, 19 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c > >>>> index 45e7264770866f929a3f4663c477330f0bf7aa84..b6439308926371891cc5f9a5e0d4e8393641560d 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> > >>>> @@ -713,14 +714,24 @@ static const struct qcom_reset_map lpass_audio_cc_sc7280_resets[] = { > >>>> [LPASS_AUDIO_SWR_WSA_CGCR] = { 0xb0, 1 }, > >>>> }; > >>>> > >>>> +static const struct regmap_config lpass_audio_cc_sc7280_reset_regmap_config = { > >>>> + .name = "lpassaudio_cc_reset", > >>>> + .reg_bits = 32, > >>>> + .reg_stride = 4, > >>>> + .val_bits = 32, > >>>> + .fast_io = true, > >>>> + .max_register = 0xc8, > >>>> +}; > >>>> + > >>>> static const struct qcom_cc_desc lpass_audio_cc_reset_sc7280_desc = { > >>>> - .config = &lpass_audio_cc_sc7280_regmap_config, > >>>> + .config = &lpass_audio_cc_sc7280_reset_regmap_config, > >>>> .resets = lpass_audio_cc_sc7280_resets, > >>>> .num_resets = ARRAY_SIZE(lpass_audio_cc_sc7280_resets), > >>>> }; > >>>> > >>>> static const struct of_device_id lpass_audio_cc_sc7280_match_table[] = { > >>>> - { .compatible = "qcom,sc7280-lpassaudiocc" }, > >>>> + { .compatible = "qcom,qcm6490-lpassaudiocc", .data = &lpass_audio_cc_reset_sc7280_desc }, > >>>> + { .compatible = "qcom,sc7280-lpassaudiocc", .data = &lpass_audio_cc_sc7280_desc }, > >>>> { } > >>>> }; > >>>> MODULE_DEVICE_TABLE(of, lpass_audio_cc_sc7280_match_table); > >>>> @@ -752,13 +763,17 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) > >>>> struct regmap *regmap; > >>>> int ret; > >>>> > >>>> + desc = device_get_match_data(&pdev->dev); > >>>> + > >>>> + if (desc->num_resets) > >>>> + return qcom_cc_probe_by_index(pdev, 1, desc); > >>> > >>> Won't this break SC7280 support by causing an early return? > >>> > >> > >> The resets are not defined for SC7280. > >> static const struct qcom_cc_desc lpass_audio_cc_sc7280_desc = { > >> .config = &lpass_audio_cc_sc7280_regmap_config, > >> .clks = lpass_audio_cc_sc7280_clocks, > >> .num_clks = ARRAY_SIZE(lpass_audio_cc_sc7280_clocks), > >> }; > >> > >> The reset get registered for SC7280 after the clocks are registered. > >> qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc); > > > > Could you please make this condition more obvious and error-prone > > rather than checking one particular non-obvious property? > > > > Dmitry, we had earlier tried [1], but seems like we could not align on > this patchset. > > If you are aligned, please let me know I can fall back on the approach. You have been using of_device_is_compatible(). Krzysztof suggested using mach data. Both approaches are fine with me (I'm sorry, Krzysztof, this is a clock driver for a single platform, it doesn't need to scale). You've settled on the second one. So far so good. But! The problem is in readability. Checking for desc->num_resets is a _hidden_ or cryptic way of checking whether to register only a first controller or both. BTW: the commit message also tells nothing about the dropped power domain and skipped PM code. Is it not required anymore? Is it handled automatically by the firmware? But I see that audio codecs still use that power domain. > > [1]: > https://lore.kernel.org/all/20240318053555.20405-3-quic_tdas@quicinc.com/ > > Do you have any suggestions that we could consider? > > >> > >>>> + > >>>> ret = lpass_audio_setup_runtime_pm(pdev); > >>>> if (ret) > >>>> return ret; > >>>> > >>>> lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc"; > >>>> lpass_audio_cc_sc7280_regmap_config.max_register = 0x2f000; > >>>> - desc = &lpass_audio_cc_sc7280_desc; > >>>> > >>>> regmap = qcom_cc_map(pdev, desc); > >>>> if (IS_ERR(regmap)) { > >>>> @@ -772,7 +787,7 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) > >>>> regmap_write(regmap, 0x4, 0x3b); > >>>> regmap_write(regmap, 0x8, 0xff05); > >>>> > >>>> - ret = qcom_cc_really_probe(&pdev->dev, &lpass_audio_cc_sc7280_desc, regmap); > >>>> + ret = qcom_cc_really_probe(&pdev->dev, desc, regmap); > >>>> if (ret) { > >>>> dev_err(&pdev->dev, "Failed to register LPASS AUDIO CC clocks\n"); > >>>> goto exit; > >>>> > >>>> -- > >>>> 2.45.2 > >>>> > >>> > >> > > > > >
On 2/13/2025 7:58 PM, Dmitry Baryshkov wrote: >>>>>> static const struct qcom_cc_desc lpass_audio_cc_reset_sc7280_desc = { >>>>>> - .config = &lpass_audio_cc_sc7280_regmap_config, >>>>>> + .config = &lpass_audio_cc_sc7280_reset_regmap_config, >>>>>> .resets = lpass_audio_cc_sc7280_resets, >>>>>> .num_resets = ARRAY_SIZE(lpass_audio_cc_sc7280_resets), >>>>>> }; >>>>>> >>>>>> static const struct of_device_id lpass_audio_cc_sc7280_match_table[] = { >>>>>> - { .compatible = "qcom,sc7280-lpassaudiocc" }, >>>>>> + { .compatible = "qcom,qcm6490-lpassaudiocc", .data = &lpass_audio_cc_reset_sc7280_desc }, >>>>>> + { .compatible = "qcom,sc7280-lpassaudiocc", .data = &lpass_audio_cc_sc7280_desc }, >>>>>> { } >>>>>> }; >>>>>> MODULE_DEVICE_TABLE(of, lpass_audio_cc_sc7280_match_table); >>>>>> @@ -752,13 +763,17 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) >>>>>> struct regmap *regmap; >>>>>> int ret; >>>>>> >>>>>> + desc = device_get_match_data(&pdev->dev); >>>>>> + >>>>>> + if (desc->num_resets) >>>>>> + return qcom_cc_probe_by_index(pdev, 1, desc); >>>>> Won't this break SC7280 support by causing an early return? >>>>> >>>> The resets are not defined for SC7280. >>>> static const struct qcom_cc_desc lpass_audio_cc_sc7280_desc = { >>>> .config = &lpass_audio_cc_sc7280_regmap_config, >>>> .clks = lpass_audio_cc_sc7280_clocks, >>>> .num_clks = ARRAY_SIZE(lpass_audio_cc_sc7280_clocks), >>>> }; >>>> >>>> The reset get registered for SC7280 after the clocks are registered. >>>> qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc); >>> Could you please make this condition more obvious and error-prone >>> rather than checking one particular non-obvious property? >>> >> Dmitry, we had earlier tried [1], but seems like we could not align on >> this patchset. >> >> If you are aligned, please let me know I can fall back on the approach. > You have been using of_device_is_compatible(). Krzysztof suggested > using mach data. Both approaches are fine with me (I'm sorry, > Krzysztof, this is a clock driver for a single platform, it doesn't > need to scale). > > You've settled on the second one. So far so good. Sure, I will go ahead with the existing approach, but ensure I replace the num_resets check with the of_device_is_compatible(), so it is more readable. Hope this aligns with your thoughts as well. > > But! The problem is in readability. Checking for desc->num_resets is a > _hidden_ or cryptic way of checking whether to register only a first > controller or both. > > BTW: the commit message also tells nothing about the dropped power > domain and skipped PM code. Is it not required anymore? Is it handled > automatically by the firmware? But I see that audio codecs still use > that power domain. Yes, it will be taken care in the firmware and I will update in the commit text. Thanks, Taniya.
On Tue, Feb 18, 2025 at 10:53:05AM +0530, Taniya Das wrote: > > > On 2/13/2025 7:58 PM, Dmitry Baryshkov wrote: > >>>>>> static const struct qcom_cc_desc lpass_audio_cc_reset_sc7280_desc = { > >>>>>> - .config = &lpass_audio_cc_sc7280_regmap_config, > >>>>>> + .config = &lpass_audio_cc_sc7280_reset_regmap_config, > >>>>>> .resets = lpass_audio_cc_sc7280_resets, > >>>>>> .num_resets = ARRAY_SIZE(lpass_audio_cc_sc7280_resets), > >>>>>> }; > >>>>>> > >>>>>> static const struct of_device_id lpass_audio_cc_sc7280_match_table[] = { > >>>>>> - { .compatible = "qcom,sc7280-lpassaudiocc" }, > >>>>>> + { .compatible = "qcom,qcm6490-lpassaudiocc", .data = &lpass_audio_cc_reset_sc7280_desc }, > >>>>>> + { .compatible = "qcom,sc7280-lpassaudiocc", .data = &lpass_audio_cc_sc7280_desc }, > >>>>>> { } > >>>>>> }; > >>>>>> MODULE_DEVICE_TABLE(of, lpass_audio_cc_sc7280_match_table); > >>>>>> @@ -752,13 +763,17 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) > >>>>>> struct regmap *regmap; > >>>>>> int ret; > >>>>>> > >>>>>> + desc = device_get_match_data(&pdev->dev); > >>>>>> + > >>>>>> + if (desc->num_resets) > >>>>>> + return qcom_cc_probe_by_index(pdev, 1, desc); > >>>>> Won't this break SC7280 support by causing an early return? > >>>>> > >>>> The resets are not defined for SC7280. > >>>> static const struct qcom_cc_desc lpass_audio_cc_sc7280_desc = { > >>>> .config = &lpass_audio_cc_sc7280_regmap_config, > >>>> .clks = lpass_audio_cc_sc7280_clocks, > >>>> .num_clks = ARRAY_SIZE(lpass_audio_cc_sc7280_clocks), > >>>> }; > >>>> > >>>> The reset get registered for SC7280 after the clocks are registered. > >>>> qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc); > >>> Could you please make this condition more obvious and error-prone > >>> rather than checking one particular non-obvious property? > >>> > >> Dmitry, we had earlier tried [1], but seems like we could not align on > >> this patchset. > >> > >> If you are aligned, please let me know I can fall back on the approach. > > You have been using of_device_is_compatible(). Krzysztof suggested > > using mach data. Both approaches are fine with me (I'm sorry, > > Krzysztof, this is a clock driver for a single platform, it doesn't > > need to scale). > > > > You've settled on the second one. So far so good. > > Sure, I will go ahead with the existing approach, but ensure I replace > the num_resets check with the of_device_is_compatible(), so it is more > readable. Hope this aligns with your thoughts as well. Ack, thanks. > > > > > But! The problem is in readability. Checking for desc->num_resets is a > > _hidden_ or cryptic way of checking whether to register only a first > > controller or both. > > > > BTW: the commit message also tells nothing about the dropped power > > domain and skipped PM code. Is it not required anymore? Is it handled > > automatically by the firmware? But I see that audio codecs still use > > that power domain. > Yes, it will be taken care in the firmware and I will update in the > commit text. Ack, thanks. > > > Thanks, > Taniya.
diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c index 45e7264770866f929a3f4663c477330f0bf7aa84..b6439308926371891cc5f9a5e0d4e8393641560d 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> @@ -713,14 +714,24 @@ static const struct qcom_reset_map lpass_audio_cc_sc7280_resets[] = { [LPASS_AUDIO_SWR_WSA_CGCR] = { 0xb0, 1 }, }; +static const struct regmap_config lpass_audio_cc_sc7280_reset_regmap_config = { + .name = "lpassaudio_cc_reset", + .reg_bits = 32, + .reg_stride = 4, + .val_bits = 32, + .fast_io = true, + .max_register = 0xc8, +}; + static const struct qcom_cc_desc lpass_audio_cc_reset_sc7280_desc = { - .config = &lpass_audio_cc_sc7280_regmap_config, + .config = &lpass_audio_cc_sc7280_reset_regmap_config, .resets = lpass_audio_cc_sc7280_resets, .num_resets = ARRAY_SIZE(lpass_audio_cc_sc7280_resets), }; static const struct of_device_id lpass_audio_cc_sc7280_match_table[] = { - { .compatible = "qcom,sc7280-lpassaudiocc" }, + { .compatible = "qcom,qcm6490-lpassaudiocc", .data = &lpass_audio_cc_reset_sc7280_desc }, + { .compatible = "qcom,sc7280-lpassaudiocc", .data = &lpass_audio_cc_sc7280_desc }, { } }; MODULE_DEVICE_TABLE(of, lpass_audio_cc_sc7280_match_table); @@ -752,13 +763,17 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) struct regmap *regmap; int ret; + desc = device_get_match_data(&pdev->dev); + + if (desc->num_resets) + return qcom_cc_probe_by_index(pdev, 1, desc); + ret = lpass_audio_setup_runtime_pm(pdev); if (ret) return ret; lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc"; lpass_audio_cc_sc7280_regmap_config.max_register = 0x2f000; - desc = &lpass_audio_cc_sc7280_desc; regmap = qcom_cc_map(pdev, desc); if (IS_ERR(regmap)) { @@ -772,7 +787,7 @@ static int lpass_audio_cc_sc7280_probe(struct platform_device *pdev) regmap_write(regmap, 0x4, 0x3b); regmap_write(regmap, 0x8, 0xff05); - ret = qcom_cc_really_probe(&pdev->dev, &lpass_audio_cc_sc7280_desc, regmap); + ret = qcom_cc_really_probe(&pdev->dev, desc, regmap); if (ret) { dev_err(&pdev->dev, "Failed to register LPASS AUDIO CC clocks\n"); goto exit;
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 resets. Signed-off-by: Taniya Das <quic_tdas@quicinc.com> --- drivers/clk/qcom/lpassaudiocc-sc7280.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)