Re: [PATCH V4 8/8] doc: board: ti: Add BeaglePlay documentation

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4235 bytes --]

On Thu, Aug 24, 2023 at 06:46:57PM +0530, Neha Malcom Francis wrote:
> Hi Simon, Nishanth
> 
> On 24/08/23 08:57, Nishanth Menon wrote:
> > On 21:01-20230823, Simon Glass wrote:
> > > Hi Nishanth,
> > > 
> > > On Wed, 23 Aug 2023 at 18:18, Nishanth Menon <nm@ti.com> wrote:
> > > > 
> > > > On 17:57-20230823, Simon Glass wrote:
> > > > [...]
> > > > > > This is how we have a common bit of rST for how to build N boards,
> > > > > > without having to do a literal copy and paste N times.
> > > > > 
> > > > > How about using this?
> > > > > 
> > > > > https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#substitution-definitions
> > > > 
> > > > I was not able to succeed with complex includes such as:
> > > > https://github.com/u-boot/u-boot/blob/master/doc/board/ti/am62x_sk.rst?plain=1#L89
> > > > 
> > > > am62x complete build procedure defined once and reused in other am62x
> > > > platforms.. But the am62x build procedure itself is reused from common
> > > > k3 build steps.
> > > 
> > > I followed through these instructions. I find the env vars quite
> > > confusing, since I don't really know what it is doing. It feels like a
> > > script:
> > > 
> > > do $a $b $c
> > > do $f $e
> > > 
> > > it is pretty hard to follow. I think it would be better to write
> > > everything out in full for each board, like rockchip does.
> > 
> > Unfortunately, this is a few major steps that is repeated for
> > (currently):
> > 	AM62x SK
> > 	Toradex Verdin
> > 	(pending: beagleplay - )
> > 	(once all the dust clears up, hopefully phytec)
> > 	SK-LP
> > 	....
> > 
> > I have no reasonable way to offer to keep them all in sync.
> > 	https://libera.irclog.whitequark.org/u-boot/2023-07-26#34662854;
> > is kind of why I went down this path.S
> > 
> > > 
> > > Some other minor feedback:
> > > 
> > > - The 'make' lines should really have -j $(nproc) added
> > 
> > Different styles of shells..
> > 
> > > - The $ signs at the start of each command in the docs are a pain
> > > since it stops me copying the commands into the terminal - can you
> > > remove them?
> > 
> > hehe.. "dont" let people blindly copy paste without understanding what is
> > going on argument?
> > 
> > If folks are OK, I sure can send a different patch series for that.. (or
> > maybe motivate someone to do that instead of me ;))
> > 
> > 
> > > - It doesn't build for me:
> > > 
> > >    BINMAN  .binman_stamp
> > > Image 'ti-dm' is missing external blobs and is non-functional: blob-ext
> > > 
> > > /binman/ti-dm/blob-ext (ti-dm/am62xx/ipc_echo_testb_mcu1_0_release_strip.xer5f):
> > >     Missing blob
> > > 
> > > Some images are invalid
> > > make[1]: *** [/scratch/sglass/cosarm/src/third_party/u-boot/files/Makefile:1115:
> > > .binman_stamp] Error 103
> > > make[1]: Leaving directory '/tmp/b/play'
> > > make: *** [Makefile:177: sub-make] Error 2
> > 
> > 
> > ^^ Neha: This is what I was complaining about.
> > 
> > https://u-boot.readthedocs.io/en/latest/board/ti/am62x_sk.html?highlight=am62#sources
> > 
> > source: https://git.ti.com/git/processor-firmware/ti-linux-firmware.git
> > is missing, we never used to break build previously binman converted now does.
> > 
> > I am wondering if I need to explicitly call out git clone instructions
> > out..
> > 
> 
> Right... this does seem to be a complaint that keeps coming up.
> 
> Simon, my intention at the time of sending out the patch was that anyone
> building the board should "not NOT" have the DM binary. The way we
> structured the filename was that it looks at BINMAN_INDIRS to find
> ti-dm/ipc_echo_testb_mcu1_0_release_strip.xer5f but I guess this is a
> confusing way to put it across. Maybe we should rework that. Or not throw an
> error at all when DM isn't found.

Well, is DM required or not? If it's optional, we have flags to mark it
optional.  If it's required, we have flags for make and buildman to tell
binman "fake it".  I am wondering if we still have the ability to have a
more verbose "need it, can't find it" error message, and so that's where
it should say that you forgot to set BINMAN_INDIRS or clone the firmware
repository.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]