[PATCH v4 1/4] can: m_can: move Message RAM initialization to function

[PATCH v4 1/4] can: m_can: move Message RAM initialization to function

[Quentin Schulz]

To avoid possible ECC/parity checksum errors when reading an
uninitialized buffer, the entire Message RAM is initialized when probing
the driver. This initialization is done in the same function reading the
Device Tree properties.

This patch moves the RAM initialization to a separate function so it can
be called separately from device initialization from Device Tree.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

v4:
  - remove unused variables from m_can_of_parse_mram,

 drivers/net/can/m_can/m_can.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index bf8fdaeb955e..5da1bdb202a3 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1489,11 +1489,23 @@ static int register_m_can_dev(struct net_device *dev)
 	return register_candev(dev);
 }
 
+static void m_can_init_ram(struct m_can_priv *priv)
+{
+	int end, i, start;
+
+	/* initialize the entire Message RAM in use to avoid possible
+	 * ECC/parity checksum errors when reading an uninitialized buffer
+	 */
+	start = priv->mcfg[MRAM_SIDF].off;
+	end = priv->mcfg[MRAM_TXB].off +
+		priv->mcfg[MRAM_TXB].num * TXB_ELEMENT_SIZE;
+	for (i = start; i < end; i += 4)
+		writel(0x0, priv->mram_base + i);
+}
+
 static void m_can_of_parse_mram(struct m_can_priv *priv,
 				const u32 *mram_config_vals)
 {
-	int i, start, end;
-
 	priv->mcfg[MRAM_SIDF].off = mram_config_vals[0];
 	priv->mcfg[MRAM_SIDF].num = mram_config_vals[1];
 	priv->mcfg[MRAM_XIDF].off = priv->mcfg[MRAM_SIDF].off +
@@ -1529,15 +1541,7 @@ static void m_can_of_parse_mram(struct m_can_priv *priv,
 		priv->mcfg[MRAM_TXE].off, priv->mcfg[MRAM_TXE].num,
 		priv->mcfg[MRAM_TXB].off, priv->mcfg[MRAM_TXB].num);
 
-	/* initialize the entire Message RAM in use to avoid possible
-	 * ECC/parity checksum errors when reading an uninitialized buffer
-	 */
-	start = priv->mcfg[MRAM_SIDF].off;
-	end = priv->mcfg[MRAM_TXB].off +
-		priv->mcfg[MRAM_TXB].num * TXB_ELEMENT_SIZE;
-	for (i = start; i < end; i += 4)
-		writel(0x0, priv->mram_base + i);
-
+	m_can_init_ram(priv);
 }
 
 static int m_can_plat_probe(struct platform_device *pdev)
-- 
2.11.0

[PATCH v4 2/4] can: m_can: make m_can_start and m_can_stop symmetric

[Quentin Schulz]

This moves clocks gating outside of the m_can_stop function as the
m_can_start function does not (and cannot, at least in current
implementation) ungate clocks. This way, both functions can now be used
symmetrically.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

added in v4

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

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 5da1bdb202a3..6115dede671e 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1324,9 +1324,6 @@ static void m_can_stop(struct net_device *dev)
 	/* disable all interrupts */
 	m_can_disable_all_interrupts(priv);
 
-	clk_disable_unprepare(priv->hclk);
-	clk_disable_unprepare(priv->cclk);
-
 	/* set the state as STOPPED */
 	priv->can.state = CAN_STATE_STOPPED;
 }
@@ -1338,6 +1335,8 @@ static int m_can_close(struct net_device *dev)
 	netif_stop_queue(dev);
 	napi_disable(&priv->napi);
 	m_can_stop(dev);
+	clk_disable_unprepare(priv->hclk);
+	clk_disable_unprepare(priv->cclk);
 	free_irq(dev->irq, dev);
 	close_candev(dev);
 	can_led_event(dev, CAN_LED_EVENT_STOP);
-- 
2.11.0

[PATCH v4 3/4] can: m_can: factorize clock gating and ungating

[Quentin Schulz]

