Date:   Thu, 24 Jan 2019 09:25:25 -0800
From:   Moritz Fischer <mdf@...nel.org>
To:     Dick Hollenbeck <dick@...tplc.com>
Cc:     Alan Tull <atull@...nel.org>, linux-fpga@...r.kernel.org,
        Moritz Fischer <mdf@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Add Cyclone-10 support to altera-ps-spi.c.

Hi Dick,

On Fri, Jan 18, 2019 at 03:41:04PM -0600, Dick Hollenbeck wrote:
>  Add Cyclone-10 support to altera-ps-spi.c.
>  Invert logic involving nCONFIG, nSTATUS, and CONFIG_DONE so it matches datasheets
>  by going with GPIO_ACTIVE_HIGH instead of GPIO_ACTIVE_LOW in the device tree.  Add
>  optional devicetree properties spi-lsb-first and write-block-size.
> 
> Signed-off-by: Dick Hollenbeck <dick@...tplc.com>
> ---
>  .../bindings/fpga/altera-passive-serial.txt   |  71 +++++--
>  drivers/fpga/altera-ps-spi.c                  | 200 ++++++++++++------
>  include/linux/fpga/fpga-mgr.h                 |   5 +-
>  3 files changed, 189 insertions(+), 87 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/fpga/altera-passive-serial.txt
> b/Documentation/devicetree/bindings/fpga/altera-passive-serial.txt
> index 48478bc07..2cffc8c0a 100644
> --- a/Documentation/devicetree/bindings/fpga/altera-passive-serial.txt
> +++ b/Documentation/devicetree/bindings/fpga/altera-passive-serial.txt
> @@ -8,22 +8,67 @@ circuits in order to play nicely with other SPI slaves on the same bus.
>  See https://www.altera.com/literature/hb/cyc/cyc_c51013.pdf
> 
>  Required properties:
> -- compatible: Must be one of the following:
> -	"altr,fpga-passive-serial",
> -	"altr,fpga-arria10-passive-serial"
> -- reg: SPI chip select of the FPGA
> -- nconfig-gpios: config pin (referred to as nCONFIG in the manual)
> -- nstat-gpios: status pin (referred to as nSTATUS in the manual)
> +- compatible:		Must be one of the following:
> +				"altr,fpga-passive-serial",
> +				"altr,fpga-arria10-passive-serial"
> +				"altr,fpga-cyclone10-passive-serial"
> +- reg:			SPI chip select of the FPGA
> +- nconfig-gpios:		nCONFIG pin
> +- nstat-gpios:		nSTATUS pin
> 
>  Optional properties:
> -- confd-gpios: confd pin (referred to as CONF_DONE in the manual)
> +- confd-gpios:		CONF_DONE pin
> +- write-block-size:	Number of bytes to write each time through a loop.
> +			Can set to 0xffffffff to perform in a single write.
> 
>  Example:
> -	fpga: fpga@0 {
> -		compatible = "altr,fpga-passive-serial";
> +
> +&pio {
> +	fpgamgr0_outs: fpgamgr0_outs {
> +		pins = "PG8";
> +		function = "gpio_out";
> +		output-high;
> +		drive-strength = <10>;			/* milliamps, down from 20 default */
> +	};
> +	fpgamgr0_ins: fpgamgr0_ins {
> +		pins = "PG6", "PG7";
> +		function = "gpio_in";
> +	};
> +
> +	fpgabridge0_outs: fpgabridge0_outs {
> +		pins =	"PA0", "PA1", "PA2", "PA3", "PA6", "PA11", "PA12", "PA17", /* data[0:7] */
> +			"PA21",				/* ACK */
> +			"PA18",				/* WR_DATA */
> +			"PA19",				/* RD_DATA */
> +			"PA20";				/* WR_ADDR */
> +
> +		function = "gpio_out";
> +		drive-strength = <10>;			/* milliamps, down from 20 default */
> +	};
> +
> +	fpgabridge0_ins: fpgabridge0_ins {
> +		pins =	"PA21";				/* ACK */
> +		function = "gpio_ins";
> +	};
> +};
> +
> +&spi0 {
> +	status = "okay";
> +
> +	fpga_mgr0: fpga-mgr@3 {
> +		compatible = "altr,fpga-cyclone10-passive-serial";
>  		spi-max-frequency = <20000000>;
> -		reg = <0>;
> -		nconfig-gpios = <&gpio4 9 GPIO_ACTIVE_LOW>;
> -		nstat-gpios = <&gpio4 11 GPIO_ACTIVE_LOW>;
> -		confd-gpios = <&gpio4 12 GPIO_ACTIVE_LOW>;
> +		reg = <3>;	/* leave room for spi node addresses 0-2 */
> +		pinctrl-names = "default";
> +		pinctrl-0 = <	&fpgamgr0_outs
> +				&fpgamgr0_ins
> +				&fpgabridge0_outs
> +				&fpgabridge0_ins
> +				>;
> +		nconfig-gpios = <&pio 6 8 GPIO_ACTIVE_HIGH>;	/* PG8 */
> +		nstat-gpios	  = <&pio 6 7 GPIO_ACTIVE_HIGH>;	/* PG7 */
> +		confd-gpios	  = <&pio 6 6 GPIO_ACTIVE_HIGH>;	/* PG6 */
> +		spi-lsb-first;	/* SPI controller should send LSbit first to this slave */
> +		write-block-size = <0xffffffff>;
>  	};
> +};
> diff --git a/drivers/fpga/altera-ps-spi.c b/drivers/fpga/altera-ps-spi.c
> index 33aafda50..0c6c90d0c 100644
> --- a/drivers/fpga/altera-ps-spi.c
> +++ b/drivers/fpga/altera-ps-spi.c
> @@ -12,8 +12,9 @@
>   * Manage Altera FPGA firmware that is loaded over SPI using the passive
>   * serial configuration method.
>   * Firmware must be in binary "rbf" format.
> - * Works on Arria 10, Cyclone V and Stratix V. Should work on Cyclone series.
> - * May work on other Altera FPGAs.
> + * Works on Arria 10, Cyclone V, Cyclone 10, and Stratix V.
> + * May work on other Altera FPGAs.  For Cyclone 10, additional file formats
> + * of *.hex and *.ttf are allegedly accepted by the FPGA.
>   */
> 
>  #include <linux/bitrev.h>
> @@ -26,8 +27,14 @@
>  #include <linux/spi/spi.h>
>  #include <linux/sizes.h>
> 
> +
> +#define FLAGS_SPI_LSB_FIRST	BIT(0)	/* saw "spi-lsb-first" in DTB's SPI FPGA manager slave */
> +
> +#define WR_BLOCKZ_BYTES		SZ_4K	/* chosen when "write-block-size" not supplied in
> devicetree */
> +
>  enum altera_ps_devtype {
>  	CYCLONE5,
> +	CYCLONE10,
>  	ARRIA10,
>  };
> 
> @@ -40,24 +47,29 @@ struct altera_ps_data {
>  };
> 
>  struct altera_ps_conf {
> -	struct gpio_desc *config;
> +	struct gpio_desc *nconfig;
>  	struct gpio_desc *confd;
>  	struct gpio_desc *status;
>  	struct spi_device *spi;
> +	u32 write_block_size;			/* in bytes */
>  	const struct altera_ps_data *data;
> -	u32 info_flags;
>  	char mgr_name[64];
> +	u8 image_flags;
> +	u8 mgr_flags;
>  };
> 
> -/*          |   Arria 10  |   Cyclone5  |   Stratix5  |
> - * t_CF2ST0 |     [; 600] |     [; 600] |     [; 600] |ns
> - * t_CFG    |        [2;] |        [2;] |        [2;] |µs
> - * t_STATUS | [268; 3000] | [268; 1506] | [268; 1506] |µs
> - * t_CF2ST1 |    [; 3000] |    [; 1506] |    [; 1506] |µs
> - * t_CF2CK  |     [3010;] |     [1506;] |     [1506;] |µs
> - * t_ST2CK  |       [10;] |        [2;] |        [2;] |µs
> - * t_CD2UM  |  [175; 830] |  [175; 437] |  [175; 437] |µs
> +/*
> + * See
> https://www.intel.com/content/www/us/en/programmable/documentation/sss1412661044400.html
> + *          |   Arria 10  |   Cyclone5  |   Stratix5  |  Cyclone 10 LP  |
> + * t_CF2ST0 |     [; 600] |     [; 600] |     [; 600] |         [; 500] |ns
> + * t_CFG    |        [2;] |        [2;] |        [2;] |          [0.5;] |µs
> + * t_STATUS | [268; 3000] | [268; 1506] | [268; 1506] |       [45; 230] |µs
> + * t_CF2ST1 |    [; 3000] |    [; 1506] |    [; 1506] |         [; 230] |µs
> + * t_CF2CK  |     [3010;] |     [1506;] |     [1506;] |          [230;] |µs
> + * t_ST2CK  |       [10;] |        [2;] |        [2;] |            [2;] |µs
> + * t_CD2UM  |  [175; 830] |  [175; 437] |  [175; 437] |      [300; 650] |µs
>   */
> +
>  static struct altera_ps_data c5_data = {
>  	/* these values for Cyclone5 are compatible with Stratix5 */
>  	.devtype = CYCLONE5,
> @@ -69,15 +81,24 @@ static struct altera_ps_data c5_data = {
> 
>  static struct altera_ps_data a10_data = {
>  	.devtype = ARRIA10,
> -	.status_wait_min_us = 268,  /* min(t_STATUS) */
> -	.status_wait_max_us = 3000, /* max(t_CF2ST1) */
> -	.t_cfg_us = 2,    /* max { min(t_CFG), max(tCF2ST0) } */
> -	.t_st2ck_us = 10, /* min(t_ST2CK) */
> +	.status_wait_min_us = 268,	/* min(t_STATUS) */
> +	.status_wait_max_us = 3000,	/* max(t_CF2ST1) */
> +	.t_cfg_us = 2,			/* max { min(t_CFG), max(tCF2ST0) } */
> +	.t_st2ck_us = 10,		/* min(t_ST2CK) */
> +};
> +
> +static struct altera_ps_data c10_data = {
> +	.devtype = CYCLONE10,
> +	.status_wait_min_us = 45,
> +	.status_wait_max_us = 230,
> +	.t_cfg_us = 1,
> +	.t_st2ck_us = 2,
>  };
> 
>  static const struct of_device_id of_ef_match[] = {
>  	{ .compatible = "altr,fpga-passive-serial", .data = &c5_data },
>  	{ .compatible = "altr,fpga-arria10-passive-serial", .data = &a10_data },
> +	{ .compatible = "altr,fpga-cyclone10-passive-serial", .data = &c10_data },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, of_ef_match);
> @@ -86,13 +107,13 @@ static enum fpga_mgr_states altera_ps_state(struct fpga_manager *mgr)
>  {
>  	struct altera_ps_conf *conf = mgr->priv;
> 
> -	if (gpiod_get_value_cansleep(conf->status))
> +	if (!gpiod_get_value_cansleep(conf->status))
>  		return FPGA_MGR_STATE_RESET;
> 
>  	return FPGA_MGR_STATE_UNKNOWN;
>  }
> 
> -static inline void altera_ps_delay(int delay_us)
> +static void altera_ps_delay(unsigned delay_us)
>  {
>  	if (delay_us > 10)
>  		usleep_range(delay_us, delay_us + 5);
> @@ -100,51 +121,56 @@ static inline void altera_ps_delay(int delay_us)
>  		udelay(delay_us);
>  }
> 
> +/* wait up to delay_us for a gpio pin to enter a goal state */
> +static bool gpio_wait(bool goal, struct gpio_desc *pin, unsigned delay_us)
> +{
> +	ktime_t deadline = ktime_add_us(ktime_get(), delay_us);
> +
> +	do {	/* delay at least once, ktime_get() may not have changed */
> +		altera_ps_delay(10);
> +		if (gpiod_get_value_cansleep(pin) == goal)
> +			return true;
> +
> +		/* as of today, ktime_before() and ktime_after() are badly implemented, not using them
> here */
> +	} while (ktime_sub(deadline,ktime_get()) > 0);
> +
> +	return false;
> +}
> +
>  static int altera_ps_write_init(struct fpga_manager *mgr,
> -				struct fpga_image_info *info,
> +				struct fpga_image_info *image_info,
>  				const char *buf, size_t count)
>  {
>  	struct altera_ps_conf *conf = mgr->priv;
> -	int min, max, waits;
> -	int i;
> -
> -	conf->info_flags = info->flags;
> +	conf->image_flags = image_info->flags;
> 
> -	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
> +	if (image_info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
>  		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
>  		return -EINVAL;
>  	}
> 
> -	gpiod_set_value_cansleep(conf->config, 1);
> +	if (!gpiod_get_value_cansleep(conf->status)) {
> +		dev_err(&mgr->dev, "nSTATUS initial state is not hi.\n");
> +		return -EIO;
> +	}
> 
> -	/* wait min reset pulse time */
> -	altera_ps_delay(conf->data->t_cfg_us);
> +	gpiod_set_value_cansleep(conf->nconfig, 0);
> 
> -	if (!gpiod_get_value_cansleep(conf->status)) {
> -		dev_err(&mgr->dev, "Status pin failed to show a reset\n");
> +	/* wait min reset pulse time */
> +	if (!gpio_wait(false, conf->status, conf->data->t_cfg_us)) {
> +		dev_err(&mgr->dev, "nSTATUS failed to show a reset\n");
>  		return -EIO;
>  	}
> 
> -	gpiod_set_value_cansleep(conf->config, 0);
> -
> -	min = conf->data->status_wait_min_us;
> -	max = conf->data->status_wait_max_us;
> -	waits = max / min;
> -	if (max % min)
> -		waits++;
> -
> -	/* wait for max { max(t_STATUS), max(t_CF2ST1) } */
> -	for (i = 0; i < waits; i++) {
> -		usleep_range(min, min + 10);
> -		if (!gpiod_get_value_cansleep(conf->status)) {
> -			/* wait for min(t_ST2CK)*/
> -			altera_ps_delay(conf->data->t_st2ck_us);
> -			return 0;
> -		}
> +	gpiod_set_value_cansleep(conf->nconfig, 1);
> +
> +	if (!gpio_wait(true, conf->status, conf->data->status_wait_max_us)) {
> +		dev_err(&mgr->dev, "nSTATUS not ready.\n");
> +		return -EIO;
>  	}
> 
> -	dev_err(&mgr->dev, "Status pin not ready.\n");
> -	return -EIO;
> +	dev_dbg(&mgr->dev, "%s: nSTATUS OK\n", __func__);
> +	return 0;
>  }
> 
>  static void rev_buf(char *buf, size_t len)
> @@ -176,12 +202,28 @@ static int altera_ps_write(struct fpga_manager *mgr, const char *buf,
>  	const char *fw_data = buf;
>  	const char *fw_data_end = fw_data + count;
> 
> +	/* u-boot does not loop when doing something similar.  Sometimes there
> +	 * can be clock glitches at the edges of these multiple frames depending
> +	 * on the quality of the CPU's spi driver.  For that
> +	 * reason it is possible to do the writing in one frame by setting the
> +	 * write_block_size large enough in the device tree such that this
> +	 * "loop" finishes up in one pass through.
> +	 */
>  	while (fw_data < fw_data_end) {
> -		int ret;
> -		size_t stride = min_t(size_t, fw_data_end - fw_data, SZ_4K);
> 
> -		if (!(conf->info_flags & FPGA_MGR_BITSTREAM_LSB_FIRST))
> +		int ret;
> +		size_t stride = min_t(size_t, fw_data_end - fw_data, conf->write_block_size);
> +
> +		/* Does the SPI controller send LSbit first, typically not.
> +		 * In any case the FPGA wants LSbit first.  Endian reverse bits in
> +		 * byte if needed so that the LSbit of each image byte is sent first,
> +		 * in the case where the SPI controller sends the MSbit first
> +		 * (of what will now be bit reversed bytes).
> +		 */
> +		if (!(conf->mgr_flags & FLAGS_SPI_LSB_FIRST)) {
> +			dev_dbg(&mgr->dev, "rev_buf() bit swapping\n");
>  			rev_buf((char *)fw_data, stride);
> +		}
> 
>  		ret = spi_write(conf->spi, fw_data, stride);
>  		if (ret) {
> @@ -192,31 +234,30 @@ static int altera_ps_write(struct fpga_manager *mgr, const char *buf,
>  		fw_data += stride;
>  	}
> 
> +	dev_dbg(&mgr->dev, "%s: OK\n", __func__);
>  	return 0;
>  }
> 
>  static int altera_ps_write_complete(struct fpga_manager *mgr,
> -				    struct fpga_image_info *info)
> +				    struct fpga_image_info *image_info)
>  {
>  	struct altera_ps_conf *conf = mgr->priv;
>  	const char dummy[] = {0};
>  	int ret;
> 
> -	if (gpiod_get_value_cansleep(conf->status)) {
> -		dev_err(&mgr->dev, "Error during configuration.\n");
> +	if (!gpiod_get_value_cansleep(conf->status)) {
> +		dev_err(&mgr->dev, "nSTATUS error at end of configuration!\n");
>  		return -EIO;
>  	}
> 
> -	if (!IS_ERR(conf->confd)) {
> -		if (!gpiod_get_raw_value_cansleep(conf->confd)) {
> -			dev_err(&mgr->dev, "CONF_DONE is inactive!\n");
> -			return -EIO;
> -		}
> +	if (!IS_ERR(conf->confd) && !gpio_wait(true, conf->confd, 500)) {
> +		dev_err(&mgr->dev, "CONF_DONE error at end of configuration!\n");
> +		return -EIO;
>  	}
> 
>  	/*
> -	 * After CONF_DONE goes high, send two additional falling edges on DCLK
> -	 * to begin initialization and enter user mode
> +	 * After CONF_DONE goes high, send at least two additional falling
> +	 * edges on DCLK to begin initialization and enter user mode.
>  	 */
>  	ret = spi_write(conf->spi, dummy, 1);
>  	if (ret) {
> @@ -224,6 +265,7 @@ static int altera_ps_write_complete(struct fpga_manager *mgr,
>  		return ret;
>  	}
> 
> +	dev_dbg(&mgr->dev, "%s: OK\n", __func__);
>  	return 0;
>  }
> 
> @@ -240,44 +282,62 @@ static int altera_ps_probe(struct spi_device *spi)
>  	const struct of_device_id *of_id;
>  	struct fpga_manager *mgr;
> 
> +	dev_dbg(&spi->dev, "altera_ps_probe\n");
> +
>  	conf = devm_kzalloc(&spi->dev, sizeof(*conf), GFP_KERNEL);
> -	if (!conf)
> +	if (!conf) {
>  		return -ENOMEM;
> +	}
> 
>  	of_id = of_match_device(of_ef_match, &spi->dev);
> -	if (!of_id)
> +	if (!of_id) {
> +		dev_dbg(&spi->dev, "no of_id\n");
>  		return -ENODEV;
> +	}
> 
>  	conf->data = of_id->data;
>  	conf->spi = spi;
> -	conf->config = devm_gpiod_get(&spi->dev, "nconfig", GPIOD_OUT_LOW);
> -	if (IS_ERR(conf->config)) {
> -		dev_err(&spi->dev, "Failed to get config gpio: %ld\n",
> -			PTR_ERR(conf->config));
> -		return PTR_ERR(conf->config);
> +	conf->nconfig = devm_gpiod_get(&spi->dev, "nconfig", GPIOD_OUT_HIGH);
> +	if (IS_ERR(conf->nconfig)) {
> +		dev_err(&spi->dev, "Failed to get nCONFIG gpio: %ld\n",
> +			PTR_ERR(conf->nconfig));
> +		return PTR_ERR(conf->nconfig);
>  	}
> 
>  	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",
> +		dev_err(&spi->dev, "Failed to get nSTATUS gpio: %ld\n",
>  			PTR_ERR(conf->status));
>  		return PTR_ERR(conf->status);
>  	}
> 
>  	conf->confd = devm_gpiod_get(&spi->dev, "confd", GPIOD_IN);
>  	if (IS_ERR(conf->confd)) {
> -		dev_warn(&spi->dev, "Not using confd gpio: %ld\n",
> +		dev_warn(&spi->dev, "Not using CONFIG_DONE gpio: %ld\n",
>  			 PTR_ERR(conf->confd));
>  	}
> 
> +	if (of_find_property(spi->dev.of_node, "spi-lsb-first", NULL)) {
> +		dev_dbg(&spi->dev, "SPI is LSbit first");
> +		conf->mgr_flags |= FLAGS_SPI_LSB_FIRST;
> +	}
> +
> +	if (of_property_read_u32(spi->dev.of_node, "write-block-size",
> +				 &conf->write_block_size))
> +		conf->write_block_size = WR_BLOCKZ_BYTES;
> +
> +	dev_dbg(&spi->dev, "write_block_size:%u\n", conf->write_block_size);
> +
>  	/* Register manager with unique name */
>  	snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s %s",
>  		 dev_driver_string(&spi->dev), dev_name(&spi->dev));
> 
>  	mgr = devm_fpga_mgr_create(&spi->dev, conf->mgr_name,
>  				   &altera_ps_ops, conf);
> -	if (!mgr)
> +	if (!mgr) {
> +		dev_dbg(&spi->dev, "no mgr\n");
>  		return -ENOMEM;
> +	}
> 
>  	spi_set_drvdata(spi, mgr);
> 
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index e8ca62b2c..7ac77c3ae 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -64,15 +64,12 @@ enum fpga_mgr_states {
>   *
>   * %FPGA_MGR_ENCRYPTED_BITSTREAM: indicates bitstream is encrypted
>   *
> - * %FPGA_MGR_BITSTREAM_LSB_FIRST: SPI bitstream bit order is LSB first
> - *
>   * %FPGA_MGR_COMPRESSED_BITSTREAM: FPGA bitstream is compressed
>   */
>  #define FPGA_MGR_PARTIAL_RECONFIG	BIT(0)
>  #define FPGA_MGR_EXTERNAL_CONFIG	BIT(1)
>  #define FPGA_MGR_ENCRYPTED_BITSTREAM	BIT(2)
> -#define FPGA_MGR_BITSTREAM_LSB_FIRST	BIT(3)
> -#define FPGA_MGR_COMPRESSED_BITSTREAM	BIT(4)
> +#define FPGA_MGR_COMPRESSED_BITSTREAM	BIT(3)
> 
>  /**
>   * struct fpga_image_info - information specific to a FPGA image
> -- 
> 2.17.1

I'm gonna skip over the patch and get straight to my reply to your
off-list emails to Alan:

As you know the Linux Kernel is an Open Source project, we value your
contribution; however it does need to follow our coding standards and
pass review.
No free lunch - not for the elderly, not for fellow maintainers - our
own patches go through the same *public* review on the list, just like
everybody else's. This ensures continued high quality and longterm
maintainability of the Linux Kernel codebase.
Please stop asking for preferential treatment based on your perceived
status as former Kicad contributor? off-list.

Your threats of 'walking away' are empty and will not get us anywhere:
If somebody beyond yourself / your employer values you contribution
enough so that he / she is willing to put in the work to fix up the
contribution up to Linux Kernel coding standards and follow the
submission process, we will happily apply your patches.
In the meantime it will stay - archived, for the public to find should
the need ever arise - outside of the Linux Kernel codebase.

In any case thank you for sharing the code with the community even in
it's current form.

Furthermore I personally would encourage you to refrain from further
insulting the community, our process, and the people working on the
project in your off-list emails.

I wish you all the best in your entrepreneurial endeavours,

Moritz

Powered by blists - more mailing lists