| Message ID | 20220825141343.1375690-16-j.zink@pengutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Lattice MachXO2 programming via I2C | expand |
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@pengutronix.de> > --- > 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. > + > + 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? > 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 >
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@pengutronix.de> > > --- > > 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. > > + > > + 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 > > > >
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@pengutronix.de> > > > --- > > > 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 | >
Hi Yilun, On Mon, 2022-08-29 at 22:57 +0800, Xu Yilun wrote: > 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@pengutronix.de> > > > > --- [snip] > > > > +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 That's a fair point. I will look in the datasheet how long the timeouts on the other operations should be set and will replace the former busy wait implementation in v2. I would not expect any breakage from that. Best regards Johannes
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); + + 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); 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) {
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@pengutronix.de> --- drivers/fpga/machxo2-common.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)