| Message ID | 1672407799-13768-2-git-send-email-quic_srivasam@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add resets for ADSP based audio clock controller driver. | expand |
On 30/12/2022 14:43, Srinivasa Rao Mandadapu 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 > clock controlling in the clock driver. > That's an ABI break change. You cannot just drop it because the driver will fail now to probe with old DTS. This ABI break (and Fixes tag below) requires extensive justification where is the bug and how it affects users. Otherwise, I see no reason for ABI changes. > Fixes: d15eb8012476 ("dt-bindings: clock: Add YAML schemas for LPASS clocks on SC7280") > Drop blank line. > Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> > Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com> > --- > Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml > index 6151fde..9c72b8e 100644 > --- a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml > +++ b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml > @@ -33,12 +33,10 @@ properties: > > reg: > items: > - - description: LPASS qdsp6ss register > - description: LPASS top-cc register > Best regards, Krzysztof
On 03/01/2023 06:51, Srinivasa Rao Mandadapu wrote: > > On 1/2/2023 9:33 PM, Krzysztof Kozlowski wrote: > Thanks for Your time Krzyszto!!! >> On 30/12/2022 14:43, Srinivasa Rao Mandadapu 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 >>> clock controlling in the clock driver. >>> >> That's an ABI break change. You cannot just drop it because the driver >> will fail now to probe with old DTS. >> >> This ABI break (and Fixes tag below) requires extensive justification >> where is the bug and how it affects users. Otherwise, I see no reason >> for ABI changes. > > As ADSP path is not used so far due to various vendor requirements, we > haven't seen this conflict till now. The device is used in sc7280.dtsi (and it is not disabled), thus it is available in every board. > > Actually, in the below commit, clock handling in remoteproc driver has > already up-streamed by Bjorn. > > If we add PIL device node and use remoteproc driver, qcom_q6v5_adsp.c, > it's mandatory to remove it from clock > > driver. > > Also the memory region is used by clock driver is part of > LPASS_QDSP6V67SS register region. > > Here is the commit 0c6de4c299cc8f3283c38f3778777e00ed7e4b69 > ("remoteproc: qcom: qdsp6-adsp: Add support for QCS404 CDSP") The commit introduces qcom,qcs404-cdsp-pil which is not used in upstream DTS... Anyway none of the reasons above explain to me why ABI has to be broken. To remind - valid reasons are usually only: "it never worked" or "I was introduced recently so there are no users". > For ABI Break resolution shall I include, device tree changes also in > this series? Or Could you please suggest better approach? Lack of DTS changes causes upstream to be broken. This is independent of ABI break. ABI means supporting users of the binding, so firmware, other OS, out-of-tree DTS users etc. All of them are broken. You need to keep backwards compatibility. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml index 6151fde..9c72b8e 100644 --- a/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml +++ b/Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml @@ -33,12 +33,10 @@ properties: reg: items: - - description: LPASS qdsp6ss register - description: LPASS top-cc register reg-names: items: - - const: qdsp6ss - const: top_cc required: @@ -54,10 +52,10 @@ examples: - | #include <dt-bindings/clock/qcom,gcc-sc7280.h> #include <dt-bindings/clock/qcom,lpass-sc7280.h> - clock-controller@3000000 { + clock-controller@3c04000 { compatible = "qcom,sc7280-lpasscc"; - reg = <0x03000000 0x40>, <0x03c04000 0x4>; - reg-names = "qdsp6ss", "top_cc"; + reg = <0x03c04000 0x4>; + reg-names = "top_cc"; clocks = <&gcc GCC_CFG_NOC_LPASS_CLK>; clock-names = "iface"; #clock-cells = <1>;
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 clock controlling in the clock driver. Fixes: d15eb8012476 ("dt-bindings: clock: Add YAML schemas for LPASS clocks on SC7280") Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com> Tested-by: Mohammad Rafi Shaik <quic_mohs@quicinc.com> --- Documentation/devicetree/bindings/clock/qcom,sc7280-lpasscc.yaml | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)