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