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

[Stefan Tauner]

On Wed, 27 Jul 2011 18:20:19 +0200
Mattias Mattsson  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);

flashrom on PowerPC (002-lh28f008bjt.patch)

[Carl-Daniel Hailfinger]

Am 14.08.2011 04:12 schrieb Stefan Tauner:
> On Wed, 27 Jul 2011 18:20:19 +0200
> Mattias Mattsson  wrote:
> > Index: 82802ab.c
> >
> > --- 82802ab.c	(revision 1396)
> > +++ 82802ab.c	(working copy)

Stefan, thanks for the detailed review.
Mattias, could you please address Stefan's comments before committing?
Thanks.

Regards,
Carl-Daniel

flashrom on PowerPC (002-lh28f008bjt.patch)

[Uwe Hermann]

Hi,

this is committed as r1420 with some of the changes mentioned.

On Sun, Aug 14, 2011 at 04:12:02AM +0200, Stefan Tauner wrote:
> > +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 :(

Fixed.
> > /* Read block lock-bits, 8 * 8 KiB + 15 * 64 KiB */
>
> although correct, we don't use the IEC prefixes nowhere else...

Changed.
> > 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?

Wrapped the line, other changes maybe in another patch or so.
> > bcfg = chip_readb(bios + i + 2); // read block lock config
>
> /* */?
> not that i find this convention necessary...

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

Same as in some other function in that file, we should probably unify
the style in a global manner at some point. Left unchanged for now.
> > 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?

Yup, that's because the BTLZC is basically the ROHS variant of the
BTLZ1. However, there are also LH28F008BJT-TTLZ2 and LH28F008BJT-TTLZB,
which are different chips (different ID, top boot block instead of
bottom boot block), where TTLZB is the ROHS version of the TTLZ2.
> > .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.

Yeah, should probably be fixed too (different patch), I know of the
following four "LH28F008BJT" chips, see above:

LH28F008BJT-BTLZ1 / LH28F008BJT-BTLZC
LH28F008BJT-TTLZ2 / LH28F008BJT-TTLZB

SHARP_LH28F008BJT_BTLZx or similar might make sense.

Uwe.