| Message ID | 1450107934-7362-1-git-send-email-abrodkin@synopsys.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Tom Rini |
| Headers | show |
On Monday, December 14, 2015 at 04:45:34 PM, Alexey Brodkin wrote: > This fixes commit 1a37889b0ad084a740b4f785031d7ae9955d947b: > ----------------------->8-------------------- > eeprom: Pull out the RW loop > > Unify the code for doing read/write into single function, since the > code for both the read and write is almost identical. This again > trims down the code duplication. > ----------------------->8-------------------- > > where the same one routine is utilized for both EEPROM writing and > reading. The only difference was supposed to be a "read" flag which > in both cases was set with 1 somehow. > > That lead to a missing delay in case of writing which lead to write > failure (in my case no data was written). > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> > Cc: Marek Vasut <marex@denx.de> > Cc: Simon Glass <sjg@chromium.org> > Cc: Tom Rini <trini@konsulko.com> > Cc: Heiko Schocher <hs@denx.de> Obviously correct, Acked-by: Marek Vasut <marex@denx.de> Thanks for spotting this, nice! Best regards, Marek Vasut
Hi Marek, On Tue, 2015-12-15 at 01:27 +0100, Marek Vasut wrote: > On Monday, December 14, 2015 at 04:45:34 PM, Alexey Brodkin wrote: > > This fixes commit 1a37889b0ad084a740b4f785031d7ae9955d947b: > > ----------------------->8-------------------- > > eeprom: Pull out the RW loop > > > > Unify the code for doing read/write into single function, since the > > code for both the read and write is almost identical. This again > > trims down the code duplication. > > ----------------------->8-------------------- > > > > where the same one routine is utilized for both EEPROM writing and > > reading. The only difference was supposed to be a "read" flag which > > in both cases was set with 1 somehow. > > > > That lead to a missing delay in case of writing which lead to write > > failure (in my case no data was written). > > > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> > > Cc: Marek Vasut <marex@denx.de> > > Cc: Simon Glass <sjg@chromium.org> > > Cc: Tom Rini <trini@konsulko.com> > > Cc: Heiko Schocher <hs@denx.de> > > Obviously correct, > > Acked-by: Marek Vasut <marex@denx.de> > > Thanks for spotting this, nice! That was a nice exercise for me. From the first glance DW SPI and ARC-specific changes were not guilty so I tried some previous RC-s and found that v2016.01-rc1 is good while rc2 is not. So I recalled articles and talks about git bisect. And literally in few next minutes I knew commit that introduced that breakage. At that point problem became really obvious. That said it's really fantastic what cool tools we have now that simplify our life as developers significantly. -Alexey
On Tuesday, December 15, 2015 at 09:07:01 AM, Alexey Brodkin wrote: > Hi Marek, > > On Tue, 2015-12-15 at 01:27 +0100, Marek Vasut wrote: > > On Monday, December 14, 2015 at 04:45:34 PM, Alexey Brodkin wrote: > > > This fixes commit 1a37889b0ad084a740b4f785031d7ae9955d947b: > > > ----------------------->8-------------------- > > > eeprom: Pull out the RW loop > > > > > > Unify the code for doing read/write into single function, since the > > > code for both the read and write is almost identical. This again > > > trims down the code duplication. > > > ----------------------->8-------------------- > > > > > > where the same one routine is utilized for both EEPROM writing and > > > reading. The only difference was supposed to be a "read" flag which > > > in both cases was set with 1 somehow. > > > > > > That lead to a missing delay in case of writing which lead to write > > > failure (in my case no data was written). > > > > > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> > > > Cc: Marek Vasut <marex@denx.de> > > > Cc: Simon Glass <sjg@chromium.org> > > > Cc: Tom Rini <trini@konsulko.com> > > > Cc: Heiko Schocher <hs@denx.de> > > > > Obviously correct, > > > > Acked-by: Marek Vasut <marex@denx.de> > > > > Thanks for spotting this, nice! > > That was a nice exercise for me. > From the first glance DW SPI and ARC-specific changes > were not guilty so I tried some previous RC-s and found that > v2016.01-rc1 is good while rc2 is not. > > So I recalled articles and talks about git bisect. > And literally in few next minutes I knew commit that introduced > that breakage. At that point problem became really obvious. ... and then you cursed at me, yeah, I did not sleep very well last night ;-) > That said it's really fantastic what cool tools we have now that > simplify our life as developers significantly. :) Best regards, Marek Vasut
Hi Marek, On Tue, 2015-12-15 at 12:47 +0100, Marek Vasut wrote: > On Tuesday, December 15, 2015 at 09:07:01 AM, Alexey Brodkin wrote: > > Hi Marek, > > > > On Tue, 2015-12-15 at 01:27 +0100, Marek Vasut wrote: > > > On Monday, December 14, 2015 at 04:45:34 PM, Alexey Brodkin wrote: > > > > This fixes commit 1a37889b0ad084a740b4f785031d7ae9955d947b: > > > > ----------------------->8-------------------- > > > > eeprom: Pull out the RW loop > > > > > > > > Unify the code for doing read/write into single function, since the > > > > code for both the read and write is almost identical. This again > > > > trims down the code duplication. > > > > ----------------------->8-------------------- > > > > > > > > where the same one routine is utilized for both EEPROM writing and > > > > reading. The only difference was supposed to be a "read" flag which > > > > in both cases was set with 1 somehow. > > > > > > > > That lead to a missing delay in case of writing which lead to write > > > > failure (in my case no data was written). > > > > > > > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> > > > > Cc: Marek Vasut <marex@denx.de> > > > > Cc: Simon Glass <sjg@chromium.org> > > > > Cc: Tom Rini <trini@konsulko.com> > > > > Cc: Heiko Schocher <hs@denx.de> > > > > > > Obviously correct, > > > > > > Acked-by: Marek Vasut <marex@denx.de> > > > > > > Thanks for spotting this, nice! > > > > That was a nice exercise for me. > > From the first glance DW SPI and ARC-specific changes > > were not guilty so I tried some previous RC-s and found that > > v2016.01-rc1 is good while rc2 is not. > > > > So I recalled articles and talks about git bisect. > > And literally in few next minutes I knew commit that introduced > > that breakage. At that point problem became really obvious. > > ... and then you cursed at me, yeah, I did not sleep very well last night ;-) Well 1 beer may definitely help to improve our relationships indeed :) But jokes aside I'm a bit confused with development cycle of U-Boot. I mean here we're free to push quite important changes at any point even if tomorrow Tom is cutting the next release. RCs are out of the question at all. I have to confess I do it myself from time to time just because it's so easy and nobody cares. But that really makes me worry because I cannot afford running U-Boot on every board I support on each and every commit. Which in turn means there's a possibility in final release some of my boards will be broken to some extent. That said I really like Linux development procedure when merge window is open just for a week or so and then only regression fixes are accepted during RC cycles. Still that very-very formal approach could be a bit of overkill for us here. But there's another good example - Buildroot. In the same way as we do it they accept patches right in master branch but only until the first RC. From RC1 on only fixes go in master, everything else goes to "next" branch. And "next" gets merged in master right after release. Again I'm not following U-boot mailing list closely so I might be missing similar ongoing discussion so please be lenient to that my rant :) -Alexey
On Tue, Dec 15, 2015 at 12:09:21PM +0000, Alexey Brodkin wrote: [snip] > But jokes aside I'm a bit confused with development cycle of U-Boot. > I mean here we're free to push quite important changes at any point > even if tomorrow Tom is cutting the next release. RCs are out of the > question at all. Well, the rule of thumb is to get changes out by "around" rc1. I did -rc1 on the 16th, Marek posted this series on the 10th and I merged it on the 21st. So all things considered, I know I've done worse merges. But the topic also does come up more often than it should, so something in the balance isn't right. > I have to confess I do it myself from time to time just because it's > so easy and nobody cares. But that really makes me worry because I cannot > afford running U-Boot on every board I support on each and every commit. > Which in turn means there's a possibility in final release some of my boards > will be broken to some extent. Well, that's just it. I put a good level of discretion to the various area custodians to take what they're comfortable with, given what they can test and how close to the release things are. > That said I really like Linux development procedure when merge window is > open just for a week or so and then only regression fixes are accepted > during RC cycles. > > Still that very-very formal approach could be a bit of overkill for us here. > But there's another good example - Buildroot. > > In the same way as we do it they accept patches right in master branch > but only until the first RC. From RC1 on only fixes go in master, > everything else goes to "next" branch. And "next" gets merged in master > right after release. > > Again I'm not following U-boot mailing list closely so I might be missing > similar ongoing discussion so please be lenient to that my rant :) Well, what I don't like about 'next' is people are either going to be testing 'next' (And not what's going to be released soon) or they're testing master (and we're just delaying problems being seen and mildly more annoying to bisect back to). At ELC-E we had talked about moving to a nominal 2 month rather than 3 month (meaning it's released when it's ready, not a hard calendar deadline) release schedule. That would remove some of the pressure of 'take $X so it's not seemed to be forgotten between releases'.
On Mon, Dec 14, 2015 at 06:45:34PM +0300, Alexey Brodkin wrote:
> This fixes commit 1a37889b0ad084a740b4f785031d7ae9955d947b:
Applied to u-boot/master, thanks!
diff --git a/common/cmd_eeprom.c b/common/cmd_eeprom.c index 6eab1ea..571240a 100644 --- a/common/cmd_eeprom.c +++ b/common/cmd_eeprom.c @@ -197,7 +197,7 @@ int eeprom_write(unsigned dev_addr, unsigned offset, * We must write the address again when changing pages * because the address counter only increments within a page. */ - ret = eeprom_rw(dev_addr, offset, buffer, cnt, 1); + ret = eeprom_rw(dev_addr, offset, buffer, cnt, 0); eeprom_write_enable(dev_addr, 0); return ret;
This fixes commit 1a37889b0ad084a740b4f785031d7ae9955d947b: ----------------------->8-------------------- eeprom: Pull out the RW loop Unify the code for doing read/write into single function, since the code for both the read and write is almost identical. This again trims down the code duplication. ----------------------->8-------------------- where the same one routine is utilized for both EEPROM writing and reading. The only difference was supposed to be a "read" flag which in both cases was set with 1 somehow. That lead to a missing delay in case of writing which lead to write failure (in my case no data was written). Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com> Cc: Marek Vasut <marex@denx.de> Cc: Simon Glass <sjg@chromium.org> Cc: Tom Rini <trini@konsulko.com> Cc: Heiko Schocher <hs@denx.de> --- common/cmd_eeprom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)