| Message ID | 20201102191120.20380-1-faiz_abbas@ti.com |
|---|---|
| Headers | show |
| Series | Add gpio support for TI's J7200 platform | expand |
On 03/11/20 12:41 am, Faiz Abbas wrote: > The following patches add gpio support for TI's J7200 platform. > > These patches were posted as a part of an older series but have now > been split into three parts. The 3 parts add configs, gpios and MMC/SD > related dts patches respectively. Series looks good to me. Reviewed-by: Lokesh Vutla <lokeshvutla@ti.com> Thanks and regards, Lokesh
On 00:41-20201103, Faiz Abbas wrote: > There are 4 instances of gpio modules in main domain: > gpio0, gpio2, gpio4 and gpio6 > > Groups are created to provide protection between different processor virtual > worlds. Each of these modules I/O pins are muxed within the group. Exactly > one module can be selected to control the corresponding pin by selecting it > in the pad mux configuration registers. Could you check with checkpatch --strict please? I see: WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) > > This group pins out 69 lines (5 banks). > > Add DT modes for each module instance in the main domain. > > Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> > --- > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 68 +++++++++++++++++++++++ dtbs_check: we added: arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@600000: Missing #address-cells in interrupt provider arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@610000: Missing #address-cells in interrupt provider arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@620000: Missing #address-cells in interrupt provider arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@630000: Missing #address-cells in interrupt provider > 1 file changed, 68 insertions(+) > > diff --git a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi > index 72d6496e88dd..c22ef2efa531 100644 > --- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi > +++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi > @@ -446,4 +446,72 @@ > dr_mode = "otg"; > }; > }; > + > + main_gpio0: gpio@600000 { > + compatible = "ti,j721e-gpio", "ti,keystone-gpio"; > + reg = <0x00 0x00600000 0x00 0x100>; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-parent = <&main_gpio_intr>; > + interrupts = <145>, <146>, <147>, <148>, > + <149>; > + interrupt-controller; > + #interrupt-cells = <2>; > + ti,ngpio = <69>; > + ti,davinci-gpio-unbanked = <0>; > + power-domains = <&k3_pds 105 TI_SCI_PD_EXCLUSIVE>; > + clocks = <&k3_clks 105 0>; > + clock-names = "gpio"; > + }; > + > + main_gpio2: gpio@610000 { > + compatible = "ti,j721e-gpio", "ti,keystone-gpio"; > + reg = <0x00 0x00610000 0x00 0x100>; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-parent = <&main_gpio_intr>; > + interrupts = <154>, <155>, <156>, <157>, > + <158>; > + interrupt-controller; > + #interrupt-cells = <2>; > + ti,ngpio = <69>; > + ti,davinci-gpio-unbanked = <0>; > + power-domains = <&k3_pds 107 TI_SCI_PD_EXCLUSIVE>; > + clocks = <&k3_clks 107 0>; > + clock-names = "gpio"; > + }; > + > + main_gpio4: gpio@620000 { > + compatible = "ti,j721e-gpio", "ti,keystone-gpio"; > + reg = <0x00 0x00620000 0x00 0x100>; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-parent = <&main_gpio_intr>; > + interrupts = <163>, <164>, <165>, <166>, > + <167>; > + interrupt-controller; > + #interrupt-cells = <2>; > + ti,ngpio = <69>; > + ti,davinci-gpio-unbanked = <0>; > + power-domains = <&k3_pds 109 TI_SCI_PD_EXCLUSIVE>; > + clocks = <&k3_clks 109 0>; > + clock-names = "gpio"; > + }; > + > + main_gpio6: gpio@630000 { > + compatible = "ti,j721e-gpio", "ti,keystone-gpio"; > + reg = <0x00 0x00630000 0x00 0x100>; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-parent = <&main_gpio_intr>; > + interrupts = <172>, <173>, <174>, <175>, > + <176>; > + interrupt-controller; > + #interrupt-cells = <2>; > + ti,ngpio = <69>; > + ti,davinci-gpio-unbanked = <0>; > + power-domains = <&k3_pds 111 TI_SCI_PD_EXCLUSIVE>; > + clocks = <&k3_clks 111 0>; > + clock-names = "gpio"; > + }; > }; > -- > 2.17.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
On 00:41-20201103, Faiz Abbas wrote: > The following patches add gpio support for TI's J7200 platform. > > These patches were posted as a part of an older series but have now > been split into three parts. The 3 parts add configs, gpios and MMC/SD > related dts patches respectively. > > Older series is here: > https://lore.kernel.org/linux-arm-kernel/20201001190541.6364-1-faiz_abbas@ti.com/ > > Series adding configs to arm64 defconfig is here: > https://lore.kernel.org/linux-arm-kernel/20201102183005.14174-1-faiz_abbas@ti.com/ > > Faiz Abbas (3): > arm64: dts: ti: k3-j7200-main: Add gpio nodes in main domain > arm64: dts: ti: k3-j7200: Add gpio nodes in wakeup domain I am not sure why we are splitting patches per domain here. We should just have a single patch that introduces the nodes, I dont see a specific benefit. In addition, series also introduces additional Missing #address-cells in interrupt provider Which we need a conclusion for and the comments already provided. -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
Hi Nishanth, On 12/11/20 10:09 PM, Nishanth Menon wrote: > On 00:41-20201103, Faiz Abbas wrote: >> There are 4 instances of gpio modules in main domain: >> gpio0, gpio2, gpio4 and gpio6 >> >> Groups are created to provide protection between different processor virtual >> worlds. Each of these modules I/O pins are muxed within the group. Exactly >> one module can be selected to control the corresponding pin by selecting it >> in the pad mux configuration registers. > Could you check with checkpatch --strict please? > > I see: > > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) > >> >> This group pins out 69 lines (5 banks). >> >> Add DT modes for each module instance in the main domain. >> >> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com> >> --- >> arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 68 +++++++++++++++++++++++ > > dtbs_check: we added: > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@600000: Missing #address-cells in interrupt provider > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@610000: Missing #address-cells in interrupt provider > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@620000: Missing #address-cells in interrupt provider > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@630000: Missing #address-cells in interrupt provider Hmm, running dtbs_check, I did not really see this. These are all the warnings I see for TI platforms: https://pastebin.ubuntu.com/p/m2my62mjQq/ The tree I am testing is linux-next of 12th Nov + these three patches applied. Also, #address-cells for interrupt provider being compulsory does not make full sense to me. Nothing in Documentation/devicetree/bindings/interrupt-controller/interrupts.txt or Documentation/devicetree/bindings/gpio/gpio-davinci.txt suggests that as well. Existing GPIO nodes for AM654 or J721E does not have #address-cells as well. Adding Grygorii as well, in case he knows more about this. Thanks, Sekhar
On 23:59-20201113, Sekhar Nori wrote: [..] > > dtbs_check: we added: > > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@600000: Missing #address-cells in interrupt provider > > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@610000: Missing #address-cells in interrupt provider > > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@620000: Missing #address-cells in interrupt provider > > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@630000: Missing #address-cells in interrupt provider > > Hmm, running dtbs_check, I did not really see this. These are all the > warnings I see for TI platforms: https://pastebin.ubuntu.com/p/m2my62mjQq/ Here is the full list of checks I ran through with kernel_patch_verify (docker) https://pastebin.ubuntu.com/p/tcnWw89CMD/ See lines 128 onwards for this series. kernel_patch_verify does'nt complain on existing warnings, but just prints when there are additional ones added in. Also make sure we have the right dtc as well dtc 1.6.0 and dt_schema 2020.8.1 was used. > > The tree I am testing is linux-next of 12th Nov + these three patches > applied. > > Also, #address-cells for interrupt provider being compulsory does not > make full sense to me. Nothing in > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt or > Documentation/devicetree/bindings/gpio/gpio-davinci.txt suggests that as > well. > > Existing GPIO nodes for AM654 or J721E does not have #address-cells as well. > > Adding Grygorii as well, in case he knows more about this. Yes - we need to have this conversation in the community :) I had tagged this internally already during the 5.10 merge cycle that we need to clean up the #address-cells warning and in some cases, maybe the bindings are probably not accurate to attempt an enforcement. I'd really like a conclusion on the topic as I recollect Lokesh and Grygorii had a debate internally, but reached no conclusion, lets get the wisdom of the community to help us here. [1] https://github.com/nmenon/kernel_patch_verify/blob/master/kpv -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
On 14/11/20 12:10 AM, Nishanth Menon wrote: > On 23:59-20201113, Sekhar Nori wrote: > [..] >>> dtbs_check: we added: >>> arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@600000: Missing #address-cells in interrupt provider >>> arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@610000: Missing #address-cells in interrupt provider >>> arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@620000: Missing #address-cells in interrupt provider >>> arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/gpio@630000: Missing #address-cells in interrupt provider >> >> Hmm, running dtbs_check, I did not really see this. These are all the >> warnings I see for TI platforms: https://pastebin.ubuntu.com/p/m2my62mjQq/ > > Here is the full list of checks I ran through with kernel_patch_verify > (docker) > https://pastebin.ubuntu.com/p/tcnWw89CMD/ > > See lines 128 onwards for this series. kernel_patch_verify does'nt > complain on existing warnings, but just prints when there are additional > ones added in. Also make sure we have the right dtc as well > dtc 1.6.0 and dt_schema 2020.8.1 was used. I was using the latest schema from master. But I changed to 2020.08.1 also, and still don't see the warning. $ dt-doc-validate --version 2020.12.dev1+gab5a73fcef26 I dont have a system-wide dtc installed. One in kernel tree is updated. $ scripts/dtc/dtc --version Version: DTC 1.6.0-gcbca977e Looking at your logs, it looks like you have more patches than just this applied. I wonder if thats making a difference. Can you check with just these patches applied to linux-next or share your tree which includes other patches? In your logs, you have such error for other interrupt controller nodes as well. For example: arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: /bus@100000/bus@30000000/interrupt-controller1: Missing #address-cells in interrupt provider Which I don't see in my logs. My guess is some other patch(es) in your patch stack either uncovers this warning or causes it. > >> >> The tree I am testing is linux-next of 12th Nov + these three patches >> applied. >> >> Also, #address-cells for interrupt provider being compulsory does not >> make full sense to me. Nothing in >> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt or >> Documentation/devicetree/bindings/gpio/gpio-davinci.txt suggests that as >> well. >> >> Existing GPIO nodes for AM654 or J721E does not have #address-cells as well. >> >> Adding Grygorii as well, in case he knows more about this. > > > Yes - we need to have this conversation in the community :) I had > tagged this internally already during the 5.10 merge cycle that we > need to clean up the #address-cells warning and in some cases, maybe > the bindings are probably not accurate to attempt an enforcement. > I'd really like a conclusion on the topic as I recollect Lokesh and > Grygorii had a debate internally, but reached no conclusion, lets get > the wisdom of the community to help us here. Adding Lokesh to cc as well. Thanks, Sekhar
On 00:39-20201114, Sekhar Nori wrote: > > I was using the latest schema from master. But I changed to 2020.08.1 > also, and still don't see the warning. > > $ dt-doc-validate --version > 2020.12.dev1+gab5a73fcef26 > > I dont have a system-wide dtc installed. One in kernel tree is updated. > > $ scripts/dtc/dtc --version > Version: DTC 1.6.0-gcbca977e > > Looking at your logs, it looks like you have more patches than just this > applied. I wonder if thats making a difference. Can you check with just > these patches applied to linux-next or share your tree which includes > other patches? > > In your logs, you have such error for other interrupt controller nodes > as well. For example: > > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: > /bus@100000/bus@30000000/interrupt-controller1: Missing #address-cells > in interrupt provider > > Which I don't see in my logs. My guess is some other patch(es) in your > patch stack either uncovers this warning or causes it. Oh boy! I sent you and myself on wild goose chase! Really sorry about messing up in the report of bug. It is not dtbs_check, it is building dtbs with W=2 that generates this warning. dtc 1.6.0 is sufficient to reproduce this behavior. Using v5.10-rc1 as baseline (happens the same with next-20201113 as well. v5.10-rc1: https://pastebin.ubuntu.com/p/Pn9HDqRjQ4/ (recording: https://asciinema.org/a/55YVpql9Bq8rh8fePTxI2xObO) v5.10-rc1 + 1st patch in the series(since we are testing): https://pastebin.ubuntu.com/p/QWQRMSv565/ (recording: https://asciinema.org/a/ZSKZkOY13l4lmZ2xWH34jMlM1) Diff: https://pastebin.ubuntu.com/p/239sYYT2QY/ -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
Hi On 13/11/2020 22:55, Nishanth Menon wrote: > On 00:39-20201114, Sekhar Nori wrote: >> >> I was using the latest schema from master. But I changed to 2020.08.1 >> also, and still don't see the warning. >> >> $ dt-doc-validate --version >> 2020.12.dev1+gab5a73fcef26 >> >> I dont have a system-wide dtc installed. One in kernel tree is updated. >> >> $ scripts/dtc/dtc --version >> Version: DTC 1.6.0-gcbca977e >> >> Looking at your logs, it looks like you have more patches than just this >> applied. I wonder if thats making a difference. Can you check with just >> these patches applied to linux-next or share your tree which includes >> other patches? >> >> In your logs, you have such error for other interrupt controller nodes >> as well. For example: >> >> arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: >> /bus@100000/bus@30000000/interrupt-controller1: Missing #address-cells >> in interrupt provider >> >> Which I don't see in my logs. My guess is some other patch(es) in your >> patch stack either uncovers this warning or causes it. > > Oh boy! I sent you and myself on wild goose chase! Really sorry about > messing up in the report of bug. > > It is not dtbs_check, it is building dtbs with W=2 that generates this > warning. dtc 1.6.0 is sufficient to reproduce this behavior. > > Using v5.10-rc1 as baseline (happens the same with next-20201113 as > well. > > v5.10-rc1: https://pastebin.ubuntu.com/p/Pn9HDqRjQ4/ (recording: > https://asciinema.org/a/55YVpql9Bq8rh8fePTxI2xObO) > > v5.10-rc1 + 1st patch in the series(since we are testing): > https://pastebin.ubuntu.com/p/QWQRMSv565/ (recording: > https://asciinema.org/a/ZSKZkOY13l4lmZ2xWH34jMlM1) > > Diff: https://pastebin.ubuntu.com/p/239sYYT2QY/ > This warning come from scripts/dtc/checks.c and was introduced by commit 3eb619b2f7d8 ("scripts/dtc: Update to upstream version v1.6.0-11-g9d7888cbf19c"). In my opinion it's false warning as there is no requirement to have #address-cells in interrupt provider node. by the way, above commit description says: "The interrupt_provider check is noisy, so turn it off for now." -- Best regards, grygorii
On 14/11/20 9:45 AM, Grygorii Strashko wrote: > Hi > > On 13/11/2020 22:55, Nishanth Menon wrote: >> On 00:39-20201114, Sekhar Nori wrote: >>> >>> I was using the latest schema from master. But I changed to 2020.08.1 >>> also, and still don't see the warning. >>> >>> $ dt-doc-validate --version >>> 2020.12.dev1+gab5a73fcef26 >>> >>> I dont have a system-wide dtc installed. One in kernel tree is updated. >>> >>> $ scripts/dtc/dtc --version >>> Version: DTC 1.6.0-gcbca977e >>> >>> Looking at your logs, it looks like you have more patches than just this >>> applied. I wonder if thats making a difference. Can you check with just >>> these patches applied to linux-next or share your tree which includes >>> other patches? >>> >>> In your logs, you have such error for other interrupt controller nodes >>> as well. For example: >>> >>> arch/arm64/boot/dts/ti/k3-j7200-main.dtsi: >>> /bus@100000/bus@30000000/interrupt-controller1: Missing #address-cells >>> in interrupt provider >>> >>> Which I don't see in my logs. My guess is some other patch(es) in your >>> patch stack either uncovers this warning or causes it. >> >> Oh boy! I sent you and myself on wild goose chase! Really sorry about >> messing up in the report of bug. >> >> It is not dtbs_check, it is building dtbs with W=2 that generates this >> warning. dtc 1.6.0 is sufficient to reproduce this behavior. >> >> Using v5.10-rc1 as baseline (happens the same with next-20201113 as >> well. >> >> v5.10-rc1: https://pastebin.ubuntu.com/p/Pn9HDqRjQ4/ (recording: >> https://asciinema.org/a/55YVpql9Bq8rh8fePTxI2xObO) >> >> v5.10-rc1 + 1st patch in the series(since we are testing): >> https://pastebin.ubuntu.com/p/QWQRMSv565/ (recording: >> https://asciinema.org/a/ZSKZkOY13l4lmZ2xWH34jMlM1) >> >> Diff: https://pastebin.ubuntu.com/p/239sYYT2QY/ >> > > This warning come from scripts/dtc/checks.c > and was introduced by commit 3eb619b2f7d8 ("scripts/dtc: Update to > upstream version v1.6.0-11-g9d7888cbf19c"). > > In my opinion it's false warning as there is no requirement to have > #address-cells in interrupt provider node. > by the way, above commit description says: "The interrupt_provider check > is noisy, so turn it off for now." Adding Andre who adding this check in upstream dtc for guidance. It looks like address-cells makes sense only if there is an interrupt-map specified as well. Since we don't use it, I can add #address-cells = <0>; to silence the warning. Let me know if there is a better way to deal with this. Thanks, Sekhar
The following patches add gpio support for TI's J7200 platform. These patches were posted as a part of an older series but have now been split into three parts. The 3 parts add configs, gpios and MMC/SD related dts patches respectively. Older series is here: https://lore.kernel.org/linux-arm-kernel/20201001190541.6364-1-faiz_abbas@ti.com/ Series adding configs to arm64 defconfig is here: https://lore.kernel.org/linux-arm-kernel/20201102183005.14174-1-faiz_abbas@ti.com/ Faiz Abbas (3): arm64: dts: ti: k3-j7200-main: Add gpio nodes in main domain arm64: dts: ti: k3-j7200: Add gpio nodes in wakeup domain arm64: dts: ti: k3-j7200-common-proc-board: Disable unused gpio modules .../dts/ti/k3-j7200-common-proc-board.dts | 16 +++++ arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 68 +++++++++++++++++++ .../boot/dts/ti/k3-j7200-mcu-wakeup.dtsi | 32 +++++++++ 3 files changed, 116 insertions(+)