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

[Jagan Teki]

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.

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