| Message ID | 20210415101037.1465-12-alexandre.torgue@foss.st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: dts: stm32: fix "make dtbs_check W=1" round1 | expand |
On 4/15/21 12:10 PM, Alexandre Torgue wrote: > Running "make dtbs_check W=1", some warnings are reported concerning > LTDC port subnode: > > /soc/display-controller@5a001000/port: > unnecessary #address-cells/#size-cells without "ranges" or child "reg" > property > /soc/display-controller@5a001000/port: graph node has single child node > 'endpoint', #address-cells/#size-cells are not necessary btw could you retain diffstat on your patches ? It's useful to see which files changed right away. [...] > diff --git a/arch/arm/boot/dts/stm32mp157c-dk2.dts b/arch/arm/boot/dts/stm32mp157c-dk2.dts > index 2bc92ef3aeb9..19ef475a48fc 100644 > --- a/arch/arm/boot/dts/stm32mp157c-dk2.dts > +++ b/arch/arm/boot/dts/stm32mp157c-dk2.dts > @@ -82,9 +82,15 @@ > }; > > <dc { > - status = "okay"; > - > port { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ltdc_ep0_out: endpoint@0 { > + reg = <0>; > + remote-endpoint = <&sii9022_in>; > + }; > + > ltdc_ep1_out: endpoint@1 { > reg = <1>; > remote-endpoint = <&dsi_in>; [...] > diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi > index 64dca5b7f748..e7f10975cacf 100644 > --- a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi > +++ b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi > @@ -277,11 +277,7 @@ > status = "okay"; > > port { > - #address-cells = <1>; > - #size-cells = <0>; > - > - ltdc_ep0_out: endpoint@0 { > - reg = <0>; > + ltdc_ep0_out: endpoint { > remote-endpoint = <&adv7513_in>; > }; > }; I think this is wrong, the AV96 can have two displays connected to two ports of the LTDC, just like DK2 for example.
Hi Marek On 4/15/21 3:21 PM, Marek Vasut wrote: > On 4/15/21 12:10 PM, Alexandre Torgue wrote: >> Running "make dtbs_check W=1", some warnings are reported concerning >> LTDC port subnode: >> >> /soc/display-controller@5a001000/port: >> unnecessary #address-cells/#size-cells without "ranges" or child "reg" >> property >> /soc/display-controller@5a001000/port: graph node has single child node >> 'endpoint', #address-cells/#size-cells are not necessary > > btw could you retain diffstat on your patches ? It's useful to see which > files changed right away. > [...] > >> diff --git a/arch/arm/boot/dts/stm32mp157c-dk2.dts >> b/arch/arm/boot/dts/stm32mp157c-dk2.dts >> index 2bc92ef3aeb9..19ef475a48fc 100644 >> --- a/arch/arm/boot/dts/stm32mp157c-dk2.dts >> +++ b/arch/arm/boot/dts/stm32mp157c-dk2.dts >> @@ -82,9 +82,15 @@ >> }; >> <dc { >> - status = "okay"; >> - >> port { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + ltdc_ep0_out: endpoint@0 { >> + reg = <0>; >> + remote-endpoint = <&sii9022_in>; >> + }; >> + >> ltdc_ep1_out: endpoint@1 { >> reg = <1>; >> remote-endpoint = <&dsi_in>; > > [...] > >> diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >> b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >> index 64dca5b7f748..e7f10975cacf 100644 >> --- a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >> +++ b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >> @@ -277,11 +277,7 @@ >> status = "okay"; >> port { >> - #address-cells = <1>; >> - #size-cells = <0>; >> - >> - ltdc_ep0_out: endpoint@0 { >> - reg = <0>; >> + ltdc_ep0_out: endpoint { >> remote-endpoint = <&adv7513_in>; >> }; >> }; > > I think this is wrong, the AV96 can have two displays connected to two > ports of the LTDC, just like DK2 for example. As for dk2 address/size cells are added only if there are 2 endpoints. It is for this reason I moved endpoint0 definition from stm32mp15xx-dkx to stm32mp151a-dk1.dts (dk1 has only one endpoint). Here it's the same, if you have second endpoint then adress/size will have to be added. alex
On 4/15/21 3:34 PM, Alexandre TORGUE wrote: > Hi Marek Hello Alexandre, >>> diff --git a/arch/arm/boot/dts/stm32mp157c-dk2.dts >>> b/arch/arm/boot/dts/stm32mp157c-dk2.dts >>> index 2bc92ef3aeb9..19ef475a48fc 100644 >>> --- a/arch/arm/boot/dts/stm32mp157c-dk2.dts >>> +++ b/arch/arm/boot/dts/stm32mp157c-dk2.dts >>> @@ -82,9 +82,15 @@ >>> }; >>> <dc { >>> - status = "okay"; >>> - >>> port { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + ltdc_ep0_out: endpoint@0 { >>> + reg = <0>; >>> + remote-endpoint = <&sii9022_in>; >>> + }; >>> + >>> ltdc_ep1_out: endpoint@1 { >>> reg = <1>; >>> remote-endpoint = <&dsi_in>; >> >> [...] >> >>> diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >>> b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >>> index 64dca5b7f748..e7f10975cacf 100644 >>> --- a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >>> +++ b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >>> @@ -277,11 +277,7 @@ >>> status = "okay"; >>> port { >>> - #address-cells = <1>; >>> - #size-cells = <0>; >>> - >>> - ltdc_ep0_out: endpoint@0 { >>> - reg = <0>; >>> + ltdc_ep0_out: endpoint { >>> remote-endpoint = <&adv7513_in>; >>> }; >>> }; >> >> I think this is wrong, the AV96 can have two displays connected to two >> ports of the LTDC, just like DK2 for example. > > As for dk2 address/size cells are added only if there are 2 endpoints. > It is for this reason I moved endpoint0 definition from stm32mp15xx-dkx > to stm32mp151a-dk1.dts (dk1 has only one endpoint). > > Here it's the same, if you have second endpoint then adress/size will > have to be added. That's a bit problematic. Consider either the use case of DTO which adds the other display, or even a custom board DTS. Without your patch, this works: arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi <dc { ... ports { ltdc_ep0_out: endpoint@0 { remote-endpoint = <&adv7513_in>; }; }; }; board-with-display.dts or board-overlay.dts <dc { ports { endpoint@1 { // just add another endpoint@1, no problem remote-endpoint = <&display>; }; }; }; With your patch, the DTS would have to modify the "endpoint" node to be "endpoint@0" probably with a whole lot of /detele-node/ etc. magic (DTO cannot do that, so that's a problem, and I do use DTOs on AV96 extensively for the various expansion cards) and then add the endpoint@1. That becomes real complicated in custom board DT, and impossible with DTO.
On 4/15/21 4:30 PM, Marek Vasut wrote: > On 4/15/21 3:34 PM, Alexandre TORGUE wrote: >> Hi Marek > > Hello Alexandre, > >>>> diff --git a/arch/arm/boot/dts/stm32mp157c-dk2.dts >>>> b/arch/arm/boot/dts/stm32mp157c-dk2.dts >>>> index 2bc92ef3aeb9..19ef475a48fc 100644 >>>> --- a/arch/arm/boot/dts/stm32mp157c-dk2.dts >>>> +++ b/arch/arm/boot/dts/stm32mp157c-dk2.dts >>>> @@ -82,9 +82,15 @@ >>>> }; >>>> <dc { >>>> - status = "okay"; >>>> - >>>> port { >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + >>>> + ltdc_ep0_out: endpoint@0 { >>>> + reg = <0>; >>>> + remote-endpoint = <&sii9022_in>; >>>> + }; >>>> + >>>> ltdc_ep1_out: endpoint@1 { >>>> reg = <1>; >>>> remote-endpoint = <&dsi_in>; >>> >>> [...] >>> >>>> diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >>>> b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >>>> index 64dca5b7f748..e7f10975cacf 100644 >>>> --- a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >>>> +++ b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >>>> @@ -277,11 +277,7 @@ >>>> status = "okay"; >>>> port { >>>> - #address-cells = <1>; >>>> - #size-cells = <0>; >>>> - >>>> - ltdc_ep0_out: endpoint@0 { >>>> - reg = <0>; >>>> + ltdc_ep0_out: endpoint { >>>> remote-endpoint = <&adv7513_in>; >>>> }; >>>> }; >>> >>> I think this is wrong, the AV96 can have two displays connected to >>> two ports of the LTDC, just like DK2 for example. >> >> As for dk2 address/size cells are added only if there are 2 endpoints. >> It is for this reason I moved endpoint0 definition from >> stm32mp15xx-dkx to stm32mp151a-dk1.dts (dk1 has only one endpoint). >> >> Here it's the same, if you have second endpoint then adress/size will >> have to be added. > > That's a bit problematic. Consider either the use case of DTO which adds > the other display, or even a custom board DTS. Without your patch, this > works: > > arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi > <dc { > ... > ports { > ltdc_ep0_out: endpoint@0 { > remote-endpoint = <&adv7513_in>; > }; > }; > }; > > board-with-display.dts or board-overlay.dts > <dc { > ports { > endpoint@1 { // just add another endpoint@1, no problem > remote-endpoint = <&display>; > }; > }; > }; > > With your patch, the DTS would have to modify the "endpoint" node to be > "endpoint@0" probably with a whole lot of /detele-node/ etc. magic (DTO > cannot do that, so that's a problem, and I do use DTOs on AV96 > extensively for the various expansion cards) and then add the > endpoint@1. That becomes real complicated in custom board DT, and > impossible with DTO. Yes I agree that it'll be problematic. So maybe so solution would be to not detect a warning for the initial case (only one endpoint with a reg)
On 4/15/21 4:35 PM, Alexandre TORGUE wrote: > > > On 4/15/21 4:30 PM, Marek Vasut wrote: >> On 4/15/21 3:34 PM, Alexandre TORGUE wrote: >>> Hi Marek >> >> Hello Alexandre, >> >>>>> diff --git a/arch/arm/boot/dts/stm32mp157c-dk2.dts >>>>> b/arch/arm/boot/dts/stm32mp157c-dk2.dts >>>>> index 2bc92ef3aeb9..19ef475a48fc 100644 >>>>> --- a/arch/arm/boot/dts/stm32mp157c-dk2.dts >>>>> +++ b/arch/arm/boot/dts/stm32mp157c-dk2.dts >>>>> @@ -82,9 +82,15 @@ >>>>> }; >>>>> <dc { >>>>> - status = "okay"; >>>>> - >>>>> port { >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + >>>>> + ltdc_ep0_out: endpoint@0 { >>>>> + reg = <0>; >>>>> + remote-endpoint = <&sii9022_in>; >>>>> + }; >>>>> + >>>>> ltdc_ep1_out: endpoint@1 { >>>>> reg = <1>; >>>>> remote-endpoint = <&dsi_in>; >>>> >>>> [...] >>>> >>>>> diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >>>>> b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >>>>> index 64dca5b7f748..e7f10975cacf 100644 >>>>> --- a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >>>>> +++ b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >>>>> @@ -277,11 +277,7 @@ >>>>> status = "okay"; >>>>> port { >>>>> - #address-cells = <1>; >>>>> - #size-cells = <0>; >>>>> - >>>>> - ltdc_ep0_out: endpoint@0 { >>>>> - reg = <0>; >>>>> + ltdc_ep0_out: endpoint { >>>>> remote-endpoint = <&adv7513_in>; >>>>> }; >>>>> }; >>>> >>>> I think this is wrong, the AV96 can have two displays connected to >>>> two ports of the LTDC, just like DK2 for example. >>> >>> As for dk2 address/size cells are added only if there are 2 >>> endpoints. It is for this reason I moved endpoint0 definition from >>> stm32mp15xx-dkx to stm32mp151a-dk1.dts (dk1 has only one endpoint). >>> >>> Here it's the same, if you have second endpoint then adress/size will >>> have to be added. >> >> That's a bit problematic. Consider either the use case of DTO which >> adds the other display, or even a custom board DTS. Without your >> patch, this works: >> >> arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi >> <dc { >> ... >> ports { >> ltdc_ep0_out: endpoint@0 { >> remote-endpoint = <&adv7513_in>; >> }; >> }; >> }; >> >> board-with-display.dts or board-overlay.dts >> <dc { >> ports { >> endpoint@1 { // just add another endpoint@1, no problem >> remote-endpoint = <&display>; >> }; >> }; >> }; >> >> With your patch, the DTS would have to modify the "endpoint" node to >> be "endpoint@0" probably with a whole lot of /detele-node/ etc. magic >> (DTO cannot do that, so that's a problem, and I do use DTOs on AV96 >> extensively for the various expansion cards) and then add the >> endpoint@1. That becomes real complicated in custom board DT, and >> impossible with DTO. > > Yes I agree that it'll be problematic. So maybe so solution would be to > not detect a warning for the initial case (only one endpoint with a reg) That looks OK. Or even better, if the checker warned only on IPs which cannot have more than one endpoint, but have endpoint@N in DT (where N in 0..+inf) . On IPs which can have one or more endpoints, the warning should not be emitted.
diff --git a/arch/arm/boot/dts/stm32f469-disco.dts b/arch/arm/boot/dts/stm32f469-disco.dts index 8c982ae79f43..f530f84474ea 100644 --- a/arch/arm/boot/dts/stm32f469-disco.dts +++ b/arch/arm/boot/dts/stm32f469-disco.dts @@ -175,7 +175,7 @@ status = "okay"; port { - ltdc_out_dsi: endpoint@0 { + ltdc_out_dsi: endpoint { remote-endpoint = <&dsi_in>; }; }; diff --git a/arch/arm/boot/dts/stm32mp151.dtsi b/arch/arm/boot/dts/stm32mp151.dtsi index 8aa87cb86821..98a703d1c3a0 100644 --- a/arch/arm/boot/dts/stm32mp151.dtsi +++ b/arch/arm/boot/dts/stm32mp151.dtsi @@ -1471,11 +1471,7 @@ resets = <&rcc LTDC_R>; status = "disabled"; - port { - #address-cells = <1>; - #size-cells = <0>; }; - }; iwdg2: watchdog@5a002000 { compatible = "st,stm32mp1-iwdg"; diff --git a/arch/arm/boot/dts/stm32mp157a-dk1.dts b/arch/arm/boot/dts/stm32mp157a-dk1.dts index 4c8be9c8eb20..c06763d24890 100644 --- a/arch/arm/boot/dts/stm32mp157a-dk1.dts +++ b/arch/arm/boot/dts/stm32mp157a-dk1.dts @@ -26,3 +26,11 @@ stdout-path = "serial0:115200n8"; }; }; + +<dc { + port { + ltdc_ep0_out: endpoint { + remote-endpoint = <&sii9022_in>; + }; + }; +}; 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 674b2d330dc4..ba1e2d7f06bf 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 = <&panel_in>; }; }; diff --git a/arch/arm/boot/dts/stm32mp157c-dk2.dts b/arch/arm/boot/dts/stm32mp157c-dk2.dts index 2bc92ef3aeb9..19ef475a48fc 100644 --- a/arch/arm/boot/dts/stm32mp157c-dk2.dts +++ b/arch/arm/boot/dts/stm32mp157c-dk2.dts @@ -82,9 +82,15 @@ }; <dc { - status = "okay"; - port { + #address-cells = <1>; + #size-cells = <0>; + + ltdc_ep0_out: endpoint@0 { + reg = <0>; + remote-endpoint = <&sii9022_in>; + }; + ltdc_ep1_out: endpoint@1 { reg = <1>; remote-endpoint = <&dsi_in>; diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts index 5c5b1ddf7bfd..6fe5b0fee7c4 100644 --- a/arch/arm/boot/dts/stm32mp157c-ev1.dts +++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts @@ -239,8 +239,7 @@ status = "okay"; port { - ltdc_ep0_out: endpoint@0 { - reg = <0>; + ltdc_ep0_out: endpoint { remote-endpoint = <&dsi_in>; }; }; diff --git a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts index 1e9bf7eea0f1..70202ef5267c 100644 --- a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts +++ b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts @@ -161,8 +161,7 @@ status = "okay"; port { - ltdc_ep0_out: endpoint@0 { - reg = <0>; + ltdc_ep0_out: endpoint { remote-endpoint = <&panel_input>; }; }; diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi index 64dca5b7f748..e7f10975cacf 100644 --- a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi +++ b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi @@ -277,11 +277,7 @@ status = "okay"; port { - #address-cells = <1>; - #size-cells = <0>; - - ltdc_ep0_out: endpoint@0 { - reg = <0>; + ltdc_ep0_out: endpoint { remote-endpoint = <&adv7513_in>; }; }; diff --git a/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi b/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi index 59f18846cf5d..ca2d21689ecc 100644 --- a/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi +++ b/arch/arm/boot/dts/stm32mp15xx-dkx.dtsi @@ -458,13 +458,6 @@ pinctrl-0 = <<dc_pins_a>; pinctrl-1 = <<dc_sleep_pins_a>; status = "okay"; - - port { - ltdc_ep0_out: endpoint@0 { - reg = <0>; - remote-endpoint = <&sii9022_in>; - }; - }; }; &m4_rproc {
Running "make dtbs_check W=1", some warnings are reported concerning LTDC port subnode: /soc/display-controller@5a001000/port: unnecessary #address-cells/#size-cells without "ranges" or child "reg" property /soc/display-controller@5a001000/port: graph node has single child node 'endpoint', #address-cells/#size-cells are not necessary Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com>