[PATCH 0/4] drm/tidss: Fixes data edge sampling

[PATCH 0/4] drm/tidss: Fixes data edge sampling

[Louis Chauvet]

Currently the driver only configure the data edge sampling partially. The 
AM62 require it to be configured in two distincts registers: one in tidss 
and one in the general device registers.

Introduce a new dt property to link the proper syscon node from the main 
device registers into the tidss driver.

Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
---
Cc: stable@vger.kernel.org

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>

---
Louis Chauvet (4):
      dt-bindings: display: ti,am65x-dss: Add clk property for data edge synchronization
      dt-bindings: mfd: syscon: Add ti,am625-dss-clk-ctrl
      arm64: dts: ti: k3-am62-main: Add tidss clk-ctrl property
      drm/tidss: Fix sampling edge configuration

 .../devicetree/bindings/display/ti/ti,am65x-dss.yaml       |  6 ++++++
 Documentation/devicetree/bindings/mfd/syscon.yaml          |  3 ++-
 arch/arm64/boot/dts/ti/k3-am62-main.dtsi                   |  6 ++++++
 drivers/gpu/drm/tidss/tidss_dispc.c                        | 14 ++++++++++++++
 4 files changed, 28 insertions(+), 1 deletion(-)
---
base-commit: 85c23f28905cf20a86ceec3cfd7a0a5572c9eb13
change-id: 20250730-fix-edge-handling-9123f7438910

Best regards,
-- 
Louis Chauvet <louis.chauvet@bootlin.com>

[PATCH 1/4] dt-bindings: display: ti,am65x-dss: Add clk property for data edge synchronization

[Louis Chauvet]

The dt-bindings for the display, specifically ti,am65x-dss, need to
include a clock property for data edge synchronization. The current
implementation does not correctly apply the data edge sampling property.

To address this, synchronization of writes to two different registers is
required: one in the TIDSS IP (which is already described in the tidss
node) and one is in the Memory Mapped Control Register Modules (added by
the previous commit).

As the Memory Mapped Control Register Modules is located in a different
IP, we need to use a phandle to write values in its registers.

Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>

---

Cc: stable@vger.kernel.org
---
 Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index 361e9cae6896c1f4d7fa1ec47a6e3a73bca2b102..b9a373b569170332f671416eb7bbc0c83f7b5ea6 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -133,6 +133,12 @@ properties:
       and OLDI_CLK_IO_CTRL registers. This property is needed for OLDI
       interface to work.
 
+  ti,clk-ctrl:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandle to syscon device node mapping CFG0_CLK_CTRL registers.
+      This property is needed for proper data sampling edge.
+
   max-memory-bandwidth:
     $ref: /schemas/types.yaml#/definitions/uint32
     description:

-- 
2.50.1

Re: [PATCH 1/4] dt-bindings: display: ti,am65x-dss: Add clk property for data edge synchronization

[Rob Herring]

On Wed, Jul 30, 2025 at 07:02:44PM +0200, Louis Chauvet wrote:
> The dt-bindings for the display, specifically ti,am65x-dss, need to
> include a clock property for data edge synchronization. The current
> implementation does not correctly apply the data edge sampling property.
> 
> To address this, synchronization of writes to two different registers is
> required: one in the TIDSS IP (which is already described in the tidss
> node) and one is in the Memory Mapped Control Register Modules (added by
> the previous commit).
> 
> As the Memory Mapped Control Register Modules is located in a different
> IP, we need to use a phandle to write values in its registers.

You can always just lookup the target node by compatible. Then you don't 
need a DT update to solve your problem.

> 
> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> 
> ---
> 
> Cc: stable@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index 361e9cae6896c1f4d7fa1ec47a6e3a73bca2b102..b9a373b569170332f671416eb7bbc0c83f7b5ea6 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -133,6 +133,12 @@ properties:
>        and OLDI_CLK_IO_CTRL registers. This property is needed for OLDI
>        interface to work.
>  
> +  ti,clk-ctrl:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to syscon device node mapping CFG0_CLK_CTRL registers.
> +      This property is needed for proper data sampling edge.
> +
>    max-memory-bandwidth:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description:
> 
> -- 
> 2.50.1
> 

Re: [PATCH 1/4] dt-bindings: display: ti,am65x-dss: Add clk property for data edge synchronization

[Louis Chauvet]

Le 31/07/2025 à 01:56, Rob Herring a écrit :
> On Wed, Jul 30, 2025 at 07:02:44PM +0200, Louis Chauvet wrote:
>> The dt-bindings for the display, specifically ti,am65x-dss, need to
>> include a clock property for data edge synchronization. The current
>> implementation does not correctly apply the data edge sampling property.
>>
>> To address this, synchronization of writes to two different registers is
>> required: one in the TIDSS IP (which is already described in the tidss
>> node) and one is in the Memory Mapped Control Register Modules (added by
>> the previous commit).
>>
>> As the Memory Mapped Control Register Modules is located in a different
>> IP, we need to use a phandle to write values in its registers.
> 
> You can always just lookup the target node by compatible. Then you don't
> need a DT update to solve your problem.

I just followed what was already done for dss_oldi_io_ctrl/ti,oldi-io-ctrl.

I will use syscon_regmap_lookup_by_compatible for the v2.

>>
>> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>
>> ---
>>
>> Cc: stable@vger.kernel.org
>> ---
>>   Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> index 361e9cae6896c1f4d7fa1ec47a6e3a73bca2b102..b9a373b569170332f671416eb7bbc0c83f7b5ea6 100644
>> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
>> @@ -133,6 +133,12 @@ properties:
>>         and OLDI_CLK_IO_CTRL registers. This property is needed for OLDI
>>         interface to work.
>>   
>> +  ti,clk-ctrl:
>> +    $ref: /schemas/types.yaml#/definitions/phandle
>> +    description:
>> +      phandle to syscon device node mapping CFG0_CLK_CTRL registers.
>> +      This property is needed for proper data sampling edge.
>> +
>>     max-memory-bandwidth:
>>       $ref: /schemas/types.yaml#/definitions/uint32
>>       description:
>>
>> -- 
>> 2.50.1
>>

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 2/4] dt-bindings: mfd: syscon: Add ti,am625-dss-clk-ctrl

[Louis Chauvet]

The dt-bindings for the multi-function device (mfd) syscon need to include
ti,am625-dss-clk-ctrl. On AM625 chips, the display controller (tidss) has
external registers to control certain clock properties. These registers
are located in the device configuration registers, so they need to be
declared using syscon. They will later be used with a phandle in the tidss
node.

Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---

Cc: stable@vger.kernel.org
---
 Documentation/devicetree/bindings/mfd/syscon.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
