Re: [PATCH 1/2] config: Add 'update_bootimg' command to update flash.bin on Phytec's imx8mm

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 3654 bytes --]

Hi Benjamin,

> Hi Lukasz,
> 
> On 01.08.24 14:54, Lukasz Majewski wrote:
> > This command allows easy update on SD card (hence the
> > update_mmc_part=1) of the flash.bin generated during u-boot build.
> >
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> >   include/configs/phycore_imx8mm.h | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/include/configs/phycore_imx8mm.h
> > b/include/configs/phycore_imx8mm.h index ce6dc87c69..fdeb11933f
> > 100644 --- a/include/configs/phycore_imx8mm.h
> > +++ b/include/configs/phycore_imx8mm.h
> > @@ -29,6 +29,17 @@
> >   	"mmcdev=" __stringify(CONFIG_SYS_MMC_ENV_DEV) "\0" \
> >   	"mmcpart=1\0" \
> >   	"mmcroot=2\0" \
> > +	"update_mmc_part=1\0" \  
> 
> You define the update_mmc_part variable here, but do not use it 
> anywhere. You use the mmcdev variable later which is good, because
> the function then works not only for SD-Card, but also for eMMC.
> 
> In the commit description you say it is for updating SD-Card, which 
> would be mmc dev 1, but the name of the variable would indicate that
> is is for a partition.
> As I understand it you want to update just the U-Boot, which is not 
> inside any partition, so you would not need this variable.

Yes, for updating u-boot it is not required. I will remove it.

> 
> > +	"update_offset=0x42\0" \
> > +	"update_filename=flash.bin\0" \
> > +	"hostname=/tftpboot/lukma/\0" \  
> We adivse to use /srv/tftp  as tftp dir in our BSP documentation. But
> I don't think you need that variable at all, because the dhcp command 
> should find the tftp dir automatically.

I can follow your documentation adn add /srv/tftp

> > +	"update_bootimg="
> > 	\
> > +		"mmc dev ${mmcdev} ; "		\
> > +		"if dhcp ${hostname}/${update_filename} ; then
> > "	\  
> 
> Does this work? For me it does not. As far as I know the syntax of
> this command is "dhcp <loadaddr> <filename>". So I would expect this
> to be: dhcp ${loadaddr} ${update_filename}
> 

When you don't provide the <loadaddr> the one from ${loadaddr} is used
by default.

However, yes - the latter - i.e. one with ${loadaddr} is more readable
and shall be used.

> > +		"setexpr fw_sz ${filesize} / 0x200 ; "	/*
> > SD block size */ \
> > +		"setexpr fw_sz ${fw_sz} + 1 ; "
> > 		\  
> Why do you add one here? Is this important for something?

Yes, it is. The / division is only giving you the value without
reminder. As the filesize can be not aligned to 0x200 you wouldn't
flash the whole binary.

> For me it 
> works also without the one.

Sometimes it works, sometimes not - it depends if you crop the relevant
part.

> It is also described in our BSP documentation without adding one. See:
> https://phytec.github.io/doc-bsp-yocto/bsp/imx8/imx8mp/mainline-head.html#flash-emmc-u-boot-image-via-network-from-running-u-boot 
> 

If you are 100% sure that all the time the size of the binary is
aligned to 0x200, then you can omit the 1 as well.

> (This is for our imx8mp mainline release but is the same for imx8mm
> with another offset).
> 
> Benjamin
> > +		"mmc write ${loadaddr} ${update_offset} ${fw_sz} ;
> > "	\
> > +		"fi\0" \
> >   	"mmcautodetect=yes\0" \
> >   	"mmcargs=setenv bootargs console=${console} " \
> >   		"root=/dev/mmcblk${mmcdev}p${mmcroot} rootwait
> > rw\0" \  
> 
> 


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]