[Buildroot] [PATCH 1/2] configs/milkv_duo: new defconfig

[Arnout Vandecappelle]

On 23/11/2023 02:22, hanyuan wrote:
> Hello Arnout,
> 
>> 2023年11月23日 04:19，Arnout Vandecappelle <arnout at mind.be> 写道：
>>
>>
>> On 22/11/2023 04:47, hanyuan wrote:
>>> Hello Arnout,
>>> Thanks for your patience and careful review! Let me explain something you 
>>> requested.
>>>> Buildroot defconfigs should be _minimal_, i.e. just enough to boot the 
>>>> device and perhaps get a (wired) network connection. Looking at all those 
>>>> files, clearly these defconfigs are _not_ minimal. I think pretty much 
>>>> everything in the overlay can be removed.
>>>> Nice trick to save the random MAC address that is generated on first boot, 
>>>> but we don't include such things in the minimal defconfig, especially if 
>>>> it's not board specific.
>>>> This USB stuff is also not board-specific, is just generic USB gadget 
>>>> configuration. It might make sense to create a Buildroot package with those 
>>>> scripts though.
>>> I think the digest of the changes is requesting us to remove the unnecessary 
>>> things such as the packages we use in the defconfig and files in the overlay. 
>>> I am sorry for my previous misunderstanding of your minimal philosophy. The 
>>> reason I did them is that I want to let the system we build could be nearly 
>>> the same as the official image. And the features are mainly about the USB 
>>> RNDIS or other multiplex functions.
>>> Now I plan to remove the whole overlay folder and move our files which give 
>>> the features into a new package (e.g. named BR2_PACKAGE_MILKV_DUO_USB_TOOLS). 
>>> Could you please give me a review on that?
>>
>> As I wrote somewhere in my review (but you didn't include it below), I think 
>> the script that toggles the USB hub host/gadget mode is still relevant, 
>> because that is something board specific. The rest of the overlay indeed can 
>> be removed, I think.
>>
>> About BR2_PACKAGE_MILKV_DUO_USB_TOOLS: as far as I could see, those scripts 
>> are not at all board-specific, they simply configure the generic gadget 
>> functions. So the package name doesn't need to have milkv-duo in it. Perhaps
>> initscripts-usb-gadget ?
>>
>> I'm not entirely sure if it is appropriate to be part of Buildroot anyway, it 
>> would be more appropriate in its own repository. But then again, if we have it 
>> in Buildroot itself, we can maintain it independently.
> 
> In fact, all the scripts concerning USB in the previous overlay are all board 
> specific, to some extent. Please take a look at the following lines:

  Okay, I didn't look at it too carefully it seems.

> 
> ${DUO_WRITE} host > /proc/cviusb/otg_role
> CVI_DIR=/tmp/usb
> CVI_GADGET=$CVI_DIR/usb_gadget/cvitek
> # Set the USB configuration
> mkdir $CVI_GADGET/configs/c.1
> mkdir $CVI_GADGET/configs/c.1/strings/0x409
> ${DUO_WRITE} "config1">$CVI_GADGET/configs/c.1/strings/0x409/configuration
> 
> We could have two strategies. Firstly, if we think they are relevant and want to 
> keep something in the overlay, let’s compress the files into one single big 
> script. And do I have other alternatives of /usr/bin? I don’t want to move the 
> USB initial scripts into the user’s PATH. Secondly, we import the package 
> BR2_PACKAGE_MILKV_DUO_USB_TOOLS. The USB functions are not must for a minimal 
> system to run. However, if we use these shell scripts, by default the USB rndis 
> will be up, allowing us to login through ssh by a usb cable. If not, we have to 
> use the serial port. What do you think, overlay or package?

  For sure, the first step is a milkv_duo_defconfig without any of this stuff, 
and add the USB things in a follow-up patch. Expect a _lot_ of review comments 
on those, and possibly finally a rejection.

>> Note that this initscripts-usb-gadget package will probably take a few more 
>> iterations before it gets accepted as well. So probably best to submit it 
>> separately from the reset of this series. Here are a few hints already that 
>> can help to make sure the initial submission is as good as possible.
>>
>> - Configuration should be in /etc/default.
>> - Helper scripts should be in /usr/bin.
>> - Use <PKG>_LINUX_CONFIG_FIXUPS to enable the required configs in the kernel.
>> - The package should probably depend on !BR2_INIT_SYSTEMD.
>> - Make sure to follow the init script skeleton (but I think you already did that).
> 
> If we do use the package to provide the USB function, I highly hope that it 
> could be named milkv-duo, which will be convenient because we could directly 
> move our code into it. And we will not have the need to make it general.

  Yeah, if it really is board-specific, the milkv-duo naming is OK.

[snip]
>>>> There's also an Upstream: link missing.
>>> Is the Upstream a must? I have found many patches in buildroot do not have this.
>>
>> The Upstream: tag is a must, check-package complains if it's not there. We 
>> require it exactly to motivate people to send their patches upstream. We want 
>> it upstream for two reasons: we don't want to maintain patches, and often 
>> upstream is better positioned to review the patches. The latter certainly 
>> applies to the OpenSBI patches.
> 
> Could I use a customed repo to avoid use patches? The upstream is still a big 
> work in progress. I could not give an upstream tag currently. We hope to let our 
> users have a taste of buildroot quickly.

  Yes, a vendor fork repository is acceptable.

[snip]
>> ifeq ($(BR2_riscv_thead),y)
>> GCC_TARGET_ARCH = 
>> rv64imafdc_xtheadba_xtheadbb_xtheadbs_xtheadcmo_xtheadcondmov_xtheadfmemidx_xtheadfmv_xtheadint_xtheadmac_xtheadmemidx_xtheadmempair_xtheadsync
>> endif
>>
>>
>> The above change should obviously be a separate patch as well.
> 
> This is totally beyond my control, haha, willing to contribute in the future.

  In the mean time I found out that the T-Head CPU used in the BeagleV A-Head is 
in fact a different one, with different incompatibilities, so my idea of 
unifying them is not valid.


  Regards,
  Arnout


[snip]