by Marc Carino

[permalink] [raw]

Subject: Re: [PATCH v5 6/8] ARM: brcmstb: add misc. DT bindings for brcmstb

Hi Mark,

>> +reboot
>> +-------
>> +Required properties
>> +
>> + - compatible
>> + The string property "brcm,brcmstb-reboot".
>> +
>> + - syscon
>> + A phandle / integer array that points to the syscon node which describes
>> + the general system reset registers.
>> + o a phandle to "sun_top_ctrl"
>> + o offset to the "reset source enable" register
>> + o offset to the "software master reset" register
>
> How variable are these values?

Very much so. Future chips will have different register maps. Because of this, Arnd
suggested that we use 'syscon' and 'regmap' to alleviate this maintenance burden.

>> +example:
>> + smpboot {
>> + compatible = "brcm,brcmstb-smpboot";
>> + syscon-cpu = <&hif_cpubiuctrl 0x88 0x178>;
>> + syscon-cont = <&hif_continuation>;
>> + };
>
> This looks odd. This doesn't seem like a device, but rather a grouping
> of disparate devices used for a particular software purpose.
>
>> +
>> +example:
>> + reboot {
>> + compatible = "brcm,brcmstb-reboot";
>> + syscon = <&sun_top_ctrl 0x304 0x308>;
>> + };
>
> As with smpboot, this seems odd.

Sure. Our H/W designers unfortunately didn't put the boot and restart registers into a
logical grouping, or standard register interface. Instead, they're all over the place.
How do you suggest naming the nodes to indicate this?

Thanks,
Marc C

