* [PATCH 0/1] Modify ASPEED SoC some default clks are critical @ 2020-09-28 7:01 Ryan Chen 2020-09-28 7:01 ` [PATCH 1/1] clk: aspeed: modify " Ryan Chen 0 siblings, 1 reply; 15+ messages in thread From: Ryan Chen @ 2020-09-28 7:01 UTC (permalink / raw) To: Michael Turquette, Stephen Boyd, Joel Stanley, Andrew Jeffery, linux-clk, linux-arm-kernel, linux-aspeed, linux-kernel, bmc-sw Cc: Ryan Chen This patch is modify for ASPEED SoC some default clks can't disable need keep default clk on. Ryan Chen (1): clk: aspeed: modify some default clks are critical drivers/clk/clk-aspeed.c | 8 ++++---- drivers/clk/clk-ast2600.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/1] clk: aspeed: modify some default clks are critical 2020-09-28 7:01 [PATCH 0/1] Modify ASPEED SoC some default clks are critical Ryan Chen @ 2020-09-28 7:01 ` Ryan Chen 2020-09-29 8:04 ` Joel Stanley 2020-10-14 2:50 ` Stephen Boyd 0 siblings, 2 replies; 15+ messages in thread From: Ryan Chen @ 2020-09-28 7:01 UTC (permalink / raw) To: Michael Turquette, Stephen Boyd, Joel Stanley, Andrew Jeffery, linux-clk, linux-arm-kernel, linux-aspeed, linux-kernel, bmc-sw Cc: Ryan Chen 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 access SuperIO and SPI slave device. 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_data aspeed_gates[] = { [ASPEED_CLK_GATE_DCLK] = { 5, -1, "dclk-gate", NULL, CLK_IS_CRITICAL }, /* DAC */ [ASPEED_CLK_GATE_REFCLK] = { 6, -1, "refclk-gate", "clkin", CLK_IS_CRITICAL }, [ASPEED_CLK_GATE_USBPORT2CLK] = { 7, 3, "usb-port2-gate", NULL, 0 }, /* USB2.0 Host port 2 */ - [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate", NULL, 0 }, /* LPC */ + [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate", NULL, CLK_IS_CRITICAL }, /* LPC */ [ASPEED_CLK_GATE_USBUHCICLK] = { 9, 15, "usb-uhci-gate", NULL, 0 }, /* USB1.1 (requires port 2 enabled) */ [ASPEED_CLK_GATE_D1CLK] = { 10, 13, "d1clk-gate", NULL, 0 }, /* GFX CRT */ [ASPEED_CLK_GATE_YCLK] = { 13, 4, "yclk-gate", NULL, 0 }, /* HAC */ [ASPEED_CLK_GATE_USBPORT1CLK] = { 14, 14, "usb-port1-gate", NULL, 0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */ - [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate", "uart", 0 }, /* UART1 */ - [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate", "uart", 0 }, /* UART2 */ + [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate", "uart", CLK_IS_CRITICAL }, /* UART1 */ + [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate", "uart", CLK_IS_CRITICAL }, /* UART2 */ [ASPEED_CLK_GATE_UART5CLK] = { 17, -1, "uart5clk-gate", "uart", 0 }, /* UART5 */ - [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate", NULL, 0 }, /* eSPI */ + [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate", NULL, CLK_IS_CRITICAL }, /* eSPI */ [ASPEED_CLK_GATE_MAC1CLK] = { 20, 11, "mac1clk-gate", "mac", 0 }, /* MAC1 */ [ASPEED_CLK_GATE_MAC2CLK] = { 21, 12, "mac2clk-gate", "mac", 0 }, /* MAC2 */ [ASPEED_CLK_GATE_RSACLK] = { 24, -1, "rsaclk-gate", NULL, 0 }, /* RSA */ diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c index bbacaccad554..6802a2d5bbe2 100644 --- a/drivers/clk/clk-ast2600.c +++ b/drivers/clk/clk-ast2600.c @@ -86,8 +86,8 @@ static const struct aspeed_gate_data aspeed_g6_gates[] = { /* Reserved 26 */ [ASPEED_CLK_GATE_EMMCCLK] = { 27, 16, "emmcclk-gate", NULL, 0 }, /* For card clk */ /* Reserved 28/29/30 */ - [ASPEED_CLK_GATE_LCLK] = { 32, 32, "lclk-gate", NULL, 0 }, /* LPC */ - [ASPEED_CLK_GATE_ESPICLK] = { 33, -1, "espiclk-gate", NULL, 0 }, /* eSPI */ + [ASPEED_CLK_GATE_LCLK] = { 32, 32, "lclk-gate", NULL, CLK_IS_CRITICAL }, /* LPC */ + [ASPEED_CLK_GATE_ESPICLK] = { 33, -1, "espiclk-gate", NULL, CLK_IS_CRITICAL }, /* eSPI */ [ASPEED_CLK_GATE_REF1CLK] = { 34, -1, "ref1clk-gate", "clkin", CLK_IS_CRITICAL }, /* Reserved 35 */ [ASPEED_CLK_GATE_SDCLK] = { 36, 56, "sdclk-gate", NULL, 0 }, /* SDIO/SD */ @@ -102,8 +102,8 @@ static const struct aspeed_gate_data aspeed_g6_gates[] = { [ASPEED_CLK_GATE_I3C5CLK] = { 45, 45, "i3c5clk-gate", NULL, 0 }, /* I3C5 */ [ASPEED_CLK_GATE_I3C6CLK] = { 46, 46, "i3c6clk-gate", NULL, 0 }, /* I3C6 */ [ASPEED_CLK_GATE_I3C7CLK] = { 47, 47, "i3c7clk-gate", NULL, 0 }, /* I3C7 */ - [ASPEED_CLK_GATE_UART1CLK] = { 48, -1, "uart1clk-gate", "uart", 0 }, /* UART1 */ - [ASPEED_CLK_GATE_UART2CLK] = { 49, -1, "uart2clk-gate", "uart", 0 }, /* UART2 */ + [ASPEED_CLK_GATE_UART1CLK] = { 48, -1, "uart1clk-gate", "uart", CLK_IS_CRITICAL }, /* UART1 */ + [ASPEED_CLK_GATE_UART2CLK] = { 49, -1, "uart2clk-gate", "uart", CLK_IS_CRITICAL }, /* UART2 */ [ASPEED_CLK_GATE_UART3CLK] = { 50, -1, "uart3clk-gate", "uart", 0 }, /* UART3 */ [ASPEED_CLK_GATE_UART4CLK] = { 51, -1, "uart4clk-gate", "uart", 0 }, /* UART4 */ [ASPEED_CLK_GATE_MAC3CLK] = { 52, 52, "mac3clk-gate", "mac34", 0 }, /* MAC3 */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical 2020-09-28 7:01 ` [PATCH 1/1] clk: aspeed: modify " Ryan Chen @ 2020-09-29 8:04 ` Joel Stanley 2020-09-29 8:37 ` Ryan Chen 2020-10-14 2:50 ` Stephen Boyd 1 sibling, 1 reply; 15+ messages in thread From: Joel Stanley @ 2020-09-29 8:04 UTC (permalink / raw) To: Ryan Chen, Jae Hyun Yoo, Andrew Jeffery Cc: Michael Turquette, Stephen Boyd, linux-clk, Linux ARM, linux-aspeed, Linux Kernel Mailing List, bmc-sw On Mon, 28 Sep 2020 at 07:01, Ryan Chen <ryan_chen@aspeedtech.com> wrote: > > 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 access SuperIO and SPI slave device. > > 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_data aspeed_gates[] = { > [ASPEED_CLK_GATE_DCLK] = { 5, -1, "dclk-gate", NULL, CLK_IS_CRITICAL }, /* DAC */ > [ASPEED_CLK_GATE_REFCLK] = { 6, -1, "refclk-gate", "clkin", CLK_IS_CRITICAL }, > [ASPEED_CLK_GATE_USBPORT2CLK] = { 7, 3, "usb-port2-gate", NULL, 0 }, /* USB2.0 Host port 2 */ > - [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate", NULL, 0 }, /* LPC */ > + [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate", NULL, CLK_IS_CRITICAL }, /* LPC */ > [ASPEED_CLK_GATE_USBUHCICLK] = { 9, 15, "usb-uhci-gate", NULL, 0 }, /* USB1.1 (requires port 2 enabled) */ > [ASPEED_CLK_GATE_D1CLK] = { 10, 13, "d1clk-gate", NULL, 0 }, /* GFX CRT */ > [ASPEED_CLK_GATE_YCLK] = { 13, 4, "yclk-gate", NULL, 0 }, /* HAC */ > [ASPEED_CLK_GATE_USBPORT1CLK] = { 14, 14, "usb-port1-gate", NULL, 0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */ > - [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate", "uart", 0 }, /* UART1 */ > - [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate", "uart", 0 }, /* UART2 */ > + [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate", "uart", CLK_IS_CRITICAL }, /* UART1 */ > + [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate", "uart", CLK_IS_CRITICAL }, /* UART2 */ > [ASPEED_CLK_GATE_UART5CLK] = { 17, -1, "uart5clk-gate", "uart", 0 }, /* UART5 */ > - [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate", NULL, 0 }, /* eSPI */ > + [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@linux.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. Cheers, Joel > [ASPEED_CLK_GATE_MAC1CLK] = { 20, 11, "mac1clk-gate", "mac", 0 }, /* MAC1 */ > [ASPEED_CLK_GATE_MAC2CLK] = { 21, 12, "mac2clk-gate", "mac", 0 }, /* MAC2 */ > [ASPEED_CLK_GATE_RSACLK] = { 24, -1, "rsaclk-gate", NULL, 0 }, /* RSA */ > diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c > index bbacaccad554..6802a2d5bbe2 100644 > --- a/drivers/clk/clk-ast2600.c > +++ b/drivers/clk/clk-ast2600.c > @@ -86,8 +86,8 @@ static const struct aspeed_gate_data aspeed_g6_gates[] = { > /* Reserved 26 */ > [ASPEED_CLK_GATE_EMMCCLK] = { 27, 16, "emmcclk-gate", NULL, 0 }, /* For card clk */ > /* Reserved 28/29/30 */ > - [ASPEED_CLK_GATE_LCLK] = { 32, 32, "lclk-gate", NULL, 0 }, /* LPC */ > - [ASPEED_CLK_GATE_ESPICLK] = { 33, -1, "espiclk-gate", NULL, 0 }, /* eSPI */ > + [ASPEED_CLK_GATE_LCLK] = { 32, 32, "lclk-gate", NULL, CLK_IS_CRITICAL }, /* LPC */ > + [ASPEED_CLK_GATE_ESPICLK] = { 33, -1, "espiclk-gate", NULL, CLK_IS_CRITICAL }, /* eSPI */ > [ASPEED_CLK_GATE_REF1CLK] = { 34, -1, "ref1clk-gate", "clkin", CLK_IS_CRITICAL }, > /* Reserved 35 */ > [ASPEED_CLK_GATE_SDCLK] = { 36, 56, "sdclk-gate", NULL, 0 }, /* SDIO/SD */ > @@ -102,8 +102,8 @@ static const struct aspeed_gate_data aspeed_g6_gates[] = { > [ASPEED_CLK_GATE_I3C5CLK] = { 45, 45, "i3c5clk-gate", NULL, 0 }, /* I3C5 */ > [ASPEED_CLK_GATE_I3C6CLK] = { 46, 46, "i3c6clk-gate", NULL, 0 }, /* I3C6 */ > [ASPEED_CLK_GATE_I3C7CLK] = { 47, 47, "i3c7clk-gate", NULL, 0 }, /* I3C7 */ > - [ASPEED_CLK_GATE_UART1CLK] = { 48, -1, "uart1clk-gate", "uart", 0 }, /* UART1 */ > - [ASPEED_CLK_GATE_UART2CLK] = { 49, -1, "uart2clk-gate", "uart", 0 }, /* UART2 */ > + [ASPEED_CLK_GATE_UART1CLK] = { 48, -1, "uart1clk-gate", "uart", CLK_IS_CRITICAL }, /* UART1 */ > + [ASPEED_CLK_GATE_UART2CLK] = { 49, -1, "uart2clk-gate", "uart", CLK_IS_CRITICAL }, /* UART2 */ > [ASPEED_CLK_GATE_UART3CLK] = { 50, -1, "uart3clk-gate", "uart", 0 }, /* UART3 */ > [ASPEED_CLK_GATE_UART4CLK] = { 51, -1, "uart4clk-gate", "uart", 0 }, /* UART4 */ > [ASPEED_CLK_GATE_MAC3CLK] = { 52, 52, "mac3clk-gate", "mac34", 0 }, /* MAC3 */ > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH 1/1] clk: aspeed: modify some default clks are critical 2020-09-29 8:04 ` Joel Stanley @ 2020-09-29 8:37 ` Ryan Chen 2020-10-07 11:34 ` Joel Stanley 0 siblings, 1 reply; 15+ messages in thread From: Ryan Chen @ 2020-09-29 8:37 UTC (permalink / raw) To: Joel Stanley, Jae Hyun Yoo, Andrew Jeffery Cc: Michael Turquette, Stephen Boyd, linux-clk, Linux ARM, linux-aspeed, Linux Kernel Mailing List, BMC-SW > 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 > <jae.hyun.yoo@linux.intel.com>; Andrew Jeffery <andrew@aj.id.au> > Cc: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd > <sboyd@kernel.org>; linux-clk@vger.kernel.org; Linux ARM > <linux-arm-kernel@lists.infradead.org>; linux-aspeed > <linux-aspeed@lists.ozlabs.org>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org>; BMC-SW <BMC-SW@aspeedtech.com> > Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical > > On Mon, 28 Sep 2020 at 07:01, Ryan Chen <ryan_chen@aspeedtech.com> > wrote: > > > > 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 access SuperIO and SPI slave device. > > > > 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_data aspeed_gates[] > = { > > [ASPEED_CLK_GATE_DCLK] = { 5, -1, "dclk-gate", > NULL, CLK_IS_CRITICAL }, /* DAC */ > > [ASPEED_CLK_GATE_REFCLK] = { 6, -1, "refclk-gate", > "clkin", CLK_IS_CRITICAL }, > > [ASPEED_CLK_GATE_USBPORT2CLK] = { 7, 3, "usb-port2-gate", > NULL, 0 }, /* USB2.0 Host port 2 */ > > - [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate", > NULL, 0 }, /* LPC */ > > + [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate", > NULL, CLK_IS_CRITICAL }, /* LPC */ > > [ASPEED_CLK_GATE_USBUHCICLK] = { 9, 15, "usb-uhci-gate", > NULL, 0 }, /* USB1.1 (requires port 2 enabled) */ > > [ASPEED_CLK_GATE_D1CLK] = { 10, 13, "d1clk-gate", > NULL, 0 }, /* GFX CRT */ > > [ASPEED_CLK_GATE_YCLK] = { 13, 4, "yclk-gate", > NULL, 0 }, /* HAC */ > > [ASPEED_CLK_GATE_USBPORT1CLK] = { 14, 14, "usb-port1-gate", > NULL, 0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */ > > - [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate", > "uart", 0 }, /* UART1 */ > > - [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate", > "uart", 0 }, /* UART2 */ > > + [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate", > "uart", CLK_IS_CRITICAL }, /* UART1 */ > > + [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate", > "uart", CLK_IS_CRITICAL }, /* UART2 */ > > [ASPEED_CLK_GATE_UART5CLK] = { 17, -1, "uart5clk-gate", > "uart", 0 }, /* UART5 */ > > - [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate", > NULL, 0 }, /* eSPI */ > > + [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 without BMC fw boot. Host can use SUART1/SUART2/GPIO.... That the reason even Linux kernel boot should not change the SoC default clk, that is the impact. Ryan > > > [ASPEED_CLK_GATE_MAC1CLK] = { 20, 11, "mac1clk-gate", > "mac", 0 }, /* MAC1 */ > > [ASPEED_CLK_GATE_MAC2CLK] = { 21, 12, "mac2clk-gate", > "mac", 0 }, /* MAC2 */ > > [ASPEED_CLK_GATE_RSACLK] = { 24, -1, "rsaclk-gate", > NULL, 0 }, /* RSA */ > > diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c > > index bbacaccad554..6802a2d5bbe2 100644 > > --- a/drivers/clk/clk-ast2600.c > > +++ b/drivers/clk/clk-ast2600.c > > @@ -86,8 +86,8 @@ static const struct aspeed_gate_data aspeed_g6_gates[] > = { > > /* Reserved 26 */ > > [ASPEED_CLK_GATE_EMMCCLK] = { 27, 16, > "emmcclk-gate", NULL, 0 }, /* For card clk */ > > /* Reserved 28/29/30 */ > > - [ASPEED_CLK_GATE_LCLK] = { 32, 32, "lclk-gate", > NULL, 0 }, /* LPC */ > > - [ASPEED_CLK_GATE_ESPICLK] = { 33, -1, "espiclk-gate", > NULL, 0 }, /* eSPI */ > > + [ASPEED_CLK_GATE_LCLK] = { 32, 32, "lclk-gate", > NULL, CLK_IS_CRITICAL }, /* LPC */ > > + [ASPEED_CLK_GATE_ESPICLK] = { 33, -1, "espiclk-gate", > NULL, CLK_IS_CRITICAL }, /* eSPI */ > > [ASPEED_CLK_GATE_REF1CLK] = { 34, -1, "ref1clk-gate", > "clkin", CLK_IS_CRITICAL }, > > /* Reserved 35 */ > > [ASPEED_CLK_GATE_SDCLK] = { 36, 56, "sdclk-gate", > NULL, 0 }, /* SDIO/SD */ > > @@ -102,8 +102,8 @@ static const struct aspeed_gate_data > aspeed_g6_gates[] = { > > [ASPEED_CLK_GATE_I3C5CLK] = { 45, 45, "i3c5clk-gate", > NULL, 0 }, /* I3C5 */ > > [ASPEED_CLK_GATE_I3C6CLK] = { 46, 46, "i3c6clk-gate", > NULL, 0 }, /* I3C6 */ > > [ASPEED_CLK_GATE_I3C7CLK] = { 47, 47, "i3c7clk-gate", > NULL, 0 }, /* I3C7 */ > > - [ASPEED_CLK_GATE_UART1CLK] = { 48, -1, "uart1clk-gate", > "uart", 0 }, /* UART1 */ > > - [ASPEED_CLK_GATE_UART2CLK] = { 49, -1, "uart2clk-gate", > "uart", 0 }, /* UART2 */ > > + [ASPEED_CLK_GATE_UART1CLK] = { 48, -1, "uart1clk-gate", > "uart", CLK_IS_CRITICAL }, /* UART1 */ > > + [ASPEED_CLK_GATE_UART2CLK] = { 49, -1, "uart2clk-gate", > "uart", CLK_IS_CRITICAL }, /* UART2 */ > > [ASPEED_CLK_GATE_UART3CLK] = { 50, -1, > "uart3clk-gate", "uart", 0 }, /* UART3 */ > > [ASPEED_CLK_GATE_UART4CLK] = { 51, -1, > "uart4clk-gate", "uart", 0 }, /* UART4 */ > > [ASPEED_CLK_GATE_MAC3CLK] = { 52, 52, > "mac3clk-gate", "mac34", 0 }, /* MAC3 */ > > -- > > 2.17.1 > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical 2020-09-29 8:37 ` Ryan Chen @ 2020-10-07 11:34 ` Joel Stanley 2020-10-08 2:33 ` Ryan Chen 0 siblings, 1 reply; 15+ messages in thread From: Joel Stanley @ 2020-10-07 11:34 UTC (permalink / raw) To: Ryan Chen Cc: Jae Hyun Yoo, Andrew Jeffery, Michael Turquette, Stephen Boyd, linux-clk, Linux ARM, linux-aspeed, Linux Kernel Mailing List, BMC-SW On Tue, 29 Sep 2020 at 08:40, Ryan Chen <ryan_chen@aspeedtech.com> wrote: > > > 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 > > <jae.hyun.yoo@linux.intel.com>; Andrew Jeffery <andrew@aj.id.au> > > Cc: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd > > <sboyd@kernel.org>; linux-clk@vger.kernel.org; Linux ARM > > <linux-arm-kernel@lists.infradead.org>; linux-aspeed > > <linux-aspeed@lists.ozlabs.org>; Linux Kernel Mailing List > > <linux-kernel@vger.kernel.org>; BMC-SW <BMC-SW@aspeedtech.com> > > Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical > > > > On Mon, 28 Sep 2020 at 07:01, Ryan Chen <ryan_chen@aspeedtech.com> > > wrote: > > > > > > 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 access SuperIO and SPI slave device. > > > > > > 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_data aspeed_gates[] > > = { > > > [ASPEED_CLK_GATE_DCLK] = { 5, -1, "dclk-gate", > > NULL, CLK_IS_CRITICAL }, /* DAC */ > > > [ASPEED_CLK_GATE_REFCLK] = { 6, -1, "refclk-gate", > > "clkin", CLK_IS_CRITICAL }, > > > [ASPEED_CLK_GATE_USBPORT2CLK] = { 7, 3, "usb-port2-gate", > > NULL, 0 }, /* USB2.0 Host port 2 */ > > > - [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate", > > NULL, 0 }, /* LPC */ > > > + [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate", > > NULL, CLK_IS_CRITICAL }, /* LPC */ > > > [ASPEED_CLK_GATE_USBUHCICLK] = { 9, 15, "usb-uhci-gate", > > NULL, 0 }, /* USB1.1 (requires port 2 enabled) */ > > > [ASPEED_CLK_GATE_D1CLK] = { 10, 13, "d1clk-gate", > > NULL, 0 }, /* GFX CRT */ > > > [ASPEED_CLK_GATE_YCLK] = { 13, 4, "yclk-gate", > > NULL, 0 }, /* HAC */ > > > [ASPEED_CLK_GATE_USBPORT1CLK] = { 14, 14, "usb-port1-gate", > > NULL, 0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */ > > > - [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate", > > "uart", 0 }, /* UART1 */ > > > - [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate", > > "uart", 0 }, /* UART2 */ > > > + [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate", > > "uart", CLK_IS_CRITICAL }, /* UART1 */ > > > + [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate", > > "uart", CLK_IS_CRITICAL }, /* UART2 */ > > > [ASPEED_CLK_GATE_UART5CLK] = { 17, -1, "uart5clk-gate", > > "uart", 0 }, /* UART5 */ > > > - [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate", > > NULL, 0 }, /* eSPI */ > > > + [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 without BMC fw boot. > 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? Cheers, Joel ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH 1/1] clk: aspeed: modify some default clks are critical 2020-10-07 11:34 ` Joel Stanley @ 2020-10-08 2:33 ` Ryan Chen 0 siblings, 0 replies; 15+ messages in thread From: Ryan Chen @ 2020-10-08 2:33 UTC (permalink / raw) To: Joel Stanley Cc: Jae Hyun Yoo, Andrew Jeffery, Michael Turquette, Stephen Boyd, linux-clk, Linux ARM, linux-aspeed, Linux Kernel Mailing List, BMC-SW > -----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 <jae.hyun.yoo@linux.intel.com>; Andrew Jeffery > <andrew@aj.id.au>; Michael Turquette <mturquette@baylibre.com>; Stephen > Boyd <sboyd@kernel.org>; linux-clk@vger.kernel.org; Linux ARM > <linux-arm-kernel@lists.infradead.org>; linux-aspeed > <linux-aspeed@lists.ozlabs.org>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org>; BMC-SW <BMC-SW@aspeedtech.com> > Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical > > On Tue, 29 Sep 2020 at 08:40, Ryan Chen <ryan_chen@aspeedtech.com> > wrote: > > > > > 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 > > > <jae.hyun.yoo@linux.intel.com>; Andrew Jeffery <andrew@aj.id.au> > > > Cc: Michael Turquette <mturquette@baylibre.com>; Stephen Boyd > > > <sboyd@kernel.org>; linux-clk@vger.kernel.org; Linux ARM > > > <linux-arm-kernel@lists.infradead.org>; linux-aspeed > > > <linux-aspeed@lists.ozlabs.org>; Linux Kernel Mailing List > > > <linux-kernel@vger.kernel.org>; BMC-SW <BMC-SW@aspeedtech.com> > > > Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are > > > critical > > > > > > On Mon, 28 Sep 2020 at 07:01, Ryan Chen <ryan_chen@aspeedtech.com> > > > wrote: > > > > > > > > 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 access > SuperIO and SPI slave device. > > > > > > > > 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_data > > > > aspeed_gates[] > > > = { > > > > [ASPEED_CLK_GATE_DCLK] = { 5, -1, "dclk-gate", > > > NULL, CLK_IS_CRITICAL }, /* DAC */ > > > > [ASPEED_CLK_GATE_REFCLK] = { 6, -1, "refclk-gate", > > > "clkin", CLK_IS_CRITICAL }, > > > > [ASPEED_CLK_GATE_USBPORT2CLK] = { 7, 3, > > > > "usb-port2-gate", > > > NULL, 0 }, /* USB2.0 Host port 2 */ > > > > - [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate", > > > NULL, 0 }, /* LPC */ > > > > + [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate", > > > NULL, CLK_IS_CRITICAL }, /* LPC */ > > > > [ASPEED_CLK_GATE_USBUHCICLK] = { 9, 15, > "usb-uhci-gate", > > > NULL, 0 }, /* USB1.1 (requires port 2 enabled) */ > > > > [ASPEED_CLK_GATE_D1CLK] = { 10, 13, "d1clk-gate", > > > NULL, 0 }, /* GFX CRT */ > > > > [ASPEED_CLK_GATE_YCLK] = { 13, 4, "yclk-gate", > > > NULL, 0 }, /* HAC */ > > > > [ASPEED_CLK_GATE_USBPORT1CLK] = { 14, 14, > > > > "usb-port1-gate", > > > NULL, 0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */ > > > > - [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate", > > > "uart", 0 }, /* UART1 */ > > > > - [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate", > > > "uart", 0 }, /* UART2 */ > > > > + [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate", > > > "uart", CLK_IS_CRITICAL }, /* UART1 */ > > > > + [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate", > > > "uart", CLK_IS_CRITICAL }, /* UART2 */ > > > > [ASPEED_CLK_GATE_UART5CLK] = { 17, -1, > "uart5clk-gate", > > > "uart", 0 }, /* UART5 */ > > > > - [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate", > > > NULL, 0 }, /* eSPI */ > > > > + [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 without > BMC fw boot. > > 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical 2020-09-28 7:01 ` [PATCH 1/1] clk: aspeed: modify " Ryan Chen 2020-09-29 8:04 ` Joel Stanley @ 2020-10-14 2:50 ` Stephen Boyd 2020-10-14 5:28 ` Joel Stanley 1 sibling, 1 reply; 15+ messages in thread From: Stephen Boyd @ 2020-10-14 2:50 UTC (permalink / raw) To: Andrew Jeffery, Joel Stanley, Michael Turquette, Ryan Chen, bmc-sw, linux-arm-kernel, linux-aspeed, linux-clk, linux-kernel Cc: Ryan Chen Quoting Ryan Chen (2020-09-28 00:01:08) > 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 access SuperIO and SPI slave device. > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > --- Is there resolution on this thread? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical 2020-10-14 2:50 ` Stephen Boyd @ 2020-10-14 5:28 ` Joel Stanley 2020-10-14 5:39 ` Ryan Chen 2020-10-14 17:16 ` Stephen Boyd 0 siblings, 2 replies; 15+ messages in thread From: Joel Stanley @ 2020-10-14 5:28 UTC (permalink / raw) To: Stephen Boyd Cc: Andrew Jeffery, Michael Turquette, Ryan Chen, BMC-SW, Linux ARM, linux-aspeed, linux-clk, Linux Kernel Mailing List On Wed, 14 Oct 2020 at 02:50, Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Ryan Chen (2020-09-28 00:01:08) > > 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 access SuperIO and SPI slave device. > > > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > > --- > > Is there resolution on this thread? Not yet. We have a system where the BMC (management controller) controls some clocks, but the peripherals that it's clocking are outside the BMC's control. In this case, the host processor us using some UARTs and what not independent of any code running on the BMC. Ryan wants to have them marked as critical so the BMC never powers them down. However, there are systems that don't use this part of the soc, so for those implementations they are not critical and Linux on the BMC can turn them off. Do you have any thoughts? Has anyone solved a similar problem already? Cheers, Joel ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH 1/1] clk: aspeed: modify some default clks are critical 2020-10-14 5:28 ` Joel Stanley @ 2020-10-14 5:39 ` Ryan Chen 2020-10-14 17:16 ` Stephen Boyd 1 sibling, 0 replies; 15+ messages in thread From: Ryan Chen @ 2020-10-14 5:39 UTC (permalink / raw) To: Joel Stanley, Stephen Boyd Cc: Andrew Jeffery, Michael Turquette, BMC-SW, Linux ARM, linux-aspeed, linux-clk, Linux Kernel Mailing List > -----Original Message----- > From: Joel Stanley <joel@jms.id.au> > Sent: Wednesday, October 14, 2020 1:28 PM > To: Stephen Boyd <sboyd@kernel.org> > Cc: Andrew Jeffery <andrew@aj.id.au>; Michael Turquette > <mturquette@baylibre.com>; Ryan Chen <ryan_chen@aspeedtech.com>; > BMC-SW <BMC-SW@aspeedtech.com>; Linux ARM > <linux-arm-kernel@lists.infradead.org>; linux-aspeed > <linux-aspeed@lists.ozlabs.org>; linux-clk@vger.kernel.org; Linux Kernel > Mailing List <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical > > On Wed, 14 Oct 2020 at 02:50, Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Ryan Chen (2020-09-28 00:01:08) > > > 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 access SuperIO and SPI slave device. > > > > > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > > > --- > > > > Is there resolution on this thread? > > Not yet. > > We have a system where the BMC (management controller) controls some > clocks, but the peripherals that it's clocking are outside the BMC's control. In > this case, the host processor us using some UARTs and what not independent of > any code running on the BMC. > > Ryan wants to have them marked as critical so the BMC never powers them > down. > > However, there are systems that don't use this part of the soc, so for those > implementations they are not critical and Linux on the BMC can turn them off. > Take an example, conflict thought about ASPEED_CLK_GATE_BCLK is CLK_IS_CRITICAL in clk-ast2600.c In my opinion, the driver should keep the SoC default clk setting. It is original chip feature. > Do you have any thoughts? Has anyone solved a similar problem already? > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical 2020-10-14 5:28 ` Joel Stanley 2020-10-14 5:39 ` Ryan Chen @ 2020-10-14 17:16 ` Stephen Boyd 2020-10-28 4:38 ` Joel Stanley 2020-10-29 2:25 ` Samuel Holland 1 sibling, 2 replies; 15+ messages in thread From: Stephen Boyd @ 2020-10-14 17:16 UTC (permalink / raw) To: Joel Stanley Cc: Andrew Jeffery, Michael Turquette, Ryan Chen, BMC-SW, Linux ARM, linux-aspeed, linux-clk, Linux Kernel Mailing List Quoting Joel Stanley (2020-10-13 22:28:00) > On Wed, 14 Oct 2020 at 02:50, Stephen Boyd <sboyd@kernel.org> wrote: > > > > Quoting Ryan Chen (2020-09-28 00:01:08) > > > 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 access SuperIO and SPI slave device. > > > > > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > > > --- > > > > Is there resolution on this thread? > > Not yet. > > We have a system where the BMC (management controller) controls some > clocks, but the peripherals that it's clocking are outside the BMC's > control. In this case, the host processor us using some UARTs and what > not independent of any code running on the BMC. > > Ryan wants to have them marked as critical so the BMC never powers them down. > > However, there are systems that don't use this part of the soc, so for > those implementations they are not critical and Linux on the BMC can > turn them off. > > Do you have any thoughts? Has anyone solved a similar problem already? > Is this critical clocks in DT? Where we want to have different DT for different device configurations to indicate that some clks should be marked critical so they're never turned off and other times they aren't so they're turned off? It also sounds sort of like the protected-clocks binding. Where you don't want to touch certain clks depending on the usage configuration of the SoC. There is a patch to make that generic that I haven't applied because it looks wrong at first glance[1]. Maybe not registering those clks to the framework on the configuration that Ryan has is good enough? [1] https://lore.kernel.org/r/20200903040015.5627-2-samuel@sholland.org ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical 2020-10-14 17:16 ` Stephen Boyd @ 2020-10-28 4:38 ` Joel Stanley 2020-10-29 2:25 ` Samuel Holland 1 sibling, 0 replies; 15+ messages in thread From: Joel Stanley @ 2020-10-28 4:38 UTC (permalink / raw) To: Stephen Boyd Cc: Andrew Jeffery, Michael Turquette, Ryan Chen, BMC-SW, Linux ARM, linux-aspeed, linux-clk, Linux Kernel Mailing List Thanks for the response Stephen. Sorry it's taken me a while to get back to you. On Wed, 14 Oct 2020 at 17:16, Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Joel Stanley (2020-10-13 22:28:00) > > On Wed, 14 Oct 2020 at 02:50, Stephen Boyd <sboyd@kernel.org> wrote: > > > > > > Quoting Ryan Chen (2020-09-28 00:01:08) > > > > 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 access SuperIO and SPI slave device. > > > > > > > > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > > > > --- > > > > > > Is there resolution on this thread? > > > > Not yet. > > > > We have a system where the BMC (management controller) controls some > > clocks, but the peripherals that it's clocking are outside the BMC's > > control. In this case, the host processor us using some UARTs and what > > not independent of any code running on the BMC. > > > > Ryan wants to have them marked as critical so the BMC never powers them down. > > > > However, there are systems that don't use this part of the soc, so for > > those implementations they are not critical and Linux on the BMC can > > turn them off. > > > > Do you have any thoughts? Has anyone solved a similar problem already? > > > > Is this critical clocks in DT? Where we want to have different DT for > different device configurations to indicate that some clks should be > marked critical so they're never turned off and other times they aren't > so they're turned off? Spot on. > It also sounds sort of like the protected-clocks binding. Where you > don't want to touch certain clks depending on the usage configuration of > the SoC. There is a patch to make that generic that I haven't applied > because it looks wrong at first glance[1]. That binding is exactly what I had in mind. I wasn't aware of it. The drawbacks outlined in the commit message do sound concerning. I take it we could avoid those drawbacks by having a driver-specific implementation of protected-clocks, like qcom does? > Maybe not registering those > clks to the framework on the configuration that Ryan has is good enough? I didn't quite follow here. Did you mean with protected-clocks, or using a different mechanism? Cheers, Joel > > [1] https://lore.kernel.org/r/20200903040015.5627-2-samuel@sholland.org ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical 2020-10-14 17:16 ` Stephen Boyd 2020-10-28 4:38 ` Joel Stanley @ 2020-10-29 2:25 ` Samuel Holland 2021-01-22 8:15 ` Ryan Chen 1 sibling, 1 reply; 15+ messages in thread From: Samuel Holland @ 2020-10-29 2:25 UTC (permalink / raw) To: Stephen Boyd, Joel Stanley Cc: Andrew Jeffery, Michael Turquette, Ryan Chen, BMC-SW, Linux ARM, linux-aspeed, linux-clk, Linux Kernel Mailing List Stephen, On 10/14/20 12:16 PM, Stephen Boyd wrote: > Quoting Joel Stanley (2020-10-13 22:28:00) >> On Wed, 14 Oct 2020 at 02:50, Stephen Boyd <sboyd@kernel.org> wrote: >>> >>> Quoting Ryan Chen (2020-09-28 00:01:08) >>>> 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 access SuperIO and SPI slave device. >>>> >>>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> >>>> --- >>> >>> Is there resolution on this thread? >> >> Not yet. >> >> We have a system where the BMC (management controller) controls some >> clocks, but the peripherals that it's clocking are outside the BMC's >> control. In this case, the host processor us using some UARTs and what >> not independent of any code running on the BMC. >> >> Ryan wants to have them marked as critical so the BMC never powers them down. >> >> However, there are systems that don't use this part of the soc, so for >> those implementations they are not critical and Linux on the BMC can >> turn them off. >> >> Do you have any thoughts? Has anyone solved a similar problem already? >> > > Is this critical clocks in DT? Where we want to have different DT for > different device configurations to indicate that some clks should be > marked critical so they're never turned off and other times they aren't > so they're turned off? > > It also sounds sort of like the protected-clocks binding. Where you > don't want to touch certain clks depending on the usage configuration of > the SoC. There is a patch to make that generic that I haven't applied > because it looks wrong at first glance[1]. Maybe not registering those > clks to the framework on the configuration that Ryan has is good enough? Could you please be more specific than the patch "looks wrong"? I'm more than happy to update the patch to address your concerns, but I cannot do that unless I know what your concerns are. Regards, Samuel > [1] https://lore.kernel.org/r/20200903040015.5627-2-samuel@sholland.org ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical 2020-10-29 2:25 ` Samuel Holland @ 2021-01-22 8:15 ` Ryan Chen 2021-01-25 0:47 ` Andrew Jeffery 0 siblings, 1 reply; 15+ messages in thread From: Ryan Chen @ 2021-01-22 8:15 UTC (permalink / raw) To: Samuel Holland, Stephen Boyd, Joel Stanley Cc: Andrew Jeffery, Michael Turquette, BMC-SW, Linux ARM, linux-aspeed, linux-clk, Linux Kernel Mailing List Hello, How about this patch progress? It does impact a lot of machine that when BMC boot at u-boot. SUART is work for Host. But after boot into kernel, due to the clk disabled. The SUART is not work for Host anymore. Regards, Ryan > -----Original Message----- > From: Samuel Holland <samuel@sholland.org> > Sent: Thursday, October 29, 2020 10:25 AM > To: Stephen Boyd <sboyd@kernel.org>; Joel Stanley <joel@jms.id.au> > Cc: Andrew Jeffery <andrew@aj.id.au>; Michael Turquette > <mturquette@baylibre.com>; Ryan Chen <ryan_chen@aspeedtech.com>; > BMC-SW <BMC-SW@aspeedtech.com>; Linux ARM > <linux-arm-kernel@lists.infradead.org>; linux-aspeed > <linux-aspeed@lists.ozlabs.org>; linux-clk@vger.kernel.org; Linux Kernel > Mailing List <linux-kernel@vger.kernel.org> > Subject: Re: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical > > Stephen, > > On 10/14/20 12:16 PM, Stephen Boyd wrote: > > Quoting Joel Stanley (2020-10-13 22:28:00) > >> On Wed, 14 Oct 2020 at 02:50, Stephen Boyd <sboyd@kernel.org> wrote: > >>> > >>> Quoting Ryan Chen (2020-09-28 00:01:08) > >>>> 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 access SuperIO and SPI slave > device. > >>>> > >>>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > >>>> --- > >>> > >>> Is there resolution on this thread? > >> > >> Not yet. > >> > >> We have a system where the BMC (management controller) controls some > >> clocks, but the peripherals that it's clocking are outside the BMC's > >> control. In this case, the host processor us using some UARTs and > >> what not independent of any code running on the BMC. > >> > >> Ryan wants to have them marked as critical so the BMC never powers them > down. > >> > >> However, there are systems that don't use this part of the soc, so > >> for those implementations they are not critical and Linux on the BMC > >> can turn them off. > >> > >> Do you have any thoughts? Has anyone solved a similar problem already? > >> > > > > Is this critical clocks in DT? Where we want to have different DT for > > different device configurations to indicate that some clks should be > > marked critical so they're never turned off and other times they > > aren't so they're turned off? > > > > It also sounds sort of like the protected-clocks binding. Where you > > don't want to touch certain clks depending on the usage configuration > > of the SoC. There is a patch to make that generic that I haven't > > applied because it looks wrong at first glance[1]. Maybe not > > registering those clks to the framework on the configuration that Ryan has is > good enough? > > Could you please be more specific than the patch "looks wrong"? I'm more > than happy to update the patch to address your concerns, but I cannot do that > unless I know what your concerns are. > > Regards, > Samuel > > > [1] > > https://lore.kernel.org/r/20200903040015.5627-2-samuel@sholland.org ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical 2021-01-22 8:15 ` Ryan Chen @ 2021-01-25 0:47 ` Andrew Jeffery 2021-02-01 7:16 ` Ryan Chen 0 siblings, 1 reply; 15+ messages in thread From: Andrew Jeffery @ 2021-01-25 0:47 UTC (permalink / raw) To: Ryan Chen, Samuel Holland, Stephen Boyd, Joel Stanley Cc: BMC-SW, linux-aspeed, Michael Turquette, Linux Kernel Mailing List, linux-clk, Linux ARM On Fri, 22 Jan 2021, at 18:45, Ryan Chen wrote: > Hello, > How about this patch progress? > It does impact a lot of machine that when BMC boot at u-boot. > SUART is work for Host. But after boot into kernel, due to the clk disabled. > The SUART is not work for Host anymore. Maybe it's worth taking Ryan's patch for now, and when the protected-clocks binding gets merged we can rip out the CLK_IS_CRITICAL flags and convert the Aspeed devicetrees to use protected-clocks instead? The only issue I see with that plan is it becomes ambiguous as to which clock each platform considers crititical/in-need-of-protection. Andrew > > Regards, > Ryan > > -----Original Message----- > > From: Samuel Holland <samuel@sholland.org> > > Sent: Thursday, October 29, 2020 10:25 AM > > To: Stephen Boyd <sboyd@kernel.org>; Joel Stanley <joel@jms.id.au> > > Cc: Andrew Jeffery <andrew@aj.id.au>; Michael Turquette > > <mturquette@baylibre.com>; Ryan Chen <ryan_chen@aspeedtech.com>; > > BMC-SW <BMC-SW@aspeedtech.com>; Linux ARM > > <linux-arm-kernel@lists.infradead.org>; linux-aspeed > > <linux-aspeed@lists.ozlabs.org>; linux-clk@vger.kernel.org; Linux Kernel > > Mailing List <linux-kernel@vger.kernel.org> > > Subject: Re: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical > > > > Stephen, > > > > On 10/14/20 12:16 PM, Stephen Boyd wrote: > > > Quoting Joel Stanley (2020-10-13 22:28:00) > > >> On Wed, 14 Oct 2020 at 02:50, Stephen Boyd <sboyd@kernel.org> wrote: > > >>> > > >>> Quoting Ryan Chen (2020-09-28 00:01:08) > > >>>> 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 access SuperIO and SPI slave > > device. > > >>>> > > >>>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > > >>>> --- > > >>> > > >>> Is there resolution on this thread? > > >> > > >> Not yet. > > >> > > >> We have a system where the BMC (management controller) controls some > > >> clocks, but the peripherals that it's clocking are outside the BMC's > > >> control. In this case, the host processor us using some UARTs and > > >> what not independent of any code running on the BMC. > > >> > > >> Ryan wants to have them marked as critical so the BMC never powers them > > down. > > >> > > >> However, there are systems that don't use this part of the soc, so > > >> for those implementations they are not critical and Linux on the BMC > > >> can turn them off. > > >> > > >> Do you have any thoughts? Has anyone solved a similar problem already? > > >> > > > > > > Is this critical clocks in DT? Where we want to have different DT for > > > different device configurations to indicate that some clks should be > > > marked critical so they're never turned off and other times they > > > aren't so they're turned off? > > > > > > It also sounds sort of like the protected-clocks binding. Where you > > > don't want to touch certain clks depending on the usage configuration > > > of the SoC. There is a patch to make that generic that I haven't > > > applied because it looks wrong at first glance[1]. Maybe not > > > registering those clks to the framework on the configuration that Ryan has is > > good enough? > > > > Could you please be more specific than the patch "looks wrong"? I'm more > > than happy to update the patch to address your concerns, but I cannot do that > > unless I know what your concerns are. > > > > Regards, > > Samuel > > > > > [1] > > > https://lore.kernel.org/r/20200903040015.5627-2-samuel@sholland.org > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH 1/1] clk: aspeed: modify some default clks are critical 2021-01-25 0:47 ` Andrew Jeffery @ 2021-02-01 7:16 ` Ryan Chen 0 siblings, 0 replies; 15+ messages in thread From: Ryan Chen @ 2021-02-01 7:16 UTC (permalink / raw) To: Andrew Jeffery, Samuel Holland, Stephen Boyd, Joel Stanley Cc: BMC-SW, linux-aspeed, Michael Turquette, Linux Kernel Mailing List, linux-clk, Linux ARM > -----Original Message----- > From: Andrew Jeffery <andrew@aj.id.au> > Sent: Monday, January 25, 2021 8:47 AM > To: Ryan Chen <ryan_chen@aspeedtech.com>; Samuel Holland > <samuel@sholland.org>; Stephen Boyd <sboyd@kernel.org>; Joel Stanley > <joel@jms.id.au> > Cc: BMC-SW <BMC-SW@aspeedtech.com>; linux-aspeed > <linux-aspeed@lists.ozlabs.org>; Michael Turquette > <mturquette@baylibre.com>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org>; linux-clk@vger.kernel.org; Linux ARM > <linux-arm-kernel@lists.infradead.org> > Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical > > > > On Fri, 22 Jan 2021, at 18:45, Ryan Chen wrote: > > Hello, > > How about this patch progress? > > It does impact a lot of machine that when BMC boot at u-boot. > > SUART is work for Host. But after boot into kernel, due to the clk disabled. > > The SUART is not work for Host anymore. > > Maybe it's worth taking Ryan's patch for now, and when the protected-clocks > binding gets merged we can rip out the CLK_IS_CRITICAL flags and convert the > Aspeed devicetrees to use protected-clocks instead? > > The only issue I see with that plan is it becomes ambiguous as to which clock > each platform considers crititical/in-need-of-protection. > Hello Joel, Will you take this patch? Or you have another approach I may modify for it. Regards, Ryan > > > -----Original Message----- > > > From: Samuel Holland <samuel@sholland.org> > > > Sent: Thursday, October 29, 2020 10:25 AM > > > To: Stephen Boyd <sboyd@kernel.org>; Joel Stanley <joel@jms.id.au> > > > Cc: Andrew Jeffery <andrew@aj.id.au>; Michael Turquette > > > <mturquette@baylibre.com>; Ryan Chen <ryan_chen@aspeedtech.com>; > > > BMC-SW <BMC-SW@aspeedtech.com>; Linux ARM > > > <linux-arm-kernel@lists.infradead.org>; linux-aspeed > > > <linux-aspeed@lists.ozlabs.org>; linux-clk@vger.kernel.org; Linux > > > Kernel Mailing List <linux-kernel@vger.kernel.org> > > > Subject: Re: Re: [PATCH 1/1] clk: aspeed: modify some default clks > > > are critical > > > > > > Stephen, > > > > > > On 10/14/20 12:16 PM, Stephen Boyd wrote: > > > > Quoting Joel Stanley (2020-10-13 22:28:00) > > > >> On Wed, 14 Oct 2020 at 02:50, Stephen Boyd <sboyd@kernel.org> > wrote: > > > >>> > > > >>> Quoting Ryan Chen (2020-09-28 00:01:08) > > > >>>> 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 > > > >>>> access SuperIO and SPI slave > > > device. > > > >>>> > > > >>>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com> > > > >>>> --- > > > >>> > > > >>> Is there resolution on this thread? > > > >> > > > >> Not yet. > > > >> > > > >> We have a system where the BMC (management controller) controls > > > >> some clocks, but the peripherals that it's clocking are outside > > > >> the BMC's control. In this case, the host processor us using some > > > >> UARTs and what not independent of any code running on the BMC. > > > >> > > > >> Ryan wants to have them marked as critical so the BMC never > > > >> powers them > > > down. > > > >> > > > >> However, there are systems that don't use this part of the soc, > > > >> so for those implementations they are not critical and Linux on > > > >> the BMC can turn them off. > > > >> > > > >> Do you have any thoughts? Has anyone solved a similar problem > already? > > > >> > > > > > > > > Is this critical clocks in DT? Where we want to have different DT > > > > for different device configurations to indicate that some clks > > > > should be marked critical so they're never turned off and other > > > > times they aren't so they're turned off? > > > > > > > > It also sounds sort of like the protected-clocks binding. Where > > > > you don't want to touch certain clks depending on the usage > > > > configuration of the SoC. There is a patch to make that generic > > > > that I haven't applied because it looks wrong at first glance[1]. > > > > Maybe not registering those clks to the framework on the > > > > configuration that Ryan has is > > > good enough? > > > > > > Could you please be more specific than the patch "looks wrong"? I'm > > > more than happy to update the patch to address your concerns, but I > > > cannot do that unless I know what your concerns are. > > > > > > Regards, > > > Samuel > > > > > > > [1] > > > > https://lore.kernel.org/r/20200903040015.5627-2-samuel@sholland.or > > > > g > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-02-01 7:20 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-28 7:01 [PATCH 0/1] Modify ASPEED SoC some default clks are critical Ryan Chen 2020-09-28 7:01 ` [PATCH 1/1] clk: aspeed: modify " Ryan Chen 2020-09-29 8:04 ` Joel Stanley 2020-09-29 8:37 ` Ryan Chen 2020-10-07 11:34 ` Joel Stanley 2020-10-08 2:33 ` Ryan Chen 2020-10-14 2:50 ` Stephen Boyd 2020-10-14 5:28 ` Joel Stanley 2020-10-14 5:39 ` Ryan Chen 2020-10-14 17:16 ` Stephen Boyd 2020-10-28 4:38 ` Joel Stanley 2020-10-29 2:25 ` Samuel Holland 2021-01-22 8:15 ` Ryan Chen 2021-01-25 0:47 ` Andrew Jeffery 2021-02-01 7:16 ` Ryan Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).