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.
> 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.
>
> >[snip]
> >>>>+ uart5: serial@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.
> >[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!
--
Tom
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
