| Message ID | 1396653256-28397-3-git-send-email-soren.brinkmann@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Soren, On 04/05/2014 01:14 AM, Soren Brinkmann wrote: > Convert all Zynq DT files to the dtc preprocessor include syntax. > This allows to include header files in the devicetrees like other > SoC-types already do. > > Inspired-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> > (http://www.spinics.net/lists/arm-kernel/msg319832.html) > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> These 4 patches needs more wider discussion if this is helpful or not. Currently I can't see any value in it because everything is just generated and fixed. I think I had the same discussion with Laurent some weeks ago regarding this. IRC the origin idea to use this was especially for people who writing these DTS by hand which is not our case - at least for majority of our customers. Thanks, Michal
?On 04/07/2014 07:58 AM, Michal Simek wrote: > Hi Soren, > > On 04/05/2014 01:14 AM, Soren Brinkmann wrote: >> Convert all Zynq DT files to the dtc preprocessor include syntax. >> This allows to include header files in the devicetrees like other >> SoC-types already do. >> >> Inspired-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> >> (http://www.spinics.net/lists/arm-kernel/msg319832.html) >> >> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > These 4 patches needs more wider discussion if this is helpful or > not. Currently I can't see any value in it because everything > is just generated and fixed. I think I had the same discussion > with Laurent some weeks ago regarding this. I would be kinda neutral here. I'd consider it helpful, it improves readability (regardless of whether they are generated or hand crafted). That's convenient for things like interrupt sensitivity, I can't remember whether 4 is level or edge type. On the other hand, the clock indices are just as much magic numbers as the memory addresses. If I suspect an error in that area, I'd start by lokking in /sys/kernel/debug/clk but wouldn't start in the DT. > IRC the origin idea to use this was especially for people who > writing these DTS by hand which is not our case - at least > for majority of our customers. I write them by hand. Is there any other way? Mike. Met vriendelijke groet / kind regards, Mike Looijmans TOPIC Embedded Systems Eindhovenseweg 32-C, NL-5683 KH Best Postbus 440, NL-5680 AK Best Telefoon: (+31) (0) 499 33 69 79 Telefax: (+31) (0) 499 33 69 70 E-mail: mike.looijmans@topic.nl Website: www.topic.nl Please consider the environment before printing this e-mail Visit us at the Hannover Messe 7 - 11 April 2014 - Hall 002/D10 (Dutch Pavillion) http://www.hannovermesse.de/exhibitor/topic-embedded-products/V229623
Hi Mike, On 04/07/2014 02:17 PM, Mike Looijmans wrote: > On 04/07/2014 07:58 AM, Michal Simek wrote: >> Hi Soren, >> >> On 04/05/2014 01:14 AM, Soren Brinkmann wrote: >>> Convert all Zynq DT files to the dtc preprocessor include syntax. >>> This allows to include header files in the devicetrees like other >>> SoC-types already do. >>> >>> Inspired-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> >>> (http://www.spinics.net/lists/arm-kernel/msg319832.html) >>> >>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> >> >> These 4 patches needs more wider discussion if this is helpful or >> not. Currently I can't see any value in it because everything >> is just generated and fixed. I think I had the same discussion >> with Laurent some weeks ago regarding this. > > I would be kinda neutral here. I'd consider it helpful, it improves readability (regardless of whether they are generated or hand crafted). That's convenient for things like interrupt sensitivity, I can't remember whether 4 is level or edge type. On the other hand, the clock indices are just as much magic numbers as the memory addresses. If I suspect an error in that area, I'd start by lokking in /sys/kernel/debug/clk but wouldn't start in the DT. > >> IRC the origin idea to use this was especially for people who >> writing these DTS by hand which is not our case - at least >> for majority of our customers. > > I write them by hand. Is there any other way? Device-tree BSP and in 2014.01 there will be new BSP which just generate them directly from the Vivado tools which just target your reference design. You can connect your custom IP (or Xilinx or 3rd party) directly to the GIC which using different IRQ sensitivity with whatever register addresses and make no sense to write it by hand. Thanks, Michal
On Mon, 2014-04-07 at 02:17PM +0200, Mike Looijmans wrote: > On 04/07/2014 07:58 AM, Michal Simek wrote: > >Hi Soren, > > > >On 04/05/2014 01:14 AM, Soren Brinkmann wrote: > >>Convert all Zynq DT files to the dtc preprocessor include syntax. > >>This allows to include header files in the devicetrees like other > >>SoC-types already do. > >> > >>Inspired-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> > >>(http://www.spinics.net/lists/arm-kernel/msg319832.html) > >> > >>Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > > >These 4 patches needs more wider discussion if this is helpful or > >not. Currently I can't see any value in it because everything > >is just generated and fixed. I think I had the same discussion > >with Laurent some weeks ago regarding this. > > I would be kinda neutral here. I'd consider it helpful, it improves > readability (regardless of whether they are generated or hand > crafted). That's convenient for things like interrupt sensitivity, I > can't remember whether 4 is level or edge type. On the other hand, > the clock indices are just as much magic numbers as the memory > addresses. If I suspect an error in that area, I'd start by lokking > in /sys/kernel/debug/clk but wouldn't start in the DT. I agree. Readability is better this way. And whether our generator spits out magic numbers of these symbolic names should not be a big difference, is it? Sören
On Mon, Apr 07, 2014 at 02:24:07PM +0200, Michal Simek wrote: > Device-tree BSP and in 2014.01 there will be new BSP which just > generate them directly from the Vivado tools which just target your > reference design. You can connect your custom IP (or Xilinx or 3rd > party) directly to the GIC which using different IRQ sensitivity > with whatever register addresses and make no sense to write it by > hand. On our Zynq design here we ended up being unwilling to use platform generation from Vivado. Basically all our IP was custom, so there was no win at all to invoking the complexity of the automatic tools. Thus we write the DT by hand, and our DT is complex, integrating peripherals that span two FPGAs. I think the in-kernel DT should use the kernel conventions, which means using #include and the binding constants over magic values. Jason
Hi! On Mon, Apr 07, 2014 at 11:10:12AM -0600, Jason Gunthorpe wrote: > On Mon, Apr 07, 2014 at 02:24:07PM +0200, Michal Simek wrote: > > > Device-tree BSP and in 2014.01 there will be new BSP which just > > generate them directly from the Vivado tools which just target your > > reference design. You can connect your custom IP (or Xilinx or 3rd > > party) directly to the GIC which using different IRQ sensitivity > > with whatever register addresses and make no sense to write it by > > hand. > > On our Zynq design here we ended up being unwilling to use platform > generation from Vivado. Basically all our IP was custom, so there was > no win at all to invoking the complexity of the automatic tools. > > Thus we write the DT by hand, and our DT is complex, integrating > peripherals that span two FPGAs. > > I think the in-kernel DT should use the kernel conventions, which > means using #include and the binding constants over magic values. > ACK. If in doubt follow common mainline practice. Although using includes for DT is not necessarily common practice, readability of DTs is really important IMHO. Regards, Steffen
On 04/07/2014 08:02 PM, Steffen Trumtrar wrote: > Hi! > > On Mon, Apr 07, 2014 at 11:10:12AM -0600, Jason Gunthorpe wrote: >> On Mon, Apr 07, 2014 at 02:24:07PM +0200, Michal Simek wrote: >> >>> Device-tree BSP and in 2014.01 there will be new BSP which just >>> generate them directly from the Vivado tools which just target your >>> reference design. You can connect your custom IP (or Xilinx or 3rd >>> party) directly to the GIC which using different IRQ sensitivity >>> with whatever register addresses and make no sense to write it by >>> hand. >> >> On our Zynq design here we ended up being unwilling to use platform >> generation from Vivado. Basically all our IP was custom, so there was >> no win at all to invoking the complexity of the automatic tools. >> >> Thus we write the DT by hand, and our DT is complex, integrating >> peripherals that span two FPGAs. >> >> I think the in-kernel DT should use the kernel conventions, which >> means using #include and the binding constants over magic values. >> > > ACK. > > If in doubt follow common mainline practice. Although using includes > for DT is not necessarily common practice, readability of DTs is > really important IMHO. Let me give you one example. When you add xilinx intc controller to zynq HW design which uses gic with headers you are using then you will find out that sensitivity for both controllers are just different. This is just one case I am aware of. I expect there will be one more. Also dtses from kernel are copied to other project because separation from kernel hasn't be done but there is any plan regarding this. Using dtc preprocessor and macros improve DTS readability but at the same time force other users to copy all necessary files from the kernel to that projects which is just hassle. With example above there are also cases where using macros is just wrong that's why I don't want to start to use it. Thanks, Michal
On Tue, Apr 08, 2014 at 09:03:27AM +0200, Michal Simek wrote: > > If in doubt follow common mainline practice. Although using includes > > for DT is not necessarily common practice, readability of DTs is > > really important IMHO. > > Let me give you one example. When you add xilinx intc controller > to zynq HW design which uses gic with headers you are using > then you will find out that sensitivity for both controllers > are just different. > This is just one case I am aware of. I expect there will be one more. I'm not sure I see the problem here, just because some bindings can't use the standard shared constants doesn't mean the GIC bindings and users should avoid them. The binding documentation is supposed to make it clear what is correct. It is just as easy to get confused with numbers, does 4 mean XILINX_INTC_IRQ_RISING or IRQ_TYPE_LEVEL_HIGH ? > Using dtc preprocessor and macros improve DTS readability but at the same > time force other users to copy all necessary files from the kernel > to that projects which is just hassle. You can run the DTS through cpp before you export it out of the kernel environment, then you get a flat file with no includes. The shared kernel conventions are more important than constraints from outside projects. Jason
On 04/08/2014 07:27 PM, Jason Gunthorpe wrote: > On Tue, Apr 08, 2014 at 09:03:27AM +0200, Michal Simek wrote: > >>> If in doubt follow common mainline practice. Although using includes >>> for DT is not necessarily common practice, readability of DTs is >>> really important IMHO. >> >> Let me give you one example. When you add xilinx intc controller >> to zynq HW design which uses gic with headers you are using >> then you will find out that sensitivity for both controllers >> are just different. >> This is just one case I am aware of. I expect there will be one more. > > I'm not sure I see the problem here, just because some bindings can't > use the standard shared constants doesn't mean the GIC bindings and > users should avoid them. The binding documentation is supposed to make > it clear what is correct. > > It is just as easy to get confused with numbers, does 4 mean > XILINX_INTC_IRQ_RISING or IRQ_TYPE_LEVEL_HIGH ? That's why you have there biding documentation to exactly know what it is. >> Using dtc preprocessor and macros improve DTS readability but at the same >> time force other users to copy all necessary files from the kernel >> to that projects which is just hassle. > > You can run the DTS through cpp before you export it out of the kernel > environment, then you get a flat file with no includes. What's the result? 1. DTSI and DTS together which completely break hierarchy 2. DTS without comments It means, yes, you get a file when you go through cpp but different then you have now. > The shared kernel conventions are more important than constraints from > outside projects. zynq-7000.dtsi is fixed and you can't just change it based on your project. For things which are in your board file like zynq-zc702 then you can use whatever you like. Maybe I just need some time to get used to it but currently... Thanks, Michal
diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi index 20a13cba65a3..66613d04de5d 100644 --- a/arch/arm/boot/dts/zynq-7000.dtsi +++ b/arch/arm/boot/dts/zynq-7000.dtsi @@ -10,7 +10,8 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. */ -/include/ "skeleton.dtsi" + +#include "skeleton.dtsi" / { compatible = "xlnx,zynq-7000"; diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts index c913f77a21eb..07713d3c716a 100644 --- a/arch/arm/boot/dts/zynq-zc702.dts +++ b/arch/arm/boot/dts/zynq-zc702.dts @@ -12,7 +12,7 @@ * GNU General Public License for more details. */ /dts-v1/; -/include/ "zynq-7000.dtsi" +#include "zynq-7000.dtsi" / { model = "Zynq ZC702 Development Board"; diff --git a/arch/arm/boot/dts/zynq-zc706.dts b/arch/arm/boot/dts/zynq-zc706.dts index 88f62c50382e..cf6566cdb02a 100644 --- a/arch/arm/boot/dts/zynq-zc706.dts +++ b/arch/arm/boot/dts/zynq-zc706.dts @@ -13,7 +13,7 @@ * GNU General Public License for more details. */ /dts-v1/; -/include/ "zynq-7000.dtsi" +#include "zynq-7000.dtsi" / { model = "Zynq ZC706 Development Board"; diff --git a/arch/arm/boot/dts/zynq-zed.dts b/arch/arm/boot/dts/zynq-zed.dts index 82d7ef1a9a9c..1541716e2cfb 100644 --- a/arch/arm/boot/dts/zynq-zed.dts +++ b/arch/arm/boot/dts/zynq-zed.dts @@ -13,7 +13,7 @@ * GNU General Public License for more details. */ /dts-v1/; -/include/ "zynq-7000.dtsi" +#include "zynq-7000.dtsi" / { model = "Zynq Zed Development Board";
Convert all Zynq DT files to the dtc preprocessor include syntax. This allows to include header files in the devicetrees like other SoC-types already do. Inspired-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> (http://www.spinics.net/lists/arm-kernel/msg319832.html) Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- Changes in v2: None arch/arm/boot/dts/zynq-7000.dtsi | 3 ++- arch/arm/boot/dts/zynq-zc702.dts | 2 +- arch/arm/boot/dts/zynq-zc706.dts | 2 +- arch/arm/boot/dts/zynq-zed.dts | 2 +- 4 files changed, 5 insertions(+), 4 deletions(-)