Re: [PATCH 16/19] arm64: dts: exynos: Add dts files for 64-bit Exynos5433 SoC

2014-12-02 Thread Mark Rutland
On Mon, Dec 01, 2014 at 02:21:46AM +, Chanwoo Choi wrote:
> Dear Mark,
> 
> On 11/28/2014 11:00 PM, Mark Rutland wrote:
> > On Fri, Nov 28, 2014 at 01:18:25PM +, Chanwoo Choi wrote:
> >> Dear Mark,
> >>
> >> On 11/27/2014 08:18 PM, Mark Rutland wrote:
> >>> On Thu, Nov 27, 2014 at 07:35:13AM +, Chanwoo Choi wrote:
>  This patch adds new Exynos5433 dtsi to support 64-bit Exynos5433 SoC
>  based on Octal core CPUs (quad Cortex-A57 and quad Cortex-A53).
> 
>  Cc: Kukjin Kim 
>  Cc: Mark Rutland 
>  Cc: Arnd Bergmann 
>  Cc: Olof Johansson 
>  Cc: Catalin Marinas 
>  Cc: Will Deacon 
>  Signed-off-by: Chanwoo Choi 
>  Acked-by: Inki Dae 
>  Acked-by: Geunsik Lim 
>  ---
>   arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi | 698 
>  +
>   arch/arm64/boot/dts/exynos/exynos5433.dtsi | 523 +++
>   2 files changed, 1221 insertions(+)
>   create mode 100644 arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi
>   create mode 100644 arch/arm64/boot/dts/exynos/exynos5433.dtsi
> >>>
> >
> > [...]
> >
>  +   cpus {
>  +   #address-cells = <2>;
>  +   #size-cells = <0>;
>  +
>  +   cpu0: cpu@100 {
>  +   device_type = "cpu";
>  +   compatible = "arm,cortex-a53", "arm,armv8";
>  +   enable-method = "psci";
> >>>
> >>> While the CPU nodes have enable-methods, I didn't spot a PSCI node
> >>> anywhere, so this dts cannot possibly have been used to bring up an SMP
> >>> system.
> >>>
> >>> How has this dts been tested?
> >>>
> >>> What PSCI revision have you implemented? Have have you tested it?
> >>
> >> My mistake,
> >> Exynos5433 supports PSCI v0.1. I'll add following PSCI nodes:
> >> I tested the boot of secondary cpu.
> >>
> >> psci {
> >> compatible = "arm,psci";
> >> method = "smc";
> >> cpu_off = <0x8402>;
> >> cpu_on = <0xC403>;
> >> };
> >
> > Ok. I take it _any_ CPU may be hotplugged (including CPU0), given that
> > you don't have MIGRATE_INFO_TYPE from PSCI 0.2 to tell you that this is
> > not possible? If not, attempting to hotplug CPU0 will result in a BUG()
> > and the kernel will explode.
> >
> > Has that been tested?
> 
> I just tested secondary CPU on during kernel booting after added 'psci' dt 
> node.
> So, I got the ON state of Octa CPUs.
> 
> Maybe I need more time to implement CPU0 and secondary cpu hotplugged 
> dynamically on runtime.

So currently PSCI CPU_OFF is not implemented at all?

> > Do all CPUs enter the kernel at EL2?
> 
> I didn't consider EL2 for hypervisor mode.
> First role of this job, I'll implement CPU on/off and suspend by using PSCI.

Is there any reason not to enter the kernel at EL2?

PSCI 0.2 mandates entering at EL2 if present (and not under a
hypervisor), and it gives the kernel a lot more flexibility to fix
things up (and there's less for FW to restore) even when a hypervisor is
not in use.

Implementing all that to EL2 is _simpler_ than implementing it to EL1.
The kernel will restore what it needs to.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 16/19] arm64: dts: exynos: Add dts files for 64-bit Exynos5433 SoC

2014-11-30 Thread Chanwoo Choi
Dear Mark,

On 11/28/2014 11:00 PM, Mark Rutland wrote:
> On Fri, Nov 28, 2014 at 01:18:25PM +, Chanwoo Choi wrote:
>> Dear Mark,
>>
>> On 11/27/2014 08:18 PM, Mark Rutland wrote:
>>> On Thu, Nov 27, 2014 at 07:35:13AM +, Chanwoo Choi wrote:
 This patch adds new Exynos5433 dtsi to support 64-bit Exynos5433 SoC
 based on Octal core CPUs (quad Cortex-A57 and quad Cortex-A53).

 Cc: Kukjin Kim 
 Cc: Mark Rutland 
 Cc: Arnd Bergmann 
 Cc: Olof Johansson 
 Cc: Catalin Marinas 
 Cc: Will Deacon 
 Signed-off-by: Chanwoo Choi 
 Acked-by: Inki Dae 
 Acked-by: Geunsik Lim 
 ---
  arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi | 698 
 +
  arch/arm64/boot/dts/exynos/exynos5433.dtsi | 523 +++
  2 files changed, 1221 insertions(+)
  create mode 100644 arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi
  create mode 100644 arch/arm64/boot/dts/exynos/exynos5433.dtsi
>>>
> 
> [...]
> 
 +   cpus {
 +   #address-cells = <2>;
 +   #size-cells = <0>;
 +
 +   cpu0: cpu@100 {
 +   device_type = "cpu";
 +   compatible = "arm,cortex-a53", "arm,armv8";
 +   enable-method = "psci";
>>>
>>> While the CPU nodes have enable-methods, I didn't spot a PSCI node
>>> anywhere, so this dts cannot possibly have been used to bring up an SMP
>>> system.
>>>
>>> How has this dts been tested?
>>>
>>> What PSCI revision have you implemented? Have have you tested it?
>>
>> My mistake,
>> Exynos5433 supports PSCI v0.1. I'll add following PSCI nodes:
>> I tested the boot of secondary cpu.
>>
>> psci {
>> compatible = "arm,psci";
>> method = "smc";
>> cpu_off = <0x8402>;
>> cpu_on = <0xC403>;
>> };
> 
> Ok. I take it _any_ CPU may be hotplugged (including CPU0), given that
> you don't have MIGRATE_INFO_TYPE from PSCI 0.2 to tell you that this is
> not possible? If not, attempting to hotplug CPU0 will result in a BUG()
> and the kernel will explode.
> 
> Has that been tested? 

I just tested secondary CPU on during kernel booting after added 'psci' dt node.
So, I got the ON state of Octa CPUs.

Maybe I need more time to implement CPU0 and secondary cpu hotplugged 
dynamically on runtime.

> 
> Do all CPUs enter the kernel at EL2?

I didn't consider EL2 for hypervisor mode.
First role of this job, I'll implement CPU on/off and suspend by using PSCI.

> 
 +   soc: soc {
 +   compatible = "simple-bus";
 +   #address-cells = <1>;
 +   #size-cells = <1>;
 +   ranges;
 +
 +   fixed-rate-clocks {
 +   #address-cells = <1>;
 +   #size-cells = <0>;
 +
 +   xusbxti: clock@0 {
 +   compatible = "fixed-clock";
 +   clock-output-names = "xusbxti";
 +   #clock-cells = <0>;
 +   };
 +   };
>>>
>>> Get rid of the fixed-rate-clocks container node. It's pointless and
>>> messy. Given you only have one there's no need for the bogus
>>> unit-address either.
>>
>> OK, I'll remove unneeded code and will add following dt node for fin_pll.
>>
>> fin_pll: xxti {
>> compatible = "fixed-clock";
>> clock-output-names = "fin_pll";
>> #clock-cells = <0>;
>> };
> 
> That looks fine to me.
> 
> [...]
> 
 +   mct@101c {
 +   compatible = "samsung,exynos4210-mct";
 +   reg = <0x101c 0x800>;
 +   interrupts = <0 102 0>, <0 103 0>, <0 104 0>, <0 
 105 0>,
 +   <0 106 0>, <0 107 0>, <0 108 0>, <0 109>,
 +   <0 110 0>, <0 111 0>, <0 112 0>, <0 113 0>;
 +   clocks = <&cmu_top CLK_FIN_PLL>, <&cmu_peris 
 CLK_PCLK_MCT>;
 +   clock-names = "fin_pll", "mct";
 +   };
>>>
>>> Hase this block had no changes whatsoever since its use in Exynos4210?
>>> Do we not need a "samsung,exynos5433-mct" comaptible string too?
>>
>> The type of Exynos5433's MCT(Multi-Core Timer) IP is the same with the type 
>> of Exynos4210 MCT.
>> Just Exynos5433 have eight local timer for Octa cores.
> 
> So "samsung,exynos4210-mct" should appear in the list. I'm just
> wondering if it's worth having:
> 
>   compatible = "samsung,exynos5433-mct", "samsung,exynos4210-mct";
> 
> Just in case we need to special-case the 5433 MCT for some reason later.

OK, I'll add "samsung,exynos5433-mct" compatible string in exynos5433.dtsi
according to your comment.

> 
>>
>>CPU0   CPU1 

Re: [PATCH 16/19] arm64: dts: exynos: Add dts files for 64-bit Exynos5433 SoC

2014-11-28 Thread Mark Rutland
On Fri, Nov 28, 2014 at 01:18:25PM +, Chanwoo Choi wrote:
> Dear Mark,
> 
> On 11/27/2014 08:18 PM, Mark Rutland wrote:
> > On Thu, Nov 27, 2014 at 07:35:13AM +, Chanwoo Choi wrote:
> >> This patch adds new Exynos5433 dtsi to support 64-bit Exynos5433 SoC
> >> based on Octal core CPUs (quad Cortex-A57 and quad Cortex-A53).
> >>
> >> Cc: Kukjin Kim 
> >> Cc: Mark Rutland 
> >> Cc: Arnd Bergmann 
> >> Cc: Olof Johansson 
> >> Cc: Catalin Marinas 
> >> Cc: Will Deacon 
> >> Signed-off-by: Chanwoo Choi 
> >> Acked-by: Inki Dae 
> >> Acked-by: Geunsik Lim 
> >> ---
> >>  arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi | 698 
> >> +
> >>  arch/arm64/boot/dts/exynos/exynos5433.dtsi | 523 +++
> >>  2 files changed, 1221 insertions(+)
> >>  create mode 100644 arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi
> >>  create mode 100644 arch/arm64/boot/dts/exynos/exynos5433.dtsi
> >

[...]

> >> +   cpus {
> >> +   #address-cells = <2>;
> >> +   #size-cells = <0>;
> >> +
> >> +   cpu0: cpu@100 {
> >> +   device_type = "cpu";
> >> +   compatible = "arm,cortex-a53", "arm,armv8";
> >> +   enable-method = "psci";
> >
> > While the CPU nodes have enable-methods, I didn't spot a PSCI node
> > anywhere, so this dts cannot possibly have been used to bring up an SMP
> > system.
> >
> > How has this dts been tested?
> >
> > What PSCI revision have you implemented? Have have you tested it?
> 
> My mistake,
> Exynos5433 supports PSCI v0.1. I'll add following PSCI nodes:
> I tested the boot of secondary cpu.
> 
> psci {
> compatible = "arm,psci";
> method = "smc";
> cpu_off = <0x8402>;
> cpu_on = <0xC403>;
> };

Ok. I take it _any_ CPU may be hotplugged (including CPU0), given that
you don't have MIGRATE_INFO_TYPE from PSCI 0.2 to tell you that this is
not possible? If not, attempting to hotplug CPU0 will result in a BUG()
and the kernel will explode.

Has that been tested? 

Do all CPUs enter the kernel at EL2?

> >> +   soc: soc {
> >> +   compatible = "simple-bus";
> >> +   #address-cells = <1>;
> >> +   #size-cells = <1>;
> >> +   ranges;
> >> +
> >> +   fixed-rate-clocks {
> >> +   #address-cells = <1>;
> >> +   #size-cells = <0>;
> >> +
> >> +   xusbxti: clock@0 {
> >> +   compatible = "fixed-clock";
> >> +   clock-output-names = "xusbxti";
> >> +   #clock-cells = <0>;
> >> +   };
> >> +   };
> >
> > Get rid of the fixed-rate-clocks container node. It's pointless and
> > messy. Given you only have one there's no need for the bogus
> > unit-address either.
> 
> OK, I'll remove unneeded code and will add following dt node for fin_pll.
> 
> fin_pll: xxti {
> compatible = "fixed-clock";
> clock-output-names = "fin_pll";
> #clock-cells = <0>;
> };

That looks fine to me.

[...]

> >> +   mct@101c {
> >> +   compatible = "samsung,exynos4210-mct";
> >> +   reg = <0x101c 0x800>;
> >> +   interrupts = <0 102 0>, <0 103 0>, <0 104 0>, <0 
> >> 105 0>,
> >> +   <0 106 0>, <0 107 0>, <0 108 0>, <0 109>,
> >> +   <0 110 0>, <0 111 0>, <0 112 0>, <0 113 0>;
> >> +   clocks = <&cmu_top CLK_FIN_PLL>, <&cmu_peris 
> >> CLK_PCLK_MCT>;
> >> +   clock-names = "fin_pll", "mct";
> >> +   };
> >
> > Hase this block had no changes whatsoever since its use in Exynos4210?
> > Do we not need a "samsung,exynos5433-mct" comaptible string too?
> 
> The type of Exynos5433's MCT(Multi-Core Timer) IP is the same with the type 
> of Exynos4210 MCT.
> Just Exynos5433 have eight local timer for Octa cores.

So "samsung,exynos4210-mct" should appear in the list. I'm just
wondering if it's worth having:

compatible = "samsung,exynos5433-mct", "samsung,exynos4210-mct";

Just in case we need to special-case the 5433 MCT for some reason later.

> 
>CPU0   CPU1   CPU2   CPU3   CPU4   CPU5   
> CPU6   CPU7
> 134:  0  0  0  0  0  0
>   0  0   GIC 134  mct_comp_irq
> 138:   3189  0  0  0  0  0
>   0  0   GIC 138  mct_tick0
> 139:  0   2670  0  0  0  0
>   0  0   GIC 139  mct_tick1
> 140:  0  0   2763  0  0  0
>   0  0   GIC 140  mct_tick2
> 141: 

Re: [PATCH 16/19] arm64: dts: exynos: Add dts files for 64-bit Exynos5433 SoC

2014-11-28 Thread Chanwoo Choi
Dear Marc,

On 11/27/2014 07:26 PM, Marc Zyngier wrote:
> On 27/11/14 07:35, Chanwoo Choi wrote:
>> This patch adds new Exynos5433 dtsi to support 64-bit Exynos5433 SoC
>> based on Octal core CPUs (quad Cortex-A57 and quad Cortex-A53).
>>
>> Cc: Kukjin Kim 
>> Cc: Mark Rutland 
>> Cc: Arnd Bergmann 
>> Cc: Olof Johansson 
>> Cc: Catalin Marinas 
>> Cc: Will Deacon 
>> Signed-off-by: Chanwoo Choi 
>> Acked-by: Inki Dae 
>> Acked-by: Geunsik Lim 
>> ---
>>  arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi | 698 
>> +
>>  arch/arm64/boot/dts/exynos/exynos5433.dtsi | 523 +++
>>  2 files changed, 1221 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi
>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos5433.dtsi
>>
> 
> [...]
> 
>> diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi 
>> b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
>> new file mode 100644
>> index 000..3d8b576
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> 
> [...]
> 
>> +   timer {
>> +   compatible = "arm,armv8-timer";
>> +   interrupts = <1 13 0xff01>,
>> +<1 14 0xff01>,
>> +<1 11 0xff01>,
>> +<1 10 0xff01>;
> 
> This is wrong. Timer interrupts for both A53 and A57 are level triggered.

I'll fix it level triggering instead of edge triggering.

If possible, could you give the document url to check the correct type of level 
trigger?
whether irq is high level trigger or low level trigger.

> 
>> +   clock-frequency = <2400>;
> 
> Please go and fix your firmware. Really...
> 
>> +   use-clocksource-only;
>> +   use-physical-timer;
>> +   };
> 
> Well, that's a total NAK. Neither of these properties are part of the
> binding, and we've already established that none of that would never be
> valid on arm64.
> 
> I suggest you finally do what we've been asking for years, which is to
> fix your boot ROM by adding the 5 lines of assembly code that are needed
> instead of repeatedly post the same bogus DT files.

I'll remove last three properties.

Best Regards,
Chanwoo Choi


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 16/19] arm64: dts: exynos: Add dts files for 64-bit Exynos5433 SoC

2014-11-28 Thread Chanwoo Choi
Dear Mark,

On 11/27/2014 08:18 PM, Mark Rutland wrote:
> On Thu, Nov 27, 2014 at 07:35:13AM +, Chanwoo Choi wrote:
>> This patch adds new Exynos5433 dtsi to support 64-bit Exynos5433 SoC
>> based on Octal core CPUs (quad Cortex-A57 and quad Cortex-A53).
>>
>> Cc: Kukjin Kim 
>> Cc: Mark Rutland 
>> Cc: Arnd Bergmann 
>> Cc: Olof Johansson 
>> Cc: Catalin Marinas 
>> Cc: Will Deacon 
>> Signed-off-by: Chanwoo Choi 
>> Acked-by: Inki Dae 
>> Acked-by: Geunsik Lim 
>> ---
>>  arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi | 698 
>> +
>>  arch/arm64/boot/dts/exynos/exynos5433.dtsi | 523 +++
>>  2 files changed, 1221 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi
>>  create mode 100644 arch/arm64/boot/dts/exynos/exynos5433.dtsi
> 
> [...]
> 
>> diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi 
>> b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
>> new file mode 100644
>> index 000..3d8b576
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
>> @@ -0,0 +1,523 @@
>> +/*
>> + * Samsung's Exynos5433 SoC device tree source
>> + *
>> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
>> + * http://www.samsung.com
>> + *
>> + * Samsung's Exynos5433 SoC device nodes are listed in this file. Exynos5433
>> + * based board files can include this file and provide values for board 
>> specfic
>> + * bindings.
>> + *
>> + * Note: This file does not include device nodes for all the controllers in
>> + * Exynos5433 SoC. As device tree coverage for Exynos5433 increases, 
>> additional
>> + * nodes can be added to 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.
>> + */
>> +
>> +#include "skeleton.dtsi"
>> +#include 
>> +
> 
> Just to check: no memory reservations required for any reason?
> 
> There also don't appear to be any memory nodes. Typically if that's
> filled in by the bootloader/FW we'd have an empty node (or one with a
> zero size entry) and a comment regarding the FW.

I add the memory node to board dtsi file because memory information
is more dependent on on h/w target than SoC.

> 
>> +/ {
>> +   compatible = "samsung,exynos5433";
>> +   #address-cells = <1>;
>> +   #size-cells = <1>;
> 
> Not two, on both counts? The CPUs can address more than 32 bits.

You're right. I'll fix it as two and then retry to test it.

> 
> Is there nothing in the physical address space above 0x?
> 
> [...]
> 
>> +   cpus {
>> +   #address-cells = <2>;
>> +   #size-cells = <0>;
>> +
>> +   cpu0: cpu@100 {
>> +   device_type = "cpu";
>> +   compatible = "arm,cortex-a53", "arm,armv8";
>> +   enable-method = "psci";
> 
> While the CPU nodes have enable-methods, I didn't spot a PSCI node
> anywhere, so this dts cannot possibly have been used to bring up an SMP
> system.
> 
> How has this dts been tested?
> 
> What PSCI revision have you implemented? Have have you tested it?

My mistake,
Exynos5433 supports PSCI v0.1. I'll add following PSCI nodes: 
I tested the boot of secondary cpu.

psci {
compatible = "arm,psci";
method = "smc";
cpu_off = <0x8402>;
cpu_on = <0xC403>;
};

> 
> I take it from the presence of GICH/GICV in the gic node that CPUs enter
> the kernel at EL2?
> 
>> +   reg = <0x0 0x100>;
>> +   clock-frequency = <105000>;
> 
> What uses this?

It is un-used. I'll drop it.

> 
>> +   };
> 
> [...]
> 
>> +   soc: soc {
>> +   compatible = "simple-bus";
>> +   #address-cells = <1>;
>> +   #size-cells = <1>;
>> +   ranges;
>> +
>> +   fixed-rate-clocks {
>> +   #address-cells = <1>;
>> +   #size-cells = <0>;
>> +
>> +   xusbxti: clock@0 {
>> +   compatible = "fixed-clock";
>> +   clock-output-names = "xusbxti";
>> +   #clock-cells = <0>;
>> +   };
>> +   };
> 
> Get rid of the fixed-rate-clocks container node. It's pointless and
> messy. Given you only have one there's no need for the bogus
> unit-address either.

OK, I'll remove unneeded code and will add following dt node for fin_pll.

fin_pll: xxti {
compatible = "fixed-clock";
clock-output-names = "fin_pll";
#clock-cells = <0>;
};

> 
>> +
>> +   cmu_top: clock-controller@0x1003{
> 
> s/@0x/@/ -- a unit-address should not have the leading '0x'. Please
> apply that to the rest of the file.

I'll remove '0x'.

> 
>> 

Re: [PATCH 16/19] arm64: dts: exynos: Add dts files for 64-bit Exynos5433 SoC

2014-11-27 Thread Mark Rutland
On Thu, Nov 27, 2014 at 07:35:13AM +, Chanwoo Choi wrote:
> This patch adds new Exynos5433 dtsi to support 64-bit Exynos5433 SoC
> based on Octal core CPUs (quad Cortex-A57 and quad Cortex-A53).
>
> Cc: Kukjin Kim 
> Cc: Mark Rutland 
> Cc: Arnd Bergmann 
> Cc: Olof Johansson 
> Cc: Catalin Marinas 
> Cc: Will Deacon 
> Signed-off-by: Chanwoo Choi 
> Acked-by: Inki Dae 
> Acked-by: Geunsik Lim 
> ---
>  arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi | 698 
> +
>  arch/arm64/boot/dts/exynos/exynos5433.dtsi | 523 +++
>  2 files changed, 1221 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi
>  create mode 100644 arch/arm64/boot/dts/exynos/exynos5433.dtsi

[...]

> diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi 
> b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> new file mode 100644
> index 000..3d8b576
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> @@ -0,0 +1,523 @@
> +/*
> + * Samsung's Exynos5433 SoC device tree source
> + *
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * Samsung's Exynos5433 SoC device nodes are listed in this file. Exynos5433
> + * based board files can include this file and provide values for board 
> specfic
> + * bindings.
> + *
> + * Note: This file does not include device nodes for all the controllers in
> + * Exynos5433 SoC. As device tree coverage for Exynos5433 increases, 
> additional
> + * nodes can be added to 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.
> + */
> +
> +#include "skeleton.dtsi"
> +#include 
> +

Just to check: no memory reservations required for any reason?

There also don't appear to be any memory nodes. Typically if that's
filled in by the bootloader/FW we'd have an empty node (or one with a
zero size entry) and a comment regarding the FW.

> +/ {
> +   compatible = "samsung,exynos5433";
> +   #address-cells = <1>;
> +   #size-cells = <1>;

Not two, on both counts? The CPUs can address more than 32 bits.

Is there nothing in the physical address space above 0x?

[...]

> +   cpus {
> +   #address-cells = <2>;
> +   #size-cells = <0>;
> +
> +   cpu0: cpu@100 {
> +   device_type = "cpu";
> +   compatible = "arm,cortex-a53", "arm,armv8";
> +   enable-method = "psci";

While the CPU nodes have enable-methods, I didn't spot a PSCI node
anywhere, so this dts cannot possibly have been used to bring up an SMP
system.

How has this dts been tested?

What PSCI revision have you implemented? Have have you tested it?

I take it from the presence of GICH/GICV in the gic node that CPUs enter
the kernel at EL2?

> +   reg = <0x0 0x100>;
> +   clock-frequency = <105000>;

What uses this?

> +   };

[...]

> +   soc: soc {
> +   compatible = "simple-bus";
> +   #address-cells = <1>;
> +   #size-cells = <1>;
> +   ranges;
> +
> +   fixed-rate-clocks {
> +   #address-cells = <1>;
> +   #size-cells = <0>;
> +
> +   xusbxti: clock@0 {
> +   compatible = "fixed-clock";
> +   clock-output-names = "xusbxti";
> +   #clock-cells = <0>;
> +   };
> +   };

Get rid of the fixed-rate-clocks container node. It's pointless and
messy. Given you only have one there's no need for the bogus
unit-address either.

> +
> +   cmu_top: clock-controller@0x1003{

s/@0x/@/ -- a unit-address should not have the leading '0x'. Please
apply that to the rest of the file.

> +   compatible = "samsung,exynos5433-cmu-top";
> +   reg = <0x1003 0x0c04>;
> +   #clock-cells = <1>;
> +   };

[...]

> +   mct@101c {
> +   compatible = "samsung,exynos4210-mct";
> +   reg = <0x101c 0x800>;
> +   interrupts = <0 102 0>, <0 103 0>, <0 104 0>, <0 105 
> 0>,
> +   <0 106 0>, <0 107 0>, <0 108 0>, <0 109>,
> +   <0 110 0>, <0 111 0>, <0 112 0>, <0 113 0>;
> +   clocks = <&cmu_top CLK_FIN_PLL>, <&cmu_peris 
> CLK_PCLK_MCT>;
> +   clock-names = "fin_pll", "mct";
> +   };

Hase this block had no changes whatsoever since its use in Exynos4210?
Do we not need a "samsung,exynos5433-mct" comaptible string too?

> +
> +   gic:interrupt-controller@11001000 {
> +   compatible = "a

Re: [PATCH 16/19] arm64: dts: exynos: Add dts files for 64-bit Exynos5433 SoC

2014-11-27 Thread Marc Zyngier
On 27/11/14 07:35, Chanwoo Choi wrote:
> This patch adds new Exynos5433 dtsi to support 64-bit Exynos5433 SoC
> based on Octal core CPUs (quad Cortex-A57 and quad Cortex-A53).
> 
> Cc: Kukjin Kim 
> Cc: Mark Rutland 
> Cc: Arnd Bergmann 
> Cc: Olof Johansson 
> Cc: Catalin Marinas 
> Cc: Will Deacon 
> Signed-off-by: Chanwoo Choi 
> Acked-by: Inki Dae 
> Acked-by: Geunsik Lim 
> ---
>  arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi | 698 
> +
>  arch/arm64/boot/dts/exynos/exynos5433.dtsi | 523 +++
>  2 files changed, 1221 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi
>  create mode 100644 arch/arm64/boot/dts/exynos/exynos5433.dtsi
> 

[...]

> diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi 
> b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> new file mode 100644
> index 000..3d8b576
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi

[...]

> +   timer {
> +   compatible = "arm,armv8-timer";
> +   interrupts = <1 13 0xff01>,
> +<1 14 0xff01>,
> +<1 11 0xff01>,
> +<1 10 0xff01>;

This is wrong. Timer interrupts for both A53 and A57 are level triggered.

> +   clock-frequency = <2400>;

Please go and fix your firmware. Really...

> +   use-clocksource-only;
> +   use-physical-timer;
> +   };

Well, that's a total NAK. Neither of these properties are part of the
binding, and we've already established that none of that would never be
valid on arm64.

I suggest you finally do what we've been asking for years, which is to
fix your boot ROM by adding the 5 lines of assembly code that are needed
instead of repeatedly post the same bogus DT files.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 16/19] arm64: dts: exynos: Add dts files for 64-bit Exynos5433 SoC

2014-11-26 Thread Chanwoo Choi
This patch adds new Exynos5433 dtsi to support 64-bit Exynos5433 SoC
based on Octal core CPUs (quad Cortex-A57 and quad Cortex-A53).

Cc: Kukjin Kim 
Cc: Mark Rutland 
Cc: Arnd Bergmann 
Cc: Olof Johansson 
Cc: Catalin Marinas 
Cc: Will Deacon 
Signed-off-by: Chanwoo Choi 
Acked-by: Inki Dae 
Acked-by: Geunsik Lim 
---
 arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi | 698 +
 arch/arm64/boot/dts/exynos/exynos5433.dtsi | 523 +++
 2 files changed, 1221 insertions(+)
 create mode 100644 arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi
 create mode 100644 arch/arm64/boot/dts/exynos/exynos5433.dtsi

diff --git a/arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi 
b/arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi
new file mode 100644
index 000..81fe925
--- /dev/null
+++ b/arch/arm64/boot/dts/exynos/exynos5433-pinctrl.dtsi
@@ -0,0 +1,698 @@
+/*
+ * Samsung's Exynos5433 SoC pin-mux and pin-config device tree source
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * Samsung's Exynos5433 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_alive {
+   gpa0: gpa0 {
+   gpio-controller;
+   #gpio-cells = <2>;
+
+   interrupt-controller;
+   interrupt-parent = <&gic>;
+   interrupts = <0 0 0>, <0 1 0>, <0 2 0>, <0 3 0>,
+<0 4 0>, <0 5 0>, <0 6 0>, <0 7 0>;
+   #interrupt-cells = <2>;
+   };
+
+   gpa1: gpa1 {
+   gpio-controller;
+   #gpio-cells = <2>;
+
+   interrupt-controller;
+   interrupt-parent = <&gic>;
+   interrupts = <0 8 0>, <0 9 0>, <0 10 0>, <0 11 0>,
+<0 12 0>, <0 13 0>, <0 14 0>, <0 15 0>;
+   #interrupt-cells = <2>;
+   };
+
+   gpa2: gpa2 {
+   gpio-controller;
+   #gpio-cells = <2>;
+
+   interrupt-controller;
+   #interrupt-cells = <2>;
+   };
+
+   gpa3: gpa3 {
+   gpio-controller;
+   #gpio-cells = <2>;
+
+   interrupt-controller;
+   #interrupt-cells = <2>;
+   };
+};
+
+&pinctrl_aud {
+   gpz0: gpz0 {
+   gpio-controller;
+   #gpio-cells = <2>;
+
+   interrupt-controller;
+   #interrupt-cells = <2>;
+   };
+
+   gpz1: gpz1 {
+   gpio-controller;
+   #gpio-cells = <2>;
+
+   interrupt-controller;
+   #interrupt-cells = <2>;
+   };
+
+   i2s0_bus: i2s0-bus {
+   samsung,pins = "gpz0-0", "gpz0-1", "gpz0-2", "gpz0-3",
+   "gpz0-4", "gpz0-5", "gpz0-6";
+   samsung,pin-function = <2>;
+   samsung,pin-pud = <1>;
+   samsung,pin-drv = <0>;
+   };
+
+   pcm0_bus: pcm0-bus {
+   samsung,pins = "gpz1-0", "gpz1-1", "gpz1-2", "gpz1-3";
+   samsung,pin-function = <3>;
+   samsung,pin-pud = <1>;
+   samsung,pin-drv = <0>;
+   };
+};
+
+&pinctrl_cpif {
+   gpv6: gpv6 {
+   gpio-controller;
+   #gpio-cells = <2>;
+
+   interrupt-controller;
+   #interrupt-cells = <2>;
+   };
+};
+
+&pinctrl_ese {
+   gpj2: gpj2 {
+   gpio-controller;
+   #gpio-cells = <2>;
+
+   interrupt-controller;
+   #interrupt-cells = <2>;
+   };
+};
+
+&pinctrl_finger {
+   gpd5: gpd5 {
+   gpio-controller;
+   #gpio-cells = <2>;
+
+   interrupt-controller;
+   #interrupt-cells = <2>;
+   };
+
+   spi2_bus: spi2-bus {
+   samsung,pins = "gpd5-0", "gpd5-2", "gpd5-3";
+   samsung,pin-function = <2>;
+   samsung,pin-pud = <3>;
+   samsung,pin-drv = <0>;
+   };
+
+   hs_i2c6_bus: hs-i2c6-bus {
+   samsung,pins = "gpd5-3", "gpd5-2";
+   samsung,pin-function = <4>;
+   samsung,pin-pud = <3>;
+   samsung,pin-drv = <0>;
+   };
+
+};
+
+&pinctrl_fsys {
+   gph1: gph1 {
+   gpio-controller;
+   #gpio-cells = <2>;
+
+   interrupt-controller;
+   #interrupt-cells = <2>;
+   };
+
+   gpr4: gpr4 {
+   gpio-controller;
+   #gpio-cells = <2>;
+
+   interrupt-controller;
+   #interrupt-cells = <2>;
+   };
+
+   gpr0: gpr0 {
+   gpio-controller;
+   #gpio-cells = <2>;
+
+   interrupt-controller;
+   #in