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

[Jagan Teki]

On Wed, Aug 30, 2017 at 12:23 PM, Suresh Gupta <suresh.gupta at nxp.com> wrote:
>
>
>> -----Original Message-----
>> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
>> Sent: Tuesday, August 29, 2017 11:08 PM
>> To: Suresh Gupta <suresh.gupta at nxp.com>
>> Cc: u-boot at lists.denx.de; York Sun <york.sun at nxp.com>
>> Subject: Re: [PATCH v2] spi: fsl_qspi: Add controller busy check before new spi
>> operation
>>
>> On Tue, Aug 29, 2017 at 6:55 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 controller is currently busy handling
>> > a transaction to an external flash device
>> >
>> > Signed-off-by: Suresh Gupta <suresh.gupta at nxp.com>
>> > ---
>> > Changes in v2:
>> >
>> > - Add wait_for_bit instead of while
>> > - move the busy check code to fsl_qspi_claim_bus form qspi_xfer
>> >
>> >  drivers/spi/fsl_qspi.c | 28 +++++++++++++++++++++++++++-
>> > drivers/spi/fsl_qspi.h |  4 ++++
>> >  2 files changed, 31 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/spi/fsl_qspi.c b/drivers/spi/fsl_qspi.c index
>> > 1dfa89a..ed23aac 100644
>> > --- a/drivers/spi/fsl_qspi.c
>> > +++ b/drivers/spi/fsl_qspi.c
>> > @@ -14,6 +14,7 @@
>> >  #include <dm.h>
>> >  #include <errno.h>
>> >  #include <watchdog.h>
>> > +#include <wait_bit.h>
>> >  #include "fsl_qspi.h"
>> >
>> >  DECLARE_GLOBAL_DATA_PTR;
>> > @@ -991,7 +992,7 @@ static int fsl_qspi_probe(struct udevice *bus)
>> >         struct fsl_qspi_platdata *plat = dev_get_platdata(bus);
>> >         struct fsl_qspi_priv *priv = dev_get_priv(bus);
>> >         struct dm_spi_bus *dm_spi_bus;
>> > -       int i;
>> > +       int i, ret;
>> >
>> >         dm_spi_bus = bus->uclass_priv;
>> >
>> > @@ -1011,6 +1012,18 @@ static int fsl_qspi_probe(struct udevice *bus)
>> >         priv->flash_num = plat->flash_num;
>> >         priv->num_chipselect = plat->num_chipselect;
>> >
>>
>> I think in previous version, this code not added in probe is it? is this because
>> probe doing mcr and other reg operations?
>
> The probe function changes the LUTs, change AHB configuration and change the endianness.
> So, changing above setting when the controller is busy in some AHB access will affect the running access.
>>
>> > +       /* make sure controller is not busy anywhere */
>> > +       ret = wait_for_bit(__func__, &priv->regs->sr,
>> > +                          QSPI_SR_BUSY_MASK |
>> > +                          QSPI_SR_AHB_ACC_MASK |
>> > +                          QSPI_SR_IP_ACC_MASK,
>> > +                          false, 1000, false);
>> > +
>> > +       if (ret) {
>> > +               printf("ERROR : The controller is busy\n");
>> > +               return -EBUSY;
>> > +       }
>>
>> Better to drop printf or use debug and
> The error above is trivial and after this error, the sf commands do not work.
> And if we hide the message under debug, normal user will not understand the issue.
> As per me this should be printf, what you say?

sf return error code, can't user understand based on that? idea is to
avoid printf's in platform driver.

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