[PATCH 0/3] Stop managing the SP clock

[PATCH 0/3] Stop managing the SP clock

[Lubomir Rintel]

Hi,

Per discussion in [1] it seems that the kernel has no business managing
this clock: once the SP clock is disabled, it's not sufficient to just
enable it in order to bring the SP core back up.

Just let the firmware keep it enabled and don't expose it to drivers.

[1] https://lore.kernel.org/lkml/154783267051.169631.3197836544646625747@swboyd.mtv.corp.google.com/

The "secure processor" (SP) core runs the keyboard firmware on the
OLPC XO 1.75 laptop.

Lubo

[PATCH 1/3] Revert "Input: olpc_apsp - enable the SP clock"

[Lubomir Rintel]

Turns out this is not such a great idea. Once the SP clock is disabled,
it's not sufficient to just enable in order to bring the SP core back up.

It seems that the kernel has no business managing this clock. Just let
the firmware keep it enabled.

This reverts commit ed22cee91a88c47e564478b012fdbcb079653499.

Link: https://lore.kernel.org/lkml/154783267051.169631.3197836544646625747@swboyd.mtv.corp.google.com/
Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 .../devicetree/bindings/serio/olpc,ap-sp.txt       |  4 ----
 drivers/input/serio/olpc_apsp.c                    | 14 --------------
 2 files changed, 18 deletions(-)

diff --git a/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt b/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
index 36603419d6f8..0e72183f52bc 100644
--- a/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
+++ b/Documentation/devicetree/bindings/serio/olpc,ap-sp.txt
@@ -4,14 +4,10 @@ Required properties:
 - compatible : "olpc,ap-sp"
 - reg : base address and length of SoC's WTM registers
 - interrupts : SP-AP interrupt
-- clocks : phandle + clock-specifier for the clock that drives the WTM
-- clock-names:  should be "sp"
 
 Example:
 	ap-sp@d4290000 {
 		compatible = "olpc,ap-sp";
 		reg = <0xd4290000 0x1000>;
 		interrupts = <40>;
-		clocks = <&soc_clocks MMP2_CLK_SP>;
-		clock-names = "sp";
 	}
diff --git a/drivers/input/serio/olpc_apsp.c b/drivers/input/serio/olpc_apsp.c
index bae08226e3d9..a7cfab3db9ee 100644
--- a/drivers/input/serio/olpc_apsp.c
+++ b/drivers/input/serio/olpc_apsp.c
@@ -23,7 +23,6 @@
 #include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
-#include <linux/clk.h>
 
 /*
  * The OLPC XO-1.75 and XO-4 laptops do not have a hardware PS/2 controller.
@@ -75,7 +74,6 @@ struct olpc_apsp {
 	struct serio *kbio;
 	struct serio *padio;
 	void __iomem *base;
-	struct clk *clk;
 	int open_count;
 	int irq;
 };
@@ -148,17 +146,11 @@ static int olpc_apsp_open(struct serio *port)
 	struct olpc_apsp *priv = port->port_data;
 	unsigned int tmp;
 	unsigned long l;
-	int error;
 
 	if (priv->open_count++ == 0) {
-		error = clk_prepare_enable(priv->clk);
-		if (error)
-			return error;
-
 		l = readl(priv->base + COMMAND_FIFO_STATUS);
 		if (!(l & CMD_STS_MASK)) {
 			dev_err(priv->dev, "SP cannot accept commands.\n");
-			clk_disable_unprepare(priv->clk);
 			return -EIO;
 		}
 
@@ -179,8 +171,6 @@ static void olpc_apsp_close(struct serio *port)
 		/* Disable interrupt 0 */
 		tmp = readl(priv->base + PJ_INTERRUPT_MASK);
 		writel(tmp | INT_0, priv->base + PJ_INTERRUPT_MASK);
-
-		clk_disable_unprepare(priv->clk);
 	}
 }
 
@@ -208,10 +198,6 @@ static int olpc_apsp_probe(struct platform_device *pdev)
 	if (priv->irq < 0)
 		return priv->irq;
 
