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:

quoted

On 2022-08-25 at 16:13:43 +0200, Johannes Zink wrote:

quoted

From: Peter Jensen <redacted>

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 <redacted>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Johannes Zink <redacted>
---

[snip]

quoted

quoted

quoted

+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.

quoted

quoted

+       __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

quoted

quoted

+       }
+
+       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 [off-list ref]");
+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    |