From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jagan Teki Date: Wed, 30 Aug 2017 13:02:39 +0530 Subject: [U-Boot] [PATCH v2] spi: fsl_qspi: Add controller busy check before new spi operation In-Reply-To: References: <1504013105-11480-1-git-send-email-suresh.gupta@nxp.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Wed, Aug 30, 2017 at 12:23 PM, Suresh Gupta wrote: > > >> -----Original Message----- >> From: Jagan Teki [mailto:jagannadh.teki at gmail.com] >> Sent: Tuesday, August 29, 2017 11:08 PM >> To: Suresh Gupta >> Cc: u-boot at lists.denx.de; York Sun >> 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 >> 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 >> > --- >> > 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 >> > #include >> > #include >> > +#include >> > #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.