[Buildroot] [PATCH] boot/mv-ddr-marvell: support custom GIT settings

[Buildroot] [PATCH] boot/mv-ddr-marvell: support custom GIT settings

[kostap at marvell.com]

From: Konstantin Porotchkin <kostap@marvell.com>

Add support for custom repositories for mv-ddr-marvell sources.
This patch allows getting the mv-ddr-marvell package sources
out of user-defined repositories.
The configuration options are similar to uboot package - once
the BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT=y, the following
configuration entries are used for fetching the package sources:
BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_URL
BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_VERSION

Change-Id: Ibdef65a3ccdfbe47123f27ab6ba311a75d66d50e
Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
---
 boot/mv-ddr-marvell/Config.in         | 16 ++++++++++++++++
 boot/mv-ddr-marvell/mv-ddr-marvell.mk |  7 +++++++
 2 files changed, 23 insertions(+)

diff --git a/boot/mv-ddr-marvell/Config.in b/boot/mv-ddr-marvell/Config.in
index 4ee8c95b2c..0a9ed7aca5 100644
--- a/boot/mv-ddr-marvell/Config.in
+++ b/boot/mv-ddr-marvell/Config.in
@@ -8,3 +8,19 @@ config BR2_TARGET_MV_DDR_MARVELL
 	  and 8040 SoCs.
 
 	  https://github.com/MarvellEmbeddedProcessors/mv-ddr-marvell/
+
+config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT
+	bool "Custom Git repository"
+
+if BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT
+
+config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_URL
+	string "URL of custom repository"
+
+config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_VERSION
+	string "Custom repository version"
+	help
+	  Revision to use in the typical format used by Git
+	  E.G. a sha id, a tag, branch, ..
+
+endif
diff --git a/boot/mv-ddr-marvell/mv-ddr-marvell.mk b/boot/mv-ddr-marvell/mv-ddr-marvell.mk
index 8d508215f8..100aaccb5f 100644
--- a/boot/mv-ddr-marvell/mv-ddr-marvell.mk
+++ b/boot/mv-ddr-marvell/mv-ddr-marvell.mk
@@ -4,9 +4,16 @@
 #
 ################################################################################
 
+ifeq ($(BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT),y)
+MV_DDR_MARVELL_VERSION = $(call qstrip,$(BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_VERSION))
+MV_DDR_MARVELL_SITE = $(call qstrip,$(BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_URL))
+MV_DDR_MARVELL_SITE_METHOD = git
+BR_NO_CHECK_HASH_FOR += $(MV_DDR_MARVELL_SOURCE)
+else
 # This is the commit for mv_ddr-armada-18.09.2
 MV_DDR_MARVELL_VERSION = 99d772547314f84921268d57e53d8769197d3e21
 MV_DDR_MARVELL_SITE = $(call github,MarvellEmbeddedProcessors,mv-ddr-marvell,$(MV_DDR_MARVELL_VERSION))
+endif
 MV_DDR_MARVELL_LICENSE = GPL-2.0+ or LGPL-2.1 with freertos-exception-2.0, BSD-3-Clause, Marvell Commercial
 MV_DDR_MARVELL_LICENSE_FILES = ddr3_init.c
 
-- 
2.17.1

[Buildroot] [PATCH] boot/mv-ddr-marvell: support custom GIT settings

[Yann E. MORIN]

Konstantin, All,

On 2018-12-24 15:18 +0200, kostap at marvell.com spake thusly:
> From: Konstantin Porotchkin <kostap@marvell.com>
> 
> Add support for custom repositories for mv-ddr-marvell sources.
> This patch allows getting the mv-ddr-marvell package sources
> out of user-defined repositories.
> The configuration options are similar to uboot package - once
> the BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT=y, the following
> configuration entries are used for fetching the package sources:
> BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_URL
> BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_VERSION
> 
> Change-Id: Ibdef65a3ccdfbe47123f27ab6ba311a75d66d50e

No need for this 'Change-Id' because it means nothing to us.

> Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
> ---
>  boot/mv-ddr-marvell/Config.in         | 16 ++++++++++++++++
>  boot/mv-ddr-marvell/mv-ddr-marvell.mk |  7 +++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/boot/mv-ddr-marvell/Config.in b/boot/mv-ddr-marvell/Config.in
> index 4ee8c95b2c..0a9ed7aca5 100644
> --- a/boot/mv-ddr-marvell/Config.in
> +++ b/boot/mv-ddr-marvell/Config.in
> @@ -8,3 +8,19 @@ config BR2_TARGET_MV_DDR_MARVELL
>  	  and 8040 SoCs.
>  
>  	  https://github.com/MarvellEmbeddedProcessors/mv-ddr-marvell/
> +
> +config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT
> +	bool "Custom Git repository"

I think I would prefer that we have a choice selection, like for U-Boot
for example:

    choice
        bool "mv-ddr-marvell version"

    config BR2_TARGET_MV_DDR_MARVELL_MARVELL
        bool "Marvell github" # Maybe get a better prompt here?

    config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT
        bool "Custom Git repository"

    endchoice

    if BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT

    config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_URL
        string "URL of custom repository"

    config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_VERSION
        string "Custom repository version"

    endif # BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT

Otherwise, we usually frown upon adding custom location to packages. Is
it customary for board integrators to have local adaptations to this,
like there is for u-boot and the other bootloaders?

As I see you're an @marvell.com, do you need that to use an internal
repository during development? If so, then why can't you just use the
OVERRIDE_SRCDIR mechanism instead?

Regards,
Yann E. MORIN.

> +if BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT
> +
> +config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_URL
> +	string "URL of custom repository"
> +
> +config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_VERSION
> +	string "Custom repository version"
> +	help
> +	  Revision to use in the typical format used by Git
> +	  E.G. a sha id, a tag, branch, ..
> +
> +endif
> diff --git a/boot/mv-ddr-marvell/mv-ddr-marvell.mk b/boot/mv-ddr-marvell/mv-ddr-marvell.mk
> index 8d508215f8..100aaccb5f 100644
> --- a/boot/mv-ddr-marvell/mv-ddr-marvell.mk
> +++ b/boot/mv-ddr-marvell/mv-ddr-marvell.mk
> @@ -4,9 +4,16 @@
>  #
>  ################################################################################
>  
> +ifeq ($(BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT),y)
> +MV_DDR_MARVELL_VERSION = $(call qstrip,$(BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_VERSION))
> +MV_DDR_MARVELL_SITE = $(call qstrip,$(BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_URL))
> +MV_DDR_MARVELL_SITE_METHOD = git
> +BR_NO_CHECK_HASH_FOR += $(MV_DDR_MARVELL_SOURCE)
> +else
>  # This is the commit for mv_ddr-armada-18.09.2
>  MV_DDR_MARVELL_VERSION = 99d772547314f84921268d57e53d8769197d3e21
>  MV_DDR_MARVELL_SITE = $(call github,MarvellEmbeddedProcessors,mv-ddr-marvell,$(MV_DDR_MARVELL_VERSION))
> +endif
>  MV_DDR_MARVELL_LICENSE = GPL-2.0+ or LGPL-2.1 with freertos-exception-2.0, BSD-3-Clause, Marvell Commercial
>  MV_DDR_MARVELL_LICENSE_FILES = ddr3_init.c
>  
> -- 
> 2.17.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [EXT] Re: [PATCH] boot/mv-ddr-marvell: support custom GIT settings

[Kostya Porotchkin]

Hi, Yann,

