Re: [PATCH V2 2/3] ARM: dts: add dts files for exynos5260 SoC

Re: [PATCH V2 2/3] ARM: dts: add dts files for exynos5260 SoC

[Tomasz Figa]

Hi Rahul, Pankaj, Arun,

[adding linux-arm-kernel, devicetree MLs and DT people on Cc]

I think it's good time to stop accepting DTS files like this and force 
new ones to use the proper structure with soc node, labels for every 
node and node references.

In case of previous Exynos 5 SoCs I hadn't complained, because they 
shared a lot of data with already existing exynos5.dtsi, but since 
Exynos5260 is completely different, I'd say it should be converted to 
the new layout.

As an example you can look at arch/arm/boot/dts/s3c64xx.dtsi and files 
that include it or, for more complete structures, DTS of other 
platforms, such as imx6*.

Btw. Please remember that linux-samsung-soc mailing list is just a 
convenient utility for reviewers of Samsung-specific patches to have all 
of them in one place. Sending patches to it alone is not enough - a 
general kernel ML list needs to be CCed as well, in this case 
linux-arm-kernel.

Also, please see my comments inline, for review comments unrelated to 
the issue described above.

On 05.02.2014 06:16, Rahul Sharma wrote:
> The patch adds the dts files for exynos5260.
>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> ---
>   arch/arm/boot/dts/exynos5260-pinctrl.dtsi |  572 +++++++++++++++++++++++++++++
>   arch/arm/boot/dts/exynos5260.dtsi         |  317 ++++++++++++++++
>   2 files changed, 889 insertions(+)
>   create mode 100644 arch/arm/boot/dts/exynos5260-pinctrl.dtsi
>   create mode 100644 arch/arm/boot/dts/exynos5260.dtsi
>
> diff --git a/arch/arm/boot/dts/exynos5260-pinctrl.dtsi b/arch/arm/boot/dts/exynos5260-pinctrl.dtsi
> new file mode 100644
> index 0000000..3f2c5c4
> --- /dev/null
> +++ b/arch/arm/boot/dts/exynos5260-pinctrl.dtsi
> @@ -0,0 +1,572 @@
> +/*
> + * Samsung's Exynos5260 SoC pin-mux and pin-config device tree source
> + *
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + *		http://www.samsung.com
> + *
> + * Samsung's Exynos5260 SoC pin-mux and pin-config options are listed as device
> + * tree nodes are listed in this file.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +/ {
> +	pinctrl@11600000 {

[snip]

> +		spi0_bus: spi0-bus {
> +			samsung,pins = "gpa2-0", "gpa2-1", "gpa2-2", "gpa2-3";

What is the reason for SPI0 to have 4 pins, while SPI1 has just 3?

> +			samsung,pin-function = <2>;
> +			samsung,pin-pud = <3>;
> +			samsung,pin-drv = <0>;
> +		};

[snip]

> diff --git a/arch/arm/boot/dts/exynos5260.dtsi b/arch/arm/boot/dts/exynos5260.dtsi
> new file mode 100644
> index 0000000..32a95c7
> --- /dev/null
> +++ b/arch/arm/boot/dts/exynos5260.dtsi
> @@ -0,0 +1,317 @@
> +/*
> + * SAMSUNG EXYNOS5260 SoC device tree source
> + *
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + *		http://www.samsung.com
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include "skeleton.dtsi"
> +#include "exynos5260-pinctrl.dtsi"
> +
> +#include <dt-bindings/clk/exynos5260-clk.h>
> +
> +/ {
> +	compatible = "samsung,exynos5260";
> +	interrupt-parent = <&gic>;
> +
> +	aliases {
> +		pinctrl0 = &pinctrl_0;
> +		pinctrl1 = &pinctrl_1;
> +		pinctrl2 = &pinctrl_2;
> +	};
> +
> +	chipid@10000000 {
> +		compatible = "samsung,exynos4210-chipid";
> +		reg = <0x10000000 0x100>;
> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cpu@0 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a15";
> +			reg = <0>;

nit: Please make this consistent with CPUs 10x below, by using hex here 
as well.

> +			cci-control-port = <&cci_control1>;
> +		};

nit: Please keep 1 blank line of spacing between nodes.

> +		cpu@1 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a15";
> +			reg = <1>;
> +			cci-control-port = <&cci_control1>;
> +		};
> +		cpu@100 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0x100>;
> +			cci-control-port = <&cci_control0>;
> +		};
> +		cpu@101 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0x101>;
> +			cci-control-port = <&cci_control0>;
> +		};
> +		cpu@102 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0x102>;
> +			cci-control-port = <&cci_control0>;
> +		};
> +		cpu@103 {
> +			device_type = "cpu";
> +			compatible = "arm,cortex-a7";
> +			reg = <0x103>;
> +			cci-control-port = <&cci_control0>;
> +		};
> +	};
> +
> +	cmus {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +

I don't think there is a need to group these nodes under a parent node 
that doesn't give any additional information, especially when the CMUs 
are scattered trough the whole address space, while we'd like to keep 
the nodes ordered by their addresses, as most platforms do.

> +		cmu_top: clock-controller@10010000 {
> +			compatible = "exynos5260-cmu-top", "samsung,exynos5260-clock";

I don't think that having the "samsung,exynos5260-clock" compatible 
string for every CMU is appropriate here, because there is no way to 
automatically recognize which CMU it is. Since every CMU instance is 
different, they need to have different compatible strings.

> +			reg = <0x10010000 0x10000>;
> +			#clock-cells = <1>;
> +		};

[snip]

> +	mct@100B0000 {
> +		compatible = "samsung,exynos4210-mct";
> +		reg = <0x100B0000 0x1000>;
> +		interrupt-controller;
> +		#interrups-cells = <1>;

MCT is not an interrupt controller, so the 2 properties above are incorrect.

> +		interrupt-parent = <&mct_map>;
> +		interrupts = <0>, <1>, <2>, <3>,
> +				<4>, <5>, <6>, <7>,
> +				<8>, <9>, <10>, <11>;
> +		clocks = <&cmu_top FIN_PLL>, <&cmu_peri PERI_CLK_MCT>;
> +		clock-names = "fin_pll", "mct";
> +
> +		mct_map: mct-map {
> +			#interrupt-cells = <1>;
> +			#address-cells = <0>;
> +			#size-cells = <0>;
> +			interrupt-map = <0 &gic 0 104 0>,
> +					<1 &gic 0 105 0>,
> +					<2 &gic 0 106 0>,
> +					<3 &gic 0 107 0>,
> +					<4 &gic 0 122 0>,
> +					<5 &gic 0 123 0>,
> +					<6 &gic 0 124 0>,
> +					<7 &gic 0 125 0>,
> +					<8 &gic 0 126 0>,
> +					<9 &gic 0 127 0>,
> +					<10 &gic 0 128 0>,
> +					<11 &gic 0 129 0>;
> +		};

There is no need for interrupt-map here, because all the interrupts are 
from GIC.

> +	};

[snip]

> +	mmc_0: mmc0@12140000 {
> +		compatible = "samsung,exynos5250-dw-mshc";
> +		reg = <0x12140000 0x2000>;
> +		interrupts = <0 156 0>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clocks = <&cmu_fsys FSYS_CLK_MMC0>, <&cmu_top TOP_SCLK_MMC0>;
> +		clock-names = "biu", "ciu";
> +		fifo-depth = <0x40>;

nit: It might be more readable to use decimal 64 here.

Best regards,
Tomasz

Re: [PATCH V2 2/3] ARM: dts: add dts files for exynos5260 SoC

[Rahul Sharma]

Hi Tomasz,

On 6 February 2014 18:51, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Rahul, Pankaj, Arun,
>
> [adding linux-arm-kernel, devicetree MLs and DT people on Cc]
>
> I think it's good time to stop accepting DTS files like this and force new
> ones to use the proper structure with soc node, labels for every node and
> node references.

I am unable to find information on SoC node and grouping inside SoC node. Please
share some pointers.

>
> In case of previous Exynos 5 SoCs I hadn't complained, because they shared a
> lot of data with already existing exynos5.dtsi, but since Exynos5260 is
> completely different, I'd say it should be converted to the new layout.
>
> As an example you can look at arch/arm/boot/dts/s3c64xx.dtsi and files that
> include it or, for more complete structures, DTS of other platforms, such as
> imx6*.
>
> Btw. Please remember that linux-samsung-soc mailing list is just a
> convenient utility for reviewers of Samsung-specific patches to have all of
> them in one place. Sending patches to it alone is not enough - a general
> kernel ML list needs to be CCed as well, in this case linux-arm-kernel.
>

I will take care of this.

> Also, please see my comments inline, for review comments unrelated to the
> issue described above.
>
>
> On 05.02.2014 06:16, Rahul Sharma wrote:
>>
>> The patch adds the dts files for exynos5260.
>>
>> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
>> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
>> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
>> ---
>>   arch/arm/boot/dts/exynos5260-pinctrl.dtsi |  572
>> +++++++++++++++++++++++++++++
>>   arch/arm/boot/dts/exynos5260.dtsi         |  317 ++++++++++++++++
>>   2 files changed, 889 insertions(+)
>>   create mode 100644 arch/arm/boot/dts/exynos5260-pinctrl.dtsi
>>   create mode 100644 arch/arm/boot/dts/exynos5260.dtsi
>>
>> diff --git a/arch/arm/boot/dts/exynos5260-pinctrl.dtsi
>> b/arch/arm/boot/dts/exynos5260-pinctrl.dtsi
>> new file mode 100644
>> index 0000000..3f2c5c4
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/exynos5260-pinctrl.dtsi
>> @@ -0,0 +1,572 @@
>> +/*
>> + * Samsung's Exynos5260 SoC pin-mux and pin-config device tree source
>> + *
>> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
>> + *             http://www.samsung.com
>> + *
>> + * Samsung's Exynos5260 SoC pin-mux and pin-config options are listed as
>> device
>> + * tree nodes are listed in this file.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> +*/
>> +
>> +/ {
>> +       pinctrl@11600000 {
>
>
> [snip]
>
>
>> +               spi0_bus: spi0-bus {
>> +                       samsung,pins = "gpa2-0", "gpa2-1", "gpa2-2",
>> "gpa2-3";
>
>
> What is the reason for SPI0 to have 4 pins, while SPI1 has just 3?
>

I should align SPI1 with SPI0.

>
>> +                       samsung,pin-function = <2>;
>> +                       samsung,pin-pud = <3>;
>> +                       samsung,pin-drv = <0>;
>> +               };
>
>
> [snip]
>
>
>> diff --git a/arch/arm/boot/dts/exynos5260.dtsi
>> b/arch/arm/boot/dts/exynos5260.dtsi
>> new file mode 100644
>> index 0000000..32a95c7
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/exynos5260.dtsi
>> @@ -0,0 +1,317 @@
>> +/*
>> + * SAMSUNG EXYNOS5260 SoC device tree source
>> + *
>> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
>> + *             http://www.samsung.com
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> +*/
>> +
>> +#include "skeleton.dtsi"
>> +#include "exynos5260-pinctrl.dtsi"
>> +
>> +#include <dt-bindings/clk/exynos5260-clk.h>
>> +
>> +/ {
>> +       compatible = "samsung,exynos5260";
>> +       interrupt-parent = <&gic>;
>> +
>> +       aliases {
>> +               pinctrl0 = &pinctrl_0;
>> +               pinctrl1 = &pinctrl_1;
>> +               pinctrl2 = &pinctrl_2;
>> +       };
>> +
>> +       chipid@10000000 {
>> +               compatible = "samsung,exynos4210-chipid";
>> +               reg = <0x10000000 0x100>;
>> +       };
>> +
>> +       cpus {
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +
>> +               cpu@0 {
>> +                       device_type = "cpu";
>> +                       compatible = "arm,cortex-a15";
>> +                       reg = <0>;
>
>
> nit: Please make this consistent with CPUs 10x below, by using hex here as
> well.
>

Done.

>
>> +                       cci-control-port = <&cci_control1>;
>> +               };
>
>
> nit: Please keep 1 blank line of spacing between nodes.
>

ok.

>
>> +               cpu@1 {
>> +                       device_type = "cpu";
>> +                       compatible = "arm,cortex-a15";
>> +                       reg = <1>;
>> +                       cci-control-port = <&cci_control1>;
>> +               };
>> +               cpu@100 {
>> +                       device_type = "cpu";
>> +                       compatible = "arm,cortex-a7";
>> +                       reg = <0x100>;
>> +                       cci-control-port = <&cci_control0>;
>> +               };
>> +               cpu@101 {
>> +                       device_type = "cpu";
>> +                       compatible = "arm,cortex-a7";
>> +                       reg = <0x101>;
>> +                       cci-control-port = <&cci_control0>;
>> +               };
>> +               cpu@102 {
>> +                       device_type = "cpu";
>> +                       compatible = "arm,cortex-a7";
>> +                       reg = <0x102>;
>> +                       cci-control-port = <&cci_control0>;
>> +               };
>> +               cpu@103 {
>> +                       device_type = "cpu";
>> +                       compatible = "arm,cortex-a7";
>> +                       reg = <0x103>;
>> +                       cci-control-port = <&cci_control0>;
>> +               };
>> +       };
>> +
>> +       cmus {
>> +               #address-cells = <1>;
>> +               #size-cells = <1>;
>> +               ranges;
>> +
>
>
> I don't think there is a need to group these nodes under a parent node that
> doesn't give any additional information, especially when the CMUs are
> scattered trough the whole address space, while we'd like to keep the nodes
> ordered by their addresses, as most platforms do.
>

This is exactly the same case as "cpus". I mean, "cpus" also doesn't provide
any common information about child cpu nodes. This looks to me as a logical
grouping and I have implemented same thing for cmu nodes.
I am ok with removing this grouping Just want to understand the rational behind
grouping cpus which seems similar to cmus.

Similarly "soc" is just a logical entity used to group SoC elements which looks
optional to me. What are we achieving with this? Please help me in understanding
this better.

>
>> +               cmu_top: clock-controller@10010000 {
>> +                       compatible = "exynos5260-cmu-top",
>> "samsung,exynos5260-clock";
>
>
> I don't think that having the "samsung,exynos5260-clock" compatible string
> for every CMU is appropriate here, because there is no way to automatically
> recognize which CMU it is. Since every CMU instance is different, they need
> to have different compatible strings.
>

Changed.

>
>> +                       reg = <0x10010000 0x10000>;
>> +                       #clock-cells = <1>;
>> +               };
>
>
> [snip]
>
>
>> +       mct@100B0000 {
>> +               compatible = "samsung,exynos4210-mct";
>> +               reg = <0x100B0000 0x1000>;
>> +               interrupt-controller;
>> +               #interrups-cells = <1>;
>
>
> MCT is not an interrupt controller, so the 2 properties above are incorrect.
>

Agree. I will change this.

>
>> +               interrupt-parent = <&mct_map>;
>> +               interrupts = <0>, <1>, <2>, <3>,
>> +                               <4>, <5>, <6>, <7>,
>> +                               <8>, <9>, <10>, <11>;
>> +               clocks = <&cmu_top FIN_PLL>, <&cmu_peri PERI_CLK_MCT>;
>> +               clock-names = "fin_pll", "mct";
>> +
>> +               mct_map: mct-map {
>> +                       #interrupt-cells = <1>;
>> +                       #address-cells = <0>;
>> +                       #size-cells = <0>;
>> +                       interrupt-map = <0 &gic 0 104 0>,
>> +                                       <1 &gic 0 105 0>,
>> +                                       <2 &gic 0 106 0>,
>> +                                       <3 &gic 0 107 0>,
>> +                                       <4 &gic 0 122 0>,
>> +                                       <5 &gic 0 123 0>,
>> +                                       <6 &gic 0 124 0>,
>> +                                       <7 &gic 0 125 0>,
>> +                                       <8 &gic 0 126 0>,
>> +                                       <9 &gic 0 127 0>,
>> +                                       <10 &gic 0 128 0>,
>> +                                       <11 &gic 0 129 0>;
>> +               };
>
>
> There is no need for interrupt-map here, because all the interrupts are from
> GIC.
>

Done.

Regards,
Rahul Sharma.


>> +       };
>
>
> [snip]
>
>
>> +       mmc_0: mmc0@12140000 {
>> +               compatible = "samsung,exynos5250-dw-mshc";
>> +               reg = <0x12140000 0x2000>;
>> +               interrupts = <0 156 0>;
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +               clocks = <&cmu_fsys FSYS_CLK_MMC0>, <&cmu_top
>> TOP_SCLK_MMC0>;
>> +               clock-names = "biu", "ciu";
>> +               fifo-depth = <0x40>;
>
>
> nit: It might be more readable to use decimal 64 here.
>
> Best regards,
> Tomasz

Re: [PATCH V2 2/3] ARM: dts: add dts files for exynos5260 SoC

[Tomasz Figa]

Hi Rahul,

On 11.02.2014 06:22, Rahul Sharma wrote:
> Hi Tomasz,
>
> On 6 February 2014 18:51, Tomasz Figa <t.figa@samsung.com> wrote:
>> Hi Rahul, Pankaj, Arun,
>>
>> [adding linux-arm-kernel, devicetree MLs and DT people on Cc]
>>
>> I think it's good time to stop accepting DTS files like this and force new
>> ones to use the proper structure with soc node, labels for every node and
>> node references.
>
> I am unable to find information on SoC node and grouping inside SoC node. Please
> share some pointers.

Well, there is not much information needed about this. Basically all the 
devices built into the SoC should be listed under respective bus nodes 
or a single soc node, instead of root level. Such node should be a 
"simple-bus" and just group the components together to separate 
board-specific devices (which are still at root level) from SoC devices.

Even though it might seem useless, it improves DT readability a bit and 
still most of the platforms use this approach, so for consistency, 
Exynos should use too.

Just for reference, back in April 2013, in his review of S3C64xx DT 
series [1], Rob Herring requested that we don't submit any new device 
trees using flat approach and start using bus hierarchy.

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/163659.html

>>
>>> +               spi0_bus: spi0-bus {
>>> +                       samsung,pins = "gpa2-0", "gpa2-1", "gpa2-2",
>>> "gpa2-3";
>>
>>
>> What is the reason for SPI0 to have 4 pins, while SPI1 has just 3?
>>
>
> I should align SPI1 with SPI0.
>

Are you sure that SPI0 is the correct one? SPI usually uses four pins - 
SDI, SDO, SCK and nCS, but we always used to treat nCS as a simple GPIO, 
due to the fact that the controller can only support one dedicated chip 
select and with direct GPIO control you can have more.

What is the fourth pin here?

>>
>>> +               cpu@1 {
>>> +                       device_type = "cpu";
>>> +                       compatible = "arm,cortex-a15";
>>> +                       reg = <1>;
>>> +                       cci-control-port = <&cci_control1>;
>>> +               };
>>> +               cpu@100 {
>>> +                       device_type = "cpu";
>>> +                       compatible = "arm,cortex-a7";
>>> +                       reg = <0x100>;
>>> +                       cci-control-port = <&cci_control0>;
>>> +               };
>>> +               cpu@101 {
>>> +                       device_type = "cpu";
>>> +                       compatible = "arm,cortex-a7";
>>> +                       reg = <0x101>;
>>> +                       cci-control-port = <&cci_control0>;
>>> +               };
>>> +               cpu@102 {
>>> +                       device_type = "cpu";
>>> +                       compatible = "arm,cortex-a7";
>>> +                       reg = <0x102>;
>>> +                       cci-control-port = <&cci_control0>;
>>> +               };
>>> +               cpu@103 {
>>> +                       device_type = "cpu";
>>> +                       compatible = "arm,cortex-a7";
>>> +                       reg = <0x103>;
>>> +                       cci-control-port = <&cci_control0>;
>>> +               };
>>> +       };
>>> +
>>> +       cmus {
>>> +               #address-cells = <1>;
>>> +               #size-cells = <1>;
>>> +               ranges;
>>> +
>>
>>
>> I don't think there is a need to group these nodes under a parent node that
>> doesn't give any additional information, especially when the CMUs are
>> scattered trough the whole address space, while we'd like to keep the nodes
>> ordered by their addresses, as most platforms do.
>>
>
> This is exactly the same case as "cpus". I mean, "cpus" also doesn't provide
> any common information about child cpu nodes. This looks to me as a logical
> grouping and I have implemented same thing for cmu nodes.
> I am ok with removing this grouping Just want to understand the rational behind
> grouping cpus which seems similar to cmus.

The "cpus" node is a defined standard node that should be present at 
root of device tree and include subnodes for all CPUs. This is a 
standard binding defined for low level code to be able to simply find 
nodes of all CPUs in the system - so they can expect that at /cpus node 
all the subnodes are subsequent CPUs.

> Similarly "soc" is just a logical entity used to group SoC elements which looks
> optional to me. What are we achieving with this? Please help me in understanding
> this better.

Also "soc" has a slightly wider meaning. It is a node grouping all nodes 
from a single address space - the node specifies #address-cells and 
#size-cells of this address space and all the devices under this 
"simple-bus" can be accessed using addresses in this format. In 
addition, it separates board-level devices from generic SoC devices.

Now, in case of "cmus", the only purpose is to group all CMU nodes 
together and, while this improves readability a bit, it doesn't make the 
DT better express the hardware topology, because the CMUs in the 
hardware are in fact scattered through the whole address space, not 
under a contiguous block of it, as the grouping would suggest.

Best regards,
Tomasz

Re: [PATCH V2 2/3] ARM: dts: add dts files for exynos5260 SoC

[Rahul Sharma]

Thanks Tomasz,

I will add these changes to v3.

Regards,
Rahul Sharma.

On 11 February 2014 15:34, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Rahul,
>
>
> On 11.02.2014 06:22, Rahul Sharma wrote:
>>
>> Hi Tomasz,
>>
>> On 6 February 2014 18:51, Tomasz Figa <t.figa@samsung.com> wrote:
>>>
>>> Hi Rahul, Pankaj, Arun,
>>>
>>> [adding linux-arm-kernel, devicetree MLs and DT people on Cc]
>>>
>>> I think it's good time to stop accepting DTS files like this and force
>>> new
>>> ones to use the proper structure with soc node, labels for every node and
>>> node references.
>>
>>
>> I am unable to find information on SoC node and grouping inside SoC node.
>> Please
>> share some pointers.
>
>
> Well, there is not much information needed about this. Basically all the
> devices built into the SoC should be listed under respective bus nodes or a
> single soc node, instead of root level. Such node should be a "simple-bus"
> and just group the components together to separate board-specific devices
> (which are still at root level) from SoC devices.
>
> Even though it might seem useless, it improves DT readability a bit and
> still most of the platforms use this approach, so for consistency, Exynos
> should use too.
>
> Just for reference, back in April 2013, in his review of S3C64xx DT series
> [1], Rob Herring requested that we don't submit any new device trees using
> flat approach and start using bus hierarchy.
>
> [1]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/163659.html
>
>
>>>
>>>> +               spi0_bus: spi0-bus {
>>>> +                       samsung,pins = "gpa2-0", "gpa2-1", "gpa2-2",
>>>> "gpa2-3";
>>>
>>>
>>>
>>> What is the reason for SPI0 to have 4 pins, while SPI1 has just 3?
>>>
>>
>> I should align SPI1 with SPI0.
>>
>
> Are you sure that SPI0 is the correct one? SPI usually uses four pins - SDI,
> SDO, SCK and nCS, but we always used to treat nCS as a simple GPIO, due to
> the fact that the controller can only support one dedicated chip select and
> with direct GPIO control you can have more.
>
> What is the fourth pin here?
>
>
>>>
>>>> +               cpu@1 {
>>>> +                       device_type = "cpu";
>>>> +                       compatible = "arm,cortex-a15";
>>>> +                       reg = <1>;
>>>> +                       cci-control-port = <&cci_control1>;
>>>> +               };
>>>> +               cpu@100 {
>>>> +                       device_type = "cpu";
>>>> +                       compatible = "arm,cortex-a7";
>>>> +                       reg = <0x100>;
>>>> +                       cci-control-port = <&cci_control0>;
>>>> +               };
>>>> +               cpu@101 {
>>>> +                       device_type = "cpu";
>>>> +                       compatible = "arm,cortex-a7";
>>>> +                       reg = <0x101>;
>>>> +                       cci-control-port = <&cci_control0>;
>>>> +               };
>>>> +               cpu@102 {
>>>> +                       device_type = "cpu";
>>>> +                       compatible = "arm,cortex-a7";
>>>> +                       reg = <0x102>;
>>>> +                       cci-control-port = <&cci_control0>;
>>>> +               };
>>>> +               cpu@103 {
>>>> +                       device_type = "cpu";
>>>> +                       compatible = "arm,cortex-a7";
>>>> +                       reg = <0x103>;
>>>> +                       cci-control-port = <&cci_control0>;
>>>> +               };
>>>> +       };
>>>> +
>>>> +       cmus {
>>>> +               #address-cells = <1>;
>>>> +               #size-cells = <1>;
>>>> +               ranges;
>>>> +
>>>
>>>
>>>
>>> I don't think there is a need to group these nodes under a parent node
>>> that
>>> doesn't give any additional information, especially when the CMUs are
>>> scattered trough the whole address space, while we'd like to keep the
>>> nodes
>>> ordered by their addresses, as most platforms do.
>>>
>>
>> This is exactly the same case as "cpus". I mean, "cpus" also doesn't
>> provide
>> any common information about child cpu nodes. This looks to me as a
>> logical
>> grouping and I have implemented same thing for cmu nodes.
>> I am ok with removing this grouping Just want to understand the rational
>> behind
>> grouping cpus which seems similar to cmus.
>
>
> The "cpus" node is a defined standard node that should be present at root of
> device tree and include subnodes for all CPUs. This is a standard binding
> defined for low level code to be able to simply find nodes of all CPUs in
> the system - so they can expect that at /cpus node all the subnodes are
> subsequent CPUs.
>
>
>> Similarly "soc" is just a logical entity used to group SoC elements which
>> looks
>> optional to me. What are we achieving with this? Please help me in
>> understanding
>> this better.
>
>
> Also "soc" has a slightly wider meaning. It is a node grouping all nodes
> from a single address space - the node specifies #address-cells and
> #size-cells of this address space and all the devices under this
> "simple-bus" can be accessed using addresses in this format. In addition, it
> separates board-level devices from generic SoC devices.
>
> Now, in case of "cmus", the only purpose is to group all CMU nodes together
> and, while this improves readability a bit, it doesn't make the DT better
> express the hardware topology, because the CMUs in the hardware are in fact
> scattered through the whole address space, not under a contiguous block of
> it, as the grouping would suggest.
>
> Best regards,
> Tomasz

Re: [PATCH V2 2/3] ARM: dts: add dts files for exynos5260 SoC

[Rahul Sharma]

On 14 February 2014 08:54, Rahul Sharma <r.sh.open@gmail.com> wrote:
> Thanks Tomasz,
>
> I will add these changes to v3.
>
> Regards,
> Rahul Sharma.
>
> On 11 February 2014 15:34, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> Hi Rahul,
>>
>>
>> On 11.02.2014 06:22, Rahul Sharma wrote:
>>>
>>> Hi Tomasz,
>>>
>>> On 6 February 2014 18:51, Tomasz Figa <t.figa@samsung.com> wrote:
>>>>
>>>> Hi Rahul, Pankaj, Arun,
>>>>
>>>> [adding linux-arm-kernel, devicetree MLs and DT people on Cc]
>>>>
>>>> I think it's good time to stop accepting DTS files like this and force
>>>> new
>>>> ones to use the proper structure with soc node, labels for every node and
>>>> node references.
>>>
>>>
>>> I am unable to find information on SoC node and grouping inside SoC node.
>>> Please
>>> share some pointers.
>>
>>
>> Well, there is not much information needed about this. Basically all the
>> devices built into the SoC should be listed under respective bus nodes or a
>> single soc node, instead of root level. Such node should be a "simple-bus"
>> and just group the components together to separate board-specific devices
>> (which are still at root level) from SoC devices.
>>
>> Even though it might seem useless, it improves DT readability a bit and
>> still most of the platforms use this approach, so for consistency, Exynos
>> should use too.
>>
>> Just for reference, back in April 2013, in his review of S3C64xx DT series
>> [1], Rob Herring requested that we don't submit any new device trees using
>> flat approach and start using bus hierarchy.
>>
>> [1]
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/163659.html
>>
>>
>>>>
>>>>> +               spi0_bus: spi0-bus {
>>>>> +                       samsung,pins = "gpa2-0", "gpa2-1", "gpa2-2",
>>>>> "gpa2-3";
>>>>
>>>>
>>>>
>>>> What is the reason for SPI0 to have 4 pins, while SPI1 has just 3?
>>>>
>>>
>>> I should align SPI1 with SPI0.
>>>
>>
>> Are you sure that SPI0 is the correct one? SPI usually uses four pins - SDI,
>> SDO, SCK and nCS, but we always used to treat nCS as a simple GPIO, due to
>> the fact that the controller can only support one dedicated chip select and
>> with direct GPIO control you can have more.
>>
>> What is the fourth pin here?
>>

As you said, these are CLK, SS, MISO, MOSI (gpa2-0 to gpa2-3). Fourth pin is
CS. It can be defined as a simple GPIO but better to include it in the
SPI pingroup
as it belongs to there.

Regards,
Rahul Sharma.

>>
>>>>
>>>>> +               cpu@1 {
>>>>> +                       device_type = "cpu";
>>>>> +                       compatible = "arm,cortex-a15";
>>>>> +                       reg = <1>;
>>>>> +                       cci-control-port = <&cci_control1>;
>>>>> +               };
>>>>> +               cpu@100 {
>>>>> +                       device_type = "cpu";
>>>>> +                       compatible = "arm,cortex-a7";
>>>>> +                       reg = <0x100>;
>>>>> +                       cci-control-port = <&cci_control0>;
>>>>> +               };
>>>>> +               cpu@101 {
>>>>> +                       device_type = "cpu";
>>>>> +                       compatible = "arm,cortex-a7";
>>>>> +                       reg = <0x101>;
>>>>> +                       cci-control-port = <&cci_control0>;
>>>>> +               };
>>>>> +               cpu@102 {
>>>>> +                       device_type = "cpu";
>>>>> +                       compatible = "arm,cortex-a7";
>>>>> +                       reg = <0x102>;
>>>>> +                       cci-control-port = <&cci_control0>;
>>>>> +               };
>>>>> +               cpu@103 {
>>>>> +                       device_type = "cpu";
>>>>> +                       compatible = "arm,cortex-a7";
>>>>> +                       reg = <0x103>;
>>>>> +                       cci-control-port = <&cci_control0>;
>>>>> +               };
>>>>> +       };
>>>>> +
>>>>> +       cmus {
>>>>> +               #address-cells = <1>;
>>>>> +               #size-cells = <1>;
>>>>> +               ranges;
>>>>> +
>>>>
>>>>
>>>>
>>>> I don't think there is a need to group these nodes under a parent node
>>>> that
>>>> doesn't give any additional information, especially when the CMUs are
>>>> scattered trough the whole address space, while we'd like to keep the
>>>> nodes
>>>> ordered by their addresses, as most platforms do.
>>>>
>>>
>>> This is exactly the same case as "cpus". I mean, "cpus" also doesn't
>>> provide
>>> any common information about child cpu nodes. This looks to me as a
>>> logical
>>> grouping and I have implemented same thing for cmu nodes.
>>> I am ok with removing this grouping Just want to understand the rational
>>> behind
>>> grouping cpus which seems similar to cmus.
>>
>>
>> The "cpus" node is a defined standard node that should be present at root of
>> device tree and include subnodes for all CPUs. This is a standard binding
>> defined for low level code to be able to simply find nodes of all CPUs in
>> the system - so they can expect that at /cpus node all the subnodes are
>> subsequent CPUs.
>>
>>
>>> Similarly "soc" is just a logical entity used to group SoC elements which
>>> looks
>>> optional to me. What are we achieving with this? Please help me in
>>> understanding
>>> this better.
>>
>>
>> Also "soc" has a slightly wider meaning. It is a node grouping all nodes
>> from a single address space - the node specifies #address-cells and
>> #size-cells of this address space and all the devices under this
>> "simple-bus" can be accessed using addresses in this format. In addition, it
>> separates board-level devices from generic SoC devices.
>>
>> Now, in case of "cmus", the only purpose is to group all CMU nodes together
>> and, while this improves readability a bit, it doesn't make the DT better
>> express the hardware topology, because the CMUs in the hardware are in fact
>> scattered through the whole address space, not under a contiguous block of
>> it, as the grouping would suggest.
>>
>> Best regards,
>> Tomasz

Re: [PATCH V2 2/3] ARM: dts: add dts files for exynos5260 SoC

[Tomasz Figa]

On 14.02.2014 12:28, Rahul Sharma wrote:
> On 14 February 2014 08:54, Rahul Sharma <r.sh.open-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Thanks Tomasz,
>>
>> I will add these changes to v3.
>>
>> Regards,
>> Rahul Sharma.
>>
>> On 11 February 2014 15:34, Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> Hi Rahul,
>>>
>>>
>>> On 11.02.2014 06:22, Rahul Sharma wrote:
>>>>
>>>> Hi Tomasz,
>>>>
>>>> On 6 February 2014 18:51, Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>>>>>
>>>>> Hi Rahul, Pankaj, Arun,
>>>>>
>>>>> [adding linux-arm-kernel, devicetree MLs and DT people on Cc]
>>>>>
>>>>> I think it's good time to stop accepting DTS files like this and force
>>>>> new
>>>>> ones to use the proper structure with soc node, labels for every node and
>>>>> node references.
>>>>
>>>>
>>>> I am unable to find information on SoC node and grouping inside SoC node.
>>>> Please
>>>> share some pointers.
>>>
>>>
>>> Well, there is not much information needed about this. Basically all the
>>> devices built into the SoC should be listed under respective bus nodes or a
>>> single soc node, instead of root level. Such node should be a "simple-bus"
>>> and just group the components together to separate board-specific devices
>>> (which are still at root level) from SoC devices.
>>>
>>> Even though it might seem useless, it improves DT readability a bit and
>>> still most of the platforms use this approach, so for consistency, Exynos
>>> should use too.
>>>
>>> Just for reference, back in April 2013, in his review of S3C64xx DT series
>>> [1], Rob Herring requested that we don't submit any new device trees using
>>> flat approach and start using bus hierarchy.
>>>
>>> [1]
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/163659.html
>>>
>>>
>>>>>
>>>>>> +               spi0_bus: spi0-bus {
>>>>>> +                       samsung,pins = "gpa2-0", "gpa2-1", "gpa2-2",
>>>>>> "gpa2-3";
>>>>>
>>>>>
>>>>>
>>>>> What is the reason for SPI0 to have 4 pins, while SPI1 has just 3?
>>>>>
>>>>
>>>> I should align SPI1 with SPI0.
>>>>
>>>
>>> Are you sure that SPI0 is the correct one? SPI usually uses four pins - SDI,
>>> SDO, SCK and nCS, but we always used to treat nCS as a simple GPIO, due to
>>> the fact that the controller can only support one dedicated chip select and
>>> with direct GPIO control you can have more.
>>>
>>> What is the fourth pin here?
>>>
>
> As you said, these are CLK, SS, MISO, MOSI (gpa2-0 to gpa2-3). Fourth pin is
> CS. It can be defined as a simple GPIO but better to include it in the
> SPI pingroup
> as it belongs to there.

I have to disagree here.

If you define a pin in pinctrl group then it is tied to this specific 
hardware function and you can't use it as GPIO.

Since it's board specific to use it as HW chip select or GPIO chip 
select (the usual setup), it should not be included in this group.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html