| Message ID | 20210409021313.433558-6-seanga2@gmail.com |
|---|---|
| State | Accepted |
| Commit | 800c7f6a1fadd6f954e5bac1515cda2231a78464 |
| Delegated to: | Andes |
| Headers | show |
| Series | riscv: k210: Enable use of AI ram bank | expand |
On 2021/04/09 11:13, Sean Anderson wrote: > No other (real) clocks have the cpu clock as their parent; instead they are > children of aclk. Move the clint clock under aclk to match them. > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > (no changes since v2) > > Changes in v2: > - New > > drivers/clk/kendryte/clk.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c > index ab86533bb4..5091f07eb0 100644 > --- a/drivers/clk/kendryte/clk.c > +++ b/drivers/clk/kendryte/clk.c > @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev) > > /* The MTIME register in CLINT runs at one 50th the CPU clock speed */ > clk_dm(K210_CLK_CLINT, > - clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50)); > + clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50)); > > return 0; > } > Looks good, but representing this clock is in fact useless, at least in Linux, since the clint driver does not use it directly and derives its rate from riscv_timebase which is set from the timebase-frequency DT property. Not sure how u-boot handles that though. Since your code allows changing the PLLs frequency, the timebase-frequency property may end up being buggy if it is not changed too.
On 4/8/21 10:54 PM, Damien Le Moal wrote: > On 2021/04/09 11:13, Sean Anderson wrote: >> No other (real) clocks have the cpu clock as their parent; instead they are >> children of aclk. Move the clint clock under aclk to match them. >> >> Signed-off-by: Sean Anderson <seanga2@gmail.com> >> --- >> >> (no changes since v2) >> >> Changes in v2: >> - New >> >> drivers/clk/kendryte/clk.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c >> index ab86533bb4..5091f07eb0 100644 >> --- a/drivers/clk/kendryte/clk.c >> +++ b/drivers/clk/kendryte/clk.c >> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev) >> >> /* The MTIME register in CLINT runs at one 50th the CPU clock speed */ >> clk_dm(K210_CLK_CLINT, >> - clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50)); >> + clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50)); >> >> return 0; >> } >> > > Looks good, but representing this clock is in fact useless, at least in Linux, > since the clint driver does not use it directly and derives its rate from > riscv_timebase which is set from the timebase-frequency DT property. > > Not sure how u-boot handles that though. Since your code allows changing the > PLLs frequency, the timebase-frequency property may end up being buggy if it is > not changed too. > U-Boot uses the clocks property since 47d7e3b5eb ("riscv: Move timer portions of SiFive CLINT to drivers/timer") :) --Sean
On 2021/04/09 11:54, Damien Le Moal wrote: > On 2021/04/09 11:13, Sean Anderson wrote: >> No other (real) clocks have the cpu clock as their parent; instead they are >> children of aclk. Move the clint clock under aclk to match them. >> >> Signed-off-by: Sean Anderson <seanga2@gmail.com> >> --- >> >> (no changes since v2) >> >> Changes in v2: >> - New >> >> drivers/clk/kendryte/clk.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c >> index ab86533bb4..5091f07eb0 100644 >> --- a/drivers/clk/kendryte/clk.c >> +++ b/drivers/clk/kendryte/clk.c >> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev) >> >> /* The MTIME register in CLINT runs at one 50th the CPU clock speed */ >> clk_dm(K210_CLK_CLINT, >> - clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50)); >> + clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50)); >> >> return 0; >> } >> > > Looks good, but representing this clock is in fact useless, at least in Linux, > since the clint driver does not use it directly and derives its rate from > riscv_timebase which is set from the timebase-frequency DT property. > > Not sure how u-boot handles that though. Since your code allows changing the > PLLs frequency, the timebase-frequency property may end up being buggy if it is > not changed too. Actually, thinking about this some more, the clint DT node should have an optional clock entry and use that clock rate to set riscv_timebase if the clock entry is present. riscv_timebase can be set using timebase-frequency DT property if the clock is not set. >
On 2021/04/09 11:58, Sean Anderson wrote: > > On 4/8/21 10:54 PM, Damien Le Moal wrote: >> On 2021/04/09 11:13, Sean Anderson wrote: >>> No other (real) clocks have the cpu clock as their parent; instead they are >>> children of aclk. Move the clint clock under aclk to match them. >>> >>> Signed-off-by: Sean Anderson <seanga2@gmail.com> >>> --- >>> >>> (no changes since v2) >>> >>> Changes in v2: >>> - New >>> >>> drivers/clk/kendryte/clk.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c >>> index ab86533bb4..5091f07eb0 100644 >>> --- a/drivers/clk/kendryte/clk.c >>> +++ b/drivers/clk/kendryte/clk.c >>> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev) >>> >>> /* The MTIME register in CLINT runs at one 50th the CPU clock speed */ >>> clk_dm(K210_CLK_CLINT, >>> - clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50)); >>> + clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50)); >>> >>> return 0; >>> } >>> >> >> Looks good, but representing this clock is in fact useless, at least in Linux, >> since the clint driver does not use it directly and derives its rate from >> riscv_timebase which is set from the timebase-frequency DT property. >> >> Not sure how u-boot handles that though. Since your code allows changing the >> PLLs frequency, the timebase-frequency property may end up being buggy if it is >> not changed too. >> > > U-Boot uses the clocks property since 47d7e3b5eb ("riscv: Move timer > portions of SiFive CLINT to drivers/timer") :) I remember that adding the clock property to Linux DT gave warning with make dtb_check. Hence I removed it. driver/clocksoure/timer-riscv.c also make the timebase-frequency DT property mandatory. Time for some more patching on Linux side I guess :) > > --Sean >
On 4/8/21 10:58 PM, Damien Le Moal wrote: > On 2021/04/09 11:54, Damien Le Moal wrote: >> On 2021/04/09 11:13, Sean Anderson wrote: >>> No other (real) clocks have the cpu clock as their parent; instead they are >>> children of aclk. Move the clint clock under aclk to match them. >>> >>> Signed-off-by: Sean Anderson <seanga2@gmail.com> >>> --- >>> >>> (no changes since v2) >>> >>> Changes in v2: >>> - New >>> >>> drivers/clk/kendryte/clk.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c >>> index ab86533bb4..5091f07eb0 100644 >>> --- a/drivers/clk/kendryte/clk.c >>> +++ b/drivers/clk/kendryte/clk.c >>> @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev) >>> >>> /* The MTIME register in CLINT runs at one 50th the CPU clock speed */ >>> clk_dm(K210_CLK_CLINT, >>> - clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50)); >>> + clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50)); >>> >>> return 0; >>> } >>> >> >> Looks good, but representing this clock is in fact useless, at least in Linux, >> since the clint driver does not use it directly and derives its rate from >> riscv_timebase which is set from the timebase-frequency DT property. >> >> Not sure how u-boot handles that though. Since your code allows changing the >> PLLs frequency, the timebase-frequency property may end up being buggy if it is >> not changed too. > > Actually, thinking about this some more, the clint DT node should have an > optional clock entry and use that clock rate to set riscv_timebase if the clock > entry is present. riscv_timebase can be set using timebase-frequency DT property > if the clock is not set. This is how it is done in U-Boot. See timer_pre_probe and timer_timebase_fallback for the mechanism. --Sean
diff --git a/drivers/clk/kendryte/clk.c b/drivers/clk/kendryte/clk.c index ab86533bb4..5091f07eb0 100644 --- a/drivers/clk/kendryte/clk.c +++ b/drivers/clk/kendryte/clk.c @@ -643,7 +643,7 @@ static int k210_clk_probe(struct udevice *dev) /* The MTIME register in CLINT runs at one 50th the CPU clock speed */ clk_dm(K210_CLK_CLINT, - clk_register_fixed_factor(NULL, "clint", "cpu", 0, 1, 50)); + clk_register_fixed_factor(NULL, "clint", "aclk", 0, 1, 50)); return 0; }
No other (real) clocks have the cpu clock as their parent; instead they are children of aclk. Move the clint clock under aclk to match them. Signed-off-by: Sean Anderson <seanga2@gmail.com> --- (no changes since v2) Changes in v2: - New drivers/clk/kendryte/clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)