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

[Neha Malcom Francis]

Hi Simon, Tom

On 24-Aug-23 8:16 PM, Simon Glass wrote:
> Hi Tom,
> 
> On Thu, 24 Aug 2023 at 08:44, Tom Rini <trini at konsulko.com> wrote:
>>
>> 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?
> 
> Yes, see the 'missing-msg' tag and the 'missing-blob-help' file. This
> should work for any external block, whether missing or faked, optional
> or required,
> 

I'll take a future action to clean that up.

> Regards,
> Simon