Hi Yilun,
On Mon, 2022-08-29 at 17:47 +0800, Xu Yilun wrote:
> On 2022-08-25 at 16:13:43 +0200, Johannes Zink wrote:
> > From: Peter Jensen <pdj@xxxxxxxxxxxxxxx>
> >
> > The configuration flash of the machxo2 fpga can also be erased and
> > written over i2c instead of spi. Add this functionality to the
> > refactored common driver. Since some commands are shorter over I2C
> > than
> > they are over SPI some quirks are added to the common driver in
> > order to
> > account for that.
> >
> > Signed-off-by: Peter Jensen <pdj@xxxxxxxxxxxxxxx>
> > Signed-off-by: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
> > Signed-off-by: Johannes Zink <j.zink@xxxxxxxxxxxxxx>
> > ---
[snip]
>
> > +static int machxo2_i2c_get_status(struct machxo2_common_priv *bus,
> > u32 *status)
> > +{
> > + struct machxo2_i2c_priv *i2cPriv =
> > to_machxo2_i2c_priv(bus);
> > + struct i2c_client *client = i2cPriv->client;
> > + u8 read_status[] = LSC_READ_STATUS;
>
> The command word could also be bus agnostic. I think a callback like
> write_then_read(bus, txbuf, n_tx, rxbuf, n_rx) could be a better
> abstraction.
I agree. The only command reading from the fpga is the get_status
functionality but your proposal provides a cleaner implementation.
I will add it in v2.
>
> > + __be32 tmp;
> > + int ret;
> > + struct i2c_msg msg[] = {
> > + {
> > + .addr = client->addr,
> > + .flags = 0,
> > + .buf = read_status,
> > + .len = ARRAY_SIZE(read_status),
> > + }, {
> > + .addr = client->addr,
> > + .flags = I2C_M_RD,
> > + .buf = (u8 *) &tmp,
> > + .len = sizeof(tmp)
> > + }
> > + };
> > +
> > + ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> > + if (ret < 0)
> > + return ret;
> > + if (ret != ARRAY_SIZE(msg))
> > + return -EIO;
> > + *status = be32_to_cpu(tmp);
> > +
> > + return 0;
> > +}
> > +
> > +static int machxo2_i2c_write(struct machxo2_common_priv *common,
> > + struct machxo2_cmd *cmds, size_t
> > cmd_count)
> > +{
> > + struct machxo2_i2c_priv *i2c_priv =
> > to_machxo2_i2c_priv(common);
> > + struct i2c_client *client = i2c_priv->client;
> > + size_t i;
> > + int ret;
> > +
> > + for (i = 0; i < cmd_count; i++) {
> > + struct i2c_msg msg[] = {
> > + {
> > + .addr = client->addr,
> > + .buf = cmds[i].cmd,
> > + .len = cmds[i].cmd_len,
> > + },
> > + };
> > +
> > + ret = i2c_transfer(client->adapter, msg,
> > ARRAY_SIZE(msg));
> > + if (ret < 0)
> > + return ret;
> > + if (ret != ARRAY_SIZE(msg))
> > + return -EIO;
> > + if (cmds[i].delay_us)
> > + usleep_range(cmds[i].delay_us,
> > cmds[i].delay_us +
> > + cmds[i].delay_us / 4);
> > + if (i < cmd_count - 1) /* on any iteration except
> > for the last one... */
> > + ret = machxo2_wait_until_not_busy(common);
>
> Seems no need to implement the loop and wait in transportation layer,
> they are common. A callback like write(bus, txbuf, n_tx) is better?
>
> Thanks,
> Yilun
I have chosen this implementation mostly due to the fact that I don't
have a SPI machxo2 device to test against, so I am intentionally
keeping changes to a minimum.
Moving the wait between single commands into the transport layer is not
functionally equivalent, e.g. the ISC_ENABLE - ISC_ERASE command
sequence in the machxo2_write_init function would require two separate
messages with a wait time between them, which would deassert the CS
line between sending the messages via SPI if not sent as a sequence of
SPI transfers. For some of the commands, the fpga requires a delay
between the different commands, which was implemented by setting the
delay property of the spi transfer objects in the original driver.
This implementation tries to mimic the timing behaviour of the SPI
transfer delay property for the I2C implementation.
Best regards
Johannes
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int machxo2_i2c_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct device *dev = &client->dev;
> > + struct machxo2_i2c_priv *priv;
> > +
> > + priv = devm_kzalloc(dev, sizeof(struct machxo2_i2c_priv),
> > GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->client = client;
> > + priv->common.get_status = machxo2_i2c_get_status;
> > + priv->common.write_commands = machxo2_i2c_write;
> > +
> > + /* Commands are usually 4b, but these aren't for i2c */
> > + priv->common.enable_3b = true;
> > + priv->common.refresh_3b = true;
> > +
> > + return machxo2_common_init(&priv->common, dev);
> > +}
> > +
> > +static const struct of_device_id of_match[] = {
> > + { .compatible = "lattice,machxo2-slave-i2c", },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, of_match);
> > +
> > +static const struct i2c_device_id lattice_ids[] = {
> > + { "machxo2-slave-i2c", 0 },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(i2c, lattice_ids);
> > +
> > +static struct i2c_driver machxo2_i2c_driver = {
> > + .driver = {
> > + .name = "machxo2-slave-i2c",
> > + .of_match_table = of_match_ptr(of_match),
> > + },
> > + .probe = machxo2_i2c_probe,
> > + .id_table = lattice_ids,
> > +};
> > +
> > +module_i2c_driver(machxo2_i2c_driver);
> > +
> > +MODULE_AUTHOR("Peter Jensen <pdj@xxxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Load Lattice FPGA firmware over I2C");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.30.2
> >
>
--
Pengutronix e.K. | Johannes Zink |
Steuerwalder Str. 21 | https://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686| Fax: +49-5121-206917-5555 |
