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, &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.
>
> 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
