[3/3] mmc: support writing sparse images

Message ID 1522996524-23376-1-git-send-email-jassisinghbrar@gmail.com
State New
Headers show
Series
  • Enable mmc to write sparse images
Related show

Commit Message

Jassi Brar April 6, 2018, 6:35 a.m.
From: Jassi Brar <jaswinder.singh@linaro.org>

Provide an alternate path for sparse-images to be
written to MMC. For example, via tftp on platforms
that don't support fastboot protocol. Or when an
image is to written at some offset, rather than the
start of a partition.

Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 cmd/mmc.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

Comments

Tom Rini May 8, 2018, 5:15 p.m. | #1
On Fri, Apr 06, 2018 at 12:05:24PM +0530, jassisinghbrar@gmail.com wrote:

> From: Jassi Brar <jaswinder.singh@linaro.org>

> 

> Provide an alternate path for sparse-images to be

> written to MMC. For example, via tftp on platforms

> that don't support fastboot protocol. Or when an

> image is to written at some offset, rather than the

> start of a partition.

> 

> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>


Applied to u-boot/master, thanks!

-- 
Tom
Jassi Brar May 14, 2018, 1:12 p.m. | #2
Hi Tom,

On Tue, May 8, 2018 at 10:45 PM, Tom Rini <trini@konsulko.com> wrote:
> On Fri, Apr 06, 2018 at 12:05:24PM +0530, jassisinghbrar@gmail.com wrote:
>
>> From: Jassi Brar <jaswinder.singh@linaro.org>
>>
>> Provide an alternate path for sparse-images to be
>> written to MMC. For example, via tftp on platforms
>> that don't support fastboot protocol. Or when an
>> image is to written at some offset, rather than the
>> start of a partition.
>>
>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
>
> Applied to u-boot/master, thanks!
>
I see you modified the patch to protect the feature with
CONFIG_FASTBOOT_FLASH, which kills the purpose -- this feature is for
platforms that don't support fastboot.

Do you want me to send the patch to revert the protection?

Thanks.
Tom Rini May 14, 2018, 2:46 p.m. | #3
On Mon, May 14, 2018 at 06:42:41PM +0530, Jassi Brar wrote:
> Hi Tom,

> 

> On Tue, May 8, 2018 at 10:45 PM, Tom Rini <trini@konsulko.com> wrote:

> > On Fri, Apr 06, 2018 at 12:05:24PM +0530, jassisinghbrar@gmail.com wrote:

> >

> >> From: Jassi Brar <jaswinder.singh@linaro.org>

> >>

> >> Provide an alternate path for sparse-images to be

> >> written to MMC. For example, via tftp on platforms

> >> that don't support fastboot protocol. Or when an

> >> image is to written at some offset, rather than the

> >> start of a partition.

> >>

> >> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>

> >

> > Applied to u-boot/master, thanks!

>

> I see you modified the patch to protect the feature with

> CONFIG_FASTBOOT_FLASH, which kills the purpose -- this feature is for

> platforms that don't support fastboot.

> 

> Do you want me to send the patch to revert the protection?


Sorry, I guess maybe things weren't clear enough all around, and we
should (functionally) revert patches 2 and 3 and try something
different.  It is OK to say "lets make writing sparse images more widely
available".  It's not OK to make every platform with MMC write grow a
decent bit in binary size.  Making a quick pass at re-enabling things on
a platform without fastboot support already grew things by nearly 2KiB.
The other part which is I believe got me down this path was that without
a change to common/Makefile to always (outside of SPL) include
common/image-sparse.o things don't link.

In sum, a new patch to add an option to allow people to opt-in for
swrite would be good.  And please make sure to do something like:
diff --git a/common/Makefile b/common/Makefile
index d0681c7dd96a..92b2aa1ca8f0 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -120,6 +120,7 @@ ifdef CONFIG_FASTBOOT_FLASH_NAND_DEV
 obj-y += fb_nand.o
 endif
 endif
+obj-$(CONFIG_MMC_WRITE) += image-sparse.o
 endif
 
 ifdef CONFIG_CMD_EEPROM_LAYOUT

too so it links everywhere.  Thanks!

-- 
Tom
Alex Kiernan May 14, 2018, 4:14 p.m. | #4
On Mon, May 14, 2018 at 3:47 PM Tom Rini <trini@konsulko.com> wrote:

> On Mon, May 14, 2018 at 06:42:41PM +0530, Jassi Brar wrote:
> > Hi Tom,
> >
> > On Tue, May 8, 2018 at 10:45 PM, Tom Rini <trini@konsulko.com> wrote:
> > > On Fri, Apr 06, 2018 at 12:05:24PM +0530, jassisinghbrar@gmail.com
wrote:
> > >
> > >> From: Jassi Brar <jaswinder.singh@linaro.org>
> > >>
> > >> Provide an alternate path for sparse-images to be
> > >> written to MMC. For example, via tftp on platforms
> > >> that don't support fastboot protocol. Or when an
> > >> image is to written at some offset, rather than the
> > >> start of a partition.
> > >>
> > >> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > >
> > > Applied to u-boot/master, thanks!
> >
> > I see you modified the patch to protect the feature with
> > CONFIG_FASTBOOT_FLASH, which kills the purpose -- this feature is for
> > platforms that don't support fastboot.
> >
> > Do you want me to send the patch to revert the protection?

> Sorry, I guess maybe things weren't clear enough all around, and we
> should (functionally) revert patches 2 and 3 and try something
> different.  It is OK to say "lets make writing sparse images more widely
> available".  It's not OK to make every platform with MMC write grow a
> decent bit in binary size.  Making a quick pass at re-enabling things on
> a platform without fastboot support already grew things by nearly 2KiB.
> The other part which is I believe got me down this path was that without
> a change to common/Makefile to always (outside of SPL) include
> common/image-sparse.o things don't link.

> In sum, a new patch to add an option to allow people to opt-in for
> swrite would be good.  And please make sure to do something like:
> diff --git a/common/Makefile b/common/Makefile
> index d0681c7dd96a..92b2aa1ca8f0 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -120,6 +120,7 @@ ifdef CONFIG_FASTBOOT_FLASH_NAND_DEV
>       obj-y += fb_nand.o
>       endif
>       endif
> +obj-$(CONFIG_MMC_WRITE) += image-sparse.o
>       endif

Isn't this just move image-sparse to lib and add a separate guard for it
(LIB_IMAGE_SPARSE?) which can be selected by both FASTBOOT and a new
command symbol (CMD_MMC_SWRITE)?

This is all overlapping with the UDP fastboot code I've been posting, so
I'd kinda like it to fit nicely with that, rather than have to refactor it
to fit something different!

--
Alex Kiernan
Tom Rini May 14, 2018, 8:50 p.m. | #5
On Mon, May 14, 2018 at 05:14:42PM +0100, Alex Kiernan wrote:
> On Mon, May 14, 2018 at 3:47 PM Tom Rini <trini@konsulko.com> wrote:

> 

> > On Mon, May 14, 2018 at 06:42:41PM +0530, Jassi Brar wrote:

> > > Hi Tom,

> > >

> > > On Tue, May 8, 2018 at 10:45 PM, Tom Rini <trini@konsulko.com> wrote:

> > > > On Fri, Apr 06, 2018 at 12:05:24PM +0530, jassisinghbrar@gmail.com

> wrote:

> > > >

> > > >> From: Jassi Brar <jaswinder.singh@linaro.org>

> > > >>

> > > >> Provide an alternate path for sparse-images to be

> > > >> written to MMC. For example, via tftp on platforms

> > > >> that don't support fastboot protocol. Or when an

> > > >> image is to written at some offset, rather than the

> > > >> start of a partition.

> > > >>

> > > >> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>

> > > >

> > > > Applied to u-boot/master, thanks!

> > >

> > > I see you modified the patch to protect the feature with

> > > CONFIG_FASTBOOT_FLASH, which kills the purpose -- this feature is for

> > > platforms that don't support fastboot.

> > >

> > > Do you want me to send the patch to revert the protection?

> 

> > Sorry, I guess maybe things weren't clear enough all around, and we

> > should (functionally) revert patches 2 and 3 and try something

> > different.  It is OK to say "lets make writing sparse images more widely

> > available".  It's not OK to make every platform with MMC write grow a

> > decent bit in binary size.  Making a quick pass at re-enabling things on

> > a platform without fastboot support already grew things by nearly 2KiB.

