Re: [PATCH 16/16] fpga: machxo2: add configuration over i2c

[Xu Yilun <yilun.xu@intel.com>]

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
>