[PATCH v2 1/3] arm64: dts: add initial dts for Samsung GH7 SoC and SSDK-GH7 board

[Mark Rutland]

On Fri, Mar 14, 2014 at 01:26:31AM +0000, Kukjin Kim wrote:
> > > > > +		cpu at 000 {
> > > > > +			device_type = "cpu";
> > > > > +			compatible = "arm,armv8";
> > > >
> > > > The "arm,armv8" should be a fallback rather than the only entry. The
> > > > precise core should be first (see arch/arm64/boot/dts/apm-storm.dtsi for
> > > > an example).
> > > >
> > > Well, do I really need another name if GH7 has just 'ARMv8 core' exactly?
> > 
> > Yes please. You presumably don't have "just 'ARMv8 core'", but rather a
> > specific ARMv8 implementation.
> > 
> OK, so in my understanding if it is really _just_ 'ARMv8 core' from S/W point
> of view, "arm,armv8" should be fine. I will make sure.

It will work, sure. But the compatible list should also list the
specific CPU. From the S/W point of view there are likely
implementation-defined registers which mean that no real CPU will be
"just" an ARMv8 core.

We have enough trouble with DTBs for 32-bit SoCs not listing things they
should. I see no reason to be lax here.

> > > > > +	gic: interrupt-controller at 1C000000 {
> > > > > +		compatible = "arm,cortex-a15-gic";
> > > >
> > > > We should have a more specific compatible string early in the list here
> > > > too.
> > > >
> > > Well, I think more specific compatible string is not required for gic, there
> > > were discussion about using another compatible string for gic-v2. And
> > > cortex-a15-gic should be fine at this moment and if another name is required
> > > as per previous discussion, we will then.
> > 
> > While it's probably ok to have "arm,cortex-a15-gic" in the compatible
> > list, it would be good to also have a more specific string, so we can
> > identify the GIC implementation precisely in future (in case we need to
> > perform any workarounds, or there's some kind of optimisation we could
> > perform for some implementations).
> > 
> Hmm... let's use just it for now then add another specific string later ;-)

Why not do this from the start? Then we can actually rely on the DTB
rather than it being useless.

> > > > > +	pmu {
> > > > > +		compatible = "arm,armv8-pmuv3";
> > > >
> > > > This is missing a specific compatible string.
> > > >
> > > I don't know why I need another specific compatible string here because I
> > > thought the 'armv8-pmuv3' is enough.
> > 
> > Each CPU microarchitecture has different PMU events and quirks. While
> > the CPU PMU might be compatible with ARMv8's PMUv3, it will also have
> > implementation-specific events, and may have quirks we need to work
> > around in future.
> > 
> > As with 32-bit, we'd expect these to be of the form
> > "${implementor},${cpu}-pmu".
> > 
> OK.
> 
> +	compatible = "samsung,gh7-pmu", "armv8-pmuv3";

Is GH7 an SoC or a CPU?

> > > > > +		serial at 12c20000 {
> > > > > +			compatible = "arm,pl011", "arm,primecell";
> > > > > +			reg = <0 0x12c20000 0 0x10000>;
> > > > > +			interrupts = <0 420 0>;
> > > > > +		};
> > > >
> > > > These are both missing required clocks.
> > > >
> > > Yes right.
> > 
> > So you'll add these?
> > 
> To be honest, still I'm not sure how it should be handled because regarding
> clocks do not need to handle in the kernel for GH7. And for it, I needed to
> use some HACKs to change clock handling in the kernel. I will provide proper
> solution soon.

Are the clocks always-on, fixed-rate clocks, or does some firmware
manage them dynamically? We have bindings for the former.

There's not much point having a DTS upstream that relies on hacks which
are not.

Cheers,
Mark.