On 08. 05. 19 5:37, Hannes Schmelzer wrote:
On 5/3/19 6:29 PM, Michal Simek wrote:
On 03. 05. 19 6:18, Tom Rini wrote:
On Fri, May 03, 2019 at 01:34:24PM +0200, Hannes Schmelzer wrote:
On 5/2/19 9:03 PM, Tom Rini wrote:
On Thu, May 02, 2019 at 08:34:32PM +0200, Hannes Schmelzer wrote:
On 5/2/19 6:06 PM, Michal Simek wrote: > Hi, Hi Michal, > On 02. 05. 19 5:14, Hannes Schmelzer wrote: >> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile >> index dfa5b02..2b00129 100644 >> --- a/arch/arm/dts/Makefile >> +++ b/arch/arm/dts/Makefile >> @@ -208,6 +208,8 @@ dtb-$(CONFIG_ARCH_ZYNQ) += \ >> zynq-zc770-xm011-x16.dtb \ >> zynq-zc770-xm012.dtb \ >> zynq-zc770-xm013.dtb \ >> + zynq-brsmarc2.dtb \ >> + zynq-brsmarc2_r512.dtb \ > Can't you detect it if you have 512M version? > u-boot itself has code for these kind of detection. > > long get_ram_size(long *base, long maxsize) I actually think not, because i need different ps7_init stuff for the two different RAM chips. (timing, adress lines, ...) But i will check if i even can drop the two different dts files. >> +/ { >> + model = "BRSMARC2 Zynq SoM"; >> + compatible = "xlnx,zynq-7000"; >> + >> + fset: factory-settings { >> + bl-version = " "; >> + order-no = " "; >> + cpu-order-no = " "; >> + hw-revision = " "; >> + serial-no = <0>; >> + device-id = <0x0>; >> + parent-id = <0x0>; >> + hw-variant = <0x0>; >> + hw-platform = <0x0>; >> + fram-offset = <0x0>; >> + fram-size = <0x0>; >> + cache-disable = <0x0>; >> + cpu-clock = <0x0>; >> + }; > What's this? No compatible string. This looks quite hacky. This are factory settings, used by the OS (in this case vxWorks), to identify on which hardware it runs, and have per device unique stuff (serial number). But you're right, it would be nice to have here some compatible string, i will change this. Today we just search for the node "factory-setting". A more comfortable ways would be vxFdtNodeOffsetByCompatible(....) >> + >> + aliases { >> + ethernet0 = &gem0; >> + ethernet1 = &gem1; >> + i2c0 = &i2c0; >> + serial0 = &uart0; >> + spi0 = &qspi; >> + mmc0 = &sdhci0; >> + fset = &fset; >> + can0 = &can0; >> + can1 = &can1; >> + }; >> + >> + memory { >> + device_type = "memory"; >> + reg = <0x0 0x10000000>; >> + }; >> + >> + board { >> + status = "okay"; >> + compatible = "bur,brsmarc2-som"; >> + usb0mux-gpios = <&gpio0 68 GPIO_ACTIVE_HIGH>; >> + usb1mux-gpios = <&gpio0 69 GPIO_ACTIVE_HIGH>; >> + powerdown-gpios = <&gpio0 0 GPIO_ACTIVE_LOW>; >> + reset-gpios = <&gpio0 9 GPIO_ACTIVE_LOW>; >> + }; > Where is mainline dt binding for this? Nowhere, because u-boot nor linux does use this, this is only for the vxWorks OS.
This is what I kinda figured was the case. We now have some interesting times ahead of us as yes, we normally think about DTS reviews in terms of Linux. But this is all for a board that uses vxWorks. Perhaps the best thing to do here is note (and for all of the other boards too, but you can wait for general feedback before v3'ing them all) in the dts comment that it's for vxWorks as people tend to assume a DTS file is for Linux (even with many counter examples).
OK. Thanks.
would it be fine having it like this ?
/* factory-settings for the vxWorks target */ fset: factory-settings { compatible = "bur,fsetv1"; bl-version = " "; ..... };
/* misc. peripheral, used in vxWorks */ board { status = "okay"; ... };
Or might be a general description on top would be more helpful?
I think a general comment at the top saying that this tree is only valid for vxWorks and U-Boot is enough detail.
Okay, i will do so like this: /* * This devicetree is only valid for u-boot and the primary OS (vxWorks 6.9.4.x) * of this board. */
It would be much better to simply put vxworks stuff to one dtsi file and common stuff to another. That it will be clear what it is vxworks part and what it is common.
In general i pay attention to describe devices as generic as possible, meaning that a) syntax is correct, b) some linux or even u-boot isn't disturbed by this descriptions.
With a look to the paragraph below, the implemented UART in FPGA, is 16550 compatible, so there might be a chance that it would work with Linux as well.
wrt the UART, are there specified bindings in vxWorks? That seems like a case where the DTS needs to be valid for what U-Boot says the binding is (and in turn, what Linux does). And perhaps there's room for clarifications to those bindings.
actually i think the UART is described valid in the fdt and can be used by u-boot or linux as well even i've not tested it yet.
I think we need to get more clarity what exactly vxworks expects and what are just your "hacks" to get it work. If vxworks deviates existing dt binding, or create completely new one.
One thing to say is, that vxWorks 6.9.4.x doesn't support fdt out of the box. All the support and usage of fdt within 6.9.4.x is introduced or backported from vxWorks 7 (which is based on fdt, even with their own and mostly different bindings) by myself - so called my "hacks" - because i don't want have thousand of different vxWorks images for one board in various only very little different hw-variants. Excactly for this purpose is the fdt.
My thinking of the dts file for a board is that it should be good readable and easy maintainable by the board maintainer - in this case it's me. So splitting it up into one more file does only degrade readability.
I don't agree with this. It is not only for you. We are doing all reviews to have codebase nice and clean. And adding hacky code(C/asm/dt) to u-boot because it is used only one person is simply bad thing to do.
>> +"scradr=" __stringify(CONFIG_SYS_LOAD_ADDR) "\0" \ >> +"dtbaddr=0x4000000\0" \ >> +"loadaddr=0x2000000\0" \ >> +"fpgaaddr=fdt get value fpgabase /fpga bur,updaddr;" \ >> +" fdt get value fpgasize /fpga bur,updsize\0" \ >> +"fpga=setenv fpgastatus disabled; run fpgaaddr;" \ >> +" sf read ${loadaddr} ${fpgabase} ${fpgasize} &&" \ >> +" fpga loadb 0 ${loadaddr} ${fpgasize} && setenv fpgastatus >> okay\0" \ >> +"fpgastatus=disabled\0" \ >> +"fpgaupd=run fpgaaddr && tftp ${loadaddr} X20CP04xx.bit &&" \ >> +" sf erase ${fpgabase} +${filesize} &&" \ >> +" sf write ${loadaddr} ${fpgabase} ${filesize}\0" \ >> +"netupd=tftp ${loadaddr} boot.bin && sf probe &&" \ >> +" sf erase 0 +${filesize} && sf write ${loadaddr} 0 ${filesize} >> &&" \ >> +" tftp ${loadaddr} u-boot-dtb.img &&" \ >> +" sf erase 0x40000 +${filesize} && sf write ${loadaddr} 0x40000 >> ${filesize}\0" \ >> +"cfgscr=mw ${dtbaddr} 0;" \ >> +" sf probe && sf read ${scradr} 0xC0000 0x10000 && source >> ${scradr};" \ >> +" fdt addr ${dtbaddr} || cp ${fdtcontroladdr} ${dtbaddr} 4000\0" \ >> +"vxargs=setenv bootargs gem(0,0)host:vxWorks h=${serverip}" \ >> +" e=${ipaddr}:${netmask} g=${gatewayip} u=vxWorksFTP >> pw=vxWorks\0" \ >> +"vxfdt=fdt addr ${dtbaddr}; fdt resize 0x10000;" \ >> +" fdt set /fpga status ${fpgastatus};" \ >> +" fdt boardsetup\0" \ >> +"startvx=run vxargs && mw 0x1100 0 && run vxfdt &&" \ >> +" bootm ${loadaddr} - ${dtbaddr}\0" \ >> +"b_break=0\0" \ >> +"b_tgts_std=mmc spi usb0 net0 net1\0" \ >> +"b_tgts_rcy=spi usb0 net0 net1\0" \ >> +"b_tgts_pme=usb0 net0 net1 mmc spi\0" \ >> +"b_deftgts=if test ${b_mode} = 12; then setenv b_tgts >> ${b_tgts_pme};" \ >> +" elif test ${b_mode} = 0; then setenv b_tgts ${b_tgts_rcy};" \ >> +" else setenv b_tgts ${b_tgts_std}; fi\0" \ >> +"b_mmc=run fpga; mmc dev 0; load mmc 0 ${loadaddr} arimg && run >> startvx\0" \ >> +"b_spi=run fpga; sf read ${loadaddr} 900000 700000 && run >> startvx\0" \ >> +"b_net0=tftp ${scradr} netscr-brsmarc2-${board_id}.img && >> source ${scradr}\0" \ >> +"b_net1=tftp ${scradr} netscript.img && source ${scradr}\0" \ >> +"b_usb0=usb start && load usb 0 ${scradr} bootscr.img && source >> ${scradr}\0" \ >> +"b_default=run b_deftgts; for target in ${b_tgts};"\ >> +" do run b_${target}; if test ${b_break} = 1; then; exit; fi; >> done\0" > we are trying to get rid of these variables. Please enable distro > boot > and put all of these to scripts and run them. No, i don't want some distro defaults ... because we've our own boot strategy.
How much of this strategy is common to all the BuR platforms? Perhaps the answer is that you should make include/environment/bur/ and put the common script in there. Thanks!
The strategy itself (having b_mode and then try several targets) is always the same but the boot-targets itself are individual. I will start a project for catching the "core" into 'include/environment/bur/' over all BuR boards. But this will take more time than a simple rework of this patch, meaning that this will follow later on.
I'm OK with that, thanks!
Zynq/ZynqMP code prioritize bootmode with distro boot. I personally want to have less include/configs/* files for the same type of boards. It means do whatever you want but wire that via scripts. Let distro boot to load your scripts and do that stuff there. And I also have no problem to have these scripts in u-boot to show others what you do.
I think that the existing code doesn't fit to my needs.
Then fix it.
I have here some resetcontroller and/or gpios below which tell me how to boot. This is really specific to b&r boards. So i will walk the above.
Then do it properly and it can be useful for others too
But i will take up the idea for having different scripts which run on different boot targets. As told already i have too look here over all "my" b&r boards and not only the zynq board for having a standard how b&r boards are working.
Ok. Looking forward for v3.
Thanks, Michal