index 27672adeb1fedb7c81b8ae86c35f4f3b26d5516f..afe4a2a19591e90c850c05ef5888f18bdb64eac9 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.yaml
+++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
@@ -121,6 +121,7 @@ select:
           - ti,am62-opp-efuse-table
           - ti,am62-usb-phy-ctrl
           - ti,am625-dss-oldi-io-ctrl
+          - ti,am625-dss-clk-ctrl
           - ti,am62p-cpsw-mac-efuse
           - ti,am654-dss-oldi-io-ctrl
           - ti,j784s4-acspcie-proxy-ctrl
@@ -228,6 +229,7 @@ properties:
           - ti,am62-opp-efuse-table
           - ti,am62-usb-phy-ctrl
           - ti,am625-dss-oldi-io-ctrl
+          - ti,am625-dss-clk-ctrl
           - ti,am62p-cpsw-mac-efuse
           - ti,am654-dss-oldi-io-ctrl
           - ti,j784s4-acspcie-proxy-ctrl
@@ -256,4 +258,3 @@ examples:
         compatible = "allwinner,sun8i-h3-system-controller", "syscon";
         reg = <0x01c00000 0x1000>;
     };
-...

-- 
2.50.1

Re: [PATCH 2/4] dt-bindings: mfd: syscon: Add ti,am625-dss-clk-ctrl

[Krzysztof Kozlowski]

On Wed, Jul 30, 2025 at 07:02:45PM +0200, Louis Chauvet wrote:
> The dt-bindings for the multi-function device (mfd) syscon need to include
> ti,am625-dss-clk-ctrl. On AM625 chips, the display controller (tidss) has
> external registers to control certain clock properties. These registers
> are located in the device configuration registers, so they need to be
> declared using syscon. They will later be used with a phandle in the tidss
> node.

I don't understand above commit msg. You add new compatible (new device)
but entire commit msg describes something else - some sort of a fix.

> 
> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")

Heh? How? How adding a new driver needs fixes in the bindings?

This is just confusing.

> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
> 
> Cc: stable@vger.kernel.org

That's not the way to add stable tag. See stable-kernel docs or any git
log history.


> ---
>  Documentation/devicetree/bindings/mfd/syscon.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
> index 27672adeb1fedb7c81b8ae86c35f4f3b26d5516f..afe4a2a19591e90c850c05ef5888f18bdb64eac9 100644
> --- a/Documentation/devicetree/bindings/mfd/syscon.yaml
> +++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
> @@ -121,6 +121,7 @@ select:
>            - ti,am62-opp-efuse-table
>            - ti,am62-usb-phy-ctrl
>            - ti,am625-dss-oldi-io-ctrl
> +          - ti,am625-dss-clk-ctrl

Don't break the order. o > c

>            - ti,am62p-cpsw-mac-efuse
>            - ti,am654-dss-oldi-io-ctrl
>            - ti,j784s4-acspcie-proxy-ctrl
> @@ -228,6 +229,7 @@ properties:
>            - ti,am62-opp-efuse-table
>            - ti,am62-usb-phy-ctrl
>            - ti,am625-dss-oldi-io-ctrl
> +          - ti,am625-dss-clk-ctrl

Same here

I don't understand also why you are adding clock to syscon. Clock
controllers have their own bindings.

Best regards,
Krzysztof

[PATCH 3/4] arm64: dts: ti: k3-am62-main: Add tidss clk-ctrl property

[Louis Chauvet]

For am62 processors, we need to use the newly created clk-ctrl property to
properly handle data edge sampling configuration. Add them in the main
device tree.

Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---

Cc: stable@vger.kernel.org
---
 arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
index 9e0b6eee9ac77d66869915b2d7bec3e2275c03ea..d3131e6da8e70fde035d3c44716f939e8167795a 100644
--- a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
@@ -76,6 +76,11 @@ audio_refclk1: clock-controller@82e4 {
 			assigned-clock-parents = <&k3_clks 157 18>;
 			#clock-cells = <0>;
 		};
