| Message ID | 20221012140612.3276257-1-foss@volatilesystems.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for XMC XM25QH128A. Read/write tested. | expand |
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!
Hi Anastasia, Anastasia Klimchuk <aklm@chromium.org> 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
> 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!
Hi, Anastasia Klimchuk <aklm@chromium.org> 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! >
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
Hi Felix, Op donderdag 10 november 2022 om 18:35:39 +00:00:00 schreef Felix Singer <felixsinger@posteo.net>: > 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 >
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 ;)
Hi, Anastasia Klimchuk <aklm@chromium.org> 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 ;)
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
Signed-off-by: Stijn Segers <foss@volatilesystems.org> --- flashchips.c | 49 ++++++++++++++++++++++++++++++++++++++++++++ include/flashchips.h | 1 + 2 files changed, 50 insertions(+)