| Message ID | 20221122105455.39294-2-vivek.2311@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | can: mcan: Add MCAN support for FSD SoC | expand |
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
> -----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 |
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
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 --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)