| Message ID | 1522996524-23376-1-git-send-email-jassisinghbrar@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable mmc to write sparse images | expand |
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
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.
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
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
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
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.
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"