RE: [PATCH v4 1/3] omap3 gpmc: functionality enhancement

2010-05-28 Thread Ghorai, Sukumar


> -Original Message-
> From: Vimal Singh [mailto:vimal.neww...@gmail.com]
> Sent: Saturday, May 29, 2010 12:20 AM
> To: Ghorai, Sukumar
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH v4 1/3] omap3 gpmc: functionality enhancement
> 
> On Fri, May 28, 2010 at 11:50 PM, Ghorai, Sukumar  wrote:
> >
> >
> >> -Original Message-
> >> From: Vimal Singh [mailto:vimal.neww...@gmail.com]
> >> Sent: Friday, May 28, 2010 9:14 PM
> >> To: Ghorai, Sukumar
> >> Cc: linux-omap@vger.kernel.org
> >> Subject: Re: [PATCH v4 1/3] omap3 gpmc: functionality enhancement
> >>
> >> On Fri, May 28, 2010 at 12:03 PM, Ghorai, Sukumar 
> wrote:
> >> >
> >> >> -Original Message-
> >> >> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> >> >> ow...@vger.kernel.org] On Behalf Of Vimal Singh
> >> >> Sent: Thursday, May 27, 2010 11:56 PM
> >> >> To: Ghorai, Sukumar
> >> >> Cc: linux-omap@vger.kernel.org
> >> >> Subject: Re: [PATCH v4 1/3] omap3 gpmc: functionality enhancement
> >> >>
> >> >> On Thu, May 27, 2010 at 6:54 PM, Sukumar Ghorai 
> >> wrote:
> >> >> [...]
> >> >> > -static unsigned                gpmc_cs_map;
> >> >> > +static unsigned        int gpmc_cs_map;        /* flag for cs
> which
> >> are
> >> >> initialized */
> >> >>
> >> >> Tab should be after 'int', not before.
> >> >>
> >> >> [...]
> >> >> > @@ -456,13 +565,22 @@ EXPORT_SYMBOL(gpmc_prefetch_enable);
> >> >> >  /**
> >> >> >  * gpmc_prefetch_reset - disables and stops the prefetch engine
> >> >> >  */
> >> >> > -void gpmc_prefetch_reset(void)
> >> >> > +int gpmc_prefetch_reset(int cs)
> >> >> >  {
> >> >> > +       u32 config1;
> >> >> > +
> >> >> > +       /* check if the same module/cs is trying to reset */
> >> >> > +       config1 = gpmc_read_reg(GPMC_PREFETCH_CONFIG1);
> >> >> > +       if (((config1 >> CS_NUM_SHIFT) & 0x7) != cs)
> >> >> > +               return -EINVAL;
> >> >> > +
> >> >>
> >> >> You really do not need this.
> >> >> Prefetch has just one instance at a time and 'reset' will be call
> only
> >> >> when driver has got access to prefetch (for either read or write
> >> >> access), from the driver.
> >> > [Ghorai] Agree. And.. NAND may not be good example. But if tomorrow
> i/o
> >> operation need to cancel for some other type of device and for big size
> of
> >> IO on progress. So is not this API required that case?
> >> >
> >>
> >> First of all, this prefetch engine is dedicated for only NAND.
> > [Ghorai] ok. Does not this below response is contradicting previous
> statement?
> >> Then, if user wants to 'cancel', say a big write operation; since
> >> there is no 'cancel' command provided by chip (nand), FS has to handle
> >> it. And, I guess, best way will be to complete current chunck of
> >> request (in this case writing of current page or sub-page) and then
> >> cancel any further pending transfer.
> >> And in that case again, prefetch 'reset' will get called for
> >> completion of current page/sub-page operation.
> >
> > [Ghorai]
> > 1. In this case why we checks - prefetch engine busy or not, if we are
> waiting to complete one request to be completed before another? As we
> reset the prefetch-engine at the end of the io?
> >
> Since there could be other request (for read/write) from another
> driver instance (in case we have more than one NAND chip on board).
> 
> > 2. The only possible scenario is - when two driver(say for two separate
> nand chip) trying to access the prefetch-engine. In that case who
> accessing the prefetch engine he should only reset the engine. And to
> avoid confusion and mistake of the user of this API, it has been added
> 'cs' number as a function parameter for reset and the same is used to
> check before reset.
> >
> 
> In this scenario, when second driver instance fails to get prefetch,
> it completes the request by cpu transfer method. And there is no
> chance for prefetch reset to get called from this driver instance.
> Note that we are not calling reset explicitly, but a driver (instance)
> resets the prefetch only if it gets first.
[Ghorai] I says.. And to avoid confusion and mistake of the user of this API, 
it has been added 'cs' number as a function parameter for reset and the same is 
used to check before reset.
What's wrong?
> 
> --
> Regards,
> Vimal Singh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 1/3] omap3 gpmc: functionality enhancement