+
+		dss_clk_ctrl: dss_clk_ctrl@8300 {
+			compatible = "ti,am625-dss-clk-ctrl", "syscon";
+			reg = <0x8300 0x4>;
+		};
 	};
 
 	dmss: bus@48000000 {
@@ -787,6 +792,7 @@ dss: dss@30200000 {
 			 <&k3_clks 186 2>;
 		clock-names = "fck", "vp1", "vp2";
 		interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
+		ti,clk-ctrl = <&dss_clk_ctrl>;
 		status = "disabled";
 
 		dss_ports: ports {

-- 
2.50.1

Re: [PATCH 3/4] arm64: dts: ti: k3-am62-main: Add tidss clk-ctrl property

[Rob Herring]

On Wed, Jul 30, 2025 at 07:02:46PM +0200, Louis Chauvet wrote:
> For am62 processors, we need to use the newly created clk-ctrl property to
> properly handle data edge sampling configuration. Add them in the main
> device tree.
> 
> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
> 
> Cc: stable@vger.kernel.org
> ---
>  arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
> index 9e0b6eee9ac77d66869915b2d7bec3e2275c03ea..d3131e6da8e70fde035d3c44716f939e8167795a 100644
> --- a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
> @@ -76,6 +76,11 @@ audio_refclk1: clock-controller@82e4 {
>  			assigned-clock-parents = <&k3_clks 157 18>;
>  			#clock-cells = <0>;
>  		};
> +
> +		dss_clk_ctrl: dss_clk_ctrl@8300 {
> +			compatible = "ti,am625-dss-clk-ctrl", "syscon";
> +			reg = <0x8300 0x4>;

H/w blocks are rarely only 4 bytes of registers... Does this belong to 
some larger block. The problem with bindings defining single registers 
like this is they don't get defined until needed and you have a constant 
stream of DT updates.

> +		};
>  	};
>  
>  	dmss: bus@48000000 {
> @@ -787,6 +792,7 @@ dss: dss@30200000 {
>  			 <&k3_clks 186 2>;
>  		clock-names = "fck", "vp1", "vp2";
>  		interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> +		ti,clk-ctrl = <&dss_clk_ctrl>;
>  		status = "disabled";
>  
>  		dss_ports: ports {
> 
> -- 
> 2.50.1
> 

Re: [PATCH 3/4] arm64: dts: ti: k3-am62-main: Add tidss clk-ctrl property

[Louis Chauvet]

Le 31/07/2025 à 02:17, Rob Herring a écrit :
> On Wed, Jul 30, 2025 at 07:02:46PM +0200, Louis Chauvet wrote:
>> For am62 processors, we need to use the newly created clk-ctrl property to
>> properly handle data edge sampling configuration. Add them in the main
>> device tree.
>>
>> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>> ---
>>
>> Cc: stable@vger.kernel.org
>> ---
>>   arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
>> index 9e0b6eee9ac77d66869915b2d7bec3e2275c03ea..d3131e6da8e70fde035d3c44716f939e8167795a 100644
>> --- a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
>> +++ b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
>> @@ -76,6 +76,11 @@ audio_refclk1: clock-controller@82e4 {
>>   			assigned-clock-parents = <&k3_clks 157 18>;
>>   			#clock-cells = <0>;
>>   		};
>> +
>> +		dss_clk_ctrl: dss_clk_ctrl@8300 {
>> +			compatible = "ti,am625-dss-clk-ctrl", "syscon";
>> +			reg = <0x8300 0x4>;
> 
> H/w blocks are rarely only 4 bytes of registers... Does this belong to
> some larger block. The problem with bindings defining single registers
> like this is they don't get defined until needed and you have a constant
> stream of DT updates.

In this case, I don't think there is a "larger block". This register 
exists only because TI had issues in the display controller [1].

Here is the extract of MMR registers ([2], page 4311):

[...]
A2E4h AUDIO_REFCLK1_CTRL_PROXY <unrelated>
A300h DPI0_CLK_CTRL_PROXY <this register, 32 bits>
<nothing between 0xA304 and 0xA319>
A320h DSS_DISPC0_CLKSEL1_PROXY <an other dss clock stuff, 32 bits>
A330h MCASP0_CLKSEL_PROXY <unrelated>
[...]

I agree that DSS_DISPC_CLKSEL_PROXY is also related to DSS clock stuff, 
so may be included in the same block, but there is also a big hole in 
between. What do you think is better?

[1]:https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1228207/am62a7-rgb-display-flickering-and-pixel-issue/4687209#4687209
[2]:https://www.ti.com/lit/ug/spruiv7b/spruiv7b.pdf#page=4311

>> +		};
>>   	};
>>   
>>   	dmss: bus@48000000 {
>> @@ -787,6 +792,7 @@ dss: dss@30200000 {
>>   			 <&k3_clks 186 2>;
>>   		clock-names = "fck", "vp1", "vp2";
>>   		interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
>> +		ti,clk-ctrl = <&dss_clk_ctrl>;
>>   		status = "disabled";
>>   
>>   		dss_ports: ports {
>>
>> -- 
>> 2.50.1
>>

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 3/4] arm64: dts: ti: k3-am62-main: Add tidss clk-ctrl property

[Krzysztof Kozlowski]

On Thu, Jul 31, 2025 at 11:50:16AM +0200, Louis Chauvet wrote:
> 
> 
> Le 31/07/2025 à 02:17, Rob Herring a écrit :
> > On Wed, Jul 30, 2025 at 07:02:46PM +0200, Louis Chauvet wrote:
> > > For am62 processors, we need to use the newly created clk-ctrl property to
> > > properly handle data edge sampling configuration. Add them in the main
> > > device tree.
> > > 
> > > Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > > ---
> > > 
> > > Cc: stable@vger.kernel.org
> > > ---
> > >   arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
> > > index 9e0b6eee9ac77d66869915b2d7bec3e2275c03ea..d3131e6da8e70fde035d3c44716f939e8167795a 100644
> > > --- a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
> > > +++ b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
> > > @@ -76,6 +76,11 @@ audio_refclk1: clock-controller@82e4 {
> > >   			assigned-clock-parents = <&k3_clks 157 18>;
> > >   			#clock-cells = <0>;
> > >   		};
> > > +
> > > +		dss_clk_ctrl: dss_clk_ctrl@8300 {
> > > +			compatible = "ti,am625-dss-clk-ctrl", "syscon";
> > > +			reg = <0x8300 0x4>;
> > 
> > H/w blocks are rarely only 4 bytes of registers... Does this belong to
> > some larger block. The problem with bindings defining single registers
> > like this is they don't get defined until needed and you have a constant
> > stream of DT updates.
> 
> In this case, I don't think there is a "larger block". This register exists
> only because TI had issues in the display controller [1].
> 
> Here is the extract of MMR registers ([2], page 4311):
> 
> [...]
> A2E4h AUDIO_REFCLK1_CTRL_PROXY <unrelated>

Here is clk ctrl proxy...

> A300h DPI0_CLK_CTRL_PROXY <this register, 32 bits>

and here as well, so pretty related. This looks also close to regular
syscon and we do not define individual syscon registers as device nodes.

Best regards,
Krzysztof

Re: [PATCH 3/4] arm64: dts: ti: k3-am62-main: Add tidss clk-ctrl property

[Louis Chauvet]

Le 05/08/2025 à 08:44, Krzysztof Kozlowski a écrit :
> On Thu, Jul 31, 2025 at 11:50:16AM +0200, Louis Chauvet wrote:
>>
>>
>> Le 31/07/2025 à 02:17, Rob Herring a écrit :
>>> On Wed, Jul 30, 2025 at 07:02:46PM +0200, Louis Chauvet wrote:
>>>> For am62 processors, we need to use the newly created clk-ctrl property to
>>>> properly handle data edge sampling configuration. Add them in the main
>>>> device tree.
>>>>
>>>> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
>>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>>> ---
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>>    arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
>>>> index 9e0b6eee9ac77d66869915b2d7bec3e2275c03ea..d3131e6da8e70fde035d3c44716f939e8167795a 100644
>>>> --- a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
>>>> +++ b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
>>>> @@ -76,6 +76,11 @@ audio_refclk1: clock-controller@82e4 {
>>>>    			assigned-clock-parents = <&k3_clks 157 18>;
>>>>    			#clock-cells = <0>;
>>>>    		};
>>>> +
>>>> +		dss_clk_ctrl: dss_clk_ctrl@8300 {
>>>> +			compatible = "ti,am625-dss-clk-ctrl", "syscon";
>>>> +			reg = <0x8300 0x4>;
>>>
>>> H/w blocks are rarely only 4 bytes of registers... Does this belong to
>>> some larger block. The problem with bindings defining single registers
>>> like this is they don't get defined until needed and you have a constant
>>> stream of DT updates.
>>
>> In this case, I don't think there is a "larger block". This register exists
>> only because TI had issues in the display controller [1].
>>
>> Here is the extract of MMR registers ([2], page 4311):
>>
>> [...]
>> A2E4h AUDIO_REFCLK1_CTRL_PROXY <unrelated>
> 
> Here is clk ctrl proxy...

(Note: I linked and copied the wrong page in my previous mail, the page 
is 4309 and register addresses are 0x82e4 (audio_refclk1_ctrl), 0x8300 
(dpi0_clk_ctrl) and 0x8320 (dss_dispc_clksel1), but the issue remain the 
same)

The AUDIO_REFCLK1_CTRL is already defined in the simple-bus node, but 
with a size of 0x4 [1] and as a clock controller.

What is the correct solution in this case? Should I create a big syscon 
that overlap with audio_refclk0/1 range?

[1]:https://elixir.bootlin.com/linux/v6.16/source/arch/arm64/boot/dts/ti/k3-am62-main.dtsi#L73

>> A300h DPI0_CLK_CTRL_PROXY <this register, 32 bits>
> 
> and here as well, so pretty related. This looks also close to regular
> syscon and we do not define individual syscon registers as device nodes.

I agree this one can be included in the syscon device. Clock related 
registers starts at 0x8000 and ends at 0x8504, should I cover the whole 
range in the syscon?

I quickly looked at the other register, here is the repartition:

- 0x8000 - "normal" clock (divider + source selection)
- 0x8040 to 0x8298 - clock source selection
- 0x82e0 to 0x82e4 - clock control for audio (already implemented as 
clock driver)
- 0x8300 - the clock quirk (it seems that this is the only quirk 
register here)
- 0x8320 to 0x8500 - clock source selection

Thanks,
Louis Chauvet

> Best regards,
> Krzysztof
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 4/4] drm/tidss: Fix sampling edge configuration

[Louis Chauvet]

As stated in the AM62x Technical Reference Manual (SPRUIV7B), the data
sampling edge needs to be configured in two distinct registers: one in the
TIDSS IP and another in the memory-mapped control register modules. Since
the latter is not within the same address range, a phandle to a syscon
device is used to access the regmap.

Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>

---

Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index c0277fa36425ee1f966dccecf2b69a2d01794899..65ca7629a2e75437023bf58f8a1bddc24db5e3da 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -498,6 +498,7 @@ struct dispc_device {
 	const struct dispc_features *feat;
 
 	struct clk *fclk;
+	struct regmap *clk_ctrl;
 
 	bool is_enabled;
 
@@ -1267,6 +1268,11 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
 		       FLD_VAL(mode->vdisplay - 1, 27, 16));
 
 	VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1, 0, 0);
+
+	if (dispc->clk_ctrl) {
+		regmap_update_bits(dispc->clk_ctrl, 0, 0x100, ipc ? 0x100 : 0x000);
+		regmap_update_bits(dispc->clk_ctrl, 0, 0x200, rf ? 0x200 : 0x000);
+	}
 }
 
 void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport)
@@ -3012,6 +3018,14 @@ int dispc_init(struct tidss_device *tidss)
 
 	dispc_init_errata(dispc);
 
+	dispc->clk_ctrl = syscon_regmap_lookup_by_phandle_optional(tidss->dev->of_node,
+								   "ti,clk-ctrl");
+	if (IS_ERR(dispc->clk_ctrl)) {
+		r = dev_err_probe(dispc->dev, PTR_ERR(dispc->clk_ctrl),
+				  "DISPC: syscon_regmap_lookup_by_phandle failed.\n");
+		return r;
+	}
+
 	dispc->fourccs = devm_kcalloc(dev, ARRAY_SIZE(dispc_color_formats),
 				      sizeof(*dispc->fourccs), GFP_KERNEL);
 	if (!dispc->fourccs)

-- 
2.50.1

Re: [PATCH 4/4] drm/tidss: Fix sampling edge configuration

[Krzysztof Kozlowski]

On Wed, Jul 30, 2025 at 07:02:47PM +0200, Louis Chauvet wrote:
> As stated in the AM62x Technical Reference Manual (SPRUIV7B), the data
> sampling edge needs to be configured in two distinct registers: one in the
> TIDSS IP and another in the memory-mapped control register modules. Since
> the latter is not within the same address range, a phandle to a syscon
> device is used to access the regmap.
> 
> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> 
> ---
> 
> Cc: stable@vger.kernel.org

Please read docs how to add stable tags.

> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index c0277fa36425ee1f966dccecf2b69a2d01794899..65ca7629a2e75437023bf58f8a1bddc24db5e3da 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -498,6 +498,7 @@ struct dispc_device {
>  	const struct dispc_features *feat;
>  
>  	struct clk *fclk;
> +	struct regmap *clk_ctrl;
>  
>  	bool is_enabled;
>  
> @@ -1267,6 +1268,11 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
>  		       FLD_VAL(mode->vdisplay - 1, 27, 16));
>  
>  	VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1, 0, 0);
> +
> +	if (dispc->clk_ctrl) {
> +		regmap_update_bits(dispc->clk_ctrl, 0, 0x100, ipc ? 0x100 : 0x000);
> +		regmap_update_bits(dispc->clk_ctrl, 0, 0x200, rf ? 0x200 : 0x000);
> +	}
>  }
>  
>  void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport)
> @@ -3012,6 +3018,14 @@ int dispc_init(struct tidss_device *tidss)
>  
>  	dispc_init_errata(dispc);
>  
> +	dispc->clk_ctrl = syscon_regmap_lookup_by_phandle_optional(tidss->dev->of_node,
> +								   "ti,clk-ctrl");
> +	if (IS_ERR(dispc->clk_ctrl)) {
> +		r = dev_err_probe(dispc->dev, PTR_ERR(dispc->clk_ctrl),
> +				  "DISPC: syscon_regmap_lookup_by_phandle failed.\n");
> +		return r;

This breaks ABI. Commit msg mentions the reason but without
justification - was everything broken? Nothing was working? Was it ever
tested?

And anyway ABI impact must be clearly documented.


Best regards,
Krzysztof

Re: [PATCH 4/4] drm/tidss: Fix sampling edge configuration

[devarsh]

Hi Louis,

Thanks for the patch.

On 30/07/25 22:32, Louis Chauvet wrote:
> As stated in the AM62x Technical Reference Manual (SPRUIV7B), the data
> sampling edge needs to be configured in two distinct registers: one in the
> TIDSS IP and another in the memory-mapped control register modules.

I don't think AM62x is thee only one which requires this and on the
contrary not all SoCs require this extra setting. We had been waiting on
confirmations from hardware team and very recently they gave a list of
SoCs which require this, as per that I think we need to limit this to
AM62x and AM62A per current supported SoCs.

Swamil,
Please confirm on this and share if any additional details required here.

Regards
Devarsh

 Since
> the latter is not within the same address range, a phandle to a syscon
> device is used to access the regmap.
> 
> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> 
> ---
> 
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index c0277fa36425ee1f966dccecf2b69a2d01794899..65ca7629a2e75437023bf58f8a1bddc24db5e3da 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -498,6 +498,7 @@ struct dispc_device {
>  	const struct dispc_features *feat;
>  
>  	struct clk *fclk;
> +	struct regmap *clk_ctrl;
>  
>  	bool is_enabled;
>  
> @@ -1267,6 +1268,11 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
>  		       FLD_VAL(mode->vdisplay - 1, 27, 16));
>  
>  	VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1, 0, 0);
> +
> +	if (dispc->clk_ctrl) {
> +		regmap_update_bits(dispc->clk_ctrl, 0, 0x100, ipc ? 0x100 : 0x000);
> +		regmap_update_bits(dispc->clk_ctrl, 0, 0x200, rf ? 0x200 : 0x000);
> +	}
>  }
>  
>  void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport)
> @@ -3012,6 +3018,14 @@ int dispc_init(struct tidss_device *tidss)
>  
>  	dispc_init_errata(dispc);
>  
> +	dispc->clk_ctrl = syscon_regmap_lookup_by_phandle_optional(tidss->dev->of_node,
> +								   "ti,clk-ctrl");
> +	if (IS_ERR(dispc->clk_ctrl)) {
> +		r = dev_err_probe(dispc->dev, PTR_ERR(dispc->clk_ctrl),
> +				  "DISPC: syscon_regmap_lookup_by_phandle failed.\n");
> +		return r;
> +	}
> +
>  	dispc->fourccs = devm_kcalloc(dev, ARRAY_SIZE(dispc_color_formats),
>  				      sizeof(*dispc->fourccs), GFP_KERNEL);
>  	if (!dispc->fourccs)
> 

