* [U-Boot] [PATCH v2] spi: fsl_qspi: Add controller busy check before new spi operation
@ 2017-08-29 13:25 Suresh Gupta
2017-08-29 17:38 ` Jagan Teki
0 siblings, 1 reply; 4+ messages in thread
From: Suresh Gupta @ 2017-08-29 13:25 UTC (permalink / raw)
To: u-boot
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@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;
+ /* 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;
+ }
+
mcr_val = qspi_read32(priv->flags, &priv->regs->mcr);
qspi_write32(priv->flags, &priv->regs->mcr,
QSPI_MCR_RESERVED_MASK | QSPI_MCR_MDIS_MASK |
@@ -1156,10 +1169,23 @@ static int fsl_qspi_claim_bus(struct udevice *dev)
struct fsl_qspi_priv *priv;
struct udevice *bus;
struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev);
+ int ret;
bus = dev->parent;
priv = dev_get_priv(bus);
+ /* 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;
+ }
+
priv->cur_amba_base = priv->amba_base[slave_plat->cs];
qspi_module_disable(priv, 0);
diff --git a/drivers/spi/fsl_qspi.h b/drivers/spi/fsl_qspi.h
index 6cb3610..e468eb2 100644
--- a/drivers/spi/fsl_qspi.h
+++ b/drivers/spi/fsl_qspi.h
@@ -105,6 +105,10 @@ struct fsl_qspi_regs {
#define QSPI_RBCT_RXBRD_SHIFT 8
#define QSPI_RBCT_RXBRD_USEIPS (1 << QSPI_RBCT_RXBRD_SHIFT)
+#define QSPI_SR_AHB_ACC_SHIFT 2
+#define QSPI_SR_AHB_ACC_MASK (1 << QSPI_SR_AHB_ACC_SHIFT)
+#define QSPI_SR_IP_ACC_SHIFT 1
+#define QSPI_SR_IP_ACC_MASK (1 << QSPI_SR_IP_ACC_SHIFT)
#define QSPI_SR_BUSY_SHIFT 0
#define QSPI_SR_BUSY_MASK (1 << QSPI_SR_BUSY_SHIFT)
--
1.9.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH v2] spi: fsl_qspi: Add controller busy check before new spi operation
2017-08-29 13:25 [U-Boot] [PATCH v2] spi: fsl_qspi: Add controller busy check before new spi operation Suresh Gupta
@ 2017-08-29 17:38 ` Jagan Teki
2017-08-30 6:53 ` Suresh Gupta
0 siblings, 1 reply; 4+ messages in thread
From: Jagan Teki @ 2017-08-29 17:38 UTC (permalink / raw)
To: u-boot
On Tue, Aug 29, 2017 at 6:55 PM, Suresh Gupta <suresh.gupta@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@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?
> + /* 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 wait_for_bit usually return
-ETIMEDOUT or -EINTR on failure so just return ret.
thanks!
--
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH v2] spi: fsl_qspi: Add controller busy check before new spi operation
2017-08-29 17:38 ` Jagan Teki
@ 2017-08-30 6:53 ` Suresh Gupta
2017-08-30 7:32 ` Jagan Teki
0 siblings, 1 reply; 4+ messages in thread
From: Suresh Gupta @ 2017-08-30 6:53 UTC (permalink / raw)
To: u-boot
> -----Original Message-----
> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
> Sent: Tuesday, August 29, 2017 11:08 PM
> To: Suresh Gupta <suresh.gupta@nxp.com>
> Cc: u-boot at lists.denx.de; York Sun <york.sun@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@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@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?
> wait_for_bit usually return -ETIMEDOUT
> or -EINTR on failure so just return ret.
Ok, I will make changes in next patch
>
> thanks!
> --
> Jagan Teki
> Free Software Engineer | www.openedev.com U-Boot, Linux | Upstream
> Maintainer Hyderabad, India.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH v2] spi: fsl_qspi: Add controller busy check before new spi operation
2017-08-30 6:53 ` Suresh Gupta
@ 2017-08-30 7:32 ` Jagan Teki
0 siblings, 0 replies; 4+ messages in thread
From: Jagan Teki @ 2017-08-30 7:32 UTC (permalink / raw)
To: u-boot
On Wed, Aug 30, 2017 at 12:23 PM, Suresh Gupta <suresh.gupta@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@nxp.com>
>> Cc: u-boot at lists.denx.de; York Sun <york.sun@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@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@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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-08-30 7:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-29 13:25 [U-Boot] [PATCH v2] spi: fsl_qspi: Add controller busy check before new spi operation Suresh Gupta
2017-08-29 17:38 ` Jagan Teki
2017-08-30 6:53 ` Suresh Gupta
2017-08-30 7:32 ` Jagan Teki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.