> > The other part which is I believe got me down this path was that without

> > a change to common/Makefile to always (outside of SPL) include

> > common/image-sparse.o things don't link.

> 

> > In sum, a new patch to add an option to allow people to opt-in for

> > swrite would be good.  And please make sure to do something like:

> > diff --git a/common/Makefile b/common/Makefile

> > index d0681c7dd96a..92b2aa1ca8f0 100644

> > --- a/common/Makefile

> > +++ b/common/Makefile

> > @@ -120,6 +120,7 @@ ifdef CONFIG_FASTBOOT_FLASH_NAND_DEV

> >       obj-y += fb_nand.o

> >       endif

> >       endif

> > +obj-$(CONFIG_MMC_WRITE) += image-sparse.o

> >       endif

> 

> Isn't this just move image-sparse to lib and add a separate guard for it

> (LIB_IMAGE_SPARSE?) which can be selected by both FASTBOOT and a new

> command symbol (CMD_MMC_SWRITE)?

> 

> This is all overlapping with the UDP fastboot code I've been posting, so

> I'd kinda like it to fit nicely with that, rather than have to refactor it

> to fit something different!


That sounds like a good idea to me, thanks!

-- 
Tom
Alex Kiernan May 14, 2018, 9:08 p.m. | #6
On Mon, May 14, 2018 at 9:50 PM Tom Rini <trini@konsulko.com> wrote:

> On Mon, May 14, 2018 at 05:14:42PM +0100, Alex Kiernan wrote:
> > On Mon, May 14, 2018 at 3:47 PM Tom Rini <trini@konsulko.com> wrote:
> >
> > > On Mon, May 14, 2018 at 06:42:41PM +0530, Jassi Brar wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, May 8, 2018 at 10:45 PM, Tom Rini <trini@konsulko.com>
wrote:
> > > > > On Fri, Apr 06, 2018 at 12:05:24PM +0530, jassisinghbrar@gmail.com
> > wrote:
> > > > >
> > > > >> From: Jassi Brar <jaswinder.singh@linaro.org>
> > > > >>
> > > > >> Provide an alternate path for sparse-images to be
> > > > >> written to MMC. For example, via tftp on platforms
> > > > >> that don't support fastboot protocol. Or when an
> > > > >> image is to written at some offset, rather than the
> > > > >> start of a partition.
> > > > >>
> > > > >> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > > > >
> > > > > Applied to u-boot/master, thanks!
> > > >
> > > > I see you modified the patch to protect the feature with
> > > > CONFIG_FASTBOOT_FLASH, which kills the purpose -- this feature is
for
> > > > platforms that don't support fastboot.
> > > >
> > > > Do you want me to send the patch to revert the protection?
> >
> > > Sorry, I guess maybe things weren't clear enough all around, and we
> > > should (functionally) revert patches 2 and 3 and try something
> > > different.  It is OK to say "lets make writing sparse images more
widely
> > > available".  It's not OK to make every platform with MMC write grow a
> > > decent bit in binary size.  Making a quick pass at re-enabling things
on
> > > a platform without fastboot support already grew things by nearly
2KiB.
> > > The other part which is I believe got me down this path was that
without
> > > a change to common/Makefile to always (outside of SPL) include
> > > common/image-sparse.o things don't link.
> >
> > > In sum, a new patch to add an option to allow people to opt-in for
> > > swrite would be good.  And please make sure to do something like:
> > > diff --git a/common/Makefile b/common/Makefile
> > > index d0681c7dd96a..92b2aa1ca8f0 100644
> > > --- a/common/Makefile
> > > +++ b/common/Makefile
> > > @@ -120,6 +120,7 @@ ifdef CONFIG_FASTBOOT_FLASH_NAND_DEV
> > >       obj-y += fb_nand.o
> > >       endif
> > >       endif
> > > +obj-$(CONFIG_MMC_WRITE) += image-sparse.o
> > >       endif
> >
> > Isn't this just move image-sparse to lib and add a separate guard for it
> > (LIB_IMAGE_SPARSE?) which can be selected by both FASTBOOT and a new
> > command symbol (CMD_MMC_SWRITE)?
> >
> > This is all overlapping with the UDP fastboot code I've been posting, so
> > I'd kinda like it to fit nicely with that, rather than have to refactor
it
> > to fit something different!

