diff mbox

[10/12] mtd: nand: davinci: don't set timings if AEMIF is used

Message ID 4F5844B3A985794BA902E12C070812375F8D53@DNCE04.ent.ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ivan Khoronzhuk Nov. 11, 2013, 5:10 p.m. UTC
If Davinci AEMIF is used we don't need to set timings and bus width.
It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

--
1.7.9.5

Comments

Santosh Shilimkar Nov. 12, 2013, 4:13 p.m. UTC | #1
On Monday 11 November 2013 12:10 PM, Khoronzhuk, Ivan wrote:
> If Davinci AEMIF is used we don't need to set timings and bus width.
> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index 4705214..879e915 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>                 goto err_clk_enable;
>         }
> 
> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>
Instead above #if, just use a variable.
	bool aemif = IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF) and then skip
the below code. #if block in the middle of the code looks ugly.

Other than that patch looks fine to me.

Regards,
Santosh
Sekhar Nori Nov. 13, 2013, 5:02 a.m. UTC | #2
On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
> If Davinci AEMIF is used we don't need to set timings and bus width.
> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index 4705214..879e915 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>                 goto err_clk_enable;
>         }
> 
> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)

This is a hack! Just because AEMIF driver is enabled, it does not
guarantee that the timings have been setup by it. Instead of configuring
timings in two drivers, why not just convert everyone over to use the
new driver. Dont worry about breaking old platforms, I will help test
and ack them.

Thanks,
Sekhar
Santosh Shilimkar Nov. 13, 2013, 2:14 p.m. UTC | #3
On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>> If Davinci AEMIF is used we don't need to set timings and bus width.
>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>> index 4705214..879e915 100644
>> --- a/drivers/mtd/nand/davinci_nand.c
>> +++ b/drivers/mtd/nand/davinci_nand.c
>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>                 goto err_clk_enable;
>>         }
>>
>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
> 
> This is a hack! Just because AEMIF driver is enabled, it does not
> guarantee that the timings have been setup by it. Instead of configuring
> timings in two drivers, why not just convert everyone over to use the
> new driver. Dont worry about breaking old platforms, I will help test
> and ack them.
> 
How about you take a stab at and convert the DaVinci code over to make
use of new driver. We are trying to re-use as much as possible from the
common drivers and also making an option so that the DaVinci arch can
move over to these drivers if they want to.

Thanks.

Regards,
Santosh
Sekhar Nori Nov. 14, 2013, 10:53 a.m. UTC | #4
On Wednesday 13 November 2013 07:44 PM, Santosh Shilimkar wrote:
> On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
>> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>> ---
>>>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>> index 4705214..879e915 100644
>>> --- a/drivers/mtd/nand/davinci_nand.c
>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>                 goto err_clk_enable;
>>>         }
>>>
>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>
>> This is a hack! Just because AEMIF driver is enabled, it does not
>> guarantee that the timings have been setup by it. Instead of configuring
>> timings in two drivers, why not just convert everyone over to use the
>> new driver. Dont worry about breaking old platforms, I will help test
>> and ack them.
>>
> How about you take a stab at and convert the DaVinci code over to make
> use of new driver. We are trying to re-use as much as possible from the
> common drivers and also making an option so that the DaVinci arch can
> move over to these drivers if they want to.

Sure I could.

Ivan,

The AEMIF driver does not use platfrom_data currently. Is that something
you can add or you want me to take a stab at that as well?

Thanks,
Sekhar
Santosh Shilimkar Nov. 14, 2013, 2:36 p.m. UTC | #5
On Thursday 14 November 2013 05:53 AM, Nori, Sekhar wrote:
> On Wednesday 13 November 2013 07:44 PM, Santosh Shilimkar wrote:
>> On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
>>> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>>
>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>> ---
>>>>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>>> index 4705214..879e915 100644
>>>> --- a/drivers/mtd/nand/davinci_nand.c
>>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>>                 goto err_clk_enable;
>>>>         }
>>>>
>>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>
>>> This is a hack! Just because AEMIF driver is enabled, it does not
>>> guarantee that the timings have been setup by it. Instead of configuring
>>> timings in two drivers, why not just convert everyone over to use the
>>> new driver. Dont worry about breaking old platforms, I will help test
>>> and ack them.
>>>
>> How about you take a stab at and convert the DaVinci code over to make
>> use of new driver. We are trying to re-use as much as possible from the
>> common drivers and also making an option so that the DaVinci arch can
>> move over to these drivers if they want to.
> 
> Sure I could.
> 
Thanks

