[flashrom] flashrom on PowerPC (002-lh28f008bjt.patch)

[Stefan Tauner]

On Wed, 27 Jul 2011 18:20:19 +0200
Mattias Mattsson <vitplister at gmail.com> wrote:

> Index: 82802ab.c
> ===================================================================
> --- 82802ab.c	(revision 1396)
> +++ 82802ab.c	(working copy)
> @@ -208,3 +208,56 @@
>  
>  	return 0;
>  }
> +
> +int unlock_lh28f008bjt(struct flashchip *flash)
> +{
> +	chipaddr bios = flash->virtual_memory;
> +	uint8_t mcfg, bcfg, need_unlock = 0, can_unlock = 0;

mixing initialized and declared-only variables is :(

> +	int i;
> +
> +	/* Wait if chip is busy */
> +	wait_82802ab(flash);
> +
> +	/* Read identifier codes */
> +	chip_writeb(0x90, bios);
> +
> +	/* Read master lock-bit */
> +	mcfg = chip_readb(bios + 0x3);
> +	msg_cdbg("master lock is ");
> +	if (mcfg) {
> +		msg_cdbg("locked!\n");
> +	} else {
> +		msg_cdbg("unlocked!\n");
> +		can_unlock = 1;
> +	}
> +
> +	/* Read block lock-bits, 8 * 8 KiB + 15 * 64 KiB */

although correct, we don't use the IEC prefixes nowhere else...

> +	for (i = 0; i < flash->total_size * 1024; i+= (i >= (64 * 1024) ? 64 * 1024 : 8 * 1024)) {

hm... line limit. looks very odd at first sight. maybe a while loop
would be better?

> +		bcfg = chip_readb(bios + i + 2); // read block lock config

/* */?
not that i find this convention necessary...

> +		msg_cdbg("block lock at %06x is %slocked!\n", i, bcfg ? "" : "un");
> +		if (bcfg) {
> +			need_unlock = 1;
> +		}
> +	}
> +
> +	/* Reset chip */
> +	chip_writeb(0xFF, bios);
> +
> +	/* Unlock: clear block lock-bits, if needed */
> +	if (can_unlock && need_unlock) {
> +		msg_cdbg("Unlock: ");
> +		chip_writeb(0x60, bios);
> +		chip_writeb(0xD0, bios);
> +		chip_writeb(0xFF, bios);
> +		wait_82802ab(flash);
> +		msg_cdbg("Done!\n");
> +	}
> +
> +	/* Error: master locked or a block is locked */
> +	if (!can_unlock && need_unlock) {
> +		msg_cerr("At least one block is locked and lockdown is active!\n");
> +		return -1;
> +	}
> +
> +	return 0;
> +}

you start messages lower-case and end them with an exclamation mark very
often. we usually do this for errors (or warnings) only.

> Index: flashchips.c
> ===================================================================
> --- flashchips.c	(revision 1396)
> +++ flashchips.c	(working copy)
> @@ -5379,6 +5379,36 @@
>  
>  	{
>  		.vendor		= "Sharp",
> +		.name		= "LH28F008BJT-BTLZ1",

why the -BTLZ1 suffix?
i have just looked briefly at the LH28F008BJT-BTLZ1 and
LH28F008BJT-BTLZC datasheets, but they seem to be almost identical?

> +		.bustype	= BUS_PARALLEL,
> +		.manufacture_id	= SHARP_ID,
> +		.model_id	= SHARP_LH28F008BJxxPB,

i am not sure about those id names either, but they are not as
important because they are not visible to the user.

> +		.total_size	= 1024,
> +		.page_size	= 64 * 1024,
> +		.tested		= TEST_OK_PREW,
> +		.probe		= probe_82802ab,
> +		.probe_timing	= TIMING_ZERO,
> +		.block_erasers	=
> +		{
> +			{
> +				.eraseblocks = {
> +					{8 * 1024, 8},
> +					{64 * 1024, 15}
> +				 },
> +				.block_erase = erase_block_82802ab,
> +			}, {
> +				.eraseblocks = { {1024 * 1024, 1} },
> +				.block_erase = erase_sector_49lfxxxc,
> +			}
> +		},
> +		.unlock		= unlock_lh28f008bjt,
> +		.write		= write_82802ab,
> +		.read		= read_memmapped,
> +		.voltage	= {2700, 3600},
> +	},
> +
> +	{
> +		.vendor		= "Sharp",
>  		.name		= "LHF00L04",
>  		.bustype	= BUS_FWH, /* A/A Mux */
>  		.manufacture_id	= SHARP_ID,
> Index: chipdrivers.h
> ===================================================================
> --- chipdrivers.h	(revision 1396)
> +++ chipdrivers.h	(working copy)
> @@ -83,6 +83,7 @@
>  void print_status_82802ab(uint8_t status);
>  int unlock_82802ab(struct flashchip *flash);
>  int unlock_28f004s5(struct flashchip *flash);
> +int unlock_lh28f008bjt(struct flashchip *flash);
>  
>  /* jedec.c */
>  uint8_t oddparity(uint8_t val);


-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner