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

From: Xu Yilun <yilun.xu@intel.com>
To: Johannes Zink <j.zink@pengutronix.de>
Cc: linux-fpga@vger.kernel.org, devicetree@vger.kernel.org,
	Rob Herring <robh+dt@kernel.org>, Moritz Fischer <mdf@kernel.org>,
	Wu Hao <hao.wu@intel.com>,
	kernel@pengutronix.de
Subject: Re: [PATCH 16/16] fpga: machxo2: add configuration over i2c
Date: Mon, 29 Aug 2022 22:45:26 +0800	[thread overview]
Message-ID: <YwzRBmXMV4o8aGO4@yilunxu-OptiPlex-7050> (raw)
In-Reply-To: <2800bc77abb68c721feb5569608684414ae3f6be.camel@pengutronix.de>

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