Suggestion: Revert commit c7fad78ec0ee ("Convert CONFIG_SYS_BR0_PRELIM et al to Kconfig")

Suggestion: Revert commit c7fad78ec0ee ("Convert CONFIG_SYS_BR0_PRELIM et al to Kconfig")

[Pali Rohár]

Hello! I would suggest to revert commit c7fad78ec0ee ("Convert
CONFIG_SYS_BR0_PRELIM et al to Kconfig").

The reason is that this commit made configuration, understanding,
maintenance and debugging of the powerpc/mpc85xx Local Bus Controller
hard, mainly impossible.

This commit completely removed usage of named macros, to easily check
address and size of the LBC memory banks. Removal was done also for
explaining comments of configuration options.

It is just a mess what this commit introduced and took me really long
time to debug and understand what is happening here... until I reverted
this commit manually in my tree.

Any opinions?

Btw, current values are wrong.

Re: Suggestion: Revert commit c7fad78ec0ee ("Convert CONFIG_SYS_BR0_PRELIM et al to Kconfig")

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]

On Tue, Apr 26, 2022 at 08:17:40PM +0200, Pali Rohár wrote:

> Hello! I would suggest to revert commit c7fad78ec0ee ("Convert
> CONFIG_SYS_BR0_PRELIM et al to Kconfig").
> 
> The reason is that this commit made configuration, understanding,
> maintenance and debugging of the powerpc/mpc85xx Local Bus Controller
> hard, mainly impossible.
> 
> This commit completely removed usage of named macros, to easily check
> address and size of the LBC memory banks. Removal was done also for
> explaining comments of configuration options.
> 
> It is just a mess what this commit introduced and took me really long
> time to debug and understand what is happening here... until I reverted
> this commit manually in my tree.
> 
> Any opinions?
> 
> Btw, current values are wrong.

AFAICT, the current values match what was in use prior.  But, these
should probably not be in CONFIG namespace at all and pulled from either
device tree or some other non-board.h header file.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Suggestion: Revert commit c7fad78ec0ee ("Convert CONFIG_SYS_BR0_PRELIM et al to Kconfig")

[Pali Rohár]

On Tuesday 26 April 2022 14:23:48 Tom Rini wrote:
> On Tue, Apr 26, 2022 at 08:17:40PM +0200, Pali Rohár wrote:
> 
> > Hello! I would suggest to revert commit c7fad78ec0ee ("Convert
> > CONFIG_SYS_BR0_PRELIM et al to Kconfig").
> > 
> > The reason is that this commit made configuration, understanding,
> > maintenance and debugging of the powerpc/mpc85xx Local Bus Controller
> > hard, mainly impossible.
> > 
> > This commit completely removed usage of named macros, to easily check
> > address and size of the LBC memory banks. Removal was done also for
> > explaining comments of configuration options.
> > 
> > It is just a mess what this commit introduced and took me really long
> > time to debug and understand what is happening here... until I reverted
> > this commit manually in my tree.
> > 
> > Any opinions?
> > 
> > Btw, current values are wrong.
> 
> AFAICT, the current values match what was in use prior.

I'm not going to verify that these values really match as playing with
those magic numbers is a pain.

But some of these values were wrong even before that commit. And this
can be verified easier (just checking that size does not match to
expected value in DTS or documentation).

> But, these should probably not be in CONFIG namespace at all

Well, they do not belong to defconfig. These values are not user
configurable and are board wiring dependent. So should have never
appeared in menu config.

> and pulled from either device tree

This is probably the correct place. But you need to know e.g. values for
NAND or NOR for accessing these storages and therefore also for loading
and reading device tree blob from NAND/NOR.

So they cannot be stored in device tree (unless you parse device tree at
compile time and put the constants into appropriate u-boot bin location
available prior loading device tree blob).

> or some other non-board.h header file.

Unfortunately, these values define configuration for LocalBus Controller
which are board dependent and therefore must be in board header file or
some other board dependent code (exported to other u-boot subsystem,
powerpc/mpc init code and drivers).

Re: Suggestion: Revert commit c7fad78ec0ee ("Convert CONFIG_SYS_BR0_PRELIM et al to Kconfig")

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]

On Tue, Apr 26, 2022 at 08:35:26PM +0200, Pali Rohár wrote:
> On Tuesday 26 April 2022 14:23:48 Tom Rini wrote:
> > On Tue, Apr 26, 2022 at 08:17:40PM +0200, Pali Rohár wrote:
> > 
> > > Hello! I would suggest to revert commit c7fad78ec0ee ("Convert
> > > CONFIG_SYS_BR0_PRELIM et al to Kconfig").
> > > 
> > > The reason is that this commit made configuration, understanding,
> > > maintenance and debugging of the powerpc/mpc85xx Local Bus Controller
> > > hard, mainly impossible.
> > > 
> > > This commit completely removed usage of named macros, to easily check
> > > address and size of the LBC memory banks. Removal was done also for
> > > explaining comments of configuration options.
> > > 
> > > It is just a mess what this commit introduced and took me really long
> > > time to debug and understand what is happening here... until I reverted
> > > this commit manually in my tree.
> > > 
> > > Any opinions?
> > > 
> > > Btw, current values are wrong.
> > 
> > AFAICT, the current values match what was in use prior.
> 
> I'm not going to verify that these values really match as playing with
> those magic numbers is a pain.

Right.  It's been a while since I linked it, but:
https://patchwork.ozlabs.org/project/uboot/patch/1500407318-7977-1-git-send-email-trini@konsulko.com/
is what I use for migrating non-obvious values to Kconfig.  So I used
that to print out all of these, I'm pretty sure before and after.

> But some of these values were wrong even before that commit. And this
> can be verified easier (just checking that size does not match to
> expected value in DTS or documentation).

So some bitrot, probably then, sigh.

> > But, these should probably not be in CONFIG namespace at all
> 
> Well, they do not belong to defconfig. These values are not user
> configurable and are board wiring dependent. So should have never
> appeared in menu config.

So they shouldn't be asked and should be:
config SYS_FOO
	hex
	default 0xBEEF

in the board Kconfig files.  And the help part of
drivers/ddr/fsl/Kconfig updated to explain where/how to figure out or
find the appropriate values.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Suggestion: Revert commit c7fad78ec0ee ("Convert CONFIG_SYS_BR0_PRELIM et al to Kconfig")

[Pali Rohár]

On Tuesday 26 April 2022 14:47:40 Tom Rini wrote:
> On Tue, Apr 26, 2022 at 08:35:26PM +0200, Pali Rohár wrote:
> > On Tuesday 26 April 2022 14:23:48 Tom Rini wrote:
> > > On Tue, Apr 26, 2022 at 08:17:40PM +0200, Pali Rohár wrote:
> > > 
> > > > Hello! I would suggest to revert commit c7fad78ec0ee ("Convert
> > > > CONFIG_SYS_BR0_PRELIM et al to Kconfig").
> > > > 
> > > > The reason is that this commit made configuration, understanding,
> > > > maintenance and debugging of the powerpc/mpc85xx Local Bus Controller
> > > > hard, mainly impossible.
> > > > 
> > > > This commit completely removed usage of named macros, to easily check
> > > > address and size of the LBC memory banks. Removal was done also for
> > > > explaining comments of configuration options.
> > > > 
> > > > It is just a mess what this commit introduced and took me really long
> > > > time to debug and understand what is happening here... until I reverted
> > > > this commit manually in my tree.
> > > > 
> > > > Any opinions?
> > > > 
> > > > Btw, current values are wrong.
> > > 
> > > AFAICT, the current values match what was in use prior.
> > 
> > I'm not going to verify that these values really match as playing with
> > those magic numbers is a pain.
> 
> Right.  It's been a while since I linked it, but:
> https://patchwork.ozlabs.org/project/uboot/patch/1500407318-7977-1-git-send-email-trini@konsulko.com/
> is what I use for migrating non-obvious values to Kconfig.  So I used
> that to print out all of these, I'm pretty sure before and after.

Ok. I guess that this should generate same values for _default_
configuration. But modification via menu config does not have to produce
correct values...

> > But some of these values were wrong even before that commit. And this
> > can be verified easier (just checking that size does not match to
> > expected value in DTS or documentation).
> 
> So some bitrot, probably then, sigh.

The point is that now with magic hex values, it is impossible to detect
that constants are not correct.

Previously when constants were defined via named macros, it was lot of
easier.

So, if I see that previously Option Register for NAND bank was
initialized to value

  (OR_AM_32KB | OR_FCM_PGS | OR_FCM_CSCT | OR_FCM_CST | OR_FCM_CHT | OR_FCM_SCY_1 | OR_FCM_TRLX | OR_FCM_EHTR)

I could easily detect that this values is incorrect for NANDs with large
paging, which needs 256 kB window.

But now if I see just magic value 0xFFFF8396 I do not spot that this
value encodes 32 kB instead of 256 kB.

This is the way how to hide issues and possible bugs.

> > > But, these should probably not be in CONFIG namespace at all
> > 
> > Well, they do not belong to defconfig. These values are not user
> > configurable and are board wiring dependent. So should have never
> > appeared in menu config.
> 
> So they shouldn't be asked and should be:
> config SYS_FOO
> 	hex
> 	default 0xBEEF
> 
> in the board Kconfig files.  And the help part of
> drivers/ddr/fsl/Kconfig updated to explain where/how to figure out or
> find the appropriate values.

Having value in board Kconfig file _without_ help text (to make config
option _not_ user selectable) is a little bit better. But it lacks help
text and still does not solve the problem with magic constants which
just hide issues.


For this one particular case for *PRELIM* macros, I'm thinking if it
would not be better idea to move all these constants into global
variables / arrays, defined in board source files. Like are already
defined configuration to TLB entries or LAWs. See files:
board/freescale/p1_p2_rdb_pc/law.c
board/freescale/p1_p2_rdb_pc/tlb.c
Code which needs these values just access (global) variable/array, see
how those files are used.

Re: Suggestion: Revert commit c7fad78ec0ee ("Convert CONFIG_SYS_BR0_PRELIM et al to Kconfig")

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5106 bytes --]

On Tue, Apr 26, 2022 at 09:07:44PM +0200, Pali Rohár wrote:
> On Tuesday 26 April 2022 14:47:40 Tom Rini wrote:
> > On Tue, Apr 26, 2022 at 08:35:26PM +0200, Pali Rohár wrote:
> > > On Tuesday 26 April 2022 14:23:48 Tom Rini wrote:
> > > > On Tue, Apr 26, 2022 at 08:17:40PM +0200, Pali Rohár wrote:
> > > > 
> > > > > Hello! I would suggest to revert commit c7fad78ec0ee ("Convert
> > > > > CONFIG_SYS_BR0_PRELIM et al to Kconfig").
> > > > > 
> > > > > The reason is that this commit made configuration, understanding,
> > > > > maintenance and debugging of the powerpc/mpc85xx Local Bus Controller
> > > > > hard, mainly impossible.
> > > > > 
> > > > > This commit completely removed usage of named macros, to easily check
> > > > > address and size of the LBC memory banks. Removal was done also for
> > > > > explaining comments of configuration options.
> > > > > 
> > > > > It is just a mess what this commit introduced and took me really long
> > > > > time to debug and understand what is happening here... until I reverted
> > > > > this commit manually in my tree.
> > > > > 
> > > > > Any opinions?
> > > > > 
> > > > > Btw, current values are wrong.
> > > > 
> > > > AFAICT, the current values match what was in use prior.
> > > 
> > > I'm not going to verify that these values really match as playing with
> > > those magic numbers is a pain.
> > 
> > Right.  It's been a while since I linked it, but:
> > https://patchwork.ozlabs.org/project/uboot/patch/1500407318-7977-1-git-send-email-trini@konsulko.com/
> > is what I use for migrating non-obvious values to Kconfig.  So I used
> > that to print out all of these, I'm pretty sure before and after.
> 
> Ok. I guess that this should generate same values for _default_
> configuration. But modification via menu config does not have to produce
> correct values...
> 
> > > But some of these values were wrong even before that commit. And this
> > > can be verified easier (just checking that size does not match to
> > > expected value in DTS or documentation).
> > 
> > So some bitrot, probably then, sigh.
> 
> The point is that now with magic hex values, it is impossible to detect
> that constants are not correct.
> 
> Previously when constants were defined via named macros, it was lot of
> easier.
> 
> So, if I see that previously Option Register for NAND bank was
> initialized to value
> 
>   (OR_AM_32KB | OR_FCM_PGS | OR_FCM_CSCT | OR_FCM_CST | OR_FCM_CHT | OR_FCM_SCY_1 | OR_FCM_TRLX | OR_FCM_EHTR)
> 
> I could easily detect that this values is incorrect for NANDs with large
> paging, which needs 256 kB window.
> 
> But now if I see just magic value 0xFFFF8396 I do not spot that this
> value encodes 32 kB instead of 256 kB.
> 
> This is the way how to hide issues and possible bugs.
> 
> > > > But, these should probably not be in CONFIG namespace at all
> > > 
> > > Well, they do not belong to defconfig. These values are not user
> > > configurable and are board wiring dependent. So should have never
> > > appeared in menu config.
> > 
> > So they shouldn't be asked and should be:
> > config SYS_FOO
> > 	hex
> > 	default 0xBEEF
> > 
> > in the board Kconfig files.  And the help part of
> > drivers/ddr/fsl/Kconfig updated to explain where/how to figure out or
> > find the appropriate values.
> 
> Having value in board Kconfig file _without_ help text (to make config
> option _not_ user selectable) is a little bit better. But it lacks help
> text and still does not solve the problem with magic constants which
> just hide issues.

So, part of the problem is that you shouldn't do what we do today of
duplicating "config FOO" in multiple places.  Copy/pasting that help
probably won't break things, but isn't I think an improvement.  There's
lots of cases where I suggest / just tell people to have:
config FOO
	hex
	default 0x100 if A
	default 0x200 if B || C
and so on, and that's also not ideal, but I think does help lead to
consolidation down the line.  That won't be the case here, since these
are board-specific and I can see objections to default 0xBEEF if
TARGET_BAR and so on.  And as you note above, these should be
constructed in many / most cases.

> For this one particular case for *PRELIM* macros, I'm thinking if it
> would not be better idea to move all these constants into global
> variables / arrays, defined in board source files. Like are already
> defined configuration to TLB entries or LAWs. See files:
> board/freescale/p1_p2_rdb_pc/law.c
> board/freescale/p1_p2_rdb_pc/tlb.c
> Code which needs these values just access (global) variable/array, see
> how those files are used.

I agree that what's there now isn't ideal.  But "leave things in the
board.h files until someone can do a clever conversion for these harder
problems" hasn't worked.  I am quite open to moving forward with better
suggestions, especially since there's quite likely more cases of magic
values needing to be moved out of where they are and not being really
user-configurable either.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: Suggestion: Revert commit c7fad78ec0ee ("Convert CONFIG_SYS_BR0_PRELIM et al to Kconfig")

[Pali Rohár]

On Tuesday 26 April 2022 15:40:42 Tom Rini wrote:
> On Tue, Apr 26, 2022 at 09:07:44PM +0200, Pali Rohár wrote:
> > On Tuesday 26 April 2022 14:47:40 Tom Rini wrote:
> > > On Tue, Apr 26, 2022 at 08:35:26PM +0200, Pali Rohár wrote:
> > > > On Tuesday 26 April 2022 14:23:48 Tom Rini wrote:
> > > > > On Tue, Apr 26, 2022 at 08:17:40PM +0200, Pali Rohár wrote:
> > > > > 
> > > > > > Hello! I would suggest to revert commit c7fad78ec0ee ("Convert
> > > > > > CONFIG_SYS_BR0_PRELIM et al to Kconfig").
> > > > > > 
> > > > > > The reason is that this commit made configuration, understanding,
> > > > > > maintenance and debugging of the powerpc/mpc85xx Local Bus Controller
> > > > > > hard, mainly impossible.
> > > > > > 
> > > > > > This commit completely removed usage of named macros, to easily check
> > > > > > address and size of the LBC memory banks. Removal was done also for
> > > > > > explaining comments of configuration options.
> > > > > > 
> > > > > > It is just a mess what this commit introduced and took me really long
> > > > > > time to debug and understand what is happening here... until I reverted
> > > > > > this commit manually in my tree.
> > > > > > 
> > > > > > Any opinions?
> > > > > > 
> > > > > > Btw, current values are wrong.
> > > > > 
> > > > > AFAICT, the current values match what was in use prior.
> > > > 
> > > > I'm not going to verify that these values really match as playing with
> > > > those magic numbers is a pain.
> > > 
> > > Right.  It's been a while since I linked it, but:
> > > https://patchwork.ozlabs.org/project/uboot/patch/1500407318-7977-1-git-send-email-trini@konsulko.com/
> > > is what I use for migrating non-obvious values to Kconfig.  So I used
> > > that to print out all of these, I'm pretty sure before and after.
> > 
> > Ok. I guess that this should generate same values for _default_
> > configuration. But modification via menu config does not have to produce
> > correct values...
> > 
> > > > But some of these values were wrong even before that commit. And this
> > > > can be verified easier (just checking that size does not match to
> > > > expected value in DTS or documentation).
> > > 
> > > So some bitrot, probably then, sigh.
> > 
> > The point is that now with magic hex values, it is impossible to detect
> > that constants are not correct.
> > 
> > Previously when constants were defined via named macros, it was lot of
> > easier.
> > 
> > So, if I see that previously Option Register for NAND bank was
> > initialized to value
> > 
> >   (OR_AM_32KB | OR_FCM_PGS | OR_FCM_CSCT | OR_FCM_CST | OR_FCM_CHT | OR_FCM_SCY_1 | OR_FCM_TRLX | OR_FCM_EHTR)
> > 
> > I could easily detect that this values is incorrect for NANDs with large
> > paging, which needs 256 kB window.
> > 
> > But now if I see just magic value 0xFFFF8396 I do not spot that this
> > value encodes 32 kB instead of 256 kB.
> > 
> > This is the way how to hide issues and possible bugs.
> > 
> > > > > But, these should probably not be in CONFIG namespace at all
> > > > 
> > > > Well, they do not belong to defconfig. These values are not user
> > > > configurable and are board wiring dependent. So should have never
> > > > appeared in menu config.
> > > 
> > > So they shouldn't be asked and should be:
> > > config SYS_FOO
> > > 	hex
> > > 	default 0xBEEF
> > > 
> > > in the board Kconfig files.  And the help part of
> > > drivers/ddr/fsl/Kconfig updated to explain where/how to figure out or
> > > find the appropriate values.
> > 
> > Having value in board Kconfig file _without_ help text (to make config
> > option _not_ user selectable) is a little bit better. But it lacks help
> > text and still does not solve the problem with magic constants which
> > just hide issues.
> 
> So, part of the problem is that you shouldn't do what we do today of
> duplicating "config FOO" in multiple places.  Copy/pasting that help
> probably won't break things, but isn't I think an improvement.  There's
> lots of cases where I suggest / just tell people to have:
> config FOO
> 	hex
> 	default 0x100 if A
> 	default 0x200 if B || C
> and so on, and that's also not ideal, but I think does help lead to
> consolidation down the line.  That won't be the case here, since these
> are board-specific and I can see objections to default 0xBEEF if
> TARGET_BAR and so on.  And as you note above, these should be
> constructed in many / most cases.
> 
> > For this one particular case for *PRELIM* macros, I'm thinking if it
> > would not be better idea to move all these constants into global
> > variables / arrays, defined in board source files. Like are already
> > defined configuration to TLB entries or LAWs. See files:
> > board/freescale/p1_p2_rdb_pc/law.c
> > board/freescale/p1_p2_rdb_pc/tlb.c
> > Code which needs these values just access (global) variable/array, see
> > how those files are used.
> 
> I agree that what's there now isn't ideal.  But "leave things in the
> board.h files until someone can do a clever conversion for these harder
> problems" hasn't worked.  I am quite open to moving forward with better
> suggestions, especially since there's quite likely more cases of magic
> values needing to be moved out of where they are and not being really
> user-configurable either.
> 
> -- 
> Tom

In any case, if I'm looking at these PRELIM macros correctly, they are
not DDR-related, so I do not understand why they are defined in
drivers/ddr subtree. They are purely LBC specific and required for
correct preliminary access to local bus, specially NAND or NOR.

Re: Suggestion: Revert commit c7fad78ec0ee ("Convert CONFIG_SYS_BR0_PRELIM et al to Kconfig")

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 6136 bytes --]

On Wed, Apr 27, 2022 at 09:20:52AM +0200, Pali Rohár wrote:
> On Tuesday 26 April 2022 15:40:42 Tom Rini wrote:
> > On Tue, Apr 26, 2022 at 09:07:44PM +0200, Pali Rohár wrote:
> > > On Tuesday 26 April 2022 14:47:40 Tom Rini wrote:
> > > > On Tue, Apr 26, 2022 at 08:35:26PM +0200, Pali Rohár wrote:
> > > > > On Tuesday 26 April 2022 14:23:48 Tom Rini wrote:
> > > > > > On Tue, Apr 26, 2022 at 08:17:40PM +0200, Pali Rohár wrote:
> > > > > > 
> > > > > > > Hello! I would suggest to revert commit c7fad78ec0ee ("Convert
> > > > > > > CONFIG_SYS_BR0_PRELIM et al to Kconfig").
> > > > > > > 
> > > > > > > The reason is that this commit made configuration, understanding,
> > > > > > > maintenance and debugging of the powerpc/mpc85xx Local Bus Controller
> > > > > > > hard, mainly impossible.
> > > > > > > 
> > > > > > > This commit completely removed usage of named macros, to easily check
> > > > > > > address and size of the LBC memory banks. Removal was done also for
> > > > > > > explaining comments of configuration options.
> > > > > > > 
> > > > > > > It is just a mess what this commit introduced and took me really long
> > > > > > > time to debug and understand what is happening here... until I reverted
> > > > > > > this commit manually in my tree.
> > > > > > > 
> > > > > > > Any opinions?
> > > > > > > 
> > > > > > > Btw, current values are wrong.
> > > > > > 
> > > > > > AFAICT, the current values match what was in use prior.
> > > > > 
> > > > > I'm not going to verify that these values really match as playing with
> > > > > those magic numbers is a pain.
> > > > 
> > > > Right.  It's been a while since I linked it, but:
> > > > https://patchwork.ozlabs.org/project/uboot/patch/1500407318-7977-1-git-send-email-trini@konsulko.com/
> > > > is what I use for migrating non-obvious values to Kconfig.  So I used
> > > > that to print out all of these, I'm pretty sure before and after.
> > > 
> > > Ok. I guess that this should generate same values for _default_
> > > configuration. But modification via menu config does not have to produce
> > > correct values...
> > > 
> > > > > But some of these values were wrong even before that commit. And this
> > > > > can be verified easier (just checking that size does not match to
> > > > > expected value in DTS or documentation).
> > > > 
> > > > So some bitrot, probably then, sigh.
> > > 
> > > The point is that now with magic hex values, it is impossible to detect
> > > that constants are not correct.
> > > 
> > > Previously when constants were defined via named macros, it was lot of
> > > easier.
> > > 
> > > So, if I see that previously Option Register for NAND bank was
> > > initialized to value
> > > 
> > >   (OR_AM_32KB | OR_FCM_PGS | OR_FCM_CSCT | OR_FCM_CST | OR_FCM_CHT | OR_FCM_SCY_1 | OR_FCM_TRLX | OR_FCM_EHTR)
> > > 
> > > I could easily detect that this values is incorrect for NANDs with large
> > > paging, which needs 256 kB window.
> > > 
> > > But now if I see just magic value 0xFFFF8396 I do not spot that this
> > > value encodes 32 kB instead of 256 kB.
> > > 
> > > This is the way how to hide issues and possible bugs.
> > > 
> > > > > > But, these should probably not be in CONFIG namespace at all
> > > > > 
> > > > > Well, they do not belong to defconfig. These values are not user
> > > > > configurable and are board wiring dependent. So should have never
> > > > > appeared in menu config.
> > > > 
> > > > So they shouldn't be asked and should be:
> > > > config SYS_FOO
> > > > 	hex
> > > > 	default 0xBEEF
> > > > 
> > > > in the board Kconfig files.  And the help part of
> > > > drivers/ddr/fsl/Kconfig updated to explain where/how to figure out or
> > > > find the appropriate values.
> > > 
> > > Having value in board Kconfig file _without_ help text (to make config
> > > option _not_ user selectable) is a little bit better. But it lacks help
> > > text and still does not solve the problem with magic constants which
> > > just hide issues.
> > 
> > So, part of the problem is that you shouldn't do what we do today of
> > duplicating "config FOO" in multiple places.  Copy/pasting that help
> > probably won't break things, but isn't I think an improvement.  There's
> > lots of cases where I suggest / just tell people to have:
> > config FOO
> > 	hex
> > 	default 0x100 if A
> > 	default 0x200 if B || C
> > and so on, and that's also not ideal, but I think does help lead to
> > consolidation down the line.  That won't be the case here, since these
> > are board-specific and I can see objections to default 0xBEEF if
> > TARGET_BAR and so on.  And as you note above, these should be
> > constructed in many / most cases.
> > 
> > > For this one particular case for *PRELIM* macros, I'm thinking if it
> > > would not be better idea to move all these constants into global
> > > variables / arrays, defined in board source files. Like are already
> > > defined configuration to TLB entries or LAWs. See files:
> > > board/freescale/p1_p2_rdb_pc/law.c
> > > board/freescale/p1_p2_rdb_pc/tlb.c
> > > Code which needs these values just access (global) variable/array, see
> > > how those files are used.
> > 
> > I agree that what's there now isn't ideal.  But "leave things in the
> > board.h files until someone can do a clever conversion for these harder
> > problems" hasn't worked.  I am quite open to moving forward with better
> > suggestions, especially since there's quite likely more cases of magic
> > values needing to be moved out of where they are and not being really
> > user-configurable either.
> 
> In any case, if I'm looking at these PRELIM macros correctly, they are
> not DDR-related, so I do not understand why they are defined in
> drivers/ddr subtree. They are purely LBC specific and required for
> correct preliminary access to local bus, specially NAND or NOR.

Some value I was checking on, I think, was referenced under there, so
that's where I put it.  Moving it all elsewhere the makes more sense is
also totally fine and appropriate.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]