Re: [PATCH 4/4] drm/tidss: Fix sampling edge configuration

[Swamil Jain]

On 8/8/25 19:16, devarsh wrote:
> Hi Louis,
> 
> Thanks for the patch.
> 
> On 30/07/25 22:32, Louis Chauvet wrote:
>> As stated in the AM62x Technical Reference Manual (SPRUIV7B), the data
>> sampling edge needs to be configured in two distinct registers: one in the
>> TIDSS IP and another in the memory-mapped control register modules.
> 
> I don't think AM62x is thee only one which requires this and on the
> contrary not all SoCs require this extra setting. We had been waiting on
> confirmations from hardware team and very recently they gave a list of
> SoCs which require this, as per that I think we need to limit this to
> AM62x and AM62A per current supported SoCs.
> 
> Swamil,
> Please confirm on this and share if any additional details required here.
> 

Yeah Devarsh, as you mentioned, this is valid for AM62X, AM62A and 
AM62P. We would have upstreamed this feature, but there are some 
corrections in Technical Reference Manual for these SoCs regarding 
programming CTRL_MMR_DPI_CLK_CTRL register fields, we are in loop with 
H/W team, waiting for their official confirmation regarding this issue.

Thanks Louis for working on this patch, but we should wait for H/W 
team's confirmation.