> -----Original Message-----
> From: Yann E. MORIN <yann.morin.1998@gmail.com> On Behalf Of Yann E.
> MORIN
> Sent: Monday, December 24, 2018 20:58
> To: Kostya Porotchkin <kostap@marvell.com>
> Cc: buildroot at buildroot.org
> Subject: [EXT] Re: [Buildroot] [PATCH] boot/mv-ddr-marvell: support custom
> GIT settings
> 
> External Email
> 
> ----------------------------------------------------------------------
> Konstantin, All,
> 
> On 2018-12-24 15:18 +0200, kostap at marvell.com spake thusly:
> > From: Konstantin Porotchkin <kostap@marvell.com>
> >
> > Add support for custom repositories for mv-ddr-marvell sources.
> > This patch allows getting the mv-ddr-marvell package sources out of
> > user-defined repositories.
> > The configuration options are similar to uboot package - once the
> > BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT=y, the following
> configuration
> > entries are used for fetching the package sources:
> > BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_URL
> > BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_VERSION
> >
> > Change-Id: Ibdef65a3ccdfbe47123f27ab6ba311a75d66d50e
> 
> No need for this 'Change-Id' because it means nothing to us.
[KP] OK, I will strip it
> 
> > Signed-off-by: Konstantin Porotchkin <kostap@marvell.com>
> > ---
> >  boot/mv-ddr-marvell/Config.in         | 16 ++++++++++++++++
> >  boot/mv-ddr-marvell/mv-ddr-marvell.mk |  7 +++++++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/boot/mv-ddr-marvell/Config.in
> > b/boot/mv-ddr-marvell/Config.in index 4ee8c95b2c..0a9ed7aca5 100644
> > --- a/boot/mv-ddr-marvell/Config.in
> > +++ b/boot/mv-ddr-marvell/Config.in
> > @@ -8,3 +8,19 @@ config BR2_TARGET_MV_DDR_MARVELL
> >  	  and 8040 SoCs.
> >
> >  	  https://github.com/MarvellEmbeddedProcessors/mv-ddr-marvell/
> > +
> > +config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT
> > +	bool "Custom Git repository"
> 
> I think I would prefer that we have a choice selection, like for U-Boot for
> example:
> 
>     choice
>         bool "mv-ddr-marvell version"
> 
>     config BR2_TARGET_MV_DDR_MARVELL_MARVELL
>         bool "Marvell github" # Maybe get a better prompt here?
> 
>     config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT
>         bool "Custom Git repository"
> 
>     endchoice
[KP] OK, it's doable. I was less thinking about "menuconfig" when making this change, but it will be better to have a choice for such case.
> 
>     if BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT
> 
>     config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_URL
>         string "URL of custom repository"
> 
>     config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_VERSION
>         string "Custom repository version"
> 
>     endif # BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT
> 
> Otherwise, we usually frown upon adding custom location to packages. Is it
> customary for board integrators to have local adaptations to this, like there is
> for u-boot and the other bootloaders?
> 
> As I see you're an @marvell.com, do you need that to use an internal
> repository during development? If so, then why can't you just use the
> OVERRIDE_SRCDIR mechanism instead?
[KP] It is more than just a local repository. I have to allow CI server to automatically build test and release images from the current internal GIT trees.
I am trying to make the Buildroot a package manager for Marvell SDK releases. 

Regards
Kosta
> 
> Regards,
> Yann E. MORIN.
> 
> > +if BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT
> > +
> > +config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_URL
> > +	string "URL of custom repository"
> > +
> > +config BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_VERSION
> > +	string "Custom repository version"
> > +	help
> > +	  Revision to use in the typical format used by Git
> > +	  E.G. a sha id, a tag, branch, ..
> > +
> > +endif
> > diff --git a/boot/mv-ddr-marvell/mv-ddr-marvell.mk
> > b/boot/mv-ddr-marvell/mv-ddr-marvell.mk
> > index 8d508215f8..100aaccb5f 100644
> > --- a/boot/mv-ddr-marvell/mv-ddr-marvell.mk
> > +++ b/boot/mv-ddr-marvell/mv-ddr-marvell.mk
> > @@ -4,9 +4,16 @@
> >  #
> >
> >
> ##########################################################
> ############
> > ##########
> >
> > +ifeq ($(BR2_TARGET_MV_DDR_MARVELL_CUSTOM_GIT),y)
> > +MV_DDR_MARVELL_VERSION = $(call
> > +qstrip,$(BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_VERSION))
> > +MV_DDR_MARVELL_SITE = $(call
> > +qstrip,$(BR2_TARGET_MV_DDR_MARVELL_CUSTOM_REPO_URL))
> > +MV_DDR_MARVELL_SITE_METHOD = git
> > +BR_NO_CHECK_HASH_FOR += $(MV_DDR_MARVELL_SOURCE) else
> >  # This is the commit for mv_ddr-armada-18.09.2
> > MV_DDR_MARVELL_VERSION =
> 99d772547314f84921268d57e53d8769197d3e21
> >  MV_DDR_MARVELL_SITE = $(call
> > github,MarvellEmbeddedProcessors,mv-ddr-
> marvell,$(MV_DDR_MARVELL_VERSI
> > ON))
> > +endif
> >  MV_DDR_MARVELL_LICENSE = GPL-2.0+ or LGPL-2.1 with
> > freertos-exception-2.0, BSD-3-Clause, Marvell Commercial
> > MV_DDR_MARVELL_LICENSE_FILES = ddr3_init.c
> >
> > --
> > 2.17.1
> >
> > _______________________________________________
> > buildroot mailing list
> > buildroot at busybox.net
> > http://lists.busybox.net/mailman/listinfo/buildroot
> 
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics'
> | conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'

