| Message ID | 20230327163249.1081824-5-quic_mohs@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Remove the qdsp6ss register from lpasscc | expand |
Quoting Mohammad Rafi Shaik (2023-03-27 09:32:49) > diff --git a/drivers/clk/qcom/lpasscc-sc7280.c b/drivers/clk/qcom/lpasscc-sc7280.c > index 48432010ce24..4719e3fa8b05 100644 > --- a/drivers/clk/qcom/lpasscc-sc7280.c > +++ b/drivers/clk/qcom/lpasscc-sc7280.c > @@ -121,17 +67,10 @@ static int lpass_cc_sc7280_probe(struct platform_device *pdev) > goto destroy_pm_clk; > } > > - lpass_regmap_config.name = "qdsp6ss"; > - desc = &lpass_qdsp6ss_sc7280_desc; > - > - ret = qcom_cc_probe_by_index(pdev, 0, desc); > - if (ret) > - goto destroy_pm_clk; > - > lpass_regmap_config.name = "top_cc"; > desc = &lpass_cc_top_sc7280_desc; > > - ret = qcom_cc_probe_by_index(pdev, 1, desc); > + ret = qcom_cc_probe_by_index(pdev, 0, desc); Instead of changing the binding, it may be better to leave it as is and ignore the first reg property in the driver. Then you don't need any DTS patch or binding patch. You can just have this one patch. After that you can introduce a new compatible string for the proper design and make it have only a single reg property and deprecate the old binding. The driver can then pick index 0 if the new compatible is present.
On 3/29/2023 8:41 AM, Stephen Boyd wrote: > Quoting Mohammad Rafi Shaik (2023-03-27 09:32:49) >> diff --git a/drivers/clk/qcom/lpasscc-sc7280.c b/drivers/clk/qcom/lpasscc-sc7280.c >> index 48432010ce24..4719e3fa8b05 100644 >> --- a/drivers/clk/qcom/lpasscc-sc7280.c >> +++ b/drivers/clk/qcom/lpasscc-sc7280.c >> @@ -121,17 +67,10 @@ static int lpass_cc_sc7280_probe(struct platform_device *pdev) >> goto destroy_pm_clk; >> } >> >> - lpass_regmap_config.name = "qdsp6ss"; >> - desc = &lpass_qdsp6ss_sc7280_desc; >> - >> - ret = qcom_cc_probe_by_index(pdev, 0, desc); >> - if (ret) >> - goto destroy_pm_clk; >> - >> lpass_regmap_config.name = "top_cc"; >> desc = &lpass_cc_top_sc7280_desc; >> >> - ret = qcom_cc_probe_by_index(pdev, 1, desc); >> + ret = qcom_cc_probe_by_index(pdev, 0, desc); > Instead of changing the binding, it may be better to leave it as is and > ignore the first reg property in the driver. Then you don't need any DTS > patch or binding patch. You can just have this one patch. After that you > can introduce a new compatible string for the proper design and make it > have only a single reg property and deprecate the old binding. The > driver can then pick index 0 if the new compatible is present. Thanks for comment, The main issue with sc7280.dtsi file. Required to upstream remoteproc_adsp node for audioreach adsp based solution. The base address for remoteproc_adsp dts node is 0x3000000. Please refer below link audioreach dts patch: https://patchwork.kernel.org/project/linux-arm-msm/patch/1675700201-12890-4-git-send-email-quic_srivasam@quicinc.com/ remoteproc_adsp: remoteproc@3000000 { compatible = "qcom,sc7280-adsp-pil"; reg = <0 0x03000000 0 0x5000>, <0 0x0355b000 0 0x10>; reg-names = "qdsp6ss_base", "lpass_efuse"; and in sc7280.dtsi lpasscc node base address also same. lpasscc: lpasscc@3000000 { compatible = "qcom,sc7280-lpasscc"; reg = <0 0x03000000 0 0x40>, <0 0x03c04000 0 0x4>, In single dtsi file should not have same physical address node. Required to sort the nodes based on physical address. As the qdsp6ss clocks are being enabled in remoteproc driver, removing the qdsp6ss reg region from lpasscc in sc7280.dtsi.
Quoting Mohammad Rafi Shaik (2023-03-29 02:24:43) > > The main issue with sc7280.dtsi file. > > Required to upstream remoteproc_adsp node for audioreach adsp based > solution. > The base address for remoteproc_adsp dts node is 0x3000000. > > Please refer below link audioreach dts patch: > https://patchwork.kernel.org/project/linux-arm-msm/patch/1675700201-12890-4-git-send-email-quic_srivasam@quicinc.com/ > > remoteproc_adsp: remoteproc@3000000 { > compatible = "qcom,sc7280-adsp-pil"; > reg = <0 0x03000000 0 0x5000>, <0 0x0355b000 0 0x10>; > reg-names = "qdsp6ss_base", "lpass_efuse"; > > and in sc7280.dtsi lpasscc node base address also same. > > lpasscc: lpasscc@3000000 { > compatible = "qcom,sc7280-lpasscc"; > reg = <0 0x03000000 0 0x40>, > <0 0x03c04000 0 0x4>, > > In single dtsi file should not have same physical address node. > Required to sort the nodes based on physical address. Yes the same address shouldn't be used twice, but it still compiles, right? The node name is different, remoteproc vs. clock-controller, so it should work for the interim while the qcom,sc7280-lpasscc-2 binding is written that only has one reg property. I'm suggesting you don't change the existing binding. Instead, deprecate the compatible and add a new compatible for the binding that omits the second reg property. Then the driver patch can work with old and new dts files.
On 27/03/2023 18:32, Mohammad Rafi Shaik wrote: > The qdsp6ss memory region is being shared by ADSP remoteproc device and > lpasscc clock device, hence causing memory conflict. > As the qdsp6ss clocks are being enabled in remoteproc driver, remove the > qdsp6ss clock registration. > > Fixes: 4ab43d171181 ("clk: qcom: Add lpass clock controller driver for SC7280") I don't understand why this is a fix. The clocks were working before, right? So removing them is both ABI break and not a fix. More over, this *cannot* be backported because it will break users, thus Fixes tag is here misleading stable folks. Best regards, Krzysztof
diff --git a/drivers/clk/qcom/lpasscc-sc7280.c b/drivers/clk/qcom/lpasscc-sc7280.c index 48432010ce24..4719e3fa8b05 100644 --- a/drivers/clk/qcom/lpasscc-sc7280.c +++ b/drivers/clk/qcom/lpasscc-sc7280.c @@ -30,48 +30,6 @@ static struct clk_branch lpass_top_cc_lpi_q6_axim_hs_clk = { }, }; -static struct clk_branch lpass_qdsp6ss_core_clk = { - .halt_reg = 0x20, - /* CLK_OFF would not toggle until LPASS is out of reset */ - .halt_check = BRANCH_HALT_SKIP, - .clkr = { - .enable_reg = 0x20, - .enable_mask = BIT(0), - .hw.init = &(struct clk_init_data){ - .name = "lpass_qdsp6ss_core_clk", - .ops = &clk_branch2_ops, - }, - }, -}; - -static struct clk_branch lpass_qdsp6ss_xo_clk = { - .halt_reg = 0x38, - /* CLK_OFF would not toggle until LPASS is out of reset */ - .halt_check = BRANCH_HALT_SKIP, - .clkr = { - .enable_reg = 0x38, - .enable_mask = BIT(0), - .hw.init = &(struct clk_init_data){ - .name = "lpass_qdsp6ss_xo_clk", - .ops = &clk_branch2_ops, - }, - }, -}; - -static struct clk_branch lpass_qdsp6ss_sleep_clk = { - .halt_reg = 0x3c, - /* CLK_OFF would not toggle until LPASS is out of reset */ - .halt_check = BRANCH_HALT_SKIP, - .clkr = { - .enable_reg = 0x3c, - .enable_mask = BIT(0), - .hw.init = &(struct clk_init_data){ - .name = "lpass_qdsp6ss_sleep_clk", - .ops = &clk_branch2_ops, - }, - }, -}; - static struct regmap_config lpass_regmap_config = { .reg_bits = 32, .reg_stride = 4, @@ -90,18 +48,6 @@ static const struct qcom_cc_desc lpass_cc_top_sc7280_desc = { .num_clks = ARRAY_SIZE(lpass_cc_top_sc7280_clocks), }; -static struct clk_regmap *lpass_qdsp6ss_sc7280_clocks[] = { - [LPASS_QDSP6SS_XO_CLK] = &lpass_qdsp6ss_xo_clk.clkr, - [LPASS_QDSP6SS_SLEEP_CLK] = &lpass_qdsp6ss_sleep_clk.clkr, - [LPASS_QDSP6SS_CORE_CLK] = &lpass_qdsp6ss_core_clk.clkr, -}; - -static const struct qcom_cc_desc lpass_qdsp6ss_sc7280_desc = { - .config = &lpass_regmap_config, - .clks = lpass_qdsp6ss_sc7280_clocks, - .num_clks = ARRAY_SIZE(lpass_qdsp6ss_sc7280_clocks), -}; - static int lpass_cc_sc7280_probe(struct platform_device *pdev) { const struct qcom_cc_desc *desc; @@ -121,17 +67,10 @@ static int lpass_cc_sc7280_probe(struct platform_device *pdev) goto destroy_pm_clk; } - lpass_regmap_config.name = "qdsp6ss"; - desc = &lpass_qdsp6ss_sc7280_desc; - - ret = qcom_cc_probe_by_index(pdev, 0, desc); - if (ret) - goto destroy_pm_clk; - lpass_regmap_config.name = "top_cc"; desc = &lpass_cc_top_sc7280_desc; - ret = qcom_cc_probe_by_index(pdev, 1, desc); + ret = qcom_cc_probe_by_index(pdev, 0, desc); if (ret) goto destroy_pm_clk;
The qdsp6ss memory region is being shared by ADSP remoteproc device and lpasscc clock device, hence causing memory conflict. As the qdsp6ss clocks are being enabled in remoteproc driver, remove the qdsp6ss clock registration. Fixes: 4ab43d171181 ("clk: qcom: Add lpass clock controller driver for SC7280") Signed-off-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com> --- drivers/clk/qcom/lpasscc-sc7280.c | 63 +------------------------------ 1 file changed, 1 insertion(+), 62 deletions(-)