Re: [PATCH v6 4/5] ARM: OMAP: gpmc: enable hwecc for AM33xx SoCs

[Daniel Mack <zonque@gmail.com>]

On 29.11.2012 21:32, Jon Hunter wrote:
> 
> On 11/29/2012 01:59 PM, Jon Hunter wrote:
>>
>> On 11/29/2012 10:01 AM, Daniel Mack wrote:
>>> The am33xx is capable of handling bch error correction modes, so
>>> enable that feature in the driver.
>>>
>>> Signed-off-by: Daniel Mack <zonque@gmail.com>
>>> ---
>>>  arch/arm/mach-omap2/gpmc-nand.c | 9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
>>> index f9f23a2..c8a72ba 100644
>>> --- a/arch/arm/mach-omap2/gpmc-nand.c
>>> +++ b/arch/arm/mach-omap2/gpmc-nand.c
>>> @@ -92,17 +92,18 @@ static int omap2_nand_gpmc_retime(
>>>  static bool gpmc_hwecc_bch_capable(enum omap_ecc ecc_opt)
>>>  {
>>>  	/* support only OMAP3 class */
>>> -	if (!cpu_is_omap34xx()) {
>>> +	if (!cpu_is_omap34xx() && !soc_is_am33xx()) {
>>>  		pr_err("BCH ecc is not supported on this CPU\n");
>>>  		return 0;
>>>  	}
>>>  
>>>  	/*
>>> -	 * For now, assume 4-bit mode is only supported on OMAP3630 ES1.x, x>=1.
>>> -	 * Other chips may be added if confirmed to work.
>>> +	 * For now, assume 4-bit mode is only supported on OMAP3630 ES1.x, x>=1
>>> +	 * and AM33xx derivates. Other chips may be added if confirmed to work.
>>>  	 */
>>>  	if ((ecc_opt == OMAP_ECC_BCH4_CODE_HW) &&
>>> -	    (!cpu_is_omap3630() || (GET_OMAP_REVISION() == 0))) {
>>> +	    (!cpu_is_omap3630() || (GET_OMAP_REVISION() == 0)) &&
>>> +	    (!soc_is_am33xx())) {
>>>  		pr_err("BCH 4-bit mode is not supported on this CPU\n");
>>>  		return 0;
>>>  	}
>>
>> Sorry I should have seen this earlier. Ideally, this type of thing
>> should be reflected by the device-tree/platform-data and we should get
>> away from these cpu_is_xxxx macros for hardware features (where we can).
>> Furthermore, we need to avoid including plat-omap/gpmc.h in drivers for
>> the single zImage work (I see the omap nand driver is including gpmc.h).
>>
>> Tony, should this be addressed now or can we live this for the minute
>> and fix-up later?
> 
> Actually, I see that you do read the ecc mode from DT, so is this really
> needed? It would be good to eliminate this.

The ecc mode is read from DT, right. But the gpmc bindings can be used
for many OMAP derivates in the future, and this check is there to make
sure the configured settings are supported by the hardware after all,
just like if the ecc mode would have been passed as static platform
data. So what is it exactly that you want to get rid of?

I agree though that we could solve this with via the of_device_id's data
pointer of the matching driver. But as you said yourself, this can well
be done later, and the problem here is that we still need the
cpu_is_xxx() macros for older platforms, right?


Thanks,
Daniel