| Message ID | 1409581243-12695-1-git-send-email-nitin.garg@freescale.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
On Mon, Sep 1, 2014 at 11:20 AM, Nitin Garg <nitin.garg@freescale.com> wrote: > Move board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg to > board/freescale/mx6sabresd/mx6q_4x_mt41j128.cfg as this is > was designed for the mx6sabresd board. This also updates the > cgtqmx6qeval which makes use of this configuration. > > Signed-off-by: Nitin Garg <nitin.garg@freescale.com> Acked-by: Fabio Estevam <fabio.estevam@freescale.com> Thanks
On Mon, Sep 1, 2014 at 11:24 AM, Fabio Estevam <festevam@gmail.com> wrote: > On Mon, Sep 1, 2014 at 11:20 AM, Nitin Garg <nitin.garg@freescale.com> wrote: >> Move board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg to >> board/freescale/mx6sabresd/mx6q_4x_mt41j128.cfg as this is >> was designed for the mx6sabresd board. This also updates the >> cgtqmx6qeval which makes use of this configuration. >> >> Signed-off-by: Nitin Garg <nitin.garg@freescale.com> > > Acked-by: Fabio Estevam <fabio.estevam@freescale.com> Acked-by: Otavio Salvador <otavio@ossystems.com.br>
Dear Nitin Garg, In message <1409581243-12695-1-git-send-email-nitin.garg@freescale.com> you wrote: > Move board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg to > board/freescale/mx6sabresd/mx6q_4x_mt41j128.cfg as this is > was designed for the mx6sabresd board. This also updates the > cgtqmx6qeval which makes use of this configuration. Sorry, but I don't get it. First you say, that mx6q_4x_mt41j128.cfg is a mx6sabresd specific file, and move it to the board specific directory. But in the next sentence you state that this very file is also used by another board (cgtqmx6qeval) - so apparently it is NOT a board specific file. Actually this makes even more sense to me, as I would expect that the file is more specific to the DDR type than to the board. So why exactly do you think it would be better to move this into a board specific place? Best regards, Wolfgang Denk
Wolfgang, (Dropping Leo e-mail as it bounces, adding Alex who seem to be working on BSP support at Congatec now) On Mon, Sep 1, 2014 at 2:59 PM, Wolfgang Denk <wd@denx.de> wrote: > In message <1409581243-12695-1-git-send-email-nitin.garg@freescale.com> you wrote: >> Move board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg to >> board/freescale/mx6sabresd/mx6q_4x_mt41j128.cfg as this is >> was designed for the mx6sabresd board. This also updates the >> cgtqmx6qeval which makes use of this configuration. > > Sorry, but I don't get it. First you say, that mx6q_4x_mt41j128.cfg > is a mx6sabresd specific file, and move it to the board specific > directory. > > But in the next sentence you state that this very file is also used by > another board (cgtqmx6qeval) - so apparently it is NOT a board > specific file. Actually this makes even more sense to me, as I would > expect that the file is more specific to the DDR type than to the > board. > > So why exactly do you think it would be better to move this into a > board specific place? All the calibration has been done by Freescale and for the SABRE SD board. This is not guarantee it works in other boards, neither expected. Congatec opted to include this file but it is their choice and moving to the board file makes it more evident. Some vendors choose to reuse other vendors provided memory configuration (this has been done in some board using Nitrogen6X values for example). If this is good or bad ... this is a hard question to answer.
Dear Otavio, In message <CAP9ODKonaZ9v76WAdd4k7or_kUWF=ATTLf+-DrqgQYVgKbt8Bg@mail.gmail.com> you wrote: > > > But in the next sentence you state that this very file is also used by > > another board (cgtqmx6qeval) - so apparently it is NOT a board > > specific file. Actually this makes even more sense to me, as I would > > expect that the file is more specific to the DDR type than to the > > board. > > > > So why exactly do you think it would be better to move this into a > > board specific place? > > All the calibration has been done by Freescale and for the SABRE SD > board. This is not guarantee it works in other boards, neither > expected. Congatec opted to include this file but it is their choice > and moving to the board file makes it more evident. This may be all true, but nevertheless this is NOT board specific code. And you can already see it being reused by other boards, so the most natural way to handle it is to factor it out into a common directory. And actually this is what we had before. If we are going to change code, we should have a good reason for such a change, like fixing bugs, adding features, or cleaning up. What exactly is that good reason here? Moving code used by more than one board from a common place to a board-specific one make things worse, not better. Best regards, Wolfgang Denk
Hi Il 01/set/2014 21:02 "Wolfgang Denk" <wd@denx.de> ha scritto: > > Dear Otavio, > > In message <CAP9ODKonaZ9v76WAdd4k7or_kUWF= ATTLf+-DrqgQYVgKbt8Bg@mail.gmail.com> you wrote: > > > > > But in the next sentence you state that this very file is also used by > > > another board (cgtqmx6qeval) - so apparently it is NOT a board > > > specific file. Actually this makes even more sense to me, as I would > > > expect that the file is more specific to the DDR type than to the > > > board. > > > > > > So why exactly do you think it would be better to move this into a > > > board specific place? > > > > All the calibration has been done by Freescale and for the SABRE SD > > board. This is not guarantee it works in other boards, neither > > expected. Congatec opted to include this file but it is their choice > > and moving to the board file makes it more evident. > > This may be all true, but nevertheless this is NOT board specific > code. And you can already see it being reused by other boards, so the > most natural way to handle it is to factor it out into a common > directory. And actually this is what we had before. > > If we are going to change code, we should have a good reason for such > a change, like fixing bugs, adding features, or cleaning up. What > exactly is that good reason here? Moving code used by more than one > board from a common place to a board-specific one make things worse, > not better. > Agree with Walfgang Michael > 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: wd@denx.de > Until you walk a mile in another man's moccasins, you can't imagine > the smell. > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
Wolfgang, On Mon, Sep 1, 2014 at 4:02 PM, Wolfgang Denk <wd@denx.de> wrote: > In message <CAP9ODKonaZ9v76WAdd4k7or_kUWF=ATTLf+-DrqgQYVgKbt8Bg@mail.gmail.com> you wrote: >> >> > But in the next sentence you state that this very file is also used by >> > another board (cgtqmx6qeval) - so apparently it is NOT a board >> > specific file. Actually this makes even more sense to me, as I would >> > expect that the file is more specific to the DDR type than to the >> > board. >> > >> > So why exactly do you think it would be better to move this into a >> > board specific place? >> >> All the calibration has been done by Freescale and for the SABRE SD >> board. This is not guarantee it works in other boards, neither >> expected. Congatec opted to include this file but it is their choice >> and moving to the board file makes it more evident. > > This may be all true, but nevertheless this is NOT board specific > code. And you can already see it being reused by other boards, so the > most natural way to handle it is to factor it out into a common > directory. And actually this is what we had before. > > If we are going to change code, we should have a good reason for such > a change, like fixing bugs, adding features, or cleaning up. What > exactly is that good reason here? Moving code used by more than one > board from a common place to a board-specific one make things worse, > not better. I disagree (but this is my personal view on this). The reason why I disagree is because the DDR calibration is very design dependant so if/when Freescale optimize their DDR data setting they may break any other board using it however they shouldn't be blamed by it as this is their DDR settings. Any board including this file (which can be and is done) takes the responsibility and risks.
Dear Otavio, In message <CAP9ODKrsC4O-P7+CmKKbeDa8wwWxVpNpXOyOctP6WcVoJ5gYeA@mail.gmail.com> you wrote: > > I disagree (but this is my personal view on this). The reason why I > disagree is because the DDR calibration is very design dependant so > if/when Freescale optimize their DDR data setting they may break any > other board using it however they shouldn't be blamed by it as this is > their DDR settings. Any board including this file (which can be and is > done) takes the responsibility and risks. Obviously, any changes to common code used by several boards needs to be tested on the affected boards. Also, it might be instructive for you to read the commit message for af7ec0b "mx6q: Factor out common DDR3 init code". Apparently Fabio considers this "common DDR3 initialization code", so what exactly are your arguments to the contrary? Best regards, Wolfgang Denk
Dear Nitin Garg, In message <1409581243-12695-1-git-send-email-nitin.garg@freescale.com> you wrote: > Move board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg to > board/freescale/mx6sabresd/mx6q_4x_mt41j128.cfg as this is > was designed for the mx6sabresd board. This also updates the > cgtqmx6qeval which makes use of this configuration. I've made my mind up. I hereby NAK this patch, as it would basically revert Fabio Estevam's commit af7ec0b which moved the originally board-specific code to a common place: commit af7ec0b0582f658873713c311497626c571f3b31 Author: Fabio Estevam <fabio.estevam@freescale.com> Date: Thu Sep 13 03:18:19 2012 +0000 mx6q: Factor out common DDR3 init code Factor out common DDR3 initialization code, allowing easier maintainance of such scripts. Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> Fabio's intention was a good one, as is proven by the re-use of this code by other boards. Best regards, Wolfgang Denk
Hi Wolfgang, On Mon, Sep 1, 2014 at 4:24 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Nitin Garg, > > In message <1409581243-12695-1-git-send-email-nitin.garg@freescale.com> you wrote: >> Move board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg to >> board/freescale/mx6sabresd/mx6q_4x_mt41j128.cfg as this is >> was designed for the mx6sabresd board. This also updates the >> cgtqmx6qeval which makes use of this configuration. > > I've made my mind up. I hereby NAK this patch, as it would basically > revert Fabio Estevam's commit af7ec0b which moved the originally > board-specific code to a common place: > > commit af7ec0b0582f658873713c311497626c571f3b31 > Author: Fabio Estevam <fabio.estevam@freescale.com> > Date: Thu Sep 13 03:18:19 2012 +0000 > > mx6q: Factor out common DDR3 init code > > Factor out common DDR3 initialization code, allowing easier > maintainance of such scripts. > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > > Fabio's intention was a good one, as is proven by the re-use of this > code by other boards. Let me provide some background on the reason I sent that patch: at that time we had the same DDR3 init code for several boards, such as mx6qsabresd, nitrogen, sabrelite, so I wanted to avoid duplicating the same init for several boards. After sometime, each board used to followed its own specific settings, as the DDR3 init is very dependant on board layout and some optimizations that are valid for one board does not apply to others. Each board developer has to be really careful about properly configuring DDR in order to achieve stability, so re-use of the DCD settings should be done really carefully. As it stands today only mx6qsabresd and congatec share the same script. I think Nitin's patch goes in the right direction, as it makes clearer for other developers that the DDR specific settings are optmized for mx6qsabresd only. Of course people can re-use it, like congatec board does today, but if in the future we find some more optimal settings for this board we should apply it to mx6sabresd, but we really don't know the consequences into other hardware. So they have been warned :-) Moving forward we should really get rid of this DCD syntax and move to SPL style. Regards, Fabio Estevam
Hi Fabio, On 09/01/2014 03:27 PM, Fabio Estevam wrote: > Hi Wolfgang, > > On Mon, Sep 1, 2014 at 4:24 PM, Wolfgang Denk <wd@denx.de> wrote: >> Dear Nitin Garg, >> >> In message <1409581243-12695-1-git-send-email-nitin.garg@freescale.com> you wrote: >>> Move board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg to >>> board/freescale/mx6sabresd/mx6q_4x_mt41j128.cfg as this is >>> was designed for the mx6sabresd board. This also updates the >>> cgtqmx6qeval which makes use of this configuration. >> >> I've made my mind up. I hereby NAK this patch, as it would basically >> revert Fabio Estevam's commit af7ec0b which moved the originally >> board-specific code to a common place: >> >> commit af7ec0b0582f658873713c311497626c571f3b31 >> Author: Fabio Estevam <fabio.estevam@freescale.com> >> Date: Thu Sep 13 03:18:19 2012 +0000 >> >> mx6q: Factor out common DDR3 init code >> >> Factor out common DDR3 initialization code, allowing easier >> maintainance of such scripts. >> >> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> >> >> Fabio's intention was a good one, as is proven by the re-use of this >> code by other boards. > > Let me provide some background on the reason I sent that patch: at > that time we had the same DDR3 init code for several boards, such as > mx6qsabresd, nitrogen, sabrelite, so I wanted to avoid duplicating the > same init for several boards. > Specifically, the Nitrogen and SABRE Lite designs use a different memory layout from SABRE SD and SABRE Auto and the DDR calibration data is quite different. Boards may share the same memory arrangement, but it's unlikely that the calibration process has been performed on multiple board types and achieved the same values. It's possible that many boards copied the layout and stack-up from the SABRE SD design such that the board functions properly with the SABRE SD values, but also likely that some vendors just don't know that their calibration results will differ. > After sometime, each board used to followed its own specific settings, > as the DDR3 init is very dependant on board layout and some > optimizations that are valid for one board does not apply to others. > Each board developer has to be really careful about properly > configuring DDR in order to achieve stability, so re-use of the DCD > settings should be done really carefully. > Note that mx6q_4x_mt41j128.cfg combines multiple things in the same config file. Separating them (especially the calibration data) as done in the nitrogen6x/ tree will help distinguish between the design-time parts of the configuration and the measured calibration. > As it stands today only mx6qsabresd and congatec share the same script. > I believe that the Wand board is using the configuration files from the nitrogen6x tree. > I think Nitin's patch goes in the right direction, as it makes clearer > for other developers that the DDR specific settings are optmized for > mx6qsabresd only. Of course people can re-use it, like congatec board > does today, but if in the future we find some more optimal settings > for this board we should apply it to mx6sabresd, but we really don't > know the consequences into other hardware. So they have been warned > :-) > > Moving forward we should really get rid of this DCD syntax and move to > SPL style. > There's no way to completely get rid of the DCD. It may be possible (even beneficial) to do run-time DDR calibration, but that's off-topic in this thread. Regards, Eric
On Mon, Sep 1, 2014 at 8:34 PM, Eric Nelson <eric.nelson@boundarydevices.com> wrote: > I believe that the Wand board is using the configuration files > from the nitrogen6x tree. Yes, I should have said "As it stands today only mx6qsabresd and congatec share the same mx6q_4x_mt41j128.cfg script." Regards, Fabio Estevam
Hi everybody, On 02/09/2014 01:51, Fabio Estevam wrote: > On Mon, Sep 1, 2014 at 8:34 PM, Eric Nelson > <eric.nelson@boundarydevices.com> wrote: > >> I believe that the Wand board is using the configuration files >> from the nitrogen6x tree. > > Yes, I should have said "As it stands today only mx6qsabresd and > congatec share the same mx6q_4x_mt41j128.cfg script." > I try to sumarize the result of the discussion. This file was factorized, but it was proofed that calibration data are very board specific, and factorizing this file is not correct. I agree with Fabio and Eric that it is very unlike to have the same calibration data for different boards and the reuse of the same data is not correct. However, with this statement the patch should also unbind the congatec board from the sabresd, not only change the reference. A congatec specific board must be also created in congatec/cgtqmx6eval, and it is only a case that it has the same values of the sabresd. Best regards, Stefano Babic
On Fri, Sep 12, 2014 at 4:53 AM, Stefano Babic <sbabic@denx.de> wrote: > I try to sumarize the result of the discussion. This file was > factorized, but it was proofed that calibration data are very board > specific, and factorizing this file is not correct. I agree with Fabio > and Eric that it is very unlike to have the same calibration data for > different boards and the reuse of the same data is not correct. > > However, with this statement the patch should also unbind the congatec > board from the sabresd, not only change the reference. A congatec > specific board must be also created in congatec/cgtqmx6eval, and it is > only a case that it has the same values of the sabresd. Yes, this is a good approach. Nitin, Please send a v3 based on Stefano's suggestion. Regards, Fabio Estevam
diff --git a/board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg b/board/freescale/mx6sabresd/mx6q_4x_mt41j128.cfg similarity index 100% rename from board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg rename to board/freescale/mx6sabresd/mx6q_4x_mt41j128.cfg diff --git a/configs/cgtqmx6qeval_defconfig b/configs/cgtqmx6qeval_defconfig index 6699381..2f83808 100644 --- a/configs/cgtqmx6qeval_defconfig +++ b/configs/cgtqmx6qeval_defconfig @@ -1,3 +1,3 @@ -CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg,MX6Q" +CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/mx6sabresd/mx6q_4x_mt41j128.cfg,MX6Q" CONFIG_ARM=y CONFIG_TARGET_CGTQMX6EVAL=y diff --git a/configs/mx6qsabresd_defconfig b/configs/mx6qsabresd_defconfig index dc8e254..67c1b77 100644 --- a/configs/mx6qsabresd_defconfig +++ b/configs/mx6qsabresd_defconfig @@ -1,3 +1,3 @@ -CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg,MX6Q" +CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/freescale/mx6sabresd/mx6q_4x_mt41j128.cfg,MX6Q" CONFIG_ARM=y CONFIG_TARGET_MX6SABRESD=y
Move board/freescale/imx/ddr/mx6q_4x_mt41j128.cfg to board/freescale/mx6sabresd/mx6q_4x_mt41j128.cfg as this is was designed for the mx6sabresd board. This also updates the cgtqmx6qeval which makes use of this configuration. Signed-off-by: Nitin Garg <nitin.garg@freescale.com> --- .../{imx/ddr => mx6sabresd}/mx6q_4x_mt41j128.cfg | 0 configs/cgtqmx6qeval_defconfig | 2 +- configs/mx6qsabresd_defconfig | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) rename board/freescale/{imx/ddr => mx6sabresd}/mx6q_4x_mt41j128.cfg (100%)