On 01/24/2014 03:03 AM, Mark Rutland wrote:
> On Wed, Jan 22, 2014 at 03:30:50AM +0000, Marc Carino wrote:
>> Document the bindings that the Broadcom STB platform needs
>> for proper bootup.
>>
>> Signed-off-by: Marc Carino <[email protected]>
>> Acked-by: Florian Fainelli <[email protected]>
>> ---
>> .../devicetree/bindings/arm/brcm-brcmstb.txt | 95 ++++++++++++++++++++
>> 1 files changed, 95 insertions(+), 0 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/arm/brcm-brcmstb.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/brcm-brcmstb.txt b/Documentation/devicetree/bindings/arm/brcm-brcmstb.txt
>> new file mode 100644
>> index 0000000..3c436cc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/brcm-brcmstb.txt
>> @@ -0,0 +1,95 @@
>> +ARM Broadcom STB platforms Device Tree Bindings
>> +-----------------------------------------------
>> +Boards with Broadcom Brahma15 ARM-based BCMxxxx (generally BCM7xxx variants)
>> +SoC shall have the following DT organization:
>> +
>> +Required root node properties:
>> + - compatible: "brcm,bcm<chip_id>", "brcm,brcmstb"
>> +
>> +example:
>> +/ {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + model = "Broadcom STB (bcm7445)";
>> + compatible = "brcm,bcm7445", "brcm,brcmstb";
>> +
>> +Further, syscon nodes that map platform-specific registers used for general
>> +system control is required:
>> +
>> + - compatible: "brcm,bcm<chip_id>-sun-top-ctrl", "syscon"
>> + - compatible: "brcm,bcm<chip_id>-hif-cpubiuctrl", "syscon"
>> + - compatible: "brcm,bcm<chip_id>-hif-continuation", "syscon"
>> +
>> +example:
>> + rdb {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + compatible = "simple-bus";
>> + ranges = <0 0x00 0xf0000000 0x1000000>;
>> +
>> + sun_top_ctrl: syscon@404000 {
>> + compatible = "brcm,bcm7445-sun-top-ctrl", "syscon";
>> + reg = <0x404000 0x51c>;
>> + };
>> +
>> + hif_cpubiuctrl: syscon@3e2400 {
>> + compatible = "brcm,bcm7445-hif-cpubiuctrl", "syscon";
>> + reg = <0x3e2400 0x5b4>;
>> + };
>> +
>> + hif_continuation: syscon@452000 {
>> + compatible = "brcm,bcm7445-hif-continuation", "syscon";
>> + reg = <0x452000 0x100>;
>> + };
>> + };
>> +
>> +Lastly, nodes that allow for support of SMP initialization and reboot are
>> +required:
>> +
>> +smpboot
>> +-------
>> +Required properties:
>> +
>> + - compatible
>> + The string "brcm,brcmstb-smpboot".
>> +
>> + - syscon-cpu
>> + A phandle / integer array property which lets the BSP know the location
>> + of certain CPU power-on registers.
>> +
>> + The layout of the property is as follows:
>> + o a phandle to the "hif_cpubiuctrl" syscon node
>> + o offset to the base CPU power zone register
>> + o offset to the base CPU reset register
>
> How variable are these values?
>
>> +
>> + - syscon-cont
>> + A phandle pointing to the syscon node which describes the CPU boot
>> + continuation registers.
>> + o a phandle to the "hif_continuation" syscon node
>> +
>> +example:
>> + smpboot {
>> + compatible = "brcm,brcmstb-smpboot";
>> + syscon-cpu = <&hif_cpubiuctrl 0x88 0x178>;
>> + syscon-cont = <&hif_continuation>;
>> + };
>
> This looks odd. This doesn't seem like a device, but rather a grouping
> of disparate devices used for a particular software purpose.
>
>> +
>> +reboot
>> +-------
>> +Required properties
>> +
>> + - compatible
>> + The string property "brcm,brcmstb-reboot".
>> +
>> + - syscon
>> + A phandle / integer array that points to the syscon node which describes
>> + the general system reset registers.
>> + o a phandle to "sun_top_ctrl"
>> + o offset to the "reset source enable" register
>> + o offset to the "software master reset" register
>
> How variable are these values?
>
>> +
>> +example:
>> + reboot {
>> + compatible = "brcm,brcmstb-reboot";
>> + syscon = <&sun_top_ctrl 0x304 0x308>;
>> + };
>
> As with smpboot, this seems odd.
>
> Thanks,
> Mark.
>

2014-01-24 21:39:32

by Marc Carino

[permalink] [raw]

Subject: Re: [PATCH v5 8/8] ARM: brcmstb: dts: add a reference DTS for Broadcom 7445

Hi Mark,

> As I commented on v3 [1], these are contiguous and can be described with
> a single entry:
>
> memory {
> device_type = "memory";
> reg = <0x0 0x00000000 0x0 0xc0000000>;
> };
>
> Is there any reason to have three entries?

Oopsies, sorry for missing that.

On BCM7445 and derivatives, there are 3 memory controllers. For each memory controller,
the first 1GB of physical DRAM is mapped to:

* 0x00_0000_0000
* 0x00_4000_0000
* 0x00_8000_0000

The memory controllers aren't interleaved. So, it's possible for the SoC to have a
discontiguous memory-mapping, where a designer chooses not to populate physical DRAM in
the middle.

The 'reg' property was broken-up to have each chunk of memory given a dedicated memblock.

All that said, if you like, I can rework the patch as you've suggested.

Thanks,
Marc C

On 01/24/2014 03:09 AM, Mark Rutland wrote:
> On Wed, Jan 22, 2014 at 03:30:52AM +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 <[email protected]>
>> Acked-by: Florian Fainelli <[email protected]>
>> ---
>> arch/arm/boot/dts/bcm7445.dts | 111 +++++++++++++++++++++++++++++++++++++++++
>> 1 files changed, 111 insertions(+), 0 deletions(-)
>> create mode 100644 arch/arm/boot/dts/bcm7445.dts
>>
>> diff --git a/arch/arm/boot/dts/bcm7445.dts b/arch/arm/boot/dts/bcm7445.dts
>> new file mode 100644
>> index 0000000..ffa3305
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/bcm7445.dts
>> @@ -0,0 +1,111 @@
>> +/dts-v1/;
>> +/include/ "skeleton.dtsi"
>> +
>> +/ {
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + model = "Broadcom STB (bcm7445)";
>> + compatible = "brcm,bcm7445", "brcm,brcmstb";
>> + interrupt-parent = <&gic>;
>> +
>> + chosen {};
>> +
>> + memory {
>> + device_type = "memory";
>> + reg = <0x00 0x00000000 0x00 0x40000000>,
>> + <0x00 0x40000000 0x00 0x40000000>,
>> + <0x00 0x80000000 0x00 0x40000000>;
>> + };
>
> As I commented on v3 [1], these are contiguous and can be described with
> a single entry:
>
> memory {
> device_type = "memory";
> reg = <0x0 0x00000000 0x0 0xc0000000>;
> };
>
> Is there any reason to have three entries?
>
> Thanks,
> Mark.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/225899.html
>