> That sounds like a good idea to me, thanks!


I'll work it into that patch series... having just had a quick look at it,
it's going to be easiest part way through it after I've done most of the
Kconfig reworking.

Patch

diff --git a/cmd/mmc.c b/cmd/mmc.c
index 58fdc36..5ced6e7 100644
--- a/cmd/mmc.c
+++ b/cmd/mmc.c
@@ -9,6 +9,8 @@ 
 #include <command.h>
 #include <console.h>
 #include <mmc.h>
+#include <sparse_format.h>
+#include <image-sparse.h>
 
 static int curr_device = -1;
 
@@ -308,6 +310,69 @@  static int do_mmc_read(cmd_tbl_t *cmdtp, int flag,
 }
 
 #if CONFIG_IS_ENABLED(MMC_WRITE)
+static lbaint_t mmc_sparse_write(struct sparse_storage *info, lbaint_t blk,
+				 lbaint_t blkcnt, const void *buffer)
+{
+	struct blk_desc *dev_desc = info->priv;
+
+	return blk_dwrite(dev_desc, blk, blkcnt, buffer);
+}
+
+static lbaint_t mmc_sparse_reserve(struct sparse_storage *info,
+				   lbaint_t blk, lbaint_t blkcnt)
+{
+	return blkcnt;
+}
+
+static int do_mmc_sparse_write(cmd_tbl_t *cmdtp, int flag,
+			       int argc, char * const argv[])
+{
+	struct sparse_storage sparse;
+	struct blk_desc *dev_desc;
+	struct mmc *mmc;
+	char dest[11];
+	void *addr;
+	u32 blk;
+
+	if (argc != 3)
+		return CMD_RET_USAGE;
+
+	addr = (void *)simple_strtoul(argv[1], NULL, 16);
+	blk = simple_strtoul(argv[2], NULL, 16);
+
+	if (!is_sparse_image(addr)) {
+		printf("Not a sparse image\n");
+		return CMD_RET_FAILURE;
+	}
+
+	mmc = init_mmc_device(curr_device, false);
+	if (!mmc)
+		return CMD_RET_FAILURE;
+
+	printf("\nMMC Sparse write: dev # %d, block # %d ... ",
+	       curr_device, blk);
+
+	if (mmc_getwp(mmc) == 1) {
+		printf("Error: card is write protected!\n");
+		return CMD_RET_FAILURE;
+	}
+
+	dev_desc = mmc_get_blk_desc(mmc);
+	sparse.priv = dev_desc;
+	sparse.blksz = 512;
+	sparse.start = blk;
+	sparse.size = dev_desc->lba - blk;
+	sparse.write = mmc_sparse_write;
+	sparse.reserve = mmc_sparse_reserve;
+	sparse.mssg = NULL;
+	sprintf(dest, "0x%08lX", sparse.start * sparse.blksz);
+
+	if (write_sparse_image(&sparse, dest, addr))
+		return CMD_RET_FAILURE;
+	else
+		return CMD_RET_SUCCESS;
+}
+
 static int do_mmc_write(cmd_tbl_t *cmdtp, int flag,
 			int argc, char * const argv[])
 {
@@ -802,6 +867,7 @@  static cmd_tbl_t cmd_mmc[] = {
 	U_BOOT_CMD_MKENT(read, 4, 1, do_mmc_read, "", ""),
 #if CONFIG_IS_ENABLED(MMC_WRITE)
 	U_BOOT_CMD_MKENT(write, 4, 0, do_mmc_write, "", ""),
+	U_BOOT_CMD_MKENT(swrite, 3, 0, do_mmc_sparse_write, "", ""),
 	U_BOOT_CMD_MKENT(erase, 3, 0, do_mmc_erase, "", ""),
 #endif
 	U_BOOT_CMD_MKENT(rescan, 1, 1, do_mmc_rescan, "", ""),
@@ -858,6 +924,7 @@  U_BOOT_CMD(
 	"info - display info of the current MMC device\n"
 	"mmc read addr blk# cnt\n"
 	"mmc write addr blk# cnt\n"
+	"mmc swrite addr blk#\n"
 	"mmc erase blk# cnt\n"
 	"mmc rescan\n"
 	"mmc part - lists available partition on current mmc device\n"