| Message ID | 1629282424-4070-2-git-send-email-mkrishn@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1,1/4] dt-bindings: msm: add DT bindings for sc7280 | expand |
Quoting Krishna Manikandan (2021-08-18 03:27:02) > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 53a21d0..fd7ff1c 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -5,6 +5,7 @@ > * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved. > */ > > +#include <dt-bindings/clock/qcom,dispcc-sc7280.h> > #include <dt-bindings/clock/qcom,gcc-sc7280.h> > #include <dt-bindings/clock/qcom,rpmh.h> > #include <dt-bindings/interconnect/qcom,sc7280.h> > @@ -1424,6 +1425,90 @@ > #power-domain-cells = <1>; > }; > > + mdss: mdss@ae00000 { subsystem@ae00000 > + compatible = "qcom,sc7280-mdss"; > + reg = <0 0x0ae00000 0 0x1000>; > + reg-names = "mdss"; > + > + power-domains = <&dispcc DISP_CC_MDSS_CORE_GDSC>; > + > + clocks = <&gcc GCC_DISP_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_MDP_CLK>; > + clock-names = "iface", "ahb", "core"; > + > + assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>; > + assigned-clock-rates = <300000000>; > + > + interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-controller; > + #interrupt-cells = <1>; > + > + interconnects = <&mmss_noc MASTER_MDP0 0 &mc_virt SLAVE_EBI1 0>; > + interconnect-names = "mdp0-mem"; > + > + iommus = <&apps_smmu 0x900 0x402>; > + > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > + status = "disabled"; > + > + mdp: mdp@ae01000 { display-controller@ae01000 > + compatible = "qcom,sc7280-dpu"; > + reg = <0 0x0ae01000 0 0x8f030>, > + <0 0x0aeb0000 0 0x2008>; > + reg-names = "mdp", "vbif"; > + > + clocks = <&gcc GCC_DISP_HF_AXI_CLK>, > + <&gcc GCC_DISP_SF_AXI_CLK>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>, > + <&dispcc DISP_CC_MDSS_MDP_LUT_CLK>, > + <&dispcc DISP_CC_MDSS_MDP_CLK>, > + <&dispcc DISP_CC_MDSS_VSYNC_CLK>; > + clock-names = "bus", "nrt_bus", "iface", "lut", "core", > + "vsync"; One line per string please. > + assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>, > + <&dispcc DISP_CC_MDSS_VSYNC_CLK>, > + <&dispcc DISP_CC_MDSS_AHB_CLK>; > + assigned-clock-rates = <300000000>, > + <19200000>, > + <19200000>; > + operating-points-v2 = <&mdp_opp_table>; > + power-domains = <&rpmhpd SC7280_CX>; > + > + interrupt-parent = <&mdss>; > + interrupts = <0>; > + > + status = "disabled"; > + > + mdp_opp_table: mdp-opp-table { mdp_opp_table: opp-table { > + compatible = "operating-points-v2"; > + > + opp-200000000 { > + opp-hz = /bits/ 64 <200000000>; > + required-opps = <&rpmhpd_opp_low_svs>; > + }; > + > + opp-300000000 { > + opp-hz = /bits/ 64 <300000000>; > + required-opps = <&rpmhpd_opp_svs>; > + }; > + > + opp-380000000 { > + opp-hz = /bits/ 64 <380000000>; > + required-opps = <&rpmhpd_opp_svs_l1>; > + }; > + > + opp-506666667 { > + opp-hz = /bits/ 64 <506666667>; > + required-opps = <&rpmhpd_opp_nom>; > + }; > + }; > + }; > + }; > +
On 2021-08-19 01:27, Stephen Boyd wrote: > Quoting Krishna Manikandan (2021-08-18 03:27:02) >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> index 53a21d0..fd7ff1c 100644 >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> @@ -5,6 +5,7 @@ >> * Copyright (c) 2020-2021, The Linux Foundation. All rights >> reserved. >> */ >> >> +#include <dt-bindings/clock/qcom,dispcc-sc7280.h> >> #include <dt-bindings/clock/qcom,gcc-sc7280.h> >> #include <dt-bindings/clock/qcom,rpmh.h> >> #include <dt-bindings/interconnect/qcom,sc7280.h> >> @@ -1424,6 +1425,90 @@ >> #power-domain-cells = <1>; >> }; >> >> + mdss: mdss@ae00000 { > > subsystem@ae00000 > >> + compatible = "qcom,sc7280-mdss"; >> + reg = <0 0x0ae00000 0 0x1000>; >> + reg-names = "mdss"; >> + >> + power-domains = <&dispcc >> DISP_CC_MDSS_CORE_GDSC>; >> + >> + clocks = <&gcc GCC_DISP_AHB_CLK>, >> + <&dispcc DISP_CC_MDSS_AHB_CLK>, >> + <&dispcc DISP_CC_MDSS_MDP_CLK>; >> + clock-names = "iface", "ahb", "core"; >> + >> + assigned-clocks = <&dispcc >> DISP_CC_MDSS_MDP_CLK>; >> + assigned-clock-rates = <300000000>; >> + >> + interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-controller; >> + #interrupt-cells = <1>; >> + >> + interconnects = <&mmss_noc MASTER_MDP0 0 >> &mc_virt SLAVE_EBI1 0>; >> + interconnect-names = "mdp0-mem"; >> + >> + iommus = <&apps_smmu 0x900 0x402>; >> + >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges; >> + >> + status = "disabled"; >> + >> + mdp: mdp@ae01000 { > > display-controller@ae01000 Stephen, In the current driver code, there is a substring comparison for "mdp" in device node name as part of probe sequence. If "mdp" is not present in the node name, it will return an error resulting in probe failure. Can we continue using mdp as nodename instead of display controller? Thanks, Krishna > >> + compatible = "qcom,sc7280-dpu"; >> + reg = <0 0x0ae01000 0 0x8f030>, >> + <0 0x0aeb0000 0 0x2008>; >> + reg-names = "mdp", "vbif"; >> + >> + clocks = <&gcc GCC_DISP_HF_AXI_CLK>, >> + <&gcc GCC_DISP_SF_AXI_CLK>, >> + <&dispcc >> DISP_CC_MDSS_AHB_CLK>, >> + <&dispcc >> DISP_CC_MDSS_MDP_LUT_CLK>, >> + <&dispcc >> DISP_CC_MDSS_MDP_CLK>, >> + <&dispcc >> DISP_CC_MDSS_VSYNC_CLK>; >> + clock-names = "bus", "nrt_bus", >> "iface", "lut", "core", >> + "vsync"; > > One line per string please. > >> + assigned-clocks = <&dispcc >> DISP_CC_MDSS_MDP_CLK>, >> + <&dispcc >> DISP_CC_MDSS_VSYNC_CLK>, >> + <&dispcc >> DISP_CC_MDSS_AHB_CLK>; >> + assigned-clock-rates = <300000000>, >> + <19200000>, >> + <19200000>; >> + operating-points-v2 = >> <&mdp_opp_table>; >> + power-domains = <&rpmhpd SC7280_CX>; >> + >> + interrupt-parent = <&mdss>; >> + interrupts = <0>; >> + >> + status = "disabled"; >> + >> + mdp_opp_table: mdp-opp-table { > > mdp_opp_table: opp-table { > >> + compatible = >> "operating-points-v2"; >> + >> + opp-200000000 { >> + opp-hz = /bits/ 64 >> <200000000>; >> + required-opps = >> <&rpmhpd_opp_low_svs>; >> + }; >> + >> + opp-300000000 { >> + opp-hz = /bits/ 64 >> <300000000>; >> + required-opps = >> <&rpmhpd_opp_svs>; >> + }; >> + >> + opp-380000000 { >> + opp-hz = /bits/ 64 >> <380000000>; >> + required-opps = >> <&rpmhpd_opp_svs_l1>; >> + }; >> + >> + opp-506666667 { >> + opp-hz = /bits/ 64 >> <506666667>; >> + required-opps = >> <&rpmhpd_opp_nom>; >> + }; >> + }; >> + }; >> + }; >> +
Quoting mkrishn@codeaurora.org (2021-09-30 04:56:59) > On 2021-08-19 01:27, Stephen Boyd wrote: > > Quoting Krishna Manikandan (2021-08-18 03:27:02) > >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi > >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi > >> index 53a21d0..fd7ff1c 100644 > >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > >> + > >> + status = "disabled"; > >> + > >> + mdp: mdp@ae01000 { > > > > display-controller@ae01000 > > Stephen, > In the current driver code, there is a substring comparison for "mdp" > in device node name as part of probe sequence. If "mdp" is not present > in the node name, it will > return an error resulting in probe failure. Can we continue using mdp > as nodename instead of display controller? > Can we fix the driver to not look for node names and look for compatible strings instead? It took me a minute to find compare_name_mdp() in drivers/gpu/drm/msm/msm_drv.c to understand what you're talking about. Perhaps looking for qcom,mdp5 in there will be sufficient instead of looking at the node name.
On 2021-09-30 23:28, Stephen Boyd wrote: > Quoting mkrishn@codeaurora.org (2021-09-30 04:56:59) >> On 2021-08-19 01:27, Stephen Boyd wrote: >> > Quoting Krishna Manikandan (2021-08-18 03:27:02) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> index 53a21d0..fd7ff1c 100644 >> >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> + >> >> + status = "disabled"; >> >> + >> >> + mdp: mdp@ae01000 { >> > >> > display-controller@ae01000 >> >> Stephen, >> In the current driver code, there is a substring comparison for >> "mdp" >> in device node name as part of probe sequence. If "mdp" is not present >> in the node name, it will >> return an error resulting in probe failure. Can we continue using >> mdp >> as nodename instead of display controller? >> > > Can we fix the driver to not look for node names and look for > compatible > strings instead? It took me a minute to find compare_name_mdp() in > drivers/gpu/drm/msm/msm_drv.c to understand what you're talking about. > Perhaps looking for qcom,mdp5 in there will be sufficient instead of > looking at the node name. Sure Stephen. I will make the necessary changes in msm_drv.c to look for compatible string instead of node name. Can I include these two changes (changing mdp--> display controller and msm_drv.c changes) in a separate series ? Thanks, Krishna
Quoting mkrishn@codeaurora.org (2021-09-30 23:39:07) > On 2021-09-30 23:28, Stephen Boyd wrote: > > Quoting mkrishn@codeaurora.org (2021-09-30 04:56:59) > >> On 2021-08-19 01:27, Stephen Boyd wrote: > >> > Quoting Krishna Manikandan (2021-08-18 03:27:02) > >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi > >> >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi > >> >> index 53a21d0..fd7ff1c 100644 > >> >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > >> >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > >> >> + > >> >> + status = "disabled"; > >> >> + > >> >> + mdp: mdp@ae01000 { > >> > > >> > display-controller@ae01000 > >> > >> Stephen, > >> In the current driver code, there is a substring comparison for > >> "mdp" > >> in device node name as part of probe sequence. If "mdp" is not present > >> in the node name, it will > >> return an error resulting in probe failure. Can we continue using > >> mdp > >> as nodename instead of display controller? > >> > > > > Can we fix the driver to not look for node names and look for > > compatible > > strings instead? It took me a minute to find compare_name_mdp() in > > drivers/gpu/drm/msm/msm_drv.c to understand what you're talking about. > > Perhaps looking for qcom,mdp5 in there will be sufficient instead of > > looking at the node name. > > Sure Stephen. I will make the necessary changes in msm_drv.c to look for > compatible string instead of node name. > Can I include these two changes (changing mdp--> display controller and > msm_drv.c changes) in a separate series ? > Sure. So you'll send the drm driver change now and we'll get the DT change after that with the more generic node name?
On 2021-10-05 07:21, Stephen Boyd wrote: > Quoting mkrishn@codeaurora.org (2021-09-30 23:39:07) >> On 2021-09-30 23:28, Stephen Boyd wrote: >> > Quoting mkrishn@codeaurora.org (2021-09-30 04:56:59) >> >> On 2021-08-19 01:27, Stephen Boyd wrote: >> >> > Quoting Krishna Manikandan (2021-08-18 03:27:02) >> >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> >> index 53a21d0..fd7ff1c 100644 >> >> >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi >> >> >> + >> >> >> + status = "disabled"; >> >> >> + >> >> >> + mdp: mdp@ae01000 { >> >> > >> >> > display-controller@ae01000 >> >> >> >> Stephen, >> >> In the current driver code, there is a substring comparison for >> >> "mdp" >> >> in device node name as part of probe sequence. If "mdp" is not present >> >> in the node name, it will >> >> return an error resulting in probe failure. Can we continue using >> >> mdp >> >> as nodename instead of display controller? >> >> >> > >> > Can we fix the driver to not look for node names and look for >> > compatible >> > strings instead? It took me a minute to find compare_name_mdp() in >> > drivers/gpu/drm/msm/msm_drv.c to understand what you're talking about. >> > Perhaps looking for qcom,mdp5 in there will be sufficient instead of >> > looking at the node name. >> >> Sure Stephen. I will make the necessary changes in msm_drv.c to look >> for >> compatible string instead of node name. >> Can I include these two changes (changing mdp--> display controller >> and >> msm_drv.c changes) in a separate series ? >> > > Sure. So you'll send the drm driver change now and we'll get the DT > change after that with the more generic node name? Yes Stephen.I have raised the change to fix the driver issue. https://patchwork.kernel.org/project/linux-arm-msm/patch/1633427071-19523-1-git-send-email-mkrishn@codeaurora.org/ Regards, Krishna
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi index 53a21d0..fd7ff1c 100644 --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi @@ -5,6 +5,7 @@ * Copyright (c) 2020-2021, The Linux Foundation. All rights reserved. */ +#include <dt-bindings/clock/qcom,dispcc-sc7280.h> #include <dt-bindings/clock/qcom,gcc-sc7280.h> #include <dt-bindings/clock/qcom,rpmh.h> #include <dt-bindings/interconnect/qcom,sc7280.h> @@ -1424,6 +1425,90 @@ #power-domain-cells = <1>; }; + mdss: mdss@ae00000 { + compatible = "qcom,sc7280-mdss"; + reg = <0 0x0ae00000 0 0x1000>; + reg-names = "mdss"; + + power-domains = <&dispcc DISP_CC_MDSS_CORE_GDSC>; + + clocks = <&gcc GCC_DISP_AHB_CLK>, + <&dispcc DISP_CC_MDSS_AHB_CLK>, + <&dispcc DISP_CC_MDSS_MDP_CLK>; + clock-names = "iface", "ahb", "core"; + + assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>; + assigned-clock-rates = <300000000>; + + interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>; + interrupt-controller; + #interrupt-cells = <1>; + + interconnects = <&mmss_noc MASTER_MDP0 0 &mc_virt SLAVE_EBI1 0>; + interconnect-names = "mdp0-mem"; + + iommus = <&apps_smmu 0x900 0x402>; + + #address-cells = <2>; + #size-cells = <2>; + ranges; + + status = "disabled"; + + mdp: mdp@ae01000 { + compatible = "qcom,sc7280-dpu"; + reg = <0 0x0ae01000 0 0x8f030>, + <0 0x0aeb0000 0 0x2008>; + reg-names = "mdp", "vbif"; + + clocks = <&gcc GCC_DISP_HF_AXI_CLK>, + <&gcc GCC_DISP_SF_AXI_CLK>, + <&dispcc DISP_CC_MDSS_AHB_CLK>, + <&dispcc DISP_CC_MDSS_MDP_LUT_CLK>, + <&dispcc DISP_CC_MDSS_MDP_CLK>, + <&dispcc DISP_CC_MDSS_VSYNC_CLK>; + clock-names = "bus", "nrt_bus", "iface", "lut", "core", + "vsync"; + assigned-clocks = <&dispcc DISP_CC_MDSS_MDP_CLK>, + <&dispcc DISP_CC_MDSS_VSYNC_CLK>, + <&dispcc DISP_CC_MDSS_AHB_CLK>; + assigned-clock-rates = <300000000>, + <19200000>, + <19200000>; + operating-points-v2 = <&mdp_opp_table>; + power-domains = <&rpmhpd SC7280_CX>; + + interrupt-parent = <&mdss>; + interrupts = <0>; + + status = "disabled"; + + mdp_opp_table: mdp-opp-table { + compatible = "operating-points-v2"; + + opp-200000000 { + opp-hz = /bits/ 64 <200000000>; + required-opps = <&rpmhpd_opp_low_svs>; + }; + + opp-300000000 { + opp-hz = /bits/ 64 <300000000>; + required-opps = <&rpmhpd_opp_svs>; + }; + + opp-380000000 { + opp-hz = /bits/ 64 <380000000>; + required-opps = <&rpmhpd_opp_svs_l1>; + }; + + opp-506666667 { + opp-hz = /bits/ 64 <506666667>; + required-opps = <&rpmhpd_opp_nom>; + }; + }; + }; + }; + pdc: interrupt-controller@b220000 { compatible = "qcom,sc7280-pdc", "qcom,pdc"; reg = <0 0x0b220000 0 0x30000>;
Add mdss and mdp DT nodes for sc7280. Signed-off-by: Krishna Manikandan <mkrishn@codeaurora.org> --- arch/arm64/boot/dts/qcom/sc7280.dtsi | 85 ++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+)