Regards,
Swamil.

> Regards
> Devarsh
> 
>   Since
>> the latter is not within the same address range, a phandle to a syscon
>> device is used to access the regmap.
>>
>> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>
>> ---
>>
>> Cc: stable@vger.kernel.org
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index c0277fa36425ee1f966dccecf2b69a2d01794899..65ca7629a2e75437023bf58f8a1bddc24db5e3da 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -498,6 +498,7 @@ struct dispc_device {
>>   	const struct dispc_features *feat;
>>   
>>   	struct clk *fclk;
>> +	struct regmap *clk_ctrl;
>>   
>>   	bool is_enabled;
>>   
>> @@ -1267,6 +1268,11 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
>>   		       FLD_VAL(mode->vdisplay - 1, 27, 16));
>>   
>>   	VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1, 0, 0);
>> +
>> +	if (dispc->clk_ctrl) {
>> +		regmap_update_bits(dispc->clk_ctrl, 0, 0x100, ipc ? 0x100 : 0x000);
>> +		regmap_update_bits(dispc->clk_ctrl, 0, 0x200, rf ? 0x200 : 0x000);
>> +	}
>>   }
>>   
>>   void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport)
>> @@ -3012,6 +3018,14 @@ int dispc_init(struct tidss_device *tidss)
>>   
>>   	dispc_init_errata(dispc);
>>   
>> +	dispc->clk_ctrl = syscon_regmap_lookup_by_phandle_optional(tidss->dev->of_node,
>> +								   "ti,clk-ctrl");
>> +	if (IS_ERR(dispc->clk_ctrl)) {
>> +		r = dev_err_probe(dispc->dev, PTR_ERR(dispc->clk_ctrl),
>> +				  "DISPC: syscon_regmap_lookup_by_phandle failed.\n");
>> +		return r;
>> +	}
>> +
>>   	dispc->fourccs = devm_kcalloc(dev, ARRAY_SIZE(dispc_color_formats),
>>   				      sizeof(*dispc->fourccs), GFP_KERNEL);
>>   	if (!dispc->fourccs)
>>
> 

Re: [PATCH 4/4] drm/tidss: Fix sampling edge configuration

[Louis Chauvet]

Le 08/08/2025 à 18:26, Swamil Jain a écrit :
> 
> 
> On 8/8/25 19:16, devarsh wrote:
>> Hi Louis,
>>
>> Thanks for the patch.
>>
>> On 30/07/25 22:32, Louis Chauvet wrote:
>>> As stated in the AM62x Technical Reference Manual (SPRUIV7B), the data
>>> sampling edge needs to be configured in two distinct registers: one in the
>>> TIDSS IP and another in the memory-mapped control register modules.
>>
>> I don't think AM62x is thee only one which requires this and on the
>> contrary not all SoCs require this extra setting. We had been waiting on
>> confirmations from hardware team and very recently they gave a list of
>> SoCs which require this, as per that I think we need to limit this to
>> AM62x and AM62A per current supported SoCs.
>>
>> Swamil,
>> Please confirm on this and share if any additional details required here.
>>
> 
> Yeah Devarsh, as you mentioned, this is valid for AM62X, AM62A and
> AM62P. We would have upstreamed this feature, but there are some
> corrections in Technical Reference Manual for these SoCs regarding
> programming CTRL_MMR_DPI_CLK_CTRL register fields, we are in loop with
> H/W team, waiting for their official confirmation regarding this issue.
> 
> Thanks Louis for working on this patch, but we should wait for H/W
> team's confirmation.

Hello all,

Thanks for the feedback. I was not aware of this current work.
Do you plan to send the fix yourself? Should I wait your HW team 
feedback and send a v2?

I also have a very similar patch ready for u-boot (depending on the same 
DT modifications), do you plan to fix u-boot too?

Thanks,
Louis Chauvet


