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-... 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.