[1/1] spi: mediatek: add power control when set_cs
diff mbox series

Message ID 1571834322-1121-2-git-send-email-luhua.xu@mediatek.com
State New
Headers show
Series
  • Add spi power control when set cs
Related show

Commit Message

luhua xu Oct. 23, 2019, 12:38 p.m. UTC
From: "luhua.xu" <luhua.xu@mediatek.com>

Use runtime PM to power spi when set_cs
As set_cs may be called from interrupt context,
set runtime PM IRQ safe for spi.

Signed-off-by: luhua.xu <luhua.xu@mediatek.com>
---
 drivers/spi/spi-mt65xx.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Mark Brown Oct. 23, 2019, 3:11 p.m. UTC | #1
On Wed, Oct 23, 2019 at 08:38:42AM -0400, Luhua Xu wrote:
> From: "luhua.xu" <luhua.xu@mediatek.com>

> Use runtime PM to power spi when set_cs
> As set_cs may be called from interrupt context,
> set runtime PM IRQ safe for spi.

Why might we be trying to set the chip select state while the device is
runtime idle?  It seems like whatever is trying to set the chip select
should be dealing with this, not the chip select operation itself since
that's unlikely to be happening in isolation.
luhua xu Oct. 24, 2019, 6:25 a.m. UTC | #2
On Wed, 2019-10-23 at 16:11 +0100, Mark Brown wrote:
> On Wed, Oct 23, 2019 at 08:38:42AM -0400, Luhua Xu wrote:
> > From: "luhua.xu" <luhua.xu@mediatek.com>
> 
> > Use runtime PM to power spi when set_cs
> > As set_cs may be called from interrupt context,
> > set runtime PM IRQ safe for spi.
> 
> Why might we be trying to set the chip select state while the device is
> runtime idle?  It seems like whatever is trying to set the chip select
> should be dealing with this, not the chip select operation itself since
> that's unlikely to be happening in isolation.

Hi Mark,
Spi framework provideds  spi_setup() to modify spi settings for spi
device (maybe spi is runtime idle now), and this will call
spi_controller->set_cs() accessing registers.
Other spi_controller callbacks that need to access hardware registers,
are triggered by spi transfer. Spi framework will get and put runtime
power in __spi_pump_message().
luhua xu Oct. 24, 2019, 10:11 a.m. UTC | #3
On Thu, 2019-10-24 at 14:25 +0800, luhua xu wrote:
> On Wed, 2019-10-23 at 16:11 +0100, Mark Brown wrote:
> > On Wed, Oct 23, 2019 at 08:38:42AM -0400, Luhua Xu wrote:
> > > From: "luhua.xu" <luhua.xu@mediatek.com>
> > 
> > > Use runtime PM to power spi when set_cs
> > > As set_cs may be called from interrupt context,
> > > set runtime PM IRQ safe for spi.
> > 
> > Why might we be trying to set the chip select state while the device is
> > runtime idle?  It seems like whatever is trying to set the chip select
> > should be dealing with this, not the chip select operation itself since
> > that's unlikely to be happening in isolation.
> 
> Hi Mark,
> Spi framework provideds  spi_setup() to modify spi settings for spi
> device (maybe spi is runtime idle now), and this will call
> spi_controller->set_cs() accessing registers.
> Other spi_controller callbacks that need to access hardware registers,
> are triggered by spi transfer. Spi framework will get and put runtime
> power in __spi_pump_message().

hi Mark,
      If the spi slaver  who wants to set cs  need to  power spi
itself , this means we need to provide spi power on/off APIs  while spi
controller is runtime PM device.
      And spi slaver  needs to power spi itself when it wants to set cs,
and don't need to do so when do spi data transfer.

      I see  several case of spi driver with power control:
      (1)No clk control  in set_cs,  such as spi-cadence.c
      (2)Enable spi clk  in probe before the first time hitting reg,
and never power down. Such as  spi-sifive.c  spi-armada-3700.c
      (3)Enable spi clk  in  controller->set_cs  callback,  such as
spi-geni-qcom.c.    
      My patch works the same as (3).For mtk platform, without this
patch, if user do spi_setup(), cs setting cannot take effect, and reg
access violation occurs.
Mark Brown Oct. 24, 2019, 10:23 a.m. UTC | #4
On Thu, Oct 24, 2019 at 02:25:19PM +0800, luhua xu wrote:

> Spi framework provideds  spi_setup() to modify spi settings for spi
> device (maybe spi is runtime idle now), and this will call
> spi_controller->set_cs() accessing registers.

OK, so fix that so it takes the power at the setup() level then.
Mark Brown Oct. 24, 2019, 11:19 a.m. UTC | #5
On Thu, Oct 24, 2019 at 06:11:20PM +0800, luhua xu wrote:

>       (3)Enable spi clk  in  controller->set_cs  callback,  such as
> spi-geni-qcom.c.    
>       My patch works the same as (3).For mtk platform, without this
> patch, if user do spi_setup(), cs setting cannot take effect, and reg
> access violation occurs.

Other drivers having problems isn't a good reason to introduce more
drivers with problems.

Patch
diff mbox series

diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 6888a4d..039b67d 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -262,8 +262,16 @@  static int mtk_spi_prepare_message(struct spi_master *master,
 static void mtk_spi_set_cs(struct spi_device *spi, bool enable)
 {
 	u32 reg_val;
+	int ret;
 	struct mtk_spi *mdata = spi_master_get_devdata(spi->master);
 
+	ret = pm_runtime_get_sync(spi->master->dev.parent);
+	if (ret < 0) {
+		pm_runtime_put_noidle(spi->master->dev.parent);
+		dev_err(&spi->dev, "failed to power device(%d)\n", ret);
+		return;
+	}
+
 	reg_val = readl(mdata->base + SPI_CMD_REG);
 	if (!enable) {
 		reg_val |= SPI_CMD_PAUSE_EN;
@@ -274,6 +282,9 @@  static void mtk_spi_set_cs(struct spi_device *spi, bool enable)
 		mdata->state = MTK_SPI_IDLE;
 		mtk_spi_reset(mdata);
 	}
+
+	pm_runtime_mark_last_busy(spi->master->dev.parent);
+	pm_runtime_put_autosuspend(spi->master->dev.parent);
 }
 
 static void mtk_spi_prepare_transfer(struct spi_master *master,
@@ -749,6 +760,7 @@  static int mtk_spi_probe(struct platform_device *pdev)
 	clk_disable_unprepare(mdata->spi_clk);
 
 	pm_runtime_enable(&pdev->dev);
+	pm_runtime_irq_safe(&pdev->dev);
 
 	ret = devm_spi_register_master(&pdev->dev, master);
 	if (ret) {