| Message ID | 20240507065319.274976-1-xingyu.wu@starfivetech.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add notifier for PLL0 clock and set it 1.5GHz on | expand |
On Tue, May 07, 2024 at 02:53:17PM +0800, Xingyu Wu wrote: > This patch is to add the notifier for PLL0 clock and set the PLL0 rate > to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC. > > The first patch is to add the notifier for PLL0 clock. Setting the PLL0 > rate need the son clock (cpu_root) to switch its parent clock to OSC > clock and switch it back after setting PLL0 rate. It need to use the > cpu_root clock from SYSCRG and register the notifier in the SYSCRG > driver. > > The second patch is to set cpu_core rate to 500MHz and PLL0 rate to > 1.5GHz to fix the problem about the lower rate of CPUfreq on the > visionfive board. The cpu_core clock rate is set to 500MHz first to > ensure that the cpu frequency will not suddenly become high and the cpu > voltage is not enough to cause a crash when the PLL0 is set to 1.5GHz. > The cpu voltage and frequency are then adjusted together by CPUfreq. Hmm, how does sequencing work here? If we split the patches between trees it sounds like without the dts patch, the clock tree would (or could) crash, or mainline if the clock changes there before the dts ones do. Am I misunderstanding that?
On 11/05/2024 05:05, Conor Dooley wrote: > > On Tue, May 07, 2024 at 02:53:17PM +0800, Xingyu Wu wrote: > > This patch is to add the notifier for PLL0 clock and set the PLL0 rate > > to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC. > > > > The first patch is to add the notifier for PLL0 clock. Setting the > > PLL0 rate need the son clock (cpu_root) to switch its parent clock to > > OSC clock and switch it back after setting PLL0 rate. It need to use > > the cpu_root clock from SYSCRG and register the notifier in the SYSCRG > > driver. > > > > The second patch is to set cpu_core rate to 500MHz and PLL0 rate to > > 1.5GHz to fix the problem about the lower rate of CPUfreq on the > > visionfive board. The cpu_core clock rate is set to 500MHz first to > > ensure that the cpu frequency will not suddenly become high and the > > cpu voltage is not enough to cause a crash when the PLL0 is set to 1.5GHz. > > The cpu voltage and frequency are then adjusted together by CPUfreq. > > Hmm, how does sequencing work here? If we split the patches between trees it > sounds like without the dts patch, the clock tree would (or > could) crash, or mainline if the clock changes there before the dts ones do. Am I > misunderstanding that? Oh, I think you misunderstood it. Patch 1 (clock driver patch) does not cause the clock tree crash without the patch 2 (dts patch), and it just provides the correct flow of how to change the PLL0 rate. The patch 2 is to set the clock rate of cpu_core and PLL0 rate, which causes the crash without patch 1. Setting cpu_core rate is to avoid crashes by insufficient cpu voltage when setting PLL0 rate. Best regards, Xingyu Wu
On Sat, May 11, 2024 at 03:02:56AM +0000, Xingyu Wu wrote: > On 11/05/2024 05:05, Conor Dooley wrote: > > > > On Tue, May 07, 2024 at 02:53:17PM +0800, Xingyu Wu wrote: > > > This patch is to add the notifier for PLL0 clock and set the PLL0 rate > > > to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC. > > > > > > The first patch is to add the notifier for PLL0 clock. Setting the > > > PLL0 rate need the son clock (cpu_root) to switch its parent clock to > > > OSC clock and switch it back after setting PLL0 rate. It need to use > > > the cpu_root clock from SYSCRG and register the notifier in the SYSCRG > > > driver. > > > > > > The second patch is to set cpu_core rate to 500MHz and PLL0 rate to > > > 1.5GHz to fix the problem about the lower rate of CPUfreq on the > > > visionfive board. The cpu_core clock rate is set to 500MHz first to > > > ensure that the cpu frequency will not suddenly become high and the > > > cpu voltage is not enough to cause a crash when the PLL0 is set to 1.5GHz. > > > The cpu voltage and frequency are then adjusted together by CPUfreq. > > > > Hmm, how does sequencing work here? If we split the patches between trees it > > sounds like without the dts patch, the clock tree would (or > > could) crash, or mainline if the clock changes there before the dts ones do. Am I > > misunderstanding that? > > Oh, I think you misunderstood it. Patch 1 (clock driver patch) does not cause the > clock tree crash without the patch 2 (dts patch), and it just provides the correct > flow of how to change the PLL0 rate. The patch 2 is to set the clock rate of > cpu_core and PLL0 rate, which causes the crash without patch 1. Setting cpu_core > rate is to avoid crashes by insufficient cpu voltage when setting PLL0 rate. So is the problem in the other direction then? My dts tree will crash if I apply the dts change without the clock patch? Additionally, what about U-Boot? Will it have problems if the dts is imported there without changes to its clock driver? Cheers, Conor.
On 11/05/2024 20:19, Conor Dooley wrote: > > On Sat, May 11, 2024 at 03:02:56AM +0000, Xingyu Wu wrote: > > On 11/05/2024 05:05, Conor Dooley wrote: > > > > > > On Tue, May 07, 2024 at 02:53:17PM +0800, Xingyu Wu wrote: > > > > This patch is to add the notifier for PLL0 clock and set the PLL0 > > > > rate to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC. > > > > > > > > The first patch is to add the notifier for PLL0 clock. Setting the > > > > PLL0 rate need the son clock (cpu_root) to switch its parent clock > > > > to OSC clock and switch it back after setting PLL0 rate. It need > > > > to use the cpu_root clock from SYSCRG and register the notifier in > > > > the SYSCRG driver. > > > > > > > > The second patch is to set cpu_core rate to 500MHz and PLL0 rate > > > > to 1.5GHz to fix the problem about the lower rate of CPUfreq on > > > > the visionfive board. The cpu_core clock rate is set to 500MHz > > > > first to ensure that the cpu frequency will not suddenly become > > > > high and the cpu voltage is not enough to cause a crash when the PLL0 is set > to 1.5GHz. > > > > The cpu voltage and frequency are then adjusted together by CPUfreq. > > > > > > Hmm, how does sequencing work here? If we split the patches between > > > trees it sounds like without the dts patch, the clock tree would (or > > > could) crash, or mainline if the clock changes there before the dts > > > ones do. Am I misunderstanding that? > > > > Oh, I think you misunderstood it. Patch 1 (clock driver patch) does > > not cause the clock tree crash without the patch 2 (dts patch), and it > > just provides the correct flow of how to change the PLL0 rate. The > > patch 2 is to set the clock rate of cpu_core and PLL0 rate, which > > causes the crash without patch 1. Setting cpu_core rate is to avoid crashes by > insufficient cpu voltage when setting PLL0 rate. > > So is the problem in the other direction then? My dts tree will crash if I apply the > dts change without the clock patch? Sorry, I tested it and it could not crash using only dts patch. It can separate the patches and use it individually. > Additionally, what about U-Boot? Will it have problems if the dts is imported > there without changes to its clock driver? > It is not apply to U-Boot. In the U-Boot, the PLL0 rate should be 1GHz to for GMAC and PMIC to work. But now the PLL0 rate should be 1.5GHz in the Linux. Best regards, Xingyu Wu
On Tue, May 14, 2024 at 07:40:02AM +0000, Xingyu Wu wrote: > On 11/05/2024 20:19, Conor Dooley wrote: > > > > On Sat, May 11, 2024 at 03:02:56AM +0000, Xingyu Wu wrote: > > > On 11/05/2024 05:05, Conor Dooley wrote: > > > > > > > > On Tue, May 07, 2024 at 02:53:17PM +0800, Xingyu Wu wrote: > > > > > This patch is to add the notifier for PLL0 clock and set the PLL0 > > > > > rate to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC. > > > > > > > > > > The first patch is to add the notifier for PLL0 clock. Setting the > > > > > PLL0 rate need the son clock (cpu_root) to switch its parent clock > > > > > to OSC clock and switch it back after setting PLL0 rate. It need > > > > > to use the cpu_root clock from SYSCRG and register the notifier in > > > > > the SYSCRG driver. > > > > > > > > > > The second patch is to set cpu_core rate to 500MHz and PLL0 rate > > > > > to 1.5GHz to fix the problem about the lower rate of CPUfreq on > > > > > the visionfive board. The cpu_core clock rate is set to 500MHz > > > > > first to ensure that the cpu frequency will not suddenly become > > > > > high and the cpu voltage is not enough to cause a crash when the PLL0 is set > > to 1.5GHz. > > > > > The cpu voltage and frequency are then adjusted together by CPUfreq. > > > > > > > > Hmm, how does sequencing work here? If we split the patches between > > > > trees it sounds like without the dts patch, the clock tree would (or > > > > could) crash, or mainline if the clock changes there before the dts > > > > ones do. Am I misunderstanding that? > > > > > > Oh, I think you misunderstood it. Patch 1 (clock driver patch) does > > > not cause the clock tree crash without the patch 2 (dts patch), and it > > > just provides the correct flow of how to change the PLL0 rate. The > > > patch 2 is to set the clock rate of cpu_core and PLL0 rate, which > > > causes the crash without patch 1. Setting cpu_core rate is to avoid crashes by > > insufficient cpu voltage when setting PLL0 rate. > > > > So is the problem in the other direction then? My dts tree will crash if I apply the > > dts change without the clock patch? > > Sorry, I tested it and it could not crash using only dts patch. It can separate the > patches and use it individually. > > > Additionally, what about U-Boot? Will it have problems if the dts is imported > > there without changes to its clock driver? > > > > It is not apply to U-Boot. In the U-Boot, the PLL0 rate should be 1GHz to for GMAC > and PMIC to work. But now the PLL0 rate should be 1.5GHz in the Linux. There's a push in U-Boot to move devicestrees to use "OF_UPSTREAM", which means importing devicetrees directly from Linux and using them in U-Boot. I don't really want to merge a patch that would present U-Boot with a problem if the VisionFive 2 moved to that model there. Cheers, Conor.
On 15/05/2024 02:08, Conor Dooley wrote: > > On Tue, May 14, 2024 at 07:40:02AM +0000, Xingyu Wu wrote: > > On 11/05/2024 20:19, Conor Dooley wrote: > > > > > > On Sat, May 11, 2024 at 03:02:56AM +0000, Xingyu Wu wrote: > > > > On 11/05/2024 05:05, Conor Dooley wrote: > > > > > > > > > > On Tue, May 07, 2024 at 02:53:17PM +0800, Xingyu Wu wrote: > > > > > > This patch is to add the notifier for PLL0 clock and set the > > > > > > PLL0 rate to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC. > > > > > > > > > > > > The first patch is to add the notifier for PLL0 clock. Setting > > > > > > the > > > > > > PLL0 rate need the son clock (cpu_root) to switch its parent > > > > > > clock to OSC clock and switch it back after setting PLL0 rate. > > > > > > It need to use the cpu_root clock from SYSCRG and register the > > > > > > notifier in the SYSCRG driver. > > > > > > > > > > > > The second patch is to set cpu_core rate to 500MHz and PLL0 > > > > > > rate to 1.5GHz to fix the problem about the lower rate of > > > > > > CPUfreq on the visionfive board. The cpu_core clock rate is > > > > > > set to 500MHz first to ensure that the cpu frequency will not > > > > > > suddenly become high and the cpu voltage is not enough to > > > > > > cause a crash when the PLL0 is set > > > to 1.5GHz. > > > > > > The cpu voltage and frequency are then adjusted together by CPUfreq. > > > > > > > > > > Hmm, how does sequencing work here? If we split the patches > > > > > between trees it sounds like without the dts patch, the clock > > > > > tree would (or > > > > > could) crash, or mainline if the clock changes there before the > > > > > dts ones do. Am I misunderstanding that? > > > > > > > > Oh, I think you misunderstood it. Patch 1 (clock driver patch) > > > > does not cause the clock tree crash without the patch 2 (dts > > > > patch), and it just provides the correct flow of how to change the > > > > PLL0 rate. The patch 2 is to set the clock rate of cpu_core and > > > > PLL0 rate, which causes the crash without patch 1. Setting > > > > cpu_core rate is to avoid crashes by > > > insufficient cpu voltage when setting PLL0 rate. > > > > > > So is the problem in the other direction then? My dts tree will > > > crash if I apply the dts change without the clock patch? > > > > Sorry, I tested it and it could not crash using only dts patch. It can > > separate the patches and use it individually. > > > > > Additionally, what about U-Boot? Will it have problems if the dts is > > > imported there without changes to its clock driver? > > > > > > > It is not apply to U-Boot. In the U-Boot, the PLL0 rate should be 1GHz > > to for GMAC and PMIC to work. But now the PLL0 rate should be 1.5GHz in the > Linux. > > There's a push in U-Boot to move devicestrees to use "OF_UPSTREAM", which > means importing devicetrees directly from Linux and using them in U-Boot. I > don't really want to merge a patch that would present U-Boot with a problem if > the VisionFive 2 moved to that model there. > > Cheers, > Conor. Would it be better if I change the rates of PLL0 and CPU core in the driver not dts, and avoid the dts of Linux and U-Boot being different? Thanks, Xingyu Wu
On Wed, May 15, 2024 at 02:23:47AM +0000, Xingyu Wu wrote: > On 15/05/2024 02:08, Conor Dooley wrote: > > There's a push in U-Boot to move devicestrees to use "OF_UPSTREAM", which > > means importing devicetrees directly from Linux and using them in U-Boot. I > > don't really want to merge a patch that would present U-Boot with a problem if > > the VisionFive 2 moved to that model there. > Would it be better if I change the rates of PLL0 and CPU core in the driver not dts, > and avoid the dts of Linux and U-Boot being different? I'd definitely prefer if we don't include stuff in the kernel tree that would cause problems for U-Boot if imported there, yeah.
On Wed, May 15, 2024 at 7:31 PM Conor Dooley <conor@kernel.org> wrote: > > On Wed, May 15, 2024 at 02:23:47AM +0000, Xingyu Wu wrote: > > On 15/05/2024 02:08, Conor Dooley wrote: > > > > There's a push in U-Boot to move devicestrees to use "OF_UPSTREAM", which > > > means importing devicetrees directly from Linux and using them in U-Boot. I > > > don't really want to merge a patch that would present U-Boot with a problem if > > > the VisionFive 2 moved to that model there. > > > Would it be better if I change the rates of PLL0 and CPU core in the driver not dts, > > and avoid the dts of Linux and U-Boot being different? > > I'd definitely prefer if we don't include stuff in the kernel tree that > would cause problems for U-Boot if imported there, yeah. > What is the current state of this patchset? I noticed this patchset on the U-Boot side from Hal Feng: [PATCH v1 0/4] Sync StarFive JH7110 clock and reset dt-bindings with Linux It seems to indicate that there is WIP for OF_UPSTREAM support. Cheers, david
On Tue, Jun 04, 2024 at 12:45:48PM +0300, David Abdurachmanov wrote: > On Wed, May 15, 2024 at 7:31 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Wed, May 15, 2024 at 02:23:47AM +0000, Xingyu Wu wrote: > > > On 15/05/2024 02:08, Conor Dooley wrote: > > > > > > There's a push in U-Boot to move devicestrees to use "OF_UPSTREAM", which > > > > means importing devicetrees directly from Linux and using them in U-Boot. I > > > > don't really want to merge a patch that would present U-Boot with a problem if > > > > the VisionFive 2 moved to that model there. > > > > > Would it be better if I change the rates of PLL0 and CPU core in the driver not dts, > > > and avoid the dts of Linux and U-Boot being different? > > > > I'd definitely prefer if we don't include stuff in the kernel tree that > > would cause problems for U-Boot if imported there, yeah. > > > > What is the current state of this patchset? v6: https://lore.kernel.org/all/20240603020607.25122-1-xingyu.wu@starfivetech.com/ Guess it didn't go to the riscv ml because the dts patch was dropped. > I noticed this patchset on the U-Boot side from Hal Feng: > [PATCH v1 0/4] Sync StarFive JH7110 clock and reset dt-bindings with Linux > > It seems to indicate that there is WIP for OF_UPSTREAM support. And as a commenter on that patchset you reference said, they should actually use OF_UPSTREAM directly rather than manual syncs like that series. I'm not sure how the U-Boot folks want to address that w.r.t. bisection though, in cases like this where the defines do not have identical names. Thanks, Conor.
This patch is to add the notifier for PLL0 clock and set the PLL0 rate to 1.5GHz to fix the lower rate of CPUfreq on the JH7110 SoC. The first patch is to add the notifier for PLL0 clock. Setting the PLL0 rate need the son clock (cpu_root) to switch its parent clock to OSC clock and switch it back after setting PLL0 rate. It need to use the cpu_root clock from SYSCRG and register the notifier in the SYSCRG driver. The second patch is to set cpu_core rate to 500MHz and PLL0 rate to 1.5GHz to fix the problem about the lower rate of CPUfreq on the visionfive board. The cpu_core clock rate is set to 500MHz first to ensure that the cpu frequency will not suddenly become high and the cpu voltage is not enough to cause a crash when the PLL0 is set to 1.5GHz. The cpu voltage and frequency are then adjusted together by CPUfreq. Changes since v4: - Fixed the wrong words. - Added the Fixes tag in first patch. v4: https://lore.kernel.org/all/20240410033148.213991-1-xingyu.wu@starfivetech.com/ Changes since v3: - Added the notifier for PLL0 clock. - Set cpu_core rate in DTS v3: https://lore.kernel.org/all/20240402090920.11627-1-xingyu.wu@starfivetech.com/ Changes since v2: - Made the steps into the process into the process of setting PLL0 rate v2: https://lore.kernel.org/all/20230821152915.208366-1-xingyu.wu@starfivetech.com/ Changes since v1: - Added the fixes tag in the commit. v1: https://lore.kernel.org/all/20230811033631.160912-1-xingyu.wu@starfivetech.com/ Xingyu Wu (2): clk: starfive: jh7110-sys: Add notifier for PLL0 clock riscv: dts: starfive: visionfive-2: Fix lower rate of CPUfreq by setting PLL0 rate to 1.5GHz .../jh7110-starfive-visionfive-2.dtsi | 6 ++++ .../clk/starfive/clk-starfive-jh7110-sys.c | 31 ++++++++++++++++++- drivers/clk/starfive/clk-starfive-jh71x0.h | 2 ++ 3 files changed, 38 insertions(+), 1 deletion(-)