> Regards,
> Swamil.
> 
>> Regards
>> Devarsh
>>
>>    Since
>>> the latter is not within the same address range, a phandle to a syscon
>>> device is used to access the regmap.
>>>
>>> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>>
>>> ---
>>>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>    drivers/gpu/drm/tidss/tidss_dispc.c | 14 ++++++++++++++
>>>    1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> index c0277fa36425ee1f966dccecf2b69a2d01794899..65ca7629a2e75437023bf58f8a1bddc24db5e3da 100644
>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>> @@ -498,6 +498,7 @@ struct dispc_device {
>>>    	const struct dispc_features *feat;
>>>    
>>>    	struct clk *fclk;
>>> +	struct regmap *clk_ctrl;
>>>    
>>>    	bool is_enabled;
>>>    
>>> @@ -1267,6 +1268,11 @@ void dispc_vp_enable(struct dispc_device *dispc, u32 hw_videoport,
>>>    		       FLD_VAL(mode->vdisplay - 1, 27, 16));
>>>    
>>>    	VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1, 0, 0);
>>> +
>>> +	if (dispc->clk_ctrl) {
>>> +		regmap_update_bits(dispc->clk_ctrl, 0, 0x100, ipc ? 0x100 : 0x000);
>>> +		regmap_update_bits(dispc->clk_ctrl, 0, 0x200, rf ? 0x200 : 0x000);
>>> +	}
>>>    }
>>>    
>>>    void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport)
>>> @@ -3012,6 +3018,14 @@ int dispc_init(struct tidss_device *tidss)
>>>    
>>>    	dispc_init_errata(dispc);
>>>    
>>> +	dispc->clk_ctrl = syscon_regmap_lookup_by_phandle_optional(tidss->dev->of_node,
>>> +								   "ti,clk-ctrl");
>>> +	if (IS_ERR(dispc->clk_ctrl)) {
>>> +		r = dev_err_probe(dispc->dev, PTR_ERR(dispc->clk_ctrl),
>>> +				  "DISPC: syscon_regmap_lookup_by_phandle failed.\n");
>>> +		return r;
>>> +	}
>>> +
>>>    	dispc->fourccs = devm_kcalloc(dev, ARRAY_SIZE(dispc_color_formats),
>>>    				      sizeof(*dispc->fourccs), GFP_KERNEL);
>>>    	if (!dispc->fourccs)
>>>
>>

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 4/4] drm/tidss: Fix sampling edge configuration

[Swamil Jain]

On 8/11/25 15:26, Louis Chauvet wrote:
> 
> 
> Le 08/08/2025 à 18:26, Swamil Jain a écrit :
>>
>>
>> On 8/8/25 19:16, devarsh wrote:
>>> Hi Louis,
>>>
>>> Thanks for the patch.
>>>
>>> On 30/07/25 22:32, Louis Chauvet wrote:
>>>> As stated in the AM62x Technical Reference Manual (SPRUIV7B), the data
>>>> sampling edge needs to be configured in two distinct registers: one 
>>>> in the
>>>> TIDSS IP and another in the memory-mapped control register modules.
>>>
>>> I don't think AM62x is thee only one which requires this and on the
>>> contrary not all SoCs require this extra setting. We had been waiting on
>>> confirmations from hardware team and very recently they gave a list of
>>> SoCs which require this, as per that I think we need to limit this to
>>> AM62x and AM62A per current supported SoCs.
>>>
>>> Swamil,
>>> Please confirm on this and share if any additional details required 
>>> here.
>>>
>>
>> Yeah Devarsh, as you mentioned, this is valid for AM62X, AM62A and
>> AM62P. We would have upstreamed this feature, but there are some
>> corrections in Technical Reference Manual for these SoCs regarding
>> programming CTRL_MMR_DPI_CLK_CTRL register fields, we are in loop with
>> H/W team, waiting for their official confirmation regarding this issue.
>>
>> Thanks Louis for working on this patch, but we should wait for H/W
>> team's confirmation.
> 
> Hello all,
> 
> Thanks for the feedback. I was not aware of this current work.
> Do you plan to send the fix yourself? Should I wait your HW team 
> feedback and send a v2?
> 
Hi Louis, H/W team confirmed that, CTRL_MMR_DPI0_CLK_CTRL.bit[8] should 
be programmed same as DSS_VP1_POL_FREQ.bit[14](IPC) and 
CTRL_MMR_DPI0_CLK_CTRL.bit[9] should be programmed same as 
DSS_VP1_POL_FREQ.bit[16](RF). Please continue with you patches.

> I also have a very similar patch ready for u-boot (depending on the same 
> DT modifications), do you plan to fix u-boot too?
> 
Please fix u-boot also.

Thanks and regards,
Swamil.

> Thanks,
> Louis Chauvet
> 
> 
>> Regards,
>> Swamil.
>>
>>> Regards
>>> Devarsh
>>>
>>>    Since
>>>> the latter is not within the same address range, a phandle to a syscon
>>>> device is used to access the regmap.
>>>>
>>>> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform 
>>>> Display SubSystem")
>>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>>>
>>>> ---
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>>    drivers/gpu/drm/tidss/tidss_dispc.c | 14 ++++++++++++++
>>>>    1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> index 
>>>> c0277fa36425ee1f966dccecf2b69a2d01794899..65ca7629a2e75437023bf58f8a1bddc24db5e3da 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>> @@ -498,6 +498,7 @@ struct dispc_device {
>>>>        const struct dispc_features *feat;
>>>>        struct clk *fclk;
>>>> +    struct regmap *clk_ctrl;
>>>>        bool is_enabled;
>>>> @@ -1267,6 +1268,11 @@ void dispc_vp_enable(struct dispc_device 
>>>> *dispc, u32 hw_videoport,
>>>>                   FLD_VAL(mode->vdisplay - 1, 27, 16));
>>>>        VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1, 0, 0);
>>>> +
>>>> +    if (dispc->clk_ctrl) {
>>>> +        regmap_update_bits(dispc->clk_ctrl, 0, 0x100, ipc ? 0x100 : 
>>>> 0x000);
>>>> +        regmap_update_bits(dispc->clk_ctrl, 0, 0x200, rf ? 0x200 : 
>>>> 0x000);
>>>> +    }
>>>>    }
>>>>    void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport)
>>>> @@ -3012,6 +3018,14 @@ int dispc_init(struct tidss_device *tidss)
>>>>        dispc_init_errata(dispc);
>>>> +    dispc->clk_ctrl = 
>>>> syscon_regmap_lookup_by_phandle_optional(tidss->dev->of_node,
>>>> +                                   "ti,clk-ctrl");
>>>> +    if (IS_ERR(dispc->clk_ctrl)) {
>>>> +        r = dev_err_probe(dispc->dev, PTR_ERR(dispc->clk_ctrl),
>>>> +                  "DISPC: syscon_regmap_lookup_by_phandle failed.\n");
>>>> +        return r;
>>>> +    }
>>>> +
>>>>        dispc->fourccs = devm_kcalloc(dev, 
>>>> ARRAY_SIZE(dispc_color_formats),
>>>>                          sizeof(*dispc->fourccs), GFP_KERNEL);
>>>>        if (!dispc->fourccs)
>>>>
>>>
> 

