diff mbox

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

Message ID 1354204892-22762-5-git-send-email-zonque@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Mack Nov. 29, 2012, 4:01 p.m. UTC
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(-)

Comments

Hunter, Jon Nov. 29, 2012, 7:59 p.m. UTC | #1
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?

Cheers
Jon
Hunter, Jon Nov. 29, 2012, 8:32 p.m. UTC | #2
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.

Cheers
Jon
Daniel Mack Nov. 29, 2012, 8:42 p.m. UTC | #3
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
Hunter, Jon Nov. 29, 2012, 8:59 p.m. UTC | #4
On 11/29/2012 02:42 PM, Daniel Mack wrote:
> 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?

The above function.

If there is a hardware bug that prevents us from using the hw-ecc mode
that is supported (which is the case for omap3630 es1.0), then we should
have a GPMC_ERRATA_xxx flag somewhere to indicate this and these errata
flags should be populated at probe.

> 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?

Yes, but we cannot/shouldn't use these cpu_is_xxx macros from within
driver code. Ok, here we can calling a platform function that is calling
the macro, but for single zImage we cannot do that either. We cannot
include platform headers in drivers for single zImage. We have to get
away from that. Therefore, such information needs to be passed by
platform data, device tree, etc.

I will let Tony decide on how he wants us to handle this.

Cheers
Jon
Daniel Mack Dec. 5, 2012, 1:04 p.m. UTC | #5
On 29.11.2012 21:59, Jon Hunter wrote:
> On 11/29/2012 02:42 PM, Daniel Mack wrote:
>> 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?
> 
> The above function.
> 
> If there is a hardware bug that prevents us from using the hw-ecc mode
> that is supported (which is the case for omap3630 es1.0), then we should
> have a GPMC_ERRATA_xxx flag somewhere to indicate this and these errata
> flags should be populated at probe.
> 
>> 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?
> 
> Yes, but we cannot/shouldn't use these cpu_is_xxx macros from within
> driver code. Ok, here we can calling a platform function that is calling
> the macro, but for single zImage we cannot do that either. We cannot
> include platform headers in drivers for single zImage. We have to get
> away from that. Therefore, such information needs to be passed by
> platform data, device tree, etc.
> 
> I will let Tony decide on how he wants us to handle this.

Any idea yet about this detail? The other small Documentation changes
you brought up are fixed in my tree and ready for resubmission.


Daniel
Tony Lindgren Dec. 5, 2012, 5:19 p.m. UTC | #6
* Daniel Mack <zonque@gmail.com> [121205 05:07]:
> On 29.11.2012 21:59, Jon Hunter wrote:
> > On 11/29/2012 02:42 PM, Daniel Mack wrote:
> >> 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?
> > 
> > The above function.
> > 
> > If there is a hardware bug that prevents us from using the hw-ecc mode
> > that is supported (which is the case for omap3630 es1.0), then we should
> > have a GPMC_ERRATA_xxx flag somewhere to indicate this and these errata
> > flags should be populated at probe.
> > 
> >> 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?
> > 
> > Yes, but we cannot/shouldn't use these cpu_is_xxx macros from within
> > driver code. Ok, here we can calling a platform function that is calling
> > the macro, but for single zImage we cannot do that either. We cannot
> > include platform headers in drivers for single zImage. We have to get
> > away from that. Therefore, such information needs to be passed by
> > platform data, device tree, etc.
> > 
> > I will let Tony decide on how he wants us to handle this.
> 
> Any idea yet about this detail? The other small Documentation changes
> you brought up are fixed in my tree and ready for resubmission.

The plat/cpu.h file will disappear after the merge window, which means
omap2+ related drivers cannot use cpu_is_omap macros.

For legacy booting systems, this flag should be just passed in the
platform_data from the platform init code. Then device tree can
deal with it based on the compatible flag.

Regards,

Tony
Daniel Mack Dec. 5, 2012, 5:26 p.m. UTC | #7
Hi Tony,

