[PATCH v2 0/3] can: m_can: Add PM Runtime Support

[PATCH v2 0/3] can: m_can: Add PM Runtime Support

[Franklin S Cooper Jr]

Add PM runtime support to the MCAN driver. To support devices that don't
use PM runtime leave the original clk calls in the driver. Perhaps in the
future when it makes sense we can remove these non pm runtime clk calls.

Version 2 changes:
Used NULL instead of 0 for unused hclk handle

Franklin S Cooper Jr (3):
  can: m_can: Make hclk optional
  can: m_can: Update documentation to indicate that hclk may be optional
  can: m_can: Add PM Runtime

 .../devicetree/bindings/net/can/m_can.txt          |  3 ++-
 drivers/net/can/m_can/m_can.c                      | 22 ++++++++++++++++++++--
 2 files changed, 22 insertions(+), 3 deletions(-)

-- 
2.10.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] can: m_can: Make hclk optional

[Franklin S Cooper Jr]

Hclk is the MCAN's interface clock. However, for OMAP based devices such as
DRA7 SoC family the interface clock is handled by hwmod. Therefore, this
interface clock is managed by hwmod driver via pm_runtime_get and
pm_runtime_put calls. Therefore, this interface clock isn't defined in DT
and thus the driver shouldn't fail if this clock isn't found.

Signed-off-by: Franklin S Cooper Jr <fcooper-l0cyMroinI0@public.gmane.org>
---
Version 2 changes:
Used NULL instead of 0 for unused hclk handle

 drivers/net/can/m_can/m_can.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index f4947a7..ea48e59 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1568,8 +1568,13 @@ static int m_can_plat_probe(struct platform_device *pdev)
 	hclk = devm_clk_get(&pdev->dev, "hclk");
 	cclk = devm_clk_get(&pdev->dev, "cclk");
 
-	if (IS_ERR(hclk) || IS_ERR(cclk)) {
-		dev_err(&pdev->dev, "no clock found\n");
+	if (IS_ERR(hclk)) {
+		dev_warn(&pdev->dev, "hclk could not be found\n");
+		hclk = NULL;
+	}
+
+	if (IS_ERR(cclk)) {
+		dev_err(&pdev->dev, "cclk could not be found\n");
 		ret = -ENODEV;
 		goto failed_ret;
 	}
-- 
2.10.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v2,1/3] can: m_can: Make hclk optional

[Sekhar Nori]

+ some OMAP folks and Linux OMAP list

On Tuesday 25 July 2017 04:21 AM, Franklin Cooper wrote:
> Hclk is the MCAN's interface clock. However, for OMAP based devices such as
> DRA7 SoC family the interface clock is handled by hwmod. Therefore, this
> interface clock is managed by hwmod driver via pm_runtime_get and
> pm_runtime_put calls. Therefore, this interface clock isn't defined in DT
> and thus the driver shouldn't fail if this clock isn't found.

I agree that hclk is defined as interface clock for M_CAN IP on DRA76x.

However, there may be a need for the driver to know the value of hclk to
properly configure the RAM watchdog register which has a counter
counting down using hclk. Looks like the driver does not use the RAM
watchdog today. But if there is a need to configure it in future, it
might be a problem.

Is there a restriction in OMAP architecture against passing the
interface clock also in the 'clocks' property in DT. I have not tried it
myself, but wonder if you hit an issue that led to this patch.

> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
> Version 2 changes:
> Used NULL instead of 0 for unused hclk handle
> 
>  drivers/net/can/m_can/m_can.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index f4947a7..ea48e59 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1568,8 +1568,13 @@ static int m_can_plat_probe(struct platform_device *pdev)
>  	hclk = devm_clk_get(&pdev->dev, "hclk");
>  	cclk = devm_clk_get(&pdev->dev, "cclk");
>  
> -	if (IS_ERR(hclk) || IS_ERR(cclk)) {
> -		dev_err(&pdev->dev, "no clock found\n");
> +	if (IS_ERR(hclk)) {
> +		dev_warn(&pdev->dev, "hclk could not be found\n");
> +		hclk = NULL;

What is the purpose of NULL setting the clock. I think this is taking it
into a very implementation defined territory and the result could be
different on different architectures. See Russell's explanation here:
https://lkml.org/lkml/2016/11/10/799

Thanks,
Sekhar

> +	}
> +
> +	if (IS_ERR(cclk)) {
> +		dev_err(&pdev->dev, "cclk could not be found\n");
>  		ret = -ENODEV;
>  		goto failed_ret;
>  	}
> 

Re: [v2,1/3] can: m_can: Make hclk optional

[Franklin S Cooper Jr]

On 08/24/2017 03:00 AM, Sekhar Nori wrote:
> + some OMAP folks and Linux OMAP list
> 
> On Tuesday 25 July 2017 04:21 AM, Franklin Cooper wrote:
>> Hclk is the MCAN's interface clock. However, for OMAP based devices such as
>> DRA7 SoC family the interface clock is handled by hwmod. Therefore, this
>> interface clock is managed by hwmod driver via pm_runtime_get and
>> pm_runtime_put calls. Therefore, this interface clock isn't defined in DT
>> and thus the driver shouldn't fail if this clock isn't found.
> 
> I agree that hclk is defined as interface clock for M_CAN IP on DRA76x.
> 
> However, there may be a need for the driver to know the value of hclk to
> properly configure the RAM watchdog register which has a counter
> counting down using hclk. Looks like the driver does not use the RAM
> watchdog today. But if there is a need to configure it in future, it
> might be a problem.

Honestly the RAM watchdog seems like a fundamental design problem.
This RAM watchdog seems to be used in case a request to access the
message ram is made but it hangs for what ever reason. Its even more
complicated since the Message RAM is external to the MCAN IP so its
implementation or how its handled probably differs from device to
device. From example say you do have this error it isn't clear how you
would recover from it. A logically answer would be to reset the entire
IP but that also assumes that Message RAM will be reset along with the
ip which likely depends on each SoC.

But if a readl/writel command hangs will the kernel eventually throw an
error on its on or will the driver just hang? If it does hang can a
driver in the ISR do something to properly terminate the driver or even
recover from it?
> 
> Is there a restriction in OMAP architecture against passing the
> interface clock also in the 'clocks' property in DT. I have not tried it
> myself, but wonder if you hit an issue that led to this patch.

No but not passing the interface clock is typical.
> 
>>
>> Signed-off-by: Franklin S Cooper Jr <fcooper-l0cyMroinI0@public.gmane.org>
>> ---
>> Version 2 changes:
>> Used NULL instead of 0 for unused hclk handle
>>
>>  drivers/net/can/m_can/m_can.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index f4947a7..ea48e59 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -1568,8 +1568,13 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>  	hclk = devm_clk_get(&pdev->dev, "hclk");
>>  	cclk = devm_clk_get(&pdev->dev, "cclk");
>>  
>> -	if (IS_ERR(hclk) || IS_ERR(cclk)) {
>> -		dev_err(&pdev->dev, "no clock found\n");
>> +	if (IS_ERR(hclk)) {
>> +		dev_warn(&pdev->dev, "hclk could not be found\n");
>> +		hclk = NULL;
> 
> What is the purpose of NULL setting the clock. I think this is taking it
> into a very implementation defined territory and the result could be
> different on different architectures. See Russell's explanation here:
> https://lkml.org/lkml/2016/11/10/799
> 
> Thanks,
> Sekhar
> 
>> +	}
>> +
>> +	if (IS_ERR(cclk)) {
>> +		dev_err(&pdev->dev, "cclk could not be found\n");
>>  		ret = -ENODEV;
>>  		goto failed_ret;
>>  	}
>>
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v2,1/3] can: m_can: Make hclk optional

[Sekhar Nori]

On Thursday 21 September 2017 06:01 AM, Franklin S Cooper Jr wrote:
> 
> 
> On 08/24/2017 03:00 AM, Sekhar Nori wrote:
>> + some OMAP folks and Linux OMAP list
>>
>> On Tuesday 25 July 2017 04:21 AM, Franklin Cooper wrote:
>>> Hclk is the MCAN's interface clock. However, for OMAP based devices such as
>>> DRA7 SoC family the interface clock is handled by hwmod. Therefore, this
>>> interface clock is managed by hwmod driver via pm_runtime_get and
>>> pm_runtime_put calls. Therefore, this interface clock isn't defined in DT
>>> and thus the driver shouldn't fail if this clock isn't found.
>>
>> I agree that hclk is defined as interface clock for M_CAN IP on DRA76x.
>>
>> However, there may be a need for the driver to know the value of hclk to
>> properly configure the RAM watchdog register which has a counter
>> counting down using hclk. Looks like the driver does not use the RAM
>> watchdog today. But if there is a need to configure it in future, it
>> might be a problem.
> 
> Honestly the RAM watchdog seems like a fundamental design problem.
> This RAM watchdog seems to be used in case a request to access the
> message ram is made but it hangs for what ever reason. Its even more
> complicated since the Message RAM is external to the MCAN IP so its
> implementation or how its handled probably differs from device to
> device. From example say you do have this error it isn't clear how you
> would recover from it. A logically answer would be to reset the entire
> IP but that also assumes that Message RAM will be reset along with the
> ip which likely depends on each SoC.
> 
> But if a readl/writel command hangs will the kernel eventually throw an
> error on its on or will the driver just hang? If it does hang can a
> driver in the ISR do something to properly terminate the driver or even
> recover from it?
>>
>> Is there a restriction in OMAP architecture against passing the
>> interface clock also in the 'clocks' property in DT. I have not tried it
>> myself, but wonder if you hit an issue that led to this patch.
> 
> No but not passing the interface clock is typical.

Okay, then it sounds like it will just be easier to pass the hclk too?

So it can be used if needed in future and also so that the driver can
stay the same as today.

Thanks,
Sekhar

Re: [v2,1/3] can: m_can: Make hclk optional

[Franklin S Cooper Jr]

On 09/21/2017 09:08 AM, Sekhar Nori wrote:
> On Thursday 21 September 2017 06:01 AM, Franklin S Cooper Jr wrote:
>>
>>
>> On 08/24/2017 03:00 AM, Sekhar Nori wrote:
>>> + some OMAP folks and Linux OMAP list
>>>
>>> On Tuesday 25 July 2017 04:21 AM, Franklin Cooper wrote:
>>>> Hclk is the MCAN's interface clock. However, for OMAP based devices such as
>>>> DRA7 SoC family the interface clock is handled by hwmod. Therefore, this
>>>> interface clock is managed by hwmod driver via pm_runtime_get and
>>>> pm_runtime_put calls. Therefore, this interface clock isn't defined in DT
>>>> and thus the driver shouldn't fail if this clock isn't found.
>>>
>>> I agree that hclk is defined as interface clock for M_CAN IP on DRA76x.
>>>
>>> However, there may be a need for the driver to know the value of hclk to
>>> properly configure the RAM watchdog register which has a counter
>>> counting down using hclk. Looks like the driver does not use the RAM
>>> watchdog today. But if there is a need to configure it in future, it
>>> might be a problem.
>>
>> Honestly the RAM watchdog seems like a fundamental design problem.
>> This RAM watchdog seems to be used in case a request to access the
>> message ram is made but it hangs for what ever reason. Its even more
>> complicated since the Message RAM is external to the MCAN IP so its
>> implementation or how its handled probably differs from device to
>> device. From example say you do have this error it isn't clear how you
>> would recover from it. A logically answer would be to reset the entire
>> IP but that also assumes that Message RAM will be reset along with the
>> ip which likely depends on each SoC.
>>
>> But if a readl/writel command hangs will the kernel eventually throw an
>> error on its on or will the driver just hang? If it does hang can a
>> driver in the ISR do something to properly terminate the driver or even
>> recover from it?
>>>
>>> Is there a restriction in OMAP architecture against passing the
>>> interface clock also in the 'clocks' property in DT. I have not tried it
>>> myself, but wonder if you hit an issue that led to this patch.
>>
>> No but not passing the interface clock is typical.
> 
> Okay, then it sounds like it will just be easier to pass the hclk too?
> 
> So it can be used if needed in future and also so that the driver can
> stay the same as today.

That is fine. For now I can just drop this patch unless I discover when
enabling it on DRA76x I am unable to add the interface clock to dt.
> 
> Thanks,
> Sekhar
> 

Re: [PATCH v2 1/3] can: m_can: Make hclk optional

[Mario Hüttel]

[-- Attachment #1.1: Type: text/plain, Size: 2347 bytes --]

> Hclk is the MCAN's interface clock. However, for OMAP based devices such as
> DRA7 SoC family the interface clock is handled by hwmod. Therefore, this
> interface clock is managed by hwmod driver via pm_runtime_get and
> pm_runtime_put calls. Therefore, this interface clock isn't defined in DT
> and thus the driver shouldn't fail if this clock isn't found.
I also agree in this point.
However, there's a problem I want to point out:

The M_CAN can only function correctly, if the condition
'hclk >= cclk' holds true.

The internal clock domain crossing can fail if this condition
is violated.

I thought about adding the condition to the driver to ensure
correct operation. But I had some problems:

1. Determine the clock rates:
    The devices you've mentioned above don't have an assigned
    hclk. Is there still a possibility to get the clock frequency?

2. What to do if 'hclk < cclk'?
    Is there a general way to process such an error? - I think not.
    Is a simple warning in case of an error enough?

Is there a way of ensuring that the condition is always met at all?

I think it is quite unlikely that the condition is violated, because
devices that actually run Linux usually have (bus) clock rates that
are high enough to ensure the correct operation.

Would it be safe to just ignore this possible fault?

Regards

Mario
   
>
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
> Version 2 changes:
> Used NULL instead of 0 for unused hclk handle
>
>  drivers/net/can/m_can/m_can.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index f4947a7..ea48e59 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1568,8 +1568,13 @@ static int m_can_plat_probe(struct platform_device *pdev)
>  	hclk = devm_clk_get(&pdev->dev, "hclk");
>  	cclk = devm_clk_get(&pdev->dev, "cclk");
>  
> -	if (IS_ERR(hclk) || IS_ERR(cclk)) {
> -		dev_err(&pdev->dev, "no clock found\n");
> +	if (IS_ERR(hclk)) {
> +		dev_warn(&pdev->dev, "hclk could not be found\n");
> +		hclk = NULL;
> +	}
> +
> +	if (IS_ERR(cclk)) {
> +		dev_err(&pdev->dev, "cclk could not be found\n");
>  		ret = -ENODEV;
>  		goto failed_ret;
>  	}



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/3] can: m_can: Make hclk optional

[Franklin S Cooper Jr]

On 09/20/2017 05:00 PM, Mario Hüttel wrote:
>> Hclk is the MCAN's interface clock. However, for OMAP based devices such as
>> DRA7 SoC family the interface clock is handled by hwmod. Therefore, this
>> interface clock is managed by hwmod driver via pm_runtime_get and
>> pm_runtime_put calls. Therefore, this interface clock isn't defined in DT
>> and thus the driver shouldn't fail if this clock isn't found.
> I also agree in this point.
> However, there's a problem I want to point out:
> 
> The M_CAN can only function correctly, if the condition
> 'hclk >= cclk' holds true.
> 
> The internal clock domain crossing can fail if this condition
> is violated.
> 
> I thought about adding the condition to the driver to ensure
> correct operation. But I had some problems:
> 
> 1. Determine the clock rates:
>     The devices you've mentioned above don't have an assigned
>     hclk. Is there still a possibility to get the clock frequency?

I believe interface clocks via hwmod aren't exposed to drivers. So the
only way to get access to the clock frequency is to add this interface
clock to dt.

> 
> 2. What to do if 'hclk < cclk'?
>     Is there a general way to process such an error? - I think not.
>     Is a simple warning in case of an error enough?
> 
> Is there a way of ensuring that the condition is always met at all?
> 
> I think it is quite unlikely that the condition is violated, because
> devices that actually run Linux usually have (bus) clock rates that
> are high enough to ensure the correct operation.
> 
> Would it be safe to just ignore this possible fault?

I think alot of peripherals have similar constraints when there is an
interface and functional clock. However, I haven't seen drivers verify
that this kind of constraint is properly met. Personally I think its a
valid fault but one that can be ignored from the driver perspective.
> 
> Regards
> 
> Mario
>    
>>
>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>> ---
>> Version 2 changes:
>> Used NULL instead of 0 for unused hclk handle
>>
>>  drivers/net/can/m_can/m_can.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index f4947a7..ea48e59 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -1568,8 +1568,13 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>  	hclk = devm_clk_get(&pdev->dev, "hclk");
>>  	cclk = devm_clk_get(&pdev->dev, "cclk");
>>  
>> -	if (IS_ERR(hclk) || IS_ERR(cclk)) {
>> -		dev_err(&pdev->dev, "no clock found\n");
>> +	if (IS_ERR(hclk)) {
>> +		dev_warn(&pdev->dev, "hclk could not be found\n");
>> +		hclk = NULL;
>> +	}
>> +
>> +	if (IS_ERR(cclk)) {
>> +		dev_err(&pdev->dev, "cclk could not be found\n");
>>  		ret = -ENODEV;
>>  		goto failed_ret;
>>  	}
> 
> 

[PATCH v2 2/3] can: m_can: Update documentation to indicate that hclk may be optional

[Franklin S Cooper Jr]

Update the documentation to reflect that hclk is now an optional clock.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/net/can/m_can.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
index 9e33177..2a0fe5b 100644
--- a/Documentation/devicetree/bindings/net/can/m_can.txt
+++ b/Documentation/devicetree/bindings/net/can/m_can.txt
@@ -12,7 +12,8 @@ Required properties:
 - interrupt-names	: Should contain "int0" and "int1"
 - clocks		: Clocks used by controller, should be host clock
 			  and CAN clock.
-- clock-names		: Should contain "hclk" and "cclk"
+- clock-names		: Should contain "hclk" and "cclk". For some socs hclk
+			  may be optional.
 - pinctrl-<n>		: Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt
 - pinctrl-names 	: Names corresponding to the numbered pinctrl states
 - bosch,mram-cfg	: Message RAM configuration data.
-- 
2.10.0

[PATCH v2 3/3] can: m_can: Add PM Runtime

[Franklin S Cooper Jr]

Add support for PM Runtime which is the new way to handle managing clocks.
However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
management approach in place.

PM_RUNTIME is required by OMAP based devices to handle clock management.
Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
to work with this driver.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 drivers/net/can/m_can/m_can.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index ea48e59..93e23f5 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -23,6 +23,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/iopoll.h>
 #include <linux/can/dev.h>
 
@@ -633,11 +634,15 @@ static int m_can_clk_start(struct m_can_priv *priv)
 	if (err)
 		clk_disable_unprepare(priv->hclk);
 
+	pm_runtime_get_sync(priv->device);
+
 	return err;
 }
 
 static void m_can_clk_stop(struct m_can_priv *priv)
 {
+	pm_runtime_put_sync(priv->device);
+
 	clk_disable_unprepare(priv->cclk);
 	clk_disable_unprepare(priv->hclk);
 }
@@ -1582,6 +1587,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
 	/* Enable clocks. Necessary to read Core Release in order to determine
 	 * M_CAN version
 	 */
+	pm_runtime_enable(&pdev->dev);
+
 	ret = clk_prepare_enable(hclk);
 	if (ret)
 		goto disable_hclk_ret;
@@ -1626,6 +1633,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
 	 */
 	tx_fifo_size = mram_config_vals[7];
 
+	pm_runtime_get_sync(&pdev->dev);
+
 	/* allocate the m_can device */
 	dev = alloc_m_can_dev(pdev, addr, tx_fifo_size);
 	if (!dev) {
@@ -1670,6 +1679,7 @@ static int m_can_plat_probe(struct platform_device *pdev)
 disable_hclk_ret:
 	clk_disable_unprepare(hclk);
 failed_ret:
+	pm_runtime_put_sync(&pdev->dev);
 	return ret;
 }
 
@@ -1726,6 +1736,9 @@ static int m_can_plat_remove(struct platform_device *pdev)
 	struct net_device *dev = platform_get_drvdata(pdev);
 
 	unregister_m_can_dev(dev);
+
+	pm_runtime_disable(&pdev->dev);
+
 	platform_set_drvdata(pdev, NULL);
 
 	free_m_can_dev(dev);
-- 
2.10.0

Re: [v2,3/3] can: m_can: Add PM Runtime

[Sekhar Nori]

+ OMAP mailing list

On Tuesday 25 July 2017 04:21 AM, Franklin Cooper wrote:
> Add support for PM Runtime which is the new way to handle managing clocks.
> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
> management approach in place.

Since hclk is the interface clock, I think at a minimum there needs to
be an assumption that pm_runtime_get_sync() will enable that clock and
make the module ready for access.

The only in-kernel user of this driver seems to be an atmel SoC. I am
ccing folks who added support for M_CAN on that SoC to see if hclk
management can be left to pm_runtime*() on their SoC.

If they are okay, I think its a good to atleast drop explicit hclk
enable disable in the driver. That way, we avoid double enable/disable
of interface clock (hclk).

> 
> PM_RUNTIME is required by OMAP based devices to handle clock management.
> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
> to work with this driver.
> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>

Thanks,
Sekhar

> ---
>  drivers/net/can/m_can/m_can.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index ea48e59..93e23f5 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -23,6 +23,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/iopoll.h>
>  #include <linux/can/dev.h>
>  
> @@ -633,11 +634,15 @@ static int m_can_clk_start(struct m_can_priv *priv)
>  	if (err)
>  		clk_disable_unprepare(priv->hclk);
>  
> +	pm_runtime_get_sync(priv->device);
> +
>  	return err;
>  }
>  
>  static void m_can_clk_stop(struct m_can_priv *priv)
>  {
> +	pm_runtime_put_sync(priv->device);
> +
>  	clk_disable_unprepare(priv->cclk);
>  	clk_disable_unprepare(priv->hclk);
>  }
> @@ -1582,6 +1587,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
>  	/* Enable clocks. Necessary to read Core Release in order to determine
>  	 * M_CAN version
>  	 */
> +	pm_runtime_enable(&pdev->dev);
> +
>  	ret = clk_prepare_enable(hclk);
>  	if (ret)
>  		goto disable_hclk_ret;
> @@ -1626,6 +1633,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
>  	 */
>  	tx_fifo_size = mram_config_vals[7];
>  
> +	pm_runtime_get_sync(&pdev->dev);
> +
>  	/* allocate the m_can device */
>  	dev = alloc_m_can_dev(pdev, addr, tx_fifo_size);
>  	if (!dev) {
> @@ -1670,6 +1679,7 @@ static int m_can_plat_probe(struct platform_device *pdev)
>  disable_hclk_ret:
>  	clk_disable_unprepare(hclk);
>  failed_ret:
> +	pm_runtime_put_sync(&pdev->dev);
>  	return ret;
>  }
>  
> @@ -1726,6 +1736,9 @@ static int m_can_plat_remove(struct platform_device *pdev)
>  	struct net_device *dev = platform_get_drvdata(pdev);
>  
>  	unregister_m_can_dev(dev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +
>  	platform_set_drvdata(pdev, NULL);
>  
>  	free_m_can_dev(dev);
> 

Re: [v2,3/3] can: m_can: Add PM Runtime

[Franklin S Cooper Jr]

On 08/24/2017 03:30 AM, Sekhar Nori wrote:
> + OMAP mailing list
> 
> On Tuesday 25 July 2017 04:21 AM, Franklin Cooper wrote:
>> Add support for PM Runtime which is the new way to handle managing clocks.
>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
>> management approach in place.
> 
> Since hclk is the interface clock, I think at a minimum there needs to
> be an assumption that pm_runtime_get_sync() will enable that clock and
> make the module ready for access.
> 
> The only in-kernel user of this driver seems to be an atmel SoC. I am
> ccing folks who added support for M_CAN on that SoC to see if hclk
> management can be left to pm_runtime*() on their SoC.
> 
> If they are okay, I think its a good to atleast drop explicit hclk
> enable disable in the driver. That way, we avoid double enable/disable
> of interface clock (hclk).

Wenyou,

Do you foresee this being a problem on your board? If not I can send a
v3 removing these clk_enable and clk_disable calls and it would be great
if you can test it out and provide your tested by.

> 
>>
>> PM_RUNTIME is required by OMAP based devices to handle clock management.
>> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
>> to work with this driver.
>>
>> Signed-off-by: Franklin S Cooper Jr <fcooper-l0cyMroinI0@public.gmane.org>
> 
> Thanks,
> Sekhar
> 
>> ---
>>  drivers/net/can/m_can/m_can.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index ea48e59..93e23f5 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -23,6 +23,7 @@
>>  #include <linux/of.h>
>>  #include <linux/of_device.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>>  #include <linux/iopoll.h>
>>  #include <linux/can/dev.h>
>>  
>> @@ -633,11 +634,15 @@ static int m_can_clk_start(struct m_can_priv *priv)
>>  	if (err)
>>  		clk_disable_unprepare(priv->hclk);
>>  
>> +	pm_runtime_get_sync(priv->device);
>> +
>>  	return err;
>>  }
>>  
>>  static void m_can_clk_stop(struct m_can_priv *priv)
>>  {
>> +	pm_runtime_put_sync(priv->device);
>> +
>>  	clk_disable_unprepare(priv->cclk);
>>  	clk_disable_unprepare(priv->hclk);
>>  }
>> @@ -1582,6 +1587,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>  	/* Enable clocks. Necessary to read Core Release in order to determine
>>  	 * M_CAN version
>>  	 */
>> +	pm_runtime_enable(&pdev->dev);
>> +
>>  	ret = clk_prepare_enable(hclk);
>>  	if (ret)
>>  		goto disable_hclk_ret;
>> @@ -1626,6 +1633,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>  	 */
>>  	tx_fifo_size = mram_config_vals[7];
>>  
>> +	pm_runtime_get_sync(&pdev->dev);
>> +
>>  	/* allocate the m_can device */
>>  	dev = alloc_m_can_dev(pdev, addr, tx_fifo_size);
>>  	if (!dev) {
>> @@ -1670,6 +1679,7 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>  disable_hclk_ret:
>>  	clk_disable_unprepare(hclk);
>>  failed_ret:
>> +	pm_runtime_put_sync(&pdev->dev);
>>  	return ret;
>>  }
>>  
>> @@ -1726,6 +1736,9 @@ static int m_can_plat_remove(struct platform_device *pdev)
>>  	struct net_device *dev = platform_get_drvdata(pdev);
>>  
>>  	unregister_m_can_dev(dev);
>> +
>> +	pm_runtime_disable(&pdev->dev);
>> +
>>  	platform_set_drvdata(pdev, NULL);
>>  
>>  	free_m_can_dev(dev);
>>
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v2,3/3] can: m_can: Add PM Runtime