2010-05-28 Thread Vimal Singh
On Fri, May 28, 2010 at 11:50 PM, Ghorai, Sukumar  wrote:
>
>
>> -Original Message-
>> From: Vimal Singh [mailto:vimal.neww...@gmail.com]
>> Sent: Friday, May 28, 2010 9:14 PM
>> To: Ghorai, Sukumar
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCH v4 1/3] omap3 gpmc: functionality enhancement
>>
>> On Fri, May 28, 2010 at 12:03 PM, Ghorai, Sukumar  wrote:
>> >
>> >> -Original Message-
>> >> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
>> >> ow...@vger.kernel.org] On Behalf Of Vimal Singh
>> >> Sent: Thursday, May 27, 2010 11:56 PM
>> >> To: Ghorai, Sukumar
>> >> Cc: linux-omap@vger.kernel.org
>> >> Subject: Re: [PATCH v4 1/3] omap3 gpmc: functionality enhancement
>> >>
>> >> On Thu, May 27, 2010 at 6:54 PM, Sukumar Ghorai 
>> wrote:
>> >> [...]
>> >> > -static unsigned                gpmc_cs_map;
>> >> > +static unsigned        int gpmc_cs_map;        /* flag for cs which
>> are
>> >> initialized */
>> >>
>> >> Tab should be after 'int', not before.
>> >>
>> >> [...]
>> >> > @@ -456,13 +565,22 @@ EXPORT_SYMBOL(gpmc_prefetch_enable);
>> >> >  /**
>> >> >  * gpmc_prefetch_reset - disables and stops the prefetch engine
>> >> >  */
>> >> > -void gpmc_prefetch_reset(void)
>> >> > +int gpmc_prefetch_reset(int cs)
>> >> >  {
>> >> > +       u32 config1;
>> >> > +
>> >> > +       /* check if the same module/cs is trying to reset */
>> >> > +       config1 = gpmc_read_reg(GPMC_PREFETCH_CONFIG1);
>> >> > +       if (((config1 >> CS_NUM_SHIFT) & 0x7) != cs)
>> >> > +               return -EINVAL;
>> >> > +
>> >>
>> >> You really do not need this.
>> >> Prefetch has just one instance at a time and 'reset' will be call only
>> >> when driver has got access to prefetch (for either read or write
>> >> access), from the driver.
>> > [Ghorai] Agree. And.. NAND may not be good example. But if tomorrow i/o
>> operation need to cancel for some other type of device and for big size of
>> IO on progress. So is not this API required that case?
>> >
>>
>> First of all, this prefetch engine is dedicated for only NAND.
> [Ghorai] ok. Does not this below response is contradicting previous statement?
>> Then, if user wants to 'cancel', say a big write operation; since
>> there is no 'cancel' command provided by chip (nand), FS has to handle
>> it. And, I guess, best way will be to complete current chunck of
>> request (in this case writing of current page or sub-page) and then
>> cancel any further pending transfer.
>> And in that case again, prefetch 'reset' will get called for
>> completion of current page/sub-page operation.
>
> [Ghorai]
> 1. In this case why we checks - prefetch engine busy or not, if we are 
> waiting to complete one request to be completed before another? As we reset 
> the prefetch-engine at the end of the io?
>
Since there could be other request (for read/write) from another
driver instance (in case we have more than one NAND chip on board).

> 2. The only possible scenario is - when two driver(say for two separate nand 
> chip) trying to access the prefetch-engine. In that case who accessing the 
> prefetch engine he should only reset the engine. And to avoid confusion and 
> mistake of the user of this API, it has been added 'cs' number as a function 
> parameter for reset and the same is used to check before reset.
>

In this scenario, when second driver instance fails to get prefetch,
it completes the request by cpu transfer method. And there is no
chance for prefetch reset to get called from this driver instance.
Note that we are not calling reset explicitly, but a driver (instance)
resets the prefetch only if it gets first.

-- 
Regards,
Vimal Singh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [PATCH v4 1/3] omap3 gpmc: functionality enhancement

2010-05-28 Thread Ghorai, Sukumar


> -Original Message-
> From: Vimal Singh [mailto:vimal.neww...@gmail.com]
> Sent: Friday, May 28, 2010 9:14 PM
> To: Ghorai, Sukumar
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH v4 1/3] omap3 gpmc: functionality enhancement
> 
> On Fri, May 28, 2010 at 12:03 PM, Ghorai, Sukumar  wrote:
> >
> >> -Original Message-
> >> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> >> ow...@vger.kernel.org] On Behalf Of Vimal Singh
> >> Sent: Thursday, May 27, 2010 11:56 PM
> >> To: Ghorai, Sukumar
> >> Cc: linux-omap@vger.kernel.org
> >> Subject: Re: [PATCH v4 1/3] omap3 gpmc: functionality enhancement
> >>
> >> On Thu, May 27, 2010 at 6:54 PM, Sukumar Ghorai 
> wrote:
> >> [...]
> >> > -static unsigned                gpmc_cs_map;
> >> > +static unsigned        int gpmc_cs_map;        /* flag for cs which
> are
> >> initialized */
> >>
> >> Tab should be after 'int', not before.
> >>
> >> [...]
> >> > @@ -456,13 +565,22 @@ EXPORT_SYMBOL(gpmc_prefetch_enable);
> >> >  /**
> >> >  * gpmc_prefetch_reset - disables and stops the prefetch engine
> >> >  */
> >> > -void gpmc_prefetch_reset(void)
> >> > +int gpmc_prefetch_reset(int cs)
> >> >  {
> >> > +       u32 config1;
> >> > +
> >> > +       /* check if the same module/cs is trying to reset */
> >> > +       config1 = gpmc_read_reg(GPMC_PREFETCH_CONFIG1);
> >> > +       if (((config1 >> CS_NUM_SHIFT) & 0x7) != cs)
> >> > +               return -EINVAL;
> >> > +
> >>
> >> You really do not need this.
> >> Prefetch has just one instance at a time and 'reset' will be call only
> >> when driver has got access to prefetch (for either read or write
> >> access), from the driver.
> > [Ghorai] Agree. And.. NAND may not be good example. But if tomorrow i/o
> operation need to cancel for some other type of device and for big size of
> IO on progress. So is not this API required that case?
> >
> 
> First of all, this prefetch engine is dedicated for only NAND.
[Ghorai] ok. Does not this below response is contradicting previous statement?
> Then, if user wants to 'cancel', say a big write operation; since
> there is no 'cancel' command provided by chip (nand), FS has to handle
> it. And, I guess, best way will be to complete current chunck of
> request (in this case writing of current page or sub-page) and then
> cancel any further pending transfer.
> And in that case again, prefetch 'reset' will get called for
> completion of current page/sub-page operation.

[Ghorai] 
1. In this case why we checks - prefetch engine busy or not, if we are waiting 
to complete one request to be completed before another? As we reset the 
prefetch-engine at the end of the io?

2. The only possible scenario is - when two driver(say for two separate nand 
chip) trying to access the prefetch-engine. In that case who accessing the 
prefetch engine he should only reset the engine. And to avoid confusion and 
mistake of the user of this API, it has been added 'cs' number as a function 
parameter for reset and the same is used to check before reset. 

3. Still you think problem? Let me know..

> 
> --
> Regards,
> Vimal Singh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 1/3] omap3 gpmc: functionality enhancement

