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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot