| Message ID | 20211101233751.49222-5-jae.hyun.yoo@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add LCLK control into Aspeed LPC sub drivers | expand |
Hi Jae, > From: linux-arm-kernel <linux-arm-kernel-bounces@lists.infradead.org> On > > From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> > > If LPC KCS driver is registered ahead of lpc-ctrl module, LPC KCS 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 > interrupts 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 drivers should enable LCLK individually so this > patch adds clock control logic into the LPC KCS driver. Have all LPC sub drivers could result in entire LPC block down if any of them disables the clock (e.g. driver unload). The LPC devices such as SIO can be used before kernel booting, even without any BMC firmware. Thereby, we recommend to make LCLK critical or guarded by protected clock instead of having all LPC sub drivers hold the LCLK control. The previous discussion for your reference: https://lkml.org/lkml/2020/9/28/153 Regards, Chiawei
On Tue, 2 Nov 2021 at 03:16, ChiaWei Wang <chiawei_wang@aspeedtech.com> wrote: > > Hi Jae, > > > From: linux-arm-kernel <linux-arm-kernel-bounces@lists.infradead.org> On > > > > From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> > > > > If LPC KCS driver is registered ahead of lpc-ctrl module, LPC KCS 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 > > interrupts 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 drivers should enable LCLK individually so this > > patch adds clock control logic into the LPC KCS driver. > > Have all LPC sub drivers could result in entire LPC block down if any of them disables the clock (e.g. driver unload). > The LPC devices such as SIO can be used before kernel booting, even without any BMC firmware. > Thereby, we recommend to make LCLK critical or guarded by protected clock instead of having all LPC sub drivers hold the LCLK control. > > The previous discussion for your reference: > https://lkml.org/lkml/2020/9/28/153 Please read the entire thread. The conclusion: https://lore.kernel.org/all/CACPK8XdBmkhZ8mcSFmDAFV8k7Qj7ajBL8TVKfK8c+5aneUMHZw@mail.gmail.com/ That is, for the devices that have a driver loaded can enable the clock. When they are unloaded, they will reduce the reference count until the last driver is unloaded. eg: https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L945 There was another fork to the thread, where we suggested that a protected clocks binding could be added: https://lore.kernel.org/all/160269577311.884498.8429245140509326318@swboyd.mtv.corp.google.com/ If you wish to use this mechanism for eg. SIO clocks, then I encourage Aspeed to submit a patch to do that. Cheers, Joel
On 11/1/2021 8:28 PM, Joel Stanley wrote: > On Tue, 2 Nov 2021 at 03:16, ChiaWei Wang <chiawei_wang@aspeedtech.com> wrote: >> >> Hi Jae, >> >>> From: linux-arm-kernel <linux-arm-kernel-bounces@lists.infradead.org> On >>> >>> From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> >>> >>> If LPC KCS driver is registered ahead of lpc-ctrl module, LPC KCS 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 >>> interrupts 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 drivers should enable LCLK individually so this >>> patch adds clock control logic into the LPC KCS driver. >> >> Have all LPC sub drivers could result in entire LPC block down if any of them disables the clock (e.g. driver unload). >> The LPC devices such as SIO can be used before kernel booting, even without any BMC firmware. >> Thereby, we recommend to make LCLK critical or guarded by protected clock instead of having all LPC sub drivers hold the LCLK control. >> >> The previous discussion for your reference: >> https://lkml.org/lkml/2020/9/28/153 > > Please read the entire thread. The conclusion: > > https://lore.kernel.org/all/CACPK8XdBmkhZ8mcSFmDAFV8k7Qj7ajBL8TVKfK8c+5aneUMHZw@mail.gmail.com/ > > That is, for the devices that have a driver loaded can enable the > clock. When they are unloaded, they will reduce the reference count > until the last driver is unloaded. eg: > > https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L945 > > There was another fork to the thread, where we suggested that a > protected clocks binding could be added: > > https://lore.kernel.org/all/160269577311.884498.8429245140509326318@swboyd.mtv.corp.google.com/ > > If you wish to use this mechanism for eg. SIO clocks, then I encourage > Aspeed to submit a patch to do that. We are revisiting the aged discussion. Thanks for bringing it back. I agree with Joel that a clock should be enabled only on systems that need the clock actually so it should be configurable by a device driver or through device tree setting, not by the static setting in clk-ast2600.c code. So that's the reason why I stopped upstreaming below change for making BCLK as a critical clock. https://www.spinics.net/lists/linux-clk/msg44836.html Instead, I submitted these two changes to make it configurable through device tree setting. https://lists.ozlabs.org/pipermail/linux-aspeed/2020-January/003394.html https://lists.ozlabs.org/pipermail/linux-aspeed/2020-January/003339.html But these were not accepted too. And recently, Samuel introduced a better and more generic way. https://lore.kernel.org/all/20200903040015.5627-2-samuel@sholland.org/ But it's not accepted yet either. Chiawei, Please refine the mechanism and submit a change to make SIO clocks configurable through device tree setting. I believe that we can keep this patch series even with the change, or it can be modified and adjusted if needed after the SIO clocks fix is accepted. Thanks, Jae
> From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> > Sent: Wednesday, November 3, 2021 12:36 AM > > On 11/1/2021 8:28 PM, Joel Stanley wrote: > > On Tue, 2 Nov 2021 at 03:16, ChiaWei Wang > <chiawei_wang@aspeedtech.com> wrote: > >> > >> Hi Jae, > >> > >>> From: linux-arm-kernel > >>> <linux-arm-kernel-bounces@lists.infradead.org> On > >>> > >>> From: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> > >>> > >>> If LPC KCS driver is registered ahead of lpc-ctrl module, LPC KCS > >>> 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 interrupts 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 drivers should enable LCLK > >>> individually so this patch adds clock control logic into the LPC KCS driver. > >> > >> Have all LPC sub drivers could result in entire LPC block down if any of them > disables the clock (e.g. driver unload). > >> The LPC devices such as SIO can be used before kernel booting, even > without any BMC firmware. > >> Thereby, we recommend to make LCLK critical or guarded by protected > clock instead of having all LPC sub drivers hold the LCLK control. > >> > >> The previous discussion for your reference: > >> https://lkml.org/lkml/2020/9/28/153 > > > > Please read the entire thread. The conclusion: > > > > > https://lore.kernel.org/all/CACPK8XdBmkhZ8mcSFmDAFV8k7Qj7ajBL8TVKfK8c > + > > 5aneUMHZw@mail.gmail.com/ > > > > That is, for the devices that have a driver loaded can enable the > > clock. When they are unloaded, they will reduce the reference count > > until the last driver is unloaded. eg: > > > > https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L945 > > > > There was another fork to the thread, where we suggested that a > > protected clocks binding could be added: > > > > > https://lore.kernel.org/all/160269577311.884498.8429245140509326318@sw > > boyd.mtv.corp.google.com/ > > > > If you wish to use this mechanism for eg. SIO clocks, then I encourage > > Aspeed to submit a patch to do that. > > We are revisiting the aged discussion. Thanks for bringing it back. > > I agree with Joel that a clock should be enabled only on systems that need the > clock actually so it should be configurable by a device driver or through device > tree setting, not by the static setting in clk-ast2600.c code. So that's the > reason why I stopped upstreaming below change for making BCLK as a critical > clock. > > https://www.spinics.net/lists/linux-clk/msg44836.html > > Instead, I submitted these two changes to make it configurable through device > tree setting. > > https://lists.ozlabs.org/pipermail/linux-aspeed/2020-January/003394.html > https://lists.ozlabs.org/pipermail/linux-aspeed/2020-January/003339.html > > But these were not accepted too. > > And recently, Samuel introduced a better and more generic way. > > https://lore.kernel.org/all/20200903040015.5627-2-samuel@sholland.org/ > > But it's not accepted yet either. > > > Chiawei, > > Please refine the mechanism and submit a change to make SIO clocks > configurable through device tree setting. I believe that we can keep this patch > series even with the change, or it can be modified and adjusted if needed after > the SIO clocks fix is accepted. Thanks for your feedback and the information shared. We will keep tracking these changes and construct a compatible patch for the SIO clocks. Regards, Chiawei
diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c index 92a37b33494c..00706472cc4d 100644 --- a/drivers/char/ipmi/kcs_bmc_aspeed.c +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c @@ -6,6 +6,7 @@ #define pr_fmt(fmt) "aspeed-kcs-bmc: " fmt #include <linux/atomic.h> +#include <linux/clk.h> #include <linux/errno.h> #include <linux/interrupt.h> #include <linux/io.h> @@ -126,6 +127,8 @@ struct aspeed_kcs_bmc { bool remove; struct timer_list timer; } obe; + + struct clk *clk; }; struct aspeed_kcs_of_ops { @@ -620,24 +623,37 @@ static int aspeed_kcs_probe(struct platform_device *pdev) return -ENODEV; } + priv->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(priv->clk)) { + rc = PTR_ERR(priv->clk); + if (rc != -EPROBE_DEFER) + dev_err(&pdev->dev, "Couldn't get clock\n"); + return rc; + } + rc = clk_prepare_enable(priv->clk); + if (rc) { + dev_err(&pdev->dev, "Couldn't enable clock\n"); + return rc; + } + spin_lock_init(&priv->obe.lock); priv->obe.remove = false; timer_setup(&priv->obe.timer, aspeed_kcs_check_obe, 0); rc = aspeed_kcs_set_address(kcs_bmc, addrs, nr_addrs); if (rc) - return rc; + goto err; /* Host to BMC IRQ */ rc = aspeed_kcs_config_downstream_irq(kcs_bmc, pdev); if (rc) - return rc; + goto err; /* BMC to Host IRQ */ if (have_upstream_irq) { rc = aspeed_kcs_config_upstream_irq(priv, upstream_irq[0], upstream_irq[1]); if (rc < 0) - return rc; + goto err; } else { priv->upstream_irq.mode = aspeed_kcs_irq_none; } @@ -650,13 +666,19 @@ static int aspeed_kcs_probe(struct platform_device *pdev) rc = kcs_bmc_add_device(&priv->kcs_bmc); if (rc) { dev_warn(&pdev->dev, "Failed to register channel %d: %d\n", kcs_bmc->channel, rc); - return rc; + goto err; } dev_info(&pdev->dev, "Initialised channel %d at 0x%x\n", kcs_bmc->channel, addrs[0]); return 0; + +err: + aspeed_kcs_enable_channel(kcs_bmc, false); + clk_disable_unprepare(priv->clk); + + return rc; } static int aspeed_kcs_remove(struct platform_device *pdev) @@ -664,6 +686,7 @@ static int aspeed_kcs_remove(struct platform_device *pdev) struct aspeed_kcs_bmc *priv = platform_get_drvdata(pdev); struct kcs_bmc_device *kcs_bmc = &priv->kcs_bmc; + clk_disable_unprepare(priv->clk); kcs_bmc_remove_device(kcs_bmc); aspeed_kcs_enable_channel(kcs_bmc, false);