Re: [PATCH v5 3/5] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller

[Karthik Ramasubramanian]

On 5/21/2018 2:49 PM, Wolfram Sang wrote:
> Hi,
> 
> On Fri, Mar 23, 2018 at 02:20:59PM -0600, Karthikeyan Ramasubramanian wrote:
>> This bus driver supports the GENI based i2c hardware controller in the
>> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable
>> module supporting a wide range of serial interfaces including I2C. The
>> driver supports FIFO mode and DMA mode of transfer and switches modes
>> dynamically depending on the size of the transfer.
>>
>> Signed-off-by: Karthikeyan Ramasubramanian 
>> Signed-off-by: Sagar Dharia 
>> Signed-off-by: Girish Mahadevan 
> 
> Is one of these people interested in maintaining this driver? Then, an
> entry for MAINTAINERS would be needed, too. (Same goes for
> drivers/soc/qcom/ IMHO, but this is not my realm, so just saying)
One of us will maintain this driver and I will update the MAINTAINERS
appropriately.
> 
>> +static const struct geni_i2c_err_log gi2c_log[] = {
>> +[GP_IRQ0] = {-EINVAL, "Unknown I2C err GP_IRQ0"},
>> +[NACK] = {-ENOTCONN, "NACK: slv unresponsive, check its 
>> power/reset-ln"},
>> +[GP_IRQ2] = {-EINVAL, "Unknown I2C err GP IRQ2"},
>> +[BUS_PROTO] = {-EPROTO, "Bus proto err, noisy/unepxected start/stop"},
>> +[ARB_LOST] = {-EBUSY, "Bus arbitration lost, clock line undriveable"},
>> +[GP_IRQ5] = {-EINVAL, "Unknown I2C err GP IRQ5"},
>> +[GENI_OVERRUN] = {-EIO, "Cmd overrun, check GENI cmd-state machine"},
>> +[GENI_ILLEGAL_CMD] = {-EILSEQ, "Illegal cmd, check GENI cmd-state 
>> machine"},
>> +[GENI_ABORT_DONE] = {-ETIMEDOUT, "Abort after timeout successful"},
>> +[GENI_TIMEOUT] = {-ETIMEDOUT, "I2C TXN timed out"},
>> +};
> 
> Please check Documentation/i2c/fault-codes for better -ERRNO values,
> especially for NACK and ARB_LOST.
I will check the fault-codes and fix the error codes here.
> 
> Rest looks good from a glimpse.
> 
> Thanks,
> 
>Wolfram
> 
Regards,
Karthik.
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/5] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller

[Wolfram Sang]

Hi,

On Fri, Mar 23, 2018 at 02:20:59PM -0600, Karthikeyan Ramasubramanian wrote:
> This bus driver supports the GENI based i2c hardware controller in the
> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable
> module supporting a wide range of serial interfaces including I2C. The
> driver supports FIFO mode and DMA mode of transfer and switches modes
> dynamically depending on the size of the transfer.
> 
> Signed-off-by: Karthikeyan Ramasubramanian 
> Signed-off-by: Sagar Dharia 
> Signed-off-by: Girish Mahadevan 

Is one of these people interested in maintaining this driver? Then, an
entry for MAINTAINERS would be needed, too. (Same goes for
drivers/soc/qcom/ IMHO, but this is not my realm, so just saying)

> +static const struct geni_i2c_err_log gi2c_log[] = {
> + [GP_IRQ0] = {-EINVAL, "Unknown I2C err GP_IRQ0"},
> + [NACK] = {-ENOTCONN, "NACK: slv unresponsive, check its 
> power/reset-ln"},
> + [GP_IRQ2] = {-EINVAL, "Unknown I2C err GP IRQ2"},
> + [BUS_PROTO] = {-EPROTO, "Bus proto err, noisy/unepxected start/stop"},
> + [ARB_LOST] = {-EBUSY, "Bus arbitration lost, clock line undriveable"},
> + [GP_IRQ5] = {-EINVAL, "Unknown I2C err GP IRQ5"},
> + [GENI_OVERRUN] = {-EIO, "Cmd overrun, check GENI cmd-state machine"},
> + [GENI_ILLEGAL_CMD] = {-EILSEQ, "Illegal cmd, check GENI cmd-state 
> machine"},
> + [GENI_ABORT_DONE] = {-ETIMEDOUT, "Abort after timeout successful"},
> + [GENI_TIMEOUT] = {-ETIMEDOUT, "I2C TXN timed out"},
> +};

Please check Documentation/i2c/fault-codes for better -ERRNO values,
especially for NACK and ARB_LOST.

Rest looks good from a glimpse.

Thanks,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH v5 3/5] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller

[Doug Anderson]

Wolfram,

On Fri, Mar 23, 2018 at 4:34 PM, Doug Anderson  wrote:
> Hi,
>
> On Fri, Mar 23, 2018 at 1:20 PM, Karthikeyan Ramasubramanian
>  wrote:
>> This bus driver supports the GENI based i2c hardware controller in the
>> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable
>> module supporting a wide range of serial interfaces including I2C. The
>> driver supports FIFO mode and DMA mode of transfer and switches modes
>> dynamically depending on the size of the transfer.
>>
>> Signed-off-by: Karthikeyan Ramasubramanian 
>> Signed-off-by: Sagar Dharia 
>> Signed-off-by: Girish Mahadevan 
>> ---
>>  drivers/i2c/busses/Kconfig |  13 +
>>  drivers/i2c/busses/Makefile|   1 +
>>  drivers/i2c/busses/i2c-qcom-geni.c | 650 
>> +
>>  3 files changed, 664 insertions(+)
>
> [...]
>
>> +/*
>> + * Hardware uses the underlying formula to calculate time periods of
>> + * SCL clock cycle. Firmware uses some additional cycles excluded from the
>> + * below formula and it is confirmed that the time periods are within
>> + * specification limits.
>
> I was hoping for more than just "oh, and there's a fudge factor", but
> I guess this is the best I'm going to get?
>
>
>> +static int geni_i2c_probe(struct platform_device *pdev)
>> +{
>> +   struct geni_i2c_dev *gi2c;
>> +   struct resource *res;
>> +   u32 proto, tx_depth;
>> +   int ret;
>> +
>> +   gi2c = devm_kzalloc(>dev, sizeof(*gi2c), GFP_KERNEL);
>> +   if (!gi2c)
>> +   return -ENOMEM;
>> +
>> +   gi2c->se.dev = >dev;
>> +   gi2c->se.wrapper = dev_get_drvdata(pdev->dev.parent);
>> +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +   gi2c->se.base = devm_ioremap_resource(>dev, res);
>> +   if (IS_ERR(gi2c->se.base))
>> +   return PTR_ERR(gi2c->se.base);
>> +
>> +   gi2c->se.clk = devm_clk_get(>dev, "se");
>> +   if (IS_ERR(gi2c->se.clk)) {
>> +   ret = PTR_ERR(gi2c->se.clk);
>> +   dev_err(>dev, "Err getting SE Core clk %d\n", ret);
>> +   return ret;
>> +   }
>> +
>> +   ret = device_property_read_u32(>dev, "clock-frequency",
>> +   >clk_freq_out);
>> +   if (ret) {
>> +   /* Clock frequency not specified, so default to 100kHz. */
>> +   dev_info(>dev,
>> +   "Bus frequency not specified, default to 100kHz.\n");
>
> If you happen to spin again, can you remove the comment since it's
> obvious from the string in the print?  It looks a lot like this code:
>
> /* Print hello, world */
> printf("hello, world\n");
>
>
> In any case, that's a pretty minor nit, so I'll add:
>
> Reviewed-by: Douglas Anderson 
>
> ...assuming that the bindings and "geni" code get Acked / landed
> somewhere.  Ideally let's not land this before the geni code lands
> since if the geni API changes for some reason it'll cause us grief.

The bindings and "geni" code have landed in Andy's tree, so whenever
you get a chance it would be super if you could land this i2c driver
(assuming it looks good to you).  I know at least a few people have
been poking at this and it seems to work for basic transfers.

Thanks!

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/5] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller

[Sagar Dharia]

Hi Doug

On 3/23/2018 5:34 PM, Doug Anderson wrote:
> Hi,
> 
> On Fri, Mar 23, 2018 at 1:20 PM, Karthikeyan Ramasubramanian
>  wrote:
>> This bus driver supports the GENI based i2c hardware controller in the
>> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable
>> module supporting a wide range of serial interfaces including I2C. The
>> driver supports FIFO mode and DMA mode of transfer and switches modes
>> dynamically depending on the size of the transfer.
>>
>> Signed-off-by: Karthikeyan Ramasubramanian 
>> Signed-off-by: Sagar Dharia 
>> Signed-off-by: Girish Mahadevan 
>> ---
>>  drivers/i2c/busses/Kconfig |  13 +
>>  drivers/i2c/busses/Makefile|   1 +
>>  drivers/i2c/busses/i2c-qcom-geni.c | 650 
>> +
>>  3 files changed, 664 insertions(+)
> 
> [...]
> 
>> +/*
>> + * Hardware uses the underlying formula to calculate time periods of
>> + * SCL clock cycle. Firmware uses some additional cycles excluded from the
>> + * below formula and it is confirmed that the time periods are within
>> + * specification limits.
> 
> I was hoping for more than just "oh, and there's a fudge factor", but
> I guess this is the best I'm going to get?
> 
According to our HW/FW team:
"We use over sampling in our FW and we use 1-2 NOPE extra in some cases.
this is why we can’t give a exact formula."

> 
>> +static int geni_i2c_probe(struct platform_device *pdev)
>> +{
>> +   struct geni_i2c_dev *gi2c;
>> +   struct resource *res;
>> +   u32 proto, tx_depth;
>> +   int ret;
>> +
>> +   gi2c = devm_kzalloc(>dev, sizeof(*gi2c), GFP_KERNEL);
>> +   if (!gi2c)
>> +   return -ENOMEM;
>> +
>> +   gi2c->se.dev = >dev;
>> +   gi2c->se.wrapper = dev_get_drvdata(pdev->dev.parent);
>> +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +   gi2c->se.base = devm_ioremap_resource(>dev, res);
>> +   if (IS_ERR(gi2c->se.base))
>> +   return PTR_ERR(gi2c->se.base);
>> +
>> +   gi2c->se.clk = devm_clk_get(>dev, "se");
>> +   if (IS_ERR(gi2c->se.clk)) {
>> +   ret = PTR_ERR(gi2c->se.clk);
>> +   dev_err(>dev, "Err getting SE Core clk %d\n", ret);
>> +   return ret;
>> +   }
>> +
>> +   ret = device_property_read_u32(>dev, "clock-frequency",
>> +   >clk_freq_out);
>> +   if (ret) {
>> +   /* Clock frequency not specified, so default to 100kHz. */
>> +   dev_info(>dev,
>> +   "Bus frequency not specified, default to 100kHz.\n");
> 
> If you happen to spin again, can you remove the comment since it's
> obvious from the string in the print?  It looks a lot like this code:
> 
> /* Print hello, world */
> printf("hello, world\n");
> 
> 
> In any case, that's a pretty minor nit, so I'll add:
> 
> Reviewed-by: Douglas Anderson 
> 
> ...assuming that the bindings and "geni" code get Acked / landed
> somewhere.  Ideally let's not land this before the geni code lands
> since if the geni API changes for some reason it'll cause us grief.
> 

Sure, thanks a lot for reviewing the patches!
-Sagar
> 
> -Doug
> 

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/5] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller

[Doug Anderson]

Hi,

On Fri, Mar 23, 2018 at 1:20 PM, Karthikeyan Ramasubramanian
 wrote:
> This bus driver supports the GENI based i2c hardware controller in the
> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable
> module supporting a wide range of serial interfaces including I2C. The
> driver supports FIFO mode and DMA mode of transfer and switches modes
> dynamically depending on the size of the transfer.
>
> Signed-off-by: Karthikeyan Ramasubramanian 
> Signed-off-by: Sagar Dharia 
> Signed-off-by: Girish Mahadevan 
> ---
>  drivers/i2c/busses/Kconfig |  13 +
>  drivers/i2c/busses/Makefile|   1 +
>  drivers/i2c/busses/i2c-qcom-geni.c | 650 
> +
>  3 files changed, 664 insertions(+)

[...]

> +/*
> + * Hardware uses the underlying formula to calculate time periods of
> + * SCL clock cycle. Firmware uses some additional cycles excluded from the
> + * below formula and it is confirmed that the time periods are within
> + * specification limits.

I was hoping for more than just "oh, and there's a fudge factor", but
I guess this is the best I'm going to get?


> +static int geni_i2c_probe(struct platform_device *pdev)
> +{
> +   struct geni_i2c_dev *gi2c;
> +   struct resource *res;
> +   u32 proto, tx_depth;
> +   int ret;
> +
> +   gi2c = devm_kzalloc(>dev, sizeof(*gi2c), GFP_KERNEL);
> +   if (!gi2c)
> +   return -ENOMEM;
> +
> +   gi2c->se.dev = >dev;
> +   gi2c->se.wrapper = dev_get_drvdata(pdev->dev.parent);
> +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +   gi2c->se.base = devm_ioremap_resource(>dev, res);
> +   if (IS_ERR(gi2c->se.base))
> +   return PTR_ERR(gi2c->se.base);
> +
> +   gi2c->se.clk = devm_clk_get(>dev, "se");
> +   if (IS_ERR(gi2c->se.clk)) {
> +   ret = PTR_ERR(gi2c->se.clk);
> +   dev_err(>dev, "Err getting SE Core clk %d\n", ret);
> +   return ret;
> +   }
> +
> +   ret = device_property_read_u32(>dev, "clock-frequency",
> +   >clk_freq_out);
> +   if (ret) {
> +   /* Clock frequency not specified, so default to 100kHz. */
> +   dev_info(>dev,
> +   "Bus frequency not specified, default to 100kHz.\n");

If you happen to spin again, can you remove the comment since it's
obvious from the string in the print?  It looks a lot like this code:

/* Print hello, world */
printf("hello, world\n");


In any case, that's a pretty minor nit, so I'll add:

Reviewed-by: Douglas Anderson 

...assuming that the bindings and "geni" code get Acked / landed
somewhere.  Ideally let's not land this before the geni code lands
since if the geni API changes for some reason it'll cause us grief.


-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 3/5] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller

[Karthikeyan Ramasubramanian]

This bus driver supports the GENI based i2c hardware controller in the
Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable
module supporting a wide range of serial interfaces including I2C. The
driver supports FIFO mode and DMA mode of transfer and switches modes
dynamically depending on the size of the transfer.

Signed-off-by: Karthikeyan Ramasubramanian 
Signed-off-by: Sagar Dharia 
Signed-off-by: Girish Mahadevan 
---
 drivers/i2c/busses/Kconfig |  13 +
 drivers/i2c/busses/Makefile|   1 +
 drivers/i2c/busses/i2c-qcom-geni.c | 650 +
 3 files changed, 664 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-qcom-geni.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index e2954fb..89e642a 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -848,6 +848,19 @@ config I2C_PXA_SLAVE
  is necessary for systems where the PXA may be a target on the
  I2C bus.
 
+config I2C_QCOM_GENI
+   tristate "Qualcomm Technologies Inc.'s GENI based I2C controller"
+   depends on ARCH_QCOM || COMPILE_TEST
+   depends on QCOM_GENI_SE
+   help
+ This driver supports GENI serial engine based I2C controller in
+ master mode on the Qualcomm Technologies Inc.'s SoCs. If you say
+ yes to this option, support will be included for the built-in I2C
+ interface on the Qualcomm Technologies Inc.'s SoCs.
+
+ This driver can also be built as a module.  If so, the module
+ will be called i2c-qcom-geni.
+
 config I2C_QUP
tristate "Qualcomm QUP based I2C controller"
depends on ARCH_QCOM
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 2ce8576..201fce1 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -84,6 +84,7 @@ obj-$(CONFIG_I2C_PNX) += i2c-pnx.o
 obj-$(CONFIG_I2C_PUV3) += i2c-puv3.o
 obj-$(CONFIG_I2C_PXA)  += i2c-pxa.o
 obj-$(CONFIG_I2C_PXA_PCI)  += i2c-pxa-pci.o
+obj-$(CONFIG_I2C_QCOM_GENI)+= i2c-qcom-geni.o
 obj-$(CONFIG_I2C_QUP)  += i2c-qup.o
 obj-$(CONFIG_I2C_RIIC) += i2c-riic.o
 obj-$(CONFIG_I2C_RK3X) += i2c-rk3x.o
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
b/drivers/i2c/busses/i2c-qcom-geni.c
new file mode 100644
index 000..24f859d
--- /dev/null
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -0,0 +1,650 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define SE_I2C_TX_TRANS_LEN0x26c
+#define SE_I2C_RX_TRANS_LEN0x270
+#define SE_I2C_SCL_COUNTERS0x278
+
+#define SE_I2C_ERR  (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)
+#define SE_I2C_ABORT   BIT(1)
+
+/* M_CMD OP codes for I2C */
+#define I2C_WRITE  0x1
+#define I2C_READ   0x2
+#define I2C_WRITE_READ 0x3
+#define I2C_ADDR_ONLY  0x4
+#define I2C_BUS_CLEAR  0x6
+#define I2C_STOP_ON_BUS0x7
+/* M_CMD params for I2C */
+#define PRE_CMD_DELAY  BIT(0)
+#define TIMESTAMP_BEFORE   BIT(1)
+#define STOP_STRETCH   BIT(2)
+#define TIMESTAMP_AFTERBIT(3)
+#define POST_COMMAND_DELAY BIT(4)
+#define IGNORE_ADD_NACKBIT(6)
+#define READ_FINISHED_WITH_ACK BIT(7)
+#define BYPASS_ADDR_PHASE  BIT(8)
+#define SLV_ADDR_MSK   GENMASK(15, 9)
+#define SLV_ADDR_SHFT  9
+/* I2C SCL COUNTER fields */
+#define HIGH_COUNTER_MSK   GENMASK(29, 20)
+#define HIGH_COUNTER_SHFT  20
+#define LOW_COUNTER_MSKGENMASK(19, 10)
+#define LOW_COUNTER_SHFT   10
+#define CYCLE_COUNTER_MSK  GENMASK(9, 0)
+
+enum geni_i2c_err_code {
+   GP_IRQ0,
+   NACK,
+   GP_IRQ2,
+   BUS_PROTO,
+   ARB_LOST,
+   GP_IRQ5,
+   GENI_OVERRUN,
+   GENI_ILLEGAL_CMD,
+   GENI_ABORT_DONE,
+   GENI_TIMEOUT,
+};
+
+#define DM_I2C_CB_ERR  ((BIT(NACK) | BIT(BUS_PROTO) | BIT(ARB_LOST)) \
+   << 5)
+
+#define I2C_AUTO_SUSPEND_DELAY 250
+#define KHZ(freq)  (1000 * freq)
+#define PACKING_BYTES_PW   4
+
+#define ABORT_TIMEOUT  HZ
+#define XFER_TIMEOUT   HZ
+#define RST_TIMEOUTHZ
+
+struct geni_i2c_dev {
+   struct geni_se se;
+   u32 tx_wm;
+   int irq;
+   int err;
+   struct i2c_adapter adap;
+   struct completion done;
+   struct i2c_msg *cur;
+   int cur_wr;
+   int cur_rd;
+   spinlock_t lock;
+   u32 clk_freq_out;
+   const struct geni_i2c_clk_fld