RE: [PATCH 1/1] clk: aspeed: modify some default clks are critical
From: Ryan Chen <ryan_chen@aspeedtech.com>
Date: 2020-10-08 02:34:02
Also in:
linux-aspeed, linux-clk, lkml
-----Original Message----- From: Joel Stanley <joel@jms.id.au> Sent: Wednesday, October 7, 2020 7:34 PM To: Ryan Chen <ryan_chen@aspeedtech.com> Cc: Jae Hyun Yoo <redacted>; Andrew Jeffery [off-list ref]; Michael Turquette [off-list ref]; Stephen Boyd [off-list ref]; linux-clk@vger.kernel.org; Linux ARM [off-list ref]; linux-aspeed [off-list ref]; Linux Kernel Mailing List [off-list ref]; BMC-SW [off-list ref] Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical On Tue, 29 Sep 2020 at 08:40, Ryan Chen [off-list ref] wrote:quoted
quoted
From: Joel Stanley <joel@jms.id.au> Sent: Tuesday, September 29, 2020 4:04 PM To: Ryan Chen <ryan_chen@aspeedtech.com>; Jae Hyun Yoo [off-list ref]; Andrew Jeffery [off-list ref] Cc: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd [off-list ref]; linux-clk@vger.kernel.org; Linux ARM [off-list ref]; linux-aspeed [off-list ref]; Linux Kernel Mailing List [off-list ref]; BMC-SW [off-list ref] Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical On Mon, 28 Sep 2020 at 07:01, Ryan Chen [off-list ref] wrote:quoted
In ASPEED SoC LCLK is LPC clock for all SuperIO device, UART1/UART2 are default for Host SuperIO UART device, eSPI clk for Host eSPI bus access eSPI slave channel, those clks can't be disable should keep default, otherwise will affect Host side accessSuperIO and SPI slave device.quoted
quoted
quoted
Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> --- drivers/clk/clk-aspeed.c | 8 ++++---- drivers/clk/clk-ast2600.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-)diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c index 411ff5fb2c07..d348c4fd3f9f 100644 --- a/drivers/clk/clk-aspeed.c +++ b/drivers/clk/clk-aspeed.c@@ -54,15 +54,15 @@ static const struct aspeed_gate_dataaspeed_gates[]= {quoted
[ASPEED_CLK_GATE_DCLK] = { 5, -1, "dclk-gate",NULL, CLK_IS_CRITICAL }, /* DAC */quoted
[ASPEED_CLK_GATE_REFCLK] = { 6, -1, "refclk-gate","clkin", CLK_IS_CRITICAL },quoted
[ASPEED_CLK_GATE_USBPORT2CLK] = { 7, 3, "usb-port2-gate",NULL, 0 }, /* USB2.0 Host port 2 */quoted
- [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate",NULL, 0 }, /* LPC */quoted
+ [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate",NULL, CLK_IS_CRITICAL }, /* LPC */quoted
[ASPEED_CLK_GATE_USBUHCICLK] = { 9, 15,"usb-uhci-gate",quoted
quoted
NULL, 0 }, /* USB1.1 (requires port 2 enabled) */quoted
[ASPEED_CLK_GATE_D1CLK] = { 10, 13, "d1clk-gate",NULL, 0 }, /* GFX CRT */quoted
[ASPEED_CLK_GATE_YCLK] = { 13, 4, "yclk-gate",NULL, 0 }, /* HAC */quoted
[ASPEED_CLK_GATE_USBPORT1CLK] = { 14, 14, "usb-port1-gate",NULL, 0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */quoted
- [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate","uart", 0 }, /* UART1 */quoted
- [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate","uart", 0 }, /* UART2 */quoted
+ [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate","uart", CLK_IS_CRITICAL }, /* UART1 */quoted
+ [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate","uart", CLK_IS_CRITICAL }, /* UART2 */quoted
[ASPEED_CLK_GATE_UART5CLK] = { 17, -1,"uart5clk-gate",quoted
quoted
"uart", 0 }, /* UART5 */quoted
- [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate",NULL, 0 }, /* eSPI */quoted
+ [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate",NULL, CLK_IS_CRITICAL }, /* eSPI */ This is fine for systems that have eSPI. For systems that do not use eSPI, the clocks are not "required". I was sent a similar patch by Jae some time ago: https://lore.kernel.org/openbmc/697a184b-ef99-a46e-bf98-4d339b3aafd8 @lin ux.intel.com/ Better is to associate drivers with these clocks, and those drivers will ensure they are left enabled. Alternatively, we will need to come up with a device tree binding to describe the hardware requirement that these clocks are left on.ASPEED BMC SoC have SuperIO device that default enable, even withoutBMC fw boot.quoted
Host can use SUART1/SUART2/GPIO.... That the reason even Linux kernel boot should not change the SoC default clk,that is the impact. Ok, that makes sense. Linux will not enable these clocks if a driver is loaded for them though, so we should load a driver for them. ESPI: We do not have an upstream driver for eSPI, but once you submit one this will solve the eSPI issue. LPC: The existing LPC driver will enable the clock, so by loading that this clock will be left enabled. UART1/UART2: This is harder, as the SUART mode means the BMC does not load a driver for these devices. We could add a property to the device tree to describe the clocks that must be left on?
That following the example, BMC is stop in u-boot stage, the eSPI and LPC UART1/2 can still work fine. But after boot into Linux, the eSPI and SUART1/2 can't work anymore, it is disappear. That is not consistence between U-boot and kernel. The reason is the same with AST SoC also have VGA device capability. And another point of view, clk framework provide CLK_IS_CRITICAL flag to support this, instead add more driver for this. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel