| Message ID | 20201117161942.38754-3-nsekhar@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: ti: J7200 GPIO support and warning fixes | expand |
Hi Rob, On 17/11/2020 18:19, Sekhar Nori wrote: > With dtc 1.6.0, building TI device-tree files with W=2 results in warnings > like below for all interrupt controllers. > > /bus@100000/bus@30000000/interrupt-controller1: Missing #address-cells in interrupt provider > > Fix these by adding #address-cells = <0>; for all interrupt controllers in > TI device-tree files. Any other #address-cells value is really only needed > if interrupt-map property is being used (which is not the case for existing > TI device-tree files) > > Signed-off-by: Sekhar Nori <nsekhar@ti.com> > --- > arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 5 +++++ > arch/arm64/boot/dts/ti/k3-am65-wakeup.dtsi | 2 ++ > arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 1 + > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 3 +++ > arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 1 + > arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts | 1 + > arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 11 +++++++++++ > arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi | 3 +++ > 8 files changed, 27 insertions(+) > > diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > index aa8725db0187..55aaa1404d7d 100644 > --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > @@ -440,6 +440,7 @@ > interrupt-controller; > interrupt-parent = <&gic500>; > #interrupt-cells = <1>; > + #address-cells = <0>; Does it really required or mandatory to have #address-cells = <0>; defined for interrupt-controller DT nodes which do not have child nodes and no "interrupt-map"? -- Best regards, grygorii
On 13:38-20201118, Grygorii Strashko wrote: > Hi Rob, > > On 17/11/2020 18:19, Sekhar Nori wrote: > > With dtc 1.6.0, building TI device-tree files with W=2 results in warnings > > like below for all interrupt controllers. > > > > /bus@100000/bus@30000000/interrupt-controller1: Missing #address-cells in interrupt provider > > > > Fix these by adding #address-cells = <0>; for all interrupt controllers in > > TI device-tree files. Any other #address-cells value is really only needed > > if interrupt-map property is being used (which is not the case for existing > > TI device-tree files) > > > > Signed-off-by: Sekhar Nori <nsekhar@ti.com> > > --- > > arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 5 +++++ > > arch/arm64/boot/dts/ti/k3-am65-wakeup.dtsi | 2 ++ > > arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 1 + > > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 3 +++ > > arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 1 + > > arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts | 1 + > > arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 11 +++++++++++ > > arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi | 3 +++ > > 8 files changed, 27 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > > index aa8725db0187..55aaa1404d7d 100644 > > --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > > +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > > @@ -440,6 +440,7 @@ > > interrupt-controller; > > interrupt-parent = <&gic500>; > > #interrupt-cells = <1>; > > + #address-cells = <0>; > Does it really required or mandatory to have #address-cells = <0>; defined for interrupt-controller DT nodes which > do not have child nodes and no "interrupt-map"? Just to help clarify (I could be mistaken as well): is'nt the interrupt map for user interrupt map nodes that refer to this interrupt controller node to state they dont need a parent address specifier - so are we claiming none of the users will have an interrupt-map (now and never in the future as well) - we we might want to explain why we think that is the case, and if we are expecting dtc to deduce that (if so how?)? -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
On 18/11/2020 17:12, Nishanth Menon wrote: > On 13:38-20201118, Grygorii Strashko wrote: >> Hi Rob, >> >> On 17/11/2020 18:19, Sekhar Nori wrote: >>> With dtc 1.6.0, building TI device-tree files with W=2 results in warnings >>> like below for all interrupt controllers. >>> >>> /bus@100000/bus@30000000/interrupt-controller1: Missing #address-cells in interrupt provider >>> >>> Fix these by adding #address-cells = <0>; for all interrupt controllers in >>> TI device-tree files. Any other #address-cells value is really only needed >>> if interrupt-map property is being used (which is not the case for existing >>> TI device-tree files) >>> >>> Signed-off-by: Sekhar Nori <nsekhar@ti.com> >>> --- >>> arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 5 +++++ >>> arch/arm64/boot/dts/ti/k3-am65-wakeup.dtsi | 2 ++ >>> arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 1 + >>> arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 3 +++ >>> arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 1 + >>> arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts | 1 + >>> arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 11 +++++++++++ >>> arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi | 3 +++ >>> 8 files changed, 27 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi >>> index aa8725db0187..55aaa1404d7d 100644 >>> --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi >>> +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi >>> @@ -440,6 +440,7 @@ >>> interrupt-controller; >>> interrupt-parent = <&gic500>; >>> #interrupt-cells = <1>; >>> + #address-cells = <0>; >> Does it really required or mandatory to have #address-cells = <0>; defined for interrupt-controller DT nodes which >> do not have child nodes and no "interrupt-map"? > > Just to help clarify (I could be mistaken as well): is'nt the > interrupt map for user interrupt map nodes that refer to this > interrupt controller node to state they dont need a parent address > specifier - so are we claiming none of the users will have an > interrupt-map (now and never in the future as well) - we we might want > to explain why we think that is the case, and if we are expecting dtc > to deduce that (if so how?)? > The main reason I commented - is hope to get some clarification from DT maintainers. 90% of interrupt-controller nodes do not have #address-cells and I never seen in in GPIO nodes (most often is present in PCI and GIC nodes). and nobody seems fixing it. So, if we are going to move this direction it's reasonable to get clarification to be sure. And there is no "never" here - #address-cells always can be added if really required. -- Best regards, grygorii
Punting over to Rob and DT team's wisdom.. On 13:17-20201119, Grygorii Strashko wrote: > > > On 18/11/2020 17:12, Nishanth Menon wrote: > > On 13:38-20201118, Grygorii Strashko wrote: > > > Hi Rob, > > > > > > On 17/11/2020 18:19, Sekhar Nori wrote: > > > > With dtc 1.6.0, building TI device-tree files with W=2 results in warnings > > > > like below for all interrupt controllers. > > > > > > > > /bus@100000/bus@30000000/interrupt-controller1: Missing #address-cells in interrupt provider > > > > > > > > Fix these by adding #address-cells = <0>; for all interrupt controllers in > > > > TI device-tree files. Any other #address-cells value is really only needed > > > > if interrupt-map property is being used (which is not the case for existing > > > > TI device-tree files) > > > > > > > > Signed-off-by: Sekhar Nori <nsekhar@ti.com> > > > > --- > > > > arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 5 +++++ > > > > arch/arm64/boot/dts/ti/k3-am65-wakeup.dtsi | 2 ++ > > > > arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 1 + > > > > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 3 +++ > > > > arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 1 + > > > > arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts | 1 + > > > > arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 11 +++++++++++ > > > > arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi | 3 +++ > > > > 8 files changed, 27 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > > > > index aa8725db0187..55aaa1404d7d 100644 > > > > --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > > > > +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > > > > @@ -440,6 +440,7 @@ > > > > interrupt-controller; > > > > interrupt-parent = <&gic500>; > > > > #interrupt-cells = <1>; > > > > + #address-cells = <0>; > > > Does it really required or mandatory to have #address-cells = <0>; defined for interrupt-controller DT nodes which > > > do not have child nodes and no "interrupt-map"? > > > > Just to help clarify (I could be mistaken as well): is'nt the > > interrupt map for user interrupt map nodes that refer to this > > interrupt controller node to state they dont need a parent address > > specifier - so are we claiming none of the users will have an > > interrupt-map (now and never in the future as well) - we we might want > > to explain why we think that is the case, and if we are expecting dtc > > to deduce that (if so how?)? > > > > The main reason I commented - is hope to get some clarification from DT maintainers. > 90% of interrupt-controller nodes do not have #address-cells and I never seen in in GPIO nodes > (most often is present in PCI and GIC nodes). > and nobody seems fixing it. So, if we are going to move this direction it's reasonable to get clarification to be sure. > > And there is no "never" here - #address-cells always can be added if really required. OK - as a GPIO node, but as an interrupt-controller node, I was looking at [1] and wondering if that was the precedence. Yes, will be good to get direction from the DT maintainers on this topic. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/interrupt-controller/open-pic.txt -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
On 19/11/20 6:58 PM, Nishanth Menon wrote: > Punting over to Rob and DT team's wisdom.. > > On 13:17-20201119, Grygorii Strashko wrote: >> >> >> On 18/11/2020 17:12, Nishanth Menon wrote: >>> On 13:38-20201118, Grygorii Strashko wrote: >>>> Hi Rob, >>>> >>>> On 17/11/2020 18:19, Sekhar Nori wrote: >>>>> With dtc 1.6.0, building TI device-tree files with W=2 results in warnings >>>>> like below for all interrupt controllers. >>>>> >>>>> /bus@100000/bus@30000000/interrupt-controller1: Missing #address-cells in interrupt provider >>>>> >>>>> Fix these by adding #address-cells = <0>; for all interrupt controllers in >>>>> TI device-tree files. Any other #address-cells value is really only needed >>>>> if interrupt-map property is being used (which is not the case for existing >>>>> TI device-tree files) >>>>> >>>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com> >>>>> --- >>>>> arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 5 +++++ >>>>> arch/arm64/boot/dts/ti/k3-am65-wakeup.dtsi | 2 ++ >>>>> arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 1 + >>>>> arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 3 +++ >>>>> arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 1 + >>>>> arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts | 1 + >>>>> arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 11 +++++++++++ >>>>> arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi | 3 +++ >>>>> 8 files changed, 27 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi >>>>> index aa8725db0187..55aaa1404d7d 100644 >>>>> --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi >>>>> +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi >>>>> @@ -440,6 +440,7 @@ >>>>> interrupt-controller; >>>>> interrupt-parent = <&gic500>; >>>>> #interrupt-cells = <1>; >>>>> + #address-cells = <0>; >>>> Does it really required or mandatory to have #address-cells = <0>; defined for interrupt-controller DT nodes which >>>> do not have child nodes and no "interrupt-map"? >>> >>> Just to help clarify (I could be mistaken as well): is'nt the >>> interrupt map for user interrupt map nodes that refer to this >>> interrupt controller node to state they dont need a parent address >>> specifier - so are we claiming none of the users will have an >>> interrupt-map (now and never in the future as well) - we we might want >>> to explain why we think that is the case, and if we are expecting dtc >>> to deduce that (if so how?)? >>> >> >> The main reason I commented - is hope to get some clarification from DT maintainers. >> 90% of interrupt-controller nodes do not have #address-cells and I never seen in in GPIO nodes >> (most often is present in PCI and GIC nodes). >> and nobody seems fixing it. So, if we are going to move this direction it's reasonable to get clarification to be sure. >> >> And there is no "never" here - #address-cells always can be added if really required. > > > OK - as a GPIO node, but as an interrupt-controller node, I was > looking at [1] and wondering if that was the precedence. > > Yes, will be good to get direction from the DT maintainers on this > topic. Shall I respin this series with 2/4 dropped while we wait for decision on this? #address-cells warnings on interrupt controller can perhaps be handled all at once (there are many of those in existing DT anyway). GPIO is basic support and holds up many other modules (like MMC/SD). Thanks, Sekhar
On 09:45-20201123, Sekhar Nori wrote: > >> The main reason I commented - is hope to get some clarification from DT maintainers. > >> 90% of interrupt-controller nodes do not have #address-cells and I never seen in in GPIO nodes > >> (most often is present in PCI and GIC nodes). > >> and nobody seems fixing it. So, if we are going to move this direction it's reasonable to get clarification to be sure. > >> > >> And there is no "never" here - #address-cells always can be added if really required. > > > > > > OK - as a GPIO node, but as an interrupt-controller node, I was > > looking at [1] and wondering if that was the precedence. > > > > Yes, will be good to get direction from the DT maintainers on this > > topic. > > Shall I respin this series with 2/4 dropped while we wait for decision > on this? > > #address-cells warnings on interrupt controller can perhaps be handled > all at once (there are many of those in existing DT anyway). > > GPIO is basic support and holds up many other modules (like MMC/SD). There are'nt too many new patches in my queue that depends on GPIO, I'd rather not introduce new warnings unless we are completely at a stalemate. I'd rather use this opportunity to understand where what we need to be doing. -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
On 24/11/20 6:51 AM, Nishanth Menon wrote: > On 09:45-20201123, Sekhar Nori wrote: >>>> The main reason I commented - is hope to get some clarification from DT maintainers. >>>> 90% of interrupt-controller nodes do not have #address-cells and I never seen in in GPIO nodes >>>> (most often is present in PCI and GIC nodes). >>>> and nobody seems fixing it. So, if we are going to move this direction it's reasonable to get clarification to be sure. >>>> >>>> And there is no "never" here - #address-cells always can be added if really required. >>> >>> >>> OK - as a GPIO node, but as an interrupt-controller node, I was >>> looking at [1] and wondering if that was the precedence. >>> >>> Yes, will be good to get direction from the DT maintainers on this >>> topic. >> >> Shall I respin this series with 2/4 dropped while we wait for decision >> on this? >> >> #address-cells warnings on interrupt controller can perhaps be handled >> all at once (there are many of those in existing DT anyway). >> >> GPIO is basic support and holds up many other modules (like MMC/SD). > > > There are'nt too many new patches in my queue that depends on GPIO, I'd > rather not introduce new warnings unless we are completely at a > stalemate. I'd rather use this opportunity to understand where what we > need to be doing. GPIO was originally submitted as part of 8 patch series titled "[PATCH 0/8] Add support for UHS modes in TI's J721e and J7200 boards" Rest of those patches need to be resubmitted after GPIO is accepted. Can you apply patch 1/4 at least. Its fairly non-controversial. It will help reduce patch backlog and fix some warnings too. Thanks, Sekhar
On 09:46-20201124, Sekhar Nori wrote: > On 24/11/20 6:51 AM, Nishanth Menon wrote: > > On 09:45-20201123, Sekhar Nori wrote: > >>>> The main reason I commented - is hope to get some clarification from DT maintainers. > >>>> 90% of interrupt-controller nodes do not have #address-cells and I never seen in in GPIO nodes > >>>> (most often is present in PCI and GIC nodes). > >>>> and nobody seems fixing it. So, if we are going to move this direction it's reasonable to get clarification to be sure. > >>>> > >>>> And there is no "never" here - #address-cells always can be added if really required. > >>> > >>> > >>> OK - as a GPIO node, but as an interrupt-controller node, I was > >>> looking at [1] and wondering if that was the precedence. > >>> > >>> Yes, will be good to get direction from the DT maintainers on this > >>> topic. > >> > >> Shall I respin this series with 2/4 dropped while we wait for decision > >> on this? > >> > >> #address-cells warnings on interrupt controller can perhaps be handled > >> all at once (there are many of those in existing DT anyway). > >> > >> GPIO is basic support and holds up many other modules (like MMC/SD). > > > > > > There are'nt too many new patches in my queue that depends on GPIO, I'd > > rather not introduce new warnings unless we are completely at a > > stalemate. I'd rather use this opportunity to understand where what we > > need to be doing. > GPIO was originally submitted as part of 8 patch series titled "[PATCH > 0/8] Add support for UHS modes in TI's J721e and J7200 boards" > > Rest of those patches need to be resubmitted after GPIO is accepted. > > Can you apply patch 1/4 at least. Its fairly non-controversial. It will > help reduce patch backlog and fix some warnings too. I see that Grygorii is suggesting 1,3,4 to be pulled in. can you repost with just the required patches alone and pick up the reviewed-bys? -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
Hi Rob, Grygorii, On 27/11/20 7:53 pm, Nishanth Menon wrote: > On 09:46-20201124, Sekhar Nori wrote: >> On 24/11/20 6:51 AM, Nishanth Menon wrote: >>> On 09:45-20201123, Sekhar Nori wrote: >>>>>> The main reason I commented - is hope to get some clarification from DT maintainers. >>>>>> 90% of interrupt-controller nodes do not have #address-cells and I never seen in in GPIO nodes >>>>>> (most often is present in PCI and GIC nodes). >>>>>> and nobody seems fixing it. So, if we are going to move this direction it's reasonable to get clarification to be sure. >>>>>> >>>>>> And there is no "never" here - #address-cells always can be added if really required. >>>>> >>>>> >>>>> OK - as a GPIO node, but as an interrupt-controller node, I was >>>>> looking at [1] and wondering if that was the precedence. >>>>> >>>>> Yes, will be good to get direction from the DT maintainers on this >>>>> topic. >>>> Is there a conclusion on this topic? Without adding address-cells for interrupt controller we will be introducing new warning for all the new nodes we are adding. Thanks and regards, Lokesh
On Thu, Nov 19, 2020 at 01:17:36PM +0200, Grygorii Strashko wrote: > > > On 18/11/2020 17:12, Nishanth Menon wrote: > > On 13:38-20201118, Grygorii Strashko wrote: > > > Hi Rob, > > > > > > On 17/11/2020 18:19, Sekhar Nori wrote: > > > > With dtc 1.6.0, building TI device-tree files with W=2 results in warnings > > > > like below for all interrupt controllers. > > > > > > > > /bus@100000/bus@30000000/interrupt-controller1: Missing #address-cells in interrupt provider > > > > > > > > Fix these by adding #address-cells = <0>; for all interrupt controllers in > > > > TI device-tree files. Any other #address-cells value is really only needed > > > > if interrupt-map property is being used (which is not the case for existing > > > > TI device-tree files) > > > > > > > > Signed-off-by: Sekhar Nori <nsekhar@ti.com> > > > > --- > > > > arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 5 +++++ > > > > arch/arm64/boot/dts/ti/k3-am65-wakeup.dtsi | 2 ++ > > > > arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 1 + > > > > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 3 +++ > > > > arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 1 + > > > > arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts | 1 + > > > > arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 11 +++++++++++ > > > > arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi | 3 +++ > > > > 8 files changed, 27 insertions(+) > > > > > > > > diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > > > > index aa8725db0187..55aaa1404d7d 100644 > > > > --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > > > > +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > > > > @@ -440,6 +440,7 @@ > > > > interrupt-controller; > > > > interrupt-parent = <&gic500>; > > > > #interrupt-cells = <1>; > > > > + #address-cells = <0>; > > > Does it really required or mandatory to have #address-cells = <0>; defined for interrupt-controller DT nodes which > > > do not have child nodes and no "interrupt-map"? > > > > Just to help clarify (I could be mistaken as well): is'nt the > > interrupt map for user interrupt map nodes that refer to this > > interrupt controller node to state they dont need a parent address > > specifier - so are we claiming none of the users will have an > > interrupt-map (now and never in the future as well) - we we might want > > to explain why we think that is the case, and if we are expecting dtc > > to deduce that (if so how?)? > > > > The main reason I commented - is hope to get some clarification from DT maintainers. > 90% of interrupt-controller nodes do not have #address-cells and I never seen in in GPIO nodes > (most often is present in PCI and GIC nodes). > and nobody seems fixing it. So, if we are going to move this direction it's reasonable to get clarification to be sure. > > And there is no "never" here - #address-cells always can be added if really required. Once required, how does one figure that out? It's not obvious and requires booting. So we need something at build time. I'm okay with loosening the check as long as it warns when a interrupt parent phandle in an interrupt-map is missing '#address-cells'. Now that I look back at the dtc change, I'm now confused why this check got applied. Both David and I wanted changes in regards to #address-cells. Either a separate check or part of interrupt-map checks. And the interrupt-map check never got applied. Andre? Rob
On Mon, 25 Jan 2021 18:01:08 -0600 Rob Herring <robh@kernel.org> wrote: Hi, > On Thu, Nov 19, 2020 at 01:17:36PM +0200, Grygorii Strashko wrote: > > > > > > On 18/11/2020 17:12, Nishanth Menon wrote: > > > On 13:38-20201118, Grygorii Strashko wrote: > > > > Hi Rob, > > > > > > > > On 17/11/2020 18:19, Sekhar Nori wrote: > > > > > With dtc 1.6.0, building TI device-tree files with W=2 results in warnings > > > > > like below for all interrupt controllers. > > > > > > > > > > /bus@100000/bus@30000000/interrupt-controller1: Missing #address-cells in interrupt provider > > > > > > > > > > Fix these by adding #address-cells = <0>; for all interrupt controllers in > > > > > TI device-tree files. Any other #address-cells value is really only needed > > > > > if interrupt-map property is being used (which is not the case for existing > > > > > TI device-tree files) > > > > > > > > > > Signed-off-by: Sekhar Nori <nsekhar@ti.com> > > > > > --- > > > > > arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 5 +++++ > > > > > arch/arm64/boot/dts/ti/k3-am65-wakeup.dtsi | 2 ++ > > > > > arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 1 + > > > > > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 3 +++ > > > > > arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 1 + > > > > > arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts | 1 + > > > > > arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 11 +++++++++++ > > > > > arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi | 3 +++ > > > > > 8 files changed, 27 insertions(+) > > > > > > > > > > diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > > > > > index aa8725db0187..55aaa1404d7d 100644 > > > > > --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > > > > > +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi > > > > > @@ -440,6 +440,7 @@ > > > > > interrupt-controller; > > > > > interrupt-parent = <&gic500>; > > > > > #interrupt-cells = <1>; > > > > > + #address-cells = <0>; > > > > Does it really required or mandatory to have #address-cells = <0>; defined for interrupt-controller DT nodes which > > > > do not have child nodes and no "interrupt-map"? > > > > > > Just to help clarify (I could be mistaken as well): is'nt the > > > interrupt map for user interrupt map nodes that refer to this > > > interrupt controller node to state they dont need a parent address > > > specifier - so are we claiming none of the users will have an > > > interrupt-map (now and never in the future as well) - we we might want > > > to explain why we think that is the case, and if we are expecting dtc > > > to deduce that (if so how?)? > > > > > > > The main reason I commented - is hope to get some clarification from DT maintainers. > > 90% of interrupt-controller nodes do not have #address-cells and I never seen in in GPIO nodes > > (most often is present in PCI and GIC nodes). > > and nobody seems fixing it. So, if we are going to move this direction it's reasonable to get clarification to be sure. > > > > And there is no "never" here - #address-cells always can be added if really required. > > Once required, how does one figure that out? It's not obvious and > requires booting. So we need something at build time. I'm okay with > loosening the check as long as it warns when a interrupt parent phandle > in an interrupt-map is missing '#address-cells'. So I think the rationale for requiring #address-cells is that the usage of an interrupt in an interrupt-map can be totally disconnected from the actual interrupt controller node. Typically the controller is in the .dtsi, but an interrupt map could be anywhere, down in some board .dts, or even some "common peripherals" intermediate .dts. Possibly even in an overlay (I2C IRQ lines?). So while not having this property works today, for your board, it might surprisingly break for someone else. And those things are hard to find (unless you know what you are looking for). Been there, done that with the VExpress DTs, and that was the reason I pushed for more tests. On top of that is the standard's default value of "2" for #address-cells, which Linux observes in this case. That leads to somewhat surprising results when interpreting interrupt-maps without an explicit #address-cells (cost me a few hours to figure out back then!) So given the already somewhat complicated nature of interrupt-maps I think it's comparably little to ask for explicit #address-cells properties, even though you might not immediately benefit from it. > Now that I look back at the dtc change, I'm now confused why this > check got applied. Both David and I wanted changes in regards to > #address-cells. Either a separate check or part of interrupt-map checks. > And the interrupt-map check never got applied. Andre? Yeah, I somewhat dropped the ball on this, after some iterations and a partial merge. Will put it on my list to revive this. Cheers, Andre.
Andre, Rob, On 16:38-20210126, Andre Przywara wrote: > > Now that I look back at the dtc change, I'm now confused why this > > check got applied. Both David and I wanted changes in regards to > > #address-cells. Either a separate check or part of interrupt-map checks. > > And the interrupt-map check never got applied. Andre? > > Yeah, I somewhat dropped the ball on this, after some iterations and a > partial merge. Will put it on my list to revive this. I was hoping we made some steps, but I did see [1] as well and it is possible that I am missing some discussion, but it is starting to get W=2 builds warnings noisy enough to start interfering with discovering real problems as we keep adding new stuff in.. Just wondering... [1] https://lore.kernel.org/linux-devicetree/CAL_Jsq++DyiKG9smQGx9FAPDJnVrezcXNb0Y5uh-5_2GBzTQpQ@mail.gmail.com/ [2] https://pastebin.ubuntu.com/p/ns6hPCBxVM/ -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
diff --git a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi index aa8725db0187..55aaa1404d7d 100644 --- a/arch/arm64/boot/dts/ti/k3-am65-main.dtsi +++ b/arch/arm64/boot/dts/ti/k3-am65-main.dtsi @@ -440,6 +440,7 @@ interrupt-controller; interrupt-parent = <&gic500>; #interrupt-cells = <1>; + #address-cells = <0>; ti,sci = <&dmsc>; ti,sci-dev-id = <100>; ti,interrupt-ranges = <0 392 32>; @@ -461,6 +462,7 @@ interrupt-controller; interrupt-parent = <&gic500>; #interrupt-cells = <1>; + #address-cells = <0>; ti,sci = <&dmsc>; ti,sci-dev-id = <182>; ti,interrupt-ranges = <0 64 64>, @@ -474,6 +476,7 @@ interrupt-parent = <&intr_main_navss>; msi-controller; #interrupt-cells = <0>; + #address-cells = <0>; ti,sci = <&dmsc>; ti,sci-dev-id = <179>; ti,interrupt-ranges = <0 0 256>; @@ -670,6 +673,7 @@ interrupts = <192>, <193>, <194>, <195>, <196>, <197>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; ti,ngpio = <96>; ti,davinci-gpio-unbanked = <0>; clocks = <&k3_clks 57 0>; @@ -685,6 +689,7 @@ interrupts = <200>, <201>, <202>, <203>, <204>, <205>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; ti,ngpio = <90>; ti,davinci-gpio-unbanked = <0>; clocks = <&k3_clks 58 0>; diff --git a/arch/arm64/boot/dts/ti/k3-am65-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am65-wakeup.dtsi index ed42f13e7663..7fe5782a1f79 100644 --- a/arch/arm64/boot/dts/ti/k3-am65-wakeup.dtsi +++ b/arch/arm64/boot/dts/ti/k3-am65-wakeup.dtsi @@ -75,6 +75,7 @@ interrupt-controller; interrupt-parent = <&gic500>; #interrupt-cells = <1>; + #address-cells = <0>; ti,sci = <&dmsc>; ti,sci-dev-id = <156>; ti,interrupt-ranges = <0 712 16>; @@ -89,6 +90,7 @@ interrupts = <60>, <61>, <62>, <63>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; ti,ngpio = <56>; ti,davinci-gpio-unbanked = <0>; clocks = <&k3_clks 59 0>; diff --git a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts index d12dd89f3405..376de272cb4e 100644 --- a/arch/arm64/boot/dts/ti/k3-am654-base-board.dts +++ b/arch/arm64/boot/dts/ti/k3-am654-base-board.dts @@ -236,6 +236,7 @@ interrupts = <25 IRQ_TYPE_EDGE_FALLING>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; }; }; diff --git a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi index 72d6496e88dd..d07081b20aee 100644 --- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi +++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi @@ -67,6 +67,7 @@ interrupt-controller; interrupt-parent = <&gic500>; #interrupt-cells = <1>; + #address-cells = <0>; ti,sci = <&dmsc>; ti,sci-dev-id = <131>; ti,interrupt-ranges = <8 392 56>; @@ -85,6 +86,7 @@ interrupt-controller; interrupt-parent = <&gic500>; #interrupt-cells = <1>; + #address-cells = <0>; ti,sci = <&dmsc>; ti,sci-dev-id = <213>; ti,interrupt-ranges = <0 64 64>, @@ -97,6 +99,7 @@ reg = <0x00 0x33d00000 0x00 0x100000>; interrupt-controller; #interrupt-cells = <0>; + #address-cells = <0>; interrupt-parent = <&main_navss_intr>; msi-controller; ti,sci = <&dmsc>; diff --git a/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi index eb2a78a53512..4801876bd107 100644 --- a/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi +++ b/arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi @@ -102,6 +102,7 @@ interrupt-controller; interrupt-parent = <&gic500>; #interrupt-cells = <1>; + #address-cells = <0>; ti,sci = <&dmsc>; ti,sci-dev-id = <137>; ti,interrupt-ranges = <16 960 16>; diff --git a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts index 52e121155563..0490cb15f0c9 100644 --- a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts +++ b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts @@ -442,6 +442,7 @@ interrupts = <11 IRQ_TYPE_EDGE_FALLING>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; }; }; diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi index ffedd9531362..7f44692e15ec 100644 --- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi +++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi @@ -114,6 +114,7 @@ interrupt-controller; interrupt-parent = <&gic500>; #interrupt-cells = <1>; + #address-cells = <0>; ti,sci = <&dmsc>; ti,sci-dev-id = <131>; ti,interrupt-ranges = <8 392 56>; @@ -135,6 +136,7 @@ interrupt-controller; interrupt-parent = <&gic500>; #interrupt-cells = <1>; + #address-cells = <0>; ti,sci = <&dmsc>; ti,sci-dev-id = <213>; ti,interrupt-ranges = <0 64 64>, @@ -149,6 +151,7 @@ interrupt-parent = <&main_navss_intr>; msi-controller; #interrupt-cells = <0>; + #address-cells = <0>; ti,sci = <&dmsc>; ti,sci-dev-id = <209>; ti,interrupt-ranges = <0 0 256>; @@ -948,6 +951,7 @@ <260>, <261>, <262>, <263>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; ti,ngpio = <128>; ti,davinci-gpio-unbanked = <0>; power-domains = <&k3_pds 105 TI_SCI_PD_EXCLUSIVE>; @@ -964,6 +968,7 @@ interrupts = <288>, <289>, <290>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; ti,ngpio = <36>; ti,davinci-gpio-unbanked = <0>; power-domains = <&k3_pds 106 TI_SCI_PD_EXCLUSIVE>; @@ -981,6 +986,7 @@ <268>, <269>, <270>, <271>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; ti,ngpio = <128>; ti,davinci-gpio-unbanked = <0>; power-domains = <&k3_pds 107 TI_SCI_PD_EXCLUSIVE>; @@ -997,6 +1003,7 @@ interrupts = <292>, <293>, <294>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; ti,ngpio = <36>; ti,davinci-gpio-unbanked = <0>; power-domains = <&k3_pds 108 TI_SCI_PD_EXCLUSIVE>; @@ -1014,6 +1021,7 @@ <276>, <277>, <278>, <279>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; ti,ngpio = <128>; ti,davinci-gpio-unbanked = <0>; power-domains = <&k3_pds 109 TI_SCI_PD_EXCLUSIVE>; @@ -1030,6 +1038,7 @@ interrupts = <296>, <297>, <298>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; ti,ngpio = <36>; ti,davinci-gpio-unbanked = <0>; power-domains = <&k3_pds 110 TI_SCI_PD_EXCLUSIVE>; @@ -1047,6 +1056,7 @@ <284>, <285>, <286>, <287>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; ti,ngpio = <128>; ti,davinci-gpio-unbanked = <0>; power-domains = <&k3_pds 111 TI_SCI_PD_EXCLUSIVE>; @@ -1063,6 +1073,7 @@ interrupts = <300>, <301>, <302>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; ti,ngpio = <36>; ti,davinci-gpio-unbanked = <0>; power-domains = <&k3_pds 112 TI_SCI_PD_EXCLUSIVE>; diff --git a/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi index e581cb1d87ee..ed3098ed7b56 100644 --- a/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi +++ b/arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi @@ -102,6 +102,7 @@ interrupt-controller; interrupt-parent = <&gic500>; #interrupt-cells = <1>; + #address-cells = <0>; ti,sci = <&dmsc>; ti,sci-dev-id = <137>; ti,interrupt-ranges = <16 960 16>; @@ -116,6 +117,7 @@ interrupts = <103>, <104>, <105>, <106>, <107>, <108>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; ti,ngpio = <84>; ti,davinci-gpio-unbanked = <0>; power-domains = <&k3_pds 113 TI_SCI_PD_EXCLUSIVE>; @@ -132,6 +134,7 @@ interrupts = <112>, <113>, <114>, <115>, <116>, <117>; interrupt-controller; #interrupt-cells = <2>; + #address-cells = <0>; ti,ngpio = <84>; ti,davinci-gpio-unbanked = <0>; power-domains = <&k3_pds 114 TI_SCI_PD_EXCLUSIVE>;
With dtc 1.6.0, building TI device-tree files with W=2 results in warnings like below for all interrupt controllers. /bus@100000/bus@30000000/interrupt-controller1: Missing #address-cells in interrupt provider Fix these by adding #address-cells = <0>; for all interrupt controllers in TI device-tree files. Any other #address-cells value is really only needed if interrupt-map property is being used (which is not the case for existing TI device-tree files) Signed-off-by: Sekhar Nori <nsekhar@ti.com> --- arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 5 +++++ arch/arm64/boot/dts/ti/k3-am65-wakeup.dtsi | 2 ++ arch/arm64/boot/dts/ti/k3-am654-base-board.dts | 1 + arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 3 +++ arch/arm64/boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 1 + arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts | 1 + arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 11 +++++++++++ arch/arm64/boot/dts/ti/k3-j721e-mcu-wakeup.dtsi | 3 +++ 8 files changed, 27 insertions(+)