This creates a function to ungate M_CAN clocks and another to gate the
same clocks, then swaps all gating/ungating code with their respective
function.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

added in v4

 drivers/net/can/m_can/m_can.c | 45 +++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 6115dede671e..653b304d7091 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -621,10 +621,8 @@ static int __m_can_get_berr_counter(const struct net_device *dev,
 	return 0;
 }
 
-static int m_can_get_berr_counter(const struct net_device *dev,
-				  struct can_berr_counter *bec)
+static int m_can_clk_start(struct m_can_priv *priv)
 {
-	struct m_can_priv *priv = netdev_priv(dev);
 	int err;
 
 	err = clk_prepare_enable(priv->hclk);
@@ -632,15 +630,31 @@ static int m_can_get_berr_counter(const struct net_device *dev,
 		return err;
 
 	err = clk_prepare_enable(priv->cclk);
-	if (err) {
+	if (err)
 		clk_disable_unprepare(priv->hclk);
-		return err;
-	}
 
-	__m_can_get_berr_counter(dev, bec);
+	return err;
+}
 
+static void m_can_clk_stop(struct m_can_priv *priv)
+{
 	clk_disable_unprepare(priv->cclk);
 	clk_disable_unprepare(priv->hclk);
+}
+
+static int m_can_get_berr_counter(const struct net_device *dev,
+				  struct can_berr_counter *bec)
+{
+	struct m_can_priv *priv = netdev_priv(dev);
+	int err;
+
+	err = m_can_clk_start(priv);
+	if (err)
+		return err;
+
+	__m_can_get_berr_counter(dev, bec);
+
+	m_can_clk_stop(priv);
 
 	return 0;
 }
@@ -1276,19 +1290,15 @@ static int m_can_open(struct net_device *dev)
 	struct m_can_priv *priv = netdev_priv(dev);
 	int err;
 
-	err = clk_prepare_enable(priv->hclk);
+	err = m_can_clk_start(priv);
 	if (err)
 		return err;
 
-	err = clk_prepare_enable(priv->cclk);
-	if (err)
-		goto exit_disable_hclk;
-
 	/* open the can device */
 	err = open_candev(dev);
 	if (err) {
 		netdev_err(dev, "failed to open can device\n");
-		goto exit_disable_cclk;
+		goto exit_disable_clks;
 	}
 
 	/* register interrupt handler */
@@ -1310,10 +1320,8 @@ static int m_can_open(struct net_device *dev)
 
 exit_irq_fail:
 	close_candev(dev);
-exit_disable_cclk:
-	clk_disable_unprepare(priv->cclk);
-exit_disable_hclk:
-	clk_disable_unprepare(priv->hclk);
+exit_disable_clks:
+	m_can_clk_stop(priv);
 	return err;
 }
 
@@ -1335,8 +1343,7 @@ static int m_can_close(struct net_device *dev)
 	netif_stop_queue(dev);
 	napi_disable(&priv->napi);
 	m_can_stop(dev);
-	clk_disable_unprepare(priv->hclk);
-	clk_disable_unprepare(priv->cclk);
+	m_can_clk_stop(priv);
 	free_irq(dev->irq, dev);
 	close_candev(dev);
 	can_led_event(dev, CAN_LED_EVENT_STOP);
-- 
2.11.0

[PATCH v4 4/4] can: m_can: add deep Suspend/Resume support

[Quentin Schulz]

This adds Power Management deep Suspend/Resume support for Bosch M_CAN
chip.

When entering deep sleep, the clocks are gated, the interrupts are
disabled. When resuming from deep sleep, the chip needs to be
reinitialized, the clocks ungated and the interrupts enabled.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

v4:
 - added a TODO comment for efficient runtime PM support,
 - used m_can_clk_start/stop functions added in v4 to have symmetric
 suspend and resume functions.

v3:
 - do not close/reopen the can interface (which was previously done when
 calling m_can_close), basically do the same routine as in probe but
 it does not close/open the can device,
 - update commit log,

