diff mbox

[11/11] ARM64: dts: Prepare Actions Semi S900 and Bubblegum-96

Message ID 267b586e-06ce-f123-b7cc-dc0feb880ca9@suse.de
State New
Headers show

Commit Message

Andreas Färber Feb. 15, 2017, 6:14 p.m. UTC 
Hi,

Am 15.02.2017 um 18:12 schrieb Mark Rutland:
> On Wed, Feb 15, 2017 at 05:55:28PM +0100, Andreas Färber wrote:
>> +#include "s900.dtsi"
>> +
>> +/ {
>> +	compatible = "ucrobotics,bubblegum-96", "acts,s900";
>> +	model = "Bubblegum-96";
>> +
>> +	aliases {
>> +		serial5 = &uart5;
>> +	};
>> +
>> +	chosen {
>> +		stdout-path = "serial5:115200n8";
>> +	};
>> +};
> 
> I didn't spot a memory node here or in the dtsi. Does the FW/bootloader
> create one?

Heh, apparently... For S500, skeleton.dtsi added that - not entirely
clean either. I'll add one here in the .dts and think about adding one
in s500-guitar.dtsi as well.


>> +
>> +	psci {
>> +		compatible = "arm,psci-0.2";
>> +		method = "smc";
>> +	};
>> +
>> +	arm-pmu {
>> +		compatible = "arm,cortex-a53-pmu";
>> +		interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
>> +		             <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
>> +		             <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
>> +		             <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
>> +	};
> 
> Please ad an interrupt-affinity property, as described in
> Documentation/devicetree/bindings/arm/pmu.txt.

I had done my best to check my properties matched the bindings, but you
caught me not checking for any missing properties.

Again, my guess is:

