June 2018 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: bitbang_spi: Add functions to optimize xfers
by Nico Huber (Code Review) June 27, 2018

June 27, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/26946 ) Change subject: bitbang_spi: Add functions to optimize xfers ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/#/c/26946/2/bitbang_spi.c File bitbang_spi.c: https://review.coreboot.org/#/c/26946/2/bitbang_spi.c@157 PS2, Line 157: bitbang_spi_set_sck(master, 0); > I did spend some time on the page... but I don't recall studying the timing diagram much. […] "For the first cycle, ..." that is actually redundant with the first sentence behind that bullet. And it must be true for all bits. But I agree they try to define an exception somehow for the first bit, but fail to specify it correctly. What I was nitpicking is that if you define that MOSI is always changed on a falling edge, then we don't comply because we don't have an edge for the first bit. I guess now, the first sentence behind that bullet just doesn't apply to the first bit for the available hardware implementations. So there is nothing to argue about any more ;) it depends on how you define CPHA=0 and it seems nobody defined it. CPOL=1 CPHA=1 seems overall less ambiguous too me, maybe that's why I would prefer it. -- To view, visit https://review.coreboot.org/26946 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic3430a9df34844cdfa82e109456be788eaa1789a Gerrit-Change-Number: 26946 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 26 Jun 2018 22:05:12 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: bitbang_spi: Add functions to optimize xfers
by Daniel Thompson (Code Review) June 26, 2018

June 26, 2018

Daniel Thompson has posted comments on this change. ( https://review.coreboot.org/26946 ) Change subject: bitbang_spi: Add functions to optimize xfers ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/#/c/26946/2/bitbang_spi.c File bitbang_spi.c: https://review.coreboot.org/#/c/26946/2/bitbang_spi.c@157 PS2, Line 157: bitbang_spi_set_sck(master, 0); > I didn't mean to change it to further optimize but to correct it. […] I did spend some time on the page... but I don't recall studying the timing diagram much. I spent my time trying to decode the bulletted text describing CPHA=0 and CPHA=1 (and wishing there was a real standard to look at). Regarding what CPHA=0 means, there's some additional text describing the first bit: "For the first cycle, the first bit must be on the MOSI line before the leading clock edge.". To be honest I suspect this phrasing is a little loose since "before" is not a sufficient timing constraint. I think it should be "a half period before". Anyway, the code implements the "half period before" timings so I still haven't seen where it is not correct. -- To view, visit https://review.coreboot.org/26946 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic3430a9df34844cdfa82e109456be788eaa1789a Gerrit-Change-Number: 26946 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 26 Jun 2018 19:57:49 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: flashchips: Add Macronix MX25U51245G
by Nico Huber (Code Review) June 26, 2018

June 26, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/26949 ) Change subject: flashchips: Add Macronix MX25U51245G ...................................................................... Patch Set 2: (2 comments) https://review.coreboot.org/#/c/26949/2//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/26949/2//COMMIT_MSG@14 PS2, Line 14: the later means I : have tested 4-byte addressing > Agree, and its why I was fairly fastidious in documenting exactly what I did test. […] As I said, we don't have clear rules. You documented what you did so the current commit message should be ok. And OK flags are fine too. (In the long run, we can't say that all OK flags are really true anyway, because don't test every chip after every change, don't know what chip works with what programmer etc...) https://review.coreboot.org/#/c/26949/2/flashchips.c File flashchips.c: https://review.coreboot.org/#/c/26949/2/flashchips.c@8572 PS2, Line 8572: 512B > 8kbit isn't it? Or are bit units implicit? […] b is for bits, B for bytes. But writing "bit" is probably less ambiguous. Yes, new text looks good. (Actually there is so much information in the datasheets, I don't know why people picked this to comment everywhere. Would be much better to put everything in code and don't comment, IMHO.) -- To view, visit https://review.coreboot.org/26949 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2117fc205006088967f3d97644375d10db1791f1 Gerrit-Change-Number: 26949 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Tue, 26 Jun 2018 18:35:05 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: programmer: Add Developerbox/CP2104 bit bang driver
by Daniel Thompson (Code Review) June 26, 2018

June 26, 2018