> Ivan,
> 
> The AEMIF driver does not use platfrom_data currently. Is that something
> you can add or you want me to take a stab at that as well?
> 
The AEMIF new driver is device tree only and thats the direction. So the
better thing would be to convert Davinci to DT and then make use of this
new driver. Thats how most of the new drivers used on arm socs are
moving. Adding platform data to new driver is a step in backward
direction so I would want to avoid that.

Once you start using new driver the $subject patch code skipping
is trivial. Till then I would like to proceed with current approach
which allows Nand support for Keystone.

Regards,
Santosh
Grygorii Strashko Nov. 18, 2013, 11:35 a.m. UTC | #6
On 11/12/2013 06:13 PM, Santosh Shilimkar wrote:
> On Monday 11 November 2013 12:10 PM, Khoronzhuk, Ivan wrote:
>> If Davinci AEMIF is used we don't need to set timings and bus width.
>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>>   drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>> index 4705214..879e915 100644
>> --- a/drivers/mtd/nand/davinci_nand.c
>> +++ b/drivers/mtd/nand/davinci_nand.c
>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>                  goto err_clk_enable;
>>          }
>>
>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>
> Instead above #if, just use a variable.
> 	bool aemif = IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF) and then skip
> the below code. #if block in the middle of the code looks ugly.

Yes, this is the hack.
The problem is that this part of code contains the call of Davinci 
platform function davinci_aemif_setup_timing() which is not accessible 
if Kernel is built for Keystone only.
That's why "#if" has been used.

This part of code has to be removed together with Davinci aemif platform 
code (aemif.c), once Davinci will be converted to DT and use
new driver.

The corresponding comment can be added here. Is it ok?

>
> Other than that patch looks fine to me.
>
> Regards,
> Santosh
>
Santosh Shilimkar Nov. 18, 2013, 2:08 p.m. UTC | #7
On Monday 18 November 2013 06:35 AM, Grygorii Strashko wrote:
> On 11/12/2013 06:13 PM, Santosh Shilimkar wrote:
>> On Monday 11 November 2013 12:10 PM, Khoronzhuk, Ivan wrote:
>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>> ---
>>>   drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>> index 4705214..879e915 100644
>>> --- a/drivers/mtd/nand/davinci_nand.c
>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>                  goto err_clk_enable;
>>>          }
>>>
>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>
>> Instead above #if, just use a variable.
>>     bool aemif = IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF) and then skip
>> the below code. #if block in the middle of the code looks ugly.
> 
> Yes, this is the hack.
> The problem is that this part of code contains the call of Davinci platform function davinci_aemif_setup_timing() which is not accessible if Kernel is built for Keystone only.
> That's why "#if" has been used.
> 
> This part of code has to be removed together with Davinci aemif platform code (aemif.c), once Davinci will be converted to DT and use
> new driver.
> 
> The corresponding comment can be added here. Is it ok?
> 
Thats more or less what I proposed in another reply. Lets clearly
document it on why such a check was added.

Regards,
Santosh
Ivan Khoronzhuk Nov. 18, 2013, 7:32 p.m. UTC | #8
On 11/18/2013 04:08 PM, Santosh Shilimkar wrote:
> On Monday 18 November 2013 06:35 AM, Grygorii Strashko wrote:
>> On 11/12/2013 06:13 PM, Santosh Shilimkar wrote:
>>> On Monday 11 November 2013 12:10 PM, Khoronzhuk, Ivan wrote:
>>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>>
>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>> ---
>>>>    drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>>    1 file changed, 15 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>>> index 4705214..879e915 100644
>>>> --- a/drivers/mtd/nand/davinci_nand.c
>>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>>                   goto err_clk_enable;
>>>>           }
>>>>
>>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>>
>>> Instead above #if, just use a variable.
>>>      bool aemif = IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF) and then skip
>>> the below code. #if block in the middle of the code looks ugly.
>>
>> Yes, this is the hack.
>> The problem is that this part of code contains the call of Davinci platform function davinci_aemif_setup_timing() which is not accessible if Kernel is built for Keystone only.
>> That's why "#if" has been used.
>>
>> This part of code has to be removed together with Davinci aemif platform code (aemif.c), once Davinci will be converted to DT and use
>> new driver.
>>
>> The corresponding comment can be added here. Is it ok?
>>
> Thats more or less what I proposed in another reply. Lets clearly
> document it on why such a check was added.
>
> Regards,
> Santosh
>

