diff mbox series

[v2,1/2] misc: Add clock control logic into Aspeed LPC SNOOP driver

Message ID 20201208091748.1920-1-wangzhiqiang.bj@bytedance.com
State Accepted, archived
Headers show
Series [v2,1/2] misc: Add clock control logic into Aspeed LPC SNOOP driver | expand

Commit Message

John Wang Dec. 8, 2020, 9:17 a.m. UTC 
From: Jae Hyun Yoo <jae.hyun.yoo@intel.com>

If LPC SNOOP driver is registered ahead of lpc-ctrl module, LPC
SNOOP block will be enabled without heart beating of LCLK until
lpc-ctrl enables the LCLK. This issue causes improper handling on
host interrupts when the host sends interrupt in that time frame.
Then kernel eventually forcibly disables the interrupt with
dumping stack and printing a 'nobody cared this irq' message out.

To prevent this issue, all LPC sub-nodes should enable LCLK
individually so this patch adds clock control logic into the LPC
SNOOP driver.

Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc
chardev")

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@intel.com>
Signed-off-by: Vernon Mauery <vernon.mauery@linux.intel.com>
Signed-off-by: John Wang <wangzhiqiang.bj@bytedance.com>
---
v2:
  reword: Add fixes line
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 30 ++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

Comments

Joel Stanley Dec. 9, 2020, 2:17 a.m. UTC | #1 
On Tue, 8 Dec 2020 at 09:17, John Wang <wangzhiqiang.bj@bytedance.com> wrote:
>
> From: Jae Hyun Yoo <jae.hyun.yoo@intel.com>
>
> If LPC SNOOP driver is registered ahead of lpc-ctrl module, LPC
> SNOOP block will be enabled without heart beating of LCLK until
> lpc-ctrl enables the LCLK. This issue causes improper handling on
> host interrupts when the host sends interrupt in that time frame.
> Then kernel eventually forcibly disables the interrupt with
> dumping stack and printing a 'nobody cared this irq' message out.
>
> To prevent this issue, all LPC sub-nodes should enable LCLK
> individually so this patch adds clock control logic into the LPC
> SNOOP driver.
>
> Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc
> chardev")
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@intel.com>
> Signed-off-by: Vernon Mauery <vernon.mauery@linux.intel.com>
> Signed-off-by: John Wang <wangzhiqiang.bj@bytedance.com>

Reviewed-by: Joel Stanley <joel@jms.id.au>

Arnd, can you merge this for v5.11, or would you prefer me to do a pull request?

The device tree patch from this series also needs to be added.

Cheers,

Joel

> ---
> v2:
>   reword: Add fixes line
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 30 ++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index 682ba0eb4eba..20acac6342ef 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -11,6 +11,7 @@
>   */
>
>  #include <linux/bitops.h>
> +#include <linux/clk.h>
>  #include <linux/interrupt.h>
>  #include <linux/fs.h>
>  #include <linux/kfifo.h>
> @@ -67,6 +68,7 @@ struct aspeed_lpc_snoop_channel {
>  struct aspeed_lpc_snoop {
>         struct regmap           *regmap;
>         int                     irq;
> +       struct clk              *clk;
>         struct aspeed_lpc_snoop_channel chan[NUM_SNOOP_CHANNELS];
>  };
>
> @@ -282,22 +284,42 @@ static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
>                 return -ENODEV;
>         }
>
> +       lpc_snoop->clk = devm_clk_get(dev, NULL);
> +       if (IS_ERR(lpc_snoop->clk)) {
> +               rc = PTR_ERR(lpc_snoop->clk);
> +               if (rc != -EPROBE_DEFER)
> +                       dev_err(dev, "couldn't get clock\n");
> +               return rc;
> +       }
> +       rc = clk_prepare_enable(lpc_snoop->clk);
> +       if (rc) {
> +               dev_err(dev, "couldn't enable clock\n");
> +               return rc;
> +       }
> +
>         rc = aspeed_lpc_snoop_config_irq(lpc_snoop, pdev);
>         if (rc)
> -               return rc;
> +               goto err;
>
>         rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 0, port);
>         if (rc)
> -               return rc;
> +               goto err;
>
>         /* Configuration of 2nd snoop channel port is optional */
>         if (of_property_read_u32_index(dev->of_node, "snoop-ports",
>                                        1, &port) == 0) {
>                 rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 1, port);
> -               if (rc)
> +               if (rc) {
>                         aspeed_lpc_disable_snoop(lpc_snoop, 0);
> +                       goto err;
> +               }
>         }
>
> +       return 0;
> +
> +err:
> +       clk_disable_unprepare(lpc_snoop->clk);
> +
>         return rc;
>  }
>
> @@ -309,6 +331,8 @@ static int aspeed_lpc_snoop_remove(struct platform_device *pdev)
>         aspeed_lpc_disable_snoop(lpc_snoop, 0);
>         aspeed_lpc_disable_snoop(lpc_snoop, 1);
>
> +       clk_disable_unprepare(lpc_snoop->clk);
> +
>         return 0;
>  }
>
> --
> 2.25.1
>
Arnd Bergmann Dec. 9, 2020, 6:41 a.m. UTC | #2 
On Wed, Dec 9, 2020 at 3:17 AM Joel Stanley <joel@jms.id.au> wrote:
>
> On Tue, 8 Dec 2020 at 09:17, John Wang <wangzhiqiang.bj@bytedance.com> wrote:
> >
> > From: Jae Hyun Yoo <jae.hyun.yoo@intel.com>
> >
> > If LPC SNOOP driver is registered ahead of lpc-ctrl module, LPC
> > SNOOP block will be enabled without heart beating of LCLK until
> > lpc-ctrl enables the LCLK. This issue causes improper handling on
> > host interrupts when the host sends interrupt in that time frame.
> > Then kernel eventually forcibly disables the interrupt with
> > dumping stack and printing a 'nobody cared this irq' message out.
> >
> > To prevent this issue, all LPC sub-nodes should enable LCLK
> > individually so this patch adds clock control logic into the LPC
> > SNOOP driver.
> >
> > Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc
> > chardev")
> >
> > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@intel.com>
> > Signed-off-by: Vernon Mauery <vernon.mauery@linux.intel.com>
> > Signed-off-by: John Wang <wangzhiqiang.bj@bytedance.com>
>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
> Arnd, can you merge this for v5.11, or would you prefer me to do a pull request?
>
> The device tree patch from this series also needs to be added.

Sure, please forward the patches to soc@kernel.org so I can
grab them from patchwork.

      Arnd
Ryan Chen Jan. 6, 2021, 9:54 a.m. UTC | #3 
Hello John, Joel, Jae,
	For this should be set LCLK to be CRITICAL it will fix LPC related driver. (KCS/BT/SNOOP)
	I have send the patch before.	
	https://patchwork.ozlabs.org/project/linux-aspeed/patch/20200928070108.14040-2-ryan_chen@aspeedtech.com/

Hello Joel,
	Will you consider this patch? 
	Beside KCS/BT/SNOOP, UART1/UART2 will be most related issue for host side. 