-	priv->clk = devm_clk_get(&pdev->dev, "sp");
-	if (IS_ERR(priv->clk))
-		return PTR_ERR(priv->clk);
-
 	/* KEYBOARD */
 	kb_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
 	if (!kb_serio)
-- 
2.20.1

[PATCH 2/3] Revert "clk: mmp2: add SP clock"

[Lubomir Rintel]

It seems that the kernel has no business managing this clock: once the SP
clock is disabled, it's not sufficient to just enable in order to bring the
SP core back up. Just let the firmware keep it enabled and don't expose it
to drivers.

This reverts commit fc27c2394d96fd19854b7e2d3f0e60df0d86fc90.

Link: https://lore.kernel.org/lkml/154783267051.169631.3197836544646625747@swboyd.mtv.corp.google.com/
Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/clk/mmp/clk-of-mmp2.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/clk/mmp/clk-of-mmp2.c b/drivers/clk/mmp/clk-of-mmp2.c
index 61fefc046ec5..d083b860f083 100644
--- a/drivers/clk/mmp/clk-of-mmp2.c
+++ b/drivers/clk/mmp/clk-of-mmp2.c
@@ -53,7 +53,6 @@
 #define APMU_DISP1	0x110
 #define APMU_CCIC0	0x50
 #define APMU_CCIC1	0xf4
