[PATCH v5 0/3] drm/msm/mdss: reset all block resets described in DT

[PATCH v5 0/3] drm/msm/mdss: reset all block resets described in DT

[Qianhui Xu]

*** AI-assistance disclosure ***
This cover letter and the three patches were prepared with the help of an
LLM coding assistant.  The author reviewed every line, checked every
statement against the tree and his own logs, and takes full responsibility
for the content.

This is v5 of the MDSS block-reset series.  It supersedes v4
(<cover.1794558882.git.xiaqinghuan@mail.ustc.edu.cn>) and changes only the
last commit message, in response to the review of v4 3/3
(<8a0c75949fcf428a2163a2e63b27fcf08d3b4f9c.1794558882.git.xiaqinghuan@mail.ustc.edu.cn>).

1/3 and 2/3 are unchanged from v4.  3/3 keeps a byte-identical diff; only its
commit message is rewritten.

v4 said that dispcc provides "a block reset for the MDSS core INT2 interrupt
controller" and that INT2 "is part of the same block, not a separate
device".  Review pointed out that 0xa000 is described by dispcc as its own
domain: it has its own GDSC (mdss_int2_gdsc) and its own clock family
(disp_cc_mdss_mdp1_clk, disp_cc_mdss_ahb1_clk, ...), and the mdss node does
not attach that domain (power-domains is MDSS_GDSC only, and the clocks list
has no mdp1 entries).  That reading of the code is correct, so v5 no longer
calls this an interrupt controller reset and no longer claims it is the same
block.

What v5 states instead is only what is observable:

  - dispcc provides two block resets for the MDSS core area (0x8000 and
    0xa000), and the second one belongs to a domain dispcc describes
    separately;
  - firmware hands over with the display hardware still running, so the MDSS
    reset returns the group to a known state before display hardware is
    programmed;
  - on this board, resetting only DISP_CC_MDSS_CORE_BCR leaves the display
    intermittently broken at cold boot (command DMA timeouts followed by a
    panel init failure, 5 of 9 cold PORs), while asserting the group gave 15
    clean boots out of 15.

The message also records the limits of that measurement: the 15/15 figure
came from the three-reset shape that still included RSCC, it has not been
repeated for this two-reset shape, and the second reset was not isolated as a
single variable.  There is no datasheet-level confirmation that the INTR2
group and the 0xa000 domain are the same hardware block; the evidence for
that is naming (dispcc's ..._CORE_INT2_BCR, MDSS_INT2_GDSC; MDP TOP's INTR2)
plus structure (same MDSS core, separate BCR/GDSC inside it).

All three Reviewed-by tags are carried over: Abel Vesa, Dmitry Baryshkov, and
Konrad Dybcio (the last one received on v4 3/3).

If the un-attached-domain concern is considered disqualifying, the
alternative is to drop 3/3 and keep only 2/3 -- the array-API change, which is
uncontroversial.  I kept 3/3 because the failing cold boot is reproducible on
this hardware and the group is what tracks with it, but I am happy to send a
v6 without it if that is preferred.

Gates: pristine v7.3-rc7 (24a19b953e) git am 3/3 clean; checkpatch --strict
3/3 at 0 errors, 0 warnings, 0 checks; code and DT diffs are identical to v4,
so v4's dt_binding_check and W=1 results carry over unchanged.

Qianhui Xu (3):
  dt-bindings: display: msm: mdss-common: describe additional block
    resets
  drm/msm/mdss: reset all block resets described in DT
  arm64: dts: qcom: sm8750: describe the second MDSS block reset

 .../devicetree/bindings/display/msm/mdss-common.yaml         | 4 +++-
 .../devicetree/bindings/display/msm/qcom,sm8750-mdss.yaml    | 3 ++-
 arch/arm64/boot/dts/qcom/sm8750.dtsi                         | 3 ++-
 drivers/gpu/drm/msm/msm_mdss.c                               | 5 ++---
 4 files changed, 9 insertions(+), 6 deletions(-)

-- 
2.53.0

[PATCH 1/3] dt-bindings: display: msm: mdss-common: describe additional block resets

[Qianhui Xu]