On 05.12.2012 18:19, Tony Lindgren wrote:
> * Daniel Mack <zonque@gmail.com> [121205 05:07]:
>> On 29.11.2012 21:59, Jon Hunter wrote:
>>> On 11/29/2012 02:42 PM, Daniel Mack wrote:
>>>> 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?
>>>
>>> The above function.
>>>
>>> If there is a hardware bug that prevents us from using the hw-ecc mode
>>> that is supported (which is the case for omap3630 es1.0), then we should
>>> have a GPMC_ERRATA_xxx flag somewhere to indicate this and these errata
>>> flags should be populated at probe.
>>>
>>>> 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?
>>>
>>> Yes, but we cannot/shouldn't use these cpu_is_xxx macros from within
>>> driver code. Ok, here we can calling a platform function that is calling
>>> the macro, but for single zImage we cannot do that either. We cannot
>>> include platform headers in drivers for single zImage. We have to get
>>> away from that. Therefore, such information needs to be passed by
>>> platform data, device tree, etc.
>>>
>>> I will let Tony decide on how he wants us to handle this.
>>
>> Any idea yet about this detail? The other small Documentation changes
>> you brought up are fixed in my tree and ready for resubmission.
> 
> The plat/cpu.h file will disappear after the merge window, which means
> omap2+ related drivers cannot use cpu_is_omap macros.
>
> For legacy booting systems, this flag should be just passed in the
> platform_data from the platform init code. Then device tree can
> deal with it based on the compatible flag.
>

Ok, thanks for explaining.

Does that mean this patch series should be postponed until after the
merge window and then build upon that change or should we merge the
patch in question here as is and then care for the cleanups after the
window?

I can also rebase this set on top of the removal patches if they exist
already somewhere.


Daniel
Tony Lindgren Dec. 5, 2012, 5:41 p.m. UTC | #8
* Daniel Mack <zonque@gmail.com> [121205 09:29]:
> On 05.12.2012 18:19, Tony Lindgren wrote:
> > 
> > The plat/cpu.h file will disappear after the merge window, which means
> > omap2+ related drivers cannot use cpu_is_omap macros.
> >
> > For legacy booting systems, this flag should be just passed in the
> > platform_data from the platform init code. Then device tree can
> > deal with it based on the compatible flag.
> >
> 
> Ok, thanks for explaining.
> 
> Does that mean this patch series should be postponed until after the
> merge window and then build upon that change or should we merge the
> patch in question here as is and then care for the cleanups after the
> window?