-#define APMU_SP		0x68
 #define MPMU_UART_PLL	0x14
 
 struct mmp2_clk_unit {
@@ -210,8 +209,6 @@ static struct mmp_clk_mix_config ccic1_mix_config = {
 	.reg_info = DEFINE_MIX_REG_INFO(4, 16, 2, 6, 32),
 };
 
-static DEFINE_SPINLOCK(sp_lock);
-
 static struct mmp_param_mux_clk apmu_mux_clks[] = {
 	{MMP2_CLK_DISP0_MUX, "disp0_mux", disp_parent_names, ARRAY_SIZE(disp_parent_names), CLK_SET_RATE_PARENT, APMU_DISP0, 6, 2, 0, &disp0_lock},
 	{MMP2_CLK_DISP1_MUX, "disp1_mux", disp_parent_names, ARRAY_SIZE(disp_parent_names), CLK_SET_RATE_PARENT, APMU_DISP1, 6, 2, 0, &disp1_lock},
@@ -242,7 +239,6 @@ static struct mmp_param_gate_clk apmu_gate_clks[] = {
 	{MMP2_CLK_CCIC1, "ccic1_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x1b, 0x1b, 0x0, 0, &ccic1_lock},
 	{MMP2_CLK_CCIC1_PHY, "ccic1_phy_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x24, 0x24, 0x0, 0, &ccic1_lock},
 	{MMP2_CLK_CCIC1_SPHY, "ccic1_sphy_clk", "ccic1_sphy_div", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x300, 0x300, 0x0, 0, &ccic1_lock},
-	{MMP2_CLK_SP, "sp_clk", NULL, CLK_SET_RATE_PARENT, APMU_SP, 0x1b, 0x1b, 0x0, 0, &sp_lock},
 };
 
 static void mmp2_axi_periph_clk_init(struct mmp2_clk_unit *pxa_unit)
-- 
2.20.1

[PATCH 3/3] Revert "dt-bindings: marvell,mmp2: Add clock id for the SP clock"

[Lubomir Rintel]

It seems that the kernel has no business managing this clock: once the SP
clock is disabled, it's not sufficient to just enable it in order to bring
the SP core back up.

Pretty sure nothing ever used this and it's safe to remove.

This reverts commit e8a2c779141415105825e65a4715f1130bba61b1.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 include/dt-bindings/clock/marvell,mmp2.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/dt-bindings/clock/marvell,mmp2.h b/include/dt-bindings/clock/marvell,mmp2.h
index 7b24fc791146..228a5e234af0 100644
--- a/include/dt-bindings/clock/marvell,mmp2.h
+++ b/include/dt-bindings/clock/marvell,mmp2.h
@@ -71,7 +71,6 @@
 #define MMP2_CLK_CCIC1_MIX		117
 #define MMP2_CLK_CCIC1_PHY		118
 #define MMP2_CLK_CCIC1_SPHY		119
-#define MMP2_CLK_SP			120
 
 #define MMP2_NR_CLKS			200
 #endif
-- 
2.20.1

Re: [PATCH 1/3] Revert "Input: olpc_apsp - enable the SP clock"

[Dmitry Torokhov]

On Mon, Jan 21, 2019 at 07:22:54AM +0100, Lubomir Rintel wrote:
> Turns out this is not such a great idea. Once the SP clock is disabled,
> it's not sufficient to just enable in order to bring the SP core back up.
> 
> It seems that the kernel has no business managing this clock. Just let
> the firmware keep it enabled.
> 
> This reverts commit ed22cee91a88c47e564478b012fdbcb079653499.
> 
> Link: https://lore.kernel.org/lkml/154783267051.169631.3197836544646625747@swboyd.mtv.corp.google.com/
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

OK, as clock folks say it is a bad idea it must be so. Merge though clk
tree?

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks.

-- 
Dmitry

Re: [PATCH 1/3] Revert "Input: olpc_apsp - enable the SP clock"

[Stephen Boyd]

Quoting Dmitry Torokhov (2019-01-23 14:05:45)
> On Mon, Jan 21, 2019 at 07:22:54AM +0100, Lubomir Rintel wrote:
> > Turns out this is not such a great idea. Once the SP clock is disabled,
> > it's not sufficient to just enable in order to bring the SP core back up.
> > 
> > It seems that the kernel has no business managing this clock. Just let
> > the firmware keep it enabled.
> > 
> > This reverts commit ed22cee91a88c47e564478b012fdbcb079653499.
> > 
> > Link: https://lore.kernel.org/lkml/154783267051.169631.3197836544646625747@swboyd.mtv.corp.google.com/
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> 
> OK, as clock folks say it is a bad idea it must be so. Merge though clk
> tree?
> 
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 

Ok sure. I'll merge them through clk tree. I'll fast track it through
clk-fixes too.

Re: [PATCH 1/3] Revert "Input: olpc_apsp - enable the SP clock"

[Stephen Boyd]

Quoting Lubomir Rintel (2019-01-20 22:22:54)
> Turns out this is not such a great idea. Once the SP clock is disabled,
> it's not sufficient to just enable in order to bring the SP core back up.
> 
> It seems that the kernel has no business managing this clock. Just let
> the firmware keep it enabled.
> 
> This reverts commit ed22cee91a88c47e564478b012fdbcb079653499.
> 
> Link: https://lore.kernel.org/lkml/154783267051.169631.3197836544646625747@swboyd.mtv.corp.google.com/
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---

Applied to clk-fixes

Re: [PATCH 2/3] Revert "clk: mmp2: add SP clock"

[Stephen Boyd]

Quoting Lubomir Rintel (2019-01-20 22:22:55)
> It seems that the kernel has no business managing this clock: once the SP
> clock is disabled, it's not sufficient to just enable in order to bring the
> SP core back up. Just let the firmware keep it enabled and don't expose it
> to drivers.
> 
> This reverts commit fc27c2394d96fd19854b7e2d3f0e60df0d86fc90.
> 
> Link: https://lore.kernel.org/lkml/154783267051.169631.3197836544646625747@swboyd.mtv.corp.google.com/
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---

Applied to clk-fixes

Re: [PATCH 3/3] Revert "dt-bindings: marvell,mmp2: Add clock id for the SP clock"

[Stephen Boyd]

Quoting Lubomir Rintel (2019-01-20 22:22:56)
> It seems that the kernel has no business managing this clock: once the SP
> clock is disabled, it's not sufficient to just enable it in order to bring
> the SP core back up.
> 
> Pretty sure nothing ever used this and it's safe to remove.
> 
> This reverts commit e8a2c779141415105825e65a4715f1130bba61b1.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---

Applied to clk-fixes