* [PATCH 0/2] clk: aspeed/ast2600: Add critical clock setting logic @ 2020-01-15 21:26 Jae Hyun Yoo 2020-01-15 21:26 ` [PATCH 1/2] clk: aspeed: add " Jae Hyun Yoo 2020-01-15 21:26 ` [PATCH 2/2] clk: ast2600: " Jae Hyun Yoo 0 siblings, 2 replies; 9+ messages in thread From: Jae Hyun Yoo @ 2020-01-15 21:26 UTC (permalink / raw) To: Joel Stanley, Michael Turquette, Stephen Boyd, Andrew Jeffery Cc: linux-clk, linux-aspeed, openbmc, Jae Hyun Yoo This commit adds critical clock setting logic that applies CLK_IS_CRITICAL flag if it detects 'clock-critical' property in device tree. With this patch, each platform can enable critical clocks through device tree setting like below: &syscon { clock-critical = <ASPEED_CLK_GATE_BCLK>, <ASPEED_CLK_GATE_MCLK>, <ASPEED_CLK_GATE_REFCLK>; }; Please review. Thanks, Jae Jae Hyun Yoo (2): clk: aspeed: add critical clock setting logic clk: ast2600: add critical clock setting logic drivers/clk/clk-aspeed.c | 5 ++++- drivers/clk/clk-ast2600.c | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] clk: aspeed: add critical clock setting logic 2020-01-15 21:26 [PATCH 0/2] clk: aspeed/ast2600: Add critical clock setting logic Jae Hyun Yoo @ 2020-01-15 21:26 ` Jae Hyun Yoo 2020-01-16 1:38 ` Joel Stanley ` (2 more replies) 2020-01-15 21:26 ` [PATCH 2/2] clk: ast2600: " Jae Hyun Yoo 1 sibling, 3 replies; 9+ messages in thread From: Jae Hyun Yoo @ 2020-01-15 21:26 UTC (permalink / raw) To: Joel Stanley, Michael Turquette, Stephen Boyd, Andrew Jeffery Cc: linux-clk, linux-aspeed, openbmc, Jae Hyun Yoo This commit adds critical clock setting logic that applies CLK_IS_CRITICAL flag if it detects 'clock-critical' property in device tree. Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> --- drivers/clk/clk-aspeed.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c index 411ff5fb2c07..d22eeb574ede 100644 --- a/drivers/clk/clk-aspeed.c +++ b/drivers/clk/clk-aspeed.c @@ -541,8 +541,11 @@ static int aspeed_clk_probe(struct platform_device *pdev) for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) { const struct aspeed_gate_data *gd = &aspeed_gates[i]; + unsigned long flags = gd->flags; u32 gate_flags; + of_clk_detect_critical(pdev->dev.of_node, i, &flags); + /* Special case: the USB port 1 clock (bit 14) is always * working the opposite way from the other ones. */ @@ -550,7 +553,7 @@ static int aspeed_clk_probe(struct platform_device *pdev) hw = aspeed_clk_hw_register_gate(dev, gd->name, gd->parent_name, - gd->flags, + flags, map, gd->clock_idx, gd->reset_idx, -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clk: aspeed: add critical clock setting logic 2020-01-15 21:26 ` [PATCH 1/2] clk: aspeed: add " Jae Hyun Yoo @ 2020-01-16 1:38 ` Joel Stanley 2020-01-16 1:44 ` Jae Hyun Yoo 2020-01-16 9:57 ` Paul Menzel 2020-01-30 17:42 ` Stephen Boyd 2 siblings, 1 reply; 9+ messages in thread From: Joel Stanley @ 2020-01-16 1:38 UTC (permalink / raw) To: Jae Hyun Yoo Cc: Michael Turquette, Stephen Boyd, Andrew Jeffery, linux-clk, linux-aspeed, OpenBMC Maillist On Wed, 15 Jan 2020 at 21:25, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote: > > This commit adds critical clock setting logic that applies > CLK_IS_CRITICAL flag if it detects 'clock-critical' property in > device tree. > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> > --- > drivers/clk/clk-aspeed.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c > index 411ff5fb2c07..d22eeb574ede 100644 > --- a/drivers/clk/clk-aspeed.c > +++ b/drivers/clk/clk-aspeed.c > @@ -541,8 +541,11 @@ static int aspeed_clk_probe(struct platform_device *pdev) > > for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) { > const struct aspeed_gate_data *gd = &aspeed_gates[i]; > + unsigned long flags = gd->flags; > u32 gate_flags; > > + of_clk_detect_critical(pdev->dev.of_node, i, &flags); This makes sense to me. > + > /* Special case: the USB port 1 clock (bit 14) is always > * working the opposite way from the other ones. > */ > @@ -550,7 +553,7 @@ static int aspeed_clk_probe(struct platform_device *pdev) > hw = aspeed_clk_hw_register_gate(dev, > gd->name, > gd->parent_name, > - gd->flags, > + flags, For completeness should we make this flags | gd->flags > map, > gd->clock_idx, > gd->reset_idx, > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clk: aspeed: add critical clock setting logic 2020-01-16 1:38 ` Joel Stanley @ 2020-01-16 1:44 ` Jae Hyun Yoo 0 siblings, 0 replies; 9+ messages in thread From: Jae Hyun Yoo @ 2020-01-16 1:44 UTC (permalink / raw) To: Joel Stanley Cc: linux-aspeed, Stephen Boyd, Michael Turquette, OpenBMC Maillist, Andrew Jeffery, linux-clk On 1/15/2020 5:38 PM, Joel Stanley wrote: > On Wed, 15 Jan 2020 at 21:25, Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> wrote: >> >> This commit adds critical clock setting logic that applies >> CLK_IS_CRITICAL flag if it detects 'clock-critical' property in >> device tree. >> >> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> >> --- >> drivers/clk/clk-aspeed.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c >> index 411ff5fb2c07..d22eeb574ede 100644 >> --- a/drivers/clk/clk-aspeed.c >> +++ b/drivers/clk/clk-aspeed.c >> @@ -541,8 +541,11 @@ static int aspeed_clk_probe(struct platform_device *pdev) >> >> for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) { >> const struct aspeed_gate_data *gd = &aspeed_gates[i]; >> + unsigned long flags = gd->flags; >> u32 gate_flags; >> >> + of_clk_detect_critical(pdev->dev.of_node, i, &flags); > > This makes sense to me. > >> + >> /* Special case: the USB port 1 clock (bit 14) is always >> * working the opposite way from the other ones. >> */ >> @@ -550,7 +553,7 @@ static int aspeed_clk_probe(struct platform_device *pdev) >> hw = aspeed_clk_hw_register_gate(dev, >> gd->name, >> gd->parent_name, >> - gd->flags, >> + flags, > > For completeness should we make this > > flags | gd->flags Not needed. of_clk_detect_critical uses OR operation inside. >> map, >> gd->clock_idx, >> gd->reset_idx, >> -- >> 2.17.1 >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clk: aspeed: add critical clock setting logic 2020-01-15 21:26 ` [PATCH 1/2] clk: aspeed: add " Jae Hyun Yoo 2020-01-16 1:38 ` Joel Stanley @ 2020-01-16 9:57 ` Paul Menzel 2020-01-16 18:41 ` Jae Hyun Yoo 2020-01-30 17:42 ` Stephen Boyd 2 siblings, 1 reply; 9+ messages in thread From: Paul Menzel @ 2020-01-16 9:57 UTC (permalink / raw) To: Jae Hyun Yoo, Joel Stanley, Michael Turquette, Stephen Boyd, Andrew Jeffery Cc: openbmc, linux-clk, linux-aspeed [-- Attachment #1: Type: text/plain, Size: 1823 bytes --] Dear Jae, On 2020-01-15 22:26, Jae Hyun Yoo wrote: > This commit adds critical clock setting logic that applies > CLK_IS_CRITICAL flag if it detects 'clock-critical' property in > device tree. Tested how? > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> > --- > drivers/clk/clk-aspeed.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c > index 411ff5fb2c07..d22eeb574ede 100644 > --- a/drivers/clk/clk-aspeed.c > +++ b/drivers/clk/clk-aspeed.c > @@ -541,8 +541,11 @@ static int aspeed_clk_probe(struct platform_device *pdev) > > for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) { > const struct aspeed_gate_data *gd = &aspeed_gates[i]; > + unsigned long flags = gd->flags; > u32 gate_flags; > > + of_clk_detect_critical(pdev->dev.of_node, i, &flags); > + The function description in `drivers/clk/clk.c` has the warning below. > * Do not use this function. It exists only for legacy Device Tree > * bindings, such as the one-clock-per-node style that are outdated. > * Those bindings typically put all clock data into .dts and the Linux > * driver has no clock data, thus making it impossible to set this flag > * correctly from the driver. Only those drivers may call > * of_clk_detect_critical from their setup functions. Will this still work? > /* Special case: the USB port 1 clock (bit 14) is always > * working the opposite way from the other ones. > */ > @@ -550,7 +553,7 @@ static int aspeed_clk_probe(struct platform_device *pdev) > hw = aspeed_clk_hw_register_gate(dev, > gd->name, > gd->parent_name, > - gd->flags, > + flags, > map, > gd->clock_idx, > gd->reset_idx, Kind regards, Paul [-- Attachment #2: S/MIME Cryptographic Signature --] [-- Type: application/pkcs7-signature, Size: 5174 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clk: aspeed: add critical clock setting logic 2020-01-16 9:57 ` Paul Menzel @ 2020-01-16 18:41 ` Jae Hyun Yoo 0 siblings, 0 replies; 9+ messages in thread From: Jae Hyun Yoo @ 2020-01-16 18:41 UTC (permalink / raw) To: Paul Menzel, Joel Stanley, Michael Turquette, Stephen Boyd, Andrew Jeffery Cc: openbmc, linux-clk, linux-aspeed Dear Paul, On 1/16/2020 1:57 AM, Paul Menzel wrote: > Dear Jae, > > > On 2020-01-15 22:26, Jae Hyun Yoo wrote: >> This commit adds critical clock setting logic that applies >> CLK_IS_CRITICAL flag if it detects 'clock-critical' property in >> device tree. > > Tested how? I added in the cover letter how I tested it. For an example, BCLK can have the flag if I add below setting into one of 'aspeed-bmc-*.dts' files. &syscon { clock-critical = <ASPEED_CLK_GATE_BCLK>; }; >> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> >> --- >> drivers/clk/clk-aspeed.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c >> index 411ff5fb2c07..d22eeb574ede 100644 >> --- a/drivers/clk/clk-aspeed.c >> +++ b/drivers/clk/clk-aspeed.c >> @@ -541,8 +541,11 @@ static int aspeed_clk_probe(struct platform_device *pdev) >> >> for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) { >> const struct aspeed_gate_data *gd = &aspeed_gates[i]; >> + unsigned long flags = gd->flags; >> u32 gate_flags; >> >> + of_clk_detect_critical(pdev->dev.of_node, i, &flags); >> + > > The function description in `drivers/clk/clk.c` has the warning below. > >> * Do not use this function. It exists only for legacy Device Tree >> * bindings, such as the one-clock-per-node style that are outdated. >> * Those bindings typically put all clock data into .dts and the Linux >> * driver has no clock data, thus making it impossible to set this flag >> * correctly from the driver. Only those drivers may call >> * of_clk_detect_critical from their setup functions. > > Will this still work? At least, it still works now and still useful for this case. Actually, I made this change as an alternative way of https://www.spinics.net/lists/linux-clk/msg44836.html because not all Aspeed BMC systems enable BCLK as a critical clock, so it's for providing more flexible way of critical clock setting for various hardware configurations. If the function is deprecated and is going to be removed soon, would it be acceptable if I add the 'critical-clock' parsing code into this driver module instead of using the function? Best Regards, Jae >> /* Special case: the USB port 1 clock (bit 14) is always >> * working the opposite way from the other ones. >> */ >> @@ -550,7 +553,7 @@ static int aspeed_clk_probe(struct platform_device *pdev) >> hw = aspeed_clk_hw_register_gate(dev, >> gd->name, >> gd->parent_name, >> - gd->flags, >> + flags, >> map, >> gd->clock_idx, >> gd->reset_idx, > > > Kind regards, > > Paul > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clk: aspeed: add critical clock setting logic 2020-01-15 21:26 ` [PATCH 1/2] clk: aspeed: add " Jae Hyun Yoo 2020-01-16 1:38 ` Joel Stanley 2020-01-16 9:57 ` Paul Menzel @ 2020-01-30 17:42 ` Stephen Boyd 2020-01-30 20:59 ` Jae Hyun Yoo 2 siblings, 1 reply; 9+ messages in thread From: Stephen Boyd @ 2020-01-30 17:42 UTC (permalink / raw) To: Andrew Jeffery, Jae Hyun Yoo, Joel Stanley, Michael Turquette Cc: linux-clk, linux-aspeed, openbmc, Jae Hyun Yoo Quoting Jae Hyun Yoo (2020-01-15 13:26:38) > This commit adds critical clock setting logic that applies > CLK_IS_CRITICAL flag if it detects 'clock-critical' property in > device tree. Yes that is what the patch does. The commit text is supposed to explain _why_ the patch is important. Please read "The canonical patch format" from Documentation/process/submitting-patches.rst to understand what is expected. > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> > --- > drivers/clk/clk-aspeed.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c > index 411ff5fb2c07..d22eeb574ede 100644 > --- a/drivers/clk/clk-aspeed.c > +++ b/drivers/clk/clk-aspeed.c > @@ -541,8 +541,11 @@ static int aspeed_clk_probe(struct platform_device *pdev) > > for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) { > const struct aspeed_gate_data *gd = &aspeed_gates[i]; > + unsigned long flags = gd->flags; > u32 gate_flags; > > + of_clk_detect_critical(pdev->dev.of_node, i, &flags); > + Do you need clks to be critical, but only sometimes? What clks need to be critical? Why aren't there drivers for those clks that turn them on as necessary? There was a lengthy discussion years ago on the list about this function and how it's not supposed to be used in newer code. Maybe we need to revisit that discussion and conclude that sometimes we actually do need clks to be turned on and kept on because we'll never have a driver for them in the kernel. Similar to how pinctrl has pin hogs. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] clk: aspeed: add critical clock setting logic 2020-01-30 17:42 ` Stephen Boyd @ 2020-01-30 20:59 ` Jae Hyun Yoo 0 siblings, 0 replies; 9+ messages in thread From: Jae Hyun Yoo @ 2020-01-30 20:59 UTC (permalink / raw) To: Stephen Boyd, Andrew Jeffery, Joel Stanley, Michael Turquette Cc: linux-clk, linux-aspeed, openbmc Hi Stephen, On 1/30/2020 9:42 AM, Stephen Boyd wrote: > Quoting Jae Hyun Yoo (2020-01-15 13:26:38) >> This commit adds critical clock setting logic that applies >> CLK_IS_CRITICAL flag if it detects 'clock-critical' property in >> device tree. > > Yes that is what the patch does. The commit text is supposed to explain > _why_ the patch is important. Please read "The canonical patch format" > from Documentation/process/submitting-patches.rst to understand what is > expected. I see. I'll add more detailed summary. >> >> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> >> --- >> drivers/clk/clk-aspeed.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c >> index 411ff5fb2c07..d22eeb574ede 100644 >> --- a/drivers/clk/clk-aspeed.c >> +++ b/drivers/clk/clk-aspeed.c >> @@ -541,8 +541,11 @@ static int aspeed_clk_probe(struct platform_device *pdev) >> >> for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) { >> const struct aspeed_gate_data *gd = &aspeed_gates[i]; >> + unsigned long flags = gd->flags; >> u32 gate_flags; >> >> + of_clk_detect_critical(pdev->dev.of_node, i, &flags); >> + > > Do you need clks to be critical, but only sometimes? What clks need to > be critical? Why aren't there drivers for those clks that turn them on > as necessary? > > There was a lengthy discussion years ago on the list about this function > and how it's not supposed to be used in newer code. Maybe we need to > revisit that discussion and conclude that sometimes we actually do need > clks to be turned on and kept on because we'll never have a driver for > them in the kernel. Similar to how pinctrl has pin hogs. Yes, I need to make BCLK as a critical only for specific platforms. BCLK is for controllers that are connected through PCI or PCIe bus to the host machine such as VGA controller, 2D graphics engine and P2A bridge in this SoC. I'm currently trying to enable VGA controller which is actually independent from Aspeed BMC SoC. It means that the VGA can be reset only when either host PCI bus reset or host power-on reset is asserted. Basically, VGA hardware module is controlled by the host machine not by the Aspeed BMC SoC. I submitted this patch as an alternative solution of https://www.spinics.net/lists/linux-clk/msg44836.html because there could be use cases that intentionally disable the VGA controller depend on hardware design. So it'd be helpful for reducing power consumption and for allocating more generic memory space instead of allocating dedicated VGA shared memory if we can flexibly config the BCLK. I think, we don't need to add VGA driver just for enabling the clock because the VGA controller is actually controlled by host machine as I explained above so I made this patch set instead. I agree with you that we need to revisit the discussion. Thanks, Jae ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] clk: ast2600: add critical clock setting logic 2020-01-15 21:26 [PATCH 0/2] clk: aspeed/ast2600: Add critical clock setting logic Jae Hyun Yoo 2020-01-15 21:26 ` [PATCH 1/2] clk: aspeed: add " Jae Hyun Yoo @ 2020-01-15 21:26 ` Jae Hyun Yoo 1 sibling, 0 replies; 9+ messages in thread From: Jae Hyun Yoo @ 2020-01-15 21:26 UTC (permalink / raw) To: Joel Stanley, Michael Turquette, Stephen Boyd, Andrew Jeffery Cc: linux-clk, linux-aspeed, openbmc, Jae Hyun Yoo This commit adds critical clock setting logic that applies CLK_IS_CRITICAL flag if it detects 'clock-critical' property in device tree. Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com> --- drivers/clk/clk-ast2600.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c index 392d01705b97..49d89ffdd4be 100644 --- a/drivers/clk/clk-ast2600.c +++ b/drivers/clk/clk-ast2600.c @@ -599,8 +599,11 @@ static int aspeed_g6_clk_probe(struct platform_device *pdev) for (i = 0; i < ARRAY_SIZE(aspeed_g6_gates); i++) { const struct aspeed_gate_data *gd = &aspeed_g6_gates[i]; + unsigned long flags = gd->flags; u32 gate_flags; + of_clk_detect_critical(pdev->dev.of_node, i, &flags); + /* * Special case: the USB port 1 clock (bit 14) is always * working the opposite way from the other ones. @@ -609,7 +612,7 @@ static int aspeed_g6_clk_probe(struct platform_device *pdev) hw = aspeed_g6_clk_hw_register_gate(dev, gd->name, gd->parent_name, - gd->flags, + flags, map, gd->clock_idx, gd->reset_idx, -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-01-30 21:00 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-15 21:26 [PATCH 0/2] clk: aspeed/ast2600: Add critical clock setting logic Jae Hyun Yoo 2020-01-15 21:26 ` [PATCH 1/2] clk: aspeed: add " Jae Hyun Yoo 2020-01-16 1:38 ` Joel Stanley 2020-01-16 1:44 ` Jae Hyun Yoo 2020-01-16 9:57 ` Paul Menzel 2020-01-16 18:41 ` Jae Hyun Yoo 2020-01-30 17:42 ` Stephen Boyd 2020-01-30 20:59 ` Jae Hyun Yoo 2020-01-15 21:26 ` [PATCH 2/2] clk: ast2600: " Jae Hyun Yoo
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).