Well to me it seems that you only have cpu_is_omap usage in
arch/arm/mach-omap2/gpmc-nand.c, which will be OK. Only the code
under drivers/* needs to be fixed for that. So your patches may
be OK, but..

> I can also rebase this set on top of the removal patches if they exist
> already somewhere.

..please check that your patches work with current linux next.

It's too late to queue thing for v3.8 merge window at this point,
as we want the patches sitting in linux next for a week at least
before they get pulled.

And we still need the acks for the device tree binding as well.

So let's plan on queueing these after -rc1.

Regards,

Tony
Hunter, Jon Dec. 5, 2012, 6:19 p.m. UTC | #9
On 12/05/2012 11:41 AM, Tony Lindgren wrote:
> * Daniel Mack <zonque@gmail.com> [121205 09:29]:
>> On 05.12.2012 18:19, Tony Lindgren wrote:
>>>
>>> The plat/cpu.h file will disappear after the merge window, which means
>>> omap2+ related drivers cannot use cpu_is_omap macros.
>>>
>>> For legacy booting systems, this flag should be just passed in the
>>> platform_data from the platform init code. Then device tree can
>>> deal with it based on the compatible flag.
>>>
>>
>> Ok, thanks for explaining.
>>
>> Does that mean this patch series should be postponed until after the
>> merge window and then build upon that change or should we merge the
>> patch in question here as is and then care for the cleanups after the
>> window?
> 
> Well to me it seems that you only have cpu_is_omap usage in
> arch/arm/mach-omap2/gpmc-nand.c, which will be OK. Only the code
> under drivers/* needs to be fixed for that. So your patches may
> be OK, but..

The real problem here is that drivers/mtd/nand/omap2.c is including
plat/gpmc.h and calling a function in arch/arm/mach-omap/gpmc-nand.c. So
although Daniel's patches are not introducing any new problems for
single zImage, they do highlight a problem that we have with the omap2
nand driver that needs to be addressed for single zImage.

So either we fix this now or after merging Daniel's changes. Either is
fine with me.

Cheers
Jon
Tony Lindgren Dec. 5, 2012, 6:33 p.m. UTC | #10
* Jon Hunter <jon-hunter@ti.com> [121205 10:22]:
> 
> On 12/05/2012 11:41 AM, Tony Lindgren wrote:
> > * Daniel Mack <zonque@gmail.com> [121205 09:29]:
> >> On 05.12.2012 18:19, Tony Lindgren wrote:
> >>>
> >>> The plat/cpu.h file will disappear after the merge window, which means
> >>> omap2+ related drivers cannot use cpu_is_omap macros.
> >>>
> >>> For legacy booting systems, this flag should be just passed in the
> >>> platform_data from the platform init code. Then device tree can
> >>> deal with it based on the compatible flag.
> >>>
> >>
> >> Ok, thanks for explaining.
> >>
> >> Does that mean this patch series should be postponed until after the
> >> merge window and then build upon that change or should we merge the
> >> patch in question here as is and then care for the cleanups after the
> >> window?
> > 
> > Well to me it seems that you only have cpu_is_omap usage in
> > arch/arm/mach-omap2/gpmc-nand.c, which will be OK. Only the code
> > under drivers/* needs to be fixed for that. So your patches may
> > be OK, but..
> 
> The real problem here is that drivers/mtd/nand/omap2.c is including
> plat/gpmc.h and calling a function in arch/arm/mach-omap/gpmc-nand.c. So
> although Daniel's patches are not introducing any new problems for
> single zImage, they do highlight a problem that we have with the omap2
> nand driver that needs to be addressed for single zImage.
>
> So either we fix this now or after merging Daniel's changes. Either is
> fine with me.

Ah I see. Yes it would be good to fix that issue first to avoid
adding any new blockers for multiplatform build.

Regards,

Tony
Daniel Mack Dec. 5, 2012, 6:40 p.m. UTC | #11
On 05.12.2012 19:33, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [121205 10:22]:
>>
>> On 12/05/2012 11:41 AM, Tony Lindgren wrote:
>>> * Daniel Mack <zonque@gmail.com> [121205 09:29]:
>>>> On 05.12.2012 18:19, Tony Lindgren wrote:
>>>>>
>>>>> The plat/cpu.h file will disappear after the merge window, which means
>>>>> omap2+ related drivers cannot use cpu_is_omap macros.
>>>>>
>>>>> For legacy booting systems, this flag should be just passed in the
>>>>> platform_data from the platform init code. Then device tree can
>>>>> deal with it based on the compatible flag.
>>>>>
>>>>
>>>> Ok, thanks for explaining.
>>>>
>>>> Does that mean this patch series should be postponed until after the
>>>> merge window and then build upon that change or should we merge the
>>>> patch in question here as is and then care for the cleanups after the
>>>> window?
>>>
>>> Well to me it seems that you only have cpu_is_omap usage in
>>> arch/arm/mach-omap2/gpmc-nand.c, which will be OK. Only the code
>>> under drivers/* needs to be fixed for that. So your patches may
>>> be OK, but..
>>
>> The real problem here is that drivers/mtd/nand/omap2.c is including
>> plat/gpmc.h and calling a function in arch/arm/mach-omap/gpmc-nand.c. So
>> although Daniel's patches are not introducing any new problems for
>> single zImage, they do highlight a problem that we have with the omap2
>> nand driver that needs to be addressed for single zImage.
>>
>> So either we fix this now or after merging Daniel's changes. Either is
>> fine with me.
> 
> Ah I see. Yes it would be good to fix that issue first to avoid
> adding any new blockers for multiplatform build.

Already fixed by Afzal here:

https://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap.git;a=commitdiff;h=2ef9f3dd

... which is also in linux-next.



Daniel
Daniel Mack Dec. 5, 2012, 6:43 p.m. UTC | #12
On 05.12.2012 18:41, Tony Lindgren wrote:
> * Daniel Mack <zonque@gmail.com> [121205 09:29]:
>> On 05.12.2012 18:19, Tony Lindgren wrote:
>>>
>>> The plat/cpu.h file will disappear after the merge window, which means
>>> omap2+ related drivers cannot use cpu_is_omap macros.
>>>
>>> For legacy booting systems, this flag should be just passed in the
>>> platform_data from the platform init code. Then device tree can
>>> deal with it based on the compatible flag.
>>>
>>
>> Ok, thanks for explaining.
>>
>> Does that mean this patch series should be postponed until after the
>> merge window and then build upon that change or should we merge the
>> patch in question here as is and then care for the cleanups after the
>> window?
> 
> Well to me it seems that you only have cpu_is_omap usage in
> arch/arm/mach-omap2/gpmc-nand.c, which will be OK. Only the code
> under drivers/* needs to be fixed for that. So your patches may
> be OK, but..
> 
>> I can also rebase this set on top of the removal patches if they exist
>> already somewhere.
> 
> ..please check that your patches work with current linux next.
> 
> It's too late to queue thing for v3.8 merge window at this point,
> as we want the patches sitting in linux next for a week at least
> before they get pulled.
> 
> And we still need the acks for the device tree binding as well.

Ok, I'll resubmit a v7 with the nits fixed that Jon pointed out, and
then hope for the maintainer's Acks.

> So let's plan on queueing these after -rc1.

No problem really. The only thing is that I'll be on vacation when the
merge window closes, hence I'd appreciate if you could already queue
them up on your side, so they'll be taken care for :)


Daniel
Hunter, Jon Dec. 5, 2012, 7:11 p.m. UTC | #13
On 12/05/2012 12:40 PM, Daniel Mack wrote:
> On 05.12.2012 19:33, Tony Lindgren wrote:
>> * Jon Hunter <jon-hunter@ti.com> [121205 10:22]:
>>>
>>> On 12/05/2012 11:41 AM, Tony Lindgren wrote:
>>>> * Daniel Mack <zonque@gmail.com> [121205 09:29]:
>>>>> On 05.12.2012 18:19, Tony Lindgren wrote:
>>>>>>
>>>>>> The plat/cpu.h file will disappear after the merge window, which means
>>>>>> omap2+ related drivers cannot use cpu_is_omap macros.
>>>>>>
>>>>>> For legacy booting systems, this flag should be just passed in the
>>>>>> platform_data from the platform init code. Then device tree can
>>>>>> deal with it based on the compatible flag.
>>>>>>
>>>>>
>>>>> Ok, thanks for explaining.
>>>>>
>>>>> Does that mean this patch series should be postponed until after the
>>>>> merge window and then build upon that change or should we merge the
>>>>> patch in question here as is and then care for the cleanups after the
>>>>> window?
>>>>
>>>> Well to me it seems that you only have cpu_is_omap usage in
>>>> arch/arm/mach-omap2/gpmc-nand.c, which will be OK. Only the code
>>>> under drivers/* needs to be fixed for that. So your patches may
>>>> be OK, but..
>>>
>>> The real problem here is that drivers/mtd/nand/omap2.c is including
>>> plat/gpmc.h and calling a function in arch/arm/mach-omap/gpmc-nand.c. So
>>> although Daniel's patches are not introducing any new problems for
>>> single zImage, they do highlight a problem that we have with the omap2
>>> nand driver that needs to be addressed for single zImage.
>>>
>>> So either we fix this now or after merging Daniel's changes. Either is
>>> fine with me.
>>
>> Ah I see. Yes it would be good to fix that issue first to avoid
>> adding any new blockers for multiplatform build.
> 
> Already fixed by Afzal here:
> 
> https://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap.git;a=commitdiff;h=2ef9f3dd
> 
> ... which is also in linux-next.

Great! So we can drop this change.

Cheers
Jon
diff mbox

Patch

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;
 	}