On Fri, Oct 14, 2022 at 11:19:29PM +0800, Xu Yilun wrote:
> 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?
>
Surely, it could. IMHO it's just easier to read this way...
> > +}
> > +
> > +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);
