request for 4.14-stable: 0d7412ed1f5d ("spi/bcm63xx-hspi: Enable the clock before calling clk_get_rate().")

request for 4.14-stable: 0d7412ed1f5d ("spi/bcm63xx-hspi: Enable the clock before calling clk_get_rate().")

[Sudip Mukherjee]

[-- Attachment #1: Type: text/plain, Size: 171 bytes --]

Hi Greg,

This was not marked for stable but seems it should be in stable.
And another commit which fixes it.
Please apply to your queue of 4.14-stable.

--
Regards
Sudip

[-- Attachment #2: 0001-spi-bcm63xx-hspi-Enable-the-clock-before-calling-clk.patch --]
[-- Type: text/x-diff, Size: 1975 bytes --]

>From 534cd63fbd8a163cd5e180018af3d278e6aeae47 Mon Sep 17 00:00:00 2001
From: Stefan Potyra <Stefan.Potyra@elektrobit.com>
Date: Thu, 26 Apr 2018 09:28:02 +0200
Subject: [PATCH 1/2] spi/bcm63xx-hspi: Enable the clock before calling
 clk_get_rate().

commit 0d7412ed1f5dc0858eb4f29650a8c9c5cce8b285 upstream

Enable the clock prior to calling clk_get_rate(), because clk_get_rate()
should only be called if the clock is enabled.

Additionally, prepare/enable the pll_clk before calling clk_get_rate()
for the same reason.

Found by Linux Driver Verification project (linuxtesting.org).

Fixes: 142168eba9dc ("spi: bcm63xx-hsspi: add bcm63xx HSSPI driver")
Signed-off-by: Stefan Potyra <Stefan.Potyra@elektrobit.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
---
 drivers/spi/spi-bcm63xx-hsspi.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-bcm63xx-hsspi.c b/drivers/spi/spi-bcm63xx-hsspi.c
index cbcba614b253..c23849f7aa7b 100644
--- a/drivers/spi/spi-bcm63xx-hsspi.c
+++ b/drivers/spi/spi-bcm63xx-hsspi.c
@@ -352,22 +352,31 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev)
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		return ret;
+
 	rate = clk_get_rate(clk);
 	if (!rate) {
 		struct clk *pll_clk = devm_clk_get(dev, "pll");
 
-		if (IS_ERR(pll_clk))
-			return PTR_ERR(pll_clk);
+		if (IS_ERR(pll_clk)) {
+			ret = PTR_ERR(pll_clk);
+			goto out_disable_clk;
+		}
+
+		ret = clk_prepare_enable(pll_clk);
+		if (ret)
+			goto out_disable_clk;
 
 		rate = clk_get_rate(pll_clk);
-		if (!rate)
-			return -EINVAL;
+		clk_disable_unprepare(pll_clk);
+		if (!rate) {
+			ret = -EINVAL;
+			goto out_disable_clk;
+		}
 	}
 
-	ret = clk_prepare_enable(clk);
-	if (ret)
-		return ret;
-
 	master = spi_alloc_master(&pdev->dev, sizeof(*bs));
 	if (!master) {
 		ret = -ENOMEM;
-- 
2.11.0


[-- Attachment #3: 0002-spi-bcm63xx-hsspi-keep-pll-clk-enabled.patch --]
[-- Type: text/x-diff, Size: 3310 bytes --]

>From 0b626628bbef51e16de187efbbfd021eb4210e1b Mon Sep 17 00:00:00 2001
From: Jonas Gorski <jonas.gorski@gmail.com>
Date: Tue, 28 Aug 2018 13:44:11 +0200
Subject: [PATCH 2/2] spi/bcm63xx-hsspi: keep pll clk enabled

commit 0fd85869c2a9c8723a98bc1f56a876e8383649f4 upstream

If the pll clock needs to be enabled to get its rate, it will also need
to be enabled to provide it. So ensure it is kept enabled through the
lifetime of the device.

Fixes: 0d7412ed1f5dc ("spi/bcm63xx-hspi: Enable the clock before calling clk_get_rate().")
Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
---
 drivers/spi/spi-bcm63xx-hsspi.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-bcm63xx-hsspi.c b/drivers/spi/spi-bcm63xx-hsspi.c
index c23849f7aa7b..9a06ffdb73b8 100644
--- a/drivers/spi/spi-bcm63xx-hsspi.c
+++ b/drivers/spi/spi-bcm63xx-hsspi.c
@@ -101,6 +101,7 @@ struct bcm63xx_hsspi {
 
 	struct platform_device *pdev;
 	struct clk *clk;
+	struct clk *pll_clk;
 	void __iomem *regs;
 	u8 __iomem *fifo;
 
@@ -332,7 +333,7 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev)
 	struct resource *res_mem;
 	void __iomem *regs;
 	struct device *dev = &pdev->dev;
-	struct clk *clk;
+	struct clk *clk, *pll_clk = NULL;
 	int irq, ret;
 	u32 reg, rate, num_cs = HSSPI_SPI_MAX_CS;
 
@@ -358,7 +359,7 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev)
 
 	rate = clk_get_rate(clk);
 	if (!rate) {
-		struct clk *pll_clk = devm_clk_get(dev, "pll");
+		pll_clk = devm_clk_get(dev, "pll");
 
 		if (IS_ERR(pll_clk)) {
 			ret = PTR_ERR(pll_clk);
@@ -373,19 +374,20 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev)
 		clk_disable_unprepare(pll_clk);
 		if (!rate) {
 			ret = -EINVAL;
-			goto out_disable_clk;
+			goto out_disable_pll_clk;
 		}
 	}
 
 	master = spi_alloc_master(&pdev->dev, sizeof(*bs));
 	if (!master) {
 		ret = -ENOMEM;
-		goto out_disable_clk;
+		goto out_disable_pll_clk;
 	}
 
 	bs = spi_master_get_devdata(master);
 	bs->pdev = pdev;
 	bs->clk = clk;
+	bs->pll_clk = pll_clk;
 	bs->regs = regs;
 	bs->speed_hz = rate;
 	bs->fifo = (u8 __iomem *)(bs->regs + HSSPI_FIFO_REG(0));
@@ -440,6 +442,8 @@ static int bcm63xx_hsspi_probe(struct platform_device *pdev)
 
 out_put_master:
 	spi_master_put(master);
+out_disable_pll_clk:
+	clk_disable_unprepare(pll_clk);
 out_disable_clk:
 	clk_disable_unprepare(clk);
 	return ret;
@@ -453,6 +457,7 @@ static int bcm63xx_hsspi_remove(struct platform_device *pdev)
 
 	/* reset the hardware and block queue progress */
 	__raw_writel(0, bs->regs + HSSPI_INT_MASK_REG);
+	clk_disable_unprepare(bs->pll_clk);
 	clk_disable_unprepare(bs->clk);
 
 	return 0;
@@ -465,6 +470,7 @@ static int bcm63xx_hsspi_suspend(struct device *dev)
 	struct bcm63xx_hsspi *bs = spi_master_get_devdata(master);
 
 	spi_master_suspend(master);
+	clk_disable_unprepare(bs->pll_clk);
 	clk_disable_unprepare(bs->clk);
 
 	return 0;
@@ -480,6 +486,12 @@ static int bcm63xx_hsspi_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	if (bs->pll_clk) {
+		ret = clk_prepare_enable(bs->pll_clk);
+		if (ret)
+			return ret;
+	}
+
 	spi_master_resume(master);
 
 	return 0;
-- 
2.11.0

Re: request for 4.14-stable: 0d7412ed1f5d ("spi/bcm63xx-hspi: Enable the clock before calling clk_get_rate().")

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On Sun, Nov 25, 2018 at 09:31:19PM +0000, Sudip Mukherjee wrote:

> This was not marked for stable but seems it should be in stable.
> And another commit which fixes it.
> Please apply to your queue of 4.14-stable.

This is more of an API correctness thing - some devices happen to be
able to get the clock rate without being enabled, I'm guessing this is
one of them otherwise it'd not work at all.  As ever it'd be good if you
could include some sort of description of why you think the backport is
needed...

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

Re: request for 4.14-stable: 0d7412ed1f5d ("spi/bcm63xx-hspi: Enable the clock before calling clk_get_rate().")

[Sudip Mukherjee]

Hi Mark,

On Mon, Nov 26, 2018 at 11:31 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Sun, Nov 25, 2018 at 09:31:19PM +0000, Sudip Mukherjee wrote:
>
> > This was not marked for stable but seems it should be in stable.
> > And another commit which fixes it.
> > Please apply to your queue of 4.14-stable.
>
> This is more of an API correctness thing - some devices happen to be
> able to get the clock rate without being enabled, I'm guessing this is
> one of them otherwise it'd not work at all.  As ever it'd be good if you
> could include some sort of description of why you think the backport is
> needed...

I was asking it to be included as it appeared from the commit message
that this was a fix for a problem where clk_get_rate() was not working
as the clock was not enabled.
If that is not the case then please ignore this mail and I will also
request Greg to ignore it as well.
Sorry for the noise.


-- 
Regards
Sudip