[U-Boot] [PATCH v2] board/BuR/zynq/brsmarc2: initial commit
Michal Simek
michal.simek at xilinx.com
Thu May 9 21:49:46 UTC 2019
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
More information about the U-Boot
mailing list