Re: [PATCH 4/4] drm/tidss: Fix sampling edge configuration

[Swamil Jain]

On 8/12/25 23:32, Swamil Jain wrote:
> 
> 
> On 8/11/25 15:26, Louis Chauvet wrote:
>>
>>
>> Le 08/08/2025 à 18:26, Swamil Jain a écrit :
>>>
>>>
>>> On 8/8/25 19:16, devarsh wrote:
>>>> Hi Louis,
>>>>
>>>> Thanks for the patch.
>>>>
>>>> On 30/07/25 22:32, Louis Chauvet wrote:
>>>>> As stated in the AM62x Technical Reference Manual (SPRUIV7B), the data
>>>>> sampling edge needs to be configured in two distinct registers: one 
>>>>> in the
>>>>> TIDSS IP and another in the memory-mapped control register modules.
>>>>
>>>> I don't think AM62x is thee only one which requires this and on the
>>>> contrary not all SoCs require this extra setting. We had been 
>>>> waiting on
>>>> confirmations from hardware team and very recently they gave a list of
>>>> SoCs which require this, as per that I think we need to limit this to
>>>> AM62x and AM62A per current supported SoCs.
>>>>
>>>> Swamil,
>>>> Please confirm on this and share if any additional details required 
>>>> here.
>>>>
>>>
>>> Yeah Devarsh, as you mentioned, this is valid for AM62X, AM62A and
>>> AM62P. We would have upstreamed this feature, but there are some
>>> corrections in Technical Reference Manual for these SoCs regarding
>>> programming CTRL_MMR_DPI_CLK_CTRL register fields, we are in loop with
>>> H/W team, waiting for their official confirmation regarding this issue.
>>>
>>> Thanks Louis for working on this patch, but we should wait for H/W
>>> team's confirmation.
>>
>> Hello all,
>>
>> Thanks for the feedback. I was not aware of this current work.
>> Do you plan to send the fix yourself? Should I wait your HW team 
>> feedback and send a v2?
>>
> Hi Louis, H/W team confirmed that, CTRL_MMR_DPI0_CLK_CTRL.bit[8] should 
> be programmed same as DSS_VP1_POL_FREQ.bit[14](IPC) and 
> CTRL_MMR_DPI0_CLK_CTRL.bit[9] should be programmed same as 
> DSS_VP1_POL_FREQ.bit[16](RF). Please continue with you patches.
> 
Please go ahead and send v2.
We are working with the documentation team to get the Technical 
Reference Manual updated in parallel.

Regards,
Swamil.

