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 |
>
