On 2022-08-29 at 15:21:19 +0200, Johannes Zink wrote:
> 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.
Not sure if it is really a problem, but I remember SPI has various APIs
to deal with different requirements.
>
> This implementation tries to mimic the timing behaviour of the SPI
> transfer delay property for the I2C implementation.
Could you firstly try on that until we have real problem? Ideally this
is a cleaner implementation, is it?
Thanks,
Yilun
>
> 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 |
>
