subject:"\[U\-Boot\] \[PATCH\] spi\: fsl_qspi\: Add controller busy check before new spi operation"

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

2017-08-23 Thread Suresh Gupta



> -Original Message-
> From: Jagan Teki [mailto:jagannadh.t...@gmail.com]
> Sent: Wednesday, August 23, 2017 10:57 AM
> To: Suresh Gupta <suresh.gu...@nxp.com>
> Cc: u-boot@lists.denx.de; Jagan Teki <ja...@openedev.com>
> Subject: Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before
> new spi operation
> 
> On Tue, Aug 22, 2017 at 4:19 PM, Suresh Gupta <suresh.gu...@nxp.com>
> wrote:
> > Thanks  Jagan for reviewing the code.
> > Please find comments in line
> >
> >> -Original Message-
> >> From: Jagan Teki [mailto:jagannadh.t...@gmail.com]
> >> Sent: Monday, August 21, 2017 7:53 PM
> >> To: Suresh Gupta <suresh.gu...@nxp.com>
> >> Cc: u-boot@lists.denx.de; Jagan Teki <ja...@openedev.com>
> >> Subject: Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy
> >> check before new spi operation
> >>
> >> On Mon, Aug 21, 2017 at 3:56 PM, Suresh Gupta <suresh.gu...@nxp.com>
> >> wrote:
> >> > It is recommended to check either controller is free to take new
> >> > spi action. The IP_ACC and AHB_ACC bits indicates that the
> >> > controller is busy in IP or AHB mode respectively.
> >> > And the BUSY bit indicates that the controller is currently busy
> >> > handling a transaction to an external flash device
> >> >
> >> > Signed-off-by: Suresh Gupta <suresh.gu...@nxp.com>
> >> > ---
> >> >  drivers/spi/fsl_qspi.c | 26 ++
> >> > drivers/spi/fsl_qspi.h |  4 
> >> >  2 files changed, 30 insertions(+)
> >> >
> >> > diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index
> >> > 1dfa89a..69e9712 100644
> >> > --- a/drivers/spi/fsl_qspi.c
> >> > +++ b/drivers/spi/fsl_qspi.c
> >> > @@ -165,6 +165,27 @@ static inline u32 qspi_endian_xchg(u32 data)
> >> > #endif  }
> >> >
> >> > +static inline u32 qspi_controller_busy(struct fsl_qspi_priv *priv) {
> >> > +   u32 sr;
> >> > +   u32 retry = 5;
> >> > +
> >> > +   do {
> >> > +   sr = qspi_read32(priv->flags, >regs->sr);
> >> > +   if ((sr & QSPI_SR_BUSY_MASK) ||
> >>
> >> Does this bit need? we can check the busy-state with AHB_ACC and
> >> IP_ACC
> >
> > The definition of the three bits is
> > Bit2 - AHB_ACC: AHB Access: Asserted when the transaction currently
> executed was initiated by AHB bus.
> > Bit1 - IP_ACC: IP Access: Asserted when transaction currently executed was
> initiated by IP bus.
> > Bit0 - BUSY: Module Busy: Asserted when module is currently busy handling a
> transaction to an external flash device.
> >
> > Also, the below are statements mentioned in the IP Block Guide For AHB
> > Access: Since the read access is triggered via the AHB bus, the
> QSPI_SR[AHB_ACC]
> > status bit is set driving in turn the QSPI_SR[BUSY] bit 
> > until the
> transaction is finished.
> > For IP Access: Since the read access is triggered by an IP command the 
> > IP_ACC
> status bit and
> > the BUSY bit are both set (both are located in the Status 
> > Register
> (QSPI_SR) ).
> >
> > So, BUSY flag is set when the controller is busy in communication with FLASH
> and this is true for both IP and AHB mode.
> > That’s the reason checking all three status bits ensures us that controller 
> > is
> free.
> >
> >>
> >> > +   (sr & QSPI_SR_AHB_ACC_MASK) ||
> >> > +   (sr & QSPI_SR_IP_ACC_MASK)) {
> >> > +   debug("The controller is busy, sr = 0x%x\n", sr);
> >> > +   udelay(1);
> >> > +   } else {
> >> > +   break;
> >> > +   }
> >> > +   } while (--retry);
> >>
> >> These retry and infine loop doesn't seems OK, how about using wait_for_bit?
> > Ok, I will use below and send a new patch
> >
> > ret = wait_for_bit(__func__, regs->sr,
> >   QSPI_SR_BUSY_MASK |
> >   QSPI_SR_AHB_ACC_MASK |
> >   QSPI_SR_IP_ACC_MASK,
> >   false, 1000, false);
> >>
> >> > +
> >> > +   return (sr & QSPI_SR_BUSY_MASK) ||
> >> > +   (sr & QSPI_

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

2017-08-22 Thread Jagan Teki

On Tue, Aug 22, 2017 at 4:19 PM, Suresh Gupta <suresh.gu...@nxp.com> wrote:
> Thanks  Jagan for reviewing the code.
> Please find comments in line
>
>> -Original Message-
>> From: Jagan Teki [mailto:jagannadh.t...@gmail.com]
>> Sent: Monday, August 21, 2017 7:53 PM
>> To: Suresh Gupta <suresh.gu...@nxp.com>
>> Cc: u-boot@lists.denx.de; Jagan Teki <ja...@openedev.com>
>> Subject: Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before
>> new spi operation
>>
>> On Mon, Aug 21, 2017 at 3:56 PM, Suresh Gupta <suresh.gu...@nxp.com>
>> wrote:
>> > It is recommended to check either controller is free to take new spi
>> > action. The IP_ACC and AHB_ACC bits indicates that the controller is
>> > busy in IP or AHB mode respectively.
>> > And the BUSY bit indicates that the controller is currently busy
>> > handling a transaction to an external flash device
>> >
>> > Signed-off-by: Suresh Gupta <suresh.gu...@nxp.com>
>> > ---
>> >  drivers/spi/fsl_qspi.c | 26 ++
>> > drivers/spi/fsl_qspi.h |  4 
>> >  2 files changed, 30 insertions(+)
>> >
>> > diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index
>> > 1dfa89a..69e9712 100644
>> > --- a/drivers/spi/fsl_qspi.c
>> > +++ b/drivers/spi/fsl_qspi.c
>> > @@ -165,6 +165,27 @@ static inline u32 qspi_endian_xchg(u32 data)
>> > #endif  }
>> >
>> > +static inline u32 qspi_controller_busy(struct fsl_qspi_priv *priv) {
>> > +   u32 sr;
>> > +   u32 retry = 5;
>> > +
>> > +   do {
>> > +   sr = qspi_read32(priv->flags, >regs->sr);
>> > +   if ((sr & QSPI_SR_BUSY_MASK) ||
>>
>> Does this bit need? we can check the busy-state with AHB_ACC and IP_ACC
>
> The definition of the three bits is
> Bit2 - AHB_ACC: AHB Access: Asserted when the transaction currently executed 
> was initiated by AHB bus.
> Bit1 - IP_ACC: IP Access: Asserted when transaction currently executed was 
> initiated by IP bus.
> Bit0 - BUSY: Module Busy: Asserted when module is currently busy handling a 
> transaction to an external flash device.
>
> Also, the below are statements mentioned in the IP Block Guide
> For AHB Access: Since the read access is triggered via the AHB bus, the 
> QSPI_SR[AHB_ACC]
> status bit is set driving in turn the QSPI_SR[BUSY] bit until 
> the transaction is finished.
> For IP Access: Since the read access is triggered by an IP command the IP_ACC 
> status bit and
> the BUSY bit are both set (both are located in the Status 
> Register (QSPI_SR) ).
>
> So, BUSY flag is set when the controller is busy in communication with FLASH 
> and this is true for both IP and AHB mode.
> That’s the reason checking all three status bits ensures us that controller 
> is free.
>
>>
>> > +   (sr & QSPI_SR_AHB_ACC_MASK) ||
>> > +   (sr & QSPI_SR_IP_ACC_MASK)) {
>> > +   debug("The controller is busy, sr = 0x%x\n", sr);
>> > +   udelay(1);
>> > +   } else {
>> > +   break;
>> > +   }
>> > +   } while (--retry);
>>
>> These retry and infine loop doesn't seems OK, how about using wait_for_bit?
> Ok, I will use below and send a new patch
>
> ret = wait_for_bit(__func__, regs->sr,
>   QSPI_SR_BUSY_MASK |
>   QSPI_SR_AHB_ACC_MASK |
>   QSPI_SR_IP_ACC_MASK,
>   false, 1000, false);
>>
>> > +
>> > +   return (sr & QSPI_SR_BUSY_MASK) ||
>> > +   (sr & QSPI_SR_AHB_ACC_MASK) || (sr &
>> > + QSPI_SR_IP_ACC_MASK);
>>
>> I didn't understand why these bits need to return?
> After wait_for_bit, this is not required
>
>> and when will the LUT trigger?
> The check is added as it is recommended that before any new transaction, 
> these bits should be 0 i.e. controller is not busy.
> This check is required before all new types of transaction with FLASH.
> So I added this in qspi_xfer() which intern calls actual hardware operations 
> like qspi_op_write, qspi_op_erase, qspi_ahb_read, qspi_op_rdsr etc., which 
> triggers the LUT.

What about moving this in claim_bus?, because xfer will call each
transfer with each transaction.and of course while probe or each
operation claim_bus is initiating.

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

2017-08-22 Thread Suresh Gupta



> -Original Message-
> From: York Sun
> Sent: Tuesday, August 22, 2017 9:56 PM
> To: Suresh Gupta ; u-boot@lists.denx.de
> Cc: ja...@openedev.com; Prabhakar Kushwaha
> 
> Subject: Re: [PATCH] spi: fsl_qspi: Add controller busy check before new spi
> operation
> 
> On 08/21/2017 03:25 AM, Suresh Gupta wrote:
> > It is recommended to check either controller is free to take new spi
> > action. The IP_ACC and AHB_ACC bits indicates that the controller is
> > busy in IP or AHB mode respectively.
> > And the BUSY bit indicates that the controller is currently busy
> > handling a transaction to an external flash device
> >
> > Signed-off-by: Suresh Gupta 
> > ---
> >   drivers/spi/fsl_qspi.c | 26 ++
> >   drivers/spi/fsl_qspi.h |  4 
> >   2 files changed, 30 insertions(+)
> >
> > diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index
> > 1dfa89a..69e9712 100644
> > --- a/drivers/spi/fsl_qspi.c
> > +++ b/drivers/spi/fsl_qspi.c
> > @@ -165,6 +165,27 @@ static inline u32 qspi_endian_xchg(u32 data)
> >   #endif
> >   }
> >
> > +static inline u32 qspi_controller_busy(struct fsl_qspi_priv *priv) {
> > +   u32 sr;
> > +   u32 retry = 5;
> > +
> > +   do {
> > +   sr = qspi_read32(priv->flags, >regs->sr);
> > +   if ((sr & QSPI_SR_BUSY_MASK) ||
> > +   (sr & QSPI_SR_AHB_ACC_MASK) ||
> > +   (sr & QSPI_SR_IP_ACC_MASK)) {
> > +   debug("The controller is busy, sr = 0x%x\n", sr);
> > +   udelay(1);
> > +   } else {
> > +   break;
> > +   }
> > +   } while (--retry);
> 
> Does the 5 microsecond-delay make any difference?
> 
> > +
> > +   return (sr & QSPI_SR_BUSY_MASK) ||
> > +   (sr & QSPI_SR_AHB_ACC_MASK) || (sr &
> QSPI_SR_IP_ACC_MASK); }
> > +
> >   static void qspi_set_lut(struct fsl_qspi_priv *priv)
> >   {
> > struct fsl_qspi_regs *regs = priv->regs; @@ -765,6 +786,11 @@ int
> > qspi_xfer(struct fsl_qspi_priv *priv, unsigned int bitlen,
> >
> > WATCHDOG_RESET();
> >
> > +   if (qspi_controller_busy(priv)) {
> > +   printf("ERROR : The controller is busy\n");
> > +   return -EBUSY;
> > +   }
> 
> If the controller should never be busy for this operation, I wonder if you 
> really
> need a delay above.

As we see in our setup that controller get free after some time. So, I took 5 
microseconds as arbitrary number. 
Moreover, below statement [1] of BG points that controller gets free after 
prefetch completes. 

[1] Snapshot from RM: 
For any AHB access, the sequence pointed to by the QSPI_BFGENCR [SEQID] field 
is used for
the flash transaction initiated. The data is returned to the master as soon as 
the requested
amount is read from the serial flash. The controller however, continues to 
prefetch the
rest of the data in anticipation of a next consecutive request.
 
> 
> York
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

2017-08-22 Thread York Sun

On 08/21/2017 03:25 AM, Suresh Gupta wrote:
> It is recommended to check either controller is free to take
> new spi action. The IP_ACC and AHB_ACC bits indicates that
> the controller is busy in IP or AHB mode respectively.
> And the BUSY bit indicates that the controller is currently
> busy handling a transaction to an external flash device
> 
> Signed-off-by: Suresh Gupta 
> ---
>   drivers/spi/fsl_qspi.c | 26 ++
>   drivers/spi/fsl_qspi.h |  4 
>   2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
> index 1dfa89a..69e9712 100644
> --- a/drivers/spi/fsl_qspi.c
> +++ b/drivers/spi/fsl_qspi.c
> @@ -165,6 +165,27 @@ static inline u32 qspi_endian_xchg(u32 data)
>   #endif
>   }
>   
> +static inline u32 qspi_controller_busy(struct fsl_qspi_priv *priv)
> +{
> + u32 sr;
> + u32 retry = 5;
> +
> + do {
> + sr = qspi_read32(priv->flags, >regs->sr);
> + if ((sr & QSPI_SR_BUSY_MASK) ||
> + (sr & QSPI_SR_AHB_ACC_MASK) ||
> + (sr & QSPI_SR_IP_ACC_MASK)) {
> + debug("The controller is busy, sr = 0x%x\n", sr);
> + udelay(1);
> + } else {
> + break;
> + }
> + } while (--retry);

Does the 5 microsecond-delay make any difference?

> +
> + return (sr & QSPI_SR_BUSY_MASK) ||
> + (sr & QSPI_SR_AHB_ACC_MASK) || (sr & QSPI_SR_IP_ACC_MASK);
> +}
> +
>   static void qspi_set_lut(struct fsl_qspi_priv *priv)
>   {
>   struct fsl_qspi_regs *regs = priv->regs;
> @@ -765,6 +786,11 @@ int qspi_xfer(struct fsl_qspi_priv *priv, unsigned int 
> bitlen,
>   
>   WATCHDOG_RESET();
>   
> + if (qspi_controller_busy(priv)) {
> + printf("ERROR : The controller is busy\n");
> + return -EBUSY;
> + }

If the controller should never be busy for this operation, I wonder if 
you really need a delay above.

York
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

2017-08-22 Thread Suresh Gupta

Thanks  Jagan for reviewing the code. 
Please find comments in line 

> -Original Message-
> From: Jagan Teki [mailto:jagannadh.t...@gmail.com]
> Sent: Monday, August 21, 2017 7:53 PM
> To: Suresh Gupta <suresh.gu...@nxp.com>
> Cc: u-boot@lists.denx.de; Jagan Teki <ja...@openedev.com>
> Subject: Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before
> new spi operation
> 
> On Mon, Aug 21, 2017 at 3:56 PM, Suresh Gupta <suresh.gu...@nxp.com>
> wrote:
> > It is recommended to check either controller is free to take new spi
> > action. The IP_ACC and AHB_ACC bits indicates that the controller is
> > busy in IP or AHB mode respectively.
> > And the BUSY bit indicates that the controller is currently busy
> > handling a transaction to an external flash device
> >
> > Signed-off-by: Suresh Gupta <suresh.gu...@nxp.com>
> > ---
> >  drivers/spi/fsl_qspi.c | 26 ++
> > drivers/spi/fsl_qspi.h |  4 
> >  2 files changed, 30 insertions(+)
> >
> > diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index
> > 1dfa89a..69e9712 100644
> > --- a/drivers/spi/fsl_qspi.c
> > +++ b/drivers/spi/fsl_qspi.c
> > @@ -165,6 +165,27 @@ static inline u32 qspi_endian_xchg(u32 data)
> > #endif  }
> >
> > +static inline u32 qspi_controller_busy(struct fsl_qspi_priv *priv) {
> > +   u32 sr;
> > +   u32 retry = 5;
> > +
> > +   do {
> > +   sr = qspi_read32(priv->flags, >regs->sr);
> > +   if ((sr & QSPI_SR_BUSY_MASK) ||
> 
> Does this bit need? we can check the busy-state with AHB_ACC and IP_ACC

The definition of the three bits is 
Bit2 - AHB_ACC: AHB Access: Asserted when the transaction currently executed 
was initiated by AHB bus.
Bit1 - IP_ACC: IP Access: Asserted when transaction currently executed was 
initiated by IP bus.
Bit0 - BUSY: Module Busy: Asserted when module is currently busy handling a 
transaction to an external flash device.

Also, the below are statements mentioned in the IP Block Guide
For AHB Access: Since the read access is triggered via the AHB bus, the 
QSPI_SR[AHB_ACC] 
status bit is set driving in turn the QSPI_SR[BUSY] bit until 
the transaction is finished.
For IP Access: Since the read access is triggered by an IP command the IP_ACC 
status bit and
the BUSY bit are both set (both are located in the Status 
Register (QSPI_SR) ).

So, BUSY flag is set when the controller is busy in communication with FLASH 
and this is true for both IP and AHB mode.
That’s the reason checking all three status bits ensures us that controller is 
free.  

> 
> > +   (sr & QSPI_SR_AHB_ACC_MASK) ||
> > +   (sr & QSPI_SR_IP_ACC_MASK)) {
> > +   debug("The controller is busy, sr = 0x%x\n", sr);
> > +   udelay(1);
> > +   } else {
> > +   break;
> > +   }
> > +   } while (--retry);
> 
> These retry and infine loop doesn't seems OK, how about using wait_for_bit?
Ok, I will use below and send a new patch

ret = wait_for_bit(__func__, regs->sr,
  QSPI_SR_BUSY_MASK |
  QSPI_SR_AHB_ACC_MASK |
  QSPI_SR_IP_ACC_MASK,
  false, 1000, false);
> 
> > +
> > +   return (sr & QSPI_SR_BUSY_MASK) ||
> > +   (sr & QSPI_SR_AHB_ACC_MASK) || (sr &
> > + QSPI_SR_IP_ACC_MASK);
> 
> I didn't understand why these bits need to return? 
After wait_for_bit, this is not required 

> and when will the LUT trigger?
The check is added as it is recommended that before any new transaction, these 
bits should be 0 i.e. controller is not busy.
This check is required before all new types of transaction with FLASH. 
So I added this in qspi_xfer() which intern calls actual hardware operations 
like qspi_op_write, qspi_op_erase, qspi_ahb_read, qspi_op_rdsr etc., which 
triggers the LUT.  

> 
> thanks!
> --
> Jagan Teki
> Free Software Engineer | www.openedev.com U-Boot, Linux | Upstream
> Maintainer Hyderabad, India.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

2017-08-21 Thread York Sun

I shouldn't take this patch without Jagan's ack.

York

On 08/21/2017 05:03 AM, Suresh Gupta wrote:
> Hi York,
> 
> Can I delegate this patch to you? Delegate to Jagan (SPI Maintainer) delays 
> the acceptance process.
> 
> Thanks
> SuresH
> 
>> -Original Message-
>> From: Suresh Gupta [mailto:suresh.gu...@nxp.com]
>> Sent: Monday, August 21, 2017 3:56 PM
>> To: u-boot@lists.denx.de
>> Cc: York Sun ; ja...@openedev.com; Prabhakar Kushwaha
>> ; Suresh Gupta 
>> Subject: [PATCH] spi: fsl_qspi: Add controller busy check before new spi
>> operation
>>
>> It is recommended to check either controller is free to take new spi action. 
>> The
>> IP_ACC and AHB_ACC bits indicates that the controller is busy in IP or AHB 
>> mode
>> respectively.
>> And the BUSY bit indicates that the controller is currently busy handling a
>> transaction to an external flash device
>>
>> Signed-off-by: Suresh Gupta 
>> ---
>>   drivers/spi/fsl_qspi.c | 26 ++  
>> drivers/spi/fsl_qspi.h
>> |  4 
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 
>> 1dfa89a..69e9712
>> 100644
>> --- a/drivers/spi/fsl_qspi.c
>> +++ b/drivers/spi/fsl_qspi.c
>> @@ -165,6 +165,27 @@ static inline u32 qspi_endian_xchg(u32 data)  #endif  }
>>
>> +static inline u32 qspi_controller_busy(struct fsl_qspi_priv *priv) {
>> +u32 sr;
>> +u32 retry = 5;
>> +
>> +do {
>> +sr = qspi_read32(priv->flags, >regs->sr);
>> +if ((sr & QSPI_SR_BUSY_MASK) ||
>> +(sr & QSPI_SR_AHB_ACC_MASK) ||
>> +(sr & QSPI_SR_IP_ACC_MASK)) {
>> +debug("The controller is busy, sr = 0x%x\n", sr);
>> +udelay(1);
>> +} else {
>> +break;
>> +}
>> +} while (--retry);
>> +
>> +return (sr & QSPI_SR_BUSY_MASK) ||
>> +(sr & QSPI_SR_AHB_ACC_MASK) || (sr &
>> QSPI_SR_IP_ACC_MASK); }
>> +
>>   static void qspi_set_lut(struct fsl_qspi_priv *priv)  {
>>  struct fsl_qspi_regs *regs = priv->regs; @@ -765,6 +786,11 @@ int
>> qspi_xfer(struct fsl_qspi_priv *priv, unsigned int bitlen,
>>
>>  WATCHDOG_RESET();
>>
>> +if (qspi_controller_busy(priv)) {
>> +printf("ERROR : The controller is busy\n");
>> +return -EBUSY;
>> +}
>> +
>>  if (dout) {
>>  if (flags & SPI_XFER_BEGIN) {
>>  priv->cur_seqid = *(u8 *)dout;
>> diff --git a/drivers/spi/fsl_qspi.h b/drivers/spi/fsl_qspi.h index 
>> 6cb3610..e468eb2
>> 100644
>> --- a/drivers/spi/fsl_qspi.h
>> +++ b/drivers/spi/fsl_qspi.h
>> @@ -105,6 +105,10 @@ struct fsl_qspi_regs {
>>   #define QSPI_RBCT_RXBRD_SHIFT  8
>>   #define QSPI_RBCT_RXBRD_USEIPS (1 <<
>> QSPI_RBCT_RXBRD_SHIFT)
>>
>> +#define QSPI_SR_AHB_ACC_SHIFT   2
>> +#define QSPI_SR_AHB_ACC_MASK(1 <<
>> QSPI_SR_AHB_ACC_SHIFT)
>> +#define QSPI_SR_IP_ACC_SHIFT1
>> +#define QSPI_SR_IP_ACC_MASK (1 << QSPI_SR_IP_ACC_SHIFT)
>>   #define QSPI_SR_BUSY_SHIFT 0
>>   #define QSPI_SR_BUSY_MASK  (1 << QSPI_SR_BUSY_SHIFT)
>>
>> --
>> 1.9.3
> 
> 

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

2017-08-21 Thread Jagan Teki

On Mon, Aug 21, 2017 at 3:56 PM, Suresh Gupta  wrote:
> It is recommended to check either controller is free to take
> new spi action. The IP_ACC and AHB_ACC bits indicates that
> the controller is busy in IP or AHB mode respectively.
> And the BUSY bit indicates that the controller is currently
> busy handling a transaction to an external flash device
>
> Signed-off-by: Suresh Gupta 
> ---
>  drivers/spi/fsl_qspi.c | 26 ++
>  drivers/spi/fsl_qspi.h |  4 
>  2 files changed, 30 insertions(+)
>
> diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
> index 1dfa89a..69e9712 100644
> --- a/drivers/spi/fsl_qspi.c
> +++ b/drivers/spi/fsl_qspi.c
> @@ -165,6 +165,27 @@ static inline u32 qspi_endian_xchg(u32 data)
>  #endif
>  }
>
> +static inline u32 qspi_controller_busy(struct fsl_qspi_priv *priv)
> +{
> +   u32 sr;
> +   u32 retry = 5;
> +
> +   do {
> +   sr = qspi_read32(priv->flags, >regs->sr);
> +   if ((sr & QSPI_SR_BUSY_MASK) ||

Does this bit need? we can check the busy-state with AHB_ACC and IP_ACC

> +   (sr & QSPI_SR_AHB_ACC_MASK) ||
> +   (sr & QSPI_SR_IP_ACC_MASK)) {
> +   debug("The controller is busy, sr = 0x%x\n", sr);
> +   udelay(1);
> +   } else {
> +   break;
> +   }
> +   } while (--retry);

These retry and infine loop doesn't seems OK, how about using wait_for_bit?

> +
> +   return (sr & QSPI_SR_BUSY_MASK) ||
> +   (sr & QSPI_SR_AHB_ACC_MASK) || (sr & QSPI_SR_IP_ACC_MASK);

I didn't understand why these bits need to return? and when will the
LUT trigger?

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

2017-08-21 Thread Suresh Gupta

Hi York, 

Can I delegate this patch to you? Delegate to Jagan (SPI Maintainer) delays the 
acceptance process. 

Thanks 
SuresH

> -Original Message-
> From: Suresh Gupta [mailto:suresh.gu...@nxp.com]
> Sent: Monday, August 21, 2017 3:56 PM
> To: u-boot@lists.denx.de
> Cc: York Sun ; ja...@openedev.com; Prabhakar Kushwaha
> ; Suresh Gupta 
> Subject: [PATCH] spi: fsl_qspi: Add controller busy check before new spi
> operation
> 
> It is recommended to check either controller is free to take new spi action. 
> The
> IP_ACC and AHB_ACC bits indicates that the controller is busy in IP or AHB 
> mode
> respectively.
> And the BUSY bit indicates that the controller is currently busy handling a
> transaction to an external flash device
> 
> Signed-off-by: Suresh Gupta 
> ---
>  drivers/spi/fsl_qspi.c | 26 ++  
> drivers/spi/fsl_qspi.h
> |  4 
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index 
> 1dfa89a..69e9712
> 100644
> --- a/drivers/spi/fsl_qspi.c
> +++ b/drivers/spi/fsl_qspi.c
> @@ -165,6 +165,27 @@ static inline u32 qspi_endian_xchg(u32 data)  #endif  }
> 
> +static inline u32 qspi_controller_busy(struct fsl_qspi_priv *priv) {
> + u32 sr;
> + u32 retry = 5;
> +
> + do {
> + sr = qspi_read32(priv->flags, >regs->sr);
> + if ((sr & QSPI_SR_BUSY_MASK) ||
> + (sr & QSPI_SR_AHB_ACC_MASK) ||
> + (sr & QSPI_SR_IP_ACC_MASK)) {
> + debug("The controller is busy, sr = 0x%x\n", sr);
> + udelay(1);
> + } else {
> + break;
> + }
> + } while (--retry);
> +
> + return (sr & QSPI_SR_BUSY_MASK) ||
> + (sr & QSPI_SR_AHB_ACC_MASK) || (sr &
> QSPI_SR_IP_ACC_MASK); }
> +
>  static void qspi_set_lut(struct fsl_qspi_priv *priv)  {
>   struct fsl_qspi_regs *regs = priv->regs; @@ -765,6 +786,11 @@ int
> qspi_xfer(struct fsl_qspi_priv *priv, unsigned int bitlen,
> 
>   WATCHDOG_RESET();
> 
> + if (qspi_controller_busy(priv)) {
> + printf("ERROR : The controller is busy\n");
> + return -EBUSY;
> + }
> +
>   if (dout) {
>   if (flags & SPI_XFER_BEGIN) {
>   priv->cur_seqid = *(u8 *)dout;
> diff --git a/drivers/spi/fsl_qspi.h b/drivers/spi/fsl_qspi.h index 
> 6cb3610..e468eb2
> 100644
> --- a/drivers/spi/fsl_qspi.h
> +++ b/drivers/spi/fsl_qspi.h
> @@ -105,6 +105,10 @@ struct fsl_qspi_regs {
>  #define QSPI_RBCT_RXBRD_SHIFT8
>  #define QSPI_RBCT_RXBRD_USEIPS   (1 <<
> QSPI_RBCT_RXBRD_SHIFT)
> 
> +#define QSPI_SR_AHB_ACC_SHIFT2
> +#define QSPI_SR_AHB_ACC_MASK (1 <<
> QSPI_SR_AHB_ACC_SHIFT)
> +#define QSPI_SR_IP_ACC_SHIFT 1
> +#define QSPI_SR_IP_ACC_MASK  (1 << QSPI_SR_IP_ACC_SHIFT)
>  #define QSPI_SR_BUSY_SHIFT   0
>  #define QSPI_SR_BUSY_MASK(1 << QSPI_SR_BUSY_SHIFT)
> 
> --
> 1.9.3

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

2017-08-21 Thread Suresh Gupta

It is recommended to check either controller is free to take
new spi action. The IP_ACC and AHB_ACC bits indicates that
the controller is busy in IP or AHB mode respectively.
And the BUSY bit indicates that the controller is currently
busy handling a transaction to an external flash device

Signed-off-by: Suresh Gupta 
---
 drivers/spi/fsl_qspi.c | 26 ++
 drivers/spi/fsl_qspi.h |  4 
 2 files changed, 30 insertions(+)

diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c
index 1dfa89a..69e9712 100644
--- a/drivers/spi/fsl_qspi.c
+++ b/drivers/spi/fsl_qspi.c
@@ -165,6 +165,27 @@ static inline u32 qspi_endian_xchg(u32 data)
 #endif
 }
 
+static inline u32 qspi_controller_busy(struct fsl_qspi_priv *priv)
+{
+   u32 sr;
+   u32 retry = 5;
+
+   do {
+   sr = qspi_read32(priv->flags, >regs->sr);
+   if ((sr & QSPI_SR_BUSY_MASK) ||
+   (sr & QSPI_SR_AHB_ACC_MASK) ||
+   (sr & QSPI_SR_IP_ACC_MASK)) {
+   debug("The controller is busy, sr = 0x%x\n", sr);
+   udelay(1);
+   } else {
+   break;
+   }
+   } while (--retry);
+
+   return (sr & QSPI_SR_BUSY_MASK) ||
+   (sr & QSPI_SR_AHB_ACC_MASK) || (sr & QSPI_SR_IP_ACC_MASK);
+}
+
 static void qspi_set_lut(struct fsl_qspi_priv *priv)
 {
struct fsl_qspi_regs *regs = priv->regs;
@@ -765,6 +786,11 @@ int qspi_xfer(struct fsl_qspi_priv *priv, unsigned int 
bitlen,
 
WATCHDOG_RESET();
 
+   if (qspi_controller_busy(priv)) {
+   printf("ERROR : The controller is busy\n");
+   return -EBUSY;
+   }
+
if (dout) {
if (flags & SPI_XFER_BEGIN) {
priv->cur_seqid = *(u8 *)dout;
diff --git a/drivers/spi/fsl_qspi.h b/drivers/spi/fsl_qspi.h
index 6cb3610..e468eb2 100644
--- a/drivers/spi/fsl_qspi.h
+++ b/drivers/spi/fsl_qspi.h
@@ -105,6 +105,10 @@ struct fsl_qspi_regs {
 #define QSPI_RBCT_RXBRD_SHIFT  8
 #define QSPI_RBCT_RXBRD_USEIPS (1 << QSPI_RBCT_RXBRD_SHIFT)
 
+#define QSPI_SR_AHB_ACC_SHIFT  2
+#define QSPI_SR_AHB_ACC_MASK   (1 << QSPI_SR_AHB_ACC_SHIFT)
+#define QSPI_SR_IP_ACC_SHIFT   1
+#define QSPI_SR_IP_ACC_MASK(1 << QSPI_SR_IP_ACC_SHIFT)
 #define QSPI_SR_BUSY_SHIFT 0
 #define QSPI_SR_BUSY_MASK  (1 << QSPI_SR_BUSY_SHIFT)
 
-- 
1.9.3

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

Re: [U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

[U-Boot] [PATCH] spi: fsl_qspi: Add controller busy check before new spi operation

9 matches

Site Navigation

Mail list logo

Footer information