[PATCH] Add support for XMC XM25QH128A. Read/write tested.

[Stijn Segers]

Signed-off-by: Stijn Segers 
---
 flashchips.c         | 49 ++++++++++++++++++++++++++++++++++++++++++++
 include/flashchips.h |  1 +
 2 files changed, 50 insertions(+)

diff --git a/flashchips.c b/flashchips.c
index 47a37ee..e3fd368 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -19707,6 +19707,55 @@ const struct flashchip flashchips[] = {
    	.voltage	= {1650, 1950},
    },

+	{
+		.vendor		= "XMC",
+		.name		= "XM25QH128A",
+		.bustype	= BUS_SPI,
+		.manufacture_id	= ST_ID,
+		.model_id	= XMC_XM25QH128A,
+		.total_size	= 16384,
+		.page_size	= 256,
+		.feature_bits	= FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_QPI | FEATURE_WRSR2,
+		.tested		= TEST_OK_PREW,
+		.probe		= probe_spi_rdid,
+		.probe_timing	= TIMING_ZERO,
+		.block_erasers	=
+		{
+			{
+				.eraseblocks = { {4 * 1024, 4096} },
+				.block_erase = spi_block_erase_20,
+			}, {
+				.eraseblocks = { {32 * 1024, 512} },
+				.block_erase = spi_block_erase_52,
+			}, {
+				.eraseblocks = { {64 * 1024, 256} },
+				.block_erase = spi_block_erase_d8,
+			}, {
+				.eraseblocks = { {16 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_60,
+			}, {
+				.eraseblocks = { {16 * 1024 * 1024, 1} },
+				.block_erase = spi_block_erase_c7,
+			}
+		},
+		.printlock	= spi_prettyprint_status_register_plain,
+		.unlock		= spi_disable_blockprotect,
+		.write		= spi_chip_write_256,
+		.read		= spi_chip_read,
+		.voltage	= {2700, 3600},
+		.reg_bits	=
+		{
+			.srp    = {STATUS1, 7, RW},
+			.srl    = {STATUS2, 0, RW},
+			.bp     = {{STATUS1, 2, RW}, {STATUS1, 3, RW}, {STATUS1, 4, RW}},
+			.tb     = {STATUS1, 5, RW},
+			.sec    = {STATUS1, 6, RW},
+			.cmp    = {STATUS2, 6, RW},
+		},
+		.decode_range	= decode_range_spi25,
+	},
+
+
    {
    	.vendor		= "XMC",
    	.name		= "XM25QH128C",
diff --git a/include/flashchips.h b/include/flashchips.h
index 5df42dc..8317da7 100644
--- a/include/flashchips.h
+++ b/include/flashchips.h
@@ -818,6 +818,7 @@
 #define ST_M45PE16		0x4015
 #define XMC_XM25QH64C		0x4017
 #define XMC_XM25QU64C		0x4117
+#define XMC_XM25QH128A		0x7018
 #define XMC_XM25QH128C		0x4018
 #define XMC_XM25QU128C		0x4118
 #define XMC_XM25QH256C		0x4019

[Anastasia Klimchuk]

Hello Stijn,

I really apologise for the delay, somehow we all missed your email,
sorry for that!

We are using Gerrit for code reviews, is it possible for you to push a
patch to Gerrit?
The guidelines are here:
https://www.flashrom.org/Development_Guidelines#Patch_submission

We do very rarely accept patches via mailing list, but as a last
resort. If you could push the patch to Gerrit, that would be great!
Thank you for your contribution!

[Stijn Segers]

Hi Anastasia,

Anastasia Klimchuk  schreef op 4 november 2022 09:50:16 CET:
> Hello Stijn,
>
> I really apologise for the delay, somehow we all missed your email,
> sorry for that!
>
> We are using Gerrit for code reviews, is it possible for you to push a
> patch to Gerrit?
> The guidelines are here:
> https://www.flashrom.org/Development_Guidelines#Patch_submission
>
> We do very rarely accept patches via mailing list, but as a last
> resort. If you could push the patch to Gerrit, that would be great!
> Thank you for your contribution!

My only experience with Gerrit is pulling patches from it, not submitting. I'll look into it tonight though. Would be great if this chip could still be included in the upcoming release.

Cheers

Stijn

[Anastasia Klimchuk]

> My only experience with Gerrit is pulling patches from it, not submitting. I'll look into it tonight though.

How is it going? :)
> Would be great if this chip could still be included in the upcoming release.

I don't see why not. We would need to have a patch in gerrit, then we
(or you) can cherry-pick it on 1.3.x branch. The critical thing is to
test it, but I understand you did test it (it's in email subject), so
you need to add testing info in commit message. Ideally after
cherry-picking you would re-test it on 1.3.x and that should be it!

[Stijn Segers]

Hi,

Anastasia Klimchuk  schreef op 10 november 2022 10:24:55 CET:
> > My only experience with Gerrit is pulling patches from it, not submitting. I'll look into it tonight though.
>
> How is it going? :)

Patch is sitting in Gerrit since November 5th 😊.

Stijn
> > Would be great if this chip could still be included in the upcoming release.
>
> I don't see why not. We would need to have a patch in gerrit, then we
> (or you) can cherry-pick it on 1.3.x branch. The critical thing is to
> test it, but I understand you did test it (it's in email subject), so
> you need to add testing info in commit message. Ideally after
> cherry-picking you would re-test it on 1.3.x and that should be it!

[Felix Singer]

Hi Stijn,

On Thu, 2022-11-10 at 19:03 +0100, Stijn Segers wrote:
> Patch is sitting in Gerrit since November 5th 😊.

I just did a quick review. Meanwhile we changed some function pointers
to enums and this needs to be fixed in your patch. The commit message 
also needs some polishing. Otherwise looks pretty good.

Thanks for your contribution!

// Felix

[Stijn Segers]

Hi Felix,

Op donderdag 10 november 2022 om 18:35:39 +00:00:00 schreef Felix 
Singer :
> Hi Stijn,
>
> On Thu, 2022-11-10 at 19:03 +0100, Stijn Segers wrote:
> > Patch is sitting in Gerrit since November 5th 😊.
>
> I just did a quick review. Meanwhile we changed some function pointers
> to enums and this needs to be fixed in your patch.

I was able to cobble this patch together because another chip from the 
same family was already supported (my C is extremely basic). I ran a 
compile again on recent master and it looks like most of the 'commands' 
in the code block were uppercased (which I suppose is the enum part). 
Looks good now, no compilation errors anymore.
> The commit message also needs some polishing. Otherwise looks pretty 
> good.

Could you tell me what I'd need to change?
> Thanks for your contribution!

Thank you for your review.
> // Felix

[Anastasia Klimchuk]

Hi Stijn,

The process for code review is as follows: you send the patch for
review (you did it!) and then reviewers add comments (Felix did it).
You are expected to fix the comments (or reply back explaining why
this is not possible, or why you disagree) and then upload a new
patchset to the same patch.

Did you get an email from Gerrit with the comments on your patch?
(that should be around Nov 10 or 11 depending on timezone).
Check the patch: https://review.coreboot.org/c/flashrom/+/69309 As you
see, there are currently 7 unresolved comments.

From your previous email, it seems like you fixed compilation errors
locally, this is great! Now you need to upload a new patchset to
Gerrit, so that we all can see it, and build bot can verify it. If the
patch builds, build bot will give you +1 (currently it's -1).
You push the new patchset in the same way as previous, but pay
attention to "Change-Id: " in the commit message, it should stay
exactly the same. Yes, new commit ID, but the same Change-Id.
> > The commit message also needs some polishing. Otherwise looks pretty
> > good.
>
> Could you tell me what I'd need to change?

This is in the comments to the patch.

In general, any conversations in the patch are happening in Gerrit,
not on the mailing list. You can reply in Gerrit and all reviewers get
an email.

If you are new to Gerrit, don't worry, you will learn quickly! Your
second patch will be much easier than the first ;)

[Stijn Segers]

Hi,

Anastasia Klimchuk  schreef op 20 november 2022 23:20:43 CET:
> Hi Stijn,
>
> The process for code review is as follows: you send the patch for
> review (you did it!) and then reviewers add comments (Felix did it).
> You are expected to fix the comments (or reply back explaining why
> this is not possible, or why you disagree) and then upload a new
> patchset to the same patch.
>
> Did you get an email from Gerrit with the comments on your patch?

No, I did not 🙈. Used to review by mail as well. Just checked Gerrit, thanks for the pointer.

Stijn
> (that should be around Nov 10 or 11 depending on timezone).
> Check the patch: https://review.coreboot.org/c/flashrom/+/69309 As you
> see, there are currently 7 unresolved comments.
>
> From your previous email, it seems like you fixed compilation errors
> locally, this is great! Now you need to upload a new patchset to
> Gerrit, so that we all can see it, and build bot can verify it. If the
> patch builds, build bot will give you +1 (currently it's -1).
> You push the new patchset in the same way as previous, but pay
> attention to "Change-Id: " in the commit message, it should stay
> exactly the same. Yes, new commit ID, but the same Change-Id.
> > > The commit message also needs some polishing. Otherwise looks pretty
> > > good.
> >
> > Could you tell me what I'd need to change?
>
> This is in the comments to the patch.
>
> In general, any conversations in the patch are happening in Gerrit,
> not on the mailing list. You can reply in Gerrit and all reviewers get
> an email.
>
> If you are new to Gerrit, don't worry, you will learn quickly! Your
> second patch will be much easier than the first ;)