[v3,2/5] Makefile: Rename idbloader.img with u-boot-spl-rockchip.bin
diff mbox series

Message ID 20191017190710.29985-3-jagan@amarulasolutions.com
State New
Headers show
Series
  • rockchip: Add Binman support
Related show

Commit Message

Jagan Teki Oct. 17, 2019, 7:07 p.m. UTC
idbloader.img name is specific to rockchip, where it usually
created using rockchip tools. Since the image is created as
per U-Boot generic builds like SPL, better to follow the
generic U-Boot naming notation like other SoC's following.

Enable idbloader.img with u-boot-spl-rockchip.bin and create
the same in spl directory since it is SPL file and Makefile.spl
will clean it by default.

Cc: Kever Yang <kever.yang@rock-chips.com>
Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Kever Yang Oct. 18, 2019, 10:56 a.m. UTC | #1
Jagan,


On 2019/10/18 上午3:07, Jagan Teki wrote:
> idbloader.img name is specific to rockchip,

This is specific for rockchip, like rksd, rkspi type in mkimage, since 
it's clear for what it stands for,

I think it can also used in U-Boot.

> where it usually
> created using rockchip tools.


No, idbloader stands for the image with idb header packaged with two 
stage loader, eg. TPL+SPL

or ddr.bin+miniloader.bin, and maybe ddr.bin + SPL, TPL+miniloader.bin,

not related to rockchip tools, it can also be created with U-Boot 
mkimage tool.

> Since the image is created as
> per U-Boot generic builds like SPL, better to follow the
> generic U-Boot naming notation like other SoC's following.
>
> Enable idbloader.img with u-boot-spl-rockchip.bin and create
> the same in spl directory since it is SPL file and Makefile.spl
> will clean it by default.
>
> Cc: Kever Yang <kever.yang@rock-chips.com>
> Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>   Makefile | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index ec55e0f6a4..918b5d53e0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -887,7 +887,7 @@ ALL-y += u-boot-with-dtb.bin
>   endif
>   
>   ifeq ($(CONFIG_ARCH_ROCKCHIP)$(CONFIG_SPL)$(CONFIG_TPL),yyy)
> -ALL-y += idbloader.img
> +ALL-y += spl/u-boot-spl-rockchip.bin


This idbloader including IDB header, TPL, and SPL, I don't think name it 
as spl and

put it in spl directory make any sense.

I though the origin output file use postfix '.bin' and those image after 
pack and can  be

dd to storage directly better to use postfix '.img'.

About the clean, it will be better to be clean like other '.img' file in 
the Makefile.


Thanks,

- Kever

>   endif
>   
>   LDFLAGS_u-boot += $(LDFLAGS_FINAL)
> @@ -1344,7 +1344,7 @@ endif
>   MKIMAGEFLAGS_u-boot-tpl.img = -n $(CONFIG_SYS_SOC) -T $(ROCKCHIP_IMG_TYPE)
>   tpl/u-boot-tpl.img: tpl/u-boot-tpl.bin FORCE
>   	$(call if_changed,mkimage)
> -idbloader.img: tpl/u-boot-tpl.img spl/u-boot-spl.bin FORCE
> +spl/u-boot-spl-rockchip.bin: tpl/u-boot-tpl.img spl/u-boot-spl.bin FORCE
>   	$(call if_changed,cat)
>   endif
>
Jagan Teki Oct. 21, 2019, 5:26 a.m. UTC | #2
Hi Kever,

On Fri, Oct 18, 2019 at 4:26 PM Kever Yang <kever.yang@rock-chips.com> wrote:
>
> Jagan,
>
>
> On 2019/10/18 上午3:07, Jagan Teki wrote:
> > idbloader.img name is specific to rockchip,
>
> This is specific for rockchip, like rksd, rkspi type in mkimage, since
> it's clear for what it stands for,
>
> I think it can also used in U-Boot.
>
> > where it usually
> > created using rockchip tools.
>
>
> No, idbloader stands for the image with idb header packaged with two
> stage loader, eg. TPL+SPL
>
> or ddr.bin+miniloader.bin, and maybe ddr.bin + SPL, TPL+miniloader.bin,
>
> not related to rockchip tools, it can also be created with U-Boot
> mkimage tool.

I understand what idbloader contains, it is like rockchip SPL. but the
main concern here is the naming convention used in U-Boot. It would be
a standard way of using naming conventions where the final output
naming conventions should have u-boot support stages(TPL, SPL, U-Boot)
and platform specific name. This way it is less confused what exactly
it contains.

Moreover most of the platform specific bin ended up using this type of
conventions.

