diff mbox series

[v3,1/2] can: m_can: Move mram init to mcan device setup

Message ID 20221122105455.39294-2-vivek.2311@samsung.com
State New
Headers
Series can: mcan: Add MCAN support for FSD SoC |

Commit Message

Vivek Yadav Nov. 22, 2022, 10:54 a.m. UTC 
  When we try to access the mcan message ram addresses, hclk is
gated by any other drivers or disabled, because of that probe gets
failed.

Move the mram init functionality to mcan device setup called by
mcan class register from mcan probe function, by that time clocks
are enabled.

Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
Signed-off-by: Vivek Yadav <vivek.2311@samsung.com>
---
 drivers/net/can/m_can/m_can.c          |  4 ++--
 drivers/net/can/m_can/m_can.h          |  1 +
 drivers/net/can/m_can/m_can_platform.c | 21 +++++++++++++++++----
 3 files changed, 20 insertions(+), 6 deletions(-)

Comments

Marc Kleine-Budde Nov. 23, 2022, 10:41 p.m. UTC | #1 
On 22.11.2022 16:24:54, Vivek Yadav wrote:
> When we try to access the mcan message ram addresses, hclk is
> gated by any other drivers or disabled, because of that probe gets
> failed.
> 
> Move the mram init functionality to mcan device setup called by
> mcan class register from mcan probe function, by that time clocks
> are enabled.

Why not call the RAM init directly from m_can_chip_config()?

Marc
Vivek Yadav Nov. 24, 2022, 9:06 a.m. UTC | #2 
> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 24 November 2022 04:12
> To: Vivek Yadav <vivek.2311@samsung.com>
> Cc: rcsekar@samsung.com; krzysztof.kozlowski+dt@linaro.org;
> wg@grandegger.com; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; pankaj.dubey@samsung.com;
> ravi.patel@samsung.com; alim.akhtar@samsung.com; linux-fsd@tesla.com;
> robh+dt@kernel.org; linux-can@vger.kernel.org; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> samsung-soc@vger.kernel.org; devicetree@vger.kernel.org;
> aswani.reddy@samsung.com; sriranjani.p@samsung.com
> Subject: Re: [PATCH v3 1/2] can: m_can: Move mram init to mcan device
> setup
> 
> On 22.11.2022 16:24:54, Vivek Yadav wrote:
> > When we try to access the mcan message ram addresses, hclk is gated by
> > any other drivers or disabled, because of that probe gets failed.
> >
> > Move the mram init functionality to mcan device setup called by mcan
> > class register from mcan probe function, by that time clocks are
> > enabled.
> 
> Why not call the RAM init directly from m_can_chip_config()?
> 
m_can_chip_config function is called from m_can open.

Configuring RAM init every time we open the CAN instance is not needed, I think only once during the probe is enough. 

If message RAM init failed then fifo Transmit and receive will fail and there will be no communication. So there is no point to "open and Configure CAN chip".

From my understanding it's better to keep RAM init inside the probe and if there is a failure happened goes to CAN probe failure.
> Marc
Thanks for reviewing the patch.
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   |
> https://protect2.fireeye.com/v1/url?k=818c3690-e0f1dc13-818dbddf-
> 74fe48600158-a08b6a4bfa0b043e&q=1&e=315ed8d1-1645-4c16-b5e7-
> 2a250ae36941&u=https%3A%2F%2Fwww.pengutronix.de%2F  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
Marc Kleine-Budde Nov. 24, 2022, 2:54 p.m. UTC | #3 
On 24.11.2022 14:36:48, Vivek Yadav wrote:
> > Why not call the RAM init directly from m_can_chip_config()?
> > 
> m_can_chip_config function is called from m_can open.
> 
> Configuring RAM init every time we open the CAN instance is not
> needed, I think only once during the probe is enough.

That probably depends on you power management. If I add a regulator to
power the external tcan4x5x chip and power it up during open(), I need
to initialize the RAM.

> If message RAM init failed then fifo Transmit and receive will fail
> and there will be no communication. So there is no point to "open and
> Configure CAN chip".

For mmio devices the RAM init will probably not fail. There are return
values and error checking for the SPI attached devices. Where the SPI
communication will fail. However if this is problem, I assume the chip
will not be detected in the first place.

> From my understanding it's better to keep RAM init inside the probe
> and if there is a failure happened goes to CAN probe failure.

Marc
Vivek Yadav Dec. 1, 2022, 4:10 a.m. UTC | #4 
> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 24 November 2022 20:24
> To: Vivek Yadav <vivek.2311@samsung.com>
> Cc: rcsekar@samsung.com; krzysztof.kozlowski+dt@linaro.org;
> wg@grandegger.com; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; pankaj.dubey@samsung.com;
> ravi.patel@samsung.com; alim.akhtar@samsung.com; linux-fsd@tesla.com;
> robh+dt@kernel.org; linux-can@vger.kernel.org; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> samsung-soc@vger.kernel.org; devicetree@vger.kernel.org;
> aswani.reddy@samsung.com; sriranjani.p@samsung.com
> Subject: Re: RE: [PATCH v3 1/2] can: m_can: Move mram init to mcan device
> setup
> 
> On 24.11.2022 14:36:48, Vivek Yadav wrote:
> > > Why not call the RAM init directly from m_can_chip_config()?
> > >
> > m_can_chip_config function is called from m_can open.
> >
> > Configuring RAM init every time we open the CAN instance is not
> > needed, I think only once during the probe is enough.
> 
> That probably depends on you power management. If I add a regulator to
> power the external tcan4x5x chip and power it up during open(), I need to
> initialize the RAM.
> 
Thanks for the clarification,
There is one doubt for which I need clarity if I add ram init in m_can_chip_config.