v2:
 - fix erroneous commit log,

 drivers/net/can/m_can/m_can.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 653b304d7091..f4947a74b65f 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1668,6 +1668,8 @@ static int m_can_plat_probe(struct platform_device *pdev)
 	return ret;
 }
 
+/* TODO: runtime PM with power down or sleep mode  */
+
 static __maybe_unused int m_can_suspend(struct device *dev)
 {
 	struct net_device *ndev = dev_get_drvdata(dev);
@@ -1676,10 +1678,10 @@ static __maybe_unused int m_can_suspend(struct device *dev)
 	if (netif_running(ndev)) {
 		netif_stop_queue(ndev);
 		netif_device_detach(ndev);
+		m_can_stop(ndev);
+		m_can_clk_stop(priv);
 	}
 
-	/* TODO: enter low power */
-
 	priv->can.state = CAN_STATE_SLEEPING;
 
 	return 0;
@@ -1690,11 +1692,18 @@ static __maybe_unused int m_can_resume(struct device *dev)
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct m_can_priv *priv = netdev_priv(ndev);
 
-	/* TODO: exit low power */
+	m_can_init_ram(priv);
 
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	if (netif_running(ndev)) {
+		int ret;
+
+		ret = m_can_clk_start(priv);
+		if (ret)
+			return ret;
+
+		m_can_start(ndev);
 		netif_device_attach(ndev);
 		netif_start_queue(ndev);
 	}
-- 
2.11.0

Re: [PATCH v4 1/4] can: m_can: move Message RAM initialization to function

[Quentin Schulz]

Hi all,

On 05/05/2017 15:50, Quentin Schulz wrote:
> To avoid possible ECC/parity checksum errors when reading an
> uninitialized buffer, the entire Message RAM is initialized when probing
> the driver. This initialization is done in the same function reading the
> Device Tree properties.
> 
> This patch moves the RAM initialization to a separate function so it can
> be called separately from device initialization from Device Tree.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>

It's been a week since I sent this patch series. Any comments?

Thanks,
Quentin

> ---
> 
> v4:
>   - remove unused variables from m_can_of_parse_mram,
> 
>  drivers/net/can/m_can/m_can.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index bf8fdaeb955e..5da1bdb202a3 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1489,11 +1489,23 @@ static int register_m_can_dev(struct net_device *dev)
>  	return register_candev(dev);
>  }
>  
> +static void m_can_init_ram(struct m_can_priv *priv)
> +{
> +	int end, i, start;
> +
> +	/* initialize the entire Message RAM in use to avoid possible
> +	 * ECC/parity checksum errors when reading an uninitialized buffer
> +	 */
> +	start = priv->mcfg[MRAM_SIDF].off;
> +	end = priv->mcfg[MRAM_TXB].off +
> +		priv->mcfg[MRAM_TXB].num * TXB_ELEMENT_SIZE;
> +	for (i = start; i < end; i += 4)
> +		writel(0x0, priv->mram_base + i);
> +}
> +
>  static void m_can_of_parse_mram(struct m_can_priv *priv,
>  				const u32 *mram_config_vals)
>  {
> -	int i, start, end;
> -
>  	priv->mcfg[MRAM_SIDF].off = mram_config_vals[0];
>  	priv->mcfg[MRAM_SIDF].num = mram_config_vals[1];
>  	priv->mcfg[MRAM_XIDF].off = priv->mcfg[MRAM_SIDF].off +
> @@ -1529,15 +1541,7 @@ static void m_can_of_parse_mram(struct m_can_priv *priv,
>  		priv->mcfg[MRAM_TXE].off, priv->mcfg[MRAM_TXE].num,
>  		priv->mcfg[MRAM_TXB].off, priv->mcfg[MRAM_TXB].num);
>  
> -	/* initialize the entire Message RAM in use to avoid possible
> -	 * ECC/parity checksum errors when reading an uninitialized buffer
> -	 */
> -	start = priv->mcfg[MRAM_SIDF].off;
> -	end = priv->mcfg[MRAM_TXB].off +
> -		priv->mcfg[MRAM_TXB].num * TXB_ELEMENT_SIZE;
> -	for (i = start; i < end; i += 4)
> -		writel(0x0, priv->mram_base + i);
> -
> +	m_can_init_ram(priv);
>  }
>  
>  static int m_can_plat_probe(struct platform_device *pdev)
> 

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 1/4] can: m_can: move Message RAM initialization to function

