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

[Suresh Gupta]

> -----Original Message-----
> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
> Sent: Wednesday, August 23, 2017 10:57 AM
> To: Suresh Gupta <suresh.gupta at nxp.com>
> Cc: u-boot at lists.denx.de; Jagan Teki <jagan at 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.gupta at nxp.com>
> wrote:
> > Thanks  Jagan for reviewing the code.
> > Please find comments in line
> >
> >> -----Original Message-----
> >> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
> >> Sent: Monday, August 21, 2017 7:53 PM
> >> To: Suresh Gupta <suresh.gupta at nxp.com>
> >> Cc: u-boot at lists.denx.de; Jagan Teki <jagan at 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.gupta at 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.gupta at 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, &priv->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.
> 
I will check and let you know tomorrow. 


> thanks!
> --
> Jagan Teki
> Free Software Engineer | www.openedev.com U-Boot, Linux | Upstream
> Maintainer Hyderabad, India.