> -----Original Message-----
> From: Linux-aspeed
> <linux-aspeed-bounces+ryan_chen=aspeedtech.com@lists.ozlabs.org> On
> Behalf Of John Wang
> Sent: Tuesday, December 8, 2020 5:18 PM
> To: xuxiaohan@bytedance.com; yulei.sh@bytedance.com
> Cc: Robert Lippert <rlippert@google.com>; moderated list:ARM/ASPEED
> MACHINE SUPPORT <linux-aspeed@lists.ozlabs.org>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Vernon Mauery
> <vernon.mauery@linux.intel.com>; open list <linux-kernel@vger.kernel.org>;
> Jae Hyun Yoo <jae.hyun.yoo@intel.com>; Patrick Venture
> <venture@google.com>; moderated list:ARM/ASPEED MACHINE SUPPORT
> <linux-arm-kernel@lists.infradead.org>
> Subject: [PATCH v2 1/2] misc: Add clock control logic into Aspeed LPC SNOOP
> driver
> 
> From: Jae Hyun Yoo <jae.hyun.yoo@intel.com>
> 
> If LPC SNOOP driver is registered ahead of lpc-ctrl module, LPC SNOOP block
> will be enabled without heart beating of LCLK until lpc-ctrl enables the LCLK.
> This issue causes improper handling on host interrupts when the host sends
> interrupt in that time frame.
> Then kernel eventually forcibly disables the interrupt with dumping stack and
> printing a 'nobody cared this irq' message out.
> 
> To prevent this issue, all LPC sub-nodes should enable LCLK individually so this
> patch adds clock control logic into the LPC SNOOP driver.
> 
> Fixes: 3772e5da4454 ("drivers/misc: Aspeed LPC snoop output using misc
> chardev")
> 
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@intel.com>
> Signed-off-by: Vernon Mauery <vernon.mauery@linux.intel.com>
> Signed-off-by: John Wang <wangzhiqiang.bj@bytedance.com>
> ---
> v2:
>   reword: Add fixes line
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 30 ++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index 682ba0eb4eba..20acac6342ef 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -11,6 +11,7 @@
>   */
> 
>  #include <linux/bitops.h>
> +#include <linux/clk.h>
>  #include <linux/interrupt.h>
>  #include <linux/fs.h>
>  #include <linux/kfifo.h>
> @@ -67,6 +68,7 @@ struct aspeed_lpc_snoop_channel {  struct
> aspeed_lpc_snoop {
>  	struct regmap		*regmap;
>  	int			irq;
> +	struct clk		*clk;
>  	struct aspeed_lpc_snoop_channel chan[NUM_SNOOP_CHANNELS];  };
> 
> @@ -282,22 +284,42 @@ static int aspeed_lpc_snoop_probe(struct
> platform_device *pdev)
>  		return -ENODEV;
>  	}
> 
> +	lpc_snoop->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(lpc_snoop->clk)) {
> +		rc = PTR_ERR(lpc_snoop->clk);
> +		if (rc != -EPROBE_DEFER)
> +			dev_err(dev, "couldn't get clock\n");
> +		return rc;
> +	}
> +	rc = clk_prepare_enable(lpc_snoop->clk);
> +	if (rc) {
> +		dev_err(dev, "couldn't enable clock\n");
> +		return rc;
> +	}
> +
>  	rc = aspeed_lpc_snoop_config_irq(lpc_snoop, pdev);
>  	if (rc)
> -		return rc;
> +		goto err;
> 
>  	rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 0, port);
>  	if (rc)
> -		return rc;
> +		goto err;
> 
>  	/* Configuration of 2nd snoop channel port is optional */
>  	if (of_property_read_u32_index(dev->of_node, "snoop-ports",
>  				       1, &port) == 0) {
>  		rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 1, port);
> -		if (rc)
> +		if (rc) {
>  			aspeed_lpc_disable_snoop(lpc_snoop, 0);
> +			goto err;
> +		}
>  	}
> 
> +	return 0;
> +
> +err:
> +	clk_disable_unprepare(lpc_snoop->clk);
> +
>  	return rc;
>  }
> 
> @@ -309,6 +331,8 @@ static int aspeed_lpc_snoop_remove(struct
> platform_device *pdev)
>  	aspeed_lpc_disable_snoop(lpc_snoop, 0);
>  	aspeed_lpc_disable_snoop(lpc_snoop, 1);
> 
> +	clk_disable_unprepare(lpc_snoop->clk);
> +
>  	return 0;
>  }
> 
> --
> 2.25.1
Arnd Bergmann Jan. 15, 2021, 5:04 p.m. UTC | #4 
On Wed, Jan 6, 2021 at 10:57 AM Ryan Chen <ryan_chen@aspeedtech.com> wrote:
>
> Hello John, Joel, Jae,
>         For this should be set LCLK to be CRITICAL it will fix LPC related driver. (KCS/BT/SNOOP)
>         I have send the patch before.
>         https://patchwork.ozlabs.org/project/linux-aspeed/patch/20200928070108.14040-2-ryan_chen@aspeedtech.com/
>
> Hello Joel,
>         Will you consider this patch?
>         Beside KCS/BT/SNOOP, UART1/UART2 will be most related issue for host side.

