Patchwork [2/3] Refine handling chips that exceed maximum programmer sizes.

login
register
about
Submitter Stefan Tauner
Date 2014-08-08 14:25:08
Message ID <1407507909-28859-3-git-send-email-stefan.tauner@alumni.tuwien.ac.at>
Download mbox | patch
Permalink /patch/4228/
State Accepted
Headers show

Comments

Stefan Tauner - 2014-08-08 14:25:08
- Change check_max_decode() to return the number of (common) busses
   where the flash chip exceeds the supported size of the programmer.
 - Refine its signature to use a flashctx pointer only.
 - Move CLI-related bits to cli_classic.c.
 - Rename check_max_decode() to count_max_decode_exceedings() to
   better reflect what it (now) really does.
 - Refine the messages printed by the caller to better integrate with the new
   setup, and simplify them.

Signed-off-by: Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at>
---
 cli_classic.c | 16 ++++++++++++----
 flashrom.c    | 24 ++++++++----------------
 programmer.h  |  2 +-
 3 files changed, 21 insertions(+), 21 deletions(-)
Daniele Forsi - 2014-08-08 14:57:23
2014-08-08 16:25 GMT+02:00 Stefan Tauner:

> --- a/cli_classic.c
> +++ b/cli_classic.c
> @@ -91,7 +91,6 @@ static int check_filename(char *filename, char *type)
>
>  int main(int argc, char *argv[])
>  {
> -       unsigned long size;
>         /* Probe for up to three flash chips. */
>         const struct flashchip *chip = NULL;
>         struct flashctx flashes[6] = {{0}};

unrelated to this changeset, but the comment above should read "six
flash chips" now
Stefan Tauner - 2014-08-08 15:07:04
On Fri, 8 Aug 2014 16:57:23 +0200
Daniele Forsi <dforsi@gmail.com> wrote:

> 2014-08-08 16:25 GMT+02:00 Stefan Tauner:
> 
> > --- a/cli_classic.c
> > +++ b/cli_classic.c
> > @@ -91,7 +91,6 @@ static int check_filename(char *filename, char *type)
> >
> >  int main(int argc, char *argv[])
> >  {
> > -       unsigned long size;
> >         /* Probe for up to three flash chips. */
> >         const struct flashchip *chip = NULL;
> >         struct flashctx flashes[6] = {{0}};
> 
> unrelated to this changeset, but the comment above should read "six
> flash chips" now
> 

Yes, I am aware of that but thanks for reporting!
Let's hope the whole array is gone soon ;)
I have added the comment change to my tested_stuff branch #22 already
just in case...
Carl-Daniel Hailfinger - 2014-08-14 22:46:17
Am 08.08.2014 16:25 schrieb Stefan Tauner:
>  - Change check_max_decode() to return the number of (common) busses
>    where the flash chip exceeds the supported size of the programmer.
>  - Refine its signature to use a flashctx pointer only.
>  - Move CLI-related bits to cli_classic.c.
>  - Rename check_max_decode() to count_max_decode_exceedings() to
>    better reflect what it (now) really does.
>  - Refine the messages printed by the caller to better integrate with the new
>    setup, and simplify them.
>
> Signed-off-by: Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at>

I like it. One small comment below.
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>


> diff --git a/flashrom.c b/flashrom.c
> index eeed90b..0ff6b63 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -1079,9 +1073,6 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f
>  			continue;
>  		}
>  
> -		size = chip->total_size * 1024;
> -		check_max_decode(buses_common, size);
> -
>  		/* Start filling in the dynamic data. */
>  		flash->chip = calloc(1, sizeof(struct flashchip));
>  		if (!flash->chip) {
> @@ -1091,6 +1082,7 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f
>  		memcpy(flash->chip, chip, sizeof(struct flashchip));
>  		flash->mst = mst;
>  
> +		size = chip->total_size * 1024;

At this point, it might be better to use "flash->chip->total_size *
1024" given that flash->chip is now valid (it wasn't valid at the old
location).


>  		base = flashbase ? flashbase : (0xffffffff - size + 1);
>  		flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size);
>  

Regards,
Carl-Daniel
Stefan Tauner - 2014-08-15 17:40:30
On Fri, 15 Aug 2014 00:46:17 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:

> Am 08.08.2014 16:25 schrieb Stefan Tauner:
> >  - Change check_max_decode() to return the number of (common) busses
> >    where the flash chip exceeds the supported size of the programmer.
> >  - Refine its signature to use a flashctx pointer only.
> >  - Move CLI-related bits to cli_classic.c.
> >  - Rename check_max_decode() to count_max_decode_exceedings() to
> >    better reflect what it (now) really does.
> >  - Refine the messages printed by the caller to better integrate with the new
> >    setup, and simplify them.
> >
> > Signed-off-by: Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at>
> 
> I like it. One small comment below.
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>

Thanks.