[Yang, Wenyou]

Hi Franklin,


On 2017/9/21 8:36, Franklin S Cooper Jr wrote:
> On 08/24/2017 03:30 AM, Sekhar Nori wrote:
>> + OMAP mailing list
>>
>> On Tuesday 25 July 2017 04:21 AM, Franklin Cooper wrote:
>>> Add support for PM Runtime which is the new way to handle managing clocks.
>>> However, to avoid breaking SoCs not using PM_RUNTIME leave the old clk
>>> management approach in place.
>> Since hclk is the interface clock, I think at a minimum there needs to
>> be an assumption that pm_runtime_get_sync() will enable that clock and
>> make the module ready for access.
>>
>> The only in-kernel user of this driver seems to be an atmel SoC. I am
>> ccing folks who added support for M_CAN on that SoC to see if hclk
>> management can be left to pm_runtime*() on their SoC.
>>
>> If they are okay, I think its a good to atleast drop explicit hclk
>> enable disable in the driver. That way, we avoid double enable/disable
>> of interface clock (hclk).
> Wenyou,
>
> Do you foresee this being a problem on your board? If not I can send a
> v3 removing these clk_enable and clk_disable calls and it would be great
> if you can test it out and provide your tested by.
Please send a v3 patch.  I would like test it on my side.

