| Message ID | 20210415101037.1465-1-alexandre.torgue@foss.st.com |
|---|---|
| Headers | show |
| Series | ARM: dts: stm32: fix "make dtbs_check W=1" round1 | expand |
Hi, On 15.04.21 12:10, Alexandre Torgue wrote: > Running "make dtbs_check W=1", some warnings are reported concerning > DSI. This patch reorder DSI nodes to avoid: > > soc/dsi@5a000000: unnecessary #address-cells/#size-cells without > "ranges" or child "reg" property This reverts parts of commit 9c32f980d9 ("ARM: dts: stm32: preset stm32mp15x video #address- and #size-cells"): The cell count for address and size is defined by the binding and not something a board would change. Avoid each board adding this boilerplate by having the cell size specification in the SoC DTSI. The DSI can have child nodes with a unit address (e.g. a panel) and ones without (ports { } container). ports is described in the dtsi, panels are described in the dts if available. Apparently, the checker is fine with ports { #address-cells = <1>; #size-cells = <0>; }; I think my rationale for the patch above was sound, so I think the checker taking offense at the DSI cells here should be considered a false positive. Thanks, Ahmad > > Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com> > > diff --git a/arch/arm/boot/dts/stm32mp157.dtsi b/arch/arm/boot/dts/stm32mp157.dtsi > index 54e73ccea446..c355fcf26ec3 100644 > --- a/arch/arm/boot/dts/stm32mp157.dtsi > +++ b/arch/arm/boot/dts/stm32mp157.dtsi > @@ -24,8 +24,6 @@ > clock-names = "pclk", "ref", "px_clk"; > resets = <&rcc DSI_R>; > reset-names = "apb"; > - #address-cells = <1>; > - #size-cells = <0>; > status = "disabled"; > > ports { > diff --git a/arch/arm/boot/dts/stm32mp157c-dk2.dts b/arch/arm/boot/dts/stm32mp157c-dk2.dts > index 19ef475a48fc..763dde1dbbaf 100644 > --- a/arch/arm/boot/dts/stm32mp157c-dk2.dts > +++ b/arch/arm/boot/dts/stm32mp157c-dk2.dts > @@ -36,6 +36,8 @@ > &dsi { > status = "okay"; > phy-dsi-supply = <®18>; > + #address-cells = <1>; > + #size-cells = <0>; > > ports { > port@0 { > diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts > index 6fe5b0fee7c4..4625bb58cc6d 100644 > --- a/arch/arm/boot/dts/stm32mp157c-ev1.dts > +++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts > @@ -102,6 +102,8 @@ > &dsi { > phy-dsi-supply = <®18>; > status = "okay"; > + #address-cells = <1>; > + #size-cells = <0>; > > ports { > port@0 { >
Hi Ahmad On 4/15/21 12:43 PM, Ahmad Fatoum wrote: > Hi, > > On 15.04.21 12:10, Alexandre Torgue wrote: >> Running "make dtbs_check W=1", some warnings are reported concerning >> DSI. This patch reorder DSI nodes to avoid: >> >> soc/dsi@5a000000: unnecessary #address-cells/#size-cells without >> "ranges" or child "reg" property > > This reverts parts of commit 9c32f980d9 ("ARM: dts: stm32: preset > stm32mp15x video #address- and #size-cells"): > > The cell count for address and size is defined by the binding and not > something a board would change. Avoid each board adding this > boilerplate by having the cell size specification in the SoC DTSI. > > > The DSI can have child nodes with a unit address (e.g. a panel) and ones > without (ports { } container). ports is described in the dtsi, panels are > described in the dts if available. > > Apparently, the checker is fine with > ports { > #address-cells = <1>; > #size-cells = <0>; > }; > > I think my rationale for the patch above was sound, so I think the checker > taking offense at the DSI cells here should be considered a false positive. If it's a "false positive" warning then we need to find a way to not print it out. Else, it'll be difficult to distinguish which warnings are "normal" and which are not. This question could also be applied to patch[3]. Arnd, Rob what is your feeling about this case ? thanks alex > Thanks, > Ahmad > >> >> Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com> >> >> diff --git a/arch/arm/boot/dts/stm32mp157.dtsi b/arch/arm/boot/dts/stm32mp157.dtsi >> index 54e73ccea446..c355fcf26ec3 100644 >> --- a/arch/arm/boot/dts/stm32mp157.dtsi >> +++ b/arch/arm/boot/dts/stm32mp157.dtsi >> @@ -24,8 +24,6 @@ >> clock-names = "pclk", "ref", "px_clk"; >> resets = <&rcc DSI_R>; >> reset-names = "apb"; >> - #address-cells = <1>; >> - #size-cells = <0>; >> status = "disabled"; >> >> ports { >> diff --git a/arch/arm/boot/dts/stm32mp157c-dk2.dts b/arch/arm/boot/dts/stm32mp157c-dk2.dts >> index 19ef475a48fc..763dde1dbbaf 100644 >> --- a/arch/arm/boot/dts/stm32mp157c-dk2.dts >> +++ b/arch/arm/boot/dts/stm32mp157c-dk2.dts >> @@ -36,6 +36,8 @@ >> &dsi { >> status = "okay"; >> phy-dsi-supply = <®18>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> >> ports { >> port@0 { >> diff --git a/arch/arm/boot/dts/stm32mp157c-ev1.dts b/arch/arm/boot/dts/stm32mp157c-ev1.dts >> index 6fe5b0fee7c4..4625bb58cc6d 100644 >> --- a/arch/arm/boot/dts/stm32mp157c-ev1.dts >> +++ b/arch/arm/boot/dts/stm32mp157c-ev1.dts >> @@ -102,6 +102,8 @@ >> &dsi { >> phy-dsi-supply = <®18>; >> status = "okay"; >> + #address-cells = <1>; >> + #size-cells = <0>; >> >> ports { >> port@0 { >> >
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.
On 4/15/21 12:10 PM, Alexandre Torgue wrote: > Prevent warning seen with "make dtbs_check W=1" command: > > Warning (avoid_unnecessary_addr_size): /soc/timers@40001c00: unnecessary > address-cells/size-cells without "ranges" or child "reg" property > > Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com> Hi Alexandre, Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> Thanks, Fabrice > > diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi > index 41e0087bdbf9..8748d5850298 100644 > --- a/arch/arm/boot/dts/stm32f429.dtsi > +++ b/arch/arm/boot/dts/stm32f429.dtsi > @@ -283,8 +283,6 @@ > }; > > timers13: timers@40001c00 { > - #address-cells = <1>; > - #size-cells = <0>; > compatible = "st,stm32-timers"; > reg = <0x40001C00 0x400>; > clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM13)>; > @@ -299,8 +297,6 @@ > }; > > timers14: timers@40002000 { > - #address-cells = <1>; > - #size-cells = <0>; > compatible = "st,stm32-timers"; > reg = <0x40002000 0x400>; > clocks = <&rcc 0 STM32F4_APB1_CLOCK(TIM14)>; > @@ -633,8 +629,6 @@ > }; > > timers10: timers@40014400 { > - #address-cells = <1>; > - #size-cells = <0>; > compatible = "st,stm32-timers"; > reg = <0x40014400 0x400>; > clocks = <&rcc 0 STM32F4_APB2_CLOCK(TIM10)>; > @@ -649,8 +643,6 @@ > }; > > timers11: timers@40014800 { > - #address-cells = <1>; > - #size-cells = <0>; > compatible = "st,stm32-timers"; > reg = <0x40014800 0x400>; > clocks = <&rcc 0 STM32F4_APB2_CLOCK(TIM11)>; > diff --git a/arch/arm/boot/dts/stm32f746.dtsi b/arch/arm/boot/dts/stm32f746.dtsi > index e1df603fc981..72c1b76684b6 100644 > --- a/arch/arm/boot/dts/stm32f746.dtsi > +++ b/arch/arm/boot/dts/stm32f746.dtsi > @@ -265,8 +265,6 @@ > }; > > timers13: timers@40001c00 { > - #address-cells = <1>; > - #size-cells = <0>; > compatible = "st,stm32-timers"; > reg = <0x40001C00 0x400>; > clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM13)>; > @@ -281,8 +279,6 @@ > }; > > timers14: timers@40002000 { > - #address-cells = <1>; > - #size-cells = <0>; > compatible = "st,stm32-timers"; > reg = <0x40002000 0x400>; > clocks = <&rcc 0 STM32F7_APB1_CLOCK(TIM14)>; > @@ -531,8 +527,6 @@ > }; > > timers10: timers@40014400 { > - #address-cells = <1>; > - #size-cells = <0>; > compatible = "st,stm32-timers"; > reg = <0x40014400 0x400>; > clocks = <&rcc 0 STM32F7_APB2_CLOCK(TIM10)>; > @@ -547,8 +541,6 @@ > }; > > timers11: timers@40014800 { > - #address-cells = <1>; > - #size-cells = <0>; > compatible = "st,stm32-timers"; > reg = <0x40014800 0x400>; > clocks = <&rcc 0 STM32F7_APB2_CLOCK(TIM11)>; > diff --git a/arch/arm/boot/dts/stm32h743.dtsi b/arch/arm/boot/dts/stm32h743.dtsi > index 05ecdf9ff03a..6e42ca2dada2 100644 > --- a/arch/arm/boot/dts/stm32h743.dtsi > +++ b/arch/arm/boot/dts/stm32h743.dtsi > @@ -485,8 +485,6 @@ > }; > > lptimer4: timer@58002c00 { > - #address-cells = <1>; > - #size-cells = <0>; > compatible = "st,stm32-lptimer"; > reg = <0x58002c00 0x400>; > clocks = <&rcc LPTIM4_CK>; > @@ -501,8 +499,6 @@ > }; > > lptimer5: timer@58003000 { > - #address-cells = <1>; > - #size-cells = <0>; > compatible = "st,stm32-lptimer"; > reg = <0x58003000 0x400>; > clocks = <&rcc LPTIM5_CK>; >
On Thu, Apr 15, 2021 at 2:23 PM Alexandre TORGUE <alexandre.torgue@foss.st.com> wrote: > On 4/15/21 12:43 PM, Ahmad Fatoum wrote: > > On 15.04.21 12:10, Alexandre Torgue wrote: > >> Running "make dtbs_check W=1", some warnings are reported concerning > >> DSI. This patch reorder DSI nodes to avoid: > >> > >> soc/dsi@5a000000: unnecessary #address-cells/#size-cells without > >> "ranges" or child "reg" property > > > > This reverts parts of commit 9c32f980d9 ("ARM: dts: stm32: preset > > stm32mp15x video #address- and #size-cells"): > > > > The cell count for address and size is defined by the binding and not > > something a board would change. Avoid each board adding this > > boilerplate by having the cell size specification in the SoC DTSI. > > > > > > The DSI can have child nodes with a unit address (e.g. a panel) and ones > > without (ports { } container). ports is described in the dtsi, panels are > > described in the dts if available. > > > > Apparently, the checker is fine with > > ports { > > #address-cells = <1>; > > #size-cells = <0>; > > }; > > > > I think my rationale for the patch above was sound, so I think the checker > > taking offense at the DSI cells here should be considered a false positive. > > If it's a "false positive" warning then we need to find a way to not > print it out. Else, it'll be difficult to distinguish which warnings are > "normal" and which are not. This question could also be applied to patch[3]. > > Arnd, Rob what is your feeling about this case ? I don't have a strong opinion on this either way, but I would just not apply this one for 5.13 in this case. Rob, Alexandre, please let me know if I should apply the other patches before the merge window, I usually don't mind taking bugfixes late before the merge window, but I still want some level of confidence that they are actually correct. Ahmad, if you feel strongly about this particular issue, would you like to suggest a patch for the checker? Arnd
On 4/19/21 3:57 PM, Arnd Bergmann wrote: > On Thu, Apr 15, 2021 at 2:23 PM Alexandre TORGUE > <alexandre.torgue@foss.st.com> wrote: >> On 4/15/21 12:43 PM, Ahmad Fatoum wrote: >>> On 15.04.21 12:10, Alexandre Torgue wrote: >>>> Running "make dtbs_check W=1", some warnings are reported concerning >>>> DSI. This patch reorder DSI nodes to avoid: >>>> >>>> soc/dsi@5a000000: unnecessary #address-cells/#size-cells without >>>> "ranges" or child "reg" property >>> >>> This reverts parts of commit 9c32f980d9 ("ARM: dts: stm32: preset >>> stm32mp15x video #address- and #size-cells"): >>> >>> The cell count for address and size is defined by the binding and not >>> something a board would change. Avoid each board adding this >>> boilerplate by having the cell size specification in the SoC DTSI. >>> >>> >>> The DSI can have child nodes with a unit address (e.g. a panel) and ones >>> without (ports { } container). ports is described in the dtsi, panels are >>> described in the dts if available. >>> >>> Apparently, the checker is fine with >>> ports { >>> #address-cells = <1>; >>> #size-cells = <0>; >>> }; >>> >>> I think my rationale for the patch above was sound, so I think the checker >>> taking offense at the DSI cells here should be considered a false positive. >> >> If it's a "false positive" warning then we need to find a way to not >> print it out. Else, it'll be difficult to distinguish which warnings are >> "normal" and which are not. This question could also be applied to patch[3]. >> >> Arnd, Rob what is your feeling about this case ? > > I don't have a strong opinion on this either way, but I would just > not apply this one for 5.13 in this case. Rob, Alexandre, please > let me know if I should apply the other patches before the > merge window, I usually don't mind taking bugfixes late before the > merge window, but I still want some level of confidence that they > are actually correct. For me, we can keep this series for the v5.14 cycle. regards alex > > Ahmad, if you feel strongly about this particular issue, would you like > to suggest a patch for the checker? > > Arnd >
Hello Arnd, On 19.04.21 15:57, Arnd Bergmann wrote: > On Thu, Apr 15, 2021 at 2:23 PM Alexandre TORGUE > <alexandre.torgue@foss.st.com> wrote: >> On 4/15/21 12:43 PM, Ahmad Fatoum wrote: >>> I think my rationale for the patch above was sound, so I think the checker >>> taking offense at the DSI cells here should be considered a false positive. >> >> If it's a "false positive" warning then we need to find a way to not >> print it out. Else, it'll be difficult to distinguish which warnings are >> "normal" and which are not. This question could also be applied to patch[3]. >> >> Arnd, Rob what is your feeling about this case ? > > I don't have a strong opinion on this either way, but I would just > not apply this one for 5.13 in this case. Rob, Alexandre, please > let me know if I should apply the other patches before the > merge window, I usually don't mind taking bugfixes late before the > merge window, but I still want some level of confidence that they > are actually correct. > > Ahmad, if you feel strongly about this particular issue, would you like > to suggest a patch for the checker? The check is certainly useful. If it's not feasible to fix the checker (e.g. because it analyzes standalone DTSIs), I am fine with reverting my commit with an indication that this is to avoid triggering a dt-validate false positive. I am not familiar with dt-validate's inner workings to offer a patch. Cheers, Ahmad > > Arnd >
Hi On 4/15/21 12:10 PM, Alexandre Torgue wrote: > Hi, > > First round to cleanup warnings and yaml validation issues seen running > "make dtbs_check W=1" command for STM32 platform. It concerns all SoC > (MCU: f429/429, f746/769, h743, MPU) and all boards (ST reference boards, > DH, Engicam, LxA ...). > > Main fixes are done in device tree files but some imply a change in yaml > dt-bindings file. > > regards > Alex > > Alexandre Torgue (13): > ARM: dts: stm32: fix gpio-keys node on STM32 MCU boards > ARM: dts: stm32: fix RCC node name on stm32f429 MCU > ARM: dts: stm32: fix timer nodes on STM32 MCU to prevent warnings > dt-bindings: mfd: stm32-timers: remove #address/size cells from > required properties > ARM: dts: stm32: update pinctrl node name on STM32 MCU to prevent > warnings > ARM: dts: stm32: fix i2c node name on stm32f746 to prevent warnings > ARM: dts: stm32: move stmmac axi config in ethernet node on stm32mp15 > dt-bindings: net: document ptp_ref clk in dwmac > ARM: dts: stm32: fix stpmic node for stm32mp1 boards > dt-bindings: mfd: add vref_ddr-supply to st,stpmic1 yaml > ARM: dts: stm32: fix LTDC port node on STM32 MCU ad MPU > ARM: dts: stm32: fix DSI port node on STM32MP15 > ARM: dts: stm32: fix ltdc pinctrl on microdev2.0-of7 > > .../bindings/mfd/st,stm32-timers.yaml | 2 - > .../devicetree/bindings/mfd/st,stpmic1.yaml | 2 +- > .../devicetree/bindings/net/snps,dwmac.yaml | 4 +- > .../devicetree/bindings/net/stm32-dwmac.yaml | 6 +- > arch/arm/boot/dts/stm32429i-eval.dts | 8 +- > arch/arm/boot/dts/stm32746g-eval.dts | 6 +- > arch/arm/boot/dts/stm32f4-pinctrl.dtsi | 2 +- > arch/arm/boot/dts/stm32f429-disco.dts | 6 +- > arch/arm/boot/dts/stm32f429-pinctrl.dtsi | 72 +++++++++--------- > arch/arm/boot/dts/stm32f429.dtsi | 10 +-- > arch/arm/boot/dts/stm32f469-disco.dts | 8 +- > arch/arm/boot/dts/stm32f469-pinctrl.dtsi | 74 +++++++++---------- > arch/arm/boot/dts/stm32f7-pinctrl.dtsi | 2 +- > arch/arm/boot/dts/stm32f746.dtsi | 12 +-- > arch/arm/boot/dts/stm32f769-disco.dts | 6 +- > arch/arm/boot/dts/stm32h743.dtsi | 4 - > arch/arm/boot/dts/stm32mp151.dtsi | 16 ++-- > arch/arm/boot/dts/stm32mp157.dtsi | 2 - > arch/arm/boot/dts/stm32mp157a-dk1.dts | 8 ++ > ...157a-microgea-stm32mp1-microdev2.0-of7.dts | 5 +- > arch/arm/boot/dts/stm32mp157a-stinger96.dtsi | 7 +- > arch/arm/boot/dts/stm32mp157c-dk2.dts | 12 ++- > arch/arm/boot/dts/stm32mp157c-ev1.dts | 5 +- > arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts | 3 +- > .../arm/boot/dts/stm32mp157c-odyssey-som.dtsi | 5 +- > arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi | 5 +- > .../boot/dts/stm32mp15xx-dhcor-avenger96.dtsi | 6 +- > arch/arm/boot/dts/stm32mp15xx-dkx.dtsi | 7 -- > arch/arm/boot/dts/stm32mp15xx-osd32.dtsi | 7 +- > 29 files changed, 130 insertions(+), 182 deletions(-) > Patches 1 to 8 and 13 applied on stm32-next. I will send a v2 for vref-ddr supply. There is still an open point about #adress-size/cells check for DSI / LTDC ports. Rob, Can we consider to have "#adress-size/cells defined even if only one endpoint (child) is defined ? and then is it possible to update the checker ? Or do we have to keep patches [11][12] and update #adress-size/cells in board dts files ? Thanks Alex
On 4/15/21 12:10 PM, Alexandre Torgue wrote: > On some STM32 MP15 boards, stpmic node is not correct which generates > warnings running "make dtbs_check W=1" command. Issues are: > > -"regulator-active-discharge" is not a boolean but an uint32. > -"regulator-over-current-protection" is not a valid entry for vref_ddr. > -LDO4 has a fixed voltage (3v3) so min/max entries are not allowed. > > Signed-off-by: Alexandre Torgue <alexandre.torgue@foss.st.com> Applied on stm32-next. Thanks. Alex > > diff --git a/arch/arm/boot/dts/stm32mp157a-stinger96.dtsi b/arch/arm/boot/dts/stm32mp157a-stinger96.dtsi > index 113c48b2ef93..a4b14ef3caee 100644 > --- a/arch/arm/boot/dts/stm32mp157a-stinger96.dtsi > +++ b/arch/arm/boot/dts/stm32mp157a-stinger96.dtsi > @@ -184,8 +184,6 @@ > > vdd_usb: ldo4 { > regulator-name = "vdd_usb"; > - regulator-min-microvolt = <3300000>; > - regulator-max-microvolt = <3300000>; > interrupts = <IT_CURLIM_LDO4 0>; > }; > > @@ -208,7 +206,6 @@ > vref_ddr: vref_ddr { > regulator-name = "vref_ddr"; > regulator-always-on; > - regulator-over-current-protection; > }; > > bst_out: boost { > @@ -219,13 +216,13 @@ > vbus_otg: pwr_sw1 { > regulator-name = "vbus_otg"; > interrupts = <IT_OCP_OTG 0>; > - regulator-active-discharge; > + regulator-active-discharge = <1>; > }; > > vbus_sw: pwr_sw2 { > regulator-name = "vbus_sw"; > interrupts = <IT_OCP_SWOUT 0>; > - regulator-active-discharge; > + regulator-active-discharge = <1>; > }; > }; > > diff --git a/arch/arm/boot/dts/stm32mp157c-odyssey-som.dtsi b/arch/arm/boot/dts/stm32mp157c-odyssey-som.dtsi > index 6cf49a0a9e69..0c0b66788ea1 100644 > --- a/arch/arm/boot/dts/stm32mp157c-odyssey-som.dtsi > +++ b/arch/arm/boot/dts/stm32mp157c-odyssey-som.dtsi > @@ -173,8 +173,6 @@ > > vdd_usb: ldo4 { > regulator-name = "vdd_usb"; > - regulator-min-microvolt = <3300000>; > - regulator-max-microvolt = <3300000>; > interrupts = <IT_CURLIM_LDO4 0>; > }; > > @@ -197,7 +195,6 @@ > vref_ddr: vref_ddr { > regulator-name = "vref_ddr"; > regulator-always-on; > - regulator-over-current-protection; > }; > > bst_out: boost { > @@ -213,7 +210,7 @@ > vbus_sw: pwr_sw2 { > regulator-name = "vbus_sw"; > interrupts = <IT_OCP_SWOUT 0>; > - regulator-active-discharge; > + regulator-active-discharge = <1>; > }; > }; > > diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi b/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi > index 272a1a67a9ad..769fcf74685a 100644 > --- a/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi > +++ b/arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi > @@ -327,8 +327,6 @@ > > vdd_usb: ldo4 { > regulator-name = "vdd_usb"; > - regulator-min-microvolt = <3300000>; > - regulator-max-microvolt = <3300000>; > interrupts = <IT_CURLIM_LDO4 0>; > }; > > @@ -350,7 +348,6 @@ > vref_ddr: vref_ddr { > regulator-name = "vref_ddr"; > regulator-always-on; > - regulator-over-current-protection; > }; > > bst_out: boost { > @@ -366,7 +363,7 @@ > vbus_sw: pwr_sw2 { > regulator-name = "vbus_sw"; > interrupts = <IT_OCP_SWOUT 0>; > - regulator-active-discharge; > + regulator-active-discharge = <1>; > }; > }; > > diff --git a/arch/arm/boot/dts/stm32mp15xx-osd32.dtsi b/arch/arm/boot/dts/stm32mp15xx-osd32.dtsi > index 713485a95795..6706d8311a66 100644 > --- a/arch/arm/boot/dts/stm32mp15xx-osd32.dtsi > +++ b/arch/arm/boot/dts/stm32mp15xx-osd32.dtsi > @@ -146,8 +146,6 @@ > > vdd_usb: ldo4 { > regulator-name = "vdd_usb"; > - regulator-min-microvolt = <3300000>; > - regulator-max-microvolt = <3300000>; > interrupts = <IT_CURLIM_LDO4 0>; > }; > > @@ -171,7 +169,6 @@ > vref_ddr: vref_ddr { > regulator-name = "vref_ddr"; > regulator-always-on; > - regulator-over-current-protection; > }; > > bst_out: boost { > @@ -182,13 +179,13 @@ > vbus_otg: pwr_sw1 { > regulator-name = "vbus_otg"; > interrupts = <IT_OCP_OTG 0>; > - regulator-active-discharge; > + regulator-active-discharge = <1>; > }; > > vbus_sw: pwr_sw2 { > regulator-name = "vbus_sw"; > interrupts = <IT_OCP_SWOUT 0>; > - regulator-active-discharge; > + regulator-active-discharge = <1>; > }; > }; > >
Hi, First round to cleanup warnings and yaml validation issues seen running "make dtbs_check W=1" command for STM32 platform. It concerns all SoC (MCU: f429/429, f746/769, h743, MPU) and all boards (ST reference boards, DH, Engicam, LxA ...). Main fixes are done in device tree files but some imply a change in yaml dt-bindings file. regards Alex Alexandre Torgue (13): ARM: dts: stm32: fix gpio-keys node on STM32 MCU boards ARM: dts: stm32: fix RCC node name on stm32f429 MCU ARM: dts: stm32: fix timer nodes on STM32 MCU to prevent warnings dt-bindings: mfd: stm32-timers: remove #address/size cells from required properties ARM: dts: stm32: update pinctrl node name on STM32 MCU to prevent warnings ARM: dts: stm32: fix i2c node name on stm32f746 to prevent warnings ARM: dts: stm32: move stmmac axi config in ethernet node on stm32mp15 dt-bindings: net: document ptp_ref clk in dwmac ARM: dts: stm32: fix stpmic node for stm32mp1 boards dt-bindings: mfd: add vref_ddr-supply to st,stpmic1 yaml ARM: dts: stm32: fix LTDC port node on STM32 MCU ad MPU ARM: dts: stm32: fix DSI port node on STM32MP15 ARM: dts: stm32: fix ltdc pinctrl on microdev2.0-of7 .../bindings/mfd/st,stm32-timers.yaml | 2 - .../devicetree/bindings/mfd/st,stpmic1.yaml | 2 +- .../devicetree/bindings/net/snps,dwmac.yaml | 4 +- .../devicetree/bindings/net/stm32-dwmac.yaml | 6 +- arch/arm/boot/dts/stm32429i-eval.dts | 8 +- arch/arm/boot/dts/stm32746g-eval.dts | 6 +- arch/arm/boot/dts/stm32f4-pinctrl.dtsi | 2 +- arch/arm/boot/dts/stm32f429-disco.dts | 6 +- arch/arm/boot/dts/stm32f429-pinctrl.dtsi | 72 +++++++++--------- arch/arm/boot/dts/stm32f429.dtsi | 10 +-- arch/arm/boot/dts/stm32f469-disco.dts | 8 +- arch/arm/boot/dts/stm32f469-pinctrl.dtsi | 74 +++++++++---------- arch/arm/boot/dts/stm32f7-pinctrl.dtsi | 2 +- arch/arm/boot/dts/stm32f746.dtsi | 12 +-- arch/arm/boot/dts/stm32f769-disco.dts | 6 +- arch/arm/boot/dts/stm32h743.dtsi | 4 - arch/arm/boot/dts/stm32mp151.dtsi | 16 ++-- arch/arm/boot/dts/stm32mp157.dtsi | 2 - arch/arm/boot/dts/stm32mp157a-dk1.dts | 8 ++ ...157a-microgea-stm32mp1-microdev2.0-of7.dts | 5 +- arch/arm/boot/dts/stm32mp157a-stinger96.dtsi | 7 +- arch/arm/boot/dts/stm32mp157c-dk2.dts | 12 ++- arch/arm/boot/dts/stm32mp157c-ev1.dts | 5 +- arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts | 3 +- .../arm/boot/dts/stm32mp157c-odyssey-som.dtsi | 5 +- arch/arm/boot/dts/stm32mp15xx-dhcom-som.dtsi | 5 +- .../boot/dts/stm32mp15xx-dhcor-avenger96.dtsi | 6 +- arch/arm/boot/dts/stm32mp15xx-dkx.dtsi | 7 -- arch/arm/boot/dts/stm32mp15xx-osd32.dtsi | 7 +- 29 files changed, 130 insertions(+), 182 deletions(-)