[Buildroot] [EXT] Re: [PATCH] boot/mv-ddr-marvell: support custom GIT settings

[Thomas Petazzoni]

Hello Kostya,

On Tue, 25 Dec 2018 09:09:26 +0000, Kostya Porotchkin wrote:

> > As I see you're an @marvell.com, do you need that to use an internal
> > repository during development? If so, then why can't you just use the
> > OVERRIDE_SRCDIR mechanism instead?  
> [KP] It is more than just a local repository. I have to allow CI server to automatically build test and release images from the current internal GIT trees.

Sorry, but this is not a valid justification. With this justification,
we would have to add version selection to *all* Buildroot packages.
Indeed, one might want to do CI testing on any random user-space
library or application. So if your use-case for wanting version
selection in mv-ddr-marvell is CI testing, then we won't accept this
patch.

However, if people porting ATF/U-Boot to new Marvell-based platforms
need to have their own custom version of mv-ddr-marvell, then having
version selection can be accepted because it puts this package in the
same situation as uboot, linux or atf, where we do offer version
selection.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [EXT] Re: [PATCH] boot/mv-ddr-marvell: support custom GIT settings

[Kostya Porotchkin]

Hi, Thomas,

> -----Original Message-----
> From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Sent: Wednesday, December 26, 2018 12:32
> To: Kostya Porotchkin <kostap@marvell.com>
> Cc: Yann E. MORIN <yann.morin.1998@free.fr>; buildroot at buildroot.org
> Subject: Re: [Buildroot] [EXT] Re: [PATCH] boot/mv-ddr-marvell: support
> custom GIT settings
> 
> Hello Kostya,
> 
> On Tue, 25 Dec 2018 09:09:26 +0000, Kostya Porotchkin wrote:
> 
> > > As I see you're an @marvell.com, do you need that to use an internal
> > > repository during development? If so, then why can't you just use
> > > the OVERRIDE_SRCDIR mechanism instead?
> > [KP] It is more than just a local repository. I have to allow CI server to
> automatically build test and release images from the current internal GIT
> trees.
> 
> Sorry, but this is not a valid justification. With this justification, we would
> have to add version selection to *all* Buildroot packages.
> Indeed, one might want to do CI testing on any random user-space library or
> application. So if your use-case for wanting version selection in mv-ddr-
> marvell is CI testing, then we won't accept this patch.
[KP] Understand. We can keep this change private if you think others will not benefit from it.
> 
> However, if people porting ATF/U-Boot to new Marvell-based platforms
> need to have their own custom version of mv-ddr-marvell, then having
> version selection can be accepted because it puts this package in the same
> situation as uboot, linux or atf, where we do offer version selection.
[KP] I am trying to find a reason for customer to use different mv-ddr releases.
It may happen if a customer wants to use older ATF base, for instance v1.3 instead of v1.5 or v2.0.
In such case the DDR API in latest mv-ddr release will not be compatible with it and the build will fail.
Another reason is a customer willing to use latest fixes in mv-ddr GIT prior to the official release drop.
For instance, the new V7 Espressobin boards were released with DDR4 chips, while our official SW only supported DDR3.
So the new board customers were forced to use outdated SW from vendor builds that supported such memory type.

Regards
Kosta
> 
> Best regards,
> 
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

[Buildroot] [EXT] Re: [PATCH] boot/mv-ddr-marvell: support custom GIT settings

[Yann E. MORIN]

Kostya, All,

Thomas, questions for you at the end. ;-)