>
>>> PM_RUNTIME is required by OMAP based devices to handle clock management.
>>> Therefore, this allows future Texas Instruments SoCs that have the MCAN IP
>>> to work with this driver.
>>>
>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>> Thanks,
>> Sekhar
>>
>>> ---
>>>   drivers/net/can/m_can/m_can.c | 13 +++++++++++++
>>>   1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>>> index ea48e59..93e23f5 100644
>>> --- a/drivers/net/can/m_can/m_can.c
>>> +++ b/drivers/net/can/m_can/m_can.c
>>> @@ -23,6 +23,7 @@
>>>   #include <linux/of.h>
>>>   #include <linux/of_device.h>
>>>   #include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>>   #include <linux/iopoll.h>
>>>   #include <linux/can/dev.h>
>>>   
>>> @@ -633,11 +634,15 @@ static int m_can_clk_start(struct m_can_priv *priv)
>>>   	if (err)
>>>   		clk_disable_unprepare(priv->hclk);
>>>   
>>> +	pm_runtime_get_sync(priv->device);
>>> +
>>>   	return err;
>>>   }
>>>   
>>>   static void m_can_clk_stop(struct m_can_priv *priv)
>>>   {
>>> +	pm_runtime_put_sync(priv->device);
>>> +
>>>   	clk_disable_unprepare(priv->cclk);
>>>   	clk_disable_unprepare(priv->hclk);
>>>   }
>>> @@ -1582,6 +1587,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>>   	/* Enable clocks. Necessary to read Core Release in order to determine
>>>   	 * M_CAN version
>>>   	 */
>>> +	pm_runtime_enable(&pdev->dev);
>>> +
>>>   	ret = clk_prepare_enable(hclk);
>>>   	if (ret)
>>>   		goto disable_hclk_ret;
>>> @@ -1626,6 +1633,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>>   	 */
>>>   	tx_fifo_size = mram_config_vals[7];
>>>   
>>> +	pm_runtime_get_sync(&pdev->dev);
>>> +
>>>   	/* allocate the m_can device */
>>>   	dev = alloc_m_can_dev(pdev, addr, tx_fifo_size);
>>>   	if (!dev) {
>>> @@ -1670,6 +1679,7 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>>   disable_hclk_ret:
>>>   	clk_disable_unprepare(hclk);
>>>   failed_ret:
>>> +	pm_runtime_put_sync(&pdev->dev);
>>>   	return ret;
>>>   }
>>>   
>>> @@ -1726,6 +1736,9 @@ static int m_can_plat_remove(struct platform_device *pdev)
>>>   	struct net_device *dev = platform_get_drvdata(pdev);
>>>   
>>>   	unregister_m_can_dev(dev);
>>> +
>>> +	pm_runtime_disable(&pdev->dev);
>>> +
>>>   	platform_set_drvdata(pdev, NULL);
>>>   
>>>   	free_m_can_dev(dev);
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Best Regards,
Wenyou Yang