>> I also have a very similar patch ready for u-boot (depending on the 
>> same DT modifications), do you plan to fix u-boot too?
>>
> Please fix u-boot also.
> 
> Thanks and regards,
> Swamil.
> 
>> Thanks,
>> Louis Chauvet
>>
>>
>>> Regards,
>>> Swamil.
>>>
>>>> Regards
>>>> Devarsh
>>>>
>>>>    Since
>>>>> the latter is not within the same address range, a phandle to a syscon
>>>>> device is used to access the regmap.
>>>>>
>>>>> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone 
>>>>> platform Display SubSystem")
>>>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>>>>
>>>>> ---
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>> ---
>>>>>    drivers/gpu/drm/tidss/tidss_dispc.c | 14 ++++++++++++++
>>>>>    1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c 
>>>>> b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>>> index 
>>>>> c0277fa36425ee1f966dccecf2b69a2d01794899..65ca7629a2e75437023bf58f8a1bddc24db5e3da 100644
>>>>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>>>>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>>>>> @@ -498,6 +498,7 @@ struct dispc_device {
>>>>>        const struct dispc_features *feat;
>>>>>        struct clk *fclk;
>>>>> +    struct regmap *clk_ctrl;
>>>>>        bool is_enabled;
>>>>> @@ -1267,6 +1268,11 @@ void dispc_vp_enable(struct dispc_device 
>>>>> *dispc, u32 hw_videoport,
>>>>>                   FLD_VAL(mode->vdisplay - 1, 27, 16));
>>>>>        VP_REG_FLD_MOD(dispc, hw_videoport, DISPC_VP_CONTROL, 1, 0, 0);
>>>>> +
>>>>> +    if (dispc->clk_ctrl) {
>>>>> +        regmap_update_bits(dispc->clk_ctrl, 0, 0x100, ipc ? 0x100 
>>>>> : 0x000);
>>>>> +        regmap_update_bits(dispc->clk_ctrl, 0, 0x200, rf ? 0x200 : 
>>>>> 0x000);
>>>>> +    }
>>>>>    }
>>>>>    void dispc_vp_disable(struct dispc_device *dispc, u32 hw_videoport)
>>>>> @@ -3012,6 +3018,14 @@ int dispc_init(struct tidss_device *tidss)
>>>>>        dispc_init_errata(dispc);
>>>>> +    dispc->clk_ctrl = 
>>>>> syscon_regmap_lookup_by_phandle_optional(tidss->dev->of_node,
>>>>> +                                   "ti,clk-ctrl");
>>>>> +    if (IS_ERR(dispc->clk_ctrl)) {
>>>>> +        r = dev_err_probe(dispc->dev, PTR_ERR(dispc->clk_ctrl),
>>>>> +                  "DISPC: syscon_regmap_lookup_by_phandle 
>>>>> failed.\n");
>>>>> +        return r;
>>>>> +    }
>>>>> +
>>>>>        dispc->fourccs = devm_kcalloc(dev, 
>>>>> ARRAY_SIZE(dispc_color_formats),
>>>>>                          sizeof(*dispc->fourccs), GFP_KERNEL);
>>>>>        if (!dispc->fourccs)
>>>>>
>>>>
>>
> 

Re: [PATCH 0/4] drm/tidss: Fixes data edge sampling

[Tomi Valkeinen]

Hi,

On 30/07/2025 20:02, Louis Chauvet wrote:
> Currently the driver only configure the data edge sampling partially. The 
> AM62 require it to be configured in two distincts registers: one in tidss 
> and one in the general device registers.
> 
> Introduce a new dt property to link the proper syscon node from the main 
> device registers into the tidss driver.
> 
> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> ---
> Cc: stable@vger.kernel.org
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>

I understand why you call this a fix, but I think this is not really a
fix. From looking at the patches, my understanding is that for DPI
outputs we have always only supported certain clock/data edge. So this
series is adding a new feature to the driver.

You can add new things to the DT bindings, but you have to keep the old
bindings working.

 Tomi

Re: [PATCH 0/4] drm/tidss: Fixes data edge sampling

[devarsh]

Hi Tomi, Louis,

On 07/08/25 18:51, Tomi Valkeinen wrote:
> Hi,
> 
> On 30/07/2025 20:02, Louis Chauvet wrote:
>> Currently the driver only configure the data edge sampling partially. The 
>> AM62 require it to be configured in two distincts registers: one in tidss 
>> and one in the general device registers.
>>
>> Introduce a new dt property to link the proper syscon node from the main 
>> device registers into the tidss driver.
>>
>> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
>> ---
>> Cc: stable@vger.kernel.org
>>
>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> 
> I understand why you call this a fix, but I think this is not really a
> fix. From looking at the patches, my understanding is that for DPI
> outputs we have always only supported certain clock/data edge.

I don't think driver makes a distinction between supported/unsupported
or errors out in case it is run with "different" clock/data edge panel
(for e.g  DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE is set per the panel
configuration). Instead it tries to program the VP registers per the
DRM_BUS_FLAG* getting passed by framework per the connected panel and
gives an incorrect behavior if those are different than defaults since
those settings are not sufficient for these displays and instead extra
MMR register settings are also required.

 So this
> series is adding a new feature to the driver.
> 

I think it is a bug due to above mentioned behavior, and good to have a
Fixes tag.

Regards
Devarsh

Re: [PATCH 0/4] drm/tidss: Fixes data edge sampling

[Tomi Valkeinen]

Hi,

On 08/08/2025 16:24, devarsh wrote:
> Hi Tomi, Louis,
> 
> On 07/08/25 18:51, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 30/07/2025 20:02, Louis Chauvet wrote:
>>> Currently the driver only configure the data edge sampling partially. The 
>>> AM62 require it to be configured in two distincts registers: one in tidss 
>>> and one in the general device registers.
>>>
>>> Introduce a new dt property to link the proper syscon node from the main 
>>> device registers into the tidss driver.
>>>
>>> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
>>> ---
>>> Cc: stable@vger.kernel.org
>>>
>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>
>> I understand why you call this a fix, but I think this is not really a
>> fix. From looking at the patches, my understanding is that for DPI
>> outputs we have always only supported certain clock/data edge.
> 
> I don't think driver makes a distinction between supported/unsupported
> or errors out in case it is run with "different" clock/data edge panel
> (for e.g  DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE is set per the panel
> configuration). Instead it tries to program the VP registers per the
> DRM_BUS_FLAG* getting passed by framework per the connected panel and
> gives an incorrect behavior if those are different than defaults since
> those settings are not sufficient for these displays and instead extra
> MMR register settings are also required.

Well, this gets into the meaning of "fix". I didn't right away see an
explicit definition in the kernel docs.

When the tidss driver was added, neither AM62x nor AM62A existed. Yet
this series "fixes" the original tidss commit for AM62x and AM62A? And
the patch proposes that this series is to be backported to stable
kernels going back to the original tidss commit?

When AM62x and AM62A support was added to the tidss, this feature was
not in the driver. So this is clearly also not a regression. Missing
this feature causes no crashes or other system level misbehavior. It
only causes the panels (that have never been supported with tidss on
AM62x and AM62A) to show garbage.

So we have a driver, to which support for new SoCs was added at some
later point, and at that point we did not add support for all kinds of
panels. Is adding support for those panels a new feature or a bug fix?
Should it be backported to stable kernels?

Documentation/process/stable-kernel-rules.rst has some guidelines. Maybe
one could argue that this is a "hardware quirk" mentioned there, or
perhaps "add a device ID" (of sorts). I might agree, if this was an
easily backportable, totally non-controversial, one-liner style patch
with no chance of regressions. Maybe the next version will be, but this
one is not.

 Tomi

Re: [PATCH 0/4] drm/tidss: Fixes data edge sampling

[Louis Chauvet]

Le 11/08/2025 à 09:56, Tomi Valkeinen a écrit :
> Hi,
> 
> On 08/08/2025 16:24, devarsh wrote:
>> Hi Tomi, Louis,
>>
>> On 07/08/25 18:51, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 30/07/2025 20:02, Louis Chauvet wrote:
>>>> Currently the driver only configure the data edge sampling partially. The
>>>> AM62 require it to be configured in two distincts registers: one in tidss
>>>> and one in the general device registers.
>>>>
>>>> Introduce a new dt property to link the proper syscon node from the main
>>>> device registers into the tidss driver.
>>>>
>>>> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
>>>> ---
>>>> Cc: stable@vger.kernel.org
>>>>
>>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>>
>>> I understand why you call this a fix, but I think this is not really a
>>> fix. From looking at the patches, my understanding is that for DPI
>>> outputs we have always only supported certain clock/data edge.
>>
>> I don't think driver makes a distinction between supported/unsupported
>> or errors out in case it is run with "different" clock/data edge panel
>> (for e.g  DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE is set per the panel
>> configuration). Instead it tries to program the VP registers per the
>> DRM_BUS_FLAG* getting passed by framework per the connected panel and
>> gives an incorrect behavior if those are different than defaults since
>> those settings are not sufficient for these displays and instead extra
>> MMR register settings are also required.
> 
> Well, this gets into the meaning of "fix". I didn't right away see an
> explicit definition in the kernel docs.
> 
> When the tidss driver was added, neither AM62x nor AM62A existed. Yet
> this series "fixes" the original tidss commit for AM62x and AM62A? And
> the patch proposes that this series is to be backported to stable
> kernels going back to the original tidss commit?

I overlooked the initial commit, I agree this should be limited to 
ad2ac9dc9426 ("drm/tidss: Add support for AM625 DSS").

> When AM62x and AM62A support was added to the tidss, this feature was
> not in the driver. So this is clearly also not a regression. Missing
> this feature causes no crashes or other system level misbehavior. It
> only causes the panels (that have never been supported with tidss on
> AM62x and AM62A) to show garbage.

For me the driver explicitly support different data sampling edge from 
the start, but when the AM625 was added, the implementation missed this 
quirk, so we have a broken driver for this platform.

> So we have a driver, to which support for new SoCs was added at some
> later point, and at that point we did not add support for all kinds of
> panels. Is adding support for those panels a new feature or a bug fix?
> Should it be backported to stable kernels?
>
> Documentation/process/stable-kernel-rules.rst has some guidelines. Maybe
> one could argue that this is a "hardware quirk" mentioned there, or
> perhaps "add a device ID" (of sorts).

For me this clearly falls in the "hardware quirk" category, as we need 
this quirk on some platforms.

> I might agree, if this was an
> easily backportable, totally non-controversial, one-liner style patch
> with no chance of regressions. Maybe the next version will be, but this
> one is not.

Ack, we will see how the v2 will be. If the v2 is not too complex, I 
will keep the fixes tag (on the proper commit).

Thanks,
Louis Chauvet

>   Tomi
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com