Ok, I'll add more explanation on it.
Ivan Khoronzhuk Nov. 18, 2013, 7:35 p.m. UTC | #9
On 11/14/2013 04:36 PM, Santosh Shilimkar wrote:
> On Thursday 14 November 2013 05:53 AM, Nori, Sekhar wrote:
>> On Wednesday 13 November 2013 07:44 PM, Santosh Shilimkar wrote:
>>> On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
>>>> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>>>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>>>
>>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>>> ---
>>>>>   drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>>>> index 4705214..879e915 100644
>>>>> --- a/drivers/mtd/nand/davinci_nand.c
>>>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>>>                  goto err_clk_enable;
>>>>>          }
>>>>>
>>>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>>
>>>> This is a hack! Just because AEMIF driver is enabled, it does not
>>>> guarantee that the timings have been setup by it. Instead of configuring
>>>> timings in two drivers, why not just convert everyone over to use the
>>>> new driver. Dont worry about breaking old platforms, I will help test
>>>> and ack them.
>>>>
>>> How about you take a stab at and convert the DaVinci code over to make
>>> use of new driver. We are trying to re-use as much as possible from the
>>> common drivers and also making an option so that the DaVinci arch can
>>> move over to these drivers if they want to.
>>
>> Sure I could.
>>
> Thanks
>
>> Ivan,
>>
>> The AEMIF driver does not use platfrom_data currently. Is that something
>> you can add or you want me to take a stab at that as well?
>>
> The AEMIF new driver is device tree only and thats the direction. So the
> better thing would be to convert Davinci to DT and then make use of this
> new driver. Thats how most of the new drivers used on arm socs are
> moving. Adding platform data to new driver is a step in backward
> direction so I would want to avoid that.
>
> Once you start using new driver the $subject patch code skipping
> is trivial. Till then I would like to proceed with current approach
> which allows Nand support for Keystone.
>
> Regards,
> Santosh
>

Agree with Santosh
Sekhar Nori Nov. 21, 2013, 5:07 p.m. UTC | #10
On 11/14/2013 8:06 PM, Santosh Shilimkar wrote:
> On Thursday 14 November 2013 05:53 AM, Nori, Sekhar wrote:
>> On Wednesday 13 November 2013 07:44 PM, Santosh Shilimkar wrote:
>>> On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
>>>> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>>>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>>>
>>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>>> ---
>>>>>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>>>> index 4705214..879e915 100644
>>>>> --- a/drivers/mtd/nand/davinci_nand.c
>>>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>>>                 goto err_clk_enable;
>>>>>         }
>>>>>
>>>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>>
>>>> This is a hack! Just because AEMIF driver is enabled, it does not
>>>> guarantee that the timings have been setup by it. Instead of configuring
>>>> timings in two drivers, why not just convert everyone over to use the
>>>> new driver. Dont worry about breaking old platforms, I will help test
>>>> and ack them.
>>>>
>>> How about you take a stab at and convert the DaVinci code over to make
>>> use of new driver. We are trying to re-use as much as possible from the
>>> common drivers and also making an option so that the DaVinci arch can
>>> move over to these drivers if they want to.
>>
>> Sure I could.
>>
> Thanks
> 
>> Ivan,
>>
>> The AEMIF driver does not use platfrom_data currently. Is that something
>> you can add or you want me to take a stab at that as well?
>>
> The AEMIF new driver is device tree only and thats the direction. So the
> better thing would be to convert Davinci to DT and then make use of this
> new driver. Thats how most of the new drivers used on arm socs are
> moving. Adding platform data to new driver is a step in backward
> direction so I would want to avoid that.

Yes, that would be the ideal way. But the reality is that there is close
to zero chance of all the DaVinci platforms using AEMIF ever getting
converted to DT. This means that we will never be able to get rid of
this piece of code.

I do agree platform data is not the direction you want to take on ARM
but at the same time its not really a deprecated mechanism as far as the
broader kernel is concerned.

So, can we consider adding platform data mechanism to the AEMIF driver?

