On 2022-10-11 at 22:38:20 +0300, Ivan Bornyakov wrote:
> Add support to the FPGA manager for programming Lattice ECP5 FPGA over
> slave SPI sysCONFIG interface.
>
> sysCONFIG interface core functionality is separate from both ECP5 and
> SPI specifics, so support for other FPGAs with different port types can
> be added in the future.
>
> Signed-off-by: Ivan Bornyakov <i.bornyakov@xxxxxxxxxxx>
[...]
> +
> +static int sysconfig_read_busy(struct sysconfig_priv *priv)
> +{
> + const u8 lsc_check_busy[] = SYSCONFIG_LSC_CHECK_BUSY;
> + u8 busy;
> + int ret;
> +
> + ret = sysconfig_cmd_read(priv, lsc_check_busy, sizeof(lsc_check_busy),
> + &busy, sizeof(busy));
> +
> + return ret ? : busy;
> +}
> +
> +static int sysconfig_poll_busy(struct sysconfig_priv *priv)
> +{
> + unsigned long timeout;
> + int ret;
> +
> + timeout = jiffies + msecs_to_jiffies(SYSCONFIG_POLL_BUSY_TIMEOUT_MS);
> +
> + while (time_before(jiffies, timeout)) {
> + ret = sysconfig_read_busy(priv);
> + if (ret <= 0)
> + return ret;
> +
> + usleep_range(SYSCONFIG_POLL_INTERVAL_US,
> + SYSCONFIG_POLL_INTERVAL_US * 2);
> + }
> +
> + return -ETIMEDOUT;
As mentioned by Ahmad, could read_poll_timeout() be used?
> +}
> +
> +static int sysconfig_read_status(struct sysconfig_priv *priv, u32 *status)
> +{
> + const u8 lsc_read_status[] = SYSCONFIG_LSC_READ_STATUS;
> + __be32 device_status;
> + int ret;
> +
> + ret = sysconfig_cmd_read(priv, lsc_read_status, sizeof(lsc_read_status),
> + &device_status, sizeof(device_status));
> + if (ret)
> + return ret;
> +
> + *status = be32_to_cpu(device_status);
> +
> + return 0;
> +}
> +
> +static int sysconfig_poll_status(struct sysconfig_priv *priv, u32 *status)
> +{
> + int ret = sysconfig_poll_busy(priv);
> +
> + if (ret)
> + return ret;
> +
> + return sysconfig_read_status(priv, status);
> +}
> +
> +static int sysconfig_poll_gpio(struct gpio_desc *gpio, bool is_active)
> +{
> + unsigned long timeout;
> + int value;
> +
> + timeout = jiffies + msecs_to_jiffies(SYSCONFIG_POLL_GPIO_TIMEOUT_MS);
> +
> + while (time_before(jiffies, timeout)) {
> + value = gpiod_get_value(gpio);
> + if (value < 0)
> + return value;
> +
> + if ((is_active && value) || (!is_active && !value))
> + return 0;
> +
> + usleep_range(SYSCONFIG_POLL_INTERVAL_US,
> + SYSCONFIG_POLL_INTERVAL_US * 2);
> + }
> +
> + return -ETIMEDOUT;
Same.
[...]
> +int sysconfig_probe(struct sysconfig_priv *priv)
> +{
> + struct gpio_desc *program, *init, *done;
> + struct device *dev = priv->dev;
> + struct fpga_manager *mgr;
> +
> + if (!dev)
> + return -ENODEV;
> +
> + if (!priv->bitstream_burst_write_init) {
> + dev_err(dev,
> + "Callback for preparation for bitstream burst write is not defined\n");
> + return -EOPNOTSUPP;
-EINVAL is better?
> + }
> +
> + if (!priv->bitstream_burst_write) {
> + dev_err(dev,
> + "Callback for bitstream burst write is not defined\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (!priv->bitstream_burst_write_complete) {
> + dev_err(dev,
> + "Callback for finishing bitstream burst write is not defined\n");
> + return -EOPNOTSUPP;
> + }
command_transfer is optional?
And I think different err log for each missing callback is too trivial,
maybe just say like "ops missing" if any mandatory callback is missing.
> +
> + program = devm_gpiod_get_optional(dev, "program", GPIOD_OUT_LOW);
> + if (IS_ERR(program))
> + return dev_err_probe(dev, PTR_ERR(program),
> + "Failed to get PROGRAM GPIO\n");
> +
> + init = devm_gpiod_get_optional(dev, "init", GPIOD_IN);
> + if (IS_ERR(init))
> + return dev_err_probe(dev, PTR_ERR(init),
> + "Failed to get INIT GPIO\n");
> +
> + done = devm_gpiod_get_optional(dev, "done", GPIOD_IN);
> + if (IS_ERR(done))
> + return dev_err_probe(dev, PTR_ERR(done),
> + "Failed to get DONE GPIO\n");
> +
> + priv->program = program;
> + priv->init = init;
> + priv->done = done;
> +
> + mgr = devm_fpga_mgr_register(dev, "Lattice sysCONFIG FPGA Manager",
> + &sysconfig_fpga_mgr_ops, priv);
> +
> + return PTR_ERR_OR_ZERO(mgr);
> +}
> +EXPORT_SYMBOL(sysconfig_probe);