2010-05-28 Thread Vimal Singh
On Fri, May 28, 2010 at 12:03 PM, Ghorai, Sukumar  wrote:
>
>> -Original Message-
>> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
>> ow...@vger.kernel.org] On Behalf Of Vimal Singh
>> Sent: Thursday, May 27, 2010 11:56 PM
>> To: Ghorai, Sukumar
>> Cc: linux-omap@vger.kernel.org
>> Subject: Re: [PATCH v4 1/3] omap3 gpmc: functionality enhancement
>>
>> On Thu, May 27, 2010 at 6:54 PM, Sukumar Ghorai  wrote:
>> [...]
>> > -static unsigned                gpmc_cs_map;
>> > +static unsigned        int gpmc_cs_map;        /* flag for cs which are
>> initialized */
>>
>> Tab should be after 'int', not before.
>>
>> [...]
>> > @@ -456,13 +565,22 @@ EXPORT_SYMBOL(gpmc_prefetch_enable);
>> >  /**
>> >  * gpmc_prefetch_reset - disables and stops the prefetch engine
>> >  */
>> > -void gpmc_prefetch_reset(void)
>> > +int gpmc_prefetch_reset(int cs)
>> >  {
>> > +       u32 config1;
>> > +
>> > +       /* check if the same module/cs is trying to reset */
>> > +       config1 = gpmc_read_reg(GPMC_PREFETCH_CONFIG1);
>> > +       if (((config1 >> CS_NUM_SHIFT) & 0x7) != cs)
>> > +               return -EINVAL;
>> > +
>>
>> You really do not need this.
>> Prefetch has just one instance at a time and 'reset' will be call only
>> when driver has got access to prefetch (for either read or write
>> access), from the driver.
> [Ghorai] Agree. And.. NAND may not be good example. But if tomorrow i/o 
> operation need to cancel for some other type of device and for big size of IO 
> on progress. So is not this API required that case?
>

First of all, this prefetch engine is dedicated for only NAND.
Then, if user wants to 'cancel', say a big write operation; since
there is no 'cancel' command provided by chip (nand), FS has to handle
it. And, I guess, best way will be to complete current chunck of
request (in this case writing of current page or sub-page) and then
cancel any further pending transfer.
And in that case again, prefetch 'reset' will get called for
completion of current page/sub-page operation.

-- 
Regards,
Vimal Singh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


RE: [PATCH v4 1/3] omap3 gpmc: functionality enhancement

2010-05-27 Thread Ghorai, Sukumar

> -Original Message-
> From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> ow...@vger.kernel.org] On Behalf Of Vimal Singh
> Sent: Thursday, May 27, 2010 11:56 PM
> To: Ghorai, Sukumar
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH v4 1/3] omap3 gpmc: functionality enhancement
> 
> On Thu, May 27, 2010 at 6:54 PM, Sukumar Ghorai  wrote:
> [...]
> > -static unsigned                gpmc_cs_map;
> > +static unsigned        int gpmc_cs_map;        /* flag for cs which are
> initialized */
> 
> Tab should be after 'int', not before.
> 
> [...]
> > @@ -456,13 +565,22 @@ EXPORT_SYMBOL(gpmc_prefetch_enable);
> >  /**
> >  * gpmc_prefetch_reset - disables and stops the prefetch engine
> >  */
> > -void gpmc_prefetch_reset(void)
> > +int gpmc_prefetch_reset(int cs)
> >  {
> > +       u32 config1;
> > +
> > +       /* check if the same module/cs is trying to reset */
> > +       config1 = gpmc_read_reg(GPMC_PREFETCH_CONFIG1);
> > +       if (((config1 >> CS_NUM_SHIFT) & 0x7) != cs)
> > +               return -EINVAL;
> > +
> 
> You really do not need this.
> Prefetch has just one instance at a time and 'reset' will be call only
> when driver has got access to prefetch (for either read or write
> access), from the driver.
[Ghorai] Agree. And.. NAND may not be good example. But if tomorrow i/o 
operation need to cancel for some other type of device and for big size of IO 
on progress. So is not this API required that case?

> 
> --
> Regards,
> Vimal Singh
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4 1/3] omap3 gpmc: functionality enhancement

2010-05-27 Thread Vimal Singh
On Thu, May 27, 2010 at 6:54 PM, Sukumar Ghorai  wrote:
[...]
> -static unsigned                gpmc_cs_map;
> +static unsigned        int gpmc_cs_map;        /* flag for cs which are 
> initialized */

Tab should be after 'int', not before.

[...]
> @@ -456,13 +565,22 @@ EXPORT_SYMBOL(gpmc_prefetch_enable);
>  /**
>  * gpmc_prefetch_reset - disables and stops the prefetch engine
>  */
> -void gpmc_prefetch_reset(void)
> +int gpmc_prefetch_reset(int cs)
>  {
> +       u32 config1;
> +
> +       /* check if the same module/cs is trying to reset */
> +       config1 = gpmc_read_reg(GPMC_PREFETCH_CONFIG1);
> +       if (((config1 >> CS_NUM_SHIFT) & 0x7) != cs)
> +               return -EINVAL;
> +

You really do not need this.
Prefetch has just one instance at a time and 'reset' will be call only
when driver has got access to prefetch (for either read or write
access), from the driver.

-- 
Regards,
Vimal Singh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html