On 5/9/19 11:49 PM, Michal Simek wrote:
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.
OK. i will split them up. have a  look to V3.

+"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.
No. The existing code will not be modified to fit for my specific hardware.
It doesn't make sense at all and i have absolutely no chance to test boards
which may be affected from my changes.

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
It is done well for my point of view!
I have here some specific hardware to support.
Nobody else, except those who have this B&R specific hardware, will find this useful.
Maybe somebody is looking at it and can fork an idea from the b&r stuff,
that would be ok.
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
cheers,
Hannes

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to