Re: [PATCH 15/16] fpga: machxo2: extend erase timeout for machxo2 FPGA

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2022-08-29 at 12:51:19 +0200, Johannes Zink wrote:
> Hi Yilun, 
> 
> On Mon, 2022-08-29 at 17:26 +0800, Xu Yilun wrote:
> > On 2022-08-25 at 16:13:42 +0200, Johannes Zink wrote:
> > > Measurements showed that some FPGAs take significantly longer than
> > > the
> > > default wait function supplied. The datasheet inidicates up to 30
> > > seconds erase times for some MachXO2 FPGAs, depending on the number
> > > of
> > > LUTs (and the corresponding configuration flash size).
> > > 
> > > Signed-off-by: Johannes Zink <j.zink@xxxxxxxxxxxxxx>
> > > ---
> > >  drivers/fpga/machxo2-common.c | 28 ++++++++++++++++++++++++++--
> > >  1 file changed, 26 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-
> > > common.c
> > > index ccf9a50fc590..e8967cdee2c6 100644
> > > --- a/drivers/fpga/machxo2-common.c
> > > +++ b/drivers/fpga/machxo2-common.c
> > > @@ -17,6 +17,8 @@
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/property.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/time.h>
> > >  #include "machxo2-common.h"
> > >  
> > >  #define MACHXO2_LOW_DELAY_USEC          5
> > > @@ -24,6 +26,8 @@
> > >  #define MACHXO2_REFRESH_USEC            4800
> > >  #define MACHXO2_MAX_BUSY_LOOP           128
> > >  #define MACHXO2_MAX_REFRESH_LOOP        16
> > > +#define MACHXO2_MAX_ERASE_USEC          (30 * USEC_PER_SEC)
> > > +#define MACHXO2_ERASE_USEC_SLEEP        (20 * USEC_PER_MSEC)
> > >  
> > >  #define MACHXO2_PAGE_SIZE               16
> > >  #define MACHXO2_BUF_SIZE                (MACHXO2_PAGE_SIZE + 4)
> > > @@ -54,6 +58,18 @@
> > >  #define ISC_ERASE_FEATURE_ROW  BIT(17)
> > >  #define ISC_ERASE_UFM          BIT(19)
> > >  
> > > +static inline int machxo2_wait_until_not_busy_timeout(struct
> > > machxo2_common_priv *priv)
> > > +{
> > > +       int ret, pollret;
> > > +       u32 status = MACHXO2_BUSY;
> > > +
> > > +       pollret = read_poll_timeout(priv->get_status, ret,
> > > +                                   (ret && ret != -EAGAIN) ||
> > > !(status & MACHXO2_BUSY),
> > > +                                   MACHXO2_ERASE_USEC_SLEEP,
> > > MACHXO2_MAX_ERASE_USEC,
> > > +                                   true, priv, &status);
> > 
> > Why just taking care of erase timeout? I see the busy wait in many
> > places.
> > 
> 
> Erasing the flash memory takes significantly longer than the other
> operations (up to 30s), which is why I decided to use this separate
> implementation. For other commands the fpga indicates no-more-busy much
> faster than for the erase_flash command.

It is almost always better to have a relatively measureable timeout,
unless it is really time critical. Apparently spi/i2c transfer is not
time critical itself. So since you have implemented a better function,
why not use it?

Thanks,
Yilun

> 
> > > +
> > > +       return ret ?: pollret;
> > > +}
> > >  
> > >  static inline u8 get_err(u32 status)
> > >  {
> > > @@ -114,6 +130,12 @@ static int machxo2_cleanup(struct fpga_manager
> > > *mgr)
> > >         if (ret)
> > >                 goto fail;
> > >  
> > > +       ret = machxo2_wait_until_not_busy_timeout(priv);
> > > +       if (ret) {
> > > +               dev_err(&mgr->dev, "Erase operation failed (%d)",
> > > ret);
> > > +               goto fail;
> > > +       }
> > > +
> > >         ret = machxo2_wait_until_not_busy(priv);
> > 
> > Is this line still needed?
> 
> agreed, this line should become obsolete, since if we reach this point
> the fpga is not indicating busy any longer or the wait has been aborted
> due to an error. I will remove it in v2.
> 
> > 
> > >         if (ret)
> > >                 goto fail;
> > > @@ -192,9 +214,11 @@ static int machxo2_write_init(struct
> > > fpga_manager *mgr,
> > >         if (ret)
> > >                 goto fail;
> > >  
> > > -       ret = machxo2_wait_until_not_busy(priv);
> > > -       if (ret)
> > > +       ret = machxo2_wait_until_not_busy_timeout(priv);
> > > +       if (ret) {
> > > +               dev_err(&mgr->dev, "Erase operation failed (%d)",
> > > ret);
> > >                 goto fail;
> > > +       }
> > >  
> > >         priv->get_status(priv, &status);
> > >         if (status & MACHXO2_FAIL) {
> > > -- 
> > > 2.30.2
> > > 
> > 
> > 
> 
> -- 
> Pengutronix e.K.                | Johannes Zink                  |
> Steuerwalder Str. 21            | https://www.pengutronix.de/    |
> 31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
> Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |
> 



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux