Re: [PATCH 1/2] i2c: Add support for Qualcomm Generic Interface (GENI) I2C controller

[Neil Armstrong]

On 19/04/2024 13:47, Caleb Connolly wrote:

Hi Neil,

On 18/04/2024 23:47, Neil Armstrong wrote:

Add Support for the Qualcomm Generic Interface (GENI) I2C interface
found on newer Qualcomm SoCs.


\o/


The Generic Interface (GENI) is a firmware based Qualcomm Universal
Peripherals (QUP) Serial Engine (SE) Wrapper which can support multiple
bus protocols depending on the firmware type loaded at early boot time
based on system configuration.

It also supports the "I2C Master Hub" which is a single function Wrapper
that only FIFO mode I2C.

It replaces the fixed-function QUP Wrapper found on older SoCs.

The geni-se.h containing the generic GENI Serial Engine registers defines
is imported from Linux.

Only FIFO mode is implemented, nor SE DMA nor GPI DMA is implemented.

nit: "neither SE DMA nor GPI DMA are implemented"


Thx!



A few minor things below, but otherwise LGTM!


Signed-off-by: Neil Armstrong 
---
  drivers/i2c/Kconfig|  10 +
  drivers/i2c/Makefile   |   1 +
  drivers/i2c/geni_i2c.c | 576 +
  include/soc/qcom/geni-se.h | 265 +
  4 files changed, 852 insertions(+)


[...]

diff --git a/drivers/i2c/geni_i2c.c b/drivers/i2c/geni_i2c.c
new file mode 100644
index 000..8c3ed3bef89
--- /dev/null
+++ b/drivers/i2c/geni_i2c.c
@@ -0,0 +1,576 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2024, Linaro Limited
+ * Author: Neil Armstrong 
+ *
+ * Based on Linux driver: drivers/i2c/busses/i2c-qcom-geni.c
+ */
+

[...]

