[PATCH] NFC: NCI: make parent aware in PM terms

[PATCH] NFC: NCI: make parent aware in PM terms

[Oliver Neukum]

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

The NCI device is a child of an i2c device.
If the i2c layer uses runtime PM the power to
the NFC device can be cut whenever the i2c
layer is done transmitting data to the NFC
device.
Under these conditions NFC can not work, as
it needs power to wait for reception of packets.

The necessary extension of runtime PM
to the NFC device requires that it
be activated as a child of the i2c device.
It is not necessary to do any runtime PM
operations. This will disable runtime PM
for this branch of the tree, but otherwise
the NFC device is inoperable.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/nfc/nxp-nci/i2c.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/nfc/nxp-nci/i2c.c b/drivers/nfc/nxp-nci/i2c.c
index 94f7f6d9cbad..dba78a5d217a 100644
--- a/drivers/nfc/nxp-nci/i2c.c
+++ b/drivers/nfc/nxp-nci/i2c.c
@@ -18,6 +18,7 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/nfc.h>
+#include <linux/pm_runtime.h>
 #include <linux/gpio/consumer.h>
 #include <asm/unaligned.h>
 
@@ -261,6 +262,7 @@ static int nxp_nci_i2c_probe(struct i2c_client *client,
 {
 	struct device *dev = &client->dev;
 	struct nxp_nci_i2c_phy *phy;
+	struct nfc_dev *ndev;
 	int r;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
@@ -297,6 +299,11 @@ static int nxp_nci_i2c_probe(struct i2c_client *client,
 	if (r < 0)
 		return r;
 
+	ndev = phy->ndev->nfc_dev;
+	pm_runtime_set_active(&ndev->dev);
+	pm_runtime_enable(&ndev->dev);
+	pm_runtime_mark_last_busy(&ndev->dev);
+
 	r = request_threaded_irq(client->irq, NULL,
 				 nxp_nci_i2c_irq_thread_fn,
 				 IRQF_TRIGGER_RISING | IRQF_ONESHOT,
@@ -310,9 +317,12 @@ static int nxp_nci_i2c_probe(struct i2c_client *client,
 static int nxp_nci_i2c_remove(struct i2c_client *client)
 {
 	struct nxp_nci_i2c_phy *phy = i2c_get_clientdata(client);
+	struct nfc_dev *ndev = phy->ndev->nfc_dev;
 
 	nxp_nci_remove(phy->ndev);
 	free_irq(client->irq, phy);
+	pm_runtime_disable(&ndev->dev);
+	pm_runtime_set_suspended(&ndev->dev);
 
 	return 0;
 }
-- 
2.26.2

Re: [PATCH] NFC: NCI: make parent aware in PM terms

[Krzysztof Kozlowski]

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


On 19/08/2021 16:02, Oliver Neukum wrote:
> The NCI device is a child of an i2c device.
> If the i2c layer uses runtime PM the power to
> the NFC device can be cut whenever the i2c
> layer is done transmitting data to the NFC
> device.
> Under these conditions NFC can not work, as
> it needs power to wait for reception of packets.

Hi,

Thanks for the patch.
However this
is unparseable.
Please wrap commit
message around 75th character:
https://elixir.bootlin.com/linux/v5.13/source/Documentation/process/submitting-patches.rst#L124

The subject prefix: "nfc: nxp-nci: "

Please also Cc all people and lists. You missed here netdev
and linux-kernel, so this cannot be applied.

> 
> The necessary extension of runtime PM
> to the NFC device requires that it
> be activated as a child of the i2c device.
> It is not necessary to do any runtime PM
> operations. This will disable runtime PM
> for this branch of the tree, but otherwise
> the NFC device is inoperable.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/nfc/nxp-nci/i2c.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/nfc/nxp-nci/i2c.c b/drivers/nfc/nxp-nci/i2c.c
> index 94f7f6d9cbad..dba78a5d217a 100644
> --- a/drivers/nfc/nxp-nci/i2c.c
> +++ b/drivers/nfc/nxp-nci/i2c.c
> @@ -18,6 +18,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/nfc.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/gpio/consumer.h>
>  #include <asm/unaligned.h>
>  
> @@ -261,6 +262,7 @@ static int nxp_nci_i2c_probe(struct i2c_client *client,
>  {
>  	struct device *dev = &client->dev;
>  	struct nxp_nci_i2c_phy *phy;
> +	struct nfc_dev *ndev;
>  	int r;
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> @@ -297,6 +299,11 @@ static int nxp_nci_i2c_probe(struct i2c_client *client,
>  	if (r < 0)
>  		return r;
>  
> +	ndev = phy->ndev->nfc_dev;
> +	pm_runtime_set_active(&ndev->dev);
> +	pm_runtime_enable(&ndev->dev);
> +	pm_runtime_mark_last_busy(&ndev->dev);

Setting PM for someone else does not look correct. This breaks
encapsulation and separation of these files. The NCI device
(nxp_nci_probe() and other parts of core.c) should instead manage
it's own runtime PM.

> +
>  	r = request_threaded_irq(client->irq, NULL,
>  				 nxp_nci_i2c_irq_thread_fn,
>  				 IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> @@ -310,9 +317,12 @@ static int nxp_nci_i2c_probe(struct i2c_client *client,
>  static int nxp_nci_i2c_remove(struct i2c_client *client)
>  {
>  	struct nxp_nci_i2c_phy *phy = i2c_get_clientdata(client);
> +	struct nfc_dev *ndev = phy->ndev->nfc_dev;
>  
>  	nxp_nci_remove(phy->ndev);
>  	free_irq(client->irq, phy);
> +	pm_runtime_disable(&ndev->dev);
> +	pm_runtime_set_suspended(&ndev->dev);
>  
>  	return 0;
>  }
> 


Best regards,
Krzysztof

Re: [PATCH] NFC: NCI: make parent aware in PM terms

[Krzysztof Kozlowski]

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

On 19/08/2021 16:45, Krzysztof Kozlowski wrote:
> 
> On 19/08/2021 16:02, Oliver Neukum wrote:
>> The NCI device is a child of an i2c device.
>> If the i2c layer uses runtime PM the power to
>> the NFC device can be cut whenever the i2c
>> layer is done transmitting data to the NFC
>> device.
>> Under these conditions NFC can not work, as
>> it needs power to wait for reception of packets.
> 
> Hi,
> 
> Thanks for the patch.
> However this
> is unparseable.
> Please wrap commit
> message around 75th character:
> https://elixir.bootlin.com/linux/v5.13/source/Documentation/process/submitting-patches.rst#L124
> 
> The subject prefix: "nfc: nxp-nci: "
> 
> Please also Cc all people and lists. You missed here netdev
> and linux-kernel, so this cannot be applied.
> 
>>
>> The necessary extension of runtime PM
>> to the NFC device requires that it
>> be activated as a child of the i2c device.
>> It is not necessary to do any runtime PM
>> operations. This will disable runtime PM
>> for this branch of the tree, but otherwise
>> the NFC device is inoperable.
>>
>> Signed-off-by: Oliver Neukum <oneukum@suse.com>
>> ---
>>  drivers/nfc/nxp-nci/i2c.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/nfc/nxp-nci/i2c.c b/drivers/nfc/nxp-nci/i2c.c
>> index 94f7f6d9cbad..dba78a5d217a 100644
>> --- a/drivers/nfc/nxp-nci/i2c.c
>> +++ b/drivers/nfc/nxp-nci/i2c.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/module.h>
>>  #include <linux/nfc.h>
>> +#include <linux/pm_runtime.h>
>>  #include <linux/gpio/consumer.h>
>>  #include <asm/unaligned.h>
>>  
>> @@ -261,6 +262,7 @@ static int nxp_nci_i2c_probe(struct i2c_client *client,
>>  {
>>  	struct device *dev = &client->dev;
>>  	struct nxp_nci_i2c_phy *phy;
>> +	struct nfc_dev *ndev;
>>  	int r;
>>  
>>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> @@ -297,6 +299,11 @@ static int nxp_nci_i2c_probe(struct i2c_client *client,
>>  	if (r < 0)
>>  		return r;
>>  
>> +	ndev = phy->ndev->nfc_dev;
>> +	pm_runtime_set_active(&ndev->dev);
>> +	pm_runtime_enable(&ndev->dev);
>> +	pm_runtime_mark_last_busy(&ndev->dev);
> 
> Setting PM for someone else does not look correct. This breaks
> encapsulation and separation of these files. The NCI device
> (nxp_nci_probe() and other parts of core.c) should instead manage
> it's own runtime PM.
> 

Except this, the code is really weird... there is no runtime PM in the
drivers but this enables it. For what purpose? It also marks it as last
busy but there is no autosuspend. I think I missed entirely the purpose
of this code.

Best regards,
Krzysztof

Re: [PATCH] NFC: NCI: make parent aware in PM terms

[Oliver Neukum]

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


On 19.08.21 16:45, Krzysztof Kozlowski wrote
Hi,
> Hi,
>
> Thanks for the patch.
> However this
> is unparseable.
> Please wrap commit
> message around 75th character:
> https://elixir.bootlin.com/linux/v5.13/source/Documentation/process/submitting-patches.rst#L124
>
> The subject prefix: "nfc: nxp-nci: "
OK.
> Please also Cc all people and lists. You missed here netdev
> and linux-kernel, so this cannot be applied.
Do you really want LKML on CC for all NFC patches?
>> The necessary extension of runtime PM
>> to the NFC device requires that it
>> be activated as a child of the i2c device.
>> It is not necessary to do any runtime PM
>> operations. This will disable runtime PM
>> for this branch of the tree, but otherwise
>> the NFC device is inoperable.
>>
>> Signed-off-by: Oliver Neukum <oneukum@suse.com>
>> ---
>>  drivers/nfc/nxp-nci/i2c.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/nfc/nxp-nci/i2c.c b/drivers/nfc/nxp-nci/i2c.c
>> index 94f7f6d9cbad..dba78a5d217a 100644
>> --- a/drivers/nfc/nxp-nci/i2c.c
>> +++ b/drivers/nfc/nxp-nci/i2c.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/module.h>
>>  #include <linux/nfc.h>
>> +#include <linux/pm_runtime.h>
>>  #include <linux/gpio/consumer.h>
>>  #include <asm/unaligned.h>
>>  
>> @@ -261,6 +262,7 @@ static int nxp_nci_i2c_probe(struct i2c_client *client,
>>  {
>>  	struct device *dev = &client->dev;
>>  	struct nxp_nci_i2c_phy *phy;
>> +	struct nfc_dev *ndev;
>>  	int r;
>>  
>>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> @@ -297,6 +299,11 @@ static int nxp_nci_i2c_probe(struct i2c_client *client,
>>  	if (r < 0)
>>  		return r;
>>  
>> +	ndev = phy->ndev->nfc_dev;
>> +	pm_runtime_set_active(&ndev->dev);
>> +	pm_runtime_enable(&ndev->dev);
>> +	pm_runtime_mark_last_busy(&ndev->dev);
> Setting PM for someone else does not look correct. This breaks
As far as I can tell I am doing this for the device noted in nfc_dev.
Could you elaborate?
> encapsulation and separation of these files. The NCI device
> (nxp_nci_probe() and other parts of core.c) should instead manage
> it's own runtime PM.

That would be better. The problem I encountered is that i2c and USB have
different
needs concerning runtime PM. Any ideas?

    Regards
        Oliver

Re: [PATCH] NFC: NCI: make parent aware in PM terms

[Krzysztof Kozlowski]

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

On 23/08/2021 12:50, Oliver Neukum wrote:
> 
> On 19.08.21 16:45, Krzysztof Kozlowski wrote
> Hi,
>> Hi,
>>
>> Thanks for the patch.
>> However this
>> is unparseable.
>> Please wrap commit
>> message around 75th character:
>> https://elixir.bootlin.com/linux/v5.13/source/Documentation/process/submitting-patches.rst#L124
>>
>> The subject prefix: "nfc: nxp-nci: "
> OK.
>> Please also Cc all people and lists. You missed here netdev
>> and linux-kernel, so this cannot be applied.
> Do you really want LKML on CC for all NFC patches?

I personally like it a lot because I have filters organized with it.
Nowadays no one reads LKML itself (too big volume) so it is purely for
archiving on lore.kernel.org for searching and for people's filters.

Therefore unless someone here objects, I would prefer to Cc LKML as
well. Anyway, netdev is important as it is tracked by patchwork.

>>> The necessary extension of runtime PM
>>> to the NFC device requires that it
>>> be activated as a child of the i2c device.
>>> It is not necessary to do any runtime PM
>>> operations. This will disable runtime PM
>>> for this branch of the tree, but otherwise
>>> the NFC device is inoperable.
>>>
>>> Signed-off-by: Oliver Neukum <oneukum@suse.com>
>>> ---
>>>  drivers/nfc/nxp-nci/i2c.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/nfc/nxp-nci/i2c.c b/drivers/nfc/nxp-nci/i2c.c
>>> index 94f7f6d9cbad..dba78a5d217a 100644
>>> --- a/drivers/nfc/nxp-nci/i2c.c
>>> +++ b/drivers/nfc/nxp-nci/i2c.c
>>> @@ -18,6 +18,7 @@
>>>  #include <linux/interrupt.h>
>>>  #include <linux/module.h>
>>>  #include <linux/nfc.h>
>>> +#include <linux/pm_runtime.h>
>>>  #include <linux/gpio/consumer.h>
>>>  #include <asm/unaligned.h>
>>>  
>>> @@ -261,6 +262,7 @@ static int nxp_nci_i2c_probe(struct i2c_client *client,
>>>  {
>>>  	struct device *dev = &client->dev;
>>>  	struct nxp_nci_i2c_phy *phy;
>>> +	struct nfc_dev *ndev;
>>>  	int r;
>>>  
>>>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>>> @@ -297,6 +299,11 @@ static int nxp_nci_i2c_probe(struct i2c_client *client,
>>>  	if (r < 0)
>>>  		return r;
>>>  
>>> +	ndev = phy->ndev->nfc_dev;
>>> +	pm_runtime_set_active(&ndev->dev);
>>> +	pm_runtime_enable(&ndev->dev);
>>> +	pm_runtime_mark_last_busy(&ndev->dev);
>> Setting PM for someone else does not look correct. This breaks
> As far as I can tell I am doing this for the device noted in nfc_dev.
> Could you elaborate?

I meant that it looks unusual that you don't do it for your own device
(client->dev) but for device allocated in different unit. Here, you
receive client->dev and mostly you should play only with it.

While I am looking at this more, there is another issue actually - you
touch runtime PM of NFC/NCI core device but it's the NFC/NCI core who
should handle it's own runtime PM.

>> encapsulation and separation of these files. The NCI device
>> (nxp_nci_probe() and other parts of core.c) should instead manage
>> it's own runtime PM.
> 
> That would be better. The problem I encountered is that i2c and USB have
> different
> needs concerning runtime PM. Any ideas?

I think we look at different code bases. I failed to find the USB
device... It's difficult for me to judge how the final runtime PM should
look like because actually you don't do any runtime PM here. You just
enable it. Where are the callbacks? Where is suspending and resuming
(get/put)?


Best regards,
Krzysztof

Re: [PATCH] NFC: NCI: make parent aware in PM terms

[Oliver Neukum]

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


On 23.08.21 13:29, Krzysztof Kozlowski wrote:
> On 23/08/2021 12:50, Oliver Neukum wrote:
>> On 19.08.21 16:45, Krzysztof Kozlowski wrote


Hi,

>> I personally like it a lot because I have filters organized with it.
>> Nowadays no one reads LKML itself (too big volume) so it is purely for
>> archiving on lore.kernel.org for searching and for people's filters.
>>
>> Therefore unless someone here objects, I would prefer to Cc LKML as
>> well. Anyway, netdev is important as it is tracked by patchwork.
very well, It shall be done.
>> As far as I can tell I am doing this for the device noted in nfc_dev.
>> Could you elaborate?
> I meant that it looks unusual that you don't do it for your own device
> (client->dev) but for device allocated in different unit. Here, you
> receive client->dev and mostly you should play only with it.
>
> While I am looking at this more, there is another issue actually - you
> touch runtime PM of NFC/NCI core device but it's the NFC/NCI core who
> should handle it's own runtime PM.

Well, I looked into that and that was very difficult. The NFC core has
no idea what hardware it is running on. On my test hardware as far as
I can tell, the power supply of my NFC hardware is just tied to
the power supply of the i2c host controller, which is a PCI device.

It goes into D3cold when the i2c bus is not transferring anything.
That makes it impossible to actually communicate over NFC.

However, at least on USB sending a device into USB suspend with
remote wakeup should work. So I do not see how to put this into
NFC core.

    Regards
        Oliver

Re: [PATCH] NFC: NCI: make parent aware in PM terms

[Krzysztof Kozlowski]

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

On 23/08/2021 13:52, Oliver Neukum wrote:
> 
> On 23.08.21 13:29, Krzysztof Kozlowski wrote:
>> On 23/08/2021 12:50, Oliver Neukum wrote:
>>> On 19.08.21 16:45, Krzysztof Kozlowski wrote
> 
> 
> Hi,
> 
>>> I personally like it a lot because I have filters organized with it.
>>> Nowadays no one reads LKML itself (too big volume) so it is purely for
>>> archiving on lore.kernel.org for searching and for people's filters.
>>>
>>> Therefore unless someone here objects, I would prefer to Cc LKML as
>>> well. Anyway, netdev is important as it is tracked by patchwork.
> very well, It shall be done.
>>> As far as I can tell I am doing this for the device noted in nfc_dev.
>>> Could you elaborate?
>> I meant that it looks unusual that you don't do it for your own device
>> (client->dev) but for device allocated in different unit. Here, you
>> receive client->dev and mostly you should play only with it.
>>
>> While I am looking at this more, there is another issue actually - you
>> touch runtime PM of NFC/NCI core device but it's the NFC/NCI core who
>> should handle it's own runtime PM.
> 
> Well, I looked into that and that was very difficult. The NFC core has
> no idea what hardware it is running on. On my test hardware as far as
> I can tell, the power supply of my NFC hardware is just tied to
> the power supply of the i2c host controller, which is a PCI device.
> 
> It goes into D3cold when the i2c bus is not transferring anything.
> That makes it impossible to actually communicate over NFC.
> 
> However, at least on USB sending a device into USB suspend with
> remote wakeup should work. So I do not see how to put this into
> NFC core.

If I understand your case correctly, you have a tree of devices:
PCI I2C bus controller
 - NXP NFC I2C device
   - NFC NCI device

and the only thing you care is to make PCI I2C bus resumed all the time.
The simplest seems to add PM runtime to the NXP NFC I2C device (so
i2c.c) with get_noresume+active+enable.


Best regards,
Krzysztof