| Message ID | 20170413152916.19896-2-bst@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Bastian, I'm merging this patch to prepare v10 patch with cyclone_ps_spi driver, but have some questions, please see below. On Thu, 13 Apr 2017 17:29:16 +0200 Bastian Stender bst@pengutronix.de wrote: ... >@@ -117,12 +134,29 @@ static int cyclonespi_write_complete(struct fpga_manager *mgr, > struct fpga_image_info *info) > { > struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv; >+ const char dummy[] = {0, 0}; >+ int ret; > > if (gpiod_get_value(conf->status)) { > dev_err(&mgr->dev, "Error during configuration.\n"); > return -EIO; > } > >+ if (gpiod_get_value(conf->confd)) { >+ dev_err(&mgr->dev, "CONF_DONE is high!\n"); >+ return -EIO; >+ } isn't CONF_DONE high pin state expected here? Below comment also sounds like it is expected. But you return here when confd gpio is high? >+ /* >+ * After CONF_DONE goes high, send two additional falling edges on DCLK >+ * to begin initialization and enter user mode >+ */ >+ ret = spi_write(conf->spi, dummy, 2); isn't sending one byte enough here? Two falling edges with single byte transfer. ... >@@ -149,6 +183,13 @@ static int cyclonespi_probe(struct spi_device *spi) > return PTR_ERR(conf->config); > } > >+ conf->confd = devm_gpiod_get(&spi->dev, "confd", GPIOD_IN); >+ if (IS_ERR(conf->confd)) { >+ dev_err(&spi->dev, "Failed to get confd gpio: %ld\n", >+ PTR_ERR(conf->confd)); >+ return PTR_ERR(conf->confd); there are designs with CONF_DONE not connected, so this should be optional, like in the binding description. I'll drop this return. Thanks, Anatolij -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Anatolij, On 05/17/2017 10:15 AM, Anatolij Gustschin wrote: > I'm merging this patch to prepare v10 patch with cyclone_ps_spi driver, > but have some questions, please see below. Nice. > On Thu, 13 Apr 2017 17:29:16 +0200 > Bastian Stender bst@pengutronix.de wrote: > ... >> @@ -117,12 +134,29 @@ static int cyclonespi_write_complete(struct fpga_manager *mgr, >> struct fpga_image_info *info) >> { >> struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv; >> + const char dummy[] = {0, 0}; >> + int ret; >> >> if (gpiod_get_value(conf->status)) { >> dev_err(&mgr->dev, "Error during configuration.\n"); >> return -EIO; >> } >> >> + if (gpiod_get_value(conf->confd)) { >> + dev_err(&mgr->dev, "CONF_DONE is high!\n"); >> + return -EIO; >> + } > > isn't CONF_DONE high pin state expected here? Below comment also sounds > like it is expected. But you return here when confd gpio is high? With the corresponding device tree entry it is working as expected: &spi1 { .. fpga: arria@0 { compatible = "altr,fpga-passive-serial"; confd-gpios = <&gpio0 7 GPIO_ACTIVE_LOW>; .. }; }; >> + /* >> + * After CONF_DONE goes high, send two additional falling edges on DCLK >> + * to begin initialization and enter user mode >> + */ >> + ret = spi_write(conf->spi, dummy, 2); > > isn't sending one byte enough here? Two falling edges with single > byte transfer. Yes, sending one byte is sufficient. > > ... >> @@ -149,6 +183,13 @@ static int cyclonespi_probe(struct spi_device *spi) >> return PTR_ERR(conf->config); >> } >> >> + conf->confd = devm_gpiod_get(&spi->dev, "confd", GPIOD_IN); >> + if (IS_ERR(conf->confd)) { >> + dev_err(&spi->dev, "Failed to get confd gpio: %ld\n", >> + PTR_ERR(conf->confd)); >> + return PTR_ERR(conf->confd); > > there are designs with CONF_DONE not connected, so this should be > optional, like in the binding description. I'll drop this return. Right. Thank you. Regards, Bastian
On Wed, May 17, 2017 at 11:41:08AM +0200, Bastian Stender wrote: > Hi Anatolij, > > On 05/17/2017 10:15 AM, Anatolij Gustschin wrote: > > I'm merging this patch to prepare v10 patch with cyclone_ps_spi driver, > > but have some questions, please see below. > > Nice. > > > On Thu, 13 Apr 2017 17:29:16 +0200 > > Bastian Stender bst@pengutronix.de wrote: > > ... > > > @@ -117,12 +134,29 @@ static int cyclonespi_write_complete(struct fpga_manager *mgr, > > > struct fpga_image_info *info) > > > { > > > struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv; > > > + const char dummy[] = {0, 0}; > > > + int ret; > > > > > > if (gpiod_get_value(conf->status)) { > > > dev_err(&mgr->dev, "Error during configuration.\n"); > > > return -EIO; > > > } > > > > > > + if (gpiod_get_value(conf->confd)) { > > > + dev_err(&mgr->dev, "CONF_DONE is high!\n"); > > > + return -EIO; > > > + } > > > > isn't CONF_DONE high pin state expected here? Below comment also sounds > > like it is expected. But you return here when confd gpio is high? > > With the corresponding device tree entry it is working as expected: > > &spi1 { > .. > fpga: arria@0 { > compatible = "altr,fpga-passive-serial"; > confd-gpios = <&gpio0 7 GPIO_ACTIVE_LOW>; > .. > }; > }; > > > > + /* > > > + * After CONF_DONE goes high, send two additional falling edges on DCLK > > > + * to begin initialization and enter user mode > > > + */ > > > + ret = spi_write(conf->spi, dummy, 2); > > > > isn't sending one byte enough here? Two falling edges with single > > byte transfer. > > Yes, sending one byte is sufficient. In the meantime Bastian is more into that topic, but IIRC completely removing that dummy write also worked for me. Best regards Uwe
On Wed, 17 May 2017 11:41:08 +0200 Bastian Stender bst@pengutronix.de wrote: ... >>> + if (gpiod_get_value(conf->confd)) { >>> + dev_err(&mgr->dev, "CONF_DONE is high!\n"); >>> + return -EIO; >>> + } >> >> isn't CONF_DONE high pin state expected here? Below comment also sounds >> like it is expected. But you return here when confd gpio is high? > >With the corresponding device tree entry it is working as expected: > >&spi1 { > .. > fpga: arria@0 { > compatible = "altr,fpga-passive-serial"; > confd-gpios = <&gpio0 7 GPIO_ACTIVE_LOW>; > .. > }; >}; Ah, okay. Then it makes sense to use gpiod_get_raw_value() here. Other devices trees might be using active high flag. Thanks, Anatolij -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 17, 2017 at 01:24:50PM +0200, Anatolij Gustschin wrote: > On Wed, 17 May 2017 11:41:08 +0200 > Bastian Stender bst@pengutronix.de wrote: > ... > >>> + if (gpiod_get_value(conf->confd)) { > >>> + dev_err(&mgr->dev, "CONF_DONE is high!\n"); > >>> + return -EIO; > >>> + } > >> > >> isn't CONF_DONE high pin state expected here? Below comment also sounds > >> like it is expected. But you return here when confd gpio is high? > > > >With the corresponding device tree entry it is working as expected: > > > >&spi1 { > > .. > > fpga: arria@0 { > > compatible = "altr,fpga-passive-serial"; > > confd-gpios = <&gpio0 7 GPIO_ACTIVE_LOW>; > > .. > > }; > >}; > > Ah, okay. Then it makes sense to use gpiod_get_raw_value() here. > Other devices trees might be using active high flag. Other device trees might be using active high because the signal is inverted between the FPGA and the gpio controller. So the argument is to use gpiod_get_value because others might want the gpio to be active high. There are many drivers out there that got this wrong and now we have to live with kludges because of this. Using gpiod_get_value has only one downside: If the device tree is wrong your device doesn't work. But that's sort of expected ... Best regards Uwe
On Wed, 17 May 2017 16:40:35 +0200 Uwe Kleine-König u.kleine-koenig@pengutronix.de wrote: ... >> >&spi1 { >> > .. >> > fpga: arria@0 { >> > compatible = "altr,fpga-passive-serial"; >> > confd-gpios = <&gpio0 7 GPIO_ACTIVE_LOW>; >> > .. >> > }; >> >}; >> >> Ah, okay. Then it makes sense to use gpiod_get_raw_value() here. >> Other devices trees might be using active high flag. > >Other device trees might be using active high because the signal is >inverted between the FPGA and the gpio controller. CONF_DONE signal is active high, so I would expect device trees to describe it as GPIO_ACTIVE_HIGH, unless an inverter is used. Invertor users should set GPIO_ACTIVE_LOW. >So the argument is to use gpiod_get_value because others might want the >gpio to be active high. There are many drivers out there that got this >wrong and now we have to live with kludges because of this. > >Using gpiod_get_value has only one downside: If the device tree is wrong >your device doesn't work. But that's sort of expected ... then, with the correct device tree the driver code should be: if (!gpiod_get_value(conf->confd)) { dev_err(&mgr->dev, "CONF_DONE is low!\n"); return -EIO; } This will work for users with and without a CONF_DONE invertor. Thanks, Anatolij -- To unsubscribe from this list: send the line "unsubscribe linux-fpga" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Thu, May 18, 2017 at 03:36:52PM +0200, Anatolij Gustschin wrote: > then, with the correct device tree the driver code should be: > > if (!gpiod_get_value(conf->confd)) { > dev_err(&mgr->dev, "CONF_DONE is low!\n"); > return -EIO; > } > > This will work for users with and without a CONF_DONE invertor. IMHO "inactive" is better than "low". Best regards Uwe
On Thursday, May 18, 2017 3:55:39 PM PDT Uwe Kleine-König wrote: > Hello, > > On Thu, May 18, 2017 at 03:36:52PM +0200, Anatolij Gustschin wrote: > > then, with the correct device tree the driver code should be: > > if (!gpiod_get_value(conf->confd)) { > > > > dev_err(&mgr->dev, "CONF_DONE is low!\n"); > > return -EIO; > > > > } > > > > This will work for users with and without a CONF_DONE invertor. > > IMHO "inactive" is better than "low". > > Best regards > Uwe Agreed. Since gpiod deals with "logical" 1 and 0, it is better to call out its meaning.
diff --git a/drivers/fpga/cyclone-ps-spi.c b/drivers/fpga/cyclone-ps-spi.c index 56c385347bb7..175370d27404 100644 --- a/drivers/fpga/cyclone-ps-spi.c +++ b/drivers/fpga/cyclone-ps-spi.c @@ -22,12 +22,12 @@ #include <linux/spi/spi.h> #include <linux/sizes.h> -#define FPGA_RESET_TIME 50 /* time in usecs to trigger FPGA config */ -#define FPGA_MIN_DELAY 50 /* min usecs to wait for config status */ -#define FPGA_MAX_DELAY 1000 /* max usecs to wait for config status */ +#define FPGA_MIN_DELAY 268 /* min usecs to wait for config status -> min(t_STATUS) */ +#define FPGA_MAX_DELAY 3000 /* max usecs to wait for config status -> max(t_STATUS) */ struct cyclonespi_conf { struct gpio_desc *config; + struct gpio_desc *confd; struct gpio_desc *status; struct spi_device *spi; }; @@ -48,6 +48,15 @@ static enum fpga_mgr_states cyclonespi_state(struct fpga_manager *mgr) return FPGA_MGR_STATE_UNKNOWN; } +/* | Arria 10 | + * t_CF2ST0 | [; 600] | ns + * t_CFG | [2;] | µs + * t_STATUS | [268; 3000] | µs + * t_CF2ST1 | [; 3000] | µs + * t_CF2CK | [3010 µs;] | µs + * t_ST2CK | [10;] | µs + * t_CD2UM | [175; 830] | µs + */ static int cyclonespi_write_init(struct fpga_manager *mgr, struct fpga_image_info *info, const char *buf, size_t count) @@ -61,17 +70,25 @@ static int cyclonespi_write_init(struct fpga_manager *mgr, } gpiod_set_value(conf->config, 1); - usleep_range(FPGA_RESET_TIME, FPGA_RESET_TIME + 20); + /* max { min(t_CFG), max(tCF2ST0) } */ + udelay(2); if (!gpiod_get_value(conf->status)) { dev_err(&mgr->dev, "Status pin failed to show a reset\n"); return -EIO; } gpiod_set_value(conf->config, 0); + + /* wait for max { max(t_STATUS), max(t_CF2ST1) } */ for (i = 0; i < (FPGA_MAX_DELAY / FPGA_MIN_DELAY); i++) { usleep_range(FPGA_MIN_DELAY, FPGA_MIN_DELAY + 20); - if (!gpiod_get_value(conf->status)) + if (!gpiod_get_value(conf->status)) { + /* + * wait for min(t_ST2CK) + */ + udelay(10); return 0; + } } dev_err(&mgr->dev, "Status pin not ready.\n"); @@ -117,12 +134,29 @@ static int cyclonespi_write_complete(struct fpga_manager *mgr, struct fpga_image_info *info) { struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv; + const char dummy[] = {0, 0}; + int ret; if (gpiod_get_value(conf->status)) { dev_err(&mgr->dev, "Error during configuration.\n"); return -EIO; } + if (gpiod_get_value(conf->confd)) { + dev_err(&mgr->dev, "CONF_DONE is high!\n"); + return -EIO; + } + + /* + * After CONF_DONE goes high, send two additional falling edges on DCLK + * to begin initialization and enter user mode + */ + ret = spi_write(conf->spi, dummy, 2); + if (ret) { + dev_err(&mgr->dev, "spi error during end sequence: %d\n", ret); + return ret; + } + return 0; } @@ -149,6 +183,13 @@ static int cyclonespi_probe(struct spi_device *spi) return PTR_ERR(conf->config); } + conf->confd = devm_gpiod_get(&spi->dev, "confd", GPIOD_IN); + if (IS_ERR(conf->confd)) { + dev_err(&spi->dev, "Failed to get confd gpio: %ld\n", + PTR_ERR(conf->confd)); + return PTR_ERR(conf->confd); + } + conf->status = devm_gpiod_get(&spi->dev, "nstat", GPIOD_IN); if (IS_ERR(conf->status)) { dev_err(&spi->dev, "Failed to get status gpio: %ld\n",