+static int geni_i2c_fifo_tx_fill(struct geni_i2c_priv *geni, struct i2c_msg 
*msg)
+{
+   ulong start = get_timer(0);
+   ulong cur_xfer = 0;
+   int i;
+
+   while (get_timer(start) < I2C_TIMEOUT_MS) {
+   u32 status = readl(geni->base + SE_GENI_M_IRQ_STATUS);
+
+   if (status & (M_CMD_ABORT_EN |
+ M_CMD_OVERRUN_EN |
+ M_ILLEGAL_CMD_EN |
+ M_CMD_FAILURE_EN |
+ M_GP_IRQ_1_EN |
+ M_GP_IRQ_3_EN |
+ M_GP_IRQ_4_EN)) {
+   debug("%s:%d cmd err\n", __func__, __LINE__);


How likely are we to hit this? Would it make sense to promote it to a
pr_warn()?

Please drop the __LINE__ and (if it makes sense to?) print the value of
status.


It's used when the tranactions is nacked, so it would spam, so I rather remove
the print entirely. It's verly unlikely we see any of the other errors.


+   writel(status, geni->base + SE_GENI_M_IRQ_CLEAR);
+   writel(0, geni->base + SE_GENI_TX_WATERMARK_REG);
+   return -EREMOTEIO;
+   }
+
+   if ((status & M_TX_FIFO_WATERMARK_EN) == 0) {
+   udelay(1);
+   goto skip_fill;
+   }
+
+   for (i = 0; i < geni->tx_wm; i++) {
+   u32 temp, tx = 0;
+   unsigned int p = 0;
+
+   while (cur_xfer < msg->len && p < sizeof(tx)) {
+   temp = msg->buf[cur_xfer++];
+   tx |= temp << (p * 8);
+   p++;
+   }
+
+   writel(tx, geni->base + SE_GENI_TX_FIFOn);
+
+   if (cur_xfer == msg->len) {
+   writel(0, geni->base + 
SE_GENI_TX_WATERMARK_REG);
+   break;
+   }
+   }
+
+skip_fill:
+   writel(status, geni->base + SE_GENI_M_IRQ_CLEAR);
+
+   if (status & M_CMD_DONE_EN)
+   return 0;
+   }
+
+   return -ETIMEDOUT;
+}
+
+static int geni_i2c_fifo_rx_drain(struct geni_i2c_priv *geni, struct i2c_msg 
*msg)
+{
+   ulong start = get_timer(0);
+   ulong cur_xfer = 0;
+   int i;
+
+   while (get_timer(start) < I2C_TIMEOUT_MS) {
+   u32 status = readl(geni->base + SE_GENI_M_IRQ_STATUS);
+   u32 rxstatus = readl(geni->base + SE_GENI_RX_FIFO_STATUS);
+   u32 rxcnt = rxstatus & RX_FIFO_WC_MSK;
+
+   if (status & (M_CMD_ABORT_EN |
+ M_CMD_FAILURE_EN |
+ M_CMD_OVERRUN_EN |
+ M_ILLEGAL_CMD_EN |
+ M_GP_IRQ_1_EN |
+ M_GP_IRQ_3_EN |
+ M_GP_IRQ_4_EN)) {
+   debug("%s:%d cmd err\n", __func__, __LINE__);


Ditto

+   writel(status, geni->base + SE_GENI_M_IRQ_CLEAR);
+   return -EIO;
+   }
+
+   if ((status & (M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN)) == 
0) {
+   udelay(1);
+   

Re: [PATCH 1/2] i2c: Add support for Qualcomm Generic Interface (GENI) I2C controller

[Caleb Connolly]

Hi Neil,

On 18/04/2024 23:47, Neil Armstrong wrote:
> Add Support for the Qualcomm Generic Interface (GENI) I2C interface
> found on newer Qualcomm SoCs.

\o/
> 
> The Generic Interface (GENI) is a firmware based Qualcomm Universal
> Peripherals (QUP) Serial Engine (SE) Wrapper which can support multiple
> bus protocols depending on the firmware type loaded at early boot time
> based on system configuration.
> 
> It also supports the "I2C Master Hub" which is a single function Wrapper
> that only FIFO mode I2C.
> 
> It replaces the fixed-function QUP Wrapper found on older SoCs.
> 
> The geni-se.h containing the generic GENI Serial Engine registers defines
> is imported from Linux.
> 
> Only FIFO mode is implemented, nor SE DMA nor GPI DMA is implemented.
nit: "neither SE DMA nor GPI DMA are implemented"

A few minor things below, but otherwise LGTM!
> 
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/i2c/Kconfig|  10 +
>  drivers/i2c/Makefile   |   1 +
>  drivers/i2c/geni_i2c.c | 576 
> +
>  include/soc/qcom/geni-se.h | 265 +
>  4 files changed, 852 insertions(+)
> 
[...]
> diff --git a/drivers/i2c/geni_i2c.c b/drivers/i2c/geni_i2c.c
> new file mode 100644
> index 000..8c3ed3bef89
> --- /dev/null
> +++ b/drivers/i2c/geni_i2c.c
> @@ -0,0 +1,576 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2024, Linaro Limited
> + * Author: Neil Armstrong 
> + *
> + * Based on Linux driver: drivers/i2c/busses/i2c-qcom-geni.c
> + */
> +
[...]
> +static int geni_i2c_fifo_tx_fill(struct geni_i2c_priv *geni, struct i2c_msg 
> *msg)
> +{
> + ulong start = get_timer(0);
> + ulong cur_xfer = 0;
> + int i;
> +
> + while (get_timer(start) < I2C_TIMEOUT_MS) {
> + u32 status = readl(geni->base + SE_GENI_M_IRQ_STATUS);
> +
> + if (status & (M_CMD_ABORT_EN |
> +   M_CMD_OVERRUN_EN |
> +   M_ILLEGAL_CMD_EN |
> +   M_CMD_FAILURE_EN |
> +   M_GP_IRQ_1_EN |
> +   M_GP_IRQ_3_EN |
> +   M_GP_IRQ_4_EN)) {
> + debug("%s:%d cmd err\n", __func__, __LINE__);

How likely are we to hit this? Would it make sense to promote it to a
pr_warn()?

Please drop the __LINE__ and (if it makes sense to?) print the value of
status.
> + writel(status, geni->base + SE_GENI_M_IRQ_CLEAR);
> + writel(0, geni->base + SE_GENI_TX_WATERMARK_REG);
> + return -EREMOTEIO;
> + }
> +
> + if ((status & M_TX_FIFO_WATERMARK_EN) == 0) {
> + udelay(1);
> + goto skip_fill;
> + }
> +
> + for (i = 0; i < geni->tx_wm; i++) {
> + u32 temp, tx = 0;
> + unsigned int p = 0;
> +
> + while (cur_xfer < msg->len && p < sizeof(tx)) {
> + temp = msg->buf[cur_xfer++];
> + tx |= temp << (p * 8);
> + p++;
> + }
> +
> + writel(tx, geni->base + SE_GENI_TX_FIFOn);
> +
> + if (cur_xfer == msg->len) {
> + writel(0, geni->base + 
> SE_GENI_TX_WATERMARK_REG);
> + break;
> + }
> + }
> +
> +skip_fill:
> + writel(status, geni->base + SE_GENI_M_IRQ_CLEAR);
> +
> + if (status & M_CMD_DONE_EN)
> + return 0;
> + }
> +
> + return -ETIMEDOUT;
> +}
> +
> +static int geni_i2c_fifo_rx_drain(struct geni_i2c_priv *geni, struct i2c_msg 
> *msg)
> +{
> + ulong start = get_timer(0);
> + ulong cur_xfer = 0;
> + int i;
> +
> + while (get_timer(start) < I2C_TIMEOUT_MS) {
> + u32 status = readl(geni->base + SE_GENI_M_IRQ_STATUS);
> + u32 rxstatus = readl(geni->base + SE_GENI_RX_FIFO_STATUS);
> + u32 rxcnt = rxstatus & RX_FIFO_WC_MSK;
> +
> + if (status & (M_CMD_ABORT_EN |
> +   M_CMD_FAILURE_EN |
> +   M_CMD_OVERRUN_EN |
> +   M_ILLEGAL_CMD_EN |
> +   M_GP_IRQ_1_EN |
> +   M_GP_IRQ_3_EN |
> +   M_GP_IRQ_4_EN)) {
> + debug("%s:%d cmd err\n", __func__, __LINE__);

Ditto
> + writel(status, geni->base + SE_GENI_M_IRQ_CLEAR);
> + return -EIO;
> + }
> +
> + if ((status & (M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN)) == 
> 0) {
> + udelay(1);
> + goto skip_drain;
> + }
> +
> + for (i = 0; cur_xfer < msg->len && i <