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

[Tom Rini]

On Thu, Aug 24, 2023 at 08:41:23AM -0600, Simon Glass wrote:
> Hi,
> 
> On Thu, 24 Aug 2023 at 08:20, Tom Rini <trini at konsulko.com> wrote:
> >
> > 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 at 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.
> 
> If it is optional, use the 'optional' tag on the entry You will get a
> warning but not an error. At the moment it is saying that the image is
> non-functional, which is not good.

Yes, and I believe these are non-optional firmwares, so do we still have
the ability to produce a more specific and verbose error message?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230824/2d4b0985/attachment.sig>