linux-kernel - Re: [PATCH v3 7/7] ARM: brcmstb: dts: add a reference DTS for Broadcom 7445

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 15 Jan 2014 17:40:33 +0000
From:	Mark Rutland <mark.rutland@....com>
To:	Marc Carino <marc.ceeeee@...il.com>
Cc:	Christian Daudt <bcm@...thebug.org>, Arnd Bergmann <arnd@...db.de>,
	Florian Fainelli <f.fainelli@...il.com>,
	Matt Porter <matt.porter@...aro.org>,
	Russell King <linux@....linux.org.uk>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v3 7/7] ARM: brcmstb: dts: add a reference DTS for
 Broadcom 7445

On Tue, Jan 14, 2014 at 11:48:53PM +0000, Marc Carino wrote:
> Add a sample DTS which will allow bootup of a board populated
> with the BCM7445 chip.
> 
> Signed-off-by: Marc Carino <marc.ceeeee@...il.com>
> Acked-by: Florian Fainelli <f.fainelli@...il.com>
> ---
>  arch/arm/boot/dts/brcmstb-7445.dts |  104 ++++++++++++++++++++++++++++++++++++
>  1 files changed, 104 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/boot/dts/brcmstb-7445.dts
> 
> diff --git a/arch/arm/boot/dts/brcmstb-7445.dts b/arch/arm/boot/dts/brcmstb-7445.dts
> new file mode 100644
> index 0000000..cbe73b4
> --- /dev/null
> +++ b/arch/arm/boot/dts/brcmstb-7445.dts
> @@ -0,0 +1,104 @@
> +/dts-v1/;
> +/include/ "skeleton.dtsi"
> +
> +/ {
> +	#address-cells = <0x1>;
> +	#size-cells = <0x1>;

I see that there are some Xen patches floating around for the B15, so
presumably you have LPAE (and therefore can have physical addresses
wider than 32-bits).

Are all peripherals and memory in the bottom 4GB of the physical address
space? If not, I'd recommend you bump #address-cells (and #size-cells)
to <2> now, or you'll need to rewrite half the DT to add currently
missing peripherals...

Also, unless you're writing an address or mask it's usually nicer to
have the value in decimal rather than hex. Please could you get rid of
the 0x prefix on the #address-cells, #size-cells, #clock-cells values?

It's a minor issue, but it's nicer for humans to read...

> +	model = "Broadcom STB (7445)";
> +	compatible = "brcm,brcmstb-7445";
> +	interrupt-parent = <&gic>;
> +
> +	chosen {};
> +
> +	memory {
> +		device_type = "memory";
> +		reg = <0x0 0x40000000 0x40000000 0x40000000 0x80000000 0x40000000>;
> +	};

As a general note, where you have list properties, please bracket
elements individually (here and elsewhere) so it's possible to read,
e.g:

	reg = <0x00000000 0x40000000>,
	      <0x40000000 0x40000000>,
	      <0x80000000 0x40000000>;

In this case, the memory is contiguous, so you can describe it with one
reg entry:

	reg = <0x0 0xc0000000>;

> +
> +	cpupll: cpupll@0 {
> +		#clock-cells = <0x0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <1500000000>;
> +	};

The unit-address (the "@0") should go, as this doesn't have a reg entry.
same for the cpu-clk-div@0 node below:

> +
> +	cpuclk: cpu-clk-div@0 {
> +		#clock-cells = <0x0>;
> +		compatible = "brcm,brcmstb-cpu-clk-div";
> +		reg = <0xf03e257c 0x4>;
> +		clocks = <&cpupll>;
> +		div-table = <0x0 0x1 0x11 0x2 0x12 0x4 0x13 0x8 0x14 0x10>;
> +	};

The unit-address here should be f03e257c (or 0,f03e257c if you move to
#address-cells = <2>).

Is this binding documented anywhere? It doesn't seem to be added in this
series, and grep on mainline gave me nothing...

> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu@0 {
> +			compatible = "brcm,brahma15";

I think this string should change, as mentioned in my cpu binding
patch comments.

> +			operating-points = <0x16e360 0x0
> +					    0x0b71b0 0x0
> +					    0x05b8d8 0x0
> +					    0x02dc6c 0x0
> +					    0x016e36 0x0>;

These might be easier to read in decimal...

> +			clocks = <&cpuclk>;
> +			device_type = "cpu";
> +			reg = <0>;
> +			clock-frequency = <1500000000>;
> +		};
> +
> +		cpu@1 {
> +			compatible = "brcm,brahma15";
> +			device_type = "cpu";
> +			reg = <1>;
> +			clock-frequency = <1500000000>;
> +		};
> +
> +		cpu@2 {
> +			compatible = "brcm,brahma15";
> +			device_type = "cpu";
> +			reg = <2>;
> +			clock-frequency = <1500000000>;
> +		};
> +
> +		cpu@3 {
> +			compatible = "brcm,brahma15";
> +			device_type = "cpu";
> +			reg = <3>;
> +			clock-frequency = <1500000000>;
> +		};
> +	};
> +
> +	gic: interrupt-controller@...00000 {
> +		compatible = "brcm,brahma15-gic", "arm,cortex-a15-gic";
> +		reg = <0xffd01000 0x1000
> +		       0xffd02000 0x2000>;

I see you have Xen patches, and therefore presumably have a GIC with
virtualization features. Could you add the missing reg entries for the
virtual control and virtual cpu interface registers please?

Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/