In the current implementation, m_can_ram_init is added in the probe and m_can_class_resume function.
If I add the ram init function in chip_config which is getting called from m_can_start, then m_can_init_ram will be called two times, once in resume and next from m_can_start also.

Can we add ram init inside the m_can_open function itself?
Because it is independent of m_can resume functionality.

> > If message RAM init failed then fifo Transmit and receive will fail
> > and there will be no communication. So there is no point to "open and
> > Configure CAN chip".
> 
> For mmio devices the RAM init will probably not fail. There are return values
> and error checking for the SPI attached devices. Where the SPI
> communication will fail. However if this is problem, I assume the chip will not
> be detected in the first place.
> 
> > From my understanding it's better to keep RAM init inside the probe
> > and if there is a failure happened goes to CAN probe failure.
> 
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   |
> https://protect2.fireeye.com/v1/url?k=2053d9ab-7fc8e0b4-205252e4-
> 000babdfecba-a8c309c53e3358f5&q=1&e=c0cfd0e2-a422-4821-a49d-
> 113cfa4da9cb&u=https%3A%2F%2Fwww.pengutronix.de%2F  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
Marc Kleine-Budde Dec. 2, 2022, 3:04 p.m. UTC | #5 
On 01.12.2022 09:40:50, Vivek Yadav wrote:
> > That probably depends on you power management. If I add a regulator
> > to power the external tcan4x5x chip and power it up during open(), I
> > need to initialize the RAM.
> > 
> Thanks for the clarification,
>
> There is one doubt for which I need clarity if I add ram init in
> m_can_chip_config.
> 
> In the current implementation, m_can_ram_init is added in the probe
> and m_can_class_resume function.
>
> If I add the ram init function in chip_config which is getting called
> from m_can_start, then m_can_init_ram will be called two times, once
> in resume and next from m_can_start also.

As m_can_start() is called from resume, remove the direct call to
m_can_init_ram() from m_can_class_resume().

> Can we add ram init inside the m_can_open function itself? Because it
> is independent of m_can resume functionality.

See above.

mainline implementation:

m_can_class_resume()
        -> m_can_init_ram()

m_can_open()
        -> m_can_start()
        -> m_can_chip_config()
        -> ops->init
                m_can_init_ram() (for tcan only)


proposed:

m_can_class_resume()
        -> m_can_start()
        -> m_can_chip_config()
        -> m_can_init_ram()

m_can_open()
        -> m_can_start()
        -> m_can_chip_config()
        -> m_can_init_ram()

In mainline m_can_init_ram() is called for the tcan during open(). So if
you call m_can_init_ram() from m_can_chip_config(), remove it from the
tcan's tcan4x5x_init() functions, and from m_can_class_resume() it
should only be called once for open and once for resume.

regards,
Marc
diff mbox series

Patch

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index a776cab1a5a4..5956f0b4d5b1 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1516,9 +1516,9 @@  static int m_can_dev_setup(struct m_can_classdev *cdev)
 	}
 
 	if (cdev->ops->init)
-		cdev->ops->init(cdev);
+		err = cdev->ops->init(cdev);
 
-	return 0;
+	return err;
 }
 
 static void m_can_stop(struct net_device *dev)
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 401410022823..c6e77b93f4a2 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -93,6 +93,7 @@  struct m_can_classdev {
 	int is_peripheral;
 
 	struct mram_cfg mcfg[MRAM_CFG_NUM];
+	bool mram_cfg_flag;
 };
 
 struct m_can_classdev *m_can_class_allocate_dev(struct device *dev, int sizeof_priv);
diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
index b5a5bedb3116..1a65b0d256fb 100644
--- a/drivers/net/can/m_can/m_can_platform.c
+++ b/drivers/net/can/m_can/m_can_platform.c
@@ -67,11 +67,28 @@  static int iomap_write_fifo(struct m_can_classdev *cdev, int offset,
 	return 0;
 }
 
+static int plat_init(struct m_can_classdev *cdev)
+{
+	int ret = 0;
+
+	if (!cdev->mram_cfg_flag) {
+		/* Initialize mcan message ram */
+		ret = m_can_init_ram(cdev);
+		if (ret)
+			return ret;
+
+		cdev->mram_cfg_flag = true;
+	}
+
+	return 0;
+}
+
 static struct m_can_ops m_can_plat_ops = {
 	.read_reg = iomap_read_reg,
 	.write_reg = iomap_write_reg,
 	.write_fifo = iomap_write_fifo,
 	.read_fifo = iomap_read_fifo,
+	.init = plat_init,
 };
 
 static int m_can_plat_probe(struct platform_device *pdev)
@@ -140,10 +157,6 @@  static int m_can_plat_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, mcan_class);
 
-	ret = m_can_init_ram(mcan_class);
-	if (ret)
-		goto probe_fail;
-
 	pm_runtime_enable(mcan_class->dev);
 	ret = m_can_class_register(mcan_class);
 	if (ret)