[PATCH 2/3] configs: keystone2: Do not include hardware.h

[Tom Rini]

On Mon, Nov 20, 2023 at 07:16:03PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 20 Nov 2023 at 08:26, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Mon, Nov 20, 2023 at 08:49:32AM -0600, Andrew Davis wrote:
> > > On 11/17/23 4:40 PM, Tom Rini wrote:
> > > > On Fri, Nov 17, 2023 at 04:38:28PM -0600, Andrew Davis wrote:
> > > >
> > > > > This is a hacky way to have this file included in all source files that
> > > > > include common.h, instead just include from the files that need it.
> > > > >
> > > > > Signed-off-by: Andrew Davis <afd at ti.com>
> > > > > ---
> > > > >   drivers/memory/ti-aemif.c            | 1 +
> > > > >   drivers/soc/ti/keystone_serdes.c     | 1 +
> > > > >   include/configs/ti_armv7_keystone2.h | 1 -
> > > > >   3 files changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
> > > > > index c4bc88c1510..41325eb0f94 100644
> > > > > --- a/drivers/memory/ti-aemif.c
> > > > > +++ b/drivers/memory/ti-aemif.c
> > > > > @@ -7,6 +7,7 @@
> > > > >    */
> > > > >   #include <common.h>
> > > > > +#include <asm/arch/hardware.h>
> > > > >   #include <asm/ti-common/ti-aemif.h>
> > > > >   #define AEMIF_WAITCYCLE_CONFIG          (KS2_AEMIF_CNTRL_BASE + 0x4)
> > > > > diff --git a/drivers/soc/ti/keystone_serdes.c b/drivers/soc/ti/keystone_serdes.c
> > > > > index 2ece1a8f647..0e1bf8ff39d 100644
> > > > > --- a/drivers/soc/ti/keystone_serdes.c
> > > > > +++ b/drivers/soc/ti/keystone_serdes.c
> > > > > @@ -8,6 +8,7 @@
> > > > >   #include <errno.h>
> > > > >   #include <common.h>
> > > > > +#include <asm/io.h>
> > > > >   #include <asm/ti-common/keystone_serdes.h>
> > > > >   #include <linux/bitops.h>
> > > >
> > > > Is there going to be a follow-up to remove common.h from these files
> > > > then? Thanks.
> > >
> > > Yes I can take a look at that next.
> > >
> > > Wasn't there some automation around this? Dropping headers from common.h
> > > one at a time until it disappears IIRC.
> >
> > Simon has posted a python tool he wrote that partially did this, but I
> > wasn't quite happy with the results at the time. It might work out
> > better now, or on top of the series I posted to remove it from include/
> 
> My tool inserts #includes based on the symbols referenced in a file.
> For example, reference to strcpy() or memcmp() would result in the
> script adding '#include <linux/string.h>' to the top of the file.

But that's not how we typically work, nor is it how the kernel works. In
a semi-current kernel tree:
$ git grep -l '<linux/string.h>' *.c | wc -l
2177
$ git grep -l '<linux/string.h>' *.h | wc -l
146
$ git grep -l 'memset(' *.c | wc -l
6957

So most C files that call memset(..) get <linux/string.h> indirectly.
Which is why I'm trying to clean up the header files first.

> I still believe this is a reasonable approach. My original series to
> handle this was posted at [1] over 3 years ago. It foundered in part
> due to the number of files it touched, but I still believe the
> principle of 'include what you use' makes sense in U-Boot.
> 
> Other problems are that in a few cases it results in some ugly code,
> or misordering of includes
> 
> On the plus side, it gets us past this problem. We shouldn't make
> perfect an enemy of good and I am disappointed that we are still
> trying to get this over the line.

What makes this hard to get over the line is the less clear examples.
Dealing with <config.h> and CFG symbols gets pretty odd. And the failure
in https://source.denx.de/u-boot/u-boot/-/jobs/739662 was just really
odd.

-- 
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/20231121/c19d3367/attachment.sig>