Sorry it did not make it into the merge window. The patch is still in
patchwork. I could just pick it up directly for v5.12, or wait for a
combined pull request with other work. Joel, please let me know
what you prefer.

        Arnd
Ryan Chen Jan. 16, 2021, 1:03 a.m. UTC | #5 
> -----Original Message-----
> From: Arnd Bergmann <arnd@kernel.org>
> Sent: Saturday, January 16, 2021 1:05 AM
> To: Ryan Chen <ryan_chen@aspeedtech.com>
> Cc: John Wang <wangzhiqiang.bj@bytedance.com>;
> xuxiaohan@bytedance.com; yulei.sh@bytedance.com; Robert Lippert
> <rlippert@google.com>; moderated list:ARM/ASPEED MACHINE SUPPORT
> <linux-aspeed@lists.ozlabs.org>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Vernon Mauery
> <vernon.mauery@linux.intel.com>; open list <linux-kernel@vger.kernel.org>;
> Jae Hyun Yoo <jae.hyun.yoo@intel.com>; Patrick Venture
> <venture@google.com>; moderated list:ARM/ASPEED MACHINE SUPPORT
> <linux-arm-kernel@lists.infradead.org>
> Subject: Re: [PATCH v2 1/2] misc: Add clock control logic into Aspeed LPC
> SNOOP driver
> 
> On Wed, Jan 6, 2021 at 10:57 AM Ryan Chen <ryan_chen@aspeedtech.com>
> wrote:
> >
> > Hello John, Joel, Jae,
> >         For this should be set LCLK to be CRITICAL it will fix LPC related
> driver. (KCS/BT/SNOOP)
> >         I have send the patch before.
> >
> > https://patchwork.ozlabs.org/project/linux-aspeed/patch/20200928070108
> > .14040-2-ryan_chen@aspeedtech.com/
> >
> > Hello Joel,
> >         Will you consider this patch?
> >         Beside KCS/BT/SNOOP, UART1/UART2 will be most related issue
> for host side.
> 
> Sorry it did not make it into the merge window. The patch is still in patchwork.
> I could just pick it up directly for v5.12, or wait for a combined pull request
> with other work. 

Hello Arnd,
Thanks your update.

>Joel, please let me know what you prefer.
> 
Hello Joel,
Could you help check on this patch? 
https://patchwork.ozlabs.org/project/linux-aspeed/patch/20200928070108.14040-2-ryan_chen@aspeedtech.com/
Arnd Bergmann Feb. 9, 2021, 11:27 p.m. UTC | #6 
On Sat, Jan 16, 2021 at 2:03 AM Ryan Chen <ryan_chen@aspeedtech.com> wrote:
> >
> > Sorry it did not make it into the merge window. The patch is still in patchwork.
> > I could just pick it up directly for v5.12, or wait for a combined pull request
> > with other work.
>
> Hello Arnd,
> Thanks your update.
>
> >Joel, please let me know what you prefer.
> >
> Hello Joel,
> Could you help check on this patch?
> https://patchwork.ozlabs.org/project/linux-aspeed/patch/20200928070108.14040-2-ryan_chen@aspeedtech.com/