On 2018-12-26 10:59 +0000, Kostya Porotchkin spake thusly:
> > -----Original Message-----
> > From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > Sent: Wednesday, December 26, 2018 12:32
> > To: Kostya Porotchkin <kostap@marvell.com>
> > Cc: Yann E. MORIN <yann.morin.1998@free.fr>; buildroot at buildroot.org
> > Subject: Re: [Buildroot] [EXT] Re: [PATCH] boot/mv-ddr-marvell: support
> > custom GIT settings
> > 
> > Hello Kostya,
> > 
> > On Tue, 25 Dec 2018 09:09:26 +0000, Kostya Porotchkin wrote:
> > 
> > > > As I see you're an @marvell.com, do you need that to use an internal
> > > > repository during development? If so, then why can't you just use
> > > > the OVERRIDE_SRCDIR mechanism instead?
> > > [KP] It is more than just a local repository. I have to allow CI server to
> > automatically build test and release images from the current internal GIT
> > trees.
> > 
> > Sorry, but this is not a valid justification. With this justification, we would
> > have to add version selection to *all* Buildroot packages.
> > Indeed, one might want to do CI testing on any random user-space library or
> > application. So if your use-case for wanting version selection in mv-ddr-
> > marvell is CI testing, then we won't accept this patch.
> [KP] Understand. We can keep this change private if you think others will not benefit from it.

If one wants a CI job to test a specific commit of a package, then this
CI job has to be able to update the .config file and/or the defconfig
file to point to the new commit to test anyway, so this already means
there is a bit of "scripting" around to prepare for the test.

What we recommend in this case, is that said "scripting":

  - does a git-clone (or whatvere VCS one is using) to a temporary
    location (and since those CI jobs more often than not, use a
    docker or docker-like container to run the job, this is by nature
    ephemeral and temporary), e.g. something not unlike:
        git clone ${CI_JOB_URL} /path/to/temp/git/clone/${CI_JOB_NAME}

  - create a local.mk file in the Buildroot build directory, that
    contains the definition for a package override-srcdir, like:
        ${CI_JOB_NAME_UPPER}_OVERRIDE_SRCDIR = /path/to/temp/git/clone/${CI_JOB_NAME}

Which is a generic way to tell buildroot to use a specific local tree.
It is true that we usually advertise this override-srcdir mechanism for
development purposes, but it is also applicable for the CI testing stuff
as well.

As you also said eralier, you plan on using Buildroot as your "SDK" (not
sure I grok all you said). I thus understand that it will be public in
the end, so when you actually release a version of your "SDK", it would
point to a released and plublic commit of the official mv-dd-rmarvell
git tree on Github, so that means at some point someone will internaly
prepare that and submit a version update in Buildroot. ;-)

> > However, if people porting ATF/U-Boot to new Marvell-based platforms
> > need to have their own custom version of mv-ddr-marvell, then having
> > version selection can be accepted because it puts this package in the same
> > situation as uboot, linux or atf, where we do offer version selection.
> [KP] I am trying to find a reason for customer to use different mv-ddr releases.
> It may happen if a customer wants to use older ATF base, for instance v1.3 instead of v1.5 or v2.0.
> In such case the DDR API in latest mv-ddr release will not be compatible with it and the build will fail.
> Another reason is a customer willing to use latest fixes in mv-ddr GIT prior to the official release drop.
> For instance, the new V7 Espressobin boards were released with DDR4 chips, while our official SW only supported DDR3.
> So the new board customers were forced to use outdated SW from vendor builds that supported such memory type.

This is a kind of justification that would make us accept a version
choice, though. If there are reasons that people have to have a local
modified tree because they are porting it to their own devices, then it
makes sense we allow them to use a custom git tree, as Thomas said.

So, before reposting a new version:

  - I'd like Thomas to confirm we want to use a choice, like for the
    others where we allow using a custom VCS tree,

  - update your commit log to use the per-device customisation
    requirement as a justification for this change, not the CI
    testing. ;-)

Thomas, is that OK with you, in the end?

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [EXT] Re: [PATCH] boot/mv-ddr-marvell: support custom GIT settings

[Kostya Porotchkin]

Yann,

