riscvarchive / riscv-linux

RISC-V Linux Port
606 stars 209 forks source link

dt-bindings: Correct RISC-V's timebase-frequency #110

Closed palmer-dabbelt closed 6 years ago

palmer-dabbelt commented 7 years ago

Someone must have read the device tree specification incorrectly, because we were putting timebase-frequency in the wrong place. This corrects the issue, moving it from

/ {
        cpus {
                timebase-frequency = X;
        }
}

to

/ {
        cpus {
                cpu@0 {
                        timebase-frequency = X;
                }
        }
}

This is great, because the timer's frequency should really be a per-cpu quantity on RISC-V systems since there's a timer per CPU. This should lead to some cleanups in our timer driver.

CC: Wesley Terpstra wesley@sifive.com Signed-off-by: Palmer Dabbelt palmer@sifive.com

terpstra commented 7 years ago

The DTS spec says this: "Properties that have identical values across CPU nodes may be placed in the cpus node instead." And the timebase-frequency actually describes the CLINT RTC frequency, not the processor frequency. Since there is one CLINT per coreplex, my original thinking was that it should be common to all cpu nodes.

However, in the meantime we've started building systems with multiple coreplexes, and in the future multiple chips. In those cases you have multiple CLINTs, so moving this value into the cpu node makes sense.

palmer-dabbelt commented 7 years ago

Ah, OK. So I think we should fix Linux to handle this behavior, and we don't need to mess with the examples.

terpstra commented 7 years ago

The actual fix needed here is a change to kernel/time.c:time_init ... I think we should probably look in both places for this information.

terpstra commented 7 years ago

rocket-chip already now emits the timebase-frequency inside the cpu node, which is why linux has stopped booting on it. This was needed to create proper output on a two coreplex system.

palmer-dabbelt commented 7 years ago

Ya, that's what I meant by fix Linux :). We need to handle everything that's valid as part of the spec. I'll fix it.

terpstra commented 7 years ago

I mean, it doesn't hurt to also update the examples so people are not surprised by the mismatch.

palmer-dabbelt commented 7 years ago

I was thinking it'd be better to have it the other way, so people aren't confused as to why we support that one (as it's more explicit in the spec that the new way is allowed). I'm going to do one of each -- how does this look?

michaeljclark commented 7 years ago

Is timebase-frequency for mtime in the CLINT?

The CLINT has msip and mtimecmp per hart but there is only a single mtime register.

frantony commented 7 years ago

@palmer-dabbelt

Please consider using clocks instead of timebase-frequency constant. Please see example from ar9331.dtsi:

        cpus {
                #address-cells = <1>;
                #size-cells = <0>;

                cpu@0 {
                        device_type = "cpu";
                        compatible = "mips,mips24Kc";
                        clocks = <&pll ATH79_CLK_CPU>;
                        reg = <0>;
                };
        };

clocks are widely used in linux kernel dts-files but neither Device Tree Specification Release 0.1 nor https://github.com/devicetree-org/devicetree-specification contain clocks definition.

terpstra commented 7 years ago

@michaeljclark yes.

@frantony The timebase-frequency is a required field in the DTS spec. I prefer we stick to the spec than copy MIPS. Especially since there is no 'clock' in the MIPS sense; the timer is part of the RISC-V platform spec.

terpstra commented 7 years ago

@palmer-dabbelt i like the one of each approach.