[U-Boot] [PATCH v2] board/BuR/zynq/brsmarc2: initial commit
Michal Simek
michal.simek at xilinx.com
Fri May 3 16:29:32 UTC 2019
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.
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.
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.
>>> [snip]
>>>>>> + uart5: serial at 40200A00 {
>>>>>> + status = "disabled";
>>>>>> + compatible = "ns16550a", "bur,DdVxSf16x5xIO";
>>>>>> + bur,hwtree = "IF21";
>>>>>> + reg = <0x40200A00 0x40>;
>>>>>> + interrupt-parent = <&intc>;
>>>>>> + interrupts = <0 90 4>;
>>>>>> + };
>>>>> all these PL stuff can't go to mainline.
>>>> Please explain me the reason why this PL stuff cannot go mainline?
>>>> Maybe a solution cold be to drop those in the mainline dts and then
>>>> patch it again into it on the local branch. But that would be a way which
>>>> i don't prefer.
>>> So, now that we're clear this is for vxWorks, "PL stuff" ? The only
>>> thing I see here that's not spelled out (but based on cursory
>>> examination of the kernel, implied as normal) is interrupt-parent as a
>>> property.
>> Right. The spelling about interrupt is the same as the other
>> peripherals within the SoC (zynq-7000.dtsi).
>>>
>>>>>> +&gem0 {
>>>>>> + status = "okay";
>>>>>> + phy-mode = "rgmii-id";
>>>>>> + phy-handle = <ðernet_phy0>;
>>>>>> + mac-address = [ 00 00 00 00 00 00 ];
>>>>> 0 mac is wrong.
>>>> No, this zeros are placeholder.
>>>> The real MAC-Adresses are get into the dts during u-boot's fdt_fixup(...).
>>>> They come from environment, and th environment is setup from some
>>>> u-boot script.
>>> But we shouldn't need the node here to set the property is I think the
>>> point.
>> OK ... i will take here "real" or at least formal correct MAC-addresses
>> into for having a fallback if nothing is programmed at all and bring up
>> the interface initially.
>
> Well, what I mean is that in the code, we can add the node and should
> then populate it with either a "real" value that we read from something
> (eeprom, just the env, whatever) or a real random address. We shouldn't
> need to have the dts file say there's a mac-address property with all
> zeros, we can add that at run-time.
+1
>
>>> [snip]
>>>>>> +"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.
Thanks,
Michal
More information about the U-Boot
mailing list