Thanks,
Sekhar
Santosh Shilimkar Nov. 22, 2013, 5:38 p.m. UTC | #11
On Thursday 21 November 2013 12:07 PM, Nori, Sekhar wrote:
> On 11/14/2013 8:06 PM, Santosh Shilimkar wrote:
>> On Thursday 14 November 2013 05:53 AM, Nori, Sekhar wrote:
>>> On Wednesday 13 November 2013 07:44 PM, Santosh Shilimkar wrote:
>>>> On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
>>>>> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>>>>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>>>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>>>>
>>>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>>>> ---
>>>>>>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>>>>> index 4705214..879e915 100644
>>>>>> --- a/drivers/mtd/nand/davinci_nand.c
>>>>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>>>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>>>>                 goto err_clk_enable;
>>>>>>         }
>>>>>>
>>>>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>>>
>>>>> This is a hack! Just because AEMIF driver is enabled, it does not
>>>>> guarantee that the timings have been setup by it. Instead of configuring
>>>>> timings in two drivers, why not just convert everyone over to use the
>>>>> new driver. Dont worry about breaking old platforms, I will help test
>>>>> and ack them.
>>>>>
>>>> How about you take a stab at and convert the DaVinci code over to make
>>>> use of new driver. We are trying to re-use as much as possible from the
>>>> common drivers and also making an option so that the DaVinci arch can
>>>> move over to these drivers if they want to.
>>>
>>> Sure I could.
>>>
>> Thanks
>>
>>> Ivan,
>>>
>>> The AEMIF driver does not use platfrom_data currently. Is that something
>>> you can add or you want me to take a stab at that as well?
>>>
>> The AEMIF new driver is device tree only and thats the direction. So the
>> better thing would be to convert Davinci to DT and then make use of this
>> new driver. Thats how most of the new drivers used on arm socs are
>> moving. Adding platform data to new driver is a step in backward
>> direction so I would want to avoid that.
> 
> Yes, that would be the ideal way. But the reality is that there is close
> to zero chance of all the DaVinci platforms using AEMIF ever getting
> converted to DT. This means that we will never be able to get rid of
> this piece of code.
>
Well that means some of those platforms support will get deprecated
soon considering other basic frameworks like irq, timer wheel etc
is assuming the DT direction. But its your call because if you
want those platforms to live, you need to convert them to DT.
 
> I do agree platform data is not the direction you want to take on ARM
> but at the same time its not really a deprecated mechanism as far as the
> broader kernel is concerned.
> 
> So, can we consider adding platform data mechanism to the AEMIF driver?
> 
Sorry but answer is "NO". If the above hack is really bothering you,
we can actually rip out the above code in question and move it
to mach-davinci. Infact thats the best direction to make the
mtd nandn driver independent of any platform quirks(dt, pdata etc)

Ivan already has a patch to do that and he is going to post
that patch. With that patch, we won't need $subject patch.

Regards,
Santosh
Sekhar Nori Nov. 24, 2013, 9:46 a.m. UTC | #12
On Friday 22 November 2013 11:08 PM, Santosh Shilimkar wrote:
> On Thursday 21 November 2013 12:07 PM, Nori, Sekhar wrote:
>> On 11/14/2013 8:06 PM, Santosh Shilimkar wrote:
>>> On Thursday 14 November 2013 05:53 AM, Nori, Sekhar wrote:
>>>> On Wednesday 13 November 2013 07:44 PM, Santosh Shilimkar wrote:
>>>>> On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
>>>>>> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>>>>>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>>>>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>>>>>
>>>>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>>>>> ---
>>>>>>>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>>>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>>>>>> index 4705214..879e915 100644
>>>>>>> --- a/drivers/mtd/nand/davinci_nand.c
>>>>>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>>>>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>>>>>                 goto err_clk_enable;
>>>>>>>         }
>>>>>>>
>>>>>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>>>>
>>>>>> This is a hack! Just because AEMIF driver is enabled, it does not
>>>>>> guarantee that the timings have been setup by it. Instead of configuring
>>>>>> timings in two drivers, why not just convert everyone over to use the
>>>>>> new driver. Dont worry about breaking old platforms, I will help test
>>>>>> and ack them.
>>>>>>
>>>>> How about you take a stab at and convert the DaVinci code over to make
>>>>> use of new driver. We are trying to re-use as much as possible from the
>>>>> common drivers and also making an option so that the DaVinci arch can
>>>>> move over to these drivers if they want to.
>>>>
>>>> Sure I could.
>>>>
>>> Thanks
>>>
>>>> Ivan,
>>>>
>>>> The AEMIF driver does not use platfrom_data currently. Is that something
>>>> you can add or you want me to take a stab at that as well?
>>>>
>>> The AEMIF new driver is device tree only and thats the direction. So the
>>> better thing would be to convert Davinci to DT and then make use of this
>>> new driver. Thats how most of the new drivers used on arm socs are
>>> moving. Adding platform data to new driver is a step in backward
>>> direction so I would want to avoid that.
>>
>> Yes, that would be the ideal way. But the reality is that there is close
>> to zero chance of all the DaVinci platforms using AEMIF ever getting
>> converted to DT. This means that we will never be able to get rid of
>> this piece of code.
>>
> Well that means some of those platforms support will get deprecated
> soon considering other basic frameworks like irq, timer wheel etc
> is assuming the DT direction. But its your call because if you
> want those platforms to live, you need to convert them to DT.
>  
>> I do agree platform data is not the direction you want to take on ARM
>> but at the same time its not really a deprecated mechanism as far as the
>> broader kernel is concerned.
>>
>> So, can we consider adding platform data mechanism to the AEMIF driver?
>>
> Sorry but answer is "NO". If the above hack is really bothering you,
> we can actually rip out the above code in question and move it
> to mach-davinci. Infact thats the best direction to make the
> mtd nandn driver independent of any platform quirks(dt, pdata etc)
> 
> Ivan already has a patch to do that and he is going to post
> that patch. With that patch, we won't need $subject patch.
> 

