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
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 <[email protected]>
---
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
On Mon, 28 Sep 2020 at 07:01, Ryan Chen <[email protected]> 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 <[email protected]>
> ---
> 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/[email protected]/
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
>
> From: Joel Stanley <[email protected]>
> Sent: Tuesday, September 29, 2020 4:04 PM
> To: Ryan Chen <[email protected]>; Jae Hyun Yoo
> <[email protected]>; Andrew Jeffery <[email protected]>
> Cc: Michael Turquette <[email protected]>; Stephen Boyd
> <[email protected]>; [email protected]; Linux ARM
> <[email protected]>; linux-aspeed
> <[email protected]>; Linux Kernel Mailing List
> <[email protected]>; BMC-SW <[email protected]>
> Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical
>
> On Mon, 28 Sep 2020 at 07:01, Ryan Chen <[email protected]>
> 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 <[email protected]>
> > ---
> > 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
> >
On Tue, 29 Sep 2020 at 08:40, Ryan Chen <[email protected]> wrote:
>
> > From: Joel Stanley <[email protected]>
> > Sent: Tuesday, September 29, 2020 4:04 PM
> > To: Ryan Chen <[email protected]>; Jae Hyun Yoo
> > <[email protected]>; Andrew Jeffery <[email protected]>
> > Cc: Michael Turquette <[email protected]>; Stephen Boyd
> > <[email protected]>; [email protected]; Linux ARM
> > <[email protected]>; linux-aspeed
> > <[email protected]>; Linux Kernel Mailing List
> > <[email protected]>; BMC-SW <[email protected]>
> > Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical
> >
> > On Mon, 28 Sep 2020 at 07:01, Ryan Chen <[email protected]>
> > 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 <[email protected]>
> > > ---
> > > 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
> -----Original Message-----
> From: Joel Stanley <[email protected]>
> Sent: Wednesday, October 7, 2020 7:34 PM
> To: Ryan Chen <[email protected]>
> Cc: Jae Hyun Yoo <[email protected]>; Andrew Jeffery
> <[email protected]>; Michael Turquette <[email protected]>; Stephen
> Boyd <[email protected]>; [email protected]; Linux ARM
> <[email protected]>; linux-aspeed
> <[email protected]>; Linux Kernel Mailing List
> <[email protected]>; BMC-SW <[email protected]>
> Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical
>
> On Tue, 29 Sep 2020 at 08:40, Ryan Chen <[email protected]>
> wrote:
> >
> > > From: Joel Stanley <[email protected]>
> > > Sent: Tuesday, September 29, 2020 4:04 PM
> > > To: Ryan Chen <[email protected]>; Jae Hyun Yoo
> > > <[email protected]>; Andrew Jeffery <[email protected]>
> > > Cc: Michael Turquette <[email protected]>; Stephen Boyd
> > > <[email protected]>; [email protected]; Linux ARM
> > > <[email protected]>; linux-aspeed
> > > <[email protected]>; Linux Kernel Mailing List
> > > <[email protected]>; BMC-SW <[email protected]>
> > > Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are
> > > critical
> > >
> > > On Mon, 28 Sep 2020 at 07:01, Ryan Chen <[email protected]>
> > > 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 <[email protected]>
> > > > ---
> > > > 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.
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 <[email protected]>
> ---
Is there resolution on this thread?
On Wed, 14 Oct 2020 at 02:50, Stephen Boyd <[email protected]> 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 <[email protected]>
> > ---
>
> 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
> -----Original Message-----
> From: Joel Stanley <[email protected]>
> Sent: Wednesday, October 14, 2020 1:28 PM
> To: Stephen Boyd <[email protected]>
> Cc: Andrew Jeffery <[email protected]>; Michael Turquette
> <[email protected]>; Ryan Chen <[email protected]>;
> BMC-SW <[email protected]>; Linux ARM
> <[email protected]>; linux-aspeed
> <[email protected]>; [email protected]; Linux Kernel
> Mailing List <[email protected]>
> Subject: Re: [PATCH 1/1] clk: aspeed: modify some default clks are critical
>
> On Wed, 14 Oct 2020 at 02:50, Stephen Boyd <[email protected]> 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 <[email protected]>
> > > ---
> >
> > 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?
>
Quoting Joel Stanley (2020-10-13 22:28:00)
> On Wed, 14 Oct 2020 at 02:50, Stephen Boyd <[email protected]> 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 <[email protected]>
> > > ---
> >
> > 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/[email protected]
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 <[email protected]> wrote:
>
> Quoting Joel Stanley (2020-10-13 22:28:00)
> > On Wed, 14 Oct 2020 at 02:50, Stephen Boyd <[email protected]> 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 <[email protected]>
> > > > ---
> > >
> > > 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/[email protected]
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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>
>>> 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/[email protected]
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 <[email protected]>
> Sent: Thursday, October 29, 2020 10:25 AM
> To: Stephen Boyd <[email protected]>; Joel Stanley <[email protected]>
> Cc: Andrew Jeffery <[email protected]>; Michael Turquette
> <[email protected]>; Ryan Chen <[email protected]>;
> BMC-SW <[email protected]>; Linux ARM
> <[email protected]>; linux-aspeed
> <[email protected]>; [email protected]; Linux Kernel
> Mailing List <[email protected]>
> 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 <[email protected]> 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 <[email protected]>
> >>>> ---
> >>>
> >>> 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/[email protected]
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 <[email protected]>
> > Sent: Thursday, October 29, 2020 10:25 AM
> > To: Stephen Boyd <[email protected]>; Joel Stanley <[email protected]>
> > Cc: Andrew Jeffery <[email protected]>; Michael Turquette
> > <[email protected]>; Ryan Chen <[email protected]>;
> > BMC-SW <[email protected]>; Linux ARM
> > <[email protected]>; linux-aspeed
> > <[email protected]>; [email protected]; Linux Kernel
> > Mailing List <[email protected]>
> > 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 <[email protected]> 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 <[email protected]>
> > >>>> ---
> > >>>
> > >>> 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/[email protected]
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> -----Original Message-----
> From: Andrew Jeffery <[email protected]>
> Sent: Monday, January 25, 2021 8:47 AM
> To: Ryan Chen <[email protected]>; Samuel Holland
> <[email protected]>; Stephen Boyd <[email protected]>; Joel Stanley
> <[email protected]>
> Cc: BMC-SW <[email protected]>; linux-aspeed
> <[email protected]>; Michael Turquette
> <[email protected]>; Linux Kernel Mailing List
> <[email protected]>; [email protected]; Linux ARM
> <[email protected]>
> 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 <[email protected]>
> > > Sent: Thursday, October 29, 2020 10:25 AM
> > > To: Stephen Boyd <[email protected]>; Joel Stanley <[email protected]>
> > > Cc: Andrew Jeffery <[email protected]>; Michael Turquette
> > > <[email protected]>; Ryan Chen <[email protected]>;
> > > BMC-SW <[email protected]>; Linux ARM
> > > <[email protected]>; linux-aspeed
> > > <[email protected]>; [email protected]; Linux
> > > Kernel Mailing List <[email protected]>
> > > 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 <[email protected]>
> 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 <[email protected]>
> > > >>>> ---
> > > >>>
> > > >>> 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/[email protected]
> > > > g
> > _______________________________________________
> > linux-arm-kernel mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >