[RFC]: Supporting PIO mode of operation in i2c_msg->flags

[RFC]: Supporting PIO mode of operation in i2c_msg->flags

[Vaibhav Hiremath]

Hi,

This is rather an old issue, for which I have also done custom & hacky
solution in the past.

Please let me know if there is already an some other method or accepted
workaround to this issue. Also, let me know if you have any suggestions
on this patch, I would be happy to work on it.


Problem Statement:
------------------
In certain embedded platforms, the board power on/off is managed by
PMIC IC, which in most of the cases is over I2C bus.
So during shutdown/reboot you may have to do I2C transactions to access
PMIC, which may sleep if I2C bus driver is not functioning in PIO by
default.

This is unexpected, to sleep after you disable the interrupts.

For example,

During reboot/shutdown, the execution call reaches to,

void machine_power_off(void)
{
         local_irq_disable();
         smp_send_stop();
         if (pm_power_off)
                 pm_power_off();
}

pm_power_off will try to do I2C transaction to access PMIC, which may
sleep/schedule.


Current implementation:
--------------------

Everyone probably does have their own custom implementation for this.
Or some other mechanism to achieve this (may be hardware support).


Proposal:
---------

If I am not missing any other way of handling this situation,
below proposal should be clean and acceptable (I hope so :) )


Add flags to choose PIO mode, for I2C transactions, and the default
would be to non-PIO mode.


struct i2c_msg flags :

#define I2C_M_PIO    0x0800


And also add respective functionality flags

#define I2C_FUNC_PIO    0x10000000 /* Driver should support PIO mode*/


In the bus driver,

Inside master_xfer() callback we will check whether the intended i2c
transaction is meant to be processed in PIO mode, if yes, then we will
call PIO api, else as usual non-pio api should serve.


static int xxx_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int 
num)
{
	...


/* We can also check i2c_check_functionality(adap,I2C_FUNC_PIO) */
	if (msg->flags & I2C_M_PIO) {
		/* PIO mode operation */
	} else {
		/* non-pio mode of operation */
	}


	...


         return ret;
}


Feedback/comments are always welcome.

Thanks,
Vaibhav

Re: [RFC]: Supporting PIO mode of operation in i2c_msg->flags

[Vaibhav Hiremath]

On Monday 25 May 2015 03:17 PM, Vaibhav Hiremath wrote:
>
> Hi,
>
> This is rather an old issue, for which I have also done custom & hacky
> solution in the past.
>
> Please let me know if there is already an some other method or accepted
> workaround to this issue. Also, let me know if you have any suggestions
> on this patch, I would be happy to work on it.
>
>
> Problem Statement:
> ------------------
> In certain embedded platforms, the board power on/off is managed by
> PMIC IC, which in most of the cases is over I2C bus.
> So during shutdown/reboot you may have to do I2C transactions to access
> PMIC, which may sleep if I2C bus driver is not functioning in PIO by
> default.
>
> This is unexpected, to sleep after you disable the interrupts.
>
> For example,
>
> During reboot/shutdown, the execution call reaches to,
>
> void machine_power_off(void)
> {
>          local_irq_disable();
>          smp_send_stop();
>          if (pm_power_off)
>                  pm_power_off();
> }
>
> pm_power_off will try to do I2C transaction to access PMIC, which may
> sleep/schedule.
>
>
> Current implementation:
> --------------------
>
> Everyone probably does have their own custom implementation for this.
> Or some other mechanism to achieve this (may be hardware support).
>
>
> Proposal:
> ---------
>
> If I am not missing any other way of handling this situation,
> below proposal should be clean and acceptable (I hope so :) )
>
>
> Add flags to choose PIO mode, for I2C transactions, and the default
> would be to non-PIO mode.
>
>
> struct i2c_msg flags :
>
> #define I2C_M_PIO    0x0800
>
>
> And also add respective functionality flags
>
> #define I2C_FUNC_PIO    0x10000000 /* Driver should support PIO mode*/
>
>
> In the bus driver,
>
> Inside master_xfer() callback we will check whether the intended i2c
> transaction is meant to be processed in PIO mode, if yes, then we will
> call PIO api, else as usual non-pio api should serve.
>
>
> static int xxx_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int
> num)
> {
>      ...
>
>
> /* We can also check i2c_check_functionality(adap,I2C_FUNC_PIO) */
>      if (msg->flags & I2C_M_PIO) {
>          /* PIO mode operation */
>      } else {
>          /* non-pio mode of operation */
>      }
>
>
>      ...
>
>
>          return ret;
> }
>
>
> Feedback/comments are always welcome.
>
> Thanks,
> Vaibhav


Any comments on the RFC?

Also,
+ linux-arm & Wolfram Sang

Thanks,
Vaibhav

Re: [RFC]: Supporting PIO mode of operation in i2c_msg->flags

[Vaibhav Hiremath]

On Thursday 28 May 2015 07:25 PM, Vaibhav Hiremath wrote:
>
>
> On Monday 25 May 2015 03:17 PM, Vaibhav Hiremath wrote:
>>
>> Hi,
>>
>> This is rather an old issue, for which I have also done custom & hacky
>> solution in the past.
>>
>> Please let me know if there is already an some other method or accepted
>> workaround to this issue. Also, let me know if you have any suggestions
>> on this patch, I would be happy to work on it.
>>
>>
>> Problem Statement:
>> ------------------
>> In certain embedded platforms, the board power on/off is managed by
>> PMIC IC, which in most of the cases is over I2C bus.
>> So during shutdown/reboot you may have to do I2C transactions to access
>> PMIC, which may sleep if I2C bus driver is not functioning in PIO by
>> default.
>>
>> This is unexpected, to sleep after you disable the interrupts.
>>
>> For example,
>>
>> During reboot/shutdown, the execution call reaches to,
>>
>> void machine_power_off(void)
>> {
>>          local_irq_disable();
>>          smp_send_stop();
>>          if (pm_power_off)
>>                  pm_power_off();
>> }
>>
>> pm_power_off will try to do I2C transaction to access PMIC, which may
>> sleep/schedule.
>>
>>
>> Current implementation:
>> --------------------
>>
>> Everyone probably does have their own custom implementation for this.
>> Or some other mechanism to achieve this (may be hardware support).
>>
>>
>> Proposal:
>> ---------
>>
>> If I am not missing any other way of handling this situation,
>> below proposal should be clean and acceptable (I hope so :) )
>>
>>
>> Add flags to choose PIO mode, for I2C transactions, and the default
>> would be to non-PIO mode.
>>
>>
>> struct i2c_msg flags :
>>
>> #define I2C_M_PIO    0x0800
>>
>>
>> And also add respective functionality flags
>>
>> #define I2C_FUNC_PIO    0x10000000 /* Driver should support PIO mode*/
>>
>>
>> In the bus driver,
>>
>> Inside master_xfer() callback we will check whether the intended i2c
>> transaction is meant to be processed in PIO mode, if yes, then we will
>> call PIO api, else as usual non-pio api should serve.
>>
>>
>> static int xxx_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int
>> num)
>> {
>>      ...
>>
>>
>> /* We can also check i2c_check_functionality(adap,I2C_FUNC_PIO) */
>>      if (msg->flags & I2C_M_PIO) {
>>          /* PIO mode operation */
>>      } else {
>>          /* non-pio mode of operation */
>>      }
>>
>>
>>      ...
>>
>>
>>          return ret;
>> }
>>
>>
>> Feedback/comments are always welcome.
>>
>> Thanks,
>> Vaibhav
>
>
> Any comments on the RFC?
>
> Also,
> + linux-arm & Wolfram Sang
>

Wolfram,

Any comment on this RFC?

Thanks,
Vaibhav

Re: [RFC]: Supporting PIO mode of operation in i2c_msg->flags

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]

> Current implementation:
> --------------------
> 
> Everyone probably does have their own custom implementation for this.
> Or some other mechanism to achieve this (may be hardware support).

Do you have examples? It is easier to look at code.

> /* We can also check i2c_check_functionality(adap,I2C_FUNC_PIO) */
> 	if (msg->flags & I2C_M_PIO) {
> 		/* PIO mode operation */
> 	} else {
> 		/* non-pio mode of operation */
> 	}

I'd rather let the core decide. Like introduce a master_xfer_nosleep
callback and let the core use the standard or the nosleep one. We'd need
the same for smbus as well.

This leads to the question if we need a M_FLAG or expose the *_nosleep
functions directly?

And I wonder about the demand for it. There are a number of SoCs with
designated I2C cores for the PMIC which do automated transfers.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC]: Supporting PIO mode of operation in i2c_msg->flags

[Vaibhav Hiremath]

On Friday 12 June 2015 05:11 AM, Wolfram Sang wrote:
>> Current implementation:
>> --------------------
>>
>> Everyone probably does have their own custom implementation for this.
>> Or some other mechanism to achieve this (may be hardware support).
>
> Do you have examples? It is easier to look at code.
>

As I mentioned in the "Problem Statement",

I2C interface for PMIC may need to be used at the end of poweroff
sequence where you cannot afford to sleep/schedule, as interrupt has
been disabled.

Earlier I was working on OMAP platform, where I remember we had
implemented something for such requirement, but as I am not in TI
anymore, I can not access the internal code.

Let me check whether I can share the current marvell codebase
outside.


>> /* We can also check i2c_check_functionality(adap,I2C_FUNC_PIO) */
>> 	if (msg->flags & I2C_M_PIO) {
>> 		/* PIO mode operation */
>> 	} else {
>> 		/* non-pio mode of operation */
>> 	}
>
> I'd rather let the core decide. Like introduce a master_xfer_nosleep
> callback and let the core use the standard or the nosleep one. We'd need
> the same for smbus as well.
>
> This leads to the question if we need a M_FLAG or expose the *_nosleep
> functions directly?
>

I thought of it, but did not convince myself after looking at the usage
of this. This function would be used only when interrupt is disabled.


> And I wonder about the demand for it. There are a number of SoCs with
> designated I2C cores for the PMIC which do automated transfers.
>

This is exactly the same reason I did not propose it as a core change.
as I feel that the use of this is very minimal and with M_FLAG driver
can very well handle it.

Thanks,
Vaibhav

Re: [RFC]: Supporting PIO mode of operation in i2c_msg->flags

[Vaibhav Hiremath]

On Sunday 14 June 2015 05:42 PM, Vaibhav Hiremath wrote:
>
>
> On Friday 12 June 2015 05:11 AM, Wolfram Sang wrote:
>>> Current implementation:
>>> --------------------
>>>
>>> Everyone probably does have their own custom implementation for this.
>>> Or some other mechanism to achieve this (may be hardware support).
>>
>> Do you have examples? It is easier to look at code.
>>
>
> As I mentioned in the "Problem Statement",
>
> I2C interface for PMIC may need to be used at the end of poweroff
> sequence where you cannot afford to sleep/schedule, as interrupt has
> been disabled.
>
> Earlier I was working on OMAP platform, where I remember we had
> implemented something for such requirement, but as I am not in TI
> anymore, I can not access the internal code.
>
> Let me check whether I can share the current marvell codebase
> outside.
>
>
>>> /* We can also check i2c_check_functionality(adap,I2C_FUNC_PIO) */
>>>     if (msg->flags & I2C_M_PIO) {
>>>         /* PIO mode operation */
>>>     } else {
>>>         /* non-pio mode of operation */
>>>     }
>>
>> I'd rather let the core decide. Like introduce a master_xfer_nosleep
>> callback and let the core use the standard or the nosleep one. We'd need
>> the same for smbus as well.
>>
>> This leads to the question if we need a M_FLAG or expose the *_nosleep
>> functions directly?
>>
>
> I thought of it, but did not convince myself after looking at the usage
> of this. This function would be used only when interrupt is disabled.
>
>
>> And I wonder about the demand for it. There are a number of SoCs with
>> designated I2C cores for the PMIC which do automated transfers.
>>
>
> This is exactly the same reason I did not propose it as a core change.
> as I feel that the use of this is very minimal and with M_FLAG driver
> can very well handle it.
>

Wolfram,

Any update on this?

If we could close on this, I can quickly change the code and submit the
patch for review.

Thanks,
Vaibhav

Re: [RFC]: Supporting PIO mode of operation in i2c_msg->flags

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 203 bytes --]


> Any update on this?

Not yet.

a) there was no code to look at

b) have a look here for current pending patches:

http://patchwork.ozlabs.org/project/linux-i2c/list/

I will answer when time is due.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC]: Supporting PIO mode of operation in i2c_msg->flags

[Vaibhav Hiremath]

On Tuesday 16 June 2015 02:48 PM, Wolfram Sang wrote:
>
>> Any update on this?
>
> Not yet.
>
> a) there was no code to look at
>

Actually its simple question, whether we can call i2c_transfer in
pm_power_off fn, where interupts are disabled and i2c_transfer fn may
sleep.

Just to illustrate my point,
I just quickly created something for you. Correct me if I am wrong here.


pm_power_off Usecase:
=====

File: arch/arm64/kernel/process.c

void machine_power_off(void)
{
         local_irq_disable();
         smp_send_stop();
         if (pm_power_off)
                 pm_power_off();
}


Dummy pm_power_off Implementation:
=====

diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
index 03b70f8..e364a2a 100644
--- a/drivers/i2c/busses/i2c-pxa.c
+++ b/drivers/i2c/busses/i2c-pxa.c
@@ -1170,7 +1170,10 @@ static int i2c_pxa_xfer(struct i2c_adapter *adap, 
struct i2c_msg msgs[], int num
         i2c_pxa_enable(i2c, true);

         for (i = adap->retries; i >= 0; i--) {
-               ret = i2c_pxa_do_xfer(i2c, msgs, num);
+               if (msgs[0].flags & I2C_M_PIO)
+                       ret = i2c_pxa_do_pio_xfer(i2c, msgs, num);
+               else
+                       ret = i2c_pxa_do_xfer(i2c, msgs, num);
                 if (ret != I2C_RETRY)
                         goto out;

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index 0464e2d..2d7b11b 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -488,6 +488,52 @@ static void pm800_pages_exit(struct pm80x_chip *chip)
                 i2c_unregister_device(subchip->gpadc_page);
  }

+static struct pm80x_chip *pm80x_chip_g;
+
+#define PM800_SW_PDOWN                 (1 << 5)
+
+static void pm800_power_off(void)
+{
+       u8 data, buf[2];
+       struct i2c_msg msgs[2];
+       struct i2c_client *client = pm80x_chip_g->client;
+
+       pr_info("turning off power....\n");
+
+       /*
+        * pm_power_off fn get called at the end of machine_power_off(),
+        * so at this stage the irqs are disabled, so we have to use
+        * PIO mode of I2C transaction for both read and write.
+        */
+       /* Read register first */
+       msgs[0].addr = client->addr;
+       msgs[0].flags = I2C_M_PIO;
+       msgs[0].len = 1;
+       msgs[0].buf = buf;
+
+       msgs[1].addr = client->addr;
+       msgs[1].flags = I2C_M_RD | I2C_M_PIO;
+       msgs[1].len = 1;
+       msgs[1].buf = &data;
+
+       buf[0] = PM800_WAKEUP1;
+       if ( __i2c_transfer(client->adapter, msgs, 2) < 0) {
+               pr_err("%s read register fails...\n", __func__);
+               WARN_ON(1);
+       }
+
+       /* issue SW power down */
+       msgs[0].addr = client->addr;
+       msgs[0].flags = I2C_M_PIO;
+       msgs[0].len = 2;
+       msgs[0].buf[0] = PM800_WAKEUP1;
+       msgs[0].buf[1] = data | PM800_SW_PDOWN;
+       if (__i2c_transfer(client->adapter, msgs, 1) < 0) {
+               pr_err("%s write data fails...\n", __func__);
+               WARN_ON(1);
+       }
+}
+
  static int device_800_init(struct pm80x_chip *chip,
                                      struct pm80x_platform_data *pdata)
  {
@@ -612,6 +658,10 @@ static int pm800_probe(struct i2c_client *client,
         if (pdata && pdata->plat_config)
                 pdata->plat_config(chip, pdata);

+       /* keep global copy, required in power_off fn */
+       pm80x_chip_g = chip;
+       pm_power_off = pm800_power_off;
+
         return 0;

  err_device_init:
diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index 0e949cb..22fda83 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -76,6 +76,7 @@ struct i2c_msg {
  #define I2C_M_IGNORE_NAK       0x1000  /* if I2C_FUNC_PROTOCOL_MANGLING */
  #define I2C_M_NO_RD_ACK                0x0800  /* if 
I2C_FUNC_PROTOCOL_MANGLING */
  #define I2C_M_RECV_LEN         0x0400  /* length will be first 
received byte */
+#define I2C_M_PIO              0x0200  /* pio mode of transaction */
         __u16 len;              /* msg length                           */
         __u8 *buf;              /* pointer to msg data                  */
  };