> -----Original Message-----
> From: Yann E. MORIN <yann.morin.1998@gmail.com> On Behalf Of Yann E.
> MORIN
> Sent: Wednesday, December 26, 2018 13:25
> To: Kostya Porotchkin <kostap@marvell.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>;
> buildroot at buildroot.org
> Subject: Re: [Buildroot] [EXT] Re: [PATCH] boot/mv-ddr-marvell: support
> custom GIT settings
> 
> Kostya, All,
> 
> Thomas, questions for you at the end. ;-)
> 
> On 2018-12-26 10:59 +0000, Kostya Porotchkin spake thusly:
> > > -----Original Message-----
> > > From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > > Sent: Wednesday, December 26, 2018 12:32
> > > To: Kostya Porotchkin <kostap@marvell.com>
> > > Cc: Yann E. MORIN <yann.morin.1998@free.fr>; buildroot at buildroot.org
> > > Subject: Re: [Buildroot] [EXT] Re: [PATCH] boot/mv-ddr-marvell:
> > > support custom GIT settings
> > >
> > > Hello Kostya,
> > >
> > > On Tue, 25 Dec 2018 09:09:26 +0000, Kostya Porotchkin wrote:
> > >
> > > > > As I see you're an @marvell.com, do you need that to use an
> > > > > internal repository during development? If so, then why can't
> > > > > you just use the OVERRIDE_SRCDIR mechanism instead?
> > > > [KP] It is more than just a local repository. I have to allow CI
> > > > server to
> > > automatically build test and release images from the current
> > > internal GIT trees.
> > >
> > > Sorry, but this is not a valid justification. With this
> > > justification, we would have to add version selection to *all* Buildroot
> packages.
> > > Indeed, one might want to do CI testing on any random user-space
> > > library or application. So if your use-case for wanting version
> > > selection in mv-ddr- marvell is CI testing, then we won't accept this patch.
> > [KP] Understand. We can keep this change private if you think others will
> not benefit from it.
> 
> If one wants a CI job to test a specific commit of a package, then this CI job
> has to be able to update the .config file and/or the defconfig file to point to
> the new commit to test anyway, so this already means there is a bit of
> "scripting" around to prepare for the test.
> 
> What we recommend in this case, is that said "scripting":
> 
>   - does a git-clone (or whatvere VCS one is using) to a temporary
>     location (and since those CI jobs more often than not, use a
>     docker or docker-like container to run the job, this is by nature
>     ephemeral and temporary), e.g. something not unlike:
>         git clone ${CI_JOB_URL} /path/to/temp/git/clone/${CI_JOB_NAME}
> 
>   - create a local.mk file in the Buildroot build directory, that
>     contains the definition for a package override-srcdir, like:
>         ${CI_JOB_NAME_UPPER}_OVERRIDE_SRCDIR =
> /path/to/temp/git/clone/${CI_JOB_NAME}
> 
> Which is a generic way to tell buildroot to use a specific local tree.
> It is true that we usually advertise this override-srcdir mechanism for
> development purposes, but it is also applicable for the CI testing stuff as well.
> 
> As you also said eralier, you plan on using Buildroot as your "SDK" (not sure I
> grok all you said).
[KP] Our intension to add vendor-specific (non-mainlined) components as external packages, which will be based on buildroot build system.
At some moment they will be public, but will remain vendor-specific and maintained out of the buildroot tree.

> I thus understand that it will be public in the end, so when
> you actually release a version of your "SDK", it would point to a released and
> plublic commit of the official mv-dd-rmarvell git tree on Github, so that
> means at some point someone will internaly prepare that and submit a
> version update in Buildroot. ;-)
[KP] Actually I can already update the mv-ddr-marvell package version - we released the 18.12 drop to Guthub yesterday.
> 
> > > However, if people porting ATF/U-Boot to new Marvell-based platforms
> > > need to have their own custom version of mv-ddr-marvell, then having
> > > version selection can be accepted because it puts this package in
> > > the same situation as uboot, linux or atf, where we do offer version
> selection.
> > [KP] I am trying to find a reason for customer to use different mv-ddr
> releases.
> > It may happen if a customer wants to use older ATF base, for instance v1.3
> instead of v1.5 or v2.0.
> > In such case the DDR API in latest mv-ddr release will not be compatible
> with it and the build will fail.
> > Another reason is a customer willing to use latest fixes in mv-ddr GIT prior
> to the official release drop.
> > For instance, the new V7 Espressobin boards were released with DDR4
> chips, while our official SW only supported DDR3.
> > So the new board customers were forced to use outdated SW from vendor
> builds that supported such memory type.
> 
> This is a kind of justification that would make us accept a version choice,
> though. If there are reasons that people have to have a local modified tree
> because they are porting it to their own devices, then it makes sense we
> allow them to use a custom git tree, as Thomas said.
> 
> So, before reposting a new version:
> 
>   - I'd like Thomas to confirm we want to use a choice, like for the
>     others where we allow using a custom VCS tree,
> 
>   - update your commit log to use the per-device customisation
>     requirement as a justification for this change, not the CI
>     testing. ;-)
> 
> Thomas, is that OK with you, in the end?
> 
> Regards,
> Yann E. MORIN.
> 
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics'
> | conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'

[Buildroot] [EXT] Re: [PATCH] boot/mv-ddr-marvell: support custom GIT settings

[Thomas Petazzoni]

Hello Kostya,

On Wed, 26 Dec 2018 12:33:56 +0000, Kostya Porotchkin wrote:

> [KP] Our intension to add vendor-specific (non-mainlined) components as external packages, which will be based on buildroot build system.
> At some moment they will be public, but will remain vendor-specific and maintained out of the buildroot tree.

I don't think you should make the assumption that those packages will
remain "out of the buildroot tree". As soon as something is publicly
available, there is a chance that someone will be interested in having
it supported as part of upstream Buildroot. So, you shouldn't base your
design on the assumption that this or that package "will remain out of
the Buildroot tree" because this may not be true in the future.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [EXT] Re: [PATCH] boot/mv-ddr-marvell: support custom GIT settings

[Thomas Petazzoni]

Hello,

On Wed, 26 Dec 2018 12:25:02 +0100, Yann E. MORIN wrote:

> What we recommend in this case, is that said "scripting":
> 
>   - does a git-clone (or whatvere VCS one is using) to a temporary
>     location (and since those CI jobs more often than not, use a
>     docker or docker-like container to run the job, this is by nature
>     ephemeral and temporary), e.g. something not unlike:
>         git clone ${CI_JOB_URL} /path/to/temp/git/clone/${CI_JOB_NAME}
> 
>   - create a local.mk file in the Buildroot build directory, that
>     contains the definition for a package override-srcdir, like:
>         ${CI_JOB_NAME_UPPER}_OVERRIDE_SRCDIR = /path/to/temp/git/clone/${CI_JOB_NAME}

Not ${CI_JOB_NAME_UPPER}_OVERRIDE_SRCDIR, but <pkg>_OVERRIDE_SRCDIR.

> As you also said eralier, you plan on using Buildroot as your "SDK" (not
> sure I grok all you said).

Marvell doesn't use the term "SDK" in the same sense as we do in the
upstream Buildroot:

 - In upstream Buildroot, what we call the "SDK" is what is generated
   by "make sdk", i.e the cross-compilation toolchain + all the
   libraries and headers that are needed to build code for the target,
   matching a given Buildroot configuration.

 - In Marvell-speak, the "SDK" is basically an entire build system +
   set of source code tarballs that will allow you to build a Linux
   system for your target. Basically, Marvell SDK is a customized
   Buildroot + external trees + source code tarballs.


> This is a kind of justification that would make us accept a version
> choice, though. If there are reasons that people have to have a local
> modified tree because they are porting it to their own devices, then it
> makes sense we allow them to use a custom git tree, as Thomas said.
> 
> So, before reposting a new version:
> 
>   - I'd like Thomas to confirm we want to use a choice, like for the
>     others where we allow using a custom VCS tree,

I'm still unsure whether people making custom Marvell boards generally
need to change mv-ddr-marvell code. However, it is true that the
mv-ddr-marvell code version needs to be in sync with the ATF version in
use (last time I updated the Macchiatobin defconfigs in upstream
Buildroot, I had to update both ATF and mv-ddr-marvell at the same
time). It is also highly HW-related low-level code, so I think it is OK
to have a version selection.

>   - update your commit log to use the per-device customisation
>     requirement as a justification for this change, not the CI
>     testing. ;-)
> 
> Thomas, is that OK with you, in the end?

Yes, provided the code is changed to use a version selection more
similar to what we use in other packages *and* the commit log is
adjusted, I'm fine with merging something like this.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com