[PATCH -next 4/4] ipmi: kcs_bmc_aspeed: add clock control logic

Jae Hyun Yoo jae.hyun.yoo at linux.intel.com
Wed Nov 3 03:35:31 AEDT 2021

On 11/1/2021 8:28 PM, Joel Stanley wrote:
> On Tue, 2 Nov 2021 at 03:16, ChiaWei Wang <chiawei_wang at aspeedtech.com> wrote:
>> Hi Jae,
>>> From: linux-arm-kernel <linux-arm-kernel-bounces at lists.infradead.org> On
>>> From: Jae Hyun Yoo <jae.hyun.yoo at 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.


Instead, I submitted these two changes to make it configurable through
device tree setting.


But these were not accepted too.

And recently, Samuel introduced a better and more generic way.


But it's not accepted yet either.


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.


More information about the Linux-aspeed mailing list