| Message ID | 1351590321-20368-1-git-send-email-sr@denx.de |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
Dear Stefan Roese, In message <1351590321-20368-1-git-send-email-sr@denx.de> you wrote: > By extracting these defines into a header, they can be re-used by other > C sources as well. This will be done by the SPL framework OS boot > support. > > Signed-off-by: Stefan Roese <sr@denx.de> > --- > Changes in v6: > - Fix compile warning: release.S:354:0: warning: "EPAPR_MAGIC" redefined Please re-read http://www.denx.de/wiki/view/U-Boot/Patches#Sending_updated_patch_versions You are supposed to provide a _history_ of changes here, but instead you describe only the latest change. This is not how it's suppoosed to be done. Also, your submission includes neither any "In-reply-to:" nor any "References:" header, i. e. there is no way to match it to any previous mail thread. This is very bad. How are we supposed to know what you are actually talking about, or where we would find any of the previous patches or the other 6 patches of this series? Best regards, Wolfgang Denk
Hi Wolfgang, On 10/30/2012 11:04 AM, Wolfgang Denk wrote: >> By extracting these defines into a header, they can be re-used by other >> C sources as well. This will be done by the SPL framework OS boot >> support. >> >> Signed-off-by: Stefan Roese <sr@denx.de> >> --- >> Changes in v6: >> - Fix compile warning: release.S:354:0: warning: "EPAPR_MAGIC" redefined > > Please re-read > http://www.denx.de/wiki/view/U-Boot/Patches#Sending_updated_patch_versions > > You are supposed to provide a _history_ of changes here, but instead > you describe only the latest change. This is not how it's suppoosed > to be done. As you know this patch is part of a patch-series. And this is the first time that this patch has a change. So this summary covers the complete history for this patch. > Also, your submission includes neither any "In-reply-to:" nor any > "References:" header, i. e. there is no way to match it to any > previous mail thread. Yes, I should have done this. Sorry. > This is very bad. How are we supposed to know > what you are actually talking about, or where we would find any of the > previous patches or the other 6 patches of this series? In this version of the patch series, I only made this small change to this patch 1/7. I wanted to spare the list a resending of the complete patchset for such a small change. So what is the recommended way to do this? Is it really recommended/required to repost the complete patch series upon a small change in only one patch? No problem, I can do this. patman makes it very easy. :) Should I repost the complete series again? Thanks, Stefan
Dear Stefan Roese, In message <508FA904.4070402@denx.de> you wrote: > > >> --- > >> Changes in v6: > >> - Fix compile warning: release.S:354:0: warning: "EPAPR_MAGIC" redefined ... > > As you know this patch is part of a patch-series. And this is the first > time that this patch has a change. So this summary covers the complete > history for this patch. But exactly this is information which I do not have, and which is not included in your patch. As is, I can only intepret this to be version 6 of this specific commit, and I wonder which changes were made in the previous 5 versions. > In this version of the patch series, I only made this small change to > this patch 1/7. I wanted to spare the list a resending of the complete > patchset for such a small change. > > So what is the recommended way to do this? Is it really > recommended/required to repost the complete patch series upon a small > change in only one patch? No problem, I can do this. patman makes it > very easy. :) > > Should I repost the complete series again? No, not at all! I understand you are using patman for patch management. So I added Simon on Cc: to have his oponion, too. I see two options: 1) Versioning is done on a per-patch base. In this case, this patch should have been submitted as "[PATCH v2 1/7]", in which case it would have been clear to everybody that this is the first and only change compared to previous submission(s). I don't dare to say "most", but at least some people have worked like this when submitting patch series (manually) in the past. I can understand if somebody argues that it is not exactly easy to collect the correspondign patches to a series if individual patches contain different version numbers. Correct threading of the messages is essential here. 2) Versioning is done on a per-series base. One problem here is that it becomes difficult to keep track if what is what when only single patches of the series change and get reposted - on the other hand it has always been a major PITA when people repost whole series after only changing a line of two in on of their many patches, so we strongly encourage posting of only the changed patches. Once more, proper threading appears to be essential. Another problem is what we are running into here: after severl versions of the patch series one patch that has been untouches previously gets changed. Now it gets posted as "V6", and it's impossible to know how many previous versions of this patch might have been posted before - one? 2? 3? 4? or 5? When the version ID refers to the patch series rather than to the individual patch, then I think it is mandatory to take this into consideration in the patch history, whih then must refer to all versions of the _series_. In the present case, the patch history should have looked like this: V2: no changes V3: no changes V4: no changes V5: no changes V6: Fix compile warning: release.S:354:0: warning: "EPAPR_MAGIC" redefined Is there any clear majority of preferences for patch versioning? My gut feeling is that more people would like version IDs on a per-series base, but I would like to see some confirmation for this, and the we should document such expectations. It appears that patman is oriented toward using a single version ID per series. Simon - would it be possible to automatically add such "no changes" information when generating the patches? Best regards, Wolfgang Denk
Hi Wolfgang, On 10/30/2012 12:05 PM, Wolfgang Denk wrote: >> As you know this patch is part of a patch-series. And this is the first >> time that this patch has a change. So this summary covers the complete >> history for this patch. > > But exactly this is information which I do not have, and which is not > included in your patch. As is, I can only intepret this to be version > 6 of this specific commit, and I wonder which changes were made in the > previous 5 versions. *If* we agree upon a per patch series versioning (see below), then this would be enough. To only list the changes that have been made to this patch. Your suggestion from below is even better. To document that no changes have been made: V2: no changes ... I'm pretty sure that Simon (or other people with a bit of python knowledge) can easily add this to patman. >> In this version of the patch series, I only made this small change to >> this patch 1/7. I wanted to spare the list a resending of the complete >> patchset for such a small change. >> >> So what is the recommended way to do this? Is it really >> recommended/required to repost the complete patch series upon a small >> change in only one patch? No problem, I can do this. patman makes it >> very easy. :) >> >> Should I repost the complete series again? > > No, not at all! Okay. > I understand you are using patman for patch management. So I added > Simon on Cc: to have his oponion, too. > > I see two options: > > 1) Versioning is done on a per-patch base. In this case, this patch > should have been submitted as "[PATCH v2 1/7]", in which case it > would have been clear to everybody that this is the first and only > change compared to previous submission(s). > > I don't dare to say "most", but at least some people have worked > like this when submitting patch series (manually) in the past. > > I can understand if somebody argues that it is not exactly easy to > collect the correspondign patches to a series if individual patches > contain different version numbers. Correct threading of the > messages is essential here. Yes, this is my main concern about option a). Very hard to match the single patches (and its versions) to the patch series version. Without proper threading. And I personally don't use threading in my mail client (my problem, I know). > 2) Versioning is done on a per-series base. > > One problem here is that it becomes difficult to keep track if > what is what when only single patches of the series change and get > reposted - on the other hand it has always been a major PITA when > people repost whole series after only changing a line of two in on > of their many patches, so we strongly encourage posting of only the > changed patches. Once more, proper threading appears to be > essential. > > Another problem is what we are running into here: after severl > versions of the patch series one patch that has been untouches > previously gets changed. Now it gets posted as "V6", and it's > impossible to know how many previous versions of this patch might > have been posted before - one? 2? 3? 4? or 5? > > When the version ID refers to the patch series rather than to the > individual patch, then I think it is mandatory to take this into > consideration in the patch history, whih then must refer to all > versions of the _series_. In the present case, the patch history > should have looked like this: > > V2: no changes > V3: no changes > V4: no changes > V5: no changes > V6: Fix compile warning: release.S:354:0: warning: "EPAPR_MAGIC" redefined > > > Is there any clear majority of preferences for patch versioning? > My gut feeling is that more people would like version IDs on a > per-series base, but I would like to see some confirmation for this, > and the we should document such expectations. As you have already guessed, I'm in favoring the 2nd option, versioning on a per-series base. What do other developers have to say? What's the recommended way to do this in the Linux world? Even if we don't need to do everything in the same way as done in Linux development, it is much easier to do it in a similar fashion for users working in both projects (U-Boot & Linux) regularly. > It appears that patman is oriented toward using a single version ID > per series. Simon - would it be possible to automatically add such > "no changes" information when generating the patches? A little motivation: Simon, you could earn yourself another beer the next time we meet! ;) Thanks, Stefan
Hi Wolfgang, Stefan, On Tue, Oct 30, 2012 at 6:33 AM, Stefan Roese <sr@denx.de> wrote: > Hi Wolfgang, > > On 10/30/2012 12:05 PM, Wolfgang Denk wrote: >>> As you know this patch is part of a patch-series. And this is the first >>> time that this patch has a change. So this summary covers the complete >>> history for this patch. >> >> But exactly this is information which I do not have, and which is not >> included in your patch. As is, I can only intepret this to be version >> 6 of this specific commit, and I wonder which changes were made in the >> previous 5 versions. > > *If* we agree upon a per patch series versioning (see below), then this > would be enough. To only list the changes that have been made to this > patch. Your suggestion from below is even better. To document that no > changes have been made: > > V2: no changes > ... > > I'm pretty sure that Simon (or other people with a bit of python > knowledge) can easily add this to patman. [snip] >> It appears that patman is oriented toward using a single version ID >> per series. Simon - would it be possible to automatically add such >> "no changes" information when generating the patches? > > A little motivation: Simon, you could earn yourself another beer the > next time we meet! ;) Sold :-) It's pretty trivial I think - I will take a look. Re the threading, and this is to some extent a separate issue, if I am resending a single patch, I sometimes copy in the message ID when patman (actually git send-email, called by patman) asks for it. We could perhaps automate this - in two ways: 1. Patman could automatically send only the patches that have changed for v6 (I suggest that unless this is combined with some sort of automatic patchwork state change, it could get a bit tricky for maintainers since they will be applying many different patch versions in a series). At present you have to manually type 'y' or 'n' to each patch. 2. Patman could (with a bit of effort) attach the message ID for the previous version of the patch to the 'in reply to' tag of the next version. This would mean that each patch would be in reply to its earlier version. My understanding was that this was not desirable, and that it is better to have the series stand alone. I recall some discussion on this. It may be that neither of these is desirable. > > Thanks, > Stefan Regards, Simon
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 10/30/12 06:33, Stefan Roese wrote: > Hi Wolfgang, > > On 10/30/2012 12:05 PM, Wolfgang Denk wrote: [snip] >> 2) Versioning is done on a per-series base. >> >> One problem here is that it becomes difficult to keep track if >> what is what when only single patches of the series change and >> get reposted - on the other hand it has always been a major PITA >> when people repost whole series after only changing a line of two >> in on of their many patches, so we strongly encourage posting of >> only the changed patches. Once more, proper threading appears to >> be essential. >> >> Another problem is what we are running into here: after severl >> versions of the patch series one patch that has been untouches >> previously gets changed. Now it gets posted as "V6", and it's >> impossible to know how many previous versions of this patch >> might have been posted before - one? 2? 3? 4? or 5? >> >> When the version ID refers to the patch series rather than to >> the individual patch, then I think it is mandatory to take this >> into consideration in the patch history, whih then must refer to >> all versions of the _series_. In the present case, the patch >> history should have looked like this: >> >> V2: no changes V3: no changes V4: no changes V5: no changes V6: >> Fix compile warning: release.S:354:0: warning: "EPAPR_MAGIC" >> redefined >> >> >> Is there any clear majority of preferences for patch versioning? >> My gut feeling is that more people would like version IDs on a >> per-series base, but I would like to see some confirmation for >> this, and the we should document such expectations. > > As you have already guessed, I'm in favoring the 2nd option, > versioning on a per-series base. > > What do other developers have to say? What's the recommended way to > do this in the Linux world? Even if we don't need to do everything > in the same way as done in Linux development, it is much easier to > do it in a similar fashion for users working in both projects > (U-Boot & Linux) regularly. So, I am in favor of per-series versioning. I also prefer whole reposting (which I believe to be the norm in the kernel) with a dash of common sense (posting just 1/7 makes sense here) due to how patchwork bundles work. - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iQIcBAEBAgAGBQJQkAP+AAoJENk4IS6UOR1WNMkP/i7oxy0L7W2N6h730IsDgE9g tHlgbBUsm5pWnqBC8/57mOmsWWv+j3zgojc3OYMasIyAkHpAJ2yM7qoAFxeGmDzS TyHvb2bswSIoL2pwlems2m0lfx3V7H8StRcZjpeztfbWRbQXIkKeAon0Bd5R+iwm v/mIMR6Sdfq+x0klnRIjkO++652nKRQ+JAHLNWNVxZ6DpOqqLBtTAXXyYHLpBEKz hGdrk2gzryoMAZKiKuiz5mgTmeHoBvsCkIiAsnYoZg94KXohvQUkQzVGV4WA4qOQ 1jk4v7vjGR3gg7c/gOjauwsTalv/6SGZ+f8kSUVV8zKBUOAXhckF0o2+nC70G74W hoOiSuS2hTz//xkGWmLl9mANCK9iYm/RtQIj4NlrwYabmQ0oUpHRv2HU2C47tffD u/9RrRqj4onjNR4GtYiy8M4iDFZSiVRpNF6TozxKWyXt2fG01tAPmkdy2mLqfdD5 Mbn0KKBV+/PkPrvzmX0xrFEJVhiMo4P4gUPESLADTX0Xd3oziVKpKjQTRIOpztIT ib98JeA5fMKPQJJnMnKwldU/0gek5JYpqFihwZMPf5lXi2G3beAXRPTOgZGif621 dCyWbfMEd/fI6393wFvODLjzKT09Q1uf6KxMitrziUEZBB6QVZYRQMKmz5Adz9v0 E9tj91WHgpF9zXOhQgkq =OWfZ -----END PGP SIGNATURE-----
On Tue, 30 Oct 2012 10:45:21 +0100 Stefan Roese <sr@denx.de> wrote: > By extracting these defines into a header, they can be re-used by other > C sources as well. This will be done by the SPL framework OS boot > support. > > Signed-off-by: Stefan Roese <sr@denx.de> > --- > Changes in v6: > - Fix compile warning: release.S:354:0: warning: "EPAPR_MAGIC" redefined > > arch/powerpc/cpu/mpc85xx/release.S | 1 - > arch/powerpc/include/asm/processor.h | 6 ++++++ > arch/powerpc/lib/bootm.c | 6 ------ > 3 files changed, 6 insertions(+), 7 deletions(-) Applied to staging/agust@denx.de. Thanks! Anatolij
diff --git a/arch/powerpc/cpu/mpc85xx/release.S b/arch/powerpc/cpu/mpc85xx/release.S index 4ba44a9..d2e94b8 100644 --- a/arch/powerpc/cpu/mpc85xx/release.S +++ b/arch/powerpc/cpu/mpc85xx/release.S @@ -351,7 +351,6 @@ __secondary_reset_vector: .align L1_CACHE_SHIFT .global __second_half_boot_page __second_half_boot_page: -#define EPAPR_MAGIC 0x45504150 #define ENTRY_ADDR_UPPER 0 #define ENTRY_ADDR_LOWER 4 #define ENTRY_R3_UPPER 8 diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h index 7aa3231..19fe250 100644 --- a/arch/powerpc/include/asm/processor.h +++ b/arch/powerpc/include/asm/processor.h @@ -1342,4 +1342,10 @@ void _nmask_and_or_msr(unsigned long nmask, unsigned long or_val); #endif #endif /* CONFIG_MACH_SPECIFIC */ +#if defined(CONFIG_MPC85xx) || defined(CONFIG_440) + #define EPAPR_MAGIC (0x45504150) +#else + #define EPAPR_MAGIC (0x65504150) +#endif + #endif /* __ASM_PPC_PROCESSOR_H */ diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c index 53dc4df..e3fee0b 100644 --- a/arch/powerpc/lib/bootm.c +++ b/arch/powerpc/lib/bootm.c @@ -87,12 +87,6 @@ static void boot_jump_linux(bootm_headers_t *images) * r8: 0 * r9: 0 */ -#if defined(CONFIG_MPC85xx) || defined(CONFIG_440) - #define EPAPR_MAGIC (0x45504150) -#else - #define EPAPR_MAGIC (0x65504150) -#endif - debug (" Booting using OF flat tree...\n"); WATCHDOG_RESET (); (*kernel) ((bd_t *)of_flat_tree, 0, 0, EPAPR_MAGIC,
By extracting these defines into a header, they can be re-used by other C sources as well. This will be done by the SPL framework OS boot support. Signed-off-by: Stefan Roese <sr@denx.de> --- Changes in v6: - Fix compile warning: release.S:354:0: warning: "EPAPR_MAGIC" redefined arch/powerpc/cpu/mpc85xx/release.S | 1 - arch/powerpc/include/asm/processor.h | 6 ++++++ arch/powerpc/lib/bootm.c | 6 ------ 3 files changed, 6 insertions(+), 7 deletions(-)