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

Wed Apr 27 15:09:21 CEST 2022

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20220427/ac2a99f1/attachment.sig>