> > diff --git a/flashrom.c b/flashrom.c
> > index eeed90b..0ff6b63 100644
> > --- a/flashrom.c
> > +++ b/flashrom.c
> > @@ -1079,9 +1073,6 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f
> >  			continue;
> >  		}
> >  
> > -		size = chip->total_size * 1024;
> > -		check_max_decode(buses_common, size);
> > -
> >  		/* Start filling in the dynamic data. */
> >  		flash->chip = calloc(1, sizeof(struct flashchip));
> >  		if (!flash->chip) {
> > @@ -1091,6 +1082,7 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f
> >  		memcpy(flash->chip, chip, sizeof(struct flashchip));
> >  		flash->mst = mst;
> >  
> > +		size = chip->total_size * 1024;
> 
> At this point, it might be better to use "flash->chip->total_size *
> 1024" given that flash->chip is now valid (it wasn't valid at the old
> location).

Mh'kay :)
Committed in r1842 after constifying the argument (and adding chip->).

Patch

diff --git a/cli_classic.c b/cli_classic.c
index 60b8b88..8034287 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -91,7 +91,6 @@  static int check_filename(char *filename, char *type)
 
 int main(int argc, char *argv[])
 {
-	unsigned long size;
 	/* Probe for up to three flash chips. */
 	const struct flashchip *chip = NULL;
 	struct flashctx flashes[6] = {{0}};
@@ -501,9 +500,18 @@  int main(int argc, char *argv[])
 
 	check_chip_supported(fill_flash->chip);
 
-	size = fill_flash->chip->total_size * 1024;
-	if (check_max_decode(fill_flash->mst->buses_supported & fill_flash->chip->bustype, size) && (!force)) {
-		msg_cerr("Chip is too big for this programmer (-V gives details). Use --force to override.\n");
+	unsigned int limitexceeded = count_max_decode_exceedings(fill_flash);
+	if (limitexceeded > 0 && !force) {
+		enum chipbustype commonbuses = fill_flash->mst->buses_supported & fill_flash->chip->bustype;
+
+		/* Sometimes chip and programmer have more than one bus in common,
+		 * and the limit is not exceeded on all buses. Tell the user. */
+		if ((bitcount(commonbuses) > limitexceeded)) {
+			msg_pdbg("There is at least one interface available which could support the size of\n"
+				 "the selected flash chip.\n");
+		}
+		msg_cerr("This flash chip is too big for this programmer (--verbose/-V gives details).\n"
+			 "Use --force/-f to override at your own risk.\n");
 		ret = 1;
 		goto out_shutdown;
 	}
diff --git a/flashrom.c b/flashrom.c
index eeed90b..0ff6b63 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1009,9 +1009,13 @@  int generate_testpattern(uint8_t *buf, uint32_t size, int variant)
 	return 0;
 }
 
-int check_max_decode(enum chipbustype buses, uint32_t size)
+/* Returns the number of busses commonly supported by the current programmer and flash chip where the latter
+ * can not be completely accessed due to size/address limits of the programmer. */
+unsigned int count_max_decode_exceedings(struct flashctx *flash)
 {
-	int limitexceeded = 0;
+	unsigned int limitexceeded = 0;
+	uint32_t size = flash->chip->total_size * 1024;
+	enum chipbustype buses = flash->mst->buses_supported & flash->chip->bustype;
 
 	if ((buses & BUS_PARALLEL) && (max_rom_decode.parallel < size)) {
 		limitexceeded++;
@@ -1045,17 +1049,7 @@  int check_max_decode(enum chipbustype buses, uint32_t size)
 			 "probe/read/erase/write may fail. ", size / 1024,
 			 max_rom_decode.spi / 1024, "SPI");
 	}
-	if (!limitexceeded)
-		return 0;
-	/* Sometimes chip and programmer have more than one bus in common,
-	 * and the limit is not exceeded on all buses. Tell the user.
-	 */
-	if (bitcount(buses) > limitexceeded)
-		/* FIXME: This message is designed towards CLI users. */
-		msg_pdbg("There is at least one common chip/programmer "
-			 "interface which can support a chip of this size. "
-			 "You can try --force at your own risk.\n");
-	return 1;
+	return limitexceeded;
 }
 
 int probe_flash(struct registered_master *mst, int startchip, struct flashctx *flash, int force)
@@ -1079,9 +1073,6 @@  int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f
 			continue;
 		}
 
-		size = chip->total_size * 1024;
-		check_max_decode(buses_common, size);
-
 		/* Start filling in the dynamic data. */
 		flash->chip = calloc(1, sizeof(struct flashchip));
 		if (!flash->chip) {
@@ -1091,6 +1082,7 @@  int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f
 		memcpy(flash->chip, chip, sizeof(struct flashchip));
 		flash->mst = mst;
 
+		size = chip->total_size * 1024;
 		base = flashbase ? flashbase : (0xffffffff - size + 1);
 		flash->virtual_memory = (chipaddr)programmer_map_flash_region("flash chip", base, size);
 
diff --git a/programmer.h b/programmer.h
index 62acfeb..7b0e77f 100644
--- a/programmer.h
+++ b/programmer.h
@@ -513,7 +513,7 @@  struct decode_sizes {
 extern struct decode_sizes max_rom_decode;
 extern int programmer_may_write;
 extern unsigned long flashbase;
-int check_max_decode(enum chipbustype buses, uint32_t size);
+unsigned int count_max_decode_exceedings(struct flashctx *flash);
 char *extract_programmer_param(const char *param_name);
 
 /* spi.c */