[U-Boot] Ret: Re: [PATCH v5] board/BuR/zynq/brsmarc2: initial commit

[Hannes Schmelzer]

"Michal Simek" <monstr at monstr.eu> wrote on 14.08.2019 08:10:31:

> 
> Hi,
> 
> first of all sorry for delay I had vacation.
Hi Michal,
thanks for your time again. I'm now working again on u-boot and 
zynq-board.

> > +
> > +   memory {
> > +      device_type = "memory";
> > +      reg = <0x0 0x20000000>;
> > +   };
> > +
> > +   chosen {
> > +      bootargs = "console=ttyPS0,115200 earlyprintk";
> 
> This looks like as Linux command line. If yes you should use earlycon
> instead of earlyprintk which is used for debugging.

yes, will do so in v6


> > +
> > +&ethernet_phy0 {
> > +      ti,ledcr = <0x0480>;
> > +      ti,rgmii-rxclk-shift;
> 
> fix indentation
> 
> > +};
> > +
> > +&ethernet_phy1 {
> > +      ti,ledcr = <0x0480>;
> 
> 
> ditto.

gets fixed in v6.

> 
> > +};
> > diff --git a/arch/arm/dts/zynq-brsmarc2.dts 
b/arch/arm/dts/zynq-brsmarc2.dts
> > new file mode 100644
> > index 0000000..b53a2e7
> > --- /dev/null
> > +++ b/arch/arm/dts/zynq-brsmarc2.dts
> > @@ -0,0 +1,8 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * B&R BRSMARC2 board DTS file
> > + *
> > + * Copyright (C) 2019 B&R Industrial Automation GmbH
> > + */
> > +
> > +#include "zynq-brsmarc2-vxworks.dtsi"
> 
> if you want to use it in this way I would expect you will do it like 
this.
> 
> #include "zynq-brsmarc2-common.dtsi"
> #include "zynq-brsmarc2-vxworks.dtsi"
> 
> Where you are saying that you are taking common + vxworks as default.
> (And remove -common.dtsi from vxworks file of course).

gets fixed in v6.

> 
> > diff --git a/board/BuR/zynq/.gitignore b/board/BuR/zynq/.gitignore
> > new file mode 100644
> > index 0000000..fa64fbc
> > --- /dev/null
> > +++ b/board/BuR/zynq/.gitignore
> > @@ -0,0 +1 @@
> > +./ps7_init_gpl.c
> 
> Why?
no reason for that, some artifact from early days.
so dropping it in v6.


> > +
> > +int board_late_init(void)
> > +{
> > +   int rc;
> > +   struct udevice *i2cdev;
> > +
> > +   br_resetc_bmode();
> > +   brdefaultip_setup(0, 0x57);
> > +
> > +   rc = i2c_get_chip_for_busnum(0, 0x5D, 1, &i2cdev);
> > +   if (rc >= 0)
> > +      rc = dm_i2c_write(i2cdev, 0xEF, NULL, 0);
> 
> IMHO 0x57/0x5D and 0xEF are all magic values and they should be macros
> or at least comments what that means.

adding some comments with description about whats going on in v6.

> > +$(hw-platform-y)_prod.zip: $(hw-platform-y)_prog.bin
> > +   $(call if_changed,prodzip)
> > +
> > +
> 
> one newline is enought.

will be fixed in v6.


> > --- /dev/null
> > +++ b/include/configs/brsmarc2.h
> > @@ -0,0 +1,150 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * specific parts for B&R brsmarc2 SoM
> > + *
> > + * Copyright (C) 2019 Hannes Schmelzer <oe5hpm at oevsv.at> -
> > + * B&R Industrial Automation GmbH - http://www.br-automation.com
> > + *
> > + */
> > +
> > +#ifndef __CONFIG_BRSMARC2_H__
> > +#define __CONFIG_BRSMARC2_H__
> > +
> > +#include <configs/bur_cfg_common.h>
> > +
> > +/* Cache options */
> > +#define CONFIG_SYS_L2CACHE_OFF
> > +#ifndef CONFIG_SYS_L2CACHE_OFF
> > +# define CONFIG_SYS_L2_PL310
> > +# define CONFIG_SYS_PL310_BASE      0xf8f02000
> > +#endif
> > +
> > +#define ZYNQ_SCUTIMER_BASEADDR      0xF8F00600
> > +#define CONFIG_SYS_TIMERBASE      ZYNQ_SCUTIMER_BASEADDR
> > +#define CONFIG_SYS_TIMER_COUNTS_DOWN
> > +#define CONFIG_SYS_TIMER_COUNTER   (CONFIG_SYS_TIMERBASE + 0x4)
> > +
> > +/* Serial drivers */
> > +#define CONFIG_BAUDRATE      115200
> 
> This is in Kconfig already.

ok, dropping it here in v6.

> 
> > +/* The following table includes the supported baudrates */
> > +#define CONFIG_SYS_BAUDRATE_TABLE  \
> > +   {300, 600, 1200, 2400, 4800, 9600, 19200, 38400, 57600, 115200, 
230400}
> 
> All these configs L2/timer included are already in zynq-common.dtsi.
> I think I told you to include this file here. If there is something what
> you don't want to enable for your board then let's fix it but I really
> don't like to c&p setting which is already written. It is purely
> duplication for no reason.
> This file should include zynq-common.dtsi and take configuration from
> that file. If there is something board specific then we should find a
> way how to fix this.
> 

this looks like to be the last big challenge to get rid with my new board 
;-)

I think you mean zynq-common.h

Yes, you told me using it ... more than one time ... but i'm still very 
unhappy
with this zynq-common.h since it is very overloaded and trimmed to
the existing eval-boards.

Maybe we have a chance to split the zynq-common.h up into at least
two parts where having at one side all soc-specific stuff (various
base-addresses, caching, timer, stack, ....), the bare-minimum and
at the other side all "application-specific", meaning distro-default,
bootcommand, filenames, boot-devices, load-addresses ...

This would give us the opportunity having boards like the existing
eval-boards and boards like mine which loving the bare minimum.

hw?

> > +
> > +/* Environment */
> > +#define CONFIG_SYS_AUTOLOAD   "no"
> 
> I have not a problem with listing things like this because it takes time
> to move things to Kconfig but copying baseaddresses and setting for SPL
> doesn't make sense.

no glue at the moment. pls a bit more explanation.

> > 
> 
> Thanks,
> Michal

cheers,
Hannes