[PATCH 4/4] mtd: nand: raw: atmel: Remove duplicate definitions

[Alexander Dahl]

Hello Eugen,

Am Tue, Dec 19, 2023 at 04:32:07PM +0200 schrieb Eugen Hristev:
> On 12/12/23 18:04, Alexander Dahl wrote:
> > These removed definitions were specific to some sam9 SoCs, but not
> > generic over all at91 SoCs.  The correct SoC specific definitions for
> > ATMEL_BASE_PMECC are spread over different header files in
> > arch/arm/mach-at91/include/mach directory.
> > 
> > Fixes a build error on a custon board based on SAMA5D2:
> > 
> >     Building current source for 73 boards (16 threads, 1 job per thread)
> >            arm:  +   vera2
> >     +drivers/mtd/nand/raw/atmel/pmecc.c:819: warning: "ATMEL_BASE_PMECC" redefined
> >     +  819 | #define ATMEL_BASE_PMECC        0xffffe000
> >     +      |
> >     +In file included from include/configs/vera2.h:11,
> >     +                 from include/config.h:3,
> >     +                 from include/linux/mtd/rawnand.h:16,
> >     +                 from drivers/mtd/nand/raw/atmel/pmecc.c:44:
> >     +include/asm/arch/sama5d2.h:171: note: this is the location of the previous definition
> >     +  171 | #define ATMEL_BASE_PMECC        (ATMEL_BASE_HSMC + 0x70)
> >     +drivers/mtd/nand/raw/atmel/pmecc.c:820: warning: "ATMEL_BASE_PMERRLOC" redefined
> >     +  820 | #define ATMEL_BASE_PMERRLOC     0xffffe600
> >     +include/asm/arch/sama5d2.h:172: note: this is the location of the previous definition
> >     +  172 | #define ATMEL_BASE_PMERRLOC     (ATMEL_BASE_HSMC + 0x500)
> > 
> > Fixes: a490e1b7c017 ("nand: atmel: Add pmecc driver")
> > Signed-off-by: Alexander Dahl <ada at thorsis.com>
> > ---
> >  drivers/mtd/nand/raw/atmel/pmecc.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/atmel/pmecc.c b/drivers/mtd/nand/raw/atmel/pmecc.c
> > index e2e3f1ee6b5..51f6bd2e65b 100644
> > --- a/drivers/mtd/nand/raw/atmel/pmecc.c
> > +++ b/drivers/mtd/nand/raw/atmel/pmecc.c
> > @@ -816,9 +816,6 @@ int atmel_pmecc_wait_rdy(struct atmel_pmecc_user *user)
> >  }
> >  EXPORT_SYMBOL_GPL(atmel_pmecc_wait_rdy);
> >  
> > -#define ATMEL_BASE_PMECC	0xffffe000
> > -#define ATMEL_BASE_PMERRLOC	0xffffe600
> > -
> >  static struct atmel_pmecc *
> >  atmel_pmecc_create(struct udevice *dev,
> >  		   const struct atmel_pmecc_caps *caps,
> 
> 
> Hi Alexander,
> 
> What happens if we try to select and build this driver without
> sama5d2/sama5d3/sama5d4/sam9x5/sam9x60 ?
> Is it even possible ?

It is not possible on non-at91 boards because <mach/at91_sfr.h> is
included so you need to select arch at91.  But it is possible on older
sam9 boards like sam9260, sam9g20 and the like.  Tried that by using
at91sam9g20ek_nandflash_defconfig as a base, then disabling
CONFIG_NAND_ATMEL, then enabling CONFIG_DM_NAND_ATMEL (which activates
build of drivers/mtd/nand/raw/atmel/pmecc.c), then enabling some more
missing dependencies (CONFIG_MFD_ATMEL_SMC, CONFIG_REGMAP,
CONFIG_SYSCON) and then it builds just fine.

(btw: should those be selected or implied by CONFIG_DM_NAND_ATMEL in
'drivers/mtd/nand/raw/Kconfig' then in another patch or is this stuff
rather loose coupled?)

> Because it appears these defines are done for those SoCs in their
> mach header, but the driver uses them in any situation. 

I don't think so.  There are two drivers, the old one is
'drivers/mtd/nand/raw/atmel_nand.c' selected by CONFIG_NAND_ATMEL
while the new one is 'drivers/mtd/nand/raw/atmel/nand-controller.c'
selected by CONFIG_DM_NAND_ATMEL (note the extra sub-directory).  Only
the new one leads to building the file in question.  The symbols
removed from pmecc.c are not used in pmecc.c but only in the old
driver.  The scope of those removed symbols would have been in pmecc.c
only however, they are pointless at the place where they are currently
defined.  (Unless someone would #include that .c file, but that
seems rather unusual.)

And the part where the symbols of the same name are used is
conditionally built only if CONFIG_ATMEL_NAND_HW_PMECC is set.  From a
quick glance I would say that one is set only for newer boards, those
families you mentioned below.  But as said, all this is for the old
driver.

> And currently, these warnings are being ignored if the driver is
> built with sama5d2/sama5d3/sama5d4/sam9x5/sam9x60 ? 

The warning only popped up for a custom board build, where I include
<asm/arch/sama5d2.h> in file 'include/configs/myboard.h' and keep some
old definitions for the old driver and then switching from the old to
the new driver.  I did not see it for any mainline at91 defconfig.
Nevertheless I consider those defines wrong in that place.

> or the
> driver actually isn't even built with these platforms at all ?

It depends on the (def)config.  Currently in mainline U-Boot to my
knowledge the new driver is only used by sam9x60 and sam7g*something,
but it was ported from Linux and it should support all at91 SoCs.  I
think it is just a matter of who migrates all those boards to the new
driver?

> So, I guess it would be safer to do #ifndef ATMEL_BASE_PMECC, #define ...
> 
> Or, we could fix the driver to get these base addresses from the platform/DT ?

As said above.  The new driver does not these definitions at all, but
already gets these addresses from DT.  See sam9x60 curiosity for
example.

Greets
Alex