Some platforms provide more than one reset for the MDSS hardware block:
beside the MDSS core reset, the SM8750 display clock controller also
exposes a block reset for the MDSS core INT2 interrupt controller.

Allow the resets property to list one or two resets, keeping the MDSS
core reset as the first entry so that existing device trees describing
a single reset remain valid.

Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
---
 .../devicetree/bindings/display/msm/mdss-common.yaml          | 4 +++-
 .../devicetree/bindings/display/msm/qcom,sm8750-mdss.yaml     | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml
index c6305a6e03..51ef4cc74e 100644
--- a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml
+++ b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml
@@ -84,8 +84,10 @@ properties:
           - const: cpu-cfg
 
   resets:
+    minItems: 1
     items:
-      - description: MDSS_CORE reset
+      - description: MDSS core reset
+      - description: MDSS core INT2 interrupt controller reset
 
   memory-region:
     maxItems: 1
diff --git a/Documentation/devicetree/bindings/display/msm/qcom,sm8750-mdss.yaml b/Documentation/devicetree/bindings/display/msm/qcom,sm8750-mdss.yaml
index 46dc0d28da..729de45ec8 100644
--- a/Documentation/devicetree/bindings/display/msm/qcom,sm8750-mdss.yaml
+++ b/Documentation/devicetree/bindings/display/msm/qcom,sm8750-mdss.yaml
@@ -101,7 +101,8 @@ examples:
             interconnect-names = "mdp0-mem",
                                  "cpu-cfg";
 
-            resets = <&disp_cc_mdss_core_bcr>;
+            resets = <&disp_cc_mdss_core_bcr>,
+                     <&disp_cc_mdss_core_int2_bcr>;
 
             power-domains = <&mdss_gdsc>;
 
-- 
2.53.0

[PATCH 2/3] drm/msm/mdss: reset all block resets described in DT

[Qianhui Xu]

msm_mdss_reset() acquires only the first reset listed in the MDSS
device node.  Platforms can describe more than one MDSS block reset: the
SM8750 display clock controller also provides a reset for the MDSS core
INT2 interrupt controller, which is left out of reset when the
bootloader hands over with the display block still running.

Switch to the reset control array API so that every reset listed in the
device node is asserted and released as one group.  Devices without a
reset in DT are unaffected, and for devices describing a single reset
the assert/deassert sequence is unchanged.

Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
---
 drivers/gpu/drm/msm/msm_mdss.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
index 9087c4b290..84b8b4bc9b 100644
--- a/drivers/gpu/drm/msm/msm_mdss.c
+++ b/drivers/gpu/drm/msm/msm_mdss.c
@@ -310,7 +310,8 @@ static int msm_mdss_reset(struct device *dev)
 {
 	struct reset_control *reset;
 
-	reset = reset_control_get_optional_exclusive(dev, NULL);
+	reset = devm_reset_control_array_get(dev,
+					     RESET_CONTROL_OPTIONAL_EXCLUSIVE);
 	if (!reset) {
 		/* Optional reset not specified */
 		return 0;
@@ -327,8 +328,6 @@ static int msm_mdss_reset(struct device *dev)
 	msleep(20);
 	reset_control_deassert(reset);
 
-	reset_control_put(reset);
-
 	return 0;
 }
 
-- 
2.53.0

[PATCH 3/3] arm64: dts: qcom: sm8750: describe the second MDSS block reset

[Qianhui Xu]

The SM8750 display clock controller provides two block resets for the
MDSS core area: DISP_CC_MDSS_CORE_BCR and DISP_CC_MDSS_CORE_INT2_BCR,
at 0x8000 and 0xa000.  The second one belongs to a domain that dispcc
describes with its own GDSC (mdss_int2_gdsc) and its own clock family
(disp_cc_mdss_mdp1_clk, disp_cc_mdss_ahb1_clk, ...).

Bootloaders hand over with the display hardware still running, so the
MDSS reset should return the whole group to a known state before any
display hardware is programmed.  Describe both resets so that the
array-API change in the previous patch asserts and releases the group
together.

On this board, resetting only DISP_CC_MDSS_CORE_BCR leaves the display
intermittently broken at cold boot (command DMA timeouts followed by a
panel init failure, 5 of 9 cold PORs), while asserting the group gave
15 clean boots out of 15.  That measurement used the three-reset shape
that also included RSCC; it has not been repeated for this two-reset
shape, and the second reset has not been isolated as a single variable.
What is observable is that resetting the group correlates with a working
display at cold boot.

Reviewed-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
---
 arch/arm64/boot/dts/qcom/sm8750.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
index dd738d13df..e8e2832f91 100644
--- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
@@ -3071,7 +3071,8 @@ mdss: display-subsystem@ae00000 {
 				 <&gcc GCC_DISP_HF_AXI_CLK>,
 				 <&dispcc DISP_CC_MDSS_MDP_CLK>;
 
-			resets = <&dispcc DISP_CC_MDSS_CORE_BCR>;
+			resets = <&dispcc DISP_CC_MDSS_CORE_BCR>,
+				 <&dispcc DISP_CC_MDSS_CORE_INT2_BCR>;
 
 			interconnects = <&mmss_noc MASTER_MDP QCOM_ICC_TAG_ALWAYS
 					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
-- 
2.53.0

[PATCH 3/3] arm64: dts: qcom: sm8750: describe the second MDSS block reset

[Qianhui Xu]

The SM8750 display clock controller provides two block resets for the
MDSS core area: DISP_CC_MDSS_CORE_BCR and DISP_CC_MDSS_CORE_INT2_BCR,
at 0x8000 and 0xa000.  The second one belongs to a domain that dispcc
describes with its own GDSC (mdss_int2_gdsc) and its own clock family
(disp_cc_mdss_mdp1_clk, disp_cc_mdss_ahb1_clk, ...).

Bootloaders hand over with the display hardware still running, so the
MDSS reset should return the whole group to a known state before any
display hardware is programmed.  Describe both resets so that the
array-API change in the previous patch asserts and releases the group
together.

On this board, resetting only DISP_CC_MDSS_CORE_BCR leaves the display
intermittently broken at cold boot (command DMA timeouts followed by a
panel init failure, 5 of 9 cold PORs), while asserting the group gave
15 clean boots out of 15.  That measurement used the three-reset shape
that also included RSCC; it has not been repeated for this two-reset
shape, and the second reset has not been isolated as a single variable.
What is observable is that resetting the group correlates with a working
display at cold boot.

Reviewed-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
---
 arch/arm64/boot/dts/qcom/sm8750.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
index dd738d13df..e8e2832f91 100644
--- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
@@ -3071,7 +3071,8 @@ mdss: display-subsystem@ae00000 {
 				 <&gcc GCC_DISP_HF_AXI_CLK>,
 				 <&dispcc DISP_CC_MDSS_MDP_CLK>;
 
-			resets = <&dispcc DISP_CC_MDSS_CORE_BCR>;
+			resets = <&dispcc DISP_CC_MDSS_CORE_BCR>,
+				 <&dispcc DISP_CC_MDSS_CORE_INT2_BCR>;
 
 			interconnects = <&mmss_noc MASTER_MDP QCOM_ICC_TAG_ALWAYS
 					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
-- 
2.53.0

Re: [PATCH 3/3] arm64: dts: qcom: sm8750: describe the second MDSS block reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] arm64: dts: qcom: sm8750: unattached
  DISP_CC_MDSS_CORE_INT2_BCR reset on mdss
--

commit 048c5dc3c561337c4cf2b84db1d42eb1d5cdd378
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

arm64: dts: qcom: sm8750: describe the second MDSS block reset

This commit adds DISP_CC_MDSS_CORE_INT2_BCR as a second reset to the
SM8750 MDSS device tree node.

> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> index cf947acc57602..0f3e649787315 100644
> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> @@ -3359,7 +3359,8 @@ mdss: display-subsystem@ae00000 {
>  				 <&gcc GCC_DISP_HF_AXI_CLK>,
>  				 <&dispcc DISP_CC_MDSS_MDP_CLK>;
>  
> -			resets = <&dispcc DISP_CC_MDSS_CORE_BCR>;
> +			resets = <&dispcc DISP_CC_MDSS_CORE_BCR>,
> +				 <&dispcc DISP_CC_MDSS_CORE_INT2_BCR>;

[Severity: Medium]
Should the mdss node assert and deassert DISP_CC_MDSS_CORE_INT2_BCR when its
corresponding power domain and clocks are not attached to the device?

In drivers/clk/qcom/dispcc-sm8750.c, DISP_CC_MDSS_CORE_INT2_BCR at 0xa000 is
the block reset for a separate secondary domain with its own power domain
(MDSS_INT2_GDSC at 0xb000) and clock branches (disp_cc_mdss_mdp1_clk,
disp_cc_mdss_mdp_lut1_clk, disp_cc_mdss_vsync1_clk, and
disp_cc_mdss_ahb1_clk).  The mdss and mdss_mdp nodes in sm8750.dtsi only
attach MDSS_GDSC and the primary MDP0 clocks, so msm_mdss_reset() toggles a
reset for a domain whose GDSC and clocks are neither attached nor enabled.

In addition, while the commit message notes that DISP_CC_MDSS_CORE_INT2_BCR
belongs to the mdss_int2_gdsc domain and that this two-reset configuration
has not been tested on hardware, the companion binding in
Documentation/devicetree/bindings/display/msm/mdss-common.yaml still
describes the second reset as an MDSS core INT2 interrupt controller reset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113155606.1677313-1-xiaqinghuan@mail.ustc.edu.cn?part=3

Re: [PATCH 3/3] arm64: dts: qcom: sm8750: describe the second MDSS block reset

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] arm64: dts: qcom: sm8750: unattached INT2 domain reset on
  mdss
--

commit b3b8b6c811f21fdd572edc7ec2e32709cdaa973b
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

arm64: dts: qcom: sm8750: describe the second MDSS block reset

This commit adds DISP_CC_MDSS_CORE_INT2_BCR to the resets property of the
mdss display-subsystem node in the SM8750 device tree.

> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> index cf947acc57602..0f3e649787315 100644
> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> @@ -3359,7 +3359,8 @@ mdss: display-subsystem@ae00000 {
>  				 <&gcc GCC_DISP_HF_AXI_CLK>,
>  				 <&dispcc DISP_CC_MDSS_MDP_CLK>;
>  
> -			resets = <&dispcc DISP_CC_MDSS_CORE_BCR>;
> +			resets = <&dispcc DISP_CC_MDSS_CORE_BCR>,
> +				 <&dispcc DISP_CC_MDSS_CORE_INT2_BCR>;

[Severity: Medium]
Should the mdss node assert DISP_CC_MDSS_CORE_INT2_BCR when it does not
attach the corresponding MDSS_INT2_GDSC power domain or clocks?

In drivers/clk/qcom/dispcc-sm8750.c, DISP_CC_MDSS_CORE_INT2_BCR (at 0xa000)
is the block reset for the secondary MDSS_CORE_INT2 domain governed by
mdss_int2_gdsc (at 0xb000) and the disp_cc_mdss_*1_clk branches.  Here,
mdss only attaches MDSS_GDSC and the primary MDSS_CORE clocks, so
mdss_probe() -> msm_mdss_init() -> msm_mdss_reset() will reset an
unattached power and clock domain.

In addition, commit "dt-bindings: display: msm: mdss-common: describe
additional block resets" in this series still documents this second reset
entry in Documentation/devicetree/bindings/display/msm/mdss-common.yaml as
"MDSS core INT2 interrupt controller reset", which contradicts the commit
message here describing it as a separate GDSC and clock domain.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113155635.1677541-1-xiaqinghuan@mail.ustc.edu.cn?part=3

Re: [PATCH 2/3] drm/msm/mdss: reset all block resets described in DT

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] drm/msm/mdss: inaccurate INT2 reset description in
  msm_mdss_reset() commit
--

commit 8dd3e2454eeeb0b73b84226f92cb892017ce9f66
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

drm/msm/mdss: reset all block resets described in DT

This commit switches msm_mdss_reset() to devm_reset_control_array_get() so
that all resets listed in the MDSS device node are asserted and deasserted
together.

> msm_mdss_reset() acquires only the first reset listed in the MDSS
> device node.  Platforms can describe more than one MDSS block reset: the
> SM8750 display clock controller also provides a reset for the MDSS core
> INT2 interrupt controller, which is left out of reset when the
> bootloader hands over with the display block still running.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index 259d362475c81..e10185ef21677 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -325,7 +325,8 @@ static int msm_mdss_reset(struct device *dev)
>  {
>  	struct reset_control *reset;
>  
> -	reset = reset_control_get_optional_exclusive(dev, NULL);
> +	reset = devm_reset_control_array_get(dev,
> +					     RESET_CONTROL_OPTIONAL_EXCLUSIVE);
>  	if (!reset) {
>  		/* Optional reset not specified */
>  		return 0;
> @@ -342,8 +343,6 @@ static int msm_mdss_reset(struct device *dev)
>  	msleep(20);
>  	reset_control_deassert(reset);
>  
> -	reset_control_put(reset);
> -
>  	return 0;
>  }

[Severity: Low]
As reported by Dmitry Baryshkov on v3 [1], is the commit message accurate in
describing DISP_CC_MDSS_CORE_INT2_BCR on SM8750 as a reset for an INT2
interrupt controller?

In drivers/clk/qcom/dispcc-sm8750.c, DISP_CC_MDSS_CORE_INT2_BCR (0xa000) is
the block reset for the second internal MDSS core domain paired with
mdss_int2_gdsc (0xb000) and the disp_cc_mdss_mdp1_clk,
disp_cc_mdss_mdp_lut1_clk, disp_cc_mdss_vsync1_clk, and
disp_cc_mdss_ahb1_clk branch clocks, as updated in commit "arm64: dts: qcom:
sm8750: describe the second MDSS block reset".

In addition, switching from reset_control_get_optional_exclusive() and
reset_control_put() to devm_reset_control_array_get() keeps the exclusive
reset controls acquired for the lifetime of the msm_mdss device rather than
releasing them after the boot reset pulse.

[1] https://lore.kernel.org/r/awb7bslhrhemsgvu7ubaejvurk6fecotgvqlwtvsoe2benqdgq@tvh2sfjbuzyu

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113155606.1677313-1-xiaqinghuan@mail.ustc.edu.cn?part=2

Re: [PATCH 1/3] dt-bindings: display: msm: mdss-common: describe additional block resets

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] dt-bindings: display: msm: misdescribed MDSS_CORE_INT2 reset in
  mdss-common
--

commit 022650a3f28469dcfe73a6138dee6ab78a21185f
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

dt-bindings: display: msm: mdss-common: describe additional block resets

This commit updates the common MDSS Device Tree binding schema to allow a
second reset entry and adds DISP_CC_MDSS_CORE_INT2_BCR to the SM8750 MDSS
binding example.

> diff --git a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml
> index c6305a6e03346..51ef4cc74e818 100644
> --- a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml
> @@ -84,8 +84,10 @@ properties:
>            - const: cpu-cfg
>  
>    resets:
> +    minItems: 1
>      items:
> -      - description: MDSS_CORE reset
> +      - description: MDSS core reset
> +      - description: MDSS core INT2 interrupt controller reset

[Severity: Low]
Is "MDSS core INT2 interrupt controller reset" an accurate description of
DISP_CC_MDSS_CORE_INT2_BCR in the binding and commit message?

In drivers/clk/qcom/dispcc-sm8750.c, DISP_CC_MDSS_CORE_INT2_BCR (0xa000) is
paired with MDSS_INT2_GDSC (0xb000) and controls the secondary internal MDSS
core clock and power domain (disp_cc_mdss_mdp1_clk,
disp_cc_mdss_mdp_lut1_clk, disp_cc_mdss_vsync1_clk and
disp_cc_mdss_ahb1_clk), while the MDSS interrupt controller registers reside
in the primary MDSS core MMIO space.  Later in the series, commit "arm64: dts:
qcom: sm8750: describe the second MDSS block reset" already describes
DISP_CC_MDSS_CORE_INT2_BCR as belonging to the mdss_int2_gdsc and *1_clk
domain rather than an interrupt controller.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113155606.1677313-1-xiaqinghuan@mail.ustc.edu.cn?part=1