[U-Boot] [PATCH v2] board/BuR/zynq/brsmarc2: initial commit

Tom Rini trini at konsulko.com
Fri May 3 13:18:19 UTC 2019


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 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 = <&ethernet_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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190503/0f9c3b9b/attachment.sig>


More information about the U-Boot mailing list