Re: [flashrom] [PATCH] New GigaDevice chips

[Stefan Tauner]

On Mon, 9 Mar 2015 10:18:36 +1000
Roman Titov  wrote:

Hello Roman!
> Well, I think this could be the result of my inexperience with svn
> (and git-svn).
>
> So, here's my steps:
> git svn clone -rHEAD (r1887, to be exact) flashrom-trunk (I have an
>
> error, when I'm trying to make full git svn clone, looks like git bug
> :/)
> 2. *code*
> 3. make 2 local commits
> 4. git format-patch, and it throw me 2 patches for 2 latest commits
> not over current origin (and I think thats is the reason)
> 5. send patches to mailing list

That's perfectly fine and how I work too (well, I use git send-email
for 4 and 5).
> My suggestions would be:
> shallow copying broke smth (idea, that git svn could bond git stuff
>
> to full svn history stuff came to me just right now)
> 2. formating 2 patches broke smth
> 3. both?

The problem is/was the last hunk. I guess you have opened the patch
file in an editor that removes white space at the end of lines.
The patch application does not seem to care but git-am does.

Anyway, here is a list of things I have noticed in the patch.
> From 3612049b7f6b2283ae4e593e66f369144b9e997b Mon Sep 17 00:00:00 2001
> From: Roman Titov 
> Date: Fri, 6 Mar 2015 17:18:43 +1000
> Subject: [PATCH 1/2] flashchips: new GigaDevice chips (GD25LQ40, GD25LQ80,
>  GD25LQ16, GD25LQ64(B), GD25LQ128)
>
> Signed-off-by: Roman Titov
>
> flashchips.c | 197 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 196 insertions(+), 1 deletion(-)
>
> diff --git a/flashchips.c b/flashchips.c
> index 8b5d1ec..31541ea 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -5466,6 +5466,123 @@ const struct flashchip flashchips[] = {
>
> {
>    	.vendor		= "GigaDevice",
> .name		= "GD25LQ40",
> .bustype	= BUS_SPI,
> .manufacture_id	= GIGADEVICE_ID,
> .model_id	= GIGADEVICE_GD25LQ40,
> .total_size	= 512,
> .page_size	= 256,
> /* OTP: 1024B total, 256B reserved; read 0x48; write 0x42, erase 0x44 */
> .feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP,
> .tested		= TEST_OK_PREW,

According to the line above and the respective ones later,
you had all that hardware at hand and tested it successfully.
Correct me if I am wrong, but I don't think that's the case ;)
> .probe		= probe_spi_rdid,
> .probe_timing	= TIMING_ZERO,
> .block_erasers	=
> {
> 	{
> 		.eraseblocks = { {4 * 1024, 128} },
> 		.block_erase = spi_block_erase_20,
> 	}, {
> 		.eraseblocks = { {32 * 1024, 16} },
> 		.block_erase = spi_block_erase_52,
> 	}, {
> 		.eraseblocks = { {64 * 1024, 8} },
> 		.block_erase = spi_block_erase_d8,
> 	}, {
> 		.eraseblocks = { {512 * 1024, 1} },
> 		.block_erase = spi_block_erase_60,
> 	}, {
> 		.eraseblocks = { {512 * 1024, 1} },
> 		.block_erase = spi_block_erase_c7,
> 	}
> },
> .printlock	= spi_prettyprint_status_register_bp4_srwd,
> .unlock		= spi_disable_blockprotect_bp4_srwd, /* TODO: 2nd status reg (read with 0x35) */
> .write		= spi_chip_write_256,
> .read		= spi_chip_read,

The chips support multiple read and opcodes (unlike flashrom yet!). 
We note that at least for new chips by adding a comment like this:
 /* Fast read (0x0B) and multi I/O supported */
(cf. with similar comments throughout the file).
> .voltage	= {1700, 1950},

It should actually be 1695, 1950 instead. 
This is also an error in the existing definition of the GD25LQ32.

Apart from these repeated copy & paste errors it looks good.
Please fix them so that I can commit your first flashrom
contribution, thanks!

Have you noticed any differences between the GD25LQ64 and its B
revision?

[PATCH] New GigaDevice chips

[Roma Titov]

On Thu, Mar 12, 2015 at 4:08 AM, Stefan Tauner 
 wrote:
> According to the line above and the respective ones later,
> you had all that hardware at hand and tested it successfully.
> Correct me if I am wrong, but I don't think that's the case ;)

Well, to tell the truth at first I made everything right here, but... I 
lost my work halfway. :c
So then I gone into CODERRAGE and completelly forgot about it.
FIXED now.
> It should actually be 1695, 1950 instead.
> This is also an error in the existing definition of the GD25LQ32.

Well, I noticed that, but didn't undersand this as a bug.
FIXED in GD25LQ32 and in new chips now.
> Have you noticed any differences between the GD25LQ64 and its B
> revision?

No, I didn't. At least at the parts significant to the task. I read in 
flashchips.h, that rev. B is just faster and was ok with that. 
Rechecked everything now. Still see no anyhow significant difference.

[PATCH] New GigaDevice chips

[Stefan Tauner]

Thanks Roman,

I have added the multi I/O comments myself and committed the change in
r1889.

Good luck with your risc-v GSoC project (hopefully ;)!