Okay I will wait for the patch.

Thanks,
Sekhar
Santosh Shilimkar Nov. 26, 2013, 12:55 a.m. UTC | #13
On Sunday 24 November 2013 04:46 AM, Sekhar Nori wrote:
> On Friday 22 November 2013 11:08 PM, Santosh Shilimkar wrote:
>> On Thursday 21 November 2013 12:07 PM, Nori, Sekhar wrote:
>>> On 11/14/2013 8:06 PM, Santosh Shilimkar wrote:

[..]

>>> Yes, that would be the ideal way. But the reality is that there is close
>>> to zero chance of all the DaVinci platforms using AEMIF ever getting
>>> converted to DT. This means that we will never be able to get rid of
>>> this piece of code.
>>>
>> Well that means some of those platforms support will get deprecated
>> soon considering other basic frameworks like irq, timer wheel etc
>> is assuming the DT direction. But its your call because if you
>> want those platforms to live, you need to convert them to DT.
>>  
>>> I do agree platform data is not the direction you want to take on ARM
>>> but at the same time its not really a deprecated mechanism as far as the
>>> broader kernel is concerned.
>>>
>>> So, can we consider adding platform data mechanism to the AEMIF driver?
>>>
>> Sorry but answer is "NO". If the above hack is really bothering you,
>> we can actually rip out the above code in question and move it
>> to mach-davinci. Infact thats the best direction to make the
>> mtd nandn driver independent of any platform quirks(dt, pdata etc)
>>
>> Ivan already has a patch to do that and he is going to post
>> that patch. With that patch, we won't need $subject patch.
>>
> 
> Okay I will wait for the patch.
> 
Ivan has posted a patch already and you are copied on it.

Regards,
Santosh
diff mbox

Patch

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 4705214..879e915 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -742,27 +742,35 @@  static int __init nand_davinci_probe(struct platform_device *pdev)
                goto err_clk_enable;
        }

+#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
        /*
-        * Setup Async configuration register in case we did not boot from
-        * NAND and so bootloader did not bother to set it up.
+        * Setup Async configuration register in case we did not boot
+        * from NAND and so bootloader did not bother to set it up.
         */
-       val = davinci_nand_readl(info, A1CR_OFFSET + info->core_chipsel * 4);
+       val = davinci_nand_readl(info, A1CR_OFFSET +
+                                info->core_chipsel * 4);

-       /* Extended Wait is not valid and Select Strobe mode is not used */
+       /*
+        * Extended Wait is not valid and Select Strobe mode is not
+        * used
+        */
        val &= ~(ACR_ASIZE_MASK | ACR_EW_MASK | ACR_SS_MASK);
        if (info->chip.options & NAND_BUSWIDTH_16)
                val |= 0x1;

-       davinci_nand_writel(info, A1CR_OFFSET + info->core_chipsel * 4, val);
+       davinci_nand_writel(info, A1CR_OFFSET +
+                           info->core_chipsel * 4, val);

        ret = 0;
        if (info->timing)
-               ret = davinci_aemif_setup_timing(info->timing, info->base,
-                                                       info->core_chipsel);
+               ret = davinci_aemif_setup_timing(info->timing,
+                                                info->base,
+                                                info->core_chipsel);
        if (ret < 0) {
                dev_dbg(&pdev->dev, "NAND timing values setup fail\n");
                goto err;
        }
+#endif

        spin_lock_irq(&davinci_nand_lock);