Daniel Thompson has posted comments on this change. ( https://review.coreboot.org/26948 ) Change subject: programmer: Add Developerbox/CP2104 bit bang driver ...................................................................... Patch Set 2: Idwer: Thanks for review. Will fix all these. -- To view, visit https://review.coreboot.org/26948 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2547a96c1a2259ad0d52cd4b6ef42261b37cccf3 Gerrit-Change-Number: 26948 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Comment-Date: Tue, 26 Jun 2018 18:28:38 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0

Change in flashrom[master]: flashchips: Add Macronix MX25U51245G
by Daniel Thompson (Code Review) June 26, 2018

June 26, 2018

Daniel Thompson has posted comments on this change. ( https://review.coreboot.org/26949 ) Change subject: flashchips: Add Macronix MX25U51245G ...................................................................... Patch Set 2: (3 comments) https://review.coreboot.org/#/c/26949/2//COMMIT_MSG Commit Message: https://review.coreboot.org/#/c/26949/2//COMMIT_MSG@14 PS2, Line 14: the later means I : have tested 4-byte addressing > Only if you verified the result with another programmer... […] Agree, and its why I was fairly fastidious in documenting exactly what I did test. You want me to add something like "but only enough to show the chip doesn't report errors when we try to use 4 byte addressing" to the commit message? More importantly, do you think the test OK flags I used are acceptable with this level of testing? https://review.coreboot.org/#/c/26949/2/flashchips.c File flashchips.c: https://review.coreboot.org/#/c/26949/2/flashchips.c@8572 PS2, Line 8572: 512B > datasheet says 8kb (4kb factory, 4kb customer) 8kbit isn't it? Or are bit units implicit? MX25U12835F has 4kbit OTP and is commented as 512B. Is that a mistake or does the B imply bytes? Changing to this is OK? /* OTP: 512B factory programmed and 512B customer programmed; enter 0xB1, exit 0xC1 */ Wow... this comment has a lot of question marks in it! https://review.coreboot.org/#/c/26949/2/flashchips.c@8579 PS2, Line 8579: { : .eraseblocks = { {4 * 1024, 16384} }, : .block_erase = spi_block_erase_20, : }, { : .eraseblocks = { {32 * 1024, 2048} }, : .block_erase = spi_block_erase_52, : }, { : .eraseblocks = { {64 * 1024, 1024} }, : .block_erase = spi_block_erase_d8, : } > Please list the native 4BA versions too: 21 5c dc Will fix. -- To view, visit https://review.coreboot.org/26949 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2117fc205006088967f3d97644375d10db1791f1 Gerrit-Change-Number: 26949 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Nico Huber <nico.h(a)gmx.de> Gerrit-Comment-Date: Tue, 26 Jun 2018 18:25:12 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: bitbang_spi: Add half-duplex optimizations
by Nico Huber (Code Review) June 26, 2018

June 26, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/26947 ) Change subject: bitbang_spi: Add half-duplex optimizations ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/#/c/26947/2/bitbang_spi.c File bitbang_spi.c: https://review.coreboot.org/#/c/26947/2/bitbang_spi.c@128 PS2, Line 128: if (i == 0) : bitbang_spi_set_sck_set_mosi(master, 0, 0); : else : bitbang_spi_set_sck(master, 0); > SPI is an ad-hoc standard; very little is defined anywhere! […] I wasn't paying attention obviously. It's not about SPI but whatever the flash chip expects after the command. I can see from random data- sheets that they don't care. But that's no proof that there is no chip that does care so let's keep it. -- To view, visit https://review.coreboot.org/26947 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I33b9f363716f651146c09113bda5fffe53b16738 Gerrit-Change-Number: 26947 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 26 Jun 2018 18:19:13 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: bitbang_spi: Add functions to optimize xfers
by Nico Huber (Code Review) June 26, 2018

June 26, 2018

Nico Huber has posted comments on this change. ( https://review.coreboot.org/26946 ) Change subject: bitbang_spi: Add functions to optimize xfers ...................................................................... Patch Set 2: Code-Review+2 (3 comments) You are right, you didn't change the clocking. It just looks confusing now. Btw. changing from CPOL=0 CPHA=0 to CPOL=1 CPHA=1 is no big deal, the only difference is the idle state of the clock. https://review.coreboot.org/#/c/26946/2/bitbang_spi.c File bitbang_spi.c: https://review.coreboot.org/#/c/26946/2/bitbang_spi.c@47 PS2, Line 47: static void bitbang_spi_set_sck_set_mosi(const struct bitbang_spi_master * const master, int sck, int mosi) Now that I looked again, the `sck` parameter is not needed, its always called with the same value. Could be renamed to `bitbang_ spi_clear_sck_set_mosi()` if you want to get rid of it. https://review.coreboot.org/#/c/26946/2/bitbang_spi.c@114 PS2, Line 114: bitbang_spi_set_sck_set_mosi(master, 0, 0); > I don't really understand what you mean here. […] You are right. All I saw was that we start at 0 and the first thing we do in the loop is set to 0 again, that seemed wrong. And I see now why, the code was wrong all the time. Your change just makes it more obvious. https://review.coreboot.org/#/c/26946/2/bitbang_spi.c@157 PS2, Line 157: bitbang_spi_set_sck(master, 0); > You mean change from a CPOL=0, CPHA=0 controller into a CPOL=1, CPHA=1 controller (using terminology […] I didn't mean to change it to further optimize but to correct it. I hope you didn't stare at the diagram on Wikipedia as long as I did, it is wrong as well (or at least doesn't reflect the descrip- tion). Let's see what CPOL=0 CPHA=0 really means. Leading edge is 0 to 1, trailing edge is 1 to 0. Now it says "For CPHA=0, the "out" side changes the data on the trailing edge of the preceding clock cycle". But with this code, there is no preceding clock cycle. I'm not sure if that is valid. But it doesn't matter as all slave implementations just sample when they see the rising edge. -- To view, visit https://review.coreboot.org/26946 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic3430a9df34844cdfa82e109456be788eaa1789a Gerrit-Change-Number: 26946 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 26 Jun 2018 18:06:43 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes

1 0

Change in flashrom[master]: bitbang_spi: Add half-duplex optimizations
by Daniel Thompson (Code Review) June 26, 2018

June 26, 2018

Daniel Thompson has posted comments on this change. ( https://review.coreboot.org/26947 ) Change subject: bitbang_spi: Add half-duplex optimizations ...................................................................... Patch Set 2: (2 comments) https://review.coreboot.org/#/c/26947/2/bitbang_spi.c File bitbang_spi.c: https://review.coreboot.org/#/c/26947/2/bitbang_spi.c@128 PS2, Line 128: if (i == 0) : bitbang_spi_set_sck_set_mosi(master, 0, 0); : else : bitbang_spi_set_sck(master, 0); > I wouldn't expect the state of MOSI to matter. Though, I never read […] SPI is an ad-hoc standard; very little is defined anywhere! I assume that the half-duplex protocol used for FLASH is described in a JEDEC document somewhere. Unfortunately so far I haven't found it... so when I developed these patches I worked pretty hard to preserve the existing behaviour as much as possible. In other words the original code parked MOSI at 0 so this patch does the same thing. https://review.coreboot.org/#/c/26947/2/bitbang_spi.c@141 PS2, Line 141: uint8_t val) > Nit, we have a 112 chars line limit, so no line break needed here. Will fix. -- To view, visit https://review.coreboot.org/26947 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I33b9f363716f651146c09113bda5fffe53b16738 Gerrit-Change-Number: 26947 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 26 Jun 2018 18:02:38 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: bitbang_spi: Add functions to optimize xfers
by Daniel Thompson (Code Review) June 26, 2018

June 26, 2018

Daniel Thompson has posted comments on this change. ( https://review.coreboot.org/26946 ) Change subject: bitbang_spi: Add functions to optimize xfers ...................................................................... Patch Set 2: (2 comments) Nico: Not sure I understood your review comments but I've replied anyway. Right now I don't plan to act (based on my estimation of the benefit) but if you really do want me to investigate what happens if we change the idle state of the clock then let me know. https://review.coreboot.org/#/c/26946/2/bitbang_spi.c File bitbang_spi.c: https://review.coreboot.org/#/c/26946/2/bitbang_spi.c@114 PS2, Line 114: bitbang_spi_set_sck_set_mosi(master, 0, 0); > Shouldn't we start with CLK = 1 now? I don't really understand what you mean here. I've not really changed how the clocking works with this patch. Other than the combining the clk/mosi/miso changes the only other change is to move bitbang_spi_set_clk() from the end of the byte loop to the beginning so it could be combined with the mosi (and adding in the final set_clk to ensure we idle the clock at the right polarity). https://review.coreboot.org/#/c/26946/2/bitbang_spi.c@157 PS2, Line 157: bitbang_spi_set_sck(master, 0); > Wouldn't be needed if we assume CLK = 1 between commands. You mean change from a CPOL=0, CPHA=0 controller into a CPOL=1, CPHA=1 controller (using terminology from https://en.wikipedia.org/wiki/Serial_Peripheral_Interface#Clock_polarity_an… )? I don't think that belongs in *this* patch (since its a much bigger change). I guess I could look into it but my gut feeling is that the gains would be extremely marginal. The effect of my combined patches was to reduce the packet count to the CP210x from 32 packets per byte to 24 packets per *byte*. Changing the idling polarity would save no more than 2 packets per *command*. Even for a tiny two byte transfer the gains will be pretty small and I believe most of the runtime is spent doing *much* bigger transfers where 2 packets would probably be lost in the noise. -- To view, visit https://review.coreboot.org/26946 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic3430a9df34844cdfa82e109456be788eaa1789a Gerrit-Change-Number: 26946 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Daniel Thompson <daniel.thompson(a)linaro.org> Gerrit-Reviewer: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 26 Jun 2018 16:34:28 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No

1 0

Change in flashrom[master]: digilent_spi: add a driver for the iCEblink40 development board
by build bot (Jenkins) (Code Review) June 26, 2018

June 26, 2018

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/23338 ) Change subject: digilent_spi: add a driver for the iCEblink40 development board ...................................................................... Patch Set 8: Build Successful https://qa.coreboot.org/job/flashrom-customrules/1354/ : SUCCESS -- To view, visit https://review.coreboot.org/23338 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ffcd9a2db4395816f0e8b6ce6c3b0d8e930c9e6 Gerrit-Change-Number: 23338 Gerrit-PatchSet: 8 Gerrit-Owner: Lubomir Rintel <lkundrak(a)v3.sk> Gerrit-Reviewer: Lubomir Rintel <lkundrak(a)v3.sk> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 26 Jun 2018 10:02:57 +0000 Gerrit-HasComments: No Gerrit-HasLabels: No

1 0