- u-boot-spl-mtk.bin - Mediatek SPL bin
- u-boot-mtk.bin - Mediatek U-Boot bin (would be final image)
- u-boot-sunxi-with-spl.bin - Allwinner U-Boot with SPL
- u-boot-x86-start16-tpl.bin

Similar naming conventions used for tegra, x86 etc.

I'm completely aware of what you're trying to abbreviate
idbloader.img, but having u-boot-spl-rockchip.bin is more readable and
understand than idbloader.

>
> > Since the image is created as
> > per U-Boot generic builds like SPL, better to follow the
> > generic U-Boot naming notation like other SoC's following.
> >
> > Enable idbloader.img with u-boot-spl-rockchip.bin and create
> > the same in spl directory since it is SPL file and Makefile.spl
> > will clean it by default.
> >
> > Cc: Kever Yang <kever.yang@rock-chips.com>
> > Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >   Makefile | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index ec55e0f6a4..918b5d53e0 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -887,7 +887,7 @@ ALL-y += u-boot-with-dtb.bin
> >   endif
> >
> >   ifeq ($(CONFIG_ARCH_ROCKCHIP)$(CONFIG_SPL)$(CONFIG_TPL),yyy)
> > -ALL-y += idbloader.img
> > +ALL-y += spl/u-boot-spl-rockchip.bin
>
>
> This idbloader including IDB header, TPL, and SPL, I don't think name it
> as spl and
>
> put it in spl directory make any sense.
>
> I though the origin output file use postfix '.bin' and those image after
> pack and can  be
>
> dd to storage directly better to use postfix '.img'.
>
> About the clean, it will be better to be clean like other '.img' file in
> the Makefile.

This make sense, I have next version set where it used handle these so
carefully.
Jagan Teki Oct. 22, 2019, 10:10 a.m. UTC | #3
On Mon, Oct 21, 2019 at 1:43 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Jagan Teki <jagan@amarulasolutions.com>
> > Date: Mon, 21 Oct 2019 10:56:39 +0530
> >
> > Hi Kever,
> >
> > On Fri, Oct 18, 2019 at 4:26 PM Kever Yang <kever.yang@rock-chips.com> wrote:
> > >
> > > Jagan,
> > >
> > >
> > > On 2019/10/18 上午3:07, Jagan Teki wrote:
> > > > idbloader.img name is specific to rockchip,
> > >
> > > This is specific for rockchip, like rksd, rkspi type in mkimage, since
> > > it's clear for what it stands for,
> > >
> > > I think it can also used in U-Boot.
> > >
> > > > where it usually
> > > > created using rockchip tools.
> > >
> > >
> > > No, idbloader stands for the image with idb header packaged with two
> > > stage loader, eg. TPL+SPL
> > >
> > > or ddr.bin+miniloader.bin, and maybe ddr.bin + SPL, TPL+miniloader.bin,
> > >
> > > not related to rockchip tools, it can also be created with U-Boot
> > > mkimage tool.
> >
> > I understand what idbloader contains, it is like rockchip SPL. but the
> > main concern here is the naming convention used in U-Boot. It would be
> > a standard way of using naming conventions where the final output
> > naming conventions should have u-boot support stages(TPL, SPL, U-Boot)
> > and platform specific name. This way it is less confused what exactly
> > it contains.
> >
> > Moreover most of the platform specific bin ended up using this type of
> > conventions.
> >
> > - u-boot-spl-mtk.bin - Mediatek SPL bin
> > - u-boot-mtk.bin - Mediatek U-Boot bin (would be final image)
> > - u-boot-sunxi-with-spl.bin - Allwinner U-Boot with SPL
> > - u-boot-x86-start16-tpl.bin
>
> No real consistency there though...

Not sure what you pointed here, each bin listed above shows the
platform and u-boot stage and prefix with u-boot- as well. of course
all can't maintain the same but has mentioned details.

>
> > Similar naming conventions used for tegra, x86 etc.
> >
> > I'm completely aware of what you're trying to abbreviate
> > idbloader.img, but having u-boot-spl-rockchip.bin is more readable and
> > understand than idbloader.
>
> As was discussed before, idbloader.img is the name used in external
> documentation.

Yes, used in external documentation. when it comes to upstream it
would follow as per existing platforms does atleast.
Kever Yang Nov. 15, 2019, 6:34 a.m. UTC | #4
Hi Jagan,

On 2019/10/22 下午6:10, Jagan Teki wrote:
> On Mon, Oct 21, 2019 at 1:43 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>> From: Jagan Teki <jagan@amarulasolutions.com>
>>> Date: Mon, 21 Oct 2019 10:56:39 +0530
>>>
>>> Hi Kever,
>>>
>>> On Fri, Oct 18, 2019 at 4:26 PM Kever Yang <kever.yang@rock-chips.com> wrote:
>>>> Jagan,
>>>>
>>>>
>>>> On 2019/10/18 上午3:07, Jagan Teki wrote:
>>>>> idbloader.img name is specific to rockchip,
>>>> This is specific for rockchip, like rksd, rkspi type in mkimage, since
>>>> it's clear for what it stands for,
>>>>
>>>> I think it can also used in U-Boot.
>>>>
>>>>> where it usually
>>>>> created using rockchip tools.
>>>>
>>>> No, idbloader stands for the image with idb header packaged with two
>>>> stage loader, eg. TPL+SPL
>>>>
>>>> or ddr.bin+miniloader.bin, and maybe ddr.bin + SPL, TPL+miniloader.bin,
>>>>
>>>> not related to rockchip tools, it can also be created with U-Boot
>>>> mkimage tool.
>>> I understand what idbloader contains, it is like rockchip SPL. but the
>>> main concern here is the naming convention used in U-Boot. It would be
>>> a standard way of using naming conventions where the final output
>>> naming conventions should have u-boot support stages(TPL, SPL, U-Boot)
>>> and platform specific name. This way it is less confused what exactly
>>> it contains.
>>>
>>> Moreover most of the platform specific bin ended up using this type of
>>> conventions.
>>>
>>> - u-boot-spl-mtk.bin - Mediatek SPL bin
>>> - u-boot-mtk.bin - Mediatek U-Boot bin (would be final image)
>>> - u-boot-sunxi-with-spl.bin - Allwinner U-Boot with SPL
>>> - u-boot-x86-start16-tpl.bin
>> No real consistency there though...
> Not sure what you pointed here, each bin listed above shows the
> platform and u-boot stage and prefix with u-boot- as well. of course
> all can't maintain the same but has mentioned details.
The image for Vendor's BootRom is always vendor specific, so I don't 
think invent
a new name for idbloader.img make any sense.
I know what's the u-boot.bin, u-boot.itb, u-boot-spl.bin, 
u-boot-tpl.bin, because all the
boards use them in the same way,  but I don't know what is 
u-boot-spl-mtk.bin or u-boot-sunxi-with-spl.bin,
and also names like lpc32xx-***, because they always specific for the 
SoC vendor, people never know
what it's before they work on this platform.
For  idbloader.img, it already used for  a period of time, it's document 
in both Rockchip document
and U-Boot document, developers know what it's when they see this name, 
I don't think add a new
name for it and add a new document and discards the document with old 
name is a good idea,
it only confuse developers. You can have a new name for binman output  
for that's new output,
but I would like not to invent new name for idbloader until most of 
developers think that's really
necessary.


Thanks,
- Kever
>
>>> Similar naming conventions used for tegra, x86 etc.
>>>
>>> I'm completely aware of what you're trying to abbreviate
>>> idbloader.img, but having u-boot-spl-rockchip.bin is more readable and
>>> understand than idbloader.
>> As was discussed before, idbloader.img is the name used in external
>> documentation.
> Yes, used in external documentation. when it comes to upstream it
> would follow as per existing platforms does atleast.
>

Patch
diff mbox series

diff --git a/Makefile b/Makefile
index ec55e0f6a4..918b5d53e0 100644
--- a/Makefile
+++ b/Makefile
@@ -887,7 +887,7 @@  ALL-y += u-boot-with-dtb.bin
 endif
 
 ifeq ($(CONFIG_ARCH_ROCKCHIP)$(CONFIG_SPL)$(CONFIG_TPL),yyy)
-ALL-y += idbloader.img
+ALL-y += spl/u-boot-spl-rockchip.bin
 endif
 
 LDFLAGS_u-boot += $(LDFLAGS_FINAL)
@@ -1344,7 +1344,7 @@  endif
 MKIMAGEFLAGS_u-boot-tpl.img = -n $(CONFIG_SYS_SOC) -T $(ROCKCHIP_IMG_TYPE)
 tpl/u-boot-tpl.img: tpl/u-boot-tpl.bin FORCE
 	$(call if_changed,mkimage)
-idbloader.img: tpl/u-boot-tpl.img spl/u-boot-spl.bin FORCE
+spl/u-boot-spl-rockchip.bin: tpl/u-boot-tpl.img spl/u-boot-spl.bin FORCE
 	$(call if_changed,cat)
 endif