Re: [PATCH v3 3/3] ARM: dts: stm32: fix several DT warnings on stm32mp15
On 5/29/23 10:07, Raphael Gallais-Pou wrote: Hi, I think if you retain the stm32mp151.dtsi { port { #address-cells = <1>; #size-cells = <0>; }; }; part, then you wouldn't be getting any warnings regarding LTDC , and you wouldn't have to remove the unit-address from endpoint@0 . btw. I do use both endpoint@0/endpoint@1 in Avenger96 DTOs, but those are not submitted yet, I have to clean them up a bit more first. One way to do it would be to make the endpoint@0 go down in the device-tree with its dependencies, so that both endpoints are the same level without generating noise. I'm afraid I really don't quite understand which warning you're referring to. Can you please share that warning and ideally how to trigger it (the command-line incantation) ? Using '$ make dtbs W=1', you can observe several of the followings: arch/arm/boot/dts/stm32mp151.dtsi:1533.9-1536.6: Warning (avoid_unnecessary_addr_size): /soc/display-controller@5a001000/port: unnecessary #address-cells/#size-cells without "ranges" or child "reg" property arch/arm/boot/dts/stm32mp151.dtsi:1533.9-1536.6: Warning (graph_child_address): /soc/display-controller@5a001000/port: graph node has single child node 'endpoint@0', #address-cells/#size-cells are not necessary This { port { #address-cells = <1>; #size-cells = <0>; }; }; part is actually annoying. This is because there is several device-trees that only got one endpoint, and some other that includes two. For instance: stm32mp15xx-dhcor-avenger96.dtsi vs stm32mp157c-dk2.dts. I would like to remove to root part of address/size field and let only the lower device-trees with with multiple endpoints handle their own fields. I hope this explains a bit better my process. After thinking about this some more, and digging through LTDC driver, and testing on EV1, I think dropping the LTDC node endpoint@N and reg= altogether and just using port/endpoint (singular) is fine. You might want to split the DSI node specific changes and the LTDC node specific changes into separate patches (LTDC specific change like you did in 1/3). Yes, I prepared a new serie with that split, to that it is better to read and review. Thank you !
Re: [PATCH v3 3/3] ARM: dts: stm32: fix several DT warnings on stm32mp15
On 5/26/23 18:55, Marek Vasut wrote: > On 5/25/23 10:14, Raphael Gallais-Pou wrote: > > Hi, Hi Marek, > >>> I think if you retain the stm32mp151.dtsi { port { #address-cells = >>> <1>; >>> #size-cells = <0>; }; }; part, then you wouldn't be getting any warnings >>> regarding LTDC , and you wouldn't have to remove the unit-address from >>> endpoint@0 . >>> >>> btw. I do use both endpoint@0/endpoint@1 in Avenger96 DTOs, but those are >>> not >>> submitted yet, I have to clean them up a bit more first. >>> One way to do it would be to make the endpoint@0 go down in the device-tree with its dependencies, so that both endpoints are the same level without generating noise. >>> >>> I'm afraid I really don't quite understand which warning you're referring >>> to. >>> Can you please share that warning and ideally how to trigger it (the >>> command-line incantation) ? >> >> Using '$ make dtbs W=1', you can observe several of the followings: >> >> arch/arm/boot/dts/stm32mp151.dtsi:1533.9-1536.6: Warning >> (avoid_unnecessary_addr_size): /soc/display-controller@5a001000/port: >> unnecessary #address-cells/#size-cells without "ranges" or child "reg" >> property >> arch/arm/boot/dts/stm32mp151.dtsi:1533.9-1536.6: Warning >> (graph_child_address): >> /soc/display-controller@5a001000/port: graph node has single child node >> 'endpoint@0', #address-cells/#size-cells are not necessary >> >> This { port { #address-cells = <1>; #size-cells = <0>; }; }; part is >> actually annoying. This is because there is several device-trees that only >> got >> one endpoint, and some other that includes two. >> >> For instance: stm32mp15xx-dhcor-avenger96.dtsi vs stm32mp157c-dk2.dts. >> >> I would like to remove to root part of address/size field and let only the >> lower >> device-trees with with multiple endpoints handle their own fields. I hope >> this >> explains a bit better my process. > > After thinking about this some more, and digging through LTDC driver, and > testing on EV1, I think dropping the LTDC node endpoint@N and reg= > altogether and just using port/endpoint (singular) is fine. > > You might want to split the DSI node specific changes and the LTDC node > specific changes into separate patches (LTDC specific change like you did in > 1/3). Yes, I prepared a new serie with that split, to that it is better to read and review. Raphaël
Re: [PATCH v3 3/3] ARM: dts: stm32: fix several DT warnings on stm32mp15
On 5/25/23 10:14, Raphael Gallais-Pou wrote: Hi, I think if you retain the stm32mp151.dtsi { port { #address-cells = <1>; #size-cells = <0>; }; }; part, then you wouldn't be getting any warnings regarding LTDC , and you wouldn't have to remove the unit-address from endpoint@0 . btw. I do use both endpoint@0/endpoint@1 in Avenger96 DTOs, but those are not submitted yet, I have to clean them up a bit more first. One way to do it would be to make the endpoint@0 go down in the device-tree with its dependencies, so that both endpoints are the same level without generating noise. I'm afraid I really don't quite understand which warning you're referring to. Can you please share that warning and ideally how to trigger it (the command-line incantation) ? Using '$ make dtbs W=1', you can observe several of the followings: arch/arm/boot/dts/stm32mp151.dtsi:1533.9-1536.6: Warning (avoid_unnecessary_addr_size): /soc/display-controller@5a001000/port: unnecessary #address-cells/#size-cells without "ranges" or child "reg" property arch/arm/boot/dts/stm32mp151.dtsi:1533.9-1536.6: Warning (graph_child_address): /soc/display-controller@5a001000/port: graph node has single child node 'endpoint@0', #address-cells/#size-cells are not necessary This { port { #address-cells = <1>; #size-cells = <0>; }; }; part is actually annoying. This is because there is several device-trees that only got one endpoint, and some other that includes two. For instance: stm32mp15xx-dhcor-avenger96.dtsi vs stm32mp157c-dk2.dts. I would like to remove to root part of address/size field and let only the lower device-trees with with multiple endpoints handle their own fields. I hope this explains a bit better my process. After thinking about this some more, and digging through LTDC driver, and testing on EV1, I think dropping the LTDC node endpoint@N and reg= altogether and just using port/endpoint (singular) is fine. You might want to split the DSI node specific changes and the LTDC node specific changes into separate patches (LTDC specific change like you did in 1/3).
Re: [PATCH v3 3/3] ARM: dts: stm32: fix several DT warnings on stm32mp15
On 5/18/23 01:33, Marek Vasut wrote: > On 5/17/23 19:04, Raphael Gallais-Pou wrote: >> Hi Marek > > Hi, > >> On 5/17/23 17:41, Marek Vasut wrote: >>> On 5/17/23 16:35, Raphael Gallais-Pou wrote: >>> >>> Hi, >>> diff --git a/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi b/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi index 0f1110e42c93..a6e2e20f12fa 100644 --- a/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi +++ b/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi @@ -457,8 +457,7 @@ { status = "okay"; port { - ltdc_ep0_out: endpoint@0 { - reg = <0>; + ltdc_ep0_out: endpoint { remote-endpoint = <_in>; }; }; >>> >>> This LTDC port/endpoint stuff always scares me, because I always feel I get >>> it >>> wrong. >>> >>> I believe the LTDC does have one "port" , correct. >>> >>> But I think (?) that the LTDC has two endpoints, endpoint@0 for DPI >>> (parallel >>> output out of the SoC) and endpoint@1 for DSI (internal connection into the >>> DSI serializer) ? >> >> You are correct indeed, I rushed the patch and did not thought about this. I >> agree that this can be confusing, as I also take some time to think through >> it. >> >>> >>> Only one of the endpoints can be connected at a time, but there are actually >>> two endpoints in the LTDC port {} node, aren't there ? >> Yes, they are mutually exclusive. >>> >>> So the original description should be OK I think , maybe >>> #address/#size-cells >>> are missing instead ? >> >> Thing is: this file is only included in two device-trees : >> stm32mp157c-dk1.dts >> and stm32mp157c-dk2.dts. >> >> Among those two files there is only one which adds a second endpoint. Thus if >> the fields are set higher in the hierarchy, a warning yields. > > I do not understand this one part, which warning are you trying to fix ? > I just ran '$ make CHECK_DTBS=1 stm32mp157a-dk1.dtb stm32mp157c-dk2.dtb' in > latest linux-next and there was no warning related to LTDC . I'm sorry, I looked back at it and my explanations are confusing. I use Alex Torgue's tree, and I'm based on the next branch, but linux-next should be the same I just checked it. > > I think if you retain the stm32mp151.dtsi { port { #address-cells = <1>; > #size-cells = <0>; }; }; part, then you wouldn't be getting any warnings > regarding LTDC , and you wouldn't have to remove the unit-address from > endpoint@0 . > > btw. I do use both endpoint@0/endpoint@1 in Avenger96 DTOs, but those are not > submitted yet, I have to clean them up a bit more first. > >> One way to do it would be to make the endpoint@0 go down in the device-tree >> with >> its dependencies, so that both endpoints are the same level without >> generating >> noise. > > I'm afraid I really don't quite understand which warning you're referring to. > Can you please share that warning and ideally how to trigger it (the > command-line incantation) ? Using '$ make dtbs W=1', you can observe several of the followings: arch/arm/boot/dts/stm32mp151.dtsi:1533.9-1536.6: Warning (avoid_unnecessary_addr_size): /soc/display-controller@5a001000/port: unnecessary #address-cells/#size-cells without "ranges" or child "reg" property arch/arm/boot/dts/stm32mp151.dtsi:1533.9-1536.6: Warning (graph_child_address): /soc/display-controller@5a001000/port: graph node has single child node 'endpoint@0', #address-cells/#size-cells are not necessary This { port { #address-cells = <1>; #size-cells = <0>; }; }; part is actually annoying. This is because there is several device-trees that only got one endpoint, and some other that includes two. For instance: stm32mp15xx-dhcor-avenger96.dtsi vs stm32mp157c-dk2.dts. I would like to remove to root part of address/size field and let only the lower device-trees with with multiple endpoints handle their own fields. I hope this explains a bit better my process. Regards, Raphaël Gallais-Pou
Re: [PATCH v3 3/3] ARM: dts: stm32: fix several DT warnings on stm32mp15
On 5/17/23 19:04, Raphael Gallais-Pou wrote: Hi Marek Hi, On 5/17/23 17:41, Marek Vasut wrote: On 5/17/23 16:35, Raphael Gallais-Pou wrote: Hi, diff --git a/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi b/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi index 0f1110e42c93..a6e2e20f12fa 100644 --- a/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi +++ b/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi @@ -457,8 +457,7 @@ { status = "okay"; port { - ltdc_ep0_out: endpoint@0 { - reg = <0>; + ltdc_ep0_out: endpoint { remote-endpoint = <_in>; }; }; This LTDC port/endpoint stuff always scares me, because I always feel I get it wrong. I believe the LTDC does have one "port" , correct. But I think (?) that the LTDC has two endpoints, endpoint@0 for DPI (parallel output out of the SoC) and endpoint@1 for DSI (internal connection into the DSI serializer) ? You are correct indeed, I rushed the patch and did not thought about this. I agree that this can be confusing, as I also take some time to think through it. Only one of the endpoints can be connected at a time, but there are actually two endpoints in the LTDC port {} node, aren't there ? Yes, they are mutually exclusive. So the original description should be OK I think , maybe #address/#size-cells are missing instead ? Thing is: this file is only included in two device-trees : stm32mp157c-dk1.dts and stm32mp157c-dk2.dts. Among those two files there is only one which adds a second endpoint. Thus if the fields are set higher in the hierarchy, a warning yields. I do not understand this one part, which warning are you trying to fix ? I just ran '$ make CHECK_DTBS=1 stm32mp157a-dk1.dtb stm32mp157c-dk2.dtb' in latest linux-next and there was no warning related to LTDC . I think if you retain the stm32mp151.dtsi { port { #address-cells = <1>; #size-cells = <0>; }; }; part, then you wouldn't be getting any warnings regarding LTDC , and you wouldn't have to remove the unit-address from endpoint@0 . btw. I do use both endpoint@0/endpoint@1 in Avenger96 DTOs, but those are not submitted yet, I have to clean them up a bit more first. One way to do it would be to make the endpoint@0 go down in the device-tree with its dependencies, so that both endpoints are the same level without generating noise. I'm afraid I really don't quite understand which warning you're referring to. Can you please share that warning and ideally how to trigger it (the command-line incantation) ? -- Best regards, Marek Vasut
Re: [PATCH v3 3/3] ARM: dts: stm32: fix several DT warnings on stm32mp15
Hi Marek On 5/17/23 17:41, Marek Vasut wrote: > On 5/17/23 16:35, Raphael Gallais-Pou wrote: > > Hi, > >> diff --git a/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi >> b/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi >> index 0f1110e42c93..a6e2e20f12fa 100644 >> --- a/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi >> +++ b/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi >> @@ -457,8 +457,7 @@ { >> status = "okay"; >> port { >> - ltdc_ep0_out: endpoint@0 { >> - reg = <0>; >> + ltdc_ep0_out: endpoint { >> remote-endpoint = <_in>; >> }; >> }; > > This LTDC port/endpoint stuff always scares me, because I always feel I get it > wrong. > > I believe the LTDC does have one "port" , correct. > > But I think (?) that the LTDC has two endpoints, endpoint@0 for DPI (parallel > output out of the SoC) and endpoint@1 for DSI (internal connection into the > DSI serializer) ? You are correct indeed, I rushed the patch and did not thought about this. I agree that this can be confusing, as I also take some time to think through it. > > Only one of the endpoints can be connected at a time, but there are actually > two endpoints in the LTDC port {} node, aren't there ? Yes, they are mutually exclusive. > > So the original description should be OK I think , maybe #address/#size-cells > are missing instead ? Thing is: this file is only included in two device-trees : stm32mp157c-dk1.dts and stm32mp157c-dk2.dts. Among those two files there is only one which adds a second endpoint. Thus if the fields are set higher in the hierarchy, a warning yields. One way to do it would be to make the endpoint@0 go down in the device-tree with its dependencies, so that both endpoints are the same level without generating noise. Raphaël
Re: [PATCH v3 3/3] ARM: dts: stm32: fix several DT warnings on stm32mp15
On 5/17/23 16:35, Raphael Gallais-Pou wrote: Hi, diff --git a/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi b/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi index 0f1110e42c93..a6e2e20f12fa 100644 --- a/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi +++ b/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi @@ -457,8 +457,7 @@ { status = "okay"; port { - ltdc_ep0_out: endpoint@0 { - reg = <0>; + ltdc_ep0_out: endpoint { remote-endpoint = <_in>; }; }; This LTDC port/endpoint stuff always scares me, because I always feel I get it wrong. I believe the LTDC does have one "port" , correct. But I think (?) that the LTDC has two endpoints, endpoint@0 for DPI (parallel output out of the SoC) and endpoint@1 for DSI (internal connection into the DSI serializer) ? Only one of the endpoints can be connected at a time, but there are actually two endpoints in the LTDC port {} node, aren't there ? So the original description should be OK I think , maybe #address/#size-cells are missing instead ?
[PATCH v3 3/3] ARM: dts: stm32: fix several DT warnings on stm32mp15
Several warnings regarding LTDC and DSI on stm32mp15* device-trees remains. Those concern: * "#size-cells" and "#address-cells" wrongly used * residual "reg" property appearing on endpoints where it could be avoided * Changed 'panel-dsi@0' to 'panel@0' according to dsi-controller.yaml Signed-off-by: Raphael Gallais-Pou --- arch/arm/boot/dts/stm32mp151.dtsi | 5 - arch/arm/boot/dts/stm32mp157.dtsi | 7 --- .../dts/stm32mp157a-icore-stm32mp1-ctouch2-of10.dts| 6 -- .../boot/dts/stm32mp157a-icore-stm32mp1-edimm2.2.dts | 6 -- .../stm32mp157a-microgea-stm32mp1-microdev2.0-of7.dts | 3 +-- arch/arm/boot/dts/stm32mp157c-dk2.dts | 8 arch/arm/boot/dts/stm32mp157c-ev1.dts | 10 +++--- arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts | 3 +-- arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi | 6 +- arch/arm/boot/dts/stm32mp15xx-dkx.dtsi | 3 +-- 10 files changed, 27 insertions(+), 30 deletions(-) diff --git a/arch/arm/boot/dts/stm32mp151.dtsi b/arch/arm/boot/dts/stm32mp151.dtsi index a98ae58e2c1c..bf3830dca742 100644 --- a/arch/arm/boot/dts/stm32mp151.dtsi +++ b/arch/arm/boot/dts/stm32mp151.dtsi @@ -1529,11 +1529,6 @@ ltdc: display-controller@5a001000 { clock-names = "lcd"; resets = < LTDC_R>; status = "disabled"; - - port { - #address-cells = <1>; - #size-cells = <0>; - }; }; iwdg2: watchdog@5a002000 { diff --git a/arch/arm/boot/dts/stm32mp157.dtsi b/arch/arm/boot/dts/stm32mp157.dtsi index 54e73ccea446..5e733cd16ff9 100644 --- a/arch/arm/boot/dts/stm32mp157.dtsi +++ b/arch/arm/boot/dts/stm32mp157.dtsi @@ -24,14 +24,7 @@ dsi: dsi@5a00 { clock-names = "pclk", "ref", "px_clk"; resets = < DSI_R>; reset-names = "apb"; - #address-cells = <1>; - #size-cells = <0>; status = "disabled"; - - ports { - #address-cells = <1>; - #size-cells = <0>; - }; }; }; }; diff --git a/arch/arm/boot/dts/stm32mp157a-icore-stm32mp1-ctouch2-of10.dts b/arch/arm/boot/dts/stm32mp157a-icore-stm32mp1-ctouch2-of10.dts index 9a2a4bc7d079..4279b26547df 100644 --- a/arch/arm/boot/dts/stm32mp157a-icore-stm32mp1-ctouch2-of10.dts +++ b/arch/arm/boot/dts/stm32mp157a-icore-stm32mp1-ctouch2-of10.dts @@ -49,6 +49,9 @@ { phy-dsi-supply = <>; ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { reg = <0>; dsi_in: endpoint { @@ -104,8 +107,7 @@ { status = "okay"; port { - ltdc_ep0_out: endpoint@0 { - reg = <0>; + ltdc_ep0_out: endpoint { remote-endpoint = <_in>; }; }; diff --git a/arch/arm/boot/dts/stm32mp157a-icore-stm32mp1-edimm2.2.dts b/arch/arm/boot/dts/stm32mp157a-icore-stm32mp1-edimm2.2.dts index 390ee8c05754..efba54289820 100644 --- a/arch/arm/boot/dts/stm32mp157a-icore-stm32mp1-edimm2.2.dts +++ b/arch/arm/boot/dts/stm32mp157a-icore-stm32mp1-edimm2.2.dts @@ -49,6 +49,9 @@ { phy-dsi-supply = <>; ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { reg = <0>; dsi_in_ltdc: endpoint { @@ -104,8 +107,7 @@ { status = "okay"; port { - ltdc_out_dsi: endpoint@0 { - reg = <0>; + ltdc_out_dsi: endpoint { remote-endpoint = <_in_ltdc>; }; }; diff --git a/arch/arm/boot/dts/stm32mp157a-microgea-stm32mp1-microdev2.0-of7.dts b/arch/arm/boot/dts/stm32mp157a-microgea-stm32mp1-microdev2.0-of7.dts index 0d7560ba2950..5116a7785201 100644 --- a/arch/arm/boot/dts/stm32mp157a-microgea-stm32mp1-microdev2.0-of7.dts +++ b/arch/arm/boot/dts/stm32mp157a-microgea-stm32mp1-microdev2.0-of7.dts @@ -81,8 +81,7 @@ { status = "okay"; port { - ltdc_ep0_out: endpoint@0 { - reg = <0>; + ltdc_ep0_out: endpoint { remote-endpoint = <_in>; }; }; diff --git a/arch/arm/boot/dts/stm32mp157c-dk2.dts b/arch/arm/boot/dts/stm32mp157c-dk2.dts index ab13e340f4ef..4bef2300ed7c 100644 --- a/arch/arm/boot/dts/stm32mp157c-dk2.dts +++ b/arch/arm/boot/dts/stm32mp157c-dk2.dts @@ -31,10 +31,15 @@ { }; { + #address-cells = <1>; + #size-cells = <0>; status =