| Message ID | 20240531102252.26061-3-quic_tdas@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Update LPASS Audio clock driver for QCM6490 board | expand |
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
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
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.
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 >
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 >
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. >
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 >
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.
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
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);
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(+)