Re: [U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board
I've published some related cleanup patches and push those patches into u-boot-85xx.git 'dev' branch. You should be able to: * drop board_lmb_reserve() * remove config.mk and CONFIG_SYS_LDSCRIPT in config.h * remove fsl_ddr_get_mem_data_rate(), fsl_ddr_get_spd() [need to rename SPD_EEPROM_ADDRESS1 to SPD_EEPROM_ADDRESS] * just set P1021 related defines rather than touch immap_qe.h - k ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board
On Mon, 31 Jan 2011 23:40:41 +0100 Wolfgang Denk wrote: > Dear Scott Wood, > > In message <20110131160713.0b78c...@udp111988uds.am.freescale.net> you wrote: > > > > Do you want the README changes to be a separate patch from the > > board/makefile changes? > > Did you not just explain that this would make no sense? I don't think so, though it makes sense to me that it should go with the makefile changes. But I'm trying to figure out precisely what you want done with this patchset, rather than what makes sense to me. I'll take that as a "squash it in with the board and makefile changes". -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board
Dear Scott Wood, In message <20110131160713.0b78c...@udp111988uds.am.freescale.net> you wrote: > > Do you want the README changes to be a separate patch from the > board/makefile changes? Did you not just explain that this would make no sense? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de The Wright Bothers weren't the first to fly. They were just the first not to crash. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board
On Mon, 31 Jan 2011 22:34:34 +0100
Wolfgang Denk wrote:
> Dear Scott Wood,
>
> In message <20110131151506.700dd...@udp111988uds.am.freescale.net> you wrote:
> >
> > > For example, why must we add the Makefile changes in the first step,
> > > when all the code it references is still missing? Should this not be
> > > the last step?
> >
> > If you make it the last step, then the board will exist but not be
> > buildable in the previous step (unless you combine them, but you said
> > that's not what you're asking for). How is that better? And is this
> > really worth bickering about?
>
> Yes, this is better, and this is how we always do it: add the featurs,
> but not enable them unless we have all together, then add the needed
> #defines and make rules to actually use the code.
Those two "this"es don't match.
The latter is what we did do. We added TPL, but it wasn't enabled
until a board actually turns on CONFIG_HAS_TPL.
The former, what I was asking above if it was what you meant, would be
to have the board be added, enabling CONFIG_HAS_TPL because that's the
only way this board can be built, with a commit that is broken until
the subsequent commit adding TPL to the toplevel makefile is added. Or
to have the toplevel makefile changes squashed into the board patch.
It's not as if this is a make rule pointing at a specific file (with no
$(BOARDDIR)) that is absent.
> > Because it's not specific to 85xx or p1021mds. The generic
> > infrastructure for TPL consists of the makefile changes and
> > documentation. It seems useful to me to separate that for review, but
>
> A dead / broken make rule and dead documentation is what the generic
> infrastructure for TPL consists of?
What is broken about it?
Yes, the makefile change and documentation are what the generic
infrastructure for TPL consists of. Yes, it's inactive until a board
enables the feature ("when we have all together"), at which point the
board is required to provide tpl/board/$(BOARDDIR)/Makefile. Code
which is not board-specific is pulled from nand_spl and main U-Boot via
this board-specific makefile.
BTW, CONFIG_HAS_TPL is actually used in the toplevel makefile changes.
> > if you want it squashed into a board-specific patch instead, fine.
> > Just tell us what you want to see.
>
> I already did, but here we go:
No, you made some vague statements of general principle, of which your
interpretation apparently differs from mine. I was hoping for
specifics about this patch set.
> First, please do not add make rules before you have code they apply
> to.
So squash the makefile changes into the board patch?
Which seems to be how nand_spl got added a while back (patch title
"Add support for AMCC Sequoia PPC440EPx eval board"). Maybe the
makefile construct you recently objected to (possibly-empty variable
rule target) would have been more visible if it had been separated out. :-)
What about the division between the mpc85xx portion and the p1021mds
portion?
> After doing this, there is this rudimentary patch to the README.
> From a strictly technical point of view it should be split nto two
> parts: the first one (documenting the existing NAND_SPL variables) is
> independent of the TPL stuff and could be handles separately. The
> second part should be mergeed into the patch that first uses these
> variables. Note that I do not insist on splitting the README changes.
> It's OK with me to keep this together.
Yes, the NAND_SPL bits were lumped in there for convenience, and to
demonstrate the correspondence.
Do you want the README changes to be a separate patch from the
board/makefile changes?
-Scott
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board
Dear Scott Wood, In message <20110131151506.700dd...@udp111988uds.am.freescale.net> you wrote: > > > For example, why must we add the Makefile changes in the first step, > > when all the code it references is still missing? Should this not be > > the last step? > > If you make it the last step, then the board will exist but not be > buildable in the previous step (unless you combine them, but you said > that's not what you're asking for). How is that better? And is this > really worth bickering about? Yes, this is better, and this is how we always do it: add the featurs, but not enable them unless we have all together, then add the needed #defines and make rules to actually use the code. > Please just say, clearly and specifically, what you want the patchset > to look like... > > > And what is the benefit of adding documentation to the README here? > > To me it makes more sense to add this when CONFIG_HAS_TPL and > > CONFIG_IN_TPL get used first. > > Because it's not specific to 85xx or p1021mds. The generic > infrastructure for TPL consists of the makefile changes and > documentation. It seems useful to me to separate that for review, but A dead / broken make rule and dead documentation is what the generic infrastructure for TPL consists of? > if you want it squashed into a board-specific patch instead, fine. > Just tell us what you want to see. I already did, but here we go: First, please do not add make rules before you have code they apply to. After doing this, there is this rudimentary patch to the README. >From a strictly technical point of view it should be split nto two parts: the first one (documenting the existing NAND_SPL variables) is independent of the TPL stuff and could be handles separately. The second part should be mergeed into the patch that first uses these variables. Note that I do not insist on splitting the README changes. It's OK with me to keep this together. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de It is a good thing for an uneducated man to read books of quotations. - Sir Winston Churchill _My Early Life_ ch. 9 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board
On Mon, 31 Jan 2011 21:50:57 +0100 Wolfgang Denk wrote: > Dear Scott Wood, > > In message <20110131143141.2959d...@udp111988uds.am.freescale.net> you wrote: > > > > I'm confused. You say "of course not all together", but the first one > > you say to include with the second, and the second you say to include > > with the third. > > I did not say this. "WHy not add the tpl target when you actually add the tpl code?" "Then this is where that file belongs to." > > Has your aversion to "dead" code grown so strong it can't exist even in > > a transitory state between members of a patchset, even when necessary > > to avoid mixing users of a facility with the facility itself in the > > same patch? I think that would do significant harm to reviewability. > > Calm down, and re-read what I wrote. I am calm, albeit confused and a bit frustrated. I did re-read it and I'm still not sure exactly what you want. > For example, why must we add the Makefile changes in the first step, > when all the code it references is still missing? Should this not be > the last step? If you make it the last step, then the board will exist but not be buildable in the previous step (unless you combine them, but you said that's not what you're asking for). How is that better? And is this really worth bickering about? Please just say, clearly and specifically, what you want the patchset to look like... > And what is the benefit of adding documentation to the README here? > To me it makes more sense to add this when CONFIG_HAS_TPL and > CONFIG_IN_TPL get used first. Because it's not specific to 85xx or p1021mds. The generic infrastructure for TPL consists of the makefile changes and documentation. It seems useful to me to separate that for review, but if you want it squashed into a board-specific patch instead, fine. Just tell us what you want to see. -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board
Dear Kumar Gala, In message you wrote: > ... > I'm in agreement with Scott on this. I believe we've taken this a bit > too far about "dead code". It should be reasonable in a patch series to > have code that will be used in a subsequent patch. Yes, but you should not enable it or add it to Makefiles before it's even there. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de You go slow, be gentle. It's no one-way street -- you know how you feel and that's all. It's how the girl feels too. Don't press. If the girl feels anything for you at all, you'll know. -- Kirk, "Charlie X", stardate 1535.8 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board
Dear Scott Wood, In message <20110131143141.2959d...@udp111988uds.am.freescale.net> you wrote: > > I'm confused. You say "of course not all together", but the first one > you say to include with the second, and the second you say to include > with the third. I did not say this. > If you're suggesting keeping them mostly separate, but just moving some > bits into the subsequent patch, that makes no sense to me. They > logically belong where they are -- e.g. Come on. Read what I wrote. > Has your aversion to "dead" code grown so strong it can't exist even in > a transitory state between members of a patchset, even when necessary > to avoid mixing users of a facility with the facility itself in the > same patch? I think that would do significant harm to reviewability. Calm down, and re-read what I wrote. For example, why must we add the Makefile changes in the first step, when all the code it references is still missing? Should this not be the last step? And what is the benefit of adding documentation to the README here? To me it makes more sense to add this when CONFIG_HAS_TPL and CONFIG_IN_TPL get used first. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Plans break down. You cannot plan the future. Only presumptuous fools plan. The wise man _steers_.- Terry Pratchett, _Making_Money_ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board
On Jan 31, 2011, at 2:31 PM, Scott Wood wrote: > On Mon, 31 Jan 2011 21:22:04 +0100 > Wolfgang Denk wrote: > >> Dear Scott Wood, >> >> In message <20110131141332.5a4a2...@udp111988uds.am.freescale.net> you wrote: >>> I think these patches are incorrectly split. >>> >>> I think the intent was to split the arch-neutral stuff from the 85xx >>> stuff from the board stuff -- you'd rather they be all bunched together? >> >> No, of course not all together. >> This patch adds stuff to the Makefile, which would result in errors if used, as the referenced directories don't exist yet. >>> >>> Lots of patches add features, disabled by default, that require CPU or >>> board code to provide things, that would cause errors if the feature >>> were enabled on a board otherwise. >> >> But here nothing is disabled. It's added to the top level Makefile. >> It's dead code if unused, and causes errors if used. WHy not add the >> tpl target when you actually add the tpl code? >> >>> I don't think it's even possible to add an empty directory with git. >> >> True. Butt that would not fix anythign, it would still not work. >> [PATCH 2/6] powerpc/85xx: add TPL support This patch creates unused files, like arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds >>> >>> It gets used in later in the patchset, when a board with tpl is added. >> >> Then this is where that file belongs to. > > I'm confused. You say "of course not all together", but the first one > you say to include with the second, and the second you say to include > with the third. > > If you're suggesting keeping them mostly separate, but just moving some > bits into the subsequent patch, that makes no sense to me. They > logically belong where they are -- e.g. > arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds is part of 85xx TPL support, it > is not p1021mds-specific. And every bit of the first two patches is > technically dead until a board is added that uses it. > > Has your aversion to "dead" code grown so strong it can't exist even in > a transitory state between members of a patchset, even when necessary > to avoid mixing users of a facility with the facility itself in the > same patch? I think that would do significant harm to reviewability. > > -Scott I'm in agreement with Scott on this. I believe we've taken this a bit too far about "dead code". It should be reasonable in a patch series to have code that will be used in a subsequent patch. - k ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board
On Mon, 31 Jan 2011 21:22:04 +0100 Wolfgang Denk wrote: > Dear Scott Wood, > > In message <20110131141332.5a4a2...@udp111988uds.am.freescale.net> you wrote: > > > > > I think these patches are incorrectly split. > > > > I think the intent was to split the arch-neutral stuff from the 85xx > > stuff from the board stuff -- you'd rather they be all bunched together? > > No, of course not all together. > > > > This patch adds stuff to the Makefile, which would result in > > > errors if used, as the referenced directories don't exist yet. > > > > Lots of patches add features, disabled by default, that require CPU or > > board code to provide things, that would cause errors if the feature > > were enabled on a board otherwise. > > But here nothing is disabled. It's added to the top level Makefile. > It's dead code if unused, and causes errors if used. WHy not add the > tpl target when you actually add the tpl code? > > > I don't think it's even possible to add an empty directory with git. > > True. Butt that would not fix anythign, it would still not work. > > > > [PATCH 2/6] powerpc/85xx: add TPL support > > > > > > This patch creates unused files, like > > > arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds > > > > It gets used in later in the patchset, when a board with tpl is added. > > Then this is where that file belongs to. I'm confused. You say "of course not all together", but the first one you say to include with the second, and the second you say to include with the third. If you're suggesting keeping them mostly separate, but just moving some bits into the subsequent patch, that makes no sense to me. They logically belong where they are -- e.g. arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds is part of 85xx TPL support, it is not p1021mds-specific. And every bit of the first two patches is technically dead until a board is added that uses it. Has your aversion to "dead" code grown so strong it can't exist even in a transitory state between members of a patchset, even when necessary to avoid mixing users of a facility with the facility itself in the same patch? I think that would do significant harm to reviewability. -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board
Dear Scott Wood, In message <20110131141332.5a4a2...@udp111988uds.am.freescale.net> you wrote: > > > I think these patches are incorrectly split. > > I think the intent was to split the arch-neutral stuff from the 85xx > stuff from the board stuff -- you'd rather they be all bunched together? No, of course not all together. > > This patch adds stuff to the Makefile, which would result in > > errors if used, as the referenced directories don't exist yet. > > Lots of patches add features, disabled by default, that require CPU or > board code to provide things, that would cause errors if the feature > were enabled on a board otherwise. But here nothing is disabled. It's added to the top level Makefile. It's dead code if unused, and causes errors if used. WHy not add the tpl target when you actually add the tpl code? > I don't think it's even possible to add an empty directory with git. True. Butt that would not fix anythign, it would still not work. > > [PATCH 2/6] powerpc/85xx: add TPL support > > > > This patch creates unused files, like > > arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds > > It gets used in later in the patchset, when a board with tpl is added. Then this is where that file belongs to. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Eureka! -- Archimedes ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board
On Mon, 31 Jan 2011 20:39:51 +0100 Wolfgang Denk wrote: > Dear haiying.w...@freescale.com, > > In message <1296499317-26616-1-git-send-email-haiying.w...@freescale.com> you > wrote: > > This patchset adds support for TPL(Tertiary Program Loader) and P1021MDS > > board. > > It is a rework of patchset at > > http://lists.denx.de/pipermail/u-boot/2010-December/082881.html, > > addresses the comments from the list and is based on the top of the tree. > > It needs to be applied after patch > > http://lists.denx.de/pipermail/u-boot/2011-January/086346.html and patch > > http://lists.denx.de/pipermail/u-boot/2011-January/086524.html > > I think these patches are incorrectly split. I think the intent was to split the arch-neutral stuff from the 85xx stuff from the board stuff -- you'd rather they be all bunched together? > [PATCH 1/6] Introduce the Tertiary Program loader > > This patch adds stuff to the Makefile, which would result in > errors if used, as the referenced directories don't exist yet. Lots of patches add features, disabled by default, that require CPU or board code to provide things, that would cause errors if the feature were enabled on a board otherwise. I don't think it's even possible to add an empty directory with git. > [PATCH 2/6] powerpc/85xx: add TPL support > > This patch creates unused files, like > arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds It gets used in later in the patchset, when a board with tpl is added. -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH0/6] patchset to support TPL and P1021MDS board
Dear haiying.w...@freescale.com, In message <1296499317-26616-1-git-send-email-haiying.w...@freescale.com> you wrote: > This patchset adds support for TPL(Tertiary Program Loader) and P1021MDS > board. > It is a rework of patchset at > http://lists.denx.de/pipermail/u-boot/2010-December/082881.html, > addresses the comments from the list and is based on the top of the tree. > It needs to be applied after patch > http://lists.denx.de/pipermail/u-boot/2011-January/086346.html and patch > http://lists.denx.de/pipermail/u-boot/2011-January/086524.html I think these patches are incorrectly split. [PATCH 1/6] Introduce the Tertiary Program loader This patch adds stuff to the Makefile, which would result in errors if used, as the referenced directories don't exist yet. [PATCH 2/6] powerpc/85xx: add TPL support This patch creates unused files, like arch/powerpc/cpu/mpc85xx/u-boot-tpl.lds This makes no sense to me. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Misquotation is, in fact, the pride and privilege of the learned. A widely-read man never quotes accurately, for the rather obvious reason that he has read too widely. - Hesketh Pearson _Common Misquotations_ introduction ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