Hi Joel,

I see there has been no new pull request for mach-aspeed in
v5.12. If you have any material at all, please send it as soon
as you can so I can pick it up this time.

As a reminder, the patch here has still not been merged, as I
never heard back from you.

       Arnd
Joel Stanley Feb. 10, 2021, 10:18 a.m. UTC | #7 
On Wed, 10 Feb 2021 at 01:43, Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Sat, Jan 16, 2021 at 2:03 AM Ryan Chen <ryan_chen@aspeedtech.com> wrote:
> > >
> > > Sorry it did not make it into the merge window. The patch is still in patchwork.
> > > I could just pick it up directly for v5.12, or wait for a combined pull request
> > > with other work.
> >
> > Hello Arnd,
> > Thanks your update.
> >
> > >Joel, please let me know what you prefer.
> > >
> > Hello Joel,
> > Could you help check on this patch?
> > https://patchwork.ozlabs.org/project/linux-aspeed/patch/20200928070108.14040-2-ryan_chen@aspeedtech.com/

Sure, I'll respond to that thread separately.

> Hi Joel,
>
> I see there has been no new pull request for mach-aspeed in
> v5.12. If you have any material at all, please send it as soon
> as you can so I can pick it up this time.

There are some patches that I have queued up. As you can see I have
been a bit behind this cycle.

I'll get a pull request to you today. Thanks for the reminder.

Cheers,

Joel

>
> As a reminder, the patch here has still not been merged, as I
> never heard back from you.
>
>        Arnd
diff mbox series

Patch

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index 682ba0eb4eba..20acac6342ef 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -11,6 +11,7 @@ 
  */
 
 #include <linux/bitops.h>
+#include <linux/clk.h>
 #include <linux/interrupt.h>
 #include <linux/fs.h>
 #include <linux/kfifo.h>
@@ -67,6 +68,7 @@  struct aspeed_lpc_snoop_channel {
 struct aspeed_lpc_snoop {
 	struct regmap		*regmap;
 	int			irq;
+	struct clk		*clk;
 	struct aspeed_lpc_snoop_channel chan[NUM_SNOOP_CHANNELS];
 };
 
@@ -282,22 +284,42 @@  static int aspeed_lpc_snoop_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	lpc_snoop->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(lpc_snoop->clk)) {
+		rc = PTR_ERR(lpc_snoop->clk);
+		if (rc != -EPROBE_DEFER)
+			dev_err(dev, "couldn't get clock\n");
+		return rc;
+	}
+	rc = clk_prepare_enable(lpc_snoop->clk);
+	if (rc) {
+		dev_err(dev, "couldn't enable clock\n");
+		return rc;
+	}
+
 	rc = aspeed_lpc_snoop_config_irq(lpc_snoop, pdev);
 	if (rc)
-		return rc;
+		goto err;
 
 	rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 0, port);
 	if (rc)
-		return rc;
+		goto err;
 
 	/* Configuration of 2nd snoop channel port is optional */
 	if (of_property_read_u32_index(dev->of_node, "snoop-ports",
 				       1, &port) == 0) {
 		rc = aspeed_lpc_enable_snoop(lpc_snoop, dev, 1, port);
-		if (rc)
+		if (rc) {
 			aspeed_lpc_disable_snoop(lpc_snoop, 0);
+			goto err;
+		}
 	}
 
+	return 0;
+
+err:
+	clk_disable_unprepare(lpc_snoop->clk);
+
 	return rc;
 }
 
@@ -309,6 +331,8 @@  static int aspeed_lpc_snoop_remove(struct platform_device *pdev)
 	aspeed_lpc_disable_snoop(lpc_snoop, 0);
 	aspeed_lpc_disable_snoop(lpc_snoop, 1);
 
+	clk_disable_unprepare(lpc_snoop->clk);
+
 	return 0;
 }