> -----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, &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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot