On Sun, Sep 04, 2022 at 12:53:27AM +0800, Xu Yilun wrote:
> On 2022-08-30 at 12:54:04 +0300, Ivan Bornyakov wrote:
> > Add support to the FPGA manager for programming Lattice ECP5 and MachXO2
> > FPGAs over slave SPI sysCONFIG interface.
> >
> > Signed-off-by: Ivan Bornyakov <i.bornyakov@xxxxxxxxxxx>
> > ---
> >
> > ... snip ...
> >
> > +static int sysconfig_spi_probe(struct spi_device *spi)
> > +{
> > + const struct sysconfig_fpga_priv *fpga_priv;
> > + const struct spi_device_id *dev_id;
> > + struct device *dev = &spi->dev;
> > + struct sysconfig_priv *priv;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + fpga_priv = of_device_get_match_data(dev);
> > + if (!fpga_priv) {
> > + dev_id = spi_get_device_id(spi);
> > + if (!dev_id)
> > + return -ENODEV;
> > +
> > + fpga_priv = (const struct sysconfig_fpga_priv *)dev_id->driver_data;
> > + }
> > +
> > + if (!fpga_priv)
> > + return -EINVAL;
> > +
> > + if (spi->max_speed_hz > fpga_priv->spi_max_speed_hz) {
> > + dev_err(dev, "SPI speed %u is too high, maximum speed is %u\n",
> > + spi->max_speed_hz, fpga_priv->spi_max_speed_hz);
> > + return -EINVAL;
> > + }
> > +
> > + priv->dev = dev;
> > + priv->spi = spi;
> > + priv->fpga_priv = fpga_priv;
> > + priv->sysconfig_transfer = sysconfig_spi_transfer;
> > + priv->sysconfig_fpga_mgr_ops.state = sysconfig_ops_state;
> > + priv->sysconfig_fpga_mgr_ops.write_init = sysconfig_ops_write_init;
>
> Why initialize the fpga_mgr_ops in spi driver? I think it should be the
> sysconfig core driver's job.
>
The reasoning was that there is bus-specific write callback.
> > + priv->sysconfig_fpga_mgr_ops.write = sysconfig_ops_spi_write;
>
> We already have the transfer callback, why we need a special spi write
> here.
>
Transfer callback performs transaction with single buffer on its input
(with optional read back). Paged write makes one write transaction with
two buffers, command + data, and it is only place where we need to
transfer several buffers. More of that, burst write needs locked bus.
IMHO, it is reasonable to have a bus-specific write.
> > + priv->sysconfig_fpga_mgr_ops.write_complete = sysconfig_ops_write_complete;
>
> same concern
>
> > +
> > + return sysconfig_probe(priv);
> > +}
> > +
> >
> > ... snip ...
> >
> > +int sysconfig_ops_write_init(struct fpga_manager *mgr,
> > + struct fpga_image_info *info,
> > + const char *buf, size_t count)
> > +{
> > + const struct sysconfig_fpga_priv *fpga_priv;
> > + struct sysconfig_priv *priv;
> > + struct device *dev;
> > + int ret;
> > +
> > + dev = &mgr->dev;
> > + priv = mgr->priv;
> > + fpga_priv = priv->fpga_priv;
> > +
> > + if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
> > + dev_err(dev, "Partial reconfiguration is not supported\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if (!fpga_priv->internal_flash) {
> > + /* Write directly to SRAM */
> > + ret = sysconfig_refresh(priv);
> > + if (ret) {
> > + dev_err(dev, "Failed to go to program mode\n");
> > + return ret;
> > + }
> > + }
> > +
> > + /* Enter ISC mode */
> > + ret = sysconfig_isc_init(priv);
> > + if (ret) {
> > + dev_err(dev, "Failed to go to ISC mode\n");
> > + return ret;
> > + }
> > +
> > + /* Initialize the Address Shift Register */
> > + ret = sysconfig_lsc_init_addr(priv);
> > + if (ret) {
> > + dev_err(dev,
> > + "Failed to initialize the Address Shift Register\n");
> > + return ret;
> > + }
> > +
> > + if (fpga_priv->spi_burst_write) {
> > + /* Prepare for SPI burst write */
> > + ret = sysconfig_lsc_burst_init(priv);
>
> Don't make the sysconfig.c dependent to sysconfig-spi.c, sysconfig.c
> should be common and workable without sysconfig-spi.c
>
Agreed. How about separate this if-block to a bus-specific
sysconfig_ops_spi_write_init()?
> > + if (ret)
> > + dev_err(dev,
> > + "Failed to prepare for bitstream burst write\n");
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static void sysconfig_cleanup(struct sysconfig_priv *priv)
> > +{
> > + sysconfig_isc_erase(priv);
> > + sysconfig_refresh(priv);
> > +}
> >
> > ... snip ...
> >
> > +int sysconfig_ops_write_complete(struct fpga_manager *mgr,
> > + struct fpga_image_info *info)
> > +{
> > + const struct sysconfig_fpga_priv *fpga_priv;
> > + struct sysconfig_priv *priv;
> > + struct device *dev;
> > + int ret;
> > +
> > + dev = &mgr->dev;
> > + priv = mgr->priv;
> > + fpga_priv = priv->fpga_priv;
> > +
> > + if (fpga_priv->spi_burst_write) {
> > + ret = sysconfig_lsc_burst_complete(priv);
>
> Same concern
>
Same as above, how about sysconfig_ops_spi_write_complete()?
> > + if (ret) {
> > + dev_err(dev,
> > + "Error while waiting bitstream write to finish\n");
> > + goto fail;
> > + }
> > + }
> > +
> > + if (fpga_priv->internal_flash) {
> > + ret = sysconfig_isc_prog_done(priv);
> > + if (!ret)
> > + ret = sysconfig_refresh(priv);
> > +
> > + if (ret) {
> > + dev_err(dev, "Failed to enable Self-Download Mode\n");
> > + goto fail;
> > + }
> > + }
> > +
> > + ret = sysconfig_isc_finish(priv);
> > +
> > +fail:
> > + if (ret)
> > + sysconfig_cleanup(priv);
> > +
> > + return ret;
> > +}
> >
> > ... snip ...
> >
> > +struct sysconfig_fpga_priv {
> > + u32 spi_max_speed_hz;
>
> Don't put bus specific stuff here, this should be common across all
> transport.
>
Personally, I don't see why not. This struct is just a bunch of
FPGA-specific constants which may or may not be used by a bus-specific
sysCONFIG implementation. IMHO it is more convinient to define them in
one place.
> > + u8 isc_enable_operand;
> > + bool spi_burst_write;
>
> same concern
>
> > + bool internal_flash;
> > +};
> > +
> > +extern const struct sysconfig_fpga_priv ecp5_data;
> > +extern const struct sysconfig_fpga_priv machxo2_data;
> > +
> > +struct sysconfig_priv {
> > + struct fpga_manager_ops sysconfig_fpga_mgr_ops;
> > + const struct sysconfig_fpga_priv *fpga_priv;
> > + struct gpio_desc *program;
> > + struct gpio_desc *init;
> > + struct gpio_desc *done;
> > + struct spi_device *spi;
>
> same concern
>
MachXO2's I2C isc_enable and lsc_refresh commands are 3-bytes long while
SPI variants are 4-bytes long. Having pointer to either spi_device or
i2c_client makes it easy to add quirks about transfer length to
sysconfig_isc_enable() and sysconfig_lsc_refresh() routines.
> > + struct device *dev;
> > + int (*sysconfig_transfer)(struct sysconfig_priv *priv,
> > + const void *tx_buf, size_t tx_len,
> > + void *rx_buf, size_t rx_len);
> > +};
> > +
> > +int sysconfig_poll_busy(struct sysconfig_priv *priv);
> > +int sysconfig_lsc_burst_init(struct sysconfig_priv *priv);
> > +int sysconfig_lsc_burst_complete(struct sysconfig_priv *priv);
> > +
> > +enum fpga_mgr_states sysconfig_ops_state(struct fpga_manager *mgr);
> > +int sysconfig_ops_write_init(struct fpga_manager *mgr,
> > + struct fpga_image_info *info,
> > + const char *buf, size_t count);
> > +int sysconfig_ops_write_complete(struct fpga_manager *mgr,
> > + struct fpga_image_info *info);
> > +
> > +int sysconfig_probe(struct sysconfig_priv *priv);
> > +
> > +#endif /* __LATTICE_SYSCONFIG_H */
> > --
> > 2.37.2
> >
> >