[Marc Kleine-Budde]

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

On 05/12/2017 08:37 AM, Quentin Schulz wrote:
> Hi all,
> 
> On 05/05/2017 15:50, Quentin Schulz wrote:
>> To avoid possible ECC/parity checksum errors when reading an
>> uninitialized buffer, the entire Message RAM is initialized when probing
>> the driver. This initialization is done in the same function reading the
>> Device Tree properties.
>>
>> This patch moves the RAM initialization to a separate function so it can
>> be called separately from device initialization from Device Tree.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> 
> It's been a week since I sent this patch series. Any comments?

Looks good, added to linux-can-next.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


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

Re: [PATCH v4 1/4] can: m_can: move Message RAM initialization to function

[Oliver Hartkopp]

On 05/15/2017 06:50 AM, Marc Kleine-Budde wrote:
> On 05/12/2017 08:37 AM, Quentin Schulz wrote:
>> Hi all,
>>
>> On 05/05/2017 15:50, Quentin Schulz wrote:
>>> To avoid possible ECC/parity checksum errors when reading an
>>> uninitialized buffer, the entire Message RAM is initialized when probing
>>> the driver. This initialization is done in the same function reading the
>>> Device Tree properties.
>>>
>>> This patch moves the RAM initialization to a separate function so it can
>>> be called separately from device initialization from Device Tree.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>
>> It's been a week since I sent this patch series. Any comments?
>
> Looks good, added to linux-can-next.

Isn't this a fix for linux-can instead?

At least it would make no sense to me to have the upgraded M_CAN driver 
in Linux 4.12 without this fix.

Regards,
Oliver

Re: [PATCH v4 1/4] can: m_can: move Message RAM initialization to function

[Alexandre Belloni]

Hi,

On 15/05/2017 at 20:51:30 -0700, Oliver Hartkopp wrote:
> On 05/15/2017 06:50 AM, Marc Kleine-Budde wrote:
> > On 05/12/2017 08:37 AM, Quentin Schulz wrote:
> > > On 05/05/2017 15:50, Quentin Schulz wrote:
> > > > To avoid possible ECC/parity checksum errors when reading an
> > > > uninitialized buffer, the entire Message RAM is initialized when probing
> > > > the driver. This initialization is done in the same function reading the
> > > > Device Tree properties.
> > > > 
> > > > This patch moves the RAM initialization to a separate function so it can
> > > > be called separately from device initialization from Device Tree.
> > > > 
> > > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> > > 
> > > It's been a week since I sent this patch series. Any comments?
> > 
> > Looks good, added to linux-can-next.
> 
> Isn't this a fix for linux-can instead?
> 
> At least it would make no sense to me to have the upgraded M_CAN driver in
> Linux 4.12 without this fix.
> 

The related suspend mode on the sama5d2 is not present in v4.12 so I
think this can wait v4.13.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v4 1/4] can: m_can: move Message RAM initialization to function

[Oliver Hartkopp]

Hi Alexandre,

On 05/16/2017 01:00 AM, Alexandre Belloni wrote:
> Hi,
>
> On 15/05/2017 at 20:51:30 -0700, Oliver Hartkopp wrote:
>> On 05/15/2017 06:50 AM, Marc Kleine-Budde wrote:
>>> Looks good, added to linux-can-next.
>>
>> Isn't this a fix for linux-can instead?
>>
>> At least it would make no sense to me to have the upgraded M_CAN driver in
>> Linux 4.12 without this fix.
>>
>
> The related suspend mode on the sama5d2 is not present in v4.12 so I
> think this can wait v4.13.
>

Agreed!

Thanks for the explanation.

Regards,
Oliver