| Message ID | 20220825141343.1375690-17-j.zink@pengutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Lattice MachXO2 programming via I2C | expand |
On 2022-08-25 at 16:13:43 +0200, Johannes Zink wrote: > From: Peter Jensen <pdj@bang-olufsen.dk> > > 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@bang-olufsen.dk> > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > Signed-off-by: Johannes Zink <j.zink@pengutronix.de> > --- > drivers/fpga/Kconfig | 8 ++ > drivers/fpga/Makefile | 1 + > drivers/fpga/machxo2-common.c | 15 +++- > drivers/fpga/machxo2-common.h | 3 + > drivers/fpga/machxo2-i2c.c | 137 ++++++++++++++++++++++++++++++++++ > 5 files changed, 163 insertions(+), 1 deletion(-) > create mode 100644 drivers/fpga/machxo2-i2c.c > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig > index e5869a732246..97081bbd7c19 100644 > --- a/drivers/fpga/Kconfig > +++ b/drivers/fpga/Kconfig > @@ -90,6 +90,14 @@ config FPGA_MGR_MACHXO2_SPI > FPGA manager driver support for Lattice MachXO2 configuration > over slave SPI interface. > > +config FPGA_MGR_MACHXO2_I2C > + tristate "Lattice MachXO2 I2C" > + depends on I2C > + select FPGA_MGR_MACHXO2_COMMON > + help > + FPGA manager driver support for Lattice MachXO2 configuration > + over slave I2C interface. > + > config FPGA_MGR_TS73XX > tristate "Technologic Systems TS-73xx SBC FPGA Manager" > depends on ARCH_EP93XX && MACH_TS72XX > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile > index f247a8de83ad..fcdf79f4d424 100644 > --- a/drivers/fpga/Makefile > +++ b/drivers/fpga/Makefile > @@ -12,6 +12,7 @@ obj-$(CONFIG_FPGA_MGR_ALTERA_PS_SPI) += altera-ps-spi.o > obj-$(CONFIG_FPGA_MGR_ICE40_SPI) += ice40-spi.o > obj-$(CONFIG_FPGA_MGR_MACHXO2_COMMON) += machxo2-common.o > obj-$(CONFIG_FPGA_MGR_MACHXO2_SPI) += machxo2-spi.o > +obj-$(CONFIG_FPGA_MGR_MACHXO2_I2C) += machxo2-i2c.o > obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o > obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10) += socfpga-a10.o > obj-$(CONFIG_FPGA_MGR_STRATIX10_SOC) += stratix10-soc.o > diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-common.c > index e8967cdee2c6..0a3c126675da 100644 > --- a/drivers/fpga/machxo2-common.c > +++ b/drivers/fpga/machxo2-common.c > @@ -100,7 +100,7 @@ static void dump_status_reg(struct device *dev, u32 status) > !!FIELD_GET(MACHXO2_DVER, status), get_err_string(get_err(status))); > } > > -static int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv) > +int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv) > { > u32 status; > int ret, loop = 0; > @@ -143,6 +143,11 @@ static int machxo2_cleanup(struct fpga_manager *mgr) > cmd.cmd = refresh; > cmd.cmd_len = sizeof(refresh); > cmd.delay_us = MACHXO2_REFRESH_USEC; > + > + /* quirk: refresh command over i2c is 1 byte shorter */ > + if (priv->refresh_3b) > + cmd.cmd_len--; > + > ret = priv->write_commands(priv, &cmd, 1); > if (ret) > goto fail; > @@ -207,6 +212,10 @@ static int machxo2_write_init(struct fpga_manager *mgr, > cmd[0].cmd_len = sizeof(enable); > cmd[0].delay_us = MACHXO2_LOW_DELAY_USEC; > > + /* quirk: enable command over i2c is 1 byte shorter */ > + if (priv->enable_3b) > + cmd[0].cmd_len--; > + > cmd[1].cmd = (u8 *)&priv->erase_cmd; > cmd[1].cmd_len = sizeof(priv->erase_cmd); > > @@ -313,6 +322,10 @@ static int machxo2_write_complete(struct fpga_manager *mgr, > cmd.cmd_len = sizeof(refresh); > cmd.delay_us = MACHXO2_REFRESH_USEC; > > + /* quirk: refresh command over i2c is 1 byte shorter */ > + if (priv->refresh_3b) > + cmd.cmd_len--; > + > do { > ret = priv->write_commands(priv, &cmd, 1); > if (ret) > diff --git a/drivers/fpga/machxo2-common.h b/drivers/fpga/machxo2-common.h > index 0f9f53b48152..8c09345adee5 100644 > --- a/drivers/fpga/machxo2-common.h > +++ b/drivers/fpga/machxo2-common.h > @@ -32,9 +32,12 @@ struct machxo2_common_priv { > int (*get_status)(struct machxo2_common_priv *priv, u32 *status); > struct device *dev; > __be32 erase_cmd; > + u8 enable_3b:1; > + u8 refresh_3b:1; > struct gpio_desc *fpga_program_n; > }; > > int machxo2_common_init(struct machxo2_common_priv *priv, struct device *dev); > +int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv); > > #endif > diff --git a/drivers/fpga/machxo2-i2c.c b/drivers/fpga/machxo2-i2c.c > new file mode 100644 > index 000000000000..a309016def1c > --- /dev/null > +++ b/drivers/fpga/machxo2-i2c.c > @@ -0,0 +1,137 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Lattice MachXO2 Slave I2C Driver > + * > + * Manage Lattice FPGA firmware that is loaded over I2C using > + * the slave serial configuration interface. > + * > + * Copyright (C) 2018 Paolo Pisati <p.pisati@gmail.com> > + * Copyright (C) 2021 Peter Jensen <pdj@bang-olufsen.dk> > + */ > + > +#include <linux/i2c.h> > +#include <linux/container_of.h> > +#include <linux/delay.h> > +#include "machxo2-common.h" > + > + > +struct machxo2_i2c_priv { > + struct machxo2_common_priv common; > + struct i2c_client *client; > +}; > + > +static inline struct machxo2_i2c_priv *to_machxo2_i2c_priv(struct machxo2_common_priv *common) > +{ > + return container_of(common, struct machxo2_i2c_priv, common); > +} > + > +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. > + __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 > + } > + > + 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@bang-olufsen.dk>"); > +MODULE_DESCRIPTION("Load Lattice FPGA firmware over I2C"); > +MODULE_LICENSE("GPL"); > -- > 2.30.2 >
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@bang-olufsen.dk> > > > > 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@bang-olufsen.dk> > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > > Signed-off-by: Johannes Zink <j.zink@pengutronix.de> > > --- [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@bang-olufsen.dk>"); > > +MODULE_DESCRIPTION("Load Lattice FPGA firmware over I2C"); > > +MODULE_LICENSE("GPL"); > > -- > > 2.30.2 > > >
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@bang-olufsen.dk> > > > > > > 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@bang-olufsen.dk> > > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > > > Signed-off-by: Johannes Zink <j.zink@pengutronix.de> > > > --- > > [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@bang-olufsen.dk>"); > > > +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 | >
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: > > 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@bang-olufsen.dk> > > > > > > > > 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@bang-olufsen.dk> > > > > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de> > > > > Signed-off-by: Johannes Zink <j.zink@pengutronix.de> > > > > --- [snip] > > > > > > > > > > > > > + > > > > +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. > > > > > 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 > > > > > 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@bang-olufsen.dk>"); > > > > +MODULE_DESCRIPTION("Load Lattice FPGA firmware over I2C"); > > > > +MODULE_LICENSE("GPL"); > > > > -- > > > > 2.30.2
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index e5869a732246..97081bbd7c19 100644 --- a/drivers/fpga/Kconfig +++ b/drivers/fpga/Kconfig @@ -90,6 +90,14 @@ config FPGA_MGR_MACHXO2_SPI FPGA manager driver support for Lattice MachXO2 configuration over slave SPI interface. +config FPGA_MGR_MACHXO2_I2C + tristate "Lattice MachXO2 I2C" + depends on I2C + select FPGA_MGR_MACHXO2_COMMON + help + FPGA manager driver support for Lattice MachXO2 configuration + over slave I2C interface. + config FPGA_MGR_TS73XX tristate "Technologic Systems TS-73xx SBC FPGA Manager" depends on ARCH_EP93XX && MACH_TS72XX diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index f247a8de83ad..fcdf79f4d424 100644 --- a/drivers/fpga/Makefile +++ b/drivers/fpga/Makefile @@ -12,6 +12,7 @@ obj-$(CONFIG_FPGA_MGR_ALTERA_PS_SPI) += altera-ps-spi.o obj-$(CONFIG_FPGA_MGR_ICE40_SPI) += ice40-spi.o obj-$(CONFIG_FPGA_MGR_MACHXO2_COMMON) += machxo2-common.o obj-$(CONFIG_FPGA_MGR_MACHXO2_SPI) += machxo2-spi.o +obj-$(CONFIG_FPGA_MGR_MACHXO2_I2C) += machxo2-i2c.o obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10) += socfpga-a10.o obj-$(CONFIG_FPGA_MGR_STRATIX10_SOC) += stratix10-soc.o diff --git a/drivers/fpga/machxo2-common.c b/drivers/fpga/machxo2-common.c index e8967cdee2c6..0a3c126675da 100644 --- a/drivers/fpga/machxo2-common.c +++ b/drivers/fpga/machxo2-common.c @@ -100,7 +100,7 @@ static void dump_status_reg(struct device *dev, u32 status) !!FIELD_GET(MACHXO2_DVER, status), get_err_string(get_err(status))); } -static int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv) +int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv) { u32 status; int ret, loop = 0; @@ -143,6 +143,11 @@ static int machxo2_cleanup(struct fpga_manager *mgr) cmd.cmd = refresh; cmd.cmd_len = sizeof(refresh); cmd.delay_us = MACHXO2_REFRESH_USEC; + + /* quirk: refresh command over i2c is 1 byte shorter */ + if (priv->refresh_3b) + cmd.cmd_len--; + ret = priv->write_commands(priv, &cmd, 1); if (ret) goto fail; @@ -207,6 +212,10 @@ static int machxo2_write_init(struct fpga_manager *mgr, cmd[0].cmd_len = sizeof(enable); cmd[0].delay_us = MACHXO2_LOW_DELAY_USEC; + /* quirk: enable command over i2c is 1 byte shorter */ + if (priv->enable_3b) + cmd[0].cmd_len--; + cmd[1].cmd = (u8 *)&priv->erase_cmd; cmd[1].cmd_len = sizeof(priv->erase_cmd); @@ -313,6 +322,10 @@ static int machxo2_write_complete(struct fpga_manager *mgr, cmd.cmd_len = sizeof(refresh); cmd.delay_us = MACHXO2_REFRESH_USEC; + /* quirk: refresh command over i2c is 1 byte shorter */ + if (priv->refresh_3b) + cmd.cmd_len--; + do { ret = priv->write_commands(priv, &cmd, 1); if (ret) diff --git a/drivers/fpga/machxo2-common.h b/drivers/fpga/machxo2-common.h index 0f9f53b48152..8c09345adee5 100644 --- a/drivers/fpga/machxo2-common.h +++ b/drivers/fpga/machxo2-common.h @@ -32,9 +32,12 @@ struct machxo2_common_priv { int (*get_status)(struct machxo2_common_priv *priv, u32 *status); struct device *dev; __be32 erase_cmd; + u8 enable_3b:1; + u8 refresh_3b:1; struct gpio_desc *fpga_program_n; }; int machxo2_common_init(struct machxo2_common_priv *priv, struct device *dev); +int machxo2_wait_until_not_busy(struct machxo2_common_priv *priv); #endif diff --git a/drivers/fpga/machxo2-i2c.c b/drivers/fpga/machxo2-i2c.c new file mode 100644 index 000000000000..a309016def1c --- /dev/null +++ b/drivers/fpga/machxo2-i2c.c @@ -0,0 +1,137 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Lattice MachXO2 Slave I2C Driver + * + * Manage Lattice FPGA firmware that is loaded over I2C using + * the slave serial configuration interface. + * + * Copyright (C) 2018 Paolo Pisati <p.pisati@gmail.com> + * Copyright (C) 2021 Peter Jensen <pdj@bang-olufsen.dk> + */ + +#include <linux/i2c.h> +#include <linux/container_of.h> +#include <linux/delay.h> +#include "machxo2-common.h" + + +struct machxo2_i2c_priv { + struct machxo2_common_priv common; + struct i2c_client *client; +}; + +static inline struct machxo2_i2c_priv *to_machxo2_i2c_priv(struct machxo2_common_priv *common) +{ + return container_of(common, struct machxo2_i2c_priv, common); +} + +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; + __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); + } + + 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@bang-olufsen.dk>"); +MODULE_DESCRIPTION("Load Lattice FPGA firmware over I2C"); +MODULE_LICENSE("GPL");