Re: [PATCH 1/6] Revert "Convert CONFIG_SYS_BR0_PRELIM et al to Kconfig"

["Pali Rohár" <pali@kernel.org>]

On Thursday 12 May 2022 12:01:26 Tom Rini wrote:
> On Sun, May 01, 2022 at 12:17:51PM -0400, Tom Rini wrote:
> > On Sun, May 01, 2022 at 05:33:19PM +0200, Pali Rohár wrote:
> > > On Sunday 01 May 2022 11:14:21 Tom Rini wrote:
> > > > On Sun, May 01, 2022 at 04:44:16PM +0200, Pali Rohár wrote:
> > > > > On Sunday 01 May 2022 10:39:39 Tom Rini wrote:
> > > > > > On Sun, May 01, 2022 at 04:23:52PM +0200, Pali Rohár wrote:
> > > > > > 
> > > > > > > This reverts commit c7fad78ec0ee41b72a58bebb61959570eb937ab1.
> > > > > > > 
> > > > > > > This commit made configuration, understanding, maintenance, debugging and
> > > > > > > future development of the powerpc/mpc85xx Local Bus Controller on P1/P2
> > > > > > > boards impossible.
> > > > > > > 
> > > > > > > All preliminary Base and Option registers depends on other code and C
> > > > > > > macros generated at C compile time and they comes from the other macros.
> > > > > > > 
> > > > > > > For example, NOR base address and NOR options are set via macros
> > > > > > > CONFIG_SYS_FLASH_BR_PRELIM and CONFIG_SYS_FLASH_OR_PRELIM. And then based
> > > > > > > on other logic are filled correct values in to the correct macros
> > > > > > > CONFIG_SYS_BR*_PRELIM and CONFIG_SYS_OR*_PRELIM.
> > > > > > > 
> > > > > > > These config options are not user configurable options and therefore
> > > > > > > should not appear in menuconfig. Moreover for P1/P2 boards they have
> > > > > > > nothing with DDR driver, so they should not appear in drivers/ddr.
> > > > > > > 
> > > > > > > This change was completely wrong direction, so revert it. It allows to
> > > > > > > start fixing issues with FLASH, NOR, NAND and CPLD LBC configuration.
> > > > > > > In current state it is impossible.
> > > > > > > 
> > > > > > > See also thread for more details:
> > > > > > > https://lore.kernel.org/u-boot/20220426181740.o2n7xfg46ytljcdx@pali/t/#u
> > > > > > > 
> > > > > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > > > 
> > > > > > NAK.  We are not moving things back in to board config headers under
> > > > > > CONFIG namespace.  Some other solution is required.
> > > > > 
> > > > > I spend time on this and I do not see any other solution. As explained
> > > > > that commit just introduced more issues then what it brought, so it was
> > > > > step backward, not forward. So please show other solution, if you do not
> > > > > like this one.
> > > > 
> > > > Anything that I suggested in the previous thread about moving to board
> > > > Kconfig files.
> > > 
> > > Kconfig does not support evaluating C macros from C header files. So it
> > > would not work.
> > 
> > Document how to derive them using tools like 'printf' when adding more
> > boards, which should not be a common case anyhow.
> > 
> > > > Or move it to some other header and out of CONFIG namespace.
> > > 
> > > This is board specific setting, used by drivers and arch code. So it
> > > needs to be in some board location, like the config header file.
> > 
> > But all include/config/*.h files are destined to be removed, so they
> > cannot live there.  Everything in the CONFIG namespace needs to be in
> > Kconfig.  Nothing outside of CONFIG namespace belongs under
> > include/configs/.
> > 
> > > > Or if dtoc (doc/develop/driver-model/of-plat.rst) isn't
> > > > sufficient today to pull out the infos to use at build time, expand it
> > > > to cover this case as it would be useful for large numbers of other
> > > > cases.
> > > 
> > > This would mean to rewrite completely everything about LBC configuration
> > > and its peripherals. I really do not have energy nor time for it.
> > 
> > Neither apparently has anyone else for the last far far too long.
> > 
> > > There are issues which needs to be fixed first, then some "code cleanup"
> > > and "non-functional" changes could be done.
> > > 
> > > But that your commit c7fad78ec0ee41b72a58bebb61959570eb937ab1 make it
> > > impossible to do anything with LBC, neither new development, nor doing
> > > bug fixes. So it is in the worst state in which it can be.
> > 
> > How?  Derive the correct hex value and put it in.
> > 
> > > That commit has same effect as conservation of the code and putting it
> > > into the state to disallow future development in this area. Because
> > > everybody who wants to touch it, has to first do what you wrote above.
> > 
> > Yes.  The code is sub-standard and needs improvement.
> > 
> > > But this is such giant work which nobody is going to do, just for fixing
> > > small bug, which is completely unrelated to that work. And that work is
> > > only refactor/code cleanup which does not bring any functional value.
> > > Nothing so fancy, that somebody would do in spare time.
> > > 
> > > So I hope that this was not your intension.
> > 
> > My intention is to get the PowerPC code up to modern U-Boot standards.
> > Or, to get it removed since there's not enough interest in maintaining
> > and updating it.
> > 
> > To re-iterate, I agree that it would be good to construct the values at
> > build time using macros.  No one likes looking at raw magic values.  But
> > for an otherwise dead end platform, documenting how these magic values
> > are constructed (something under doc/arch/ or doc/board/) for anyone in
> > the future doing more work is Good Enough for me.  Alternatively,
> > re-working things so that instead of being pulled from
> > include/configs/board-config.h they come from board/.../something.h is
> > Good Enough, so long as they are NOT using the CONFIG prefix.  They can
> > use CFG_ or just LBC_ or anything else that makes sense.
> 
> Getting back to this.  I see 13 more "LBC" CONFIG symbols that need
> something done to them.  I'm sure most or all of them are in the same
> situation as the PRELIM ones in this thread.  Have you attempted to move
> them out of the CONFIG namespace by chance?  If not, I might start
> looking soon at where to move them to, rather than migrate to Kconfig.
> Thanks.

No, have not looked at this. I was fixing SDHC issues (patches are now on ML).