@@ -99,6 +104,7 @@
                             <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
                             <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
                             <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+               interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>;
        };

        timer {

>> +
>> +	timer {
>> +		compatible = "arm,armv8-timer";
>> +		interrupts = <GIC_PPI 13
>> +			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 14
>> +			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 11
>> +			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>> +			     <GIC_PPI 10
>> +			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>> +	};
>> +
>> +	soc {
>> +		compatible = "simple-bus";
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges;
>> +
>> +		gic: interrupt-controller@e00f1000 {
>> +			compatible = "arm,gic-400";
>> +			reg = <0x0 0xe00f1000 0x0 0x1000>,
>> +			      <0x0 0xe00f2000 0x0 0x1000>,
>> +			      <0x0 0xe00f4000 0x0 0x2000>,
>> +			      <0x0 0xe00f6000 0x0 0x2000>;
> 
> I believe that the second entry should be 0x2000 in length.

The vendor tree does have 0x1000, but that might be a mistake of course.
I vaguely recall having such discussions elsewhere in the past.

@@ -122,7 +128,7 @@
                gic: interrupt-controller@e00f1000 {
                        compatible = "arm,gic-400";
                        reg = <0x0 0xe00f1000 0x0 0x1000>,
-                             <0x0 0xe00f2000 0x0 0x1000>,
+                             <0x0 0xe00f2000 0x0 0x2000>,
                              <0x0 0xe00f4000 0x0 0x2000>,
                              <0x0 0xe00f6000 0x0 0x2000>;
                        interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4)
| IRQ_TYPE_LEVEL_LOW)>;

Thanks for the quick review,

Andreas

Comments

Mark Rutland Feb. 15, 2017, 6:23 p.m. UTC | #1 
Hi,

On Wed, Feb 15, 2017 at 07:14:14PM +0100, Andreas Färber wrote:
> Am 15.02.2017 um 18:12 schrieb Mark Rutland:
> > On Wed, Feb 15, 2017 at 05:55:28PM +0100, Andreas Färber wrote:
> >> +	reserved-memory {
> >> +		#address-cells = <2>;
> >> +		#size-cells = <2>;
> >> +		ranges;
> >> +	};
> > 
> > Why have this empty node?
> 
> Forgotten about... I see these:
> 
>    reserving fdt memory region: addr=19000000 size=3000000
>    reserving fdt memory region: addr=20000000 size=20000000
>    reserving fdt memory region: addr=18fff000 size=1000
>    reserving fdt memory region: addr=1f000000 size=1000000
> 
> Vendor tree says 0x1f000000 is for secure monitor; 0x19000000 for fb,
> 0x20000000 for ion (ignoring), and 0x20000000 for afinfo - whatever that
> may be (not me! :)).
> 
> So as minimum:
> 
> --- a/arch/arm64/boot/dts/actions/s900.dtsi
> +++ b/arch/arm64/boot/dts/actions/s900.dtsi
> @@ -86,6 +86,11 @@
>                 #address-cells = <2>;
>                 #size-cells = <2>;
>                 ranges;
> +
> +               secmon@1f000000 {
> +                       reg = <0x0 0x1f000000 0x0 0x1000000>;
> +                       no-map;
> +               };
>         };
> 
>         psci {

Once you've added that node, could you try booting with "memtest" on the
command line?

Assuming CONFIG_MEMTEST is enabled, that'll go and verify that the
kernel can access all of the (unresereved) memory without issues, which
should save us lots of pain later on if there's something else that
needs to be reserved.

> >> +	timer {
> >> +		compatible = "arm,armv8-timer";
> >> +		interrupts = <GIC_PPI 13
> >> +			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> >> +			     <GIC_PPI 14
> >> +			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> >> +			     <GIC_PPI 11
> >> +			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> >> +			     <GIC_PPI 10
> >> +			(GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
> >> +	};
> >> +
> >> +	soc {
> >> +		compatible = "simple-bus";
> >> +		#address-cells = <2>;
> >> +		#size-cells = <2>;
> >> +		ranges;
> >> +
> >> +		gic: interrupt-controller@e00f1000 {
> >> +			compatible = "arm,gic-400";
> >> +			reg = <0x0 0xe00f1000 0x0 0x1000>,
> >> +			      <0x0 0xe00f2000 0x0 0x1000>,
> >> +			      <0x0 0xe00f4000 0x0 0x2000>,
> >> +			      <0x0 0xe00f6000 0x0 0x2000>;
> > 
> > I believe that the second entry should be 0x2000 in length.
> 
> The vendor tree does have 0x1000, but that might be a mistake of course.
> I vaguely recall having such discussions elsewhere in the past.

It is a mistake, and a common one [1] at that! :)

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-January/480590.html
diff mbox

Patch

--- a/arch/arm64/boot/dts/actions/s900-bubblegum96.dts
+++ b/arch/arm64/boot/dts/actions/s900-bubblegum96.dts
@@ -55,6 +55,11 @@ 
        chosen {
                stdout-path = "serial5:115200n8";
        };
+
+       memory {
+               device_type = "memory";
+               reg = <0x0 0x0 0x0 0x80000000>;
+       };
 };

 &uart5 {

>> +	reserved-memory {
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges;
>> +	};
> 
> Why have this empty node?

Forgotten about... I see these:

   reserving fdt memory region: addr=19000000 size=3000000
   reserving fdt memory region: addr=20000000 size=20000000
   reserving fdt memory region: addr=18fff000 size=1000
   reserving fdt memory region: addr=1f000000 size=1000000

Vendor tree says 0x1f000000 is for secure monitor; 0x19000000 for fb,
0x20000000 for ion (ignoring), and 0x20000000 for afinfo - whatever that
may be (not me! :)).

So as minimum:

--- a/arch/arm64/boot/dts/actions/s900.dtsi
+++ b/arch/arm64/boot/dts/actions/s900.dtsi
@@ -86,6 +86,11 @@ 
                #address-cells = <2>;
                #size-cells = <2>;
                ranges;
+
+               secmon@1f000000 {
+                       reg = <0x0 0x1f000000 0x0 0x1000000>;
+                       no-map;
+               };
        };

        psci {