Hi Yilun,

On Mon, 2022-08-29 at 22:45 +0800, Xu Yilun wrote:

On 2022-08-29 at 15:21:19 +0200, Johannes Zink wrote:

quoted

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

quoted

quoted

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

I assume this could probably be implemented by clearing the cs_change
bit in the SPI transfer, though just sending multiple transfers in
sequence with the appropriate timing appears a bit more elegant to me,
since it doesn't reimplement the behaviour for spi, it simply extends
the i2c part for what is not supported natively in the i2c api. Either
way, some sort of waiting has to be implemented (please see my comment
below).

Also, please bear in mind that I do not have SPI connected on my board,
which is why I opted to stay as close as possible to the original
implementation and only refactor the spi transfers with functionally
equivalent code in order to keep the risk of breaking things as low as
possible.

quoted

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?

The delays themselves are required by the device family datasheet and
by the sysConfig protocol definition, as part of the command sequence
timing. 

Since I have no SPI connected on my hardware, I am only able to test
the I2C implementation, which works well with the timings taken from
the original spi driver (except for the erase timeout, which needs to
be extended as seen in Patch 15 of this series). Extending the delay
times such that usleep_range can be used has proven to be acceptable,
at least for the i2c implementation. 

Best regards
Johannes

Thanks,
Yilun

quoted

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    |