[PATCH] nand: omap2: fix building with CONFIG_MTD_NAND_OMAP_BCH=m

[PATCH] nand: omap2: fix building with CONFIG_MTD_NAND_OMAP_BCH=m

[Arnd Bergmann]

If the OMAP nand driver is built-in but the BCH driver is a module,
we get a link-time error:

drivers/built-in.o: In function `omap_elm_correct_data':
:(.text+0x174e88): undefined reference to `elm_decode_bch_error_page'
drivers/built-in.o: In function `omap_nand_probe':
:(.text+0x175b48): undefined reference to `elm_config'

There are two possible ways to deal with this, either prevent that
configuration in Kconfig or make sure we don't reference the ELM
driver in this case.

This patch picks the second approach, which makes it possible to
use the ELM driver in other modules while still having the OMAP
NAND driver built-in.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 93af53b8633c ("nand: omap2: Remove horrible ifdefs to fix module probe")

diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
index b8686c00f15f..cbca66ce7c10 100644
--- a/include/linux/platform_data/elm.h
+++ b/include/linux/platform_data/elm.h
@@ -42,7 +42,7 @@ struct elm_errorvec {
 	int error_loc[16];
 };
 
-#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)
+#if defined(CONFIG_MTD_NAND_OMAP_BCH) || (defined(CONFIG_MTD_NAND_OMAP_BCH_MODULE) && defined(MODULE))
 void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
 		struct elm_errorvec *err_vec);
 int elm_config(struct device *dev, enum bch_ecc bch_type,

Re: [PATCH] nand: omap2: fix building with CONFIG_MTD_NAND_OMAP_BCH=m

[Roger Quadros]

Hi Arnd & Brian,

On 09/30/2014 03:04 PM, Arnd Bergmann wrote:
> If the OMAP nand driver is built-in but the BCH driver is a module,
> we get a link-time error:
> 
> drivers/built-in.o: In function `omap_elm_correct_data':
> :(.text+0x174e88): undefined reference to `elm_decode_bch_error_page'
> drivers/built-in.o: In function `omap_nand_probe':
> :(.text+0x175b48): undefined reference to `elm_config'
> 
> There are two possible ways to deal with this, either prevent that
> configuration in Kconfig or make sure we don't reference the ELM
> driver in this case.
> 
> This patch picks the second approach, which makes it possible to
> use the ELM driver in other modules while still having the OMAP
> NAND driver built-in.

With this patch NAND probe on DRA7xx fails like so

[    2.077313] omap-gpmc 50000000.gpmc: GPMC revision 6.0
[    2.083842] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xca
[    2.090524] nand: Micron MT29F2G16ABAEAWP
[    2.094728] nand: 256MiB, SLC, page size: 2048, OOB size: 64
[    2.100745] nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme
[    2.109764] omap2-nand: probe of omap2-nand.0 failed with error -38

OMAP NAND driver is the only user of the ELM module and we want it
to be usable in all possible configurations when enabled.

Let's pick either one of the below patches instead

http://article.gmane.org/gmane.linux.ports.arm.omap/118488
OR
http://article.gmane.org/gmane.linux.ports.arm.omap/118847

cheers,
-roger

> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 93af53b8633c ("nand: omap2: Remove horrible ifdefs to fix module probe")
> 
> diff --git a/include/linux/platform_data/elm.h b/include/linux/platform_data/elm.h
> index b8686c00f15f..cbca66ce7c10 100644
> --- a/include/linux/platform_data/elm.h
> +++ b/include/linux/platform_data/elm.h
> @@ -42,7 +42,7 @@ struct elm_errorvec {
>  	int error_loc[16];
>  };
>  
> -#if IS_ENABLED(CONFIG_MTD_NAND_OMAP_BCH)
> +#if defined(CONFIG_MTD_NAND_OMAP_BCH) || (defined(CONFIG_MTD_NAND_OMAP_BCH_MODULE) && defined(MODULE))
>  void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc,
>  		struct elm_errorvec *err_vec);
>  int elm_config(struct device *dev, enum bch_ecc bch_type,
> 

Re: [PATCH] nand: omap2: fix building with CONFIG_MTD_NAND_OMAP_BCH=m

[Arnd Bergmann]

On Wednesday 01 October 2014 12:32:09 Roger Quadros wrote:
> 
> With this patch NAND probe on DRA7xx fails like so
> 
> [    2.077313] omap-gpmc 50000000.gpmc: GPMC revision 6.0
> [    2.083842] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xca
> [    2.090524] nand: Micron MT29F2G16ABAEAWP
> [    2.094728] nand: 256MiB, SLC, page size: 2048, OOB size: 64
> [    2.100745] nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme
> [    2.109764] omap2-nand: probe of omap2-nand.0 failed with error -38
> 
> OMAP NAND driver is the only user of the ELM module and we want it
> to be usable in all possible configurations when enabled.

I don't understand. Is the BCH driver optional or not?

The help text says:

          This config enables the ELM hardware engine, which can be used to
          locate and correct errors when using BCH ECC scheme. This offloads
          the cpu from doing ECC error searching and correction. However some
          legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
          so they should not enable this config symbol.

which adds more to the confusion. The help text sounds like everything should
work even if ELM is disabled (which contradicts your finding above) but you
must not enable the driver if you are on an older machine (which would
break multiplatform builds).

> Let's pick either one of the below patches instead
> 
> http://article.gmane.org/gmane.linux.ports.arm.omap/118488

This doesn't let you have the BCH driver as a module, which seems
wrong.

> http://article.gmane.org/gmane.linux.ports.arm.omap/118847

Looks good, although I think you can simplify this to

config MTD_NAND_OMAP_BCH_BUILD
	def_tristate MTD_NAND_OMAP2 && MTD_NAND_OMAP_BCH

which makes it 'm' if MTD_NAND_OMAP_BCH is set to m.

	Arnd

Re: [PATCH] nand: omap2: fix building with CONFIG_MTD_NAND_OMAP_BCH=m

[Roger Quadros]

On 10/01/2014 12:56 PM, Arnd Bergmann wrote:
> On Wednesday 01 October 2014 12:32:09 Roger Quadros wrote:
>>
>> With this patch NAND probe on DRA7xx fails like so
>>
>> [    2.077313] omap-gpmc 50000000.gpmc: GPMC revision 6.0
>> [    2.083842] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xca
>> [    2.090524] nand: Micron MT29F2G16ABAEAWP
>> [    2.094728] nand: 256MiB, SLC, page size: 2048, OOB size: 64
>> [    2.100745] nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme
>> [    2.109764] omap2-nand: probe of omap2-nand.0 failed with error -38
>>
>> OMAP NAND driver is the only user of the ELM module and we want it
>> to be usable in all possible configurations when enabled.
> 
> I don't understand. Is the BCH driver optional or not?
> 
It is optional. If it is disabled we error out on platforms that have
the ELM IP. But if it is enabled, we don't want to fail probe on such platforms.

> The help text says:
> 
>           This config enables the ELM hardware engine, which can be used to
>           locate and correct errors when using BCH ECC scheme. This offloads
>           the cpu from doing ECC error searching and correction. However some
>           legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
>           so they should not enable this config symbol.
> 
> which adds more to the confusion. The help text sounds like everything should
> work even if ELM is disabled (which contradicts your finding above) but you
> must not enable the driver if you are on an older machine (which would
> break multiplatform builds).

I understand. We need to update that description. It is no longer the case that
we have to disable ELM engine to work on older platforms.

> 
>> Let's pick either one of the below patches instead
>>
>> http://article.gmane.org/gmane.linux.ports.arm.omap/118488
> 
> This doesn't let you have the BCH driver as a module, which seems
> wrong.
> 
>> http://article.gmane.org/gmane.linux.ports.arm.omap/118847
> 
> Looks good, although I think you can simplify this to
> 
> config MTD_NAND_OMAP_BCH_BUILD
> 	def_tristate MTD_NAND_OMAP2 && MTD_NAND_OMAP_BCH
> 
> which makes it 'm' if MTD_NAND_OMAP_BCH is set to m.

Right. I will resend this patch with this fixup
and update the Kconfig description as well.

cheers,
-roger

Re: [PATCH] nand: omap2: fix building with CONFIG_MTD_NAND_OMAP_BCH=m

[Arnd Bergmann]

On Wednesday 01 October 2014 13:07:51 Roger Quadros wrote:
> On 10/01/2014 12:56 PM, Arnd Bergmann wrote:
> > On Wednesday 01 October 2014 12:32:09 Roger Quadros wrote:
> >>
> >> With this patch NAND probe on DRA7xx fails like so
> >>
> >> [    2.077313] omap-gpmc 50000000.gpmc: GPMC revision 6.0
> >> [    2.083842] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xca
> >> [    2.090524] nand: Micron MT29F2G16ABAEAWP
> >> [    2.094728] nand: 256MiB, SLC, page size: 2048, OOB size: 64
> >> [    2.100745] nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme
> >> [    2.109764] omap2-nand: probe of omap2-nand.0 failed with error -38
> >>
> >> OMAP NAND driver is the only user of the ELM module and we want it
> >> to be usable in all possible configurations when enabled.
> > 
> > I don't understand. Is the BCH driver optional or not?
> > 
> It is optional. If it is disabled we error out on platforms that have
> the ELM IP. But if it is enabled, we don't want to fail probe on such platforms.

Wouldn't it be better to treat the absence of the ELM driver the
same way as the absence of the ELM hardware?

> >> Let's pick either one of the below patches instead
> >>
> >> http://article.gmane.org/gmane.linux.ports.arm.omap/118488
> > 
> > This doesn't let you have the BCH driver as a module, which seems
> > wrong.
> > 
> >> http://article.gmane.org/gmane.linux.ports.arm.omap/118847
> > 
> > Looks good, although I think you can simplify this to
> > 
> > config MTD_NAND_OMAP_BCH_BUILD
> >       def_tristate MTD_NAND_OMAP2 && MTD_NAND_OMAP_BCH
> > 
> > which makes it 'm' if MTD_NAND_OMAP_BCH is set to m.
> 
> Right. I will resend this patch with this fixup
> and update the Kconfig description as well.

Ok, thanks!

	Arnd

Re: [PATCH] nand: omap2: fix building with CONFIG_MTD_NAND_OMAP_BCH=m

[Roger Quadros]

On 10/01/2014 01:13 PM, Arnd Bergmann wrote:
> On Wednesday 01 October 2014 13:07:51 Roger Quadros wrote:
>> On 10/01/2014 12:56 PM, Arnd Bergmann wrote:
>>> On Wednesday 01 October 2014 12:32:09 Roger Quadros wrote:
>>>>
>>>> With this patch NAND probe on DRA7xx fails like so
>>>>
>>>> [    2.077313] omap-gpmc 50000000.gpmc: GPMC revision 6.0
>>>> [    2.083842] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xca
>>>> [    2.090524] nand: Micron MT29F2G16ABAEAWP
>>>> [    2.094728] nand: 256MiB, SLC, page size: 2048, OOB size: 64
>>>> [    2.100745] nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme
>>>> [    2.109764] omap2-nand: probe of omap2-nand.0 failed with error -38
>>>>
>>>> OMAP NAND driver is the only user of the ELM module and we want it
>>>> to be usable in all possible configurations when enabled.
>>>
>>> I don't understand. Is the BCH driver optional or not?
>>>
>> It is optional. If it is disabled we error out on platforms that have
>> the ELM IP. But if it is enabled, we don't want to fail probe on such platforms.
> 
> Wouldn't it be better to treat the absence of the ELM driver the
> same way as the absence of the ELM hardware?

By absence of ELM driver you meant not enabled right?

We will have to use Software BCH library in that case and is not optimal.
That again is a Kconfig option that needs to be enabled if ELM driver is disabled.

cheers,
-roger

Re: [PATCH] nand: omap2: fix building with CONFIG_MTD_NAND_OMAP_BCH=m

[Ezequiel Garcia]

On 01 Oct 12:13 PM, Arnd Bergmann wrote:
> On Wednesday 01 October 2014 13:07:51 Roger Quadros wrote:
> > On 10/01/2014 12:56 PM, Arnd Bergmann wrote:
> > > On Wednesday 01 October 2014 12:32:09 Roger Quadros wrote:
> > >>
> > >> With this patch NAND probe on DRA7xx fails like so
> > >>
> > >> [    2.077313] omap-gpmc 50000000.gpmc: GPMC revision 6.0
> > >> [    2.083842] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xca
> > >> [    2.090524] nand: Micron MT29F2G16ABAEAWP
> > >> [    2.094728] nand: 256MiB, SLC, page size: 2048, OOB size: 64
> > >> [    2.100745] nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme
> > >> [    2.109764] omap2-nand: probe of omap2-nand.0 failed with error -38
> > >>
> > >> OMAP NAND driver is the only user of the ELM module and we want it
> > >> to be usable in all possible configurations when enabled.
> > > 
> > > I don't understand. Is the BCH driver optional or not?
> > > 
> > It is optional. If it is disabled we error out on platforms that have
> > the ELM IP. But if it is enabled, we don't want to fail probe on such platforms.
> 
> Wouldn't it be better to treat the absence of the ELM driver the
> same way as the absence of the ELM hardware?
> 

Indeed, it would be better to provide a fallback to the non-ELM behavior
if *anything* prevents initializing it.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH] nand: omap2: fix building with CONFIG_MTD_NAND_OMAP_BCH=m

[Ezequiel Garcia]

On 01 Oct 11:56 AM, Arnd Bergmann wrote:
> On Wednesday 01 October 2014 12:32:09 Roger Quadros wrote:
> > 
> > With this patch NAND probe on DRA7xx fails like so
> > 
> > [    2.077313] omap-gpmc 50000000.gpmc: GPMC revision 6.0
> > [    2.083842] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xca
> > [    2.090524] nand: Micron MT29F2G16ABAEAWP
> > [    2.094728] nand: 256MiB, SLC, page size: 2048, OOB size: 64
> > [    2.100745] nand: using OMAP_ECC_BCH8_CODE_HW ECC scheme
> > [    2.109764] omap2-nand: probe of omap2-nand.0 failed with error -38
> > 
> > OMAP NAND driver is the only user of the ELM module and we want it
> > to be usable in all possible configurations when enabled.
> 
> I don't understand. Is the BCH driver optional or not?
> 
> The help text says:
> 
>           This config enables the ELM hardware engine, which can be used to
>           locate and correct errors when using BCH ECC scheme. This offloads
>           the cpu from doing ECC error searching and correction. However some
>           legacy OMAP families like OMAP2xxx, OMAP3xxx do not have ELM engine
>           so they should not enable this config symbol.
> 
> which adds more to the confusion. The help text sounds like everything should
> work even if ELM is disabled (which contradicts your finding above) but you
> must not enable the driver if you are on an older machine (which would
> break multiplatform builds).
> 
> > Let's pick either one of the below patches instead
> > 
> > http://article.gmane.org/gmane.linux.ports.arm.omap/118488
> 
> This doesn't let you have the BCH driver as a module, which seems
> wrong.
> 
> > http://article.gmane.org/gmane.linux.ports.arm.omap/118847
> 
> Looks good, although I think you can simplify this to
> 
> config MTD_NAND_OMAP_BCH_BUILD
> 	def_tristate MTD_NAND_OMAP2 && MTD_NAND_OMAP_BCH
> 
> which makes it 'm' if MTD_NAND_OMAP_BCH is set to m.
> 

Just to clarify, you are talking about this one being the correct one,
right?

http://article.gmane.org/gmane.linux.ports.arm.omap/118488
"[PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module"

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Re: [PATCH] nand: omap2: fix building with CONFIG_MTD_NAND_OMAP_BCH=m

[Arnd Bergmann]

On Wednesday 01 October 2014 07:24:19 Ezequiel Garcia wrote:
> > 
> > config MTD_NAND_OMAP_BCH_BUILD
> >       def_tristate MTD_NAND_OMAP2 && MTD_NAND_OMAP_BCH
> > 
> > which makes it 'm' if MTD_NAND_OMAP_BCH is set to m.
> > 
> 
> Just to clarify, you are talking about this one being the correct one,
> right?
> 
> http://article.gmane.org/gmane.linux.ports.arm.omap/118488
> "[PATCH 3/3] mtd: nand: Force omap_elm to be built as a module if omap2_nand is a module"
> 
> 

Yes. The four lines you have in there

+config MTD_NAND_OMAP_BCH_BUILD
+	tristate
+	depends on MTD_NAND_OMAP2
+	default m if MTD_NAND_OMAP2=m && MTD_NAND_OMAP_BCH
+	default y if MTD_NAND_OMAP2=y && MTD_NAND_OMAP_BCH

should be entirely equivalent to the two lines I wrote above, so my version
is just a readability change, functionally they both are correct as far as
I can tell.

	Arnd